Fix AI customization count misalignment for plugin-storage items#308348
Fix AI customization count misalignment for plugin-storage items#308348joshspicer wants to merge 1 commit intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes incorrect representation of plugin-storage AI customization items in the Sessions app, ensuring counts and tree grouping stay aligned while also preventing repeated observable subscriptions in the overview.
Changes:
- Refactored AI customization tree grouping to dynamically group by storage (including
plugin) via shared helpers and a stable storage order. - Updated the overview view to register MCP/plugin
autorunsubscriptions once in the constructor instead of perloadCounts()call.
Show a summary per file
| File | Description |
|---|---|
| src/vs/sessions/contrib/aiCustomizationTreeView/browser/aiCustomizationTreeViewViews.ts | Replaces per-storage filtering with groupByStorage + ordered group building so plugin-storage items appear and counts align. |
| src/vs/sessions/contrib/aiCustomizationTreeView/browser/aiCustomizationOverviewView.ts | Moves MCP/plugin reactive count tracking into the constructor to avoid accumulating duplicate autorun registrations. |
Copilot's findings
- Files reviewed: 2/2 changed files
- Comments generated: 1
| private groupByStorage<T extends { readonly storage: AICustomizationPromptsStorage }>(items: readonly T[]): Map<AICustomizationPromptsStorage, readonly T[]> { | ||
| const grouped = new Map<AICustomizationPromptsStorage, T[]>(); | ||
| for (const item of items) { | ||
| let bucket = grouped.get(item.storage); | ||
| if (!bucket) { | ||
| bucket = []; | ||
| grouped.set(item.storage, bucket); | ||
| } | ||
| bucket.push(item); | ||
| } | ||
| return grouped; | ||
| } |
There was a problem hiding this comment.
groupByStorage declares a return type of Map<AICustomizationPromptsStorage, readonly T[]>, but it constructs new Map<..., T[]>() and returns it directly. Because Map is mutable (it has set), Map<..., T[]> is generally not assignable to Map<..., readonly T[]> and this is likely to fail type-checking. Consider changing the return type (and ICachedTypeData.files) to ReadonlyMap<..., readonly T[]> (since callers only use .get), or keep Map but make the value type consistent (e.g. Map<..., T[]>) and adjust the cache field type accordingly.
joshspicer
force-pushed
the
copilot/elegant-orangutan
branch
from
b68e22f to
ce40e92
Compare
There was a problem hiding this comment.
Pull request overview
Fixes count/display misalignment in the Sessions app AI Customization UI by ensuring plugin-storage items are grouped and shown in the tree, and by preventing repeated reactive subscriptions in the overview.
Changes:
- Refactors tree grouping logic to dynamically group by storage type (including
plugin) using shared helpers and a stable storage-order list. - Moves MCP server and plugin count
autorunsubscriptions to the overview view constructor to avoid accumulating duplicate subscriptions.
Show a summary per file
| File | Description |
|---|---|
| src/vs/sessions/contrib/aiCustomizationTreeView/browser/aiCustomizationTreeViewViews.ts | Refactors storage grouping/caching so all storages (incl. plugin) are represented consistently in the tree. |
| src/vs/sessions/contrib/aiCustomizationTreeView/browser/aiCustomizationOverviewView.ts | Registers MCP/plugin count autoruns once in the constructor to avoid leaks/redundant work. |
Copilot's findings
- Files reviewed: 2/2 changed files
- Comments generated: 1
| @@ -460,42 +477,12 @@ class UnifiedAICustomizationDataSource implements IAsyncDataSource<RootElement, | |||
| } | |||
| } | |||
|
|
|||
| const workspaceItems = allItems.filter(item => item.storage === PromptsStorage.local); | |||
| const userItems = allItems.filter(item => item.storage === PromptsStorage.user); | |||
| const extensionItems = allItems.filter(item => item.storage === PromptsStorage.extension); | |||
| const builtinItems = allItems.filter(item => item.storage === BUILTIN_STORAGE); | |||
|
|
|||
| cached.files = new Map<string, readonly IPromptPath[]>([ | |||
| [PromptsStorage.local, workspaceItems], | |||
| [PromptsStorage.user, userItems], | |||
| [PromptsStorage.extension, extensionItems], | |||
| [BUILTIN_STORAGE, builtinItems], | |||
| ]); | |||
|
|
|||
| const itemCount = allItems.length; | |||
| this.totalItemCount += itemCount; | |||
| cached.files = this.groupByStorage(allItems); | |||
| this.totalItemCount += allItems.length; | |||
| this.onItemCountChanged(this.totalItemCount); | |||
| } | |||
|
|
|||
| const workspaceItems = cached.files!.get(PromptsStorage.local) || []; | |||
| const userItems = cached.files!.get(PromptsStorage.user) || []; | |||
| const extensionItems = cached.files!.get(PromptsStorage.extension) || []; | |||
| const builtinItems = cached.files!.get(BUILTIN_STORAGE) || []; | |||
|
|
|||
| if (workspaceItems.length > 0) { | |||
| groups.push(this.createGroupItem(promptType, PromptsStorage.local, workspaceItems.length)); | |||
| } | |||
| if (userItems.length > 0) { | |||
| groups.push(this.createGroupItem(promptType, PromptsStorage.user, userItems.length)); | |||
| } | |||
| if (extensionItems.length > 0) { | |||
| groups.push(this.createGroupItem(promptType, PromptsStorage.extension, extensionItems.length)); | |||
| } | |||
| if (builtinItems.length > 0) { | |||
| groups.push(this.createGroupItem(promptType, BUILTIN_STORAGE, builtinItems.length)); | |||
| } | |||
|
|
|||
| return groups; | |||
| return this.buildGroupItems(promptType, cached.files); | |||
| } | |||
There was a problem hiding this comment.
cached.files is an optional property, and after if (!cached.files) { cached.files = ... } TypeScript does not reliably narrow cached.files to non-undefined. Passing cached.files directly into buildGroupItems is likely to fail type-checking (and could be unsafe if the property is mutated elsewhere). Assign to a local const files = cached.files ??= ... and pass files, or use a non-null assertion if you're certain it is set here.
- Tree view: Replace brittle per-storage-type filtering in getStorageGroups() with dynamic groupByStorage()/buildGroupItems() helpers. Previously, items with storage=plugin were counted in totalItemCount but never displayed because only local/user/ extension/builtin were explicitly handled. - Overview view: Move MCP and plugin autorun registrations from loadCounts() into the constructor. loadCounts() is called on every service event, so each call leaked duplicate autorun subscriptions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
joshspicer
force-pushed
the
copilot/elegant-orangutan
branch
from
ce40e92 to
50f1a0d
Compare
Summary
Fixes count misalignment in the Sessions app's AI Customization views where plugin-storage items were not correctly represented.
Bugs Fixed
1. Tree view dropped plugin-storage items
UnifiedAICustomizationDataSource.getStorageGroups()manually filtered forlocal,user,extension, andbuiltinstorage types — but neverplugin. Items withstorage === PromptsStorage.pluginwere included intotalItemCount(affecting the empty-state context key) but never displayed as groups in the tree.Fix: Replaced brittle per-storage-type filtering with dynamic
groupByStorage()+buildGroupItems()helpers that handle all storage types via a singleSTORAGE_ORDERarray. Adding a new storage type now requires one line.2. Overview view leaked autorun subscriptions
loadCounts()registered newautorunsubscriptions for MCP and plugin counts on every call. SinceloadCounts()is triggered by workspace/agent/command change events, this accumulated duplicate autoruns — a memory leak producing redundant work.Fix: Moved MCP and plugin autorun registrations into the constructor so they register exactly once.
3. Net simplification
Removed ~75 lines of repetitive per-storage-type filter/push code, replaced with 2 reusable helpers.