-
Notifications
You must be signed in to change notification settings - Fork 0
feat: add skills section to sidebar with agent-based filtering #17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: howcode
Are you sure you want to change the base?
Conversation
… correct directory scoping
Summary of ChangesHello @IgorWarzocha, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the application's user experience by integrating a new "Skills" section into the sidebar across both web and TUI interfaces. This feature allows users to easily discover and access skills relevant to their current context, driven by an intelligent agent-based filtering system. The backend now includes a dedicated "/skill" API for managing and serving these filtered skills, supported by a crucial fix to the permission evaluation system that ensures accurate and specific access control. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new "Skills" section to both the web and TUI sidebars, which displays skills filtered by the current agent's permissions. This is supported by a new /skill API endpoint and an important fix to the permission evaluation logic. The changes are well-implemented across the frontend and backend. I have a couple of suggestions to improve efficiency and code readability.
| function setSelectedAgent(directory: string, agent: string) { | ||
| setGlobalStore("selected_agent", directory, agent) | ||
| loadSkills(directory, undefined, agent) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The loadSkills call within setSelectedAgent is redundant. The only caller of this function, in packages/app/src/context/local.tsx, immediately calls globalSync.skill.load again with more specific parameters (including sessionID). This results in two network requests to load skills, where the second one's result overwrites the first. Removing this call will improve efficiency and make the function's behavior align better with its name (i.e., only setting the agent, without the side effect of loading data).
function setSelectedAgent(directory: string, agent: string) {
setGlobalStore("selected_agent", directory, agent)
}
| const agentName = | ||
| (agentParam && agentParam !== "undefined" && agentParam !== "" ? agentParam : undefined) ?? | ||
| (await (async () => { | ||
| if (!sessionID) return undefined | ||
| const messages = await Session.messages({ sessionID, limit: 10 }) | ||
| return messages.findLast((m) => m.info.role === "user")?.info.agent | ||
| })()) ?? | ||
| (await Agent.defaultAgent()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic for determining agentName is a bit complex and hard to read due to the chained nullish coalescing operators (??) and an immediately-invoked async function expression (IIAFE). Refactoring this into a more straightforward async IIAFE would greatly improve readability and maintainability.
| const agentName = | |
| (agentParam && agentParam !== "undefined" && agentParam !== "" ? agentParam : undefined) ?? | |
| (await (async () => { | |
| if (!sessionID) return undefined | |
| const messages = await Session.messages({ sessionID, limit: 10 }) | |
| return messages.findLast((m) => m.info.role === "user")?.info.agent | |
| })()) ?? | |
| (await Agent.defaultAgent()) | |
| const agentName = await (async () => { | |
| if (agentParam && agentParam !== "undefined" && agentParam !== "") { | |
| return agentParam; | |
| } | |
| if (sessionID) { | |
| const messages = await Session.messages({ sessionID, limit: 10 }); | |
| const agentFromSession = messages.findLast((m) => m.info.role === "user")?.info.agent; | |
| if (agentFromSession) { | |
| return agentFromSession; | |
| } | |
| } | |
| return Agent.defaultAgent(); | |
| })(); |
Remove redundant loadSkills call and simplify agentName resolution logic.
|
/oc verify this is all sound |
|
Based on my review of the code, the implementation is sound. Here's my analysis: ✅ Correct ImplementationWeb UI (packages/app/src/pages/layout.tsx:970-998)
TUI UI (packages/opencode/src/cli/cmd/tui/routes/session/sidebar.tsx:161-185)
TUI Skill Loading (packages/opencode/src/cli/cmd/tui/routes/session/index.tsx:250-257)
Skill Endpoint (packages/opencode/src/server/skill.ts)
Permission Fix (packages/opencode/src/permission/next.ts:220-242)
Web Integration (packages/app/src/context/local.tsx:77-83)
Note on Reviewer CommentsThe reviewer mentioned a redundant function setSelectedAgent(directory: string, agent: string) {
setGlobalStore("selected_agent", directory, agent)
}There is no redundant call - the reviewer was likely looking at an earlier version or there's been a fix. The reviewer also suggested refactoring the IIAFE in The PR is ready to merge. |

Summary
Adds a foldable "Skills" section to the sidebar (both web and TUI) that displays available skills filtered by the currently selected agent's permissions.
Changes
packages/app/src/pages/layout.tsxpackages/opencode/src/cli/cmd/tui/routes/session/sidebar.tsx/skillendpoint inpackages/opencode/src/server/skill.tswith permission-based filteringPermissionNext.evaluate()for correct skill filteringTechnical Details
PermissionNext.evaluate()globalSync.skill.setSelectedAgent()Files Modified
packages/app/src/pages/layout.tsx- Skills section UIpackages/app/src/context/global-sync.tsx- Skill loading APIpackages/app/src/context/local.tsx- Agent change detectionpackages/opencode/src/cli/cmd/tui/routes/session/sidebar.tsx- TUI skills sectionpackages/opencode/src/server/skill.ts- New skill endpointpackages/opencode/src/permission/next.ts- Specificity fixpackages/opencode/src/cli/cmd/tui/context/sync.tsx- Skill sync supportpackages/opencode/src/cli/cmd/tui/routes/session/index.tsx- Agent-based skill loading