fix: Allow longer lines in attachments messages #37428#37438
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 |
|
Lay Sandeep Shah seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
WalkthroughAdded inline styles to the AttachmentText component to preserve whitespace and improve wrapping: Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes
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.
Actionable comments posted: 1
📜 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/client/components/message/content/attachments/structure/AttachmentText.tsx(1 hunks)
🔇 Additional comments (1)
apps/meteor/client/components/message/content/attachments/structure/AttachmentText.tsx (1)
13-17: Verify aggressive text breaking doesn't damage URLs and code snippets.The CSS rule
overflowWrap: 'anywhere'allows the browser to break lines at any character, which may cause usability issues:
- URLs — Long URLs might break at confusing points (e.g.,
https://ex|ample.com/path).- Code snippets or identifiers — Breaking mid-token reduces readability.
Note: The presence of both
wordBreak: 'break-word'andoverflowWrap: 'anywhere'is redundant—in CSS cascade,overflowWraptakes precedence, makingbreak-wordineffective.Test with:
- Messages containing long URLs
- Messages with code blocks or long identifiers
- Different viewport sizes (mobile, desktop) to confirm behavior
If URLs or code break inappropriately, consider using
overflowWrap: 'break-word'alone (breaks only when no other valid break point exists) or applying conditional styles if different content types need different treatment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/meteor/client/components/message/content/attachments/structure/AttachmentText.tsx (1)
13-17: Verify responsive wrapping behavior against PR objectives.The inline styles look appropriate for preventing overflow and allowing wrapping. The previous deprecation issue with
wordBreak: 'break-word'has been correctly addressed by usingwordBreak: 'normal'.However, the PR description states:
"Adjusted wrapping logic to be responsive: conservative wrapping on small screens; allowing longer lines on medium/large screens."
The current implementation applies uniform styles regardless of viewport size. There are no media queries or conditional logic to differentiate behavior between mobile (≤480–600px) and desktop (≥1024px) viewports.
Can you clarify how the responsive wrapping behavior is achieved? If the Box component or parent containers handle this automatically, that's fine—but if viewport-specific logic is needed, it may require additional implementation.
Optional:
wordBreak: 'normal'is the default value and technically redundant, though keeping it explicit does provide clarity and documents the intent after addressing the deprecation.
📜 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/client/components/message/content/attachments/structure/AttachmentText.tsx(1 hunks)
Proposed changes :
This PR increases the allowed line length for text inside message attachments, so lines no longer wrap prematurely on desktop. The previous behavior caused attachment text to break into many short lines.
What I changed :
Raised the per-line character/width threshold used by the attachment text renderer.
Adjusted the wrapping logic so it’s responsive:
i . On small screens, lines still wrap conservatively to avoid horizontal scroll.
ii. On medium/large screens, lines can span more characters before wrapping.
Updated styles to respect container width and preserve word breaks/URLs correctly.
Added safeguards to avoid overflow and to keep emoji/code spans aligned.
This aligns with the request in the issue “Allow longer lines in attachments messages.”
GitHub
Steps to test or reproduce
Using the REST API, send a message with a long attachments[0].text field (include long words, a URL, and normal prose), for example to a test channel.
View the message on:
A mobile-sized viewport (≤ 480–600px wide).
A desktop viewport (≥ 1024px wide).
Expected behavior after this PR:
On mobile, lines wrap early enough to avoid horizontal scrolling.
On desktop, lines are noticeably longer per line (fewer total lines), with no overflow or layout shift.
Screenshots :
Summary by CodeRabbit