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); + }); +});