diff --git a/docs/plans/architecture-improvements.md b/docs/plans/architecture-improvements.md new file mode 100644 index 0000000..a433431 --- /dev/null +++ b/docs/plans/architecture-improvements.md @@ -0,0 +1,734 @@ +# fdbck — architecture audit + +**Status:** design proposal, no code touched. +**Branch:** `mai/cronus/architecture-audit` off main (`bc278a8`). +**Author:** cronus, 2026-05-06. +**Scope:** identify *deepening opportunities* — refactors that turn shallow +modules into deep ones, with the aim of better testability and AI-navigability. + +m's brief: small codebase (~3 900 lines across 1 stylesheet, 3 components, +6 routes, 12 server helpers, schemas, 8 API endpoints), perfect candidate for +a focused architecture pass before the codebase calcifies. + +The doc is structured as: §1 quick survey, §2 friction findings (the seven +coverage areas), §3 deepening opportunities with the "deletion test" applied, +§4 a 1-pager prioritised proposal list. m picks; I implement what's chosen. + +--- + +## 1. Survey + +### Map of the territory + +``` +src/ +├── app.d.ts +├── hooks.server.ts ← auth + public-scope policy gate (40L) +├── lib/ +│ ├── components/ +│ │ ├── FormBuilder.svelte (438L) ← per-question-type editor cards +│ │ ├── Icon.svelte (97L) +│ │ └── Results.svelte (375L) ← per-question-type result charts +│ ├── schemas.ts (160L) ← every zod schema lives here +│ ├── styles/ +│ │ └── feedback.css (1500+L) ← tokens + every component +│ └── server/ +│ ├── auth.ts, errors.ts, fdb.ts, feedback.ts, public-scope.ts, +│ │ rate-limit.ts, request-context.ts, response.ts, results.ts, +│ │ shlink.ts, supabase.ts (≈800L total across 11 files) +│ └── (3 .test.ts files: rate-limit, public-scope, results) +└── routes/ + ├── +page.svelte, login/, /f/[slug]/+page.svelte (865L) + ├── admin/feedback/+page.svelte, [id]/+page.svelte (693L), new/+page.svelte + └── api/ + ├── auth/{sign-in,sign-out} + ├── public/feedback/[slug]/{posts, results, submit, +} + └── admin/feedback/{+, [id]/+, [id]/{export, share, posts/[post_id]/hide}} +``` + +### Decisions inherited from `docs/plans/feedback-feature.md` (not up for revision) + +- **Slug = access token.** 32-char base62 (~190 bits), no auth on participant + side. Don't propose adding auth; m chose Security-by-Obscurity. +- **In-memory rate-limiter.** Single Node instance; explicit "if scaled + horizontally, replace with Redis." Don't propose Redis until that scale. +- **JSON form-definition.** No drag-drop form-builder. Visual editor exists + but JSON paste is the canonical interchange. (See `m/fdbck#1` history.) +- **Polling at 3s for chat / 5s for admin refresh / 5s for live results.** + Real-time is "trivial migration when needed" but not in scope. +- **Public-scope policy gate** is a security architecture decision (see + `flexsiebels#59`); `requireAuth` / `markVisibilityFiltered` / + `markDbAccessed` need to keep being called from every endpoint that + touches the DB. Don't propose dissolving the gate. + +### What's healthy already + +- Server modules are correctly sliced (auth / errors / rate-limit / public-scope + / request-context / fdb-schema-accessor / supabase-client / results + aggregation / shlink). Each file has one job and a small interface. +- The discriminated-union `FeedbackQuestionSchema` is a sane *data* model. + The trouble is its *behavioural* fan-out (see §3.A). +- The public-scope gate is genuinely deep: small interface + (`requireAuth` / `markVisibilityFiltered`), large guarantee (anonymous + responses can't accidentally leak DB data). It was clearly a model. +- Versioning logic in `results.ts` is well-isolated: pure functions + (`todayVersion`, `nextVersionToday`, `nextVersion`) tested in + `results.test.ts`. Good seam. +- Honeypot + rate-limit + closed-instance gates are consistent across + POST endpoints — same shape every time, no drift. + +--- + +## 2. Friction findings + +For each, I name the modules involved, describe the friction, apply the +deletion test ("if this disappeared, where would the complexity go?"), and +note implications for tests, locality, and i18n. + +### 2.A — The question-type fan-out (the biggest) + +A "question type" lives, today, as a discriminated-union *strip* across the +following files: + +| Concern | File | Lines | +| --- | --- | --- | +| zod schema branch | `lib/schemas.ts` | 23–64 | +| default question stub | `FormBuilder.svelte` `defaultQuestion` | 64–88 | +| editor card branches | `FormBuilder.svelte` template | 250–416 | +| participant input branches | `routes/f/[slug]/+page.svelte` | 634–757 | +| participant answer summary | `routes/f/[slug]/+page.svelte` `summariseSubmittedAnswer` | 326–362 | +| participant required-validation | `routes/f/[slug]/+page.svelte` `submitForm` | 220–235 | +| admin submissions-cell summary | `routes/admin/feedback/[id]/+page.svelte` `summarizeAnswer` | 297–312 | +| results aggregation | `lib/server/results.ts` `emptyStats` / `ingest` / `finalise` | 105–250 | +| results chart branches | `lib/components/Results.svelte` template | 200–360 | +| public-results sanitization | `lib/server/results.ts` `publicResults` | 252–263 | +| server submit required-validation | `api/public/feedback/[slug]/submit/+server.ts` | 37–47 | +| CSV column expansion | `api/admin/feedback/[id]/export/+server.ts` | 105–130 | + +That's **twelve places** for one concept. m's commit history confirms the +pain: m/fdbck#1 (`date_ranked_choice`) needed schema + builder + participant +renderer + results aggregator + Results.svelte chart + CSV expansion, and the +strip even diverged: the server-side required-validation in `submit/+server.ts` +quietly skips `date_ranked_choice` (it only checks for empty-string / +empty-array, not "no options rated"), so the gate is only enforced +client-side. That's a real bug-shape bug, not just untidy code. + +Two small navigation papercuts inside this strip: + +- `FormBuilder.svelte`'s `setScaleLabel` is misnamed — it only handles + `date_ranked_choice`'s nested `scale.{min,max}_label` (early-returns on + every other type), while the standalone `scale` question's labels use + inline handlers. Half-grown helper from the m/fdbck#1 add. +- `summarizeAnswer` (admin) and `summariseSubmittedAnswer` (participant) are + near-homophones with different signatures (`(v)` vs. `(q, v)`) and + divergent locales (`Yes/No` vs. `Ja/Nein`) and divergent date_ranked_choice + rendering (`"3.4 avg (5 rated)"` vs. per-option list). Same job, two code + paths. + +**Deletion test:** delete any single one of these twelve callers, the +complexity reappears immediately at the other eleven. They all earn keep. +But they don't have to live in twelve files. + +### 2.B — Page-level `.svelte` files have grown into kitchen sinks + +| File | Lines | Concerns it juggles | +| --- | ---: | --- | +| `routes/f/[slug]/+page.svelte` | 865 | localStorage session + display name; chat polling; per-user color hashing; 3 different time formatters; compose autosize + IME-aware Enter; single-submission server-load + client-retry; live-results polling; per-question-type input branches; per-type answer summary; submit; reset-and-resubmit | +| `routes/admin/feedback/[id]/+page.svelte` | 693 | 4-tab mode; status pill optimistic toggle; share strip + replace; click-outside menu; full edit form (title/desc/toggles/visual/JSON); form-version pill; polling refresh; CSV/JSON export; submissions table; chat moderator | +| `routes/admin/feedback/new/+page.svelte` | 281 | Title/desc/toggles + the same Visual/JSON segmented editor as `[id]/+page.svelte` (literal copy of `editForm` / `editFormJson` / `switchEditMode` / `ensureBuilderForm` from the detail page) | + +Both detail pages of the admin flow share a question-authoring sub-tree +that's been *copy-pasted*. The participant page has a chat sub-tree that +mirrors (loosely) the admin chat moderator UI but with bubbles instead of +boxes. + +**Deletion test:** if we delete the participant chat block (≈ 200 lines), the +behaviour vanishes. If we extract a `` component, the same behaviour survives and now has *one* +place that knows how to fetch / post / autosize / IME / colorize / scroll. +Concentrates ~250 lines into one file with a small props interface. Deep. + +Same for `` — the Visual/JSON segmented +editor used in two places. ~80 lines saved on the create page; the edit-tab +on the detail page also shrinks. Already partially modularised via +`FormBuilder.svelte` (the visual mode), but the `editFormJson` / +`syncJsonFromVisual` / `syncVisualFromJson` / `switchEditMode` helpers are +*not* in `FormBuilder.svelte` — they sit at page level twice. + +### 2.C — The `requireAuth → ownerOf → try/catch` pattern is shallow but copied + +Every admin API endpoint opens with the same 4–6 lines: + +```ts +const err = requireAuth(locals.userId); +if (err) return err; +try { + const inst = await getInstanceById(params.id); + if (!inst) return notFound('Instance not found'); + if (inst.owner_user_id !== locals.userId) return unauthorized('Not your instance'); + // ... actual handler ... +} catch (e) { + return handleApiError(e, ''); +} +``` + +Affected: `/api/admin/feedback/[id]/+server.ts` (defines `ownerOf` helper — +only used inside this file), `/api/admin/feedback/[id]/posts/[post_id]/hide`, +`/api/admin/feedback/[id]/share`, `/api/admin/feedback/[id]/export`. The list +endpoint `/api/admin/feedback/+server.ts` has the auth half but not the +ownership half (lists by `owner_user_id = userId`). + +The `ownerOf` helper *exists* in one file and the others copy its body +inline. Adding a fifth admin-instance endpoint means re-opening that strip +again. + +**Deletion test:** delete the inline 6-liners and replace with a +`withOwnedInstance(handler)` wrapper. Complexity moves into one wrapper, +then disappears as new endpoints are added. The wrapper is genuinely deep +because: + +``` +interface: handler({ inst, params, request, locals }) → Response +implementation: 30 lines of auth + ownership + error handling +leverage: every admin-instance endpoint is N lines shorter +``` + +Today's `ownerOf` is a *partial* version of this — it returns +`{ ok, response | inst }` but each endpoint still writes the destructure + +the `try/catch` around it. + +### 2.D — "Find existing submission" appears in three places + +The single-submission gate looks up "did this client submit before?" in +three different code paths with three different shapes: + +| Caller | Lookup keys | Shape returned | +| --- | --- | --- | +| `routes/f/[slug]/+page.server.ts` (server load) | IP + UA only | `previousSubmission \| null` | +| `api/public/feedback/[slug]/+server.ts` (client retry GET) | session_id OR IP+UA | `previousSubmission \| null` | +| `api/public/feedback/[slug]/submit/+server.ts` (gate on POST) | session_id OR IP+UA | `existing \| null` (then 409) | + +Same `select`, same `eq` columns, same ordering. Three near-identical +PostgREST queries. + +**Deletion test:** delete the inline triple and replace with +`findExistingSubmission(instId, { sessionId, ip, ua })` returning a +`SubmissionLookup | null`. The complexity (the IP+UA fallback rule, the +session_id-first ordering, the `maybeSingle()` behaviour) concentrates in +one helper. Deep. + +### 2.E — Schema ergonomics + +`schemas.ts` is 160 lines and currently fine. It already uses +`z.discriminatedUnion('type', [...])` with a base extended per type. m +asked whether breaking it into per-question-type files would help future +adds. + +My read: **per-question-type files are useful only as part of §2.A** (a +per-type module that bundles schema + UI parts + server logic together). +Splitting just the schema is a paper-shuffle and would *worsen* navigation +because you'd lose the at-a-glance discriminated-union view. The right +unit is "everything about question type X in one file," not "everything +about question type X's *schema* in one file." + +The non-question schemas in this file (`FeedbackInstance{Create,Update}`, +`FeedbackSubmission`, `FeedbackPost`, `FeedbackPostHide`, `SignIn`, +`ShareCreate`) belong together — they're API request bodies. They could +live in `lib/api-schemas.ts` while question schemas move to per-type +modules. Mild win, not load-bearing. + +### 2.F — Testability gaps + +Today: `rate-limit.test.ts` (4 cases), `public-scope.test.ts` (full table of +allow/block decisions), `results.test.ts` (aggregation + version stamping). + +Untested: +- **Slug generation.** `generateSlug` returns 32 base62 chars. Not tested + for length, alphabet, or birthday-collision behaviour over a large pool. +- **`stampVersion`.** `nextVersion` (pure) is tested; the wiring that + reads `feedback_submissions.form_snapshot.version` and *picks* the next + one is not. This is the entry point that future edits will most likely + break. Needs an integration test with a fake fdb client. +- **Per-question-type validation roundtrips.** Each branch of + `FeedbackQuestionSchema` accepts a valid example and rejects bad shapes. + Catches drift like the `date_ranked_choice` server-side gap. +- **Server-side required-question gating.** Submit handler with a missing + required answer of every type. Would have caught the + `date_ranked_choice` gap. +- **CSV column expansion.** Per-type. The `[opt_id]` suffix path is + fragile and only exercised against the export endpoint manually. +- **Public-results sanitization.** `publicResults` strips text answers; + one-line test would lock the contract. +- **Single-submission gate.** The IP+UA + session_id matrix has at least + 6 paths (cleared-storage / new-IP / different-UA / first-submit / + resubmit-attempt / closed-instance). Today only tested by m clicking. +- **`findExistingSubmission`** (when extracted, §2.D) — 6-row table test. +- **`withOwnedInstance`** (when extracted, §2.C) — auth-fail / not-found / + not-yours / pass-through. + +If §3.A's per-type module bundle lands, each type's module gets a small +unit-test file and most of these gaps close per-type. The other gaps are +cross-cutting (slug, stampVersion, withOwnedInstance, findExistingSubmission) +and need their own tests. + +### 2.G — i18n future (m/fdbck#3) + +Hardcoded strings live inline: + +- **German** in `/f/[slug]/+page.svelte`: "Anonym", "Sende …", "Absenden", + "Ja", "Nein", "Diese Feedback-Sitzung ist geschlossen — neue Beiträge sind + nicht mehr möglich.", "Live-Feedback", "Fragebogen", "Live-Ergebnisse", + "Antwort gesendet", "Noch eine Antwort senden", "Du hast bereits + abgesendet…", "Bitte beantworte: {label}", "Bitte bewerte mindestens eine + Option: {label}", "passt nicht", "passt super", "Beitrag entfernt", + "(von Moderation entfernt — du siehst es noch)", "Nachricht…", "Senden" +- **English** on admin pages: "Forms", "New form", "Save", "Edit", + "Delete", "Open", "Closed", "Copy link", "Replace short link", "Yes", + "No", "responses", "messages", "anonymous", every menu item, every + banner. + +Today's architecture **hurts** i18n because each per-question-type strip +(§2.A) carries inline strings that an i18n migration would need to find +and rewrite *separately* in each strip. After §2.A's per-type module bundle: +each type owns its strings and can take the locale through one chokepoint +(`registry[q.type].render(q, answer, ctx)` where `ctx` carries `t()`). +Same string moves once, not 12 times. + +The participant footer string `fdbck.msbls.de` is fine as branding; the +trust-evoking copy on the participant page is German because that's the +audience. Rewriting all of it for i18n is m/fdbck#3's job — but the +*architecture* shouldn't multiply the work by 12. + +### 2.H — AI-navigability for a coder picking up cold + +A new agent (or a returning future-you) reading this codebase needs to +answer "how does a question type work?" The current answer is "open these +8–12 files in this order." That's the high-water mark of friction. + +After §2.A: "open `lib/questions/.ts`." One file per type tells the +whole story. + +Other navigability friction: + +- The `parseFormDefinition` quirk (supabase-js with `.schema('fdbck')` + returning JSONB as encoded strings) is documented in `feedback.ts:46` + and applied inside `getInstanceBy{Slug,Id}`. But the admin list endpoint + `/api/admin/feedback/+server.ts` and the detail endpoint + `/api/admin/feedback/[id]/+server.ts` GET both **don't** call it on the + rows they return. Two of these routes happen to render via Svelte which + doesn't care; one renders via JSON which means the browser receives a + JSON-encoded string for `form_definition` and has to `JSON.parse` it. + Inconsistency. A thin Repository wrapper that *always* normalises would + make this invisible. +- `void q;` at line 219 of `results.ts` is a "tell the linter the + parameter is intentionally unused" hack. The function takes `q` but the + switch statement only ever needs `s`. Either drop `q` from the + signature or use it for a defensive check. +- `(s as ScaleStats & { _sum?: number })._sum` cast pattern (results.ts:162, + 211) — using a private temp on the public stats object during + aggregation, then `delete`ing in `finalise`. Functional, but readers + pause. A separate `Map` or a `WIPStats` type used during + aggregation and converted to public `Stats` in `finalise` would be + clearer. Low priority. + +### 2.I — CSS coupling (low priority, mentioned for completeness) + +`feedback.css` is now 1500+ lines, single file. Tokens + viewport reset + +header + banner + section + question + option + scale + button + chat +(admin) + bubble (participant) + tabs + builder + results + cards + menus ++ status pill + segment + share strip + closed line + compose + summary + +date-ranked + per-question density + dark-mode overrides for each cluster. + +Splitting into `tokens.css`, `forms.css`, `chat.css`, `admin-list.css`, +`admin-detail.css` would help locate styles. But the file *reads* fine +today — clear comment-banner sections separate clusters. Defer until it +hurts. + +--- + +## 3. Deepening opportunities + +For each candidate, I name the modules, restate the problem in one +sentence, sketch the solution in plain English, and explain the benefits +in terms of locality + leverage + tests. + +### 3.A — Per-question-type module bundle + +**Files:** `lib/schemas.ts` (lines 23–64), `lib/components/FormBuilder.svelte`, +`lib/components/Results.svelte`, `lib/server/results.ts`, +`routes/f/[slug]/+page.svelte`, `routes/admin/feedback/[id]/+page.svelte`, +`api/public/feedback/[slug]/submit/+server.ts`, +`api/admin/feedback/[id]/export/+server.ts`. + +**Problem:** every question type is implemented as a 12-place strip; m/fdbck#1 +demonstrated the cost (and shipped a server-side validation gap as a +side-effect). Adding a new type touches 8–12 files in the same review. + +**Solution:** introduce `lib/questions/` with one file per type +(`scale.ts`, `single-choice.ts`, `multi-choice.ts`, `boolean.ts`, +`short-text.ts`, `long-text.ts`, `date-ranked-choice.ts`) plus a +`registry.ts`. Each module exports a value satisfying: + +```ts +interface QuestionTypeModule { + type: T; + schema: ZodType>; + defaultStub(): Extract; + isAnswerEmpty(question, answer): boolean; + emptyStats(question): QuestionStats; + ingest(stats, question, answer, createdAt): void; + finalise(stats): void; + sanitizeForPublic(stats): QuestionStats; + csvColumns(question): { header: string; key: string; optId?: string }[]; + csvCellFor(question, answer, col): string; + // UI: + BuilderEditor: SvelteComponent; + ParticipantInput: SvelteComponent; + AdminCellSummary(question, answer, locale): string; + ResultsBlock: SvelteComponent; +} +``` + +The schema discriminated-union becomes +`z.discriminatedUnion('type', QUESTION_MODULES.map(m => m.schema))`. The +participant page becomes ``. The admin +table cell becomes `registry[q.type].AdminCellSummary(q, a, locale)`. The +aggregator becomes `registry[q.type].ingest(...)`. The CSV expansion +becomes `for col of registry[q.type].csvColumns(q): out.push(...)`. The +required-validation gate (server + client) reduces to +`if (q.required && registry[q.type].isAnswerEmpty(q, answers[q.id])) ...` +— ONE rule, two callers, no drift. + +**Benefits:** + +- **Locality**: a new type = one file + one line in `registry.ts`. Today's + 12-place strip becomes one place. +- **Leverage**: the registry's interface is small (~10 named exports per + type) but the implementation behind it covers the whole app's + per-type behaviour. +- **Tests**: every module gets a colocated `.test.ts`. The empty- + answer rule, the schema roundtrip, the aggregation, the CSV expansion + all become unit-testable in isolation. The server-side gap that exists + today closes by construction (one `isAnswerEmpty`, two callers). +- **i18n** (m/fdbck#3 prep): each module's `AdminCellSummary` and the + ResultsBlock take a locale via context; one chokepoint per type. +- **AI-navigability**: "how does a `boolean` question work?" → open + `lib/questions/boolean.ts`. Done. + +**Cost:** non-trivial rewrite. Probably 1 commit per question type +(7 commits) plus a CSS-friendly slot system if the BuilderEditor / +ParticipantInput / ResultsBlock components are pulled out. The Svelte +side may need a `` +pattern — Svelte 5 handles this cleanly. **Largest deepening on this list.** + +### 3.B — `` + `` extraction + +**Files:** `routes/f/[slug]/+page.svelte` (the chat half), +`routes/admin/feedback/[id]/+page.svelte` (the chat-tab + the edit-tab), +`routes/admin/feedback/new/+page.svelte` (the edit-tab clone). + +**Problem:** participant page is 865 lines doing 9 concerns; admin detail +is 693 doing 9 concerns; the new-form page literally copy-pastes the +edit-tab's `editForm` / `editFormJson` / `switchEditMode` helpers from +the detail page. + +**Solution:** + +1. **``** + — owns: `posts` state, polling, `postChat`, `fetchPosts`, autosize, + IME-aware Enter, color hashing, time formatting, scroll-to-bottom. + Used in both `/f/[slug]` (participant role) and `/admin/feedback/[id]` + Chat tab (moderator role with extra hide button). One component, + two render variants. +2. **``** + — owns: visual / JSON segmented toggle; `syncJsonFromVisual`, + `syncVisualFromJson`; the `` mount; the JSON textarea + + Insert-sample button. Used in both `/admin/feedback/new` and + `/admin/feedback/[id]` Edit tab. + +**Benefits:** + +- Page files shrink to ~400 / ~450 / ~150 lines. +- The chat fetch/post protocol (lastSeenAt cursor, deduping by `id`, + optimistic append) lives in one place. Today it's *almost* duplicated + between participant and admin Chat tab — admin polls via the detail + endpoint instead of `/posts`, so the de-dup logic is participant-only, + but the bubble vs. box renderer is the only real difference. +- Tests: `` becomes mountable in component tests. Today the + chat behaviour is untestable through its current interface (it's + inlined in a 865-line page). + +**Cost:** medium. Extract + rewire both pages. Independent of §3.A. + +### 3.C — `withOwnedInstance` admin-route wrapper + +**Files:** `api/admin/feedback/[id]/+server.ts` (defines `ownerOf`), +`api/admin/feedback/[id]/posts/[post_id]/hide/+server.ts`, +`api/admin/feedback/[id]/share/+server.ts`, +`api/admin/feedback/[id]/export/+server.ts`. + +**Problem:** every admin-instance endpoint opens with the same 6-line +auth + ownership + try/catch preamble. The `ownerOf` helper exists in +one file and is duplicated inline in three others. + +**Solution:** lift the preamble into a wrapper: + +```ts +// lib/server/admin-route.ts +export function withOwnedInstance

( + handler: (ctx: { inst: FeedbackInstance; event: RequestEvent

}) => Promise, + context: string, // for handleApiError +): RequestHandler

{ + return async (event) => { + const err = requireAuth(event.locals.userId); + if (err) return err; + try { + const inst = await getInstanceById(event.params.id); + if (!inst) return notFound('Instance not found'); + if (inst.owner_user_id !== event.locals.userId) return unauthorized('Not your instance'); + return await handler({ inst, event }); + } catch (e) { + return handleApiError(e, context); + } + }; +} +``` + +Each endpoint becomes: + +```ts +export const POST = withOwnedInstance(async ({ inst, event }) => { + const body = await parseBody(event.request, FeedbackPostHideSchema); + // ... only the actual work ... + return json({ post: data }); +}, 'admin feedback post hide'); +``` + +**Benefits:** + +- **Locality**: the auth+ownership rule is one place; new endpoints can't + forget it. +- **Tests**: `withOwnedInstance` itself becomes testable (no auth, no + instance, wrong owner, happy path). Today every endpoint re-tests this + by inspection. +- **AI-navigability**: a coder reading the handler sees only the + endpoint-specific logic; the security envelope is implicit. + +**Cost:** small. One helper, four call-site rewrites. Independent of §3.A +and §3.B. + +### 3.D — `findExistingSubmission` extraction + +**Files:** `routes/f/[slug]/+page.server.ts` (lines 17–38), +`api/public/feedback/[slug]/+server.ts` (lines 24–70), +`api/public/feedback/[slug]/submit/+server.ts` (lines 55–93). + +**Problem:** the single-submission lookup query (session_id OR IP+UA, +ordered by created_at desc, limit 1) is inlined three times. The +participant load path uses IP+UA-only by design; the API GET and submit +paths use both. + +**Solution:** add a helper to `lib/server/feedback.ts`: + +```ts +export interface SubmissionLookup { + submitted_at: string; + display_name: string | null; + answers: Record; +} + +export async function findExistingSubmission( + instanceId: string, + by: { sessionId?: string; ip?: string; ua?: string }, +): Promise { + // session_id first (if provided), then IP+UA fallback (if both provided), + // mirrors today's three call sites. +} +``` + +**Benefits:** + +- **Locality**: the priority rule (session-id-first, IP+UA-fallback) lives + in one place. Today changing it requires touching three queries. +- **Tests**: one matrix test — `(sessionId yes/no) × (ip yes/no) × + (ua yes/no) × (db row matches yes/no)`. + +**Cost:** small. Independent of everything. + +### 3.E — Repository for `feedback_instances` row normalisation + +**Files:** `lib/server/feedback.ts` (`parseFormDefinition`), +`api/admin/feedback/+server.ts` (skips it), +`api/admin/feedback/[id]/+server.ts` (skips it, returns the raw row to JSON). + +**Problem:** supabase-js with `.schema('fdbck')` returns `form_definition` +JSONB as a JSON-encoded *string* instead of a decoded object. Two +callers normalise via `parseFormDefinition`; two callers don't, leaking +encoded strings into JSON responses where the browser has to +double-decode. + +**Solution:** wrap fdb-query of `feedback_instances` in a small repository +that always normalises: + +```ts +// lib/server/feedback-repo.ts +export async function findInstanceBySlug(slug): Promise; +export async function findInstanceById(id): Promise; +export async function listInstancesForOwner(userId): Promise<(FeedbackInstance & { counts })[]>; +export async function updateInstance(id, patch): Promise; +export async function deleteInstance(id): Promise; +export async function insertInstance(input): Promise; +``` + +Every endpoint goes through this repo; no caller writes raw `fdb().from(...)` +queries against `feedback_instances`. The string-decoding wart hides +inside the repo. + +**Benefits:** + +- **Locality**: the supabase-js JSONB quirk lives in one place. The + current "remember to call `parseFormDefinition`" trap goes away. +- **Tests**: repo methods are mockable. Today the admin endpoints are + hard to unit-test because they speak SQL directly. +- **Adapter signal**: today there's *one* implementation + (`getSupabaseAdmin().schema('fdbck')`). The repo creates a hypothetical + seam (no second adapter today). **One adapter = hypothetical seam, not + yet a real seam** per LANGUAGE.md — so this is a lower-priority "set + up the seam, don't over-design it." Don't introduce a port/adapter + pattern; just collapse the inline queries into typed repo methods. + +**Cost:** medium. Many call sites. Probably 2–3 commits. + +### 3.F — Closing the testability gaps + +Already covered point-by-point in §2.F. Recommended unit tests: + +| Test | File | Lines (rough) | +| --- | --- | --- | +| `generateSlug` length / alphabet / collision | `lib/server/feedback.test.ts` | ~30 | +| `findExistingSubmission` matrix | `lib/server/feedback.test.ts` | ~80 (after §3.D) | +| `withOwnedInstance` 4 paths | `lib/server/admin-route.test.ts` | ~60 (after §3.C) | +| `parseFormDefinition` round-trip + already-decoded passthrough | `lib/server/feedback.test.ts` | ~15 | +| `stampVersion` integration with fake fdb | `lib/server/feedback.test.ts` | ~80 | +| Per-type schema accept-valid / reject-bad | per-type `lib/questions/.test.ts` | ~20 each (after §3.A) | +| Per-type empty-answer / required-violation | per-type | ~15 each (after §3.A) | +| Per-type CSV expansion roundtrip | per-type | ~20 each (after §3.A) | +| `publicResults` strips text answers | `lib/server/results.test.ts` (extend) | ~15 | +| Honeypot drops submission silently | `routes/api/public/feedback/[slug]/submit/+server.test.ts` | ~30 | + +The biggest payoff is the per-type tests post-§3.A, because they sit +beside the implementation and don't need integration scaffolding. + +--- + +## 4. Prioritised proposals (1-pager) + +m picks one or more; cronus then implements per shift. Each proposal lists +its dependencies on others. + +### Tier 1 — biggest leverage, biggest commit + +1. **§3.A — Per-question-type module bundle.** Highest leverage on this + list. Closes the existing server-side `date_ranked_choice` validation + gap by construction. Sets up i18n cleanly. Roughly 1 commit per type + (7 commits) plus a registry-and-shape commit; medium-large cost. **My + recommendation: do this first** if m has appetite for a focused 1–2- + shift refactor. Everything else gets easier afterwards. + +### Tier 2 — independent, small commits + +2. **§3.C — `withOwnedInstance` wrapper.** Small, localised, removes a + real copy-paste. Independent of §3.A. ~1 commit. +3. **§3.D — `findExistingSubmission` helper.** Small, localised, removes + 3-place duplication. Independent of §3.A. ~1 commit. +4. **§3.F — testability gaps that don't depend on §3.A.** Specifically: + `generateSlug`, `stampVersion`, `publicResults` strip-text, honeypot. + ~1 commit per topic. Independent of all the above. + +### Tier 3 — more invasive, less urgent + +5. **§3.B — `` + `` extraction.** Medium-cost + component refactor. Real win for the 865/693-line page files but the + two-call-sites-each ratio means the leverage is moderate. Independent + of §3.A. **Note:** the post-§3.A registry approach changes how + `` is built (because the visual/JSON branches inside it + now consume `registry`), so wait until after §3.A to do §3.B. +6. **§3.E — `feedback_instances` repository.** Medium-cost rewire. Wins + are: hides the JSONB-decoding quirk in one place, sets up the + testability seam for admin endpoints. Currently has *one* concrete + implementation, so per LANGUAGE.md this is a hypothetical seam — + don't go full ports-and-adapters, just collapse inline queries into + typed repo methods. Independent of §3.A. + +### Anti-scope (NOT proposed) + +- **No new dependencies.** Same constraint as the redesign work. +- **No CSS split** (§2.I). The single feedback.css reads fine today. +- **No real-time / WebSocket migration.** Polling is the documented + decision; defer until it actually hurts. +- **No auth tier on participant pages.** Slug-as-token is the documented + decision. +- **No proper i18n shipped here.** Just *prepare* for it via §3.A's + per-type modules. m/fdbck#3 is a separate ticket. +- **No port/adapter pattern for fdb.** One adapter = hypothetical seam, + per LANGUAGE.md. §3.E is a "collapse inline queries," not a "design + for a swap-out we won't do." + +--- + +## 5. Open questions for m before implementing anything + +1. **Tier 1 vs. Tier 2 first?** §3.A is the highest leverage but biggest + commit. §3.C / §3.D / §3.F are all tiny and independent. Picking up + the tier-2 wins first is honest progress with low risk; tier-1 is the + real architectural payoff. **My recommendation: §3.C + §3.D + §3.F's + easy tests in one shift to clear quick debt; then §3.A in a focused + shift afterwards.** +2. **Component test runner.** We have `bun test ./*.test.ts` for server- + side units but no Svelte component test runner configured. After + §3.A, the per-type Svelte parts (`BuilderEditor`, `ParticipantInput`, + `ResultsBlock`) want component-level tests. Do we add one (e.g. + `@testing-library/svelte` + jsdom)? **Recommend: only if §3.A lands; + otherwise skip.** +3. **Naming.** `lib/questions/scale.ts` vs. `lib/question-types/scale.ts` + vs. `lib/q/scale.ts`. I'd default to `lib/questions/`. Confirm. +4. **Registry shape.** A flat `Record` + centralised in `lib/questions/registry.ts`, vs. each module + self-registering on import. Prefer the explicit registry — it's the + one place that lists every type, ordering matters for the + "+ Add question" buttons in `FormBuilder`. +5. **`setScaleLabel` rename + the `(s as ScaleStats & { _sum?: number })` + cleanup** — these are tiny papercut fixes (§2.A nav-friction and + §2.H ugly-cast). Do them as a "code-quality" sub-commit at the start + of §3.A's branch, or as a separate trivial commit? Either is fine. + **Recommend: bundle into the §3.A branch as the very first commit so + the diff is calmer afterwards.** + +--- + +## 6. What's not on this list and why + +- **`results.ts` aggregation logic.** Already deep, well-tested, + well-named. The `(s as X & { _sum })` pattern is ugly but isolated. + Don't refactor without §3.A pulling it apart. +- **`public-scope.ts` policy gate.** Genuinely deep. Don't touch. +- **`auth.ts` + `request-context.ts`.** Small, tested via + `public-scope.test.ts` indirectly. Don't touch. +- **`shlink.ts` / share endpoint.** External-API adapter, single caller, + small surface. Don't touch. +- **`Icon.svelte`.** A 14-icon switch statement; trivially extendable. + Fine. +- **`feedback.css`.** Long but readable. §2.I. + +--- + +## TL;DR (for the head's nudge) + +The single biggest deepening opportunity is **§3.A — bundle each question +type into its own module** (schema + UI parts + server logic). It closes a +real validation gap, sets up i18n cleanly, makes adding a type a +one-file-plus-one-line change instead of a 12-place strip, and unlocks +clean per-type unit tests. Everything else on this list is either a small +independent win (`withOwnedInstance`, `findExistingSubmission`, the slug / +stampVersion / publicResults tests) or downstream of §3.A (§3.B `` +extraction, §3.F per-type tests).