rip out osc 23198 and osc 9283 handlers #2736
Conversation
WalkthroughThis pull request removes terminal-based RPC client initialization and related terminal lifecycle code across multiple components. It deletes test UI components Page and Button, removes Wave OSC 9283 handling from the frontend terminal wrapper, eliminates WebSocket/proxy wiring and related OSC-to-shell forwarding in the shell controller, drops terminal raw-mode and signal handlers from wshutil, and changes wsh command setup to return an error when WAVETERM_JWT is missing instead of initializing a fallback terminal client. Packet-based RPC setup remains. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
⏰ 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)
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: 0
🧹 Nitpick comments (1)
cmd/test/test-main.go (1)
6-8: Consider removing this file entirely.The empty
main()function makes this test binary a no-op. If the test components (Page, Button) and terminal-based test infrastructure are no longer needed, consider deletingcmd/test/test-main.goentirely rather than maintaining an empty placeholder.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cmd/test/test-main.gocmd/wsh/cmd/wshcmd-root.gofrontend/app/view/term/termwrap.tspkg/blockcontroller/shellcontroller.gopkg/wshutil/wshutil.go
🧰 Additional context used
🧠 Learnings (5)
📚 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:
cmd/wsh/cmd/wshcmd-root.gopkg/blockcontroller/shellcontroller.gopkg/wshutil/wshutil.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:
cmd/wsh/cmd/wshcmd-root.go
📚 Learning: 2025-10-15T03:21:02.229Z
Learnt from: sawka
Repo: wavetermdev/waveterm PR: 2433
File: pkg/aiusechat/tools_readfile.go:197-197
Timestamp: 2025-10-15T03:21:02.229Z
Learning: In Wave Terminal's AI tool definitions (pkg/aiusechat/tools_*.go), the Description field should not mention approval requirements even when ToolApproval returns ApprovalNeedsApproval. This prevents the LLM from asking users for approval before calling the tool, avoiding redundant double-approval prompts since the runtime will enforce approval anyway.
Applied to files:
cmd/wsh/cmd/wshcmd-root.go
📚 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/app/view/term/termwrap.ts
📚 Learning: 2025-01-22T22:27:25.739Z
Learnt from: esimkowitz
Repo: wavetermdev/waveterm PR: 1790
File: pkg/remote/connparse/connparse.go:76-82
Timestamp: 2025-01-22T22:27:25.739Z
Learning: The GetRpcContext() method in wshutil package is guaranteed to never return nil due to type constraints, making nil checks unnecessary.
Applied to files:
pkg/wshutil/wshutil.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). (3)
- GitHub Check: Build for TestDriver.ai
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
cmd/wsh/cmd/wshcmd-root.go (1)
84-94: LGTM - Improved session validation.The explicit error when
WAVETERM_JWTis missing correctly enforces Wave-managed sessions, replacing the previous silent fallback to terminal-based RPC. This tightens security and aligns with the PR's removal of terminal RPC client paths.frontend/app/view/term/termwrap.ts (1)
8-8: LGTM - Import cleanup aligns with OSC handler removal.The import changes are consistent with removing OSC 9283 Wave command handling. OSC 7 (cwd tracking) and OSC 16162 (shell integration) remain intact as expected.
pkg/blockcontroller/shellcontroller.go (1)
516-556: LGTM - Direct PTY read simplifies IO path.Replacing the wshProxy-based PTY buffer with direct reads from
shellProc.Cmdremoves an unnecessary indirection layer. Error handling remains correct, and the change aligns with removing the WebSocket proxy infrastructure.pkg/wshutil/wshutil.go (1)
128-135: LGTM - Simplified shutdown aligns with terminal handling removal.The simplified
DoShutdownimplementation is consistent with removing terminal state restoration and signal handlers. While the terminal might not be restored on abnormal exit, this is expected given the PR's goal of removing all terminal-based infrastructure.
No description provided.