From 8b06fee6f3fc1579089a0d2fbb328602da70b4d9 Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Fri, 11 Sep 2026 18:29:19 +0200 Subject: [PATCH] fix: keep refused discards, one writer per key, toggles past saves Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01ViTUkt4czrvZdxdWwxqRAQ --- frontend/src/lib/itemStore.svelte.ts | 65 ++++++++++++++++++--- frontend/src/lib/itemStore.test.ts | 87 +++++++++++++++++++++++++++- 2 files changed, 141 insertions(+), 11 deletions(-) diff --git a/frontend/src/lib/itemStore.svelte.ts b/frontend/src/lib/itemStore.svelte.ts index f8f91839b2..6413b3dcb1 100644 --- a/frontend/src/lib/itemStore.svelte.ts +++ b/frontend/src/lib/itemStore.svelte.ts @@ -172,7 +172,13 @@ class Entry { absorbs: ((next: V, deployed: V) => boolean) | undefined refs = 0 disposed = false + /** Replaced by an entry that moved onto its key: it no longer owns the row there. */ + retired = false + handles = new Set>() stopWatch: (() => void) | undefined + /** Fields a `patch` has put on the deployed side ahead of the server; a save landing + * meanwhile keeps them rather than rolling the baseline back past them. */ + private patched: Record = {} /** The value as last observed, serialized: `touch` acts only on a real change. */ private seen: string | undefined /** The row last handed to the syncer (`null`: none, `undefined`: not known — the next @@ -217,7 +223,7 @@ class Entry { /** Mirror `dirty ? value : null` to the draft row. The only place a row is written. */ reconcile(): void { const key = this.key - if (!this.loaded || this.origin === 'new' || isTemporaryPath(key.path)) return + if (this.retired || !this.loaded || this.origin === 'new' || isTemporaryPath(key.path)) return const desired = this.dirty ? (serialize(this.value) ?? null) : null this.ports.hint(key, desired !== null) if (desired === this.row) return @@ -358,7 +364,7 @@ class Entry { return { ok: false, error: this.error } } this.error = undefined - this.deployed = sent + this.deployed = { ...sent, ...this.patched } as V this.origin = 'deployed' this.template = undefined const moved = to !== from.path @@ -377,9 +383,17 @@ class Entry { return this.run(async () => { if (!this.loaded) return { removed: false } if (this.origin === 'draft') { + const kept = snapshot(this.value) + const keptRow = this.row this.replaceValue(undefined) this.reconcile() await this.settleRows([this.key]) + // The row is the item: while its delete has not landed, the item is still there. + if (this.rowRefused(this.key)) { + this.row = keptRow + this.replaceValue(kept) + return { removed: false } + } this.removed = true return { removed: true } } @@ -400,6 +414,11 @@ class Entry { await Promise.all(keys.filter((k) => !isTemporaryPath(k.path)).map((k) => this.ports.flush(k))) } + /** The syncer settles a rejected or failed write without throwing; this is how to tell. */ + private rowRefused(key: ItemKey): boolean { + return this.ports.conflicted(key) || this.ports.failure(key) !== undefined + } + /** * Re-key to `path`. The row at the path left behind follows from nothing living there. The * one at `path` was never read: whatever it holds predates the write that just landed there, @@ -427,12 +446,19 @@ class Entry { const before = snapshot(this[baseKey]) as Record | undefined const sent = snapshot(fields) as Record if (before !== undefined) this[baseKey] = { ...before, ...sent } as V + if (baseKey === 'deployed') Object.assign(this.patched, sent) if (this.value !== undefined) Object.assign(this.value as object, fields) this.touch() + const settled = () => { + for (const [k, v] of Object.entries(sent)) { + if (deepEqual(this.patched[k], v)) delete this.patched[k] + } + } return this.run(async () => { try { await write(this.key) } catch (e) { + settled() const giveBack = (side: V | undefined): V | undefined => { if (side === undefined || before === undefined) return undefined const out = snapshot(side) as Record @@ -453,8 +479,8 @@ class Entry { this.reconcile() return { ok: false, error: this.error } } + settled() this.error = undefined - // A save that landed while this waited set the baseline to what it sent. if (this[baseKey] !== undefined) this[baseKey] = { ...this[baseKey], ...sent } as V this.touch() this.reconcile() @@ -550,7 +576,8 @@ export type ItemSpec = { /** A class, not a literal: Svelte deep-proxies plain objects put in `$state`, and a proxied * handle would no longer be the one `saveEach` recognizes. */ class Handle implements ItemHandle { - readonly entry: Entry + /** Reactive: a handle follows its item onto the entry that took over its key. */ + entry: Entry = $state.raw()! readonly spec: ItemSpec readonly adapter: ItemAdapter @@ -558,6 +585,7 @@ class Handle implements ItemHandle { this.entry = entry this.spec = spec this.adapter = adapter + entry.handles.add(this) } get key() { @@ -654,10 +682,30 @@ export function createItemStore(ports: ItemRowPort) { rekey(entry, from) { const k = keyString(from) if (entries.get(k) === entry) entries.delete(k) - entries.set(keyString(entry.key), entry) + const to = keyString(entry.key) + const displaced = entries.get(to) + if (displaced && displaced !== entry) retire(displaced, entry) + entries.set(to, entry) } } + /** + * An entry moved onto a key another live entry holds. One key has one row writer, so the + * old one stops writing and every handle on it moves to the entry that now is the item — + * the same outcome as when nobody had it open: what was there is superseded by the write. + */ + function retire(old: Entry, into: Entry): void { + old.retired = true + old.stopWatch?.() + for (const handle of old.handles) { + handle.entry = into + into.handles.add(handle) + into.refs++ + old.refs-- + } + old.handles.clear() + } + function watch(entry: Entry): void { // Detached from whichever component acquired it first: the entry outlives that component // while another holder, or a command in flight, still has it. @@ -691,14 +739,15 @@ export function createItemStore(ports: ItemRowPort) { watch(entry) } entry.refs++ - const held = entry + const handle = new Handle(entry, spec, adapter) let released = false return { - handle: new Handle(held, spec, adapter), + handle, release() { if (released) return released = true - internals.release(held) + handle.entry.handles.delete(handle) + internals.release(handle.entry) } } } diff --git a/frontend/src/lib/itemStore.test.ts b/frontend/src/lib/itemStore.test.ts index 4b1af8d4d4..d9904f629b 100644 --- a/frontend/src/lib/itemStore.test.ts +++ b/frontend/src/lib/itemStore.test.ts @@ -26,9 +26,15 @@ function deferred() { function fakeRows() { const writes: { path: string; value: unknown }[] = [] const conflicts = new Set() + /** Paths whose next write the server fails. */ + const failing = new Set() + const failures = new Map() const hints = new Map() const port: ItemRowPort = { - write: (key, value) => void writes.push({ path: key.path, value }), + write: (key, value) => { + writes.push({ path: key.path, value }) + if (failing.has(key.path)) failures.set(key.path, 'unreachable') + }, flush: async () => {}, overwrite: async (key, value) => { conflicts.delete(key.path) @@ -36,11 +42,11 @@ function fakeRows() { }, seedSync: () => {}, conflicted: (key) => conflicts.has(key.path), - failure: () => undefined, + failure: (key) => failures.get(key.path), dropPending: () => {}, hint: (key, on) => void hints.set(key.path, on) } - return { port, writes, conflicts, hints } + return { port, writes, conflicts, failing, hints } } const deployedRes: Res = { path: 'u/me/r', description: 'deployed', args: { a: 1 } } @@ -193,6 +199,34 @@ describe('item store: commands', () => { expect([first.dirty, second.dirty, second.busy]).toEqual([true, true, false]) }) + it('keeps a toggle made during a save out of the draft row', async () => { + type Sched = { path: string; enabled: boolean; summary: string } + const rows = fakeRows() + const saveGate = deferred() + 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: true, summary: 'a' } }), + write: () => saveGate.promise + } as ItemAdapter + ) + await settle() + item.value = { path: 's', enabled: true, summary: 'b' } + const saving = item.save() + const toggling = item.patch({ enabled: false }, async () => {}) + saveGate.resolve() + await saving + // The save's baseline predates the toggle; the toggle's field survives it, so nothing + // is left unsaved and the row goes. + expect(item.dirty).toBe(false) + expect(rows.writes.at(-1)).toEqual({ path: 's', value: null }) + await toggling + expect(item.deployed).toEqual({ path: 's', enabled: false, summary: 'b' }) + expect(rows.writes.at(-1)).toEqual({ path: 's', value: null }) + }) + it('holds a toggle behind a save, and keeps an unrelated edit through the toggle', async () => { type Sched = { path: string; enabled: boolean; summary: string } const rows = fakeRows() @@ -348,6 +382,18 @@ describe('item store: origins', () => { expect(rows.writes).toEqual([{ path: 'u/me/r', value: null }]) }) + it('keeps a draft-only item whose delete did not land', async () => { + const rows = fakeRows() + const draft = { ...deployedRes, description: 'only a draft' } + const { item } = await open(rows, adapter({ draft })) + rows.failing.add('u/me/r') + + expect(await item.discard()).toEqual({ removed: false }) + expect(item.removed).toBe(false) + expect(item.value).toEqual(draft) + expect(item.status).toBe('failed') + }) + it('creates under a temporary path, then moves to the real one and clears any row there', async () => { const rows = fakeRows() const temp = newItemPath() @@ -387,6 +433,41 @@ describe('item store: origins', () => { }) }) +describe('item store: one entry per key', () => { + it('moves onto a key another editor holds, which then follows the item there', async () => { + const rows = fakeRows() + const store = createItemStore(rows.port) + const at = (path: string, load: ItemLoad) => + store.acquire( + { workspace: 'w', kind: 'resource', path }, + { workspace: 'w', path }, + adapter(load) + ).handle + const other = at('u/me/b', { + draft: { ...deployedRes, path: 'u/me/b', description: 'draft b' } + }) + const moving = at('u/me/r', { deployed: deployedRes }) + await settle() + + moving.value = { ...deployedRes, path: 'u/me/b' } + expect(await moving.save()).toMatchObject({ ok: true, moved: true }) + + expect(other.key.path).toBe('u/me/b') + expect(other.value).toEqual({ ...deployedRes, path: 'u/me/b' }) + expect(other.origin).toBe('deployed') + // The displaced entry writes nothing more: `u/me/b` has one writer. + const before = rows.writes.length + other.value = { ...deployedRes, path: 'u/me/b', description: 'typed in the other editor' } + expect(moving.value?.description).toBe('typed in the other editor') + expect(rows.writes.slice(before)).toEqual([ + { + path: 'u/me/b', + value: { ...deployedRes, path: 'u/me/b', description: 'typed in the other editor' } + } + ]) + }) +}) + describe('item store: conflicts', () => { it('stops writing a key the server rejected, until the conflict is resolved', async () => { const rows = fakeRows()