diff --git a/frontend/src/lib/components/common/listRow/listHighlight.svelte.ts b/frontend/src/lib/components/common/listRow/listHighlight.svelte.ts index 4da6f18f32..c7325698e3 100644 --- a/frontend/src/lib/components/common/listRow/listHighlight.svelte.ts +++ b/frontend/src/lib/components/common/listRow/listHighlight.svelte.ts @@ -35,9 +35,9 @@ export function useListHighlight(opts: { untrack(() => (index = resting)) }) - function move(delta: number) { + function land(next: number) { const count = opts.count() - if (count === 0) return + if (count === 0 || next < 0 || next >= count) return pointerOwns = false // Rows are tabbable, so focus can sit on one. Enter then activates whatever is // focused, which has to stay the highlighted row — so any row counts, not just @@ -47,16 +47,29 @@ export function useListHighlight(opts: { const focusedId = document.activeElement?.id const rowWasFocused = !!focusedId && Array.from({ length: count }, (_, i) => opts.rowId(i)).includes(focusedId) - index = index < 0 ? (delta > 0 ? 0 : count - 1) : (index + delta + count) % count + index = next const row = document.getElementById(opts.rowId(index)) row?.scrollIntoView({ block: 'nearest' }) if (rowWasFocused) row?.focus() } + function move(delta: number) { + const count = opts.count() + if (count === 0) return + land(index < 0 ? (delta > 0 ? 0 : count - 1) : (index + delta + count) % count) + } + return { get index() { return index }, + /** Step the highlight, for a list whose own keys move it beyond Up and Down — + * a tree stepping into the children a folder just revealed. */ + move, + /** Put the highlight on a row named outright, rather than a step from wherever + * it is — the row a caller's own key landed on, or the one that has focus. A + * step cannot say this: from nothing lit it can only reach an end of the list. */ + moveTo: land, /** Wire to each row's `onMouseEnter`. */ hovered(i: number) { if (pointerOwns) index = i diff --git a/frontend/src/lib/components/copilot/chat/AIChatManager.test.ts b/frontend/src/lib/components/copilot/chat/AIChatManager.test.ts index c0bc289002..7142646d90 100644 --- a/frontend/src/lib/components/copilot/chat/AIChatManager.test.ts +++ b/frontend/src/lib/components/copilot/chat/AIChatManager.test.ts @@ -580,11 +580,11 @@ describe('AIChatManager global skills', () => { mocks.tryGetCurrentModel.mockReturnValue(model) }) - // Only selected skills reach the prompt, and the selection is keyed by - // workspace and account (see skills/enabledSkills.ts). - function selectSkills(workspace: string, ...paths: string[]) { + // Every readable skill reaches the prompt; only the paths someone decided about + // are stored, keyed by workspace and account (see skills/enabledSkills.ts). + function turnOffSkills(workspace: string, ...paths: string[]) { const stored = JSON.parse(localStorage.getItem('wm_skills_enabled') ?? '{}') - stored[`${workspace}:${TEST_EMAIL}`] = paths + stored[`${workspace}:${TEST_EMAIL}`] = Object.fromEntries(paths.map((p) => [p, false])) localStorage.setItem('wm_skills_enabled', JSON.stringify(stored)) } @@ -594,8 +594,6 @@ describe('AIChatManager global skills', () => { resolveParentSkills = resolve }) mocks.workspace = 'parent' - selectSkills('parent', 'f/skills/parent-skill') - selectSkills('child', 'f/skills/child-skill') mocks.listResource.mockImplementation(({ workspace }: { workspace: string }) => { if (workspace === 'parent') { return parentSkills @@ -639,12 +637,12 @@ describe('AIChatManager global skills', () => { expect(manager.systemMessage.content).not.toContain('parent-skill') }) - it('leaves a readable but unselected skill out of the prompt', async () => { + it('leaves a skill turned off out of the prompt', async () => { mocks.listResource.mockResolvedValue([ - { path: 'f/skills/selected', description: 'the one turned on' }, - { path: 'f/skills/unselected', description: 'readable but never turned on' } + { path: 'f/skills/selected', description: 'left on, like every skill starts' }, + { path: 'f/skills/unselected', description: 'the one turned off' } ]) - selectSkills('test_workspace', 'f/skills/selected') + turnOffSkills('test_workspace', 'f/skills/unselected') const manager = new AIChatManager() manager.isSessionChat = true @@ -659,7 +657,6 @@ describe('AIChatManager global skills', () => { mocks.listResource.mockResolvedValue([ { path: 'u/admin/review-code', description: 'review code for bugs' } ]) - selectSkills('test_workspace', 'u/admin/review-code') mocks.runChatLoop.mockImplementation(async (config: any) => { const userMessage = config.messages[config.messages.length - 1] expect(userMessage.content).toContain('Use the skill at "u/admin/review-code". find bugs') @@ -686,7 +683,6 @@ describe('AIChatManager global skills', () => { { path: 'u/admin/deploy', description: 'personal deploy steps' }, { path: 'f/team/deploy', description: 'the team deploy steps' } ]) - selectSkills('test_workspace', 'u/admin/deploy', 'f/team/deploy') mocks.runChatLoop.mockImplementation(async (config: any) => { // Picking either one would silently apply instructions the user did not // choose, so the text is left alone for the model to ask about. diff --git a/frontend/src/lib/components/copilot/chat/AssistantMcpSection.svelte b/frontend/src/lib/components/copilot/chat/AssistantMcpSection.svelte index 9440aa8f58..1637165f87 100644 --- a/frontend/src/lib/components/copilot/chat/AssistantMcpSection.svelte +++ b/frontend/src/lib/components/copilot/chat/AssistantMcpSection.svelte @@ -482,7 +482,7 @@ switch that decides whether this chat carries its tools. {#snippet subtitle()}{server.description}{/snippet} {#snippet trailing()} await toggle(server.path, e.detail)} diff --git a/frontend/src/lib/components/copilot/chat/AssistantSkillsSection.svelte b/frontend/src/lib/components/copilot/chat/AssistantSkillsSection.svelte index b545eaea18..d55f8a9cd1 100644 --- a/frontend/src/lib/components/copilot/chat/AssistantSkillsSection.svelte +++ b/frontend/src/lib/components/copilot/chat/AssistantSkillsSection.svelte @@ -24,19 +24,25 @@ and the actions that create, edit, import and delete them. import { userStore } from '$lib/stores' import { sendUserToast } from '$lib/toast' import { logFeatureUsage } from '$lib/utils/featureUsage' - import { untrack } from 'svelte' + import { tick, untrack } from 'svelte' import { ArrowLeft, BookOpen, + ChevronDown, + ChevronRight, Eye, + Folder, + FolderTree, FolderUp, Pencil, Plus, RotateCcw, - Trash2 + Trash2, + User } from 'lucide-svelte' + import { useListHighlight } from '$lib/components/common/listRow/listHighlight.svelte' import { getAiChatManager } from './aiChatManagerContext' - import { isSkillEnabled, setSkillEnabled } from './skills/enabledSkills' + import { forgetSkill, isSkillEnabled, setSkillEnabled } from './skills/enabledSkills' import { ambiguousSkillNames, deleteSkillResource, @@ -52,6 +58,17 @@ and the actions that create, edit, import and delete them. parseSkillMd, type SkillUpload } from './skills/skillMd' + import { + buildSkillTree, + countSkills, + folderKey, + nodeSkills, + skillFolderPaths, + skillKey, + visibleEntries, + type SkillTreeEntry, + type SkillTreeNode + } from './skills/skillTree' let { ws, @@ -162,6 +179,53 @@ What the assistant should do when this skill applies. let overwriteChoices: Record = $state({}) let ambiguous = $derived(ambiguousSkillNames(skills)) + // Skills in one folder are a list; spread over several, the folder is what tells + // two of them apart, so they are grouped under it instead. + let grouped = $derived(skillFolderPaths(skills).size > 1) + let tree = $derived(buildSkillTree(skills)) + /** Folders the user closed, keyed by full prefix. Everything opens expanded: the + * point of this list is seeing what the assistant carries. */ + let collapsed = $state>({}) + /** The list top to bottom as it stands, for the keyboard to walk. Empty whenever + * the rows are not the thing on screen — a reload after a save or a delete shows + * the loading line, and a failed one shows the error, both over the rows this + * would otherwise still walk. Flat mode has no folders, so the same keys drive + * both layouts. */ + let entries: SkillTreeEntry[] = $derived( + loading || loadError !== undefined + ? [] + : grouped + ? visibleEntries(tree, (path) => collapsed[path] === true) + : skills.map((skill) => ({ kind: 'skill', key: skillKey(skill.path), skill })) + ) + /** Where each entry sits in the walk, so a row rendered deep in the tree can say + * whether it is the highlighted one. */ + let entryIndexByKey = $derived(new Map(entries.map((e, i) => [e.key, i]))) + /** The row the highlight goes back to when the list changes shape under it. + * Folding is the case: it changes the row count without reshuffling what the rows + * mean, and the folder just folded is where someone still is — unlike a search, + * which reranks everything and belongs back at its top hit. + * + * Deliberately not `$state`. `useListHighlight` reads this through `restingIndex` + * inside the effect that reacts to the row count, so a reactive write here would + * re-run that effect and reset the highlight — clearing it on the next arrow would + * undo the very move that cleared it. */ + let stickyKey: string | undefined = undefined + const highlight = useListHighlight({ + count: () => entries.length, + rowId: (index) => entryDomId(entries[index]?.key ?? ''), + // Otherwise nothing is lit until a key or the pointer picks a row: this list has + // no search ranking one to the top. + restingIndex: () => (stickyKey === undefined ? -1 : (entryIndexByKey.get(stickyKey) ?? -1)), + onActivate: (index) => { + const entry = entries[index] + if (entry === undefined) return + if (entry.kind === 'skill') openSkill(entry.skill) + else fold(entry.key, entry.node.path, !collapsed[entry.node.path]) + } + // No `activateEnterFrom`: Enter is answered by `onListKeydown`, which does not + // depend on where focus happens to be. + }) let parsed = $derived(parseSkillMd(content)) // The Path field is what names the skill, and Path validates it. The frontmatter // `name` only seeds that field and is never persisted, so validating it here @@ -219,21 +283,154 @@ What the assistant should do when this skill applies. if (!active && !contentChanged && !pathChanged) editorOpen = false }) + /** The list takes focus when this section is the one on screen. + * + * Without it the keyboard here is unreachable by the way people arrive: the click + * on "Skills" in the settings sidebar leaves that button focused, and the sidebar + * answers Up/Down itself (`arrowTabNav`), so the arrows would walk the sections + * rather than the skills. Taking focus is also what makes the keys unambiguous — + * a control keeps Space and Enter, and none holds them once the list has focus. */ + let listEl: HTMLDivElement | undefined = $state(undefined) + $effect(() => { + if (!active || editorOpen) return + const el = listEl + untrack(() => { + if (el && !el.contains(document.activeElement)) el.focus({ preventScroll: true }) + }) + }) + /** Escape leaves the editor rather than the whole modal: `blocksClose` stops the * modal's own handler, so this is the only thing left to answer the key. */ function onKeydown(event: KeyboardEvent) { // Every section stays mounted while the modal is open, and `stopPropagation` // does nothing between listeners on `window`: without this, a key aimed at the // section on screen is answered by the four behind it too. - if (!active || event.key !== 'Escape' || !editorOpen) return + if (!active) return + // A dialog of ours is up and owns the keyboard. if (toDelete !== undefined || pendingImport !== undefined) return - event.preventDefault() - event.stopPropagation() - closeEditor() + if (editorOpen) { + if (event.key !== 'Escape') return + event.preventDefault() + event.stopPropagation() + closeEditor() + return + } + onListKeydown(event) + } + + /** DOM id of a row, so the highlight can bring itself into view. */ + function entryDomId(key: string): string { + return `wm-skill-entry-${key}` + } + + /** Fold or unfold, keeping the highlight on the folder rather than losing it to + * the row count changing underneath. */ + function fold(key: string, path: string, shut: boolean) { + stickyKey = key + collapsed[path] = shut + // Held only across the re-render this fold causes. Left set, it would pull the + // highlight back to this folder on the next change of any kind — another fold, a + // save, a delete, a workspace switch. + tick().then(() => (stickyKey = undefined)) + } + + /** The tree keys this list adds to `useListHighlight`: Left and Right fold a folder + * or step into it, Space flips the switch under the highlight. Up, Down and Enter + * are the composable's, and so is everything about the mouse — a scroll under a + * resting pointer does not move the highlight, only a real movement does. + * + * Answered at the `window`, not on the list: focus moves around this modal — the + * page transition out of the editor parks it elsewhere — and a handler bound to the + * list would go silent whenever it did. Keys left unanswered keep their meaning: + * Left and Right with nothing lit still step between this list and the editor, + * which is `PagedContent` reading the same event. */ + function onListKeydown(event: KeyboardEvent) { + if (event.metaKey || event.ctrlKey || event.altKey || event.defaultPrevented) return + const target = event.target as HTMLElement | null + // A focused control answers its own activation keys — a row's switch, a folder + // header, the buttons above the list. `Toggle` hides a real checkbox behind its + // label (frontend/AGENTS.md), and that checkbox holds focus after a plain click. + const control = target?.closest?.('button, a, input, select, textarea, [role="button"]') as + | HTMLElement + | null + | undefined + if ((event.key === ' ' || event.key === 'Enter') && control) { + // The control answers the key, and the highlight follows it there: a switch + // holds focus after a plain click, and leaving another row lit would draw one + // row while flipping another. + const row = control.closest('[id^="wm-skill-entry-"]') + const index = row ? (entryIndexByKey.get(row.id.replace('wm-skill-entry-', '')) ?? -1) : -1 + if (index >= 0) highlight.moveTo(index) + return + } + if (event.key === 'ArrowDown' || event.key === 'ArrowUp') { + // A row reached with Tab is where the walk carries on from. `useListHighlight` + // cannot see that itself: `ListRow` puts the row's id on its outer div while + // focus sits on the button inside it. + const focusedRow = target?.closest?.('[id^="wm-skill-entry-"]') + const focusedIndex = focusedRow + ? (entryIndexByKey.get(focusedRow.id.replace('wm-skill-entry-', '')) ?? -1) + : -1 + if (focusedIndex >= 0) highlight.moveTo(focusedIndex) + // Taking the highlight takes the keyboard with it: a control left focused + // would keep Space and act on its own row while another one is lit. + if (control) { + control.blur() + listEl?.focus({ preventScroll: true }) + } + highlight.onKeydown(event) + return + } + const current = entries[highlight.index] + if (current === undefined) { + highlight.onKeydown(event) + return + } + const answer = () => { + event.preventDefault() + event.stopPropagation() + } + if (event.key === ' ') { + answer() + if (current.kind === 'skill') void toggle(current.skill.path, !current.skill.enabled) + else void toggleFolder(current.node, !folderEnabled(current.node)) + } else if (event.key === 'Enter') { + answer() + if (current.kind === 'skill') openSkill(current.skill) + else fold(current.key, current.node.path, !collapsed[current.node.path]) + } else if (event.key === 'ArrowRight') { + answer() + if (current.kind === 'skill') { + // Forward from a lit skill is that skill. Left unanswered the key reaches + // `PagedContent`, which steps to the editor page — showing whichever skill + // it was last parked on, not this one. + openSkill(current.skill) + } else if (collapsed[current.node.path]) { + fold(current.key, current.node.path, false) + } else { + // Into what opening it revealed: the next row down is its first child. + highlight.move(1) + } + } else if (event.key === 'ArrowLeft') { + if (current.kind === 'folder' && !collapsed[current.node.path]) { + answer() + fold(current.key, current.node.path, true) + } else if (current.parentKey !== undefined) { + answer() + const parent = entries.findIndex((e) => e.key === current.parentKey) + if (parent >= 0) highlight.moveTo(parent) + } + } else { + highlight.onKeydown(event) + } } function closeEditor() { editorOpen = false + // The list is what answers the keys, so it takes focus back as the editor gives + // way. Left to the effect above, the page transition lands focus somewhere else + // afterwards and the arrows do nothing until something is clicked. + tick().then(() => listEl?.focus({ preventScroll: true })) } /** Left and Right step between the two pages, which is `PagedContent` answering the @@ -254,6 +451,8 @@ What the assistant should do when this skill applies. untrack(() => { loadSeq++ skills = [] + // Keyed by path, and another workspace's folders are not these. + collapsed = {} listNotice = undefined toDelete = undefined pendingImport = undefined @@ -289,7 +488,7 @@ What the assistant should do when this skill applies. // A notice, not `loadError`: that one replaces the list, and a truncated // read still has skills worth showing. listNotice = truncated - ? `Showing the first ${found.length} skills; this workspace has more. Delete unused ones so the rest can be selected.` + ? `Showing the first ${found.length} skills; this workspace has more. Delete unused ones so the rest are listed here.` : undefined // Seeded rather than filled by the bindings: an unset entry would hand // DropdownV2 an `undefined` open state instead of a closed one. @@ -307,19 +506,41 @@ What the assistant should do when this skill applies. async function toggle(p: string, enabled: boolean) { if (blockedByPendingFork()) return if (!setSkillEnabled(ws, p, enabled)) { - sendUserToast('Could not save the selection for this account.', true) + sendUserToast('Could not save this choice for this account.', true) return } const skill = skills.find((s) => s.path === p) if (skill) skill.enabled = enabled - // Whether people select skills at all. Never the skill itself: a path is + // Whether people turn skills off at all. Never the skill itself: a path is // workspace-authored text. logFeatureUsage('ai_session', 'skill_toggle', { key: enabled ? 'on' : 'off', workspace: ws }) - // The prompt lists exactly the enabled skills, so it has to be rebuilt + // The prompt lists exactly the skills that are on, so it has to be rebuilt // before the next message rather than on the next mode change. await aiChatManager.refreshGlobalSkills(ws) } + /** A folder is on only when everything under it is, so its switch always offers + * the completing action: a folder with one skill off turns fully on first. */ + function folderEnabled(node: SkillTreeNode): boolean { + return nodeSkills(node).every((s) => s.enabled) + } + + async function toggleFolder(node: SkillTreeNode, enabled: boolean) { + if (blockedByPendingFork()) return + for (const skill of nodeSkills(node)) { + if (skill.enabled === enabled) continue + if (!setSkillEnabled(ws, skill.path, enabled)) { + sendUserToast('Could not save this choice for this account.', true) + return + } + skill.enabled = enabled + } + // Once for the folder, not once per skill: what this counts is the action a + // person took. Never the folder itself, which is workspace-authored text. + logFeatureUsage('ai_session', 'skill_toggle', { key: enabled ? 'on' : 'off', workspace: ws }) + await aiChatManager.refreshGlobalSkills(ws) + } + /** Personal folder the folder import writes into. The username is not always a * legal path segment — a superadmin who is not a member of the workspace gets * their email back from `whoami` — and `resource.path` is CHECK-constrained, so @@ -425,11 +646,13 @@ What the assistant should do when this skill applies. validated.skill.description, validated.skill.instructions ) - // A move leaves the old path selected but gone; carry the choice over - // so an edit that renames does not silently switch the skill off. - if (editing.path !== path && isSkillEnabled(target, editing.path)) { - setSkillEnabled(target, editing.path, false) - setSkillEnabled(target, path, true) + // A move leaves the old path's state behind on a path that no longer + // exists; carry it over so an edit that renames does not switch the + // skill back on, and does not inherit whatever the new path held. + if (editing.path !== path) { + const wasEnabled = isSkillEnabled(target, editing.path) + forgetSkill(target, editing.path) + setSkillEnabled(target, path, wasEnabled) } } else { await saveSkillResource( @@ -438,8 +661,9 @@ What the assistant should do when this skill applies. validated.skill.description, validated.skill.instructions ) - // Authoring a skill is the act of choosing it. - setSkillEnabled(target, path, true) + // A skill written here now is not the one someone turned off at this path + // earlier, so it starts from the default rather than inheriting that. + forgetSkill(target, path) } editorOpen = false await refresh(target) @@ -456,9 +680,9 @@ What the assistant should do when this skill applies. const target = ws try { await deleteSkillResource(target, skill.path) - // A later skill at this path is a different one; it must be turned on - // deliberately rather than inherit this one's selection. - setSkillEnabled(target, skill.path, false) + // A later skill at this path is a different one, and must not inherit a + // decision aimed at this one. + forgetSkill(target, skill.path) sendUserToast(`Deleted ${skill.path}`) await refresh(target) } catch (e) { @@ -589,7 +813,7 @@ What the assistant should do when this skill applies. await saveSkillResource(target, dest, skill.description, skill.instructions, { overwrite }) - setSkillEnabled(target, dest, true) + forgetSkill(target, dest) written++ } catch (e) { failed.push(`${skill.name} (${e.body ?? e.message})`) @@ -626,10 +850,22 @@ What the assistant should do when this skill applies. /> {#snippet listPage()} -
+ + + +
{#snippet action()}
@@ -668,8 +904,8 @@ What the assistant should do when this skill applies. {#if forkPending} - Skills are read-only until the first message creates this session's fork. Editing or - selecting one now would apply to the parent workspace and stop applying once the fork is + Skills are read-only until the first message creates this session's fork. Editing one or + turning it off now would apply to the parent workspace and stop applying once the fork is created. {/if} @@ -698,63 +934,143 @@ What the assistant should do when this skill applies. /> {:else}
- {#each skills as skill (skill.path)} - {#snippet icon()} - - {/snippet} - {#snippet title()} - - {ambiguous.has(skill.name) ? skill.path : skill.name} - - {/snippet} - {#snippet subtitle()}{skill.description}{/snippet} - {#snippet trailing()} - await toggle(skill.path, e.detail)} - /> - openSkill(skill) - }, - { - displayName: 'Delete', - icon: Trash2, - type: 'delete', - disabled: !skill.canWrite || forkPending, - action: () => (toDelete = skill) - } - ]} - /> - {/snippet} - openSkill(skill)} - /> - {/each} + {#if grouped} + {#each tree as node (node.path)} + {@render folder(node, true)} + {/each} + {:else} + {#each skills as skill (skill.path)} + {@render row(skill)} + {/each} + {/if}
{/if}
{/snippet} + +{#snippet row(skill: Row)} + {#snippet icon()} + + {/snippet} + {#snippet title()} + + + {!grouped && ambiguous.has(skill.name) ? skill.path : skill.name} + + {/snippet} + {#snippet subtitle()}{skill.description}{/snippet} + {#snippet trailing()} + await toggle(skill.path, e.detail)} + /> + openSkill(skill) + }, + { + displayName: 'Delete', + icon: Trash2, + type: 'delete', + disabled: !skill.canWrite || forkPending, + action: () => (toDelete = skill) + } + ]} + /> + {/snippet} + highlight.hovered(entryIndexByKey.get(skillKey(skill.path)) ?? -1)} + subtitle={skill.description ? subtitle : undefined} + onClick={() => openSkill(skill)} + /> +{/snippet} + + +{#snippet folder(node: SkillTreeNode, root: boolean)} +
+ + +
highlight.hovered(entryIndexByKey.get(folderKey(node.path)) ?? -1)} + > + + await toggleFolder(node, e.detail)} + /> +
+ {#if !collapsed[node.path]} +
+ {#each node.children as child (child.path)} + {@render folder(child, false)} + {/each} + {#each node.skills as skill (skill.path)} + {@render row(skill)} + {/each} +
+ {/if} +
+{/snippet} + {#snippet editorPage()} -
+ +