feat(FR-1792): add download vfolder button to Data page #4831
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has required the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
Coverage report for
|
St.❔ |
Category | Percentage | Covered / Total |
|---|---|---|---|
| 🔴 | Statements | 52.17% (-2.61% 🔻) |
168/322 |
| 🔴 | Branches | 32.29% (-0.45% 🔻) |
93/288 |
| 🔴 | Functions | 40.26% (-2.21% 🔻) |
31/77 |
| 🔴 | Lines | 53.55% (-3.27% 🔻) |
151/282 |
Show files with reduced coverage 🔻
St.❔ |
File | Statements | Branches | Functions | Lines |
|---|---|---|---|---|---|
| 🔴 | helper/index.ts | 57.86% (-6.42% 🔻) |
43.75% (-1.12% 🔻) |
51.61% (-7.65% 🔻) |
59.85% (-8.57% 🔻) |
Test suite run success
62 tests passing in 4 suites.
Report generated by 🧪jest coverage report action from 65df5a3
Coverage report for
|
St.❔ |
Category | Percentage | Covered / Total |
|---|---|---|---|
| 🔴 | Statements | 4.37% (-0% 🔻) |
517/11843 |
| 🔴 | Branches | 3.58% (-0% 🔻) |
298/8331 |
| 🔴 | Functions | 2.62% | 95/3625 |
| 🔴 | Lines | 4.34% (-0% 🔻) |
503/11583 |
Test suite run success
145 tests passing in 14 suites.
Report generated by 🧪jest coverage report action from 65df5a3
There was a problem hiding this comment.
Pull request overview
This PR adds a download button for virtual folders on the Data page, allowing users to download entire vfolders as zip archives. The implementation extracts a reusable initiateDownload helper function and enhances the UI styling for both download and delete buttons to provide better visual feedback when disabled.
Key changes:
- New
BAIVFolderDownloadButtoncomponent for downloading vfolders - Extracted
initiateDownloadhelper function for reusability across components - Enhanced UI styling for download and delete buttons with proper disabled states
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
react/src/components/VFolderNodes.tsx |
Added download button to vfolder actions, integrated permission checks using useMergedAllowedStorageHostPermission hook, and refactored editable variable for better code clarity |
packages/backend.ai-ui/src/helper/index.ts |
Extracted initiateDownload helper function from FileItemControls to enable reuse across components |
packages/backend.ai-ui/src/components/fragments/index.ts |
Added exports for new BAIVFolderDownloadButton component and its props interface |
packages/backend.ai-ui/src/components/fragments/BAIVFolderDownloadButton.tsx |
New button component for downloading vfolders with styled icon and background based on disabled state |
packages/backend.ai-ui/src/components/baiClient/FileExplorer/FileItemControls.tsx |
Refactored to use extracted initiateDownload helper and enhanced icon colors to properly reflect disabled state |
| try { | ||
| const tokenResponse = await baiClient.vfolder.request_download_token( | ||
| './', | ||
| vfolderId, | ||
| true, | ||
| ); | ||
| const downloadParams = new URLSearchParams({ | ||
| token: tokenResponse.token, | ||
| archive: 'true', | ||
| }); | ||
| const downloadURL = `${tokenResponse.url}?${downloadParams.toString()}`; | ||
| await initiateDownload(downloadURL, `${vfolderName}.zip`); | ||
| } catch (error) { | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
The try-catch block is redundant because it simply re-throws the error without any additional handling. Since the mutation errors will be handled by the useMutation hook's error handling mechanism, this try-catch can be removed and the mutationFn can directly perform the operations and let errors propagate naturally.
| try { | |
| const tokenResponse = await baiClient.vfolder.request_download_token( | |
| './', | |
| vfolderId, | |
| true, | |
| ); | |
| const downloadParams = new URLSearchParams({ | |
| token: tokenResponse.token, | |
| archive: 'true', | |
| }); | |
| const downloadURL = `${tokenResponse.url}?${downloadParams.toString()}`; | |
| await initiateDownload(downloadURL, `${vfolderName}.zip`); | |
| } catch (error) { | |
| throw error; | |
| } | |
| const tokenResponse = await baiClient.vfolder.request_download_token( | |
| './', | |
| vfolderId, | |
| true, | |
| ); | |
| const downloadParams = new URLSearchParams({ | |
| token: tokenResponse.token, | |
| archive: 'true', | |
| }); | |
| const downloadURL = `${tokenResponse.url}?${downloadParams.toString()}`; | |
| await initiateDownload(downloadURL, `${vfolderName}.zip`); |
| * | ||
| * @param downloadURL | ||
| * @param fileName |
There was a problem hiding this comment.
The JSDoc comment for the initiateDownload function is incomplete. It should include a description of what the function does, document the return type (Promise that resolves when download is initiated), and explain the iOS Safari special handling. Consider following the style of other JSDoc comments in this file with proper descriptions and examples.
| * | |
| * @param downloadURL | |
| * @param fileName | |
| * Initiates a file download in the browser for the given URL and file name. | |
| * | |
| * This function programmatically triggers a download of the resource at `downloadURL` | |
| * and saves it as `fileName`. On iOS Safari, due to download restrictions, it opens | |
| * the URL in a new tab instead of using the download attribute. | |
| * | |
| * @param downloadURL - The URL of the file to download. | |
| * @param fileName - The desired name for the downloaded file. | |
| * @returns A Promise that resolves when the download is initiated. | |
| * | |
| * @example | |
| * ```typescript | |
| * await initiateDownload('https://example.com/file.txt', 'myfile.txt'); | |
| * ``` | |
| * | |
| * @remarks | |
| * - On iOS Safari, the function uses `window.open` to open the file in a new tab, | |
| * as the download attribute is not supported for programmatic downloads. | |
| * - On other browsers, it creates a temporary anchor element with the `download` | |
| * attribute to trigger the download. |
| const newWindow = window.open(downloadURL, '_blank'); | ||
| newWindow && resolve(); |
There was a problem hiding this comment.
The iOS Safari check could fail silently if window.open is blocked by popup blockers. When newWindow is null (popup blocked), the promise resolves without actually initiating the download. Consider rejecting the promise or throwing an error when newWindow is null to properly handle this case.
| const BAIVFolderDownloadButton = ({ | ||
| vfolderId, | ||
| vfolderName, | ||
| ...buttonProps | ||
| }: BAIVFolderDownloadButtonProps) => { |
There was a problem hiding this comment.
The new component is missing the 'use memo' directive. According to the React Component Guidelines for this project, new React components should include the 'use memo' directive at the top of the component function to leverage React Compiler optimizations. This directive should be added immediately after the opening brace of the component function.
| const { mutateAsync } = useMutation({ | ||
| mutationFn: async () => { | ||
| try { | ||
| const tokenResponse = await baiClient.vfolder.request_download_token( | ||
| './', | ||
| vfolderId, | ||
| true, | ||
| ); | ||
| const downloadParams = new URLSearchParams({ | ||
| token: tokenResponse.token, | ||
| archive: 'true', | ||
| }); | ||
| const downloadURL = `${tokenResponse.url}?${downloadParams.toString()}`; | ||
| await initiateDownload(downloadURL, `${vfolderName}.zip`); | ||
| } catch (error) { | ||
| throw error; | ||
| } | ||
| }, | ||
| }); |
There was a problem hiding this comment.
The mutation lacks error handling and user feedback. Unlike the similar FileItemControls component which provides success/error messages using onSuccess and onError callbacks with the useMutation hook, this component silently fails if the download fails. Add onSuccess callback to show a success message (using the existing 'comp:FileExplorer.DownloadStarted' translation key with the vfolder name) and onError callback to display error messages to the user.
resolves #4830 (FR-1792)
Checklist: (if applicable)