938c161a26
Give /scanner a md+ camera, live match inspector, and history strip (with device picker, batch scan, and tips) without regressing the mobile immersive checkout. Co-authored-by: Cursor <cursoragent@cursor.com>
21 lines
2.9 KiB
Markdown
21 lines
2.9 KiB
Markdown
## Reviewer Report
|
||
|
||
| Check | Status | Notes |
|
||
| --- | --- | --- |
|
||
| Scope match | ✅ | Working-tree JS/tests match the union of briefs 1–6 `files:` (camera hook, Tips/Result/History components, batch helper, page + Camera + queue/identification hooks, matching tests). Convoy docs under `.convoys/scanner-desktop-layout/` expected. No out-of-list app code. **Note:** branch tip is behind `origin/main` (merge-base `0d52858`); a naive `git diff origin/main` includes unrelated dashboard-home-realignment reversals — rebase before PR. |
|
||
| Conventions | ✅ | `VOCAB` used for add actions; `<Modal>` / `GlassSurface` / `Button`; CSS variables (no hex / forbidden stale strings in scanner surfaces); relative imports; no new API routes or schema. |
|
||
| Security | ✅ | No new auth/API surfaces; client-only. Device id in `sessionStorage` only. Depth deferred to `role-security-auditor` in this fan-out. |
|
||
| Regression risk | medium | Touches `/scanner` composition, `useCameraScanner` stream constraints, and `verificationPausedRef` ownership (identification overwrite removed). Mobile chrome gated via `variant` + `isDesktop`, but pause/batch heuristics are easy to break. |
|
||
| Test coverage | ✅ | 45 tests green across briefs 1–6 artifacts (`use-camera-scanner`, batch helper, Tips/Result/History/Camera, page viewport split). Gaps: checkout sheet not exercised open on mobile; Identifying… never wired to real in-flight identify. |
|
||
| Documentation | ✅ | Convoy + briefs present; no AGENTS.md change required for this UI composition. |
|
||
|
||
### Findings
|
||
|
||
- 🟡 **Suggestion:** `isIdentifying={Boolean(identification.disambiguation)}` in `pages/scanner.js` does not match Brief 3’s “Identifying…” intent — disambiguation is post-match choice, and the hook has no in-flight identify flag. Empty rail will not show Identifying… during real scans; it may flash the wrong copy if disambiguation opens with no focused card. Wire a real verifying signal or leave `false` until one exists.
|
||
- 🟡 **Suggestion:** Batch Scan does not switch the strip to **Scan Queue** when `batchProgress.active` (Design direction / strip UX). Progress banner only appears if the user is already on that tab.
|
||
- 🟡 **Suggestion:** Device picker messages use short status strings (`No camera found`, etc.) but Design direction’s locked table includes action hints (e.g. “Connect a webcam or use Upload Image”). Align copy with the Design table.
|
||
- 🟡 **Suggestion:** `test/pages/scanner.test.js` asserts the checkout sheet is absent when closed on mobile, but Brief 6 asks for no-regression coverage of the sheet at narrow width — open `isCheckoutOpen` (or equivalent) and assert the sheet mounts under `max-md`.
|
||
- 🟢 **Nice to have:** Rebase/merge `origin/main` before opening the PR so the diff does not look like a dashboard revert; implementation itself stays in-scope once compared to the brief file union.
|
||
|
||
### Approval recommendation
|
||
- approve
|