fix(canvas): gate autosave on canvas provenance, not selection
The previous guard compared the pinned design against the live activeDesignId (the selection). On a design switch the selection flips synchronously while the new canvas loads asynchronously, so the on-screen nodes briefly still belong to the previous design — arming the timer with the newly-selected id would save those stale nodes under the wrong design. Track the design the canvas was actually loaded as (loadedDesignId provenance) and gate autosave on that instead: pin it when the timer is armed and re-check the live provenance at fire time, skipping the save if a different canvas has since loaded. Also validate the persisted `enabled` flag by type (mirror of `delay`), and add the matching regression tests (provenance switch skip, enabled type validation). ha-relevant: maybe
This commit is contained in:
+15
-2
@@ -66,6 +66,14 @@ export default function App() {
|
|||||||
const [autosave, setAutosave] = useState<AutosaveSettings>(readAutosaveSettings)
|
const [autosave, setAutosave] = useState<AutosaveSettings>(readAutosaveSettings)
|
||||||
useEffect(() => subscribeAutosaveSettings(setAutosave), [])
|
useEffect(() => subscribeAutosaveSettings(setAutosave), [])
|
||||||
|
|
||||||
|
// Provenance: which design the in-memory canvas was loaded as. Differs from
|
||||||
|
// activeDesignId (the selection) during a switch, so autosave gates on this to
|
||||||
|
// avoid writing one design's canvas under another's id. Ref mirror for the
|
||||||
|
// fire-time guard, which must read the live value without re-arming the timer.
|
||||||
|
const [loadedDesignId, setLoadedDesignId] = useState<string | null>(null)
|
||||||
|
const loadedDesignIdRef = useRef<string | null>(loadedDesignId)
|
||||||
|
useEffect(() => { loadedDesignIdRef.current = loadedDesignId }, [loadedDesignId])
|
||||||
|
|
||||||
const [themeModalOpen, setThemeModalOpen] = useState(false)
|
const [themeModalOpen, setThemeModalOpen] = useState(false)
|
||||||
const [styleEditorType, setStyleEditorType] = useState<NodeType | null>(null)
|
const [styleEditorType, setStyleEditorType] = useState<NodeType | null>(null)
|
||||||
const [searchOpen, setSearchOpen] = useState(false)
|
const [searchOpen, setSearchOpen] = useState(false)
|
||||||
@@ -133,9 +141,9 @@ export default function App() {
|
|||||||
enabled: autosave.enabled,
|
enabled: autosave.enabled,
|
||||||
delaySeconds: autosave.delay,
|
delaySeconds: autosave.delay,
|
||||||
hasUnsavedChanges,
|
hasUnsavedChanges,
|
||||||
activeDesignId,
|
designId: loadedDesignId,
|
||||||
changeSignals: [nodes, edges],
|
changeSignals: [nodes, edges],
|
||||||
getActiveDesignId: () => useDesignStore.getState().activeDesignId,
|
getLiveDesignId: () => loadedDesignIdRef.current,
|
||||||
onSave: (designId) => { void handleSaveRef.current(designId, { silent: true }) },
|
onSave: (designId) => { void handleSaveRef.current(designId, { silent: true }) },
|
||||||
})
|
})
|
||||||
|
|
||||||
@@ -168,6 +176,9 @@ export default function App() {
|
|||||||
} catch {
|
} catch {
|
||||||
setFloorMap(null)
|
setFloorMap(null)
|
||||||
loadCanvas(demoNodes, demoEdges)
|
loadCanvas(demoNodes, demoEdges)
|
||||||
|
} finally {
|
||||||
|
// Record provenance so autosave writes back under the design just loaded.
|
||||||
|
setLoadedDesignId(designId ?? null)
|
||||||
}
|
}
|
||||||
}, [loadCanvas, setTheme, setCustomStyle, setFloorMap])
|
}, [loadCanvas, setTheme, setCustomStyle, setFloorMap])
|
||||||
|
|
||||||
@@ -186,6 +197,8 @@ export default function App() {
|
|||||||
setFloorMap(null)
|
setFloorMap(null)
|
||||||
loadCanvas(demoNodes, demoEdges)
|
loadCanvas(demoNodes, demoEdges)
|
||||||
}
|
}
|
||||||
|
// Record provenance so autosave writes back under the design just loaded.
|
||||||
|
setLoadedDesignId(designId)
|
||||||
}, [loadCanvas, setTheme, setCustomStyle, setFloorMap])
|
}, [loadCanvas, setTheme, setCustomStyle, setFloorMap])
|
||||||
|
|
||||||
const loadDesignsAndCanvas = useCallback(async () => {
|
const loadDesignsAndCanvas = useCallback(async () => {
|
||||||
|
|||||||
@@ -6,21 +6,21 @@ interface Args {
|
|||||||
enabled?: boolean
|
enabled?: boolean
|
||||||
delaySeconds?: number
|
delaySeconds?: number
|
||||||
hasUnsavedChanges?: boolean
|
hasUnsavedChanges?: boolean
|
||||||
activeDesignId?: string | null
|
designId?: string | null
|
||||||
changeSignals?: unknown[]
|
changeSignals?: unknown[]
|
||||||
getActiveDesignId?: () => string | null
|
getLiveDesignId?: () => string | null
|
||||||
onSave?: (designId: string) => void
|
onSave?: (designId: string) => void
|
||||||
}
|
}
|
||||||
|
|
||||||
function args(over: Args = {}) {
|
function args(over: Args = {}) {
|
||||||
const activeDesignId = 'activeDesignId' in over ? (over.activeDesignId ?? null) : 'design-a'
|
const designId = 'designId' in over ? (over.designId ?? null) : 'design-a'
|
||||||
return {
|
return {
|
||||||
enabled: over.enabled ?? true,
|
enabled: over.enabled ?? true,
|
||||||
delaySeconds: over.delaySeconds ?? 5,
|
delaySeconds: over.delaySeconds ?? 5,
|
||||||
hasUnsavedChanges: over.hasUnsavedChanges ?? true,
|
hasUnsavedChanges: over.hasUnsavedChanges ?? true,
|
||||||
activeDesignId,
|
designId,
|
||||||
changeSignals: over.changeSignals ?? [[], []],
|
changeSignals: over.changeSignals ?? [[], []],
|
||||||
getActiveDesignId: over.getActiveDesignId ?? (() => activeDesignId),
|
getLiveDesignId: over.getLiveDesignId ?? (() => designId),
|
||||||
onSave: over.onSave ?? vi.fn(),
|
onSave: over.onSave ?? vi.fn(),
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -36,7 +36,8 @@ describe('useAutosave', () => {
|
|||||||
vi.advanceTimersByTime(4999)
|
vi.advanceTimersByTime(4999)
|
||||||
expect(onSave).not.toHaveBeenCalled()
|
expect(onSave).not.toHaveBeenCalled()
|
||||||
vi.advanceTimersByTime(1)
|
vi.advanceTimersByTime(1)
|
||||||
expect(onSave).toHaveBeenCalledExactlyOnceWith('design-a')
|
expect(onSave).toHaveBeenCalledTimes(1)
|
||||||
|
expect(onSave).toHaveBeenCalledWith('design-a')
|
||||||
})
|
})
|
||||||
|
|
||||||
it('does nothing when disabled', () => {
|
it('does nothing when disabled', () => {
|
||||||
@@ -53,9 +54,9 @@ describe('useAutosave', () => {
|
|||||||
expect(onSave).not.toHaveBeenCalled()
|
expect(onSave).not.toHaveBeenCalled()
|
||||||
})
|
})
|
||||||
|
|
||||||
it('does nothing when there is no active design', () => {
|
it('does nothing when the canvas has no known provenance', () => {
|
||||||
const onSave = vi.fn()
|
const onSave = vi.fn()
|
||||||
renderHook(() => useAutosave(args({ onSave, activeDesignId: null })))
|
renderHook(() => useAutosave(args({ onSave, designId: null })))
|
||||||
vi.advanceTimersByTime(60_000)
|
vi.advanceTimersByTime(60_000)
|
||||||
expect(onSave).not.toHaveBeenCalled()
|
expect(onSave).not.toHaveBeenCalled()
|
||||||
})
|
})
|
||||||
@@ -70,25 +71,27 @@ describe('useAutosave', () => {
|
|||||||
vi.advanceTimersByTime(4000)
|
vi.advanceTimersByTime(4000)
|
||||||
expect(onSave).not.toHaveBeenCalled()
|
expect(onSave).not.toHaveBeenCalled()
|
||||||
vi.advanceTimersByTime(1000)
|
vi.advanceTimersByTime(1000)
|
||||||
expect(onSave).toHaveBeenCalledExactlyOnceWith('design-a')
|
expect(onSave).toHaveBeenCalledTimes(1)
|
||||||
})
|
})
|
||||||
|
|
||||||
it('skips the save if the active design changed while the timer was pending', () => {
|
it('skips the save if a different canvas loaded while the timer was pending', () => {
|
||||||
const onSave = vi.fn()
|
const onSave = vi.fn()
|
||||||
// Pinned at arm time = 'design-a', but live id has moved on to 'design-b'.
|
// Pinned provenance at arm time = 'design-a', but a switch loaded 'design-b'
|
||||||
|
// before the timer fired — the on-screen canvas is no longer design-a's.
|
||||||
renderHook(() =>
|
renderHook(() =>
|
||||||
useAutosave(args({ onSave, activeDesignId: 'design-a', getActiveDesignId: () => 'design-b' })),
|
useAutosave(args({ onSave, designId: 'design-a', getLiveDesignId: () => 'design-b' })),
|
||||||
)
|
)
|
||||||
vi.advanceTimersByTime(5000)
|
vi.advanceTimersByTime(5000)
|
||||||
expect(onSave).not.toHaveBeenCalled()
|
expect(onSave).not.toHaveBeenCalled()
|
||||||
})
|
})
|
||||||
|
|
||||||
it('saves under the pinned design id, not the live one, when they still match', () => {
|
it('saves under the pinned provenance id when it still matches at fire time', () => {
|
||||||
const onSave = vi.fn()
|
const onSave = vi.fn()
|
||||||
renderHook(() =>
|
renderHook(() =>
|
||||||
useAutosave(args({ onSave, activeDesignId: 'design-a', getActiveDesignId: () => 'design-a' })),
|
useAutosave(args({ onSave, designId: 'design-a', getLiveDesignId: () => 'design-a' })),
|
||||||
)
|
)
|
||||||
vi.advanceTimersByTime(5000)
|
vi.advanceTimersByTime(5000)
|
||||||
expect(onSave).toHaveBeenCalledExactlyOnceWith('design-a')
|
expect(onSave).toHaveBeenCalledTimes(1)
|
||||||
|
expect(onSave).toHaveBeenCalledWith('design-a')
|
||||||
})
|
})
|
||||||
})
|
})
|
||||||
|
|||||||
@@ -7,22 +7,28 @@ interface UseAutosaveOptions {
|
|||||||
delaySeconds: number
|
delaySeconds: number
|
||||||
/** True when the canvas has edits not yet persisted. */
|
/** True when the canvas has edits not yet persisted. */
|
||||||
hasUnsavedChanges: boolean
|
hasUnsavedChanges: boolean
|
||||||
/** The design the in-memory canvas currently belongs to. */
|
/**
|
||||||
activeDesignId: string | null
|
* The design the in-memory canvas actually belongs to — its *provenance*, set
|
||||||
|
* when a canvas is loaded, NOT the currently-selected design. These differ
|
||||||
|
* during a design switch: the selection flips synchronously while the new
|
||||||
|
* canvas loads asynchronously, so for a brief window the on-screen nodes still
|
||||||
|
* belong to the previous design. Gating on provenance (not selection) is what
|
||||||
|
* prevents saving one design's canvas under another design's id.
|
||||||
|
*/
|
||||||
|
designId: string | null
|
||||||
/**
|
/**
|
||||||
* Values that represent canvas edits (e.g. nodes, edges). Any change to one
|
* 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
|
* 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
|
* period. Its length MUST stay constant across renders (React requires a
|
||||||
* what counts as "activity".
|
* stable dependency-array size) — pass a fixed-shape array like [nodes, edges].
|
||||||
*/
|
*/
|
||||||
changeSignals: unknown[]
|
changeSignals: readonly unknown[]
|
||||||
/**
|
/**
|
||||||
* Reads the *live* active design id at fire time. Used to detect that the user
|
* Reads the *live* canvas provenance at fire time. If it no longer matches the
|
||||||
* switched designs while the timer was pending — if so, the in-memory canvas
|
* id pinned when the timer was armed, a different canvas has since loaded, so
|
||||||
* now belongs to a different design and saving it under the pinned id would
|
* the save is skipped rather than written under the wrong (now-stale) id.
|
||||||
* clobber the wrong design, so the save is skipped.
|
|
||||||
*/
|
*/
|
||||||
getActiveDesignId: () => string | null
|
getLiveDesignId: () => string | null
|
||||||
/** Persist the canvas under the given design id. */
|
/** Persist the canvas under the given design id. */
|
||||||
onSave: (designId: string) => void
|
onSave: (designId: string) => void
|
||||||
}
|
}
|
||||||
@@ -36,30 +42,31 @@ export function useAutosave({
|
|||||||
enabled,
|
enabled,
|
||||||
delaySeconds,
|
delaySeconds,
|
||||||
hasUnsavedChanges,
|
hasUnsavedChanges,
|
||||||
activeDesignId,
|
designId,
|
||||||
changeSignals,
|
changeSignals,
|
||||||
getActiveDesignId,
|
getLiveDesignId,
|
||||||
onSave,
|
onSave,
|
||||||
}: UseAutosaveOptions): void {
|
}: UseAutosaveOptions): void {
|
||||||
// Keep the latest callbacks in refs so the timer always calls the current
|
// 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.
|
// versions without re-arming (which would reset the debounce) on every render.
|
||||||
const onSaveRef = useRef(onSave)
|
const onSaveRef = useRef(onSave)
|
||||||
const getActiveDesignIdRef = useRef(getActiveDesignId)
|
const getLiveDesignIdRef = useRef(getLiveDesignId)
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
onSaveRef.current = onSave
|
onSaveRef.current = onSave
|
||||||
getActiveDesignIdRef.current = getActiveDesignId
|
getLiveDesignIdRef.current = getLiveDesignId
|
||||||
})
|
})
|
||||||
|
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
if (!enabled || !hasUnsavedChanges || !activeDesignId) return
|
if (!enabled || !hasUnsavedChanges || !designId) return
|
||||||
const designId = activeDesignId
|
const pinnedDesignId = designId
|
||||||
const t = setTimeout(() => {
|
const t = setTimeout(() => {
|
||||||
// Skip if the active design changed while the timer was pending.
|
// Skip if a different canvas has loaded while the timer was pending.
|
||||||
if (getActiveDesignIdRef.current() !== designId) return
|
if (getLiveDesignIdRef.current() !== pinnedDesignId) return
|
||||||
onSaveRef.current(designId)
|
onSaveRef.current(pinnedDesignId)
|
||||||
}, delaySeconds * 1000)
|
}, delaySeconds * 1000)
|
||||||
return () => clearTimeout(t)
|
return () => clearTimeout(t)
|
||||||
// changeSignals is spread so any canvas edit resets the debounce.
|
// changeSignals is spread so any canvas edit resets the debounce; its length
|
||||||
|
// must stay constant (documented on the option).
|
||||||
// eslint-disable-next-line react-hooks/exhaustive-deps
|
// eslint-disable-next-line react-hooks/exhaustive-deps
|
||||||
}, [enabled, delaySeconds, hasUnsavedChanges, activeDesignId, ...changeSignals])
|
}, [enabled, delaySeconds, hasUnsavedChanges, designId, ...changeSignals])
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -46,6 +46,11 @@ describe('autosaveSettings', () => {
|
|||||||
expect(readAutosaveSettings().delay).toBe(DEFAULT_AUTOSAVE_SETTINGS.delay)
|
expect(readAutosaveSettings().delay).toBe(DEFAULT_AUTOSAVE_SETTINGS.delay)
|
||||||
})
|
})
|
||||||
|
|
||||||
|
it('rejects a non-boolean enabled and falls back to default', () => {
|
||||||
|
localStorage.setItem('homelable.autosave', JSON.stringify({ enabled: 'yes', delay: 10 }))
|
||||||
|
expect(readAutosaveSettings().enabled).toBe(DEFAULT_AUTOSAVE_SETTINGS.enabled)
|
||||||
|
})
|
||||||
|
|
||||||
it('notifies subscribers on write and stops after unsubscribe', () => {
|
it('notifies subscribers on write and stops after unsubscribe', () => {
|
||||||
const listener = vi.fn()
|
const listener = vi.fn()
|
||||||
const unsubscribe = subscribeAutosaveSettings(listener)
|
const unsubscribe = subscribeAutosaveSettings(listener)
|
||||||
|
|||||||
@@ -19,7 +19,7 @@ export function readAutosaveSettings(): AutosaveSettings {
|
|||||||
if (!raw) return DEFAULT_AUTOSAVE_SETTINGS
|
if (!raw) return DEFAULT_AUTOSAVE_SETTINGS
|
||||||
const parsed = JSON.parse(raw) as Partial<AutosaveSettings>
|
const parsed = JSON.parse(raw) as Partial<AutosaveSettings>
|
||||||
return {
|
return {
|
||||||
enabled: parsed.enabled ?? DEFAULT_AUTOSAVE_SETTINGS.enabled,
|
enabled: typeof parsed.enabled === 'boolean' ? parsed.enabled : DEFAULT_AUTOSAVE_SETTINGS.enabled,
|
||||||
delay: typeof parsed.delay === 'number' && parsed.delay > 0 ? parsed.delay : DEFAULT_AUTOSAVE_SETTINGS.delay,
|
delay: typeof parsed.delay === 'number' && parsed.delay > 0 ? parsed.delay : DEFAULT_AUTOSAVE_SETTINGS.delay,
|
||||||
}
|
}
|
||||||
} catch {
|
} catch {
|
||||||
|
|||||||
Reference in New Issue
Block a user