Skip to content

feat: add skills.sh search backend#2189

Open
janburzinski wants to merge 8 commits into
mainfrom
emdash/skillssh-support-q7qfm
Open

feat: add skills.sh search backend#2189
janburzinski wants to merge 8 commits into
mainfrom
emdash/skillssh-support-q7qfm

Conversation

@janburzinski
Copy link
Copy Markdown
Collaborator

summary

  • adds support for skills.sh in skills
  • Parse multiline skill descriptions so YAML block markers do not appear in the skills catalog. (@mezotv )

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 22, 2026

Greptile Summary

This PR integrates skills.sh as a live search backend alongside the existing OpenAI/Anthropic catalog, and fixes multiline YAML block scalar values in skill frontmatter so markers like | or > no longer surface as visible text.

  • skills.sh search: SkillsService.searchSkillSh fetches the skills.sh API, normalises paths, enforces path-traversal guards on install, and caches results (5 min TTL). The renderer side adds a skillShSearchSkills section below catalog results when a query is active.
  • YAML block scalars: parseFrontmatter now handles literal (|) and folded (>) block scalars with chomping modifiers; a new validation.test.ts file covers the main cases.
  • UI polish: The skill detail modal gains a shimmer loading state and a sourceMeta lookup for source attribution; the compact markdown renderer adds table, blockquote, and HR component renderers.

Confidence Score: 4/5

Safe to merge with minor fixes — the core install path is guarded against path traversal and the YAML parser is well-tested; the main rough edges are a dead Set check in the deduplication logic and a mismatched fallback icon.

The skills.sh install path correctly validates names and checks bounds; the YAML block scalar parser handles edge cases and is covered by tests. Two issues in useSkills.ts warrant attention: the duplicate installedIds/installedNames Sets (the first check never fires for skillssh IDs, so deduplication is thinner than intended) and the absence of search debouncing. The Vercel icon fallback for skills.sh is an incorrect brand mapping that would be visible to users on avatar load failure.

useSkills.ts (duplicate Set logic and missing debounce) and skillIcons.ts (wrong fallback icon for skillssh source)

Important Files Changed

Filename Overview
src/main/core/skills/SkillsService.ts Adds skills.sh search backend with caching, path sanitization, and path-traversal guards for install; logic is sound but skillssh install has no fallback to generated stub on fetch failure.
src/renderer/features/skills/components/useSkills.ts Two issues: installedIds and installedNames are identical Sets (copy-paste bug making the first check dead), and no debounce on skills.sh search query fires a request per keystroke.
src/renderer/features/skills/components/skillIcons.ts Maps skillssh fallback icon to 'vercel', causing Vercel branding to appear when a skills.sh GitHub avatar fails to load.
src/shared/skills/validation.ts Adds literal and folded YAML block scalar parsing to parseFrontmatter; logic and edge-case handling (empty blocks, chomping indicators) are correct and well-tested.
src/renderer/features/skills/components/SkillDetailModal.tsx Adds loading shimmer, skills.sh source metadata, and filters out YAML path bodies; straightforward UI additions with no logic issues.
src/shared/skills/types.ts Adds skillssh to the source union and new optional fields (sourceRef, catalogSkillId, skillShPath, installs) — clean extension of the type.
src/main/core/skills/controller.ts Adds searchSkillSh RPC handler with consistent error wrapping — no issues.
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 3
src/renderer/features/skills/components/useSkills.ts:179-186
`installedIds` and `installedNames` are constructed from the exact same expression — both map installed skills to `s.id`. Because skills.sh search results carry compound IDs like `skillssh:owner/repo/linear`, `installedIds.has(skill.id)` never matches, making that first check dead. The effective deduplication already comes from the second check (`installedNames.has(skill.catalogSkillId ?? skill.id)`). The second `new Set(...)` can be removed, and the variable should be renamed to reflect what is actually being compared.

```suggestion
  const skillShSearchSkills = useMemo(() => {
    const installedNames = new Set(catalog?.skills.filter((s) => s.installed).map((s) => s.id));
    return skillShSkills.filter(
      (skill) => !installedNames.has(skill.catalogSkillId ?? skill.id)
    );
  }, [catalog, skillShSkills]);
```

### Issue 2 of 3
src/renderer/features/skills/components/skillIcons.ts:105-109
The `skillssh` source is mapped to `'vercel'`, so any skills.sh skill whose GitHub avatar fails to load will fall back to the Vercel logo — incorrect branding. Since skills.sh skills all have `iconUrl` set to the owner's GitHub avatar, the fallback is only triggered on load failure; returning `undefined` here would render the letter-avatar instead, which is a more neutral fallback.

```suggestion
const sourceIcons: Record<string, string> = {
  openai: 'openai',
  anthropic: 'anthropic',
};
```

### Issue 3 of 3
src/renderer/features/skills/components/useSkills.ts:125-135
**Missing debounce on skills.sh search**

`skillShQuery` updates synchronously with every keystroke — typing a six-character query fires five separate React Query requests (each unique key after 2 chars). React Query deduplicates in-flight requests for the same key, but not across different ones. Adding a debounced variant of `searchQuery` (e.g. 300 ms) as the query key source would reduce API calls to skills.sh substantially.

Reviews (1): Last reviewed commit: "fix: harden Skills.SH installs" | Re-trigger Greptile

Comment thread src/renderer/features/skills/components/useSkills.ts
Comment thread src/renderer/features/skills/components/skillIcons.ts
Comment thread src/renderer/features/skills/components/useSkills.ts Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant