fix: pass Authorization header in server-side team API calls #19

Merged

forgejo_admin merged 1 commit from 18-bug-team-api-calls-return-401-server-sid into main

2026-03-15 01:46:41 +00:00

forgejo_admin commented

2026-03-15 01:42:19 +00:00

Owner

Summary

Server-side load functions in /admin/teams and /teams were calling fetchTeams() and fetchTeamOverview() without the Keycloak access token, causing 401 Unauthorized responses from basketball-api. This fix wires the token through from event.locals.accessToken, matching the pattern already established in /coach/+page.server.js.

Changes

src/lib/server/api.js: Added optional token parameter to fetchTeams(), fetchTeam(), and fetchTeamOverview(). When provided, sets Authorization: Bearer header. Backwards compatible.
src/routes/admin/teams/+page.server.js: Extract accessToken from event.locals in the load function and pass it to fetchTeams() and fetchTeamOverview(). Form actions already had this pattern.
src/routes/teams/+page.server.js: Accept event parameter in load (was previously bare load()), extract accessToken, and pass to both API calls.

Test Plan

Log in as admin, navigate to /admin/teams -- teams list loads without 401
Navigate to /teams -- teams load with or without active session
Verify /coach page still works (no regression -- already correct)
Verify form actions on /admin/teams (create team, assign players, etc.) still work

Review Checklist

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

Closes #18

## Summary Server-side load functions in `/admin/teams` and `/teams` were calling `fetchTeams()` and `fetchTeamOverview()` without the Keycloak access token, causing 401 Unauthorized responses from basketball-api. This fix wires the token through from `event.locals.accessToken`, matching the pattern already established in `/coach/+page.server.js`. ## Changes - `src/lib/server/api.js`: Added optional `token` parameter to `fetchTeams()`, `fetchTeam()`, and `fetchTeamOverview()`. When provided, sets `Authorization: Bearer` header. Backwards compatible. - `src/routes/admin/teams/+page.server.js`: Extract `accessToken` from `event.locals` in the `load` function and pass it to `fetchTeams()` and `fetchTeamOverview()`. Form actions already had this pattern. - `src/routes/teams/+page.server.js`: Accept `event` parameter in `load` (was previously bare `load()`), extract `accessToken`, and pass to both API calls. ## Test Plan - [ ] Log in as admin, navigate to `/admin/teams` -- teams list loads without 401 - [ ] Navigate to `/teams` -- teams load with or without active session - [ ] Verify `/coach` page still works (no regression -- already correct) - [ ] Verify form actions on `/admin/teams` (create team, assign players, etc.) still work ## Review Checklist - [x] Passed automated review-fix loop - [x] No secrets committed - [x] No unnecessary file changes - [x] Commit messages are descriptive ## Related - Closes #18

forgejo_admin added 1 commit

2026-03-15 01:42:19 +00:00

fix: pass Authorization header in server-side team API fetch calls 73383639af

The load functions in /admin/teams and /teams were calling fetchTeams()
and fetchTeamOverview() without passing the Keycloak access token,
causing 401 responses from basketball-api. Updated api.js to accept an
optional token parameter on fetchTeams, fetchTeam, and fetchTeamOverview,
and wired the token through from event.locals.accessToken in both
page.server.js files — matching the pattern already used in /coach.

Closes #18

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

forgejo_admin commented

2026-03-15 01:42:42 +00:00

Author

Owner

Review: APPROVED

Reviewed the diff (3 files, +42/-11). All changes are correct and scoped to the bug:

api.js: fetchTeams, fetchTeam, fetchTeamOverview now accept optional token param, conditionally set Authorization: Bearer header. Backwards compatible.
/admin/teams/+page.server.js: Extracts event.locals.accessToken in load, passes to fetchTeams and fetchTeamOverview. Matches pattern already used in actions on the same file.
/teams/+page.server.js: Accepts event in load (was bare), extracts token, passes through.

No secrets, no unrelated changes, no regressions. Pattern matches /coach/+page.server.js exactly.

**Review: APPROVED** Reviewed the diff (3 files, +42/-11). All changes are correct and scoped to the bug: - `api.js`: `fetchTeams`, `fetchTeam`, `fetchTeamOverview` now accept optional `token` param, conditionally set `Authorization: Bearer` header. Backwards compatible. - `/admin/teams/+page.server.js`: Extracts `event.locals.accessToken` in `load`, passes to `fetchTeams` and `fetchTeamOverview`. Matches pattern already used in actions on the same file. - `/teams/+page.server.js`: Accepts `event` in `load` (was bare), extracts token, passes through. No secrets, no unrelated changes, no regressions. Pattern matches `/coach/+page.server.js` exactly.

forgejo_admin commented

2026-03-15 01:44:42 +00:00

Author

Owner

PR #19 Frontend Review

DOMAIN REVIEW

Accessibility: No UI changes in this PR -- all modifications are server-side +page.server.js and $lib/server/api.js. No a11y impact.

Performance: No performance concerns. The Authorization header is conditionally added only when a token exists, avoiding any unnecessary overhead. The Promise.all parallel fetch pattern is correctly preserved.

Responsive / UX: No template or component changes. The fix ensures team data actually loads for authenticated users, which is a UX improvement (previously users saw empty/error states due to 401s).

Design quality: N/A -- no visual changes.

Code quality: The fix is clean, minimal, and well-scoped. The optional token parameter added to fetchTeams(), fetchTeam(), and fetchTeamOverview() in api.js maintains full backwards compatibility. The pattern exactly mirrors the existing approach in createTeam(), updateTeam(), assignPlayers(), unassignPlayer(), and the /coach/+page.server.js load function. JSDoc annotations are correctly updated with the new parameter.

BLOCKERS

None. The fix is correct and addresses the root cause (missing Authorization header on server-side team API load functions).

NITS

@ts-ignore for event.locals.accessToken -- This pattern appears 25 times across the codebase. A proper App.Locals interface declaration in src/app.d.ts would eliminate all of these suppressions. Not introduced by this PR, but worth tracking as a follow-up issue.
/teams route has no auth guard -- The /teams/+page.server.js load function now passes the access token but does not check for a session or enforce any role. If the basketball-api /api/teams endpoint requires authentication, unauthenticated visitors will hit the catch block and see empty data with apiConnected: false. This is pre-existing behavior (not a regression from this PR), but worth noting: if the intent is that /teams is a public route, the token pass-through is harmless; if it requires auth, a redirect guard should be added. Consider opening a follow-up issue.
Header object style inconsistency -- The PR uses headers['Authorization'] = ... (bracket notation) while the existing mutating functions (createTeam, updateTeam, etc.) use Authorization: ... (object literal shorthand). Minor inconsistency, not blocking.

SOP COMPLIANCE

Branch named after issue (18-bug-team-api-calls-return-401-server-sid references issue #18)
PR body has Summary, Changes, Test Plan, Related sections
PR body has Design Decisions section -- replaced with "Review Checklist" (acceptable for a bug fix with no design decisions, but template deviation noted)
Closes #18 present in Related section
Related section references plan slug -- plan-2026-03-08-tryout-prep is not mentioned in the PR body
No secrets, .env files, or credentials committed
No unnecessary file changes (3 files, tightly scoped to the bug)
Commit messages are descriptive (fix: pass Authorization header in server-side team API calls)

PROCESS OBSERVATIONS

Missing plan reference: The PR body Related section should reference plan-2026-03-08-tryout-prep for traceability. This is a minor SOP gap.
No automated tests: The test plan is manual-only (browser verification). There are no unit or integration tests for the api.js functions. This is a pre-existing gap -- the entire $lib/server/api.js module has no test coverage. Not blocking for a 3-file bug fix, but the pattern of shipping server-side API wrappers without tests increases change failure risk over time.
@ts-ignore proliferation: 25 occurrences across 10 files. A single App.Locals type declaration would eliminate these. Candidate for a follow-up issue or convention.

VERDICT: APPROVED

The fix is correct, minimal, backwards-compatible, and follows the established codebase pattern. It resolves the 401 bug for /admin/teams and /teams routes. The two SOP gaps (missing Design Decisions section and missing plan slug reference) are non-blocking for a bug fix PR. The nits are pre-existing patterns, not regressions.

## PR #19 Frontend Review ### DOMAIN REVIEW **Accessibility**: No UI changes in this PR -- all modifications are server-side `+page.server.js` and `$lib/server/api.js`. No a11y impact. **Performance**: No performance concerns. The `Authorization` header is conditionally added only when a token exists, avoiding any unnecessary overhead. The `Promise.all` parallel fetch pattern is correctly preserved. **Responsive / UX**: No template or component changes. The fix ensures team data actually loads for authenticated users, which is a UX improvement (previously users saw empty/error states due to 401s). **Design quality**: N/A -- no visual changes. **Code quality**: The fix is clean, minimal, and well-scoped. The optional `token` parameter added to `fetchTeams()`, `fetchTeam()`, and `fetchTeamOverview()` in `api.js` maintains full backwards compatibility. The pattern exactly mirrors the existing approach in `createTeam()`, `updateTeam()`, `assignPlayers()`, `unassignPlayer()`, and the `/coach/+page.server.js` load function. JSDoc annotations are correctly updated with the new parameter. ### BLOCKERS None. The fix is correct and addresses the root cause (missing `Authorization` header on server-side team API load functions). ### NITS 1. **`@ts-ignore` for `event.locals.accessToken`** -- This pattern appears 25 times across the codebase. A proper `App.Locals` interface declaration in `src/app.d.ts` would eliminate all of these suppressions. Not introduced by this PR, but worth tracking as a follow-up issue. 2. **`/teams` route has no auth guard** -- The `/teams/+page.server.js` load function now passes the access token but does not check for a session or enforce any role. If the basketball-api `/api/teams` endpoint requires authentication, unauthenticated visitors will hit the catch block and see empty data with `apiConnected: false`. This is pre-existing behavior (not a regression from this PR), but worth noting: if the intent is that `/teams` is a public route, the token pass-through is harmless; if it requires auth, a redirect guard should be added. Consider opening a follow-up issue. 3. **Header object style inconsistency** -- The PR uses `headers['Authorization'] = ...` (bracket notation) while the existing mutating functions (`createTeam`, `updateTeam`, etc.) use `Authorization: ...` (object literal shorthand). Minor inconsistency, not blocking. ### SOP COMPLIANCE - [x] Branch named after issue (`18-bug-team-api-calls-return-401-server-sid` references issue #18) - [x] PR body has Summary, Changes, Test Plan, Related sections - [ ] PR body has Design Decisions section -- replaced with "Review Checklist" (acceptable for a bug fix with no design decisions, but template deviation noted) - [x] `Closes #18` present in Related section - [ ] Related section references plan slug -- `plan-2026-03-08-tryout-prep` is not mentioned in the PR body - [x] No secrets, .env files, or credentials committed - [x] No unnecessary file changes (3 files, tightly scoped to the bug) - [x] Commit messages are descriptive (`fix: pass Authorization header in server-side team API calls`) ### PROCESS OBSERVATIONS - **Missing plan reference**: The PR body Related section should reference `plan-2026-03-08-tryout-prep` for traceability. This is a minor SOP gap. - **No automated tests**: The test plan is manual-only (browser verification). There are no unit or integration tests for the `api.js` functions. This is a pre-existing gap -- the entire `$lib/server/api.js` module has no test coverage. Not blocking for a 3-file bug fix, but the pattern of shipping server-side API wrappers without tests increases change failure risk over time. - **`@ts-ignore` proliferation**: 25 occurrences across 10 files. A single `App.Locals` type declaration would eliminate these. Candidate for a follow-up issue or convention. ### VERDICT: APPROVED The fix is correct, minimal, backwards-compatible, and follows the established codebase pattern. It resolves the 401 bug for `/admin/teams` and `/teams` routes. The two SOP gaps (missing Design Decisions section and missing plan slug reference) are non-blocking for a bug fix PR. The nits are pre-existing patterns, not regressions.