GameList/REVIEW.md

# Review - Pick'n'Play

Date: 2026-02-08

## Scope and baseline

- 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`).

## Executive summary

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.

## Findings (highest priority first)

### P0 - Core invariants are not concurrency-safe

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.

Impact:
- Under concurrent requests, business rules can be violated (multiple owners, over-limit suggestions) or requests can fail with server errors.

Recommendation:
- Enforce invariants in the DB and handle race paths explicitly (transaction isolation, constrained indexes, retry/409 handling on expected conflicts).

### 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).