Skip to content

fix: Windows sub-agent completion halves TUI render width#2708

Open
jrcjrcc wants to merge 1 commit into
Hmbown:mainfrom
jrcjrcc:windows-half-width-fix
Open

fix: Windows sub-agent completion halves TUI render width#2708
jrcjrcc wants to merge 1 commit into
Hmbown:mainfrom
jrcjrcc:windows-half-width-fix

Conversation

@jrcjrcc
Copy link
Copy Markdown

@jrcjrcc jrcjrcc commented Jun 3, 2026

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 after re-entering alt-screen
  • 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() + size() fallback
  • tests: 3 unit tests for size() fallback chain

Summary

Testing

  • cargo fmt --all -- --check
  • cargo clippy --workspace --all-targets --all-features
  • cargo test --workspace --all-features

Checklist

  • Updated docs or comments as needed
  • Added or updated tests where relevant
  • Verified TUI behavior manually if UI changes

Greptile Summary

This PR fixes Windows TUI render width halving caused by a spurious second EnterAlternateScreen emitted by AgentComplete when the terminal was never paused, and by ColorCompatBackend falling through to the WinAPI buffer width instead of the real window width.

  • ui.rs: Guards resume_terminal() behind event_broker.is_paused() to prevent the double alt-screen entry; caches crossterm::terminal::size() into the backend before reset_terminal_viewport so ratatui's autoresize sees the correct width; also propagates the resize-event size into terminal_size alongside forced_size.
  • color_compat.rs: Adds terminal_size field with set_terminal_size() accessor and a three-tier size() priority chain (forced_size > terminal_size > live crossterm query), with three unit tests covering the fallback logic.
  • subagent_routing.rs: Replaces mark_history_updated() calls with targeted bump_history_cell(idx), but the if 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

Filename Overview
crates/tui/src/tui/subagent_routing.rs Replaces mark_history_updated with bump_history_cell in delegate/ChildSpawned paths, but the if-is_fanout block never calls bump_history_cell or sets needs_redraw, dropping the redraw signal for all fanout card mutations.
crates/tui/src/tui/color_compat.rs Adds terminal_size cache field with set_terminal_size() and three-tier priority in size(): forced_size > terminal_size > live crossterm query. Logic and tests are correct.
crates/tui/src/tui/ui.rs Adds event_broker.is_paused() guard to prevent double EnterAlternateScreen; caches terminal size before reset_terminal_viewport in resume_terminal(); also sets terminal_size alongside forced_size in the resize handler.
crates/tui/src/tools/shell.rs Removes a redundant cast from as_raw_handle() to *mut c_void since RawHandle is already that type on Windows.

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 ✓
Loading

Comments Outside Diff (1)

  1. crates/tui/src/tui/subagent_routing.rs, line 149-168 (link)

    P1 Fanout branch silently drops the redraw signal

    Before this PR, app.mark_history_updated() was called unconditionally at the end of the function, covering every code path that didn't return early — including both fanout sub-paths. The PR replaces it with app.bump_history_cell(idx) inside only the else (delegate) branch. The entire if is_fanout { … } block exits without calling either bump_history_cell or mark_history_updated, so needs_redraw is never set to true.

    Concretely:

    • "Reuse existing fanout card" path (line 156–157): mutates the card via claim_pending_worker but neither increments history_version nor sets needs_redraw. The updated card is invisible until the next unrelated event forces a redraw.
    • "Create new fanout card" path (line 159–167): add_message does increment history_version, but it does not set needs_redraw = true (see add_message impl in app.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.

    Fix in Codex Fix in Claude Code Fix in Cursor

Fix All in Codex Fix All in Claude Code Fix All in Cursor

Reviews (3): Last reviewed commit: "fix: Windows sub-agent completion halves..." | Re-trigger Greptile

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 3, 2026

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 .github/APPROVED_CONTRIBUTORS will be closed automatically.

Please read CONTRIBUTING.md for the expected contribution shape. A maintainer can grant PR access by commenting /lgtm on a pull request.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread crates/tui/src/tui/color_compat.rs Outdated
match self.forced_size {
Some(size) => Ok(size),
None => self.inner.size(),
if let Some(size) = self.terminal_size.or(self.forced_size) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Suggested change
if let Some(size) = self.terminal_size.or(self.forced_size) {
if let Some(size) = self.forced_size.or(self.terminal_size) {

Comment thread crates/tui/src/tui/subagent_routing.rs Outdated
Comment on lines +132 to +135
let idx = app.subagent_card_index.get(&agent_id).copied();
if let Some(idx) = idx {
app.bump_history_cell(idx);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
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);

Comment thread crates/tui/src/tui/subagent_routing.rs Outdated
Comment on lines +178 to +181
match app.subagent_card_index.get(&agent_id) {
Some(&idx) => app.bump_history_cell(idx),
None => app.mark_history_updated(),
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This map lookup is redundant. idx was just inserted into app.subagent_card_index on line 174 and is already in scope. You can call app.bump_history_cell(idx) directly without re-querying the map.

        app.bump_history_cell(idx);

Comment thread crates/tui/src/tui/ui.rs Outdated
Comment thread crates/tui/src/tui/subagent_routing.rs
Comment thread crates/tui/src/tui/subagent_routing.rs Outdated
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)
@jrcjrcc jrcjrcc force-pushed the windows-half-width-fix branch from 4463c46 to f8387d6 Compare June 4, 2026 05:51
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.

1 participant