From ed1ccc05b9e837954eef617b430af50b3023f523 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Fri, 4 Sep 2026 00:30:08 +0200 Subject: [PATCH] fix(frontend): settle new-item drafts before the drawer closes Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Wy24UHSVRZdDaPWiBay9MG --- .../lib/components/AppConnectDrawer.svelte | 7 +- .../src/lib/components/AppConnectInner.svelte | 12 +- .../src/lib/components/ResourceEditor.svelte | 30 ++++- .../components/ResourceEditorDrawer.svelte | 6 +- .../src/lib/components/ResourceForm.svelte | 8 +- .../src/lib/components/VariableEditor.svelte | 38 +++++- .../src/lib/components/VariableForm.svelte | 4 +- .../useNewItemDraftSync.svelte.dom.test.ts | 50 +++++++- .../components/useNewItemDraftSync.svelte.ts | 115 ++++++++++++++---- .../(root)/(logged)/resources/+page.svelte | 14 ++- .../(root)/(logged)/variables/+page.svelte | 14 ++- 11 files changed, 242 insertions(+), 56 deletions(-) diff --git a/frontend/src/lib/components/AppConnectDrawer.svelte b/frontend/src/lib/components/AppConnectDrawer.svelte index 8de484929e..d0772c807f 100644 --- a/frontend/src/lib/components/AppConnectDrawer.svelte +++ b/frontend/src/lib/components/AppConnectDrawer.svelte @@ -73,9 +73,14 @@ { + on:close={async () => { + // Flushed before the step reset (which retires the form the draft mirrors) + // and before `close`, whose list refetch would otherwise outrun the draft's + // own commit delay and the syncer's debounce, and miss the new row. + const flushed = appConnectInner?.flushDraft() step = 1 handedOff = false + await flushed dispatch('close') }} size="700px" diff --git a/frontend/src/lib/components/AppConnectInner.svelte b/frontend/src/lib/components/AppConnectInner.svelte index 226142eb9c..1060e7fc02 100644 --- a/frontend/src/lib/components/AppConnectInner.svelte +++ b/frontend/src/lib/components/AppConnectInner.svelte @@ -993,7 +993,9 @@ } }) } - newDraftSync.finish() + // Awaited: `refresh` refetches the list, and a debounced delete would + // leave the just-created resource still flagged as a draft. + await newDraftSync.finish() dispatch('refresh', path) dispatch('close') sendUserToast( @@ -1005,10 +1007,16 @@ } } + /** Settle the pending draft write before the drawer's close event, whose + * list refetch would otherwise outrun the debounced POST. */ + export async function flushDraft(): Promise { + await newDraftSync.flush() + } + export async function back() { if (step == 2 && manual) { // Back abandons this form; the draft it mirrored goes with it. - newDraftSync.finish() + await newDraftSync.finish() newDraftSync.reset() pathDirty = false } diff --git a/frontend/src/lib/components/ResourceEditor.svelte b/frontend/src/lib/components/ResourceEditor.svelte index 43b6d138ed..59eab2ed6f 100644 --- a/frontend/src/lib/components/ResourceEditor.svelte +++ b/frontend/src/lib/components/ResourceEditor.svelte @@ -331,6 +331,32 @@ export function localDraftCurrent(): ResourceState | undefined { return current } + /** A draft-only item's list row is keyed by the path INSIDE the draft, while + * the autosave handle stays keyed on the path the editor opened. Renaming + * one would leave the row pointing at a key that holds no draft — it would + * 404 on reopen and its delete would miss — so move the draft to the path + * the form now carries. Reads its state synchronously: the caller runs this + * as the drawer closes, and awaiting first would race the editor's teardown. */ + function moveRenamedDraftOnly(): Promise { + const ws = selected + if (!ws || !initialPath || existedInitially[ws] !== false) return Promise.resolve() + const s = states[ws]?.draft + if (!s || !s.path || s.path === initialPath) return Promise.resolve() + UserDraft.save('resource', s.path, $state.snapshot(s) as ResourceState, { workspace: ws }) + UserDraft.remove('resource', initialPath, { workspace: ws }) + return Promise.all([ + UserDraftDbSyncer.flush({ workspace: ws, itemKind: 'resource', path: s.path }), + UserDraftDbSyncer.flush({ workspace: ws, itemKind: 'resource', path: initialPath }) + ]) + } + + /** Settle every pending draft write. The drawer awaits this before its + * `onClose`, whose list refetch would otherwise outrun the debounced POST. */ + export async function flushDraft(): Promise { + // Both started before the first await so they read live editor state. + await Promise.all([newDraftSync.flush(), moveRenamedDraftOnly()]) + } + /** Returns true when the item was draft-only: discarding deleted it * outright, so there is nothing left for the editor to show. */ export async function discardLocalDraft(): Promise { @@ -422,7 +448,9 @@ // `remove`. See VariableEditor for the full rationale. UserDraft.discard('resource', initialPath, s, { workspace: ws }) } else { - newDraftSync.finish() + // Awaited: the caller refetches the list right after, and a debounced + // delete would leave the just-created item still flagged as a draft. + await newDraftSync.finish() } // Path now exists server-side — drop the autocomplete cache so // it shows up immediately instead of after the 60s TTL. diff --git a/frontend/src/lib/components/ResourceEditorDrawer.svelte b/frontend/src/lib/components/ResourceEditorDrawer.svelte index 5f7317963a..6c8ea4ad89 100644 --- a/frontend/src/lib/components/ResourceEditorDrawer.svelte +++ b/frontend/src/lib/components/ResourceEditorDrawer.svelte @@ -49,6 +49,7 @@ localDraftDeployed: () => unknown localDraftCurrent: () => unknown discardLocalDraft: () => Promise + flushDraft: () => Promise } | undefined = $state(undefined) let hasLocalDraft = $state(false) @@ -101,8 +102,11 @@ bind:this={drawer} size="50rem" {disableChatOffset} - on:close={() => { + on:close={async () => { clearPageDrawerAnchor(RESOURCES_PATH) + // Before `onClose`: its list refetch would otherwise outrun the draft's + // own commit delay and the syncer's debounce, and miss the new row. + await resourceEditor?.flushDraft() onClose?.() }} > diff --git a/frontend/src/lib/components/ResourceForm.svelte b/frontend/src/lib/components/ResourceForm.svelte index 5ee39d7ebe..3f56b10669 100644 --- a/frontend/src/lib/components/ResourceForm.svelte +++ b/frontend/src/lib/components/ResourceForm.svelte @@ -30,9 +30,9 @@ initialPath: string hidePath?: boolean /** `Path`'s validation error (`''` when valid). */ - pathError?: string + pathError: string /** Whether the user edited the path (as opposed to `Path`'s auto-filled name). */ - pathDirty?: boolean + pathDirty: boolean labels: string[] | undefined description: string args: Record @@ -57,8 +57,8 @@ path = $bindable(), initialPath, hidePath = false, - pathError = $bindable(''), - pathDirty = $bindable(false), + pathError = $bindable(), + pathDirty = $bindable(), labels = $bindable(), description = $bindable(), args = $bindable(), diff --git a/frontend/src/lib/components/VariableEditor.svelte b/frontend/src/lib/components/VariableEditor.svelte index 50cfeeb7aa..5bbb9d041b 100644 --- a/frontend/src/lib/components/VariableEditor.svelte +++ b/frontend/src/lib/components/VariableEditor.svelte @@ -230,6 +230,33 @@ }) }) + /** A draft-only item's list row is keyed by the path INSIDE the draft, while + * the autosave handle stays keyed on the path the editor opened. Renaming + * one would leave the row pointing at a key that holds no draft — it would + * 404 on reopen and its delete would miss — so move the draft to the path + * the form now carries. Reads its state synchronously: the caller runs this + * as the drawer closes, and awaiting first would race the form's teardown. */ + function moveRenamedDraftOnly(): Promise { + const ws = selected + if (!ws || !editPath || existedInitially[ws] !== false) return Promise.resolve() + const s = states[ws]?.draft + if (!s || !s.path || s.path === editPath) return Promise.resolve() + const from = editPath + UserDraft.save('variable', s.path, $state.snapshot(s) as VariableState, { workspace: ws }) + UserDraft.remove('variable', from, { workspace: ws }) + return Promise.all([ + UserDraftDbSyncer.flush({ workspace: ws, itemKind: 'variable', path: s.path }), + UserDraftDbSyncer.flush({ workspace: ws, itemKind: 'variable', path: from }) + ]) + } + + /** Settle every pending draft write before the drawer's close event, whose + * list refetch would otherwise outrun the debounced POST. */ + async function flushDraft(): Promise { + // Both started before the first await so they read live form state. + await Promise.all([newDraftSync.flush(), moveRenamedDraftOnly()]) + } + function reset() { // Clearing workspaceSpecs triggers useMany's reconcile to release // every acquired entry. The $derived `states` then collapses to {}. @@ -328,7 +355,9 @@ if (editPath) { UserDraft.discard('variable', editPath, s, { workspace: ws }) } else { - newDraftSync.finish() + // Awaited: the caller refetches the list right after, and a debounced + // delete would leave the just-created item still flagged as a draft. + await newDraftSync.finish() } // Path now exists server-side — drop the autocomplete cache so // it shows up immediately instead of after the 60s TTL. @@ -346,10 +375,11 @@ { + on:close={async () => { clearPageDrawerAnchor(VARIABLES_PATH) - // A new variable left unsaved persists as a draft-only row, which a - // list only sees on refetch. + // A new variable left unsaved persists as a draft-only row, which a list + // only sees on refetch — settle the write before asking for that refetch. + await flushDraft() dispatch('close') }} > diff --git a/frontend/src/lib/components/VariableForm.svelte b/frontend/src/lib/components/VariableForm.svelte index e03f524b3b..d01ea17c1b 100644 --- a/frontend/src/lib/components/VariableForm.svelte +++ b/frontend/src/lib/components/VariableForm.svelte @@ -26,7 +26,7 @@ initialPath: string pathError: string /** Whether the user edited the path (as opposed to `Path`'s auto-filled name). */ - pathDirty?: boolean + pathDirty: boolean variable: Variable labels: string[] | undefined wsSpecific: boolean @@ -42,7 +42,7 @@ path = $bindable(), initialPath, pathError = $bindable(), - pathDirty = $bindable(false), + pathDirty = $bindable(), variable = $bindable(), labels = $bindable(), wsSpecific = $bindable(), diff --git a/frontend/src/lib/components/useNewItemDraftSync.svelte.dom.test.ts b/frontend/src/lib/components/useNewItemDraftSync.svelte.dom.test.ts index cf6fa8727a..76bb1ded2b 100644 --- a/frontend/src/lib/components/useNewItemDraftSync.svelte.dom.test.ts +++ b/frontend/src/lib/components/useNewItemDraftSync.svelte.dom.test.ts @@ -3,12 +3,16 @@ import { flushSync } from 'svelte' const save = vi.fn() const remove = vi.fn() +const flush = vi.fn(async () => {}) vi.mock('$lib/userDraft.svelte', () => ({ UserDraft: { save: (...a: unknown[]) => save(...a), remove: (...a: unknown[]) => remove(...a) } })) +vi.mock('$lib/userDraftDbSyncer.svelte', () => ({ + UserDraftDbSyncer: { flush: (...a: unknown[]) => flush(...(a as [])) } +})) import { useNewItemDraftSync } from './useNewItemDraftSync.svelte' @@ -18,8 +22,8 @@ afterEach(() => { vi.clearAllMocks() }) -/** Drives the helper through a new-item drawer session and pins the writes - * it must and must not make: nothing for an untouched form (the path field +/** Drives the helper through a new-item drawer session and pins the writes it + * must and must not make: nothing for an untouched form (the path field * auto-fills a name on mount), a move that deletes the key it left, a delete * once the path fails validation, and no delete at all on teardown — a draft * left behind by closing the drawer is the feature. */ @@ -86,7 +90,44 @@ describe('useNewItemDraftSync', () => { expect(remove).toHaveBeenCalledTimes(2) }) - it('finish deletes the persisted key and stops mirroring until reset', () => { + /** The commit is delayed so the key can't land on a half-typed path, and the + * editor is destroyed the moment its drawer closes. Closing right after the + * first edit therefore tears down mid-delay, and the draft must survive it. */ + it('persists a commit still pending when the editor is torn down', async () => { + const form = $state({ path: 'u/me/quick', touched: false, n: 1 }) + let sync: ReturnType | undefined + const cleanup = $effect.root(() => { + sync = useNewItemDraftSync({ + itemKind: 'resource', + enabled: () => true, + workspace: () => 'w', + path: () => form.path, + pathError: () => '', + touched: () => form.touched, + value: () => ({ n: form.n }) + }) + }) + flushSync() + form.touched = true + flushSync() + + // Torn down half-way through the commit delay. + vi.advanceTimersByTime(500) + cleanup() + vi.advanceTimersByTime(500) + expect(save).toHaveBeenCalledWith('resource', 'u/me/quick', { n: 1 }, { workspace: 'w' }) + + // The drawer's close handler awaits this, so the list refetch behind it + // sees the row rather than racing the syncer's debounce. + await sync!.flush() + expect(flush).toHaveBeenCalledWith({ + workspace: 'w', + itemKind: 'resource', + path: 'u/me/quick' + }) + }) + + it('finish deletes the persisted key and stops mirroring until reset', async () => { const form = $state({ path: 'u/me/item', touched: true, n: 1 }) let sync: ReturnType | undefined const cleanup = $effect.root(() => { @@ -105,8 +146,7 @@ describe('useNewItemDraftSync', () => { flushSync() expect(save).toHaveBeenCalledTimes(1) - sync!.finish() - flushSync() + await sync!.finish() expect(remove).toHaveBeenCalledWith('variable', 'u/me/item', { workspace: 'w' }) form.n = 2 diff --git a/frontend/src/lib/components/useNewItemDraftSync.svelte.ts b/frontend/src/lib/components/useNewItemDraftSync.svelte.ts index 1c87be33f5..dfa102cf53 100644 --- a/frontend/src/lib/components/useNewItemDraftSync.svelte.ts +++ b/frontend/src/lib/components/useNewItemDraftSync.svelte.ts @@ -1,5 +1,6 @@ import { untrack } from 'svelte' import { UserDraft, type UserDraftItemKind } from '$lib/userDraft.svelte' +import { UserDraftDbSyncer } from '$lib/userDraftDbSyncer.svelte' /** Longer than `Path`'s 500ms debounced existence check, so the key never * lands on a half-typed path and the check's verdict is in before a commit. */ @@ -27,8 +28,12 @@ export interface NewItemDraftSyncOptions { export interface NewItemDraftSync { /** Storage path of the persisted draft, `''` when none. */ readonly draftPath: string + /** Commit anything still pending and settle it server-side. Callers MUST + * await this before a list refetch (both the commit delay and the syncer's + * own debounce outlive a closing drawer, so a refetch would miss the row). */ + flush(): Promise /** Delete the persisted draft and stop mirroring, once the item is created. */ - finish(): void + finish(): Promise /** Re-arm for the next drawer session (an editor instance that outlives * its drawer). Forgets the previous session's key without deleting it: a * draft left behind by closing the drawer is the point. */ @@ -46,50 +51,112 @@ export interface NewItemDraftSync { export function useNewItemDraftSync(opts: NewItemDraftSyncOptions): NewItemDraftSync { let draftPath = $state('') let finished = $state(false) - // Last key actually written: a moved or finished draft deletes exactly the - // row it left behind, and component teardown deletes nothing. - let writtenPath = '' + // The key last written, workspace included: a move or a delete has to target + // the row actually left behind, not wherever the form points now. + let written: { workspace: string; path: string } | undefined + let writtenValue: string | undefined + // The commit the timer will make, snapshotted at schedule time so it still + // lands once the editor is gone: closing a drawer a keystroke after the + // first edit must keep the draft, so a pending commit is never cancelled by + // teardown — only superseded by a newer one, or consumed by `flush`. + let pending: + | { timer: ReturnType; key: string; value: V | undefined } + | undefined + let pendingWorkspace: string | undefined + + function write(workspace: string | undefined, path: string, value: V | undefined): void { + if (written && (written.path !== path || written.workspace !== workspace)) { + UserDraft.remove(opts.itemKind, written.path, { workspace: written.workspace }) + written = undefined + writtenValue = undefined + } + if (!workspace || !path || value === undefined) return + const serialized = JSON.stringify(value) + if (written && serialized === writtenValue) return + UserDraft.save(opts.itemKind, path, value, { workspace }) + written = { workspace, path } + writtenValue = serialized + } + + function dropPending(): void { + if (!pending) return + clearTimeout(pending.timer) + pending = undefined + } + + function commit(): void { + const p = pending + if (!p) return + dropPending() + draftPath = p.key + write(pendingWorkspace, p.key, p.value) + } $effect(() => { if (!opts.enabled() || finished) return const p = opts.path() - const target = p !== '' && opts.pathError() === '' && opts.touched() ? p : '' - if (target === untrack(() => draftPath)) return - const t = setTimeout(() => (draftPath = target), COMMIT_DELAY_MS) - return () => clearTimeout(t) + const key = p !== '' && opts.pathError() === '' && opts.touched() ? p : '' + const workspace = opts.workspace() + const value = opts.value() + if (key === untrack(() => draftPath)) { + // Back on the committed key: a transition away from it is stale. + untrack(dropPending) + return + } + untrack(() => { + dropPending() + pendingWorkspace = workspace + pending = { timer: setTimeout(commit, COMMIT_DELAY_MS), key, value } + }) }) $effect(() => { if (!opts.enabled() || finished) return - const ws = opts.workspace() - const p = draftPath - const v = opts.value() + const workspace = opts.workspace() + const key = draftPath + const value = opts.value() untrack(() => { - if (!ws) return - if (writtenPath && writtenPath !== p) { - UserDraft.remove(opts.itemKind, writtenPath, { workspace: ws }) - writtenPath = '' - } - if (!p || v === undefined) return - UserDraft.save(opts.itemKind, p, v, { workspace: ws }) - writtenPath = p + if (key) write(workspace, key, value) }) }) + async function settle(): Promise { + if (!written) return + await UserDraftDbSyncer.flush({ + workspace: written.workspace, + itemKind: opts.itemKind, + path: written.path + }) + } + return { get draftPath() { return draftPath }, - finish() { + async flush() { + commit() + await settle() + }, + async finish() { finished = true - const ws = untrack(() => opts.workspace()) - if (writtenPath && ws) UserDraft.remove(opts.itemKind, writtenPath, { workspace: ws }) - writtenPath = '' + dropPending() + const w = written + written = undefined + writtenValue = undefined draftPath = '' + if (!w) return + UserDraft.remove(opts.itemKind, w.path, { workspace: w.workspace }) + await UserDraftDbSyncer.flush({ + workspace: w.workspace, + itemKind: opts.itemKind, + path: w.path + }) }, reset() { finished = false - writtenPath = '' + dropPending() + written = undefined + writtenValue = undefined draftPath = '' } } diff --git a/frontend/src/routes/(root)/(logged)/resources/+page.svelte b/frontend/src/routes/(root)/(logged)/resources/+page.svelte index 46c3be91e2..5a19ef429a 100644 --- a/frontend/src/routes/(root)/(logged)/resources/+page.svelte +++ b/frontend/src/routes/(root)/(logged)/resources/+page.svelte @@ -1,5 +1,6 @@