feat: implement admin templates bulk upload and matching#150
Conversation
|
@shridmishra is attempting to deploy a commit to the Pankaj's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 WalkthroughWalkthroughAdds bulk CRC template upload support across the backend route, API client, and admin page. The flow accepts multiple ChangesCRC bulk template upload
Sequence Diagram(s)sequenceDiagram
participant CRCAdminPage
participant ApiService
participant CRCBulkUploadRoute
participant AdmZip
participant UploadThing
participant crc_controls
participant crc_control_templates
CRCAdminPage->>ApiService: uploadCRCTemplatesBulk(files)
ApiService->>CRCBulkUploadRoute: POST /crc/templates/bulk-upload
CRCBulkUploadRoute->>AdmZip: extract ZIP entries and collect .doc/.docx files
CRCBulkUploadRoute->>crc_controls: load control_id values
CRCBulkUploadRoute->>UploadThing: upload matched template file
CRCBulkUploadRoute->>crc_control_templates: upsert URL, file key, filename, size, updated_at
CRCBulkUploadRoute-->>ApiService: summary and per-file details
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
61657a6 to
310c944
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@backend/src/routes/crc.ts`:
- Around line 1994-2002: The audit logging in the CRC template upload flow can
still throw after the UploadThing upload and DB upsert have succeeded, which
causes the request to be marked failed incorrectly. Update the `recordEvent`
call in the CRC route so it does not control the success/failure of the
persisted upload path; wrap it separately with its own error handling or route
it through a transactional/outbox-style audit path. Keep the main upload/update
logic in the same flow around the `recordEvent` and the success response so a
post-save audit failure does not flip the upload outcome.
- Around line 1892-1898: The extracted template handling in the crc route
assigns every Word file the DOCX MIME type even when the entry name ends with
.doc, so update the logic in the file-processing block that builds
templatesToProcess to choose the mimetype based on the actual extension. Use the
existing entry.name checks in that branch to differentiate .docx from .doc, and
preserve the correct Word MIME for each so UploadThing/download metadata remains
accurate.
- Around line 1986-1991: The legacy-file cleanup in the control/template route
bypasses the same path whitelist used elsewhere, so malformed stored control IDs
could be turned into unsafe unlink targets. Update the cleanup logic in the
control route to use resolveTemplatePath for each legacy extension before
calling fs.existsSync/fs.unlinkSync, and only remove the file when the resolved
path is valid and stays within TEMPLATES_DIR. Keep the fix localized around
matchedControlId and the legacy .docx/.doc removal loop.
- Around line 1926-1936: The template-to-control matching in the CRC route is
currently substring-based in the loop over templatesToProcess, which can cause
an unknown longer control name to match a shorter valid control and let
duplicate files silently overwrite each other. Update the matching logic around
matchedControlId/validControlIds to require bounded token-style matches instead
of plain includes, and precompute/validate matches so any ambiguous or duplicate
template-to-control assignment is rejected before upload processing continues.
- Around line 70-72: The bulk template upload route currently only limits
per-file size via bulkTemplateUpload, so /templates/bulk-upload can still accept
an excessive total payload. Tighten the request budget by adding an overall
upload cap in the bulkTemplateUpload multer configuration or by reducing the
array("files", 150) count/per-file limits in the route handler, keeping the fix
localized to bulkTemplateUpload and the /templates/bulk-upload path.
- Around line 1882-1883: The ZIP handling in crc.ts currently calls
AdmZip.extractAllTo before applying the template filter, which can unpack
irrelevant or malicious contents first. Update the extraction flow around the
zip extraction logic to pre-scan the archive entries, enforce limits on entry
count and total uncompressed size, and then extract only the allowed .doc/.docx
template files. Use the existing zip, extractDir, and AdmZip-based code path to
move filtering ahead of extraction so non-template entries are never
materialized.
In `@frontend/src/app/admin/crc/page.tsx`:
- Around line 468-473: The handleTemplateBulkFileChange handler keeps the hidden
file input value, so re-selecting the same file after removal won’t trigger
onChange. Update this handler to clear the input element after copying
e.target.files into setTemplateBulkFiles, following the same reset pattern
already used by the single-template upload handler in this page.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e31c8895-c9bf-45fa-808c-07c11cf407dc
⛔ Files ignored due to path filters (1)
backend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
backend/package.jsonbackend/src/routes/crc.tsfrontend/src/app/admin/crc/page.tsxfrontend/src/lib/api.ts
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 (2)
frontend/src/app/admin/crc/page.tsx (1)
2051-2053: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdd an
aria-labelto the per-file remove button.This icon-only button has no accessible name, so screen readers announce it without context. Every other icon-only button on this page (upload/replace/delete, preview-row edit/remove) is labeled; match that pattern here.
♿ Proposed fix
- <Button variant="ghost" size="icon" className="size-6 text-muted-foreground hover:text-foreground" onClick={() => handleTemplateBulkRemoveFile(idx)} disabled={templateBulkUploading}> + <Button variant="ghost" size="icon" className="size-6 text-muted-foreground hover:text-foreground" onClick={() => handleTemplateBulkRemoveFile(idx)} disabled={templateBulkUploading} aria-label={`Remove ${file.name}`}>🤖 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 `@frontend/src/app/admin/crc/page.tsx` around lines 2051 - 2053, The per-file remove button in the template bulk upload list is an icon-only control without an accessible name. Update the Button rendered near handleTemplateBulkRemoveFile so it includes a clear aria-label describing the action, matching the labeling pattern used by the other icon-only buttons on this page.backend/src/routes/crc.ts (1)
1944-1948: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winNormalize direct-upload MIME metadata from the extension.
The ZIP path now sets
.doc/.docxMIME types correctly, but direct files admitted by extension fallback still pass through whatever multipart MIME the client sent, such asapplication/octet-stream. Derive the Word MIME fromfile.originalnamebefore creating the UploadThingFile.Proposed fix
+ const nameLower = file.originalname.toLowerCase(); + const mimetype = nameLower.endsWith(".docx") + ? "application/vnd.openxmlformats-officedocument.wordprocessingml.document" + : "application/msword"; templatesToProcess.push({ filePath: file.path, originalname: file.originalname, - mimetype: file.mimetype, + mimetype, size: file.size, isTemp: false, // Will be deleted at the end as part of standard multer file cleanups });🤖 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 `@backend/src/routes/crc.ts` around lines 1944 - 1948, The direct-upload path in the crc route is still using the client-provided multipart mimetype, so Word files admitted by extension fallback can be mislabeled. Update the logic around templatesToProcess.push in the upload handling flow to derive the MIME type from file.originalname before constructing the UploadThing File, matching the ZIP path’s .doc/.docx normalization and avoiding application/octet-stream for Word documents.
🤖 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 `@backend/src/routes/crc.ts`:
- Around line 1888-1907: The ZIP safety checks are currently applied per archive
inside the CRC route, so a single request can still exceed the intended limits
across many ZIPs. Update the ZIP extraction flow in the crc route handler to
keep request-scoped counters for total extracted template count and total
uncompressed bytes, and enforce the same limits cumulatively across all archives
processed in the request. Use the existing ZIP entry scan/extraction logic
around entries, MAX_ZIP_ENTRIES, and MAX_ZIP_UNCOMPRESSED_SIZE to validate
against the shared request totals before matching/uploading.
---
Outside diff comments:
In `@backend/src/routes/crc.ts`:
- Around line 1944-1948: The direct-upload path in the crc route is still using
the client-provided multipart mimetype, so Word files admitted by extension
fallback can be mislabeled. Update the logic around templatesToProcess.push in
the upload handling flow to derive the MIME type from file.originalname before
constructing the UploadThing File, matching the ZIP path’s .doc/.docx
normalization and avoiding application/octet-stream for Word documents.
In `@frontend/src/app/admin/crc/page.tsx`:
- Around line 2051-2053: The per-file remove button in the template bulk upload
list is an icon-only control without an accessible name. Update the Button
rendered near handleTemplateBulkRemoveFile so it includes a clear aria-label
describing the action, matching the labeling pattern used by the other icon-only
buttons on this page.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 61ad2a52-d589-44cb-8839-16715d982863
📒 Files selected for processing (2)
backend/src/routes/crc.tsfrontend/src/app/admin/crc/page.tsx
Summary by CodeRabbit
.doc,.docx, and.zipfiles and showing a per-file results breakdown (success/unmatched/failed) and totals.