From 006374d98b49997ef35f1bf345da2974f225ab56 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Tue, 18 Aug 2026 14:50:42 -0700 Subject: [PATCH] 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. --- ...hydration-workspace-cleanup-browse.test.ts | 79 +++++++++++++++++++ src/renderer/src/store/slices/ui.ts | 10 ++- 2 files changed, 88 insertions(+), 1 deletion(-) create mode 100644 src/renderer/src/store/slices/ui-hydration-workspace-cleanup-browse.test.ts diff --git a/src/renderer/src/store/slices/ui-hydration-workspace-cleanup-browse.test.ts b/src/renderer/src/store/slices/ui-hydration-workspace-cleanup-browse.test.ts new file mode 100644 index 00000000000..c91e4e0d803 --- /dev/null +++ b/src/renderer/src/store/slices/ui-hydration-workspace-cleanup-browse.test.ts @@ -0,0 +1,79 @@ +import { describe, expect, it } from 'vitest' +import { createDefaultWorkspaceCleanupBrowseState } from '../../../../shared/workspace-cleanup-browse-state' +import { createUIStore, makePersistedUI } from './ui-slice-test-harness' + +function persistedWithIdleDays(idleMinDays: number | null) { + const browse = createDefaultWorkspaceCleanupBrowseState() + browse.filters.activity.idleMinDays = idleMinDays + return makePersistedUI({ workspaceCleanup: { dismissals: {}, browse } }) +} + +describe('workspace cleanup browse hydration', () => { + it('restores persisted filters on startup', () => { + const store = createUIStore() + + store.getState().hydratePersistedUI(persistedWithIdleDays(30), 'startup') + + expect(store.getState().workspaceCleanupBrowse.filters.activity.idleMinDays).toBe(30) + }) + + it('does not let a sync broadcast revert a filter the user just set', () => { + // The browse writer debounces 250ms, so any other ui.set inside that window + // broadcasts state that still holds the previous filters. Re-hydrating it here + // reverted the edit, and the pending debounce then persisted the revert. + const store = createUIStore() + store.getState().hydratePersistedUI(persistedWithIdleDays(30), 'startup') + + const edited = createDefaultWorkspaceCleanupBrowseState() + edited.filters.activity.idleMinDays = 45 + store.setState({ workspaceCleanupBrowse: edited }) + + store.getState().hydratePersistedUI(persistedWithIdleDays(30), 'sync') + + expect(store.getState().workspaceCleanupBrowse.filters.activity.idleMinDays).toBe(45) + }) + + it('does not let a sync broadcast revert a typed query', () => { + const store = createUIStore() + const edited = createDefaultWorkspaceCleanupBrowseState() + edited.filters.query = 'release' + store.setState({ workspaceCleanupBrowse: edited }) + + store.getState().hydratePersistedUI(persistedWithIdleDays(null), 'sync') + + expect(store.getState().workspaceCleanupBrowse.filters.query).toBe('release') + }) + + it('defaults an absent source to sync, so an unlabelled broadcast cannot revert either', () => { + const store = createUIStore() + const edited = createDefaultWorkspaceCleanupBrowseState() + edited.filters.activity.idleMinDays = 45 + store.setState({ workspaceCleanupBrowse: edited }) + + store.getState().hydratePersistedUI(persistedWithIdleDays(30)) + + expect(store.getState().workspaceCleanupBrowse.filters.activity.idleMinDays).toBe(45) + }) + + it('still hydrates dismissals on sync, which are main-owned and not edited here', () => { + const store = createUIStore() + const dismissal = { + worktreeId: 'repo-1::/repo/one', + dismissedAt: Date.now(), + fingerprint: 'fp-1', + classifierVersion: 2 + } + + store.getState().hydratePersistedUI( + makePersistedUI({ + workspaceCleanup: { + dismissals: { [dismissal.worktreeId]: dismissal }, + browse: createDefaultWorkspaceCleanupBrowseState() + } + }), + 'sync' + ) + + expect(store.getState().workspaceCleanupDismissals[dismissal.worktreeId]).toBeDefined() + }) +}) diff --git a/src/renderer/src/store/slices/ui.ts b/src/renderer/src/store/slices/ui.ts index e89e990719d..7165119e6f0 100644 --- a/src/renderer/src/store/slices/ui.ts +++ b/src/renderer/src/store/slices/ui.ts @@ -2625,7 +2625,15 @@ export const createUISlice: StateCreator = (set, get) // Why the normalizer rather than a cast: this blob is hand-editable and // may come from an older or newer build; it degrades field by field // instead of bricking the cleanup dialog. - workspaceCleanupBrowse: normalizeWorkspaceCleanupBrowseState(ui.workspaceCleanup?.browse), + // Why startup-only, like activeView below: the browse writer debounces 250ms, so + // any other ui.set inside that window broadcasts state still holding the previous + // filters. Re-hydrating it here reverted the edit, and the pending debounce then + // persisted the revert. Nothing else edits browse state (mobile has no cleanup + // consumer, the popout is a separate store), so filters are per-window. + workspaceCleanupBrowse: + source === 'startup' + ? normalizeWorkspaceCleanupBrowseState(ui.workspaceCleanup?.browse) + : s.workspaceCleanupBrowse, // Why: restore only on startup; on 'sync' broadcasts it would clobber the window's current per-window view. activeView: source === 'startup'