diff --git a/frontend/src/lib/components/AppConnectInner.svelte b/frontend/src/lib/components/AppConnectInner.svelte index 1060e7fc02..fa55b79976 100644 --- a/frontend/src/lib/components/AppConnectInner.svelte +++ b/frontend/src/lib/components/AppConnectInner.svelte @@ -329,7 +329,15 @@ } let pathError = $state('') - let pathDirty = $state(false) + + async function resourcePathIsFree(p: string): Promise { + try { + return !(await ResourceService.existsResource({ workspace: effectiveWorkspace, path: p })) + } catch { + // Fail closed: an unanswered check is not evidence the path is free. + return false + } + } // Fields saved as linked secret variables never enter the draft: a resource // draft is stored as-is, without the encryption those variables get. @@ -349,8 +357,7 @@ workspace: () => effectiveWorkspace, path: () => path, pathError: () => pathError, - touched: () => - pathDirty || + contentTouched: () => description !== '' || (labels?.length ?? 0) > 0 || wsSpecific || @@ -367,7 +374,8 @@ labels, wsSpecific, resource_type: resourceType - }) + }), + pathIsFree: resourcePathIsFree }) export async function open(rt?: string) { @@ -1015,10 +1023,11 @@ export async function back() { if (step == 2 && manual) { - // Back abandons this form; the draft it mirrored goes with it. - await newDraftSync.finish() + // Back abandons this form; the draft it mirrored goes with it. Not + // awaited: the step change is the user's feedback and must not wait on + // a POST (`finish` captures what it deletes before returning). + void newDraftSync.finish() newDraftSync.reset() - pathDirty = false } if (step == 4) { step -= 2 @@ -1351,7 +1360,6 @@ { + try { + return !(await ResourceService.existsResource({ workspace: ws, path: p })) + } catch { + // Fail closed: an unanswered check is not evidence the path is free. + return false + } + } // A new resource's handle is keyed on the empty `initialPath`, so it is // detached and never POSTs; the form is mirrored under the typed path @@ -216,12 +228,12 @@ workspace: () => selected, path: () => current?.path ?? '', pathError: () => pathError, - touched: () => - pathDirty || - (!!current && - !!selected && - !draftValuesEqual({ ...current, path: '' }, { ...initialStates[selected], path: '' })), - value: () => (current ? ($state.snapshot(current) as ResourceState) : undefined) + contentTouched: () => + !!current && + !!selected && + !draftValuesEqual({ ...current, path: '' }, { ...initialStates[selected], path: '' }), + value: () => (current ? ($state.snapshot(current) as ResourceState) : undefined), + pathIsFree: (p) => (selected ? resourcePathIsFree(selected, p) : Promise.resolve(false)) }) // New-resource bootstrap: seed empty state per workspace (edit mode @@ -337,15 +349,21 @@ * 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 { + async function moveRenamedDraftOnly(): Promise { const ws = selected - if (!ws || !initialPath || existedInitially[ws] !== false) return Promise.resolve() + if (!ws || !initialPath || existedInitially[ws] !== false || saving) return 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 }) + if (!s || !s.path || s.path === initialPath || pathError !== '') return + const value = $state.snapshot(s) as ResourceState + const target = s.path + // The path may have been typed too recently for `Path`'s debounced check + // to have run: moving onto an occupied path would hand this draft to the + // item living there. + if (!(await resourcePathIsFree(ws, target))) return + UserDraft.save('resource', target, value, { workspace: ws }) UserDraft.remove('resource', initialPath, { workspace: ws }) - return Promise.all([ - UserDraftDbSyncer.flush({ workspace: ws, itemKind: 'resource', path: s.path }), + await Promise.all([ + UserDraftDbSyncer.flush({ workspace: ws, itemKind: 'resource', path: target }), UserDraftDbSyncer.flush({ workspace: ws, itemKind: 'resource', path: initialPath }) ]) } @@ -408,6 +426,9 @@ export async function save(): Promise { const dirty = dirtyWorkspaces + // Synchronous, before the first await: the drawer closes right after + // calling this, and the close-time draft move must see it. + saving = true try { for (const ws of dirty) { const s = states[ws].draft! @@ -447,6 +468,10 @@ // Reset the handle to the new deployed baseline via `discard`, not // `remove`. See VariableEditor for the full rationale. UserDraft.discard('resource', initialPath, s, { workspace: ws }) + // Flushed: the caller refetches the list right after, and the + // discard's delete rides the same debounce — the just-deployed item + // would still come back rendered as a draft. + await UserDraftDbSyncer.flush({ workspace: ws, itemKind: 'resource', path: initialPath }) } else { // Awaited: the caller refetches the list right after, and a debounced // delete would leave the just-created item still flagged as a draft. @@ -479,7 +504,6 @@ current!.path, setPath} bind:pathError - bind:pathDirty bind:labels={current.labels} bind:description={current.description} bind:args={current.args} diff --git a/frontend/src/lib/components/ResourceForm.svelte b/frontend/src/lib/components/ResourceForm.svelte index 3f56b10669..305b0b049b 100644 --- a/frontend/src/lib/components/ResourceForm.svelte +++ b/frontend/src/lib/components/ResourceForm.svelte @@ -31,8 +31,6 @@ hidePath?: boolean /** `Path`'s validation error (`''` when valid). */ pathError: string - /** Whether the user edited the path (as opposed to `Path`'s auto-filled name). */ - pathDirty: boolean labels: string[] | undefined description: string args: Record @@ -58,7 +56,6 @@ initialPath, hidePath = false, pathError = $bindable(), - pathDirty = $bindable(), labels = $bindable(), description = $bindable(), args = $bindable(), @@ -167,7 +164,6 @@ disabled={initialPath != '' && !isOwner(initialPath, $userStore, ws)} bind:path bind:error={pathError} - bind:dirty={pathDirty} {initialPath} namePlaceholder="resource" kind="resource" diff --git a/frontend/src/lib/components/VariableEditor.svelte b/frontend/src/lib/components/VariableEditor.svelte index 5bbb9d041b..5844a01ab7 100644 --- a/frontend/src/lib/components/VariableEditor.svelte +++ b/frontend/src/lib/components/VariableEditor.svelte @@ -58,7 +58,18 @@ let perWsUser: Record = $state({}) let selected: string | undefined = $state(undefined) let pathError = $state('') - let pathDirty = $state(false) + // Set before `save` awaits anything, so a close-time draft move can't leave + // a draft on the variable being created. + let saving = $state(false) + + async function variablePathIsFree(ws: string, p: string): Promise { + try { + return !(await VariableService.existsVariable({ workspace: ws, path: p })) + } catch { + // Fail closed: an unanswered check is not evidence the path is free. + return false + } + } const handlesArray = UserDraft.useMany(() => workspaceSpecs.map((s) => ({ @@ -128,12 +139,12 @@ workspace: () => selected, path: () => current?.path ?? '', pathError: () => pathError, - touched: () => - pathDirty || - (!!current && - !!selected && - !draftValuesEqual({ ...current, path: '' }, { ...initialStates[selected], path: '' })), - value: () => (current ? ($state.snapshot(current) as VariableState) : undefined) + contentTouched: () => + !!current && + !!selected && + !draftValuesEqual({ ...current, path: '' }, { ...initialStates[selected], path: '' }), + value: () => (current ? ($state.snapshot(current) as VariableState) : undefined), + pathIsFree: (p) => (selected ? variablePathIsFree(selected, p) : Promise.resolve(false)) }) // The list-page `*` hint is owned by UserDraftDbSyncer (set on save, cleared @@ -236,16 +247,22 @@ * 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 { + async function moveRenamedDraftOnly(): Promise { const ws = selected - if (!ws || !editPath || existedInitially[ws] !== false) return Promise.resolve() + if (!ws || !editPath || existedInitially[ws] !== false || saving) return const s = states[ws]?.draft - if (!s || !s.path || s.path === editPath) return Promise.resolve() + if (!s || !s.path || s.path === editPath || pathError !== '') return + const value = $state.snapshot(s) as VariableState + const target = s.path const from = editPath - UserDraft.save('variable', s.path, $state.snapshot(s) as VariableState, { workspace: ws }) + // The path may have been typed too recently for `Path`'s debounced check + // to have run: moving onto an occupied path would hand this draft to the + // item living there. + if (!(await variablePathIsFree(ws, target))) return + UserDraft.save('variable', target, value, { workspace: ws }) UserDraft.remove('variable', from, { workspace: ws }) - return Promise.all([ - UserDraftDbSyncer.flush({ workspace: ws, itemKind: 'variable', path: s.path }), + await Promise.all([ + UserDraftDbSyncer.flush({ workspace: ws, itemKind: 'variable', path: target }), UserDraftDbSyncer.flush({ workspace: ws, itemKind: 'variable', path: from }) ]) } @@ -266,7 +283,7 @@ extraPerms = {} perWsUser = {} pathError = '' - pathDirty = false + saving = false newDraftSync.reset() } @@ -312,6 +329,8 @@ async function save(): Promise { const dirty = dirtyWorkspaces + // Synchronous, before the first await, so the close-time draft move sees it. + saving = true try { for (const ws of dirty) { const s = states[ws].draft! @@ -354,6 +373,10 @@ existedInitially[ws] = true if (editPath) { UserDraft.discard('variable', editPath, s, { workspace: ws }) + // Flushed: the caller refetches the list right after, and the + // discard's delete rides the same debounce — the just-deployed item + // would still come back rendered as a draft. + await UserDraftDbSyncer.flush({ workspace: ws, itemKind: 'variable', path: editPath }) } else { // Awaited: the caller refetches the list right after, and a debounced // delete would leave the just-created item still flagged as a draft. @@ -436,7 +459,6 @@ bind:this={form} bind:path={current.path} bind:pathError - bind:pathDirty bind:variable={current.variable} bind:labels={current.labels} bind:wsSpecific={current.wsSpecific} diff --git a/frontend/src/lib/components/VariableForm.svelte b/frontend/src/lib/components/VariableForm.svelte index d01ea17c1b..662f949957 100644 --- a/frontend/src/lib/components/VariableForm.svelte +++ b/frontend/src/lib/components/VariableForm.svelte @@ -25,8 +25,6 @@ path: string initialPath: string pathError: string - /** Whether the user edited the path (as opposed to `Path`'s auto-filled name). */ - pathDirty: boolean variable: Variable labels: string[] | undefined wsSpecific: boolean @@ -42,7 +40,6 @@ path = $bindable(), initialPath, pathError = $bindable(), - pathDirty = $bindable(), variable = $bindable(), labels = $bindable(), wsSpecific = $bindable(), @@ -76,7 +73,6 @@ { workspace: () => 'w', path: () => form.path, pathError: () => form.pathError, - touched: () => form.touched, + contentTouched: () => form.touched, value: () => ({ n: form.n }) }) $effect(() => { @@ -103,7 +103,7 @@ describe('useNewItemDraftSync', () => { workspace: () => 'w', path: () => form.path, pathError: () => '', - touched: () => form.touched, + contentTouched: () => form.touched, value: () => ({ n: form.n }) }) }) @@ -127,6 +127,104 @@ describe('useNewItemDraftSync', () => { }) }) + /** A close cuts the commit delay short, so `Path`'s debounced existence + * check may not have run. Keying a draft on an occupied path would hand it + * to the item already there, and saving from that item would overwrite it. */ + it('refuses to commit a forced flush onto an occupied path', async () => { + const form = $state({ path: 'u/me/taken', touched: false, n: 1 }) + let sync: ReturnType | undefined + const cleanup = $effect.root(() => { + sync = useNewItemDraftSync({ + itemKind: 'resource', + enabled: () => true, + workspace: () => 'w', + path: () => form.path, + // Still clear: the check that would set it has not run yet. + pathError: () => '', + contentTouched: () => form.touched, + value: () => ({ n: form.n }), + pathIsFree: async () => false + }) + }) + flushSync() + form.touched = true + flushSync() + + await sync!.flush() + expect(save).not.toHaveBeenCalled() + cleanup() + }) + + /** A move deletes the key it left on the same debounce as the write, so the + * close-time flush has to settle both or the list refetch renders a ghost + * row at the old path. */ + it('settles the key a move deleted, not just the one it wrote', async () => { + const form = $state({ path: 'u/me/first', touched: true, n: 1 }) + let sync: ReturnType | undefined + const cleanup = $effect.root(() => { + sync = useNewItemDraftSync({ + itemKind: 'resource', + enabled: () => true, + workspace: () => 'w', + path: () => form.path, + pathError: () => '', + contentTouched: () => form.touched, + value: () => ({ n: form.n }) + }) + }) + flushSync() + vi.advanceTimersByTime(1000) + flushSync() + + form.path = 'u/me/second' + flushSync() + vi.advanceTimersByTime(1000) + flushSync() + + await sync!.flush() + const flushed = flush.mock.calls.map((c: any[]) => c[0].path) + expect(flushed).toContain('u/me/first') + expect(flushed).toContain('u/me/second') + cleanup() + }) + + /** `Path` auto-fills a unique name on mount and flips its own `dirty` on any + * keyup, tabbing included. Only a departure from that name counts, or an + * untouched drawer would leave a phantom row behind. */ + it('treats the auto-filled path as untouched but a typed one as an edit', () => { + const form = $state({ path: '', n: 1 }) + const cleanup = $effect.root(() => { + useNewItemDraftSync({ + itemKind: 'resource', + enabled: () => true, + workspace: () => 'w', + path: () => form.path, + pathError: () => '', + contentTouched: () => false, + value: () => ({ n: form.n }) + }) + }) + flushSync() + // `Path` fills its generated name in after mount. + form.path = 'u/me/lucky_resource' + flushSync() + vi.advanceTimersByTime(2000) + flushSync() + expect(save).not.toHaveBeenCalled() + + form.path = 'u/me/typed_by_hand' + flushSync() + vi.advanceTimersByTime(1000) + flushSync() + expect(save).toHaveBeenCalledWith( + 'resource', + 'u/me/typed_by_hand', + { n: 1 }, + { workspace: 'w' } + ) + cleanup() + }) + 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 @@ -137,7 +235,7 @@ describe('useNewItemDraftSync', () => { workspace: () => 'w', path: () => form.path, pathError: () => '', - touched: () => form.touched, + contentTouched: () => form.touched, value: () => ({ n: form.n }) }) }) diff --git a/frontend/src/lib/components/useNewItemDraftSync.svelte.ts b/frontend/src/lib/components/useNewItemDraftSync.svelte.ts index dfa102cf53..7c81456fa2 100644 --- a/frontend/src/lib/components/useNewItemDraftSync.svelte.ts +++ b/frontend/src/lib/components/useNewItemDraftSync.svelte.ts @@ -16,13 +16,16 @@ export interface NewItemDraftSyncOptions { path: () => string /** Reactive `Path` validation error (`''` when valid). */ pathError: () => string - /** Reactive: the user edited the name or the content. `Path` auto-fills a - * name on mount, so opening and closing an untouched drawer must not leave - * a draft behind. */ - touched: () => boolean + /** Reactive: the user edited the form's content. The path is not part of + * this — `Path` auto-fills a name on mount, and this helper tracks a + * departure from that name itself. */ + contentTouched: () => boolean /** Reactive deep read of the value to persist (`$state.snapshot` of the * form state); `undefined` while there is nothing to persist. */ value: () => V | undefined + /** Whether nothing is deployed at `path` yet. Consulted only when a close + * forces a commit early, where `Path`'s own check may not have run. */ + pathIsFree?: (path: string) => Promise } export interface NewItemDraftSync { @@ -55,6 +58,10 @@ export function useNewItemDraftSync(opts: NewItemDraftSyncOptions): NewIte // the row actually left behind, not wherever the form points now. let written: { workspace: string; path: string } | undefined let writtenValue: string | undefined + // Every key this session has touched and not yet settled — the deletes a + // move leaves behind included, since those POST on the same debounce as the + // write and would otherwise still be pending when the list refetches. + let unsettled: { workspace: string; path: string }[] = [] // 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 @@ -63,10 +70,27 @@ export function useNewItemDraftSync(opts: NewItemDraftSyncOptions): NewIte | { timer: ReturnType; key: string; value: V | undefined } | undefined let pendingWorkspace: string | undefined + // `Path` auto-fills a unique name on mount, so a non-empty path is no + // evidence the user did anything. Only a departure from the name it settled + // on counts (`Path.dirty` can't: it flips on any keyup, tabbing included). + let autoPath: string | undefined + + function markUnsettled(workspace: string, path: string): void { + if (!unsettled.some((k) => k.workspace === workspace && k.path === path)) { + unsettled.push({ workspace, path }) + } + } + + function touched(): boolean { + const p = opts.path() + if (autoPath === undefined && p !== '') autoPath = p + return opts.contentTouched() || (p !== '' && p !== autoPath) + } 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 }) + markUnsettled(written.workspace, written.path) written = undefined writtenValue = undefined } @@ -74,6 +98,7 @@ export function useNewItemDraftSync(opts: NewItemDraftSyncOptions): NewIte const serialized = JSON.stringify(value) if (written && serialized === writtenValue) return UserDraft.save(opts.itemKind, path, value, { workspace }) + markUnsettled(workspace, path) written = { workspace, path } writtenValue = serialized } @@ -95,7 +120,7 @@ export function useNewItemDraftSync(opts: NewItemDraftSyncOptions): NewIte $effect(() => { if (!opts.enabled() || finished) return const p = opts.path() - const key = p !== '' && opts.pathError() === '' && opts.touched() ? p : '' + const key = p !== '' && opts.pathError() === '' && touched() ? p : '' const workspace = opts.workspace() const value = opts.value() if (key === untrack(() => draftPath)) { @@ -121,12 +146,13 @@ export function useNewItemDraftSync(opts: NewItemDraftSyncOptions): NewIte }) async function settle(): Promise { - if (!written) return - await UserDraftDbSyncer.flush({ - workspace: written.workspace, - itemKind: opts.itemKind, - path: written.path - }) + const keys = unsettled + unsettled = [] + await Promise.all( + keys.map((k) => + UserDraftDbSyncer.flush({ workspace: k.workspace, itemKind: opts.itemKind, path: k.path }) + ) + ) } return { @@ -134,6 +160,16 @@ export function useNewItemDraftSync(opts: NewItemDraftSyncOptions): NewIte return draftPath }, async flush() { + const p = pending + if (p && p.key) { + // Forced by a close, so the commit delay that lets `Path`'s debounced + // existence check land was cut short. Re-check before keying on it: + // a path that already holds an item would take this draft as an edit + // of that item, and saving from there would overwrite its value. + const free = + opts.pathError() === '' && (opts.pathIsFree ? await opts.pathIsFree(p.key) : true) + if (!free && pending === p) dropPending() + } commit() await settle() }, @@ -144,19 +180,19 @@ export function useNewItemDraftSync(opts: NewItemDraftSyncOptions): NewIte 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 - }) + if (w) { + UserDraft.remove(opts.itemKind, w.path, { workspace: w.workspace }) + markUnsettled(w.workspace, w.path) + } + await settle() }, reset() { finished = false dropPending() written = undefined writtenValue = undefined + unsettled = [] + autoPath = undefined draftPath = '' } }