feat(terminal): improve input handling for interactive CLI tools #2523
Conversation
|
|
WalkthroughAdds image-first clipboard paste support to the terminal UI and a backend RPC to create temporary files. Frontend changes introduce unified paste handling that prefers image clipboard items, convert image Blobs to temp files via a new RPC, and paste the created temp file path; text-paste fallbacks remain. IME composition state tracking, paste deduplication, and Escape-key handling during composition were added. Ctrl+Shift+V now invokes the unified paste flow and Shift+Enter default newline behavior was changed. New RPC types and handlers (GetTempDirCommand / CommandGetTempDirData) let the server return os.TempDir() optionally joined with a requested filename. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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. 🔧 golangci-lint (2.5.0)Error: unknown linters: 'unusedfunc,unusedparams', run 'golangci-lint help linters' to see the list of supported linters 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/app/view/term/term-model.ts(2 hunks)frontend/app/view/term/termwrap.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/app/view/term/termwrap.ts (2)
frontend/app/store/wshclientapi.ts (1)
RpcApi(602-602)frontend/app/store/wshrpcutil.ts (1)
TabRpcClient(37-37)
frontend/app/view/term/term-model.ts (2)
frontend/app/store/wshclientapi.ts (1)
RpcApi(602-602)frontend/app/store/wshrpcutil.ts (1)
TabRpcClient(37-37)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: merge-gatekeeper
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
frontend/app/view/term/termwrap.ts (3)
11-11: Remove unused import.The
stringToBase64import appears unused. The image paste implementation relies onFileReader.readAsDataURL, which already produces base64-encoded output.Apply this diff:
-import { base64ToArray, base64ToString, fireAndForget, stringToBase64 } from "@/util/util"; +import { base64ToArray, base64ToString, fireAndForget } from "@/util/util";
712-720: Consider simplifying the wrapper.This method simply extracts a blob and delegates to
handleImagePasteBlob. Since it's only called once (line 461), you could inline it.However, keeping the wrapper maintains clear separation between the Clipboard API path and the blob processing logic, which may be preferred for readability.
707-709: Consider user feedback for paste failures.Errors during image paste are logged but provide no user-facing feedback. Users may not understand why their paste didn't work.
Consider displaying a toast notification or status message when image paste fails, especially for the size limit (line 678).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/app/view/term/termwrap.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/app/view/term/termwrap.ts (2)
frontend/app/store/wshclientapi.ts (1)
RpcApi(602-602)frontend/app/store/wshrpcutil.ts (1)
TabRpcClient(37-37)
🔇 Additional comments (2)
frontend/app/view/term/termwrap.ts (2)
424-486: LGTM! Clipboard handling is robust.The async paste handler correctly prioritizes image data, implements multiple fallback paths, and properly manages the
pasteActiveflag. The use ofhandleImagePasteBlobavoids the stack overflow issue mentioned in previous reviews.
667-672: LGTM!The method correctly enables image paste support for all terminals by returning
trueunconditionally.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/app/view/term/term-model.ts(2 hunks)frontend/app/view/term/termwrap.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/app/view/term/termwrap.ts (2)
frontend/app/store/wshclientapi.ts (1)
RpcApi(602-602)frontend/app/store/wshrpcutil.ts (1)
TabRpcClient(37-37)
frontend/app/view/term/term-model.ts (2)
frontend/app/store/wshclientapi.ts (1)
RpcApi(602-602)frontend/app/store/wshrpcutil.ts (1)
TabRpcClient(37-37)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
frontend/app/view/term/termwrap.ts (1)
696-698: Consider stronger random generation for filename uniqueness.
Math.random()provides weak randomness and could lead to collisions in high-frequency paste scenarios. Consider usingcrypto.randomUUID()or a cryptographically secure random string.Apply this diff:
- const timestamp = Date.now(); - const random = Math.random().toString(36).substring(2, 8); - const filename = `waveterm_paste_${timestamp}_${random}.${ext}`; + const timestamp = Date.now(); + const random = crypto.randomUUID().split('-')[0]; // or use full UUID + const filename = `waveterm_paste_${timestamp}_${random}.${ext}`;frontend/app/view/term/term-model.ts (1)
463-466: Consider stronger random generation for filename uniqueness.Similar to the implementation in termwrap.ts,
Math.random()provides weak randomness. Consider usingcrypto.randomUUID()for better uniqueness and to avoid potential collisions.Apply this diff:
- const timestamp = Date.now(); - const random = Math.random().toString(36).substring(2, 8); - const filename = `waveterm_paste_${timestamp}_${random}.${ext}`; + const timestamp = Date.now(); + const random = crypto.randomUUID().split('-')[0]; + const filename = `waveterm_paste_${timestamp}_${random}.${ext}`;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
frontend/app/store/wshclientapi.ts(1 hunks)frontend/app/view/term/term-model.ts(2 hunks)frontend/app/view/term/termwrap.ts(2 hunks)pkg/wshrpc/wshclient/wshclient.go(1 hunks)pkg/wshrpc/wshrpctypes.go(2 hunks)pkg/wshrpc/wshserver/wshserver.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-22T01:28:41.417Z
Learnt from: esimkowitz
Repo: wavetermdev/waveterm PR: 1790
File: pkg/remote/fileshare/wshfs/wshfs.go:122-122
Timestamp: 2025-01-22T01:28:41.417Z
Learning: The RpcClient in pkg/remote/fileshare/wshfs/wshfs.go is initialized and handled downstream by either main-server or wshcmd-connserver, as documented in the package comment.
Applied to files:
frontend/app/store/wshclientapi.tspkg/wshrpc/wshclient/wshclient.go
🧬 Code graph analysis (6)
frontend/app/store/wshclientapi.ts (2)
frontend/app/store/wshclient.ts (1)
WshClient(159-159)pkg/wshrpc/wshrpctypes.go (1)
RpcOpts(329-335)
pkg/wshrpc/wshrpctypes.go (2)
frontend/app/store/wshclientapi.ts (1)
GetTempDirCommand(256-258)pkg/wshrpc/wshclient/wshclient.go (1)
GetTempDirCommand(313-315)
pkg/wshrpc/wshserver/wshserver.go (2)
frontend/app/store/wshclientapi.ts (1)
GetTempDirCommand(256-258)pkg/wshrpc/wshclient/wshclient.go (1)
GetTempDirCommand(313-315)
frontend/app/view/term/termwrap.ts (2)
frontend/app/store/wshclientapi.ts (1)
RpcApi(607-607)frontend/app/store/wshrpcutil.ts (1)
TabRpcClient(37-37)
pkg/wshrpc/wshclient/wshclient.go (3)
frontend/app/store/wshclientapi.ts (1)
GetTempDirCommand(256-258)pkg/wshutil/wshrpc.go (1)
WshRpc(47-61)pkg/wshrpc/wshrpctypes.go (1)
RpcOpts(329-335)
frontend/app/view/term/term-model.ts (2)
frontend/app/store/wshclientapi.ts (1)
RpcApi(607-607)frontend/app/store/wshrpcutil.ts (1)
TabRpcClient(37-37)
🔇 Additional comments (7)
frontend/app/view/term/termwrap.ts (1)
700-702: Verify path separator handling across platforms.Line 702 uses a forward slash
/to join the temp directory and filename. While this typically works on Windows when handled by Node.js/Electron file APIs, ensure the backend RPCFileWriteCommandnormalizes paths correctly on all platforms.pkg/wshrpc/wshserver/wshserver.go (1)
447-449: LGTM!The implementation correctly uses
os.TempDir()which is cross-platform and returns the appropriate temporary directory for each OS (e.g.,/tmpon Unix-like systems,%TEMP%on Windows).frontend/app/store/wshclientapi.ts (1)
255-258: LGTM!The RPC client method follows the established pattern for commands with no input data, correctly passing
nullfor the data parameter and returning aPromise<string>.pkg/wshrpc/wshrpctypes.go (1)
86-86: LGTM!The command constant and interface method are correctly defined and follow the established naming conventions and patterns in the codebase.
Also applies to: 216-216
pkg/wshrpc/wshclient/wshclient.go (1)
312-315: LGTM!The client wrapper correctly follows the pattern for commands with no input data. Note that this file is generated code (line 4), so any issues should be addressed in the code generator rather than here.
frontend/app/view/term/term-model.ts (2)
602-608: Verify Shift+Enter behavior change is documented.The default value for
shiftEnterNewlineEnabledhas changed fromfalsetotrue(line 602), and the implementation now sends a standard newline\n(line 604) instead of the previous escape sequence. This is a breaking change in default behavior that users may notice.Ensure this behavior change is:
- Documented in release notes or changelog
- Communicated to users who may have workflows dependent on the old behavior
- Tested with Claude Code and other interactive CLI tools mentioned in the PR objectives
468-470: Verify path separator handling.Line 470 uses a forward slash
/to join paths. While this typically works cross-platform in Node.js/Electron, verify that theFileWriteCommandRPC properly handles path separators on Windows.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
frontend/app/view/term/term-model.ts (2)
475-475: Validate data URL format before splitting.The code assumes the data URL will always contain a comma separator. While this should always be true for valid FileReader output, defensive validation would improve robustness.
// Extract base64 data from data URL (remove "data:image/png;base64," prefix) - const base64Data = dataUrl.split(',')[1]; + const parts = dataUrl.split(','); + if (parts.length < 2) { + throw new Error("Invalid data URL format"); + } + const base64Data = parts[1];
445-455: Consider extracting MIME type mapping as a constant.The MIME-to-extension mapping is comprehensive but could be extracted as a class-level or module-level constant for reusability and maintainability, especially if other components need similar mappings.
+ private static readonly MIME_TO_EXT: Record<string, string> = { + "image/png": "png", + "image/jpeg": "jpg", + "image/jpg": "jpg", + "image/gif": "gif", + "image/webp": "webp", + "image/bmp": "bmp", + "image/svg+xml": "svg", + "image/tiff": "tiff", + }; + async handleImagePasteBlob(blob: Blob): Promise<void> { try { // Check size limit (5MB) if (blob.size > 5 * 1024 * 1024) { console.error("Image too large (>5MB):", blob.size); return; } - // Map MIME types to file extensions - const mimeToExt: Record<string, string> = { - "image/png": "png", - "image/jpeg": "jpg", - "image/jpg": "jpg", - "image/gif": "gif", - "image/webp": "webp", - "image/bmp": "bmp", - "image/svg+xml": "svg", - "image/tiff": "tiff", - }; - const ext = mimeToExt[blob.type] || "png"; + const ext = TermViewModel.MIME_TO_EXT[blob.type] || "png";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/app/view/term/term-model.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/app/view/term/term-model.ts (2)
frontend/app/store/wshclientapi.ts (1)
RpcApi(607-607)frontend/app/store/wshrpcutil.ts (1)
TabRpcClient(37-37)
🔇 Additional comments (2)
frontend/app/view/term/term-model.ts (2)
394-427: LGTM! Clean clipboard handling with proper fallbacks.The implementation correctly prioritizes image paste for supported terminals, falls back to text paste, and includes robust error handling. The previous debug logging statements have been successfully removed.
596-598: LGTM! Shift+Enter and paste handling correctly implemented.The changes successfully implement the PR objectives:
- Shift+Enter default changed to
true(line 596), enabling newline insertion by default- Newline character
\nis sent correctly (line 598)- Paste trigger properly integrated with new
handlePaste()method (line 605)Also applies to: 605-605
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/app/view/term/term-model.ts (1)
37-46: Consider extracting duplicated MIME_TO_EXT constant.This mapping is identical to the one in
frontend/app/view/term/termwrap.ts(lines 330-339). See the comment on that file for a suggested refactor to share this constant.
🧹 Nitpick comments (2)
frontend/app/view/term/termwrap.ts (1)
330-339: Consider extracting MIME_TO_EXT to a shared constant.This exact mapping is duplicated in
frontend/app/view/term/term-model.ts(lines 37-46). Consider extracting it to a shared utility module to maintain a single source of truth.For example, create a new file
frontend/app/view/term/termutil.ts:export const MIME_TO_EXT: Record<string, string> = { "image/png": "png", "image/jpeg": "jpg", "image/jpg": "jpg", "image/gif": "gif", "image/webp": "webp", "image/bmp": "bmp", "image/svg+xml": "svg", "image/tiff": "tiff", };Then import it in both files. However, this is optional since the duplication is minimal and localized.
frontend/app/view/term/term-model.ts (1)
447-493: Consider extracting shared image paste logic.This implementation is nearly identical to
termwrap.tslines 685-731 (only difference:this.termRef.current?.terminal.paste()vsthis.terminal.paste()). Since both classes handle image pasting the same way, consider extracting the core logic into a shared utility function to reduce duplication and maintenance burden.Example refactor:
// In termutil.ts or similar export async function createTempFileFromBlob( blob: Blob, MIME_TO_EXT: Record<string, string>, client: WshClient ): Promise<string> { if (blob.size > 5 * 1024 * 1024) { throw new Error("Image too large (>5MB)"); } const ext = MIME_TO_EXT[blob.type] || "png"; const timestamp = Date.now(); const random = Math.random().toString(36).substring(2, 8); const filename = `waveterm_paste_${timestamp}_${random}.${ext}`; const tempPath = await RpcApi.GetTempDirCommand(client, { filename }); const dataUrl = await new Promise<string>((resolve, reject) => { const reader = new FileReader(); reader.onload = () => resolve(reader.result as string); reader.onerror = reject; reader.readAsDataURL(blob); }); const parts = dataUrl.split(","); if (parts.length < 2) { throw new Error("Invalid data URL format"); } await RpcApi.FileWriteCommand(client, { info: { path: tempPath }, data64: parts[1], }); return tempPath; }Then both classes can call this shared function and just handle the terminal paste differently.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
frontend/app/store/wshclientapi.ts(1 hunks)frontend/app/view/term/term-model.ts(3 hunks)frontend/app/view/term/termwrap.ts(3 hunks)frontend/types/gotypes.d.ts(1 hunks)pkg/wshrpc/wshclient/wshclient.go(1 hunks)pkg/wshrpc/wshrpctypes.go(3 hunks)pkg/wshrpc/wshserver/wshserver.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/wshrpc/wshclient/wshclient.go
- pkg/wshrpc/wshrpctypes.go
🧰 Additional context used
🧬 Code graph analysis (5)
frontend/types/gotypes.d.ts (1)
pkg/wshrpc/wshrpctypes.go (1)
CommandGetTempDirData(609-611)
pkg/wshrpc/wshserver/wshserver.go (3)
frontend/app/store/wshclientapi.ts (1)
GetTempDirCommand(306-308)pkg/wshrpc/wshclient/wshclient.go (1)
GetTempDirCommand(372-375)pkg/wshrpc/wshrpctypes.go (1)
CommandGetTempDirData(609-611)
frontend/app/store/wshclientapi.ts (1)
pkg/wshrpc/wshrpctypes.go (2)
CommandGetTempDirData(609-611)RpcOpts(329-335)
frontend/app/view/term/termwrap.ts (2)
frontend/app/store/wshclientapi.ts (1)
RpcApi(607-607)frontend/app/store/wshrpcutil.ts (1)
TabRpcClient(37-37)
frontend/app/view/term/term-model.ts (2)
frontend/app/store/wshclientapi.ts (1)
RpcApi(607-607)frontend/app/store/wshrpcutil.ts (1)
TabRpcClient(37-37)
🔇 Additional comments (11)
frontend/types/gotypes.d.ts (1)
259-262: LGTM! Type definition correctly mirrors backend struct.The generated TypeScript type properly represents the Go struct with an optional
filenamefield, enabling platform-aware temp file path construction.frontend/app/store/wshclientapi.ts (1)
305-308: LGTM! RPC command follows established patterns.The
GetTempDirCommandimplementation is consistent with other RPC methods in the file and correctly typed.pkg/wshrpc/wshserver/wshserver.go (1)
447-453: LGTM! Platform-aware temp directory implementation.The implementation correctly uses
os.TempDir()andfilepath.Join()to ensure cross-platform compatibility, addressing the Windows path separator concerns from previous reviews.frontend/app/view/term/termwrap.ts (4)
435-497: LGTM! Robust paste handling with proper fallbacks.The async paste handler correctly:
- Prioritizes image detection via
ClipboardEvent.clipboardData(Electron compatibility)- Falls back to the Clipboard API for modern browsers
- Includes text paste fallback on errors
- Properly manages
pasteActivestate with a finally block
678-683: LGTM! Clear and well-documented.The method appropriately returns
truefor all terminals with a clear comment explaining the design decision.
685-731: LGTM! Comprehensive image paste implementation.This implementation correctly addresses all previous review concerns:
- ✅ 5 MB size limit prevents excessive memory usage
- ✅ Unique filename with timestamp + random component prevents collisions
- ✅ Platform-aware temp path via
GetTempDirCommandbackend RPC- ✅ Proper base64 conversion using FileReader
- ✅ Error handling with console.error logging
- ✅ UX: Trailing space after path for easier command composition
733-741: LGTM! Clean wrapper with error handling.The method is a straightforward adapter that extracts the blob and delegates to
handleImagePasteBlob.frontend/app/view/term/term-model.ts (4)
405-438: LGTM! Clean paste handling without debug statements.The implementation:
- Correctly prioritizes image pastes over text
- Includes proper error handling with double fallback (clipboard.read → clipboard.readText)
- No debug console.log statements (addressed previous review feedback)
440-445: LGTM! Consistent with termwrap implementation.The method has the same clear design as the termwrap version.
600-602: Verify Shift+Enter default change is intentional.The default value for
shiftenternewlinehas changed fromfalse(implicit) totrue(explicit via?? true). This means Shift+Enter will now insert newlines by default instead of executing commands, which is a breaking behavioral change for existing users.Please confirm this is the intended behavior per the PR objectives. If so, consider:
- Documenting this change in release notes
- Adding a migration notice for users who relied on the old behavior
- Ensuring the config setting
term:shiftenternewlineallows users to opt back into the old behavior
609-609: LGTM! Paste trigger correctly updated.The change from direct text paste to calling
handlePaste()enables the new image paste functionality while maintaining text paste as a fallback.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/app/view/term/termwrap.ts (1)
668-684: Code duplication noted.These methods are duplicated from
term-model.ts. See the refactoring suggestion in the review ofterm-model.tslines 430-446.
🧹 Nitpick comments (1)
frontend/app/view/term/term-model.ts (1)
430-446: Consider extracting duplicated methods to reduce maintenance burden.The
supportsImageInput()andhandleImagePasteBlob()methods are duplicated identically intermwrap.ts(lines 668-684). Consider extracting these to a shared utility module or base class to maintain consistency and reduce duplication.For example, you could move these methods to
termutil.ts:export function supportsImageInput(): boolean { return true; } export async function handleImagePasteBlob( blob: Blob, client: WshClient, terminal: { paste: (text: string) => void } ): Promise<void> { try { const tempPath = await createTempFileFromBlob(blob, client); terminal.paste(tempPath + " "); } catch (err) { console.error("Error pasting image:", err); } }Then import and use them in both files.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/app/view/term/term-model.ts(3 hunks)frontend/app/view/term/termutil.ts(1 hunks)frontend/app/view/term/termwrap.ts(3 hunks)
🔇 Additional comments (7)
frontend/app/view/term/termutil.ts (2)
41-50: LGTM! Good coverage of common image formats.The MIME type mapping covers all the common image formats users are likely to paste.
62-101: Excellent implementation with proper safeguards.The function correctly:
- Enforces a size limit to prevent abuse
- Generates unique filenames using timestamp and random components
- Obtains platform-appropriate temp paths from the backend (addressing previous review feedback)
- Handles errors gracefully
frontend/app/view/term/term-model.ts (3)
395-428: Well-structured paste handling with proper prioritization.The implementation correctly prioritizes image data over text and includes comprehensive error handling with a text-only fallback. The use of the Clipboard API is appropriate for this use case.
551-560: Shift+Enter behavior updated as documented.The changes correctly implement the feature described in the PR:
- Default
shiftenternewlinechanged totrue- Shift+Enter now sends a standard newline (
\n) instead of the escape sequence
561-565: Good integration of image paste support with existing keyboard shortcuts.Updating Ctrl+Shift+V to use
handlePaste()enables image pasting via keyboard while maintaining backward compatibility for text pasting.frontend/app/view/term/termwrap.ts (2)
425-487: Robust clipboard handling with excellent cross-platform support.The async paste handler elegantly handles multiple clipboard APIs:
- ClipboardEvent.clipboardData (Electron)
- Clipboard API (modern browsers)
- Multiple fallback layers for error cases
The finally block ensures proper cleanup of the
pasteActiveflag. All previous review concerns about call stack issues and hardcoded paths have been addressed.
686-694: Clean helper method for Clipboard API integration.The
handleImagePaste()method provides a clean abstraction for processingClipboardItemobjects and properly delegates to the sharedhandleImagePasteBlob()logic.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/app/view/term/termwrap.ts (1)
429-491: Consider whether 30ms is sufficient for async image processing.The
finallyblock clearspasteActiveafter 30ms, but ifhandleImagePasteBlob()takes longer (e.g., large image, slow I/O), the flag might be cleared before the paste operation completes. This could affect the multi-input callback behavior triggered bypasteActiveinhandleTermData(line 570).Consider either:
- Awaiting the image paste operation and clearing
pasteActiveimmediately after- Increasing the timeout if empirical testing shows 30ms is insufficient
- Using a more deterministic completion signal
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/app/view/term/term-model.ts(3 hunks)frontend/app/view/term/termutil.ts(1 hunks)frontend/app/view/term/termwrap.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/app/view/term/termutil.ts
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/app/view/term/termwrap.ts (1)
frontend/app/store/wshrpcutil.ts (1)
TabRpcClient(37-37)
🔇 Additional comments (8)
frontend/app/view/term/termwrap.ts (3)
21-25: LGTM! Clean import additions.The imports from termutil are appropriately scoped and used by the new image paste handling methods below.
672-690: LGTM! Clean delegation to termutil.The methods appropriately delegate image handling to the utility layer and provide the terminal paste callback. Error handling in
handleImagePasteis properly included.
676-679: 5 MB size limit is properly enforced.Verification confirms that
handleImagePasteBlobUtilcallscreateTempFileFromBlob(in termutil.ts, line 127), which includes the 5 MB validation at line 64 with the checkif (blob.size > 5 * 1024 * 1024)and throws an error with appropriate user feedback: "Image too large (>5MB)".frontend/app/view/term/term-model.ts (5)
32-38: LGTM! Import additions align with new functionality.The extended imports from termutil provide the necessary utilities for image paste handling and are appropriately used in the methods below.
401-434: LGTM! Clean image-first paste handling.The implementation properly prioritizes image paste over text and includes appropriate fallbacks. The use of Clipboard API (without ClipboardData) differs from
termwrap.tsbut is appropriate for this context where there's no ClipboardEvent available.
436-444: LGTM! Consistent delegation pattern.The methods appropriately delegate to termutil and provide the terminal paste callback through
termRef.current, maintaining consistency with the termwrap implementation.
551-553: LGTM! Shift+Enter behavior updated as intended.The default for
shiftenternewlinechanged totrue, and the newline character is now standard\ninstead of\u001b\n. This aligns with the PR objectives to improve input handling for interactive CLI tools.
560-560: LGTM! Keyboard shortcut integrated with new paste flow.The Ctrl+Shift+V handler now delegates to
handlePaste(), enabling the image-first paste behavior through the keyboard shortcut.
|
@togo01 thanks for working on this. i think you're right that we should default the Shift+Enter case to work better with claude code rather than have it default to the less friendly version. but i'm not sure about this change: i think sending the ESC + "\n" is right (basically sends an Alt+Enter). When I test with Claude Code, the ESC+\n inserts the newline correctly without submitting (also the same way that VSCode solved it with /term-setup). Are you seeing something different? |
Thanks for the feedback! You're absolutely right - I've reverted the change back to use \u001b\n (ESC + newline). |
Enable Shift+Enter to insert newlines in terminal by default, improving multi-line input support for interactive CLI tools like Claude Code. Changes: - Change default shiftenternewline config from false to true - Send standard newline character (\n) instead of escape sequence (\u001b\n) This allows users to input multi-line commands and text without requiring manual configuration changes.
Add support for pasting images in terminal by saving them as temporary files and pasting the file path, similar to iTerm2 behavior. This enables AI tools like Claude Code to receive and process pasted images by reading the temporary file path. Changes: - Add handlePaste() method to intercept Cmd+Shift+V paste events - Add handleImagePasteBlob() to save images to /tmp and paste path - Detect image data in clipboard using ClipboardEvent and Clipboard API - Support both screenshot paste and file copy scenarios - Add 5MB size limit for pasted images The implementation creates temporary files in /tmp with format: waveterm_paste_[timestamp].[ext]
Reuse handleImagePasteBlob() method instead of manually converting blob to base64 with String.fromCharCode(), which causes RangeError for images larger than ~64KB. This ensures consistent behavior and prevents stack overflow errors when pasting large screenshots. Fixes: CodeRabbit review feedback
Address CodeRabbit review feedback: - Use MIME type lookup table instead of split() to handle complex types like image/svg+xml correctly - Add random component to filename to prevent collisions when pasting multiple images in quick succession - Remove unused stringToBase64 import from termwrap.ts - Add TODO comment documenting cross-platform temp directory limitation The /tmp/ path currently works on macOS/Linux. Windows support would require backend API to provide platform-appropriate temp directory.
Add GetTempDirCommand RPC to provide platform-appropriate temp directory paths from the backend, replacing hardcoded /tmp/ paths. This ensures image paste works correctly on: - macOS: /tmp or /var/folders/... - Linux: /tmp or $TMPDIR - Windows: %TEMP% or %LOCALAPPDATA%\Temp Backend changes: - Add GetTempDirCommand to wshrpc interface (wshrpctypes.go) - Implement GetTempDirCommand using os.TempDir() (wshserver.go) - Add client wrapper in wshclient.go Frontend changes: - Add GetTempDirCommand to RpcApi (wshclientapi.ts) - Update handleImagePasteBlob to use GetTempDirCommand in term-model.ts - Update handleImagePasteBlob to use GetTempDirCommand in termwrap.ts Addresses CodeRabbit review feedback about cross-platform compatibility.
Remove temporary debug logging from handlePaste() method: - Remove handlePaste() called log - Remove clipboard items count log - Remove clipboard item types log - Remove found image processing log - Remove pasting text length log Preserve console.error statements for actual error reporting. Addresses CodeRabbit review feedback.
Address three critical review items: 1. Extract MIME type mapping as class constants - Add MIME_TO_EXT static readonly in TermViewModel and TermWrap - Improves reusability and maintainability 2. Validate data URL format before splitting - Add defensive check for comma separator in data URL - Throw error if format is invalid - Prevents potential undefined access 3. Fix cross-platform path joining issue - Extend GetTempDirCommand to accept optional filename parameter - Backend now joins paths using filepath.Join (platform-aware) - Eliminates hardcoded '/' separator that breaks on Windows Backend changes: - Add CommandGetTempDirData struct with optional FileName field - Update GetTempDirCommand signature and implementation - Use filepath.Join for cross-platform path handling Frontend changes: - Extract MIME type mappings to static constants - Pass filename to GetTempDirCommand instead of manual joining - Add data URL format validation with error handling Addresses CodeRabbit PR review feedback.
Extract MIME type mapping and image paste logic to shared termutil module: - Move MIME_TO_EXT constant from TermViewModel and TermWrap to termutil.ts - Create createTempFileFromBlob() utility function to handle image blob processing - Refactor both classes to use shared utilities, reducing code duplication - Maintain all functionality: size validation, format detection, temp file creation This addresses CodeRabbit review feedback about code duplication.
Further reduce code duplication by extracting supportsImageInput() and handleImagePasteBlob() methods to termutil.ts: - Add supportsImageInput() utility function - Add handleImagePasteBlob() utility with callback for terminal paste - Update both TermViewModel and TermWrap to delegate to shared utilities - Maintain all functionality while eliminating duplicate method implementations This addresses additional CodeRabbit review feedback about method duplication.
Use escape sequence + newline (\u001b\n) instead of plain newline (\n) for Shift+Enter, following the terminal standard and matching VSCode's /term-setup implementation. This ensures proper multi-line input support in Claude Code and other AI coding tools without accidentally submitting incomplete commands.
Fix duplicate text input when switching input methods via Capslock during Chinese/Japanese/Korean IME composition. ## Problem When using Chinese Zhuyin IME in terminal, switching to English mode with Capslock during composition causes the composed text to be sent twice, resulting in duplicate output. ## Solution ### IME Composition Handling - Add IME composition state tracking in TermWrap (isComposing, etc.) - Register compositionstart/update/end event listeners on xterm textarea - Block all data sends during composition, only allow after compositionend - Prevents xterm.js from sending data during compositionupdate phase ### Deduplication Logic - Implement 50ms time window deduplication mechanism - Track first send, block duplicate send from Capslock switching - Ensure Ctrl+Space and Fn switching work correctly (single send) ### Paste Deduplication - Fix duplicate paste issue caused by xterm.js paste() method - paste() triggers onData event, causing handleTermData to be called twice - Use pasteActive flag and time window to allow only first send ### Edge Cases - Add blur event handler to reset composition state on focus loss - Add Escape key handling in term-model.ts to cancel composition ## Test Results ✅ macOS Zhuyin + Capslock switching - no duplicate ✅ macOS Zhuyin + Ctrl+Space switching - normal input ✅ macOS Zhuyin + Fn switching - normal input ✅ English input - normal ✅ Shift+Enter multiline - normal ✅ Text paste - no duplicate ✅ Image paste - normal ✅ Basic commands - normal
togo01
force-pushed
the
feature/terminal-input-improvements
branch
from
7343252 to
1cf388f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
frontend/app/view/term/term-model.ts (1)
32-38: Remove unused import.
createTempFileFromBlobisn’t used in this file.import { computeTheme, - createTempFileFromBlob, DefaultTermTheme, handleImagePasteBlob as handleImagePasteBlobUtil, supportsImageInput as supportsImageInputUtil, } from "./termutil";frontend/app/view/term/termutil.ts (2)
41-50: Broaden MIME coverage.Add common types like AVIF/HEIC/HEIF/ICO for better cross-platform paste coverage.
export const MIME_TO_EXT: Record<string, string> = { "image/png": "png", "image/jpeg": "jpg", "image/jpg": "jpg", "image/gif": "gif", "image/webp": "webp", "image/bmp": "bmp", "image/svg+xml": "svg", "image/tiff": "tiff", + "image/avif": "avif", + "image/heic": "heic", + "image/heif": "heif", + "image/x-icon": "ico", + "image/vnd.microsoft.icon": "ico", };
62-101: Use stronger unique ID for filenames.Prefer
crypto.randomUUID()when available to avoid rare collisions.- const timestamp = Date.now(); - const random = Math.random().toString(36).substring(2, 8); - const filename = `waveterm_paste_${timestamp}_${random}.${ext}`; + const timestamp = Date.now(); + const rand = + typeof crypto !== "undefined" && typeof crypto.randomUUID === "function" + ? crypto.randomUUID() + : Math.random().toString(36).slice(2, 10); + const filename = `waveterm_paste_${timestamp}_${rand}.${ext}`;frontend/app/view/term/termwrap.ts (1)
637-687: IME and paste dedup logic is sensible.Blocking during composition, 50 ms dedup windows for paste and IME duplicates are reasonable defaults.
Consider centralizing
DEDUP_WINDOW_MSin a module constant shared with related handlers for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
frontend/app/store/wshclientapi.ts(1 hunks)frontend/app/view/term/term-model.ts(4 hunks)frontend/app/view/term/termutil.ts(1 hunks)frontend/app/view/term/termwrap.ts(7 hunks)frontend/types/gotypes.d.ts(1 hunks)pkg/wshrpc/wshclient/wshclient.go(1 hunks)pkg/wshrpc/wshrpctypes.go(3 hunks)pkg/wshrpc/wshserver/wshserver.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/app/store/wshclientapi.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-22T01:28:41.417Z
Learnt from: esimkowitz
Repo: wavetermdev/waveterm PR: 1790
File: pkg/remote/fileshare/wshfs/wshfs.go:122-122
Timestamp: 2025-01-22T01:28:41.417Z
Learning: The RpcClient in pkg/remote/fileshare/wshfs/wshfs.go is initialized and handled downstream by either main-server or wshcmd-connserver, as documented in the package comment.
Applied to files:
pkg/wshrpc/wshclient/wshclient.go
🧬 Code graph analysis (7)
pkg/wshrpc/wshserver/wshserver.go (3)
frontend/app/store/wshclientapi.ts (1)
GetTempDirCommand(331-333)pkg/wshrpc/wshclient/wshclient.go (1)
GetTempDirCommand(402-405)pkg/wshrpc/wshrpctypes.go (1)
CommandGetTempDirData(627-629)
pkg/wshrpc/wshclient/wshclient.go (3)
frontend/app/store/wshclientapi.ts (1)
GetTempDirCommand(331-333)pkg/wshutil/wshrpc.go (1)
WshRpc(47-61)pkg/wshrpc/wshrpctypes.go (2)
CommandGetTempDirData(627-629)RpcOpts(347-353)
frontend/types/gotypes.d.ts (1)
pkg/wshrpc/wshrpctypes.go (1)
CommandGetTempDirData(627-629)
pkg/wshrpc/wshrpctypes.go (2)
frontend/app/store/wshclientapi.ts (1)
GetTempDirCommand(331-333)pkg/wshrpc/wshclient/wshclient.go (1)
GetTempDirCommand(402-405)
frontend/app/view/term/term-model.ts (2)
frontend/app/store/wshrpcutil.ts (1)
TabRpcClient(37-37)frontend/app/store/global.ts (1)
getOverrideConfigAtom(834-834)
frontend/app/view/term/termutil.ts (3)
frontend/app/store/wshclientapi.ts (1)
RpcApi(637-637)frontend/app/view/term/term-model.ts (2)
supportsImageInput(436-438)handleImagePasteBlob(440-444)frontend/app/view/term/termwrap.ts (2)
supportsImageInput(784-786)handleImagePasteBlob(788-792)
frontend/app/view/term/termwrap.ts (1)
frontend/app/store/wshrpcutil.ts (1)
TabRpcClient(37-37)
🔇 Additional comments (10)
frontend/types/gotypes.d.ts (1)
282-286: New RPC type addition looks correct.
filename?: stringaligns with Go’sjson:"filename,omitempty". No issues.pkg/wshrpc/wshclient/wshclient.go (1)
401-405: Client wrapper LGTM.Consistent with other RPC wrappers; generic type and command string are correct.
pkg/wshrpc/wshrpctypes.go (1)
86-86: RPC plumbing is consistent end-to-end.Command constant, interface, and data struct align with client/server and TS types.
Also applies to: 226-227, 627-630
frontend/app/view/term/term-model.ts (2)
401-434: Clipboard handling LGTM with graceful fallback.Image-first via
navigator.clipboard.read()and text fallback are correct; errors are handled.Please verify Electron permission behavior on Linux/Windows where
navigator.clipboard.read()may be restricted and confirm the text fallback path triggers as expected.
545-551: IME Escape reset and Shift+Enter behavior look right.ESC resets composition state; Shift+Enter sends ESC+LF with default enabled.
Also applies to: 560-570
frontend/app/view/term/termutil.ts (1)
121-134: Helper flow LGTM.Temp-file creation + paste callback with error logging is clean.
frontend/app/view/term/termwrap.ts (4)
359-372: IME state scaffolding LGTM.Clear flags tracked for composition and first-send dedup.
444-506: Robust paste pipeline.Prioritizes images via ClipboardEvent and falls back to Clipboard API/text. Dedup flagging via
pasteActiveis correct.
561-586: Composition listeners + blur reset LGTM.Attaching to xterm textarea and cleaning up on dispose is correct.
784-802: Image helpers integration LGTM.Reuses termutil helpers; pastes resulting path into terminal.
Fix critical path traversal vulnerability where attackers could use ../ sequences or absolute paths to access files outside the temp directory. Now sanitizes input by extracting only the base filename and rejecting traversal attempts.
|
overall this looks good, tested both the image pasting and the Shift+Enter code! i'm going to merge it as-is and continue testing a bit more... i can fix anything that comes up in a follow-up PR rather than waiting forever to get it merged. |
Thanks for the quick review and merge. I'm pleased to see this feature/fix incorporated. |
3 participants
Summary
This PR includes three terminal input improvements that enhance the experience when using interactive CLI tools and IME input:
Motivation
Shift+Enter for newlines
Currently, pressing Shift+Enter in the terminal behaves the same as Enter, making it difficult to input multi-line commands or text in interactive CLI tools. This change enables Shift+Enter to insert newlines by default, matching common terminal emulator behavior.
Image paste support
Interactive AI tools like Claude Code support receiving images through file paths, but Wave Terminal currently doesn't support pasting images. This change implements image paste functionality similar to iTerm2's behavior: when an image is pasted, it's saved to a temporary file and the path is pasted into the terminal.
IME duplicate input fix
When using Chinese/Japanese/Korean IME in the terminal, switching input methods with Capslock during composition causes the composed text to be sent twice, resulting in duplicate output (e.g., "你好" becomes "你好你好"). This issue severely impacts users who frequently switch between languages.
Changes
Shift+Enter newline (
frontend/app/view/term/term-model.ts)shiftenternewlineconfig fromfalsetotrue\n) instead of escape sequence (\^[\n)Image paste (
frontend/app/view/term/term-model.ts,frontend/app/view/term/termwrap.ts)handlePaste()method to intercept Cmd+Shift+V paste eventshandleImagePasteBlob()to save images to/tmpand paste the file pathClipboardEvent.clipboardDataand Clipboard APIwaveterm_paste_[timestamp].[ext]IME duplicate input fix (
frontend/app/view/term/termwrap.ts,frontend/app/view/term/term-model.ts)IME Composition Handling:
Deduplication Logic:
Edge Case Handling:
Testing
Shift+Enter
Image paste
IME Input Testing
IME Input:
Regression Testing:
Demo
2025-11-05.15-32-58.mp4
2025-11-07.04-09-29.mp4
2025-11-07.04-10-26.mp4
All features have been tested successfully on macOS with Claude Code in Wave Terminal.