Repository navigation
Wire facet droppers to context-aware counts (frontend for #12978) - #13064
Conversation
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 internetarchive#13060
for more information, see https://pre-commit.ci
|
Thanks for the contribution, @Armansiddiqui9! 🤖 Copilot has been assigned for an initial review. The linked issue (#13060) hasn't been triaged yet — triage happens on Mondays and Fridays. There are currently 27 open non-draft PRs ahead of yours. PR triage checklist (maintainers / Richy)
Note This comment was automatically generated by PAM, Open Library's Project AI Manager. PAM provides status visibility, performs basic project management functions, and gives actionable feedback so contributors aren't left waiting. |
|
This is looking great. Once the backend work is complete we'll do one more pass. At that point, if you could record a short video of the feature working, including the new loading indicator, that would be really helpful. I'm encouraging all of us to add videos to UI-related PRs. They convey a lot about the scope of a feature, get people excited, and give reviewers confidence that the change has actually been tested. |
|
See also #12980 (now sure if it implements the APIs but we likely want to treat these as 2 different PRs -- the APIs and the UI wiring) |
|
@mekarpeles agreed, treating them as separate PRs. The frontend (#13060) is ready for review independently. It degrades gracefully when the backend isn't available (falls back to the uncounted catalog), so nothing breaks. Will do full end-to-end testing once #12980 merges and record the demo video Lokesh requested at that point. |
lokesh
added
the
State: Blocked
|
Blocked on #12980 |
# Conflicts: # openlibrary/components/lit/custom-elements.json
|
@mekarpeles this PR adds counts to the search facet dropdowns. It is blocked on your work which adds a new endpoint that returns the facet counts for the search: #12980 |
|
#12980 is now merged! We can wire up the UI |
The facet counts came from a plain WorkSearchScheme, so they ignored the solr_editions setting and skipped the edition-level block-join that /search.json opts into. Availability-filtered counts could then disagree with the result set they were meant to describe. Pass solr_editions through from the request context and request the same key,editions fields, so a facet count and the search it annotates are answering the same question. Claude-Session: https://claude.ai/code/session_018u1CiBQgEQ8YEBqBf3J5Hc
The endpoint lives only in FastAPI, and local dev has no reverse proxy in front of the two servers — web.py forwards an allowlist of paths to fast_web instead. /search/facets was missing from it, so the browser got a 404 from web.py on :8080 and the language dropper silently fell back to its uncounted list. The `/search` entry only matches /search.json. Claude-Session: https://claude.ai/code/session_018u1CiBQgEQ8YEBqBf3J5Hc
The header modal's language dropper always showed the whole catalogue — only the /search filter row got counts. It now fetches the same facet counts for the query on screen. - Move mergeFacetCounts into searchFacets.js so both surfaces share it, and join on `value` (the MARC code) rather than `label`. Both sides localize their labels independently, so a label join would drop every row under some UI languages. The old fixtures used value === label, so nothing caught it. - Strip an existing filter on the field being counted. Solr ANDs an fq on the faceted field, which would zero out every other language and strand the patron on the one already ticked. - Cache the merged list per query + availability, so re-opening the dropper on an unchanged search costs nothing and ticking a checkbox doesn't refetch. - Open the dropper only once its counts are in. It used to open on a spinner, then swap in a shorter, re-sorted list — two jarring transitions under the pointer. ol-select-popover now fires a cancelable `ol-select-popover-request-open`; a host that defers calls show() when ready. Hosts that don't listen are unaffected. The wait is capped at 500ms, past which it opens on the catalogue as before. - Make the dropper's loading text translatable (`loadingLabel`) — it was the one hardcoded string left in the component. Claude-Session: https://claude.ai/code/session_018u1CiBQgEQ8YEBqBf3J5Hc
|
I pushed a few commits to this branch. Here is what they do. Why the counts were not showing up locally
Counts in the header search modal
Merging on the code, not the name
Not filtering the list you are filtering
Opening the dropper without the flash
Smaller things
Testing
|
|
I've been stuck on this for about a day, kept getting the counts to work in the unit tests, but nothing showed up in the browser locally, no errors in the console either. Traced it down to /search/facets.json not being on the list of paths web.py proxies over to FastAPI in dev, so it was 404ing silently. Added that route so the frontend could actually talk to the endpoint, which is what finally got the context-aware counts rendering on my end. Just saw your commits and reading through them now. Looks like you found the same proxy gap plus a good number of things I hadn't caught (the self-narrowing bug on multi-select, the modal counts, and the solr_editions consistency issue). Going to pull this, test it locally end-to-end, and put together the screen recording once I've confirmed everything's working. |
…s, fix watch script missing lit-components build, fix mobile tray clipped by dvh
Tested commits end to end (desktop + mobile, screenshots attached) - counts, merge-on-value, and the no-flash open all work great. Wired SearchFilterBar.js to the new shared searchFacets.js API (it was still on the old local mergeFacetCounts/ol-popover-open contract), and fixed a mobile tray clipping issue with 85dvh. |
lokesh
removed
the
State: Blocked
github-actions
Bot
added
the
Needs: Response
`watch` ran watch:js, watch:css and watch:components but never watch:lit-components, so editing anything under openlibrary/components/lit/ left static/build/lit-components/production/ol-components.js untouched. The page kept serving the previously built bundle, with no error to show for it. This bites hardest when a change spans both bundles: the search modal's facet counts need SearchModal.js (webpack) to hear an event that OlSelectPopover.js (the Lit bundle) fires, so a stale Lit bundle leaves the listener attached to an event nothing emits and the dropper opens silently uncounted. Claude-Session: https://claude.ai/code/session_011AqgrMKn29JVncut49JNtf
vite-lit.config.mjs is the only Vite config asking for `minify: 'terser'`, and terser has never been in package.json. It resolved anyway because webpack pulled it in transitively via terser-webpack-plugin, whose ^5.31.1 range is what pinned the lockfile's terser 5.31.1. Vite only ever listed it as an optional peer. That makes it a landmine for internetarchive#13331, which drops webpack for Vite: without terser-webpack-plugin the package disappears, `make lit-components` dies with "terser not found", and because the target only swaps the bundle into place on success, the previously built ol-components.js survives. Nothing in the browser looks broken; the Lit elements are simply frozen at the last good build. On testing that stranded OlSelectPopover on a revision that never fires ol-select-popover-request-open, so the search modal's facet droppers listened for an event nothing emitted and opened silently uncounted. Switch to the default minifier, matching every other Vite config here rather than adding a dependency to keep one config's choice alive. Claude-Session: https://claude.ai/code/session_011AqgrMKn29JVncut49JNtf
/search/facets.json only exists in FastAPI. Local dev proxies the bare path
through web.py, but everywhere else web.py answers it from the deprecated-
endpoint handler, which raises internalerror unless LOCAL_DEV is set. On the
testing server that is a hard 500:
DEPRECATED ENDPOINT ACCESSED: /search/facets.json. This endpoint has been
migrated to FastAPI and should not be accessed in production.
fetchFacetCounts() surfaces that as a failed load, so the dropper silently
falls back to its uncounted catalogue. Testing exposes FastAPI behind /_fast,
where the same query returns counts, so pick the prefix from the hostname the
way testingStatusUrl() already does for /status/testing.json.
Claude-Session: https://claude.ai/code/session_011AqgrMKn29JVncut49JNtf
Got it up on testing after fighting with some unrelated build issues. Looking great! I'll merge after a bit more code review. |
Resolves package-lock.json conflict by regenerating against the merged package.json (keeps master's concurrently v10 bump alongside the Vite dependency changes). Also picks up #13064's terser fix (minify: true).
Closes #13060
Depends on #12980 (backend API, currently WIP — see note below)
Feature
Makes the Language filter on
/searchreflect the current search insteadof always showing the full catalogue. When a patron has an active query,
opening the Language dropper now shows only languages with matching
results, with a count next to each, sorted by relevance, no more dead-end
selections that silently return zero books.
Technical
OlSelectPopover.js— extended item shape to{value, label, count?}.Renders count as a muted, comma-formatted trailing number. Added a
loadingattribute for a host-driven spinner state. No fetch or domainlogic added — stays a pure UI component per the
building-components guide.
search-modal/searchFacets.js(new) — service module owning the/search/facets.jsoncall. ExposesfetchFacetCounts(field, searchParams).SearchFilterBar.js— orchestration. Onol-popover-open, fetchesthe language catalogue and facet counts in parallel, merges by MARC code,
sorts descending, hides zero-count values (keeps the current selection
visible even at 0 so it can still be unticked). Falls back to the
uncounted catalogue on error or when there's no active query.
searchFacets.test.js(new) — 16 unit tests covering the servicemodule's response handling and the merge/sort/hide-zero logic. All passing.
Testing
npm run test:js— 483/483 passing (including the 16 new tests)/search/facets.json(since feat(search): add GET /search/facets.json — context-aware facet values API #12980 is still WIP) and verified againstthe real
/languages.jsoncatalogue. Confirmed counts render correctly,comma-formatted, sorted descending, zero-count items hidden, loading
state visible during fetch.
q=is empty)Open question for @RayBB
Confirmed locally that the frontend's language catalogue keys
valueonMARC code (
eng,fre,ger...) via/languages.json, not thedisplay name. Please make sure
/search/facets.json'svaluefieldreturns matching MARC codes, not full language names — otherwise the
merge in
SearchFilterBar.jswill silently match nothing and the dropperwill render empty. Happy to pair on this if useful.
Stakeholders
@lokesh @RayBB