feat(skills): add Open skills folder action to the Skills surface - #1482
Merged
Conversation
The /path response gains a `skills` field: the update-safe home for user-added skills (~/.agents/skills), already scanned by skill discovery's external .agents root. An `ensureSkills` query flag creates it on demand, mirroring ensureConfig, so the renderer never has to resolve or mkdir the path itself. Skill.userSkillsDir() single-sources the path next to builtinRoots so discovery and the API can't drift. control-openapi exposes ensureSkills as a boolean; the SDK is regenerated. The fixed default-path stores in global-sync carry the new required field. Groundwork for a discoverable, update-safe place to add custom skills (#1478).
Custom skills dropped into the install bundle's resources/skills are wiped on every update, since the Windows updater replaces the whole install directory (#1478). There was no in-app way to add a skill and no signposted safe location, so users found the only visible skills folder, which is the volatile one. Add a desktop-only "Open skills folder" button that opens ~/.agents/skills (created on demand via the /path ensureSkills flag), a discoverable and update-safe home that skill discovery already scans. The action is gated on canOpenLocalPath, so the web build hides it. Lay the header out so the title and actions share one vertically-centered row with the description on its own line below, instead of the actions floating against a taller title block. OpenSkillsFolderButton is extracted so the gate and click wiring can be rendered in isolation; covered by a handler unit test, a component render test (both gate directions + the open flow), and an e2e assertion that the web surface hides the button. Localized en/zh. Does not migrate existing resources/skills content or warn on it; tracked as follow-up.
Contributor
📝 WalkthroughWalkthroughAdds an "Open Skills Folder" button to the Skills surface. The server’s ChangesOpen Skills Folder feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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 |
github-actions
Bot
added
app
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Suggested priority: P2 (includes user-path files (packages/app/src/context/global-sync.test.ts, packages/app/src/context/global-sync.tsx, packages/app/src/context/global-sync/bootstrap.test.ts, packages/app/src/context/global-sync/child-store.ts, packages/app/src/i18n/en.ts, packages/app/src/i18n/zh.ts, packages/app/src/pages/skills/open-skills-folder.test.ts, packages/app/src/pages/skills/open-skills-folder.ts, packages/app/src/pages/skills/skills-folder-button.test.ts, packages/app/src/pages/skills/skills-folder-button.tsx, packages/app/src/pages/skills/skills-surface.tsx)).
P1/P0 are reserved for maintainer confirmation. Please relabel manually if this is a release blocker, security issue, data-loss risk, or updater/runtime failure.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/app/src/pages/skills/skills-folder-button.test.ts (1)
69-70: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReplace the fixed sleep with a deterministic completion signal.
Waiting 20 ms here makes this check timing-dependent. Since the mocks already control
openPath, you can await a promise resolved by that mock instead of sleeping.Proposed fix
const pathGetCalls = [] const openPathCalls = [] +let resolveOpened +const opened = new Promise((resolve) => { + resolveOpened = resolve +}) const globalSDK = { client: { path: { @@ const platform = { openPath: (path) => { openPathCalls.push(path) + resolveOpened() return Promise.resolve() }, } @@ assert(button, "desktop host should render the open-folder action") assert(button.textContent === "skills.openFolder", "button should use the i18n label key") button.click() - await new Promise((resolve) => setTimeout(resolve, 20)) + await opened🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/app/src/pages/skills/skills-folder-button.test.ts` around lines 69 - 70, Replace the fixed 20 ms sleep in the skills-folder-button test with an awaited signal from the mocked openPath flow so the assertion is deterministic. Update the test around button.click() to wait for the mock-controlled promise to resolve, using the existing openPath mock setup in skills-folder-button.test.ts rather than relying on setTimeout.packages/opencode/src/server/routes/instance/httpapi/handlers/root.ts (1)
81-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the Effect file-system service for the new skills-directory mkdir.
This new
fs.mkdir(...)call adds another rawfs/promiseswrite inside Effect-based handler code. Please thread the repo’s file-system service into this path instead so the handler stays consistent with the runtime/test abstractions. As per coding guidelines, "PreferFileSystem.FileSysteminstead of rawfs/promisesfor effectful file I/O in Effect services."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/src/server/routes/instance/httpapi/handlers/root.ts` around lines 81 - 82, The new skills-directory creation in the root handler is using a raw fs/promises mkdir call instead of the Effect file-system abstraction. Update the logic in the root route handler around ensureSkills to use the repo’s FileSystem.FileSystem service for mkdir so it stays consistent with the rest of the Effect-based code and test/runtime abstractions.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/app/src/pages/skills/skills-surface.tsx`:
- Around line 133-141: The search textbox in skills-surface lacks an accessible
name because the surrounding label is icon-only; update the input in
skills-surface to include an explicit aria-label or add hidden label text so
assistive technologies can announce it properly. Use the existing search input
markup and keep the current query/setQuery behavior unchanged while adding the
accessible name near the input element.
---
Nitpick comments:
In `@packages/app/src/pages/skills/skills-folder-button.test.ts`:
- Around line 69-70: Replace the fixed 20 ms sleep in the skills-folder-button
test with an awaited signal from the mocked openPath flow so the assertion is
deterministic. Update the test around button.click() to wait for the
mock-controlled promise to resolve, using the existing openPath mock setup in
skills-folder-button.test.ts rather than relying on setTimeout.
In `@packages/opencode/src/server/routes/instance/httpapi/handlers/root.ts`:
- Around line 81-82: The new skills-directory creation in the root handler is
using a raw fs/promises mkdir call instead of the Effect file-system
abstraction. Update the logic in the root route handler around ensureSkills to
use the repo’s FileSystem.FileSystem service for mkdir so it stays consistent
with the rest of the Effect-based code and test/runtime abstractions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 78feebb4-dbf9-4c77-a27a-cc2374fede61
⛔ Files ignored due to path filters (2)
packages/sdk/js/src/v2/gen/sdk.gen.tsis excluded by!**/gen/**packages/sdk/js/src/v2/gen/types.gen.tsis excluded by!**/gen/**
📒 Files selected for processing (17)
packages/app/e2e/skills/skills-panel.spec.tspackages/app/src/context/global-sync.test.tspackages/app/src/context/global-sync.tsxpackages/app/src/context/global-sync/bootstrap.test.tspackages/app/src/context/global-sync/child-store.tspackages/app/src/i18n/en.tspackages/app/src/i18n/zh.tspackages/app/src/pages/skills/open-skills-folder.test.tspackages/app/src/pages/skills/open-skills-folder.tspackages/app/src/pages/skills/skills-folder-button.test.tspackages/app/src/pages/skills/skills-folder-button.tsxpackages/app/src/pages/skills/skills-surface.tsxpackages/opencode/src/server/control-openapi.tspackages/opencode/src/server/routes/instance/httpapi/groups/root.tspackages/opencode/src/server/routes/instance/httpapi/handlers/root.tspackages/opencode/src/skill/index.tspackages/sdk/openapi.json
The search box sat inside an icon-only label, so assistive tech had no usable name for the textbox. Add an aria-label reusing the placeholder copy. Flagged in review.
Replace the fixed 20ms sleep after the click with a promise the openPath mock resolves, so the assertion no longer races a timer. Flagged in review.
getPaths used raw fs.mkdir inside the Effect handler. Switch both the config and skills mkdirs to AppFileSystem.Service.makeDirectory (orDie to preserve the existing fail-fast behavior) and provide AppFileSystem to the production router runtime, matching the house FS abstraction. Flagged in review.
Owner
Author
|
Addressed all three CodeRabbit findings:
Verification: |
The as-Path fixtures enumerated every path field except the newly required skills, compiling only because the cast suppressed the missing-property check. Add skills so the fixtures' typed shape matches the real PathInfo.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/app/src/context/global-sync/bootstrap.test.ts (1)
140-167: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a non-empty sentinel for
skillsin at least one bootstrap fixture.Every new
skillsfield here is"", so these tests still pass if bootstrap forgets to reconcilepath.skillsat all. Give one mocked path/expected snapshot a distinct value and assert it survives the merge path.Example test hardening
- path: { get: async () => ({ data: { state: "", config: "", skills: "", worktree: "", directory, home: "" } as Path }) }, + path: { get: async () => ({ data: { state: "", config: "", skills: "/tmp/test-skills", worktree: "", directory, home: "" } as Path }) }, - path: { state: "", config: "", skills: "", worktree: "", directory: "", home: "" } as Path, + path: { state: "", config: "", skills: "/tmp/test-skills", worktree: "", directory: "", home: "" } as Path,Also applies to: 201-228, 271-325, 358-380, 431-473, 505-526, 562-583, 612-638, 672-698, 746-773
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/app/src/context/global-sync/bootstrap.test.ts` around lines 140 - 167, The bootstrapDirectory tests are not asserting that path.skills is actually preserved because both the mocked input and expected snapshot use an empty string. Update at least one bootstrapDirectory fixture to give Path.skills a non-empty sentinel value and assert that the merged global.path.skills still contains that value after bootstrap, so the test fails if skills reconciliation is skipped.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/app/src/context/global-sync/bootstrap.test.ts`:
- Around line 140-167: The bootstrapDirectory tests are not asserting that
path.skills is actually preserved because both the mocked input and expected
snapshot use an empty string. Update at least one bootstrapDirectory fixture to
give Path.skills a non-empty sentinel value and assert that the merged
global.path.skills still contains that value after bootstrap, so the test fails
if skills reconciliation is skipped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6141abdf-30f7-4b7b-aab4-a5f1689041fc
📒 Files selected for processing (5)
packages/app/src/context/global-sync/bootstrap.test.tspackages/app/src/pages/skills/skills-folder-button.test.tspackages/app/src/pages/skills/skills-surface.tsxpackages/opencode/src/server/production-httpapi.tspackages/opencode/src/server/routes/instance/httpapi/handlers/root.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/opencode/src/server/routes/instance/httpapi/handlers/root.ts
- packages/app/src/pages/skills/skills-folder-button.test.ts
- packages/app/src/pages/skills/skills-surface.tsx
Astro-Han
added a commit
that referenced
this pull request
Jun 29, 2026
Bump desktop release metadata to 2026.6.13 to ship two user-facing fixes already merged to dev. Goal: - Cut a stable release carrying the sidebar drag fix (#1481) and the Skills folder entry (#1482). Change boundary: - packages/desktop-electron/package.json: 2026.6.12 -> 2026.6.13. - bun.lock: sync the matching workspace package version (only the version line changed). Verification: - bun install --frozen-lockfile + --lockfile-only in the release worktree; git diff is exactly the two version lines; git diff --check passed. - PR #1483 CI green (one flaky e2e connecting-indicator test re-run to pass; unrelated to a version bump). Residual risk: none for the bump itself. Release build/publish/mirror/verification follow .github/RELEASE_CHECKLIST.md.
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
1 participant
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
Adds a desktop-only Open skills folder button to the Skills surface that opens
~/.agents/skills, the update-safe home for user-added skills, creating it on demand./pathAPI gains askillsfield plus anensureSkillsquery flag (mirrorsensureConfig);Skill.userSkillsDir()single-sources the path next tobuiltinRoots. SDK regenerated.OpenSkillsFolderButton(extracted for isolated testing) gated oncanOpenLocalPath, so the web build hides it.Why
Custom skills dropped into the install bundle's
resources/skillsare wiped on every update, because the Windows NSIS updater replaces the whole install directory (RMDir /r $INSTDIR). There was no in-app way to add a skill and no signposted safe location, so users found the only visible skills folder, which happens to be the volatile one, and lost their work silently and irreversibly on update.~/.agents/skillslives under the home dir (outside the install bundle) and is already scanned by skill discovery, so it survives updates. This change makes that location discoverable from the app.Related Issue
#1478
Human Review Status
Pending
Review Focus
/pathcontract change and the regenerated SDK diff (skillsis a required field now; the fixed default-path stores were updated to carry it).Risk Notes
resources/skillscontent or warn when a custom skill is still found there; both are tracked as follow-ups. Residual risk: a user who keeps usingresources/skillsdirectly still loses content on update, but with a signposted button the path of least resistance now leads away from the trap.platform.openPath); the path resolves to~/.agents/skillson both macOS and Windows. No packaging/updater/signing code is touched.How To Verify
Visual: walked the Skills route in
bun run dev:desktop(zh locale) confirming the button renders and the header reads calmly;bun run snap skills-surfaceconfirms the web header layout and that the gate hides the button.Screenshots or Recordings
Checklist
bug,enhancement,task,documentation. Type labels are author-added; the labeler bot does NOT assign them. Add the label in the GitHub UI, then tick this.app,ui,platform,harness,ci. The labeler bot assignedapp(packages/app),ui(**/*.tsx per labeler.yml), andharness(packages/opencode + packages/sdk); all correct for the changed paths.P0,P1,P2,P3. The triage bot assignedP2; confirmed (high-severity but low-frequency, and this is the discoverability fix rather than the wipe itself).Pending,Approved by @<reviewer>, orNot required: <reason>(default isPending; "not required" is restricted to bot-authored low-risk PRs).dev, and my PR title and commit messages use Conventional Commits in English.Summary by CodeRabbit
New Features
Bug Fixes