feat: add QR verification route and print QR translations - #94
Conversation
📝 WalkthroughWalkthroughAdds QR creation and verification to the login flow with new API types and endpoints, a QR service wrapper, updated print and verify pages, router wiring, environment configuration, and localized QR/login strings. ChangesQR Login Create & Verify Flow
Sequence Diagram(s)sequenceDiagram
participant User
participant PrintQR
participant QRService
participant QRVerify
participant Router
User->>PrintQR: Open print QR page
PrintQR->>QRService: createQR({ userId, password })
QRService-->>PrintQR: { token }
PrintQR-->>User: Render QRGeneration and print action
User->>Router: Open /QR/verify?token=...
Router->>QRVerify: Render QRVerify
QRVerify->>QRService: verifyQR({ token })
QRService-->>QRVerify: success
QRVerify-->>User: Navigate /home
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
src/types/qr.types.ts (1)
14-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDefine a stricter type for QRVerifyResponse.
Record<string, unknown>provides no type safety and doesn't document what the backend returns after verification. Consider defining the actual response shape (e.g., user session data, authentication tokens, user info, or success status).💡 Example stricter type definition
-export type QRVerifyResponse = Record<string, unknown> +export interface QRVerifyResponse { + success: boolean + userId?: string + // Add other fields returned by the backend +}🤖 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/types/qr.types.ts` at line 14, The QRVerifyResponse type is currently defined as Record<string, unknown>, which lacks type safety and doesn't document the actual response structure from the backend. Replace this generic definition with a proper interface or type that specifies the actual properties returned after QR code verification, such as user session data, authentication tokens, user information, or success status flags. This will provide better type checking and self-document what the verification endpoint returns.src/pages/login/QRLogin/QRVerify.tsx (3)
15-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFallback to empty string may send invalid data to the API.
The query function uses
token ?? ''as a fallback. While theenabled: Boolean(token)guard should prevent execution when token is null, this creates a fragile dependency where the fallback could send an empty string if the enabled logic changes.Consider making the dependency explicit:
♻️ Remove the fallback for clarity
const { isError, isPending, isSuccess } = useQuery({ queryKey: ['verifyQR', token], - queryFn: () => verifyQR({ token: token ?? '' }), - enabled: Boolean(token), + queryFn: () => verifyQR({ token: token! }), + enabled: Boolean(token?.trim()), refetchOnWindowFocus: false, retry: false, })The non-null assertion (
token!) is safe here becauseenabledguarantees token exists when the function runs.🤖 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/QRVerify.tsx` at line 15, In the queryFn callback of the useQuery hook, replace the fallback pattern `token ?? ''` with the non-null assertion `token!` instead. This makes the dependency on the enabled guard explicit and removes the fragile empty string fallback, since the enabled condition (Boolean(token)) guarantees that token is not null or undefined when the query function executes.
13-19: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider invalidating the token query after successful verification.
After successful verification, the token remains in the React Query cache. While the user navigates away, the cached token could be reused if they return to this route, potentially allowing replay if the backend doesn't invalidate tokens properly.
Consider invalidating the query on success:
const queryClient = useQueryClient() const { isError, isPending, isSuccess } = useQuery({ queryKey: ['verifyQR', token], queryFn: () => verifyQR({ token: token ?? '' }), enabled: Boolean(token), refetchOnWindowFocus: false, retry: false, onSuccess: () => { queryClient.invalidateQueries({ queryKey: ['verifyQR'] }) } })Note: Primary token validation should be server-side (expiration, one-time use, etc.).
🤖 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/QRVerify.tsx` around lines 13 - 19, The useQuery hook in QRVerify.tsx is missing cache invalidation after successful token verification, which could allow the token to be reused if the user returns to the route. Import useQueryClient from React Query at the top of the file, then call useQueryClient to get the queryClient instance. Add an onSuccess callback to the useQuery configuration that calls queryClient.invalidateQueries with the queryKey ['verifyQR'] to clear the cached token data after successful verification.
25-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueError state provides no diagnostic information.
When verification fails, users only see a generic "Unable to verify QR" message. This makes it difficult to diagnose issues (expired token, network error, invalid token, etc.).
Consider logging error details for debugging:
const { isError, isPending, isSuccess, error } = useQuery({ // ... existing config }) if (isError) { console.error('QR verification failed:', error) return <SGLTypography>{t('qrVerify.verifyError')}</SGLTypography> }Or conditionally show different messages based on error type if your API returns structured errors.
🤖 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/QRVerify.tsx` around lines 25 - 27, The error handling in the QR verification component does not capture or log error details when verification fails, making it difficult to diagnose the root cause. In the condition where isError is true, add logging of the error object obtained from the useQuery hook to console.error with a descriptive message before returning the generic SGLTypography error message. This will provide diagnostic information (such as expired token, network error, or invalid token) for debugging purposes without changing the user-facing error message.src/pages/QRPrint/PrintQR.tsx (2)
43-43: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional chaining is redundant with query state checks.
The code uses
data?.tokeneven though this line only renders whenisPendingandisErrorare both false. At this point,datashould exist if the query succeeded.However, the optional chaining provides defensive coding that won't break if query states change, so keeping it is reasonable. Consider adding explicit
isSuccesscheck if you want to be more precise:{isSuccess && data.token && <QRGeneration token={data.token} />}🤖 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/QRPrint/PrintQR.tsx` at line 43, The optional chaining in the conditional rendering of QRGeneration component is redundant since query state checks already guarantee data exists. Add an explicit isSuccess check before the conditional that renders QRGeneration to make the query state handling more precise and clear. Change the condition from just checking data?.token to explicitly verifying isSuccess alongside the data.token check in the JSX line containing QRGeneration.
19-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffQuery executes immediately on mount.
The
useQueryhook triggers the QR creation API call as soon as the component mounts, without any user action. Depending on your UX requirements, you might want users to explicitly request QR generation.If user-triggered generation is preferred, consider:
- Using
enabled: falseand a manualrefetch()call- Switching to
useMutationfor explicit control- Adding a "Generate QR" button before showing the QR code
Current behavior may be acceptable for a print-preview page where QR generation is expected immediately.
🤖 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/QRPrint/PrintQR.tsx` around lines 19 - 28, The useQuery hook in the PrintQR component automatically executes the createQR API call on component mount. To make QR generation user-triggered, add the `enabled: false` property to the useQuery configuration to prevent automatic execution, then extract and call the refetch() function returned from useQuery when a user action occurs (such as a button click). Alternatively, replace useQuery with useMutation for more explicit control and attach the mutation trigger to a user action like a "Generate QR" button.
🤖 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/locales/en/translation.json`:
- Line 82: The login.login translation key in the translation.json file is
unused throughout the codebase and should be either removed or documented.
Remove the entire line containing "login.login": "Login" from the translation
file, or if this key is intended for future use, add a comment above it (in
JSON-compatible format) explaining its reserved purpose. Verify that the
remaining login keys (login.patient, login.staff, login.enterPatientNumber,
login.example, login.sendCode, login.quickLogin, login.scanQr,
login.supportInfo) are still properly used in the login flow before confirming
the change.
In `@src/pages/QRPrint/PrintQR.tsx`:
- Line 47: The SGLTypography component is rendering the MOCK_PASSWORD variable
in plain text, which poses a security risk. Replace the direct display of
MOCK_PASSWORD with a masked version (such as asterisks or dots like ••••••••) or
implement a toggle button that allows users to show/hide the password.
Alternatively, if the password display is not essential for the QR workflow,
consider removing it entirely. Update the component to use a state variable to
track visibility if implementing a toggle.
- Line 49: The SGLButton component with the text 'printQR.print' is missing an
onClick handler, so clicking it performs no action. Add an onClick prop to the
SGLButton that calls a print function or method. This function should handle the
print operation, either by calling the browser's print functionality via
window.print() or by triggering your application's custom print logic for QR
codes. Ensure the handler is properly bound and can access any necessary context
or state required to perform the print action.
- Around line 13-14: Remove the hardcoded credentials MOCK_PATIENT_NUMBER and
MOCK_PASSWORD from the module level in PrintQR.tsx and instead load them from
environment variables (using process.env or similar based on your framework).
Ensure these credentials are only used in development or test environments by
either reading from an environment configuration file, using a feature flag to
gate the functionality, or by clearly documenting that real credentials must
never be used here. Update the code to reference the environment variables
instead of the hardcoded constant values.
In `@src/services/QR/QR.service.ts`:
- Around line 10-12: The `createQR` function transmits sensitive authentication
data (userId and password) in the request body without HTTPS enforcement or
encryption, creating a security vulnerability. Before this feature reaches
production, ensure the backend enforces HTTPS on the `/api/login/QR/create` and
`/api/login/QR/verify` endpoints, implement proper password hashing and
encryption mechanisms, add HTTPS enforcement to the frontend apiService
configuration (currently allowing HTTP in development), and replace the TODO
comment in QRGeneration.tsx with actual implementations for authentication
validation, encryption, and token expiration as noted.
---
Nitpick comments:
In `@src/pages/login/QRLogin/QRVerify.tsx`:
- Line 15: In the queryFn callback of the useQuery hook, replace the fallback
pattern `token ?? ''` with the non-null assertion `token!` instead. This makes
the dependency on the enabled guard explicit and removes the fragile empty
string fallback, since the enabled condition (Boolean(token)) guarantees that
token is not null or undefined when the query function executes.
- Around line 13-19: The useQuery hook in QRVerify.tsx is missing cache
invalidation after successful token verification, which could allow the token to
be reused if the user returns to the route. Import useQueryClient from React
Query at the top of the file, then call useQueryClient to get the queryClient
instance. Add an onSuccess callback to the useQuery configuration that calls
queryClient.invalidateQueries with the queryKey ['verifyQR'] to clear the cached
token data after successful verification.
- Around line 25-27: The error handling in the QR verification component does
not capture or log error details when verification fails, making it difficult to
diagnose the root cause. In the condition where isError is true, add logging of
the error object obtained from the useQuery hook to console.error with a
descriptive message before returning the generic SGLTypography error message.
This will provide diagnostic information (such as expired token, network error,
or invalid token) for debugging purposes without changing the user-facing error
message.
In `@src/pages/QRPrint/PrintQR.tsx`:
- Line 43: The optional chaining in the conditional rendering of QRGeneration
component is redundant since query state checks already guarantee data exists.
Add an explicit isSuccess check before the conditional that renders QRGeneration
to make the query state handling more precise and clear. Change the condition
from just checking data?.token to explicitly verifying isSuccess alongside the
data.token check in the JSX line containing QRGeneration.
- Around line 19-28: The useQuery hook in the PrintQR component automatically
executes the createQR API call on component mount. To make QR generation
user-triggered, add the `enabled: false` property to the useQuery configuration
to prevent automatic execution, then extract and call the refetch() function
returned from useQuery when a user action occurs (such as a button click).
Alternatively, replace useQuery with useMutation for more explicit control and
attach the mutation trigger to a user action like a "Generate QR" button.
In `@src/types/qr.types.ts`:
- Line 14: The QRVerifyResponse type is currently defined as Record<string,
unknown>, which lacks type safety and doesn't document the actual response
structure from the backend. Replace this generic definition with a proper
interface or type that specifies the actual properties returned after QR code
verification, such as user session data, authentication tokens, user
information, or success status flags. This will provide better type checking and
self-document what the verification endpoint returns.
🪄 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: 769e5e2f-f281-4391-85fc-2dc4324285dc
📒 Files selected for processing (11)
src/constants/api.constants.tssrc/locales/ar/translation.jsonsrc/locales/en/translation.jsonsrc/locales/he/translation.jsonsrc/locales/ru/translation.jsonsrc/main.tsxsrc/pages/QRPrint/PrintQR.tsxsrc/pages/login/QRLogin/QRVerify.tsxsrc/pages/login/QRLogin/constants.tssrc/services/QR/QR.service.tssrc/types/qr.types.ts
| "balanceTraining.description": "A series of exercises to improve stability and prevent falls, personally tailored to your needs.", | ||
| "balanceTraining.button": "Watch balance training videos", | ||
| "login.patient": "Patient", | ||
| "login.login": "Login", |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for usage of the login.login translation key
rg -n "t\(['\"]login\.login['\"]" --type tsx --type ts --type jsx --type jsRepository: ColmanDevClubORG/Sagol360Management
Length of output: 201
🏁 Script executed:
#!/bin/bash
# Search for usage of the login.login translation key with corrected syntax
rg -i "login\.login" --type ts --type js -g "*.tsx" -g "*.jsx"Repository: ColmanDevClubORG/Sagol360Management
Length of output: 173
🏁 Script executed:
#!/bin/bash
# First, explore the repository structure
echo "=== Repository structure ==="
git ls-files | head -30
echo -e "\n=== Checking for component files that might use translations ==="
find . -type f \( -name "*.tsx" -o -name "*.ts" -o -name "*.jsx" -o -name "*.js" \) ! -path "*/node_modules/*" | head -20
echo -e "\n=== Search for any usage of 'login' string more broadly ==="
rg -i "login" --max-count 5 -g "*.tsx" -g "*.jsx" -g "*.ts" -g "*.js"
echo -e "\n=== Check translation.json file structure ==="
cat src/locales/en/translation.json | head -100Repository: ColmanDevClubORG/Sagol360Management
Length of output: 2085
🏁 Script executed:
#!/bin/bash
# Check the translation.json file directly
echo "=== Checking translation.json content around line 82 ==="
sed -n '75,95p' src/locales/en/translation.json
echo -e "\n=== Search for how translations are used (useTranslation, i18n) ==="
rg "useTranslation|i18n|from.*i18next" | head -20
echo -e "\n=== Check for LoginComponent or Login-related files ==="
find . -type f -iname "*login*" ! -path "*/node_modules/*"
echo -e "\n=== Search for 'login\.' key references in all files ==="
rg "login\." --type json --type ts --type tsx --type js --type jsx 2>/dev/null | head -30Repository: ColmanDevClubORG/Sagol360Management
Length of output: 1383
🏁 Script executed:
#!/bin/bash
# Check for any Login component or page files
echo "=== Search for Login-related component files ==="
find . -type f \( -name "*.tsx" -o -name "*.ts" -o -name "*.jsx" -o -name "*.js" \) ! -path "*/node_modules/*" | xargs grep -l -i "login" 2>/dev/null | head -20
echo -e "\n=== Check App.tsx to see how translations are set up ==="
cat src/App.tsx
echo -e "\n=== Look for i18n configuration files ==="
find . -type f \( -name "*i18n*" -o -name "*i18next*" \) ! -path "*/node_modules/*"
echo -e "\n=== Check package.json for i18n dependencies ==="
grep -A 5 -B 5 "i18n\|i18next\|translation" package.jsonRepository: ColmanDevClubORG/Sagol360Management
Length of output: 4410
🏁 Script executed:
#!/bin/bash
# Check the Login component files to see if they use the login.* translation keys
echo "=== Check Login.tsx ==="
cat src/pages/login/Login.tsx
echo -e "\n=== Check LoginOption.tsx ==="
cat src/pages/login/LoginOption/LoginOption.tsx
echo -e "\n=== Check LoginForm.tsx ==="
head -60 src/pages/login/LoginForm/LoginForm.tsxRepository: ColmanDevClubORG/Sagol360Management
Length of output: 2889
🏁 Script executed:
#!/bin/bash
# Search for actual usage of login.login in all code files
echo "=== Search for login.login string in all files ==="
rg "login\.login" --type ts --type tsx --type js --type jsx
echo -e "\n=== Check LoginPatient and LoginStaff components ==="
head -50 src/pages/login/LoginPatient/LoginPatient.tsx
echo -e "\n=== Check if any other files reference login.login ==="
rg "login\.login" --type jsonRepository: ColmanDevClubORG/Sagol360Management
Length of output: 1867
Remove the unused login.login translation key or clarify its purpose.
The login.login key was added to the translation file but is not referenced anywhere in the codebase. The login flow uses login.patient, login.staff, login.enterPatientNumber, login.example, login.sendCode, login.quickLogin, login.scanQr, and login.supportInfo instead. Either remove this unused key or add a comment explaining if it's reserved for future use.
🤖 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/locales/en/translation.json` at line 82, The login.login translation key
in the translation.json file is unused throughout the codebase and should be
either removed or documented. Remove the entire line containing "login.login":
"Login" from the translation file, or if this key is intended for future use,
add a comment above it (in JSON-compatible format) explaining its reserved
purpose. Verify that the remaining login keys (login.patient, login.staff,
login.enterPatientNumber, login.example, login.sendCode, login.quickLogin,
login.scanQr, login.supportInfo) are still properly used in the login flow
before confirming the change.
| const MOCK_PATIENT_NUMBER = '1622017' | ||
| const MOCK_PASSWORD = '123456789' |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Remove hardcoded credentials before merging to production.
The mock patient number and password are hardcoded as module-level constants. This poses a security risk if these credentials correspond to real accounts or if this code is deployed without proper feature flags.
Consider one of these approaches:
- Use environment variables or configuration files for test credentials
- Gate this feature behind a feature flag
- Add clear warnings in code comments that these must not be real credentials
🛡️ Suggested refactor to use environment variables
-const MOCK_PATIENT_NUMBER = '1622017'
-const MOCK_PASSWORD = '123456789'
+const MOCK_PATIENT_NUMBER = import.meta.env.VITE_MOCK_PATIENT_NUMBER || ''
+const MOCK_PASSWORD = import.meta.env.VITE_MOCK_PASSWORD || ''📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const MOCK_PATIENT_NUMBER = '1622017' | |
| const MOCK_PASSWORD = '123456789' | |
| const MOCK_PATIENT_NUMBER = import.meta.env.VITE_MOCK_PATIENT_NUMBER || '' | |
| const MOCK_PASSWORD = import.meta.env.VITE_MOCK_PASSWORD || '' |
🤖 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/QRPrint/PrintQR.tsx` around lines 13 - 14, Remove the hardcoded
credentials MOCK_PATIENT_NUMBER and MOCK_PASSWORD from the module level in
PrintQR.tsx and instead load them from environment variables (using process.env
or similar based on your framework). Ensure these credentials are only used in
development or test environments by either reading from an environment
configuration file, using a feature flag to gate the functionality, or by
clearly documenting that real credentials must never be used here. Update the
code to reference the environment variables instead of the hardcoded constant
values.
|
|
||
| <SGLTypography styles={styles.textStyle}>{MOCK_PATIENT_NUMBER}</SGLTypography> | ||
|
|
||
| <SGLTypography styles={styles.textStyle}>{MOCK_PASSWORD}</SGLTypography> |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Avoid displaying passwords in plain text.
The password is rendered directly on the screen without masking. This creates a security risk if someone views the screen over the user's shoulder or if the page is screenshotted/recorded.
Consider:
- Masking the password (show
••••••••or similar) - Adding a toggle button to reveal/hide if users need to see it
- Removing password display entirely if not required for the QR workflow
🤖 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/QRPrint/PrintQR.tsx` at line 47, The SGLTypography component is
rendering the MOCK_PASSWORD variable in plain text, which poses a security risk.
Replace the direct display of MOCK_PASSWORD with a masked version (such as
asterisks or dots like ••••••••) or implement a toggle button that allows users
to show/hide the password. Alternatively, if the password display is not
essential for the QR workflow, consider removing it entirely. Update the
component to use a state variable to track visibility if implementing a toggle.
|
|
||
| export const buildQrLoginUrl = (token: string) => | ||
| `${window.location.origin}/qr-login?token=${encodeURIComponent(token)}` | ||
| `${window.location.origin}${QR_VERIFY_ROUTE}?token=${encodeURIComponent(token)}` |
There was a problem hiding this comment.
Why via window.location?
Generally we prefer to minimize the usage of window
There was a problem hiding this comment.
I will add and say that we should add locationService or relay on react router library
| { path: '/', element: <Navigate to="/login" replace /> }, | ||
| { path: '/login', element: <Login /> }, | ||
| { path: '/printQR', element: <PrintQR /> }, | ||
| { path: QR_VERIFY_ROUTE, element: <QRVerify /> }, |
There was a problem hiding this comment.
Why do you verify the qr here?
There was a problem hiding this comment.
main.tsx only registers the frontend landing route for the scanned QR URL.
The actual verification is still done by the backend: QRVerify reads the token from the URL, sends it to the backend verify API, and redirects to /home on success.
5fcf2fc to
0bf8425
Compare
|
|
||
| export const buildQrLoginUrl = (token: string) => | ||
| `${window.location.origin}/qr-login?token=${encodeURIComponent(token)}` | ||
| `${window.location.origin}${QR_VERIFY_ROUTE}?token=${encodeURIComponent(token)}` |
There was a problem hiding this comment.
I will add and say that we should add locationService or relay on react router library
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/QRLogin/constants.ts`:
- Around line 3-10: The QR login URL builder is falling back to an empty base
URL, which can generate a relative QR target that won’t work when scanned.
Update getAppBaseUrl/buildQrLoginUrl to validate that VITE_APP_BASE_URL is
present and absolute, and fail fast (e.g. throw or otherwise surface a config
error) instead of returning an empty-string fallback. Keep the normalization
logic in buildQrLoginUrl, but only after the base URL has been verified.
🪄 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: c522157e-5ef8-4023-885a-1f9646cda76c
📒 Files selected for processing (3)
.env.examplesrc/pages/QRPrint/PrintQR.tsxsrc/pages/login/QRLogin/constants.ts
✅ Files skipped from review due to trivial changes (1)
- .env.example
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pages/QRPrint/PrintQR.tsx
| const getAppBaseUrl = () => import.meta.env.VITE_APP_BASE_URL || '' | ||
|
|
||
| export const buildQrLoginUrl = (token: string) => { | ||
| const appBaseUrl = getAppBaseUrl() | ||
| const normalizedAppBaseUrl = appBaseUrl.endsWith('/') ? appBaseUrl.slice(0, -1) : appBaseUrl | ||
|
|
||
| return `${normalizedAppBaseUrl}${QR_VERIFY_ROUTE}?token=${encodeURIComponent(token)}` | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Empty VITE_APP_BASE_URL fallback produces a relative QR URL that can't be scanned.
This URL is encoded into a printed QR code (QRGeneration → QRCodeCanvas value={buildQrLoginUrl(token)}) that is scanned by an external device. When VITE_APP_BASE_URL is unset, getAppBaseUrl() returns '', so the result becomes a relative path like /QR/verify?token=.... A relative URL has no origin and cannot resolve when scanned by a phone camera, silently yielding non-functional QR codes instead of failing fast on misconfiguration.
Consider failing fast (or validating an absolute URL) rather than defaulting to ''.
🐛 Proposed fix to fail fast on missing/invalid base URL
-const getAppBaseUrl = () => import.meta.env.VITE_APP_BASE_URL || ''
+const getAppBaseUrl = () => {
+ const baseUrl = import.meta.env.VITE_APP_BASE_URL
+ if (!baseUrl) {
+ throw new Error('VITE_APP_BASE_URL is not configured; cannot build an absolute QR verification URL.')
+ }
+ return baseUrl
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const getAppBaseUrl = () => import.meta.env.VITE_APP_BASE_URL || '' | |
| export const buildQrLoginUrl = (token: string) => { | |
| const appBaseUrl = getAppBaseUrl() | |
| const normalizedAppBaseUrl = appBaseUrl.endsWith('/') ? appBaseUrl.slice(0, -1) : appBaseUrl | |
| return `${normalizedAppBaseUrl}${QR_VERIFY_ROUTE}?token=${encodeURIComponent(token)}` | |
| } | |
| const getAppBaseUrl = () => { | |
| const baseUrl = import.meta.env.VITE_APP_BASE_URL | |
| if (!baseUrl) { | |
| throw new Error('VITE_APP_BASE_URL is not configured; cannot build an absolute QR verification URL.') | |
| } | |
| return baseUrl | |
| } | |
| export const buildQrLoginUrl = (token: string) => { | |
| const appBaseUrl = getAppBaseUrl() | |
| const normalizedAppBaseUrl = appBaseUrl.endsWith('/') ? appBaseUrl.slice(0, -1) : appBaseUrl | |
| return `${normalizedAppBaseUrl}${QR_VERIFY_ROUTE}?token=${encodeURIComponent(token)}` | |
| } |
🤖 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/constants.ts` around lines 3 - 10, The QR login URL
builder is falling back to an empty base URL, which can generate a relative QR
target that won’t work when scanned. Update getAppBaseUrl/buildQrLoginUrl to
validate that VITE_APP_BASE_URL is present and absolute, and fail fast (e.g.
throw or otherwise surface a config error) instead of returning an empty-string
fallback. Keep the normalization logic in buildQrLoginUrl, but only after the
base URL has been verified.
3 participants
Description
Please include a summary of the changes and the related issue.
Related Issue(s)
Fixes # (issue number)
Checklist:
Screenshots (if appropriate):
Summary by CodeRabbit