deckhearth/.convoys/scanner-disambiguation-render-test.md

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

1. **`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.
2. `returns null when disambiguation prop is falsy` — the early-return branch
3. `renders dialog with the correct ARIA shape` — `role="dialog"`, `aria-modal`, `aria-labelledby`
4. `renders one button per candidate with accessible labels`
5. `calls onPick with the selected candidate`
6. `renders the vision hint when one is provided` — exercises the optional `disambiguation.visionHint` branch
7. `disables the "send for review" button while submitting and shows in-flight copy`
8. `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

- [x] `test/components/ScanDisambiguationDialog.test.js` exists with the 8 listed assertions
- [x] Mutation test confirmed all 8 tests fail when the `useFocusTrap` import is removed
- [x] 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)