From 1b95662f49174a0c75cd4592e4f9116d004edefc Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Fri, 4 Sep 2026 02:01:43 +0200 Subject: [PATCH] fix(frontend): keep a draft-only rename consistent with its storage key Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Wy24UHSVRZdDaPWiBay9MG --- .../src/lib/components/ResourceEditor.svelte | 4 + .../src/lib/components/VariableEditor.svelte | 4 + .../useNewItemDraftSync.svelte.dom.test.ts | 82 ++++++++++++++++++- .../components/useNewItemDraftSync.svelte.ts | 30 ++++++- 4 files changed, 117 insertions(+), 3 deletions(-) diff --git a/frontend/src/lib/components/ResourceEditor.svelte b/frontend/src/lib/components/ResourceEditor.svelte index be0caf9aeb..7830f43ad5 100644 --- a/frontend/src/lib/components/ResourceEditor.svelte +++ b/frontend/src/lib/components/ResourceEditor.svelte @@ -237,10 +237,14 @@ !draftValuesEqual({ ...current, path: '' }, { ...openedWith[selected], path: '' }), value: () => (current ? ($state.snapshot(current) as ResourceState) : undefined), pathIsFree: (p) => (selected ? resourcePathIsFree(selected, p) : Promise.resolve(false)), + keyed: (v, p) => ({ ...v, path: p }), onAbandonKey: (ws, p) => { // The handle is pinned to the path this editor opened; once the draft // has moved off it, its next write would recreate the row it left. if (p === initialPath) UserDraft.stopSync('resource', p, { workspace: ws }) + }, + onResumeKey: (ws, p) => { + if (p === initialPath) UserDraft.restartSync('resource', p, { workspace: ws }) } }) diff --git a/frontend/src/lib/components/VariableEditor.svelte b/frontend/src/lib/components/VariableEditor.svelte index 45d05447b1..93773e1c55 100644 --- a/frontend/src/lib/components/VariableEditor.svelte +++ b/frontend/src/lib/components/VariableEditor.svelte @@ -149,10 +149,14 @@ !draftValuesEqual({ ...current, path: '' }, { ...openedWith[selected], path: '' }), value: () => (current ? ($state.snapshot(current) as VariableState) : undefined), pathIsFree: (p) => (selected ? variablePathIsFree(selected, p) : Promise.resolve(false)), + keyed: (v, p) => ({ ...v, path: p }), onAbandonKey: (ws, p) => { // The handle is pinned to the path this editor opened; once the draft // has moved off it, its next write would recreate the row it left. if (p === editPath) UserDraft.stopSync('variable', p, { workspace: ws }) + }, + onResumeKey: (ws, p) => { + if (p === editPath) UserDraft.restartSync('variable', p, { workspace: ws }) } }) diff --git a/frontend/src/lib/components/useNewItemDraftSync.svelte.dom.test.ts b/frontend/src/lib/components/useNewItemDraftSync.svelte.dom.test.ts index d7619afcb3..3870492d0b 100644 --- a/frontend/src/lib/components/useNewItemDraftSync.svelte.dom.test.ts +++ b/frontend/src/lib/components/useNewItemDraftSync.svelte.dom.test.ts @@ -4,12 +4,14 @@ import { flushSync } from 'svelte' const save = vi.fn() const remove = vi.fn() const discard = vi.fn() +const forcePersist = vi.fn(async () => {}) const flush = vi.fn(async () => {}) vi.mock('$lib/userDraft.svelte', () => ({ UserDraft: { save: (...a: unknown[]) => save(...a), remove: (...a: unknown[]) => remove(...a), - discard: (...a: unknown[]) => discard(...a) + discard: (...a: unknown[]) => discard(...a), + forcePersist: (...a: unknown[]) => forcePersist(...a) } })) vi.mock('$lib/userDraftDbSyncer.svelte', () => ({ @@ -220,6 +222,84 @@ describe('useNewItemDraftSync', () => { cleanup() }) + /** The list synthesizes a draft-only row from the path INSIDE the draft while + * get and delete address the key, so a stored draft has to describe its own + * key. A rename the draft can't follow yet must not smuggle the new path + * into the row it is still living in. */ + it('stores a draft describing the key it lives under, not an unusable path', async () => { + const form = $state({ path: 'u/me/home', pathError: '', n: 1 }) + let sync: ReturnType | undefined + const cleanup = $effect.root(() => { + sync = useNewItemDraftSync({ + itemKind: 'resource', + enabled: () => true, + workspace: () => 'w', + path: () => form.path, + pathError: () => form.pathError, + contentTouched: () => false, + value: () => ({ path: form.path, n: form.n }), + keyed: (v, p) => ({ ...v, path: p }) + }) + sync.adopt('w', 'u/me/home', { path: 'u/me/home', n: 1 }) + }) + flushSync() + + // Renamed to a path the draft cannot move to, then edited. + form.path = 'u/me/taken' + form.pathError = 'path already used' + form.n = 2 + flushSync() + vi.advanceTimersByTime(2000) + flushSync() + await sync!.flush() + expect(save).toHaveBeenLastCalledWith( + 'resource', + 'u/me/home', + { path: 'u/me/home', n: 2 }, + { workspace: 'w' } + ) + }) + + /** Renaming away suspends the handle pinned to the original key; renaming + * back has to resume it, or the draft is deleted at both keys and written + * to neither. */ + it('resumes a key it returns to after abandoning it', async () => { + const form = $state({ path: 'u/me/there', n: 1 }) + const events: string[] = [] + let sync: ReturnType | undefined + const cleanup = $effect.root(() => { + sync = useNewItemDraftSync({ + itemKind: 'resource', + enabled: () => true, + workspace: () => 'w', + path: () => form.path, + pathError: () => '', + contentTouched: () => false, + value: () => ({ n: form.n }), + onAbandonKey: (_ws, p) => events.push(`abandon:${p}`), + onResumeKey: (_ws, p) => events.push(`resume:${p}`) + }) + sync.adopt('w', 'u/me/there', { n: 1 }) + }) + flushSync() + + form.path = 'u/me/away' + flushSync() + vi.advanceTimersByTime(1000) + flushSync() + + form.path = 'u/me/there' + flushSync() + vi.advanceTimersByTime(1000) + flushSync() + await sync!.flush() + expect(events).toEqual(['abandon:u/me/there', 'abandon:u/me/away', 'resume:u/me/there']) + expect(save).toHaveBeenLastCalledWith('resource', 'u/me/there', { n: 1 }, { workspace: 'w' }) + // The resumed handle's own change detection can no-op this write away. + expect(forcePersist).toHaveBeenCalledWith('resource', 'u/me/there', { workspace: 'w' }) + cleanup() + }) + /** An editor's own autosave handle stays pinned to the path it opened, so * once the draft moves the helper has to hand that key back for suspension — * otherwise the handle's next write recreates the row just deleted. */ diff --git a/frontend/src/lib/components/useNewItemDraftSync.svelte.ts b/frontend/src/lib/components/useNewItemDraftSync.svelte.ts index a7368c1582..1af61e8e36 100644 --- a/frontend/src/lib/components/useNewItemDraftSync.svelte.ts +++ b/frontend/src/lib/components/useNewItemDraftSync.svelte.ts @@ -30,6 +30,15 @@ export interface NewItemDraftSyncOptions { * deleted. An editor whose own autosave handle is pinned to that key MUST * suspend it here, or the handle's next write would recreate the row. */ onAbandonKey?: (workspace: string, path: string) => void + /** Called before writing to a key previously passed to `onAbandonKey`, so + * the editor can resume the handle it suspended there. */ + onResumeKey?: (workspace: string, path: string) => void + /** Return `value` with its own path set to `path`. A stored draft has to + * describe the key it lives under: the list synthesizes a draft-only row + * from the path INSIDE the draft, while get and delete address the key, so + * letting the two diverge makes the row unreachable. Divergence is normal + * while the form holds a path the draft cannot move to yet. */ + keyed?: (value: V, path: string) => V } export interface NewItemDraftSync { @@ -90,6 +99,8 @@ export function useNewItemDraftSync(opts: NewItemDraftSyncOptions): NewIte // edits anything, and an invalid path leaves it where it is rather than // deleting it. Only a brand-new item's draft is gated on being touched. let adopted = false + // Keys handed to `onAbandonKey`, so a return to one can resume it. + const abandoned = new Set() function markUnsettled(workspace: string, path: string): void { if (!unsettled.some((k) => k.workspace === workspace && k.path === path)) { @@ -110,14 +121,28 @@ export function useNewItemDraftSync(opts: NewItemDraftSyncOptions): NewIte // mid-rename. The fallback leaves the cell holding what the form holds. UserDraft.discard(opts.itemKind, written.path, value, { workspace: written.workspace }) opts.onAbandonKey?.(written.workspace, written.path) + abandoned.add(`${written.workspace}/${written.path}`) markUnsettled(written.workspace, written.path) written = undefined writtenValue = undefined } if (!workspace || !path || value === undefined) return - const serialized = JSON.stringify(value) + // Stored describing its own key: while the form holds a path the draft + // cannot move to yet, the row must still name where it actually lives. + const stored = opts.keyed ? opts.keyed(value, path) : value + const serialized = JSON.stringify(stored) if (written && serialized === writtenValue) return - UserDraft.save(opts.itemKind, path, value, { workspace }) + const resumed = abandoned.delete(`${workspace}/${path}`) + if (resumed) opts.onResumeKey?.(workspace, path) + UserDraft.save(opts.itemKind, path, stored, { workspace }) + if (resumed) { + // A live handle at this key mirrors CHANGES, and its baseline advanced + // while it was suspended — the value we just restored can equal it, so + // nothing would be sent and the row we deleted on the way out would + // never come back. Safe here: only a never-deployed draft is keyed this + // way, so there is no baseline this could overwrite. + void UserDraft.forcePersist(opts.itemKind, path, { workspace }) + } markUnsettled(workspace, path) written = { workspace, path } writtenValue = serialized @@ -230,6 +255,7 @@ export function useNewItemDraftSync(opts: NewItemDraftSyncOptions): NewIte reset() { finished = false adopted = false + abandoned.clear() dropPending() written = undefined writtenValue = undefined