73424aae59
* 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>
39 lines
3.5 KiB
Markdown
39 lines
3.5 KiB
Markdown
## 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**.
|