mirror of
https://github.com/stablyai/orca.git
synced 2026-10-06 00:02:43 +00:00
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.
This commit is contained in:
+108
@@ -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<AppState>()(
|
||||
(...a) =>
|
||||
({
|
||||
workspaceCleanupDismissals: {},
|
||||
...createWorkspaceCleanupBrowseSlice(...a)
|
||||
}) as unknown as AppState
|
||||
)
|
||||
|
||||
vi.mock('@/store', () => ({
|
||||
useAppStore: <T,>(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(<Probe />))
|
||||
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<AppState>)
|
||||
})
|
||||
|
||||
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'
|
||||
})
|
||||
})
|
||||
})
|
||||
+23
-18
@@ -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<WorkspaceCleanupBrowseController['patchFilters']>(
|
||||
(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,
|
||||
|
||||
+24
-1
@@ -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()
|
||||
|
||||
@@ -139,10 +139,13 @@ export function FacetNumberField({
|
||||
return (
|
||||
<FacetField label={label}>
|
||||
<div className="flex items-center gap-1.5">
|
||||
{/* 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. */}
|
||||
<Input
|
||||
type="number"
|
||||
type="text"
|
||||
inputMode="numeric"
|
||||
min={0}
|
||||
aria-label={label}
|
||||
value={value === null ? '' : String(value)}
|
||||
placeholder={placeholder}
|
||||
|
||||
@@ -11,9 +11,14 @@ export const WORKSPACE_CLEANUP_BROWSE_PERSIST_DEBOUNCE_MS = 250
|
||||
|
||||
let persistTimer: ReturnType<typeof setTimeout> | 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)
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user