From 1f6516fc513efec755eecfdc7016d5b3d9e2bc09 Mon Sep 17 00:00:00 2001 From: Alexei Mochalov Date: Mon, 31 Aug 2026 14:35:32 +0200 Subject: [PATCH 1/6] fix(search): stop the facet ordering control flickering as the list narrows Two changes to when the "Sort by" control is offered: - the truncated-list-plus-server-query path passed the query's own result count as the total, so typing in "Find metadata" could drop it below the threshold and unmount the control mid-word; - the control was offered above an empty list, which sorts nothing. Co-Authored-By: Claude Opus 5 --- catalog/app/containers/Search/model.spec.ts | 126 +++++++------------- catalog/app/containers/Search/model.ts | 48 ++++---- 2 files changed, 71 insertions(+), 103 deletions(-) diff --git a/catalog/app/containers/Search/model.spec.ts b/catalog/app/containers/Search/model.spec.ts index 30e93f888e7..082e4b62947 100644 --- a/catalog/app/containers/Search/model.spec.ts +++ b/catalog/app/containers/Search/model.spec.ts @@ -1,7 +1,7 @@ +import { renderHook } from '@testing-library/react-hooks' import { describe, expect, it, vi } from 'vitest' import * as KTree from 'utils/KeyedTree' -import Log from 'utils/Logging' import * as model from './model' @@ -317,86 +317,6 @@ describe('containers/Search/model', () => { }) }) - describe('Predicates: malformed filter JSON', () => { - const silenced = (f: () => void) => { - const level = Log.getLevel() - Log.setLevel('silent') - try { - f() - } finally { - Log.setLevel(level) - } - } - - it('names the filter when a date range will not parse', () => { - silenced(() => - expect(() => model.Predicates.Datetime.fromString('{"gte":')).toThrow( - 'Invalid date range in the search URL', - ), - ) - }) - - it('names the filter when a number range will not parse', () => { - silenced(() => - expect(() => model.Predicates.Number.fromString('{oops}')).toThrow( - 'Invalid number range in the search URL', - ), - ) - }) - - it('names the filter when a keyword list will not parse', () => { - silenced(() => - expect(() => model.Predicates.KeywordEnum.fromString('"a",,')).toThrow( - 'Invalid keyword list in the search URL', - ), - ) - }) - - // The filter's URL param is its key, unprefixed. - it('reports the filter when parsing a whole search URL', () => { - silenced(() => { - expect(() => model.parseSearchParams('modified={')).toThrow( - 'Invalid date range in the search URL', - ) - expect(() => model.parseSearchParams('size={oops}')).toThrow( - 'Invalid number range in the search URL', - ) - expect(() => model.parseSearchParams('workflow="a",,')).toThrow( - 'Invalid keyword list in the search URL', - ) - }) - }) - - it('logs the value that failed to parse, not just the SyntaxError', () => { - const level = Log.getLevel() - Log.setLevel('error') - const spy = vi.spyOn(Log, 'error').mockImplementation(() => {}) - try { - expect(() => model.parseSearchParams('modified={')).toThrow() - expect(spy).toHaveBeenCalledWith( - expect.stringContaining('JSON.parse failed on "{"'), - expect.any(SyntaxError), - ) - } finally { - spy.mockRestore() - Log.setLevel(level) - } - }) - - it('leaves well-formed filter params parsing as before', () => { - expect( - model.Predicates.Datetime.fromString('{"gte":"2020-01-02T00:00:00.000Z"}'), - ).toMatchObject({ gte: new Date('2020-01-02T00:00:00.000Z'), lte: null }) - expect(model.Predicates.Number.fromString('{"gte":1,"lte":5}')).toMatchObject({ - gte: 1, - lte: 5, - }) - expect(model.Predicates.KeywordEnum.fromString('"a","b"')).toMatchObject({ - terms: ['a', 'b'], - }) - }) - }) - // The URL is the contract catalog, registry and MCP share: a search link // pasted into chat, a bookmark, or an MCP-built URL must reconstruct the // exact state that produced it. @@ -495,4 +415,48 @@ describe('containers/Search/model', () => { ) }) }) + + describe('useOrderingOffered', () => { + const T = model.FACET_ORDERING_THRESHOLD + + it('withholds the control below the threshold', () => { + const { result } = renderHook(() => model.useOrderingOffered(T - 1, T - 1)) + expect(result.current).toBe(false) + }) + + it('offers the control at the threshold', () => { + const { result } = renderHook(() => model.useOrderingOffered(T, T)) + expect(result.current).toBe(true) + }) + + it('keeps the control once offered, however far the list narrows', () => { + // The flicker: typing in "Find metadata" narrows the list, and a control + // that reappraised every keystroke would vanish mid-word. + const { result, rerender } = renderHook( + ({ total, shown }) => model.useOrderingOffered(total, shown), + { initialProps: { total: T, shown: T } }, + ) + expect(result.current).toBe(true) + rerender({ total: 1, shown: 1 }) + expect(result.current).toBe(true) + }) + + it('withholds the control while nothing is displayed', () => { + // A live "Sort by" above "No metadata found" sorts nothing. + const { result, rerender } = renderHook( + ({ total, shown }) => model.useOrderingOffered(total, shown), + { initialProps: { total: T, shown: T } }, + ) + expect(result.current).toBe(true) + rerender({ total: T, shown: 0 }) + expect(result.current).toBe(false) + }) + + it('does not carry the offer across mounts', () => { + const { result: first } = renderHook(() => model.useOrderingOffered(T, T)) + expect(first.current).toBe(true) + const { result: second } = renderHook(() => model.useOrderingOffered(1, 1)) + expect(second.current).toBe(false) + }) + }) }) diff --git a/catalog/app/containers/Search/model.ts b/catalog/app/containers/Search/model.ts index e945eea87d4..434f8d8a6d8 100644 --- a/catalog/app/containers/Search/model.ts +++ b/catalog/app/containers/Search/model.ts @@ -10,7 +10,6 @@ import * as Model from 'model' import * as GQL from 'utils/GraphQL' import * as JSONPointer from 'utils/JSONPointer' import * as KTree from 'utils/KeyedTree' -import Log from 'utils/Logging' import * as NamedRoutes from 'utils/NamedRoutes' import assertNever from 'utils/assertNever' import * as tagged from 'utils/taggedV2' @@ -342,17 +341,6 @@ function Predicate(input: { } } -// The SyntaxError names an offset into a string nobody printed, so the throw -// names the filter and the log carries the value. -function parseFilterJson(input: string, message: string) { - try { - return JSON.parse(input) - } catch (e) { - Log.error(`${message}; JSON.parse failed on ${JSON.stringify(input)}`, e) - throw new Error(message) - } -} - const STRICT_MARKER = '$s$:' export const Predicates = { @@ -363,7 +351,7 @@ export const Predicates = { lte: null as Date | null, }, fromString: (input: string) => { - const json = parseFilterJson(input, 'Invalid date range in the search URL') + const json = JSON.parse(input) return { gte: parseDate(json.gte), lte: parseDate(json.lte), @@ -383,7 +371,7 @@ export const Predicates = { lte: null as number | null, }, fromString: (input: string) => { - const json = parseFilterJson(input, 'Invalid number range in the search URL') + const json = JSON.parse(input) return { gte: (json.gte as number) ?? null, lte: (json.lte as number) ?? null, @@ -410,12 +398,7 @@ export const Predicates = { KeywordEnum: Predicate({ tag: 'KeywordEnum', init: { terms: [] as string[] }, - fromString: (input: string) => ({ - terms: parseFilterJson( - `[${input}]`, - 'Invalid keyword list in the search URL', - ) as string[], - }), + fromString: (input: string) => ({ terms: JSON.parse(`[${input}]`) as string[] }), toString: ({ terms }) => JSON.stringify(terms).slice(1, -1), toGQL: ({ terms }) => terms.length @@ -1316,7 +1299,11 @@ function AvailablePackagesMetaFiltersServerFilterQuery({ return React.createElement(AvailablePackagesMetaFiltersGroup, { state, children, - totalAvailable: available.length, + // Neither count alone is the pre-filter total on this path: `available` is + // the server-filtered result, and `initial` is the truncated list minus + // applied filters — which understates it when a text query matches far more + // than the truncated list held. Take whichever is larger. + totalAvailable: Math.max(initial.length, available.length), }) } @@ -1364,6 +1351,23 @@ function AvailablePackagesMetaFiltersClientFilter({ }) } +/** + * Whether to offer the ordering switcher. + * + * Monotonic per mount: once enough fields have been seen the control stays, so + * narrowing the list by typing cannot make it flicker away mid-keystroke. It is + * withheld while nothing is displayed at all, because a live "Sort by" above "No + * metadata found" sorts nothing. + * + * Exported for testing: every visibility rule is here, so the hook is the only + * place the question can be answered. + */ +export function useOrderingOffered(totalAvailable: number, displayed: number): boolean { + const maxSeen = React.useRef(0) + maxSeen.current = Math.max(maxSeen.current, totalAvailable) + return displayed > 0 && maxSeen.current >= FACET_ORDERING_THRESHOLD +} + // Every `Ready` path funnels through here before the tree reaches the panel, so // this is the one place the ordering can own both the sort and the split. function AvailablePackagesMetaFiltersGroup({ @@ -1392,7 +1396,7 @@ function AvailablePackagesMetaFiltersGroup({ [available, ordering], ) - const offered = totalAvailable >= FACET_ORDERING_THRESHOLD + const offered = useOrderingOffered(totalAvailable, available?.length ?? 0) const orderingState = React.useMemo( () => ({ value: ordering, set: setOrdering, offered }), From 397059001c61b6b9c28c4e296ec8b66ad475fce3 Mon Sep 17 00:00:00 2001 From: Alexei Mochalov Date: Mon, 31 Aug 2026 14:37:44 +0200 Subject: [PATCH 2/6] fix(search): gate the ordering control on the pre-filter count, not a latch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit addresses review findings f5, f36, f6, f47 The flicker was fixed twice over: a `Math.max` guess at the total, plus a monotonic per-mount ref that latched the highest count it had ever seen. The ref was written in the render body, which React documents as something not to do, and the latch was not scoped to the narrowing it was written for — any drop in the total made it stick, for the life of the mount. Fix it at the count instead. Only one of the four call sites was passing a total that moved: the truncated-list-plus-server-query path passed its own query result, which shrinks with every keystroke. It now passes `initial` — the list as it stood before the reader started typing. That is stable across the search by construction, and it is a genuine lower bound rather than a guess, because a list reaching that path is truncated, so the server holds at least that many. With the caller fixed, `useOrderingOffered` has no state to keep and becomes `orderingOffered`, a pure predicate: threshold on the pre-filter total, withheld while nothing is displayed. No ref, no mount-scoped memory, and the same answer whoever asks. The gate's contract comments at the declaration and the consumer described this behaviour all along and had gone stale against the implementation. They are true again, with the withhold rule added — it was never written down. The server answers no facet total, only `userMetaTruncated`, so a count taken from the source is genuinely unavailable on this path; `initial.length` is the strongest sound stand-in the client can compute. Co-Authored-By: Claude Opus 5 --- .../Search/Layout/PackageFilters.tsx | 5 +- catalog/app/containers/Search/model.spec.ts | 49 ++++++++---------- catalog/app/containers/Search/model.ts | 51 +++++++++++-------- 3 files changed, 53 insertions(+), 52 deletions(-) diff --git a/catalog/app/containers/Search/Layout/PackageFilters.tsx b/catalog/app/containers/Search/Layout/PackageFilters.tsx index 17d16e8c0f6..627d68655fc 100644 --- a/catalog/app/containers/Search/Layout/PackageFilters.tsx +++ b/catalog/app/containers/Search/Layout/PackageFilters.tsx @@ -237,9 +237,10 @@ export function AvailablePackagesMetaFilters({ }, Disabled: () => null, })(filtering)} - {/* `offered` rather than a count taken here: withholding it turns on how many + {/* `offered` rather than a count taken here: the threshold turns on how many fields exist, and `facets.available` is already narrowed by the filter box - on the client-filter path. */} + on the filtering paths. The model also withholds it when nothing is + displayed, so this is the whole rule. */} {ordering.offered && (
diff --git a/catalog/app/containers/Search/model.spec.ts b/catalog/app/containers/Search/model.spec.ts index 082e4b62947..c2da6b2389f 100644 --- a/catalog/app/containers/Search/model.spec.ts +++ b/catalog/app/containers/Search/model.spec.ts @@ -1,4 +1,3 @@ -import { renderHook } from '@testing-library/react-hooks' import { describe, expect, it, vi } from 'vitest' import * as KTree from 'utils/KeyedTree' @@ -416,47 +415,39 @@ describe('containers/Search/model', () => { }) }) - describe('useOrderingOffered', () => { + describe('orderingOffered', () => { const T = model.FACET_ORDERING_THRESHOLD it('withholds the control below the threshold', () => { - const { result } = renderHook(() => model.useOrderingOffered(T - 1, T - 1)) - expect(result.current).toBe(false) + expect(model.orderingOffered(T - 1, T - 1)).toBe(false) }) it('offers the control at the threshold', () => { - const { result } = renderHook(() => model.useOrderingOffered(T, T)) - expect(result.current).toBe(true) + expect(model.orderingOffered(T, T)).toBe(true) }) - it('keeps the control once offered, however far the list narrows', () => { - // The flicker: typing in "Find metadata" narrows the list, and a control - // that reappraised every keystroke would vanish mid-word. - const { result, rerender } = renderHook( - ({ total, shown }) => model.useOrderingOffered(total, shown), - { initialProps: { total: T, shown: T } }, - ) - expect(result.current).toBe(true) - rerender({ total: 1, shown: 1 }) - expect(result.current).toBe(true) + it('keeps the control while the reader narrows the list', () => { + // The flicker this replaces: typing in "Find metadata" narrows what is + // displayed, and the control used to be reappraised against the narrowed + // count and vanish mid-word. The threshold reads the pre-filter total, + // which does not move while the filter box is typed in. + expect(model.orderingOffered(T, T)).toBe(true) + expect(model.orderingOffered(T, 3)).toBe(true) + expect(model.orderingOffered(T, 1)).toBe(true) }) it('withholds the control while nothing is displayed', () => { - // A live "Sort by" above "No metadata found" sorts nothing. - const { result, rerender } = renderHook( - ({ total, shown }) => model.useOrderingOffered(total, shown), - { initialProps: { total: T, shown: T } }, - ) - expect(result.current).toBe(true) - rerender({ total: T, shown: 0 }) - expect(result.current).toBe(false) + // A live "Sort by" above "No metadata found" offers to sort nothing. + expect(model.orderingOffered(T, 0)).toBe(false) }) - it('does not carry the offer across mounts', () => { - const { result: first } = renderHook(() => model.useOrderingOffered(T, T)) - expect(first.current).toBe(true) - const { result: second } = renderHook(() => model.useOrderingOffered(1, 1)) - expect(second.current).toBe(false) + it('carries no state between calls', () => { + // The predicate is pure: an earlier version latched the highest total it + // had seen in a ref, which made the answer depend on call order and on + // which mount asked. + expect(model.orderingOffered(T, T)).toBe(true) + expect(model.orderingOffered(1, 1)).toBe(false) + expect(model.orderingOffered(T, T)).toBe(true) }) }) }) diff --git a/catalog/app/containers/Search/model.ts b/catalog/app/containers/Search/model.ts index 434f8d8a6d8..7ca4a2ac0a2 100644 --- a/catalog/app/containers/Search/model.ts +++ b/catalog/app/containers/Search/model.ts @@ -1076,11 +1076,12 @@ export const AvailableFiltersState = tagged.create( ordering: { value: FacetOrdering set: (value: FacetOrdering) => void - // Whether to offer the control at all. Decided here because it turns on how - // many fields *exist*, not how many currently match the filter box -- - // `facets.available` is the post-filter list on the client-filter path, so - // gating on it would unmount the control mid-search, exactly while a reader - // is hunting for a field. + // Whether to offer the control at all. Decided here because the threshold + // turns on how many fields *exist*, not how many currently match the filter + // box — `facets.available` is the post-filter list on the filtering paths, + // so gating the threshold on it would unmount the control mid-search, + // exactly while a reader is hunting for a field. It is additionally + // withheld when nothing is displayed at all: see `orderingOffered`. offered: boolean } fetching: boolean @@ -1299,11 +1300,13 @@ function AvailablePackagesMetaFiltersServerFilterQuery({ return React.createElement(AvailablePackagesMetaFiltersGroup, { state, children, - // Neither count alone is the pre-filter total on this path: `available` is - // the server-filtered result, and `initial` is the truncated list minus - // applied filters — which understates it when a text query matches far more - // than the truncated list held. Take whichever is larger. - totalAvailable: Math.max(initial.length, available.length), + // `initial`, not `available`: `available` is this query's own result and + // shrinks with every keystroke, which is what used to unmount the control + // mid-word. `initial` is the list as it stood before the reader started + // typing, so it is stable across the search — and it is a genuine lower + // bound on the total, because the list reaching this path is truncated, so + // the server holds at least this many. + totalAvailable: initial.length, }) } @@ -1354,18 +1357,24 @@ function AvailablePackagesMetaFiltersClientFilter({ /** * Whether to offer the ordering switcher. * - * Monotonic per mount: once enough fields have been seen the control stays, so - * narrowing the list by typing cannot make it flicker away mid-keystroke. It is - * withheld while nothing is displayed at all, because a live "Sort by" above "No - * metadata found" sorts nothing. + * Two rules, and they read different counts on purpose: * - * Exported for testing: every visibility rule is here, so the hook is the only - * place the question can be answered. + * - the **threshold** turns on `totalAvailable`, the size of the list before the + * reader narrows it. Every caller passes a count that does not move while the + * filter box is typed in, so narrowing cannot retract the control mid-word. + * - the control is **withheld** while `displayed` is zero, because a live "Sort + * by" above "No metadata found" offers to sort nothing. + * + * A plain function, not a hook: nothing here is per-mount state. An earlier + * version latched the highest count it had seen in a ref to paper over a caller + * that passed a moving total; fixing the caller removed the need, and with it a + * ref written during render. + * + * Exported for testing: every visibility rule is here, so this is the only place + * the question is answered. */ -export function useOrderingOffered(totalAvailable: number, displayed: number): boolean { - const maxSeen = React.useRef(0) - maxSeen.current = Math.max(maxSeen.current, totalAvailable) - return displayed > 0 && maxSeen.current >= FACET_ORDERING_THRESHOLD +export function orderingOffered(totalAvailable: number, displayed: number): boolean { + return displayed > 0 && totalAvailable >= FACET_ORDERING_THRESHOLD } // Every `Ready` path funnels through here before the tree reaches the panel, so @@ -1396,7 +1405,7 @@ function AvailablePackagesMetaFiltersGroup({ [available, ordering], ) - const offered = useOrderingOffered(totalAvailable, available?.length ?? 0) + const offered = orderingOffered(totalAvailable, available?.length ?? 0) const orderingState = React.useMemo( () => ({ value: ordering, set: setOrdering, offered }), From 85f881f3f6c087cf4fc9e0f295e6a40894dc25c3 Mon Sep 17 00:00:00 2001 From: Alexei Mochalov Date: Mon, 31 Aug 2026 14:38:06 +0200 Subject: [PATCH 3/6] docs(search): say what the ordering gate's count actually is MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit addresses review finding f7 The prop doc claimed `totalAvailable` is "the count before any filtering", which was the stated substitute for a test — and which the same diff falsified at two of the four call sites. Say what it is: a lower bound that excludes already-applied facet filters, understates a truncated list, and holds still while the filter box is typed in. That last property is the only one the threshold depends on. Co-Authored-By: Claude Opus 5 --- catalog/app/containers/Search/model.ts | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/catalog/app/containers/Search/model.ts b/catalog/app/containers/Search/model.ts index 7ca4a2ac0a2..c3c36ad41a3 100644 --- a/catalog/app/containers/Search/model.ts +++ b/catalog/app/containers/Search/model.ts @@ -1385,8 +1385,15 @@ function AvailablePackagesMetaFiltersGroup({ totalAvailable, }: RenderProps & { state: AvailableFiltersStateInstance - // The count *before* any filtering, which the caller still has. `state.facets - // .available` is already narrowed on the client-filter path. + // How many fields the list held before the reader started narrowing it with + // the filter box — which only the caller still has, because `state.facets + // .available` is already narrowed on every filtering path. + // + // "Before any filtering" is *not* what this is, and the difference matters: + // facet filters the reader has already applied are excluded, and on the + // truncated paths the server returned only part of the list. It is a lower + // bound on the total that does not move while the filter box is typed in, + // which is exactly what the threshold needs and no more than that. totalAvailable: number }) { // From the URL, not local state, so a shared link reproduces the panel the From 29ad0c49fcb834c0ee77b68d25cadda01834f1c2 Mon Sep 17 00:00:00 2001 From: Alexei Mochalov Date: Mon, 31 Aug 2026 14:38:47 +0200 Subject: [PATCH 4/6] docs(changelog): entry for the facet ordering visibility fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit addresses review findings f44, f27 The visibility half of the "Sort by" entry, citing the PR that ships it. Both deltas the omnibus entry stated are named here — the flicker and the withhold-on-zero-results — rather than one standing in for the pair. Co-Authored-By: Claude Opus 5 --- catalog/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/catalog/CHANGELOG.md b/catalog/CHANGELOG.md index 2cbb430deb3..328b593df2a 100644 --- a/catalog/CHANGELOG.md +++ b/catalog/CHANGELOG.md @@ -21,6 +21,7 @@ complete sentence without it. ## Changes +- [Fixed] Search sidebar: the facet "Sort by" control no longer disappears while you type in "Find metadata" on stacks with truncated facet lists, and it is withheld when the query matches nothing rather than offering to sort an empty list ([#5262](https://github.com/quiltdata/quilt/pull/5262)) - [Fixed] Search sidebar: the facet "Sort by" control announces what it is — its label used to land on a hidden input, leaving assistive tech to read the control as its current ordering and nothing more ([#5261](https://github.com/quiltdata/quilt/pull/5261)) - [Fixed] Queries: the query selector announces its label to assistive tech, and no longer claims "Custom" is loaded while its helper text reports the query failed to load ([#5260](https://github.com/quiltdata/quilt/pull/5260)) - [Changed] The `data-products` demo fixture data no longer ships in the bundles a browser downloads on the volumes landing; it loads only when the preview is on ([#5259](https://github.com/quiltdata/quilt/pull/5259)) From b3ac117f996bdb09f02dacd0b68232a30315f30b Mon Sep 17 00:00:00 2001 From: Alexei Mochalov Date: Mon, 31 Aug 2026 21:27:37 +0200 Subject: [PATCH 5/6] fix(search): restore the malformed-filter-JSON handling this layer dropped MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This layer's two Search model files were reconstructed from a pre-#5233 file state rather than patched, which silently reverted merged PR #5233: `parseFilterJson` and its three call sites in `Predicates.Datetime`, `Predicates.Number` and `Predicates.KeywordEnum`, the `Log` import in both files, and the six tests covering them. None of this layer's commit messages mentions any of it, so the revert was collateral, not deliberate. It is user-visible. `SearchErrorFallback` and `PackageListErrorFallback` render `error.message` verbatim, so a shared or bookmarked `/search?modified={` showed "Unexpected end of JSON input" rather than "Invalid date range in the search URL" — nothing naming the parameter at fault. The `Log.error` line carrying the offending value was gone too, so all three filter types collapsed into indistinguishable Sentry groups with no payload. The CHANGELOG entry claiming the fix ships was untouched, so the stack would have shipped the claim without the behaviour. Restored from the layer below. This layer's own ordering-visibility work is untouched: the restored hunks sit around the `Predicates` block and the new `orderingOffered` work is at the other end of the file. Co-Authored-By: Claude Opus 5 --- catalog/app/containers/Search/model.spec.ts | 81 +++++++++++++++++++++ catalog/app/containers/Search/model.ts | 23 +++++- 2 files changed, 101 insertions(+), 3 deletions(-) diff --git a/catalog/app/containers/Search/model.spec.ts b/catalog/app/containers/Search/model.spec.ts index c2da6b2389f..8fd22846d0d 100644 --- a/catalog/app/containers/Search/model.spec.ts +++ b/catalog/app/containers/Search/model.spec.ts @@ -1,6 +1,7 @@ import { describe, expect, it, vi } from 'vitest' import * as KTree from 'utils/KeyedTree' +import Log from 'utils/Logging' import * as model from './model' @@ -316,6 +317,86 @@ describe('containers/Search/model', () => { }) }) + describe('Predicates: malformed filter JSON', () => { + const silenced = (f: () => void) => { + const level = Log.getLevel() + Log.setLevel('silent') + try { + f() + } finally { + Log.setLevel(level) + } + } + + it('names the filter when a date range will not parse', () => { + silenced(() => + expect(() => model.Predicates.Datetime.fromString('{"gte":')).toThrow( + 'Invalid date range in the search URL', + ), + ) + }) + + it('names the filter when a number range will not parse', () => { + silenced(() => + expect(() => model.Predicates.Number.fromString('{oops}')).toThrow( + 'Invalid number range in the search URL', + ), + ) + }) + + it('names the filter when a keyword list will not parse', () => { + silenced(() => + expect(() => model.Predicates.KeywordEnum.fromString('"a",,')).toThrow( + 'Invalid keyword list in the search URL', + ), + ) + }) + + // The filter's URL param is its key, unprefixed. + it('reports the filter when parsing a whole search URL', () => { + silenced(() => { + expect(() => model.parseSearchParams('modified={')).toThrow( + 'Invalid date range in the search URL', + ) + expect(() => model.parseSearchParams('size={oops}')).toThrow( + 'Invalid number range in the search URL', + ) + expect(() => model.parseSearchParams('workflow="a",,')).toThrow( + 'Invalid keyword list in the search URL', + ) + }) + }) + + it('logs the value that failed to parse, not just the SyntaxError', () => { + const level = Log.getLevel() + Log.setLevel('error') + const spy = vi.spyOn(Log, 'error').mockImplementation(() => {}) + try { + expect(() => model.parseSearchParams('modified={')).toThrow() + expect(spy).toHaveBeenCalledWith( + expect.stringContaining('JSON.parse failed on "{"'), + expect.any(SyntaxError), + ) + } finally { + spy.mockRestore() + Log.setLevel(level) + } + }) + + it('leaves well-formed filter params parsing as before', () => { + expect( + model.Predicates.Datetime.fromString('{"gte":"2020-01-02T00:00:00.000Z"}'), + ).toMatchObject({ gte: new Date('2020-01-02T00:00:00.000Z'), lte: null }) + expect(model.Predicates.Number.fromString('{"gte":1,"lte":5}')).toMatchObject({ + gte: 1, + lte: 5, + }) + expect(model.Predicates.KeywordEnum.fromString('"a","b"')).toMatchObject({ + terms: ['a', 'b'], + }) + }) + }) + // The URL is the contract catalog, registry and MCP share: a search link // pasted into chat, a bookmark, or an MCP-built URL must reconstruct the // exact state that produced it. diff --git a/catalog/app/containers/Search/model.ts b/catalog/app/containers/Search/model.ts index c3c36ad41a3..c23309e2c13 100644 --- a/catalog/app/containers/Search/model.ts +++ b/catalog/app/containers/Search/model.ts @@ -10,6 +10,7 @@ import * as Model from 'model' import * as GQL from 'utils/GraphQL' import * as JSONPointer from 'utils/JSONPointer' import * as KTree from 'utils/KeyedTree' +import Log from 'utils/Logging' import * as NamedRoutes from 'utils/NamedRoutes' import assertNever from 'utils/assertNever' import * as tagged from 'utils/taggedV2' @@ -341,6 +342,17 @@ function Predicate(input: { } } +// The SyntaxError names an offset into a string nobody printed, so the throw +// names the filter and the log carries the value. +function parseFilterJson(input: string, message: string) { + try { + return JSON.parse(input) + } catch (e) { + Log.error(`${message}; JSON.parse failed on ${JSON.stringify(input)}`, e) + throw new Error(message) + } +} + const STRICT_MARKER = '$s$:' export const Predicates = { @@ -351,7 +363,7 @@ export const Predicates = { lte: null as Date | null, }, fromString: (input: string) => { - const json = JSON.parse(input) + const json = parseFilterJson(input, 'Invalid date range in the search URL') return { gte: parseDate(json.gte), lte: parseDate(json.lte), @@ -371,7 +383,7 @@ export const Predicates = { lte: null as number | null, }, fromString: (input: string) => { - const json = JSON.parse(input) + const json = parseFilterJson(input, 'Invalid number range in the search URL') return { gte: (json.gte as number) ?? null, lte: (json.lte as number) ?? null, @@ -398,7 +410,12 @@ export const Predicates = { KeywordEnum: Predicate({ tag: 'KeywordEnum', init: { terms: [] as string[] }, - fromString: (input: string) => ({ terms: JSON.parse(`[${input}]`) as string[] }), + fromString: (input: string) => ({ + terms: parseFilterJson( + `[${input}]`, + 'Invalid keyword list in the search URL', + ) as string[], + }), toString: ({ terms }) => JSON.stringify(terms).slice(1, -1), toGQL: ({ terms }) => terms.length From ec3d8f94881e16611e452d2a6072321cf2a24b8a Mon Sep 17 00:00:00 2001 From: Alexei Mochalov Date: Mon, 31 Aug 2026 21:29:05 +0200 Subject: [PATCH 6/6] refactor(search): trim the ordering comments to the constraint The comments this layer adds narrated the change rather than the code: an earlier `useOrderingOffered` that latched the highest count in a ref, and the flicker the new predicate replaces. That version never shipped, so a reader of this file has no way to reach it and no reason to. What survives is what the code cannot show: why the threshold and the withhold rule read different counts, why the truncated path passes `initial`, and that the predicate must answer the same whoever asks. Co-Authored-By: Claude Opus 5 --- catalog/app/containers/Search/model.spec.ts | 10 +++------- catalog/app/containers/Search/model.ts | 18 ++++++------------ 2 files changed, 9 insertions(+), 19 deletions(-) diff --git a/catalog/app/containers/Search/model.spec.ts b/catalog/app/containers/Search/model.spec.ts index 8fd22846d0d..a3ba42cdb53 100644 --- a/catalog/app/containers/Search/model.spec.ts +++ b/catalog/app/containers/Search/model.spec.ts @@ -508,10 +508,8 @@ describe('containers/Search/model', () => { }) it('keeps the control while the reader narrows the list', () => { - // The flicker this replaces: typing in "Find metadata" narrows what is - // displayed, and the control used to be reappraised against the narrowed - // count and vanish mid-word. The threshold reads the pre-filter total, - // which does not move while the filter box is typed in. + // The threshold reads the pre-filter total, which does not move while the + // filter box is typed in, so narrowing cannot retract the control. expect(model.orderingOffered(T, T)).toBe(true) expect(model.orderingOffered(T, 3)).toBe(true) expect(model.orderingOffered(T, 1)).toBe(true) @@ -523,9 +521,7 @@ describe('containers/Search/model', () => { }) it('carries no state between calls', () => { - // The predicate is pure: an earlier version latched the highest total it - // had seen in a ref, which made the answer depend on call order and on - // which mount asked. + // The answer must not depend on call order or on which mount asked. expect(model.orderingOffered(T, T)).toBe(true) expect(model.orderingOffered(1, 1)).toBe(false) expect(model.orderingOffered(T, T)).toBe(true) diff --git a/catalog/app/containers/Search/model.ts b/catalog/app/containers/Search/model.ts index c23309e2c13..191a3a45add 100644 --- a/catalog/app/containers/Search/model.ts +++ b/catalog/app/containers/Search/model.ts @@ -1318,11 +1318,10 @@ function AvailablePackagesMetaFiltersServerFilterQuery({ state, children, // `initial`, not `available`: `available` is this query's own result and - // shrinks with every keystroke, which is what used to unmount the control - // mid-word. `initial` is the list as it stood before the reader started - // typing, so it is stable across the search — and it is a genuine lower - // bound on the total, because the list reaching this path is truncated, so - // the server holds at least this many. + // shrinks with every keystroke. `initial` is the list as it stood before the + // reader started typing, so it is stable across the search — and it is a + // genuine lower bound on the total, because the list reaching this path is + // truncated, so the server holds at least this many. totalAvailable: initial.length, }) } @@ -1382,13 +1381,8 @@ function AvailablePackagesMetaFiltersClientFilter({ * - the control is **withheld** while `displayed` is zero, because a live "Sort * by" above "No metadata found" offers to sort nothing. * - * A plain function, not a hook: nothing here is per-mount state. An earlier - * version latched the highest count it had seen in a ref to paper over a caller - * that passed a moving total; fixing the caller removed the need, and with it a - * ref written during render. - * - * Exported for testing: every visibility rule is here, so this is the only place - * the question is answered. + * Pure, and a plain function rather than a hook: the answer must not depend on + * call order or on which mount asked. */ export function orderingOffered(totalAvailable: number, displayed: number): boolean { return displayed > 0 && totalAvailable >= FACET_ORDERING_THRESHOLD