Feat: Download specific folder / subfolder in dataroom (viewmode) #1626
Conversation
|
@AshishViradiya153 is attempting to deploy a commit to the mftsio Team on Vercel. A member of the Team first needs to authorize it. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughFolder download functionality was added to the dataroom viewer. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FolderCard
participant API (/api/links/download/dataroom-folder)
participant Lambda (ZIP Creator)
participant S3
User->>FolderCard: Clicks "Download" on folder
FolderCard->>API: POST folderId, dataroomId, viewId, linkId
API->>API: Validate permissions and fetch folder/documents
API->>Lambda: Invoke with file keys & folder structure
Lambda->>S3: Read files and create ZIP archive
Lambda->>API: Return download URL
API->>FolderCard: Respond with download URL
FolderCard->>User: Initiates ZIP file download
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (2)
components/view/dataroom/folder-card.tsx (2)
36-87: Download functionality implementationThe
downloadDocumentfunction implements a robust folder download flow with:
- Client-side permission validation
- Appropriate user feedback via toast notifications
- Error handling
- Programmatic download initiation
I have one suggestion about the toast message:
The loading toast message says "Downloading dataroom document..." but it should reference a folder instead:
- loading: "Downloading dataroom document...", + loading: "Downloading dataroom folder...",
113-143: Conditional UI for download functionalityThe dropdown menu for download actions is conditionally rendered only when
allowDownloadis true, which is a good practice for hiding actions that aren't available. The implementation properly stops event propagation to prevent folder navigation when clicking the download action.Consider disabling the dropdown trigger button completely when downloads aren't allowed, rather than not rendering the menu at all. This would provide a more consistent UI while still preventing downloads:
-{allowDownload ? ( <div className="z-10"> <DropdownMenu open={open} onOpenChange={setOpen}> <!-- dropdown content --> </DropdownMenu> </div> -) : null} +<div className="z-10"> + <DropdownMenu open={open} onOpenChange={setOpen} disabled={!allowDownload}> + <!-- dropdown content --> + </DropdownMenu> +</div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
components/view/dataroom/folder-card.tsx(2 hunks)components/view/viewer/dataroom-viewer.tsx(5 hunks)pages/api/links/download/dataroom-folder.ts(1 hunks)pages/room_ppreview_demo.tsx(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
components/view/dataroom/folder-card.tsx (2)
components/ui/dropdown-menu.tsx (5)
DropdownMenu(189-189)DropdownMenuTrigger(190-190)DropdownMenuContent(191-191)DropdownMenuLabel(195-195)DropdownMenuItem(192-192)components/ui/button.tsx (1)
Button(71-71)
🔇 Additional comments (16)
pages/room_ppreview_demo.tsx (2)
128-130: Added required props to FolderCardThe addition of
isPreview,linkId, andallowDownloadprops to theFolderCardcomponent properly aligns with the component's updated interface. SettingallowDownloadtofalseensures downloads are disabled in this demo context.
146-148: Props alignment with component requirementsThese props consistently match the ones added to the first
FolderCardinstance, maintaining a uniform configuration throughout the demo page.components/view/viewer/dataroom-viewer.tsx (4)
37-39: Type extension to support folder downloadingThe
FolderOrDocumenttype has been properly extended to include anallowDownloadproperty for folders, which is necessary for the download permission logic.
132-153: Comprehensive folder download permission logicThe implementation correctly determines download permissions for folders by checking:
- If global download permission is enabled
- If folder contains documents
- If all documents in the folder are downloadable
- If no document has "notion" type which can't be downloaded
This thorough approach ensures that folders are only downloadable when appropriate.
171-171: Updated useMemo dependency arrayThe dependency array has been properly updated to include
allowDownload, ensuring the mixed items list is recalculated when download permissions change.
193-197: Folder component props for download functionalityThe
FolderCardnow receives all necessary props for download functionality:isPreview,linkId,viewId, andallowDownload. This enables proper permission checks and API interactions for folder downloads.components/view/dataroom/folder-card.tsx (3)
1-16: Import dependencies for download functionalityThe necessary imports have been added for the download functionality, including UI components for the dropdown menu and icons.
22-25: Updated component prop interfaceThe
FolderCardPropstype has been extended with the new required props for download functionality, maintaining type safety.
31-35: Updated function parametersThe component function signature now includes the new props, ensuring they're available for use in the implementation.
pages/api/links/download/dataroom-folder.ts (7)
8-10: Extended request timeout for large folder downloadsSetting
maxDuration: 180is appropriate for this endpoint since folder zipping and download URL generation can take significant time, especially for large folders.
30-76: Comprehensive permission checksThe implementation includes thorough permission validation, checking:
- View existence with correct link and type
- Link download permissions
- Link archival status
- Link expiration
- View recency (within 23 hours)
This ensures downloads are only allowed in appropriate contexts.
78-126: Data retrieval logicThe implementation correctly fetches the root folder, subfolders, and all documents within these folders. The document selection includes the necessary fields for download processing.
129-147: Group permission handlingThe endpoint properly filters folders and documents based on group permissions when applicable, ensuring users only have access to content they're authorized to download.
149-193: Folder structure creation helperThe
addFileToStructurehelper function builds a normalized folder hierarchy with proper slugified paths. This ensures consistent folder naming and structure in the generated ZIP file.
241-247: Error handlingThe error handling is thorough, catching all potential errors and returning an appropriate 500 response with the error message. This ensures that any issues during processing are properly communicated to the client.
221-240:Details
❓ Verification inconclusive
AWS Lambda integration
The implementation correctly invokes the Lambda function to generate the ZIP file, parses the response, and returns the download URL to the client.
Verify that the Lambda function exists in both development and production environments:
Also, ensure the Lambda function properly handles the passed parameters and returns the expected response format.
🏁 Script executed:
#!/bin/bash # Check if Lambda function exists in environments # Define function names for both environments DEV_FUNCTION="bulk-download-zip-creator-dev" PROD_FUNCTION="bulk-download-zip-creator-prod" # Display current environment echo "Current NODE_ENV: $NODE_ENV" # Get environment-appropriate function name FUNCTION_NAME="${NODE_ENV:-development}" if [[ "$FUNCTION_NAME" == "development" ]]; then FUNCTION_NAME="$DEV_FUNCTION" else FUNCTION_NAME="$PROD_FUNCTION" fi echo "Checking Lambda function: $FUNCTION_NAME" # Verify Lambda function exists aws lambda get-function --function-name "$FUNCTION_NAME" --query 'Configuration.FunctionName' || echo "Function does not exist or is not accessible"Length of output: 680
Verify AWS Lambda function configuration
- Confirm that both
bulk-download-zip-creator-devandbulk-download-zip-creator-prodexist and are accessible (e.g. via AWS Console or your AWS CLI).- Ensure the Next.js service’s IAM role has permission to invoke these functions (
lambda:InvokeFunction).- Verify the Lambda handler accepts the passed payload (
sourceBucket,fileKeys,folderStructure) and returns a response in the form:{ "statusCode": 200, "body": JSON.stringify({ "downloadUrl": "<url>" }) }- Once confirmed, the client-side code that decodes the payload and extracts
downloadUrlis correct and requires no further changes.
| for (const folder of allFolders) { | ||
| const docs = allDocuments.filter((doc) => doc.folderId === folder.id); | ||
|
|
||
| if (docs.length === 0) { | ||
| addFileToStructure(folder.path, rootFolder, "", ""); | ||
| continue; | ||
| } | ||
|
|
||
| for (const doc of docs) { | ||
| const version = doc.document.versions[0]; | ||
| if (!version || version.type === "notion" || version.storageType === "VERCEL_BLOB") continue; | ||
|
|
||
| const fileKey = version.originalFile ?? version.file; | ||
| addFileToStructure(folder.path, rootFolder, doc.document.name, fileKey); | ||
| } | ||
| } |
There was a problem hiding this comment.
Document processing
The implementation processes documents within folders and excludes unsupported types (notion) and storage types (VERCEL_BLOB). This ensures only downloadable files are included in the ZIP.
There's a potential issue with empty folders. When a folder has no documents (docs.length === 0), it adds the folder with empty strings for file name and key:
-if (docs.length === 0) {
- addFileToStructure(folder.path, rootFolder, "", "");
- continue;
-}
+if (docs.length === 0) {
+ // Just add the empty folder structure without trying to add a file
+ const fullPath = folder.path;
+ let relativePath = "";
+ if (fullPath !== rootFolder.path) {
+ const pathRegex = new RegExp(`^${rootFolder.path}/(.*)$`);
+ const match = fullPath.match(pathRegex);
+ relativePath = match ? match[1] : "";
+ }
+
+ const pathParts = [slugify(rootFolder.name)];
+ if (relativePath) {
+ pathParts.push(...relativePath.split("/").filter(Boolean).map(part => slugify(part)));
+ }
+
+ let currentPath = "";
+ for (const part of pathParts) {
+ currentPath += "/" + part;
+ if (!folderStructure[currentPath]) {
+ folderStructure[currentPath] = {
+ name: part,
+ path: currentPath,
+ files: [],
+ };
+ }
+ }
+ continue;
+}Or better yet, refactor the addFileToStructure function to handle empty folders separately.
📝 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.
| for (const folder of allFolders) { | |
| const docs = allDocuments.filter((doc) => doc.folderId === folder.id); | |
| if (docs.length === 0) { | |
| addFileToStructure(folder.path, rootFolder, "", ""); | |
| continue; | |
| } | |
| for (const doc of docs) { | |
| const version = doc.document.versions[0]; | |
| if (!version || version.type === "notion" || version.storageType === "VERCEL_BLOB") continue; | |
| const fileKey = version.originalFile ?? version.file; | |
| addFileToStructure(folder.path, rootFolder, doc.document.name, fileKey); | |
| } | |
| } | |
| for (const folder of allFolders) { | |
| const docs = allDocuments.filter((doc) => doc.folderId === folder.id); | |
| if (docs.length === 0) { | |
| // Just add the empty folder structure without trying to add a file | |
| const fullPath = folder.path; | |
| let relativePath = ""; | |
| if (fullPath !== rootFolder.path) { | |
| const pathRegex = new RegExp(`^${rootFolder.path}/(.*)$`); | |
| const match = fullPath.match(pathRegex); | |
| relativePath = match ? match[1] : ""; | |
| } | |
| const pathParts = [slugify(rootFolder.name)]; | |
| if (relativePath) { | |
| pathParts.push( | |
| ...relativePath | |
| .split("/") | |
| .filter(Boolean) | |
| .map((part) => slugify(part)) | |
| ); | |
| } | |
| let currentPath = ""; | |
| for (const part of pathParts) { | |
| currentPath += "/" + part; | |
| if (!folderStructure[currentPath]) { | |
| folderStructure[currentPath] = { | |
| name: part, | |
| path: currentPath, | |
| files: [], | |
| }; | |
| } | |
| } | |
| continue; | |
| } | |
| for (const doc of docs) { | |
| const version = doc.document.versions[0]; | |
| if (!version || version.type === "notion" || version.storageType === "VERCEL_BLOB") | |
| continue; | |
| const fileKey = version.originalFile ?? version.file; | |
| addFileToStructure(folder.path, rootFolder, doc.document.name, fileKey); | |
| } | |
| } |
| if (req.method !== "POST") { | ||
| res.setHeader("Allow", ["POST"]); | ||
| return res.status(405).end(`Method ${req.method} Not Allowed`); | ||
| } | ||
|
|
||
| try { | ||
| const { folderId, dataroomId, viewId, linkId } = req.body as { | ||
| folderId: string; | ||
| dataroomId: string; | ||
| viewId: string; | ||
| linkId: string; | ||
| }; | ||
| if (!folderId) { | ||
| return res.status(400).json({ error: "folderId is required in request body" }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Request validation
The endpoint correctly validates the HTTP method and required parameters. However, the validation is incomplete.
Add validation for all required parameters in the request body:
-if (!folderId) {
+if (!folderId || !dataroomId || !linkId) {
return res.status(400).json({ error: "folderId is required in request body" });
+ return res.status(400).json({
+ error: "folderId, dataroomId, and linkId are required in request body"
+ });
}Also, consider validating types of these parameters to ensure they are strings.
Committable suggestion skipped: line range outside the PR's diff.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
pages/api/links/download/dataroom-folder.ts (2)
215-218: 🛠️ Refactor suggestionInefficient empty folder handling.
The current implementation adds an entry with empty strings for fileName and fileKey when a folder has no documents, which could cause issues in the Lambda function.
Modify the empty folder handling to only create the folder structure without attempting to add an empty file:
-if (docs.length === 0) { - addFileToStructure(folder.path, rootFolder, "", ""); - continue; -} +if (docs.length === 0) { + // Just add the empty folder structure without trying to add a file + const fullPath = folder.path; + let relativePath = ""; + if (fullPath !== rootFolder.path) { + const pathRegex = new RegExp(`^${rootFolder.path}/(.*)$`); + const match = fullPath.match(pathRegex); + relativePath = match ? match[1] : ""; + } + + const pathParts = [slugify(rootFolder.name)]; + if (relativePath) { + pathParts.push(...relativePath.split("/").filter(Boolean).map(part => slugify(part))); + } + + let currentPath = ""; + for (const part of pathParts) { + currentPath += "/" + part; + if (!folderStructure[currentPath]) { + folderStructure[currentPath] = { + name: part, + path: currentPath, + files: [], + }; + } + } + continue; +}
30-34: 🛠️ Refactor suggestionIncomplete request validation.
The validation only checks for
folderIdbut doesn't validate other required parameters.Enhance the validation to check all required parameters:
-if (!folderId) { - return res - .status(400) - .json({ error: "folderId is required in request body" }); -} +if (!folderId || !dataroomId || !viewId || !linkId) { + return res + .status(400) + .json({ error: "Missing required parameters: folderId, dataroomId, viewId, and linkId are required" }); +}
🧹 Nitpick comments (2)
pages/api/links/download/dataroom-folder.ts (2)
56-59: Error message lacks specificity.The generic error message "Error downloading" doesn't provide enough context about what went wrong.
Consider using more descriptive error messages:
-return res.status(404).json({ error: "Error downloading" }); +return res.status(404).json({ error: "View not found" });
254-257: Lambda response error handling could be improved.The code throws a generic error if the Lambda payload is empty, but doesn't handle other potential issues like function errors.
Enhance the error handling to check for Lambda function execution errors:
const command = new InvokeCommand(params); const response = await client.send(command); -if (!response.Payload) throw new Error("Lambda returned empty payload"); +if (!response.Payload) { + throw new Error("Lambda returned empty payload"); +} + +// Check if Lambda function execution had an error +if (response.FunctionError) { + console.error("Lambda function error:", response.FunctionError); + throw new Error(`Lambda execution failed: ${response.FunctionError}`); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
components/datarooms/groups/group-permissions.tsx(2 hunks)components/view/dataroom/folder-card.tsx(2 hunks)pages/api/links/download/bulk.ts(1 hunks)pages/api/links/download/dataroom-folder.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- components/view/dataroom/folder-card.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
pages/api/links/download/dataroom-folder.ts (1)
pages/api/links/download/bulk.ts (1)
config(9-12)
🔇 Additional comments (4)
components/datarooms/groups/group-permissions.tsx (2)
26-28: Import reordering improves organization.The reordering of imports places related utilities together, making the code more organized.
456-465: Enhanced permission inheritance logic correctly propagates download rights.The updated logic now triggers parent folder permission updates when either view or download permissions are enabled on a child item. This ensures parent folders maintain appropriate access levels for navigation to downloadable content.
This change is essential for the new folder download functionality to work correctly by ensuring users can navigate through the folder hierarchy to reach downloadable items.
pages/api/links/download/bulk.ts (1)
237-239: Good addition: Early return for empty downloads prevents unnecessary Lambda calls.This check prevents invoking the Lambda function when there are no files to download, saving resources and providing a clear error message to the client.
pages/api/links/download/dataroom-folder.ts (1)
14-269: Overall API implementation is well structured.The API endpoint properly validates permissions, retrieves relevant data, and handles the folder download process. It integrates well with the existing bulk download functionality and follows similar patterns for permission checking and Lambda invocation.
The structured approach to building the folder hierarchy with proper path handling is good for ensuring a clean ZIP file structure.
| const rootPath = "/" + slugify(rootFolder.name); | ||
| if (!folderStructure[rootPath]) { | ||
| folderStructure[rootPath] = { | ||
| name: slugify(rootFolder.name), | ||
| path: rootPath, | ||
| files: [], | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing empty fileKeys check before Lambda invocation.
Unlike the bulk.ts file, there's no check to prevent Lambda invocation when no files are available for download.
Add a check similar to the one in bulk.ts:
const rootPath = "/" + slugify(rootFolder.name);
if (!folderStructure[rootPath]) {
folderStructure[rootPath] = {
name: slugify(rootFolder.name),
path: rootPath,
files: [],
};
}
+if (fileKeys.length === 0) {
+ return res.status(404).json({ error: "No files to download" });
+}
+
const client = getLambdaClient();| export const config = { | ||
| maxDuration: 180, | ||
| }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Resource configuration missing memory allocation.
The config export is missing the memory allocation that's present in the bulk.ts file.
Consider adding memory allocation similar to the bulk download endpoint:
export const config = {
maxDuration: 180,
+ memory: 2048,
};🏁 Script executed:
#!/bin/bash
# Check if the bulk download endpoint has a memory configuration
grep -A 3 "export const config" pages/api/links/download/bulk.tsLength of output: 129
Add memory allocation to dataroom-folder endpoint config
The config export in pages/api/links/download/dataroom-folder.ts only sets maxDuration and should include a memory value to match the bulk download endpoint.
• File: pages/api/links/download/dataroom-folder.ts (lines 10–12)
• Bulk endpoint reference: pages/api/links/download/bulk.ts sets memory: 2048
Suggested change:
export const config = {
maxDuration: 180,
+ memory: 2048,
};📝 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.
| export const config = { | |
| maxDuration: 180, | |
| }; | |
| export const config = { | |
| maxDuration: 180, | |
| memory: 2048, | |
| }; |
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Chores