regression: Collapsible quote not rendering after #6121 #6677
+2,415
−1
Conversation
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
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
|
Caution Review failedThe pull request is closed. WalkthroughUpdated attachment filtering to treat attachments with Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Story as Storybook
participant Message
participant Attachments
participant Filter as removeQuote()
participant Renderer
Story->>Message: Render message with attachments (includes `collapsed` flag)
Message->>Attachments: Pass attachments array
Attachments->>Filter: Evaluate each attachment
Note right of Filter #E6F4EA: Non-quote if image_url OR audio_url OR video_url OR actions.length > 0 OR collapsed == true
Filter-->>Attachments: Return nonQuoteAttachments
Attachments->>Renderer: Render non-quote attachments (including collapsed)
Renderer-->>Message: Display attachments in UI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
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 |
diegolmello
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
diegolmello
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
diegolmello
had a problem deploying
to
official_ios_build
GitHub Actions
Error
diegolmello
had a problem deploying
to
official_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: 1
🧹 Nitpick comments (1)
app/containers/message/Components/Attachments/Attachments.tsx (1)
76-78: Add a key to AttachedActions to avoid list reconciliation issues.Currently missing a
key, which can cause warnings and unstable reconciliation.- return <AttachedActions attachment={file} getCustomEmoji={getCustomEmoji} />; + return <AttachedActions key={`actions-${index}`} attachment={file} getCustomEmoji={getCustomEmoji} />;
📜 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)
app/containers/message/Components/Attachments/Attachments.tsx(1 hunks)
⏰ 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: ESLint and Test / run-eslint-and-test
|
|
||
| const removeQuote = (file?: IAttachment) => | ||
| file?.image_url || file?.audio_url || file?.video_url || (file?.actions?.length || 0) > 0; | ||
| file?.image_url || file?.audio_url || file?.video_url || (file?.actions?.length || 0) > 0 || file?.collapsed; |
There was a problem hiding this comment.
Fix filter: truthy check skips non-collapsed quotes. Use explicit boolean check.
file?.collapsed only includes true, excluding quotes with collapsed === false. Since rendering later checks typeof file.collapsed === 'boolean', align the filter to include both states.
- file?.image_url || file?.audio_url || file?.video_url || (file?.actions?.length || 0) > 0 || file?.collapsed;
+ file?.image_url || file?.audio_url || file?.video_url || (file?.actions?.length || 0) > 0 || typeof file?.collapsed === 'boolean';Optional (naming clarity):
- Rename
removeQuotetoisRenderableAttachment. - Rename
nonQuoteAttachmentstorenderableAttachments.
-const removeQuote = (file?: IAttachment) =>
- file?.image_url || file?.audio_url || file?.video_url || (file?.actions?.length || 0) > 0 || typeof file?.collapsed === 'boolean';
+const isRenderableAttachment = (file?: IAttachment) =>
+ file?.image_url || file?.audio_url || file?.video_url || (file?.actions?.length || 0) > 0 || typeof file?.collapsed === 'boolean';
-const nonQuoteAttachments = attachments?.filter(removeQuote);
+const renderableAttachments = attachments?.filter(isRenderableAttachment);And update subsequent uses accordingly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| file?.image_url || file?.audio_url || file?.video_url || (file?.actions?.length || 0) > 0 || file?.collapsed; | |
| file?.image_url || file?.audio_url || file?.video_url || (file?.actions?.length || 0) > 0 || typeof file?.collapsed === 'boolean'; |
🤖 Prompt for AI Agents
In app/containers/message/Components/Attachments/Attachments.tsx around line 16,
the filter uses file?.collapsed as a truthy check which excludes attachments
where collapsed === false (quotes that should still be considered); change the
filter to explicitly include boolean collapsed values (e.g. typeof
file.collapsed === 'boolean' or file.collapsed !== undefined) alongside the
existing image/audio/video/actions checks so both true and false collapsed are
kept, and optionally rename removeQuote to isRenderableAttachment and
nonQuoteAttachments to renderableAttachments and update all subsequent
references to match.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/containers/message/Message.stories.tsx (1)
1989-2017: Nit: rename constant to singular for clarity.The value is a single attachment object. Optional, but improves readability.
Apply:
-const collapsedAttachments = { +const collapsedAttachment = { collapsed: true, title: 'Title collapsed', fields: [ { title: 'Field 1', value: 'Value 1' }, { title: 'Field 2', value: 'Value 2' } ] }; export const CollapsedAttachments = () => ( <> - <Message msg='Message' attachments={[collapsedAttachments]} /> + <Message msg='Message' attachments={[collapsedAttachment]} /> {/* technically not CollapsibleQuote, but it's similar enough to write a story for */} - <Message msg='Message' attachments={[{ ...collapsedAttachments, collapsed: false }]} /> + <Message msg='Message' attachments={[{ ...collapsedAttachment, collapsed: false }]} /> </> ); export const CollapsedAttachmentsLargeFont = () => ( <> - <MessageLargeFont msg='Message' attachments={[collapsedAttachments]} /> + <MessageLargeFont msg='Message' attachments={[collapsedAttachment]} /> {/* technically not CollapsibleQuote, but it's similar enough to write a story for */} - <MessageLargeFont msg='Message' attachments={[{ ...collapsedAttachments, collapsed: false }]} /> + <MessageLargeFont msg='Message' attachments={[{ ...collapsedAttachment, collapsed: false }]} /> </> );
📜 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 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(1 hunks)
⏰ 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: ESLint and Test / run-eslint-and-test
🔇 Additional comments (3)
app/containers/message/Message.stories.tsx (3)
2003-2009: Story coverage LGTM.Good side-by-side of collapsed vs non-collapsed.
2011-2017: Large-font story LGTM.Mirrors the normal variant as expected.
1989-2002: Resolved — IAttachment already declarescollapsed?: boolean.app/definitions/IAttachment.ts contains
collapsed?: boolean.
diegolmello
force-pushed
the
regression.collapsible-quote
branch
from
60a2335 to
075c465
Compare
diegolmello
requested a deployment
to
approve_e2e_testing
GitHub Actions
Waiting
diegolmello
requested a deployment
to
official_android_build
GitHub Actions
Waiting
diegolmello
requested a deployment
to
experimental_ios_build
GitHub Actions
Waiting
diegolmello
requested a deployment
to
official_ios_build
GitHub Actions
Waiting
diegolmello
requested a deployment
to
experimental_android_build
GitHub Actions
Waiting
10 tasks
10 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Proposed changes
Regression introduced on #6121
Issue(s)
https://rocketchat.atlassian.net/browse/NATIVE-1031
How to test or reproduce
Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit
Bug Fixes
Documentation