From 4bc9e6b00c3b703cf74ee7a8dd17ea72eba29042 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Sun, 23 Aug 2026 18:39:48 -0700 Subject: [PATCH] fix(workspace-cleanup): stop a wheel tick setting a filter, and land every filter patch (#15298) A profile carried `activity.idleMinDays = 20` that the user never set, hiding 253 of 799 workspaces on open. Chromium mutates a *focused* number input on every wheel tick, and before #14629 the facet panel could not scroll, so the natural response -- cursor into the panel, spin the wheel -- walked the threshold up and persisted it. Two fixes: - `FacetNumberField` renders `type="text" inputMode="numeric"`. A wheel cannot mutate a text input, and the parser already takes strings. `preventDefault` on a focused number input would also work, but it blocks the wheel's default action -- which includes scrolling the nearest scrollable ancestor -- and would re-break the panel scrolling #14629 just fixed, in exactly the reported gesture. All five numeric facets share this one field. - `patchFilters` closed over the render's `browse` snapshot, so two patches in one tick dropped one. It now writes through a functional update against the latest store state. `toggleSortField` and `clearFilters` had the same defect. Both tests were confirmed to fail against the unfixed source before being kept. --- ...se-workspace-cleanup-browse-state.test.tsx | 108 ++++++++++++++++++ .../use-workspace-cleanup-browse-state.ts | 41 ++++--- .../workspace-cleanup-facet-controls.test.tsx | 25 +++- .../workspace-cleanup-facet-controls.tsx | 7 +- .../store/slices/workspace-cleanup-browse.ts | 12 +- 5 files changed, 170 insertions(+), 23 deletions(-) create mode 100644 src/renderer/src/components/workspace-cleanup/use-workspace-cleanup-browse-state.test.tsx diff --git a/src/renderer/src/components/workspace-cleanup/use-workspace-cleanup-browse-state.test.tsx b/src/renderer/src/components/workspace-cleanup/use-workspace-cleanup-browse-state.test.tsx new file mode 100644 index 00000000000..8e946009725 --- /dev/null +++ b/src/renderer/src/components/workspace-cleanup/use-workspace-cleanup-browse-state.test.tsx @@ -0,0 +1,108 @@ +// @vitest-environment happy-dom +import { act } from 'react' +import { createRoot, type Root } from 'react-dom/client' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { create } from 'zustand' +import type { AppState } from '@/store/types' +import { + createWorkspaceCleanupBrowseSlice, + resetWorkspaceCleanupBrowsePersistTimer +} from '@/store/slices/workspace-cleanup-browse' +import { createDefaultWorkspaceCleanupBrowseState } from '../../../../shared/workspace-cleanup-browse-state' +import { useWorkspaceCleanupBrowseState } from './use-workspace-cleanup-browse-state' +import type { WorkspaceCleanupBrowseController } from './use-workspace-cleanup-browse-state' + +const store = create()( + (...a) => + ({ + workspaceCleanupDismissals: {}, + ...createWorkspaceCleanupBrowseSlice(...a) + }) as unknown as AppState +) + +vi.mock('@/store', () => ({ + useAppStore: (selector: (state: AppState) => T): T => store(selector) +})) + +let root: Root | null = null +let container: HTMLDivElement | null = null + +function mountController(): { current: WorkspaceCleanupBrowseController | null } { + const ref: { current: WorkspaceCleanupBrowseController | null } = { current: null } + function Probe(): null { + ref.current = useWorkspaceCleanupBrowseState() + return null + } + container = document.createElement('div') + document.body.appendChild(container) + root = createRoot(container) + act(() => root!.render()) + return ref +} + +describe('useWorkspaceCleanupBrowseState', () => { + beforeEach(() => { + vi.useFakeTimers() + ;(globalThis as { window: unknown }).window = { + ...globalThis.window, + api: { ui: { set: vi.fn().mockResolvedValue(undefined) } } + } + store.setState({ + workspaceCleanupBrowse: createDefaultWorkspaceCleanupBrowseState() + } as Partial) + }) + + afterEach(() => { + if (root) { + act(() => root!.unmount()) + } + root = null + container = null + document.body.replaceChildren() + resetWorkspaceCleanupBrowsePersistTimer() + vi.useRealTimers() + }) + + it('keeps both patches when two groups are written in one tick', () => { + const controller = mountController() + + // Why one act(): a checkbox toggle writes several groups at once. Reading the + // rendered `browse` snapshot per call would make the second patch overwrite + // the first, which is what this asserts against. + act(() => { + controller.current!.patchFilters('activity', { idleMinDays: 20 }) + controller.current!.patchFilters('size', { maxBytes: 500 }) + }) + + const filters = store.getState().workspaceCleanupBrowse.filters + expect(filters.activity.idleMinDays).toBe(20) + expect(filters.size.maxBytes).toBe(500) + }) + + it('keeps both patches when the same group is written twice in one tick', () => { + const controller = mountController() + + act(() => { + controller.current!.patchFilters('git', { minAhead: 2 }) + controller.current!.patchFilters('git', { branchQuery: 'release' }) + }) + + const git = store.getState().workspaceCleanupBrowse.filters.git + expect(git.minAhead).toBe(2) + expect(git.branchQuery).toBe('release') + }) + + it('flips sort direction against the latest state, not the rendered snapshot', () => { + const controller = mountController() + + act(() => { + controller.current!.toggleSortField('size') + controller.current!.toggleSortField('size') + }) + + expect(store.getState().workspaceCleanupBrowse.sort).toEqual({ + field: 'size', + direction: 'desc' + }) + }) +}) diff --git a/src/renderer/src/components/workspace-cleanup/use-workspace-cleanup-browse-state.ts b/src/renderer/src/components/workspace-cleanup/use-workspace-cleanup-browse-state.ts index 948928b9a93..76f4164b694 100644 --- a/src/renderer/src/components/workspace-cleanup/use-workspace-cleanup-browse-state.ts +++ b/src/renderer/src/components/workspace-cleanup/use-workspace-cleanup-browse-state.ts @@ -26,37 +26,42 @@ export function useWorkspaceCleanupBrowseState(): WorkspaceCleanupBrowseControll const browse = useAppStore((s) => s.workspaceCleanupBrowse) const updateBrowse = useAppStore((s) => s.updateWorkspaceCleanupBrowseState) + // Why the updater form: a checkbox toggle writes several fields at once, so two + // patches can land in one tick. Reading `browse` from the render would make the + // second overwrite the first. const patchFilters = useCallback( (key, value) => { - const current = browse.filters[key] - const next = - typeof current === 'object' && current !== null - ? { ...current, ...(value as object) } - : value - // Cast: a computed key over a union widens the spread result past - // WorkspaceCleanupFilterState even though `key` is constrained to it. - const filters = { ...browse.filters, [key]: next } as WorkspaceCleanupFilterState - updateBrowse({ ...browse, filters }) + updateBrowse((current) => { + const group = current.filters[key] + const next = + typeof group === 'object' && group !== null ? { ...group, ...(value as object) } : value + // Cast: a computed key over a union widens the spread result past + // WorkspaceCleanupFilterState even though `key` is constrained to it. + const filters = { ...current.filters, [key]: next } as WorkspaceCleanupFilterState + return { ...current, filters } + }) }, - [browse, updateBrowse] + [updateBrowse] ) // Why: re-picking the active sort flips its direction. const toggleSortField = useCallback( (field: WorkspaceCleanupSortField) => { - const direction = - browse.sort.field === field && browse.sort.direction === 'asc' ? 'desc' : 'asc' - updateBrowse({ ...browse, sort: { field, direction } }) + updateBrowse((current) => { + const direction = + current.sort.field === field && current.sort.direction === 'asc' ? 'desc' : 'asc' + return { ...current, sort: { field, direction } } + }) }, - [browse, updateBrowse] + [updateBrowse] ) const clearFilters = useCallback(() => { - updateBrowse({ - ...browse, + updateBrowse((current) => ({ + ...current, filters: createDefaultWorkspaceCleanupFilterState() - }) - }, [browse, updateBrowse]) + })) + }, [updateBrowse]) return { filters: browse.filters, diff --git a/src/renderer/src/components/workspace-cleanup/workspace-cleanup-facet-controls.test.tsx b/src/renderer/src/components/workspace-cleanup/workspace-cleanup-facet-controls.test.tsx index 10049ceeb4e..5c36b15a054 100644 --- a/src/renderer/src/components/workspace-cleanup/workspace-cleanup-facet-controls.test.tsx +++ b/src/renderer/src/components/workspace-cleanup/workspace-cleanup-facet-controls.test.tsx @@ -108,11 +108,34 @@ describe('workspace cleanup facet controls', () => { renderFacets(createDefaultWorkspaceCleanupFilterState(), onPatch) const input = control('Idle for at least') as HTMLInputElement | null - expect(input?.type).toBe('number') act(() => typeInto(input, '17')) expect(onPatch).toHaveBeenCalledWith('activity', { idleMinDays: 17 }) }) + it('keeps every numeric threshold off type=number so a wheel tick cannot set it', () => { + // Why assert the type rather than dispatch a wheel event: the spinner is a + // Chromium behaviour happy-dom does not implement, so a dispatched wheel + // changes nothing here either way and the test would pass without the fix. + // The type is the property that makes the whole class impossible. + renderFacets(createDefaultWorkspaceCleanupFilterState(), createPatchMock()) + + const numericFacets = [ + 'Idle for at least', + 'At least', + 'At most', + 'Commits ahead ≥', + 'Commits behind ≥' + ] + const found = numericFacets + .map((label) => control(label) as HTMLInputElement | null) + .filter((input): input is HTMLInputElement => input !== null) + expect(found).toHaveLength(numericFacets.length) + for (const input of found) { + expect(input.type).toBe('text') + expect(input.inputMode).toBe('numeric') + } + }) + it('multi-selects git states', () => { const onPatch = createPatchMock() const filters = createDefaultWorkspaceCleanupFilterState() diff --git a/src/renderer/src/components/workspace-cleanup/workspace-cleanup-facet-controls.tsx b/src/renderer/src/components/workspace-cleanup/workspace-cleanup-facet-controls.tsx index 7dc1c0d92e7..481e4ecd6b8 100644 --- a/src/renderer/src/components/workspace-cleanup/workspace-cleanup-facet-controls.tsx +++ b/src/renderer/src/components/workspace-cleanup/workspace-cleanup-facet-controls.tsx @@ -139,10 +139,13 @@ export function FacetNumberField({ return (
+ {/* Why text, not number: Chromium mutates a focused number input on every + wheel tick, which silently walked a threshold up inside the scrollable + facet panel. preventDefault would stop that but also stop the panel + scrolling, re-breaking #14629. */} | null = null +/** Updater form exists so two patches in one tick cannot read the same stale snapshot. */ +export type WorkspaceCleanupBrowseUpdate = + | WorkspaceCleanupBrowseState + | ((current: WorkspaceCleanupBrowseState) => WorkspaceCleanupBrowseState) + export type WorkspaceCleanupBrowseSlice = { workspaceCleanupBrowse: WorkspaceCleanupBrowseState - updateWorkspaceCleanupBrowseState: (next: WorkspaceCleanupBrowseState) => void + updateWorkspaceCleanupBrowseState: (next: WorkspaceCleanupBrowseUpdate) => void } export const createWorkspaceCleanupBrowseSlice: StateCreator< @@ -25,7 +30,10 @@ export const createWorkspaceCleanupBrowseSlice: StateCreator< workspaceCleanupBrowse: createDefaultWorkspaceCleanupBrowseState(), updateWorkspaceCleanupBrowseState: (next) => { - set({ workspaceCleanupBrowse: next }) + set((state) => ({ + workspaceCleanupBrowse: + typeof next === 'function' ? next(state.workspaceCleanupBrowse) : next + })) if (persistTimer !== null) { clearTimeout(persistTimer) }