rstillwell/deckhearth
1
0
Fork 0
Code Issues 1 Pull requests Projects Releases Packages Wiki Activity Actions
deckhearth/.convoys/scanner-desktop-layout/audits/reviewer-20260815.md
varutasu 938c161a26
feat(scanner): add desktop workstation layout (#165) 
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>
2026-08-15 17:21:23 -05:00

2.9 KiB
Raw Blame History

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