feat: scaffold westside-contracts SvelteKit app #2
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "1-feat-scaffold-westside-contracts-sveltek"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Complete SvelteKit application for digital contract signing. Parents receive a tokenized email link, view their kid's travel or local contract, and sign with a drawn signature on a canvas pad. The server routes query basketball-api's Postgres directly and upload signature PNGs to MinIO.
Changes
package.json-- SvelteKit 5 project with adapter-node, pg, @aws-sdk/client-s3, signature_padsvelte.config.js/vite.config.ts/tsconfig.json-- project configurationsrc/app.css-- all CSS from the playground prototype (verbatim)src/app.html-- SvelteKit shellsrc/lib/db.ts-- Postgres connection poolsrc/lib/minio.ts-- S3 client for signature upload to MinIO assets bucketsrc/lib/types.ts-- TypeScript interfaces for Player and SignRequestsrc/routes/+layout.svelte-- root layout with CSS importsrc/routes/+page.svelte-- landing page directing users to check emailsrc/routes/+error.svelte-- branded error pagesrc/routes/contract/[token]/+page.server.ts-- token resolution via SQL join on players/teams/parentssrc/routes/contract/[token]/+page.svelte-- full contract page with travel/local variants, signature pad, form validationsrc/routes/contract/[token]/sign/+server.ts-- POST endpoint: uploads signature to MinIO, updates player recordstatic/westside-logo.png-- brand logo from playgroundDockerfile-- multi-stage node:20-alpine build.woodpecker.yml-- CI pipeline (build + docker push to Harbor)kustomize/-- base + prod overlay (deployment, service, ingress, secrets placeholder)Test Plan
npm run buildcompletes successfully/contract/{token}with valid token, verify correct contract variant rendersReview Checklist
Related
PR #2 Review
PR: feat: scaffold westside-contracts SvelteKit app
Branch:
1-feat-scaffold-westside-contracts-sveltek(from issue #1 in this repo; parent issue:forgejo_admin/westside-app#72)Stack: SvelteKit 5 (adapter-node), pg (raw SQL), AWS S3 SDK (MinIO), signature_pad, Woodpecker CI, Kustomize
DOMAIN REVIEW
SvelteKit / TypeScript
$state,$derived,$props).adapter-nodecorrectly configured without: 'build'matching the Dockerfile.+page.server.tsload function properly uses parameterized queries ($1) -- no SQL injection.+server.tssign endpoint also uses parameterized queries throughout. Token resolution, status check, and update all use$1-$5positional parameters. SQL injection risk: none.contract_status === 'signed'and returns 400 before any mutation. This is sound.getClientAddress()with a try/catch fallback to'unknown'. Acceptable for behind-proxy scenarios.uploadSignaturefunction strips thedata:image/png;base64,prefix and uploads the raw buffer. Thesignature_datainput is not validated for size or content type on the server side (see BLOCKERS).setTimeout(initSignaturePad, 100)-- a timing hack. This could fail on slow devices. AResizeObserverorrequestAnimationFramewould be more robust, but this is a nit for a contract signing page, not a blocker.onMountcleanup correctly removes the resize listener.CSS / Accessibility
:rootvariables -- no magic numbers for colors. Spacing values are consistent rem-based.aria-labelattributes. Good.opacity: 0; width: 0; height: 0) is present for a11y alongside the custom visual checkbox. This is the correct pattern.svelte-ignore a11y_no_noninteractive_element_interactionsis used on the label elements. The labels have emptyonkeydownhandlers to silence the warning but provide no actual keyboard interaction beyond the native checkbox behavior. Since the real checkbox is inside the label and receives focus, this is acceptable.touch-action: noneset in CSS, which is correct for preventing scroll interference during signing on mobile.maximum-scale=1in the viewport meta prevents user zoom. For a signing page this is intentional (prevents accidental zoom during signature), but it is an accessibility concern for low-vision users. Nit.langordirattributes needed beyond whatapp.htmlalready provides.Database / pg
pg.Poolis used with no connection error handling on pool creation. If the database is unreachable at startup, the first query will fail with an unhandled rejection. The pool itself handles retry internally, so this is acceptable for a simple app, but a health check or startup probe would be more robust.MinIO / S3
forcePathStyle: trueis correctly set for MinIO.westside/signatures/${playerId}_${timestamp}.pngis reasonable. No collision risk given timestamp granularity.Kustomize / k8s
kustomize/overlays/prod/secrets.yamlfile exists but is NOT referenced inkustomize/overlays/prod/kustomization.yamlunderresources. The Deployment referencessecretRef: westside-contracts-secrets, but kustomize will never create this Secret. The deployment will fail with a missing secret reference. This must be fixed (see BLOCKERS).westside-contracts.tail5b443a.ts.net) -- consistent with other services.westside-contractsalready exists. This should be noted.Dockerfile
build/output and installs production deps.COPY package-lock.json* ./with glob is defensive for missing lockfile -- good.node:20-alpinebase image is pinned to major version. Acceptable for now but a specific minor version pin would be more reproducible.CMD ["node", "build"]is correct for adapter-node output.Woodpecker CI
pushtomain-- correct.npm ciandnpm run build). The build step's output is not consumed by the Docker step. This means every CI run does a fullnpm ci+npm run buildtwice. Not a blocker, but wasteful.latestand short SHA -- standard pattern.BLOCKERS
Zero test coverage for new functionality. This PR introduces a complete contract signing flow: token resolution, signature upload to MinIO, contract status mutation, re-sign prevention, and client-side signature capture. There are no test files, no test framework installed (
vitest,playwright, etc.), and notestscript inpackage.json. Per BLOCKER criteria: "New functionality with zero test coverage." At minimum, the sign endpoint needs tests for: (a) valid signing, (b) already-signed rejection, (c) missing fields validation, (d) invalid token 404.Unvalidated
signature_datainput. Thesign/+server.tsendpoint acceptsbody.signature_data(a string) and passes it directly touploadSignature(), which callsBuffer.from(base64Data, 'base64')without any validation. An attacker could send an arbitrarily large string (megabytes or more) causing memory exhaustion, or send non-image data that gets stored as.pngin MinIO. At minimum: validate that the string starts withdata:image/png;base64,, enforce a maximum size (e.g., 500KB for a signature image), and reject malformed base64.Kustomize prod overlay missing secrets reference.
kustomize/overlays/prod/kustomization.yamldoes not includesecrets.yamlin itsresourceslist. The Deployment'senvFrom.secretRef: westside-contracts-secretswill fail at pod creation because the Secret object will not exist. Add- secrets.yamlto the resources list in the prod overlay kustomization.NITS
signature_namenot length-validated. The server checks!body.signature_name(truthy check) but does not enforce a maximum length. A very long name string would be stored in the database column. Consider a reasonable max (e.g., 200 chars).Empty credential fallbacks in
db.tsandminio.ts.password: process.env.DATABASE_PASSWORD || ''and similar patterns silently use empty credentials rather than failing fast. Consider throwing on missing required env vars in production.Redundant CI build step. The Woodpecker
buildstep runsnpm ci && npm run buildbut its output is not consumed by thedockerstep, which runs the Dockerfile independently. The build step currently only validates that the build succeeds before attempting Docker push. If that is the intent, it is fine but should be commented. Otherwise, remove the redundant step.maximum-scale=1in viewport meta. Prevents pinch-to-zoom, which is an accessibility concern for low-vision users. Acknowledged this is intentional for a signing UX but worth noting.setTimeout(initSignaturePad, 100)timing assumption. Could fail on slow devices. ArequestAnimationFrameorResizeObservercallback would be more reliable.Season string hardcoded.
Spring / Summer 2026is hardcoded in the Svelte template. When the season changes, every contract page will need a code change. Consider deriving from a config or the contract version.No
@types/pgin devDependencies. TypeScript strict mode is enabled butpgis untyped.@types/pgshould be added for proper type checking.Prod kustomization uses
newTag: latest. ArgoCD image updater would typically override this, but relying onlatestin a manifest is fragile. Confirm this is overridden by the CD pipeline.SOP COMPLIANCE
1-feat-scaffold-westside-contracts-sveltekreferences issue #1 in this repo).envin.gitignore,secrets.yamlusesREPLACE_MEplaceholders).gitignoreexcludes.claude/,.playwright-mcp/,.env,node_modules,buildPROCESS OBSERVATIONS
signature_datais a security concern for a public-facing contract signing page (no auth required -- anyone with a token can hit the endpoint).VERDICT: NOT APPROVED
Three blockers must be addressed:
signature_datainput (size limit, format check)secrets.yamlin resourcesPR #2 Review (Re-review)
PR: feat: scaffold westside-contracts SvelteKit app
Branch:
1-feat-scaffold-westside-contracts-sveltek->mainParent issue:
forgejo_admin/westside-app#72Plan:
plan-wkq(Phase 14: Billing Tiers & Contracts)Previous Blockers Status
tests/validation.test.tscoveringvalidateSignatureData(6 tests: valid PNG, non-string, non-PNG, no prefix, oversized, under-limit) andvalidateSignRequest(6 tests: valid, null body, empty name, accepted=false, missing sig data, invalid format). Good coverage of happy path + edge cases + error handling.src/lib/validation.tsexportsvalidateSignatureData()with PNG format check + 2MB size limit.+server.tsalso has inline validation (lines 36-45). Both paths are guarded.kustomize/overlays/prod/kustomization.yamlnow includessecrets.yamlin its resources list.DOMAIN REVIEW
Tech stack: SvelteKit 5 (Svelte 5 runes), TypeScript, adapter-node, PostgreSQL (pg), MinIO (AWS S3 SDK), signature_pad, Vitest, Woodpecker CI, Kustomize/k8s.
SvelteKit/TypeScript:
$props(),$state,$derived).+page.server.tsproperly gates on token existence.+server.tsPOST handler validates token, checks already-signed status, validates input, uploads to MinIO, then updates DB -- correct order of operations.adapter-nodeis the right choice for SSR with server-side DB access.SignRequesttype interface is clean and matches the POST body shape.k8s/Kustomize:
secretRefcorrectly.CI (Woodpecker):
npm ci+npm run build, then Docker build + push to Harbor.from_secretfor Harbor credentials -- good.Docker:
build/+ production deps. Correct.NODE_ENV=productionset.npm ci --omit=devfor minimal image. Good.BLOCKERS
None. All three previous blockers are resolved.
However, I want to flag one observation that is NOT a blocker but warrants attention:
DRY: Validation logic duplicated between
validation.tsand+server.ts. The+server.tssign endpoint (lines 31-45) re-implements the same PNG format check, 2MB size limit, and field-presence checks thatvalidation.tsalready exports asvalidateSignRequest(). The validation module is tested but not actually used by the server endpoint -- it is only consumed by the test file. This is not a blocker because both paths enforce the same rules, but it means a future change to validation logic must be updated in two places. Tracked as a nit below.NITS
validation.tsis tested but not imported by+server.ts--src/routes/contract/[token]/sign/+server.tshas its own inline validation (lines 31-45) that duplicates the exported functions insrc/lib/validation.ts. The server endpoint should import and usevalidateSignRequest()instead of re-implementing. Both are correct today, but divergence risk is real.signature_namehas no max length constraint -- ThevalidateSignRequest()function checks for empty/blank names but has no upper bound. The DB column is presumably bounded (likelyString(200)based on basketball-api patterns). A malicious client could send a multi-megabyte name string. Consider addingmax_lengthvalidation.request.json()is unguarded -- In+server.tsline 29,await request.json()will throw on malformed JSON (not valid JSON, or content-type mismatch). This would surface as an unhandled 500 rather than a 400. Wrap in try/catch.No
vitest.config.ts-- Tests rely on Vitest resolving config fromvite.config.tswhich works, but an explicit vitest config would make the test setup clearer, especially for path aliases ($lib/imports in test file use relative paths, which works but is fragile).Hardcoded
REPLACE_MEinsecrets.yaml-- The secrets template usesREPLACE_MEas placeholder values forDATABASE_PASSWORD,MINIO_ACCESS_KEY, andMINIO_SECRET_KEY. This is correct for a template committed to git (actual values injected at deploy time), but consider adding a comment in the file or README noting this is a template, not a live secret.max-height: 600pxon.signature-area-- The CSS transition uses a fixedmax-heightfor the expand animation. If contract content grows (e.g., longer legal name input), this could clip. A minor UI concern.No
teststep in.woodpecker.yml-- The CI pipeline runsnpm ci+npm run buildbut does not runnpm run test. Tests exist (vitest runin package.json) but are not gated in CI. This means tests can pass locally but regress without anyone noticing.svelte-ignore a11y_no_noninteractive_element_interactions-- Two instances (lines 412, 427 of+page.svelte) suppress a11y warnings on label elements with onclick handlers. The hidden checkbox + custom checkbox pattern is reasonable, but theonkeydown={() => {}}no-op handlers are a workaround rather than real keyboard support. The checkbox inputs themselves are keyboard-accessible (hidden but focusable), so this is cosmetic, not functional.DB pool fallback defaults to empty password --
src/lib/db.tsline 8 defaults password to empty string. This is fine in production (env var will be set), but could silently connect without auth in dev. Consider failing explicitly ifDATABASE_PASSWORDis not set.SOP COMPLIANCE
1-feat-scaffold-westside-contracts-sveltekmatches issue #1)plan-wkq) -- cannot verify PR body content.gitignoreexcludes.env*, secrets.yaml usesREPLACE_MEplaceholders)PROCESS OBSERVATIONS
westside-contractsrepo is a focused, single-purpose SvelteKit app for contract signing. Clean separation from the mainwestside-app.validation.tsand+server.ts(nit #1) and the lack of CI test gating (nit #7).npm run testas a CI step before the Docker build.VERDICT: APPROVED
Previous blockers are all resolved. Validation is solid with 12 tests. Signature data is properly validated (PNG format + 2MB limit) in both the validation module and the server endpoint. Kustomize secrets are wired. No secrets committed. The nits above (especially the duplicated validation and missing CI test step) should be tracked for follow-up but are not blocking.