-
Notifications
You must be signed in to change notification settings - Fork 670
add OSC 52 clipboard support for terminal applications #2725
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
add OSC 52 clipboard support for terminal applications #2725
Conversation
) Implements OSC 52 escape sequence handling to allow terminal applications like zellij, tmux, and neovim to copy text to the system clipboard. Features: - Write-only clipboard access (read queries blocked for security) - 75KB size limit matching common terminal implementations - Base64 whitespace handling per RFC 4648 - Dual-phase size validation (estimated + actual) Also fixes StreamCancelFn type mismatch introduced in cfbb39a where the implementations were updated but the type definition was not.
WalkthroughThis pull request modifies three interconnected components to add OSC 52 clipboard copy support in the terminal wrapper and simplify the streaming cancellation mechanism across the stack. The frontend adds OSC 52 parsing and clipboard handling with validation and size limits. The backend web handler and RPC client utilities remove context passing from the streaming cancellation function signature, simplifying the cancellation flow from Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/app/view/term/termwrap.ts (1)
123-189: Well-implemented OSC 52 handler with proper security considerations.The implementation correctly:
- Blocks clipboard read queries (
?) to prevent clipboard theft- Applies dual-phase size validation (estimated before decode, actual after)
- Strips whitespace per RFC 4648 for chunked base64 data
- Uses async clipboard API with proper error handling
One minor edge case: after stripping whitespace on line 166,
cleanBase64Datacould become empty ifbase64Datawas only whitespace. Consider adding a check:🔎 Optional fix for edge case
// strip whitespace from base64 data (some terminals chunk with newlines per RFC 4648) const cleanBase64Data = base64Data.replace(/\s+/g, ""); + if (cleanBase64Data.length === 0) { + return true; + } const decodedText = base64ToString(cleanBase64Data);
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/app/view/term/termwrap.tspkg/web/web.gopkg/wshrpc/wshclient/wshclientutil.go
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: esimkowitz
Repo: wavetermdev/waveterm PR: 1725
File: pkg/remote/fileshare/wavefs/wavefs.go:441-494
Timestamp: 2025-01-29T04:21:11.649Z
Learning: The `CopyRemote` function in WaveTerm's file operations has proper error handling that prevents partial writes by failing fast and using context cancellation. Each step (path cleaning, file operations, tar reading/writing) is guarded by error checks that prevent proceeding with writes on error.
📚 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/wshclientutil.gopkg/web/web.go
📚 Learning: 2025-01-29T04:21:11.649Z
Learnt from: esimkowitz
Repo: wavetermdev/waveterm PR: 1725
File: pkg/remote/fileshare/wavefs/wavefs.go:441-494
Timestamp: 2025-01-29T04:21:11.649Z
Learning: The `CopyRemote` function in WaveTerm's file operations has proper error handling that prevents partial writes by failing fast and using context cancellation. Each step (path cleaning, file operations, tar reading/writing) is guarded by error checks that prevent proceeding with writes on error.
Applied to files:
pkg/web/web.go
⏰ 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
🔇 Additional comments (4)
frontend/app/view/term/termwrap.ts (2)
38-38: Good choice for the size limit.The 75 KB limit aligns with common terminal implementations and provides a reasonable safeguard against excessive clipboard data.
529-541: OSC handlers are correctly registered.The OSC 52 handler is properly integrated alongside the existing OSC 7, 9283, and 16162 handlers, following the established pattern of passing
blockIdandloadedstate.pkg/web/web.go (1)
257-275: Cancellation signature change is consistent with the RPC layer.The simplified
streamCancelFn func()signature aligns with the corresponding change inwshclientutil.go. The nil check before invoking the cancel function and returningctx.Err()maintains proper error semantics.pkg/wshrpc/wshclient/wshclientutil.go (1)
66-68: Simplified cancellation signature is correctly implemented.The type definition
StreamCancelFn func()inwshrpctypes.go:376matches the implementation at lines 66-68. The signature is consistently used throughout the codebase (e.g.,web.go:257), and the fire-and-forget pattern withcontext.Background()is appropriate for cancellation.
Summary
Features
Additional Fix
Also fixes StreamCancelFn type mismatch from #2709 where the implementations were updated but the type definition was not.