From 92ebe73494e5717bc6fcaee5dd05118dae13fd75 Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Sat, 5 Sep 2026 16:21:42 +0200 Subject: [PATCH] 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 --- .../src/lib/components/ResourceEditor.svelte | 12 ++++- frontend/src/lib/userDraftEditGate.ts | 50 +++++++++++++------ frontend/src/lib/userDraftPrune.test.ts | 38 +++++++++++++- frontend/src/lib/userDraftPrune.ts | 43 ++++++++++++---- 4 files changed, 115 insertions(+), 28 deletions(-) diff --git a/frontend/src/lib/components/ResourceEditor.svelte b/frontend/src/lib/components/ResourceEditor.svelte index 5b5ea4b87c..13f838cb89 100644 --- a/frontend/src/lib/components/ResourceEditor.svelte +++ b/frontend/src/lib/components/ResourceEditor.svelte @@ -136,8 +136,16 @@ // to its form — the form is not on screen yet — but it would open the gate, // and the gating effect would then un-suspend the moment the fetch lands, // in time for the schema's materialized values to POST as a draft. - onUserInput(() => { - if (selected && selected in states) userEdited[selected] = true + // + // The same holds for the rest of the load: the schema arrives separately and + // materializes on arrival, so until it settles a bare click is not enough to + // call this an edit. `Path`, the labels and the description ARE editable + // through that window though — they render above the schema form's skeleton + // — so a real value event still opens the gate and keeps that edit. + onUserInput((kind) => { + if (!selected || !(selected in states)) return + if (kind === 'precursor' && loadingSchema) return + userEdited[selected] = true }) $effect(() => { diff --git a/frontend/src/lib/userDraftEditGate.ts b/frontend/src/lib/userDraftEditGate.ts index 2593e9b9a7..906decbb52 100644 --- a/frontend/src/lib/userDraftEditGate.ts +++ b/frontend/src/lib/userDraftEditGate.ts @@ -1,5 +1,22 @@ 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, or an assistive technology activating a + * control, produce no pointer or key event in this document at all. */ +const VALUE_EVENTS = ['input', 'change', 'drop', 'paste'] as const +const PRECURSOR_EVENTS = ['pointerdown', 'keydown'] 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 @@ -16,25 +33,30 @@ import { onDestroy } from 'svelte' * gating means for them — suspending the autosave, absorbing the settled value * into the deployed baseline, or both. * - * `pointerdown` and `keydown` are the two events that precede every human - * edit, and 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. + * 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: () => void): void { +export function onUserInput(handle: (kind: UserInputKind) => void): void { if (typeof document === 'undefined') return - const onEvent = (e: Event) => { - if (e.isTrusted) handle() + 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]) } - document.addEventListener('pointerdown', onEvent, true) - document.addEventListener('keydown', onEvent, true) + for (const type of VALUE_EVENTS) register(type, 'value') + for (const type of PRECURSOR_EVENTS) register(type, 'precursor') onDestroy(() => { - document.removeEventListener('pointerdown', onEvent, true) - document.removeEventListener('keydown', onEvent, true) + 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 index 5ed0489ef6..78bf581ddc 100644 --- a/frontend/src/lib/userDraftPrune.test.ts +++ b/frontend/src/lib/userDraftPrune.test.ts @@ -150,12 +150,48 @@ describe('pruneMeaninglessDrafts', () => { await pruneMeaninglessDrafts('main', 'me@x.dev') expect(sendUserToast).not.toHaveBeenCalled() discardDraft.mockResolvedValue({ success: true }) - // Sentinel unwritten, so the next mount retries the draft left behind. + // A same-session retry sees the key as busy (a failed save leaves the + // payload parked), attempts nothing — and must still not seal the pass. + await pruneMeaninglessDrafts('main', 'me@x.dev') + expect(discardedPaths()).toEqual(['u/me/r']) + // Once the failure clears, the draft left behind is finally retried. syncState = 'none' 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(discardDraft).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()) diff --git a/frontend/src/lib/userDraftPrune.ts b/frontend/src/lib/userDraftPrune.ts index 702c067326..401cf3a709 100644 --- a/frontend/src/lib/userDraftPrune.ts +++ b/frontend/src/lib/userDraftPrune.ts @@ -8,12 +8,19 @@ * browser, after the localStorage→DB migration so anything it just uploaded is * swept too. * + * Scoped to the kinds whose editors this 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. + * with no diff support, a failed fetch, and the legacy workspace-level rows, + * which belong to nobody, are admin-gated to migrate, and whose discard path + * takes no `last_sync` and so could not be conditioned anyway. * * 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 @@ -36,6 +43,12 @@ import { invalidateWorkspaceDrafts } from './workspaceDrafts.svelte' const SENTINEL_PREFIX = 'userdraft/pruned/v1/' +/** The editors whose forms are built from a schema, and so the only kinds that + * could have banked a draft nobody wrote. */ +function isGatedKind(kind: UserDraftItemKind): boolean { + return kind === 'resource' || kind.startsWith('trigger_') +} + /** 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 @@ -46,7 +59,6 @@ const inFlight = new Set() type Candidate = { kind: UserDraftItemKind path: string - legacy: boolean /** The row's `created_at` as listed — the baseline the delete is conditioned on. */ createdAt: string } @@ -106,15 +118,24 @@ export async function pruneMeaninglessDrafts(workspace: string, userKey: string) 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 — a later pass in the same session would skip it + // again (a failed key reads as busy) and then seal with nothing attempted. + 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) => !busyLocally(workspace, r.kind, r.path)) + .filter((r) => isGatedKind(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, - legacy: r.legacy_draft, createdAt: r.created_at })) @@ -124,27 +145,27 @@ export async function pruneMeaninglessDrafts(workspace: string, userKey: string) }) let discarded = 0 - let failed = 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)) continue + if (busyLocally(workspace, c.kind, c.path)) { + unresolved++ + continue + } const q = { workspace, itemKind: c.kind, path: c.path } UserDraftDbSyncer.recordRemoteSync(q, c.createdAt) - const res = await discardDraft(c.kind, c.path, workspace, false, c.legacy, false) + const res = await discardDraft(c.kind, c.path, workspace, false, false, false) // Neither outcome throws: the syncer swallows an HTTP failure into its // per-key state, and a delete refused for a moved row comes back as a // conflict. So `success` alone says nothing about whether the row went. - if (!res.success || UserDraftDbSyncer.getState(q).state === 'failed') failed++ + if (!res.success || UserDraftDbSyncer.getState(q).state === 'failed') unresolved++ else if (!UserDraftDbSyncer.getConflict(q).conflict) discarded++ } if (discarded > 0) { invalidateWorkspaceDrafts(workspace) sendUserToast(`Cleared ${discarded} draft${discarded > 1 ? 's' : ''} that carried no changes`) } - // Only once every deletion this pass attempted actually landed. A draft - // left behind by a failed delete would otherwise never be revisited. - if (failed === 0) { + if (unresolved === 0) { try { localStorage.setItem(sentinel, new Date().toISOString()) } catch {