Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds a debounced, scheduler-based terminal resize flow with explicit dispose handling and scroll-preserving resize detection; also adds a planning document comparing v2 xterm.js integration divergences with reference Electron+xterm implementations. Changes
Sequence Diagram(s)sequenceDiagram
participant Container
participant ResizeObserver
participant ResizeScheduler
participant TerminalRuntime
participant FontSubsystem
ResizeObserver->>ResizeScheduler: notify size change
ResizeScheduler->>ResizeScheduler: debounce (RESIZE_DEBOUNCE_MS)
ResizeScheduler-->>ResizeObserver: cancel previous timers (if reattach)
ResizeScheduler->>TerminalRuntime: invoke measureAndResize()
TerminalRuntime->>FontSubsystem: ensure fonts settled (post-open settle)
TerminalRuntime->>TerminalRuntime: measure cols/rows, detect pinned-to-bottom
alt size changed
TerminalRuntime->>TerminalRuntime: restore viewport if pinned
TerminalRuntime->>TerminalRuntime: fit/refresh
else no size change
TerminalRuntime-->>ResizeScheduler: skip onResize callback
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR stabilises the v2 terminal resize path by introducing a 75 ms debounced Confidence Score: 5/5Safe to merge; all remaining findings are P2 style suggestions. The logic is sound: debounce + zero-size guard + scroll preservation + dimension-change gating are all correctly implemented. Dispose is called in every exit path (attach, detach, dispose). The only finding is a minor ordering issue (refresh before scroll restore) that is harmless due to rAF batching and does not affect correctness. No files require special attention.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/lib/terminal/terminal-runtime.ts | Adds 75 ms debounced ResizeObserver scheduler, scroll-position preservation around fit(), and backend resize gating on actual dimension changes; correctly disposes pending timeouts on attach/detach/dispose. One minor ordering concern: refresh() is called before scroll restore rather than after. |
| plans/20260425-v2-terminal-rendering-divergences.md | New planning document cataloguing v2 xterm rendering divergences vs VS Code/Hyper/Tabby; items #4 and #5 are now addressed by this PR. No code changes, purely informational. |
Sequence Diagram
sequenceDiagram
participant DOM as ResizeObserver
participant Sched as ResizeScheduler
participant MR as measureAndResize
participant XTerm as xterm.js
participant Back as Backend
DOM->>Sched: observe(entries)
alt any entry has zero size
Sched->>Sched: dispose() – cancel pending timeout
else valid dimensions
Sched->>Sched: dispose() then setTimeout(run, 75ms)
Note over Sched: debounce: only last resize in 75ms window fires
Sched->>MR: run()
MR->>XTerm: capture viewportY / baseY / cols / rows
MR->>XTerm: fitAddon.fit()
MR->>XTerm: refresh(0, rows-1)
alt was pinned to bottom
MR->>XTerm: scrollToBottom()
else scrolled up
MR->>XTerm: scrollToLine(clamp(savedY, newBaseY))
end
alt cols or rows changed
MR-->>Sched: return true
Sched->>Back: onResize() → send resize message
else unchanged
MR-->>Sched: return false
Note over Back: no backend message sent
end
end
Prompt To Fix All With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/lib/terminal/terminal-runtime.ts
Line: 218-221
Comment:
**`refresh` fires before scroll position is restored**
`terminal.refresh()` is queued before `scrollToBottom()` / `scrollToLine()` runs. Because xterm schedules rendering via `requestAnimationFrame`, the browser typically batches everything into one frame and the final scroll position wins — so this is harmless in practice. However, the order reads as "repaint then scroll", which may confuse future readers and could theoretically produce a flash if xterm ever synchronises viewport state during `refresh`. Moving `refresh` to after the scroll-restore block makes intent explicit and matches the pattern suggested in the diff's own plan document (section 5, "mirror Tabby's pattern").
```suggestion
runtime.fitAddon.fit();
runtime.lastCols = terminal.cols;
runtime.lastRows = terminal.rows;
if (wasPinnedToBottom) {
terminal.scrollToBottom();
} else {
const targetY = Math.min(savedViewportY, terminal.buffer.active.baseY);
if (terminal.buffer.active.viewportY !== targetY) {
terminal.scrollToLine(targetY);
}
}
terminal.refresh(0, Math.max(0, terminal.rows - 1));
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(desktop): stabilize v2 terminal resi..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| runtime.fitAddon.fit(); | ||
| runtime.lastCols = runtime.terminal.cols; | ||
| runtime.lastRows = runtime.terminal.rows; | ||
| runtime.lastCols = terminal.cols; | ||
| runtime.lastRows = terminal.rows; | ||
| terminal.refresh(0, Math.max(0, terminal.rows - 1)); |
There was a problem hiding this comment.
refresh fires before scroll position is restored
terminal.refresh() is queued before scrollToBottom() / scrollToLine() runs. Because xterm schedules rendering via requestAnimationFrame, the browser typically batches everything into one frame and the final scroll position wins — so this is harmless in practice. However, the order reads as "repaint then scroll", which may confuse future readers and could theoretically produce a flash if xterm ever synchronises viewport state during refresh. Moving refresh to after the scroll-restore block makes intent explicit and matches the pattern suggested in the diff's own plan document (section 5, "mirror Tabby's pattern").
| runtime.fitAddon.fit(); | |
| runtime.lastCols = runtime.terminal.cols; | |
| runtime.lastRows = runtime.terminal.rows; | |
| runtime.lastCols = terminal.cols; | |
| runtime.lastRows = terminal.rows; | |
| terminal.refresh(0, Math.max(0, terminal.rows - 1)); | |
| runtime.fitAddon.fit(); | |
| runtime.lastCols = terminal.cols; | |
| runtime.lastRows = terminal.rows; | |
| if (wasPinnedToBottom) { | |
| terminal.scrollToBottom(); | |
| } else { | |
| const targetY = Math.min(savedViewportY, terminal.buffer.active.baseY); | |
| if (terminal.buffer.active.viewportY !== targetY) { | |
| terminal.scrollToLine(targetY); | |
| } | |
| } | |
| terminal.refresh(0, Math.max(0, terminal.rows - 1)); |
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: 218-221
Comment:
**`refresh` fires before scroll position is restored**
`terminal.refresh()` is queued before `scrollToBottom()` / `scrollToLine()` runs. Because xterm schedules rendering via `requestAnimationFrame`, the browser typically batches everything into one frame and the final scroll position wins — so this is harmless in practice. However, the order reads as "repaint then scroll", which may confuse future readers and could theoretically produce a flash if xterm ever synchronises viewport state during `refresh`. Moving `refresh` to after the scroll-restore block makes intent explicit and matches the pattern suggested in the diff's own plan document (section 5, "mirror Tabby's pattern").
```suggestion
runtime.fitAddon.fit();
runtime.lastCols = terminal.cols;
runtime.lastRows = terminal.rows;
if (wasPinnedToBottom) {
terminal.scrollToBottom();
} else {
const targetY = Math.min(savedViewportY, terminal.buffer.active.baseY);
if (terminal.buffer.active.viewportY !== targetY) {
terminal.scrollToLine(targetY);
}
}
terminal.refresh(0, Math.max(0, terminal.rows - 1));
```
How can I resolve this? If you propose a fix, please make it concise.
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/lib/terminal/terminal-runtime.ts (1)
366-384:⚠️ Potential issue | 🟠 MajorFont-change resize doesn't notify the backend.
updateRuntimeAppearancecallsmeasureAndResize(runtime)but ignores its boolean return and has noonResizeparameter to invoke. A font-family/font-size change frequently changes the cell metrics and thereforeterminal.cols/rows— but the PTY backend won't see a resize message until the nextResizeObserverfire happens to coincide with a layout change. Until then, output is rendered at the new cell size against PTY dimensions sized for the old cell size, which is exactly the kind of drift this PR is trying to remove ("Sends backend resize messages only when calculated cols or rows actually change").Threading an
onResizethrough (or storing it on the runtime whenattachToContaineris called and reusing it here) would close the gap.♻️ One option: stash the active
onResizeon the runtime and reuse itexport interface TerminalRuntime { ... _disposeResizeObserver: (() => void) | null; + _onResize: (() => void) | null; ... }export function attachToContainer( runtime: TerminalRuntime, container: HTMLDivElement, onResize?: () => void, ) { ... + runtime._onResize = onResize ?? null; if (measureAndResize(runtime)) onResize?.(); ... }export function updateRuntimeAppearance( runtime: TerminalRuntime, appearance: TerminalAppearance, ) { ... if (fontChanged) { terminal.options.fontFamily = appearance.fontFamily; terminal.options.fontSize = appearance.fontSize; if (hostIsVisible(runtime.container)) { - measureAndResize(runtime); + if (measureAndResize(runtime)) runtime._onResize?.(); } } }Don't forget to clear
_onResizeindetachFromContainer/disposeRuntime.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/lib/terminal/terminal-runtime.ts` around lines 366 - 384, updateRuntimeAppearance sets terminal font options and calls measureAndResize(runtime) but ignores its boolean result and doesn't notify the PTY; change it to call the runtime's onResize callback when measureAndResize(runtime) returns true so backend receives immediate resize events after font changes. Thread an onResize handler into attachToContainer and store it on the TerminalRuntime (e.g., runtime._onResize or runtime.onResize) so updateRuntimeAppearance can invoke runtime._onResize(cols, rows) when measureAndResize returns true; ensure the handler is cleared in detachFromContainer / disposeRuntime to avoid leaks.
🧹 Nitpick comments (2)
plans/20260425-v2-terminal-rendering-divergences.md (1)
68-71: Nit: add a language to fenced code blocks (MD040).
markdownlintflags lines 68, 86, 136, 158, 186 for missing fence languages. These are all illustrativexterm/xtermFrontendsnippets; tagging them withtext(orts) clears the warnings and keeps the doc lint-clean.-``` +```text xtermFrontend.ts:290 this.webGLAddon?.clearTextureAtlas() xtermFrontend.ts:301 this.canvasAddon?.clearTextureAtlas()(Apply the same to the other four blocks.) <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@plans/20260425-v2-terminal-rendering-divergences.mdaround lines 68 - 71,
Several fenced code blocks in the document (the illustrative xterm/xtermFrontend
snippets such as the block showing "xtermFrontend.ts:290
this.webGLAddon?.clearTextureAtlas()" and the one with "xtermFrontend.ts:301
this.canvasAddon?.clearTextureAtlas()") lack a fence language and trigger
markdownlint MD040; update each of those fenced blocks (including the other four
flagged blocks at lines mentioned in the review) to include a language tag
(preferably "ts" or "text") immediately after the opening triple backticks so
linting passes and the snippets remain clearly identified.</details> </blockquote></details> <details> <summary>apps/desktop/src/renderer/lib/terminal/terminal-runtime.ts (1)</summary><blockquote> `257-269`: **Zero-size short-circuit drops the whole batch, not just the bad entry.** `entries.some(...)` causes `dispose()` + early return if *any* entry is zero-sized, even if the other entries are valid. Today this is benign because each scheduler observes exactly one container, so the batch only ever contains that container's entry. If a future change observes multiple elements (e.g. wrapper + container) with the same observer, this would silently drop legitimate resizes. Cheap hardening: filter out zero-sized entries and only bail when *all* are zero. <details> <summary>♻️ Possible refactor</summary> ```diff - const observe: ResizeObserverCallback = (entries) => { - if ( - entries.some( - (entry) => - entry.contentRect.width <= 0 || entry.contentRect.height <= 0, - ) - ) { - dispose(); - return; - } - dispose(); - timeoutId = setTimeout(run, RESIZE_DEBOUNCE_MS); - }; + const observe: ResizeObserverCallback = (entries) => { + const hasNonZero = entries.some( + (entry) => + entry.contentRect.width > 0 && entry.contentRect.height > 0, + ); + dispose(); + if (!hasNonZero) return; + timeoutId = setTimeout(run, RESIZE_DEBOUNCE_MS); + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/lib/terminal/terminal-runtime.ts` around lines 257 - 269, The resize observer callback observe currently returns early if any entry is zero-sized which drops the whole batch; change the logic in the ResizeObserverCallback (observe) to ignore/filter out entries with entry.contentRect.width <= 0 or height <= 0, then if the filtered list is empty call dispose() and return, otherwise call dispose(), set timeoutId = setTimeout(run, RESIZE_DEBOUNCE_MS) to schedule handling; keep references to dispose, timeoutId, run and RESIZE_DEBOUNCE_MS to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/desktop/src/renderer/lib/terminal/terminal-runtime.ts`:
- Around line 366-384: updateRuntimeAppearance sets terminal font options and
calls measureAndResize(runtime) but ignores its boolean result and doesn't
notify the PTY; change it to call the runtime's onResize callback when
measureAndResize(runtime) returns true so backend receives immediate resize
events after font changes. Thread an onResize handler into attachToContainer and
store it on the TerminalRuntime (e.g., runtime._onResize or runtime.onResize) so
updateRuntimeAppearance can invoke runtime._onResize(cols, rows) when
measureAndResize returns true; ensure the handler is cleared in
detachFromContainer / disposeRuntime to avoid leaks.
---
Nitpick comments:
In `@apps/desktop/src/renderer/lib/terminal/terminal-runtime.ts`:
- Around line 257-269: The resize observer callback observe currently returns
early if any entry is zero-sized which drops the whole batch; change the logic
in the ResizeObserverCallback (observe) to ignore/filter out entries with
entry.contentRect.width <= 0 or height <= 0, then if the filtered list is empty
call dispose() and return, otherwise call dispose(), set timeoutId =
setTimeout(run, RESIZE_DEBOUNCE_MS) to schedule handling; keep references to
dispose, timeoutId, run and RESIZE_DEBOUNCE_MS to locate the change.
In `@plans/20260425-v2-terminal-rendering-divergences.md`:
- Around line 68-71: Several fenced code blocks in the document (the
illustrative xterm/xtermFrontend snippets such as the block showing
"xtermFrontend.ts:290 this.webGLAddon?.clearTextureAtlas()" and the one with
"xtermFrontend.ts:301 this.canvasAddon?.clearTextureAtlas()") lack a fence
language and trigger markdownlint MD040; update each of those fenced blocks
(including the other four flagged blocks at lines mentioned in the review) to
include a language tag (preferably "ts" or "text") immediately after the opening
triple backticks so linting passes and the snippets remain clearly identified.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 473a3b1a-f45c-4c47-9423-ac8576ce5f4e
📒 Files selected for processing (2)
apps/desktop/src/renderer/lib/terminal/terminal-runtime.tsplans/20260425-v2-terminal-rendering-divergences.md
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
PR1〜PR5 (#435 #436 #437 #438 #440) で 13 commits 全件 cherry-pick + 手動 conflict 解消で取り込み済み。 本コミットは git 履歴上 behind=0 とするための ours マージ記録。 取り込み済み 13 commits: - 1f55c62 Fix host service restart adoption (superset-sh#3732) - 0fe65d2 test(desktop): remove host-service-coordinator test (superset-sh#3734) - 3012b5a Add optimistic Electric collection updates (superset-sh#3722) - c272a51 fix(desktop): drop branch row from v2 sidebar workspace item (superset-sh#3733) - c2f3fdc feat(desktop): add fade-edge mask utilities (superset-sh#3735) - 682d07c fix v2 terminal osc links (superset-sh#3736) - 7c0d22b feat(ports): surface remote host-service ports in the sidebar (superset-sh#3676) - 6a3be2d [codex] Stabilize v2 terminal resize (superset-sh#3739) - 8928ac6 [codex] Improve v2 pane header responsiveness (superset-sh#3737) - 5fe3d22 refactor(desktop): tidy v2 terminal session dropdown (superset-sh#3743) - 66c23d6 Fix automation timezone scheduling (superset-sh#3738) - 16e270c [codex] Add terminal session titles (superset-sh#3740) - 583fa5d fix(desktop): refit v2 terminal after font settle (superset-sh#3742)
1 participant
Summary
Root Cause
The v2 terminal resize path called fit immediately for every ResizeObserver callback, did not preserve xterm viewport state around fit, and notified the backend even when terminal dimensions were unchanged. During pane/sidebar/window layout churn this could produce excess resize traffic and visible scroll/render instability.
Validation
Summary by cubic
Stabilizes v2 terminal resizing to remove flicker and scroll jumps while reducing unnecessary resize messages to the backend. Also adds a plan documenting verified terminal rendering divergences and follow-ups.
ResizeObservercallbacks (75ms) and ignored zero-size entries.fit(), then explicitly refreshed the viewport.colsorrowschanged.Written for commit 505f24c. Summary will update on new commits.
Summary by CodeRabbit