Split questions.ts up

[stefan-burke]

Summary by CodeRabbit

• New Features
  • Improved custom-question handling for choice and free-text answers, including stricter parsing/validation, encrypted free-text loading, and free-text string de-duplication.
  • Added support for question/answer aggregates and enhanced ordering operations (including swaps and next-order assignment).
• Bug Fixes
  • Made attendee answer saving, retrieval, and rendering more consistent across admin and public flows.
• Refactor
  • Split question/answer data access into dedicated modules and updated app behavior to use them consistently, along with broader test updates and improved custom-question data wiring.
• Tests
  • Enhanced test SQLite temp-file tracking and cleanup for reliability.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

The shared questions database code was split into focused modules, and callers across features, UI, and tests now import from those paths. Test database helpers also moved to tracked temp-file utilities with sidecar cleanup.

Changes

Questions DB modularization

Layer / File(s)	Summary
Core types and tables src/shared/db/question-types.ts, src/shared/db/questions/tables.ts	New shared question and answer types, display-type validation, and the question, answer, and listing-question table schemas are defined.
Query helpers src/shared/db/questions/queries.ts, src/shared/db/questions/parsing.ts, src/shared/db/questions/aggregates.ts, src/shared/db/questions/delete.ts, src/shared/db/questions/strings.ts, src/shared/db/questions/sort-order.ts	Question reads, listing membership lookups, parsing, aggregate recalculation, modifier links, deletion helpers, string interning, and ordering helpers are added.
Attendee answers src/shared/db/questions/attendee-answers.ts	Attendee answer saving, grouped reads, decrypted text loading, and attendee question data helpers are added.
Feature wiring updates src/features/admin/*, src/features/public/*, src/shared/merge/attendee-merge.ts, src/shared/qr.ts, src/ui/templates/**, biome.json	Consumers switch to the new modular question DB files, and the lint override list drops the removed barrel module path.

Tracked test database files

Layer / File(s)	Summary
Temp DB utilities test/test-utils/temp-db-files.ts	Creates tracked temp DB files, removes SQLite sidecars, and cleans up the shared temp directory on unload.
Temp DB consumers and tests test/test-utils/db.ts, test/lib/db/*, test/lib/test-utils/*, test/lib/server-*/*, test/shared/*, test/ui/*	DB test helpers and temp-file tests use the tracked temp DB utilities, and coverage is added for cleanup of SQLite sidecar files.

Estimated code review effort: 4 (Complex) | ~60 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: breaking up the monolithic questions.ts module into smaller modules.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch split-questions

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/features/admin/attendees-merge.ts (1)

379-389: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win

parseConflictDecisions discards its built map and returns undefined.

The function builds out (lines 383-387) but returns undefined on line 388, dropping the result entirely. The declared return type is still Record<string, T>, so this is also a type mismatch. Callers parseBookingDecisions and parseMoneyDecisions (and transitively parseMergeDecisionForm) will get undefined for booking/money conflict decisions instead of the parsed map, breaking attendee-merge conflict resolution. This appears unrelated to the questions.ts module split and looks like an accidental edit.

🐛 Proposed fix

   const out: Record<string, T> = {};
   for (const { key } of conflictBookingEntries(diff)) {
     const value = parse(key);
     if (value !== undefined) out[key] = value;
   }
-  return  undefined ;
+  return out;
 };

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/features/admin/attendees-merge.ts` around lines 379 - 389,
`parseConflictDecisions` in `attendees-merge.ts` is accidentally discarding the
assembled conflict map and returning `undefined`. Update the function to return
the built `out` record so its declared `Record<string, T>` return type matches
the actual value; this will restore correct results for `parseBookingDecisions`,
`parseMoneyDecisions`, and `parseMergeDecisionForm`.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/features/admin/attendee-form-routes.ts`:
- Line 395: The `answers` field in `attendee-form-routes.ts` is using an inline
`import()` type annotation instead of the existing top-level `AttendeeAnswerSet`
import. Update the surrounding schema/type definition to reference the already
imported `AttendeeAnswerSet` symbol directly, keeping the style consistent with
the `import type` usage near `saveAttendeeAnswers` and avoiding the redundant
inline module import.

In `@src/shared/db/questions/attendee-answers.ts`:
- Around line 99-198: saveAttendeeAnswers currently performs the DELETE and
INSERT phases as separate batches, which can leave attendee_answers empty if the
insert phase fails. Update saveAttendeeAnswers to run the full
clear-and-reinsert flow inside a single transaction, or add equivalent rollback
handling around the existing executeBatch calls so the delete in
attendee-answers.ts cannot commit without the matching inserts. Keep the
existing normalization and statement-building logic intact, but make the atomic
boundary encompass both the initial delete batch and the final insert batch.

In `@src/shared/db/questions/tables.ts`:
- Around line 28-37: The questionsTable definition intentionally omits
sort_order, so add a brief note near the defineTable<Question, QuestionInput>
schema in questionsTable to explain that sort_order is managed separately in
sort-order.ts and via raw ORDER BY clauses. Make sure the comment clearly marks
this as intentional so the absence of sort_order from Question/questionsTable is
not mistaken for a bug.

---

Outside diff comments:
In `@src/features/admin/attendees-merge.ts`:
- Around line 379-389: `parseConflictDecisions` in `attendees-merge.ts` is
accidentally discarding the assembled conflict map and returning `undefined`.
Update the function to return the built `out` record so its declared
`Record<string, T>` return type matches the actual value; this will restore
correct results for `parseBookingDecisions`, `parseMoneyDecisions`, and
`parseMergeDecisionForm`.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 5efbc266-02d5-4f6e-9958-16c92ec08fca

📥 Commits

Reviewing files that changed from the base of the PR and between 40c53e3 and 78552c9.

📒 Files selected for processing (82)

• biome.json
• src/features/admin/attendee-form-routes.ts
• src/features/admin/attendee-page-data.ts
• src/features/admin/attendees-csv.ts
• src/features/admin/attendees-merge.ts
• src/features/admin/calendar.ts
• src/features/admin/group-page-data.ts
• src/features/admin/listing-page-data.ts
• src/features/admin/listings-view.ts
• src/features/admin/modifiers.ts
• src/features/admin/questions.ts
• src/features/api/payment-processing.ts
• src/features/public/ticket-form.ts
• src/features/public/ticket-payment.ts
• src/features/public/ticket-submit/parse.ts
• src/features/public/ticket-submit/paths.ts
• src/features/public/ticket-submit/prepare.ts
• src/features/public/types.ts
• src/shared/db/attendees/servicing.ts
• src/shared/db/question-types.ts
• src/shared/db/questions.ts
• src/shared/db/questions/aggregates.ts
• src/shared/db/questions/attendee-answers.ts
• src/shared/db/questions/delete.ts
• src/shared/db/questions/parsing.ts
• src/shared/db/questions/queries.ts
• src/shared/db/questions/sort-order.ts
• src/shared/db/questions/strings.ts
• src/shared/db/questions/tables.ts
• src/shared/merge/attendee-merge.ts
• src/shared/qr.ts
• src/ui/templates/admin/attendee-detail.tsx
• src/ui/templates/admin/attendee-form.tsx
• src/ui/templates/admin/attendees.tsx
• src/ui/templates/admin/questions.tsx
• src/ui/templates/attendee-table.tsx
• src/ui/templates/components/question-text.tsx
• src/ui/templates/public/reservations.tsx
• test/e2e/n-plus-one-guard.test.ts
• test/lib/checkout-pricing-consistency.test.ts
• test/lib/column-order/attendee-answers.test.ts
• test/lib/csv-questions.test.ts
• test/lib/parse-question-answers.test.ts
• test/lib/render-questions.test.ts
• test/lib/server-attendee-form/questions.test.ts
• test/lib/server-attendees.test.ts
• test/lib/server-booking-preserve.test.ts
• test/lib/server-calculate.test.ts
• test/lib/server-groups/attendees.test.ts
• test/lib/server-listing-export-checkin.test.ts
• test/lib/server-listings/export.test.ts
• test/lib/server-listings/show-groups-and-answers.test.ts
• test/lib/server-misc-admin-handlers.test.ts
• test/lib/server-modifiers/scope-links.test.ts
• test/lib/server-parents-gate.test.ts
• test/lib/server-public/custom-questions-multi.test.ts
• test/lib/server-public/custom-questions-single.test.ts
• test/lib/server-questions/answer-edit.test.ts
• test/lib/server-questions/answers.test.ts
• test/lib/server-questions/helpers.ts
• test/lib/server-questions/listing-questions.test.ts
• test/lib/server-questions/question-delete.test.ts
• test/lib/server-questions/questions.test.ts
• test/lib/server-webhooks/custom-questions-multi.test.ts
• test/lib/server-webhooks/custom-questions-single.test.ts
• test/lib/servicing/atomicity.test.ts
• test/lib/servicing/custom-questions.test.ts
• test/lib/ticket-form.test.ts
• test/shared/db/listings/delete.test.ts
• test/shared/db/modifier-resolve.test.ts
• test/shared/db/questions/attendee-answers.test.ts
• test/shared/db/questions/crud.test.ts
• test/shared/db/questions/helpers.ts
• test/shared/db/questions/listing-mapping.test.ts
• test/shared/db/questions/with-listings.test.ts
• test/shared/merge/attendee-merge/apply.test.ts
• test/shared/merge/attendee-merge/diff.test.ts
• test/shared/merge/attendee-merge/helpers.ts
• test/test-utils/factories.ts
• test/ui/templates/admin/attendee-detail.test.ts
• test/ui/templates/admin/attendees.test.tsx
• test/ui/templates/components/question-text.test.tsx

💤 Files with no reviewable changes (2)

• src/shared/db/questions.ts
• biome.json

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/test-utils/temp-db-files.ts`:
- Around line 12-18: The shared temp directory cleanup in removeIfPresent is
swallowing a non-empty directory failure because Deno.removeSync is used without
recursive cleanup. Update removeIfPresent (and the call site that deletes the
shared temp dir) to detect directory deletes and pass recursive: true, or
otherwise ensure the directory is removed even when stray files exist. Keep the
existing base-path/sidecar cleanup logic in temp-db-files.ts but make the final
directory removal robust so stray artifacts do not prevent cleanup.

🪄 Autofix (Beta)

✅ Autofix completed

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 26466d66-f20f-4416-aaa2-efe6e6e5b3b0

📥 Commits

Reviewing files that changed from the base of the PR and between 78552c9 and 0df8601.

📒 Files selected for processing (8)

• src/features/admin/attendees-merge.ts
• test/lib/db/free-text-migration.test.ts
• test/lib/db/legacy-migration.test.ts
• test/lib/db/with-transaction.test.ts
• test/lib/test-utils/stubs-and-mocks.test.ts
• test/lib/test-utils/temp-db-files.test.ts
• test/test-utils/db.ts
• test/test-utils/temp-db-files.ts

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/shared/db/questions/attendee-answers.ts (1)

161-179: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Filter stale text answers before interning strings.

getOrCreateStringIds currently writes/refreshes every submitted free-text value before liveTextQuestionIds filters deleted questions at Line 206. Stale form data for a removed question can therefore persist unused free-text in strings.

🛡️ Proposed fix

-  const [stringIds, questionIdsByAnswer, liveTextQuestionIds] =
-    await Promise.all([
-      getOrCreateStringIds(
-        [...normalized.values()].flatMap((set) =>
-          set.textAnswers.map((a) => a.text),
-        ),
-      ),
-      questionIdsByAnswerId(
-        unique([...normalized.values()].flatMap((set) => set.answerIds)),
-      ),
-      existingQuestionIds(
-        unique(
-          [...normalized.values()].flatMap((set) => [
-            ...set.textAnswerIds.map((answer) => answer.questionId),
-            ...set.textAnswers.map((answer) => answer.questionId),
-          ]),
-        ),
-      ),
-    ]);
+  const [questionIdsByAnswer, liveTextQuestionIds] = await Promise.all([
+    questionIdsByAnswerId(
+      unique([...normalized.values()].flatMap((set) => set.answerIds)),
+    ),
+    existingQuestionIds(
+      unique(
+        [...normalized.values()].flatMap((set) => [
+          ...set.textAnswerIds.map((answer) => answer.questionId),
+          ...set.textAnswers.map((answer) => answer.questionId),
+        ]),
+      ),
+    ),
+  ]);
+  const stringIds = await getOrCreateStringIds(
+    [...normalized.values()].flatMap((set) =>
+      set.textAnswers
+        .filter((answer) => liveTextQuestionIds.has(answer.questionId))
+        .map((answer) => answer.text),
+    ),
+  );
...
-      ...textAnswers.map((answer) => ({
-        questionId: answer.questionId,
-        stringId: stringIds.get(answer.text)!,
-      })),
+      ...textAnswers
+        .filter((answer) => liveTextQuestionIds.has(answer.questionId))
+        .map((answer) => ({
+          questionId: answer.questionId,
+          stringId: stringIds.get(answer.text)!,
+        })),

Also applies to: 200-206

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/shared/db/questions/attendee-answers.ts` around lines 161 - 179, The
free-text interning in attendee-answers is happening before deleted-question
filtering, so stale answers can still be written into strings. Update the flow
in the attendee answer processing around getOrCreateStringIds and
existingQuestionIds so only textAnswers whose questionId is present in
liveTextQuestionIds are passed into string interning. Keep the
liveTextQuestionIds filter as the source of truth, and apply it before building
the list for getOrCreateStringIds to avoid persisting unused stale values.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@src/shared/db/questions/attendee-answers.ts`:
- Around line 161-179: The free-text interning in attendee-answers is happening
before deleted-question filtering, so stale answers can still be written into
strings. Update the flow in the attendee answer processing around
getOrCreateStringIds and existingQuestionIds so only textAnswers whose
questionId is present in liveTextQuestionIds are passed into string interning.
Keep the liveTextQuestionIds filter as the source of truth, and apply it before
building the list for getOrCreateStringIds to avoid persisting unused stale
values.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: ea186fc6-342e-4c55-894e-50cefab3b527

📥 Commits

Reviewing files that changed from the base of the PR and between 0df8601 and 63b4cb9.

📒 Files selected for processing (3)

• src/features/admin/attendee-form-routes.ts
• src/shared/db/questions/attendee-answers.ts
• src/shared/db/questions/tables.ts

[stefan-burke]

parseConflictDecisions returning undefined — fixed

Valid catch, and a real bug — thank you. Confirmed the cause: a mutated return out; → return undefined; was written in place by the mutation-testing step, then the background precommit died mid-restore (the mutation step timed out after ~10 min — it was mutating the full split-commit diff including the 1985-line payment-processing.ts, impractically slow). The un-restored mutant was then committed in 78552c91 Refactor, so HEAD on the PR carried return undefined; until 0df86011 Save WIP restored the correct return out;.

Verified the fix is on the PR branch now: src/features/admin/attendees-merge.ts:388 reads return out; (commit 0df86011). The declared Record<string, T> return type and the callers parseBookingDecisions/parseMoneyDecisions/parseMergeDecisionForm now receive the built conflict map as intended.

The merge-decision parsers are deliberately private — exporting them only for tests would be flagged by the no test-only exports rule (test/lib/code-quality.test.ts:438), so they are exercised behaviourally by the e2e merge tests in test/lib/server-attendees.test.ts (the POST merge with take_source replaces target booking case at line ~3635 asserts the target booking actually receives the source's quantity — a check that would fail if the decision were dropped, since validateAttendeeMergeDecision rejects with "Missing decision for booking" when decision.bookings[key] is undefined).

[coderabbitai]

Note

Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it.

Fixes Applied Successfully

Fixed 1 file(s) based on 1 unresolved review comment.

Files modified:

• test/test-utils/temp-db-files.ts

Commit: 2acbadf914f336d116cc5b79990c18b250fdcdd9

The changes have been pushed to the split-questions branch.

Time taken: 4m 27s

[stefan-burke]

Filter stale text answers before interning — analyzed, deferred to a focused follow-up

@coderabbitai — the observation is valid in the abstract (a deleted-question free-text value does reach getOrCreateStringIds before the liveTextQuestionIds filter), but this is pre-existing behaviour carried verbatim out of the monolith by this pure-split PR, not a regression introduced here, so I've left the ordering unchanged and recorded the reasoning below.

Verified unchanged from main: main's src/shared/db/questions.ts:749-767 runs getOrCreateStringIds over every submitted free-text value in the same Promise.all as existingQuestionIds (the liveTextQuestionIds lookup), filtering only at the INSERT stage (monolith line 794). The split attendee-answers.ts:161-206 is byte-identical in this respect. Reordering here would change runtime behaviour in a refactor whose sole purpose is the module split.

The concern is already mitigated and intentionally tested:

• The free-text value for a question deleted between checkout and finalize was already interned at submit time (src/features/public/ticket-submit/parse.ts:149, computeListingTextAnswerIdMap) when the question was still live — finalize does not create new PII; the second getOrCreateStringIds only refreshes the created timestamp on a string the submit path already wrote.
• The attendee_answers orphan is prevented, not merely cleaned up: the liveTextQuestionIds filter at attendee-answers.ts:206 drops the deleted-question row, asserted by test/shared/db/questions/attendee-answers.test.ts:248 ("skips a text answer whose question was deleted at finalize"). The only residual is a strings row with used_count = 0, which pruneUnusedStrings (src/shared/db/prune.ts:95, age-based WHERE used_count = 0 AND created < ?) reclaims — and that "leave freed rows for the pruner" behaviour is itself an asserted invariant (test Fix setup CSRF cookie path to match both /setup and /setup/ #19, "deduplicates identical text answers and leaves freed rows for the pruner").
• All free-text is owner-key encrypted at rest; the window is bounded by prune retention, not indefinite.

Why defer rather than fix in this PR: the proposed reorder (compute liveTextQuestionIds first, then intern only live texts) changes the interning/refresh semantics getOrCreateStringIds relies on — see test #263, "refreshes created on a reused but still-unattached string" — and would force the stringIds.get(answer.text)! at attendee-answers.ts:204 to be reworked so a pre-filtered text can't yield an undefined id before the .filter runs. That belongs in a dedicated change that updates those tests, not in a pure module split — same shape of deferral as the delete/insert atomicity thread on this PR, which CodeRabbit already accepted.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@stefan-burke Thanks for the thorough analysis — this is a reasonable call. To summarize why I agree:

• The ordering is genuinely inherited from main's questions.ts and not something this split PR changes, so treating it as pre-existing/out-of-scope for a pure module-split PR is fair.
• The actual risk (orphaned attendee_answers rows) is already prevented by the liveTextQuestionIds filter, and the only residual (strings row with used_count = 0) is bounded and reclaimed by pruneUnusedStrings, both of which are covered by existing tests.
• The proposed reorder isn't a drop-in change — it would touch the interning/refresh semantics that another test already locks in, so it deserves its own dedicated PR with test updates rather than being folded into this refactor.

I won't press further on this in the current PR. If you'd like, I'm happy to open a follow-up issue to track the "filter stale text answers before interning" improvement so it doesn't get lost — just let me know and I'll file it with a link back to this thread.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2735cb4355

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.