[codex] Improve terminal connection diagnostics#3801
Conversation
📝 WalkthroughWalkthroughAdds detailed WebSocket endpoint and close-event logging in the renderer transport, surfaces terminal session creation failures in the UI, and returns a more specific error message from the host-service when a requested terminal session is missing. Changes
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 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. Comment |
Greptile SummaryThis is a diagnostics-only PR that surfaces previously hidden terminal connection failures as in-terminal messages — no startup or control-flow behavior changes. It adds Confidence Score: 5/5Safe to merge — all changes are diagnostic messages with no control-flow impact. The only finding is P2: the "Reconnecting…" suffix in the close-event message can be inaccurate at max reconnect attempts, but it does not affect any behavior. All three files are well-guarded (optional chaining, No files require special attention.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts | Adds formatWsEndpoint (strips query params for safe display) and formatCloseDetails; improves close/error event messages. Minor UX issue: "Reconnecting…" is shown even when max reconnect attempts are already reached. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx | Adds explicit return after active-session invalidation and surfaces ensureSession failures (non-active result or thrown error) directly in the terminal widget. Optional chaining guards handle the unmounted-terminal case correctly. |
| packages/host-service/src/terminal/terminal.ts | Replaces the terse "Session not found" error with a detailed message that names the terminal ID and explains the missing workspaceId context. Close reason updated to "Terminal session not found" for consistency. |
Sequence Diagram
sequenceDiagram
participant R as TerminalPane (renderer)
participant T as terminalRuntimeRegistry
participant H as host-service
R->>H: tRPC terminal.ensureSession
alt status === active
H-->>R: { status: active }
R->>R: invalidate session list and return
else status !== active (NEW)
H-->>R: { status: ..., error?: ... }
R->>T: writeln([terminal] Failed to create terminal session: ...)
end
Note over R: .finally() always runs connect()
R->>T: connect(terminalId, wsUrl)
T->>H: WebSocket /terminal/:id
alt session found
H-->>T: open + replay
else session not found, no workspaceId (IMPROVED msg)
H-->>T: error message with terminalId + workspaceId context
H-->>T: ws.close(1011, Terminal session not found)
T->>T: writeln([terminal] WebSocket closed code 1011 reason ...) NEW
end
alt WebSocket error
H-->>T: error event (IMPROVED msg)
T->>T: writeln([terminal] WebSocket error while connecting to endpoint)
end
Prompt To Fix All With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts
Line: 188-192
Comment:
**"Reconnecting…" message misleads when max attempts are exhausted**
When the WebSocket closes unexpectedly, the new message always ends with `"Reconnecting..."`, but `scheduleReconnect` silently returns without scheduling anything once `_reconnectAttempt >= MAX_RECONNECT_ATTEMPTS` (10). The user sees "Reconnecting…" but no further reconnection ever happens, giving a false impression that recovery is in progress.
Consider making the terminal message conditional on the remaining attempt budget:
```suggestion
if (!transport._exited && event.code !== 1000) {
const willReconnect =
transport._reconnectAttempt < MAX_RECONNECT_ATTEMPTS;
terminal.writeln(
`\r\n[terminal] WebSocket closed while connected to ${formatWsEndpoint(transport.currentUrl)} (${formatCloseDetails(event)}). ${willReconnect ? "Reconnecting..." : "Max reconnect attempts reached."}`,
);
}
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Improve terminal connection diagnostics" | Re-trigger Greptile
Kitenite
force-pushed
the
debug-terminal-connection
branch
from
361f35e to
d478bbc
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 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-ws-transport.ts">
<violation number="1" location="apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts:190">
P2: The message unconditionally says "Reconnecting..." but `scheduleReconnect` silently no-ops once `_reconnectAttempt >= MAX_RECONNECT_ATTEMPTS`. After the last allowed attempt, this gives a false impression that recovery is in progress. Make the message conditional on remaining attempts.</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}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/host-service/src/terminal/terminal.ts (1)
597-609: Diagnostic improvements look good; minor suggestion to align with PR description.The new error message clearly identifies the missing session by
terminalIdand points at the missingworkspaceIdfallback, which is a useful improvement over the prior generic "Session not found". That said, the PR description states this path should also "suggest callingterminal.ensureSessionor providingworkspaceId", but the rendered error only mentions the missing fallback. Consider mentioningensureSessionexplicitly so v1 callers see actionable guidance:Suggested wording tweak
- message: `Terminal session "${terminalId}" not found; missing workspaceId fallback.`, + message: `Terminal session "${terminalId}" not found. Call terminal.ensureSession before attaching, or provide a workspaceId query parameter.`,Note: WebSocket close reasons are limited to 123 UTF-8 bytes (RFC 6455), so keep the longer wording in the JSON
errorpayload and leave theclose()reason short as it currently is.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/src/terminal/terminal.ts` around lines 597 - 609, Update the JSON error payload sent when a session is missing to explicitly suggest calling terminal.ensureSession or providing a workspaceId; specifically modify the block that checks sessions.get(terminalId) (uses terminalId, c.req.query, sendMessage, ws.close) so the sendMessage(...) error message includes guidance like "call terminal.ensureSession or provide workspaceId", while keeping the ws.close(1011, "Terminal session not found") short per RFC limits.apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx (1)
153-185: SurfacingensureSessionfailures in-terminal LGTM; minor nit on cancellation.Resolving
result.errorto either: <reason>or" for an unknown reason", plus a separate prefix for rejections, gives users a much clearer picture than the prior silent-failure path.One small consideration:
cancelledis only checked in.finallybefore callingconnect, but not in.then/.catch. If the pane unmounts betweenmutateAsyncresolving and thefinallyrunning, you'll still callwritelnon whatever the registry returns for thatterminalId/terminalInstanceId. The?.writelnchain makes this safe in practice (the registry returns null afterdetach), so this is a nit rather than a bug — but if you want strict symmetry with the connect guard, gating the writelns on!cancelledwould make the intent explicit:Optional: gate diagnostic writes on `cancelled`
.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);🤖 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 153 - 185, The current ensureSession handling calls terminalRuntimeRegistry.getTerminal(...)? .writeln in both the .then and .catch branches even if the operation was cancelled; add a guard on the cancelled flag before performing those diagnostic writeln calls (the same way .finally gates connect) so that in the promise resolution/rejection paths you return early when cancelled is true, e.g. check !cancelled before computing details and calling terminalRuntimeRegistry.getTerminal(terminalId, terminalInstanceId)?.writeln in the .then and before the terminalRuntimeRegistry.getTerminal(...)? .writeln in the .catch; keep the existing optional chaining to remain safe.
🤖 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 153-185: The current ensureSession handling calls
terminalRuntimeRegistry.getTerminal(...)? .writeln in both the .then and .catch
branches even if the operation was cancelled; add a guard on the cancelled flag
before performing those diagnostic writeln calls (the same way .finally gates
connect) so that in the promise resolution/rejection paths you return early when
cancelled is true, e.g. check !cancelled before computing details and calling
terminalRuntimeRegistry.getTerminal(terminalId, terminalInstanceId)?.writeln in
the .then and before the terminalRuntimeRegistry.getTerminal(...)? .writeln in
the .catch; keep the existing optional chaining to remain safe.
In `@packages/host-service/src/terminal/terminal.ts`:
- Around line 597-609: Update the JSON error payload sent when a session is
missing to explicitly suggest calling terminal.ensureSession or providing a
workspaceId; specifically modify the block that checks sessions.get(terminalId)
(uses terminalId, c.req.query, sendMessage, ws.close) so the sendMessage(...)
error message includes guidance like "call terminal.ensureSession or provide
workspaceId", while keeping the ws.close(1011, "Terminal session not found")
short per RFC limits.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a90e653e-aebd-4765-9d1a-a3b1a3ae2e39
📒 Files selected for processing (3)
apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsxpackages/host-service/src/terminal/terminal.ts
Kitenite
force-pushed
the
debug-terminal-connection
branch
from
d478bbc to
a290a00
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts (2)
188-199: Optional: derivewillReconnectfromscheduleReconnectinstead of duplicating its gate.
willReconnecthere re-implements the same predicatesscheduleReconnectchecks (_reconnectTimer,currentUrl/_terminalset,_reconnectAttempt < MAX_RECONNECT_ATTEMPTS). They're correct today, but ifscheduleReconnect's gating ever changes (e.g., a new bail-out condition), the user-facing message will silently drift from the actual behavior. Consider havingscheduleReconnectreturn whether it scheduled, and use that to pick between "Reconnecting..." vs "Max reconnect attempts reached."♻️ Sketch
-function scheduleReconnect(transport: TerminalTransport) { - if (transport._reconnectTimer) return; - if (transport._exited) return; - if (!transport.currentUrl || !transport._terminal) return; - if (transport._reconnectAttempt >= MAX_RECONNECT_ATTEMPTS) return; +function scheduleReconnect(transport: TerminalTransport): boolean { + if (transport._reconnectTimer) return false; + if (transport._exited) return false; + if (!transport.currentUrl || !transport._terminal) return false; + if (transport._reconnectAttempt >= MAX_RECONNECT_ATTEMPTS) return false; @@ + return true; } @@ - if (!transport._exited && event.code !== 1000) { - const willReconnect = - !transport._reconnectTimer && - Boolean(transport.currentUrl && transport._terminal) && - transport._reconnectAttempt < MAX_RECONNECT_ATTEMPTS; - terminal.writeln( - `\r\n[terminal] WebSocket closed while connected to ${formatWsEndpoint(transport.currentUrl)} (${formatCloseDetails(event)}). ${willReconnect ? "Reconnecting..." : "Max reconnect attempts reached."}`, - ); - } - // Auto-reconnect on unexpected close (host-service restart, network blip) - scheduleReconnect(transport); + const scheduled = scheduleReconnect(transport); + if (!transport._exited && event.code !== 1000) { + terminal.writeln( + `\r\n[terminal] WebSocket closed while connected to ${formatWsEndpoint(transport.currentUrl)} (${formatCloseDetails(event)}). ${scheduled ? "Reconnecting..." : "Max reconnect attempts reached."}`, + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts` around lines 188 - 199, The message computes willReconnect locally duplicating scheduleReconnect's gating; change scheduleReconnect(transport) to return a boolean indicating whether it scheduled a reconnect, then call const didSchedule = scheduleReconnect(transport) and use didSchedule to decide the terminal.writeln text instead of recomputing transport._reconnectTimer / transport.currentUrl / transport._terminal / transport._reconnectAttempt < MAX_RECONNECT_ATTEMPTS; update scheduleReconnect's callers accordingly so the single source of truth for reconnect gating is the scheduleReconnect function (referenced symbols: scheduleReconnect, willReconnect, transport._reconnectTimer, transport.currentUrl, transport._terminal, transport._reconnectAttempt, MAX_RECONNECT_ATTEMPTS, terminal.writeln, formatWsEndpoint, formatCloseDetails).
201-206: Note:errorevent has no actionable detail; consider deduping with the close log.Browser
WebSocketEvents onerrorcarry no usable info, and a failed connection always fires botherrorandclose. With this change, every transport failure now writes two[terminal] WebSocket ...lines (error + close). Not incorrect, but consider whether you want to suppress this line when aclosewill follow within the same task to keep the terminal output less noisy. Up to you.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts` around lines 201 - 206, The error handler currently always writes a terminal line and causes a duplicate message because a subsequent close also logs; instead, dedupe by having the error handler set a transient flag (e.g., transport._sawSocketError) and NOT call terminal.writeln, then have the existing socket "close" handler consult that flag and emit a single consolidated terminal.writeln using formatWsEndpoint(transport.currentUrl); ensure you clear the flag after use so future connections behave normally (update socket.addEventListener("error", ...), the transport object, and the socket close handler accordingly).
🤖 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/lib/terminal/terminal-ws-transport.ts`:
- Around line 188-199: The message computes willReconnect locally duplicating
scheduleReconnect's gating; change scheduleReconnect(transport) to return a
boolean indicating whether it scheduled a reconnect, then call const didSchedule
= scheduleReconnect(transport) and use didSchedule to decide the
terminal.writeln text instead of recomputing transport._reconnectTimer /
transport.currentUrl / transport._terminal / transport._reconnectAttempt <
MAX_RECONNECT_ATTEMPTS; update scheduleReconnect's callers accordingly so the
single source of truth for reconnect gating is the scheduleReconnect function
(referenced symbols: scheduleReconnect, willReconnect,
transport._reconnectTimer, transport.currentUrl, transport._terminal,
transport._reconnectAttempt, MAX_RECONNECT_ATTEMPTS, terminal.writeln,
formatWsEndpoint, formatCloseDetails).
- Around line 201-206: The error handler currently always writes a terminal line
and causes a duplicate message because a subsequent close also logs; instead,
dedupe by having the error handler set a transient flag (e.g.,
transport._sawSocketError) and NOT call terminal.writeln, then have the existing
socket "close" handler consult that flag and emit a single consolidated
terminal.writeln using formatWsEndpoint(transport.currentUrl); ensure you clear
the flag after use so future connections behave normally (update
socket.addEventListener("error", ...), the transport object, and the socket
close handler accordingly).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f7ee8110-e8fe-4cfe-9617-fec5c95cce07
📒 Files selected for processing (3)
apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsxpackages/host-service/src/terminal/terminal.ts
1 participant
Summary
terminal.ensureSessionfailures directly in the v2 terminalRoot Cause
The terminal startup path could collapse distinct failures into generic messages like
[terminal] websocket errororSession not found, hiding whether the failure came fromensureSession, host-service session lookup, or the WebSocket transport.Impact
No terminal startup/control-flow behavior changed. This is diagnostics-only; v2 still waits for
ensureSessionbefore connecting.Validation
bun run lint:fixbun run testbun run --cwd packages/host-service typecheckbun run --cwd apps/desktop typecheckNotes
bun testfails because it does not load the desktop package Bun preload setup (apps/desktop/bunfig.toml); the repo test scriptbun run testpasses.Summary by cubic
Improves terminal connection diagnostics in the v2 terminal. Errors now identify the failing step and show a sanitized WebSocket endpoint with close details; runtime behavior is unchanged.
terminal.ensureSessionfailures in-terminal, including session creation errors and request failures.host-service, replace generic “Session not found” with aterminalId-aware message suggestingterminal.ensureSessionorworkspaceId, and use a clearer close reason.Written for commit a290a00. Summary will update on new commits. Review in cubic
Summary by CodeRabbit