Repository navigation
feat(app): add $ trigger for skills in desktop prompt - #51496
shirishpothi wants to merge 2 commits into
Conversation
|
The following comment was made by an LLM, it may be inaccurate: |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved moderate findings affect filtering, prompt preservation, matching, and shell-mode behavior.
Review effort: Lite
Findings: 6
Open (6)
What changed in this PR
Adds $skill autocomplete to desktop/web prompt composers, inserting selected skills as /skill commands.
Changes:
- Detects lowercase
$skilltokens. - Reuses command-popover selection and submission flows.
- Adds tests for insertion and ignored tokens.
| File | Review summary |
|---|---|
packages/session-ui/src/v2/components/prompt-input/machine.ts |
Moderate findings: filter suggestions to skills, preserve suffixes using the cursor range, and disable $ detection in shell mode. |
packages/session-ui/src/v2/components/prompt-input/machine.test.ts |
Adds coverage for skill insertion and ignored dollar expressions. |
packages/app/src/components/prompt-input.tsx |
Moderate findings: filter to skills, preserve existing prompt parts and suffixes, and correct the trigger matcher’s hyphen range. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| .current() | ||
| .map((part) => ("content" in part ? part.content : "")) | ||
| .join("") | ||
| const dollarToken = currentText.match(/(^|\s)\$[a-z][a-zA-Z0-9-_]*$/) |
There was a problem hiding this comment.
Fixed — hyphen moved to the end of the class ([a-zA-Z0-9_-]) in all four matchers, and bare $ is now supported via an optional group that still rejects $HOME/``/currency.
| const dollarToken = currentText.match(/(^|\s)\$[a-z][a-zA-Z0-9-_]*$/) | ||
| if (dollarToken && !menu) { | ||
| const tokenStart = currentText.lastIndexOf("$") | ||
| const prefix = currentText.slice(0, tokenStart) | ||
| const text = `${prefix}/${cmd.trigger} ` |
There was a problem hiding this comment.
Fixed — skill selection now replaces only the `` range at the editor cursor (setRangeEdge start/end) instead of matching the full draft, so trailing text is preserved.
| const text = `${prefix}/${cmd.trigger} ` | ||
| setEditorText(text) | ||
| prompt.set([{ type: "text", content: text, start: 0, end: text.length }, ...images], text.length) |
There was a problem hiding this comment.
Fixed — selection does a DOM-range replacement of the `` and re-parses via handleInput, so existing @ file/agent pills and attachments are retained. The draft-reconstruction path is removed.
| } else if (skillMatch) { | ||
| slashOnInput(skillMatch[1] ?? "") | ||
| setStore({ popover: "slash", slashMenu: false, slashMenuQuery: "" }) |
There was a problem hiding this comment.
Fixed - $ popover now uses a dedicated skillCommands list sourced from skill.list (app.skills), separate from the / slash commands. Selecting inserts /skill text which submits via the existing command path.
| const skill = value.slice(0, cursor ?? value.length).match(/(?:^|\s)\$([a-z][a-zA-Z0-9-_]*)$/) | ||
| if (skill) { | ||
| const query = skill[1] ?? "" | ||
| return changed({ ...state, popover: { type: "command-inline", query }, focus: "editor" }, [ | ||
| ...setText, | ||
| { type: "popover.filter", popover: "command", query }, |
There was a problem hiding this comment.
Fixed - V2 machine has a dedicated skill-inline popover routed to a separate skills accessor (skill-only suggestions), so $ never filters or inserts ordinary commands.
| : current.match(/(?:^|\s)\$[a-z][a-zA-Z0-9-_]*$/) | ||
| ? replaceTrigger(current, "$", `${item.label} `) | ||
| : replaceTrigger(current, "/", `${item.label} `), |
There was a problem hiding this comment.
Fixed - replaceSkillTrigger replaces the $ token using persisted.cursor and appends the trailing suffix, with a non-destructive fallback. Covered by a mid-line cursor test.
Issue for this PR
Related to #20982, #48022, #50664
No open PR adds a
$skill trigger for desktop/web. Verified with search: only TUI$skillPR is #29217, CLI@skillis #50430, desktop skill-slash alignment is #48023.Type of change
What does this PR do?
Adds
$as an additive skill trigger in the desktop/web prompt composer. Typing$+ lowercase name opens a skill-only popover backed byskill.list; selecting replaces just the$tokenat the cursor with/skill, reusing the existing command submit path.@stays for files/agents,/stays for commands.$HOME,$1, and currency amounts do not trigger, and$is disabled in shell mode.CLI
runkeeps@skill(see #50430) since$conflicts with shell env expansion.Follow-ups not in scope: live refresh on skill updates, V2-protocol skill sync (returns empty until #48022 lands).
How did you verify your code works?
bun test src/v2/components/prompt-input/machine.test.tsinpackages/session-ui— 16 pass, including$insert, bare$, mid-line suffix preservation, shell-mode guard, and$HOME/$1ignore cases.bun buildsyntax check on changed session-ui modules; app files reviewed against existingaddPartrange-edit pattern (full typecheck needs repo install).Screenshots / recordings
UI reuses the existing command popover, no new visual component. Manual desktop check pending.
Checklist