fix(frontend): scanner — add camera permission recovery and scan feedback (#702)#745
fix(frontend): scanner — add camera permission recovery and scan feedback (#702)#745ericsocrat merged 5 commits intomainfrom
Conversation
…back (#702) - Add typed CameraErrorKind (permission-denied | no-camera | generic) - Show differentiated error cards with icons, hints, and Retry button - Fix bug: remove setMode('manual') so error cards stay visible - Add haptic feedback (navigator.vibrate) on successful scan - Replace hardcoded English with i18n keys (scannedCount, cameraBlocked, etc.) - Add 4 new tests: no-camera card, permission-denied card, retry button, haptic - Fix ZXing mock: use class constructor for dynamic import compatibility
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Pull request overview
This PR improves the frontend barcode scanner experience by adding typed camera error recovery states (with differentiated UI), haptic feedback on scan success, and i18n for batch scan count and camera recovery messaging, along with updated test mocks to support ZXing dynamic imports.
Changes:
- Introduces
CameraErrorKindand a recovery UI with distinct cards for no-camera vs permission-denied vs generic camera errors. - Adds
navigator.vibrate(100)on successful scan and replaces hardcoded batch text witht("scan.scannedCount", { count }). - Fixes ZXing test mocking by replacing the arrow-function “constructor” with a real class and adds new recovery/feedback tests.
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 | Typed camera error states, recovery card UI, retry behavior, haptic feedback, and i18n batch count usage. |
| frontend/src/app/app/scan/page.test.tsx | ZXing mock rewritten as a constructible class; adds tests for camera recovery and vibration feedback. |
| frontend/messages/en.json | Adds new scan-related i18n strings for recovery messaging. |
| frontend/messages/pl.json | Adds new scan-related i18n strings for recovery messaging. |
| frontend/messages/de.json | Adds new scan-related i18n strings for recovery messaging. |
| } catch (err: unknown) { | ||
| let errName = ""; | ||
| if (err instanceof Error) { | ||
| errName = err.name; | ||
| } else if (err && typeof err === "object" && "name" in err) { | ||
| errName = String(err.name); | ||
| } | ||
| if ( | ||
| errName === "NotAllowedError" || | ||
| errName === "PermissionDeniedError" | ||
| ) { | ||
| setCameraError(t("scan.permissionDenied")); | ||
| setCameraError("permission-denied"); | ||
| showToast({ type: "error", messageKey: "scan.permissionDenied" }); | ||
| } else { | ||
| setCameraError(t("scan.cameraError")); | ||
| setCameraError("generic"); | ||
| } | ||
| setMode("manual"); | ||
| } |
There was a problem hiding this comment.
startScanner doesn’t clean up if an exception is thrown after creating the ZXing reader (e.g., if decodeFromVideoDevice throws synchronously). That can leave readerRef (and potentially an opened stream) in an inconsistent state. Suggest calling stopScanner() inside the catch (or using try { ... } catch { ... } finally { ... } to reset refs / stop tracks on failure) before setting cameraError.
| <p className="mt-1 text-xs text-amber-700"> | ||
| {cameraError === "no-camera" | ||
| ? t("scan.noCameraHint") | ||
| : cameraError === "permission-denied" | ||
| ? t("scan.cameraBlockedHint") | ||
| : t("scan.cameraError")} | ||
| </p> |
There was a problem hiding this comment.
For the generic camera error UI, both the title and hint fall back to t("scan.cameraError"). Since scan.cameraError is a full sentence (e.g. "Could not start camera. Try manual entry instead."), this currently renders the same sentence twice. Consider adding distinct i18n keys for a short title vs. the longer hint (or reuse an existing title-style key) so the card isn’t duplicated.
| <p className="mt-1 text-xs text-amber-700"> | |
| {cameraError === "no-camera" | |
| ? t("scan.noCameraHint") | |
| : cameraError === "permission-denied" | |
| ? t("scan.cameraBlockedHint") | |
| : t("scan.cameraError")} | |
| </p> | |
| {cameraError !== "generic" && ( | |
| <p className="mt-1 text-xs text-amber-700"> | |
| {cameraError === "no-camera" | |
| ? t("scan.noCameraHint") | |
| : t("scan.cameraBlockedHint")} | |
| </p> | |
| )} |
| const vibrateSpy = vi.fn(); | ||
| Object.defineProperty(navigator, "vibrate", { | ||
| value: vibrateSpy, | ||
| writable: true, | ||
| configurable: true, | ||
| }); | ||
|
|
||
| mockRecordScan.mockResolvedValue({ | ||
| ok: true, | ||
| data: { | ||
| found: true, | ||
| product_id: 42, | ||
| product_name: "Vibrate Test", | ||
| brand: "TestBrand", | ||
| nutri_score: "B", | ||
| }, | ||
| }); | ||
| const user = userEvent.setup(); | ||
|
|
||
| render(<ScanPage />, { wrapper: createWrapper() }); | ||
| await user.click(screen.getByText("Manual")); | ||
| await user.type( | ||
| screen.getByPlaceholderText("Enter EAN barcode (8 or 13 digits)"), | ||
| "5901234123457", | ||
| ); | ||
| await user.click(screen.getByText("Look up")); | ||
|
|
||
| await waitFor(() => { | ||
| expect(vibrateSpy).toHaveBeenCalledWith(100); | ||
| }); |
There was a problem hiding this comment.
This test defines navigator.vibrate via Object.defineProperty but never restores the original value. Since Vitest may reuse the same jsdom environment across test files, this can leak global state into other tests. Store the previous navigator.vibrate and restore it in a finally/afterEach, or use vi.stubGlobal('navigator', ...) + vi.unstubAllGlobals() cleanup.
| const vibrateSpy = vi.fn(); | |
| Object.defineProperty(navigator, "vibrate", { | |
| value: vibrateSpy, | |
| writable: true, | |
| configurable: true, | |
| }); | |
| mockRecordScan.mockResolvedValue({ | |
| ok: true, | |
| data: { | |
| found: true, | |
| product_id: 42, | |
| product_name: "Vibrate Test", | |
| brand: "TestBrand", | |
| nutri_score: "B", | |
| }, | |
| }); | |
| const user = userEvent.setup(); | |
| render(<ScanPage />, { wrapper: createWrapper() }); | |
| await user.click(screen.getByText("Manual")); | |
| await user.type( | |
| screen.getByPlaceholderText("Enter EAN barcode (8 or 13 digits)"), | |
| "5901234123457", | |
| ); | |
| await user.click(screen.getByText("Look up")); | |
| await waitFor(() => { | |
| expect(vibrateSpy).toHaveBeenCalledWith(100); | |
| }); | |
| const originalVibrateDescriptor = Object.getOwnPropertyDescriptor( | |
| navigator, | |
| "vibrate", | |
| ); | |
| const vibrateSpy = vi.fn(); | |
| Object.defineProperty(navigator, "vibrate", { | |
| value: vibrateSpy, | |
| writable: true, | |
| configurable: true, | |
| }); | |
| try { | |
| mockRecordScan.mockResolvedValue({ | |
| ok: true, | |
| data: { | |
| found: true, | |
| product_id: 42, | |
| product_name: "Vibrate Test", | |
| brand: "TestBrand", | |
| nutri_score: "B", | |
| }, | |
| }); | |
| const user = userEvent.setup(); | |
| render(<ScanPage />, { wrapper: createWrapper() }); | |
| await user.click(screen.getByText("Manual")); | |
| await user.type( | |
| screen.getByPlaceholderText("Enter EAN barcode (8 or 13 digits)"), | |
| "5901234123457", | |
| ); | |
| await user.click(screen.getByText("Look up")); | |
| await waitFor(() => { | |
| expect(vibrateSpy).toHaveBeenCalledWith(100); | |
| }); | |
| } finally { | |
| if (originalVibrateDescriptor) { | |
| Object.defineProperty(navigator, "vibrate", originalVibrateDescriptor); | |
| } else { | |
| Reflect.deleteProperty(navigator, "vibrate"); | |
| } | |
| } |
| <button | ||
| onClick={() => { | ||
| setCameraError(null); | ||
| setMode("camera"); |
There was a problem hiding this comment.
The Retry Camera handler calls startScanner() without stopping any existing reader/stream first. If the previous attempt partially initialized (or readerRef.current is still set from an earlier run), this can result in multiple readers/streams or leaked resources. Consider calling stopScanner() before startScanner() here, or making startScanner() itself always stop/reset any existing scanner before creating a new reader.
| setMode("camera"); | |
| setMode("camera"); | |
| stopScanner(); |
| @@ -170,8 +177,7 @@ | |||
|
|
|||
| const devices = await reader.listVideoInputDevices(); | |||
| if (devices.length === 0) { | |||
There was a problem hiding this comment.
In startScanner, if listVideoInputDevices() returns an empty list, the function returns after setting readerRef.current = reader. That leaves the reader instance hanging around (and makes future restarts harder to reason about). Consider calling stopScanner() (or at least reader.reset() + clearing readerRef.current) before returning in the devices.length === 0 branch.
| if (devices.length === 0) { | |
| if (devices.length === 0) { | |
| reader.reset(); | |
| readerRef.current = null; |
Bundle Size Report
✅ Bundle size is within acceptable limits. |
…very tests with main mock pattern
…rat/tryvit into fix/702-scanner-recovery
| @@ -170,8 +177,7 @@ export default function ScanPage() { | |||
|
|
|||
| const devices = await reader.listVideoInputDevices(); | |||
| if (devices.length === 0) { | |||
There was a problem hiding this comment.
When devices.length === 0, readerRef.current has already been set to a BrowserMultiFormatReader, but the function returns without resetting/cleaning it up. Consider calling stopScanner() before setting the error and returning so the reader state is cleared consistently.
| if (devices.length === 0) { | |
| if (devices.length === 0) { | |
| stopScanner(); |
| <button | ||
| onClick={() => { | ||
| setCameraError(null); | ||
| setMode("camera"); | ||
| startScanner(); | ||
| }} | ||
| className="btn-secondary mt-3" | ||
| > |
There was a problem hiding this comment.
Retry starts a new scanner instance without explicitly stopping any existing reader/stream first. If a previous attempt left a stream active (or readerRef.current set), this can lead to multiple active instances or 'camera already in use' failures. Consider calling stopScanner() before startScanner() here.
| const vibrateSpy = vi.fn(); | ||
| Object.defineProperty(navigator, "vibrate", { | ||
| value: vibrateSpy, | ||
| writable: true, | ||
| configurable: true, | ||
| }); | ||
|
|
||
| mockRecordScan.mockResolvedValue({ | ||
| ok: true, | ||
| data: { | ||
| found: true, | ||
| product_id: 42, | ||
| product_name: "Vibrate Test", | ||
| brand: "TestBrand", | ||
| nutri_score: "B", | ||
| }, | ||
| }); | ||
| const user = userEvent.setup(); | ||
|
|
||
| render(<ScanPage />, { wrapper: createWrapper() }); | ||
| await user.click(screen.getByText("Manual")); | ||
| await user.type( | ||
| screen.getByPlaceholderText("Enter EAN barcode (8 or 13 digits)"), | ||
| "5901234123457", | ||
| ); | ||
| await user.click(screen.getByText("Look up")); | ||
|
|
||
| await waitFor(() => { | ||
| expect(vibrateSpy).toHaveBeenCalledWith(100); | ||
| }); |
There was a problem hiding this comment.
This test mutates the global navigator.vibrate and doesn't restore it afterwards, which can leak state into subsequent tests. Consider saving the original descriptor/value and restoring (or deleting) it in a cleanup step (e.g., finally/afterEach) to keep tests isolated.
| const vibrateSpy = vi.fn(); | |
| Object.defineProperty(navigator, "vibrate", { | |
| value: vibrateSpy, | |
| writable: true, | |
| configurable: true, | |
| }); | |
| mockRecordScan.mockResolvedValue({ | |
| ok: true, | |
| data: { | |
| found: true, | |
| product_id: 42, | |
| product_name: "Vibrate Test", | |
| brand: "TestBrand", | |
| nutri_score: "B", | |
| }, | |
| }); | |
| const user = userEvent.setup(); | |
| render(<ScanPage />, { wrapper: createWrapper() }); | |
| await user.click(screen.getByText("Manual")); | |
| await user.type( | |
| screen.getByPlaceholderText("Enter EAN barcode (8 or 13 digits)"), | |
| "5901234123457", | |
| ); | |
| await user.click(screen.getByText("Look up")); | |
| await waitFor(() => { | |
| expect(vibrateSpy).toHaveBeenCalledWith(100); | |
| }); | |
| const originalVibrateDescriptor = Object.getOwnPropertyDescriptor( | |
| navigator, | |
| "vibrate", | |
| ); | |
| const vibrateSpy = vi.fn(); | |
| try { | |
| Object.defineProperty(navigator, "vibrate", { | |
| value: vibrateSpy, | |
| writable: true, | |
| configurable: true, | |
| }); | |
| mockRecordScan.mockResolvedValue({ | |
| ok: true, | |
| data: { | |
| found: true, | |
| product_id: 42, | |
| product_name: "Vibrate Test", | |
| brand: "TestBrand", | |
| nutri_score: "B", | |
| }, | |
| }); | |
| const user = userEvent.setup(); | |
| render(<ScanPage />, { wrapper: createWrapper() }); | |
| await user.click(screen.getByText("Manual")); | |
| await user.type( | |
| screen.getByPlaceholderText("Enter EAN barcode (8 or 13 digits)"), | |
| "5901234123457", | |
| ); | |
| await user.click(screen.getByText("Look up")); | |
| await waitFor(() => { | |
| expect(vibrateSpy).toHaveBeenCalledWith(100); | |
| }); | |
| } finally { | |
| if (originalVibrateDescriptor) { | |
| Object.defineProperty(navigator, "vibrate", originalVibrateDescriptor); | |
| } else { | |
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |
| delete (navigator as any).vibrate; | |
| } | |
| } |
| } | ||
| setMode("manual"); | ||
| } | ||
| }, [stopScanner, t]); |
There was a problem hiding this comment.
t is included in the startScanner dependency list, but within the shown code the callback no longer uses t. Removing unused dependencies keeps the callback stable and can avoid unnecessary re-creations (and potential effect re-runs) as translations change.
The component no longer auto-switches to manual mode when no camera is found — it stays in camera mode and shows an error card. The test must explicitly switch to manual first, then back to camera.
Summary
Closes #702 — Adds camera permission recovery UI, differentiated error cards, haptic feedback, and i18n fixes to the barcode scanner page.
Changes
Camera Error Recovery (typed error states)
CameraErrorKind = "permission-denied" | "no-camera" | "generic"replaces the untypedstring | nullerror stateBug Fix — Error card was invisible
setMode("manual")was called immediately aftersetCameraError(...), so mode changed away from"camera"and the error card (rendered only in camera mode) was never visiblesetMode("manual")from both the no-camera branch and the catch block, keeping the user in camera mode so the recovery UI is shownHaptic Feedback
navigator.vibrate(100)on successful barcode scan (inscanMutation.onSuccess)i18n
"Batch: {N} scanned"witht("scan.scannedCount", { count })noCameraTitle,noCameraHint,cameraBlocked,cameraBlockedHint,retryCamera,scannedCountTest Infrastructure Fix
vi.fn().mockImplementation(() => ({...})). Arrow functions cannot be used as constructors. When the component loads ZXing via dynamic import (await import("@zxing/library")) and callsnew BrowserMultiFormatReader(), the arrow function threwTypeError: ... is not a constructorMockBrowserMultiFormatReaderclass using hoisted mock variables (mockListDevices,mockDecode,mockReaderReset)Tests
4 new tests (34 total, all passing):
listVideoInputDevicesreturns[]decodeFromVideoDevicethrowsNotAllowedErrorlistVideoInputDevicesis called againnavigator.vibrate(100)called on successful scanVerification
Files Changed
frontend/src/app/app/scan/page.tsxfrontend/src/app/app/scan/page.test.tsxfrontend/messages/en.jsonfrontend/messages/pl.jsonfrontend/messages/de.json