Add Terminal in-app and themed; a tabbed pane with Session#890
Open
brucestarlove wants to merge 6 commits intohumanlayer:mainfrom
Open
Add Terminal in-app and themed; a tabbed pane with Session#890brucestarlove wants to merge 6 commits intohumanlayer:mainfrom
brucestarlove wants to merge 6 commits intohumanlayer:mainfrom
Conversation
…sing Go PTY, websocket + xterm.js
Contributor
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to 253ef6b in 2 minutes and 26 seconds. Click for details.
- Reviewed
1199lines of code in9files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. humanlayer-wui/src/components/TerminalPane.tsx:165
- Draft comment:
The WebSocket onmessage handler writes a Uint8Array directly to the terminal using terminal.write. Since xterm.js expects a string in its write() method, consider decoding the binary data (e.g. via TextDecoder) before writing it. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment suggests that xterm.js write() only accepts strings, but this is likely incorrect. Modern versions of xterm.js (especially v4+) support both string and Uint8Array/Uint8ClampedArray for the write() method. The code is handling binary data from a PTY (pseudo-terminal), which is typically binary data that should be written directly without decoding. The author has set ws.binaryType = 'arraybuffer' on line 154 and 271, indicating intentional binary handling. If the code were incorrect, it would fail at runtime, which would be caught during testing. The comment appears to be based on outdated or incorrect understanding of the xterm.js API. I might be wrong about xterm.js supporting Uint8Array - perhaps older versions only supported strings. However, the code is new (entire file is new), so it's likely using a modern version. Also, if this were a real bug, it would be immediately obvious when testing the terminal. Even if older versions of xterm.js only supported strings, this is a new file being added, so the author would be using a current version. More importantly, if terminal.write() didn't accept Uint8Array, this would cause an immediate runtime error that would be caught during basic testing. The fact that the author explicitly handles binary data suggests this is intentional and working. This comment should be deleted. The xterm.js write() method does accept Uint8Array in modern versions, and the code's intentional binary handling (setting binaryType to 'arraybuffer') indicates this is the correct approach. If this were wrong, it would fail immediately during testing.
2. humanlayer-wui/src/components/internal/SessionDetail/components/ActiveSession.tsx:777
- Draft comment:
The terminal tab is lazily loaded and toggled via a hotkey and Tabs trigger which is a good optimization. Just ensure state consistency when switching between tabs, especially that terminalOpened is properly managed to avoid reinitializing the terminal unnecessarily. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%None
Workflow ID: wflow_9Pa1IFqgCp6T5Im2
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Author
Terminal Integration PlanSummaryImplement an interactive terminal within the session detail view using Go PTY on the backend and xterm.js on the frontend. The terminal will be scoped to the session's working directory. Backend Implementation (Go)1. WebSocket Terminal HandlerCreate a new handler in
2. Route RegistrationWire the handler in // Register terminal WebSocket endpoint
terminalHandler := handlers.NewTerminalHandler(sessionManager, conversationStore)
v1.GET("/terminal", terminalHandler.HandleWebSocket)3. Security Considerations
Frontend Implementation (React/TypeScript)1. Install DependenciesAdd to
2. TerminalPane ComponentCreate
3. UI IntegrationModify
4. StylingUse existing theme CSS variables for terminal colors:
Key Files to Create/Modify| File | Change | |------|--------| | | | | | | Testing Steps
|
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.
Summary
Adds a built-in, session-scoped terminal pane that uses xterm.js to connect to the active workspace via WebSocket, keeping shell context aligned with the agent session. Includes reconnect/status feedback so developers can run quick commands without leaving the IDE. Same theme as user's preferred app theme. Shortcut cmd/ctrl+\ (backslash).
What problem(s) was I solving?
Alt-tabbing into a terminal app for simple commands, like git.
What user-facing changes did I ship?
How I implemented it
How to verify it
Do the thing
make check testpasses^ just a bunch of
buildtag: +build line is no longer needed (govet) // +build integrationfrom main repoDescription for the changelog
Adds a built-in, session-scoped terminal pane that uses xterm.js to connect to the active workspace via WebSocket, keeping shell context aligned with the agent session. Includes reconnect/status feedback so developers can run quick commands without leaving the IDE. Same theme as user's preferred app theme. Shortcut cmd/ctrl+\ (backslash).
A picture of a cute animal (not mandatory but encouraged)
Important
Adds a terminal pane using xterm.js with WebSocket integration for real-time terminal interaction in the session UI.
xterm.jsinTerminalPane.tsx, connecting via WebSocket to the active workspace.ActiveSession.tsxwith tabs for switching between conversation and terminal.cmd/ctrl+\to toggle terminal inHotkeyPanel.tsx.ActiveSession.tsxto include terminal tab and lazy-loadTerminalPane.xterm,@xterm/addon-fit, and@xterm/addon-web-linkstopackage.json.terminal.gofor terminal sessions.http_server.go.This description was created by
for 253ef6b. You can customize this summary. It will automatically update as commits are pushed.