[codex] Fix renderer stress degradation#4500
Merged
Merged
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 renderer stress fixture generator and CDP harness, refactors terminal addon lifecycle into controller modules, extends terminal runtime/registry for stress operations, introduces pending V2 workspace navigation with serialized navigation, gates tab-specific queries, and implements a git diff-stats TRPC endpoint plus tests. ChangesRenderer Stress Testing Feature
Terminal Addon Controller Architecture Refactoring
V2 Workspace Navigation State Management and UI Updates
Tab-Based Query Optimization and Efficient Data Fetching
Git Diff Stats Query and Numstat Parsing
Pull Request Runtime Graceful Shutdown & App Disposal
Misc, Tests, and Infra
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
|
Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 57 files
You’re at about 91% of the monthly review limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
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/scripts/stress-renderer.ts">
<violation number="1" location="apps/desktop/scripts/stress-renderer.ts:1200">
P1: `rendererStress` is evaluated in a different runtime, so this outer constant is out of scope and will throw at runtime in the WebGL recovery branch.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
apps/desktop/src/renderer/lib/terminal/terminal-runtime.ts (1)
123-138: ⚡ Quick winConsider extracting the double-RAF pattern to a shared utility.
This function uses the same double-RAF pattern as
afterPendingXtermRefreshinterminal-webgl-addon-controller.ts(lines 13-21). Both defer work until after the next paint with identical fallback logic for missingrequestAnimationFrame.♻️ Refactoring suggestion
Extract to a shared utility module (e.g.,
terminal-utils.tsorterminal-scheduling.ts):export function afterPendingRefresh(callback: () => void): void { if (typeof requestAnimationFrame !== "function") { setTimeout(callback, 0); return; } requestAnimationFrame(() => { requestAnimationFrame(callback); }); }Then both modules can import and use this helper.
🤖 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/lib/terminal/terminal-runtime.ts` around lines 123 - 138, The double-RAF scheduling logic duplicated in disposeTerminalAfterPendingRefresh and afterPendingXtermRefresh should be extracted into a shared helper (e.g., afterPendingRefresh) and imported where needed; create a new function afterPendingRefresh(callback: () => void) that implements the requestAnimationFrame -> requestAnimationFrame fallback to setTimeout behavior, replace the inline double-RAF in disposeTerminalAfterPendingRefresh (and in terminal-webgl-addon-controller.ts where afterPendingXtermRefresh exists) to call afterPendingRefresh(() => terminal.dispose()) or afterPendingRefresh(yourCallback), and remove the duplicated code so both modules import and use the single utility.packages/host-service/test/integration/git.integration.test.ts (1)
62-75: ⚡ Quick winAdd a same-path staged+unstaged case for
getDiffStats.This test only covers disjoint files. Add one case where the same file has both staged and unstaged edits, then assert totals include both contributions.
🤖 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 `@packages/host-service/test/integration/git.integration.test.ts` around lines 62 - 75, Add a new same-path staged+unstaged scenario in the "getDiffStats returns sidebar totals without full status payload" test: create a file (e.g., "mixed.txt"), write initial lines, stage those lines with scenario.repo.git.add("mixed.txt"), then modify/append the file with additional lines without staging the second change, and then call scenario.host.trpc.git.getDiffStats.query({ workspaceId: scenario.workspaceId }) and update the expect(stats).toEqual(...) to include additions from both the staged and unstaged edits; refer to the existing helpers used in the test such as writeFileSync, scenario.repo.git.add and scenario.host.trpc.git.getDiffStats.query to locate where to insert the new writes and assertion.
🤖 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/lib/terminal/terminal-runtime-registry.ts`:
- Around line 174-183: The listEntries method currently treats an
instanceId-only call as unscoped and returns all entries; update
listEntries(terminalId?: string, instanceId?: string) so that if instanceId is
provided without terminalId it returns an empty array (guard the instanceId-only
case) instead of falling through to Array.from(this.entries.values()); modify
the conditional flow inside listEntries (the function name and the parameters
terminalId/instanceId and use of this.getEntry / this.getEntries / this.entries)
to check for instanceId && !terminalId and return [] early.
- Around line 66-71: getWebglContext currently probes canvases by calling
canvas.getContext("webgl2" / "webgl" / "experimental-webgl"), which can allocate
new GPU contexts; stop probing in getWebglContext and instead rely on the
runtime to consult already-tracked active WebGL contexts or the xterm WebGL
addon state. Replace the getWebglContext logic so it does not call
canvas.getContext; either accept an existing
WebGLRenderingContext/WebGL2RenderingContext passed in from the terminal runtime
wrapper or query a registry/flag maintained by the runtime (or the WebGL addon)
that tracks active contexts, and update any code that invokes getWebglContext to
provide or look up the pre-existing context rather than creating one.
In `@packages/host-service/src/trpc/router/git/git.ts`:
- Around line 67-77: The current applyNumstatToStatsMap function (and the
similar blocks at the other occurrences) overwrites existing DiffStats for a
path with Map.set, losing prior additions/deletions when a file appears in
multiple buckets; change the logic to read the existing entry
(byPath.get(record.path) || {additions:0,deletions:0}), sum record.additions and
record.deletions into the existing totals, and then put the aggregated object
back into the map (byPath.set(record.path, { additions: existing.additions +
record.additions, deletions: existing.deletions + record.deletions })); apply
the same aggregation fix to the analogous code blocks referenced in the review
so totals accumulate instead of being replaced.
In
`@packages/host-service/src/trpc/router/workspace-creation/shared/git-config.ts`:
- Line 9: Summary: Remove the ad-hoc cast `git as Parameters<typeof
gitConfigWrite>[0]` by fixing the underlying type mismatch between GitClient and
SimpleGit. Fix: update the type definitions so they align — either redefine
GitClient to be an alias of SimpleGit (e.g., export type GitClient = SimpleGit |
Awaited<ReturnType<GitFactory>>> simplified to SimpleGit) or change the
gitConfigWrite signature to accept the GitClient alias (or a union like
SimpleGit | GitClient) so callers like gitConfigWrite(git, ...) no longer need
casts; apply the same change consistently for usages in git-config.ts,
adopt-existing-worktree.ts, and workspaces.ts. Ensure the chosen approach
preserves type safety and removes the need for `as Parameters<typeof
gitConfigWrite>[0]` casts.
In `@packages/host-service/src/trpc/router/workspaces/workspaces.ts`:
- Around line 911-919: The gitConfigWrite call is writing to the main repo
because it lacks the worktree '-C' argument; update the gitConfigWrite
invocation (the one that writes branch.${resolvedBranch}.base with
baseShortName) to include the worktreePath via the '-C' option so it targets the
worktree git config (match the pattern used in recordBaseBranchConfig and
enablePushAutoSetupRemote), i.e. pass the worktreePath as the first CLI args
array element after '-C' to gitConfigWrite so the config is written to the
correct worktree instead of the main repo.
---
Nitpick comments:
In `@apps/desktop/src/renderer/lib/terminal/terminal-runtime.ts`:
- Around line 123-138: The double-RAF scheduling logic duplicated in
disposeTerminalAfterPendingRefresh and afterPendingXtermRefresh should be
extracted into a shared helper (e.g., afterPendingRefresh) and imported where
needed; create a new function afterPendingRefresh(callback: () => void) that
implements the requestAnimationFrame -> requestAnimationFrame fallback to
setTimeout behavior, replace the inline double-RAF in
disposeTerminalAfterPendingRefresh (and in terminal-webgl-addon-controller.ts
where afterPendingXtermRefresh exists) to call afterPendingRefresh(() =>
terminal.dispose()) or afterPendingRefresh(yourCallback), and remove the
duplicated code so both modules import and use the single utility.
In `@packages/host-service/test/integration/git.integration.test.ts`:
- Around line 62-75: Add a new same-path staged+unstaged scenario in the
"getDiffStats returns sidebar totals without full status payload" test: create a
file (e.g., "mixed.txt"), write initial lines, stage those lines with
scenario.repo.git.add("mixed.txt"), then modify/append the file with additional
lines without staging the second change, and then call
scenario.host.trpc.git.getDiffStats.query({ workspaceId: scenario.workspaceId })
and update the expect(stats).toEqual(...) to include additions from both the
staged and unstaged edits; refer to the existing helpers used in the test such
as writeFileSync, scenario.repo.git.add and
scenario.host.trpc.git.getDiffStats.query to locate where to insert the new
writes and assertion.
🪄 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: e44c6128-48bc-41e2-8feb-d046640c1592
📒 Files selected for processing (57)
apps/desktop/package.jsonapps/desktop/scripts/prepare-renderer-stress-fixtures.tsapps/desktop/scripts/stress-renderer.tsapps/desktop/src/main/index.tsapps/desktop/src/renderer/hooks/host-service/useDiffStats/useDiffStats.tsapps/desktop/src/renderer/hooks/host-service/useGitStatus/useGitStatus.tsapps/desktop/src/renderer/index.tsxapps/desktop/src/renderer/lib/terminal/terminal-addons.tsapps/desktop/src/renderer/lib/terminal/terminal-image-addon-controller.test.tsapps/desktop/src/renderer/lib/terminal/terminal-image-addon-controller.tsapps/desktop/src/renderer/lib/terminal/terminal-runtime-registry.tsapps/desktop/src/renderer/lib/terminal/terminal-runtime.tsapps/desktop/src/renderer/lib/terminal/terminal-webgl-addon-controller.test.tsapps/desktop/src/renderer/lib/terminal/terminal-webgl-addon-controller.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/DashboardSidebar.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarHoverCardOverlay/DashboardSidebarHoverCardOverlay.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/DashboardSidebarProjectSection.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/components/DashboardSidebarCollapsedProjectContent/DashboardSidebarCollapsedProjectContent.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/components/DashboardSidebarExpandedProjectContent/DashboardSidebarExpandedProjectContent.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/DashboardSidebarWorkspaceItem.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/hooks/useDashboardSidebarWorkspaceItemActions/useDashboardSidebarWorkspaceItemActions.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/SortableWorkspaceItem/SortableWorkspaceItem.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarShortcuts/useDashboardSidebarShortcuts.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/hooks/useHybridSearch/useHybridSearch.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/hooks/useTasksData/useTasksData.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/utils/workspace-navigation.test.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/utils/workspace-navigation.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/WorkspaceSidebar.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/useChangesTab.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/useReviewTab.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useChangeset/useChangeset.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useOpenInExternalEditor/useOpenInExternalEditor.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useRendererStressWorkspaceBridge/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useRendererStressWorkspaceBridge/useRendererStressWorkspaceBridge.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useSidebarDiffRef/useSidebarDiffRef.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/layout.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspaces/components/V2WorkspacesList/V2WorkspacesList.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspaces/components/V2WorkspacesList/components/V2WorkspaceRow/V2WorkspaceRow.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/v1-terminal-cache.tsapps/desktop/src/renderer/stores/v2-workspace-navigation.tsapps/desktop/src/shared/workspace-run-definition.test.tsapps/desktop/src/shared/workspace-run-definition.tspackages/host-service/src/app.tspackages/host-service/src/runtime/pull-requests/pull-requests.test.tspackages/host-service/src/runtime/pull-requests/pull-requests.tspackages/host-service/src/trpc/router/git/git.tspackages/host-service/src/trpc/router/git/utils/git-helpers.test.tspackages/host-service/src/trpc/router/git/utils/git-helpers.tspackages/host-service/src/trpc/router/workspace-creation/shared/git-config.tspackages/host-service/src/trpc/router/workspaces/workspaces.tspackages/host-service/test/integration/bug-hunt-3.integration.test.tspackages/host-service/test/integration/git.integration.test.tspackages/panes/src/react/components/Workspace/Workspace.tsxpackages/shared/src/host-info.ts
Greptile SummaryThis PR hardens the desktop renderer and host-service against degradation during heavy usage and rapid workspace switching, addressing WebGL/WASM pressure from parked terminals, slow host-identity shell calls, and racing git-status syncs.
Confidence Score: 3/5Safe to merge with one functional edge case in the WebGL controller worth resolving first. The WebGL controller's fallbackToDom uses an uncancellable setTimeout, so if a genuine GPU context-loss fires in the same event-loop turn as a terminal park (workspace switch), the global suggestedRendererType is permanently set to dom for the session — exactly the kind of heavy-switching scenario this PR optimises for. The rest of the changes are well-structured with good test coverage. terminal-webgl-addon-controller.ts needs attention for the uncancelled fallback timeout; git-helpers.ts has a minor oldPath type inconsistency in parseNumstatRecords.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/lib/terminal/terminal-webgl-addon-controller.ts | New WebGL addon controller with per-terminal enable/disable lifecycle; a deferred fallbackToDom setTimeout is not cancelled by disable(), risking permanent session-wide DOM fallback on rapid workspace parking during a context loss. |
| packages/host-service/src/trpc/router/git/utils/git-helpers.ts | Extracts parseNumstatRecords from parseNumstat to avoid double-counting renames; pushes oldPath as empty string instead of omitting it when the git entry has no old path. |
| packages/host-service/src/runtime/pull-requests/pull-requests.ts | Adds stopped flag, backgroundTasks tracking, and a drainInFlightWork drain loop to stop(); prevents new work from starting post-stop and awaits in-flight tasks up to 10 passes before clearing caches. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/utils/workspace-navigation.ts | Adds a serialised navigation queue for plain workspace switches to prevent route-transition races; module-level queue variables are not reset between test runs, making tests fragile if any promise fails to settle. |
| packages/host-service/src/trpc/router/git/git.ts | Adds getDiffStats endpoint that aggregates against-base, staged, unstaged, and untracked counts server-side using parseNumstatRecords, removing the duplicate computation from the client hook. |
| apps/desktop/src/renderer/lib/terminal/terminal-runtime.ts | Wires enable/disable hooks for image and WebGL addons into attach/detach lifecycle; defers terminal.dispose() two rAF frames to avoid xterm viewport sync races on unmount. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/layout.tsx | Guards relaxed so cached workspace data renders during loading; adds a React key on the provider so pane state resets fully on each workspace switch. |
| packages/shared/src/host-info.ts | Adds a 1500 ms timeout to the ioreg and reg.exe shell calls used to read the machine ID, preventing host-identity lookup from blocking the renderer during workspace switches. |
| apps/desktop/src/renderer/stores/v2-workspace-navigation.ts | New Zustand store for tracking the pending workspace ID during navigation, used to optimistically highlight the destination workspace in the sidebar before the route settles. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[navigateToV2Workspace called] --> B{in-flight navigation?}
B -- No --> C[startV2WorkspaceNavigation]
B -- Yes, plain switch --> D[queueV2WorkspaceNavigation - coalesce]
B -- Yes, replace or search params --> E[cancelQueuedNavigation + startV2WorkspaceNavigation]
C --> F[setPendingWorkspaceId in store]
D --> F
E --> F
F --> G[Sidebar highlights destination immediately]
C --> H[Router transition begins]
D --> I[wait for in-flight to settle, then run last queued target]
I --> H
E --> H
H --> J[WorkspaceProvider remounts with new key]
J --> K[clearPendingWorkspaceId]
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
apps/desktop/src/renderer/lib/terminal/terminal-webgl-addon-controller.ts:65-72
**`disable()` does not cancel the deferred `fallbackToDom` timeout**
`fallbackToDom` schedules `setTimeout(() => disposeAddon({ markDomFallback: true }), 0)` and guards against double-scheduling via `fallbackScheduled`. However `disable()` resets `fallbackScheduled = false` without cancelling the already-queued `setTimeout`. As a result, if a genuine WebGL context-loss fires and then `disable()` is called within the same event-loop turn (as happens during the rapid workspace parking this PR optimises for), the `setTimeout` still fires and calls `disposeAddon({ markDomFallback: true })` — permanently setting the module-level `suggestedRendererType = "dom"` and preventing all subsequent terminals in the session from using WebGL, even though the `disable()` signal was meant only to park the terminal, not to declare a permanent GPU failure.
Storing the `setTimeout` handle and clearing it in `disable()` / `dispose()` would prevent the leak.
### Issue 2 of 3
packages/host-service/src/trpc/router/git/utils/git-helpers.ts:115-118
`result.push({ path: newPath, oldPath, ...stats })` unconditionally includes `oldPath` even when it is the empty-string fallback from `?? ""`. The `NumstatRecord` interface declares `oldPath` as optional (`oldPath?: string`), so an empty string is semantically wrong and any consumer that checks `record.oldPath !== undefined` instead of truthiness would incorrectly treat the record as a rename. Guard with a truthy check before including the field.
```suggestion
if (pathMaybe === "") {
const oldPath = entries[++i] ?? "";
const newPath = entries[++i] ?? "";
if (newPath)
result.push({
path: newPath,
...(oldPath ? { oldPath } : {}),
...stats,
});
```
### Issue 3 of 3
apps/desktop/src/renderer/routes/_authenticated/_dashboard/utils/workspace-navigation.ts:31-34
**Module-level navigation state is not reset between tests**
`inFlightV2WorkspaceNavigation` and `queuedV2WorkspaceNavigation` live at module scope. The new test suite resets the Zustand store in `beforeEach` but has no mechanism to reset these two variables. If any test assertion fails before all navigation promises have settled, the stale `inFlightV2WorkspaceNavigation` will cause the first plain-switch in the next test to be queued rather than started immediately, masking the bug under test. Exporting a `_resetNavigationStateForTesting()` helper (or using a factory function) would make the tests hermetic.
Reviews (1): Last reviewed commit: "Simplify terminal WebGL fallback" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+65
to
+72
| const fallbackToDom = () => { | ||
| if (fallbackScheduled) return; | ||
| fallbackScheduled = true; | ||
| console.info("[terminal:webgl] context lost; falling back to DOM renderer"); | ||
| setTimeout(() => { | ||
| disposeAddon({ markDomFallback: true }); | ||
| }, 0); | ||
| }; |
There was a problem hiding this comment.
disable() does not cancel the deferred fallbackToDom timeout
fallbackToDom schedules setTimeout(() => disposeAddon({ markDomFallback: true }), 0) and guards against double-scheduling via fallbackScheduled. However disable() resets fallbackScheduled = false without cancelling the already-queued setTimeout. As a result, if a genuine WebGL context-loss fires and then disable() is called within the same event-loop turn (as happens during the rapid workspace parking this PR optimises for), the setTimeout still fires and calls disposeAddon({ markDomFallback: true }) — permanently setting the module-level suggestedRendererType = "dom" and preventing all subsequent terminals in the session from using WebGL, even though the disable() signal was meant only to park the terminal, not to declare a permanent GPU failure.
Storing the setTimeout handle and clearing it in disable() / dispose() would prevent the leak.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/lib/terminal/terminal-webgl-addon-controller.ts
Line: 65-72
Comment:
**`disable()` does not cancel the deferred `fallbackToDom` timeout**
`fallbackToDom` schedules `setTimeout(() => disposeAddon({ markDomFallback: true }), 0)` and guards against double-scheduling via `fallbackScheduled`. However `disable()` resets `fallbackScheduled = false` without cancelling the already-queued `setTimeout`. As a result, if a genuine WebGL context-loss fires and then `disable()` is called within the same event-loop turn (as happens during the rapid workspace parking this PR optimises for), the `setTimeout` still fires and calls `disposeAddon({ markDomFallback: true })` — permanently setting the module-level `suggestedRendererType = "dom"` and preventing all subsequent terminals in the session from using WebGL, even though the `disable()` signal was meant only to park the terminal, not to declare a permanent GPU failure.
Storing the `setTimeout` handle and clearing it in `disable()` / `dispose()` would prevent the leak.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+115
to
+118
| if (pathMaybe === "") { | ||
| const oldPath = entries[++i] ?? ""; | ||
| const newPath = entries[++i] ?? ""; | ||
| if (newPath) result.set(newPath, stats); | ||
| if (oldPath) result.set(oldPath, stats); | ||
| if (newPath) result.push({ path: newPath, oldPath, ...stats }); |
There was a problem hiding this comment.
result.push({ path: newPath, oldPath, ...stats }) unconditionally includes oldPath even when it is the empty-string fallback from ?? "". The NumstatRecord interface declares oldPath as optional (oldPath?: string), so an empty string is semantically wrong and any consumer that checks record.oldPath !== undefined instead of truthiness would incorrectly treat the record as a rename. Guard with a truthy check before including the field.
Suggested change
| if (pathMaybe === "") { | |
| const oldPath = entries[++i] ?? ""; | |
| const newPath = entries[++i] ?? ""; | |
| if (newPath) result.set(newPath, stats); | |
| if (oldPath) result.set(oldPath, stats); | |
| if (newPath) result.push({ path: newPath, oldPath, ...stats }); | |
| if (pathMaybe === "") { | |
| const oldPath = entries[++i] ?? ""; | |
| const newPath = entries[++i] ?? ""; | |
| if (newPath) | |
| result.push({ | |
| path: newPath, | |
| ...(oldPath ? { oldPath } : {}), | |
| ...stats, | |
| }); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/host-service/src/trpc/router/git/utils/git-helpers.ts
Line: 115-118
Comment:
`result.push({ path: newPath, oldPath, ...stats })` unconditionally includes `oldPath` even when it is the empty-string fallback from `?? ""`. The `NumstatRecord` interface declares `oldPath` as optional (`oldPath?: string`), so an empty string is semantically wrong and any consumer that checks `record.oldPath !== undefined` instead of truthiness would incorrectly treat the record as a rename. Guard with a truthy check before including the field.
```suggestion
if (pathMaybe === "") {
const oldPath = entries[++i] ?? "";
const newPath = entries[++i] ?? "";
if (newPath)
result.push({
path: newPath,
...(oldPath ? { oldPath } : {}),
...stats,
});
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+31
to
+34
| interface QueuedV2WorkspaceNavigation extends V2WorkspaceNavigationRequest { | ||
| waiters: Array<{ | ||
| resolve: () => void; | ||
| reject: (error: unknown) => void; |
There was a problem hiding this comment.
inFlightV2WorkspaceNavigation and queuedV2WorkspaceNavigation live at module scope. The new test suite resets the Zustand store in beforeEach but has no mechanism to reset these two variables. If any test assertion fails before all navigation promises have settled, the stale inFlightV2WorkspaceNavigation will cause the first plain-switch in the next test to be queued rather than started immediately, masking the bug under test. Exporting a _resetNavigationStateForTesting() helper (or using a factory function) would make the tests hermetic.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/utils/workspace-navigation.ts
Line: 31-34
Comment:
**Module-level navigation state is not reset between tests**
`inFlightV2WorkspaceNavigation` and `queuedV2WorkspaceNavigation` live at module scope. The new test suite resets the Zustand store in `beforeEach` but has no mechanism to reset these two variables. If any test assertion fails before all navigation promises have settled, the stale `inFlightV2WorkspaceNavigation` will cause the first plain-switch in the next test to be queued rather than started immediately, masking the bug under test. Exporting a `_resetNavigationStateForTesting()` helper (or using a factory function) would make the tests hermetic.
How can I resolve this? If you propose a fix, please make it concise.# Conflicts: # apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx
# Conflicts: # apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/hooks/useDashboardSidebarWorkspaceItemActions/useDashboardSidebarWorkspaceItemActions.ts
Kitenite
added a commit
that referenced
this pull request
May 14, 2026
This was referenced May 14, 2026
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
This PR hardens the desktop renderer and host-service paths that degraded during heavy workspace usage and rapid workspace switching.
Root Cause
Rapid workspace switching was forcing too much synchronous renderer work while retaining many heavy panes. Terminal WebGL and image addons could also retain GPU/WASM pressure across parked runtimes, and xterm's built-in WebGL context-loss notification waits several seconds before falling back, which left terminals visually blank during context loss.
Validation
bun run lintbun run typecheckbun --cwd apps/desktop test src/renderer/lib/terminal/terminal-image-addon-controller.test.ts src/renderer/lib/terminal/terminal-webgl-addon-controller.test.tsbun --cwd apps/desktop stress:renderer -- --port 9333 --scenario terminal-heavy --iterations 120 --terminal-iterations 120 --terminal-tab-count 24 --terminal-panes-per-tab 4 --terminal-lines 40 --terminal-payload-bytes 1024 --progress-every 20 --timeout-ms 240000 --max-heartbeat-delay-ms 1500 --max-long-task-ms 2000 --settle-ms 1000Summary by cubic
Fixes renderer slowdowns and freezes under heavy workspace switching and terminal pressure by adding stress tooling, browser layout scheduling, tab/changes windowing, and tighter terminal/host-service lifecycles. Adds CDP profiling hooks and disables session recording to keep the UI responsive.
Performance and Stability
WorkspaceProviderper workspace to isolate pane state (tests added).TerminalPane, tab items, and sortable items; windowed TabBar (visible range + overscan); virtualized “Changes” list (folder grouping) and cheaper overflow‑fade measuring; system font discovery batched on idle.git.getDiffStats; gateuseGitStatus, changeset, review, task search, external‑editor lookup, and diff refs by active tab/search..git/configwrites viagitConfigWrite;PullRequestRuntimeManager.stop()awaits background tasks; close filesystem on shutdown; bounded machine‑id timeouts; newgit.getDiffStatsfor sidebar totals; concurrentworkspace.destroyand create/delete churn leave no duplicates or stale worktrees (tests).New Features
stress:rendererandstress:renderer:fixtureswith route‑sweep, terminal‑heavy, workspace‑switch, and heavy workspace scenarios; CDP control viaSUPERSET_RENDERER_STRESS_CDP_PORT; CPU profiling and React render probes; dev‑onlywindow.__SUPERSET_RENDERER_STRESS_NAVIGATE__.Written for commit 8a6efae. Summary will update on new commits.
Summary by CodeRabbit
New Features
Performance Improvements
Bug Fixes
Chores
Tests