From a0b411eb9875832901620eb2bed933838046d13f Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Sat, 5 Sep 2026 19:28:59 +0200 Subject: [PATCH] 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 --- frontend/src/lib/userDraftPrune.test.ts | 39 +++++++++++++++- frontend/src/lib/userDraftPrune.ts | 60 ++++++++++++++++++++----- 2 files changed, 88 insertions(+), 11 deletions(-) diff --git a/frontend/src/lib/userDraftPrune.test.ts b/frontend/src/lib/userDraftPrune.test.ts index a6ae3076f6..b9400f5c25 100644 --- a/frontend/src/lib/userDraftPrune.test.ts +++ b/frontend/src/lib/userDraftPrune.test.ts @@ -24,10 +24,20 @@ vi.mock('./toast', () => ({ sendUserToast: (...a: unknown[]) => sendUserToast(.. 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(), - recordRemoteSync: (...a: unknown[]) => recordRemoteSync(...(a as [])), + 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 @@ -75,6 +85,7 @@ beforeEach(() => { syncState = 'none' conflict = undefined liveDraft = false + syncerCalls = [] }) describe('pruneMeaninglessDrafts', () => { @@ -135,6 +146,32 @@ describe('pruneMeaninglessDrafts', () => { { 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() + // 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() + }) + + 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 () => { diff --git a/frontend/src/lib/userDraftPrune.ts b/frontend/src/lib/userDraftPrune.ts index d878aae917..f3fe320489 100644 --- a/frontend/src/lib/userDraftPrune.ts +++ b/frontend/src/lib/userDraftPrune.ts @@ -40,6 +40,11 @@ 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. */ @@ -61,14 +66,35 @@ type Candidate = { 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' } -/** `undefined` when the diff could not be fetched — distinct from `false`, so - * the caller can leave the pass open rather than strand a row it never judged. */ +/** 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 @@ -84,8 +110,8 @@ async function carriesNoChanges( // two sides would compare equal by construction. if (!hasDraft || noDeployed) return false return draftValuesEqual(draft, deployed) - } catch { - return undefined + } catch (e) { + return isPermanentlyUnjudgeable(e) ? false : undefined } } @@ -166,11 +192,17 @@ export async function pruneMeaninglessDrafts(workspace: string, userKey: string) 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, and the syncer keeps a - // refused baseline until something resolves it. 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. Adopt what the server reported instead. + // 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++ } @@ -178,12 +210,20 @@ export async function pruneMeaninglessDrafts(workspace: string, userKey: string) invalidateWorkspaceDrafts(workspace) sendUserToast(`Cleared ${discarded} draft${discarded > 1 ? 's' : ''} that carried no changes`) } - if (unresolved === 0) { + // 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