Conversation
Problem: every v2-workspace switch yanked the xterm wrapper out of the DOM and re-opened a WebSocket, producing a visible "switching and reattaching" flash instead of VSCode-style hide/show. The v2-workspace layout's WorkspaceTrpcProvider has a load-bearing key that unmounts the whole subtree on every switch, so the React component for TerminalPane goes away — but the xterm instance, wrapper div, and transport in terminalRuntimeRegistry should survive, and previously the DOM node didn't. Three entangled issues were fixed together: 1. Parking container. On detach, the wrapper used to be wrapper.remove()'d, taking the rendered canvases with it. Now it's appended to a hidden body-level div (#v2-terminal-parking) so xterm stays attached to the document. Re-mount in the new workspace is a DOM move back from parking to the live container — mirrors the existing v1 persistent-webview pattern at usePersistentWebview.ts:14-27 and VSCode's TerminalInstance setVisible model. 2. DOM vs transport split. The previous single registry.attach() both mounted DOM and opened the WebSocket. That forced TerminalPane to gate attach on ensureSession, which made warm returns wait on a tRPC round-trip (visible delay) and made cold mounts race — opening a WS before the server session existed produced "Session not found. Call terminal. ensureSession first.". Split into mount() (synchronous, DOM-only, safe on every mount) and connect() (called only after ensureSession resolves). Matches VSCode's TerminalInstance.attachToElement + _createProcess and Tabby's XTermFrontend.attach + setSession. 3. Effect dep narrowing. TerminalPane's attach effect used to depend on [terminalId, websocketUrl, initialThemeType, workspaceId]; prop churn during the provider key remount (workspaceId flipping while pane data caught up) forced repeated detach/attach cycles. Narrowed to [terminalId] with the others read through refs. websocketUrl changes now go through registry.reconnect(), which is hard-gated on transport already being live so it never opens a WS before ensureSession has resolved. Log walkthrough on a warm workspace switch: pane:effect-cleanup → registry:detach → runtime:detach (wrapper parks) pane:effect-mount → registry:mount → runtime:attach wasParked:true registry:reconnect-skip same-url pane:ensureSession-ok → registry:connect → transport:connect-skip idempotent Instrumentation stays in this commit for the rollout check; a follow-up strips the termLog calls once this is confirmed in production.
Removes the temporary termLog tracing added to diagnose the workspace-switch reattach bug. The behavior fix (parking + mount/connect split) landed in the previous commit and is confirmed stable in logs; no functional change here.
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughDecouples terminal DOM mounting from transport: replaces Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Component as TerminalPane (React)
participant Registry as TerminalRuntimeRegistry
participant Runtime as TerminalRuntime (DOM)
participant Transport as Transport / WebSocket
Component->>Registry: mount(container, terminalId, options)
Registry->>Runtime: create or reuse runtime.wrapper
Runtime->>Runtime: attach wrapper to container (or move to parking on detach)
Component-->>Registry: ensureSession() (async)
Component->>Registry: connect(terminalId, wsUrl) -- after session attempt
Registry->>Transport: open or reuse transport for terminalId with wsUrl
Transport->>Transport: WebSocket connect / handshake
Transport-->>Registry: connected / error
Registry-->>Component: connection state/events
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
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 PR re-architects the v2 terminal lifecycle to eliminate workspace-switch flicker and the "Session not found" race: on detach the xterm wrapper is parked in a hidden body-level container (instead of being removed), and the old single Confidence Score: 4/5Safe to merge; two P2 findings worth addressing before shipping — one accessibility gap and one transport-state edge case — but neither blocks the primary fix. The parking + DOM-first lifecycle strategy is well-reasoned and the
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/lib/terminal/terminal-runtime.ts | Adds getParkingContainer() (body-level hidden div) and changes detachFromContainer to move the xterm wrapper there instead of removing it; adds idempotency guard to attachToContainer. Minor concern: parking container lacks inert, leaving parked xterm textareas keyboard-reachable. |
| apps/desktop/src/renderer/lib/terminal/terminal-runtime-registry.ts | Splits attach into mount (synchronous DOM), connect (WebSocket), and new reconnect (URL-change-only swap). Logic is sound but reconnect doesn't guard the "connecting" state, which could trigger a double-connect if a URL change races an in-flight WebSocket handshake. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx | Main lifecycle effect narrowed to [terminalId] with prop values read through refs; mount called synchronously before async ensureSession→connect chain; separate reconnect effect handles URL changes. The cancelled flag correctly prevents stale connects on cleanup. |
Sequence Diagram
sequenceDiagram
participant React as TerminalPane
participant Reg as Registry
participant RT as TerminalRuntime
participant Park as ParkingContainer
participant WS as WebSocket
Note over React: Cold mount
React->>Reg: mount(id, container, appearance)
Reg->>RT: createRuntime() / attachToContainer()
RT-->>React: xterm visible (blank cursor)
React->>Reg: ensureSession → connect(id, wsUrl)
Reg->>WS: open WebSocket
Note over React: Workspace switch (unmount)
React->>Reg: detach(id)
Reg->>RT: detachFromContainer()
RT->>Park: appendChild(wrapper)
Note over WS: Transport stays alive
Note over React: Warm return (remount)
React->>Reg: mount(id, container, appearance)
Reg->>RT: attachToContainer() — reparents wrapper
Park-->>RT: wrapper moved back
RT-->>React: xterm visible (buffer preserved)
React->>Reg: ensureSession → connect(id, wsUrl)
Reg->>WS: no-op (already connected)
Note over React: URL change (token refresh)
React->>Reg: reconnect(id, newUrl)
alt transport disconnected
Reg-->>React: no-op (ensureSession path handles it)
else transport live
Reg->>WS: reconnect to new URL
end
Prompt To Fix All With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/lib/terminal/terminal-runtime.ts
Line: 133-147
Comment:
**Parked terminals remain keyboard-focusable**
The parking container hides terminals visually and disables mouse interaction via `pointerEvents: none`, but xterm's internal `<textarea>` elements stay in the tab order. Users cycling through focusable elements with Tab can land inside an off-screen, inactive terminal.
Setting `inert` on the container prevents both keyboard focus and accessibility-tree exposure without touching individual child nodes:
```suggestion
el.id = PARKING_CONTAINER_ID;
el.setAttribute("inert", "");
el.style.position = "fixed";
el.style.left = "-9999px";
el.style.top = "-9999px";
el.style.width = "100vw";
el.style.height = "100vh";
el.style.overflow = "hidden";
el.style.pointerEvents = "none";
```
`inert` is broadly supported and is the platform-idiomatic way to make a subtree non-interactive.
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/lib/terminal/terminal-runtime-registry.ts
Line: 109-115
Comment:
**`reconnect` fires during `"connecting"` state on URL change**
The guard skips reconnection only when `connectionState === "disconnected"`. If the underlying WebSocket is still opening (`"connecting"`) and the URL changes — e.g. a fast token refresh that races the initial `ensureSession` resolution — `reconnect` will call `connect` with the new URL while the first socket is mid-handshake. If `connect` does not abort the in-flight socket first, the terminal could end up with two live transports briefly. Consider extending the guard:
```ts
if (
entry.transport.connectionState === "disconnected" ||
entry.transport.connectionState === "connecting"
) return;
```
or confirming that `connect` in `terminal-ws-transport` always cancels any in-progress socket before opening a new one.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "chore(desktop): strip v2 terminal lifecy..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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
🧹 Nitpick comments (1)
apps/desktop/src/renderer/lib/terminal/terminal-runtime.ts (1)
132-148: Consider making the parking containerinertto hide it from focus and AT.
pointer-events: noneblocks mouse events, but xterm's underlying<textarea>inside the parked wrapper is still reachable via Tab and announced by screen readers. A keyboard user tabbing through the app could land in a hidden terminal, and typed input would silently go to the parked instance. Addinginert(or at minimumaria-hidden="true"+tabindex="-1"management) would remove the subtree from focus and the accessibility tree while parked.♻️ Suggested tweak
const el = document.createElement("div"); el.id = PARKING_CONTAINER_ID; + el.inert = true; + el.setAttribute("aria-hidden", "true"); el.style.position = "fixed"; el.style.left = "-9999px"; el.style.top = "-9999px";🤖 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-runtime.ts` around lines 132 - 148, The parked terminal container created by getParkingContainer (PARKING_CONTAINER_ID) is hidden visually but still reachable by keyboard/screen readers; update getParkingContainer to mark the container and its subtree as non-interactive and removed from the accessibility tree when parked by adding the inert attribute (preferred) or, if inert isn't available, set aria-hidden="true" and ensure tabindex="-1" is managed for the container; ensure these attributes are applied before appending to document.body and removed when the container is reused so the terminal can become interactive again.
🤖 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-runtime-registry.ts`:
- Around line 109-115: Update the JSDoc for the reconnect(terminalId: string,
wsUrl: string) method to state that the guard only prevents reconnect when the
transport is in the initial "disconnected" state; clarify that reconnect is
intentionally allowed for "connecting", "open", and "closed" states (where
"closed" represents a previously live transport eligible for URL-based
reconnect), and reference entry.transport.connectionState and the
connect(entry.transport, entry.runtime.terminal, wsUrl) call so readers know
which symbols govern the behavior.
---
Nitpick comments:
In `@apps/desktop/src/renderer/lib/terminal/terminal-runtime.ts`:
- Around line 132-148: The parked terminal container created by
getParkingContainer (PARKING_CONTAINER_ID) is hidden visually but still
reachable by keyboard/screen readers; update getParkingContainer to mark the
container and its subtree as non-interactive and removed from the accessibility
tree when parked by adding the inert attribute (preferred) or, if inert isn't
available, set aria-hidden="true" and ensure tabindex="-1" is managed for the
container; ensure these attributes are applied before appending to document.body
and removed when the container is reused so the terminal can become interactive
again.
🪄 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: 29eae077-a0d0-49b0-ad6b-6ed8bf08d905
📒 Files selected for processing (3)
apps/desktop/src/renderer/lib/terminal/terminal-runtime-registry.tsapps/desktop/src/renderer/lib/terminal/terminal-runtime.tsapps/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}
Contributor
There was a problem hiding this comment.
2 issues 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-runtime.ts">
<violation number="1" location="apps/desktop/src/renderer/lib/terminal/terminal-runtime.ts:145">
P2: The parking container hides terminals visually and disables pointer events, but xterm's internal `<textarea>` elements retain `tabindex="0"`. Keyboard users pressing Tab can focus into an off-screen, inactive terminal. Add `el.setAttribute("inert", "")` to remove the subtree from the tab order and accessibility tree — this is well-supported and is the platform-idiomatic way to make a hidden subtree fully non-interactive.</violation>
<violation number="2" location="apps/desktop/src/renderer/lib/terminal/terminal-runtime.ts:242">
P1: Blur focus before parking the wrapper; otherwise a hidden parked terminal can remain focused and capture keystrokes after detach.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Browser webviews were being destroyed on workspace switch, discarding
guest-page state (URL, scroll, history) even though the registry was
designed to persist them.
Root cause: usePaneRegistry wired browser destruction through the Panes
library's onRemoved hook:
onRemoved: (pane) => browserRuntimeRegistry.destroy(pane.id)
Under ideal conditions the v2 layout's `key={`${workspace.id}:${hostUrl}`}`
remounts the WorkspaceTrpcProvider subtree on every switch, so each
workspace gets its own Workspace component whose previous-panes diff
never observes a cross-workspace "removal". But the remount isn't
always prompt — layout.tsx's useLiveQuery can return stale WS-A data
for a tick while page.tsx's already flipped to WS-B. During that tick
the existing WorkspaceContent stays mounted, useV2WorkspacePaneLayout
calls store.replaceState(WS-B panes) on the same store instance, and
the Panes diff correctly sees "WS-A's browser is gone" → fires
onRemoved → destroys the webview. By the time the user returns,
attach() runs the cold createEntry path and the guest page is lost.
Terminals don't hit this because destruction goes through
useGlobalTerminalLifecycle, a global sweep against every workspace's
persisted paneLayout — cross-workspace "removal" isn't a removal from
the sweep's perspective.
Fix: mirror the terminal pattern exactly.
- Added useGlobalBrowserLifecycle under
_authenticated/components/GlobalBrowserLifecycle/, following the
same shape as useGlobalTerminalLifecycle (extract pane.ids from all
workspace layouts, diff against previous, 500 ms grace delay
destroy to tolerate cross-workspace pane moves).
- Mounted <GlobalBrowserLifecycle /> alongside <GlobalTerminalLifecycle />
in _authenticated/layout.tsx.
- Removed the onRemoved wiring from usePaneRegistry.tsx — the sweep
replaces it.
Verified with instrumentation that a workspace switch on a live browser
pane no longer reaches browserRuntimeRegistry.destroy, and that closing
a browser pane still destroys after the 500 ms grace.
Followup to PR #3687 (terminal-side fix landed earlier on this branch).
Plan doc at apps/desktop/plans/20260423-1226-v2-pane-persistence-across-workspace-switch.md
captures the full root-cause analysis for both runtimes as a reference
for future pane persistence work.
Also removes a now-redundant biome-ignore comment in TerminalPane
that biome flagged as having no effect after the dep narrowing in the
earlier commit.
Kitenite
changed the title
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/desktop/src/renderer/routes/_authenticated/components/GlobalBrowserLifecycle/hooks/useGlobalBrowserLifecycle/useGlobalBrowserLifecycle.ts (2)
13-27: Minor: theas WorkspaceState<unknown>cast hides the shape contract.
row.paneLayoutis typedunknown, and the cast is taken on faith. The current code is safe at runtime becauseif (!layout?.tabs) continueandObject.values(tab.panes)both tolerate missing shape, and any entry missingkindsimply fails the=== "browser"check. Still, a narrow type guard would be nicer-to-have and would surface schema drift if the persistedpaneLayoutrepresentation ever changes (e.g., gets stored as a JSON string instead of a parsed object).♻️ Sketch
-function extractBrowserPaneIds(rows: { paneLayout: unknown }[]): Set<string> { - const ids = new Set<string>(); - for (const row of rows) { - const layout = row.paneLayout as WorkspaceState<unknown> | undefined; - if (!layout?.tabs) continue; +function isWorkspaceState( + value: unknown, +): value is WorkspaceState<unknown> { + return ( + typeof value === "object" && + value !== null && + Array.isArray((value as { tabs?: unknown }).tabs) + ); +} + +function extractBrowserPaneIds(rows: { paneLayout: unknown }[]): Set<string> { + const ids = new Set<string>(); + for (const row of rows) { + if (!isWorkspaceState(row.paneLayout)) continue; + const layout = row.paneLayout; for (const tab of layout.tabs) { for (const pane of Object.values(tab.panes)) { if (pane.kind === "browser") { ids.add(pane.id); } } } } return ids; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/components/GlobalBrowserLifecycle/hooks/useGlobalBrowserLifecycle/useGlobalBrowserLifecycle.ts` around lines 13 - 27, The function extractBrowserPaneIds currently casts row.paneLayout to WorkspaceState<unknown>; replace that unsafe cast with a proper runtime type guard (e.g., isWorkspaceState(value): value is WorkspaceState<unknown>) and use it to early-return/continue when row.paneLayout isn't the expected shape; ensure the guard verifies the presence and types of tabs and pane containers so the subsequent loops over layout.tabs and Object.values(tab.panes) are safe and will surface schema drift instead of silently trusting the cast.
7-11: Optional: shareDESTROY_DELAY_MSwith the terminal lifecycle.The JSDoc explicitly calls out that this "matches the terminal-side timing so the two runtimes behave consistently," but the two constants are currently declared independently in each hook. Extracting a single shared constant (e.g., alongside the pane-persistence plan or in a small shared module) would prevent the two from silently drifting if one is later tuned.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/components/GlobalBrowserLifecycle/hooks/useGlobalBrowserLifecycle/useGlobalBrowserLifecycle.ts` around lines 7 - 11, The local DESTROY_DELAY_MS in useGlobalBrowserLifecycle should be extracted to a single shared constant so it can't drift from the terminal-side value; create a small shared module (e.g., paneLifecycleConstants or alongside the pane-persistence plan) that exports DESTROY_DELAY_MS, replace the local const in useGlobalBrowserLifecycle with an import of that shared constant, and update the terminal-side lifecycle to import the same shared constant as well so both runtimes use the identical 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/components/GlobalBrowserLifecycle/hooks/useGlobalBrowserLifecycle/useGlobalBrowserLifecycle.ts`:
- Around line 13-27: The function extractBrowserPaneIds currently casts
row.paneLayout to WorkspaceState<unknown>; replace that unsafe cast with a
proper runtime type guard (e.g., isWorkspaceState(value): value is
WorkspaceState<unknown>) and use it to early-return/continue when row.paneLayout
isn't the expected shape; ensure the guard verifies the presence and types of
tabs and pane containers so the subsequent loops over layout.tabs and
Object.values(tab.panes) are safe and will surface schema drift instead of
silently trusting the cast.
- Around line 7-11: The local DESTROY_DELAY_MS in useGlobalBrowserLifecycle
should be extracted to a single shared constant so it can't drift from the
terminal-side value; create a small shared module (e.g., paneLifecycleConstants
or alongside the pane-persistence plan) that exports DESTROY_DELAY_MS, replace
the local const in useGlobalBrowserLifecycle with an import of that shared
constant, and update the terminal-side lifecycle to import the same shared
constant as well so both runtimes use the identical value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0186cccd-a828-46b8-8458-90cc33b610ec
📒 Files selected for processing (8)
apps/desktop/plans/20260423-1226-v2-pane-persistence-across-workspace-switch.mdapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsxapps/desktop/src/renderer/routes/_authenticated/components/GlobalBrowserLifecycle/GlobalBrowserLifecycle.tsxapps/desktop/src/renderer/routes/_authenticated/components/GlobalBrowserLifecycle/hooks/useGlobalBrowserLifecycle/index.tsapps/desktop/src/renderer/routes/_authenticated/components/GlobalBrowserLifecycle/hooks/useGlobalBrowserLifecycle/useGlobalBrowserLifecycle.tsapps/desktop/src/renderer/routes/_authenticated/components/GlobalBrowserLifecycle/index.tsapps/desktop/src/renderer/routes/_authenticated/layout.tsx
✅ Files skipped from review due to trivial changes (3)
- apps/desktop/src/renderer/routes/_authenticated/components/GlobalBrowserLifecycle/index.ts
- apps/desktop/src/renderer/routes/_authenticated/components/GlobalBrowserLifecycle/hooks/useGlobalBrowserLifecycle/index.ts
- apps/desktop/plans/20260423-1226-v2-pane-persistence-across-workspace-switch.md
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx
… doc Two follow-ups from PR #3687 review: - `inert` on #v2-terminal-parking. Parked terminals' internal <textarea> still had `tabindex=0`, so a keyboard user tabbing through the app could land in an off-screen terminal and have keystrokes silently go to the wrong pane. `inert` removes the subtree from the tab order and the accessibility tree, and moves focus out automatically, which also handles the "blur before park" concern for free. aria-hidden added for belt-and-suspenders on older engines. - `reconnect` JSDoc. Clarify that the guard only skips `"disconnected"` (never-opened transport, caller should use ensureSession + connect path). `"connecting"`, `"open"`, `"closed"` are all intentionally allowed through — `connect()` aborts any in-flight or stale socket before opening the new one.
- Collapse TerminalPane's .then()/.catch() into .catch()/.finally(): one connect() call site instead of two identical guarded calls. Same semantics (connect after ensureSession settles, even on rejection), cancellation check consolidated. - Merge the three connect-related comments into one block explaining "connect regardless of outcome" + idempotency. Drop the redundant "// DOM first" inline that the block comment above already covered. - Trim detachFromContainer's 5-line comment down to 2 lines pointing at getParkingContainer — the helper's docstring already explains the parking rationale in full.
The module-level `subscribeToState(terminalId)` built a fresh closure on
every render, so `useSyncExternalStore` saw a new subscribe function each
time and re-subscribed to `terminalRuntimeRegistry.onStateChange` on every
TerminalPane render — this is the anti-pattern React's useSyncExternalStore
docs explicitly warn about ("If you don't memoize the subscribe function,
React will resubscribe to your store every time your component re-renders").
Inline the helpers and wrap both subscribe + getSnapshot in useCallback
keyed on [terminalId]. Re-subscribe now only fires when the pane's
terminalId actually changes (cold create / destroy), not on every
keystroke-triggered re-render.
Kitenite
deleted the
please-debug-our-terminal-is-now-switching-and-reattaching-when-switching-workspace-instead-of-being
branch
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
11 tasks
8 tasks
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
Both v2 terminals and browsers were visibly rebuilt on every workspace switch. Root cause is a shared lifecycle hazard — the v2 layout's
WorkspaceTrpcProviderkeyremount + the wayuseLiveQueryupdates can propagate — but the fixes are different for each runtime.Terminal (commit
e56992653)#v2-terminal-parkingdiv on detach instead ofwrapper.remove(), so xterm stays in the document and the next mount is a DOM move back.registry.attachintomount(synchronous DOM) andconnect(called only afterensureSession). Matches VSCodeTerminalInstance.attachToElement+_createProcessand Tabby'sXTermFrontend.attach+setSession. Eliminates the "Session not found" race on cold mount and the tRPC-gated delay on warm return.TerminalPane's attach effect deps to[terminalId];workspaceId/websocketUrlgo through refs.websocketUrlchanges route throughregistry.reconnect(), which hard-guards on the transport already being live.Browser (commit
3b88c0207)onRemoved: (pane) => browserRuntimeRegistry.destroy(pane.id)inusePaneRegistrywas firing during workspace switches when the provider key didn't remount promptly: the existingWorkspaceContentstayed mounted,useV2WorkspacePaneLayoutcalledstore.replaceState(WS-B panes)on the same store, and the Panes library's diff correctly observed "WS-A's browser is gone now" → destroyed the webview. Return trip ran the coldcreateEntrypath, losing state.useGlobalBrowserLifecyclesweeper, mirrorsuseGlobalTerminalLifecycle— diffs browser pane ids across every workspace's persistedpaneLayoutand destroys with a 500 ms grace delay. TheonRemovedwiring is gone.Reference
apps/desktop/plans/20260423-1226-v2-pane-persistence-across-workspace-switch.mdcaptures full root-cause analysis for both runtimes, the shared "register state outside the remounting subtree" pattern, and a note on the code editor (same class of risk, not reported yet).Test plan
#v2-terminal-parkingand#browser-runtime-rootvisible ondocument.bodyin DevTools as expectedSummary by CodeRabbit
Bug Fixes
Improvements
New Features
Documentation