From 25b48e01956686e1caef48ae6019998b75efd8c2 Mon Sep 17 00:00:00 2001 From: Frank Tovar Date: Sat, 7 Feb 2026 00:03:13 +0100 Subject: [PATCH] Add maintainability review backlog --- REVIEW.md | 227 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 227 insertions(+) create mode 100644 REVIEW.md diff --git a/REVIEW.md b/REVIEW.md new file mode 100644 index 0000000..45c668c --- /dev/null +++ b/REVIEW.md @@ -0,0 +1,227 @@ +# Maintainability Review - Pick'n'Play + +## A) Executive summary + +This codebase is functional and reasonably tested on backend behavior, but long-term change safety is currently limited by concentration risk (god modules), hidden side effects in read paths, and drifting operational contracts. + +Top 5 maintainability risks (priority order): + +1. Hidden state mutation in phase reads (Critical) +- `Endpoints/EndpointHelpers.cs:37` (`GetPhase`) reads and mutates player phase, then writes via `SaveChangesAsync` (`Endpoints/EndpointHelpers.cs:69`). +- This function is called from GET paths (`Endpoints/StateEndpoints.cs:19`, `Endpoints/StateEndpoints.cs:42`) and filters (`Infrastructure/PhaseRequirementFilter.cs:17`), creating non-obvious write behavior and duplicate checks. +- Impact: hard-to-debug regressions, surprising side effects, and fragile refactors. + +2. Operational contract drift between docs/scripts/code (High) +- `API.md:3` claims admin auth via `X-Admin-Key`/`key`; admin runtime actually checks authenticated admin user (`Infrastructure/AdminOnlyFilter.cs:12`). +- `scripts/smoke.ps1:46`/`scripts/smoke.ps1:51` call admin endpoints using `X-Admin-Key` and contain a `TODO` (`scripts/smoke.ps1:48`), so smoke automation is not trustworthy. +- Impact: incident response and deployment validation are unreliable. + +3. High-change, high-complexity frontend hotspots (High) +- Git churn: `wwwroot/app.js` (76 changes), `wwwroot/js/ui.js` (55), `wwwroot/js/i18n.js` (50). +- `wwwroot/js/ui.js` is 1123 lines and owns rendering, validation, modal orchestration, admin flows, and vote logic. +- Hidden module coupling through globals: `wwwroot/js/data.js:131`-`wwwroot/js/data.js:134`, plus `window` callbacks consumed in `wwwroot/js/ui.js:473`, `wwwroot/js/ui.js:696`, `wwwroot/js/ui.js:1009`. +- Impact: every UI change risks regressions outside its feature area. + +4. Rule duplication and divergence across backend/frontend (High) +- Suggestion validation duplicated in create and update (`Endpoints/SuggestEndpoints.cs:45`, `Endpoints/SuggestEndpoints.cs:152`) plus separate JS copy (`wwwroot/js/ui.js:648`, `wwwroot/js/ui.js:1019`). +- Auth length constraints duplicated backend (`Endpoints/AuthEndpoints.cs:19`, `Endpoints/AuthEndpoints.cs:25`) and frontend (`wwwroot/app.js:92`, `wwwroot/app.js:121`). +- Impact: inconsistent behavior, repeated bug fixes, and shotgun surgery. + +5. Tooling guardrails are weak and build hygiene is unstable (Medium) +- `dotnet build GameList.sln` currently reports MSB3026 copy warnings involving recursive test output paths under `GameList.Tests`. +- No CI workflow found (`.github/workflows` absent), no root `README.md`, and no analyzer config files detected (`.editorconfig`, `Directory.Build.props`, `global.json` absent). +- Impact: regressions and process drift are caught late and inconsistently. + +Where to start (recommended sequence): +- Start with P0 tasks that reduce surprise and operational risk: phase-read side effects, auth contract drift, build hygiene, and validation centralization. +- Apply these guiding principles: +- Keep reads pure and writes explicit. +- Consolidate business rules in one place per rule. +- Reduce module fan-in/fan-out before adding features. +- Add small guardrails (build warnings, CI gates) before larger refactors. + +Assumptions and validation: +- Assumption A1: local git history reflects meaningful churn/risk. Validate by comparing with remote `main`/PR stats if different branch practices exist. +- Assumption A2: build warnings are reproducible environment signals, not one-off file locks. Validate by running clean build in CI and on a fresh clone. + +## B) Maintainability map + +Major modules/components and responsibilities: +- `Program.cs`: app bootstrap, infrastructure wiring, middleware order, DB migration on startup, route registration. +- `Endpoints/*.cs`: HTTP endpoint definitions and most business logic (auth, phase, suggestions, votes, results, admin). +- `Endpoints/EndpointHelpers.cs`: cross-cutting helper hub (auth lookup, phase alignment, URL checks, image probing, link graph utilities). +- `Infrastructure/*.cs`: filters/middleware/auth helpers (`AdminOnlyFilter`, phase filters, cookie identity helpers, global exception wrapper). +- `Data/AppDbContext.cs`: EF Core model and persistence mappings. +- `Domain/*.cs`: entity data structures (`Player`, `Suggestion`, `Vote`, `AppState`, `Phase`). +- `wwwroot/app.js` + `wwwroot/js/*.js`: frontend orchestration, shared state, API calls, rendering, i18n, effects. +- `GameList.Tests/*.cs`: integration-heavy endpoint tests plus helper/unit tests. +- `scripts/*.ps1`: deployment and smoke automation. + +Boundary quality: +- Backend boundaries are leaky: endpoint layer owns domain rules, persistence orchestration, and security checks directly. +- `Infrastructure` depends on `Endpoints` (`Infrastructure/PhaseRequirementFilter.cs:3`) while `Endpoints` depend back on `Infrastructure` (`Endpoints/EndpointHelpers.cs:22`), creating conceptual circular ownership. +- Frontend boundaries are leaky: `ui.js` mixes rendering, form validation, and server mutation calls; shared mutable state is directly mutated across modules. + +Worst coupling points: +- `Endpoints/EndpointHelpers.cs` (84 call sites): de facto god module. +- `wwwroot/js/ui.js` + global `state` object (131 direct state references across JS modules). +- Suggestion and phase workflows span endpoint functions, helper functions, filters, and duplicated client-side checks. +- Operational behavior is split across `API.md`, `scripts/smoke.ps1`, and runtime filters with contradictory contracts. + +## C) Critical task list + +[P0] Make phase reads side-effect free and move reconciliation to explicit writes +- Problem: Severity `Critical`, Category `Architecture`. `GetPhase` mutates and persists state during read operations, which causes hidden writes and duplicated logic in endpoints/filters. Impact: unsafe refactors and non-deterministic behavior. +- Evidence: `Endpoints/EndpointHelpers.cs:37`, `Endpoints/EndpointHelpers.cs:69`, `Endpoints/StateEndpoints.cs:19`, `Infrastructure/PhaseRequirementFilter.cs:17`, `Endpoints/ResultsEndpoints.cs:26`. +- Recommendation: Split into `GetCurrentPhaseAsync` (pure read) and explicit `ReconcilePhaseAsync` (write command). Run reconciliation only on intentional transition points (admin toggle, phase change commands, migration job), not on GET paths. +- Acceptance criteria (testable): GET `/api/state` and GET `/api/me` never call `SaveChangesAsync`; integration tests verify no phase mutations occur during read-only requests; filters perform one phase check path without side effects. +- Effort / Risk: `M / Med`. +- Dependencies (if any): none. + +[P0] Repair admin auth contract drift across code, docs, and smoke automation +- Problem: Severity `High`, Category `Documentation/Tooling`. Documentation and smoke script specify header-based admin auth, runtime requires authenticated admin cookie. Impact: smoke checks and runbooks are misleading during incidents/releases. +- Evidence: `API.md:3`, `scripts/smoke.ps1:46`, `scripts/smoke.ps1:48`, `scripts/smoke.ps1:51`, `Infrastructure/AdminOnlyFilter.cs:12`. +- Recommendation: Make one contract authoritative (cookie-based admin role), update docs and scripts to follow it, and add one integration smoke test path that validates real auth flow. +- Acceptance criteria (testable): `scripts/smoke.ps1` passes against local app without manual edits; `API.md` no longer references `X-Admin-Key` unless implemented; one automated test verifies admin route rejection/acceptance behavior. +- Effort / Risk: `S / Low`. +- Dependencies (if any): none. + +[P0] Eliminate build instability and warning noise from project layout/content rules +- Problem: Severity `High`, Category `Tooling`. Current build emits MSB3026 warnings and recursive copy paths under test output, reducing trust in build outputs and masking real issues. +- Evidence: `dotnet build GameList.sln` warning output referencing `GameList.Tests\bin\Debug\net10.0\GameList.Tests\bin\Debug\...`; current exclusions only in `GameList.csproj:17`-`GameList.csproj:21`. +- Recommendation: Ensure test assets are fully excluded from web content pipeline (or move tests outside web project root), then enforce clean build (`0 warnings`) in CI. +- Acceptance criteria (testable): `dotnet build GameList.sln` emits zero warnings; publish output contains only expected runtime artifacts; CI fails on warning regressions. +- Effort / Risk: `M / Med`. +- Dependencies (if any): none. + +[P0] Centralize validation rules to stop backend/frontend drift +- Problem: Severity `High`, Category `Complexity/Duplication`. Validation rules are duplicated in multiple backend endpoints and frontend forms. Impact: inconsistent behavior and repeated fixes. +- Evidence: `Endpoints/SuggestEndpoints.cs:45`, `Endpoints/SuggestEndpoints.cs:152`, `Endpoints/SuggestEndpoints.cs:298`, `wwwroot/js/ui.js:648`, `wwwroot/js/ui.js:1019`, `Endpoints/AuthEndpoints.cs:19`, `wwwroot/app.js:92`. +- Recommendation: Extract backend validators (e.g., `SuggestionValidator`, `AuthValidator`) and reuse in create/update paths; simplify frontend to UX-only prechecks and rely on server responses for source-of-truth. +- Acceptance criteria (testable): create/update share one backend validator path; tests cover validator once and both endpoints; frontend no longer re-implements server-only security rules. +- Effort / Risk: `M / Med`. +- Dependencies (if any): none. + +[P0] Harden request safety defaults (forwarded headers and redirect handling) +- Problem: Severity `High`, Category `Security`. Forwarded headers are trusted without explicit proxy/network allowlist, and image validation likely follows redirects despite "no redirects" policy. +- Evidence: `Program.cs:69`, `Endpoints/EndpointHelpers.cs:108`, `Endpoints/EndpointHelpers.cs:113`, `Endpoints/EndpointHelpers.cs:133`, `Endpoints/SuggestEndpoints.cs:57`. +- Recommendation: Configure known proxies/networks for forwarded headers; enforce `AllowAutoRedirect = false` in image validation client and add tests for redirect-chain and private-host edge cases. +- Acceptance criteria (testable): integration tests prove redirected URLs are rejected; forwarded header spoofing test fails when source is untrusted; documentation updated with trusted proxy requirements. +- Effort / Risk: `M / Med`. +- Dependencies (if any): none. + +[P1] Extract service-layer workflows from endpoint lambdas +- Problem: Severity `High`, Category `Architecture`. Endpoint files contain business orchestration, persistence, and policy logic inline; large lambdas are hard to reason about and reuse. +- Evidence: `Endpoints/SuggestEndpoints.cs:43`, `Endpoints/AdminEndpoints.cs:105`, `Endpoints/VoteEndpoints.cs:35`, `Endpoints/ResultsEndpoints.cs:30`. +- Recommendation: Introduce focused application services (`SuggestionService`, `VoteService`, `AdminWorkflowService`) and keep endpoints as transport adapters. +- Acceptance criteria (testable): endpoint handlers reduced to routing + DTO mapping + service calls; domain rule tests target service methods directly; endpoint tests remain green. +- Effort / Risk: `L / Med`. +- Dependencies (if any): P0 phase-read cleanup recommended first. + +[P1] Decompose frontend UI monolith and remove `window` cross-module hooks +- Problem: Severity `High`, Category `Architecture/Complexity`. UI logic is concentrated in `ui.js`; global `window` callbacks hide dependencies and increase accidental complexity. +- Evidence: `wwwroot/js/ui.js:390`, `wwwroot/js/data.js:131`, `wwwroot/js/data.js:134`, `wwwroot/js/ui.js:473`, `wwwroot/js/ui.js:1009`. +- Recommendation: Split UI by feature (`suggestions-ui`, `votes-ui`, `admin-ui`, `modals-ui`) and use explicit imports/events instead of `window` globals. +- Acceptance criteria (testable): no feature code depends on `window.refreshPhaseData`/`window.loadVoteData`; module boundaries documented; smoke behavior unchanged. +- Effort / Risk: `L / Med`. +- Dependencies (if any): none. + +[P1] Replace uncontrolled polling with serialized refresh scheduling +- Problem: Severity `Medium`, Category `Reliability/Complexity`. A fixed 4-second interval can overlap async refreshes and race state updates. +- Evidence: `wwwroot/app.js:293`-`wwwroot/app.js:297`, `wwwroot/js/data.js:82`. +- Recommendation: Add a scheduler that avoids overlapping refreshes (single-flight), pauses when tab hidden, and supports explicit event-triggered refresh. +- Acceptance criteria (testable): at most one in-flight refresh at any time; no duplicate toasts/state flicker during slow network simulation; tests for scheduler behavior. +- Effort / Risk: `M / Low`. +- Dependencies (if any): none. + +[P1] Remove legacy/dead paths to reduce cognitive load +- Problem: Severity `Medium`, Category `Other`. Legacy `Reveal` phase and dead UI hooks remain in active code, increasing confusion. +- Evidence: `Domain/Phase.cs:6`, `Endpoints/StateEndpoints.cs:107`, `wwwroot/js/data.js:30`, `wwwroot/js/ui.js:156`, `wwwroot/js/ui.js:1191`. +- Recommendation: Remove obsolete phase enum/value handling and dead UI references (`all-suggestions`, `nav-vote-next`) or reintroduce missing elements intentionally. +- Acceptance criteria (testable): no references to removed phase/UI ids remain; tests validate expected phase transitions only (`Suggest`, `Vote`, `Results`). +- Effort / Risk: `S / Low`. +- Dependencies (if any): P0 phase cleanup. + +[P1] Make write workflows transaction-consistent and explicit +- Problem: Severity `Medium`, Category `Correctness/Architecture`. Several multi-step state changes rely on multiple DB commands without explicit transaction grouping. +- Evidence: `Endpoints/SuggestEndpoints.cs:103`-`Endpoints/SuggestEndpoints.cs:109`, `Endpoints/AdminEndpoints.cs:16`-`Endpoints/AdminEndpoints.cs:31`, `Endpoints/AdminEndpoints.cs:220`-`Endpoints/AdminEndpoints.cs:229`. +- Recommendation: Wrap multi-entity updates in explicit transactions where consistency matters, or refactor into idempotent command handlers with compensating behavior. +- Acceptance criteria (testable): fault-injection tests prove no partial state after exceptions; transaction boundaries documented per workflow. +- Effort / Risk: `M / Med`. +- Dependencies (if any): service-layer extraction helpful but not required. + +[P1] Strengthen test quality for flaky/time-sensitive cases and security edges +- Problem: Severity `Medium`, Category `Testing`. Some tests depend on sleeps and do not cover realistic redirect behavior or overlapping refresh flows. +- Evidence: `GameList.Tests/SuggestionTests.cs:379`, `GameList.Tests/SuggestionTests.cs:575`, `GameList.Tests/HelperTests.cs:95`. +- Recommendation: replace `Task.Delay` ordering checks with deterministic seeded timestamps where feasible; add explicit redirect-follow tests and concurrency-path tests. +- Acceptance criteria (testable): no timing sleeps in endpoint tests for ordering; new tests cover redirect-chain rejection and race-sensitive refresh logic. +- Effort / Risk: `M / Low`. +- Dependencies (if any): P0 redirect-hardening task. + +[P2] Externalize i18n/FAQ content from executable JS modules +- Problem: Severity `Low`, Category `Complexity/Documentation`. Translation and FAQ payloads are embedded in code, making review and localization hard. +- Evidence: `wwwroot/js/i18n.js:1`-`wwwroot/js/i18n.js:799`. +- Recommendation: move translation dictionaries and FAQ markdown into versioned JSON/MD assets, load via data module, keep code focused on i18n mechanics. +- Acceptance criteria (testable): `i18n.js` contains behavior only; language assets are schema-validated; app renders identical strings. +- Effort / Risk: `M / Low`. +- Dependencies (if any): frontend module split helps. + +[P2] Improve repository-level engineering docs and ownership map +- Problem: Severity `Low`, Category `Documentation`. There is no root README/architecture map/runbook tying module ownership, local setup, and deployment flow together. +- Evidence: root README absent; operational docs fragmented across `API.md`, `SPEC.md`, `IIS.md`, `scripts/deploy-ftp.ps1`. +- Recommendation: add concise `README.md` with architecture diagram, local run/test commands, operational boundaries, and links to deeper docs. +- Acceptance criteria (testable): new contributors can run app + tests using README only; docs align with live auth/deploy behavior. +- Effort / Risk: `S / Low`. +- Dependencies (if any): P0 auth contract and build hygiene fixes. + +## D) Quick wins vs strategic refactors + +Quick wins (hours to 1 day each): +- Remove stale admin key wording in `API.md` and align with actual auth behavior. +- Fix `scripts/smoke.ps1` to authenticate via register/login and remove duplicate factory reset call. +- Add `Content Remove="GameList.Tests\**\*"` (or equivalent) and verify clean build output. +- Add a build check script that fails on warnings and run it locally/CI. +- Delete dead `DeletePlayerRequest` in `Contracts/Dtos.cs:23` if unused. +- Remove unused endpoint handler parameters (`IConfiguration _`) in `Endpoints/StateEndpoints.cs:55`, `Endpoints/StateEndpoints.cs:79`. +- Remove dead UI references (`all-suggestions`, `nav-vote-next`) or add explicit TODO with owner/date. +- Replace `Task.Delay` test ordering hacks with deterministic setup in affected tests. +- Add a short "module ownership" section to `README.md` once created. + +Strategic refactors (multi-day/week), staged: + +1) Phase and policy workflow refactor +- Stage 1: introduce pure phase reader API and separate reconciliation command. +- Stage 2: migrate filters/endpoints to pure reads; remove write side effects from GET. +- Stage 3: enforce via tests that read endpoints do not mutate persistence. + +2) Backend application service extraction +- Stage 1: extract suggestion create/update/delete logic to one service. +- Stage 2: extract vote/link/unlink workflows with explicit transaction semantics. +- Stage 3: standardize endpoint responses with typed DTOs/ProblemDetails. + +3) Frontend architecture split +- Stage 1: split `ui.js` into modal/forms/vote/admin render modules without behavior changes. +- Stage 2: remove `window.*` bridges and introduce explicit orchestration module. +- Stage 3: add lightweight frontend tests for state transitions and API error handling. + +4) Engineering guardrail rollout +- Stage 1: introduce analyzer/lint config and formatting scripts. +- Stage 2: add CI pipeline gates (build, test, coverage, vulnerability check). +- Stage 3: enable warning budgets and file-size complexity thresholds for hotspot files. + +## E) Suggested guardrails + +Recommended tooling and standards: +- C# static analysis: enable .NET analyzers (`AnalysisLevel latest`), nullable warnings strict, and treat warnings as errors in CI. +- JS quality: add ESLint + Prettier for `wwwroot/**/*.js` and fail CI on lint errors. +- Contract safety: standardize error responses using RFC7807 `ProblemDetails` and validate API DTO schema. +- CI gates: run `dotnet build`, `dotnet test GameList.Tests/GameList.Tests.csproj`, coverage collection, and `dotnet list ... --vulnerable`. +- Churn guardrails: add a simple check warning when files exceed thresholds (e.g., >500 LOC for JS UI modules, >250 LOC endpoint files). +- Documentation gate: PR checklist requires updates to `API.md`/runbooks when auth/endpoints/deploy scripts change. +- Security defaults: require known proxy config for forwarded headers and explicit no-redirect HTTP client behavior for external URL validation. + +Pragmatic coding standards to prevent backsliding: +- Keep endpoint handlers transport-focused; place business rules in services/validators. +- Keep reads side-effect free; state changes only in explicit commands. +- Avoid duplicate rule definitions across server/client; one source of truth per rule. +- Prefer typed DTOs over anonymous response shapes for non-trivial payloads.