Randall Stillwell 237870c17e convoy: render-test regression-lock for ScanDisambiguationDialog (PR #144 )

PR #144 (`31da384`, 2026-06-13) shipped a runtime
`ReferenceError: useFocusTrap is not defined` to production because
the component called the hook without importing it. The sibling
`enable-no-undef-eslint-rule` convoy closes that bug class at LINT
time. This PR locks the same regression at RENDER time so the bug
would still fail CI even if the lint rule were dropped or disabled.

## What changes

- `test/components/ScanDisambiguationDialog.test.js` — 8 tests:

  1. `renders without crashing (PR #144 regression-lock)` — the
     direct lock-in. Mutation-tested: commenting out the
     `useFocusTrap` import causes all 8 tests to fail with the same
     `ReferenceError` shape that hit prod.
  2. `returns null when disambiguation is falsy`
  3. ARIA shape (`role`, `aria-modal`, `aria-labelledby`)
  4. One button per candidate with accessible labels
  5. `onPick` callback receives the selected candidate
  6. Vision-hint branch renders when provided
  7. Submitting state disables the "send for review" button
  8. `onCancel` callback fires on Cancel click

## Why vitest + jsdom and not Playwright smoke

| Path | Catches PR #144 | Setup | Runtime |
|------|-----------------|-------|---------|
| Playwright smoke | ✓ if disambiguation mounts in the smoke run | High (auth bypass, stable multi-candidate fixture image) | ~10s + browser |
| Vitest render | ✓ directly — render-throw → test fail | Low | <100ms |

Re-scoped the queued `scanner-disambiguation-smoke-test` task to the
vitest shape because a render test catches the exact same bug class
at 1/100th the cost and matches the existing `test/components/*.test.js`
pattern (`Modal.test.js`, `ScannedCardItem.test.js`, etc.). A Playwright
disambiguation smoke is still useful as integration-layer coverage and
is queued as `scanner-disambiguation-playwright-smoke`.

## Verification

- [x] `npm run test:run` — 26 files / 131 tests pass (up from 25/123)
- [x] Mutation test: with `useFocusTrap` import commented out, all 8
      tests fail with `ReferenceError`. With import restored, all pass.

## Test plan

- [ ] CI on this PR green
- [ ] Squash + merge
- [ ] Smoke test post-merge: scan a card that triggers disambiguation
      in prod and confirm no console errors (the original PR #144 bug
      shape)

## Convoy doc

`.convoys/scanner-disambiguation-render-test.md` documents D1 (cover
the early-return branch explicitly), D2 (`fireEvent` not `userEvent`),
D3 (do NOT mock `useFocusTrap` — the missing-hook is exactly what
we're locking), and the two queued follow-ups
(`add-component-render-smoke-pattern`, `scanner-disambiguation-playwright-smoke`).

Co-authored-by: Cursor <cursoragent@cursor.com>

2026-06-13 01:20:21 -05:00

5.1 KiB

Raw Permalink Blame History

slug: scanner-disambiguation-render-test status: shipping opened: 2026-06-13 owner: rstillw related:

PR #144 (31da384) — the runtime useFocusTrap is not defined ReferenceError this test locks in against
enable-no-undef-eslint-rule convoy — sibling that closes the same bug class at lint time

scanner-disambiguation-render-test

Problem

PR #144 shipped a runtime ReferenceError: useFocusTrap is not defined to production because ScanDisambiguationDialog called a hook it never imported. The sibling enable-no-undef-eslint-rule convoy closes that bug class at lint time. This convoy locks the regression at render time as well, so even if someone disables the lint rule (or a future rule-set change drops it), the same bug would still fail CI.

Why vitest + jsdom instead of Playwright smoke

The task was originally queued as "scanner-disambiguation-smoke-test." Re-scoped because:

Path	Catches PR #144	Setup cost	Run time
Playwright smoke	✓ if the disambiguation modal mounts during the smoke run	High — need auth bypass, a real way to enter the disambiguation state (multiple-candidate scan), and a stable fixture image	~10s + browser overhead
Vitest + @testing-library/react	✓ directly — the render-throw is caught by the test	Low — fixture is plain JS, props are explicit	<100ms

A vitest render test catches the exact same bug class (ReferenceError during component render) at 1/100th the cost, and matches the existing test/components/*.test.js pattern (Modal.test.js, ScannedCardItem.test.js, etc.). A Playwright smoke test of the disambiguation flow could be added later as an integration-coverage layer but is not the right tool for regression-locking THIS specific bug class.

The deferred Playwright disambiguation smoke test is queued separately (see § Follow-ups).

What ships

test/components/ScanDisambiguationDialog.test.js — 8 tests:

renders without crashing when given a disambiguation (PR #144 regression-lock) — the cheapest, most direct regression-lock. If useFocusTrap (or any other imported identifier) is missing, render() throws and this assertion fails. Comment in the test calls this out by name.
returns null when disambiguation prop is falsy — the early-return branch
renders dialog with the correct ARIA shape — role="dialog", aria-modal, aria-labelledby
renders one button per candidate with accessible labels
calls onPick with the selected candidate
renders the vision hint when one is provided — exercises the optional disambiguation.visionHint branch
disables the "send for review" button while submitting and shows in-flight copy
calls onCancel when the Cancel button is clicked

Verification

Mutation test executed locally: commented out the useFocusTrap import → all 8 tests fail with the same ReferenceError shape that hit prod in PR #144. Restored the import → all 8 pass. Full suite: 26 files / 131 tests pass (up from 25 / 123 pre-PR).

Decisions

D1. Cover the early-return branch explicitly even though it's a one-liner. Cost is negligible (~2 lines) and it locks the if (!disambiguation) return null; semantics — a future refactor that returns a placeholder instead would intentionally break this test and force a review of the contract change.
D2. Use fireEvent for click handlers, not userEvent. userEvent is more realistic but adds a dependency (@testing-library/user-event) for marginal gain on these simple button-click assertions. Matches existing test pattern in Modal.test.js.
D3. Do NOT mock useFocusTrap. The hook is real and runs against jsdom. Reasoning: the original PR #144 bug was that the hook was missing; mocking would mask exactly the kind of failure this test is designed to catch.

Follow-ups (queued, not in this PR)

add-component-render-smoke-pattern — sweep the other components that conditionally mount (UploadImageModal, CollectionEditModal, CollectionDeleteModal, ShareModal, CollaboratorFacepile's expanded view, etc.) and add a minimal "renders without crashing with realistic props" test to each. Same regression-lock value, one PR per ~5 components.
scanner-disambiguation-playwright-smoke — add a Playwright smoke test that exercises the full scan → ambiguous-match → pick-candidate flow against a deployed preview. Higher value as integration-layer coverage, but blocked on (a) a stable test fixture image that consistently produces multiple-candidate OCR matches and (b) auth bypass for the scanner route. Deferred.

Acceptance

test/components/ScanDisambiguationDialog.test.js exists with the 8 listed assertions
Mutation test confirmed all 8 tests fail when the useFocusTrap import is removed
Full vitest suite passes (26 files / 131 tests)
CI on the PR green

Non-goals

Adding @testing-library/user-event (use fireEvent to match existing pattern)
Adding a Playwright smoke for this flow (queued separately)
Adding render tests for other modal components (queued separately)
Refactoring the component itself (it's already well-scoped post-PR #144 fix)

5.1 KiB Raw Permalink Blame History