Open
Conversation
Contributor
|
I already have a PR for "Fixed compile issues in browser tool URL parsing and API auth middleware extractor" here |
WalkthroughThis PR extends public TypeScript interfaces with optional fields for skill descriptions and response totals, adds CSS class support to the Markdown component, refactors multiple agent detail/skills UIs with flex layouts and edit links, enhances channel detail rendering with Markdown whitespace preservation and load throttling, and implements async description enrichment for registry skills with updated SQL timestamp comparison. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant API as skills.rs API
participant Cache as Cache
participant GitHub as GitHub
Client->>API: registry_browse/search request
activate API
API->>Cache: check if descriptions cached for skills
alt Descriptions found in cache
Cache-->>API: cached descriptions
else Descriptions not cached
API->>GitHub: fetch SKILL.md from owner/repo
activate GitHub
GitHub-->>API: SKILL.md content
deactivate GitHub
API->>API: parse markdown & extract description
API->>Cache: store description
end
API->>API: populate RegistrySkill.description & total
API-->>Client: RegistryBrowseResponse with descriptions
deactivate API
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
interface/src/routes/ChannelDetail.tsx (1)
321-331: Minor indentation inconsistency.The cortex toggle wrapper
<div>at line 321 appears to break from the surrounding indentation level (it's inside the<div className="ml-auto flex items-center gap-3">but doesn't share its indent). Not a functional issue, but it makes the JSX structure harder to read at a glance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/routes/ChannelDetail.tsx` around lines 321 - 331, Indentation of the cortex toggle wrapper div in ChannelDetail.tsx is inconsistent; adjust the JSX so the <div className="flex overflow-hidden rounded-md border border-app-line bg-app-darkBox"> (the cortex toggle wrapper containing the Button that calls setCortexOpen and reads cortexOpen) is indented to match the surrounding <div className="ml-auto flex items-center gap-3"> children, keeping the Button and its props (onClick, variant, size, className, title) aligned with the other sibling elements for consistent JSX formatting.src/api/skills.rs (2)
256-263: Description enrichment adds latency to every browse/search response.
enrich_registry_descriptionsruns inline in bothregistry_browseandregistry_searchhandlers, blocking the response until all GitHub fetches complete (up to 3s timeout × 4 candidate paths per skill, though parallelized). First-time page loads with many uncached skills will be noticeably slow.Consider enriching descriptions asynchronously (fire-and-forget background task that populates the cache) and returning whatever is cached, or returning partial results and letting the frontend re-fetch.
Also applies to: 306-313
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/skills.rs` around lines 256 - 263, The call to enrich_registry_descriptions is blocking the HTTP response in registry_browse and registry_search by awaiting GitHub fetches; change to fire-and-forget background enrichment so the handlers return immediately with whatever descriptions are already cached. Concretely, replace the await enrich_registry_descriptions(&client, &mut skills).await with spawning a background task (e.g., tokio::spawn or equivalent) that clones the needed client/config and the list of skill identifiers and calls enrich_registry_descriptions in the background to populate the cache, while the handler returns Json(RegistryBrowseResponse { skills, ... }) (and do the same for the RegistrySearchResponse branch). Ensure any shared cache or client is cloned or Arc-wrapped and handle errors inside the task so the handler never awaits it.
12-13: Unbounded in-memory cache will grow indefinitely.
REGISTRY_SKILL_DESCRIPTION_CACHEhas no eviction policy, TTL, or size limit. Over time, as users browse/search different skills, this HashMap grows without bound. With thousands of registry skills, each storing a description string up to 220 chars, the memory impact is modest per entry but there's no ceiling.Consider adding a bounded cache (e.g., an LRU with a max capacity) or a simple TTL-based eviction. Even a periodic clear on a timer would suffice.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/skills.rs` around lines 12 - 13, REGISTRY_SKILL_DESCRIPTION_CACHE is an unbounded LazyLock<HashMap<String,String>> and will grow indefinitely; replace it with a bounded, concurrent cache (e.g., an LRU or TTL cache) or add periodic eviction: for example swap the LazyLock<RwLock<HashMap<...>>> for a concurrent cache type (moka::sync::Cache or lru::LruCache behind a tokio::sync::Mutex) with a configured max_capacity and/or TTL, update all accesses that read/write REGISTRY_SKILL_DESCRIPTION_CACHE to use the cache's get/insert/evict APIs (or implement a background task that periodically prunes old entries if you prefer TTL), and ensure the chosen cache provides safe async/concurrent usage in place of the current tokio::sync::RwLock wrapper.
🤖 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 353-411: The GitHub form is sharing the global installMutation,
causing its UI states and messages to reflect installs triggered from registry
cards; create a separate mutation for the GitHub form (e.g.,
githubInstallMutation using the same useMutation handler currently used by
installMutation) and replace usages inside the form: onSubmit should call
githubInstallMutation.mutate(spec), the submit button disabled/isPending checks
should use githubInstallMutation.isPending, and the success/error messages
should use githubInstallMutation.isSuccess / githubInstallMutation.isError and
githubInstallMutation.data; leave the existing installMutation in place for
registry card buttons so their feedback remains scoped to registry installs.
In `@src/api/skills.rs`:
- Around line 393-394: The code currently builds raw URLs with a hardcoded
"main" branch (in the loop iterating candidate_paths and building url from
source and path), which fails for repos whose default branch is not main; change
the construction in that loop to resolve the default branch instead — either
call the GitHub REST API (GET /repos/{owner}/{repo}) to read default_branch and
use it when formatting url, or use the raw.githubusercontent.com "HEAD" redirect
by formatting url as "https://raw.githubusercontent.com/{source}/HEAD/{path}" so
the request follows the repo's default branch, and if you prefer robustness add
a small fallback sequence (try HEAD, then "main", then "master") when fetching
each candidate path.
- Around line 316-363: enrich_registry_descriptions is not caching "not found"
results so skills without SKILL.md trigger repeated HTTP calls; change the logic
to store a sentinel (e.g., an empty string or a dedicated marker) in
REGISTRY_SKILL_DESCRIPTION_CACHE when fetch_registry_skill_description returns
None, and when reading from REGISTRY_SKILL_DESCRIPTION_CACHE treat that sentinel
as meaning "looked up but no description" (i.e., set skills[index].description =
None/skip) so failed lookups aren't retried; update the join_set result handling
around fetch_registry_skill_description, the cache.insert call, and the initial
cached read to recognize and handle the sentinel (use registry_skill_key,
REGISTRY_SKILL_DESCRIPTION_CACHE, enrich_registry_descriptions, and
fetch_registry_skill_description to locate changes).
---
Nitpick comments:
In `@interface/src/routes/ChannelDetail.tsx`:
- Around line 321-331: Indentation of the cortex toggle wrapper div in
ChannelDetail.tsx is inconsistent; adjust the JSX so the <div className="flex
overflow-hidden rounded-md border border-app-line bg-app-darkBox"> (the cortex
toggle wrapper containing the Button that calls setCortexOpen and reads
cortexOpen) is indented to match the surrounding <div className="ml-auto flex
items-center gap-3"> children, keeping the Button and its props (onClick,
variant, size, className, title) aligned with the other sibling elements for
consistent JSX formatting.
In `@src/api/skills.rs`:
- Around line 256-263: The call to enrich_registry_descriptions is blocking the
HTTP response in registry_browse and registry_search by awaiting GitHub fetches;
change to fire-and-forget background enrichment so the handlers return
immediately with whatever descriptions are already cached. Concretely, replace
the await enrich_registry_descriptions(&client, &mut skills).await with spawning
a background task (e.g., tokio::spawn or equivalent) that clones the needed
client/config and the list of skill identifiers and calls
enrich_registry_descriptions in the background to populate the cache, while the
handler returns Json(RegistryBrowseResponse { skills, ... }) (and do the same
for the RegistrySearchResponse branch). Ensure any shared cache or client is
cloned or Arc-wrapped and handle errors inside the task so the handler never
awaits it.
- Around line 12-13: REGISTRY_SKILL_DESCRIPTION_CACHE is an unbounded
LazyLock<HashMap<String,String>> and will grow indefinitely; replace it with a
bounded, concurrent cache (e.g., an LRU or TTL cache) or add periodic eviction:
for example swap the LazyLock<RwLock<HashMap<...>>> for a concurrent cache type
(moka::sync::Cache or lru::LruCache behind a tokio::sync::Mutex) with a
configured max_capacity and/or TTL, update all accesses that read/write
REGISTRY_SKILL_DESCRIPTION_CACHE to use the cache's get/insert/evict APIs (or
implement a background task that periodically prunes old entries if you prefer
TTL), and ensure the chosen cache provides safe async/concurrent usage in place
of the current tokio::sync::RwLock wrapper.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
interface/src/api/client.tsinterface/src/components/Markdown.tsxinterface/src/hooks/useChannelLiveState.tsinterface/src/routes/AgentDetail.tsxinterface/src/routes/AgentSkills.tsxinterface/src/routes/ChannelDetail.tsxsrc/api/skills.rssrc/conversation/history.rs
Comment on lines
+353
to
+411
| <div className="rounded-lg border border-app-line bg-app-box p-6"> | ||
| <h3 className="text-sm font-medium text-ink"> | ||
| Install from GitHub | ||
| </h3> | ||
| <p className="mt-1 text-xs text-ink-faint"> | ||
| Install any skill from a GitHub repository | ||
| </p> | ||
| <form | ||
| onSubmit={(e) => { | ||
| e.preventDefault(); | ||
| const formData = new FormData(e.currentTarget); | ||
| const spec = formData.get("spec") as string; | ||
| if (spec) { | ||
| installMutation.mutate(spec); | ||
| e.currentTarget.reset(); | ||
| } | ||
| }} | ||
| className="mt-3 flex gap-2" | ||
| > | ||
| <input | ||
| type="text" | ||
| name="spec" | ||
| placeholder="owner/repo or owner/repo/skill-name" | ||
| className="flex-1 rounded-md border border-app-line bg-app-darkBox px-3 py-2 text-sm text-ink placeholder-ink-faint focus:border-accent focus:outline-none" | ||
| /> | ||
| <Button | ||
| type="submit" | ||
| variant="default" | ||
| size="default" | ||
| disabled={installMutation.isPending} | ||
| > | ||
| {installMutation.isPending ? ( | ||
| <> | ||
| <FontAwesomeIcon | ||
| icon={faSpinner} | ||
| className="animate-spin" | ||
| /> | ||
| Installing... | ||
| </> | ||
| ) : ( | ||
| <> | ||
| <FontAwesomeIcon icon={faDownload} /> | ||
| Install | ||
| </> | ||
| )} | ||
| </Button> | ||
| </form> | ||
| {installMutation.isError && ( | ||
| <p className="mt-2 text-xs text-red-400"> | ||
| Failed to install skill. Check the repository format. | ||
| </p> | ||
| )} | ||
| {installMutation.isSuccess && ( | ||
| <p className="mt-2 text-xs text-green-400"> | ||
| Installed {installMutation.data.installed.length} skill(s):{" "} | ||
| {installMutation.data.installed.join(", ")} | ||
| </p> | ||
| )} | ||
| </div> |
There was a problem hiding this comment.
Shared installMutation causes leaked feedback between the GitHub form and registry cards.
The "Install from GitHub" form and the registry card install buttons share the same installMutation. This means:
- The form's success message (lines 405–410) will display even when a registry card triggered the install.
- The form's "Installing..." state (line 382) activates when any registry card install is pending.
installMutation.isErrorunder the form fires for registry card failures too.
Consider using a separate mutation for the GitHub form, or at minimum, track which source triggered the install to scope the feedback messages.
🤖 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 353 - 411, The GitHub form
is sharing the global installMutation, causing its UI states and messages to
reflect installs triggered from registry cards; create a separate mutation for
the GitHub form (e.g., githubInstallMutation using the same useMutation handler
currently used by installMutation) and replace usages inside the form: onSubmit
should call githubInstallMutation.mutate(spec), the submit button
disabled/isPending checks should use githubInstallMutation.isPending, and the
success/error messages should use githubInstallMutation.isSuccess /
githubInstallMutation.isError and githubInstallMutation.data; leave the existing
installMutation in place for registry card buttons so their feedback remains
scoped to registry installs.
Comment on lines
+316
to
+363
| async fn enrich_registry_descriptions(client: &reqwest::Client, skills: &mut [RegistrySkill]) { | ||
| let mut join_set = tokio::task::JoinSet::new(); | ||
|
|
||
| for index in 0..skills.len() { | ||
| if skills[index] | ||
| .description | ||
| .as_ref() | ||
| .is_some_and(|description| !description.trim().is_empty()) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| let source = skills[index].source.clone(); | ||
| let skill_id = skills[index].skill_id.clone(); | ||
| let cache_key = registry_skill_key(&source, &skill_id); | ||
|
|
||
| let cached = { | ||
| let cache = REGISTRY_SKILL_DESCRIPTION_CACHE.read().await; | ||
| cache.get(&cache_key).cloned() | ||
| }; | ||
|
|
||
| if let Some(description) = cached { | ||
| skills[index].description = Some(description); | ||
| continue; | ||
| } | ||
|
|
||
| let client = client.clone(); | ||
| join_set.spawn(async move { | ||
| let description = fetch_registry_skill_description(&client, &source, &skill_id).await; | ||
| (index, cache_key, description) | ||
| }); | ||
| } | ||
|
|
||
| while let Some(result) = join_set.join_next().await { | ||
| let Ok((index, cache_key, description)) = result else { | ||
| continue; | ||
| }; | ||
| let Some(description) = description else { | ||
| continue; | ||
| }; | ||
|
|
||
| if let Some(skill) = skills.get_mut(index) { | ||
| skill.description = Some(description.clone()); | ||
| } | ||
|
|
||
| let mut cache = REGISTRY_SKILL_DESCRIPTION_CACHE.write().await; | ||
| cache.insert(cache_key, description); | ||
| } |
There was a problem hiding this comment.
No negative caching causes repeated HTTP storms for skills without SKILL.md.
When fetch_registry_skill_description returns None (lines 353-355), nothing is cached. On the next browse/search request, the same skill triggers another round of up to 4 HTTP requests to GitHub. For skills that genuinely have no SKILL.md, this repeats on every page load.
Cache a sentinel value (e.g., empty string or a dedicated "not found" marker) so failed lookups aren't retried every time.
Proposed fix
while let Some(result) = join_set.join_next().await {
let Ok((index, cache_key, description)) = result else {
continue;
};
- let Some(description) = description else {
- continue;
- };
+ let description = description.unwrap_or_default();
if let Some(skill) = skills.get_mut(index) {
- skill.description = Some(description.clone());
+ if !description.is_empty() {
+ skill.description = Some(description.clone());
+ }
}
let mut cache = REGISTRY_SKILL_DESCRIPTION_CACHE.write().await;
cache.insert(cache_key, description);
}Then when reading from the cache, treat an empty string as "already looked up, no description found":
if let Some(description) = cached {
- skills[index].description = Some(description);
+ if !description.is_empty() {
+ skills[index].description = Some(description);
+ }
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
| async fn enrich_registry_descriptions(client: &reqwest::Client, skills: &mut [RegistrySkill]) { | |
| let mut join_set = tokio::task::JoinSet::new(); | |
| for index in 0..skills.len() { | |
| if skills[index] | |
| .description | |
| .as_ref() | |
| .is_some_and(|description| !description.trim().is_empty()) | |
| { | |
| continue; | |
| } | |
| let source = skills[index].source.clone(); | |
| let skill_id = skills[index].skill_id.clone(); | |
| let cache_key = registry_skill_key(&source, &skill_id); | |
| let cached = { | |
| let cache = REGISTRY_SKILL_DESCRIPTION_CACHE.read().await; | |
| cache.get(&cache_key).cloned() | |
| }; | |
| if let Some(description) = cached { | |
| skills[index].description = Some(description); | |
| continue; | |
| } | |
| let client = client.clone(); | |
| join_set.spawn(async move { | |
| let description = fetch_registry_skill_description(&client, &source, &skill_id).await; | |
| (index, cache_key, description) | |
| }); | |
| } | |
| while let Some(result) = join_set.join_next().await { | |
| let Ok((index, cache_key, description)) = result else { | |
| continue; | |
| }; | |
| let Some(description) = description else { | |
| continue; | |
| }; | |
| if let Some(skill) = skills.get_mut(index) { | |
| skill.description = Some(description.clone()); | |
| } | |
| let mut cache = REGISTRY_SKILL_DESCRIPTION_CACHE.write().await; | |
| cache.insert(cache_key, description); | |
| } | |
| async fn enrich_registry_descriptions(client: &reqwest::Client, skills: &mut [RegistrySkill]) { | |
| let mut join_set = tokio::task::JoinSet::new(); | |
| for index in 0..skills.len() { | |
| if skills[index] | |
| .description | |
| .as_ref() | |
| .is_some_and(|description| !description.trim().is_empty()) | |
| { | |
| continue; | |
| } | |
| let source = skills[index].source.clone(); | |
| let skill_id = skills[index].skill_id.clone(); | |
| let cache_key = registry_skill_key(&source, &skill_id); | |
| let cached = { | |
| let cache = REGISTRY_SKILL_DESCRIPTION_CACHE.read().await; | |
| cache.get(&cache_key).cloned() | |
| }; | |
| if let Some(description) = cached { | |
| if !description.is_empty() { | |
| skills[index].description = Some(description); | |
| } | |
| continue; | |
| } | |
| let client = client.clone(); | |
| join_set.spawn(async move { | |
| let description = fetch_registry_skill_description(&client, &source, &skill_id).await; | |
| (index, cache_key, description) | |
| }); | |
| } | |
| while let Some(result) = join_set.join_next().await { | |
| let Ok((index, cache_key, description)) = result else { | |
| continue; | |
| }; | |
| let description = description.unwrap_or_default(); | |
| if let Some(skill) = skills.get_mut(index) { | |
| if !description.is_empty() { | |
| skill.description = Some(description.clone()); | |
| } | |
| } | |
| let mut cache = REGISTRY_SKILL_DESCRIPTION_CACHE.write().await; | |
| cache.insert(cache_key, description); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/skills.rs` around lines 316 - 363, enrich_registry_descriptions is
not caching "not found" results so skills without SKILL.md trigger repeated HTTP
calls; change the logic to store a sentinel (e.g., an empty string or a
dedicated marker) in REGISTRY_SKILL_DESCRIPTION_CACHE when
fetch_registry_skill_description returns None, and when reading from
REGISTRY_SKILL_DESCRIPTION_CACHE treat that sentinel as meaning "looked up but
no description" (i.e., set skills[index].description = None/skip) so failed
lookups aren't retried; update the join_set result handling around
fetch_registry_skill_description, the cache.insert call, and the initial cached
read to recognize and handle the sentinel (use registry_skill_key,
REGISTRY_SKILL_DESCRIPTION_CACHE, enrich_registry_descriptions, and
fetch_registry_skill_description to locate changes).
Comment on lines
+393
to
+394
| for path in candidate_paths.drain(..) { | ||
| let url = format!("https://raw.githubusercontent.com/{source}/main/{path}"); |
There was a problem hiding this comment.
Hardcoded main branch will miss repos using master or other default branches.
Line 394 always constructs URLs with /main/. Repos that use master, develop, or any other default branch name will never resolve.
Consider trying a fallback branch (e.g., master) or using the GitHub API to resolve the default branch. Alternatively, GitHub's raw content CDN redirects HEAD → default branch if you use the API endpoint instead of raw.githubusercontent.com.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/skills.rs` around lines 393 - 394, The code currently builds raw URLs
with a hardcoded "main" branch (in the loop iterating candidate_paths and
building url from source and path), which fails for repos whose default branch
is not main; change the construction in that loop to resolve the default branch
instead — either call the GitHub REST API (GET /repos/{owner}/{repo}) to read
default_branch and use it when formatting url, or use the
raw.githubusercontent.com "HEAD" redirect by formatting url as
"https://raw.githubusercontent.com/{source}/HEAD/{path}" so the request follows
the repo's default branch, and if you prefer robustness add a small fallback
sequence (try HEAD, then "main", then "master") when fetching each candidate
path.
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
Polishes the web UI across channels, skills, and overview pages, and fixes a few backend/API issues that were causing incorrect behavior in pagination and registry data.
Changes
Channels
Skills
outlinestyle.Overview
Shared UI
Backend
browsertool URL parsing and API auth middleware extractor.totalpassthrough and description enrichment fallback in skills API.Testing
cargo checknpm run build