Skip to content

fix: send SIGWINCH to PTY when terminal element resizes#342

Merged
Wirasm merged 2 commits into
mainfrom
kild/310-terminal-resize
Feb 11, 2026
Merged

fix: send SIGWINCH to PTY when terminal element resizes#342
Wirasm merged 2 commits into
mainfrom
kild/310-terminal-resize

Conversation

@Wirasm

@Wirasm Wirasm commented Feb 11, 2026

Copy link
Copy Markdown
Owner

Summary

  • Store PTY master handle in Terminal struct (via Arc<Mutex<>>) instead of moving it into the reader thread
  • Add ResizeHandle struct that bundles resize refs (term, pty_master, current_size) with a resize_if_changed() method
  • Call resize_if_changed() from TerminalElement::prepaint() when computed dimensions differ from stored size
  • Sends SIGWINCH to child process and reflows terminal grid on resize

Programs like vim, htop, and shell prompts now respond correctly to window resize. $COLUMNS/$LINES reflect actual terminal dimensions.

Closes #310

Test plan

  • cargo fmt --check passes
  • cargo clippy --all -- -D warnings passes (0 warnings)
  • cargo test --all passes (494 passed)
  • cargo build --all succeeds
  • Manual: run kild-ui, verify echo $COLUMNS matches window width
  • Manual: resize window, verify echo $COLUMNS updates
  • Manual: open vim, resize window, verify vim redraws correctly

The embedded terminal was hardcoded to 80x24. GPUI computes correct
cols/rows from element bounds in prepaint() but never sent them to the
PTY. Store the PTY master handle in Terminal (via Arc<Mutex<>>), add
ResizeHandle that bundles the refs needed for resize operations, and
call resize_if_changed() from prepaint() when dimensions change.

This sends SIGWINCH to the child process and reflows the terminal grid
so programs like vim/htop respond to window resize correctly.

Closes #310
@Wirasm

Wirasm commented Feb 11, 2026

Copy link
Copy Markdown
Owner Author

PR Review Summary

Critical Issues (1 found)

Agent Issue Location
code-reviewer, silent-failure-hunter .expect() on current_size lock panics on poison — crashes entire UI state.rs:409-412

Important Issues (2 found)

Agent Issue Location
code-reviewer, silent-failure-hunter pty_master.lock() failure is completely silent — no logging state.rs:420
silent-failure-hunter, type-design-analyzer TerminalError::PtyResize variant defined but never used errors.rs:30-31

Suggestions (3 found)

Agent Suggestion Location
type-design-analyzer Return Result<(), TerminalError> from resize_if_changed() to surface failures and use the PtyResize variant state.rs:406
comment-analyzer SIGWINCH comment should say "updates kernel winsize, triggering SIGWINCH" (not "sends SIGWINCH") state.rs:419
comment-analyzer Lock ordering comment should document that each lock is scoped and released before acquiring the next state.rs:405

Strengths

  • Clean architectural separation via ResizeHandle — keeps resize logic out of both Terminal and TerminalElement
  • Efficient idempotency check prevents unnecessary PTY syscalls
  • Well-documented lock ordering prevents deadlocks
  • Correct prepaint ordering (resize before snapshot)
  • Good Arc ownership model for PTY master keepalive

Type Design (7.5/10)

Dimension Score
Encapsulation 8/10
Invariant Expression 6/10
Invariant Usefulness 9/10
Invariant Enforcement 7/10

Verdict: NEEDS FIXES

Two error handling issues violate the project's "No Silent Failures" principle:

  1. Must fix: Replace .expect() with match/log/return to prevent UI crash on lock poison
  2. Should fix: Log pty_master lock failures explicitly — currently completely silent
  3. Should fix: Either use PtyResize error variant or remove it

Core implementation is solid. Fixes are straightforward and don't require architectural changes.

Recommended Actions

  1. Replace .expect("current_size lock poisoned") with match + tracing::error! + early return
  2. Add tracing::error! logging when pty_master.lock() fails (currently silent if let Ok)
  3. Either make resize_if_changed() return Result<(), TerminalError> (using PtyResize variant) or remove the unused variant
  4. Minor comment accuracy fix for SIGWINCH mechanism description

- Replace .expect() on current_size lock with map_err + early return
  to prevent UI crash on lock poison
- Replace silent if-let-Ok on pty_master lock with explicit error
  logging and propagation
- Make resize_if_changed() return Result<(), TerminalError>, using the
  previously-unused PtyResize error variant
- Handle Result in TerminalElement::prepaint() with tracing::error
- Fix SIGWINCH comment accuracy (updates kernel winsize, not direct signal)
- Document lock scope management in ordering comment
@Wirasm Wirasm merged commit 3eb0182 into main Feb 11, 2026
6 checks passed
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.

kild-ui: terminal resize — send SIGWINCH to PTY when element bounds change

1 participant