feat(site/src/pages/AgentsPage): add personal skills slash menu#25386
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c70f11b799
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
This stack of pull requests is managed by Graphite. Learn more about stacking. |
3ee05f6 to
a6f2ec8
Compare
c70f11b to
288095a
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
Well-structured feature: the slash-trigger detection, menu rendering, and filter logic are cleanly separated, and the story coverage is unusually thorough for a chat input plugin. The double-selection guard via hasSelectedRef, the replacement safety checks in replaceActiveSkillsTrigger, and the lazy enabled: skillsMenuOpen query gating are all solid.
1 P1, 2 P2, 7 P3, 6 Nit, 2 Note posted inline.
The P1 (cmdk double-highlight after arrow navigation) and two P2s (Enter submitting during loading, LexicalTypeaheadMenuPlugin reimplementation) need attention before merge. The cmdk issue is both visual and accessibility: aria-selected diverges from the actual keyboard selection.
Process note: commit 0c75e9cb6 ("fix personal skills slash menu selection") bundles two unrelated changes (drop description from trigger text + add hasSelectedRef guard). A commit per concern would help bisect.
"The menu presents itself as a modal interaction (blocking arrows, showing a popover), but Enter punches through the floor." Hisoka
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
1 similar comment
|
/coder-agents-review |
There was a problem hiding this comment.
18 of 20 R1 findings addressed. The P1 cmdk double-highlight fix, both P2 fixes (Enter swallow, scroll/resize anchor refresh), sticky Escape dismissal, useEffectEvent refactor, filterPersonalSkills extraction, and all Nits are verified.
DEREM-7 (LexicalTypeaheadMenuPlugin): Panel closed 4/4 in favor of the author. Pariston read the library source and found its rendering contract incompatible with the project's Popover+cmdk pattern. Mafuuu, Nami, and Meruem independently verified every concrete bug is fixed and the maintenance cost is bounded.
1 P0 (failing test, likely the CI test-js failure), 2 P3 new.
"The integration constraints justify the custom code. The custom plugin is well-tested via 12 story-based interaction tests that exercise the keyboard paths." Pariston
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
There was a problem hiding this comment.
All 26 prior findings resolved. R3 Netero scan clean. Panel (Bisky, Mafuuu, Kite) found one P3.
The feature is well-decomposed: trigger detection in SkillsTriggerPlugin, menu rendering in PersonalSkillsTriggerMenu, filtering in personalSkills.ts, state in ChatMessageInput. The useEffectEvent usage, controlled cmdk value prop, Enter/Tab behavioral split, sticky Escape dismissal, and scroll/resize anchor refresh are all verified working. 13 Storybook stories with play functions cover the full interaction matrix.
Approving with one minor P3 inline.
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
There was a problem hiding this comment.
All 27 findings resolved across 4 rounds. Netero clean. Panel (Bisky, Mafuuu, Chopper) clean. No open findings.
27 findings raised, 22 author-fixed, 1 contested and panel-closed (4/4), 4 dropped by orchestrator. Zero regressions introduced across 4 fix commits.
🤖 This review was automatically generated with Coder Agents.
> Mux updated this PR on behalf of Mike. ## Stack Context This stack splits experimental personal skills into smaller reviewable PRs. Personal skills are user-owned `SKILL.md` files stored by Coder and injected into chatd alongside workspace skills. Stack order: 1. #25362 personal skill resolver 2. #25363 storage, permissions, API, and SDK 3. #25365 API test coverage 4. #25366 chattool and chatd integration 5. #25066 settings UI and docs 6. #25386 personal skills slash menu ## What? Adds the shared personal skill parser and resolver package, plus reusable skill-name validation exported from `workspacesdk`. The parser enforces the full personal skill contract: max raw size, kebab-case name, max name length, and non-empty body. ## Why? The rest of the stack needs one source-aware resolver for personal and workspace skills, including collision handling and qualified aliases. Keeping personal skill constraints in the parser prevents callers from accidentally parsing invalid personal skills. ## Validation - `go test ./coderd/x/skills ./codersdk/workspacesdk` - pre-commit hooks on this branch
5cf6084 to
c77ce40
Compare
a6f2ec8 to
5f36739
Compare
|
Dogfooded this against a real dev build ( Bug: Escape on the open menu blurs the editorRepro:
Observed in both Google Chrome and Playwright's bundled Why Storybook is green
Likely cause
Sketch (un-validated): const handleEscape = useEffectEvent((event: KeyboardEvent) => {
if (!open) return false;
event.preventDefault();
dismissedTriggerRef.current = editor
.getEditorState()
.read(() => activeTriggerFromSelection());
onTriggerChange(null);
queueMicrotask(() => editor.focus()); // defer past Radix's blur
return true;
});If this is the right shape, the Storybook story should also be augmented with a real-browser interaction check (e.g., a Playwright story or Reported by Coder Agents (generated). Full dogfood notes + screenshots available on request. |
5f36739 to
4f2cddf
Compare
c77ce40 to
d35a725
Compare
> Mux updated this PR on behalf of Mike. ## Stack Context This PR is the storage, permissions, API, and SDK layer for experimental personal skills. #25362 has landed on `main`, so this branch is restacked directly on `main`. Stack order: 1. #25363 storage, permissions, API, and SDK 2. #25365 API test coverage 3. #25366 chattool and chatd integration 4. #25066 settings UI and docs 5. #25386 personal skills slash menu ## What? Adds the `user_skills` database table, generated queries, RBAC resources and scopes, audit resource handling, experimental user-scoped CRUD endpoints, SDK types, and generated API/site types. Follow-up review and restack fixes: - Enforce a bounded personal skill description in parser and database constraints. - Return `403 Forbidden` for unauthorized create and update attempts. - Return explicit conflict responses when soft-deleted users are targeted. - Keep user admins out of personal skills, while site owners can read and delete but not create or update. - Document trigger-raised constraint names and keep schema constants covered by tests. - Reuse `UserSkillMetadata` in the full `UserSkill` SDK response type. - Generate user skill IDs in Go instead of relying on a database default. - Rebase on latest `main` and renumber the user skills migration to `000502_user_skills`. ## Why? Personal skills need durable user-owned storage with owner authorization, limited site-owner moderation, and a hidden API surface before chatd can consume them. ## Validation - `make gen` - `go test ./coderd/database -run '^TestUserSkillSchemaConstants$' -count=1` - `go test ./coderd/database/dbauthz -run '^TestMethodTestSuite/TestUserSkills$' -count=1` - `go test ./coderd -run '^TestPatchUserSkill$' -count=1` - `go test ./codersdk ./coderd/database/db2sdk` - `make lint` - pre-commit hook on `97fd58108d`
4f2cddf to
c96c28a
Compare
86d3de7 to
41f8166
Compare
c96c28a to
d702fb2
Compare
41f8166 to
03f1cb1
Compare
> Mux updated this PR on behalf of Mike. ## Stack Context This PR is the API test coverage slice in the experimental personal skills stack. The storage, schema, permissions, API, and SDK implementation merged in #25363. Stack order: 1. #25362 personal skill resolver 2. #25363 storage, permissions, API, and SDK 3. #25365 API test coverage 4. #25366 chattool and chatd integration 5. #25066 settings UI and docs 6. #25386 personal skills slash menu ## What? Adds API and audit tests for personal skill CRUD, validation failures, limits, authorization, soft-delete cleanup, and audit content tracking. This PR is now test-only. It does not include migrations, generated database code, or API implementation changes. ## Why? The feature touches storage, permissions, and audit behavior. These tests make the server behavior reviewable and protected without re-reviewing the implementation that already merged in #25363. ## Validation - `go test ./coderd -run '^(TestUserSkill|TestPatchUserSkill)' -count=1` - `go test ./enterprise/coderd -run '^TestUserSkillAuditDiffTracksContent$' -count=1` - pre-commit hook via `gt modify --no-edit`
|
/coder-agents-review |
There was a problem hiding this comment.
Good progress on the rebase. The author-reported Escape blur fix is verified in code (handleEscape now calls event.stopPropagation() + queueMicrotask(() => editor.focus())).
1 P2, 1 P4 new. The P2 is a structural issue: skill selection does not suppress the trigger re-detection that Escape suppresses via dismissedTriggerRef.
The code refactoring since R4 (extracted parsePersonalSkillTrigger, isPersonalSkillTriggerToken, isSameSkillsTrigger, removed hasSelectedRef) is clean. Netero found no mechanical issues.
"Escape sets dismissedTriggerRef.current before calling onTriggerChange(null), so refreshTrigger suppresses the re-detected trigger. Selection paths do not set the dismissed ref." Mafuuu
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
There was a problem hiding this comment.
All 29 findings resolved across 6 rounds. Netero clean. Panel (Mafuuu, Bisky, Razor) clean.
DEREM-28 fix verified by Mafuuu (who raised it): suppressedSkillsTriggerRef stores the trigger location before replacement. When Lexical's microtask-deferred update listener re-detects the replacement text, handleSkillsTriggerChange matches against the suppressed location and discards the re-trigger. The mechanism is timing-safe and one-shot (cleared on next trigger change).
29 findings raised, 24 author-fixed, 1 contested and panel-closed (4/4), 4 dropped by orchestrator. Zero open. Clean approval.
"Traced all four modes against the full diff. The dual-suppression design (dismissedTriggerRef for Escape, suppressedSkillsTriggerRef for selection) keeps their promises." Mafuuu
🤖 This review was automatically generated with Coder Agents.
> Mux updated this PR on behalf of Mike. ## Stack Context This PR builds on #25365 in the experimental personal skills stack and completes the chat integration. Stack order: 1. #25362 personal skill resolver 2. #25363 storage, permissions, API, and SDK 3. #25365 API test coverage 4. #25366 chattool and chatd integration 5. #25066 settings UI and docs 6. #25386 personal skills slash menu ## What? Updates chattool skill formatting and `read_skill` resolution so tools can read personal skills from the database, then injects personal skill metadata into chatd prompts and registers the skill-reading tools when skills are available. This branch has also been merged with current `origin/main` to resolve merge conflicts. ## Why? The chattool and chatd changes need to land together so the intermediate stack state stays buildable. This completes personal skill availability in chats without syncing personal skills into workspace filesystems. ## Validation - `go test -count=1 ./coderd/x/chatd/chattool -run 'TestFormatResolvedSkillIndex|TestReadSkillTool|TestReadSkillFileTool'` - `go test -count=1 ./coderd/x/chatd -run 'TestPersonalSkillsInSystemPrompt|TestPersonalAndWorkspaceSkillCollisionInSystemPrompt|TestSkillIndexRefreshReplacesStaleAliases|TestFetchPersonalSkillMetadata|TestLoadPersonalSkillBody'` - `go test -count=1 ./coderd -run 'Test.*UserSkill'` - `git diff --cached --check` - `make lint` - pre-commit hook
ec486ef to
795e45c
Compare

Context
PR #25066 has merged. This branch is rebased onto
mainand now contains only the personal skills slash menu UI changes.Summary
/slash-trigger menu in the agent chat composer that filters personal skills by name and description./<skill-name>on click, Enter, or Tab selection while preserving normal composer behavior when the menu is closed.Validation
cd site && pnpm exec vitest run --project=unit src/pages/AgentsPage/components/ChatMessageInput/ChatMessageInput.test.tsx src/pages/AgentsPage/utils/personalSkills.test.tscd site && pnpm lint:typescd site && pnpm lint:check