feat: update jersey/checkout pages to support session auth (#198) #200

Merged

forgejo_admin merged 1 commit from 198-jersey-checkout-session-auth into main 2026-03-30 21:40:02 +00:00

Conversation 2 Commits 1 Files changed 2 +111 -16

forgejo_admin commented 2026-03-30 21:06:31 +00:00

Contributor

Copy link

Summary

Add dual-auth support to jersey and checkout pages so logged-in parents can access them via ?player_id= with Keycloak session auth, while preserving the existing ?token= email-link flow unchanged.

Changes

• src/routes/(app)/jersey/+page.svelte — import ready, isAuthenticated, getToken from keycloak.js; add playerId state and authMode derived signal; branch onMount to handle ?player_id= with Bearer auth or ?token= with raw fetch; update handleSelect to build checkout URL and headers based on auth mode; redirect to /signin if session mode and not authenticated
• src/routes/(app)/checkout/+page.svelte — same dual-auth pattern: add playerId/authMode; branch onMount for session vs token mode; update handleSubmit to use Bearer auth in session mode; redirect to /signin when unauthenticated with ?player_id=

Test Plan

• Visit /jersey?player_id=5 while logged in — jersey page loads using Keycloak session auth
• Visit /jersey?token=abc123 — existing token flow works unchanged
• Visit /jersey?player_id=5 while NOT logged in — redirects to /signin
• Visit /checkout?player_id=5&category=jersey while logged in — checkout works with session auth
• Visit /checkout?token=abc123&category=jersey — existing token flow works unchanged
• npm run build passes (verified)

Review Checklist

• Only target files modified (jersey/+page.svelte, checkout/+page.svelte)
• Existing token-based flow unchanged
• Session mode uses Bearer auth via getToken()
• Unauthenticated session-mode access redirects to /signin
• npm run build passes with no new warnings
• No unrelated changes

Related Notes

• Closes #198
• Depends on: basketball-api PR #258 (dual-auth on backend, merged)
• Parent: #196 (spike: player self-service jersey ordering)

## Summary Add dual-auth support to jersey and checkout pages so logged-in parents can access them via `?player_id=` with Keycloak session auth, while preserving the existing `?token=` email-link flow unchanged. ## Changes - `src/routes/(app)/jersey/+page.svelte` — import `ready`, `isAuthenticated`, `getToken` from keycloak.js; add `playerId` state and `authMode` derived signal; branch onMount to handle `?player_id=` with Bearer auth or `?token=` with raw fetch; update `handleSelect` to build checkout URL and headers based on auth mode; redirect to `/signin` if session mode and not authenticated - `src/routes/(app)/checkout/+page.svelte` — same dual-auth pattern: add `playerId`/`authMode`; branch onMount for session vs token mode; update `handleSubmit` to use Bearer auth in session mode; redirect to `/signin` when unauthenticated with `?player_id=` ## Test Plan - Visit `/jersey?player_id=5` while logged in — jersey page loads using Keycloak session auth - Visit `/jersey?token=abc123` — existing token flow works unchanged - Visit `/jersey?player_id=5` while NOT logged in — redirects to `/signin` - Visit `/checkout?player_id=5&category=jersey` while logged in — checkout works with session auth - Visit `/checkout?token=abc123&category=jersey` — existing token flow works unchanged - `npm run build` passes (verified) ## Review Checklist - [x] Only target files modified (jersey/+page.svelte, checkout/+page.svelte) - [x] Existing token-based flow unchanged - [x] Session mode uses Bearer auth via `getToken()` - [x] Unauthenticated session-mode access redirects to `/signin` - [x] `npm run build` passes with no new warnings - [x] No unrelated changes ## Related Notes - Closes #198 - Depends on: basketball-api PR #258 (dual-auth on backend, merged) - Parent: #196 (spike: player self-service jersey ordering)

forgejo_admin added 1 commit 2026-03-30 21:06:31 +00:00

feat: update jersey/checkout pages to support session auth (#198) ... 7ad9b8c131

Add dual-auth support to jersey and checkout pages. When visited with
?player_id= by a Keycloak-authenticated user, pages use Bearer token
via getToken() for API calls. When visited with ?token= (email links),
the existing token-based flow works unchanged. Unauthenticated access
with ?player_id= redirects to /signin.

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

forgejo_admin commented 2026-03-30 21:07:33 +00:00

Author

Contributor

Copy link

QA Review -- PR #200

Scope Check

• Only 2 files modified: jersey/+page.svelte and checkout/+page.svelte (matches ticket #198 file targets)
• No unrelated changes
• npm run build passes per PR description

Code Quality

Auth mode detection -- clean $derived signal: token ? 'token' : (playerId ? 'session' : null). Token takes precedence if both params present (correct -- email links must always work).

Keycloak ready gate -- both files await ready before isAuthenticated() in session mode. Matches the established pattern in billing/+page.svelte.

Bearer token injection -- uses getToken() which handles auto-refresh. Correct.

Session storage -- correctly guarded with if (token) to avoid pollution in session mode.

Template guards -- {:else if !authMode} correctly replaces {:else if !token} for the "Invalid Link" error state.

Function guards -- handleSelect and handleSubmit check !authMode instead of !token. Correct.

Nits (non-blocking)

1. Minor inconsistency: Jersey page has a reusable sessionFetchOpts() helper but also inlines Bearer header logic in handleSelect. Checkout page inlines everywhere. Not a bug -- just a style inconsistency. Could DRY up later.

2. sessionFetchOpts parameter name: The options parameter shadows the outer options state variable. No runtime issue (different scope), but could be renamed to fetchOptions for clarity in a future pass.

Acceptance Criteria Verification

• /jersey?player_id=5 while logged in -- uses Bearer auth via getToken()
• /jersey?token=abc123 -- existing flow unchanged (raw fetch, no Keycloak)
• /jersey?player_id=5 while NOT logged in -- goto('/signin')
• /checkout?player_id=5&category=jersey while logged in -- session auth with Bearer
• /checkout?token=abc123&category=jersey -- existing flow unchanged
• API calls in session mode use Bearer token from Keycloak

VERDICT: APPROVED

Clean, focused, additive change. Both auth paths are correctly branched with no regression risk to the existing token flow. Nits are cosmetic only.

## QA Review -- PR #200 ### Scope Check - [x] Only 2 files modified: `jersey/+page.svelte` and `checkout/+page.svelte` (matches ticket #198 file targets) - [x] No unrelated changes - [x] `npm run build` passes per PR description ### Code Quality **Auth mode detection** -- clean `$derived` signal: `token ? 'token' : (playerId ? 'session' : null)`. Token takes precedence if both params present (correct -- email links must always work). **Keycloak ready gate** -- both files `await ready` before `isAuthenticated()` in session mode. Matches the established pattern in `billing/+page.svelte`. **Bearer token injection** -- uses `getToken()` which handles auto-refresh. Correct. **Session storage** -- correctly guarded with `if (token)` to avoid pollution in session mode. **Template guards** -- `{:else if !authMode}` correctly replaces `{:else if !token}` for the "Invalid Link" error state. **Function guards** -- `handleSelect` and `handleSubmit` check `!authMode` instead of `!token`. Correct. ### Nits (non-blocking) 1. **Minor inconsistency**: Jersey page has a reusable `sessionFetchOpts()` helper but also inlines Bearer header logic in `handleSelect`. Checkout page inlines everywhere. Not a bug -- just a style inconsistency. Could DRY up later. 2. **`sessionFetchOpts` parameter name**: The `options` parameter shadows the outer `options` state variable. No runtime issue (different scope), but could be renamed to `fetchOptions` for clarity in a future pass. ### Acceptance Criteria Verification - [x] `/jersey?player_id=5` while logged in -- uses Bearer auth via `getToken()` - [x] `/jersey?token=abc123` -- existing flow unchanged (raw fetch, no Keycloak) - [x] `/jersey?player_id=5` while NOT logged in -- `goto('/signin')` - [x] `/checkout?player_id=5&category=jersey` while logged in -- session auth with Bearer - [x] `/checkout?token=abc123&category=jersey` -- existing flow unchanged - [x] API calls in session mode use Bearer token from Keycloak ### VERDICT: APPROVED Clean, focused, additive change. Both auth paths are correctly branched with no regression risk to the existing token flow. Nits are cosmetic only.

forgejo_admin commented 2026-03-30 21:10:20 +00:00

Author

Contributor

Copy link

PR #200 Review

DOMAIN REVIEW

Tech stack: SvelteKit (Svelte 5 runes: $state, $derived), vanilla fetch, Keycloak-js OIDC client.

Auth flow analysis: The dual-auth pattern is correctly implemented. Token mode (email links with ?token=) is preserved exactly as-is. Session mode (logged-in users with ?player_id=) correctly awaits Keycloak init, checks isAuthenticated(), and redirects to /signin when unauthenticated. Bearer tokens are attached to session-mode API calls via getToken().

Backwards compatibility: Confirmed. The token path through both onMount flows is unchanged. The $derived authMode signal correctly prioritizes token over playerId (token takes precedence if both are present), which is the right call -- an email link should always use the token path.

SvelteKit patterns: $state, $derived, onMount lifecycle are all used correctly. Reactive derivation of authMode from token/playerId is clean.

Scope: Only the 2 target files are modified. No scope creep.

BLOCKERS

None. No secrets committed, no unvalidated input leading to injection, auth logic is correct.

NITS

NIT 1: DRY -- manual Bearer construction vs existing apiFetch

$lib/api.js exports apiFetch() which already handles await ready, isAuthenticated() check, getToken(), and Bearer header construction. It is used in 12 other pages in this codebase. The PR instead manually constructs Bearer headers in 3 separate locations across 2 files:

• jersey/+page.svelte line ~116: sessionFetchOpts() helper (GET player-info)
• jersey/+page.svelte line ~256: inline Bearer construction (POST checkout)
• checkout/+page.svelte line ~113: inline Bearer construction (POST create-session)

For session-mode calls, apiFetch would work directly:

• GET: apiFetch('/jersey/player-info?player_id=' + playerId)
• POST: apiFetch('/jersey/checkout?player_id=' + playerId, { method: 'POST', body: JSON.stringify(body) })

This would eliminate all 3 manual Bearer constructions and align with the rest of the codebase. The token-mode paths legitimately need raw fetch (token-in-URL, no Bearer), so those stay as-is.

NIT 2: Internal inconsistency in jersey page

The jersey page defines a sessionFetchOpts() helper for the GET request but does NOT use it for the POST request in handleSelect(), instead duplicating the Bearer logic inline. If the manual approach is kept (over NIT 1), at minimum sessionFetchOpts() should be reused in handleSelect().

NIT 3: getToken() null not handled as short-circuit

If getToken() returns null (token refresh failed), the code still proceeds with the fetch -- just without an Authorization header. This will 401 from the API. The getToken() implementation in keycloak.js calls login() on refresh failure (triggering a redirect) AND returns null, creating a race between the redirect and the fetch. Consider short-circuiting: if bearerToken is null, return early instead of sending an unauthenticated request.

In jersey sessionFetchOpts():

const bearerToken = await getToken();
if (bearerToken) headers['Authorization'] = `Bearer ${bearerToken}`;
// ^ silently continues without auth if null

In checkout handleSubmit():

if (bearerToken) {
    // sets header
}
// ^ silently continues without auth if null

NIT 4: player_id not URI-encoded

playerId from URL search params is interpolated directly into API URLs without encodeURIComponent(). Low risk since player_id is expected to be numeric, but the checkout page already encodes category with encodeURIComponent(category || 'jersey'), so this is inconsistent. For defense-in-depth:

`${API_BASE}/jersey/player-info?player_id=${encodeURIComponent(playerId)}`

SOP COMPLIANCE

• Branch named after issue: 198-jersey-checkout-session-auth matches issue #198
• PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related Notes all present
• Related references parent: References #198, spike #196, and basketball-api PR #258
• No secrets committed
• No unrelated changes (only 2 files modified, both target files)
• Tests exist: No tests, but the entire repo has no test infrastructure -- this is pre-existing technical debt, not introduced by this PR

PROCESS OBSERVATIONS

• Change failure risk: Low. The dual-auth pattern is straightforward and the token-mode path is preserved. The main risk is the getToken() null race (NIT 3), which would manifest as a flash of error before redirect -- not data loss.
• DRY divergence risk: The manual Bearer construction (NIT 1) creates 3 places that must stay in sync with apiFetch if auth patterns change. This is the most impactful nit for long-term maintainability.
• Test debt: This repo has zero test infrastructure. Every PR adds to the untested surface area. Not a blocker for this PR, but a growing risk.

VERDICT: APPROVED

The auth logic is correct, backwards compatibility is preserved, and scope is tight. The nits (especially NIT 1 on DRY and NIT 3 on getToken null handling) are worth addressing but are not blockers.

## PR #200 Review ### DOMAIN REVIEW **Tech stack**: SvelteKit (Svelte 5 runes: `$state`, `$derived`), vanilla `fetch`, Keycloak-js OIDC client. **Auth flow analysis**: The dual-auth pattern is correctly implemented. Token mode (email links with `?token=`) is preserved exactly as-is. Session mode (logged-in users with `?player_id=`) correctly awaits Keycloak init, checks `isAuthenticated()`, and redirects to `/signin` when unauthenticated. Bearer tokens are attached to session-mode API calls via `getToken()`. **Backwards compatibility**: Confirmed. The `token` path through both `onMount` flows is unchanged. The `$derived` authMode signal correctly prioritizes `token` over `playerId` (token takes precedence if both are present), which is the right call -- an email link should always use the token path. **SvelteKit patterns**: `$state`, `$derived`, `onMount` lifecycle are all used correctly. Reactive derivation of `authMode` from `token`/`playerId` is clean. **Scope**: Only the 2 target files are modified. No scope creep. ### BLOCKERS None. No secrets committed, no unvalidated input leading to injection, auth logic is correct. ### NITS **NIT 1: DRY -- manual Bearer construction vs existing `apiFetch`** `$lib/api.js` exports `apiFetch()` which already handles `await ready`, `isAuthenticated()` check, `getToken()`, and Bearer header construction. It is used in 12 other pages in this codebase. The PR instead manually constructs Bearer headers in 3 separate locations across 2 files: - `jersey/+page.svelte` line ~116: `sessionFetchOpts()` helper (GET player-info) - `jersey/+page.svelte` line ~256: inline Bearer construction (POST checkout) - `checkout/+page.svelte` line ~113: inline Bearer construction (POST create-session) For session-mode calls, `apiFetch` would work directly: - GET: `apiFetch('/jersey/player-info?player_id=' + playerId)` - POST: `apiFetch('/jersey/checkout?player_id=' + playerId, { method: 'POST', body: JSON.stringify(body) })` This would eliminate all 3 manual Bearer constructions and align with the rest of the codebase. The token-mode paths legitimately need raw `fetch` (token-in-URL, no Bearer), so those stay as-is. **NIT 2: Internal inconsistency in jersey page** The jersey page defines a `sessionFetchOpts()` helper for the GET request but does NOT use it for the POST request in `handleSelect()`, instead duplicating the Bearer logic inline. If the manual approach is kept (over NIT 1), at minimum `sessionFetchOpts()` should be reused in `handleSelect()`. **NIT 3: `getToken()` null not handled as short-circuit** If `getToken()` returns `null` (token refresh failed), the code still proceeds with the fetch -- just without an Authorization header. This will 401 from the API. The `getToken()` implementation in `keycloak.js` calls `login()` on refresh failure (triggering a redirect) AND returns null, creating a race between the redirect and the fetch. Consider short-circuiting: if `bearerToken` is null, return early instead of sending an unauthenticated request. In jersey `sessionFetchOpts()`: ```js const bearerToken = await getToken(); if (bearerToken) headers['Authorization'] = `Bearer ${bearerToken}`; // ^ silently continues without auth if null ``` In checkout `handleSubmit()`: ```js if (bearerToken) { // sets header } // ^ silently continues without auth if null ``` **NIT 4: `player_id` not URI-encoded** `playerId` from URL search params is interpolated directly into API URLs without `encodeURIComponent()`. Low risk since player_id is expected to be numeric, but the checkout page already encodes `category` with `encodeURIComponent(category || 'jersey')`, so this is inconsistent. For defense-in-depth: ```js `${API_BASE}/jersey/player-info?player_id=${encodeURIComponent(playerId)}` ``` ### SOP COMPLIANCE - [x] Branch named after issue: `198-jersey-checkout-session-auth` matches issue #198 - [x] PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related Notes all present - [x] Related references parent: References #198, spike #196, and basketball-api PR #258 - [x] No secrets committed - [x] No unrelated changes (only 2 files modified, both target files) - [ ] Tests exist: No tests, but the entire repo has no test infrastructure -- this is pre-existing technical debt, not introduced by this PR ### PROCESS OBSERVATIONS - **Change failure risk**: Low. The dual-auth pattern is straightforward and the token-mode path is preserved. The main risk is the `getToken()` null race (NIT 3), which would manifest as a flash of error before redirect -- not data loss. - **DRY divergence risk**: The manual Bearer construction (NIT 1) creates 3 places that must stay in sync with `apiFetch` if auth patterns change. This is the most impactful nit for long-term maintainability. - **Test debt**: This repo has zero test infrastructure. Every PR adds to the untested surface area. Not a blocker for this PR, but a growing risk. ### VERDICT: APPROVED The auth logic is correct, backwards compatibility is preserved, and scope is tight. The nits (especially NIT 1 on DRY and NIT 3 on getToken null handling) are worth addressing but are not blockers.

forgejo_admin merged commit 0cc37992f1 into main 2026-03-30 21:40:02 +00:00

forgejo_admin deleted branch 198-jersey-checkout-session-auth 2026-03-30 21:40:02 +00:00

forgejo_admin referenced this pull request from a commit 2026-03-30 21:40:02 +00:00

feat: update jersey/checkout pages to support session auth (#198) (#200)

forgejo_admin referenced this pull request 2026-03-30 21:40:26 +00:00

Clean up QA nits from frontend session auth PR #200 #201

forgejo_admin referenced this pull request 2026-03-30 21:49:39 +00:00

feat: add jersey order card to player profile page (#197) #202

forgejo_admin referenced this pull request 2026-03-30 21:54:15 +00:00

feat: add jersey order card to player profile page (#197) #202

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

Subscribe

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

ldraney/westside-app!200

No description provided.