From 2343c16f5037d49d4c17ed466fde2b590e52eb08 Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Sat, 5 Sep 2026 20:25:31 +0200 Subject: [PATCH] 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 --- frontend/src/lib/userDraftPrune.test.ts | 107 ++++++++---------------- frontend/src/lib/userDraftPrune.ts | 58 +++++++------ 2 files changed, 65 insertions(+), 100 deletions(-) diff --git a/frontend/src/lib/userDraftPrune.test.ts b/frontend/src/lib/userDraftPrune.test.ts index b9400f5c25..0276598d45 100644 --- a/frontend/src/lib/userDraftPrune.test.ts +++ b/frontend/src/lib/userDraftPrune.test.ts @@ -5,48 +5,34 @@ import { describe, it, expect, beforeEach, vi } from 'vitest' // per-kind diff — and assert on what it discards. const listDrafts = vi.fn() const getDraftDiffValues = vi.fn() -const discardDraft = vi.fn(async () => ({ success: true })) +const updateDraft = vi.fn(async () => ({ status: 'saved', current_timestamp: 'x' })) vi.mock('./gen', () => ({ - DraftService: { listDrafts: (...a: unknown[]) => listDrafts(...(a as [])) } + DraftService: { + listDrafts: (...a: unknown[]) => listDrafts(...(a as [])), + updateDraft: (...a: unknown[]) => updateDraft(...(a as [])) + } })) -// Only the two network-touching functions are stubbed; `canDiffDraftKind` is -// the real one, so the kind filter is pinned against the actual overlay table. +// 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 [])), - discardDraft: (...a: unknown[]) => discardDraft(...(a as [])) + 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' -let conflict: unknown = undefined -const recordRemoteSync = vi.fn() -const dropPending = vi.fn() -/** Ordered log of the two calls whose ORDER is the guard being pinned. */ -let syncerCalls: string[] = [] vi.mock('./userDraftDbSyncer.svelte', () => ({ UserDraftDbSyncer: { - save: vi.fn(), - dropPending: (...a: unknown[]) => { - syncerCalls.push('dropPending') - return dropPending(...(a as [])) - }, - recordRemoteSync: (...a: unknown[]) => { - syncerCalls.push('recordRemoteSync') - return recordRemoteSync(...(a as [])) - }, getState: () => ({ get state() { return syncState } - }), - getConflict: () => ({ - get conflict() { - return conflict - } }) } })) @@ -76,16 +62,14 @@ const diff = (over: Record = {}) => ({ noDeployed: false, ...over }) -const discardedPaths = () => discardDraft.mock.calls.map((c: any[]) => c[1]) +const discardedPaths = () => updateDraft.mock.calls.map((c: any[]) => c[0].path as string) beforeEach(() => { localStorage.clear() vi.clearAllMocks() - discardDraft.mockResolvedValue({ success: true }) + updateDraft.mockResolvedValue({ status: 'saved', current_timestamp: 'x' }) syncState = 'none' - conflict = undefined liveDraft = false - syncerCalls = [] }) describe('pruneMeaninglessDrafts', () => { @@ -100,28 +84,28 @@ describe('pruneMeaninglessDrafts', () => { listDrafts.mockResolvedValue([row()]) getDraftDiffValues.mockResolvedValue(diff({ draft: { value: { host: 'other' } } })) await pruneMeaninglessDrafts('main', 'me@x.dev') - expect(discardDraft).not.toHaveBeenCalled() + 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(discardDraft).not.toHaveBeenCalled() + 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(discardDraft).not.toHaveBeenCalled() + 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(discardDraft).not.toHaveBeenCalled() + 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()) @@ -132,36 +116,29 @@ describe('pruneMeaninglessDrafts', () => { it('conditions the delete on the timestamp it judged, so a row that moved is spared', async () => { listDrafts.mockResolvedValue([row()]) getDraftDiffValues.mockResolvedValue(diff()) - conflict = { serverTimestamp: '2026-01-02T00:00:00Z', localLastSync: null } + updateDraft.mockResolvedValue({ status: 'conflict', current_timestamp: 'newer' }) await pruneMeaninglessDrafts('main', 'me@x.dev') - expect(recordRemoteSync).toHaveBeenCalledWith( - { workspace: 'main', itemKind: 'resource', path: 'u/me/r' }, - '2026-01-01T00:00:00Z' - ) - // The discard was attempted but refused, so nothing is reported as cleared. + 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() - // …and the refused baseline is handed back to the server's, so the next - // autosave for this key is not refused too. - expect(recordRemoteSync).toHaveBeenLastCalledWith( - { workspace: 'main', itemKind: 'resource', path: 'u/me/r' }, - '2026-01-02T00:00:00Z' - ) - // Order matters: the refused delete stays parked for the pagehide flush, - // which would re-send it with whatever baseline is current. Adopting the - // server's timestamp first would be handing it the one value that works. - expect(syncerCalls.slice(-2)).toEqual(['dropPending', 'recordRemoteSync']) }) 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(discardDraft).not.toHaveBeenCalled() + 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(discardDraft).not.toHaveBeenCalled() + expect(updateDraft).not.toHaveBeenCalled() }) it('gives up after a bounded number of unresolved passes', async () => { @@ -183,7 +160,7 @@ describe('pruneMeaninglessDrafts', () => { listDrafts.mockResolvedValue([row()]) getDraftDiffValues.mockResolvedValue(diff()) await pruneMeaninglessDrafts('main', 'me@x.dev') - expect(discardDraft).not.toHaveBeenCalled() + expect(updateDraft).not.toHaveBeenCalled() }) it('leaves alone a draft this tab is editing', async () => { @@ -191,7 +168,7 @@ describe('pruneMeaninglessDrafts', () => { listDrafts.mockResolvedValue([row()]) getDraftDiffValues.mockResolvedValue(diff()) await pruneMeaninglessDrafts('main', 'me@x.dev') - expect(discardDraft).not.toHaveBeenCalled() + expect(updateDraft).not.toHaveBeenCalled() }) it('leaves alone a draft with a write queued or in flight', async () => { @@ -199,26 +176,16 @@ describe('pruneMeaninglessDrafts', () => { listDrafts.mockResolvedValue([row()]) getDraftDiffValues.mockResolvedValue(diff()) await pruneMeaninglessDrafts('main', 'me@x.dev') - expect(discardDraft).not.toHaveBeenCalled() + expect(updateDraft).not.toHaveBeenCalled() }) - it('does not count, or seal the pass on, a delete the syncer failed to send', async () => { + it('does not count, or seal the pass on, a delete that failed to send', async () => { listDrafts.mockResolvedValue([row()]) getDraftDiffValues.mockResolvedValue(diff()) - // The discard's own POST is what fails, so the state flips during it. - discardDraft.mockImplementation(async () => { - syncState = 'failed' - return { success: true } - }) + updateDraft.mockRejectedValueOnce(new Error('network')) await pruneMeaninglessDrafts('main', 'me@x.dev') expect(sendUserToast).not.toHaveBeenCalled() - discardDraft.mockResolvedValue({ success: true }) - // 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' + // 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']) }) @@ -227,7 +194,7 @@ describe('pruneMeaninglessDrafts', () => { listDrafts.mockResolvedValue([row({ legacy_draft: true })]) getDraftDiffValues.mockResolvedValue(diff()) await pruneMeaninglessDrafts('main', 'me@x.dev') - expect(discardDraft).not.toHaveBeenCalled() + expect(updateDraft).not.toHaveBeenCalled() }) it('only sweeps the kinds whose editors are gated', async () => { @@ -260,9 +227,9 @@ describe('pruneMeaninglessDrafts', () => { getDraftDiffValues.mockResolvedValue(diff()) await pruneMeaninglessDrafts('main', 'me@x.dev') await pruneMeaninglessDrafts('main', 'me@x.dev') - expect(discardDraft).toHaveBeenCalledTimes(1) + expect(updateDraft).toHaveBeenCalledTimes(1) await pruneMeaninglessDrafts('other', 'me@x.dev') - expect(discardDraft).toHaveBeenCalledTimes(2) + expect(updateDraft).toHaveBeenCalledTimes(2) }) it('retries next mount when the listing failed', async () => { diff --git a/frontend/src/lib/userDraftPrune.ts b/frontend/src/lib/userDraftPrune.ts index f3fe320489..4084da3837 100644 --- a/frontend/src/lib/userDraftPrune.ts +++ b/frontend/src/lib/userDraftPrune.ts @@ -21,21 +21,25 @@ * * 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. Two guards keep that from eating work. Anything this tab is - * currently writing is skipped outright — a discard POSTs immediately, which - * cancels the autosave the user's keystrokes have queued. And the delete - * itself is a compare-and-delete: the listing's timestamp is seeded as the - * `last_sync` baseline, so the backend refuses it if the row moved since, - * whoever moved it. Without the seed a freshly loaded tab has no baseline and - * the delete is unconditional. + * 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, discardDraft, getDraftDiffValues } from './utils_draft_deploy' +import { canDiffDraftKind, getDraftDiffValues } from './utils_draft_deploy' import { invalidateWorkspaceDrafts } from './workspaceDrafts.svelte' const SENTINEL_PREFIX = 'userdraft/pruned/v1/' @@ -183,28 +187,22 @@ export async function pruneMeaninglessDrafts(workspace: string, userKey: string) 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, 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. - const conflict = UserDraftDbSyncer.getConflict(q).conflict - if (!res.success || UserDraftDbSyncer.getState(q).state === 'failed') unresolved++ - else if (conflict) { - // The row moved past the baseline we seeded. Drop our refused delete - // BEFORE adopting the server's timestamp: a refused save stays parked - // for the `pagehide` keepalive flush, which re-sends it with whatever - // baseline is current by then — so adopting first would hand it the - // one timestamp that makes the delete succeed, against a row that now - // holds someone's newer draft. - UserDraftDbSyncer.dropPending(q) - // Then re-baseline. The syncer keeps a refused baseline until - // something resolves it, and nothing here would: these drawer editors - // never re-seed on load and mount no conflict modal, so every later - // autosave for this key would be refused and the user's edit lost. - UserDraftDbSyncer.recordRemoteSync(q, conflict.serverTimestamp) - } else discarded++ + 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)