From d3d50f60c4a3c2a5d2f9f8562e42830c031870d6 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 11 Jul 2026 12:38:10 +0200 Subject: [PATCH 1/5] fix(spa): bare-channel create reachable on an empty lineup (#212 reopened) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ChannelsScreen returned ChannelsEmptyState before the action bar that owns "New blank channel", so a fresh install could never create its first channel. The empty state now offers both create paths (bare-create + ChannelBuilder), reusing the exact createBlankChannel handler (number = max+1 → 1 on empty, group "ErsatzTV", default ffmpeg profile). Regression: App.test.tsx bare-creates from a [] lineup, asserts the POST payload (number "1") + navigation to /app/edit-channel/{id}. Co-Authored-By: Claude Fable 5 --- web/src/App.test.tsx | 77 +++++++++++++++++++++++++++++++++++++++++++- web/src/App.tsx | 60 +++++++++++++++++++++++++++++++--- web/src/shell.css | 15 +++++++++ 3 files changed, 146 insertions(+), 6 deletions(-) diff --git a/web/src/App.test.tsx b/web/src/App.test.tsx index e9d6289d4..c6a89fa49 100644 --- a/web/src/App.test.tsx +++ b/web/src/App.test.tsx @@ -42,7 +42,7 @@ describe('ChicoryTV SPA scaffold', () => { fireEvent.click(screen.getByRole('link', { name: 'Channels' })); expect(screen.getByRole('heading', { name: 'Channels' })).toBeInTheDocument(); - expect(await screen.findByText('No channels returned')).toBeInTheDocument(); + expect(await screen.findByText('No channels yet')).toBeInTheDocument(); expect(screen.getByRole('link', { name: 'Channels' })).toHaveAttribute( 'aria-current', 'page' @@ -849,6 +849,41 @@ describe('ChicoryTV SPA scaffold', () => { expect(window.location.pathname).toBe('/app/channels'); }); + it('bare-creates the first channel from an empty lineup (#212 regression)', async () => { + // Fresh install: GET /api/channels returns []. The empty state must still offer bare-create — the + // old early return rendered a dead-end card before the action bar, so no channel could be created. + mockDashboardApi({ + channels: [], + createChannelResponse: { id: 7 }, + ffmpegSettings: defaultFfmpegSettings({ defaultFFmpegProfileId: 3 }) + }); + + render(); + + fireEvent.click(screen.getByRole('link', { name: 'Channels' })); + expect(await screen.findByText('No channels yet')).toBeInTheDocument(); + + // Both create affordances are reachable on an empty lineup (the empty-state "Add Channel" is in + // addition to the always-present TopBar one, so there are two). + expect(screen.getAllByRole('button', { name: 'Add Channel' }).length).toBeGreaterThanOrEqual(2); + fireEvent.click(screen.getByRole('button', { name: 'New blank channel' })); + + await waitFor(() => + expect(window.fetch).toHaveBeenCalledWith('/api/channels', expect.objectContaining({ method: 'POST' })) + ); + + const body = requestBodyFor('/api/channels'); + expect(body).toMatchObject({ + ffmpegProfileId: 3, + group: 'ErsatzTV', + name: 'New Channel', + // max(number)+1 degenerates to 1 on an empty lineup. + number: '1' + }); + + await waitFor(() => expect(window.location.pathname).toBe('/app/edit-channel/7')); + }); + it('clears the selection when the channel view filter changes', async () => { mockDashboardApi({ channels: [ @@ -924,6 +959,46 @@ describe('ChicoryTV SPA scaffold', () => { expect(window.fetch).toHaveBeenCalledWith('/api/languages', expect.any(Object)); }); + it('honors the unsaved-changes guard on browser Back/popstate (#230 finding 1)', async () => { + mockDashboardApi({ + scheduleItems: [scheduleItem({ id: 11, name: 'Saturday Cartoons', collectionName: 'Saturday Cartoons' })], + schedules: [schedule({ id: 5, name: 'Prime Time Cartoons' })] + }); + + render(); + + fireEvent.click(screen.getByRole('link', { name: 'Schedules' })); + expect(await screen.findByRole('heading', { name: 'Prime Time Cartoons' })).toBeInTheDocument(); + await screen.findByRole('list', { name: 'Schedule lineup' }); + + // Dirty the item draft so the registered navigation guard is armed. + fireEvent.click(screen.getByRole('button', { name: 'Add item' })); + expect(await screen.findByText(/unsaved changes/)).toBeInTheDocument(); + expect(window.location.pathname).toBe('/app/schedules'); + + // Simulate browser Back: popstate fires AFTER the URL has already moved. The guard cannot cancel + // it, so on a vetoed confirm the App handler must re-push the pre-pop path and keep the route. + const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(false); + window.history.pushState(null, '', '/app/guide'); + window.dispatchEvent(new PopStateEvent('popstate')); + + expect(confirmSpy).toHaveBeenCalled(); + expect(screen.getByRole('heading', { name: 'Prime Time Cartoons' })).toBeInTheDocument(); + expect(window.location.pathname).toBe('/app/schedules'); + + // Accept the discard on the next Back → route changes and the schedules screen (with its draft) + // unmounts, matching the sidebar-nav case. + confirmSpy.mockReturnValue(true); + window.history.pushState(null, '', '/app/guide'); + window.dispatchEvent(new PopStateEvent('popstate')); + + expect(await screen.findByRole('heading', { name: 'Guide' })).toBeInTheDocument(); + expect(window.location.pathname).toBe('/app/guide'); + expect(screen.queryByRole('heading', { name: 'Prime Time Cartoons' })).not.toBeInTheDocument(); + + confirmSpy.mockRestore(); + }); + it('opens the create-schedule dialog from the TopBar Add Schedule button', async () => { mockDashboardApi({ schedules: [schedule({ id: 5, name: 'Prime Time Cartoons' })] }); diff --git a/web/src/App.tsx b/web/src/App.tsx index f8f5f7fb4..9fce3a50f 100644 --- a/web/src/App.tsx +++ b/web/src/App.tsx @@ -1255,10 +1255,41 @@ function ChannelsErrorState({ error, refresh }: { error: string; refresh: () => ); } -function ChannelsEmptyState() { +// Empty lineup (fresh install) must still let the user create their first channel — both the +// library-to-lineup ChannelBuilder flow ("Add Channel") and the bare-create flow ("New blank +// channel"). The bare-create handler is passed in from ChannelsScreen so the defaults (number = +// max+1, which degenerates to 1 on an empty lineup; group "ErsatzTV"; default ffmpeg profile) are +// identical to the action bar's. Regression for #212 (reopened): the old early return rendered a +// dead-end card before the action bar, so a fresh install could never create a channel. +function ChannelsEmptyState({ + creatingBlank, + onCreateBlank +}: { + creatingBlank: boolean; + onCreateBlank: () => void; +}) { return ( - No channels} subtitle="The API returned an empty channel lineup."> -
No channels returned
+ No channels} subtitle="No channels yet — create your first one."> +
+

No channels yet

+
+ + +
+
); } @@ -1458,7 +1489,7 @@ function ChannelsScreen() { }; if (channels.length === 0) { - return ; + return void createBlankChannel()} />; } return ( @@ -3442,12 +3473,30 @@ export function App() { const [activeRoute, setActiveRoute] = useState(() => routeFromLocation()); const healthState = useDashboardHealthQuery(); + // The last path the guard has approved us being on. Browser Back/Forward (popstate) can't be + // cancelled, so when the dirty guard vetoes a pop we re-push THIS path to undo the browser's URL + // change. Kept in a ref (not state) so the popstate handler reads it synchronously at event time. + const currentPathRef = useRef(window.location.pathname); + useEffect(() => { applyDesignSystemTheme(theme); }, [theme]); useEffect(() => { - const onPopState = () => setActiveRoute(routeFromLocation()); + // popstate covers both real Back/Forward and the synthetic pop navigateToPath() dispatches. If a + // screen has registered a dirty guard and it vetoes (user cancelled the confirm), we cannot stop + // the navigation — the URL has already moved — so we restore it by re-pushing the pre-pop path + // and leave activeRoute untouched. Re-pushing does NOT fire popstate, but that's fine: only one + // screen is mounted at a time and the guard-registering screen (schedules) owns no internal + // popstate listener, so no sub-path screen's pathname state can desync (see spa-conventions §8). + const onPopState = () => { + if (!canLeaveCurrentScreen()) { + window.history.pushState(null, '', currentPathRef.current); + return; + } + currentPathRef.current = window.location.pathname; + setActiveRoute(routeFromLocation()); + }; window.addEventListener('popstate', onPopState); @@ -3474,6 +3523,7 @@ export function App() { } window.history.pushState(null, '', routeHref(nextRoute)); + currentPathRef.current = window.location.pathname; setActiveRoute(nextRoute); }; diff --git a/web/src/shell.css b/web/src/shell.css index 3dc15264c..2aa669fc0 100644 --- a/web/src/shell.css +++ b/web/src/shell.css @@ -1275,6 +1275,21 @@ body { font-size: var(--text-sm, 13px); } +.ctv-channels-empty { + display: flex; + flex-direction: column; + align-items: center; + gap: 16px; + padding: 16px 0; +} + +.ctv-channels-empty-actions { + display: flex; + gap: 10px; + flex-wrap: wrap; + justify-content: center; +} + .ctv-schedule-inspector { display: flex; flex-direction: column; -- 2.47.3 From f09045e135c6c77553548e4d05e1f0a17f354da6 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 11 Jul 2026 12:38:19 +0200 Subject: [PATCH 2/5] fix(spa): dirty guard covers browser Back/Forward (popstate) (#230 finding 1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit canLeaveCurrentScreen() was only consulted in App's navigate() (sidebar/nav clicks); browser Back/Forward switched screens unguarded. A popstate can't be cancelled, so App's popstate handler now, on a vetoed guard, re-pushes the pre-pop path (tracked in currentPathRef, updated on every approved navigation) and leaves activeRoute untouched — undoing the browser's URL change. The same handler covers the synthetic pop navigateToPath() dispatches. Re-pushing is safe: only one screen is mounted at a time and the guard-registering screen (schedules) owns no internal popstate listener, so no sub-path screen's pathname state can desync. Effect cleanup keeps StrictMode double-mount from double-registering. Docs: spa-conventions §8 rewritten from navigate-only to describe popstate coverage. Regression: App.test.tsx dirties the schedules draft, simulates popstate → confirm called; cancel keeps route + re-pushes path; accept switches route and unmounts the draft. Co-Authored-By: Claude Fable 5 --- docs/spa-conventions.md | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/docs/spa-conventions.md b/docs/spa-conventions.md index d95b89efa..a847ad923 100644 --- a/docs/spa-conventions.md +++ b/docs/spa-conventions.md @@ -211,11 +211,22 @@ draft to navigation. The shared module `web/src/navigationGuard.ts` is the seam: window.confirm(...))`, returning the unregister fn from the mount `useEffect` for cleanup. Only one guard is active at a time (the mounted screen); a stale unregister only clears its own guard. - `App.tsx`'s `navigate` handler calls `canLeaveCurrentScreen()` before `pushState` — a `false` - return aborts the in-app sidebar/nav click. This covers React-router-less in-app navigation only. -- The screen **also** guards the three paths the module can't see — a **schedule switch** and any - in-screen action that would replace the draft use their own `window.confirm(...)`, and a - `beforeunload` listener (installed while `dirty`) covers full-page unloads (reload / close tab / - external link). + return aborts the in-app sidebar/nav click. +- **Browser Back/Forward (`popstate`) is also covered.** `App.tsx`'s `popstate` handler consults + `canLeaveCurrentScreen()` too. A `popstate` cannot be cancelled — by the time it fires the URL has + already changed — so on a veto the handler **re-pushes the pre-pop path** (tracked in a + `currentPathRef` updated on every approved navigation) via `history.pushState` and leaves + `activeRoute` untouched, effectively undoing the browser's URL change. This same handler also runs + for the **synthetic** pop `navigateToPath()` dispatches, so programmatic in-app navigation is + guarded as well. The re-push does **not** re-fire `popstate`; that's safe because only one screen + is mounted at a time and the guard-registering screen (the schedules editor) owns no internal + `popstate`/sub-path listener — so a blocked pop cannot desync a sub-path screen's own pathname + state (§2). A sub-path screen that both registers a dirty guard *and* tracks its own pathname would + need to re-validate this; none does today. +- The screen **also** guards the paths the module still can't see — an in-screen action that would + replace the draft (schedule switch, opening the properties editor) uses its own + `window.confirm(...)`, and a `beforeunload` listener (installed while `dirty`) covers full-page + unloads (reload / close tab / external link). Keep the guard predicate reading a **ref** (`dirtyRef`), not the `dirty` state value, so `canLeaveCurrentScreen()` sees the current dirtiness synchronously at click time. -- 2.47.3 From 11b43d5e06e7c0cbda5a4be8ed3b4bd3a0c034c1 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 11 Jul 2026 12:38:30 +0200 Subject: [PATCH 3/5] 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 && ( <> - + )} -- 2.47.3 From 6f6fcf6c634e69903947d6666076e2cbffce75ee Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 11 Jul 2026 12:55:07 +0200 Subject: [PATCH 4/5] =?UTF-8?q?fix(spa):=20review=20follow-ups=20=E2=80=94?= =?UTF-8?q?=20surface=20empty-state=20create=20error,=20gate=20Edit=20whil?= =?UTF-8?q?e=20saving?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings from the PR #242 adversarial review (Codex + Claude fork): 1. ChannelsEmptyState swallowed mutationError: on the fresh-install path #212 targets, a bare-create can 4xx (e.g. no default ffmpeg profile), but the empty branch never rendered the error alert the non-empty screen shows — the user saw only a spinner re-enable. The empty state now renders the same ctv-channels-error alert. Regression: App.test.tsx asserts the error surfaces + no navigation on an empty lineup. 2. SchedulesScreen Edit button was not gated on `saving` (Codex): during an items save PUT the draft is still dirty, so a discard-to-open → shuffle-flip could let the in-flight PUT resolve AFTER the shuffle reload and clobber the normalized draft with the pre-shuffle body. Edit is now disabled while saving (consistent with the schedule Select). Regression: a deferred PUT proves Edit is disabled in-flight and re-enables once the save settles. Deferred as nits (both reviews rate low): vetoed navigateToPath leaves two stray history entries (cosmetic, rare — fixing means refactoring central nav); rapid double-Back is best-effort (inherent popstate non-cancellability). Co-Authored-By: Claude Fable 5 --- web/src/App.test.tsx | 23 +++++++++++++++ web/src/App.tsx | 20 ++++++++++++- web/src/screens/SchedulesScreen.test.tsx | 36 ++++++++++++++++++++++++ web/src/screens/SchedulesScreen.tsx | 7 ++++- 4 files changed, 84 insertions(+), 2 deletions(-) diff --git a/web/src/App.test.tsx b/web/src/App.test.tsx index c6a89fa49..14310cc32 100644 --- a/web/src/App.test.tsx +++ b/web/src/App.test.tsx @@ -884,6 +884,29 @@ describe('ChicoryTV SPA scaffold', () => { await waitFor(() => expect(window.location.pathname).toBe('/app/edit-channel/7')); }); + it('surfaces a bare-create failure on an empty lineup (#242 review follow-up)', async () => { + // The fresh-install path #212 exists for: a new install may have no default ffmpeg profile, so + // bare-create can 4xx. The empty state must render that error, not swallow it behind a spinner + // that just re-enables. (Regression for the empty branch dropping mutationError.) + mockDashboardApi({ + channels: [], + createChannelFailure: { detail: 'No FFmpeg profile is configured', status: 422, title: 'Validation failed' }, + ffmpegSettings: defaultFfmpegSettings({ defaultFFmpegProfileId: 0 }) + }); + + render(); + + fireEvent.click(screen.getByRole('link', { name: 'Channels' })); + expect(await screen.findByText('No channels yet')).toBeInTheDocument(); + + fireEvent.click(screen.getByRole('button', { name: 'New blank channel' })); + + expect(await screen.findByText('No FFmpeg profile is configured')).toBeInTheDocument(); + // Still on the empty lineup — no navigation, and the create affordances remain reachable. + expect(window.location.pathname).toBe('/app/channels'); + expect(screen.getByRole('button', { name: 'New blank channel' })).toBeInTheDocument(); + }); + it('clears the selection when the channel view filter changes', async () => { mockDashboardApi({ channels: [ diff --git a/web/src/App.tsx b/web/src/App.tsx index 9fce3a50f..95a1fb178 100644 --- a/web/src/App.tsx +++ b/web/src/App.tsx @@ -1261,17 +1261,29 @@ function ChannelsErrorState({ error, refresh }: { error: string; refresh: () => // max+1, which degenerates to 1 on an empty lineup; group "ErsatzTV"; default ffmpeg profile) are // identical to the action bar's. Regression for #212 (reopened): the old early return rendered a // dead-end card before the action bar, so a fresh install could never create a channel. +// mutationError is rendered here too: a fresh install may have no default ffmpeg profile, so +// bare-create can 4xx — the non-empty screen shows that alert, and this branch must not swallow it +// (a silent spinner on the one screen that exists to bootstrap the first channel is the worst place +// to hide the failure). function ChannelsEmptyState({ creatingBlank, + mutationError, onCreateBlank }: { creatingBlank: boolean; + mutationError: string | null; onCreateBlank: () => void; }) { return ( No channels} subtitle="No channels yet — create your first one.">

No channels yet

+ {mutationError && ( +
+
+ )}
{activeSchedule && ( <> - + )} -- 2.47.3 From b26045676b42d94e7f248904900faed640f060f4 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 11 Jul 2026 13:06:03 +0200 Subject: [PATCH 5/5] fix(spa): gate Delete on saving to close the last switch-during-save clobber MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex re-review of the prior fix commit found the Edit gate closed the exact repro but two paths remained. One is reachable: Delete was the only schedule-switch path that guardedSwitch's `saving` guard didn't cover — deleting mid-save runs applySwitch to the next schedule while the in-flight items PUT is still outstanding, and that PUT's completion handler then overwrites the next schedule's draft with the deleted schedule's response. Delete is now `disabled={saving}`, consistent with the Select, Edit, and guardedSwitch. Regression: the deferred-PUT test now also asserts Delete is disabled in-flight and re-enables after the save settles. Also widens the test mock's onRequest return type to `Response | Promise | null` (removes the `as unknown as Response` cast — a test-only type hole the re-review flagged). Deferred to #248: the other residual path (properties dialog not focus-trapped, so keyboard focus can escape to underlying Add/Save mid-save) is a pre-existing, cross-cutting overlay.tsx a11y gap affecting all dialogs — out of scope for this targeted blocker fix. Co-Authored-By: Claude Fable 5 --- web/src/screens/SchedulesScreen.test.tsx | 14 +++++++++----- web/src/screens/SchedulesScreen.tsx | 7 +++++-- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/web/src/screens/SchedulesScreen.test.tsx b/web/src/screens/SchedulesScreen.test.tsx index 2a5d8960a..f536bc51f 100644 --- a/web/src/screens/SchedulesScreen.test.tsx +++ b/web/src/screens/SchedulesScreen.test.tsx @@ -80,7 +80,7 @@ function responseItem(overrides: Record = {}) { interface MockOptions { schedules?: unknown[]; items?: unknown[]; - onRequest?: (url: string, method: string, body: unknown) => Response | null; + onRequest?: (url: string, method: string, body: unknown) => Response | Promise | null; } interface MockHandle { @@ -482,8 +482,8 @@ describe('SchedulesScreen — Edit is blocked while an items save is in flight ( const handle = mockApi({ onRequest: (url, method) => { if (url === '/api/schedules/1/items' && method === 'PUT') { - // Hold the PUT in flight; resolve only after we've asserted Edit is disabled. - return putPending as unknown as Response; + // Hold the PUT in flight; resolve only after we've asserted Edit/Delete are disabled. + return putPending; } return null; } @@ -496,12 +496,16 @@ describe('SchedulesScreen — Edit is blocked while an items save is in flight ( 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)); - // Edit is disabled while saving → the discard-to-open race is unreachable. + // Edit AND Delete are disabled while saving → the discard-to-open race (Edit) and the + // switch-during-save race (Delete → applySwitch → stale PUT clobbers the next schedule) are both + // unreachable. Delete is the only schedule-switch path guardedSwitch's `saving` guard missed. expect((screen.getByRole('button', { name: /^Edit$/ }) as HTMLButtonElement).disabled).toBe(true); + expect((screen.getByRole('button', { name: /^Delete$/ }) as HTMLButtonElement).disabled).toBe(true); - // Resolve the PUT; Edit re-enables once the save settles over the fresh baseline. + // Resolve the PUT; both re-enable once the save settles over the fresh baseline. const submitted = handle.requests.filter((r) => r.url === '/api/schedules/1/items' && r.method === 'PUT').pop(); resolvePut(jsonResponse(((submitted?.body as { items: Record[] }).items).map((it, i) => responseItem({ ...it, id: 500 + i, index: i, name: `Item ${i}` })))); await waitFor(() => expect((screen.getByRole('button', { name: /^Edit$/ }) as HTMLButtonElement).disabled).toBe(false)); + expect((screen.getByRole('button', { name: /^Delete$/ }) as HTMLButtonElement).disabled).toBe(false); }); }); diff --git a/web/src/screens/SchedulesScreen.tsx b/web/src/screens/SchedulesScreen.tsx index e23f237b7..5a291e692 100644 --- a/web/src/screens/SchedulesScreen.tsx +++ b/web/src/screens/SchedulesScreen.tsx @@ -352,7 +352,10 @@ export function SchedulesScreen() { // the draft is still dirty, so without the gate a user could discard-to-open the editor, flip // shuffle, and have the in-flight items PUT resolve AFTER the shuffle reload — clobbering the // freshly normalized draft with the pre-shuffle response. Blocking Edit while saving closes that - // race (Codex review, PR #242). + // race (Codex review, PR #242). Delete is gated on `saving` for the same class of race: it is the + // only schedule-switch path (via applySwitch) that guardedSwitch's `saving` guard didn't already + // cover, so deleting mid-save could switch schedules and let the stale items PUT clobber the next + // schedule's draft (Codex re-review, PR #242). const openEditForm = () => { if (dirtyRef.current && !window.confirm(DIRTY_PROMPT)) { return; @@ -455,7 +458,7 @@ export function SchedulesScreen() { {activeSchedule && ( <> - + )}
-- 2.47.3