Repository navigation
feat(sidebar): main keeps one live set of paired-server rows, checked against the window in dev and test - #30027
Conversation
… against the window in dev and test
…-rows # Conflicts: # src/main/runtime/rpc/methods/sidebar-inputs.ts # src/renderer/src/lib/sidebar-shadow/sidebar-shadow-comparison.ts # src/renderer/src/lib/sidebar-shadow/workspace-activity-shadow.ts # src/renderer/src/main.tsx # src/shared/rpc-contract/rpc-params-catalog.generated.ts
📝 Walkthrough
Priority: ➖ Normal Merge Risk: 🔵 Low · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 47.06% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 34 functions across 27 files. (1 skipped:… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly identifies the main change: a single live paired-server row set maintained by the main process and checked against the window. It is specific and related to the changeset. Description check The description is detailed and covers the required explanation, changes, rationale, testing, visual proof, review notes, compatibility considerations, and checklist. It does not link an issue, but it… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Description check
-
Explanation
The description is detailed and covers the required explanation, changes, rationale, testing, visual proof, review notes, compatibility considerations, and checklist. It does not link an issue, but it explains that the change is part of an existing sidebar refactor plan.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 47.06% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 34 functions across 27 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
- Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
cf9d97c2-4642-45cd-9ca6-2bb6ab7db380
⛔ Files ignored due to path filters: 1
src/shared/rpc-contract/rpc-params-catalog.generated.tsis excluded by!**/*.generated.*
📒 Files selected for processing: 28
src/main/ipc/runtime-environment-mobile-relay-hosts.tssrc/main/ipc/runtime-environment-status-owner.tssrc/main/runtime/mobile-desktop-relay/mobile-desktop-relay-hosts.tssrc/main/runtime/mobile-desktop-relay/mobile-desktop-relay.integration.test.tssrc/main/runtime/mobile-desktop-relay/mobile-desktop-relay.test.tssrc/main/runtime/mobile-desktop-relay/mobile-relay-host-catalog.integration.test.tssrc/main/runtime/mobile-desktop-relay/mobile-relay-host-catalog.test.tssrc/main/runtime/mobile-desktop-relay/mobile-relay-host-catalog.tssrc/main/runtime/mobile-desktop-relay/mobile-relay-host-descriptions.tssrc/main/runtime/orca-runtime.tssrc/main/runtime/rpc/methods/sidebar-inputs.tssrc/main/runtime/runtime-rpc/runtime-rpc-pairing.tssrc/main/runtime/runtime-rpc/runtime-rpc-shutdown.tssrc/main/runtime/runtime-rpc/runtime-rpc-state.tssrc/main/runtime/sidebar-inputs/paired-server-rows-producer.test.tssrc/main/runtime/sidebar-inputs/paired-server-rows-producer.tssrc/renderer/src/assets/mobile-page.csssrc/renderer/src/components/native-chat/use-native-chat-paste-lifetime.tssrc/renderer/src/lib/sidebar-shadow/paired-server-rows-shadow.test.tssrc/renderer/src/lib/sidebar-shadow/paired-server-rows-shadow.tssrc/renderer/src/lib/sidebar-shadow/sidebar-shadow-comparison.tssrc/renderer/src/lib/sidebar-shadow/workspace-activity-shadow.test.tssrc/renderer/src/lib/sidebar-shadow/workspace-activity-shadow.tssrc/renderer/src/main.tsxsrc/shared/paired-server-hosts.tssrc/shared/sidebar-list/paired-server-rows.tstests/e2e/paired-mobile-desktop-mirror-workspace-visibility.spec.tstests/e2e/paired-server-rows-shadow.spec.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review.
| for (const entry of this.servers.values()) { | ||
| const inContact = shown.get(entry.environmentId)?.health === 'available' | ||
| if (inContact !== entry.inContact) { | ||
| entry.inContact = inContact | ||
| entry.confirmed = false | ||
| this.revision += 1 | ||
| } | ||
| // Why current(): a replaced transport can end the stream without telling it. | ||
| if (inContact && !entry.watch?.current()) { | ||
| entry.watch?.close() | ||
| this.watch(hosts, entry) | ||
| } else if (!inContact && entry.watch) { | ||
| entry.watch.close() | ||
| entry.watch = null | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '108,155p' src/main/runtime/sidebar-inputs/paired-server-rows-producer.ts
sed -n '205,251p' src/main/runtime/sidebar-inputs/paired-server-rows-producer.tsRepository: stablyai/orca
Length of output: 3274
🏁 Script executed:
sed -n '1,235p' src/main/runtime/sidebar-inputs/paired-server-rows-producer.tsRepository: stablyai/orca
Length of output: 8678
Fetch rows when contact begins.
When inContact changes to true, sync() starts the watch but does not call refresh(). A subscription can close without an onChange callback, and the fallback requires demand. The server can therefore remain without rows after contact. Add the refresh call without adding another revision increment. The existing refresh coalescing and refreshAgain handling prevent a fetch loop.
Suggested fix
entry.inContact = inContact
entry.confirmed = false
this.revision += 1
+ if (inContact) {
+ void this.refresh(entry.environmentId)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (const entry of this.servers.values()) { | |
| const inContact = shown.get(entry.environmentId)?.health === 'available' | |
| if (inContact !== entry.inContact) { | |
| entry.inContact = inContact | |
| entry.confirmed = false | |
| this.revision += 1 | |
| } | |
| // Why current(): a replaced transport can end the stream without telling it. | |
| if (inContact && !entry.watch?.current()) { | |
| entry.watch?.close() | |
| this.watch(hosts, entry) | |
| } else if (!inContact && entry.watch) { | |
| entry.watch.close() | |
| entry.watch = null | |
| } | |
| } | |
| for (const entry of this.servers.values()) { | |
| const inContact = shown.get(entry.environmentId)?.health === 'available' | |
| if (inContact !== entry.inContact) { | |
| entry.inContact = inContact | |
| entry.confirmed = false | |
| this.revision += 1 | |
| if (inContact) { | |
| void this.refresh(entry.environmentId) | |
| } | |
| } | |
| // Why current(): a replaced transport can end the stream without telling it. | |
| if (inContact && !entry.watch?.current()) { | |
| entry.watch?.close() | |
| this.watch(hosts, entry) | |
| } else if (!inContact && entry.watch) { | |
| entry.watch.close() | |
| entry.watch = null | |
| } | |
| } |
ELI5
The phone and desktop workspace lists should come from one source: Orca's background process ("main"). For workspaces on a paired Orca server, main used to fetch the server's list only when a phone asked, and the desktop window fetched its own copy separately. Now main keeps one live copy of every paired server's workspace rows by itself, even with no window open. The phone reads that copy. In dev and test builds the window checks main's copy against its own and counts any disagreement. Nothing visible changes.
What Changed
No visible change. The phone gets the same rows as before.
src/main/runtime/sidebar-inputs/paired-server-rows-producer.ts).unverifiable, never treated as gone. Only removing or re-pairing the server drops them. A late answer from before a re-pair is discarded.mobile-relay-host-catalog.ts). It reads main's set, refreshing first as it did before.runtime:<server>|<workspace id>, so a local workspace and a server workspace with the same id can't collide. The activity comparison from feat(sidebar): main produces per-workspace activity facts, checked against the window in dev and test #29980 now also leaves out an id that both a local and a server workspace use, because the window keys activity by bare id and can't tell which host it belongs to.describePairedServerHosts, computes them for the phone catalog, main's set, and the window's comparison.sidebarInputs.pairedServerRows, a local, host-owned method.paired-server-rowsandpaired-server-hosts.readWindownow receives main's read, so a domain can limit the window's side to what main covers.Older servers. Main still uses
worktree.psandruntime.clientEvents.subscribe, which every server already supports. Missing optional fields get defaults (for example, a missingworkspaceKindreads as a regular git workspace), and an unknown workspace kind falls back the same way instead of being rejected. A server that sends no change events is still covered by the 30 s fallback and by phone reads.Why
This is step 5d of the sidebar plan. It settles the first must-fix in the design review, "One normalizer is not one live input source": one aggregate in main, initial population, subscriptions that work without a window, deletions removed, and loss of contact reported as unverifiable.
Alternatives considered:
runtime-environment-subscription-handlers.ts). Rejected: they are tied to the renderer's lifetime, and a desktop without a window still has to serve its phone.Linked Issue
Part of the sidebar refactor plan (step 5d). No issue.
Visual Proof
N/A. Nothing visible changes. The comparison is development tooling and does not run in production builds.
Testing
I manually tested these changes locally
Automated tests added/updated, or explained why not below
Producer unit tests:
Catalog unit tests updated. One race now reads better: a phone request made just before a re-pair gets the new server's rows instead of none, and still never the old server's.
Shadow unit tests: the window-side reader, and the scoping that leaves out an id both hosts use.
e2e on macOS with a hidden window (
ORCA_BACKGROUND_LAUNCH=1):paired-server-rows-shadowruns a desktop paired to anorca servehost. Workspaces created on the server itself reach main with no phone involved. Pinning and deleting on the server follow, and the rows read unverifiable (and are kept) while the server process is down, then live again.expectNoSidebarShadowMismatcheswas added to that spec and topaired-mobile-desktop-mirror-workspace-visibility.paired-mobile-desktop-mirrorand the three sidebar specs from feat(sidebar): main produces per-workspace activity facts, checked against the window in dev and test #29980 pass, run twice each (14/14), with zero disagreements.Positive controls, not committed: changing a server row's name only in the window's store, and clearing the window's server status, were each caught by their domain.
Gates:
pnpm tc, oxlint,check:code-quality:changed, the focus-read ratchet, the runtime-electron ratchet, the rpc params catalog check, and the census tests overALL_RPC_METHODS.desktop-browser-bridge-wiring(nonode:sqlite) and three socket integration suites time out. The same catalog integration test also fails in a lane at origin/main. All of them pass under Node.Review
Opus elegance and bug review, two rounds, all findings applied:
Agent skill upstream boundary
Notes