explorer: search-results "50+" → real "N of M" count (closes #232)#236
Merged
Conversation
…org#232) The search-results line under the in-map search box used to read `50+ results for "pottery"` whenever the SELECT hit `LIMIT 50`, hiding whether the true result set was 51 or 50,000. For `pottery Cyprus` (world scope) the real count is ~7,124; users saw `50+` and had no way to gauge whether to refine, scroll, or look elsewhere. Fix follows the same pattern that closed isamplesorg#206 in `loadViewportSamples()`: when the SELECT cap is hit, fire a follow-up `COUNT(*)` with the same WHERE clause and replace the label. Below the cap, `results.length` IS the truth and we skip the second scan. Before: `50+ results for "pottery"` After: `50 of 7,124 results for "pottery"` Implementation notes: - The area-scope path can fall back to the world-mode SELECT when `viewerBboxSQL` returns null. Track which shape actually ran (`effectiveQueryShape` / `effectiveBboxSQL`) so the COUNT uses the matching WHERE/JOIN. A previous draft that keyed off `effectiveScope === 'area'` would have generated a malformed COUNT in the fallback case. - Cancellation: a second search bumps `_searchSeq`. The COUNT guards against `searchId !== _searchSeq` so a stale COUNT can't overwrite a fresher search's text or pollute its structured log. - Failure handling: a COUNT error leaves the initial `50+` label in place — under-disclosure beats clearing the line. Logged via console.warn; `count_ms` is still recorded for isamplesorg#167 analysis. - Initial render keeps `50+` while the COUNT scan is in flight, so cold-cache users see "more exist" signaling immediately rather than a bare 50 with no qualifier. - Structured log (isamplesorg#167) gains `total_count` and `count_ms` fields. `elapsed_ms` continues to mark end-of-handler so it remains an honest end-to-end "search complete" latency including the COUNT. Validation: - New spec `tests/playwright/search-real-count.spec.js`: - cap-hit: "pottery" produces `50+` then `50 of N (N>50)`, and the structured log carries `total_count` and `count_ms`. - no-results: "xyzzyqqqplugh" still short-circuits to "No results" without firing COUNT (`total_count: null`, `count_ms: 0`). - Fault-injection: confirmed both new tests FAIL against pre-fix code and PASS against the fix. - Full local suite (`search-real-count` + `url-roundtrip` + `facetnote-url-load`): 9 passed (1.7m). No regression. Roadmap step 2 of isamplesorg#234. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Two blockers raised in Codex review (PR thread):
1. **Stale-search guard placement.** `_searchSeq` was checked only after
the COUNT awaited. A superseded search could still mutate
`searchResults.textContent`, repaint `samplesSection`, trigger
`camera.flyTo`, AND launch a wasted COUNT scan before the existing
guard fired. Fix: check `searchId !== _searchSeq` immediately after
the SELECT returns and before any UI mutation or COUNT launch. The
later COUNT-specific guard stays as a second line of defense.
2. **Predicate snapshot.** The COUNT recomputed `sourceFilterSQL('f.source')`
and `facetFilterSQL()` at the moment it built its SQL, rather than
reusing the strings the SELECT used. If a user toggled a source
checkbox or material facet while the SELECT was in flight, the COUNT
would scan a different filter state than the rendered 50 rows,
producing a label like "50 of M" where M is incoherent with the
visible result set. Fix: snapshot `sourceSQL` / `facetSQL` once at
the top of the try block and reuse for both SELECT (all shapes) and
COUNT.
Also:
- Add `superseded` to the `isamples.search` structured log (isamplesorg#167) so
downstream perf analysis can distinguish abandoned searches from
live ones.
- Add a filtered cap-hit test (Codex coverage nit). The test ticks the
first material facet then searches; asserts the structured-log
invariant `total_count present ⟺ cap was hit` and `superseded === false`.
The test tolerates all three terminal states (cap hit / sub-cap /
no-results) because the first material URI is data-dependent and may
not overlap with the search term — what we're proving is the
invariant, not a specific count.
- Add `superseded === false` assertion to the existing happy-path
cap-hit test for coverage on the new field.
Validation: 8/8 local Playwright tests pass (search-real-count [3] +
url-roundtrip [5] + facetnote-url-load [2]). Fault-injection: confirmed
the filtered-cap test still catches the bug if the snapshot is
removed.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Codex round-2 found two remaining issues: 1. **Blocker — stale error UI write.** The success-path freshness guard was correctly placed, but the outer `catch` still unconditionally wrote `searchResults.textContent = "Search error: ..."`. A superseded search whose SELECT later rejected would overwrite a newer search's UI with its stale error. Fix: gate the catch's UI write on the same `searchId !== _searchSeq` predicate so superseded errors are logged + recorded in the structured log (with `superseded: true`) but never repaint the live search line. 2. **Telemetry caveat — DOM-live booleans in `finally`.** The `has_source_filter` / `has_facet_filter` log fields were computed inside `finally` from the live DOM. If the user toggled a checkbox during the SELECT/COUNT await chain, the log would report post-toggle state instead of search-fire-time state. Fix: snapshot both booleans alongside the SQL predicates at search-fire time (declared at function scope so they remain in scope through `finally`). Also strengthened the filtered-cap test (Codex round-2 coverage gap): The previous test ticked a facet BEFORE searching, so SELECT and COUNT saw the same DOM state regardless of whether the snapshot existed — the test passed even without the round-1 snapshot fix. The new "telemetry snapshot survives a facet toggle during search await" test exercises the race directly: fires search with no facet, waits for the visible `50+` (proves SELECT done), THEN ticks the material facet, then waits for the final `50 of N` label. Asserts `has_facet_filter === false` in the structured log — which only holds if the snapshot survives the toggle. Fault-injection confirmed: the test fails if the snapshot is reverted to live-DOM reads. Validation: 8/8 local Playwright tests pass (search-real-count [3] + url-roundtrip [5] + facetnote-url-load [2]). Telemetry-snapshot test verified to fail against pre-fix code. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Codex round 3 found: 1. **Blocker — invalid-submit stale overwrite.** A valid search in flight could still have its catch-path error overwrite the "Type at least 2 characters" message: the early-return for empty / short terms did NOT bump `_searchSeq`, so when the prior search's SELECT later rejected, its catch saw `searchId === _searchSeq` and wrote "Search error: …" over the live UI. Fix: bump `_searchSeq` at the top of `doSearch`, before the length guard. Every submission — valid or invalid — now invalidates in-flight work. 2. **Coverage — race test was still timing-dependent.** The previous test accepted either `50+` or the final `50 of N` label before toggling. If the COUNT happened to land first, the toggle was a no-op and the test passed regardless of whether the snapshot existed. Fix: monkey-patch the page's `db.query` to introduce a 1.5s delay on COUNT(*) queries (scoped to the page context via the OJS reactive-value accessor, same pattern url-roundtrip.spec.js uses for `viewer`). The toggle is now GUARANTEED to land between SELECT completion and COUNT completion. Wait for EXACT `50+` state before toggling — no more "or final label" loophole. Source-filter snapshot coverage is documented as a TODO: Quarto's see-also rendering duplicates `#sourceFilter` checkboxes, making a toggle-and-count assertion ambiguous in the preview environment. That's a fixture problem orthogonal to isamplesorg#236; leaving it for a follow-up rather than blocking this PR. Validation: 8/8 local Playwright tests pass. Fault-injection (revert `hadFacetFilter` snapshot to live `hasFacetFilters()` call in `finally`) confirmed: the new test fails deterministically under the held-open-COUNT timing. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
Author
Codex review iteration log (4 rounds → LGTM)
Round 4: LGTM (Codex)
Known follow-up filed as a TODO comment in the test:
Final test count: 8 passed (3 search-real-count + 5 url-roundtrip + 2 facetnote-url-load, total ~2 min) plus the fault-injection sanity check on every round. Thanks codex for the four-round audit — the catch-path supersession (round 2) and invalid-submit window (round 3) were both real bugs I'd have missed. |
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
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.
Summary
Roadmap step 2 of #234 (#232 "50+" → real count, quick-win).
The in-map search-results line used to read
50+ results for "pottery"whenever the SELECT hitLIMIT 50, hiding whether the true result set is 51 or 50,000. Forpottery Cyprus(world scope) the real count is ~7,124; users saw50+and couldn't gauge whether to refine.Before:
50+ results for "pottery"After:
50 of 7,124 results for "pottery"Mirrors the pattern used to close #206 (real-count follow-up in
loadViewportSamples()).Approach
COUNT(*)with the same WHERE clause and replace the label.results.lengthIS the truth — no second scan.50+while COUNT is in flight, so users see "more exist" signaling immediately.Key implementation details
Effective query shape. The area-scope path can fall back to the world-mode SELECT when
viewerBboxSQL()returns null. Track which shape actually ran (effectiveQueryShape/effectiveBboxSQL) so the COUNT uses the matching WHERE/JOIN. Keying offeffectiveScope === 'area'would have generated a malformed COUNT in the fallback case.Cancellation. A second search bumps
_searchSeq. The COUNT guards againstsearchId !== _searchSeqso a stale COUNT can't overwrite a fresher search's text or pollute its structured log.Failure handling. A COUNT error leaves the initial
50+label in place — under-disclosure beats clearing the line. Logged viaconsole.warn;count_msis still recorded.Structured log (Explorer FTS Track 1a: Browser perf-smoke baseline #167) additions.
total_count(real count, ornullwhen not computed) andcount_ms(COUNT scan duration).elapsed_mscontinues to mark end-of-handler so it remains an end-to-end "search complete" latency including the COUNT.Test plan
tests/playwright/search-real-count.spec.js:pottery): initial50+then50 of N (N>50), structured log carriestotal_countandcount_ms > 0.xyzzyqqqplugh): still short-circuits toNo results; COUNT not fired;total_count: null,count_ms: 0.search-real-count+url-roundtrip+facetnote-url-load→ 9 passed (1.7m).pottery/pottery Cyprus.What's NOT in this PR
Acceptance criteria (from #232)
50 of N results for "term"when the cap is hit.12 results for "term"(unchanged) when below the cap._searchSeqinvariant; covered conceptually rather than via a flaky two-SELECT race in Playwright).🤖 Generated with Claude Code