From 6371a34b5d9d7e83ba30f55fda8a010c9dba52aa Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Mon, 14 Sep 2026 15:59:54 +0200 Subject: [PATCH] fix: record the enabled state a schedule write leaves, not the one sent Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_015vn1jCHUcAdaG9C4NsUehF --- .../schedules/ScheduleEditorInner.svelte | 12 +++--- .../triggers/schedules/scheduleCfg.test.ts | 21 +++++----- .../triggers/schedules/scheduleCfg.ts | 20 ++++++---- frontend/src/lib/itemStore.svelte.ts | 38 +++++++++++++++---- frontend/src/lib/itemStore.test.ts | 29 ++++++++++++++ 5 files changed, 88 insertions(+), 32 deletions(-) diff --git a/frontend/src/lib/components/triggers/schedules/ScheduleEditorInner.svelte b/frontend/src/lib/components/triggers/schedules/ScheduleEditorInner.svelte index 87865767d5..3e3c05fb45 100644 --- a/frontend/src/lib/components/triggers/schedules/ScheduleEditorInner.svelte +++ b/frontend/src/lib/components/triggers/schedules/ScheduleEditorInner.svelte @@ -55,9 +55,9 @@ import { onUserInput } from '$lib/userDraftEditGate' import { isTemporaryPath, newItemPath, useItem, type ItemAdapter } from '$lib/itemStore.svelte' import { - draftOnlyScheduleCfg, newScheduleCfg, normalizeScheduleCfg, + scheduleCfgAfterWrite, scheduleCfgOf, scheduleFormOf, type NewScheduleOptions, @@ -198,7 +198,7 @@ : undefined return { deployed: no_deployed ? undefined : normalizeScheduleCfg(deployedSchedule), - draft: no_deployed && loadedDraft ? draftOnlyScheduleCfg(loadedDraft) : loadedDraft, + draft: loadedDraft, draftSavedAt: draft_saved_at } } catch (err) { @@ -207,11 +207,9 @@ } }, async write({ workspace, path, value, deployed }) { - await writeScheduleCfg( - value, - deployed !== undefined || standsForDeployed.has(path), - workspace - ) + const update = deployed !== undefined || standsForDeployed.has(path) + await writeScheduleCfg(value, update, workspace) + return scheduleCfgAfterWrite(value, update, deployed) } } diff --git a/frontend/src/lib/components/triggers/schedules/scheduleCfg.test.ts b/frontend/src/lib/components/triggers/schedules/scheduleCfg.test.ts index 36ce2cefaf..a0b438c670 100644 --- a/frontend/src/lib/components/triggers/schedules/scheduleCfg.test.ts +++ b/frontend/src/lib/components/triggers/schedules/scheduleCfg.test.ts @@ -1,7 +1,7 @@ import { describe, it, expect } from 'vitest' import { - draftOnlyScheduleCfg, normalizeScheduleCfg, + scheduleCfgAfterWrite, scheduleCfgOf, scheduleFormOf } from './scheduleCfg' @@ -43,15 +43,18 @@ describe('schedule config normalization', () => { expect(once).not.toHaveProperty('edited_by') }) - // Deploying a draft-only schedule creates it, and a create always enables it. A draft that - // kept its stored state would show a disabled schedule the server has enabled, and later - // updates omit the field, so nothing would ever correct it. - it('records a draft-only schedule as enabled, however it was stored', () => { - expect( - draftOnlyScheduleCfg(normalizeScheduleCfg({ ...deployed, enabled: false })) - ).toMatchObject({ enabled: true }) + // Neither endpoint takes `enabled` from the config, and the item records this as its deployed + // side: a disabled config recorded as sent would show a running schedule as disabled for good. + it('records the enabled state a write leaves on the server, not the one sent', () => { + const disabled = normalizeScheduleCfg({ ...deployed, enabled: false }) + const created = scheduleCfgAfterWrite(disabled, false, undefined) + // Serialized like the form's own config, since saves compare them exactly. + expect(JSON.stringify(created)).toBe(JSON.stringify(normalizeScheduleCfg(deployed))) const { enabled: _dropped, ...withoutEnabled } = deployed - expect(draftOnlyScheduleCfg(normalizeScheduleCfg(withoutEnabled))).toMatchObject({ + expect( + scheduleCfgAfterWrite(normalizeScheduleCfg(withoutEnabled), false, undefined) + ).toMatchObject({ enabled: true }) + expect(scheduleCfgAfterWrite(disabled, true, normalizeScheduleCfg(deployed))).toMatchObject({ enabled: true }) }) diff --git a/frontend/src/lib/components/triggers/schedules/scheduleCfg.ts b/frontend/src/lib/components/triggers/schedules/scheduleCfg.ts index 6645ac7295..be96c9405d 100644 --- a/frontend/src/lib/components/triggers/schedules/scheduleCfg.ts +++ b/frontend/src/lib/components/triggers/schedules/scheduleCfg.ts @@ -174,13 +174,18 @@ export function normalizeScheduleCfg(raw: ScheduleCfg): ScheduleCfg { } /** - * A schedule that exists only as a draft, as deploying it will leave it: deploying one creates - * it, and a create always enables it (`writeScheduleCfg`), so the draft has to carry the state - * that create produces. Left as stored, a disabled draft would record `enabled: false` against - * a server that says otherwise, and later updates omit the field, so nothing corrects it. + * What the server holds once `writeScheduleCfg` has written `cfg`. Neither endpoint takes + * `enabled` from it (a create enables the schedule, an update leaves the field as it was), and + * a later update cannot correct it, so recording what was sent could show a running schedule as + * disabled for good. `deployed` is the schedule an update was measured against, if known. */ -export function draftOnlyScheduleCfg(cfg: ScheduleCfg): ScheduleCfg { - return { ...cfg, enabled: true } +export function scheduleCfgAfterWrite( + cfg: ScheduleCfg, + update: boolean, + deployed: ScheduleCfg | undefined +): ScheduleCfg { + const enabled = !update ? true : deployed ? deployed.enabled : cfg.enabled + return normalizeScheduleCfg({ ...cfg, enabled }) } /** The handlers a new schedule starts with, from the workspace's defaults. */ @@ -263,8 +268,7 @@ export async function newScheduleCfg(opts: NewScheduleOptions): Promise = ItemKey & { export type ItemAdapter = { load?: (key: ItemKey) => Promise> - write?: (ctx: ItemWriteContext) => Promise + /** Resolves to what the server holds after the write, when that is not `value`: an endpoint + * that sets a field on its own (a schedule create enabling it) would otherwise be recorded + * as holding what was sent. */ + write?: (ctx: ItemWriteContext) => Promise /** Where a save puts `value`. Defaults to its `path` field. */ pathOf?: (value: V) => string /** @@ -351,20 +354,23 @@ class Entry { return { ok: true, path: from.path, moved: false } } const to = (adapter.pathOf ?? ((v: V) => (v as { path: string }).path))(sent) + let held: V try { if (!adapter.write) throw new Error('This item cannot be saved') - await adapter.write({ - ...from, - value: sent, - deployed: this.origin === 'deployed' ? snapshot(this.deployed) : undefined, - meta: this.meta - }) + held = + (await adapter.write({ + ...from, + value: sent, + deployed: this.origin === 'deployed' ? snapshot(this.deployed) : undefined, + meta: this.meta + })) ?? sent } catch (e) { this.error = errorMessage(e) return { ok: false, error: this.error } } this.error = undefined - this.deployed = { ...sent, ...this.patched } as V + if (held !== sent) this.adopt(sent, held) + this.deployed = { ...held, ...this.patched } as V this.origin = 'deployed' this.template = undefined const moved = to !== from.path @@ -379,6 +385,22 @@ class Entry { return result } + /** Give the value each field the server set otherwise than `sent` asked, unless the user has + * changed that field since: left as sent, it would read as a draft of a change nobody made. */ + private adopt(sent: V, held: V): void { + if (this.value === undefined) return + const s = sent as Record + const h = held as Record + const next = snapshot(this.value) as Record + let changed = false + for (const k of new Set([...Object.keys(s), ...Object.keys(h)])) { + if (deepEqual(s[k], h[k]) || !deepEqual(next[k], s[k])) continue + next[k] = h[k] + changed = true + } + if (changed) this.replaceValue(next as V) + } + discard(): Promise { return this.run(async () => { if (!this.loaded) return { removed: false } diff --git a/frontend/src/lib/itemStore.test.ts b/frontend/src/lib/itemStore.test.ts index 166e146011..733350210c 100644 --- a/frontend/src/lib/itemStore.test.ts +++ b/frontend/src/lib/itemStore.test.ts @@ -308,6 +308,35 @@ describe('item store: commands', () => { expect(await item.save()).toMatchObject({ ok: true }) expect(writes).toEqual([{ path: 's', permissioned_as: 'u/b' }]) }) + + it('records what a write left on the server, and gives the value the fields it set', async () => { + type Sched = { path: string; enabled: boolean; summary: string } + const rows = fakeRows() + const store = createItemStore(rows.port) + const gate = deferred() + const { handle: item } = store.acquire( + { workspace: 'w', kind: 'trigger_schedule', path: 's' }, + { workspace: 'w', path: 's' }, + { + load: async () => ({ draft: { path: 's', enabled: false, summary: '' } }), + write: async (ctx) => { + await gate.promise + return { ...ctx.value, enabled: true } + } + } as ItemAdapter + ) + await settle() + + const saving = item.save() + item.value = { path: 's', enabled: false, summary: 'typed during the save' } + gate.resolve() + expect(await saving).toMatchObject({ ok: true }) + + const kept = { path: 's', enabled: true, summary: 'typed during the save' } + expect(item.deployed).toEqual({ path: 's', enabled: true, summary: '' }) + expect(item.value).toEqual(kept) + expect(rows.writes.at(-1)).toEqual({ path: 's', value: kept }) + }) }) describe('item store: outside writes', () => {