The shell TopBar rendered a primary-action button (Plus icon) for every screen, but only SchedulesScreen subscribed to its ctv:primary-action event — so every other screen's button was dead (a labelled no-op, or a bare "+" for the ~10 routes whose primaryAction was ''; the button rendered unconditionally). Pre-existing since the TopBar's introduction (surfaced by the #229 live-E2E).
Resolution
The Plus-icon button is a "create new item" affordance. Keep + wire it only on the 8 list screens with a single create flow, via a shared usePrimaryAction(routeId, handler) hook (web/src/primaryAction.ts); drop it (primaryAction: '') everywhere else. The TopBar now renders the button only when primaryAction is non-empty.
DROP (rest): action screens (Save/Refresh/Play/Validate/Reset/Scan — "+" is wrong, in-body control exists), ambiguous (collections tabs), silent no-op (builder, playlists), misplaced (dashboard, libraries). Plus relabelled the stale post-#295 apiKey "Save key"/description.
Verification
npm run build + npm run lint clean; 734/734 vitest pass. New App.test.tsx invariant test: a create screen's banner navigates; an action screen shows no banner button.
Pure-SPA change, no server/write-path effect.
Docs
docs/spa-conventions.md §10 (the usePrimaryAction hook + wire-vs-drop convention)
## What
The shell TopBar rendered a primary-action button (Plus icon) for **every** screen, but only `SchedulesScreen` subscribed to its `ctv:primary-action` event — so every other screen's button was dead (a labelled no-op, or a bare "+" for the ~10 routes whose `primaryAction` was `''`; the button rendered unconditionally). Pre-existing since the TopBar's introduction (surfaced by the #229 live-E2E).
## Resolution
The Plus-icon button is a **"create new item"** affordance. Keep + wire it only on the 8 list screens with a single create flow, via a shared `usePrimaryAction(routeId, handler)` hook (`web/src/primaryAction.ts`); drop it (`primaryAction: ''`) everywhere else. The TopBar now renders the button **only when `primaryAction` is non-empty**.
- **WIRE (8):** channels, schedules (refactored onto the hook), multiCollections, rerunCollections, traktLists, fillerPresets, ffmpegProfiles, watermarks.
- **DROP (rest):** action screens (Save/Refresh/Play/Validate/Reset/Scan — "+" is wrong, in-body control exists), ambiguous (collections tabs), silent no-op (builder, playlists), misplaced (dashboard, libraries). Plus relabelled the stale post-#295 `apiKey` "Save key"/description.
## Verification
- `npm run build` + `npm run lint` clean; **734/734 vitest** pass. New `App.test.tsx` invariant test: a create screen's banner navigates; an action screen shows no banner button.
- Pure-SPA change, no server/write-path effect.
## Docs
- `docs/spa-conventions.md` §10 (the `usePrimaryAction` hook + wire-vs-drop convention)
- `docs/decisions.md` 2026-07-12
fixes #238
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Independent review — two cold passes, both green after fixes
Pass 1 (full PR diff): MERGEABLE-WITH-NITS. Verified Rules-of-Hooks placement in all 8 wired screens, the label⟺subscription invariant (8/8 both directions, char-for-char), the ref/stale-handler pattern, every wiring target, and the tests. Two non-blocking findings:
Low — traktLists TopBar button was a silent no-op on the /app/trakt-lists/{id} detail sub-route.
Nit — the invariant test only spot-checked 2 of 8 screens.
Fixes (commit 51b67dea): guard the trakt handler on editingId (route back to the list from the detail view; the screen is keyed by pathname so setAddOpen wouldnt survive a navigate), and replace the spot-check with a data-driven it.each over the 4 URL-navigating create screens (a typo`d route id now fails red). Docs wording corrected.
Pass 2 (re-review of the fix commit 51b67dea): MERGEABLE. Guard correct + necessary (no stale-closure hazard — handler ref refreshed each render), all 4 test triples match the route table, test isolation sound.
Local gate: 738/738 vitest, lint + build clean. Pure-SPA change, no server/write-path effect (live-E2E intentionally skipped per the pure-SPA-leaf exemption — the App.test render+click drives the actual banner→navigation flow).
## Independent review — two cold passes, both green after fixes
**Pass 1 (full PR diff):** MERGEABLE-WITH-NITS. Verified Rules-of-Hooks placement in all 8 wired screens, the label⟺subscription invariant (8/8 both directions, char-for-char), the ref/stale-handler pattern, every wiring target, and the tests. Two non-blocking findings:
- **Low** — traktLists TopBar button was a silent no-op on the `/app/trakt-lists/{id}` detail sub-route.
- **Nit** — the invariant test only spot-checked 2 of 8 screens.
**Fixes (commit `51b67dea`):** guard the trakt handler on `editingId` (route back to the list from the detail view; the screen is keyed by pathname so `setAddOpen` wouldnt survive a navigate), and replace the spot-check with a data-driven `it.each` over the 4 URL-navigating create screens (a typo`d route id now fails red). Docs wording corrected.
**Pass 2 (re-review of the fix commit `51b67dea`):** MERGEABLE. Guard correct + necessary (no stale-closure hazard — handler ref refreshed each render), all 4 test triples match the route table, test isolation sound.
Local gate: **738/738 vitest**, lint + build clean. Pure-SPA change, no server/write-path effect (live-E2E intentionally skipped per the pure-SPA-leaf exemption — the App.test render+click drives the actual banner→navigation flow).
Review-verdict: MERGEABLE @ 51b67dea
timothy
merged commit 51b67dea06 into main2026-07-12 23:51:26 +02:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
What
The shell TopBar rendered a primary-action button (Plus icon) for every screen, but only
SchedulesScreensubscribed to itsctv:primary-actionevent — so every other screen's button was dead (a labelled no-op, or a bare "+" for the ~10 routes whoseprimaryActionwas''; the button rendered unconditionally). Pre-existing since the TopBar's introduction (surfaced by the #229 live-E2E).Resolution
The Plus-icon button is a "create new item" affordance. Keep + wire it only on the 8 list screens with a single create flow, via a shared
usePrimaryAction(routeId, handler)hook (web/src/primaryAction.ts); drop it (primaryAction: '') everywhere else. The TopBar now renders the button only whenprimaryActionis non-empty.apiKey"Save key"/description.Verification
npm run build+npm run lintclean; 734/734 vitest pass. NewApp.test.tsxinvariant test: a create screen's banner navigates; an action screen shows no banner button.Docs
docs/spa-conventions.md§10 (theusePrimaryActionhook + wire-vs-drop convention)docs/decisions.md2026-07-12fixes #238
🤖 Generated with Claude Code
cd719445b2to51b67dea06Independent review — two cold passes, both green after fixes
Pass 1 (full PR diff): MERGEABLE-WITH-NITS. Verified Rules-of-Hooks placement in all 8 wired screens, the label⟺subscription invariant (8/8 both directions, char-for-char), the ref/stale-handler pattern, every wiring target, and the tests. Two non-blocking findings:
/app/trakt-lists/{id}detail sub-route.Fixes (commit
51b67dea): guard the trakt handler oneditingId(route back to the list from the detail view; the screen is keyed by pathname sosetAddOpenwouldnt survive a navigate), and replace the spot-check with a data-drivenit.eachover the 4 URL-navigating create screens (a typo`d route id now fails red). Docs wording corrected.Pass 2 (re-review of the fix commit
51b67dea): MERGEABLE. Guard correct + necessary (no stale-closure hazard — handler ref refreshed each render), all 4 test triples match the route table, test isolation sound.Local gate: 738/738 vitest, lint + build clean. Pure-SPA change, no server/write-path effect (live-E2E intentionally skipped per the pure-SPA-leaf exemption — the App.test render+click drives the actual banner→navigation flow).
Review-verdict: MERGEABLE @
51b67dea