TUI fixups#203
Merged
Merged
Conversation
Signed-off-by: Dobes Vandermeer <dobes.vandermeer@newsela.com>
Contributor
📝 WalkthroughWalkthroughAdded editor hooks and terminal panic-guard; migrated textarea and crossterm deps; preserved visible input when queuing messages; made input-height wrapping-aware; introduced REPL CommandOutcome to refine command control flow. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TUI
participant App
participant Repl
participant Editor
User->>TUI: Press Enter (LLM busy)
TUI->>App: queue pending_message (keep app.input visible)
Note right of App: pending_message stored, input unchanged
alt pending_message processed
App->>TUI: take pending_message (clear app.input)
TUI->>TUI: push user transcript
alt message starts with "."
TUI->>Repl: run_repl_command -> CommandOutcome
else
TUI->>Repl: start_prompt
end
end
User->>TUI: Open external editor
TUI->>App: edit_with_tui_hooks(before, edit_op, after)
App->>Editor: tui_before_editor (cleanup terminal / leave alt screen)
Editor-->>App: editor closed
App->>Editor: tui_after_editor (re-enable alternate screen / raw mode)
sequenceDiagram
participant TuiSetup
participant Terminal
participant PanicGuard
participant Cleanup
TuiSetup->>Terminal: enable raw mode, alt screen, push keyboard flags
TuiSetup->>PanicGuard: install PanicTerminalHookGuard (wrap panic hook)
TuiSetup->>Terminal: run event loop
alt panic occurs
PanicGuard->>Cleanup: cleanup_terminal_state()
Cleanup->>Terminal: disable raw mode, pop keyboard flags, disable mouse, leave alt screen
else normal exit
TuiSetup->>Cleanup: restore_terminal() -> cleanup_terminal_state()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/tui/terminal.rs (1)
59-73:⚠️ Potential issue | 🟠 MajorConvert to async or use blocking wrapper for I/O operations.
The
setup_terminal()function performs synchronous I/O operations (enable_raw_mode, execute, Terminal::new) but is called directly from an async context inlifecycle.rs. According to coding guidelines, all I/O operations must useasync fnand.awaitvia the Tokio runtime. Wrap the blocking operations withtokio::task::block_in_place()orspawn_blocking()to prevent blocking the async executor.The keyboard enhancement flags themselves are correctly configured—the conditional check and flag combinations are appropriate—but the synchronous I/O handling must be addressed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tui/terminal.rs` around lines 59 - 73, The setup_terminal function performs blocking I/O but is invoked from async code; make it safe by converting it to async fn setup_terminal() -> Result<Terminal<CrosstermBackend<Stdout>>> and run the existing synchronous body inside tokio::task::block_in_place (or use tokio::task::spawn_blocking if you prefer returning a JoinHandle) so the blocking calls (enable_raw_mode, stdout.execute, Terminal::new) run off the async executor; keep the current logic and flags inside the closure and return the constructed Terminal from that closure.
🧹 Nitpick comments (1)
Cargo.toml (1)
82-82: Use crates.io release instead of tracking moving branch.
ratatui-textareaversion0.8.0is available on crates.io and is the latest stable release. Trackingbranch = "main"makes the dependency non-reproducible across lockfile regenerations; switch to the published crate.📦 Suggested manifest change
-ratatui-textarea = { git = "https://github.com/ratatui/ratatui-textarea", branch = "main", version = "0.8.0" } +ratatui-textarea = "0.8.0"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Cargo.toml` at line 82, The Cargo.toml currently pulls ratatui-textarea from a git branch which makes builds non-reproducible; update the dependency declaration for ratatui-textarea to use the published crates.io version (remove git = ... and branch = ... and set version = "0.8.0") so Cargo pulls the released crate instead of tracking main, ensuring reproducible lockfiles and builds.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/config/mod.rs`:
- Around line 1385-1392: Create a shared helper method edit_with_tui_hooks that
suspends/restores the TUI around editor invocations (mirroring the behavior
currently in edit_session which wraps edit_file) and use it everywhere edit_file
is currently called directly: replace edit_file() calls in edit_config,
upsert_agent, and new_macro with self.edit_with_tui_hooks(...), and for the
async edit_rag_docs create an async variant or awaitable wrapper so it also runs
under the same before/after hooks; ensure the helper accepts a closure or async
block to invoke edit_file (and preserves signatures/return values) and keep the
existing set_tui_editor_hooks, tui_before_editor, and tui_after_editor fields as
the hook sources.
In `@src/tui/input.rs`:
- Around line 68-72: Queued dot-commands placed into self.app.pending_message
while self.app.llm_busy are later replayed via start_prompt, causing commands
like .info/.set to be sent to the model instead of executed; update the replay
logic in the Finished and Errored arms (and the similar blocks at the other
occurrences) to check pending_message and if it starts with a dot command invoke
run_repl_command(pending) instead of start_prompt(pending), then clear
pending_message and refresh_input_chrome() as currently done so the command is
executed locally rather than sent to the LLM.
In `@src/tui/lifecycle.rs`:
- Around line 170-172: install_external_editor_bridge currently disables the
editor hooks by calling config.write().set_tui_editor_hooks(None, None) and
ignores the provided Terminal, which prevents edit_session from
suspending/restoring terminal state; instead, set tui_before_editor and
tui_after_editor callbacks via
config.write().set_tui_editor_hooks(Some(before_cb), Some(after_cb)) where
before_cb captures a clone or weak ref to the Terminal/CrosstermBackend and
calls the appropriate suspend/cleanup methods (e.g., disable raw mode, leave
alternate screen, show cursor) and after_cb restores terminal state (re-enable
raw mode, enter alternate screen, hide cursor) so edit_session can safely launch
the external editor; replace the unused _terminal parameter by using it (or a
clone) inside these callbacks and reference install_external_editor_bridge,
edit_session, and set_tui_editor_hooks when making the change.
In `@src/tui/render.rs`:
- Around line 186-206: The input_height() math can overflow when narrowing to
u16; change the accumulator so body_lines is computed as a usize (not u16) while
wrapping lines using body_width, then when returning convert safely by clamping
the final value to u16::MAX before adding the extra padding: compute total =
min(body_lines, (u16::MAX as usize).saturating_sub(2)) + 2 and return total as
u16. Update references in input_height (variables body_lines, wrapped,
body_width, last_known_input_width, lines) so all intermediate arithmetic stays
in usize and only the final value is narrowed with a safe clamp.
In `@src/tui/terminal.rs`:
- Around line 20-33: The panic hook capture/restore in
PanicTerminalHookGuard::install is incorrect because multiple panic::take_hook()
calls remove and lose the original hook; change the logic to take the original
hook once, wrap it in a shared owner (e.g., Arc<dyn
Fn(&panic::PanicHookInfo<'_>) + Sync + Send + 'static>) and clone that Arc into
the new hook closure so the closure can call the original hook after calling
cleanup_terminal_state(), then store that Arc in self.previous_hook for Drop to
restore the original hook; update the type of previous_hook and the install
function to use the Arc-wrapped hook and ensure panic::set_hook is only called
once with the new chained closure that calls the Arc-cloned original.
- Line 54: The code references a non-existent WrapMode and set_wrap_mode() on
the TextArea/input: remove the WrapMode import from the ratatui_textarea import
list (the Input as TextInput, Key, TextArea import) and delete the call
input.set_wrap_mode(WrapMode::WordOrGlyph) in terminal.rs; alternatively, if you
intend to use configurable wrap modes, upgrade the ratatui-textarea dependency
in Cargo.toml to a version that exposes WrapMode and set_wrap_mode(), then
update imports to match that newer API (ensuring the symbol names
Input/TextArea/WrapMode match the upgraded crate).
---
Outside diff comments:
In `@src/tui/terminal.rs`:
- Around line 59-73: The setup_terminal function performs blocking I/O but is
invoked from async code; make it safe by converting it to async fn
setup_terminal() -> Result<Terminal<CrosstermBackend<Stdout>>> and run the
existing synchronous body inside tokio::task::block_in_place (or use
tokio::task::spawn_blocking if you prefer returning a JoinHandle) so the
blocking calls (enable_raw_mode, stdout.execute, Terminal::new) run off the
async executor; keep the current logic and flags inside the closure and return
the constructed Terminal from that closure.
---
Nitpick comments:
In `@Cargo.toml`:
- Line 82: The Cargo.toml currently pulls ratatui-textarea from a git branch
which makes builds non-reproducible; update the dependency declaration for
ratatui-textarea to use the published crates.io version (remove git = ... and
branch = ... and set version = "0.8.0") so Cargo pulls the released crate
instead of tracking main, ensuring reproducible lockfiles and builds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7550aa23-b0dc-4d57-a90d-646e9f163a7b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
Cargo.tomlsrc/config/mod.rssrc/repl/completer.rssrc/tui/input.rssrc/tui/lifecycle.rssrc/tui/mod.rssrc/tui/render.rssrc/tui/terminal.rssrc/tui/tests.rssrc/tui/types.rs
Comment on lines
68
to
72
| if self.app.llm_busy { | ||
| // Queue the message to send when LLM finishes | ||
| // Keep the text in input so user can see/edit it | ||
| self.app.pending_message = Some(text); | ||
| self.app.input = Self::new_input(); | ||
| self.refresh_input_chrome(); |
Contributor
There was a problem hiding this comment.
Queued dot-commands are still replayed as chat prompts.
When Enter is pressed while llm_busy, .info, .set, etc. are stored in pending_message. Both replay paths then call start_prompt(pending), so the queued command goes to the model instead of run_repl_command(...).
🪄 Suggested fix
+ async fn replay_pending_message(&mut self, pending: String) -> Result<()> {
+ self.app.input = Self::new_input();
+ self.app
+ .transcript
+ .push(TranscriptEntry::User(pending.clone()));
+ self.pin_transcript_to_bottom();
+ if pending.trim_start().starts_with('.') {
+ self.run_repl_command(&pending).await?;
+ self.refresh_input_chrome();
+ } else {
+ self.start_prompt(pending).await?;
+ }
+ Ok(())
+ }
+
...
- if let Some(pending) = self.app.pending_message.take() {
- // Clear input now that the pending message is actually being submitted
- self.app.input = Self::new_input();
- self.app
- .transcript
- .push(TranscriptEntry::User(pending.clone()));
- self.pin_transcript_to_bottom();
- self.start_prompt(pending).await?;
- }
+ if let Some(pending) = self.app.pending_message.take() {
+ self.replay_pending_message(pending).await?;
+ }Apply the same replacement in both the Finished and Errored arms.
Also applies to: 197-205, 214-221
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tui/input.rs` around lines 68 - 72, Queued dot-commands placed into
self.app.pending_message while self.app.llm_busy are later replayed via
start_prompt, causing commands like .info/.set to be sent to the model instead
of executed; update the replay logic in the Finished and Errored arms (and the
similar blocks at the other occurrences) to check pending_message and if it
starts with a dot command invoke run_repl_command(pending) instead of
start_prompt(pending), then clear pending_message and refresh_input_chrome() as
currently done so the command is executed locally rather than sent to the LLM.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/config/mod.rs`:
- Around line 1673-1678: The call to edit_file() inside the async
edit_rag_docs() blocks the Tokio worker; replace the direct blocking edit_file()
call in the config.write().edit_with_tui_hooks closure with an async wrapper
that spawns a dedicated OS thread (std::thread::spawn) to run edit_file(editor,
path) and use a tokio::sync::oneshot channel to notify the async context when
the editor exits. Specifically, change the code around edit_rag_docs() /
edit_with_tui_hooks / editor() so the closure captures editor and temp_file
(converted to owned types) and sends completion via the oneshot sender from the
spawned thread, then await the oneshot receiver in the async function instead of
calling edit_file() directly.
In `@src/repl/mod.rs`:
- Around line 273-276: The REPL loop is being starved because
process_pending_async_resume() returns CommandOutcome::Continue both for
"nothing to resume" and for "resume completed", and the current matches(...) {
continue; } prevents read_line() from ever running; change the logic in the loop
around process_pending_async_resume() (the call to
process_pending_async_resume().await? and the matches on
CommandOutcome::Continue) to use a distinct "resume_handled" signal (or restore
the previous bool) that only causes continue when a resume was actually
processed, and explicitly handle CommandOutcome::Exit (do not let it fall
through to read_line()); apply the same fix to the other occurrence that mirrors
this logic (the block around lines 319-353) so exit and normal read_line() paths
behave correctly.
In `@src/tui/render.rs`:
- Line 95: The height calculation in input_height() is using
self.app.last_known_input_width which is only updated later in draw(), causing
incorrect clamping on first render; update input_height() (and the similar logic
around the other occurrences noted) to use the current frame width (size.width
or the equivalent local variable passed into draw()) instead of
last_known_input_width, and keep last_known_input_width only as a stored value
updated after layout (e.g., in draw()) so the immediate height measurement uses
the current frame's width.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 10ed629f-eafd-49e4-b661-0d882b444844
📒 Files selected for processing (7)
src/config/mod.rssrc/repl/mod.rssrc/tui/input.rssrc/tui/lifecycle.rssrc/tui/mod.rssrc/tui/render.rssrc/tui/terminal.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/tui/input.rs
- src/tui/mod.rs
- src/tui/terminal.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/config/mod.rs`:
- Around line 1673-1687: The problem is the closure passed to
edit_with_tui_hooks spawns the editor thread and returns immediately, allowing
the after-hook to run before the editor exits; fix by making the editor run
synchronously inside the closure instead of spawning a thread: in the block
where you call config_write.edit_with_tui_hooks(|_| { ... }), remove
std::thread::spawn and call edit_file(&editor, &temp_file_path) directly, send
its Result over the oneshot tx, then return Ok(()) so the closure only returns
after edit_file completes (this ensures before()/after() brackets the full
editor lifetime).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 452e7784-5480-4703-943c-1d6d4f11ab0f
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
Cargo.tomlsrc/config/mod.rssrc/repl/mod.rssrc/tui/render.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- Cargo.toml
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/config/mod.rs (1)
1673-1685: The oneshot channel is now unnecessary.Since
edit_fileruns synchronously within the closure (nostd::thread::spawn), the channel adds complexity without benefit. The result is sent immediately and awaited right after the lock is released.♻️ Suggested simplification
- let (tx, rx) = tokio::sync::oneshot::channel(); - { - let mut config_write = config.write(); - let editor = config_write.editor()?; - let temp_file_path = temp_file.clone(); - config_write.edit_with_tui_hooks(|_| { - let result = edit_file(&editor, &temp_file_path); - let _ = tx.send(result); - Ok(()) - })?; - } - rx.await - .map_err(|_| anyhow!("Editor hook completion channel unexpectedly closed"))??; + { + let mut config_write = config.write(); + let editor = config_write.editor()?; + let temp_file_path = temp_file.clone(); + config_write.edit_with_tui_hooks(|_| { + edit_file(&editor, &temp_file_path) + })?; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/mod.rs` around lines 1673 - 1685, The oneshot channel (tx/rx) is unnecessary because edit_file is called synchronously inside the edit_with_tui_hooks closure; remove the tokio::sync::oneshot::channel creation and usage, call edit_file(&editor, &temp_file) directly inside the closure (or capture its result and return it via the closure's Result), propagate its Result out of edit_with_tui_hooks and handle errors the same way as before (keep the config.write()/editor()/edit_with_tui_hooks sequence but return the edit_file result directly instead of sending it through tx and awaiting rx).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/config/mod.rs`:
- Around line 1673-1685: The oneshot channel (tx/rx) is unnecessary because
edit_file is called synchronously inside the edit_with_tui_hooks closure; remove
the tokio::sync::oneshot::channel creation and usage, call edit_file(&editor,
&temp_file) directly inside the closure (or capture its result and return it via
the closure's Result), propagate its Result out of edit_with_tui_hooks and
handle errors the same way as before (keep the
config.write()/editor()/edit_with_tui_hooks sequence but return the edit_file
result directly instead of sending it through tx and awaiting rx).
This was referenced Apr 8, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Tests