Harden dashboard API lifecycle races - #222
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughIntroduces a new ChangesDashboard Lifecycle Gating
Sequence Diagram(s)sequenceDiagram
participant Caller as Async Action<br/>(mutation, refresh)
participant captureDashboardLifecycle
participant AsyncOp as Async Operation<br/>(HTTP, spawn, timer)
participant isDashboardLifecycleCurrent
participant UI as UI Update Path<br/>(render, error, flash)
Caller->>captureDashboardLifecycle: capture(host, opts)
captureDashboardLifecycle-->>Caller: DashboardLifecycleToken
Caller->>AsyncOp: start operation
Note over Caller,AsyncOp: user may navigate away<br/>or advance inputEpoch
AsyncOp-->>Caller: resolve / reject
Caller->>isDashboardLifecycleCurrent: check(host, token)
alt lifecycle still current
isDashboardLifecycleCurrent-->>Caller: true
Caller->>UI: renderDashboard /<br/>showDashboardError /<br/>footerFlash
UI-->>Caller: UI updated
else lifecycle became stale
isDashboardLifecycleCurrent-->>Caller: false
Caller->>Caller: early return
Note over Caller: no UI update<br/>no stale ghost render
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Owner
Author
|
Sub-agent review fixes pushed in 8db3824:\n\n- Fixed runtime guard repair callbacks so internal busy/guard state is cleaned up even when the dashboard lifecycle is stale; stale UI errors/renders remain suppressed.\n- Fixed worktree create settlement so service polling is not cancelled by later dashboard input; only final UI feedback/focus is epoch-gated.\n\nVerification after fixes:\n- yarn vitest run src/multiplexer/dashboard-control.test.ts src/multiplexer/worktrees.test.ts\n- yarn typecheck\n- yarn lint\n- yarn vitest run\n- yarn build |
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/multiplexer/dashboard-control.ts (1)
268-302:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStale repair callbacks still persist
runtimeGuardRepairFailedKeybefore the lifecycle gate.At Line 300, the stale callback writes
runtimeGuardRepairFailedKeybefore the staleness check at Line 301. Because Line 268 short-circuits future repairs on that key, a stale failure can suppress later valid repair attempts in a new dashboard lifecycle.Suggested fix
const fail = (message: string) => { if (settled) return; settled = true; releaseRuntimeGuardRepairLock(lockPath); host.runtimeGuardRepairing = false; - host.runtimeGuardRepairFailedKey = repairKey; if (!isDashboardLifecycleCurrent(host, lifecycle)) return; + host.runtimeGuardRepairFailedKey = repairKey; showRuntimeGuardRepairFailure(host, "Aimux repair failed", message); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/multiplexer/dashboard-control.ts` around lines 268 - 302, The fail callback function is assigning host.runtimeGuardRepairFailedKey before the lifecycle staleness check at isDashboardLifecycleCurrent, causing stale callbacks to persist state that blocks future repairs. Move the line host.runtimeGuardRepairFailedKey = repairKey to occur after the if (!isDashboardLifecycleCurrent(host, lifecycle)) return check so that only callbacks from the current lifecycle can update this state and affect future repair attempts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/multiplexer/archives.ts`:
- Line 161: The lifecycle guard on line 161 in the isDashboardLifecycleCurrent
check is using the current host.mode instead of the origin mode from the
captured lifecycle token. Change the condition from checking host.mode ===
"dashboard" to checking lifecycle.mode === "dashboard" to ensure the gate is
based on the operation's original mode, not the current mode. Apply the same fix
to line 179 to ensure consistent lifecycle mode checking across both locations
and prevent stale-flow leaks and improper error suppression.
In `@src/multiplexer/dashboard-lifecycle.ts`:
- Around line 26-33: The lifecycle check function implements fail-open behavior
when host capabilities are missing, which bypasses the gating that callers opted
into. When token.screen is defined (indicating the caller requires screen
gating), the function should return false if the host lacks the capability to
verify it, rather than defaulting to true. Refactor the screen validation logic
in this function to ensure that if token.screen is provided and the host cannot
be verified to match that screen (via host.isDashboardScreen function,
host.dashboardState?.isScreen method, or host.dashboardState?.screen property),
the check returns false instead of true, ensuring fail-closed behavior for
opted-in guards.
- Around line 1-41: The source file changes to captureDashboardLifecycle,
isDashboardLifecycleCurrent, and renderDashboardIfCurrent functions modify
runtime behavior but the dist/ directory has not been regenerated. Per
repository policy, runtime behavior changes in src/**/*.ts files require
regenerating the compiled TypeScript output in dist/ and committing it alongside
source changes. Run yarn build to regenerate the dist/ directory output from
these TypeScript source modifications, then include the generated dist/ files in
the PR commit.
In `@src/multiplexer/project-event-stream.ts`:
- Around line 50-51: The await call to
resolveCurrentProjectServiceEndpointForDashboard lacks error handling and can
crash the entire dashboard stream loop if endpoint resolution fails during
control-plane operations. Wrap the endpoint resolution await call in a try/catch
block to gracefully handle any rejections from
resolveCurrentProjectServiceEndpointForDashboard, allowing the loop to recover
and retry instead of terminating.
In `@src/multiplexer/subscreens.ts`:
- Around line 51-56: The lifecycle token captured by captureDashboardLifecycle
in the block containing the refreshCoordinationFromService logic does not
include the screen property, allowing the lifecycle check to pass even when the
dashboard screen changes while the input epoch remains the same. Add the screen
property to the options object passed to captureDashboardLifecycle (alongside
inputEpoch: true) so that the subsequent isDashboardLifecycleCurrent check can
properly detect screen changes and prevent the async continuation from executing
coordination side effects on outdated screen contexts.
---
Outside diff comments:
In `@src/multiplexer/dashboard-control.ts`:
- Around line 268-302: The fail callback function is assigning
host.runtimeGuardRepairFailedKey before the lifecycle staleness check at
isDashboardLifecycleCurrent, causing stale callbacks to persist state that
blocks future repairs. Move the line host.runtimeGuardRepairFailedKey =
repairKey to occur after the if (!isDashboardLifecycleCurrent(host, lifecycle))
return check so that only callbacks from the current lifecycle can update this
state and affect future repair attempts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a74c2691-71c7-4087-96d3-788550039cb7
📒 Files selected for processing (19)
src/multiplexer/archives.test.tssrc/multiplexer/archives.tssrc/multiplexer/coordination.tssrc/multiplexer/dashboard-control.test.tssrc/multiplexer/dashboard-control.tssrc/multiplexer/dashboard-lifecycle.tssrc/multiplexer/dashboard-ops.test.tssrc/multiplexer/dashboard-ops.tssrc/multiplexer/dashboard-view-methods.test.tssrc/multiplexer/dashboard-view-methods.tssrc/multiplexer/notifications.tssrc/multiplexer/project-event-stream.test.tssrc/multiplexer/project-event-stream.tssrc/multiplexer/runtime-state.test.tssrc/multiplexer/runtime-state.tssrc/multiplexer/subscreens.test.tssrc/multiplexer/subscreens.tssrc/multiplexer/worktrees.test.tssrc/multiplexer/worktrees.ts
Owner
Author
|
Second sub-agent review fixes pushed in 65c6f37:\n\n- Split settlement lifecycle from UI lifecycle for worktree graveyard and pending-create settlement. Later dashboard input no longer prematurely clears pending creates/removals.\n- Strengthened project-service endpoint validation to require the health response projectStateDir to match the current project.\n- Kept non-dashboard graveyard resurrection failures visible while guarding dashboard graveyard delete/resurrect UI after stale navigation.\n\nVerification after fixes:\n- yarn vitest run src/multiplexer/dashboard-control.test.ts src/multiplexer/worktrees.test.ts src/multiplexer/dashboard-view-methods.test.ts src/multiplexer/archives.test.ts\n- yarn typecheck\n- yarn lint\n- yarn vitest run\n- yarn build |
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Owner
Author
|
Sub-agent orphan finding resolved in 0ded6b8: graveyard worktree delete now mirrors resurrection stale-snapshot recovery by forcing a graveyard refresh and dashboard model refresh after reporting the error. The other sub-agent finding overlapped the CodeRabbit SSE endpoint-resolution thread fixed in the same commit. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/multiplexer/worktrees.test.ts (2)
641-644: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAssert stale removal-job cleanup directly.
Please add
expect(host.worktreeRemovalJob).toBeNull()in this stale-input case so the test also protects the.finallylifecycle cleanup path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/multiplexer/worktrees.test.ts` around lines 641 - 644, The test case for the stale-input scenario is missing an assertion to verify that the worktree removal job cleanup is properly executed. Add an expect statement after the existing assertions in this test block to verify that host.worktreeRemovalJob is null, ensuring the .finally lifecycle cleanup path is protected and properly nullifies the removal job reference.
204-212: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winStrengthen stale-epoch create assertions to validate UI suppression explicitly.
This case proves settlement continues, but it doesn’t explicitly verify that stale post-settle UI updates are suppressed after
dashboardInputEpochchanges.Proposed assertion update
handleWorktreeInputKey(host, Buffer.from("\r")); await vi.waitFor(() => expect(postToProjectService).toHaveBeenCalledOnce()); + host.dashboardState.focusedWorktreePath = "/repo/.aimux/worktrees/existing"; host.dashboardInputEpoch = 1; resolveCreate(); await vi.waitFor(() => expect(pending.state.get("worktree:/repo/.aimux/worktrees/demo")).toBeNull()); expect(host.dashboardWorktreeGroupsCache[0]).toMatchObject({ name: "demo", branch: "demo" }); - expect(host.dashboardState.focusedWorktreePath).toBe("/repo/.aimux/worktrees/demo"); + expect(host.dashboardState.focusedWorktreePath).toBe("/repo/.aimux/worktrees/existing"); + expect(host.dashboardUiStateStore.markSelectionDirty).not.toHaveBeenCalled(); expect(host.showDashboardError).not.toHaveBeenCalled();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/multiplexer/worktrees.test.ts` around lines 204 - 212, The test proves that settlement continues after the dashboardInputEpoch changes, but it does not explicitly verify that stale UI updates are suppressed. Add explicit assertions between the dashboardInputEpoch assignment and after resolveCreate() that validate UI update methods (such as dashboard state modifications or UI callback functions) are not invoked by the stale post-settle operations, ensuring that the epoch change successfully suppresses outdated updates from completing promises.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/multiplexer/worktrees.test.ts`:
- Around line 641-644: The test case for the stale-input scenario is missing an
assertion to verify that the worktree removal job cleanup is properly executed.
Add an expect statement after the existing assertions in this test block to
verify that host.worktreeRemovalJob is null, ensuring the .finally lifecycle
cleanup path is protected and properly nullifies the removal job reference.
- Around line 204-212: The test proves that settlement continues after the
dashboardInputEpoch changes, but it does not explicitly verify that stale UI
updates are suppressed. Add explicit assertions between the dashboardInputEpoch
assignment and after resolveCreate() that validate UI update methods (such as
dashboard state modifications or UI callback functions) are not invoked by the
stale post-settle operations, ensuring that the epoch change successfully
suppresses outdated updates from completing promises.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b57876dc-7af9-4cec-b5b1-f3bc47902c71
📒 Files selected for processing (16)
src/multiplexer/archives.test.tssrc/multiplexer/archives.tssrc/multiplexer/dashboard-control.test.tssrc/multiplexer/dashboard-control.tssrc/multiplexer/dashboard-lifecycle.test.tssrc/multiplexer/dashboard-lifecycle.tssrc/multiplexer/dashboard-ops.test.tssrc/multiplexer/dashboard-view-methods.test.tssrc/multiplexer/dashboard-view-methods.tssrc/multiplexer/notifications.test.tssrc/multiplexer/project-event-stream.test.tssrc/multiplexer/project-event-stream.tssrc/multiplexer/subscreens.test.tssrc/multiplexer/subscreens.tssrc/multiplexer/worktrees.test.tssrc/multiplexer/worktrees.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- src/multiplexer/project-event-stream.ts
- src/multiplexer/dashboard-view-methods.ts
- src/multiplexer/project-event-stream.test.ts
- src/multiplexer/subscreens.ts
- src/multiplexer/archives.ts
- src/multiplexer/dashboard-control.ts
- src/multiplexer/worktrees.ts
This was referenced Jun 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification
Summary by CodeRabbit
Bug Fixes