From 0d4cbe5a889de19a722ed548ed92ec383a359436 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Wed, 16 Sep 2026 00:47:26 +0200 Subject: [PATCH] fix: let go of the value once it is back on the deployed one Co-Authored-By: Claude Opus 5 (1M context) --- frontend/src/lib/itemStore.svelte.ts | 55 ++++++++++++++-------------- frontend/src/lib/itemStore.test.ts | 24 ++++++++++++ 2 files changed, 51 insertions(+), 28 deletions(-) diff --git a/frontend/src/lib/itemStore.svelte.ts b/frontend/src/lib/itemStore.svelte.ts index a611ff8f2d..0d03ad5a3a 100644 --- a/frontend/src/lib/itemStore.svelte.ts +++ b/frontend/src/lib/itemStore.svelte.ts @@ -160,12 +160,10 @@ const outOfDate = { error: 'This item changed elsewhere and could not be re-read. Reload it before saving.' } as const -/** Where a command asked to undo or fold into something found it, so it can tell that what is - * here now is no longer that. Only commands that act on the value they were asked about need it: - * a read does not, since it answers against whoever owns the value now rather than against when - * it was issued. `edits`: what the user can see change, which a discard measures against, so an - * outside write of the value already shown does not cancel one. `externals`: every outside - * write, that one included, which is what a patch and a fold must not write over. */ +/** Where a command that undoes or folds into a value found it, so it can tell what is here now + * is no longer that. A read needs none: it asks who owns the value instead. `edits`: what the + * user can see change, so an outside write of the value already shown does not cancel a + * discard. `externals`: every outside write, which a patch and a fold must not write over. */ type AsOf = { edits: number; externals: number } class Entry { @@ -313,6 +311,13 @@ class Entry { // actually there. A reload is what clears it. if (this.stale) return const desired = this.dirty ? (serialize(this.value) ?? null) : null + // Back on the deployed value, so there is nothing of anyone's here for a read to defer to + // — a save landed, or the user put it back. Left standing, it would have the next read + // keep this value while advancing the baseline under it, and that gap drafted. + if (desired === null) this.local = false + // Back on the deployed value, so there is nothing of anyone's here for a read to defer to + // — a save landed, or the user put it back. Left standing, it would have the next read + // keep this value while advancing the baseline under it, and that gap drafted. this.ports.hint(key, desired !== null) // A rejected write is not retried: the row stays as the server has it until the conflict // is resolved, by a reload or an overwrite. The refused payload stays parked, which is @@ -337,11 +342,10 @@ class Entry { waiting: boolean ) { if (this.ports.rowMark(key) !== asOf) return - // A payload the server refused *before* this read predates it, and the baseline taken - // below would make it acceptable, so it goes; the reconcile after the read re-queues - // whatever the value needs. One this read is holding on screen is the other way round: - // no baseline is taken over it, and it is the only record of an edit that never reached - // anyone, so it stays parked for the user to resolve however long that takes. + // Refused before this read, and the baseline taken below would make it acceptable, so it + // goes; the reconcile after re-queues whatever the value needs. One this read holds on + // screen is the other way round: no baseline is taken over it, and it is the only record + // of that edit, so it stays parked until the user resolves it. if (!waiting && this.ports.conflicted(key)) this.ports.dropPending(key) this.row = row === null || row === undefined ? null : (serialize(row) ?? null) // A payload still waiting to be sent was written against the baseline it had then. Taking @@ -429,11 +433,10 @@ class Entry { try { if (this.retired) return superseded if (!adapter.load) throw new Error('This item cannot be loaded') - // A row queued by whoever held this key before is debounced: send it, or this read - // answers with the deployed value and the draft reappears under a clean editor. - // Not one written when no row existed, though — it carries no baseline, so it - // would go out unconditional and take over a row created since. The read below - // gives it one, and `pending` keeps showing it meanwhile. + // A row queued by whoever held this key is debounced: send it, or this read answers + // with the deployed value and the draft reappears under a clean editor. Not one written + // when no row existed — it has no baseline, so it would go out unconditional over a row + // created since; the read below gives it one, and `pending` shows it meanwhile. unbasedPending = this.ports.unbased(key) if (!unbasedPending) await this.ports.flush(key) // Taken here, not when the command was asked for: rows this read's own flush sent @@ -482,11 +485,10 @@ class Entry { } this.serverDeployed = snapshot(res.deployed) this.origin = res.deployed !== undefined ? 'deployed' : 'draft' - // A row the syncer took but never landed is the only copy of that edit, whether it - // failed to send or the server refused it, and whether that happened during this read - // or before an earlier editor was closed. A refusal nobody has answered yet does not - // go stale by being left alone, so the item opens on it and stays conflicted until - // the user settles it. Resolving is what ends that, not reopening. + // A row the syncer took but never landed is the only copy of that edit, however it failed + // and whenever. A refusal nobody has answered does not go stale by being left alone, so + // the item opens on it and stays conflicted until the user settles it: resolving is what + // ends that, not reopening. const unsent = waitingBefore // A parked delete is what this tab wants gone, not what is there: the row is still the // server's, so the reconcile at the end re-issues the delete instead of believing it @@ -502,13 +504,10 @@ class Entry { this.replaceValue(draft ?? res.deployed) } this.removed = this.value === undefined - // Raised after the value above has been taken, so what is on screen is still the - // user's own unsent edit and "keep mine" means theirs: a payload written when this - // key had no row, meeting a row that exists now. Sending it would overwrite that - // unconditionally, and basing it on this read would claim it was built on a draft it - // never saw — so neither happens until the user says which wins. Asked again here - // rather than taken from before the read: that payload may have landed while the read - // was out, and then it has a baseline of its own and the row is its own. + // A payload written when this key had no row, meeting a row that exists now: sending it + // would overwrite that unconditionally, and basing it on this read would claim it was + // built on a draft it never saw, so neither happens until the user says which wins. + // Asked after the read, not before: it may have landed meanwhile and have a baseline. if (this.ports.unbased(key) && res.draft !== undefined && res.draftSavedAt !== undefined) { this.ports.markConflict(key, res.draftSavedAt) } diff --git a/frontend/src/lib/itemStore.test.ts b/frontend/src/lib/itemStore.test.ts index 3a26a3cbb4..755c4c48e5 100644 --- a/frontend/src/lib/itemStore.test.ts +++ b/frontend/src/lib/itemStore.test.ts @@ -179,6 +179,30 @@ async function open( const settle = () => new Promise((r) => setTimeout(r, 0)) describe('item store: the draft row follows value ≠ deployed', () => { + it('takes a new deployed value after an edit was put back, without drafting the old one', async () => { + const rows = fakeRows() + let deployed = deployedRes + const { item } = await open( + rows, + adapter(async () => ({ deployed })) + ) + // Edited and then put back by hand. Nothing here is the user's any more, so this editor + // speaks for the item no differently than one just opened. + item.value = { ...deployedRes, description: 'edited' } + item.value = structuredClone(deployedRes) + expect(item.dirty).toBe(false) + + deployed = { ...deployedRes, description: 'deployed elsewhere' } + const writesBefore = rows.writes.length + await item.reload() + + // Holding the old value against the new baseline would read as an unsaved change nobody + // made, and post it as a draft over whatever is there. + expect(item.value).toEqual(deployed) + expect(item.dirty).toBe(false) + expect(rows.writes.length).toBe(writesBefore) + }) + it('writes a row when the value diverges and deletes it when it comes back', async () => { const rows = fakeRows() const { item } = await open(rows, adapter({ deployed: deployedRes }))