timothy/ersatztv
1
0
Fork 0
Code Issues 52 Pull Requests 6 Actions 2 Packages Projects Releases 9 Wiki Activity

Extract ChicoryTV shell/routing and replace implicit TopBar action coupling #247

New Issue
Closed
opened 2026-07-11 12:50:17 +02:00 by timothy · 3 comments
timothy commented 2026-07-11 12:50:17 +02:00
Owner
Copy Link
Copy Source

Child of #243. Final phase after the domain screens are extracted.

HARD PREREQUISITE: #238 must land before this issue starts. Removing the ctv:primary-action CustomEvent (below) is only safe once #238 has established a working per-screen primary-action delivery path — today that global event is the sole delivery path and only SchedulesScreen listens for it (SchedulesScreen.tsx:183), so deleting it before #238 breaks Schedules' one working primary button. #238 is currently open/unowned (triaged backlog) — it must be scheduled and completed first; do not start #247 while #238 is open. (Verified in the plan review, adversarial-reviewer#24, 2026-07-11.)

Scope

Turn App.tsx into a small composition root by extracting:

  • route metadata/matching and nav-group definitions;
  • Sidebar, TopBar, Connect menu, version display, and theme switcher;
  • media/playout sub-route wrappers and screen dispatch;
  • explicit screen-to-shell primary action contract.

Remove the string-keyed window CustomEvent mechanism (ctv:primary-action) once #238 has established/fixed the intended action behavior. Prefer an explicit action registration/context or route-render contract with cleanup and ownership tests. Keep the replacement minimal — one declared action per screen with cleanup/ownership tests — not a generic screen-action dispatch framework (the epic's non-goals forbid a speculative generic framework before repeated screens prove the abstraction).

Preservation invariants (must not silently break during the shell extraction)

  • Route table stays ONE stable module-level array of shared ScreenRoute object references. routeFromLocation() (App.tsx:596-612) returns the same object reference for an allowSubPaths base path and every sub-path under it; App's setActiveRoute then bails via Object.is, which is why sub-path screens own their own popstate (spa-conventions §2). If route metadata is refactored into per-screen registration that rebuilds route objects per render, this bail breaks and navigating /blocks/42 → /blocks/17 stops re-rendering — with no test failure unless one exists. Add a regression test that navigates between two sub-paths of one allowSubPaths route and asserts the correct sub-screen re-renders.
  • Keep the navigate + popstate handlers + currentPathRef + activeRoute state co-located in the composition root (App.tsx:2954-3003). They close over each other; the #230 re-push-on-veto (finding 1) restores currentPathRef.current to undo an already-committed browser URL change. They cannot be split across screen modules. navigationGuard.ts is already standalone and survives extraction unchanged.

Sequencing

Land after #238 (hard prerequisite, above) and after #230's dirty popstate/navigation contract — which is already settled in-code (PRs f09045e1+11b43d5e via #242): popstate re-push, beforeunload, ref-based guard, shuffle-toggle reload all present on main. This issue must preserve those settled semantics, not invent replacements in parallel.

Acceptance criteria

  • App.tsx is a small composition root with no domain-specific screen implementation.
  • Every visible TopBar primary action is either functional or intentionally absent; no dead metadata-only button (i.e. #238 is resolved).
  • Browser back/forward, app navigation, dirty guards, unknown routes, query-only routes, and allowSubPaths behavior have focused shell/routing tests. Explicitly: the §2 sub-path reference-equality re-render and the #230 popstate re-push each retain a passing regression test against the composition-root shell — these are the two behaviors most likely to break invisibly.
  • No global window event is required for normal screen actions.
  • Route objects/components do not rely on reference-equality accidents for sub-path updates. (The stable-array invariant above is the intended mechanism, not an accident — keep it deliberate and tested.)
  • docs/spa-conventions.md and docs/decisions.md describe the final screen/shell/action ownership model.
  • No new routing dependency unless separately proposed and approved.
  • Full web test, lint, typecheck/build, and API-generation check pass.

Rollback

Structural-only and independently revertible after its prerequisite behavior fixes.

#238 promoted from soft "land after" to a HARD blocking prerequisite, and the two silent-break invariants (INV-1/INV-2) added, per the plan review on adversarial-reviewer#24 (2026-07-11).

Done-when

  • adversarial review passed on the current PR head
  • App.tsx is a small composition root with no domain-specific screen implementation
  • shell/routing extraction preserves route identity, sub-path ownership, navigation guards, unknown routes, and query-only routes with focused tests
  • TopBar primary actions use an explicit one-action-per-screen ownership contract with cleanup tests and no normal-path window CustomEvent
  • docs/spa-conventions.md and docs/decisions.md document the final shell/action ownership model
  • full web tests, lint, typecheck/build, and generated-API check pass
  • PR CI is green with no API, route, styling, dependency, or intentional behavior change
Child of https://gitea.tblindustries.be/timothy/ersatztv/issues/243. Final phase after the domain screens are extracted. > **HARD PREREQUISITE: #238 must land before this issue starts.** Removing the `ctv:primary-action` CustomEvent (below) is only safe once #238 has established a working per-screen primary-action delivery path — today that global event is the *sole* delivery path and only `SchedulesScreen` listens for it (`SchedulesScreen.tsx:183`), so deleting it before #238 breaks Schedules' one working primary button. #238 is currently open/unowned (triaged backlog) — it must be scheduled and completed first; do not start #247 while #238 is open. (Verified in the plan review, [adversarial-reviewer#24](https://gitea.tblindustries.be/timothy/adversarial-reviewer/issues/24), 2026-07-11.) ## Scope Turn `App.tsx` into a small composition root by extracting: - route metadata/matching and nav-group definitions; - Sidebar, TopBar, Connect menu, version display, and theme switcher; - media/playout sub-route wrappers and screen dispatch; - explicit screen-to-shell primary action contract. Remove the string-keyed `window` CustomEvent mechanism (`ctv:primary-action`) once #238 has established/fixed the intended action behavior. Prefer an explicit action registration/context or route-render contract with cleanup and ownership tests. **Keep the replacement minimal** — one declared action per screen with cleanup/ownership tests — **not** a generic screen-action dispatch framework (the epic's non-goals forbid a speculative generic framework before repeated screens prove the abstraction). ## Preservation invariants (must not silently break during the shell extraction) - **Route table stays ONE stable module-level array of shared `ScreenRoute` object references.** `routeFromLocation()` (`App.tsx:596-612`) returns the *same object reference* for an `allowSubPaths` base path and every sub-path under it; `App`'s `setActiveRoute` then bails via `Object.is`, which is *why* sub-path screens own their own `popstate` (spa-conventions §2). If route metadata is refactored into per-screen registration that rebuilds route objects per render, this bail breaks and navigating `/blocks/42 → /blocks/17` stops re-rendering — with no test failure unless one exists. **Add a regression test** that navigates between two sub-paths of one `allowSubPaths` route and asserts the correct sub-screen re-renders. - **Keep the `navigate` + `popstate` handlers + `currentPathRef` + `activeRoute` state co-located in the composition root** (`App.tsx:2954-3003`). They close over each other; the #230 re-push-on-veto (finding 1) restores `currentPathRef.current` to undo an already-committed browser URL change. They cannot be split across screen modules. `navigationGuard.ts` is already standalone and survives extraction unchanged. ## Sequencing Land after **#238** (hard prerequisite, above) and after **#230**'s dirty popstate/navigation contract — which is **already settled in-code** (PRs `f09045e1`+`11b43d5e` via #242): popstate re-push, `beforeunload`, ref-based guard, shuffle-toggle reload all present on `main`. This issue must preserve those settled semantics, not invent replacements in parallel. ## Acceptance criteria - `App.tsx` is a small composition root with no domain-specific screen implementation. - Every visible TopBar primary action is either functional or intentionally absent; no dead metadata-only button (i.e. #238 is resolved). - Browser back/forward, app navigation, dirty guards, unknown routes, query-only routes, and `allowSubPaths` behavior have focused shell/routing tests. **Explicitly: the §2 sub-path reference-equality re-render and the #230 popstate re-push each retain a passing regression test against the composition-root shell** — these are the two behaviors most likely to break invisibly. - No global window event is required for normal screen actions. - Route objects/components do not rely on reference-equality accidents for sub-path updates. (The stable-array invariant above is the intended mechanism, not an accident — keep it deliberate and tested.) - `docs/spa-conventions.md` and `docs/decisions.md` describe the final screen/shell/action ownership model. - No new routing dependency unless separately proposed and approved. - Full web test, lint, typecheck/build, and API-generation check pass. ## Rollback Structural-only and independently revertible after its prerequisite behavior fixes. --- *#238 promoted from soft "land after" to a HARD blocking prerequisite, and the two silent-break invariants (INV-1/INV-2) added, per the plan review on [adversarial-reviewer#24](https://gitea.tblindustries.be/timothy/adversarial-reviewer/issues/24) (2026-07-11).* ## Done-when - [x] adversarial review passed on the current PR head - [x] `App.tsx` is a small composition root with no domain-specific screen implementation - [x] shell/routing extraction preserves route identity, sub-path ownership, navigation guards, unknown routes, and query-only routes with focused tests - [x] TopBar primary actions use an explicit one-action-per-screen ownership contract with cleanup tests and no normal-path window CustomEvent - [x] `docs/spa-conventions.md` and `docs/decisions.md` document the final shell/action ownership model - [x] full web tests, lint, typecheck/build, and generated-API check pass - [x] PR CI is green with no API, route, styling, dependency, or intentional behavior change
timothy added this to the ChicoryTV modularization milestone 2026-07-11 12:50:17 +02:00
timothy added the enhancementpriority: mediumfrontend labels 2026-07-11 12:50:17 +02:00
timothy commented 2026-07-12 12:21:56 +02:00
Author
Owner
Copy Link
Copy Source

New preservation invariant + non-goal from the independent plan re-review (adversarial-reviewer#24, 2026-07-12, code-verified against main @ 9e2d1608). The body's INV-1/INV-2 line numbers are also stale — current refs below.

INV-2b (NEW, load-bearing) — App-owned popstate for guarded sub-path routes. #202 landed a third sub-path pattern this issue's body doesn't mention. LibrariesRouteScreen (App.tsx 2593-2617) does NOT self-listen for popstate; it takes its sub-path as a prop (subPath) and derives everything via parseLibrariesSubRoute. App is the single popstate owner for it and writes librariesSubPath state (declared 2765) only on approval (2794-2796 in the popstate handler, 2828-2830 in navigate), gated on the arrived route being libraries; on a veto it re-pushes and writes nothing. Reason: React commits child effects before the parent, so a wrapper-owned listener would switch sub-screen before App's guard could veto — desyncing them (spa-conventions.md §8; decisions.md 2026-07-11 §D.2 finding 4). #247 must preserve: App as sole popstate owner, the route-id-gated librariesSubPath write, and the wrapper deriving sub-path from the prop (never the event). Keep the existing App.test.tsx App-owned-popstate regression test (dirty editor at /app/libraries/local/3: confirm→false keeps URL+sub-screen; confirm→true navigates) green against the extracted shell.

INV-4 update — there are now TWO self-owning wrappers, not one. PlayoutsRouteScreen (2522-2542) and MediaRouteScreen (2548-2580, new since the first review — owns both pathname and search state + own popstate). Each moves as a unit. So #247 must preserve three sub-path mechanisms: two self-owning wrappers + one App-owned wrapper (INV-2b) + the key={window.location.pathname|search} remount used by ~8 other allowSubPaths screens (2645-2745).

G-4 (NEW non-goal) — do NOT unify the wrapper vs. key-remount sub-path patterns. Their coexistence is deliberate; collapsing them into one generic mechanism during the shell extraction is exactly the "speculative generic framework" the epic forbids and would risk both the Object.is bail (INV-1) and the guarded-route contract (INV-2b). Add to this issue's non-goals.

PlaceholderScreen fallthrough (2752). The route-table ↔ if-ladder dispatch (2619-2753) is not exhaustiveness-checked — a moved route missing its branch silently renders PlaceholderScreen, no test failure. Recommend adding a per-route "renders its screen, not the placeholder" test (or an exhaustive switch) so the shell extraction can't drop a route silently.

Refreshed INV-1/INV-2 line numbers: route table 216-579 (ScreenRoute iface 200-214); routeFromLocation() 622-638; nav/popstate/currentPathRef/activeRoute co-located in App() 2755-2846 (activeRoute 2757, currentPathRef 2770, popstate 2776-2802, #230 re-push 2783-2787, navigate 2806-2831). G-1 re-confirmed: #238 is still a hard prerequisite and still open+unowned — one dispatcher (920), one listener (SchedulesScreen.tsx:197-198), no fix commit.

**New preservation invariant + non-goal from the independent plan re-review** ([adversarial-reviewer#24](https://gitea.tblindustries.be/timothy/adversarial-reviewer/issues/24), 2026-07-12, code-verified against `main @ 9e2d1608`). The body's INV-1/INV-2 line numbers are also stale — current refs below. **INV-2b (NEW, load-bearing) — App-owned popstate for guarded sub-path routes.** #202 landed a *third* sub-path pattern this issue's body doesn't mention. `LibrariesRouteScreen` (App.tsx `2593-2617`) does **NOT** self-listen for `popstate`; it takes its sub-path as a **prop** (`subPath`) and derives everything via `parseLibrariesSubRoute`. **App is the single popstate owner** for it and writes `librariesSubPath` state (declared `2765`) **only on approval** (`2794-2796` in the popstate handler, `2828-2830` in `navigate`), **gated on the arrived route being `libraries`**; on a veto it re-pushes and writes nothing. Reason: React commits child effects before the parent, so a wrapper-owned listener would switch sub-screen *before* App's guard could veto — desyncing them (`spa-conventions.md` §8; `decisions.md` 2026-07-11 §D.2 finding 4). **#247 must preserve: App as sole `popstate` owner, the route-id-gated `librariesSubPath` write, and the wrapper deriving sub-path from the prop (never the event).** Keep the existing `App.test.tsx` App-owned-popstate regression test (dirty editor at `/app/libraries/local/3`: `confirm→false` keeps URL+sub-screen; `confirm→true` navigates) green against the *extracted* shell. **INV-4 update — there are now TWO self-owning wrappers, not one.** `PlayoutsRouteScreen` (`2522-2542`) **and** `MediaRouteScreen` (`2548-2580`, new since the first review — owns **both** `pathname` and `search` state + own `popstate`). Each moves as a unit. So #247 must preserve **three** sub-path mechanisms: two self-owning wrappers + one App-owned wrapper (INV-2b) + the `key={window.location.pathname|search}` remount used by ~8 other `allowSubPaths` screens (`2645-2745`). **G-4 (NEW non-goal) — do NOT unify the wrapper vs. `key`-remount sub-path patterns.** Their coexistence is deliberate; collapsing them into one generic mechanism during the shell extraction is exactly the "speculative generic framework" the epic forbids and would risk both the Object.is bail (INV-1) and the guarded-route contract (INV-2b). Add to this issue's non-goals. **PlaceholderScreen fallthrough (`2752`).** The route-table ↔ `if`-ladder dispatch (`2619-2753`) is not exhaustiveness-checked — a moved route missing its branch silently renders `PlaceholderScreen`, no test failure. Recommend adding a per-route "renders its screen, not the placeholder" test (or an exhaustive `switch`) so the shell extraction can't drop a route silently. **Refreshed INV-1/INV-2 line numbers:** route table `216-579` (`ScreenRoute` iface `200-214`); `routeFromLocation()` `622-638`; nav/popstate/`currentPathRef`/`activeRoute` co-located in `App()` `2755-2846` (activeRoute `2757`, currentPathRef `2770`, popstate `2776-2802`, #230 re-push `2783-2787`, navigate `2806-2831`). **G-1 re-confirmed: #238 is still a hard prerequisite and still open+unowned** — one dispatcher (`920`), one listener (`SchedulesScreen.tsx:197-198`), no fix commit.
timothy added the in-progress label 2026-07-15 22:06:29 +02:00
timothy commented 2026-07-15 22:06:34 +02:00
Author
Owner
Copy Link
Copy Source

Claiming #247 for implementation in this session. I will preserve the documented shell/routing invariants, keep the extraction behavior-preserving, and follow the required review, verification, and tracker session-end workflow.

Claiming #247 for implementation in this session. I will preserve the documented shell/routing invariants, keep the extraction behavior-preserving, and follow the required review, verification, and tracker session-end workflow.
timothy commented 2026-07-16 13:42:57 +02:00
Author
Owner
Copy Link
Copy Source

Done

What was done: PR #361 merged at 82467f188bcf09834e21b8fd5160c94e5d859ede. App.tsx is now the small composition root; stable route metadata/nav moved to app/routes.tsx, shell chrome to app/AppShell.tsx, and exhaustive wrappers/dispatch to app/ScreenContent.tsx. The global ctv:primary-action event was replaced by a minimal one-action React context with latest-handler and opaque-owner cleanup semantics. Builder, Settings, and component behavior coverage was colocated while App retained route/shell/guard composition smokes.

Root cause: n/a — planned behavior-preserving structural work.

Files changed: web/src/App.tsx, web/src/app/{routes,AppShell,ScreenContent}.tsx, web/src/primaryAction.ts, focused App/route/action/screen/component tests, docs/spa-conventions.md, and docs/decisions.md.

Verification: 90 web test files / 775 tests; full lint, typecheck, production build, and API-generation check; full UTC .NET restore/build/test; hosted-SPA live E2E covering setup/login, shell/Connect, functional Schedule primary action, sibling Playout subpaths, query-only stability, search, and unknown routes with no browser errors; cold review MERGEABLE at 947c8aacd899ff1ee3c99d283860dd7a287644c4; CI run 648 green (image publish skipped as expected).

Deferred: none.

Follow-up issues: none.

Docs updated: docs/spa-conventions.md and append-only docs/decisions.md record the final shell/routing/action ownership model. No API, route, styling, dependency, or intentional behavior change.

## Done **What was done**: PR #361 merged at `82467f188bcf09834e21b8fd5160c94e5d859ede`. `App.tsx` is now the small composition root; stable route metadata/nav moved to `app/routes.tsx`, shell chrome to `app/AppShell.tsx`, and exhaustive wrappers/dispatch to `app/ScreenContent.tsx`. The global `ctv:primary-action` event was replaced by a minimal one-action React context with latest-handler and opaque-owner cleanup semantics. Builder, Settings, and component behavior coverage was colocated while App retained route/shell/guard composition smokes. **Root cause**: n/a — planned behavior-preserving structural work. **Files changed**: `web/src/App.tsx`, `web/src/app/{routes,AppShell,ScreenContent}.tsx`, `web/src/primaryAction.ts`, focused App/route/action/screen/component tests, `docs/spa-conventions.md`, and `docs/decisions.md`. **Verification**: 90 web test files / 775 tests; full lint, typecheck, production build, and API-generation check; full UTC .NET restore/build/test; hosted-SPA live E2E covering setup/login, shell/Connect, functional Schedule primary action, sibling Playout subpaths, query-only stability, search, and unknown routes with no browser errors; cold review MERGEABLE at `947c8aacd899ff1ee3c99d283860dd7a287644c4`; CI run 648 green (image publish skipped as expected). **Deferred**: none. **Follow-up issues**: none. **Docs updated**: `docs/spa-conventions.md` and append-only `docs/decisions.md` record the final shell/routing/action ownership model. No API, route, styling, dependency, or intentional behavior change.
timothy removed the in-progress label 2026-07-16 13:42:57 +02:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
ad-hoc
One-off / ad-hoc work not tracked by a dedicated issue
 api
REST API / HTTP endpoints
 bug
Something isn't working
 ci-cd
Build, test, deploy pipeline
 content
Channel content / schedules / playlists
 dependencies
Dependency updates (Renovate)
 enhancement
New feature or improvement
 frontend
ChicoryTV React SPA frontend
 in-progress
Claimed by an active session — do not pick up
 jellyfin
Jellyfin tuner / IPTV integration
 parked
Excluded from automatic queue pickup; work only when explicitly selected
 priority: high
priority: low
priority: medium
review
Adversarial review finding
 security
Security / vulnerability fix
No labels enhancement frontend priority: medium
Milestone
No items
No Milestone ChicoryTV modularization
Projects
Clear projects
No projects
Assignees
Clear assignees
renovate timothy (Timothy)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: timothy/ersatztv#247