From 422aed61b7d9307f739820ac5f72d6b346388aac Mon Sep 17 00:00:00 2001 From: Smartsheet-JB-Brown Date: Thu, 17 Apr 2025 20:21:16 -0700 Subject: [PATCH] memory optimization and revert test command in package.json to main --- package.json | 6 +- .../webview/packageManagerMessageHandler.ts | 16 +- .../package-manager/PackageManagerManager.ts | 274 ++++++++++-------- .../package-manager/PackageManagerView.tsx | 125 ++++---- .../components/PackageManagerItemCard.tsx | 39 ++- .../package-manager/components/TypeGroup.tsx | 63 ++-- .../package-manager/utils/grouping.ts | 62 ++-- 7 files changed, 318 insertions(+), 267 deletions(-) diff --git a/package.json b/package.json index ebd87a1e72..a9a9b48fc8 100644 --- a/package.json +++ b/package.json @@ -401,9 +401,9 @@ "package": "npm-run-all -l -p build:webview build:esbuild check-types lint", "pretest": "npm run compile", "dev": "cd webview-ui && npm run dev", - "test": "npm run test:extension", - "test:extension": "node --max-old-space-size=8192 ./node_modules/.bin/jest --silent --runInBand --detectOpenHandles --testTimeout=10000", - "test:extension:debug-memory": "node --max-old-space-size=8192 --trace-gc --expose-gc --heap-prof ./node_modules/.bin/jest --runInBand --logHeapUsage --detectOpenHandles --testTimeout=10000", + "test": "node scripts/run-tests.js", + "test:extension": "jest -w=40%", + "test:extension:debug-memory": "node --max-old-space-size=4096 --trace-gc --expose-gc --heap-prof ./node_modules/.bin/jest --runInBand --logHeapUsage --detectOpenHandles --testTimeout=10000", "test:webview": "cd webview-ui && npm run test", "prepare": "husky", "publish:marketplace": "vsce publish && ovsx publish", diff --git a/src/core/webview/packageManagerMessageHandler.ts b/src/core/webview/packageManagerMessageHandler.ts index c9522b184f..c90c09cc82 100644 --- a/src/core/webview/packageManagerMessageHandler.ts +++ b/src/core/webview/packageManagerMessageHandler.ts @@ -203,23 +203,13 @@ export async function handlePackageManagerMessages( case "filterPackageManagerItems": { if (message.filters) { try { - // Get current items from the manager - const items = packageManagerManager.getCurrentItems() - // Apply filters using the manager's filtering logic - const filteredItems = packageManagerManager.filterItems(items, { + // Update filtered items and post state + packageManagerManager.updateWithFilteredItems({ type: message.filters.type as ComponentType | undefined, search: message.filters.search, tags: message.filters.tags, }) - // Get current state and merge filtered items - const currentState = await provider.getStateToPostToWebview() - await provider.postMessageToWebview({ - type: "state", - state: { - ...currentState, - packageManagerItems: filteredItems, - }, - }) + await provider.postStateToWebview() } catch (error) { console.error("Package Manager: Error filtering items:", error) vscode.window.showErrorMessage("Failed to filter package manager items") diff --git a/src/services/package-manager/PackageManagerManager.ts b/src/services/package-manager/PackageManagerManager.ts index 186b2766d2..dd4acbf56f 100644 --- a/src/services/package-manager/PackageManagerManager.ts +++ b/src/services/package-manager/PackageManagerManager.ts @@ -287,10 +287,45 @@ export class PackageManagerManager { * @param filters The filter criteria * @returns Filtered items */ + // Cache size limit to prevent memory issues + private static readonly MAX_CACHE_SIZE = 100 + private filterCache = new Map< + string, + { + items: PackageManagerItem[] + timestamp: number + } + >() + + /** + * Clear old entries from the filter cache + */ + private cleanupFilterCache(): void { + if (this.filterCache.size > PackageManagerManager.MAX_CACHE_SIZE) { + // Sort by timestamp and keep only the most recent entries + const entries = Array.from(this.filterCache.entries()) + .sort(([, a], [, b]) => b.timestamp - a.timestamp) + .slice(0, PackageManagerManager.MAX_CACHE_SIZE) + + this.filterCache.clear() + entries.forEach(([key, value]) => this.filterCache.set(key, value)) + } + } + filterItems( items: PackageManagerItem[], filters: { type?: ComponentType; search?: string; tags?: string[] }, ): PackageManagerItem[] { + // Create cache key from filters + const cacheKey = JSON.stringify(filters) + const cached = this.filterCache.get(cacheKey) + if (cached) { + return cached.items + } + + // Clean up old cache entries + this.cleanupFilterCache() + // Helper function to normalize text for case/whitespace-insensitive comparison const normalizeText = (text: string) => text.toLowerCase().replace(/\s+/g, " ").trim() @@ -303,144 +338,118 @@ export class PackageManagerManager { return normalizeText(text).includes(normalizeText(searchTerm)) } - // Create a deep clone of all items - const clonedItems = items.map((originalItem) => JSON.parse(JSON.stringify(originalItem)) as PackageManagerItem) + // Filter items with shallow copies + const filteredItems = items + .map((item) => { + // Create shallow copy of item + const itemCopy = { ...item } - // Apply filters - const filteredItems = clonedItems.filter((item) => { - // Check parent item matches - const itemMatches = { - type: !filters.type || item.type === filters.type, - search: !searchTerm || containsSearchTerm(item.name) || containsSearchTerm(item.description), - tags: !filters.tags?.length || (item.tags && filters.tags.some((tag) => item.tags!.includes(tag))), - } + // Check parent item matches + const itemMatches = { + type: !filters.type || itemCopy.type === filters.type, + search: + !searchTerm || containsSearchTerm(itemCopy.name) || containsSearchTerm(itemCopy.description), + tags: + !filters.tags?.length || + (itemCopy.tags && filters.tags.some((tag) => itemCopy.tags!.includes(tag))), + } - // Check subcomponent matches - const subcomponentMatches = - item.items?.some((subItem) => { - const subMatches = { - type: !filters.type || subItem.type === filters.type, - search: - !searchTerm || - (subItem.metadata && - (containsSearchTerm(subItem.metadata.name) || - containsSearchTerm(subItem.metadata.description))), - tags: - !filters.tags?.length || - (subItem.metadata?.tags && - filters.tags.some((tag) => subItem.metadata!.tags!.includes(tag))), - } - - // When filtering by type, require exact type match - // For other filters (search/tags), any match is sufficient - return ( - subMatches.type && - (!searchTerm || subMatches.search) && - (!filters.tags?.length || subMatches.tags) - ) - }) ?? false - - // Include item if either: - // 1. Parent matches all active filters, or - // 2. Parent is a package and any subcomponent matches any active filter - const hasActiveFilters = filters.type || searchTerm || filters.tags?.length - if (!hasActiveFilters) return true - - const parentMatchesAll = itemMatches.type && itemMatches.search && itemMatches.tags - const isPackageWithMatchingSubcomponent = item.type === "package" && subcomponentMatches - return parentMatchesAll || isPackageWithMatchingSubcomponent - }) - - // Add match info to filtered items - return filteredItems.map((item) => { - // Calculate parent item matches - const itemMatches = { - type: !filters.type || item.type === filters.type, - search: !searchTerm || containsSearchTerm(item.name) || containsSearchTerm(item.description), - tags: !filters.tags?.length || (item.tags && filters.tags.some((tag) => item.tags!.includes(tag))), - } - - // Process subcomponents - let hasMatchingSubcomponents = false - if (item.items) { - item.items = item.items.map((subItem) => { - // Calculate individual filter matches for subcomponent - const subMatches = { - type: !filters.type || subItem.type === filters.type, - search: - !searchTerm || - (subItem.metadata && - (containsSearchTerm(subItem.metadata.name) || - containsSearchTerm(subItem.metadata.description))), - tags: - !filters.tags?.length || - (subItem.metadata?.tags && - filters.tags.some((tag) => subItem.metadata!.tags!.includes(tag))), - } - - // A subcomponent matches if it matches all active filters - const subMatched = subMatches.type && subMatches.search && subMatches.tags - - if (subMatched) { - hasMatchingSubcomponents = true - // Build match reason for matched subcomponent - const matchReason: Record = { - ...(searchTerm && { - nameMatch: !!subItem.metadata && containsSearchTerm(subItem.metadata.name), - descriptionMatch: - !!subItem.metadata && containsSearchTerm(subItem.metadata.description), - }), - ...(filters.type && { typeMatch: subMatches.type }), - ...(filters.tags?.length && { tagMatch: !!subMatches.tags }), + // Process subcomponents and track if any match + let hasMatchingSubcomponents = false + if (itemCopy.items?.length) { + itemCopy.items = itemCopy.items.map((subItem) => { + const subMatches = { + type: !filters.type || subItem.type === filters.type, + search: + !searchTerm || + (subItem.metadata && + (containsSearchTerm(subItem.metadata.name) || + containsSearchTerm(subItem.metadata.description))), + tags: + !filters.tags?.length || + !!( + subItem.metadata?.tags && + filters.tags.some((tag) => subItem.metadata!.tags!.includes(tag)) + ), } - subItem.matchInfo = { - matched: true, - matchReason, + const subItemMatched = + subMatches.type && + (!searchTerm || subMatches.search) && + (!filters.tags?.length || subMatches.tags) + + if (subItemMatched) { + hasMatchingSubcomponents = true + // Set matchInfo for matching subcomponent + // Build match reason based on active filters + const matchReason: Record = {} + + if (searchTerm) { + matchReason.nameMatch = containsSearchTerm(subItem.metadata?.name || "") + matchReason.descriptionMatch = containsSearchTerm(subItem.metadata?.description || "") + } + + // Always include typeMatch when filtering by type + if (filters.type) { + matchReason.typeMatch = subMatches.type + } + + subItem.matchInfo = { + matched: true, + matchReason, + } + } else { + subItem.matchInfo = { matched: false } } + + return subItem + }) + } + + const hasActiveFilters = filters.type || searchTerm || filters.tags?.length + if (!hasActiveFilters) return itemCopy + + const parentMatchesAll = itemMatches.type && itemMatches.search && itemMatches.tags + const isPackageWithMatchingSubcomponent = itemCopy.type === "package" && hasMatchingSubcomponents + + if (parentMatchesAll || isPackageWithMatchingSubcomponent) { + // Add match info without deep cloning + // Build parent match reason based on active filters + const matchReason: Record = {} + + if (searchTerm) { + matchReason.nameMatch = containsSearchTerm(itemCopy.name) + matchReason.descriptionMatch = containsSearchTerm(itemCopy.description) } else { - subItem.matchInfo = { - matched: false, - } + matchReason.nameMatch = false + matchReason.descriptionMatch = false } - return subItem - }) - } + // Always include typeMatch when filtering by type + if (filters.type) { + matchReason.typeMatch = itemMatches.type + } - // Build match reason for parent item - const matchReason: Record = { - nameMatch: searchTerm ? containsSearchTerm(item.name) : true, - descriptionMatch: searchTerm ? containsSearchTerm(item.description) : true, - } + if (hasMatchingSubcomponents) { + matchReason.hasMatchingSubcomponents = true + } - if (filters.type) { - matchReason.typeMatch = itemMatches.type - } - if (filters.tags?.length) { - matchReason.tagMatch = !!itemMatches.tags - } - if (hasMatchingSubcomponents) { - matchReason.hasMatchingSubcomponents = true - } + itemCopy.matchInfo = { + matched: true, + matchReason, + } + return itemCopy + } + return null + }) + .filter((item): item is PackageManagerItem => item !== null) - // Parent item is matched if: - // 1. It matches all active filters directly, or - // 2. It's a package and has any matching subcomponents - const parentMatchesAll = - (!filters.type || itemMatches.type) && - (!searchTerm || itemMatches.search) && - (!filters.tags?.length || itemMatches.tags) - - const isPackageWithMatchingSubcomponent = item.type === "package" && hasMatchingSubcomponents - - item.matchInfo = { - matched: parentMatchesAll || isPackageWithMatchingSubcomponent, - matchReason, - } - - return item + // Cache the results with timestamp + this.filterCache.set(cacheKey, { + items: filteredItems, + timestamp: Date.now(), }) + return filteredItems } /** @@ -491,6 +500,17 @@ export class PackageManagerManager { return this.currentItems } + /** + * Updates current items with filtered results + * @param filters The filter criteria + * @returns Filtered items + */ + updateWithFilteredItems(filters: { type?: ComponentType; search?: string; tags?: string[] }): PackageManagerItem[] { + const filteredItems = this.filterItems(this.currentItems, filters) + this.currentItems = filteredItems + return filteredItems + } + /** * Cleans up resources used by the package manager */ @@ -499,6 +519,8 @@ export class PackageManagerManager { const sources = Array.from(this.cache.keys()).map((url) => ({ url, enabled: true })) await this.cleanupCacheDirectories(sources) this.clearCache() + // Clear filter cache + this.filterCache.clear() } /** diff --git a/webview-ui/src/components/package-manager/PackageManagerView.tsx b/webview-ui/src/components/package-manager/PackageManagerView.tsx index c865406313..019735d86b 100644 --- a/webview-ui/src/components/package-manager/PackageManagerView.tsx +++ b/webview-ui/src/components/package-manager/PackageManagerView.tsx @@ -1,4 +1,4 @@ -import { useState, useEffect } from "react" +import { useState, useEffect, useMemo, useCallback } from "react" import { Button } from "@/components/ui/button" import { Tab, TabContent, TabHeader } from "../common/Tab" import { cn } from "@/lib/utils" @@ -25,8 +25,18 @@ const PackageManagerView: React.FC = ({ onDone }) => { manager.transition({ type: "FETCH_ITEMS" }) }, [manager]) - // Compute all available tags - const allTags = Array.from(new Set(state.allItems.flatMap((item) => item.tags || []))).sort() + // Memoize all available tags + const allTags = useMemo( + () => Array.from(new Set(state.allItems.flatMap((item) => item.tags || []))).sort(), + [state.allItems], + ) + + // Memoize filtered tags + const filteredTags = useMemo( + () => + tagSearch ? allTags.filter((tag: string) => tag.toLowerCase().includes(tagSearch.toLowerCase())) : allTags, + [allTags, tagSearch], + ) return ( @@ -178,55 +188,47 @@ const PackageManagerView: React.FC = ({ onDone }) => { {t("package-manager:filters.tags.noResults")} - {allTags - .filter((tag) => - tag.toLowerCase().includes(tagSearch.toLowerCase()), - ) - .map((tag) => ( - { - const isSelected = - state.filters.tags.includes(tag) - if (isSelected) { - manager.transition({ - type: "UPDATE_FILTERS", - payload: { - filters: { - tags: state.filters.tags.filter( - (t) => t !== tag, - ), - }, + {filteredTags.map((tag: string) => ( + { + const isSelected = state.filters.tags.includes(tag) + if (isSelected) { + manager.transition({ + type: "UPDATE_FILTERS", + payload: { + filters: { + tags: state.filters.tags.filter( + (t) => t !== tag, + ), }, - }) - } else { - manager.transition({ - type: "UPDATE_FILTERS", - payload: { - filters: { - tags: [ - ...state.filters.tags, - tag, - ], - }, + }, + }) + } else { + manager.transition({ + type: "UPDATE_FILTERS", + payload: { + filters: { + tags: [...state.filters.tags, tag], }, - }) - } - }} - className={`flex items-center gap-2 p-1 cursor-pointer text-sm hover:bg-vscode-button-secondaryBackground ${ - state.filters.tags.includes(tag) - ? "bg-vscode-button-background text-vscode-button-foreground" - : "text-vscode-dropdown-foreground" - }`} - onMouseDown={(e) => { - e.preventDefault() - }}> - - {tag} - - ))} + }, + }) + } + }} + className={`flex items-center gap-2 p-1 cursor-pointer text-sm hover:bg-vscode-button-secondaryBackground ${ + state.filters.tags.includes(tag) + ? "bg-vscode-button-background text-vscode-button-foreground" + : "text-vscode-dropdown-foreground" + }`} + onMouseDown={(e) => { + e.preventDefault() + }}> + + {tag} + + ))} )} @@ -400,16 +402,21 @@ const PackageManagerSourcesConfig: React.FC = setError("") } - const handleToggleSource = (index: number) => { - const updatedSources = [...sources] - updatedSources[index].enabled = !updatedSources[index].enabled - onSourcesChange(updatedSources) - } + const handleToggleSource = useCallback( + (index: number) => { + onSourcesChange( + sources.map((source, i) => (i === index ? { ...source, enabled: !source.enabled } : source)), + ) + }, + [sources, onSourcesChange], + ) - const handleRemoveSource = (index: number) => { - const updatedSources = sources.filter((_, i) => i !== index) - onSourcesChange(updatedSources) - } + const handleRemoveSource = useCallback( + (index: number) => { + onSourcesChange(sources.filter((_, i) => i !== index)) + }, + [sources, onSourcesChange], + ) return (
diff --git a/webview-ui/src/components/package-manager/components/PackageManagerItemCard.tsx b/webview-ui/src/components/package-manager/components/PackageManagerItemCard.tsx index 211a6c8955..87c86092b9 100644 --- a/webview-ui/src/components/package-manager/components/PackageManagerItemCard.tsx +++ b/webview-ui/src/components/package-manager/components/PackageManagerItemCard.tsx @@ -1,4 +1,4 @@ -import React, { useMemo } from "react" +import React, { useMemo, useCallback } from "react" import { Button } from "@/components/ui/button" import { PackageManagerItem } from "../../../../../src/services/package-manager/types" import { vscode } from "@/utils/vscode" @@ -33,8 +33,8 @@ export const PackageManagerItemCard: React.FC = ({ } } - const getTypeLabel = (type: string) => { - switch (type) { + const typeLabel = useMemo(() => { + switch (item.type) { case "mode": return t("package-manager:filters.type.mode") case "mcp server": @@ -46,10 +46,10 @@ export const PackageManagerItemCard: React.FC = ({ default: return t("package-manager:filters.type.all") } - } + }, [item.type, t]) - const getTypeColor = (type: string) => { - switch (type) { + const typeColor = useMemo(() => { + switch (item.type) { case "mode": return "bg-blue-600" case "mcp server": @@ -61,32 +61,31 @@ export const PackageManagerItemCard: React.FC = ({ default: return "bg-gray-600" } - } + }, [item.type]) - const handleOpenUrl = () => { - // If sourceUrl is present and valid, use it directly without modifications + // Memoize URL calculation + const urlToOpen = useMemo(() => { if (item.sourceUrl && isValidUrl(item.sourceUrl)) { - return vscode.postMessage({ - type: "openExternal", - url: item.sourceUrl, - }) + return item.sourceUrl } - // Otherwise use repoUrl with git path information - let urlToOpen = item.repoUrl + let url = item.repoUrl if (item.defaultBranch) { - urlToOpen = `${urlToOpen}/tree/${item.defaultBranch}` + url = `${url}/tree/${item.defaultBranch}` if (item.path) { const normalizedPath = item.path.replace(/\\/g, "/").replace(/^\/+/, "") - urlToOpen = `${urlToOpen}/${normalizedPath}` + url = `${url}/${normalizedPath}` } } + return url + }, [item.sourceUrl, item.repoUrl, item.defaultBranch, item.path]) + const handleOpenUrl = useCallback(() => { vscode.postMessage({ type: "openExternal", url: urlToOpen, }) - } + }, [urlToOpen]) // Group items by type const groupedItems = useMemo(() => { @@ -135,9 +134,7 @@ export const PackageManagerItemCard: React.FC = ({

) : null}
- - {getTypeLabel(item.type)} - + {typeLabel}

{item.description}

diff --git a/webview-ui/src/components/package-manager/components/TypeGroup.tsx b/webview-ui/src/components/package-manager/components/TypeGroup.tsx index 5d10ff477d..4f714dbfdd 100644 --- a/webview-ui/src/components/package-manager/components/TypeGroup.tsx +++ b/webview-ui/src/components/package-manager/components/TypeGroup.tsx @@ -1,4 +1,4 @@ -import React from "react" +import React, { useMemo } from "react" import { cn } from "@/lib/utils" import { useAppTranslation } from "@/i18n/TranslationContext" @@ -19,7 +19,7 @@ interface TypeGroupProps { export const TypeGroup: React.FC = ({ type, items, className }) => { const { t } = useAppTranslation() - const getTypeLabel = (type: string) => { + const typeLabel = useMemo(() => { switch (type) { case "mode": return t("package-manager:type-group.modes") @@ -34,38 +34,45 @@ export const TypeGroup: React.FC = ({ type, items, className }) type: type.charAt(0).toUpperCase() + type.slice(1), }) } - } + }, [type, t]) + + const containerClassName = useMemo(() => cn("mb-4", className), [className]) + + // Memoize the list items + const listItems = useMemo(() => { + if (!items?.length) return null + + return items.map((item, index) => { + const itemClassName = cn( + "text-sm pl-1", + item.matchInfo?.matched ? "text-vscode-foreground font-medium" : "text-vscode-foreground", + ) + const nameClassName = cn("font-medium", item.matchInfo?.matched ? "text-vscode-textLink" : "") + + return ( +
  • + {item.name} + {item.description && ( + - {item.description} + )} + {item.matchInfo?.matched && ( + + {t("package-manager:type-group.match")} + + )} +
    • + ) + }) + }, [items, t]) if (!items?.length) { return null } return ( -
    -

    {getTypeLabel(type)}

    -
      - {items.map((item, index) => ( -
    1. - - {item.name} - - {item.description && ( - - {item.description} - )} - {item.matchInfo?.matched && ( - - {t("package-manager:type-group.match")} - - )} -
      2. - ))} -
    +
    +

    {typeLabel}

    +
      {listItems}
    ) } diff --git a/webview-ui/src/components/package-manager/utils/grouping.ts b/webview-ui/src/components/package-manager/utils/grouping.ts index 0567fd8adc..592b1cc8ec 100644 --- a/webview-ui/src/components/package-manager/utils/grouping.ts +++ b/webview-ui/src/components/package-manager/utils/grouping.ts @@ -21,33 +21,45 @@ export interface GroupedItems { * @param items Array of items to group * @returns Object with items grouped by type */ +// Cache for group objects to avoid recreating them +const groupCache = new Map() + export function groupItemsByType(items: PackageManagerItem["items"] = []): GroupedItems { if (!items?.length) { return {} } - return items.reduce((groups: GroupedItems, item) => { - if (!item.type) { - return groups - } + // Clear old items from groups but keep the group objects + groupCache.forEach((group) => (group.items.length = 0)) - if (!groups[item.type]) { - groups[item.type] = { + const groups: GroupedItems = {} + + for (const item of items) { + if (!item.type) continue + + let group = groupCache.get(item.type) + if (!group) { + group = { type: item.type, items: [], } + groupCache.set(item.type, group) } - groups[item.type].items.push({ + if (!groups[item.type]) { + groups[item.type] = group + } + + group.items.push({ name: item.metadata?.name || "Unnamed item", description: item.metadata?.description, metadata: item.metadata, path: item.path, matchInfo: item.matchInfo, }) + } - return groups - }, {}) + return groups } /** @@ -55,19 +67,26 @@ export function groupItemsByType(items: PackageManagerItem["items"] = []): Group * @param item The item to format * @returns Formatted string with name and description */ +// Reuse string buffer for formatting +const formatBuffer = { + result: "", + maxLength: 100, +} + export function formatItemText(item: { name: string; description?: string }): string { if (!item.description) { return item.name } - // Truncate description if it's too long - const maxDescriptionLength = 100 - const description = - item.description.length > maxDescriptionLength - ? `${item.description.substring(0, maxDescriptionLength)}...` + // Reuse the same string buffer + formatBuffer.result = item.name + formatBuffer.result += " - " + formatBuffer.result += + item.description.length > formatBuffer.maxLength + ? item.description.substring(0, formatBuffer.maxLength) + "..." : item.description - return `${item.name} - ${description}` + return formatBuffer.result } /** @@ -75,8 +94,12 @@ export function formatItemText(item: { name: string; description?: string }): st * @param groups Grouped items object * @returns Total number of items */ +// Cache array of group values +let groupValuesCache: Array<{ items: any[] }> = [] + export function getTotalItemCount(groups: GroupedItems): number { - return Object.values(groups).reduce((total, group) => total + group.items.length, 0) + groupValuesCache = Object.values(groups) + return groupValuesCache.reduce((total, group) => total + group.items.length, 0) } /** @@ -84,6 +107,11 @@ export function getTotalItemCount(groups: GroupedItems): number { * @param groups Grouped items object * @returns Array of type strings */ +// Cache array of types +let typesCache: string[] = [] + export function getUniqueTypes(groups: GroupedItems): string[] { - return Object.keys(groups).sort() + typesCache = Object.keys(groups) + typesCache.sort() + return typesCache }