feat: 5 UX improvements — bell, help, spinner, quit confirm, Ctrl+R#46
Merged
kienbui1995 merged 1 commit intomainfrom Apr 13, 2026
Merged
feat: 5 UX improvements — bell, help, spinner, quit confirm, Ctrl+R#46kienbui1995 merged 1 commit intomainfrom
kienbui1995 merged 1 commit intomainfrom
Conversation
1. Bell notification (\a) when agent finishes — alerts in other tabs 2. /help now categorized (Navigation, Session, Agent, Code, Tools, Workflow, Config) 3. Animated spinner (⠋⠙⠹...) during streaming and tool execution 4. /quit confirmation — warns about unsaved session, double-tap to exit 5. Ctrl+R reverse history search — type query then Ctrl+R to find match 197 tests, 0 warnings.
📝 WalkthroughWalkthroughThe changes introduce three new interactive features: Ctrl+R history search, animated spinner during streaming/tool execution, and double-tap quit confirmation with unsaved session detection. Additionally, the help command output was restructured into categorized sections, and a terminal bell notification was added for stream completion events. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mc/crates/mc-tui/src/app.rs (1)
295-304:⚠️ Potential issue | 🟡 MinorThis adds a second bell on every normal completion.
run_tui()already rings once afterUiMessage::Done, so Line 303 makes successful turns beep twice. Keep the bell in one place, or split normal-complete vs. cancel paths explicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mc/crates/mc-tui/src/app.rs` around lines 295 - 304, The AppEvent::StreamDone handler in App (match arm for AppEvent::StreamDone) is emitting a bell via eprint!("\x07") causing double beeps because run_tui() already rings on UiMessage::Done; remove the unconditional eprint from the AppEvent::StreamDone arm (or change it to only ring on cancellation paths) so only one bell is produced—locate the AppEvent::StreamDone match in mc-tui's App (app.rs) and delete or conditionally gate the eprint call to avoid duplicating the bell triggered by run_tui()/UiMessage::Done.
🧹 Nitpick comments (2)
mc/crates/mc-tui/src/app.rs (1)
506-511: Add#[must_use]tospinner_char.This is a new public value-returning method, and the crate convention is to mark those as
#[must_use]. Based on learnings: Add#[must_use]attribute on all public functions returning values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mc/crates/mc-tui/src/app.rs` around lines 506 - 511, Add the #[must_use] attribute to the public method spinner_char to follow crate convention for public value-returning functions; locate the pub fn spinner_char(&mut self) -> char in the file (it advances self.spinner_tick and returns from FRAMES) and prepend #[must_use] so callers are warned if the returned char is ignored.mc/crates/mc-tui/src/history.rs (1)
96-104: Add#[must_use]tosearch.This is a new public value-returning API, and the crate convention is to mark those as
#[must_use]. Based on learnings: Add#[must_use]attribute on all public functions returning values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mc/crates/mc-tui/src/history.rs` around lines 96 - 104, Add the #[must_use] attribute to the public method search so its returned Option<&str> is annotated; locate the pub fn search(&self, query: &str) -> Option<&str> declaration and place #[must_use] immediately above it (follow crate convention to add #[must_use] to public functions that return values).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mc/crates/mc-cli/src/main.rs`:
- Around line 631-643: The Ctrl+R handler currently reuses app.input as the
search query so after the first match the query becomes the full matched command
and subsequent Ctrl+R calls just re-find the same entry; modify the KeyEvent
branch handling KeyCode::Char('r') to capture the original query text (let
original_query = app.input.as_str().to_string()) and use a search cursor stored
on the app state (e.g., add a field like history_search_index or
history_search_matches on the App struct) to step through matches instead of
calling app.history.search(&app.input) repeatedly; implement logic so the first
Ctrl+R triggers a search (populate matches via
app.history.search_all(original_query) or similar), set cursor = 0 and set
app.input to matches[cursor], and subsequent Ctrl+R increments the cursor to
show the previous match (wrapping or stopping as desired), and reset the
cursor/matches when the user edits app.input.
In `@mc/crates/mc-tui/src/commands.rs`:
- Around line 19-34: Replace the hard-coded help text in the "/help" match arm
with generated output derived from the single source of truth
App::SLASH_COMMANDS: iterate App::SLASH_COMMANDS to build sections
(Navigation/Session/Agent/Code/Tools/Workflow/Config) and the tab-completion
list so both UI and completion always reflect the registry; ensure
App::SLASH_COMMANDS itself contains the missing entries (/branch, /summary,
/providers, /time, /rewind, /connect) and remove or stop referencing commands
that are not present in the registry (/plugin, /auto-test, /auto-commit,
/dry-run, /diff-preview), and update the completion handler to read from
App::SLASH_COMMANDS rather than a separate hardcoded list.
- Around line 35-44: App::submit_input currently drops all submissions when
app.state != AgentState::Idle which prevents the "/quit" double-tap path; change
submit logic to allow slash-commands that are safe (at least "/quit") to bypass
the non-idle gate by checking the raw input string and permitting "/quit" to be
processed even when state != Idle. In the "/quit" command handler (the match arm
handling "/quit"), flip the condition order so the pending_quit check is
evaluated first and can set should_quit = true on the second tap (e.g., if
app.pending_quit { app.should_quit = true } else { set pending_quit and push
warning }), and ensure that any non-"/quit" interaction clears app.pending_quit
(clear pending_quit in App::submit_input for normal inputs and in other command
handlers) so the pending_quit state doesn't persist incorrectly.
---
Outside diff comments:
In `@mc/crates/mc-tui/src/app.rs`:
- Around line 295-304: The AppEvent::StreamDone handler in App (match arm for
AppEvent::StreamDone) is emitting a bell via eprint!("\x07") causing double
beeps because run_tui() already rings on UiMessage::Done; remove the
unconditional eprint from the AppEvent::StreamDone arm (or change it to only
ring on cancellation paths) so only one bell is produced—locate the
AppEvent::StreamDone match in mc-tui's App (app.rs) and delete or conditionally
gate the eprint call to avoid duplicating the bell triggered by
run_tui()/UiMessage::Done.
---
Nitpick comments:
In `@mc/crates/mc-tui/src/app.rs`:
- Around line 506-511: Add the #[must_use] attribute to the public method
spinner_char to follow crate convention for public value-returning functions;
locate the pub fn spinner_char(&mut self) -> char in the file (it advances
self.spinner_tick and returns from FRAMES) and prepend #[must_use] so callers
are warned if the returned char is ignored.
In `@mc/crates/mc-tui/src/history.rs`:
- Around line 96-104: Add the #[must_use] attribute to the public method search
so its returned Option<&str> is annotated; locate the pub fn search(&self,
query: &str) -> Option<&str> declaration and place #[must_use] immediately above
it (follow crate convention to add #[must_use] to public functions that return
values).
🪄 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: 07219dac-f2d5-452c-b9e6-cbd22611b768
📒 Files selected for processing (5)
mc/crates/mc-cli/src/main.rsmc/crates/mc-tui/src/app.rsmc/crates/mc-tui/src/commands.rsmc/crates/mc-tui/src/history.rsmc/crates/mc-tui/src/ui.rs
Comment on lines
+631
to
+643
| event::KeyEvent { | ||
| code: KeyCode::Char('r'), | ||
| modifiers, | ||
| .. | ||
| } if modifiers.contains(KeyModifiers::CONTROL) => { | ||
| // Ctrl+R: reverse history search | ||
| let query = app.input.as_str().to_string(); | ||
| if !query.is_empty() { | ||
| if let Some(found) = app.history.search(&query) { | ||
| app.input.set(found); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Ctrl+R can't walk past the first match.
Line 637 reuses the current input buffer as the next query, so after the first hit the query becomes the full matched command and the next Ctrl+R re-finds the same history entry. Preserve the original search text plus a match index/cursor if you want repeated Ctrl+R to step backward through matches.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mc/crates/mc-cli/src/main.rs` around lines 631 - 643, The Ctrl+R handler
currently reuses app.input as the search query so after the first match the
query becomes the full matched command and subsequent Ctrl+R calls just re-find
the same entry; modify the KeyEvent branch handling KeyCode::Char('r') to
capture the original query text (let original_query =
app.input.as_str().to_string()) and use a search cursor stored on the app state
(e.g., add a field like history_search_index or history_search_matches on the
App struct) to step through matches instead of calling
app.history.search(&app.input) repeatedly; implement logic so the first Ctrl+R
triggers a search (populate matches via app.history.search_all(original_query)
or similar), set cursor = 0 and set app.input to matches[cursor], and subsequent
Ctrl+R increments the cursor to show the previous match (wrapping or stopping as
desired), and reset the cursor/matches when the user edits app.input.
Comment on lines
+19
to
+34
| "/help" => { | ||
| app.push("━━━ Navigation ━━━"); | ||
| app.push(" /help /quit /clear /status /version /whoami /theme /vim"); | ||
| app.push("━━━ Session ━━━"); | ||
| app.push(" /save /load /sessions /fork /branches /switch /history /search /export /resume"); | ||
| app.push("━━━ Agent ━━━"); | ||
| app.push(" /model /models /plan /thinking /effort /compact /tokens /context /cost /retry"); | ||
| app.push("━━━ Code ━━━"); | ||
| app.push(" /diff /log /commit /stash /undo /review /security-review /debug /template /spec"); | ||
| app.push("━━━ Tools ━━━"); | ||
| app.push(" /run /grep /tree /cat /head /tail /files /wc /pwd /env /open /size /image"); | ||
| app.push("━━━ Workflow ━━━"); | ||
| app.push(" /todo /test /ship /auto-test /auto-commit /dry-run /diff-preview /loop /cron"); | ||
| app.push("━━━ Config ━━━"); | ||
| app.push(" /config /permissions /init /add /memory /pin /alias /copy /agents /plugin"); | ||
| }, |
There was a problem hiding this comment.
/help, completion, and the command handler are already drifting.
This block omits implemented commands like /branch, /summary, /providers, /time, /rewind, and /connect, while entries exposed here such as /plugin, /auto-test, /auto-commit, /dry-run, and /diff-preview still are not in App::SLASH_COMMANDS. Please generate help text and tab-completion from one command registry.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mc/crates/mc-tui/src/commands.rs` around lines 19 - 34, Replace the
hard-coded help text in the "/help" match arm with generated output derived from
the single source of truth App::SLASH_COMMANDS: iterate App::SLASH_COMMANDS to
build sections (Navigation/Session/Agent/Code/Tools/Workflow/Config) and the
tab-completion list so both UI and completion always reflect the registry;
ensure App::SLASH_COMMANDS itself contains the missing entries (/branch,
/summary, /providers, /time, /rewind, /connect) and remove or stop referencing
commands that are not present in the registry (/plugin, /auto-test,
/auto-commit, /dry-run, /diff-preview), and update the completion handler to
read from App::SLASH_COMMANDS rather than a separate hardcoded list.
Comment on lines
+35
to
+44
| "/quit" => { | ||
| if app.state != AgentState::Idle { | ||
| app.push("⚠ Agent is running. Press /quit again to force exit."); | ||
| app.pending_quit = true; | ||
| } else if !app.pending_quit && app.total_input_tokens > 0 { | ||
| app.push("⚠ Unsaved session. /quit again to exit, or /save first."); | ||
| app.pending_quit = true; | ||
| } else { | ||
| app.should_quit = true; | ||
| } |
There was a problem hiding this comment.
The double-tap /quit flow isn't wired end-to-end yet.
App::submit_input() still drops all submits while app.state != AgentState::Idle, so this busy-agent branch cannot be reached from the input box. Even after relaxing that, Line 36 still wins on the second /quit, so the promised force-exit path never reaches should_quit = true, and pending_quit is never cleared after other interactions.
🛠 Suggested direction
"/quit" => {
- if app.state != AgentState::Idle {
+ if app.pending_quit {
+ app.should_quit = true;
+ } else if app.state != AgentState::Idle {
app.push("⚠ Agent is running. Press /quit again to force exit.");
app.pending_quit = true;
- } else if !app.pending_quit && app.total_input_tokens > 0 {
+ } else if app.total_input_tokens > 0 {
app.push("⚠ Unsaved session. /quit again to exit, or /save first.");
app.pending_quit = true;
} else {
app.should_quit = true;
}
},You'll also need to clear pending_quit on any non-/quit interaction, and let safe slash commands like /quit bypass the non-idle submit gate.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "/quit" => { | |
| if app.state != AgentState::Idle { | |
| app.push("⚠ Agent is running. Press /quit again to force exit."); | |
| app.pending_quit = true; | |
| } else if !app.pending_quit && app.total_input_tokens > 0 { | |
| app.push("⚠ Unsaved session. /quit again to exit, or /save first."); | |
| app.pending_quit = true; | |
| } else { | |
| app.should_quit = true; | |
| } | |
| "/quit" => { | |
| if app.pending_quit { | |
| app.should_quit = true; | |
| } else if app.state != AgentState::Idle { | |
| app.push("⚠ Agent is running. Press /quit again to force exit."); | |
| app.pending_quit = true; | |
| } else if app.total_input_tokens > 0 { | |
| app.push("⚠ Unsaved session. /quit again to exit, or /save first."); | |
| app.pending_quit = true; | |
| } else { | |
| app.should_quit = true; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mc/crates/mc-tui/src/commands.rs` around lines 35 - 44, App::submit_input
currently drops all submissions when app.state != AgentState::Idle which
prevents the "/quit" double-tap path; change submit logic to allow
slash-commands that are safe (at least "/quit") to bypass the non-idle gate by
checking the raw input string and permitting "/quit" to be processed even when
state != Idle. In the "/quit" command handler (the match arm handling "/quit"),
flip the condition order so the pending_quit check is evaluated first and can
set should_quit = true on the second tap (e.g., if app.pending_quit {
app.should_quit = true } else { set pending_quit and push warning }), and ensure
that any non-"/quit" interaction clears app.pending_quit (clear pending_quit in
App::submit_input for normal inputs and in other command handlers) so the
pending_quit state doesn't persist incorrectly.
This was referenced Apr 13, 2026
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.
UX Polish
\abell alerts user in other tabs⟳ streaming...⠋ streaming...with rotating braille charsNote: Mouse scroll was already implemented ✅
197 tests, 0 warnings.
Summary by CodeRabbit
New Features
UI Improvements