fix(frontend): validate and settle draft keys around close and save

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Wy24UHSVRZdDaPWiBay9MG

frontend/src/lib/components/AppConnectInner.svelte

@@ -329,7 +329,15 @@
 	}
 
 	let pathError = $state('')
-	let pathDirty = $state(false)
+
+	async function resourcePathIsFree(p: string): Promise<boolean> {
+		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 @@
 				<ResourcePathHint />
 				<Path
 					bind:error={pathError}
-					bind:dirty={pathDirty}
 					bind:path
 					initialPath=""
 					namePlaceholder={resourceType}

frontend/src/lib/components/ResourceEditor.svelte

@@ -205,7 +205,19 @@
 	)
 
 	let pathError = $state('')
-	let pathDirty = $state(false)
+	// Set before `save` awaits anything: the drawer closes without awaiting the
+	// write, and the close-time draft move would otherwise leave a draft on the
+	// resource being created.
+	let saving = $state(false)
+
+	async function resourcePathIsFree(ws: string, p: string): Promise<boolean> {
+		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<unknown> {
+	async function moveRenamedDraftOnly(): Promise<void> {
 		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<void> {
 		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 @@
 				<ResourceForm
 					bind:path={() => current!.path, setPath}
 					bind:pathError
-					bind:pathDirty
 					bind:labels={current.labels}
 					bind:description={current.description}
 					bind:args={current.args}

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<string, any>
@@ -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"

frontend/src/lib/components/VariableEditor.svelte

@@ -58,7 +58,18 @@
 	let perWsUser: Record<string, UserExt | undefined> = $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<boolean> {
+		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<VariableState>(() =>
 		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<unknown> {
+	async function moveRenamedDraftOnly(): Promise<void> {
 		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<void> {
 		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}

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 @@
 	<Path
 		disabled={initialPath != '' && !isOwner(initialPath, $userStore, ws)}
 		bind:error={pathError}
-		bind:dirty={pathDirty}
 		bind:path
 		{initialPath}
 		namePlaceholder="variable"

frontend/src/lib/components/useNewItemDraftSync.svelte.dom.test.ts

@@ -38,7 +38,7 @@ describe('useNewItemDraftSync', () => {
 				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<typeof useNewItemDraftSync> | 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<typeof useNewItemDraftSync> | 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<typeof useNewItemDraftSync> | undefined
@@ -137,7 +235,7 @@ describe('useNewItemDraftSync', () => {
 				workspace: () => 'w',
 				path: () => form.path,
 				pathError: () => '',
-				touched: () => form.touched,
+				contentTouched: () => form.touched,
 				value: () => ({ n: form.n })
 			})
 		})

frontend/src/lib/components/useNewItemDraftSync.svelte.ts

@@ -16,13 +16,16 @@ export interface NewItemDraftSyncOptions<V> {
 	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<boolean>
 }
 
 export interface NewItemDraftSync {
@@ -55,6 +58,10 @@ export function useNewItemDraftSync<V>(opts: NewItemDraftSyncOptions<V>): 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<V>(opts: NewItemDraftSyncOptions<V>): NewIte
 		| { timer: ReturnType<typeof setTimeout>; 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<V>(opts: NewItemDraftSyncOptions<V>): 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<V>(opts: NewItemDraftSyncOptions<V>): 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<V>(opts: NewItemDraftSyncOptions<V>): NewIte
 	})
 
 	async function settle(): Promise<void> {
-		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<V>(opts: NewItemDraftSyncOptions<V>): 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<V>(opts: NewItemDraftSyncOptions<V>): 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 = ''
 		}
 	}