Files
GameList/REVIEW.md

5.4 KiB

Maintainability Review - Pick'n'Play

A) Current focus

This document tracks only active work. Completed work is intentionally omitted and can be reviewed in git history.

Active maintainability risks (priority order):

  1. Frontend module concentration and global coupling (Critical)
  • wwwroot/js/ui.js remains the dominant hotspot and owns rendering, modal flows, admin flows, and vote logic.
  • Cross-feature coupling still exists via shared mutable state usage across UI/data modules (wwwroot/js/ui.js:180, wwwroot/js/ui.js:401, wwwroot/js/ui.js:622, wwwroot/js/data.js:82).
  • Impact: high regression surface and expensive refactors even after removing global window bridges.
  1. Legacy phase path remains in active code (Medium)
  • Reveal is still present in Domain/Phase.cs:6 and compatibility branches remain in Endpoints/StateEndpoints.cs:102 and Endpoints/StateEndpoints.cs:111.
  • Impact: extra cognitive load and more branches to reason about during phase-flow changes.
  1. Unauthenticated 401 response shape is still framework-driven (Medium)
  • Endpoint and filter unauthorized responses are standardized when app logic executes (Infrastructure/AdminOnlyFilter.cs:15, Infrastructure/PhaseRequirementFilter.cs:15, Endpoints/SuggestEndpoints.cs:18), but anonymous challenge responses remain middleware-controlled (GameList.Tests/StateTests.cs:214).
  • Impact: clients must tolerate both app-produced problem payloads and framework challenge responses.
  1. Static analysis and frontend lint guardrails are still missing (Medium)
  • CI currently gates restore/build/test only (.github/workflows/ci.yml:23-.github/workflows/ci.yml:29).
  • Impact: style drift and low-signal warnings can enter the codebase undetected.

B) Active task list

[P1] Decompose frontend UI monolith by feature

  • Problem: Severity High, Category Architecture/Complexity. ui.js still mixes rendering, form behavior, and mutation flows.
  • Evidence: wwwroot/js/ui.js:180, wwwroot/js/ui.js:401, wwwroot/js/ui.js:622, wwwroot/js/ui.js:903, wwwroot/js/data.js:82.
  • Recommendation: split by feature (suggestions-ui, votes-ui, admin-ui, modals-ui) and keep orchestration in a thin composition layer.
  • Acceptance criteria (testable): UI behavior is preserved while feature modules own isolated responsibilities and no single file coordinates all vote/admin/modal/suggestion interactions.
  • Effort / Risk: L / Med.
  • Dependencies (if any): none.

[P1] Remove legacy Reveal phase compatibility branches

  • Problem: Severity Medium, Category Complexity. Legacy phase compatibility logic is still present in runtime paths.
  • Evidence: Domain/Phase.cs:6, Endpoints/StateEndpoints.cs:102, Endpoints/StateEndpoints.cs:111, wwwroot/js/data.js:30.
  • Recommendation: remove Reveal from enum/transition logic and update affected tests/documentation.
  • Acceptance criteria (testable): no runtime references to Phase.Reveal; phase transitions cover only Suggest, Vote, and Results.
  • Effort / Risk: S / Low.
  • Dependencies (if any): none.

[P1] Unify client handling of unauthenticated 401 challenge responses

  • Problem: Severity Medium, Category API/Contracts. Fully unauthenticated requests can still bypass application-level problem shaping.
  • Evidence: challenge behavior is exercised in GameList.Tests/StateTests.cs:214 while app-level unauthorized shaping is exercised in GameList.Tests/AuthTests.cs:164.
  • Recommendation: either customize cookie-auth challenge response to RFC7807 or explicitly codify dual-shape handling contract in API docs and frontend error layer.
  • Acceptance criteria (testable): single documented 401 contract path, with tests validating response shape and frontend behavior.
  • Effort / Risk: M / Med.
  • Dependencies (if any): none.

[P2] Add static analysis and JS lint/format guardrails

  • Problem: Severity Medium, Category Tooling. CI does not enforce analyzers or JS lint/format checks.
  • Evidence: .github/workflows/ci.yml:23-.github/workflows/ci.yml:29.
  • Recommendation: add .NET analyzer configuration and ESLint/Prettier checks, then enforce in CI.
  • Acceptance criteria (testable): CI fails on analyzer/lint violations; local scripts are documented in root docs.
  • Effort / Risk: M / Low.
  • Dependencies (if any): none.

[P2] Externalize i18n and FAQ content from executable JS

  • Problem: Severity Low, Category Complexity/Documentation. Translation and FAQ payloads are embedded in code.
  • Evidence: wwwroot/js/i18n.js:1-wwwroot/js/i18n.js:799.
  • Recommendation: move language dictionaries and FAQ markdown into versioned data assets with schema checks.
  • Acceptance criteria (testable): i18n.js holds behavior only; assets load and render identical user-facing text.
  • Effort / Risk: M / Low.
  • Dependencies (if any): frontend module split is helpful but optional.

C) Suggested execution order

  1. Decompose ui.js by feature and keep orchestration thin.
  2. Remove Reveal phase compatibility branches.
  3. Normalize/declare unauthenticated 401 contract behavior.
  4. Add analyzers + JS lint gates in CI.
  5. Externalize i18n/FAQ assets.

D) Guardrails

  • 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.