Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughAdds the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 7 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (7 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/blade/src/app/_components/shared/scanner.tsx (1)
237-268:⚠️ Potential issue | 🟡 MinorAdd length validation before extracting userId from QR code.
The QR code contains
user:${userId}(generated server-side), sosubstring(5)extracts the userId. However, if the QR code content is shorter than expected, this returns an empty or partial string. While the API will return a "not found" error, it's better UX to validate the format immediately and show a clear message.The server-side schema (
z.string()) doesn't validate length, so defensive validation on the client improves the experience.🛡️ Suggested fix
const result = detectedCodes[0]; if (!result) return; + if (!result.rawValue || result.rawValue.length < 6) { + toast.error("Invalid QR code format"); + return; + } if (scanningRef.current) return;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/blade/src/app/_components/shared/scanner.tsx` around lines 237 - 268, Validate the scanned QR payload before slicing: inside the Scanner onScan handler, check result.rawValue startsWith("user:") and has a sufficient length (e.g., rawValue.length > 5) before calling result.rawValue.substring(5); if it fails, show a clear toast.error like "Invalid QR code" and return early (ensure scanningRef.current is still handled as in the finally block). Then proceed to set form.setValue("userId", userId) and the existing flow that calls form.handleSubmit which triggers hackerEventCheckIn.mutate or memberCheckIn.mutate based on eventType.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/blade/package.json`:
- Line 40: Remove the old react-qr-reader dependency from package.json to
complete the migration to `@yudiel/react-qr-scanner`: locate the dependencies
block in package.json and delete the "react-qr-reader" entry (the legacy package
string), ensuring only "@yudiel/react-qr-scanner": "^2.5.1" remains so there are
no duplicate/unused QR scanner packages listed.
---
Outside diff comments:
In `@apps/blade/src/app/_components/shared/scanner.tsx`:
- Around line 237-268: Validate the scanned QR payload before slicing: inside
the Scanner onScan handler, check result.rawValue startsWith("user:") and has a
sufficient length (e.g., rawValue.length > 5) before calling
result.rawValue.substring(5); if it fails, show a clear toast.error like
"Invalid QR code" and return early (ensure scanningRef.current is still handled
as in the finally block). Then proceed to set form.setValue("userId", userId)
and the existing flow that calls form.handleSubmit which triggers
hackerEventCheckIn.mutate or memberCheckIn.mutate based on eventType.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 6c546670-3ab3-410f-9623-f2b4e464e26d
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!pnpm-lock.yaml
📒 Files selected for processing (3)
apps/blade/package.jsonapps/blade/src/app/_components/admin/club/members/scanner.tsxapps/blade/src/app/_components/shared/scanner.tsx
| "@trpc/client": "catalog:", | ||
| "@trpc/react-query": "catalog:", | ||
| "@trpc/server": "catalog:", | ||
| "@yudiel/react-qr-scanner": "^2.5.1", |
There was a problem hiding this comment.
Remove the old react-qr-reader dependency.
The new @yudiel/react-qr-scanner is added (line 40), but react-qr-reader (line 56) is still listed. Since the migration replaces the old library, it should be removed to reduce bundle size and avoid confusion.
🧹 Suggested fix
"@yudiel/react-qr-scanner": "^2.5.1",
"canvas-confetti": "^1.9.4",
...
"react-qr-code": "^2.0.18",
- "react-qr-reader": "3.0.0-beta-1",
"recharts": "^3.7.0",Also applies to: 56-56
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/blade/package.json` at line 40, Remove the old react-qr-reader
dependency from package.json to complete the migration to
`@yudiel/react-qr-scanner`: locate the dependencies block in package.json and
delete the "react-qr-reader" entry (the legacy package string), ensuring only
"@yudiel/react-qr-scanner": "^2.5.1" remains so there are no duplicate/unused QR
scanner packages listed.
There was a problem hiding this comment.
@myr124, glad it helps! Since react-qr-reader is still listed in package.json, here's the one-line fix needed:
- "react-qr-reader": "3.0.0-beta-1",Also make sure to run pnpm install after the change to update the lockfile and fully remove the old package from node_modules. This keeps the bundle lean and avoids any accidental import of the legacy library in the future.
Would you like me to plan that change for you?
There was a problem hiding this comment.
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)
MIGRATION_STATUS.md (1)
66-92:⚠️ Potential issue | 🟡 MinorRemove false duplicate function references from MIGRATION_STATUS.md.
Lines 66–92 reference duplicate functions in
apps/blade/src/lib/utils.tsthat don't exist:
formatDateRange(line 68–79): No duplicate atapps/blade/src/lib/utils.ts:29. Only definition is atpackages/utils/src/time.ts:54.getPermsAsList(line 82–92): No duplicate atapps/blade/src/lib/utils.ts:120. Only definition is atpackages/utils/src/permissions.ts:45.The file
apps/blade/src/lib/utils.tsdoesn't exist. Remove these false migration items or update with actual duplicates if they exist elsewhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MIGRATION_STATUS.md` around lines 66 - 92, Update MIGRATION_STATUS.md to remove or correct the false duplicate entries: delete the `formatDateRange` and `getPermsAsList` duplicate references (lines describing `apps/blade/src/lib/utils.ts`) since those functions only exist in `packages/utils` (`formatDateRange`/`formatTimeRange` at packages/utils/src/time.ts and `getPermsAsList` at packages/utils/src/permissions.ts); alternatively, if a real duplicate exists elsewhere, replace the incorrect path with the actual file where the duplicate lives and update the suggested actions (e.g., import fixes to `@forge/utils` and renaming guidance for `formatDateRange`/`formatTimeRange`) so the migration notes accurately reflect the codebase.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/blade/src/app/_components/shared/scanner.tsx`:
- Around line 241-248: Validate result.rawValue in the onScan handler before
slicing: ensure result and result.rawValue are defined and that rawValue has at
least 6 characters and the expected prefix pattern (the first 5 chars) before
calling result.rawValue.substring(5); if validation fails, log or ignore the
scan and reset scanningRef appropriately to avoid silencing errors or setting an
empty userId; update the onScan logic (referencing onScan, scanningRef.current,
result, result.rawValue, and form.setValue("userId", ...)) to perform this
defensive check and handle malformed QR payloads gracefully.
- Around line 237-240: The QR handler currently assumes result.rawValue is
present and long enough (used to compute userId via substring) which can crash
on malformed QR data; update the QR scan result handling (the code that reads
result.rawValue and assigns userId) to first validate that result.rawValue is a
non-empty string and has at least 5 characters (and optionally startsWith the
expected prefix), show a user-facing error via toast.error and return early if
validation fails, otherwise safely compute userId = result.rawValue.substring(5)
and proceed; references: Scanner component, result.rawValue usage, userId
extraction, and toast.error.
---
Outside diff comments:
In `@MIGRATION_STATUS.md`:
- Around line 66-92: Update MIGRATION_STATUS.md to remove or correct the false
duplicate entries: delete the `formatDateRange` and `getPermsAsList` duplicate
references (lines describing `apps/blade/src/lib/utils.ts`) since those
functions only exist in `packages/utils` (`formatDateRange`/`formatTimeRange` at
packages/utils/src/time.ts and `getPermsAsList` at
packages/utils/src/permissions.ts); alternatively, if a real duplicate exists
elsewhere, replace the incorrect path with the actual file where the duplicate
lives and update the suggested actions (e.g., import fixes to `@forge/utils` and
renaming guidance for `formatDateRange`/`formatTimeRange`) so the migration
notes accurately reflect the codebase.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 1c8cc80d-0395-44d0-93a8-a74e3d0fdebc
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!pnpm-lock.yaml
📒 Files selected for processing (3)
MIGRATION_STATUS.mdapps/blade/src/app/_components/admin/club/members/scanner.tsxapps/blade/src/app/_components/shared/scanner.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/blade/src/app/_components/admin/club/members/scanner.tsx
| <Scanner | ||
| scanDelay={2000} | ||
| constraints={{ facingMode: "environment" }} | ||
| onResult={async (result, _) => { | ||
| formats={["qr_code"]} |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
@yudiel/react-qr-scanner version 2.5.1 API documentation formats prop
💡 Result:
In @yudiel/react-qr-scanner v2.5.1, the Scanner prop:
formats?: BarcodeFormat[]— Optional. A list of barcode formats to detect. Default: “All” (if you don’t pass it, it will detect all supported formats). [1]
BarcodeFormat is a string union of:
'aztec' | 'code_128' | 'code_39' | 'code_93' | 'codabar' | 'databar' | 'databar_expanded' | 'data_matrix' | 'dx_film_edge' | 'ean_13' | 'ean_8' | 'itf' | 'maxi_code' | 'micro_qr_code' | 'pdf417' | 'qr_code' | 'rm_qr_code' | 'upc_a' | 'upc_e' | 'linear_codes' | 'matrix_codes' | 'unknown'. [1]
Example: formats={['qr_code','ean_13','code_128']}. [1]
Sources:
[1] v2.5.1 README (API Reference + Type Definitions) (raw.githubusercontent.com)
Citations:
🏁 Script executed:
find . -name "scanner.tsx" | grep -E "apps/blade"Repository: KnightHacks/forge
Length of output: 176
🏁 Script executed:
cat -n ./apps/blade/src/app/_components/shared/scanner.tsx | sed -n '230,260p'Repository: KnightHacks/forge
Length of output: 1527
🏁 Script executed:
cat -n ./apps/blade/src/app/_components/shared/scanner.tsx | head -20Repository: KnightHacks/forge
Length of output: 707
🏁 Script executed:
cat -n ./apps/blade/src/app/_components/shared/scanner.tsx | sed -n '240,275p'Repository: KnightHacks/forge
Length of output: 1656
Approve library migration, but add defensive validation for QR code format.
The formats={["qr_code"]} configuration aligns with @yudiel/react-qr-scanner v2.5.1 API. However, line 247 assumes rawValue exists and has at least 5 characters without validation:
const userId = result.rawValue?.substring(5) ?? "";
// or with explicit check:
if (!result.rawValue || result.rawValue.length < 5) {
toast.error("Invalid QR code format");
return;
}
const userId = result.rawValue.substring(5);This prevents runtime errors if the QR code format differs from expectations (e.g., missing prefix or malformed data).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/blade/src/app/_components/shared/scanner.tsx` around lines 237 - 240,
The QR handler currently assumes result.rawValue is present and long enough
(used to compute userId via substring) which can crash on malformed QR data;
update the QR scan result handling (the code that reads result.rawValue and
assigns userId) to first validate that result.rawValue is a non-empty string and
has at least 5 characters (and optionally startsWith the expected prefix), show
a user-facing error via toast.error and return early if validation fails,
otherwise safely compute userId = result.rawValue.substring(5) and proceed;
references: Scanner component, result.rawValue usage, userId extraction, and
toast.error.
| onScan={async (detectedCodes) => { | ||
| const result = detectedCodes[0]; | ||
| if (!result) return; | ||
| if (scanningRef.current) return; | ||
| scanningRef.current = true; | ||
| try { | ||
| const userId = result.getText().substring(5); | ||
| const userId = result.rawValue.substring(5); | ||
| form.setValue("userId", userId); |
There was a problem hiding this comment.
Add defensive validation for rawValue before extraction.
Line 247 assumes rawValue exists and follows a specific format (prefix of 5 characters). If the QR code is malformed or from an unexpected source, this could silently produce an empty userId or throw if rawValue is undefined.
Consider validating the format:
🛡️ Suggested defensive check
onScan={async (detectedCodes) => {
const result = detectedCodes[0];
if (!result) return;
+ if (!result.rawValue || result.rawValue.length < 5) {
+ toast.error("Invalid QR code format");
+ return;
+ }
if (scanningRef.current) return;
scanningRef.current = true;
try {
const userId = result.rawValue.substring(5);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/blade/src/app/_components/shared/scanner.tsx` around lines 241 - 248,
Validate result.rawValue in the onScan handler before slicing: ensure result and
result.rawValue are defined and that rawValue has at least 6 characters and the
expected prefix pattern (the first 5 chars) before calling
result.rawValue.substring(5); if validation fails, log or ignore the scan and
reset scanningRef appropriately to avoid silencing errors or setting an empty
userId; update the onScan logic (referencing onScan, scanningRef.current,
result, result.rawValue, and form.setValue("userId", ...)) to perform this
defensive check and handle malformed QR payloads gracefully.
2 participants
that
Why
Fixes lack of video output in qr code scanning
What
Closes: #410
Replaced react-qr-scanner library with newer one from yudiel that has very similar components. I think it works better with the more strict mounting behavior for react 19 .
Test Plan
I went into events page and opened up scanner, set it to project launch event and video output showed up with my qr code scanning successfully
Checklist
db:pushbefore mergingSummary by CodeRabbit
New Features
Chores
Documentation