From 8e14fc484035e90a1ce7e665315a6882755d00a1 Mon Sep 17 00:00:00 2001 From: Jeremy Prevost Date: Mon, 6 Jul 2026 16:06:57 -0400 Subject: [PATCH 1/5] Add reranker gem and refactor to use it --- .env.test | 2 + AGENTS.md | 2 +- Gemfile | 1 + Gemfile.lock | 10 + README.md | 2 + app/controllers/search_controller.rb | 13 +- app/models/merged_search_paginator.rb | 69 ------ app/models/merged_search_service.rb | 260 +++----------------- app/views/search/results.html.erb | 4 +- test/controllers/search_controller_test.rb | 135 +--------- test/models/merged_search_paginator_test.rb | 62 ----- test/models/merged_search_service_test.rb | 127 ++++++---- 12 files changed, 147 insertions(+), 540 deletions(-) delete mode 100644 app/models/merged_search_paginator.rb delete mode 100644 test/models/merged_search_paginator_test.rb diff --git a/.env.test b/.env.test index 93bf405e..0693b52f 100644 --- a/.env.test +++ b/.env.test @@ -18,6 +18,8 @@ RACK_ATTACK_VERBOSE_LOGGING=false REQUESTS_PER_PERIOD=10 RESULTS_GLOBAL_LIMIT_PER_SEC=10 RESULTS_PER_PAGE=20 +ALL_TAB_RESULTS_PER_SOURCE=50 +ALL_TAB_SCORER=zscore SYNDETICS_PRIMO_URL=https://syndetics.com/index.php?client=primo TACOS_HOST=FAKE_TACOS_HOST TACOS_URL=http://FAKE_TACOS_HOST/graphql diff --git a/AGENTS.md b/AGENTS.md index 1fa396d7..614ba36e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -30,7 +30,7 @@ This file highlights the important, discoverable conventions and workflows an AI Essential ENV vars for core functionality: `TIMDEX_GRAPHQL`, `PRIMO_API_URL`, `PRIMO_API_KEY`, `RESULTS_PER_PAGE`, `TIMDEX_INDEX`, `TIMDEX_SOURCES`. Filter customization: `FILTER_*` (e.g., `FILTER_LANGUAGE`, `FILTER_CONTENT_TYPE`) and `ACTIVE_FILTERS` (comma-separated list controlling visibility/order of filters; note that filter aggregation keys in the schema use `*Filter` suffix, e.g., `languageFilter`, `contentTypeFilter`). Tests rely on `.env.test` values for VCR cassette generation and use `ClimateControl` gem to mock feature flags. -- **Parallel fetching & multi-source pagination:** The `all` tab uses `MergedSearchService` (with `MergedSearchPaginator`) to fetch Primo and Timdex concurrently via `Thread.new`, then intelligently merges paginated results. Primo has a practical offset limit (~960 records); when this limit is reached, the UI shows a `show_continuation` flag to indicate search is exhausted. Merged totals are cached for 12 hours. Be careful when refactoring to preserve thread-safety, caching semantics, and offset limit handling. +- **Parallel fetching & reranking:** The `all` tab uses `MergedSearchService` to fetch Primo and TIMDEX concurrently via `Thread.new`, then passes both result sets to the `reranker` gem for scoring and ordering. The scorer is configurable via `ALL_TAB_SCORER` (default: `zscore`); the number of results fetched per source is controlled by `ALL_TAB_RESULTS_PER_SOURCE` (default: 50). There is no pagination on the all tab — results are returned as a flat reranked list. Be careful when refactoring to preserve thread-safety and error handling. - **JS stack & conventions:** Rails importmap is in use (`importmap-rails`). JavaScript entry is `app/javascript/application.js`. Stimulus controllers live in `app/javascript/controllers` and are imported by `importmap` via `config/importmap.rb`. Key controllers include `content_loader_controller.js` (dynamic content loading) and tab management via `source_tabs.js` (which handles geospatial UI state). Prefer small, focused changes to Stimulus controllers rather than heavy bundler-based rewrites. diff --git a/Gemfile b/Gemfile index 07b42619..86ff2a34 100644 --- a/Gemfile +++ b/Gemfile @@ -20,6 +20,7 @@ gem 'rails', '~> 8.1.0' gem 'rails_cloudflare_turnstile' gem 'rails_semantic_logger' gem 'redis' +gem 'reranker', git: 'https://github.com/MITLibraries/reranker.git', tag: 'v0.1.0' gem 'scout_apm' gem 'sentry-rails' gem 'sentry-ruby' diff --git a/Gemfile.lock b/Gemfile.lock index 1f0192f9..a5d4a2a9 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,3 +1,11 @@ +GIT + remote: https://github.com/MITLibraries/reranker.git + revision: 85b0d19c88c853c76f8de54786b4341ad2e43d4d + tag: v0.1.0 + specs: + reranker (0.1.0) + enumerable-stats (>= 1.0, < 2.0) + GIT remote: https://github.com/mitlibraries/mitlibraries-theme revision: 135390daff870937212f1d9020abb6aea53c87d9 @@ -133,6 +141,7 @@ GEM dotenv (= 3.2.0) railties (>= 6.1) drb (2.2.3) + enumerable-stats (1.4.0) erb (6.0.6) erubi (1.13.1) faraday (2.14.3) @@ -452,6 +461,7 @@ DEPENDENCIES rails_cloudflare_turnstile rails_semantic_logger redis + reranker! rubocop rubocop-rails scout_apm diff --git a/README.md b/README.md index 8c66daf2..80d39438 100644 --- a/README.md +++ b/README.md @@ -144,6 +144,8 @@ may have unexpected consequences if applied to other TIMDEX UI apps. - `BLOCKED_USER_AGENTS` - comma-separated list of user agent strings to hard-block with 403 Forbidden responses (bypasses throttling; much cheaper). Default blocks `Sogou web spider` which was responsible for 76.94k spoofed attack requests from non-Chinese IPs. Example: `"Sogou web spider,BadBot/2.0"` - `REDIRECT_REQUESTS_PER_PERIOD`- number of requests that can be made that the query string starts with our legacy redirect parameter to throttle per `REQUEST_PERIOD` - `REDIRECT_REQUEST_PERIOD`- time in minutes used along with `REDIRECT_REQUESTS_PER_PERIOD` +- `ALL_TAB_RESULTS_PER_SOURCE`: Number of results to fetch from each API (Primo and TIMDEX) for the combined all tab. These results are then merged and reranked. Defaults to 50 if unset. Keep below 960 to avoid hitting Primo's offset limit. +- `ALL_TAB_SCORER`: Scorer to use when merging results on the combined all tab. Valid values: `zscore` (default), `zipper`, `simple`, `random`. See the [reranker gem](https://github.com/MITLibraries/reranker) for details. - `RESULTS_PER_PAGE`: The number of results to display per page. Use an even number to avoid peculiarities. Defaults to 20 if unset. - `ROBOTS_ENV`: Determines which version of `robots.txt` is used. This is read by the Robots controller. Any value other than `production` results in the non-production version being used. - `SCOUT_AUTO_INSTRUMENTS`: default is `false`. Recommended setting is `true` unless we add manual instrumentation in the future. diff --git a/app/controllers/search_controller.rb b/app/controllers/search_controller.rb index 16e4dd5e..2b57d580 100644 --- a/app/controllers/search_controller.rb +++ b/app/controllers/search_controller.rb @@ -99,25 +99,18 @@ def load_timdex_results end def load_all_results - current_page = @enhanced_query[:page] || 1 - per_page = ENV.fetch('RESULTS_PER_PAGE', '20').to_i - - # Inject wrapper fetchers instead of using the service defaults. We use lambdas here so the - # service can call back into this controller's instance methods while preserving request-scoped - # context (for example `@enhanced_query`) and the controller's caching/normalization behavior. - # Using lambdas keeps the service decoupled from controller internals. + # Inject wrapper fetchers so the service can call back into this controller's + # instance methods while preserving request-scoped context and caching behavior. merge_service = MergedSearchService.new( enhanced_query: @enhanced_query, active_tab: @active_tab, primo_fetcher: ->(offset:, per_page:, query: nil) { fetch_primo_data(offset: offset, per_page: per_page) }, timdex_fetcher: ->(offset:, per_page:, query: nil) { fetch_timdex_data(offset: offset, per_page: per_page) } ) - data = merge_service.fetch(page: current_page, per_page: per_page) + data = merge_service.fetch @results = data[:results] @errors = data[:errors] - @pagination = data[:pagination] - @show_primo_continuation = data[:show_primo_continuation] end def fetch_primo_data(offset: nil, per_page: nil) diff --git a/app/models/merged_search_paginator.rb b/app/models/merged_search_paginator.rb deleted file mode 100644 index 19f36934..00000000 --- a/app/models/merged_search_paginator.rb +++ /dev/null @@ -1,69 +0,0 @@ -# frozen_string_literal: true - -# MergedSearchPaginator encapsulates stateless merged pagination logic for combining two API result sets. -# It calculates the merge plan, API offsets, and merges the results for a given page. -class MergedSearchPaginator - attr_reader :primo_total, :timdex_total, :current_page, :per_page - - def initialize(primo_total:, timdex_total:, current_page:, per_page:) - @primo_total = primo_total - @timdex_total = timdex_total - @current_page = current_page - @per_page = per_page - end - - # Returns an array of :primo and :timdex symbols for the merged result order on this page - def merge_plan - total_results = primo_total + timdex_total - start_index = (current_page - 1) * per_page - end_index = [start_index + per_page, total_results].min - plan = [] - primo_used = 0 - timdex_used = 0 - i = 0 - while i < end_index - if primo_used < primo_total && (timdex_used >= timdex_total || primo_used <= timdex_used) - source = :primo - primo_used += 1 - elsif timdex_used < timdex_total - source = :timdex - timdex_used += 1 - end - plan << source if i >= start_index - i += 1 - end - plan - end - - # Returns [primo_offset, timdex_offset] for the start of this page - # Returns [primo_offset, timdex_offset] for the start of this page. - # - # If an API has been exhausted (the computed offset is greater-than-or-equal - # to that API's total), this method returns `nil` for that API to signal - # callers that no request should be made against that service for this page. - # - # This avoids unnecessary requests where the API would only return empty - # results when queried at an offset beyond its available records. - def api_offsets - start_index = (current_page - 1) * per_page - primo_offset = 0 - timdex_offset = 0 - i = 0 - while i < start_index - if primo_offset < primo_total && (timdex_offset >= timdex_total || primo_offset <= timdex_offset) - primo_offset += 1 - elsif timdex_offset < timdex_total - timdex_offset += 1 - else - break - end - i += 1 - end - # If the computed offset reached or exceeded the total for a source, - # return nil for that source to indicate it is exhausted. - primo_offset = nil if primo_offset >= primo_total - timdex_offset = nil if timdex_offset >= timdex_total - - [primo_offset, timdex_offset] - end -end diff --git a/app/models/merged_search_service.rb b/app/models/merged_search_service.rb index 880c0b7d..201e0128 100644 --- a/app/models/merged_search_service.rb +++ b/app/models/merged_search_service.rb @@ -1,20 +1,9 @@ -require 'digest' - -# Orchestrates merged "all" tab searches across Primo and TIMDEX. -# -# Handles parallel fetches, per-query totals caching, pagination calculation via -# `MergedSearchPaginator`, and assembly of a controller-friendly response hash. +# Orchestrates merged "all" tab searches across Primo and TIMDEX using the +# reranker gem. Fetches a configurable number of results from each source in +# parallel, then delegates ordering to the configured scorer. class MergedSearchService - # Time to live value for cache expiration. - TTL = 12.hours - # Initialize a new MergedSearchService. # - # The service requires two callable fetchers (for Primo and TIMDEX) that - # perform the underlying source requests. Fetchers are injected to keep - # the orchestration logic decoupled from transport, caching, and - # normalization concerns. - # # @param enhanced_query [Hash] query hash produced by `Enhancer` # @param active_tab [String] the currently active tab (e.g. 'all') # @param primo_fetcher [#call] callable used to fetch Primo results; must accept `offset:, per_page:, query:` @@ -26,234 +15,65 @@ def initialize(enhanced_query:, active_tab:, primo_fetcher:, timdex_fetcher:) @timdex_fetcher = timdex_fetcher end - # Execute merged search orchestration for the requested page. - # - # @param page [Integer] page number to fetch - # @param per_page [Integer] number of results per page - # @return [Hash] keys: :results, :errors, :pagination, :show_primo_continuation - def fetch(page:, per_page:) - current_page = (page || 1).to_i - per_page = (per_page || 20).to_i - if current_page == 1 - first_page_fetch(current_page, per_page) - else - deeper_page_fetch(current_page, per_page) - end - end - - # Handle page 1: perform the full-size parallel fetch, cache - # totals, build the paginator, and return the assembled result. - # - # Executes a full-size parallel fetch (requests `per_page` items from each - # backend), computes and caches per-query totals, constructs a - # `MergedSearchPaginator`, and assembles the controller-facing response. - # - # @param current_page [Integer] the current page (expected to be 1) - # @param per_page [Integer] the number of results per merged page - # @return [Hash] keys: :results, :errors, :pagination, :show_primo_continuation - def first_page_fetch(current_page, per_page) - primo_data, timdex_data = parallel_fetch(offset: 0, per_page: per_page) - - totals = { primo: primo_data[:hits].to_i, timdex: timdex_data[:hits].to_i } - write_cached_totals(totals) - - paginator = build_paginator_from_totals(totals, current_page, per_page) - - assemble_all_tab_result(paginator, primo_data, timdex_data, current_page, per_page) - end - - # Handle deeper pages: ensure totals are available (falling back to summary - # calls when missing), build the paginator, fetch required chunks, and - # assemble the final result. - # - # Ensures per-query totals are available by reading cached totals or - # performing summary requests (per_page == 1) if the cached totals aren't - # present. (They should be, but there may be edge cases.) - # Builds a `MergedSearchPaginator`, fetches the page-sized chunks required - # for the merged layout, and returns the assembled controller-facing response. + # Fetch and rerank results from both sources. # - # @param current_page [Integer] the requested page number (> 1) - # @param per_page [Integer] the number of results per merged page - # @return [Hash] keys: :results, :errors, :pagination, :show_primo_continuation - def deeper_page_fetch(current_page, per_page) - totals = Rails.cache.read(totals_cache_key) + # @param per_source [Integer, nil] number of results to request from each API; + # defaults to the ALL_TAB_RESULTS_PER_SOURCE env var (or 50). + # @return [Hash] keys: :results, :errors + def fetch(per_source: nil) + per_source = (per_source || ENV.fetch('ALL_TAB_RESULTS_PER_SOURCE', '50')).to_i + primo_data, timdex_data = parallel_fetch(per_page: per_source) - unless totals - primo_summary, timdex_summary = parallel_fetch(offset: 0, per_page: 1) - totals = { primo: primo_summary[:hits].to_i, timdex: timdex_summary[:hits].to_i } - write_cached_totals(totals) - end + primo_results = primo_data[:results] || [] + timdex_results = timdex_data[:results] || [] + errors = combine_errors(primo_data[:errors], timdex_data[:errors]) - paginator = build_paginator_from_totals(totals, current_page, per_page) - primo_data, timdex_data = fetch_all_tab_page_chunks(paginator) + merged = Reranker::Reranker.new(configured_scorer).rerank( + primo_results, timdex_results, query: @enhanced_query[:q] + ) - assemble_all_tab_result(paginator, primo_data, timdex_data, current_page, per_page, deeper: true) + { results: merged, errors: errors } end private - # Generate the cache key used to store per-query totals for this enhanced query/tab. - # - # @return [String] cache key ending in '/totals' - def totals_cache_key - base = generate_cache_key(@enhanced_query.merge(tab: @active_tab)) - "#{base}/totals" - end - - # Persist per-query totals to the application cache. - # - # Tests use a test-local `Rails.cache` (MemoryStore) so they do not need to - # inject a separate cache instance; production code uses the configured - # `Rails.cache` store. - # - # @param totals [Hash] { primo: Integer, timdex: Integer } - def write_cached_totals(totals) - Rails.cache.write(totals_cache_key, totals, expires_in: TTL) - end - - # Perform parallel fetches from Primo and TIMDEX using the configured - # fetchers. Each fetcher should return the usual response hash including - # `:results` and `:hits`. + # Fetch from Primo and TIMDEX concurrently. # - # WARNING: exceptions raised inside these threads will not automatically - # propagate to the caller; callers/tests should account for this. + # WARNING: exceptions raised inside threads do not automatically propagate; + # callers should account for this. # - # @param offset [Integer] api offset to request - # @param per_page [Integer] number of items to request - # @return [Array] [primo_response, timdex_response] - def parallel_fetch(offset:, per_page:) + # @param per_page [Integer] number of results to request per source + # @return [Array] [primo_response, timdex_response] + def parallel_fetch(per_page:) primo = nil timdex = nil - threads = [] - threads << Thread.new { primo = @primo_fetcher.call(offset: offset, per_page: per_page, query: @enhanced_query) } - threads << Thread.new { timdex = @timdex_fetcher.call(offset: offset, per_page: per_page, query: @enhanced_query) } + threads = [ + Thread.new { primo = @primo_fetcher.call(offset: 0, per_page: per_page, query: @enhanced_query) }, + Thread.new { timdex = @timdex_fetcher.call(offset: 0, per_page: per_page, query: @enhanced_query) } + ] threads.each(&:join) [primo, timdex] end - # Compute API offsets from the paginator and fetch the page-sized chunks - # required to assemble the merged page. - # - # @param paginator [MergedSearchPaginator] - # @return [Array] [primo_data, timdex_data] - def fetch_all_tab_page_chunks(paginator) - merge_plan = paginator.merge_plan - primo_count = merge_plan.count(:primo) - timdex_count = merge_plan.count(:timdex) - primo_offset, timdex_offset = paginator.api_offsets - - # Only spawn fetch threads when we both need results for the merge plan - # and the paginator indicates a valid offset for that API. A `nil` offset - # means the API is exhausted and should not be queried for this page. - primo_thread = if primo_count > 0 && !primo_offset.nil? - Thread.new do - @primo_fetcher.call(offset: primo_offset, per_page: primo_count, query: @enhanced_query) - end - end - timdex_thread = if timdex_count > 0 && !timdex_offset.nil? - Thread.new do - @timdex_fetcher.call(offset: timdex_offset, per_page: timdex_count, query: @enhanced_query) - end - end - - primo_data = if primo_thread - primo_thread.value - else - { results: [], errors: nil, hits: paginator.primo_total, - show_continuation: false } - end - timdex_data = timdex_thread ? timdex_thread.value : { results: [], errors: nil, hits: paginator.timdex_total } - - [primo_data, timdex_data] - end - - # Assemble the final hash returned to the controller for rendering. - # - # @param paginator [MergedSearchPaginator] - # @param primo_data [Hash] response from Primo fetcher - # @param timdex_data [Hash] response from TIMDEX fetcher - # @param current_page [Integer] - # @param per_page [Integer] - # @param deeper [Boolean] whether this was a deeper-page flow - # @return [Hash] response with :results, :errors, :pagination, :show_primo_continuation - def assemble_all_tab_result(paginator, primo_data, timdex_data, current_page, per_page, deeper: false) - primo_total = primo_data[:hits] || 0 - timdex_total = timdex_data[:hits] || 0 - - merged = merge_results(paginator, primo_data[:results] || [], timdex_data[:results] || []) - errors = combine_errors(primo_data[:errors], timdex_data[:errors]) - pagination = Analyzer.new(@enhanced_query, timdex_total, :all, primo_total, per_page).pagination - - show_primo_continuation = if deeper - # Use the Primo-specific API offset (calculated from the paginator) - # when deciding whether to show a Primo continuation. - # - # If the paginator returns `nil` for a exhausted API we still - # want to show the continuation when the requested page is far - # beyond the Primo API's practical offset limit. Fall back to - # checking the merged page's start index when the API offset - # is unavailable. - primo_api_offset, _timdex_api_offset = paginator.api_offsets - primo_data[:show_continuation] || - (primo_api_offset && primo_api_offset >= Analyzer::PRIMO_MAX_OFFSET) || - (primo_api_offset.nil? && ((current_page - 1) * per_page) >= Analyzer::PRIMO_MAX_OFFSET) - else - primo_data[:show_continuation] - end - - { results: merged, errors: errors, pagination: pagination, show_primo_continuation: show_primo_continuation } + # Instantiate the scorer based on the ALL_TAB_SCORER env var. + # Defaults to ZscoreScorer. + # + # @return [Reranker::Scorer] + def configured_scorer + case ENV.fetch('ALL_TAB_SCORER', 'zscore').downcase + when 'zscore' then Reranker::ZscoreScorer.new + when 'zipper' then Reranker::ZipperMergeScorer.new + when 'simple' then Reranker::SimpleScorer.new + when 'random' then Reranker::RandomScorer.new + else Reranker::ZscoreScorer.new + end end - # Merge multiple error arrays into a single array or nil when empty. + # Merge multiple error arrays into a single array, or nil when empty. # # @return [Array, nil] def combine_errors(*error_arrays) all_errors = error_arrays.compact.flatten all_errors.any? ? all_errors : nil end - - # Build a `MergedSearchPaginator` given cached totals. - # - # @param totals [Hash] { primo: Integer, timdex: Integer } - # @return [MergedSearchPaginator] - def build_paginator_from_totals(totals, current_page, per_page) - MergedSearchPaginator.new(primo_total: totals[:primo] || 0, timdex_total: totals[:timdex] || 0, - current_page: current_page, per_page: per_page) - end - - # Note: default fetcher implementations were removed to enforce explicit - # dependency injection. Callers must provide `primo_fetcher` and - # `timdex_fetcher` when constructing `MergedSearchService`. - - # Generate a cache key based on the supplied query hash. - # - # @param query [Hash] - # @return [String] MD5 hex digest - def generate_cache_key(query) - CacheKeyGenerator.call(query) - end - - # Helps callers (including `MergedSearchPaginator`) delegate merging logic to the orchestration - # layer. This method iterates the paginator's `merge_plan` and pulls items from the respective - # source result arrays in order. - # - # @param paginator [MergedSearchPaginator] - # @param primo_results [Array] - # @param timdex_results [Array] - # @return [Array] merged results - def merge_results(paginator, primo_results, timdex_results) - merged = [] - primo_idx = 0 - timdex_idx = 0 - paginator.merge_plan.each do |source| - if source == :primo - merged << primo_results[primo_idx] if primo_idx < primo_results.length - primo_idx += 1 - else - merged << timdex_results[timdex_idx] if timdex_idx < timdex_results.length - timdex_idx += 1 - end - end - merged - end end diff --git a/app/views/search/results.html.erb b/app/views/search/results.html.erb index f5cf8bbe..9329db5a 100644 --- a/app/views/search/results.html.erb +++ b/app/views/search/results.html.erb @@ -17,14 +17,14 @@ <% elsif @results.present? && @errors.blank? %>
-

<%= results_summary(@pagination[:hits]) %>

+

<%= results_summary(@pagination&.[](:hits) || @results.length) %>

<% if @show_nls_warning %> <%= render partial: 'search/nls_alert' %> <% end %>
-
    +
      <% @results.each do |result| %> <% if result[:api] == 'primo' %> <%= render(partial: 'search/result_primo', locals: { result: result }) %> diff --git a/test/controllers/search_controller_test.rb b/test/controllers/search_controller_test.rb index 010c527e..e7da510b 100644 --- a/test/controllers/search_controller_test.rb +++ b/test/controllers/search_controller_test.rb @@ -881,26 +881,12 @@ def source_filter_count(controller) assert_select 'a[href*="tab=website"]', count: 1 end - test 'all tab page 1 writes totals to cache' do - # This integration-level behavior is covered by unit tests on `MergedSearchService`. - # Here we assert the controller delegates to the service. + test 'all tab delegates to MergedSearchService' do mock_service = mock('merged_service') - mock_service.expects(:fetch).returns({ results: [], errors: nil, pagination: {}, show_primo_continuation: false }) + mock_service.expects(:fetch).returns({ results: [], errors: nil }) MergedSearchService.expects(:new).returns(mock_service) - get '/results?q=test' - assert_response :success - end - - test 'all tab deeper page reads cached totals and avoids summary calls' do - # This behavior is covered in greater depth by `MergedSearchService` unit tests. - mock_service = mock('merged_service') - mock_service.expects(:fetch).with(page: 2, - per_page: 20).returns({ results: [], - errors: nil, pagination: {}, show_primo_continuation: false }) - MergedSearchService.expects(:new).returns(mock_service) - - get '/results?q=test&page=2' + get '/results?q=test&tab=all' assert_response :success end @@ -1067,120 +1053,13 @@ def source_filter_count(controller) assert_select '.tab-navigation .tab-link.active', text: 'All' end - test 'all tab shows primo continuation when page exceeds API offset limit' do - sample_doc = { - api: 'primo', - title: 'Sample Primo Document Title', - format: 'Article', - year: '2025', - creators: [ - { value: 'Foo Barston', link: nil }, - { value: 'Baz Quxley', link: nil } - ], - links: [{ 'kind' => 'full record', 'url' => 'https://example.com/record' }] - } - mock_primo = mock('primo_search') - mock_primo.expects(:search).returns({ 'docs' => [sample_doc], 'info' => { 'total' => 1 } }).at_least_once - PrimoSearch.expects(:new).returns(mock_primo).at_least_once - mock_normalizer = mock('normalizer') - mock_normalizer.expects(:normalize).returns([sample_doc]).at_least_once - NormalizePrimoResults.expects(:new).returns(mock_normalizer).at_least_once - mock_timdex_search_success - - get '/results?q=test&tab=all&page=49' - assert_response :success - - # Should show primo continuation partial - assert_select '.primo-continuation', count: 1 - assert_select '.primo-continuation h2', text: /You have reached the limit for these search results/ - end - - test 'all tab pagination displays combined hit counts' do - sample_docs = (1..10).map do |i| - { - title: "Sample Primo Document Title \\#{i}", - format: 'Article', - year: '2025', - creators: [{ value: "Author \\#{i}", link: nil }], - links: [{ 'kind' => 'full record', 'url' => "https://example.com/record\\#{i}" }] - } - end - mock_primo = mock('primo_search') - mock_primo.expects(:search).returns({ - 'docs' => sample_docs, - 'info' => { 'total' => 500 } - }).at_least_once - PrimoSearch.expects(:new).returns(mock_primo).at_least_once - mock_normalizer = mock('normalizer') - mock_normalizer.expects(:normalize).returns(sample_docs).at_least_once - NormalizePrimoResults.expects(:new).returns(mock_normalizer).at_least_once - mock_timdex_search_with_hits(300) - - get '/results?q=test&tab=all' - assert_response :success - - # Should show pagination with combined hit counts (500 + 300 = 800) - assert_select '.pagination-container' - assert_select '.pagination-container .current', text: /1 - 20 of 800/ - end - - test 'all tab pagination includes next page link when more results available' do - sample_docs = (1..10).map do |i| - { - title: "Sample Primo Document Title \\#{i}", - format: 'Article', - year: '2025', - creators: [{ value: "Author \\#{i}", link: nil }], - links: [{ 'kind' => 'full record', 'url' => "https://example.com/record\\#{i}" }] - } - end - mock_primo = mock('primo_search') - mock_primo.expects(:search).returns({ - 'docs' => sample_docs, - 'info' => { 'total' => 500 } - }).at_least_once - PrimoSearch.expects(:new).returns(mock_primo).at_least_once - mock_normalizer = mock('normalizer') - mock_normalizer.expects(:normalize).returns(sample_docs).at_least_once - NormalizePrimoResults.expects(:new).returns(mock_normalizer).at_least_once - mock_timdex_search_with_hits(300) + test 'all tab does not show pagination controls' do + mock_primo_search_all_tab + mock_timdex_search_all_tab get '/results?q=test&tab=all' assert_response :success - - # Should show next page link when there are more than 20 total results - assert_select '.pagination-container .next a[href*="page=2"]' - end - - test 'all tab pagination on page 2 includes previous page link' do - sample_docs = (1..10).map do |i| - { - title: "Sample Primo Document Title \\#{i}", - format: 'Article', - year: '2025', - creators: [{ value: "Author \\#{i}", link: nil }], - links: [{ 'kind' => 'full record', 'url' => "https://example.com/record\\#{i}" }] - } - end - mock_primo = mock('primo_search') - mock_primo.expects(:search).returns({ - 'docs' => sample_docs, - 'info' => { 'total' => 500 } - }).at_least_once - PrimoSearch.expects(:new).returns(mock_primo).at_least_once - mock_normalizer = mock('normalizer') - mock_normalizer.expects(:normalize).returns(sample_docs).at_least_once - NormalizePrimoResults.expects(:new).returns(mock_normalizer).at_least_once - mock_timdex_search_with_hits(300) - - get '/results?q=test&tab=all&page=2' - assert_response :success - - # Should show previous page link - assert_select '.pagination-container .previous a[href*="page=1"]' - - # Should show current range (21-40 for page 2) - assert_select '.pagination-container .current', text: /21 - 40 of 800/ + assert_select '#pagination', count: 0 end test 'results can be returned in JSON format when env is set and valid token is provided' do diff --git a/test/models/merged_search_paginator_test.rb b/test/models/merged_search_paginator_test.rb deleted file mode 100644 index 948c8c22..00000000 --- a/test/models/merged_search_paginator_test.rb +++ /dev/null @@ -1,62 +0,0 @@ -# frozen_string_literal: true - -require 'test_helper' - -class MergedSearchPaginatorTest < ActiveSupport::TestCase - test 'merge_plan handles balanced results' do - paginator = MergedSearchPaginator.new(primo_total: 3, timdex_total: 3, current_page: 1, per_page: 6) - assert_equal(%i[primo timdex primo timdex primo timdex], paginator.merge_plan) - end - - test 'merge_plan handles unbalanced results' do - paginator = MergedSearchPaginator.new(primo_total: 6, timdex_total: 2, current_page: 1, per_page: 8) - assert_equal(%i[primo timdex primo timdex primo primo primo primo], paginator.merge_plan) - end - - test 'api_offsets are calculated as expected' do - paginator = MergedSearchPaginator.new(primo_total: 10, timdex_total: 10, current_page: 2, per_page: 5) - assert_equal([3, 2], paginator.api_offsets) - end - - test 'merge_results handles even results' do - paginator = MergedSearchPaginator.new(primo_total: 2, timdex_total: 2, current_page: 1, per_page: 4) - primo = %w[P1 P2] - timdex = %w[T1 T2] - svc = MergedSearchService.new(enhanced_query: { q: 'test' }, active_tab: 'all', primo_fetcher: fake_fetcher, - timdex_fetcher: fake_fetcher) - assert_equal(%w[P1 T1 P2 T2], svc.send(:merge_results, paginator, primo, timdex)) - end - - test 'merge_results with shorter array' do - paginator = MergedSearchPaginator.new(primo_total: 3, timdex_total: 1, current_page: 1, per_page: 4) - primo = %w[P1 P2 P3] - timdex = %w[T1] - svc = MergedSearchService.new(enhanced_query: { q: 'test' }, active_tab: 'all', primo_fetcher: fake_fetcher, - timdex_fetcher: fake_fetcher) - assert_equal(%w[P1 T1 P2 P3], svc.send(:merge_results, paginator, primo, timdex)) - end - - test 'api_offsets breaks when start_index exceeds totals' do - # Use very small totals and request a page far beyond available results to exercise the break - paginator = MergedSearchPaginator.new(primo_total: 1, timdex_total: 1, current_page: 5, per_page: 20) - primo_offset, timdex_offset = paginator.api_offsets - - # Offsets should stop at the available totals (1 each) - assert_nil primo_offset - assert_nil timdex_offset - end - - test 'merge_plan returns all primo when timdex is empty' do - paginator = MergedSearchPaginator.new(primo_total: 2, timdex_total: 0, current_page: 1, per_page: 5) - plan = paginator.merge_plan - - assert_equal %i[primo primo], plan - end - - test 'merge_plan returns all timdex when primo is empty' do - paginator = MergedSearchPaginator.new(primo_total: 0, timdex_total: 2, current_page: 1, per_page: 5) - plan = paginator.merge_plan - - assert_equal %i[timdex timdex], plan - end -end diff --git a/test/models/merged_search_service_test.rb b/test/models/merged_search_service_test.rb index dffb47c8..5c61b68e 100644 --- a/test/models/merged_search_service_test.rb +++ b/test/models/merged_search_service_test.rb @@ -1,5 +1,4 @@ require 'test_helper' -require 'ostruct' class MergedSearchServiceTest < ActiveSupport::TestCase test 'page 1 writes totals to cache' do @@ -59,45 +58,25 @@ class MergedSearchServiceTest < ActiveSupport::TestCase end end - test 'falls back to summary and writes cache when totals are missing' do + test 'fetch calls both fetchers at offset 0 with configured per_source' do q = { q: 'test' } - calls = [] - primo_fetcher = lambda do |offset:, per_page:, query:| - calls << [:primo, offset, per_page] - if per_page == 1 - { results: [], hits: 7, errors: nil, show_continuation: false } - else - { results: ['foo'], hits: 7, errors: nil, show_continuation: false } - end - end - timdex_fetcher = lambda do |offset:, per_page:, query:| - calls << [:timdex, offset, per_page] - if per_page == 1 - { results: [], hits: 3, errors: nil } - else - { results: ['bar'], hits: 3, errors: nil } - end + calls = [] + fetcher = lambda do |offset:, per_page:, query: nil| + calls << [offset, per_page] + { results: [], hits: 0, errors: nil, show_continuation: false } end - svc = MergedSearchService.new(enhanced_query: q, active_tab: 'all', primo_fetcher: primo_fetcher, - timdex_fetcher: timdex_fetcher) - - res = svc.fetch(page: 2, per_page: 20) - - # summary calls should have been made with per_page == 1 - assert_includes calls, [:primo, 0, 1] - assert_includes calls, [:timdex, 0, 1] + svc = MergedSearchService.new(enhanced_query: { q: 'test' }, active_tab: 'all', + primo_fetcher: fetcher, timdex_fetcher: fetcher) + ClimateControl.modify(ALL_TAB_RESULTS_PER_SOURCE: '25') { svc.fetch } - # totals cached - key = svc.send(:totals_cache_key) - totals = Rails.cache.read(key) - refute_nil totals - assert_equal 7, totals[:primo] - assert_equal 3, totals[:timdex] - - assert res[:results].is_a?(Array) + assert_equal 2, calls.length + calls.each do |offset, per_page| + assert_equal 0, offset + assert_equal 25, per_page + end end test 'all tab cold cache deeper page calls timdex twice' do @@ -136,25 +115,76 @@ class MergedSearchServiceTest < ActiveSupport::TestCase { results: [], hits: 0, errors: nil } } - svc = MergedSearchService.new(enhanced_query: { q: 'foo' }, active_tab: 'all', + + svc = MergedSearchService.new(enhanced_query: { q: 'test' }, active_tab: 'all', + primo_fetcher: primo_fetcher, timdex_fetcher: timdex_fetcher) + result = svc.fetch + + assert_equal 2, result[:results].length + titles = result[:results].map { |r| r[:title] } + assert_includes titles, 'P1' + assert_includes titles, 'T1' + end + + test 'fetch defaults to 50 results per source when env var is not set' do + per_page_seen = [] + fetcher = lambda do |offset:, per_page:, query: nil| + per_page_seen << per_page + { results: [], hits: 0, errors: nil } + end + + svc = MergedSearchService.new(enhanced_query: { q: 'test' }, active_tab: 'all', + primo_fetcher: fetcher, timdex_fetcher: fetcher) + ClimateControl.modify(ALL_TAB_RESULTS_PER_SOURCE: nil) { svc.fetch } + + assert_equal [50, 50], per_page_seen + end + + test 'configured_scorer returns ZscoreScorer by default' do + svc = MergedSearchService.new(enhanced_query: { q: 'test' }, active_tab: 'all', + primo_fetcher: fake_fetcher, timdex_fetcher: fake_fetcher) + ClimateControl.modify(ALL_TAB_SCORER: nil) do + assert_instance_of Reranker::ZscoreScorer, svc.send(:configured_scorer) + end + end + + test 'configured_scorer maps ALL_TAB_SCORER env var to correct scorer class' do + svc = MergedSearchService.new(enhanced_query: { q: 'test' }, active_tab: 'all', + primo_fetcher: fake_fetcher, timdex_fetcher: fake_fetcher) + + { + 'zscore' => Reranker::ZscoreScorer, + 'zipper' => Reranker::ZipperMergeScorer, + 'simple' => Reranker::SimpleScorer, + 'random' => Reranker::RandomScorer + }.each do |scorer_name, scorer_class| + ClimateControl.modify(ALL_TAB_SCORER: scorer_name) do + assert_instance_of scorer_class, svc.send(:configured_scorer), + "Expected #{scorer_class} for ALL_TAB_SCORER=#{scorer_name}" + end + end + end + + test 'fetch combines errors from both sources' do + primo_fetcher = fake_fetcher(errors: [{ 'message' => 'Primo error' }]) + timdex_fetcher = fake_fetcher(errors: [{ 'message' => 'TIMDEX error' }]) + + svc = MergedSearchService.new(enhanced_query: { q: 'test' }, active_tab: 'all', primo_fetcher: primo_fetcher, timdex_fetcher: timdex_fetcher) + result = svc.fetch - paginator = OpenStruct.new( - merge_plan: %i[primo primo], - api_offsets: [10, 0], - primo_total: 5, - timdex_total: 0 - ) - - primo_data, timdex_data = svc.send(:fetch_all_tab_page_chunks, paginator) - assert primo_data[:results].is_a?(Array) - assert timdex_data[:results].is_a?(Array) - assert_equal 0, timdex_data[:hits] + assert_equal 2, result[:errors].length + end + + test 'fetch returns nil errors when both sources have no errors' do + svc = MergedSearchService.new(enhanced_query: { q: 'test' }, active_tab: 'all', + primo_fetcher: fake_fetcher, timdex_fetcher: fake_fetcher) + assert_nil svc.fetch[:errors] end test 'combine_errors merges arrays or returns nil' do - svc = MergedSearchService.new(enhanced_query: { q: 'foo' }, active_tab: 'all', primo_fetcher: fake_fetcher, - timdex_fetcher: fake_fetcher) + svc = MergedSearchService.new(enhanced_query: { q: 'foo' }, active_tab: 'all', + primo_fetcher: fake_fetcher, timdex_fetcher: fake_fetcher) assert_nil svc.send(:combine_errors, nil, []) merged = svc.send(:combine_errors, [{ 'message' => 'a' }], [{ 'message' => 'b' }]) assert_equal 2, merged.length @@ -217,4 +247,5 @@ class MergedSearchServiceTest < ActiveSupport::TestCase expected = %w[P1 T1 P2 T2 P3 T3 P4 T4 P5 T5 P6 T6 P7 T7 T8 T9 T10 T11] assert_equal expected, merged end + end From da117153966af6beec446dbaf47d97ddfc93b8ab Mon Sep 17 00:00:00 2001 From: Jeremy Prevost Date: Fri, 31 Jul 2026 15:22:31 -0400 Subject: [PATCH 2/5] Load More reranked results --- .env.test | 1 + AGENTS.md | 2 +- README.md | 5 +- .../stylesheets/partials/_pagination.scss | 11 +- app/controllers/search_controller.rb | 50 +++- app/javascript/loading_spinner.js | 19 +- app/models/merged_search_service.rb | 231 ++++++++++++++++-- app/views/search/_load_more.html.erb | 22 ++ app/views/search/_result_item.html.erb | 8 + app/views/search/results.html.erb | 14 +- app/views/search/results.turbo_stream.erb | 7 + app/views/search/results_geo.html.erb | 6 +- app/views/search/results_geo.turbo_stream.erb | 7 + test/controllers/search_controller_test.rb | 9 +- test/models/merged_search_service_test.rb | 210 ++++++++-------- 15 files changed, 446 insertions(+), 156 deletions(-) create mode 100644 app/views/search/_load_more.html.erb create mode 100644 app/views/search/_result_item.html.erb create mode 100644 app/views/search/results.turbo_stream.erb create mode 100644 app/views/search/results_geo.turbo_stream.erb diff --git a/.env.test b/.env.test index 0693b52f..7e6a0a2e 100644 --- a/.env.test +++ b/.env.test @@ -20,6 +20,7 @@ RESULTS_GLOBAL_LIMIT_PER_SEC=10 RESULTS_PER_PAGE=20 ALL_TAB_RESULTS_PER_SOURCE=50 ALL_TAB_SCORER=zscore +LOAD_MORE_MAX_RESULTS=200 SYNDETICS_PRIMO_URL=https://syndetics.com/index.php?client=primo TACOS_HOST=FAKE_TACOS_HOST TACOS_URL=http://FAKE_TACOS_HOST/graphql diff --git a/AGENTS.md b/AGENTS.md index 614ba36e..d6ab841a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -30,7 +30,7 @@ This file highlights the important, discoverable conventions and workflows an AI Essential ENV vars for core functionality: `TIMDEX_GRAPHQL`, `PRIMO_API_URL`, `PRIMO_API_KEY`, `RESULTS_PER_PAGE`, `TIMDEX_INDEX`, `TIMDEX_SOURCES`. Filter customization: `FILTER_*` (e.g., `FILTER_LANGUAGE`, `FILTER_CONTENT_TYPE`) and `ACTIVE_FILTERS` (comma-separated list controlling visibility/order of filters; note that filter aggregation keys in the schema use `*Filter` suffix, e.g., `languageFilter`, `contentTypeFilter`). Tests rely on `.env.test` values for VCR cassette generation and use `ClimateControl` gem to mock feature flags. -- **Parallel fetching & reranking:** The `all` tab uses `MergedSearchService` to fetch Primo and TIMDEX concurrently via `Thread.new`, then passes both result sets to the `reranker` gem for scoring and ordering. The scorer is configurable via `ALL_TAB_SCORER` (default: `zscore`); the number of results fetched per source is controlled by `ALL_TAB_RESULTS_PER_SOURCE` (default: 50). There is no pagination on the all tab — results are returned as a flat reranked list. Be careful when refactoring to preserve thread-safety and error handling. +- **Parallel fetching, reranking & load more:** The `all` tab uses `MergedSearchService` to fetch Primo and TIMDEX concurrently via `Thread.new`, then passes both result sets to the `reranker` gem for scoring and ordering. The scorer is configurable via `ALL_TAB_SCORER` (default: `zscore`). `ALL_TAB_RESULTS_PER_SOURCE` (default: 50) controls how many candidates are fetched from each API when the cached all-tab pool needs to grow; `RESULTS_PER_PAGE` controls the initial visible batch size and each "Load more results" increment. The all tab preserves already-shown result order when reranking an expanded candidate pool, appending only unseen records. Source-specific tabs use the same load-more UI but still rely on their normal API page/offset behavior internally. `LOAD_MORE_MAX_RESULTS` caps visible load-more growth. Be careful when refactoring to preserve thread-safety, cache keys, stable-prefix behavior, and source exhaustion handling. - **JS stack & conventions:** Rails importmap is in use (`importmap-rails`). JavaScript entry is `app/javascript/application.js`. Stimulus controllers live in `app/javascript/controllers` and are imported by `importmap` via `config/importmap.rb`. Key controllers include `content_loader_controller.js` (dynamic content loading) and tab management via `source_tabs.js` (which handles geospatial UI state). Prefer small, focused changes to Stimulus controllers rather than heavy bundler-based rewrites. diff --git a/README.md b/README.md index 80d39438..1e158cf6 100644 --- a/README.md +++ b/README.md @@ -144,9 +144,10 @@ may have unexpected consequences if applied to other TIMDEX UI apps. - `BLOCKED_USER_AGENTS` - comma-separated list of user agent strings to hard-block with 403 Forbidden responses (bypasses throttling; much cheaper). Default blocks `Sogou web spider` which was responsible for 76.94k spoofed attack requests from non-Chinese IPs. Example: `"Sogou web spider,BadBot/2.0"` - `REDIRECT_REQUESTS_PER_PERIOD`- number of requests that can be made that the query string starts with our legacy redirect parameter to throttle per `REQUEST_PERIOD` - `REDIRECT_REQUEST_PERIOD`- time in minutes used along with `REDIRECT_REQUESTS_PER_PERIOD` -- `ALL_TAB_RESULTS_PER_SOURCE`: Number of results to fetch from each API (Primo and TIMDEX) for the combined all tab. These results are then merged and reranked. Defaults to 50 if unset. Keep below 960 to avoid hitting Primo's offset limit. +- `ALL_TAB_RESULTS_PER_SOURCE`: Number of candidate results to fetch from each API (Primo and TIMDEX) when the combined all tab needs to grow its cached reranked pool. Defaults to 50 if unset. This is not the number of results shown per click; use `RESULTS_PER_PAGE` for that. Keep below 960 to avoid hitting Primo's offset limit. - `ALL_TAB_SCORER`: Scorer to use when merging results on the combined all tab. Valid values: `zscore` (default), `zipper`, `simple`, `random`. See the [reranker gem](https://github.com/MITLibraries/reranker) for details. -- `RESULTS_PER_PAGE`: The number of results to display per page. Use an even number to avoid peculiarities. Defaults to 20 if unset. +- `LOAD_MORE_MAX_RESULTS`: Maximum number of results that can be shown through load-more requests for a single results tab. Defaults to 200 if unset. This guards against accidental oversized all-tab reranking requests. +- `RESULTS_PER_PAGE`: The number of results shown in the initial batch and added by each "Load more results" click. For source-specific tabs this also remains the underlying API page size. Defaults to 20 if unset. - `ROBOTS_ENV`: Determines which version of `robots.txt` is used. This is read by the Robots controller. Any value other than `production` results in the non-production version being used. - `SCOUT_AUTO_INSTRUMENTS`: default is `false`. Recommended setting is `true` unless we add manual instrumentation in the future. - `SCOUT_DEV_TRACE`: only set in dev. Enables local, in-browser traces (does not send to ScoutAPM service. Other SCOUT ENV should not be set when using this) diff --git a/app/assets/stylesheets/partials/_pagination.scss b/app/assets/stylesheets/partials/_pagination.scss index cd16e5ac..e073b5c2 100644 --- a/app/assets/stylesheets/partials/_pagination.scss +++ b/app/assets/stylesheets/partials/_pagination.scss @@ -54,4 +54,13 @@ .center-elements {display: none;} } -} \ No newline at end of file +} + +.load-more { + clear: both; + display: flex; + justify-content: center; + padding-top: 1em; + border-top: 1px solid $color-border-default; + margin-bottom: 48px; +} diff --git a/app/controllers/search_controller.rb b/app/controllers/search_controller.rb index 2b57d580..b154fb52 100644 --- a/app/controllers/search_controller.rb +++ b/app/controllers/search_controller.rb @@ -25,7 +25,10 @@ def results # Load GeoData results if applicable if Feature.enabled?(:geodata) load_geodata_results - render 'results_geo' + respond_to do |format| + format.turbo_stream { render :results_geo } + format.html { render 'results_geo' } + end return end @@ -42,6 +45,7 @@ def results # Render the response in HTML or JSON format respond_to do |format| format.json { render json: { results: @results, pagination: @pagination, errors: @errors } } + format.turbo_stream { render :results } format.html { render :results } end end @@ -81,6 +85,8 @@ def load_geodata_results raw_results = extract_results(response) @results = NormalizeTimdexResults.new(raw_results, @enhanced_query[:q]).normalize @filters = extract_filters(response) + @append_results = @results + @load_more = load_more_from_pagination(@pagination) end def load_primo_results @@ -89,6 +95,8 @@ def load_primo_results @pagination = data[:pagination] @errors = data[:errors] @show_primo_continuation = data[:show_continuation] + @append_results = @results + @load_more = load_more_from_pagination(@pagination) end def load_timdex_results @@ -96,6 +104,8 @@ def load_timdex_results @results = data[:results] @pagination = data[:pagination] @errors = data[:errors] + @append_results = @results + @load_more = load_more_from_pagination(@pagination) end def load_all_results @@ -107,10 +117,46 @@ def load_all_results primo_fetcher: ->(offset:, per_page:, query: nil) { fetch_primo_data(offset: offset, per_page: per_page) }, timdex_fetcher: ->(offset:, per_page:, query: nil) { fetch_timdex_data(offset: offset, per_page: per_page) } ) - data = merge_service.fetch + display_count = sanitized_load_count + stable_count = [display_count - load_more_batch_size, 0].max + data = merge_service.fetch(display_count: display_count, stable_count: stable_count) @results = data[:results] + @append_results = data[:append_results] @errors = data[:errors] + @load_more = data[:load_more] + end + + # Builds the shared load-more view model for source-specific tabs that still + # use traditional page/offset mechanics internally. The UI presents one + # growing list, while the next request simply fetches the next existing page. + def load_more_from_pagination(pagination) + return nil if pagination.blank? + + { + display_count: pagination[:end], + next_page: pagination[:next], + has_more: pagination[:next].present?, + total_hits: pagination[:hits] + } + end + + # Returns the visible all-tab result count requested by the browser. This is + # intentionally separate from `page`: source-specific tabs keep using page for + # API offsets, while the all tab uses a display count to grow a stable reranked + # result list. + def sanitized_load_count + requested = params[:load_count].to_i + requested = load_more_batch_size if requested < 1 + [requested, max_load_count].min + end + + def load_more_batch_size + ENV.fetch('RESULTS_PER_PAGE', '20').to_i + end + + def max_load_count + ENV.fetch('LOAD_MORE_MAX_RESULTS', '200').to_i end def fetch_primo_data(offset: nil, per_page: nil) diff --git a/app/javascript/loading_spinner.js b/app/javascript/loading_spinner.js index 9aa6655c..bf8da4ea 100644 --- a/app/javascript/loading_spinner.js +++ b/app/javascript/loading_spinner.js @@ -14,7 +14,7 @@ function swapTabs(new_target) { } } -// Loading spinner behavior for pagination (Turbo Frame updates) +// Loading spinner behavior for frame and stream updates document.addEventListener('turbo:frame-render', function(event) { if (window.pendingFocusAction === 'pagination') { // Focus on first result for pagination @@ -51,8 +51,24 @@ document.addEventListener('turbo:frame-render', function(event) { }; }); +document.addEventListener('turbo:before-stream-render', function(event) { + if (window.pendingFocusAction === 'load-more') { + document.getElementById('search-results')?.classList.remove('spinner'); + window.pendingFocusAction = null; + } +}); + document.addEventListener('click', function(event) { const clickedElement = event.target; + const loadMoreLink = clickedElement.closest('.load-more-link'); + + // Handle load-more clicks. Results append in place, so do not scroll users + // back to the top or move focus away from their current reading position. + if (loadMoreLink) { + document.getElementById('search-results').classList.add('spinner'); + window.pendingFocusAction = 'load-more'; + return; + } // Handle pagination clicks if (clickedElement.matches('.first a, .previous a, .next a')) { @@ -100,4 +116,3 @@ document.addEventListener('turbo:load', function(event) { } }); - diff --git a/app/models/merged_search_service.rb b/app/models/merged_search_service.rb index 201e0128..4eb8ce4a 100644 --- a/app/models/merged_search_service.rb +++ b/app/models/merged_search_service.rb @@ -1,7 +1,15 @@ # Orchestrates merged "all" tab searches across Primo and TIMDEX using the -# reranker gem. Fetches a configurable number of results from each source in -# parallel, then delegates ordering to the configured scorer. +# reranker gem. +# +# The all tab cannot use traditional page numbers because each "load more" +# request may add new candidates from both source APIs and reranking that larger +# pool can change global order. To keep the user experience stable, this service +# caches a per-query candidate pool and ordered list of record keys. When the +# pool grows, it reranks everything again but preserves the prefix the browser +# has already displayed, appending only unseen records after that prefix. class MergedSearchService + TTL = 12.hours + # Initialize a new MergedSearchService. # # @param enhanced_query [Hash] query hash produced by `Enhancer` @@ -15,46 +23,178 @@ def initialize(enhanced_query:, active_tab:, primo_fetcher:, timdex_fetcher:) @timdex_fetcher = timdex_fetcher end - # Fetch and rerank results from both sources. + # Fetch and rerank results from both sources for the all tab. # - # @param per_source [Integer, nil] number of results to request from each API; - # defaults to the ALL_TAB_RESULTS_PER_SOURCE env var (or 50). - # @return [Hash] keys: :results, :errors - def fetch(per_source: nil) + # @param display_count [Integer, nil] number of ordered results the caller + # wants visible after this request. + # @param stable_count [Integer] number of leading results already displayed in + # the browser. Those records retain their relative order even if the expanded + # candidate pool would rerank them differently. + # @param per_source [Integer, nil] number of results to request from each API + # per source fetch; defaults to ALL_TAB_RESULTS_PER_SOURCE or 50. + # @return [Hash] keys: :results, :append_results, :errors, :load_more + def fetch(display_count: nil, stable_count: 0, per_source: nil) per_source = (per_source || ENV.fetch('ALL_TAB_RESULTS_PER_SOURCE', '50')).to_i - primo_data, timdex_data = parallel_fetch(per_page: per_source) + display_count = (display_count || ENV.fetch('RESULTS_PER_PAGE', '20')).to_i + display_count = [display_count, 1].max + stable_count = [stable_count.to_i, 0].max - primo_results = primo_data[:results] || [] - timdex_results = timdex_data[:results] || [] - errors = combine_errors(primo_data[:errors], timdex_data[:errors]) + state = Rails.cache.read(state_cache_key) || empty_state + state = ensure_ordered_results(state, display_count: display_count, stable_count: stable_count, + per_source: per_source) + Rails.cache.write(state_cache_key, state, expires_in: TTL) - merged = Reranker::Reranker.new(configured_scorer).rerank( - primo_results, timdex_results, query: @enhanced_query[:q] - ) + ordered_results = records_for_keys(state[:ordered_keys], state).first(display_count) + append_results = ordered_results[stable_count...display_count] || [] + total_hits = state[:primo_hits].to_i + state[:timdex_hits].to_i - { results: merged, errors: errors } + { + results: ordered_results, + append_results: append_results, + errors: state[:errors], + load_more: { + display_count: display_count, + next_count: display_count + visible_batch_size, + has_more: has_more?(state, display_count), + total_hits: total_hits + } + } end private + # Returns an empty serializable cache state for a merged all-tab search. + # Source result arrays hold normalized records. `ordered_keys` is the stable + # display order, represented by deterministic record keys so duplicated + # records can be removed safely when additional source chunks arrive. + def empty_state + { + primo_results: [], + timdex_results: [], + ordered_keys: [], + primo_hits: 0, + timdex_hits: 0, + primo_exhausted: false, + timdex_exhausted: false, + errors: nil + } + end + + # Ensures the cached state contains enough ordered results to satisfy the + # requested display count, fetching more source candidates when necessary. + def ensure_ordered_results(state, display_count:, stable_count:, per_source:) + state = rerank_state(state, stable_count: stable_count) if state[:ordered_keys].empty? && any_results?(state) + + while state[:ordered_keys].length < display_count && sources_available?(state) + previous_count = state[:ordered_keys].length + state = fetch_next_source_chunks(state, per_source: per_source) + state = rerank_state(state, stable_count: stable_count) + break if state[:ordered_keys].length == previous_count + end + + state + end + + # Fetches the next chunk from each source that can still produce records. The + # source offset is simply the number of normalized records already cached for + # that source, which preserves each API's native offset semantics while the + # service manages display order separately. + def fetch_next_source_chunks(state, per_source:) + primo_offset = state[:primo_results].length + timdex_offset = state[:timdex_results].length + + primo_data, timdex_data = parallel_fetch( + primo_offset: state[:primo_exhausted] || primo_offset >= Analyzer::PRIMO_MAX_OFFSET ? nil : primo_offset, + timdex_offset: state[:timdex_exhausted] ? nil : timdex_offset, + per_page: per_source + ) + + update_state_from_source!(state, :primo, primo_data, requested_offset: primo_offset, per_page: per_source) + update_state_from_source!(state, :timdex, timdex_data, requested_offset: timdex_offset, per_page: per_source) + state[:errors] = combine_errors(state[:errors], primo_data&.[](:errors), timdex_data&.[](:errors)) + + state + end + + # Merges a source response into the cached state and records whether that + # source appears exhausted. A source is exhausted when it returns fewer records + # than requested, reports no more total hits, or Primo signals continuation. + def update_state_from_source!(state, source, data, requested_offset:, per_page:) + return mark_exhausted!(state, source, reason: 'fetch skipped') if data.nil? + + results_key = source_results_key(source) + hits_key = source_hits_key(source) + incoming = Array(data[:results]) + + state[hits_key] = data[:hits].to_i + state[results_key] = dedupe_records(state[results_key] + incoming) + + exhaustion_reason = exhaustion_reason(data, incoming, requested_offset, per_page, state[hits_key]) + mark_exhausted!(state, source, reason: exhaustion_reason, offset: requested_offset, + returned: incoming.length, hits: state[hits_key]) if exhaustion_reason + end + + def mark_exhausted!(state, source, reason:, offset: nil, returned: nil, hits: nil) + state["#{source}_exhausted".to_sym] = true + Rails.logger.debug do + details = { source: source, reason: reason, offset: offset, returned: returned, hits: hits }.compact + "All-tab load more exhausted source: #{details}" + end + end + + # Returns a short reason string when a source can no longer provide useful + # load-more candidates, or nil while the source should remain available. + def exhaustion_reason(data, incoming, requested_offset, per_page, hits) + return 'Primo continuation required' if data[:show_continuation] + return 'empty response' if incoming.empty? + return 'short response' if incoming.length < per_page + return 'reported hits reached' if hits.to_i.positive? && requested_offset + incoming.length >= hits.to_i + + nil + end + # Fetch from Primo and TIMDEX concurrently. # # WARNING: exceptions raised inside threads do not automatically propagate; # callers should account for this. # + # @param primo_offset [Integer, nil] next Primo offset, or nil to skip Primo + # @param timdex_offset [Integer, nil] next TIMDEX offset, or nil to skip TIMDEX # @param per_page [Integer] number of results to request per source # @return [Array] [primo_response, timdex_response] - def parallel_fetch(per_page:) + def parallel_fetch(primo_offset:, timdex_offset:, per_page:) primo = nil timdex = nil - threads = [ - Thread.new { primo = @primo_fetcher.call(offset: 0, per_page: per_page, query: @enhanced_query) }, - Thread.new { timdex = @timdex_fetcher.call(offset: 0, per_page: per_page, query: @enhanced_query) } - ] + threads = [] + if primo_offset && primo_offset < Analyzer::PRIMO_MAX_OFFSET + threads << Thread.new do + primo = @primo_fetcher.call(offset: primo_offset, per_page: per_page, query: @enhanced_query) + end + end + if timdex_offset + threads << Thread.new do + timdex = @timdex_fetcher.call(offset: timdex_offset, per_page: per_page, query: @enhanced_query) + end + end threads.each(&:join) [primo, timdex] end + # Reranks the expanded candidate pool and preserves any already-visible prefix. + # This method intentionally treats the reranker as an interchangeable ordering + # engine: any scorer that returns ordered result hashes through the gem's + # stable API can be used without changing the load-more logic. + def rerank_state(state, stable_count:) + prefix = state[:ordered_keys].first(stable_count) + reranked = Reranker::Reranker.new(configured_scorer).rerank( + state[:primo_results], state[:timdex_results], query: @enhanced_query[:q] + ) + reranked_keys = dedupe_keys(reranked.map { |record| record_key(record) }) + + state[:ordered_keys] = prefix + reranked_keys.reject { |key| prefix.include?(key) } + state + end + # Instantiate the scorer based on the ALL_TAB_SCORER env var. # Defaults to ZscoreScorer. # @@ -69,6 +209,57 @@ def configured_scorer end end + def state_cache_key + query = @enhanced_query.except(:page).merge( + tab: @active_tab, + scorer: ENV.fetch('ALL_TAB_SCORER', 'zscore'), + boost_sources: ENV.fetch('ALL_TAB_BOOST_SOURCES', ''), + per_source: ENV.fetch('ALL_TAB_RESULTS_PER_SOURCE', '50') + ) + "#{CacheKeyGenerator.call(query)}/all-tab-load-more" + end + + def visible_batch_size + ENV.fetch('RESULTS_PER_PAGE', '20').to_i + end + + def any_results?(state) + state[:primo_results].any? || state[:timdex_results].any? + end + + def sources_available?(state) + !state[:primo_exhausted] || !state[:timdex_exhausted] + end + + def has_more?(state, display_count) + state[:ordered_keys].length > display_count || sources_available?(state) + end + + def source_results_key(source) + "#{source}_results".to_sym + end + + def source_hits_key(source) + "#{source}_hits".to_sym + end + + def records_for_keys(keys, state) + records_by_key = (state[:primo_results] + state[:timdex_results]).index_by { |record| record_key(record) } + keys.filter_map { |key| records_by_key[key] } + end + + def dedupe_records(records) + records.reverse.index_by { |record| record_key(record) }.values.reverse + end + + def dedupe_keys(keys) + keys.each_with_object([]) { |key, unique| unique << key unless unique.include?(key) } + end + + def record_key(record) + [record[:api], record[:identifier] || record[:sourceLink] || record[:source_link] || record[:title]].join(':') + end + # Merge multiple error arrays into a single array, or nil when empty. # # @return [Array, nil] diff --git a/app/views/search/_load_more.html.erb b/app/views/search/_load_more.html.erb new file mode 100644 index 00000000..545d4b9c --- /dev/null +++ b/app/views/search/_load_more.html.erb @@ -0,0 +1,22 @@ +<%# Shared load-more control. The UI is consistent across tabs, while the + backing params differ: all tab grows by visible result count; source tabs + keep using page internally to request the next API offset. %> +<% if @load_more&.[](:has_more) %> + <% params_copy = @enhanced_query.dup %> + <% params_copy[:tab] = @active_tab if @active_tab.present? %> + <% if @active_tab == 'all' %> + <% params_copy.delete(:page) %> + <% params_copy[:load_count] = @load_more[:next_count] %> + <% else %> + <% params_copy[:page] = @load_more[:next_page] %> + <% end %> + +
      + <%= link_to results_path(params_copy), class: 'btn button-secondary load-more-link', + data: { turbo_stream: true }, rel: 'nofollow' do %> + Load more results + <% end %> +
      +<% else %> +
      +<% end %> diff --git a/app/views/search/_result_item.html.erb b/app/views/search/_result_item.html.erb new file mode 100644 index 00000000..7b21067f --- /dev/null +++ b/app/views/search/_result_item.html.erb @@ -0,0 +1,8 @@ +<%# Renders one normalized result regardless of source API. Keeping this branch in + one partial lets full-page HTML and Turbo Stream append responses use exactly + the same result markup. %> +<% if result[:api] == 'primo' %> + <%= render(partial: 'search/result_primo', locals: { result: result }) %> +<% elsif result[:api] == 'timdex' %> + <%= render(partial: 'search/result', locals: { result: result }) %> +<% end %> diff --git a/app/views/search/results.html.erb b/app/views/search/results.html.erb index 9329db5a..a3d67290 100644 --- a/app/views/search/results.html.erb +++ b/app/views/search/results.html.erb @@ -17,23 +17,17 @@ <% elsif @results.present? && @errors.blank? %>
      -

      <%= results_summary(@pagination&.[](:hits) || @results.length) %>

      +

      <%= results_summary(@pagination&.[](:hits) || @load_more&.[](:total_hits) || @results.length) %>

      <% if @show_nls_warning %> <%= render partial: 'search/nls_alert' %> <% end %>
      -
        - <% @results.each do |result| %> - <% if result[:api] == 'primo' %> - <%= render(partial: 'search/result_primo', locals: { result: result }) %> - <% elsif result[:api] == 'timdex' %> - <%= render(partial: 'search/result', locals: { result: result }) %> - <% end %> - <% end %> +
          + <%= render partial: 'search/result_item', collection: @results, as: :result %>
        - <%= render partial: "pagination" %> + <%= render partial: "load_more" %> <%# Note `results_callouts` is also displayed in no errors condition below %> <%= render partial: "results_callouts" %>
      diff --git a/app/views/search/results.turbo_stream.erb b/app/views/search/results.turbo_stream.erb new file mode 100644 index 00000000..ebf5dd54 --- /dev/null +++ b/app/views/search/results.turbo_stream.erb @@ -0,0 +1,7 @@ +<%= turbo_stream.append 'results-list' do %> + <%= render partial: 'search/result_item', collection: @append_results, as: :result %> +<% end %> + +<%= turbo_stream.replace 'load-more' do %> + <%= render partial: 'search/load_more' %> +<% end %> diff --git a/app/views/search/results_geo.html.erb b/app/views/search/results_geo.html.erb index 1d025023..78d371c0 100644 --- a/app/views/search/results_geo.html.erb +++ b/app/views/search/results_geo.html.erb @@ -36,7 +36,7 @@
      <% if @results.present? && @errors.blank? %>

      <%= results_summary(@pagination[:hits]) %> returned

      -
        +
          <%= render(partial: 'search/result_geo', collection: @results, as: :result_geo) %>
        <% elsif @errors.blank? %> @@ -50,9 +50,7 @@ <%= render partial: 'shared/ask', locals: { display: 'aside' } if @results.blank? %> <% if @results.present? %> - + <%= render partial: "load_more" %> <%= render partial: 'shared/ask', locals: { display: 'view-md' } %> <% end %>
      diff --git a/app/views/search/results_geo.turbo_stream.erb b/app/views/search/results_geo.turbo_stream.erb new file mode 100644 index 00000000..27606b96 --- /dev/null +++ b/app/views/search/results_geo.turbo_stream.erb @@ -0,0 +1,7 @@ +<%= turbo_stream.append 'results-list' do %> + <%= render partial: 'search/result_geo', collection: @append_results, as: :result_geo %> +<% end %> + +<%= turbo_stream.replace 'load-more' do %> + <%= render partial: 'search/load_more' %> +<% end %> diff --git a/test/controllers/search_controller_test.rb b/test/controllers/search_controller_test.rb index e7da510b..b8916cd4 100644 --- a/test/controllers/search_controller_test.rb +++ b/test/controllers/search_controller_test.rb @@ -449,18 +449,18 @@ def build_timdex_mock_response end end - test 'primo results with valid query has div for pagination' do + test 'primo results with valid query has load more control' do mock_primo_search_success get '/results?q=data&tab=primo' assert_response :success - assert_select '#pagination' + assert_select '#load-more' end - test 'timdex results with valid query has div for pagination' do + test 'timdex results with valid query has load more control' do mock_timdex_search_success get '/results?q=data&tab=timdex' assert_response :success - assert_select '#pagination' + assert_select '#load-more' end test 'primo results with valid query has div for results which is populated' do @@ -1060,6 +1060,7 @@ def source_filter_count(controller) get '/results?q=test&tab=all' assert_response :success assert_select '#pagination', count: 0 + assert_select '#load-more' end test 'results can be returned in JSON format when env is set and valid token is provided' do diff --git a/test/models/merged_search_service_test.rb b/test/models/merged_search_service_test.rb index 5c61b68e..3ed8b120 100644 --- a/test/models/merged_search_service_test.rb +++ b/test/models/merged_search_service_test.rb @@ -1,59 +1,63 @@ require 'test_helper' class MergedSearchServiceTest < ActiveSupport::TestCase - test 'page 1 writes totals to cache' do + test 'fetch writes state cache with source hit totals' do query = { q: 'test' } primo_fetcher = lambda do |offset:, per_page:, query:| - { results: ['foo'], hits: 42, errors: nil, show_continuation: false } + { results: [{ title: 'P1', score: 0.9, api: 'primo', identifier: 'p1' }], hits: 42, errors: nil, + show_continuation: false } end timdex_fetcher = lambda do |offset:, per_page:, query:| - { results: ['bar'], hits: 37, errors: nil } + { results: [{ title: 'T1', score: 0.8, api: 'timdex', identifier: 't1' }], hits: 37, errors: nil } end service = MergedSearchService.new(enhanced_query: query, active_tab: 'all', primo_fetcher: primo_fetcher, timdex_fetcher: timdex_fetcher) - res = service.fetch(page: 1, per_page: 20) + res = service.fetch(display_count: 20) assert_equal 2, res[:results].length # Verify cache written - key = service.send(:totals_cache_key) + key = service.send(:state_cache_key) cached = Rails.cache.read(key) refute_nil cached - assert_equal 42, cached[:primo] - assert_equal 37, cached[:timdex] + assert_equal 42, cached[:primo_hits] + assert_equal 37, cached[:timdex_hits] end - test 'deeper page reads cached totals and avoids summary calls' do + test 'fetch uses cached state when enough ordered results already exist' do query = { q: 'test' } - service = MergedSearchService.new(enhanced_query: query, active_tab: 'all', - primo_fetcher: fake_fetcher, timdex_fetcher: fake_fetcher) - - # populate cache so service uses it instead of summary calls - Rails.cache.write(service.send(:totals_cache_key), { primo: 50, timdex: 50 }) - - # fetchers that would raise if a summary call (per_page == 1) is attempted + # Fetchers that would raise if the service attempted to fetch despite a + # cache hit with enough ordered results. primo_fetcher = lambda do |offset:, per_page:, query:| - raise 'Summary call made' if per_page == 1 - - { results: ['foo'], hits: 50, errors: nil, show_continuation: false } + raise 'Unexpected Primo fetch' end timdex_fetcher = lambda do |offset:, per_page:, query:| - raise 'Summary call made' if per_page == 1 - - { results: ['bar'], hits: 50, errors: nil } + raise 'Unexpected TIMDEX fetch' end service = MergedSearchService.new(enhanced_query: query, active_tab: 'all', primo_fetcher: primo_fetcher, timdex_fetcher: timdex_fetcher) + cached_state = { + primo_results: [{ title: 'P1', score: 0.9, api: 'primo', identifier: 'p1' }], + timdex_results: [{ title: 'T1', score: 0.8, api: 'timdex', identifier: 't1' }], + ordered_keys: %w[primo:p1 timdex:t1], + primo_hits: 50, + timdex_hits: 50, + primo_exhausted: false, + timdex_exhausted: false, + errors: nil + } + Rails.cache.write(service.send(:state_cache_key), cached_state) + # Should not raise assert_nothing_raised do - res = service.fetch(page: 2, per_page: 20) + res = service.fetch(display_count: 2, stable_count: 0) assert res[:results].is_a?(Array) end end @@ -61,7 +65,6 @@ class MergedSearchServiceTest < ActiveSupport::TestCase test 'fetch calls both fetchers at offset 0 with configured per_source' do q = { q: 'test' } - calls = [] fetcher = lambda do |offset:, per_page:, query: nil| calls << [offset, per_page] @@ -79,51 +82,71 @@ class MergedSearchServiceTest < ActiveSupport::TestCase end end - test 'all tab cold cache deeper page calls timdex twice' do - timdex_calls = [] + test 'fetch returns merged results from both sources' do + primo_fetcher = fake_fetcher(results: [{ title: 'P1', score: 0.9, api: 'primo', identifier: 'p1' }], hits: 1) + timdex_fetcher = fake_fetcher(results: [{ title: 'T1', score: 0.8, api: 'timdex', identifier: 't1' }], hits: 1) - fake_timdex = fake_fetcher(results: %w[t1 t2 t3 t4 t5], hits: 150, calls: timdex_calls) - fake_primo = fake_fetcher(results: %w[p1 p2 p3], hits: 100) - service = MergedSearchService.new(enhanced_query: { q: 'test' }, active_tab: 'all', - primo_fetcher: fake_primo, timdex_fetcher: fake_timdex) + svc = MergedSearchService.new(enhanced_query: { q: 'test' }, active_tab: 'all', + primo_fetcher: primo_fetcher, timdex_fetcher: timdex_fetcher) + result = svc.fetch - # Verify cache is cold - assert_nil Rails.cache.read(service.send(:totals_cache_key)) + assert_equal 2, result[:results].length + titles = result[:results].map { |r| r[:title] } + assert_includes titles, 'P1' + assert_includes titles, 'T1' + end - # Fetch page 7, verify TIMDEX was called twice - service.fetch(page: 7, per_page: 10) - assert_equal 2, timdex_calls.length + test 'fetch appends only the new stable slice on load more' do + primo_calls = [] + timdex_calls = [] + primo_fetcher = lambda do |offset:, per_page:, query: nil| + primo_calls << offset + results = [ + { title: 'P1', score: 0.9, api: 'primo', identifier: 'p1' }, + { title: 'P2', score: 0.8, api: 'primo', identifier: 'p2' }, + { title: 'P3', score: 0.7, api: 'primo', identifier: 'p3' }, + { title: 'P4', score: 0.6, api: 'primo', identifier: 'p4' } + ].slice(offset, per_page) || [] + { results: results, hits: 4, errors: nil, show_continuation: false } + end + timdex_fetcher = lambda do |offset:, per_page:, query: nil| + timdex_calls << offset + results = [ + { title: 'T1', score: 0.95, api: 'timdex', identifier: 't1' }, + { title: 'T2', score: 0.85, api: 'timdex', identifier: 't2' }, + { title: 'T3', score: 0.75, api: 'timdex', identifier: 't3' }, + { title: 'T4', score: 0.65, api: 'timdex', identifier: 't4' } + ].slice(offset, per_page) || [] + { results: results, hits: 4, errors: nil } + end - # First call: summary (offset=0, per_page=1) - assert_equal({ offset: 0, per_page: 1 }, timdex_calls[0]) + svc = MergedSearchService.new(enhanced_query: { q: 'test' }, active_tab: 'all', + primo_fetcher: primo_fetcher, timdex_fetcher: timdex_fetcher) - # Second call: page data (offset/per_page computed by MergedSearchPaginator) - paginator = MergedSearchPaginator.new(primo_total: 100, timdex_total: 150, current_page: 7, per_page: 10) - expected_offset = paginator.api_offsets[1] - expected_count = paginator.merge_plan.count(:timdex) - assert_equal({ offset: expected_offset, per_page: expected_count }, timdex_calls[1]) - end + first = svc.fetch(display_count: 2, stable_count: 0, per_source: 1) + second = svc.fetch(display_count: 4, stable_count: 2, per_source: 1) - test 'fetch_all_tab_page_chunks handles zero-count branches' do - called = [] - primo_fetcher = lambda { |offset:, per_page:, query:| - called << [:primo, offset, per_page] - { results: ['P'], hits: 5, errors: nil, show_continuation: false } - } - timdex_fetcher = lambda { |offset:, per_page:, query:| - called << [:timdex, offset, per_page] - { results: [], hits: 0, errors: nil } - } + assert_equal(first[:results].map { |result| result[:identifier] }, + second[:results].first(2).map { |result| result[:identifier] }) + assert_equal(second[:results].last(2).map { |result| result[:identifier] }, + second[:append_results].map { |result| result[:identifier] }) + assert_equal [0, 1], primo_calls + assert_equal [0, 1], timdex_calls + end + test 'fetch stops when duplicate source chunks do not grow the ordered result set' do + calls = [] + fetcher = lambda do |offset:, per_page:, query: nil| + calls << offset + { results: [{ title: 'Same', score: 1.0, api: 'primo', identifier: 'same' }], hits: 100, errors: nil } + end svc = MergedSearchService.new(enhanced_query: { q: 'test' }, active_tab: 'all', - primo_fetcher: primo_fetcher, timdex_fetcher: timdex_fetcher) - result = svc.fetch + primo_fetcher: fetcher, timdex_fetcher: fake_fetcher) + result = svc.fetch(display_count: 5, stable_count: 0, per_source: 1) - assert_equal 2, result[:results].length - titles = result[:results].map { |r| r[:title] } - assert_includes titles, 'P1' - assert_includes titles, 'T1' + assert_equal 1, result[:results].length + assert_operator calls.length, :<=, 2 end test 'fetch defaults to 50 results per source when env var is not set' do @@ -194,58 +217,25 @@ class MergedSearchServiceTest < ActiveSupport::TestCase # intentionally removed; the service now requires injected fetchers so # per-backend behavior should be tested in their respective unit tests. - test 'merge_results handles unbalanced API responses correctly' do - # Test case 1: Primo has fewer results than TIMDEX - paginator = MergedSearchPaginator.new(primo_total: 3, timdex_total: 5, current_page: 1, per_page: 8) - primo_results = %w[P1 P2 P3] - timdex_results = %w[T1 T2 T3 T4 T5] - svc = MergedSearchService.new(enhanced_query: { q: 'test' }, active_tab: 'all', primo_fetcher: fake_fetcher, - timdex_fetcher: fake_fetcher) - merged = svc.send(:merge_results, paginator, primo_results, timdex_results) - expected = %w[P1 T1 P2 T2 P3 T3 T4 T5] - assert_equal expected, merged - - # Test case 2: TIMDEX has fewer results than Primo - paginator = MergedSearchPaginator.new(primo_total: 5, timdex_total: 3, current_page: 1, per_page: 8) - primo_results = %w[P1 P2 P3 P4 P5] - timdex_results = %w[T1 T2 T3] - svc = MergedSearchService.new(enhanced_query: { q: 'test' }, active_tab: 'all', primo_fetcher: fake_fetcher, - timdex_fetcher: fake_fetcher) - merged = svc.send(:merge_results, paginator, primo_results, timdex_results) - expected = %w[P1 T1 P2 T2 P3 T3 P4 P5] - assert_equal expected, merged - - # Test case 3: Results exceed per_page limit (default 20) - paginator = MergedSearchPaginator.new(primo_total: 15, timdex_total: 15, current_page: 1, per_page: 20) - primo_results = (1..15).map { |i| "P#{i}" } - timdex_results = (1..15).map { |i| "T#{i}" } - svc = MergedSearchService.new(enhanced_query: { q: 'test' }, active_tab: 'all', primo_fetcher: fake_fetcher, - timdex_fetcher: fake_fetcher) - merged = svc.send(:merge_results, paginator, primo_results, timdex_results) - assert_equal 20, merged.length - assert_equal 'P1', merged[0] - assert_equal 'T1', merged[1] - assert_equal 'P2', merged[2] - assert_equal 'T2', merged[3] - - # Test case 4: One array is empty - paginator = MergedSearchPaginator.new(primo_total: 0, timdex_total: 3, current_page: 1, per_page: 3) - primo_results = [] - timdex_results = %w[T1 T2 T3] - svc = MergedSearchService.new(enhanced_query: { q: 'test' }, active_tab: 'all', primo_fetcher: fake_fetcher, - timdex_fetcher: fake_fetcher) - merged = svc.send(:merge_results, paginator, primo_results, timdex_results) - assert_equal %w[T1 T2 T3], merged - - # Test case 5: more than 10 results from a single source can display when appropriate - paginator = MergedSearchPaginator.new(primo_total: 7, timdex_total: 11, current_page: 1, per_page: 18) - primo_results = (1..7).map { |i| "P#{i}" } - timdex_results = (1..11).map { |i| "T#{i}" } - svc = MergedSearchService.new(enhanced_query: { q: 'test' }, active_tab: 'all', primo_fetcher: fake_fetcher, - timdex_fetcher: fake_fetcher) - merged = svc.send(:merge_results, paginator, primo_results, timdex_results) - expected = %w[P1 T1 P2 T2 P3 T3 P4 T4 P5 T5 P6 T6 P7 T7 T8 T9 T10 T11] - assert_equal expected, merged + test 'fetch handles unbalanced source responses and returns all available records' do + primo_all = (1..3).map { |i| { title: "P#{i}", score: 1.0 - (i * 0.01), api: 'primo', identifier: "p#{i}" } } + timdex_all = (1..5).map { |i| { title: "T#{i}", score: 1.0 - (i * 0.01), api: 'timdex', identifier: "t#{i}" } } + + primo_fetcher = lambda do |offset:, per_page:, query: nil| + { results: primo_all.slice(offset, per_page) || [], hits: primo_all.length, errors: nil, + show_continuation: false } + end + timdex_fetcher = lambda do |offset:, per_page:, query: nil| + { results: timdex_all.slice(offset, per_page) || [], hits: timdex_all.length, errors: nil } + end + + svc = MergedSearchService.new(enhanced_query: { q: 'test' }, active_tab: 'all', + primo_fetcher: primo_fetcher, timdex_fetcher: timdex_fetcher) + result = svc.fetch(display_count: 8, stable_count: 0, per_source: 2) + + assert_equal 8, result[:results].length + ids = result[:results].map { |record| record[:identifier] }.sort + assert_equal %w[p1 p2 p3 t1 t2 t3 t4 t5], ids end end From 90f97a72e337f4c6681a1400ebc44440d0800eee Mon Sep 17 00:00:00 2001 From: Jeremy Prevost Date: Thu, 30 Jul 2026 17:09:22 -0400 Subject: [PATCH 3/5] Display result number --- app/assets/stylesheets/partials/_results.scss | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/assets/stylesheets/partials/_results.scss b/app/assets/stylesheets/partials/_results.scss index 79170a11..68eba5d9 100644 --- a/app/assets/stylesheets/partials/_results.scss +++ b/app/assets/stylesheets/partials/_results.scss @@ -45,8 +45,10 @@ } .results-list.use { - list-style-type: none; + list-style-type: decimal; + list-style-position: outside; padding: 0; + padding-left: 2.5rem; margin-bottom: 0; } From 45a7229146011a1eb7ec78d7f80b033f88192791 Mon Sep 17 00:00:00 2001 From: Jeremy Prevost Date: Mon, 3 Aug 2026 15:42:23 -0400 Subject: [PATCH 4/5] Keep default zipper for initial work --- AGENTS.md | 2 +- app/models/merged_search_service.rb | 14 ++++++++------ test/models/merged_search_service_test.rb | 5 ++--- 3 files changed, 11 insertions(+), 10 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index d6ab841a..9be3732c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -30,7 +30,7 @@ This file highlights the important, discoverable conventions and workflows an AI Essential ENV vars for core functionality: `TIMDEX_GRAPHQL`, `PRIMO_API_URL`, `PRIMO_API_KEY`, `RESULTS_PER_PAGE`, `TIMDEX_INDEX`, `TIMDEX_SOURCES`. Filter customization: `FILTER_*` (e.g., `FILTER_LANGUAGE`, `FILTER_CONTENT_TYPE`) and `ACTIVE_FILTERS` (comma-separated list controlling visibility/order of filters; note that filter aggregation keys in the schema use `*Filter` suffix, e.g., `languageFilter`, `contentTypeFilter`). Tests rely on `.env.test` values for VCR cassette generation and use `ClimateControl` gem to mock feature flags. -- **Parallel fetching, reranking & load more:** The `all` tab uses `MergedSearchService` to fetch Primo and TIMDEX concurrently via `Thread.new`, then passes both result sets to the `reranker` gem for scoring and ordering. The scorer is configurable via `ALL_TAB_SCORER` (default: `zscore`). `ALL_TAB_RESULTS_PER_SOURCE` (default: 50) controls how many candidates are fetched from each API when the cached all-tab pool needs to grow; `RESULTS_PER_PAGE` controls the initial visible batch size and each "Load more results" increment. The all tab preserves already-shown result order when reranking an expanded candidate pool, appending only unseen records. Source-specific tabs use the same load-more UI but still rely on their normal API page/offset behavior internally. `LOAD_MORE_MAX_RESULTS` caps visible load-more growth. Be careful when refactoring to preserve thread-safety, cache keys, stable-prefix behavior, and source exhaustion handling. +- **Parallel fetching, reranking & load more:** The `all` tab uses `MergedSearchService` to fetch Primo and TIMDEX concurrently via `Thread.new`, then passes both result sets to the `reranker` gem for scoring and ordering. The scorer is configurable via `ALL_TAB_SCORER` (default: `zipper`). `ALL_TAB_RESULTS_PER_SOURCE` (default: 50) controls how many candidates are fetched from each API when the cached all-tab pool needs to grow; `RESULTS_PER_PAGE` controls the initial visible batch size and each "Load more results" increment. The all tab preserves already-shown result order when reranking an expanded candidate pool, appending only unseen records. Source-specific tabs use the same load-more UI but still rely on their normal API page/offset behavior internally. `LOAD_MORE_MAX_RESULTS` caps visible load-more growth. Be careful when refactoring to preserve thread-safety, cache keys, stable-prefix behavior, and source exhaustion handling. - **JS stack & conventions:** Rails importmap is in use (`importmap-rails`). JavaScript entry is `app/javascript/application.js`. Stimulus controllers live in `app/javascript/controllers` and are imported by `importmap` via `config/importmap.rb`. Key controllers include `content_loader_controller.js` (dynamic content loading) and tab management via `source_tabs.js` (which handles geospatial UI state). Prefer small, focused changes to Stimulus controllers rather than heavy bundler-based rewrites. diff --git a/app/models/merged_search_service.rb b/app/models/merged_search_service.rb index 4eb8ce4a..625b3e4e 100644 --- a/app/models/merged_search_service.rb +++ b/app/models/merged_search_service.rb @@ -130,12 +130,14 @@ def update_state_from_source!(state, source, data, requested_offset:, per_page:) state[results_key] = dedupe_records(state[results_key] + incoming) exhaustion_reason = exhaustion_reason(data, incoming, requested_offset, per_page, state[hits_key]) + return unless exhaustion_reason + mark_exhausted!(state, source, reason: exhaustion_reason, offset: requested_offset, - returned: incoming.length, hits: state[hits_key]) if exhaustion_reason + returned: incoming.length, hits: state[hits_key]) end def mark_exhausted!(state, source, reason:, offset: nil, returned: nil, hits: nil) - state["#{source}_exhausted".to_sym] = true + state[:"#{source}_exhausted"] = true Rails.logger.debug do details = { source: source, reason: reason, offset: offset, returned: returned, hits: hits }.compact "All-tab load more exhausted source: #{details}" @@ -196,11 +198,11 @@ def rerank_state(state, stable_count:) end # Instantiate the scorer based on the ALL_TAB_SCORER env var. - # Defaults to ZscoreScorer. + # Defaults to ZipperMergeScorer. # # @return [Reranker::Scorer] def configured_scorer - case ENV.fetch('ALL_TAB_SCORER', 'zscore').downcase + case ENV.fetch('ALL_TAB_SCORER', 'zipper').downcase when 'zscore' then Reranker::ZscoreScorer.new when 'zipper' then Reranker::ZipperMergeScorer.new when 'simple' then Reranker::SimpleScorer.new @@ -236,11 +238,11 @@ def has_more?(state, display_count) end def source_results_key(source) - "#{source}_results".to_sym + :"#{source}_results" end def source_hits_key(source) - "#{source}_hits".to_sym + :"#{source}_hits" end def records_for_keys(keys, state) diff --git a/test/models/merged_search_service_test.rb b/test/models/merged_search_service_test.rb index 3ed8b120..db789da9 100644 --- a/test/models/merged_search_service_test.rb +++ b/test/models/merged_search_service_test.rb @@ -163,11 +163,11 @@ class MergedSearchServiceTest < ActiveSupport::TestCase assert_equal [50, 50], per_page_seen end - test 'configured_scorer returns ZscoreScorer by default' do + test 'configured_scorer returns Zipper by default' do svc = MergedSearchService.new(enhanced_query: { q: 'test' }, active_tab: 'all', primo_fetcher: fake_fetcher, timdex_fetcher: fake_fetcher) ClimateControl.modify(ALL_TAB_SCORER: nil) do - assert_instance_of Reranker::ZscoreScorer, svc.send(:configured_scorer) + assert_instance_of Reranker::ZipperMergeScorer, svc.send(:configured_scorer) end end @@ -237,5 +237,4 @@ class MergedSearchServiceTest < ActiveSupport::TestCase ids = result[:results].map { |record| record[:identifier] }.sort assert_equal %w[p1 p2 p3 t1 t2 t3 t4 t5], ids end - end From 7875e39a7ff5d983a5f18d71dffa5684dabddc50 Mon Sep 17 00:00:00 2001 From: Jeremy Prevost Date: Tue, 4 Aug 2026 08:59:31 -0400 Subject: [PATCH 5/5] Address automated review feedback --- README.md | 2 +- app/javascript/loading_spinner.js | 4 ++-- app/models/merged_search_service.rb | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 1e158cf6..50af0630 100644 --- a/README.md +++ b/README.md @@ -145,7 +145,7 @@ may have unexpected consequences if applied to other TIMDEX UI apps. - `REDIRECT_REQUESTS_PER_PERIOD`- number of requests that can be made that the query string starts with our legacy redirect parameter to throttle per `REQUEST_PERIOD` - `REDIRECT_REQUEST_PERIOD`- time in minutes used along with `REDIRECT_REQUESTS_PER_PERIOD` - `ALL_TAB_RESULTS_PER_SOURCE`: Number of candidate results to fetch from each API (Primo and TIMDEX) when the combined all tab needs to grow its cached reranked pool. Defaults to 50 if unset. This is not the number of results shown per click; use `RESULTS_PER_PAGE` for that. Keep below 960 to avoid hitting Primo's offset limit. -- `ALL_TAB_SCORER`: Scorer to use when merging results on the combined all tab. Valid values: `zscore` (default), `zipper`, `simple`, `random`. See the [reranker gem](https://github.com/MITLibraries/reranker) for details. +- `ALL_TAB_SCORER`: Scorer to use when merging results on the combined all tab. Valid values: `zipper` (default), `zscore`, `simple`, `random`. See the [reranker gem](https://github.com/MITLibraries/reranker) for details. - `LOAD_MORE_MAX_RESULTS`: Maximum number of results that can be shown through load-more requests for a single results tab. Defaults to 200 if unset. This guards against accidental oversized all-tab reranking requests. - `RESULTS_PER_PAGE`: The number of results shown in the initial batch and added by each "Load more results" click. For source-specific tabs this also remains the underlying API page size. Defaults to 20 if unset. - `ROBOTS_ENV`: Determines which version of `robots.txt` is used. This is read by the Robots controller. Any value other than `production` results in the non-production version being used. diff --git a/app/javascript/loading_spinner.js b/app/javascript/loading_spinner.js index bf8da4ea..4845ab78 100644 --- a/app/javascript/loading_spinner.js +++ b/app/javascript/loading_spinner.js @@ -65,7 +65,7 @@ document.addEventListener('click', function(event) { // Handle load-more clicks. Results append in place, so do not scroll users // back to the top or move focus away from their current reading position. if (loadMoreLink) { - document.getElementById('search-results').classList.add('spinner'); + document.getElementById('search-results')?.classList.add('spinner'); window.pendingFocusAction = 'load-more'; return; } @@ -73,7 +73,7 @@ document.addEventListener('click', function(event) { // Handle pagination clicks if (clickedElement.matches('.first a, .previous a, .next a')) { // Throw the spinner on the search results immediately - document.getElementById('search-results').classList.add('spinner'); + document.getElementById('search-results')?.classList.add('spinner'); // Position the window at the top of the results window.scrollTo({ top: 0, behavior: 'smooth' }); diff --git a/app/models/merged_search_service.rb b/app/models/merged_search_service.rb index 625b3e4e..d86cae17 100644 --- a/app/models/merged_search_service.rb +++ b/app/models/merged_search_service.rb @@ -214,7 +214,7 @@ def configured_scorer def state_cache_key query = @enhanced_query.except(:page).merge( tab: @active_tab, - scorer: ENV.fetch('ALL_TAB_SCORER', 'zscore'), + scorer: ENV.fetch('ALL_TAB_SCORER', 'zipper'), boost_sources: ENV.fetch('ALL_TAB_BOOST_SOURCES', ''), per_source: ENV.fetch('ALL_TAB_RESULTS_PER_SOURCE', '50') )