From a73c2be1b622ddea37891c33c111d1203ff5c5c9 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Wed, 16 Sep 2026 00:07:20 +0200 Subject: [PATCH] refactor: have a read ask who owns the value, not what moved since Co-Authored-By: Claude Opus 5 (1M context) --- frontend/src/lib/itemStore.svelte.ts | 62 ++++++++++++++++++++-------- 1 file changed, 44 insertions(+), 18 deletions(-) diff --git a/frontend/src/lib/itemStore.svelte.ts b/frontend/src/lib/itemStore.svelte.ts index 84388faf47..c673531ad2 100644 --- a/frontend/src/lib/itemStore.svelte.ts +++ b/frontend/src/lib/itemStore.svelte.ts @@ -160,10 +160,12 @@ const outOfDate = { error: 'This item changed elsewhere and could not be re-read. Reload it before saving.' } as const -/** What a command did not do, so it can tell what moved under it. `edits`: what the user can see - * change, which a discard measures against — an outside write of the value already shown does - * not cancel one. `externals`: every outside write, that one included, since it made the value a - * row a read in flight has not got. */ +/** 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. */ type AsOf = { edits: number; externals: number } class Entry { @@ -211,6 +213,10 @@ class Entry { busy = $derived(this.commands > 0) + /** A patch is holding a field ahead of the server. The value carries it and so does + * `deployed`, so the two agree and `dirty` cannot see it: it takes asking. */ + private patching = $derived(Object.keys(this.patched).length > 0) + status: ItemStatus = $derived.by(() => { if (this.loads > 0 && !this.loaded) return 'loading' if (this.commands > 0) return 'saving' @@ -257,6 +263,11 @@ class Entry { * it is deliberately keeping the one a newer edit put there. Decided inside the command, so * after whatever load was queued in front of it, not before. */ private ownsRow = false + /** Whether the value here was put here by someone editing this item — the user, or an outside + * writer — rather than installed by a command reading it. A read must not replace one that + * was: it is an intent, and the read predates it. Cleared by every command that installs a + * value of its own, which is what makes the value the server's again. */ + private local = false private queue: Promise = Promise.resolve() private ports: ItemRowPort private store: StoreInternals @@ -286,7 +297,10 @@ class Entry { this.serverDeployed = value } else { this.pristine = false + this.local = true } + } else { + this.local = true } this.reconcile() } @@ -338,9 +352,12 @@ class Entry { if (!waiting) this.ports.seedSync(key, draftSavedAt, asOf) } + /** Install a value on the item's behalf: a read, a discard, a delete. Answers whatever local + * value was here, so the next read is free to speak for this item again. */ private replaceValue(value: V | undefined): void { this.value = snapshot(value) this.seen = serialize(this.value) + this.local = false this.revision = nextRevision++ } @@ -400,11 +417,11 @@ class Entry { this.replaceValue(template) } - /** `editedBefore`: the edit count as of when this read was decided on, for a caller that did - * something async first. */ - load(adapter: ItemAdapter, asOf?: AsOf, unasked = false): Promise { + /** `unasked`: a read this entry decided to do rather than one a caller asked for, which must + * not answer a conflict on the user's behalf. */ + load(adapter: ItemAdapter, unasked = false): Promise { this.loads++ - return this.run(async (at) => { + return this.run(async () => { const key = this.key let rowsAtRead = 0 let valuesAtRead = 0 @@ -449,9 +466,11 @@ class Entry { // its place, nor answer the conflict on the user's behalf. Checked here rather than // before the read, because the flush that precedes one is itself a way to find out. const standOff = unasked && this.ports.conflicted(key) - // Whatever landed since the read was asked for — an external write, an edit — is - // newer than what it read, so the read only moves the deployed side under it. - const keepValue = standOff || this.edits !== at.edits || this.externals !== at.externals + // A read answers with what the server held when it went out. Anything here that someone + // put here is newer than that, whenever it arrived: an edit, an outside write, a field + // a patch is holding ahead of the server. The read moves the deployed side under it + // and leaves it alone. + const keepValue = standOff || this.local || this.patching // More rows appeared while the read was out than values arrived to account for them, // so one was written by someone this entry cannot see — and it is later than anything // held here. Writing from this would post an older value over that row, on the @@ -501,18 +520,16 @@ class Entry { } this.reconcile() return { ok: true } - }, asOf) + }) } /** Read the item again after someone else wrote it. The edits on screen are in the draft row, - * so they come back over what was written; one typed since the read was asked for stays. */ + * so they come back over what was written; one typed while the flush is out stays, because + * it makes the value local and a read never answers over one of those. */ async reread(): Promise { if (!this.loaded || this.retired || !this.adapter) return { ok: true } - // Taken before the row goes, not after it lands: an edit typed while that write is in - // flight is in neither it nor the read that follows, and is newer than both. - const asOf = this.asOf() await this.ports.flush(this.key) - return this.load(this.adapter, asOf, true) + return this.load(this.adapter, true) } /** Re-read for a caller that has just written the deployed item, after whatever is queued: @@ -583,11 +600,17 @@ class Entry { /** An outside write (the AI chat, another editor): a real divergence, never settling. */ applyExternal(value: V): number { this.externals++ - if (this.loaded && serialize(value) === serialize(this.value)) return this.revision + if (this.loaded && serialize(value) === serialize(this.value)) { + // Nothing to look at changed, but that value is now on the server as a row a read in + // flight has not got, so it is still an intent that read must not answer over. + this.local = true + return this.revision + } this.edits++ this.pristine = false this.removed = false this.replaceValue(value) + this.local = true // Whatever row this produces is counted by `reconcile`; a value that produces none // accounts for nothing, and must not pay for a row some other writer puts there. this.reconcile() @@ -878,6 +901,9 @@ class Entry { // the timestamp the reload adopts — over the version the user just chose. if (how === 'reload') { this.ports.dropPending(this.key) + // Taking the server's version is the user saying their own is spent, so the read that + // follows is not one that has to defer to it: without this it would keep it. + this.local = false return this.load(adapter) } return this.run(async () => {