Repository navigation
fix(runtime): isolate navigation across paired clients - #9664
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (17)
🚧 Files skipped from review as they are similar to previous changes (10)
📝 WalkthroughWalkthroughThe change introduces explicit runtime navigation targets and paired-device identity propagation through RPC handling. Runtime tab snapshots now maintain per-client selection state, project active tabs independently, and scope idempotency and subscriptions by client. Mobile, renderer, and CLI flows send caller-local activation and selection options, while terminal focus can avoid host navigation. It also adds legacy notification redirects, AI vault resume-preparation handling, web-client packaging fallbacks, and tests for schema validation, reconnects, and multi-client isolation. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
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.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/main/runtime/client-session-tab-selection.test.ts (1)
91-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest doesn't actually exercise "fallback within the selected group."
The
removedfixture dropsgroup-right(and all browser tabs) entirely, so resolution falls through to the globalsnapshot.tabs[0]fallback rather than the in-group fallback path (selection.activeTabIdByGroupId[selectedGroup.id]inclient-session-tab-selection.ts, lines 79-84) that the test name implies. Consider adding a case where the selected group still exists but only the specific selected tab within it disappears (e.g., a sibling leaf tab), to cover that branch on this isolation-critical helper.src/main/runtime/client-session-tab-selection.ts (1)
78-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNon-obvious fallback chain lacks a "why" comment.
The tab-resolution fallback chain and per-group
activeTabIdrecomputation are logically correct (verified againstclient-session-tab-selection.test.ts) but involve several layered??fallbacks with implicit precedence (previous exact tab → in-group tracked tab → group's first tab → global first tab). A brief comment on why this precedence order exists (e.g., preferring the most specific prior selection before falling back) would help future maintainers, consistent with the rest of the file (e.g., line 149's rationale comment).As per coding guidelines, "For non-obvious TypeScript code, add a concise comment explaining why the constraint exists, not how the code works."
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 20da6655-b0f3-4cf0-ba1b-282678ada5f9
📒 Files selected for processing (41)
mobile/app/h/[hostId]/index.tsxmobile/app/h/[hostId]/session/[worktreeId].tsxmobile/src/session/ai-vault-resume-launch.test.tsmobile/src/session/ai-vault-resume-launch.tsmobile/src/session/mobile-session-startup-source.test.tsmobile/src/session/mobile-session-tab-activation.test.tsmobile/src/session/mobile-session-tab-activation.tsmobile/src/session/pr-ai-triage-launch.test.tsmobile/src/session/pr-ai-triage-launch.tsmobile/src/session/use-mobile-diff-review-send-actions.tsmobile/src/worktree/mobile-worktree-activation-source.test.tssrc/main/runtime/client-session-tab-selection.test.tssrc/main/runtime/client-session-tab-selection.tssrc/main/runtime/multi-client-navigation-isolation.integration.test.tssrc/main/runtime/orca-runtime.test.tssrc/main/runtime/orca-runtime.tssrc/main/runtime/rpc/core.tssrc/main/runtime/rpc/dispatcher.tssrc/main/runtime/rpc/methods/session-tabs-schemas.test.tssrc/main/runtime/rpc/methods/session-tabs-schemas.tssrc/main/runtime/rpc/methods/session-tabs.test.tssrc/main/runtime/rpc/methods/session-tabs.tssrc/main/runtime/rpc/methods/terminal-navigation.test.tssrc/main/runtime/rpc/methods/terminal.tssrc/main/runtime/rpc/methods/worktree-schemas.test.tssrc/main/runtime/rpc/methods/worktree-schemas.tssrc/main/runtime/rpc/methods/worktree.test.tssrc/main/runtime/rpc/methods/worktree.tssrc/main/runtime/runtime-rpc.tssrc/renderer/src/components/terminal-pane/remote-runtime-pty-transport.test.tssrc/renderer/src/components/terminal-pane/remote-runtime-pty-transport.tssrc/renderer/src/lib/worktree-activation-created-agent.test.tssrc/renderer/src/lib/worktree-activation-empty-remote.test.tssrc/renderer/src/lib/worktree-activation.tssrc/renderer/src/runtime/web-runtime-session.test.tssrc/renderer/src/runtime/web-runtime-session.tssrc/renderer/src/runtime/web-session-tabs-sync.test.tssrc/renderer/src/runtime/web-session-tabs-sync.tssrc/shared/runtime-navigation.test.tssrc/shared/runtime-navigation.tssrc/shared/runtime-types.ts
|
Added local browser-level multi-client E2E coverage in The new Validation:
|
…active-worktree-isolation # Conflicts: # src/main/runtime/orca-runtime.ts
Summary
caller | host | clients | allnavigation intent with live-only explicit follow delivery.Screenshots
No visual change.
Root cause
Remote worktree activation called the host notifier and broadcast an unscoped client navigation event. Session-tab activation also rewrote the single shared
mobileSessionTabsByWorktreeactive selection, so every subscriber consumed the last writer as navigation. Existing echo suppression stopped a loop but did not establish ownership.Deterministic reproduction
src/main/runtime/multi-client-navigation-isolation.integration.test.tsstarts realOrcaRuntimeRpcServerinstances and authenticates independent E2EE WebSocket sessions from distinct pairing offers/device IDs. It covers:The harness uses message barriers rather than sleeps and exercises the real dispatcher, pairing registry, encrypted WebSocket transport, subscriptions, runtime routing, graph-backed snapshots, and per-device projection.
Run locally on macOS or Linux; Windows and Docker are not required:
pnpm exec vitest run --config config/vitest.config.ts src/main/runtime/multi-client-navigation-isolation.integration.test.ts pnpm test:e2e:multi-client-navigationThe E2E command builds Orca and its paired web client, launches one real Electron runtime host, and opens two clients in isolated Electron Chromium partitions.
Testing
pnpm lint— focused oxlint, React Doctor, max-lines ratchet, and reliability gates pass; the repository-wide switch-exhaustiveness command reports pre-existing errors in untouchedskill-freshness-group.tsxanddaemon-server.ts.pnpm typecheckpnpm test— proportionate targeted suites were run rather than every repository test.pnpm build— Electron E2E and paired web production builds completed successfully.Exact results:
git diff --checkpassed.AI Review Report
Ran
$internal-review-until-cleananchored at merge base0e7b0ac22079f4ab6dba708bb595daaa627e3285, including pre-review and post-fix elegance interrogation, a performance audit, and two adversarial review rounds.The review checked navigation ownership, paired identity vs. connection identity, reconnect/restart/stale-snapshot behavior, graph fanout, explicit follow routing, mixed versions, session-tab fallback, terminal creation/focus, privacy boundaries, and cross-platform behavior on macOS, Linux, Windows, WSL, and SSH. No platform heuristic, shortcut, label, shell, path, or platform-specific Electron behavior was added.
It validated two CodeRabbit findings and fixed them:
terminal.focuscould still steer the host implicitly; it now uses the central navigation resolver, while intentional CLI/renderer reveals send explicitnavigation: 'host';It also corrected the selected-group fallback test and added a concise ownership-precedence comment. The final post-fix round found no additional actionable issue.
The final design supersedes PR #9029 rather than reusing it unchanged. #9029 suppresses ordinary host focus but still rewrites/fans out one global active session-tab snapshot, so it needs rework to provide independent client selections.
Security Audit
Reviewed RPC schema validation, authenticated device identity, pairing credentials, transient connection IDs, IPC boundaries, navigation-state disclosure, command/path handling, secrets, and dependency impact.
No security follow-up is required for this change.
Notes
notifyClients:false; terminal creation also sendsactivate:false. New hosts treat older paired clients as caller-local by default.