fix(canvas): guard autosave against design-switch clobber, add tests
Extract the debounced autosave into a useAutosave hook. Pin the active design id when the timer is armed and re-check it at fire time: if the user switched designs while the timer was pending, the in-memory canvas belongs to a different design, so skip the save instead of writing it under the wrong design id. Also skip when there is no active design. Add unit tests for autosaveSettings (persistence, validation, pub/sub) and useAutosave (debounce, enable/unsaved/design guards, switch skip). ha-relevant: maybe
This commit is contained in:
Pouzor
+12
-7
@@ -38,6 +38,7 @@ import { ShortcutsModal } from '@/components/modals/ShortcutsModal'
|
||||
import { ConfirmAddToGroupModal } from '@/components/modals/ConfirmAddToGroupModal'
|
||||
import { useCanvasStore } from '@/stores/canvasStore'
|
||||
import { readAutosaveSettings, subscribeAutosaveSettings, type AutosaveSettings } from '@/utils/autosaveSettings'
|
||||
import { useAutosave } from '@/hooks/useAutosave'
|
||||
import { useDesignStore } from '@/stores/designStore'
|
||||
import { useAuthStore } from '@/stores/authStore'
|
||||
import { useThemeStore } from '@/stores/themeStore'
|
||||
@@ -126,13 +127,17 @@ export default function App() {
|
||||
const handleSaveRef = useRef(handleSave)
|
||||
useEffect(() => { handleSaveRef.current = handleSave }, [handleSave])
|
||||
|
||||
// Autosave: debounce — resets on every node/edge change, fires after `delay` seconds
|
||||
// of inactivity if there are unsaved changes and autosave is enabled.
|
||||
useEffect(() => {
|
||||
if (!autosave.enabled || !hasUnsavedChanges) return
|
||||
const t = setTimeout(() => { void handleSaveRef.current(undefined, { silent: true }) }, autosave.delay * 1000)
|
||||
return () => clearTimeout(t)
|
||||
}, [nodes, edges, autosave.enabled, autosave.delay, hasUnsavedChanges])
|
||||
// Debounced, opt-in autosave. Pins the active design when armed and re-checks
|
||||
// it at fire time so a mid-switch save can't clobber the wrong design.
|
||||
useAutosave({
|
||||
enabled: autosave.enabled,
|
||||
delaySeconds: autosave.delay,
|
||||
hasUnsavedChanges,
|
||||
activeDesignId,
|
||||
changeSignals: [nodes, edges],
|
||||
getActiveDesignId: () => useDesignStore.getState().activeDesignId,
|
||||
onSave: (designId) => { void handleSaveRef.current(designId, { silent: true }) },
|
||||
})
|
||||
|
||||
const loadCanvasFromApi = useCallback(async (designId?: string) => {
|
||||
try {
|
||||
|
||||
@@ -0,0 +1,94 @@
|
||||
import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'
|
||||
import { renderHook } from '@testing-library/react'
|
||||
import { useAutosave } from '../useAutosave'
|
||||
|
||||
interface Args {
|
||||
enabled?: boolean
|
||||
delaySeconds?: number
|
||||
hasUnsavedChanges?: boolean
|
||||
activeDesignId?: string | null
|
||||
changeSignals?: unknown[]
|
||||
getActiveDesignId?: () => string | null
|
||||
onSave?: (designId: string) => void
|
||||
}
|
||||
|
||||
function args(over: Args = {}) {
|
||||
const activeDesignId = 'activeDesignId' in over ? (over.activeDesignId ?? null) : 'design-a'
|
||||
return {
|
||||
enabled: over.enabled ?? true,
|
||||
delaySeconds: over.delaySeconds ?? 5,
|
||||
hasUnsavedChanges: over.hasUnsavedChanges ?? true,
|
||||
activeDesignId,
|
||||
changeSignals: over.changeSignals ?? [[], []],
|
||||
getActiveDesignId: over.getActiveDesignId ?? (() => activeDesignId),
|
||||
onSave: over.onSave ?? vi.fn(),
|
||||
}
|
||||
}
|
||||
|
||||
describe('useAutosave', () => {
|
||||
beforeEach(() => vi.useFakeTimers())
|
||||
afterEach(() => vi.useRealTimers())
|
||||
|
||||
it('saves after the inactivity delay when enabled with unsaved changes', () => {
|
||||
const onSave = vi.fn()
|
||||
renderHook(() => useAutosave(args({ onSave, delaySeconds: 5 })))
|
||||
expect(onSave).not.toHaveBeenCalled()
|
||||
vi.advanceTimersByTime(4999)
|
||||
expect(onSave).not.toHaveBeenCalled()
|
||||
vi.advanceTimersByTime(1)
|
||||
expect(onSave).toHaveBeenCalledExactlyOnceWith('design-a')
|
||||
})
|
||||
|
||||
it('does nothing when disabled', () => {
|
||||
const onSave = vi.fn()
|
||||
renderHook(() => useAutosave(args({ onSave, enabled: false })))
|
||||
vi.advanceTimersByTime(60_000)
|
||||
expect(onSave).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('does nothing when there are no unsaved changes', () => {
|
||||
const onSave = vi.fn()
|
||||
renderHook(() => useAutosave(args({ onSave, hasUnsavedChanges: false })))
|
||||
vi.advanceTimersByTime(60_000)
|
||||
expect(onSave).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('does nothing when there is no active design', () => {
|
||||
const onSave = vi.fn()
|
||||
renderHook(() => useAutosave(args({ onSave, activeDesignId: null })))
|
||||
vi.advanceTimersByTime(60_000)
|
||||
expect(onSave).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('debounces: a change before the delay resets the timer', () => {
|
||||
const onSave = vi.fn()
|
||||
const { rerender } = renderHook((p: Args) => useAutosave(args(p)), {
|
||||
initialProps: { onSave, changeSignals: [1] },
|
||||
})
|
||||
vi.advanceTimersByTime(4000)
|
||||
rerender({ onSave, changeSignals: [2] }) // edit resets debounce
|
||||
vi.advanceTimersByTime(4000)
|
||||
expect(onSave).not.toHaveBeenCalled()
|
||||
vi.advanceTimersByTime(1000)
|
||||
expect(onSave).toHaveBeenCalledExactlyOnceWith('design-a')
|
||||
})
|
||||
|
||||
it('skips the save if the active design changed while the timer was pending', () => {
|
||||
const onSave = vi.fn()
|
||||
// Pinned at arm time = 'design-a', but live id has moved on to 'design-b'.
|
||||
renderHook(() =>
|
||||
useAutosave(args({ onSave, activeDesignId: 'design-a', getActiveDesignId: () => 'design-b' })),
|
||||
)
|
||||
vi.advanceTimersByTime(5000)
|
||||
expect(onSave).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('saves under the pinned design id, not the live one, when they still match', () => {
|
||||
const onSave = vi.fn()
|
||||
renderHook(() =>
|
||||
useAutosave(args({ onSave, activeDesignId: 'design-a', getActiveDesignId: () => 'design-a' })),
|
||||
)
|
||||
vi.advanceTimersByTime(5000)
|
||||
expect(onSave).toHaveBeenCalledExactlyOnceWith('design-a')
|
||||
})
|
||||
})
|
||||
@@ -0,0 +1,65 @@
|
||||
import { useEffect, useRef } from 'react'
|
||||
|
||||
interface UseAutosaveOptions {
|
||||
/** Whether autosave is enabled (user opt-in). */
|
||||
enabled: boolean
|
||||
/** Inactivity delay in seconds before firing a save. */
|
||||
delaySeconds: number
|
||||
/** True when the canvas has edits not yet persisted. */
|
||||
hasUnsavedChanges: boolean
|
||||
/** The design the in-memory canvas currently belongs to. */
|
||||
activeDesignId: string | null
|
||||
/**
|
||||
* Values that represent canvas edits (e.g. nodes, edges). Any change to one
|
||||
* of these resets the debounce timer, so the save only fires after a quiet
|
||||
* period. Kept separate from the trigger flags so the caller controls exactly
|
||||
* what counts as "activity".
|
||||
*/
|
||||
changeSignals: unknown[]
|
||||
/**
|
||||
* Reads the *live* active design id at fire time. Used to detect that the user
|
||||
* switched designs while the timer was pending — if so, the in-memory canvas
|
||||
* now belongs to a different design and saving it under the pinned id would
|
||||
* clobber the wrong design, so the save is skipped.
|
||||
*/
|
||||
getActiveDesignId: () => string | null
|
||||
/** Persist the canvas under the given design id. */
|
||||
onSave: (designId: string) => void
|
||||
}
|
||||
|
||||
/**
|
||||
* Debounced canvas autosave. Fires `onSave(designId)` after `delaySeconds` of
|
||||
* inactivity when enabled and there are unsaved changes. Opt-in only — the
|
||||
* caller decides whether `enabled` is set (see ADR: autosave defaults to off).
|
||||
*/
|
||||
export function useAutosave({
|
||||
enabled,
|
||||
delaySeconds,
|
||||
hasUnsavedChanges,
|
||||
activeDesignId,
|
||||
changeSignals,
|
||||
getActiveDesignId,
|
||||
onSave,
|
||||
}: UseAutosaveOptions): void {
|
||||
// Keep the latest callbacks in refs so the timer always calls the current
|
||||
// versions without re-arming (which would reset the debounce) on every render.
|
||||
const onSaveRef = useRef(onSave)
|
||||
const getActiveDesignIdRef = useRef(getActiveDesignId)
|
||||
useEffect(() => {
|
||||
onSaveRef.current = onSave
|
||||
getActiveDesignIdRef.current = getActiveDesignId
|
||||
})
|
||||
|
||||
useEffect(() => {
|
||||
if (!enabled || !hasUnsavedChanges || !activeDesignId) return
|
||||
const designId = activeDesignId
|
||||
const t = setTimeout(() => {
|
||||
// Skip if the active design changed while the timer was pending.
|
||||
if (getActiveDesignIdRef.current() !== designId) return
|
||||
onSaveRef.current(designId)
|
||||
}, delaySeconds * 1000)
|
||||
return () => clearTimeout(t)
|
||||
// changeSignals is spread so any canvas edit resets the debounce.
|
||||
// eslint-disable-next-line react-hooks/exhaustive-deps
|
||||
}, [enabled, delaySeconds, hasUnsavedChanges, activeDesignId, ...changeSignals])
|
||||
}
|
||||
@@ -0,0 +1,58 @@
|
||||
import { describe, it, expect, beforeEach, vi } from 'vitest'
|
||||
import {
|
||||
DEFAULT_AUTOSAVE_SETTINGS,
|
||||
readAutosaveSettings,
|
||||
writeAutosaveSettings,
|
||||
subscribeAutosaveSettings,
|
||||
} from '../autosaveSettings'
|
||||
|
||||
describe('autosaveSettings', () => {
|
||||
beforeEach(() => {
|
||||
localStorage.clear()
|
||||
})
|
||||
|
||||
it('defaults to disabled with a 5s delay', () => {
|
||||
expect(DEFAULT_AUTOSAVE_SETTINGS).toEqual({ enabled: false, delay: 5 })
|
||||
})
|
||||
|
||||
it('returns defaults when nothing stored', () => {
|
||||
expect(readAutosaveSettings()).toEqual(DEFAULT_AUTOSAVE_SETTINGS)
|
||||
})
|
||||
|
||||
it('roundtrips through localStorage', () => {
|
||||
writeAutosaveSettings({ enabled: true, delay: 30 })
|
||||
expect(readAutosaveSettings()).toEqual({ enabled: true, delay: 30 })
|
||||
})
|
||||
|
||||
it('falls back to defaults when stored value is corrupted', () => {
|
||||
localStorage.setItem('homelable.autosave', '{not json')
|
||||
expect(readAutosaveSettings()).toEqual(DEFAULT_AUTOSAVE_SETTINGS)
|
||||
})
|
||||
|
||||
it('fills missing fields from defaults', () => {
|
||||
localStorage.setItem('homelable.autosave', JSON.stringify({ enabled: true }))
|
||||
expect(readAutosaveSettings()).toEqual({ enabled: true, delay: DEFAULT_AUTOSAVE_SETTINGS.delay })
|
||||
})
|
||||
|
||||
it('rejects a non-positive delay and falls back to default', () => {
|
||||
localStorage.setItem('homelable.autosave', JSON.stringify({ enabled: true, delay: 0 }))
|
||||
expect(readAutosaveSettings().delay).toBe(DEFAULT_AUTOSAVE_SETTINGS.delay)
|
||||
localStorage.setItem('homelable.autosave', JSON.stringify({ enabled: true, delay: -10 }))
|
||||
expect(readAutosaveSettings().delay).toBe(DEFAULT_AUTOSAVE_SETTINGS.delay)
|
||||
})
|
||||
|
||||
it('rejects a non-numeric delay and falls back to default', () => {
|
||||
localStorage.setItem('homelable.autosave', JSON.stringify({ enabled: true, delay: 'soon' }))
|
||||
expect(readAutosaveSettings().delay).toBe(DEFAULT_AUTOSAVE_SETTINGS.delay)
|
||||
})
|
||||
|
||||
it('notifies subscribers on write and stops after unsubscribe', () => {
|
||||
const listener = vi.fn()
|
||||
const unsubscribe = subscribeAutosaveSettings(listener)
|
||||
writeAutosaveSettings({ enabled: true, delay: 10 })
|
||||
expect(listener).toHaveBeenCalledWith({ enabled: true, delay: 10 })
|
||||
unsubscribe()
|
||||
writeAutosaveSettings({ enabled: false, delay: 3 })
|
||||
expect(listener).toHaveBeenCalledTimes(1)
|
||||
})
|
||||
})
|
||||
Reference in New Issue
Block a user