Repository navigation
feat(sidebar): put main's sidebar inputs behind the ORCA_SIDEBAR_INPUTS dark build switch - #30050
Conversation
…TS dark build switch
📝 Walkthrough
Priority: ⬇️ Low Merge Risk: 🔵 Low · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 31.03% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 29 functions across 29 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly and concisely describes the primary change: placing main sidebar inputs behind the ORCA_SIDEBAR_INPUTS dark build switch. Description check The description is complete and focused. It includes the ELI5 summary, scope, rationale, linked issue context, visual-proof explanation, detailed testing, review notes, compatibility considerations, a… 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 complete and focused. It includes the ELI5 summary, scope, rationale, linked issue context, visual-proof explanation, detailed testing, review notes, compatibility considerations, and checklist status. The template's Fixes # field is not populated with a direct issue link, but the description references the relevant follow-up issues and is otherwise substantially complete.
-
- 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:
80e509b5-6417-4d22-b544-088b7e10ef8c
📒 Files selected for processing: 31
config/dark-build-switches.cjsconfig/electron-builder.config.cjsconfig/layout-owner-build.cjsconfig/scripts/build-orcad-template.mjsconfig/scripts/dark-build-switches.test.mjsconfig/scripts/generate-rpc-params-catalog.mjsconfig/scripts/headless-detector-compiler-cache.test.mjsconfig/scripts/layout-owner-build.test.mjsconfig/scripts/orcad-entry-build.mjselectron.vite.config.tssrc/main/orcad/orcad-entry.tssrc/main/orcad/orcad-push-startup.test.tssrc/main/runtime/mobile-desktop-relay/mobile-relay-host-catalog.tssrc/main/runtime/mobile-desktop-relay/on-demand-server-worktree-rows.test.tssrc/main/runtime/mobile-desktop-relay/on-demand-server-worktree-rows.tssrc/main/runtime/mobile-desktop-relay/server-worktree-fetch.tssrc/main/runtime/orca-runtime.tssrc/main/runtime/rpc/methods/index.tssrc/main/runtime/rpc/methods/sidebar-inputs.tssrc/main/runtime/runtime-rpc/runtime-rpc-pairing.tssrc/main/runtime/sidebar-inputs/orca-runtime-workspace-activity-facts.test.tssrc/main/runtime/sidebar-inputs/orca-runtime-workspace-review-facts.test.tssrc/main/runtime/sidebar-inputs/orca-runtime-workspace-review-facts.tssrc/main/runtime/sidebar-inputs/paired-server-rows-producer.tssrc/main/runtime/sidebar-inputs/sidebar-inputs-switch-off.test.tssrc/main/startup/main-process-runtime-service.tssrc/renderer/src/main.tsxsrc/shared/sidebar-inputs-switch.tstests/e2e/helpers/sidebar-shadow.tstests/e2e/paired-server-rows-shadow.spec.tstests/e2e/sidebar-review-facts.spec.ts
💤 Files with no reviewable changes: 2
- config/scripts/layout-owner-build.test.mjs
- config/layout-owner-build.cjs
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review.
| if ( | ||
| !reply?.success || | ||
| !this.hosts.list().environments.some((entry) => entry.id === host.environmentId) | ||
| ) { | ||
| return | ||
| } | ||
| this.cached.set(host.environmentId, { | ||
| // Why the identity taken before the fetch: the call's own check cannot see a re-pair after dispatch. | ||
| fence: relayHostFence(host.identity), | ||
| worktrees: stampServerWorktreeRows(host.environmentId, reply.data.worktrees) | ||
| }) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Do not let a late answer from an old pairing overwrite the current pairing's cached rows.
refresh keys in-flight fetches by environmentId and fence. After a re-pair, a fetch under the old pairing and a fetch under the new pairing can therefore run at the same time. cached is keyed only by environmentId. The guard at lines 72-75 checks only that the environment still exists. The comment at line 79 states that the transport's expected check cannot detect a re-pair after dispatch, so the old call can still succeed.
The test at lines 311-332 covers this sequence. The new-pairing fetch stores its rows first. The old-pairing answer then arrives and replaces them with an entry that has the old fence. rows(env, newFence) then returns null. Suppose the server becomes unreachable, or the next refresh fails, before another fetch succeeds. In that case the phone gets { worktrees: null } instead of the last rows from the current pairing. This contradicts the "keeps its last rows" contract.
Store the rows only when the current listing's fence still equals the fence captured before the fetch.
🐛 Proposed fix
--- "a/src/main/runtime/mobile-desktop-relay/on-demand-server-worktree-rows.ts"
+++ "b/src/main/runtime/mobile-desktop-relay/on-demand-server-worktree-rows.ts"
@@ -67,19 +67,19 @@
// Why the transport bound too: it ends the hung call, so the next poll fetches afresh instead of joining it.
{ timeoutMs: SERVER_FETCH_TIMEOUT_MS, expected: host.identity }
)
const reply = response.ok ? WorktreePsReplySchema.safeParse(response.result) : null
- // Why: a removed server's late answer must not outlive it.
- if (
- !reply?.success ||
- !this.hosts.list().environments.some((entry) => entry.id === host.environmentId)
- ) {
+ const fence = relayHostFence(host.identity)
+ const current = this.hosts
+ .list()
+ .environments.find((entry) => entry.id === host.environmentId)
+ // Why: a removed or re-paired server's late answer must not replace newer rows.
+ if (!reply?.success || !current || relayHostFence(current) !== fence) {
return
}
this.cached.set(host.environmentId, {
- // Why the identity taken before the fetch: the call's own check cannot see a re-pair after dispatch.
- fence: relayHostFence(host.identity),
+ fence,
worktrees: stampServerWorktreeRows(host.environmentId, reply.data.worktrees)
})
} catch {
// An unreachable server keeps its last rows.
}Add an assertion to the re-pair test: after answerOldFetch, mark the server unreachable, call worktrees, and expect new-server-row.
ELI5
Over the last few PRs, the main process started building the sidebar's inputs itself (agent and terminal activity per workspace, PR state, focus and pins, and a live list of each paired server's workspaces) and checking them against what the window computes. None of that is used by the sidebar yet, but it already runs in every build. This PR puts all of it behind a build switch,
ORCA_SIDEBAR_INPUTS=main, exactly like the existing layout-owner switch. Without the switch (every shipped build), none of it is built, started, or registered, and the phone's server list goes back to fetching a server's workspaces only when the phone asks.What Changed
src/shared/sidebar-inputs-switch.tsexportsORCA_SIDEBAR_INPUTS_IS_MAIN, filled by the electron-vite define and absent (off) in tests, the CLI and the web client. Every gate branches on that constant.config/layout-owner-build.cjsbecomesconfig/dark-build-switches.cjs, listing both switches. Each switch gets its define, itsout/mainmarker, and the packaging and orcad-template refusals. The layout switch keeps its variable, marker file name, marker text and error messages.runtime.sidebarFacts, which is null when off.runtime.pairedServerRows(the live server rows, their per-server event streams and the 30 s refresh) is null when off.sidebarInputs.*RPC methods are registered only when the switch is on.OnDemandServerWorktreeRowsis the old catalog cache moved behind a small interface (ServerWorktreeRows), and the producer implements the same interface. The old catalog test file is restored next to it and runs against it, unchanged apart from the new constructor argument and two stub fields. The one difference: a removed server's late answer is dropped when it arrives, where it used to be dropped at the next list read. The result is the same, because server ids are never reused.ORCA_SIDEBAR_INPUTS_IS_MAIN ? new Registry() : null.SIDEBAR_INPUT_METHODS, which is already gated.main.tsx.Why
The owner asked for the new sidebar work to land on main without being part of runtime behaviour. I copied the layout-owner switch's shape because it is already proven and already enforced at packaging time. I generalized the build helper rather than copying it, so that each call site (vite config, orcad entry build, packaging, orcad template, RPC catalog generator) keeps a single call that covers both switches. For the facts layer, a switch-chosen base class was the only option that keeps strict types, needs no casts, and leaves no override running when the switch is off. Guarding each of the seven overrides would still have left their bookkeeping in place.
Linked Issue
Follow-up to #29980, #30018, #30026 and #30027.
Visual Proof
N/A. With the switch off, nothing the user sees changes, and the phone's server list behaves as it did before #30027. With the switch on, behaviour matches main today. The checks below cover both.
Testing
Bundle comparison, switch off, before (37f23b9) vs after, built in the same lane path:
[sidebar-shadow] ...,sidebarInputs.*, the domain names), and none are added....[],pairedServerRows = null,OrcaRuntimeSidebarBase = OrcaRuntimeWithMigrationCatalog,sidebarFactsRuntimefolds toreturn null). ThesidebarInputsRPC module, the desktop-facts read and the server-rows producer class are gone. The restored on-demand class is added.sidebarInputs.8 → 0, producer internals 2–4 → 0. The activity and PR layer classes stay as uncalled class definitions, because the bundler keeps classes with a base class; they are never constructed.Bundle comparison, switch on: preload is byte-identical and the renderer differs only by the embedded build commit.
Hidden-app e2e (
ORCA_BACKGROUND_LAUNCH=1, headless, CDP only), on six specs: the three sidebar specs,sidebar-review-facts,paired-server-rows-shadowandpaired-mobile-desktop-mirror-workspace-visibility.ORCA_SIDEBAR_INPUTS=main): 6 passed, with 0 shadow mismatches.sidebarInputs.*were skipped. The shadow helper also asserts that no comparison is installed.Unit tests:
sidebarInputs.*method is registered.CI: no added CI cost. CI e2e builds have the switch off, so the shadow assertions check that nothing is installed and the two RPC specs skip.
Gates:
pnpm tc, oxlint, oxfmt,check:code-quality:changed,verify:rpc-params-catalog, the focus-read ratchet and the runtime-electron ratchet all pass. The runtime-rpc and cross-version-wire suites fail the same 19 tests with and without this change; those failures come from the local runtime, which has nonode:sqliteand has socket timeouts.I manually tested these changes locally
Automated tests added/updated, or explained why not below
Review
An independent review of the elegance and bugs found no bugs. I applied its design notes:
ServerWorktreeRowsis now a plain interface.A second review, of the changes after those fixes, also found no bugs. Following it,
runtime.sidebarFactsis now a getter rather than a field that holds the runtime itself. That change also fixed a problem I found myself: the first version value-imported the facts layer from the RPC methods module, which pulled the whole runtime into every importer of the method list. A cross-version wire test caught this through a leaked global.Agent skill upstream boundary
Notes
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)