feat: integrate Tool Engine for container management and runtime dete…#11
Merged
Conversation
…ction - Added Tool Engine panel to the Dashboard for managing container tools. - Implemented runtime detection for Podman and Docker, providing user feedback on installation status. - Enhanced MCP server management to support workspace roots and filesystem paths. - Improved error handling and user notifications during tool installation and uninstallation processes. - Updated Setup Wizard to include container runtime installation as a prerequisite.
Closed
📝 WalkthroughWalkthroughAdds a Tool Engine (runtime detection, catalog, install/uninstall), containerized file-manager image and build tooling, migrates legacy MCP filesystem config into Changes
Sequence Diagram(s)sequenceDiagram
participant Frontend as Client
participant App as Backend
participant ToolSvc as Tool Engine Service
participant Runtime as Container Runtime
participant MCP as MCP Service / Registry
Frontend->>App: GET /v1/toolengine/runtime
App->>ToolSvc: detect_runtime()
ToolSvc->>Runtime: probe podman/docker (version, rootless)
Runtime-->>ToolSvc: RuntimeInfo
ToolSvc-->>App: RuntimeStatus
App-->>Frontend: RuntimeStatus
sequenceDiagram
participant Frontend as Client
participant App as Backend
participant ToolSvc as Tool Engine Service
participant Runtime as Container Runtime
participant MCP as MCP Service / Registry
Frontend->>App: POST /v1/toolengine/install {tool_id}
App->>ToolSvc: install_tool(tool_id)
ToolSvc->>Runtime: ensure image (inspect → pull → build)
Runtime-->>ToolSvc: image ready / digest checked
ToolSvc->>MCP: persist ServerEntry (stdio) in mcp.json
MCP-->>ToolSvc: saved
ToolSvc-->>App: install success
App->>MCP: spawn rebuild_registry_into_state(state) (async)
App-->>Frontend: 200 OK
par Background
MCP->>MCP: connect_one_server(file-manager)
MCP->>Runtime: start provider process
Runtime-->>MCP: provider ready
MCP->>MCP: publish provider -> registry updated
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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 docstrings
🧪 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: 12
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/mcp/transport.rs (1)
105-143:⚠️ Potential issue | 🟠 MajorClean up
pendingon stdin IO failure.Line 117 stores the sender before the request is written. If Line 122 or Line 125 fails, this returns early without removing that entry, so repeated transport failures will leak pending requests indefinitely.
Suggested fix
let (tx, rx) = oneshot::channel(); self.pending.lock().await.insert(id, tx); - { + let io_result: Result<(), String> = { let mut stdin = self.stdin.lock().await; - stdin - .write_all(&payload) - .await - .map_err(|e| format!("write stdin: {e}"))?; - stdin.flush().await.map_err(|e| format!("flush: {e}"))?; - } + async { + stdin + .write_all(&payload) + .await + .map_err(|e| format!("write stdin: {e}"))?; + stdin.flush().await.map_err(|e| format!("flush: {e}"))?; + Ok(()) + } + .await + }; + if let Err(e) = io_result { + self.pending.lock().await.remove(&id); + return Err(e); + } let secs = timeout.as_secs().max(1);🤖 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 105 - 143, call_with_timeout inserts a sender into self.pending before writing to stdin, but on write_all or flush errors the function returns early without removing that entry, leaking pending requests; fix by ensuring the pending entry for id is removed on any early return from the write/flush error paths (either move the insert to after successful write/flush, or add cleanup that locks self.pending and removes id before returning on errors from stdin.write_all or stdin.flush), referencing call_with_timeout, self.pending, id, stdin.write_all and stdin.flush to locate the spots to change.
🧹 Nitpick comments (8)
src/shared/ui/WizardLayout.tsx (1)
66-66: LGTM — layout correctly updated for 5-step wizard.The change from
xl:grid-cols-4toxl:grid-cols-5properly accommodates the new container runtime installation step mentioned in the PR objectives.Optional: Consider adding an intermediate breakpoint for smoother responsive behavior.
Currently, the grid jumps from 2 columns (sm) to 5 columns (xl). Between 640px and 1280px, 5 items in a 2-column grid creates an asymmetric 2+2+1 layout. Adding
md:grid-cols-3orlg:grid-cols-5would provide a more balanced visual transition on medium and large screens.♻️ Optional: Add intermediate breakpoint
- <ol className="grid gap-3 sm:grid-cols-2 xl:grid-cols-5" aria-label="Setup steps"> + <ol className="grid gap-3 sm:grid-cols-2 lg:grid-cols-5" aria-label="Setup steps">or
- <ol className="grid gap-3 sm:grid-cols-2 xl:grid-cols-5" aria-label="Setup steps"> + <ol className="grid gap-3 sm:grid-cols-2 md:grid-cols-3 lg:grid-cols-5" aria-label="Setup steps">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/ui/WizardLayout.tsx` at line 66, The ordered list in WizardLayout.tsx currently uses "grid gap-3 sm:grid-cols-2 xl:grid-cols-5" which causes a jarring layout between sm and xl; update the className on the <ol> (in the WizardLayout component) to include an intermediate breakpoint such as "md:grid-cols-3" or "lg:grid-cols-5" (e.g., "grid gap-3 sm:grid-cols-2 md:grid-cols-3 xl:grid-cols-5") to produce a smoother responsive transition while keeping the aria-label intact.src-tauri/src/modules/tool_engine/container/file-manager/build (1)
4-7: Add explicit fallback error when no runtime is installed.Current behavior emits a generic
execfailure if both runtimes are missing. A direct check gives clearer operator feedback.🛠️ Suggested update
if command -v podman >/dev/null 2>&1; then exec podman build -t file-manager:0.1.0 . fi -exec docker build -t file-manager:0.1.0 . +if command -v docker >/dev/null 2>&1; then + exec docker build -t file-manager:0.1.0 . +fi +echo "Neither podman nor docker is installed or available in PATH." >&2 +exit 127🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/modules/tool_engine/container/file-manager/build` around lines 4 - 7, The script currently falls through to an `exec` failure when neither podman nor docker exists; update the build script to explicitly detect absence of both runtimes by using `command -v podman` and `command -v docker` checks and, if both are missing, print a clear error message to stderr (e.g., “No container runtime found: install docker or podman”) and exit with a non-zero status instead of attempting `exec` and relying on a generic failure; keep the existing branches that `exec podman build -t file-manager:0.1.0 .` and `exec docker build -t file-manager:0.1.0 .` when those commands are present.src-tauri/src/modules/mcp/registry.rs (2)
232-234: Minor: helper adds little value.
ollama_tool_descriptionsimply clones and unwraps to default. The inline expressiont.description.clone().unwrap_or_default()at the call site would be equally clear without the extra indirection.🤖 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 232 - 234, The helper function ollama_tool_description simply returns t.description.clone().unwrap_or_default(); remove the function and replace its call sites with the inline expression t.description.clone().unwrap_or_default() (referencing the ToolDef type) so callers directly use the description clone/unwrapping logic; delete ollama_tool_description and update any usages accordingly.
17-18: Mapping/mcpto/tmpmay cause confusion.When the model uses
/mcp(the container WORKDIR), it gets silently rewritten to/tmp. This could lead to unexpected behavior where the model thinks it's writing to one location but data ends up elsewhere. Consider logging a warning or returning an error for this case instead.🤖 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 17 - 18, The current mapping silently rewrites the container WORKDIR "/mcp" to "/tmp" (check the conditional comparing t == "/mcp" that returns "/tmp"), which is confusing; update the logic to avoid silent remapping by either (A) returning an error/Result::Err with a clear message indicating that mapping "/mcp" is disallowed, or (B) logging a warning via the module's logger and returning the original "/mcp" (or refusing to remap) instead of returning "/tmp"; adjust the surrounding function signature (e.g., change to Result<...>) if needed and ensure the error/warning message references "/mcp" so callers can handle it.src/modules/toolengine/components/ToolEnginePanel.tsx (2)
199-200: Minor: "Docker command list" label may be inaccurate.The label says "Docker command list" but the system supports both Podman and Docker. Consider using "Container commands" or "MCP tools" for accuracy.
📝 Suggested label change
- <p className="truncate text-xs font-semibold text-white/90"> - Docker command list + <p className="truncate text-xs font-semibold text-white/90"> + Container tools🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/toolengine/components/ToolEnginePanel.tsx` around lines 199 - 200, The label string in the JSX paragraph element inside ToolEnginePanel (the <p className="truncate text-xs font-semibold text-white/90"> node currently rendering "Docker command list") is inaccurate because the panel supports both Podman and Docker; update that text to a neutral term such as "Container commands" (or "MCP tools") to reflect both runtimes so locate the <p ...> element in ToolEnginePanel.tsx and replace the inner text accordingly.
25-37: Runtime state preserved when fetch returns null.At line 30, if
fetchRuntimeStatus()returnsnull, the previousruntimestate is kept. This differs fromcataloghandling (lines 34-36) wherenullshows an error. Consider whether a failed runtime check should clear the state or show an error.🔧 Consistent error handling for runtime fetch
if (cancelledRef.current || id !== seqRef.current) return; setLoading(false); - if (rt !== null) setRuntime(rt); + setRuntime(rt); // null means detection failed, show "not found" state if (cat !== null) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/toolengine/components/ToolEnginePanel.tsx` around lines 25 - 37, The loadData callback treats a null runtime differently than catalog: if fetchRuntimeStatus() returns null it leaves the previous runtime state instead of clearing it or surfacing an error; update loadData (the function using fetchRuntimeStatus, setRuntime, fetchToolCatalog, setCatalog, setCatalogError) so that when rt === null you call setRuntime(null) and also either set a runtime error state (e.g., setRuntimeError("Could not load runtime status")) or clear any existing runtime error, mirroring the catalog handling; add the runtime error state and its setter if not present and ensure you update canceled/seq checks remain unchanged.src-tauri/src/modules/tool_engine/tools.json (1)
10-10: Empty digest disables image integrity verification.The empty
digestfield means the image tag (file-manager:0.1.0) can be replaced without detection. For local development this is fine, but for production consider populating this with the actual image digest to prevent tampering.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/modules/tool_engine/tools.json` at line 10, The empty "digest" field for the image entry (the digest key next to the image tag "file-manager:0.1.0") disables integrity verification; fix it by replacing the empty string with the image's actual content digest (e.g., sha256:...) obtained from your registry or by pulling the image and inspecting its repoDigest, and ensure the "digest" value exactly matches the registry-provided digest for that image tag so tampered images are detected.src-tauri/src/infrastructure/http_server.rs (1)
660-673: Lock ordering:tool_engine_mutexthenmcp_config_mutex.The install handler acquires
tool_engine_mutex(line 661) thenmcp_config_mutex(line 668). Ensure all other handlers follow the same ordering to prevent deadlocks. The uninstall handler at lines 715-722 follows the same pattern, which is good.🤖 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 660 - 673, The install handler currently locks tool_engine_mutex then mcp_config_mutex (see the block acquiring state.tool_engine_mutex.lock().await then state.mcp_config_mutex.lock().await); ensure all other handlers follow this same lock ordering to avoid deadlocks by auditing and updating any handlers that acquire these two mutexes in the opposite order (e.g., change places where mcp_config_mutex is locked before tool_engine_mutex to lock tool_engine_mutex first or refactor to hold only one lock at a time); specifically review the install and uninstall flows and any functions that touch both state.tool_engine_mutex and state.mcp_config_mutex and make their acquisition order consistent.
🤖 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 422-425: The current fire-and-forget tokio::spawn calls that
invoke mcp_service::rebuild_registry_into_state(&bg).await hide failures from
API clients; modify those call sites (the tokio::spawn blocks using state.clone)
to capture and surface errors instead of immediately returning HTTP 200: either
await the future in the request handler and return an error response on failure,
or keep the background task but forward any Err returned by
mcp_service::rebuild_registry_into_state to the frontend via your existing
SSE/event mechanism (emit an error-severity event) and flip a shared health
flag/state that your health endpoint reads; update all instances invoking
mcp_service::rebuild_registry_into_state (including the other similar
tokio::spawn sites) to follow this pattern so rebuild failures are observable by
clients.
- Around line 669-672: The code silently returns an empty host_paths when
mcp_service::read_config(&state.mcp_config_path) fails; change the match to
capture the error (e.g., Err(e)) and log it (using the existing logger in scope)
before returning Vec::new(), referencing mcp_service::read_config,
mcp_service::filesystem_allowed_paths and state.mcp_config_path so the location
is clear; if the surrounding function must be async to await a logger or use
async logging, restructure the block accordingly (or call a synchronous logger)
so the error is not swallowed silently.
In `@src-tauri/src/modules/mcp/registry.rs`:
- Around line 7-34: The function rewrite_file_manager_path currently returns
paths containing ".." unchanged; update rewrite_file_manager_path to
canonicalize and reject/sanitize path traversal by first normalizing backslashes
to "/" then splitting on '/' and resolving components: skip empty and "." parts,
handle ".." by popping the last added segment (and if a ".." would escape the
intended root, return a safe fallback like "/app" or an explicit error), and
finally join the resolved segments and prepend "/app" so inputs like
"pengine/../../etc" cannot produce escapes; apply this logic while preserving
the existing early returns for absolute (leading '/') and special prefixes
("/opt/mcp-filesystem", "/mcp", "/mcp/") to keep their behavior.
In `@src-tauri/src/modules/tool_engine/container/file-manager/Dockerfile`:
- Around line 1-8: The Dockerfile currently runs as root; create a non-root user
and switch to it before runtime by adding steps that create a user/group (e.g.,
mcp user), change ownership of the WORKDIR (/mcp) and any installed files (npm
output under /mcp/node_modules), and add a USER line so the container runs as
that user; ensure these changes are applied after RUN npm ci and before
ENTRYPOINT so ENTRYPOINT ["node",
"/mcp/node_modules/@modelcontextprotocol/server-filesystem/dist/index.js"]
executes as the non-root user.
In `@src-tauri/src/modules/tool_engine/service.rs`:
- Around line 324-327: The load/modify/save cycle that updates mcp.json (calls
to mcp_service::load_or_init_config and mcp_service::save_config that insert
server_key(tool_id)/server_entry) must be protected by the MCP config lock to
avoid races; wrap the read-modify-write sequence in the mcp_config_mutex (the
same mutex used for MCP edits) so that both occurrences (the block around
cfg.servers.insert(...) and the similar block later at lines ~389-392) acquire
the mutex before calling mcp_service::load_or_init_config, perform
cfg.servers.insert(server_key(tool_id), server_entry), then call
mcp_service::save_config, and finally release the mutex. Ensure you use the
existing mcp_config_mutex accessor (or add one if missing) rather than the
tool_engine_mutex.
- Around line 347-375: The code currently treats any runtime.binary mismatch as
drift and replaces the installed ServerEntry::Stdio command with runtime.binary;
instead, when cfg.servers.get(&key) returns Some(ServerEntry::Stdio { command:
cmd, args: cur, .. }) preserve cmd and only update args (recompute argv via
podman_run_argv_for_tool using host_paths) plus keep direct_return and env; only
set command = runtime.binary when inserting a brand-new entry (i.e. when
cfg.servers.get(&key) is None). Adjust the equality/check logic so you do not
overwrite an existing command (use cmd.clone() when updating args) and continue
to pull direct_return/env from the existing entry as you already do.
In `@src/modules/bot/components/SetupWizard.tsx`:
- Around line 26-30: The step title "Install Podman" in the SetupWizard steps
array is inconsistent with the runtime check that accepts Podman or Docker;
update the step object (the item with title "Install Podman" in SetupWizard) to
a neutral phrasing like "Install a container runtime" and adjust its summary if
needed to mention "Podman or Docker" so the displayed copy matches the actual
validation logic (the runtime acceptance implemented around the runtime check
that currently allows Podman or Docker).
In `@src/modules/bot/components/SetupWizardSteps.tsx`:
- Around line 433-448: The QR/link is built from the original
verifiedBot.bot_username so edits to botUsername don't affect the displayed
StyledQrCode; update the code so telegramBotUrl is derived from the current
editable value (botUsername) falling back to verifiedBot.bot_username, or
recompute telegramBotUrl inside this component before rendering StyledQrCode;
ensure the onBotUsernameChange state is the source of truth used when
constructing telegramBotUrl (and update any prop passed from SetupWizard.tsx if
telegramBotUrl was computed there) so StyledQrCode and the displayed link
reflect the live botUsername input.
- Around line 224-230: The banner renders literal "undefined" if
RuntimeStatus.kind or RuntimeStatus.version are missing; update the
SetupWizardSteps.tsx rendering to guard or coalesce those optional fields
(runtimeStatus.kind and runtimeStatus.version) before interpolation — e.g. use
conditional rendering or fallback strings like '(unknown)' or an empty string
(runtimeStatus.kind ?? 'unknown') and (runtimeStatus.version ?? '') when
composing the display line so the UI never shows "undefined".
- Around line 40-46: The bot token input renders credentials in plain text;
update the input with id "token" in SetupWizardSteps.tsx (the element using
value={botToken} and onChange={(event) => onBotTokenChange(event.target.value)})
to conceal the token by changing it to a password-style field (type="password")
and optionally add a visible/hidden toggle control that flips the input type for
user convenience while keeping the default masked; ensure the onBotTokenChange
handler and value binding remain unchanged so state flow is preserved.
In `@src/modules/mcp/components/McpServerCard.tsx`:
- Around line 32-50: appMountPathsForFolders currently implements its own
basename/sanitizer logic and can diverge from the backend
workspace_app_bind_pairs (e.g., drive roots and non-ASCII names); update
appMountPathsForFolders to mirror the backend algorithm exactly by reusing the
same name-generation rules: derive the mount label the same way
workspace_app_bind_pairs does (handle drive root/empty basenames, apply the same
character-replacement/normalization rules, and duplicate-suffix logic), and keep
the same duplicate-resolution loop that produces `_1`, `_2`, … so the produced
`/app/<name>` strings always match the Rust implementation.
- Around line 337-344: The current applyTeFolders flow rejects an empty tePaths
array (setting setTeApplyError when tePaths.length === 0) and the submit control
is also disabled when tePaths is empty, which prevents clearing all mounts
despite backend support; modify applyTeFolders to allow an empty tePaths (remove
the tePaths.length === 0 early-return and the setTeApplyError call) so
putMcpFilesystemPaths(tePaths, 60_000) is called with [] when needed, and update
the submit/button disabled logic (the UI check that disables submission based on
tePaths.length === 0) to not block submission for an empty list while preserving
other disables like teApplyBusy.
---
Outside diff comments:
In `@src-tauri/src/modules/mcp/transport.rs`:
- Around line 105-143: call_with_timeout inserts a sender into self.pending
before writing to stdin, but on write_all or flush errors the function returns
early without removing that entry, leaking pending requests; fix by ensuring the
pending entry for id is removed on any early return from the write/flush error
paths (either move the insert to after successful write/flush, or add cleanup
that locks self.pending and removes id before returning on errors from
stdin.write_all or stdin.flush), referencing call_with_timeout, self.pending,
id, stdin.write_all and stdin.flush to locate the spots to change.
---
Nitpick comments:
In `@src-tauri/src/infrastructure/http_server.rs`:
- Around line 660-673: The install handler currently locks tool_engine_mutex
then mcp_config_mutex (see the block acquiring
state.tool_engine_mutex.lock().await then state.mcp_config_mutex.lock().await);
ensure all other handlers follow this same lock ordering to avoid deadlocks by
auditing and updating any handlers that acquire these two mutexes in the
opposite order (e.g., change places where mcp_config_mutex is locked before
tool_engine_mutex to lock tool_engine_mutex first or refactor to hold only one
lock at a time); specifically review the install and uninstall flows and any
functions that touch both state.tool_engine_mutex and state.mcp_config_mutex and
make their acquisition order consistent.
In `@src-tauri/src/modules/mcp/registry.rs`:
- Around line 232-234: The helper function ollama_tool_description simply
returns t.description.clone().unwrap_or_default(); remove the function and
replace its call sites with the inline expression
t.description.clone().unwrap_or_default() (referencing the ToolDef type) so
callers directly use the description clone/unwrapping logic; delete
ollama_tool_description and update any usages accordingly.
- Around line 17-18: The current mapping silently rewrites the container WORKDIR
"/mcp" to "/tmp" (check the conditional comparing t == "/mcp" that returns
"/tmp"), which is confusing; update the logic to avoid silent remapping by
either (A) returning an error/Result::Err with a clear message indicating that
mapping "/mcp" is disallowed, or (B) logging a warning via the module's logger
and returning the original "/mcp" (or refusing to remap) instead of returning
"/tmp"; adjust the surrounding function signature (e.g., change to Result<...>)
if needed and ensure the error/warning message references "/mcp" so callers can
handle it.
In `@src-tauri/src/modules/tool_engine/container/file-manager/build`:
- Around line 4-7: The script currently falls through to an `exec` failure when
neither podman nor docker exists; update the build script to explicitly detect
absence of both runtimes by using `command -v podman` and `command -v docker`
checks and, if both are missing, print a clear error message to stderr (e.g.,
“No container runtime found: install docker or podman”) and exit with a non-zero
status instead of attempting `exec` and relying on a generic failure; keep the
existing branches that `exec podman build -t file-manager:0.1.0 .` and `exec
docker build -t file-manager:0.1.0 .` when those commands are present.
In `@src-tauri/src/modules/tool_engine/tools.json`:
- Line 10: The empty "digest" field for the image entry (the digest key next to
the image tag "file-manager:0.1.0") disables integrity verification; fix it by
replacing the empty string with the image's actual content digest (e.g.,
sha256:...) obtained from your registry or by pulling the image and inspecting
its repoDigest, and ensure the "digest" value exactly matches the
registry-provided digest for that image tag so tampered images are detected.
In `@src/modules/toolengine/components/ToolEnginePanel.tsx`:
- Around line 199-200: The label string in the JSX paragraph element inside
ToolEnginePanel (the <p className="truncate text-xs font-semibold
text-white/90"> node currently rendering "Docker command list") is inaccurate
because the panel supports both Podman and Docker; update that text to a neutral
term such as "Container commands" (or "MCP tools") to reflect both runtimes so
locate the <p ...> element in ToolEnginePanel.tsx and replace the inner text
accordingly.
- Around line 25-37: The loadData callback treats a null runtime differently
than catalog: if fetchRuntimeStatus() returns null it leaves the previous
runtime state instead of clearing it or surfacing an error; update loadData (the
function using fetchRuntimeStatus, setRuntime, fetchToolCatalog, setCatalog,
setCatalogError) so that when rt === null you call setRuntime(null) and also
either set a runtime error state (e.g., setRuntimeError("Could not load runtime
status")) or clear any existing runtime error, mirroring the catalog handling;
add the runtime error state and its setter if not present and ensure you update
canceled/seq checks remain unchanged.
In `@src/shared/ui/WizardLayout.tsx`:
- Line 66: The ordered list in WizardLayout.tsx currently uses "grid gap-3
sm:grid-cols-2 xl:grid-cols-5" which causes a jarring layout between sm and xl;
update the className on the <ol> (in the WizardLayout component) to include an
intermediate breakpoint such as "md:grid-cols-3" or "lg:grid-cols-5" (e.g.,
"grid gap-3 sm:grid-cols-2 md:grid-cols-3 xl:grid-cols-5") to produce a smoother
responsive transition while keeping the aria-label intact.
🪄 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: 7d00c05f-abed-4bff-9fc6-e4747b19d98b
⛔ Files ignored due to path filters (1)
src-tauri/src/modules/tool_engine/container/file-manager/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (34)
src-tauri/.gitignoresrc-tauri/mcp.example.jsonsrc-tauri/src/app.rssrc-tauri/src/infrastructure/http_server.rssrc-tauri/src/modules/bot/agent.rssrc-tauri/src/modules/mcp/client.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/tool_engine/container/file-manager/Dockerfilesrc-tauri/src/modules/tool_engine/container/file-manager/buildsrc-tauri/src/modules/tool_engine/container/file-manager/package.jsonsrc-tauri/src/modules/tool_engine/mod.rssrc-tauri/src/modules/tool_engine/runtime.rssrc-tauri/src/modules/tool_engine/service.rssrc-tauri/src/modules/tool_engine/tools.jsonsrc-tauri/src/modules/tool_engine/types.rssrc-tauri/src/shared/state.rssrc-tauri/tests/mcp_tools.rssrc/modules/bot/components/SetupWizard.tsxsrc/modules/bot/components/SetupWizardSteps.tsxsrc/modules/mcp/components/McpServerCard.tsxsrc/modules/mcp/components/McpToolsPanel.tsxsrc/modules/mcp/index.tssrc/modules/toolengine/components/ToolEnginePanel.tsxsrc/modules/toolengine/index.tssrc/pages/DashboardPage.tsxsrc/pages/SetupPage.tsxsrc/shared/api/config.tssrc/shared/mcpEvents.tssrc/shared/ui/WizardLayout.tsx
💤 Files with no reviewable changes (1)
- src-tauri/src/modules/mcp/protocol.rs
…management - Updated Setup Wizard to include detailed steps for installing Podman or Docker as container runtimes. - Enhanced mock API responses for container runtime detection in end-to-end tests. - Improved error handling and user feedback in Tool Engine panel for runtime status. - Refactored components to streamline workspace mount path management and improve user experience during setup.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
e2e/setup-dashboard.spec.ts (1)
123-134: Make runtime mock route resilient to query params.Using an exact URL at Line 123 can miss requests like
/v1/toolengine/runtime?refresh=1, causing flaky tests.Suggested fix
- await page.route(`${PENGINE_API_BASE}/v1/toolengine/runtime`, async (route) => { + await page.route( + (url) => url.href.startsWith(`${PENGINE_API_BASE}/v1/toolengine/runtime`), + async (route) => { await route.fulfill({ status: 200, contentType: "application/json", body: JSON.stringify({ available: true, kind: "podman", version: "5.0.0", rootless: true, }), }); - }); + }, + );🤖 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 123 - 134, The route matching for the runtime mock uses an exact URL string in the page.route call, which fails when the request includes query params (e.g., ?refresh=1); update the page.route matcher to be resilient to query strings by matching the path portion instead of exact URL — for example change the matcher passed to page.route (currently `${PENGINE_API_BASE}/v1/toolengine/runtime`) to a pattern/RegExp or a predicate that checks route.request().url startsWith or its pathname equals `/v1/toolengine/runtime`, then keep the existing route.fulfill handler (the mock response) inside that same async callback so requests with any query params still get fulfilled.src/modules/bot/components/SetupWizardSteps.tsx (1)
503-510: Avoid rendering a no-op “Open dashboard” button.At Line 503, the button is shown even when
onCompleteSetupis undefined; in that case it does nothing on click.Suggested fix
- {connectStatus === "connected" && ( + {connectStatus === "connected" && onCompleteSetup && ( <button type="button" className="primary-button mt-5 w-full rounded-xl text-xs" - onClick={() => onCompleteSetup?.()} + onClick={onCompleteSetup} > Open dashboard </button> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/bot/components/SetupWizardSteps.tsx` around lines 503 - 510, The "Open dashboard" button is rendered even when onCompleteSetup is undefined, producing a no-op; update the render condition in SetupWizardSteps so the button only appears when connectStatus === "connected" AND onCompleteSetup is defined (e.g., change the JSX conditional to {connectStatus === "connected" && onCompleteSetup && ( ... )}), so the button is not shown when clicking would do nothing.
🤖 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/bot/components/SetupWizardSteps.tsx`:
- Around line 142-144: The "Ready to continue." message is currently shown
whenever ollamaModel is truthy even if ollamaReachable is false; update the JSX
in SetupWizardSteps (the conditional rendering that uses ollamaModel) to require
both ollamaModel and ollamaReachable to be true (e.g., change the condition to
ollamaModel && ollamaReachable) so the ready message is only displayed when the
model exists and the service is reachable; keep the existing element (<p
className="mt-3 font-mono text-xs text-emerald-300">Ready to continue.</p>) but
render it only under the combined condition.
---
Nitpick comments:
In `@e2e/setup-dashboard.spec.ts`:
- Around line 123-134: The route matching for the runtime mock uses an exact URL
string in the page.route call, which fails when the request includes query
params (e.g., ?refresh=1); update the page.route matcher to be resilient to
query strings by matching the path portion instead of exact URL — for example
change the matcher passed to page.route (currently
`${PENGINE_API_BASE}/v1/toolengine/runtime`) to a pattern/RegExp or a predicate
that checks route.request().url startsWith or its pathname equals
`/v1/toolengine/runtime`, then keep the existing route.fulfill handler (the mock
response) inside that same async callback so requests with any query params
still get fulfilled.
In `@src/modules/bot/components/SetupWizardSteps.tsx`:
- Around line 503-510: The "Open dashboard" button is rendered even when
onCompleteSetup is undefined, producing a no-op; update the render condition in
SetupWizardSteps so the button only appears when connectStatus === "connected"
AND onCompleteSetup is defined (e.g., change the JSX conditional to
{connectStatus === "connected" && onCompleteSetup && ( ... )}), so the button is
not shown when clicking would do nothing.
🪄 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: fd2d0e93-0e5b-4255-885c-f5288435bc5e
📒 Files selected for processing (15)
e2e/setup-dashboard.spec.tssrc-tauri/src/app.rssrc-tauri/src/infrastructure/http_server.rssrc-tauri/src/modules/mcp/registry.rssrc-tauri/src/modules/mcp/service.rssrc-tauri/src/modules/mcp/transport.rssrc-tauri/src/modules/tool_engine/container/file-manager/Dockerfilesrc-tauri/src/modules/tool_engine/container/file-manager/buildsrc-tauri/src/modules/tool_engine/service.rssrc/modules/bot/components/SetupWizard.tsxsrc/modules/bot/components/SetupWizardSteps.tsxsrc/modules/mcp/components/McpServerCard.tsxsrc/modules/toolengine/components/ToolEnginePanel.tsxsrc/shared/ui/WizardLayout.tsxsrc/shared/workspaceMounts.ts
✅ Files skipped from review due to trivial changes (1)
- src/shared/ui/WizardLayout.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- src-tauri/src/modules/tool_engine/container/file-manager/Dockerfile
- src-tauri/src/modules/tool_engine/container/file-manager/build
- src/modules/toolengine/components/ToolEnginePanel.tsx
- src/modules/bot/components/SetupWizard.tsx
…nd UI improvements - Updated Tool Engine panel to display installation and removal progress for container images. - Added new CSS animations for visual feedback during tool operations. - Improved Setup Wizard to ensure readiness checks for container runtimes. - Refactored component logic to manage busy states and enhance user experience during tool actions.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/modules/bot/components/SetupWizardSteps.tsx (1)
497-499:⚠️ Potential issue | 🟡 MinorAlign “Ollama ready” summary with reachability, not model alone.
This checklist can show a false positive (
✓) when a model name exists but Ollama is currently unreachable. Gate it with reachability too for consistent status semantics.Suggested fix
export function WizardStepConnect(props: { botId: string | null; status: TokenStatus; ollamaModel: string | null; + ollamaReachable: boolean | null; runtimeStatus: RuntimeStatus | null; pengineReachable: boolean | null; @@ const { @@ ollamaModel, + ollamaReachable, @@ - <p>{ollamaModel ? "✓" : "○"} Ollama ready</p> + <p>{ollamaReachable === true && ollamaModel ? "✓" : "○"} Ollama ready</p>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/bot/components/SetupWizardSteps.tsx` around lines 497 - 499, The "Ollama ready" check currently only checks ollamaModel and can show ✓ even if Ollama is unreachable; change the condition to require both a model and reachability (e.g., use ollamaModel && ollamaReachable or ollamaModel && ollamaStatus?.reachable) so the UI only marks "Ollama ready" when a model exists and Ollama is reachable; update the JSX expression where "Ollama ready" is rendered to use that combined condition.
🧹 Nitpick comments (1)
src/modules/bot/components/SetupWizardSteps.tsx (1)
457-460: Use a single source of truth for the editable username field.
value={botUsername || verifiedBot.bot_username}prevents users from truly clearing the field and can cause confusing input behavior. Keep the input controlled strictly bybotUsernameand use placeholder/default initialization for fallback display.Suggested refactor
- value={botUsername || verifiedBot.bot_username} + value={botUsername} onChange={(event) => onBotUsernameChange(event.target.value)} - placeholder="@YourPengineBot" + placeholder={`@${verifiedBot.bot_username}`}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/bot/components/SetupWizardSteps.tsx` around lines 457 - 460, The input is using a fallback in its value prop (value={botUsername || verifiedBot.bot_username}) which prevents clearing and causes confusing behavior; change the input to be controlled only by botUsername (value={botUsername}) and initialize or sync botUsername from verifiedBot.bot_username (e.g., in the component's state via useState or useEffect) so the displayed default comes from state, and use the input's placeholder for the fallback display; keep using onBotUsernameChange to update botUsername onChange.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/modules/bot/components/SetupWizardSteps.tsx`:
- Around line 497-499: The "Ollama ready" check currently only checks
ollamaModel and can show ✓ even if Ollama is unreachable; change the condition
to require both a model and reachability (e.g., use ollamaModel &&
ollamaReachable or ollamaModel && ollamaStatus?.reachable) so the UI only marks
"Ollama ready" when a model exists and Ollama is reachable; update the JSX
expression where "Ollama ready" is rendered to use that combined condition.
---
Nitpick comments:
In `@src/modules/bot/components/SetupWizardSteps.tsx`:
- Around line 457-460: The input is using a fallback in its value prop
(value={botUsername || verifiedBot.bot_username}) which prevents clearing and
causes confusing behavior; change the input to be controlled only by botUsername
(value={botUsername}) and initialize or sync botUsername from
verifiedBot.bot_username (e.g., in the component's state via useState or
useEffect) so the displayed default comes from state, and use the input's
placeholder for the fallback display; keep using onBotUsernameChange to update
botUsername onChange.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bbe5cf0e-52e3-4494-8b82-d1278e515536
📒 Files selected for processing (4)
e2e/setup-dashboard.spec.tssrc/index.csssrc/modules/bot/components/SetupWizardSteps.tsxsrc/modules/toolengine/components/ToolEnginePanel.tsx
✅ Files skipped from review due to trivial changes (2)
- src/index.css
- src/modules/toolengine/components/ToolEnginePanel.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/setup-dashboard.spec.ts
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.
…ction
Summary by CodeRabbit
New Features
Improvements