fix: drop file fail on windows #4624
Open
Conversation
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
|
|
|
lintao4 seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
Walkthrough在 FileDropService 中，将两个内部目标路径的计算从 Uri.file(...).path 切换为 Uri.file(...).fsPath，其余写入流创建、写入与关闭逻辑未改动；无公开 API 变更。 Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Assessment against linked issues
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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/addons/src/node/file-drop.service.ts (3)
12-16: 建议：在创建写入流前加固路径安全与错误处理当前直接
path.join(targetDir, fileName)没有防目录穿越（如..）、也未确保父目录存在、且对createWriteStream的error事件未做清理，可能导致：
- 文件被写到
targetDir之外（安全/多工作区隔离风险）。- 目录不存在时创建流失败、Map 中遗留无效句柄。
- 异常未被及时清理，造成资源泄漏。
可在不改变对外行为的前提下，局部增强健壮性（保持
.fsPath以兼容 Windows）：- async $ensureFileExist(fileName: string, targetDir: string): Promise<boolean> { - const targetPath = Uri.file(path.join(targetDir, fileName)).fsPath; - this.writeableStreams.set(targetPath, fs.createWriteStream(targetPath)); - return true; - } + async $ensureFileExist(fileName: string, targetDir: string): Promise<boolean> { + const base = path.resolve(targetDir); + const abs = path.resolve(base, fileName); + if (!abs.startsWith(base + path.sep)) { + throw new Error('Invalid fileName: outside targetDir'); + } + await fs.promises.mkdir(path.dirname(abs), { recursive: true }); + const targetPath = Uri.file(abs).fsPath; + const stream = fs.createWriteStream(targetPath); + // 出错时及时清理，避免 Map 残留 + stream.once('error', () => this.writeableStreams.delete(targetPath)); + this.writeableStreams.set(targetPath, stream); + return true; + }
18-33: 建议：修正写入流程以避免数据损坏或丢失（ArrayBuffer 处理、编码、end vs close、未初始化流）
chunk类型包含ArrayBuffer时，直接传给stream.write可能抛错；应先转Buffer。- 对字符串使用
'binary'编码容易引入内容歧义；建议尽量以Buffer写入，避免编码参与。close()可能在缓冲区未完全刷新时关闭文件，推荐在最后一次写入后调用end()并在回调中清理。- 若未先调用
$ensureFileExist，stream为空时当前实现不会 reject，调用方无法感知失败。可在本段内做最小入侵式调整：
- $writeStream(chunk: string | ArrayBuffer, fileName: string, targetDir: string, done: boolean): Promise<void> { - const targetPath = Uri.file(path.join(targetDir, fileName)).fsPath; - const stream = this.writeableStreams.get(targetPath); - return new Promise((resolve, reject) => { - stream?.write(chunk, 'binary', (err) => { - if (err) { - reject(err); - } - resolve(); - if (done) { - stream.close(); - this.writeableStreams.delete(targetPath); - } - }); - }); - } + $writeStream(chunk: string | ArrayBuffer, fileName: string, targetDir: string, done: boolean): Promise<void> { + const base = path.resolve(targetDir); + const abs = path.resolve(base, fileName); + if (!abs.startsWith(base + path.sep)) { + return Promise.reject(new Error('Invalid fileName: outside targetDir')); + } + const targetPath = Uri.file(abs).fsPath; + const stream = this.writeableStreams.get(targetPath); + if (!stream) { + return Promise.reject(new Error('WriteStream not initialized')); + } + return new Promise((resolve, reject) => { + const data = typeof chunk === 'string' ? Buffer.from(chunk, 'binary') : Buffer.from(chunk); + stream.write(data, (err) => { + if (err) return reject(err); + if (done) { + stream.end(() => { + this.writeableStreams.delete(targetPath); + resolve(); + }); + } else { + resolve(); + } + }); + }); + }
18-33: 回归验证建议：覆盖 Windows 与非 Windows 路径与内容一致性为确保此次修复不引入回归，建议最小用例集：
- Windows 10/11（含不同盘符）：分别将文本文件与二进制文件（如 PNG）拖入文件树，打开后与源文件做哈希比对应一致。示例（PowerShell）：
Get-FileHash .\source.png; Get-FileHash .\workspace\target.png- 包含空格与非 ASCII 文件名（如中文）。
- 目标目录包含/不包含预创建子目录（验证递归创建逻辑，如采纳上述建议）。
- 未调用
$ensureFileExist直接$writeStream的错误反馈是否符合预期（应明确失败）。如需，我可以补充一个最小的集成测试（利用临时目录 + 伪造 chunk 数据）以自动化覆盖上述场景。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/addons/src/node/file-drop.service.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: weiwen111
PR: opensumi/core#4139
File: packages/core-browser/src/window/window.service.ts:0-0
Timestamp: 2024-10-30T12:51:05.546Z
Learning: 在 Web 环境中，使用 `workspaceUri.path.toString()` 获取路径可能仍然包含 `file://` 前缀，应该使用 `workspaceUri.fsPath` 来获取不含 `file://` 前缀的路径。
🧬 Code graph analysis (1)
packages/addons/src/node/file-drop.service.ts (1)
scripts/download.js (1)
targetDir(14-14)
🔇 Additional comments (1)
packages/addons/src/node/file-drop.service.ts (1)
12-16: Windows 路径修复方向正确：改用 fsPath 规避盘符前置斜杠与 file:// 前缀问题在 Windows 上由
Uri.file(...).path切换为.fsPath能避免诸如/C:/...这类路径导致的写入失败，和我们既有经验一致（此前在 Web 环境也有类似结论：应使用fsPath获取无file://前缀的路径）。本次修改对定位到的问题是有效的。
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4624 +/- ##
==========================================
+ Coverage 52.59% 52.61% +0.01%
==========================================
Files 1686 1686
Lines 104362 104362
Branches 22682 22682
==========================================
+ Hits 54894 54905 +11
+ Misses 41097 41087 -10
+ Partials 8371 8370 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
Types
Background or solution
fix #3615
在 $ensureFileExis方法里的targetPath原来类似于图片上这种，在创建文件写入流会报错，报错原因是路径生成错误。
Changelog
修复在windows上，拖拽文件到IDE中，文件损坏的问题
Summary by CodeRabbit