Repository navigation
feat(sidebar): ui.setCollapsed changes one section's collapse without undoing another device - #30044
Conversation
…sh writer per counter
📝 Walkthrough
Priority: ⬇️ Low Merge Risk: 🔵 Low · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 10 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly identifies the primary change: adding per-section collapse updates that prevent one device from undoing another device's change. Description check The description is complete and focused. It explains the user impact, implementation, ordering behavior, feature gating, compatibility limits, testing, review findings, and checklist status. It does n… 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 explains the user impact, implementation, ordering behavior, feature gating, compatibility limits, testing, review findings, and checklist status. It does not provide a direct issue reference in the Linked Issue section, but the remaining required sections are 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:
a7f6dd45-4b54-4f5f-97d8-808db5be571c
⛔ Files ignored due to path filters: 1
src/shared/rpc-contract/rpc-params-catalog.generated.tsis excluded by!**/*.generated.*
📒 Files selected for processing: 6
src/main/runtime/orca-runtime.tssrc/main/runtime/rpc/methods/client-ui-set-collapsed.test.tssrc/main/runtime/rpc/methods/client-ui.tssrc/main/runtime/runtime-rpc/runtime-rpc-mobile-paired-desktop-methods.tssrc/main/runtime/workspace-view-collapse-writes.tssrc/shared/rpc-contract/workspace-view-collapse-params.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.
| const runtime = fake as unknown as OrcaRuntimeService | ||
| const dispatcher = new RpcDispatcher({ runtime, methods: CLIENT_UI_METHODS }) | ||
| let requestId = 0 | ||
| const call = async (method: string, params: unknown, context: object = ownPhone) => { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Give both test contexts a named dispatch-options type.
Both context: object parameters fail the reported anti-slop lint check. The static-analysis job cannot pass with these declarations. Use a named type derived from RpcDispatcher.dispatch for both parameters.
Proposed change
+type TestContext = NonNullable<Parameters<RpcDispatcher['dispatch']>[1]>
+
- const call = async (method: string, params: unknown, context: object = ownPhone) => {
+ const call = async (method: string, params: unknown, context: TestContext = ownPhone) => {
...
- context: object = ownPhone
+ context: TestContext = ownPhoneAlso applies to: 37-37
🧰 Tools
🪛 GitHub Actions: PR 30044 | source 092111eb53c479e1c81f67d188acc427dde85e37 | workflow 092111eb53c479e1c81f67d188acc427dde85e37 | unit full / 14_static analysis and typecheck.txt
[error] 28-28: Lint check failed: Parameter context uses the broad object type. Accept a named owner type; parse external input at its boundary before calling this function.
🪛 GitHub Actions: PR 30044 | source 092111eb53c479e1c81f67d188acc427dde85e37 | workflow 092111eb53c479e1c81f67d188acc427dde85e37 | unit full / static analysis and typecheck
[error] 28-28: Oxlint anti-slop check failed: Parameter context uses the broad object type. Accept a named owner type and parse external input at its boundary. Command: bun --smol node_modules/oxlint/bin/oxlint --config config/oxlint-anti-slop.json src config tests mobile --deny-warnings.
Source: Pipeline failures
…se-per-key # Conflicts: # src/main/runtime/orca-runtime.ts
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:
379270b7-739e-4b40-9b19-b0320648c7d5
⛔ Files ignored due to path filters: 1
src/shared/rpc-contract/rpc-params-catalog.generated.tsis excluded by!**/*.generated.*
📒 Files selected for processing: 7
src/main/runtime/orca-runtime.tssrc/main/runtime/rpc/methods/client-ui-set-collapsed.test.tssrc/main/runtime/rpc/methods/sidebar-inputs.tssrc/main/runtime/runtime-rpc/runtime-rpc-mobile-method-routing.test.tssrc/main/runtime/runtime-rpc/runtime-rpc-mobile-paired-desktop-methods.tssrc/main/runtime/sidebar-inputs/sidebar-inputs-switch-off.test.tstests/e2e/sidebar-collapse-per-key.spec.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review.
| }) | ||
|
|
||
| it('routes no per-section collapse write while the sidebar-inputs switch is off', () => { | ||
| expect(MOBILE_RPC_METHOD_ROUTES.has('ui.setCollapsed')).toBe(false) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
rg -n 'PAIRED_DESKTOP_METHODS|MOBILE_RPC_METHOD_ROUTES' src/main/runtime/runtime-rpc
sed -n '1,45p' src/main/runtime/runtime-rpc/runtime-rpc-mobile-method-routing.test.ts
sed -n '100,120p' src/main/runtime/runtime-rpc/runtime-rpc-mobile-paired-desktop-methods.tsRepository: stablyai/orca
Length of output: 4000
🏁 Script executed:
set -eu
printf '%s\n' '--- test ---'
nl -ba src/main/runtime/runtime-rpc/runtime-rpc-mobile-method-routing.test.ts | sed -n '1,40p'
printf '%s\n' '--- paired methods ---'
nl -ba src/main/runtime/runtime-rpc/runtime-rpc-mobile-paired-desktop-methods.ts | sed -n '1,16p;100,125p'
printf '%s\n' '--- switch bindings ---'
rg -n -F -- 'ORCA_SIDEBAR_INPUTS_IS_MAIN' src/main/runtime/runtime-rpc src/main | head -40
printf '%s\n' '--- routing imports and map ---'
nl -ba src/main/runtime/runtime-rpc/runtime-rpc-mobile-method-routing.ts | sed -n '1,12p;208,220p'Repository: stablyai/orca
Length of output: 6066
Make the routing assertion conditional on the build switch.
When ORCA_SIDEBAR_INPUTS_IS_MAIN is true, the route exists. Update the test to compare route presence with the switch value.
Suggested fix
import { PAIRED_DESKTOP_METHODS } from './runtime-rpc-mobile-paired-desktop-methods'
+import { ORCA_SIDEBAR_INPUTS_IS_MAIN } from '../../../shared/sidebar-inputs-switch'
describe('mobile RPC method routing census', () => {
@@
- it('routes no per-section collapse write while the sidebar-inputs switch is off', () => {
- expect(MOBILE_RPC_METHOD_ROUTES.has('ui.setCollapsed')).toBe(false)
+ it('routes the per-section collapse write according to the sidebar-inputs switch', () => {
+ expect(MOBILE_RPC_METHOD_ROUTES.has('ui.setCollapsed')).toBe(
+ ORCA_SIDEBAR_INPUTS_IS_MAIN
+ )
})…se-per-key # Conflicts: # src/main/runtime/rpc/methods/sidebar-inputs.ts # src/main/runtime/sidebar-inputs/sidebar-inputs-switch-off.test.ts
ELI5
The sidebar remembers which sections you folded shut. Today, when the desktop and its phone each fold a section, each one sends its whole list of folded sections, so the second one can quietly undo the first. This adds a way to say "fold just this one section" so two devices stop overwriting each other. Nothing uses it yet.
Dark: the method exists only in a build with
ORCA_SIDEBAR_INPUTS=main(ORCA_SIDEBAR_INPUTS_IS_MAIN, #30050). Shipped builds have the switch off and keep exactly main's behaviour and method set.What Changed
ui.setCollapsed({ key, collapsed, opId: { writer, seq } }). It folds or unfolds one section in the savedcollapsedGroupsand leaves every other section alone. It saves through the same UI-state write asui.set, so the desktop window and phones get the same change events as they do today.seqeach writer has applied to each section, in a bounded in-memory map. When a writer resends an op that already landed, or an older op arrives after its newer one, main does nothing and returnsapplied: falsewith the section's current value. In both cases the op is finished, so the client can drop its pending entry even if the value differs. Example: phone A folds a section, phone B unfolds it, then A reconnects and resends its fold. B's unfold stays.ui.set. Regenerated the RPC catalog.src/shared/sidebar-inputs-switch.ts:SIDEBAR_INPUT_METHODS, which is registered only when the switch is on.collapseWriteOrder, which is null when the switch is off.sidebarInputs.*entries.Before and after for users: none. The window and the phone still use the whole-list
ui.set. Switching them over is a later PR.Why
The owner decided that for each section, the last change wins. Resending the desired value is not enough to make retries safe: a late retry would still undo another device's newer change. Each writer numbers its own changes, so main can tell a stale or repeated op from a new one without comparing clocks across devices.
Alternatives considered:
Who may call it: the same callers as
ui.set.collapsedGroupsis not one of the fields each side of a pairing keeps for itself, so a different scope here would make the two writes disagree. Separating views per device is owned by a later PR.Mixed versions:
forbidden, because the mobile allowlist check runs before method lookup. Other old callers get method-not-found. Future callers must treat both answers as "fall back toui.set".Known limits:
writerid whenever its counter restarts.Linked Issue
Part of the sidebar list redesign (design PR 10, per-section collapse).
Visual Proof
N/A. No caller uses the new method, so nothing visible changes.
Testing
src/main/runtime/rpc/methods/client-ui-set-collapsed.test.tsruns the real dispatcher and method. It covers desktop and phone folding different sections, both changing the same section, a retry after another writer's newer change, an older op arriving after a newer one from the same writer, and mixing with an old client's whole-listui.set.pnpm tc, client-ui, mobile allowlist, phone-method routing and caller-scope tests, RPC catalog check, changed-code quality gate, focus-read ratchet.ui.setCollapsedexists.ui.setCollapsedstring, handler or route, and the field folds tocollapseWriteOrder=null. Two unused definitions remain in a shared chunk: the ordering class and the params schema expression, because the bundler keeps them. Nothing calls either one. All other changes are minifier renames.tests/e2e/sidebar-collapse-per-key.spec.ts, run withORCA_BACKGROUND_LAUNCH=1, headless, CDP only:applied: falseand leaves the newer change in place.sidebar-review-factsalso passed in the same run.method_not_found.Review
An Opus review of the diff found no ordering or eviction bugs. Applied from it: the old-host
forbiddenanswer is now documented, the rule for minting a new writer id is spelled out, the caller scope now matchesui.setinstead of being narrowed, and the eviction code is simpler.A second Opus review covered the gating. It found nothing reachable with the switch off. Applied from it:
SIDEBAR_INPUT_METHODS.describelevel, before the app launches.method_not_found.Agent skill upstream boundary
Notes
Covers SSH, folder workspaces and paired servers: collapse keys are opaque strings, and the phone's call is answered by its paired Mac, the same as
ui.set. The change is the same on every platform.Checklist
N/Awith reason for visualspnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (typecheck and targeted tests local; CI covers the rest)