fix(settings): flush a pending text-setting draft on beforeunload and drop the dirty flag

A window close or app quit never unmounts the React tree, so the unmount
flush could not run and a value typed within the last 700ms was lost. The
close coordinator already dispatches a synthetic beforeunload while the tree
is mounted, so listen for it the same way the session checkpoint does.

The pending timer is now the single source of truth for "uncommitted edits";
the separate dirty flag it duplicated is gone.
This commit is contained in:
Neil
2026-09-04 14:29:14 -07:00
parent 806fee13e8
commit 3a6304b8bd
2 changed files with 144 additions and 14 deletions
@@ -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')
})
})
@@ -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 `<Input>` 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<ReturnType<typeof setTimeout> | 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])