fix(client): query host cell size when the ioctl reports no pixels - #2160
Conversation
kangal-bot
added
the
ai-review
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe client parses XTWINOPS cell-size reports, tracks pending replies, stores reported dimensions, and uses them during terminal geometry and resize polling. Input and application handlers consume these reports without forwarding them as UI events. ChangesTerminal cell-size reporting
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ClientLoop
participant HostTerminal
participant RawInputParser
participant ResizePoller
ClientLoop->>HostTerminal: Request XTWINOPS cell size
HostTerminal-->>RawInputParser: Return cell-size escape sequence
RawInputParser-->>ClientLoop: Emit HostCellSizeReport
ClientLoop->>ResizePoller: Update shared cell dimensions
ResizePoller-->>ClientLoop: Calculate current terminal geometry
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThe PR adds a one-shot XTWINOPS cell-size query when terminal ioctls provide no pixel dimensions, then caches the response for the existing resize path.
Confidence Score: 5/5The PR appears safe to merge because no blocking failure remains eligible or outstanding in the supplied follow-up-review context. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/client/mod.rs | Adds query gating, atomic cell-size caching, fallback selection, and pixel-aware resize propagation. |
| src/raw_input.rs | Adds exact XTWINOPS response parsing and bounded framing support for split or malformed replies. |
| src/client/input.rs | Arms the Unix stdin framer when the one-shot host cell-size query is sent. |
| src/app/runtime.rs | Explicitly ignores the thin-client-only cell-size report in the application runtime. |
| src/app/mod.rs | Adds exhaustive no-op handling for the new raw-input event. |
Reviews (9): Last reviewed commit: "fix(input): discard timed-out host cell ..." | Re-trigger Greptile
![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
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5b577b9b-0b9c-402f-bcdb-e981f5a21a94
📒 Files selected for processing (5)
src/app/mod.rssrc/app/runtime.rssrc/client/input.rssrc/client/mod.rssrc/raw_input.rs
4c83047 to
39766a7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f199a0e3-6f77-40bc-b438-344d09feb1ad
📒 Files selected for processing (3)
src/client/input.rssrc/client/mod.rssrc/raw_input.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/mod.rs
|
@coderabbitai review |
✅ Action performedReview finished.
|
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)
src/client/input.rs (1)
100-123: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDrain pending queries before the second timeout flush.
A resize query can increment
pending_host_cell_size_querieswhilestdin_read_ready()waits at Lines 115-118. Line 120 then callsflush_timeout()without registering that query. The framer can release the held Escape and clear its reply holds before the delayed reply arrives. A reply split afterESCcan then reach pane input.Drain the counter immediately before the second
flush_timeout(). Add a regression test where a second query is registered between the first and second timeout flushes.Proposed fix
if held_escape && stdin_read_ready( &reader, crate::raw_input::RAW_INPUT_IDLE_FLUSH_TIMEOUT_MS, ) == Some(false) - && !send_unix_input_chunks( - framer.flush_timeout(), - &event_tx, - &mut pending_palette, - ) { - return; + arm_pending_host_cell_size_queries( + &mut framer, + &pending_host_cell_size_queries, + ); + if !send_unix_input_chunks( + framer.flush_timeout(), + &event_tx, + &mut pending_palette, + ) { + return; + } }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: db1b1f08-ca12-415b-ac45-07ff5225e2fb
📒 Files selected for processing (3)
src/client/input.rssrc/client/mod.rssrc/raw_input.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/raw_input.rs
- src/client/mod.rs
|
Addressed the outside-diff-range finding (drain before the retry flush) in 130d3bb: the held-escape retry path now drains the shared pending-queries counter immediately before the second timeout flush, so a resize re-query registered during the retry wait extends the hold instead of leaking the held escape. Added the suggested regression test ( |
ogulcancelik
left a comment
ogulcancelik
left a comment
There was a problem hiding this comment.
thanks for the detailed reproduction. the underlying fix makes sense, but the implementation has grown too complex for this scope. re-querying after every resize introduces cross-thread outstanding-reply accounting and timeout races that now dominate the patch.
please simplify this to one cell-size query during initial attach, cache the valid response, and propagate it through the existing resize path. live font-size tracking can follow separately if needed.
please also substantially reduce the inline comments and duplicated tests. comments should explain only non-obvious invariants or reasoning, not restate the code. if you are using claude, instruct it to write comments only when they are strictly necessary. i’m not comfortable merging the current shape.
|
Simplified in 7e98404 as requested: the client now sends a single Comments are trimmed to non-obvious invariants (the poller baseline race, the packed cache format) and the duplicated tests are gone — the split-reply hold is covered once, in the framer's own tests. PR description updated to match. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
Terminals behind ConPTY (WSL) report zero pixel sizes through TIOCGWINSZ, so the client fell back to a hardcoded 8x16 cell size and pane graphics rendered blurry. Ask the host terminal with CSI 16 t when the ioctl reports no pixels, parse the CSI 6 ; height ; width t reply on the existing stdin path, and let the resize poller propagate the reported size through the existing resize message. Terminals that never reply keep the 8x16 fallback. refs herdrdev#2146
A cell size reply split right after its ESC introducer could leak a plain Escape key through the idle flush, since only the host color query armed the framer hold. Arm the hold for the cell size query the same way and cover the split reply in tests. refs herdrdev#2146
The framer hold was armed only for the startup query, so a reply to a resize-time re-query split at its ESC introducer could still leak a plain Escape key. Count each emitted query in a shared counter that the stdin reader drains into the framer, accumulating outstanding replies instead of resetting them. refs herdrdev#2146
The held-escape retry path flushed without draining the shared query counter, so a resize re-query issued during the retry wait could not extend the framer hold and the held escape leaked as a plain Escape key. Drain the counter right before the second timeout flush. refs herdrdev#2146
Simplify the cell size query to a single probe during initial attach, as requested in review. Dropping the per-resize re-query removes the cross-thread outstanding-reply accounting: the shared pending-query counter, its drain points in the stdin reader, and the accumulating framer counter all reduce to the same one-shot bool handoff the theme query already uses. The cached reply still propagates through the existing resize poll path. Also trims restating comments and duplicated tests across the touched files. refs herdrdev#2146
425917d to
f6f551a
Compare
ogulcancelik
left a comment
ogulcancelik
left a comment
There was a problem hiding this comment.
the simplified implementation looks good. i fixed a couple of remaining input-framing edge cases.
|
ty! |
Range 9a4ce5e..10eb0bc (8 commits): host color-scheme propagation to pane apps (herdrdev#2214), preserve hover during extended-button drags, older-git snapshot-check support, publish only released docs, non-blocking foreground cwd checks (herdrdev#2213), non-UTF-8 CLI arg reporting (herdrdev#2207), host cell-size query when ioctl reports no pixels (herdrdev#2160), preserve claude settings formatting (herdrdev#2089). Resolutions: Cargo.toml/Cargo.lock both-add reconciled to keep fork's jiff alongside upstream's jsonc-parser. justfile install-local recipe preserved while adopting upstream justfile changes. Fork status strip, StatusConfig, and herdrdev#1876 non-US shifted-keybinding revert (3c15069) all preserved; input and terminal_theme fixes adopted additively.
3 participants
Summary
CSI 16 t) once duringattach when the terminal size ioctl reports zero pixels, and parse the
CSI 6 ; height ; width treply on the client's existing stdin paththe server — no wire protocol or server changes
whose ioctl reports pixels untouched
Under WSL, ConPTY zeroes the pixel fields of TIOCGWINSZ, so the client sent the
hardcoded 8x16 fallback to the server and pane graphics rendered at roughly 60%
of the real pixel count (blurry). The outer terminal knows the real cell size
and reports it over XTWINOPS: WezTerm answers
CSI 16 twith 10x21 px.Root cause vs. the issue
Issue #2146 pointed at
HostCellSize::try_from_terminalinsrc/kitty_graphics.rs. That path only serves the monolithic--no-sessionmode; in the default client/server mode the value that reaches rendering comes
from
current_terminal_geometryinsrc/client/mod.rs, which had the samezero-pixel blind spot. Same problem and same goal as the issue, different
location — this PR fixes the client path. The monolithic path is left as is to
keep the change focused; happy to follow up if wanted.
Files to review (5, +305 / -27):
src/client/mod.rs(start here)src/raw_input.rsCSI 6 ; h ; w tparser and the newHostCellSizeReporteventsrc/app/runtime.rs,src/app/mod.rs,src/client/input.rsDesign notes
the stdin framer is armed the same one-shot way — at most one reply is ever
in flight
AtomicU64cache read by the resize poller; the poller'sbaseline starts from the handshake values, so the first report reliably
triggers a resize message
review; so is the monolithic path
should_query_host_terminal_themeVerification
just ci(fmt + clippy + nextest) andjust windows-lint— clean; failingtests on the dev box are identical on a clean master worktree, so no
regressions from this change
sharp on attach; cell size 8x16 → 10x21
refs #2146