Per-screen + per-server-helper audit. Identifies six deepening
opportunities ranked by leverage:
T1: §3.A per-question-type module bundle (highest leverage; closes a
real server-side date_ranked_choice validation gap; unblocks i18n)
T2: §3.C withOwnedInstance wrapper, §3.D findExistingSubmission helper,
§3.F testability gaps
T3: §3.B ChatPanel + FormEditor component extraction, §3.E
feedback_instances repository
Each candidate explained against the deletion test (LANGUAGE.md vocab):
modules, depth, locality, leverage, seams, adapters. Tier-1 is the only
load-bearing refactor; tier-2 are small independent wins; tier-3 is
medium-cost cleanup that gets easier after tier-1.
Anti-scope listed: no new deps, no CSS split, no real-time migration,
no auth tier on /f/[slug], no port/adapter for fdb (one adapter =
hypothetical seam, per LANGUAGE.md).
5 open questions for m at the end before any code lands.
Design only — no source files touched. Awaiting m's pick before any
coder shift.
34 KiB
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#1history.) - 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/markDbAccessedneed 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
FeedbackQuestionSchemais 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.tsis well-isolated: pure functions (todayVersion,nextVersionToday,nextVersion) tested inresults.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'ssetScaleLabelis misnamed — it only handlesdate_ranked_choice's nestedscale.{min,max}_label(early-returns on every other type), while the standalonescalequestion's labels use inline handlers. Half-grown helper from the m/fdbck#1 add.summarizeAnswer(admin) andsummariseSubmittedAnswer(participant) are near-homophones with different signatures ((v)vs.(q, v)) and divergent locales (Yes/Novs.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 <ChatPanel slug sessionId isMine={...}> 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 <FormEditor bind:value bind:editMode> — 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:
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, '<context-tag>');
}
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.
generateSlugreturns 32 base62 chars. Not tested for length, alphabet, or birthday-collision behaviour over a large pool. stampVersion.nextVersion(pure) is tested; the wiring that readsfeedback_submissions.form_snapshot.versionand 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
FeedbackQuestionSchemaaccepts a valid example and rejects bad shapes. Catches drift like thedate_ranked_choiceserver-side gap. - Server-side required-question gating. Submit handler with a missing
required answer of every type. Would have caught the
date_ranked_choicegap. - CSV column expansion. Per-type. The
[opt_id]suffix path is fragile and only exercised against the export endpoint manually. - Public-results sanitization.
publicResultsstrips 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/<type>.ts." One file per type tells the
whole story.
Other navigability friction:
- The
parseFormDefinitionquirk (supabase-js with.schema('fdbck')returning JSONB as encoded strings) is documented infeedback.ts:46and applied insidegetInstanceBy{Slug,Id}. But the admin list endpoint/api/admin/feedback/+server.tsand the detail endpoint/api/admin/feedback/[id]/+server.tsGET 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 forform_definitionand has toJSON.parseit. Inconsistency. A thin Repository wrapper that always normalises would make this invisible. void q;at line 219 ofresults.tsis a "tell the linter the parameter is intentionally unused" hack. The function takesqbut the switch statement only ever needss. Either dropqfrom the signature or use it for a defensive check.(s as ScaleStats & { _sum?: number })._sumcast pattern (results.ts:162, 211) — using a private temp on the public stats object during aggregation, thendeleteing infinalise. Functional, but readers pause. A separateMap<qid, sum>or aWIPStatstype used during aggregation and converted to publicStatsinfinalisewould 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:
interface QuestionTypeModule<T extends FeedbackQuestion['type']> {
type: T;
schema: ZodType<Extract<FeedbackQuestion, { type: T }>>;
defaultStub(): Extract<FeedbackQuestion, { type: T }>;
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 <ParticipantInput {q} bind:answer />. 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
<type>.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 (oneisAnswerEmpty, two callers). - i18n (m/fdbck#3 prep): each module's
AdminCellSummaryand the ResultsBlock take a locale via context; one chokepoint per type. - AI-navigability: "how does a
booleanquestion work?" → openlib/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 <svelte:component this={registry[q.type].ParticipantInput}>
pattern — Svelte 5 handles this cleanly. Largest deepening on this list.
3.B — <ChatPanel> + <FormEditor> 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:
<ChatPanel slug={inst.slug} sessionId chatStatus role="participant|moderator" />— owns:postsstate, 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.<FormEditor bind:value={editForm} bind:json={editFormJson} bind:mode={editMode} />— owns: visual / JSON segmented toggle;syncJsonFromVisual,syncVisualFromJson; the<FormBuilder>mount; the JSON textarea + Insert-sample button. Used in both/admin/feedback/newand/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:
<ChatPanel>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:
// lib/server/admin-route.ts
export function withOwnedInstance<P>(
handler: (ctx: { inst: FeedbackInstance; event: RequestEvent<P> }) => Promise<Response>,
context: string, // for handleApiError
): RequestHandler<P> {
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:
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:
withOwnedInstanceitself 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:
export interface SubmissionLookup {
submitted_at: string;
display_name: string | null;
answers: Record<string, unknown>;
}
export async function findExistingSubmission(
instanceId: string,
by: { sessionId?: string; ip?: string; ua?: string },
): Promise<SubmissionLookup | null> {
// 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:
// lib/server/feedback-repo.ts
export async function findInstanceBySlug(slug): Promise<FeedbackInstance | null>;
export async function findInstanceById(id): Promise<FeedbackInstance | null>;
export async function listInstancesForOwner(userId): Promise<(FeedbackInstance & { counts })[]>;
export async function updateInstance(id, patch): Promise<FeedbackInstance>;
export async function deleteInstance(id): Promise<void>;
export async function insertInstance(input): Promise<FeedbackInstance>;
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/<type>.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
- §3.A — Per-question-type module bundle. Highest leverage on this
list. Closes the existing server-side
date_ranked_choicevalidation 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
- §3.C —
withOwnedInstancewrapper. Small, localised, removes a real copy-paste. Independent of §3.A. ~1 commit. - §3.D —
findExistingSubmissionhelper. Small, localised, removes 3-place duplication. Independent of §3.A. ~1 commit. - §3.F — testability gaps that don't depend on §3.A. Specifically:
generateSlug,stampVersion,publicResultsstrip-text, honeypot. ~1 commit per topic. Independent of all the above.
Tier 3 — more invasive, less urgent
- §3.B —
<ChatPanel>+<FormEditor>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<FormEditor>is built (because the visual/JSON branches inside it now consumeregistry), so wait until after §3.A to do §3.B. - §3.E —
feedback_instancesrepository. 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
- 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.
- Component test runner. We have
bun test ./*.test.tsfor 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. - Naming.
lib/questions/scale.tsvs.lib/question-types/scale.tsvs.lib/q/scale.ts. I'd default tolib/questions/. Confirm. - Registry shape. A flat
Record<type, QuestionTypeModule>centralised inlib/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 inFormBuilder. setScaleLabelrename + 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.tsaggregation 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.tspolicy gate. Genuinely deep. Don't touch.auth.ts+request-context.ts. Small, tested viapublic-scope.test.tsindirectly. 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 <ChatPanel>
extraction, §3.F per-type tests).