401 lines
30 KiB
Markdown
401 lines
30 KiB
Markdown
# SPA conventions — "Add a screen" playbook
|
|
|
|
Purpose: a precise playbook for adding a new screen (or sub-path editor) to the ChicoryTV React SPA
|
|
(`web/`), for an agent with no prior context in this repo. **Update this doc in the same PR that
|
|
changes any convention below.**
|
|
|
|
Companion to `api-conventions.md` (the API surface the SPA talks to) and `docs/contributing.md`
|
|
(general repo conventions).
|
|
|
|
## 1. Stack & layout
|
|
|
|
Vite + React + TypeScript, builds to `ErsatzTV/wwwroot/app` (see `web/vite.config.ts`:
|
|
`base: '/app/'`, `build.outDir: '../ErsatzTV/wwwroot/app'`), served by the ASP.NET host at `/app`.
|
|
|
|
- **Composition root**: `web/src/App.tsx` — owns the active route, approved Libraries sub-path,
|
|
navigation guard integration, and theme; it composes the shell with the matched screen.
|
|
- **Routes + nav**: `web/src/app/routes.tsx` — the ONE stable module-level array of `ScreenRoute`
|
|
objects (`path`, `label`, `title`, `kicker`, `icon`, etc.) plus `allowSubPaths?: boolean`, the
|
|
matcher, and sidebar group definitions. Route objects must never be rebuilt per render.
|
|
- **Shell + screen dispatch**: `web/src/app/AppShell.tsx` owns Sidebar/TopBar/Connect/version/theme
|
|
chrome; `web/src/app/ScreenContent.tsx` exhaustively maps a matched route to its screen and owns
|
|
the Media/Libraries route wrappers.
|
|
- **Screens**: `web/src/screens/*.tsx`, one file per top-level screen, generally with a colocated
|
|
`*.test.tsx`.
|
|
- **API clients**: `web/src/api/<domain>.ts` (see §4).
|
|
- **Styling**: `web/src/shell.css` (+ `web/src/components/components.css`) — utility classes with a
|
|
`ctv-` prefix (~690 occurrences across those two files). Reuse an existing `ctv-*` class before
|
|
inventing a new one.
|
|
|
|
## 2. CRITICAL: sub-path screens must own their own pathname state
|
|
|
|
If a route sets `allowSubPaths: true` (e.g. so `/app/blocks/{id}` works under the `/app/blocks` nav
|
|
entry), **the screen component itself must track `window.location.pathname` and listen for
|
|
`popstate`** — do not rely on the composition root re-rendering `ScreenContent` when the sub-path
|
|
changes.
|
|
|
|
**Why**: `app/routes.tsx`'s `routeFromLocation()` matches an `allowSubPaths` route by prefix
|
|
(`pathname.startsWith(\`${route.path}/\`)`) and returns the **same `ScreenRoute` object reference**
|
|
for the base path and every sub-path under it. `App`'s state update is
|
|
`setActiveRoute(routeFromLocation())`; React's `useState` setter bails via `Object.is` when the new
|
|
value is reference-equal to the old one — so navigating from `/app/blocks` to `/app/blocks/42` (or
|
|
between `/app/blocks/42` and `/app/blocks/17`) **never re-invokes `ScreenContent`** at the `App`
|
|
level. See the comment block directly above `PlayoutsRouteScreen` in
|
|
`screens/PlayoutsScreen.tsx` for the canonical explanation, and its implementation
|
|
(`useState(() => window.location.pathname)` +
|
|
`useEffect` with a `popstate` listener local to the wrapper component) for the fix.
|
|
|
|
Exemplars of screens that already do this correctly: `BlocksScreen.tsx`, `TemplatesScreen.tsx`,
|
|
`DecosScreen.tsx`, `DecoTemplatesScreen.tsx`, and the `PlayoutsRouteScreen` wrapper colocated in
|
|
`PlayoutsScreen.tsx` (which owns two sibling sub-paths, `/playouts/{id}/alternate-schedules` and
|
|
`/playouts/{id}/templates`, dispatching internally via `parsePlayoutSubRoute`).
|
|
|
|
**Exception — a guarded sub-path route defers pathname ownership to App.** When a sub-path route's
|
|
screens ALSO register a dirty guard (§8), the wrapper must **not** self-listen for `popstate`; App
|
|
owns the pathname and passes the approved sub-path down as a prop. The `LibrariesRouteScreen` wrapper
|
|
in `app/ScreenContent.tsx` is the exemplar (sub-path parsed by `parseLibrariesSubRoute`, dispatched
|
|
via a flat `switch`, sub-path supplied by App's `librariesSubPath` state). See §8 for why child-before-parent
|
|
effect order makes the self-listening pattern unsafe here.
|
|
|
|
## 3. Data loading pattern
|
|
|
|
Reference implementation: `web/src/screens/LogsScreen.tsx`. Structure to copy for any screen that
|
|
fetches from the API:
|
|
|
|
- A **discriminated-union state type** covering loading/success/error, e.g.
|
|
`type LogsState = { status: 'loading'; ... } | { status: 'success'; ... } | { status: 'error'; ... }`.
|
|
- A `seqRef` (monotonically incremented request counter) + `activeRef` (mount-tracking boolean,
|
|
flipped in a mount/unmount `useEffect`) pair — guards against a stale, slower request overwriting
|
|
a newer one's result, and against setting state after unmount.
|
|
- The actual fetch lives in a `useCallback` (`load`), called from a **separate** `useEffect(() => {
|
|
load(); }, [load])`.
|
|
- **Lint rule — `react-hooks` "no set-state-in-effect"**: never call `setState` **synchronously in
|
|
the body** of a `useEffect`. State transitions happen only inside event handlers or promise
|
|
`.then()`/`.catch()` callbacks (as in `LogsScreen`'s `load`). This is enforced by
|
|
`eslint-plugin-react-hooks` in `web/eslint.config.js` — a synchronous `setState` in an effect body
|
|
will fail `npm run lint`.
|
|
|
|
## 3a. "Keep results visible during refetch" ⇒ gate mutations + show a refreshing cue
|
|
|
|
Some grid screens deliberately keep the **previous** successful result set rendered while a refetch
|
|
is in flight (no full-screen loading state on a query/kind/page change), so the grid doesn't flash
|
|
empty. `SearchScreen.tsx` and `MediaBrowseScreen.tsx` do this. If such a screen also carries
|
|
**mutation surfaces** (per-card Add-to menu, Select/select-mode, a selection action bar, "Add all",
|
|
"Save as smart collection"), those surfaces would otherwise stay live over a **stale** result set —
|
|
an add/select action then targets the about-to-be-replaced items, or (worse) a query-wide "Add all"
|
|
bulk request resolves against the previous query. This was issue #221 (adversarial-reviewer#18).
|
|
|
|
Convention — when a screen keeps stale results visible during a refetch:
|
|
|
|
- **Key the success state to the request params that produced it.** Store the identifying params on
|
|
the `status: 'success'` variant (`SearchScreen`: the `query`; `MediaBrowseScreen`: a
|
|
`kind|query|page` `key`), set in the seq-guarded `.then`. Derive
|
|
`const refreshing = state.status === 'success' && state.<key> !== <current params>;` in render.
|
|
Prefer this over a synchronously-set `refreshing` flag: setting state synchronously from the load
|
|
path trips the `react-hooks` "no set-state-in-effect" rule (§3).
|
|
**Invariant, not a guarantee**: the derivation is only self-correcting when *every* value the
|
|
current params can take will actually trigger a fetch. If `load()` early-returns for some param
|
|
value (e.g. `SearchScreen`'s blank-query guard), `state` never updates for that value and a stale
|
|
`status: 'success'` variant lingers — so the comparison must exclude params that suppress
|
|
fetching, or gate the whole flag on the same condition that gates the fetch (`SearchScreen`:
|
|
`const refreshing = hasQuery && state.status === 'success' && state.query !== query.trim();` —
|
|
fixed post-review in #222 after the naive derivation got stuck `true` once the query was cleared
|
|
to empty, see PR discussion for #221).
|
|
- **While `refreshing`:** show a visible cue (a `role="status"` "Refreshing…" row with `<Spinner>`
|
|
plus the `.ctv-media-grid-dim` opacity class on the grid) and **disable every mutation surface** —
|
|
per-card Add-to menu (withhold the `actions` node), select toggle + in-grid selection
|
|
(`const canSelect = selectMode && !refreshing;` gates `onToggleSelect`), the selection action bar,
|
|
"Add all", "Save as smart collection". Card navigation (`onOpen`) **may** stay live — but only
|
|
outside select mode: while `selectMode && refreshing`, `MediaPosterCard` falls back to `onOpen`
|
|
whenever `onToggleSelect` is undefined, so both props must be withheld together or a mid-select
|
|
click navigates away instead of no-op'ing. The select-mode toggle itself should only be disabled
|
|
while refreshing when *entering* select mode (`refreshing && !selectMode`) — exiting only clears
|
|
selection, not a mutation, so it must stay available.
|
|
- **Bind async bulk completions to their request params, not just mount.** A whole-query/whole-set
|
|
request (e.g. `getSearchAllItems`) must, on resolve, check that its snapshotted params are still
|
|
current (compare against a ref that always holds the committed value — `SearchScreen` reuses
|
|
`lastQueryRef`) and **discard** otherwise. Checking only `activeRef` (mounted) is insufficient.
|
|
|
|
## 4. API client modules
|
|
|
|
One file per domain in `web/src/api/`, e.g. `logs.ts`, `blocks.ts`, `playouts.ts`. Pattern (see
|
|
`web/src/api/logs.ts`):
|
|
|
|
- Re-export the generated response/DTO types from `./generated/v1`:
|
|
`export type LogEntry = components['schemas']['LogEntryResponseModel'];`
|
|
- A typed params interface for the endpoint's query string (e.g. `GetLogsParams`).
|
|
- The fetch function builds a `URLSearchParams` from only the params that are set, then calls the
|
|
shared `request<T>(url)` helper from `./client`.
|
|
- An error-message helper (e.g. `messageFromLogsError`) that narrows `unknown` → `ApiError` (from
|
|
`./client`) → a human string, with a fallback message — screens use this instead of stringifying
|
|
errors themselves.
|
|
- `web/src/api/index.ts` re-exports everything so screens import from `'../api'`, not from the
|
|
individual domain file directly.
|
|
|
|
**Trust the generated key casing — it mirrors the runtime.** Since #198 the OpenAPI spec is generated
|
|
to match the runtime Newtonsoft serializer exactly (a schema transformer runs the same contract
|
|
resolver; see `api-conventions.md` §5a), so the generated types carry the real wire keys — including
|
|
Newtonsoft's acronym quirks like `ffmpegProfileId` (channel FFmpeg-profile id) and `ffmpegProfile`
|
|
(channel FFmpeg-profile display name). **Do not** hand-cast responses to "fix" a key or dual-read a
|
|
spec-cased vs runtime-cased key (the old `PlaybackTroubleshootingScreen` `#198` escape hatch that read
|
|
`data.channel.fFmpegProfileId` has been removed — read `ffmpegProfileId` straight off the typed
|
|
response). When you mock an API response in a test, use the generated (runtime) casing.
|
|
|
|
## 4a. Optimistic-concurrency editors (ETag / If-Match / 412)
|
|
|
|
Replace-all editors (Blocks is the reference; see `api-conventions.md` §7a for the server contract,
|
|
issue #253) must round-trip the aggregate's concurrency ETag so a stale tab can't silently overwrite a
|
|
fresher edit:
|
|
|
|
- **Transport seam** (`web/src/api/client.ts`): `requestWithMeta<T>(path, options)` returns
|
|
`{ data, etag }` (reads the `ETag` response header). `request<T>` delegates to it and drops the meta —
|
|
keep using `request` for endpoints without a concurrency token.
|
|
- **Domain module** (`web/src/api/blocks.ts`): expose a `…WithMeta` load
|
|
(`getBlockItemsWithMeta` → `{ data, etag }`) and make the replace accept the last-seen ETag and return
|
|
the new one: `replaceBlock(id, body, ifMatch?)` → `requestWithMeta(..., { headers: ifMatch ? {'If-Match': ifMatch} : undefined })`.
|
|
- **Editor**: hold the ETag in a `useRef`; set it from the load GET, and **replace it from the PUT
|
|
response's ETag on every successful save** (a same-tab second save otherwise 412s against its own
|
|
write). Keyed reload: a `reloadKey` state in the load `useEffect` dep array lets the conflict flow
|
|
re-fetch.
|
|
- **412 UX**: catch `error instanceof ApiError && error.status === 412` on save and open a blocking
|
|
"changed elsewhere — reload (unsaved changes discarded)" `ConfirmDialog` (Reload bumps `reloadKey`),
|
|
distinct from a 409 ("build in progress — retry shortly"). All other errors stay the generic save-error
|
|
path. Reference: `web/src/screens/BlocksScreen.tsx` `BlockEditor`.
|
|
- **List-derived editors** (Multi/Rerun collections open the editor from a paged-list row, not a per-record
|
|
GET): fetch the single record via the `…WithMeta` by-id helper (`getMultiCollectionWithMeta` /
|
|
`getRerunCollectionWithMeta`) when the editor mounts, and build the draft from **that** response — so the
|
|
ETag and the draft data come from one read and stay consistent (the fail-safe ordering; don't pair a
|
|
list-row draft with a separately-fetched ETag). Editors that navigate back to the list on save (Playout,
|
|
Multi/Rerun, Collection reorder) need only the 412 branch — no post-save ETag rotation.
|
|
|
|
## 5. Artwork rendering
|
|
|
|
Render `item.artwork` / `item.poster` (or whatever the DTO field is named) **directly as an `<img
|
|
src>`** — since PR #181, API responses already return rooted, directly-usable URLs (see
|
|
`api-conventions.md` §4). **Do not** client-side-prefix artwork paths (no `/artwork/posters/` string
|
|
building in SPA code) — if you see that pattern, it's stale/wrong.
|
|
|
|
## 5b. HLS video preview
|
|
|
|
Screens that preview an ErsatzTV HLS stream use the reusable `HlsPlayer` component
|
|
(`web/src/media/HlsPlayer.tsx`, introduced with #145). Pass it a `src` (the `.m3u8` URL, or `null`
|
|
for idle) and — when the manifest GET itself starts a server-side session (e.g. troubleshooting
|
|
`playback.m3u8`) — a `playToken` you increment per play, so a repeat play with an identical URL
|
|
still tears down and re-attaches (an unchanged `src` alone is a state no-op that never issues a new
|
|
request); it attaches `hls.js` when Media Source Extensions are available and falls back to native
|
|
HLS (`video.canPlayType('application/vnd.apple.mpegurl')`, i.e. Safari) otherwise, and tears down the
|
|
`hls.js` instance on `src` change and unmount. Its config mirrors the legacy `_Host.cshtml`
|
|
`previewChannel` (`liveDurationInfinity: true` + an unbounded manifest `maxTimeToFirstByteMs`) because
|
|
the troubleshooting `playback.m3u8` endpoint blocks until segments exist before it 302s to the live
|
|
manifest. **In tests, mock `hls.js` wholesale** (`vi.mock('hls.js', …)` with a class exposing
|
|
`static isSupported()`, `static Events`, and `loadSource`/`attachMedia`/`on`/`destroy`) so jsdom never
|
|
touches a real `MediaSource`; assert the manifest URL via the mocked `loadSource` spy (see
|
|
`PlaybackTroubleshootingScreen.test.tsx`).
|
|
|
|
## 5c. Media "Add to…" affordances
|
|
|
|
Screens that let the user add media items to a collection/playlist/schedule use the shared layer in
|
|
`web/src/media/addTo/` — `AddToMenu` (popover for a `MediaPosterCard` `actions` slot or a detail
|
|
page's action row) and the `AddToCollectionDialog` / `AddToPlaylistDialog` / `AddToScheduleDialog` /
|
|
`SaveAsSmartCollectionDialog` it drives. Do **not** build screen-local target pickers. Multi-select
|
|
on grid screens is an explicit "Select" toggle (see `docs/decisions.md` 2026-07-10 for the rationale
|
|
and the accepted deviations from Blazor).
|
|
|
|
## 5d. Client-local preferences: `localStorage`, namespaced `ctv-*` keys
|
|
|
|
Per-browser UI preferences (theme, an auth token, a screen's remembered page size) live in
|
|
`window.localStorage` under a namespaced `ctv-` key, **not** a round-trip through the API — the
|
|
established pattern is `designSystem.ts`'s `getStoredDesignSystemTheme`/`applyDesignSystemTheme`
|
|
(`ctv-theme`): a small `getStorage()` helper that returns `window.localStorage` wrapped in a
|
|
try/catch (so a disabled/unavailable storage API degrades to the default instead of throwing), a
|
|
getter that validates the stored value against the known option set before trusting it, and a
|
|
setter that writes straight through. `LogsScreen.tsx`'s page-size persistence (`ctv-logs-page-size`,
|
|
#213) follows the same shape. Reserve this for state that's genuinely local to the browser/user
|
|
session — if a preference needs to be shared across devices or is really server/business state
|
|
(e.g. Blazor's `ConfigElement`-backed settings), it belongs behind an API endpoint instead; see
|
|
`docs/decisions.md` 2026-07-11 for the specific reasoning on logs page-size.
|
|
|
|
## 5e. Session auth: the boot gate, the machine-key screen + the global 401 signal (#295)
|
|
|
|
Since #295 the browser authenticates with a **session cookie**, not an API key. The whole `/api`
|
|
surface still answers a missing/expired session with **401**, and mutating verbs additionally require a
|
|
CSRF header. The former keyless-bootstrap `ApiKeyScreen` (the `X-Api-Key`/`localStorage` model of #197)
|
|
is gone. The SPA seams now are:
|
|
|
|
- **The request client sends no API key; it relies on the cookie and adds CSRF automatically**
|
|
(`web/src/api/client.ts`): the session cookie rides along with every same-origin request, and every
|
|
mutating verb (POST/PUT/PATCH/DELETE) gets an `X-Csrf` header set automatically — screens making a
|
|
POST get CSRF for free. There is **no** `getStoredApiKey`/`X-Api-Key` path anymore; the only residual
|
|
`localStorage` touch is `clearLegacyStoredApiKey()` (`web/src/api/auth.ts`), a one-shot cleanup the
|
|
boot gate runs once to purge any stale `ctv-api-key`.
|
|
- **The boot gate owns login/setup** (`web/src/AuthGate.tsx`), wrapping `<App/>` outside the
|
|
shell/router. It asks the PUBLIC `/api/v1/auth/config`, then `/api/v1/auth/session`, and renders Setup /
|
|
Login / the app. It also publishes `AuthContext` (`{ username, method, signOut, requireLogin }`) —
|
|
read `method` via `useContext(AuthContext)` to branch on the auth kind (e.g. `'local'` vs OIDC).
|
|
- **The machine-key screen is a normal authenticated screen** (`web/src/screens/ApiKeyScreen.tsx`,
|
|
route `/app/api-key`, System nav group). It loads the server machine key from `/api/v1/auth/machine-key`
|
|
(via `getMachineKey()`) and displays it **masked** (a masked `<code>` that never puts the key in the
|
|
DOM until Reveal) with Reveal + Copy affordances — the key is for MCP / external REST clients; the
|
|
browser no longer uses it. For **local** accounts only (`AuthContext.method === 'local'`) it also
|
|
renders a "Local admin password" card that calls `changePassword(current, new)`, mapping a wrong-
|
|
current-password **401** to the server ProblemDetails detail inline.
|
|
- **File-download endpoints go through a fetch-blob helper, never a browser tab.** Endpoints that
|
|
return a binary body (the troubleshooting archive / media-sample POSTs) must **not** go through
|
|
`request()` (it JSON-parses) and must **not** be opened with `window.open` (a session-cookie GET in a
|
|
new tab can't carry the CSRF header and bypasses error handling). Instead `fetch(path, { method:
|
|
'POST', headers: { 'X-Csrf': '1' } })`, then `response.blob()` → object URL → anchor-click →
|
|
revoke, reading the filename from `Content-Disposition`; a non-ok response throws an `ApiError`. See
|
|
`downloadTroubleshootingArchive` / `downloadTroubleshootingMediaSample` in `web/src/api/troubleshoot.ts`.
|
|
- **401 UX is one shell-level banner, not per-screen handling.** Rather than special-casing 401 in
|
|
every screen's error path, the request client emits a single app-wide signal on 401
|
|
(`auth.ts` `notifyUnauthorized` / `subscribeUnauthorized`), and `web/src/UnauthorizedBanner.tsx`
|
|
(rendered once in the App shell) subscribes and prompts re-login. This is the DRY seam for "the server
|
|
rejected us" — reuse it instead of adding bespoke 401 branches. Auth flows that expect a 401 as an
|
|
inline answer (login, change-password) pass `suppressUnauthorizedSignal` so they don't trip it.
|
|
|
|
## 6. Tests
|
|
|
|
- **vitest**, colocated `*.test.ts` / `*.test.tsx` next to the source file.
|
|
- Every screen with meaningful logic gets a screen test; every API client module gets a
|
|
param-mapping / URL-building test (e.g. `logs.test.ts` next to `logs.ts`).
|
|
- `web/src/App.test.tsx` covers composition, navigation, shell behavior, and integration regressions
|
|
such as sub-path-to-sub-path rendering and guarded popstate restoration. Detailed screen behavior
|
|
belongs in the screen's colocated test. `web/src/app/routes.test.tsx` independently pins stable
|
|
route-object identity and query/path matching.
|
|
- **Nav-label test-selector care**: `getByRole('link'/'button', { name: /Regex/ })` matches by
|
|
substring by default — a loose regex can match more than one nav item. Verified example: the
|
|
System nav button is matched with an **anchored** regex (`name: /^System/`) rather than a bare
|
|
`/System/`, specifically to avoid ambiguous matches against other labels that start with or
|
|
contain "System". Anchor (`^`/`$`) or use exact strings in `getByRole` name matchers whenever a
|
|
new label could be a substring of (or share a substring with) an existing one — check
|
|
`app/routes.tsx`'s nav `label:` list for collisions before picking a new label.
|
|
- **Extracted-screen tests own their own fetch mock** (Schedules #207, Channels #244, Playouts
|
|
#245): when a
|
|
screen is pulled out of `App.tsx` into `web/src/screens/<Name>Screen.tsx`, its colocated
|
|
`<Name>Screen.test.tsx` builds a **self-contained** `vi.spyOn(window, 'fetch')` mock scoped to
|
|
that screen's own endpoints (plus local `jsonResponse`/fixture-factory helpers) and renders the
|
|
screen component **directly** — it must not import from `App.test.tsx` or reuse the monolithic
|
|
`mockDashboardApi()`. `App.test.tsx` keeps only a thin **nav-smoke** test for the extracted
|
|
screen (route to it, assert it mounted and hit its own endpoint) plus genuinely cross-cutting
|
|
shell concerns (route table, sub-path ownership §2, the unsaved-changes/popstate guard §8, the
|
|
TopBar action wiring). Detailed screen behavior lives in the screen's own test file. See
|
|
`ChannelsScreen.test.tsx` / `SchedulesScreen.test.tsx` / `PlayoutsScreen.test.tsx` for the shape.
|
|
|
|
## 7. Verification gate — run before every commit touching `web/`
|
|
|
|
From `web/`:
|
|
```bash
|
|
npm test # vitest
|
|
npm run lint # eslint .
|
|
npm run build # tsc -b && vite build
|
|
```
|
|
Also run `npm run check:api` if you touched anything OpenAPI-relevant (see `api-conventions.md` §5)
|
|
— it regenerates `src/api/generated/v1.d.ts` and fails the build if it's out of sync with what's
|
|
committed.
|
|
|
|
## 8. Unsaved-changes navigation guard
|
|
|
|
Screens with a **draft / explicit-Save** model (edits accumulate locally, one Save flushes them —
|
|
e.g. the schedules editor, `web/src/screens/SchedulesScreen.tsx`) must guard against losing the
|
|
draft to navigation. The shared module `web/src/navigationGuard.ts` is the seam:
|
|
|
|
- The screen **registers a guard** on mount: `registerNavigationGuard(() => !dirtyRef.current ||
|
|
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.
|
|
- **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.
|
|
|
|
- **Guarded sub-path routes: App owns pathname/popstate (resolves the old §2 caveat).** A sub-path
|
|
wrapper that BOTH tracks its own pathname (§2) AND whose sub-screens register a dirty guard cannot
|
|
self-listen for `popstate`: React commits **child** passive effects before the **parent**, so a
|
|
wrapper-owned `popstate` listener fires *before* App's guard-restore handler and would switch
|
|
sub-screen before App could veto and re-push — desyncing the two. **Resolution (design #202 §D.2,
|
|
finding 4):** App is the single `popstate` owner. On a pop it consults `canLeaveCurrentScreen()`,
|
|
and **only on approval** does it update a `currentSubPath` state value (`librariesSubPath`) that it
|
|
passes **down** into the wrapper; on a veto it re-pushes the pre-pop path and updates nothing, so
|
|
the wrapper never sees a vetoed path. The wrapper (`LibrariesRouteScreen`) derives its sub-path
|
|
purely from that prop — never from the raw event — so effect-commit order is irrelevant. **Scope
|
|
the state write to the guarded route only:** the `setLibrariesSubPath` call is gated on the arrived
|
|
route being `libraries`, so unguarded sub-path routes (Playouts/Media) skip it entirely and stay
|
|
byte-identical (they still bail App's `setActiveRoute` via `Object.is` and self-own their pathname
|
|
per §2). The regression test is the App-owned-popstate case in `App.test.tsx` (dirty editor at
|
|
`/app/libraries/local/3`: `confirm→false` keeps URL + mounted sub-screen; `confirm→true`
|
|
navigates). Unguarded sub-path screens (`PlayoutsRouteScreen`, `MediaRouteScreen`) keep
|
|
self-owning their pathname — only screens that register a dirty guard defer to App.
|
|
- 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.
|
|
|
|
- **Successful save + same-callback navigation:** `useDirtyGuard` returns a `markClean` callback for
|
|
the narrow case where a successful save queues the new draft/baseline and immediately calls
|
|
`navigateToPath(...)` in that same promise callback. React has not committed the clean render before
|
|
the synthetic `popstate`, so call `markClean()` after the durable save succeeds and immediately before
|
|
navigating. Still queue the real clean draft/baseline state; the callback only transfers the synchronous
|
|
guard truth across that one event boundary. Only do this while the successful request still owns the
|
|
current draft: disable or otherwise gate draft mutations for the entire in-flight save, or revision-check
|
|
the completion before marking clean. Otherwise the completion can discard edits made after the request
|
|
started. Do not replace the callback with a timer or a forced render. Screens that remain mounted after
|
|
saving do not need the callback.
|
|
|
|
## 9. Review checklist — temporal semantics
|
|
|
|
- **For every effect / timer / async completion, ask: *when* does it fire (mount, dependency change,
|
|
unmount, StrictMode double-invoke) and *which* render/request does it still own?** A debounce timer
|
|
fires on mount too (§3, the `lastQueryRef` no-change guard exists precisely for that); a `.then`
|
|
can resolve after the params it was launched for have moved on (§3a, the `refreshing` gate and the
|
|
Add-all query binding exist for that). A guard that only checks "still mounted" (`activeRef`) does
|
|
not answer "still current".
|
|
- **For any "make X consistent with Y" change, re-validate the exemplar Y's temporal behavior before
|
|
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/AppShell.tsx`) renders at most **one** primary-action button (top-right, Plus
|
|
icon) for the active screen. `App.tsx` wraps the shell and matched screen in the minimal
|
|
`PrimaryActionProvider` from `web/src/primaryAction.ts`: the active screen explicitly registers one
|
|
handler, and the TopBar reads that matching registration directly from React context. No window event,
|
|
string-keyed dispatcher, or generic screen-action framework participates in normal screen actions.
|
|
|
|
- **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 provider owns exactly one registration.** Each mounted hook gets an opaque owner token; registering a
|
|
newer screen replaces the previous registration, and cleanup clears only the registration owned by that
|
|
hook. A stale unmount therefore cannot erase the newer screen's action. The hook keeps the latest handler in
|
|
a ref, so ordinary screen renders update behavior without reclaiming/churning ownership. Directly-rendered
|
|
screen tests outside the provider remain harmless; there is simply no shell action consumer.
|
|
- **The route must ALSO declare a matching non-empty `primaryAction` label** in the stable
|
|
`app/routes.tsx` table. The TopBar renders the button only when that label is non-empty **and** the active
|
|
route owns a matching registration — metadata alone can no longer produce a dead button, while a
|
|
registration without a label remains intentionally invisible. 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. `primaryAction.test.tsx` separately pins matching,
|
|
latest-handler, route-change, cleanup, and stale-owner semantics. 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.
|