Complete unified project and per-chat machine routing - #917
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
Warning Review limit reached
Next review available in: 28 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR adds per-chat runtime pinning, cross-machine lane and session synchronization, machine-aware project tabs and session UI, git-origin-based project matching, ahead-counter divergence detection, continuity recovery wiring, remote-launch timeout handling, and removal of the obsolete local-work IPC path. ChangesRuntime and project identity
Pinned runtime routing
Cross-machine workspace behavior
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
arul28
changed the title
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 47b4e2b3d0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| session={session} | ||
| online={row.online} | ||
| machineName={row.machineName} | ||
| onOpen={() => openForeignLane(row)} |
There was a problem hiding this comment.
When a user clicks a specific foreign session, this callback ignores session and only calls switchRemoteProject through openForeignLane. The project tab is therefore rebound—moving Lanes/PRs/Files with it—while the clicked session is never selected, so the new per-chat routing path remains unreachable from the cross-machine sidebar. Route the clicked session and its binding into the Work selection state instead of switching the whole project.
Useful? React with 👍 / 👎.
| // Foreign lane/session churn shows up here as local activity; ADE has no | ||
| // cross-machine change feed, and adding a poll is explicitly not allowed. | ||
| const unsubscribeSessions = window.ade?.sessions?.onChanged?.(() => scheduleRefresh()); | ||
| if (unsubscribeSessions) runtime.disposers.push(unsubscribeSessions); | ||
| const unsubscribeLanes = window.ade?.lanes?.onLifecycleEvent?.(() => scheduleRefresh()); | ||
| if (unsubscribeLanes) runtime.disposers.push(unsubscribeLanes); |
There was a problem hiding this comment.
When the tab is bound to local runtime or remote machine A, these lifecycle hooks only receive local/A events: preload's remote event pump polls getProjectRuntimeBinding(), which is the active binding. Activity created on another connected machine B therefore never schedules a refresh, and connection snapshots do not change for ordinary lane/session churn, leaving B's sidebar rows stale indefinitely until unrelated activity occurs on the bound machine. The union needs a change feed or refresh trigger for every target it reads.
AGENTS.md reference: AGENTS.md:L32-L34
Useful? React with 👍 / 👎.
| const chatRuntimePin = useMemo( | ||
| () => chatMachineRouter.pinForLane(selectedSession?.laneId ?? laneId), | ||
| [chatMachineRouter, laneId, selectedSession?.laneId], | ||
| ); | ||
| const chatRuntimePinRef = useRef<OpenProjectBinding | null>(chatRuntimePin); | ||
| chatRuntimePinRef.current = chatRuntimePin; |
There was a problem hiding this comment.
When a foreign chat is displayed while the project tab remains bound to another machine, this pin routes history and mutation calls correctly, but window.ade.agentChat.onEvent remains unpinned and preload's singleton event pump follows only the active project binding. Replies and approval events from the selected chat's machine consequently do not stream into the pane, making a successfully sent message appear stuck until a later history reload. Extend the event subscription contract to the pinned binding as well.
AGENTS.md reference: AGENTS.md:L32-L34
Useful? React with 👍 / 👎.
| const thisMachine = deriveLaneMachineOptions({ | ||
| connections: [], | ||
| boundTargetId: projectBinding.targetId, | ||
| boundProject, | ||
| repoDisplayName: projectBinding.displayName, | ||
| localProjectRoots: localRoots, |
There was a problem hiding this comment.
When a remote repository and an unrelated local repository share the same display name or folder basename (for example two different api repositories), this derivation treats the local folder-name match as the remote repository's counterpart and returns it. Selecting “This Mac” then silently switches the window to the wrong repository, despite this commit already making local recent-project origins available. Pass and compare the local origin identity, and refuse the switch when only the name matches.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/desktop/src/renderer/components/lanes/LaneGitActionsPane.tsx (1)
1358-1386: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMissing
lanes/crossMachineLanesByMachineIdinresolvePushDivergence's dependency array.The callback body reads
lanesandcrossMachineLanesByMachineIdfrom closure (line 1366) viaselectOtherMachineBranchStates({ lanes, crossMachineLanesByMachineId }, lane.id), but neither is listed in theuseCallbackdeps (1379-1386).lanetrackslanesindirectly through its own memo, butcrossMachineLanesByMachineIdhas no such proxy — if the cross-machine union updates without any of the listed deps changing, this guard will silently evaluate against stale foreign-machine branch state at click time, which can suppress a real push-divergence warning.🐛 Proposed fix
}, [ currentMachineHeadSha, currentMachineId, currentMachineName, + crossMachineLanesByMachineId, lane, + lanes, otherMachineBranchStates, syncStatus, ]);🤖 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 `@apps/desktop/src/renderer/components/lanes/LaneGitActionsPane.tsx` around lines 1358 - 1386, Add lanes and crossMachineLanesByMachineId to the dependency array of resolvePushDivergence so selectOtherMachineBranchStates uses current closure values when the callback runs. Preserve the existing fallback selection and divergence detection behavior.apps/desktop/src/renderer/components/lanes/laneMachines.ts (1)
204-240: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject name-only local matches in
resolveThisMachineProjectRoot.
resolveThisMachineProjectRoottruststhisMachine?.project?.rootPathas soon as it exists, even whenLaneMachineProjectRef.matchedByis"name". A same-named-but-different-repo checkout would then rebind “This Mac” to the wrong repo. TreatmatchedBy: "name"as “no local checkout found” and return the missing-message result.🤖 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 `@apps/desktop/src/renderer/components/lanes/laneMachines.ts` around lines 204 - 240, Update resolveThisMachineProjectRoot to accept thisMachine.project.rootPath only when the project is not matchedBy "name". Treat name-only matches as absent and return the existing missing-message result, while preserving trusted bound or other project matches.apps/desktop/src/preload/preload.ts (1)
6120-6192: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winEnsure pin-only
getEventHistory/getEventHistoryPageerrors do not treat temporary unreachability as authoritative.The pinned branches only call
callPinnedRuntimeAction, while the unpinned fallback intentionally returns{ sessionFound: false, unavailable: true }for remote sessions. If the pinned remote runtime is unreachable, callers should preserve that same non-destructive unavailable shape instead of surfacing a transport/action error or anysessionFound: falseresponse the renderer consumes.🤖 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 `@apps/desktop/src/preload/preload.ts` around lines 6120 - 6192, Update the pinned branches of getEventHistory and getEventHistoryPage to catch temporary remote-runtime transport or action errors and return the same non-destructive unavailable response shape used by the unpinned remote fallback, preserving the requested session ID and beforeOffset where applicable. Ensure pinned errors do not surface directly or return an authoritative sessionFound: false result, while leaving successful pinned responses and local behavior unchanged.
🧹 Nitpick comments (9)
apps/desktop/src/renderer/components/app/TopBar.test.tsx (1)
674-675: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
fireEvent.clickdoesn't dispatchmousedown, so the outside-click path is untested.Consider
userEvent.click(or an explicitfireEvent.mouseDown) here so the menu'swindowmousedown listener is exercised — that path is where the caret toggle currently misbehaves (seeapps/desktop/src/renderer/components/app/TopBar.tsxLine 517).🤖 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 `@apps/desktop/src/renderer/components/app/TopBar.test.tsx` around lines 674 - 675, Update the interaction in the relevant TopBar test to dispatch the mousedown event required by the menu’s window listener, preferably by using userEvent.click or explicitly firing mouseDown before the selection. Ensure the test exercises the outside-click path associated with the caret toggle near the TopBar menu logic.apps/desktop/src/renderer/components/app/TopBar.tsx (1)
533-592: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
role="menu"without focus or arrow-key handling.Nothing receives focus when the portal opens and there is no roving-focus/arrow-key handling, so the menu doesn't behave as an ARIA menu; the items are reachable only by tabbing to the end of
document.body. Focusing the active item on open (and handling ArrowUp/ArrowDown) would make it operable in place. The repo already hasuseDialogFocusTrapused forConnectionsPanelin this file if you prefer reusing that pattern.As per coding guidelines, "preserve existing patterns before introducing new abstractions".
🤖 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 `@apps/desktop/src/renderer/components/app/TopBar.tsx` around lines 533 - 592, Update the machine menu component around the portal and its machine-item buttons so opening the menu focuses the active item, or the first available item when none is active. Add in-place ArrowUp/ArrowDown handling with wraparound and prevent default scrolling, using the existing focus-management pattern such as useDialogFocusTrap where appropriate; keep selection and close behavior unchanged.Sources: Coding guidelines, Path instructions
apps/desktop/src/renderer/components/terminals/SessionListPane.tsx (1)
433-452: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
aria-labelon a plain<span>is not reliably announced.The marker has no role, so assistive tech may ignore the label entirely — in glyph mode that leaves the "this work is elsewhere" signal visual-only. Adding
role="img"(or a visually-hidden text node) makes the machine name reachable in both modes.♿ Proposed tweak
<span + role="img" className={cn( "inline-flex shrink-0 items-center gap-1 rounded-full border border-white/10 bg-white/[0.04] px-1.5 py-px text-[10px] font-medium leading-none", marker.online ? "text-muted-fg/70" : "text-muted-fg/45", )}🤖 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 `@apps/desktop/src/renderer/components/terminals/SessionListPane.tsx` around lines 433 - 452, Add an explicit accessible role to the span returned by LaneMachineMarker so its existing aria-label is announced in both name and glyph modes. Preserve the current title, label content, data attributes, styling, and conditional machine-name rendering.apps/desktop/src/renderer/state/crossMachineLanes.test.ts (1)
67-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo coverage for the sync engine itself.
The suite exercises the pure builders and the store, but
startCrossMachineLaneSync/runRefresh/readMachine— ref counting, scope changes, theMAX_PARALLEL_MACHINE_READSbound, timeout handling and generation-based cancellation — are the riskiest parts of this layer and are untested. A stubbedwindow.ade.remoteRuntimewith fake timers would cover the cancellation and retention paths end to end.🤖 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 `@apps/desktop/src/renderer/state/crossMachineLanes.test.ts` around lines 67 - 78, The test suite needs end-to-end coverage for the cross-machine lane sync engine. Extend crossMachineLanes.test.ts around startCrossMachineLaneSync, runRefresh, and readMachine using a stubbed window.ade.remoteRuntime and fake timers; cover reference counting, scope changes, MAX_PARALLEL_MACHINE_READS concurrency, timeout handling, and generation-based cancellation while verifying retained store state.apps/desktop/src/renderer/components/terminals/SessionListPane.test.tsx (1)
834-841: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the dimming and read-only behaviour the comment claims.
The offline case verifies marker mode and the machine name, but not the
dimmedgroup styling or thatCrossMachineSessionRowrenders disabled — the two behaviours the retention policy is actually about. A check on the foreign session button'sdisabledstate would lock that in.🤖 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 `@apps/desktop/src/renderer/components/terminals/SessionListPane.test.tsx` around lines 834 - 841, Extend the offline-machine test around seedForeignMachine and renderPane to assert the retained lane has the dimmed styling and the foreign session row’s button is disabled. Locate the relevant lane/group styling and CrossMachineSessionRow session button in the rendered output, while preserving the existing marker-mode and machine-name assertions.apps/desktop/src/renderer/state/appStore.ts (1)
1489-1526: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winContent-equal refreshes still produce a new slice identity.
decodeForeignLanes/decodeForeignSessionsalways allocate fresh arrays, soprevious.lanes === next.lanesnever holds on a successful read. Every coalesced refresh (session-changed / lane-lifecycle churn) therefore replacescrossMachineLanesByMachineId, invalidatingbuildCrossMachineLaneRows, the marker map, andselectOtherMachineBranchStates' cache even when nothing changed. A shallow id/updated-at comparison before accepting the new arrays would keep the "identity matters" intent thatapplyCrossMachineLaneScopealready documents.Also note
lastSyncedAtMsis excluded from the equality block, so a same-reference re-merge silently drops the timestamp refresh.🤖 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 `@apps/desktop/src/renderer/state/appStore.ts` around lines 1489 - 1526, Update mergeCrossMachineLanes to reuse previous lanes and sessions arrays when their entries are shallowly equal by stable identifiers and updated-at values, rather than relying on reference equality from decodeForeignLanes/decodeForeignSessions. Preserve the previous array references for content-equal refreshes, and include lastSyncedAtMs in the no-op comparison so timestamp changes are not silently discarded.apps/desktop/src/renderer/state/crossMachineLanes.ts (1)
448-504: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueAbort in-flight remote runtime calls when the read timeout expires.
withTimeoutonly rejects the wrapper promise; the underlyingremoteRuntime.callActioncontinues over IPC. Passing the cancellation signal down toremoteConnectionService.callActionwould prevent a wedged machine from accumulating in-flight reads behind repeated refreshes.🤖 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 `@apps/desktop/src/renderer/state/crossMachineLanes.ts` around lines 448 - 504, Update readMachine’s remote call flow so each timed-out withTimeout operation cancels the underlying remoteRuntime.callAction, propagating an AbortSignal through the callAction and remoteConnectionService.callAction layers. Ensure the signal is aborted when MACHINE_READ_TIMEOUT_MS expires while preserving existing generation checks and error/retention behavior.apps/desktop/src/renderer/components/terminals/useWorkSessions.test.ts (1)
682-737: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood regression test, but only covers remote-binding pins.
This nicely locks in the remote-machine liveness behavior, but doesn't exercise a
kind: "local"pin (e.g. a background local project tab). As noted onuseWorkSessions.tsLine 464-480, the local-binding branch ofcanMutatePinnedProjectUicurrently uses a different key format than the one produced byAgentChatPane.tsx, so a local-pin regression wouldn't be caught here. Consider adding a third case pinning to an open local tab (constructed the same wayAgentChatPane.tsxbuilds it) alongside the existing open/closed remote cases.🤖 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 `@apps/desktop/src/renderer/components/terminals/useWorkSessions.test.ts` around lines 682 - 737, Add a third regression case in the “applies optimistic UI for a session pinned to another open machine” test for an open local tab, using the same local pin shape and key construction as AgentChatPane. Include that local tab in fakeAppStoreState.openProjectTabRoots, mock its PTY creation, launch a session with the local pin, and assert the optimistic session is retained, while preserving the existing remote open/closed assertions.apps/desktop/src/renderer/components/chat/AgentChatComposer.tsx (1)
1519-1550: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated "resolve this machine's root + surface error" logic across two files. Both sites reimplement the identical
resolveThisMachineProjectRoot→ ok/error branch →switchProjectToPath(...).catch(...)sequence; extracting a small shared hook would prevent the two from drifting the next time this path changes.
apps/desktop/src/renderer/components/chat/AgentChatComposer.tsx#L1519-L1550: extract the local-machine branch (1530-1543) into a shared helper/hook that takes an error-setter and returns the switch logic.apps/desktop/src/renderer/components/personalChats/PersonalChatsPage.tsx#L517-L554: replace the equivalent inline block (521-536) with the same shared helper/hook.🤖 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 `@apps/desktop/src/renderer/components/chat/AgentChatComposer.tsx` around lines 1519 - 1550, The local-machine project-root resolution and switch error handling is duplicated across both handlers. In apps/desktop/src/renderer/components/chat/AgentChatComposer.tsx lines 1519-1550, extract the local branch from handleMachineChange into a shared helper or hook that accepts an error setter and performs resolveThisMachineProjectRoot, error reporting, and switchProjectToPath handling; in apps/desktop/src/renderer/components/personalChats/PersonalChatsPage.tsx lines 517-554, replace the equivalent inline logic with that shared implementation.
🤖 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 `@apps/desktop/src/renderer/components/app/TopBar.tsx`:
- Around line 517-531: Update MachineSwitcherMenu’s outside-click handling to
ignore mousedown events originating from its trigger, either by passing the
anchor element into the menu or stopping propagation on the caret button. In
apps/desktop/src/renderer/components/app/TopBar.tsx lines 517-531, preserve
closing for clicks outside the menu and trigger. In
apps/desktop/src/renderer/components/app/TopBar.test.tsx lines 674-675, use
userEvent.click or explicitly fire mousedown, and assert that clicking the caret
toggles the menu closed.
In `@apps/desktop/src/renderer/components/lanes/CreateLaneDialog.test.tsx`:
- Around line 249-289: Update makeMachineProps to delegate to the existing
makeProps fixture builder instead of duplicating the full DialogProps object.
Preserve its current override behavior by passing overrides through to
makeProps, and remove the redundant field definitions from makeMachineProps.
In `@apps/desktop/src/renderer/components/lanes/CreateLaneDialogHost.tsx`:
- Around line 321-335: Fix the race between restoreBindingFromBeforeOpen and the
asynchronous machine switch initiated by handleSelectMachine. Track or await the
in-flight switch, defer the previous-binding comparison until it settles, and
then restore the pre-open binding if the switch changed it; keep the pending
state active until this check completes. Add a
CreateLaneDialogHostBinding.test.tsx case covering dialog closure before the
mocked switch promise resolves.
In `@apps/desktop/src/renderer/components/terminals/SessionListPane.tsx`:
- Line 1526: The empty-state condition in SessionListPane’s session list must
account for foreign sessions in every grouping mode, not only when isByLane is
true. Update the !hasAnySessions guard to use the union of local and foreign
session availability, and ensure the empty-state copy accurately describes the
visible/hidden state and next action.
- Around line 1153-1162: Update openForeignLane to stop silently swallowing
switchRemoteProject failures; let the existing projectTransitionError handling
surface the failure, or log the caught error with useful context if local
handling is required. Preserve the current validation and successful
remote-project switch behavior.
- Around line 1189-1191: Update the lane marker and collapse-state indexing used
by SessionListPane so keys include both machineId and lane.id, preventing
cross-machine collisions and foreign marker rendering. Apply the composite key
consistently to markersByLaneId and workCollapsedLaneIds, including lookups and
row-key generation, or alternatively enforce lane-id uniqueness per machine at
creation.
In `@apps/desktop/src/renderer/components/terminals/useWorkSessions.ts`:
- Around line 464-480: Update canMutatePinnedProjectUi so local bindings built
from openProjectTabRoots use the established local: key format before passing
them to isLivePinnedBinding. Keep the existing projectBinding and
openRemoteProjectTabs handling unchanged, ensuring pins for open background
local tabs match under exact key comparison.
In `@apps/desktop/src/renderer/state/crossMachineLanes.ts`:
- Around line 601-611: Update
apps/desktop/src/renderer/state/crossMachineLanes.ts:601-611 in
startCrossMachineLaneSync so runtime state is keyed per scope key, or explicitly
rejects/warns on conflicting scopes, instead of overwriting one global scope;
synchronously invalidate crossMachineLaneScopeKey when subscribing rather than
deferring it to runRefresh. Update
apps/desktop/src/shared/laneDivergence.ts:144-149 so the ahead > 0 comparison
cannot match same-named branches from a previous repository, and consider aging
out others entries from long-offline slices using lastSyncedAtMs.
---
Outside diff comments:
In `@apps/desktop/src/preload/preload.ts`:
- Around line 6120-6192: Update the pinned branches of getEventHistory and
getEventHistoryPage to catch temporary remote-runtime transport or action errors
and return the same non-destructive unavailable response shape used by the
unpinned remote fallback, preserving the requested session ID and beforeOffset
where applicable. Ensure pinned errors do not surface directly or return an
authoritative sessionFound: false result, while leaving successful pinned
responses and local behavior unchanged.
In `@apps/desktop/src/renderer/components/lanes/LaneGitActionsPane.tsx`:
- Around line 1358-1386: Add lanes and crossMachineLanesByMachineId to the
dependency array of resolvePushDivergence so selectOtherMachineBranchStates uses
current closure values when the callback runs. Preserve the existing fallback
selection and divergence detection behavior.
In `@apps/desktop/src/renderer/components/lanes/laneMachines.ts`:
- Around line 204-240: Update resolveThisMachineProjectRoot to accept
thisMachine.project.rootPath only when the project is not matchedBy "name".
Treat name-only matches as absent and return the existing missing-message
result, while preserving trusted bound or other project matches.
---
Nitpick comments:
In `@apps/desktop/src/renderer/components/app/TopBar.test.tsx`:
- Around line 674-675: Update the interaction in the relevant TopBar test to
dispatch the mousedown event required by the menu’s window listener, preferably
by using userEvent.click or explicitly firing mouseDown before the selection.
Ensure the test exercises the outside-click path associated with the caret
toggle near the TopBar menu logic.
In `@apps/desktop/src/renderer/components/app/TopBar.tsx`:
- Around line 533-592: Update the machine menu component around the portal and
its machine-item buttons so opening the menu focuses the active item, or the
first available item when none is active. Add in-place ArrowUp/ArrowDown
handling with wraparound and prevent default scrolling, using the existing
focus-management pattern such as useDialogFocusTrap where appropriate; keep
selection and close behavior unchanged.
In `@apps/desktop/src/renderer/components/chat/AgentChatComposer.tsx`:
- Around line 1519-1550: The local-machine project-root resolution and switch
error handling is duplicated across both handlers. In
apps/desktop/src/renderer/components/chat/AgentChatComposer.tsx lines 1519-1550,
extract the local branch from handleMachineChange into a shared helper or hook
that accepts an error setter and performs resolveThisMachineProjectRoot, error
reporting, and switchProjectToPath handling; in
apps/desktop/src/renderer/components/personalChats/PersonalChatsPage.tsx lines
517-554, replace the equivalent inline logic with that shared implementation.
In `@apps/desktop/src/renderer/components/terminals/SessionListPane.test.tsx`:
- Around line 834-841: Extend the offline-machine test around seedForeignMachine
and renderPane to assert the retained lane has the dimmed styling and the
foreign session row’s button is disabled. Locate the relevant lane/group styling
and CrossMachineSessionRow session button in the rendered output, while
preserving the existing marker-mode and machine-name assertions.
In `@apps/desktop/src/renderer/components/terminals/SessionListPane.tsx`:
- Around line 433-452: Add an explicit accessible role to the span returned by
LaneMachineMarker so its existing aria-label is announced in both name and glyph
modes. Preserve the current title, label content, data attributes, styling, and
conditional machine-name rendering.
In `@apps/desktop/src/renderer/components/terminals/useWorkSessions.test.ts`:
- Around line 682-737: Add a third regression case in the “applies optimistic UI
for a session pinned to another open machine” test for an open local tab, using
the same local pin shape and key construction as AgentChatPane. Include that
local tab in fakeAppStoreState.openProjectTabRoots, mock its PTY creation,
launch a session with the local pin, and assert the optimistic session is
retained, while preserving the existing remote open/closed assertions.
In `@apps/desktop/src/renderer/state/appStore.ts`:
- Around line 1489-1526: Update mergeCrossMachineLanes to reuse previous lanes
and sessions arrays when their entries are shallowly equal by stable identifiers
and updated-at values, rather than relying on reference equality from
decodeForeignLanes/decodeForeignSessions. Preserve the previous array references
for content-equal refreshes, and include lastSyncedAtMs in the no-op comparison
so timestamp changes are not silently discarded.
In `@apps/desktop/src/renderer/state/crossMachineLanes.test.ts`:
- Around line 67-78: The test suite needs end-to-end coverage for the
cross-machine lane sync engine. Extend crossMachineLanes.test.ts around
startCrossMachineLaneSync, runRefresh, and readMachine using a stubbed
window.ade.remoteRuntime and fake timers; cover reference counting, scope
changes, MAX_PARALLEL_MACHINE_READS concurrency, timeout handling, and
generation-based cancellation while verifying retained store state.
In `@apps/desktop/src/renderer/state/crossMachineLanes.ts`:
- Around line 448-504: Update readMachine’s remote call flow so each timed-out
withTimeout operation cancels the underlying remoteRuntime.callAction,
propagating an AbortSignal through the callAction and
remoteConnectionService.callAction layers. Ensure the signal is aborted when
MACHINE_READ_TIMEOUT_MS expires while preserving existing generation checks and
error/retention behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d216b49b-3cbd-4312-9b1b-aa4602beafe7
⛔ Files ignored due to path filters (7)
docs/ARCHITECTURE.mdis excluded by!docs/**docs/features/chat/README.mdis excluded by!docs/**docs/features/lanes/README.mdis excluded by!docs/**docs/features/personal-chats/README.mdis excluded by!docs/**docs/features/remote-runtime/README.mdis excluded by!docs/**docs/features/remote-runtime/internal-architecture.mdis excluded by!docs/**docs/features/terminals-and-sessions/README.mdis excluded by!docs/**
📒 Files selected for processing (43)
apps/desktop/src/main/services/ipc/ipcTimeouts.tsapps/desktop/src/main/services/ipc/runtimeBridge.tsapps/desktop/src/main/services/projects/projectLifecycle.test.tsapps/desktop/src/main/services/projects/recentProjectSummary.test.tsapps/desktop/src/main/services/projects/recentProjectSummary.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.test.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/browserMock.tsapps/desktop/src/renderer/components/app/CommandPalette.test.tsxapps/desktop/src/renderer/components/app/TopBar.test.tsxapps/desktop/src/renderer/components/app/TopBar.tsxapps/desktop/src/renderer/components/app/projectTabGrouping.tsapps/desktop/src/renderer/components/chat/AgentChatComposer.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.test.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/chat/ChatGitToolbar.tsxapps/desktop/src/renderer/components/chat/thisMachineProjectRoot.test.tsapps/desktop/src/renderer/components/chat/thisMachineProjectRoot.tsapps/desktop/src/renderer/components/lanes/CreateLaneDialog.test.tsxapps/desktop/src/renderer/components/lanes/CreateLaneDialogHost.tsxapps/desktop/src/renderer/components/lanes/CreateLaneDialogHostBinding.test.tsxapps/desktop/src/renderer/components/lanes/CreateLaneDialogMachines.test.tsxapps/desktop/src/renderer/components/lanes/LaneGitActionsPane.tsxapps/desktop/src/renderer/components/lanes/PushDivergenceGuard.test.tsxapps/desktop/src/renderer/components/lanes/laneMachines.test.tsapps/desktop/src/renderer/components/lanes/laneMachines.tsapps/desktop/src/renderer/components/personalChats/PersonalChatsPage.tsxapps/desktop/src/renderer/components/remoteTargets/RemoteTargetList.test.tsxapps/desktop/src/renderer/components/terminals/SessionListPane.test.tsxapps/desktop/src/renderer/components/terminals/SessionListPane.tsxapps/desktop/src/renderer/components/terminals/laneComboboxMachineGroups.test.tsxapps/desktop/src/renderer/components/terminals/useWorkSessions.test.tsapps/desktop/src/renderer/components/terminals/useWorkSessions.tsapps/desktop/src/renderer/lib/chatMachineRouting.test.tsapps/desktop/src/renderer/lib/chatMachineRouting.tsapps/desktop/src/renderer/state/appStore.tsapps/desktop/src/renderer/state/crossMachineLanes.test.tsapps/desktop/src/renderer/state/crossMachineLanes.tsapps/desktop/src/shared/ipc.tsapps/desktop/src/shared/laneDivergence.test.tsapps/desktop/src/shared/laneDivergence.tsapps/desktop/src/shared/machineIdentity.ts
💤 Files with no reviewable changes (6)
- apps/desktop/src/renderer/components/lanes/CreateLaneDialogMachines.test.tsx
- apps/desktop/src/renderer/components/remoteTargets/RemoteTargetList.test.tsx
- apps/desktop/src/renderer/components/app/CommandPalette.test.tsx
- apps/desktop/src/main/services/ipc/ipcTimeouts.ts
- apps/desktop/src/shared/ipc.ts
- apps/desktop/src/renderer/browserMock.ts
|
@codex review |
The create-lane machine picker resolved another machine's checkout by git origin and then, unconditionally, fell back to matching on the repo folder name. A folder name is not an identity: a local `~/src/api` (github.com/acme/api) matched an unrelated `api` (github.com/other/api) on another machine, rendered as a valid choice, and selecting it called switchRemoteProject with that project id — rebinding the whole app tab to the wrong repository and then creating a lane in it. Two changes. A name candidate whose known origin contradicts the known origin of the current repo is now rejected outright: matching names with differing origins are proof the repos are *different*, not the same. And a match is tagged with how it was established, so a name-only hit reports `repoMatch: "unknown"` rather than `"matched"` — it stays visible as a candidate, because absence of proof is not proof of absence, but it no longer claims to be verified. Found by /quality Track A. The failing assertion this changes had encoded the bug: it asserted a folder-name match reported as proven. Co-Authored-By: Claude <noreply@anthropic.com>
The id for the machine ADE runs on existed in five places under three names —
laneMachines, projectTabGrouping, AgentChatComposer, PersonalChatsPage, and a
hardcoded literal in LaneGitActionsPane — with **two different values**
("this-mac" and "local"), kept in sync by comments.
That is not cosmetic. The push-divergence guard decides whether a branch lives
on *another* machine by comparing these ids. The moment one producer supplies
"local" where the consumer expects "this-mac", the self-filter stops matching
and ADE warns that This Mac has diverged from itself.
`shared/machineIdentity.ts` is now the only definition. Existing export names
are kept as aliases so call sites are unchanged.
Found by /quality Track B.
Co-Authored-By: Claude <noreply@anthropic.com>
Two features from #912 shipped with no production caller. This wires them. **Tab merge.** `groupProjectTabs` had zero importers — the join existed, the merge didn't. TopBar now renders one tab per repository group with the current machine named inline and a dropdown listing every machine in the group. That dropdown is load-bearing: a merged group hides the non-active machine's tab, so without it a checkout would be stranded with no way back. The join key was also absent from the path the renderer actually uses. `IPC.projectListRecent` is served by `toShallowRecentProjectSummary`, which never set `gitOriginUrl` — so even fully wired, every tab would have arrived with an undefined origin and nothing would ever have merged. **Origin parsing was returning wrong data.** The hand-rolled regex ended a section only on a `[` at column 0, so an indented ` [remote "upstream"]` returned *upstream's* URL — silently merging two unrelated repos into one tab. It also kept surrounding quotes and trailing `;`/`#` comments, and missed `[REMOTE "origin"]` (git section names are case-insensitive). Section scanning now delegates to the already-tested `readOriginRemoteUrlFromGitConfig`; only value undecoration is layered on top. The worktree walk is structural instead of a substring search for `worktrees`, so `~/worktrees/myrepo` no longer truncates to `~`, and a submodule inside a linked worktree reports its own origin rather than the superproject's. New `recentProjectSummary.test.ts` covers all of it — the file that decides whether two repos are the same repo previously had no tests. **Divergence guard.** `LaneSummary` has no `headSha`, so `toMachineBranchState` always produced null and the rule skipped every candidate — it could never return a warning. The rule is now grounded in `ahead`, which every `LaneSummary`/`lane_state_snapshots` record genuinely carries: a machine with commits ahead on your branch holds work that moving the upstream tip would strand. Head shas only ever silence a warning now, never suppress one by being absent, and the doc comment that promised a fallback the code didn't have is gone. The two-path delivery registry with zero producers is deleted; one documented seam remains. Found by /quality. Co-Authored-By: Claude <noreply@anthropic.com>
**Lane picker grouping was unreachable** — `LaneCombobox` supported grouping lanes by machine with a per-machine "Auto-create lane here", but no call site passed `machines`, so it always short-circuited to the flat list. Wired at the chat draft lane selector, gated so a single-machine setup makes no extra IPC calls and renders byte-for-byte as before. **The auto-create id contract had already drifted.** `isAutoCreateLaneOptionId` accepts both the bare id and the per-machine `<id>:<machineId>` form, but three call sites still compared with `===`. Once grouping went live, a per-machine id would have fallen through and been treated as a real lane id. **"This Mac" could switch you to an unrelated repository.** When the tab was bound to another machine, selecting This Mac fell back to `openProjectTabRoots[0]` — the first open local tab in insertion order, with no relation to the repo on screen — and switched to it. The premise of this whole model is that the machine is a dimension *of one repo's tab*; that fallback silently changed the repo instead. Both copies of the logic now resolve the local counterpart by identity and refuse to switch when there isn't one, saying so, rather than landing you somewhere arbitrary. **Cancelling the create-lane dialog left the app rebound.** Picking a machine mutates the active binding (lane creation is routed by it); closing without creating left the app on the other machine with no sign a dialog did it. The binding is captured on open and restored on cancel; an intentional create keeps the switch. Removed the now-dead `remoteRuntimeCheckLocalWork` IPC — channel, handler, preload binding, type, timeout entry, browser mock, and test stubs — left with no callers when `RemoteProjectOpenDialog` was deleted. Found by /quality. Co-Authored-By: Claude <noreply@anthropic.com>
The Work sidebar now unions lanes and chats across every connected machine, independent of which machine the tab is bound to. That was the point of the whole model: you should see what is in flight anywhere without switching tabs. The tab's machine still drives Lanes/PRs/Files/Git — the execution context is unchanged; only the sidebar became global. Machines are tagged onto lanes, never onto chats: a lane owns its machine because `worktree_path` is an absolute path on exactly one machine, and chats inherit through `laneId`. No schema change. A machine that drops keeps its lanes on screen, dimmed and read-only, rather than having them disappear. That is stated in the merge logic instead of being an accident of where an `await` sits — if a wifi blip reflowed half the sidebar away, this would be worse than the two tabs it replaces. The machine marker follows the rule that it only appears when work is *not* here: a bare glyph by default, the machine's name when it is offline, when two or more distinct foreign machines are visible at once, or when the lane carries a same-branch warning. Monochrome — the lane accent already owns colour. No polling. Reads are driven by the existing connection-snapshot and lane/session change events, coalesced, bounded to four machines in parallel with per-machine timeouts and generation-guarded cancellation, over the already-allowlisted `remoteRuntime.callAction` path. Local lanes paint first; foreign rows append as they arrive, so a slow machine cannot stall the sidebar. This also supplies what the push divergence guard was missing. It has been running on a permanently empty list; it now reads the union at click time, so it still costs nothing until you actually press push. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e2b652c8f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| machineId: THIS_MACHINE_ID, | ||
| machineName: THIS_MACHINE_NAME, | ||
| online: true, | ||
| isThisMachine: true, |
There was a problem hiding this comment.
When the project tab is bound to a remote machine, useAppStore().lanes contains that remote binding's lanes, but this loop unconditionally labels them as This Mac. The refresh path also fetches the bound remote target into machines while filtering out THIS_MACHINE_ID, so the active remote machine's sessions appear twice and the actual local machine's sessions disappear from the unified Work sidebar. Derive these rows from the active binding and explicitly load the local checkout when the active binding is remote.
Useful? React with 👍 / 👎.
| const others = otherMachineBranchStates.length > 0 | ||
| ? otherMachineBranchStates | ||
| : publishedOtherMachineBranchStates; | ||
| : selectOtherMachineBranchStates({ lanes, crossMachineLanesByMachineId }, lane.id); |
There was a problem hiding this comment.
On a remote-bound tab, lanes belongs to that remote machine, but selectOtherMachineBranchStates classifies the first matching lane from this array as This Mac while the cross-machine map also contains the same bound remote lane. Consequently, pushing a remote lane with ahead > 0 can report that its own machine also has the branch and display a false divergence warning. Pass the active binding's machine identity into the selector or omit the bound machine's duplicate slice.
Useful? React with 👍 / 👎.
Clicking a chat whose lane lives on another machine now streams from that machine without rebinding the tab. Previously every project-scoped call went through `callProjectRuntimeActionIfBound` to whichever machine the *window* was bound to, so a union sidebar could show you a chat it could not open — or could only open by dragging Lanes, PRs and Files along with it. A chat's machine is its lane's machine; `chatMachineRouting` resolves it from lane ownership over state the renderer already holds. The primitive already existed — `callPinnedRuntimeAction`, used until now only for detached launches — and is promoted to the default path for chat-scoped calls. Everything is additive: `pin` is an optional trailing argument, and a chat on the active binding passes no argument at all and allocates nothing, so the common path is byte-for-byte what it was. Fixes the guard that would have silently swallowed this. `canMutatePinnedProjectUi` dropped UI updates whenever a pin differed from the active binding, which meant "stale detached launch" before and means "chat on another machine" now — it would have discarded updates for precisely the chats this enables. It now asks whether the pinned binding is still open rather than whether it is the active one. Co-Authored-By: Claude <noreply@anthropic.com>
Test steward pass. The machine-selection feature had fractured into four small files in `components/lanes/` — a folder already carrying 20 test files for 39 sources. `CreateLaneDialogMachines` is folded into `CreateLaneDialog.test.tsx` (both render the same component), and the LaneCombobox grouping test moves next to `LaneCombobox.tsx` in `components/terminals/`, where its subject lives; it was only parked under `lanes/` by a file-ownership constraint while the feature was being built. Adds the one gate finding from /quality that had no test: the `canMutatePinnedProjectUi` change. Every other gate item was already pinned — name-only repo matches, the git-config parser cases, the structural worktree walk, offline lane retention, and per-chat routing not rebinding the tab. This one I made by hand and left uncovered, which is exactly the gap that let dead code ship in the first place. Co-Authored-By: Claude <noreply@anthropic.com>
Documents one-tab-per-repo with a machine dropdown, the cross-machine union Work sidebar, per-chat runtime routing, machine selection at lane creation, and the shared machine/project identity modules. Source file maps updated for the added modules. Removes the dirty-local-work confirmation dialog and `remoteRuntimeCheckLocalWork`, both deleted from the code. Co-Authored-By: Claude <noreply@anthropic.com>
|
@codex review |
4e2b652 to
2f53b20
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f53b202aa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| onOpen={(event) => | ||
| onSelectSession(session.id, event, row.sessions.map((candidate) => candidate.id))} |
There was a problem hiding this comment.
When a foreign row represents a shell or CLI session, this callback only selects the session and never performs the switchRemoteProject promised by docs/features/terminals-and-sessions/README.md:528-530. TerminalView consequently opens under the current binding and calls pty.write, pty.resize, and sessions.get without the owning machine's pin, so the terminal cannot receive its remote output and input is sent to the wrong runtime. Either switch to row.binding before selection or thread that binding through the terminal surface.
AGENTS.md reference: AGENTS.md:L34-L35
Useful? React with 👍 / 👎.
| // Held in a ref so call sites can read it without perturbing any existing | ||
| // effect/callback dependency array. | ||
| const chatRuntimePin = useMemo( | ||
| () => chatMachineRouter.pinForLane(selectedSession?.laneId ?? foreignSelectedLaneId ?? laneId), |
There was a problem hiding this comment.
When this resolves a pin for a chat on another machine, several actions still use the active project runtime: cancelDispatchedSteer at line 4363 and killDroidWorker at line 4431 pass no pin, while their preload implementations at preload.ts:5750-5758 and 6162-6166 do not accept one; continuity recovery has the same gap. Those controls therefore fail with an unknown session or mutate a coincidentally matching session on the active machine. Extend the optional pin contract and use chatRuntimePinRef for all session-scoped operations.
AGENTS.md reference: AGENTS.md:L34-L35
Useful? React with 👍 / 👎.
| const rootPath = thisMachine?.project?.rootPath ?? null; | ||
| return rootPath && thisMachine?.project?.matchedBy === "origin" | ||
| ? { ok: true, rootPath } | ||
| : { ok: false, message: THIS_MACHINE_PROJECT_MISSING_MESSAGE }; |
There was a problem hiding this comment.
For a remote-bound tab, deriveLaneMachineOptions is invoked without local origin metadata and can only mark a local-root match as matchedBy: "name"; this condition then requires "origin", making the success branch unreachable even when the top bar has already grouped an open local checkout with this remote checkout by origin. Selecting This Mac from the composer therefore always reports that the repository must be opened locally. Pass the recent-project origin identity into this resolver or reuse the verified tab group association.
Useful? React with 👍 / 👎.
| // The preload event pump follows the active binding only. A bounded refresh | ||
| // is therefore required for every other connected machine; without it their | ||
| // lane/session rows remain stale indefinitely. | ||
| runtime.refreshTimer = setInterval(scheduleRefresh, FOREIGN_MACHINE_REFRESH_MS); |
There was a problem hiding this comment.
The five-second interval schedules a new runRefresh, which increments runtime.generation even if the previous read is still running, while each read is explicitly allowed eight seconds. On a connected machine whose lane/session calls consistently take between five and eight seconds, every response fails the generation check and no foreign rows are ever merged. Track an in-flight refresh or schedule the next refresh only after the current one settles.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c770b224b6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| request: { cursor, limit: 200 }, | ||
| }) as RemoteRuntimeStreamEventsResult; | ||
| if (cancelled) return; | ||
| cursor = Number.isFinite(batch.nextCursor) ? Math.max(0, Math.floor(batch.nextCursor)) : cursor; |
There was a problem hiding this comment.
When a non-active remote runtime restarts or reconnects, its event buffer gets a new eventEpoch and event IDs restart from 1, but this poller retains the previous cursor. Since polling with a cursor larger than the new buffer's IDs returns no events and preserves that cursor, the pinned chat stops receiving live output until it is remounted or the new runtime produces enough events to pass the old cursor. Track batch.eventEpoch and reset the cursor on an epoch change, as the bound-runtime event pump already does.
Useful? React with 👍 / 👎.
| return; | ||
| } | ||
| setError(null); | ||
| void switching.catch((err: unknown) => { |
There was a problem hiding this comment.
When the user selects another machine's auto-create row and immediately presses Send, this starts the asynchronous project switch without awaiting it or disabling submission. launchDraftSession can therefore capture the old projectBinding and explicitly pin lane/session creation to the previous machine, so the requested work launches on the wrong checkout even if the switch completes moments later. Keep the switch promise as pending state and prevent or await launch until it settles.
Useful? React with 👍 / 👎.
| recent.kind !== "remote" | ||
| && recent.exists | ||
| && openRoots.has(recent.rootPath) | ||
| && recent.rootPath !== projectBinding.rootPath |
There was a problem hiding this comment.
If the remote and local machines use the same absolute path for this repository—a common setup such as /Users/alice/src/ADE on two Macs—this condition rejects the valid local checkout even when it is an open local recent and its verified git origin matches. Remote and local path namespaces are independent, so path equality does not identify the same checkout; the existing kind, open-root, existence, and origin checks are sufficient.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/desktop/src/renderer/components/terminals/TerminalsPage.test.tsx (1)
412-439: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the required rebind-before-open ordering.
These assertions pass even if selection opens before
switchRemoteProjectcompletes, which is the unsafe regression this test is meant to prevent.Proposed test tightening
+ const calls: string[] = []; + workMocks.fns.switchRemoteProject.mockImplementationOnce(async () => { + calls.push("switch"); + }); + workMocks.currentWork.setSelectedSessionId.mockImplementationOnce(() => calls.push("select")); + workMocks.currentWork.openSessionTab.mockImplementationOnce(() => calls.push("open")); + sessionListPaneProps.latest?.onSelectForeignRuntimeSession?.( session, { @@ expect(workMocks.currentWork.openSessionTab).toHaveBeenCalledWith("shell-foreign"); }); + expect(calls).toEqual(["switch", "select", "open"]);🤖 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 `@apps/desktop/src/renderer/components/terminals/TerminalsPage.test.tsx` around lines 412 - 439, Update the test around sessionListPaneProps.latest?.onSelectForeignRuntimeSession to verify that switchRemoteProject resolves before setSelectedSessionId and openSessionTab are invoked. Make the mocked switchRemoteProject asynchronous and assert the ordering explicitly, while preserving the existing argument assertions and selected-session behavior.apps/desktop/src/renderer/state/crossMachineLanes.test.ts (1)
131-155: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFake-timer/global-mock cleanup is not failure-safe.
Both tests restore
vi.useRealTimers()(and, in the scheduling test,window.ade) only as the last statement in the test body. If any earlierexpectthrows, that cleanup is skipped and the mockedDate/timers/window.adeleak into every later test in this file, causing unrelated cascading failures that are hard to trace back to this test.🧹 Suggested fix: restore in afterEach or try/finally
afterEach(() => { resetCrossMachineLaneSyncForTest(); + vi.useRealTimers(); });For the scheduling test, wrap the body (or at least the mock setup/teardown) in
try { ... } finally { stop(); window.ade = originalAde; vi.useRealTimers(); }so cleanup always runs.Also applies to: 439-498
🤖 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 `@apps/desktop/src/renderer/state/crossMachineLanes.test.ts` around lines 131 - 155, Make fake-timer and global mock cleanup failure-safe in the affected tests, including the identity-stability test and the scheduling test around its mock setup. Move restoration into shared afterEach cleanup or finally blocks so vi.useRealTimers() and window.ade restoration always execute when assertions or other test steps throw.
🤖 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 `@apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx`:
- Line 6801: Update the virtualized row rendering around MeasuredEventRow to
forward both onRestoreCancelledQueue and settledQueueRecoveryIds alongside the
existing onRecoverContinuity prop. Ensure these values are passed through to
each row so Undo actions work and settled recovery cards are filtered
consistently with the non-virtualized path.
---
Nitpick comments:
In `@apps/desktop/src/renderer/components/terminals/TerminalsPage.test.tsx`:
- Around line 412-439: Update the test around
sessionListPaneProps.latest?.onSelectForeignRuntimeSession to verify that
switchRemoteProject resolves before setSelectedSessionId and openSessionTab are
invoked. Make the mocked switchRemoteProject asynchronous and assert the
ordering explicitly, while preserving the existing argument assertions and
selected-session behavior.
In `@apps/desktop/src/renderer/state/crossMachineLanes.test.ts`:
- Around line 131-155: Make fake-timer and global mock cleanup failure-safe in
the affected tests, including the identity-stability test and the scheduling
test around its mock setup. Move restoration into shared afterEach cleanup or
finally blocks so vi.useRealTimers() and window.ade restoration always execute
when assertions or other test steps throw.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e7a7a364-6d63-476b-a4cd-ec499e37245a
⛔ Files ignored due to path filters (7)
docs/ARCHITECTURE.mdis excluded by!docs/**docs/features/chat/README.mdis excluded by!docs/**docs/features/lanes/README.mdis excluded by!docs/**docs/features/personal-chats/README.mdis excluded by!docs/**docs/features/remote-runtime/README.mdis excluded by!docs/**docs/features/remote-runtime/internal-architecture.mdis excluded by!docs/**docs/features/terminals-and-sessions/README.mdis excluded by!docs/**
📒 Files selected for processing (49)
apps/ade-cli/src/tuiClient/remoteLaunchBudget.tsapps/ade-cli/src/tuiClient/remoteLauncher.tsapps/desktop/src/main/services/ipc/ipcTimeouts.tsapps/desktop/src/main/services/ipc/runtimeBridge.tsapps/desktop/src/main/services/projects/projectLifecycle.test.tsapps/desktop/src/main/services/projects/recentProjectSummary.test.tsapps/desktop/src/main/services/projects/recentProjectSummary.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.test.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/browserMock.tsapps/desktop/src/renderer/components/app/CommandPalette.test.tsxapps/desktop/src/renderer/components/app/TopBar.test.tsxapps/desktop/src/renderer/components/app/TopBar.tsxapps/desktop/src/renderer/components/app/projectTabGrouping.tsapps/desktop/src/renderer/components/chat/AgentChatComposer.tsxapps/desktop/src/renderer/components/chat/AgentChatMessageList.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.test.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/chat/ChatContinuityRecoveryCard.tsxapps/desktop/src/renderer/components/chat/ChatGitToolbar.tsxapps/desktop/src/renderer/components/chat/thisMachineProjectRoot.test.tsapps/desktop/src/renderer/components/chat/thisMachineProjectRoot.tsapps/desktop/src/renderer/components/lanes/CreateLaneDialog.test.tsxapps/desktop/src/renderer/components/lanes/CreateLaneDialogHost.tsxapps/desktop/src/renderer/components/lanes/CreateLaneDialogHostBinding.test.tsxapps/desktop/src/renderer/components/lanes/CreateLaneDialogMachines.test.tsxapps/desktop/src/renderer/components/lanes/LaneGitActionsPane.tsxapps/desktop/src/renderer/components/lanes/PushDivergenceGuard.test.tsxapps/desktop/src/renderer/components/lanes/laneMachines.test.tsapps/desktop/src/renderer/components/lanes/laneMachines.tsapps/desktop/src/renderer/components/personalChats/PersonalChatsPage.tsxapps/desktop/src/renderer/components/remoteTargets/RemoteTargetList.test.tsxapps/desktop/src/renderer/components/terminals/SessionListPane.test.tsxapps/desktop/src/renderer/components/terminals/SessionListPane.tsxapps/desktop/src/renderer/components/terminals/TerminalsPage.test.tsxapps/desktop/src/renderer/components/terminals/TerminalsPage.tsxapps/desktop/src/renderer/components/terminals/laneComboboxMachineGroups.test.tsxapps/desktop/src/renderer/components/terminals/useWorkSessions.test.tsapps/desktop/src/renderer/components/terminals/useWorkSessions.tsapps/desktop/src/renderer/lib/chatMachineRouting.test.tsapps/desktop/src/renderer/lib/chatMachineRouting.tsapps/desktop/src/renderer/state/appStore.tsapps/desktop/src/renderer/state/crossMachineLanes.test.tsapps/desktop/src/renderer/state/crossMachineLanes.tsapps/desktop/src/shared/ipc.tsapps/desktop/src/shared/laneDivergence.test.tsapps/desktop/src/shared/laneDivergence.tsapps/desktop/src/shared/machineIdentity.ts
💤 Files with no reviewable changes (4)
- apps/desktop/src/renderer/components/lanes/CreateLaneDialogMachines.test.tsx
- apps/desktop/src/renderer/components/remoteTargets/RemoteTargetList.test.tsx
- apps/desktop/src/main/services/ipc/ipcTimeouts.ts
- apps/desktop/src/renderer/components/app/CommandPalette.test.tsx
🚧 Files skipped from review as they are similar to previous changes (29)
- apps/desktop/src/main/services/projects/projectLifecycle.test.ts
- apps/desktop/src/renderer/components/terminals/laneComboboxMachineGroups.test.tsx
- apps/desktop/src/renderer/lib/chatMachineRouting.test.ts
- apps/desktop/src/renderer/components/chat/thisMachineProjectRoot.test.ts
- apps/desktop/src/shared/machineIdentity.ts
- apps/desktop/src/shared/ipc.ts
- apps/desktop/src/renderer/components/app/projectTabGrouping.ts
- apps/desktop/src/renderer/browserMock.ts
- apps/desktop/src/renderer/components/lanes/CreateLaneDialog.test.tsx
- apps/desktop/src/renderer/lib/chatMachineRouting.ts
- apps/desktop/src/renderer/components/lanes/laneMachines.test.ts
- apps/desktop/src/renderer/components/terminals/useWorkSessions.test.ts
- apps/desktop/src/renderer/components/lanes/laneMachines.ts
- apps/desktop/src/shared/laneDivergence.test.ts
- apps/desktop/src/renderer/components/chat/AgentChatComposer.tsx
- apps/desktop/src/renderer/components/app/TopBar.test.tsx
- apps/desktop/src/renderer/components/lanes/PushDivergenceGuard.test.tsx
- apps/desktop/src/main/services/ipc/runtimeBridge.ts
- apps/desktop/src/renderer/components/lanes/CreateLaneDialogHost.tsx
- apps/desktop/src/renderer/components/app/TopBar.tsx
- apps/desktop/src/preload/preload.test.ts
- apps/desktop/src/renderer/state/appStore.ts
- apps/desktop/src/shared/laneDivergence.ts
- apps/desktop/src/renderer/state/crossMachineLanes.ts
- apps/desktop/src/renderer/components/lanes/LaneGitActionsPane.tsx
- apps/desktop/src/renderer/components/terminals/SessionListPane.tsx
- apps/desktop/src/preload/global.d.ts
- apps/desktop/src/preload/preload.ts
- apps/desktop/src/renderer/components/chat/AgentChatPane.tsx
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f0239c8fcb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (const lane of input.localLanes) { | ||
| rows.push({ | ||
| lane, | ||
| machineId: THIS_MACHINE_ID, | ||
| machineName: THIS_MACHINE_NAME, |
There was a problem hiding this comment.
When the tab is bound to a remote checkout, localLanes is actually the active remote binding's lane list, but this loop labels every row as THIS_MACHINE_ID. The refresh also fetches that bound remote target into machines, so the by-lane Work sidebar renders its chats twice—once as local and once as foreign—while chats on This Mac are absent; the same misclassification can make divergence checks compare a remote lane with its duplicate. Derive these rows from the active binding's machine instead of assuming the active lane slice is always local.
Useful? React with 👍 / 👎.
| const chatRuntimePin = useMemo( | ||
| () => chatMachineRouter.pinForLane(selectedSession?.laneId ?? foreignSelectedLaneId ?? laneId), | ||
| [chatMachineRouter, foreignSelectedLaneId, laneId, selectedSession?.laneId], | ||
| ); | ||
| const chatRuntimePinRef = useRef<OpenProjectBinding | null>(chatRuntimePin); |
There was a problem hiding this comment.
When a chat from another machine is selected while the tab remains bound elsewhere, handleDeleteSelectedChat still invokes agentChat.delete without this resolved pin. Preload consequently sends deleteSession to the tab's active runtime rather than the runtime owning the selected chat, so deletion fails and the foreign chat remains. Thread chatRuntimePinRef.current through this selected-chat action just as archive and unarchive already do.
AGENTS.md reference: AGENTS.md:L34-L34
Useful? React with 👍 / 👎.
| cached | ||
| && cached.laneId === trimmedLaneId | ||
| && cached.lanes === state.lanes | ||
| && cached.machines === state.crossMachineLanesByMachineId | ||
| ) { |
There was a problem hiding this comment.
If an offline machine's branch state is queried while it is still inside the 60-second retention window, this cache returns that result indefinitely because its key contains no time component and an already-offline entry does not change on later refreshes. Pushes made after the intended expiry therefore continue warning about arbitrarily stale commits. Bypass memoization for offline entries or incorporate the next expiry deadline into cache validation.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 77d83ed888
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (pin?.kind === "local" && pin.key !== currentProjectBinding?.key) { | ||
| return removeLocal; | ||
| } |
There was a problem hiding this comment.
When a remote-bound tab opens a This Mac chat from the cross-machine union, the local pin differs from currentProjectBinding, so this branch returns with only the legacy IPC.agentChatEvent fanout. Pinned operations execute through IPC.localRuntimeCallAction, while production brain events are delivered through IPC.localRuntimeStreamEvents; the existing runtime pump polls only the active binding. Consequently, assistant text, tool activity, and completion events for this chat are not delivered live. Subscribe to the pinned local root's runtime event stream, analogous to the foreign-remote path.
AGENTS.md reference: AGENTS.md:L34-L34
Useful? React with 👍 / 👎.
| const sessionsById = useMemo(() => { | ||
| const map = new Map(localSessionsById); | ||
| for (const machine of Object.values(crossMachineLanesByMachineId)) { | ||
| for (const session of machine.sessions) { | ||
| if (!map.has(session.id)) map.set(session.id, session); |
There was a problem hiding this comment.
These additions allow a foreign chat to enter openItemIds, but the pruning effect at lines 1489-1505 still constructs validIds exclusively from the local sessions array. Whenever that local list refreshes—such as the five-second running-session refresh, a focus refresh, or a local session event—the effect removes every open foreign session and closes the active foreign chat tab. Build the pruning set from the combined session index or otherwise retain sessions still present in crossMachineLanesByMachineId.
Useful? React with 👍 / 👎.
| const chatRuntimePin = useMemo( | ||
| () => chatMachineRouter.pinForLane(selectedSession?.laneId ?? foreignSelectedLaneId ?? laneId), | ||
| [chatMachineRouter, foreignSelectedLaneId, laneId, selectedSession?.laneId], |
There was a problem hiding this comment.
For a foreign chat this resolves the correct machine pin, but several session-scoped paths still ignore it: the subagent transcript reads around lines 4563/4663, Codex goal mutations around 4702-4737, handoff at 9242, and scheduled-work controls around 10814-10827 all call bridge APIs without the pin, whose preload implementations therefore use the window's active binding. Those controls query or mutate the wrong runtime and normally fail with a missing session. Extend the remaining preload/type signatures and pass chatRuntimePinRef consistently.
AGENTS.md reference: AGENTS.md:L34-L34
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/desktop/src/preload/preload.ts (1)
2966-2970: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider backing off on repeated pinned-poll failures.
A pinned runtime that is down (machine asleep, SSH dropped) keeps this loop invoking IPC every 2 s and logging a warning each time, for as long as the chat stays open. A capped escalating delay would cut both the IPC churn and the console noise.
♻️ Sketch
+ let consecutiveFailures = 0; const poll = async (): Promise<void> => { let delay = REMOTE_RUNTIME_EVENT_IDLE_POLL_MS; try { ... + consecutiveFailures = 0; } catch (error) { if (!cancelled) console.warn("ADE pinned chat event polling failed", error); - delay = 2_000; + consecutiveFailures += 1; + delay = Math.min(2_000 * 2 ** (consecutiveFailures - 1), 30_000); }🤖 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 `@apps/desktop/src/preload/preload.ts` around lines 2966 - 2970, Update the pinned chat polling loop around poll’s catch block to use capped escalating backoff after consecutive failures instead of always resetting delay to 2 seconds. Increase the delay for each repeated failure up to a defined maximum, reset it after a successful poll, and preserve cancellation handling and the existing timer scheduling.
🤖 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 `@apps/desktop/src/renderer/components/chat/AgentChatPane.test.tsx`:
- Around line 5191-5197: Extend the test around finishSwitch() to assert the
deferred draft launch is released after the remote switch settles. After
awaiting the switch completion, verify the relevant create or launch callback is
invoked, while preserving the existing pre-settlement expectation that create
has not been called.
---
Nitpick comments:
In `@apps/desktop/src/preload/preload.ts`:
- Around line 2966-2970: Update the pinned chat polling loop around poll’s catch
block to use capped escalating backoff after consecutive failures instead of
always resetting delay to 2 seconds. Increase the delay for each repeated
failure up to a defined maximum, reset it after a successful poll, and preserve
cancellation handling and the existing timer scheduling.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f4742cfc-eb3a-49e7-a4c2-67da00233cb2
📒 Files selected for processing (21)
apps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.test.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/components/chat/AgentChatMessageList.test.tsxapps/desktop/src/renderer/components/chat/AgentChatMessageList.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.test.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/chat/thisMachineProjectRoot.test.tsapps/desktop/src/renderer/components/chat/thisMachineProjectRoot.tsapps/desktop/src/renderer/components/lanes/LaneGitActionsPane.tsxapps/desktop/src/renderer/components/lanes/PushDivergenceGuard.test.tsxapps/desktop/src/renderer/components/terminals/SessionListPane.tsxapps/desktop/src/renderer/components/terminals/TerminalsPage.test.tsxapps/desktop/src/renderer/components/terminals/TerminalsPage.tsxapps/desktop/src/renderer/components/terminals/useWorkSessions.test.tsapps/desktop/src/renderer/components/terminals/useWorkSessions.tsapps/desktop/src/renderer/lib/agentChatSlashCommandsCache.test.tsapps/desktop/src/renderer/lib/agentChatSlashCommandsCache.tsapps/desktop/src/renderer/state/appStore.tsapps/desktop/src/renderer/state/crossMachineLanes.test.tsapps/desktop/src/renderer/state/crossMachineLanes.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- apps/desktop/src/renderer/components/terminals/TerminalsPage.test.tsx
- apps/desktop/src/renderer/components/chat/thisMachineProjectRoot.ts
- apps/desktop/src/renderer/components/terminals/TerminalsPage.tsx
- apps/desktop/src/renderer/components/terminals/useWorkSessions.ts
- apps/desktop/src/renderer/components/terminals/SessionListPane.tsx
- apps/desktop/src/renderer/components/chat/thisMachineProjectRoot.test.ts
- apps/desktop/src/renderer/state/appStore.ts
- apps/desktop/src/renderer/components/lanes/LaneGitActionsPane.tsx
- apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx
- apps/desktop/src/preload/global.d.ts
- apps/desktop/src/renderer/components/chat/AgentChatPane.tsx
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ec8f7863f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| pin.kind === "remote" | ||
| ? IPC.remoteRuntimeStreamEvents | ||
| : IPC.localRuntimeStreamEvents, |
There was a problem hiding this comment.
When a remote-bound tab opens a chat pinned to This Mac, this polls localRuntimeStreamEvents, but that main-process handler returns an empty batch and delivers events through IPC.runtimeEvent. The runtime notification handler rejects those events because their binding differs from currentProjectBinding, while this callback is registered only with agentChatEventFanout; consequently the pinned chat loads history and accepts actions but never displays live assistant output. The local pinned path needs a binding-aware event subscription consistent with the main IPC contract.
AGENTS.md reference: AGENTS.md:L34-L34
Useful? React with 👍 / 👎.
| void switchProject | ||
| .then(() => handleSelectSession(session.id, event, visibleSessionIds)) |
There was a problem hiding this comment.
When opening a foreign shell or CLI session, handleSelectSession is invoked only after the project switch, but the callback still closes over the source useWorkSessions instance and its source projectStateKey. It therefore writes the session ID into the old project's Work view while the renderer now reads the destination project's view, so the machine changes but the requested shell usually does not open. Apply the selection against the destination scope rather than calling the pre-switch closure.
Useful? React with 👍 / 👎.
| const parent = path.dirname(gitDir); | ||
| if (path.basename(parent) !== "worktrees") return gitDir; | ||
| const commonGitDir = path.dirname(parent); | ||
| if (path.basename(commonGitDir) !== ".git") return gitDir; |
There was a problem hiding this comment.
For a linked worktree whose common Git directory is a bare repository or a separate git dir, the metadata path is still <common>/worktrees/<name>, but <common> is commonly named repo.git rather than .git. This check returns the per-worktree administration directory, causing readGitOriginUrl to look for a nonexistent worktrees/<name>/config and omit gitOriginUrl; those checkouts then fail repository-identity grouping and cross-machine matching. Detect the common directory structurally (for example via commondir) without requiring its basename to be .git.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
ADE/apps/desktop/src/renderer/components/lanes/CreateLaneDialogHost.tsx
Lines 526 to 528 in a8afc8b
Require an origin match before rebinding for lane creation
When the dialog starts from a local project, repoOriginUrl is always null, so deriveLaneMachineOptions may return a remote checkout based only on a matching folder/display name with matchedBy: "name" and repoMatch: "unknown"; canCreateLaneOnMachine permits that state, and this path immediately rebinds to its project. If another connected machine has an unrelated same-named repository, selecting it creates the lane in the wrong repository. Resolve the local origin first or refuse to rebind unless the candidate has a proven origin match; the draft lane picker has the same unchecked path.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const multiMachinePossible = | ||
| showDraftLaunchControls | ||
| && (openRemoteProjectTabs.length > 0 || projectBinding?.kind === "remote"); |
There was a problem hiding this comment.
For a local project with a connected remote machine but no remote project tab currently open, this predicate is false, so the snapshot is never fetched and the draft lane combobox cannot offer that machine. This is a common state after pairing a machine or closing its project tab, and it makes the new per-machine lane creation unavailable until the user first opens a remote tab. Gate only on the draft controls and let the connection snapshot determine whether a second machine exists.
Useful? React with 👍 / 👎.
1 participant
Follow-up to #912 that completes the unified project/machine model after
/qualityfound several shipped paths had no production caller.What this finishes
Quality and test gates
/quality: completed; all four High reachability findings were wired, plus the reachable wrong-repository rebind bug and related Medium/Low findings./test: completed; test files consolidated, a missing pinned-UI regression test added, and docs/mobile/CLI/TUI parity checked.CreateLaneDialog.test.tsxpasses 10/10 and desktop typecheck is clean.Review focus
Summary by CodeRabbit