Repository navigation
fix(workspace-cleanup): stop a sync broadcast reverting a filter you just set - #15369
Conversation
…just set `workspaceCleanupBrowse` re-hydrated on every hydration source. Two lines below it, `activeView` is startup-guarded for the same reason and says so. How it fired: the browse writer debounces 250ms. Any other `ui.set` inside that window makes main broadcast to all windows (`main/ipc/ui.ts:54-61`) carrying state that still holds the previous filters. The window hydrated that over the edit, and the pending debounce then wrote `get().workspaceCleanupBrowse` -- by then the reverted value -- so the revert was persisted, not just displayed. Your own write echoing back is harmless; it is somebody else's write landing first that does the damage. Intermittent, which is why it reads as "Orca forgot my filter". Guarded to `source === 'startup'`, matching activeView. The cost is that browse state no longer syncs between windows or from mobile. That costs nothing today: `mobile/src` has no workspace-cleanup consumer, and the popout runs a separate store and JS context, so nothing else in the product edits browse state. Filters become per-window, which is the call already made for activeView. Rejected the alternative (last-write-wins via a local dirty generation): it preserves a cross-window sync nobody uses, in exchange for more state to get wrong on a path that just proved it can silently discard user input. Three of the five tests fail against the unguarded source; the two that stay green assert what must not change -- startup still restores, and dismissals still hydrate on sync because they are main-owned.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughStartup hydration restores Merge Risk: ⚪ Minimal · up to This localized change prevents workspace-cleanup filters from being silently reverted by unrelated UI broadcasts while preserving startup restoration and dismissal synchronization; no actionable merge-blocking risk remains after normal checks and review. 🚥 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 |
Review statusReady to land. I kept the focused startup-only hydration guard and tightened the code/test rationale to the project comment style.
Owner decision: none required for the current product behavior. Revisit versioned synchronization only if another window or mobile surface later edits cleanup filters. |
…browse-hydration-guard
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
1 similar comment
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
…browse-hydration-guard
QA validation — Electron CDP run (
|
| # | Check | Result |
|---|---|---|
| 1 | Filter survives unrelated UI updates inside the 250 ms debounce | ✅ PASS |
| 2 | Filter survives full quit + relaunch | ✅ PASS |
| 3 | Filters restore correctly on normal startup | ✅ PASS |
| 4 | Dismissals (Ignored rows) still behave | ✅ PASS |
Check 1 — filter not reverted by a sync broadcast (PASS)
Set Ignored: Excluded via the real radio control, then fired an unrelated ui.set (sidebarWidth) before the browse writer's 250 ms debounce elapsed, with a listener recording every ui:stateChanged broadcast:
- radio click at t=98 ms → store
dismissed: "exclude", debounce armed - broadcast at t=112 ms carrying stale
dismissed: "any"(the exact bug trigger) → after hydration the store still read"exclude"— the guard refused the revert - broadcast at t=452 ms carried
"exclude"(the debounced write's echo); finalui.get()from main:"exclude"— the correct value was persisted, not the reverted one
Also survived: a typed query (qa-old), repeated sidebarWidth writes, and a theme settings change ("change a setting"). Note the dialog is modal, so sidebar toggles aren't clickable while it is open — the broadcast-race injection above exercises the same path those would.
Filter applied (chip + count):
Query + chip intact after the churn (Showing 2 of 4):
Checks 2 + 3 — quit, relaunch, startup restore (PASS)
Fully quit the instance (SIGTERM to the recorded process group), relaunched on the same profile. On startup the store hydrated query: "qa-old", dismissed: "exclude", and the qa-old-alpha dismissal; reopening the dialog through the status bar showed the restored query, the Ignored: Excluded chip, and Showing 1 of 4:
Check 4 — dismissals unbroken (PASS)
Clicked "Ignore qa-old-alpha": the dismissal landed in the store and persisted immediately (it rides along with the browse write), and with Ignored: Excluded active the row vanished (Showing 1 of 4):
Flipping the facet to Ignored: Only shows exactly the ignored row with its "Ignored" badge; the dismissal also survived the relaunch:
Extras — themes and 600 px minimum width
Light theme: contrast fine (chip, badges, disabled Delete button all legible). ✅
600 px width: no horizontal overflow; controls wrap cleanly. One pre-existing cosmetic nit (not from this PR — it touches no rendering): the dialog title "Delete Inactive Workspaces" breaks mid-word ("Workspac / es") at 600 px — a break-word-style wrap on the heading.
Not covered
- True multi-window / mobile-originated
ui.set(single-window instance; the injectedui.setbroadcast is the identical main-process path). - The known pre-existing dead mouse-wheel scroll inside the Filters popover (reproduces on main; unrelated to this change).
…just set (stablyai#15369) * fix(workspace-cleanup): stop a sync broadcast reverting a filter you just set `workspaceCleanupBrowse` re-hydrated on every hydration source. Two lines below it, `activeView` is startup-guarded for the same reason and says so. How it fired: the browse writer debounces 250ms. Any other `ui.set` inside that window makes main broadcast to all windows (`main/ipc/ui.ts:54-61`) carrying state that still holds the previous filters. The window hydrated that over the edit, and the pending debounce then wrote `get().workspaceCleanupBrowse` -- by then the reverted value -- so the revert was persisted, not just displayed. Your own write echoing back is harmless; it is somebody else's write landing first that does the damage. Intermittent, which is why it reads as "Orca forgot my filter". Guarded to `source === 'startup'`, matching activeView. The cost is that browse state no longer syncs between windows or from mobile. That costs nothing today: `mobile/src` has no workspace-cleanup consumer, and the popout runs a separate store and JS context, so nothing else in the product edits browse state. Filters become per-window, which is the call already made for activeView. Rejected the alternative (last-write-wins via a local dirty generation): it preserves a cross-window sync nobody uses, in exchange for more state to get wrong on a path that just proved it can silently discard user input. Three of the five tests fail against the unguarded source; the two that stay green assert what must not change -- startup still restores, and dismissals still hydrate on sync because they are main-owned. * chore(workspace-cleanup): tighten hydration guard rationale (cherry picked from commit d14923e)
…just set (stablyai#15369) * fix(workspace-cleanup): stop a sync broadcast reverting a filter you just set `workspaceCleanupBrowse` re-hydrated on every hydration source. Two lines below it, `activeView` is startup-guarded for the same reason and says so. How it fired: the browse writer debounces 250ms. Any other `ui.set` inside that window makes main broadcast to all windows (`main/ipc/ui.ts:54-61`) carrying state that still holds the previous filters. The window hydrated that over the edit, and the pending debounce then wrote `get().workspaceCleanupBrowse` -- by then the reverted value -- so the revert was persisted, not just displayed. Your own write echoing back is harmless; it is somebody else's write landing first that does the damage. Intermittent, which is why it reads as "Orca forgot my filter". Guarded to `source === 'startup'`, matching activeView. The cost is that browse state no longer syncs between windows or from mobile. That costs nothing today: `mobile/src` has no workspace-cleanup consumer, and the popout runs a separate store and JS context, so nothing else in the product edits browse state. Filters become per-window, which is the call already made for activeView. Rejected the alternative (last-write-wins via a local dirty generation): it preserves a cross-window sync nobody uses, in exchange for more state to get wrong on a path that just proved it can silently discard user input. Three of the five tests fail against the unguarded source; the two that stay green assert what must not change -- startup still restores, and dismissals still hydrate on sync because they are main-owned. * chore(workspace-cleanup): tighten hydration guard rationale
…just set (stablyai#15369) * fix(workspace-cleanup): stop a sync broadcast reverting a filter you just set `workspaceCleanupBrowse` re-hydrated on every hydration source. Two lines below it, `activeView` is startup-guarded for the same reason and says so. How it fired: the browse writer debounces 250ms. Any other `ui.set` inside that window makes main broadcast to all windows (`main/ipc/ui.ts:54-61`) carrying state that still holds the previous filters. The window hydrated that over the edit, and the pending debounce then wrote `get().workspaceCleanupBrowse` -- by then the reverted value -- so the revert was persisted, not just displayed. Your own write echoing back is harmless; it is somebody else's write landing first that does the damage. Intermittent, which is why it reads as "Orca forgot my filter". Guarded to `source === 'startup'`, matching activeView. The cost is that browse state no longer syncs between windows or from mobile. That costs nothing today: `mobile/src` has no workspace-cleanup consumer, and the popout runs a separate store and JS context, so nothing else in the product edits browse state. Filters become per-window, which is the call already made for activeView. Rejected the alternative (last-write-wins via a local dirty generation): it preserves a cross-window sync nobody uses, in exchange for more state to get wrong on a path that just proved it can silently discard user input. Three of the five tests fail against the unguarded source; the two that stay green assert what must not change -- startup still restores, and dismissals still hydrate on sync because they are main-owned. * chore(workspace-cleanup): tighten hydration guard rationale
ELI5
You change a filter in the workspace-cleanup dialog. A quarter-second later, something unrelated elsewhere in Orca saves its own UI state. That save broadcasts to every window — carrying the old filters, because yours hasn't been written yet. Your window takes that as truth and puts the old filter back. Then your pending save fires and writes the reverted value to disk. Your filter is gone, and it looks random.
What Changed
One line in
hydratePersistedUI:workspaceCleanupBrowsenow only hydrates whensource === 'startup', matchingactiveViewtwo lines below it.Why
store.onUIChangedbroadcasts to every window on every UI change (src/main/ipc/ui.ts:54-61). The browse writer debounces 250 ms (WORKSPACE_CLEANUP_BROWSE_PERSIST_DEBOUNCE_MS). Your own write echoing back is harmless — the value matches. The damage comes from someone else'sui.setlanding inside your window: main broadcasts state still holding the previous filters, the renderer hydrates it over your edit, and then the pending debounce writesget().workspaceCleanupBrowse— which is now the reverted value. So it is persisted, not merely displayed.Plenty of things route through
ui.set(sidebar state, modal dismissals, tour state, mobile). It needs a coincidence inside 250 ms, so it is intermittent rather than constant — which is the worst kind, because it reads as "Orca randomly forgot my filter" rather than a reproducible bug.activeViewalready carries the guard and the reason, in the same function:The trade, stated plainly. The guard means browse state no longer syncs between windows or from mobile. I checked whether that costs anything real, and it does not today:
mobile/srchas no workspace-cleanup consumer at all, and the dashboard popout runs a separate store and JS context. Nothing else in the product edits browse state. Filters become per-window, which is the same call already made foractiveView.The alternative I rejected: last-write-wins via a local dirty generation. It preserves cross-window sync — a sync nobody currently uses — in exchange for more state to get wrong, on a code path that has just demonstrated it can silently discard user input. Worth revisiting only if a second surface ever edits these filters.
Linked Issue
No tracking issue. Found while verifying, for @BrennanKB5, whether cleanup filters survive restarts — the answer is yes, except through this path. Same family as the originally reported "a filter I never set was applied".
Visual Proof
N/A— nothing renders differently. The change is that an edit stops being silently reverted, which does not photograph; a before/after would be two identical screenshots separated by a race. Covered by tests that fail against the unguarded source instead.Testing
New
ui-hydration-workspace-cleanup-browse.test.ts, 5 cases. I ran them against the unguarded source and confirmed exactly the right split:Fail before the fix (3) — these are the bug:
'sync') cannot revert eitherPass before and after (2) — these assert what must not change:
That second group is the point of the guard being narrow: it would be easy to "fix" this by skipping the whole
workspaceCleanupblock and quietly break dismissal sync.Gates on the rebased branch:
pnpm typecheck,npx oxlint,pnpm lint:react-doctor:changed, plus 330 tests across the UI-hydration and workspace-cleanup suites — all clean. No strings added.Review
The judgement worth checking is the per-window trade. If you think cleanup filters should follow you between windows, this is the wrong fix and the dirty-generation approach is the right one — say so and I will swap it. My read is that per-window matches
activeViewand matches what actually happens today, since no second surface writes these filters.Agent skill upstream boundary
Notes
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm testpass locallyAuthor