diff --git a/docs/decisions.md b/docs/decisions.md index 6c516a62f..1c456cf5e 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -1680,3 +1680,41 @@ worried about was already correct (each handler loads/validates → 404 before ` first-party SPA only ever echoes the single canonical strong tag we emit, so no shipped client changes behavior; the change only makes a hand-written/tooling `If-Match` get the RFC-correct status. Docs: `api-conventions.md` §7a. Refs #265 #253 #197. + +## 2026-07-12 — TopBar primary-action button: wire creates, drop the rest (#238) + +The shell TopBar rendered a prominent top-right primary-action button (Plus icon) for **every** screen, but +only `SchedulesScreen` had ever subscribed to its `ctv:primary-action` event — so on every other screen the +button was **dead** (either a labelled no-op like "Save Changes"/"Add Channel", or, for the ~10 routes whose +`primaryAction` was `''`, a bare labelless "+"; the button rendered unconditionally). Issue #238 (from a #229 +live-E2E finding, pre-existing since the TopBar's introduction). + +**Decision — the Plus-icon button is a "create new item" affordance; keep+wire it only where that fits:** + +- **TopBar renders the button only when the active route declares a non-empty `primaryAction`** (was: + unconditional). This alone removes every empty-`''` dead "+". +- **WIRE** (8 list screens with a single unambiguous create flow) via a shared `usePrimaryAction(routeId, + handler)` hook (`web/src/primaryAction.ts`), each delegating to the same top-level create handler its in-body + control uses (`navigateToPath('/app/new-channel')`, `setEditing({kind:'new'})`, `navigateToPath('…/add')`, + etc.): channels, schedules (refactored onto the hook), multiCollections, rerunCollections, traktLists, + fillerPresets, ffmpegProfiles, watermarks. +- **DROP** (`primaryAction: ''`, no button) everywhere else, for one of four reasons: (1) the action isn't a + create so the "+" is wrong and a correct in-body control already exists — editChannel (savebar), settings + (savebar), guide (Jump to now), playouts (Reset all), logs/troubleshooting/blockPlayoutTroubleshooting + (Refresh), playbackTroubleshooting (Play), yamlValidator (Validate); (2) ambiguous — collections (two create + types behind tabs); (3) a silent no-op — builder ("Create Channel" is disabled until the form is valid), + playlists (needs a group first); (4) semantically misplaced — dashboard (status page), libraries ("Scan" is + per-library-row, no global target). + +**Why not wire everything** (the SchedulesScreen precedent): the recon found every screen already carries a +correct, disabled-state-aware, context-aware in-body control, and several banner actions are unreachable +without refactoring handlers out from under early returns, or would render a silent no-op — reintroducing the +very dead-button class #238 fixes. Wiring only the single-create-flow screens gives one explainable rule +("primary button = create a new item on a list screen") and needs no risky refactors. + +**Also fixed here:** the `apiKey` route's `primaryAction: 'Save key'` + its description/comment were stale +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 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 dc8838ce9..ef0f08204 100644 --- a/docs/spa-conventions.md +++ b/docs/spa-conventions.md @@ -339,3 +339,31 @@ Keep the guard predicate reading a **ref** (`dirtyRef`), not the `dirty` state v copying it.** #221 came from copying a fetch model that keeps stale results visible onto screens that had gained mutation surfaces — the exemplar was safe read-only, the copy was not. Copying a pattern copies its *assumptions*; confirm they still hold in the new context. + +## 10. TopBar primary-action button (`usePrimaryAction`) + +The shell TopBar (`App.tsx`) renders at most **one** primary-action button (top-right, Plus icon) for the +active screen. The TopBar has no reference to the screen component, so the click is delivered as a window +`CustomEvent` keyed on the active route id; the seam is `web/src/primaryAction.ts`: + +- **A screen opts in** by calling `usePrimaryAction(routeId, handler)` at the **top of the component, before + any early return** (it's a hook). The handler must be reachable there — a create/navigate handler or a + `useState` setter, not something defined below a loading/error `return`. Reference: `SchedulesScreen` + (`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. + 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 + Profile", "Add Watermark"). **Drop it** (`primaryAction: ''`) where the primary action is **not** a create + (Save / Refresh / Play / Validate / Reset / Scan — the "+" is wrong, and those screens already carry the + correct in-body control), or where a single banner action would be **ambiguous** (Collections has two create + types behind tabs), **a silent no-op** (the Channel builder's Create is disabled until the form is valid), + or **semantically misplaced** (Dashboard is a status page; Libraries "Scan" is per-row). Coexisting with an + in-body create control is fine (Schedules has both) — the banner is a convenience, not the sole entry point. + See `docs/decisions.md` 2026-07-12 for the full rationale. diff --git a/web/src/App.test.tsx b/web/src/App.test.tsx index a727628d1..671f4bd78 100644 --- a/web/src/App.test.tsx +++ b/web/src/App.test.tsx @@ -31,11 +31,41 @@ describe('ChicoryTV SPA scaffold', () => { expect(screen.getByRole('banner')).toBeInTheDocument(); expect(screen.getByRole('searchbox', { name: 'Search' })).toBeInTheDocument(); expect(screen.getByRole('button', { name: 'Connect' })).toBeInTheDocument(); - expect(screen.getByRole('button', { name: 'Add Channel' })).toBeInTheDocument(); + // The Dashboard declares no primaryAction, so the TopBar renders no primary-action button + // here (issue #238 — the old code rendered a dead "Add Channel" button on every screen). + expect(screen.queryByRole('button', { name: 'Add Channel' })).not.toBeInTheDocument(); expect(await screen.findAllByText('Healthy')).toHaveLength(2); expect(designSystemStylesheet).toBe('../../design-system/styles.css'); }); + // 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(); + + 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). + fireEvent.click(screen.getByRole('link', { name: 'Logs' })); + expect(await screen.findByRole('heading', { name: 'Logs' })).toBeInTheDocument(); + expect(within(screen.getByRole('banner')).queryByRole('button', { name: /Refresh/ })).toBeNull(); + }); + it('routes between shell screen slots without a page reload', async () => { render(); diff --git a/web/src/App.tsx b/web/src/App.tsx index 7190a5ced..a213ff985 100644 --- a/web/src/App.tsx +++ b/web/src/App.tsx @@ -73,6 +73,7 @@ import { RemoteLibrariesEditScreen } from './screens/RemoteLibrariesEditScreen'; import { PathReplacementsEditScreen } from './screens/PathReplacementsEditScreen'; import { SchedulesScreen } from './screens/SchedulesScreen'; import { canLeaveCurrentScreen } from './navigationGuard'; +import { dispatchPrimaryAction } from './primaryAction'; import { ArtistDetailScreen, MovieDetailScreen, @@ -223,7 +224,7 @@ const routes: ScreenRoute[] = [ kicker: 'Overview', description: 'On-air status, health checks, and recent server activity.', icon: