Conversation
❌ 1 blocking issue (1 total)
|
![qltysh[bot]](https://avatars.githubusercontent.com/in/890766?s=60&v=4){kind=small}
Coverage Report for CI Build 26600286704Coverage increased (+0.001%) to 98.318%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
There was a problem hiding this comment.
Pull request overview
Adds a new static “About Natural Language Search” page and wires it into the UI so users can learn what NLS is, when it applies, and its limitations.
Changes:
- Adds
/about-natural-language-searchroute, controller action, view, and a basic integration test. - Updates “Learn more” links in the search form and NLS warning callout to point to the new page.
- Adjusts index page title behavior and updates a TIMDEX source URL env var fetch behavior.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| test/helpers/application_helper_test.rb | Updates expectations for index page title format. |
| test/controllers/static_controller_test.rb | Adds request specs for static pages. |
| config/routes.rb | Adds route for the new about-NLS page. |
| app/views/static/about_natural_language_search.html.erb | New static informational page for NLS. |
| app/views/search/_nls_alert.html.erb | Updates “Learn more” link target in NLS warning. |
| app/views/search/_form.html.erb | Updates “Learn more” link target in NLS toggle area. |
| app/models/normalize_timdex_record.rb | Changes env var fetching for MIT Alma source URL construction. |
| app/helpers/application_helper.rb | Changes index page title formatting when PLATFORM_NAME is set. |
| app/controllers/static_controller.rb | Adds controller action for the new static page. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,44 @@ | |||
| <% content_for :page_title, "About Natural Language Search" %> | |||
Comment on lines
+4
to
+7
| <h2>Opting in to Natural Language Search</h2> | ||
|
|
||
| <h3>What does turning on the Natural Language Search (NLS) do?</h3> | ||
|
|
Member
Author
There was a problem hiding this comment.
FWIW this is incorrect. We have an H1 on the page and this starting at H2 is correct.
Comment on lines
+19
to
+43
| <p>In our experience, NLS works well for: | ||
| <ul> | ||
| <li>Natural language queries ("what causes ocean acidification?")</li> | ||
| <li>Concept-based searches rather than exact phrases</li> | ||
| <li>Exploratory research where related terms are valuable</li> | ||
| </ul> | ||
| </p> | ||
| <p>Traditional keyword searching works well for: | ||
| <ul> | ||
| <li>Specific item searches (i.e. pasting an article title or full citation into the search box)</li> | ||
| </ul> | ||
| </p> | ||
| <p>Do you agree? <a href="https://libraries.mit.edu/use-feedback">Let us know what you think.</a></p> | ||
|
|
||
| <h3>Are there any limitations I should be aware of?</h3> | ||
|
|
||
| <p>This search tool (the default on the MIT Libraries homepage) searches across many MIT Libraries catalogs, indexes, and content sources and not all of these can be searched with NLS. You'll still get results from all sources - but we will revert to traditional keyword matching when NLS is not possible.</p> | ||
|
|
||
| <p>That means if you have enabled NLS, this is what you'll get on each tab: | ||
| <ul> | ||
| <li>All tab (default): results listing "Articles, Books & More" as a source use traditional keyword matching, all other results use NLS</li> | ||
| <li>Articles/Books & Media tabs: all results use traditional keyword matching</li> | ||
| <li>All other tabs: all results use NLS</li> | ||
| </ul> | ||
| </p> |
Comment on lines
+19
to
+43
| <p>In our experience, NLS works well for: | ||
| <ul> | ||
| <li>Natural language queries ("what causes ocean acidification?")</li> | ||
| <li>Concept-based searches rather than exact phrases</li> | ||
| <li>Exploratory research where related terms are valuable</li> | ||
| </ul> | ||
| </p> | ||
| <p>Traditional keyword searching works well for: | ||
| <ul> | ||
| <li>Specific item searches (i.e. pasting an article title or full citation into the search box)</li> | ||
| </ul> | ||
| </p> | ||
| <p>Do you agree? <a href="https://libraries.mit.edu/use-feedback">Let us know what you think.</a></p> | ||
|
|
||
| <h3>Are there any limitations I should be aware of?</h3> | ||
|
|
||
| <p>This search tool (the default on the MIT Libraries homepage) searches across many MIT Libraries catalogs, indexes, and content sources and not all of these can be searched with NLS. You'll still get results from all sources - but we will revert to traditional keyword matching when NLS is not possible.</p> | ||
|
|
||
| <p>That means if you have enabled NLS, this is what you'll get on each tab: | ||
| <ul> | ||
| <li>All tab (default): results listing "Articles, Books & More" as a source use traditional keyword matching, all other results use NLS</li> | ||
| <li>Articles/Books & Media tabs: all results use traditional keyword matching</li> | ||
| <li>All other tabs: all results use NLS</li> | ||
| </ul> | ||
| </p> |
Comment on lines
+19
to
+43
| <p>In our experience, NLS works well for: | ||
| <ul> | ||
| <li>Natural language queries ("what causes ocean acidification?")</li> | ||
| <li>Concept-based searches rather than exact phrases</li> | ||
| <li>Exploratory research where related terms are valuable</li> | ||
| </ul> | ||
| </p> | ||
| <p>Traditional keyword searching works well for: | ||
| <ul> | ||
| <li>Specific item searches (i.e. pasting an article title or full citation into the search box)</li> | ||
| </ul> | ||
| </p> | ||
| <p>Do you agree? <a href="https://libraries.mit.edu/use-feedback">Let us know what you think.</a></p> | ||
|
|
||
| <h3>Are there any limitations I should be aware of?</h3> | ||
|
|
||
| <p>This search tool (the default on the MIT Libraries homepage) searches across many MIT Libraries catalogs, indexes, and content sources and not all of these can be searched with NLS. You'll still get results from all sources - but we will revert to traditional keyword matching when NLS is not possible.</p> | ||
|
|
||
| <p>That means if you have enabled NLS, this is what you'll get on each tab: | ||
| <ul> | ||
| <li>All tab (default): results listing "Articles, Books & More" as a source use traditional keyword matching, all other results use NLS</li> | ||
| <li>Articles/Books & Media tabs: all results use traditional keyword matching</li> | ||
| <li>All other tabs: all results use NLS</li> | ||
| </ul> | ||
| </p> |
| <aside class="nls-alert"> | ||
| <i class="fa-regular fa-circle-exclamation"></i> | ||
| <p>Natural language search is not available for these results<a href="#">Learn more</a></p> | ||
| <p>Natural language search is not available for these results<a href="/about-natural-language-search" data-turbo-frame="_top">Learn more</a></p> |
| <div class="semantic-search-toggle <%= @natural_language_search_optin ? 'toggled-on' : 'toggled-off' %>" data-controller="natural-language-search-toggle" data-matomo-seen="Semantic Search Toggle, Seen by user, Tab: {{getActiveTabName}}"> | ||
| <button type="button" aria-pressed="<%= @natural_language_search_optin %>" data-action="click->natural-language-search-toggle#toggle" data-matomo-click="Semantic Search Toggle, Toggle Clicked, Was: {{getToggleState}}">Natural language search</button> | ||
| <a href="#" data-matomo-click="Semantic Search Toggle, Learn more Clicked, Tab: {{getActiveTabName}}">Learn more</a> | ||
| <a href="/about-natural-language-search" data-matomo-click="Semantic Search Toggle, Learn more Clicked, Tab: {{getActiveTabName}}">Learn more</a> |
Comment on lines
13
to
15
| 'MIT Alma' => ['MIT Libraries Catalog', | ||
| "#{ENV.fetch('MIT_PRIMO_URL')}/discovery/search?vid=#{ENV.fetch('PRIMO_VID')}&lang=en"] | ||
| "#{ENV.fetch('MIT_PRIMO_URL', nil)}/discovery/search?vid=#{ENV.fetch('PRIMO_VID', nil)}&lang=en"] | ||
| }.freeze |
Member
Author
There was a problem hiding this comment.
This is a quick bug fix for geodata, a ticket to address it more robustly will be opened.
Comment on lines
16
to
18
| def index_page_title | ||
| ENV.fetch('PLATFORM_NAME', nil) ? "Search #{ENV.fetch('PLATFORM_NAME')} | MIT Libraries" : 'Search | MIT Libraries' | ||
| ENV.fetch('PLATFORM_NAME', nil) ? ENV.fetch('PLATFORM_NAME') : 'Search | MIT Libraries' | ||
| end |
Comment on lines
+5
to
+11
| get '/about-natural-language-search' | ||
|
|
||
| assert_response :success | ||
| end | ||
|
|
||
| test 'style guide page returns success' do | ||
| get '/style-guide' |
matt-bernhardt
approved these changes
May 28, 2026
matt-bernhardt
left a comment
matt-bernhardt
left a comment
Member
There was a problem hiding this comment.
I think there's an improper closing tag in the page content, but from looking in the inspector it appears that browsers are correcting for this appropriately. The W3C's validator does flag it as a broken closing tag, however.
All this said, if you want to merge I'm not going to block it.
The rest of the changes all look good. I've confirmed that running this in Geodata mode doesn't crash anymore - feeling guilty that I missed that the first time.
| <h3>How can I tell you what I think about NLS?</h3> | ||
|
|
||
| <p><a href="https://libraries.mit.edu/use-feedback">Send us your feedback!</a> Sharing your experiences (good and bad) will help us understand how to make these search options work better.</p> | ||
| <p>In our experience, NLS works well for:</> |
Member
There was a problem hiding this comment.
I think this is a broken closing tag? Should it be </p> ?
Member
Author
There was a problem hiding this comment.
Doh, will fix. Great catch and yeah modern browsers are super forgiving but we should still write valid code!
Why are these changes being introduced: * To provide users with information about the natural language search feature, how to use it, and limitations to be aware of. Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/USE-601 How does this address that need: * Creates static page with content
This is not necessarily the final version of this, but it addresses a current double "search" while we await final intent
I'll open a separate ticket to fix this better, but for now this should allow us to deploy geodata again.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why are these changes being introduced:
feature, how to use it, and limitations to be aware of.
Relevant ticket(s):
How does this address that need:
NOTE: a separate commit adjusting index page title is also included
NOTE: a separate bugfix for geodata introduced recently is also included.
Developer
Accessibility
New ENV
Approval beyond code review
Additional context needed to review
E.g., if the PR includes updated dependencies and/or data
migration, or how to confirm the feature is working.
Code Reviewer
Code
added technical debt.
Documentation
(not just this pull request message).
Testing