Feat: Add download button config similar to print config - #3
Open
suparthghimire wants to merge 1 commit into
Open
Feat: Add download button config similar to print config#3suparthghimire wants to merge 1 commit into
suparthghimire wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughAdded a configurable download flag to the viewer config and updated multiple renderers and toolbar components to respect Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/CommonToolbar.tsx`:
- Around line 154-155: The download toggle from CommonToolbar (enableDownload /
enablePrint) is not respected by the DOCX/MSDOC fallback UIs; update the DOCX
and MSDOC renderers so they check the same enableDownload flag before rendering
download links. Specifically, modify the docx renderer component
(src/renderers/docx/index.tsx) and the msdoc renderer component
(src/renderers/msdoc/index.tsx) to accept/consume the enableDownload prop (or
access the same config) and wrap the fallback download link markup in a
conditional that only renders when enableDownload is true; also apply the same
conditional to the other fallback block noted (around the 260-285 area) so all
download links are gated by enableDownload.
- Around line 157-160: The download logic casts currentDocument.fileData to
string which breaks when fileData is an ArrayBuffer; update handleDownload in
CommonToolbar.tsx (and the analogous handlers in PDFControls.tsx) to detect
ArrayBuffer vs string/URI: if fileData is an ArrayBuffer, create a Blob (with
appropriate mime/type if available), call URL.createObjectURL(blob) to use as
the download href, trigger the download with the computed filename, and then
revokeObjectURL; if fileData is a string/URI, proceed with fetch or use the URI
directly; remove the unsafe type cast `(fileData as string)` and ensure filename
fallback uses currentDocument.name or derived name.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5eed4765-56a9-4946-aade-c1098fc4fa2e
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
src/__tests__/index.test.tsxsrc/components/CommonToolbar.tsxsrc/components/ProxyRenderer.tsxsrc/models.tssrc/renderers/pdf/components/PDFControls.tsx
…le download in ui
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/ProxyRenderer.tsx`:
- Around line 149-158: The download attribute currently uses
currentDocument?.uri which causes blob/signed/query URLs to be used as the saved
filename; update the LinkButton (id "no-renderer-download") to use the resolved
filename from the document contents instead — e.g. set the download prop to
currentDocument?.contents?.fileName (or the exact resolved fileName field on
Contents) while keeping href as currentDocument?.uri so the link still points to
the resource.
In `@src/renderers/pdf/components/PDFControls.tsx`:
- Around line 58-83: handlePrint incorrectly assumes currentDocument.fileData is
a string and calls .startsWith(), which breaks when fileData is an ArrayBuffer;
update handlePrint to first branch on the type of currentDocument.fileData: if
it's an ArrayBuffer, create a Blob using currentDocument.fileType ||
"application/pdf", createObjectURL, load that URL into a hidden iframe (or new
window), call print on the iframe/window, then revokeObjectURL; otherwise, only
call .startsWith() after confirming typeof fileData === "string" (or fall back
to currentDocument.uri). Apply the same type-safe ArrayBuffer handling to the
other print handler block referenced (the second handler at the other toolbar
section).
In `@src/stories/features.stories.tsx`:
- Around line 302-324: The two stories render identical states because both
DownloadButton and DownloadButtonDisabled pass download.enableDownload: false to
DocViewer; update DownloadButton to show the enabled/default behavior by either
removing the download property from the config or setting
download.enableDownload: true in the DownloadButton story (leave
DownloadButtonDisabled as-is), ensuring the DocViewer call in DownloadButton
reflects an enabled download state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e7babe41-1eeb-484a-b85f-cc97f85c312d
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
src/__tests__/index.test.tsxsrc/components/CommonToolbar.tsxsrc/components/ProxyRenderer.tsxsrc/models.tssrc/renderers/docx/index.tsxsrc/renderers/msdoc/index.tsxsrc/renderers/pdf/components/PDFControls.tsxsrc/stories/features.stories.tsx
✅ Files skipped from review due to trivial changes (1)
- src/tests/index.test.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/CommonToolbar.tsx
Comment on lines
+149
to
+158
| {enableDownload && ( | ||
| <LinkButton | ||
| id="no-renderer-download" | ||
| className="rdv-download-btn" | ||
| href={currentDocument?.uri} | ||
| download={currentDocument?.uri} | ||
| > | ||
| {t("downloadButtonLabel")} | ||
| </LinkButton> | ||
| )} |
There was a problem hiding this comment.
Use the resolved filename in the no-renderer download link.
download={currentDocument?.uri} makes blob URLs, signed URLs, and query strings show up as the saved filename in this fallback path. Contents already has the resolved fileName, so this button should use that instead.
💡 Proposed fix
{enableDownload && (
<LinkButton
id="no-renderer-download"
className="rdv-download-btn"
href={currentDocument?.uri}
- download={currentDocument?.uri}
+ download={fileName}
>
{t("downloadButtonLabel")}
</LinkButton>
)}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| {enableDownload && ( | |
| <LinkButton | |
| id="no-renderer-download" | |
| className="rdv-download-btn" | |
| href={currentDocument?.uri} | |
| download={currentDocument?.uri} | |
| > | |
| {t("downloadButtonLabel")} | |
| </LinkButton> | |
| )} | |
| {enableDownload && ( | |
| <LinkButton | |
| id="no-renderer-download" | |
| className="rdv-download-btn" | |
| href={currentDocument?.uri} | |
| download={fileName} | |
| > | |
| {t("downloadButtonLabel")} | |
| </LinkButton> | |
| )} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ProxyRenderer.tsx` around lines 149 - 158, The download
attribute currently uses currentDocument?.uri which causes blob/signed/query
URLs to be used as the saved filename; update the LinkButton (id
"no-renderer-download") to use the resolved filename from the document contents
instead — e.g. set the download prop to currentDocument?.contents?.fileName (or
the exact resolved fileName field on Contents) while keeping href as
currentDocument?.uri so the link still points to the resource.
Comment on lines
58
to
83
| const handleDownload = useCallback(() => { | ||
| const url = currentDocument?.fileData as string; | ||
| const name = currentDocument?.fileName || currentDocument?.uri || "download"; | ||
| if (!currentDocument) return; | ||
|
|
||
| const fileData = currentDocument.fileData; | ||
| const name = | ||
| currentDocument.fileName || | ||
| currentDocument.uri?.split("/").pop() || | ||
| "download"; | ||
|
|
||
| // Handle ArrayBuffer fileData | ||
| if (fileData instanceof ArrayBuffer) { | ||
| const mimeType = currentDocument.fileType || "application/pdf"; | ||
| const blob = new Blob([fileData], { type: mimeType }); | ||
| const blobUrl = URL.createObjectURL(blob); | ||
| const a = document.createElement("a"); | ||
| a.href = blobUrl; | ||
| a.download = name; | ||
| a.click(); | ||
| URL.revokeObjectURL(blobUrl); | ||
| return; | ||
| } | ||
|
|
||
| // Handle string (data URL or regular URL) or fall back to URI | ||
| const url = | ||
| (typeof fileData === "string" ? fileData : null) || currentDocument.uri; | ||
| if (!url) return; |
There was a problem hiding this comment.
ArrayBuffer-backed PDFs still expose a broken print action.
handleDownload now supports currentDocument.fileData as an ArrayBuffer, but Line 104 in handlePrint still treats fileData as a string and calls .startsWith(). Because this toolbar section renders whenever any truthy fileData exists, enablePrint: true on an ArrayBuffer document falls into the catch path and prints the page chrome instead of the PDF.
🛠️ Proposed fix
- const handlePrint = useCallback(async () => {
- const fileData = currentDocument?.fileData as string | undefined;
+ const handlePrint = useCallback(async () => {
+ const fileData = currentDocument?.fileData;
if (!fileData) return;
let blobUrl: string | undefined;
try {
let blob: Blob;
- if (fileData.startsWith("data:")) {
- const res = await fetch(fileData);
- blob = await res.blob();
- } else {
- const res = await fetch(fileData);
- blob = await res.blob();
- }
+ if (fileData instanceof ArrayBuffer) {
+ blob = new Blob([fileData], {
+ type: currentDocument?.fileType || "application/pdf",
+ });
+ } else {
+ const res = await fetch(fileData);
+ blob = await res.blob();
+ }Also applies to: 157-184
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderers/pdf/components/PDFControls.tsx` around lines 58 - 83,
handlePrint incorrectly assumes currentDocument.fileData is a string and calls
.startsWith(), which breaks when fileData is an ArrayBuffer; update handlePrint
to first branch on the type of currentDocument.fileData: if it's an ArrayBuffer,
create a Blob using currentDocument.fileType || "application/pdf",
createObjectURL, load that URL into a hidden iframe (or new window), call print
on the iframe/window, then revokeObjectURL; otherwise, only call .startsWith()
after confirming typeof fileData === "string" (or fall back to
currentDocument.uri). Apply the same type-safe ArrayBuffer handling to the other
print handler block referenced (the second handler at the other toolbar
section).
Comment on lines
+302
to
+324
| export const DownloadButton = () => ( | ||
| <div style={{ height: "100vh" }}> | ||
| <DocViewer | ||
| documents={[{ uri: pdfMultiplePagesFile }]} | ||
| config={{ | ||
| download: { enableDownload: false }, | ||
| pdfVerticalScrollByDefault: false, | ||
| }} | ||
| /> | ||
| </div> | ||
| ); | ||
|
|
||
| export const DownloadButtonDisabled = () => ( | ||
| <div style={{ height: "100vh" }}> | ||
| <DocViewer | ||
| documents={[{ uri: pdfMultiplePagesFile }]} | ||
| config={{ | ||
| download: { enableDownload: false }, | ||
| pdfVerticalScrollByDefault: false, | ||
| }} | ||
| /> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
DownloadButton and DownloadButtonDisabled currently show the same state.
Both stories set download.enableDownload: false, so Storybook never exposes the enabled/default behavior introduced by this PR. DownloadButton should enable the flag or omit it entirely.
💡 Proposed fix
export const DownloadButton = () => (
<div style={{ height: "100vh" }}>
<DocViewer
documents={[{ uri: pdfMultiplePagesFile }]}
config={{
- download: { enableDownload: false },
pdfVerticalScrollByDefault: false,
}}
/>
</div>
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export const DownloadButton = () => ( | |
| <div style={{ height: "100vh" }}> | |
| <DocViewer | |
| documents={[{ uri: pdfMultiplePagesFile }]} | |
| config={{ | |
| download: { enableDownload: false }, | |
| pdfVerticalScrollByDefault: false, | |
| }} | |
| /> | |
| </div> | |
| ); | |
| export const DownloadButtonDisabled = () => ( | |
| <div style={{ height: "100vh" }}> | |
| <DocViewer | |
| documents={[{ uri: pdfMultiplePagesFile }]} | |
| config={{ | |
| download: { enableDownload: false }, | |
| pdfVerticalScrollByDefault: false, | |
| }} | |
| /> | |
| </div> | |
| ); | |
| export const DownloadButton = () => ( | |
| <div style={{ height: "100vh" }}> | |
| <DocViewer | |
| documents={[{ uri: pdfMultiplePagesFile }]} | |
| config={{ | |
| pdfVerticalScrollByDefault: false, | |
| }} | |
| /> | |
| </div> | |
| ); | |
| export const DownloadButtonDisabled = () => ( | |
| <div style={{ height: "100vh" }}> | |
| <DocViewer | |
| documents={[{ uri: pdfMultiplePagesFile }]} | |
| config={{ | |
| download: { enableDownload: false }, | |
| pdfVerticalScrollByDefault: false, | |
| }} | |
| /> | |
| </div> | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/stories/features.stories.tsx` around lines 302 - 324, The two stories
render identical states because both DownloadButton and DownloadButtonDisabled
pass download.enableDownload: false to DocViewer; update DownloadButton to show
the enabled/default behavior by either removing the download property from the
config or setting download.enableDownload: true in the DownloadButton story
(leave DownloadButtonDisabled as-is), ensuring the DocViewer call in
DownloadButton reflects an enabled download state.
mehuljariwala
requested changes
Apr 13, 2026
mehuljariwala
left a comment
mehuljariwala
left a comment
Owner
There was a problem hiding this comment.
Please fix all code-rabbit comment
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds
config.download.enableDownloadinIConfigto enable or disable download buttonScreenshot
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Docs