fix: Windows sub-agent completion halves TUI render width#2708
Conversation
|
Thanks @jrcjrcc for taking the time to contribute. This repository is currently observing a maintainer-managed contribution gate in dry-run mode, so this pull request is staying open. When enforcement is enabled, pull requests from contributors who are not listed in Please read |
There was a problem hiding this comment.
Code Review
This pull request introduces terminal size caching in the TUI backend to prevent stale buffer dimensions (particularly on Windows after re-entering the alternate screen) and refactors subagent mailbox handling to update specific history cells instead of marking the entire history as updated. The review feedback highlights a critical issue where the newly introduced terminal_size incorrectly takes precedence over forced_size, which would break viewport resizing behavior. Additionally, the reviewer identified redundant map lookups for idx in subagent_routing.rs that can be simplified since the index is already in scope.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| match self.forced_size { | ||
| Some(size) => Ok(size), | ||
| None => self.inner.size(), | ||
| if let Some(size) = self.terminal_size.or(self.forced_size) { |
There was a problem hiding this comment.
The forced_size option is designed to temporarily override the terminal size during resize events to prevent viewport flickering/shrinking. By prioritizing terminal_size over forced_size (self.terminal_size.or(self.forced_size)), the forced size will be ignored once terminal_size is cached. Swap the order so that forced_size takes precedence. Note that the corresponding unit test size_prefers_terminal_size_over_forced_size should also be updated to reflect this correct priority.
| if let Some(size) = self.terminal_size.or(self.forced_size) { | |
| if let Some(size) = self.forced_size.or(self.terminal_size) { |
| let idx = app.subagent_card_index.get(&agent_id).copied(); | ||
| if let Some(idx) = idx { | ||
| app.bump_history_cell(idx); | ||
| } |
There was a problem hiding this comment.
This map lookup is redundant. idx is already retrieved and in scope from the outer if let Some(&idx) = app.subagent_card_index.get(&agent_id) check on line 121. You can call app.bump_history_cell(idx) directly.
| let idx = app.subagent_card_index.get(&agent_id).copied(); | |
| if let Some(idx) = idx { | |
| app.bump_history_cell(idx); | |
| } | |
| app.bump_history_cell(idx); |
| match app.subagent_card_index.get(&agent_id) { | ||
| Some(&idx) => app.bump_history_cell(idx), | ||
| None => app.mark_history_updated(), | ||
| } |
883b814 to
4463c46
Compare
Root cause: AgentComplete unconditionally calls resume_terminal() even when the terminal was never paused, causing a secondary EnterAlternateScreen on Windows that creates a new buffer whose width may differ from the window width. Additionally, ColorCompatBackend had no terminal_size cache, so size() fell through to crossterm::terminal::size() which on Windows returns the WinAPI buffer width rather than the window width. Changes: - AgentComplete: add event_broker.is_paused() guard - resume_terminal(): cache real terminal size before reset_viewport - Resize handler: also set terminal_size alongside forced_size - subagent_routing: 3x mark_history_updated -> bump_history_cell(idx) - color_compat: add terminal_size field, set_terminal_size(), fix size() fallback priority (forced_size > terminal_size) - tests: 3 unit tests for size() fallback chain Review feedback addressed: - forced_size now takes priority over terminal_size (gemini-code-assist) - Redundant map lookups removed in subagent_routing (both bots) - set_terminal_size moved before reset_terminal_viewport (greptile-apps)
4463c46 to
f8387d6
Compare
Root cause: AgentComplete unconditionally calls resume_terminal() even when the terminal was never paused, causing a secondary EnterAlternateScreen on Windows that creates a new buffer whose width may differ from the window width. Additionally, ColorCompatBackend had no terminal_size cache, so size() fell through to crossterm::terminal::size() which on Windows returns the WinAPI buffer width rather than the window width.
Changes:
Summary
Testing
cargo fmt --all -- --checkcargo clippy --workspace --all-targets --all-featurescargo test --workspace --all-featuresChecklist
Greptile Summary
This PR fixes Windows TUI render width halving caused by a spurious second
EnterAlternateScreenemitted byAgentCompletewhen the terminal was never paused, and byColorCompatBackendfalling through to the WinAPI buffer width instead of the real window width.ui.rs: Guardsresume_terminal()behindevent_broker.is_paused()to prevent the double alt-screen entry; cachescrossterm::terminal::size()into the backend beforereset_terminal_viewportso ratatui'sautoresizesees the correct width; also propagates the resize-event size intoterminal_sizealongsideforced_size.color_compat.rs: Addsterminal_sizefield withset_terminal_size()accessor and a three-tiersize()priority chain (forced_size>terminal_size> live crossterm query), with three unit tests covering the fallback logic.subagent_routing.rs: Replacesmark_history_updated()calls with targetedbump_history_cell(idx), but theif is_fanout { … }block is missing the redraw signal on both sub-paths (see inline comment).Confidence Score: 4/5
Safe to merge for the Windows width fix; the fanout card mutation path will not trigger a repaint until an unrelated event fires.
The terminal size caching and is_paused guard are correct. However, removing the unconditional mark_history_updated at the end of handle_subagent_mailbox left both fanout sub-paths without a needs_redraw signal: card mutations via claim_pending_worker become invisible until the next unrelated event, and the add_message path (confirmed to not set needs_redraw) is similarly affected.
crates/tui/src/tui/subagent_routing.rs — the if is_fanout block needs bump_history_cell calls in both its sub-paths.
Important Files Changed
Sequence Diagram
sequenceDiagram participant EB as EventBroker participant UI as run_event_loop participant RT as resume_terminal() participant BE as ColorCompatBackend participant CT as crossterm::terminal Note over UI: AgentComplete received UI->>EB: is_paused()? alt terminal is paused EB-->>UI: true UI->>RT: resume_terminal() RT->>CT: terminal::size() CT-->>RT: (cols, rows) RT->>BE: set_terminal_size(cols, rows) RT->>BE: reset_terminal_viewport → clear() → autoresize() BE->>BE: size() → terminal_size cache hit ✓ else terminal was never paused EB-->>UI: false Note over UI: skip resume_terminal() (no double EnterAlternateScreen) end Note over UI: Resize event UI->>BE: force_size(new_size) UI->>BE: set_terminal_size(new_size) UI->>UI: draw_app_frame_inner() UI->>BE: clear_forced_size() BE->>BE: size() → terminal_size cache hit ✓Comments Outside Diff (1)
crates/tui/src/tui/subagent_routing.rs, line 149-168 (link)Before this PR,
app.mark_history_updated()was called unconditionally at the end of the function, covering every code path that didn'treturnearly — including both fanout sub-paths. The PR replaces it withapp.bump_history_cell(idx)inside only theelse(delegate) branch. The entireif is_fanout { … }block exits without calling eitherbump_history_cellormark_history_updated, soneeds_redrawis never set totrue.Concretely:
claim_pending_workerbut neither incrementshistory_versionnor setsneeds_redraw. The updated card is invisible until the next unrelated event forces a redraw.add_messagedoes incrementhistory_version, but it does not setneeds_redraw = true(seeadd_messageimpl inapp.rs), so the render loop may not pick up the new cell.Both sub-paths need a
bump_history_cell(idx)call to restore the behaviour that was present before this change.Reviews (3): Last reviewed commit: "fix: Windows sub-agent completion halves..." | Re-trigger Greptile