40 lines
3.5 KiB
Markdown
40 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**.
|