diff --git a/packages/host/app/components/ai-assistant/skill-menu/index.gts b/packages/host/app/components/ai-assistant/skill-menu/index.gts index 35815ead8ca..46965f10105 100644 --- a/packages/host/app/components/ai-assistant/skill-menu/index.gts +++ b/packages/host/app/components/ai-assistant/skill-menu/index.gts @@ -1,6 +1,7 @@ import { fn } from '@ember/helper'; import { on } from '@ember/modifier'; import { action } from '@ember/object'; +import { service } from '@ember/service'; import Component from '@glimmer/component'; import { tracked } from '@glimmer/tracking'; @@ -11,18 +12,19 @@ import pluralize from 'pluralize'; import { Button } from '@cardstack/boxel-ui/components'; -import { baseRRI, skillCardRef } from '@cardstack/runtime-common'; -import { chooseCard, chooseFile } from '@cardstack/runtime-common'; +import { baseRRI, chooseCard, skillCardRef } from '@cardstack/runtime-common'; import SkillToggle from '@cardstack/host/components/ai-assistant/skill-menu/skill-toggle'; import PillMenu from '@cardstack/host/components/pill-menu'; -import type { RoomSkill } from '@cardstack/host/resources/room'; +import { skillsRealmURL } from '@cardstack/host/lib/utils'; -import type { FileDef } from '@cardstack/base/file-api'; +import type { RoomSkill } from '@cardstack/host/resources/room'; +import type RealmServerService from '@cardstack/host/services/realm-server'; // A skill expressed as a markdown file is a `MarkdownDef` whose frontmatter -// declares `boxel.kind: skill`. The file chooser is scoped to exactly those. +// declares `boxel.kind: skill`. One branch of the mixed chooser's base filter +// selects exactly those. const markdownDefRef = { module: baseRRI('markdown-file-def'), name: 'MarkdownDef', @@ -75,8 +77,8 @@ export default class AiAssistantSkillMenu extends Component { class='attach-button' @kind='primary' @size='extra-small' - {{on 'click' this.attachSkillCard}} - @disabled={{this.doAttachSkillCard.isRunning}} + {{on 'click' this.attachSkill}} + @disabled={{this.doAttachSkill.isRunning}} @loading={{this.isAttachingSkill}} data-test-pill-menu-add-button > @@ -86,21 +88,6 @@ export default class AiAssistantSkillMenu extends Component { Choose a Skill to add {{/if}} - + @service declare private realmServer: RealmServerService; + @tracked private isExpanded = false; @tracked private isAttachingSkill = false; - @tracked private isAttachingSkillMarkdown = false; private urlForRealmLookup(skill: RoomSkill) { return skill.fileDef.sourceUrl; @@ -184,53 +172,65 @@ export default class AiAssistantSkillMenu extends Component { } @action - private attachSkillCard() { - this.doAttachSkillCard.perform(); + private attachSkill() { + this.doAttachSkill.perform(); } - private doAttachSkillCard = restartableTask(async () => { - let selectedCardIds = + // One chooser attaches either kind of skill: a Skill card or a skill-bearing + // markdown file (a MarkdownDef with `boxel.kind: skill`). The mixed chooser + // (`includeFiles`) surfaces both in a single list and tags each pick with its + // kind, which routes the result to the matching attach callback. + private doAttachSkill = restartableTask(async () => { + // Exclude already-attached skills, matched client-side against the menu's + // own skill list by `id`. Card skills are excluded immediately; file skills + // once their rows carry `id` in the search doc (Phase 1 reindex), and the + // feature ships without waiting on that reindex. + // + // The exclusion must be NULL-safe: a bare `not: { eq: { id } }` drops any + // row whose `id` is absent, because negated `eq` compiles to `NOT (id = X)` + // and `NOT (NULL = X)` is NULL (excluded) under three-valued logic. On a + // realm not yet carrying the stamped `id`, every skill-file row lacks `id`, + // so that would hide *all* skill files whenever any skill is attached. The + // `{ eq: { id: null } }` branch (an `IS NULL` test) keeps those rows. + let exclusions = this.args.skills?.map((skill: RoomSkill) => ({ - not: { eq: { id: skill.cardId } }, + any: [{ eq: { id: null } }, { not: { eq: { id: skill.cardId } } }], })) ?? []; - // query for only displaying skill cards that are not already selected let query = { filter: { - every: [{ type: skillCardRef }, ...selectedCardIds], + every: [ + { + any: [ + { type: skillCardRef }, + { on: markdownDefRef, eq: { kind: 'skill' } }, + ], + }, + ...exclusions, + ], }, + // Scope to the realms where attachable skills live: the user's own + // workspaces, any catalog realms, and the shared Boxel Skills realm. + realms: [ + ...new Set([ + ...this.realmServer.userRealmIdentifiers, + ...this.realmServer.catalogRealmIdentifiers, + skillsRealmURL, + ]), + ], }; - let cardId = await chooseCard(query); - if (cardId) { - try { - this.isAttachingSkill = true; - await this.args.onChooseCard?.(cardId); - } finally { - this.isAttachingSkill = false; - } + let chosen = await chooseCard(query, { includeFiles: true }); + if (!chosen) { + return; } - }); - - @action - private attachSkillMarkdown() { - this.doAttachSkillMarkdown.perform(); - } - - // Parallel to doAttachSkillCard: pick a skill markdown file (a MarkdownDef - // with `boxel.kind: skill`) rather than a Skill card. The file chooser is - // scoped to skill markdown via the indexed `kind` field. - private doAttachSkillMarkdown = restartableTask(async () => { - let file = await chooseFile({ - fileType: markdownDefRef, - fileTypeName: 'Skill', - fileFieldFilter: { kind: 'skill' }, - }); - if (file?.sourceUrl) { - try { - this.isAttachingSkillMarkdown = true; - await this.args.onChooseSkillMarkdown?.(file.sourceUrl); - } finally { - this.isAttachingSkillMarkdown = false; + try { + this.isAttachingSkill = true; + if (chosen.kind === 'file') { + await this.args.onChooseSkillMarkdown?.(chosen.id); + } else { + await this.args.onChooseCard?.(chosen.id); } + } finally { + this.isAttachingSkill = false; } }); diff --git a/packages/host/app/components/card-search/panel.gts b/packages/host/app/components/card-search/panel.gts index 5acd63506ac..6941f915da5 100644 --- a/packages/host/app/components/card-search/panel.gts +++ b/packages/host/app/components/card-search/panel.gts @@ -67,7 +67,11 @@ export default class SearchPanel extends Component { private get initialFocusedSectionId(): string | null { let realmURLs = this.args.initialSelectedRealms; - if (!realmURLs || realmURLs.length === 0) { + // Only auto-focus a section when the search is pinned to a single realm. + // Focusing collapses every other realm's section to its header, so for a + // multi-realm scope (e.g. the skill chooser's user + catalog + skills) + // focusing the first realm would hide the results of all the others. + if (!realmURLs || realmURLs.length !== 1) { return null; } return `realm:${realmURLs[0].href}`; diff --git a/packages/host/app/utils/card-search/query-builder.ts b/packages/host/app/utils/card-search/query-builder.ts index ff2d77162cd..2b53f371ce9 100644 --- a/packages/host/app/utils/card-search/query-builder.ts +++ b/packages/host/app/utils/card-search/query-builder.ts @@ -60,12 +60,12 @@ function stripTypeFromFilter(filter: Filter): Filter | undefined { return on ? { every: children, on } : { every: children }; } if (isAnyFilter(filter)) { - const children = filter.any - .map((f) => stripTypeFromFilter(f)) - .filter((f): f is Filter => f !== undefined); - if (children.length === 0) return undefined; - if (children.length === 1) return on ? { ...children[0], on } : children[0]; - return on ? { any: children, on } : { any: children }; + // Never strip within an `any`: the branches are alternatives, so dropping + // a branch's type (or collapsing a stripped-empty branch) changes which + // rows match. e.g. the skill menu's `any: [{ type: skillCardRef }, { on: + // markdownDefRef, eq: { kind: 'skill' } }]` would collapse to the markdown + // branch alone, excluding skill cards. Keep the alternation intact. + return filter; } if (isNotFilter(filter)) { const inner = stripTypeFromFilter(filter.not); @@ -211,8 +211,10 @@ export function buildSearchQuery( let filters: Filter[]; if (baseFilter) { // When typeFilter is present, strip the baseFilter's (parent) type - // constraint as redundant — the picked subtype already implies it. Safe - // because the type picker only offers subtypes of the baseFilter type. + // constraint as redundant — the picked subtype already implies it. + // `stripTypeFromFilter` only unwraps a top-level `{ type }` node and leaves + // any `any`-alternation intact, so a compound base filter passes through + // whole. const effectiveBaseFilter = typeFilter ? stripTypeFromFilter(baseFilter) : baseFilter; diff --git a/packages/host/tests/integration/components/ai-assistant-panel/skills-test.gts b/packages/host/tests/integration/components/ai-assistant-panel/skills-test.gts index 890abd7cc8b..bae6fddc7f3 100644 --- a/packages/host/tests/integration/components/ai-assistant-panel/skills-test.gts +++ b/packages/host/tests/integration/components/ai-assistant-panel/skills-test.gts @@ -10,6 +10,7 @@ import { REPLACE_MARKER, SEARCH_MARKER, SEPARATOR_MARKER, + ensureTrailingSlash, rri, skillCardRef, } from '@cardstack/runtime-common'; @@ -22,6 +23,7 @@ import { } from '@cardstack/runtime-common/matrix-constants'; import OperatorMode from '@cardstack/host/components/operator-mode/container'; +import ENV from '@cardstack/host/config/environment'; import type OperatorModeStateService from '@cardstack/host/services/operator-mode-state-service'; @@ -56,6 +58,8 @@ import { setupRenderingTest } from '../../../helpers/setup'; import type { FileDef } from '@cardstack/base/file-api'; +const testRealmServerURL = ensureTrailingSlash(ENV.realmServerURL); + module('Integration | ai-assistant-panel | skills', function (hooks) { const realmName = 'Operator Mode Workspace'; let loader: Loader; @@ -79,6 +83,11 @@ module('Integration | ai-assistant-panel | skills', function (hooks) { let mockMatrixUtils = setupMockMatrix(hooks, { loggedInAs: '@testuser:localhost', activeRealms: [testRealmURL], + // Populate the user's realm list so `realmServer.userRealmIdentifiers` is + // non-empty — the skill menu scopes its mixed chooser to those realms, and + // without this the search fans out across every server realm (the large + // shared skills realm included) and the intended tiles never render. + activeRealmServers: [testRealmServerURL], autostart: true, now: (() => { // deterministic clock so that, for example, screenshots @@ -416,23 +425,19 @@ Instructions live in the markdown body. assert.dom('[data-test-skill-menu]').containsText('Skills: 2 of 2 active'); }); - test('skill picker can add a skill markdown file through the UI', async function (assert) { + test('skill picker can add a skill markdown file through the single mixed chooser', async function (assert) { const roomId = await renderAiAssistantPanel(); let skillId = `${testRealmURL}realm-sync-skill.md`; await click('[data-test-skill-menu][data-test-pill-menu-button]'); - await waitFor( - '[data-test-skill-menu] [data-test-pill-menu-add-markdown-button]', - ); - await click( - '[data-test-skill-menu] [data-test-pill-menu-add-markdown-button]', - ); + await waitFor('[data-test-skill-menu] [data-test-pill-menu-add-button]'); + await click('[data-test-skill-menu] [data-test-pill-menu-add-button]'); - // The file chooser is scoped to skill markdown files. - await waitFor('[data-test-choose-file-modal]'); - await waitFor('[data-test-file="realm-sync-skill.md"]'); - await click('[data-test-file="realm-sync-skill.md"]'); - await click('[data-test-choose-file-modal-add-button]'); + // A skill markdown file surfaces as a result tile in the same mixed chooser + // as skill cards; picking it routes to the markdown-attach path by kind. + await waitFor(`[data-test-item-button="${skillId}"]`); + await click(`[data-test-item-button="${skillId}"]`); + await click('[data-test-card-chooser-go-button]'); await waitUntil( () => @@ -468,37 +473,47 @@ Instructions live in the markdown body. ); }); - test('canceling the skill-file chooser re-enables the add-markdown button', async function (assert) { + test('the separate skill-file button is retired in favor of the single mixed chooser', async function (assert) { await renderAiAssistantPanel(); await click('[data-test-skill-menu][data-test-pill-menu-button]'); - await waitFor( - '[data-test-skill-menu] [data-test-pill-menu-add-markdown-button]', - ); - await click( - '[data-test-skill-menu] [data-test-pill-menu-add-markdown-button]', - ); + await waitFor('[data-test-skill-menu] [data-test-pill-menu-add-button]'); + + assert + .dom('[data-test-skill-menu] [data-test-pill-menu-add-button]') + .exists('a single add button remains'); + assert + .dom('[data-test-skill-menu] [data-test-pill-menu-add-markdown-button]') + .doesNotExist( + 'the second, file-tree skill button is gone — one chooser handles both kinds', + ); + }); + + test('canceling the skill chooser re-enables the add button', async function (assert) { + await renderAiAssistantPanel(); + + await click('[data-test-skill-menu][data-test-pill-menu-button]'); + await waitFor('[data-test-skill-menu] [data-test-pill-menu-add-button]'); + await click('[data-test-skill-menu] [data-test-pill-menu-add-button]'); - await waitFor('[data-test-choose-file-modal]'); - await click('[data-test-choose-file-modal-cancel-button]'); + await waitFor('[data-test-card-chooser-modal]'); + await click('[data-test-card-chooser-cancel-button]'); // Canceling settles the chooser promise, so the trigger button returns to // its enabled state. A chooser that left its deferred unsettled would hang // the attach task and leave this button stuck disabled. await waitUntil( - () => !document.querySelector('[data-test-choose-file-modal]'), + () => !document.querySelector('[data-test-card-chooser-modal]'), ); assert - .dom('[data-test-skill-menu] [data-test-pill-menu-add-markdown-button]') - .isNotDisabled('the add-markdown button is re-enabled after canceling'); + .dom('[data-test-skill-menu] [data-test-pill-menu-add-button]') + .isNotDisabled('the add button is re-enabled after canceling'); // The chooser can be reopened after canceling. - await click( - '[data-test-skill-menu] [data-test-pill-menu-add-markdown-button]', - ); - await waitFor('[data-test-choose-file-modal]'); + await click('[data-test-skill-menu] [data-test-pill-menu-add-button]'); + await waitFor('[data-test-card-chooser-modal]'); assert - .dom('[data-test-choose-file-modal]') + .dom('[data-test-card-chooser-modal]') .exists('the chooser reopens after canceling'); }); @@ -529,6 +544,35 @@ Instructions live in the markdown body. .exists('a different skill remains available in the picker'); }); + test('skill picker excludes an already-attached markdown skill', async function (assert) { + await renderAiAssistantPanel(); + let attachedMarkdownSkillId = `${testRealmURL}realm-sync-skill.md`; + let availableCardSkillId = `${testRealmURL}Skill/example`; + + // The file row carries `id` in its search doc (Phase 1), so the same + // client-built `not: { eq: { id } }` exclusion that hides an attached card + // skill also hides an attached markdown skill from the mixed chooser. + await addSkillToAiAssistant(attachedMarkdownSkillId); + + if ( + !document.querySelector( + '[data-test-skill-menu] [data-test-pill-menu-add-button]', + ) + ) { + await click('[data-test-skill-menu][data-test-pill-menu-button]'); + } + await waitFor('[data-test-skill-menu] [data-test-pill-menu-add-button]'); + await click('[data-test-skill-menu] [data-test-pill-menu-add-button]'); + await waitFor(`[data-test-item-button="${availableCardSkillId}"]`); + + assert + .dom(`[data-test-item-button="${attachedMarkdownSkillId}"]`) + .doesNotExist('the already-attached markdown skill is excluded'); + assert + .dom(`[data-test-item-button="${availableCardSkillId}"]`) + .exists('an unattached skill card remains available'); + }); + test('skill pill menu opens the skill card', async function (assert) { await renderAiAssistantPanel(); diff --git a/packages/host/tests/unit/card-search-query-builder-test.ts b/packages/host/tests/unit/card-search-query-builder-test.ts index 66cca4bea73..a5754fe8455 100644 --- a/packages/host/tests/unit/card-search-query-builder-test.ts +++ b/packages/host/tests/unit/card-search-query-builder-test.ts @@ -189,6 +189,39 @@ module('Unit | card-search/query-builder', function () { }); }); + test('a compound `any` base filter stays intact when types are selected (not collapsed)', function (assert) { + // The skill menu's base filter spans a card type and a file-field + // constraint. Regression: stripping the type branch collapsed the `any` + // to just the `{ on, eq }` branch and promoted it into a required `every` + // clause, so skill cards were excluded (only markdown skills survived). + let authorRef = { + module: 'http://test-realm/test/author' as RealmResourceIdentifier, + name: 'Author', + }; + let markdownRef = { + module: + 'https://cardstack.com/base/markdown-file-def' as RealmResourceIdentifier, + name: 'MarkdownDef', + }; + let baseFilter: Filter = { + any: [{ type: authorRef }, { on: markdownRef, eq: { kind: 'skill' } }], + }; + let query = buildSearchQuery('', SORT_AZ, baseFilter, [ + `${authorRef.module}/${authorRef.name}`, + `${markdownRef.module}/${markdownRef.name}`, + ]); + assert.deepEqual(query, { + filter: { + every: [ + // Kept whole — both branches survive so cards AND files match. + baseFilter, + { any: [{ type: authorRef }, { type: markdownRef }] }, + ], + }, + sort: SORT_AZ.sort, + }); + }); + test('cardsOnly adds no filter anchor — the card scope is pinned on the wire', function (assert) { let query = buildSearchQuery('', SORT_AZ, undefined, undefined, { cardsOnly: true, diff --git a/packages/matrix/tests/skills.spec.ts b/packages/matrix/tests/skills.spec.ts index 80527850d3d..34f8ca672e3 100644 --- a/packages/matrix/tests/skills.spec.ts +++ b/packages/matrix/tests/skills.spec.ts @@ -39,6 +39,10 @@ test.describe('Skills', () => { .click(); } await page.locator('[data-test-pill-menu-add-button]').click(); + // The chooser spans the shared Boxel Skills realm (dozens of skills), so the + // target tile can render below the fold. Narrow to it by URL first so it's + // the visible result before clicking. + await page.locator('[data-test-search-field]').fill(cardId); await page.locator(`[data-test-item-button="${cardId}"]`).click(); await page.locator('[data-test-card-chooser-go-button]').click();