xTr1m/GameList
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Introduce typed API responses and align workflow outputs

Browse Source
This commit is contained in:
Frank Tovar
2026-02-07 01:19:51 +01:00
parent 35d842d6ee
commit 79dc8f899f
7 changed files with 99 additions and 77 deletions
Download Patch File Download Diff File Expand all files Collapse all files

25
REVIEW.md
View File

@@ -10,6 +10,7 @@ Progress update (as of February 7, 2026):
- 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`).
- In progress: API/response contract standardization started with typed response DTOs and client parsing compatibility (`Contracts/Responses.cs:5`, `Contracts/Responses.cs:35`, `wwwroot/js/api.js:25`).
Top 5 maintainability risks (priority order):
@@ -19,7 +20,7 @@ Top 5 maintainability risks (priority order):
- Impact: hard-to-debug regressions and fragile refactors in UI workflows.
2. Rule duplication still present between backend/frontend validations (High)
- Suggestion validation is centralized on the backend (`Endpoints/SuggestionWorkflowService.cs:39`, `Endpoints/SuggestionWorkflowService.cs:109`, `Endpoints/SuggestionValidator.cs:7`) but frontend still duplicates parts (`wwwroot/js/ui.js:648`, `wwwroot/js/ui.js:1019`).
- Suggestion validation is centralized on the backend (`Endpoints/SuggestionWorkflowService.cs:39`, `Endpoints/SuggestionWorkflowService.cs:115`, `Endpoints/SuggestionValidator.cs:7`) but frontend still duplicates parts (`wwwroot/js/ui.js:648`, `wwwroot/js/ui.js:1019`).
- Auth validation is centralized on the backend (`Endpoints/AuthEndpoints.cs:18`, `Endpoints/AuthEndpoints.cs:65`, `Endpoints/AuthValidator.cs:11`) while frontend length checks remain duplicated (`wwwroot/app.js:92`, `wwwroot/app.js:121`).
- Impact: inconsistent behavior and repeated fixes across server/client.
@@ -29,8 +30,8 @@ Top 5 maintainability risks (priority order):
- 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. Endpoint contract consistency and response shaping are still uneven (High)
- Service-layer extraction is now in place for suggestions, votes, admin, and results (`Endpoints/SuggestionWorkflowService.cs:8`, `Endpoints/VoteWorkflowService.cs:8`, `Endpoints/AdminWorkflowService.cs:8`, `Endpoints/ResultsWorkflowService.cs:7`), but response shapes are still mostly anonymous objects and ad-hoc error payloads.
4. Endpoint contract consistency and error shaping are still uneven (High)
- Service-layer extraction is now 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 DTO adoption has started (`Contracts/Responses.cs:5`, `Contracts/Responses.cs:35`), but many endpoints still emit ad-hoc error payloads.
- Impact: API evolution and client compatibility changes are still high-friction.
5. Static-analysis and frontend lint guardrails remain incomplete (Medium)
@@ -77,7 +78,7 @@ Worst coupling points:
[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:9`, `GameList.Tests/StateTests.cs:236`, `GameList.Tests/FiltersTests.cs:55`.
- 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`.
@@ -101,7 +102,7 @@ Worst coupling points:
[P0][Partial] 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: backend centralized in `Endpoints/SuggestionWorkflowService.cs:39`, `Endpoints/SuggestionWorkflowService.cs:109`, `Endpoints/SuggestionValidator.cs:7`, `Endpoints/AuthEndpoints.cs:18`, `Endpoints/AuthEndpoints.cs:65`, `Endpoints/AuthValidator.cs:11`; frontend duplicates remain in `wwwroot/js/ui.js:648`, `wwwroot/js/ui.js:1019`, `wwwroot/app.js:92`.
- Evidence: backend centralized in `Endpoints/SuggestionWorkflowService.cs:39`, `Endpoints/SuggestionWorkflowService.cs:115`, `Endpoints/SuggestionValidator.cs:7`, `Endpoints/AuthEndpoints.cs:18`, `Endpoints/AuthEndpoints.cs:65`, `Endpoints/AuthValidator.cs:11`; frontend duplicates remain in `wwwroot/js/ui.js:648`, `wwwroot/js/ui.js:1019`, `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`.
@@ -117,7 +118,7 @@ Worst coupling points:
[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:7`, `Program.cs:37`, `Program.cs:38`, `Program.cs:39`, `Program.cs:40`).
- 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`.
@@ -149,7 +150,7 @@ Worst coupling points:
[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:71`, `Endpoints/SuggestionWorkflowService.cs:75`, `Endpoints/AdminWorkflowService.cs:74`, `Endpoints/AdminWorkflowService.cs:208`, `Endpoints/AdminWorkflowService.cs:227`.
- 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`.
@@ -157,12 +158,20 @@ Worst coupling points:
[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:121`.
- 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][In Progress] Standardize API response contracts and error envelopes
- Problem: Severity `Medium`, Category `API/Contracts`. Success payloads are being typed, but error payloads are still inconsistent (`{ error }` objects, plain status codes, and mixed shapes).
- Evidence: typed response DTOs added in `Contracts/Responses.cs:5` and used in service workflows (`Endpoints/SuggestionWorkflowService.cs:83`, `Endpoints/VoteWorkflowService.cs:83`, `Endpoints/AdminWorkflowService.cs:44`, `Endpoints/ResultsWorkflowService.cs:63`); frontend parser now supports `error`, `detail`, and `title` in `wwwroot/js/api.js:25`.
- Recommendation: migrate endpoint error responses to `ProblemDetails` consistently while keeping backward-compatible `error` fields during transition; continue replacing anonymous success payloads with DTOs.
- Acceptance criteria (testable): all API endpoints return typed success DTOs; all non-2xx responses include `ProblemDetails` fields and remain consumable by current frontend.
- 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.
- Evidence: `wwwroot/js/i18n.js:1`-`wwwroot/js/i18n.js:799`.