fix: update import status to error on invalid file type validation#38522
Open
Lakshyalamba wants to merge 4 commits intoRocketChat:developfrom
Open
fix: update import status to error on invalid file type validation#38522Lakshyalamba wants to merge 4 commits intoRocketChat:developfrom
Lakshyalamba wants to merge 4 commits intoRocketChat:developfrom
Conversation
Contributor
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
🦋 Changeset detectedLatest commit: 4676005 The changes in this PR will be included in the next version bump. This PR includes changesets to release 40 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
WalkthroughServer validation now treats CSV imports with non- Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant DB
Client->>Server: getImportFileData(importId)
Server->>DB: fetch import operation record
alt importRecord.valid && not (type == "CSV" && contentType != "text/csv")
Server-->>Client: continue/waiting response
else invalid OR (type == "CSV" && contentType != "text/csv")
Server->>DB: update import operation { status: "ERROR", valid: false }
Server-->>Client: error/throw
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@apps/meteor/app/importer/server/methods/getImportFileData.ts`:
- Around line 38-49: The preflight CSV check in getImportFileData rejects valid
CSVs by only allowing 'text/csv'; change the isInvalidCSV logic so
operation.importerKey === 'csv' checks operation.contentType against a list of
common CSV MIME types (e.g. 'text/csv', 'text/plain',
'application/vnd.ms-excel', 'text/comma-separated-values', plus any other
platform variants) instead of a strict inequality, by replacing the
single-string check with an allowlist (e.g.
allowedCsvTypes.includes(operation.contentType)); keep the existing behavior
that marks the operation invalid via Imports.updateOne and throws the
Meteor.Error when the content type is not in the allowlist, and apply the same
relaxed check in the frontend ImportOperationSummary.tsx at the referenced line
to avoid duplicate false failures.
In `@apps/meteor/client/views/admin/import/ImportOperationSummary.tsx`:
- Around line 105-111: In ImportOperationSummary.tsx replace the conditional
inside TableCell to only check valid === false for failure (i.e. render
t('importer_status_import_failed') when valid === false, otherwise use
t(status.replace(...))), remove the CSV-specific guard (type === 'CSV' &&
contentType !== 'text/csv'); also drop the now-unused contentType prop from this
component's props and callers and update any related typings to simplify the
component interface. Ensure you reference the status, valid, type and
contentType symbols when making these edits so you remove all duplicated CSV
content-type logic.
- Line 24: The CSV validation condition in ImportOperationSummary (check
involving variables valid, type, and contentType) treats undefined contentType
as a mismatch and marks CSV imports as Error; update the boolean expression used
to compute the status so it only enforces the CSV MIME check when contentType
exists (e.g., replace the subcondition (type === 'CSV' && contentType !==
'text/csv') with a guarded check such as (type === 'CSV' && contentType != null
&& contentType !== 'text/csv') or (type === 'CSV' && contentType && contentType
!== 'text/csv')), ensuring imports with undefined contentType are not
incorrectly flagged.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 2 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="apps/meteor/app/importer/server/methods/getImportFileData.ts">
<violation number="1" location="apps/meteor/app/importer/server/methods/getImportFileData.ts:39">
P1: The strict `text/csv` MIME type check will incorrectly reject legitimate CSV uploads. Browsers report CSV file MIME types inconsistently across operating systems (e.g., Windows often reports `text/plain` or `application/vnd.ms-excel`). Consider accepting multiple CSV-compatible MIME types.</violation>
</file>
<file name="apps/meteor/client/views/admin/import/ImportOperationSummary.tsx">
<violation number="1" location="apps/meteor/client/views/admin/import/ImportOperationSummary.tsx:107">
P2: Optional contentType is compared strictly to 'text/csv'; when contentType is undefined (legacy records), CSV imports will be marked as failed regardless of actual status, causing regression in status display.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
apps/meteor/client/views/admin/import/ImportOperationSummary.tsx
Outdated
Show resolved
Hide resolved
|
|
Lakshyalamba
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixed a bug where the workspace import status remained "File Loaded" even if the file validation failed (e.g., uploading a PDF to a CSV importer).
Description
executeGetImportFileDatainapps/meteor/app/importer/server/methods/getImportFileData.tsto explicitly update the database status toimporter_import_failedand setvalid: falsewhen an invalid file type is detected.ImportOperationSummary.tsxto handle thevalid: falsestate and show the correct error translation.Steps to test
Screenshots
Summary by CodeRabbit
Bug Fixes
Documentation