feat(scan): premium scanner UX improvements (#784)#820
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Bundle Size Report
✅ Bundle size is within acceptable limits. |
There was a problem hiding this comment.
Pull request overview
Improves the premium barcode scanner UX by adding an in-page “product found” preview flow, scan inactivity guidance, and small usability enhancements for manual entry—backed by updated unit tests and i18n strings.
Changes:
- Replace immediate navigation on successful scan with a “Found” preview overlay (View Details / Scan Next).
- Add a 15s idle “Having trouble?” timeout hint, plus animated scan line and scanning status text.
- Add a “Paste barcode” button in manual mode and expand scanner tests + translations (en/pl/de).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/app/app/scan/page.tsx | Adds found preview overlay, timeout UI, scan line animation, status text, and manual paste button. |
| frontend/src/app/app/scan/page.test.tsx | Updates existing scanner tests and adds new tests for found preview, score band, nutri-score, paste button, and timeout. |
| frontend/messages/en.json | Adds new scanner UX i18n keys (status, timeout, found preview, paste). |
| frontend/messages/pl.json | Adds Polish translations for the new scanner UX keys. |
| frontend/messages/de.json | Adds German translations for the new scanner UX keys. |
| {/* Scanning status */} | ||
| {!scanTimeout && ( | ||
| <p className="animate-pulse text-center text-sm text-foreground-secondary"> | ||
| {t("scan.scanningStatus")} | ||
| </p> | ||
| )} | ||
|
|
||
| {/* Timeout hint */} | ||
| {scanTimeout && ( | ||
| <div className="card border-warning-border bg-warning-bg text-center"> | ||
| <p className="text-sm font-semibold text-warning-text"> | ||
| {t("scan.timeoutTitle")} | ||
| </p> | ||
| <p className="mt-1 text-xs text-warning-text/80"> | ||
| {t("scan.timeoutHint")} | ||
| </p> | ||
| </div> |
There was a problem hiding this comment.
The scanning status text / timeout hint is meant to guide the user, but it isn’t in an aria-live region, so screen readers likely won’t announce it when it appears/changes. Consider rendering it as an <output aria-live="polite"> / role="status" (or using the existing LiveRegion pattern) so the status update is accessible.
| if (u >= 1 && u <= 20) return { band: "green", label: "Excellent", color: "#22c55e", bgColor: "#dcfce7", textColor: "#166534" }; | ||
| if (u >= 21 && u <= 40) return { band: "yellow", label: "Good", color: "#eab308", bgColor: "#fef9c3", textColor: "#854d0e" }; | ||
| if (u >= 41 && u <= 60) return { band: "orange", label: "Moderate", color: "#f97316", bgColor: "#fff7ed", textColor: "#9a3412" }; | ||
| if (u >= 61 && u <= 80) return { band: "red", label: "Poor", color: "#ef4444", bgColor: "#fef2f2", textColor: "#991b1b" }; | ||
| if (u >= 81 && u <= 100) return { band: "darkred", label: "Bad", color: "#991b1b", bgColor: "#fef2f2", textColor: "#7f1d1d" }; |
There was a problem hiding this comment.
The @/lib/score-utils mock doesn’t match the real ScoreBand shape: in production bgColor/textColor are Tailwind class strings (e.g. bg-score-green/10), but the mock returns hex colors. This can hide UI bugs and makes the tests less representative. Either use the real score-utils implementation in these tests (it’s deterministic), or adjust the mock to return the same kinds of values as production.
| if (u >= 1 && u <= 20) return { band: "green", label: "Excellent", color: "#22c55e", bgColor: "#dcfce7", textColor: "#166534" }; | |
| if (u >= 21 && u <= 40) return { band: "yellow", label: "Good", color: "#eab308", bgColor: "#fef9c3", textColor: "#854d0e" }; | |
| if (u >= 41 && u <= 60) return { band: "orange", label: "Moderate", color: "#f97316", bgColor: "#fff7ed", textColor: "#9a3412" }; | |
| if (u >= 61 && u <= 80) return { band: "red", label: "Poor", color: "#ef4444", bgColor: "#fef2f2", textColor: "#991b1b" }; | |
| if (u >= 81 && u <= 100) return { band: "darkred", label: "Bad", color: "#991b1b", bgColor: "#fef2f2", textColor: "#7f1d1d" }; | |
| if (u >= 1 && u <= 20) | |
| return { | |
| band: "green", | |
| label: "Excellent", | |
| color: "#22c55e", | |
| bgColor: "bg-score-green/10", | |
| textColor: "text-score-green", | |
| }; | |
| if (u >= 21 && u <= 40) | |
| return { | |
| band: "yellow", | |
| label: "Good", | |
| color: "#eab308", | |
| bgColor: "bg-score-yellow/10", | |
| textColor: "text-score-yellow", | |
| }; | |
| if (u >= 41 && u <= 60) | |
| return { | |
| band: "orange", | |
| label: "Moderate", | |
| color: "#f97316", | |
| bgColor: "bg-score-orange/10", | |
| textColor: "text-score-orange", | |
| }; | |
| if (u >= 61 && u <= 80) | |
| return { | |
| band: "red", | |
| label: "Poor", | |
| color: "#ef4444", | |
| bgColor: "bg-score-red/10", | |
| textColor: "text-score-red", | |
| }; | |
| if (u >= 81 && u <= 100) | |
| return { | |
| band: "darkred", | |
| label: "Bad", | |
| color: "#991b1b", | |
| bgColor: "bg-score-darkred/10", | |
| textColor: "text-score-darkred", | |
| }; |
| // Spy on setTimeout — verify the 15 s scan timeout is registered | ||
| const spy = vi.spyOn(globalThis, "setTimeout"); | ||
|
|
||
| render(<ScanPage />, { wrapper: createWrapper() }); | ||
|
|
||
| // Wait for camera to fully initialise | ||
| await waitFor(() => { | ||
| expect(mockDecodeFromDevice).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| // The timeout useEffect should have called setTimeout(fn, 15_000) | ||
| expect(spy).toHaveBeenCalledWith(expect.any(Function), 15_000); | ||
|
|
||
| spy.mockRestore(); |
There was a problem hiding this comment.
The timeout test only asserts that setTimeout(..., 15_000) was registered, but it doesn’t verify that the timeout hint actually renders after the timer fires. Using fake timers (vi.useFakeTimers() + advanceTimersByTime(15_000)) and asserting the banner content would better validate the user-visible behavior.
| // Spy on setTimeout — verify the 15 s scan timeout is registered | |
| const spy = vi.spyOn(globalThis, "setTimeout"); | |
| render(<ScanPage />, { wrapper: createWrapper() }); | |
| // Wait for camera to fully initialise | |
| await waitFor(() => { | |
| expect(mockDecodeFromDevice).toHaveBeenCalled(); | |
| }); | |
| // The timeout useEffect should have called setTimeout(fn, 15_000) | |
| expect(spy).toHaveBeenCalledWith(expect.any(Function), 15_000); | |
| spy.mockRestore(); | |
| vi.useFakeTimers(); | |
| try { | |
| render(<ScanPage />, { wrapper: createWrapper() }); | |
| // Wait for camera to fully initialise | |
| await waitFor(() => { | |
| expect(mockDecodeFromDevice).toHaveBeenCalled(); | |
| }); | |
| // Simulate 15 seconds passing to trigger the timeout hint | |
| await act(async () => { | |
| vi.advanceTimersByTime(15_000); | |
| }); | |
| // Verify that the user-visible timeout hint is shown | |
| expect( | |
| screen.getByText(/taking too long/i), | |
| ).toBeInTheDocument(); | |
| } finally { | |
| vi.useRealTimers(); | |
| } |
| className="inline-flex items-center gap-1 rounded-full px-3 py-1 text-sm font-semibold" | ||
| style={{ backgroundColor: band.bgColor, color: band.textColor }} |
There was a problem hiding this comment.
getScoreBand() returns Tailwind class names for bgColor/textColor (e.g. bg-score-green/10, text-score-green-text), but this uses them as CSS color values via the style prop. That will result in invalid CSS and the score pill won’t be styled as intended. Use band.bgColor/band.textColor in className (or use band.color for inline styling) instead of assigning them to backgroundColor/color.
| className="inline-flex items-center gap-1 rounded-full px-3 py-1 text-sm font-semibold" | |
| style={{ backgroundColor: band.bgColor, color: band.textColor }} | |
| className={`inline-flex items-center gap-1 rounded-full px-3 py-1 text-sm font-semibold ${band.bgColor} ${band.textColor}`} |
| {/* Animated scan line */} | ||
| <div | ||
| className="absolute left-2 right-2 h-0.5 bg-error/70" | ||
| style={{ | ||
| animation: "scanLine 2s ease-in-out infinite", | ||
| top: "50%", | ||
| }} | ||
| /> | ||
| <style>{`@keyframes scanLine { 0%,100% { top: 15%; } 50% { top: 85%; } }`}</style> |
There was a problem hiding this comment.
The animated scan line injects a <style> tag with global @keyframes inside the component render. This can add duplicate style tags over time and the keyframe name is global (possible collisions). Prefer moving the keyframes to a stylesheet/Tailwind config (or a CSS module) and referencing it via a class/utility.
| <Button | ||
| variant="secondary" | ||
| onClick={() => handleReset()} | ||
| className="flex-1" | ||
| > | ||
| {t("scan.scanNext")} | ||
| </Button> |
There was a problem hiding this comment.
Scan Next currently calls handleReset() which forces mode back to "camera" (because keepCamera defaults to false). This is a concrete behavior change for manual lookups: after finding a product via manual entry, Scan Next unexpectedly switches the user into camera mode. Consider preserving the current mode for Scan Next (e.g., call handleReset(true) or adjust handleReset to support “keep current mode” explicitly).
| className="absolute left-2 right-2 h-0.5 bg-error/70" | ||
| style={{ | ||
| animation: "scanLine 2s ease-in-out infinite", | ||
| top: "50%", | ||
| }} | ||
| /> | ||
| <style>{`@keyframes scanLine { 0%,100% { top: 15%; } 50% { top: 85%; } }`}</style> |
There was a problem hiding this comment.
The scan line uses continuous animation but doesn’t appear to respect prefers-reduced-motion. Consider disabling/reducing the animation for users who prefer reduced motion (e.g., via Tailwind motion-safe: / motion-reduce: utilities or a media query that removes the animation).
| className="absolute left-2 right-2 h-0.5 bg-error/70" | |
| style={{ | |
| animation: "scanLine 2s ease-in-out infinite", | |
| top: "50%", | |
| }} | |
| /> | |
| <style>{`@keyframes scanLine { 0%,100% { top: 15%; } 50% { top: 85%; } }`}</style> | |
| className="absolute left-2 right-2 h-0.5 bg-error/70 scan-line" | |
| style={{ | |
| top: "50%", | |
| }} | |
| /> | |
| <style>{` | |
| @media (prefers-reduced-motion: no-preference) { | |
| @keyframes scanLine { | |
| 0%, 100% { top: 15%; } | |
| 50% { top: 85%; } | |
| } | |
| .scan-line { | |
| animation: scanLine 2s ease-in-out infinite; | |
| } | |
| } | |
| `}</style> |
| style={{ | ||
| animation: "scanLine 2s ease-in-out infinite", | ||
| top: "50%", | ||
| }} | ||
| /> | ||
| <style>{`@keyframes scanLine { 0%,100% { top: 15%; } 50% { top: 85%; } }`}</style> |
There was a problem hiding this comment.
Injecting a <style> tag inside the component render makes the animation harder to discover/reuse and can lead to duplicated style tags if the markup is reused elsewhere. Consider moving @keyframes scanLine to a stylesheet (global CSS/CSS module) and referencing it via a class to keep styling concerns out of the JSX.
| style={{ | |
| animation: "scanLine 2s ease-in-out infinite", | |
| top: "50%", | |
| }} | |
| /> | |
| <style>{`@keyframes scanLine { 0%,100% { top: 15%; } 50% { top: 85%; } }`}</style> | |
| ref={(el) => { | |
| if (el && !el.dataset.animated) { | |
| el.dataset.animated = "true"; | |
| el.animate( | |
| [ | |
| { top: "15%" }, | |
| { top: "85%" }, | |
| { top: "15%" }, | |
| ], | |
| { | |
| duration: 2000, | |
| iterations: Infinity, | |
| easing: "ease-in-out", | |
| } | |
| ); | |
| } | |
| }} | |
| style={{ | |
| top: "50%", | |
| }} | |
| /> |
| > | ||
| {foundProduct.nutri_score} | ||
| </span> | ||
| <span className="text-xs text-foreground-muted">Nutri-Score</span> |
There was a problem hiding this comment.
"Nutri-Score" is hardcoded UI text while surrounding copy uses i18n. Consider adding an i18n key for this label (or reusing an existing one) so it’s translated consistently across locales.
| <span className="text-xs text-foreground-muted">Nutri-Score</span> | |
| <span className="text-xs text-foreground-muted"> | |
| {t("scan.nutriScore")} | |
| </span> |
| it("shows timeout hint after 15 seconds of camera scanning", async () => { | ||
| mockListDevices.mockResolvedValue([ | ||
| { deviceId: "cam1", label: "Back Camera" } as MediaDeviceInfo, | ||
| ]); | ||
|
|
||
| // Spy on setTimeout — verify the 15 s scan timeout is registered | ||
| const spy = vi.spyOn(globalThis, "setTimeout"); | ||
|
|
||
| render(<ScanPage />, { wrapper: createWrapper() }); | ||
|
|
||
| // Wait for camera to fully initialise | ||
| await waitFor(() => { | ||
| expect(mockDecodeFromDevice).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| // The timeout useEffect should have called setTimeout(fn, 15_000) | ||
| expect(spy).toHaveBeenCalledWith(expect.any(Function), 15_000); | ||
|
|
||
| spy.mockRestore(); | ||
| }); |
There was a problem hiding this comment.
This test asserts that setTimeout is registered, but it doesn’t verify the user-facing behavior: that after 15 seconds the timeout hint banner renders (and that the scanning status text is hidden). Consider using fake timers (advance by 15s) and asserting the presence of scan.timeoutTitle/scan.timeoutHint content in the DOM.
| {/* Scanning status */} | ||
| {!scanTimeout && ( | ||
| <p className="animate-pulse text-center text-sm text-foreground-secondary"> | ||
| {t("scan.scanningStatus")} | ||
| </p> | ||
| )} | ||
|
|
||
| {/* Timeout hint */} | ||
| {scanTimeout && ( | ||
| <div className="card border-warning-border bg-warning-bg text-center"> | ||
| <p className="text-sm font-semibold text-warning-text"> | ||
| {t("scan.timeoutTitle")} | ||
| </p> | ||
| <p className="mt-1 text-xs text-warning-text/80"> | ||
| {t("scan.timeoutHint")} | ||
| </p> | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
The new scanning status text is visually shown, but it isn’t exposed as a live region (e.g. role="status"/aria-live) so screen readers may not announce it (and the later timeout hint) when it appears/changes. Consider using the existing LiveRegion pattern or adding role="status" aria-live="polite" aria-atomic around these messages.
| {/* Scanning status */} | |
| {!scanTimeout && ( | |
| <p className="animate-pulse text-center text-sm text-foreground-secondary"> | |
| {t("scan.scanningStatus")} | |
| </p> | |
| )} | |
| {/* Timeout hint */} | |
| {scanTimeout && ( | |
| <div className="card border-warning-border bg-warning-bg text-center"> | |
| <p className="text-sm font-semibold text-warning-text"> | |
| {t("scan.timeoutTitle")} | |
| </p> | |
| <p className="mt-1 text-xs text-warning-text/80"> | |
| {t("scan.timeoutHint")} | |
| </p> | |
| </div> | |
| )} | |
| {/* Scanning status and timeout messages (live region for SRs) */} | |
| <div | |
| role="status" | |
| aria-live="polite" | |
| aria-atomic="true" | |
| className="space-y-2" | |
| > | |
| {!scanTimeout && ( | |
| <p className="animate-pulse text-center text-sm text-foreground-secondary"> | |
| {t("scan.scanningStatus")} | |
| </p> | |
| )} | |
| {scanTimeout && ( | |
| <div className="card border-warning-border bg-warning-bg text-center"> | |
| <p className="text-sm font-semibold text-warning-text"> | |
| {t("scan.timeoutTitle")} | |
| </p> | |
| <p className="mt-1 text-xs text-warning-text/80"> | |
| {t("scan.timeoutHint")} | |
| </p> | |
| </p> | |
| )} | |
| </div> |
| {/* Scanning status */} | ||
| {!scanTimeout && ( | ||
| <p className="animate-pulse text-center text-sm text-foreground-secondary"> | ||
| {t("scan.scanningStatus")} | ||
| </p> |
There was a problem hiding this comment.
PR description mentions a “pulsing green dot with status text”, but the implementation currently only renders pulsing text (no dot/visual indicator element). Either add the dot indicator (e.g. a small rounded element using success color) or adjust the PR description to match the implemented UI.
| vi.mock("@/lib/score-utils", () => ({ | ||
| toTryVitScore: (u: number) => 100 - u, | ||
| getScoreBand: (u: number) => { | ||
| if (u >= 1 && u <= 20) return { band: "green", label: "Excellent", color: "#22c55e", bgColor: "#dcfce7", textColor: "#166534" }; | ||
| if (u >= 21 && u <= 40) return { band: "yellow", label: "Good", color: "#eab308", bgColor: "#fef9c3", textColor: "#854d0e" }; | ||
| if (u >= 41 && u <= 60) return { band: "orange", label: "Moderate", color: "#f97316", bgColor: "#fff7ed", textColor: "#9a3412" }; | ||
| if (u >= 61 && u <= 80) return { band: "red", label: "Poor", color: "#ef4444", bgColor: "#fef2f2", textColor: "#991b1b" }; | ||
| if (u >= 81 && u <= 100) return { band: "darkred", label: "Bad", color: "#991b1b", bgColor: "#fef2f2", textColor: "#7f1d1d" }; | ||
| return null; | ||
| }, | ||
| })); |
There was a problem hiding this comment.
This @/lib/score-utils mock doesn’t match the real ScoreBand contract: in production bgColor/textColor are Tailwind class names, not hex colors. This can mask styling bugs (and will make tests misleading once the component is corrected to use classes). Prefer using the real module here, or mock it with the same shape/values as score-utils.ts returns.
| vi.mock("@/lib/score-utils", () => ({ | |
| toTryVitScore: (u: number) => 100 - u, | |
| getScoreBand: (u: number) => { | |
| if (u >= 1 && u <= 20) return { band: "green", label: "Excellent", color: "#22c55e", bgColor: "#dcfce7", textColor: "#166534" }; | |
| if (u >= 21 && u <= 40) return { band: "yellow", label: "Good", color: "#eab308", bgColor: "#fef9c3", textColor: "#854d0e" }; | |
| if (u >= 41 && u <= 60) return { band: "orange", label: "Moderate", color: "#f97316", bgColor: "#fff7ed", textColor: "#9a3412" }; | |
| if (u >= 61 && u <= 80) return { band: "red", label: "Poor", color: "#ef4444", bgColor: "#fef2f2", textColor: "#991b1b" }; | |
| if (u >= 81 && u <= 100) return { band: "darkred", label: "Bad", color: "#991b1b", bgColor: "#fef2f2", textColor: "#7f1d1d" }; | |
| return null; | |
| }, | |
| })); | |
| vi.mock("@/lib/score-utils", async () => { | |
| const actual = await vi.importActual<typeof import("@/lib/score-utils")>( | |
| "@/lib/score-utils", | |
| ); | |
| return { | |
| ...actual, | |
| }; | |
| }); |
| > | ||
| {foundProduct.nutri_score} | ||
| </span> | ||
| <span className="text-xs text-foreground-muted">Nutri-Score</span> | ||
| </div> |
There was a problem hiding this comment.
The Nutri-Score display here is hardcoded and reimplements the badge UI/semantics. For consistency and accessibility (aria-label/tooltip handling), prefer reusing the existing NutriScoreBadge component and localizing the label instead of rendering "Nutri-Score" directly.
Summary\n\nPremium barcode scanner UX enhancements for Issue #784:\n\n- Found preview overlay: When a product is scanned, show an inline preview (product name, brand, TryVit score band, Nutri-Score badge) with View Details / Scan Next actions instead of immediate navigation\n- Scan timeout hint: After 15 seconds of idle camera scanning, show a helpful banner suggesting manual entry or repositioning\n- Animated scan line: CSS-animated red laser line over the camera viewfinder for visual feedback\n- Scanning status indicator: Pulsing green dot with status text while camera is active\n- Paste barcode button: ClipboardPaste icon button in manual mode for quick EAN entry from clipboard\n\n## Changes\n\n- page.tsx: 11 incremental changes (imports, state, timeout effect, scan line, status indicator, timeout banner, found preview overlay, paste button)\n- en/pl/de.json: 7 new i18n keys each (scanningStatus, timeoutTitle, timeoutHint, productFound, viewDetails, scanNext, pasteBarcode)\n- page.test.tsx: 3 existing tests updated + 5 new tests (found preview flow, score band, nutri-score, paste button, timeout)\n\n## Testing\n\n- 46/46 scanner tests passing\n- TypeScript: 0 errors\n\nCloses #784