From 11b43d5e06e7c0cbda5a4be8ed3b4bd3a0c034c1 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 11 Jul 2026 12:38:30 +0200 Subject: [PATCH] fix(spa): reload item draft when a save flips shuffleScheduleItems (#230 finding 2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Editing a schedule's properties to change shuffleScheduleItems for the active schedule left the open items draft stale: hidden Fixed/Flood start values could be saved back and the inspector kept offering start-type controls the schedule no longer supports. onScheduleSaved now detects a shuffle flip on the active schedule and reloads the items via GET so the server's EnforceProperties re-normalizes the draft. The schedule-level flags already refresh from the save response (setBoot maps `saved` into the list). Decision: chose "block opening the edit dialog while the item draft is dirty" (confirm-to-discard, guardedSwitch semantics) over confirm-at-reload — the smaller fully-consistent change. It guarantees the properties editor only ever opens over a clean baseline draft, so the post-save reload is lossless and avoids the awkward state where a cancelled discard leaves a now-shuffled schedule holding Fixed values. Regression: SchedulesScreen.test.tsx — Fixed item on a non-shuffled schedule → edit properties to shuffle=true → a fresh items GET fires, the Fixed option is gone, and a subsequent Save's PUT carries no Fixed startType. Co-Authored-By: Claude Fable 5 --- web/src/screens/SchedulesScreen.test.tsx | 51 ++++++++++++++++++++++++ web/src/screens/SchedulesScreen.tsx | 35 +++++++++++++++- 2 files changed, 85 insertions(+), 1 deletion(-) diff --git a/web/src/screens/SchedulesScreen.test.tsx b/web/src/screens/SchedulesScreen.test.tsx index cf20c87d1..c6513e0fc 100644 --- a/web/src/screens/SchedulesScreen.test.tsx +++ b/web/src/screens/SchedulesScreen.test.tsx @@ -418,3 +418,54 @@ describe('SchedulesScreen — conditional rendering', () => { expect(values).toEqual(['Dynamic']); }); }); + +describe('SchedulesScreen — shuffle toggle re-normalizes the draft (#230 finding 2)', () => { + it('reloads the item draft when a properties save flips shuffleScheduleItems', async () => { + let shuffled = false; + const handle = mockApi({ + items: [responseItem({ startType: 'Fixed', startTime: '20:00:00' })], + onRequest: (url, method, body) => { + if (url === '/api/schedules/1' && method === 'PUT') { + shuffled = (body as { shuffleScheduleItems: boolean }).shuffleScheduleItems; + return jsonResponse({ ...schedule, shuffleScheduleItems: shuffled }); + } + // Server EnforceProperties strips Fixed → Dynamic on GET once the schedule shuffles. + if (url === '/api/schedules/1/items' && method === 'GET' && shuffled) { + return jsonResponse({ items: [responseItem({ startType: 'Dynamic', startTime: null })], totalDurationEstimate: '00:30:00' }); + } + return null; + } + }); + render(); + await waitFor(() => expect(screen.getByLabelText('Schedule lineup').querySelectorAll('[role="listitem"]').length).toBeGreaterThan(0)); + + // Precondition: the inspector offers Fixed on the non-shuffled schedule. + const before = screen.getByLabelText('Start type') as HTMLSelectElement; + expect(Array.from(before.options).map((o) => o.value)).toContain('Fixed'); + + const getsBefore = handle.requests.filter((r) => r.url === '/api/schedules/1/items' && r.method === 'GET').length; + + // Edit properties → enable shuffle → Save. + fireEvent.click(screen.getByRole('button', { name: /^Edit$/ })); + const dialog = screen.getByRole('dialog'); + fireEvent.click(within(dialog).getByLabelText('Shuffle schedule items')); + fireEvent.click(within(dialog).getByRole('button', { name: /^Save$/ })); + + // A fresh items GET re-normalizes the draft after the shuffle flip. + await waitFor(() => + expect(handle.requests.filter((r) => r.url === '/api/schedules/1/items' && r.method === 'GET').length).toBe(getsBefore + 1) + ); + + // The Fixed option is gone and the reloaded draft carries the normalized Dynamic value. + await waitFor(() => { + const after = screen.getByLabelText('Start type') as HTMLSelectElement; + expect(Array.from(after.options).map((o) => o.value)).toEqual(['Dynamic']); + }); + + // A subsequent Save can no longer carry Fixed (the reloaded draft was normalized server-side). + fireEvent.click(screen.getByRole('button', { name: /Add item/ })); + fireEvent.click(screen.getByRole('button', { name: /^Save$/ })); + await waitFor(() => expect(handle.requests.some((r) => r.url === '/api/schedules/1/items' && r.method === 'PUT')).toBe(true)); + expect(lastPut(handle).every((it) => it.startType !== 'Fixed')).toBe(true); + }); +}); diff --git a/web/src/screens/SchedulesScreen.tsx b/web/src/screens/SchedulesScreen.tsx index a4cd4a9e4..2e4472f96 100644 --- a/web/src/screens/SchedulesScreen.tsx +++ b/web/src/screens/SchedulesScreen.tsx @@ -342,8 +342,32 @@ export function SchedulesScreen() { }); }; + // Opening the properties editor is blocked while the item draft is dirty (confirm-to-discard, same + // semantics as guardedSwitch). This is the smaller, fully-consistent fix for the shuffle-toggle + // normalization gap (#230 finding 2): if editing shuffleScheduleItems could change under a dirty + // draft, a post-save reload would either lose the draft or (on a cancelled discard) leave a + // now-shuffled schedule holding Fixed/Flood values the schedule no longer supports. Guaranteeing + // the form only ever opens over a clean (baseline) draft makes the post-save reload lossless. + const openEditForm = () => { + if (dirtyRef.current && !window.confirm(DIRTY_PROMPT)) { + return; + } + if (dirtyRef.current) { + const restored = baselineRef.current; + setItems(restored); + setSelectedKey(restored[0]?._key ?? null); + setDirtyState(false); + setMutationError(null); + } + setForm('edit'); + }; + const onScheduleSaved = (saved: ProgramSchedule) => { setForm(null); + // Did the ACTIVE schedule's shuffle flag flip? Capture BEFORE setBoot rewrites the list. + const editedActive = saved.id === activeId; + const shuffleChanged = + editedActive && (activeSchedule?.shuffleScheduleItems ?? false) !== (saved.shuffleScheduleItems ?? false); setBoot((current) => { if (current.status !== 'ready') { return current; @@ -358,6 +382,15 @@ export function SchedulesScreen() { // New schedule: switch focus to it, but through the same dirty guard as the schedule picker so // an unsaved draft on the current schedule isn't silently wiped without a confirm (F5). guardedSwitch(saved.id); + } else if (shuffleChanged && activeId != null) { + // Toggling shuffleScheduleItems re-normalizes the lineup server-side (EnforceProperties strips + // hidden Fixed/Flood start values on GET). The open draft was built from the pre-toggle GET, so + // reload it — otherwise those hidden values could be saved back and the inspector would keep + // offering start-type controls the schedule no longer supports (#230 finding 2). The draft is + // guaranteed clean here (openEditForm blocks opening the editor while dirty), so this is lossless. + setItemsLoaded(false); + setItemsLoading(true); + loadItems(activeId); } }; @@ -416,7 +449,7 @@ export function SchedulesScreen() { {activeSchedule && ( <> - + )}