diff --git a/ui/desktop/src/components/bottom_menu/BottomMenuSkillSelection.test.tsx b/ui/desktop/src/components/bottom_menu/BottomMenuSkillSelection.test.tsx index c8712475d..3bc821767 100644 --- a/ui/desktop/src/components/bottom_menu/BottomMenuSkillSelection.test.tsx +++ b/ui/desktop/src/components/bottom_menu/BottomMenuSkillSelection.test.tsx @@ -478,3 +478,70 @@ describe('BottomMenuSkillSelection', () => { expect(await screen.findByText(/Could not read the skill catalog/)).toBeInTheDocument(); }); }); + +/** + * QA finding F5, this picker's copy of it — the same defect the Settings list + * carried, and the same fix: `skills/searchCatalog.ts`, which is the BAAM + * matcher over this catalog's fields. + * + * This list is flat, so the ranking applies to it directly: the rows come out + * best match first rather than in the catalog's alphabetical order. + */ +describe('BottomMenuSkillSelection search', () => { + beforeEach(() => { + mocks.overrides.clear(); + vi.clearAllMocks(); + serve(view()); + }); + + const search = (term: string) => + fireEvent.change(screen.getByPlaceholderText('Search skills...'), { target: { value: term } }); + + it('finds the skills a multi-word phrase names, best match first', async () => { + serve(view({ skills: [skill('ggplot'), skill('pdf'), skill('r-scripting')] })); + render(); + await openMenu(); + + search('R scripting ggplot visualization'); + + await waitFor(() => expect(screen.getAllByRole('menuitemcheckbox')).toHaveLength(2)); + const rows = screen.getAllByRole('menuitemcheckbox'); + // Catalog order is alphabetical — ggplot, pdf, r-scripting — so this is the + // ranking and not the order the rows arrived in. + expect(rows[0]).toHaveTextContent('r-scripting'); + expect(rows[1]).toHaveTextContent('ggplot'); + }); + + it('holds a one-letter query to whole words', async () => { + serve(view({ skills: [skill('markdown-render'), skill('r-scripting')] })); + render(); + await openMenu(); + + search('R'); + + await waitFor(() => expect(screen.getAllByRole('menuitemcheckbox')).toHaveLength(1)); + expect(screen.getAllByRole('menuitemcheckbox')[0]).toHaveTextContent('r-scripting'); + }); + + /** + * "Enable all" writes every row the filter left on screen, so a filter that + * returns everything under a one-letter query is a bulk write nobody asked + * for. The count in the button is the filtered count. + */ + it('counts only the matched rows in Enable all', async () => { + serve( + view({ + skills: [ + skill('markdown-render', { state: { ...skill('x').state, effective: false } }), + skill('r-scripting', { state: { ...skill('x').state, effective: false } }), + ], + }) + ); + render(); + await openMenu(); + + search('R'); + + expect(await screen.findByRole('button', { name: 'Enable all (1)' })).toBeInTheDocument(); + }); +}); diff --git a/ui/desktop/src/components/bottom_menu/BottomMenuSkillSelection.tsx b/ui/desktop/src/components/bottom_menu/BottomMenuSkillSelection.tsx index b8f2e3bc5..16779d15e 100644 --- a/ui/desktop/src/components/bottom_menu/BottomMenuSkillSelection.tsx +++ b/ui/desktop/src/components/bottom_menu/BottomMenuSkillSelection.tsx @@ -13,6 +13,7 @@ import { useSkillCatalog, type SkillCatalogEntry, } from '../skills/useSkillCatalog'; +import { rankCatalogEntries } from '../skills/searchCatalog'; import { toastService } from '../../toasts'; import BuiltInBadge from '../ui/BuiltInBadge'; import { Tooltip, TooltipContent, TooltipTrigger } from '../ui/Tooltip'; @@ -82,23 +83,20 @@ export const BottomMenuSkillSelection = ({ sessionId }: BottomMenuSkillSelection [applyToggle, scope] ); - const filteredEntries = useMemo(() => { - const q = searchQuery.toLowerCase(); - if (!q) return entries; - return entries.filter((entry) => { - if (entry.kind === 'single') { - return ( - entry.skill.name.toLowerCase().includes(q) || - entry.skill.description.toLowerCase().includes(q) - ); - } - return ( - entry.bundle.displayName.toLowerCase().includes(q) || - entry.bundle.name.toLowerCase().includes(q) || - entry.bundle.skills.some((name) => name.toLowerCase().includes(q)) - ); - }); - }, [entries, searchQuery]); + // One matcher, shared with Settings -> Skills, the Browse modals and the + // model's own search — see `skills/searchCatalog.ts`. This filter used to ask + // whether the WHOLE query occurred inside one field (QA finding F5), so a + // phrase naming two installed skills found neither. The list is flat, so the + // ranking reaches it directly: best match first, catalog order when nothing + // is typed. + // + // ⚠ It also feeds "Enable all", which writes every row left on screen — so a + // filter that leaks (`R` matching the letter anywhere) is a bulk write over + // rows the user never asked about, not just a long list. + const filteredEntries = useMemo( + () => rankCatalogEntries(entries, searchQuery).hits.map((hit) => hit.entry), + [entries, searchQuery] + ); const activeCount = useMemo(() => entries.filter((e) => e.enabled).length, [entries]); const visibleEnabledCount = useMemo( diff --git a/ui/desktop/src/components/skills/SkillsView.test.tsx b/ui/desktop/src/components/skills/SkillsView.test.tsx index 63109bd4a..a1fdebeff 100644 --- a/ui/desktop/src/components/skills/SkillsView.test.tsx +++ b/ui/desktop/src/components/skills/SkillsView.test.tsx @@ -437,3 +437,97 @@ describe('SkillsView built-in bundles', () => { expect(within(row as HTMLElement).getByLabelText(/Delete skill package/)).toBeInTheDocument(); }); }); + +/** + * QA finding F5, this view's copy of it. + * + * The filter asked whether the WHOLE lowercased query occurred inside one + * field, so a phrase naming two installed skills matched neither of them, and + * a one-letter query matched every skill whose prose contained that letter. + * Both are measured below on the rows the daemon serves; the matcher they now + * go through is `searchCatalog.ts`, which is `baam/search.ts` with this + * catalog's fields. + */ +describe('SkillsView search', () => { + const search = (term: string) => + fireEvent.change(screen.getByLabelText('Search skills'), { target: { value: term } }); + + it('finds the skills a multi-word phrase names, best match first', async () => { + serve({ skills: [skill('ggplot'), skill('pdf'), skill('r-scripting')] }); + render(); + await screen.findByText('ggplot'); + + search('R scripting ggplot visualization'); + + // Both are named by the query; before this change the whole phrase was + // looked for as a substring and neither row survived. + expect(await screen.findByText('r-scripting')).toBeInTheDocument(); + expect(screen.getByText('ggplot')).toBeInTheDocument(); + expect(screen.queryByText('pdf')).not.toBeInTheDocument(); + + // One ranked list under a query, not the provenance groups: `r-scripting` + // matches two of the query's terms and `ggplot` one, and a heading would + // have ordered them alphabetically instead. + const matches = screen.getByRole('heading', { level: 2, name: /Matches \(2\)/ }).parentElement!; + const text = matches.textContent ?? ''; + expect(text.indexOf('r-scripting')).toBeLessThan(text.indexOf('ggplot')); + expect(screen.queryByText(/Biorouter Skills/)).not.toBeInTheDocument(); + }); + + it('holds a one-letter query to whole words', async () => { + serve({ skills: [skill('markdown-render'), skill('r-scripting')] }); + render(); + await screen.findByText('r-scripting'); + + search('R'); + + expect( + await screen.findByRole('heading', { level: 2, name: /Matches \(1\)/ }) + ).toBeInTheDocument(); + expect(screen.getByText('r-scripting')).toBeInTheDocument(); + // `markdown-render` holds the letter twice and means nothing by it. + expect(screen.queryByText('markdown-render')).not.toBeInTheDocument(); + }); + + it('keeps the provenance groups when nothing is typed', async () => { + serve({ + skills: [ + skill('my-skill'), + skill('word', { + sourceRoot: '/extensions/BiorOffice/skills', + source: { kind: 'extension', extension: 'BiorOffice', label: 'BiorOffice' }, + }), + ], + }); + render(); + + expect(await screen.findByText('Biorouter Skills (1)')).toBeInTheDocument(); + expect(screen.getByText('From BiorOffice (1)')).toBeInTheDocument(); + expect(screen.queryByText(/Matches \(/)).not.toBeInTheDocument(); + }); + + /** + * The Delete a row offers follows the ROW's own source, not the heading it + * happens to sit under — which is the thing one flat ranked list could + * quietly lose, since `fromExtension` used to be a property of the group. + */ + it('still offers no Delete for an extension-supplied skill inside the matches list', async () => { + serve({ + skills: [ + skill('r-scripting'), + skill('r-plotting', { + sourceRoot: '/extensions/BiorOffice/skills', + source: { kind: 'extension', extension: 'BiorOffice', label: 'BiorOffice' }, + }), + ], + }); + render(); + await screen.findByText('r-scripting'); + + search('R'); + + expect(await screen.findByText('r-plotting')).toBeInTheDocument(); + expect(screen.getByLabelText('Delete r-scripting')).toBeInTheDocument(); + expect(screen.queryByLabelText('Delete r-plotting')).not.toBeInTheDocument(); + }); +}); diff --git a/ui/desktop/src/components/skills/SkillsView.tsx b/ui/desktop/src/components/skills/SkillsView.tsx index 874f8e764..4af02f622 100644 --- a/ui/desktop/src/components/skills/SkillsView.tsx +++ b/ui/desktop/src/components/skills/SkillsView.tsx @@ -21,6 +21,7 @@ import { ReadableContent } from '../Layout/ReadableContent'; import { removeSkillPackage } from '../../api'; import type { CatalogBundle, CatalogSkill } from '../../api'; import { skillCatalogToggleKey, useSkillCatalog, type SkillCatalogEntry } from './useSkillCatalog'; +import { isBrowseQuery, rankCatalogEntries } from './searchCatalog'; /** * Settings → Skills. @@ -40,8 +41,6 @@ type Group = { key: string; title: string; entries: SkillCatalogEntry[]; - /** Skills an installed extension supplies. Not the user's to delete. */ - fromExtension: boolean; }; export default function SkillsView() { @@ -70,23 +69,20 @@ export default function SkillsView() { ); const groups = useMemo((): Group[] => { - const match = (entry: SkillCatalogEntry) => { - if (!searchTerm) return true; - const q = searchTerm.toLowerCase(); - if (entry.kind === 'single') { - return ( - entry.skill.name.toLowerCase().includes(q) || - entry.skill.description.toLowerCase().includes(q) - ); - } - return ( - entry.bundle.displayName.toLowerCase().includes(q) || - entry.bundle.name.toLowerCase().includes(q) || - entry.bundle.skills.some((name) => name.toLowerCase().includes(q)) - ); - }; + // One matcher, shared with the composer's picker, the Browse modals and the + // model's own search — see `searchCatalog.ts`. The filter here used to ask + // whether the WHOLE query occurred inside one field (QA finding F5). + const visible = rankCatalogEntries(entries, searchTerm).hits.map((hit) => hit.entry); + + // ⚠ **A search is one ranked list, not the provenance headings.** The + // headings are a grouping, and a grouping discards the rank: a Biorouter + // skill matching one word of the query would sit above a project skill + // matching all of them. `BrowseSkillsModal` resolved the same tension the + // same way, down to the "Matches (n)" heading. + if (!isBrowseQuery(searchTerm)) { + return visible.length > 0 ? [{ key: 'matches', title: 'Matches', entries: visible }] : []; + } - const visible = entries.filter(match); const biorouter = visible.filter((e) => sourceOf(e).kind === 'biorouter'); const project = visible.filter((e) => sourceOf(e).kind === 'project'); const other = visible.filter((e) => ['claudeHome', 'agentsHome'].includes(sourceOf(e).kind)); @@ -107,14 +103,12 @@ export default function SkillsView() { key: 'biorouter', title: 'Biorouter Skills', entries: biorouter, - fromExtension: false, }); for (const [extension, extensionEntries] of [...byExtension].sort()) { out.push({ key: `extension:${extension}`, title: `From ${extension}`, entries: extensionEntries, - fromExtension: true, }); } if (other.length) @@ -122,14 +116,12 @@ export default function SkillsView() { key: 'other', title: 'Skills From Other Agents', entries: other, - fromExtension: false, }); if (project.length) out.push({ key: 'project', title: 'From This Project', entries: project, - fromExtension: false, }); return out; }, [entries, searchTerm]); @@ -259,8 +251,14 @@ export default function SkillsView() {
- {group.entries.map((entry) => - entry.kind === 'bundle' ? ( + {group.entries.map((entry) => { + // ⚠ Per ENTRY, not per group. A skill an installed + // extension supplies is not the user's to delete — the + // extension would put it back — and under a query every + // provenance is in one "Matches" list, so a flag on the + // group would offer Delete on rows that must not have it. + const fromExtension = sourceOf(entry).kind === 'extension'; + return entry.kind === 'bundle' ? ( void window.electron.openDirectoryInExplorer(entry.bundle.directory) } - onDelete={group.fromExtension ? undefined : () => setPendingDelete(entry)} + onDelete={fromExtension ? undefined : () => setPendingDelete(entry)} onToggle={(enabled) => void toggle(entry, enabled)} /> ) : ( @@ -289,12 +287,12 @@ export default function SkillsView() { onClick={() => void window.electron.openDirectoryInExplorer(entry.skill.directory) } - onDelete={group.fromExtension ? undefined : () => setPendingDelete(entry)} + onDelete={fromExtension ? undefined : () => setPendingDelete(entry)} onShare={() => void copySkill(entry.skill)} onToggle={(enabled) => void toggle(entry, enabled)} /> - ) - )} + ); + })}
))} diff --git a/ui/desktop/src/components/skills/searchCatalog.test.ts b/ui/desktop/src/components/skills/searchCatalog.test.ts new file mode 100644 index 000000000..b10196892 --- /dev/null +++ b/ui/desktop/src/components/skills/searchCatalog.test.ts @@ -0,0 +1,125 @@ +import { describe, expect, it } from 'vitest'; +import type { CatalogBundle, CatalogSkill } from '../../api'; +import type { SkillCatalogEntry } from './useSkillCatalog'; +import { catalogSearchFields, rankCatalogEntries } from './searchCatalog'; +import { Weight } from '../baam/search'; + +const state = { + machineEnabled: true, + session: 'default' as const, + sessionViaBundle: false, + hiddenContext: false, + effective: true, +}; + +function single(name: string, description = `${name} does things`): SkillCatalogEntry { + const skill: CatalogSkill = { + name, + description, + slug: name, + directory: `/skills/${name}`, + sourceRoot: '/skills', + source: { kind: 'biorouter', extension: null, label: 'Biorouter' }, + bundle: null, + builtin: false, + state, + }; + return { kind: 'single', key: name, skill, enabled: true }; +} + +function pack(name: string, members: string[], displayName = name): SkillCatalogEntry { + const bundle: CatalogBundle = { + name, + displayName, + directory: `/skills/${name}`, + sourceRoot: '/skills', + source: { kind: 'biorouter', extension: null, label: 'Biorouter' }, + skills: members, + package: null, + builtin: false, + state, + }; + return { kind: 'bundle', key: name, bundle, enabled: true }; +} + +const names = (entries: readonly SkillCatalogEntry[], query: string) => + rankCatalogEntries(entries, query).hits.map((hit) => + hit.entry.kind === 'single' ? hit.entry.skill.name : hit.entry.bundle.displayName + ); + +describe('installed-catalog search fields', () => { + /** + * A single row's fields are the Rust ones, so the picker and + * `skills__searchSkills` read the same text at the same weights + * (`search_fields` in `agents/skills_extension.rs`). + */ + it('weights a single skill the way the model-facing search does', () => { + expect(catalogSearchFields(single('ggplot', 'plots'))).toEqual([ + ['ggplot', Weight.Name], + ['plots', Weight.Prose], + [undefined, Weight.Label], + ]); + }); + + /** + * ⚠ The decision this file exists to record: a member name is a LABEL on the + * row that contains it, not that row's name. See the note on `searchCatalog.ts`. + */ + it('reads a bundle row as its own names plus its members as labels', () => { + expect(catalogSearchFields(pack('tidyverse', ['ggplot', 'dplyr'], 'Tidyverse'))).toEqual([ + ['Tidyverse', Weight.Name], + ['tidyverse', Weight.Name], + ['ggplot', Weight.Label], + ['dplyr', Weight.Label], + ]); + }); +}); + +describe('installed-catalog search', () => { + /** + * QA finding F5. The whole phrase is in no single field, so the filter this + * replaces returned nothing; a query is a union of its words, ranked. + */ + it('finds every skill a multi-word phrase names, best match first', () => { + const entries = [single('ggplot'), single('pdf'), single('r-scripting')]; + expect(names(entries, 'R scripting ggplot visualization')).toEqual(['r-scripting', 'ggplot']); + }); + + it('holds a one-letter query to whole words', () => { + const entries = [single('markdown-render'), single('r-scripting')]; + expect(names(entries, 'R')).toEqual(['r-scripting']); + }); + + it('finds a package by a skill it contains', () => { + const entries = [single('pdf'), pack('tidyverse', ['ggplot', 'dplyr'], 'Tidyverse')]; + expect(names(entries, 'ggplot')).toEqual(['Tidyverse']); + }); + + /** + * ⚠ **What makes {@link Weight.Label} the right weight for a member name, and + * the assertion that fails if it is changed to `Name`.** + * + * A skill called `ggplot` and a package that merely contains one both hold the + * query as written and both match its only term, so the two are separated by + * the field weight alone: 3 (a whole-word match) × 3 (`Name`) = 9 against + * 3 × 2 (`Label`) = 6. The scores are asserted, not just the order, because at + * `Name` the two would TIE at 39 — and a tie keeps catalog order, which lists + * every bundle before every single skill, so the package would silently win + * every such query. + */ + it('ranks a skill above a package that merely contains one by that name', () => { + const skill = single('ggplot'); + const bundle = pack('tidyverse', ['ggplot', 'dplyr'], 'Tidyverse'); + // Catalog order, which `useSkillCatalog` builds bundles-first. + const hits = rankCatalogEntries([bundle, skill], 'ggplot').hits; + + expect(hits.map((hit) => hit.score)).toEqual([39, 36]); + expect(hits[0].entry).toBe(skill); + expect(hits[1].entry).toBe(bundle); + }); + + it('returns every row in catalog order when nothing is typed', () => { + const entries = [pack('tidyverse', ['ggplot']), single('pdf')]; + expect(names(entries, ' ')).toEqual(['tidyverse', 'pdf']); + }); +}); diff --git a/ui/desktop/src/components/skills/searchCatalog.ts b/ui/desktop/src/components/skills/searchCatalog.ts new file mode 100644 index 000000000..c6c9c2277 --- /dev/null +++ b/ui/desktop/src/components/skills/searchCatalog.ts @@ -0,0 +1,102 @@ +/** + * Free-text search over the INSTALLED skill catalog — the matcher behind + * Settings → Skills and the composer's skill picker. + * + * It is not a matcher. The rules all live in `baam/search.ts`, the port of + * `crates/biorouter/src/catalog_search.rs`, and this file says only what text of + * an installed row is searched and what a match there is worth. Both surfaces + * used to carry their own copy of a whole-phrase `includes(query)` test — QA + * finding F5's third and fourth copies, after the Rust catalog search (PR #266) + * and the Browse modals (PR #255). Measured on this tree against the rows the + * two pickers render, before the change: `R scripting ggplot visualization` + * listed **0 of 3** rows on both surfaces while `ggplot` alone listed 1, and + * `R` listed **2 of 2** — `markdown-render`, which holds the letter twice and + * means nothing by it, and `r-scripting`. + * + * ⚠ **`baam/search.ts` stays where it is, and this file does not copy it.** The + * matcher is shared by import; only the name of its directory is now narrower + * than its callers. Moving it would rewrite four BAAM files for a rename, and + * the header that has to stay accurate — the one warning that a rule change is + * a change to three files, `landing/marketplace-search.js` included — is that + * file's, not this one's. + * + * # The two decisions this file had to make + * + * **1. A bundle row has no counterpart in Rust.** `skills__searchSkills` ranks + * individual skills (`search_fields` in `agents/skills_extension.rs`): the + * skill's own name at {@link Weight.Name}, its description at + * {@link Weight.Prose}, the bundle it ships in at {@link Weight.Label}. No row + * stands for a whole package there, so a member's name is the `Name` field of a + * different entry. In these two pickers the package IS a row — the member is + * reached through it, and in the composer only the package can be toggled — so + * member names have to be searchable somewhere, or a package is unfindable by + * what it contains. + * + * They are searched at {@link Weight.Label}, which is a TypeScript-only rule and + * is argued rather than ported: on a bundle row a member's name is not what the + * row is called, it is one of the labels saying what the row contains — the same + * role a tag plays on a marketplace card. The weight is load-bearing, and + * `searchCatalog.test.ts` pins it with the scores measured on this tree: for the + * query `ggplot`, a skill of its own by that name scores 39 and a package merely + * containing a member called `ggplot` scores 36, so the skill itself ranks + * first. At `Name` both score 39 and the tie falls to catalog order — which puts + * every bundle above every single skill, so the package would always win. + * + * **2. `SkillsView` groups by provenance and would throw the ranking away.** It + * renders Biorouter / per-extension / other-agent / project headings, so under a + * query a Biorouter skill matching one word would sit above a project skill + * matching all of them. It now does what `BrowseSkillsModal` does — one ranked + * "Matches (n)" list under a query, the headings when browsing — and reads + * `isBrowseQuery` from the same module to decide which. The composer's list is + * flat, so the ranking reaches it directly. + */ + +import { + rankEntries, + SKILL_NOISE, + Weight, + type SearchField, + type SearchResult, +} from '../baam/search'; +import type { SkillCatalogEntry } from './useSkillCatalog'; + +export { isBrowseQuery } from '../baam/search'; + +/** + * What one installed row is searched over. + * + * For a single skill these are exactly the Rust fields, so a user typing here + * and a model calling `skills__searchSkills` read the same text: the name, the + * description, and the bundle when the row is a member of one. (`slug` is + * searched on neither side, and was not searched by the filter this replaces.) + * + * For a bundle row, see the note on this file about member names. + */ +export function catalogSearchFields(entry: SkillCatalogEntry): SearchField[] { + if (entry.kind === 'single') { + return [ + [entry.skill.name, Weight.Name], + [entry.skill.description, Weight.Prose], + // Total over the absent case rather than conditional: a picker row is a + // standalone skill today, but the field is the Rust one and stays. + [entry.skill.bundle ?? undefined, Weight.Label], + ]; + } + return [ + [entry.bundle.displayName, Weight.Name], + [entry.bundle.name, Weight.Name], + ...entry.bundle.skills.map((member): SearchField => [member, Weight.Label]), + ]; +} + +/** + * Rank installed catalog rows against what the user typed, best first. An empty + * query returns every row in catalog order, which is what both surfaces showed + * before a character was typed. + */ +export function rankCatalogEntries( + entries: readonly SkillCatalogEntry[], + query: string +): SearchResult { + return rankEntries(query, SKILL_NOISE, entries, catalogSearchFields); +}