diff --git a/frontend/src/lib/itemStore.svelte.ts b/frontend/src/lib/itemStore.svelte.ts index 6ee0c7bacc..f8f91839b2 100644 --- a/frontend/src/lib/itemStore.svelte.ts +++ b/frontend/src/lib/itemStore.svelte.ts @@ -175,8 +175,9 @@ class Entry { stopWatch: (() => void) | undefined /** 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). What `reconcile` diffs against. */ - private row: string | null = null + /** The row last handed to the syncer (`null`: none, `undefined`: not known — the next + * reconcile writes whatever the rule says). What `reconcile` diffs against. */ + private row: string | null | undefined = null /** Counts value changes, so a load can tell whether one landed while it was in flight. */ private changes = 0 private queue: Promise = Promise.resolve() @@ -338,7 +339,9 @@ class Entry { const from = this.key // A create or a draft-only item is always a write; anything else already deployed is // saved — which is also what makes a second click behind an in-flight save a no-op. - if (this.origin === 'deployed' && draftValuesEqual(sent, this.deployed)) { + // Compared exactly, not as drafts are: a field the draft comparison ignores (a + // schedule's run-as) is still one an explicit save must send. + if (this.origin === 'deployed' && serialize(sent) === serialize(this.deployed)) { return { ok: true, path: from.path, moved: false } } const to = (adapter.pathOf ?? ((v: V) => (v as { path: string }).path))(sent) @@ -397,52 +400,62 @@ class Entry { await Promise.all(keys.filter((k) => !isTemporaryPath(k.path)).map((k) => this.ports.flush(k))) } - /** Re-key to `path`. The row at the path left behind follows from nothing living there. */ + /** + * 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, + * so it is reconciled like any other. + */ private moveTo(path: string): void { const from = this.key if (!isTemporaryPath(from.path)) { this.ports.hint(from, false) if (this.row !== null) this.ports.write(from, null) } - this.row = null + this.row = undefined this.key = { ...from, path } this.store.rekey(this, from) } /** - * Run `write`, then fold `fields` into the deployed side: the server holds them now. The - * value takes them at once; on failure it gives back those it still holds. + * Write `fields` to the server through `write`. Both sides take them at once — the value + * because the user asked, the baseline because the server is about to hold them — so the + * request never reads as a draft. On failure each side gives back those it still holds. */ patch(fields: Partial, write: (key: ItemKey) => Promise): Promise { this.pristine = false + const baseKey = this.origin === 'new' ? 'template' : 'deployed' + const before = snapshot(this[baseKey]) as Record | undefined + const sent = snapshot(fields) as Record + if (before !== undefined) this[baseKey] = { ...before, ...sent } as V if (this.value !== undefined) Object.assign(this.value as object, fields) this.touch() return this.run(async () => { try { await write(this.key) } catch (e) { - const base = (this.origin === 'new' ? this.template : this.deployed) as - | Record - | undefined - if (this.value !== undefined && base !== undefined) { - const reverted = snapshot(this.value) as Record + const giveBack = (side: V | undefined): V | undefined => { + if (side === undefined || before === undefined) return undefined + const out = snapshot(side) as Record let changed = false - for (const [k, v] of Object.entries(fields)) { - if (deepEqual(reverted[k], v)) { - reverted[k] = base[k] + for (const [k, v] of Object.entries(sent)) { + if (deepEqual(out[k], v)) { + out[k] = before[k] changed = true } } - if (changed) this.replaceValue(reverted as V) + return changed ? (out as V) : undefined } + const base = giveBack(this[baseKey]) + if (base !== undefined) this[baseKey] = base + const value = giveBack(this.value) + if (value !== undefined) this.replaceValue(value) this.error = errorMessage(e) this.reconcile() return { ok: false, error: this.error } } this.error = undefined - if (this.origin === 'deployed' && this.deployed !== undefined) { - this.deployed = { ...this.deployed, ...snapshot(fields) } - } + // 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() return { ok: true } @@ -696,8 +709,10 @@ export function createItemStore(ports: ItemRowPort) { */ function saveEach(items: ItemHandle[]): Promise { const pending = items.map((item) => { - if (!(item instanceof Handle)) throw new Error('saveEach takes the handles useItems returns') - const { entry, adapter } = item as Handle + // `useItem` hands out a forwarder; the item it forwards to is its `current`. + const handle = item instanceof Handle ? item : (item as { current?: unknown }).current + if (!(handle instanceof Handle)) throw new Error('saveEach takes handles from useItem(s)') + const { entry, adapter } = handle as Handle return { entry, adapter, started: entry.begin(), asked: entry.ask() } }) return (async () => { diff --git a/frontend/src/lib/itemStore.test.ts b/frontend/src/lib/itemStore.test.ts index 220183ac82..4b1af8d4d4 100644 --- a/frontend/src/lib/itemStore.test.ts +++ b/frontend/src/lib/itemStore.test.ts @@ -240,12 +240,37 @@ describe('item store: commands', () => { { load: async () => ({ deployed: { path: 's', enabled: true } }) } as ItemAdapter ) await settle() - const outcome = await item.patch({ enabled: false }, async () => { - throw new Error('refused') - }) - expect(outcome).toEqual({ ok: false, error: 'refused' }) - expect(item.value).toEqual({ path: 's', enabled: true }) + const refusal = deferred() + const toggling = item.patch({ enabled: false }, () => refusal.promise) + // In flight the toggle is the server's to hold, not a draft of the user's. expect(item.dirty).toBe(false) + expect(rows.writes).toEqual([]) + + refusal.reject(new Error('refused')) + expect(await toggling).toEqual({ ok: false, error: 'refused' }) + expect(item.value).toEqual({ path: 's', enabled: true }) + expect(item.deployed).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() + const store = createItemStore(rows.port) + const writes: unknown[] = [] + const { handle: item } = store.acquire( + { workspace: 'w', kind: 'trigger_schedule', path: 's' }, + { workspace: 'w', path: 's' }, + { + load: async () => ({ deployed: { path: 's', permissioned_as: 'u/a' } }), + write: async (ctx) => void writes.push(ctx.value) + } as ItemAdapter + ) + await settle() + item.value = { path: 's', permissioned_as: 'u/b' } + expect(item.dirty).toBe(false) + expect(await item.save()).toMatchObject({ ok: true }) + expect(writes).toEqual([{ path: 's', permissioned_as: 'u/b' }]) }) }) @@ -323,7 +348,7 @@ describe('item store: origins', () => { expect(rows.writes).toEqual([{ path: 'u/me/r', value: null }]) }) - it('creates under a temporary path, then moves to the real one with no row at either', async () => { + it('creates under a temporary path, then moves to the real one and clears any row there', async () => { const rows = fakeRows() const temp = newItemPath() const a = adapter({}) @@ -340,12 +365,13 @@ describe('item store: origins', () => { expect(item.key.path).toBe('u/me/created') expect(item.origin).toBe('deployed') expect(item.dirty).toBe(false) - expect(rows.writes).toEqual([]) + // Nothing under the temporary path; a draft left at the real one predates the create. + expect(rows.writes).toEqual([{ path: 'u/me/created', value: null }]) expect(store.bridge.read('w', 'resource', 'u/me/created')).toEqual({ value: undefined }) expect(store.bridge.read('w', 'resource', temp)).toBeUndefined() }) - it('moves a renamed item and deletes the row it left behind', async () => { + it('moves a renamed item and clears the rows at both paths', async () => { const rows = fakeRows() const { item } = await open(rows, adapter({ deployed: deployedRes })) item.value = { ...deployedRes, path: 'u/me/renamed' } @@ -354,8 +380,10 @@ describe('item store: origins', () => { ]) expect(await item.save()).toEqual({ ok: true, path: 'u/me/renamed', moved: true }) - expect(rows.writes.at(-1)).toEqual({ path: 'u/me/r', value: null }) - expect(rows.writes.some((w) => w.path === 'u/me/renamed')).toBe(false) + expect(rows.writes.slice(1)).toEqual([ + { path: 'u/me/r', value: null }, + { path: 'u/me/renamed', value: null } + ]) }) })