CodeRabbit shadow review for PR #1#6
Closed
TraderSamwise wants to merge 45 commits into
Closed
Conversation
📝 WalkthroughWalkthroughAdds first-class teammate agents across API/UI/runtime with team-aware navigation and rendering, centralizes typed pending actions for sessions/services/worktrees, extends tmux with a “team” command, tightens daemon project-service lifecycle, and updates dashboards, persistence, tests, and docs accordingly. ChangesTeammates + Pending + Tmux
Sequence Diagram(s)sequenceDiagram
participant Client as Desktop UI
participant Server as MetadataServer
participant Model as DashboardModel
participant Ops as SessionActions
participant Tmux as TmuxRuntimeManager
Client->>Server: POST /agents/teammates/create (parentSessionId,…)
Server->>Ops: createTeammateAgent(...)
Ops-->>Server: {sessionId, teamId, reused?}
Server-->>Client: Teammate created/reused
Client->>Model: withMetadataSessionPending(starting)
Model-->>Tmux: statusline refresh (force)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~180 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
TraderSamwise
marked this pull request as draft
May 21, 2026 15:10
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/dashboard/pending-actions.ts (1)
315-337:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent stale settle callbacks from clearing newer pending actions.
settleCreatePendingalways callsclearEntry(target, itemId). If a newer pending action for the same target/id is set before this async flow completes, the newer action is removed accidentally.Suggested fix
export function settleCreatePending( target: PendingActionTarget, itemId: string, onSettled: () => void, - opts?: { isSettled?: () => boolean | Promise<boolean>; timeoutMs?: number }, + opts?: { isSettled?: () => boolean | Promise<boolean>; timeoutMs?: number; expectedToken?: number }, ): void { @@ - this.clearEntry(target, itemId); + if (opts?.expectedToken !== undefined) { + this.clearEntryIfToken(target, itemId, opts.expectedToken); + } else { + this.clearEntry(target, itemId); + } onSettled();🤖 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/dashboard/pending-actions.ts` around lines 315 - 337, settleCreatePending currently unconditionally calls clearEntry(target, itemId) after an async wait which can remove a newer pending action; fix by recording a unique marker when this settle flow starts (e.g., capture startedAt or a generated token) and, just before calling clearEntry, verify the current pending entry for target/itemId still matches that marker (compare the entry's createdAt or token stored when the pending action was enqueued); only call clearEntry(target, itemId) and invoke onSettled() if the markers match, otherwise skip clearing so newer pending actions are not removed.
🤖 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 `@scripts/tmux-control.sh`:
- Around line 446-473: The selection can pick a direct teammate with no
tmuxWindowId even though another direct teammate is live; update the logic that
computes direct/target to filter for live windows before choosing the first
entry: after building and sorting direct (or immediately after constructing
direct), filter it (e.g., into a new list like direct_live) to only include
sessions where session.get("tmuxWindowId") is truthy (and/or
session.get("status") == "live" if you prefer status-based filtering), then use
that filtered list when checking emptiness, selecting target, and logging
(adjust logs that reference direct[0] to use the first item from the filtered
list); keep teammate_order, parse_time and sorting unchanged but apply them to
the filtered list so you never pick a teammate without a tmuxWindowId for
target.
In `@src/dashboard/ui-state-store.ts`:
- Around line 40-52: The loadSharedState method can leave stale orderState when
JSON read/parse fails; before the try block reset this.orderState to an empty
mapping (e.g., agentOrderByWorktreeKey and serviceOrderByWorktreeKey as empty
maps/objects) so that if readFileSync/JSON.parse in loadSharedState throws,
orderState is not left from a previous load; update loadSharedState to set
this.orderState = { agentOrderByWorktreeKey: sanitizeOrderMap({}),
serviceOrderByWorktreeKey: sanitizeOrderMap({}) } (or equivalent empty
structures) before the try, referencing loadSharedState, orderState, and
sanitizeOrderMap.
In `@src/debug-state.ts`:
- Around line 417-430: filterNotifications currently filters notifications but
never records hits in the target resolution, so notifications-only matches stay
missing; update filterNotifications(source, target) to call addMatch with the
target (and appropriate context/path or identifier) whenever the computed
notifications array is non-empty (i.e., when any entry matched), then return the
same SourceResult ({ status: "found", path: source.path, value: { notifications
} }) so targetResolution sees the match; reference the functions/values
filterNotifications, addMatch, source, target, and the returned notifications to
locate and implement the change.
In `@src/metadata-server.ts`:
- Around line 2021-2041: The POST /agents/teammates/create handler forwards
requests to this.options.lifecycle.createTeammateAgent without validating or
authorizing body.parentSessionId; update the handler to extract parentSessionId
from the body, run the same parent-session validation/authorization used by
other teammate endpoints (reuse the existing helper or function that checks
session existence, ownership, and prevents nested-team parents — e.g., the
validate/authorize function used elsewhere for teammate routes), and if invalid
return the appropriate 400/403 error before calling
this.options.lifecycle.createTeammateAgent; only call this.options.onChange()
and send 200 on successful creation.
In `@src/multiplexer/dashboard-control.ts`:
- Around line 626-630: The code in postToProjectService currently retries on
attempt === 0 regardless of status, which can resend non-retryable failures
(e.g., 409/500) and cause duplicate non-idempotent writes; change the retry
condition so that it only retries when isProjectServiceRetryableStatus(status)
is true (remove the attempt === 0 clause), keeping the existing retry flow that
calls ensureDashboardControlPlane(host) and sleepProjectServiceRetry(attempt)
only for retryable statuses; update any related comments and ensure lastError
still captures the failure for non-retryable responses so the function returns
or throws without retrying.
In `@src/multiplexer/dashboard-model.ts`:
- Around line 318-321: The catch blocks currently call
setPendingDashboardSessionAction(host, sessionId, null) unconditionally and can
clear newer pending actions; change both error-path calls to be token-aware by
passing the same token used on success (i.e., call
setPendingDashboardSessionAction(host, sessionId, tokenOrNull) so the setter
only clears when the token matches), ensuring you compare or supply the token
used when initiating the lifecycle operation (refer to the token variable used
alongside setPendingDashboardSessionAction in the success path) and apply the
same fix to the second occurrence around the other catch (the other
setPendingDashboardSessionAction call).
In `@src/multiplexer/dashboard-ops.ts`:
- Around line 70-93: The current restoreWarningLines early-returns when
result.warning is empty which drops teammateFailures; update restoreWarningLines
to always process result.teammateFailures (inspect result.teammateFailures into
failures as currently done) regardless of result.warning, then build lines =
failures.length > 0 ? failures : (warning ? [warning] : []), derive uniqueLines
from that, and only append the stale-teammates fallback ("Stale teammates remain
offline; create a new team to replace them.") when uniqueLines is empty; keep
using the same variables/function names (restoreWarningLines, result.warning,
result.teammateFailures, failures, uniqueLines) so behavior reflects teammate
failures independently and avoids adding the fallback when backend-provided
failure lines exist.
- Around line 839-853: The dashboard resume path (resumeOfflineSession)
currently posts only { sessionId } and thus drops session metadata; update
resumeOfflineSession to mirror resumeOfflineSessionWithFeedback by including the
full session seed and team in the payload when calling host.postToProjectService
(e.g., send { sessionId: session.id, session: sessionSeed } and include team:
session.team or the equivalent sessionSeed.team), so callers via
dashboardSessionActionDeps().resumeOfflineSession preserve the same session
shape as the main restore flow; modify the payload passed to
host.postToProjectService in resumeOfflineSession accordingly.
In `@src/multiplexer/persistence-methods.ts`:
- Around line 309-324: The teammates mapping currently forces active: false
which drops the real active state; change the mapping in persistence-methods.ts
(the orderedTeammates.map callback) to copy the session's active flag (e.g., use
session.active or the existing property that represents focus/active) instead of
hard-coding false so the teammates entry preserves its real active status for
the tmux statusline payload.
In `@src/multiplexer/session-actions.ts`:
- Around line 379-384: The code currently creates a teammate session before
validating that agent input is supported, causing orphaned sessions when
initialPrompt is provided but host.writeAgentInput isn't implemented; modify
createTeammateAgent to perform the initialPrompt check and throw before any
session creation: if opts.initialPrompt?.trim() then assert typeof
host.writeAgentInput === "function" (and throw the existing Error message)
before calling whatever session-creation logic uses transport.id, and only call
host.writeAgentInput after the session is successfully created; update
references to opts.initialPrompt, host.writeAgentInput, createTeammateAgent and
transport.id accordingly so the session is never created when agent input
support is missing.
In `@src/multiplexer/session-capture.ts`:
- Around line 37-39: The current hasExactAimuxSessionPreamble uses includes(...)
which can match sessionId substrings (e.g., "codex-1" inside "codex-10"); change
it to assert an exact token match by using a regex that escapes the sessionId
and requires a word-boundary or end-of-string after it (e.g., new RegExp with
\\b or (?:$) after the escaped sessionId) so the preamble text "This is an
aimux-managed session with session ID <id>" only matches when <id> is an exact
session token; update hasExactAimuxSessionPreamble to build/escape the regex and
test text with it.
- Around line 31-35: The function readSessionCaptureSample currently reads the
entire file into memory; change it to stream/partial-read the file using fs.stat
and either fs.createReadStream to read the first SESSION_CAPTURE_MAX_BYTES and a
final tail read or use fs.open + fs.read to read the first and last
SESSION_CAPTURE_MAX_BYTES directly by offset so you never load the whole file;
keep the same return format (head + "\n" + tail) and reference the same
constants and function name readSessionCaptureSample and
SESSION_CAPTURE_MAX_BYTES while ensuring you handle small files (return full
content if file size <= SESSION_CAPTURE_MAX_BYTES * 2) and close any file
descriptors or streams after reading.
In `@src/statusline-model.ts`:
- Around line 396-399: resolveCurrentTeammates is including teammates regardless
of status; update its filter to exclude inactive sessions by checking the
session status before sorting/slicing. Modify the existing chain on
data.teammates (in resolveCurrentTeammates) to include an additional filter such
as session => session.status === 'active' (or session.isActive === true if your
model uses that flag) alongside the existing parentSessionId check so only
active teammates are considered, then sort with compareTeammateSessions and
slice as before.
---
Outside diff comments:
In `@src/dashboard/pending-actions.ts`:
- Around line 315-337: settleCreatePending currently unconditionally calls
clearEntry(target, itemId) after an async wait which can remove a newer pending
action; fix by recording a unique marker when this settle flow starts (e.g.,
capture startedAt or a generated token) and, just before calling clearEntry,
verify the current pending entry for target/itemId still matches that marker
(compare the entry's createdAt or token stored when the pending action was
enqueued); only call clearEntry(target, itemId) and invoke onSettled() if the
markers match, otherwise skip clearing so newer pending actions are not removed.
🪄 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 Plus
Run ID: 87e1c6c5-4123-401d-bf9a-d95c39bf5bff
📒 Files selected for processing (86)
.gitignoreREADME.mddesktop-ui/src/lib/WorktreePanel.sveltedesktop-ui/src/stores/state.svelte.jsscripts/tmux-control.shsrc/daemon.test.tssrc/daemon.tssrc/dashboard/index.test.tssrc/dashboard/index.tssrc/dashboard/pending-actions.test.tssrc/dashboard/pending-actions.tssrc/dashboard/quick-jump.test.tssrc/dashboard/quick-jump.tssrc/dashboard/session-actions.tssrc/dashboard/session-registry.test.tssrc/dashboard/session-registry.tssrc/dashboard/state.tssrc/dashboard/ui-state-store.tssrc/debug-state.test.tssrc/debug-state.tssrc/fast-control.test.tssrc/fast-control.tssrc/instance-directory.test.tssrc/instance-directory.tssrc/instance-registry.tssrc/main.tssrc/metadata-server.test.tssrc/metadata-server.tssrc/multiplexer/archives.tssrc/multiplexer/dashboard-control.test.tssrc/multiplexer/dashboard-control.tssrc/multiplexer/dashboard-interaction.test.tssrc/multiplexer/dashboard-interaction.tssrc/multiplexer/dashboard-model.test.tssrc/multiplexer/dashboard-model.tssrc/multiplexer/dashboard-ops.test.tssrc/multiplexer/dashboard-ops.tssrc/multiplexer/dashboard-state-methods.tssrc/multiplexer/dashboard-tail-methods.test.tssrc/multiplexer/dashboard-tail-methods.tssrc/multiplexer/dashboard-view-methods.test.tssrc/multiplexer/dashboard-view-methods.tssrc/multiplexer/graveyard-view-model.test.tssrc/multiplexer/graveyard-view-model.tssrc/multiplexer/index.tssrc/multiplexer/notifications.test.tssrc/multiplexer/notifications.tssrc/multiplexer/persistence-methods.test.tssrc/multiplexer/persistence-methods.tssrc/multiplexer/runtime-lifecycle-methods.tssrc/multiplexer/runtime-state.test.tssrc/multiplexer/runtime-state.tssrc/multiplexer/service-state-snapshot.test.tssrc/multiplexer/service-state-snapshot.tssrc/multiplexer/services.test.tssrc/multiplexer/services.tssrc/multiplexer/session-actions.test.tssrc/multiplexer/session-actions.tssrc/multiplexer/session-capture.tssrc/multiplexer/session-launch.test.tssrc/multiplexer/session-launch.tssrc/multiplexer/session-runtime-core.test.tssrc/multiplexer/session-runtime-core.tssrc/multiplexer/subscreens.test.tssrc/multiplexer/subscreens.tssrc/multiplexer/worktrees.test.tssrc/multiplexer/worktrees.tssrc/paths.tssrc/pending-actions.tssrc/session-bootstrap.test.tssrc/session-bootstrap.tssrc/session-runtime.tssrc/session-semantics.test.tssrc/session-semantics.tssrc/statusline-model.tssrc/team.test.tssrc/team.tssrc/tmux/control-script.test.tssrc/tmux/runtime-manager.test.tssrc/tmux/runtime-manager.tssrc/tmux/statusline.test.tssrc/tmux/statusline.tssrc/tui/screens/dashboard-renderers.test.tssrc/tui/screens/dashboard-renderers.tssrc/tui/screens/overlay-renderers.tssrc/tui/screens/subscreen-renderers.ts
Owner
Author
|
Fixed in 5d32b4d - the outside-diff pending-actions finding is addressed by token-aware settle cleanup so stale settle callbacks cannot clear newer pending actions. |
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.
Purpose
Temporary shadow PR to let CodeRabbit review the already-merged PR #1 diff.
Original PR: #1
Original base commit: 2f2ea84
Original head commit: 0b86517
Do not merge this PR. It exists only for review tooling.
Summary by CodeRabbit
Release Notes
New Features
aimux debug-state <target>command for troubleshooting session/service/worktree state.Documentation
Improvements