chat customizations: improve agentic engineering dev loop (#304305)

* fix #304030: show custom agent names as-is without title-casing

formatDisplayName was converting dashes/underscores to spaces and
applying title case (e.g. nls-code-helper → Nls Code Helper). This
made names inconsistent between the customization view and the agent
dropdown. Now only strips .md extension, showing names exactly as
defined in frontmatter headers.

* fix #304131: show workspace-relative paths in customization tooltips

Workspace items now show paths relative to the project root (e.g.
.github/instructions/coding-standards.instructions.md) instead of
absolute paths (/Users/.../workspace/.github/...).

* fix #304133: show extension name instead of full path in tooltips

Extension items now show 'Extension: github.copilot-chat' in the
tooltip instead of the full filesystem path into the extension
directory. The extensionLabel is populated from the extensionIdByUri
map during item post-processing.

* fix #304178: use save icon instead of ambiguous checkmark

The editor save indicator now uses Codicon.save (floppy disk) instead
of Codicon.check (bare checkmark), making it clear that the indicator
is about save status rather than an enable/disable toggle.

* fix #304183: show user-scoped hooks in sessions window

HOOKS_FILTER excluded PromptsStorage.user, so hooks in
~/.copilot/hooks/ were filtered out in the sessions window (and
any harness using the restricted filter). User hooks are now
supported in CLI, so add user storage to the allowed sources.

* fix: improve list layout footer fallback and remove extension tooltip

- Use 80px footer height fallback instead of 0 when sectionHeader
  has not rendered yet, preventing list from overlapping the footer
  on first paint
- Wait for async section load in fixture before final layout
- Remove extensionLabel tooltip (implementation detail, not relevant
  to users)

* fix #304178: show loading spinner before saved icon, remove green color

The save indicator now shows a spinning loading animation while the
file is being saved (onDidChangeDirty dirty→clean transition), then
switches to the save icon when the save completes (onDidSave). The
special green color was removed to use the default description
foreground color instead.

* fix: simplify list layout to use CSS flex instead of manual height calculation

All three list widgets (AICustomizationListWidget, McpListWidget,
PluginListWidget) now rely on CSS flex layout to distribute space
between search bar, list container, and footer. The layout() method
sets the widget height, clears any manual list container height,
and reads clientHeight (forcing a synchronous reflow) to get the
flex-computed height for the WorkbenchList.

This eliminates hardcoded fallback values, requestAnimationFrame
correction loops, and manual offsetHeight arithmetic that caused
the list to overlap the footer on first render.

* revert: drop save indicator icon changes (#304178)

Reverts the checkmark→save icon change, the loading spinner, and the
green color removal. The save indicator is back to the original
Codicon.check with green color — this will be addressed separately.

* fix: show Built-in/Extension name in tooltips, fix MCP scroll, hide copy path for built-in

- Built-in items show 'Built-in' in tooltip instead of file path
- Non-built-in extension items show 'Extension: {displayName}'
- Copy path context menu hidden for built-in items
- Fix MCP/Plugin scroll: fall back to passed height when container
  is hidden (display:none → clientHeight=0), and re-layout when
  section becomes visible via selectSectionById
- Remove unused resolveExtensionGroupKey method

* fix: use explicit height calculation for list layout instead of flex clientHeight

The flex-based clientHeight approach was unreliable — reflow timing
varied between the component explorer and the real product, causing
the list to clip behind the footer at certain window sizes.

Reverts to explicit height calculation by measuring sibling elements
(search bar, footer, back link) via offsetHeight and subtracting
from total height. For hidden widgets (MCP/Plugin when not the
active tab), layout is skipped since offsetHeight returns 0; the
selectSectionById re-layout handles showing them later.

Removes the rAF wait from the fixture since explicit measurement
doesn't depend on paint timing.

* Stabilize AI customizations editor fixtures

* Add narrow viewport fixture variants and fix widget overflow clipping

* fix: defer layout when offsetHeight returns 0 during display:none → visible transition

When switching sections in the Chat Customizations editor, the
container transitions from display:none to visible. The layout
method fires synchronously before the browser has reflowed, causing
offsetHeight to return 0 for sibling elements. Previously, layout
bailed out entirely, leaving the virtual list with no viewport
height — resulting in missing rows and empty space.

Now defers layout to the next animation frame so measurements
are accurate after the reflow completes.

* skill: add live debugging workflow to chat-customizations-editor skill

* skill: reference launch skill instead of duplicating agent-browser docs

* skill: add fixture gotchas — built-in grouping, editor contribution mocks, screenshot stability

* ci: add blocksCi labels to key customization fixtures for automated regression gating

* refactor: replace type assertion mutations with immutable map in applyBuiltinGroupKeys

* fix: show extension display name instead of ID for agent extension source labels
This commit is contained in:
Josh Spicer
2026-03-24 17:05:32 -07:00
committed by GitHub
parent ad32a00629
commit b4e7793b6c
8 changed files with 467 additions and 129 deletions

View File

@@ -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