Skip to content

qr scanner for the login#84

Open
beny25585 wants to merge 8 commits into
mainfrom
feature/add-qr-login
Open

qr scanner for the login#84
beny25585 wants to merge 8 commits into
mainfrom
feature/add-qr-login

Conversation

@beny25585

@beny25585 beny25585 commented May 27, 2026

Copy link
Copy Markdown
Collaborator

Description

Please include a summary of the changes and the related issue.

Related Issue(s)

Fixes # (issue number)

Checklist:

  • I have performed a self-review of my own code
  • My changes generate no new warnings

Screenshots (if appropriate):

Summary by CodeRabbit

  • New Features
    • QR code–based patient login: generate a QR for quick login or scan one to proceed to the home screen.
    • Quick-login button now triggers the scanner and reports success or failure with user-facing feedback.
  • Style
    • Updated login and scanner layouts, including card, loading-overlay, close control and centered error messaging.

@beny25585 beny25585 requested a review from Tamir198 May 27, 2026 22:54
@coderabbitai

coderabbitai Bot commented May 27, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds a QR-based patient login: runtime dependency for camera scanning, a QRScanner React component and styles, in-memory token create/verify services and URL token extractor, NAV_ROUTES and QuickLogin button callback wiring, and LoginPatient changes to generate/verify tokens and conditionally render scanner or QR display.

Changes

QR Login Scanner Integration

Layer / File(s) Summary
html5-qrcode dependency, QRScanner, and styles
package.json, src/pages/login/QRLogin/QRScanner.tsx, src/pages/login/QRLogin/styles.ts
Adds html5-qrcode runtime dependency. New QRScanner initializes camera scanning on mount, shows a loading overlay until camera permission is granted, stops and calls onSuccess(decodedText) on first successful decode, and exposes a close button. Adds scanner, loading, and close-button styles.
Token generation, verification, and extraction
src/services/qrService.ts, src/utils/qrUtils.ts, src/pages/login/QRLogin/constants.ts
New in-memory qrService with createToken(userId) and verifyToken(token). Adds QR_TOKEN_PARAM = 'token' and extractQrToken(qrContent) to parse token query param from scanned URL strings.
Navigation routing and QuickLoginQRButton callback
src/components/NavBar/constants.ts, src/pages/login/LoginPatient/QuickLoginQRButton/QuickLoginQRButton.tsx
Adds NAV_ROUTES mapping derived from NAV_LINKS for typed route lookup. QuickLoginQRButton now accepts an onClick prop and forwards it to the underlying button.
LoginPatient QR flow and styles
src/pages/login/LoginPatient/LoginPatient.tsx, src/pages/login/LoginPatient/styles.ts
LoginPatient adds scannerOpen, qrToken, and scannerError state. Patient-number submit generates and stores a QR token. handleQrSuccess extracts token from scanned URL, verifies it, and navigates on success or sets an error and closes scanner on failure. Adds qrCardContainer and errorText styles.

Sequence Diagram(s)

sequenceDiagram
  participant User
  participant LoginPatient
  participant QRScanner
  participant qrService
  participant QRGeneration
  participant Router
  User->>LoginPatient: submit patient number
  activate LoginPatient
  LoginPatient->>qrService: createToken(serializedNumber)
  LoginPatient->>LoginPatient: set qrToken
  LoginPatient-->>User: render QRGeneration
  deactivate LoginPatient
  User->>LoginPatient: click Quick Login QR button
  activate LoginPatient
  LoginPatient->>QRScanner: open scanner
  deactivate LoginPatient
  User->>QRScanner: scan QR code
  activate QRScanner
  QRScanner->>LoginPatient: onSuccess(scannedUrl)
  deactivate QRScanner
  activate LoginPatient
  LoginPatient->>LoginPatient: extractQrToken(scannedUrl)
  LoginPatient->>qrService: verifyToken(token)
  qrService-->>LoginPatient: { userId }
  LoginPatient->>Router: navigate(NAV_ROUTES.home)
  deactivate LoginPatient
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Possibly related PRs

Suggested reviewers

  • Tamir198

Poem

🐰 I hopped to scan a tiny code,

A twirl, a beep along the road,
A token found, then doors unbarred,
I danced — the login not so hard. 🥕

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description is an unfilled template with no actual content provided by the author; all sections remain empty placeholders. Provide a meaningful summary of changes, identify related issue(s), confirm checklist items, and add screenshots if applicable.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'qr scanner for the login' clearly and concisely describes the main feature added—a QR scanner component integrated into the login flow.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feature/add-qr-login

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/pages/login/LoginPatient/LoginPatient.tsx (1)

30-57: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Complete the QR login flow and close scanner state.

The token is extracted but never consumed, and scanner state is never closed, so this path does not actually log in and can leave the scanner mounted indefinitely.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/pages/login/LoginPatient/LoginPatient.tsx` around lines 30 - 57, The QR
handler handleQrSuccess currently extracts a token but never uses it and never
closes the scanner; update handleQrSuccess (the callback passed to <QRScanner
onSuccess={handleQrSuccess}>) to call the existing login flow with the extracted
token (e.g., invoke the app's token-auth function or reuse the component's
submit/login routine) and then close the scanner by calling
setScannerOpen(false); also ensure errors are surfaced to the UI (not just
console.error) and guard against missing/invalid tokens before attempting login.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/pages/login/LoginPatient/LoginPatient.tsx`:
- Around line 32-40: Remove the raw QR payload logging to avoid leaking
credentials: delete the console.log('QR result:', result) and stop logging the
full error object that may contain the token; instead, in the QR scan handler
use only safe logs like a success/failed scan boolean or redacted messages, and
if you must log errors from the try/catch use err.message (or sanitize err)
rather than the full err which could include the token variable obtained from
result; reference the variables/functions in this block: result, token, and the
existing console.log/console.error calls to locate where to change.

In `@src/pages/login/QRLogin/QRScanner.tsx`:
- Around line 24-56: Html5Qrcode.getCameras branch leaves loading=true when no
devices or on error; update the flow in the Html5Qrcode.getCameras().then(...)
and .catch(...) handlers to call setLoading(false) whenever devices is empty or
camera discovery fails (unless already cancelled), and ensure
scannerReadyRef.current is set appropriately; specifically, in the devices
handler add a fallback path for (!devices?.length || cancelled) that sets
setLoading(false) (and optionally scannerReadyRef.current = false) before
returning, and in the .catch(err) handler call setLoading(false) (unless
cancelled) in addition to logging the error so the overlay can clear.

---

Outside diff comments:
In `@src/pages/login/LoginPatient/LoginPatient.tsx`:
- Around line 30-57: The QR handler handleQrSuccess currently extracts a token
but never uses it and never closes the scanner; update handleQrSuccess (the
callback passed to <QRScanner onSuccess={handleQrSuccess}>) to call the existing
login flow with the extracted token (e.g., invoke the app's token-auth function
or reuse the component's submit/login routine) and then close the scanner by
calling setScannerOpen(false); also ensure errors are surfaced to the UI (not
just console.error) and guard against missing/invalid tokens before attempting
login.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: bb7abc93-c877-4f74-b991-060c90d41d9c

📥 Commits

Reviewing files that changed from the base of the PR and between 19aaab1 and 4297481.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (4)
  • package.json
  • src/pages/login/LoginPatient/LoginPatient.tsx
  • src/pages/login/LoginPatient/QuickLoginQRButton/QuickLoginQRButton.tsx
  • src/pages/login/QRLogin/QRScanner.tsx

Comment thread src/pages/login/LoginPatient/LoginPatient.tsx Outdated
Comment thread src/pages/login/QRLogin/QRScanner.tsx Outdated

@Tamir198 Tamir198 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont see any link to the task in clickup, I remember that elad is working on the qr part of the front end.
also please fix those things before we continue:

  • no hardcoded strings, they should come from translation file
  • no inline styles
  • no hardcoded colors, bring everything from the theme object

@beny25585

beny25585 commented May 29, 2026

Copy link
Copy Markdown
Collaborator Author

@Tamir198

Copy link
Copy Markdown
Member

Sync with @Elad-Abutbul about what we have in terms of the the auth and qr, he did the initialed work

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/pages/login/LoginPatient/LoginPatient.tsx`:
- Around line 37-50: handleQrSuccess currently swallows errors and only logs
them, leaving the scanner running with no user feedback; update handleQrSuccess
to set a visible error state (e.g., setQrError / qrError) and stop/close the
scanner on failure so the user sees the problem and can retry or cancel.
Specifically, catch the error from the URL parsing/verifyToken call in
handleQrSuccess, call a state setter like setQrError with a user-friendly
message, and also invoke whatever scanner-control method exists in this
component (e.g., setIsScanning(false), closeScanner(), or navigate back) to stop
the scanner UI; ensure verifyToken errors are surfaced in the message and the UI
displays qrError so users can recover.
- Around line 69-70: The QR scanner UI currently opens via setScannerOpen(true)
(triggered by QuickLoginQRButton) but has no way to dismiss it; update the usage
to pass an onClose prop to QRScanner (e.g., <QRScanner
onSuccess={handleQrSuccess} onClose={() => setScannerOpen(false)} />) and
implement/ensure QRScanner accepts an onClose handler and calls it when the user
cancels/closes the camera (escape/backdrop/close button), so scannerOpen can be
set to false without a full reload.
- Around line 52-54: handleContinue currently calls navigate('/home')
unconditionally; change it to perform token/auth verification first (e.g., call
an existing verifyQRCode/verifyToken or authContext.verifyToken method, or
implement one) and only call navigate('/home') if verification succeeds,
otherwise show an error and do not navigate. Alternatively disable/remove the
"continue" button until a successful token verification completes. Update the
handleContinue function and the button click handler to await the verification
result and gate navigate('/home') on that result.
- Around line 29-35: The onSubmit handler currently logs the patient identifier
(data.serializedNumber) which exposes PII; remove the console.log and any other
direct logging of data.serializedNumber in the onSubmit function, keeping the
token creation call (createToken(data.serializedNumber)) and state update
(setQrToken(token)); if you need telemetry or audit, log a non-identifying
success message or a hashed/metadata-only value instead, and leave
methods.reset() as-is.

In `@src/pages/login/QRLogin/qrLoginService.ts`:
- Around line 14-16: The token lookup uses a falsy check that treats valid falsy
values as missing; change the validation around tokenStore.get(token) so it
checks explicitly for undefined (e.g. const userId = tokenStore.get(token); if
(userId === undefined) throw new Error('invalid token')) rather than using if
(!userId); update the code that references userId accordingly to preserve valid
empty-string or other falsy values.
- Around line 13-19: verifyToken currently returns a valid userId but leaves the
token in tokenStore allowing replay; after successfully retrieving userId you
must consume the token (e.g. call tokenStore.delete(token) or
tokenStore.consume(token) if available) before returning so the token cannot be
reused — keep the existing missing-token throw behavior, and if tokenStore
supports an atomic consume/claim method prefer that to avoid race conditions
(refer to verifyToken, tokenStore.get and use tokenStore.delete or
tokenStore.consume).
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 559f0693-e3a6-4582-a5a4-91c631ac9b1a

📥 Commits

Reviewing files that changed from the base of the PR and between 4297481 and 9f528f3.

📒 Files selected for processing (4)
  • src/pages/login/LoginPatient/LoginPatient.tsx
  • src/pages/login/LoginPatient/styles.ts
  • src/pages/login/QRLogin/QRScanner.tsx
  • src/pages/login/QRLogin/qrLoginService.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/pages/login/QRLogin/QRScanner.tsx

Comment thread src/pages/login/LoginPatient/LoginPatient.tsx
Comment thread src/pages/login/LoginPatient/LoginPatient.tsx Outdated
Comment thread src/pages/login/LoginPatient/LoginPatient.tsx Outdated
Comment thread src/pages/login/QRLogin/qrLoginService.ts Outdated
Comment thread src/pages/login/QRLogin/qrLoginService.ts
@beny25585 beny25585 requested a review from Tamir198 May 29, 2026 21:26

@Tamir198 Tamir198 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a video of your changes working locally? I want to make sure that the problem is with my side.

Also please check your files again, you have a lot of logic inside your components, best practice is to extract them into data hook

Comment thread src/pages/login/LoginPatient/LoginPatient.tsx Outdated
Comment thread src/pages/login/LoginPatient/LoginPatient.tsx Outdated
Comment thread src/pages/login/LoginPatient/LoginPatient.tsx Outdated
Comment thread src/pages/login/LoginPatient/LoginPatient.tsx Outdated
Comment thread src/pages/login/QRLogin/qrLoginService.ts Outdated
Comment thread src/pages/login/LoginPatient/LoginPatient.tsx Outdated
Comment thread src/pages/login/QRLogin/qrLoginService.ts Outdated
@beny25585 beny25585 requested a review from Tamir198 May 31, 2026 07:30
@beny25585

Copy link
Copy Markdown
Collaborator Author
2026-05-31.101900.mp4

add you a video that show its work

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
src/services/qrService.ts (1)

1-7: ⚖️ Poor tradeoff

In-memory token store does not persist across restarts or scale horizontally.

tokenStore is a module-scoped Map, so all tokens are lost on server/app restart and aren't shared between multiple server instances. This is acceptable for local development, but ensure persistent storage (Redis, DB) is planned before production.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/services/qrService.ts` around lines 1 - 7, The tokenStore currently
defined as a module-scoped Map in src/services/qrService.ts (tokenStore and
createToken) is ephemeral and will not survive process restarts or scale across
instances; replace it with a persistent/shared backing (e.g., Redis or your DB)
or make the store pluggable via dependency injection. Change the implementation
of createToken (and any other token accessors) to use the chosen persistent
client API (SET/GET or equivalent) instead of tokenStore.set/tokenStore.get, add
appropriate TTL handling if needed, and ensure the Redis/DB client is
initialized and injected into the module (or exported service) so tokens persist
across restarts and are available to multiple server instances.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/pages/login/LoginPatient/LoginPatient.tsx`:
- Around line 40-54: In handleQrSuccess remove the leftover debug
console.log(NAV_ROUTES.home) call so the function only extracts the token via
extractQrToken(result), verifies it with verifyToken(token), navigates to
NAV_ROUTES.home on success, and handles errors with setScannerError and
setScannerOpen; just delete the console.log line referencing NAV_ROUTES.home to
clean up debug output.

In `@src/services/qrService.ts`:
- Around line 9-15: verifyToken currently leaves tokens in tokenStore allowing
replay; update verifyToken to consume the token when it's validated by
retrieving the value with tokenStore.get(token) and then removing it (e.g.,
tokenStore.delete(token)) before resolving; if removal requires a different API
use the store's remove method but ensure the token is deleted atomically after
successful lookup so you return Promise.reject(new Error('invalid token')) when
absent and Promise.resolve({ userId }) only after the token has been removed.

---

Nitpick comments:
In `@src/services/qrService.ts`:
- Around line 1-7: The tokenStore currently defined as a module-scoped Map in
src/services/qrService.ts (tokenStore and createToken) is ephemeral and will not
survive process restarts or scale across instances; replace it with a
persistent/shared backing (e.g., Redis or your DB) or make the store pluggable
via dependency injection. Change the implementation of createToken (and any
other token accessors) to use the chosen persistent client API (SET/GET or
equivalent) instead of tokenStore.set/tokenStore.get, add appropriate TTL
handling if needed, and ensure the Redis/DB client is initialized and injected
into the module (or exported service) so tokens persist across restarts and are
available to multiple server instances.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 04e3da58-5d13-4d3b-919d-e31759d02083

📥 Commits

Reviewing files that changed from the base of the PR and between 9f528f3 and 5bedf71.

📒 Files selected for processing (7)
  • src/components/NavBar/constants.ts
  • src/pages/login/LoginPatient/LoginPatient.tsx
  • src/pages/login/LoginPatient/styles.ts
  • src/pages/login/QRLogin/QRScanner.tsx
  • src/pages/login/QRLogin/constants.ts
  • src/services/qrService.ts
  • src/utils/qrUtils.ts
✅ Files skipped from review due to trivial changes (2)
  • src/pages/login/QRLogin/constants.ts
  • src/utils/qrUtils.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/pages/login/LoginPatient/styles.ts
  • src/pages/login/QRLogin/QRScanner.tsx

Comment thread src/pages/login/LoginPatient/LoginPatient.tsx Outdated
Comment thread src/services/qrService.ts Outdated

@GilHeller GilHeller left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address both @Tamir198's comments and mine. While reviewing them, feel free to reply if you disagree with a suggestion or need additional clarification. Once a comment has been addressed, please resolve it.

Also, make sure the code passes the Prettier check before marking the review as complete.

Comment thread src/pages/login/LoginPatient/LoginPatient.tsx Outdated
Comment thread src/pages/login/QRLogin/QRScanner.tsx Outdated
Comment thread src/pages/login/QRLogin/QRScanner.tsx Outdated
Comment thread src/pages/login/QRLogin/QRScanner.tsx Outdated
Comment thread src/services/qrService.ts
return Promise.reject(new Error('invalid token'))
}
return Promise.resolve({ userId })
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider making this function async instead of manually wrapping values with Promise.resolve() and Promise.reject(). This simplifies the implementation and improves readability:

export const verifyToken = async (token: string): Promise<{ userId: string }> => {
  const userId = tokenStore.get(token)
  if (!userId) {
    throw new Error('invalid token')
  }
  return { userId }
}

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/pages/login/QRLogin/QRScanner.tsx (1)

16-64: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Add an effect cleanup that actually cancels scanner startup.

cancelled is a const, so the guards on Lines 37, 41, and 50 never flip. If this component unmounts while getCameras()/start() is in flight, it can still call setLoading(false), and an already-started Html5Qrcode instance is never stopped. That can leave the camera active and make the next scanner mount flaky.

Suggested fix
   useEffect(() => {
     if (startedRef.current) return
     startedRef.current = true

     const qr = new Html5Qrcode('reader')
     qrRef.current = qr

-    const cancelled = false
+    let cancelled = false
     let stopped = false
@@
-    startScanner()
-  })
+    void startScanner()
+
+    return () => {
+      cancelled = true
+      if (scannerReadyRef.current && !stoppedRef.current) {
+        void qr.stop().catch(() => {})
+      }
+    }
+  }, [])
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/pages/login/QRLogin/QRScanner.tsx` around lines 16 - 64, The effect never
cancels async startup because `cancelled` is a const and never flipped, so add a
cancellable flag (use a let or better a ref like `cancelledRef`) and return a
cleanup from the useEffect that sets that flag and stops the scanner: in the
cleanup set `cancelledRef.current = true`, if `qrRef.current` exists and
`stoppedRef.current` is false call `await qrRef.current.stop()` (handle errors)
and reset any relevant refs (e.g. `startedRef.current = false`,
`scannerReadyRef.current = false`) so in-flight
`Html5Qrcode.getCameras()`/`qr.start()` handlers (in startScanner,
handleCameraError, handleDecode) check the cancellable flag/ref before calling
setLoading or onSuccess and avoid leaving the camera running.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@src/pages/login/QRLogin/QRScanner.tsx`:
- Around line 16-64: The effect never cancels async startup because `cancelled`
is a const and never flipped, so add a cancellable flag (use a let or better a
ref like `cancelledRef`) and return a cleanup from the useEffect that sets that
flag and stops the scanner: in the cleanup set `cancelledRef.current = true`, if
`qrRef.current` exists and `stoppedRef.current` is false call `await
qrRef.current.stop()` (handle errors) and reset any relevant refs (e.g.
`startedRef.current = false`, `scannerReadyRef.current = false`) so in-flight
`Html5Qrcode.getCameras()`/`qr.start()` handlers (in startScanner,
handleCameraError, handleDecode) check the cancellable flag/ref before calling
setLoading or onSuccess and avoid leaving the camera running.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 398b91ac-bbbc-4199-aa25-2328c0d9bf75

📥 Commits

Reviewing files that changed from the base of the PR and between 5bedf71 and 44f1e4c.

📒 Files selected for processing (2)
  • src/pages/login/QRLogin/QRScanner.tsx
  • src/pages/login/QRLogin/styles.ts

@beny25585 beny25585 requested a review from GilHeller June 7, 2026 13:40

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
src/pages/login/LoginPatient/LoginPatient.tsx (1)

87-87: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Unreachable dead code — remove duplicate QRScanner render.

This line is never reached: when scannerOpen is true, the ternary at line 62 renders <QRScanner> and skips the entire !qrToken branch containing this code. This appears to be leftover from a previous iteration.

🧹 Proposed fix
           {scannerError && (
             <SGLTypography styles={styles.errorText} variant="smallText">
               {scannerError}
             </SGLTypography>
           )}
-          {scannerOpen && <QRScanner onSuccess={handleQrSuccess} />}
           <LoginSupportInfo text={t('login.supportInfo')} />
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/pages/login/LoginPatient/LoginPatient.tsx` at line 87, There’s a
duplicate, unreachable QRScanner render: remove the redundant JSX "{scannerOpen
&& <QRScanner onSuccess={handleQrSuccess} />}" because QRScanner is already
rendered by the earlier ternary that checks scannerOpen inside the !qrToken
branch; keep the single QRScanner instance (the one using handleQrSuccess) and,
if any props or state usage from the removed occurrence are unique, migrate them
into the existing QRScanner render (refer to scannerOpen, QRScanner, and
handleQrSuccess).
src/pages/login/QRLogin/QRScanner.tsx (1)

17-62: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Missing cleanup function leaks camera resource on unmount.

If the component unmounts while the scanner is active (e.g., user navigates away, parent conditionally removes it), qr.stop() is never called and the camera remains open. Add a cleanup function to stop the scanner on unmount.

Additionally, the cancelled variable (line 24) is const false and never reassigned, making the if (!cancelled) checks dead code.

🛠️ Proposed fix with cleanup
   useEffect(() => {
     if (startedRef.current) return
     startedRef.current = true

     const qr = new Html5Qrcode('reader')
     qrRef.current = qr

-    const cancelled = false
+    let cancelled = false
     let stopped = false

     const handleDecode = async (decodedText: string) => {
       if (stopped) return
       stopped = true
       stoppedRef.current = true
       await qr.stop()
       onSuccess(decodedText)
     }
     const handleDecodeError = () => {}

     const handleScannerStarted = () => {
       scannerReadyRef.current = true
       if (!cancelled) setLoading(false)
     }

     const handleCameraError = (err: unknown) => {
       if (!cancelled) {
         setLoading(false)
         console.log((err as Error)?.message || 'Camera access denied or unavailable')
       }
     }

     const startScanner = async () => {
       try {
         await qr.start(
           { facingMode: 'environment' },
           { fps: 20, qrbox: 250 },
           handleDecode,
           handleDecodeError,
         )
         handleScannerStarted()
       } catch (err) {
         handleCameraError(err)
       }
     }
     startScanner()
+
+    return () => {
+      cancelled = true
+      if (scannerReadyRef.current && !stoppedRef.current) {
+        qr.stop().catch(() => {})
+      }
+    }
-  })
+  }, [onSuccess])
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/pages/login/QRLogin/QRScanner.tsx` around lines 17 - 62, The useEffect
that initializes Html5Qrcode leaks the camera because it has no cleanup and uses
a never-changed const cancelled; change the effect to return a cleanup function
that stops the scanner and tears down refs: make cancelled a mutable (let or a
ref like cancelledRef) and set it true in cleanup, ensure you await or call
qrRef.current.stop() inside the cleanup and guard with startedRef/current and
stoppedRef/current, and also set scannerReadyRef.current and loading state
appropriately; update handleDecode to use stoppedRef and qrRef.current
consistently and catch errors from qr.stop() so the camera is always released on
unmount or when stopping the scanner.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@src/pages/login/LoginPatient/LoginPatient.tsx`:
- Line 87: There’s a duplicate, unreachable QRScanner render: remove the
redundant JSX "{scannerOpen && <QRScanner onSuccess={handleQrSuccess} />}"
because QRScanner is already rendered by the earlier ternary that checks
scannerOpen inside the !qrToken branch; keep the single QRScanner instance (the
one using handleQrSuccess) and, if any props or state usage from the removed
occurrence are unique, migrate them into the existing QRScanner render (refer to
scannerOpen, QRScanner, and handleQrSuccess).

In `@src/pages/login/QRLogin/QRScanner.tsx`:
- Around line 17-62: The useEffect that initializes Html5Qrcode leaks the camera
because it has no cleanup and uses a never-changed const cancelled; change the
effect to return a cleanup function that stops the scanner and tears down refs:
make cancelled a mutable (let or a ref like cancelledRef) and set it true in
cleanup, ensure you await or call qrRef.current.stop() inside the cleanup and
guard with startedRef/current and stoppedRef/current, and also set
scannerReadyRef.current and loading state appropriately; update handleDecode to
use stoppedRef and qrRef.current consistently and catch errors from qr.stop() so
the camera is always released on unmount or when stopping the scanner.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 98f5611c-d4ec-4c20-a6cd-924fe7f52514

📥 Commits

Reviewing files that changed from the base of the PR and between 44f1e4c and dc7e432.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (3)
  • src/pages/login/LoginPatient/LoginPatient.tsx
  • src/pages/login/QRLogin/QRScanner.tsx
  • src/pages/login/QRLogin/styles.ts
✅ Files skipped from review due to trivial changes (1)
  • src/pages/login/QRLogin/styles.ts

Comment thread src/components/NavBar/constants.ts Outdated
Comment thread src/pages/login/LoginPatient/LoginPatient.tsx Outdated
const navigate = useNavigate()
const [scannerOpen, setScannerOpen] = useState(false)
const [scannerError, setScannerError] = useState<string | null>(null)
const [qrToken, setQrToken] = useState<string>()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you really need the generic in here?

console.log(NAV_ROUTES.home)

if (!token) {
throw new Error('cant get token')

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should come from constants files

{scannerOpen ? (
<QRScanner onSuccess={handleQrSuccess} onClose={() => setScannerOpen(false)} />
) : !qrToken ? (
<>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need this fragment?

Comment thread src/pages/login/QRLogin/styles.ts Outdated
Comment thread src/pages/login/QRLogin/styles.ts Outdated
Comment thread src/pages/login/QRLogin/styles.ts Outdated
Comment thread src/pages/login/QRLogin/styles.ts Outdated
Comment thread src/services/qrService.ts Outdated
return token
}

export const verifyToken = (token: string): Promise<{ userId: string }> => {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dont we have a type for { userId: string } ?

@beny25585 beny25585 requested a review from Tamir198 June 15, 2026 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants