[codex] Stabilize renderer stress with minimal terminal changes#4570
Open
Kitenite wants to merge 18 commits into
Open
[codex] Stabilize renderer stress with minimal terminal changes#4570Kitenite wants to merge 18 commits into
Kitenite wants to merge 18 commits into
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:
📝 WalkthroughWalkthroughAdds a development-only renderer stress harness (CDP scripts, fixture generator, in-app bridge), wires stress-debug attributes and navigation hooks into V2 workspace UI, introduces terminal/browser runtime stress utilities and deferred disposal/pruning, and adds a host-service git diff-stat TRPC query with parsing and tests. ChangesRenderer stress, UI wiring, terminal & browser runtime
Host-service git diff stats
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
🚥 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 |
Contributor
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
Kitenite
changed the title
![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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/utils/workspace-navigation.ts`:
- Around line 113-123: The current code immediately calls
queuedV2WorkspaceNavigation?.resolve(), which settles a superseded
navigateToV2Workspace caller before navigation completes; instead, change the
queuing logic in workspace-navigation so that when a new request supersedes an
existing queuedV2WorkspaceNavigation you call its reject with a clear
SupersededError (or similar) or otherwise defer settling previous promises until
the scheduled drain finishes; specifically update the block that assigns
queuedV2WorkspaceNavigation and calls scheduleV2WorkspaceNavigationDrain() to
reject the prior queuedV2WorkspaceNavigation (using its reject()) or accumulate
callers and only resolve/reject them when the actual navigation drain
(scheduleV2WorkspaceNavigationDrain / the drain handler) completes.
- Around line 166-170: A pending bare-workspace switch enqueued by
enqueueV2WorkspaceNavigation can later fire and overwrite a newer immediate
navigation that includes search params; before calling
performV2WorkspaceNavigation(workspaceId, ...) you must cancel/clear any queued
bare-workspace entry for the same workspaceId. Add a call to the cancellation
helper (e.g. clearEnqueuedV2WorkspaceNavigation(workspaceId) or
cancelQueuedV2WorkspaceNavigation(workspaceId)) right before
performV2WorkspaceNavigation, or implement such a helper to clear the
timer/queue entry created by enqueueV2WorkspaceNavigation so the queued timer
cannot later navigate without terminalId/chatSessionId/openUrl*.
🪄 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: c7ca410a-dca6-4a42-b523-5ff155a57050
📒 Files selected for processing (8)
apps/desktop/scripts/stress-renderer.tsapps/desktop/src/renderer/lib/terminal/terminal-runtime-registry.test.tsapps/desktop/src/renderer/lib/terminal/terminal-runtime-registry.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/hooks/useDashboardSidebarWorkspaceItemActions/useDashboardSidebarWorkspaceItemActions.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/utils/workspace-navigation.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalRemoteControlButton/TerminalRemoteControlButton.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/desktop/src/renderer/lib/terminal/terminal-runtime-registry.test.ts
- apps/desktop/src/renderer/lib/terminal/terminal-runtime-registry.ts
- apps/desktop/scripts/stress-renderer.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/browserRuntimeRegistry.ts (2)
427-436: 💤 Low valuePrune loop logic looks correct; minor style nit on
return.The detached-only filter plus ascending
lastUsedAtsort correctly evicts least-recently-used detached entries, and becausedetach()/attach()bumplastUsedAtbefore pruning, the just-detached entry won’t be the eviction target. One small nit:if (!paneId) return;exits the whole method on a falsy paneId — since Map keys here are non-empty strings, this branch is unreachable, butbreak(or omitting the guard) communicates intent more clearly.♻️ Optional tweak
private pruneDetachedRuntimes(): void { const detachedEntries = Array.from(this.entries.entries()) .filter(([, entry]) => !entry.visible) .sort(([, a], [, b]) => a.lastUsedAt - b.lastUsedAt); while (detachedEntries.length > MAX_DETACHED_BROWSER_RUNTIMES) { const [paneId] = detachedEntries.shift() ?? []; - if (!paneId) return; + if (!paneId) break; this.destroy(paneId); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/BrowserPane/browserRuntimeRegistry.ts around lines 427 - 436, The guard inside pruneDetachedRuntimes currently does `if (!paneId) return;` which inadvertently suggests an early-return on an impossible falsy key; change this to `break` (or remove the guard entirely) so the loop only stops the current prune iteration rather than exiting the whole method, leaving the rest of the eviction loop to continue; update the pruneDetachedRuntimes method that iterates over this.entries, uses detachedEntries and calls this.destroy(paneId) to use `break` (or drop the check) instead of `return`.
164-179: 💤 Low valueConsider moving
lastUsedAtupdate past the no-op early return.
entry.lastUsedAt = Date.now()runs before theif (!changed) returnguard, so every webview event that produces an identical state patch still refreshes the LRU timestamp. For detached entries that keep receiving background events (e.g., latedid-stop-loading, repeated favicon updates with the same URL), this can defer eviction beyond what the LRU intends. If you want “any activity counts as used” keep as is; otherwise move the assignment below the change check so only real state mutations bump the timestamp.♻️ Optional tweak
private setState(paneId: string, patch: Partial<BrowserRuntimeState>) { const entry = this.entries.get(paneId); if (!entry) return; - entry.lastUsedAt = Date.now(); let changed = false; for (const key in patch) { const k = key as keyof BrowserRuntimeState; if (entry.state[k] !== patch[k]) { changed = true; break; } } if (!changed) return; + entry.lastUsedAt = Date.now(); entry.state = { ...entry.state, ...patch }; this.notify(paneId); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/BrowserPane/browserRuntimeRegistry.ts around lines 164 - 179, The setState method currently updates entry.lastUsedAt before checking whether the incoming patch actually changes state; move the assignment to entry.lastUsedAt = Date.now() so it happens only after you detect a real change (i.e., after computing `changed` and before assigning the merged state and calling this.notify), ensuring only real state mutations in setState (not no-op patches) bump the LRU timestamp; locate this in the setState function where `entry`, `changed`, `patch`, `entry.state`, and `this.notify(paneId)` are referenced.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/BrowserPane/browserRuntimeRegistry.ts:
- Around line 427-436: The guard inside pruneDetachedRuntimes currently does `if
(!paneId) return;` which inadvertently suggests an early-return on an impossible
falsy key; change this to `break` (or remove the guard entirely) so the loop
only stops the current prune iteration rather than exiting the whole method,
leaving the rest of the eviction loop to continue; update the
pruneDetachedRuntimes method that iterates over this.entries, uses
detachedEntries and calls this.destroy(paneId) to use `break` (or drop the
check) instead of `return`.
- Around line 164-179: The setState method currently updates entry.lastUsedAt
before checking whether the incoming patch actually changes state; move the
assignment to entry.lastUsedAt = Date.now() so it happens only after you detect
a real change (i.e., after computing `changed` and before assigning the merged
state and calling this.notify), ensuring only real state mutations in setState
(not no-op patches) bump the LRU timestamp; locate this in the setState function
where `entry`, `changed`, `patch`, `entry.state`, and `this.notify(paneId)` are
referenced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 68cc2c7b-03da-4bd3-92dd-c5ff90f7fc3c
📒 Files selected for processing (2)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/browserRuntimeRegistry.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.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.
10 issues found across 39 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/main/index.ts">
<violation number="1" location="apps/desktop/src/main/index.ts:61">
P2: Validate `SUPERSET_RENDERER_STRESS_CDP_PORT` is within the valid port range before enabling CDP; numeric-only validation accepts invalid ports.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarShortcuts/useDashboardSidebarShortcuts.ts">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarShortcuts/useDashboardSidebarShortcuts.ts:111">
P2: Handle rejected `navigateToV2Workspace` promises in these hotkey paths. Prefixing with `void` only marks the Promise as intentionally ignored; it does not prevent unhandled rejections if navigation fails.
(Based on your team's feedback about handling async rejections explicitly in this file.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/desktop/src/renderer/index.tsx">
<violation number="1" location="apps/desktop/src/renderer/index.tsx:54">
P2: Handle navigation failures inside the dev stress helper to avoid unhandled promise rejections.
(Based on your team's feedback about handling async rejections explicitly.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/DashboardSidebarWorkspaceItem.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/DashboardSidebarWorkspaceItem.tsx:220">
P2: Passing `diffStats` as `null` unconditionally removes diff-stat badges for all workspace rows. Preserve the previous pending-only behavior so non-pending workspaces can still show additions/deletions.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/browserRuntimeRegistry.ts">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/browserRuntimeRegistry.ts:434">
P2: Detached-runtime eviction now calls `destroy()`, but unregister errors are silently swallowed. This hides backend sync failures introduced by the new LRU eviction path.
(Based on your team's feedback about surfacing async unregister failures instead of swallowing them.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspaces/components/V2WorkspacesList/components/V2WorkspaceRow/V2WorkspaceRow.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspaces/components/V2WorkspacesList/components/V2WorkspaceRow/V2WorkspaceRow.tsx:61">
P2: Handle the navigation promise rejection instead of discarding it with `void`, otherwise failed workspace transitions can surface as unhandled promise rejections.
(Based on your team's feedback about handling async navigation errors explicitly.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/utils/workspace-navigation.ts">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/utils/workspace-navigation.ts:170">
P2: Immediate search navigations do not clear pending queued workspace switches, so an older queued navigation can run afterward and override the user’s latest action.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarHoverCardOverlay/DashboardSidebarHoverCardOverlay.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarHoverCardOverlay/DashboardSidebarHoverCardOverlay.tsx:30">
P2: Gating `useDiffStats` by `open` can leave hover-card diff stats stale after reopening, because updates are event-driven and the query cache is never considered stale.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalRemoteControlButton/TerminalRemoteControlButton.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalRemoteControlButton/TerminalRemoteControlButton.tsx:79">
P2: `localActive` fallback is applied even when fresh query data says there is no live session, so the button can incorrectly stay “live” with a stale link after server-side revoke.</violation>
</file>
<file name="apps/desktop/scripts/prepare-renderer-stress-fixtures.ts">
<violation number="1" location="apps/desktop/scripts/prepare-renderer-stress-fixtures.ts:565">
P2: Avoid swallowing `git add` failures; emit a contextual warning so incomplete fixture setup is detectable.
(Based on your team's feedback about avoiding empty catch blocks that hide async failures.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Re-trigger cubic
| if (workspace) { | ||
| revealWorkspace(workspace.id); | ||
| navigateToV2Workspace(workspace.id, navigate); | ||
| void navigateToV2Workspace(workspace.id, navigate); |
Contributor
There was a problem hiding this comment.
P2: Handle rejected navigateToV2Workspace promises in these hotkey paths. Prefixing with void only marks the Promise as intentionally ignored; it does not prevent unhandled rejections if navigation fails.
(Based on your team's feedback about handling async rejections explicitly in this file.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarShortcuts/useDashboardSidebarShortcuts.ts, line 111:
<comment>Handle rejected `navigateToV2Workspace` promises in these hotkey paths. Prefixing with `void` only marks the Promise as intentionally ignored; it does not prevent unhandled rejections if navigation fails.
(Based on your team's feedback about handling async rejections explicitly in this file.) </comment>
<file context>
@@ -108,7 +108,7 @@ export function useDashboardSidebarShortcuts(
if (workspace) {
revealWorkspace(workspace.id);
- navigateToV2Workspace(workspace.id, navigate);
+ void navigateToV2Workspace(workspace.id, navigate);
}
},
</file context>
Suggested change
| void navigateToV2Workspace(workspace.id, navigate); | |
| void navigateToV2Workspace(workspace.id, navigate).catch((error) => { | |
| console.warn("Failed to navigate to workspace via shortcut", error); | |
| }); |
| renameValue={renameValue} | ||
| shortcutLabel={shortcutLabel} | ||
| diffStats={isPending ? null : diffStats} | ||
| diffStats={null} |
Contributor
There was a problem hiding this comment.
P2: Passing diffStats as null unconditionally removes diff-stat badges for all workspace rows. Preserve the previous pending-only behavior so non-pending workspaces can still show additions/deletions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/DashboardSidebarWorkspaceItem.tsx, line 220:
<comment>Passing `diffStats` as `null` unconditionally removes diff-stat badges for all workspace rows. Preserve the previous pending-only behavior so non-pending workspaces can still show additions/deletions.</comment>
<file context>
@@ -219,11 +217,12 @@ export function DashboardSidebarWorkspaceItem({
renameValue={renameValue}
shortcutLabel={shortcutLabel}
- diffStats={isPending ? null : diffStats}
+ diffStats={null}
workspaceStatus={workspaceStatus}
isInSection={isInSection}
</file context>
| const handleOpen = useCallback(() => { | ||
| const open = () => navigateToV2Workspace(workspace.id, navigate); | ||
| const open = () => { | ||
| void navigateToV2Workspace(workspace.id, navigate); |
Contributor
There was a problem hiding this comment.
P2: Handle the navigation promise rejection instead of discarding it with void, otherwise failed workspace transitions can surface as unhandled promise rejections.
(Based on your team's feedback about handling async navigation errors explicitly.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspaces/components/V2WorkspacesList/components/V2WorkspaceRow/V2WorkspaceRow.tsx, line 61:
<comment>Handle the navigation promise rejection instead of discarding it with `void`, otherwise failed workspace transitions can surface as unhandled promise rejections.
(Based on your team's feedback about handling async navigation errors explicitly.) </comment>
<file context>
@@ -57,7 +57,9 @@ export function V2WorkspaceRow({
const handleOpen = useCallback(() => {
- const open = () => navigateToV2Workspace(workspace.id, navigate);
+ const open = () => {
+ void navigateToV2Workspace(workspace.id, navigate);
+ };
if (workspace.hostType === "local-device") {
</file context>
Suggested change
| void navigateToV2Workspace(workspace.id, navigate); | |
| void navigateToV2Workspace(workspace.id, navigate).catch((error) => { | |
| console.error("Failed to navigate to workspace", { | |
| workspaceId: workspace.id, | |
| error, | |
| }); | |
| }); |
|
|
||
| const open = hoveredId !== null && payload !== null && !contextMenuOpen; | ||
| const diffStats = useDiffStats(hoveredId ?? ""); | ||
| const diffStats = useDiffStats(hoveredId ?? "", { enabled: open }); |
Contributor
There was a problem hiding this comment.
P2: Gating useDiffStats by open can leave hover-card diff stats stale after reopening, because updates are event-driven and the query cache is never considered stale.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarHoverCardOverlay/DashboardSidebarHoverCardOverlay.tsx, line 30:
<comment>Gating `useDiffStats` by `open` can leave hover-card diff stats stale after reopening, because updates are event-driven and the query cache is never considered stale.</comment>
<file context>
@@ -27,7 +27,7 @@ export function DashboardSidebarHoverCardOverlay({
const open = hoveredId !== null && payload !== null && !contextMenuOpen;
- const diffStats = useDiffStats(hoveredId ?? "");
+ const diffStats = useDiffStats(hoveredId ?? "", { enabled: open });
// Suppress the transform transition until Radix has placed the popover at
</file context>
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="apps/desktop/src/renderer/lib/terminal/terminal-runtime-registry.ts">
<violation number="1" location="apps/desktop/src/renderer/lib/terminal/terminal-runtime-registry.ts:121">
P2: Avoid swallowing errors in this catch block. Log the failure (or rethrow) so atlas-clear failures are observable during stress diagnostics.
(Based on your team's feedback about handling errors explicitly and avoiding empty catch blocks.) [FEEDBACK_USED]</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
Ready to review this PR? Stage has broken it down into 6 individual chapters for you: 
Chapters generated by Stage for commit c1313b3 on May 16, 2026 1:22am UTC. |
Contributor
There was a problem hiding this comment.
6 issues found across 28 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/posthog.ts">
<violation number="1">
P1: This init call removed explicit PostHog capture opt-outs, which reverts to default autocapture/session-recording behavior. Re-add the explicit disable flags to avoid unintended client-side data capture and event volume.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/layout.tsx">
<violation number="1">
P2: This guard now blocks all rendering when `isReady` is false, which defeats the in-flight `cloudRow` fallback and can hide workspace creation/error states behind a blank loading view.
(Based on your team's feedback about verifying the removed `!workspace` loading guard behavior.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalRemoteControlButton/TerminalRemoteControlButton.tsx">
<violation number="1">
P2: `hydrate` swallows polling errors without surfacing them, which hides recurring backend/network failures and makes remote-control state issues hard to diagnose.
(Based on your team's feedback about surfacing async polling failures instead of silent catches.) [FEEDBACK_USED]</violation>
<violation number="2">
P2: Polling is now per-button interval polling, which can multiply identical `listForWorkspace` requests by the number of terminal panes in a workspace. This creates avoidable renderer/network load under multi-terminal usage.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/utils/workspace-navigation.ts">
<violation number="1">
P1: This now defaults to push navigation for V2 workspace switches, which reintroduces route-history growth during rapid switching. Preserve the previous `replace` default when already inside a V2 workspace route.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/hooks/useDashboardSidebarWorkspaceItemActions/useDashboardSidebarWorkspaceItemActions.ts">
<violation number="1">
P2: Direct navigation omits `search`, which preserves current query params and can carry workspace-specific route state into the next workspace. Explicitly clear search params here to keep sidebar workspace switching deterministic.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
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
Narrows the workspace-switching fix back down to renderer paths that reproduced the freeze, while keeping production terminal behavior close to
mainand preserving browser panes across workspace switches.Key changes:
WebglAddon.clearTextureAtlas()only when xterm reports atlas-model pressure or a large atlas page. This does not touch PTYs, replay, serialized buffers, pane ownership, or parking.clearTextureAtlas().Root Cause
The deeper renderer freeze was not primarily terminal replay. Rapid workspace switching with browser panes left inactive workspace browser webviews alive without any budget. The previous cleanup path only handled pane IDs removed from persisted state, so pane IDs in inactive workspaces still existed and their webviews, documents, frames, and listeners could accumulate across switches.
We do want browser panes to survive ordinary workspace switches, so this PR keeps that behavior and adds a cap to detached browser runtimes instead of destroying every browser on workspace unmount.
The stable terminal glyph corruption is xterm WebGL atlas pressure. Upstream has an open matching issue: xtermjs/xterm.js#5847. The local repro writes the same true-color atlas rewrite pattern, then compares a fixed sentinel frame before and after clearing the atlas. Before the guard, that detector changed 241,147 pixels; after the guard, it changed 0 pixels.
Impact
Switching workspaces should feel immediate, including when background terminals or browser panes exist. Browser panes from recently used workspaces stay live across switches. Older detached browser runtimes can be evicted once the retained detached-browser budget is exceeded, and returning to those panes recreates the webview from persisted pane state.
If a terminal loses its WebGL context, the current terminal repaints through xterm's fallback path and later terminal runtimes avoid WebGL in that renderer process. If xterm reaches the WebGL atlas-pressure path, the atlas is cleared and the visible rows are refreshed without rebuilding terminal sessions or replaying terminal history.
Validation
bun run lint:fixbun run lintbun --cwd apps/desktop typecheckgit diff --checkSUPERSET_RENDERER_STRESS_CDP_PORT=9333 bun dev, then stress-tested through the Electron renderer over CDP.workspace-switch, 1000 clicks, interval 0 ms, passed witherrorCount: 0,maxHeartbeatDelayMs: 123.2,maxLongTaskDurationMs: 122, documents4 -> 9, nodes1641 -> 1624.terminal-heavy, 2 tabs x 1 pane, 80 iterations, 120 lines, 2048-byte payloads, passed witherrorCount: 0,maxHeartbeatDelayMs: 63.4,maxLongTaskDurationMs: 0.terminal-heavy, 1500 atlas-rewrite cycles, 1 tab x 1 pane,--terminal-corruption-check, failed as expected withchangedPixels: 241147afterclearTextureAtlas().pendingAtlasClear: true, 4096px atlas pages present, andchangedPixels: 0against threshold3891.all, 1000 workspace switches, 240 route sweeps, 500 mixed-pane operations, 200 terminal-heavy operations, 32 terminal tabs x 4 panes, passed witherrorCount: 0,failures: [],maxHeartbeatDelayMs: 897.4,maxLongTaskDurationMs: 464.renderer: nullandcanvasCount: 0, confirming DOM fallback for later terminals.Notes
I checked the current xterm beta line against our installed beta. The upstream issue remains open and the newer beta commits did not show an obvious atlas/glyph-corruption fix, so this PR does not upgrade xterm.
Desktop/dev logs were noisy with Electric shape long polls, host-service tunnel reconnects, expired local Stripe test key errors, and missing local worktree watcher errors. Those were separate from the renderer stress and WebGL atlas behavior tested here.