test: raise coverage 49.67% → 98.84%, fix the config that hid the gap

.claude/TODO/2026-04-17-components-template-editor-missing-tests.md

@@ -16,7 +16,7 @@ files:
   - src/components/template-editor/actions.ts
 discovered: 2026-04-17
 discovered_by: components-template-editor
-status: open
+status: resolved
 ---
 
 ## Problem
@@ -39,3 +39,40 @@ Add at minimum:
 - Regressions in `compileMjml`'s auth gate or size cap can ship unnoticed (security/DoS risk).
 - Merge-field category changes or taxonomy refactors will not surface breakage.
 - Phase 2 of the template editor (adding MP persistence, token resolution) will be harder to land safely without a baseline.
+
+---
+
+## Resolution (2026-09-13)
+Closed by the coverage push. Every file in `src/components/template-editor/`
+now has a co-located test, plus the two route files:
+
+| File | Tests | Statements | Lines |
+|---|---|---|---|
+| `merge-fields.ts` | 5 | 100% | 100% |
+| `grapes-config.ts` | 6 | 100% | 100% |
+| `actions.ts` | 7 | 100% | 100% |
+| `merge-field-picker.tsx` | 7 | 100% | 100% |
+| `editor-import-dialog.tsx` | 8 | 95.65% | 100% |
+| `editor-code-dialog.tsx` | 11 | 97.67% | 100% |
+| `editor-export-dialog.tsx` | 11 | 98.08% | 100% |
+| `editor-toolbar.tsx` | 21 | 100% | 100% |
+| `editor-canvas.tsx` | 7 | 100% | 100% |
+| `template-editor-form.tsx` | 3 | 100% | 100% |
+| `templateeditor/template-editor.tsx` | 6 | 100% | 100% |
+| `templateeditor/page.tsx` | 1 | 100% | 100% |
+
+Directory aggregate: 98.8% statements, 100% lines, across 90 tests.
+
+Two mocking notes worth keeping, both learned the hard way:
+- `useEditor()` must be mocked with a **stable object reference** built once via
+  `vi.hoisted()`. Several components run `useEffect`s keyed on the editor
+  object's identity, so a `useEditor: () => ({...})` factory returns a new
+  object on every render and spins an infinite render loop that hangs the runner.
+- `editor-canvas.tsx` imports real `.css`, which this repo's Vite/PostCSS
+  config cannot process under test — both stylesheet imports are mocked to
+  empty modules.
+
+The two sibling template-editor TODOs (`-no-mp-persistence`,
+`-merge-token-resolver`) were re-verified against the code during this work and
+remain **accurate and open**. The new tests document current behaviour; they do
+not paper over either defect.

.claude/TODO/2026-09-13-components-select-uncontrolled-to-controlled.md

@@ -0,0 +1,71 @@
+---
+title: Radix Select fields switched from uncontrolled to controlled on first selection
+severity: medium
+tags: [bug]
+area: components
+files: [src/components/group-wizard/step-attributes.tsx, src/components/group-wizard/step-identity.tsx, src/components/group-wizard/step-meeting.tsx, src/components/group-wizard/step-organization.tsx, src/app/(web)/tools/addeditfamily/add-edit-family.tsx]
+discovered: 2026-09-13
+discovered_by: coverage-review-orchestrator
+status: resolved
+---
+
+## Problem
+Fourteen `Select` fields across the group wizard and Add/Edit Family passed
+`undefined` as their value while nothing was selected:
+
+```tsx
+value={field.value ? String(field.value) : undefined}
+```
+
+A Radix `Select` with `value={undefined}` is **uncontrolled**. As soon as the
+user picked an option, `field.value` became truthy and the component received a
+string — switching to **controlled** mid-life. React logs:
+
+```
+Select is changing from uncontrolled to controlled. Components should not
+switch from controlled to uncontrolled (or vice versa). Decide between using
+a controlled or uncontrolled value for the lifetime of the component.
+```
+
+Beyond the console noise, this is a real correctness problem: while the field
+is uncontrolled, React state is not the source of truth for it. Programmatic
+resets — `form.reset()` between wizard runs, or clearing a field back to its
+empty value — are not guaranteed to be reflected in the rendered trigger,
+because the component keeps its own internal value until it becomes controlled.
+
+The falsy check compounds it. `field.value` of `0` is a legitimate lookup id in
+Ministry Platform, and it is falsy, so a genuinely-selected id of 0 would also
+have rendered as "no selection".
+
+## Evidence
+- `src/components/group-wizard/step-meeting.tsx:49,95,123,151`
+- `src/components/group-wizard/step-attributes.tsx:70,98,126,154`
+- `src/components/group-wizard/step-organization.tsx:59,87,162`
+- `src/components/group-wizard/step-identity.tsx:64,128`
+- `src/app/(web)/tools/addeditfamily/add-edit-family.tsx:1168`
+  (variant: `value={value > 0 ? String(value) : undefined}`)
+- Surfaced by the new step tests — 7 warnings per `vitest run`, e.g.
+  `step-organization.test.tsx > StepOrganization > selects a congregation`.
+
+## Fix applied
+All fourteen sites now pass `""` instead of `undefined`:
+
+```tsx
+value={field.value ? String(field.value) : ""}
+```
+
+`""` is Radix's documented "no selection" value for a controlled `Select`: the
+`SelectValue` placeholder still renders, and the component is controlled for
+its entire lifetime. (`SelectItem` may not use `""` as its own value, which is
+what makes it safe as the sentinel.)
+
+Verified: 1,535 tests pass, `npm run test:run` emits zero warnings, `tsc
+--noEmit` is clean, and `npm run build` succeeds.
+
+## Follow-up worth considering
+The `field.value ? ...` truthiness test is still wrong for a legitimate id of
+`0`. No current lookup uses 0 as a real id, so this is latent rather than
+live — but the correct predicate is `field.value != null`, and a future MP
+lookup that does use 0 would fail silently. Left as-is here because changing
+the predicate is a behavioural change beyond the warning fix, and it deserves
+its own review against the actual lookup data.

.claude/TODO/2026-09-13-dead-empty-fields-branch-handlenext.md

@@ -0,0 +1,66 @@
+---
+title: Unreachable "empty STEP_FIELDS" branch in GroupWizard.handleNext
+severity: low
+tags: [refactor, missing-test]
+area: components
+files: [src/app/(web)/tools/groupwizard/group-wizard.tsx]
+discovered: 2026-09-13
+discovered_by: coverage-agent-group-wizard
+status: open
+---
+
+## Problem
+`handleNext` in `group-wizard.tsx` (lines 120-133) special-cases a step whose
+`STEP_FIELDS[currentStep]` entry is missing or empty by skipping validation
+and advancing immediately:
+
+```ts
+const fields = STEP_FIELDS[currentStep];
+if (!fields || fields.length === 0) {
+  setCompletedSteps((prev) => new Set(prev).add(currentStep));
+  setCurrentStep((prev) => Math.min(prev + 1, WIZARD_STEPS.length - 1));
+  return;
+}
+```
+
+The only entry in `STEP_FIELDS` (see `schema.ts`) with an empty array is
+index `5` (the Review step, `STEP_FIELDS[5] = []`). But `WizardNavigation`
+never renders a "Next" button on the last step (`currentStep === totalSteps - 1`)
+— it renders the Submit/"Save Changes" button instead, which calls
+`handleSubmit`, not `handleNext`. `STEP_FIELDS` is otherwise fully populated
+for indices 0-4, and `currentStep` never exceeds `WIZARD_STEPS.length - 1`
+(5), so this branch is unreachable through the actual UI.
+
+This isn't a security or correctness bug (nothing lets a user skip real
+validation — the branch simply can never execute), but it is 3 statements of
+dead defensive code that both inflates the file's apparent branch count and
+cannot be exercised by any user-facing test, which is why coverage tooling
+flags `group-wizard.tsx` at 94.5% statements instead of ≥95% despite every
+reachable branch being covered.
+
+## Evidence
+- `src/app/(web)/tools/groupwizard/group-wizard.tsx:120-126` — the
+  `!fields || fields.length === 0` branch.
+- `src/components/group-wizard/schema.ts:58-65` — `STEP_FIELDS`, only index
+  `5` is empty, and index 5 is the last step.
+- `src/components/group-wizard/wizard-navigation.tsx:28,55-75` — `isReviewStep`
+  gates rendering Submit vs. Next; the last step never shows the Next button
+  that calls `handleNext`.
+- `npx vitest run --coverage` on `group-wizard.tsx` reports lines 123-125
+  uncovered even after `src/app/(web)/tools/groupwizard/group-wizard.test.tsx`
+  was extended with 12 additional cases covering every other branch/function
+  in the file (94.5% statements / 96.25% lines final).
+
+## Proposed fix
+Either:
+1. Remove the `!fields || fields.length === 0` special case entirely (the
+   `form.trigger([])` call with an empty array resolves `true` immediately
+   in react-hook-form, so simplifying to always call `form.trigger(fields)`
+   would behave identically and remove the dead branch), or
+2. If the branch is meant as future-proofing for a step reachable via some
+   other entry point, add a code comment explaining that intent so the next
+   reader (and coverage tooling) doesn't flag it as untested dead code.
+
+## Impact if not fixed
+None functionally — this is a coverage/clarity issue, not a behavior bug.
+Low priority; safe to leave as documented, known-dead code.

.claude/TODO/2026-09-13-mergetemplate-logs-address-pii-on-error.md

@@ -0,0 +1,58 @@
+---
+title: mergeTemplate (and sibling actions) console.error the raw docxtemplater error, which can carry household addresses
+severity: high
+tags: [security, bug]
+area: components
+files: [src/components/address-labels/actions.ts]
+discovered: 2026-09-13
+discovered_by: coverage-agent-address-labels
+status: open
+---
+
+## Problem
+
+CLAUDE.md rule 14 requires that errors log "identifiers and shape" only — never
+record content — "including inside thrown error messages, which travel further
+than logs do." `mergeTemplate` (and its siblings `generateLabelPdf` /
+`generateLabelDocx`) violate this by passing the entire caught `error` object to
+`console.error`, not just `error.message`.
+
+For `mergeTemplate` this is a concrete PII leak, not a theoretical one:
+docxtemplater's scope-parser error path attaches the live merge scope object to
+`err.properties.scope` (see `node_modules/docxtemplater/js/errors.js:593-600`,
+function that throws `"scopeparser_execution_failed"`). In this feature, the
+merge scope is exactly the `addresses` array built in `mergeTemplate` — each
+entry carries `Name`, `AddressLine1`, `AddressLine2`, `City`, `State`,
+`PostalCode` for a real household. A user-uploaded template with a malformed
+tag (e.g. a token that calls a method that throws, or a bad expression) causes
+docxtemplater to throw with that scope attached, and
+`console.error('mergeTemplate error:', error)` writes the full object —
+including the nested `.properties.scope` with every printable household's
+name and address — to server logs.
+
+## Evidence
+
+- `src/components/address-labels/actions.ts:328` — `console.error('mergeTemplate error:', error);` logs the raw `error`, not `error.message`.
+- `src/components/address-labels/actions.ts:189` and `:232` — same pattern for `generateLabelPdf` / `generateLabelDocx` (lower risk here, but same anti-pattern).
+- `node_modules/docxtemplater/js/errors.js:593-601` — `err.properties.scope = scope;` attaches the full render scope (the caller's data) to certain template errors.
+- `mergeTemplate`'s `addresses` array (`actions.ts:286-296`) contains `Name`, `AddressLine1`, `AddressLine2`, `City`, `State`, `PostalCode` — real household PII — which is exactly what would appear in that scope.
+
+## Proposed fix
+
+Log only identifiers/shape, per rule 14: e.g.
+```ts
+console.error('mergeTemplate error:', error instanceof Error ? error.message : 'non-Error thrown');
+```
+or, if the `docxtemplater` error's `.properties.id`/`.name` is useful for
+diagnosing which failure mode occurred, log those specific fields explicitly
+rather than the object itself — never `error` or `error.properties` in bulk.
+Apply the same change to the `console.error` calls in `generateLabelPdf` and
+`generateLabelDocx` for consistency and defense in depth.
+
+## Impact if not fixed
+
+Any user who uploads a mail-merge template with a tag that fails docxtemplater's
+scope parser causes every printable household's name and mailing address for
+that batch to be written to application/server logs (e.g. Vercel function
+logs), which are typically retained, more widely readable, and less access
+controlled than the MP database itself.

.claude/TODO/2026-09-13-page-logs-raw-error-object.md

@@ -0,0 +1,62 @@
+---
+title: "AddEditFamilyPage logs the raw error object instead of an identifier"
+severity: medium
+tags: [security, drift]
+area: components
+files: [src/app/(web)/tools/addeditfamily/page.tsx]
+discovered: 2026-09-13
+discovered_by: coverage-agent-addeditfamily
+status: open
+---
+
+## Problem
+`AddEditFamilyPage` swallows a failed `resolveContactIdFromPage` call with:
+
+```ts
+console.warn("Failed to resolve Contact_ID from page record:", error);
+```
+
+CLAUDE.md rule 14 requires errors to log identifiers and shape only — never
+record content, `$filter` strings, or request/response bodies, "including
+inside thrown error messages, which travel further than logs do." Logging the
+raw `error` object prints its `.message` (and, depending on the runtime,
+`.stack`), which can carry the interpolated MP `$filter` string built in
+`FamilyService.resolveContactIdFromPage` (`` `${primaryKey} = ${recordId}` ``)
+or other request detail if the underlying `getTableRecords` call or
+`validateColumnName`/`validatePositiveInt` throws with that detail embedded.
+
+The sibling file `src/lib/tool-params.server.ts` (same call-swallowing shape,
+literally a few lines away in the import graph) already gets this right:
+
+```ts
+// Identifier only. A caller without an MP security role also lands
+// here — the tools layout redirects them to /no-access, and the page
+// simply renders without page metadata in the meantime.
+console.warn('tool_params.page_data_unavailable', { pageID: parsedPageID });
+```
+
+`page.tsx` should follow the same pattern instead of passing `error` through.
+
+## Evidence
+- `src/app/(web)/tools/addeditfamily/page.tsx:29` — `console.warn("Failed to resolve Contact_ID from page record:", error)`
+- `src/lib/tool-params.server.ts:58-60` — the correct pattern, logging an identifier object with no error content
+- `src/services/familyService.ts:140-165` — `resolveContactIdFromPage` builds a `$filter` string from `primaryKey`/`recordId` that could surface in a thrown error's `.message`
+
+## Proposed fix
+Replace the `console.warn` call with an identifier-only log, e.g.:
+
+```ts
+console.warn("addeditfamily.resolve_contact_id_failed", {
+  tableName: params.pageData.Table_Name,
+  recordID: params.recordID,
+});
+```
+
+and drop `error` from the log call entirely (or log only `error instanceof Error ? error.name : typeof error` if a coarse classification is wanted).
+
+## Impact if not fixed
+Low likelihood, low blast radius — this only logs when the page-record
+resolution has already failed, and only to server logs. But it is the second
+place after `tool-params.server.ts`'s example that this exact swallow-and-warn
+shape appears, and it is the one that gets it wrong; left uncorrected, it is
+an easy pattern to copy into the next new page.