feat(desktop): move v2 terminal ws reconnect notices to a pane-side log#3888
Merged
Conversation
Reconnect/close/error chatter no longer streams into xterm scrollback. The transport now buffers up to 200 log entries and exposes them via the runtime registry; a pane-header button reveals a popover with the log and a clear action, and only appears when there's something to show. Removes the move-to-background button from the v2 terminal pane header.
Contributor
📝 WalkthroughWalkthroughA terminal logging system has been added, replacing direct error writes to the terminal buffer with a buffered log mechanism. The transport layer now stores timestamped log entries with severity levels, exposes listener notifications, and the runtime registry provides public APIs to access and manage logs. A new UI component displays these logs via a button in the terminal header. Changes
Sequence DiagramsequenceDiagram
participant Transport as TerminalTransport
participant Registry as TerminalRuntimeRegistry
participant Listener as UI Subscriber
participant UI as TerminalLogsButton
Transport->>Transport: Connection error/close event
Transport->>Transport: pushLog(entry)
Transport->>Transport: Append to logs (max 200)
Transport->>Listener: Notify logListeners
UI->>Registry: useSyncExternalStore(onLogsChange)
Listener->>Registry: onLogsChange(callback)
Registry->>Transport: Call transport.logListeners.add()
Registry-->>UI: Trigger subscription
UI->>Registry: getLogs(terminalId)
Registry->>Transport: Return transport.logs[]
Transport-->>Registry: TerminalLogEntry[]
Registry-->>UI: Updated logs
UI->>UI: Render button (if logs exist)
UI->>UI: Display popover with entries
User->>UI: Click Clear
UI->>Registry: clearLogs(terminalId)
Registry->>Transport: clearLogs()
Transport->>Listener: Notify logListeners
Listener->>UI: Trigger re-render
UI->>UI: Return null (no logs)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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. Review rate limit: 5/8 reviews remaining, refill in 19 minutes and 20 seconds.Comment |
Greptile SummaryThis PR reroutes WebSocket reconnect/close/error notices away from the xterm scrollback buffer and into a bounded in-memory log (max 200 entries) exposed via the runtime registry. A new Confidence Score: 5/5Safe to merge; the one finding is a UX polish suggestion and does not block correct behaviour. All remaining findings are P2. The transport, registry, and UI layers are well-structured, the useSyncExternalStore subscription pattern is correct, and the EMPTY_LOGS sentinel prevents snapshot thrash. The missing reconnect-success log entry is a quality improvement, not a defect. apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts — open handler does not emit a reconnect-success log entry.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts | Moves WebSocket close/error notices from xterm scrollback to a bounded in-memory log (max 200 entries). The open handler on reconnect does not emit a success entry, so the warning badge persists indefinitely after recovery. |
| apps/desktop/src/renderer/lib/terminal/terminal-runtime-registry.ts | Adds getLogs, clearLogs, and onLogsChange methods. Uses a frozen EMPTY_LOGS sentinel to avoid useSyncExternalStore snapshot thrash on missing entries. Pattern is consistent with existing state/title change subscriptions. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalLogsButton/TerminalLogsButton.tsx | New component using useSyncExternalStore for log subscription; renders null when no logs exist. Correct use of Radix Popover + Tooltip composition and stopPropagation for pane-focus prevention. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalHeaderExtras/TerminalHeaderExtras.tsx | Replaces the move-to-background button with TerminalLogsButton. Clean removal of dead imports. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalLogsButton/index.ts | Simple barrel export for the new TerminalLogsButton component. |
Sequence Diagram
sequenceDiagram
participant WS as WebSocket
participant Transport as TerminalTransport
participant Registry as TerminalRuntimeRegistry
participant UI as TerminalLogsButton
WS->>Transport: close / error event
Transport->>Transport: pushLog(level, message)
Transport->>Registry: notify logListeners
Registry->>UI: useSyncExternalStore callback
UI->>Registry: getLogs(terminalId, instanceId)
UI-->>UI: render badge (warn/error colour)
UI->>Registry: clearLogs(terminalId, instanceId)
Registry->>Transport: transport.logs = []
Transport->>Registry: notify logListeners
Registry->>UI: useSyncExternalStore callback
UI-->>UI: return null (hidden)
Comments Outside Diff (1)
-
apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts, line 198-211 (link)No "reconnected" log entry on successful reconnect
After a network blip, the close handler pushes a
warnentry ("Reconnecting…") so the badge appears. But theopenhandler only resets_reconnectAttempt— it never pushes aninfoentry to indicate the connection was restored. As a result the warning badge stays visible with only "Reconnecting…" in the log until the user manually clears it, giving no confirmation that the connection is back up.Consider pushing a log entry in the
openhandler when_reconnectAttempt > 0(i.e. this is a reconnect, not the initial connect):socket.addEventListener("open", () => { if (transport.socket !== socket) return; if (transport._reconnectAttempt > 0) { pushLog( transport, "info", `Reconnected to ${formatWsEndpoint(transport.currentUrl)}.`, ); } transport._reconnectAttempt = 0; setConnectionState(transport, "open"); sendResize(transport, terminal.cols, terminal.rows); if (options.initialCommand) { socket.send( JSON.stringify({ type: "initialCommand", data: options.initialCommand, }), ); } });
Prompt To Fix With AI
This is a comment left during a code review. Path: apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts Line: 198-211 Comment: **No "reconnected" log entry on successful reconnect** After a network blip, the close handler pushes a `warn` entry ("Reconnecting…") so the badge appears. But the `open` handler only resets `_reconnectAttempt` — it never pushes an `info` entry to indicate the connection was restored. As a result the warning badge stays visible with only "Reconnecting…" in the log until the user manually clears it, giving no confirmation that the connection is back up. Consider pushing a log entry in the `open` handler when `_reconnectAttempt > 0` (i.e. this is a reconnect, not the initial connect): ```ts socket.addEventListener("open", () => { if (transport.socket !== socket) return; if (transport._reconnectAttempt > 0) { pushLog( transport, "info", `Reconnected to ${formatWsEndpoint(transport.currentUrl)}.`, ); } transport._reconnectAttempt = 0; setConnectionState(transport, "open"); sendResize(transport, terminal.cols, terminal.rows); if (options.initialCommand) { socket.send( JSON.stringify({ type: "initialCommand", data: options.initialCommand, }), ); } }); ``` How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts:198-211
**No "reconnected" log entry on successful reconnect**
After a network blip, the close handler pushes a `warn` entry ("Reconnecting…") so the badge appears. But the `open` handler only resets `_reconnectAttempt` — it never pushes an `info` entry to indicate the connection was restored. As a result the warning badge stays visible with only "Reconnecting…" in the log until the user manually clears it, giving no confirmation that the connection is back up.
Consider pushing a log entry in the `open` handler when `_reconnectAttempt > 0` (i.e. this is a reconnect, not the initial connect):
```ts
socket.addEventListener("open", () => {
if (transport.socket !== socket) return;
if (transport._reconnectAttempt > 0) {
pushLog(
transport,
"info",
`Reconnected to ${formatWsEndpoint(transport.currentUrl)}.`,
);
}
transport._reconnectAttempt = 0;
setConnectionState(transport, "open");
sendResize(transport, terminal.cols, terminal.rows);
if (options.initialCommand) {
socket.send(
JSON.stringify({
type: "initialCommand",
data: options.initialCommand,
}),
);
}
});
```
Reviews (1): Last reviewed commit: "feat(desktop): move v2 terminal ws recon..." | Re-trigger Greptile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalLogsButton/TerminalLogsButton.tsx (1)
95-116: ⚡ Quick winSplit
LogRowinto its own component file.
TerminalLogsButton.tsxcurrently defines multiple components (TerminalLogsButton+LogRow). Please moveLogRowto its own component file to align with repo structure rules.♻️ Suggested split
-import { - type TerminalLogEntry, - terminalRuntimeRegistry, -} from "renderer/lib/terminal/terminal-runtime-registry"; +import { terminalRuntimeRegistry } from "renderer/lib/terminal/terminal-runtime-registry"; +import { LogRow } from "./LogRow"; @@ -function LogRow({ entry }: { entry: TerminalLogEntry }) { - ... -}// apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalLogsButton/LogRow/LogRow.tsx import { cn } from "@superset/ui/utils"; import type { TerminalLogEntry } from "renderer/lib/terminal/terminal-runtime-registry"; export function LogRow({ entry }: { entry: TerminalLogEntry }) { // existing row body }// .../TerminalLogsButton/LogRow/index.ts export { LogRow } from "./LogRow";As per coding guidelines:
**/*.{tsx,ts}: Do not create multi-component files; use one component per file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalLogsButton/TerminalLogsButton.tsx around lines 95 - 116, The file defines two React components in one file (LogRow and TerminalLogsButton); split LogRow into its own module by creating LogRow.tsx exporting the LogRow component (preserve its props type TerminalLogEntry, imports like cn and formatTime), add an index.ts that re-exports LogRow, and update TerminalLogsButton to import { LogRow } from the new folder instead of the local definition; ensure the implementation and prop types remain unchanged and any utilities (cn, formatTime) are imported where LogRow now lives.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalLogsButton/TerminalLogsButton.tsx:
- Around line 95-116: The file defines two React components in one file (LogRow
and TerminalLogsButton); split LogRow into its own module by creating LogRow.tsx
exporting the LogRow component (preserve its props type TerminalLogEntry,
imports like cn and formatTime), add an index.ts that re-exports LogRow, and
update TerminalLogsButton to import { LogRow } from the new folder instead of
the local definition; ensure the implementation and prop types remain unchanged
and any utilities (cn, formatTime) are imported where LogRow now lives.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3db32cf4-2664-4707-bb42-989c547f7521
📒 Files selected for processing (5)
apps/desktop/src/renderer/lib/terminal/terminal-runtime-registry.tsapps/desktop/src/renderer/lib/terminal/terminal-ws-transport.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalHeaderExtras/TerminalHeaderExtras.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalLogsButton/TerminalLogsButton.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalLogsButton/index.ts
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
Kitenite
added a commit
that referenced
this pull request
May 12, 2026
…ove-to-background button (#4459) * feat(desktop): surface background terminal sessions in v2 tab bar; restore move-to-background button - Add renderTabBarTrailing slot to @superset/panes Workspace/TabBar - New BackgroundTerminalsButton in v2-workspace tab bar: lists daemon terminal sessions with no pane attached; click to re-open as a tab, trash icon to kill - Restore the 'Move terminal to background' archive button in the v2 terminal pane header (removed in #3888) * fix(desktop): scope background-session kill button disabled state per row
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.
Reconnect/close/error chatter no longer streams into xterm scrollback. The transport now buffers up to 200 log entries and exposes them via the runtime registry; a pane-header button reveals a popover with the log and a clear action, and only appears when there's something to show. Removes the move-to-background button from the v2 terminal pane header.
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by cubic
Moved WebSocket reconnect/close/error notices out of terminal scrollback into a pane-side connection log, keeping the terminal clean and making connection issues easier to spot. Added a header button that shows a popover log with a clear action and only appears when there are events.
New Features
TerminalLogsButtonin the pane header opens a popover with the log and a Clear action; the button only renders when logs exist.Refactors
xtermoutput.Written for commit 96102a6. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
Release Notes