fix(ui): prevent app crash when import type is an object#41570
fix(ui): prevent app crash when import type is an object#41570SrijanGupta2006 wants to merge 2 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 |
|
WalkthroughImport operation summaries now safely render non-string ChangesImport summary rendering
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@apps/meteor/client/views/admin/import/ImportOperationSummary.tsx`:
- Line 109: Normalize total and all sibling counter fields before they reach the
ImportOperationSummary render, resolving pending values and converting
object-wrapped counters into usable numeric or string values. Update the data
preparation branches feeding ImportOperationSummary rather than relying on the
TableCell’s JSON.stringify fallback, while preserving the existing displayed
counter values.
🪄 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 Plus
Run ID: 119aed53-57e4-47d0-89dd-78bfda277c66
📒 Files selected for processing (1)
apps/meteor/client/views/admin/import/ImportOperationSummary.tsx
📜 Review details
🧰 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/ImportOperationSummary.tsx
🧠 Learnings (2)
📚 Learning: 2026-03-27T14:52:56.865Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39892
File: apps/meteor/client/views/room/contextualBar/Threads/Thread.tsx:150-155
Timestamp: 2026-03-27T14:52:56.865Z
Learning: In Rocket.Chat, there are two different `ModalBackdrop` components with different prop APIs. During review, confirm the import source: (1) `rocket.chat/fuselage` `ModalBackdrop` uses `ModalBackdropProps` based on `BoxProps` (so it supports `onClick` and other Box/DOM props) and does not have an `onDismiss` prop; (2) `rocket.chat/ui-client` `ModalBackdrop` uses a narrower props interface like `{ children?: ReactNode; onDismiss?: () => void }` and handles Escape keypress and outside mouse-up, and it does not forward arbitrary DOM props such as `onClick`. Flag mismatched props (e.g., `onDismiss` passed to the fuselage component or `onClick` passed to the ui-client component) and ensure the usage matches the correct component being imported.
Applied to files:
apps/meteor/client/views/admin/import/ImportOperationSummary.tsx
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
apps/meteor/client/views/admin/import/ImportOperationSummary.tsx
🔇 Additional comments (2)
apps/meteor/client/views/admin/import/ImportOperationSummary.tsx (2)
90-98: LGTM!
99-99: 🗄️ Data Integrity & IntegrationNo change needed.
IImportdefinestypeasstringandcount.totalasnumber;ImportOperationSummaryPropsmatches that contract.> Likely an incorrect or invalid review comment.
| <TableCell align='center'>{channels}</TableCell> | ||
| <TableCell align='center'>{messages}</TableCell> | ||
| <TableCell align='center'>{total}</TableCell> | ||
| <TableCell align='center'>{typeof total === 'object'?JSON.stringify(total):total}</TableCell> |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate file:"
fd -a 'ImportOperationSummary\.tsx$' . || true
file="$(fd 'ImportOperationSummary\.tsx$' . | head -n1 || true)"
if [ -n "${file:-}" ]; then
echo "File: $file"
wc -l "$file"
echo "--- outline ---"
ast-grep outline "$file" || true
echo "--- relevant lines 1-160 ---"
sed -n '1,160p' "$file" | cat -n
fi
echo "--- Search for total usage / import summary producers ---"
rg -n "total|ImportOperationSummary|JSON\.stringify\(total\)|Promise\.resolve|0\[object Promise\]" . -g '*.{ts,tsx,js}' -SRepository: RocketChat/Rocket.Chat
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Behavioral probes for current rendering candidates"
node - <<'JS'
const values = [1, "ok", Promise.resolve(1), Promise.resolve(null), null, undefined, 0n];
for (const total of values) {
try {
const child = typeof total === 'object' ? JSON.stringify(total) : total;
console.log(JSON.stringify({
input: total === undefined ? 'undefined' : total === null ? 'null' : typeof total,
json: child === undefined ? 'undefined' : child === null ? 'null' : child
}));
} catch (e) {
console.log(JSON.stringify({ input: typeof total, error: e.toString() }));
}
}
JSRepository: RocketChat/Rocket.Chat
Length of output: 482
🏁 Script executed:
#!/bin/bash
set -euo pipefail
target="$(fd 'ImportOperationSummary\.tsx$' . | head -n1 || true)"
if [ -z "${target:-}" ]; then
echo "Target file not found"
exit 0
fi
echo "Target: $target"
wc -l "$target"
echo "--- relevant lines 1-160 ---"
sed -n '1,160p' "$target" | cat -n
echo "--- focused imports/usages around ImportOperationSummary ---"
rg -n "ImportOperationSummary|import.*ImportOperation|Total|import.*Total|importTotal|total.*Promise|Promise.*total|0\\[object Promise\\]" apps/meteor/client apps/meteor/server -S \
--iglob '*.{ts,tsx,js}' --max-count 220
echo "--- list candidate files under import summaries ---"
fd -i 'import|Import' apps/meteor/client apps/meteor/server -e ts -e tsx -e js -e jsp | sed -n '1,120p'
echo "--- git diff stat/name-only ---"
git diff --stat || true
git diff --name-only || trueRepository: RocketChat/Rocket.Chat
Length of output: 40211
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- GetLatestImportOperations implementation ---"
method_file="$(fd 'getLatestImportOperations\.ts$' apps/meteor/server/meteor-methods/import | head -n1 || true)"
if [ -n "${method_file:-}" ]; then
wc -l "$method_file"
sed -n '1,220p' "$method_file" | cat -n
fi
echo "--- ImportHistoryPage usage of currentOperation.data ---"
sed -n '140,200p' apps/meteor/client/views/admin/import/ImportHistoryPage.tsx | cat -n
echo "--- import data type definitions for progress/count data ---"
rg -n "IImportOperation|ImportOperation|addCountToTotal|progress\\.count\\.total|progress\\.count|increaseTotalCount|totalCount" apps/meteor/server/lib/import apps/meteor/server/services/import apps/meteor/server/meteor-methods/import app/importer apps/meteor -g '*.{ts,tsx,js}' -S \
| sed -n '1,240p'Repository: RocketChat/Rocket.Chat
Length of output: 34715
Render ImportOperationSummary with normalized counters.
The current rendering path passes raw operation data into the summary through several branches, so JSON.stringify(total) can hide unresolved values as {} or show values like 0[object Promise] unchanged. Ensure total (and the sibling counter fields) is normalized to a numeric/string value before rendering.
🤖 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 `@apps/meteor/client/views/admin/import/ImportOperationSummary.tsx` at line
109, Normalize total and all sibling counter fields before they reach the
ImportOperationSummary render, resolving pending values and converting
object-wrapped counters into usable numeric or string values. Update the data
preparation branches feeding ImportOperationSummary rather than relying on the
TableCell’s JSON.stringify fallback, while preserving the existing displayed
counter values.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
1 participant
Proposed changes (including videos or screenshots)
Added a safeguard in the ImportOperationSummary component to check the data type of the type prop before rendering it in the table cell. If it receives an object, it now uses JSON.stringify() to safely display it, preventing the application from crashing with an "Objects are not valid as a React child" error on narrow viewports.
BEFORE: The application crashes completely when the component attemtps to render an object.
AFTER: The UI remains stable, and the object is safely stringified in the table cell.
Issue(s)
Fixes #29509
Steps to test or reproduce
apps/meteor/client/views/admin/import/ImportOperationSummary.tsx.totalortypeprops at the top of the component (e.g.,total = { _isPromise: true } as any;).totalcolumn renders.Objects are not valid as a React childerror. With this PR, the object is safely stringified and the UI remains stable.Further comments
Issue #29509 indicates that an unresolved Promise/object is occasionally being passed from the backend to the
totalfield during the "Download Pending Avatars" phase of a Slack import. While the root cause likely lies in the backend importer logic, this PR adds a defensive frontend safeguard. By checking iftypeortotalare objects and safely stringifying them, we prevent a complete React Error Boundary crash, ensuring the rest of the Admin UI remains accessible to the user.Summary by CodeRabbit