Big Paste Handler Cleanup #2535
Conversation
cleanup base64 encoding flow etc use \n instead of ESC-\n for shift+enter...
|
Caution Review failedThe pull request is closed. WalkthroughAdds a "native-paste" IPC handler in the main process and exposes it via the preload API as api.nativePaste(). Refactors terminal clipboard handling: introduces extractClipboardData and extractAllClipboardData, centralizes paste logic into TermWrap.pasteHandler, removes old image-paste helpers and direct WshClient dependency for blob→temp-file, and expands MIME mappings. Adds a new WriteTempFile RPC (types, client wrapper, and server implementation) that accepts base64 data and returns a temp file path. Multiple React ref typings changed from MutableRefObject to RefObject. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–60 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (7)
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: 4
🧹 Nitpick comments (1)
frontend/types/custom.d.ts (1)
126-126: Optional: make nativePaste awaitable if needed.If UI needs to react after paste completes or handle errors, switch to ipcRenderer.invoke/ipcMain.handle returning a result. Otherwise this is fine.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
emain/emain-ipc.ts(1 hunks)emain/preload.ts(1 hunks)frontend/app/store/wshclientapi.ts(1 hunks)frontend/app/view/term/term-model.ts(3 hunks)frontend/app/view/term/termutil.ts(4 hunks)frontend/app/view/term/termwrap.ts(4 hunks)frontend/types/custom.d.ts(1 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)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-01T00:57:23.025Z
Learnt from: sawka
Repo: wavetermdev/waveterm PR: 2504
File: frontend/app/aipanel/aipanel-contextmenu.ts:15-16
Timestamp: 2025-11-01T00:57:23.025Z
Learning: In the waveterm codebase, types defined in custom.d.ts are globally available and do not require explicit imports. Backend types defined in gotypes.d.ts are also globally available.
Applied to files:
frontend/types/gotypes.d.ts
📚 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/wshserver/wshserver.gopkg/wshrpc/wshclient/wshclient.go
🧬 Code graph analysis (8)
frontend/types/gotypes.d.ts (1)
pkg/wshrpc/wshrpctypes.go (1)
CommandWriteTempFileData(633-636)
pkg/wshrpc/wshserver/wshserver.go (3)
frontend/app/store/wshclientapi.ts (1)
WriteTempFileCommand(616-618)pkg/wshrpc/wshclient/wshclient.go (1)
WriteTempFileCommand(735-738)pkg/wshrpc/wshrpctypes.go (1)
CommandWriteTempFileData(633-636)
frontend/app/store/wshclientapi.ts (3)
frontend/app/store/wshclient.ts (1)
WshClient(159-159)pkg/remote/fileshare/wshfs/wshfs.go (1)
WshClient(22-22)pkg/wshrpc/wshrpctypes.go (2)
CommandWriteTempFileData(633-636)RpcOpts(349-355)
pkg/wshrpc/wshrpctypes.go (2)
frontend/app/store/wshclientapi.ts (1)
WriteTempFileCommand(616-618)pkg/wshrpc/wshclient/wshclient.go (1)
WriteTempFileCommand(735-738)
pkg/wshrpc/wshclient/wshclient.go (3)
frontend/app/store/wshclientapi.ts (1)
WriteTempFileCommand(616-618)pkg/wshutil/wshrpc.go (1)
WshRpc(47-61)pkg/wshrpc/wshrpctypes.go (2)
CommandWriteTempFileData(633-636)RpcOpts(349-355)
frontend/app/view/term/term-model.ts (2)
frontend/app/block/blocktypes.ts (1)
BlockNodeModel(7-12)frontend/app/view/term/termwrap.ts (1)
TermWrap(332-772)
frontend/app/view/term/termutil.ts (2)
frontend/app/store/wshclientapi.ts (1)
RpcApi(642-642)frontend/app/store/wshrpcutil.ts (1)
TabRpcClient(37-37)
frontend/app/view/term/termwrap.ts (1)
frontend/app/view/term/termutil.ts (2)
extractAllClipboardData(161-196)createTempFileFromBlob(64-97)
⏰ 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). (3)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Build for TestDriver.ai
🔇 Additional comments (5)
frontend/types/gotypes.d.ts (1)
466-471: LGTM: ambient CommandWriteTempFileData matches Go struct.Fields and optionality align with wshrpc.CommandWriteTempFileData; good addition. Based on learnings.
emain/emain-ipc.ts (1)
403-405: LGTM: native-paste IPC wiring is correct.Handler is minimal and matches the exposed preload API.
emain/preload.ts (1)
65-65: LGTM: preload exposes nativePaste consistently.API surface matches custom.d.ts and emain handler.
frontend/app/store/wshclientapi.ts (1)
615-618: LGTM: RPC wrapper for writetempfile follows conventions.Signature and routing align with server method.
pkg/wshrpc/wshclient/wshclient.go (1)
734-739: LGTM: client wrapper aligns with server RPC shape.Consistent with existing helpers and command string.
| if (data.image && SupportsImageInput) { | ||
| if (!firstImage) { | ||
| await new Promise((r) => setTimeout(r, 150)); | ||
| } | ||
| const tempPath = await createTempFileFromBlob(data.image); | ||
| this.terminal.paste(tempPath + " "); | ||
| firstImage = false; |
There was a problem hiding this comment.
Quote the temp file path before pasting it into the terminal.
createTempFileFromBlob can easily return something like C:\Users\First Last\AppData\Local\Temp\waveterm_paste.png. Pasting tempPath + " " drops that straight into the shell without quotes, so the space in the user name splits the argument and the command breaks (this happens on default Windows installs all the time). Please wrap the path in quotes (and escape embedded quotes) before calling terminal.paste, so the pasted text is a valid single argument across OSes.
Consider something like:
- const tempPath = await createTempFileFromBlob(data.image);
- this.terminal.paste(tempPath + " ");
+ const tempPath = await createTempFileFromBlob(data.image);
+ const escapedPath = `"${tempPath.replace(/"/g, '\\"')}" `;
+ this.terminal.paste(escapedPath);📝 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.
| if (data.image && SupportsImageInput) { | |
| if (!firstImage) { | |
| await new Promise((r) => setTimeout(r, 150)); | |
| } | |
| const tempPath = await createTempFileFromBlob(data.image); | |
| this.terminal.paste(tempPath + " "); | |
| firstImage = false; | |
| if (data.image && SupportsImageInput) { | |
| if (!firstImage) { | |
| await new Promise((r) => setTimeout(r, 150)); | |
| } | |
| const tempPath = await createTempFileFromBlob(data.image); | |
| const escapedPath = `"${tempPath.replace(/"/g, '\\"')}" `; | |
| this.terminal.paste(escapedPath); | |
| firstImage = false; |
🤖 Prompt for AI Agents
In frontend/app/view/term/termwrap.ts around lines 752-758, the temp file path
returned by createTempFileFromBlob is pasted unquoted which breaks on paths with
spaces (e.g., Windows user profiles); wrap the path in quotes and escape any
embedded quotes before calling this. Specifically, construct a safe string by
replacing any internal double-quotes with an escaped version, surround the
resulting path with double-quotes, preserve the trailing space currently added,
and pass that quoted-and-escaped string to this.terminal.paste so the pasted
text is treated as a single argument across OSes.
| Command_WriteTempFile = "writetempfile" | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
API contract mismatch: filename marked optional in types, but server requires it.
Type and json tags make filename optional, yet server WriteTempFileCommand errors when it’s empty. This will break callers that omit filename.
Run to find call sites that may omit filename:
Also applies to: 228-229, 633-637
🏁 Script executed:
#!/bin/bash
# Show usages of WriteTempFileCommand and nearby args
rg -nC3 '\bWriteTempFileCommand\s*\(' --type=ts
# Grep for payloads without a filename key (heuristic)
rg -nC2 'WriteTempFileCommand\([^,]+,\s*\{[^\}]*\}' --type=ts | rg -nP '(?s)filename\s*:'Length of output: 1012
🏁 Script executed:
#!/bin/bash
# 1. Find CommandWriteTempFileData type definition in TypeScript
echo "=== CommandWriteTempFileData Type Definition ==="
rg -nC5 'interface CommandWriteTempFileData|type CommandWriteTempFileData' --type=ts
# 2. Search for WriteTempFileCommand in all TS/JS files (broader pattern)
echo -e "\n=== All WriteTempFileCommand references ==="
rg -n 'WriteTempFileCommand' --type=ts --type=js
# 3. Find server WriteTempFileCommand implementation in Go
echo -e "\n=== Go server implementation ==="
rg -nC8 'WriteTempFileCommand|writetempfile' --type=goLength of output: 6698
🏁 Script executed:
#!/bin/bash
# Find CommandWriteTempFileData struct definition in Go
echo "=== Go CommandWriteTempFileData struct definition ==="
rg -nC8 'type CommandWriteTempFileData struct' --type=go
# Also check for any json/struct tags
echo -e "\n=== Checking for omitempty or optional tags ==="
rg -nB2 -A4 'CommandWriteTempFileData' --type=go | grep -A4 'type CommandWriteTempFileData'Length of output: 1313
Filename field marked optional in schema but required by server—remove omitempty tag and update TypeScript type.
Type definitions allow omitting filename (Go struct tag json:"filename,omitempty", TS filename?: string), but wshserver.go:471-474 explicitly rejects empty filename with error. This violates the API contract and will break any caller that omits the field.
Fix: Remove omitempty tag from CommandWriteTempFileData.FileName in wshrpctypes.go:634 and make filename required in the TypeScript type at gotypes.d.ts:468.
🤖 Prompt for AI Agents
In pkg/wshrpc/wshrpctypes.go around lines 87-88 (and specifically the
CommandWriteTempFileData struct at ~line 634) the JSON tag for the FileName
field uses `omitempty` which makes filename optional, but the server rejects
empty filenames; remove `omitempty` from the FileName json tag so the field is
always serialized, rebuild or run the type generator to update TypeScript
output, and update gotypes.d.ts at line ~468 to make filename a required string
(remove the optional `?`) so Go and TS schemas match the server contract.
| func (ws *WshServer) WriteTempFileCommand(ctx context.Context, data wshrpc.CommandWriteTempFileData) (string, error) { | ||
| if data.FileName == "" { | ||
| return "", fmt.Errorf("filename is required") | ||
| } | ||
| name := filepath.Base(data.FileName) | ||
| if name == "" || name == "." || name == ".." { | ||
| return "", fmt.Errorf("invalid filename") | ||
| } | ||
| tempDir, err := os.MkdirTemp("", "waveterm-") | ||
| if err != nil { | ||
| return "", fmt.Errorf("error creating temp directory: %w", err) | ||
| } | ||
| decoded, err := base64.StdEncoding.DecodeString(data.Data64) | ||
| if err != nil { | ||
| return "", fmt.Errorf("error decoding base64 data: %w", err) | ||
| } | ||
| tempPath := filepath.Join(tempDir, name) | ||
| err = os.WriteFile(tempPath, decoded, 0600) | ||
| if err != nil { | ||
| return "", fmt.Errorf("error writing temp file: %w", err) | ||
| } | ||
| return tempPath, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
Fix contract + add safety: allow empty filename, cap size, and use os.CreateTemp.
Current impl rejects empty filename (contradicts types) and decodes unbounded data. Generate a secure temp filename when none provided and enforce a max size. Also avoid creating a throwaway directory per call.
Apply this diff:
func (ws *WshServer) WriteTempFileCommand(ctx context.Context, data wshrpc.CommandWriteTempFileData) (string, error) {
- if data.FileName == "" {
- return "", fmt.Errorf("filename is required")
- }
- name := filepath.Base(data.FileName)
- if name == "" || name == "." || name == ".." {
- return "", fmt.Errorf("invalid filename")
- }
- tempDir, err := os.MkdirTemp("", "waveterm-")
- if err != nil {
- return "", fmt.Errorf("error creating temp directory: %w", err)
- }
- decoded, err := base64.StdEncoding.DecodeString(data.Data64)
- if err != nil {
- return "", fmt.Errorf("error decoding base64 data: %w", err)
- }
- tempPath := filepath.Join(tempDir, name)
- err = os.WriteFile(tempPath, decoded, 0600)
- if err != nil {
- return "", fmt.Errorf("error writing temp file: %w", err)
- }
- return tempPath, nil
+ // Guard: estimate decoded size before allocating
+ decodedLen := base64.StdEncoding.DecodedLen(len(data.Data64))
+ if decodedLen > int(wshrpc.MaxFileSize) {
+ return "", fmt.Errorf("file too large: %d > %d", decodedLen, wshrpc.MaxFileSize)
+ }
+
+ // Decode data
+ decoded, err := base64.StdEncoding.DecodeString(data.Data64)
+ if err != nil {
+ return "", fmt.Errorf("error decoding base64 data: %w", err)
+ }
+
+ var tempPath string
+ if strings.TrimSpace(data.FileName) != "" {
+ name := filepath.Base(data.FileName)
+ if name == "" || name == "." || name == ".." {
+ return "", fmt.Errorf("invalid filename")
+ }
+ tempDir, err := os.MkdirTemp("", "waveterm-")
+ if err != nil {
+ return "", fmt.Errorf("error creating temp directory: %w", err)
+ }
+ tempPath = filepath.Join(tempDir, name)
+ } else {
+ // Generate a secure unique filename if none provided
+ f, err := os.CreateTemp("", "waveterm-*")
+ if err != nil {
+ return "", fmt.Errorf("error creating temp file: %w", err)
+ }
+ tempPath = f.Name()
+ _ = f.Close() // will rewrite with correct permissions below
+ }
+
+ if err := os.WriteFile(tempPath, decoded, 0600); err != nil {
+ return "", fmt.Errorf("error writing temp file: %w", err)
+ }
+ return tempPath, nil
}Committable suggestion skipped: line range outside the PR's diff.
Handle both types of paste data. Write utility functions to normalize paste events to {text; image}[]. Fix duplication issue (call preventDefault() early). Handle multiple image pasting (by adding a slight delay). Convert Ctrl:Shift:v to use a native paste which allows capturing of images!