refactor: server-side QR extraction for institution submissions#560
Conversation
Move QR code decoding from browser-only to server-side using sharp + @zxing/library. This is more reliable than client-side canvas decoding, which fails on compressed images due to binarization issues. Changes: - Create lib/qr-decode.ts: shared server-side QR decode utility extracted from the admin QR replacement tool - Update submit-institution.ts: decode QR from image buffer after R2 upload, with fallback to client-provided value during transition - Update upload-qr-replacement.ts: use shared utility instead of inline decode (DRY) - Add server-side duplicate QR check after extraction - Persist qrContent in DB insert (was previously only used for payment type detection) Addresses khrnchn#553
📝 WalkthroughWalkthroughShared QR decoding was extracted to Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (browser/form)
participant Server as Server (submit / admin endpoints)
participant QR as decodeQrFromBuffer
participant DB as Database (institutions)
participant R2 as R2 Storage
Client->>Server: POST form + optional QR image
Server->>QR: decodeQrFromBuffer(imageBuffer)
QR-->>Server: decodedText|null
alt decodedText present
Server->>DB: query for existing institution by decodedText
else decodedText null
Server->>Server: use trimmed client qrContent (if any)
Server->>DB: query for existing institution by final qrContent
end
alt duplicate not found
Server->>R2: upload QR image
R2-->>Server: upload URL/metadata
Server->>DB: insert institution (qrContent || null, r2 metadata...)
DB-->>Server: insert result
Server-->>Client: success
else duplicate found
Server-->>Client: error (duplicate)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
- Add .toColorspace("srgb") before .ensureAlpha() to force 3-channel
RGB output for grayscale inputs (which would otherwise produce
2-channel GA data, corrupting the RGBA-to-RGB pixel copy loop)
- Add channel count guard before pixel processing
- Use arrow function per project convention
Addresses CodeRabbit review feedback on khrnchn#560.
|
Add pre-existing bug fix found by coderabbitai |
thanks for this! will look into it in the morning |
- Move QR decoding and duplicate check before R2 upload to prevent orphaned objects in storage when duplicates are rejected - Remove redundant early client-side duplicate check (server-side extraction is now authoritative) - Add console.warn to QR decode catch block for observability
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/`(user)/contribute/_lib/submit-institution.ts:
- Around line 293-311: Add a DB-level unique constraint on
institutions.qrContent to prevent race conditions: update the Drizzle schema for
the institutions table to mark qrContent as .unique() (or run an ALTER TABLE ...
ADD UNIQUE(qr_content) migration) and create/apply the migration; keep the
existing application-level duplicate check around qrContent but also catch
unique-constraint violations on insert (the code that writes the institution
record) and translate that DB error into the same return payload used when an
existing QR is detected so concurrent inserts return the same user-facing error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f1284187-5ddc-4f00-b623-6fa86fa21aff
📒 Files selected for processing (2)
app/(user)/contribute/_lib/submit-institution.tslib/qr-decode.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/qr-decode.ts
| // --- Duplicate QR check (before R2 upload to avoid orphaned objects) | ||
| if (qrContent) { | ||
| const [existingQr] = await db | ||
| .select({ id: institutions.id }) | ||
| .from(institutions) | ||
| .where(eq(institutions.qrContent, qrContent)) | ||
| .limit(1); | ||
|
|
||
| if (existingQr) { | ||
| return { | ||
| status: "error", | ||
| errors: { | ||
| general: [ | ||
| "QR code ini telah pun wujud dalam sistem. Sila semak semula.", | ||
| ], | ||
| }, | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if institutions table has a unique constraint/index on qrContent
fd -t f -e ts 'institutions' --exec grep -l 'qrContent' {} | head -5 | xargs -I {} sh -c 'echo "=== {} ===" && cat {}'
# Look for unique constraint or index definition
rg -n 'unique|index' -A2 -B2 --type ts | grep -i 'qrContent' -A2 -B2Repository: khrnchn/sedekah-je
Length of output: 50375
Add unique constraint on institutions.qrContent to prevent race conditions.
The duplicate check (lines 295-299) runs before the database insert, but there's a window where concurrent requests could insert the same qrContent, bypassing the application-level check. The qrContent field currently lacks a database-level unique constraint. Adding one will prevent duplicate entries even in race conditions:
ALTER TABLE institutions ADD UNIQUE(qr_content);Or in the Drizzle schema definition, update the field to include .unique().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/`(user)/contribute/_lib/submit-institution.ts around lines 293 - 311, Add
a DB-level unique constraint on institutions.qrContent to prevent race
conditions: update the Drizzle schema for the institutions table to mark
qrContent as .unique() (or run an ALTER TABLE ... ADD UNIQUE(qr_content)
migration) and create/apply the migration; keep the existing application-level
duplicate check around qrContent but also catch unique-constraint violations on
insert (the code that writes the institution record) and translate that DB error
into the same return payload used when an existing QR is detected so concurrent
inserts return the same user-facing error.
|
lgtm thank you sir |
|
current project sedang develop (kerja hakiki) pun using the same approach (but more complex 💀 for no reason). Anyway, happy to help project |
kim salam akif dengan aimand hahaha |
Addresses #553
Summary
Moves QR code decoding from browser-only (canvas +
@zxing/browser) to server-side (sharp+@zxing/library) for the institution submission flow. Server-side extraction is more reliable becausesharppreserves full image fidelity without browser rendering variance or canvas compression artifacts.What changed
lib/qr-decode.tssubmit-institution.tsupload-qr-replacement.tsdecodeQrFromBuffer()instead of inline decode (37 lines -> 1 line)How it works
sharp+@zxing/libraryqrContentif server extraction fails (belt-and-suspenders during transition)qrContentinto DB (previously only used for payment type detection, not persisted)What stays the same
Follow-up opportunities
The client-side extraction code (
useQrExtractionLazy,QRProcessor,QRExtractionFeature) can be simplified in a follow-up PR now that the server handles extraction. These are shared across multiple features (main form, edit sheet, quest form, admin replacement), so removing them requires updating all consumers:hooks/use-qr-extraction-lazy.ts-- used by quest form + admin replacement uploadapp/(user)/contribute/_components/qr-processor.tsx-- bridge componentapp/(user)/contribute/_components/qr-extraction-feature.tsx-- upload UI + extraction feedback@zxing/browsercan be removed frompackage.jsononce all consumers migrateTest plan
qrContentis populated in DBqrContent = nullSummary by CodeRabbit
Refactor
Improvements