-
Notifications
You must be signed in to change notification settings - Fork 13.1k
feat(apps): new IMessage.files to apps #38099
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
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 |
|
WalkthroughAdded a plural Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #38099 +/- ##
===========================================
- Coverage 70.85% 70.46% -0.39%
===========================================
Files 3161 3162 +1
Lines 109785 110165 +380
Branches 19688 19890 +202
===========================================
- Hits 77783 77631 -152
- Misses 29973 30500 +527
- Partials 2029 2034 +5
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issues found across 3 files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/meteor/app/apps/server/converters/messages.js`:
- Around line 342-349: The fileId resolver currently only falls back to
message.file, so new-format messages using message.files can yield undefined;
update the fileId arrow function (the attachment.fileId handler) to first check
for message.files && message.files.length === 1 and return message.files[0]._id,
then fall back to message.file._id if present, and otherwise return
attachment.fileId; also remove the inline comment from that function body per
guidelines.
In `@apps/meteor/app/apps/server/converters/threads.ts`:
- Around line 203-214: The fileId getter currently only falls back to
message.file; update the logic in the fileId function inside the converter in
threads.ts to also check message.files when message.attachments length === 1: if
isFileAttachment(attachment) and message.files?.length === 1 and
message.files[0]?._id return that ID (before falling back to message.file),
remove the inline comment as per guidelines, and keep other return paths
(existing attachment.fileId and final undefined) intact.
🧹 Nitpick comments (2)
apps/meteor/app/apps/server/converters/messages.js (1)
98-114: Replace inline comment with a self-documenting predicate.The logic is fine, but the inline comment can be removed by naming the condition.
Proposed refactor
- return message.files?.map((file) => { + return message.files?.map((file) => { if (!file || file.typeGroup) { return file; } - // Thumbnails from older messages did not have any identification but we can extrapolate this information from other data - if (message.files.length === 2 && message.attachments?.length === 1 && file === message.files[1]) { + const isLegacyThumb = + message.files.length === 2 && message.attachments?.length === 1 && file === message.files[1]; + if (isLegacyThumb) { return { ...file, typeGroup: 'thumb', }; }As per coding guidelines: Avoid code comments in the implementation.
apps/meteor/app/apps/server/converters/threads.ts (1)
122-138: Replace inline comment with a self-documenting predicate.The logic is fine, but the inline comment can be removed by naming the condition.
Proposed refactor
- files: async (message: IMessage) => { + files: async (message: IMessage) => { return message.files?.map((file) => { if (!file || file.typeGroup) { return file; } - // Thumbnails from older messages did not have any identification but we can extrapolate this information from other data - if (message.files?.length === 2 && message.attachments?.length === 1 && file === message.files[1]) { + const isLegacyThumb = + message.files?.length === 2 && message.attachments?.length === 1 && file === message.files[1]; + if (isLegacyThumb) { return { ...file, typeGroup: 'thumb', }; }As per coding guidelines: Avoid code comments in the implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 issue found across 5 files
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/app/apps/server/converters/threads.ts">
<violation number="1" location="apps/meteor/app/apps/server/converters/threads.ts:129">
P2: The new thumbnail detection in `files` checks `message.attachments?.length`, but `attachments` is deleted earlier in the same map execution. Because `transformMappedData` iterates map entries in order, this condition will always be false and legacy thumbnails won’t get `typeGroup: 'thumb'`. Capture the attachments count before deletion or use the original message value instead.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issues found across 6 files
Needs changeset with bump to apps-engine
Proposed changes (including videos or screenshots)
Issue(s)
CORE-1633
Steps to test or reproduce
Further comments
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.