GameList/REVIEW.md

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.