9 feature mcp model context protocol#10
Merged
Merged
Conversation
- Added MCP module to facilitate interaction with external tools via JSON-RPC. - Introduced native tools, including a dice rolling feature, that run in-process without subprocess overhead. - Enhanced the agent loop to utilize MCP tools, allowing for tool calls during conversation. - Created a dashboard panel to display available MCP tools and their commands. - Updated API to serve tool information and integrated MCP functionality into the existing application structure. - Added design documentation for MCP and its implementation details.
- Added support for managing filesystem paths in MCP configuration, allowing users to set allowed folders for tools. - Implemented new API endpoints for fetching MCP configuration and updating filesystem paths. - Integrated Tauri dialog plugin for a native folder picker, enhancing user experience in selecting filesystem paths. - Updated MCP tools panel to display configuration details and allow users to apply filesystem paths directly. - Added example MCP configuration file for reference and improved documentation for new features.
- Updated the agent's system message to clarify tool usage rules for better user guidance. - Refined text handling in the agent to ensure non-empty responses are logged appropriately. - Added a `direct_return` option in the MCP client to allow tools to return results directly to users, bypassing the model for summarization. - Adjusted the MCP service and types to support the new `direct_return` functionality, enhancing tool integration.
- Added new API endpoints for managing MCP servers, including fetching, creating, updating, and deleting server entries. - Introduced `ServerEntry` types to support both standard and native server configurations. - Created `AddServerForm` component for user-friendly server addition and configuration. - Developed `McpServerCard` component for displaying and managing individual server entries. - Updated `McpToolsPanel` to integrate server management features, allowing users to view and interact with configured servers. - Enhanced the `TerminalPreview` component for improved display of terminal output.
- Added `fetchOllamaModels` and `setPreferredOllamaModel` functions to the API for improved model management. - Introduced `OllamaModelsResponse` type to encapsulate model fetching responses. - Updated `DashboardPage` to utilize new functions for fetching and setting preferred models, enhancing user interaction with available models. - Refactored existing model handling to support the new structure and improve overall functionality.
- Enhanced the `chat_with_tools` function to return a structured `ChatResult`, indicating whether tools were supported by the model. - Updated the agent's `run_turn` function to handle cases where tools are not supported, logging appropriate messages for better user feedback. - Refactored message handling to ensure consistent response processing and improved error handling when tools are not available.
3 tasks
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a proof‑of‑concept Model Context Protocol (MCP) integration: new Rust MCP subsystem (types, protocol, transport, client, native providers, registry, service), Tauri wiring and HTTP endpoints, bot agent tool‑use loop, frontend MCP UI and APIs, docs, example config, tests, and dialog support. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (Telegram)
participant Handler as Message Handler
participant Agent as Agent
participant Registry as Tool Registry
participant MCP as MCP Client / Native
participant Ollama as Ollama API
User->>Handler: inbound message
Handler->>Agent: run_turn(state, message)
Agent->>Ollama: resolve model (preferred/active)
Agent->>Registry: fetch available tools
Registry-->>Agent: tools list
Agent->>Ollama: chat_with_tools(model, messages, tools)
Ollama-->>Agent: assistant message (+ tool_calls)
loop tool-call iterations (≤ MAX_STEPS)
Agent->>Registry: resolve tool name
Registry->>MCP: call_tool(name, args)
MCP-->>Registry: tool output
Registry-->>Agent: tool output (+direct flag)
Agent->>Ollama: chat_with_tools(messages + tool results, tools)
Ollama-->>Agent: next assistant message (+ tool_calls)
end
alt assistant produced text
Agent-->>Handler: return model text
else only tool results
Agent->>Ollama: summarize tool outputs (no tools)
Ollama-->>Agent: summary
Agent-->>Handler: return summary or last tool output
end
Handler->>User: reply
sequenceDiagram
participant App as Tauri App
participant Service as MCP Service
participant Config as mcp.json
participant Native as Native Provider
participant Stdio as StdioTransport
participant Child as MCP Server (subprocess)
App->>Service: load_or_init_config(path)
Service->>Config: read or create default
App->>Service: build_registry(cfg)
loop each server entry
alt native
Service->>Native: native_for(id)
Native-->>Service: NativeProvider (tools)
else stdio
Service->>Stdio: spawn(command,args,env)
Stdio->>Child: spawn child process
Stdio->>Child: send initialize, notify
Child-->>Stdio: initialize response
Stdio->>Child: tools/list
Child-->>Stdio: tools response
Stdio-->>Service: McpClient (connected, tools cached)
end
Service->>Service: collect Provider
end
Service-->>App: ToolRegistry (ready)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. 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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src-tauri/src/modules/ollama/service.rs (1)
32-40:⚠️ Potential issue | 🟠 MajorKeep
/api/tagsfrom masking a valid active model.If
/api/pssucceeds but/api/tagsfails, Line 39 returns an error and drops theactivemodel already discovered above. Sinceactive_model()now depends onmodel_catalog(), a partial Ollama outage here makes the app behave as if no usable model exists even when one is already loaded.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/modules/ollama/service.rs` around lines 32 - 40, The call to OLLAMA_TAGS_URL in model_catalog() currently returns Err on failure which wipes out an active model discovered earlier by active_model(); change model_catalog() so failures from client.get(OLLAMA_TAGS_URL) or non-success resp.status() are non-fatal: catch the error, log or map it to a warning, skip tag parsing, and return the existing/partial catalog (or Ok with the catalog sans tags) so active_model() can still find the previously discovered active model; specifically modify the code around the client.get(OLLAMA_TAGS_URL) / resp handling to avoid returning Err and instead propagate a usable catalog/state to active_model().
🧹 Nitpick comments (9)
src-tauri/Cargo.toml (1)
37-39: Redundant dev-dependencies.Both
serde_json(line 24) andtokiowithfeatures = ["full"](line 25) are already declared in[dependencies]. Cargo automatically makes main dependencies available to tests, so these dev-dependencies are unnecessary.♻️ Proposed cleanup
-[dev-dependencies] -serde_json = "1" -tokio = { version = "1", features = ["macros", "rt-multi-thread"] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/Cargo.toml` around lines 37 - 39, Remove the redundant entries from the [dev-dependencies] section in Cargo.toml by deleting the serde_json and tokio lines since serde_json and tokio (with full features) are already declared under [dependencies]; ensure only unique dev-only crates remain and that no other dev-dependency references (serde_json or tokio) are required by tests before removing.src-tauri/src/infrastructure/http_server.rs (1)
430-478: Consider TOCTOU race condition in config updates.The load → modify → save pattern has a potential race condition if multiple requests arrive concurrently. For a single-user desktop app this is low risk, but consider adding a mutex around config file operations if MCP management becomes more heavily used.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/infrastructure/http_server.rs` around lines 430 - 478, The handler handle_mcp_server_upsert uses a load→modify→save sequence (mcp_service::load_or_init_config, cfg.servers.insert, mcp_service::save_config, mcp_service::rebuild_registry_into_state) and is vulnerable to TOCTOU races; protect this whole sequence with a shared async mutex on the AppState (e.g., add a tokio::sync::Mutex or tokio::sync::RwLock field like mcp_config_mutex in AppState) and acquire the lock at the start of handle_mcp_server_upsert before calling load_or_init_config, hold it while inserting into cfg and saving via save_config and while calling rebuild_registry_into_state, then release it afterward so concurrent requests serialize config mutations.src/modules/mcp/components/McpToolsPanel.tsx (1)
40-45: Avoid 1s steady-state polling for an empty registry.When no MCP tools are connected, this keeps hitting the tools endpoint every second for the whole dashboard session. An idle interval/backoff here would reduce needless local API churn without changing behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/mcp/components/McpToolsPanel.tsx` around lines 40 - 45, The pollTools function currently polls fetchMcpTools every 1s when no tools are returned, causing unnecessary steady-state requests; change its backoff logic in pollTools so that when data.length === 0 you use a larger idle interval (e.g., exponential/backoff or a fixed longer delay like 30s) before scheduling timer = setTimeout(() => pollTools(), next), and ensure you still use the shorter interval (10_000) when tools exist; keep handling of cancelled and setTools unchanged and update any related timer clearing logic if present to match the new interval behavior.src-tauri/src/modules/mcp/registry.rs (1)
66-71:all_tools()returns unfiltered tools whiletool_names()returns filtered names.
all_tools()returns all tools without filtering, buttool_names()only returns names of tools that passshould_expose_to_model. This inconsistency could be confusing. Consider documenting or aligning the behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/modules/mcp/registry.rs` around lines 66 - 71, all_tools() currently returns every ToolDef from providers but tool_names() only returns names of tools that pass should_expose_to_model, causing inconsistent behavior; update all_tools() to mirror tool_names() by filtering ToolDef instances with the should_expose_to_model predicate (call the same function used in tool_names()) before collecting, or alternatively add a clear doc comment on all_tools() stating it returns unfiltered tools—ensure you reference all_tools(), tool_names(), should_expose_to_model, providers and tools() when making the change so the behavior is explicit and consistent.src/modules/mcp/components/McpServerCard.tsx (1)
375-394: Consider using path value as key instead of index.Using array index as React key can cause subtle bugs if the list is reordered. Since paths are unique strings (duplicates are prevented by
addPath), the path itself would be a more stable key.♻️ Use path as key
- {paths.map((p, i) => ( + {paths.map((p) => ( <div - key={i} + key={p} className="flex items-center gap-2 rounded-md border border-white/8 bg-white/5 px-2 py-1" > <p className="min-w-0 flex-1 truncate font-mono text-[11px] text-white/80" title={p}> {p} </p> <button type="button" - onClick={() => onRemove(i)} + onClick={() => onRemove(paths.indexOf(p))}Note: If using path as key, you'd need to adjust the
onRemovesignature or use a different removal approach.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/mcp/components/McpServerCard.tsx` around lines 375 - 394, The list item mapping in McpServerCard uses the array index as the React key which is unstable; change the key to use the unique path string (use key={p}) and update the removal logic to accept a path rather than an index: update the onRemove prop/signature (or add a new onRemoveByPath handler) so that the remove action receives the path (or resolves the index via paths.indexOf(path)) instead of relying on the mapped index; adjust any callers of onRemove and the addPath logic that enforces uniqueness accordingly.src-tauri/src/modules/mcp/transport.rs (1)
47-71: Consider logging stderr output for debugging.The stderr reader task consumes and discards all output. For debugging MCP server issues, consider logging stderr content at debug/trace level instead of silently dropping it.
♻️ Optional: Log stderr for debugging
tokio::spawn(async move { let mut lines = BufReader::new(stderr).lines(); - while let Ok(Some(_)) = lines.next_line().await {} + while let Ok(Some(line)) = lines.next_line().await { + tracing::debug!(target: "mcp_stderr", "{}", line); + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/modules/mcp/transport.rs` around lines 47 - 71, The stderr reader currently discards all output; instead, capture and log each stderr line for debugging by replacing the empty consumer in the tokio::spawn that reads from BufReader::new(stderr).lines() with code that awaits lines.next_line().await, trims and skips empty lines, and logs the content at debug/trace level (e.g., via tracing::debug! or log::debug!) including context like "mcp stderr:"; keep the task non-blocking and handle serde/unexpected errors by logging them rather than silently ignoring, referencing the existing pending, BufReader, stderr, lines.next_line and tokio::spawn identifiers to locate where to change behavior.src-tauri/src/modules/bot/agent.rs (1)
266-273: Unwrap is safe here but could be clearer.
tool_results.last().map(|(_, c)| c.clone()).unwrap()is safe because this block is only entered when!tool_results.is_empty(). However, usingexpectwith a reason or a comment would make the safety invariant explicit.♻️ Optional: Add safety comment or use expect
- let fallback = tool_results.last().map(|(_, c)| c.clone()).unwrap(); + // Safety: tool_results is non-empty (checked at line 224) + let fallback = tool_results.last().map(|(_, c)| c.clone()).unwrap();Or:
- let fallback = tool_results.last().map(|(_, c)| c.clone()).unwrap(); + let fallback = tool_results + .last() + .map(|(_, c)| c.clone()) + .expect("tool_results checked non-empty at block entry");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/modules/bot/agent.rs` around lines 266 - 273, Replace the blind unwrap on tool_results.last().map(|(_, c)| c.clone()).unwrap() with an explicit safety assertion: either call expect(...) with a clear message (e.g., "tool_results must be non-empty here") or keep the unwrap but add a brief comment above the line explaining the invariant enforced earlier; update the fallback assignment in this block (where fallback is set and returned in TurnResult) to use the expect message so the intent and safety are explicit.doc/design/mcp.md (2)
17-29: Add language specifier to fenced code blocks.Per markdownlint, fenced code blocks should have a language specified. For ASCII diagrams, use
textorplaintext.📝 Proposed fix for diagram block
-``` +```text Telegram message │🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doc/design/mcp.md` around lines 17 - 29, The fenced ASCII diagram block (the code fence containing "Telegram message" → "bot::service::text_handler" → "bot::agent::run_turn" etc.) is missing a language tag; update that fenced code block to include a language specifier such as "text" or "plaintext" so it satisfies markdownlint (e.g., change ``` to ```text) while leaving the diagram content unchanged.
33-42: Add language specifier to directory tree code block.📝 Proposed fix
-``` +```text src-tauri/src/modules/mcp/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doc/design/mcp.md` around lines 33 - 42, Update the Markdown directory-tree code block that starts with "src-tauri/src/modules/mcp/" in doc/design/mcp.md to include a language specifier by replacing the opening fence ``` with ```text so the tree is rendered as a text block; the block contains the file list (mod.rs, protocol.rs, types.rs, transport.rs, client.rs, registry.rs, service.rs).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@doc/design/mcp.md`:
- Around line 64-73: The header "Three messages" is inconsistent with the list
which enumerates four messages (initialize, notifications/initialized,
tools/list, tools/call); either update the header to "Four messages" or
remove/merge one of the listed items to match the intended scope—locate the
section containing the header and the four items (`initialize`,
`notifications/initialized`, `tools/list`, `tools/call`) and make the header and
list consistent (preferably change the header to "Four messages" if all four are
intended).
In `@src-tauri/src/app.rs`:
- Around line 28-55: The startup logs can be lost because app_handle is set
inside tauri::async_runtime::spawn and may not run before mcp_state.emit_log;
change the ordering to set shared_state.app_handle synchronously before emitting
logs by awaiting the lock and assigning the handle prior to calling
mcp_service::load_or_init_config—e.g., replace the
tauri::async_runtime::spawn(...) that sets *state.app_handle.lock().await =
Some(handle) with a tauri::async_runtime::block_on(...) (or otherwise ensure the
async assignment completes) so that state.app_handle is Some(...) before any
calls to emit_log or the block_on that loads MCP.
In `@src-tauri/src/modules/bot/agent.rs`:
- Around line 178-191: The code currently holds a read lock on state.mcp across
an await (state.mcp.read().await.call_tool(...).await), which can block writers;
instead, acquire the read lock only to perform a lookup/clone of the
MCP/Provider or the specific tool handle needed (e.g., get an Arc/clone of the
provider or ToolEntry), release the lock, then call call_tool(...) on the cloned
handle outside the lock; if Provider::call_tool ties lookup and invocation
together, modify the MCP API to separate lookup (e.g.,
get_provider_or_tool(&name) -> Arc<Provider/ToolHandle>) and invocation so you
can drop the read guard before awaiting the async call.
In `@src-tauri/src/modules/mcp/transport.rs`:
- Around line 81-108: The call method leaves the pending map entry for id in
self.pending when the timeout or receiver error occurs, leaking memory; ensure
the entry is removed in all exit paths: after creating the oneshot
(oneshot::channel) and inserting tx into self.pending, remove the pending entry
(self.pending.lock().await.remove(&id)) before returning on timeout (the
tokio::time::timeout Err branch), before returning on rx.map_err branch, and
also remove it once a successful resp is received (just after awaiting rx) so
the pending map never retains orphaned senders; reference symbols: call,
next_id, pending, id, tx/rx, tokio::time::timeout.
In `@src-tauri/src/modules/ollama/service.rs`:
- Around line 149-153: The extract_message function currently returns
Value::Null on missing "message", which lets a malformed 200 response silently
corrupt conversation state; change fn extract_message(body: &serde_json::Value)
to return Result<serde_json::Value, anyhow::Error> (or a suitable error type)
and return Err with a clear protocol error when "message" is absent, then update
the caller (the code that pushes the result into conversation history in
agent.rs) to propagate or handle the error instead of accepting Null so
malformed responses fail fast and are not appended to history.
In `@src/modules/mcp/components/AddServerForm.tsx`:
- Line 6: The form currently calls props.onAdd(name, entry) and immediately
resets/closes, which loses user input on backend failures; update the
AddServerForm's submit flow (e.g., the handleSubmit or onSubmit handler) to
await props.onAdd(...) and only call resetForm/clearInputs and
closeModal/props.onClose after the awaited promise resolves successfully,
wrapping the await in try/catch to handle rejections by showing an error
state/message and leaving the form open; alternatively, change the onAdd
contract to return a success boolean or throw on failure and act accordingly so
the form only clears/closes on confirmed success.
In `@src/modules/mcp/index.ts`:
- Around line 31-35: The code uses AbortSignal.timeout() directly in
fetchMcpConfig (and elsewhere in this module) which may throw on embedded
engines that lack that API; update fetchMcpConfig to feature-detect
AbortSignal.timeout (e.g. typeof AbortSignal !== 'undefined' && typeof
AbortSignal.timeout === 'function') and if available use it, otherwise create an
AbortController, call fetch with controller.signal and set a setTimeout to call
controller.abort() after timeoutMs (and clear the timer on success); apply the
same pattern to other functions in this module that currently call
AbortSignal.timeout() so platform fallbacks use AbortController and proper timer
cleanup to avoid spurious throws and misleading null/false/[] returns.
In `@src/modules/ollama/api/index.ts`:
- Around line 43-54: The helper setPreferredOllamaModel currently collapses all
failures to false; change its return shape from Promise<boolean> to something
that carries backend error details (e.g., Promise<{ ok: boolean; error?: string
}>), then in setPreferredOllamaModel check resp.ok and on failure parse the
response body (try JSON then fallback to text) to extract a validation/message
field and return { ok: false, error: extractedMessage }; also update the catch
to return { ok: false, error: (caught error).message } so callers (the
dashboard) can display the server-provided reason for rejection.
In `@src/pages/DashboardPage.tsx`:
- Around line 91-112: The status banner currently treats any non-"running"
service as offline because allRunning = services.every(s => s.status ===
"running"), so on first render when services are "checking" it shows red; update
the logic in DashboardPage (where allRunning is computed) to detect the initial
checking state (e.g., const anyChecking = services.some(s => s.status ===
"checking")) and use that to render a distinct neutral "checking" state: change
the conditional that sets the indicator classes and text (the span and the
paragraph that currently use allRunning) to first check anyChecking and render a
neutral color/text like "Checking services..." (fallback to existing allRunning
? "All systems running" : "Some services offline" when not checking).
---
Outside diff comments:
In `@src-tauri/src/modules/ollama/service.rs`:
- Around line 32-40: The call to OLLAMA_TAGS_URL in model_catalog() currently
returns Err on failure which wipes out an active model discovered earlier by
active_model(); change model_catalog() so failures from
client.get(OLLAMA_TAGS_URL) or non-success resp.status() are non-fatal: catch
the error, log or map it to a warning, skip tag parsing, and return the
existing/partial catalog (or Ok with the catalog sans tags) so active_model()
can still find the previously discovered active model; specifically modify the
code around the client.get(OLLAMA_TAGS_URL) / resp handling to avoid returning
Err and instead propagate a usable catalog/state to active_model().
---
Nitpick comments:
In `@doc/design/mcp.md`:
- Around line 17-29: The fenced ASCII diagram block (the code fence containing
"Telegram message" → "bot::service::text_handler" → "bot::agent::run_turn" etc.)
is missing a language tag; update that fenced code block to include a language
specifier such as "text" or "plaintext" so it satisfies markdownlint (e.g.,
change ``` to ```text) while leaving the diagram content unchanged.
- Around line 33-42: Update the Markdown directory-tree code block that starts
with "src-tauri/src/modules/mcp/" in doc/design/mcp.md to include a language
specifier by replacing the opening fence ``` with ```text so the tree is
rendered as a text block; the block contains the file list (mod.rs, protocol.rs,
types.rs, transport.rs, client.rs, registry.rs, service.rs).
In `@src-tauri/Cargo.toml`:
- Around line 37-39: Remove the redundant entries from the [dev-dependencies]
section in Cargo.toml by deleting the serde_json and tokio lines since
serde_json and tokio (with full features) are already declared under
[dependencies]; ensure only unique dev-only crates remain and that no other
dev-dependency references (serde_json or tokio) are required by tests before
removing.
In `@src-tauri/src/infrastructure/http_server.rs`:
- Around line 430-478: The handler handle_mcp_server_upsert uses a
load→modify→save sequence (mcp_service::load_or_init_config, cfg.servers.insert,
mcp_service::save_config, mcp_service::rebuild_registry_into_state) and is
vulnerable to TOCTOU races; protect this whole sequence with a shared async
mutex on the AppState (e.g., add a tokio::sync::Mutex or tokio::sync::RwLock
field like mcp_config_mutex in AppState) and acquire the lock at the start of
handle_mcp_server_upsert before calling load_or_init_config, hold it while
inserting into cfg and saving via save_config and while calling
rebuild_registry_into_state, then release it afterward so concurrent requests
serialize config mutations.
In `@src-tauri/src/modules/bot/agent.rs`:
- Around line 266-273: Replace the blind unwrap on tool_results.last().map(|(_,
c)| c.clone()).unwrap() with an explicit safety assertion: either call
expect(...) with a clear message (e.g., "tool_results must be non-empty here")
or keep the unwrap but add a brief comment above the line explaining the
invariant enforced earlier; update the fallback assignment in this block (where
fallback is set and returned in TurnResult) to use the expect message so the
intent and safety are explicit.
In `@src-tauri/src/modules/mcp/registry.rs`:
- Around line 66-71: all_tools() currently returns every ToolDef from providers
but tool_names() only returns names of tools that pass should_expose_to_model,
causing inconsistent behavior; update all_tools() to mirror tool_names() by
filtering ToolDef instances with the should_expose_to_model predicate (call the
same function used in tool_names()) before collecting, or alternatively add a
clear doc comment on all_tools() stating it returns unfiltered tools—ensure you
reference all_tools(), tool_names(), should_expose_to_model, providers and
tools() when making the change so the behavior is explicit and consistent.
In `@src-tauri/src/modules/mcp/transport.rs`:
- Around line 47-71: The stderr reader currently discards all output; instead,
capture and log each stderr line for debugging by replacing the empty consumer
in the tokio::spawn that reads from BufReader::new(stderr).lines() with code
that awaits lines.next_line().await, trims and skips empty lines, and logs the
content at debug/trace level (e.g., via tracing::debug! or log::debug!)
including context like "mcp stderr:"; keep the task non-blocking and handle
serde/unexpected errors by logging them rather than silently ignoring,
referencing the existing pending, BufReader, stderr, lines.next_line and
tokio::spawn identifiers to locate where to change behavior.
In `@src/modules/mcp/components/McpServerCard.tsx`:
- Around line 375-394: The list item mapping in McpServerCard uses the array
index as the React key which is unstable; change the key to use the unique path
string (use key={p}) and update the removal logic to accept a path rather than
an index: update the onRemove prop/signature (or add a new onRemoveByPath
handler) so that the remove action receives the path (or resolves the index via
paths.indexOf(path)) instead of relying on the mapped index; adjust any callers
of onRemove and the addPath logic that enforces uniqueness accordingly.
In `@src/modules/mcp/components/McpToolsPanel.tsx`:
- Around line 40-45: The pollTools function currently polls fetchMcpTools every
1s when no tools are returned, causing unnecessary steady-state requests; change
its backoff logic in pollTools so that when data.length === 0 you use a larger
idle interval (e.g., exponential/backoff or a fixed longer delay like 30s)
before scheduling timer = setTimeout(() => pollTools(), next), and ensure you
still use the shorter interval (10_000) when tools exist; keep handling of
cancelled and setTools unchanged and update any related timer clearing logic if
present to match the new interval behavior.
🪄 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: 671b5a6b-50f3-46d9-8f64-94aa2c9f2858
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.locksrc-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (36)
.gitignoredoc/design/README.mddoc/design/mcp.mdpackage.jsonsrc-tauri/Cargo.tomlsrc-tauri/capabilities/default.jsonsrc-tauri/mcp.example.jsonsrc-tauri/src/app.rssrc-tauri/src/infrastructure/http_server.rssrc-tauri/src/lib.rssrc-tauri/src/modules/bot/agent.rssrc-tauri/src/modules/bot/commands.rssrc-tauri/src/modules/bot/mod.rssrc-tauri/src/modules/bot/service.rssrc-tauri/src/modules/mcp/client.rssrc-tauri/src/modules/mcp/mod.rssrc-tauri/src/modules/mcp/native.rssrc-tauri/src/modules/mcp/protocol.rssrc-tauri/src/modules/mcp/registry.rssrc-tauri/src/modules/mcp/service.rssrc-tauri/src/modules/mcp/transport.rssrc-tauri/src/modules/mcp/types.rssrc-tauri/src/modules/mod.rssrc-tauri/src/modules/ollama/service.rssrc-tauri/src/shared/state.rssrc-tauri/tests/mcp_tools.rssrc/modules/bot/components/TerminalPreview.tsxsrc/modules/mcp/components/AddServerForm.tsxsrc/modules/mcp/components/McpServerCard.tsxsrc/modules/mcp/components/McpToolsPanel.tsxsrc/modules/mcp/index.tssrc/modules/ollama/api/index.tssrc/modules/ollama/index.tssrc/modules/ollama/types.tssrc/pages/DashboardPage.tsxsrc/shared/ui/TopMenu.tsx
… and configuration management - Updated MCP configuration to support multiple allowed filesystem paths, enhancing flexibility for tool operations. - Refactored API functions to handle arrays of paths, improving the management of filesystem settings. - Enhanced error handling in the Ollama service and DashboardPage, providing clearer feedback to users on model selection and configuration issues. - Improved server management components to handle asynchronous operations more robustly, ensuring better user experience during server addition and updates. - Adjusted logging and response handling in the agent to ensure consistent and informative output during tool interactions.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
src/modules/mcp/components/McpServerCard.tsx (2)
385-391: Consider adding aria-labels for accessibility.The remove button displays only "x" which may not be descriptive enough for screen reader users. Similarly, the "del" button at line 121 could benefit from clearer accessible names.
♻️ Suggested enhancement
<button type="button" onClick={() => onRemove(p)} + aria-label={`Remove folder ${p}`} className="shrink-0 font-mono text-[10px] text-rose-300/50 hover:text-rose-200" > x </button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/mcp/components/McpServerCard.tsx` around lines 385 - 391, The remove buttons in McpServerCard are not accessible because they rely on single characters ("x" and "del"); update the relevant <button> elements (the one calling onRemove(p) and the "del" button) to include descriptive aria-label attributes (e.g., aria-label={`Remove ${p.name || 'server'}`} or aria-label="Delete profile") and ensure any onClick handler references remain unchanged (onRemove). If appropriate, also add title attributes for tooltip support and verify screen readers announce the action clearly.
182-182: Consider deriving filesystem detection from currentargsTextstate.Currently
isFsis derived from the originalentry.args, so if a user manually edits the args textarea to remove the "server-filesystem" marker, theFolderHelperUI remains visible butupdatePathsbecomes a no-op (early return at line 202). This could cause minor confusion.This is a low-impact edge case since users typically wouldn't manually remove the filesystem marker, but deriving from
argsTextwould make the UI reactive to all edits.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/mcp/components/McpServerCard.tsx` at line 182, isFs is currently computed from the original entry.args via isFilesystemServer(entry) so the FolderHelper UI can remain visible when the user edits the args textarea (argsText) to remove the filesystem marker; change the computation to derive filesystem detection from the current argsText state instead (e.g., call isFilesystemServer on argsText or adapt isFilesystemServer to accept the argsText string) so the FolderHelper visibility and updatePaths behavior remain consistent with live user edits; update any places that rely on isFs (notably FolderHelper rendering and updatePaths early-return logic) to use the new argsText-derived value.src-tauri/src/modules/bot/agent.rs (2)
52-71: Synchronous config file read on every agent turn.
mcp_service::read_configperforms blocking I/O (std::fs::read_to_string) inside this async function. While acceptable for small config files at low request rates, this could block the async runtime at scale.Consider caching filesystem allowed paths in
AppState(updated when config changes) rather than reading from disk on every turn.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/modules/bot/agent.rs` around lines 52 - 71, The code builds fs_context by calling mcp_service::read_config(&state.mcp_config_path) inside an async agent turn, causing synchronous blocking I/O; instead cache the filesystem allowed paths in AppState (e.g., add a field like cached_filesystem_paths updated when the config file changes or on startup) and read from that cache here rather than calling mcp_service::read_config each time; keep using mcp_service::filesystem_allowed_paths to compute the cached value when updating AppState, and change fs_context to read from the new AppState cache (falling back to empty string) so agent turns avoid blocking std::fs I/O.
220-238: Lastdirect_returntool wins when multiple tools are called.If multiple tools in the same batch have
direct_return: true, lines 220-222 overwritedirect_replywith each one, and only the last tool's result is returned (lines 233-238). This may be intentional, but if not, earlier direct results are silently discarded.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/modules/bot/agent.rs` around lines 220 - 238, The current logic uses a single Option<String> direct_reply and overwrites it when multiple tools set direct_return, losing earlier results; change direct_reply to a Vec<String> (or similar collection), push result_text when is_direct is true, and when constructing the TurnResult for direct replies, join the collected strings (e.g., with "\n\n") into one text payload and return that with ReplySource::Tool; update any variable/type usages of direct_reply and any pattern matches that expect an Option to handle the Vec accordingly.src-tauri/src/modules/mcp/registry.rs (1)
95-112: Ambiguous tool resolution when server not specified.When
namedoesn't contain a dot (bare tool name),resolve_toolreturns the first matching tool across all providers. If multiple providers expose tools with the same name, this silently picks the first one based on provider registration order rather than signaling ambiguity.This may be intentional for simplicity, but consider logging a warning or documenting this behavior explicitly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/modules/mcp/registry.rs` around lines 95 - 112, resolve_tool currently returns the first match when a bare tool name is given, causing silent ambiguity if multiple providers expose the same tool; update resolve_tool to collect all matches across self.providers when server is None (use provider.tools().iter().find or filter), and if more than one provider matches return an Err indicating ambiguity (include the conflicting provider.server_name() values and the tool name), otherwise return the single matched (Provider, tool, def.direct_return). Keep existing behavior when server is specified (match provider.server_name() == s) and ensure Provider.clone() and def.direct_return are still used in the successful case.src-tauri/src/modules/ollama/service.rs (2)
17-59: Silent failures when Ollama endpoints are partially reachable.
model_catalogsilently swallows errors from both/api/psand/api/tagsendpoints (lines 22-30, 33-46). If/api/psfails but/api/tagssucceeds,activewill beNonewithout any indication of the failure. Similarly, if/api/tagsfails,modelswill be empty.The function only errors when both results are empty (lines 54-56), but partial failures could indicate connectivity issues that should be surfaced.
Consider logging a warning when individual endpoints fail but the overall result is still usable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/modules/ollama/service.rs` around lines 17 - 59, The model_catalog function currently swallows errors from OLLAMA_PS_URL and OLLAMA_TAGS_URL; update model_catalog to emit warning logs when either endpoint call/JSON parse/status check fails (but still proceed if the other endpoint yields usable data). Specifically, in the blocks that call client.get(OLLAMA_PS_URL) and client.get(OLLAMA_TAGS_URL) (and their resp.json::<serde_json::Value>().await steps and non-success status branch), capture the Err cases and non-success responses and call a warning logger (e.g., via log::warn! or tracing::warn!) including the URL (OLLAMA_PS_URL or OLLAMA_TAGS_URL) and the underlying error or status; keep the existing behavior of returning Err only when both active is None and models.is_empty(). Ensure logs reference the endpoint and whether the failure was a request error, non-success status, or JSON deserialization error so partial failures are visible.
128-131:tools_supportedreflects whether tools were sent, not model capability.When
has_toolsis false (empty tools array),tools_supportedwill befalse(line 130) even though the model might actually support tools. This is a semantic nuance - the flag indicates "tools were used in this request" rather than "model supports tools".The current behavior works for the agent loop (per context snippet), but consider renaming to
tools_usedortools_sentfor clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/modules/ollama/service.rs` around lines 128 - 131, The ChatResult is currently constructed with tools_supported: has_tools, but has_tools only indicates whether tools were sent in this request, not whether the model supports tools; rename the field to tools_used or tools_sent (e.g., ChatResult.tools_used) and update the construction site (where ChatResult { message: extract_message(&body)?, tools_used: has_tools }) and all consumers of ChatResult (agent loop and any call sites) to use the new name so the semantics clearly reflect "tools were sent/used in this request" rather than "model supports tools."src-tauri/src/modules/mcp/service.rs (1)
16-34: CWD-dependent config resolution may cause unexpected behavior.
resolve_mcp_config_pathrelies onstd::env::current_dir()which can vary depending on how the application is launched (IDE, terminal, installer, etc.). This could lead to different config files being used unexpectedly.Consider documenting this behavior clearly or preferring a deterministic path (e.g., always relative to the executable or always app-data) for production builds.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/modules/mcp/service.rs` around lines 16 - 34, The function resolve_mcp_config_path currently depends on std::env::current_dir() which makes config resolution non-deterministic; change it to avoid CWD: use std::env::current_exe() to derive the executable directory (or fall back to the provided store_path) when checking for project-scoped mcp.json, and only fall back to the app_data path if those checks fail; update logic in resolve_mcp_config_path to compute exe_dir via current_exe().parent(), check exe_dir.join("src-tauri").join("mcp.json") and exe_dir.join("mcp.json") instead of cwd, and keep the final fallback using store_path.parent().join("mcp.json").
🤖 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-tauri/src/modules/mcp/service.rs`:
- Around line 45-51: The save_config function currently swallows errors from
std::fs::create_dir_all (the let _ = ... call), causing misleading failures
later; update save_config to check/create the parent directory properly by
calling create_dir_all(parent).map_err(|e| format!("create parent dirs for
mcp.json: {e}"))? so directory-creation errors are propagated with context (use
the existing path.parent() check and return early on error) before
serializing/writing the config.
- Around line 151-165: The filesystem and server-delete handlers must acquire
the same mcp_config_mutex as handle_mcp_server_upsert to avoid races: in
handle_mcp_filesystem_put and handle_mcp_server_delete, obtain the
mcp_config_mutex (the same async write-lock used in handle_mcp_server_upsert)
before calling read_config/load_or_init_config and hold it until after
save_config completes, mirroring the lock scope/pattern in
rebuild_registry_into_state/handle_mcp_server_upsert so reads and writes cannot
interleave and overwrite each other.
In `@src/modules/ollama/api/index.ts`:
- Around line 55-61: The error handling currently calls resp.json() then falls
back to resp.text(), but resp.json() can consume the stream and prevent text
recovery; change the logic in the block that reads the response (the code using
resp, const body, and message) to first await resp.text() into a variable (e.g.,
raw), then try JSON.parse(raw) to extract body.error or body.message, and
finally set message = parsed?.error || parsed?.message || raw || `Request failed
(HTTP ${resp.status})`; return { ok: false, error: message } so you always
preserve the backend error text even if JSON parsing fails.
In `@src/pages/DashboardPage.tsx`:
- Around line 160-172: The select is currently disabled when
availableModels.length === 0 which prevents resetting a persisted selection to
the special "__active__" value; update the disabled logic on the select (where
savingModel, availableModels, and selectedModel are used) to only block
interaction when savingModel is true or when the catalog is empty AND the user
is not trying to choose the reset value (e.g. use disabled = savingModel ||
(availableModels.length === 0 && selectedModel !== "__active__")) so users can
still pick "Active" to reset to null even with an empty catalog.
---
Nitpick comments:
In `@src-tauri/src/modules/bot/agent.rs`:
- Around line 52-71: The code builds fs_context by calling
mcp_service::read_config(&state.mcp_config_path) inside an async agent turn,
causing synchronous blocking I/O; instead cache the filesystem allowed paths in
AppState (e.g., add a field like cached_filesystem_paths updated when the config
file changes or on startup) and read from that cache here rather than calling
mcp_service::read_config each time; keep using
mcp_service::filesystem_allowed_paths to compute the cached value when updating
AppState, and change fs_context to read from the new AppState cache (falling
back to empty string) so agent turns avoid blocking std::fs I/O.
- Around line 220-238: The current logic uses a single Option<String>
direct_reply and overwrites it when multiple tools set direct_return, losing
earlier results; change direct_reply to a Vec<String> (or similar collection),
push result_text when is_direct is true, and when constructing the TurnResult
for direct replies, join the collected strings (e.g., with "\n\n") into one text
payload and return that with ReplySource::Tool; update any variable/type usages
of direct_reply and any pattern matches that expect an Option to handle the Vec
accordingly.
In `@src-tauri/src/modules/mcp/registry.rs`:
- Around line 95-112: resolve_tool currently returns the first match when a bare
tool name is given, causing silent ambiguity if multiple providers expose the
same tool; update resolve_tool to collect all matches across self.providers when
server is None (use provider.tools().iter().find or filter), and if more than
one provider matches return an Err indicating ambiguity (include the conflicting
provider.server_name() values and the tool name), otherwise return the single
matched (Provider, tool, def.direct_return). Keep existing behavior when server
is specified (match provider.server_name() == s) and ensure Provider.clone() and
def.direct_return are still used in the successful case.
In `@src-tauri/src/modules/mcp/service.rs`:
- Around line 16-34: The function resolve_mcp_config_path currently depends on
std::env::current_dir() which makes config resolution non-deterministic; change
it to avoid CWD: use std::env::current_exe() to derive the executable directory
(or fall back to the provided store_path) when checking for project-scoped
mcp.json, and only fall back to the app_data path if those checks fail; update
logic in resolve_mcp_config_path to compute exe_dir via current_exe().parent(),
check exe_dir.join("src-tauri").join("mcp.json") and exe_dir.join("mcp.json")
instead of cwd, and keep the final fallback using
store_path.parent().join("mcp.json").
In `@src-tauri/src/modules/ollama/service.rs`:
- Around line 17-59: The model_catalog function currently swallows errors from
OLLAMA_PS_URL and OLLAMA_TAGS_URL; update model_catalog to emit warning logs
when either endpoint call/JSON parse/status check fails (but still proceed if
the other endpoint yields usable data). Specifically, in the blocks that call
client.get(OLLAMA_PS_URL) and client.get(OLLAMA_TAGS_URL) (and their
resp.json::<serde_json::Value>().await steps and non-success status branch),
capture the Err cases and non-success responses and call a warning logger (e.g.,
via log::warn! or tracing::warn!) including the URL (OLLAMA_PS_URL or
OLLAMA_TAGS_URL) and the underlying error or status; keep the existing behavior
of returning Err only when both active is None and models.is_empty(). Ensure
logs reference the endpoint and whether the failure was a request error,
non-success status, or JSON deserialization error so partial failures are
visible.
- Around line 128-131: The ChatResult is currently constructed with
tools_supported: has_tools, but has_tools only indicates whether tools were sent
in this request, not whether the model supports tools; rename the field to
tools_used or tools_sent (e.g., ChatResult.tools_used) and update the
construction site (where ChatResult { message: extract_message(&body)?,
tools_used: has_tools }) and all consumers of ChatResult (agent loop and any
call sites) to use the new name so the semantics clearly reflect "tools were
sent/used in this request" rather than "model supports tools."
In `@src/modules/mcp/components/McpServerCard.tsx`:
- Around line 385-391: The remove buttons in McpServerCard are not accessible
because they rely on single characters ("x" and "del"); update the relevant
<button> elements (the one calling onRemove(p) and the "del" button) to include
descriptive aria-label attributes (e.g., aria-label={`Remove ${p.name ||
'server'}`} or aria-label="Delete profile") and ensure any onClick handler
references remain unchanged (onRemove). If appropriate, also add title
attributes for tooltip support and verify screen readers announce the action
clearly.
- Line 182: isFs is currently computed from the original entry.args via
isFilesystemServer(entry) so the FolderHelper UI can remain visible when the
user edits the args textarea (argsText) to remove the filesystem marker; change
the computation to derive filesystem detection from the current argsText state
instead (e.g., call isFilesystemServer on argsText or adapt isFilesystemServer
to accept the argsText string) so the FolderHelper visibility and updatePaths
behavior remain consistent with live user edits; update any places that rely on
isFs (notably FolderHelper rendering and updatePaths early-return logic) to use
the new argsText-derived value.
🪄 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: 064edef1-b0a6-4487-b9c6-5611854d562f
⛔ Files ignored due to path filters (1)
src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
doc/design/mcp.mdsrc-tauri/Cargo.tomlsrc-tauri/src/app.rssrc-tauri/src/infrastructure/http_server.rssrc-tauri/src/modules/bot/agent.rssrc-tauri/src/modules/mcp/registry.rssrc-tauri/src/modules/mcp/service.rssrc-tauri/src/modules/mcp/transport.rssrc-tauri/src/modules/ollama/service.rssrc-tauri/src/shared/state.rssrc/modules/bot/components/SetupWizard.tsxsrc/modules/mcp/components/AddServerForm.tsxsrc/modules/mcp/components/McpServerCard.tsxsrc/modules/mcp/components/McpToolsPanel.tsxsrc/modules/mcp/index.tssrc/modules/ollama/api/index.tssrc/pages/DashboardPage.tsx
✅ Files skipped from review due to trivial changes (4)
- src-tauri/Cargo.toml
- src/modules/bot/components/SetupWizard.tsx
- src/modules/mcp/components/AddServerForm.tsx
- doc/design/mcp.md
🚧 Files skipped from review as they are similar to previous changes (6)
- src/modules/mcp/components/McpToolsPanel.tsx
- src-tauri/src/infrastructure/http_server.rs
- src-tauri/src/shared/state.rs
- src-tauri/src/modules/mcp/transport.rs
- src-tauri/src/app.rs
- src/modules/mcp/index.ts
- Refactored `McpServerCard` to enhance filesystem detection in live args and improve accessibility with aria-labels and titles for buttons. - Updated error handling in the `setPreferredOllamaModel` function to provide clearer feedback on request failures, including improved message extraction from responses. - Adjusted the `DashboardPage` to ensure model selection is only disabled when necessary, enhancing user experience. - Enhanced the agent's handling of tool responses to support multiple direct replies, improving interaction clarity.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src-tauri/src/modules/mcp/registry.rs (1)
95-135: Consider case-sensitivity for server name matching.The
resolve_toolmethod compares server names with exact equality (provider.server_name() != s). If server names in config could have inconsistent casing, this could lead to "tool not found" errors. Consider whether case-insensitive matching is desired for robustness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/modules/mcp/registry.rs` around lines 95 - 135, In resolve_tool, the server-name comparison uses exact equality which can fail on casing differences; change the checks that compare provider.server_name() and the parsed s to a case-insensitive comparison (for example use provider.server_name().eq_ignore_ascii_case(s) or normalize both to lowercase) and also trim whitespace from s before comparing so "Server" vs "server " won't fail; update the server-filtering loop and the ambiguous-case servers collection to still present original server_name() values but use the case-insensitive test when deciding matches.src/modules/mcp/components/McpServerCard.tsx (2)
227-235: Consider logging or providing feedback when folder picker fails.The
pickFolderfunction silently catches errors without any feedback. While this is acceptable for the "not running in desktop" case, other failures (e.g., user permission issues) might benefit from user feedback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/mcp/components/McpServerCard.tsx` around lines 227 - 235, The pickFolder function currently swallows all errors; update pickFolder to distinguish the "not running in desktop" case from other failures and surface feedback: catch the error from invoke("pick_mcp_filesystem_folder") in pickFolder, log the error (or call an existing UI feedback mechanism) with the error message and context, and for non-desktop environments keep the current silent behavior; use the existing addPath flow unchanged and reference the pickFolder function and invoke call to locate where to add the error handling and logging/user-feedback.
382-403: Duplicate paths would cause React key conflicts.Using
key={p}(line 386) means duplicate paths would create duplicate keys, causing React warnings and potential rendering issues. TheaddPathfunction (lines 214-221) does prevent duplicates, but consider using an index-based key as a defensive measure.♻️ Suggested defensive fix
- {paths.map((p) => ( + {paths.map((p, idx) => ( <div - key={p} + key={`${idx}-${p}`} className="flex items-center gap-2 rounded-md border border-white/8 bg-white/5 px-2 py-1" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/mcp/components/McpServerCard.tsx` around lines 382 - 403, The list rendering uses key={p} which can conflict if duplicate paths ever appear; update the map in the JSX (where paths.map is used) to include the index (e.g., map((p, i) => ...)) and build a stable unique key combining the path and index or another stable identifier (for example `${p}-${i}`) so each rendered item has a unique key; no other logic changes needed—keep the onRemove handler and display the same content.
🤖 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-tauri/src/infrastructure/http_server.rs`:
- Around line 417-429: handle_mcp_servers_list currently calls
load_or_init_config without acquiring the shared mcp_config_mutex; update the
function to acquire the async mutex on state.mcp_config_mutex (await the lock)
before calling load_or_init_config using State(state) and state.mcp_config_path,
perform the config read while holding the lock, then release the lock (drop
guard) before returning the Json(McpServersResponse) to ensure consistent reads
with other handlers that mutate the config.
---
Nitpick comments:
In `@src-tauri/src/modules/mcp/registry.rs`:
- Around line 95-135: In resolve_tool, the server-name comparison uses exact
equality which can fail on casing differences; change the checks that compare
provider.server_name() and the parsed s to a case-insensitive comparison (for
example use provider.server_name().eq_ignore_ascii_case(s) or normalize both to
lowercase) and also trim whitespace from s before comparing so "Server" vs
"server " won't fail; update the server-filtering loop and the ambiguous-case
servers collection to still present original server_name() values but use the
case-insensitive test when deciding matches.
In `@src/modules/mcp/components/McpServerCard.tsx`:
- Around line 227-235: The pickFolder function currently swallows all errors;
update pickFolder to distinguish the "not running in desktop" case from other
failures and surface feedback: catch the error from
invoke("pick_mcp_filesystem_folder") in pickFolder, log the error (or call an
existing UI feedback mechanism) with the error message and context, and for
non-desktop environments keep the current silent behavior; use the existing
addPath flow unchanged and reference the pickFolder function and invoke call to
locate where to add the error handling and logging/user-feedback.
- Around line 382-403: The list rendering uses key={p} which can conflict if
duplicate paths ever appear; update the map in the JSX (where paths.map is used)
to include the index (e.g., map((p, i) => ...)) and build a stable unique key
combining the path and index or another stable identifier (for example
`${p}-${i}`) so each rendered item has a unique key; no other logic changes
needed—keep the onRemove handler and display the same content.
🪄 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: e2d0f3c9-8460-457f-9d07-d245e434f086
📒 Files selected for processing (9)
src-tauri/src/infrastructure/http_server.rssrc-tauri/src/modules/bot/agent.rssrc-tauri/src/modules/mcp/registry.rssrc-tauri/src/modules/mcp/service.rssrc-tauri/src/modules/ollama/service.rssrc-tauri/src/shared/state.rssrc/modules/mcp/components/McpServerCard.tsxsrc/modules/ollama/api/index.tssrc/pages/DashboardPage.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pages/DashboardPage.tsx
- Added error handling for folder selection in `McpServerCard`, providing user feedback when folder picker fails. - Introduced a new state for managing folder selection errors, improving the user experience during folder operations. - Updated `FolderHelper` to display error messages related to folder picking, ensuring users are informed of issues. - Refactored server configuration loading in the HTTP server to use a mutex for improved concurrency handling.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
e2e/setup-dashboard.spec.ts (1)
111-125: Use predicate route matching for MCP mocks to avoid brittle E2E behavior.At Line 111–125, exact URL matching can miss legitimate calls like query-string variants (or adjacent MCP endpoints), making tests flaky depending on local environment.
Proposed fix
- await page.route(`${PENGINE_API_BASE}/v1/mcp/servers`, async (route) => { + await page.route((url) => url.href.startsWith(`${PENGINE_API_BASE}/v1/mcp/servers`), async (route) => { await route.fulfill({ status: 200, contentType: "application/json", body: JSON.stringify({ servers: {} }), }); }); - await page.route(`${PENGINE_API_BASE}/v1/mcp/tools`, async (route) => { + await page.route((url) => url.href.startsWith(`${PENGINE_API_BASE}/v1/mcp/tools`), async (route) => { await route.fulfill({ status: 200, contentType: "application/json", body: JSON.stringify([]), }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/setup-dashboard.spec.ts` around lines 111 - 125, The MCP mock routes use exact string matching which breaks for query strings or variant paths; change the two page.route calls to predicate-based matching by passing a function that inspects route.request().url() (or the url.href) and returns true for startsWith or RegExp matches against `${PENGINE_API_BASE}/v1/mcp/servers` and `${PENGINE_API_BASE}/v1/mcp/tools`, then fulfill the response inside that predicate handler (keep the same body/status). This updates the mocks for servers and tools to match query-string variants and adjacent endpoints while retaining the existing fulfill logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/setup-dashboard.spec.ts`:
- Around line 99-105: The PUT mock at page.route for
`${PENGINE_API_BASE}/v1/ollama/model` returns a generic `{ ok: true }` which
doesn't match the expected model-selection contract; update the fulfilled
response in that route handler to return the contract-shaped payload used by the
UI (e.g., include keys like `selected_model` and any other fields the UI reads
such as `available_models` or `version`), so tests exercise the same shape as
real responses and the model-selection flows in the code that reads the mock
(the route handler registered via page.route) behave correctly.
In `@src/modules/mcp/components/McpServerCard.tsx`:
- Around line 47-50: handleDelete currently closes the confirmation
(setConfirmDelete(false)) before awaiting onDelete(name), which loses the UI
context on failure; change handleDelete so it awaits onDelete(name) first and
only calls setConfirmDelete(false) after the delete resolves successfully (wrap
the await onDelete(name) in a try/catch and only close the dialog on success,
optionally surface the error on failure), referencing the handleDelete function,
setConfirmDelete, onDelete, and name.
- Around line 253-256: When parsing envText in McpServerCard.tsx, guard against
empty keys by trimming the left side before assignment: inside the for loop over
envText.split("\n") compute const key = line.slice(0, eq).trim(); if (key !==
"") then set env[key] = line.slice(eq + 1).trim(); otherwise skip that line (do
not assign env[""]). This ensures env and downstream code never receive an
empty-string environment variable name.
---
Nitpick comments:
In `@e2e/setup-dashboard.spec.ts`:
- Around line 111-125: The MCP mock routes use exact string matching which
breaks for query strings or variant paths; change the two page.route calls to
predicate-based matching by passing a function that inspects
route.request().url() (or the url.href) and returns true for startsWith or
RegExp matches against `${PENGINE_API_BASE}/v1/mcp/servers` and
`${PENGINE_API_BASE}/v1/mcp/tools`, then fulfill the response inside that
predicate handler (keep the same body/status). This updates the mocks for
servers and tools to match query-string variants and adjacent endpoints while
retaining the existing fulfill logic.
🪄 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: 14054e57-bf29-423c-9a4f-2ebbb3810334
📒 Files selected for processing (4)
e2e/setup-dashboard.spec.tssrc-tauri/src/infrastructure/http_server.rssrc-tauri/src/modules/mcp/registry.rssrc/modules/mcp/components/McpServerCard.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src-tauri/src/modules/mcp/registry.rs
- src-tauri/src/infrastructure/http_server.rs
- Improved error handling in `McpServerCard` during server deletion, providing user feedback on failure. - Updated API routing in `setup-dashboard.spec.ts` to handle model selection more robustly. - Refactored server and tool routing to use URL matching for better clarity and maintainability. - Enhanced inline editing form to prevent empty environment variable keys, ensuring data integrity.
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/modules/mcp/components/McpToolsPanel.tsx`:
- Around line 18-19: The component currently replaces prior data with
failure-returned null/[] from fetchMcpServers and fetchMcpTools, causing UI to
show "Loading…" or "No MCP tools connected" on transient backend errors; update
McpToolsPanel to track explicit loading and error states (e.g.,
toolsLoading/toolsError and serversLoading/serversError) instead of inferring
from tools/servers, and change state updates so setTools and setServers only
replace the stored snapshot on success while preserving the last-good snapshot
on failure; ensure fetch handlers check the return values from fetchMcpTools and
fetchMcpServers and set the new error flags accordingly so UI can render
previous data plus an error indicator rather than a false empty/loading state.
- Line 115: The notice paragraph in McpToolsPanel (the JSX expression {notice &&
<p ...>}) should be turned into an ARIA live region so screen readers announce
updates: add properties such as aria-live="polite" (or "assertive" if
immediate), role="status" and aria-atomic="true" to the <p> that renders
{notice} to ensure changes after add/save/delete are communicated to assistive
tech.
- Around line 24-27: The reload function and pollTools/fetchMcpServers calls can
be out-of-order and overwrite fresher state; replace the simple cancelled flag
with per-operation tracking (e.g., a monotonically-incremented requestId or
AbortController) so that only the latest inflight
fetchMcpTools()/fetchMcpServers() responses call setTools/setServers; implement
requestId increment at the start of reload() and pollTools(), pass the id to the
response handlers and ignore results whose id does not match the most recent, or
use AbortController to cancel previous fetches before starting a new one, and
remove/replace the current cancelled logic accordingly.
🪄 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: c62ff238-42a8-4335-b52d-faa4cf49332a
📒 Files selected for processing (3)
e2e/setup-dashboard.spec.tssrc/modules/mcp/components/McpServerCard.tsxsrc/modules/mcp/components/McpToolsPanel.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- e2e/setup-dashboard.spec.ts
- src/modules/mcp/components/McpServerCard.tsx
- Updated `fetchMcpTools` to return `null` on request failures, enhancing error handling. - Refactored `McpToolsPanel` to manage loading states and errors for tools and servers, providing user feedback during data fetching. - Introduced polling mechanism for tools with dynamic loading intervals based on availability, improving user experience. - Enhanced accessibility by adding ARIA roles to status messages for better screen reader support.
This was referenced Apr 11, 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.
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests