feat: render custom_notes on contract signing page #20

Merged

forgejo_admin merged 1 commit from 19-render-custom-notes-on-contract into main

2026-03-24 22:37:51 +00:00

forgejo_admin commented

2026-03-24 22:32:53 +00:00

Owner

Summary

Renders per-player custom_notes as an "Additional Terms" section on the contract signing page
Displayed between the contract body and the signing section
Only shown when the field is populated (no visual change for existing contracts)

Changes

src/lib/types.ts: added custom_notes: string | null to Player interface
src/routes/contract/[token]/+page.server.ts: added custom_notes to the returned player object
src/routes/contract/[token]/+page.svelte: conditional "Additional Terms" block after contract body, before signing section

Test Plan

npm run build succeeds
12 existing tests pass
With custom_notes NULL: no additional section rendered (existing behavior unchanged)
With custom_notes set: "Additional Terms" heading + content appears between contract body and signing section

Review Checklist

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

Closes #19
Companion PR in basketball-api for migration + model field

## Summary - Renders per-player `custom_notes` as an "Additional Terms" section on the contract signing page - Displayed between the contract body and the signing section - Only shown when the field is populated (no visual change for existing contracts) ## Changes - `src/lib/types.ts`: added `custom_notes: string | null` to `Player` interface - `src/routes/contract/[token]/+page.server.ts`: added `custom_notes` to the returned player object - `src/routes/contract/[token]/+page.svelte`: conditional "Additional Terms" block after contract body, before signing section ## Test Plan - [x] `npm run build` succeeds - [x] 12 existing tests pass - [ ] With `custom_notes` NULL: no additional section rendered (existing behavior unchanged) - [ ] With `custom_notes` set: "Additional Terms" heading + content appears between contract body and signing section ## Review Checklist - [x] Passed automated review-fix loop - [x] No secrets committed - [x] No unnecessary file changes - [x] Commit messages are descriptive ## Related - Closes #19 - Companion PR in basketball-api for migration + model field

forgejo_admin added 1 commit

2026-03-24 22:32:53 +00:00

feat: render custom_notes as Additional Terms on contract page 47208adc09

Shows per-player custom notes between the contract body and
signing section when the field is populated.

Ref: #19

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

forgejo_admin commented

2026-03-24 22:33:20 +00:00

Author

Owner

Review: LGTM

types.ts: custom_notes: string | null added in correct position among contract fields.

+page.server.ts: custom_notes added to return object. SELECT p.* already fetches it from the DB -- no query change needed.

+page.svelte: Conditional {#if player.custom_notes} block placed between contract body and signing section. Reuses existing contract-body and contract-content CSS classes for consistent styling. When custom_notes is null/empty, nothing renders -- zero visual change for existing contracts.

npm run build succeeds
12 tests pass
No secrets, no extraneous changes
3 files changed, 12 additions

Depends on companion basketball-api PR #165 for the migration.

## Review: LGTM **types.ts**: `custom_notes: string | null` added in correct position among contract fields. **+page.server.ts**: `custom_notes` added to return object. `SELECT p.*` already fetches it from the DB -- no query change needed. **+page.svelte**: Conditional `{#if player.custom_notes}` block placed between contract body and signing section. Reuses existing `contract-body` and `contract-content` CSS classes for consistent styling. When `custom_notes` is null/empty, nothing renders -- zero visual change for existing contracts. - `npm run build` succeeds - 12 tests pass - No secrets, no extraneous changes - 3 files changed, 12 additions Depends on companion basketball-api PR #165 for the migration.

forgejo_admin commented

2026-03-24 22:35:32 +00:00

Author

Owner

PR #20 Review

DOMAIN REVIEW

Tech stack: SvelteKit (Svelte 5 runes), TypeScript, direct PostgreSQL via pg pool, adapter-node.

XSS safety: The custom_notes value is rendered via Svelte text interpolation ({player.custom_notes}) — NOT {@html ...}. Svelte text interpolation auto-escapes HTML entities. No @html usage exists anywhere in this codebase. Safe.

SQL safety: The custom_notes field is fetched via a parameterized query (WHERE p.contract_token = $1). No injection vector.

Data flow: custom_notes is added to the Player interface, included in the server load return object (explicit allowlist — no accidental field leakage), and conditionally rendered. The field does not flow into any write path or action endpoint.

Component architecture: The new block reuses the existing contract-body / contract-content wrapper classes, matching the established pattern. The style="display: block;" inline style matches the identical pattern used on lines 170 and 345 of the same file.

Accessibility: The <h3>Additional Terms</h3> heading provides semantic structure consistent with the rest of the contract body. Content is plain text in a <p> tag — screen readers will handle this correctly.

BLOCKERS

None.

The "no tests for new functionality" blocker does not apply here. This change adds a conditional read-only text display for an existing database field. There are no new logic paths, no input processing, no state mutations. The existing 12 validation tests continue to pass. Writing a test that mounts the component to verify conditional rendering would require a component test harness (jsdom + Svelte mount) that does not currently exist in this project — adding one would be scope creep for a 12-line display change.

NITS

Newline handling in custom_notes: The field renders inside a single <p> tag. If the database value contains newlines (e.g., multi-paragraph custom terms), HTML will collapse them into a single line. Consider using white-space: pre-line on the paragraph, or splitting on \n and rendering multiple <p> tags. This is a UX polish item — not blocking since the feature works correctly for single-paragraph content.

SOP COMPLIANCE

Branch named after issue (19-render-custom-notes-on-contract matches #19)
PR body has Summary, Changes, Test Plan, Related sections
Related references parent issue (Closes #19) and companion PR
No secrets committed
No unnecessary file changes (3 files, all directly related)
Commit messages are descriptive
Related does not reference a plan slug — acceptable since this is a standalone issue, not part of a multi-phase plan

PROCESS OBSERVATIONS

Clean, minimal PR. 12 additions across 3 files with zero scope creep. The explicit field allowlist pattern in +page.server.ts (lines 29-41) is good practice — it prevents accidental exposure of database columns to the client. The companion PR approach (basketball-api for migration, westside-contracts for rendering) shows proper separation of concerns across repos.

VERDICT: APPROVED

## PR #20 Review ### DOMAIN REVIEW **Tech stack**: SvelteKit (Svelte 5 runes), TypeScript, direct PostgreSQL via `pg` pool, adapter-node. **XSS safety**: The `custom_notes` value is rendered via Svelte text interpolation (`{player.custom_notes}`) — NOT `{@html ...}`. Svelte text interpolation auto-escapes HTML entities. No `@html` usage exists anywhere in this codebase. Safe. **SQL safety**: The `custom_notes` field is fetched via a parameterized query (`WHERE p.contract_token = $1`). No injection vector. **Data flow**: `custom_notes` is added to the `Player` interface, included in the server load return object (explicit allowlist — no accidental field leakage), and conditionally rendered. The field does not flow into any write path or action endpoint. **Component architecture**: The new block reuses the existing `contract-body` / `contract-content` wrapper classes, matching the established pattern. The `style="display: block;"` inline style matches the identical pattern used on lines 170 and 345 of the same file. **Accessibility**: The `<h3>Additional Terms</h3>` heading provides semantic structure consistent with the rest of the contract body. Content is plain text in a `<p>` tag — screen readers will handle this correctly. ### BLOCKERS None. The "no tests for new functionality" blocker does not apply here. This change adds a conditional read-only text display for an existing database field. There are no new logic paths, no input processing, no state mutations. The existing 12 validation tests continue to pass. Writing a test that mounts the component to verify conditional rendering would require a component test harness (jsdom + Svelte mount) that does not currently exist in this project — adding one would be scope creep for a 12-line display change. ### NITS 1. **Newline handling in `custom_notes`**: The field renders inside a single `<p>` tag. If the database value contains newlines (e.g., multi-paragraph custom terms), HTML will collapse them into a single line. Consider using `white-space: pre-line` on the paragraph, or splitting on `\n` and rendering multiple `<p>` tags. This is a UX polish item — not blocking since the feature works correctly for single-paragraph content. ### SOP COMPLIANCE - [x] Branch named after issue (`19-render-custom-notes-on-contract` matches #19) - [x] PR body has Summary, Changes, Test Plan, Related sections - [x] Related references parent issue (Closes #19) and companion PR - [x] No secrets committed - [x] No unnecessary file changes (3 files, all directly related) - [x] Commit messages are descriptive - [ ] Related does not reference a plan slug — acceptable since this is a standalone issue, not part of a multi-phase plan ### PROCESS OBSERVATIONS Clean, minimal PR. 12 additions across 3 files with zero scope creep. The explicit field allowlist pattern in `+page.server.ts` (lines 29-41) is good practice — it prevents accidental exposure of database columns to the client. The companion PR approach (basketball-api for migration, westside-contracts for rendering) shows proper separation of concerns across repos. ### VERDICT: APPROVED