Skip to content

Better tui#26

Merged
themartto merged 44 commits into
devfrom
better-tui
May 27, 2026
Merged

Better tui#26
themartto merged 44 commits into
devfrom
better-tui

Conversation

@themartto
Copy link
Copy Markdown
Contributor

@themartto themartto commented May 23, 2026

Summary by CodeRabbit

  • New Features

    • Full-featured interactive terminal UI (chat, session restore/browsing, model switching, theme picker) and streaming LLM output with “thinking” chunks.
  • Bug Fixes

    • Fixed tool call history/replay gaps, ensured in-progress/replayed failures are signaled correctly, and improved tool error reporting.
  • Configuration

    • Optional theme_color setting added.
  • Chores

    • Release bumped to 0.1.1; toolchain requirement raised; terminal UI dependencies added.
  • Documentation

    • CHANGELOG and README updated.

Review Change Stack

themartto added 28 commits May 16, 2026 07:58
acp_prompt was always using the global AgentState.llm regardless of the
session's model override. Sessions created with a specific model were
silently using the wrong client. Now builds a fresh client from the
session config when provider or model differs from the default.
Opens a centered overlay listing all configured providers and models.
Arrow keys navigate the list, Enter switches model mid-session (history
preserved), Esc cancels. The status bar updates immediately on switch.
Direct switch via :models <name> still works without opening the popup.

Also fixes cursor visibility (hidden while picker is open to prevent
input artifacts) and scroll bounds (saturating arithmetic to prevent
panic on navigation).
@themartto
Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 23, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 23, 2026

📝 Walkthrough

Walkthrough

Adds a Ratatui TUI (types, rendering, app state, runtime), introduces AppConfig.theme_color and resolve_with_provider, enables per-session model switching (ACP + SessionHandle), implements streaming LLM across providers (OpenAI/Anthropic/Gemini) and agent loop, improves tool execution error handling, and bumps version/toolchain.

Changes

Terminal User Interface Implementation

Layer / File(s) Summary
Version bump and TUI dependencies
CHANGELOG.md, Cargo.toml
Package bumped to 0.1.1, rust-version raised to 1.91, and ratatui + crossterm added.
AppConfig theme and resolver
src/config/types.rs, src/config/client.rs, src/config/resolve.rs
Add optional theme_color: Option<String>, update test fixtures, and add AppConfig::resolve_with_provider.
ACP per-session model handling
src/acp/mod.rs
Add acp_update_session_model, conditional per-session LLM client selection in acp_prompt, streaming ThinkingContent tunneling, and rework acp_load_session provider/model derivation (with warnings).
Client session API
src/client.rs
Add SessionHandle::switch_model and SessionHandle::restore for programmatic model switching and session restoration.
Tool execution error handling
src/tools/execute_command.rs
Return ToolExecutionError on non-zero command exit and update tests to expect Err with diagnostic text.
WS path canonicalization
src/transport/ws.rs
Refactor relative canonicalization branch in validate_path_opt to an early-return form.
Agent streaming integration
src/core/agent.rs
Add call_llm_streaming helper and update run_agent_loop to consume LlmChunk streaming events and emit StreamEvent chunks during streaming requests.
LLM streaming API surface
src/core/llm/mod.rs
Add LlmChunk enum and LlmClient::send_streaming trait method with a default fallback.
Anthropic streaming & request shape
src/core/llm/anthropic.rs
Extend request with stream/thinking, extract system instruction, add send_streaming SSE parsing and tool-call reassembly, and reuse helpers in non-streaming send.
OpenAI-style streaming
src/core/llm/openai.rs
Add send_openai_style_streaming to parse SSE chunks into LlmChunks and implement send_streaming for OpenAiClient.
OpenAI-compatible streaming
src/core/llm/openai_compatible.rs
Add send_streaming that forwards to send_openai_style_streaming.
Gemini streaming & system helper
src/core/llm/gemini.rs
Extract gemini_system_instruction and add send_streaming for Gemini SSE streaming and tool-call accumulation.
Retry client streaming delegation
src/core/llm/retry.rs
Forward send_streaming to the wrapped inner client.
StreamEvent variant
src/core/models.rs
Add StreamEvent::ThinkingContent for model internal reasoning chunks.
MCP spawn behavior
src/mcp/client.rs
Enable kill_on_drop(true) for stdio MCP spawn path.
TUI core types
src/tui/types.rs
Add AgentUpdate, ChatItem, Status, Screen (with is_overlay), and ConfigRow.
TUI rendering system
src/tui/render.rs
Add theme palette and mapping, build_lines chat rendering with bubbles/word-wrap/thinking/tool previews, popups for model/session/config/mcp/skills/theme, and word_wrap.
TUI app state & interaction
src/tui/app.rs
Add App state, save_theme_to_config, push, handle_update, handle_key, handle_command, open_session, draw, and draw_chat.
TUI module & runtime
src/tui/mod.rs
Restructure module, add TerminalGuard, spawn background task for session prompt/switch/restore handling, implement convert_update mapping, and run the main tokio/select! render/input loop.

Sequence Diagram(s)

sequenceDiagram
  participant User
  participant Terminal
  participant TUI_App
  participant Background_Task
  participant AgentState

  User->>Terminal: keyboard input
  Terminal->>TUI_App: KeyEvent
  TUI_App->>TUI_App: handle_key / handle_command
  alt model switch
    TUI_App->>Background_Task: send (provider,model) via channel
    Background_Task->>AgentState: SessionHandle.switch_model(provider,model)
    AgentState-->>Background_Task: (provider_name, model_name)
    Background_Task->>TUI_App: AgentUpdate::ModelChanged
  else session restore
    TUI_App->>Background_Task: send (session_id,cwd)
    Background_Task->>AgentState: SessionHandle.restore(session_id,cwd)
    AgentState-->>Background_Task: restored SessionUpdate events
    Background_Task->>TUI_App: AgentUpdate variants (TextChunk/ToolCall/ToolResult...)
  else prompt
    TUI_App->>Background_Task: send prompt text
    Background_Task->>AgentState: prompt(text)
    loop streaming
      AgentState->>Background_Task: StreamEvent / LlmChunk
      Background_Task->>TUI_App: AgentUpdate mapping
    end
  end
  TUI_App->>Terminal: draw frame
  Terminal->>User: rendered UI
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Poem

🐰 A terminal blooms with colors bright,

Ratatui stitches the UI's light,
Streams of thought hop line by line,
Models switch mid-chat so fine,
This rabbit sips carrot code and cheers.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 61.73% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title 'Better tui' is vague and does not clearly convey the scope and purpose of the changeset, which includes streaming support, TUI application, configuration updates, and multiple core features. Consider a more descriptive title that highlights the primary addition: e.g., 'Add Ratatui-based TUI with real-time streaming' or 'Implement streaming support and TUI chat application'.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 6

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@CHANGELOG.md`:
- Line 15: The release note "Added `CHANGELOG.md`" is inaccurate because
CHANGELOG.md already contains a 0.1.0 section; update the entry in CHANGELOG.md
to a correct phrasing such as "Updated changelog (added 0.1.0 release notes)" or
"Documented 0.1.0 release in changelog" so it no longer claims the file was
newly added; locate the line containing the exact string "Added `CHANGELOG.md`"
and replace it with the corrected wording.

In `@src/acp/mod.rs`:
- Around line 120-127: The session branch that rebuilds the per-session client
uses s.config directly (the block constructing llm and calling
build_http_client/create_client) but s.config may only contain partial overrides
(provider_name/model) and miss provider defaults like api_base/api_key/timeout;
hydrate/merge the full provider configuration into s.config before constructing
the HTTP client and calling create_client so the reconstructed client uses the
correct endpoint/credentials; specifically, locate the branch that sets llm (the
comparison of s.config vs self.config) and replace the direct use of s.config
with a merged/filled config (pulling defaults for the provider and applying
session overrides) prior to calling crate::config::build_http_client and
crate::config::create_client.

In `@src/tui/app.rs`:
- Around line 289-307: In handle_model_picker_key, when Enter is pressed you
currently extract only the model from picker_items and send model.clone() via
switch_model_tx, which drops the provider; instead send the full (provider,
model) pair so the receiver can disambiguate backends. Locate the match arm in
handle_model_picker_key that reads self.picker_items.get(self.picker_selected)
and replace the send of model.clone() with sending the cloned tuple
(provider.clone(), model.clone()) (or otherwise construct the same pair type
expected by switch_model_tx); ensure switch_model_tx’s message type and any
receivers are updated accordingly and keep the existing transition to
Screen::Chat and picker_selected usage unchanged.
- Around line 599-652: open_session currently appends restored messages to the
existing chat; before loading conv.messages you must clear the current
transcript and reset view state so the restored session replaces the old one. In
open_session (the RagContext::new().and_then(...) success branch) call the
component's chat-reset routine (e.g. clear self.items or invoke an existing
reset/clear method) and reset any scroll/pinned state fields before the for msg
in &conv.messages loop, then proceed to push the restored ChatItem entries and
the "─── session restored" SystemInfo.

In `@src/tui/mod.rs`:
- Around line 111-181: The function enables raw mode/alternate screen/keyboard
enhancement (enable_raw_mode, EnterAlternateScreen,
PushKeyboardEnhancementFlags) but only restores state in the panic hook, so any
? error path leaks the terminal; fix by creating a RAII guard type (e.g.,
TerminalGuard) that captures kbd_enhanced and a handle to the terminal/backend
and implements Drop to PopKeyboardEnhancementFlags, LeaveAlternateScreen,
disable_raw_mode, and show_cursor; instantiate this guard immediately after the
successful
enable_raw_mode/EnterAlternateScreen/PushKeyboardEnhancementFlags/Terminal::new
sequence (and remove duplicate cleanup in the panic hook or have the hook call
into the guard) so cleanup runs on all exit paths including normal returns and
early errors, and reference Terminal, EventStream, and update_rx usage remains
unchanged.

In `@src/tui/render.rs`:
- Around line 136-173: The bubble border can overflow because word_wrap only
breaks on whitespace, so in user_bubble the computed content_w can exceed the
viewport; fix by post-processing the wrapped lines (the Vec produced from
word_wrap) inside user_bubble to hard-split any run/token longer than
content_max into chunks of at most content_max characters before computing
content_w and rendering; specifically iterate each line from word_wrap, for each
contiguous run longer than content_max slice it into width-sized pieces and push
those pieces into the new wrapped list, then use that new wrapped list for
content_w calculation and rendering (same approach should also be applied to the
other similar function/section noted in the comment).
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 8a712a69-b850-4891-8f31-e0014a0258be

📥 Commits

Reviewing files that changed from the base of the PR and between 4588b77 and 63855d1.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (13)
  • CHANGELOG.md
  • Cargo.toml
  • src/acp/mod.rs
  • src/client.rs
  • src/config/client.rs
  • src/config/resolve.rs
  • src/config/types.rs
  • src/tools/execute_command.rs
  • src/transport/ws.rs
  • src/tui/app.rs
  • src/tui/mod.rs
  • src/tui/render.rs
  • src/tui/types.rs

Comment thread CHANGELOG.md
Comment thread src/acp/mod.rs
Comment thread src/tui/app.rs
Comment thread src/tui/app.rs
Comment thread src/tui/mod.rs Outdated
Comment thread src/tui/render.rs
@themartto
Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 23, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/acp/mod.rs`:
- Around line 248-261: The code silently ignores a restored
conversation.meta.provider that is missing from self.app_config.providers and
thus can restore a session with wrong defaults; update the branch in acp::mod
(the block handling if let Some(provider_name) = &conversation.meta.provider) to
fail fast when self.app_config.providers.get(provider_name) returns None by
returning an error (or propagating a Result) instead of falling through to the
else model branch, so that session_config is not populated with base defaults
for a non-existent provider; reference the provider_name lookup and ensure the
error includes the missing provider_name and occurs before any assignment to
session_config fields.

In `@src/config/resolve.rs`:
- Around line 56-74: resolve_with_provider currently accepts any model string
even if the chosen provider defines an allowed models list; add validation after
validate_provider to check the provider's models whitelist (e.g. inspect
provider.models or similar field) and return a config Error if the requested
model is not contained in that list, so invalid (provider, model) pairs are
rejected before constructing AgentConfig; update resolve_with_provider to
perform this membership check and only proceed to build AgentConfig when the
model is allowed.
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 8c725d3d-edb4-4387-a98b-855f14d7bf8b

📥 Commits

Reviewing files that changed from the base of the PR and between 63855d1 and 3373eca.

📒 Files selected for processing (6)
  • src/acp/mod.rs
  • src/client.rs
  • src/config/resolve.rs
  • src/tui/app.rs
  • src/tui/mod.rs
  • src/tui/render.rs

Comment thread src/acp/mod.rs
Comment on lines +248 to +261
if let Some(provider_name) = &conversation.meta.provider {
if let Some(provider_cfg) = self.app_config.providers.get(provider_name) {
session_config.provider_name = provider_name.clone();
session_config.api_base = provider_cfg.api_base.clone();
session_config.api_key = provider_cfg.resolve_api_key();
session_config.timeout_secs = provider_cfg.timeout_secs.unwrap_or(120);
session_config.max_tokens = provider_cfg.max_tokens;
session_config.model = conversation
.meta
.model
.clone()
.unwrap_or_else(|| provider_cfg.default_model.clone());
}
} else if let Some(model) = &conversation.meta.model {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Fail fast when restored provider is missing from config.

If history metadata has a provider that no longer exists in app_config, this branch silently falls back to base defaults. That can restore a session onto the wrong provider/model without any signal.

💡 Proposed fix
         let mut session_config = self.config.clone();
         if let Some(provider_name) = &conversation.meta.provider {
-            if let Some(provider_cfg) = self.app_config.providers.get(provider_name) {
-                session_config.provider_name = provider_name.clone();
-                session_config.api_base = provider_cfg.api_base.clone();
-                session_config.api_key = provider_cfg.resolve_api_key();
-                session_config.timeout_secs = provider_cfg.timeout_secs.unwrap_or(120);
-                session_config.max_tokens = provider_cfg.max_tokens;
-                session_config.model = conversation
-                    .meta
-                    .model
-                    .clone()
-                    .unwrap_or_else(|| provider_cfg.default_model.clone());
-            }
+            let provider_cfg = self.app_config.providers.get(provider_name).ok_or_else(|| {
+                Error::config(format!(
+                    "Provider '{}' referenced by session '{}' not found in config",
+                    provider_name, session_id
+                ))
+            })?;
+            session_config.provider_name = provider_name.clone();
+            session_config.api_base = provider_cfg.api_base.clone();
+            session_config.api_key = provider_cfg.resolve_api_key();
+            session_config.timeout_secs = provider_cfg.timeout_secs.unwrap_or(120);
+            session_config.max_tokens = provider_cfg.max_tokens;
+            session_config.model = conversation
+                .meta
+                .model
+                .clone()
+                .unwrap_or_else(|| provider_cfg.default_model.clone());
         } else if let Some(model) = &conversation.meta.model {
             session_config.model = model.clone();
         }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/acp/mod.rs` around lines 248 - 261, The code silently ignores a restored
conversation.meta.provider that is missing from self.app_config.providers and
thus can restore a session with wrong defaults; update the branch in acp::mod
(the block handling if let Some(provider_name) = &conversation.meta.provider) to
fail fast when self.app_config.providers.get(provider_name) returns None by
returning an error (or propagating a Result) instead of falling through to the
else model branch, so that session_config is not populated with base defaults
for a non-existent provider; reference the provider_name lookup and ensure the
error includes the missing provider_name and occurs before any assignment to
session_config fields.

Comment thread src/config/resolve.rs
@themartto
Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 27, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 4

🧹 Nitpick comments (2)
src/core/llm/retry.rs (1)

49-56: 💤 Low value

Streaming bypasses retry logic.

Unlike send(), send_streaming() delegates directly without any retry handling. This is likely intentional since retrying mid-stream would cause duplicate/inconsistent chunks. Consider adding a brief comment to clarify this design decision.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/core/llm/retry.rs` around lines 49 - 56, send_streaming currently
delegates straight to self.inner.send_streaming without retry handling, which
can be confusing; add a short clarifying comment above the async fn
send_streaming (in retry.rs, function send_streaming) stating that streaming
intentionally bypasses the retry logic because retrying mid-stream would produce
duplicate or inconsistent chunks and thus retries are disabled for streaming
paths. Ensure the comment mentions that send() retains retry behavior while
send_streaming does not.
src/core/llm/gemini.rs (1)

372-372: 💤 Low value

Avoid unnecessary String allocation.

&"sse".to_string() allocates a heap String. Use a static reference directly.

♻️ Suggested fix
-            .query(&[("key", &self.api_key), ("alt", &"sse".to_string())])
+            .query(&[("key", &self.api_key), ("alt", "sse")])
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/core/llm/gemini.rs` at line 372, The .query call is creating an
unnecessary heap allocation by using &"sse".to_string(); replace that allocation
with a static &str literal (e.g., use "sse" or &"sse") in the .query argument so
the tuple becomes ("alt", "sse") (located in the .query(...) call that includes
("key", &self.api_key)).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/core/agent.rs`:
- Around line 77-112: The loop can break when chunk_rx.recv() returns None
before the pinned streaming future (choice_fut) completes, leaving maybe_choice
None and causing a spurious "stream ended prematurely" error; to fix, after the
loop check if maybe_choice.is_none() and if so await the pinned choice_fut to
obtain its Result<Choice> (then set maybe_choice accordingly) before
unwrapping/propagating the value—adjust the logic around the existing
call_llm_streaming, choice_fut, maybe_choice, chunk_rx, LlmChunk and StreamEvent
handling to ensure the streaming future is awaited when the channel closes.

In `@src/core/llm/gemini.rs`:
- Around line 427-431: Run rustfmt (cargo fmt) to fix the formatting around the
let-chain in the if-condition that checks part.text and !text.is_empty(); ensure
the condition and block for the code that calls text_buf.push_str(&text) and
chunk_tx.send(LlmChunk::Text(text)) are formatted per rustfmt (or rewrite the
conditional to a rustfmt-friendly style, e.g., split into a simple if let
Some(text) = part.text { if !text.is_empty() { ... } } so cargo fmt --check
passes).

In `@src/core/llm/openai.rs`:
- Around line 174-183: The let-chain conditions using
delta["reasoning_content"].as_str() and delta["content"].as_str() are not
formatted to match cargo fmt; run `cargo fmt` (or apply rustfmt) to reformat the
if-let chains in openai.rs so the conditions and block indentation match the
project's style, specifically in the blocks that reference delta, chunk_tx,
text_buf and LlmChunk (the `if let Some(reasoning) =
delta["reasoning_content"].as_str() && !reasoning.is_empty()` and `if let
Some(content) = delta["content"].as_str() && !content.is_empty()` statements);
commit the formatted changes so `cargo fmt --check` passes.

In `@src/tui/mod.rs`:
- Around line 200-204: The formatting of the boolean assignment for is_thinking
(the chained calls on t.meta -> as_ref -> and_then -> get -> and_then -> as_str)
does not match rustfmt’s expectations; run cargo fmt to reformat src/tui/mod.rs
(or manually reflow the chained method calls on the is_thinking assignment to
match rustfmt output) so the expression is formatted correctly and the CI check
passes.

---

Nitpick comments:
In `@src/core/llm/gemini.rs`:
- Line 372: The .query call is creating an unnecessary heap allocation by using
&"sse".to_string(); replace that allocation with a static &str literal (e.g.,
use "sse" or &"sse") in the .query argument so the tuple becomes ("alt", "sse")
(located in the .query(...) call that includes ("key", &self.api_key)).

In `@src/core/llm/retry.rs`:
- Around line 49-56: send_streaming currently delegates straight to
self.inner.send_streaming without retry handling, which can be confusing; add a
short clarifying comment above the async fn send_streaming (in retry.rs,
function send_streaming) stating that streaming intentionally bypasses the retry
logic because retrying mid-stream would produce duplicate or inconsistent chunks
and thus retries are disabled for streaming paths. Ensure the comment mentions
that send() retains retry behavior while send_streaming does not.
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 1e651fa5-1a76-4751-8328-b714b86cc0c8

📥 Commits

Reviewing files that changed from the base of the PR and between 3373eca and 76fdab2.

📒 Files selected for processing (15)
  • src/acp/mod.rs
  • src/config/resolve.rs
  • src/core/agent.rs
  • src/core/llm/anthropic.rs
  • src/core/llm/gemini.rs
  • src/core/llm/mod.rs
  • src/core/llm/openai.rs
  • src/core/llm/openai_compatible.rs
  • src/core/llm/retry.rs
  • src/core/models.rs
  • src/mcp/client.rs
  • src/tui/app.rs
  • src/tui/mod.rs
  • src/tui/render.rs
  • src/tui/types.rs
🚧 Files skipped from review as they are similar to previous changes (4)
  • src/config/resolve.rs
  • src/tui/types.rs
  • src/tui/app.rs
  • src/tui/render.rs

Comment thread src/core/agent.rs
Comment on lines +77 to +112
let choice = if callback.is_some() {
let (chunk_tx, mut chunk_rx) = mpsc::unbounded_channel::<LlmChunk>();
let choice_fut = call_llm_streaming(llm, messages, &tools, prompt_builder, chunk_tx);
tokio::pin!(choice_fut);

let mut maybe_choice: Option<Result<Choice>> = None;
loop {
tokio::select! {
result = &mut choice_fut, if maybe_choice.is_none() => {
maybe_choice = Some(result);
}
maybe_chunk = chunk_rx.recv() => {
match maybe_chunk {
Some(LlmChunk::Text(text)) => {
if let Some(cb) = callback.as_mut() {
cb(StreamEvent::LlmResponse { content: text });
}
}
Some(LlmChunk::Thinking(thought)) => {
if let Some(cb) = callback.as_mut() {
cb(StreamEvent::ThinkingContent { content: thought });
}
}
None => break,
}
}
}
}
maybe_choice.unwrap_or_else(|| {
Err(crate::error::Error::Other(
"stream ended prematurely".into(),
))
})?
} else {
call_llm(llm, messages, &tools, prompt_builder).await?
};
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Race between channel close and choice completion can spuriously error.

When send_streaming completes, it drops chunk_tx and returns Choice simultaneously. If tokio::select! picks recv() → None before the choice_fut arm in that final poll cycle, the loop breaks with maybe_choice still None, yielding a spurious "stream ended prematurely" error even though the choice is ready.

After the loop, await the pinned future if maybe_choice is None:

Proposed fix
-            maybe_choice.unwrap_or_else(|| {
-                Err(crate::error::Error::Other(
-                    "stream ended prematurely".into(),
-                ))
-            })?
+            match maybe_choice {
+                Some(result) => result?,
+                None => choice_fut.await?,
+            }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/core/agent.rs` around lines 77 - 112, The loop can break when
chunk_rx.recv() returns None before the pinned streaming future (choice_fut)
completes, leaving maybe_choice None and causing a spurious "stream ended
prematurely" error; to fix, after the loop check if maybe_choice.is_none() and
if so await the pinned choice_fut to obtain its Result<Choice> (then set
maybe_choice accordingly) before unwrapping/propagating the value—adjust the
logic around the existing call_llm_streaming, choice_fut, maybe_choice,
chunk_rx, LlmChunk and StreamEvent handling to ensure the streaming future is
awaited when the channel closes.

Comment thread src/core/llm/gemini.rs Outdated
Comment on lines +427 to +431
if let Some(text) = part.text
&& !text.is_empty() {
text_buf.push_str(&text);
let _ = chunk_tx.send(LlmChunk::Text(text));
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Fix formatting to pass CI.

The pipeline is failing due to cargo fmt --check differences around the let-chain condition. Run cargo fmt to apply the expected formatting.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/core/llm/gemini.rs` around lines 427 - 431, Run rustfmt (cargo fmt) to
fix the formatting around the let-chain in the if-condition that checks
part.text and !text.is_empty(); ensure the condition and block for the code that
calls text_buf.push_str(&text) and chunk_tx.send(LlmChunk::Text(text)) are
formatted per rustfmt (or rewrite the conditional to a rustfmt-friendly style,
e.g., split into a simple if let Some(text) = part.text { if !text.is_empty() {
... } } so cargo fmt --check passes).

Comment thread src/core/llm/openai.rs Outdated
Comment on lines +174 to +183
if let Some(reasoning) = delta["reasoning_content"].as_str()
&& !reasoning.is_empty() {
let _ = chunk_tx.send(LlmChunk::Thinking(reasoning.to_string()));
}

if let Some(content) = delta["content"].as_str()
&& !content.is_empty() {
text_buf.push_str(content);
let _ = chunk_tx.send(LlmChunk::Text(content.to_string()));
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Fix formatting to pass CI.

The pipeline is failing due to cargo fmt --check differences around the let-chain conditions. Run cargo fmt to apply the expected formatting.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/core/llm/openai.rs` around lines 174 - 183, The let-chain conditions
using delta["reasoning_content"].as_str() and delta["content"].as_str() are not
formatted to match cargo fmt; run `cargo fmt` (or apply rustfmt) to reformat the
if-let chains in openai.rs so the conditions and block indentation match the
project's style, specifically in the blocks that reference delta, chunk_tx,
text_buf and LlmChunk (the `if let Some(reasoning) =
delta["reasoning_content"].as_str() && !reasoning.is_empty()` and `if let
Some(content) = delta["content"].as_str() && !content.is_empty()` statements);
commit the formatted changes so `cargo fmt --check` passes.

Comment thread src/tui/mod.rs Outdated
Comment on lines +200 to +204
let is_thinking = t
.meta
.as_ref()
.and_then(|m| m.get("kind"))
.and_then(|v| v.as_str()) == Some("thinking");
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Fix formatting to pass CI.

The pipeline is failing due to cargo fmt --check detecting formatting differences on this chain. Run cargo fmt to apply the expected formatting.

🧰 Tools
🪛 GitHub Actions: CI / Check

[error] 201-201: cargo fmt --check failed due to Rust formatting differences in this file (line break/indentation in .and_then(|v| v.as_str()) == Some("thinking")). Run cargo fmt to apply the formatting.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/tui/mod.rs` around lines 200 - 204, The formatting of the boolean
assignment for is_thinking (the chained calls on t.meta -> as_ref -> and_then ->
get -> and_then -> as_str) does not match rustfmt’s expectations; run cargo fmt
to reformat src/tui/mod.rs (or manually reflow the chained method calls on the
is_thinking assignment to match rustfmt output) so the expression is formatted
correctly and the CI check passes.

@themartto themartto marked this pull request as ready for review May 27, 2026 03:03
@themartto themartto merged commit c6a3446 into dev May 27, 2026
3 checks passed
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