fix desktop terminal attach latency#3847
Conversation
|
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:
📝 WalkthroughWalkthroughTerminal WebSocket URL now includes Changes
Sequence Diagram(s)sequenceDiagram
participant Pane as TerminalPane
participant Registry as terminalRuntimeRegistry
participant Transport as TerminalTransport
participant Server as WebSocket Server
participant TRPC as workspaceTrpc/cache
Pane->>Registry: mount(wsConfig)
Pane->>Registry: connect(websocketUrl with workspaceId & themeType)
Registry->>Transport: open WebSocket
Transport->>Server: WebSocket OPEN -> session created
Server-->>Transport: "title" message (terminal title set)
Transport-->>Registry: notifyServerAttach()
Registry-->>Pane: onServerAttach callback (title present)
Pane->>TRPC: invalidate('terminal.listSessions') [dedup key: workspaceId,terminalId,instanceId,websocketUrl]
Transport-->>Registry: onStateChange("closed") -> clear dedupe marker
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Greptile SummaryThis PR reduces terminal attach latency by eliminating the blocking Confidence Score: 5/5Safe to merge; only P2 style findings remain. The core change is correct — removing the tRPC blocking call and moving session creation to the WS handler. Both remaining findings are P2: a stale comment and a heuristic delay for cache invalidation. Neither affects correctness or reliability in a breaking way. No files require special attention.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx | Removes the blocking ensureSession tRPC call before WebSocket connect, moves session creation to the server-side WebSocket handler via URL params; two minor P2 issues: a stale comment and a heuristic 250 ms invalidation timer. |
Sequence Diagram
sequenceDiagram
participant R as React (TerminalPane)
participant WS as WebSocket (server)
participant DB as Session Store
note over R,DB: Before (slow path)
R->>WS: tRPC ensureSession (HTTP)
WS-->>DB: create/verify session
DB-->>WS: ok
WS-->>R: {status: active}
R->>WS: connect() WebSocket
R->>DB: invalidateTerminalSessions
note over R,DB: After (fast path)
R->>WS: connect() WebSocket (workspaceId + themeType in URL)
WS->>DB: create/verify session on open
R->>DB: invalidateTerminalSessions (250 ms timer, fire-and-forget)
Comments Outside Diff (1)
-
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx, line 158-162 (link)Stale comment references removed
ensureSessionThe reconnect effect's comment still says "we let the
ensureSessionpath above open it", butensureSessionwas deleted in this PR. The comment now describes a non-existent code path and will mislead future readers.Prompt To Fix With AI
This is a comment left during a code review. Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx Line: 158-162 Comment: **Stale comment references removed `ensureSession`** The reconnect effect's comment still says _"we let the `ensureSession` path above open it"_, but `ensureSession` was deleted in this PR. The comment now describes a non-existent code path and will mislead future readers. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx
Line: 158-162
Comment:
**Stale comment references removed `ensureSession`**
The reconnect effect's comment still says _"we let the `ensureSession` path above open it"_, but `ensureSession` was deleted in this PR. The comment now describes a non-existent code path and will mislead future readers.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx
Line: 146-150
Comment:
**Magic 250 ms heuristic for session-list invalidation**
The old code called `invalidateTerminalSessions` only after `ensureSession` confirmed the session was `"active"`, so the cache was refreshed at the right moment. The 250 ms timer fires unconditionally — on a slow connection the WebSocket handshake may not have completed yet, leaving the session list stale until the next automatic refetch cycle. Consider invalidating inside a WebSocket `onOpen` / connection-state callback instead so timing is driven by the actual event rather than an arbitrary delay.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix desktop terminal attach latency" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const invalidateTimer = window.setTimeout(() => { | ||
| void invalidateTerminalSessionsRef.current({ | ||
| workspaceId: sessionWorkspaceId, | ||
| themeType: initialThemeTypeRef.current, | ||
| }) | ||
| .then((result) => { | ||
| if (result.status === "active") { | ||
| void invalidateTerminalSessionsRef.current({ | ||
| workspaceId: sessionWorkspaceId, | ||
| }); | ||
| return; | ||
| } | ||
| if (cancelled) return; | ||
| const details = result.error | ||
| ? `: ${result.error}` | ||
| : " for an unknown reason"; | ||
| terminalRuntimeRegistry | ||
| .getTerminal(terminalId, terminalInstanceId) | ||
| ?.writeln( | ||
| `\r\n[terminal] Failed to create terminal session${details}`, | ||
| ); | ||
| }) | ||
| .catch((err) => { | ||
| console.error("[TerminalPane] ensureSession failed:", err); | ||
| if (cancelled) return; | ||
| const message = err instanceof Error ? err.message : String(err); | ||
| terminalRuntimeRegistry | ||
| .getTerminal(terminalId, terminalInstanceId) | ||
| ?.writeln( | ||
| `\r\n[terminal] terminal.ensureSession request failed: ${message}`, | ||
| ); | ||
| }) | ||
| .finally(() => { | ||
| if (cancelled) return; | ||
| terminalRuntimeRegistry.connect( | ||
| terminalId, | ||
| websocketUrlRef.current, | ||
| terminalInstanceId, | ||
| ); | ||
| }); | ||
| }, 250); |
There was a problem hiding this comment.
The old code called invalidateTerminalSessions only after ensureSession confirmed the session was "active", so the cache was refreshed at the right moment. The 250 ms timer fires unconditionally — on a slow connection the WebSocket handshake may not have completed yet, leaving the session list stale until the next automatic refetch cycle. Consider invalidating inside a WebSocket onOpen / connection-state callback instead so timing is driven by the actual event rather than an arbitrary delay.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx
Line: 146-150
Comment:
**Magic 250 ms heuristic for session-list invalidation**
The old code called `invalidateTerminalSessions` only after `ensureSession` confirmed the session was `"active"`, so the cache was refreshed at the right moment. The 250 ms timer fires unconditionally — on a slow connection the WebSocket handshake may not have completed yet, leaving the session list stale until the next automatic refetch cycle. Consider invalidating inside a WebSocket `onOpen` / connection-state callback instead so timing is driven by the actual event rather than an arbitrary delay.
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx (1)
146-150: Consider extracting the 250ms delay as a named constant.The magic number could be clearer with a descriptive constant name explaining its purpose (e.g.,
SESSION_CACHE_INVALIDATION_DELAY_MS). This is a minor readability nit.💡 Optional refactor
+const SESSION_CACHE_INVALIDATION_DELAY_MS = 250; + // ... in the effect: -const invalidateTimer = window.setTimeout(() => { +const invalidateTimer = window.setTimeout(() => { void invalidateTerminalSessionsRef.current({ workspaceId: sessionWorkspaceId, }); -}, 250); +}, SESSION_CACHE_INVALIDATION_DELAY_MS);🤖 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/TerminalPane.tsx around lines 146 - 150, Extract the magic number 250ms into a clearly named constant (e.g., SESSION_CACHE_INVALIDATION_DELAY_MS) and replace the inline literal in the window.setTimeout call inside TerminalPane (the invalidateTimer setup that calls invalidateTerminalSessionsRef.current with workspaceId). Define the constant near the top of this module or component so its purpose is clear and reuseable, and keep the existing behavior (250) as the constant value.
🤖 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/TerminalPane.tsx:
- Around line 146-150: Extract the magic number 250ms into a clearly named
constant (e.g., SESSION_CACHE_INVALIDATION_DELAY_MS) and replace the inline
literal in the window.setTimeout call inside TerminalPane (the invalidateTimer
setup that calls invalidateTerminalSessionsRef.current with workspaceId). Define
the constant near the top of this module or component so its purpose is clear
and reuseable, and keep the existing behavior (250) as the constant value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c84091ab-4c8e-4311-9113-4a3827b52059
📒 Files selected for processing (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Kitenite
force-pushed
the
trpc-throttle-investigati
branch
2 times, most recently
from
c255c0e to
e8fb4bc
Compare
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/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx:
- Around line 150-178: The dedupe key stored in lastInvalidatedOpenSessionRef
never gets cleared, so after a close->open cycle invalidateIfOpen (used with
terminalRuntimeRegistry.onStateChange) will short-circuit forever; modify the
state-change handling so that when
terminalRuntimeRegistry.getConnectionState(...) reports a non-"open" state
(e.g., "closed"), you clear lastInvalidatedOpenSessionRef.current (or set it to
null) so the next open will regenerate the invalidateKey and call
invalidateTerminalSessionsRef.current; update the callback registered with
terminalRuntimeRegistry.onStateChange (used in the effect) to clear the ref on
close and keep the existing invalidateIfOpen behavior on open.
🪄 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: b149196b-1729-466e-a8f5-f27686c956d4
📒 Files selected for processing (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx
Kitenite
force-pushed
the
trpc-throttle-investigati
branch
4 times, most recently
from
c5ef4a6 to
5777c27
Compare
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
Kitenite
force-pushed
the
trpc-throttle-investigati
branch
from
5777c27 to
2cc8086
Compare
Kitenite
force-pushed
the
trpc-throttle-investigati
branch
from
2cc8086 to
13f02d6
Compare
1 participant
Description
Fixes V2 terminal attach latency during workspace switches by removing renderer-side waits on terminal session tRPC before opening terminal WebSockets.
Terminal panes now mount xterm and connect the WebSocket immediately. The WebSocket URL carries
workspaceIdandthemeType, allowing host-service to create or attach the terminal session on open. This avoids tRPC head-of-line blocking when workspace switch work and terminal creation happen at the same time.V2 presets and pending terminal launches no longer pre-create terminal sessions through tRPC. They add terminal panes with a transient
initialCommand;TerminalPanesends that command as the first WebSocket frame after open, and host-service queues it behind the same shell-ready gate used by session creation.Server-side automation dispatch now uses the explicit
terminal.launchSession({ workspaceId, terminalId?, initialCommand, themeType? })API. The oldterminal.ensureSessionprocedure has been removed.The terminal session list cache is invalidated from terminal connection state when the WebSocket reaches
open, with the dedupe reset when the socket leavesopenso reconnects refresh the list again.Type of Change
Testing
bun --filter @superset/desktop typecheckbun --filter @superset/host-service typecheckbun --filter @superset/trpc typecheckbunx @biomejs/biome@2.4.2 check packages/host-service/src/trpc/router/terminal/terminal.ts packages/trpc/src/router/automation/dispatch.ts