|
|
@@ -1,675 +1,162 @@
|
|
|
-# Compliance Review -- MovieDice
|
|
|
+# Compliance Review — MovieDice (UI + Filter Batch)
|
|
|
|
|
|
-Reviewed: 2026-04-05 | Reviewer: Claude (automated)
|
|
|
-Scope: Pre-implementation architecture review of PROJECT_SCOPE.md | Standard: General (TMDB ToS, GDPR, WCAG 2.1 AA, PWA, Docker)
|
|
|
-
|
|
|
-**Note:** This is a pre-implementation review. No source code exists yet. All findings are based on the architectural design and project scope document. Line references point to PROJECT_SCOPE.md.
|
|
|
+Reviewed 2026-05-21. Scope: uncommitted batch on master (cert filter, layout/footer refactor, carousel button conversion, settings refresh, privacy/legal footer).
|
|
|
|
|
|
---
|
|
|
|
|
|
## CRITICAL
|
|
|
|
|
|
-### 1. TMDB API Key Exposed to Client-Side Code
|
|
|
-
|
|
|
-`PROJECT_SCOPE.md:259` -- The tech stack specifies TMDB API integration with client-side search (debounced queries from the browser). The scope does not specify server-side proxying of TMDB API calls. If the TMDB API key is used directly in client-side fetch calls, it will be exposed in browser network requests and potentially in bundled JavaScript. TMDB's terms require that API keys not be publicly accessible. This also enables abuse by third parties who extract the key.
|
|
|
-
|
|
|
-**Fix:** All TMDB API calls must be routed through Next.js API routes (or Server Actions). The `TMDB_API_KEY` environment variable must only be accessible server-side. Create a `/api/tmdb/search`, `/api/tmdb/popular`, and `/api/tmdb/trailer` proxy layer. Mark the env var in `next.config.js` without the `NEXT_PUBLIC_` prefix to ensure it is never bundled into client code.
|
|
|
-
|
|
|
-**Implementation Risk:** Adds a network hop (client -> Next.js server -> TMDB), which slightly increases latency for search queries. Mitigate with TanStack Query caching and appropriate `Cache-Control` headers on the proxy responses.
|
|
|
-
|
|
|
----
|
|
|
-
|
|
|
-### 2. TMDB Attribution Requirement Not Addressed
|
|
|
-
|
|
|
-`PROJECT_SCOPE.md` (entire document) -- TMDB's Terms of Use require visible attribution on any application using their API. The scope document makes no mention of displaying the TMDB logo, a link to themoviedb.org, or the required disclaimer text ("This product uses the TMDB API but is not endorsed or certified by TMDB"). Omitting attribution violates the API terms and risks having the API key revoked.
|
|
|
-
|
|
|
-**Fix:** Add a mandatory implementation item: display the TMDB attribution logo and text in the app footer on every page. The logo must link to https://www.themoviedb.org/. Include this in Phase 1 (Foundation) as a hard requirement, not a polish item.
|
|
|
-
|
|
|
-**Implementation Risk:** Minimal. This is a UI addition only. Ensure the logo meets TMDB's brand guidelines for size and placement.
|
|
|
-
|
|
|
----
|
|
|
-
|
|
|
-### 3. No Privacy Policy or Terms of Service Specified
|
|
|
-
|
|
|
-`PROJECT_SCOPE.md:313-319` -- The Privacy section notes "no personal data beyond display name is stored," but this is incorrect from a legal standpoint. The app collects and stores: UUIDs (personal data under GDPR), display names, avatar color preferences, recovery codes (hashed), group membership data, movie preferences (which movies a user added), and IP addresses in server logs. Even with anonymous auth, GDPR and similar privacy laws (CCPA, LGPD) require a privacy policy explaining what data is collected, how it is used, how long it is retained, and how users can request deletion.
|
|
|
-
|
|
|
-**Fix:** Create a privacy policy page accessible from the landing page footer. Document: (a) what data is collected (UUID, display name, avatar color, group membership, movie additions, IP addresses in logs), (b) purpose of each data point, (c) retention period, (d) how to request account deletion, (e) cookie/local storage usage, (f) third-party data sharing (Supabase as processor, TMDB API calls). Add this as a Phase 1 implementation item.
|
|
|
-
|
|
|
-**Implementation Risk:** Requires legal review for completeness. A template-based approach is acceptable for MVP but should be reviewed by a legal professional before public launch.
|
|
|
-
|
|
|
----
|
|
|
-
|
|
|
-### 4. Recovery Code Security -- Insufficient Specification
|
|
|
-
|
|
|
-`PROJECT_SCOPE.md:34,269` -- Recovery codes are described as "a longer alphanumeric code shown once after account creation" with recovery codes "hashed before storage." However, the scope does not specify: (a) the entropy/length of recovery codes, (b) the hashing algorithm (must be bcrypt/argon2, not SHA-256), (c) rate limiting on recovery code attempts, (d) brute-force protection. A weak recovery code or fast hash function could allow an attacker to take over accounts by guessing recovery codes.
|
|
|
-
|
|
|
-**Fix:** Mandate the following in the implementation: (a) Recovery codes must be at least 128 bits of entropy (e.g., 24 alphanumeric characters or a mnemonic phrase). (b) Hash with bcrypt (cost factor 12+) or argon2id. (c) Rate limit recovery code attempts to 5 per minute per IP. (d) Lock out after 10 failed attempts with a cooldown period. (e) Log all recovery code attempts for security monitoring.
|
|
|
-
|
|
|
-**Implementation Risk:** Rate limiting requires server-side state tracking (Redis, Supabase table, or in-memory with caveats). Bcrypt hashing adds ~250ms per attempt, which is acceptable for a security-critical operation.
|
|
|
-
|
|
|
----
|
|
|
-
|
|
|
-### 5. Invite Code as Sole Access Control -- Brute Force Risk
|
|
|
-
|
|
|
-`PROJECT_SCOPE.md:35-36,315` -- Invite codes are described as "short human-readable" (e.g., WOLF-42) and serve as the sole access control for group membership. A code like WOLF-42 has extremely low entropy (a word from a list + a 2-digit number). An attacker could enumerate valid invite codes and join arbitrary groups, accessing their movie lists. There is no mention of rate limiting on join attempts.
|
|
|
-
|
|
|
-**Fix:** (a) Increase invite code entropy: use at least 3 words + 3 digits (e.g., WOLF-RAIN-42X) or a longer random string. (b) Rate limit join attempts to 5 per minute per IP/user. (c) Lock out after 15 failed attempts. (d) Consider adding optional group passwords for sensitive groups. (e) The invite code regeneration feature (already in scope) is good -- ensure it is prominently surfaced to admins.
|
|
|
-
|
|
|
-**Implementation Risk:** Longer codes reduce shareability (harder to type/remember). Balance security with UX by testing code formats with users. Rate limiting adds server-side complexity.
|
|
|
-
|
|
|
----
|
|
|
-
|
|
|
-### 6. Master Admin TOTP Secret in Environment Variable -- Rotation and Backup Gaps
|
|
|
-
|
|
|
-`PROJECT_SCOPE.md:329-330` -- The TOTP secret is stored as an environment variable. This means: (a) rotating the TOTP secret requires redeployment, (b) there is no backup/recovery mechanism if the authenticator app is lost, (c) the TOTP secret is visible to anyone with access to the deployment environment, (d) there is no session expiry specified for admin sessions.
|
|
|
-
|
|
|
-**Fix:** (a) Document the TOTP secret rotation procedure (generate new secret, update env var, redeploy, re-enroll authenticator). (b) Generate and store backup codes for the master admin (encrypted, stored separately from the TOTP secret). (c) Specify admin session expiry (recommend 1 hour max, 15 minutes idle timeout). (d) Implement CSRF protection on admin actions. (e) Log all admin actions with timestamps for audit trail.
|
|
|
-
|
|
|
-**Implementation Risk:** Adding backup codes increases the attack surface slightly but prevents lockout. Session expiry may be annoying for extended admin sessions but is necessary for security.
|
|
|
-
|
|
|
----
|
|
|
-
|
|
|
-## CODE QUALITY
|
|
|
-
|
|
|
-### 7. No Linting, Formatting, or Type Checking Standards Specified
|
|
|
-
|
|
|
-`PROJECT_SCOPE.md` (entire document) -- The scope specifies no code quality tooling. For a Next.js/TypeScript project, the following should be established before any code is written: ESLint configuration, Prettier configuration, TypeScript strict mode, import ordering rules, and a pre-commit hook to enforce them. Without these, code quality will drift from the first commit, especially if multiple developers contribute.
|
|
|
-
|
|
|
-**Fix:** Add a Phase 0 or Phase 1.0 task: (a) Initialize with TypeScript strict mode (`"strict": true` in tsconfig.json). (b) Configure ESLint with `next/core-web-vitals` and `next/typescript` presets. (c) Configure Prettier with consistent rules (single quotes, trailing commas, 100-char line width). (d) Add `lint-staged` + `husky` for pre-commit enforcement. (e) Add `.editorconfig` for cross-editor consistency.
|
|
|
-
|
|
|
-**Implementation Risk:** None. This is a one-time setup cost that saves significant time later.
|
|
|
-
|
|
|
----
|
|
|
-
|
|
|
-### 8. No Testing Strategy Defined
|
|
|
-
|
|
|
-`PROJECT_SCOPE.md:406-421` -- Phase 9 describes manual QA and cross-device testing, but there is no mention of automated testing at any level: no unit tests, no integration tests, no end-to-end tests, no API tests. For a real-time collaborative app with complex state management (group membership, watched state, cross-list rolls), manual testing alone is insufficient and will lead to regressions.
|
|
|
-
|
|
|
-**Fix:** Add a testing mandate to Phase 1: (a) Set up Vitest for unit tests (utility functions, emotion-to-genre mapping, invite code generation). (b) Set up React Testing Library for component tests. (c) Set up Playwright for E2E tests (at minimum: onboarding flow, add movie, roll dice, real-time sync). (d) Require tests for all business logic before merge. (e) Add CI pipeline (GitHub Actions) that runs tests on every PR.
|
|
|
-
|
|
|
-**Implementation Risk:** Adds development time but significantly reduces bug density. Start with critical path E2E tests and expand coverage over time.
|
|
|
-
|
|
|
----
|
|
|
-
|
|
|
-### 9. No CI/CD Pipeline Specified
|
|
|
-
|
|
|
-`PROJECT_SCOPE.md:322-330` -- Deployment is described as "Vercel auto-deploys from main branch" (and Docker for the user's primary case), but there is no mention of: CI checks before deployment, automated testing gates, build verification, security scanning, or environment promotion (staging -> production).
|
|
|
-
|
|
|
-**Fix:** Establish a CI pipeline before development begins: (a) GitHub Actions workflow running on every PR: lint, type-check, test, build. (b) Block merges to main if any step fails. (c) For Docker: add a workflow that builds the Docker image and runs smoke tests against it. (d) Add dependency vulnerability scanning (npm audit, Dependabot/Renovate). (e) Consider a staging environment for pre-production validation.
|
|
|
-
|
|
|
-**Implementation Risk:** Minimal. GitHub Actions free tier is sufficient for this project size. The main risk is CI flakiness from E2E tests, which can be mitigated with retry logic.
|
|
|
-
|
|
|
----
|
|
|
-
|
|
|
-### 10. Emotion-to-Genre Mapping Lacks Extensibility Pattern
|
|
|
-
|
|
|
-`PROJECT_SCOPE.md:452-469` -- The emotion-to-genre mapping is defined as a static table in the scope document. The implementation should use a configuration-driven approach (JSON file or database table) rather than hardcoded if-else/switch statements, to allow easy updates without code changes.
|
|
|
-
|
|
|
-**Fix:** Implement the mapping as a typed constant object (e.g., `EMOTION_GENRE_MAP`) in a dedicated configuration file. Include the mapping table in the scope as a data contract. Consider moving this to the database post-MVP to allow runtime updates.
|
|
|
-
|
|
|
-**Implementation Risk:** Minimal. A configuration file is slightly more complex than inline constants but much more maintainable.
|
|
|
-
|
|
|
----
|
|
|
-
|
|
|
-## DOCUMENTATION GAPS
|
|
|
-
|
|
|
-### 11. No API Route Documentation Standard
|
|
|
-
|
|
|
-`PROJECT_SCOPE.md` (entire document) -- The scope defines several API-like interactions (TMDB search, group join, movie add/remove, admin actions) but does not specify an API documentation standard. For a project with both client-side and server-side routes, undocumented APIs lead to integration errors and make onboarding new developers difficult.
|
|
|
-
|
|
|
-**Fix:** Establish a documentation requirement: every Next.js API route and Server Action must include a JSDoc header documenting: HTTP method, path, request body/query parameters with types, response shape with status codes, authentication requirements, and error responses. Consider generating an OpenAPI spec from these annotations.
|
|
|
+### 1. STRICT cert walker silently hides legitimate titles in search
|
|
|
|
|
|
----
|
|
|
-
|
|
|
-### 12. No Error Code or Error Response Standard
|
|
|
+`src/lib/tmdb/certification.ts:51-77`, `src/app/api/tmdb/search/route.ts:32-39` — The walker requires `sawRecognizedCert && sawPositiveMatch`. A title is rejected if **no recognized country reports any certification at all** — extremely common for older, foreign-language, and indie titles (TMDB lets contributors leave `certification` empty). `/api/tmdb/search` post-filters with the same walker, so users searching legitimate PG/PG-13-equivalent movies see them silently vanish with no UX signal — invisible failure mode is worse than a permissive filter.
|
|
|
|
|
|
-`PROJECT_SCOPE.md:383` -- Phase 5.6 mentions "Error handling: invalid invite code, TMDB API failure, network errors" but does not define an error response format, error codes, or error categorization. Without a standard, each error will be handled ad-hoc with inconsistent user-facing messages.
|
|
|
+**Fix:** Define an explicit policy for "no cert data" titles. Options: (a) allow US-released-but-empty-cert when paired with a non-recent release date; (b) treat known no-rating tokens (`NR`, `""`, `Not Rated`, `Unrated`) as explicitly allowed; (c) fall back to TMDB `adult` flag + popularity threshold when cert data is absent. Document the choice in the file header.
|
|
|
|
|
|
-**Fix:** Define a standard error response shape before development: `{ error: { code: string, message: string, details?: object } }`. Create an error code enum (e.g., `INVALID_INVITE_CODE`, `TMDB_UNAVAILABLE`, `RATE_LIMITED`, `UNAUTHORIZED`). Map each code to a user-friendly message. Centralize error handling in a shared utility.
|
|
|
+**Implementation Risk:** Loosening could let through R-rated indies with missing US cert. Recommend a one-week log of "rejected for no-cert" counts before deciding the policy.
|
|
|
|
|
|
---
|
|
|
|
|
|
-### 13. Database Migration Strategy Not Specified
|
|
|
-
|
|
|
-`PROJECT_SCOPE.md:265-308` -- The data model is defined but there is no mention of how database schema changes will be managed. Without a migration strategy, schema changes during development will be ad-hoc and difficult to reproduce across environments.
|
|
|
+### 2. Pre-existing `movies` rows now break `<ListMoreInfoModal>`
|
|
|
|
|
|
-**Fix:** Use Supabase's migration system or a tool like `supabase db diff` / `supabase migration new`. Store all migrations in version control. Never modify the database schema directly in production -- always through versioned migrations. Document this in the project README.
|
|
|
-
|
|
|
----
|
|
|
+`src/app/api/tmdb/movie/[id]/route.ts:25-28`, `src/components/dice/list-more-info-modal.tsx` — Any movie added to `public.movies` before cert filtering shipped (or one that flips after a future allowlist tightening) will now fail `isMovieAllowedByCert` when the modal opens. The route correctly 404s to avoid existence leakage, but the user just sees the modal fetch fail on a movie that's already on their list.
|
|
|
|
|
|
-### 14. Supabase Row-Level Security (RLS) Not Mentioned
|
|
|
+**Fix:** Add a `?context=in-list` (or similar) opt-in to `/api/tmdb/movie/[id]` that skips the cert gate when the caller is fetching details for a movie already in the user's group (server can verify via session + `movies` lookup before bypassing). Alternative: backfill — sweep `movies`, soft-flag any that no longer pass, surface UI message instead of silent 404.
|
|
|
|
|
|
-`PROJECT_SCOPE.md:265-308` -- The data model defines tables and relationships but makes no mention of Row-Level Security policies. Supabase strongly recommends RLS for all tables. Without RLS, any authenticated user (or anyone with the anon key) could read/write any row in any table via the Supabase client, completely bypassing group membership checks.
|
|
|
-
|
|
|
-**Fix:** This is a borderline-critical security issue. Define RLS policies for every table: (a) `users`: users can only read/update their own row. (b) `groups`: only members can read; only admin can update/delete. (c) `group_members`: only members of the group can read; only admin can delete others. (d) `movies`: only group members can CRUD. (e) `landing_reel_posters`: public read, no public write. (f) `admin_sessions`: no public access. Implement these in Phase 1 alongside schema creation.
|
|
|
-
|
|
|
-**Implementation Risk:** RLS policies that are too restrictive will break functionality. Test each policy thoroughly. Supabase's service role key (used server-side) bypasses RLS, which is correct for admin operations.
|
|
|
+**Implementation Risk:** Context-bypass leaks existence to authenticated members of the owning list only — acceptable. Backfill is destructive and needs admin sign-off.
|
|
|
|
|
|
---
|
|
|
|
|
|
-## PERFORMANCE
|
|
|
+## HIGH
|
|
|
|
|
|
-### 15. Landing Page Slot-Machine Animation -- Heavy Asset Loading
|
|
|
+### 3. Admin layout double `min-h-screen` breaks login/dashboard chrome
|
|
|
|
|
|
-`PROJECT_SCOPE.md:74-79` -- The landing page slot-machine animation spins through ~20 movie poster images across 3 reels. This means loading 20 poster images before the animation can play smoothly. On mobile connections, this could be 2-4 MB of images that must load before the primary CTA is interactive, causing slow page loads -- the user's primary concern.
|
|
|
+`src/app/admin/layout.tsx:5-9`, `src/app/admin/login/page.tsx:40`, `src/app/admin/page.tsx:13` — The layout now wraps children in `flex min-h-screen flex-col bg-neutral-950` + `<TMDBFooter />` below a `flex-1` slot. Both child pages still self-render their own `min-h-screen` container. Net effect: the child fills 100vh, the footer renders below it, the page now requires scroll by default, and the centered login UX is offset. `bg-neutral-950` is duplicated (harmless).
|
|
|
|
|
|
-**Fix:** (a) Use TMDB's smallest poster size (`w92` or `w154`) for the reel animation -- full-size posters are unnecessary at reel-spin speed. (b) Preload the reel poster set using `<link rel="preload">` or a service worker cache. (c) Store the poster URLs in the `landing_reel_posters` table (already planned) and serve them from a Next.js API route with aggressive `Cache-Control` headers (e.g., `max-age=86400`). (d) Use CSS `will-change: transform` on reel elements for GPU-accelerated animation. (e) Consider using a single sprite sheet or CSS-based animation with `background-position` for the reel spin to avoid per-image loading. (f) Show a skeleton/placeholder while posters load, and allow the Roll button to be interactive even before all images are loaded.
|
|
|
+**Fix:** Remove `min-h-screen` from both `admin/login/page.tsx` and `admin/page.tsx`. Drop the now-redundant inner `bg-neutral-950`. Replace login centering with `flex-1 flex items-center justify-center` on an inner container so it still centers within the layout's flex-1 main area.
|
|
|
|
|
|
-**Implementation Risk:** Smaller poster sizes may look blurry on high-DPI screens. Use `w185` as a compromise. Sprite sheets add build complexity but dramatically improve animation performance.
|
|
|
+**Implementation Risk:** None if done correctly — Playwright/manual check the centered-login layout after.
|
|
|
|
|
|
---
|
|
|
|
|
|
-### 16. No Image Optimization Strategy
|
|
|
+### 4. List-page back/settings links use raw `<a href>` and drop client state
|
|
|
|
|
|
-`PROJECT_SCOPE.md:293` -- The scope stores `poster_path` (TMDB relative path) and constructs full URLs at render time. However, there is no mention of: (a) using Next.js `<Image>` component for automatic optimization, (b) responsive image sizing (different sizes for grid view vs. expanded panel vs. reel animation), (c) lazy loading for below-the-fold images, (d) blur placeholder generation, (e) WebP/AVIF format negotiation.
|
|
|
+`src/app/(app)/list/[id]/page.tsx:46-67, 70-91` — Both navigation chrome elements use `<a href="/home">` / `<a href="/list/{id}/settings">` instead of `<Link>` from `next/link`. Each click triggers a full document load: TanStack Query cache evicts, IndexedDB-persisted offline data must rehydrate, `AuthBootstrap` re-runs, optimistic mutation state is lost. The rest of the app uses `<Link>`.
|
|
|
|
|
|
-**Fix:** Mandate the use of Next.js `<Image>` component (or `next/image` with a custom TMDB loader) for all poster images. Configure TMDB image sizes per context: `w185` for grid thumbnails, `w342` for expanded panel, `w92`/`w154` for reel animation. Enable lazy loading for all images below the fold. Add TMDB's image domain to `next.config.js` `images.remotePatterns`. Use `placeholder="blur"` with a low-resolution blur data URL generated at add-time or cached.
|
|
|
+**Fix:** Convert both anchors to `<Link href={...}>`. Preserve className, aria-label, svg child.
|
|
|
|
|
|
-**Implementation Risk:** Next.js `<Image>` with external domains requires proper configuration. TMDB images are already served from a CDN, so double-optimization is unnecessary -- focus on correct sizing and lazy loading.
|
|
|
+**Implementation Risk:** None; `<Link>` accepts arbitrary children including svg.
|
|
|
|
|
|
---
|
|
|
|
|
|
-### 17. Unbounded Real-Time Subscriptions
|
|
|
+## MEDIUM
|
|
|
|
|
|
-`PROJECT_SCOPE.md:48,365` -- Supabase real-time subscriptions are specified for live add/remove/watched-status updates. If a user belongs to multiple groups, the app would need to maintain one subscription per group (or a single subscription filtered to all their groups). The scope does not address: (a) subscription lifecycle management (subscribe on mount, unsubscribe on unmount), (b) reconnection logic on network interruption, (c) subscription count limits (Supabase free tier has limits on concurrent connections).
|
|
|
+### 5. `aria-live="polite"` on the carousel teaser `<button>`
|
|
|
|
|
|
-**Fix:** (a) Only subscribe to the currently-viewed list, not all lists simultaneously. Unsubscribe when navigating away. (b) On the home page, use polling or a single subscription for movie counts rather than full list subscriptions. (c) Implement exponential backoff reconnection. (d) Set a maximum subscription count and degrade gracefully if exceeded. (e) Use Supabase's channel-based subscriptions with proper cleanup in React `useEffect` return functions.
|
|
|
+`src/components/dice/list-roll-carousel.tsx:234` — The teaser button has `aria-live="polite"`, but its accessible name (`More info about {title}`) is static once the winner is set. Worse, the button sits **inside** another `aria-live="polite"` region at line 204 — overlapping live regions can cause double-announce or AT confusion. `RollAnnouncer` is the canonical announcement source elsewhere.
|
|
|
|
|
|
-**Implementation Risk:** Limiting subscriptions to the active view means the home page movie counts may be slightly stale. This is an acceptable trade-off for resource efficiency. Document the staleness window (e.g., "counts refresh every 30 seconds on the home page").
|
|
|
+**Fix:** Remove `aria-live="polite"` from the `<button>` on line 234. Also drop the wrapping `aria-live` on line 204 — rely on `RollAnnouncer` for the announcement.
|
|
|
|
|
|
---
|
|
|
|
|
|
-### 18. Infinite Scroll Without Virtualization
|
|
|
+### 6. Carousel "i" glyph is decorative noise post-conversion
|
|
|
|
|
|
-`PROJECT_SCOPE.md:41,145-147` -- The grid loads 12 movies initially and appends more on scroll. For groups with large watchlists (50-200+ movies), all previously loaded movie cards remain in the DOM. This causes memory bloat and rendering slowdowns, especially on low-end mobile devices with the full-bleed poster images.
|
|
|
+`src/components/dice/list-roll-carousel.tsx:255-260` — The whole card is now the click target. The italic-serif "i" badge is `aria-hidden` (correct) but visually implies a separate hit target. Users may hesitate ("do I click the i?").
|
|
|
|
|
|
-**Fix:** Implement windowed/virtualized scrolling using a library like `@tanstack/react-virtual` or `react-window`. Only render DOM nodes for movies currently in or near the viewport. This keeps memory and DOM node count constant regardless of list size.
|
|
|
-
|
|
|
-**Implementation Risk:** Virtualization adds complexity to the inline panel expansion (since the expanded panel occupies space between rows). The virtualization library must account for variable row heights. Test thoroughly with the inline expansion UX.
|
|
|
+**Fix:** Remove the `<span aria-hidden>i</span>` or replace with a clearly decorative chevron / "tap for info" microcopy. Recompute spacing of the genre chip row below.
|
|
|
|
|
|
---
|
|
|
|
|
|
-### 19. Cross-List Roll May Require Expensive Query
|
|
|
+### 7. `/api/tmdb/search` post-filter fan-out is heavy
|
|
|
|
|
|
-`PROJECT_SCOPE.md:119,369` -- Rolling across all user lists combined requires fetching all unwatched movies from all groups the user belongs to. Without optimization, this is an N+1 query pattern (one query per group) or a complex join. For users in many groups with large lists, this could be slow.
|
|
|
+`src/app/api/tmdb/search/route.ts:32-39`, `src/lib/tmdb/client.ts:57-88` — Each search makes up to 20× `/movie/{id}?append_to_response=release_dates` calls (concurrency 6). A debounced user typing 8 chars produces 5-7 search requests → up to 140 detail calls cold-cache. `s-maxage=300` mitigates repeat queries but per-instance edges and cold deploys will push TMDB's 50 req/sec budget.
|
|
|
|
|
|
-**Fix:** Implement a single optimized SQL query: `SELECT m.* FROM movies m JOIN group_members gm ON m.group_id = gm.group_id WHERE gm.user_id = $1 AND m.watched = false`. Create a composite index on `movies(group_id, watched)` and `group_members(user_id)`. Consider caching the roll pool briefly (TanStack Query with a 30-second `staleTime`).
|
|
|
+**Fix:** Cache cert verdicts in Postgres keyed by `tmdb_id` with a 30-day TTL, lazily populated. Alternatively, only post-filter at add-time (search shows raw results, add endpoint rejects disallowed) — different UX trade-off.
|
|
|
|
|
|
-**Implementation Risk:** The query is straightforward but must be tested with realistic data volumes. Index creation is a one-time cost.
|
|
|
+**Implementation Risk:** Cached verdicts go stale if TMDB updates a rating — 30-day TTL is acceptable. Lazy at add-time means user can see ineligible titles in results.
|
|
|
|
|
|
---
|
|
|
|
|
|
-### 20. Trailer URL Refresh Job -- Potential TMDB Rate Limit Violation
|
|
|
+### 8. Privacy-page CCPA anchor offset hack is non-functional
|
|
|
|
|
|
-`PROJECT_SCOPE.md:52,388-392` -- The bi-weekly background job refreshes trailer URLs for movies where `trailer_url IS NULL`. If many movies have null trailers, this job could fire dozens or hundreds of TMDB API requests in rapid succession, exceeding the ~40 requests/10 seconds rate limit.
|
|
|
+`src/app/(public)/privacy/page.tsx:138-140` — `<span id="ccpa" className="block -translate-y-4" aria-hidden>` placed inside `<section id="gdpr">`. The transform doesn't actually create a scroll-offset (the anchor target still resolves to the original layout box in all major browsers), and both anchors land at the same line. `/privacy#ccpa` and `/privacy#gdpr` are visually identical, undermining the legal-footer claim of differentiated landings.
|
|
|
|
|
|
-**Fix:** (a) Implement request throttling in the refresh job: maximum 3 requests per second with exponential backoff on 429 responses. (b) Process movies in batches of 10 with a 5-second delay between batches. (c) Log the total count of movies needing refresh at job start. (d) Set a maximum of 100 movies processed per job run to prevent runaway execution. (e) Track and alert on persistent null trailers (some movies genuinely have no trailer).
|
|
|
+**Fix:** Split section 6 into two sub-sections — "Your Rights (CCPA / US California)" with `id="ccpa"` and "Your Rights (GDPR / EU/EEA)" with `id="gdpr"`, each enumerating regime-specific rights. Or place `id="ccpa"` and `id="gdpr"` on separate paragraphs within one section.
|
|
|
|
|
|
-**Implementation Risk:** Throttling means the job takes longer to complete. For large backlogs, the job may not process all null trailers in a single run, which is acceptable -- it will catch up over multiple runs.
|
|
|
+**Implementation Risk:** Wording care: CCPA-specific rights (sale/share opt-out, non-discrimination) and GDPR-specific (lodge a complaint, restriction-of-processing) shouldn't be conflated.
|
|
|
|
|
|
---
|
|
|
|
|
|
-### 21. No Code Splitting or Bundle Analysis Strategy
|
|
|
-
|
|
|
-`PROJECT_SCOPE.md` (entire document) -- The app has several distinct page types (landing, home, list view, admin panel) with distinct dependencies (e.g., the admin panel needs `otplib`, the landing page needs the reel animation, the list view needs real-time subscriptions). Without explicit code splitting, the entire app's JavaScript will be loaded on every page, leading to slow initial page loads -- the user's primary concern.
|
|
|
+### 9. Privacy policy CCPA coverage is incomplete
|
|
|
|
|
|
-**Fix:** (a) Next.js App Router provides route-based code splitting by default -- ensure each major view is a separate route segment. (b) Use `next/dynamic` with `{ ssr: false }` for heavy client-only components (reel animation, roll animation, inline panel). (c) Lazy-load `otplib` only on the admin route. (d) Add `@next/bundle-analyzer` to the project and review bundle sizes during Phase 5 polish. (e) Set a performance budget: initial JS bundle under 200KB gzipped, First Contentful Paint under 1.5s on 4G.
|
|
|
+`src/app/(public)/privacy/page.tsx:140-175` — Section 6 names CCPA but lists GDPR rights only. Missing CCPA-specific items: (a) categories of personal info collected (Identifiers — UUID; Internet/Network activity — server logs); (b) affirmative "We do not sell or share personal information for cross-context behavioral advertising"; (c) right to non-discrimination; (d) right to limit use of sensitive personal info; (e) right to correct; (f) verification method for rights requests (since there's no email collected, document the in-app account-deletion as the verified channel).
|
|
|
|
|
|
-**Implementation Risk:** Lazy loading animations may cause a flash of empty content. Use suspense boundaries with skeleton loaders to handle the loading state.
|
|
|
+**Fix:** Add a CCPA-specific block with the above. Worth a legal review if the deployment serves California residents at scale.
|
|
|
|
|
|
---
|
|
|
|
|
|
-## INFRASTRUCTURE
|
|
|
+### 10. Legal footer cookie notice understates browser storage in use
|
|
|
|
|
|
-### 22. Docker Deployment Not Fully Specified
|
|
|
+`src/components/shared/legal-footer.tsx:19-21` — Footer says "essential cookies for authentication." Privacy policy §8 also discloses localStorage usage by Supabase libs and admin iron-session cookies. "Essential cookies" is technically true (localStorage isn't a cookie) but most plain-English / ePrivacy interpretations treat any client-side identifier storage as "cookies or similar tracking technologies."
|
|
|
|
|
|
-`PROJECT_SCOPE.md:262,322` -- The scope mentions Vercel deployment but the user's primary deployment target is Docker. The scope does not include: Dockerfile creation, Docker Compose configuration, health checks, container resource limits, log aggregation, or reverse proxy setup.
|
|
|
-
|
|
|
-**Fix:** Add Docker infrastructure tasks to Phase 1: (a) Create a multi-stage Dockerfile using `node:20-alpine` base, `output: 'standalone'` in next.config.js, non-root user, and `tini` for PID 1 signal handling. (b) Create `docker-compose.yml` with the Next.js service, environment variable mapping, health check (`/api/health`), restart policy, and resource limits (`mem_limit: 512m`). (c) Create a `.dockerignore` excluding `node_modules`, `.git`, `.env`, `research/`. (d) Document the Docker deployment procedure in the README. (e) Add a `/api/health` endpoint that checks Supabase connectivity.
|
|
|
-
|
|
|
-**Implementation Risk:** Standalone Next.js output has different behavior from the standard build (e.g., static assets must be copied separately). Test the Docker build thoroughly against all routes.
|
|
|
+**Fix:** Reword to "This site uses essential cookies and local storage for authentication. See our Privacy Policy."
|
|
|
|
|
|
---
|
|
|
|
|
|
-### 23. No Logging or Monitoring Strategy
|
|
|
+## LOW
|
|
|
|
|
|
-`PROJECT_SCOPE.md:427` -- Phase 10.3 briefly mentions "basic error monitoring (Vercel logs + Sentry free tier)" but this is the final phase. For a Docker-deployed app, logging and monitoring must be planned from Phase 1: (a) structured logging format, (b) log levels, (c) log aggregation for containerized deployments, (d) error tracking, (e) uptime monitoring.
|
|
|
+### 11. Double `aria-live` regions around the roll result
|
|
|
|
|
|
-**Fix:** (a) Use a structured logger (e.g., `pino`) from Phase 1 -- output JSON logs so they can be parsed by any log aggregation tool. (b) Define log levels: ERROR for failures, WARN for degraded states (TMDB rate limited, Supabase reconnecting), INFO for significant events (user created, group created, admin action), DEBUG for development. (c) For Docker: output logs to stdout/stderr (Docker captures these by default). (d) Add Sentry error tracking in Phase 1, not Phase 10. (e) Create a `/api/health` endpoint for uptime monitoring. (f) Log all Master Admin actions for audit trail.
|
|
|
+`src/components/dice/list-roll-carousel.tsx:202-211`, `src/components/movies/movie-list-client.tsx:161` — `RollAnnouncer` is the canonical `aria-live` source. The carousel's outer container also wraps the teaser in `aria-live="polite"`. Two announcement surfaces for the same logical event can double-read.
|
|
|
|
|
|
-**Implementation Risk:** Structured logging adds a dependency but is essential for debugging production issues. `pino` is lightweight and performant.
|
|
|
+**Fix:** Drop `aria-live` from the carousel teaser container. Verify via NVDA/VoiceOver that `RollAnnouncer` fires at settle.
|
|
|
|
|
|
---
|
|
|
|
|
|
-### 24. Environment Variable Validation Not Specified
|
|
|
-
|
|
|
-`PROJECT_SCOPE.md:324-330` -- Five environment variables are listed as required, but there is no mention of validating them at startup. If any are missing or malformed, the app will fail at runtime with cryptic errors rather than at startup with a clear message.
|
|
|
-
|
|
|
-**Fix:** Use a validation library (e.g., `zod` or `t3-env`) to validate all environment variables at application startup. Fail fast with a descriptive error message if any required variable is missing or invalid. Include format validation (e.g., TMDB_API_KEY must be a 32-character hex string, SUPABASE_URL must be a valid URL, MASTER_ADMIN_TOTP_SECRET must be valid base32).
|
|
|
+### 12. Legal-footer hash links compound finding #8
|
|
|
|
|
|
-**Implementation Risk:** None. This is a reliability improvement with no downside.
|
|
|
+`src/components/shared/legal-footer.tsx:9-17` — `<Link href="/privacy#ccpa">` and `#gdpr` are honored by `<Link>`, but with finding #8 active, navigation appears to do nothing. Resolving #8 fixes this.
|
|
|
|
|
|
---
|
|
|
|
|
|
-### 25. No Database Backup Strategy
|
|
|
+### 13. `SearchBar` placeholder "ADD A MOVIE" with `text-center` is unconventional
|
|
|
|
|
|
-`PROJECT_SCOPE.md:323` -- Supabase free tier is mentioned with an "upgrade path available if scale demands," but there is no mention of database backups. Supabase free tier includes daily backups with 7-day retention, but (a) this is not documented in the scope, (b) for Docker/self-hosted Supabase scenarios, backups must be manually configured, (c) there is no documented restore procedure.
|
|
|
+`src/components/movies/search-bar.tsx:49-52` — Center-aligned uppercase placeholder + pulse-glow reads more like a button than a text input. Keyboard users are OK (aria-label "Search movies").
|
|
|
|
|
|
-**Fix:** (a) Document Supabase's backup capabilities and retention for the hosted scenario. (b) For Docker deployment, add a backup cron job (e.g., `pg_dump` daily to a mounted volume or object storage). (c) Document the restore procedure. (d) Test the restore procedure at least once before launch.
|
|
|
-
|
|
|
-**Implementation Risk:** Backup storage costs are minimal but non-zero. For self-hosted Supabase, backup automation requires additional Docker service configuration.
|
|
|
+**Fix:** Keep pulse-glow; revert placeholder to left-aligned + a leading "+" or magnifier glyph to disambiguate input-vs-button. (NOTE: user explicitly requested centered bold "ADD A MOVIE" placeholder — escalate before changing.)
|
|
|
|
|
|
---
|
|
|
|
|
|
-## COMPLIANCE
|
|
|
-
|
|
|
-### 26. TMDB Terms of Use -- Image Hosting Violation Risk
|
|
|
+### 14. No CPRA/PIPEDA/LGPD coverage
|
|
|
|
|
|
-`PROJECT_SCOPE.md:293,302-308` -- The data model correctly stores `poster_path` as a relative path and constructs the full URL at render time from TMDB's CDN. However, the `landing_reel_posters` table also stores `poster_path`. If the periodic refresh job or any caching layer downloads and re-serves these images from the app's own domain (e.g., via Next.js image optimization proxy), this may violate TMDB's requirement that images be served from their CDN.
|
|
|
+Privacy policy and footer mention only CCPA + GDPR. For a low-data anonymous-auth service the omission is defensible, but worth flagging:
|
|
|
|
|
|
-**Fix:** (a) Ensure all TMDB images are served directly from `image.tmdb.org` in production. (b) If using Next.js `<Image>` with a custom loader, configure it to rewrite URLs to TMDB's CDN rather than proxying through the Next.js server. (c) Alternatively, add `image.tmdb.org` to `images.remotePatterns` in `next.config.js` and let Next.js optimize on-the-fly (this is a gray area -- TMDB's ToS should be checked for whether CDN-proxied optimization is permitted). (d) Document the decision and rationale.
|
|
|
-
|
|
|
-**Implementation Risk:** Direct CDN serving means no control over image availability if TMDB's CDN has an outage. This is an acceptable risk given the ToS requirement.
|
|
|
+- **CPRA** — California's CCPA update; functionally covered by CCPA copy if you reword §6 to mention "CCPA as amended by CPRA"
|
|
|
+- **PIPEDA** (Canada), **LGPD** (Brazil) — broadly aligned with GDPR principles, no separate text required, but a single line ("Residents of other jurisdictions with comparable data-protection regimes — e.g., PIPEDA, LGPD — may have equivalent rights; contact the administrator.") would close the loop.
|
|
|
|
|
|
---
|
|
|
|
|
|
-### 27. GDPR -- Local Storage User ID Without Consent Mechanism
|
|
|
-
|
|
|
-`PROJECT_SCOPE.md:107-108` -- The app stores a user ID in local storage (or cookie) to identify returning users. Under the EU ePrivacy Directive (which complements GDPR), storing non-essential identifiers on a user's device requires informed consent. The scope does not include any consent mechanism (cookie banner, storage consent prompt).
|
|
|
+## POSITIVES
|
|
|
|
|
|
-**Fix:** (a) Determine if the stored user ID qualifies as "strictly necessary" for the service (it likely does, since the app cannot function without identifying the user). If so, consent is not required but a privacy policy must explain the storage. (b) If the app stores any analytics identifiers, tracking pixels, or third-party cookies (e.g., Sentry, Vercel Analytics), those require explicit consent. (c) Add a privacy policy link in the footer. (d) If deploying for EU users, implement a minimal consent banner for any non-essential storage.
|
|
|
-
|
|
|
-**Implementation Risk:** Consent banners add friction to the onboarding flow, which conflicts with the "low tolerance for signup friction" user trait. Minimize non-essential storage to avoid needing a banner.
|
|
|
-
|
|
|
----
|
|
|
-
|
|
|
-### 28. GDPR -- No Account Deletion Flow for Regular Users
|
|
|
-
|
|
|
-`PROJECT_SCOPE.md:53,399` -- The Master Admin can delete any user, but there is no self-service account deletion flow for regular users. GDPR Article 17 (Right to Erasure) requires that users can request deletion of their personal data. The scope only mentions "Leave this list" for regular users, which removes group membership but does not delete the user account or their data across other groups.
|
|
|
-
|
|
|
-**Fix:** Add a "Delete My Account" option accessible from user settings. This must: (a) Remove the user from all groups (triggering ownership transfer or list deletion as applicable). (b) Delete the user record from the `users` table. (c) Anonymize or delete the user's `added_by` attributions in the `movies` table (set to null or a "[deleted user]" sentinel). (d) Clear local storage. (e) Confirm deletion is complete. (f) This should cascade properly -- define the cascade behavior in the database schema.
|
|
|
-
|
|
|
-**Implementation Risk:** Cascade deletion is complex when a user is the admin of multiple groups. Each group must be handled independently (transfer or delete). Test edge cases: user is admin of 3 groups, member of 2 others, has added movies to all of them.
|
|
|
-
|
|
|
----
|
|
|
-
|
|
|
-### 29. GDPR -- Data Retention Period Not Defined
|
|
|
-
|
|
|
-`PROJECT_SCOPE.md` (entire document) -- No data retention period is specified for any data type. GDPR requires that personal data not be kept longer than necessary. Questions that need answers: How long is a user account retained if unused? How long are deleted lists' data retained? Are server logs with IP addresses rotated?
|
|
|
-
|
|
|
-**Fix:** Define retention policies: (a) Inactive user accounts: auto-delete after 12 months of no activity (or prompt re-confirmation). (b) Deleted lists: hard delete immediately (no soft delete unless needed for recovery). (c) Server logs: rotate every 30 days. (d) Admin action logs: retain for 90 days. (e) Document these in the privacy policy.
|
|
|
-
|
|
|
-**Implementation Risk:** Auto-deletion of inactive accounts could surprise returning users. Implement a warning mechanism (though without email, this is challenging with anonymous auth). Consider extending the retention period and documenting it clearly.
|
|
|
-
|
|
|
----
|
|
|
-
|
|
|
-### 30. WCAG 2.1 AA -- Animation Accessibility (prefers-reduced-motion)
|
|
|
-
|
|
|
-`PROJECT_SCOPE.md:242-244` -- The scope defines two distinct animations (slot-machine reel, scatter/eliminate) as core features. WCAG 2.3.3 (AAA) and general best practice at AA level require respecting the `prefers-reduced-motion` media query. Users with vestibular disorders can be physically affected by spinning/scattering animations. The scope mentions no motion preference handling.
|
|
|
-
|
|
|
-**Fix:** (a) Implement `prefers-reduced-motion` detection. When active: replace the slot-machine reel with an instant reveal, replace the scatter/eliminate animation with a simple fade-in or card flip. (b) The result should still feel satisfying -- use opacity transitions and scale transforms rather than spatial movement. (c) Add a manual toggle in user settings for users who want reduced motion regardless of system setting. (d) Test both animation paths.
|
|
|
-
|
|
|
-**Implementation Risk:** Designing two animation variants doubles animation development work. Start with the full animation, then create the reduced version. The reduced version can be much simpler.
|
|
|
-
|
|
|
----
|
|
|
-
|
|
|
-### 31. WCAG 2.1 AA -- Inline Panel Expansion Accessibility
|
|
|
-
|
|
|
-`PROJECT_SCOPE.md:149-172` -- The inline panel expansion (tapping a poster to expand details below the row) has several accessibility concerns: (a) No mention of keyboard navigation (how does a keyboard user open/close the panel?). (b) No mention of focus management (focus should move to the panel on open, return to the trigger on close). (c) No mention of screen reader announcements (the panel expansion should be announced). (d) The "tap outside to collapse" interaction has no keyboard equivalent.
|
|
|
-
|
|
|
-**Fix:** (a) Make movie cards focusable and openable with Enter/Space. (b) On panel open, move focus to the first interactive element in the panel. (c) On panel close, return focus to the triggering card. (d) Add Escape key to close the panel. (e) Use `aria-expanded` on the trigger card and `role="region"` with `aria-label` on the panel. (f) Announce panel open/close with `aria-live="polite"`.
|
|
|
-
|
|
|
-**Implementation Risk:** Focus management with a virtualized grid (if implementing finding #18) adds complexity. Ensure the focus trap works correctly within the expanded panel.
|
|
|
-
|
|
|
----
|
|
|
-
|
|
|
-### 32. WCAG 2.1 AA -- Delete Confirmation Accessibility
|
|
|
-
|
|
|
-`PROJECT_SCOPE.md:163-166` -- The two-tap delete flow (shake animation + text change) relies entirely on visual feedback. Screen reader users will not perceive the shake animation or the visual text change unless it is announced. Additionally, the shake animation may be disorienting for users with vestibular disorders.
|
|
|
-
|
|
|
-**Fix:** (a) On first tap, change the button's `aria-label` to "Click to confirm delete" and announce it via `aria-live="assertive"`. (b) Respect `prefers-reduced-motion` -- replace shake with a color change or border highlight. (c) Ensure the confirmation state is visually distinct even without animation (e.g., red background, different text). (d) Consider using a standard confirmation dialog (`window.confirm()` or a custom accessible modal) as an alternative to the shake pattern for accessibility.
|
|
|
-
|
|
|
-**Implementation Risk:** A confirmation dialog is more universally accessible but less visually distinctive than the shake pattern. Consider offering both: shake for sighted users, dialog for screen reader users (detect via accessibility API or offer a setting).
|
|
|
-
|
|
|
----
|
|
|
-
|
|
|
-### 33. PWA -- Offline Strategy Needs Definition
|
|
|
-
|
|
|
-`PROJECT_SCOPE.md:248,403` -- The scope mentions "offline tolerance: show cached list, disable write actions" and Phase 8.2 mentions "offline graceful degradation." However, the caching strategy is not defined: (a) Which assets are precached by the service worker? (b) How are movie lists cached for offline viewing? (c) How is the transition between online and offline states communicated to the user? (d) What happens to in-flight writes when the connection drops?
|
|
|
-
|
|
|
-**Fix:** Define the offline strategy: (a) Precache: app shell, critical CSS/JS, fonts. (b) Runtime cache: movie list data (via TanStack Query's persistence plugin or service worker). (c) On connection loss: show a non-intrusive banner ("You're offline -- viewing cached data"), disable all write buttons (add, delete, mark watched, roll), gray out disabled controls. (d) On reconnection: auto-reconnect Supabase subscription, refresh stale data, remove the banner. (e) Do NOT queue offline writes (too complex for MVP; risk of conflicts).
|
|
|
-
|
|
|
-**Implementation Risk:** TanStack Query's `persistQueryClient` plugin can handle offline caching but adds complexity. For MVP, a simpler approach is acceptable: cache the most recent list view and show it read-only when offline.
|
|
|
-
|
|
|
----
|
|
|
-
|
|
|
-## Positives
|
|
|
-
|
|
|
-- Recovery codes hashed before storage -- good security practice for account recovery
|
|
|
-- Invite code regeneration feature allows revoking compromised codes
|
|
|
-- TOTP 2FA for Master Admin rather than password-only auth
|
|
|
-- TMDB poster_path stored as relative path, not full URL -- allows flexible CDN URL construction
|
|
|
-- Debounced search (300ms) prevents excessive API calls
|
|
|
-- Real-time subscriptions for collaborative UX rather than polling
|
|
|
-- Display names only, no email/password -- good data minimization for MVP
|
|
|
-- Trailer URL stored at add-time reduces real-time API dependency
|
|
|
-- Admin self-removal triggers ownership transfer rather than orphaning the list
|
|
|
-- Two-tap delete confirmation prevents accidental deletions
|
|
|
-- Phase-based implementation plan with clear MVP cutoff
|
|
|
+- `<button>` carousel conversion is clean — no nested interactive descendants. The inner element became `<span aria-hidden>`, and `<ListMoreInfoModal>` is portaled to `document.body` (`list-more-info-modal.tsx:267-269`), so no nested-`<button>` HTML invalidity.
|
|
|
+- `/api/tmdb/search` cache stores the **filtered** payload (`NextResponse.json(filtered, ...)`). No cache poisoning of disallowed titles into shared edge cache.
|
|
|
+- `/api/tmdb/movie/[id]` correctly returns identical 404 for "doesn't exist" and "cert-blocked" — no existence leak. Intent documented in code.
|
|
|
+- `prefers-reduced-motion` honored consistently: `globals.css` opts out both `animate-emerge` and `animate-pulse-glow`; `ListRollCarousel` short-circuits to settled; `useRoll` reads the same media query.
|
|
|
+- No new `Sentry.setUser()` calls introduced. No new PII logging — `console.error` in TMDB routes logs error objects only.
|
|
|
+- `MovieListClient`'s `RollBar` gating on `allMovies.length > 0` is correct.
|
|
|
+- `RollSection` correctly gated on `hasGroups && hasMovies` — no orphan roll UI on a fresh account.
|
|
|
+- `JoinListButton` `size` prop is additive, no regression for default callers.
|
|
|
+- `SettingsPanel` shake-to-arm consistent with `ListMoreInfoModal` and `DeleteButton`. No `window.confirm` regressions.
|
|
|
+- TanStack query keys unchanged across the home-page refactor (`["tmdb-search", q]`, `useUserGroups`, `useAllUserMovies`) — cache behavior preserved.
|
|
|
+- Recovery codes, anonymous UUIDs, no-email-collection claims in `/privacy` match implemented auth flow.
|
|
|
+- Footer mounting is single per route group (no double-mount on `(public)` pages).
|
|
|
|
|
|
---
|
|
|
|
|
|
## Summary
|
|
|
|
|
|
-| Severity | Total |
|
|
|
-| ------------------ | ----- |
|
|
|
-| Critical | 6 |
|
|
|
-| Code Quality | 4 |
|
|
|
-| Documentation Gaps | 4 |
|
|
|
-| Performance | 7 |
|
|
|
-| Infrastructure | 4 |
|
|
|
-| Compliance | 8 |
|
|
|
-
|
|
|
-**Total findings: 33**
|
|
|
-
|
|
|
-This is a pre-implementation review. All findings are recommendations for the architecture and implementation plan. No source code was reviewed because none exists yet.
|
|
|
-
|
|
|
-**Top 5 actions to take before writing any code:**
|
|
|
-
|
|
|
-1. Add TMDB API server-side proxy requirement to Phase 1 (Finding #1)
|
|
|
-2. Establish linting, TypeScript strict mode, and testing infrastructure (Findings #7, #8, #9)
|
|
|
-3. Define Supabase RLS policies alongside the schema (Finding #14)
|
|
|
-4. Add TMDB attribution to every page (Finding #2)
|
|
|
-5. Create Docker deployment infrastructure in Phase 1 (Finding #22)
|
|
|
-
|
|
|
----
|
|
|
-
|
|
|
-## Files Reviewed
|
|
|
-
|
|
|
-_Every file reviewed, listed for coverage verification._
|
|
|
-
|
|
|
-| # | File | Type |
|
|
|
-| --- | ------------------ | ------------------------------------- |
|
|
|
-| 1 | `PROJECT_SCOPE.md` | Project scope / architecture document |
|
|
|
-
|
|
|
-**Note:** This project contains only the scope document. No source code, configuration files, or infrastructure files exist yet. This review covers the architectural design and implementation plan specified in PROJECT_SCOPE.md.
|
|
|
-
|
|
|
----
|
|
|
-
|
|
|
-## Second Review -- Updated Scope Analysis
|
|
|
-
|
|
|
-Reviewed: 2026-04-05 | Reviewer: Claude (automated)
|
|
|
-Scope: Second-pass review of updated PROJECT_SCOPE.md | Standard: TMDB ToS, GDPR, WCAG 2.1 AA, PWA, Docker CIS, TLS
|
|
|
-
|
|
|
-This section contains only NEW findings. The following first-review items are now addressed in the updated scope and are confirmed resolved: #1 (TMDB server proxy), #2 (TMDB attribution), #3 (privacy policy), #4 (recovery code security), #7 (linting/formatting), #8 (testing), #9 (CI via husky), #11 (API docs in markdown), #13 (migration strategy), #14 (RLS), #22 (Docker deployment), #23 (Sentry from Phase 1), #24 (env validation), #27 (privacy policy link), #28 (self-service deletion in Extra Features), #29 (12-month retention), #30 (prefers-reduced-motion), #31 (inline panel keyboard/aria).
|
|
|
-
|
|
|
----
|
|
|
-
|
|
|
-### CRITICAL
|
|
|
-
|
|
|
-### 34. TMDB Metadata Staleness -- No Refresh Mechanism for Stored Movie Data
|
|
|
-
|
|
|
-`PROJECT_SCOPE.md:326-338` -- The `movies` table stores TMDB metadata (title, year, poster_path, genres) at add-time and never refreshes it. The trailer URL refresh job only targets `trailer_url IS NULL` entries. TMDB's Terms of Service require that cached data be kept reasonably current. If TMDB updates a movie's poster, corrects a title, or reclassifies genres, the app serves stale data indefinitely. Over the 12-month retention window, this drift could be significant. Additionally, poster_path values can become invalid if TMDB re-processes images, resulting in broken poster images.
|
|
|
-
|
|
|
-**Fix:** Add a metadata freshness job (monthly cadence via pg_cron) that re-fetches title, poster_path, genres, and year from TMDB for movies added more than 30 days ago. Throttle at 3 requests/second with 429 backoff. Process in batches of 50 per run. Add a `metadata_refreshed_at` column to the `movies` table. This is the same pattern as the trailer refresh job but for core metadata.
|
|
|
-
|
|
|
-**Implementation Risk:** Additional TMDB API usage. At 3 req/s and 50 movies/run, a batch takes ~17 seconds. For large datasets, spread across multiple runs. Risk of overwriting user-visible data if TMDB makes incorrect changes -- consider logging changes for review.
|
|
|
-
|
|
|
----
|
|
|
-
|
|
|
-### 35. TMDB Adult Content Not Filtered
|
|
|
-
|
|
|
-`PROJECT_SCOPE.md:299-300` -- The TMDB API proxy routes search queries to TMDB but the scope does not specify filtering adult content from results. TMDB's API includes an `include_adult` parameter (default false for search, but true for discover/popular). The landing page "popular/top-rated" fetch for reel posters could surface adult content if the parameter is not explicitly set. Similarly, the search endpoint should explicitly exclude adult results to prevent inappropriate content appearing in a social app used by families and friend groups.
|
|
|
-
|
|
|
-**Fix:** Explicitly set `include_adult=false` on all TMDB API calls in the server proxy. Add this as a documented requirement in the TMDB proxy implementation (Phase 1.3 / Phase 3.1). Also filter results server-side by checking the `adult` field on each result object, since some adult-flagged movies can still appear in non-adult searches depending on TMDB's classification.
|
|
|
-
|
|
|
-**Implementation Risk:** Minimal. This is a query parameter addition. Double-filtering (parameter + server-side check) provides defense in depth.
|
|
|
-
|
|
|
----
|
|
|
-
|
|
|
-### 36. Supabase Studio Publicly Accessible in Docker Stack
|
|
|
-
|
|
|
-`PROJECT_SCOPE.md:391` -- The self-hosted Supabase Docker stack includes Supabase Studio (the admin dashboard UI), which by default binds to a port accessible from outside the container. Studio provides full database access using the service role key. If Studio is exposed through Caddy or directly on a public port, anyone who discovers it has unrestricted read/write access to every table, bypassing all RLS policies.
|
|
|
-
|
|
|
-**Fix:** (a) Do NOT expose the Supabase Studio port in docker-compose (remove or comment out the port mapping). (b) If Studio access is needed, bind it to `127.0.0.1` only and access via SSH tunnel. (c) Alternatively, add Caddy basic auth or IP restriction in front of the Studio route. (d) Document this as a security-critical operational requirement.
|
|
|
-
|
|
|
-**Implementation Risk:** Developers lose convenient Studio access during development. SSH tunnel is the standard solution for self-hosted deployments. During local development, Studio can be accessed directly.
|
|
|
-
|
|
|
----
|
|
|
-
|
|
|
-### COMPLIANCE
|
|
|
-
|
|
|
-### 37. GDPR -- Supabase `auth.users` Table Not Addressed in Deletion Flow
|
|
|
-
|
|
|
-`PROJECT_SCOPE.md:569` -- The self-service account deletion (Extra Features) and Master Admin deletion mention cascading deletes on the app's `public.users` table. However, Supabase's GoTrue auth system maintains its own `auth.users` table containing: user UUID, created_at, last_sign_in_at, raw_app_meta_data, raw_user_meta_data, and the `is_anonymous` flag. Deleting a row from `public.users` does NOT delete the corresponding `auth.users` record. Under GDPR Article 17, erasure must be complete. An orphaned `auth.users` record still constitutes retained personal data.
|
|
|
-
|
|
|
-**Fix:** Account deletion must call `supabase.auth.admin.deleteUser(userId)` using the service role key to remove the `auth.users` record. This should be performed server-side (API route or Server Action) after the application-level cascade completes. Document this as a required step in both the self-service deletion flow and the Master Admin deletion flow.
|
|
|
-
|
|
|
-**Implementation Risk:** The `admin.deleteUser` call requires the service role key and must be server-side only. If the GoTrue delete fails after the public table cascade succeeds, the user is partially deleted -- implement as a transaction or handle the error with a retry/alert mechanism.
|
|
|
-
|
|
|
----
|
|
|
-
|
|
|
-### 38. GDPR -- Supabase Component Logs Contain Personal Data with No Rotation
|
|
|
-
|
|
|
-`PROJECT_SCOPE.md:389-394` -- The self-hosted Docker stack includes Kong (API gateway), GoTrue (auth), PostgREST, and Realtime -- each producing logs containing IP addresses, user agent strings, JWTs, and request paths (which may include user IDs). Docker's default logging driver (`json-file`) has no size limit or rotation. These logs (a) contain personal data under GDPR, (b) grow unbounded consuming disk space, and (c) have no defined retention period. The scope's 12-month data retention policy and privacy policy mention server logs with IPs but do not address Supabase's own container logs.
|
|
|
-
|
|
|
-**Fix:** (a) Configure Docker log rotation for ALL containers in docker-compose: `logging: { driver: "json-file", options: { max-size: "10m", max-file: "5" } }`. (b) Document that Supabase container logs contain personal data. (c) Include Supabase logs in the privacy policy's log retention disclosure (recommend 30-day effective retention via rotation). (d) If log aggregation is added later, ensure the aggregation target also has a retention policy.
|
|
|
-
|
|
|
-**Implementation Risk:** Log rotation means older logs are lost. For debugging production issues, 50MB per container (5 x 10MB) is sufficient for recent history. If forensic logging is needed, consider shipping logs to a dedicated store with its own retention policy.
|
|
|
-
|
|
|
----
|
|
|
-
|
|
|
-### 39. GDPR -- Sentry Error Reports May Contain User Context
|
|
|
-
|
|
|
-`PROJECT_SCOPE.md:422` -- Sentry is added in Phase 1.8 for error monitoring. By default, Sentry's JavaScript SDK captures: error stack traces, breadcrumbs (user interactions, console logs, network requests), request URLs (which may contain user/group IDs), and browser metadata. If Sentry's `setUser()` is called or if user context appears in error messages, Sentry receives personal data. This constitutes an international data transfer (to Sentry's US servers) requiring disclosure in the privacy policy and potentially a Standard Contractual Clauses (SCC) basis under GDPR.
|
|
|
-
|
|
|
-**Fix:** (a) Configure Sentry's `beforeSend` callback to strip any user-identifying information from error events. (b) Do NOT call `Sentry.setUser()` with the anonymous UUID. (c) Sanitize URLs in breadcrumbs to remove UUID path segments. (d) Disclose Sentry as a third-party data processor in the privacy policy. (e) If using Sentry's cloud service, reference Sentry's DPA and SCC compliance in the privacy policy. (f) Alternatively, self-host Sentry to keep all error data on-premises.
|
|
|
-
|
|
|
-**Implementation Risk:** Aggressive sanitization may make debugging harder (errors without user context are harder to reproduce). A middle ground: use a hashed/truncated user identifier that cannot be reversed to the original UUID.
|
|
|
-
|
|
|
----
|
|
|
-
|
|
|
-### 40. GDPR -- Privacy Policy Missing Required Sections
|
|
|
-
|
|
|
-`PROJECT_SCOPE.md:373` -- The scope describes the privacy policy as "factual description of what data is stored, how it is used, and user rights." This is a good start but GDPR (Articles 13-14) and CCPA require specific sections that are not mentioned: (a) identity and contact details of the data controller, (b) lawful basis for each processing activity (Art. 6(1) -- likely legitimate interest or contract performance), (c) international data transfers (TMDB API calls to TMDB servers, Sentry to Sentry servers), (d) automated decision-making disclosure (the emotion-to-genre mapping is algorithmic but likely does not qualify as "solely automated" under Art. 22), (e) right to lodge a complaint with a supervisory authority, (f) CCPA categories of personal information and "do not sell" disclosure, (g) children's data disclaimer (COPPA: under-13; GDPR: under-16 in some member states).
|
|
|
-
|
|
|
-**Fix:** Create a privacy policy template with all required sections before implementation. At minimum: controller identity, data inventory with lawful basis per item, retention periods per data type, third-party recipients (TMDB, Sentry), international transfers, full list of user rights with exercise instructions, children's disclaimer, cookie/localStorage disclosure, and change notification procedure. Phase 5.1 should reference this template.
|
|
|
-
|
|
|
-**Implementation Risk:** A comprehensive privacy policy for an anonymous-auth app is unusual and may feel heavyweight. Keep language plain and concise. Consider using a privacy policy generator as a starting point, then customizing.
|
|
|
-
|
|
|
----
|
|
|
-
|
|
|
-### 41. WCAG 2.1 AA -- Slot Machine Reel Violates 2.2.2 Pause, Stop, Hide
|
|
|
-
|
|
|
-`PROJECT_SCOPE.md:74-79` -- WCAG 2.2.2 requires that any moving, blinking, or scrolling content that (a) starts automatically, (b) lasts more than 5 seconds, and (c) is presented alongside other content must have a mechanism to pause, stop, or hide it. The slot-machine reel animation is user-triggered (button press), so it does NOT auto-start -- however, if the reel spin duration exceeds 5 seconds or if the reels loop/auto-play on page load as a decorative element, this criterion applies. The scope specifies the in-app roll at "2-3 seconds" but does not specify the landing page reel duration. Additionally, `prefers-reduced-motion` replaces the animation entirely but does not provide a pause/stop control for users who want motion but need control.
|
|
|
-
|
|
|
-**Fix:** (a) Ensure the landing page reel animation is strictly user-triggered (no auto-play or looping on page load). (b) Cap the reel animation duration at under 5 seconds. (c) If any decorative animation loops (e.g., a subtle poster scroll preview before the user taps Roll), add a pause button. (d) Document the animation durations as testable success criteria.
|
|
|
-
|
|
|
-**Implementation Risk:** Minimal if animations are user-triggered and time-bounded. The main risk is a future design decision to add auto-playing reel teasers on the landing page -- if added, a pause control would be required.
|
|
|
-
|
|
|
----
|
|
|
-
|
|
|
-### 42. WCAG 2.1 AA -- Poster Images Missing Alt Text Specification
|
|
|
-
|
|
|
-`PROJECT_SCOPE.md:146-152, 160-161` -- The scope specifies poster `<img>` tags with `loading="lazy"` but does not specify `alt` text for any poster image. WCAG 1.1.1 (Non-text Content) requires meaningful alt text for all informative images. Posters in the grid, the expanded panel, the reel animation, and the roll result all need alt text. During the reel spin animation, rapidly cycling posters should be `aria-hidden="true"` (they are decorative in that context), but the final result poster must have alt text.
|
|
|
-
|
|
|
-**Fix:** (a) All poster `<img>` tags: `alt="[Movie Title] ([Year]) poster"`. (b) Reel animation posters during spin: `aria-hidden="true"` on the container or individual images. (c) Roll result poster: meaningful alt text. (d) Added-by avatar overlay and binoculars emoji overlay: `aria-hidden="true"` with meaning conveyed via `aria-label` on the parent card element or screen-reader-only text (e.g., `<span class="sr-only">Watched</span>`). (e) Add this to the component requirements in Phase 3.4 and 3.6.
|
|
|
-
|
|
|
-**Implementation Risk:** None. This is a straightforward implementation requirement.
|
|
|
-
|
|
|
----
|
|
|
-
|
|
|
-### 43. WCAG 2.1 AA -- Status Messages Not Announced (WCAG 4.1.3)
|
|
|
-
|
|
|
-`PROJECT_SCOPE.md:194, 459` -- Several user actions produce status messages that sighted users perceive visually but screen reader users would miss: (a) roll result ("You got: [Movie Title]"), (b) search results count, (c) genre filter applied ("Showing [N] movies in [Genre]"), (d) "No matches -- showing full list" fallback, (e) movie added/removed confirmation, (f) watched state toggle confirmation. WCAG 4.1.3 (Status Messages, AA) requires that status messages be programmatically determinable via `role="status"` or `aria-live` without receiving focus.
|
|
|
-
|
|
|
-**Fix:** Wrap dynamic status messages in an `aria-live="polite"` region (or use `role="status"`) for: roll results, search result counts, filter state changes, empty state messages, and action confirmations. Use `aria-live="assertive"` only for error messages and the delete confirmation state change. Do NOT move focus to these messages -- let the live region announce them.
|
|
|
-
|
|
|
-**Implementation Risk:** Multiple `aria-live` regions competing can cause announcement queuing issues. Use a single shared announcer component (common pattern: a visually hidden div that receives message text via state) rather than multiple live regions scattered across components.
|
|
|
-
|
|
|
----
|
|
|
-
|
|
|
-### INFRASTRUCTURE
|
|
|
-
|
|
|
-### 44. Docker Compose Missing Security Hardening (CIS Benchmark)
|
|
|
-
|
|
|
-`PROJECT_SCOPE.md:389-394` -- The scope specifies a docker-compose configuration with non-root user, tini, and health check, which is good. However, several CIS Docker Benchmark controls are not addressed: (a) no capability dropping (`cap_drop: [ALL]`), (b) no `no-new-privileges` security option, (c) no memory limits specified, (d) no CPU limits, (e) no read-only root filesystem. These are standard hardening measures for production Docker deployments.
|
|
|
-
|
|
|
-**Fix:** Add to the docker-compose service definition for the Next.js container:
|
|
|
-
|
|
|
-```yaml
|
|
|
-security_opt:
|
|
|
- - no-new-privileges:true
|
|
|
-cap_drop:
|
|
|
- - ALL
|
|
|
-mem_limit: 512m
|
|
|
-memswap_limit: 512m
|
|
|
-cpus: 1.0
|
|
|
-read_only: true
|
|
|
-tmpfs:
|
|
|
- - /tmp
|
|
|
- - /app/.next/cache
|
|
|
-```
|
|
|
-
|
|
|
-Apply similar hardening to Supabase containers where applicable (some Supabase containers may need specific capabilities).
|
|
|
-
|
|
|
-**Implementation Risk:** `read_only: true` requires identifying all writable paths and mounting them as `tmpfs` or named volumes. The Next.js standalone output writes to `.next/cache` at runtime. Test thoroughly to ensure no write operations fail. Supabase containers (especially Postgres) need writable data directories.
|
|
|
-
|
|
|
----
|
|
|
-
|
|
|
-### 45. Caddy HSTS Header Must Be Configured at Proxy Level
|
|
|
-
|
|
|
-`PROJECT_SCOPE.md:379-384, 392` -- The scope mentions HSTS in the security headers section and suggests configuring it in `next.config.js` or Caddy. For HSTS to be effective, it MUST be set at the TLS termination point, which is Caddy. If HSTS is only set in Next.js, it is applied after TLS has already been established, which is correct timing but means Caddy could theoretically serve a non-HSTS response if the request bypasses Next.js. More importantly, Caddy does NOT add HSTS by default. The scope should explicitly mandate HSTS configuration in the Caddyfile rather than leaving it ambiguous ("or").
|
|
|
-
|
|
|
-**Fix:** Configure HSTS in the Caddyfile explicitly:
|
|
|
-
|
|
|
-```
|
|
|
-header Strict-Transport-Security "max-age=63072000; includeSubDomains; preload"
|
|
|
-```
|
|
|
-
|
|
|
-Other security headers (CSP, X-Frame-Options, etc.) can be set in either Caddy or Next.js, but HSTS should be at the Caddy level. Remove the ambiguity -- specify which headers go where.
|
|
|
-
|
|
|
-**Implementation Risk:** HSTS with `preload` and a long max-age is difficult to undo once deployed. Start with a shorter max-age (e.g., `max-age=86400`) during testing, then increase to production values. Do NOT submit to the HSTS preload list until confident the domain will always serve HTTPS.
|
|
|
-
|
|
|
----
|
|
|
-
|
|
|
-### 46. Self-Hosted Supabase PostgreSQL -- No Encryption at Rest
|
|
|
-
|
|
|
-`PROJECT_SCOPE.md:391` -- The self-hosted Supabase stack runs PostgreSQL in a Docker container with data stored on a Docker volume. Docker volumes are NOT encrypted by default. The database contains user data (UUIDs, display names, group membership, movie preferences) that constitutes personal data under GDPR. While GDPR does not strictly mandate encryption at rest, Article 32 requires "appropriate technical measures" to protect personal data, and encryption at rest is listed as an example measure. An unencrypted volume on a compromised host exposes all user data.
|
|
|
-
|
|
|
-**Fix:** (a) Use full-disk encryption on the Docker host (LUKS on Linux, FileVault on macOS, BitLocker on Windows). This is the simplest approach and protects all Docker volumes. (b) Document this as a deployment requirement. (c) If the host is a cloud VM, enable the cloud provider's disk encryption feature. (d) For defense in depth, consider column-level encryption via pgcrypto for the most sensitive fields (recovery_code is already hashed, but display_name and group membership are plaintext).
|
|
|
-
|
|
|
-**Implementation Risk:** Full-disk encryption has negligible performance impact on modern hardware with AES-NI. Column-level encryption adds application complexity and prevents SQL queries on encrypted columns. Recommend host-level encryption as sufficient for this app's threat model.
|
|
|
-
|
|
|
----
|
|
|
-
|
|
|
-### 47. No Database Backup Strategy in Updated Scope
|
|
|
-
|
|
|
-`PROJECT_SCOPE.md:389-404` -- Finding #25 from the first review flagged the missing backup strategy. The updated scope now specifies self-hosted Supabase but still does not include a backup mechanism. For self-hosted PostgreSQL, there are NO automatic backups. A Docker volume failure, accidental `DROP TABLE`, or host disk failure results in complete data loss. This is especially critical because anonymous auth means users cannot be contacted to rebuild their accounts.
|
|
|
-
|
|
|
-**Fix:** Add a backup task to Phase 1.7 (Docker infrastructure): (a) Create a `pg_dump` cron job (daily, retain 7 days) writing to a separate volume or off-host storage. (b) Add a `backup` service in docker-compose that runs the dump on a schedule. (c) Document the restore procedure. (d) Test restore at least once before launch. (e) Consider WAL archiving for point-in-time recovery if data volume warrants it. Example:
|
|
|
-
|
|
|
-```yaml
|
|
|
-backup:
|
|
|
- image: postgres:16
|
|
|
- command: >
|
|
|
- sh -c 'while true; do
|
|
|
- pg_dump -h db -U postgres > /backups/moviedice_$$(date +%Y%m%d).sql
|
|
|
- find /backups -mtime +7 -delete
|
|
|
- sleep 86400
|
|
|
- done'
|
|
|
- volumes:
|
|
|
- - ./backups:/backups
|
|
|
-```
|
|
|
-
|
|
|
-**Implementation Risk:** Backup storage consumes disk space. For a small app, daily dumps are small (< 1 MB). The backup container needs access to the Postgres container's network and credentials. Ensure backup files are not publicly accessible.
|
|
|
-
|
|
|
----
|
|
|
-
|
|
|
-### PERFORMANCE
|
|
|
-
|
|
|
-### 48. TMDB Server Proxy Missing 429 Rate Limit Handling
|
|
|
-
|
|
|
-`PROJECT_SCOPE.md:299-300` -- The TMDB server proxy (`/api/tmdb/*`) routes all client requests through the Next.js server to TMDB. The scope specifies debounce (300ms) and TanStack Query caching, which reduces request volume. However, there is no specification for handling TMDB's HTTP 429 (Too Many Requests) responses. If multiple users search simultaneously or the trailer/reel refresh jobs run during peak usage, the proxy could hit the ~40 req/10s limit. Without 429 handling, the proxy would pass through TMDB error responses to the client, causing confusing failures.
|
|
|
-
|
|
|
-**Fix:** (a) Implement server-side rate limiting on the TMDB proxy using a token bucket or sliding window (e.g., 30 req/10s to stay under TMDB's 40 req/10s with headroom for background jobs). (b) On receiving a 429 from TMDB, read the `Retry-After` header and return a friendly error to the client with a retry suggestion. (c) Queue or delay background job requests to avoid competing with interactive user requests. (d) Add `Cache-Control` response headers on the proxy to enable HTTP-level caching of TMDB responses (e.g., `public, max-age=300` for search results).
|
|
|
-
|
|
|
-**Implementation Risk:** Server-side rate limiting requires in-memory state (or Redis). For a single-instance Docker deployment, in-memory is fine. The rate limiter should prioritize interactive requests over background jobs.
|
|
|
-
|
|
|
----
|
|
|
-
|
|
|
-### DOCUMENTATION GAPS
|
|
|
-
|
|
|
-### 49. Invite Code Entropy Not Quantified
|
|
|
-
|
|
|
-`PROJECT_SCOPE.md:35, 315` -- The first review (Finding #5) flagged the low entropy of invite codes like WOLF-42. The updated scope changes the format to WORD-WORD (e.g., WOLF-MOON), but does not specify the word list size. If the word list has 1,000 words, WORD-WORD yields 1,000,000 combinations -- brute-forceable in under a day even with rate limiting at 5 attempts/window. If the word list has 4,000 words, it yields 16,000,000 combinations, which is more robust but still feasible for a determined attacker. The rate limiting on the join endpoint ("5-10 failed attempts per IP per window") does not specify the window duration.
|
|
|
-
|
|
|
-**Fix:** (a) Specify the word list size (recommend 4,000+ words for ~22 bits of entropy, or add a third word for ~36 bits). (b) Specify the rate limit window duration (recommend 15 minutes). (c) Document the expected brute-force time given the word list size and rate limit parameters. (d) Consider adding a numeric suffix (WOLF-MOON-73) for additional entropy without significant UX cost.
|
|
|
-
|
|
|
-**Implementation Risk:** Larger word lists or three-word codes reduce memorability. WOLF-MOON-73 is still human-readable and provides ~29 bits with a 4,000-word list + 2-digit suffix. Rate limiting is the primary defense; code entropy is defense in depth.
|
|
|
-
|
|
|
----
|
|
|
-
|
|
|
-### 50. PWA Manifest and Icon Requirements Not Specified
|
|
|
-
|
|
|
-`PROJECT_SCOPE.md:275, 481` -- The scope specifies `@serwist/next` for PWA support and mentions "home screen installation" but does not specify: (a) the `display` mode for the manifest (should be `standalone`), (b) required icon sizes (192x192 and 512x512 minimum, plus maskable variants for Android adaptive icons), (c) iOS-specific meta tags (`apple-mobile-web-app-capable`, `apple-touch-icon`, `apple-touch-startup-image` for splash screens), (d) manifest fields (`name`, `short_name`, `start_url`, `theme_color`, `background_color`). Without these specifications, Phase 8.1 implementation may miss requirements for cross-platform installability.
|
|
|
-
|
|
|
-**Fix:** Add a PWA configuration specification to the scope or Phase 8.1: (a) `display: "standalone"`, (b) Icons: 192x192, 512x512 (PNG), plus maskable variants with safe zone, (c) `name: "MovieDice"`, `short_name: "MovieDice"`, (d) `start_url: "/"`, (e) `theme_color` and `background_color` matching the app's color scheme, (f) iOS meta tags for Safari PWA support, (g) Service worker must NOT cache `/api/*` routes or WebSocket connections.
|
|
|
-
|
|
|
-**Implementation Risk:** Maskable icon design requires a safe zone (inner 80% circle). The app icon must look good both as a full-bleed and as a masked circle. iOS splash screen images require multiple resolutions -- `@serwist/next` may or may not auto-generate these.
|
|
|
-
|
|
|
----
|
|
|
-
|
|
|
-### 51. Error Response Format Not Standardized
|
|
|
-
|
|
|
-`PROJECT_SCOPE.md:459` -- Finding #12 from the first review flagged the missing error response standard. The updated scope adds error handling in Phase 5.6 ("invalid invite code, TMDB API failure, network errors") but still does not define a standard error response shape, error codes, or error categorization. Without a standard, each API route will return errors in ad-hoc formats, making client-side error handling inconsistent and brittle.
|
|
|
-
|
|
|
-**Fix:** Define before Phase 2 implementation begins: (a) Standard error response: `{ error: { code: string, message: string, details?: unknown } }`. (b) Error code enum: `INVALID_INVITE_CODE`, `TMDB_UNAVAILABLE`, `TMDB_RATE_LIMITED`, `RATE_LIMITED`, `UNAUTHORIZED`, `NOT_FOUND`, `VALIDATION_ERROR`, `INTERNAL_ERROR`. (c) Map each code to a user-friendly message string. (d) Create a shared `createErrorResponse(code, details?)` utility used by all API routes. (e) Document error codes in the API markdown documentation.
|
|
|
-
|
|
|
-**Implementation Risk:** None. Defining this early prevents inconsistency. Retrofitting error formats after multiple API routes exist is much harder.
|
|
|
-
|
|
|
----
|
|
|
-
|
|
|
-## Second Review Summary
|
|
|
-
|
|
|
-| Severity | New Findings |
|
|
|
-| ------------------ | ---------------------- |
|
|
|
-| Critical | 3 (#34, #35, #36) |
|
|
|
-| Compliance | 4 (#37, #38, #39, #40) |
|
|
|
-| Infrastructure | 4 (#44, #45, #46, #47) |
|
|
|
-| Performance | 1 (#48) |
|
|
|
-| Documentation Gaps | 3 (#49, #50, #51) |
|
|
|
-
|
|
|
-**New findings total: 15** (numbered 34-51, continuing from first review)
|
|
|
-
|
|
|
-### Positives -- Improvements Since First Review
|
|
|
-
|
|
|
-- Server-side TMDB proxy now explicitly specified with `NEXT_PUBLIC_` prohibition -- eliminates API key exposure
|
|
|
-- TMDB attribution footer on all pages with logo + link + disclaimer -- ToS compliant
|
|
|
-- Privacy policy page added as Phase 5.1 deliverable
|
|
|
-- 12-month data retention policy with auto-delete -- addresses GDPR storage limitation
|
|
|
-- Recovery code hardened: 128-bit entropy, Argon2id, rate limiting, single-use -- solid specification
|
|
|
-- Invite code format improved to WORD-WORD with rate limiting on join
|
|
|
-- RLS policies defined per-table with specific access rules
|
|
|
-- prefers-reduced-motion respected for both animation types
|
|
|
-- Inline panel keyboard navigation with aria-expanded and role="region"
|
|
|
-- ESLint + Prettier + TypeScript strict + husky enforced from Phase 1
|
|
|
-- Vitest + Playwright testing established
|
|
|
-- Sentry monitoring moved to Phase 1 (was Phase 10)
|
|
|
-- Supabase migrations workflow via CLI
|
|
|
-- Security headers (CSP, HSTS, X-Frame-Options) specified
|
|
|
-- Self-service account deletion added to Extra Features backlog
|
|
|
-- Env validation via zod at startup
|
|
|
-- Docker infrastructure fully specified: multi-stage build, non-root user, tini, health check
|
|
|
-- iron-session with HttpOnly/Secure/SameSite=Strict and 8-hour expiry for admin
|
|
|
-- Caddy reverse proxy for HTTPS termination
|
|
|
-- TMDB native image sizes avoiding in-container sharp processing -- good performance decision
|
|
|
-- Real-time subscriptions scoped to active list only with useEffect cleanup
|
|
|
-
|
|
|
-### Top 5 Actions for Updated Scope
|
|
|
-
|
|
|
-1. Secure Supabase Studio -- do not expose publicly in docker-compose (Finding #36)
|
|
|
-2. Configure Docker log rotation and container security hardening (Findings #38, #44)
|
|
|
-3. Add `auth.users` cleanup to account deletion flows (Finding #37)
|
|
|
-4. Specify TMDB adult content filtering and 429 handling in proxy (Findings #35, #48)
|
|
|
-5. Add database backup mechanism to Phase 1.7 (Finding #47)
|
|
|
+| Severity | Total |
|
|
|
+| -------- | ----- |
|
|
|
+| Critical | 2 |
|
|
|
+| High | 2 |
|
|
|
+| Medium | 6 |
|
|
|
+| Low | 4 |
|
User