From c3f7f8a45830fb548aa628ebf6e2b6c95c6de67f Mon Sep 17 00:00:00 2001 From: Diego Imbert <70353967+diegoimbert@users.noreply.github.com> Date: Mon, 7 Sep 2026 19:31:59 +0200 Subject: [PATCH] fix: stop an untouched item's form from saving a draft nobody wrote (#10964) * feat: gate drafts on real user input so a moved-on schema is not a draft Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01ScVgqGpuyDMWdzVNPm7Q5f * Revert "feat: gate drafts on real user input so a moved-on schema is not a draft" This reverts commit 6cd86cf727dc46d9836440ae345ca6161c3fb31c. * fix: stop counting empty schema-added fields and server metadata as drafts Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01ScVgqGpuyDMWdzVNPm7Q5f * feat: sweep away existing drafts that carry no changes, once per workspace Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01ScVgqGpuyDMWdzVNPm7Q5f * Reapply "feat: gate drafts on real user input so a moved-on schema is not a draft" This reverts commit b7b18e345e6984544290009247faca1e341b46bd. * Revert "fix: stop counting empty schema-added fields and server metadata as drafts" This reverts commit 9787270ad883722735d3873ee98d37a7fd11ad23. * docs: describe the sweep by the gate that now prevents new phantom drafts Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01ScVgqGpuyDMWdzVNPm7Q5f * fix: make the draft sweep a compare-and-delete so it cannot eat live edits Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01ScVgqGpuyDMWdzVNPm7Q5f * fix: close the gate's load-time window and stop sealing a failed sweep Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01ScVgqGpuyDMWdzVNPm7Q5f * fix: open the gate on the edit itself, and stop the sweep at ownerless drafts Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01ScVgqGpuyDMWdzVNPm7Q5f * fix: count a click as an edit, and keep an unjudged row from sealing the sweep Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01ScVgqGpuyDMWdzVNPm7Q5f * fix: release the sweep's sync baseline when its delete is refused Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01ScVgqGpuyDMWdzVNPm7Q5f * fix: drop the refused delete before re-baselining, and bound the sweep's retries Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01ScVgqGpuyDMWdzVNPm7Q5f * refactor: send the sweep's delete straight to the API, not through the syncer Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01ScVgqGpuyDMWdzVNPm7Q5f * fix: never absorb a change the resource type's schema could not have made Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01ScVgqGpuyDMWdzVNPm7Q5f * fix: push an edit the gate only notices after the write has landed Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01ScVgqGpuyDMWdzVNPm7Q5f * fix: stop the gating effect re-suspending a resource opened on a draft Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01ScVgqGpuyDMWdzVNPm7Q5f * chore: update ee-repo-ref to d33ea730c550cdbc7d050aeb6d40dcef3d134e07 This commit updates the EE repository reference after PR #782 was merged in windmill-ee-private. Previous ee-repo-ref: 313c572c9dcbcaafd8a1594df4054f9dd26f395c New ee-repo-ref: d33ea730c550cdbc7d050aeb6d40dcef3d134e07 Automated by sync-ee-ref workflow. --------- Co-authored-by: Claude Opus 5 (1M context) Co-authored-by: windmill-internal-app[bot] --- .../src/lib/components/ResourceEditor.svelte | 108 +++++++- .../triggers/useTriggerDraftSync.svelte.ts | 106 ++++++-- frontend/src/lib/userDraft.svelte.ts | 5 +- frontend/src/lib/userDraftEditGate.ts | 66 +++++ frontend/src/lib/userDraftPrune.test.ts | 243 ++++++++++++++++++ frontend/src/lib/userDraftPrune.ts | 230 +++++++++++++++++ frontend/src/lib/utils_draft_deploy.ts | 13 + .../src/routes/(root)/(logged)/+layout.svelte | 11 +- 8 files changed, 759 insertions(+), 23 deletions(-) create mode 100644 frontend/src/lib/userDraftEditGate.ts create mode 100644 frontend/src/lib/userDraftPrune.test.ts create mode 100644 frontend/src/lib/userDraftPrune.ts diff --git a/frontend/src/lib/components/ResourceEditor.svelte b/frontend/src/lib/components/ResourceEditor.svelte index 89acac79de..3c9eda9467 100644 --- a/frontend/src/lib/components/ResourceEditor.svelte +++ b/frontend/src/lib/components/ResourceEditor.svelte @@ -2,7 +2,7 @@ import type { Schema } from '$lib/common' import { ResourceService, WorkspaceService, type Resource, type ResourceType } from '$lib/gen' import { canWrite } from '$lib/utils' - import { createEventDispatcher, untrack } from 'svelte' + import { createEventDispatcher, onDestroy, untrack } from 'svelte' import { userStore, workspaceStore } from '$lib/stores' import { sendUserToast } from '$lib/toast' import { clearJsonSchemaResourceCache } from './schema/jsonSchemaResource.svelte' @@ -13,7 +13,9 @@ import { getUserExt } from '$lib/user' import type { UserExt } from '$lib/stores' import { UserDraft, draftValuesEqual, type UserDraftHandle } from '$lib/userDraft.svelte' + import { UserDraftDbSyncer } from '$lib/userDraftDbSyncer.svelte' import { setLocalDraftHint } from '$lib/localDraftHints.svelte' + import { onUserInput } from '$lib/userDraftEditGate' interface Props { canSave?: boolean @@ -108,6 +110,55 @@ workspaceSpecs.push({ ws, defaultValue }) } + // Gated per workspace until the user puts something into that workspace's + // form (see `onUserInput`): the autosave stays suspended and the deployed + // baseline absorbs whatever the form settles on. A workspace opened ON a + // saved draft keeps its baseline — that divergence is the user's own. + let userEdited: Record = $state({}) + let openedOnDraft: Record = $state({}) + const suspendedWorkspaces = new Set() + + function setGated(ws: string, gated: boolean): void { + if (!initialPath) return + if (gated === suspendedWorkspaces.has(ws)) return + if (gated) { + UserDraft.stopSync('resource', initialPath, { workspace: ws }) + suspendedWorkspaces.add(ws) + } else { + UserDraft.restartSync('resource', initialPath, { workspace: ws }) + suspendedWorkspaces.delete(ws) + } + } + + // Nothing counts until this workspace's form is on screen, and while the + // schema is still arriving a precursor alone does not: it would open the gate + // just in time for the schema's materialized values to POST. `Path`, the + // labels and the description render above that skeleton and stay editable + // throughout, so a real value event still counts and keeps the edit. + onUserInput((kind) => { + if (!selected || !(selected in states)) return + if (kind === 'precursor' && loadingSchema) return + userEdited[selected] = true + }) + + $effect(() => { + const wss = Object.keys(states) + const edited = { ...userEdited } + const onDraft = { ...openedOnDraft } + untrack(() => { + // A workspace opened on a saved draft is never suspended — there is no + // phantom to prevent, and a write made while suspended is dropped for + // good. Without `onDraft` here this effect re-suspends it the moment its + // handle appears, undoing the decision made when it was opened. + for (const ws of wss) setGated(ws, !edited[ws] && !onDraft[ws]) + }) + }) + + // `stopSync` must be paired or the key stays unsynced for the session. + onDestroy(() => { + for (const ws of [...suspendedWorkspaces]) setGated(ws, false) + }) + let isValid = $state(true) let jsonError = $state('') let perWsValid: Record = $state({}) @@ -245,6 +296,13 @@ } // Open with the saved draft if present, else the deployed. const s: ResourceState = savedDraftState ?? deployedState + openedOnDraft[ws] = !!savedDraftState + // Gate BEFORE the handle is acquired: `stopSync` queues on a + // not-yet-live entry, and the form can settle before the effect + // above gets a chance to run. Only worth doing when no draft exists + // yet — where one does, there is no phantom to prevent and + // suspending could only drop a write. + if (!savedDraftState) setGated(ws, true) ensureHandle(ws, s) initialStates[ws] = structuredClone(deployedState) // Draft-only paths (`no_deployed`) have no row — saving must @@ -259,6 +317,47 @@ }) }) + /** The schema can only ever write `args`. `path`, `labels`, `description` and + * `wsSpecific` are beyond its reach, so a difference in one of those is the + * user's — whatever event did or didn't reach the gate. Removing a label runs + * a click handler and emits nothing native, and would otherwise be absorbed. */ + function differsOutsideArgs(a: ResourceState, b: ResourceState | undefined): boolean { + return !!b && !draftValuesEqual({ ...a, args: null }, { ...b, args: null }) + } + + // Absorb the form's settling writes into the deployed baseline while the + // selected workspace is gated, so they show up neither as the "unsaved + // changes" banner nor, once `discardIf` reads the baseline, as a draft. + // Only the selected workspace has a form rendered against it. + $effect(() => { + const ws = selected + if (!ws || !initialPath) return + if (userEdited[ws] || openedOnDraft[ws]) return + // `$state.snapshot` deep-reads, so nested `args` mutations re-run this. + const settled = states[ws]?.draft + ? ($state.snapshot(states[ws].draft) as ResourceState) + : undefined + untrack(() => { + if (!settled) return + if (differsOutsideArgs(settled, initialStates[ws])) { + // An edit, not settling. This runs AFTER the write landed, and a write + // made while suspended is swallowed for good (the mirror advances its + // baseline either way), so un-suspend and push the value here rather + // than leaving it to whichever effect happens to run next. + userEdited[ws] = true + setGated(ws, false) + void UserDraftDbSyncer.save({ + workspace: ws, + itemKind: 'resource', + path: initialPath, + value: settled + }) + return + } + if (!draftValuesEqual(settled, initialStates[ws])) initialStates[ws] = settled + }) + }) + // Keep current.path bound to the outer `path` prop for consumers $effect(() => { if (current) path = current.path @@ -292,6 +391,13 @@ } export function discardLocalDraft(): void { if (!selected) return + // Back to the deployed value with nothing of the user's left in it, so + // the gate closes again — otherwise the form settles on the schema's + // values a second time and the discarded draft comes straight back. + // `discard` POSTs the delete itself, so suspending first is safe. + openedOnDraft[selected] = false + userEdited[selected] = false + setGated(selected, true) UserDraft.discard('resource', initialPath ?? '', initialStates[selected], { workspace: selected }) diff --git a/frontend/src/lib/components/triggers/useTriggerDraftSync.svelte.ts b/frontend/src/lib/components/triggers/useTriggerDraftSync.svelte.ts index ba09167d6d..98d38ac25f 100644 --- a/frontend/src/lib/components/triggers/useTriggerDraftSync.svelte.ts +++ b/frontend/src/lib/components/triggers/useTriggerDraftSync.svelte.ts @@ -2,9 +2,21 @@ import { untrack } from 'svelte' import { deepEqual } from 'fast-equals' import { UserDraft, normalizeDraftForCompare, type UserDraftItemKind } from '$lib/userDraft.svelte' import { setLocalDraftHint } from '$lib/localDraftHints.svelte' +import { onUserInput } from '$lib/userDraftEditGate' type Cfg = Record +/** + * Detach a config from whatever holds it. The draft cell is deeply reactive, + * so handing its object straight to `applyCfg` would make the form's own + * `$state` (a schedule's `args`, say) the very object the cell holds — every + * later keystroke would then mutate the draft in place behind the autosave's + * back. + */ +function snapshotCfg(cfg: V): V { + return structuredClone($state.snapshot(cfg)) as V +} + /** * Whether `a` differs from `b` after `normalizeDraftForCompare` (JSON * round-trip to drop `undefined`-valued keys, plus ignored deploy-directive @@ -84,8 +96,10 @@ export interface TriggerDraftSync { * …)` (another tab, a programmatic write) propagate into the open editor. * * - **apply-effect**: reflects external `handle.draft` changes into the form. + * - **absorb-effect**: folds the form's own settling into the baseline until + * the user's first input, so a schema that moved on is not a draft. * - **persist-effect**: writes form edits back through the handle, dropping - * the draft when the form is back at the deployed baseline. + * the draft when the form is back at the baseline. * * Both effect bodies are `untrack`ed and gated by `cfgDiffers` * idempotence so they can't feed back into each other. Must be called once @@ -99,14 +113,56 @@ export function useTriggerDraftSync(opts: TriggerDraftSyncOptions): TriggerDraft }) const handle = $derived(handles[0]) + // Gated until the user puts something in (see `onUserInput`): the baseline + // absorbs whatever the form settles on and nothing persists, so an untouched + // trigger never reports unsaved changes. A drawer opened ON a restored draft + // absorbs nothing — that divergence is the user's own. + let settledBaseline: Cfg | undefined = $state(undefined) + let userEdited = $state(false) + let openedOnDraft = $state(false) + // These editors are mounted by the list page, not by the drawer, so input + // arriving while the drawer is still loading is the click that opened it + // (or anything else on the page behind) — never an edit to this form. + onUserInput(() => { + if (!opts.drawerLoading()) userEdited = true + }) + + /** The deployed config, plus whatever the form settled on by itself. */ + const baseline = $derived(settledBaseline ?? opts.deployed()) + + $effect(() => { + // A reload re-opens the gate's window: the drawer is being pointed at a + // different trigger, or the same one re-read from the backend. The click + // that opened it landed before this, hence the reset of `userEdited`. + if (!opts.drawerLoading()) return + untrack(() => { + settledBaseline = undefined + userEdited = false + openedOnDraft = false + }) + }) + + // absorb-effect: pre-edit form drift joins the baseline. + $effect(() => { + if (opts.drawerLoading() || userEdited || openedOnDraft) return + const cfg = opts.getCfg() + const deployed = opts.deployed() + if (cfg == null || deployed == null) return + // Snapshot before untracking: `getCfg` hands back the form's `$state` + // objects by reference, so only a deep read subscribes to the nested + // writes the form makes as it settles. + const settled = snapshotCfg(cfg) + untrack(() => { + if (cfgDiffers(settled, settledBaseline ?? deployed)) settledBaseline = settled + }) + }) + // Live "is there a local draft?" — the form diverges from the deployed // baseline. Gated on `!drawerLoading` (the baseline isn't settled yet // mid-load) and on a non-null baseline (a brand-new trigger has none, so // "unsaved changes" / discard-to-deployed is meaningless there). const hasDraft = $derived( - !opts.drawerLoading() && - opts.deployed() != null && - cfgDiffers(opts.getCfg() as Cfg, opts.deployed() as Cfg) + !opts.drawerLoading() && baseline != null && cfgDiffers(opts.getCfg() as Cfg, baseline as Cfg) ) // Reactive "banner is possible" — depends on `drawerLoading()` so it @@ -131,7 +187,7 @@ export function useTriggerDraftSync(opts: TriggerDraftSyncOptions): TriggerDraft if (opts.drawerLoading() || d == null) return untrack(() => { if (cfgDiffers(d, opts.getCfg() as Cfg)) { - void opts.applyCfg(d) + void opts.applyCfg(snapshotCfg(d)) } }) }) @@ -148,30 +204,32 @@ export function useTriggerDraftSync(opts: TriggerDraftSyncOptions): TriggerDraft discardTimer = undefined if (opts.drawerLoading()) return const cfg = opts.getCfg() - const deployed = opts.deployed() const h = handle if (!h || cfg == null) return - if (!cfgDiffers(cfg, deployed) && cfgDiffers(h.draft, deployed)) { - discard(opts.path(), deployed, true) + if (!cfgDiffers(cfg, baseline) && cfgDiffers(h.draft, baseline)) { + discard(opts.path(), baseline, true) } }, 600) } // persist-effect: form edits → handle; drop the draft when back at the - // deployed baseline. + // baseline. $effect(() => { if (opts.drawerLoading() || !opts.path()) return + // Nothing persists before the user's first input — the form's own + // settling is not an edit, and gating here rather than relying on the + // absorb-effect having run first keeps the two effects order-independent. + if (!userEdited && !openedOnDraft) return const cfg = opts.getCfg() if (cfg == null) return untrack(() => { const h = handle if (!h) return - const deployed = opts.deployed() - if (cfgDiffers(cfg, deployed)) { + if (cfgDiffers(cfg, baseline)) { if (cfgDiffers(cfg, h.draft)) h.draft = cfg - } else if (cfgDiffers(h.draft, deployed)) { - // Only when a draft actually exists to drop: `h.draft` equals - // `deployed` right after a discard or the post-load seed. + } else if (cfgDiffers(h.draft, baseline)) { + // Only when a draft actually exists to drop: `h.draft` equals the + // baseline right after a discard or the post-load seed. scheduleAutoDiscard() } }) @@ -202,7 +260,7 @@ export function useTriggerDraftSync(opts: TriggerDraftSyncOptions): TriggerDraft return hasBaseline }, get deployed() { - return opts.deployed() + return baseline }, get current() { return opts.getCfg() @@ -211,8 +269,14 @@ export function useTriggerDraftSync(opts: TriggerDraftSyncOptions): TriggerDraft const d = handle?.draft if (cfgDiffers(d, opts.getCfg() as Cfg)) { // Overlay the local autosave on the just-loaded backend config. - await opts.applyCfg(d) + await opts.applyCfg(snapshotCfg(d)) } + // The form is not rendered while the drawer loads, so anything that + // diverges from the deployed config right now is a draft restored onto + // it — by the overlay above, or by the editor from the backend before + // calling this — never the form settling. Absorbing that into the + // baseline would hide the user's own work behind a clean drawer. + openedOnDraft = cfgDiffers(opts.getCfg() as Cfg, opts.deployed()) // Adopt the post-load form state as the cell's baseline without // POSTing, consuming the entry's one-shot first-write seed guard. // Trigger drawers never write the cell programmatically on open, so @@ -221,14 +285,20 @@ export function useTriggerDraftSync(opts: TriggerDraftSyncOptions): TriggerDraft const p = opts.path() const cfg = opts.getCfg() if (ws && p && cfg != null) { - UserDraft.seed(opts.itemKind, p, structuredClone($state.snapshot(cfg)) as Cfg, { + UserDraft.seed(opts.itemKind, p, snapshotCfg(cfg) as Cfg, { workspace: ws }) } }, async resetToDeployed(path: string) { - const deployedCfg = structuredClone($state.snapshot(opts.deployed())) as Cfg + const deployedCfg = snapshotCfg(opts.deployed()) as Cfg discard(path, deployedCfg) + // Nothing of the user's is left in the form, so the gate closes again + // — otherwise the form settles on the schema's values a second time + // and the discarded draft comes straight back. + settledBaseline = undefined + userEdited = false + openedOnDraft = false await opts.applyCfg(deployedCfg) }, discard diff --git a/frontend/src/lib/userDraft.svelte.ts b/frontend/src/lib/userDraft.svelte.ts index 7942694f31..57e911dd3c 100644 --- a/frontend/src/lib/userDraft.svelte.ts +++ b/frontend/src/lib/userDraft.svelte.ts @@ -219,7 +219,10 @@ const DRAFT_COMPARE_IGNORED_FIELDS = [ 'version_id', 'parent_version', 'is_draft', - 'assets' + 'assets', + // Fixed at creation and absent from the resource editor's draft shape, so + // it only ever shows up on the deployed side of a comparison. + 'resource_type' ] as const /** diff --git a/frontend/src/lib/userDraftEditGate.ts b/frontend/src/lib/userDraftEditGate.ts new file mode 100644 index 0000000000..e3057a73ee --- /dev/null +++ b/frontend/src/lib/userDraftEditGate.ts @@ -0,0 +1,66 @@ +import { onDestroy } from 'svelte' + +/** + * What kind of event opened the gate. + * + * - `value`: the user changed something — the event *is* the edit. + * - `precursor`: a gesture that usually precedes an edit. Needed because a + * custom component (a picker, a toggle built out of divs) writes its value + * through Svelte state and fires no native value event at all, so waiting for + * one would drop those edits. The cost is that a bare click counts too. + */ +export type UserInputKind = 'value' | 'precursor' + +/** Events that ARE an edit. `drop` and `paste` matter on their own: text + * dragged in from another application produces no pointer or key event in this + * document at all. */ +const VALUE_EVENTS = ['input', 'change', 'drop', 'paste'] as const +/** `click` is here for the controls that mutate state from a click handler and + * fire no native value event — ArgInput's "Add item", say. A mouse always sends + * `pointerdown` first, but an assistive technology can activate one with a + * trusted `click` alone, and that is a real edit with nothing else to catch it. */ +const PRECURSOR_EVENTS = ['pointerdown', 'keydown', 'click'] as const + +/** + * A draft is supposed to record what the USER changed, but an editor built + * from a schema writes into the value on its own: the form materializes a + * property the stored item never carried (an empty string, `false`, the first + * option of a required enum, a schema `default`) and deletes one a `showExpr` + * hides. So merely opening an item whose schema has moved on makes it diverge + * from the deployed value with nobody having touched it — a draft nobody asked + * for, cluttering the workspace. + * + * An editor guards against that by gating its draft on this: nothing the form + * settles on counts until the user has actually put something in. Callers + * decide what a gate covers (the resource editor keys it by workspace, since + * switching workspaces re-renders the form against a fresh value) and what + * gating means for them — suspending the autosave, absorbing the settled value + * into the deployed baseline, or both. + * + * Capture phase puts this ahead of the handler that writes the value, so a gate + * opened here is already open by the time the edit lands. Listening on the + * document rather than the editor's own subtree is deliberate: pickers and + * modals render in portals outside it, and missing a real edit would silently + * drop the user's work, while opening the gate too eagerly only costs the + * phantom draft that existed before. + * + * Registers for the lifetime of the calling component — call it during init. + */ +export function onUserInput(handle: (kind: UserInputKind) => void): void { + if (typeof document === 'undefined') return + const listeners: Array<[string, (e: Event) => void]> = [] + const register = (type: string, kind: UserInputKind) => { + const onEvent = (e: Event) => { + // A programmatic `dispatchEvent` is untrusted, which is what keeps the + // form's own settling from opening the gate it is gated by. + if (e.isTrusted) handle(kind) + } + document.addEventListener(type, onEvent, true) + listeners.push([type, onEvent]) + } + for (const type of VALUE_EVENTS) register(type, 'value') + for (const type of PRECURSOR_EVENTS) register(type, 'precursor') + onDestroy(() => { + for (const [type, onEvent] of listeners) document.removeEventListener(type, onEvent, true) + }) +} diff --git a/frontend/src/lib/userDraftPrune.test.ts b/frontend/src/lib/userDraftPrune.test.ts new file mode 100644 index 0000000000..0276598d45 --- /dev/null +++ b/frontend/src/lib/userDraftPrune.test.ts @@ -0,0 +1,243 @@ +import { describe, it, expect, beforeEach, vi } from 'vitest' + +// The sweep DELETES drafts, so the guard that matters is which rows it picks. +// Stub the two collaborators it decides from — the draft listing and the +// per-kind diff — and assert on what it discards. +const listDrafts = vi.fn() +const getDraftDiffValues = vi.fn() +const updateDraft = vi.fn(async () => ({ status: 'saved', current_timestamp: 'x' })) + +vi.mock('./gen', () => ({ + DraftService: { + listDrafts: (...a: unknown[]) => listDrafts(...(a as [])), + updateDraft: (...a: unknown[]) => updateDraft(...(a as [])) + } +})) +// Only `getDraftDiffValues` is stubbed; `canDiffDraftKind` is the real one, so +// the kind filter is pinned against the actual overlay table. +vi.mock('./utils_draft_deploy', async (orig) => ({ + ...(await orig>()), + getDraftDiffValues: (...a: unknown[]) => getDraftDiffValues(...(a as [])) +})) +vi.mock('./localDraftHints.svelte', () => ({ setLocalDraftHint: vi.fn() })) +vi.mock('./workspaceDrafts.svelte', () => ({ invalidateWorkspaceDrafts: vi.fn() })) +const sendUserToast = vi.fn() +vi.mock('./toast', () => ({ sendUserToast: (...a: unknown[]) => sendUserToast(...(a as [])) })) + +// The sweep reads exactly one thing from the syncer — whether this tab is +// mid-write on the key — and writes nothing back to it. +let syncState = 'none' +vi.mock('./userDraftDbSyncer.svelte', () => ({ + UserDraftDbSyncer: { + getState: () => ({ + get state() { + return syncState + } + }) + } +})) + +let liveDraft = false +vi.mock('./userDraft.svelte', async (orig) => ({ + ...(await orig>()), + UserDraft: { has: () => liveDraft } +})) + +import { pruneMeaninglessDrafts } from './userDraftPrune' + +const row = (over: Record = {}) => ({ + kind: 'resource', + path: 'u/me/r', + draft_only: false, + legacy_draft: false, + mine: true, + can_write: true, + created_at: '2026-01-01T00:00:00Z', + ...over +}) +const diff = (over: Record = {}) => ({ + deployed: { value: { host: 'h' } }, + draft: { value: { host: 'h' } }, + hasDraft: true, + noDeployed: false, + ...over +}) +const discardedPaths = () => updateDraft.mock.calls.map((c: any[]) => c[0].path as string) + +beforeEach(() => { + localStorage.clear() + vi.clearAllMocks() + updateDraft.mockResolvedValue({ status: 'saved', current_timestamp: 'x' }) + syncState = 'none' + liveDraft = false +}) + +describe('pruneMeaninglessDrafts', () => { + it('discards a draft whose diff against the deployed value is empty', async () => { + listDrafts.mockResolvedValue([row()]) + getDraftDiffValues.mockResolvedValue(diff()) + await pruneMeaninglessDrafts('main', 'me@x.dev') + expect(discardedPaths()).toEqual(['u/me/r']) + }) + + it('keeps a draft that carries a real change', async () => { + listDrafts.mockResolvedValue([row()]) + getDraftDiffValues.mockResolvedValue(diff({ draft: { value: { host: 'other' } } })) + await pruneMeaninglessDrafts('main', 'me@x.dev') + expect(updateDraft).not.toHaveBeenCalled() + }) + + it('never touches a draft-only item — the draft is the whole item', async () => { + listDrafts.mockResolvedValue([row({ draft_only: true })]) + getDraftDiffValues.mockResolvedValue(diff()) + await pruneMeaninglessDrafts('main', 'me@x.dev') + expect(updateDraft).not.toHaveBeenCalled() + }) + + it('never touches another user’s row, or one it cannot write', async () => { + listDrafts.mockResolvedValue([row({ mine: false }), row({ path: 'u/me/b', can_write: false })]) + getDraftDiffValues.mockResolvedValue(diff()) + await pruneMeaninglessDrafts('main', 'me@x.dev') + expect(updateDraft).not.toHaveBeenCalled() + }) + + it('leaves a draft alone when its diff cannot be fetched, and retries it later', async () => { + listDrafts.mockResolvedValue([row()]) + getDraftDiffValues.mockRejectedValue(new Error('boom')) + await pruneMeaninglessDrafts('main', 'me@x.dev') + expect(updateDraft).not.toHaveBeenCalled() + // A row that could not be judged is not a row that carries changes, so + // the pass must stay open rather than strand it. + getDraftDiffValues.mockResolvedValue(diff()) + await pruneMeaninglessDrafts('main', 'me@x.dev') + expect(discardedPaths()).toEqual(['u/me/r']) + }) + + it('conditions the delete on the timestamp it judged, so a row that moved is spared', async () => { + listDrafts.mockResolvedValue([row()]) + getDraftDiffValues.mockResolvedValue(diff()) + updateDraft.mockResolvedValue({ status: 'conflict', current_timestamp: 'newer' }) + await pruneMeaninglessDrafts('main', 'me@x.dev') + expect(updateDraft).toHaveBeenCalledWith({ + workspace: 'main', + kind: 'resource', + path: 'u/me/r', + requestBody: { value: null, last_sync: '2026-01-01T00:00:00Z', force: false } + }) + // Refused, so nothing is reported as cleared — and the sweep leaves no + // state behind for the editor's own autosave to trip over. + expect(sendUserToast).not.toHaveBeenCalled() + }) + + it('does not keep retrying a row the server will never judge', async () => { + listDrafts.mockResolvedValue([row()]) + getDraftDiffValues.mockRejectedValue({ status: 404 }) + await pruneMeaninglessDrafts('main', 'me@x.dev') + expect(updateDraft).not.toHaveBeenCalled() + // Sealed: a 4xx is final, unlike the transient case above. + listDrafts.mockResolvedValue([row()]) + getDraftDiffValues.mockResolvedValue(diff()) + await pruneMeaninglessDrafts('main', 'me@x.dev') + expect(updateDraft).not.toHaveBeenCalled() + }) + + it('gives up after a bounded number of unresolved passes', async () => { + listDrafts.mockResolvedValue([row()]) + getDraftDiffValues.mockRejectedValue(new Error('network')) + for (let i = 0; i < 3; i++) await pruneMeaninglessDrafts('main', 'me@x.dev') + expect(listDrafts).toHaveBeenCalledTimes(3) + // Sealed on the third: an unresolvable row cannot re-list forever. + await pruneMeaninglessDrafts('main', 'me@x.dev') + expect(listDrafts).toHaveBeenCalledTimes(3) + }) + + it('skips a kind no diff can be computed for, and still seals', async () => { + listDrafts.mockResolvedValue([row({ kind: 'trigger_webhook', path: 'u/me/hook' })]) + await pruneMeaninglessDrafts('main', 'me@x.dev') + expect(getDraftDiffValues).not.toHaveBeenCalled() + // Unjudgeable is permanent, not transient: leaving the pass open would + // re-run the sweep on every page load forever. + listDrafts.mockResolvedValue([row()]) + getDraftDiffValues.mockResolvedValue(diff()) + await pruneMeaninglessDrafts('main', 'me@x.dev') + expect(updateDraft).not.toHaveBeenCalled() + }) + + it('leaves alone a draft this tab is editing', async () => { + liveDraft = true + listDrafts.mockResolvedValue([row()]) + getDraftDiffValues.mockResolvedValue(diff()) + await pruneMeaninglessDrafts('main', 'me@x.dev') + expect(updateDraft).not.toHaveBeenCalled() + }) + + it('leaves alone a draft with a write queued or in flight', async () => { + syncState = 'pending' + listDrafts.mockResolvedValue([row()]) + getDraftDiffValues.mockResolvedValue(diff()) + await pruneMeaninglessDrafts('main', 'me@x.dev') + expect(updateDraft).not.toHaveBeenCalled() + }) + + it('does not count, or seal the pass on, a delete that failed to send', async () => { + listDrafts.mockResolvedValue([row()]) + getDraftDiffValues.mockResolvedValue(diff()) + updateDraft.mockRejectedValueOnce(new Error('network')) + await pruneMeaninglessDrafts('main', 'me@x.dev') + expect(sendUserToast).not.toHaveBeenCalled() + // The pass stayed open, so the draft left behind is retried. + await pruneMeaninglessDrafts('main', 'me@x.dev') + expect(discardedPaths()).toEqual(['u/me/r', 'u/me/r']) + }) + + it('never touches a legacy workspace-level row', async () => { + listDrafts.mockResolvedValue([row({ legacy_draft: true })]) + getDraftDiffValues.mockResolvedValue(diff()) + await pruneMeaninglessDrafts('main', 'me@x.dev') + expect(updateDraft).not.toHaveBeenCalled() + }) + + it('only sweeps the kinds whose editors are gated', async () => { + listDrafts.mockResolvedValue([ + row({ kind: 'script', path: 'u/me/s' }), + row({ kind: 'flow', path: 'u/me/f' }), + row({ kind: 'app', path: 'u/me/a' }), + row({ kind: 'trigger_schedule', path: 'u/me/sched' }), + row() + ]) + getDraftDiffValues.mockResolvedValue(diff()) + await pruneMeaninglessDrafts('main', 'me@x.dev') + expect(discardedPaths().sort()).toEqual(['u/me/r', 'u/me/sched']) + // The expensive payload fetches are never made for the ungated kinds. + expect(getDraftDiffValues).toHaveBeenCalledTimes(2) + }) + + it('leaves the pass open when a row was skipped as busy', async () => { + liveDraft = true + listDrafts.mockResolvedValue([row()]) + getDraftDiffValues.mockResolvedValue(diff()) + await pruneMeaninglessDrafts('main', 'me@x.dev') + liveDraft = false + await pruneMeaninglessDrafts('main', 'me@x.dev') + expect(discardedPaths()).toEqual(['u/me/r']) + }) + + it('runs once per workspace and user', async () => { + listDrafts.mockResolvedValue([row()]) + getDraftDiffValues.mockResolvedValue(diff()) + await pruneMeaninglessDrafts('main', 'me@x.dev') + await pruneMeaninglessDrafts('main', 'me@x.dev') + expect(updateDraft).toHaveBeenCalledTimes(1) + await pruneMeaninglessDrafts('other', 'me@x.dev') + expect(updateDraft).toHaveBeenCalledTimes(2) + }) + + it('retries next mount when the listing failed', async () => { + listDrafts.mockRejectedValueOnce(new Error('offline')) + await pruneMeaninglessDrafts('main', 'me@x.dev') + listDrafts.mockResolvedValue([row()]) + getDraftDiffValues.mockResolvedValue(diff()) + await pruneMeaninglessDrafts('main', 'me@x.dev') + expect(discardedPaths()).toEqual(['u/me/r']) + }) +}) diff --git a/frontend/src/lib/userDraftPrune.ts b/frontend/src/lib/userDraftPrune.ts new file mode 100644 index 0000000000..465048d09d --- /dev/null +++ b/frontend/src/lib/userDraftPrune.ts @@ -0,0 +1,230 @@ +/** + * One-off sweep that drops drafts carrying no changes. + * + * `onUserInput` stops new ones from being written; the ones already stored need + * this pass to clear. Runs once per (workspace, user) per browser, after the + * localStorage→DB migration so anything it just uploaded is swept too. + * + * Scoped to the kinds whose editors that gate covers. A script, flow or app + * draft only ever came from an explicit edit, so there is no phantom to clear + * there — and `getDraftDiffValues` would fetch each one's full deployed payload + * at login to prove it. + * + * A draft is dropped only when the diff the user would be shown is empty: both + * sides come from `getDraftDiffValues`, the same canonicalization the diff + * drawer renders, compared with the same `draftValuesEqual` the editors use. + * Anything that can't be established is left alone — a `draft_only` item (no + * deployed counterpart, so discarding would destroy the item itself), a kind + * with no diff support, a failed fetch, and the legacy workspace-level rows, + * which belong to nobody and are admin-gated to migrate. + * + * Deleting is the dangerous half, and the equality behind it is always stale: + * it was read one round trip ago, and every candidate is read before any is + * deleted. So the delete is a compare-and-delete — `last_sync` is the + * timestamp the row was judged on, and the backend drops it only if nothing has + * written it since, whoever wrote it. + * + * It is sent straight to `DraftService`, NOT through `UserDraftDbSyncer`. That + * syncer exists to autosave an editor's own live value, and everything it does + * for that — parking the payload for the `pagehide` flush, debouncing, holding + * a per-tab `last_sync` baseline and conflict state — is a way for a one-shot + * delete to reach back into whatever the user is doing in the same tab. This + * sweep wants exactly one conditional request and no state afterwards. + */ + +import { DraftService } from './gen' +import type { UserDraftItemKind } from './gen' +import { sendUserToast } from './toast' +import { setLocalDraftHint } from './localDraftHints.svelte' +import { UserDraft, draftValuesEqual } from './userDraft.svelte' +import { UserDraftDbSyncer } from './userDraftDbSyncer.svelte' +import { canDiffDraftKind, getDraftDiffValues } from './utils_draft_deploy' +import { invalidateWorkspaceDrafts } from './workspaceDrafts.svelte' + +const SENTINEL_PREFIX = 'userdraft/pruned/v1/' + +/** A pass that leaves anything unresolved runs again next mount, which costs a + * listing plus an overlay GET per row. Bounded so no permanently-unresolvable + * row can make that repeat forever — whatever the reason it can't be judged. */ +const MAX_PASSES = 3 + +/** The editors whose forms are built from a schema, and so the only kinds that + * could have banked a draft nobody wrote — minus the ones no diff can be + * computed for, which would throw and keep the pass unsealed forever. */ +function isSweepableKind(kind: UserDraftItemKind): boolean { + return (kind === 'resource' || kind.startsWith('trigger_')) && canDiffDraftKind(kind) +} + +/** Overlay GETs are one round trip each and a cluttered workspace has dozens; + * a small window keeps the sweep off the critical path of a fresh login. */ +const CONCURRENCY = 4 + +/** Guards against the layout effect firing again before the sentinel lands. */ +const inFlight = new Set() + +type Candidate = { + kind: UserDraftItemKind + path: string + /** The row's `created_at` as listed — the baseline the delete is conditioned on. */ + createdAt: string +} + +const attemptsKey = (sentinel: string) => `${sentinel}:attempts` + +function readAttempts(sentinel: string): number { + try { + const n = Number(localStorage.getItem(attemptsKey(sentinel))) + return Number.isFinite(n) && n > 0 ? n : 0 + } catch { + return 0 + } +} + +/** Is this tab holding or writing this draft right now? */ +function busyLocally(workspace: string, kind: UserDraftItemKind, path: string): boolean { + if (UserDraft.has(kind, path, { workspace })) return true + return UserDraftDbSyncer.getState({ workspace, itemKind: kind, path }).state !== 'none' +} + +/** A 4xx is the server's final answer for this row — the item is gone, or the + * kind's overlay endpoint isn't served by this build (a feature-gated trigger + * on CE). Retrying it on every page load would never succeed. Anything else + * (network, 5xx) is worth another pass. 429 asks for exactly that. */ +function isPermanentlyUnjudgeable(e: unknown): boolean { + const status = (e as { status?: unknown })?.status + return typeof status === 'number' && status >= 400 && status < 500 && status !== 429 +} + +/** `undefined` when the diff could not be fetched and might be next time — + * distinct from `false`, so the caller can leave the pass open rather than + * strand a row it never judged. */ +async function carriesNoChanges( + workspace: string, + { kind, path }: Candidate +): Promise { + try { + const { deployed, draft, hasDraft, noDeployed } = await getDraftDiffValues( + kind, + path, + workspace + ) + // `hasDraft` false means the overlay had no draft row and the item's own + // value stood in for the draft side — there is nothing to discard, and the + // two sides would compare equal by construction. + if (!hasDraft || noDeployed) return false + return draftValuesEqual(draft, deployed) + } catch (e) { + return isPermanentlyUnjudgeable(e) ? false : undefined + } +} + +async function mapWithLimit( + items: T[], + limit: number, + fn: (item: T) => Promise +): Promise { + const out = new Array(items.length) + let next = 0 + await Promise.all( + Array.from({ length: Math.min(limit, items.length) }, async () => { + while (next < items.length) { + const i = next++ + out[i] = await fn(items[i]) + } + }) + ) + return out +} + +export async function pruneMeaninglessDrafts(workspace: string, userKey: string): Promise { + if (typeof localStorage === 'undefined') return + const sentinel = `${SENTINEL_PREFIX}${workspace}/${userKey}` + if (inFlight.has(sentinel)) return + try { + if (localStorage.getItem(sentinel)) return + } catch { + // Storage unavailable (private mode): the sweep can't record that it ran, + // and re-running it on every mount would cost an overlay GET per draft. + return + } + inFlight.add(sentinel) + try { + const rows = await DraftService.listDrafts({ workspace }) + // Anything left unresolved keeps the pass open: a row skipped as busy was + // never judged, and one whose delete failed is still there. Sealing on + // either would strand it. + let unresolved = 0 + const candidates: Candidate[] = rows + // `draft_only` rows ARE the item; `mine` / `can_write` are the same + // gate the discard endpoint enforces, so anything else would 403. + .filter((r) => !r.draft_only && r.mine && r.can_write) + .filter((r) => isSweepableKind(r.kind) && !r.legacy_draft) + .filter((r) => { + if (!busyLocally(workspace, r.kind, r.path)) return true + unresolved++ + return false + }) + .map((r) => ({ + kind: r.kind, + path: r.path, + createdAt: r.created_at + })) + + const empty: Candidate[] = [] + await mapWithLimit(candidates, CONCURRENCY, async (c) => { + const verdict = await carriesNoChanges(workspace, c) + if (verdict === undefined) unresolved++ + else if (verdict) empty.push(c) + }) + + let discarded = 0 + for (const c of empty) { + // Re-check: the reads above took a while, and the user may have opened + // this item in the meantime. + if (busyLocally(workspace, c.kind, c.path)) { + unresolved++ + continue + } + try { + const resp = await DraftService.updateDraft({ + workspace, + kind: c.kind, + path: c.path, + requestBody: { value: null, last_sync: c.createdAt, force: false } + }) + // `conflict` means the row moved past the timestamp we judged it on, + // so it is no longer the empty draft we decided to drop. + if (resp.status === 'saved') { + setLocalDraftHint(workspace, c.kind, c.path, false) + discarded++ + } + } catch { + unresolved++ + } + } + if (discarded > 0) { + invalidateWorkspaceDrafts(workspace) + sendUserToast(`Cleared ${discarded} draft${discarded > 1 ? 's' : ''} that carried no changes`) + } + // Seal once nothing is left hanging, or once we have tried enough times + // that whatever is hanging is not going to resolve. + const attempts = readAttempts(sentinel) + 1 + if (unresolved === 0 || attempts >= MAX_PASSES) { + try { + localStorage.setItem(sentinel, new Date().toISOString()) + localStorage.removeItem(attemptsKey(sentinel)) + } catch { + // Nothing to do — the pass is idempotent, it just runs again. + } + } else { + try { + localStorage.setItem(attemptsKey(sentinel), String(attempts)) + } catch {} + } + } catch { + // Fire-and-forget from the layout: a workspace whose draft list can't be + // read is left exactly as it was. + } finally { + inFlight.delete(sentinel) + } +} diff --git a/frontend/src/lib/utils_draft_deploy.ts b/frontend/src/lib/utils_draft_deploy.ts index f322bb27b9..0d121d0627 100644 --- a/frontend/src/lib/utils_draft_deploy.ts +++ b/frontend/src/lib/utils_draft_deploy.ts @@ -95,6 +95,19 @@ const OVERLAY_GETTERS: Partial< EmailTriggerService.getEmailTrigger({ workspace, path, getDraft: true }) } +/** Whether `getDraftDiffValues` can produce a diff for this kind at all. The + * script/flow/app family is handled inline; every other kind needs an overlay + * getter and throws without one — several trigger kinds have none. */ +export function canDiffDraftKind(kind: DraftKind): boolean { + return ( + kind === 'script' || + kind === 'flow' || + kind === 'app' || + kind === 'raw_app' || + OVERLAY_GETTERS[kind] !== undefined + ) +} + /** Strip the per-user draft-overlay metadata, returning `{deployed, draft}`. */ function splitOverlay(r: any): { deployed: any diff --git a/frontend/src/routes/(root)/(logged)/+layout.svelte b/frontend/src/routes/(root)/(logged)/+layout.svelte index a20c61f2ff..e82d328c76 100644 --- a/frontend/src/routes/(root)/(logged)/+layout.svelte +++ b/frontend/src/routes/(root)/(logged)/+layout.svelte @@ -74,6 +74,7 @@ import { createUsageResources, registerUsageResources } from '$lib/usage.svelte' import { purgeLegacyUserDrafts } from '$lib/userDraftLegacyMigration' import { migrateUserDraftsToDb } from '$lib/userDraftDbMigration' + import { pruneMeaninglessDrafts } from '$lib/userDraftPrune' import DraftMigrationErrorModal from '$lib/components/DraftMigrationErrorModal.svelte' import { onDestroy, setContext, untrack } from 'svelte' import { base } from '$app/paths' @@ -792,12 +793,16 @@ // drafts). `migrateUserDraftsToDb` then pushes the workspace-scoped // `userdraft/w/{ws}/{kind}/{path}` keys — written by the editor with the // correct workspace — onto the server-side draft table, clearing LS on - // success. + // success. `pruneMeaninglessDrafts` then clears the drafts an older, stricter + // comparison saved for changes nobody made; it runs after the upload so the + // entries that just landed are swept in the same pass. $effect(() => { - if ($workspaceStore && $userStore) { + const ws = $workspaceStore + const email = $userStore?.email + if (ws && email) { untrack(() => { purgeLegacyUserDrafts() - void migrateUserDraftsToDb() + void migrateUserDraftsToDb().then(() => pruneMeaninglessDrafts(ws, email)) }) } })