feat: support single-repo projects and enhance skills UI - #362
Merged
Conversation
Single-repo projects: discover_repos() now detects when the project root itself is a git repo (has .git directory) and returns it with relative_path=".". Worktrees for single-repo projects are placed in the parent directory as siblings, matching standard git worktree conventions. All consumers (API handlers, LLM tool, UI) updated to handle the "." repo path and "../" worktree paths correctly. Skills UI: add detail modals for viewing full SKILL.md content of both installed and registry skills, file upload for installing skill archives, and corresponding API endpoints (get_skill_content, upload_skill, registry_skill_content) with GitHub raw content fetching and caching.
Contributor
WalkthroughAdds backend endpoints and data structures to serve and upload skill content, a registry content cache and fetcher, UI modals and upload flow to view/install skill SKILL.md content, and single-repo project support adjusting discovery and worktree path semantics plus small AgentProjects UI tweaks. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
🚥 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)
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 |
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
|
|
||
| let mut all_installed = Vec::new(); | ||
|
|
||
| while let Ok(Some(field)) = multipart.next_field().await { |
Contributor
There was a problem hiding this comment.
while let Ok(Some(..)) = multipart.next_field().await will silently stop on the first parse error and still return 200. Probably better to propagate the error as a 400.
Suggested change
| while let Ok(Some(field)) = multipart.next_field().await { | |
| while let Some(field) = multipart.next_field().await.map_err(|error| { | |
| tracing::warn!(%error, "failed to read upload multipart field"); | |
| StatusCode::BAD_REQUEST | |
| })? { |
| let mut all_installed = Vec::new(); | ||
|
|
||
| while let Ok(Some(field)) = multipart.next_field().await { | ||
| let filename = field |
Contributor
There was a problem hiding this comment.
field.file_name() can include path separators / absolute paths; temp_dir.path().join(&filename) can write outside the temp dir. Worth normalizing to a basename (or ignoring the provided name entirely).
Suggested change
| let filename = field | |
| let filename = field | |
| .file_name() | |
| .and_then(|n| std::path::Path::new(n).file_name()) | |
| .and_then(|n| n.to_str()) | |
| .unwrap_or("upload.zip") | |
| .to_string(); |
| pub async fn discover_repos(project_root: &Path) -> anyhow::Result<Vec<DiscoveredRepo>> { | ||
| // Check if the project root itself is a git repo (single-repo project). | ||
| let root_dot_git = project_root.join(".git"); | ||
| if root_dot_git.exists() && root_dot_git.is_dir() { |
Contributor
There was a problem hiding this comment.
This only treats a repo as "single-repo" when .git is a directory. If the project root is a git worktree, .git is usually a file, so it won’t be detected.
Suggested change
| if root_dot_git.exists() && root_dot_git.is_dir() { | |
| if root_dot_git.exists() && (root_dot_git.is_dir() || root_dot_git.is_file()) { |
| }, | ||
| }); | ||
|
|
||
| const uploadMutation = useMutation({ |
Contributor
There was a problem hiding this comment.
Minor UX thing: clearing the file input only on success makes it hard to retry uploading the same file after a failure (the input value doesn’t change, so onChange may not fire). Clearing on onSettled keeps retries smooth.
Suggested change
| const uploadMutation = useMutation({ | |
| const uploadMutation = useMutation({ | |
| mutationFn: (files: File[]) => api.uploadSkillFiles(agentId, files), | |
| onSuccess: () => { | |
| queryClient.invalidateQueries({ queryKey: ["skills", agentId] }); | |
| }, | |
| onSettled: () => { | |
| if (fileInputRef.current) { | |
| fileInputRef.current.value = ""; | |
| } | |
| }, | |
| }); |
![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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/api/projects.rs (1)
194-238:⚠️ Potential issue | 🟠 MajorDetect single-repo layout per repo, not by repo count.
This should key off
repo.path == "."inside the loop. Once a single-repo project has any additional repo row,repos.len() == 1becomes false even though the root repo still keeps its worktrees as siblings. At that point scans start storingfeature-xinstead of../feature-x, which can duplicate worktrees and make later deletes target the wrong path.Suggested fix
- let is_single_repo = repos.len() == 1 && repos[0].path == "."; - for repo in &repos { + let is_single_repo = repo.path == "."; let repo_abs_path = root.join(&repo.path); if !repo_abs_path.is_dir() { continue; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/projects.rs` around lines 194 - 238, The code currently computes is_single_repo once based on repos.len() == 1 which is wrong; change the logic to detect single-repo layout per repo inside the loop by checking repo.path == "." (e.g., compute let is_single_repo = repo.path == "." at the top of the for repo in &repos loop) and then use that per-repo flag when computing (name, relative_path) so worktrees for the root repo continue to be stored with the "../..." sibling path; update any references to the earlier top-level is_single_repo variable accordingly (in the block that builds name and relative_path).src/tools/project_manage.rs (1)
395-435:⚠️ Potential issue | 🟠 MajorUse the repo’s stored path to decide sibling worktree handling.
This scan logic has the same mixed-layout bug as the API path: once a single-repo project has any additional repo,
all_repos.len() == 1becomes false and the root repo’s sibling worktrees get stored as if they lived under the project root. That makes later tool-driven remove/path reporting inconsistent with create_worktree.Suggested fix
- let is_single_repo = all_repos.len() == 1 && all_repos[0].path == "."; - for repo in &all_repos { + let is_single_repo = repo.path == "."; let repo_abs_path = root.join(&repo.path); if let Ok(worktrees) = git::list_worktrees(&repo_abs_path).await {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/project_manage.rs` around lines 395 - 435, The relative_path computation incorrectly uses the project root and the is_single_repo flag to decide sibling worktree handling; instead use the specific repo's stored path (repo.path / repo_abs_path) to determine whether a worktree is a sibling and to compute the "../" relative path. Update the branch in the loop that builds relative_path to base the parent/strip_prefix logic on repo_abs_path (or repo.path) and its parent rather than the global root so sibling worktrees for the repo are detected and stored consistently; adjust the is_single_repo check usage or replace it with a per-repo check using repo.path to decide whether to produce "../<path>" or a path relative to the project root. Ensure references affected include is_single_repo, repo_abs_path, worktree_info, and relative_path.
🧹 Nitpick comments (3)
src/api/skills.rs (2)
666-718: Code duplication withfetch_registry_skill_description.The
fetch_registry_skill_contentfunction shares significant logic withfetch_registry_skill_description(lines 611-664): same candidate path generation, same branch iteration, same GitHub URL construction, and same HTTP request pattern. The only difference is that one extracts a description and the other returns full content.♻️ Consider extracting shared logic into a helper
/// Fetch raw SKILL.md content from GitHub, returning the full text if found. async fn fetch_skill_markdown( client: &reqwest::Client, source: &str, skill_id: &str, ) -> Option<String> { let repo_name = source.split('/').next_back().unwrap_or_default(); let candidate_paths = if repo_name == skill_id { vec![ "SKILL.md".to_string(), format!("{skill_id}/SKILL.md"), format!("skills/{skill_id}/SKILL.md"), format!(".claude/skills/{skill_id}/SKILL.md"), ] } else { vec![ format!("{skill_id}/SKILL.md"), format!("skills/{skill_id}/SKILL.md"), format!(".claude/skills/{skill_id}/SKILL.md"), "SKILL.md".to_string(), ] }; for path in candidate_paths { for branch in ["HEAD", "main", "master"] { let url = format!("https://raw.githubusercontent.com/{source}/{branch}/{path}"); let response = match client .get(&url) .header(reqwest::header::USER_AGENT, "spacebot-registry-client") .timeout(Duration::from_secs(5)) .send() .await { Ok(response) => response, Err(_) => continue, }; if !response.status().is_success() { continue; } match response.text().await { Ok(markdown) if !markdown.trim().is_empty() => return Some(markdown), _ => continue, } } } None } // Then use it in both places: async fn fetch_registry_skill_description(...) -> Option<String> { fetch_skill_markdown(client, source, skill_id) .await .and_then(|md| extract_skill_description(&md)) } async fn fetch_registry_skill_content(...) -> Option<String> { fetch_skill_markdown(client, source, skill_id).await }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/skills.rs` around lines 666 - 718, The two functions fetch_registry_skill_content and fetch_registry_skill_description duplicate candidate path generation and GitHub fetching logic; extract that shared logic into a new async helper fetch_skill_markdown(client: &reqwest::Client, source: &str, skill_id: &str) -> Option<String> that builds candidate_paths (using the same repo_name check), iterates branches ["HEAD","main","master"], constructs the raw.githubusercontent URL, performs the reqwest get with the same USER_AGENT and timeout, and returns the first non-empty markdown string; then replace the bodies of fetch_registry_skill_content to simply await fetch_skill_markdown(...) and fetch_registry_skill_description to await fetch_skill_markdown(...) and then and_then(|md| extract_skill_description(&md)) so the fetching logic is centralized and reqwest usage is unchanged.
329-342: Consider validating file extension before processing.The upload handler accepts any file and attempts to install it. While
install_from_filewill fail on invalid archives, adding early validation could provide better error messages and avoid unnecessary temp file creation.💡 Optional: Add file extension validation
let filename = field .file_name() .map(|n| n.to_string()) .unwrap_or_else(|| "upload.zip".to_string()); let data = field.bytes().await.map_err(|error| { tracing::warn!(%error, "failed to read upload field"); StatusCode::BAD_REQUEST })?; if data.is_empty() { continue; } + // Validate file extension + let is_valid_extension = filename.ends_with(".zip") || filename.ends_with(".skill"); + if !is_valid_extension { + tracing::warn!(filename = %filename, "invalid file extension for skill upload"); + return Err(StatusCode::BAD_REQUEST); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/skills.rs` around lines 329 - 342, Validate the uploaded filename's extension before reading/processing the bytes and calling install_from_file: after obtaining filename (the filename variable) check (case-insensitively) that it ends_with ".zip" (or another allowed extension) and if not return an early StatusCode::BAD_REQUEST with a clear message, skipping temp file creation and install_from_file; keep the check adjacent to where filename and data are obtained (within the multipart.next_field() loop) so invalid uploads are rejected quickly and logged via tracing::warn!.interface/src/routes/AgentSkills.tsx (1)
808-843: Upload status messages should auto-dismiss.The success/error messages for uploads persist indefinitely until the next action. Consider clearing them after a timeout or on tab change to avoid stale feedback.
💡 Optional: Auto-dismiss upload status messages
You could reset the mutation state after a delay:
const uploadMutation = useMutation({ mutationFn: (files: File[]) => api.uploadSkillFiles(agentId, files), onSuccess: () => { queryClient.invalidateQueries({ queryKey: ["skills", agentId] }); if (fileInputRef.current) { fileInputRef.current.value = ""; } }, }); // Add effect to auto-dismiss useEffect(() => { if (uploadMutation.isSuccess || uploadMutation.isError) { const timer = setTimeout(() => { uploadMutation.reset(); }, 5000); return () => clearTimeout(timer); } }, [uploadMutation.isSuccess, uploadMutation.isError]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/routes/AgentSkills.tsx` around lines 808 - 843, The upload status messages never clear because the uploadMutation state isn't reset; add an effect that watches uploadMutation.isSuccess and uploadMutation.isError (and optionally visibility/tab changes) and calls uploadMutation.reset() after a short timeout (e.g., 4–5s) or immediately on tab change, ensuring you still keep the existing onSuccess behavior that invalidates queries and clears fileInputRef.value; reference the uploadMutation object and fileInputRef in the effect and clear the timeout in the cleanup to avoid leaks.
🤖 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/projects/git.rs`:
- Around line 39-61: The check that treats project_root.join(".git") only as a
directory should be relaxed to accept a .git file (worktree or submodule case);
update the conditional around root_dot_git (in src/projects/git.rs) so that it
triggers when root_dot_git.exists() (or exists() && (is_dir() || is_file())),
then keep the existing retrieval calls (get_remote_url, get_default_branch,
get_current_branch) and return the DiscoveredRepo as before; this ensures
single-repo detection works for both .git directories and .git files.
---
Outside diff comments:
In `@src/api/projects.rs`:
- Around line 194-238: The code currently computes is_single_repo once based on
repos.len() == 1 which is wrong; change the logic to detect single-repo layout
per repo inside the loop by checking repo.path == "." (e.g., compute let
is_single_repo = repo.path == "." at the top of the for repo in &repos loop) and
then use that per-repo flag when computing (name, relative_path) so worktrees
for the root repo continue to be stored with the "../..." sibling path; update
any references to the earlier top-level is_single_repo variable accordingly (in
the block that builds name and relative_path).
In `@src/tools/project_manage.rs`:
- Around line 395-435: The relative_path computation incorrectly uses the
project root and the is_single_repo flag to decide sibling worktree handling;
instead use the specific repo's stored path (repo.path / repo_abs_path) to
determine whether a worktree is a sibling and to compute the "../" relative
path. Update the branch in the loop that builds relative_path to base the
parent/strip_prefix logic on repo_abs_path (or repo.path) and its parent rather
than the global root so sibling worktrees for the repo are detected and stored
consistently; adjust the is_single_repo check usage or replace it with a
per-repo check using repo.path to decide whether to produce "../<path>" or a
path relative to the project root. Ensure references affected include
is_single_repo, repo_abs_path, worktree_info, and relative_path.
---
Nitpick comments:
In `@interface/src/routes/AgentSkills.tsx`:
- Around line 808-843: The upload status messages never clear because the
uploadMutation state isn't reset; add an effect that watches
uploadMutation.isSuccess and uploadMutation.isError (and optionally
visibility/tab changes) and calls uploadMutation.reset() after a short timeout
(e.g., 4–5s) or immediately on tab change, ensuring you still keep the existing
onSuccess behavior that invalidates queries and clears fileInputRef.value;
reference the uploadMutation object and fileInputRef in the effect and clear the
timeout in the cleanup to avoid leaks.
In `@src/api/skills.rs`:
- Around line 666-718: The two functions fetch_registry_skill_content and
fetch_registry_skill_description duplicate candidate path generation and GitHub
fetching logic; extract that shared logic into a new async helper
fetch_skill_markdown(client: &reqwest::Client, source: &str, skill_id: &str) ->
Option<String> that builds candidate_paths (using the same repo_name check),
iterates branches ["HEAD","main","master"], constructs the raw.githubusercontent
URL, performs the reqwest get with the same USER_AGENT and timeout, and returns
the first non-empty markdown string; then replace the bodies of
fetch_registry_skill_content to simply await fetch_skill_markdown(...) and
fetch_registry_skill_description to await fetch_skill_markdown(...) and then
and_then(|md| extract_skill_description(&md)) so the fetching logic is
centralized and reqwest usage is unchanged.
- Around line 329-342: Validate the uploaded filename's extension before
reading/processing the bytes and calling install_from_file: after obtaining
filename (the filename variable) check (case-insensitively) that it ends_with
".zip" (or another allowed extension) and if not return an early
StatusCode::BAD_REQUEST with a clear message, skipping temp file creation and
install_from_file; keep the check adjacent to where filename and data are
obtained (within the multipart.next_field() loop) so invalid uploads are
rejected quickly and logged via tracing::warn!.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: badc3fc3-6111-42b1-8785-3bf68374c2a0
📒 Files selected for processing (8)
interface/src/api/client.tsinterface/src/routes/AgentProjects.tsxinterface/src/routes/AgentSkills.tsxsrc/api/projects.rssrc/api/server.rssrc/api/skills.rssrc/projects/git.rssrc/tools/project_manage.rs
…guard, .git file detection, per-repo worktree path logic
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@interface/src/routes/AgentSkills.tsx`:
- Around line 247-250: The clickable card divs (the element with className "flex
cursor-pointer ..." in AgentSkills.tsx and the similar card at lines ~306-309)
are not keyboard-accessible; replace or augment these divs so they are focusable
and respond to keyboard activation by either changing them to a semantic
<button> or adding tabindex={0}, role="button", and an onKeyDown handler that
triggers the same onClick logic when Enter or Space is pressed. Update the JSX
for the card components and ensure the existing onClick handler (onClick) is
reused by the key handler or passed to the button to avoid duplicate behavior.
In `@src/api/projects.rs`:
- Around line 203-237: The current logic in the worktree handling block
(variables: is_root_repo, worktree.path, root, producing name and relative_path)
falls back to using the file_name when strip_prefix(root) fails, which registers
a fake in-project path for external worktrees; instead, either skip external
worktrees altogether or preserve a path that round-trips to the real location by
using the absolute worktree.path when strip_prefix(root) returns Err. Update the
branch that computes relative_path to: if worktree.path.strip_prefix(root)
succeeds use that relative string, otherwise set relative_path to
worktree.path.to_string_lossy().to_string() (or omit the worktree from the
results), and ensure name is still derived from file_name if desired; apply this
change where name and relative_path are assigned so delete/disk-usage resolve
the correct directory.
In `@src/api/skills.rs`:
- Around line 343-370: The handler currently calls field.bytes().await which
buffers the entire upload into memory; replace that with streaming writes to
disk: create the temp_dir and temp_path and open tokio::fs::File for writing
before reading the field, then iterate the multipart field's async chunk stream
(use the field's async chunk/stream API) and write each chunk to file with
file.write_all(...).await, tracking a running total and explicitly reject with
StatusCode::BAD_REQUEST (and a warning log) if total exceeds a configured
MAX_UPLOAD_BYTES; preserve the existing error mappings/logs for temp dir/file
creation, write and sync operations and keep the zero-length check by validating
total == 0 after streaming, then call the existing installer (install_from_file
or similar) with the temp_path.
In `@src/tools/project_manage.rs`:
- Around line 280-292: The current computation of relative_path in the worktree
handling (the block using is_root_repo, root, worktree_info, and worktree_name)
incorrectly falls back to storing worktree_name when strip_prefix(root) fails,
which misrepresents out-of-root worktrees; update the logic in both occurrences
(the relative_path assignment around worktree_info.path.strip_prefix(root) and
the similar block at 421-433) to either skip non-descendent worktrees with a
logged warning (using process or logger used in this module) or persist an
unambiguous absolute/prefixed path (e.g., keep full worktree_info.path or prefix
with "../") instead of just worktree_name so remove/disk-usage operations do not
target root/worktree_name by mistake. Ensure references to is_root_repo, root,
worktree_info.path, and worktree_name are used to detect out-of-root cases and
handle them consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 04c0ff99-be52-474a-92f4-789269dbc9a4
📒 Files selected for processing (5)
interface/src/routes/AgentSkills.tsxsrc/api/projects.rssrc/api/skills.rssrc/projects/git.rssrc/tools/project_manage.rs
Comment on lines
+247
to
+250
| <div | ||
| className="flex cursor-pointer flex-col rounded-lg border border-app-line bg-app-box p-4 transition-colors hover:border-app-line-hover" | ||
| onClick={onClick} | ||
| > |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd interface/src/routes && wc -l AgentSkills.tsxRepository: spacedriveapp/spacebot
Length of output: 85
🏁 Script executed:
cd interface/src/routes && sed -n '240,315p' AgentSkills.tsxRepository: spacedriveapp/spacebot
Length of output: 2025
Make the clickable cards keyboard-accessible.
Lines 247-250 and 306-309 use div elements as clickable controls with only onClick handlers. Keyboard users cannot open the detail modals because these divs aren't focusable and don't handle keyboard input.
♭ Suggested fix
<div
+ role="button"
+ tabIndex={0}
className="flex cursor-pointer flex-col rounded-lg border border-app-line bg-app-box p-4 transition-colors hover:border-app-line-hover"
onClick={onClick}
+ onKeyDown={(e) => {
+ if (e.key === "Enter" || e.key === " ") {
+ e.preventDefault();
+ onClick();
+ }
+ }}
>📝 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
| <div | |
| className="flex cursor-pointer flex-col rounded-lg border border-app-line bg-app-box p-4 transition-colors hover:border-app-line-hover" | |
| onClick={onClick} | |
| > | |
| <div | |
| role="button" | |
| tabIndex={0} | |
| className="flex cursor-pointer flex-col rounded-lg border border-app-line bg-app-box p-4 transition-colors hover:border-app-line-hover" | |
| onClick={onClick} | |
| onKeyDown={(e) => { | |
| if (e.key === "Enter" || e.key === " ") { | |
| e.preventDefault(); | |
| onClick(); | |
| } | |
| }} | |
| > |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@interface/src/routes/AgentSkills.tsx` around lines 247 - 250, The clickable
card divs (the element with className "flex cursor-pointer ..." in
AgentSkills.tsx and the similar card at lines ~306-309) are not
keyboard-accessible; replace or augment these divs so they are focusable and
respond to keyboard activation by either changing them to a semantic <button> or
adding tabindex={0}, role="button", and an onKeyDown handler that triggers the
same onClick logic when Enter or Space is pressed. Update the JSX for the card
components and ensure the existing onClick handler (onClick) is reused by the
key handler or passed to the button to avoid duplicate behavior.
Comment on lines
+203
to
+237
| // For single-repo projects, worktrees live in the parent | ||
| // directory. Compute the relative path accordingly. | ||
| let (name, relative_path) = if is_root_repo { | ||
| let name = worktree | ||
| .path | ||
| .file_name() | ||
| .map(|n| n.to_string_lossy().to_string()) | ||
| .unwrap_or_default(); | ||
| // Store as relative to the parent directory (e.g. "../feat-branch"). | ||
| let parent = root.parent(); | ||
| let rel = parent | ||
| .and_then(|p| worktree.path.strip_prefix(p).ok()) | ||
| .map(|p| format!("../{}", p.to_string_lossy())) | ||
| .unwrap_or_else(|| worktree.path.to_string_lossy().to_string()); | ||
| (name, rel) | ||
| } else { | ||
| let relative_path = worktree | ||
| .path | ||
| .strip_prefix(root) | ||
| .map(|p| p.to_string_lossy().to_string()) | ||
| .unwrap_or_else(|_| { | ||
| worktree | ||
| .path | ||
| .file_name() | ||
| .map(|n| n.to_string_lossy().to_string()) | ||
| .unwrap_or_default() | ||
| }); | ||
|
|
||
| let name = worktree | ||
| .path | ||
| .file_name() | ||
| .map(|n| n.to_string_lossy().to_string()) | ||
| .unwrap_or_default(); | ||
| (name, relative_path) | ||
| }; |
Contributor
There was a problem hiding this comment.
This still corrupts discovered paths for external worktrees.
If a repo already has a worktree outside project.root_path, Line 221-229 falls back to the basename and registers a fake in-project path. From there, delete/disk-usage operations resolve the wrong directory. Please either skip those worktrees or store a path that still round-trips to the actual location.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/projects.rs` around lines 203 - 237, The current logic in the
worktree handling block (variables: is_root_repo, worktree.path, root, producing
name and relative_path) falls back to using the file_name when
strip_prefix(root) fails, which registers a fake in-project path for external
worktrees; instead, either skip external worktrees altogether or preserve a path
that round-trips to the real location by using the absolute worktree.path when
strip_prefix(root) returns Err. Update the branch that computes relative_path
to: if worktree.path.strip_prefix(root) succeeds use that relative string,
otherwise set relative_path to worktree.path.to_string_lossy().to_string() (or
omit the worktree from the results), and ensure name is still derived from
file_name if desired; apply this change where name and relative_path are
assigned so delete/disk-usage resolve the correct directory.
Comment on lines
+343
to
+370
| let data = field.bytes().await.map_err(|error| { | ||
| tracing::warn!(%error, "failed to read upload field"); | ||
| StatusCode::BAD_REQUEST | ||
| })?; | ||
|
|
||
| if data.is_empty() { | ||
| continue; | ||
| } | ||
|
|
||
| // Write to a temp file, then install via the existing installer | ||
| let temp_dir = tempfile::tempdir().map_err(|error| { | ||
| tracing::warn!(%error, "failed to create temp dir"); | ||
| StatusCode::INTERNAL_SERVER_ERROR | ||
| })?; | ||
|
|
||
| let temp_path = temp_dir.path().join(&filename); | ||
| let mut file = tokio::fs::File::create(&temp_path).await.map_err(|error| { | ||
| tracing::warn!(%error, "failed to create temp file"); | ||
| StatusCode::INTERNAL_SERVER_ERROR | ||
| })?; | ||
| file.write_all(&data).await.map_err(|error| { | ||
| tracing::warn!(%error, "failed to write temp file"); | ||
| StatusCode::INTERNAL_SERVER_ERROR | ||
| })?; | ||
| file.sync_all().await.map_err(|error| { | ||
| tracing::warn!(%error, "failed to sync temp file"); | ||
| StatusCode::INTERNAL_SERVER_ERROR | ||
| })?; |
Contributor
There was a problem hiding this comment.
Stream uploads to disk instead of buffering the whole archive.
Line 343 loads each multipart field into memory before writing it out. A large .zip/.skill upload can exhaust the worker long before install_from_file runs. Stream the field into the temp file chunk-by-chunk and reject oversize uploads explicitly.
📦 Suggested direction
- let data = field.bytes().await.map_err(|error| {
- tracing::warn!(%error, "failed to read upload field");
- StatusCode::BAD_REQUEST
- })?;
-
- if data.is_empty() {
- continue;
- }
-
// Write to a temp file, then install via the existing installer
let temp_dir = tempfile::tempdir().map_err(|error| {
tracing::warn!(%error, "failed to create temp dir");
StatusCode::INTERNAL_SERVER_ERROR
})?;
@@
let temp_path = temp_dir.path().join(&filename);
let mut file = tokio::fs::File::create(&temp_path).await.map_err(|error| {
tracing::warn!(%error, "failed to create temp file");
StatusCode::INTERNAL_SERVER_ERROR
})?;
- file.write_all(&data).await.map_err(|error| {
- tracing::warn!(%error, "failed to write temp file");
- StatusCode::INTERNAL_SERVER_ERROR
- })?;
+ let mut field = field;
+ let mut wrote_any = false;
+ while let Some(chunk) = field.chunk().await.map_err(|error| {
+ tracing::warn!(%error, "failed to read upload chunk");
+ StatusCode::BAD_REQUEST
+ })? {
+ wrote_any = true;
+ file.write_all(&chunk).await.map_err(|error| {
+ tracing::warn!(%error, "failed to write temp file");
+ StatusCode::INTERNAL_SERVER_ERROR
+ })?;
+ }
+ if !wrote_any {
+ continue;
+ }📝 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
| let data = field.bytes().await.map_err(|error| { | |
| tracing::warn!(%error, "failed to read upload field"); | |
| StatusCode::BAD_REQUEST | |
| })?; | |
| if data.is_empty() { | |
| continue; | |
| } | |
| // Write to a temp file, then install via the existing installer | |
| let temp_dir = tempfile::tempdir().map_err(|error| { | |
| tracing::warn!(%error, "failed to create temp dir"); | |
| StatusCode::INTERNAL_SERVER_ERROR | |
| })?; | |
| let temp_path = temp_dir.path().join(&filename); | |
| let mut file = tokio::fs::File::create(&temp_path).await.map_err(|error| { | |
| tracing::warn!(%error, "failed to create temp file"); | |
| StatusCode::INTERNAL_SERVER_ERROR | |
| })?; | |
| file.write_all(&data).await.map_err(|error| { | |
| tracing::warn!(%error, "failed to write temp file"); | |
| StatusCode::INTERNAL_SERVER_ERROR | |
| })?; | |
| file.sync_all().await.map_err(|error| { | |
| tracing::warn!(%error, "failed to sync temp file"); | |
| StatusCode::INTERNAL_SERVER_ERROR | |
| })?; | |
| // Write to a temp file, then install via the existing installer | |
| let temp_dir = tempfile::tempdir().map_err(|error| { | |
| tracing::warn!(%error, "failed to create temp dir"); | |
| StatusCode::INTERNAL_SERVER_ERROR | |
| })?; | |
| let temp_path = temp_dir.path().join(&filename); | |
| let mut file = tokio::fs::File::create(&temp_path).await.map_err(|error| { | |
| tracing::warn!(%error, "failed to create temp file"); | |
| StatusCode::INTERNAL_SERVER_ERROR | |
| })?; | |
| let mut field = field; | |
| let mut wrote_any = false; | |
| while let Some(chunk) = field.chunk().await.map_err(|error| { | |
| tracing::warn!(%error, "failed to read upload chunk"); | |
| StatusCode::BAD_REQUEST | |
| })? { | |
| wrote_any = true; | |
| file.write_all(&chunk).await.map_err(|error| { | |
| tracing::warn!(%error, "failed to write temp file"); | |
| StatusCode::INTERNAL_SERVER_ERROR | |
| })?; | |
| } | |
| if !wrote_any { | |
| continue; | |
| } | |
| file.sync_all().await.map_err(|error| { | |
| tracing::warn!(%error, "failed to sync temp file"); | |
| StatusCode::INTERNAL_SERVER_ERROR | |
| })?; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/skills.rs` around lines 343 - 370, The handler currently calls
field.bytes().await which buffers the entire upload into memory; replace that
with streaming writes to disk: create the temp_dir and temp_path and open
tokio::fs::File for writing before reading the field, then iterate the multipart
field's async chunk stream (use the field's async chunk/stream API) and write
each chunk to file with file.write_all(...).await, tracking a running total and
explicitly reject with StatusCode::BAD_REQUEST (and a warning log) if total
exceeds a configured MAX_UPLOAD_BYTES; preserve the existing error mappings/logs
for temp dir/file creation, write and sync operations and keep the zero-length
check by validating total == 0 after streaming, then call the existing installer
(install_from_file or similar) with the temp_path.
Comment on lines
+280
to
+292
| let relative_path = if is_root_repo { | ||
| let parent = root.parent(); | ||
| parent | ||
| .and_then(|p| worktree_info.path.strip_prefix(p).ok()) | ||
| .map(|p| format!("../{}", p.to_string_lossy())) | ||
| .unwrap_or_else(|| worktree_info.path.to_string_lossy().to_string()) | ||
| } else { | ||
| worktree_info | ||
| .path | ||
| .strip_prefix(root) | ||
| .map(|path| path.to_string_lossy().to_string()) | ||
| .unwrap_or_else(|_| worktree_name.clone()) | ||
| }; |
Contributor
There was a problem hiding this comment.
Don't invent an in-project path for out-of-root worktrees.
When strip_prefix(root) fails on Line 289 or Line 430, the fallback on Line 291/432 stores only worktree_name. That turns an external worktree like /tmp/feat-x into feat-x, so later remove/disk-usage operations point at root/feat-x instead of the real directory. Skip those entries with a warning, or persist an unambiguous path format.
Also applies to: 421-433
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tools/project_manage.rs` around lines 280 - 292, The current computation
of relative_path in the worktree handling (the block using is_root_repo, root,
worktree_info, and worktree_name) incorrectly falls back to storing
worktree_name when strip_prefix(root) fails, which misrepresents out-of-root
worktrees; update the logic in both occurrences (the relative_path assignment
around worktree_info.path.strip_prefix(root) and the similar block at 421-433)
to either skip non-descendent worktrees with a logged warning (using process or
logger used in this module) or persist an unambiguous absolute/prefixed path
(e.g., keep full worktree_info.path or prefix with "../") instead of just
worktree_name so remove/disk-usage operations do not target root/worktree_name
by mistake. Ensure references to is_root_repo, root, worktree_info.path, and
worktree_name are used to detect out-of-root cases and handle them consistently.
rktmeister
pushed a commit
to rktmeister/spacebot
that referenced
this pull request
Mar 11, 2026
…epo-projects-and-skills-ui feat: support single-repo projects and enhance skills UI
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
discover_repos()now detects when the project root itself is a git repo and registers it withrelative_path: ".". Previously, only child directories were scanned, causing single-repo projects to show nothing in the UI.git worktreeconventions. Stored as../namerelative paths in the DB..zip/.skillarchives directly from their machine.GET /agents/skills/content,POST /agents/skills/upload,GET /skills/registry/contentwith GitHub raw content fetching and Moka caching.SkillContentResponse,UploadSkillResponse,RegistrySkillContentResponsetypes and corresponding client methods.Changes
src/projects/git.rs.gitbefore scanning children; addis_single_repo_project()helpersrc/api/projects.rs"."repo path in worktree discovery, creation, and deletionsrc/tools/project_manage.rssrc/api/skills.rssrc/api/server.rsinterface/src/routes/AgentProjects.tsxinterface/src/routes/AgentSkills.tsxinterface/src/api/client.tsNote
This PR extends single-repo project detection—previously only multi-repo scenarios were handled correctly. Projects now auto-detect whether the root is itself a git repo and adjust worktree placement accordingly (placing them in the parent directory rather than inside the project root). On the skills side, users can now click skill cards to view full SKILL.md content in modals, upload skills directly from disk via zip/skill archives, and inspect registry skills with source links and GitHub content fetching.
Written by Tembo for commit 3c56dde. This will update automatically on new commits.