fix: File rendering as URL #6725
Conversation
diegolmello
force-pushed
the
fix.file-name-url
branch
from
91f5036 to
c034d18
Compare
diegolmello
temporarily deployed
to
approve_e2e_testing
GitHub Actions
Inactive
WalkthroughRemoved theme-based color/styling from Markdown render paths, simplified Markdown props/API, dropped style/isReply from attachment component props and interfaces, changed Reply to use MarkdownPreview for file-only attachments, stopped forwarding some Markdown props from Message content, and added Storybook stories for file attachments/replies. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant MR as MessageRenderer
participant A as Attachments
participant R as Reply
participant MD as Markdown
participant MP as MarkdownPreview
U->>MR: Render message
MR->>A: Render attachments (no style/isReply props)
alt message has reply
A->>R: Render reply block
alt attachment.type == 'file' AND no text
R-->>MP: Render filename via MarkdownPreview
else
R-->>MD: Render description via Markdown (username/emojis)
end
else
A-->>MD: Render msg only when msg exists
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
⛔ Files ignored due to path filters (3)
📒 Files selected for processing (13)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used🧬 Code graph analysis (4)app/containers/message/Components/Attachments/Reply.tsx (2)
app/containers/UIKit/index.tsx (2)
app/containers/message/Components/Attachments/Attachments.tsx (1)
app/containers/message/Components/Attachments/Audio.tsx (1)
🔇 Additional comments (9)
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
temporarily deployed
to
experimental_ios_build
GitHub Actions
Inactive
diegolmello
temporarily deployed
to
experimental_android_build
GitHub Actions
Inactive
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: 0
🧹 Nitpick comments (1)
app/containers/message/Components/Attachments/CollapsibleQuote/index.tsx (1)
38-42: Consider removing the unusedfieldTextstyle.The
fieldTextstyle definition is no longer used after removing the style prop from the Markdown component at line 96.Apply this diff to remove the unused style:
fieldContainer: { flexDirection: 'column', paddingLeft: 10, paddingVertical: 10 }, - fieldText: { - fontSize: 15, - padding: 10, - ...sharedStyles.textRegular - }, fieldTitle: { fontSize: 15, ...sharedStyles.textBold },
📜 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 (3)
app/containers/UIKit/__snapshots__/UiKitModal.test.tsx.snapis excluded by!**/*.snapapp/containers/markdown/__snapshots__/Markdown.test.tsx.snapis excluded by!**/*.snapapp/containers/message/__snapshots__/Message.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (16)
app/containers/UIKit/index.tsx(1 hunks)app/containers/markdown/Markdown.stories.tsx(1 hunks)app/containers/markdown/index.tsx(1 hunks)app/containers/message/Components/Attachments/Attachments.tsx(3 hunks)app/containers/message/Components/Attachments/Audio.tsx(2 hunks)app/containers/message/Components/Attachments/CollapsibleQuote/index.tsx(1 hunks)app/containers/message/Components/Attachments/Image/Container.tsx(1 hunks)app/containers/message/Components/Attachments/Image/definitions.ts(0 hunks)app/containers/message/Components/Attachments/Reply.tsx(6 hunks)app/containers/message/Components/Attachments/Video.tsx(3 hunks)app/containers/message/Content.tsx(0 hunks)app/containers/message/Message.stories.tsx(3 hunks)app/containers/message/interfaces.ts(1 hunks)app/views/CannedResponseDetail.tsx(1 hunks)app/views/E2EHowItWorksView.tsx(1 hunks)app/views/RoomInfoView/Item.tsx(1 hunks)
💤 Files with no reviewable changes (2)
- app/containers/message/Components/Attachments/Image/definitions.ts
- app/containers/message/Content.tsx
🧰 Additional context used
🧬 Code graph analysis (5)
app/containers/message/Components/Attachments/Audio.tsx (1)
app/containers/message/hooks/useMediaAutoDownload.tsx (1)
useMediaAutoDownload(51-188)
app/containers/UIKit/index.tsx (2)
app/containers/markdown/Markdown.stories.tsx (1)
Text(51-59)app/lib/constants/colors.ts (1)
themes(304-304)
app/containers/message/Components/Attachments/Reply.tsx (2)
app/definitions/IAttachment.ts (1)
IAttachment(9-48)app/definitions/IEmoji.ts (1)
TGetCustomEmoji(23-23)
app/containers/message/Components/Attachments/Attachments.tsx (1)
app/containers/message/interfaces.ts (1)
IMessageAttachments(10-16)
app/views/RoomInfoView/Item.tsx (1)
app/containers/MessageComposer/components/Toolbar/Markdown.tsx (1)
Markdown(9-46)
⏰ 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 (14)
app/views/RoomInfoView/Item.tsx (1)
24-24: LGTM! Markdown styling simplified.The removal of the
styleprop aligns with the PR's broader refactor to standardize Markdown rendering with default styling across the codebase.app/containers/markdown/Markdown.stories.tsx (1)
127-127: LGTM! Story updated for default styling.The story correctly demonstrates Markdown link rendering without custom styling, consistent with the broader refactor to remove theme-dependent styling from Markdown components.
app/views/CannedResponseDetail.tsx (1)
80-80: LGTM! Markdown simplified in canned response detail.The removal of style customization is consistent with the broader refactor. Content will now render with default Markdown styling instead of custom font/color settings.
app/containers/UIKit/index.tsx (1)
64-74: LGTM! UIKit Markdown rendering simplified.The removal of theme-based styling from
MarkdownPreviewandMarkdowncomponents in the UIKit context simplifies the rendering logic. However, ensure that UIKit messages remain visually distinguishable and that the removal of color styling doesn't impact the user experience for UIKit interactions.app/views/E2EHowItWorksView.tsx (1)
33-36: LGTM! E2E help content simplified.The removal of custom styling from the E2E "How It Works" view Markdown components is consistent with the broader refactor to standardize Markdown rendering with default styles.
app/containers/message/Message.stories.tsx (1)
983-1342: Excellent test coverage for file attachments!The four new story exports provide comprehensive coverage for file attachment scenarios:
- Single files with filenames
- Files with markdown descriptions
- Multiple file attachments
- Both normal and large font variants
These stories effectively demonstrate the fix for the issue where attachments with URL-like filenames were opening in the browser. The use of
type: 'file',title, andtitle_linkproperties correctly represents file attachments.app/containers/message/interfaces.ts (1)
1-16: No remainingstyle/isReplyusages found; safe to merge.app/containers/markdown/index.tsx (1)
1-35: Verified: No<Markdown>usages with removed props.
All outdated props (tmid,numberOfLines,customEmojis,enableMessageParser,testID,style) have been dropped from callsites.app/containers/message/Components/Attachments/Image/Container.tsx (1)
12-20: LGTM!The removal of
styleandisReplyprops simplifies the component interface and aligns with the broader refactoring across attachment components. The Markdown rendering now uses default styling consistently.Also applies to: 37-37
app/containers/message/Components/Attachments/Reply.tsx (2)
151-151: Correct memoization update.Properly updated the memoization check to include
attachment.typecomparison, which is necessary since the Description component now branches on this field.
132-142: Approve MarkdownPreview usage for file attachmentsThe
attachment.type === 'file'check correctly covers all file‐type cases and!attachment.textreliably isolates filename‐only attachments forMarkdownPreview.app/containers/message/Components/Attachments/Video.tsx (1)
37-43: LGTM!The interface simplification and conditional Markdown rendering are consistent with the changes across other attachment components (Audio, Image). The component now uses default Markdown styling and only renders the description when
msgis present.Also applies to: 73-73, 105-105
app/containers/message/Components/Attachments/Attachments.tsx (1)
19-19: LGTM!The parent
Attachmentscomponent correctly propagates the interface simplification by no longer passingstyleandisReplyprops to child attachment components. This aligns with the updatedIMessageAttachmentsinterface and the changes in the child components (Image, Audio, Video).Also applies to: 34-44, 48-48, 53-61
app/containers/message/Components/Attachments/Audio.tsx (1)
11-16: LGTM!The interface simplification and conditional Markdown rendering are consistent with the parallel changes in Video and Image attachment components. The component maintains its audio playback functionality while simplifying the description rendering.
Also applies to: 18-18, 26-26
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
app/containers/message/Components/Attachments/CollapsibleQuote/index.tsx (1)
96-96: LGTM! Remove unused fieldText style.The Markdown component now uses default styling. Consider removing the now-unused
fieldTextstyle definition at lines 38-42.Apply this diff to remove the unused style:
fieldTitle: { fontSize: 15, ...sharedStyles.textBold }, - fieldText: { - fontSize: 15, - padding: 10, - ...sharedStyles.textRegular - }, marginTop: { marginTop: 4 },app/containers/message/Components/Attachments/Attachments.tsx (1)
18-22: API reduction is cohesive; consider memo comparator breadth
- Removing style/isReply and passing only required props to children is consistent.
- The memo comparator only checks attachments; if author/showAttachment/getCustomEmoji change, children won’t re-render. If that’s a concern, include them in the comparator (or keep as-is if performance > freshness here).
Example tweak:
- (prevProps, nextProps) => dequal(prevProps.attachments, nextProps.attachments) + (prevProps, nextProps) => + dequal(prevProps.attachments, nextProps.attachments) && + prevProps.author === nextProps.author && + prevProps.showAttachment === nextProps.showAttachment && + prevProps.getCustomEmoji === nextProps.getCustomEmojiAlso applies to: 30-45, 47-49, 75-76
app/containers/message/Components/Attachments/Reply.tsx (2)
122-143: Using MarkdownPreview for file-name text prevents linkification — this addresses the bugGood call on treating file attachments (with no explicit text) as plain preview to avoid URL-like filenames becoming links.
Please add a story/E2E covering a file named like “https://foo.txt” inside a reply to ensure it renders as text and tapping downloads (not opens browser). I can help scaffold it.
277-277: Conditional Markdown for msg is fine; tiny safety nitIf user.username can be undefined in some contexts, pass username={user?.username} to avoid crashes. If guaranteed by context, ignore.
app/containers/message/Message.stories.tsx (2)
983-1042: Consider adding edge cases for URL-like filenames.The new story demonstrates file attachments well, but doesn't include test cases for the core bug being fixed: attachments with filenames that resemble URLs (e.g., "https://example.com", "www.google.com").
Consider adding examples like:
{ type: 'file', title: 'https://example.com', title_link: '/file-upload/edge123/https-example.com' }, { type: 'file', title: 'www.google.com', title_link: '/file-upload/edge456/www-google.com' }This would ensure the fix is properly visible and testable in Storybook.
1175-1226: Consider adding URL-like filename edge cases in reply context.Similar to the standalone file attachments story, this reply-with-file story would benefit from including edge cases where the file title resembles a URL to test the fix in the reply/quote context.
📜 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 (3)
app/containers/UIKit/__snapshots__/UiKitModal.test.tsx.snapis excluded by!**/*.snapapp/containers/markdown/__snapshots__/Markdown.test.tsx.snapis excluded by!**/*.snapapp/containers/message/__snapshots__/Message.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (16)
app/containers/UIKit/index.tsx(1 hunks)app/containers/markdown/Markdown.stories.tsx(1 hunks)app/containers/markdown/index.tsx(1 hunks)app/containers/message/Components/Attachments/Attachments.tsx(3 hunks)app/containers/message/Components/Attachments/Audio.tsx(2 hunks)app/containers/message/Components/Attachments/CollapsibleQuote/index.tsx(1 hunks)app/containers/message/Components/Attachments/Image/Container.tsx(1 hunks)app/containers/message/Components/Attachments/Image/definitions.ts(0 hunks)app/containers/message/Components/Attachments/Reply.tsx(6 hunks)app/containers/message/Components/Attachments/Video.tsx(3 hunks)app/containers/message/Content.tsx(0 hunks)app/containers/message/Message.stories.tsx(3 hunks)app/containers/message/interfaces.ts(1 hunks)app/views/CannedResponseDetail.tsx(1 hunks)app/views/E2EHowItWorksView.tsx(1 hunks)app/views/RoomInfoView/Item.tsx(1 hunks)
💤 Files with no reviewable changes (2)
- app/containers/message/Content.tsx
- app/containers/message/Components/Attachments/Image/definitions.ts
🧰 Additional context used
🧬 Code graph analysis (8)
app/containers/message/Components/Attachments/Attachments.tsx (1)
app/containers/message/interfaces.ts (1)
IMessageAttachments(10-16)
app/containers/message/Components/Attachments/Image/Container.tsx (1)
app/containers/MessageComposer/components/Toolbar/Markdown.tsx (1)
Markdown(9-46)
app/containers/message/Components/Attachments/Audio.tsx (1)
app/containers/message/hooks/useMediaAutoDownload.tsx (1)
useMediaAutoDownload(51-188)
app/views/CannedResponseDetail.tsx (1)
app/containers/MessageComposer/components/Toolbar/Markdown.tsx (1)
Markdown(9-46)
app/containers/UIKit/index.tsx (2)
app/containers/markdown/Markdown.stories.tsx (1)
Text(51-59)app/lib/constants/colors.ts (1)
themes(304-304)
app/containers/message/Message.stories.tsx (1)
ios/RocketChat Watch App/Formatters/MessageFormatter.swift (1)
date(63-74)
app/containers/message/Components/Attachments/Reply.tsx (2)
app/definitions/IAttachment.ts (1)
IAttachment(9-48)app/definitions/IEmoji.ts (1)
TGetCustomEmoji(23-23)
app/containers/message/Components/Attachments/Video.tsx (1)
app/containers/MessageComposer/components/Toolbar/Markdown.tsx (1)
Markdown(9-46)
⏰ 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 (15)
app/views/E2EHowItWorksView.tsx (1)
33-36: LGTM! Consistent Markdown styling removal.All four Markdown components now use default styling. Ensure the removed
infoStylewas not providing critical visual formatting for the E2E help content.app/containers/message/Components/Attachments/Image/Container.tsx (2)
34-40: LGTM! Simplified conditional rendering.The Markdown rendering now consistently uses
usernameandgetCustomEmojiwhenmsgis present, removing the previous style-based conditional path.
12-20: Approve removal ofstyleandisReplyprops. Verification confirms no call sites pass these props.app/views/CannedResponseDetail.tsx (1)
80-80: LGTM! Markdown rendering simplified.The removal of the
itemContentstyle aligns with the broader refactor to standardize Markdown rendering across the codebase.app/containers/message/Components/Attachments/Audio.tsx (2)
11-16: LGTM! Interface simplified consistently.The removal of
styleandisReplyprops aligns with the broader API simplification across attachment components.
24-29: Conditional Markdown rendering aligned across attachments
Audio.tsx now only renders Markdown when msg is truthy, matching the behavior in Video.tsx and Reply.tsx; no changes required.app/containers/markdown/index.tsx (1)
24-35: Removed Markdown props verified – no remaining references
Search across.tsx/.jsxfiles confirmed no usages oftmid,numberOfLines,customEmojis,enableMessageParser,testID, orstyle. Breaking change validated.app/containers/message/interfaces.ts (1)
10-16: Verify removal ofstyleandisReplyin all IMessageAttachments consumers
Automated search found no matches; manually confirm that Attachments.tsx, Image, Audio, and Video components no longer reference these props.app/views/RoomInfoView/Item.tsx (1)
24-24: Confirm Markdown styling change
Default Markdown rendering now uses fontSize 16 (vs previous 14) and italics for empty content. Ensure the larger text size and italic treatment are acceptable for RoomInfo items.app/containers/markdown/Markdown.stories.tsx (1)
127-127: Story update aligns with styling removalSwitching to plain Markdown here matches the new API surface and styling approach.
app/containers/UIKit/index.tsx (1)
64-66: Context blocks: move to MarkdownPreview — good cleanupRemoving theme styling and using MarkdownPreview in context is consistent with the PR changes.
app/containers/message/Components/Attachments/Video.tsx (1)
73-76: Approve prop simplification and msg rendering—msgderives solely fromattachment.description, so no filename linkification risk.app/containers/message/Components/Attachments/Reply.tsx (1)
219-232: Allow file attachments to remain clickable even when message_link is set
Replace the disabled prop in Reply.tsx:-disabled={!!(loading || attachment.message_link)} +disabled={!!( + loading || + (attachment.message_link && !(attachment.type === 'file' && attachment.title_link)) +)}Verify whether any file-type reply attachments include a message_link.
app/containers/message/Message.stories.tsx (2)
1044-1103: LGTM! Consistent large font variant.The large font variant properly mirrors the normal font story structure.
1291-1342: LGTM! Consistent large font variant for replies with files.The large font variant properly mirrors the normal font story structure for reply-with-file scenarios.
| return <MarkdownPreview msg={element.text} numberOfLines={0} />; | ||
| } | ||
| return <Markdown msg={element.text} style={[isContext && { color: themes[theme].fontSecondaryInfo }]} />; | ||
| return <Markdown msg={element.text} />; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
numberOfLines={0} can be invalid in RN Text trees — confirm MarkdownPreview behavior
RN Text expects numberOfLines > 0; passing 0 may be ignored or cause issues if forwarded. If unlimited lines are desired, omit the prop.
Apply this minimal change if safe:
- return <MarkdownPreview msg={element.text} numberOfLines={0} />;
+ return <MarkdownPreview msg={element.text} />;To check other occurrences:
🏁 Script executed:
#!/bin/bash
rg -n "MarkdownPreview.*numberOfLines=\{0\}"Length of output: 323
🏁 Script executed:
#!/bin/bash
# Find the file(s) defining MarkdownPreview
rg -l "MarkdownPreview" -g "*.tsx" | grep -vE "index.tsx|Reply.tsx"
# Show definition and prop types
rg -n "MarkdownPreview" -C5 -g "*.tsx"
# Look for numberOfLines usage within its implementation
rg -n "numberOfLines" -C5 -g "*.tsx"Length of output: 80723
Remove numberOfLines={0} from MarkdownPreview
Delete the prop in all instances to avoid passing 0 to RN Text, which hides content.
Occurrences:
- app/containers/UIKit/index.tsx (plain_text at line 64, mrkdwn at line 72)
- app/containers/message/Components/Attachments/Reply.tsx (line 139)
Use<MarkdownPreview msg={…} />instead.
🤖 Prompt for AI Agents
In app/containers/UIKit/index.tsx around lines 72–75 (and also check the
plain_text at ~line 64) and in
app/containers/message/Components/Attachments/Reply.tsx around line 139, remove
the prop numberOfLines={0} from all <MarkdownPreview> usages so RN Text isn’t
passed 0 which hides content; update each usage to call MarkdownPreview with
only the msg prop (i.e., remove the numberOfLines attribute).
|
Android Build Available Rocket.Chat Experimental 4.66.0.107562 Internal App Sharing: https://play.google.com/apps/test/RQVpXLytHNc/ahAO29uNQJ_NSkLypKZjQQNmS2pxnwgNRqjMBp3tPEngHaR-KNwMbkLUCwg_FKhAch9QVcp7bGho1Vkx5dFMsugXOB |
|
iOS Build Available Rocket.Chat Experimental 4.66.0.107563 |
| tmid?: string; | ||
| numberOfLines?: number; | ||
| customEmojis?: boolean; | ||
| useRealName?: boolean; | ||
| channels?: IUserChannel[]; | ||
| enableMessageParser?: boolean; | ||
| // TODO: Refactor when migrate Room | ||
| navToRoomInfo?: Function; | ||
| testID?: string; | ||
| style?: StyleProp<TextStyle>[]; |
There was a problem hiding this comment.
This is technically out of scope, but I found it and wanted to fix it 🙈
I can open as a separate PR, if you think it's too chaotic.
diegolmello
force-pushed
the
fix.file-name-url
branch
from
c034d18 to
2e693de
Compare
diegolmello
requested a deployment
to
approve_e2e_testing
GitHub Actions
Waiting
diegolmello
requested a deployment
to
experimental_android_build
GitHub Actions
Waiting
diegolmello
requested a deployment
to
official_android_build
GitHub Actions
Waiting
diegolmello
requested a deployment
to
official_ios_build
GitHub Actions
Waiting
diegolmello
requested a deployment
to
experimental_ios_build
GitHub Actions
Waiting
2 participants
Proposed changes
Issue(s)
https://rocketchat.atlassian.net/browse/NATIVE-1049
How to test or reproduce
Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit
New Features
Style
Refactor
Documentation