diff --git a/src/renderer/src/components/settings/use-debounced-settings-text-draft.test.ts b/src/renderer/src/components/settings/use-debounced-settings-text-draft.test.ts index 6e13cc4695c..da12fd96a26 100644 --- a/src/renderer/src/components/settings/use-debounced-settings-text-draft.test.ts +++ b/src/renderer/src/components/settings/use-debounced-settings-text-draft.test.ts @@ -1,5 +1,6 @@ // @vitest-environment happy-dom +import { StrictMode } from 'react' import { act, cleanup, renderHook } from '@testing-library/react' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { useDebouncedSettingsTextDraft } from './use-debounced-settings-text-draft' @@ -97,3 +98,125 @@ describe('useDebouncedSettingsTextDraft', () => { expect(commit).not.toHaveBeenCalled() }) }) + +describe('useDebouncedSettingsTextDraft flush paths', () => { + it('commits a pending edit on beforeunload, since a window close never unmounts the tree', () => { + const commit = vi.fn() + const { result, unmount } = renderHook(() => + useDebouncedSettingsTextDraft({ value: '', commit }) + ) + + act(() => result.current.onChange('quit-mid-word')) + act(() => { + window.dispatchEvent(new Event('beforeunload', { cancelable: true })) + }) + + expect(commit).toHaveBeenCalledExactlyOnceWith('quit-mid-word') + + // The later unmount and timer must not commit the same value again. + unmount() + act(() => { + vi.advanceTimersByTime(700) + }) + expect(commit).toHaveBeenCalledTimes(1) + }) + + it('does not commit on beforeunload when nothing is pending', () => { + const commit = vi.fn() + renderHook(() => useDebouncedSettingsTextDraft({ value: 'x', commit })) + + act(() => { + window.dispatchEvent(new Event('beforeunload', { cancelable: true })) + }) + + expect(commit).not.toHaveBeenCalled() + }) + + it('stops listening for beforeunload after unmount', () => { + const commit = vi.fn() + const { result, unmount } = renderHook(() => + useDebouncedSettingsTextDraft({ value: '', commit }) + ) + + act(() => result.current.onChange('abc')) + unmount() + act(() => { + window.dispatchEvent(new Event('beforeunload', { cancelable: true })) + }) + + expect(commit).toHaveBeenCalledExactlyOnceWith('abc') + }) + + it('commits every edit burst, not only the first', () => { + const commit = vi.fn() + const { result } = renderHook(() => useDebouncedSettingsTextDraft({ value: '', commit })) + + act(() => result.current.onChange('one')) + act(() => { + vi.advanceTimersByTime(700) + }) + act(() => result.current.onChange('one two')) + act(() => { + vi.advanceTimersByTime(700) + }) + + expect(commit).toHaveBeenNthCalledWith(1, 'one') + expect(commit).toHaveBeenNthCalledWith(2, 'one two') + }) + + it('adopts external values again once a pending edit has been committed', () => { + const commit = vi.fn() + const { result, rerender } = renderHook( + ({ value }) => useDebouncedSettingsTextDraft({ value, commit }), + { initialProps: { value: '' } } + ) + + act(() => result.current.onChange('typed')) + act(() => { + vi.advanceTimersByTime(700) + }) + expect(commit).toHaveBeenCalledExactlyOnceWith('typed') + + // The store echoes the commit, then another window writes a different value. + rerender({ value: 'typed' }) + rerender({ value: 'from-another-window' }) + + expect(result.current.value).toBe('from-another-window') + }) + + it('keeps a keystroke typed while the previous commit is still in flight', () => { + const commit = vi.fn() + const { result, rerender } = renderHook( + ({ value }) => useDebouncedSettingsTextDraft({ value, commit }), + { initialProps: { value: '' } } + ) + + act(() => result.current.onChange('abc')) + act(() => { + vi.advanceTimersByTime(700) + }) + act(() => result.current.onChange('abcd')) + // The store echoes the first commit after the user has already typed more. + rerender({ value: 'abc' }) + + expect(result.current.value).toBe('abcd') + + act(() => result.current.onBlur()) + expect(commit).toHaveBeenLastCalledWith('abcd') + expect(commit).toHaveBeenCalledTimes(2) + }) + + it('does not spuriously commit under StrictMode effect replay', () => { + const commit = vi.fn() + const { result } = renderHook(() => useDebouncedSettingsTextDraft({ value: 'x', commit }), { + wrapper: StrictMode + }) + + expect(commit).not.toHaveBeenCalled() + + act(() => result.current.onChange('xy')) + act(() => result.current.onBlur()) + + expect(commit).toHaveBeenCalledExactlyOnceWith('xy') + }) +}) diff --git a/src/renderer/src/components/settings/use-debounced-settings-text-draft.ts b/src/renderer/src/components/settings/use-debounced-settings-text-draft.ts index 8c0926e9b34..1f9fe235ead 100644 --- a/src/renderer/src/components/settings/use-debounced-settings-text-draft.ts +++ b/src/renderer/src/components/settings/use-debounced-settings-text-draft.ts @@ -10,11 +10,16 @@ export type DebouncedSettingsTextDraft = { } /** - * Local draft for a free-text setting, committed on a debounce and flushed on blur and unmount. + * Local draft for a free-text setting, committed on a debounce and flushed on blur, unmount, and + * window unload. * * Why: binding an `` straight to `updateSettings` sends one IPC round trip per keystroke, * and each one replaces the `settings` object identity in every other window, re-rendering every * component subscribed to it. The committed value is unchanged — only the number of commits is. + * + * A pending timer is the single source of truth for "the draft has uncommitted edits": `onChange` + * is the only place that arms it and `flush` the only place that clears it, so there is no separate + * dirty flag to fall out of sync. */ export function useDebouncedSettingsTextDraft(args: { value: string @@ -23,7 +28,6 @@ export function useDebouncedSettingsTextDraft(args: { const { value, commit } = args const [draft, setDraft] = useState(value) const draftRef = useRef(draft) - const dirtyRef = useRef(false) const commitRef = useRef(commit) const timerRef = useRef | null>(null) @@ -32,10 +36,10 @@ export function useDebouncedSettingsTextDraft(args: { commitRef.current = commit }, [commit]) - // Why gated on dirty: an external write (another window, a reset) should land in the field, but - // must not yank characters out from under someone mid-edit. + // Why gated on a pending commit: an external write (another window, a reset) should land in the + // field, but must not yank characters out from under someone mid-edit. useEffect(() => { - if (dirtyRef.current) { + if (timerRef.current !== null) { return } draftRef.current = value @@ -43,21 +47,17 @@ export function useDebouncedSettingsTextDraft(args: { }, [value]) const flush = useCallback(() => { - if (timerRef.current !== null) { - clearTimeout(timerRef.current) - timerRef.current = null - } - if (!dirtyRef.current) { + if (timerRef.current === null) { return } - dirtyRef.current = false + clearTimeout(timerRef.current) + timerRef.current = null commitRef.current(draftRef.current) }, []) const onChange = useCallback( (next: string) => { draftRef.current = next - dirtyRef.current = true setDraft(next) if (timerRef.current !== null) { clearTimeout(timerRef.current) @@ -67,10 +67,17 @@ export function useDebouncedSettingsTextDraft(args: { [flush] ) - // Why on unmount too: closing the pane mid-word must persist the same value typing it would have. - // `flush` has no dependencies, so this cleanup only ever runs on unmount. + // Why unmount: closing the pane (or the settings search hiding the section) mid-word must persist + // the same value typing it would have. `flush` has no dependencies, so this cleanup only ever runs + // on unmount. + // Why beforeunload: a window close or app quit never unmounts the tree, so the cleanup cannot run. + // The close coordinator dispatches a synthetic beforeunload while the tree is still mounted so + // listeners like this one can flush; `updateSettings` issues its IPC synchronously, ahead of the + // close confirmation, so main persists the value before it flushes the store on quit. useEffect(() => { + window.addEventListener('beforeunload', flush) return () => { + window.removeEventListener('beforeunload', flush) flush() } }, [flush])