fix: Prevent endless polling on invalid file upload in Slack Users CSV importer#38548
fix: Prevent endless polling on invalid file upload in Slack Users CSV importer#38548KumarHarshit3603 wants to merge 9 commits into
Conversation
|
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 |
|
|
|
|
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:
WalkthroughAdds client-side file-type validation to block wrong uploads, wraps server CSV parsing in a try-catch to set progress to ERROR on exceptions, and bounds the poller with a capped retry mechanism to prevent infinite getImportFileData calls. Changes
Sequence DiagramsequenceDiagram
participant Client as NewImportPage
participant Server as getImportFileData
participant Parser as SlackUsersImporter
participant Poller as PrepareImportPage
rect rgba(200,150,100,0.5)
Note over Client: File selection & validation
Client->>Client: validateFile(file, importerKey)
alt validation fails
Client->>Client: setFileValidationError()\nclear files\ndisable Import button
else validation passes
Client->>Server: upload/submit file
end
end
rect rgba(100,150,200,0.5)
Note over Server,Parser: Server-side parsing with error handling
Server->>Parser: start CSV parsing
alt parse error
Parser->>Parser: catch(error)
Parser->>Server: updateProgress(ERROR)
Parser->>Server: return progress(ERROR)
else parse success
Parser->>Server: return progress(success)
end
end
rect rgba(150,100,200,0.5)
Note over Poller,Server: Polling with retry limit
Poller->>Server: getImportFileData()
alt response indicates ERROR
Server->>Poller: throw Meteor.Error("error-import-file-format-invalid")
Poller->>Poller: handle error / stop polling
else response not final
Poller->>Poller: increment retryCount\nif retryCount >= maxRetries -> reject
Poller->>Server: wait 1s and retry
else final progress
Poller->>Poller: resolve()
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 4 files
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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/meteor/client/views/admin/import/NewImportPage.tsx`:
- Line 252: isImportDisabled can become a string because fileValidationError may
be a non-empty string and the || chain short-circuits; change the expression so
it always yields a boolean by coercing fileValidationError (or the whole
sub-expression) to a boolean. Update the isImportDisabled assignment
(referencing isImportDisabled, isLoading, importer, fileType,
fileValidationError, files) to use explicit boolean coercion, e.g. use
!!fileValidationError or Boolean(...) around the upload-specific checks so
disabled always receives a true/false value.
- Around line 140-144: The current error message in NewImportPage mixes a
translated string with hardcoded English (t('Invalid_Import_File_Type') + ':
Expected ' + allowedTypes.join(', ')); replace this with a single parameterized
translation key (e.g., "Invalid_Import_File_Type_Expected") and pass the allowed
types as a placeholder when calling t; update the call in the invalidFiles
branch that uses getAllowedFileTypes(importerKey) and
setFileValidationError(...) to call t('Invalid_Import_File_Type_Expected', {
types: allowedTypes.join(', ') }), and add the new translation key ("Invalid
Import file type: Expected {{types}}") to the project translation files for each
locale.
🧹 Nitpick comments (6)
apps/meteor/app/importer/server/methods/getImportFileData.ts (1)
53-56: Remove the code comment.As per coding guidelines, code comments should be avoided in the implementation.
Proposed fix
if (readySteps.indexOf(instance.progress.step) >= 0) { - // If import failed/errored, throw error instead of trying to build selection if (instance.progress.step === ProgressStep.ERROR) { throw new Meteor.Error('error-import-file-format-invalid', 'Failed to process import file. Please check the file format and try again.', 'getImportFileData'); }As per coding guidelines:
**/*.{ts,tsx,js}: Avoid code comments in the implementation.apps/meteor/client/views/admin/import/PrepareImportPage.tsx (3)
27-31: Remove code comments from implementation.Lines 28, 39-40, and 51 contain explanatory comments. Per coding guidelines, these should be removed.
As per coding guidelines:
**/*.{ts,tsx,js}: Avoid code comments in the implementation.
34-43: Stall detection condition is largely redundant withmaxRetries.Both
maxRetries(300 × 1s = 5 min) and the stall check (2 min + 120 identical responses ≈ 2 min) serve as upper bounds. The stall check will always fire beforemaxRetriesif the response is stuck, makingmaxRetriesonly relevant if responses keep changing without reaching the predicate. This is fine as defense-in-depth, but worth noting for maintainability.
52-52:JSON.stringifycomparison on every poll iteration.This works for the small response objects returned by
getImportFileDatabut is worth noting as a potential concern if response payloads grow. A shallow key comparison or status-field check would be more efficient and explicit.apps/meteor/client/views/admin/import/NewImportPage.tsx (2)
91-107: ExtractgetAllowedFileTypesoutside the component.This pure function doesn't depend on component state or props (it receives
importerKeyParamas argument). Defining it inside the component recreates it on every render. Move it to module scope.Proposed fix
+const getAllowedFileTypes = (importerKeyParam: string | undefined): string[] => { + if (!importerKeyParam) return []; + if (importerKeyParam === 'csv' || importerKeyParam === 'slack-users') return ['text/csv', '.csv', 'text/plain']; + if (importerKeyParam === 'slack' || importerKeyParam === 'hipchat') return ['application/zip', '.zip']; + return []; +}; + function NewImportPage() {
326-330: Remove code comments from implementation.Multiple lines in this file contain explanatory comments (lines 90, 96, 101, 116, 137). Per coding guidelines, these should be removed.
As per coding guidelines:
**/*.{ts,tsx,js}: Avoid code comments in the implementation.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/meteor/app/importer-slack-users/server/SlackUsersImporter.tsapps/meteor/app/importer/server/methods/getImportFileData.tsapps/meteor/client/views/admin/import/NewImportPage.tsxapps/meteor/client/views/admin/import/PrepareImportPage.tsx
🧰 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.tsapps/meteor/app/importer-slack-users/server/SlackUsersImporter.tsapps/meteor/client/views/admin/import/PrepareImportPage.tsxapps/meteor/client/views/admin/import/NewImportPage.tsx
🧠 Learnings (5)
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `page.waitFor()` with specific conditions instead of hardcoded timeouts in Playwright tests
Applied to files:
apps/meteor/client/views/admin/import/PrepareImportPage.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Implement proper wait strategies for dynamic content in Playwright tests
Applied to files:
apps/meteor/client/views/admin/import/PrepareImportPage.tsx
📚 Learning: 2025-12-16T17:29:45.163Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37834
File: apps/meteor/tests/e2e/page-objects/fragments/admin-flextab-emoji.ts:12-22
Timestamp: 2025-12-16T17:29:45.163Z
Learning: In page object files under `apps/meteor/tests/e2e/page-objects/`, always import `expect` from `../../utils/test` (Playwright's async expect), not from Jest. Jest's `expect` has a synchronous signature and will cause TypeScript errors when used with web-first assertions like `toBeVisible()`.
Applied to files:
apps/meteor/client/views/admin/import/PrepareImportPage.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
apps/meteor/client/views/admin/import/PrepareImportPage.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/client/views/admin/import/PrepareImportPage.tsx
⏰ 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 comments (2)
apps/meteor/app/importer-slack-users/server/SlackUsersImporter.ts (1)
44-110: LGTM — try-catch properly prevents unhandled parse errors.The error handling correctly catches CSV parse failures, logs the error, and transitions to the ERROR state, which aligns with the server-side changes in
getImportFileData.ts.apps/meteor/client/views/admin/import/NewImportPage.tsx (1)
91-107: Hardcoded importer-to-file-type mapping requires manual maintenance.The mapping at lines 91-107 is currently necessary since server-side importer definitions do not expose file type information. Neither the
ImporterInfotype nor the/v1/importers.listendpoint includes MIME type or file extension metadata. If new importers are added or existing ones change their expected file formats, this mapping must be updated manually. Consider extending theImporterInfotype to include an optionalallowedFileTypesfield that importers can declare, which could then be returned by/v1/importers.listto eliminate the need for client-side hardcoding.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/meteor/client/views/admin/import/PrepareImportPage.tsx`:
- Around line 28-42: The stall-detection branch in callPromise is unreachable
because retryCount (checked against maxRetries = 60) will always hit before
sameResponseCount (>120); remove the dead stall block that checks Date.now() -
lastProgressTime and sameResponseCount, or if stall detection is desired, adjust
thresholds so they’re consistent (e.g., set maxRetries > 120 or reduce the
sameResponseCount threshold under maxRetries); update the logic around
maxRetries, sameResponseCount, lastProgressTime and callPromise to use the
chosen approach and ensure retryCount and sameResponseCount are
incremented/checked consistently.
🧹 Nitpick comments (1)
apps/meteor/client/views/admin/import/PrepareImportPage.tsx (1)
50-51: Remove the code comment.As per coding guidelines, avoid code comments in TypeScript/JavaScript implementation files.
- // Track if we're making progress or stuck in loop - if (lastResponse && JSON.stringify(result) === JSON.stringify(lastResponse)) { + if (lastResponse && JSON.stringify(result) === JSON.stringify(lastResponse)) {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/meteor/client/views/admin/import/PrepareImportPage.tsx
🧰 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/client/views/admin/import/PrepareImportPage.tsx
🧠 Learnings (4)
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `page.waitFor()` with specific conditions instead of hardcoded timeouts in Playwright tests
Applied to files:
apps/meteor/client/views/admin/import/PrepareImportPage.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Implement proper wait strategies for dynamic content in Playwright tests
Applied to files:
apps/meteor/client/views/admin/import/PrepareImportPage.tsx
📚 Learning: 2025-12-16T17:29:45.163Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37834
File: apps/meteor/tests/e2e/page-objects/fragments/admin-flextab-emoji.ts:12-22
Timestamp: 2025-12-16T17:29:45.163Z
Learning: In page object files under `apps/meteor/tests/e2e/page-objects/`, always import `expect` from `../../utils/test` (Playwright's async expect), not from Jest. Jest's `expect` has a synchronous signature and will cause TypeScript errors when used with web-first assertions like `toBeVisible()`.
Applied to files:
apps/meteor/client/views/admin/import/PrepareImportPage.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
apps/meteor/client/views/admin/import/PrepareImportPage.tsx
⏰ 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 comments (1)
apps/meteor/client/views/admin/import/PrepareImportPage.tsx (1)
104-178: Effect logic for error-state detection looks correct.The
loadCurrentOperationfunction properly checksImportingErrorStates(line 158) and navigates back, and thewaitForrejection inloadImportFileData(line 124) is caught and handled. With the backend changes (throwingMeteor.Errorwhen progress isERROR), thewaitForcall will reject quickly, which is the desired behavior for invalid uploads.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
KumarHarshit3603
force-pushed
the
api-fix
branch
from
fe22f54 to
35f6d65
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/meteor/client/views/admin/import/NewImportPage.tsx`:
- Line 88: The fileValidationError state (declared via useState as
fileValidationError / setFileValidationError) must be cleared when the importer
changes; update handleImporterKeyChange (or add a useEffect watching
importerKey) to call setFileValidationError('') whenever the selected importer
key changes so stale validation errors are removed when switching importers.
🧹 Nitpick comments (4)
apps/meteor/app/importer/server/methods/getImportFileData.ts (1)
53-56: Remove the implementation comment.As per coding guidelines,
**/*.{ts,tsx,js}files should avoid code comments in the implementation. The intent is already clear from the code structure.Proposed fix
if (readySteps.indexOf(instance.progress.step) >= 0) { - // If import failed/errored, throw error instead of trying to build selection if (instance.progress.step === ProgressStep.ERROR) { throw new Meteor.Error('error-import-file-format-invalid', 'Failed to process import file. Please check the file format and try again.', 'getImportFileData'); } return instance.buildSelection(); }apps/meteor/client/views/admin/import/PrepareImportPage.tsx (1)
50-56: Remove implementation comments.Lines 50 has a comment that violates the coding guideline for
**/*.{ts,tsx,js}files. As per coding guidelines, avoid code comments in the implementation.Proposed fix
- // Track if we're making progress or stuck in loop if (lastResponse && JSON.stringify(result) === JSON.stringify(lastResponse)) {apps/meteor/client/views/admin/import/NewImportPage.tsx (2)
90-107: Remove implementation comments and extract helpers outside the component.Multiple comments in these functions violate the coding guideline for
**/*.{ts,tsx,js}files. Also,getAllowedFileTypesandvalidateFileare pure functions that don't depend on component state — they should be extracted outside the component to avoid recreation on every render.Proposed refactor
Move these before the component definition:
+const allowedFileTypesByImporter: Record<string, string[]> = { + csv: ['text/csv', '.csv', 'text/plain'], + 'slack-users': ['text/csv', '.csv', 'text/plain'], + slack: ['application/zip', '.zip'], + hipchat: ['application/zip', '.zip'], +}; + +const getAllowedFileTypes = (importerKeyParam: string | undefined): string[] => + (importerKeyParam && allowedFileTypesByImporter[importerKeyParam]) || []; + +const validateFile = (file: File, importerKeyParam: string | undefined): boolean => { + const allowedTypes = getAllowedFileTypes(importerKeyParam); + if (allowedTypes.length === 0) return true; + const fileExtension = `.${file.name.split('.').pop()?.toLowerCase()}`; + return allowedTypes.includes(file.type) || allowedTypes.includes(fileExtension); +}; + function NewImportPage() {And remove lines 91–122 from inside the component.
135-148: Remove implementation comments.Line 137 (
// Validate all files) violates the coding guideline for**/*.{ts,tsx,js}files. As per coding guidelines, avoid code comments in the implementation.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/meteor/app/importer-slack-users/server/SlackUsersImporter.tsapps/meteor/app/importer/server/methods/getImportFileData.tsapps/meteor/client/views/admin/import/NewImportPage.tsxapps/meteor/client/views/admin/import/PrepareImportPage.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/app/importer-slack-users/server/SlackUsersImporter.ts
🧰 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.tsapps/meteor/client/views/admin/import/NewImportPage.tsxapps/meteor/client/views/admin/import/PrepareImportPage.tsx
🧠 Learnings (7)
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
apps/meteor/client/views/admin/import/NewImportPage.tsx
📚 Learning: 2025-11-19T12:32:29.696Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37547
File: packages/i18n/src/locales/en.i18n.json:634-634
Timestamp: 2025-11-19T12:32:29.696Z
Learning: Repo: RocketChat/Rocket.Chat
Context: i18n workflow
Learning: In this repository, new translation keys should be added to packages/i18n/src/locales/en.i18n.json only; other locale files are populated via the external translation pipeline and/or fall back to English. Do not request adding the same key to all locale files in future reviews.
Applied to files:
apps/meteor/client/views/admin/import/NewImportPage.tsx
📚 Learning: 2025-10-06T20:32:23.658Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/utilities.ts:557-573
Timestamp: 2025-10-06T20:32:23.658Z
Learning: In packages/apps-engine/tests/test-data/utilities.ts, the field name `isSubscripbedViaBundle` in the `IMarketplaceSubscriptionInfo` type should not be flagged as a typo, as it may match the upstream API's field name.
Applied to files:
apps/meteor/client/views/admin/import/NewImportPage.tsx
📚 Learning: 2025-11-19T18:20:07.720Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: packages/i18n/src/locales/en.i18n.json:918-921
Timestamp: 2025-11-19T18:20:07.720Z
Learning: Repo: RocketChat/Rocket.Chat — i18n/formatting
Learning: This repository uses a custom message formatting parser in UI blocks/messages; do not assume standard Markdown rules. For keys like Call_ended_bold, Call_not_answered_bold, Call_failed_bold, and Call_transferred_bold in packages/i18n/src/locales/en.i18n.json, retain the existing single-asterisk emphasis unless maintainers request otherwise.
Applied to files:
apps/meteor/client/views/admin/import/NewImportPage.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `page.waitFor()` with specific conditions instead of hardcoded timeouts in Playwright tests
Applied to files:
apps/meteor/client/views/admin/import/PrepareImportPage.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Implement proper wait strategies for dynamic content in Playwright tests
Applied to files:
apps/meteor/client/views/admin/import/PrepareImportPage.tsx
📚 Learning: 2025-12-16T17:29:45.163Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37834
File: apps/meteor/tests/e2e/page-objects/fragments/admin-flextab-emoji.ts:12-22
Timestamp: 2025-12-16T17:29:45.163Z
Learning: In page object files under `apps/meteor/tests/e2e/page-objects/`, always import `expect` from `../../utils/test` (Playwright's async expect), not from Jest. Jest's `expect` has a synchronous signature and will cause TypeScript errors when used with web-first assertions like `toBeVisible()`.
Applied to files:
apps/meteor/client/views/admin/import/PrepareImportPage.tsx
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
KumarHarshit3603
force-pushed
the
api-fix
branch
from
35f6d65 to
734a5f8
Compare
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
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/client/views/admin/import/NewImportPage.tsx">
<violation number="1" location="apps/meteor/client/views/admin/import/NewImportPage.tsx:147">
P3: Hardcoded English concatenation in a user-facing error message breaks i18n and will show untranslated text for non-English locales.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| if (invalidFiles.length > 0) { | ||
| const allowedTypes = getAllowedFileTypes(importerKey); | ||
| setFileValidationError(t('Invalid_Import_File_Type') + ': Expected ' + allowedTypes.join(', ')); |
There was a problem hiding this comment.
P3: Hardcoded English concatenation in a user-facing error message breaks i18n and will show untranslated text for non-English locales.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/client/views/admin/import/NewImportPage.tsx, line 147:
<comment>Hardcoded English concatenation in a user-facing error message breaks i18n and will show untranslated text for non-English locales.</comment>
<file context>
@@ -144,7 +144,7 @@ function NewImportPage() {
if (invalidFiles.length > 0) {
const allowedTypes = getAllowedFileTypes(importerKey);
- setFileValidationError(t('Invalid_Import_File_Type_Expected', { types: allowedTypes.join(', ') }));
+ setFileValidationError(t('Invalid_Import_File_Type') + ': Expected ' + allowedTypes.join(', '));
setFiles([]);
return;
</file context>
2 participants
fix: Prevent endless polling on invalid file upload in Slack Users CSV importer
Summary(proposed changes)
This PR fixes an infinite polling loop that occurs when a user uploads an invalid file while trying to import a Slack Users CSV.
Before this change:
ERRORgetImportFileDataevery ~1s forever{"waiting":true,"success":true}responsesAfter this change:
ProgressStep.ERRORimmediately on parsing failuregetImportFileDataendpoint early-exits and throws when status is already ERRORResult:
No more infinite network requests for invalid files
Legitimate large CSV files still have enough time to process (30–60s timeout preserved)
Issue(s)
Closes #38499
Steps to test or reproduce
getImportFileDataCompare with a valid CSV file:
Further comments
Backend changes summary
SlackUsersImporter.tstry/catchProgressStep.ERRORimmediately + log errorgetImportFileData.tsERROR{"waiting":true}responses foreverFrontend changes summary
PrepareImportPage.tsxThe fix maintains good UX for valid imports while cleanly failing fast on invalid ones.
Thanks for reviewing!
Summary by CodeRabbit