feat(THU-535): wire skills into the chat composer#906
Merged
Conversation
- Resolve `/slug` tokens to ephemeral system messages on every send, re-reading the user's current skill library so edits take effect immediately (Skills v1 §4). - Add a skills bar above the composer plus a slash-popup autocomplete for inserting either skill chips or full instruction text. - Highlight resolved skill tokens via a transparent-textarea overlay in `PromptInput`, and account for resolved-instruction tokens in context-overflow estimates. - Add `xs`/`icon-xs`/`icon-sm`/`icon-lg` button sizes and Skills v1 PostHog event names used by the new UI. - Support a router-state `runSkill` handoff from settings (single- consume via ref so StrictMode doesn't double-insert).
Semgrep Security ScanNo security issues found. |
PR Metrics
Updated Thu, 28 May 2026 20:58:12 GMT · run #1660 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
- Require whitespace/start-of-input before `/slug` so tokens inside URLs or paths (e.g. `docs/meeting-notes`) no longer inject skill instructions, matching the autocomplete popup's existing behavior. - Add long-press to open the suggestion chip's action menu on touch, and block Radix's open-on-pointer-down so mouse left-clicks still reach the insert-on-click handler instead of being swallowed by the trigger. - Close the dropdown after "Add to chat" selects, so the menu doesn't linger over the composer.
- Reset consumedRunSkillRef after the runSkill router state clears so re-clicking "Run skill" on the same skill inserts the token again. - Drop the unreachable tooltip from unresolved skill tokens; the overlay is pointer-events-none, so color alone carries the cue.
- Move card chrome (border, bg, radius) onto the prompt input itself so the chat surface keeps consistent styling without the outer wrapper. - Accept Tab as a confirm key in the skill slash command popup, matching typical autocomplete affordances.
|
Preview environment destroyed 🧹 Stack |
- add SkillRefAlerts strip below input with enable/create deep links - extend token highlighter with tri-state classifier (enabled/disabled/unknown) to color committed disabled tokens orange and unknown tokens red - mark in-progress tokens (end of input, no trailing space) so the user isn't nagged mid-keystroke - wire skills view to consume editSkill/createSkill router state, with createInitialName pre-filling the create form - cover Tab acceptance in the slash popup alongside Enter
…send - extract resolveSkillTokenInstructions + extractLastUserText so the composer's overflow estimate uses the same resolver as ai/fetch - extract appendSlashToken and computeSkillRefProblems into compose-chat-input with unit tests - consolidate textarea ref in chat-prompt-input - make MobileOverlay a button + add document-level Escape handler - clear pending long-press timer on SuggestionChip unmount
- align with project style for early returns in conditionals
…unmount - target /chats/new directly because the `/` index route's <Navigate replace /> drops location.state, losing the runSkill payload - close the suggestion chip menu before triggering reorder/unpin so siblings don't stay dimmed when the chip unmounts
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
- Drop pin management UI from /settings/skills (pinned rows now show a read-only badge); pinning is exclusively a composer-side affordance. - Replace the SkillRefAlerts strip with hover popovers on disabled / unknown slash tokens, with Enable / Create-it deep links to settings. - Hide the chat skills bar once a thread has any message — pinning is a "starting a new chat" affordance, not in-thread chrome. - Soften the row enable/disable spring and round the suggestion chip popup to match the ModeSelector dropdown shape.
- Commit value and cursor in the same render so the popup doesn't re-detect the slash token at the stale caret after selection. - Skip the trailing space when the following text already starts with whitespace.
- per product direction, the slash popup is now the canonical place to pin skills to the chat bar (rather than /settings/skills) - disable pin button at the cap so users can't silently exceed the limit - pin button blocks the row's mousedown-select so toggling doesn't also insert the slash token
- Replace hand-rolled Popover + mouse-timer with Radix HoverCard to avoid the trigger/content overlap ping-pong that re-opened the popover on close. - Add @radix-ui/react-hover-card dep and matching shadcn-style hover-card primitive. - Adjust tests to assert the trigger surface (open/close cycle is timer- and pointer-driven, unsuitable for JSDOM).
raivieiraadriano92
previously approved these changes
May 28, 2026
darkbanjo
referenced
this pull request
May 28, 2026
* feat(THU-534): skills DAL, encryption registry, and React Query hooks - add skills DAL with create/update/delete, pin/reorder, enable toggles - register skills encrypted columns (name, description, instruction) - export Skill type and SkillRow from types - add useSkills React Query hooks * feat(THU-534): Settings → Skills page (list, detail, create/edit, soft-delete) Co-Authored-By: Jessica Cheng <jessica@example.com> * feat(THU-534): drag-to-reorder pinned skills with 10-pin cap inline error - Add PinnedSection with drag-to-reorder above the main skills list - Surface PinLimitExceededError inline next to the pin trigger (auto-dismiss after 4s) - Expose pin/unpin from the list row dropdown alongside enable/edit/delete * feat(THU-534): seed default skills for new users + fix empty-state layout - Add three starter skills (meeting-notes, weekly-review, task-triage) reconciled like other defaults so users land in a usable state - Keep empty/no-selection state inside the master/detail layout so the list and its create button stay in their normal position * fix(THU-534): guard active.id access when skills list is empty - Prevents a crash when entering detail mode with no active skill selected * chore: use bun --watch directly for backend dev script - Replaces ./scripts/dev.sh wrapper with a direct bun --watch invocation * Revert "chore: use bun --watch directly for backend dev script" This reverts commit c4f9fd0. * fix(THU-534): type `active` as `Skill | undefined` so TS catches unguarded accesses Used `skills.at(0)` instead of `skills[0]` so the return type is honestly `Skill | undefined`. The tsconfig doesn't have `noUncheckedIndexedAccess`, so `skills[0]` was typed as `Skill` even when empty, and a `| undefined` annotation wouldn't widen the rhs. Switching to `.at(0)` made TS flag the remaining unguarded `active.name` access in DeleteSkillDialog and force a proper conditional render. * fix(THU-534): align skills panels flush to top of settings content area Drop top padding from SkillsList and SkillDetail so the headings sit immediately below the page header instead of starting ~16px lower than the rest of the settings chrome. * fix(THU-534): show enable toggle on pinned rows + drop settings header on skills route - PinnedRow now renders the same Switch as the main list rows so users can enable/disable a pinned skill without clicking into the detail view - SettingsLayout skips the global Header on /settings/skills — the SkillsView brings its own page chrome (heading + create action + mobile sidebar trigger), so the inset Header would otherwise add ~56px of empty space above the Skills heading. Matches the reference design. * fix(THU-534): align skills title to sidebar logo y-position + decouple pin from enable - SkillsList/SkillDetail title rows now use `h-[var(--touch-height-xl)]` matching the sidebar header height, so the "Skills" heading and skill detail title sit at the same y-position as the Thunderbolt app logo - Toggling a pinned skill off no longer auto-unpins it. Pin state and enabled state are independent now: a disabled pinned skill stays in the Pinned section with its grip + pin icon, just dimmed - Update detail-view tooltip to reflect the new semantics * fix(THU-534): add row dropdown menu to pinned skills for feature parity Pinned rows now expose the same Unpin / Edit / Run in chat / Delete menu that the main library rows already have. Without it, the only way to act on a pinned skill was to click into the detail view. * feat(THU-534): DISABLED section + cross-section animation; single themed separator - Split the unpinned library list into Enabled and DISABLED sections - New LibraryRow component wraps each row in m.li layout layoutId={id} inside a LayoutGroup, so toggling enabled state animates the row's move between sections via framer-motion's shared-layout animation - Drop the duplicate border between SkillsList and SkillDetail/empty panel (was stacking to 2px effective); switch list-side border to border-sidebar-border so it matches the main sidebar's separator color and weight * feat(THU-534): enforce AgentSkills spec on skill names (1-64 chars, [a-z0-9-], no leading/trailing/consecutive hyphens) - Add `validateSkillName(raw)` to the skills DAL - accepts both bare slug and `/slug` forms and returns a human-readable error string (or null) per the AgentSkills name spec (https://agentskills.io/specification#name-field) - DAL `createSkill` and `updateSkill` throw new `SkillNameInvalidError` on violation; `SkillsView.handleSubmit` catches it the same way it catches `SkillNameTakenError` and surfaces the message inline - `SkillForm` shows the same validation live as the user types - replaces the empty space below the field with either a help string ("Lowercase letters, numbers, and hyphens. 1-64 characters.") or the inline error, and disables submit while the name is invalid - 10 new tests cover spec edge cases (empty, length, case, punctuation, Unicode, leading/trailing/consecutive hyphens, /-prefix handling) * refactor(THU-534): store skill names as bare slugs per AgentSkills spec The `/` is the chat trigger, not part of the name. Storing bare slugs makes us spec-compliant and forward-compatible with the AgentSkills `SKILL.md` import path (filesystem-based skills already use the bare form). - DAL validator no longer strips a leading `/`; it rejects any character outside [a-z0-9-]. Bare slugs only. - `SkillForm` renders a Stripe-style fixed `/` glyph inside the input as a visual prefix the user can't select or edit. Pasted `/` is stripped from the value silently. - Display sites (list rows, pinned rows, detail header, delete + dependents dialogs) prepend `/` for presentation only; storage stays bare. - `findDependents` regex now looks for `/${slug}` (the chat token form) in other skills' instructions. - Default seeds (`meeting-notes`, `weekly-review`, `task-triage`) lose the `/` prefix; the reconciler's defaultHash diff will rewrite old rows on next sign-in for users whose seeds were never edited. - `useLibrarySkills` / `usePinnedSkills` / `useEnabledSkills` mutations now invalidate the `['skills']` query key in `onSuccess` — fixes the bug where newly created skills didn't appear without a manual refresh (PowerSync's reactive `useQuery` wasn't picking up the table change reliably). - Tests updated to use bare slugs throughout; 25 pass. * refactor(THU-534): switch skill dialogs to AlertDialog primitive The bespoke `Dialog` chrome (`gap-6 p-8 max-w-[466px] h-12 w-full grid-cols-2`) read as chunky and inconsistent with the rest of the app's confirmation flows (delete-chat-dialog, revoke-device-dialog, etc.). Switch all three skill dialogs (delete, dependents, discard-changes) to the shared `AlertDialog` primitive — smaller default padding, right-aligned footer with normal-sized buttons, left-aligned text. Also tighten copy: - Delete: `Delete /name?` title + one-line description about dependents - Dependents: `Delete /name?` title + pluralized count ("1 skill" / "N skills") - Discard: unchanged copy, restyled chrome * refactor(THU-534): auto-unpin a skill when it's disabled Reverses an earlier decision to keep enabled/pin state independent. A disabled skill can't be summoned from the chat pinned bar (THU-535), so leaving it in a pin slot would waste one of the 10 available. Disabling now unpins as a side-effect; re-enabling does NOT auto-repin -- the user pins again deliberately. The row animates from the PINNED section into DISABLED via the existing m.li layoutId shared-layout transition. * test(THU-534): cover find-dependents + SkillsView state machine + add stories; AGENTS.md cleanup Tests - `src/skills/find-dependents.test.ts` — 10 cases covering empty libraries, description vs instruction matches, multiple dependents, self-reference exclusion, word-boundary at space/EOL/newline, prefix collisions (`/foo` vs `/foo-bar`), bare-slug non-matches (post-spec rename), regex-special-char escape, case sensitivity - `src/skills/skills-view.test.tsx` — 5 integration cases through the DOM: auto-unpin on row-switch disable, no-auto-repin on re-enable, dependents dialog dispatch on disable (scoped via `within(alertdialog)` so /b in the list row doesn't ambiguate), spec-violation surfaces inline live as the user types, `SkillNameTakenError` surfaces inline on submit - 40 passing tests across the three skills test files Stories - `src/stories/DeleteSkillDialog.stories.tsx` (Open, LongName, Closed) - `src/stories/DependentsDialog.stories.tsx` (Disable + single dep, Delete + multiple deps) - `src/stories/DiscardCreateDialog.stories.tsx` (Create + Edit variants) - `src/stories/SkillForm.stories.tsx` (Create, Edit, NameTaken, SpecViolationFromServer) AGENTS.md cleanup - `skill-form.tsx`: drop the \`useEffect\`-notifying-parent anti-pattern. \`onDirtyChange\` now fires from each onChange handler with the next hypothetical state computed inline. React docs + AGENTS.md call this pattern out explicitly. Net: one fewer effect, no behavior change. - `find-dependents.ts`: drop defensive null checks on \`description\` / \`instruction\`. The \`Skill\` type marks them non-nullable (soft-deletes are filtered upstream by \`getAllSkills\`), so the checks were unreachable per AGENTS.md "prefer optimistic code over defensive code". - `dal/skills.ts`: add JSDoc to \`getSkill\`, \`getSkillByName\`, \`setEnabled\` so all exported utility functions match the JSDoc convention. * refactor(THU-534): harden skill delete flow and polish pinned label - Snapshot the target skill when opening the delete dialog so a concurrent sync mutating `skills` can't redirect the confirm to a different row. - Show the leading slash on pinned skill rows to match how skills are referenced elsewhere. - Rename `SkillNameTakenError`'s constructor param to `skillName` to avoid shadowing the inherited `Error.name` field. * chore(THU-534): broaden framer-motion mock in chat-list-item test - Bun's mock.module is process-global; the narrow mock leaked into concurrent tests and crashed components rendering <m.ul>, LayoutGroup, or LazyMotion with "Element type is invalid". - Proxy motion/m to auto-generate any tag and stub LayoutGroup, LazyMotion, domAnimation, domMax so other tests in the same bun process aren't polluted. * test(THU-534): stabilize framer-motion mock in chat-list-item test - Cache motion tag components per tag to keep references stable across renders, preventing unmount-remount cycles that triggered "Maximum update depth exceeded" in downstream concurrent tests. * refactor(THU-534): address Cursor low-severity feedback - Widen find-dependents regex to match punctuation-terminated references (e.g. `/skill,`, `/skill.`, `(/skill)`) using a `[a-z0-9-]` lookahead instead of whitespace/end-of-string only. - Extract `useSkillsQuery` to deduplicate the shared skills subscription so `useEnabledSkills` no longer registers unused create/update/remove mutations via `useLibrarySkills`. * chore(THU-534): fix mobile cancel and jump-to-dependent edge cases - drive mobile list nav from performLeave so the panel stays visible while the discard-confirmation dialog is open - slide mobile panel in when jumping to a dependent skill from the dependents dialog so the edit form has a surface to render * refactor(THU-534): extract skills-view reducer and migrate legacy names - Move SkillsView's 10 useState hooks into a typed reducer per CLAUDE.md guidance. - Add one-shot `stripLegacyNameSlashes` DAL helper that runs inside the reconciler transaction so default-hash matching sees post-rename rows (covers user-edited rows the reconciler treats as user-modified). - Hoist the framer-motion bun mock into `test-utils/framer-motion-mock.ts` so concurrent test files share one process-global registration. * chore(THU-534): clear stale name errors and tidy disable/unpin - add CLEAR_NAME_ERROR action so a uniqueness error from the parent drops as soon as the user edits the name field - validate skill name against the trimmed value so submit and inline validation agree on whitespace-padded input - extract disableSkill helper to colocate auto-unpin with the disable write and reuse it from the dependents-confirm path - drop the redundant unpin call after softDeleteSkill; the DAL already nulls pinnedOrder on the tombstone row * chore(THU-534): block submit while server-side name error is showing - Include nameError in canSubmit so a SkillNameTakenError disables the button until the user edits the name (handleNameChange clears it). * refactor(THU-534): simplify skills dropdown menu styling - Drop custom width/padding/radius overrides in favor of DropdownMenuContent defaults - Replace per-item height/gap/svg overrides with default item styling - Add cursor-pointer to menu triggers and items for clearer affordance * chore(THU-534): reset edit state on dependents jump and drop nullish guards - clear isDirty/nameError on OPEN_DEPENDENTS and JUMP_TO_DEPENDENT so a stale dirty flag from the prior form doesn't trigger a spurious discard-changes dialog on the freshly remounted edit form - remove ?? '' fallbacks now that skill fields are guaranteed non-null - convert SettingsLayout to an arrow function per project style --------- Co-authored-by: Jessica Cheng <jessica@example.com>
- replace slash popup pin toggles with a `+` popover on the bar - drop the legacy `/`-prefixed name migration; defaults now ship bare - swap starter skills for daily-brief and important-emails defaults - rename "Run in chat" to "Add to chat" across detail / library / chip - auto-focus name input in create mode; trim form helper copy - guard `appendSlashToken` against double-append when slug already trails
Semgrep Security ScanNo security issues found. |
- Export `skillRowTransition` from `library-row` and reuse it on the enabled/disabled `<m.ul>`/`<m.div>` wrappers so containers and rows animate in lockstep instead of the wrappers reflowing early. - Switch wrappers to `layout="position"` so sections reposition without interpolating height (which stretched the `<h2>` heading). - Drop the row's delay from 1.2s to 0.3s now that wrappers wait too.
…Chip - Read input via ref so deferred runSkill microtask doesn't operate on a stale closure value (a draft restore racing the microtask could silently discard the user's text). - Move setCursorPos out of the rAF so value and cursor commit together, avoiding a brief stale-cursor flash that re-opened the slash popup.
…ender - Address PR review feedback: update setCursorPos alongside setInput so the intermediate render does not briefly reopen the slash popup with a stale cursor position inside a /slug token. - Mirrors the same fix already applied in addSkillChip and the selectSkill helper in use-slash-command.ts.
5 tasks
- Use rounded-2xl on the outer popup so it aligns with ModeSelector and the chip dropdown; nest rows at rounded-xl so the highlight sits concentrically inside the p-1 padding.
- Adds regression coverage so new users always land in chat with the starter chip bar populated; pinning is now only manageable from the composer, so an unpinned default would leave the bar empty until the user discovers the `+` popover.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ed46e6b. Configure here.
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
2 participants
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.
/slugtokens to ephemeral system messages on every send, re-reading the user's current skill library so edits take effect immediately (Skills v1 §4).PromptInput, and account for resolved-instruction tokens in context-overflow estimates.xs/icon-xs/icon-sm/icon-lgbutton sizes and Skills v1 PostHog event names used by the new UI.runSkillhandoff from settings (single- consume via ref so StrictMode doesn't double-insert).Note
Medium Risk
Changes the inference message stack (prepended system content) and default seeded skills; scope is large but covered by tests and shared resolution helpers keep overflow vs send aligned.
Overview
Wires Skills v1 into the chat composer:
/slugtokens in the latest user message become ephemeral system messages on every send/regenerate (via sharedresolveSkillTokenInstructionsinai/fetch.tsand the composer), using the current enabled skill library—not a snapshot at type time.The composer gains a pinned skills bar (hidden after the first message), slash autocomplete, token highlighting through a
PromptInputoverlay, context-overflow estimates that include resolved instruction tokens, and routerrunSkill/ settings deep-links (editSkill,createSkill) that navigate to/chats/newor pre-fill create. Settings drops the pinned list/reorder UI in favor of chat-only pinning (ChatSkillsBar,ReorderPanel,SuggestionChip).Defaults shift to
daily-briefandimportant-emails(pinned/enabled);stripLegacyNameSlashesand settings pin controls are removed. Adds@radix-ui/react-hover-card, PostHogskill_*events, and small button size variants.Reviewed by Cursor Bugbot for commit ed46e6b. Bugbot is set up for automated code reviews on this repo. Configure here.