feat: Added-pdf-preview-feature#38674
feat: Added-pdf-preview-feature#38674KumarHarshit3603 wants to merge 5 commits intoRocketChat:developfrom
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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a modal-based PDF preview flow: introduces Changes
Sequence DiagramsequenceDiagram
actor User
participant Attachment as GenericFileAttachment
participant ModalSvc as useSetModal
participant PdfModal as PdfPreviewModal
participant Desktop as DesktopViewer
User->>Attachment: Click file title
alt file-decrypt link
Attachment->>User: register UID & trigger download (forAttachmentDownload)
else PDF
Attachment->>Attachment: build previewUrl & downloadUrl
Attachment->>ModalSvc: setModal(PdfPreviewModal{title,url,downloadUrl,onClose,onOpenInApp})
ModalSvc->>PdfModal: render with props
PdfModal->>User: display iframe (url)
User->>PdfModal: Click action
alt Close
PdfModal->>ModalSvc: onClose -> clear modal
else Download
PdfModal->>User: navigate to downloadUrl
else Open in App
PdfModal->>Desktop: request openDocumentViewer(downloadUrl or url)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
`@apps/meteor/client/components/message/content/attachments/file/GenericFileAttachment.tsx`:
- Around line 63-65: The modal remains visible after launching the desktop
viewer because openInApp (defined where openDocumentViewer is wrapped) only
calls openDocumentViewer; update that wrapper so after calling
openDocumentViewer(previewUrl.toString(), format, '') it also dismisses the
attachment modal by invoking the component's close handler (e.g., onClose,
dismissModal, or the prop that toggles the modal visibility) — ensure you check
that the close handler exists before calling and preserve error handling (call
close after successful launch or in a finally block) so the modal is always
closed when opening in the external viewer.
In
`@apps/meteor/client/components/message/content/attachments/file/PdfPreviewModal.tsx`:
- Line 30: The component PdfPreviewModal uses raw English fallbacks ('PDF' and
'PDF preview') for ModalTitle (and the subtitle/label on line 34), which
bypasses i18n; replace those hardcoded fallbacks by passing them through the
translation function (t) so the fallback becomes t('PDF') and t('PDF preview')
(or their existing translation keys) when title or the subtitle prop is absent,
updating usages in PdfPreviewModal (ModalTitle and the subtitle element) to call
t(...) instead of raw strings.
- Line 34: PdfPreviewModal renders a user-uploaded URL in an iframe without
sandboxing; add a restrictive sandbox attribute on the iframe element in
PdfPreviewModal (the line with <iframe src={url} ... />) to limit
capabilities—use a minimal set of allowances required for your PDF renderer (for
example include only "allow-scripts allow-same-origin" if the viewer needs them,
or an empty sandbox if not) so the embedded document cannot perform navigation
or broader scripting actions.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/meteor/client/components/message/content/attachments/file/GenericFileAttachment.tsxapps/meteor/client/components/message/content/attachments/file/PdfPreviewModal.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/client/components/message/content/attachments/file/PdfPreviewModal.tsxapps/meteor/client/components/message/content/attachments/file/GenericFileAttachment.tsx
🔇 Additional comments (2)
apps/meteor/client/components/message/content/attachments/file/GenericFileAttachment.tsx (2)
57-58:getURL(link)returns a full absolute URL; the code is correct.When
useMediaUrl()is called, it providesgetURLfromAttachmentProvider, which wraps the utility function with{ full: true }. This option makesgetURLreturn a full absolute URL (e.g.,https://host.com/path/to/file) by concatenating the Site_Url setting with the relative path. Thenew URL(absoluteUrl, base)constructor ignores the base argument when given an absolute URL, so this pattern works as intended.
54-54: Useformat?.toLowerCase() === 'pdf'instead of case-sensitive'PDF'check.Sharp's
metadata().formatreturns lowercase format names. Since PDFs arrive asformat: 'pdf', the current case-sensitive checkformat === 'PDF'will never match, preventing the PDF preview modal from opening.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
apps/meteor/client/components/message/content/attachments/file/GenericFileAttachment.tsx
Show resolved
Hide resolved
apps/meteor/client/components/message/content/attachments/file/PdfPreviewModal.tsx
Outdated
Show resolved
Hide resolved
apps/meteor/client/components/message/content/attachments/file/PdfPreviewModal.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@apps/meteor/client/components/message/content/attachments/file/PdfPreviewModal.tsx`:
- Line 34: The iframe in PdfPreviewModal currently sets
sandbox='allow-same-origin' which prevents native PDF rendering; remove the
sandbox attribute from the iframe element (<iframe src={url} style={iframeStyle}
title={title || t('PDF_preview')} />) to restore browser PDF viewers, or
alternatively replace the iframe approach with a JS-based renderer such as
PDF.js inside PdfPreviewModal if sandboxing must be preserved.
🧹 Nitpick comments (2)
apps/meteor/client/components/message/content/attachments/file/PdfPreviewModal.tsx (2)
14-20: Consider addingaria-labelor accessible description to the modal for screen readers.The modal's accessibility relies on
ModalTitlefor labeling. This is acceptable if Fuselage'sModalcomponent automatically associates the title viaaria-labelledby. No action needed if that's the case, but worth confirming.
38-41: Download button does not specify a filename in thedownloadattribute.The
downloadboolean attribute will let the browser pick the filename from the URL, which may be an opaque ID rather than the original filename. Passingtitlewould preserve the original name.Proposed fix
- <Button is='a' href={downloadUrl} download secondary> + <Button is='a' href={downloadUrl} download={title || true} secondary>
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/meteor/client/components/message/content/attachments/file/GenericFileAttachment.tsxapps/meteor/client/components/message/content/attachments/file/PdfPreviewModal.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/client/components/message/content/attachments/file/PdfPreviewModal.tsxapps/meteor/client/components/message/content/attachments/file/GenericFileAttachment.tsx
🧬 Code graph analysis (2)
apps/meteor/client/components/message/content/attachments/file/PdfPreviewModal.tsx (1)
packages/ui-contexts/src/index.ts (1)
useTranslation(81-81)
apps/meteor/client/components/message/content/attachments/file/GenericFileAttachment.tsx (2)
packages/ui-contexts/src/index.ts (1)
useSetModal(71-71)packages/message-parser/src/utils.ts (1)
link(77-80)
⏰ 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: cubic · AI code reviewer
🔇 Additional comments (3)
apps/meteor/client/components/message/content/attachments/file/GenericFileAttachment.tsx (3)
46-52: Encrypted PDF links skip the preview modal — confirm this is intentional.The
/file-decrypt/check (line 46) precedes theformat === 'PDF'check (line 54), so encrypted PDFs bypass the preview and go straight to the download flow. If preview of decrypted PDFs is desired in the future, this ordering would need adjustment. Just flagging for awareness; the current behavior matches the PR objectives.
9-9: Modal integration looks well-structured.The wiring of
useSetModal,PdfPreviewModal, and the close/open-in-app handlers is clean. The previously flagged issue (modal not closing on "Open in app") is addressed in theopenInAppcallback (lines 64-67). The lifecycle management viasetModal(null)is correct.Also applies to: 20-20, 37-37, 62-78
57-58: The server properly handles thecontentDispositionquery parameter. The implementation inapps/meteor/app/file-upload/server/config/helper.tsexplicitly checks forquery.contentDisposition === 'inline'and returns the corresponding disposition. The fallback to'attachment'is intentional. No action required.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
apps/meteor/client/components/message/content/attachments/file/PdfPreviewModal.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@apps/meteor/client/components/message/content/attachments/file/PdfPreviewModal.tsx`:
- Around line 38-41: The download link in PdfPreviewModal (the <Button is='a'
href={downloadUrl} download ... />) can point to a cross-origin URL returned by
GenericFileAttachment.getExternalUrl/getURL when openDocumentViewer is
unavailable, which makes the download attribute ignored; update the logic that
produces downloadUrl (or getExternalUrl/getURL call path) to ensure same-origin
URLs by wrapping external hrefs with new URL(href, window.location.origin)
before assigning to download, and for truly cross-origin resources implement a
programmatic fallback: fetch the resource, create a blob/object URL and set that
as the download href (or trigger download) so clicking the Button triggers a
download instead of navigation; reference getExternalUrl, getURL,
openDocumentViewer, and PdfPreviewModal/Button when making the change.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/meteor/client/components/message/content/attachments/file/PdfPreviewModal.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/client/components/message/content/attachments/file/PdfPreviewModal.tsx
🧬 Code graph analysis (1)
apps/meteor/client/components/message/content/attachments/file/PdfPreviewModal.tsx (1)
packages/ui-contexts/src/index.ts (1)
useTranslation(81-81)
⏰ 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: cubic · AI code reviewer
🔇 Additional comments (2)
apps/meteor/client/components/message/content/attachments/file/PdfPreviewModal.tsx (2)
1-22: Imports, types, and style constant look good.Clean setup with proper typing and no unnecessary dependencies.
24-54: Component is well-structured overall.The modal layout, conditional rendering of footer buttons, and i18n usage are all clean. The past review items (sandbox removal, i18n fallbacks) have been addressed.
One small note: there is no loading or error state for the iframe. The PR description acknowledges this as a possible future enhancement, which seems reasonable for an initial implementation.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
apps/meteor/client/components/message/content/attachments/file/PdfPreviewModal.tsx
Show resolved
Hide resolved
ggazzo
added
the
valid
coderabbitai
bot
added
the
type: feature
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/client/components/message/content/attachments/file/GenericFileAttachment.tsx (1)
84-96: Remove duplicate code ingetExternalUrl.Both branches of the
if (openDocumentViewer)conditional perform identical operations. The check is redundant.♻️ Proposed simplification
const getExternalUrl = () => { if (!hasDownload || !link) return undefined; - if (openDocumentViewer) { - const url = new URL(getURL(link), window.location.origin); - url.searchParams.set('download', ''); - return url.toString(); - } - const url = new URL(getURL(link), window.location.origin); url.searchParams.set('download', ''); return url.toString(); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/components/message/content/attachments/file/GenericFileAttachment.tsx` around lines 84 - 96, The getExternalUrl function contains a redundant if (openDocumentViewer) branch that duplicates the same URL construction and download param logic; simplify by removing the conditional and always return the constructed URL when hasDownload and link are truthy. Update getExternalUrl (referencing hasDownload, link, openDocumentViewer, and getURL) to early-return undefined if !hasDownload || !link, then create the URL from getURL(link), set searchParams 'download', and return url.toString().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/meteor/client/components/message/content/attachments/file/GenericFileAttachment.tsx`:
- Around line 84-96: The getExternalUrl function contains a redundant if
(openDocumentViewer) branch that duplicates the same URL construction and
download param logic; simplify by removing the conditional and always return the
constructed URL when hasDownload and link are truthy. Update getExternalUrl
(referencing hasDownload, link, openDocumentViewer, and getURL) to early-return
undefined if !hasDownload || !link, then create the URL from getURL(link), set
searchParams 'download', and return url.toString().
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/meteor/client/components/message/content/attachments/file/GenericFileAttachment.tsx
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/client/components/message/content/attachments/file/GenericFileAttachment.tsx
🔇 Additional comments (3)
apps/meteor/client/components/message/content/attachments/file/GenericFileAttachment.tsx (3)
9-9: LGTM!New imports for
useSetModalandPdfPreviewModalare appropriate for the PDF preview modal feature.Also applies to: 20-20
54-80: LGTM!PDF preview flow is well-implemented:
- Correctly prevents default event behavior
- Constructs preview URL with
contentDisposition=inlinefor iframe embedding- Conditionally provides download URL based on
hasDownload- The
openInApphandler now properly closes the modal after launching the native viewer (previous feedback addressed)
98-118: LGTM!The JSX structure correctly integrates the
handleTitleClickhandler with theMessageGenericPreviewTitlecomponent.
2 participants
Proposed changes (including videos or screenshots)
PdfPreviewModalthat embeds PDFs inside a Fuselage modal via iframe, with download and “open in app” controls.GenericFileAttachmentto open the modal on PDF clicks while preserving decrypt/download flows and desktop viewer support.Issue(s)
Steps to test or reproduce
openDocumentVieweris available./file-decrypt/) links still trigger the registered download flow.Further comments
14.02.2026_06.18.01_REC.mp4
Summary by CodeRabbit