From 67c1faed532c05dea2505f144fd88dd035bd3678 Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Sun, 6 Sep 2026 04:07:20 +0200 Subject: [PATCH] fix(editors): read what a save sends before it is sent, and settle its draft in one place MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The resource save read its snapshot back out of the live form after the request returned, so the "did anything change meanwhile" test compared the edit against itself and always said no — the mid-request edit became the deployed baseline instead. Its payloads, its submitted path and the schedule's config are now all pinned before the first await, which also makes the schedule's test see edits to `args`, `retry` and `labels` rather than only its scalar fields. The three copies of the settling rule become `settleDraftAfterWrite`. A save that moved the item resets the cell rather than keeping a diverged one: the edit cannot follow — a freshly acquired cell reads only its own default — and the alternative is a draft stranded under a path the item no longer occupies. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01KcyBRxE8j3ujCYkum9KrC4 --- .../src/lib/components/ResourceEditor.svelte | 42 ++++++---- .../components/ResourceEditorDrawer.svelte | 6 +- .../src/lib/components/VariableEditor.svelte | 23 +++--- .../schedules/ScheduleEditorInner.svelte | 27 ++++--- frontend/src/lib/userDraft.svelte.ts | 22 ++++++ .../src/lib/userDraftSettleAfterWrite.test.ts | 78 +++++++++++++++++++ 6 files changed, 158 insertions(+), 40 deletions(-) create mode 100644 frontend/src/lib/userDraftSettleAfterWrite.test.ts 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() + }) +})