fix(desktop): prevent xterm garbling via tRPC-first terminal sessions#3252
Merged
fix(desktop): prevent xterm garbling via tRPC-first terminal sessions#3252
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded a reconnect path for terminal websockets: Changes
Sequence Diagram(s)sequenceDiagram
participant TerminalPane
participant Registry as TerminalRuntimeRegistry
participant Transport as TerminalWS
participant WebSocket
Note over TerminalPane,WebSocket: Old flow (re-attach on URL change)
TerminalPane->>Registry: detach(terminalId)
Registry->>Transport: cleanup()
Transport-->>WebSocket: close()
TerminalPane->>Registry: attach(terminalId, newUrl)
Registry->>Transport: connect(transport, terminal, newUrl)
Transport->>WebSocket: new WebSocket(newUrl)
WebSocket-->>Transport: open
Note over TerminalPane,WebSocket: New flow (reconnect in-place)
TerminalPane->>Registry: reconnect(terminalId, newUrl)
Registry->>Transport: connect(existingTransport, existingTerminal, newUrl)
Transport->>Transport: compare baseUrl(currentUrl) vs baseUrl(newUrl)
alt origin+pathname match
Transport-->>Registry: update currentUrl, return (no close)
else different base
Transport-->>WebSocket: close()
Transport->>WebSocket: new WebSocket(newUrl)
WebSocket-->>Transport: open
end
Registry-->>TerminalPane: done
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/lib/terminal/terminal-addons.ts">
<violation number="1" location="apps/desktop/src/renderer/lib/terminal/terminal-addons.ts:60">
P2: Do not silently swallow `clearTextureAtlas` failures; log the error so reattach rendering issues remain diagnosable.
(Based on your team's feedback about avoiding silent catch blocks that hide runtime failures.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/renderer/lib/terminal/terminal-addons.ts`:
- Line 21: Update the JSDoc for the function loadAddons to reflect that it now
returns an AddonHandle instead of a simple cleanup function: locate the JSDoc
immediately above loadAddons(terminal: XTerm) and replace the description and
`@returns` tag to describe the AddonHandle type and its responsibilities (what
methods/properties callers can expect) rather than saying it returns a cleanup
function; ensure the text references the AddonHandle symbol and accurately
documents any cleanup method name on that handle if applicable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6f01cac7-faa5-4923-a15c-c13db118739f
📒 Files selected for processing (2)
apps/desktop/src/renderer/lib/terminal/terminal-addons.tsapps/desktop/src/renderer/lib/terminal/terminal-runtime.ts
|
Greptile encountered an error while reviewing this PR. Please reach out to support@greptile.com for assistance. |
Two fixes for terminal rendering corruption when switching workspaces: 1. TerminalPane: split attach/detach effect (deps: [terminalId] only) from reconnect effect (deps: [terminalId, websocketUrl]). This prevents DOM wrapper removal on workspace switches where only the URL query params change, preserving the WebGL context. 2. terminal-ws-transport: compare origin+pathname only in connect() idempotency check. Query params (workspaceId, token) change during workspace switches without meaning the terminal session changed. Full URL comparison caused spurious socket reconnects that triggered replay messages and garbled the terminal buffer.
Kitenite
force-pushed
the
xterm-rendering-changes-in-pane-v2
branch
from
f243b47 to
42b1207
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts`:
- Around line 106-111: The early return in the isActive check prevents updating
transport.currentUrl when only the query portion of wsUrl changes (e.g.,
refreshed token from useWorkspaceWsUrl()), so scheduleReconnect() later retries
the old URL; modify the logic to still update transport.currentUrl to the new
wsUrl whenever baseUrl(transport.currentUrl) === baseUrl(wsUrl) but the full
wsUrl differs (or introduce a separate reconnectTarget field) before returning
so future scheduleReconnect() uses the refreshed URL; touch symbols:
transport.currentUrl, baseUrl(...), wsUrl, isActive, scheduleReconnect(),
useWorkspaceWsUrl().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0f8538e9-d8aa-4d47-8e34-bfaf2b11cd0f
📒 Files selected for processing (3)
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/TerminalPane.tsx
…econnect When connect() skips reconnection (same base URL), still update transport.currentUrl to the new full URL so that scheduleReconnect uses the latest token/query params if the socket later drops.
…h auto-reconnect" This reverts commit e66cfd2.
Contributor
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts">
<violation number="1" location="apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts:110">
P1: `currentUrl` is no longer updated on same-endpoint active connections, so auto-reconnect may use stale query/token values.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Create session via tRPC first, then connect WebSocket as a data pipe. Eliminates query param instability that causes rendering garbling.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/desktop/docs/TERMINAL_WS_SESSION_MIGRATION.md (2)
3-6: Clarify the relationship between the current PR and this migration plan.The statement "PR
#3252works around this" may confuse readers since this document is part of PR#3252. Consider adding a sentence explicitly stating that this document describes a future migration plan, and that the current PR implements a temporary workaround (thewebsocketUrlRef, separate reconnect effect, andclearTextureAtlascall) that will be replaced by the tRPC-based approach described here.📝 Suggested clarification
## Problem -WebSocket URL includes `workspaceId`, `themeType` as query params. When workspace context changes, the URL changes, causing spurious reconnects that garble rendering. PR `#3252` works around this. +WebSocket URL includes `workspaceId`, `themeType` as query params. When workspace context changes, the URL changes, causing spurious reconnects that garble rendering. + +**Current state:** PR `#3252` implements a workaround by keeping TerminalPane mounted and reconnecting only the transport layer. + +**Future fix (this doc):** Migrate to tRPC-based session creation to eliminate query-param instability entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/docs/TERMINAL_WS_SESSION_MIGRATION.md` around lines 3 - 6, Update the migration doc to explicitly state that it outlines a future migration plan and that PR `#3252` contains a temporary workaround: add one clear sentence near the top saying this document describes the intended tRPC-based migration while the current PR implements a stopgap using websocketUrlRef, a separate reconnect effect, and a clearTextureAtlas call, and that those temporary pieces will be removed when the tRPC approach is implemented.
13-19: Add language identifier to the code block.The fenced code block should specify a language for proper syntax highlighting and rendering.
📝 Proposed fix
### Flow -``` +```text 1. Client generates terminalId (crypto.randomUUID) — already done in pane data 2. tRPC: terminal.createSession({ terminalId, workspaceId, themeType, cols, rows }) → host-service creates PTY, returns { status, scrollback? } 3. WebSocket: ws://host/terminal/{terminalId}?token=Z → attaches to existing session, forwards data</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@apps/desktop/docs/TERMINAL_WS_SESSION_MIGRATION.mdaround lines 13 - 19, The
fenced code block that begins with "1. Client generates terminalId
(crypto.randomUUID) — already done in pane data" lacks a language identifier;
fix it by adding a language hint after the opening(for exampletext) to
the block containing the three numbered steps so the renderer applies proper
syntax highlighting.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In@apps/desktop/docs/TERMINAL_WS_SESSION_MIGRATION.md:
- Around line 3-6: Update the migration doc to explicitly state that it outlines
a future migration plan and that PR#3252contains a temporary workaround: add
one clear sentence near the top saying this document describes the intended
tRPC-based migration while the current PR implements a stopgap using
websocketUrlRef, a separate reconnect effect, and a clearTextureAtlas call, and
that those temporary pieces will be removed when the tRPC approach is
implemented.- Around line 13-19: The fenced code block that begins with "1. Client generates
terminalId (crypto.randomUUID) — already done in pane data" lacks a language
identifier; fix it by adding a language hint after the opening ``` (for exampleapplies proper syntax highlighting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID:
970b91b5-7ec8-45ec-bb4e-d1c884bc3ca3📒 Files selected for processing (1)
apps/desktop/docs/TERMINAL_WS_SESSION_MIGRATION.md
…nect Move terminal session creation from WebSocket query params to a dedicated tRPC `terminal.ensureSession` mutation. This makes the WebSocket URL stable across workspace switches (no workspaceId/themeType in query params), preventing spurious reconnects that caused xterm rendering garbling via buffer replay. - Add `terminal.ensureSession` tRPC mutation in host-service - TerminalPane calls ensureSession before attaching WebSocket - Revert transport `connect()` to simple URL string equality - Remove unused `reconnect()` from terminal runtime registry - WS handler retains query-param fallback for v1 compatibility
Contributor
There was a problem hiding this comment.
1 issue found across 7 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/host-service/src/trpc/router/terminal/terminal.ts">
<violation number="1" location="packages/host-service/src/trpc/router/terminal/terminal.ts:15">
P3: `cols` and `rows` are added to the tRPC input but never used, which creates misleading dead API surface.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…-pane-v2 # Conflicts: # apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx
Contributor
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
Kitenite
changed the title
11 tasks
MocA-Love
added a commit
to MocA-Love/superset
that referenced
this pull request
Apr 10, 2026
…perset-sh#3252) Cherry-picked from upstream 54e8a1c (fix(desktop): prevent xterm garbling via tRPC-first terminal sessions). Only the terminal-related portion was taken; the quit lifecycle changes (setSkipQuitConfirmation / quitApp in main/index.ts and auto-updater.ts) were deliberately excluded because they collide with fork's independent requestQuit(mode: QuitMode) / GitHub API based auto-updater implementation. Changes: - Add terminal.ensureSession tRPC mutation in host-service - TerminalPane calls ensureSession before attaching WebSocket - WebSocket URL becomes stable (no workspaceId/themeType query params), preventing spurious reconnects that caused xterm rendering garbling on workspace switches - host-service WS handler retains query-param fallback for backwards compatibility with v1 callers Fork-specific decisions: - main/index.ts and auto-updater.ts intentionally left untouched - setSkipQuitConfirmation() helper NOT added to fork (fork uses prepareQuit('stop') instead)
Merged
11 tasks
hyrness
added a commit
to hyrness/superset
that referenced
this pull request
Apr 15, 2026
…context loss After the ensureSession change (superset-sh#3252), terminals can receive data (including wide/CJK characters) via the WebSocket before the WebGL renderer initializes (deferred to rAF). When WebGL takes over, the pre-rendered cells are not correctly re-rendered, causing intermittent garbling that fixes on mouse selection. Two fixes: 1. Call terminal.refresh() after loading the WebGL addon to force a full repaint of all buffered cells with the WebGL renderer. 2. Set suggestedRendererType = "dom" in the onContextLoss handler so that subsequent terminals in the same session skip WebGL after a GPU context loss. Fixes superset-sh#3406
1 task
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
terminal.ensureSessionbefore WebSocket connect/terminal/{terminalId}?token=...) — no workspaceId/themeType in query paramsreconnect()from terminal runtime registryconnect()to simple URL string equality checkTest plan