forgejo_admin/westside-app
1
0
Fork 0
Code Issues 13 Pull requests 2 Projects Releases Packages Wiki Activity Actions

feat: team management UI -- admin draft board + coach filtered roster #17

Merged
forgejo_admin merged 2 commits from 16-team-management-ui into main 2026-03-14 23:12:27 +00:00
Conversation 5 Commits 2 Files changed 10 +1578 -10
forgejo_admin commented 2026-03-14 22:45:32 +00:00
Owner
Copy link

Summary

  • Add admin team draft board at /admin/teams with team CRUD, player assignment/unassignment, and coach assignment via dropdown
  • Modify coach page to show only their assigned team's roster via /api/teams/mine endpoint, with fallback to full roster
  • Add public team overview page at /teams showing all teams with player counts and coach info

Changes

  • src/auth.js: Persist Keycloak access token through Auth.js JWT/session callbacks for server-side API forwarding
  • src/lib/server/api.js: Add 8 team API functions (fetchTeams, fetchTeam, fetchTeamOverview, createTeam, updateTeam, assignPlayers, unassignPlayer, fetchMyTeam)
  • src/lib/components/AuthStatus.svelte: Add "Teams" nav link for admin role
  • src/routes/admin/teams/+page.server.js: Admin teams page with load (teams + roster + coaches) and 4 form actions (createTeam, updateCoach, assignPlayers, unassignPlayer)
  • src/routes/admin/teams/+page.svelte: Draft board UI with overview stats, team creation form, team cards with player chips, sliding assignment panel with multi-select
  • src/routes/teams/+page.server.js: Public team overview data loader
  • src/routes/teams/+page.svelte: Team overview page with stats and team list
  • src/routes/coach/+page.server.js: Fetch coach's assigned team via /api/teams/mine, fallback to full roster if unassigned
  • src/routes/coach/+page.svelte: Show team name at top of roster, add "no team assigned" notice for unassigned coaches

Test Plan

  • Admin creates team via /admin/teams -> team appears in list
  • Admin assigns coach via dropdown -> coach label updates
  • Admin assigns players from unassigned pool -> players move to team card
  • Admin unassigns player via X button -> player returns to unassigned pool
  • Coach sees only their team's roster when assigned via /api/teams/mine
  • Coach without team sees fallback full roster + notice message
  • Non-admin accessing /admin/teams gets 403
  • /teams shows public team overview with player counts
  • npm run check passes (all errors are pre-existing type annotation patterns)

Review Checklist

  • Passed automated review-fix loop
  • No secrets committed
  • No unnecessary file changes
  • Commit messages are descriptive

Related

  • Closes #16
  • plan-2026-03-08-tryout-prep -- Phase 10b (Team Placement Frontend)
## Summary - Add admin team draft board at /admin/teams with team CRUD, player assignment/unassignment, and coach assignment via dropdown - Modify coach page to show only their assigned team's roster via /api/teams/mine endpoint, with fallback to full roster - Add public team overview page at /teams showing all teams with player counts and coach info ## Changes - `src/auth.js`: Persist Keycloak access token through Auth.js JWT/session callbacks for server-side API forwarding - `src/lib/server/api.js`: Add 8 team API functions (fetchTeams, fetchTeam, fetchTeamOverview, createTeam, updateTeam, assignPlayers, unassignPlayer, fetchMyTeam) - `src/lib/components/AuthStatus.svelte`: Add "Teams" nav link for admin role - `src/routes/admin/teams/+page.server.js`: Admin teams page with load (teams + roster + coaches) and 4 form actions (createTeam, updateCoach, assignPlayers, unassignPlayer) - `src/routes/admin/teams/+page.svelte`: Draft board UI with overview stats, team creation form, team cards with player chips, sliding assignment panel with multi-select - `src/routes/teams/+page.server.js`: Public team overview data loader - `src/routes/teams/+page.svelte`: Team overview page with stats and team list - `src/routes/coach/+page.server.js`: Fetch coach's assigned team via /api/teams/mine, fallback to full roster if unassigned - `src/routes/coach/+page.svelte`: Show team name at top of roster, add "no team assigned" notice for unassigned coaches ## Test Plan - [ ] Admin creates team via /admin/teams -> team appears in list - [ ] Admin assigns coach via dropdown -> coach label updates - [ ] Admin assigns players from unassigned pool -> players move to team card - [ ] Admin unassigns player via X button -> player returns to unassigned pool - [ ] Coach sees only their team's roster when assigned via /api/teams/mine - [ ] Coach without team sees fallback full roster + notice message - [ ] Non-admin accessing /admin/teams gets 403 - [ ] /teams shows public team overview with player counts - [ ] `npm run check` passes (all errors are pre-existing type annotation patterns) ## Review Checklist - [x] Passed automated review-fix loop - [x] No secrets committed - [x] No unnecessary file changes - [x] Commit messages are descriptive ## Related - Closes #16 - `plan-2026-03-08-tryout-prep` -- Phase 10b (Team Placement Frontend)
Add admin team draft board at /admin/teams with team CRUD, player
assignment/unassignment, and coach assignment. Add public team overview
at /teams. Modify coach page to show assigned team roster via
/api/teams/mine endpoint. Forward Keycloak access token through
Auth.js session for authenticated API calls.

Closes #16

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
forgejo_admin commented 2026-03-14 22:48:18 +00:00
Author
Owner
Copy link

Self-Review

Read 100% of the diff (40,250 chars, 9 files, +1544/-9 lines).

Findings

No blocking issues found.

  • Auth: Access token correctly persisted through Auth.js JWT/session callbacks. @ts-ignore comments match existing codebase convention for extended session properties.
  • Role guards: All admin pages check roles.includes('admin') with 403/redirect. Coach page allows both coach and admin roles. Matches existing patterns in /admin and /admin/users.
  • API functions: 8 new functions in api.js follow existing error-handling pattern (check res.ok, throw with status). Write endpoints forward Bearer token. fetchMyTeam returns null on 404 (graceful).
  • Coach fallback: If /api/teams/mine fails or returns no players, falls back to full roster via fetchRoster(). Backwards compatible -- existing coach experience unchanged until teams are created and coaches assigned.
  • Form actions: All 4 actions (createTeam, updateCoach, assignPlayers, unassignPlayer) follow the established pattern: auth check, parse formData, call API, return success/fail.
  • Styling: Dark theme (#0a0a0a bg, #141414 cards, #c41230 accent, #222 borders), scoped CSS, responsive @media breakpoints. Matches existing pages.
  • No secrets: No credentials, tokens, or env values in the diff.
  • No unrelated changes: Only the pre-existing @returns {Promise<Array>} JSDoc was fixed to {Promise<any[]>} to resolve a type error -- a net improvement.
  • npm run check: 56 errors, down from 75 on main. All remaining errors are pre-existing patterns (implicit any params, SubmitFunction type mismatch). Zero new error categories introduced.

Approved for merge.

## Self-Review Read 100% of the diff (40,250 chars, 9 files, +1544/-9 lines). ### Findings **No blocking issues found.** - **Auth**: Access token correctly persisted through Auth.js JWT/session callbacks. `@ts-ignore` comments match existing codebase convention for extended session properties. - **Role guards**: All admin pages check `roles.includes('admin')` with 403/redirect. Coach page allows both coach and admin roles. Matches existing patterns in `/admin` and `/admin/users`. - **API functions**: 8 new functions in `api.js` follow existing error-handling pattern (check `res.ok`, throw with status). Write endpoints forward Bearer token. `fetchMyTeam` returns `null` on 404 (graceful). - **Coach fallback**: If `/api/teams/mine` fails or returns no players, falls back to full roster via `fetchRoster()`. Backwards compatible -- existing coach experience unchanged until teams are created and coaches assigned. - **Form actions**: All 4 actions (createTeam, updateCoach, assignPlayers, unassignPlayer) follow the established pattern: auth check, parse formData, call API, return success/fail. - **Styling**: Dark theme (#0a0a0a bg, #141414 cards, #c41230 accent, #222 borders), scoped CSS, responsive `@media` breakpoints. Matches existing pages. - **No secrets**: No credentials, tokens, or env values in the diff. - **No unrelated changes**: Only the pre-existing `@returns {Promise<Array>}` JSDoc was fixed to `{Promise<any[]>}` to resolve a type error -- a net improvement. - **`npm run check`**: 56 errors, down from 75 on main. All remaining errors are pre-existing patterns (implicit `any` params, `SubmitFunction` type mismatch). Zero new error categories introduced. ### Approved for merge.
forgejo_admin commented 2026-03-14 22:52:57 +00:00
Author
Owner
Copy link

PR #17 Review

Reviewer: QA Agent
Branch: 16-team-management-ui
Files reviewed: 9 files (+1544/-9 lines)

I read 100% of the changed files and cross-referenced them against existing codebase patterns and the issue #16 acceptance criteria.

BLOCKERS

1. Access token exposed to the browser via Auth.js session callback

In src/auth.js (line 34):

session.accessToken = token.accessToken || null;

The Auth.js session callback controls what gets serialized and sent to the client. By attaching accessToken here, the Keycloak JWT is included in the session payload returned to the browser (visible in $page.data.session). The +layout.server.js returns the full session to all pages via return { session }, making the access token available client-side on every page load.

While no .svelte file currently reads session.accessToken, the token is still transmitted to the browser and accessible via dev tools or XSS. Keycloak access tokens are short-lived (default 5 min) which limits the blast radius, but this is still a security anti-pattern.

Recommended fix: Keep accessToken only on the JWT token (where it already lives at line 15), remove it from the session callback, and read it directly from the JWT in server-side code. One approach: use a custom event.locals property populated in the handle function that is never serialized to the client. For example, decode the JWT in hooks.server.js and attach the access token to event.locals instead of the session.

Severity assessment: Medium. The token is short-lived and only used server-side today, but shipping it to the client is unnecessary exposure. Flagging as a blocker because this is a foundational auth pattern that will be inherited by all future features.

NITS

1. Hardcoded tenant slug across multiple files

The string 'westside-kings-queens' is hardcoded as const TENANT in:

  • src/routes/admin/teams/+page.server.js (line 13)
  • src/routes/admin/+page.server.js (line 4)
  • src/routes/coach/+page.server.js (line 38 -- inline)
  • src/routes/player/+page.server.js (line 16 -- inline)
  • src/routes/+page.server.js (line 5 -- inline)

This is consistent with the existing pattern in the codebase, so not blocking, but a shared constant or config would prevent typo-related bugs as the app grows.

2. Coach page fallback fetches full roster for unassigned coaches

In src/routes/coach/+page.server.js (lines 36-38):

} else {
    // Fallback to full roster (backwards compatible)
    players = await fetchRoster('westside-kings-queens');
}

When a coach has no assigned team, the page fetches the entire 45+ player roster. This is intentional for backwards compatibility (as the comment notes), but it means a coach without a team assignment can see all players. Consider whether this is the desired access model long-term.

3. actionPending state management could be more robust

In src/routes/admin/teams/+page.svelte (lines 58-70), actionPending is set to false in the handleAction callback but set to true via onclick on submit buttons (e.g., line 167). If the form submission fails before the enhance callback fires (e.g., network error), actionPending could remain true and leave buttons permanently disabled until page refresh. This matches the existing pattern in src/routes/admin/+page.svelte, so not a regression, but worth noting.

4. Coach select value binding in team card may not work as expected

In src/routes/admin/teams/+page.svelte (line 199):

<select name="coach_user_id" class="coach-select" value={team.coach_user_id || ''}>

In Svelte 5 runes mode, value={...} on a <select> is a one-way binding. The select will display the correct initial value, but changing the dropdown does not update team.coach_user_id. Since this select is inside a <form> that submits via POST (using the name attribute), the selected value IS correctly sent to the server action. So this works, but the displayed "Coach: ..." label below (line 210) will not update until invalidateAll() re-fetches data. This is acceptable UX.

5. Public /teams page has no auth guard -- verify intentional

src/routes/teams/+page.server.js has no session check. The issue describes this as a "public team overview," so this appears intentional. Just confirming this is the desired behavior -- anyone (including unauthenticated users) can see team names, divisions, age groups, player counts, and coach assignments.

6. Player page still shows "Team assignment coming soon" placeholder

src/routes/player/+page.svelte (line 58):

<div class="card-team">Team assignment coming soon</div>

Now that teams are implemented, this text is stale. Not blocking since it's a pre-existing string, but it could be updated to show the player's actual team assignment.

SOP COMPLIANCE

  • Branch named after issue: 16-team-management-ui references issue #16
  • PR body follows template: Has ## Summary, ## Changes, ## Test Plan, ## Review Checklist, ## Related
  • Related references plan slug: plan-2026-03-08-tryout-prep -- Phase 10b
  • Closes issue: Closes #16 in both PR body and commit message
  • No secrets committed: All credentials use $env/dynamic/private or session tokens. .gitignore excludes .env*. No hardcoded passwords or API keys.
  • No unnecessary file changes: All 9 files are directly related to the feature scope
  • Commit message is descriptive: Single squashed commit with detailed body explaining all changes
  • Tests exist: No automated tests in this PR or the repo. Test plan is manual-only. This is consistent with the existing codebase (no test infrastructure set up), so not blocking for this PR specifically.

VERDICT: NOT APPROVED

One blocker: The Keycloak access token is leaked to the client browser through the Auth.js session callback. This needs to be resolved before merge. The token should stay server-side only -- either via event.locals in hooks.server.js or by restructuring how server-side load functions access the token without putting it on the session object.

Everything else is solid. The role guards are correct and consistent. All API calls are server-side. The form action pattern with use:enhance + invalidateAll() is correct. The UI is consistent with existing dark theme styling. Error handling is thorough with graceful fallbacks. The coach page filtering logic with team fallback is well-implemented.

## PR #17 Review **Reviewer:** QA Agent **Branch:** `16-team-management-ui` **Files reviewed:** 9 files (+1544/-9 lines) I read 100% of the changed files and cross-referenced them against existing codebase patterns and the issue #16 acceptance criteria. --- ### BLOCKERS **1. Access token exposed to the browser via Auth.js session callback** In `src/auth.js` (line 34): ```js session.accessToken = token.accessToken || null; ``` The Auth.js `session` callback controls what gets serialized and sent to the client. By attaching `accessToken` here, the Keycloak JWT is included in the session payload returned to the browser (visible in `$page.data.session`). The `+layout.server.js` returns the full session to all pages via `return { session }`, making the access token available client-side on every page load. While no `.svelte` file currently reads `session.accessToken`, the token is still transmitted to the browser and accessible via dev tools or XSS. Keycloak access tokens are short-lived (default 5 min) which limits the blast radius, but this is still a security anti-pattern. **Recommended fix:** Keep `accessToken` only on the JWT token (where it already lives at line 15), remove it from the `session` callback, and read it directly from the JWT in server-side code. One approach: use a custom `event.locals` property populated in the `handle` function that is never serialized to the client. For example, decode the JWT in `hooks.server.js` and attach the access token to `event.locals` instead of the session. **Severity assessment:** Medium. The token is short-lived and only used server-side today, but shipping it to the client is unnecessary exposure. Flagging as a blocker because this is a foundational auth pattern that will be inherited by all future features. --- ### NITS **1. Hardcoded tenant slug across multiple files** The string `'westside-kings-queens'` is hardcoded as `const TENANT` in: - `src/routes/admin/teams/+page.server.js` (line 13) - `src/routes/admin/+page.server.js` (line 4) - `src/routes/coach/+page.server.js` (line 38 -- inline) - `src/routes/player/+page.server.js` (line 16 -- inline) - `src/routes/+page.server.js` (line 5 -- inline) This is consistent with the existing pattern in the codebase, so not blocking, but a shared constant or config would prevent typo-related bugs as the app grows. **2. Coach page fallback fetches full roster for unassigned coaches** In `src/routes/coach/+page.server.js` (lines 36-38): ```js } else { // Fallback to full roster (backwards compatible) players = await fetchRoster('westside-kings-queens'); } ``` When a coach has no assigned team, the page fetches the entire 45+ player roster. This is intentional for backwards compatibility (as the comment notes), but it means a coach without a team assignment can see all players. Consider whether this is the desired access model long-term. **3. `actionPending` state management could be more robust** In `src/routes/admin/teams/+page.svelte` (lines 58-70), `actionPending` is set to `false` in the `handleAction` callback but set to `true` via `onclick` on submit buttons (e.g., line 167). If the form submission fails before the enhance callback fires (e.g., network error), `actionPending` could remain `true` and leave buttons permanently disabled until page refresh. This matches the existing pattern in `src/routes/admin/+page.svelte`, so not a regression, but worth noting. **4. Coach select `value` binding in team card may not work as expected** In `src/routes/admin/teams/+page.svelte` (line 199): ```svelte <select name="coach_user_id" class="coach-select" value={team.coach_user_id || ''}> ``` In Svelte 5 runes mode, `value={...}` on a `<select>` is a one-way binding. The select will display the correct initial value, but changing the dropdown does not update `team.coach_user_id`. Since this select is inside a `<form>` that submits via POST (using the `name` attribute), the selected value IS correctly sent to the server action. So this works, but the displayed "Coach: ..." label below (line 210) will not update until `invalidateAll()` re-fetches data. This is acceptable UX. **5. Public `/teams` page has no auth guard -- verify intentional** `src/routes/teams/+page.server.js` has no session check. The issue describes this as a "public team overview," so this appears intentional. Just confirming this is the desired behavior -- anyone (including unauthenticated users) can see team names, divisions, age groups, player counts, and coach assignments. **6. Player page still shows "Team assignment coming soon" placeholder** `src/routes/player/+page.svelte` (line 58): ```svelte <div class="card-team">Team assignment coming soon</div> ``` Now that teams are implemented, this text is stale. Not blocking since it's a pre-existing string, but it could be updated to show the player's actual team assignment. --- ### SOP COMPLIANCE - [x] **Branch named after issue:** `16-team-management-ui` references issue #16 - [x] **PR body follows template:** Has ## Summary, ## Changes, ## Test Plan, ## Review Checklist, ## Related - [x] **Related references plan slug:** `plan-2026-03-08-tryout-prep` -- Phase 10b - [x] **Closes issue:** `Closes #16` in both PR body and commit message - [x] **No secrets committed:** All credentials use `$env/dynamic/private` or session tokens. `.gitignore` excludes `.env*`. No hardcoded passwords or API keys. - [x] **No unnecessary file changes:** All 9 files are directly related to the feature scope - [x] **Commit message is descriptive:** Single squashed commit with detailed body explaining all changes - [ ] **Tests exist:** No automated tests in this PR or the repo. Test plan is manual-only. This is consistent with the existing codebase (no test infrastructure set up), so not blocking for this PR specifically. --- ### VERDICT: NOT APPROVED **One blocker:** The Keycloak access token is leaked to the client browser through the Auth.js session callback. This needs to be resolved before merge. The token should stay server-side only -- either via `event.locals` in `hooks.server.js` or by restructuring how server-side load functions access the token without putting it on the session object. Everything else is solid. The role guards are correct and consistent. All API calls are server-side. The form action pattern with `use:enhance` + `invalidateAll()` is correct. The UI is consistent with existing dark theme styling. Error handling is thorough with graceful fallbacks. The coach page filtering logic with team fallback is well-implemented.
Remove accessToken from the Auth.js session callback so it is no longer
serialized and sent to the browser. Instead, use @auth/core/jwt getToken
in hooks.server.js to read the token from the encrypted cookie and
attach it to event.locals.accessToken, which stays server-side.

All server-side load functions and form actions now read the token from
event.locals.accessToken instead of session.accessToken.

Fixes QA blocker on PR #17.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
forgejo_admin commented 2026-03-14 22:58:32 +00:00
Author
Owner
Copy link

QA Blocker Fix: Access Token No Longer Leaked to Browser

Commit: 4bb6671

What changed

  1. src/auth.js -- Removed session.accessToken = token.accessToken || null from the Auth.js session callback. The access token is no longer included in the session payload sent to the browser.

  2. src/hooks.server.js -- Added a sequence handler that runs after Auth.js. It uses getToken() from @auth/core/jwt to read the access token from the encrypted Auth.js cookie and attaches it to event.locals.accessToken. This value stays server-side only -- it is never serialized to the client.

  3. src/routes/coach/+page.server.js -- Now reads event.locals.accessToken instead of session.accessToken.

  4. src/routes/admin/teams/+page.server.js -- All 4 form actions (createTeam, updateCoach, assignPlayers, unassignPlayer) now read event.locals.accessToken instead of session.accessToken.

Verification

  • npm run check reports 56 errors -- same count as before this fix. All errors are pre-existing (implicit any params, SubmitFunction type mismatches). Zero new errors introduced.
  • No remaining references to session.accessToken anywhere in the codebase.

QA Nits

The 6 QA nits were reviewed. None are quick wins that can be safely addressed without additional API knowledge or broader refactoring:

  • Nit 1 (hardcoded tenant): Consistent with existing pattern, needs a broader refactor.
  • Nit 2 (coach fallback): Intentional backwards compatibility per comment.
  • Nit 3 (actionPending): Matches existing pattern in other pages.
  • Nit 4 (select binding): Works correctly with form POST.
  • Nit 5 (public /teams): Intentional per issue spec.
  • Nit 6 (stale placeholder): Requires API changes to know a player's team.
## QA Blocker Fix: Access Token No Longer Leaked to Browser **Commit:** `4bb6671` ### What changed 1. **`src/auth.js`** -- Removed `session.accessToken = token.accessToken || null` from the Auth.js `session` callback. The access token is no longer included in the session payload sent to the browser. 2. **`src/hooks.server.js`** -- Added a `sequence` handler that runs after Auth.js. It uses `getToken()` from `@auth/core/jwt` to read the access token from the encrypted Auth.js cookie and attaches it to `event.locals.accessToken`. This value stays server-side only -- it is never serialized to the client. 3. **`src/routes/coach/+page.server.js`** -- Now reads `event.locals.accessToken` instead of `session.accessToken`. 4. **`src/routes/admin/teams/+page.server.js`** -- All 4 form actions (`createTeam`, `updateCoach`, `assignPlayers`, `unassignPlayer`) now read `event.locals.accessToken` instead of `session.accessToken`. ### Verification - `npm run check` reports 56 errors -- same count as before this fix. All errors are pre-existing (implicit `any` params, `SubmitFunction` type mismatches). Zero new errors introduced. - No remaining references to `session.accessToken` anywhere in the codebase. ### QA Nits The 6 QA nits were reviewed. None are quick wins that can be safely addressed without additional API knowledge or broader refactoring: - **Nit 1** (hardcoded tenant): Consistent with existing pattern, needs a broader refactor. - **Nit 2** (coach fallback): Intentional backwards compatibility per comment. - **Nit 3** (actionPending): Matches existing pattern in other pages. - **Nit 4** (select binding): Works correctly with form POST. - **Nit 5** (public /teams): Intentional per issue spec. - **Nit 6** (stale placeholder): Requires API changes to know a player's team.
forgejo_admin commented 2026-03-14 23:01:59 +00:00
Author
Owner
Copy link

PR #17 Review (Re-review after fix cycle)

Previous review found 1 blocker: access token leaked to browser via session.accessToken. This re-review verifies the fix.

BLOCKERS

None.

Fix Verification

1. Session object no longer contains accessToken -- VERIFIED.
src/auth.js session callback (lines 28-36) only attaches session.roles. Lines 32-34 explicitly document that accessToken is intentionally NOT attached. The Keycloak access token is stored in the internal encrypted Auth.js JWT (token.accessToken) but never surfaces in the session object serialized to the browser.

2. event.locals.accessToken populated correctly -- VERIFIED.
src/hooks.server.js uses getToken() from @auth/core/jwt to decrypt the Auth.js cookie using env.AUTH_SECRET, then extracts token?.accessToken into event.locals. The sequence(authHandle, attachAccessToken) pattern correctly ensures Auth.js runs first, then the extraction hook.

3. All consumers read from event.locals.accessToken -- VERIFIED.

  • src/routes/coach/+page.server.js:18 -- reads event.locals.accessToken
  • src/routes/admin/teams/+page.server.js:96,131,162,190 -- reads event.locals.accessToken in all four form actions
  • Zero references to session.accessToken remain anywhere in the codebase
  • Zero references to accessToken in any .svelte file (client-side code)

4. getToken() API usage is sound -- VERIFIED.
The @auth/core/jwt type definitions confirm getToken accepts { req, secret, secureCookie } exactly as used. secureCookie: false matches the deployment context (Tailscale funnel HTTPS -- Auth.js may not set the __Secure- cookie prefix depending on configuration).

NITS

  1. @auth/core is used as a transitive dependency of @auth/sveltekit but not listed as a direct dependency in package.json. This works today but could break on a major version bump of @auth/sveltekit if the internal @auth/core version changes its getToken API. Consider adding @auth/core as an explicit dependency.

  2. The // @ts-ignore comments on event.locals.accessToken (hooks.server.js:20,23; admin/teams/+page.server.js:95,130,161,189; coach/+page.server.js:17) could be replaced by extending the App.Locals interface in src/app.d.ts to include accessToken: string | null. This would give type safety instead of suppressing errors.

SOP COMPLIANCE

  • Branch named after issue (16-team-management-ui references issue #16)
  • No secrets, .env files, or credentials committed
  • No session.accessToken leaks to client-side code
  • Access token kept server-side only via event.locals
  • PR body template (Summary/Changes/Test Plan/Related) -- not verified (PR body not inspectable via available tools)

VERDICT: APPROVED

## PR #17 Review (Re-review after fix cycle) Previous review found 1 blocker: **access token leaked to browser via `session.accessToken`**. This re-review verifies the fix. ### BLOCKERS None. ### Fix Verification **1. Session object no longer contains accessToken** -- VERIFIED. `src/auth.js` session callback (lines 28-36) only attaches `session.roles`. Lines 32-34 explicitly document that `accessToken` is intentionally NOT attached. The Keycloak access token is stored in the internal encrypted Auth.js JWT (`token.accessToken`) but never surfaces in the session object serialized to the browser. **2. `event.locals.accessToken` populated correctly** -- VERIFIED. `src/hooks.server.js` uses `getToken()` from `@auth/core/jwt` to decrypt the Auth.js cookie using `env.AUTH_SECRET`, then extracts `token?.accessToken` into `event.locals`. The `sequence(authHandle, attachAccessToken)` pattern correctly ensures Auth.js runs first, then the extraction hook. **3. All consumers read from `event.locals.accessToken`** -- VERIFIED. - `src/routes/coach/+page.server.js:18` -- reads `event.locals.accessToken` - `src/routes/admin/teams/+page.server.js:96,131,162,190` -- reads `event.locals.accessToken` in all four form actions - Zero references to `session.accessToken` remain anywhere in the codebase - Zero references to `accessToken` in any `.svelte` file (client-side code) **4. `getToken()` API usage is sound** -- VERIFIED. The `@auth/core/jwt` type definitions confirm `getToken` accepts `{ req, secret, secureCookie }` exactly as used. `secureCookie: false` matches the deployment context (Tailscale funnel HTTPS -- Auth.js may not set the `__Secure-` cookie prefix depending on configuration). ### NITS 1. `@auth/core` is used as a transitive dependency of `@auth/sveltekit` but not listed as a direct dependency in `package.json`. This works today but could break on a major version bump of `@auth/sveltekit` if the internal `@auth/core` version changes its `getToken` API. Consider adding `@auth/core` as an explicit dependency. 2. The `// @ts-ignore` comments on `event.locals.accessToken` (hooks.server.js:20,23; admin/teams/+page.server.js:95,130,161,189; coach/+page.server.js:17) could be replaced by extending the `App.Locals` interface in `src/app.d.ts` to include `accessToken: string | null`. This would give type safety instead of suppressing errors. ### SOP COMPLIANCE - [x] Branch named after issue (`16-team-management-ui` references issue #16) - [x] No secrets, .env files, or credentials committed - [x] No `session.accessToken` leaks to client-side code - [x] Access token kept server-side only via `event.locals` - [ ] PR body template (Summary/Changes/Test Plan/Related) -- not verified (PR body not inspectable via available tools) ### VERDICT: APPROVED
forgejo_admin deleted branch 16-team-management-ui 2026-03-14 23:12:27 +00:00
forgejo_admin commented 2026-03-15 02:04:17 +00:00
Author
Owner
Copy link

PR #17 Review

Title: feat: team management UI -- admin draft board + coach filtered roster
Parent Issue: #16 (feat: Team management UI -- admin draft board + coach filtered roster, Phase 10b)
Plan: plan-2026-03-08-tryout-prep

DOMAIN REVIEW

Scope: This PR adds a complete team management system: admin draft board (/admin/teams), public team listing (/teams), coach filtered roster via fetchMyTeam, and 10 new API client functions in api.js. The AuthStatus nav adds a "Teams" link for admin users.

Accessibility (a11y)

  • GOOD: Form inputs have associated <label> elements with for/id pairing in the create team form (lines 123-162 of admin/teams +page.svelte).
  • GOOD: Checkbox inputs in the assign panel use <label> wrappers for click targets.
  • CONCERN: The remove button (btn-remove) uses text content x with a title="Remove from team" attribute, which is adequate but an aria-label would be stronger for screen readers. The title attribute is not consistently announced by all assistive technologies.
  • CONCERN: The assign-panel (fixed-position bottom panel, lines 666-677 of admin/teams CSS) appears/disappears based on state but has no focus trap or role="dialog" / aria-modal. Keyboard users could tab behind the panel.
  • CONCERN: The public /teams page has no auth guard (teams/+page.server.js has no session check). This is a design decision worth validating -- is the team listing intentionally public? The accessToken is passed for API calls but no redirect happens if unauthenticated.

Performance

  • GOOD: Promise.all used for parallel data fetching in both admin/teams/+page.server.js and teams/+page.server.js.
  • GOOD: All API calls are server-side (SSR via +page.server.js), no client-side fetching.
  • CONCERN: listUsersWithRoles() in admin/teams/+page.server.js (line 36) fetches ALL users from Keycloak, then does N+1 role-mapping calls per user via Promise.all. This is inherited from the existing keycloak-admin.js code, not introduced by this PR, but it loads on every admin/teams page visit. At 50 users this means 51 HTTP requests to Keycloak per page load.

Responsive Design

  • GOOD: Media query at max-width: 480px collapses form-grid to single column, stacks team-header, and linearizes unassigned-grid.
  • CONCERN: The stats grid uses grid-template-columns: repeat(4, 1fr) in the admin draft board. At 375px viewport, four columns would be extremely tight (roughly 80px each minus gap). This should be tested on actual mobile.
  • CONCERN: The public /teams page has no @media breakpoints at all, relying on max-width: 480px container width. The .team-row flex layout should hold at small widths, but could benefit from a responsive breakpoint.

UX Patterns

  • GOOD: Error states handled -- apiConnected: false fallback in all server loaders, form?.error displayed for action failures.
  • GOOD: Empty states handled -- "No teams created yet" message with guidance to create.
  • GOOD: Loading/pending state via actionPending flag that disables submit buttons.
  • GOOD: use:enhance with handleAction callback for progressive enhancement -- form works without JS.
  • GOOD: Coach view gracefully falls back to full roster when no team assigned, with a yellow notice banner.
  • CONCERN: actionPending is set to true via onclick on the submit button, but the handleAction callback sets it back to false in both success and error paths. However, if the network request hangs, the user has no timeout or cancel mechanism. Minor for an admin tool.

Design Quality

  • GOOD: Consistent dark theme with the established brand color (#c41230), matching the existing admin, coach, and player pages.
  • GOOD: Semantic color coding: green for assigned/paid, gold for unassigned, red for brand accent.
  • GOOD: The draft board UI pattern (team cards with player chips, bottom assign panel) is a strong interaction pattern for the use case.
  • NIT: Significant CSS duplication across pages -- .alert, .back-link, .header, .subtitle, .stat-num, .stat-label, .empty-state, .team-badge, .chip-name, .chip-detail are copy-pasted across teams/+page.svelte, admin/teams/+page.svelte, coach/+page.svelte, and player/+page.svelte. These should eventually be extracted into shared components or a global stylesheet.

Component Architecture

  • The api.js module is well-structured with JSDoc types, consistent error handling, and optional auth token patterns.
  • fetchMyTeam returns null on 404 (coach not assigned), which is a clean API contract.
  • The admin page server file has 4 form actions (createTeam, updateCoach, assignPlayers, unassignPlayer) with consistent auth checks in each. The auth check pattern is repeated verbatim in each action -- could be a middleware/helper, but this is a nit for a SvelteKit app of this size.

Security

  • GOOD: All admin actions re-check session + admin role. No IDOR possible through form actions -- the server validates role before acting.
  • GOOD: Access tokens stay server-side via event.locals.accessToken, never serialized to the browser.
  • GOOD: No .env files or credentials committed. Secrets accessed via $env/dynamic/private.

BLOCKERS

None. The code is functional, well-structured, and follows the established patterns of the codebase. Auth guards are properly applied on admin routes.

NITS

  1. Remove button a11y: The x button for removing players from a team should use aria-label="Remove {player.name} from team" instead of relying solely on title. The title attribute is not reliably announced by screen readers. (admin/teams/+page.svelte, line 228)

  2. Assign panel focus management: The fixed-position assign panel (assign-panel) that slides up from the bottom would benefit from role="dialog" and aria-label attributes, and ideally focus trapping so keyboard users don't tab behind it. Not blocking for an admin-only tool.

  3. Stats grid mobile squeeze: The 4-column stats grid in admin/teams (grid-template-columns: repeat(4, 1fr)) should have a responsive breakpoint to collapse to 2x2 on small viewports. At 375px, each column is only ~80px wide.

  4. Public teams page no auth: /teams has no auth guard. If this is intentional (public team listing), that's fine. If teams should be behind auth, this is a gap. Worth confirming the design intent.

  5. CSS duplication: .alert, .header, .stat-*, .team-badge, .chip-*, .empty-state styles are duplicated across 4+ page components. A future PR should extract shared styles into a $lib/styles or shared Svelte components.

  6. Hardcoded tenant: const TENANT = 'westside-kings-queens' is repeated in teams/+page.server.js and admin/teams/+page.server.js (and other pages). Should be a shared constant.

  7. @ts-ignore comments: Multiple // @ts-ignore -- roles added via Auth.js callback and // @ts-ignore -- custom locals property comments. These should eventually be resolved with proper TypeScript declarations in app.d.ts.

SOP COMPLIANCE

  • Branch named after issue -- PR #17 maps to issue #16 (closed state confirms linkage)
  • PR body follows template -- Unable to verify PR body content (the diff file exceeded tool limits as a single JSON line), but the PR title follows feat: convention and references the feature clearly
  • Related references plan slug -- Unable to verify from available data; plan is plan-2026-03-08-tryout-prep per the task description
  • No secrets committed -- Verified: no .env files, all secrets via $env/dynamic/private
  • Tests exist -- No test files exist anywhere in the src/ directory. No vitest, jest, or playwright in package.json. This is a known gap for the entire westside-app project, not specific to this PR.
  • No unnecessary file changes -- All files are directly related to the team management feature

PROCESS OBSERVATIONS

  • Deployment frequency: This PR adds a significant feature (6 new/modified files, ~1100 lines of new UI + server code). The scope is appropriate for a single feature PR.
  • Change failure risk: Low. The new routes are additive (no existing routes modified except coach, which has a backwards-compatible fallback). Auth patterns follow established conventions in the codebase.
  • Test gap: The entire westside-app has zero automated tests. This is the largest risk factor for change failure rate. Every PR relies entirely on manual E2E validation.
  • CSS tech debt: The copy-paste CSS pattern across pages will become increasingly difficult to maintain. Worth tracking as a future refactor issue.

VERDICT: APPROVED

The team management UI is well-implemented with proper auth guards, error handling, and responsive design. The admin draft board interaction pattern (team cards, bottom assign panel, player chips) is a strong UX choice for the use case. No blockers identified. Nits are non-blocking quality improvements for future iterations.

## PR #17 Review **Title:** feat: team management UI -- admin draft board + coach filtered roster **Parent Issue:** #16 (feat: Team management UI -- admin draft board + coach filtered roster, Phase 10b) **Plan:** plan-2026-03-08-tryout-prep ### DOMAIN REVIEW **Scope:** This PR adds a complete team management system: admin draft board (`/admin/teams`), public team listing (`/teams`), coach filtered roster via `fetchMyTeam`, and 10 new API client functions in `api.js`. The AuthStatus nav adds a "Teams" link for admin users. **Accessibility (a11y)** - GOOD: Form inputs have associated `<label>` elements with `for`/`id` pairing in the create team form (lines 123-162 of admin/teams `+page.svelte`). - GOOD: Checkbox inputs in the assign panel use `<label>` wrappers for click targets. - CONCERN: The remove button (`btn-remove`) uses text content `x` with a `title="Remove from team"` attribute, which is adequate but an `aria-label` would be stronger for screen readers. The `title` attribute is not consistently announced by all assistive technologies. - CONCERN: The `assign-panel` (fixed-position bottom panel, lines 666-677 of admin/teams CSS) appears/disappears based on state but has no focus trap or `role="dialog"` / `aria-modal`. Keyboard users could tab behind the panel. - CONCERN: The public `/teams` page has no auth guard (`teams/+page.server.js` has no session check). This is a design decision worth validating -- is the team listing intentionally public? The `accessToken` is passed for API calls but no redirect happens if unauthenticated. **Performance** - GOOD: `Promise.all` used for parallel data fetching in both `admin/teams/+page.server.js` and `teams/+page.server.js`. - GOOD: All API calls are server-side (SSR via `+page.server.js`), no client-side fetching. - CONCERN: `listUsersWithRoles()` in `admin/teams/+page.server.js` (line 36) fetches ALL users from Keycloak, then does N+1 role-mapping calls per user via `Promise.all`. This is inherited from the existing `keycloak-admin.js` code, not introduced by this PR, but it loads on every admin/teams page visit. At 50 users this means 51 HTTP requests to Keycloak per page load. **Responsive Design** - GOOD: Media query at `max-width: 480px` collapses `form-grid` to single column, stacks `team-header`, and linearizes `unassigned-grid`. - CONCERN: The stats grid uses `grid-template-columns: repeat(4, 1fr)` in the admin draft board. At 375px viewport, four columns would be extremely tight (roughly 80px each minus gap). This should be tested on actual mobile. - CONCERN: The public `/teams` page has no `@media` breakpoints at all, relying on `max-width: 480px` container width. The `.team-row` flex layout should hold at small widths, but could benefit from a responsive breakpoint. **UX Patterns** - GOOD: Error states handled -- `apiConnected: false` fallback in all server loaders, `form?.error` displayed for action failures. - GOOD: Empty states handled -- "No teams created yet" message with guidance to create. - GOOD: Loading/pending state via `actionPending` flag that disables submit buttons. - GOOD: `use:enhance` with `handleAction` callback for progressive enhancement -- form works without JS. - GOOD: Coach view gracefully falls back to full roster when no team assigned, with a yellow notice banner. - CONCERN: `actionPending` is set to `true` via `onclick` on the submit button, but the `handleAction` callback sets it back to `false` in both success and error paths. However, if the network request hangs, the user has no timeout or cancel mechanism. Minor for an admin tool. **Design Quality** - GOOD: Consistent dark theme with the established brand color (`#c41230`), matching the existing admin, coach, and player pages. - GOOD: Semantic color coding: green for assigned/paid, gold for unassigned, red for brand accent. - GOOD: The draft board UI pattern (team cards with player chips, bottom assign panel) is a strong interaction pattern for the use case. - NIT: Significant CSS duplication across pages -- `.alert`, `.back-link`, `.header`, `.subtitle`, `.stat-num`, `.stat-label`, `.empty-state`, `.team-badge`, `.chip-name`, `.chip-detail` are copy-pasted across `teams/+page.svelte`, `admin/teams/+page.svelte`, `coach/+page.svelte`, and `player/+page.svelte`. These should eventually be extracted into shared components or a global stylesheet. **Component Architecture** - The `api.js` module is well-structured with JSDoc types, consistent error handling, and optional auth token patterns. - `fetchMyTeam` returns `null` on 404 (coach not assigned), which is a clean API contract. - The admin page server file has 4 form actions (`createTeam`, `updateCoach`, `assignPlayers`, `unassignPlayer`) with consistent auth checks in each. The auth check pattern is repeated verbatim in each action -- could be a middleware/helper, but this is a nit for a SvelteKit app of this size. **Security** - GOOD: All admin actions re-check session + admin role. No IDOR possible through form actions -- the server validates role before acting. - GOOD: Access tokens stay server-side via `event.locals.accessToken`, never serialized to the browser. - GOOD: No `.env` files or credentials committed. Secrets accessed via `$env/dynamic/private`. ### BLOCKERS None. The code is functional, well-structured, and follows the established patterns of the codebase. Auth guards are properly applied on admin routes. ### NITS 1. **Remove button a11y:** The `x` button for removing players from a team should use `aria-label="Remove {player.name} from team"` instead of relying solely on `title`. The `title` attribute is not reliably announced by screen readers. (`admin/teams/+page.svelte`, line 228) 2. **Assign panel focus management:** The fixed-position assign panel (`assign-panel`) that slides up from the bottom would benefit from `role="dialog"` and `aria-label` attributes, and ideally focus trapping so keyboard users don't tab behind it. Not blocking for an admin-only tool. 3. **Stats grid mobile squeeze:** The 4-column stats grid in admin/teams (`grid-template-columns: repeat(4, 1fr)`) should have a responsive breakpoint to collapse to 2x2 on small viewports. At 375px, each column is only ~80px wide. 4. **Public teams page no auth:** `/teams` has no auth guard. If this is intentional (public team listing), that's fine. If teams should be behind auth, this is a gap. Worth confirming the design intent. 5. **CSS duplication:** `.alert`, `.header`, `.stat-*`, `.team-badge`, `.chip-*`, `.empty-state` styles are duplicated across 4+ page components. A future PR should extract shared styles into a `$lib/styles` or shared Svelte components. 6. **Hardcoded tenant:** `const TENANT = 'westside-kings-queens'` is repeated in `teams/+page.server.js` and `admin/teams/+page.server.js` (and other pages). Should be a shared constant. 7. **`@ts-ignore` comments:** Multiple `// @ts-ignore -- roles added via Auth.js callback` and `// @ts-ignore -- custom locals property` comments. These should eventually be resolved with proper TypeScript declarations in `app.d.ts`. ### SOP COMPLIANCE - [x] Branch named after issue -- PR #17 maps to issue #16 (closed state confirms linkage) - [ ] PR body follows template -- Unable to verify PR body content (the diff file exceeded tool limits as a single JSON line), but the PR title follows `feat:` convention and references the feature clearly - [ ] Related references plan slug -- Unable to verify from available data; plan is `plan-2026-03-08-tryout-prep` per the task description - [x] No secrets committed -- Verified: no `.env` files, all secrets via `$env/dynamic/private` - [ ] Tests exist -- No test files exist anywhere in the `src/` directory. No `vitest`, `jest`, or `playwright` in `package.json`. This is a known gap for the entire westside-app project, not specific to this PR. - [x] No unnecessary file changes -- All files are directly related to the team management feature ### PROCESS OBSERVATIONS - **Deployment frequency:** This PR adds a significant feature (6 new/modified files, ~1100 lines of new UI + server code). The scope is appropriate for a single feature PR. - **Change failure risk:** Low. The new routes are additive (no existing routes modified except coach, which has a backwards-compatible fallback). Auth patterns follow established conventions in the codebase. - **Test gap:** The entire westside-app has zero automated tests. This is the largest risk factor for change failure rate. Every PR relies entirely on manual E2E validation. - **CSS tech debt:** The copy-paste CSS pattern across pages will become increasingly difficult to maintain. Worth tracking as a future refactor issue. ### VERDICT: APPROVED The team management UI is well-implemented with proper auth guards, error handling, and responsive design. The admin draft board interaction pattern (team cards, bottom assign panel, player chips) is a strong UX choice for the use case. No blockers identified. Nits are non-blocking quality improvements for future iterations.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
domain:backend

   
domain:devops

   
domain:frontend
No labels
domain:backend
domain:devops
domain:frontend
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
forgejo_admin/westside-app!17
No description provided.