Open
Conversation
Contributor
|
I already have a PR for "Fixed compile issues in browser tool URL parsing and API auth middleware extractor" here |
WalkthroughAdds optional description/total fields to registry interfaces, enables className on Markdown, deduplicates channel history merges, refactors multiple Agent/Channel UIs, implements cached on-demand GitHub SKILL.md enrichment, and changes SQL timestamp comparison to use datetime(). Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant API as Backend API
participant Cache as Description Cache
participant GitHub as GitHub Repo
Client->>API: registry_browse / registry_search
activate API
API->>Cache: lookup descriptions for skill keys
alt cached
Cache-->>API: cached descriptions
else not cached
API->>GitHub: fetch SKILL.md candidates (owner/repo, branches)
activate GitHub
GitHub-->>API: SKILL.md content or 404
deactivate GitHub
API->>API: parse Markdown -> extract description
API->>Cache: store description
end
API->>API: attach description & total to response
API-->>Client: RegistryBrowseResponse (with description/total)
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
Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
interface/src/routes/ChannelDetail.tsx (1)
153-183:⚠️ Potential issue | 🔴 CriticalFix WorkerRunItem JSX + missing expansion state.
expandedis referenced without state and theLinkis closed with</button>, which will fail to compile and breaks expansion behavior. Add local state, a toggle, and close theLinkcorrectly (or restructure the wrapper to avoid nested interactive elements).🛠️ Suggested fix (adds state + toggle and corrects JSX structure)
function WorkerRunItem({ item, agentId }: { item: TimelineWorkerRun; agentId: string }) { + const [expanded, setExpanded] = useState(false); return ( <div className="flex gap-3 px-3 py-2"> <span className="flex-shrink-0 pt-0.5 text-tiny text-ink-faint"> {formatTimestamp(new Date(item.started_at).getTime())} </span> <div className="min-w-0 flex-1"> - <Link - to="/agents/$agentId/workers" - params={{ agentId }} - search={{ worker: item.id }} - className="block w-full rounded-md bg-amber-500/10 px-3 py-2 text-left transition-colors hover:bg-amber-500/15" - > - <div className="flex min-w-0 items-center gap-2 overflow-hidden"> - <div className="h-2 w-2 rounded-full bg-amber-400/50" /> - <span className="text-sm font-medium text-amber-300">Worker</span> - <span className="min-w-0 flex-1 truncate text-sm text-ink-dull">{item.task}</span> - {item.result && ( - <span className="flex-shrink-0 self-start text-tiny leading-5 text-ink-faint"> - {expanded ? "▾" : "▸"} - </span> - )} - </div> - </button> + <div className="rounded-md bg-amber-500/10 px-3 py-2 text-left transition-colors hover:bg-amber-500/15"> + <div className="flex min-w-0 items-center gap-2 overflow-hidden"> + <div className="h-2 w-2 rounded-full bg-amber-400/50" /> + <span className="text-sm font-medium text-amber-300">Worker</span> + <Link + to="/agents/$agentId/workers" + params={{ agentId }} + search={{ worker: item.id }} + className="min-w-0 flex-1 truncate text-sm text-ink-dull hover:text-ink" + > + {item.task} + </Link> + {item.result && ( + <button + type="button" + aria-expanded={expanded} + onClick={(e) => { + e.stopPropagation(); + setExpanded(!expanded); + }} + className="flex-shrink-0 self-start text-tiny leading-5 text-ink-faint" + > + {expanded ? "▾" : "▸"} + </button> + )} + </div> + </div> {expanded && item.result && ( <div className="mt-1 rounded-md border border-amber-500/10 bg-amber-500/5 px-3 py-2"> <div className="text-sm text-ink-dull"> <Markdown className="whitespace-pre-wrap break-words">{item.result}</Markdown> </div> </div> )} </div> </div> ); }🤖 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 153 - 183, The WorkerRunItem component references expanded without state and incorrectly closes the Link with </button>, breaking compilation and expansion behavior; add a local boolean state (e.g., const [expanded, setExpanded] = useState(false>) inside WorkerRunItem, add a toggle handler (setExpanded(prev => !prev)) wired to a non-conflicting control (e.g., an onClick on a chevron span or a dedicated button) and ensure the Link JSX is closed with </Link> (or restructure to avoid nesting interactive elements) so the expansion conditional (expanded && item.result) works and the markup is valid.
♻️ Duplicate comments (1)
src/api/skills.rs (1)
394-405: Negative caching is now correctly implemented — previous concern addressed.
description(Option<String>) is always inserted intoREGISTRY_SKILL_DESCRIPTION_CACHE, includingNonefor skills without aSKILL.md. Both read-sites correctly distinguish a cache miss (get→None) from a cached negative (get→Some(None)), preventing repeated HTTP storms for missing descriptions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/skills.rs` around lines 394 - 405, Negative-caching is intentionally used here: the Option<String> named description (including None when SKILL.md is missing) is always inserted into REGISTRY_SKILL_DESCRIPTION_CACHE, but readers distinguish a cache miss (None) from a cached negative (Some(None)). Add a brief clarifying comment immediately above the REGISTRY_SKILL_DESCRIPTION_CACHE.insert(cache_key, description) call (near join_set.join_next handling and skills.get_mut usage) stating that inserting None is intentional for negative caching and that read-sites must check for Some(None) vs None; no logic change required beyond the comment.
🧹 Nitpick comments (1)
interface/src/routes/AgentSkills.tsx (1)
233-255:githubInstallMutationandinstallMutationshare an identicalmutationFnandonSuccess.Both were kept separate to isolate UI state, which is correct. But the implementation is duplicated. Extract a shared function to keep them DRY:
♻️ Proposed refactor
+ const skillInstallFn = (spec: string) => + api.installSkill({ agent_id: agentId, spec, instance: false }); + const onInstallSuccess = () => + queryClient.invalidateQueries({ queryKey: ["skills", agentId] }); + const installMutation = useMutation({ - mutationFn: (spec: string) => - api.installSkill({ agent_id: agentId, spec, instance: false }), - onSuccess: () => { - queryClient.invalidateQueries({ queryKey: ["skills", agentId] }); - }, + mutationFn: skillInstallFn, + onSuccess: onInstallSuccess, }); const githubInstallMutation = useMutation({ - mutationFn: (spec: string) => - api.installSkill({ agent_id: agentId, spec, instance: false }), - onSuccess: () => { - queryClient.invalidateQueries({ queryKey: ["skills", agentId] }); - }, + mutationFn: skillInstallFn, + onSuccess: onInstallSuccess, });🤖 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 233 - 255, installMutation and githubInstallMutation duplicate the same mutationFn and onSuccess; extract a shared helper (e.g., createInstallMutationOptions or getInstallMutationHandlers) that returns the mutationFn (calling api.installSkill with agent_id: agentId, spec, instance: false) and the onSuccess callback (calling queryClient.invalidateQueries({ queryKey: ["skills", agentId] })), then use that helper when creating both installMutation and githubInstallMutation to remove the duplicated implementation while preserving separate useMutation instances for UI state.
🤖 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/hooks/useChannelLiveState.ts`:
- Around line 586-594: The current code sets hasMore to false when deduplication
yields zero new items (olderItems.length === 0) which can prematurely stop
pagination; change the logic so hasMore reflects the API's data.has_more (do not
override it based on olderItems length), and implement/propagate a cursor from
the response (e.g., response cursor/last_cursor) into the channel state for
subsequent fetches—update the merge block for [channelId] (where existingKeys,
olderItems, itemKey, data.has_more, timeline are used) to store the returned
cursor and use data.has_more directly to decide hasMore so pagination continues
even when dedupe returns no new items.
In `@src/api/skills.rs`:
- Around line 473-480: The nested conditional in the for loop triggers Clippy's
`collapsible_if`; change the logic to an early-continue pattern: inside the for
(index, line) in lines.iter().enumerate() loop, replace the nested if that
checks heading.starts_with('#') && heading.contains("description") with a single
guard that continues when the condition is false, then call
extract_paragraph(&lines[(index + 1)..]) and return Some(description) when it
yields Some. This keeps the same behavior while removing the nested if and
references the existing symbols heading, lines, index, and the
extract_paragraph(...) call.
---
Outside diff comments:
In `@interface/src/routes/ChannelDetail.tsx`:
- Around line 153-183: The WorkerRunItem component references expanded without
state and incorrectly closes the Link with </button>, breaking compilation and
expansion behavior; add a local boolean state (e.g., const [expanded,
setExpanded] = useState(false>) inside WorkerRunItem, add a toggle handler
(setExpanded(prev => !prev)) wired to a non-conflicting control (e.g., an
onClick on a chevron span or a dedicated button) and ensure the Link JSX is
closed with </Link> (or restructure to avoid nesting interactive elements) so
the expansion conditional (expanded && item.result) works and the markup is
valid.
---
Duplicate comments:
In `@src/api/skills.rs`:
- Around line 394-405: Negative-caching is intentionally used here: the
Option<String> named description (including None when SKILL.md is missing) is
always inserted into REGISTRY_SKILL_DESCRIPTION_CACHE, but readers distinguish a
cache miss (None) from a cached negative (Some(None)). Add a brief clarifying
comment immediately above the REGISTRY_SKILL_DESCRIPTION_CACHE.insert(cache_key,
description) call (near join_set.join_next handling and skills.get_mut usage)
stating that inserting None is intentional for negative caching and that
read-sites must check for Some(None) vs None; no logic change required beyond
the comment.
---
Nitpick comments:
In `@interface/src/routes/AgentSkills.tsx`:
- Around line 233-255: installMutation and githubInstallMutation duplicate the
same mutationFn and onSuccess; extract a shared helper (e.g.,
createInstallMutationOptions or getInstallMutationHandlers) that returns the
mutationFn (calling api.installSkill with agent_id: agentId, spec, instance:
false) and the onSuccess callback (calling queryClient.invalidateQueries({
queryKey: ["skills", agentId] })), then use that helper when creating both
installMutation and githubInstallMutation to remove the duplicated
implementation while preserving separate useMutation instances for UI state.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lock,!**/*.lockCargo.tomlis excluded by!**/*.toml
📒 Files selected for processing (6)
interface/src/api/client.tsinterface/src/hooks/useChannelLiveState.tsinterface/src/routes/AgentSkills.tsxinterface/src/routes/ChannelDetail.tsxsrc/api/skills.rssrc/conversation/history.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/conversation/history.rs
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/skills.rs`:
- Around line 394-397: The loop calling join_set.join_next() currently discards
task failures by matching only Ok(...) and continuing on anything else; update
the while let Some(result) = join_set.join_next().await loop to explicitly
handle the Err case (and non-Ok variants) instead of silently continuing:
inspect the Result from join_next(), log the error with context (include
index/cache_key when available) or propagate it up (return Err) so enrichment
failures are visible, and only continue on expected success; change the pattern
matching around the let Ok((index, cache_key, description)) = result else {
continue; } to handle and report the error path for debugging and correctness.
- Around line 438-456: The code in the HTTP fetch loop (using
client.get(&url)...send().await and response.text().await) currently swallows
errors (Err(_) branches) and skips non-success statuses without any logging;
update those branches to log diagnostic details (at debug/trace level) including
the URL, the error returned from the request or text() call, and the
response.status() for non-success cases so failures are observable; locate the
match arms around client.get(...).send().await and the match for
response.text().await in skills.rs and replace the Err(_) and the
!response.status().is_success() paths with calls to your project's logging
facility (e.g., debug!/trace!/error!) that include the url, error, and status to
aid debugging.
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