Add critical architecture and quality review findings

REVIEW.md

@@ -1,24 +1,163 @@
-# Maintainability Review - Pick'n'Play
+# Review - Pick'n'Play
 
-## A) Current focus
+Date: 2026-02-08
 
-This document tracks only active work. Completed work is intentionally omitted and can be reviewed in git history.
+## Scope and baseline
 
-Active maintainability risks (priority order):
+- Reviewed backend, frontend, data model, tests, CI, deployment scripts, and documentation.
+- Ran local CI checks via `scripts/ci-local.ps1 -SkipNpmInstall` (pass: lint, format, build, tests).
+- Ran coverage collection (`dotnet test --collect:"XPlat Code Coverage"`).
+- Latest collected coverage: line-rate `92.75%`, branch-rate `74.88%` (`GameList.Tests/TestResults/*/coverage.cobertura.xml:2`).
 
-- None at the moment.
+## Executive summary
 
-## B) Active task list
+The app is in a solid functional state for a small group and has good safety basics (auth, rate limits, tests, security headers).  
+However, several design and reliability issues will slow down extension work and make this repo hard to use as a reusable foundation for new apps unless addressed first.
 
-- None.
+## Findings (highest priority first)
 
-## C) Suggested execution order
+### P0 - Core invariants are not concurrency-safe
 
-1. Add new items when fresh risks are identified.
+Evidence:
+- Owner bootstrap is check-then-insert (`Endpoints/AuthEndpoints.cs:45`, `Endpoints/AuthEndpoints.cs:70`, `Endpoints/AuthEndpoints.cs:71`) without a DB-level uniqueness guard for owner role (`Data/AppDbContext.cs:25`).
+- Suggestion cap (<=5) is count-then-insert (`Endpoints/SuggestionWorkflowService.cs:63`, `Endpoints/SuggestionWorkflowService.cs:64`, `Endpoints/SuggestionWorkflowService.cs:82`, `Endpoints/SuggestionWorkflowService.cs:92`).
+- Vote upsert is read-then-insert/update (`Endpoints/VoteWorkflowService.cs:70`, `Endpoints/VoteWorkflowService.cs:79`, `Endpoints/VoteWorkflowService.cs:92`) and can race into unique-key conflicts.
 
-## D) Guardrails
+Impact:
+- Under concurrent requests, business rules can be violated (multiple owners, over-limit suggestions) or requests can fail with server errors.
 
-- Keep endpoint handlers transport-focused and move business rules into services/validators.
+Recommendation:
-- Keep reads side-effect free and isolate all persistence changes to explicit command paths.
+- Enforce invariants in the DB and handle race paths explicitly (transaction isolation, constrained indexes, retry/409 handling on expected conflicts).
-- Maintain one source of truth per validation rule (backend authoritative, frontend UX hints only).
+
-- Prefer typed DTOs over anonymous response shapes for non-trivial API payloads.
+### P0 - Business layer is tightly coupled to HTTP transport
+
+Evidence:
+- Workflow services return `Task<IResult>` directly across the board (`Endpoints/SuggestionWorkflowService.cs:10`, `Endpoints/VoteWorkflowService.cs:10`, `Endpoints/ResultsWorkflowService.cs:10`, `Endpoints/StateWorkflowService.cs:10`, `Endpoints/AdminWorkflowService.cs:11`).
+
+Impact:
+- Domain logic cannot be reused cleanly by other apps/services without carrying ASP.NET HTTP result types everywhere.
+- Harder to unit test business logic in isolation.
+
+Recommendation:
+- Split into application services (domain result objects/errors) + thin endpoint adapters (HTTP mapping only).
+
+### P1 - Startup/runtime side effects are risky for production scale-out
+
+Evidence:
+- Automatic schema migration on app startup (`Program.cs:161`).
+- Runtime mutation of static frontend file for base path (`Program.cs:149`, `Program.cs:277`), with silent catch (`Program.cs:313`).
+
+Impact:
+- Multi-instance startup races and operational fragility.
+- Read-only deployments or immutable artifacts can fail subtly.
+
+Recommendation:
+- Move migrations to explicit deployment step; remove runtime file rewrite and make base path purely configuration-driven at build/deploy time.
+
+### P1 - Null-safety gaps in auth input validation can produce 500s
+
+Evidence:
+- Direct `.Trim()` on potentially null request values (`Endpoints/AuthValidator.cs:15`, `Endpoints/AuthValidator.cs:71`, `Endpoints/AuthEndpoints.cs:26`, `Endpoints/AuthEndpoints.cs:90`).
+
+Impact:
+- Malformed JSON payloads can bypass intended 400 responses and trigger 500 errors.
+
+Recommendation:
+- Treat inbound fields as nullable at boundaries, normalize safely, and fail closed with typed validation errors.
+
+### P1 - Deployment automation is environment-specific and hard-coded
+
+Evidence:
+- Script contains fixed host/user/path/tooling details (`scripts/deploy-ftp.ps1:2`, `scripts/deploy-ftp.ps1:3`, `scripts/deploy-ftp.ps1:11`, `scripts/deploy-ftp.ps1:15`).
+- Uses direct FTP credential in command string (`scripts/deploy-ftp.ps1:102`).
+
+Impact:
+- Not portable, hard to onboard new maintainers/environments, and higher operational/security risk.
+
+Recommendation:
+- Externalize all environment values into secure config; provide a generic deploy profile template per environment.
+
+### P1 - Coverage policy and reality are misaligned
+
+Evidence:
+- Documentation claims target of 100% line/branch (`TESTS.md:96`).
+- Actual measured coverage is lower (`GameList.Tests/TestResults/*/coverage.cobertura.xml:2`).
+- CI/local scripts run tests but do not enforce coverage thresholds (`.github/workflows/ci.yml:43`, `scripts/ci-local.ps1:56`, `scripts/ci-local.ps1:59`).
+
+Impact:
+- False confidence in quality gates and unclear definition of done.
+
+Recommendation:
+- Decide real threshold policy, enforce it in CI, and keep docs aligned with measured truth.
+
+### P2 - Frontend refresh strategy is chatty and DB-heavy
+
+Evidence:
+- Global polling every 4s (`wwwroot/app.js:27`, `wwwroot/app.js:49`).
+- Each cycle can trigger multiple API reads (`wwwroot/js/data.js:6`, `wwwroot/js/data.js:93`, `wwwroot/js/data.js:101`).
+
+Impact:
+- Scales poorly with player count/sessions; unnecessary backend load.
+
+Recommendation:
+- Move to event-driven or adaptive refresh (backoff, ETag/delta, push where needed).
+
+### P2 - Frontend DOM drift and invalid markup
+
+Evidence:
+- Missing closing `</div>` for status bar around section boundary (`wwwroot/index.html:83`, `wwwroot/index.html:102`).
+- JS references elements not present in HTML: `all-suggestions`, `nav-vote-next` (`wwwroot/js/suggestions-ui.js:52`, `wwwroot/js/votes-ui.js:264`).
+
+Impact:
+- Browser autocorrection masks real bugs, increases regression risk, and confuses future contributors.
+
+Recommendation:
+- Reconcile HTML/JS contracts and remove dead UI paths.
+
+### P2 - Login flow can fail silently for non-auth errors
+
+Evidence:
+- In login catch block, non-401/non-auth-handler errors are swallowed without user feedback (`wwwroot/js/app-auth-handlers.js:113`, `wwwroot/js/app-auth-handlers.js:116`).
+
+Impact:
+- Poor UX and harder support/debugging.
+
+Recommendation:
+- Add explicit fallback toast/logging for unexpected errors in login flow.
+
+### P2 - Formatting checks do not cover the whole frontend codebase
+
+Evidence:
+- ESLint scans all JS (`package.json:6`), but Prettier scripts only include a subset of files (`package.json:7`, `package.json:8`).
+
+Impact:
+- Style drift and inconsistent diffs across untouched files.
+
+Recommendation:
+- Align formatting scope with lint scope.
+
+### P2 - Suggestion validation does synchronous external network checks on write paths
+
+Evidence:
+- Suggestion validation calls image reachability check (`Endpoints/SuggestionValidator.cs:13`).
+- Reachability check performs outbound call with timeout per request (`Endpoints/EndpointHelpers.cs:175`, `Endpoints/EndpointHelpers.cs:194`, `Endpoints/EndpointHelpers.cs:197`).
+
+Impact:
+- Slower writes and external dependency coupling even for basic edit operations.
+
+Recommendation:
+- Make this validation less blocking (cache results, only revalidate when URL changes, or async verification workflow).
+
+## Positive foundations worth keeping
+
+- Security baseline is present: cookie hardening + rate limiting + security headers (`Program.cs:100`, `Program.cs:104`, `Program.cs:121`, `Program.cs:137`).
+- Global exception handling and health endpoint exist (`Infrastructure/PlayerIdentityExtensions.cs:40`, `Infrastructure/PlayerIdentityExtensions.cs:68`).
+- Broad integration test suite exists and is currently green (104 tests).
+
+## Suggested initial remediation order
+
+1. Concurrency/invariant hardening for owner bootstrap, suggestion limits, and vote upsert.
+2. Service boundary refactor (`IResult` decoupling) to make logic reusable for future apps.
+3. Startup/deployment hardening (migration strategy + remove runtime file rewrite + parameterized deploy script).
+4. Coverage policy enforcement and documentation correction.
+5. Frontend cleanup pass (invalid markup, dead selectors, polling strategy, error handling).