feat: contract signing UI + login redirect fix

forgejo_admin commented

2026-03-18 14:57:32 +00:00

Author

Owner

PR #40 Review

DOMAIN REVIEW

Tech stack: SvelteKit (Svelte 5 runes), CSS (global app.css with design tokens), Keycloak-js auth.

4 files changed, 184 additions, 23 deletions.

Login redirect fix (src/lib/keycloak.js): Correct. The hardcoded redirectUri to /my-players is replaced with /. The home page (src/routes/+page.svelte) already handles authenticated users by calling getRoleRedirectPath(), which routes admin to /admin, coach to /coach, and player to /my-players. This properly delegates redirect logic to the existing function instead of hardcoding a single role's destination.

Contract offer card (src/routes/my-players/+page.svelte): Clean implementation. Players with status === 'offered' get a prominent yellow-bordered CTA card instead of the regular card row. The {#if}/{:else} branching is well-structured. Mock data is gated behind ?mock=offered query param, clearly marked with a comment.

Contract signing page (src/routes/players/[id]/+page.svelte): Full 7-section contract with checkbox acceptance + typed digital signature. State management is clean -- contractAccepted, contractSignature, contractEnabled (derived), contractSubmitting, contractSigned. The handleSignContract() function properly guards with contractEnabled and contractSubmitting checks, POSTs to /players/{id}/contract with JSON body, and gracefully falls through to success on API error (intentional for pre-API demo phase).

CSS (src/app.css): All new styles use design tokens consistently (--color-yellow-*, --space-*, --radius-*). No magic numbers. Yellow theme tokens are pre-existing. New classes follow the established naming convention (contract-card, contract-terms, contract-sign, contract-success).

Accessibility: Checkbox has proper id="contract-check" / for="contract-check" pairing. Signature input has a visible label. Button has disabled binding. Contract terms area has max-height: 400px; overflow-y: auto for scrollability on small screens.

BLOCKERS

None.

Test coverage: No test framework is configured in this project (package.json has no vitest, playwright, or jest dependency). Zero tests exist across the entire codebase. This PR cannot be held to a standard the project does not yet have. The PR body documents a manual Test Plan with 6 verification steps and notes Playwright screenshot validation. Not blocking, but the project will need a test framework eventually.

Security: No secrets committed. Signature data is sent as a JSON field via JSON.stringify, not interpolated into HTML or URLs. The apiFetch helper properly attaches Bearer tokens. No XSS or injection vectors.

NITS

Unused import: { page } is imported from $app/stores in src/routes/my-players/+page.svelte (line 3) but never used. The mock query param is read via window.location.search directly. Remove the import or use $page.url.searchParams instead.
Catch-swallows-error in handleSignContract(): The catch block at line 130 silently succeeds on API failure (contractSigned = true; player.status = 'active'). This is documented as intentional for demo phase, but the comment should include a TODO or link to the issue that will wire the real endpoint, so it doesn't survive into production.
Hardcoded mock ID: Player ID 999 is used as the mock trigger in +page.svelte (line 46: if (id === '999')). If the API ever assigns ID 999 to a real player, the mock would intercept. Consider using a non-numeric sentinel or environment-gated flag instead. Low risk now, but worth a TODO.
Repeated $200/month text: The dollar amount appears in the contract terms (section 1), the checkbox label, and the success message. When billing tiers become dynamic (Phase 14 scope), these will need to be driven by data. Not blocking for a UI-first phase, but noting for traceability.
badge-offered duplicates badge-pending styling: Both classes at lines 283 and 288 of app.css have identical background, color, and border values. Consider whether these should share a base class or if the duplication is intentional (semantic separation for future divergence).

SOP COMPLIANCE

Branch named after issue: 39-contract-flow-ui references issue #39
PR body has: Summary, Changes, Test Plan, Related
Related references plan slug: plan-wkq Phase 14
No secrets committed
No unnecessary file changes -- all 4 files are directly related to the contract flow and login fix
Commit message is descriptive: feat: contract signing UI + login redirect fix
Review Checklist in PR body is thorough

PROCESS OBSERVATIONS

Deployment frequency: This is a UI-first feature with mock data, meaning it can ship and iterate without backend dependency. Good for deployment frequency.
Change failure risk: Low. The login redirect fix is the only behavioral change to existing code, and the redirect chain (/ -> getRoleRedirectPath()) is well-established. All contract UI is additive and gated behind status === 'offered'.
Documentation: PR body is comprehensive with a 6-step manual test plan and screenshot references. The mock data approach is clearly documented with comments and query-param gating.

VERDICT: APPROVED

## PR #40 Review ### DOMAIN REVIEW **Tech stack**: SvelteKit (Svelte 5 runes), CSS (global app.css with design tokens), Keycloak-js auth. **4 files changed, 184 additions, 23 deletions.** **Login redirect fix** (`src/lib/keycloak.js`): Correct. The hardcoded `redirectUri` to `/my-players` is replaced with `/`. The home page (`src/routes/+page.svelte`) already handles authenticated users by calling `getRoleRedirectPath()`, which routes admin to `/admin`, coach to `/coach`, and player to `/my-players`. This properly delegates redirect logic to the existing function instead of hardcoding a single role's destination. **Contract offer card** (`src/routes/my-players/+page.svelte`): Clean implementation. Players with `status === 'offered'` get a prominent yellow-bordered CTA card instead of the regular card row. The `{#if}/{:else}` branching is well-structured. Mock data is gated behind `?mock=offered` query param, clearly marked with a comment. **Contract signing page** (`src/routes/players/[id]/+page.svelte`): Full 7-section contract with checkbox acceptance + typed digital signature. State management is clean -- `contractAccepted`, `contractSignature`, `contractEnabled` (derived), `contractSubmitting`, `contractSigned`. The `handleSignContract()` function properly guards with `contractEnabled` and `contractSubmitting` checks, POSTs to `/players/{id}/contract` with JSON body, and gracefully falls through to success on API error (intentional for pre-API demo phase). **CSS** (`src/app.css`): All new styles use design tokens consistently (`--color-yellow-*`, `--space-*`, `--radius-*`). No magic numbers. Yellow theme tokens are pre-existing. New classes follow the established naming convention (`contract-card`, `contract-terms`, `contract-sign`, `contract-success`). **Accessibility**: Checkbox has proper `id="contract-check"` / `for="contract-check"` pairing. Signature input has a visible label. Button has `disabled` binding. Contract terms area has `max-height: 400px; overflow-y: auto` for scrollability on small screens. ### BLOCKERS None. **Test coverage**: No test framework is configured in this project (`package.json` has no vitest, playwright, or jest dependency). Zero tests exist across the entire codebase. This PR cannot be held to a standard the project does not yet have. The PR body documents a manual Test Plan with 6 verification steps and notes Playwright screenshot validation. Not blocking, but the project will need a test framework eventually. **Security**: No secrets committed. Signature data is sent as a JSON field via `JSON.stringify`, not interpolated into HTML or URLs. The `apiFetch` helper properly attaches Bearer tokens. No XSS or injection vectors. ### NITS 1. **Unused import**: `{ page }` is imported from `$app/stores` in `src/routes/my-players/+page.svelte` (line 3) but never used. The mock query param is read via `window.location.search` directly. Remove the import or use `$page.url.searchParams` instead. 2. **Catch-swallows-error in `handleSignContract()`**: The catch block at line 130 silently succeeds on API failure (`contractSigned = true; player.status = 'active'`). This is documented as intentional for demo phase, but the comment should include a TODO or link to the issue that will wire the real endpoint, so it doesn't survive into production. 3. **Hardcoded mock ID**: Player ID `999` is used as the mock trigger in `+page.svelte` (line 46: `if (id === '999')`). If the API ever assigns ID 999 to a real player, the mock would intercept. Consider using a non-numeric sentinel or environment-gated flag instead. Low risk now, but worth a TODO. 4. **Repeated `$200/month` text**: The dollar amount appears in the contract terms (section 1), the checkbox label, and the success message. When billing tiers become dynamic (Phase 14 scope), these will need to be driven by data. Not blocking for a UI-first phase, but noting for traceability. 5. **`badge-offered` duplicates `badge-pending` styling**: Both classes at lines 283 and 288 of `app.css` have identical `background`, `color`, and `border` values. Consider whether these should share a base class or if the duplication is intentional (semantic separation for future divergence). ### SOP COMPLIANCE - [x] Branch named after issue: `39-contract-flow-ui` references issue #39 - [x] PR body has: Summary, Changes, Test Plan, Related - [x] Related references plan slug: `plan-wkq` Phase 14 - [x] No secrets committed - [x] No unnecessary file changes -- all 4 files are directly related to the contract flow and login fix - [x] Commit message is descriptive: `feat: contract signing UI + login redirect fix` - [x] Review Checklist in PR body is thorough ### PROCESS OBSERVATIONS - **Deployment frequency**: This is a UI-first feature with mock data, meaning it can ship and iterate without backend dependency. Good for deployment frequency. - **Change failure risk**: Low. The login redirect fix is the only behavioral change to existing code, and the redirect chain (`/` -> `getRoleRedirectPath()`) is well-established. All contract UI is additive and gated behind `status === 'offered'`. - **Documentation**: PR body is comprehensive with a 6-step manual test plan and screenshot references. The mock data approach is clearly documented with comments and query-param gating. ### VERDICT: APPROVED

2026-03-18 14:55:15 +00:00

Summary

Add the Program Contract signing flow UI and fix the post-login redirect.

Closes #39

Changes

src/routes/players/[id]/+page.svelte — contract terms (7 sections), checkbox, digital signature, sign button. Status transitions offered→active on sign. Mock data for player ID 999.
src/routes/my-players/+page.svelte — "Team Offer" card with yellow badge and "Review & Sign Contract" CTA when player status is offered. Mock data via ?mock=offered query param.
src/app.css — .badge-offered, .contract-offer-card, .contract-card, .contract-terms, .contract-sign, .contract-success styles.
src/lib/keycloak.js — fix login redirect from hardcoded /my-players to / (home page handles role-based redirect via getRoleRedirectPath()).

Test Plan

Navigate to /my-players?mock=offered — see Team Offer card with CTA
Click CTA — lands on /players/999 with full contract
Check box + type signature — button enables
Click sign — badge flips to Active, success message appears, Payment card shows
Sign in as admin — redirects to /admin (not /my-players)
Sign in as coach — redirects to /coach

Review Checklist

No scoped style blocks — all CSS in app.css
Contract terms match Issue #39 assumptions
Mock data is clearly marked and query-param gated
Login redirect fix uses existing getRoleRedirectPath() function

plan-wkq → Phase 14 (Billing Tiers & Contracts)
phase-wkq-15-production-port — SPA rebuild context
Validated via Playwright: westside-contract-offer-card.png, westside-contract-signing-page.png, westside-contract-signed-success.png

## Summary Add the Program Contract signing flow UI and fix the post-login redirect. Closes #39 ## Changes - `src/routes/players/[id]/+page.svelte` — contract terms (7 sections), checkbox, digital signature, sign button. Status transitions offered→active on sign. Mock data for player ID 999. - `src/routes/my-players/+page.svelte` — "Team Offer" card with yellow badge and "Review & Sign Contract" CTA when player status is `offered`. Mock data via `?mock=offered` query param. - `src/app.css` — `.badge-offered`, `.contract-offer-card`, `.contract-card`, `.contract-terms`, `.contract-sign`, `.contract-success` styles. - `src/lib/keycloak.js` — fix login redirect from hardcoded `/my-players` to `/` (home page handles role-based redirect via `getRoleRedirectPath()`). ## Test Plan - [ ] Navigate to `/my-players?mock=offered` — see Team Offer card with CTA - [ ] Click CTA — lands on `/players/999` with full contract - [ ] Check box + type signature — button enables - [ ] Click sign — badge flips to Active, success message appears, Payment card shows - [ ] Sign in as admin — redirects to `/admin` (not `/my-players`) - [ ] Sign in as coach — redirects to `/coach` ## Review Checklist - [x] No scoped style blocks — all CSS in app.css - [x] Contract terms match Issue #39 assumptions - [x] Mock data is clearly marked and query-param gated - [x] Login redirect fix uses existing `getRoleRedirectPath()` function ## Related - `plan-wkq` → Phase 14 (Billing Tiers & Contracts) - `phase-wkq-15-production-port` — SPA rebuild context - Validated via Playwright: `westside-contract-offer-card.png`, `westside-contract-signing-page.png`, `westside-contract-signed-success.png`

forgejo_admin added 1 commit

feat: contract signing UI + login redirect fix 1d608c395b

- Add Program Contract section to /players/[id] — 7-section terms,
  checkbox, digital signature, "Sign Contract & Start Billing" button
- Add "Team Offer" card to /my-players with CTA linking to contract
- Add offered/active status badges and CSS (yellow border, green success)
- Fix login redirect: / instead of hardcoded /my-players — home page
  handles role-based redirect via getRoleRedirectPath()
- Add mock data (?mock=offered) for contract flow testing

Closes #39

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

forgejo_admin merged commit ae6d554efa into main

2026-03-18 15:04:31 +00:00

forgejo_admin deleted branch 39-contract-flow-ui

forgejo_admin referenced this pull request from a commit

feat: contract signing UI + login redirect fix (#40)

Rows
Columns

feat: contract signing UI + login redirect fix #40