feat(tui): improve terminal input and frame rendering - #885
Conversation
|
This PR changes mirrored Maestro source files in the public repo, but it does not link the matching private source-of-truth PR. Add one of these to the PR body, then re-run the check:
Mirrored files touched:
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6b04c4261
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let mut chars = text.chars(); | ||
| match (chars.next(), chars.next()) { | ||
| (Some(character), None) if !character.is_control() => KeyCode::Char(character), | ||
| _ => convert_uncurses_key_code(key.code)?, | ||
| } |
There was a problem hiding this comment.
When a Kitty/uncurses key event carries multiple Unicode scalars in key.text—for example, composed IME input or a grapheme containing combining characters—this branch rejects the text and falls back to the single logical key.code, silently dropping the remaining characters. The protocol-aware reader should enqueue or insert the entire associated text rather than collapsing it to one KeyCode::Char.
Useful? React with 👍 / 👎.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Two terminal-protocol owners active simultaneously (crossterm setup + uncurses reader)
terminal::init still unconditionally enables bracketed paste, mouse capture, focus change, and pushes keyboard-enhancement flags on the global tty handle (packages/tui-rs/src/terminal/setup.rs:213-231). When TerminalEventReader::open succeeds, uncurses opens a separate tty handle and enables its own protocol modes, so both stacks are active at once. The PR's stated design is "one owner for terminal input." Functionally this is likely fine because uncurses is a full protocol parser and its enables are applied last, and cleanup double-resets harmlessly. However, it's worth confirming there's no mode that crossterm enables at a level uncurses doesn't expect (which could theoretically cause reply/protocol bytes to be surfaced as key input — the exact failure this PR aims to prevent), and that the drop-order (uncurses reader take() then terminal::restore()) leaves the shell fully reset in all paths.
(Refers to lines 213-231)
Was this helpful? React with 👍 or 👎 to provide feedback.
| fn poll_terminal_event(&mut self, timeout: Duration) -> Result<Option<AppTerminalEvent>> { | ||
| if let Some(reader) = &mut self.terminal_events { | ||
| return reader.poll(timeout).map_err(Into::into); | ||
| } | ||
| if event::poll(timeout)? { | ||
| return Ok(AppTerminalEvent::from_crossterm(event::read()?)); | ||
| } | ||
| Ok(None) | ||
| } |
There was a problem hiding this comment.
🔍 uncurses poll error propagation may crash the TUI on EINTR from background processes
poll_terminal_event returns reader.poll(timeout).map_err(Into::into) (packages/tui-rs/src/app.rs:1035-1043), and any Err from the uncurses reader propagates up through run_inner and terminates the loop. This TUI routinely spawns background bash/tool child processes, whose termination raises SIGCHLD. If uncurses's underlying poll() surfaces EINTR (rather than retrying internally, as crossterm's mio-based reader does), a benign signal could cause the reader to return an error and take down the whole TUI. The previous crossterm path (event::poll) was robust to EINTR. Worth confirming that uncurses::event::EventSource::poll/try_read retry on EINTR before relying on this in production.
Was this helpful? React with 👍 or 👎 to provide feedback.
| Some(MouseEvent { | ||
| kind, | ||
| column: mouse.x, | ||
| row: mouse.y, | ||
| modifiers: convert_uncurses_modifiers(mouse.modifiers), | ||
| }) |
There was a problem hiding this comment.
🔍 Mouse coordinate convention between uncurses and crossterm not verified
convert_uncurses_mouse copies mouse.x/mouse.y directly into crossterm's MouseEvent.column/row (packages/tui-rs/src/terminal/events.rs:387-392), which the slash-popup hit test in app.rs compares against 0-based ratatui Rect coordinates and uses in mouse.row - popup.y - 1 for row indexing. crossterm normalizes native 1-based terminal mouse reports to 0-based. If uncurses reports raw 1-based coordinates, click-to-select on the completion popup would be off by one row/column (selecting the wrong item), though the in_y guard prevents an underflow panic. Worth verifying uncurses's coordinate origin matches crossterm's 0-based convention.
Was this helpful? React with 👍 or 👎 to provide feedback.
| let next = match event { | ||
| AppTerminalEvent::ColorScheme(scheme) => { | ||
| let next = match scheme { | ||
| uncurses::event::ColorScheme::Light => "light", | ||
| uncurses::event::ColorScheme::Dark => "dark", | ||
| }; | ||
| self.theme_follower = Some(crate::themes::osc11::AutoThemeFollower::new(next)); | ||
| Some(next) | ||
| } | ||
| AppTerminalEvent::BackgroundColor { red, green, blue } => { | ||
| let luminance = crate::themes::osc11::relative_luminance(*red, *green, *blue); | ||
| self.theme_follower | ||
| .as_mut() | ||
| .and_then(|follower| follower.observe_luminance(luminance)) | ||
| } | ||
| _ => None, | ||
| }; | ||
| let Some(next) = next else { | ||
| return false; | ||
| }; | ||
| if crate::themes::current_theme_name() == next { | ||
| return false; | ||
| } | ||
| if crate::themes::set_theme_by_name(next).is_ok() { | ||
| return true; | ||
| } | ||
| false | ||
| } |
There was a problem hiding this comment.
📝 Info: ColorScheme events reset the hysteresis follower, discarding accumulated background-luminance readings
In apply_terminal_theme_event, each ColorScheme event replaces the follower with AutoThemeFollower::new(next) (packages/tui-rs/src/app.rs:1070), discarding any consecutive-reading state accumulated from BackgroundColor samples. Because color-scheme notifications are authoritative and applied immediately, this is intentional and correct. One minor edge case: if set_theme_by_name(next) were to fail, the follower's current would be left desynced from the actual applied theme; this cannot happen for the built-in "light"/"dark" names but would matter if the mapping ever pointed at a custom theme.
Was this helpful? React with 👍 or 👎 to provide feedback.
| fn initialize_terminal_events(&mut self) { | ||
| if uncurses_input_enabled(std::env::var_os("MAESTRO_UNCURSES_INPUT").as_deref()) { | ||
| self.terminal_events = TerminalEventReader::open().ok(); | ||
| } | ||
|
|
||
| if self.theme_follower.is_some() { | ||
| if self.terminal_events.is_some() { | ||
| // Mode 2031 requests push notifications when the terminal's | ||
| // color scheme changes. OSC 11 covers terminals that only | ||
| // expose their actual background color. | ||
| let _ = terminal::enable_theme_reporting(); | ||
| self.last_theme_query = Some(Instant::now()); | ||
| } else if self.capabilities.enhanced_keys { | ||
| // Compatibility path for terminals where uncurses cannot open | ||
| // the controlling tty. | ||
| crate::themes::osc11::apply_auto_theme_from_terminal(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
📝 Info: Theme-following only activates once an event/reply arrives; no synchronous startup apply in the uncurses path
Previously, when theme_follow was on and the terminal was enhanced, apply_auto_theme_from_terminal() ran synchronously at construction, so the theme was resolved before the first frame. In the new uncurses path, the follower is created but the theme is only applied after enable_theme_reporting's query round-trips and a ColorScheme/BackgroundColor event is decoded (packages/tui-rs/src/app.rs:1020-1032). This introduces a brief startup window where the theme is the default rather than the terminal-derived one. This is consistent with the PR's live-following goal and not a correctness bug, but is a visible behavior change from the prior one-shot probe.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Superseded by the canonical source-of-truth flow: evalops/maestro-internal#3182 is merged, and generated public mirror PR #886 carries the reviewed change plus provenance metadata. |
1 participant
Summary
MAESTRO_UNCURSES_INPUT=0is an immediate kill switch.Checklist
cargo fmt --all -- --checkand package clippycargo test -p maestro-tui --lib -- --test-threads=1(3,477 passed)cargo check -p maestro-tui --all-targetsOptional
run-evalslabel to run evals on this PR (otherwise evals are skipped on PRs).skip-integrationlabel to skip integration tests when appropriate (include justification in the PR body).[skip ci],[skip nix]), explain why below.Post-Deploy Monitoring & Validation
theme_followis off, partial frames, or a shell left in raw mode as rollback signals.MAESTRO_UNCURSES_INPUT=0. Revert this change if reports are widespread or the fallback does not restore normal behavior.New concepts
One owner for terminal input
Terminal replies and user keystrokes share the same byte stream. Letting two parsers read that stream creates a race: whichever reader wins consumes bytes the other can never recover.
This change gives uncurses sole ownership of the controlling terminal input and translates its typed events into Maestro's existing crossterm-shaped application events. That preserves the current widget and keybinding layers while allowing OSC and DEC replies to coexist safely with ordinary input.
Use this pattern when an application both queries terminal capabilities and accepts interactive input. It is unnecessary for simple line-oriented commands that never request terminal replies.