Take search results to the Combined view when the match is there#761
Open
hoyla wants to merge 4 commits into
Open
Take search results to the Combined view when the match is there#761hoyla wants to merge 4 commits into
hoyla wants to merge 4 commits into
Conversation
Elasticsearch has no concept of the "Combined" view (it is assembled in the frontend from the page index), so search picks the document field with the most highlights (e.g. text, ocr.english) and forces that view via ?view=. That suppressed the Combined default and landed users in flat text/OCR even when Combined shows the same match in the view we actually want people to use (#760). For a paged document, prefer Combined whenever the search query is also reachable there, and only fall back to the search-forced flat view when it is not (lossy OCR, cross-field, or future translation matches that don't exist in the page index). To learn whether the match is reachable we probe /api/pages2/{uri}/search - the same request the PageViewer's Controls already make once Combined is active. The decision is resolved once per document load (guarded by a ref) so a user manually switching to the Text/OCR tab afterwards is respected. The switch goes through setResourceView, which the middleware syncs to the URL via replace, so there is no back-button trap. The view-choosing logic is extracted into a pure helper (resolveInitialPagedView) with unit tests.
PageViewerOrFallback is not remounted when the :uri route param changes,
so its cached page-count response belonged to the previous document.
Stepping to the next/previous search result therefore evaluated the
view-choosing logic against a stale count: if the previous document was
paged but the next one is not, the viewer briefly treated the new
document as paged and imposed the Combined view on it, mounting the page
viewer and firing page fetches that 404 ("Not Found").
Clear the cached page count when the uri changes so the decision waits
for the new document's real count before choosing a view.
Also give useHighlightNavigation's fetch a catch so a pages 404 can never
surface as an uncaught runtime error.
The previous attempt (clearing the count on uri change) was insufficient: setResponse(null) only takes effect on the next render, but the view-choosing effect runs in the same effect-flush and still read the previous document's count, so it continued to impose Combined on the next (non-paged) search result and 404. Tag the cached count with the uri it was fetched for and treat it as "not yet known" until the current document's count arrives. The view decision and the page-viewer render now both wait for the correct count instead of acting on the previous document's. Verified by driving the browser: paged result -> Combined; next result (non-paged .odt) -> its own Text view, no Combined imposed, no runtime error; previous result -> back to Combined.
hoyla
changed the title
The frontend probe hit /api/pages2/:uri/search, which is not a cheap existence check: per hit page it does a permission check, an ES multi-search, an S3 PDF download and PDFBox geometry extraction (capped at 501 pages) - and Controls re-fires the identical request once Combined is active, so every clicked paged result paid it twice, with the flat view flashing while the decision waited. Instead, getPageCount now accepts the search query and returns searchMatchesInPages alongside the count: one cheap ES count query (Pages2.hasSearchMatch) using the same query builder as the page highlighters, so quoting and stemming semantics match what Combined will actually show. A failed or unparseable query recovers to "no match" - opening the document must never break because of q. With every decision input arriving in one response, the wait/keep/set resolver states, the decidedForUri ref and the separate probe and decision effects are no longer needed: the view is decided once, inside the pageCount fetch callback, and resolveInitialPagedView shrinks to a pure function returning the view to switch to (or undefined to leave it alone). Nothing re-fires on view changes, so a manual tab switch can never be overridden, and dispatching before setResponse means no frame renders in the view being left. The fetch effect also gains a cancellation guard: without it, a late response from a superseded request (stepping quickly between results) could clobber the current document's data with a mismatched uri tag - blanking the viewer - or dispatch a view change after navigating away. An older backend simply omits the new field, which resolves to keeping the search-forced view, i.e. the pre-existing behaviour.
hoyla
changed the title
hoyla
marked this pull request as ready for review
July 16, 2026 08:05
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This change means that when you click a Giant search result for a paged document, it opens in the Combined view with the match highlighted — provided the match is actually reachable in Combined. Previously all search results took users to the Text or OCR view even when Combined was available.
Fixes #760. Background discussion in #733. Most of the the 350 lines are tests and comments rather than actual code changes.
Why
Elasticsearch has no concept of the "Combined" view — it's assembled in the frontend from the page index. Search picks the document field with the most highlights (
text,ocr.english, etc) and forces that view via?view=, which suppressed the Combined default and dropped users into flat text/OCR even when Combined would show the same match in the view we actually want people to use.How
The viewer needs to know, before choosing a view, whether the search query is reachable in the page index. Elasticsearch can't say during search itself, so the answer is folded into a request the viewer already makes:
Backend.
GET /api/pages2/:uri/pageCountaccepts an optionalqand returnssearchMatchesInPagesalongside the count —true/falsewhen a query was sent,nullotherwise. It is answered byPages2.hasSearchMatch: a single Elasticsearch count query built with the same query builder as the page highlighters, so quoting and stemming semantics match exactly what the Combined view will highlight. A failed or unparseable query recovers to "no match" — opening a document can never break because ofq. (Deliberately not answered via the existing/pages2/:uri/searchendpoint, which fans out per hit page — permission check, ES multi-search, S3 PDF download, PDFBox geometry extraction — far too heavy to use as a boolean.)Frontend. All decision inputs arrive in that one response, so the view is decided exactly once, inside the pageCount fetch callback, by a pure unit-tested helper (
resolveInitialPagedView):Nothing re-fires when the view changes, so a user switching to the Text/OCR tab afterwards is never overridden. The switch is dispatched before the page count is stored, so no frame ever renders in the view being left. It goes through
setResourceView, which the middleware syncs to the URL viareplace— no back-button trap; after a search click the URL settles on?view=combined&q=….The match-driven rule self-heals the cases where page text is not a superset of the document fields — lossy Tesseract-era OCR, cross-field matches, any future translation view: those answer "no match", so the search-forced flat view is kept and the user still lands on a real match.
Behaviour
?view=text/ocr)?view=)Robustness
PageViewerOrFallbackis not remounted when the/viewer/:uriroute param changes, so the cached page count is tagged with the uri it was fetched for and a mismatched count reads as "not yet known". Without this, stepping to the next/previous result imposed Combined on a non-paged document using the previous document's count, and 404'd.useHighlightNavigationcatches fetch failures, so a pages 404 can never surface as an uncaught runtime error.A note on shared URLs
A shared
?view=textwith no query is honoured. A shared?view=text&q=…resolves by match reachability — it's indistinguishable from a fresh search click. This is an accepted trade-off: aview+qURL never preserved the sharer's intent anyway (search navigation always focuses the first match, so it lands on match number 1 in any view). Directing a colleague to a specific passage is better addressed by snippet/selection sharing, which encodes an anchor rather than re-running a query.OCR engines
Under the OcrMyPdf engine, page text retains the born-digital layer and adds OCR, so a
text/ocrmatch is almost always also in the pages → answer "yes". Under the Tesseract engine, page text is a lossy OCR re-derivation, so a born-digitaltextmatch can be missing from the pages → answer "no" → the forced flat view is kept. The fix is correct under either engine; the per-document answer is what makes it robust.Testing
resolveInitialPagedView: 11 unit tests, green.ElasticsearchPagesITest: 10/10 green, including 5 newhasSearchMatchcases (match, no match, quoted phrases, cross-document isolation, no pages). Local caveat: this branch predates the Bump testcontainers-scala 0.41.4 -> 0.44.1 #755 testcontainers bump, so against Docker Engine 29+ the suite only runs with that bump applied (verified that way locally); CI runs it as-is.CI=true npm run buildtypechecks clean.Follow-up (out of scope)
Stepping next/previous between search results navigates without a
?view=(deliberately dropped bybuildLink), so it always lands on the Combined default without consulting match reachability. Unifying it with the click path would mean passing the stepped-to result'sfieldWithMostHighlights(already available onstate.search.currentResults) throughDocumentFooter.