feat: drag and drop file paths into terminal#2857
Open
JustHereToHelp wants to merge 1 commit intowavetermdev:mainfrom
Open
feat: drag and drop file paths into terminal#2857JustHereToHelp wants to merge 1 commit intowavetermdev:mainfrom
JustHereToHelp wants to merge 1 commit intowavetermdev:mainfrom
Conversation
Drag files from Finder or Wave's file browser into a terminal block to insert the quoted path. Uses webUtils.getPathForFile() through a preload bridge since Electron 32 removed File.path. Paths with spaces are properly shell-quoted. Fixes wavetermdev#746, fixes wavetermdev#2813
Contributor
WalkthroughThe pull request introduces drag-and-drop file support for the terminal by adding a new API method to retrieve file paths from File objects. The change spans three files: the electron preload layer exposes a Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| const file = e.dataTransfer.files[i]; | ||
| const filePath = getApi().getPathForFile(file); | ||
| if (filePath) { | ||
| const quoted = "'" + filePath.replace(/'/g, "'\\''") + "'"; |
Contributor
There was a problem hiding this comment.
WARNING: Shell quoting issue on Windows
The POSIX-style quoting '\'' won't work correctly on Windows shells (cmd.exe, PowerShell). Windows uses different quoting rules:
- cmd.exe: Uses
"for quoting and^for escaping - PowerShell: Uses
"for quoting and`for escaping
Consider adding platform-specific quoting:
import { PLATFORM, PlatformWindows } from "@/util/platformutil";
// In the drop handler:
if (filePath) {
let quoted: string;
if (PLATFORM === PlatformWindows) {
// Windows: use double quotes and escape internal quotes
quoted = '"' + filePath.replace(/"/g, '""') + '"';
} else {
// POSIX: use single quotes and escape internal quotes
quoted = "'" + filePath.replace(/'/g, "'\\''" + "'";
}
paths.push(quoted);
}
Contributor
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (3 files)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/app/view/term/termwrap.ts`:
- Around line 202-225: The dragover/drop listeners attached to this.connectElem
are inline and never removed; refactor by extracting the callbacks into named
bound methods (e.g., onDragOver and onDrop) or stored function properties on the
class, replace the inline addEventListener calls with those references, and in
the class dispose() (or equivalent teardown) call
this.connectElem.removeEventListener("dragover", this.onDragOver) and
removeEventListener("drop", this.onDrop) so listeners are cleaned up and not
duplicated on re-init.
Comment on lines
+202
to
+225
| this.connectElem.addEventListener("dragover", (e: DragEvent) => { | ||
| e.preventDefault(); | ||
| if (e.dataTransfer) { | ||
| e.dataTransfer.dropEffect = "copy"; | ||
| } | ||
| }); | ||
| this.connectElem.addEventListener("drop", (e: DragEvent) => { | ||
| e.preventDefault(); | ||
| if (!e.dataTransfer || e.dataTransfer.files.length === 0) { | ||
| return; | ||
| } | ||
| const paths: string[] = []; | ||
| for (let i = 0; i < e.dataTransfer.files.length; i++) { | ||
| const file = e.dataTransfer.files[i]; | ||
| const filePath = getApi().getPathForFile(file); | ||
| if (filePath) { | ||
| const quoted = "'" + filePath.replace(/'/g, "'\\''") + "'"; | ||
| paths.push(quoted); | ||
| } | ||
| } | ||
| if (paths.length > 0) { | ||
| this.terminal.paste(paths.join(" ")); | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
Clean up drag/drop listeners on dispose.
These listeners are added with inline callbacks and never removed. On re-init/rebind of the same terminal element, this can duplicate drop handling.
♻️ Suggested fix
- this.connectElem.addEventListener("dragover", (e: DragEvent) => {
+ const dragOverHandler = (e: DragEvent) => {
e.preventDefault();
if (e.dataTransfer) {
e.dataTransfer.dropEffect = "copy";
}
- });
- this.connectElem.addEventListener("drop", (e: DragEvent) => {
+ };
+ const dropHandler = (e: DragEvent) => {
e.preventDefault();
if (!e.dataTransfer || e.dataTransfer.files.length === 0) {
return;
}
const paths: string[] = [];
for (let i = 0; i < e.dataTransfer.files.length; i++) {
const file = e.dataTransfer.files[i];
const filePath = getApi().getPathForFile(file);
if (filePath) {
const quoted = "'" + filePath.replace(/'/g, "'\\''") + "'";
paths.push(quoted);
}
}
if (paths.length > 0) {
this.terminal.paste(paths.join(" "));
}
- });
+ };
+ this.connectElem.addEventListener("dragover", dragOverHandler);
+ this.connectElem.addEventListener("drop", dropHandler);
+ this.toDispose.push({
+ dispose: () => {
+ this.connectElem.removeEventListener("dragover", dragOverHandler);
+ this.connectElem.removeEventListener("drop", dropHandler);
+ },
+ });📝 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.
Suggested change
| this.connectElem.addEventListener("dragover", (e: DragEvent) => { | |
| e.preventDefault(); | |
| if (e.dataTransfer) { | |
| e.dataTransfer.dropEffect = "copy"; | |
| } | |
| }); | |
| this.connectElem.addEventListener("drop", (e: DragEvent) => { | |
| e.preventDefault(); | |
| if (!e.dataTransfer || e.dataTransfer.files.length === 0) { | |
| return; | |
| } | |
| const paths: string[] = []; | |
| for (let i = 0; i < e.dataTransfer.files.length; i++) { | |
| const file = e.dataTransfer.files[i]; | |
| const filePath = getApi().getPathForFile(file); | |
| if (filePath) { | |
| const quoted = "'" + filePath.replace(/'/g, "'\\''") + "'"; | |
| paths.push(quoted); | |
| } | |
| } | |
| if (paths.length > 0) { | |
| this.terminal.paste(paths.join(" ")); | |
| } | |
| }); | |
| const dragOverHandler = (e: DragEvent) => { | |
| e.preventDefault(); | |
| if (e.dataTransfer) { | |
| e.dataTransfer.dropEffect = "copy"; | |
| } | |
| }; | |
| const dropHandler = (e: DragEvent) => { | |
| e.preventDefault(); | |
| if (!e.dataTransfer || e.dataTransfer.files.length === 0) { | |
| return; | |
| } | |
| const paths: string[] = []; | |
| for (let i = 0; i < e.dataTransfer.files.length; i++) { | |
| const file = e.dataTransfer.files[i]; | |
| const filePath = getApi().getPathForFile(file); | |
| if (filePath) { | |
| const quoted = "'" + filePath.replace(/'/g, "'\\''") + "'"; | |
| paths.push(quoted); | |
| } | |
| } | |
| if (paths.length > 0) { | |
| this.terminal.paste(paths.join(" ")); | |
| } | |
| }; | |
| this.connectElem.addEventListener("dragover", dragOverHandler); | |
| this.connectElem.addEventListener("drop", dropHandler); | |
| this.toDispose.push({ | |
| dispose: () => { | |
| this.connectElem.removeEventListener("dragover", dragOverHandler); | |
| this.connectElem.removeEventListener("drop", dropHandler); | |
| }, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app/view/term/termwrap.ts` around lines 202 - 225, The dragover/drop
listeners attached to this.connectElem are inline and never removed; refactor by
extracting the callbacks into named bound methods (e.g., onDragOver and onDrop)
or stored function properties on the class, replace the inline addEventListener
calls with those references, and in the class dispose() (or equivalent teardown)
call this.connectElem.removeEventListener("dragover", this.onDragOver) and
removeEventListener("drop", this.onDrop) so listeners are cleaned up and not
duplicated on re-init.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Fixes #746, fixes #2813
Drag a file from Finder into a terminal and it pastes the quoted path. Uses
webUtils.getPathForFile()through a preload bridge since Electron 32 killedFile.path. Handles spaces in filenames.Needs app restart after install (preload change).