Files
fdbck/docs/plans/architecture-improvements.md
mAi c13d84d0f3 docs: architecture audit — deepening opportunities for fdbck
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.
2026-05-08 11:08:15 +02:00

34 KiB
Raw Permalink Blame History

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 2364
default question stub FormBuilder.svelte defaultQuestion 6488
editor card branches FormBuilder.svelte template 250416
participant input branches routes/f/[slug]/+page.svelte 634757
participant answer summary routes/f/[slug]/+page.svelte summariseSubmittedAnswer 326362
participant required-validation routes/f/[slug]/+page.svelte submitForm 220235
admin submissions-cell summary routes/admin/feedback/[id]/+page.svelte summarizeAnswer 297312
results aggregation lib/server/results.ts emptyStats / ingest / finalise 105250
results chart branches lib/components/Results.svelte template 200360
public-results sanitization lib/server/results.ts publicResults 252263
server submit required-validation api/public/feedback/[slug]/submit/+server.ts 3747
CSV column expansion api/admin/feedback/[id]/export/+server.ts 105130

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 <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 46 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. 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 812 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 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 deleteing in finalise. Functional, but readers pause. A separate Map<qid, sum> 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 2364), 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 812 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 (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 <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:

  1. <ChatPanel slug={inst.slug} sessionId chatStatus role="participant|moderator" /> — 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. <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/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: <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: 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 1738), api/public/feedback/[slug]/+server.ts (lines 2470), api/public/feedback/[slug]/submit/+server.ts (lines 5593).

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 23 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

  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 12- shift refactor. Everything else gets easier afterwards.

Tier 2 — independent, small commits

  1. §3.C — withOwnedInstance wrapper. Small, localised, removes a real copy-paste. Independent of §3.A. ~1 commit.
  2. §3.D — findExistingSubmission helper. Small, localised, removes 3-place duplication. Independent of §3.A. ~1 commit.
  3. §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

  1. §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 consume registry), so wait until after §3.A to do §3.B.
  2. §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<type, QuestionTypeModule> 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 <ChatPanel> extraction, §3.F per-type tests).