fix(import): restore message visibility after Slack/CSV import#39258
Open
CodeMatrix1 wants to merge 2 commits intoRocketChat:developfrom
Open
fix(import): restore message visibility after Slack/CSV import#39258CodeMatrix1 wants to merge 2 commits intoRocketChat:developfrom
CodeMatrix1 wants to merge 2 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 |
|
Contributor
WalkthroughThe importer converters are improved to properly handle message and room data during import operations. MessageConverter now conditionally includes optional fields to reduce undefined/null propagation, while RoomConverter adds post-creation caching of imported identifiers for room resolution. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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}
![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: 1
🧹 Nitpick comments (1)
apps/meteor/app/importer/server/classes/converters/RoomConverter.ts (1)
135-141: Centralize room-cache registration and remove the inline implementation comment.Line 135-141 works, but keeping this mapping only in
insertRoommakes the behavior harder to keep consistent with the update path. Moving it to a shared helper called frominsertOrUpdateRoomkeeps room ID/name cache registration in one place and removes the added implementation comment.♻️ Proposed refactor
async insertOrUpdateRoom(existingRoom: IRoom | null, data: IImportChannel, startedByUserId: string): Promise<void> { if (existingRoom) { await this.updateRoom(existingRoom, data, startedByUserId); } else { await this.insertRoom(data, startedByUserId); } + + this.registerImportedRoomIdsInCache(data); if (data.archived && data._id) { await this.archiveRoomById(data._id); } } + +private registerImportedRoomIdsInCache(roomData: IImportChannel): void { + for (const importId of roomData.importIds) { + this._cache.addRoom(importId, roomData._id as string); + if (roomData.name) { + this._cache.addRoomName(importId, roomData.name); + } + } +} @@ - // Register imported room ids in cache (required for message import) - for (const importId of roomData.importIds) { - this._cache.addRoom(importId, roomData._id as string); - if (roomData.name) { - this._cache.addRoomName(importId, roomData.name); - } - } - await this.updateRoomId(roomData._id as 'string', roomData); }As per coding guidelines "Avoid code comments in the implementation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/importer/server/classes/converters/RoomConverter.ts` around lines 135 - 141, Extract the loop that registers imported room ids into cache (calls to this._cache.addRoom and this._cache.addRoomName) into a new private helper method on RoomConverter (e.g., registerImportedRoomIds or similar) and call that helper from insertOrUpdateRoom (and insertRoom if it currently contains the same logic) so both insert and update paths share the same implementation; remove the inline implementation comment and ensure the helper accepts the roomData object (using roomData.importIds, roomData._id, roomData.name) and preserves existing behavior.
🤖 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/classes/converters/MessageConverter.ts`:
- Around line 105-125: The current object spread can still emit editedBy or
reactions as undefined because await this._cache.findImportedUser(data.editedBy)
or await this.convertMessageReactions(data.reactions) may return
undefined/empty; to fix, compute the values first (e.g., const importedEditedBy
= await this._cache.findImportedUser(data.editedBy); const convertedReactions =
data.reactions ? await this.convertMessageReactions(data.reactions) : undefined)
and only spread { editedBy: importedEditedBy } if importedEditedBy !==
undefined, and only spread { reactions: convertedReactions } if
convertedReactions !== undefined (and/or non-empty), so editedBy and reactions
are omitted instead of serialized as undefined.
---
Nitpick comments:
In `@apps/meteor/app/importer/server/classes/converters/RoomConverter.ts`:
- Around line 135-141: Extract the loop that registers imported room ids into
cache (calls to this._cache.addRoom and this._cache.addRoomName) into a new
private helper method on RoomConverter (e.g., registerImportedRoomIds or
similar) and call that helper from insertOrUpdateRoom (and insertRoom if it
currently contains the same logic) so both insert and update paths share the
same implementation; remove the inline implementation comment and ensure the
helper accepts the roomData object (using roomData.importIds, roomData._id,
roomData.name) and preserves existing behavior.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/meteor/app/importer/server/classes/converters/MessageConverter.tsapps/meteor/app/importer/server/classes/converters/RoomConverter.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/classes/converters/RoomConverter.tsapps/meteor/app/importer/server/classes/converters/MessageConverter.ts
🧠 Learnings (7)
📓 Common learnings
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings by mapping subscription documents to room IDs, never undefined, even when user has no room subscriptions.
Applied to files:
apps/meteor/app/importer/server/classes/converters/RoomConverter.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings (mapping subscription documents to room IDs), never undefined, even when user has no room subscriptions.
Applied to files:
apps/meteor/app/importer/server/classes/converters/RoomConverter.ts
📚 Learning: 2025-10-28T16:53:42.761Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Applied to files:
apps/meteor/app/importer/server/classes/converters/RoomConverter.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/classes/converters/RoomConverter.tsapps/meteor/app/importer/server/classes/converters/MessageConverter.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/classes/converters/RoomConverter.tsapps/meteor/app/importer/server/classes/converters/MessageConverter.ts
📚 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/app/importer/server/classes/converters/MessageConverter.ts
Comment on lines
+105
to
+125
| ...(data.editedAt ? { editedAt: data.editedAt } : {}), | ||
| ...(data.editedBy | ||
| ? { | ||
| editedBy: | ||
| (await this._cache.findImportedUser(data.editedBy)) || | ||
| undefined, | ||
| } | ||
| : {}), | ||
|
|
||
| ...(mentions?.length ? { mentions } : {}), | ||
| ...(channels?.length ? { channels } : {}), | ||
| ...(data._importFile ? { _importFile: data._importFile } : {}), | ||
| ...(data.url ? { url: data.url } : {}), | ||
| ...(data.attachments?.length ? { attachments: data.attachments } : {}), | ||
| ...(data.bot ? { bot: data.bot } : {}), | ||
| ...(data.emoji ? { emoji: data.emoji } : {}), | ||
| ...(data.alias ? { alias: data.alias } : {}), | ||
| ...(data._id ? { _id: data._id } : {}), | ||
| ...(data.reactions | ||
| ? { reactions: await this.convertMessageReactions(data.reactions) } | ||
| : {}), |
Contributor
There was a problem hiding this comment.
editedBy and reactions can still be emitted as undefined.
Line 105-125 conditionally spreads these keys, but the computed values can be undefined (findImportedUser miss or empty reaction map), so the fields may still be serialized instead of omitted.
🛠️ Proposed fix
protected async buildMessageObject(data: IImportMessage, rid: string, creator: UserIdentification): Promise<MessageObject> {
// Convert the mentions and channels first because these conversions can also modify the msg in the message object
const mentions = data.mentions && (await this.convertMessageMentions(data));
const channels = data.channels && (await this.convertMessageChannels(data));
+ const editedBy = data.editedBy ? await this._cache.findImportedUser(data.editedBy) : undefined;
+ const reactions = data.reactions ? await this.convertMessageReactions(data.reactions) : undefined;
return {
@@
- ...(data.editedBy
- ? {
- editedBy:
- (await this._cache.findImportedUser(data.editedBy)) ||
- undefined,
- }
- : {}),
+ ...(editedBy ? { editedBy } : {}),
@@
- ...(data.reactions
- ? { reactions: await this.convertMessageReactions(data.reactions) }
- : {}),
+ ...(reactions ? { reactions } : {}),
};
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/app/importer/server/classes/converters/MessageConverter.ts`
around lines 105 - 125, The current object spread can still emit editedBy or
reactions as undefined because await this._cache.findImportedUser(data.editedBy)
or await this.convertMessageReactions(data.reactions) may return
undefined/empty; to fix, compute the values first (e.g., const importedEditedBy
= await this._cache.findImportedUser(data.editedBy); const convertedReactions =
data.reactions ? await this.convertMessageReactions(data.reactions) : undefined)
and only spread { editedBy: importedEditedBy } if importedEditedBy !==
undefined, and only spread { reactions: convertedReactions } if
convertedReactions !== undefined (and/or non-empty), so editedBy and reactions
are omitted instead of serialized as undefined.
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
1 participant
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
Fixes an issue where messages imported via Slack or CSV importers were not appearing in channels after import completion.
Although rooms were successfully created, message conversion failed because newly created rooms were not registered in the importer cache used for resolving imported room IDs.
This resulted in message insertion errors (
importer-message-unknown-room) and missing chat history after import.Changes
ConverterCacheafter successful room creationResult
After this change:
Testing
fixes issue #39257
316b6553-4513-4861-975b-58f496129a85.mp4
Tested using manufactured Slack-style datasets and CSV imports containing:
Messages are now successfully inserted and visible in imported channels.
Note/Correction
second commit to be named
fix(import): preserve explicit null/boolean fields (e.g., thread-related fields) during message import