Merged
Conversation
- Update preview-data API to return Office Online embed URL for sheets with advancedExcelEnabled - Add DocumentPreviewData.advancedExcelEnabled field - Create PreviewExcelViewer component using Office Online iframe - Update PreviewViewer to render Excel advanced mode preview Co-authored-by: Marc Seitz <mfts@users.noreply.github.com>
…ent detail page - Add advancedExcelEnabled prop to DocumentPreviewButton - Pass advancedExcelEnabled from document to preview button on detail page Co-authored-by: Marc Seitz <mfts@users.noreply.github.com>
- Include advancedExcelEnabled in dataroom documents API responses - Add advancedExcelEnabled to DataroomFolderDocument type - Pass advancedExcelEnabled to DocumentPreviewButton in dataroom card Co-authored-by: Marc Seitz <mfts@users.noreply.github.com>
|
Cursor Agent can help with this pull request. Just |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
WalkthroughAdds optional advancedExcelEnabled flag across types, APIs, and UI; introduces PreviewExcelViewer (Office Online embed) and conditions preview rendering for sheet documents when the flag is true; wires the flag through dataroom document payloads to DocumentPreviewButton. Changes
Possibly related PRs
🚥 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. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
components/documents/preview-viewers/preview-excel-viewer.tsx (2)
6-14: UnusedonCloseprop.The
onCloseprop is declared in the interface and destructured but never used within the component. If this is intentional for interface consistency with other preview viewers, consider documenting it. Otherwise, remove it to avoid confusion.♻️ Option: Remove unused prop if not needed for interface consistency
interface PreviewExcelViewerProps { documentData: DocumentPreviewData; - onClose: () => void; } export function PreviewExcelViewer({ documentData, - onClose, }: PreviewExcelViewerProps) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/documents/preview-viewers/preview-excel-viewer.tsx` around lines 6 - 14, The PreviewExcelViewer component declares and destructures the onClose prop in PreviewExcelViewerProps and the PreviewExcelViewer function but never uses it; either remove onClose from the interface and the component parameter list (and update any callers to stop passing onClose) or, if you intend to keep it for consistency, leave the prop but add a short comment in PreviewExcelViewer explaining it is intentionally unused; search for PreviewExcelViewer and PreviewExcelViewerProps to update the type/signature and all call sites accordingly.
47-54: Add accessibility and security attributes to the iframe.The iframe is missing a
titleattribute, which is important for accessibility (screen readers use it to describe the frame content). Consider also addingreferrerPolicyto control referrer information sent to Office Online.♿ Proposed fix to add accessibility and security attributes
<iframe className={cn( "h-full w-full rounded-md transition-opacity duration-200", iframeLoaded ? "opacity-100" : "opacity-0", )} src={embedUrl} + title={`Excel preview: ${documentData.documentName}`} + referrerPolicy="no-referrer-when-downgrade" onLoad={() => setIframeLoaded(true)} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/documents/preview-viewers/preview-excel-viewer.tsx` around lines 47 - 54, The iframe lacks accessibility and referrer controls; update the iframe JSX (the element using embedUrl and iframeLoaded state) to include a descriptive title prop (e.g., title={`Preview: ${fileName || 'document'}`} or a static "Document preview") for screen readers and add a referrerPolicy attribute (e.g., referrerPolicy="no-referrer" or "strict-origin-when-cross-origin") to limit referrer information sent to Office Online.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pages/api/teams/`[teamId]/documents/[id]/preview-data.ts:
- Around line 133-135: Guard construction of the preview URL by checking
storageConfig.advancedDistributionHost before building the string: in the
handler where getTeamStorageConfigById(document.teamId) is called, only set
returnData.file =
`https://${storageConfig.advancedDistributionHost}/${primaryVersion.file}` when
storageConfig?.advancedDistributionHost is truthy; otherwise fall back to a safe
alternative (e.g., leave returnData.file as primaryVersion.file or construct
using the standard storage host) to avoid producing "https://undefined/..." and
breaking previews.
- Around line 139-143: DocumentCard opens the preview modal without checking
advancedExcelEnabled, causing the /preview-data endpoint to return 400 for
non-advanced sheets; update DocumentCard to mirror DocumentPreviewButton's
guard: before opening the preview modal (the handler that triggers the preview
dropdown action), check the document.advancedExcelEnabled flag and disable/hide
the "Quick preview" action or show the same UI path as DocumentPreviewButton
when advancedExcelEnabled is false. Alternatively (if preferred), change the
/preview-data endpoint handler to return 200 with a payload indicating
advancedExcelEnabled:false instead of 400 so PreviewViewer can render its inline
fallback; reference DocumentCard, DocumentPreviewButton, and the /preview-data
preview handler when making the change.
---
Nitpick comments:
In `@components/documents/preview-viewers/preview-excel-viewer.tsx`:
- Around line 6-14: The PreviewExcelViewer component declares and destructures
the onClose prop in PreviewExcelViewerProps and the PreviewExcelViewer function
but never uses it; either remove onClose from the interface and the component
parameter list (and update any callers to stop passing onClose) or, if you
intend to keep it for consistency, leave the prop but add a short comment in
PreviewExcelViewer explaining it is intentionally unused; search for
PreviewExcelViewer and PreviewExcelViewerProps to update the type/signature and
all call sites accordingly.
- Around line 47-54: The iframe lacks accessibility and referrer controls;
update the iframe JSX (the element using embedUrl and iframeLoaded state) to
include a descriptive title prop (e.g., title={`Preview: ${fileName ||
'document'}`} or a static "Document preview") for screen readers and add a
referrerPolicy attribute (e.g., referrerPolicy="no-referrer" or
"strict-origin-when-cross-origin") to limit referrer information sent to Office
Online.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bc37fa34-3cfc-4ed8-b6f7-6ca8ab4237b1
📒 Files selected for processing (11)
components/datarooms/dataroom-document-card.tsxcomponents/documents/document-preview-button.tsxcomponents/documents/preview-viewers/index.tscomponents/documents/preview-viewers/preview-excel-viewer.tsxcomponents/documents/preview-viewers/preview-viewer.tsxlib/swr/use-dataroom.tslib/types/document-preview.tspages/api/teams/[teamId]/datarooms/[id]/documents/index.tspages/api/teams/[teamId]/datarooms/[id]/folders/documents/[...name].tspages/api/teams/[teamId]/documents/[id]/preview-data.tspages/documents/[id]/index.tsx
Comment on lines
+133
to
+135
| const storageConfig = await getTeamStorageConfigById(document.teamId); | ||
| returnData.file = `https://${storageConfig.advancedDistributionHost}/${primaryVersion.file}`; | ||
| } else { |
Contributor
There was a problem hiding this comment.
Guard advancedDistributionHost before building the preview URL.
getTeamStorageConfigById() returns a config where advancedDistributionHost is optional, so this can emit https://undefined/... and silently break advanced Excel previews for teams without that distribution configured.
Suggested fix
if (!primaryVersion.file.includes("https://")) {
const storageConfig = await getTeamStorageConfigById(document.teamId);
+ if (!storageConfig.advancedDistributionHost) {
+ return res.status(503).json({
+ message:
+ "Advanced Excel preview is not configured for this team.",
+ });
+ }
returnData.file = `https://${storageConfig.advancedDistributionHost}/${primaryVersion.file}`;
} else {📝 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
| const storageConfig = await getTeamStorageConfigById(document.teamId); | |
| returnData.file = `https://${storageConfig.advancedDistributionHost}/${primaryVersion.file}`; | |
| } else { | |
| const storageConfig = await getTeamStorageConfigById(document.teamId); | |
| if (!storageConfig.advancedDistributionHost) { | |
| return res.status(503).json({ | |
| message: | |
| "Advanced Excel preview is not configured for this team.", | |
| }); | |
| } | |
| returnData.file = `https://${storageConfig.advancedDistributionHost}/${primaryVersion.file}`; | |
| } else { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pages/api/teams/`[teamId]/documents/[id]/preview-data.ts around lines 133 -
135, Guard construction of the preview URL by checking
storageConfig.advancedDistributionHost before building the string: in the
handler where getTeamStorageConfigById(document.teamId) is called, only set
returnData.file =
`https://${storageConfig.advancedDistributionHost}/${primaryVersion.file}` when
storageConfig?.advancedDistributionHost is truthy; otherwise fall back to a safe
alternative (e.g., leave returnData.file as primaryVersion.file or construct
using the standard storage host) to avoid producing "https://undefined/..." and
breaking previews.
Comment on lines
+139
to
+143
| } else { | ||
| return res.status(400).json({ | ||
| message: | ||
| "Sheet preview requires advanced Excel mode. Enable it from the document settings.", | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="$(fd 'document-preview-button\.tsx$' | head -n1)"
echo "Inspecting: $file"
sed -n '1,260p' "$file"
echo
echo "--- preview-data / sheet handling ---"
rg -n -C3 'preview-data|advancedExcelEnabled|sheet|setOpen|onClick|fetch\(' "$file"Repository: mfts/papermark
Length of output: 4487
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== DocumentPreviewModal implementation ==="
fd 'document-preview-modal\.tsx$' -x cat -n {} | head -100
echo
echo "=== PreviewViewer fallback for sheets ==="
fd 'preview-viewer\.tsx$' -x cat -n {} | grep -A 10 -B 5 'sheet\|Enable advanced'
echo
echo "=== Check where preview-data endpoint is called ==="
rg 'preview-data' --type tsx --type ts -n | head -20Repository: mfts/papermark
Length of output: 4678
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== useDocumentPreview hook implementation ==="
fd 'use-document-preview' -x cat -n {} | head -80
echo
echo "=== Search for /preview-data calls ==="
rg '/preview-data|preview-data' -n --type-list | head -5
rg 'preview-data' -n | head -30Repository: mfts/papermark
Length of output: 1551
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== All references to preview-data ==="
rg 'preview-data' -n
echo
echo "=== Check if there are other places opening preview modal ==="
rg 'DocumentPreviewModal|setIsPreviewOpen' -n --max-count=30
echo
echo "=== Verify the guard check in document-preview-button ==="
rg 'supportsPreview|return null' -B3 -A3 components/documents/document-preview-button.tsxRepository: mfts/papermark
Length of output: 1717
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== document-card.tsx preview modal usage ==="
rg -n -B 20 -A 5 'DocumentPreviewModal' components/documents/document-card.tsx | grep -A 25 'line 508'
echo
echo "=== Check document-card supportsPreview logic ==="
sed -n '400,520p' components/documents/document-card.tsx
echo
echo "=== Check if document-card has any conditions on opening preview ==="
rg -n 'setOpen|onClick' components/documents/document-card.tsx | head -20Repository: mfts/papermark
Length of output: 103
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== File size of document-card.tsx ==="
wc -l components/documents/document-card.tsx
echo
echo "=== Context around DocumentPreviewModal in document-card.tsx ==="
cat -n components/documents/document-card.tsx | sed -n '480,530p'
echo
echo "=== Check isPreviewOpen state and handlers in document-card ==="
rg -n 'isPreviewOpen|setIsPreviewOpen|handlePreview' components/documents/document-card.tsx | head -20Repository: mfts/papermark
Length of output: 1390
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for where previewOpen is set to true ==="
rg -n 'previewOpen|setPreviewOpen' components/documents/document-card.tsx
echo
echo "=== Get full context around preview opening logic ==="
cat -n components/documents/document-card.tsx | sed -n '1,150p'Repository: mfts/papermark
Length of output: 6508
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Context around line 427 where setPreviewOpen(true) is called ==="
cat -n components/documents/document-card.tsx | sed -n '400,450p'Repository: mfts/papermark
Length of output: 2908
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Preview-data endpoint implementation ==="
cat pages/api/teams/[teamId]/documents/[id]/preview-data.tsRepository: mfts/papermark
Length of output: 5411
DocumentCard's "Quick preview" can trigger the 400 error for non-advanced sheets.
DocumentCard (line 427) opens the preview modal without checking advancedExcelEnabled, unlike DocumentPreviewButton. When a non-advanced sheet is previewed via the card's dropdown menu, the /preview-data endpoint returns HTTP 400, showing an error state in the modal instead of PreviewViewer's inline fallback UI (which requires documentData to exist).
Either guard the preview action in DocumentCard to match DocumentPreviewButton's logic, or change the endpoint to return 200 with the sheet flag so the fallback UI can render.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pages/api/teams/`[teamId]/documents/[id]/preview-data.ts around lines 139 -
143, DocumentCard opens the preview modal without checking advancedExcelEnabled,
causing the /preview-data endpoint to return 400 for non-advanced sheets; update
DocumentCard to mirror DocumentPreviewButton's guard: before opening the preview
modal (the handler that triggers the preview dropdown action), check the
document.advancedExcelEnabled flag and disable/hide the "Quick preview" action
or show the same UI path as DocumentPreviewButton when advancedExcelEnabled is
false. Alternatively (if preferred), change the /preview-data endpoint handler
to return 200 with a payload indicating advancedExcelEnabled:false instead of
400 so PreviewViewer can render its inline fallback; reference DocumentCard,
DocumentPreviewButton, and the /preview-data preview handler when making the
change.
…ne fallback DocumentCard opens the preview modal without checking advancedExcelEnabled, which caused the /preview-data endpoint to return 400 for non-advanced sheets, showing a red error in the modal. Instead, return 200 with the base payload (advancedExcelEnabled: false, no file) so PreviewViewer can render its existing inline fallback message. Co-authored-by: Marc Seitz <mfts@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pages/api/teams/[teamId]/documents/[id]/preview-data.ts (1)
133-135:⚠️ Potential issue | 🟠 MajorGuard
advancedDistributionHostbefore building the sheet preview URL.Line 134 can still emit
https://undefined/...becausegetTeamStorageConfigById()returns a config whereadvancedDistributionHostis optional. That silently breaks advanced Excel previews for teams without this distribution configured.Suggested fix
if (document.advancedExcelEnabled) { // Advanced Excel mode: use Office Online viewer URL if (!primaryVersion.file.includes("https://")) { const storageConfig = await getTeamStorageConfigById(document.teamId); + if (!storageConfig.advancedDistributionHost) { + return res.status(503).json({ + message: "Advanced Excel preview is not configured for this team.", + }); + } returnData.file = `https://${storageConfig.advancedDistributionHost}/${primaryVersion.file}`; } else { returnData.file = primaryVersion.file; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pages/api/teams/`[teamId]/documents/[id]/preview-data.ts around lines 133 - 135, The code builds a preview URL using storageConfig.advancedDistributionHost without checking it, which can produce "https://undefined/..."; update the block around getTeamStorageConfigById to guard advancedDistributionHost (from storageConfig) before composing returnData.file: if storageConfig?.advancedDistributionHost is truthy set returnData.file = `https://${storageConfig.advancedDistributionHost}/${primaryVersion.file}`, otherwise skip setting returnData.file (or fall back to the existing non-distributed URL path used elsewhere) so teams without advancedDistributionHost configured don't get an invalid URL; modify the code that assigns returnData.file and reference storageConfig, advancedDistributionHost, returnData.file and primaryVersion.file when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pages/api/teams/`[teamId]/documents/[id]/preview-data.ts:
- Around line 133-135: The code builds a preview URL using
storageConfig.advancedDistributionHost without checking it, which can produce
"https://undefined/..."; update the block around getTeamStorageConfigById to
guard advancedDistributionHost (from storageConfig) before composing
returnData.file: if storageConfig?.advancedDistributionHost is truthy set
returnData.file =
`https://${storageConfig.advancedDistributionHost}/${primaryVersion.file}`,
otherwise skip setting returnData.file (or fall back to the existing
non-distributed URL path used elsewhere) so teams without
advancedDistributionHost configured don't get an invalid URL; modify the code
that assigns returnData.file and reference storageConfig,
advancedDistributionHost, returnData.file and primaryVersion.file when making
the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 592b62f9-4484-43e4-913f-872b4261ab7f
📒 Files selected for processing (1)
pages/api/teams/[teamId]/documents/[id]/preview-data.ts
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Enables advanced Excel file preview (Office Online embed) on the document detail page and dataroom admin side.
Summary by CodeRabbit