Skip to content

Reapply renderer stress fixes without terminal sizing regression#4541

Merged
Kitenite merged 5 commits into
mainfrom
accurate-tuesday
May 14, 2026
Merged

Reapply renderer stress fixes without terminal sizing regression#4541
Kitenite merged 5 commits into
mainfrom
accurate-tuesday

Conversation

@Kitenite
Copy link
Copy Markdown
Collaborator

@Kitenite Kitenite commented May 14, 2026

Summary

  • reapply renderer stress/performance improvements after [codex] Fix renderer stress degradation #4500 was reverted
  • keep terminal attach/detach sizing behavior intact and suppress duplicate terminal replay after buffered content exists
  • add terminal WebGL context-loss fallback without toggling WebGL on pane switches

Verification

  • bun test apps/desktop/src/renderer/lib/terminal/terminal-webgl-addon-controller.test.ts apps/desktop/src/renderer/lib/terminal/terminal-runtime-registry.test.ts apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.test.ts
  • bunx biome check apps/desktop/src/renderer/lib/terminal/terminal-webgl-canvas-registry.ts apps/desktop/src/renderer/lib/terminal/terminal-webgl-addon-controller.ts apps/desktop/src/renderer/lib/terminal/terminal-webgl-addon-controller.test.ts apps/desktop/src/renderer/lib/terminal/terminal-addons.ts apps/desktop/src/renderer/lib/terminal/terminal-runtime-registry.ts apps/desktop/src/renderer/lib/terminal/terminal-runtime-registry.test.ts apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts
  • bun --cwd apps/desktop typecheck

Notes

  • no pty-daemon, terminal-host attach/update, or host-service terminal daemon changes

Summary by cubic

Reapplies renderer performance work with stress tooling and hardened terminal behavior, without reintroducing the terminal sizing regression. Also coalesces workspace navigation, adds fast diff totals, and documents the renderer stress QA flow.

  • New Features

    • Dev stress tools: stress:renderer, stress:renderer:fixtures, CDP via SUPERSET_RENDERER_STRESS_CDP_PORT, in‑app bridge, and dev‑only window.__SUPERSET_RENDERER_STRESS_NAVIGATE__.
    • Fast diff totals via new git.getDiffStats and updated useDiffStats.
    • UI perf: windowed Tab Bar, virtualized grouped Changes list, coalesced browser layout with frame budgets, and lighter overflow fade re‑measure.
    • Docs: added RENDERER_STRESS_QA.md and linked from the V2 launch test plan.
  • Bug Fixes

    • Terminal: robust WebGL context‑loss fallback without toggling on pane switches; prevent duplicate server replay once output exists; preserve attach/detach sizing; focus runtime on activation; block session replay and canvas capture for terminal surfaces; bound image addon memory (disable sixel, per‑terminal limits); defer and chunk background output writes. Tests added.
    • Workspace switching: queue/coalesce v2 navigation, remount the workspace provider on switch, and indicate pending workspace in lists. Tests added.
    • Host robustness: retry .git/config writes, coalesce ensure‑main‑workspace, wait for PR runtime background tasks on stop, and close the filesystem on shutdown. Integration tests added.
    • Less background work: gate queries and searches by active tab, batch/idle font discovery, and use ResizeObserver entries for split orientation.

Written for commit 7750eb0. Summary will update on new commits.

Summary by CodeRabbit

  • New Features

    • Renderer stress tools with an in-app bridge for scripted stress scenarios
    • Pending-state V2 workspace navigation to surface in-flight switches
    • Server-side diff-stats endpoint for accurate additions/deletions totals
  • Performance

    • Tab bar windowing/virtualization and deferred layout scheduling
    • Terminal output buffering and non-blocking system font discovery
  • Bug Fixes

    • Improved WebGL fallback/session-replay blocking and terminal cleanup
    • Safer concurrent workspace create/destroy flows
  • Tests

    • New suites for renderer stress, terminal WebGL, git diffs, and navigation
  • Documentation

    • Added renderer stress QA guide

Review Change Stack

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 14, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 21f900f9-3796-4fc2-8cea-77b053d7a174

📥 Commits

Reviewing files that changed from the base of the PR and between aa6cbd2 and 7750eb0.

📒 Files selected for processing (7)
  • apps/desktop/docs/RENDERER_STRESS_QA.md
  • apps/desktop/docs/V2_LAUNCH_TEST_PLAN.md
  • apps/desktop/src/renderer/lib/terminal/terminal-addons.ts
  • apps/desktop/src/renderer/lib/terminal/terminal-webgl-addon-controller.ts
  • apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.test.ts
  • apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useRendererStressWorkspaceBridge/useRendererStressWorkspaceBridge.ts
✅ Files skipped from review due to trivial changes (1)
  • apps/desktop/docs/V2_LAUNCH_TEST_PLAN.md
🚧 Files skipped from review as they are similar to previous changes (5)
  • apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.test.ts
  • apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts
  • apps/desktop/src/renderer/lib/terminal/terminal-webgl-addon-controller.ts
  • apps/desktop/src/renderer/lib/terminal/terminal-addons.ts
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useRendererStressWorkspaceBridge/useRendererStressWorkspaceBridge.ts

📝 Walkthrough

<review_stack_artifact>

</review_stack_artifact>

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch accurate-tuesday

@capy-ai
Copy link
Copy Markdown

capy-ai Bot commented May 14, 2026

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.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 14, 2026

🧹 Preview Cleanup Complete

The following preview resources have been cleaned up:

  • ⚠️ Neon database branch

Thank you for your contribution! 🎉

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (11)
apps/desktop/src/renderer/lib/posthog.ts (1)

33-44: 💤 Low value

Consider adding a clarifying comment about the session recording configuration.

The config sets disable_session_recording: true (line 33) but also provides detailed session_recording configuration (lines 37-44). While this appears intentional—likely preparatory config for when session recording is enabled—a brief comment explaining why session_recording is configured despite recording being disabled would improve code clarity and prevent future confusion.

📝 Suggested clarification
 		disable_scroll_properties: true,
 		disable_session_recording: true,
 		person_profiles: "identified_only",
 		persistence: "localStorage",
 		debug: false,
+		// Configure session recording settings even when disabled to ensure
+		// proper blocking/canvas config is applied if recording gets enabled
 		session_recording: {
 			blockSelector: POSTHOG_SESSION_REPLAY_BLOCK_SELECTOR,
🤖 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/posthog.ts` around lines 33 - 44, Add a brief
inline comment explaining why session_recording options are present even though
disable_session_recording is true: clarify that disable_session_recording: true
currently disables replay capture but the nested session_recording object
(including POSTHOG_SESSION_REPLAY_BLOCK_SELECTOR, captureCanvas settings, etc.)
is preconfigured for future enablement or runtime toggling; place this comment
near the disable_session_recording and session_recording entries in the same
config block so readers understand the intentional setup and avoid confusion.
packages/host-service/src/runtime/pull-requests/pull-requests.ts (1)

355-368: 💤 Low value

Consider documenting the 10-pass drain limit.

The drainInFlightWork method uses a hardcoded loop limit of 10 passes to bound the drain operation. While this prevents infinite loops, the rationale for choosing 10 is not documented. Consider adding a comment explaining:

  • Why 10 passes is sufficient for normal shutdown
  • What happens if tasks are still in-flight after 10 passes (they're abandoned)
  • Whether this is a concern for production scenarios
📝 Optional: Add explanatory comment
 	private async drainInFlightWork(): Promise<void> {
+		// Bounded drain: up to 10 passes to prevent infinite loops from tasks
+		// that spawn new tasks. In practice, 2-3 passes suffice for normal
+		// shutdown; remaining in-flight work is abandoned after the limit.
 		for (let pass = 0; pass < 10; pass++) {
🤖 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/src/runtime/pull-requests/pull-requests.ts` around
lines 355 - 368, The drainInFlightWork method uses a hardcoded 10-pass loop to
wait for backgroundTasks, workspaceSyncState.running, and inFlightProjects to
settle but lacks explanation; add a concise comment above drainInFlightWork
explaining why 10 passes was chosen (e.g., expected max retries/duration under
normal shutdown), state that after 10 passes any remaining tasks are
intentionally abandoned (describe consequences), and note whether this is
acceptable for production or should be revisited—refer to drainInFlightWork and
the collections backgroundTasks, workspaceSyncState (state.running), and
inFlightProjects so reviewers can find and understand the limit and its
trade-offs.
apps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/FontSettingSection/hooks/useSystemFonts/useSystemFonts.ts (1)

169-187: ⚡ Quick win

Yield more reliably during queryLocalFonts iteration.

checked is only bumped after the dedup continue, but queryLocalFonts() typically returns one FontData entry per face/style (Regular, Bold, Italic, …) for each family. With heavy duplicate runs, the loop can churn through hundreds of skipped entries between yields and still occupy the main thread for a noticeable stretch. Moving the counter increment ahead of continue keeps the batched-yield cadence honest regardless of dedup ratio.

♻️ Proposed fix
 				let checked = 0;
 				for (const fd of fontData) {
+					checked += 1;
+					if (checked % FONT_DISCOVERY_BATCH_SIZE === 0) {
+						await yieldFontDiscoveryWork();
+					}
 					if (seen.has(fd.family)) continue;
 					seen.add(fd.family);
 
 					result.push({ family: fd.family, category: classifyFont(fd.family) });
-
-					checked += 1;
-					if (checked % FONT_DISCOVERY_BATCH_SIZE === 0) {
-						await yieldFontDiscoveryWork();
-					}
 				}
🤖 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/settings/appearance/components/AppearanceSettings/components/FontSettingSection/hooks/useSystemFonts/useSystemFonts.ts`
around lines 169 - 187, The loop over window.queryLocalFonts can skip many
entries due to the seen.has(fd.family) continue, which prevents checked from
advancing and delays yieldFontDiscoveryWork; update the loop in useSystemFonts
so you increment the checked counter and run the batch-yield check (using
FONT_DISCOVERY_BATCH_SIZE and await yieldFontDiscoveryWork()) before the
duplicate check (seen.has(fd.family)), then continue if duplicated, otherwise
add to result (result.push({ family: fd.family, category:
classifyFont(fd.family) })) and mark seen.add(fd.family); this preserves
batching cadence even when many faces/styles share the same family.
packages/panes/src/react/components/Workspace/components/TabBar/TabBar.tsx (1)

212-214: 💤 Low value

Consider removing potentially redundant effect.

This useLayoutEffect calls readScrollMetrics on every change to the callback reference. Since readScrollMetrics is a stable callback with no dependencies, this effectively runs only on mount. The ResizeObserver effect above (lines 199-210) already triggers updateOverflow on mount via observeResizeTargets, which schedules a measurement. This second effect may cause an extra immediate measurement.

🤖 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/panes/src/react/components/Workspace/components/TabBar/TabBar.tsx`
around lines 212 - 214, The useLayoutEffect that calls readScrollMetrics on
[readScrollMetrics] appears redundant because readScrollMetrics is stable and
the ResizeObserver setup (observeResizeTargets -> updateOverflow) already
schedules an initial measurement on mount; remove the useLayoutEffect block that
invokes readScrollMetrics to avoid an extra immediate measurement, leaving the
ResizeObserver/observeResizeTargets/updateOverflow flow to handle the initial
measurement.
packages/panes/src/react/components/Workspace/components/TabBar/components/TabItem/TabItem.tsx (1)

218-234: 💤 Low value

Verify memo equality function covers all memoization-safe props.

The custom equality function excludes icon, accessory, and event handlers from the comparison. This assumes:

  1. Event handlers (onSelect, onClose, etc.) are either stable or acceptable to use when stale
  2. icon and accessory are pure functions of tab, so when tab is referentially equal, these will produce the same output

This optimization is valid if renderTabIcon and renderTabAccessory (passed from the parent) are pure functions of the tab. If they depend on other external state, stale renders could occur.

🤖 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/panes/src/react/components/Workspace/components/TabBar/components/TabItem/TabItem.tsx`
around lines 218 - 234, The memo equality function areTabItemPropsEqual
currently compares only tab, tabs, registry, index, and isActive but omits props
that can change rendering (icon, accessory and event handlers); update
areTabItemPropsEqual to also compare previous.icon === next.icon and
previous.accessory === next.accessory and equality of any event handler props
present on TabItemProps (e.g., previous.onSelect === next.onSelect,
previous.onClose === next.onClose, previous.onContextMenu ===
next.onContextMenu, previous.onDragStart === next.onDragStart) so memoization is
safe unless the parent guarantees those functions/renderer props are stable, or
alternatively document that renderTabIcon/renderTabAccessory and handlers must
be stable and keep current comparator. Ensure you modify the
areTabItemPropsEqual function referenced by TabItem and keep types consistent
with TabItemProps.
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useRendererStressWorkspaceBridge/useRendererStressWorkspaceBridge.ts (1)

506-510: 💤 Low value

closeOldestTab removes only one tab regardless of overflow.

The helper only ever calls removeTab on tabs[0] and exits, even when tabs.length > keepCount + 1. The naming and the keepCount parameter suggest it would prune down to keepCount tabs. The caller in stress-renderer.ts invokes this per-iteration, so it converges over time, but a single call after a replaceWithGeneratedLayout(80, ...) won't approach keepCount for many iterations.

If the per-iteration "one tab" behavior is intentional, consider renaming to closeOneOldTab to match. Otherwise loop until state.tabs.length <= keepCount.

♻️ Suggested loop fix (if intent is to prune to keepCount)
 			closeOldestTab: (keepCount = 4) => {
 				const state = store.getState();
-				if (state.tabs.length <= keepCount) return;
-				state.removeTab(state.tabs[0].id);
+				while (store.getState().tabs.length > keepCount) {
+					const tabs = store.getState().tabs;
+					if (tabs.length === 0) break;
+					store.getState().removeTab(tabs[0].id);
+				}
 			},
🤖 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/useRendererStressWorkspaceBridge/useRendererStressWorkspaceBridge.ts
around lines 506 - 510, The closeOldestTab helper only removes a single tab even
when tabs.length > keepCount; update closeOldestTab so it repeatedly removes the
oldest tab until state.tabs.length <= keepCount (e.g., call store.getState() and
loop while state.tabs.length > keepCount, invoking
state.removeTab(state.tabs[0].id) each iteration) so it prunes down to the
requested keepCount; references: closeOldestTab, store.getState,
state.removeTab, state.tabs.
apps/desktop/src/main/lib/extensions/index.ts (1)

170-173: 💤 Low value

Worth a brief comment explaining the CDP-stress gating.

The early return when SUPERSET_RENDERER_STRESS_CDP_PORT is set is non-obvious: it ties this module to the stress-renderer harness contract (which relies on its own __REACT_DEVTOOLS_GLOBAL_HOOK__ probe via --react-probe and prefers no extension overhead). A one-line comment will save the next reader a grep.

📝 Suggested annotation
 	if (env.NODE_ENV !== "development") return;
+	// Skip during renderer stress runs to avoid extension overhead/noise;
+	// stress harness uses its own React DevTools hook probe when available.
 	if (process.env.SUPERSET_RENDERER_STRESS_CDP_PORT) return;
🤖 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/main/lib/extensions/index.ts` around lines 170 - 173, Add a
one-line comment above the early return in loadReactDevToolsExtension explaining
that the function skips installing the React DevTools when
process.env.SUPERSET_RENDERER_STRESS_CDP_PORT is set because the stress-renderer
harness uses its own __REACT_DEVTOOLS_GLOBAL_HOOK__ probe (invoked via
--react-probe) and expects no extension overhead; reference env.NODE_ENV and
SUPERSET_RENDERER_STRESS_CDP_PORT in the comment so future readers can grep for
the gating contract.
apps/desktop/scripts/prepare-renderer-stress-fixtures.ts (1)

169-185: 💤 Low value

Avoid shadowing the global process inside run().

Inside this helper, the local process (Bun subprocess) shadows the Node process global. While currently harmless because the body doesn't reference the global, future edits adding things like process.exit or process.env inside this function will silently target the spawned child handle. Renaming the local (e.g., child or proc) eliminates the foot-gun.

♻️ Suggested rename
-	const process = Bun.spawn([command, ...args], {
+	const child = Bun.spawn([command, ...args], {
 		cwd: options.cwd,
 		stdout: "pipe",
 		stderr: "pipe",
 	});
 	const [stdout, stderr, code] = await Promise.all([
-		new Response(process.stdout).text(),
-		new Response(process.stderr).text(),
-		process.exited,
+		new Response(child.stdout).text(),
+		new Response(child.stderr).text(),
+		child.exited,
 	]);
🤖 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/scripts/prepare-renderer-stress-fixtures.ts` around lines 169 -
185, The local Bun subprocess variable named process inside the run() helper
shadows the Node global; rename that local (e.g., to child or proc) and update
all usages (new Response(process.stdout), new Response(process.stderr),
process.exited and any other references) to the new name so the helper no longer
masks the global process while preserving the same behavior and error handling.
apps/desktop/scripts/stress-renderer.ts (1)

502-516: 💤 Low value

Redundant id guard in onMessage.

When message.method is a string, the function already returns at Line 502 if message.id is not a number. The duplicate check at Line 504 only triggers when message.method is not a string (pure response), so the inner one is dead in practice. Consider hoisting the check once after the event-dispatch block to clarify control flow.

♻️ Suggested simplification
 	private onMessage(raw: string): void {
 		const message = JSON.parse(raw) as CdpResponse;
 		if (typeof message.method === "string") {
 			const listeners = this.listeners.get(message.method);
 			if (listeners) {
 				for (const listener of listeners) {
 					listener(message.params);
 				}
 			}
-			if (typeof message.id !== "number") return;
 		}
 		if (typeof message.id !== "number") return;
🤖 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/scripts/stress-renderer.ts` around lines 502 - 516, The
onMessage handler contains a redundant guard for message.id causing dead code;
remove the duplicate check and hoist a single verify step after the
event-dispatch branch so both notifications and responses follow the same flow.
Specifically, in the onMessage function: keep the initial branch that handles
message.method (the event dispatch path), then after that branch run one unified
check of typeof message.id === "number" before accessing this.pending,
pending.timer, pending.reject/resolve; reference the existing symbols onMessage,
message.id, this.pending, pending.timer, pending.reject and pending.resolve when
updating the logic.
apps/desktop/src/renderer/lib/terminal/terminal-session-replay.ts (1)

5-17: 💤 Low value

Optional: The defensive optional chaining may be unnecessary.

Standard Element objects always have classList and setAttribute. The optional chaining here (lines 14-16) is overly defensive unless you're handling non-standard DOM objects or mocking scenarios. Consider simplifying to direct calls if all callers guarantee standard Elements.

♻️ Simplified version
-type ReplayBlockedElement = Element & {
-	classList?: {
-		add?: (...tokens: string[]) => void;
-	};
-	setAttribute?: (qualifiedName: string, value: string) => void;
-};
-
 export function markTerminalSessionReplayBlocked(element: Element): void {
-	const target = element as ReplayBlockedElement;
-	target.classList?.add?.(TERMINAL_SESSION_REPLAY_BLOCK_CLASS);
-	target.setAttribute?.("data-ph-no-capture", "true");
-	target.setAttribute?.(TERMINAL_SESSION_REPLAY_BLOCK_ATTRIBUTE, "true");
+	element.classList.add(TERMINAL_SESSION_REPLAY_BLOCK_CLASS);
+	element.setAttribute("data-ph-no-capture", "true");
+	element.setAttribute(TERMINAL_SESSION_REPLAY_BLOCK_ATTRIBUTE, "true");
 }
🤖 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-session-replay.ts` around
lines 5 - 17, The current markTerminalSessionReplayBlocked function uses overly
defensive optional chaining on DOM APIs; since standard Element always exposes
classList and setAttribute, simplify by removing the ReplayBlockedElement
wrapper and the optional chaining and call
element.classList.add(TERMINAL_SESSION_REPLAY_BLOCK_CLASS) and
element.setAttribute("data-ph-no-capture","true") and
element.setAttribute(TERMINAL_SESSION_REPLAY_BLOCK_ATTRIBUTE,"true") directly
inside markTerminalSessionReplayBlocked to make behavior clearer and eliminate
unnecessary types/guards.
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/components/ChangesFoldersView/ChangesFoldersView.tsx (1)

103-114: 💤 Low value

Consider making scrollMargin reactive to layout changes.

The scrollMargin is computed once during virtualizer creation using listRef.current?.offsetTop ?? 0. If the offset changes (e.g., when the PRActionHeader height changes dynamically), the scroll position calculations will be incorrect. Consider whether this offset is truly stable, or if the virtualizer should be recreated when layout changes.

🤖 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/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/components/ChangesFoldersView/ChangesFoldersView.tsx
around lines 103 - 114, The virtualizer is created with a one-time scrollMargin
computed from listRef.current?.offsetTop which can change when layout (e.g.,
PRActionHeader) resizes; make scrollMargin reactive by measuring the offset in a
state (via useLayoutEffect or a ResizeObserver) and feed that state into the
useVirtualizer call so the virtualizer is recreated/updated when the offset
changes. Update code around virtualizer, listRef and scrollMargin (and any
PRActionHeader height measurement logic) so the measured offset is stored in a
variable (e.g., scrollMarginState) and passed to useVirtualizer instead of
reading listRef.current inline.
🤖 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/v2-workspace/`$workspaceId/hooks/useRendererStressWorkspaceBridge/useRendererStressWorkspaceBridge.ts:
- Around line 511-525: churnActivePaneData can apply FilePaneData to a terminal
pane because isRendererStressPaneKind(active.pane.kind) returns false for
"terminal", causing nextKind to default to "file" and then calling
state.setPaneData with createStressPane(...).data; update churnActivePaneData to
detect terminal panes (using active.pane.kind === "terminal" or
!isRendererStressPaneKind) and either skip churning for terminals, convert the
pane kind before calling state.setPaneData, or explicitly call
bridge.addTab(...) to open a new file tab; if leaving mismatched-data behavior
is intentional, add a clear comment above churnActivePaneData explaining that
terminals will be converted and why. Ensure you update checks around
isRendererStressPaneKind, the setPaneData call, and any callers that assume the
pane kind remains unchanged (references: churnActivePaneData,
isRendererStressPaneKind, state.setPaneData, createStressPane, bridge.addTab).

---

Nitpick comments:
In `@apps/desktop/scripts/prepare-renderer-stress-fixtures.ts`:
- Around line 169-185: The local Bun subprocess variable named process inside
the run() helper shadows the Node global; rename that local (e.g., to child or
proc) and update all usages (new Response(process.stdout), new
Response(process.stderr), process.exited and any other references) to the new
name so the helper no longer masks the global process while preserving the same
behavior and error handling.

In `@apps/desktop/scripts/stress-renderer.ts`:
- Around line 502-516: The onMessage handler contains a redundant guard for
message.id causing dead code; remove the duplicate check and hoist a single
verify step after the event-dispatch branch so both notifications and responses
follow the same flow. Specifically, in the onMessage function: keep the initial
branch that handles message.method (the event dispatch path), then after that
branch run one unified check of typeof message.id === "number" before accessing
this.pending, pending.timer, pending.reject/resolve; reference the existing
symbols onMessage, message.id, this.pending, pending.timer, pending.reject and
pending.resolve when updating the logic.

In `@apps/desktop/src/main/lib/extensions/index.ts`:
- Around line 170-173: Add a one-line comment above the early return in
loadReactDevToolsExtension explaining that the function skips installing the
React DevTools when process.env.SUPERSET_RENDERER_STRESS_CDP_PORT is set because
the stress-renderer harness uses its own __REACT_DEVTOOLS_GLOBAL_HOOK__ probe
(invoked via --react-probe) and expects no extension overhead; reference
env.NODE_ENV and SUPERSET_RENDERER_STRESS_CDP_PORT in the comment so future
readers can grep for the gating contract.

In `@apps/desktop/src/renderer/lib/posthog.ts`:
- Around line 33-44: Add a brief inline comment explaining why session_recording
options are present even though disable_session_recording is true: clarify that
disable_session_recording: true currently disables replay capture but the nested
session_recording object (including POSTHOG_SESSION_REPLAY_BLOCK_SELECTOR,
captureCanvas settings, etc.) is preconfigured for future enablement or runtime
toggling; place this comment near the disable_session_recording and
session_recording entries in the same config block so readers understand the
intentional setup and avoid confusion.

In `@apps/desktop/src/renderer/lib/terminal/terminal-session-replay.ts`:
- Around line 5-17: The current markTerminalSessionReplayBlocked function uses
overly defensive optional chaining on DOM APIs; since standard Element always
exposes classList and setAttribute, simplify by removing the
ReplayBlockedElement wrapper and the optional chaining and call
element.classList.add(TERMINAL_SESSION_REPLAY_BLOCK_CLASS) and
element.setAttribute("data-ph-no-capture","true") and
element.setAttribute(TERMINAL_SESSION_REPLAY_BLOCK_ATTRIBUTE,"true") directly
inside markTerminalSessionReplayBlocked to make behavior clearer and eliminate
unnecessary types/guards.

In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/components/ChangesFoldersView/ChangesFoldersView.tsx:
- Around line 103-114: The virtualizer is created with a one-time scrollMargin
computed from listRef.current?.offsetTop which can change when layout (e.g.,
PRActionHeader) resizes; make scrollMargin reactive by measuring the offset in a
state (via useLayoutEffect or a ResizeObserver) and feed that state into the
useVirtualizer call so the virtualizer is recreated/updated when the offset
changes. Update code around virtualizer, listRef and scrollMargin (and any
PRActionHeader height measurement logic) so the measured offset is stored in a
variable (e.g., scrollMarginState) and passed to useVirtualizer instead of
reading listRef.current inline.

In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/useRendererStressWorkspaceBridge/useRendererStressWorkspaceBridge.ts:
- Around line 506-510: The closeOldestTab helper only removes a single tab even
when tabs.length > keepCount; update closeOldestTab so it repeatedly removes the
oldest tab until state.tabs.length <= keepCount (e.g., call store.getState() and
loop while state.tabs.length > keepCount, invoking
state.removeTab(state.tabs[0].id) each iteration) so it prunes down to the
requested keepCount; references: closeOldestTab, store.getState,
state.removeTab, state.tabs.

In
`@apps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/FontSettingSection/hooks/useSystemFonts/useSystemFonts.ts`:
- Around line 169-187: The loop over window.queryLocalFonts can skip many
entries due to the seen.has(fd.family) continue, which prevents checked from
advancing and delays yieldFontDiscoveryWork; update the loop in useSystemFonts
so you increment the checked counter and run the batch-yield check (using
FONT_DISCOVERY_BATCH_SIZE and await yieldFontDiscoveryWork()) before the
duplicate check (seen.has(fd.family)), then continue if duplicated, otherwise
add to result (result.push({ family: fd.family, category:
classifyFont(fd.family) })) and mark seen.add(fd.family); this preserves
batching cadence even when many faces/styles share the same family.

In `@packages/host-service/src/runtime/pull-requests/pull-requests.ts`:
- Around line 355-368: The drainInFlightWork method uses a hardcoded 10-pass
loop to wait for backgroundTasks, workspaceSyncState.running, and
inFlightProjects to settle but lacks explanation; add a concise comment above
drainInFlightWork explaining why 10 passes was chosen (e.g., expected max
retries/duration under normal shutdown), state that after 10 passes any
remaining tasks are intentionally abandoned (describe consequences), and note
whether this is acceptable for production or should be revisited—refer to
drainInFlightWork and the collections backgroundTasks, workspaceSyncState
(state.running), and inFlightProjects so reviewers can find and understand the
limit and its trade-offs.

In
`@packages/panes/src/react/components/Workspace/components/TabBar/components/TabItem/TabItem.tsx`:
- Around line 218-234: The memo equality function areTabItemPropsEqual currently
compares only tab, tabs, registry, index, and isActive but omits props that can
change rendering (icon, accessory and event handlers); update
areTabItemPropsEqual to also compare previous.icon === next.icon and
previous.accessory === next.accessory and equality of any event handler props
present on TabItemProps (e.g., previous.onSelect === next.onSelect,
previous.onClose === next.onClose, previous.onContextMenu ===
next.onContextMenu, previous.onDragStart === next.onDragStart) so memoization is
safe unless the parent guarantees those functions/renderer props are stable, or
alternatively document that renderTabIcon/renderTabAccessory and handlers must
be stable and keep current comparator. Ensure you modify the
areTabItemPropsEqual function referenced by TabItem and keep types consistent
with TabItemProps.

In `@packages/panes/src/react/components/Workspace/components/TabBar/TabBar.tsx`:
- Around line 212-214: The useLayoutEffect that calls readScrollMetrics on
[readScrollMetrics] appears redundant because readScrollMetrics is stable and
the ResizeObserver setup (observeResizeTargets -> updateOverflow) already
schedules an initial measurement on mount; remove the useLayoutEffect block that
invokes readScrollMetrics to avoid an extra immediate measurement, leaving the
ResizeObserver/observeResizeTargets/updateOverflow flow to handle the initial
measurement.
🪄 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: b5cc673c-fdc7-4116-955d-755014d869da

📥 Commits

Reviewing files that changed from the base of the PR and between 55f31db and de16750.

📒 Files selected for processing (81)
  • apps/desktop/package.json
  • apps/desktop/scripts/prepare-renderer-stress-fixtures.ts
  • apps/desktop/scripts/stress-renderer.ts
  • apps/desktop/src/main/index.ts
  • apps/desktop/src/main/lib/extensions/index.ts
  • apps/desktop/src/renderer/hooks/host-service/useDiffStats/useDiffStats.ts
  • apps/desktop/src/renderer/hooks/host-service/useGitStatus/useGitStatus.ts
  • apps/desktop/src/renderer/index.tsx
  • apps/desktop/src/renderer/lib/posthog.test.ts
  • apps/desktop/src/renderer/lib/posthog.ts
  • apps/desktop/src/renderer/lib/terminal/terminal-addons.ts
  • apps/desktop/src/renderer/lib/terminal/terminal-runtime-registry.test.ts
  • apps/desktop/src/renderer/lib/terminal/terminal-runtime-registry.ts
  • apps/desktop/src/renderer/lib/terminal/terminal-runtime.ts
  • apps/desktop/src/renderer/lib/terminal/terminal-session-replay.ts
  • apps/desktop/src/renderer/lib/terminal/terminal-webgl-addon-controller.test.ts
  • apps/desktop/src/renderer/lib/terminal/terminal-webgl-addon-controller.ts
  • apps/desktop/src/renderer/lib/terminal/terminal-webgl-canvas-registry.ts
  • apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.test.ts
  • apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/DashboardSidebar.tsx
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarHoverCardOverlay/DashboardSidebarHoverCardOverlay.tsx
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/DashboardSidebarProjectSection.tsx
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/components/DashboardSidebarCollapsedProjectContent/DashboardSidebarCollapsedProjectContent.tsx
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/components/DashboardSidebarExpandedProjectContent/DashboardSidebarExpandedProjectContent.tsx
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/DashboardSidebarWorkspaceItem.tsx
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/hooks/useDashboardSidebarWorkspaceItemActions/useDashboardSidebarWorkspaceItemActions.ts
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/SortableWorkspaceItem/SortableWorkspaceItem.tsx
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarShortcuts/useDashboardSidebarShortcuts.ts
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/hooks/useHybridSearch/useHybridSearch.ts
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/hooks/useTasksData/useTasksData.tsx
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/utils/workspace-navigation.test.ts
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/utils/workspace-navigation.ts
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/WorkspaceSidebar.tsx
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/ChangesFileList.tsx
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/components/ChangesFoldersView/ChangesFoldersView.tsx
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/components/ChangesSection/ChangesSection.tsx
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/useChangesTab.tsx
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useReviewTab/useReviewTab.tsx
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useChangeset/useChangeset.ts
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useOpenInExternalEditor/useOpenInExternalEditor.ts
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/browserRuntimeRegistry.ts
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsx
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useRendererStressWorkspaceBridge/index.ts
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useRendererStressWorkspaceBridge/useRendererStressWorkspaceBridge.ts
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useSidebarDiffRef/useSidebarDiffRef.ts
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/layout.tsx
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspaces/components/V2WorkspacesList/V2WorkspacesList.tsx
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspaces/components/V2WorkspacesList/components/V2WorkspaceRow/V2WorkspaceRow.tsx
  • apps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/FontSettingSection/hooks/useSystemFonts/useSystemFonts.ts
  • apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/hooks/useSplitOrientation/useSplitOrientation.ts
  • apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts
  • apps/desktop/src/renderer/stores/v2-workspace-navigation.ts
  • apps/desktop/src/shared/workspace-run-definition.test.ts
  • apps/desktop/src/shared/workspace-run-definition.ts
  • packages/host-service/src/app.ts
  • packages/host-service/src/runtime/pull-requests/pull-requests.test.ts
  • packages/host-service/src/runtime/pull-requests/pull-requests.ts
  • packages/host-service/src/trpc/router/git/git.ts
  • packages/host-service/src/trpc/router/git/utils/config-write.ts
  • packages/host-service/src/trpc/router/git/utils/git-helpers.test.ts
  • packages/host-service/src/trpc/router/git/utils/git-helpers.ts
  • packages/host-service/src/trpc/router/project/utils/ensure-main-workspace.ts
  • packages/host-service/src/trpc/router/workspace-creation/shared/adopt-existing-worktree.ts
  • packages/host-service/src/trpc/router/workspace-creation/shared/git-config.ts
  • packages/host-service/src/trpc/router/workspaces/workspaces.ts
  • packages/host-service/test/integration/bug-hunt-3.integration.test.ts
  • packages/host-service/test/integration/git.integration.test.ts
  • packages/host-service/test/integration/workspace-cleanup.integration.test.ts
  • packages/host-service/test/integration/workspace-create-delete.integration.test.ts
  • packages/panes/src/react/components/Workspace/Workspace.tsx
  • packages/panes/src/react/components/Workspace/components/TabBar/TabBar.tsx
  • packages/panes/src/react/components/Workspace/components/TabBar/components/TabItem/TabItem.tsx
  • packages/panes/src/react/components/Workspace/components/TabBar/utils/index.ts
  • packages/panes/src/react/components/Workspace/components/TabBar/utils/utils.test.ts
  • packages/panes/src/react/components/Workspace/components/TabBar/utils/utils.ts
  • packages/shared/src/host-info.ts
  • packages/ui/src/components/overflow-fade/OverflowFadeContainer/OverflowFadeContainer.tsx
  • packages/ui/src/hooks/use-overflow-fade.ts

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented May 14, 2026

Greptile Summary

This PR reapplies renderer stress/performance improvements after a revert, refactoring the WebGL terminal addon lifecycle into a dedicated controller and adding an output queue that throttles writes to the terminal DOM across animation frames.

  • WebGL context-loss fallback (terminal-webgl-addon-controller.ts): extracted into a dedicated module with dual-path fallback (xterm's onContextLoss + DOM capture listener) and canvas marking for session-replay blocking.
  • Output queue throttling (terminal-runtime.ts): PTY data chunked at 4 KB, flushed at most 2 writes per animation frame across all runtimes, with up to 1 MB buffered for parked terminals.
  • Replay suppression (terminal-ws-transport.ts): new _suppressReplay sticky flag prevents the server from re-delivering scrollback when a terminal is restored from serialized content.

Confidence Score: 4/5

The terminal infrastructure changes are well-structured and the core invariants are correctly implemented with good test coverage; both comments are cosmetic cleanup items with no runtime impact.

The output-queue throttling, WebGL context-loss controller, and replay-suppression logic are all internally consistent and covered by tests. No correctness issues were found in the attach/detach sizing path or the reconnect flow.

All critical terminal files look solid. The stress-test scripts were not reviewed in depth but are development-only tools gated by process.env.NODE_ENV.

Important Files Changed

Filename Overview
apps/desktop/src/renderer/lib/terminal/terminal-webgl-addon-controller.ts New module: manages WebGL addon lifecycle with dual context-loss paths, canvas session-replay marking, and a process-global DOM-fallback flag following the VS Code pattern.
apps/desktop/src/renderer/lib/terminal/terminal-runtime.ts Adds output chunking (4 KB), per-frame write throttling (2 writes/frame across all runtimes), a 1 MB parked-runtime queue cap, and a hasBufferedContent flag used downstream for replay suppression.
apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts Introduces _suppressReplay (sticky flag), _writeOutput (backpressure-aware write callback stored for auto-reconnects), and tightens _hasReceivedBytes to only flip on non-empty binary frames.
apps/desktop/src/renderer/lib/terminal/terminal-runtime-registry.ts Adds cross-pane buffer serialization, shouldReplayTerminalRuntime gating on connect/reconnect, and stress-debug helpers; no correctness concerns found.
apps/desktop/src/renderer/lib/posthog.ts Disables session recording and canvas capture; adds comprehensive blockSelector covering all xterm DOM surfaces including WebGL canvases; extracts config to testable buildPostHogInitConfig.

Sequence Diagram

sequenceDiagram
    participant R as TerminalRuntime
    participant A as terminal-addons
    participant C as WebglAddonController
    participant X as XTerm
    participant D as DOM

    R->>A: loadAddons(terminal)
    A->>C: loadTerminalWebglAddon(terminal)
    C->>C: scheduleAnimationFrame(callback)
    C->>X: new WebglAddon()
    C->>X: addon.onContextLoss(fallbackToDom)
    C->>X: terminal.loadAddon(addon)
    C->>D: addEventListener webglcontextlost capture
    C->>C: markWebglCanvases

    alt WebGL context lost
        D-->>C: webglcontextlost capture phase
        C->>C: event.preventDefault
        C->>C: "fallbackToDom sets suggestedRendererType=dom"
        C->>X: addon.dispose
        C->>C: unmarkWebglCanvases
        C->>X: terminal.refresh after 2 rAFs
    end

    alt Dispose handle called
        R->>C: handle.dispose
        C->>C: "disposed=true cancelAnimationFrame"
        C->>X: addon.dispose if loaded
    end
Loading

Comments Outside Diff (1)

  1. apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts, line 475-477 (link)

    P2 _writeOutput is the only new field that isn't nulled in disposeTransport. Every other closure-holding field (_terminal, onDataDisposable) is cleared, but _writeOutput keeps the (data) => writeRuntimeOutput(runtime, data) closure alive until the transport itself is collected. In practice this is harmless — cancelReconnect prevents the closure from ever being invoked after disposal — but the omission is inconsistent with the rest of the cleanup and delays GC of runtime.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts
    Line: 475-477
    
    Comment:
    `_writeOutput` is the only new field that isn't nulled in `disposeTransport`. Every other closure-holding field (`_terminal`, `onDataDisposable`) is cleared, but `_writeOutput` keeps the `(data) => writeRuntimeOutput(runtime, data)` closure alive until the transport itself is collected. In practice this is harmless — `cancelReconnect` prevents the closure from ever being invoked after disposal — but the omission is inconsistent with the rest of the cleanup and delays GC of `runtime`.
    
    
    
    How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts:475-477
`_writeOutput` is the only new field that isn't nulled in `disposeTransport`. Every other closure-holding field (`_terminal`, `onDataDisposable`) is cleared, but `_writeOutput` keeps the `(data) => writeRuntimeOutput(runtime, data)` closure alive until the transport itself is collected. In practice this is harmless — `cancelReconnect` prevents the closure from ever being invoked after disposal — but the omission is inconsistent with the rest of the cleanup and delays GC of `runtime`.

```suggestion
	transport.onDataDisposable?.dispose();
	transport.onDataDisposable = null;
	transport._writeOutput = null;
	transport.stateListeners.clear();
```

### Issue 2 of 2
apps/desktop/src/renderer/lib/terminal/terminal-webgl-addon-controller.ts:112-120
The `fallbackToDom` function sets `suggestedRendererType = "dom"` immediately and then re-sets it inside the deferred `disposeAddon({ markDomFallback: true })` call. The second assignment is redundant because `disposeAddon` always runs after the guard has already made the global flag `"dom"`. Removing the duplicate from `disposeAddon` makes the invariant explicit: the flag is an immediate signal, the timeout is purely for disposal cleanup.

```suggestion
	const fallbackToDom = () => {
		if (disposed || fallbackScheduled) return;
		fallbackScheduled = true;
		suggestedRendererType = "dom";
		fallbackTimeoutId = setTimeout(() => {
			fallbackTimeoutId = null;
			disposeAddon({ markDomFallback: false });
		}, 0);
	};
```

Reviews (1): Last reviewed commit: "Fix terminal WebGL context loss fallback" | Re-trigger Greptile

Comment on lines +112 to +120
const fallbackToDom = () => {
if (disposed || fallbackScheduled) return;
fallbackScheduled = true;
suggestedRendererType = "dom";
fallbackTimeoutId = setTimeout(() => {
fallbackTimeoutId = null;
disposeAddon({ markDomFallback: true });
}, 0);
};
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 The fallbackToDom function sets suggestedRendererType = "dom" immediately and then re-sets it inside the deferred disposeAddon({ markDomFallback: true }) call. The second assignment is redundant because disposeAddon always runs after the guard has already made the global flag "dom". Removing the duplicate from disposeAddon makes the invariant explicit: the flag is an immediate signal, the timeout is purely for disposal cleanup.

Suggested change
const fallbackToDom = () => {
if (disposed || fallbackScheduled) return;
fallbackScheduled = true;
suggestedRendererType = "dom";
fallbackTimeoutId = setTimeout(() => {
fallbackTimeoutId = null;
disposeAddon({ markDomFallback: true });
}, 0);
};
const fallbackToDom = () => {
if (disposed || fallbackScheduled) return;
fallbackScheduled = true;
suggestedRendererType = "dom";
fallbackTimeoutId = setTimeout(() => {
fallbackTimeoutId = null;
disposeAddon({ markDomFallback: false });
}, 0);
};
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: 112-120

Comment:
The `fallbackToDom` function sets `suggestedRendererType = "dom"` immediately and then re-sets it inside the deferred `disposeAddon({ markDomFallback: true })` call. The second assignment is redundant because `disposeAddon` always runs after the guard has already made the global flag `"dom"`. Removing the duplicate from `disposeAddon` makes the invariant explicit: the flag is an immediate signal, the timeout is purely for disposal cleanup.

```suggestion
	const fallbackToDom = () => {
		if (disposed || fallbackScheduled) return;
		fallbackScheduled = true;
		suggestedRendererType = "dom";
		fallbackTimeoutId = setTimeout(() => {
			fallbackTimeoutId = null;
			disposeAddon({ markDomFallback: false });
		}, 0);
	};
```

How can I resolve this? If you propose a fix, please make it concise.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
apps/desktop/src/renderer/lib/terminal/terminal-addons.ts (1)

16-20: ⚡ Quick win

Update the loadAddons JSDoc to match the delegated WebGL lifecycle.

The comment still says this function defers WebGL via requestAnimationFrame, but this change moves that behavior behind loadTerminalWebglAddon(). Keeping the contract accurate here will prevent future callers from relying on timing semantics that no longer live in this module.

♻️ Proposed comment update
 /**
  * Load optional addons onto an already-opened terminal. Returns a cleanup
- * function and addon instances. WebGL is deferred to rAF to avoid
- * racing with xterm's post-open viewport sync.
+ * function and addon instances. WebGL setup/teardown is delegated to
+ * `loadTerminalWebglAddon`.
  */

Also applies to: 40-46

🤖 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-addons.ts` around lines 16 -
20, Update the JSDoc for the loadAddons function to remove the claim that WebGL
is deferred via requestAnimationFrame and instead state that WebGL
lifecycle/deferral is handled by loadTerminalWebglAddon; mention that loadAddons
only loads addons into an already-open terminal and returns a cleanup function
and addon instances, and ensure the same wording change is applied to the
duplicate comment block near loadAddons/loadTerminalWebglAddon so callers won't
rely on requestAnimationFrame timing from this module.
🤖 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/lib/terminal/terminal-addons.ts`:
- Around line 16-20: Update the JSDoc for the loadAddons function to remove the
claim that WebGL is deferred via requestAnimationFrame and instead state that
WebGL lifecycle/deferral is handled by loadTerminalWebglAddon; mention that
loadAddons only loads addons into an already-open terminal and returns a cleanup
function and addon instances, and ensure the same wording change is applied to
the duplicate comment block near loadAddons/loadTerminalWebglAddon so callers
won't rely on requestAnimationFrame timing from this module.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: a10df78d-0346-4050-8a3f-9bf64cfc07c9

📥 Commits

Reviewing files that changed from the base of the PR and between de16750 and aa6cbd2.

📒 Files selected for processing (4)
  • apps/desktop/src/renderer/lib/terminal/terminal-addons.ts
  • apps/desktop/src/renderer/lib/terminal/terminal-image-addon.test.ts
  • apps/desktop/src/renderer/lib/terminal/terminal-image-addon.ts
  • apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts
✅ Files skipped from review due to trivial changes (2)
  • apps/desktop/src/renderer/lib/terminal/terminal-image-addon.ts
  • apps/desktop/src/renderer/lib/terminal/terminal-image-addon.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant