From d07050aeb08ed423c1c3cecd1d58b6e82f066907 Mon Sep 17 00:00:00 2001 From: ARMAN S Date: Tue, 30 Jun 2026 17:29:54 +0530 Subject: [PATCH 1/4] feat(search): add count + loading state to OlSelectPopover Extends item shape from {value, label} to {value, label, count?}. Renders count as a muted, comma-formatted trailing number when present. Adds host-driven loading boolean attribute, swaps suggestions list for a spinner while true. No fetch or domain logic added - stays a pure UI component per the building-components architecture guide. Part of #13060 --- openlibrary/components/lit/OlSelectPopover.js | 53 ++++++++++++++++++- .../components/lit/custom-elements.json | 19 +++++++ 2 files changed, 71 insertions(+), 1 deletion(-) diff --git a/openlibrary/components/lit/OlSelectPopover.js b/openlibrary/components/lit/OlSelectPopover.js index c51115b03e2..56b68fc3285 100644 --- a/openlibrary/components/lit/OlSelectPopover.js +++ b/openlibrary/components/lit/OlSelectPopover.js @@ -84,6 +84,7 @@ export class OlSelectPopover extends FocusableHostMixin(LitElement) { clearLabel: { type: String, attribute: 'clear-label' }, noMatchesLabel: { type: String, attribute: 'no-matches-label' }, _query: { state: true }, + loading: { type: Boolean, reflect: true }, }; static styles = css` @@ -365,6 +366,45 @@ export class OlSelectPopover extends FocusableHostMixin(LitElement) { outline: var(--focus-width) solid var(--color-focus-ring); outline-offset: 2px; } + + /* ── Item count ──────────────────────────────────────────────── */ + + .item-count { + margin-left: auto; + flex-shrink: 0; + color: var(--accessible-grey); + font-size: var(--font-size-label-medium); + font-variant-numeric: tabular-nums; + } + + /* ── Host-driven loading state ───────────────────────────────────────── */ + + @keyframes ol-sp-spin { + to { transform: rotate(360deg); } + } + + .loading-row { + display: flex; + align-items: center; + justify-content: center; + gap: var(--spacing-inline-sm); + padding: var(--spacing-inset-md); + color: var(--accessible-grey); + font-size: var(--font-size-body-medium); + } + .loading-spinner { + width: 14px; + height: 14px; + border: 2px solid var(--color-border-subtle); + border-top-color: var(--accessible-grey); + border-radius: 50%; + flex-shrink: 0; + animation: ol-sp-spin 0.65s linear infinite; + } + + @media (prefers-reduced-motion: reduce) { + .loading-spinner { animation: none; opacity: 0.5; } + } `; /** Chevron icon for the default trigger */ @@ -396,6 +436,7 @@ export class OlSelectPopover extends FocusableHostMixin(LitElement) { // re-homes the item between the selected/suggestions groups (which // destroys its DOM node, so its focus is lost). this._restoreFocusToValue = null; + this.loading = false; } /** @@ -522,6 +563,13 @@ export class OlSelectPopover extends FocusableHostMixin(LitElement) { ` : nothing}
+ ${this.loading + ? html` +
+ + Loading… +
` + : html` + `}
${hasSelected ? html`
@@ -559,6 +607,9 @@ export class OlSelectPopover extends FocusableHostMixin(LitElement) { @change=${this._onItemToggle} /> ${item.label} + ${item.count !== null && item.count !== undefined + ? html`` + : nothing} `; diff --git a/openlibrary/components/lit/custom-elements.json b/openlibrary/components/lit/custom-elements.json index a87b6b3be56..54581f245fa 100644 --- a/openlibrary/components/lit/custom-elements.json +++ b/openlibrary/components/lit/custom-elements.json @@ -3344,6 +3344,17 @@ }, "default": "null" }, + { + "kind": "field", + "name": "loading", + "privacy": "public", + "type": { + "text": "boolean" + }, + "default": "false", + "attribute": "loading", + "reflects": true + }, { "kind": "field", "name": "shadowRootOptions", @@ -3489,6 +3500,14 @@ }, "default": "''", "fieldName": "_query" + }, + { + "name": "loading", + "type": { + "text": "boolean" + }, + "default": "false", + "fieldName": "loading" } ], "mixins": [ From dc61f690e5e93d1fbb0fb6ed955b67ed4a4b7129 Mon Sep 17 00:00:00 2001 From: ARMAN S Date: Tue, 30 Jun 2026 17:30:53 +0530 Subject: [PATCH 2/4] test(search): unit tests for searchFacets and mergeFacetCounts --- .../plugins/openlibrary/js/SearchFilterBar.js | 92 +++++++-- .../js/search-modal/searchFacets.js | 36 ++++ tests/unit/js/searchFacets.test.js | 178 ++++++++++++++++++ 3 files changed, 289 insertions(+), 17 deletions(-) create mode 100644 openlibrary/plugins/openlibrary/js/search-modal/searchFacets.js create mode 100644 tests/unit/js/searchFacets.test.js diff --git a/openlibrary/plugins/openlibrary/js/SearchFilterBar.js b/openlibrary/plugins/openlibrary/js/SearchFilterBar.js index 4092a8503d2..9124b045e72 100644 --- a/openlibrary/plugins/openlibrary/js/SearchFilterBar.js +++ b/openlibrary/plugins/openlibrary/js/SearchFilterBar.js @@ -21,6 +21,8 @@ * bar — the user gets the filters they last set in this session. * * The full language catalogue is fetched lazily on first popover open. + * Context-aware facet counts are fetched + * in parallel with the catalogue whenever there is an active search query. */ import { @@ -35,6 +37,7 @@ import { readStoredLanguages, } from './search-modal/constants.js'; import { fetchLanguageOptions } from './search-modal/languages.js'; +import { fetchFacetCounts } from './search-modal/searchFacets.js'; import { trackEvent } from './ol.analytics.js'; // Every query param the availability filter owns, across all of its values. @@ -44,6 +47,35 @@ const AVAILABILITY_PARAM_KEYS = [ ...new Set(Object.values(AVAILABILITY_TO_PARAMS).flatMap(Object.keys)), ]; +// ── Facet field config ───────────────────────────────────────────────────── +// +// Maps each OlSelectPopover to its Solr facet field name (validated server-side +// against WorkSearchScheme.facet_fields). +/** @type {Map} */ +let POPOVER_FIELD_CONFIG; + +// ── Merge helpers ────────────────────────────────────────────────────────── + +/** + * Merge context-aware facet counts from the API into the item list, then + * sort and filter according to the issue spec: + * @param {Array<{value: string, label: string}>} items - Full catalogue list. + * @param {Array<{value: string, count: number}>} counts - API response. + * @param {string[]} selectedValues - Currently selected item values. + * @returns {Array<{value: string, label: string, count: number}>} + */ +export function mergeFacetCounts(items, counts, selectedValues) { + const countMap = new Map(counts.map(c => [c.value, c.count])); + const selectedSet = new Set(selectedValues); + + return items + .map(it => ({ ...it, count: countMap.get(it.value) ?? 0 })) + .filter(it => it.count > 0 || selectedSet.has(it.value)) + .sort((a, b) => b.count - a.count); +} + +// ── sessionStorage helpers ───────────────────────────────────────────────── + function writeStoredAvailability(value) { ssSet(SS_AVAILABILITY_KEY, value || DEFAULT_AVAILABILITY); } @@ -127,11 +159,18 @@ export function initSearchFilterBar(container) { const availabilityEl = container.querySelector('ol-toggle'); const languageEl = container.querySelector('ol-select-popover'); + // Build the facet field config now that we have element references. + // Future filters: add an entry here. + POPOVER_FIELD_CONFIG = new Map([ + ...(languageEl ? [[languageEl, 'language']] : []), + ]); + + // True when the page has a meaningful search query — facet counts are only + // fetched in this case. An empty or whitespace-only q= is treated as no + // query (popularity-sorted catalogue, no per-query counts). + const hasQuery = (currentParams.get('q') || '').trim().length > 0; + if (availabilityEl) { - // Binary availability: the toggle reads as "on" whenever any - // readable-scoped filter is in the URL (readable / open / borrowable), - // and "off" for the all-books default. Flipping it on applies the broad - // `readable` filter; flipping it off clears every availability param. availabilityEl.checked = availabilityFromParams((name) => currentParams.get(name)) !== DEFAULT_AVAILABILITY; availabilityEl.addEventListener('ol-toggle-change', (e) => { @@ -144,8 +183,6 @@ export function initSearchFilterBar(container) { Object.entries(mapped).forEach(([key, val]) => params.set(key, val)); }); }); - // The readable-count sublabel is rendered server-side (work_search.html), - // so it's already present on first paint — nothing to fetch here. } if (languageEl) { @@ -154,17 +191,38 @@ export function initSearchFilterBar(container) { languageEl.items = DEFAULT_LANGUAGE_OPTIONS; languageEl.selected = currentParams.getAll('language'); - // Defer fetching the full catalogue list until the popover first opens. - // Most searches never touch the language filter, so this avoids the - // /languages.json request entirely for them. ol-popover-open bubbles - // (composed) out of the popover's shadow root up to this host. - let languagesLoaded = false; - languageEl.addEventListener('ol-popover-open', () => { - if (languagesLoaded) return; - languagesLoaded = true; - fetchLanguageOptions().then((options) => { - languageEl.items = options; - }); + // Defer fetching the full catalogue + context-aware counts until the + // popover first opens. Most searches never touch the language filter, + // avoiding both the /languages.json and /search/facets.json requests. + // On subsequent opens of the same dropper (same page load / same query): + // counts are already merged into items; nothing to re-fetch. + let loaded = false; + + languageEl.addEventListener('ol-popover-open', async() => { + if (loaded) return; + loaded = true; + + const field = POPOVER_FIELD_CONFIG.get(languageEl); + languageEl.loading = true; + + try { + // Fetch catalogue and counts in parallel to minimise latency. + const [options, counts] = await Promise.all([ + fetchLanguageOptions(), + hasQuery && field + ? fetchFacetCounts(field, currentParams) + : Promise.resolve([]), + ]); + + languageEl.items = (hasQuery && counts.length > 0) + ? mergeFacetCounts(options, counts, languageEl.selected || []) + : options; + } catch (_err) { + // Graceful degradation: keep DEFAULT_LANGUAGE_OPTIONS seeded at + // init. Filtering must never break (spec requirement). + } finally { + languageEl.loading = false; + } }); languageEl.addEventListener('ol-select-popover-change', (e) => { diff --git a/openlibrary/plugins/openlibrary/js/search-modal/searchFacets.js b/openlibrary/plugins/openlibrary/js/search-modal/searchFacets.js new file mode 100644 index 00000000000..5c841ac5de2 --- /dev/null +++ b/openlibrary/plugins/openlibrary/js/search-modal/searchFacets.js @@ -0,0 +1,36 @@ +/** + * Service module — context-aware facet counts. + * + * Sole owner of the /search/facets.json URL and response shape. + * Mirrors the fetchLanguageOptions() pattern in search-modal/languages.js. + */ + +const FACETS_URL = '/search/facets.json'; + +/** + * Fetch context-aware facet value counts for a single Solr field. + * + * @param {string} field - A field name from WorkSearchScheme.facet_fields, + * e.g. 'language', 'author_facet', 'subject_facet'. + * @param {URLSearchParams} searchParams - The current page search params + * (q, availability flags, etc.) forwarded as-is to the backend. + * @returns {Promise>} + * Count-descending list of {value, count} pairs (count > 0 only). + */ +export async function fetchFacetCounts(field, searchParams) { + const params = new URLSearchParams(searchParams); + params.set('field', field); + + const response = await fetch(`${FACETS_URL}?${params.toString()}`); + if (!response.ok) { + throw new Error(`fetchFacetCounts: HTTP ${response.status} for field="${field}"`); + } + + const data = await response.json(); + + // The API returns either shape for forward-compatibility with multi-field + // Flat array (single-field): [{value, count}, ...] + // Field-keyed map (multi): { "language": [{value, count}], ... } + if (Array.isArray(data)) return data; + return data[field] ?? []; +} diff --git a/tests/unit/js/searchFacets.test.js b/tests/unit/js/searchFacets.test.js new file mode 100644 index 00000000000..a1be29c19e9 --- /dev/null +++ b/tests/unit/js/searchFacets.test.js @@ -0,0 +1,178 @@ +/** + * Unit tests for: + * - fetchFacetCounts() (searchFacets.js) + * - mergeFacetCounts() (SearchFilterBar.js) + * + * Run with: jest (or vitest — both work without config changes) + * + * Issue: #13060 — feat(search): Wire facet droppers to context-aware counts + */ + +import { fetchFacetCounts } from '../../../openlibrary/plugins/openlibrary/js/search-modal/searchFacets.js'; +import { mergeFacetCounts } from '../../../openlibrary/plugins/openlibrary/js/SearchFilterBar.js'; +// ───────────────────────────────────────────────────────────────────────────── +// fetchFacetCounts +// ───────────────────────────────────────────────────────────────────────────── + +describe('fetchFacetCounts', () => { + const MOCK_FLAT = [ + { value: 'English', count: 665 }, + { value: 'German', count: 32 }, + { value: 'Spanish', count: 18 }, + ]; + + beforeEach(() => { + jest.resetAllMocks(); + }); + + test('calls /search/facets.json with field + forwarded search params', async() => { + global.fetch = jest.fn().mockResolvedValue({ + ok: true, + json: async() => MOCK_FLAT, + }); + + const params = new URLSearchParams('q=lord+of+the+rings&sort=new'); + await fetchFacetCounts('language', params); + + const url = global.fetch.mock.calls[0][0]; + expect(url).toContain('/search/facets.json'); + expect(url).toContain('field=language'); + expect(url).toContain('q=lord+of+the+rings'); + }); + + test('returns a flat array when the API responds with a flat array', async() => { + global.fetch = jest.fn().mockResolvedValue({ + ok: true, + json: async() => MOCK_FLAT, + }); + + const result = await fetchFacetCounts('language', new URLSearchParams('q=foo')); + expect(result).toEqual(MOCK_FLAT); + }); + + test('unwraps a field-keyed map when the API responds with the multi-field shape', async() => { + const multiShape = { language: MOCK_FLAT, author_facet: [] }; + global.fetch = jest.fn().mockResolvedValue({ + ok: true, + json: async() => multiShape, + }); + + const result = await fetchFacetCounts('language', new URLSearchParams('q=foo')); + expect(result).toEqual(MOCK_FLAT); + }); + + test('returns [] when a field-keyed map does not contain the requested field', async() => { + global.fetch = jest.fn().mockResolvedValue({ + ok: true, + json: async() => ({ author_facet: [{ value: 'Tolkien', count: 12 }] }), + }); + + const result = await fetchFacetCounts('language', new URLSearchParams('q=foo')); + expect(result).toEqual([]); + }); + + test('throws on a non-2xx HTTP response', async() => { + global.fetch = jest.fn().mockResolvedValue({ ok: false, status: 500 }); + + await expect( + fetchFacetCounts('language', new URLSearchParams('q=foo')) + ).rejects.toThrow('HTTP 500'); + }); + + test('propagates network errors', async() => { + global.fetch = jest.fn().mockRejectedValue(new TypeError('Failed to fetch')); + + await expect( + fetchFacetCounts('language', new URLSearchParams('q=foo')) + ).rejects.toThrow('Failed to fetch'); + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// mergeFacetCounts +// ───────────────────────────────────────────────────────────────────────────── + +describe('mergeFacetCounts', () => { + // Fixture catalogue (what fetchLanguageOptions() returns) + const ITEMS = [ + { value: 'English', label: 'English' }, + { value: 'German', label: 'German' }, + { value: 'Spanish', label: 'Spanish' }, + { value: 'French', label: 'French' }, + { value: 'Dutch', label: 'Dutch' }, + ]; + + // Fixture counts (what fetchFacetCounts() returns) + const COUNTS = [ + { value: 'English', count: 665 }, + { value: 'German', count: 32 }, + { value: 'Spanish', count: 18 }, + // French and Dutch are absent → 0 results + ]; + + test('attaches counts to matching items', () => { + const result = mergeFacetCounts(ITEMS, COUNTS, []); + const english = result.find(it => it.value === 'English'); + expect(english).toBeDefined(); + expect(english.count).toBe(665); + }); + + test('hides zero-count items when they are not selected', () => { + const result = mergeFacetCounts(ITEMS, COUNTS, []); + const values = result.map(it => it.value); + expect(values).not.toContain('French'); + expect(values).not.toContain('Dutch'); + }); + + test('keeps a zero-count item that is currently selected', () => { + const result = mergeFacetCounts(ITEMS, COUNTS, ['French']); + const french = result.find(it => it.value === 'French'); + expect(french).toBeDefined(); + expect(french.count).toBe(0); + }); + + test('sorts by count descending', () => { + const result = mergeFacetCounts(ITEMS, COUNTS, []); + const counts = result.map(it => it.count); + expect(counts).toEqual([...counts].sort((a, b) => b - a)); + }); + + test('zero-count selected item sorts last (count=0)', () => { + const result = mergeFacetCounts(ITEMS, COUNTS, ['French']); + const last = result[result.length - 1]; + expect(last.value).toBe('French'); + expect(last.count).toBe(0); + }); + + test('preserves original label for each item', () => { + const result = mergeFacetCounts(ITEMS, COUNTS, []); + const german = result.find(it => it.value === 'German'); + expect(german.label).toBe('German'); + }); + + test('returns empty array when counts is empty', () => { + // No query → caller passes [] → falls back to uncounted list. + // This path is never called through merge (caller uses the raw + // options list directly), but we guard it anyway. + const result = mergeFacetCounts(ITEMS, [], []); + expect(result).toHaveLength(0); + }); + + test('returns empty array when items is empty', () => { + const result = mergeFacetCounts([], COUNTS, []); + expect(result).toHaveLength(0); + }); + + test('does not mutate the original items array', () => { + const original = ITEMS.map(it => ({ ...it })); + mergeFacetCounts(ITEMS, COUNTS, []); + expect(ITEMS).toEqual(original); + }); + + test('handles multiple selected zero-count items', () => { + const result = mergeFacetCounts(ITEMS, COUNTS, ['French', 'Dutch']); + const retained = result.filter(it => it.count === 0).map(it => it.value); + expect(retained).toContain('French'); + expect(retained).toContain('Dutch'); + }); +}); From 695820190d04e465682a830531da273187d271cf Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Tue, 30 Jun 2026 12:12:26 +0000 Subject: [PATCH 3/4] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- openlibrary/components/lit/OlSelectPopover.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openlibrary/components/lit/OlSelectPopover.js b/openlibrary/components/lit/OlSelectPopover.js index 56b68fc3285..89ab843ec27 100644 --- a/openlibrary/components/lit/OlSelectPopover.js +++ b/openlibrary/components/lit/OlSelectPopover.js @@ -366,7 +366,7 @@ export class OlSelectPopover extends FocusableHostMixin(LitElement) { outline: var(--focus-width) solid var(--color-focus-ring); outline-offset: 2px; } - + /* ── Item count ──────────────────────────────────────────────── */ .item-count { From 1e00aec911c5fa599c4b0beaafbedb92ce7eba83 Mon Sep 17 00:00:00 2001 From: ARMAN S Date: Tue, 7 Jul 2026 00:49:23 +0530 Subject: [PATCH 4/4] fix(search): match facet counts by label not value --- openlibrary/plugins/openlibrary/js/SearchFilterBar.js | 4 ++-- tests/unit/js/searchFacets.test.js | 7 +++---- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/openlibrary/plugins/openlibrary/js/SearchFilterBar.js b/openlibrary/plugins/openlibrary/js/SearchFilterBar.js index 9124b045e72..fb79e6801de 100644 --- a/openlibrary/plugins/openlibrary/js/SearchFilterBar.js +++ b/openlibrary/plugins/openlibrary/js/SearchFilterBar.js @@ -65,11 +65,11 @@ let POPOVER_FIELD_CONFIG; * @returns {Array<{value: string, label: string, count: number}>} */ export function mergeFacetCounts(items, counts, selectedValues) { - const countMap = new Map(counts.map(c => [c.value, c.count])); + const countMap = new Map(counts.map(c => [c.label, c.count])); const selectedSet = new Set(selectedValues); return items - .map(it => ({ ...it, count: countMap.get(it.value) ?? 0 })) + .map(it => ({ ...it, count: countMap.get(it.label) ?? 0 })) .filter(it => it.count > 0 || selectedSet.has(it.value)) .sort((a, b) => b.count - a.count); } diff --git a/tests/unit/js/searchFacets.test.js b/tests/unit/js/searchFacets.test.js index a1be29c19e9..379abaac5bd 100644 --- a/tests/unit/js/searchFacets.test.js +++ b/tests/unit/js/searchFacets.test.js @@ -104,10 +104,9 @@ describe('mergeFacetCounts', () => { // Fixture counts (what fetchFacetCounts() returns) const COUNTS = [ - { value: 'English', count: 665 }, - { value: 'German', count: 32 }, - { value: 'Spanish', count: 18 }, - // French and Dutch are absent → 0 results + { value: 'English', label: 'English', count: 665 }, + { value: 'German', label: 'German', count: 32 }, + { value: 'Spanish', label: 'Spanish', count: 18 }, ]; test('attaches counts to matching items', () => {