Review: Playground overhaul 2/4: Public pages (8 pages)

review-382-2026-03-25 Review

review needs-refinement

Verdict: NEEDS_REFINEMENT

Template Completeness

  • [x] Lineage — present, traces to westside-app#84 and parent note
  • [x] Repo — present (forgejo_admin/westside-playground)
  • [x] User Story — present, two stories (WS-S26, WS-S27)
  • [x] Context — present, explains public pages + Keycloak mocks
  • [x] File Targets — present, 8 files listed with descriptions
  • [x] Acceptance Criteria — present, 6 items
  • [x] Test Expectations — present, 3 items
  • [x] Constraints — present, 4 items
  • [x] Checklist — present
  • [x] Related — present
All required template sections are present.

File Targets

  • [x] index.html — verified: exists (11k). Has component docs (@route, @auth, @api). Currently references shared/app.css.
  • [x] staff.html — verified: exists (9.8k). Has 5 coach cards (Marcus, James Taylor, KJ Ng, Ken, Abbie Sa). Currently references css/style.css (old path).
  • [x] sponsors.html — verified: exists (5.4k). Currently references css/style.css (old path).
  • [x] register.html — verified: exists (16k). Has component docs. Currently references shared/app.css.
  • [x] schedule.html — does not exist yet, correctly marked as create
  • [x] login.html — does not exist yet, correctly marked as create
  • [x] forgot-password.html — does not exist yet, correctly marked as create
  • [x] reset-password.html — does not exist yet, correctly marked as create
  • [ ] staff.html — ISSUE: missing component doc comment (@route, @auth, @api). Ticket says to add them but doesn't note they're currently absent.
  • [ ] sponsors.html — ISSUE: missing component doc comment (@route, @auth, @api). Same gap.
  • [ ] CSS reference shared/style.css — ISSUE: this file does not exist yet. Sub-ticket #85 will create it by merging css/style.css + shared/app.css. Ticket correctly depends on #85 but should explicitly state the filename is the post-#85 unified file.

Repo Placement

MISMATCH (minor): The Forgejo issue is filed on forgejo_admin/westside-app (issue #86), but the ### Repo section correctly says work targets forgejo_admin/westside-playground. The westside-playground repo exists on Forgejo. All four sub-tickets (#85-#88) are filed on westside-app as the project tracker. An agent dispatched to this issue could branch from the wrong repo. The ticket should add a note: "Issue tracked on westside-app; PR branch and work happen on westside-playground repo."

Dependencies

  • ws-85 (sub-ticket 1: CSS/JS unification) — board item #381, column: todo, state: open. This ticket explicitly depends on #85 being merged AND phone-approved. Correctly documented in labels (depends:ws-85) and Constraints section.
  • ws-84 (parent overhaul) — board item #380, column: todo. Parent decomposition. Correctly referenced.
  • ws-87 (sub-ticket 3: auth pages) — board item #383, column: todo. Sibling, also depends on #85. No conflict.
  • ws-88 (sub-ticket 4: cleanup) — board item #384, column: todo. Depends on this ticket (depends:ws-86+ws-87). Correctly documented.
  • ws-76 (Coach Manny) — open issue. Ticket correctly notes Coach Manny is added separately via #76.
  • signin.html overlap — NOT DOCUMENTED. Existing signin.html (Keycloak OIDC redirect page) overlaps conceptually with new login.html (Keycloak-themed static mock). Agent may be confused about whether to update, replace, or coexist with signin.html.

Acceptance Criteria

  • [x] "All 8 pages exist and render on mobile" — testable via visual check on 390px viewport
  • [x] "All reference shared/style.css and shared/app.js" — testable via grep. Correct post-#85 filename. Temporally gated by dependency.
  • [ ] "Consistent public nav across all public pages" — AMBIGUOUS: current pages have three different nav patterns (full nav on staff/sponsors, minimal nav on register, no nav element on index). Ticket doesn't define which links the target nav should contain or which pattern to follow.
  • [ ] "No stale tryout references" — AMBIGUOUS: index.html currently has a tryout banner, tryout CTA button, and tryout links. register.html references tryout fee. The ticket says "No tryout CTA when tryouts inactive" but doesn't say whether to remove or conditionally hide. Test expectation "grep for tryout returns zero (except schedule TBD)" conflicts with keeping any tryout content.
  • [x] "Login flow is navigable" — testable: click through login -> dashboard, forgot -> email confirmation -> reset -> login
  • [x] "Component doc comment at top of each page" — testable via grep for @route/@auth/@api

Blast Radius

  • Nav changes: If the agent defines a standard public nav, it will need to touch pages beyond the 8 listed. tryouts.html, success.html, and signin.html also have public nav. The ticket scopes 8 pages but nav consistency may require updating others. Risk of scope creep or inconsistent result.
  • Tryout content removal: Removing tryout references from index.html would break the primary user flow (landing page -> tryout registration) that users currently see. This needs explicit guidance on intent.
  • No downstream risk: This is playground-only (static HTML). The SvelteKit westside-app is a separate repo/deployment. No production blast radius.

Recommendation

Four specific refinements needed before this ticket is READY:
  • Clarify repo placement for agent dispatch: Add a note to the issue body stating "Issue tracked on westside-app; PR branch and work happen on the westside-playground repo." This prevents an agent from branching on the wrong repo.
  • Define the target public nav: Specify which links the consistent public nav should contain. Current candidates: Home, About, Staff, Sponsors, Schedule, Sign In. Should it include the new schedule page? Should tryouts.html stay in the nav? State the canonical link list.
  • Define "stale tryout" handling on index.html: Clarify what happens to the tryout banner and CTA on the landing page. Options: (a) remove entirely and replace with generic program CTA, (b) replace with "no upcoming tryouts — check back soon," (c) keep tryout section but update dates/text. The test expectation "grep for tryout returns zero" implies full removal, but the landing page currently revolves around tryout promotion.
  • Document signin.html vs login.html relationship: Add to Context or File Targets that signin.html is the real Keycloak OIDC redirect page (kept as-is or updated in sub-ticket 3), while login.html is a new static Keycloak-themed mock. Clarify coexistence so the agent doesn't accidentally merge or conflict them.