Remote Machine Indicators -> Primary - #984
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
Warning Review limit reached
Next review available in: 44 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 (2)
📝 WalkthroughWalkthroughThe PR baselines PR merge settlement notifications and adds physical machine identity handling across cross-machine lanes, command-palette thread indexing, session cards, and running chat composers. Tests cover historical merges, remote attribution, lane rendering, marker states, and machine chips. ChangesPR merge settlement
Machine identity UI
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/components/app/commandPaletteThreads.tsx (1)
156-168: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDerive
machineOnlinefor a remote-bound tab instead of hardcodingtrue.
buildThreadIndexattributes the tab's own sessions toactiveMachine, then always setsmachineOnline: true. The id dedupe at Line 178 drops that machine's foreign slice, so its realonlinevalue never reaches the entry. When the bound machine goes unreachable, its rows keep the live treatment:ThreadResultRowcomputesofflinefromentry.machineOnline, sodata-dimmedand the receded styling never apply. That contradicts the stated rule that an unreachable machine's rows recede rather than look live.Read the connectivity from the matching foreign slice when
activeMachineis set.🐛 Proposed fix
const laneById = new Map(lanes.map((lane) => [lane.id, lane] as const)); const seen = new Set<string>(); const entries: ThreadIndexEntry[] = []; + // The bound machine's own slice is skipped by the id dedupe below, so its + // reachability has to be read here or it is lost. + const activeMachineOnline = activeMachine + ? foreignMachines[activeMachine.machineId]?.online ?? true + : true; for (const session of sessions) { seen.add(session.id); entries.push( makeEntry({ session, lane: laneById.get(session.laneId) ?? null, machineId: activeMachine?.machineId ?? THIS_MACHINE_ID, machineName: activeMachine?.machineName ?? THIS_MACHINE_NAME, - machineOnline: true, + machineOnline: activeMachineOnline, binding: null, }), ); }🤖 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/commandPaletteThreads.tsx` around lines 156 - 168, Update the session-entry construction in buildThreadIndex to derive machineOnline from the matching foreign slice when activeMachine is set, rather than hardcoding true. Preserve true for local or otherwise unbound tabs, and ensure the value reflects the bound machine’s actual online state so ThreadResultRow can apply offline styling.
🤖 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.
Outside diff comments:
In `@apps/desktop/src/renderer/components/app/commandPaletteThreads.tsx`:
- Around line 156-168: Update the session-entry construction in buildThreadIndex
to derive machineOnline from the matching foreign slice when activeMachine is
set, rather than hardcoding true. Preserve true for local or otherwise unbound
tabs, and ensure the value reflects the bound machine’s actual online state so
ThreadResultRow can apply offline styling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5b617461-1b0b-4fe8-a462-5e4c91e1b238
⛔ Files ignored due to path filters (7)
docs/features/chat/README.mdis excluded by!docs/**docs/features/chat/composer-and-ui.mdis excluded by!docs/**docs/features/pull-requests/README.mdis excluded by!docs/**docs/features/search/README.mdis excluded by!docs/**docs/features/sync-and-multi-device/README.mdis excluded by!docs/**docs/features/terminals-and-sessions/README.mdis excluded by!docs/**docs/features/terminals-and-sessions/ui-surfaces.mdis excluded by!docs/**
📒 Files selected for processing (14)
apps/desktop/src/main/services/prs/prAsync.test.tsapps/desktop/src/main/services/prs/prMergeAutoSettlementService.tsapps/desktop/src/renderer/components/app/CommandPalette.test.tsxapps/desktop/src/renderer/components/app/CommandPalette.tsxapps/desktop/src/renderer/components/app/commandPaletteThreads.tsxapps/desktop/src/renderer/components/chat/AgentChatComposer.test.tsxapps/desktop/src/renderer/components/chat/AgentChatComposer.tsxapps/desktop/src/renderer/components/terminals/LaneMachineMarker.tsxapps/desktop/src/renderer/components/terminals/SessionCard.test.tsxapps/desktop/src/renderer/components/terminals/SessionCard.tsxapps/desktop/src/renderer/components/terminals/SessionListPane.test.tsxapps/desktop/src/renderer/components/terminals/SessionListPane.tsxapps/desktop/src/renderer/state/crossMachineLanes.test.tsapps/desktop/src/renderer/state/crossMachineLanes.ts
|
@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: 9026152a2e
ℹ️ 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".
| // — asserted by reading the same map rather than assumed, so this stays | ||
| // correct if foreign handoffs ever land. | ||
| if ((unfilteredHandoffCountByLaneId.get(compositeLaneId) ?? 0) > 0) continue; | ||
| if (isHeaderlessRoster(row.sessions)) ids.add(compositeLaneId); |
There was a problem hiding this comment.
When a foreign lane contains one parent chat plus spawned child sessions, isHeaderlessRoster returns true because it counts only the parent as top-level. Unlike the local path, however, the foreign branch of renderCards maps every session flat rather than nesting or excluding children, so this removes the lane header even though multiple cards are rendered and applies the singleton lane identity, machine marker, and lane actions to every child. Keep the header for this foreign roster until its child sessions are rendered as one nested unit.
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: 6dcda8f355
ℹ️ 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".
| "inline-flex h-6 shrink-0 items-center gap-1 px-1", | ||
| "font-sans text-[9px] font-medium text-muted-fg/60", | ||
| )} | ||
| style={{ whiteSpace: "nowrap" }} |
There was a problem hiding this comment.
When a remote runtime has a long display name, especially in a narrow grid-tile composer, this shrink-0 and nowrap label has no maximum width. Because it sits in the toolbar's horizontally scrolling region with its scrollbar hidden, the new machine indicator can extend beyond the visible strip and become unreachable or obscure adjacent model controls. Constrain and truncate the visible label while retaining the full name in the tooltip and accessible label.
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: 381914db4a
ℹ️ 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".
| ): ThreadIndexEntry[] { | ||
| const laneById = new Map(lanes.map((lane) => [lane.id, lane] as const)); | ||
| const activeMachineOnline = activeMachine | ||
| ? (foreignMachines[activeMachine.machineId]?.online ?? true) |
There was a problem hiding this comment.
When a remote-bound project is restored while its target is disconnected and there is no retained crossMachineLanesByMachineId slice for that target, this ?? true marks every cached thread as online. The palette consequently omits its offline dimming and presents stale running state as live even though the remoteSnapshot already loaded by CommandPalette reports the target as disconnected. Derive this value from the target's connection snapshot, reserving the online default for a locally bound project.
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: e2036d6a51
ℹ️ 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".
| // Single-machine setups take this branch forever: no marker map is built and | ||
| // the lane header renders exactly as it did before this feature existed. | ||
| if (foreignRows.length === 0) return EMPTY_CROSS_MACHINE_UNION; | ||
| if (!rows.some((row) => !row.isThisMachine)) { |
There was a problem hiding this comment.
When a project remains bound to a remote target after that target disconnects, this branch now exposes markers for the active-binding lanes, but buildCrossMachineLaneRows hardcodes those rows to online: true and drops the retained slice for the same machine. The sidebar consequently keeps the lane undimmed and its marker describes the target as connected even though the connection snapshot reports it offline; derive the active row's reachability from the bound target connection before returning these markers.
AGENTS.md reference: AGENTS.md:L59-L60
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: 6f13ee77bd
ℹ️ 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 activeMachineOnline = activeRemoteBinding | ||
| ? input.machines[activeRemoteBinding.targetId]?.online ?? true |
There was a problem hiding this comment.
When a remotely bound target disconnects without a retained slice, this fallback leaves all of its cached lanes online: true indefinitely. Fresh evidence in this revision is that the new source cannot normally exist for this case: resolveEligibleMachines explicitly excludes scope.boundTargetId, and applyCrossMachineLaneScope clears the machine record when the binding scope changes, so the connection snapshot has no path into input.machines[activeRemoteBinding.targetId]. The active lane markers therefore continue to report the disconnected target as connected; propagate the bound target's connection-derived reachability into these rows instead of defaulting it to live.
Useful? React with 👍 / 👎.
| // exactly one thing, "this work isn't here", and no lane type is exempt. | ||
| // Note this list is the ACTIVE BINDING's lanes, which is not necessarily | ||
| // this machine: bind the tab to another Mac and every lane here is marked. | ||
| const machineMarker = markersByLaneId.get(lane.id) ?? null; |
There was a problem hiding this comment.
When a retained active-target slice does report online: false, the resulting marker is consumed only by LaneMachineMarker; the active lane still takes renderLaneGroup, whose collapsed state ignores reachability and whose StickyGroupHeader never receives dimmed. Fresh evidence is that the newly propagated false state therefore only dims the small glyph while the lane and its cards remain full-opacity and open, unlike every other offline machine group. Use machineMarker.online to apply the same offline group treatment to active-binding lanes.
Useful? React with 👍 / 👎.
| ? renderCards(list, { | ||
| showLaneIdentity: true, | ||
| lanePr: primaryPr, | ||
| machineMarker, |
There was a problem hiding this comment.
When the tab is remote-bound and one of its lanes has two or more sessions, only the header receives machineMarker; the grouped cards get suppressMachineChip but neither a foreignRow/runtimePin nor the marker. SessionCard derives its hover-card Machine row from exactly those values, so hovering any grouped session omits its owning machine even though the header identifies it. Pass the marker through to grouped cards as metadata while continuing to suppress the duplicate inline glyph.
Useful? React with 👍 / 👎.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes