fix: update import status to error on invalid file type validation#38522
Open
Lakshyalamba wants to merge 7 commits into
Open
fix: update import status to error on invalid file type validation#38522Lakshyalamba wants to merge 7 commits into
Lakshyalamba wants to merge 7 commits into
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: 5644018 The changes in this PR will be included in the next version bump. This PR includes changesets to release 41 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
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughServer-side import validation now flags CSV uploads with disallowed content types as invalid, updates the import operation in the DB to 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 isInvalidCSV (importerKey == "csv" && contentType ∈ VALID_CSV_CONTENT_TYPES)
Server-->>Client: return waiting / continue
else invalid OR isInvalidCSV
Server->>DB: update import operation { status: "ERROR", valid: false }
Server-->>Client: throw Meteor.Error / return failure
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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.
|
|
Lakshyalamba
changed the title
Lakshyalamba
force-pushed
the
fix/import-status-validation
branch
from
e14313e to
cf4d745
Compare
Author
|
Hi @RocketChat/frontend! Since I'm an external contributor, could someone please approve the CI workflows to run? Also, could you help add the required stat: QA assured label and milestone so this can be reviewed? Thanks! |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/meteor/app/importer/server/methods/getImportFileData.ts`:
- Around line 49-52: The isInvalidCSV check treats a missing contentType as an
explicit empty string and marks CSV uploads invalid; change it so absence of
contentType is NOT considered invalid. Update the isInvalidCSV logic in
getImportFileData.ts to only evaluate VALID_CSV_CONTENT_TYPES when
operation.contentType is present (e.g., check operation.contentType != null or
truthy before calling includes), keeping operation.importerKey === 'csv' part
intact so a missing contentType will be treated as unknown and not force ERROR.
- Around line 16-22: The helper updateImportOperationStatus currently writes
straight through with Imports.updateOne which doesn't refresh the running
Importer instance's cached importRecord (importRecord.valid) and can let an
in-progress importer overwrite ERROR; instead update the record via the same
path that refreshes importer cache—call the Importer instance's updateRecord (or
the Meteor collection method that triggers observers, e.g., Imports.update with
the operationId) so Importer.updateRecord is invoked and importRecord.valid is
refreshed; replace the direct Imports.updateOne call in
updateImportOperationStatus with the collection update that triggers
Importer.updateRecord or invoke Importer.updateRecord for the given operationId.
🪄 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: CHILL
Plan: Pro
Run ID: e4e4e404-272e-463a-8bca-3d8d4f85c5f3
📒 Files selected for processing (1)
apps/meteor/app/importer/server/methods/getImportFileData.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/importer/server/methods/getImportFileData.ts
🧠 Learnings (10)
📓 Common learnings
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:710-757
Timestamp: 2026-03-15T14:31:28.969Z
Learning: In RocketChat/Rocket.Chat, the `UserCreateParamsPOST` type in `apps/meteor/app/api/server/v1/users.ts` (migrated from `packages/rest-typings/src/v1/users/UserCreateParamsPOST.ts`) intentionally has `fields: string` (non-optional) and `settings?: IUserSettings` without a corresponding AJV schema entry. This is a pre-existing divergence carried over verbatim from the original rest-typings source (PR `#39647`). Do not flag this type/schema misalignment during the OpenAPI migration review — it is tracked as a separate follow-up fix.
📚 Learning: 2026-03-15T14:31:28.969Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:710-757
Timestamp: 2026-03-15T14:31:28.969Z
Learning: In RocketChat/Rocket.Chat, the `UserCreateParamsPOST` type in `apps/meteor/app/api/server/v1/users.ts` (migrated from `packages/rest-typings/src/v1/users/UserCreateParamsPOST.ts`) intentionally has `fields: string` (non-optional) and `settings?: IUserSettings` without a corresponding AJV schema entry. This is a pre-existing divergence carried over verbatim from the original rest-typings source (PR `#39647`). Do not flag this type/schema misalignment during the OpenAPI migration review — it is tracked as a separate follow-up fix.
Applied to files:
apps/meteor/app/importer/server/methods/getImportFileData.ts
📚 Learning: 2026-03-19T13:59:40.678Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38357
File: apps/meteor/app/apps/server/converters/uploads.ts:45-49
Timestamp: 2026-03-19T13:59:40.678Z
Learning: In `apps/meteor/app/apps/server/converters/uploads.ts`, the `room` async handler in `convertToApp` uses non-null assertions (`upload.rid!` and `result!`) intentionally. The data flow guarantees that any upload reaching this point must have a `rid`; if it does not, throwing an error is the desired behavior (fail-fast / data integrity guard). Do not flag these non-null assertions as unsafe during code review.
Applied to files:
apps/meteor/app/importer/server/methods/getImportFileData.ts
📚 Learning: 2026-02-12T15:39:28.416Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 32703
File: apps/meteor/client/lib/chats/flows/uploadFiles.ts:52-58
Timestamp: 2026-02-12T15:39:28.416Z
Learning: In `apps/meteor/client/lib/chats/flows/uploadFiles.ts`, when E2E encryption is required but not allowed (e.g., `E2E_Enable_Encrypt_Files` setting is disabled), the function intentionally abandons the entire upload queue and displays a toast error. This fail-fast behavior prevents partial uploads when encryption requirements cannot be met and is the expected behavior, not a bug.
Applied to files:
apps/meteor/app/importer/server/methods/getImportFileData.ts
📚 Learning: 2026-03-11T18:17:53.972Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39425
File: apps/meteor/client/lib/chats/flows/processMessageUploads.ts:112-119
Timestamp: 2026-03-11T18:17:53.972Z
Learning: In `apps/meteor/client/lib/chats/flows/processMessageUploads.ts`, when sending multiple file uploads, each file is confirmed via its own `/rooms.mediaConfirm/${rid}/${fileId}` call and produces a separate message. Only the first file's confirm payload carries the composed message text (`msg`); all subsequent files receive `msg: ''`. This one-message-per-file behavior is intentional by design — do not flag it as a bug or suggest batching into a single message.
Applied to files:
apps/meteor/app/importer/server/methods/getImportFileData.ts
📚 Learning: 2026-03-12T17:12:49.121Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39425
File: apps/meteor/client/lib/chats/flows/uploadFiles.ts:28-33
Timestamp: 2026-03-12T17:12:49.121Z
Learning: Rocket.Chat — apps/meteor/client/lib/chats/flows/uploadFiles.ts: When E2E_Enable_Encrypt_Files is disabled, plaintext file uploads are allowed in E2E rooms; this fallback is expected and should not be flagged as a security regression.
Applied to files:
apps/meteor/app/importer/server/methods/getImportFileData.ts
📚 Learning: 2026-03-11T18:15:53.272Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39425
File: apps/meteor/app/api/server/v1/rooms.ts:294-296
Timestamp: 2026-03-11T18:15:53.272Z
Learning: In Rocket.Chat's `rooms.mediaConfirm/:rid/:fileId` endpoint (apps/meteor/app/api/server/v1/rooms.ts), updating `file.name` from `bodyParams.fileName` without updating `file.path` is intentionally safe. The file path is keyed by `_id` (not by filename), so the stored path remains valid regardless of a rename. `file.name` only affects the display name in the message attachment; do not flag this as a path-divergence issue.
Applied to files:
apps/meteor/app/importer/server/methods/getImportFileData.ts
📚 Learning: 2026-02-24T19:36:55.089Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/page-objects/fragments/home-content.ts:60-82
Timestamp: 2026-02-24T19:36:55.089Z
Learning: In RocketChat/Rocket.Chat e2e tests (apps/meteor/tests/e2e/page-objects/fragments/home-content.ts), thread message preview listitems do not have aria-roledescription="message", so lastThreadMessagePreview locator cannot be scoped to messageListItems (which filters for aria-roledescription="message"). It should remain scoped to page.getByRole('listitem') or mainMessageList.getByRole('listitem').
Applied to files:
apps/meteor/app/importer/server/methods/getImportFileData.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/app/importer/server/methods/getImportFileData.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/app/importer/server/methods/getImportFileData.ts
🔇 Additional comments (1)
apps/meteor/app/importer/server/methods/getImportFileData.ts (1)
14-15: Good MIME allowlist expansion.This is much safer than the old single-value
text/csvcheck for browser-uploaded CSVs.
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