Skip to content

SCIX-893 feat(search): add title quickfield and all-terms modal - #908

Open
thostetler wants to merge 2 commits into
adsabs:masterfrom
thostetler:feature/scix-893-quickfields-rework
Open

SCIX-893 feat(search): add title quickfield and all-terms modal#908
thostetler wants to merge 2 commits into
adsabs:masterfrom
thostetler:feature/scix-893-quickfields-rework

Conversation

@thostetler

@thostetler thostetler commented Jul 30, 2026

Copy link
Copy Markdown
Member

The quickfields row lacked a title field, and its all-terms combobox used a cramped 200px input with a popper tooltip that was itself hidden below md.

  • Add title as the trailing quickfield in both default and biology/physical-science modes
  • Replace the combobox with an icon-button trigger that opens a command-palette modal: filter, grouped browse list, keyboard nav, and a detail pane with sanitized description, syntax, and examples
  • Reveal quickfields by breakpoint so trailing fields drop first on small screens (e.g. author and first author stay, the rest appear as the row widens); the trigger label collapses to an icon there too
  • Derive the quickfields from the shared term list so field metadata has a single source of truth
  • Preserve the quick-fields and all-search-terms tour hooks

Ticket: SCIX-893 and SCIX-894.
demo: Kooha-2026-07-30-16-29-43.webm

@thostetler
thostetler requested a review from shinyichen July 30, 2026 20:07
@codecov

codecov Bot commented Jul 30, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.50720% with 26 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.9%. Comparing base (3d29af9) to head (c62e2b0).

Files with missing lines Patch % Lines
src/components/SearchBar/AllSearchTermsModal.tsx 92.6% 22 Missing ⚠️
src/components/SearchBar/models.ts 85.8% 3 Missing ⚠️
src/components/SearchBar/QuickFields.tsx 96.5% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##           master    #908     +/-   ##
========================================
+ Coverage    67.9%   67.9%   +0.1%     
========================================
  Files         356     355      -1     
  Lines       41871   41749    -122     
  Branches     2193    2199      +6     
========================================
- Hits        28404   28344     -60     
+ Misses      13418   13358     -60     
+ Partials       49      47      -2     
Files with missing lines Coverage Δ
src/components/SearchBar/index.ts 100.0% <100.0%> (ø)
src/components/SearchBar/QuickFields.tsx 96.9% <96.5%> (-0.4%) ⬇️
src/components/SearchBar/models.ts 99.6% <85.8%> (-0.4%) ⬇️
src/components/SearchBar/AllSearchTermsModal.tsx 92.6% <92.6%> (ø)

... and 5 files with indirect coverage changes

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@thostetler
thostetler force-pushed the feature/scix-893-quickfields-rework branch from 7531c0a to d286395 Compare July 30, 2026 20:20
@thostetler thostetler changed the title SCIX-893 feat(search): rework quickfields row and all-terms modal SCIX-893 feat(search): add title quickfield and all-terms modal Jul 30, 2026
@thostetler
thostetler force-pushed the feature/scix-893-quickfields-rework branch from d286395 to fa107dc Compare July 30, 2026 20:25
@thostetler
thostetler marked this pull request as ready for review July 30, 2026 20:35
Copilot AI review requested due to automatic review settings July 30, 2026 20:35

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Risk summary

Moderate risk. The UI refactor is contained to the SearchBar quickfields/all-terms picker, but it introduces (1) a likely failing unit test due to responsive display behavior in jsdom, and (2) an accessibility regression vs the previous Downshift combobox unless ARIA listbox/option semantics are added.

Changes:

  • Add title as a quickfield and derive quickfields from allSearchTerms by id (single source of truth).
  • Replace the all-terms combobox dropdown with a modal-based command-palette (filter, keyboard nav, detail pane with sanitized HTML).
  • Add breakpoint-based quickfield reveal (trailing fields hidden first on small screens) and update tests accordingly.

Findings (priority order)

blocker

  • Unit test clicks a quickfield that is intentionally hidden at the base breakpoint (likely non-interactable in jsdom), causing user.click to fail.
    • Location: src/components/SearchBar/__tests__/SearchBar.test.tsx (new “title quickfield…” test)

high

  • Accessibility regression: the modal list and rows need listbox/option semantics (and active-descendant wiring) to support assistive tech comparable to the previous Downshift implementation.
    • Location: src/components/SearchBar/AllSearchTermsModal.tsx

medium

  • Potential O(n²) render cost from items.indexOf(option) inside options.map over a large allSearchTerms list.
    • Location: src/components/SearchBar/AllSearchTermsModal.tsx

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/components/SearchBar/QuickFields.tsx Adds title quickfield, derives items per mode, and applies breakpoint-based display to trailing quickfields.
src/components/SearchBar/models.ts Derives quickfields from allSearchTerms by id to keep metadata in one place; adds title to quickfield lists.
src/components/SearchBar/index.ts Re-exports the new modal component instead of the removed dropdown.
src/components/SearchBar/AllSearchTermsModal.tsx New modal-based all-terms picker with filtering, keyboard navigation, and a sanitized detail pane.
src/components/SearchBar/AllSearchTermsDropdown.tsx Removes the old combobox dropdown implementation.
src/components/SearchBar/tests/SearchBar.test.tsx Updates tests for the modal and adds coverage for title presence/insertion behavior.
Comments suppressed due to low confidence (2)

src/components/SearchBar/AllSearchTermsModal.tsx:121

  • The menu container is missing list semantics, which makes the modal harder to use with assistive tech compared to the previous Downshift combobox. Adding role="listbox" (and a stable id for aria-controls) improves screen reader support and aligns with the keyboard navigation behavior implemented in the input.
            <Box maxH="360px" overflowY="auto" data-testid="allSearchTermsMenu">

src/components/SearchBar/AllSearchTermsModal.tsx:145

  • Each clickable row is rendered as a plain
    with mouse handlers but without option semantics. Adding role="option"/aria-selected (and a stable id) improves accessibility and allows aria-activedescendant on the input to reference the active row.
                  <Flex
                    key={option.id}
                    ref={isActive ? activeRowRef : undefined}
                    px={4}
                    py={2}

Comment thread src/components/SearchBar/__tests__/SearchBar.test.tsx
Comment thread src/components/SearchBar/AllSearchTermsModal.tsx
Comment thread src/components/SearchBar/AllSearchTermsModal.tsx Outdated

@shinyichen shinyichen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I just want to point out that descriptions contain links, and with a mouse it is very hard to get to the link. If I move my mouse down towards the description area, the description would have moved to another term. I could move my mouse outside the modal, than down than towards the description, but we shouldn't expect users to do that. I wonder if the description can be moved to the right side to resolve that problem.

Screen.Recording.2026-08-03.at.2.24.25.PM.mov

@thostetler

Copy link
Copy Markdown
Member Author

@shinyichen good point, let me think about how to handle links here

The quickfields row lacked a title field, and its all-terms combobox used a
cramped 200px input with a popper tooltip that was itself hidden below md.

- Add title as the trailing quickfield in default and BIO_PHYSICAL_SCIENCE
- Derive quickfields from allSearchTerms by id to drop the duplicated copies
- Replace the combobox with an icon-button trigger opening a command-palette
  modal (matchSorter filter, grouped browse list, keyboard nav, detail pane
  with sanitized description plus full syntax and examples)
- Reveal quickfields by breakpoint so trailing fields drop first on small
  screens; the trigger label collapses to icon-only there too
- Keep the data-tour quick-fields and all-search-terms hooks intact
- Fix latent quickfields type to require only default and allow per-mode
  overrides
Address PR review feedback on the all-search-terms modal.

- Move detail pane beside the list (stacks on mobile) so description
  links are reachable without hover-selection swapping the term
- Fix rel/target on description links (ref typo) and force safe
  new-tab anchors via a scoped DOMPurify hook
- Add listbox/option roles, stable ids, and aria-activedescendant
- Precompute id->index map to drop O(n^2) row indexing
- Widen modal and pin columns so long syntax tokens do not shrink
  the list
@thostetler
thostetler force-pushed the feature/scix-893-quickfields-rework branch from 47c8f2b to c62e2b0 Compare August 5, 2026 15:38
@thostetler

Copy link
Copy Markdown
Member Author

Kooha-2026-08-05-08-36-07.webm
Shifted it to the right like you mentioned, I think this two-column setup is a little better. It should shift to be one-column on mobile

@thostetler

Copy link
Copy Markdown
Member Author

@shinyichen if you wouldn't mind, please check this out one more time. Thanks!

@shinyichen
shinyichen self-requested a review August 6, 2026 17:49

@shinyichen shinyichen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks awesome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants