Files
gridpilot.gg/plans/media-streamlining-debug-fix-plan.md
2025-12-31 15:39:28 +01:00

365 lines
16 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
# Media streamlining debug fix plan
Goal: make media rendering (avatars, team logos, league logos) deterministic, debuggable, and boring. Remove misleading stubs from runtime, converge on one URL shape (`/media/...`) end-to-end, and add observability so broken images can be diagnosed in minutes.
Non-goals:
- No CDN rollout (we still design for it).
- No “AI generation” pipeline. Keep existing deterministic SVG generation in [`MediaGenerationService`](core/media/domain/services/MediaGenerationService.ts:9).
## 1) Current state (facts from code)
### Backend (API)
- The canonical HTTP routes exist in [`MediaController`](apps/api/src/domain/media/MediaController.ts:25):
- Team logo: `GET /media/teams/:teamId/logo` (SVG) [`getTeamLogo()`](apps/api/src/domain/media/MediaController.ts:72)
- League logo: `GET /media/leagues/:leagueId/logo` (SVG) [`getLeagueLogo()`](apps/api/src/domain/media/MediaController.ts:83)
- Driver avatar: `GET /media/avatar/:driverId` (SVG) [`getDriverAvatar()`](apps/api/src/domain/media/MediaController.ts:111)
- Default: `GET /media/default/:variant` (PNG placeholder) [`getDefaultMedia()`](apps/api/src/domain/media/MediaController.ts:125)
- Seeding sets `logoRef` for teams/leagues to “generated” references:
- Team: [`RacingTeamFactory.createTeams()`](adapters/bootstrap/racing/RacingTeamFactory.ts:26) sets [`MediaReference.generated()`](core/domain/media/MediaReference.ts:114) via line [`logoRef: MediaReference.generated('team', teamId)`](adapters/bootstrap/racing/RacingTeamFactory.ts:51)
- League: [`RacingLeagueFactory.create()`](adapters/bootstrap/racing/RacingLeagueFactory.ts:14) sets [`logoRef: MediaReference.generated('league', leagueData.id)`](adapters/bootstrap/racing/RacingLeagueFactory.ts:403)
- Presenters resolve `MediaReference` → URL string via a `MediaResolverPort`:
- Teams list: [`AllTeamsPresenter.present()`](apps/api/src/domain/team/presenters/AllTeamsPresenter.ts:25) resolves via [`this.mediaResolver.resolve()`](apps/api/src/domain/team/presenters/AllTeamsPresenter.ts:45)
### Frontend (Website)
- The landing page cards render with Next `Image`:
- Team card: [`TeamCard`](apps/website/components/teams/TeamCard.tsx:67) uses [`<Image src={logoUrl}>`](apps/website/components/teams/TeamCard.tsx:101)
- Some UI code uses an internal URL builder that does not match the APIs route shapes:
- [`getMediaUrl()`](apps/website/lib/utilities/media.ts:11) builds `/media/generated/team-logo/:id` etc.
- Example usage: [`TeamLadderRow`](apps/website/components/teams/TeamLadderRow.tsx:18) uses [`getMediaUrl('team-logo', teamId)`](apps/website/components/teams/TeamLadderRow.tsx:29)
- Next.js image config currently allows localhost and allows SVG:
- [`next.config.mjs`](apps/website/next.config.mjs:1) includes `remotePatterns` for `localhost:3001` and `dangerouslyAllowSVG: true`.
## 2) Suspected root causes (ranked)
### A. URL shape mismatch in Website fallback builder
The Website builder [`getMediaUrl()`](apps/website/lib/utilities/media.ts:11) generates paths like:
- `/media/generated/team-logo/:id`
But the API serves:
- `/media/teams/:id/logo` or `/media/generated/team/:id` (generic endpoint)
Result: 404s for any page that uses [`getMediaUrl()`](apps/website/lib/utilities/media.ts:11) instead of `logoUrl` returned by the API.
### B. Runtime accidentally uses the in-memory resolver (misleading)
In API Team DI, the runtime media resolver is currently the stub [`InMemoryMediaResolverAdapter`](adapters/media/MediaResolverInMemoryAdapter.ts:60) via [`TeamProviders`](apps/api/src/domain/team/TeamProviders.ts:28).
That adapter is explicitly described as “fake URLs” and has URL shapes that dont match the API controller, e.g. system-default returns `${base}/default/${ref.variant}` in [`InMemoryMediaResolverAdapter.resolve()`](adapters/media/MediaResolverInMemoryAdapter.ts:80).
Even if team logos are “generated” and map to `/media/teams/:id/logo`, this is an architectural footgun:
- It makes it easy for other entity presenters (drivers/leagues/etc.) to emit non-existent URLs.
- It undermines confidence when debugging.
### C. Next.js `Image` error symptoms
You reported: Next.js `Image` errors about remote host not configured and or SVG blocked.
Given [`next.config.mjs`](apps/website/next.config.mjs:12) appears to allow `localhost:3001` and enables SVG, this suggests at least one of:
- The actual `src` host differs (e.g. `127.0.0.1`, `api:3000`, or another hostname).
- The `src` is not a valid URL string at runtime (empty string, malformed).
- A stale container is running with older config.
The plan below makes `src` always same-origin to the Website (relative `/media/...`), eliminating this entire class of errors.
## 3) Target architecture (strict, minimal, easy-to-reason)
### 3.1 Invariants (rules)
1) Canonical media URLs are always *paths* starting with `/media/`.
2) API DTO fields like `team.logoUrl` are either:
- `null`, or
- a path `/media/...` (never absolute URLs, never empty string).
3) The Website renders media using *only*:
- DTO-provided `/media/...` URLs, or
- a single shared Website builder that produces `/media/...` URLs matching the API routes.
4) The Website never needs to know `http://localhost:3001`.
5) All runtime resolution uses exactly one resolver implementation (no stubs).
### 3.2 One canonical path schema
Canonical HTTP paths (served by API, fetched by browser via Website proxy rewrite):
- Team logo SVG: `/media/teams/{teamId}/logo`
- League logo SVG: `/media/leagues/{leagueId}/logo`
- Driver avatar SVG: `/media/avatar/{driverId}`
- Defaults (PNG): `/media/default/{variant}`
- Uploaded: `/media/uploaded/{mediaId}`
`/media/generated/:type/:id` can remain, but should become an internal alias only (not returned by resolvers/presenters).
### 3.3 Single resolver for the whole API
- Runtime resolver: [`MediaResolverAdapter`](adapters/media/MediaResolverAdapter.ts:53) using the concrete sub-resolvers:
- [`DefaultMediaResolverAdapter`](adapters/media/resolvers/DefaultMediaResolverAdapter.ts:34)
- [`GeneratedMediaResolverAdapter`](adapters/media/resolvers/GeneratedMediaResolverAdapter.ts:35)
- [`UploadedMediaResolverAdapter`](adapters/media/resolvers/UploadedMediaResolverAdapter.ts:37)
Resolver output must be *path-only*:
- For any `MediaReference`, `resolve()` returns `/media/...` or `null`.
- No `baseUrl` parameter is needed for DTOs.
Rationale: once URLs are path-only, the Website can proxy them and Next `Image` becomes deterministic.
### 3.4 Proper storage abstraction (core port) + adapter implementation
This is required to align with Clean Architecture rules in [`DATA_FLOW.md`](docs/architecture/DATA_FLOW.md:1) and avoid runtime stubs.
#### 3.4.1 Core (ports + use-cases)
We already have a core port [`MediaStoragePort`](apps/api/src/domain/media/MediaProviders.ts:9) used by the media use-cases (upload/delete). The plan is to make it real and remove mock usage in runtime.
Target responsibilities:
- Core Application port (interface): `MediaStoragePort`
- `uploadMedia(file, metadata) -> { success, url?, filename?, storageKey?, contentType? }`
- `deleteMedia(storageKey) -> void`
- (optional but recommended) `getReadStream(storageKey) -> stream` or `getBytes(storageKey) -> Buffer`
- Core Domain entity (or value object): `Media` should reference a storage identifier (e.g. `storageKey`) and `contentType`.
- The domain does not store absolute URLs.
- The resolver + controller decide how a `storageKey` becomes `/media/uploaded/{id}`.
#### 3.4.2 Adapters (file storage)
Add a concrete adapter: `FileSystemMediaStorageAdapter` under `adapters/`.
Implementation rules:
- Store files under a single base directory (configured via env):
- `GRIDPILOT_MEDIA_STORAGE_DIR=/data/media` (container path)
- Use deterministic, collision-resistant keys:
- `uploaded/{mediaId}/{originalFilename}` or `uploaded/{mediaId}` (single-file per mediaId)
- Enforce content-type allowlist for images (at minimum `image/png`, `image/jpeg`, `image/svg+xml`).
- Never return public absolute URLs from the adapter. Return `storageKey` only.
Docker alignment:
- Add a named volume mounted into `api` container for persisted dev media.
#### 3.4.3 API serving route for uploaded media
The API endpoint [`GET /media/uploaded/:mediaId`](apps/api/src/domain/media/MediaController.ts:169) is currently a stub.
Target:
- Look up `Media` by `mediaId` in `IMediaRepository`.
- Read bytes/stream from `MediaStoragePort` using `storageKey`.
- Set headers:
- `Content-Type: <stored contentType>`
- `Cache-Control: public, max-age=31536000, immutable` (if content-addressed) OR `max-age=3600` (if mutable)
- Return 404 if missing.
This makes “uploaded” a first-class, debuggable path in the same `/media/...` scheme.
## 4) End-to-end trace (pseudocode)
This is the required mental model for debugging.
### 4.1 Seed → DB
```text
teamId = seedId(team-1)
team.logoRef = MediaReference.generated(team, teamId)
persist team.logoRef as JSON
```
### 4.2 API Use Case → Presenter → DTO
```text
usecase GetAllTeamsUseCase
loads Team entities
returns { teams: [{ id, name, logoRef, logoUrl: null, ... }] }
presenter AllTeamsPresenter
for each team:
ref = MediaReference.fromJSON(team.logoRef)
dto.logoUrl = MediaResolver.resolve(ref)
=> /media/teams/{teamId}/logo
response JSON contains logoUrl string or null
```
### 4.3 Website → React component → img src
```text
LandingService.getHomeDiscovery
calls GET {apiBaseUrl}/teams/all
creates TeamCardViewModel with dto.logoUrl
TeamCard
Image src = team.logoUrl
(src is relative /media/...)
```
### 4.4 Browser fetch → Website rewrite → API bytes
```text
browser GET http://localhost:3000/media/teams/{id}/logo
Next rewrite proxies to http://api:3000/media/teams/{id}/logo
API returns image/svg+xml bytes
browser renders
```
## 5) Debuggability improvements (must-have)
### 5.1 Add a debug resolve endpoint in API
Add `GET /media/debug/resolve` in [`MediaController`](apps/api/src/domain/media/MediaController.ts:25).
Input options:
- Query param `ref` as base64url JSON of `MediaReferenceProps`.
- Or explicit query params: `type`, `variant`, `avatarVariant`, `generationRequestId`, `mediaId`.
Output JSON:
- `ref`: the parsed ref (as JSON)
- `refHash`: same as [`MediaReference.hash()`](core/domain/media/MediaReference.ts:271)
- `resolvedPath`: `/media/...` or null
- `resolver`: which branch handled it (default or generated or uploaded or none)
- `notes`: validation warnings (e.g. generationRequestId format)
This endpoint exists to debug resolvers without hitting entity APIs.
### 5.2 Structured logs
Add structured logs on each media request:
- In [`MediaController.getTeamLogo()`](apps/api/src/domain/media/MediaController.ts:72) and similar endpoints:
- log: route, entityId, cache-control chosen
- log: svg length, deterministic seed used
- In resolver:
- log: `refHash`, resolved path, branch
### 5.3 Curl recipes (copy/paste)
Teams API returning logoUrl:
```bash
curl -sS http://localhost:3001/teams/all | jq '.teams[0] | {id, name, logoUrl}'
```
Team logo bytes:
```bash
TEAM_ID=$(curl -sS http://localhost:3001/teams/all | jq -r '.teams[0].id')
curl -i http://localhost:3001/media/teams/$TEAM_ID/logo | sed -n '1,20p'
```
Expected:
- `HTTP/1.1 200 OK`
- `content-type: image/svg+xml`
Website proxy path (after rewrite is added):
```bash
curl -i http://localhost:3000/media/teams/$TEAM_ID/logo | sed -n '1,20p'
```
## 6) Concrete fixes (file-by-file)
### 6.1 Remove misleading runtime stubs
1) Stop using [`InMemoryMediaResolverAdapter`](adapters/media/MediaResolverInMemoryAdapter.ts:60) in API runtime providers.
- Replace in [`TeamProviders`](apps/api/src/domain/team/TeamProviders.ts:28) (and similar providers in drivers/leagues if present) with the real [`MediaResolverAdapter`](adapters/media/MediaResolverAdapter.ts:53).
2) Ensure any “in-memory” resolver remains test-only:
- Keep it referenced only in unit tests, not in app modules/providers.
### 6.2 Make resolver output path-only
Update [`MediaResolverAdapter.resolve()`](adapters/media/MediaResolverAdapter.ts:81) and sub-resolvers to return `/media/...` paths:
- [`DefaultMediaResolverAdapter.resolve()`](adapters/media/resolvers/DefaultMediaResolverAdapter.ts:44): `/media/default/...`
- [`GeneratedMediaResolverAdapter.resolve()`](adapters/media/resolvers/GeneratedMediaResolverAdapter.ts:45):
- team → `/media/teams/{id}/logo`
- league → `/media/leagues/{id}/logo`
- driver → `/media/avatar/{id}`
- [`UploadedMediaResolverAdapter.resolve()`](adapters/media/resolvers/UploadedMediaResolverAdapter.ts:47): `/media/uploaded/{mediaId}`
Remove all “baseUrl” joining logic from resolvers.
### 6.3 Website must stop inventing wrong media URLs
1) Replace or delete [`getMediaUrl()`](apps/website/lib/utilities/media.ts:11).
- Either remove it entirely, or redefine it to output canonical `/media/...` paths.
2) Update all call sites found via:
- [`TeamLadderRow`](apps/website/components/teams/TeamLadderRow.tsx:18)
- [`LeagueHeader`](apps/website/components/leagues/LeagueHeader.tsx:1)
- [`FriendPill`](apps/website/components/social/FriendPill.tsx:1)
- [`apps/website/app/teams/[id]/page.tsx`](apps/website/app/teams/[id]/page.tsx:195)
- [`apps/website/app/profile/page.tsx`](apps/website/app/profile/page.tsx:409)
to use either:
- DTO-provided URLs, or
- a single canonical builder aligned with API routes.
### 6.4 Add Website rewrite for `/media/*`
Extend [`next.config.mjs rewrites()`](apps/website/next.config.mjs:47) to also proxy `/media/:path*` to `http://api:3000/media/:path*` in dev.
This yields same-origin image URLs for the browser:
- `src=/media/...` always.
### 6.5 Tests
1) Unit tests for resolver mapping:
- Add tests around [`GeneratedMediaResolverAdapter.resolve()`](adapters/media/resolvers/GeneratedMediaResolverAdapter.ts:45) to ensure `team-<id>``/media/teams/<id>/logo`.
2) API presenter contract test:
- Verify `logoUrl` is `null` or starts with `/media/` in [`AllTeamsPresenter`](apps/api/src/domain/team/presenters/AllTeamsPresenter.ts:8).
3) E2E Playwright image smoke:
- Add a test that loads the landing page, finds at least one team logo `<img>`, and asserts the image request returns 200.
- Use existing Playwright config files like [`playwright.website.config.ts`](playwright.website.config.ts:1).
4) Media upload + serve integration test:
- Upload an image via `POST /media/upload`.
- Verify response includes a `mediaId` and DTO uses `/media/uploaded/{mediaId}` (path-only rule).
- Fetch `/media/uploaded/{mediaId}` and assert status 200 + correct `Content-Type`.
## 7) Mermaid flow (new architecture)
```mermaid
flowchart TD
A[Bootstrap seed sets MediaReference] --> B[DB stores logoRef JSON]
B --> C[API use case returns logoRef]
C --> D[Presenter resolves ref to media path]
D --> E[DTO logoUrl is slash media path]
E --> F[Website renders Image src slash media path]
F --> G[Next rewrite proxies to API media route]
G --> H[MediaController returns SVG or PNG bytes]
```
## 8) TDD execution order (implementation guidance)
1) Add unit tests for canonical resolver mapping (generated/system-default/uploaded).
2) Change resolver implementations to return path-only and make tests pass.
3) Update API providers to use real resolver everywhere (remove runtime usage of in-memory resolver).
4) Add `/media/:path*` rewrite in Website.
5) Replace Website `getMediaUrl` and all call sites.
6) Add API debug endpoint and structured logs.
7) Replace mock `MediaStoragePort` with real filesystem adapter, wire env + volume.
8) Implement uploaded media serving endpoint (remove stub), add integration test.
9) Add Playwright test verifying image loads.
## 9) Acceptance criteria
1) `GET http://localhost:3001/teams/all` returns `logoUrl` values that are either `null` or begin with `/media/`.
2) `GET http://localhost:3000/media/teams/{id}/logo` returns 200 with `image/svg+xml`.
3) No Next `Image` remote-host/SVG errors in dev for logos.
4) Playwright test passes: at least one image request returns 200 on a real page.
5) Upload flow works end-to-end:
- `POST /media/upload` stores a file via filesystem adapter.
- `GET /media/uploaded/{mediaId}` returns the stored bytes with correct headers.