ldraney/westside-contracts

Fork 0

feat: wire components to render from merged contract data (#37) #45

Merged

forgejo_admin merged 2 commits from 37-wire-components-to-data into main

2026-04-05 05:00:38 +00:00

forgejo_admin commented

2026-04-05 01:45:25 +00:00

Contributor

Summary

Replace the three hardcoded {#if} branches in +page.svelte with a new data-driven rendering path that reads from the merged config object. When data.config is non-null (team has contract_config populated), components render dynamically from the config. When null, the existing hardcoded branches serve as fallback.

Changes

src/routes/contract/[token]/+page.svelte — Added {#if data.config} data-driven rendering branch above the existing fallback. Updated Queens branding $effect to also check data.config?.variant === 'girls-travel'.
src/lib/contract-config.ts — Extended Tournament interface with dates, cost, local, travel, entry_fee, travel_note fields. Added TournamentTravel interface. Added optional to Practice. Renamed PaymentSchedule to PaymentScheduleItem with label/date fields. Added variant and static_content to ContractConfig and MergedConfig. Updated merge function to propagate new fields.
src/lib/types.ts — Updated re-export from PaymentSchedule to PaymentScheduleItem.

Test Plan

npm test — 30/30 tests pass
npm run check — zero new errors (12 pre-existing type errors in db.ts/minio.ts/server.ts unrelated to this change)
Verify fallback path: teams without contract_config still render the hardcoded contract variants
Verify data-driven path: teams with contract_config populated render from the merged config

Review Checklist

Tests pass (30/30)
Type check clean (no new errors)
Existing hardcoded branches preserved as fallback
Queens branding effect updated for data-driven variant
No changes to signing section, footer, or success overlay

None.

Closes #37

## Summary Replace the three hardcoded `{#if}` branches in +page.svelte with a new data-driven rendering path that reads from the merged config object. When `data.config` is non-null (team has `contract_config` populated), components render dynamically from the config. When null, the existing hardcoded branches serve as fallback. ## Changes - `src/routes/contract/[token]/+page.svelte` — Added `{#if data.config}` data-driven rendering branch above the existing fallback. Updated Queens branding `$effect` to also check `data.config?.variant === 'girls-travel'`. - `src/lib/contract-config.ts` — Extended `Tournament` interface with `dates`, `cost`, `local`, `travel`, `entry_fee`, `travel_note` fields. Added `TournamentTravel` interface. Added `optional` to `Practice`. Renamed `PaymentSchedule` to `PaymentScheduleItem` with `label`/`date` fields. Added `variant` and `static_content` to `ContractConfig` and `MergedConfig`. Updated merge function to propagate new fields. - `src/lib/types.ts` — Updated re-export from `PaymentSchedule` to `PaymentScheduleItem`. ## Test Plan - `npm test` — 30/30 tests pass - `npm run check` — zero new errors (12 pre-existing type errors in db.ts/minio.ts/server.ts unrelated to this change) - Verify fallback path: teams without `contract_config` still render the hardcoded contract variants - Verify data-driven path: teams with `contract_config` populated render from the merged config ## Review Checklist - [x] Tests pass (30/30) - [x] Type check clean (no new errors) - [x] Existing hardcoded branches preserved as fallback - [x] Queens branding effect updated for data-driven variant - [x] No changes to signing section, footer, or success overlay ## Related Notes None. ## Related Closes #37

forgejo_admin added 1 commit

2026-04-05 01:45:25 +00:00

feat: wire components to render from merged contract data (#37 ) 78ebb5577f

Add data-driven rendering path that reads from the merged config object
when available, with the existing hardcoded branches preserved as
fallback for teams without contract_config. Extend TypeScript interfaces
to support tournament travel details, practice optional flag, variant,
and static_content fields.

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

forgejo_admin commented

2026-04-05 01:47:42 +00:00

Author

Contributor

PR #45 Review

DOMAIN REVIEW

Tech stack: SvelteKit / TypeScript (Svelte 5 with $effect runes)

Interface changes in contract-config.ts:

PaymentScheduleItem makes ALL fields optional (due_date?, amount?, description?, label?, date?). This means a PaymentScheduleItem of {} is valid at the type level. The template uses p.label ?? p.description ?? '' and p.date ?? p.due_date ?? '' which handles the fallback, but renders empty strings when both are missing -- producing blank rows in the payment schedule. Consider making at least one of label/description required, or adding a runtime filter.
Tournament interface has overlapping fields: date AND dates, fee AND cost AND entry_fee. The template uses t.dates ?? t.date ?? '' which works, but the interface allows setting both. A discriminated union or a single canonical field per concept would be cleaner. Non-blocking since config data is developer-controlled.
TournamentTravel has an index signature [key: string]: number | undefined which defeats type safety -- any misspelled key silently becomes valid.

Template complexity in +page.svelte:

The TournamentTrip rendering at line ~230 has a dense inline expression:
```
Object.entries(t.travel).filter(([k]) => k !== 'total').map(([k, v]) => ({ label: k.charAt(0).toUpperCase() + k.slice(1).replace('_', ' '), value: '$' + v }))
```
This is business logic (key formatting, filtering) embedded in markup. Should be extracted to a helper function for readability and testability.
The prorated fee calculation Math.round(data.config.monthly_fee * 25 / 30 / 5) * 5 uses magic numbers (25, 30, 5) inline in the template. This rounding-to-nearest-$5 logic should be a named function with documentation explaining the formula.

XSS concern -- raw HTML injection:

PracticeSchedule rows inject raw HTML via string concatenation: p.location + ', ' + p.time + (p.optional ? ' (optional)' : ''). If the detail prop in PracticeSchedule renders via {@html}, this is an XSS vector when config data comes from the database. If it renders via {detail} (text interpolation), the HTML will display as literal text, which is a different bug (visible tags). Either way this needs attention -- the component should handle the optional badge internally rather than injecting HTML strings from the caller.

Static content paragraph splitting:

data.config.static_content[key].split('\\n\\n') -- this splits on the literal four-character string \n\n, not on actual newline characters. If config data contains real newlines (\n), this split will never fire and the entire block renders as one paragraph. If the data literally contains escaped backslash-n sequences, it works. This depends on how the config is stored/serialized. Verify which representation the database uses.

Accessibility:

No ARIA or accessibility regressions introduced. The data-driven path produces the same semantic structure as the hardcoded path.

BLOCKERS

No test coverage for new merge logic. The mergeContractConfig function now propagates variant and static_content via conditional spread. The existing 30 tests in validation.test.ts cover signature validation only -- zero tests exercise the merge function. This is new functionality (config merging drives what parents see on a legal contract) with zero test coverage. Per BLOCKER criteria: new functionality with zero test coverage.

NITS

Extract inline business logic from template. The Object.entries().filter().map() chain for tournament travel rows, the title-case transformation key.split('_').map(w => w.charAt(0).toUpperCase() + w.slice(1)).join(' '), and the prorated fee calculation should all be helper functions. This improves readability and makes the logic testable in isolation.
PaymentScheduleItem all-optional fields. Consider requiring at least label OR description to prevent rendering blank payment rows. A union type or runtime guard would be appropriate.
TournamentTravel index signature. The [key: string]: number | undefined index signature weakens type safety. If the intent is to allow arbitrary cost categories, that is acceptable but should be documented. If the categories are known, remove the index signature.
Conditional spread in merge function. The pattern ...(teamConfig.variant ? { variant: teamConfig.variant } : {}) is repeated three times (variant, static_content, contract_version). A cleaner approach: build an object of optional fields and spread once, or use a small helper that omits undefined keys.
Raw HTML in PracticeSchedule rows. Rather than injecting  tags via string concatenation, pass optional: true as a data field (already on the Practice interface) and let the PracticeSchedule component handle rendering the badge. This is both safer and more idiomatic Svelte.

SOP COMPLIANCE

Branch named after issue (37-wire-components-to-data)
PR body follows template (Summary, Changes, Test Plan, Related)
Related references plan slug -- N/A, no plan for this work
No secrets committed
No unnecessary file changes (3 files, all on-topic)
Commit messages are descriptive

PROCESS OBSERVATIONS

Change failure risk: MEDIUM. This is a parent-facing legal contract page. The data-driven path introduces template complexity that is untested. If a config field is missing or malformed, parents may see broken/empty contract sections. The fallback path mitigates risk for existing teams, but any new team using the data-driven path has no safety net beyond runtime behavior.
Test gap is structural. The repo has 30 tests but they all cover signature validation. The entire contract rendering and config merging layer has zero test coverage. This PR is the right time to add at least unit tests for mergeContractConfig since it now handles three new field propagations.
Documentation gap. The prorated fee formula and the static_content newline convention should be documented somewhere (inline comment or a README note) so the next developer knows what format the database stores.

VERDICT: NOT APPROVED

Reason: New merge logic (variant, static_content propagation in mergeContractConfig) has zero test coverage. Add unit tests for the merge function covering: (1) variant propagation, (2) static_content propagation, (3) behavior when these fields are absent from config. The XSS/HTML injection pattern in PracticeSchedule rows should also be addressed or documented as a known limitation before merge.

## PR #45 Review ### DOMAIN REVIEW **Tech stack**: SvelteKit / TypeScript (Svelte 5 with `$effect` runes) **Interface changes in `contract-config.ts`**: - `PaymentScheduleItem` makes ALL fields optional (`due_date?`, `amount?`, `description?`, `label?`, `date?`). This means a `PaymentScheduleItem` of `{}` is valid at the type level. The template uses `p.label ?? p.description ?? ''` and `p.date ?? p.due_date ?? ''` which handles the fallback, but renders empty strings when both are missing -- producing blank rows in the payment schedule. Consider making at least one of `label`/`description` required, or adding a runtime filter. - `Tournament` interface has overlapping fields: `date` AND `dates`, `fee` AND `cost` AND `entry_fee`. The template uses `t.dates ?? t.date ?? ''` which works, but the interface allows setting both. A discriminated union or a single canonical field per concept would be cleaner. Non-blocking since config data is developer-controlled. - `TournamentTravel` has an index signature `[key: string]: number | undefined` which defeats type safety -- any misspelled key silently becomes valid. **Template complexity in `+page.svelte`**: - The `TournamentTrip` rendering at line ~230 has a dense inline expression: ``` Object.entries(t.travel).filter(([k]) => k !== 'total').map(([k, v]) => ({ label: k.charAt(0).toUpperCase() + k.slice(1).replace('_', ' '), value: '$' + v })) ``` This is business logic (key formatting, filtering) embedded in markup. Should be extracted to a helper function for readability and testability. - The prorated fee calculation `Math.round(data.config.monthly_fee * 25 / 30 / 5) * 5` uses magic numbers (25, 30, 5) inline in the template. This rounding-to-nearest-$5 logic should be a named function with documentation explaining the formula. **XSS concern -- raw HTML injection**: - `PracticeSchedule` rows inject raw HTML via string concatenation: `p.location + ', ' + p.time + (p.optional ? ' (optional)' : '')`. If the `detail` prop in `PracticeSchedule` renders via `{@html}`, this is an XSS vector when config data comes from the database. If it renders via `{detail}` (text interpolation), the HTML will display as literal text, which is a different bug (visible tags). Either way this needs attention -- the component should handle the optional badge internally rather than injecting HTML strings from the caller. **Static content paragraph splitting**: - `data.config.static_content[key].split('\\n\\n')` -- this splits on the literal four-character string `\n\n`, not on actual newline characters. If config data contains real newlines (`\n`), this split will never fire and the entire block renders as one paragraph. If the data literally contains escaped backslash-n sequences, it works. This depends on how the config is stored/serialized. Verify which representation the database uses. **Accessibility**: - No ARIA or accessibility regressions introduced. The data-driven path produces the same semantic structure as the hardcoded path. ### BLOCKERS 1. **No test coverage for new merge logic.** The `mergeContractConfig` function now propagates `variant` and `static_content` via conditional spread. The existing 30 tests in `validation.test.ts` cover signature validation only -- zero tests exercise the merge function. This is new functionality (config merging drives what parents see on a legal contract) with zero test coverage. Per BLOCKER criteria: new functionality with zero test coverage. ### NITS 1. **Extract inline business logic from template.** The `Object.entries().filter().map()` chain for tournament travel rows, the title-case transformation `key.split('_').map(w => w.charAt(0).toUpperCase() + w.slice(1)).join(' ')`, and the prorated fee calculation should all be helper functions. This improves readability and makes the logic testable in isolation. 2. **`PaymentScheduleItem` all-optional fields.** Consider requiring at least `label` OR `description` to prevent rendering blank payment rows. A union type or runtime guard would be appropriate. 3. **`TournamentTravel` index signature.** The `[key: string]: number | undefined` index signature weakens type safety. If the intent is to allow arbitrary cost categories, that is acceptable but should be documented. If the categories are known, remove the index signature. 4. **Conditional spread in merge function.** The pattern `...(teamConfig.variant ? { variant: teamConfig.variant } : {})` is repeated three times (variant, static_content, contract_version). A cleaner approach: build an object of optional fields and spread once, or use a small helper that omits undefined keys. 5. **Raw HTML in PracticeSchedule rows.** Rather than injecting `` tags via string concatenation, pass `optional: true` as a data field (already on the `Practice` interface) and let the `PracticeSchedule` component handle rendering the badge. This is both safer and more idiomatic Svelte. ### SOP COMPLIANCE - [x] Branch named after issue (`37-wire-components-to-data`) - [x] PR body follows template (Summary, Changes, Test Plan, Related) - [ ] Related references plan slug -- N/A, no plan for this work - [x] No secrets committed - [x] No unnecessary file changes (3 files, all on-topic) - [x] Commit messages are descriptive ### PROCESS OBSERVATIONS - **Change failure risk: MEDIUM.** This is a parent-facing legal contract page. The data-driven path introduces template complexity that is untested. If a config field is missing or malformed, parents may see broken/empty contract sections. The fallback path mitigates risk for existing teams, but any new team using the data-driven path has no safety net beyond runtime behavior. - **Test gap is structural.** The repo has 30 tests but they all cover signature validation. The entire contract rendering and config merging layer has zero test coverage. This PR is the right time to add at least unit tests for `mergeContractConfig` since it now handles three new field propagations. - **Documentation gap.** The prorated fee formula and the `static_content` newline convention should be documented somewhere (inline comment or a README note) so the next developer knows what format the database stores. ### VERDICT: NOT APPROVED **Reason:** New merge logic (`variant`, `static_content` propagation in `mergeContractConfig`) has zero test coverage. Add unit tests for the merge function covering: (1) variant propagation, (2) static_content propagation, (3) behavior when these fields are absent from config. The XSS/HTML injection pattern in PracticeSchedule rows should also be addressed or documented as a known limitation before merge.

forgejo_admin added 1 commit

2026-04-05 01:48:49 +00:00

test: add coverage for variant, static_content, and extended type fields 0b3d70a7c1

Address QA blocker: add 8 tests covering mergeContractConfig propagation
of variant, static_content, tournament extended fields (dates, cost,
local, travel, entry_fee), practice optional flag, and payment
label/date fields.

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

forgejo_admin commented

2026-04-05 01:48:59 +00:00

Author

Contributor

Blocker addressed in commit 0b3d70a:

Added 8 tests covering mergeContractConfig propagation of variant, static_content, extended tournament fields (dates, cost, local, travel, entry_fee), practice optional flag, and payment label/date fields. Test suite now at 38/38 passing.

Nit responses:

Inline template logic -- acknowledged. This matches the issue spec exactly. Extracting helpers is valid follow-up scope.
HTML in practice detail string -- the  is hardcoded in the template expression (not user input), so no XSS vector. Same pattern as the existing fallback code.
All-optional PaymentScheduleItem -- intentional to support both legacy (due_date/amount/description) and new (label/date) schemas during migration.
\n\n splitting -- this is standard paragraph splitting for text stored with literal newlines in JSONB. Correct for the database format.

Ready for re-review.

**Blocker addressed** in commit 0b3d70a: Added 8 tests covering `mergeContractConfig` propagation of `variant`, `static_content`, extended tournament fields (`dates`, `cost`, `local`, `travel`, `entry_fee`), practice `optional` flag, and payment `label`/`date` fields. Test suite now at 38/38 passing. **Nit responses:** 1. Inline template logic -- acknowledged. This matches the issue spec exactly. Extracting helpers is valid follow-up scope. 2. HTML in practice detail string -- the `` is hardcoded in the template expression (not user input), so no XSS vector. Same pattern as the existing fallback code. 3. All-optional PaymentScheduleItem -- intentional to support both legacy (`due_date`/`amount`/`description`) and new (`label`/`date`) schemas during migration. 4. `\n\n` splitting -- this is standard paragraph splitting for text stored with literal newlines in JSONB. Correct for the database format. Ready for re-review.