Revert "Reapply renderer stress fixes without terminal sizing regression"#4566
Merged
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (85)
📝 WalkthroughWalkthroughThis PR removes renderer stress-testing infrastructure (scripts, CDPport enablement, stress harness), simplifies terminal runtime/transport by eliminating output buffering and replay logic, refactors git diff stats querying, removes workspace navigation state management in favor of route-based routing, makes hook query/effect gating unconditional (removes ChangesRenderer Stress Infrastructure Removal
Terminal Runtime & WebSocket Transport Simplification
Git Status Query Refactoring
Workspace Navigation & Route-Based Routing
Hook Unconditional Query Pattern
UI Component Refactoring
Font Discovery and Terminal Helpers Refactoring
PostHog and Miscellaneous Cleanup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
✨ 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. |
Contributor
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
Greptile SummaryThis PR reverts #4541 ("Reapply renderer stress fixes without terminal sizing regression") across 85 files, shedding ~6,800 lines of net additions to restore the pre-#4541 baseline after a terminal sizing regression was discovered.
Confidence Score: 3/5Safe to merge as a roll-back, but two improvements from #4541 — the empty-buffer replay guard and the explicit PostHog terminal-content block — are worth carrying forward into the follow-up re-apply. The revert is clean and intentional: it removes a confirmed terminal sizing regression. However, it re-introduces a transport-level issue where a zero-byte binary frame permanently disables session replay for the lifetime of the transport, potentially causing users to miss terminal output after reconnection. That fix was an explicit, targeted change in #4541 that should survive the re-apply. The PostHog session recording concern is lower urgency but worth tracking before session recording is turned on. terminal-ws-transport.ts (replay suppression on empty frame) and posthog.ts (terminal content not blocked from session recordings)
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts | Reverts replay-suppression logic: removes _suppressReplay, _writeOutput, and the byteLength guard on _hasReceivedBytes, re-introducing a case where empty binary frames incorrectly suppress replay on reconnect. |
| apps/desktop/src/renderer/lib/terminal/terminal-runtime.ts | Removes the entire output-chunking/RAF-flush queue, hasBufferedContent tracking, deferred terminal disposal, and RAF-debounced focus; restores direct terminal.dispose() which may race with pending xterm renders. |
| apps/desktop/src/renderer/lib/terminal/terminal-addons.ts | Replaces the dedicated WebGL addon controller (with canvas registry and session-replay marking) with inline single-rAF WebGL setup; functionally equivalent for normal usage. |
| apps/desktop/src/renderer/lib/posthog.ts | Removes session_recording blockSelector (covering terminal elements and WebGL canvases) and disable_session_recording flag; terminal content could appear in PostHog recordings if recording is later enabled. |
| packages/panes/src/react/components/Workspace/components/TabBar/TabBar.tsx | Removes the custom scroll-metrics + tab-windowing virtualization (getVisibleTabWindow) and reverts to OverflowFadeContainer; drops 119 lines of scroll-tracking logic cleanly. |
| apps/desktop/src/renderer/lib/terminal/terminal-runtime-registry.ts | Removes stress-test debug API (writeForStress, forceRuntimeWebglContextLoss, listEntries, focus), simplifies connect() calls by dropping writeRuntimeOutput and shouldReplayTerminalRuntime hooks. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useRendererStressWorkspaceBridge/useRendererStressWorkspaceBridge.ts | Entire 681-line renderer stress bridge deleted; this was the IPC bridge used by the stress-test scripts and is safe to remove alongside the scripts themselves. |
| packages/ui/src/hooks/use-overflow-fade.ts | Removes RAF-batched deferred measurement and measureKey prop; overflow updates now fire synchronously on scroll/resize, which is the original pre-#4541 behaviour. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[WebSocket binary frame received] --> B{ArrayBuffer?}
B -- Yes --> C[terminal.write new Uint8Array]
C --> D[_hasReceivedBytes = true UNCONDITIONAL]
B -- No --> E[Parse JSON message]
D --> F[On next connect]
F --> G{_hasReceivedBytes?}
G -- true --> H[append replay=0 to URL]
G -- false --> I[use URL as-is, server replays]
H --> J[User may miss terminal output if empty frame triggered the flag]
style D fill:#f88,stroke:#c00
style J fill:#f88,stroke:#c00
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-ws-transport.ts:309-313
**Replay suppressed on empty binary frame**
`_hasReceivedBytes` is set to `true` unconditionally for any `ArrayBuffer`, including zero-byte frames. If the server ever sends an empty binary frame (heartbeat, protocol keep-alive, or a flush signal), replay will be permanently suppressed for the lifetime of the transport. On the next reconnect the URL gets `replay=0` appended, so the user misses any terminal output that accumulated before the disconnect.
PR #4541 specifically guarded this with `if (data.byteLength > 0)` — that fix is lost in this revert. This is the clearest regression worth carrying forward into the follow-up PR that re-applies the stress fixes without the sizing issue.
### Issue 2 of 3
apps/desktop/src/renderer/lib/terminal/terminal-runtime.ts:325-326
**Synchronous terminal disposal may race with pending renders**
`runtime.terminal.dispose()` is called synchronously. If xterm.js has a pending animation-frame render in flight when `disposeRuntime` is called (e.g. during tab close), disposing mid-render can throw internal xterm errors. PR #4541 introduced `disposeTerminalAfterPendingRefresh` (double-rAF deferral) specifically to avoid this. Since the fix is being dropped here, it's worth re-incorporating in the follow-up re-apply.
### Issue 3 of 3
apps/desktop/src/renderer/lib/posthog.ts:11-30
**Terminal content no longer explicitly blocked from session recording**
The reverted config had `disable_session_recording: true` and a `blockSelector` that explicitly excluded terminal elements (`.xterm`, `.xterm-screen`, `.xterm-viewport`, the canvas layer, and `[data-terminal-webgl-canvas]`) from PostHog recordings. Neither is present after this revert. If session recording is later enabled via the PostHog dashboard or a future config change, raw terminal output could be captured in session replays. The explicit opt-out is worth restoring in the follow-up PR.
Reviews (1): Last reviewed commit: "Revert "Reapply renderer stress fixes wi..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
309
to
313
| if (event.data instanceof ArrayBuffer) { | ||
| const data = new Uint8Array(event.data); | ||
| transport._writeOutput?.(data); | ||
| if (data.byteLength > 0) { | ||
| transport._hasReceivedBytes = true; | ||
| } | ||
| terminal.write(new Uint8Array(event.data)); | ||
| transport._hasReceivedBytes = true; | ||
| return; | ||
| } |
There was a problem hiding this comment.
_hasReceivedBytes is set to true unconditionally for any ArrayBuffer, including zero-byte frames. If the server ever sends an empty binary frame (heartbeat, protocol keep-alive, or a flush signal), replay will be permanently suppressed for the lifetime of the transport. On the next reconnect the URL gets replay=0 appended, so the user misses any terminal output that accumulated before the disconnect.
PR #4541 specifically guarded this with if (data.byteLength > 0) — that fix is lost in this revert. This is the clearest regression worth carrying forward into the follow-up PR that re-applies the stress fixes without the sizing issue.
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: 309-313
Comment:
**Replay suppressed on empty binary frame**
`_hasReceivedBytes` is set to `true` unconditionally for any `ArrayBuffer`, including zero-byte frames. If the server ever sends an empty binary frame (heartbeat, protocol keep-alive, or a flush signal), replay will be permanently suppressed for the lifetime of the transport. On the next reconnect the URL gets `replay=0` appended, so the user misses any terminal output that accumulated before the disconnect.
PR #4541 specifically guarded this with `if (data.byteLength > 0)` — that fix is lost in this revert. This is the clearest regression worth carrying forward into the follow-up PR that re-applies the stress fixes without the sizing issue.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
325
to
+326
| runtime.wrapper.remove(); | ||
| runtime.terminal.dispose(); |
There was a problem hiding this comment.
runtime.terminal.dispose() is called synchronously. If xterm.js has a pending animation-frame render in flight when disposeRuntime is called (e.g. during tab close), disposing mid-render can throw internal xterm errors. PR #4541 introduced disposeTerminalAfterPendingRefresh (double-rAF deferral) specifically to avoid this. Since the fix is being dropped here, it's worth re-incorporating in the follow-up re-apply.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/lib/terminal/terminal-runtime.ts
Line: 325-326
Comment:
**Synchronous terminal disposal may race with pending renders**
`runtime.terminal.dispose()` is called synchronously. If xterm.js has a pending animation-frame render in flight when `disposeRuntime` is called (e.g. during tab close), disposing mid-render can throw internal xterm errors. PR #4541 introduced `disposeTerminalAfterPendingRefresh` (double-rAF deferral) specifically to avoid this. Since the fix is being dropped here, it's worth re-incorporating in the follow-up re-apply.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+11
to
30
| return; | ||
| } | ||
|
|
||
| export function buildPostHogInitConfig(): Partial<PostHogConfig> { | ||
| return { | ||
| posthogFull.init(env.NEXT_PUBLIC_POSTHOG_KEY, { | ||
| api_host: env.NEXT_PUBLIC_POSTHOG_HOST, | ||
| defaults: "2025-11-30", | ||
| autocapture: false, | ||
| capture_pageview: false, | ||
| capture_pageleave: false, | ||
| capture_exceptions: true, | ||
| disable_scroll_properties: true, | ||
| disable_session_recording: true, | ||
| person_profiles: "identified_only", | ||
| persistence: "localStorage", | ||
| debug: false, | ||
| session_recording: { | ||
| blockSelector: POSTHOG_SESSION_REPLAY_BLOCK_SELECTOR, | ||
| captureCanvas: { | ||
| recordCanvas: false, | ||
| canvasFps: 0, | ||
| canvasQuality: "0", | ||
| }, | ||
| }, | ||
| loaded: (ph) => { | ||
| ph.register({ | ||
| app_name: "desktop", | ||
| platform: window.navigator.platform, | ||
| }); | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| export function initPostHog() { | ||
| if (!env.NEXT_PUBLIC_POSTHOG_KEY) { | ||
| console.log("[posthog] No key configured, skipping"); | ||
| return; | ||
| } | ||
|
|
||
| posthogFull.init(env.NEXT_PUBLIC_POSTHOG_KEY, buildPostHogInitConfig()); | ||
| }); | ||
| } |
There was a problem hiding this comment.
The reverted config had disable_session_recording: true and a blockSelector that explicitly excluded terminal elements (.xterm, .xterm-screen, .xterm-viewport, the canvas layer, and [data-terminal-webgl-canvas]) from PostHog recordings. Neither is present after this revert. If session recording is later enabled via the PostHog dashboard or a future config change, raw terminal output could be captured in session replays. The explicit opt-out is worth restoring in the follow-up PR.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/lib/posthog.ts
Line: 11-30
Comment:
**Terminal content no longer explicitly blocked from session recording**
The reverted config had `disable_session_recording: true` and a `blockSelector` that explicitly excluded terminal elements (`.xterm`, `.xterm-screen`, `.xterm-viewport`, the canvas layer, and `[data-terminal-webgl-canvas]`) from PostHog recordings. Neither is present after this revert. If session recording is later enabled via the PostHog dashboard or a future config change, raw terminal output could be captured in session replays. The explicit opt-out is worth restoring in the follow-up PR.
How can I resolve this? If you propose a fix, please make it concise.
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.
Reverts #4541
Summary by cubic
Reverts the renderer stress tooling and related experimental paths to simplify the desktop app. Removes CDP/stress fixtures, custom terminal WebGL/session-replay layers, and queued navigation, returning to leaner defaults.
SUPERSET_RENDERER_STRESS_CDP_PORTusage; devtools load unconditionally in dev. Cleanedapps/desktop/package.jsonscripts.@xterm/addon-webgland@xterm/addon-imagedirectly; deleted custom WebGL controller/canvas registry/session-replay code and output queuing. Simplified WS transport and helpers; updated addon loader.useDiffStatsnow derives totals fromgit.getStatus; removed diff-stats codepath on the host. Review/changes/editor hooks always enabled.@superset/ui/overflow-fade-*.use-overflow-fadedrops measureKey prop.git-configwrites usesimple-gittypes directly; removed ensure-main-workspace coalescing and adjusted workspace config writes. Pruned related tests.Written for commit 98e6e71. Summary will update on new commits.
Summary by CodeRabbit
Removed Features
Improvements
enabledparameters.Bug Fixes & Refactoring