feat: add directory browse button to Create Project dialog#476
Open
TheDarkSkyXD wants to merge 1 commit intospacedriveapp:mainfrom
Open
feat: add directory browse button to Create Project dialog#476TheDarkSkyXD wants to merge 1 commit intospacedriveapp:mainfrom
TheDarkSkyXD wants to merge 1 commit intospacedriveapp:mainfrom
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis pull request adds filesystem directory browsing capabilities across the application stack. It introduces a backend REST endpoint for listing directories with sorting and filtering logic, extends the TypeScript API client with directory listing interfaces and methods, and implements a React UI component with both native Tauri dialog integration and a web-mode fallback browser. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.41.1)src/api/server.rsThanks 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 |
Adds a "Browse" button next to the Root Path input so users can select a project directory without manually copying/pasting the path. - Tauri desktop: opens native OS folder picker via tauri-plugin-dialog - Web/Docker: inline directory browser backed by new GET /api/fs/list-dir endpoint Works on Windows, macOS, Linux, and Docker. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
TheDarkSkyXD
force-pushed
the
feat/project-directory-browser
branch
from
6a17eda to
5aa3def
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
interface/src/components/ModelSelect.tsx (2)
198-214: Minor UX inconsistency: ArrowUp doesn't open the dropdown.
ArrowDownopens the dropdown when closed (lines 200-203), butArrowUpdoes not. This means pressing ArrowUp when closed silently updateshighlightIndexwithout any visible effect. Consider adding similar open-on-keypress behavior for ArrowUp for consistency.♻️ Suggested fix
if (e.key === "ArrowUp") { e.preventDefault(); + if (!open) { + setOpen(true); + setFilter(value); + } setHighlightIndex((prev) => prev > 0 ? prev - 1 : flatList.length - 1, ); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/components/ModelSelect.tsx` around lines 198 - 214, The ArrowUp handler should mirror ArrowDown's behavior when the menu is closed: if open is false, call setOpen(true) and setFilter(value) before updating highlight via setHighlightIndex so pressing ArrowUp opens the dropdown and shows the highlighted item; update the ArrowUp branch (the block using setHighlightIndex, open, flatList, value, setOpen, setFilter) to include that open-on-keypress logic.
313-316: O(n²) complexity fromflatList.indexOf(model)in render loop.
flatList.indexOf(model)is called for each model during render. SinceindexOfis O(n) and this runs for all n models, the overall complexity is O(n²). With many models from multiple providers, this could cause noticeable UI lag.Consider pre-computing a lookup map for O(1) index access.
♻️ Suggested fix: Add index map memo
const flatList = useMemo(() => { const items: ModelInfo[] = []; for (const p of sortedProviders) { for (const m of grouped[p]) { items.push(m); } } return items; }, [sortedProviders, grouped]); + // Pre-compute index lookup for O(1) access during render + const flatIndexMap = useMemo(() => { + const map = new Map<string, number>(); + flatList.forEach((m, idx) => map.set(m.id, idx)); + return map; + }, [flatList]);Then in the render loop:
- const flatIdx = flatList.indexOf(model); + const flatIdx = flatIndexMap.get(model.id) ?? -1;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/components/ModelSelect.tsx` around lines 313 - 316, The render loop in ModelSelect uses flatList.indexOf(model) for each model causing O(n²) work; create a memoized map from model id (or model reference) to index (e.g., indexMap = useMemo(() => { const m=new Map(); flatList.forEach((mObj,i)=>m.set(mObj.id ?? mObj, i)); return m; }, [flatList])) and replace flatList.indexOf(model) in the grouped[prov].map callback with a constant-time lookup (e.g., const flatIdx = indexMap.get(model.id) or indexMap.get(model)); keep the existing highlightIndex and isSelected checks unchanged.src/agent/compactor.rs (1)
180-191: Consider extracting the ToolResult boundary advancement into a shared helper.The same ~12-line pattern appears in
emergency_truncate,run_compaction, andWorker::compact_history(seesrc/agent/worker.rs:758-769). A helper function would reduce duplication:/// Advances the removal boundary past User messages containing ToolResult blocks. fn advance_past_tool_results(history: &[Message], mut boundary: usize, max: usize) -> usize { while boundary < max { let has_tool_result = matches!( history.get(boundary), Some(Message::User { content }) if content.iter().any(|item| matches!(item, UserContent::ToolResult(_))) ); if has_tool_result { boundary += 1; } else { break; } } boundary }This is optional since the inline code is well-commented and self-contained at each site.
Also applies to: 235-246
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/compactor.rs` around lines 180 - 191, Extract the repeated loop that advances a removal boundary past consecutive User messages containing ToolResult blocks into a shared helper like advance_past_tool_results(history: &[Message], boundary: usize, max: usize) -> usize, then replace the inlined loops in compactor.rs (the loop inside the function containing remove_count) and the similar blocks in emergency_truncate, run_compaction, and Worker::compact_history with calls to this helper; ensure the helper uses the same matching logic (matches!(history.get(boundary), Some(Message::User { content }) if content.iter().any(|item| matches!(item, UserContent::ToolResult(_)))) and returns the updated boundary.interface/src/routes/AgentProjects.tsx (1)
219-231: Consider logging errors in native dialog fallback.The catch block silently returns
null, which is fine for user-facing behavior, but logging the error would help with debugging when the native dialog fails unexpectedly.🔧 Proposed improvement
async function openNativeFolderDialog(): Promise<string | null> { try { const { open } = await import("@tauri-apps/plugin-dialog"); const selected = await open({ directory: true, multiple: false, title: "Select Project Directory", }); return typeof selected === "string" ? selected : null; - } catch { + } catch (e) { + console.warn("Native folder dialog unavailable:", e); return null; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/routes/AgentProjects.tsx` around lines 219 - 231, The catch in openNativeFolderDialog currently swallows errors; update it to capture the thrown error (e.g., catch (err) or catch (error)) and log the error before returning null so failures of the native dialog are observable during debugging—use an appropriate logger (console.error or the app's logging utility) and include a clear message referencing openNativeFolderDialog and the error details.src/api/fs.rs (1)
71-80: Silent iteration errors may hide filesystem issues.The
while let Ok(Some(entry))pattern stops iteration silently on the first error. Per coding guidelines ("Don't silently discard errors"), consider logging iteration errors.🛠️ Suggested improvement
- while let Ok(Some(entry)) = read_dir.next_entry().await { + loop { + let entry = match read_dir.next_entry().await { + Ok(Some(e)) => e, + Ok(None) => break, + Err(e) => { + tracing::warn!("Error reading directory entry: {e}"); + continue; + } + }; let name = entry.file_name().to_string_lossy().to_string();Based on learnings: coding guidelines state "Don't silently discard errors. No
let _ =on Results. Handle them, log them, or propagate them."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/fs.rs` around lines 71 - 80, The loop using read_dir.next_entry().await with pattern while let Ok(Some(entry)) silently stops on errors; update the iteration in the function where you build entries (using read_dir.next_entry().await and pushing DirEntry) to explicitly handle Err cases: match the Result from next_entry().await, on Ok(Some(entry)) proceed as before, on Ok(None) break, and on Err(e) log the error (including context/path) via the module/logger used in this file and then either continue to the next entry or return the error depending on the function's error-handling strategy; ensure you reference read_dir.next_entry().await and DirEntry so the change is applied in the correct loop.
🤖 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/api/fs.rs`:
- Around line 33-91: The list_dir handler allows listing arbitrary filesystem
paths after canonicalize() which creates an unrestricted traversal risk; update
list_dir to enforce a boundary check (or allowlist) after computing dir:
canonicalize the allowed root (e.g., home root from dirs::home_dir() or a
configured WORKSPACE_ROOT), canonicalize the requested dir, then verify
requested_dir.starts_with(allowed_root) (or check membership against an explicit
allowlist) and return BadRequest/Forbidden if it fails; reference the function
list_dir, the variable dir, and the canonicalize/parent logic when adding this
check (or alternatively implement and document an explicit opt-in mode with
rate-limiting/audit if you choose to allow global access).
In `@src/tools/browser.rs`:
- Around line 2057-2067: The current closure of the browser treats every
browser.close().await error as benign; change it to only swallow known
benign/disconnected cases (e.g., "oneshot canceled", websocket disconnected) by
matching on the error string or its concrete error variants and logging them
with tracing::warn, and for all other errors return a structured error (e.g.,
Err(BrowserError::ShutdownFailed(error)) or the existing tool Result error type)
instead of continuing to report success; locate the browser.close().await call
and the surrounding block that currently logs then ignores the error and
implement the selective match/return there.
---
Nitpick comments:
In `@interface/src/components/ModelSelect.tsx`:
- Around line 198-214: The ArrowUp handler should mirror ArrowDown's behavior
when the menu is closed: if open is false, call setOpen(true) and
setFilter(value) before updating highlight via setHighlightIndex so pressing
ArrowUp opens the dropdown and shows the highlighted item; update the ArrowUp
branch (the block using setHighlightIndex, open, flatList, value, setOpen,
setFilter) to include that open-on-keypress logic.
- Around line 313-316: The render loop in ModelSelect uses
flatList.indexOf(model) for each model causing O(n²) work; create a memoized map
from model id (or model reference) to index (e.g., indexMap = useMemo(() => {
const m=new Map(); flatList.forEach((mObj,i)=>m.set(mObj.id ?? mObj, i)); return
m; }, [flatList])) and replace flatList.indexOf(model) in the grouped[prov].map
callback with a constant-time lookup (e.g., const flatIdx =
indexMap.get(model.id) or indexMap.get(model)); keep the existing highlightIndex
and isSelected checks unchanged.
In `@interface/src/routes/AgentProjects.tsx`:
- Around line 219-231: The catch in openNativeFolderDialog currently swallows
errors; update it to capture the thrown error (e.g., catch (err) or catch
(error)) and log the error before returning null so failures of the native
dialog are observable during debugging—use an appropriate logger (console.error
or the app's logging utility) and include a clear message referencing
openNativeFolderDialog and the error details.
In `@src/agent/compactor.rs`:
- Around line 180-191: Extract the repeated loop that advances a removal
boundary past consecutive User messages containing ToolResult blocks into a
shared helper like advance_past_tool_results(history: &[Message], boundary:
usize, max: usize) -> usize, then replace the inlined loops in compactor.rs (the
loop inside the function containing remove_count) and the similar blocks in
emergency_truncate, run_compaction, and Worker::compact_history with calls to
this helper; ensure the helper uses the same matching logic
(matches!(history.get(boundary), Some(Message::User { content }) if
content.iter().any(|item| matches!(item, UserContent::ToolResult(_)))) and
returns the updated boundary.
In `@src/api/fs.rs`:
- Around line 71-80: The loop using read_dir.next_entry().await with pattern
while let Ok(Some(entry)) silently stops on errors; update the iteration in the
function where you build entries (using read_dir.next_entry().await and pushing
DirEntry) to explicitly handle Err cases: match the Result from
next_entry().await, on Ok(Some(entry)) proceed as before, on Ok(None) break, and
on Err(e) log the error (including context/path) via the module/logger used in
this file and then either continue to the next entry or return the error
depending on the function's error-handling strategy; ensure you reference
read_dir.next_entry().await and DirEntry so the change is applied in the correct
loop.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9f600a97-ead2-408f-a9cd-bd8ea8dcb2c5
⛔ Files ignored due to path filters (53)
desktop/src-tauri/Cargo.tomlis excluded by!**/*.tomldesktop/src-tauri/capabilities/default.jsonis excluded by!**/*.jsondesktop/src-tauri/gen/schemas/windows-schema.jsonis excluded by!**/gen/**,!**/*.json,!**/gen/**desktop/src-tauri/icons/64x64.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/Square107x107Logo.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/Square142x142Logo.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/Square150x150Logo.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/Square284x284Logo.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/Square30x30Logo.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/Square310x310Logo.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/Square44x44Logo.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/Square71x71Logo.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/Square89x89Logo.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/StoreLogo.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-anydpi-v26/ic_launcher.xmlis excluded by!**/*.xmldesktop/src-tauri/icons/android/mipmap-hdpi/ic_launcher.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-hdpi/ic_launcher_foreground.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-hdpi/ic_launcher_round.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-mdpi/ic_launcher.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-mdpi/ic_launcher_foreground.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-mdpi/ic_launcher_round.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-xhdpi/ic_launcher.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-xhdpi/ic_launcher_foreground.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-xhdpi/ic_launcher_round.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-xxhdpi/ic_launcher.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-xxhdpi/ic_launcher_foreground.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-xxhdpi/ic_launcher_round.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-xxxhdpi/ic_launcher.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-xxxhdpi/ic_launcher_foreground.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-xxxhdpi/ic_launcher_round.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/values/ic_launcher_background.xmlis excluded by!**/*.xmldesktop/src-tauri/icons/icon.icois excluded by!**/*.ico,!**/*.icodesktop/src-tauri/icons/icon.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-20x20@1x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-20x20@2x-1.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-20x20@2x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-20x20@3x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-29x29@1x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-29x29@2x-1.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-29x29@2x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-29x29@3x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-40x40@1x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-40x40@2x-1.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-40x40@2x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-40x40@3x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-512@2x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-60x60@2x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-60x60@3x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-76x76@1x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-76x76@2x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-83.5x83.5@2x.pngis excluded by!**/*.png,!**/*.pngdesktop/tauri.windows.conf.jsonis excluded by!**/*.jsoninterface/package.jsonis excluded by!**/*.json
📒 Files selected for processing (13)
desktop/src-tauri/src/main.rsinterface/src/api/client.tsinterface/src/components/ModelSelect.tsxinterface/src/routes/AgentProjects.tsxscripts/bundle-sidecar.ps1src/agent/compactor.rssrc/agent/worker.rssrc/api.rssrc/api/fs.rssrc/api/models.rssrc/api/server.rssrc/config/types.rssrc/tools/browser.rs
Comment on lines
+33
to
+91
| /// List the contents of a directory. Defaults to the user's home directory. | ||
| pub(super) async fn list_dir( | ||
| Query(query): Query<ListDirQuery>, | ||
| ) -> Result<Json<ListDirResponse>, impl IntoResponse> { | ||
| let dir = match &query.path { | ||
| Some(p) if !p.is_empty() => PathBuf::from(p), | ||
| _ => dirs::home_dir().unwrap_or_else(|| PathBuf::from("/")), | ||
| }; | ||
|
|
||
| let dir = match dir.canonicalize() { | ||
| Ok(d) => d, | ||
| Err(e) => { | ||
| return Err(( | ||
| StatusCode::BAD_REQUEST, | ||
| Json(serde_json::json!({ "error": format!("Invalid path: {e}") })), | ||
| )); | ||
| } | ||
| }; | ||
|
|
||
| if !dir.is_dir() { | ||
| return Err(( | ||
| StatusCode::BAD_REQUEST, | ||
| Json(serde_json::json!({ "error": "Path is not a directory" })), | ||
| )); | ||
| } | ||
|
|
||
| let mut entries = Vec::new(); | ||
|
|
||
| let mut read_dir = match tokio::fs::read_dir(&dir).await { | ||
| Ok(rd) => rd, | ||
| Err(e) => { | ||
| return Err(( | ||
| StatusCode::INTERNAL_SERVER_ERROR, | ||
| Json(serde_json::json!({ "error": format!("Cannot read directory: {e}") })), | ||
| )); | ||
| } | ||
| }; | ||
|
|
||
| while let Ok(Some(entry)) = read_dir.next_entry().await { | ||
| let name = entry.file_name().to_string_lossy().to_string(); | ||
| // Skip hidden files/directories (starting with .) | ||
| if name.starts_with('.') { | ||
| continue; | ||
| } | ||
| let is_dir = entry.file_type().await.map(|ft| ft.is_dir()).unwrap_or(false); | ||
| let path = entry.path().to_string_lossy().to_string(); | ||
| entries.push(DirEntry { name, path, is_dir }); | ||
| } | ||
|
|
||
| // Sort: directories first, then alphabetically | ||
| entries.sort_by(|a, b| b.is_dir.cmp(&a.is_dir).then(a.name.to_lowercase().cmp(&b.name.to_lowercase()))); | ||
|
|
||
| let parent = dir.parent().map(|p| p.to_string_lossy().to_string()); | ||
|
|
||
| Ok(Json(ListDirResponse { | ||
| path: dir.to_string_lossy().to_string(), | ||
| parent, | ||
| entries, | ||
| })) |
Contributor
There was a problem hiding this comment.
Critical: Unrestricted filesystem traversal vulnerability.
This endpoint allows any authenticated API user to list any directory on the server. Unlike src/tools/send_file.rs which enforces workspace boundary checks using .starts_with(), this endpoint has no access restrictions after canonicalize().
An attacker with API access (or an agent with malicious instructions) could enumerate sensitive directories like /etc, /root, ~/.ssh, or any other location on the server.
🔒 Recommended fix: Add boundary checks or explicit allowlist
Option 1 — Restrict to user's home directory tree:
+ // Security: restrict listing to user's home directory tree
+ let home_dir = dirs::home_dir().unwrap_or_else(|| PathBuf::from("/"));
+ let home_canonical = home_dir.canonicalize().unwrap_or(home_dir);
+
let dir = match dir.canonicalize() {
Ok(d) => d,
Err(e) => {
return Err((
StatusCode::BAD_REQUEST,
Json(serde_json::json!({ "error": format!("Invalid path: {e}") })),
));
}
};
+ if !dir.starts_with(&home_canonical) {
+ return Err((
+ StatusCode::FORBIDDEN,
+ Json(serde_json::json!({ "error": "Access denied: path outside home directory" })),
+ ));
+ }
+
if !dir.is_dir() {Option 2 — Accept this as intentional for Docker/desktop use and document it clearly, adding rate limiting or audit logging.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/fs.rs` around lines 33 - 91, The list_dir handler allows listing
arbitrary filesystem paths after canonicalize() which creates an unrestricted
traversal risk; update list_dir to enforce a boundary check (or allowlist) after
computing dir: canonicalize the allowed root (e.g., home root from
dirs::home_dir() or a configured WORKSPACE_ROOT), canonicalize the requested
dir, then verify requested_dir.starts_with(allowed_root) (or check membership
against an explicit allowlist) and return BadRequest/Forbidden if it fails;
reference the function list_dir, the variable dir, and the canonicalize/parent
logic when adding this check (or alternatively implement and document an
explicit opt-in mode with rate-limiting/audit if you choose to allow global
access).
src/tools/browser.rs
Outdated
Comment on lines
+2057
to
+2067
| if let Some(mut browser) = browser { | ||
| if let Err(error) = browser.close().await { | ||
| // The browser connection may already be dead (e.g. "oneshot canceled" | ||
| // from a dropped WebSocket). That's fine — the browser is effectively | ||
| // closed. Log it but don't fail the tool call. | ||
| tracing::warn!( | ||
| policy = "close_browser", | ||
| %error, | ||
| "browser.close() failed, treating as already closed" | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
Don’t always return success when browser shutdown fails
This now treats all browser.close() failures as benign, which can mask real shutdown failures and leave a running browser process while reporting "Browser closed". Please only ignore explicitly benign/disconnected cases, and return a structured BrowserError for other failures.
Proposed fix
- if let Some(mut browser) = browser {
- if let Err(error) = browser.close().await {
- // The browser connection may already be dead (e.g. "oneshot canceled"
- // from a dropped WebSocket). That's fine — the browser is effectively
- // closed. Log it but don't fail the tool call.
- tracing::warn!(
- policy = "close_browser",
- %error,
- "browser.close() failed, treating as already closed"
- );
- }
- }
+ if let Some(mut browser) = browser {
+ if let Err(error) = browser.close().await {
+ let error_text = error.to_string();
+ let is_benign_disconnect =
+ error_text.contains("oneshot canceled")
+ || error_text.contains("channel closed")
+ || error_text.contains("connection closed");
+
+ if is_benign_disconnect {
+ tracing::warn!(
+ policy = "close_browser",
+ %error,
+ "browser.close() failed due to disconnected session; treating as already closed"
+ );
+ } else {
+ return Err(BrowserError::new(format!(
+ "failed to close browser: {error}"
+ )));
+ }
+ }
+ }Based on learnings: Applies to src/tools/**/*.rs — “Return tool errors as structured results, not panics. The LLM sees the error and can recover.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if let Some(mut browser) = browser { | |
| if let Err(error) = browser.close().await { | |
| // The browser connection may already be dead (e.g. "oneshot canceled" | |
| // from a dropped WebSocket). That's fine — the browser is effectively | |
| // closed. Log it but don't fail the tool call. | |
| tracing::warn!( | |
| policy = "close_browser", | |
| %error, | |
| "browser.close() failed, treating as already closed" | |
| ); | |
| } | |
| if let Some(mut browser) = browser { | |
| if let Err(error) = browser.close().await { | |
| let error_text = error.to_string(); | |
| let is_benign_disconnect = | |
| error_text.contains("oneshot canceled") | |
| || error_text.contains("channel closed") | |
| || error_text.contains("connection closed"); | |
| if is_benign_disconnect { | |
| tracing::warn!( | |
| policy = "close_browser", | |
| %error, | |
| "browser.close() failed due to disconnected session; treating as already closed" | |
| ); | |
| } else { | |
| return Err(BrowserError::new(format!( | |
| "failed to close browser: {error}" | |
| ))); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tools/browser.rs` around lines 2057 - 2067, The current closure of the
browser treats every browser.close().await error as benign; change it to only
swallow known benign/disconnected cases (e.g., "oneshot canceled", websocket
disconnected) by matching on the error string or its concrete error variants and
logging them with tracing::warn, and for all other errors return a structured
error (e.g., Err(BrowserError::ShutdownFailed(error)) or the existing tool
Result error type) instead of continuing to report success; locate the
browser.close().await call and the surrounding block that currently logs then
ignores the error and implement the selective match/return there.
jamiepine
approved these changes
Mar 24, 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
2 participants
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
tauri-plugin-dialogGET /api/fs/list-dirbackend endpointChanges
desktop/src-tauri/Cargo.toml— addedtauri-plugin-dialogdependencydesktop/src-tauri/src/main.rs— registered dialog plugindesktop/src-tauri/capabilities/default.json— addeddialog:defaultpermissioninterface/package.json— added@tauri-apps/plugin-dialogoptional dependencyinterface/src/api/client.ts— addedDirEntry,ListDirResponsetypes andapi.listDir()interface/src/routes/AgentProjects.tsx— added Browse button,DirectoryBrowsercomponent, and native dialog integrationsrc/api/fs.rs— new filesystem listing endpoint (directories first, skips hidden files, defaults to home dir)src/api.rs+src/api/server.rs— wired the newfsmodule and/fs/list-dirrouteTest plan
🤖 Generated with Claude Code