Prune REVIEW to active backlog only
This commit is contained in:
273
REVIEW.md
273
REVIEW.md
@@ -1,241 +1,94 @@
|
||||
# Maintainability Review - Pick'n'Play
|
||||
|
||||
## A) Executive summary
|
||||
## A) Current focus
|
||||
|
||||
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.
|
||||
This document tracks only active work. Completed work is intentionally omitted and can be reviewed in git history.
|
||||
|
||||
Progress update (as of February 7, 2026):
|
||||
- Completed: phase reads are side-effect free with explicit reconciliation on write routes (`Endpoints/EndpointHelpers.cs:37`, `Endpoints/EndpointHelpers.cs:61`, `Endpoints/StateEndpoints.cs:62`).
|
||||
- Completed: admin auth docs aligned to account-based admin sessions (`API.md:3`).
|
||||
- Completed: build/test guardrails added (`.github/workflows/ci.yml`) and root ownership/setup docs added (`README.md:1`).
|
||||
- Completed: backend validators centralized for suggestions and auth (`Endpoints/SuggestionValidator.cs:7`, `Endpoints/AuthValidator.cs:11`).
|
||||
- Completed: request safety hardened for redirects and forwarded headers (`Program.cs:44`, `Program.cs:108`, `Endpoints/EndpointHelpers.cs:105`, `GameList.Tests/HelperTests.cs:121`, `GameList.Tests/HelperTests.cs:219`).
|
||||
- Completed: API/response contract standardization now includes typed auth/state/workflow DTOs and shared `ProblemDetails` envelopes across endpoint/filter paths (`Contracts/Responses.cs:58`, `Endpoints/AuthEndpoints.cs:54`, `Endpoints/StateEndpoints.cs:22`, `Infrastructure/AdminOnlyFilter.cs:15`, `Endpoints/EndpointHelpers.cs:90`, `wwwroot/js/api.js:25`, `GameList.Tests/AuthTests.cs:164`).
|
||||
|
||||
Top 5 maintainability risks (priority order):
|
||||
Active maintainability risks (priority order):
|
||||
|
||||
1. Frontend module concentration and global coupling (Critical)
|
||||
- `wwwroot/js/ui.js` is still the dominant hotspot 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: hard-to-debug regressions and fragile refactors in UI workflows.
|
||||
- `wwwroot/js/ui.js` remains the dominant hotspot and owns rendering, modal flows, admin flows, and vote logic.
|
||||
- Hidden module coupling still exists through global bridges in `wwwroot/js/data.js:131`-`wwwroot/js/data.js:134`, with consumers in `wwwroot/js/ui.js:473`, `wwwroot/js/ui.js:684`, and `wwwroot/js/ui.js:997`.
|
||||
- Impact: high regression surface and expensive refactors.
|
||||
|
||||
2. Rule duplication still present between backend/frontend validations (High)
|
||||
- Suggestion and auth validation are centralized on the backend (`Endpoints/SuggestionValidator.cs:7`, `Endpoints/AuthValidator.cs:11`, `Endpoints/AuthEndpoints.cs:18`, `Endpoints/AuthEndpoints.cs:65`).
|
||||
- Frontend now keeps UX-only prechecks (required credentials and min/max player input affordances) instead of server-side policy duplication (`wwwroot/app.js:92`, `wwwroot/app.js:119`, `wwwroot/js/ui.js:645`).
|
||||
- Impact: lower drift risk, though UI-side range hints still require occasional sync with server limits.
|
||||
2. Frontend refresh scheduling can overlap async work (High)
|
||||
- Refresh loop is still interval-driven in `wwwroot/app.js:290` with async work rooted in `wwwroot/js/data.js:82`.
|
||||
- Impact: overlapping requests can race state updates and produce noisy UI transitions.
|
||||
|
||||
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.
|
||||
3. 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.
|
||||
|
||||
4. Endpoint contract consistency and error shaping are still uneven (High)
|
||||
- Service-layer extraction is in place for suggestions, votes, admin, and results (`Endpoints/SuggestionWorkflowService.cs:8`, `Endpoints/VoteWorkflowService.cs:8`, `Endpoints/AdminWorkflowService.cs:8`, `Endpoints/ResultsWorkflowService.cs:8`), and typed response DTOs now cover auth/state/workflow payloads (`Contracts/Responses.cs:5`, `Contracts/Responses.cs:58`, `Endpoints/AuthEndpoints.cs:54`, `Endpoints/StateEndpoints.cs:22`).
|
||||
- Endpoint/filter unauthorized paths now use uniform `ProblemDetails` envelopes (`Infrastructure/AdminOnlyFilter.cs:15`, `Infrastructure/PhaseRequirementFilter.cs:15`, `Infrastructure/PhaseOrJokerFilter.cs:19`, `Endpoints/SuggestEndpoints.cs:18`, `Endpoints/VoteEndpoints.cs:18`, `Endpoints/ResultsEndpoints.cs:19`).
|
||||
- Impact: API evolution risk is reduced; remaining inconsistency is framework-generated 401 challenge bodies for fully unauthenticated requests.
|
||||
4. 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.
|
||||
|
||||
5. Static-analysis and frontend lint guardrails remain incomplete (Medium)
|
||||
- Build/test CI exists (`.github/workflows/ci.yml`) and project content rules are fixed (`GameList.csproj:17`-`GameList.csproj:21`), but analyzers/lint/format gates are still absent.
|
||||
- Impact: regressions and style drift can still slip through.
|
||||
5. 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.
|
||||
|
||||
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 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 documented but still spread across multiple files (`API.md`, `IIS.md`, `README.md`) without a single versioned runbook artifact.
|
||||
|
||||
## C) Critical task list
|
||||
|
||||
[P0][Done] Make phase reads side-effect free and move reconciliation to explicit writes
|
||||
- Problem: Severity `Critical`, Category `Architecture`. Read endpoints/filters previously relied on mutating phase reads. Impact: unsafe refactors and non-deterministic behavior.
|
||||
- Evidence: `Endpoints/EndpointHelpers.cs:37`, `Endpoints/EndpointHelpers.cs:61`, `Endpoints/StateEndpoints.cs:20`, `Infrastructure/PhaseRequirementFilter.cs:17`, `Endpoints/ResultsWorkflowService.cs:10`, `GameList.Tests/StateTests.cs:236`, `GameList.Tests/FiltersTests.cs:55`.
|
||||
- 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][Done] 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: runbooks are misleading during incidents/releases.
|
||||
- Evidence: `API.md:3`, `Infrastructure/AdminOnlyFilter.cs:12`.
|
||||
- Recommendation: Make one contract authoritative (account-based admin role), update docs to follow it, and add one integration smoke test path that validates real auth flow.
|
||||
- Acceptance criteria (testable): `API.md` no longer references `X-Admin-Key`; one automated test verifies admin route rejection/acceptance behavior.
|
||||
- Effort / Risk: `S / Low`.
|
||||
- Dependencies (if any): none.
|
||||
|
||||
[P0][Done] 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: content exclusions now include `Content` in `GameList.csproj:17`-`GameList.csproj:21`; CI gate added in `.github/workflows/ci.yml`.
|
||||
- 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][Done] Centralize validation rules to stop backend/frontend drift
|
||||
- Problem: Severity `High`, Category `Complexity/Duplication`. Validation rules were duplicated in multiple backend endpoints and frontend forms, increasing drift risk.
|
||||
- Evidence: backend validators are centralized (`Endpoints/SuggestionValidator.cs:7`, `Endpoints/AuthValidator.cs:11`, `Endpoints/SuggestionWorkflowService.cs:39`, `Endpoints/SuggestionWorkflowService.cs:115`, `Endpoints/AuthEndpoints.cs:18`, `Endpoints/AuthEndpoints.cs:65`); frontend server-policy duplicates removed for auth length and image host/url security checks (`wwwroot/app.js:92`, `wwwroot/app.js:119`, `wwwroot/js/ui.js:645`).
|
||||
- Recommendation: Keep backend validators as the source of truth; limit frontend checks to UX hints and required-field guidance.
|
||||
- Acceptance criteria (testable): create/update share one backend validator path; tests cover validator-backed routes; frontend no longer re-implements server-only security rules.
|
||||
- Effort / Risk: `M / Med`.
|
||||
- Dependencies (if any): none.
|
||||
|
||||
[P0][Done] 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:44`, `Program.cs:74`, `Program.cs:108`, `Endpoints/EndpointHelpers.cs:105`, `GameList.Tests/HelperTests.cs:121`, `GameList.Tests/HelperTests.cs:219`, `IIS.md:17`.
|
||||
- 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][Done] 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: extraction completed for suggestions, votes, admin, and results (`Endpoints/SuggestionWorkflowService.cs:8`, `Endpoints/VoteWorkflowService.cs:8`, `Endpoints/AdminWorkflowService.cs:8`, `Endpoints/ResultsWorkflowService.cs:8`, `Program.cs:37`, `Program.cs:38`, `Program.cs:39`, `Program.cs:40`).
|
||||
- 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.
|
||||
## B) Active task list
|
||||
|
||||
[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.
|
||||
- Problem: Severity `High`, Category `Architecture/Complexity`. `ui.js` still mixes rendering, form behavior, and mutation flows.
|
||||
- Evidence: `wwwroot/js/ui.js:390`, `wwwroot/js/ui.js:684`, `wwwroot/js/ui.js:997`, `wwwroot/js/data.js:131`, `wwwroot/js/data.js:134`.
|
||||
- Recommendation: split by feature (`suggestions-ui`, `votes-ui`, `admin-ui`, `modals-ui`) and replace `window.*` callbacks with explicit imports/events.
|
||||
- Acceptance criteria (testable): no feature path depends on `window.refreshPhaseData`, `window.loadVoteData`, or `window.loadSuggestData`.
|
||||
- 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.
|
||||
- Problem: Severity `Medium`, Category `Reliability/Complexity`. Fixed 4-second polling can overlap when requests take longer than interval.
|
||||
- Evidence: `wwwroot/app.js:290`, `wwwroot/js/data.js:82`.
|
||||
- Recommendation: introduce a single-flight scheduler with backpressure, visibility pause/resume, and explicit trigger support.
|
||||
- Acceptance criteria (testable): at most one in-flight refresh at a time; no duplicate refresh overlap during induced latency.
|
||||
- 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:99`, `wwwroot/js/data.js:30`, `wwwroot/js/ui.js:156`, `wwwroot/js/ui.js:1149`.
|
||||
- Recommendation: Remove obsolete phase enum/value handling and dead UI references (`all-suggestions`, `nav-vote-next`).
|
||||
- Acceptance criteria (testable): no references to removed phase/UI ids remain; tests validate expected phase transitions only (`Suggest`, `Vote`, `Results`).
|
||||
[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): P0 phase cleanup.
|
||||
- Dependencies (if any): none.
|
||||
|
||||
[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/SuggestionWorkflowService.cs:72`, `Endpoints/SuggestionWorkflowService.cs:77`, `Endpoints/AdminWorkflowService.cs:17`, `Endpoints/AdminWorkflowService.cs:181`, `Endpoints/AdminWorkflowService.cs:198`.
|
||||
- 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: redirect and forwarded-header cases are covered (`GameList.Tests/HelperTests.cs:121`, `GameList.Tests/HelperTests.cs:219`), and delay-based ordering checks were removed from `GameList.Tests/SuggestionTests.cs` and `GameList.Tests/AdminTests.cs`.
|
||||
- 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.
|
||||
|
||||
[P1][Done] Standardize API response contracts and error envelopes
|
||||
- Problem: Severity `Medium`, Category `API/Contracts`. Success payloads and error envelopes were inconsistent across endpoint, service, and filter layers.
|
||||
- Evidence: typed response DTOs now cover workflow + auth + state paths (`Contracts/Responses.cs:5`, `Contracts/Responses.cs:58`, `Endpoints/AuthEndpoints.cs:54`, `Endpoints/StateEndpoints.cs:22`, `Endpoints/SuggestionWorkflowService.cs:83`, `Endpoints/VoteWorkflowService.cs:83`, `Endpoints/AdminWorkflowService.cs:44`, `Endpoints/ResultsWorkflowService.cs:63`); shared `ProblemDetails` helper is used across services and endpoint filters (`Endpoints/EndpointHelpers.cs:90`, `Endpoints/SuggestEndpoints.cs:18`, `Endpoints/VoteEndpoints.cs:18`, `Infrastructure/AdminOnlyFilter.cs:15`, `Infrastructure/PhaseRequirementFilter.cs:15`, `Infrastructure/PhaseOrJokerFilter.cs:19`); global exception handling and client parsing remain compatible via `error`/`detail`/`title` (`Infrastructure/PlayerIdentityExtensions.cs:48`, `wwwroot/js/api.js:25`, `GameList.Tests/AuthTests.cs:164`).
|
||||
- Recommendation: retain this contract as baseline and keep backward-compatible `error` extension in future error-shape changes.
|
||||
- Acceptance criteria (testable): endpoint/service/filter responses use typed success payloads and standardized `ProblemDetails` error envelopes, with current frontend parsing unchanged.
|
||||
[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] 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.
|
||||
[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 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.
|
||||
- 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 helps.
|
||||
- Dependencies (if any): frontend module split is helpful but optional.
|
||||
|
||||
[P2][Done] Improve repository-level engineering docs and ownership map
|
||||
- Problem: Severity `Low`, Category `Documentation`. Contributor setup and ownership context needed consolidation.
|
||||
- Evidence: root `README.md` now provides setup/ownership map and links to `API.md`, `SPEC.md`, and `IIS.md`.
|
||||
- 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.
|
||||
## C) Suggested execution order
|
||||
|
||||
## D) Quick wins vs strategic refactors
|
||||
1. Decompose `ui.js` and remove `window` hooks.
|
||||
2. Introduce serialized refresh scheduler.
|
||||
3. Remove `Reveal` phase compatibility branches.
|
||||
4. Normalize/declare unauthenticated 401 contract behavior.
|
||||
5. Add analyzers + JS lint gates in CI.
|
||||
6. Externalize i18n/FAQ assets.
|
||||
|
||||
Quick wins (hours to 1 day each):
|
||||
- Completed: removed stale admin key wording in `API.md` and aligned with actual auth behavior.
|
||||
- Completed: added `Content Remove="GameList.Tests\**\*"` and verified clean build output.
|
||||
- Completed: added CI build/test gate for warning regressions.
|
||||
- Completed: removed dead `DeletePlayerRequest` from `Contracts/Dtos.cs`.
|
||||
- Completed: removed unused endpoint handler parameters in `Endpoints/StateEndpoints.cs`.
|
||||
- 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.
|
||||
- Completed: added module ownership section in `README.md`.
|
||||
## D) Guardrails
|
||||
|
||||
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.
|
||||
- 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.
|
||||
|
||||
Reference in New Issue
Block a user