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

Fix SPA foundation review findings (#78-#81 follow-up) #95

New Issue
Closed
opened 2026-07-02 18:31:12 +02:00 by timothy · 1 comment
timothy commented 2026-07-02 18:31:12 +02:00
Owner
Copy Link
Copy Source

Follow-up from a high-effort code review of the SPA foundation (#78 scaffold, #79 themes, #80 primitives, #81 API client), commits c0b20a0a..d1cc12e0. Verified by an independent adversarial pass; ranked most-severe first.

Functional — API client (web/src/api/client.ts, auth.ts)

  • 1. Empty-body 200 crashes request() (client.ts:59)
    Only 204 is special-cased, but many .NET write endpoints return Ok() with an empty 200 body. await response.json() throws "Unexpected end of JSON input", so a successful call surfaces as a failure. Treat empty/non-JSON 2xx bodies as no content (check content-length/content-type, or catch the parse).
  • 2. Server error messages masked for application/problem+json (client.ts:98)
    readProblemDetails only parses bodies whose Content-Type contains application/json. .NET emits ProblemDetails (422/404, and the 401 "A valid API key is required for write requests.") as application/problem+json, which fails the substring check → real detail dropped, UI shows generic "Request failed with status N." Match problem+json too.
  • 3. Duplicate/conflicting Content-Type header (client.ts:91)
    headers['Content-Type'] ??= 'application/json' uses a capitalized key while headersToRecord lowercases keys; a caller-supplied content-type isn't detected and a second Content-Type is appended → 415/400. Normalize header-key casing before defaulting.
  • 4. getStoredApiKey accesses localStorage unguarded (auth.ts:4)
    No try/catch (unlike designSystem.ts getStorage()); Safari private mode / sandboxed iframe / no-window makes every mutating request throw before fetch. Reuse the defensive getStorage() pattern.

UX / UI

  • 5. Flash-of-wrong-theme on reload (index.html:2)
    Static data-theme="warm" was removed (#79); theme now applies only in a post-mount useEffect, so Cool/Dual users see a warm→cool snap every load. Set the persisted theme pre-paint (inline head script or restore a boot attribute).
  • 6. Toast renders Info icon for warn/error (feedback.tsx:50)
    tone === 'ok' ? Check : Info — error toasts get a neutral info glyph. Map warn→triangle-alert, error→circle-x (or similar).

Type generator (web/scripts/generate-openapi-types.mjs)

  • 7. Method loop treats every path-item key as an HTTP method (:131)
    Path-level parameters/summary, or two ops sharing an operationId, produce bogus/duplicate entries → generated v1.d.ts fails typecheck and breaks the build. Restrict to known HTTP-method keys.
  • 8. Large unused operations interface emitted (:130)
    Nothing imports operations (only components is used); dead code inflating the committed artifact and every check:api diff. Drop it until a consumer needs it.

Refuted (not a defect): duplicated classNames() helper across 4 component modules.

Related: #78/#79/#80/#81 (source), epic #59.

Follow-up from a high-effort code review of the SPA foundation (#78 scaffold, #79 themes, #80 primitives, #81 API client), commits `c0b20a0a..d1cc12e0`. Verified by an independent adversarial pass; ranked most-severe first. ## Functional — API client (`web/src/api/client.ts`, `auth.ts`) - [ ] **1. Empty-body 200 crashes `request()`** (`client.ts:59`) Only 204 is special-cased, but many .NET write endpoints return `Ok()` with an empty 200 body. `await response.json()` throws *"Unexpected end of JSON input"*, so a successful call surfaces as a failure. Treat empty/non-JSON 2xx bodies as no content (check `content-length`/`content-type`, or catch the parse). - [ ] **2. Server error messages masked for `application/problem+json`** (`client.ts:98`) `readProblemDetails` only parses bodies whose Content-Type contains `application/json`. .NET emits ProblemDetails (422/404, and the 401 "A valid API key is required for write requests.") as `application/problem+json`, which fails the substring check → real detail dropped, UI shows generic *"Request failed with status N."* Match `problem+json` too. - [ ] **3. Duplicate/conflicting `Content-Type` header** (`client.ts:91`) `headers['Content-Type'] ??= 'application/json'` uses a capitalized key while `headersToRecord` lowercases keys; a caller-supplied `content-type` isn't detected and a second `Content-Type` is appended → 415/400. Normalize header-key casing before defaulting. - [ ] **4. `getStoredApiKey` accesses `localStorage` unguarded** (`auth.ts:4`) No try/catch (unlike `designSystem.ts` `getStorage()`); Safari private mode / sandboxed iframe / no-window makes every mutating request throw before `fetch`. Reuse the defensive `getStorage()` pattern. ## UX / UI - [ ] **5. Flash-of-wrong-theme on reload** (`index.html:2`) Static `data-theme="warm"` was removed (#79); theme now applies only in a post-mount `useEffect`, so Cool/Dual users see a warm→cool snap every load. Set the persisted theme pre-paint (inline head script or restore a boot attribute). - [ ] **6. Toast renders Info icon for `warn`/`error`** (`feedback.tsx:50`) `tone === 'ok' ? Check : Info` — error toasts get a neutral info glyph. Map `warn`→triangle-alert, `error`→circle-x (or similar). ## Type generator (`web/scripts/generate-openapi-types.mjs`) - [ ] **7. Method loop treats every path-item key as an HTTP method** (`:131`) Path-level `parameters`/`summary`, or two ops sharing an `operationId`, produce bogus/duplicate entries → generated `v1.d.ts` fails typecheck and breaks the build. Restrict to known HTTP-method keys. - [ ] **8. Large unused `operations` interface emitted** (`:130`) Nothing imports `operations` (only `components` is used); dead code inflating the committed artifact and every `check:api` diff. Drop it until a consumer needs it. _Refuted (not a defect): duplicated `classNames()` helper across 4 component modules._ Related: #78/#79/#80/#81 (source), epic #59.
timothy added the frontend label 2026-07-02 18:31:12 +02:00
timothy commented 2026-07-02 18:37:10 +02:00
Author
Owner
Copy Link
Copy Source

Done

What was done: Hardened the SPA foundation findings: successful empty/non-JSON 2xx API responses now return no content instead of throwing; ProblemDetails parsing accepts application/problem+json; request headers are normalized before defaulting JSON content type; API key storage reads/writes tolerate unavailable localStorage; persisted theme is applied pre-paint in index.html; warning/error toasts use semantic icons; the OpenAPI generator is testable, no longer emits the unused operations interface, and regenerated v1.d.ts reflects that.

Root cause: The SPA foundation assumed ideal browser/API responses: JSON on every successful 2xx, only application/json error payloads, stable localStorage access, React-mount timing for theme application, and path-item keys all being OpenAPI operations. Those assumptions did not hold for .NET empty Ok() responses, ProblemDetails content types, restricted storage contexts, reload paint timing, or OpenAPI path-level metadata.

Files changed: web/src/api/client.ts, web/src/api/client.test.ts, web/src/api/auth.ts, web/src/api/auth.test.ts, web/index.html, web/src/components/feedback.tsx, web/src/App.test.tsx, web/scripts/generate-openapi-types.mjs, web/scripts/generate-openapi-types.test.mjs, web/src/api/generated/v1.d.ts.

Verification: npm test -- --run (29 tests passed); npm run typecheck; npm run lint; npm run check:api; npm run build.

Deferred: n/a

Follow-up issues: n/a

Docs updated: no docs/skills impact

## Done **What was done**: Hardened the SPA foundation findings: successful empty/non-JSON 2xx API responses now return no content instead of throwing; ProblemDetails parsing accepts `application/problem+json`; request headers are normalized before defaulting JSON content type; API key storage reads/writes tolerate unavailable localStorage; persisted theme is applied pre-paint in `index.html`; warning/error toasts use semantic icons; the OpenAPI generator is testable, no longer emits the unused `operations` interface, and regenerated `v1.d.ts` reflects that. **Root cause**: The SPA foundation assumed ideal browser/API responses: JSON on every successful 2xx, only `application/json` error payloads, stable localStorage access, React-mount timing for theme application, and path-item keys all being OpenAPI operations. Those assumptions did not hold for .NET empty `Ok()` responses, ProblemDetails content types, restricted storage contexts, reload paint timing, or OpenAPI path-level metadata. **Files changed**: `web/src/api/client.ts`, `web/src/api/client.test.ts`, `web/src/api/auth.ts`, `web/src/api/auth.test.ts`, `web/index.html`, `web/src/components/feedback.tsx`, `web/src/App.test.tsx`, `web/scripts/generate-openapi-types.mjs`, `web/scripts/generate-openapi-types.test.mjs`, `web/src/api/generated/v1.d.ts`. **Verification**: `npm test -- --run` (29 tests passed); `npm run typecheck`; `npm run lint`; `npm run check:api`; `npm run build`. **Deferred**: n/a **Follow-up issues**: n/a **Docs updated**: no docs/skills impact
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 frontend
Milestone
No items
No Milestone
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#95