diff --git a/REVIEW.md b/REVIEW.md index c7d02f5..f880bc2 100644 --- a/REVIEW.md +++ b/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. -- Keep reads side-effect free and isolate all persistence changes to explicit command paths. -- 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. +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` 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 `` 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).