-
Notifications
You must be signed in to change notification settings - Fork 1.4k
fix: message thread preview style #6886
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
Conversation
WalkthroughThe PR adds a Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
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: 0
🧹 Nitpick comments (1)
app/containers/message/Message.tsx (1)
170-171: LGTM! The flex style correctly enables ellipsis for thread/info messages.Adding
flex: 1to theA11y.Indexallows it to stretch horizontally within its flex row container, enabling text truncation with ellipsis. This is the appropriate fix for the thread-specific rendering path.Optional refactoring for performance:
The inline style object
{{ flex: 1 }}creates a new object on every render. Consider extracting it tostyles.ts:Suggested refactor
In
styles.ts, add:flex1: { flex: 1 }Then update the component:
- style={{ flex: 1 }}> + style={styles.flex1}>Regarding consistency with the second
A11y.Index(lines 189-193):The regular message path has a different structural layout: the
A11y.Indexwraps a flex container, rather than being a flex item itself. This architectural difference justifies why it doesn't require the sameflex: 1styling. No changes needed there.
📜 Review details
Configuration used: Organization 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.tsx
⏰ 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 (1)
app/containers/message/Message.tsx (1)
170-171: AI summary inconsistency detected.The AI-generated summary claims the style prop was added "in two locations within Message.tsx", but only one
A11y.Indexcomponent (lines 170-171) was modified. The secondA11y.Indexat lines 188-192 remains unchanged.
OtavioStasiak
left a comment
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.
Looks good to me!
Proposed changes
Fixed the missing ellipsis on thread preview
Issue(s)
https://rocketchat.atlassian.net/browse/CORE-1609
How to test or reproduce
Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.