From 23193d8eb84b2da46c7a99cf91aaa9d26ff218c0 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Sun, 20 Sep 2026 01:20:48 +0200 Subject: [PATCH] fix: free a reopened editor from a resolution the closed one left in flight Co-Authored-By: Claude Opus 5 (1M context) --- .../src/lib/components/ResourceEditor.svelte | 30 +++++--------- .../src/lib/components/VariableEditor.svelte | 29 +++++-------- .../src/lib/draftConflictSession.svelte.ts | 41 +++++++++++++++++++ frontend/src/lib/draftConflictSession.test.ts | 31 ++++++++++++++ 4 files changed, 93 insertions(+), 38 deletions(-) create mode 100644 frontend/src/lib/draftConflictSession.svelte.ts create mode 100644 frontend/src/lib/draftConflictSession.test.ts diff --git a/frontend/src/lib/components/ResourceEditor.svelte b/frontend/src/lib/components/ResourceEditor.svelte index 7f49502dae..817cc76d9f 100644 --- a/frontend/src/lib/components/ResourceEditor.svelte +++ b/frontend/src/lib/components/ResourceEditor.svelte @@ -20,6 +20,7 @@ import { useActingUser } from '$lib/actingUser.svelte' import { UserDraft, draftValuesEqual, type UserDraftHandle } from '$lib/userDraft.svelte' import { UserDraftDbSyncer } from '$lib/userDraftDbSyncer.svelte' + import { useDraftConflictSession } from '$lib/draftConflictSession.svelte' import DraftConflictAlert from './DraftConflictAlert.svelte' import { setLocalDraftHint } from '$lib/localDraftHints.svelte' import { onUserInput } from '$lib/userDraftEditGate' @@ -266,22 +267,16 @@ ) const anyDirty = $derived(dirtyWorkspaces.length > 0) - /** Which editing session a conflict resolution belongs to. Comparing `selected`/path is not - * enough — this component outlives the drawer and reopening the same resource reuses it with - * those same values — so a resolution carries the session it started in and every step checks - * it is still the current one. */ - let resolveGeneration = 0 - /** Nothing outstanding speaks for this editor any more. Exported because the drawer, not this - * component, is what knows a session has ended. */ + /** This component outlives the drawer and reopening the same resource reuses it with the same + * `selected`/path, so a resolution cannot tell from those whether it still speaks for what is + * on screen. Ending is exported because the drawer, not this component, knows when a session + * is over. */ + const conflictSession = useDraftConflictSession() export function endEditingSession(): void { - resolveGeneration++ + conflictSession.end() } onDestroy(endEditingSession) - /** The session whose resolution is in flight, or 0. Scoped by generation rather than a plain - * flag: a request left over from a closed session must not leave the next one showing busy - * buttons it can never un-disable, and its `finally` must not clear a newer one's. */ - let resolvingFor = $state(0) - const resolvingConflict = $derived(resolvingFor !== 0 && resolvingFor === resolveGeneration) + const resolvingConflict = $derived(conflictSession.busy) /** The server refused this tab's autosave because the row moved under it: another tab, or the * AI chat, which writes these drafts too. Nothing typed here reaches the server until the user * picks a version, and the unsaved-changes banner says the opposite — that the edits are held @@ -301,9 +296,8 @@ const p = initialPath if (!ws || !p || resolvingConflict) return const query = { workspace: ws, itemKind: 'resource' as const, path: p } - const gen = ++resolveGeneration - const stillOurs = () => gen === resolveGeneration && selected === ws - resolvingFor = gen + const token = conflictSession.start() + const stillOurs = () => conflictSession.holds(token) && selected === ws try { if (keepMine) { // Settle the key first: an ordinary autosave still queued would displace the forced @@ -357,9 +351,7 @@ // resolve again — which is the whole point of reading first. sendUserToast(`Could not load the other version: ${e}`, true) } finally { - // Only if it is still ours: a stale one settling later must not clear the busy state of - // the session that replaced it. - if (resolvingFor === gen) resolvingFor = 0 + conflictSession.finish(token) } } diff --git a/frontend/src/lib/components/VariableEditor.svelte b/frontend/src/lib/components/VariableEditor.svelte index 0b54df0f22..2a7c9a98a5 100644 --- a/frontend/src/lib/components/VariableEditor.svelte +++ b/frontend/src/lib/components/VariableEditor.svelte @@ -25,6 +25,7 @@ import LocalDraftBanner from './LocalDraftBanner.svelte' import DraftConflictAlert from './DraftConflictAlert.svelte' import { UserDraftDbSyncer } from '$lib/userDraftDbSyncer.svelte' + import { useDraftConflictSession } from '$lib/draftConflictSession.svelte' import { isEncryptedDraftValue } from '$lib/encryptedDraft' import { setLocalDraftHint } from '$lib/localDraftHints.svelte' @@ -125,22 +126,15 @@ Object.keys(states).filter((ws) => !draftValuesEqual(states[ws].draft, initialStates[ws])) ) - /** Which editing session a conflict resolution belongs to. Comparing `selected`/path is not - * enough — reopening the same variable reuses this component and those same values — so a - * resolution carries the session it started in and every step checks it is still the current - * one. Ended by `endEditingSession`, which every entry point and the teardown go through. */ - let resolveGeneration = 0 - /** Nothing outstanding speaks for this editor any more: a different variable, a different - * session on the same one, or the component going away. */ + /** Ended by every entry point and by the teardown: reopening the same variable reuses this + * component, so `selected`/path cannot tell a resolution whether it still speaks for what is + * on screen. */ + const conflictSession = useDraftConflictSession() function endEditingSession(): void { - resolveGeneration++ + conflictSession.end() } onDestroy(endEditingSession) - /** The session whose resolution is in flight, or 0. Scoped by generation rather than a plain - * flag: a request left over from a closed session must not leave the next one showing busy - * buttons it can never un-disable, and its `finally` must not clear a newer one's. */ - let resolvingFor = $state(0) - const resolvingConflict = $derived(resolvingFor !== 0 && resolvingFor === resolveGeneration) + const resolvingConflict = $derived(conflictSession.busy) /** The server refused this tab's autosave because the row moved under it: another tab, or the * AI chat, which writes these drafts too. Nothing typed here reaches the server until the user * picks a version, and the unsaved-changes banner says the opposite — that the edits are held @@ -160,9 +154,8 @@ const p = editPath if (!ws || !p || resolvingConflict) return const query = { workspace: ws, itemKind: 'variable' as const, path: p } - const gen = ++resolveGeneration - const stillOurs = () => gen === resolveGeneration && selected === ws - resolvingFor = gen + const token = conflictSession.start() + const stillOurs = () => conflictSession.holds(token) && selected === ws try { if (keepMine) { // Settle the key first: an ordinary autosave still queued would displace the forced @@ -224,9 +217,7 @@ // resolve again — which is the whole point of reading first. sendUserToast(`Could not load the other version: ${e}`, true) } finally { - // Only if it is still ours: a stale one settling later must not clear the busy state of - // the session that replaced it. - if (resolvingFor === gen) resolvingFor = 0 + conflictSession.finish(token) } } diff --git a/frontend/src/lib/draftConflictSession.svelte.ts b/frontend/src/lib/draftConflictSession.svelte.ts new file mode 100644 index 0000000000..4938941306 --- /dev/null +++ b/frontend/src/lib/draftConflictSession.svelte.ts @@ -0,0 +1,41 @@ +/** + * Identity for one editing session's conflict resolution. + * + * A drawer editor outlives what it opens: reopening the same item reuses the component with the + * same workspace and path, so an awaited step cannot tell from those alone whether it still + * speaks for what is on screen. Every resolution takes a token and checks it before touching + * shared editor state. + */ +export function useDraftConflictSession() { + let generation = $state(0) + /** The token of the resolution in flight, or 0. A token rather than a flag: one left over from + * a closed session must not clear the busy state of the session that replaced it. */ + let inFlight = $state(0) + + return { + /** A resolution started in the current session is in flight. Ending a session clears it, so + * a reopened editor never shows buttons that only a stale request settling can re-enable. */ + get busy(): boolean { + return inFlight !== 0 + }, + /** Claim the session for a resolution about to start. */ + start(): number { + inFlight = ++generation + return inFlight + }, + /** Whether `token` still speaks for the editor. */ + holds(token: number): boolean { + return token === generation + }, + /** Release `token`'s claim. A stale token is ignored. */ + finish(token: number): void { + if (inFlight === token) inFlight = 0 + }, + /** Nothing outstanding speaks for this editor any more: a different item, a different + * session on the same one, or the component going away. */ + end(): void { + generation++ + inFlight = 0 + } + } +} diff --git a/frontend/src/lib/draftConflictSession.test.ts b/frontend/src/lib/draftConflictSession.test.ts new file mode 100644 index 0000000000..0fee72ba45 --- /dev/null +++ b/frontend/src/lib/draftConflictSession.test.ts @@ -0,0 +1,31 @@ +import { describe, it, expect } from 'vitest' + +import { useDraftConflictSession } from './draftConflictSession.svelte' + +/** + * A drawer editor is reused by the next thing it opens, so a resolution the closed session left + * in flight must neither speak for the new one nor hold its buttons disabled until it settles. + */ +describe('useDraftConflictSession', () => { + it('frees a reopened session from a resolution the closed one left in flight', () => { + const session = useDraftConflictSession() + + const stale = session.start() + expect(session.busy).toBe(true) + + session.end() + expect(session.busy).toBe(false) + expect(session.holds(stale)).toBe(false) + + const fresh = session.start() + expect(session.busy).toBe(true) + expect(session.holds(fresh)).toBe(true) + + // The stale request settles last: it must not release the live session's claim. + session.finish(stale) + expect(session.busy).toBe(true) + + session.finish(fresh) + expect(session.busy).toBe(false) + }) +})