fix(import): prevent infinite calls to /getImportFileData#38573
fix(import): prevent infinite calls to /getImportFileData#38573PurnenduMIshra129th wants to merge 9 commits into
Conversation
…that it will not loop in a infinte call
|
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 |
|
WalkthroughwaitFor in the import preparation page now accepts a new maxRetries parameter (default 60) and tracks attempts; it retries the predicate up to maxRetries times with 1s delays and rejects with a timeout error if the limit is reached. Changes
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. No actionable comments were generated in the recent review. 🎉 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✏️ Tip: You can disable this entire section by setting 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 |
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 27-41: The current waitFor implementation uses a hardcoded
maxRetries = 5 which is too low and causes legitimate long-running imports to
error; update waitFor to accept a configurable maxRetries parameter (or
timeout/ms interval) and replace the hardcoded maxRetries reference so callers
can pass different thresholds from getImportFileData and
getCurrentImportOperation (e.g., 60–120 attempts), and keep the default high
enough to avoid premature failures; also ensure the reject message still
includes the attempted limit and that call sites (getImportFileData,
getCurrentImportOperation) are updated to pass appropriate retry values.
🧹 Nitpick comments (1)
apps/meteor/client/views/admin/import/PrepareImportPage.tsx (1)
86-108: The error surfaced to the user on retry exhaustion is generic — consider a more specific message for the invalid-file case.When
waitForrejects after 5 attempts, the catch block at line 105 passes the rawError("waitFor: predicate not satisfied after 5 attempts")tohandleError. This internal message will likely be confusing to end users. A message like "The uploaded file could not be processed. Please ensure the file format is valid." would be far more actionable.Proposed improvement
} catch (error) { - handleError(error, t('Failed_To_Load_Import_Data')); + handleError(t('Invalid_import_file_type_or_processing_timeout')); router.navigate('/admin/import'); }(You'd need a corresponding translation key, or use an existing one that fits.)
📜 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 (5)
📓 Common learnings
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
📚 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: 2026-02-04T12:09:05.769Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38374
File: apps/meteor/tests/end-to-end/apps/app-logs-nested-requests.ts:26-37
Timestamp: 2026-02-04T12:09:05.769Z
Learning: In E2E tests at apps/meteor/tests/end-to-end/apps/, prefer sleeping for a fixed duration (e.g., 1 second) over implementing polling/retry logic when waiting for asynchronous operations to complete. Tests should fail deterministically if the expected result isn't available after the sleep.
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.
|
Hi!
How to fix: You can change the title by clicking the "Edit" button at the top of this page (to the right of the current title or in the checks in the commit rightside). Follow the Conventional Commits format.
How to fix: Try clicking the 'recheck' link in the CLAassistant comment. You can also go to the checks list at the bottom of the page, click on "license/cla", and ensure everything is authorized. Once these are green, we can look into the linting and logic issues! |
…et.Chat into infiniteApiCall
…tion use cases based on suggestion
…9th/Rocket.Chat into infiniteApiCall
|
hi @TheRazorbill can u tell how this pr will be merged , should i have to ping in some group , why my pr only 3 successfull checks and tests done flow is not running ? what am i doing wrong ? |
|
The "waiting for status" is normal; a maintainer needs to manually approve the workflow for security. For more visibility, you can share your PR link in the #landing-the-first-merged-pr channel on the open server. Some maintainers look there! Note: No need to tag people. Contribution guides and issue tagging are currently being improved by the team, so it might take a bit of time, but work is in progress. |
|
thanks for your support to clearing the points |
|
Kindly please assign this issue to me please I want work with it |
Proposed changes (including videos or screenshots)
This pull request fixes a problem in the import workflow.
Currently, if a user uploads the wrong file type (for example, a PDF instead of a Slack users CSV), the system blocks the file but still keeps calling the getImportFileData API repeatedly. The API response stays as {"waiting": true, "success": true}, which causes endless requests in the browser network tab.
With this change:
Issue(s)
#38499 (comment)
Steps to test or reproduce
Already a issue is create - #38499 (comment)
Further comments
Alternatives also considered as prevent the file in client side directly so that it won't able to call the api , first logic fail is better than after which causes this issue . Example - if user is selecting a specifc file but upload other files then he should not be able to hit the api , we will break it on client.
Summary by CodeRabbit