From 237870c17eb454f158595a7e16352896405a8393 Mon Sep 17 00:00:00 2001 From: Randall Stillwell Date: Sat, 13 Jun 2026 01:20:21 -0500 Subject: [PATCH] convoy: render-test regression-lock for ScanDisambiguationDialog (PR #144) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .convoys/.metrics.jsonl | 2 + .../scanner-disambiguation-render-test.md | 70 ++++++++ .../ScanDisambiguationDialog.test.js | 169 ++++++++++++++++++ 3 files changed, 241 insertions(+) create mode 100644 .convoys/scanner-disambiguation-render-test.md create mode 100644 test/components/ScanDisambiguationDialog.test.js diff --git a/.convoys/.metrics.jsonl b/.convoys/.metrics.jsonl index f6f170a..3b9bb04 100644 --- a/.convoys/.metrics.jsonl +++ b/.convoys/.metrics.jsonl @@ -72,3 +72,5 @@ {"ts": "2026-06-13T04:14:48Z", "role": "role-implementer", "convoy": "rotate-default-admin", "repo": "tcg-vault", "skip_flags": [], "classification": "security", "duration_s": 900, "outcome": "pr-open"} {"ts": "2026-06-13T06:16:56Z", "role": "role-architect", "convoy": "enable-no-undef-eslint-rule", "repo": "tcg-vault", "skip_flags": [], "classification": "ci", "duration_s": 180, "outcome": "architecture-only"} {"ts": "2026-06-13T06:16:56Z", "role": "role-implementer", "convoy": "enable-no-undef-eslint-rule", "repo": "tcg-vault", "skip_flags": [], "classification": "ci", "duration_s": 900, "outcome": "pr-open"} +{"ts": "2026-06-13T06:20:00Z", "role": "role-architect", "convoy": "scanner-disambiguation-render-test", "repo": "tcg-vault", "skip_flags": [], "classification": "ci", "duration_s": 180, "outcome": "architecture-only"} +{"ts": "2026-06-13T06:20:00Z", "role": "role-implementer", "convoy": "scanner-disambiguation-render-test", "repo": "tcg-vault", "skip_flags": [], "classification": "ci", "duration_s": 600, "outcome": "pr-open"} diff --git a/.convoys/scanner-disambiguation-render-test.md b/.convoys/scanner-disambiguation-render-test.md new file mode 100644 index 0000000..c0a905d --- /dev/null +++ b/.convoys/scanner-disambiguation-render-test.md @@ -0,0 +1,70 @@ +--- +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) diff --git a/test/components/ScanDisambiguationDialog.test.js b/test/components/ScanDisambiguationDialog.test.js new file mode 100644 index 0000000..bea716f --- /dev/null +++ b/test/components/ScanDisambiguationDialog.test.js @@ -0,0 +1,169 @@ +// @vitest-environment jsdom +// +// Regression lock for PR #144 (`31da384`, 2026-06-13) — `useFocusTrap` +// was called by this component but never imported, shipping a runtime +// `ReferenceError: useFocusTrap is not defined` to production. The +// `enable-no-undef-eslint-rule` convoy closes that bug class at lint +// time; this file additionally exercises the component at render time +// so the regression is locked in BOTH static-analysis AND +// dynamic-execution paths. A future identical bug (call a hook without +// importing it) would fail this test even if someone disabled the +// lint rule. +// +// The "renders without crashing" test is intentionally the FIRST +// assertion — it's the cheapest failure mode and most directly tied +// to the original PR #144 regression. +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { render, screen, fireEvent, cleanup } from '@testing-library/react'; +import ScanDisambiguationDialog from '../../components/ScanDisambiguationDialog'; + +const baseFixture = { + message: 'Multiple matches found. Pick one.', + candidates: [ + { + id: 'card-1', + name: 'Lightning Bolt', + set_name: 'Alpha', + set_code: 'LEA', + card_number: '161', + image_url: 'https://cards.scryfall.io/normal/front/1/1.jpg', + }, + { + id: 'card-2', + name: 'Lightning Bolt', + set_name: 'Beta', + set_code: 'LEB', + card_number: '161', + // intentionally no image — exercises the placeholder branch + }, + ], +}; + +const noop = () => {}; + +describe('ScanDisambiguationDialog', () => { + afterEach(() => cleanup()); + + // ── Regression-lock for PR #144 ───────────────────────────────────── + // If `useFocusTrap` (or any other imported identifier) is missing, + // React's render throws `ReferenceError` from inside the function + // component, and this assertion fails. Do NOT relax this test to + // accept thrown errors — that's the entire point. + it('renders without crashing when given a disambiguation (PR #144 regression-lock)', () => { + expect(() => { + render( + + ); + }).not.toThrow(); + }); + + it('returns null when disambiguation prop is falsy', () => { + const { container } = render( + + ); + expect(container.querySelector('[role="dialog"]')).toBeNull(); + }); + + it('renders dialog with the correct ARIA shape', () => { + render( + + ); + const dialog = screen.getByRole('dialog'); + expect(dialog.getAttribute('aria-modal')).toBe('true'); + expect(dialog.getAttribute('aria-labelledby')).toBe('disambiguation-title'); + expect(screen.getByText('Which card is this?')).toBeTruthy(); + expect(screen.getByText(baseFixture.message)).toBeTruthy(); + }); + + it('renders one button per candidate with accessible labels', () => { + render( + + ); + const buttons = screen.getAllByRole('button', { name: /Select Lightning Bolt/i }); + expect(buttons).toHaveLength(2); + expect(buttons[0].getAttribute('aria-label')).toContain('Alpha'); + expect(buttons[1].getAttribute('aria-label')).toContain('Beta'); + }); + + it('calls onPick with the selected candidate', () => { + const onPick = vi.fn(); + render( + + ); + fireEvent.click(screen.getByRole('button', { name: /Select Lightning Bolt, Alpha/i })); + expect(onPick).toHaveBeenCalledTimes(1); + expect(onPick).toHaveBeenCalledWith(baseFixture.candidates[0]); + }); + + it('renders the vision hint when one is provided', () => { + render( + + ); + expect(screen.getByText(/Vision detected set: LEA/i)).toBeTruthy(); + }); + + it('disables the "send for review" button while submitting and shows in-flight copy', () => { + render( + + ); + const reviewBtn = screen.getByRole('button', { name: /Submitting/i }); + expect(reviewBtn.hasAttribute('disabled')).toBe(true); + }); + + it('calls onCancel when the Cancel button is clicked', () => { + const onCancel = vi.fn(); + render( + + ); + fireEvent.click(screen.getByRole('button', { name: /^Cancel$/i })); + expect(onCancel).toHaveBeenCalledTimes(1); + }); +});