rstillwell/deckhearth
1
0
Fork 0
Code Issues 1 Pull requests Projects Releases Packages Wiki Activity Actions
deckhearth/.convoys/scanner-mobile-checkout/audits/reviewer-20260814.md
varutasu 73424aae59
Mobile scanner checkout: scan first, commit later (#157) 
* Start scanner-mobile-checkout convoy for the cart-then-commit phone flow.

Co-authored-by: Cursor <cursoragent@cursor.com>

* Ship a cart-then-commit mobile scanner so phone sessions stay on the camera.

Scan matches enqueue locally instead of auto-writing ownership, checkout happens in a sheet, and audit fixes cover stale commit detection, returnUrl open redirects, nested Escape, and ember detection chrome.

Co-authored-by: Cursor <cursoragent@cursor.com>

---------

Co-authored-by: Cursor <cursoragent@cursor.com>
2026-08-14 20:20:43 -05:00

3.5 KiB
Raw Blame History

Reviewer Report

Check Status Notes
Scope match ✅ All app changes map to briefs 1–4 files:; no unauthorized expansion. Convoy docs/metrics only extras.
Conventions ✅ Vocab via VOCAB; CSS tokens / GlassSurface; Modal for leave + list picker; login returnUrl hardened (/ only, blocks // and ://).
Security ✅ Shallow L1–L2: no new API routes; client cart in sessionStorage; returnUrl open-redirect guarded. Defer depth to security-auditor when PR exists.
Regression risk medium Scanner page rewritten (phase machine removed); auto-route deleted (intentional); Layout immersive gated by prop but TopSearchBar/MobileNavigation paths change for /scanner.
Test coverage ⚠️ Brief-required suites green (27/27). Commit success/error detection untested and currently wrong (see Critical).
Documentation ✅ No AGENTS.md / schema updates required for this convoy.

Findings

  • 🔴 Critical (must fix before merge): Stale React state used to detect commit failure in ScannerCheckoutContent.handleCommitOwned and pages/scanner.js handleListPick. After await queue.commitSelectedToOwned() / commitSelectedToCollection(), queue.scannedCards is still the pre-commit render snapshot, so a successful commit looks like “all remaining” → false error banner; list picker never closes on success (setIsListPickerOpen(false) skipped). Fix: have commit helpers return successfulIds (or a boolean), and branch on that — do not re-read hook state immediately after await.
  • 🟡 Suggestion (consider): ReviewCardItem still mounts the per-row destination <select> when !showCheckbox. Checkout path is fine (showCheckbox), but Brief 4 asked to remove inline destination pickers; dead path adds confusion and retains pre-existing #ffffff on that control.
  • 🟡 Suggestion (consider): Add a focused test that commit success returns/propagates success without relying on immediate queue.scannedCards re-read (locks the Critical fix). Optional: login returnUrl honor + reject open redirect.
  • 🟢 Nice to have (optional): Convoy markdown todos still unchecked despite implemented work — conductor/doc-writer hygiene, not a merge blocker.

Approval recommendation

  • request-changes

AC evidence (briefs 1–4)

Brief AC Evidence
1 Immersive hides mobile nav / TopSearchBar below md Layout.js !isImmersive MobileNav; TopSearchBar max-md:hidden; tests added
1 Default chrome unchanged chrome = 'default'; regression tests
2 No fetch in handleCardScanned Auto-route block removed; queue test asserts no route helpers
2 sessionStorage cart SCANNER_CART_STORAGE_KEY + load/save/clear + tests
2 Commit removes rows successfulIds filter in handleBulkAction
3 Facing swap + flash rear-only switchFacingMode; showFlash env-only; torch off on user
3 Peek 3s / toast SVG / gallery adapter ScannerScanPeek, SVG ToastIcon, identifyFromGalleryFile
4 No Setup; returnUrl; sheet + pause; leave Modal pages/scanner.js + hardened login.js
4 Desktop side panel ScannerReview variant="side-panel"
4 Checkout sheet tests header count, disabled primary, vocab labels

Test run

npm run test:run -- test/components/Layout.test.js test/lib/scanner-session.test.js test/lib/use-scanner-queue.test.js test/components/ScannerCheckoutSheet.test.js → 27/27 passed.