diff --git a/frontend/src/lib/components/ResourceEditor.svelte b/frontend/src/lib/components/ResourceEditor.svelte index 9920df30c9..59685fdeba 100644 --- a/frontend/src/lib/components/ResourceEditor.svelte +++ b/frontend/src/lib/components/ResourceEditor.svelte @@ -12,7 +12,12 @@ import { resource } from 'runed' import { getUserExt } from '$lib/user' import type { UserExt } from '$lib/stores' - import { UserDraft, draftValuesEqual, type UserDraftHandle } from '$lib/userDraft.svelte' + import { + UserDraft, + draftValuesEqual, + settleDraftAfterWrite, + type UserDraftHandle + } from '$lib/userDraft.svelte' import { setLocalDraftHint } from '$lib/localDraftHints.svelte' interface Props { @@ -331,12 +336,21 @@ /** Whether the write landed. It toasts its own failure, so most callers ignore this; * one that follows the save with bookkeeping of its own has to know not to. */ export async function save(): Promise { - const dirty = dirtyWorkspaces + // Everything the writes send, read before the first await. The form stays + // editable while they are in flight, so read later these would be whatever + // the user has since typed — sent under an earlier workspace's path, and + // adopted as a baseline the server never saw. + const from = initialPath ?? '' + const payloads = dirtyWorkspaces.map((ws) => ({ + ws, + s: $state.snapshot(states[ws].draft!) as ResourceState, + ini: $state.snapshot(initialStates[ws]) as ResourceState, + existed: !!existedInitially[ws] + })) + const savedPath = payloads[0]?.s.path ?? from try { - for (const ws of dirty) { - const s = states[ws].draft! - const ini = initialStates[ws] - if (existedInitially[ws]) { + for (const { ws, s, ini, existed } of payloads) { + if (existed) { await ResourceService.updateResource({ workspace: ws, path: ini.path, @@ -366,22 +380,20 @@ }) } // Reset the handle to the new deployed baseline via `discard`, not - // `remove`. See VariableEditor for the full rationale — including why the - // cell is only dropped while it still holds what was written. - const written = $state.snapshot(s) as ResourceState - initialStates[ws] = written + // `remove`. See VariableEditor for the full rationale. + initialStates[ws] = s existedInitially[ws] = true - if (draftValuesEqual(states[ws]?.draft, written)) { - UserDraft.discard('resource', initialPath ?? '', written, { workspace: ws }) - } + settleDraftAfterWrite('resource', s, states[ws]?.draft, from, savedPath, { workspace: ws }) // Path now exists server-side — drop the autocomplete cache so // it shows up immediately instead of after the 60s TTL. invalidateWorkspacePaths(ws) } sendUserToast( - dirty.length > 1 ? `Saved resource in ${dirty.length} workspaces` : `Saved resource` + payloads.length > 1 + ? `Saved resource in ${payloads.length} workspaces` + : `Saved resource` ) - dispatch('refresh', current?.path ?? path) + dispatch('refresh', savedPath) return true } catch (err) { sendUserToast(`Could not save resource: ${err.body ?? err.message}`, true) diff --git a/frontend/src/lib/components/ResourceEditorDrawer.svelte b/frontend/src/lib/components/ResourceEditorDrawer.svelte index 4675d02455..c0f9add493 100644 --- a/frontend/src/lib/components/ResourceEditorDrawer.svelte +++ b/frontend/src/lib/components/ResourceEditorDrawer.svelte @@ -218,6 +218,10 @@ // workspace, while the write is in flight. const from = path const fromWs = effectiveWorkspace + // The path the form holds now is the one `save()` is about to send; read + // after the await it would be a rename the user typed meanwhile, and the + // tab would follow to a path this write never created. + const submitted = livePath ?? path // Closed before the write is awaited, the way it always was: `save()` toasts its // own failures and never rejects, so waiting would only add visible lag to every // caller of this drawer. `onSaved` still fires after the write lands. @@ -234,7 +238,7 @@ // Rendered inline there is no drawer to close, so the mounted editor would // otherwise keep the pre-save baseline. Follow a rename before remounting, // or it comes back up on a path the save just moved the item off. - const savedPath = livePath ?? path + const savedPath = submitted if (savedPath) path = savedPath editorGeneration++ onSaved?.(savedPath, from, fromWs) diff --git a/frontend/src/lib/components/VariableEditor.svelte b/frontend/src/lib/components/VariableEditor.svelte index 379b13532e..557dca8def 100644 --- a/frontend/src/lib/components/VariableEditor.svelte +++ b/frontend/src/lib/components/VariableEditor.svelte @@ -22,7 +22,12 @@ import { resource } from 'runed' import { getUserExt } from '$lib/user' import type { UserExt } from '$lib/stores' - import { UserDraft, draftValuesEqual, type UserDraftHandle } from '$lib/userDraft.svelte' + import { + UserDraft, + draftValuesEqual, + settleDraftAfterWrite, + type UserDraftHandle + } from '$lib/userDraft.svelte' import LocalDraftBanner from './LocalDraftBanner.svelte' import { isEncryptedDraftValue } from '$lib/encryptedDraft' import { setLocalDraftHint } from '$lib/localDraftHints.svelte' @@ -329,15 +334,13 @@ initialStates[ws] = s existedInitially[ws] = true } - // The just-saved state is the new deployed baseline; reset the - // handle to it via `discard` (not `remove` — blanking the cell to - // `undefined` reads as dirty). The `value: null` POST also deletes - // the server draft row so `is_draft` clears on refetch. Only while the - // cell still holds what was written: the form stays editable during the - // request, and an edit made then is a change on top of the save. - if (draftValuesEqual(states[ws]?.draft, s)) { - UserDraft.discard('variable', from ?? '', s, { workspace: ws }) - } + // The just-saved state is the new deployed baseline; `settleDraftAfterWrite` + // resets the handle to it via `discard` (not `remove` — blanking the cell + // to `undefined` reads as dirty), and carries a mid-request edit onto the + // path the save wrote to. + settleDraftAfterWrite('variable', s, states[ws]?.draft, from ?? '', savedPath ?? '', { + workspace: ws + }) // Path now exists server-side — drop the autocomplete cache so // it shows up immediately instead of after the 60s TTL. invalidateWorkspacePaths(ws) diff --git a/frontend/src/lib/components/triggers/schedules/ScheduleEditorInner.svelte b/frontend/src/lib/components/triggers/schedules/ScheduleEditorInner.svelte index 040a5ad47e..0e7d51d070 100644 --- a/frontend/src/lib/components/triggers/schedules/ScheduleEditorInner.svelte +++ b/frontend/src/lib/components/triggers/schedules/ScheduleEditorInner.svelte @@ -44,8 +44,7 @@ import { runScheduleNow } from '../scheduled/utils' import { handleConfigChange } from '../utils' import { withForkConflictRetry } from '$lib/utils/forkConflict' - import { deepEqual } from 'fast-equals' - import { normalizeDraftForCompare } from '$lib/userDraft.svelte' + import { settleDraftAfterWrite } from '$lib/userDraft.svelte' import { useTriggerDraftSync } from '../useTriggerDraftSync.svelte' import LocalDraftBanner from '$lib/components/LocalDraftBanner.svelte' import TextInput from '$lib/components/text_input/TextInput.svelte' @@ -618,21 +617,21 @@ async function scheduleScript(): Promise { const previousPath = initialPath const previousWs = wsId - const scheduleCfg = getScheduleCfg() + // Snapshotted, not just built: the object is fresh but its `args`, `retry` and + // `labels` alias live state, so an edit made while the request is in flight + // would read back as part of what was sent. + const scheduleCfg = $state.snapshot(getScheduleCfg()) as Record deploymentLoading = true const isSaved = await saveScheduleFromCfg(scheduleCfg, edit, wsId!) if (isSaved) { - // Drop the local draft only while the form still holds what was saved: it - // stays editable during the request, and an edit made then is a change on - // top of the save, which the reset would silently swallow. - if ( - deepEqual( - normalizeDraftForCompare($state.snapshot(getScheduleCfg())), - normalizeDraftForCompare(scheduleCfg) - ) - ) { - draftSync.discard(previousPath, scheduleCfg) - } + settleDraftAfterWrite( + 'trigger_schedule', + scheduleCfg, + getScheduleCfg(), + previousPath, + scheduleCfg.path, + { workspace: previousWs ?? undefined } + ) onUpdate?.(scheduleCfg.path, previousPath, previousWs) drawer?.closeDrawer() } diff --git a/frontend/src/lib/userDraft.svelte.ts b/frontend/src/lib/userDraft.svelte.ts index 1fc0aafaac..699eb4717c 100644 --- a/frontend/src/lib/userDraft.svelte.ts +++ b/frontend/src/lib/userDraft.svelte.ts @@ -268,6 +268,28 @@ export function draftValuesEqual(a: unknown, b: unknown): boolean { return deepEqual(normalizeDraftForCompare(a), normalizeDraftForCompare(b)) } +/** + * Settle a draft cell after the write of `written` — the value actually sent — + * landed. A form stays editable while its request is in flight, so `live` may + * hold a newer edit; that is a change on top of the save rather than part of it, + * and resetting the cell would swallow it silently. A save that also moved the + * item is the exception: the edit cannot follow (a freshly acquired cell reads + * only its own default), so the cell is reset rather than left orphaned under a + * path the item no longer occupies. + */ +export function settleDraftAfterWrite( + itemKind: UserDraftItemKind, + written: V, + live: V | undefined, + fromPath: string, + savedPath: string, + opts?: UserDraftOptions +): void { + if (savedPath !== fromPath || draftValuesEqual(live, written)) { + UserDraft.discard(itemKind, fromPath, written, opts) + } +} + export type UserDraftHandle = { get draft(): V | undefined set draft(value: V | undefined) diff --git a/frontend/src/lib/userDraftSettleAfterWrite.test.ts b/frontend/src/lib/userDraftSettleAfterWrite.test.ts new file mode 100644 index 0000000000..039629f8da --- /dev/null +++ b/frontend/src/lib/userDraftSettleAfterWrite.test.ts @@ -0,0 +1,78 @@ +import { describe, it, expect, beforeEach, vi } from 'vitest' + +const discard = vi.fn() +vi.mock('./gen', () => ({ DraftService: { updateDraft: vi.fn() } })) +vi.mock('./gen/core/OpenAPI', () => ({ OpenAPI: { BASE: '' } })) +vi.mock('./localDraftHints.svelte', () => ({ setLocalDraftHint: vi.fn() })) + +import { settleDraftAfterWrite, UserDraft } from './userDraft.svelte' + +beforeEach(() => { + discard.mockClear() + vi.spyOn(UserDraft, 'discard').mockImplementation(discard as any) +}) + +const OPTS = { workspace: 'ws' } +const sent = { path: 'u/me/a', value: 'sent' } + +/** + * The forms stay editable while their save is in flight, so the cell can hold a + * newer edit by the time the write returns. Resetting it unconditionally — what + * every editor did — swallows that edit with no trace. + */ +describe('settleDraftAfterWrite', () => { + it('resets the cell when it still holds what was written', () => { + settleDraftAfterWrite('variable', sent, { ...sent }, 'u/me/a', 'u/me/a', OPTS) + expect(discard).toHaveBeenCalledWith('variable', 'u/me/a', sent, OPTS) + }) + + it('keeps an edit made while the write was in flight', () => { + settleDraftAfterWrite( + 'variable', + sent, + { path: 'u/me/a', value: 'typed' }, + 'u/me/a', + 'u/me/a', + OPTS + ) + expect(discard).not.toHaveBeenCalled() + }) + + // A renamed save is the exception: a freshly acquired cell reads only its own + // default, so the edit cannot follow the item and the alternative to resetting + // is an orphan draft under a path the item no longer occupies. + it('resets even a diverged cell when the write moved the item', () => { + settleDraftAfterWrite( + 'variable', + sent, + { path: 'u/me/a', value: 'typed' }, + 'u/me/a', + 'u/me/b', + OPTS + ) + expect(discard).toHaveBeenCalledWith('variable', 'u/me/a', sent, OPTS) + }) + + it('compares through the draft normalization, so a nested edit is not missed', () => { + const written = { path: 'u/me/a', args: { host: 'h' } } + settleDraftAfterWrite( + 'resource', + written, + { path: 'u/me/a', args: { host: 'h' } }, + 'u/me/a', + 'u/me/a', + OPTS + ) + expect(discard).toHaveBeenCalledTimes(1) + discard.mockClear() + settleDraftAfterWrite( + 'resource', + written, + { path: 'u/me/a', args: { host: 'edited' } }, + 'u/me/a', + 'u/me/a', + OPTS + ) + expect(discard).not.toHaveBeenCalled() + }) +})