From b793815db1c396496fe600f9bbd977d60764eca9 Mon Sep 17 00:00:00 2001 From: CD Cabrera Date: Fri, 22 May 2026 13:50:53 -0400 Subject: [PATCH 1/2] feat(search): pf-4120 parallel result filtering * pf.search, parallel filtering with abort * docs.filterWords, update blocklist --- cspell.config.json | 1 + src/__tests__/patternFly.search.test.ts | 350 +++++++++++++++++++++++- src/docs.filterWords.ts | 2 +- src/patternFly.search.ts | 274 ++++++++++++++++++- src/tool.searchPatternFlyDocs.ts | 2 +- tests/e2e/httpTransport.test.ts | 37 +++ tests/e2e/stdioTransport.test.ts | 41 ++- 7 files changed, 693 insertions(+), 14 deletions(-) diff --git a/cspell.config.json b/cspell.config.json index c82c2014..2247adb3 100644 --- a/cspell.config.json +++ b/cspell.config.json @@ -14,6 +14,7 @@ "patternfly", "prefault", "rereview", + "rescan", "rsort", "sparkline", "streamable", diff --git a/src/__tests__/patternFly.search.test.ts b/src/__tests__/patternFly.search.test.ts index 4ce03cbc..5f6a4459 100644 --- a/src/__tests__/patternFly.search.test.ts +++ b/src/__tests__/patternFly.search.test.ts @@ -1,4 +1,9 @@ -import { filterPatternFly, searchPatternFly } from '../patternFly.search'; +import { + dynamicFilterPatternFly, + filterPatternFly, + searchPatternFly, + type FilterPatternFlyFilters +} from '../patternFly.search'; describe('filterPatternFly', () => { const mockResources = new Map([ @@ -86,6 +91,294 @@ describe('filterPatternFly', () => { expect(result.byEntry).toEqual(expect.arrayContaining([{ section: 1 }])); expect(Array.from(result.byResource).length).toBeGreaterThanOrEqual(0); }); + + it('should return no results when signal is already aborted before the resource loop', async () => { + const controller = new AbortController(); + + controller.abort(); + + const result = await filterPatternFly(undefined, mockResources as any, { signal: controller.signal }); + + expect(result.byEntry).toEqual([]); + expect(result.byResource.size).toBe(0); + }); + + it('should throw signalError when aborted during entry filtering', async () => { + const controller = new AbortController(); + const signalError = new DOMException('Filter operation aborted', 'AbortError'); + + controller.abort(); + + await expect( + filterPatternFly(undefined, mockResources as any, { signal: controller.signal, signalError }) + ).rejects.toBe(signalError); + }); +}); + +describe('filterPatternFly.memo', () => { + const mockResources = new Map([ + ['button', { + name: 'button', + entries: [ + { name: 'button', section: 'components', category: 'action', version: 'v6' } + ] + }], + ['modal', { + name: 'modal', + entries: [ + { name: 'modal', section: 'components', category: 'view', version: 'v6' } + ] + }] + ]); + + it('should cache by filters and map for MCP-style calls', async () => { + const filters = { name: 'moda' }; + const first = filterPatternFly.memo(filters, mockResources as any); + const second = filterPatternFly.memo(filters, mockResources as any); + + expect(first).toBe(second); + await expect(first).resolves.toMatchObject({ + byEntry: expect.arrayContaining([expect.objectContaining({ name: 'modal' })]) + }); + }); + + it('should not reuse cache across different filters', async () => { + const first = filterPatternFly.memo({ name: 'moda' }, mockResources as any); + const second = filterPatternFly.memo({ name: 'button' }, mockResources as any); + + expect(first).not.toBe(second); + await expect(first).resolves.toMatchObject({ + byEntry: expect.arrayContaining([expect.objectContaining({ name: 'modal' })]) + }); + await expect(second).resolves.toMatchObject({ + byEntry: expect.arrayContaining([expect.objectContaining({ name: 'button' })]) + }); + }); + + it('should not reuse cache across different resource maps', async () => { + const filters = { name: 'moda' }; + const modalOnlyResources = new Map([ + ['modal', { + name: 'modal', + entries: [{ name: 'modal', section: 'components', category: 'view', version: 'v6' }] + }] + ]); + const first = filterPatternFly.memo(filters, mockResources as any); + const second = filterPatternFly.memo(filters, modalOnlyResources as any); + + expect(first).not.toBe(second); + }); + + it('should exclude settings from cache key', async () => { + const filters = { name: 'modal' }; + const signalError = new DOMException('', 'AbortError'); + const first = filterPatternFly.memo(filters, mockResources as any); + const second = filterPatternFly.memo(filters, mockResources as any, { + signal: new AbortController().signal, + signalError + }); + + expect(first).toBe(second); + await expect(second).resolves.toMatchObject({ + byEntry: expect.arrayContaining([expect.objectContaining({ name: 'modal' })]) + }); + }); +}); + +describe('dynamicFilterPatternFly', () => { + const mockResources = new Map([ + ['button', { + name: 'button', + entries: [ + { name: 'button', section: 'components', category: 'action', version: 'v6' }, + { name: 'button', section: 'components', category: 'action', version: 'v5' } + ] + }], + ['modal', { + name: 'modal', + entries: [ + { name: 'modal', section: 'components', category: 'view', version: 'v6' } + ] + }], + ['card', { + name: 'card', + entries: [ + { name: 'card', section: 'layouts', category: 'view', version: 'v6' } + ] + }] + ]); + + it.each([ + { + description: 'return immediate single match if filters already narrow it down', + searchQuery: 'ignored', + filters: { name: 'modal' }, + options: {}, + expectedNames: ['modal'] + }, + { + description: 'find a single match by applying searchQuery to "name"', + searchQuery: 'modal', + filters: {}, + options: { searchFilters: ['name'] }, + expectedNames: ['modal'] + }, + { + description: 'find a single match by applying searchQuery to "section"', + searchQuery: 'layouts', + filters: {}, + options: { searchFilters: ['section'] }, + expectedNames: ['card'] + }, + { + description: 'fallback to original results when using a broad section', + searchQuery: 'components', + filters: {}, + options: { searchFilters: ['name'] }, + expectedNames: ['button', 'button', 'modal', 'card'] + }, + { + description: 'fallback to original when using a broad category', + searchQuery: 'view', + filters: {}, + options: {}, + expectedNames: ['button', 'button', 'modal', 'card'] + }, + { + description: 'skip iterative filter if useExistingFilters is true and filter is already set', + searchQuery: 'modal', + filters: { name: 'button' }, + options: { useExistingFilters: true, searchFilters: ['name'] }, + expectedNames: ['button', 'button'] + }, + { + description: 'use custom searchFilters with increased max results', + searchQuery: 'view', + filters: {}, + options: { searchFilters: ['category'], maxResultsLimit: 2 }, + expectedNames: ['modal', 'card'] + } + ])('should $description', async ({ searchQuery, filters, options, expectedNames }) => { + const result = await dynamicFilterPatternFly( + searchQuery, + filters as any, + mockResources as any, + options as any + ); + + expect(result.byEntry.map(result => result.name)).toEqual(expectedNames); + }); + + it.each([ + { + description: 'name filter wins and aborts sibling section scans', + searchQuery: 'modal', + filters: {}, + options: { searchFilters: ['name', 'section'] as const }, + expectedNames: ['modal'] + }, + { + description: 'section filter wins and aborts sibling name scans', + searchQuery: 'layouts', + filters: {}, + options: { searchFilters: ['section', 'name'] as const }, + expectedNames: ['card'] + } + ])('should wire parallel filter passes with shared signal when $description', async ({ + searchQuery, + filters, + options, + expectedNames + }) => { + const result = await dynamicFilterPatternFly( + searchQuery, + filters as any, + mockResources as any, + options as any + ); + + expect(result.byEntry.map(entry => entry.name)).toEqual(expectedNames); + }); + + it('should not call filterPatternFly.memo during parallel or fallback passes', async () => { + const memoSpy = jest.spyOn(filterPatternFly, 'memo'); + + await dynamicFilterPatternFly( + 'modal', + {}, + mockResources as any, + { searchFilters: ['name', 'section'] } + ); + + await dynamicFilterPatternFly( + 'components', + {}, + mockResources as any, + { searchFilters: ['name'] } + ); + + expect(memoSpy).not.toHaveBeenCalled(); + + memoSpy.mockRestore(); + }); + + it('should cap parallel filter passes when searchFilters exceeds the internal limit', async () => { + const oversizedFilters = [ + 'name', + 'section', + 'category', + 'version', + 'path', + 'name', + 'section', + 'category', + 'version', + 'path', + 'name', + 'section', + 'category', + 'version', + 'path' + ]; + + const result = await dynamicFilterPatternFly( + 'modal', + {}, + mockResources as any, + { searchFilters: oversizedFilters as (keyof FilterPatternFlyFilters)[] } + ); + + expect(result.byEntry.map(entry => entry.name)).toEqual(['modal']); + }); + + it('should not return partial results after a parallel pass wins', async () => { + await dynamicFilterPatternFly( + 'modal', + {}, + mockResources as any, + { searchFilters: ['name', 'section'] } + ); + + const result = await dynamicFilterPatternFly( + 'components', + {}, + mockResources as any, + { searchFilters: ['name'] } + ); + + expect(result.byEntry.map(entry => entry.name)).toEqual(['button', 'button', 'modal', 'card']); + }); + + it('should fallback to base filter when no pass matches maxResultsLimit', async () => { + const result = await dynamicFilterPatternFly( + 'components', + {}, + mockResources as any, + { searchFilters: ['name'] } + ); + + expect(result.byEntry.map(entry => entry.name)).toEqual(['button', 'button', 'modal', 'card']); + }); }); describe('searchPatternFly', () => { @@ -161,16 +454,32 @@ describe('searchPatternFly', () => { expectedType: 'contains' }, { - description: 'patternfly:// URI', + description: 'patternfly:// URI with filter', search: 'patternfly://docs/modal', + options: { dynamicFilter: true }, expectedLength: 1, expectedName: 'modal', expectedType: 'exact' }, + { + description: 'patternfly:// URI without filter', + search: 'patternfly://docs/modal', + expectedLength: 1, + expectedName: 'modal', + expectedType: 'exact' + }, + { + description: 'hash entry id with filter', + search: 'btn-v6-hash', + options: { dynamicFilter: true }, + expectedLength: 1, + expectedName: 'button', + expectedType: 'exact' + }, { description: 'hash entry id without filter', search: 'btn-v6-hash', - options: {}, + options: { dynamicFilter: false }, expectedLength: 2, expectedName: 'button', expectedType: 'exact' @@ -210,4 +519,39 @@ describe('searchPatternFly', () => { expect(searchResults?.length).toBe(2); expect(searchResults?.[0]?.matchType).toBe('all'); }); + + it('should not call filterPatternFly.memo when dynamicFilter is enabled', async () => { + const memoSpy = jest.spyOn(filterPatternFly, 'memo'); + + await searchPatternFly('patternfly://docs/modal', {}, { dynamicFilter: true, ...mockOptions }); + + expect(memoSpy).not.toHaveBeenCalled(); + + memoSpy.mockRestore(); + }); + + it('should call filterPatternFly.memo for scoped non-dynamic filtering', async () => { + const memoSpy = jest.spyOn(filterPatternFly, 'memo'); + + await searchPatternFly('button', {}, mockOptions); + + expect(memoSpy).toHaveBeenCalledTimes(1); + expect(memoSpy).toHaveBeenCalledWith({}, expect.any(Map)); + + memoSpy.mockRestore(); + }); + + it('should reuse filterPatternFly.memo cache for repeated identical scoped searches', async () => { + const memoSpy = jest.spyOn(filterPatternFly, 'memo'); + + const first = searchPatternFly('button', {}, mockOptions); + const second = searchPatternFly('button', {}, mockOptions); + + await Promise.all([first, second]); + + expect(memoSpy).toHaveBeenCalledTimes(2); + expect(memoSpy.mock.results[0]?.value).toBe(memoSpy.mock.results[1]?.value); + + memoSpy.mockRestore(); + }); }); diff --git a/src/docs.filterWords.ts b/src/docs.filterWords.ts index 4cf14340..da897df4 100644 --- a/src/docs.filterWords.ts +++ b/src/docs.filterWords.ts @@ -4,7 +4,7 @@ * @note It's tempting to remove category and section names from this list, don't. Instead, the search * should be leveraging filters which allow for "section" and "category" specifically. */ -const INDEX_BLOCKLIST_WORDS = ['patternfly', 'component', 'components', 'documentation', 'example', 'examples']; +const INDEX_BLOCKLIST_WORDS = ['patternfly', 'component', 'components', 'documentation', 'example', 'examples', 'view']; /** * Technical terms and acronyms that should be exempt from length and noise filtering. diff --git a/src/patternFly.search.ts b/src/patternFly.search.ts index fd42af69..6f02021f 100644 --- a/src/patternFly.search.ts +++ b/src/patternFly.search.ts @@ -5,6 +5,7 @@ import { type FuzzySearchResult } from './server.search'; import { memo } from './server.caching'; +import { generateHash } from './server.helpers'; import { DEFAULT_OPTIONS } from './options.defaults'; import { getPatternFlyMcpResources, @@ -76,7 +77,8 @@ interface SearchPatternFlyResult extends FuzzySearchResult, PatternFlyMcpResourc * @interface SearchPatternFlyResults * * @property isSearchWildCardAll - Whether the search query matched all components - * @property {SearchPatternFlyResult | undefined} firstExactMatch - `@deprecated Use exactMatches[0] or searchResults` Exact-ranked result + * @property {SearchPatternFlyResult | undefined} firstExactMatch - `@deprecated Unreliable when the query is a hash, URI, or + * path (compares name to the query string). Prefer exactMatches[0] or searchResults`. * @property {SearchPatternFlyResult[]} exactMatches - Exact matches within search results * @property {SearchPatternFlyResult[]} remainingMatches - Contrast to `exactMatches`, the remaining matches within search results * @property {SearchPatternFlyResult[]} searchResults - All search results, exact and remaining matches @@ -101,16 +103,73 @@ interface SearchPatternFlyResults { * * @property {Promise} [mcpResources] - Object of multifaceted documentation entries to search. * @property [allowWildCardAll] - Allow a search query to match all components. + * @property [dynamicFilter] - Allow a search query to attempt a multi-filter match on returned search results for tighter results. * @property [maxDistance] - Maximum edit distance for fuzzy search. * @property [maxResults] - Maximum number of results to return. */ interface SearchPatternFlyOptions { mcpResources?: Promise; allowWildCardAll?: boolean; + dynamicFilter?: boolean; maxDistance?: number; maxResults?: number; } +/** + * Optional settings for {@link filterPatternFly}. + * + * @interface FilterPatternFlySettings + * + * @property [maxSyncTime] - Max synchronous time slice in milliseconds before yielding when `signal` is set. Defaults to `25`. + * @property [signal] - Abort signal; breaks the resource loop when aborted. + * @property [signalError] - Error to throw when aborted (avoids returning partial results on sibling passes). + */ +interface FilterPatternFlySettings { + maxSyncTime?: number; + signal?: AbortSignal; + signalError?: Error | DOMException; +} + +/** + * Filter keys tried in parallel by {@link dynamicFilterPatternFly}. Order is priority + * (e.g. `name` first for hash/entry id and URI narrowing). Do not randomize — truncation + * and `Promise.any` both keep this sequence; reorder only with intentional product priority. + */ +const SEARCH_FILTERS: (keyof FilterPatternFlyFilters)[] = ['name', 'section', 'category', 'version', 'path']; + +/** + * Max parallel dynamic-filter passes (excluding the always-included base pass). Matches + * {@link SEARCH_FILTERS} length; longer custom `searchFilters` arrays are truncated from the + * front so priority order is preserved. Do not randomize the slice. + */ +const MAX_DYNAMIC_FILTER_PASSES = SEARCH_FILTERS.length; + +/** + * Filtering and manage PatternFly MCP resources. + * + * Allows handling resources as + * - a `Promise` that resolves to `PatternFlyMcpAvailableResources` + * - Use the `Promise` when the resources are retrieved asynchronously and require processing upon resolution. + * - a `Map` instance where the key is a string and the value is `PatternFlyMcpResourceFilteredMetadata`. + * - Use the `Map` when the resources are already available and stored in key-value pairs for quick access. + * + */ +type FilterPatternFlyMcpResources = | Promise | + Map; + +/** + * Used for configuring the `filterPatternFly.memo`. + * + * @property {FilterPatternFlyFilters} 0 The filters to be applied, which specify the behavior or conditions for the memoized functionality. + * @property {FilterPatternFlyMcpResources} [1] Optional MCP resources configuration to be utilized during memo execution. + * @property {FilterPatternFlySettings} [2] Optional settings that influence runtime behavior or processing settings. + */ +type FilterPatternFlyMemoArgs = [ + filters: FilterPatternFlyFilters | undefined, + mcpResources?: FilterPatternFlyMcpResources | undefined, + settings?: FilterPatternFlySettings | undefined +]; + /** * Apply sequenced priority filters for predictable filtering, filter PatternFly data. * @@ -123,20 +182,34 @@ interface SearchPatternFlyOptions { * - Exact "version" filtering only * - Has `prefix`, `suffix` filtering for any non-"version" field. * + * @note Memoization: {@link filterPatternFly.memo} caches by `filters` + `resources` map content. Used for MCP + * resource handlers (full-catalog reads) and {@link searchPatternFly}'s non-dynamic scoped filter pass. + * {@link dynamicFilterPatternFly}'s `Promise.any` race still calls bare {@link filterPatternFly}; see that + * function's developer note before reintroducing memo there. + * * @note Filter formats are generally assumed to be string values. If expanding to other types, ensure * proper handling of non-string values. Future updates should align with the string coercion used * in other code base searches. * + * @performance At current catalog scale (~5–6ms cold), the filter loop stays synchronous. + * When `signal` is set, optional time-slicing (`maxSyncTime` + `setImmediate` every 200 resources) + * yields if execution exceeds the threshold — skipped when no signal is set. + * * @param {FilterPatternFlyFilters} filters - Available filters for PatternFly data. * @param [mcpResources] - An optional map of available PatternFly documentation entries to search. * Internally, defaults to `getPatternFlyMcpResources.resources` + * @param [settings] - Optional {@link FilterPatternFlySettings}. + * @param [settings.maxSyncTime] - Optional maximum synchronous time slice in milliseconds before yielding to event loop. + * @param [settings.signal] - Optional abort signal; breaks the resource loop when aborted. + * @param [settings.signalError] - Optional error to throw when aborted. * @returns {Promise} - Filtered PatternFly results. * - `byEntry`: Array of filtered documentation entries. * - `byResource`: Map of filtered resources by resource name. */ const filterPatternFly = async ( filters: FilterPatternFlyFilters | undefined, - mcpResources?: Promise | Map + mcpResources?: FilterPatternFlyMcpResources, + { maxSyncTime = 25, signal, signalError }: FilterPatternFlySettings = {} ): Promise => { const getResources = await (mcpResources || getPatternFlyMcpResources.memo()); const resources = (getResources as PatternFlyMcpAvailableResources)?.resources || @@ -144,6 +217,7 @@ const filterPatternFly = async ( // Normalize filters - Currently, this is set to string filtering. Review expanding if/when necessary. let updatedFilters: FilterPatternFlyFilters = {}; + const startTime = (signal && performance.now()) || undefined; if (filters) { // Allow strings and coerced numbers as strings @@ -165,8 +239,36 @@ const filterPatternFly = async ( normalizePropertyValue.endsWith(filterValue); }; + const isBlocking = (i: number) => + signal && startTime && (i % 200 === 0) && (performance.now() - startTime > maxSyncTime); + + let index = 0; + for (const [name, resource] of resources) { + if (isBlocking(index)) { + await new Promise(resolve => setImmediate(resolve)); + } + + index += 1; + + if (signal?.aborted) { + if (signalError) { + throw signalError; + } + + break; + } + const matchedEntries = resource.entries.filter(entry => { + // Throw on abort so sibling passes reject instead of returning partial entry batches. + if (signal?.aborted) { + if (signalError) { + throw signalError; + } + + return false; + } + const matchesVersion = !updatedFilters.version || String(entry.version).toLowerCase() === updatedFilters.version; const matchesCategory = !updatedFilters.category || filterMatch(entry.category, updatedFilters.category); const matchesSection = !updatedFilters.section || filterMatch(entry.section, updatedFilters.section); @@ -182,6 +284,14 @@ const filterPatternFly = async ( return matchesVersion && matchesCategory && matchesSection && matchesPath && matchesName; }); + if (signal?.aborted) { + if (signalError) { + throw signalError; + } + + break; + } + if (matchedEntries.length > 0) { byEntry.push(...matchedEntries); const { versions, ...filteredResource } = resource; @@ -213,9 +323,145 @@ const filterPatternFly = async ( }; /** - * Memoized version of filterPatternFly + * Memoized version of filterPatternFly for MCP resource handlers. + * + * @note Use {@link filterPatternFly.memo} for full-catalog MCP resource reads (index/template handlers) and + * {@link searchPatternFly}'s non-dynamic scoped filter pass. Do **not** use it inside + * {@link dynamicFilterPatternFly}'s `Promise.any` race or its fallback — see that function's developer note for + * why and how to reintroduce memo there safely. + * + * @note Cache key hashes `filters` and `resources` only. `signal`, `signalError`, and `maxSyncTime` are + * forwarded on cache miss but excluded from the key. `cacheErrors: false` — rejected/aborted passes are not cached. */ -filterPatternFly.memo = memo(filterPatternFly, DEFAULT_OPTIONS.resourceMemoOptions.default); +filterPatternFly.memo = memo(filterPatternFly, { + ...DEFAULT_OPTIONS.resourceMemoOptions.default, + cacheErrors: false, + keyHash: (args: Readonly) => { + const [filters, resources] = args; + + return generateHash([filters, resources]); + } +}); + +/** + * Filter down the tightest possible results. The first pass that matches `maxResultsLimit` + * wins. If no matches, return the base filter. + * + * @note Parallel pass count is at most {@link MAX_DYNAMIC_FILTER_PASSES} filter passes plus one base + * pass; longer `searchFilters` lists are truncated from the front. Keep {@link SEARCH_FILTERS} + * aligned with product priority — do not randomize pass order or truncation. + * + * @note **Memo split (read before changing this race).** {@link filterPatternFly.memo} is used for MCP + * resource handlers and {@link searchPatternFly}'s non-dynamic path, but this function calls bare + * {@link filterPatternFly} in the `Promise.any` race and in the catch fallback. That is intentional: perf + * testing showed memo here was a net loss once cache-poison handling was included. + * + * @note **Do not drop `filterPatternFly.memo` into `Promise.any` or the fallback without the guards + * below.** Parallel passes share filters/maps but differ by abort timing; memoizing naively caches partial + * results from losing passes or collides entries across races. Fixing that cost more than memo saved. + * + * @note **If you need memo back in this race, reproduce all of the following (prior working design):** + * - Give each `Promise.any` race one shared settings object (`signal`, `signalError`, and a module-private + * `_passId` from `randomUUID()`) so memo keys from that race do not collide with MCP {@link filterPatternFly.memo} + * calls or other concurrent races. + * - Extend settings with internal `_passId?: string` (not public API) and hash it in {@link filterPatternFly.memo}'s + * `keyHash` alongside `filters` and `resources`: `generateHash([filters, resources, settings._passId])`. + * - Keep `cacheErrors: false` on {@link filterPatternFly.memo} so rejected/aborted passes are never cached. + * - Keep `signalError` on {@link filterPatternFly} so aborted siblings **throw** instead of returning partial + * `byEntry` batches (otherwise memo stores poisoned slices). + * - Still call bare {@link filterPatternFly} on the catch fallback unless you also scope that path's cache key + * (fallback runs without `signal` after `finally` aborts the shared controller). + * + * @note Parallel passes today share one settings object per race (`signal`, `signalError`) so aborted siblings + * throw via `signalError` even without memo. + * + * @param searchQuery - Search query. + * @param filters - Available filters for PatternFly data. + * @param mcpResources - Scoped resources for PatternFly data. + * @param [options] - Optional settings object + * @param [options.searchFilters] - Array of filters to search typically from {@link filterPatternFly}. Defaults to {@link SEARCH_FILTERS}. + * @param [options.maxFilterPasses] - Max number of parallel filter passes. Defaults to {@link MAX_DYNAMIC_FILTER_PASSES}. + * @param [options.maxResultsLimit] - Max number of results internal conditions need to match before they return the original result. Defaults to `1`. + * @param [options.useExistingFilters] - Use the existing filters or bypass them. Defaults to `true`. + * @returns {Promise} - A Promise resolving to the filtering results. + */ +const dynamicFilterPatternFly = async ( + searchQuery: string, + filters: FilterPatternFlyFilters | undefined, + mcpResources?: Promise | Map, + { + searchFilters = SEARCH_FILTERS, + maxFilterPasses = MAX_DYNAMIC_FILTER_PASSES, + maxResultsLimit = 1, + useExistingFilters = true + }: { searchFilters?: (keyof FilterPatternFlyFilters)[]; maxFilterPasses?: number; maxResultsLimit?: number; useExistingFilters?: boolean } = {} +): Promise => { + // Error name + const dynamicFilterPassNotMatched = 'DynamicFilterPassNotMatchedError'; + + // Centralized error + const createDynamicFilterPassNotMatchedError = () => { + const error = new Error('Dynamic filter pass did not match maxResultsLimit'); + + error.name = dynamicFilterPassNotMatched; + + return error; + }; + + // Matching conditions based on options + const isCloseMatch = (output: FilterPatternFlyResults) => + output.byEntry.length === maxResultsLimit; + + const abortController = new AbortController(); + const { signal } = abortController; + + // Run match and handle abort + const passFail = (promise: Promise) => + promise.then(output => { + if (signal.aborted || !isCloseMatch(output)) { + return Promise.reject(createDynamicFilterPassNotMatchedError()); + } + + abortController.abort(); + + return output; + }).catch((err: unknown) => { + if (signal.aborted || (err instanceof DOMException && err.name === 'AbortError')) { + return Promise.reject(createDynamicFilterPassNotMatchedError()); + } + + throw err; + }); + + // Limit the filters to ones not already set; cap parallel passes to avoid runaway fan-out. + const filtersToTry = searchFilters + .filter(filter => !(useExistingFilters && filters && filters[filter])) + .slice(0, maxFilterPasses); + + const settings = { + signal, + signalError: new DOMException('Filter operation aborted', 'AbortError') + }; + + try { + // See dynamicFilterPatternFly @note — do not memoize here without _passId keyHash + cacheErrors: false + signalError. + return await Promise.any([ + ...filtersToTry.map(filter => + passFail(filterPatternFly({ ...filters, [filter]: searchQuery }, mcpResources, settings))), + passFail(filterPatternFly(filters, mcpResources, settings)) + ]); + } catch { + // Base pass is in the race; if all reject, rescan without signal (finally aborts shared controller). + return filterPatternFly(filters, mcpResources); + } finally { + abortController.abort(); + } +}; + +/** + * Memoized version of dynamicFilterPatternFly + */ +dynamicFilterPatternFly.memo = memo(dynamicFilterPatternFly, DEFAULT_OPTIONS.resourceMemoOptions.default); /** * Search for PatternFly component documentation URLs using fuzzy search. @@ -224,7 +470,8 @@ filterPatternFly.memo = memo(filterPatternFly, DEFAULT_OPTIONS.resourceMemoOptio * dictates that this function remains purely data-driven, apply default versions in the caller. * See both MCP resources and tools for examples. * - * @note Uses `filterPatternFly` for additional filtering. + * @note Non-dynamic filtering uses {@link filterPatternFly.memo} on the scoped `searchResultsFilterMap` + * (cache key = filters + map content). Dynamic filtering uses {@link dynamicFilterPatternFly.memo} instead. * * @param searchQuery - Search query. Values are coerced to string for fuzzy search. * @param {FilterPatternFlyFilters} filters - Available filters for PatternFly data. @@ -235,6 +482,8 @@ filterPatternFly.memo = memo(filterPatternFly, DEFAULT_OPTIONS.resourceMemoOptio * - `keywordsMap`: Map of normalized keywords against versioned entries * - `resources`: Map of names against entries * @param [settings.allowWildCardAll] - Allow a search query to match all resources. Defaults to `false`. + * @param [settings.dynamicFilter] - Allow a search query to attempt a multi-filter match on returned search results. Defaults to `false`. + * Useful for narrowing down search results to a specific resource. * @param [settings.maxDistance] - Maximum edit distance for fuzzy search. Defaults to `3`. * @param [settings.maxResults] - Maximum number of results to return. Defaults to `10`. * @returns Object containing search results and matched URLs @@ -249,6 +498,7 @@ filterPatternFly.memo = memo(filterPatternFly, DEFAULT_OPTIONS.resourceMemoOptio const searchPatternFly = async (searchQuery: unknown, filters?: FilterPatternFlyFilters | undefined, { mcpResources, allowWildCardAll = false, + dynamicFilter = false, maxDistance = 3, maxResults = 10 }: SearchPatternFlyOptions = {}): Promise => { @@ -321,8 +571,16 @@ const searchPatternFly = async (searchQuery: unknown, filters?: FilterPatternFly } } - // Apply filtering - const { byResource } = await filterPatternFly(updatedFilters, searchResultsFilterMap); + let filtered: FilterPatternFlyResults; + + // Filter resources. Dynamic filtering applies the search query to each filter as a fallback. + if (dynamicFilter && !isSearchWildCardAll) { + filtered = await dynamicFilterPatternFly.memo(coercedSearchQuery, updatedFilters, searchResultsFilterMap); + } else { + filtered = await filterPatternFly.memo(updatedFilters, searchResultsFilterMap); + } + + const { byResource } = filtered; // Loop fuzzy results, apply and update search results with resources. for (const [name, fuzzyMatch] of fuzzyResultsMap) { @@ -374,10 +632,12 @@ const searchPatternFly = async (searchQuery: unknown, filters?: FilterPatternFly searchPatternFly.memo = memo(searchPatternFly, DEFAULT_OPTIONS.toolMemoOptions.searchPatternFlyDocs); export { + dynamicFilterPatternFly, filterPatternFly, searchPatternFly, type FilterPatternFlyFilters, type FilterPatternFlyResults, + type FilterPatternFlySettings, type SearchPatternFlyResult, type SearchPatternFlyResults }; diff --git a/src/tool.searchPatternFlyDocs.ts b/src/tool.searchPatternFlyDocs.ts index 6f2b469f..a913ccdc 100644 --- a/src/tool.searchPatternFlyDocs.ts +++ b/src/tool.searchPatternFlyDocs.ts @@ -47,7 +47,7 @@ const searchPatternFlyDocsTool = (options = getOptions()): McpTool => { const { isSearchWildCardAll, exactMatches, remainingMatches, searchResults, totalPotentialMatches } = await searchPatternFly.memo( searchQuery, { version: updatedVersion }, - { allowWildCardAll: true, maxResults: options.minMax.toolSearches.max } + { allowWildCardAll: true, dynamicFilter: true, maxResults: options.minMax.toolSearches.max } ); assertInput( diff --git a/tests/e2e/httpTransport.test.ts b/tests/e2e/httpTransport.test.ts index b7436565..facb63ac 100644 --- a/tests/e2e/httpTransport.test.ts +++ b/tests/e2e/httpTransport.test.ts @@ -229,6 +229,43 @@ describe('Builtin tools, HTTP transport', () => { contains.forEach(item => expect(text).toContain(item)); }); + it('should stay stable across repeated and concurrent sha1 searches', async () => { + const entryHash = '19b2a9418c744e70da9e3dd0965d1948ec1ebbe4'; + const queries = [ + entryHash, + `patternfly://docs/${entryHash}`, + entryHash + ]; + + const callSearch = (searchQuery: string) => CLIENT?.send({ + method: 'tools/call', + params: { + name: 'searchPatternFlyDocs', + arguments: { searchQuery } + } + }); + + const serialResponses = await Promise.all(queries.map(query => callSearch(query))); + + serialResponses.forEach(response => { + const text = response?.result?.content?.[0]?.text || ''; + + expect(text).toContain('Showing 1 exact match'); + expect(text).toContain('**button**'); + }); + + const concurrentResponses = await Promise.all( + Array.from({ length: 3 }, () => callSearch(entryHash)) + ); + + concurrentResponses.forEach(response => { + const text = response?.result?.content?.[0]?.text || ''; + + expect(text).toContain('Showing 1 exact match'); + expect(text).toContain('**button**'); + }); + }); + it('should return expected markdown structure for search results', async () => { const response = await CLIENT?.send({ method: 'tools/call', diff --git a/tests/e2e/stdioTransport.test.ts b/tests/e2e/stdioTransport.test.ts index cb41852e..7e4d0ddd 100644 --- a/tests/e2e/stdioTransport.test.ts +++ b/tests/e2e/stdioTransport.test.ts @@ -214,7 +214,7 @@ describe('Builtin tools, STDIO', () => { description: 'hash search query', searchQuery: '19b2a9418c744e70da9e3dd0965d1948ec1ebbe4', contains: [ - 'Showing 2 exact match', + 'Showing 1 exact match', '**button**' ] }, @@ -230,7 +230,7 @@ describe('Builtin tools, STDIO', () => { description: 'uri search query', searchQuery: 'patternfly://docs/19b2a9418c744e70da9e3dd0965d1948ec1ebbe4', contains: [ - 'Showing 2 exact match', + 'Showing 1 exact match', '**button**' ] }, @@ -257,6 +257,43 @@ describe('Builtin tools, STDIO', () => { contains.forEach(item => expect(text).toContain(item)); }); + it('should stay stable across repeated and concurrent sha1 searches', async () => { + const entryHash = '19b2a9418c744e70da9e3dd0965d1948ec1ebbe4'; + const queries = [ + entryHash, + `patternfly://docs/${entryHash}`, + entryHash + ]; + + const callSearch = (searchQuery: string) => CLIENT.send({ + method: 'tools/call', + params: { + name: 'searchPatternFlyDocs', + arguments: { searchQuery } + } + }); + + const serialResponses = await Promise.all(queries.map(query => callSearch(query))); + + serialResponses.forEach(response => { + const text = response?.result?.content?.[0]?.text || ''; + + expect(text).toContain('Showing 1 exact match'); + expect(text).toContain('**button**'); + }); + + const concurrentResponses = await Promise.all( + Array.from({ length: 3 }, () => callSearch(entryHash)) + ); + + concurrentResponses.forEach(response => { + const text = response?.result?.content?.[0]?.text || ''; + + expect(text).toContain('Showing 1 exact match'); + expect(text).toContain('**button**'); + }); + }); + it('should return expected markdown structure for search results', async () => { const response = await CLIENT.send({ method: 'tools/call', From fd43402149d52337096b7f32954d910a041d5ac2 Mon Sep 17 00:00:00 2001 From: CD Cabrera Date: Fri, 29 May 2026 16:46:49 -0400 Subject: [PATCH 2/2] fix: review update --- src/__tests__/patternFly.search.test.ts | 56 +++++++++++++++++++++++++ src/patternFly.search.ts | 21 +++++++--- 2 files changed, 71 insertions(+), 6 deletions(-) diff --git a/src/__tests__/patternFly.search.test.ts b/src/__tests__/patternFly.search.test.ts index 5f6a4459..d45ca7d3 100644 --- a/src/__tests__/patternFly.search.test.ts +++ b/src/__tests__/patternFly.search.test.ts @@ -379,6 +379,62 @@ describe('dynamicFilterPatternFly', () => { expect(result.byEntry.map(entry => entry.name)).toEqual(['button', 'button', 'modal', 'card']); }); + + it('should cache by searchQuery, filters, and map', async () => { + const mockResource = new Map([ + ['modal', { + entries: [ + { name: 'modal', section: 'components', category: 'view', version: 'v6' } + ] + }] + ]); + + const first = dynamicFilterPatternFly.memo('modal', {}, mockResource as any); + const second = dynamicFilterPatternFly.memo('modal', {}, mockResource as any); + + expect(first).toBe(second); + + await expect(first).resolves.toMatchObject({ + byEntry: expect.arrayContaining([ + expect.objectContaining({ name: 'modal' }) + ]) + }); + }); + + it('should not keep rejected fallback failures in cache', async () => { + const mockResource = new Map([ + ['modal', { + entries: [ + { name: 'modal', section: 'components', category: 'view', version: 'v6' } + ] + }] + ]); + let resourceLoads = 0; + + // Represents the number of filters and fallback. + const failThroughLoad = 7; + const flakyMcpResources = { + then(onFulfilled?: (value: any) => any, onRejected?: (reason: any) => any) { + resourceLoads += 1; + + if (resourceLoads <= failThroughLoad) { + return Promise.reject(new Error('Failed to load')).then(onFulfilled, onRejected); + } + + return Promise.resolve(mockResource).then(onFulfilled, onRejected); + } + }; + + await expect(dynamicFilterPatternFly.memo('modal', {}, flakyMcpResources as any)) + .rejects.toThrow('Failed to load'); + + await expect(dynamicFilterPatternFly.memo('modal', {}, flakyMcpResources as any)) + .resolves.toMatchObject({ + byEntry: expect.arrayContaining([ + expect.objectContaining({ name: 'modal' }) + ]) + }); + }); }); describe('searchPatternFly', () => { diff --git a/src/patternFly.search.ts b/src/patternFly.search.ts index 6f02021f..1ccf5089 100644 --- a/src/patternFly.search.ts +++ b/src/patternFly.search.ts @@ -351,10 +351,12 @@ filterPatternFly.memo = memo(filterPatternFly, { * pass; longer `searchFilters` lists are truncated from the front. Keep {@link SEARCH_FILTERS} * aligned with product priority — do not randomize pass order or truncation. * - * @note **Memo split (read before changing this race).** {@link filterPatternFly.memo} is used for MCP - * resource handlers and {@link searchPatternFly}'s non-dynamic path, but this function calls bare - * {@link filterPatternFly} in the `Promise.any` race and in the catch fallback. That is intentional: perf - * testing showed memo here was a net loss once cache-poison handling was included. + * @note **Memo split (read before changing this race).** This function is memoized externally + * via {@link dynamicFilterPatternFly.memo} (with `cacheErrors: false`), but its internal calls + * to {@link filterPatternFly} must remain bare primarily for performance but also to avoid dealing with + * cache poison and partial results or collisions across races. If you decide to ignore this warning and + * re-implement the memo internally, inside `promise.any`, you'll need to pass + * `_passId keyHash + cacheErrors: false + signalError`. * * @note **Do not drop `filterPatternFly.memo` into `Promise.any` or the fallback without the guards * below.** Parallel passes share filters/maps but differ by abort timing; memoizing naively caches partial @@ -444,7 +446,7 @@ const dynamicFilterPatternFly = async ( }; try { - // See dynamicFilterPatternFly @note — do not memoize here without _passId keyHash + cacheErrors: false + signalError. + // Inner passes should remain non-memoized for performance see @note above return await Promise.any([ ...filtersToTry.map(filter => passFail(filterPatternFly({ ...filters, [filter]: searchQuery }, mcpResources, settings))), @@ -460,8 +462,15 @@ const dynamicFilterPatternFly = async ( /** * Memoized version of dynamicFilterPatternFly + * + * @note `cacheErrors: false` so a rejected fallback doesn't stick. This aligns with + * {@link filterPatternFly.memo}. Parallel pass poison is avoided by calling bare + * {@link filterPatternFly} inside the race, not by this outer memo setting alone. */ -dynamicFilterPatternFly.memo = memo(dynamicFilterPatternFly, DEFAULT_OPTIONS.resourceMemoOptions.default); +dynamicFilterPatternFly.memo = memo(dynamicFilterPatternFly, { + ...DEFAULT_OPTIONS.resourceMemoOptions.default, + cacheErrors: false +}); /** * Search for PatternFly component documentation URLs using fuzzy search.