fix: 解决通过路径会下载两次文件 #5319
fix: 解决通过路径会下载两次文件 #5319
Conversation
解决Chrome、Safari通过路径一次下载两个文件的BUG
wangwenting-dev
requested review from
anncwb,
vince292007 and
mynetfan
as code owners
|
|
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
packages/@core/base/shared/src/utils/download.tsOops! Something went wrong! :( ESLint: 9.17.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/node_modules/@vben/eslint-config/dist/index.mjs' imported from /eslint.config.mjs WalkthroughThe pull request modifies the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 0
🧹 Nitpick comments (3)
packages/@core/base/shared/src/utils/download.ts (3)
Line range hint
13-41: Document browser-specific behavior.The function implements different download strategies based on the browser, but this isn't clearly documented. Consider adding JSDoc comments explaining:
- Why Chrome/Safari require a different approach
- What the
?downloadparameter does for other browsers/** * 通过 URL 下载文件，支持跨域 + * Chrome/Safari: Uses direct download trigger to prevent double downloads + * Other browsers: Appends ?download parameter and uses window.open * @throws {Error} - 当下载失败时抛出错误 */
Line range hint
21-35: Consider improving browser detection logic.The current browser detection using
userAgentstring is fragile and may break with browser updates. Consider using feature detection instead of browser detection where possible.- const isChrome = window.navigator.userAgent.toLowerCase().includes('chrome'); - const isSafari = window.navigator.userAgent.toLowerCase().includes('safari'); + // Use more reliable browser detection + const isWebKit = 'webkitURL' in window; + const isModernBrowser = 'download' in HTMLAnchorElement.prototype;
Line range hint
13-41: Consider refactoring for better maintainability.The function mixes different download strategies and browser-specific logic. Consider splitting this into separate methods for better maintainability:
function downloadForWebKit(url: string, fileName: string) { triggerDownload(url, fileName); } function downloadForLegacyBrowsers(url: string, target: string) { const downloadUrl = !url.includes('?') ? `${url}?download` : url; openWindow(downloadUrl, { target }); } export async function downloadFileFromUrl({ fileName, source, target = '_blank', }: DownloadOptions): Promise<void> { if (!source || typeof source !== 'string') { throw new Error('Invalid URL.'); } if (/iP/.test(window.navigator.userAgent)) { console.error('Your browser does not support download!'); return; } const isWebKit = 'webkitURL' in window; return isWebKit ? downloadForWebKit(source, resolveFileName(source, fileName)) : downloadForLegacyBrowsers(source, target); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/@core/base/shared/src/utils/download.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: post-update (windows-latest)
- GitHub Check: post-update (ubuntu-latest)
🔇 Additional comments (2)
packages/@core/base/shared/src/utils/download.ts (2)
34-34: LGTM! The fix addresses the double download issue.The added return statement prevents the second download by ensuring the function exits immediately after triggering the download in Chrome/Safari browsers.
34-34: Verify the fix across different scenarios.While the fix addresses the double download issue, please ensure testing covers:
- Downloads with and without query parameters
- Files with different MIME types
- Cross-origin downloads
- Different file sizes
Run the following script to find test files that should be updated:
✅ Verification successful
Early return for iOS devices is safe and well-implemented
The early return specifically handles iOS devices where downloads aren't supported, preventing failed download attempts while properly maintaining the specialized Chrome/Safari path and the fallback mechanism for other browsers. No additional checks are needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for existing tests related to downloadFileFromUrl rg -l "downloadFileFromUrl" --type test # Look for browser-specific test configurations rg -l "browser.*test" --type testLength of output: 133
Script:
#!/bin/bash # Find test files echo "=== Test files ===" fd -e test.ts -e test.tsx -e spec.ts -e spec.tsx # Search for download-related tests echo -e "\n=== Download tests ===" rg "describe.*download|test.*download" -A 2 # Find implementation of downloadFileFromUrl echo -e "\n=== Implementation ===" ast-grep --pattern 'function downloadFileFromUrl($_) { $$$ }' # Look for browser-specific configurations echo -e "\n=== Browser configs ===" rg "browser.*config" -A 2Length of output: 3206
Script:
#!/bin/bash # Check the download utility implementation echo "=== Download Implementation ===" cat packages/@core/base/shared/src/utils/download.ts # Look for browser detection patterns echo -e "\n=== Browser Detection ===" rg "navigator|userAgent|browser" packages/@core/base/shared/src/utils/download.ts # Find related download functions echo -e "\n=== Related Functions ===" ast-grep --pattern 'function $_(url$_) { $$$ }'Length of output: 4453
fix: download from url triggered twice sometimes (vbenjs#5319)
解决Chrome、Safari通过路径一次下载两个文件的BUG
Description
https://github.com/vbenjs/vue-vben-admin/blob/main/packages/%40core/base/shared/src/utils/download.ts
function downloadFileFromUrl(){
....
if (isChrome || isSafari) {
triggerDownload(source, resolveFileName(source, fileName));
return;
}
....
}
通过路径下载 如果在Chrome、Safari中会单独判断，走了triggerDownload去下载， 如果没有return会触发两次下载
Type of change
Please delete options that are not relevant.
pnpm-lock.yamlunless you introduce a new test example.Checklist
pnpm run docs:devcommand.pnpm test.feat:,fix:,perf:,docs:, orchore:.Summary by CodeRabbit