diff --git a/Domain/Phase.cs b/Domain/Phase.cs index d611688..09f7f63 100644 --- a/Domain/Phase.cs +++ b/Domain/Phase.cs @@ -3,7 +3,6 @@ namespace GameList.Domain; public enum Phase { Suggest = 0, - Reveal = 1, Vote = 2, Results = 3 } diff --git a/Endpoints/EndpointHelpers.cs b/Endpoints/EndpointHelpers.cs index 5a7401d..321d14a 100644 --- a/Endpoints/EndpointHelpers.cs +++ b/Endpoints/EndpointHelpers.cs @@ -51,7 +51,7 @@ internal static class EndpointHelpers public static Phase GetCurrentPhase(Phase phase, bool resultsOpen) { - var normalized = phase == Phase.Reveal ? Phase.Vote : phase; + var normalized = NormalizePhase(phase); if (resultsOpen) return Phase.Results; @@ -63,9 +63,10 @@ internal static class EndpointHelpers { var changed = false; - if (player.CurrentPhase == Phase.Reveal) + var normalized = NormalizePhase(player.CurrentPhase); + if (player.CurrentPhase != normalized) { - player.CurrentPhase = Phase.Vote; + player.CurrentPhase = normalized; changed = true; } @@ -84,6 +85,17 @@ internal static class EndpointHelpers return changed; } + private static Phase NormalizePhase(Phase phase) + { + return phase switch + { + Phase.Suggest => Phase.Suggest, + Phase.Vote => Phase.Vote, + Phase.Results => Phase.Results, + _ => Phase.Vote // legacy/invalid phase fallback + }; + } + public static IResult PhaseMismatch(Phase required, Phase current) => BadRequestError($"This endpoint is available in the {required} phase. Your current phase is {current}."); diff --git a/Endpoints/StateEndpoints.cs b/Endpoints/StateEndpoints.cs index 41569b4..48f21cc 100644 --- a/Endpoints/StateEndpoints.cs +++ b/Endpoints/StateEndpoints.cs @@ -99,7 +99,6 @@ public static class StateEndpoints private static Phase NextPhase(Phase current) => current switch { Phase.Suggest => Phase.Vote, - Phase.Reveal => Phase.Vote, // legacy safety Phase.Vote => Phase.Results, _ => Phase.Results }; @@ -108,7 +107,6 @@ public static class StateEndpoints { Phase.Results => Phase.Vote, Phase.Vote => Phase.Suggest, - Phase.Reveal => Phase.Suggest, // legacy safety _ => Phase.Suggest }; } diff --git a/GameList.Tests/StateTests.cs b/GameList.Tests/StateTests.cs index cb3782e..8c60511 100644 --- a/GameList.Tests/StateTests.cs +++ b/GameList.Tests/StateTests.cs @@ -50,7 +50,7 @@ public class StateTests PasswordHash = [1], PasswordSalt = [1], DisplayName = "Legacy", - CurrentPhase = Phase.Reveal, + CurrentPhase = (Phase)1, VotesFinal = true }; playerId = player.Id; @@ -80,7 +80,7 @@ public class StateTests var phase = await Endpoints.EndpointHelpers.GetCurrentPhaseAsync(db, playerId); var player = await db.Players.FindAsync(playerId); Assert.Equal(Phase.Vote, phase); - Assert.Equal(Phase.Reveal, player!.CurrentPhase); + Assert.Equal((Phase)1, player!.CurrentPhase); Assert.True(player.VotesFinal); } } diff --git a/REVIEW.md b/REVIEW.md index eb3f095..a65778b 100644 --- a/REVIEW.md +++ b/REVIEW.md @@ -11,15 +11,11 @@ Active maintainability risks (priority order): - 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. -2. 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. - -3. Unauthenticated 401 response shape is still framework-driven (Medium) +2. 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. -4. Static analysis and frontend lint guardrails are still missing (Medium) +3. 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. @@ -33,14 +29,6 @@ Active maintainability risks (priority order): - 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`. @@ -68,10 +56,9 @@ Active maintainability risks (priority order): ## 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. +2. Normalize/declare unauthenticated 401 contract behavior. +3. Add analyzers + JS lint gates in CI. +4. Externalize i18n/FAQ assets. ## D) Guardrails diff --git a/wwwroot/app.js b/wwwroot/app.js index 65684df..2b5e9e9 100644 --- a/wwwroot/app.js +++ b/wwwroot/app.js @@ -24,7 +24,6 @@ import { import { loadState, loadSuggestData, - loadRevealData, loadVoteData, loadResults, refreshPhaseData, diff --git a/wwwroot/js/data.js b/wwwroot/js/data.js index 34f6d2c..60fe5d7 100644 --- a/wwwroot/js/data.js +++ b/wwwroot/js/data.js @@ -27,7 +27,7 @@ export async function loadSuggestData() { renderMySuggestions(); } -export async function loadRevealData() { +export async function loadSuggestionsData() { if (state.phase === "Vote" || state.phase === "Results") { const prev = state.allSuggestions ?? []; const prevById = Object.fromEntries(prev.map((s) => [s.id, s])); @@ -84,7 +84,7 @@ export async function refreshPhaseData() { const prevPhase = state.phase; const prevResultsOpen = state.resultsOpen; await loadState(); - await Promise.all([loadSuggestData(), loadRevealData(), loadResults()]); + await Promise.all([loadSuggestData(), loadSuggestionsData(), loadResults()]); if (state.phase === "Vote") { if (!state.votesRendered) await loadVoteData(); } else {