diff --git a/.github/skills/chat-customizations-editor/SKILL.md b/.github/skills/chat-customizations-editor/SKILL.md index 16f1d82da3a..8d4ce8edda1 100644 --- a/.github/skills/chat-customizations-editor/SKILL.md +++ b/.github/skills/chat-customizations-editor/SKILL.md @@ -79,6 +79,49 @@ Each customization type requires its own mock path in `createMockPromptsService` All test data lives in `allFiles` (prompt-based items) and the `mcpWorkspace/UserServers` arrays. Add enough items per category (8+) to invoke scrolling. +### Exercising built-in grouping + +The list widget regroups items from the default chat extension under a "Built-in" header. Three things must be in place for fixtures to exercise this: +1. Include `BUILTIN_STORAGE` in the harness descriptor's visible sources +2. Mock `IProductService.defaultChatAgent.chatExtensionId` (e.g., `'GitHub.copilot-chat'`) +3. Give mock items extension provenance via `extensionId` / `extensionDisplayName` matching that ID + +Without all three, built-in regrouping silently doesn't run and the fixture only shows flat lists. + +### Editor contribution service mocks + +The management editor embeds a `CodeEditorWidget`. Electron-side editor contributions (e.g., `AgentFeedbackEditorWidgetContribution`) are instantiated automatically and crash if their injected services aren't registered. The fixture must mock at minimum: +- `IAgentFeedbackService` — needs `onDidChangeFeedback`, `onDidChangeNavigation` as `Event.None` +- `ICodeReviewService` — needs `getReviewState()` / `getPRReviewState()` returning idle observables +- `IChatEditingService` — needs `editingSessionsObs` as empty observable +- `IAgentSessionsService` — needs `model.sessions` as empty array + +These are cross-layer imports from `vs/sessions/` — use `// eslint-disable-next-line local/code-import-patterns` on the import lines. + +### CI regression gates + +Key fixtures have `blocksCi: true` in their labels. The `screenshot-test.yml` GitHub Action captures screenshots on every PR to `main` and **fails the CI status check** if any `blocks-ci`-labeled fixture's screenshot changes. This catches layout regressions automatically. + +Currently gated fixtures: `LocalHarness`, `McpServersTab`, `McpServersTabNarrow`, `AgentsTabNarrow`. When adding a new section or layout-critical fixture, add `blocksCi: true`: + +```typescript +MyFixture: defineComponentFixture({ + labels: { kind: 'screenshot', blocksCi: true }, + render: ctx => renderEditor(ctx, { ... }), +}), +``` + +Don't add `blocksCi` to every fixture — only ones that cover critical layout paths (default view, section with list + footer, narrow viewport). Too many gated fixtures creates noisy CI. + +### Screenshot stability + +Scrollbar fade transitions cause screenshot instability — the scrollbar shifts from `visible` to `invisible fade` class ~2 seconds after a programmatic scroll. After calling `revealLastItem()` or any scroll action, wait for the transition to complete before the fixture's render promise resolves: + +```typescript +await new Promise(resolve => setTimeout(resolve, 2400)); +// Then optionally poll until .scrollbar.vertical loses the 'visible' class +``` + ### Running unit tests ```bash @@ -87,3 +130,53 @@ npm run compile-check-ts-native && npm run valid-layers-check ``` See the `sessions` skill for sessions-window specific guidance. + +## Debugging Layout in the Real Product + +Component fixtures use mock data and a fixed container size. Layout bugs caused by reflow timing, real data shapes, or narrow window sizes often **don't reproduce in fixtures**. When a user reports a broken layout, debug in the live Code OSS product. + +For launching Code OSS with CDP and connecting `agent-browser`, see the **`launch` skill**. Use `--user-data-dir /tmp/code-oss-debug` to avoid colliding with an already-running instance from another worktree. + +### Navigating to the customizations editor + +After connecting, use `snapshot -i` to find the "Open Customizations" button (in the Chat panel header), then click it. To switch sections, use `eval` with a DOM click since sidebar items aren't interactive refs: + +```bash +npx agent-browser eval "const items = [...document.querySelectorAll('.section-list-item')]; \ + items.find(el => el.textContent?.includes('MCP'))?.click();" +``` + +### Inspecting widget layout + +`agent-browser eval` doesn't always print return values. Use `document.title` as a return channel: + +```bash +npx agent-browser eval "const w = document.querySelector('.mcp-list-widget'); \ + const lc = w?.querySelector('.mcp-list-container'); \ + const rows = lc?.querySelectorAll('.monaco-list-row'); \ + document.title = 'DBG:rows=' + (rows?.length ?? -1) \ + + ',listH=' + (lc?.offsetHeight ?? -1) \ + + ',seStH=' + (lc?.querySelector('.monaco-scrollable-element')?.style?.height ?? '') \ + + ',wH=' + (w?.offsetHeight ?? -1);" +npx agent-browser eval "document.title" 2>&1 +``` + +Key diagnostics: +- **`rows`** — fewer than expected means `list.layout()` never received the correct viewport height. +- **`seStH`** — empty means the list was never properly laid out. +- **`listH` vs `wH`** — list container height should be widget height minus search bar minus footer. + +### Common layout issues + +| Symptom | Root cause | Fix pattern | +|---------|-----------|-------------| +| List shows 0-1 rows in a tall container | `layout()` bailed out because `offsetHeight` returned 0 during `display:none → visible` transition | Defer layout via `DOM.getWindow(this.element).requestAnimationFrame(...)` | +| Badge or row content clips at right edge | Widget container missing `overflow: hidden` | Add `overflow: hidden` to the widget's CSS class | +| Items visible in fixture but not in product | Fixture uses many mock items; real product has few | Add fixture variants with fewer items or narrower dimensions (`width`/`height` options) | + +### Fixture vs real product gaps + +Fixtures render at a fixed size (default 900×600) with many mock items. They won't catch: +- **Reflow timing** — the real product's `display:none → visible` transition may not have reflowed before `layout()` fires +- **Narrow windows** — add narrow fixture variants (e.g., `width: 550, height: 400`) +- **Real data counts** — a user with 1 MCP server sees very different layout than a fixture with 12 diff --git a/src/vs/workbench/contrib/chat/browser/aiCustomization/aiCustomizationListWidget.ts b/src/vs/workbench/contrib/chat/browser/aiCustomization/aiCustomizationListWidget.ts index f0587f7f68c..1c7cb87c835 100644 --- a/src/vs/workbench/contrib/chat/browser/aiCustomization/aiCustomizationListWidget.ts +++ b/src/vs/workbench/contrib/chat/browser/aiCustomization/aiCustomizationListWidget.ts @@ -108,6 +108,10 @@ export interface IAICustomizationListItem { readonly badgeTooltip?: string; /** When set, overrides the default prompt-type icon. */ readonly typeIcon?: ThemeIcon; + /** True when item comes from the default chat extension (grouped under Built-in). */ + readonly isBuiltin?: boolean; + /** Display name of the contributing extension (for non-built-in extension items). */ + readonly extensionLabel?: string; nameMatches?: IMatch[]; descriptionMatches?: IMatch[]; } @@ -266,16 +270,12 @@ function storageToIcon(storage: PromptsStorage): ThemeIcon { } /** - * Formats a name for display: strips a trailing .md extension, converts dashes/underscores - * to spaces and applies title case. - * Note: callers that pass IMatch highlight ranges must compute those ranges against the - * formatted string (not the raw input), since .md stripping changes string length. + * Formats a name for display by stripping a trailing .md extension. + * Names from frontmatter headers are shown as-is to stay consistent + * with how they appear in agent dropdowns and error messages. */ export function formatDisplayName(name: string): string { - return name - .replace(/\.md$/i, '') - .replace(/[-_]/g, ' ') - .replace(/\b\w/g, c => c.toUpperCase()); + return name.replace(/\.md$/i, ''); } /** @@ -334,10 +334,18 @@ class AICustomizationItemRenderer implements IListRenderer { - const uriLabel = this.labelService.getUriLabel(element.uri, { relative: false }); - let content = `${element.name}\n${uriLabel}`; + let content: string; + if (element.isBuiltin) { + content = `${element.name}\n${localize('builtinSource', "Built-in")}`; + } else if (element.extensionLabel) { + content = `${element.name}\n${localize('fromExtension', "Extension: {0}", element.extensionLabel)}`; + } else { + const isWorkspaceItem = element.storage === PromptsStorage.local; + const uriLabel = this.labelService.getUriLabel(element.uri, { relative: isWorkspaceItem }); + content = `${element.name}\n${uriLabel}`; + } if (element.badgeTooltip) { content += `\n\n${element.badgeTooltip}`; } @@ -739,8 +747,8 @@ export class AICustomizationListWidget extends Disposable { const { secondary } = getContextMenuActions(actions, 'inline'); - // Add copy path actions - const copyActions = [ + // Add copy path actions (not shown for built-in items where the path is an implementation detail) + const copyActions = item.isBuiltin ? [] : [ new Separator(), new Action('copyFullPath', localize('copyFullPath', "Copy Full Path"), undefined, true, async () => { await this.clipboardService.writeText(item.uri.fsPath); @@ -1062,24 +1070,6 @@ export class AICustomizationListWidget extends Disposable { return !!chatExtensionId && ExtensionIdentifier.equals(extensionId, chatExtensionId); } - /** - * Resolves the display group key for an extension-storage item. - * Items from the default chat extension are re-grouped under "Built-in"; - * all other extension items keep their original storage as group key. - * - * Returns `undefined` when no override is needed (the item will fall back - * to its `storage` value for grouping). - * - * This is the single point where extension → group mapping is decided, - * making it easy to add dynamic filter layers in the future. - */ - private resolveExtensionGroupKey(extensionId: ExtensionIdentifier | undefined): string | undefined { - if (extensionId && this.isChatExtensionItem(extensionId)) { - return BUILTIN_STORAGE; - } - return undefined; - } - /** * Post-processes items to assign groupKey overrides for extension-sourced * items. Applies the built-in grouping consistently across all item types. @@ -1088,22 +1078,28 @@ export class AICustomizationListWidget extends Disposable { * agent hooks) are left untouched — groupKey overrides are only applied to * items whose current groupKey is `undefined`. */ - private applyBuiltinGroupKeys(items: IAICustomizationListItem[], extensionIdByUri: ReadonlyMap): void { - for (const item of items) { - if (item.groupKey !== undefined) { - continue; // respect explicit groupKey from upstream (e.g. instruction categories) - } + private applyBuiltinGroupKeys(items: IAICustomizationListItem[], extensionInfoByUri: ReadonlyMap): IAICustomizationListItem[] { + return items.map(item => { if (item.storage !== PromptsStorage.extension) { - continue; + return item; } - const extId = extensionIdByUri.get(item.uri.toString()); - const override = this.resolveExtensionGroupKey(extId); - if (override) { - // IAICustomizationListItem.groupKey is readonly for consumers but - // we own the items array here, so the mutation is safe. - (item as { groupKey?: string }).groupKey = override; + const extInfo = extensionInfoByUri.get(item.uri.toString()); + if (!extInfo) { + return item; } - } + const isBuiltin = this.isChatExtensionItem(extInfo.id); + if (isBuiltin) { + return { + ...item, + isBuiltin: true, + groupKey: item.groupKey ?? BUILTIN_STORAGE, + }; + } + return { + ...item, + extensionLabel: extInfo.displayName || extInfo.id.value, + }; + }); } /** @@ -1114,12 +1110,19 @@ export class AICustomizationListWidget extends Disposable { const promptType = sectionToPromptType(section); const items: IAICustomizationListItem[] = []; const disabledUris = this.promptsService.getDisabledPromptFiles(promptType); - const extensionIdByUri = new Map(); + const extensionInfoByUri = new Map(); if (promptType === PromptsType.agent) { // Use getCustomAgents which has parsed name/description from frontmatter const agents = await this.promptsService.getCustomAgents(CancellationToken.None); + // Build extension display name lookup from raw file list + const allAgentFiles = await this.promptsService.listPromptFiles(PromptsType.agent, CancellationToken.None); + for (const file of allAgentFiles) { + if (file.extension) { + extensionInfoByUri.set(file.uri.toString(), { id: file.extension.identifier, displayName: file.extension.displayName }); + } + } for (const agent of agents) { const filename = basename(agent.uri); items.push({ @@ -1133,9 +1136,9 @@ export class AICustomizationListWidget extends Disposable { pluginUri: agent.source.storage === PromptsStorage.plugin ? agent.source.pluginUri : undefined, disabled: disabledUris.has(agent.uri), }); - // Track extension ID for built-in grouping - if (agent.source.storage === PromptsStorage.extension) { - extensionIdByUri.set(agent.uri.toString(), agent.source.extensionId); + // Track extension ID for built-in grouping (if not already set from file list) + if (agent.source.storage === PromptsStorage.extension && !extensionInfoByUri.has(agent.uri.toString())) { + extensionInfoByUri.set(agent.uri.toString(), { id: agent.source.extensionId }); } } } else if (promptType === PromptsType.skill) { @@ -1145,7 +1148,7 @@ export class AICustomizationListWidget extends Disposable { const allSkillFiles = await this.promptsService.listPromptFiles(PromptsType.skill, CancellationToken.None); for (const file of allSkillFiles) { if (file.extension) { - extensionIdByUri.set(file.uri.toString(), file.extension.identifier); + extensionInfoByUri.set(file.uri.toString(), { id: file.extension.identifier, displayName: file.extension.displayName }); } } const seenUris = new ResourceSet(); @@ -1204,7 +1207,7 @@ export class AICustomizationListWidget extends Disposable { disabled: disabledUris.has(command.promptPath.uri), }); if (command.promptPath.extension) { - extensionIdByUri.set(command.promptPath.uri.toString(), command.promptPath.extension.identifier); + extensionInfoByUri.set(command.promptPath.uri.toString(), { id: command.promptPath.extension.identifier, displayName: command.promptPath.extension.displayName }); } } } else if (promptType === PromptsType.hook) { @@ -1314,7 +1317,7 @@ export class AICustomizationListWidget extends Disposable { const promptFiles = await this.promptsService.listPromptFiles(promptType, CancellationToken.None); for (const file of promptFiles) { if (file.extension) { - extensionIdByUri.set(file.uri.toString(), file.extension.identifier); + extensionInfoByUri.set(file.uri.toString(), { id: file.extension.identifier, displayName: file.extension.displayName }); } } const agentInstructionFiles = await this.promptsService.listAgentInstructions(CancellationToken.None, undefined); @@ -1410,11 +1413,11 @@ export class AICustomizationListWidget extends Disposable { // are re-grouped under "Built-in" instead of "Extensions". // This is a single-pass transformation applied after all items are // collected, keeping the item-building code free of grouping logic. - this.applyBuiltinGroupKeys(items, extensionIdByUri); + const groupedItems = this.applyBuiltinGroupKeys(items, extensionInfoByUri); // Apply storage source filter (removes items not in visible sources or excluded user roots) const filter = this.workspaceService.getStorageSourceFilter(promptType); - const filteredItems = applyStorageSourceFilter(items, filter); + const filteredItems = applyStorageSourceFilter(groupedItems, filter); items.length = 0; items.push(...filteredItems); @@ -1706,32 +1709,36 @@ export class AICustomizationListWidget extends Disposable { } } + /** + * Scrolls the list so the last item is visible. + */ + revealLastItem(): void { + if (this.displayEntries.length > 0) { + this.list.reveal(this.displayEntries.length - 1); + } + } + /** * Layouts the widget. */ layout(height: number, width: number): void { - const sectionFooterHeight = this.sectionHeader.offsetHeight || 0; - const searchBarHeight = this.searchAndButtonContainer.offsetHeight || 52; - const listHeight = height - sectionFooterHeight - searchBarHeight; - + this.element.style.height = `${height}px`; this.searchInput.layout(); - this.listContainer.style.height = `${Math.max(0, listHeight)}px`; - this.list.layout(Math.max(0, listHeight), width); - // Re-layout once after footer renders if we used a zero fallback - if (sectionFooterHeight === 0) { - DOM.getWindow(this.listContainer).requestAnimationFrame(() => { - if (this._store.isDisposed) { - return; - } - const actualFooterHeight = this.sectionHeader.offsetHeight; - if (actualFooterHeight > 0) { - const correctedHeight = height - actualFooterHeight - searchBarHeight; - this.listContainer.style.height = `${Math.max(0, correctedHeight)}px`; - this.list.layout(Math.max(0, correctedHeight), width); - } - }); + // Measure sibling elements to calculate the remaining space for the list. + // When offsetHeight returns 0 the container just became visible + // after display:none and the browser hasn't reflowed yet — defer + // layout to the next frame so measurements are accurate. + const searchBarHeight = this.searchAndButtonContainer.offsetHeight; + if (searchBarHeight === 0) { + DOM.getWindow(this.element).requestAnimationFrame(() => this.layout(height, width)); + return; } + const footerHeight = this.sectionHeader.offsetHeight; + const listHeight = Math.max(0, height - searchBarHeight - footerHeight); + + this.listContainer.style.height = `${listHeight}px`; + this.list.layout(listHeight, width); } /** diff --git a/src/vs/workbench/contrib/chat/browser/aiCustomization/aiCustomizationManagementEditor.ts b/src/vs/workbench/contrib/chat/browser/aiCustomization/aiCustomizationManagementEditor.ts index 2896cbbef08..f5de7aec226 100644 --- a/src/vs/workbench/contrib/chat/browser/aiCustomization/aiCustomizationManagementEditor.ts +++ b/src/vs/workbench/contrib/chat/browser/aiCustomization/aiCustomizationManagementEditor.ts @@ -1281,6 +1281,11 @@ export class AICustomizationManagementEditor extends EditorPane { if (this.isPromptsSection(sectionId)) { void this.listWidget.setSection(sectionId); } + // Re-layout after visibility change so the newly-visible widget + // can measure its flex-computed container height correctly. + if (this.dimension) { + this.layout(this.dimension); + } this.ensureSectionsListReflectsActiveSection(sectionId); } } @@ -1292,6 +1297,19 @@ export class AICustomizationManagementEditor extends EditorPane { void this.listWidget.refresh(); } + /** + * Scrolls the active list widget so the last item is visible. + */ + public revealLastItem(): void { + if (this.selectedSection === AICustomizationManagementSection.McpServers) { + this.mcpListWidget?.revealLastItem(); + } else if (this.selectedSection === AICustomizationManagementSection.Plugins) { + this.pluginListWidget?.revealLastItem(); + } else { + this.listWidget.revealLastItem(); + } + } + /** * Generates a debug report for the current section. */ diff --git a/src/vs/workbench/contrib/chat/browser/aiCustomization/mcpListWidget.ts b/src/vs/workbench/contrib/chat/browser/aiCustomization/mcpListWidget.ts index a4b197e51e1..23fea69dcde 100644 --- a/src/vs/workbench/contrib/chat/browser/aiCustomization/mcpListWidget.ts +++ b/src/vs/workbench/contrib/chat/browser/aiCustomization/mcpListWidget.ts @@ -891,28 +891,24 @@ export class McpListWidget extends Disposable { layout(height: number, width: number): void { this.lastHeight = height; this.lastWidth = width; - const sectionFooterHeight = this.sectionHeader.offsetHeight || 0; - const searchBarHeight = this.searchAndButtonContainer.offsetHeight || 52; - const backLinkHeight = this.browseMode ? (this.backLink.offsetHeight || 28) : 0; - const listHeight = height - sectionFooterHeight - searchBarHeight - backLinkHeight; - this.listContainer.style.height = `${Math.max(0, listHeight)}px`; - this.list.layout(Math.max(0, listHeight), width); + this.element.style.height = `${height}px`; - // Re-layout once after footer renders if we used a zero fallback - if (sectionFooterHeight === 0) { - DOM.getWindow(this.listContainer).requestAnimationFrame(() => { - if (this._store.isDisposed) { - return; - } - const actualFooterHeight = this.sectionHeader.offsetHeight; - if (actualFooterHeight > 0) { - const correctedHeight = height - actualFooterHeight - searchBarHeight - backLinkHeight; - this.listContainer.style.height = `${Math.max(0, correctedHeight)}px`; - this.list.layout(Math.max(0, correctedHeight), width); - } - }); + // Measure sibling elements to calculate the list height. + // When offsetHeight returns 0 the container just became visible + // after display:none and the browser hasn't reflowed yet — defer + // layout to the next frame so measurements are accurate. + const searchBarHeight = this.searchAndButtonContainer.offsetHeight; + if (searchBarHeight === 0) { + DOM.getWindow(this.element).requestAnimationFrame(() => this.layout(this.lastHeight, this.lastWidth)); + return; } + const footerHeight = this.sectionHeader.offsetHeight; + const backLinkHeight = this.browseMode ? this.backLink.offsetHeight : 0; + const listHeight = Math.max(0, height - searchBarHeight - footerHeight - backLinkHeight); + + this.listContainer.style.height = `${listHeight}px`; + this.list.layout(listHeight, width); } /** @@ -922,6 +918,15 @@ export class McpListWidget extends Disposable { this.searchInput.focus(); } + /** + * Scrolls the list so the last item is visible. + */ + revealLastItem(): void { + if (this.list.length > 0) { + this.list.reveal(this.list.length - 1); + } + } + /** * Focuses the list. */ diff --git a/src/vs/workbench/contrib/chat/browser/aiCustomization/media/aiCustomizationManagement.css b/src/vs/workbench/contrib/chat/browser/aiCustomization/media/aiCustomizationManagement.css index b4e17ae435e..4c6fd79b107 100644 --- a/src/vs/workbench/contrib/chat/browser/aiCustomization/media/aiCustomizationManagement.css +++ b/src/vs/workbench/contrib/chat/browser/aiCustomization/media/aiCustomizationManagement.css @@ -168,6 +168,7 @@ display: flex; flex-direction: column; height: 100%; + overflow: hidden; } /* Harness dropdown in sidebar */ @@ -719,6 +720,7 @@ display: flex; flex-direction: column; height: 100%; + overflow: hidden; } .mcp-list-widget .section-footer { diff --git a/src/vs/workbench/contrib/chat/browser/aiCustomization/pluginListWidget.ts b/src/vs/workbench/contrib/chat/browser/aiCustomization/pluginListWidget.ts index a51a9f6fb0e..28ec72c3c1f 100644 --- a/src/vs/workbench/contrib/chat/browser/aiCustomization/pluginListWidget.ts +++ b/src/vs/workbench/contrib/chat/browser/aiCustomization/pluginListWidget.ts @@ -710,33 +710,36 @@ export class PluginListWidget extends Disposable { layout(height: number, width: number): void { this.lastHeight = height; this.lastWidth = width; - const sectionFooterHeight = this.sectionHeader.offsetHeight || 0; - const searchBarHeight = this.searchAndButtonContainer.offsetHeight || 52; - const backLinkHeight = this.browseMode ? (this.backLink.offsetHeight || 28) : 0; - const listHeight = height - sectionFooterHeight - searchBarHeight - backLinkHeight; - this.listContainer.style.height = `${Math.max(0, listHeight)}px`; - this.list.layout(Math.max(0, listHeight), width); + this.element.style.height = `${height}px`; - if (sectionFooterHeight === 0) { - DOM.getWindow(this.listContainer).requestAnimationFrame(() => { - if (this._store.isDisposed) { - return; - } - const actualFooterHeight = this.sectionHeader.offsetHeight; - if (actualFooterHeight > 0) { - const correctedHeight = height - actualFooterHeight - searchBarHeight - backLinkHeight; - this.listContainer.style.height = `${Math.max(0, correctedHeight)}px`; - this.list.layout(Math.max(0, correctedHeight), width); - } - }); + // Measure sibling elements to calculate the list height. + // When offsetHeight returns 0 the container just became visible + // after display:none and the browser hasn't reflowed yet — defer + // layout to the next frame so measurements are accurate. + const searchBarHeight = this.searchAndButtonContainer.offsetHeight; + if (searchBarHeight === 0) { + DOM.getWindow(this.element).requestAnimationFrame(() => this.layout(this.lastHeight, this.lastWidth)); + return; } + const footerHeight = this.sectionHeader.offsetHeight; + const backLinkHeight = this.browseMode ? this.backLink.offsetHeight : 0; + const listHeight = Math.max(0, height - searchBarHeight - footerHeight - backLinkHeight); + + this.listContainer.style.height = `${listHeight}px`; + this.list.layout(listHeight, width); } focusSearch(): void { this.searchInput.focus(); } + revealLastItem(): void { + if (this.list.length > 0) { + this.list.reveal(this.list.length - 1); + } + } + focus(): void { this.list.domFocus(); if (this.list.length > 0) { diff --git a/src/vs/workbench/contrib/chat/common/customizationHarnessService.ts b/src/vs/workbench/contrib/chat/common/customizationHarnessService.ts index 0c368744fe1..31db34d4dbc 100644 --- a/src/vs/workbench/contrib/chat/common/customizationHarnessService.ts +++ b/src/vs/workbench/contrib/chat/common/customizationHarnessService.ts @@ -163,10 +163,10 @@ export interface ICustomizationHarnessService { // #region Shared filter constants /** - * Hooks are always restricted to local + plugin sources regardless of harness. + * Hooks filter — local, user, and plugin sources. */ const HOOKS_FILTER: IStorageSourceFilter = { - sources: [PromptsStorage.local, PromptsStorage.plugin], + sources: [PromptsStorage.local, PromptsStorage.user, PromptsStorage.plugin], }; // #endregion diff --git a/src/vs/workbench/test/browser/componentFixtures/aiCustomizationManagementEditor.fixture.ts b/src/vs/workbench/test/browser/componentFixtures/aiCustomizationManagementEditor.fixture.ts index 465246d562f..737a184ee5b 100644 --- a/src/vs/workbench/test/browser/componentFixtures/aiCustomizationManagementEditor.fixture.ts +++ b/src/vs/workbench/test/browser/componentFixtures/aiCustomizationManagementEditor.fixture.ts @@ -24,6 +24,7 @@ import { IWorkspace, IWorkspaceContextService, WorkbenchState } from '../../../. import { IEditorGroup } from '../../../services/editor/common/editorGroupsService.js'; import { IExtensionService } from '../../../services/extensions/common/extensions.js'; import { IProductService } from '../../../../platform/product/common/productService.js'; +import { ExtensionIdentifier } from '../../../../platform/extensions/common/extensions.js'; import { IPathService } from '../../../services/path/common/pathService.js'; import { IWorkingCopyService } from '../../../services/workingCopy/common/workingCopyService.js'; import { IWebviewService } from '../../../contrib/webview/browser/webview.js'; @@ -45,6 +46,12 @@ import { IWorkbenchLocalMcpServer, LocalMcpServerScope } from '../../../services import { McpListWidget } from '../../../contrib/chat/browser/aiCustomization/mcpListWidget.js'; import { PluginListWidget } from '../../../contrib/chat/browser/aiCustomization/pluginListWidget.js'; import { IIterativePager } from '../../../../base/common/paging.js'; +// eslint-disable-next-line local/code-import-patterns +import { IAgentFeedbackService } from '../../../../sessions/contrib/agentFeedback/browser/agentFeedbackService.js'; +// eslint-disable-next-line local/code-import-patterns +import { CodeReviewStateKind, ICodeReviewService, ICodeReviewState, IPRReviewState, PRReviewStateKind } from '../../../../sessions/contrib/codeReview/browser/codeReviewService.js'; +import { IChatEditingService } from '../../../contrib/chat/common/editing/chatEditingService.js'; +import { IAgentSessionsService } from '../../../contrib/chat/browser/agentSessions/agentSessionsService.js'; import { ComponentFixtureContext, createEditorServices, defineComponentFixture, defineThemedFixtureGroup, registerWorkbenchServices } from './fixtureUtils.js'; // Ensure theme colors & widget CSS are loaded @@ -66,6 +73,8 @@ interface IFixtureFile { readonly name?: string; readonly description?: string; readonly applyTo?: string; + readonly extensionId?: string; + readonly extensionDisplayName?: string; } function createMockEditorGroup(): IEditorGroup { @@ -74,6 +83,17 @@ function createMockEditorGroup(): IEditorGroup { }(); } +function toExtensionInfo(file: IFixtureFile): { identifier: ExtensionIdentifier; displayName?: string } | undefined { + if (!file.extensionId) { + return undefined; + } + + return { + identifier: new ExtensionIdentifier(file.extensionId), + displayName: file.extensionDisplayName, + }; +} + function createMockPromptsService(files: IFixtureFile[], agentInstructions: IResolvedAgentFile[]): IPromptsService { const applyToMap = new ResourceMap(); const descriptionMap = new ResourceMap(); @@ -84,16 +104,24 @@ function createMockPromptsService(files: IFixtureFile[], agentInstructions: IRes override readonly onDidChangeSkills = Event.None; override readonly onDidChangeInstructions = Event.None; override getDisabledPromptFiles(): ResourceSet { return new ResourceSet(); } - override async listPromptFiles(type: PromptsType) { + override async listPromptFiles(type: PromptsType, _token: CancellationToken) { return files.filter(f => f.type === type).map(f => ({ - uri: f.uri, storage: f.storage as PromptsStorage.local, type: f.type, name: f.name, description: f.description, + uri: f.uri, + storage: f.storage as PromptsStorage.local, + type: f.type, + name: f.name, + description: f.description, + extension: toExtensionInfo(f) as never, })); } override async listAgentInstructions() { return agentInstructions; } override async getCustomAgents() { return files.filter(f => f.type === PromptsType.agent).map(a => ({ uri: a.uri, name: a.name ?? 'agent', description: a.description, storage: a.storage, - source: { storage: a.storage }, + source: { + storage: a.storage, + extensionId: a.extensionId ? new ExtensionIdentifier(a.extensionId) : undefined, + }, })) as never[]; } override async parseNew(uri: URI, _token: CancellationToken): Promise { @@ -118,7 +146,7 @@ function createMockPromptsService(files: IFixtureFile[], agentInstructions: IRes override async getPromptSlashCommands(): Promise { const promptFiles = files.filter(f => f.type === PromptsType.prompt); const commands = await Promise.all(promptFiles.map(async f => { - const promptPath = { uri: f.uri, storage: f.storage, type: f.type }; + const promptPath = { uri: f.uri, storage: f.storage, type: f.type, extension: toExtensionInfo(f) as never }; const parsedPromptFile = await this.parseNew(f.uri, CancellationToken.None); return { name: f.name ?? 'prompt', @@ -164,11 +192,55 @@ function makeLocalMcpServer(id: string, label: string, scope: LocalMcpServerScop }(); } +function createMockAgentFeedbackService(): IAgentFeedbackService { + return new class extends mock() { + override readonly onDidChangeFeedback = Event.None; + override readonly onDidChangeNavigation = Event.None; + override getFeedback() { return []; } + override getMostRecentSessionForResource() { return undefined; } + override async revealFeedback(): Promise { } + override getNextFeedback() { return undefined; } + override getNavigationBearing() { return { activeIdx: -1, totalCount: 0 }; } + override getNextNavigableItem() { return undefined; } + override setNavigationAnchor(): void { } + override clearFeedback(): void { } + override removeFeedback(): void { } + override async addFeedbackAndSubmit(): Promise { } + }(); +} + +function createMockCodeReviewService(): ICodeReviewService { + return new class extends mock() { + private readonly reviewState = observableValue('fixture.reviewState', { kind: CodeReviewStateKind.Idle }); + private readonly prReviewState = observableValue('fixture.prReviewState', { kind: PRReviewStateKind.None }); + + override getReviewState() { + return this.reviewState; + } + + override getPRReviewState() { + return this.prReviewState; + } + + override hasReview(): boolean { + return false; + } + + override requestReview(): void { } + override removeComment(): void { } + override dismissReview(): void { } + override async resolvePRReviewThread(): Promise { } + }(); +} + // ============================================================================ // Realistic test data — a project that has Copilot + Claude customizations // ============================================================================ const allFiles: IFixtureFile[] = [ + // Instructions - extension (built-in + third-party) + { uri: URI.file('/extensions/github.copilot-chat/instructions/coding.instructions.md'), storage: PromptsStorage.extension, type: PromptsType.instructions, name: 'Copilot Coding', description: 'Built-in coding guidance', extensionId: 'GitHub.copilot-chat', extensionDisplayName: 'GitHub Copilot Chat' }, + { uri: URI.file('/extensions/acme.tools/instructions/team.instructions.md'), storage: PromptsStorage.extension, type: PromptsType.instructions, name: 'Team Conventions', description: 'Third-party extension instructions', extensionId: 'acme.tools', extensionDisplayName: 'Acme Tools' }, // Instructions — workspace { uri: URI.file('/workspace/.github/instructions/coding-standards.instructions.md'), storage: PromptsStorage.local, type: PromptsType.instructions, name: 'Coding Standards', description: 'Repository-wide coding standards' }, { uri: URI.file('/workspace/.github/instructions/testing.instructions.md'), storage: PromptsStorage.local, type: PromptsType.instructions, name: 'Testing', description: 'Testing best practices', applyTo: '**/*.test.ts' }, @@ -198,6 +270,9 @@ const allFiles: IFixtureFile[] = [ { uri: URI.file('/home/dev/.copilot/agents/planner.agent.md'), storage: PromptsStorage.user, type: PromptsType.agent, name: 'Planner', description: 'Project planning agent' }, { uri: URI.file('/home/dev/.copilot/agents/debugger.agent.md'), storage: PromptsStorage.user, type: PromptsType.agent, name: 'Debugger', description: 'Interactive debugging assistant' }, { uri: URI.file('/home/dev/.copilot/agents/nls-helper.agent.md'), storage: PromptsStorage.user, type: PromptsType.agent, name: 'NLS Helper', description: 'Natural language searching code for clarity' }, + // Agents - extension (built-in + third-party) + { uri: URI.file('/extensions/github.copilot-chat/agents/workspace-guide.agent.md'), storage: PromptsStorage.extension, type: PromptsType.agent, name: 'Workspace Guide', description: 'Built-in workspace exploration agent', extensionId: 'GitHub.copilot-chat', extensionDisplayName: 'GitHub Copilot Chat' }, + { uri: URI.file('/extensions/acme.tools/agents/api-helper.agent.md'), storage: PromptsStorage.extension, type: PromptsType.agent, name: 'API Helper', description: 'Third-party API agent', extensionId: 'acme.tools', extensionDisplayName: 'Acme Tools' }, // Skills — workspace { uri: URI.file('/workspace/.github/skills/deploy/SKILL.md'), storage: PromptsStorage.local, type: PromptsType.skill, name: 'Deploy', description: 'Deployment automation' }, { uri: URI.file('/workspace/.github/skills/refactor/SKILL.md'), storage: PromptsStorage.local, type: PromptsType.skill, name: 'Refactor', description: 'Code refactoring patterns' }, @@ -210,6 +285,9 @@ const allFiles: IFixtureFile[] = [ // Skills — user { uri: URI.file('/home/dev/.copilot/skills/git-workflow/SKILL.md'), storage: PromptsStorage.user, type: PromptsType.skill, name: 'Git Workflow', description: 'Branch and PR workflows' }, { uri: URI.file('/home/dev/.copilot/skills/code-review/SKILL.md'), storage: PromptsStorage.user, type: PromptsType.skill, name: 'Code Review', description: 'Structured code review checklist' }, + // Skills - extension (built-in + third-party) + { uri: URI.file('/extensions/github.copilot-chat/skills/workspace/SKILL.md'), storage: PromptsStorage.extension, type: PromptsType.skill, name: 'Workspace Search', description: 'Built-in workspace search skill', extensionId: 'GitHub.copilot-chat', extensionDisplayName: 'GitHub Copilot Chat' }, + { uri: URI.file('/extensions/acme.tools/skills/audit/SKILL.md'), storage: PromptsStorage.extension, type: PromptsType.skill, name: 'Audit', description: 'Third-party audit skill', extensionId: 'acme.tools', extensionDisplayName: 'Acme Tools' }, // Prompts — workspace { uri: URI.file('/workspace/.github/prompts/explain.prompt.md'), storage: PromptsStorage.local, type: PromptsType.prompt, name: 'Explain', description: 'Explain selected code' }, { uri: URI.file('/workspace/.github/prompts/review.prompt.md'), storage: PromptsStorage.local, type: PromptsType.prompt, name: 'Review', description: 'Review changes' }, @@ -222,6 +300,9 @@ const allFiles: IFixtureFile[] = [ // Prompts — user { uri: URI.file('/home/dev/.copilot/prompts/translate.prompt.md'), storage: PromptsStorage.user, type: PromptsType.prompt, name: 'Translate', description: 'Translate strings for i18n' }, { uri: URI.file('/home/dev/.copilot/prompts/commit-msg.prompt.md'), storage: PromptsStorage.user, type: PromptsType.prompt, name: 'Commit Message', description: 'Generate conventional commit' }, + // Prompts - extension (built-in + third-party) + { uri: URI.file('/extensions/github.copilot-chat/prompts/trace.prompt.md'), storage: PromptsStorage.extension, type: PromptsType.prompt, name: 'Trace', description: 'Built-in tracing prompt', extensionId: 'GitHub.copilot-chat', extensionDisplayName: 'GitHub Copilot Chat' }, + { uri: URI.file('/extensions/acme.tools/prompts/lint.prompt.md'), storage: PromptsStorage.extension, type: PromptsType.prompt, name: 'Lint', description: 'Third-party lint prompt', extensionId: 'acme.tools', extensionDisplayName: 'Acme Tools' }, // Hooks — workspace { uri: URI.file('/workspace/.github/hooks/pre-commit.json'), storage: PromptsStorage.local, type: PromptsType.hook, name: 'Pre-Commit Lint', description: 'Run linting before commit' }, { uri: URI.file('/workspace/.github/hooks/post-save.json'), storage: PromptsStorage.local, type: PromptsType.hook, name: 'Post-Save Format', description: 'Auto-format on save' }, @@ -267,6 +348,66 @@ interface IRenderEditorOptions { readonly managementSections?: readonly AICustomizationManagementSection[]; readonly availableHarnesses?: readonly IHarnessDescriptor[]; readonly selectedSection?: AICustomizationManagementSection; + readonly scrollToBottom?: boolean; + readonly width?: number; + readonly height?: number; +} + +async function waitForAnimationFrames(count: number): Promise { + for (let i = 0; i < count; i++) { + await new Promise(resolve => mainWindow.requestAnimationFrame(() => resolve())); + } +} + +function getVisibleEditorSignature(container: HTMLElement): string { + const sectionCounts = [...container.querySelectorAll('.section-list-item')].map(item => item.textContent?.replace(/\s+/g, ' ').trim() ?? '').join('|'); + const visibleContent = [...container.querySelectorAll('.prompts-content-container, .mcp-content-container, .plugin-content-container')] + .find(node => node instanceof HTMLElement && node.style.display !== 'none'); + const visibleRows = visibleContent + ? [...visibleContent.querySelectorAll('.monaco-list-row')].map(row => row.textContent?.replace(/\s+/g, ' ').trim() ?? '').join('|') + : ''; + + return `${sectionCounts}@@${visibleRows}`; +} + +async function waitForEditorToSettle(container: HTMLElement): Promise { + let previousSignature = ''; + let stableIterations = 0; + + await new Promise(resolve => setTimeout(resolve, 150)); + + for (let i = 0; i < 20; i++) { + await waitForAnimationFrames(2); + await new Promise(resolve => setTimeout(resolve, 25)); + + const signature = getVisibleEditorSignature(container); + if (signature && signature === previousSignature) { + stableIterations++; + if (stableIterations >= 2) { + return; + } + } else { + stableIterations = 0; + previousSignature = signature; + } + } +} + +async function waitForVisibleScrollbarsToFade(container: HTMLElement): Promise { + const deadline = Date.now() + 4000; + + while (Date.now() < deadline) { + const hasVisibleScrollbar = [...container.querySelectorAll('.scrollbar.vertical')].some(scrollbar => { + const style = mainWindow.getComputedStyle(scrollbar); + return scrollbar.classList.contains('visible') && style.opacity !== '0'; + }); + + if (!hasVisibleScrollbar) { + return; + } + + await new Promise(resolve => setTimeout(resolve, 100)); + } } // ============================================================================ @@ -274,8 +415,8 @@ interface IRenderEditorOptions { // ============================================================================ async function renderEditor(ctx: ComponentFixtureContext, options: IRenderEditorOptions): Promise { - const width = 900; - const height = 600; + const width = options.width ?? 900; + const height = options.height ?? 600; ctx.container.style.width = `${width}px`; ctx.container.style.height = `${height}px`; @@ -290,7 +431,7 @@ async function renderEditor(ctx: ComponentFixtureContext, options: IRenderEditor AICustomizationManagementSection.Plugins, ]; const availableHarnesses = options.availableHarnesses ?? [ - createVSCodeHarnessDescriptor([PromptsStorage.extension]), + createVSCodeHarnessDescriptor([PromptsStorage.extension, BUILTIN_STORAGE]), createCliHarnessDescriptor(getCliUserRoots(userHome), []), createClaudeHarnessDescriptor(getClaudeUserRoots(userHome), []), ]; @@ -301,8 +442,21 @@ async function renderEditor(ctx: ComponentFixtureContext, options: IRenderEditor colorTheme: ctx.theme, additionalServices: (reg) => { const harnessService = createMockHarnessService(options.harness, availableHarnesses); + const agentFeedbackService = createMockAgentFeedbackService(); + const codeReviewService = createMockCodeReviewService(); registerWorkbenchServices(reg); reg.define(IListService, ListService); + reg.defineInstance(IAgentFeedbackService, agentFeedbackService); + reg.defineInstance(ICodeReviewService, codeReviewService); + reg.defineInstance(IChatEditingService, new class extends mock() { + override readonly editingSessionsObs = constObservable([]); + }()); + reg.defineInstance(IAgentSessionsService, new class extends mock() { + override readonly model = new class extends mock() { + override readonly sessions = []; + }(); + override getSession() { return undefined; } + }()); reg.defineInstance(IPromptsService, createMockPromptsService(allFiles, agentInstructions)); reg.defineInstance(IAICustomizationWorkspaceService, new class extends mock() { override readonly isSessionsWindow = isSessionsWindow; @@ -372,7 +526,11 @@ async function renderEditor(ctx: ComponentFixtureContext, options: IRenderEditor override readonly onDidChangeMarketplaces = Event.None; }()); reg.defineInstance(IPluginInstallService, new class extends mock() { }()); - reg.defineInstance(IProductService, new class extends mock() { }()); + reg.defineInstance(IProductService, new class extends mock() { + override readonly defaultChatAgent = new class extends mock>() { + override readonly chatExtensionId = 'GitHub.copilot-chat'; + }(); + }()); }, }); @@ -382,16 +540,19 @@ async function renderEditor(ctx: ComponentFixtureContext, options: IRenderEditor editor.create(ctx.container); editor.layout(new Dimension(width, height)); - // setInput may fail on unmocked service calls — catch to still show the editor shell - try { - await editor.setInput(AICustomizationManagementEditorInput.getOrCreate(), undefined, {}, CancellationToken.None); - } catch { - // Expected in fixture — some services are partially mocked - } + await editor.setInput(AICustomizationManagementEditorInput.getOrCreate(), undefined, {}, CancellationToken.None); if (options.selectedSection) { editor.selectSectionById(options.selectedSection); - editor.layout(new Dimension(width, height)); + } + + await waitForEditorToSettle(ctx.container); + + if (options.scrollToBottom) { + editor.revealLastItem(); + await waitForAnimationFrames(2); + await new Promise(resolve => setTimeout(resolve, 2400)); + await waitForVisibleScrollbarsToFade(ctx.container); } } @@ -475,7 +636,7 @@ async function renderMcpBrowseMode(ctx: ComponentFixtureContext): Promise }()); reg.defineInstance(ICustomizationHarnessService, new class extends mock() { override readonly activeHarness = observableValue('activeHarness', CustomizationHarness.VSCode); - override getActiveDescriptor() { return createVSCodeHarnessDescriptor([PromptsStorage.extension]); } + override getActiveDescriptor() { return createVSCodeHarnessDescriptor([PromptsStorage.extension, BUILTIN_STORAGE]); } }()); }, }); @@ -603,7 +764,7 @@ export default defineThemedFixtureGroup({ path: 'chat/aiCustomizations/' }, { // Full editor with Local (VS Code) harness — all sections visible, harness dropdown, // Generate buttons, AGENTS.md shortcut, all storage groups LocalHarness: defineComponentFixture({ - labels: { kind: 'screenshot' }, + labels: { kind: 'screenshot', blocksCi: true }, render: ctx => renderEditor(ctx, { harness: CustomizationHarness.VSCode }), }), @@ -645,7 +806,7 @@ export default defineThemedFixtureGroup({ path: 'chat/aiCustomizations/' }, { // MCP Servers tab with many servers to verify scrollable list layout McpServersTab: defineComponentFixture({ - labels: { kind: 'screenshot' }, + labels: { kind: 'screenshot', blocksCi: true }, render: ctx => renderEditor(ctx, { harness: CustomizationHarness.VSCode, selectedSection: AICustomizationManagementSection.McpServers, @@ -718,4 +879,53 @@ export default defineThemedFixtureGroup({ path: 'chat/aiCustomizations/' }, { labels: { kind: 'screenshot' }, render: renderPluginBrowseMode, }), + + // Scrolled-to-bottom variants — verify last items are fully visible above footer + PromptsTabScrolled: defineComponentFixture({ + labels: { kind: 'screenshot' }, + render: ctx => renderEditor(ctx, { + harness: CustomizationHarness.VSCode, + selectedSection: AICustomizationManagementSection.Prompts, + scrollToBottom: true, + }), + }), + + McpServersTabScrolled: defineComponentFixture({ + labels: { kind: 'screenshot' }, + render: ctx => renderEditor(ctx, { + harness: CustomizationHarness.VSCode, + selectedSection: AICustomizationManagementSection.McpServers, + scrollToBottom: true, + }), + }), + + PluginsTabScrolled: defineComponentFixture({ + labels: { kind: 'screenshot' }, + render: ctx => renderEditor(ctx, { + harness: CustomizationHarness.VSCode, + selectedSection: AICustomizationManagementSection.Plugins, + scrollToBottom: true, + }), + }), + + // Narrow viewport — catches badge clipping and layout overflow at small sizes + McpServersTabNarrow: defineComponentFixture({ + labels: { kind: 'screenshot', blocksCi: true }, + render: ctx => renderEditor(ctx, { + harness: CustomizationHarness.VSCode, + selectedSection: AICustomizationManagementSection.McpServers, + width: 550, + height: 400, + }), + }), + + AgentsTabNarrow: defineComponentFixture({ + labels: { kind: 'screenshot', blocksCi: true }, + render: ctx => renderEditor(ctx, { + harness: CustomizationHarness.VSCode, + selectedSection: AICustomizationManagementSection.Agents, + width: 550, + height: 400, + }), + }), });