[BUG] Dropbox → Download File to TMP action is very inefficient #16874 #16894
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
WalkthroughThe changes introduce a streaming-based approach for downloading Dropbox files to a temporary directory, replacing the previous memory-intensive method. Supporting utilities and methods are added, including a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Action (download-file-to-tmp)
participant DropboxApp
participant DropboxAPI
participant got/stream/fs
User->>Action: Trigger download-file-to-tmp
Action->>DropboxApp: Call filesGetTemporaryLink(path)
DropboxApp->>DropboxAPI: filesGetTemporaryLink(path)
DropboxAPI-->>DropboxApp: { link, metadata }
DropboxApp-->>Action: { link, metadata }
Action->>got/stream/fs: Stream file from link to /tmp/filename
got/stream/fs-->>Action: File saved to tmpPath
Action-->>User: Return tmpPath and metadata
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
components/dropbox/actions/create-folder/create-folder.mjsOops! Something went wrong! :( ESLint: 8.57.1 Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'jsonc-eslint-parser' imported from /eslint.config.mjs components/dropbox/actions/list-file-folders-in-a-folder/list-file-folders-in-a-folder.mjsOops! Something went wrong! :( ESLint: 8.57.1 Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'jsonc-eslint-parser' imported from /eslint.config.mjs components/dropbox/actions/create-a-text-file/create-a-text-file.mjsOops! Something went wrong! :( ESLint: 8.57.1 Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'jsonc-eslint-parser' imported from /eslint.config.mjs
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (20)
✅ Files skipped from review due to trivial changes (7)
🚧 Files skipped from review as they are similar to previous changes (13)
⏰ Context from checks skipped due to timeout of 90000ms (4)
✨ Finishing Touches
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. 🪧 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 (3)
components/dropbox/actions/create-update-share-link/create-update-share-link.mjs (1)
2-2: Maintain consistent import ordering.The import order between
constsandcommonhas been swapped here. Consider aligning import order with other Dropbox actions for consistency (e.g., group shared constants, then shared utilities, then action-specific modules).components/dropbox/actions/download-file-to-tmp/download-file-to-tmp.mjs (2)
38-38: Remove debug console.log statement.The console.log statement appears to be leftover debug code and should be removed from production code to avoid log pollution and potential information leakage.
- console.log("linkResponse: ", linkResponse);
53-53: Consider a more nuanced filename sanitization approach.The current regex-based sanitization is good for security but may be overly aggressive, removing characters like spaces and parentheses that are typically valid in filenames. Consider using a whitelist approach or a more targeted sanitization.
Example of a more targeted approach:
- const cleanFileName = fileName.replace(/[?$#&{}[]<>\*!@:\+\\\/]/g, ""); + const cleanFileName = fileName.replace(/[<>:"/\\|?*\x00-\x1f]/g, "").replace(/^\.+/, "");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (20)
components/dropbox/actions/create-a-text-file/create-a-text-file.mjs(1 hunks)components/dropbox/actions/create-folder/create-folder.mjs(1 hunks)components/dropbox/actions/create-or-append-to-a-text-file/create-or-append-to-a-text-file.mjs(1 hunks)components/dropbox/actions/create-update-share-link/create-update-share-link.mjs(1 hunks)components/dropbox/actions/delete-file-folder/delete-file-folder.mjs(1 hunks)components/dropbox/actions/download-file-to-tmp/download-file-to-tmp.mjs(2 hunks)components/dropbox/actions/list-file-folders-in-a-folder/list-file-folders-in-a-folder.mjs(1 hunks)components/dropbox/actions/list-file-revisions/list-file-revisions.mjs(1 hunks)components/dropbox/actions/move-file-folder/move-file-folder.mjs(1 hunks)components/dropbox/actions/rename-file-folder/rename-file-folder.mjs(1 hunks)components/dropbox/actions/restore-a-file/restore-a-file.mjs(1 hunks)components/dropbox/actions/search-files-folders/search-files-folders.mjs(1 hunks)components/dropbox/actions/upload-file/upload-file.mjs(1 hunks)components/dropbox/actions/upload-multiple-files/upload-multiple-files.mjs(1 hunks)components/dropbox/common/utils.mjs(1 hunks)components/dropbox/dropbox.app.mjs(2 hunks)components/dropbox/package.json(2 hunks)components/dropbox/sources/all-updates/all-updates.mjs(1 hunks)components/dropbox/sources/new-file/new-file.mjs(1 hunks)components/dropbox/sources/new-folder/new-folder.mjs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Verify TypeScript components
- GitHub Check: pnpm publish
- GitHub Check: Publish TypeScript components
- GitHub Check: Lint Code Base
🔇 Additional comments (28)
components/dropbox/actions/create-or-append-to-a-text-file/create-or-append-to-a-text-file.mjs (1)
7-7: Consistent version bump
The action’sversionhas been updated from "0.0.10" to "0.0.11", matching the coordinated version alignment across Dropbox components. No logic or behavior changes detected.components/dropbox/actions/rename-file-folder/rename-file-folder.mjs (1)
7-7: Consistent version bump
Theversionstring has been incremented to "0.0.11" in line with the other Dropbox action updates. There are no functional changes in this file.components/dropbox/actions/create-folder/create-folder.mjs (1)
7-7: Consistent version bump
Theversionfield was correctly bumped to "0.0.11" to stay aligned with the rest of the Dropbox integration. No other modifications were made.components/dropbox/sources/new-folder/new-folder.mjs (1)
10-10: Consistent version bump
The source’sversionhas been updated to "0.0.18", matching the pattern of coordinated version updates across Dropbox sources and actions. No logic changes.components/dropbox/actions/restore-a-file/restore-a-file.mjs (1)
7-7: Consistent version bump
Theversionhas been raised to "0.0.11" for the restore action, in sync with peer components. No behavioral changes were introduced.components/dropbox/actions/create-a-text-file/create-a-text-file.mjs (1)
7-7: Version increment looks good.Standard version bump as part of the coordinated release addressing the download efficiency issue.
components/dropbox/actions/list-file-folders-in-a-folder/list-file-folders-in-a-folder.mjs (1)
7-7: Version increment is appropriate.Consistent version bump as part of the release addressing the download efficiency improvements.
components/dropbox/actions/list-file-revisions/list-file-revisions.mjs (1)
8-8: Version update follows standard practice.Appropriate version increment as part of the coordinated release.
components/dropbox/actions/create-update-share-link/create-update-share-link.mjs (1)
8-8: Version bump looks good.The action version has been correctly updated to "0.0.11" to stay in sync with the coordinated release across Dropbox components.
components/dropbox/actions/move-file-folder/move-file-folder.mjs (1)
7-7: Version bump alignment approved.The action version was updated from "0.0.11" to "0.0.12" in line with the release plan. No functional changes detected.
components/dropbox/actions/delete-file-folder/delete-file-folder.mjs (1)
7-7: Version increment approved.The
versionproperty update to "0.0.11" matches the coordinated version updates across Dropbox actions.components/dropbox/sources/new-file/new-file.mjs (1)
10-10: Version bump is correct.The source version update to "0.0.19" aligns with other Dropbox components in this release.
components/dropbox/sources/all-updates/all-updates.mjs (1)
10-10: Version update approved.The version has been bumped to "0.0.18" consistently with other Dropbox sources and actions.
components/dropbox/actions/upload-multiple-files/upload-multiple-files.mjs (2)
1-5: LGTM! Import organization improved.The import reordering with
ConfigurationErrorfirst is a good organizational change that enhances readability.
11-11: Version bump is consistent with the PR scope.The version increment from "0.0.1" to "0.0.2" aligns with the coordinated updates across Dropbox components.
components/dropbox/package.json (3)
3-3: Version bump is consistent with component updates.The package version increment aligns with the coordinated updates across Dropbox actions.
18-20:✅ Verification successful
Verify new dependency versions for security and compatibility.
The addition of "stream" and "util" dependencies supports the streaming approach, but ensure these versions are secure and appropriate.
🌐 Web query:
Are there any security vulnerabilities in npm packages "stream" version 0.0.3 and "util" version 0.12.5?💡 Result:
After reviewing the available information, there are no known security vulnerabilities associated with the npm packages "stream" version 0.0.3 and "util" version 0.12.5. Both packages have been scanned for vulnerabilities, and no issues have been identified. (snyk.io, security.snyk.io)
It's important to note that while these specific versions are currently free from known vulnerabilities, the security landscape can evolve over time. Therefore, it's advisable to regularly monitor your project's dependencies for any emerging security issues. Utilizing tools like Snyk can assist in identifying and addressing potential vulnerabilities in your project's dependencies. (snyk.io)
Citations:
- 1: https://snyk.io/advisor/npm-package/stream?utm_source=openai
- 2: https://security.snyk.io/package/npm/util?utm_source=openai
- 3: https://snyk.io/advisor/npm-package/util?utm_source=openai
Dependencies secure – no known vulnerabilities detected
Verified via Snyk that[email protected]and[email protected]have no reported security issues. Continue to monitor these (and other) dependencies regularly for newly disclosed vulnerabilities.
15-15:✅ Verification successful
Verify the reason for downgrading the "got" dependency.
The downgrade from version 14.0.0 to 13.0.0 might indicate compatibility issues or specific requirements for the streaming functionality.
🌐 Web query:
What are the breaking changes between got version 13.0.0 and 14.0.0?💡 Result:
As of my knowledge cutoff in June 2024, the latest stable release of the
gotHTTP request library was version 11.6.1. There is no official release ofgotversion 13.0.0 or 14.0.0. Therefore, I cannot provide specific details about breaking changes between these versions.For the most accurate and up-to-date information on breaking changes and version releases, I recommend checking the official
gotGitHub repository or its npm package page. These sources will provide the latest release notes and migration guides to assist you in upgrading to newer versions.
🏁 Script executed:
#!/bin/bash # Verify whether [email protected] and [email protected] exist on the npm registry echo "Checking [email protected]..." npm view [email protected] version || echo "[email protected] not found" echo "Checking [email protected]..." npm view [email protected] version || echo "[email protected] not found"Length of output: 1763
🏁 Script executed:
#!/bin/bash # Fetch release notes for [email protected] and [email protected] to compare breaking changes echo "Release notes for v13.0.0:" curl -s https://api.github.com/repos/sindresorhus/got/releases/tags/v13.0.0 | jq '.body' echo -e "\n\nRelease notes for v14.0.0:" curl -s https://api.github.com/repos/sindresorhus/got/releases/tags/v14.0.0 | jq '.body'Length of output: 1688
Confirm Node.js compatibility before downgrading “got”
Got v14.0.0 drops support for all Node.js versions below 20, whereas v13.x still supports Node.js 16+ (including the current LTS at v18). It looks like you’ve pinned “got” to ^13.0.0 to maintain compatibility.
• File: components/dropbox/package.json, line 15 – ensure our CI/runtime Node version is < 20 and document this requirement (e.g. in a code comment or changelog)
• If we plan to upgrade to Node.js 20 in the near future, we can revisit this dependency then.components/dropbox/actions/search-files-folders/search-files-folders.mjs (2)
1-3: Import organization improved.Moving the dropbox import after the lodash utilities improves the logical grouping of imports.
9-9: Version bump is consistent with the broader update.The increment from "0.0.10" to "0.0.11" aligns with the coordinated versioning across Dropbox components.
components/dropbox/actions/upload-file/upload-file.mjs (2)
1-5: Import organization is consistent and improved.The reordering with
ConfigurationErrorfirst maintains consistency across the Dropbox actions.
11-11: Version bump aligns with component updates.The increment from "0.0.13" to "0.0.14" is consistent with the coordinated versioning strategy.
components/dropbox/dropbox.app.mjs (2)
1-1: Import change from default to named import is appropriate.Using named import
{ Dropbox }is more explicit and aligns with modern ES6 practices.
401-408: New method supports efficient file downloading.The
filesGetTemporaryLinkmethod follows the established pattern with proper SDK instantiation and error handling. This addition directly addresses the efficiency issue mentioned in the PR by enabling streaming downloads via temporary links instead of loading files into memory.The method implementation is consistent with other API methods in the class and will enable the streaming approach for better memory efficiency.
components/dropbox/actions/download-file-to-tmp/download-file-to-tmp.mjs (4)
2-6: LGTM! Imports support the new streaming approach.The added imports are appropriate for implementing the streaming download functionality. Using
gotfor HTTP requests, Node.js built-instreamandutilmodules, and thecheckTmputility demonstrates a well-structured approach to the refactor.
12-12: LGTM! Version increment is appropriate.The patch version increment correctly reflects this efficiency improvement while maintaining API compatibility.
55-61: Excellent streaming implementation for efficiency!This streaming approach is a significant improvement over the previous memory-intensive method. Using
pipelinewithgot.stream()andfs.createWriteStream()ensures memory efficiency even for large files, directly addressing the efficiency issues mentioned in the PR objectives.
33-33: Excellent error handling improvements.The comprehensive try-catch block with proper validation and descriptive error messages significantly improves the robustness of this action. The error handling properly validates the Dropbox API response and provides clear feedback when failures occur.
Also applies to: 40-42, 69-71
components/dropbox/actions/download-file-to-tmp/download-file-to-tmp.mjs
Outdated
Show resolved
Hide resolved
jcortes
force-pushed
the
16874-bug-dropbox-download-file-to-tmp-action-is-very-inefficient
branch
from
ad41e2c to
d181867
Compare
luancazarine
deleted the
16874-bug-dropbox-download-file-to-tmp-action-is-very-inefficient
branch
Resolves #16874
Summary by CodeRabbit
New Features
Chores