diff --git a/docs/decisions.md b/docs/decisions.md index c1a431860..1c456cf5e 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -1716,4 +1716,5 @@ very dead-button class #238 fixes. Wiring only the single-create-flow screens gi post-#295 (the screen displays the machine key + changes the local password; there is nothing to "save") — relabelled to reflect the #295 reality. Convention + hook documented in `spa-conventions.md` §10. The implicit route-label ⟺ screen-subscription coupling is the kind of thing #247 (shell extraction) will -formalize; #238 keeps it a documented convention guarded by an `App.test.tsx` invariant test. Refs #238 #247. +formalize; #238 keeps it a documented convention guarded by a data-driven `App.test.tsx` test over the URL-navigating +create screens (a typo'd route id → the banner navigates nowhere → red). Refs #238 #247. diff --git a/docs/spa-conventions.md b/docs/spa-conventions.md index f5b3846de..ef0f08204 100644 --- a/docs/spa-conventions.md +++ b/docs/spa-conventions.md @@ -352,9 +352,11 @@ active screen. The TopBar has no reference to the screen component, so the click (`usePrimaryAction('schedules', () => setForm('create'))`). - **The route must ALSO declare a matching non-empty `primaryAction` label** in the `routes` table. The TopBar renders the button **only when `route.primaryAction` is non-empty** — so declaring a label without - subscribing renders a dead button, and subscribing without a label renders nothing. Keep the two in lockstep - (invariant covered by the `#238` test in `App.test.tsx`: every non-empty-`primaryAction` route is a create - screen whose banner works; a no-`primaryAction` route shows no banner button). + subscribing renders a dead button, and subscribing without a label renders nothing. Keep the two in lockstep. + The `#238` tests in `App.test.tsx` guard this: a data-driven `it.each` asserts each URL-navigating create + screen's banner actually navigates (so a typo'd route id → a button that navigates nowhere → red), plus a + drop test that an action screen shows no banner button. The dialog/editor create screens (schedules, multi/ + rerun collections, trakt) are exercised by their own create-flow tests. - **When to WIRE vs DROP (issue #238).** The Plus icon makes the button semantically a *"create new item"* affordance. **Keep + wire it only on list screens with a single, unambiguous create flow** ("Add Channel", "Add Schedule", "Add Multi-Collection", "Add Rerun Collection", "Add Trakt List", "Add Filler Preset", "Add diff --git a/web/src/App.test.tsx b/web/src/App.test.tsx index 11df3ca2d..671f4bd78 100644 --- a/web/src/App.test.tsx +++ b/web/src/App.test.tsx @@ -38,16 +38,26 @@ describe('ChicoryTV SPA scaffold', () => { expect(designSystemStylesheet).toBe('../../design-system/styles.css'); }); - it('wires the TopBar primary-action button on a create screen and drops it elsewhere (#238)', async () => { + // Data-driven over every WIRED create screen whose primary action navigates to a URL — this + // catches a typo'd route-id in usePrimaryAction on any of them (the button would render but do + // nothing). The button lives in the shell header (role="banner"), scoped so an in-body Add + // control can't satisfy the assertion. (#238) + it.each([ + ['/app/channels', 'Add Channel', '/app/new-channel'], + ['/app/filler-presets', 'Add Filler Preset', '/app/filler-presets/add'], + ['/app/ffmpeg-profiles', 'Add Profile', '/app/ffmpeg-profiles/add'], + ['/app/watermarks', 'Add Watermark', '/app/watermarks/add'] + ])('wires the TopBar create button on %s → %s (#238)', async (path, label, target) => { + window.history.pushState(null, '', path); render(); - // A "create list" screen (Channels) shows a working "+ Add Channel" TopBar button that - // navigates to the builder; the button lives in the shell header (role="banner"). - fireEvent.click(screen.getByRole('link', { name: 'Channels' })); - expect(await screen.findByRole('heading', { name: 'Channels' })).toBeInTheDocument(); - const banner = screen.getByRole('banner'); - fireEvent.click(within(banner).getByRole('button', { name: 'Add Channel' })); - expect(window.location.pathname).toBe('/app/new-channel'); + const banner = await screen.findByRole('banner'); + fireEvent.click(within(banner).getByRole('button', { name: label })); + expect(window.location.pathname).toBe(target); + }); + + it('drops the TopBar primary-action button on action screens (#238)', async () => { + render(); // An "action" screen (Logs) declares no primaryAction → no TopBar button at all // (its Refresh control lives in-body, where the "+" primary styling would be wrong). diff --git a/web/src/screens/TraktListsScreen.tsx b/web/src/screens/TraktListsScreen.tsx index 15ce84568..95df36542 100644 --- a/web/src/screens/TraktListsScreen.tsx +++ b/web/src/screens/TraktListsScreen.tsx @@ -277,8 +277,17 @@ export function TraktListsScreen() { const [rowError, setRowError] = useState(null); const activeRef = useRef(true); - // TopBar "Add Trakt List" primary action. - usePrimaryAction('traktLists', () => setAddOpen(true)); + // TopBar "Add Trakt List" primary action. On the list, open the add dialog. On the + // detail sub-route (/app/trakt-lists/{id}) the add dialog isn't mounted and this component + // is keyed by pathname (so state wouldn't survive a navigate), so route back to the list + // instead of flipping a state nobody renders — avoids a silent no-op button (#238 review). + usePrimaryAction('traktLists', () => { + if (editingId != null) { + navigateToPath(TRAKT_BASE_PATH); + } else { + setAddOpen(true); + } + }); // Fetch only; state updates happen in the async callbacks (never synchronously in the // effect body). `loading` starts true and flips false in `finally`, so the mount effect