From 8455c7da3a595b835ca608d85c3615af110e99fc Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Tue, 15 Sep 2026 11:44:14 +0200 Subject: [PATCH] fix: tag which patch owns a field rather than inferring it from the value Co-Authored-By: Claude Opus 5 (1M context) --- frontend/src/lib/itemStore.svelte.ts | 26 +++++++++++++++---------- frontend/src/lib/itemStore.test.ts | 29 ++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 10 deletions(-) diff --git a/frontend/src/lib/itemStore.svelte.ts b/frontend/src/lib/itemStore.svelte.ts index 4071dd553e..3023a1af0e 100644 --- a/frontend/src/lib/itemStore.svelte.ts +++ b/frontend/src/lib/itemStore.svelte.ts @@ -139,6 +139,9 @@ function errorMessage(e: unknown): string { /** Revisions are unique across entries, so a consumer switching items never mistakes one * item's revision for another's. */ let nextRevision = 1 +/** Identifies which patch owns a field it put ahead of the server: two patches of one field can + * carry the same value, so ownership cannot be told from the value. */ +let nextPatch = 1 const superseded = { ok: false, error: 'Another save of this item replaced this one' } as const @@ -151,19 +154,21 @@ class Entry { value: V | undefined = $state() /** What the server last told us this item holds. */ private serverDeployed: V | undefined = $state.raw() - /** Fields a `patch` has put ahead of the server, until it lands or is refused. */ - private patched: Record = $state({}) + /** Fields a `patch` has put ahead of the server, each tagged with the patch that owns it, + * until that patch lands or is refused. */ + private patched: Record = $state({}) /** * The baseline the value is measured against: what the server holds, with whatever a patch * is currently ahead of it on. Derived rather than assigned, so that no writer of the server * side can forget the overlay and roll the baseline back past an optimistic toggle — which * reads as an unsaved change to a field the user never touched. */ - deployed: V | undefined = $derived( - this.serverDeployed === undefined - ? undefined - : ({ ...this.serverDeployed, ...this.patched } as V) - ) + deployed: V | undefined = $derived.by(() => { + if (this.serverDeployed === undefined) return undefined + const out = { ...this.serverDeployed } as Record + for (const [field, held] of Object.entries(this.patched)) out[field] = held.value + return out as V + }) template: V | undefined = $state.raw() origin: ItemOrigin | undefined = $state() meta: unknown = $state.raw() @@ -589,6 +594,7 @@ class Entry { */ patch(fields: Partial, write: (key: ItemKey) => Promise): Promise { this.pristine = false + const id = nextPatch++ const onTemplate = this.origin === 'new' const sent = snapshot(fields) as Record if (onTemplate) { @@ -596,14 +602,14 @@ class Entry { // racing it, so the patch lands on it directly. if (this.template !== undefined) this.template = { ...this.template, ...sent } as V } else { - Object.assign(this.patched, sent) + for (const [k, v] of Object.entries(sent)) this.patched[k] = { by: id, value: v } } if (this.value !== undefined) Object.assign(this.value as object, fields) this.touch() /** Release each field this patch still owns; one a later patch has taken stays with it. */ const release = () => { - for (const [k, v] of Object.entries(sent)) { - if (deepEqual(this.patched[k], v)) delete this.patched[k] + for (const k of Object.keys(sent)) { + if (this.patched[k]?.by === id) delete this.patched[k] } } return this.run(async () => { diff --git a/frontend/src/lib/itemStore.test.ts b/frontend/src/lib/itemStore.test.ts index 28b04e5a1d..02ce5bc699 100644 --- a/frontend/src/lib/itemStore.test.ts +++ b/frontend/src/lib/itemStore.test.ts @@ -403,6 +403,35 @@ describe('item store: commands', () => { expect(rows.writes).toEqual([]) }) + it('keeps a toggle whose value an earlier accepted one repeats', async () => { + type Sched = { path: string; enabled: boolean } + const rows = fakeRows() + const store = createItemStore(rows.port) + const { handle: item } = store.acquire( + { workspace: 'w', kind: 'trigger_schedule', path: 's' }, + { workspace: 'w', path: 's' }, + { load: async () => ({ deployed: { path: 's', enabled: false } }) } as ItemAdapter + ) + await settle() + const gates = [deferred(), deferred(), deferred()] + // enable, disable, enable: the first and third carry the same value, so only a token can + // say which of them still owns the field. + const first = item.patch({ enabled: true }, () => gates[0].promise) + const second = item.patch({ enabled: false }, () => gates[1].promise) + const third = item.patch({ enabled: true }, () => gates[2].promise) + gates[0].resolve() + gates[1].reject(new Error('refused')) + gates[2].reject(new Error('refused')) + + expect(await first).toEqual({ ok: true }) + expect(await second).toMatchObject({ ok: false }) + expect(await third).toMatchObject({ ok: false }) + // The server took the first and refused the rest, so it holds `true`. + expect(item.deployed).toEqual({ path: 's', enabled: true }) + expect(item.value).toEqual({ path: 's', enabled: true }) + expect(rows.writes).toEqual([]) + }) + it('saves a change the draft comparison ignores', async () => { type Sched = { path: string; permissioned_as?: string } const rows = fakeRows()