feat: Support message with multiple attachments#6068
Conversation
|
This PR will also close #5942 |
diegolmello
force-pushed
the
feat.multiple-files-render
branch
from
009223d to
8717d5a
Compare
|
I have noticed an issue: when we send the same images in different messages, only the image in the first message will open in the preview, not the ones in the rest. For example, if we send images one and two across three messages, we can tap and open the preview for the image in the first message, but not for the images in the second and third messages. |
|
Fixed in #6093 |
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
WalkthroughAdds multi-attachment message story variants (images, video, audio, files, large-font, and isHeader=false cases) and threads downloadUrl through the media download flow so attachment title_link is set when the downloaded URL matches an attachment’s image_url. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
OtavioStasiak
had a problem deploying
to
official_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/containers/message/Message.stories.tsx (1)
746-758:⚠️ Potential issue | 🟡 Minor
<Message>used instead of<MessageLargeFont>insideWithImageLargeFontThe new multi-file block at line 746 renders with the default (1×) font scale instead of the
FONT_SCALE_LIMITscale that the rest of this export uses. This undermines the visual accuracy of large-font snapshots.🔧 Proposed fix
- <Message + <MessageLargeFont msg='multi file' attachments={[ { title: 'This is a title', image_url: '/dummypath' }, { title: 'This is a title', image_url: '/dummypath' } ]} - /> + />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/containers/message/Message.stories.tsx` around lines 746 - 758, The WithImageLargeFont story is rendering the multi-file block with the default Message component instead of the large-font variant; update the component used for that multi-attachment render to MessageLargeFont so it respects the FONT_SCALE_LIMIT applied elsewhere in the export (locate the Message usage in the WithImageLargeFont story and swap it to MessageLargeFont).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/containers/message/Message.stories.tsx`:
- Around line 1119-1129: The snapshot for FileAttachmentsWithFilenamesLargeFont
is using the regular Message component instead of the large-font variant; update
the JSX in the story where the multi-file block is rendered (the <Message ... />
instance inside FileAttachmentsWithFilenamesLargeFont) to use <MessageLargeFont
... /> so the multi-file attachments snapshot reflects large-font scaling
consistently with other fixtures.
In `@app/lib/methods/handleMediaDownload.ts`:
- Line 216: The current use of substring matching with
downloadUrl.includes(att.image_url) when setting title_link can produce false
positives; update the check in the title_link assignment to perform an exact
match between the attachment URL and the downloadUrl (e.g., normalize both by
removing query strings or comparing full normalized URLs) before choosing uri
over att.title_link—replace the includes() test with a precise equality check on
the normalized att.image_url and downloadUrl values in the same scope where
title_link is computed.
- Around line 214-218: The attachments mapping in handleMediaDownload.ts only
checks att.image_url when deciding to set att.title_link, so video/audio
attachments never get their title_link updated; update the condition inside the
attachments?.map callback to check for att.image_url || att.video_url ||
att.audio_url and test downloadUrl.includes(...) against whichever URL exists
(preferring image_url then video_url then audio_url), then set title_link to uri
when that match is true and preserve the existing e2e logic (encryption ? 'done'
: undefined) so video/audio attachments receive the local file:// URI in
title_link just like images.
---
Outside diff comments:
In `@app/containers/message/Message.stories.tsx`:
- Around line 746-758: The WithImageLargeFont story is rendering the multi-file
block with the default Message component instead of the large-font variant;
update the component used for that multi-attachment render to MessageLargeFont
so it respects the FONT_SCALE_LIMIT applied elsewhere in the export (locate the
Message usage in the WithImageLargeFont story and swap it to MessageLargeFont).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
app/containers/message/__snapshots__/Message.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
app/containers/message/Message.stories.tsxapp/lib/methods/handleMediaDownload.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). (2)
- GitHub Check: ESLint and Test / run-eslint-and-test
- GitHub Check: format
🧰 Additional context used
🧬 Code graph analysis (1)
app/lib/methods/handleMediaDownload.ts (3)
app/definitions/IAttachment.ts (1)
IAttachment(9-48)app/definitions/IMessage.ts (1)
TMessageModel(180-183)app/lib/database/services/Message.ts (1)
getMessageById(7-19)
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Failure
|
Approved... tested with the latest changes
|
OtavioStasiak
had a problem deploying
to
official_android_build
GitHub Actions
Failure
OtavioStasiak
had a problem deploying
to
official_ios_build
GitHub Actions
Failure
OtavioStasiak
had a problem deploying
to
experimental_android_build
GitHub Actions
Failure
OtavioStasiak
had a problem deploying
to
experimental_ios_build
GitHub Actions
Failure
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/containers/message/Message.stories.tsx (1)
746-758:⚠️ Potential issue | 🟡 Minor
<Message>used instead of<MessageLargeFont>insideWithImageLargeFontThe multi-file image block renders with the regular
Messagecomponent, so large-font scaling will not be reflected in this story variant — inconsistent with every other multi-file block added in the LargeFont stories (WithVideoLargeFont,WithAudioLargeFont,FileAttachmentsWithFilenamesLargeFont).🔧 Proposed fix
- <Message + <MessageLargeFont msg='multi file' attachments={[ { title: 'This is a title', image_url: '/dummypath' }, { title: 'This is a title', image_url: '/dummypath' } ]} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/containers/message/Message.stories.tsx` around lines 746 - 758, The story variant WithImageLargeFont incorrectly renders the multi-file image block using the regular Message component; change the component instance in that block from Message to MessageLargeFont so the large-font scaling is applied consistently (locate the JSX where Message is used inside WithImageLargeFont and replace it with MessageLargeFont).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@app/containers/message/Message.stories.tsx`:
- Around line 746-758: The story variant WithImageLargeFont incorrectly renders
the multi-file image block using the regular Message component; change the
component instance in that block from Message to MessageLargeFont so the
large-font scaling is applied consistently (locate the JSX where Message is used
inside WithImageLargeFont and replace it with MessageLargeFont).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
app/containers/message/__snapshots__/Message.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (1)
app/containers/message/Message.stories.tsx
📜 Review details
🔇 Additional comments (5)
app/containers/message/Message.stories.tsx (5)
676-676: LGTM — description text updated consistently with the rest of the file.
700-712: LGTM — new multi-file image block correctly uses<Message>for the normal story variant.
786-839: LGTM —WithVideomulti-file block uses the correct<Message>component, andWithVideoLargeFontcorrectly uses<MessageLargeFont>.
858-949: LGTM —WithAudioandWithAudioLargeFontmulti-file blocks use the correct components.
1119-1129: LGTM — the multi-file file-attachment block now correctly uses<MessageLargeFont>, addressing the previous review comment.
3 participants
Proposed changes
Issue(s)
closes #5942
How to test or reproduce
Screenshots
Types of changes
Checklist
Further comments
CORE-848
Summary by CodeRabbit
New Features
Tests