fix: prevent null fields in Slack import breaking message history #37805
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 |
|
|
WalkthroughUpdated MessageConverter.buildMessageObject to use conditional spreads and nullish coalescing so optional fields (mentions, channels, replies, editedBy, reactions, attachments, etc.) are only included when present or default to empty arrays, preventing undefined/null assignments. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 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)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 0
🧹 Nitpick comments (1)
apps/meteor/app/importer/server/classes/converters/MessageConverter.ts (1)
86-87: Consider ternary operator for clearer null-handling.The
&&operator with??can preserve non-null/undefined falsy values (false, 0, ''). Ifdata.mentionsisfalseor0, the expression evaluates to that value instead of[].While Slack import data is unlikely to contain such values, using a ternary would be more explicit:
-const mentions = (data.mentions && (await this.convertMessageMentions(data))) ?? []; -const channels = (data.channels && (await this.convertMessageChannels(data))) ?? []; +const mentions = data.mentions ? (await this.convertMessageMentions(data)) ?? [] : []; +const channels = data.channels ? (await this.convertMessageChannels(data)) ?? [] : [];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/meteor/app/importer/server/classes/converters/MessageConverter.ts(2 hunks)
🧰 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/MessageConverter.ts
🔇 Additional comments (4)
apps/meteor/app/importer/server/classes/converters/MessageConverter.ts (4)
97-101: Excellent fix for the null-field issue.Using
??instead of||correctly preserves meaningful falsy values while converting null to undefined:
- Line 98:
groupablecan be explicitlyfalse(message is not groupable)- Line 99:
tmidis the critical fix—null breaks message-loading queries; now properly undefined- Line 101:
tcountcan be0(thread with no replies yet)This directly resolves the root cause described in issue #37058.
102-102: Correct handling of optional replies field.The logic properly omits the
repliesproperty whendata.repliesis absent (undefined prevents property insertion), while preserving the converted array when present. This aligns with the conditional property pattern used elsewhere (lines 113-114).
103-104: Proper null-to-undefined conversion for edit metadata.Both fields correctly convert null to undefined:
editedAt: preserves valid timestamps (including 0 if present) while removing nulleditedBy: chains user lookup with??to handle missing user data gracefully
107-108: Consistent null-handling for remaining optional fields.All fields correctly use
??to convert null → undefined while preserving meaningful falsy values:
bot(line 110): preserves explicitfalse(not a bot) vs undefined (unknown)url,alias: preserve empty strings if present (more explicit than undefined)_importFile: import metadata properly defaultedAlso applies to: 110-110, 112-112
- Adds migration v333 to fix issue where Slack imports create messages with null fields - Removes null values from fields like tmid, groupable, tlm, etc. using - Converts null mentions and urls arrays to empty arrays using - Fixes message history not loading after Slack import Fixes RocketChat#37058 Complements RocketChat#37805
|
@sanki92 With this pull request it still seems like Slack messages do no show up after import. To show any message at all, the following is required (other commands are also needed to properly display the messages): |
|
@sanki92 I am not really familiar with MongoDB, but according to Claud AI:
The following seems to work and imported Slack message finally show up after import: diff --git a/apps/meteor/app/importer/server/classes/converters/MessageConverter.ts b/apps/meteor/app/importer/server/classes/converters/MessageConverter.ts
index 87ddfe29c4..3aed0db174 100644
--- a/apps/meteor/app/importer/server/classes/converters/MessageConverter.ts
+++ b/apps/meteor/app/importer/server/classes/converters/MessageConverter.ts
@@ -94,24 +94,24 @@ export class MessageConverter extends RecordConverter<IImportMessageRecord> {
},
msg: data.msg,
ts: data.ts,
- t: data.t ?? undefined,
- groupable: data.groupable ?? undefined,
- tmid: data.tmid ?? undefined,
- tlm: data.tlm ?? undefined,
- tcount: data.tcount ?? undefined,
- replies: (data.replies && (await this.convertMessageReplies(data.replies))) ?? undefined,
- editedAt: data.editedAt ?? undefined,
- editedBy: data.editedBy && ((await this._cache.findImportedUser(data.editedBy)) ?? undefined),
mentions,
channels,
- _importFile: data._importFile ?? undefined,
- url: data.url ?? undefined,
- attachments: data.attachments,
- bot: data.bot ?? undefined,
- emoji: data.emoji,
- alias: data.alias ?? undefined,
- ...(data._id ? { _id: data._id } : {}),
- ...(data.reactions ? { reactions: await this.convertMessageReactions(data.reactions) } : {}),
+ ...(data.t != null && { t: data.t }),
+ ...(data.groupable != null && { groupable: data.groupable }),
+ ...(data.tmid != null && { tmid: data.tmid }),
+ ...(data.tlm != null && { tlm: data.tlm }),
+ ...(data.tcount != null && { tcount: data.tcount }),
+ ...(data.replies && { replies: await this.convertMessageReplies(data.replies) }),
+ ...(data.editedAt && { editedAt: data.editedAt }),
+ ...(data.editedBy && { editedBy: await this._cache.findImportedUser(data.editedBy) }),
+ ...(data._importFile && { _importFile: data._importFile }),
+ ...(data.url && { url: data.url }),
+ ...(data.attachments && { attachments: data.attachments }),
+ ...(data.bot != null && { 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) }),
};
}Can you confirm if this change would make sense or if it need additional adaptations? |
sanki92
force-pushed
the
fix/slack-import-null-fields
branch
from
8c5df77 to
240192f
Compare
|
@ghuls You're absolutely right, thanks for catching that! I've applied your fix with one addition, pre-computing the async values before checking them: const replies = data.replies ? await this.convertMessageReplies(data.replies) : undefined;
const editedBy = data.editedBy ? await this._cache.findImportedUser(data.editedBy) : undefined;
const reactions = data.reactions ? await this.convertMessageReactions(data.reactions) : undefined;
// Then:
...(replies && replies.length > 0 && { replies }),
...(editedBy && { editedBy }),
...(reactions && { reactions }),This handles cases where cache lookups return Should work now for new imports without needing the database cleanup script! |
3 participants
Proposed changes
Fixes Slack import where messages dont show in chat history even though they're in the database.
The problem is in
MessageConverter.buildMessageObject- it was setting fields tonullinstead ofundefined. Whentmid(thread message ID) isnull, the message loading query breaks and nothing shows up in chat.Changed to use
??operator to convertnull→undefinedwhile keeping valid falsy values like0andfalse:tmid,tlm,tcount,groupable,editedAt, etc. now use?? undefinedmentionsandchannelsdefault to[]instead ofnullThis makes imported messages match the structure of regular messages
Issue(s)
Closes #37058
Steps to test or reproduce
Before this fix, you'd see empty chat history even though messages are in the database and searchable.
Further comments
Thanks to @tlueder for the detailed bug report and database investigation that made this fix possible.
For existing imports with the bug, you can clean up the database using the script from issue #37058:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.