fix(desktop): trigger TUI resize on reattach and focus#792
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughEnhanced the Terminal component's focus-handling effect to trigger a resize nudge sequence when the terminal pane gains focus in alternate screen mode. This uses requestAnimationFrame timing and delayed resizing to force SIGWINCH-based repaints for terminal user interfaces (TUIs). Added paneId to the effect dependency to scope behavior per tab. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx`:
- Around line 999-1001: The resize nudge currently calls resizeRef.current({
paneId, cols, rows: rows - 1 }) even when rows === 1 which results in rows: 0
and may not trigger SIGWINCH; change the guard to only perform the down-nudge
when rows > 1, i.e., if (cols > 0 && rows > 1) then do the resizeRef.current
down-nudge and subsequent up-nudge. Also extract the 100 (ms) timeout into a
named module-level constant (e.g., RESIZE_NUDGE_TIMEOUT_MS) and use that
constant instead of the magic number in the setTimeout call so the meaning is
clear and follows coding guidelines.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (1)
1002-1007: Extract the 100ms nudge delay into a named constant.Centralizing timing values improves maintainability and keeps the nudge behavior consistent.
♻️ Suggested refactor
+const FOCUS_RESIZE_NUDGE_DELAY_MS = 100;- setTimeout(() => { + setTimeout(() => { if (xtermRef.current !== xterm) return; resizeRef.current({ paneId, cols, rows }); // Force xterm to repaint after SIGWINCH completes xterm.refresh(0, rows - 1); - }, 100); + }, FOCUS_RESIZE_NUDGE_DELAY_MS);As per coding guidelines, extract magic numbers to named constants.
| if (cols > 0 && rows > 0) { | ||
| // Resize down then up to guarantee SIGWINCH is sent to PTY | ||
| resizeRef.current({ paneId, cols, rows: rows - 1 }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
xterm.js PTY resize minimum rows cols constraints specification
💡 Result:
Short answer
- xterm.js itself does not impose a hard minimum for cols/rows — cols/rows can only be set in the constructor and can be changed at runtime with Terminal.resize(cols, rows). You should debounce resize calls so the PTY can process them. [1][2]
What actually matters for the running process
- The PTY/window size is maintained in the kernel via struct winsize and the TIOCSWINSZ/TIOCGWINSZ ioctls; the kernel fields are unsigned shorts (ws_row, ws_col) and a value of 0 is treated as “undefined”. The kernel/PTY layer does not enforce an application-level minimum; programs or helper utilities often fall back to 80×24 when sizes are missing or zero. [3][4]
Practical constraints and gotchas
- You must send the new size from the browser to your backend and call TIOCSWINSZ on the slave PTY; if you don’t, shells and full‑screen apps will continue to see the old/default (often 80×24). Many examples/tools (e.g., resize implementations) apply a minimum fallback like 80×24. [5][2][6]
References
- xterm.js Terminal API (cols/rows, resize). [1]
- xterm.js docs: cols/rows set only at constructor; Terminal.resize exists and debounce recommendation. [2]
- TIOCSWINSZ / struct winsize (ws_row, ws_col; 0 = undefined). man pages. [3][4]
- historical resize.c: common fallback to 80×24 when inputs are missing/too small. [5]
Sources
[1] xterm.js Terminal API docs.
[2] xterm.js class/docs pages (cols/rows options, Terminal.resize).
[3] man7.org TIOCSWINSZ/TIOCGWINSZ manual.
[4] tty(7) / winsize description (0 means undefined).
[5] example resize.c showing 80×24 fallback.
🏁 Script executed:
# First, let's examine the actual Terminal.tsx file to understand the context
find . -name "Terminal.tsx" -path "*/apps/desktop/src/renderer/*" | head -5Repository: superset-sh/superset
Length of output: 175
🏁 Script executed:
# Search for resize-related code in the Terminal component
fd "Terminal.tsx" -path "*/apps/desktop/src/renderer/*" -exec cat -n {} + | head -100Repository: superset-sh/superset
Length of output: 232
🏁 Script executed:
cat -n ./apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx | sed -n '990,1015p'Repository: superset-sh/superset
Length of output: 995
🏁 Script executed:
# Get more context around the resize logic
cat -n ./apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx | sed -n '960,1020p'Repository: superset-sh/superset
Length of output: 2607
🏁 Script executed:
# Search for any existing row/col validation in the file
rg "rows.*[<>]|cols.*[<>]" ./apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx -A 2 -B 2Repository: superset-sh/superset
Length of output: 846
Guard resize nudge for rows > 1 and extract magic number timeout constant.
When rows === 1, the resize-down call sends rows: 0, which prevents the resize from actually changing dimensions and may not trigger SIGWINCH as intended. Additionally, the 100ms timeout is a magic number that should be extracted to a named constant at the module top per coding guidelines.
Suggested fix
+const RESIZE_NUDGE_TIMEOUT_MS = 100;
if (cols > 0 && rows > 1) {
// Resize down then up to guarantee SIGWINCH is sent to PTY
resizeRef.current({ paneId, cols, rows: rows - 1 });
setTimeout(() => {
if (xtermRef.current === xterm) {
resizeRef.current({ paneId, cols, rows });
xterm.refresh(0, rows - 1);
}
- }, 100);
+ }, RESIZE_NUDGE_TIMEOUT_MS);🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx`
around lines 999 - 1001, The resize nudge currently calls resizeRef.current({
paneId, cols, rows: rows - 1 }) even when rows === 1 which results in rows: 0
and may not trigger SIGWINCH; change the guard to only perform the down-nudge
when rows > 1, i.e., if (cols > 0 && rows > 1) then do the resizeRef.current
down-nudge and subsequent up-nudge. Also extract the 100 (ms) timeout into a
named module-level constant (e.g., RESIZE_NUDGE_TIMEOUT_MS) and use that
constant instead of the magic number in the setTimeout call so the meaning is
clear and follows coding guidelines.
Kitenite
force-pushed
the
opencode-test
branch
2 times, most recently
from
1e3cdb0 to
61b8764
Compare
Kitenite
changed the title
When switching between panes within a tab, TUI apps in alternate screen mode would show stale/blank content because they didn't know the terminal regained focus. Two mechanisms exist for TUI repaint: 1. Focus reporting (mode 1004): App sends ESC[?1004h, terminal sends CSI I on focus - handled automatically by xterm.js 2. SIGWINCH: Resize signal triggers repaint - needed for apps without focus reporting Added resize nudge (same pattern as alt-screen reattach) when isFocused changes for alt-screen terminals. This ensures TUIs like vim/htop repaint correctly regardless of whether they use focus reporting.
Kitenite
force-pushed
the
opencode-test
branch
from
61b8764 to
52bc9b6
Compare
The reattach cases are sufficient - pane focus change doesn't need SIGWINCH since apps with focus reporting get CSI I automatically.
This reverts commit a35282b.
Pane focus change doesn't need SIGWINCH - apps with focus reporting get CSI I automatically from xterm.js. Reattach cases still trigger resize nudge for TUI repaint.
This reverts commit 19c4da7.
This reverts commit ca3ec28.
…e nudge"" This reverts commit 1d4e03c.
- Remove resize nudge pattern (resize down/up) - Just call fitAddon.fit() on reattach after streaming is enabled - Fixes race condition by ensuring resize happens after attach
Kitenite
changed the title
Kitenite
changed the title
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
1 participant
Summary
Test plan