Adds docs/README.md (index), api-conventions.md, spa-conventions.md, e2e-local.md + scripts/e2e-local.sh, blazor-route-parity.md (#91 phase-b tracker), domain-model.md, decisions.md. Rule (CLAUDE.md): read these at session start instead of re-recon; update in the same PR that changes a convention/route/decision. Part 2 = #185. Handoff: #180/#141/#158/#161 closed (PRs #181/#182/#183); #145 playback-only; #91 readiness plan posted; next prompt = #185 + quick wins + #155/#151/#152/#153. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
113 lines
6.5 KiB
Markdown
113 lines
6.5 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`.
|
|
|
|
- **Routes + nav**: `web/src/App.tsx` — one big route table of `ScreenRoute` objects (`path`,
|
|
`label`, `title`, `kicker`, `icon`, etc.) plus an `allowSubPaths?: boolean` flag.
|
|
- **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 `App.tsx` re-rendering `ScreenContent` when the sub-path changes.
|
|
|
|
**Why**: `App.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 `App.tsx` (~line 3540) 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 in `App.tsx`
|
|
(which owns two sibling sub-paths, `/playouts/{id}/alternate-schedules` and
|
|
`/playouts/{id}/templates`, dispatching internally via `parsePlayoutSubRoute`).
|
|
|
|
## 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`.
|
|
|
|
## 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.
|
|
|
|
## 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.
|
|
|
|
## 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 navigation + the route table, including regressions like the
|
|
sub-path bug in §2 (see the tests around `PlayoutsRouteScreen`, ~line 1682+, that click into
|
|
`/app/playouts/{id}/...` sub-paths and assert the correct sub-screen rendered).
|
|
- **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.tsx`'s nav `label:` list for collisions before picking a new label.
|
|
|
|
## 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.
|