feat(skills): readd skills.sh #2017
Conversation
…killssh-sync-to-the-skills-library # Conflicts: # src/main/core/skills/SkillsService.ts
Screen.Recording.2026-05-14.at.13.02.33.mov(dont mind my macbook dying; also will still make some lil design adjustens) |
Greptile SummaryThis PR replaces the direct GitHub API catalog fetcher (OpenAI + Anthropic repos) with a skills.sh-backed integration, adds a real-time debounced search flow, and refreshes the Skills UI with skeleton loading states, install-count metadata, and a revised icon hierarchy.
Confidence Score: 4/5Safe to merge after addressing one defect: installing from search can write the wrong repo's files to disk when two repos share a skill name. The searchSkillCache is keyed by CatalogSkill.id = entry.skillId, which is only unique within a single skills.sh repo. When a search returns two repos with identical skill names, the cache retains only the last entry, but both cards may render. Installing either one routes through the same cache key, so the wrong repo's files can be written to disk. src/shared/skills/skillssh.ts (toSkillsshCatalogSkill line 39) and src/main/core/skills/SkillsService.ts (fetchSkillsshSearch lines 667-674, searchSkillCache population lines 173-174)
|
| Filename | Overview |
|---|---|
| src/main/core/skills/SkillsService.ts | Replaces direct GitHub catalog fetching with skills.sh API integration; contains a P1 id-collision bug in searchSkillCache that can install the wrong skill when multiple repos share a skillId. |
| src/shared/skills/skillssh.ts | New helper module; root of the id-collision issue: toSkillsshCatalogSkill sets id from entry.skillId (non-unique across repos) instead of the API's full entry.id path. |
| src/renderer/features/skills/components/useSkills.ts | Adds debounced remote search with keepPreviousData and per-query cache; correctly gates hasActiveSearch behind debounce settlement. |
| src/renderer/features/skills/components/SkillsView.tsx | Replaces static skill grid with debounced search UX, animated skeletons, and empty/no-results states. |
| src/renderer/features/skills/components/SkillDetailModal.tsx | Adds isLoading skeleton, richer source header with repo slug and install count; getSourceIconUrl now correctly reads repoSlug owner. |
| src/renderer/features/skills/components/SkillCard.tsx | Adds install-count badge, installed chip, and subtitle metadata; removes framer-motion in favour of CSS active:scale. |
| src/renderer/features/skills/components/SkillIconRenderer.tsx | Adds GitHub owner avatar fallback tier; caches processed SVG strings in a module-level map. |
| src/main/core/skills/controller.ts | Exposes new searchCatalog RPC endpoint with standard error handling. |
| src/shared/skills/types.ts | Adds 'skillssh' to CatalogSkill.source union, plus installs and repoSlug optional fields. |
| src/renderer/index.css | Adds skeleton-shimmer utility class with prefers-reduced-motion media query. |
Sequence Diagram
sequenceDiagram
participant UI as SkillsView
participant Hook as useSkills
participant RPC as skills RPC
participant Svc as SkillsService
participant SH as skills.sh API
UI->>Hook: setSearchQuery(text)
Note over Hook: debounce 350ms
Hook->>RPC: "searchCatalog({query})"
RPC->>Svc: searchCatalog(q)
Svc->>SH: "GET /api/search?q=...&limit=50"
SH-->>Svc: "{skills: [{id, skillId, source, installs}]}"
Note over Svc: populate searchSkillCache[skillId]
Svc-->>RPC: CatalogIndex (hydrated + local matches)
RPC-->>Hook: searchCatalog result
Hook-->>UI: render SkillCards
UI->>Hook: onInstall(skill.id)
Hook->>RPC: "install({skillId})"
RPC->>Svc: installSkill(skillId)
Svc->>Svc: findCatalogSkill(skillId) searchSkillCache
Svc->>SH: "GET /api/download/{owner}/{repo}/{skillId}"
SH-->>Svc: "{files: [{path, contents}]}"
Note over Svc: writeSkillsshFiles (path-traversal check)
Svc-->>RPC: installed CatalogSkill
RPC-->>Hook: success markSkillInstalled in cache
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
src/main/core/skills/SkillsService.ts:173-174
**Wrong skill installed when `skillId` collides across repos**
`toSkillsshCatalogSkill` sets `CatalogSkill.id = entry.skillId`, which is only unique within a single repository. The skills.sh API's canonical unique identifier is the full `entry.id` (e.g., `"owner/repo/brainstorming"`). When a search returns two skills from different repos with the same `skillId`, `searchSkillCache.set(skill.id, skill)` overwrites the first with the second — yet both may appear as separate cards in the UI. When the user clicks Install on the first card, `installSkill('review')` → `findCatalogSkill('review')` → returns the wrong repo's cached skill → downloads and installs that repo's files instead.
The fix requires threading `entry.id` from `SkillsshSearchResponse` through `SkillsshListSkill` and `toSkillsshCatalogSkill` so the skills.sh full path becomes the canonical `CatalogSkill.id`. Note that `installSkill` also uses `skillId` to name the local directory (`path.join(SKILLS_ROOT, skillId)`), so the directory naming scheme will need adjustment alongside the id change.
Reviews (4): Last reviewed commit: "Fix skills install state refresh" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const catalogPattern = new RegExp(String.raw`(\[\{\\"source\\"[\s\S]*?\}\]),\\"totalSkills\\"`); | ||
| const match = html.match(catalogPattern); | ||
| if (!match) { | ||
| throw new Error('Unable to parse skills.sh catalog'); | ||
| } | ||
|
|
||
| return JSON.parse(match[1].replace(/\\"/g, '"')) as SkillsshListSkill[]; | ||
| } |
There was a problem hiding this comment.
parseSkillsshCatalogHtml screen-scrapes Next.js RSC serialized state using a regex that targets a specific double-encoded JSON fragment. Any Next.js version bump, server-side restructuring, or CDN caching policy change on skills.sh will cause match to be null, throw "Unable to parse skills.sh catalog", and silently fall back to an arbitrarily stale disk cache with no user-visible warning. Consider using a stable, documented API endpoint (like /api/search?q= already used in fetchSkillsshSearch) for the catalog listing instead of HTML extraction.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/shared/skills/skillssh.ts
Line: 29-36
Comment:
**Fragile HTML scraping as primary catalog source**
`parseSkillsshCatalogHtml` screen-scrapes Next.js RSC serialized state using a regex that targets a specific double-encoded JSON fragment. Any Next.js version bump, server-side restructuring, or CDN caching policy change on skills.sh will cause `match` to be `null`, throw `"Unable to parse skills.sh catalog"`, and silently fall back to an arbitrarily stale disk cache with no user-visible warning. Consider using a stable, documented API endpoint (like `/api/search?q=` already used in `fetchSkillsshSearch`) for the catalog listing instead of HTML extraction.
How can I resolve this? If you propose a fix, please make it concise.| function getSourceIconUrl(source: CatalogSkill['source']): string { | ||
| if (source === 'skillssh') return 'https://github.com/vercel.png'; | ||
| if (source === 'openai') return 'https://github.com/openai.png'; | ||
| return 'https://github.com/anthropics.png'; | ||
| } |
There was a problem hiding this comment.
getSourceIconUrl returns Vercel's GitHub avatar for skillssh skills, and skillIcons.ts maps skillssh to the 'vercel' icon. skills.sh is not a Vercel product. Users will see the Vercel logo next to all skills.sh-sourced skills.
| function getSourceIconUrl(source: CatalogSkill['source']): string { | |
| if (source === 'skillssh') return 'https://github.com/vercel.png'; | |
| if (source === 'openai') return 'https://github.com/openai.png'; | |
| return 'https://github.com/anthropics.png'; | |
| } | |
| function getSourceIconUrl(source: CatalogSkill['source']): string { | |
| if (source === 'skillssh') return 'https://github.com/skills-sh.png'; | |
| if (source === 'openai') return 'https://github.com/openai.png'; | |
| return 'https://github.com/anthropics.png'; | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/renderer/features/skills/components/SkillDetailModal.tsx
Line: 35-39
Comment:
**skills.sh attributed to Vercel**
`getSourceIconUrl` returns Vercel's GitHub avatar for `skillssh` skills, and `skillIcons.ts` maps `skillssh` to the `'vercel'` icon. skills.sh is not a Vercel product. Users will see the Vercel logo next to all skills.sh-sourced skills.
```suggestion
function getSourceIconUrl(source: CatalogSkill['source']): string {
if (source === 'skillssh') return 'https://github.com/skills-sh.png';
if (source === 'openai') return 'https://github.com/openai.png';
return 'https://github.com/anthropics.png';
}
```
How can I resolve this? If you propose a fix, please make it concise.| const sourceIcons: Record<string, string> = { | ||
| skillssh: 'vercel', | ||
| openai: 'openai', | ||
| anthropic: 'anthropic', | ||
| }; |
There was a problem hiding this comment.
vercel icon is used as the source icon for skillssh, which will display the Vercel SVG brand mark next to all skills.sh skills throughout the UI.
| const sourceIcons: Record<string, string> = { | |
| skillssh: 'vercel', | |
| openai: 'openai', | |
| anthropic: 'anthropic', | |
| }; | |
| const sourceIcons: Record<string, string> = { | |
| openai: 'openai', | |
| anthropic: 'anthropic', | |
| }; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/renderer/features/skills/components/skillIcons.ts
Line: 101-105
Comment:
Same incorrect attribution: the `vercel` icon is used as the source icon for `skillssh`, which will display the Vercel SVG brand mark next to all skills.sh skills throughout the UI.
```suggestion
const sourceIcons: Record<string, string> = {
openai: 'openai',
anthropic: 'anthropic',
};
```
How can I resolve this? If you propose a fix, please make it concise.
Greptile SummaryThis PR replaces the OpenAI/Anthropic GitHub-sourced skill catalogs with a single skills.sh integration: a live catalog scraped from the skills.sh homepage, a debounced full-text search endpoint, per-skill file download, and a path-traversal guard for multi-file installs. The renderer gains skeleton loaders, install-count badges, per-owner GitHub avatars, and a search-driven "Browse skills.sh" section.
Confidence Score: 3/5The core install/uninstall path is safe, but the catalog now blocks the UI on a live HTTP scrape of skills.sh at every cold start, and the modal shows the wrong org icon for non-Vercel skills.sh entries. The HTML-scraping catalog loader runs synchronously in front of the disk cache on every app start, adding up to 15 s of latency if skills.sh is slow. The Vercel avatar hardcode in the detail modal will show incorrect branding for the majority of skills.sh entries. The GitHub-specific Accept header is sent to skills.sh endpoints. src/main/core/skills/SkillsService.ts (cold-start blocking), src/shared/skills/skillssh.ts (HTML scraping fragility), src/renderer/features/skills/components/SkillDetailModal.tsx (Vercel avatar hardcode)
|
| Filename | Overview |
|---|---|
| src/main/core/skills/SkillsService.ts | Major refactor swapping GitHub-sourced catalogs for skills.sh; remote fetch on every cold start (15 s timeout before fallback), new writeSkillsshFiles with path-traversal guard, redirect handling generalised to all 3xx. |
| src/shared/skills/skillssh.ts | New module introducing skills.sh integration: HTML catalog scraping, search/detail API types, and helper functions. The HTML-regex catalog parser and hardcoded GitHub Accept header are fragile. |
| src/renderer/features/skills/components/SkillDetailModal.tsx | Adds isLoading skeleton, per-source label/icon, repoSlug link, and installs count. getSourceIconUrl hardcodes the Vercel GitHub avatar for every skillssh skill regardless of actual owner. |
| src/renderer/features/skills/components/useSkills.ts | Adds debounced remote search query, merges local and remote results, exposes isDetailLoading and hasActiveSearch. Logic is sound; keepPreviousData prevents flicker. |
| src/renderer/features/skills/components/SkillsView.tsx | New skeleton loaders, 'Results from skills.sh' section, empty-state with refresh action, and inline search spinner. During debounce, the heading appears while local-catalog results are still shown. |
| src/main/core/skills/SkillsService.test.ts | New tests covering the symlink-safety fix: verifies real agent skill directories are not replaced and foreign symlinks are not removed during unsync. |
Prompt To Fix All With AI
Fix the following 5 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 5
src/renderer/features/skills/components/SkillDetailModal.tsx:35-39
The `getSourceIconUrl` function returns the Vercel GitHub avatar for every `skillssh` skill, regardless of the actual repo owner. Skills hosted on skills.sh come from many organizations (Anthropic, OpenAI, Vercel, community authors, etc.). A user installing an Anthropic skill from skills.sh would see the Vercel logo next to "skills.sh" in the modal header. The `SkillIconRenderer` already solves this correctly by reading `skill.repoSlug` — the same approach should be applied here.
```suggestion
function getSourceIconUrl(skill: CatalogSkill): string {
if (skill.repoSlug) {
const owner = skill.repoSlug.split('/')[0];
if (owner) return `https://github.com/${owner}.png`;
}
if (skill.source === 'openai') return 'https://github.com/openai.png';
return 'https://github.com/anthropics.png';
}
```
### Issue 2 of 5
src/main/core/skills/SkillsService.ts:33
The `Accept: application/vnd.github.v3+json` header is sent to every URL, including `skills.sh/api/search`, `skills.sh/api/download/…`, and the `skills.sh/` catalog HTML page. This is a GitHub REST API content-type hint and is meaningless (and potentially confusing) for skills.sh endpoints. If skills.sh ever uses the `Accept` header to gate content — for example returning a JSON error payload instead of the expected HTML/JSON — this could break catalog loading or search silently.
```suggestion
{ headers: { 'User-Agent': 'emdash-skills', Accept: 'application/json, text/html, */*' } },
```
### Issue 3 of 5
src/shared/skills/skillssh.ts:35
The `replace(/\\"/g, '"')` unescape step will corrupt any real backslash-quote in a description. If skills.sh ships a skill with an escaped quote in its name or description, parsing will silently produce corrupted text or throw on `JSON.parse`.
```suggestion
const jsonStr = match[1].replace(/\\"/g, '"');
return JSON.parse(jsonStr) as SkillsshListSkill[];
```
### Issue 4 of 5
src/main/core/skills/SkillsService.ts:71-81
**Blocking cold-start on every process launch**
`getCatalogIndex` now unconditionally calls `fetchRemoteCatalog` before consulting either the disk cache or the bundled catalog. The 15 s `req.setTimeout` means a slow or unreachable skills.sh would block the Skills view for up to 15 seconds on every app start.
### Issue 5 of 5
src/renderer/features/skills/components/SkillsView.tsx:56-58
**"Results from skills.sh" heading while local results are shown**
During the debounce window (user has typed ≥ 2 chars but the 350 ms has not elapsed), `hasActiveSearch` is already `true` so the section title flips to "Results from skills.sh", yet `filteredSkills` is still derived from the local catalog because `debouncedSearchQuery` hasn't updated.
Reviews (2): Last reviewed commit: "Improve skills loading skeleton" | Re-trigger Greptile
| function getSourceIconUrl(source: CatalogSkill['source']): string { | ||
| if (source === 'skillssh') return 'https://github.com/vercel.png'; | ||
| if (source === 'openai') return 'https://github.com/openai.png'; | ||
| return 'https://github.com/anthropics.png'; | ||
| } |
There was a problem hiding this comment.
getSourceIconUrl function returns the Vercel GitHub avatar for every skillssh skill, regardless of the actual repo owner. Skills hosted on skills.sh come from many organizations (Anthropic, OpenAI, Vercel, community authors, etc.). A user installing an Anthropic skill from skills.sh would see the Vercel logo next to "skills.sh" in the modal header. The SkillIconRenderer already solves this correctly by reading skill.repoSlug — the same approach should be applied here.
| function getSourceIconUrl(source: CatalogSkill['source']): string { | |
| if (source === 'skillssh') return 'https://github.com/vercel.png'; | |
| if (source === 'openai') return 'https://github.com/openai.png'; | |
| return 'https://github.com/anthropics.png'; | |
| } | |
| function getSourceIconUrl(skill: CatalogSkill): string { | |
| if (skill.repoSlug) { | |
| const owner = skill.repoSlug.split('/')[0]; | |
| if (owner) return `https://github.com/${owner}.png`; | |
| } | |
| if (skill.source === 'openai') return 'https://github.com/openai.png'; | |
| return 'https://github.com/anthropics.png'; | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/renderer/features/skills/components/SkillDetailModal.tsx
Line: 35-39
Comment:
The `getSourceIconUrl` function returns the Vercel GitHub avatar for every `skillssh` skill, regardless of the actual repo owner. Skills hosted on skills.sh come from many organizations (Anthropic, OpenAI, Vercel, community authors, etc.). A user installing an Anthropic skill from skills.sh would see the Vercel logo next to "skills.sh" in the modal header. The `SkillIconRenderer` already solves this correctly by reading `skill.repoSlug` — the same approach should be applied here.
```suggestion
function getSourceIconUrl(skill: CatalogSkill): string {
if (skill.repoSlug) {
const owner = skill.repoSlug.split('/')[0];
if (owner) return `https://github.com/${owner}.png`;
}
if (skill.source === 'openai') return 'https://github.com/openai.png';
return 'https://github.com/anthropics.png';
}
```
How can I resolve this? If you propose a fix, please make it concise.| @@ -24,16 +32,16 @@ function httpsGet(url: string, redirectCount = 0): Promise<string> { | |||
| url, | |||
| { headers: { 'User-Agent': 'emdash-skills', Accept: 'application/vnd.github.v3+json' } }, | |||
There was a problem hiding this comment.
Accept: application/vnd.github.v3+json header is sent to every URL, including skills.sh/api/search, skills.sh/api/download/…, and the skills.sh/ catalog HTML page. This is a GitHub REST API content-type hint and is meaningless (and potentially confusing) for skills.sh endpoints. If skills.sh ever uses the Accept header to gate content — for example returning a JSON error payload instead of the expected HTML/JSON — this could break catalog loading or search silently.
| { headers: { 'User-Agent': 'emdash-skills', Accept: 'application/vnd.github.v3+json' } }, | |
| { headers: { 'User-Agent': 'emdash-skills', Accept: 'application/json, text/html, */*' } }, |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/main/core/skills/SkillsService.ts
Line: 33
Comment:
The `Accept: application/vnd.github.v3+json` header is sent to every URL, including `skills.sh/api/search`, `skills.sh/api/download/…`, and the `skills.sh/` catalog HTML page. This is a GitHub REST API content-type hint and is meaningless (and potentially confusing) for skills.sh endpoints. If skills.sh ever uses the `Accept` header to gate content — for example returning a JSON error payload instead of the expected HTML/JSON — this could break catalog loading or search silently.
```suggestion
{ headers: { 'User-Agent': 'emdash-skills', Accept: 'application/json, text/html, */*' } },
```
How can I resolve this? If you propose a fix, please make it concise.| throw new Error('Unable to parse skills.sh catalog'); | ||
| } | ||
|
|
||
| return JSON.parse(match[1].replace(/\\"/g, '"')) as SkillsshListSkill[]; |
There was a problem hiding this comment.
replace(/\\"/g, '"') unescape step will corrupt any real backslash-quote in a description. If skills.sh ships a skill with an escaped quote in its name or description, parsing will silently produce corrupted text or throw on JSON.parse.
| return JSON.parse(match[1].replace(/\\"/g, '"')) as SkillsshListSkill[]; | |
| const jsonStr = match[1].replace(/\\"/g, '"'); | |
| return JSON.parse(jsonStr) as SkillsshListSkill[]; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/shared/skills/skillssh.ts
Line: 35
Comment:
The `replace(/\\"/g, '"')` unescape step will corrupt any real backslash-quote in a description. If skills.sh ships a skill with an escaped quote in its name or description, parsing will silently produce corrupted text or throw on `JSON.parse`.
```suggestion
const jsonStr = match[1].replace(/\\"/g, '"');
return JSON.parse(jsonStr) as SkillsshListSkill[];
```
How can I resolve this? If you propose a fix, please make it concise.| return this.mergeInstalledState(this.catalogCache); | ||
| } | ||
|
|
||
| // Try disk cache — only use if its version matches current | ||
| // Always try skills.sh on first load for this process. The disk cache is only an offline fallback. | ||
| try { | ||
| const catalog = await this.fetchRemoteCatalog(); | ||
| this.catalogCache = catalog; | ||
| return this.mergeInstalledState(catalog); | ||
| } catch (error) { | ||
| log.warn('Failed to load remote skills catalog, using cached or bundled catalog', error); | ||
| } |
There was a problem hiding this comment.
getCatalogIndex now unconditionally calls fetchRemoteCatalog before consulting either the disk cache or the bundled catalog. The 15 s req.setTimeout means a slow or unreachable skills.sh would block the Skills view for up to 15 seconds on every app start.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/main/core/skills/SkillsService.ts
Line: 71-81
Comment:
**Blocking cold-start on every process launch**
`getCatalogIndex` now unconditionally calls `fetchRemoteCatalog` before consulting either the disk cache or the bundled catalog. The 15 s `req.setTimeout` means a slow or unreachable skills.sh would block the Skills view for up to 15 seconds on every app start.
How can I resolve this? If you propose a fix, please make it concise.| isDetailLoading, | ||
| showDetailModal, | ||
| installedSkills, |
There was a problem hiding this comment.
During the debounce window (user has typed ≥ 2 chars but the 350 ms has not elapsed), hasActiveSearch is already true so the section title flips to "Results from skills.sh", yet filteredSkills is still derived from the local catalog because debouncedSearchQuery hasn't updated.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/renderer/features/skills/components/SkillsView.tsx
Line: 56-58
Comment:
**"Results from skills.sh" heading while local results are shown**
During the debounce window (user has typed ≥ 2 chars but the 350 ms has not elapsed), `hasActiveSearch` is already `true` so the section title flips to "Results from skills.sh", yet `filteredSkills` is still derived from the local catalog because `debouncedSearchQuery` hasn't updated.
How can I resolve this? If you propose a fix, please make it concise.
|
openai & anthopic skills are still included as well, right? |
| private async fetchSkillsshCatalog(): Promise<CatalogSkill[]> { | ||
| const catalogQueries = ['skill', 'ai', 'agent', 'code', 'docs', 'github', 'react', 'cloud']; | ||
| const skillGroups = await Promise.all( | ||
| catalogQueries.map((query) => this.fetchSkillsshSearch(query, 100)) | ||
| ); | ||
|
|
||
| if (!description) { | ||
| description = `${entry.name.replace(/-/g, ' ')}`; | ||
| } | ||
| return mergeCatalogSkills(...skillGroups); | ||
| } |
There was a problem hiding this comment.
Promise.all rejects the moment any single one of the 8 concurrent search requests fails. A transient 429 from skills.sh, a flaky connection, or a timeout on one term causes every successfully fetched batch to be thrown away, and getCatalogIndex falls back to the bundled catalog with no user-visible indication. The previous catalog fetching code used Promise.allSettled precisely to avoid this cliff edge — any partial result was still useful. With 8 simultaneous requests to the same host the risk of hitting a rate limit on at least one is non-trivial.
| private async fetchSkillsshCatalog(): Promise<CatalogSkill[]> { | |
| const catalogQueries = ['skill', 'ai', 'agent', 'code', 'docs', 'github', 'react', 'cloud']; | |
| const skillGroups = await Promise.all( | |
| catalogQueries.map((query) => this.fetchSkillsshSearch(query, 100)) | |
| ); | |
| if (!description) { | |
| description = `${entry.name.replace(/-/g, ' ')}`; | |
| } | |
| return mergeCatalogSkills(...skillGroups); | |
| } | |
| private async fetchSkillsshCatalog(): Promise<CatalogSkill[]> { | |
| const catalogQueries = ['skill', 'ai', 'agent', 'code', 'docs', 'github', 'react', 'cloud']; | |
| const results = await Promise.allSettled( | |
| catalogQueries.map((query) => this.fetchSkillsshSearch(query, 100)) | |
| ); | |
| const skillGroups = results | |
| .filter((r): r is PromiseFulfilledResult<CatalogSkill[]> => r.status === 'fulfilled') | |
| .map((r) => r.value); | |
| if (skillGroups.length === 0) { | |
| throw new Error('All skills.sh catalog queries failed'); | |
| } | |
| return mergeCatalogSkills(...skillGroups); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/main/core/skills/SkillsService.ts
Line: 580-587
Comment:
`Promise.all` rejects the moment **any single one** of the 8 concurrent search requests fails. A transient 429 from skills.sh, a flaky connection, or a timeout on one term causes every successfully fetched batch to be thrown away, and `getCatalogIndex` falls back to the bundled catalog with no user-visible indication. The previous catalog fetching code used `Promise.allSettled` precisely to avoid this cliff edge — any partial result was still useful. With 8 simultaneous requests to the same host the risk of hitting a rate limit on at least one is non-trivial.
```suggestion
private async fetchSkillsshCatalog(): Promise<CatalogSkill[]> {
const catalogQueries = ['skill', 'ai', 'agent', 'code', 'docs', 'github', 'react', 'cloud'];
const results = await Promise.allSettled(
catalogQueries.map((query) => this.fetchSkillsshSearch(query, 100))
);
const skillGroups = results
.filter((r): r is PromiseFulfilledResult<CatalogSkill[]> => r.status === 'fulfilled')
.map((r) => r.value);
if (skillGroups.length === 0) {
throw new Error('All skills.sh catalog queries failed');
}
return mergeCatalogSkills(...skillGroups);
}
```
How can I resolve this? If you propose a fix, please make it concise.
| for (const skill of searchSkills) { | ||
| this.searchSkillCache.set(skill.id, skill); |
There was a problem hiding this comment.
skillId collides across repos
toSkillsshCatalogSkill sets CatalogSkill.id = entry.skillId, which is only unique within a single repository. The skills.sh API's canonical unique identifier is the full entry.id (e.g., "owner/repo/brainstorming"). When a search returns two skills from different repos with the same skillId, searchSkillCache.set(skill.id, skill) overwrites the first with the second — yet both may appear as separate cards in the UI. When the user clicks Install on the first card, installSkill('review') → findCatalogSkill('review') → returns the wrong repo's cached skill → downloads and installs that repo's files instead.
The fix requires threading entry.id from SkillsshSearchResponse through SkillsshListSkill and toSkillsshCatalogSkill so the skills.sh full path becomes the canonical CatalogSkill.id. Note that installSkill also uses skillId to name the local directory (path.join(SKILLS_ROOT, skillId)), so the directory naming scheme will need adjustment alongside the id change.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/main/core/skills/SkillsService.ts
Line: 173-174
Comment:
**Wrong skill installed when `skillId` collides across repos**
`toSkillsshCatalogSkill` sets `CatalogSkill.id = entry.skillId`, which is only unique within a single repository. The skills.sh API's canonical unique identifier is the full `entry.id` (e.g., `"owner/repo/brainstorming"`). When a search returns two skills from different repos with the same `skillId`, `searchSkillCache.set(skill.id, skill)` overwrites the first with the second — yet both may appear as separate cards in the UI. When the user clicks Install on the first card, `installSkill('review')` → `findCatalogSkill('review')` → returns the wrong repo's cached skill → downloads and installs that repo's files instead.
The fix requires threading `entry.id` from `SkillsshSearchResponse` through `SkillsshListSkill` and `toSkillsshCatalogSkill` so the skills.sh full path becomes the canonical `CatalogSkill.id`. Note that `installSkill` also uses `skillId` to name the local directory (`path.join(SKILLS_ROOT, skillId)`), so the directory naming scheme will need adjustment alongside the id change.
How can I resolve this? If you propose a fix, please make it concise.2 participants
No description provided.