feat(tui): install skills via typed query in the /skills list#1005
feat(tui): install skills via typed query in the /skills list#1005sahrizvi wants to merge 2 commits into
Conversation
The `/skills` list didn't offer a discoverable way to install from a
GitHub repo / URL / absolute path — the only entry point was `ctrl+i`,
whose wire byte (0x09) collides with Tab on default terminals, so many
users couldn't trigger it.
Surface a synthetic "Install <query>" row at the top of the list when the
filter matches an installable shape, so pressing Enter opens the install
dialog prefilled with the typed text. Backed by a shared
`classifyInstallSource` classifier used by both the list preview and
`installSkillDirect` — an earlier `q.includes("/")` check drifted from
the installer and offered the row for skill-search queries like
`dbt/snowflake` that the installer then rejected as "Path not found".
- Add module-scope `classifyInstallSource` (recognises github URLs, clean
`owner/repo` shorthand, POSIX absolute paths, Windows drive-letter
paths — rejects short strings, sub-paths, `~`, relatives).
- Route the sentinel through `onSelect` to `showInstall`, forward the
captured filter text as `initialValue` on `DialogSkillInstall` and
`DialogSkillCreate`.
- Filter the sentinel out of `onMove` so highlighting the synthetic row
doesn't set `currentSkill` to the sentinel string (would trip
`ctrl+a`'s action picker into a degenerate lookup miss).
- Harden `showActions` to bail on the sentinel or a lookup miss as
belt-and-braces.
- Build options via a spread instead of `Array.prototype.unshift` so the
memo stays pure (Solid dev-mode double-eval would otherwise
double-prepend).
- Slim the fork-feature-guards test to a minimal presence check
(sentinel, classifier symbol, sentinel-route in `onSelect`, prefill
plumbing) — behavioural coverage of the classifier moves to a new
`test/altimate/skill-install-classifier.test.ts` unit suite.
Verified: `bun run typecheck` clean; new classifier suite 10/10;
fork-feature-guards 18/18; `test/session` 731/731.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
📝 WalkthroughWalkthroughAdds shared install-source classification, synthetic install actions in the skill list, filter-prefilled create/install dialogs, and guards preventing action pickers from handling synthetic rows. Tests cover source normalization, accepted path and repository forms, and UI wiring. ChangesSkill installation flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant DialogSkillList
participant classifyInstallSource
participant DialogSkillInstall
participant installSkillDirect
User->>DialogSkillList: Enter install source filter
DialogSkillList->>classifyInstallSource: Classify filter
classifyInstallSource-->>DialogSkillList: Return source kind
DialogSkillList->>DialogSkillInstall: Select synthetic install row
DialogSkillInstall->>installSkillDirect: Submit initial filter value
installSkillDirect-->>DialogSkillInstall: Install via Git or absolute path
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
dev-punia-altimate
left a comment
dev-punia-altimate
left a comment
There was a problem hiding this comment.
🤖 Code Review — OpenCodeReview (Gemini) — 2 finding(s)
- 2 anchored to a line (posted inline when the comment stream is on)
- 0 without a line anchor
All findings (full text)
1. packages/opencode/src/plugin/tui/altimate/skill-ops.tsx (L592-L596)
[🟠 MEDIUM] Performance Issue: Unnecessary reactivity overhead
Inside this createMemo, the signal filter() is read. This means the memo depends on filter() and will re-evaluate on every keystroke. During this re-evaluation, the list.map iterates over all skills, executing detectToolReferences (which parses content using regex). This can cause noticeable UI lag during search when there are many skills.
Suggestion:
Separate the reactivity into two memos: one for computing the base list of skills() (which only updates when the underlying skills data changes), and another for handling the dynamic "Install" option that reacts to filter() changes.
const baseOptions = createMemo<TuiDialogSelectOption<string>[]>(() => {
const list = skills() ?? []
const maxWidth = Math.max(0, ...list.map((s) => s.name.length))
return list.map((skill) => {
const tools = detectToolReferences(skill.content)
const category = SKILL_CATEGORIES[skill.name] ?? "Other"
const desc = skill.description?.replace(/\s+/g, " ").trim()
return {
title: skill.name.padEnd(maxWidth),
description: desc,
footer: tools.length > 0 ? tools.map((t) => `\u25b6 ${t}`).join(" ") : undefined,
value: skill.name,
category,
} satisfies TuiDialogSelectOption<string>
})
})
const options = createMemo<TuiDialogSelectOption<string>[]>(() => {
const items = baseOptions()
const q = filter().trim()
if (classifyInstallSource(q) !== null) {
const installOption: TuiDialogSelectOption<string> = {
title: `Install ${q}`,
description: "Press Enter to install from this GitHub repo, URL, or path",
footer: undefined,
value: INSTALL_ACTION_VALUE,
category: "Install",
}
return [installOption, ...items]
}
return items
})2. packages/opencode/src/plugin/tui/altimate/skill-ops.tsx (L177-L180)
[🟠 MEDIUM] Bug: Inconsistent string normalization
The classifyInstallSource function accepts leading/trailing whitespaces and strips .git suffixes (e.g., " owner/repo "), returning a valid kind. However, installSkillDirect relies on the unmodified normalized string.
If normalized has spaces or an existing .git suffix (e.g., " owner/repo " or "owner/repo.git"), it passes the if check but generates a malformed URL like https://github.com/ owner/repo .git or https://github.com/owner/repo.git.git.
Suggestion:
Ensure the string used to construct the URL matches the cleaned format recognized by the classifier.
const kind = classifyInstallSource(normalized)
if (kind === "github-url" || kind === "owner-repo") {
const cleaned = normalized.trim().replace(/\.+$/, "").replace(/\.git$/, "")
const url = cleaned.startsWith("http") ? cleaned : `https://github.com/${cleaned}.git`
const label = url.replace(/https?:\/\/github\.com\//, "").replace(/\.git$/, "")| const options = createMemo<TuiDialogSelectOption<string>[]>(() => { | ||
| const list = skills() ?? [] | ||
| const maxWidth = Math.max(0, ...list.map((s) => s.name.length)) | ||
| return list.map((skill) => { | ||
| const items = list.map((skill) => { | ||
| const tools = detectToolReferences(skill.content) |
There was a problem hiding this comment.
[🟠 MEDIUM] Performance Issue: Unnecessary reactivity overhead
Inside this createMemo, the signal filter() is read. This means the memo depends on filter() and will re-evaluate on every keystroke. During this re-evaluation, the list.map iterates over all skills, executing detectToolReferences (which parses content using regex). This can cause noticeable UI lag during search when there are many skills.
Suggestion:
Separate the reactivity into two memos: one for computing the base list of skills() (which only updates when the underlying skills data changes), and another for handling the dynamic "Install" option that reacts to filter() changes.
const baseOptions = createMemo<TuiDialogSelectOption<string>[]>(() => {
const list = skills() ?? []
const maxWidth = Math.max(0, ...list.map((s) => s.name.length))
return list.map((skill) => {
const tools = detectToolReferences(skill.content)
const category = SKILL_CATEGORIES[skill.name] ?? "Other"
const desc = skill.description?.replace(/\s+/g, " ").trim()
return {
title: skill.name.padEnd(maxWidth),
description: desc,
footer: tools.length > 0 ? tools.map((t) => `\u25b6 ${t}`).join(" ") : undefined,
value: skill.name,
category,
} satisfies TuiDialogSelectOption<string>
})
})
const options = createMemo<TuiDialogSelectOption<string>[]>(() => {
const items = baseOptions()
const q = filter().trim()
if (classifyInstallSource(q) !== null) {
const installOption: TuiDialogSelectOption<string> = {
title: `Install ${q}`,
description: "Press Enter to install from this GitHub repo, URL, or path",
footer: undefined,
value: INSTALL_ACTION_VALUE,
category: "Install",
}
return [installOption, ...items]
}
return items
})| const kind = classifyInstallSource(normalized) | ||
| if (kind === "github-url" || kind === "owner-repo") { | ||
| const url = normalized.startsWith("http") ? normalized : `https://github.com/${normalized}.git` | ||
| const label = url.replace(/https?:\/\/github\.com\//, "").replace(/\.git$/, "") |
There was a problem hiding this comment.
[🟠 MEDIUM] Bug: Inconsistent string normalization
The classifyInstallSource function accepts leading/trailing whitespaces and strips .git suffixes (e.g., " owner/repo "), returning a valid kind. However, installSkillDirect relies on the unmodified normalized string.
If normalized has spaces or an existing .git suffix (e.g., " owner/repo " or "owner/repo.git"), it passes the if check but generates a malformed URL like https://github.com/ owner/repo .git or https://github.com/owner/repo.git.git.
Suggestion:
Ensure the string used to construct the URL matches the cleaned format recognized by the classifier.
const kind = classifyInstallSource(normalized)
if (kind === "github-url" || kind === "owner-repo") {
const cleaned = normalized.trim().replace(/\.+$/, "").replace(/\.git$/, "")
const url = cleaned.startsWith("http") ? cleaned : `https://github.com/${cleaned}.git`
const label = url.replace(/https?:\/\/github\.com\//, "").replace(/\.git$/, "")Two medium findings from the automated review pass on this PR: 1. `installSkillDirect` used the raw `normalized` input when building a clone URL, but `classifyInstallSource` tolerates a trailing `.git` suffix / whitespace / trailing dots. An input like `owner/repo.git` passed the classifier's `owner-repo` check and then produced `https://github.com/owner/repo.git.git`. Extracted the trim/strip logic into `normalizeInstallSource` and use it in both the classifier and the URL builder so the two can't disagree on shape. 2. The `options` `createMemo` read the `filter()` signal, so every keystroke re-executed the full `list.map` — including `detectToolReferences` (regex parse per skill). Split into a `baseOptions` memo that depends only on `skills()` and a derived `options` memo that composes the synthetic Install row on top. Now a filter change only re-checks `classifyInstallSource(q)` and prepends one item; `detectToolReferences` only re-runs when the underlying skills list changes. Also adds a `normalizeInstallSource` unit-test suite (4 cases pinning the `.git` / trailing-dot / whitespace behaviour) alongside the existing classifier tests. Verified: `bun run typecheck` clean; classifier suite 14/14 (54 expects); fork-feature-guards 18/18 (61 expects).
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
dev-punia-altimate
left a comment
dev-punia-altimate
left a comment
There was a problem hiding this comment.
🤖 Code Review — OpenCodeReview (Gemini) — 1 finding(s)
- 1 anchored to a line (posted inline when the comment stream is on)
- 0 without a line anchor
All findings (full text)
1. packages/opencode/src/plugin/tui/altimate/skill-ops.tsx (L192-L193)
[🟠 MEDIUM] There are two potential issues here:
- Stripping
.gitfrom HTTP URLs: Usingcleanedfor explicit HTTP URLs inadvertently removes the.gitsuffix if the user explicitly provided one (e.g.,https://git.example.com/repo.gitbecomeshttps://git.example.com/repo). While GitHub tolerates this, many self-hosted Git servers require the suffix and will fail to clone. - Conflating logic: Relying on
cleaned.startsWith("http")can incorrectly match anowner-reposhorthand if the owner happens to behttporhttps(e.g.,http/my-repo), resulting in an invalid URL instead of correctly cloninghttps://github.com/http/my-repo.git.
You can fix both by leveraging the already computed kind variable to avoid false positives and preserving the original suffix for HTTP URLs while still cleaning up any trailing dots.
Suggested change:
const cleaned = normalizeInstallSource(normalized)
const url = kind === "github-url"
? normalized.trim().replace(/\.+$/, "")
: `https://github.com/${cleaned}.git`
| const cleaned = normalizeInstallSource(normalized) | ||
| const url = cleaned.startsWith("http") ? cleaned : `https://github.com/${cleaned}.git` |
There was a problem hiding this comment.
[🟠 MEDIUM] There are two potential issues here:
- Stripping
.gitfrom HTTP URLs: Usingcleanedfor explicit HTTP URLs inadvertently removes the.gitsuffix if the user explicitly provided one (e.g.,https://git.example.com/repo.gitbecomeshttps://git.example.com/repo). While GitHub tolerates this, many self-hosted Git servers require the suffix and will fail to clone. - Conflating logic: Relying on
cleaned.startsWith("http")can incorrectly match anowner-reposhorthand if the owner happens to behttporhttps(e.g.,http/my-repo), resulting in an invalid URL instead of correctly cloninghttps://github.com/http/my-repo.git.
You can fix both by leveraging the already computed kind variable to avoid false positives and preserving the original suffix for HTTP URLs while still cleaning up any trailing dots.
Suggested change:
| const cleaned = normalizeInstallSource(normalized) | |
| const url = cleaned.startsWith("http") ? cleaned : `https://github.com/${cleaned}.git` | |
| const cleaned = normalizeInstallSource(normalized) | |
| const url = kind === "github-url" | |
| ? normalized.trim().replace(/\.+$/, "") | |
| : `https://github.com/${cleaned}.git` |
🤖 Code Review — OpenCodeReview (Gemini) — No Issues FoundNo supported files changed. |
|
Note AI-assisted code review. Produced by Claude (Opus 4.8) running an adversarial multi-agent Method: a recon pass first pinned the Verdict: mergeable with fixes. The core design is sound — one exported Findingsmedium —
|
3 participants
Issue for this PR
Closes #
No linked issue — internal quality fix on the fork's inline
/skillsdialog.Type of change
What does this PR do?
Gives the
/skillslist a discoverable Enter-driven install path.When the filter text matches an installable shape (github URL, clean
owner/repo, or an absolute path), a syntheticInstall <query>row is prepended at the top of the list. Enter opens the install dialog with the typed text prefilled. Symmetric wiring on the create dialog.Why: the pre-existing
ctrl+ishortcut can't be relied on — its wire byte (0x09) collides withTabon default terminals, so users trying to install from a URL/repo/path they typed into the search box saw the list filter to zero results and had no obvious next action.How: backing the "when to offer the row" check with the same classifier the installer uses (
classifyInstallSource) kills a class of bugs where the list would advertise an install of things the installer would then reject — e.g. skill-search queries likedbt/snowflakeorowner/repo/subpath.Files:
packages/opencode/src/plugin/tui/altimate/skill-ops.tsxclassifyInstallSource(source)— recognisesgithub-url,owner-repo,absolute-path; returnsnullfor short strings, sub-paths, relative paths, and~-paths.normalizeInstallSource(source)— shared trim/strip helper (also used byinstallSkillDirectwhen building the clone URL, soowner/repo.gitcan't produceowner/repo.git.git).INSTALL_ACTION_VALUEsentinel — namespaced so it can't collide with real skill names.DialogSkillList— synthetic Install row via spread (pure memo);onSelectroutes sentinel toshowInstall;onMovefilters sentinel;showActionsbails on sentinel/lookup-miss.optionsmemo split intobaseOptions(deps onskills()only) + composedoptions(deps onfilter()) — avoids re-runningdetectToolReferencesregex parse per skill on every keystroke.packages/opencode/test/altimate/skill-install-classifier.test.ts(new) — 14 unit tests pinning classifier + normalizer edge cases (owner/repo, http(s), POSIX absolute, Windows drive-letter, short-string rejection, multi-slash rejection,~/relative rejection,.git/ trailing-dot / whitespace stripping).packages/opencode/test/upstream/fork-feature-guards.test.ts— slimmed the merge-drop guard to a minimal presence check (classifier symbol, sentinel, sentinel-route, prefill plumbing on both sub-dialogs).How did you verify your code works?
Local validation (all green):
bun run typecheck— clean across the whole workspacebun test test/altimate/skill-install-classifier.test.ts— 14 pass / 0 fail / 54 expectsbun test test/upstream/fork-feature-guards.test.ts— 18 pass / 0 fail / 61 expectsbun test test/session— 731 pass / 0 fail / 1636 expectsManual smoke plan on a built binary:
/skills→ typehttps://github.com/anthropics/skills.git→ Enter opens install dialog with URL prefilled./skills→ typeowner/repo→ Enter opens install dialog withowner/repoprefilled → clone URL ishttps://github.com/owner/repo.git(single.git, no double-suffix)./skills→ typeowner/repo.git→ same clone URL (double-suffix bug regression check)./skills→ typedbt-develop(real search term) → no Install row surfaces; list filters skills normally.ctrl+adoes nothing (sentinel bail-out).Screenshots / recordings
Terminal-only UI change — no screenshot supplied. Behaviour is fully covered by the classifier unit tests and the fork-feature-guards presence checks.
Checklist
Follow-ups intentionally deferred (from prior review)
Search skills, or type a repo/URL…copy shows even when the filter isn't installable. Low priority.DialogPrompt's cursor lands at end-of-text; fine for appending/path, mildly awkward for editing a pasted URL.Review response
Addressed both findings from OpenCodeReview (Gemini) in commit
b8e7e64c3:skill-ops.tsx:177-180) —owner/repo.gitproducedowner/repo.git.git. ExtractednormalizeInstallSourceand use it in both classifier and URL builder. Regression covered by new normalizer test suite.skill-ops.tsx:592-596) —filter()reactivity madedetectToolReferencesre-run per keystroke. Split intobaseOptions(skills-only) + composedoptions(filter-aware).Summary by CodeRabbit
New Features
Tests