fix: quote image not loading#6698
Conversation
|
Warning Rate limit exceeded@OtavioStasiak has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 22 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
WalkthroughTightened quote-attachment detection in Quote.tsx. Added original URL extraction in useMediaAutoDownload and passed it to formatAttachmentUrl. Extended formatAttachmentUrl to accept an optional original URL and short-circuit for non-server URLs while preserving existing handling for protected files and special schemes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor UI as UI
participant Hook as useMediaAutoDownload
participant Formatter as formatAttachmentUrl
participant Server as Current Server
UI->>Hook: Render with file
Hook->>Hook: getOriginalURL(file) → originalUrl
Hook->>Formatter: formatAttachmentUrl(url, user, server, settings, originalUrl)
alt originalUrl provided and not starting with Server
Formatter-->>Hook: return originalUrl (short-circuit)
else URL requires processing
opt http(s) with rc_token
Formatter->>Formatter: encode with token
end
opt protected files enabled
Formatter->>Formatter: setParamInUrl
end
Formatter-->>Hook: return processed URL
end
Hook-->>UI: Final URL for media
sequenceDiagram
autonumber
actor Renderer as Message Renderer
participant Quote as Quote.tsx
Renderer->>Quote: Render attachment
Quote->>Quote: Check image/audio/video/collapsed
alt color & text both absent AND media/collapsed present
Quote-->>Renderer: Not a quote
else
Quote-->>Renderer: Treat as quote
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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
🧹 Nitpick comments (1)
app/containers/message/hooks/useMediaAutoDownload.tsx (1)
38-49: Consider consolidating URL extraction logic.
getOriginalURLduplicates the priority order already established ingetFileType(image → video → audio). This introduces maintenance overhead if the priority order ever changes.Consider refactoring to reduce duplication:
+const getOriginalURL = (file: IAttachment, fileType: MediaTypes | null): string | null => { + if (!fileType) return null; + return file[`${fileType}_url`] || null; +};Then update the call site:
- const originalUrl = getOriginalURL(file); + const originalUrl = getOriginalURL(file, fileType);This keeps the priority order in one place (
getFileType) and reduces code duplication.
📜 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 (3)
app/containers/message/Components/Attachments/Quote.tsx(1 hunks)app/containers/message/hooks/useMediaAutoDownload.tsx(2 hunks)app/lib/methods/helpers/formatAttachmentUrl.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app/lib/methods/helpers/formatAttachmentUrl.ts (2)
app/lib/store/auxStore.ts (1)
store(6-6)app/lib/methods/isImageBase64.ts (1)
isImageBase64(3-8)
app/containers/message/hooks/useMediaAutoDownload.tsx (2)
app/definitions/IAttachment.ts (1)
IAttachment(9-48)app/lib/methods/helpers/formatAttachmentUrl.ts (1)
formatAttachmentUrl(13-44)
🔇 Additional comments (3)
app/lib/methods/helpers/formatAttachmentUrl.ts (1)
13-19: LGTM: Parameter formatting improved.The multi-line parameter formatting enhances readability.
app/containers/message/hooks/useMediaAutoDownload.tsx (1)
64-71: LGTM: Original URL passed to formatAttachmentUrl.The
originalUrlis correctly extracted and passed toformatAttachmentUrl, enabling the new URL handling logic.However, ensure that the guard in
formatAttachmentUrl(line 26-28) handles cases wherefile.title_linkis a relative path. Iftitle_linkis relative, the guard won't execute, andoriginalUrlwon't be used. Verify this is the intended behavior.app/containers/message/Components/Attachments/Quote.tsx (1)
14-14: LGTM: Tightened quote attachment criteria.The updated predicate now correctly allows attachments with
image_url,audio_url, orvideo_urlto be treated as quotes when they also havecolorortext. This aligns with the PR objective to fix quote images not loading and ensures quote attachments with images render as expected.The old logic unconditionally excluded attachments with media URLs from quotes, which caused the reported bug. The new logic fixes this by only excluding them when both
colorandtextare absent.
* fix: quote image not loading * chore: snapshot tests
Proposed changes
Fix quote image not loading and message quote with image not rendering as expected.
Issue(s)
https://rocketchat.atlassian.net/browse/SUP-879
How to test or reproduce
chat.sendMessagewith the object;Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit