Implement unified 'all' search tab #274

jazairi · 2025-11-12T21:36:03Z

Why these changes are being introduced:

A key requirement for USE UI is a tab that combines results from multiple sources. The current interface requires users to search each source separately, creating friction in the discovery process.

Relevant ticket(s):

How this addresses that need:

Add new 'all' tab as the default search mode
Implement parallel API fetching using Ruby threads to query both Primo and TIMDEX simultaneously
Use zipper merge pattern to interleave results from both sources
Update view templates to render source-specific results for 'all' tab
Add tab parameter validation to prevent invalid selections
Change default tab from 'primo' to 'all' in ApplicationController

Side effects of this change:

Default search behavior changes from Primo-only to combined results
A valid_tab? method now confirms that the tab param is one of 'primo,' 'timdex', or 'all'
Possible increased server load due to parallel API calls (I haven't confirmed this, but it seems likely)
Some peripherally related tests have been updated to account for new behavior
Pagination is not yet implemented (this will be done in USE-178 and USE-179)

Developer

Accessibility

ANDI or WAVE has been run in accordance to our guide.
This PR contains no changes to the view layer.
New issues flagged by ANDI or WAVE have been resolved.
New issues flagged by ANDI or WAVE have been ticketed (link in the Pull Request details above).
No new accessibility issues have been flagged.

New ENV

All new ENV is documented in README.
All new ENV has been added to Heroku Pipeline, Staging and Prod.
ENV has not changed.

Approval beyond code review

UXWS/stakeholder approval has been confirmed.
UXWS/stakeholder review will be completed retroactively.
UXWS/stakeholder review is not needed.

Additional context needed to review

Some functionality to confirm:

GeoData continues to work as expected
Searches default to 'all' tab if cookie is not set
Searches default to session tab if cookie is set
If an invalid tab is provided and a cookie is set, it defaults to the tab set in the cookie
If an invalid tab is provided and no cookie is set, it defaults to the 'all' tab
The 'all' tab alternates between Primo and TIMDEX results (when available)
The primo continuation partials shows at higher page counts

Code Reviewer

Code

I have confirmed that the code works as intended.
Any CodeClimate issues have been fixed or confirmed as
added technical debt.

Documentation

The commit message is clear and follows our guidelines
(not just this pull request message).
The documentation has been updated or is unnecessary.
New dependencies are appropriate or there were no changes.

Testing

There are appropriate tests covering any new functionality.
No additional test coverage is required.

coveralls · 2025-11-12T21:39:04Z

Pull Request Test Coverage Report for Build 19340970745

Details

59 of 59 (100.0%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.08%) to 97.953%

Totals
Change from base Build 19335274631:	0.08%
Covered Lines:	909
Relevant Lines:	928

💛 - Coveralls

JPrevost

Nice initial implementation of this feature!

Before we merge I think we should decide what to do with geodata in terms of set_active_tab. We should either not set it, allow it to be valid, or only allow it to be valid in geodata mode.

⚠️ Work in progress from Matt (open PR) changes the tabs to use a Helper so if that code lands first you'll need to refactor (and vice versa). It should be clean as he abstracted out the link generation so there is less copy/paste.

JPrevost · 2025-11-13T13:39:13Z

test/controllers/application_controller_test.rb

      assert_select '#tab-to-target' do
        refute_select '[value=?]', 'primo'
-        assert_select '[value=?]', 'geodata'
+        assert_select '[value=?]', 'all'


This test name and assertions are no longer aligned. The test says to expect geodata but shows all is returned. See my other comment about our need to confirm our intent with the geodata logic in set_active_tab which will affect whether we change the test name or assertions here.

JPrevost · 2025-11-13T13:47:25Z

app/controllers/application_controller.rb

  def set_active_tab
    @active_tab = if Feature.enabled?(:geodata)
                    # Determine which tab to load - default to primo unless gdt is enabled
                    'geodata'


This value can never be achieved unless we include it in the valid_tab? method. I believe a few tests were changed to remove geodata as a possible tab value. I could probably be swayed to not set geodata as a tab as it isn't actually currently a tab, but if we are doing that we should remove the logic that actually tries to set it as an invalid value :)

My reasoning for setting a geodata tab in the first place was possibly flawed and I'm open to removing this first condition and running in geodata mode to confirm if it is actually needed.

I think it still works because we're not calling valid_tab? in that branch of the condition. But either way, it's confusing enough that it should be refactored. I'll consider alternatives.

JPrevost · 2025-11-13T13:48:53Z

app/controllers/application_controller.rb

+  private
+
+  def valid_tab?(tab)
+    %w[primo timdex all].include?(tab)


Alternatively to removing the first condition in the active_tab logic, we should instead set geodata as a valid tab. I think in real-world scenarios that is fine but when changing your local or a pr build between geodata and normal you may end up in wonky states where the cookie says to load an invalid tab?

I think I'd either remove the geodata logic from active_tab or keep it and add which tabs are valid here in each mode with a feature flag. I'm happy to chat if it would be helpful to work through the edge cases we are trying to support in real time.

JPrevost · 2025-11-13T13:51:59Z

test/controllers/application_controller_test.rb

+    assert_equal cookies[:last_tab], 'all'
+  end
+
+  test 'set_active_tab ignores invalid cookie value and uses default' do


I appreciate this test even though it may feel like it isn't a state we can get in to. I suspect we'll change tab configurations a few times over the next few months and this will be critical to make sure user browsers can handle it. 🍻

JPrevost · 2025-11-13T13:55:07Z

app/controllers/search_controller.rb

+    # Zipper merge results from both APIs
+    primo_results = primo_data[:results] || []
+    timdex_results = timdex_data[:results] || []
+    @results = primo_results.zip(timdex_results).flatten.compact


jazairi · 2025-11-13T15:57:16Z

@JPrevost I think the latest commit makes the GeoData toggling logic a bit clearer. Could you confirm? In the process, I removed a bunch of instances of 'gdt/GDT', so apologies that it touches more files than necessary.

JPrevost

@jazairi Those changes work for me.

I'm going to hold off on formal approval until the rebase with the active tab logic Matt just merged as I know there is a merge conflict and want to do a click test of the resolved merge before approval.

Why these changes are being introduced: A key requirement for USE UI is a tab that combines results from multiple sources. The current interface requires users to search each source separately, creating friction in the discovery process. Relevant ticket(s): - [USE-176](https://mitlibraries.atlassian.net/browse/USE-176) - [USE-177](https://mitlibraries.atlassian.net/browse/USE-177) How this addresses that need: - Add new 'all' tab as the default search mode - Implement parallel API fetching using Ruby threads to query both Primo and TIMDEX simultaneously - Use zipper merge pattern to interleave results from both sources - Update view templates to render source-specific results for 'all' tab - Add tab parameter validation to prevent invalid selections - Change default tab from 'primo' to 'all' in ApplicationController Side effects of this change: - Default search behavior changes from Primo-only to combined results - A `valid_tab?` method now confirms that the tab param is one of 'primo,' 'timdex', or 'all' - Possible increased server load due to parallel API calls (I haven't confirmed this, but it seems likely) - Some peripherally related tests have been updated to account for new behavior - Pagination is not yet implemented (this will be done in USE-178 and USE-179)

Why these changes are being introduced: With the implementation of the 'all' tab, which includes a `valid_tab?` method, it's become confusing as to how GeoData fits into the tab system. We currently treat it as a tab, but it's also treated as a feature. Relevant ticket(s): - [USE-176](https://mitlibraries.atlassian.net/browse/USE-176) - [USE-177](https://mitlibraries.atlassian.net/browse/USE-177) How this addresses that need: This normalizes the GeoData feature flagging in the controller, such that we only check for `Feature.enabled?` when toggling GeoData, rather than a combination of `active_tab` and the `Feature` model. Side effects of this change: Remaining instances where 'GeoData/geodata' was referred to as 'GDT/gdt' have been updated to use the actual property name.

jazairi · 2025-11-13T17:57:21Z

@JPrevost The latest commit includes a rebase of Matt's work.

JPrevost

jazairi requested review from JPrevost and matt-bernhardt November 12, 2025 21:38

mitlib temporarily deployed to timdex-ui-pi-use-176-aeh7wzjyn November 12, 2025 21:38 Inactive

JPrevost reviewed Nov 13, 2025

View reviewed changes

jazairi temporarily deployed to timdex-ui-pi-use-176-aeh7wzjyn November 13, 2025 15:56 Inactive

JPrevost reviewed Nov 13, 2025

View reviewed changes

jazairi added 2 commits November 13, 2025 12:55

jazairi force-pushed the use-176 branch from eb49c33 to ed66d9b Compare November 13, 2025 17:56

jazairi temporarily deployed to timdex-ui-pi-use-176-aeh7wzjyn November 13, 2025 17:56 Inactive

JPrevost approved these changes Nov 13, 2025

View reviewed changes

jazairi merged commit dba03f9 into main Nov 13, 2025
5 checks passed

jazairi deleted the use-176 branch November 13, 2025 18:58

Implement unified 'all' search tab #274

Implement unified 'all' search tab #274

Uh oh!

Conversation

jazairi commented Nov 12, 2025 • edited by atlassian bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why these changes are being introduced:

Relevant ticket(s):

How this addresses that need:

Side effects of this change:

Developer

Accessibility

New ENV

Approval beyond code review

Additional context needed to review

Code Reviewer

Code

Documentation

Testing

Uh oh!

coveralls commented Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 19340970745

Details

💛 - Coveralls

Uh oh!

JPrevost left a comment

Choose a reason for hiding this comment

Uh oh!

JPrevost Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

JPrevost Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

jazairi Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

JPrevost Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

JPrevost Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

JPrevost Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

jazairi commented Nov 13, 2025

Uh oh!

JPrevost left a comment

Choose a reason for hiding this comment

Uh oh!

jazairi commented Nov 13, 2025

Uh oh!

JPrevost left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jazairi commented Nov 12, 2025 •

edited by atlassian bot

Loading

coveralls commented Nov 12, 2025 •

edited

Loading