From c45568a48ccea9d94ddfb04deab82da847fda474 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Wed, 16 Sep 2026 12:51:24 -0400 Subject: [PATCH 1/3] feat(organizations): search the admin org list by relevance MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The admin picker filtered on `name=ct=`, a raw substring match that is case-sensitive — typing `acme` never found "Acme Corporation". It also could not survive a typo, a missing accent or lost punctuation. Sends `search=` instead, which CM ranks by relevance: a case-, accent- and punctuation-folded match, plus a semantic leg over embeddings, so `mcdonalds` finds "McDonald & Sons" and `bank` finds the banks. `sort(name)` is now sent only when browsing unfiltered. With a search term the order IS the ranking, and the server ignores any sort — leaving it in would suggest a guarantee that no longer holds. Requires the matching central-manager change (HarperFast/central-manager#806). CM must be deployed first: an older CM parses `search` as a condition on an Organization attribute that does not exist. Co-Authored-By: Claude Opus 5 --- .../queries/getAllOrganizations.test.ts | 30 ++++++++++++++----- .../queries/getAllOrganizations.ts | 18 ++++++++--- 2 files changed, 37 insertions(+), 11 deletions(-) diff --git a/src/features/organizations/queries/getAllOrganizations.test.ts b/src/features/organizations/queries/getAllOrganizations.test.ts index b785d1d40..73a9f6369 100644 --- a/src/features/organizations/queries/getAllOrganizations.test.ts +++ b/src/features/organizations/queries/getAllOrganizations.test.ts @@ -44,15 +44,24 @@ describe('buildAllOrganizationsUrl', () => { expect(buildAllOrganizationsUrl(2, '')).toBe('/Admin/Organization/?status=ne=DELETED&sort(name)&limit(24,37)'); }); - it('puts the name filter before the status condition (the API ignores name when status comes first)', () => { + it('sends a filter as a relevance search, not a case-sensitive substring match', () => { + // `name=ct=acme` never matched "Acme Corporation" — the bug this replaces. expect(buildAllOrganizationsUrl(0, 'acme')).toBe( - '/Admin/Organization/?name=ct=acme&status=ne=DELETED&sort(name)&limit(0,13)', + '/Admin/Organization/?search=acme&status=ne=DELETED&limit(0,13)', ); }); + it('omits sort(name) while searching, because the order is the relevance ranking', () => { + expect(buildAllOrganizationsUrl(0, 'acme')).not.toContain('sort('); + }); + + it('still sorts by name when browsing unfiltered', () => { + expect(buildAllOrganizationsUrl(0, '')).toContain('sort(name)'); + }); + it('URI-encodes the filter value', () => { expect(buildAllOrganizationsUrl(0, 'a&b=c')).toBe( - '/Admin/Organization/?name=ct=a%26b%3Dc&status=ne=DELETED&sort(name)&limit(0,13)', + '/Admin/Organization/?search=a%26b%3Dc&status=ne=DELETED&limit(0,13)', ); }); @@ -140,13 +149,20 @@ describe('getAllOrganizationsQueryOptions', () => { expect(mockedGet).toHaveBeenNthCalledWith(2, '/Admin/Organization/org-7'); }); - it('runs a name search when the value is not an id', async () => { + it('runs a relevance search when the value is not an id', async () => { mockedGet.mockResolvedValue({ data: [] }); await getAllOrganizationsQueryOptions(0, 'Acme').queryFn!({} as never); - expect(mockedGet).toHaveBeenCalledWith( - '/Admin/Organization/?name=ct=Acme&status=ne=DELETED&sort(name)&limit(0,13)', - ); + expect(mockedGet).toHaveBeenCalledWith('/Admin/Organization/?search=Acme&status=ne=DELETED&limit(0,13)'); + }); + + it('finds an organization whatever case the operator types', async () => { + mockedGet.mockResolvedValue({ data: [] }); + + await getAllOrganizationsQueryOptions(0, 'acme').queryFn!({} as never); + + // The server folds case; the client just has to stop sending `name=ct=`. + expect(mockedGet).toHaveBeenCalledWith('/Admin/Organization/?search=acme&status=ne=DELETED&limit(0,13)'); }); }); diff --git a/src/features/organizations/queries/getAllOrganizations.ts b/src/features/organizations/queries/getAllOrganizations.ts index 34d06aa3b..2a187ceb2 100644 --- a/src/features/organizations/queries/getAllOrganizations.ts +++ b/src/features/organizations/queries/getAllOrganizations.ts @@ -18,16 +18,26 @@ export interface AllOrganizationsPage { * Builds the Harper REST query for one page of the admin organization list. * Harper collections return a bare array (no total count), so we request one * record past the page boundary to learn whether a next page exists. + * + * A filter is sent as `search`, which ranks by relevance server-side rather + * than filtering on a raw substring. The previous `name=ct=` was a + * case-sensitive match, so `acme` never found "Acme Corporation"; `search` + * folds case, accents and punctuation and falls back to a semantic match, so + * `mcdonalds` finds "McDonald & Sons" and `bank` finds the banks. + * + * `sort(name)` is therefore only sent when browsing unfiltered — with a search + * term the order IS the relevance ranking, and the server ignores any sort. */ export function buildAllOrganizationsUrl(pageIndex: number, nameFilter: string): string { const start = pageIndex * ALL_ORGANIZATIONS_PAGE_SIZE; const conditions = [ - // The name filter must precede the status condition: with status first, - // the API ignores the name condition entirely. - ...(nameFilter ? [`name=ct=${encodeURIComponent(nameFilter)}`] : []), + // Kept ahead of the status condition, as the name filter had to be: the + // server reads `search` independently of position, but the ordering + // costs nothing and keeps one less thing depending on that detail. + ...(nameFilter ? [`search=${encodeURIComponent(nameFilter)}`] : []), // Terminated organizations stick around with status DELETED; hide them. 'status=ne=DELETED', - 'sort(name)', + ...(nameFilter ? [] : ['sort(name)']), `limit(${start},${start + ALL_ORGANIZATIONS_PAGE_SIZE + 1})`, ]; return `/Admin/Organization/?${conditions.join('&')}`; From 61c2ff622b65ae9af88500c06c4b99a34d3ab510 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Wed, 16 Sep 2026 13:41:46 -0400 Subject: [PATCH 2/3] fix(organizations): treat a whitespace-only filter as unfiltered browsing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Untrimmed, a filter of only spaces sent `search=%20%20%20` and dropped `sort(name)` — so clearing a filter back to spaces returned the whole list in relevance order for a term that matches nothing in particular, instead of the alphabetical browse it looks like. Trims before deciding, so blank is blank. Co-Authored-By: Claude Opus 5 --- .../queries/getAllOrganizations.test.ts | 14 ++++++++++++++ .../organizations/queries/getAllOrganizations.ts | 9 +++++++-- 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/src/features/organizations/queries/getAllOrganizations.test.ts b/src/features/organizations/queries/getAllOrganizations.test.ts index 73a9f6369..de345a29d 100644 --- a/src/features/organizations/queries/getAllOrganizations.test.ts +++ b/src/features/organizations/queries/getAllOrganizations.test.ts @@ -59,6 +59,20 @@ describe('buildAllOrganizationsUrl', () => { expect(buildAllOrganizationsUrl(0, '')).toContain('sort(name)'); }); + it('treats a whitespace-only filter as unfiltered browsing', () => { + // Untrimmed this sent `search=%20%20%20` and dropped sort(name), so clearing a + // filter back to spaces left the whole list in relevance order for a blank term. + expect(buildAllOrganizationsUrl(0, ' ')).toBe( + '/Admin/Organization/?status=ne=DELETED&sort(name)&limit(0,13)', + ); + }); + + it('trims surrounding whitespace off a real filter', () => { + expect(buildAllOrganizationsUrl(0, ' acme ')).toBe( + '/Admin/Organization/?search=acme&status=ne=DELETED&limit(0,13)', + ); + }); + it('URI-encodes the filter value', () => { expect(buildAllOrganizationsUrl(0, 'a&b=c')).toBe( '/Admin/Organization/?search=a%26b%3Dc&status=ne=DELETED&limit(0,13)', diff --git a/src/features/organizations/queries/getAllOrganizations.ts b/src/features/organizations/queries/getAllOrganizations.ts index 2a187ceb2..3221e172d 100644 --- a/src/features/organizations/queries/getAllOrganizations.ts +++ b/src/features/organizations/queries/getAllOrganizations.ts @@ -30,14 +30,19 @@ export interface AllOrganizationsPage { */ export function buildAllOrganizationsUrl(pageIndex: number, nameFilter: string): string { const start = pageIndex * ALL_ORGANIZATIONS_PAGE_SIZE; + // Trimmed, so a filter of only spaces is unfiltered browsing rather than a + // search for whitespace: untrimmed it would send `search=%20%20%20` AND drop + // `sort(name)`, leaving the full list in relevance order for a term that + // matches nothing in particular. + const search = nameFilter.trim(); const conditions = [ // Kept ahead of the status condition, as the name filter had to be: the // server reads `search` independently of position, but the ordering // costs nothing and keeps one less thing depending on that detail. - ...(nameFilter ? [`search=${encodeURIComponent(nameFilter)}`] : []), + ...(search ? [`search=${encodeURIComponent(search)}`] : []), // Terminated organizations stick around with status DELETED; hide them. 'status=ne=DELETED', - ...(nameFilter ? [] : ['sort(name)']), + ...(search ? [] : ['sort(name)']), `limit(${start},${start + ALL_ORGANIZATIONS_PAGE_SIZE + 1})`, ]; return `/Admin/Organization/?${conditions.join('&')}`; From 8116322f81ed3c6007a273ec292b597bddac63ae Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Wed, 16 Sep 2026 13:57:43 -0400 Subject: [PATCH 3/3] refactor(organizations): normalize the search value once, at the query boundary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the cross-model review. The trim lived in buildAllOrganizationsUrl while the cache key used the raw value, so `acme`, `acme ` and ` acme` were three query-cache entries resolving to one identical request — a pasted trailing space refetched a relevance search the cache already held. Normalizing once in getAllOrganizationsQueryOptions fixes that, and incidentally removes the undefined exposure the review flagged: the builder no longer assumes its argument is a string. Also trims the comments three separate lenses flagged as narrating the diff's history rather than the code. The remaining review finding — offset pagination over a ranking the server recomputes per request — is fixed on the server (HarperFast/central-manager 6299574c) by breaking ties on id, since with sort() ignored in search mode the client has no tie-breaker it can ask for. Co-Authored-By: Claude Opus 5 --- .../queries/getAllOrganizations.test.ts | 15 ++++-- .../queries/getAllOrganizations.ts | 48 ++++++++++--------- 2 files changed, 36 insertions(+), 27 deletions(-) diff --git a/src/features/organizations/queries/getAllOrganizations.test.ts b/src/features/organizations/queries/getAllOrganizations.test.ts index de345a29d..81eb27459 100644 --- a/src/features/organizations/queries/getAllOrganizations.test.ts +++ b/src/features/organizations/queries/getAllOrganizations.test.ts @@ -45,7 +45,6 @@ describe('buildAllOrganizationsUrl', () => { }); it('sends a filter as a relevance search, not a case-sensitive substring match', () => { - // `name=ct=acme` never matched "Acme Corporation" — the bug this replaces. expect(buildAllOrganizationsUrl(0, 'acme')).toBe( '/Admin/Organization/?search=acme&status=ne=DELETED&limit(0,13)', ); @@ -60,8 +59,6 @@ describe('buildAllOrganizationsUrl', () => { }); it('treats a whitespace-only filter as unfiltered browsing', () => { - // Untrimmed this sent `search=%20%20%20` and dropped sort(name), so clearing a - // filter back to spaces left the whole list in relevance order for a blank term. expect(buildAllOrganizationsUrl(0, ' ')).toBe( '/Admin/Organization/?status=ne=DELETED&sort(name)&limit(0,13)', ); @@ -176,7 +173,17 @@ describe('getAllOrganizationsQueryOptions', () => { await getAllOrganizationsQueryOptions(0, 'acme').queryFn!({} as never); - // The server folds case; the client just has to stop sending `name=ct=`. expect(mockedGet).toHaveBeenCalledWith('/Admin/Organization/?search=acme&status=ne=DELETED&limit(0,13)'); }); + + it('shares one cache entry across equivalent filters, and requests the normalized term', () => { + const key = (v: string) => getAllOrganizationsQueryOptions(0, v).queryKey; + expect(key(' acme ')).toEqual(key('acme')); + expect(key(' ')).toEqual(key('')); + }); + + it('survives a caller that has no filter value at all', () => { + expect(() => getAllOrganizationsQueryOptions(0, undefined as unknown as string)).not.toThrow(); + expect(buildAllOrganizationsUrl(0, undefined as unknown as string)).toContain('sort(name)'); + }); }); diff --git a/src/features/organizations/queries/getAllOrganizations.ts b/src/features/organizations/queries/getAllOrganizations.ts index 3221e172d..97265c4ed 100644 --- a/src/features/organizations/queries/getAllOrganizations.ts +++ b/src/features/organizations/queries/getAllOrganizations.ts @@ -19,26 +19,16 @@ export interface AllOrganizationsPage { * Harper collections return a bare array (no total count), so we request one * record past the page boundary to learn whether a next page exists. * - * A filter is sent as `search`, which ranks by relevance server-side rather - * than filtering on a raw substring. The previous `name=ct=` was a - * case-sensitive match, so `acme` never found "Acme Corporation"; `search` - * folds case, accents and punctuation and falls back to a semantic match, so - * `mcdonalds` finds "McDonald & Sons" and `bank` finds the banks. - * - * `sort(name)` is therefore only sent when browsing unfiltered — with a search - * term the order IS the relevance ranking, and the server ignores any sort. + * `search` is ranked by relevance server-side, so `sort(name)` is sent only + * when browsing unfiltered: with a term the order is the ranking, and the + * server ignores any sort. Expects an already-normalized term — see + * {@link normalizeSearchValue}. */ export function buildAllOrganizationsUrl(pageIndex: number, nameFilter: string): string { const start = pageIndex * ALL_ORGANIZATIONS_PAGE_SIZE; - // Trimmed, so a filter of only spaces is unfiltered browsing rather than a - // search for whitespace: untrimmed it would send `search=%20%20%20` AND drop - // `sort(name)`, leaving the full list in relevance order for a term that - // matches nothing in particular. - const search = nameFilter.trim(); + const search = normalizeSearchValue(nameFilter); const conditions = [ - // Kept ahead of the status condition, as the name filter had to be: the - // server reads `search` independently of position, but the ordering - // costs nothing and keeps one less thing depending on that detail. + // Kept ahead of the status condition, as the name filter had to be. ...(search ? [`search=${encodeURIComponent(search)}`] : []), // Terminated organizations stick around with status DELETED; hide them. 'status=ne=DELETED', @@ -48,6 +38,14 @@ export function buildAllOrganizationsUrl(pageIndex: number, nameFilter: string): return `/Admin/Organization/?${conditions.join('&')}`; } +/** + * Normalizes a raw filter value. Whitespace-only is unfiltered browsing, not a + * search for spaces. + */ +export function normalizeSearchValue(value: string | undefined | null): string { + return typeof value === 'string' ? value.trim() : ''; +} + export async function getAllOrganizations(pageIndex: number, nameFilter: string): Promise { const { data } = await apiClient.get(buildAllOrganizationsUrl(pageIndex, nameFilter) as '/Organization/'); return { @@ -101,15 +99,19 @@ export async function getOrganizationForClusterPage(clusterId: string): Promise< } /** - * Query for the admin organization search. When the search value looks like an - * organization or cluster id (a lowercase `org-`/`clu-` prefix — see - * {@link detectEntityId}), it resolves that id server-side instead of running a - * title/name search, so pasting an id jumps straight to the matching org. + * Query for the admin organization search. A value that looks like an + * organization or cluster id (see {@link detectEntityId}) is resolved by id + * instead, so pasting an id jumps straight to the matching org. */ export function getAllOrganizationsQueryOptions(pageIndex: number, searchValue: string) { - const entity = detectEntityId(searchValue); + // Normalized once, for both the cache key and the request. Keying on the raw + // value made `acme`, `acme ` and ` acme` three entries resolving to one + // identical request, so a pasted trailing space refetched a search the cache + // already held. + const search = normalizeSearchValue(searchValue); + const entity = detectEntityId(search); return queryOptions({ - queryKey: ['admin-all-organizations', searchValue, pageIndex], + queryKey: ['admin-all-organizations', search, pageIndex], queryFn: () => { if (entity?.kind === 'organization') { return getOrganizationByIdPage(entity.id); @@ -117,7 +119,7 @@ export function getAllOrganizationsQueryOptions(pageIndex: number, searchValue: if (entity?.kind === 'cluster') { return getOrganizationForClusterPage(entity.id); } - return getAllOrganizations(pageIndex, searchValue); + return getAllOrganizations(pageIndex, search); }, // Keep the previous page on screen while the next one loads. placeholderData: keepPreviousData,