SCIX-867 feat: add SearchModifierBar with ADS Compatibility mode#866
Open
thostetler wants to merge 3 commits into
Open
SCIX-867 feat: add SearchModifierBar with ADS Compatibility mode#866thostetler wants to merge 3 commits into
thostetler wants to merge 3 commits into
Conversation
thostetler
force-pushed
the
worktree-search-mode-bar
branch
from
2c52eba to
5692120
Compare
thostetler
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #866 +/- ##
========================================
+ Coverage 62.7% 62.8% +0.2%
========================================
Files 323 327 +4
Lines 38053 38306 +253
Branches 1721 1761 +40
========================================
+ Hits 23824 24045 +221
- Misses 14189 14216 +27
- Partials 40 45 +5
🚀 New features to boost your workflow:
|
thostetler
force-pushed
the
worktree-search-mode-bar
branch
from
5692120 to
dd3966a
Compare
The app had no way for users to scope searches to ADS-style astronomy and physics content with date-sorted results. ADS users arriving via referrer also had no awareness that their experience was configured differently. - Add SearchModifierBar dropdown (Default / ADS Compatibility mode) - Persist mode selection in Zustand store, seeded from scix_prefs cookie - Apply fq_database filter and date sort when ADS Compatibility mode is active - Auto-enable ADS Compatibility mode on ADS referrer redirect; show toast - Place SearchModifierBar on landing page, search results, abstract pages, and classic form - Disable dropdown during active search loading - Immediate mode switch on search results page via onNavigate prop - Strip mode from Solr query key to prevent double-fetch on AppMode sync - Add dev-only per-page options (1, 3) to config
Four new test files (SearchModifierBar, useSearchMode, prefs-cookie, searchMode) were missing explicit `test` imports from vitest (globals are disabled in this project). SearchBar suite had five tests timing out due to a Framer Motion rAF cascade inside vi.advanceTimersByTimeAsync under fake timers. Fixed by excluding requestAnimationFrame from the fake timer scope, using fireEvent.change instead of user.type for long strings with special characters, shortening the arrow-down test input, removing unnecessary timer advancement after synchronous dispatch actions, and adding a 15s timeout on the Escape-key test which accumulates suite overhead.
thostetler
force-pushed
the
worktree-search-mode-bar
branch
from
dd3966a to
d8f7067
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces an “ADS Compatibility mode” search option intended to scope searches to ADS-like astronomy/physics content (with date sorting) and persist that preference via Zustand + a scix_prefs cookie seeded by middleware/SSR.
Changes:
- Adds a
SearchModemodel (ADS_COMPATvsALL_RELEVANT) with helpers to apply/strip ADS defaults and to build outgoing search params. - Introduces
scix_prefscookie read/write utilities and SSR seeding ofmode/searchModefrom that cookie. - Adds the
SearchModifierBardropdown component and wires parts of the app to usebuildSearchOutgoingwhen navigating to/search.
Risk summary
High regression risk due to (1) cookie format/encoding inconsistencies, (2) incomplete/mismatched mode enabling behavior for ADS referrers, and (3) adding _app.getInitialProps (which changes Next.js rendering/optimization behavior app-wide).
Findings (priority order)
blocker
- Cookie written by middleware is raw JSON and not safely encoded, while readers assume URL-encoded content; this can break parsing and preference seeding.
- Legacy ADS referrer flow does not actually enable ADS Compatibility mode (only sets
mode), yet the UI claims it did; behavior and messaging diverge. - Classic form unconditionally sets
ads_compat=1and still doesn’t apply the ADS defaults consistently, making the param misleading and behavior incorrect. - Search results page does not render the SearchModifierBar despite PR description (and there’s no implemented “immediate switch” behavior there).
high
_app.getInitialPropsis introduced solely to pass cookies into Chakra’scookieStorageManagerSSR, which opts the entire app out of automatic static optimization and can have significant performance/operational impact.
medium
- Search mode UI injects a zero-width space into the displayed label to satisfy tests; this is brittle for UX/accessibility/i18n and should be solved in tests instead.
applySearchModeDefaultsstrips ADS database filters when leaving ADS mode but does not revert the ADS-impliedsort=date desc, so “All relevant content” can remain date-sorted.
low
readPrefsCookie’s regex requires"; "(semicolon + space) which is not guaranteed in realCookieheaders, so valid cookies may be missed.
Reviewed changes
Copilot reviewed 25 out of 26 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| tsconfig.json | Switches TS module resolution to bundler. |
| src/utils/common/searchMode.ts | Adds SearchMode enum/options + helpers to apply/strip ADS defaults and build outgoing params. |
| src/utils/common/prefs-cookie.ts | Adds scix_prefs cookie read/write helpers. |
| src/utils/common/tests/searchMode.test.ts | Unit tests for ADS defaulting/stripping behavior. |
| src/utils/common/tests/prefs-cookie.test.ts | Unit tests for cookie parsing/writing behavior. |
| src/store/store.ts | Adjusts SSR hydration behavior to avoid overriding user searchMode and to sync mode earlier. |
| src/store/slices/appMode.ts | Adds searchMode + actions; writes prefs cookie on updates. |
| src/ssr-utils.ts | Seeds mode/searchMode from prefs cookie with validation and precedence rules. |
| src/tests/ssr-utils.test.ts | Updates SSR tests for cookie-based mode/searchMode seeding and precedence. |
| src/lib/useSearchMode.ts | Adds a small Zustand-backed hook for reading/updating searchMode. |
| src/lib/tests/useSearchMode.test.ts | Hook unit tests. |
| src/middleware.ts | Sets scix_prefs cookie on discipline redirects and ADS referrer redirects (currently incomplete). |
| src/pages/index.tsx | Adds landing-page SearchModifierBar + toast for ADS referrer + outgoing query building. |
| src/pages/search/index.tsx | Syncs searchMode from URL param and strips it from query key; uses buildSearchOutgoing on submit. |
| src/components/SearchModifierBar/* | New dropdown component + tests. |
| src/components/SearchQueryLink/SearchQueryLink.tsx | Ensures links to /search include ADS mode defaults when active. |
| src/components/AbstractSearchForm/AbstractSearchForm.tsx | Uses buildSearchOutgoing when submitting from abstract pages. |
| src/components/ClassicForm/ClassicForm.tsx | Forces ads_compat=1 on submit (currently incorrect behavior). |
| src/pages/_app.tsx | Adds cookies to pageProps via _app.getInitialProps. |
| src/providers.tsx / src/pages/_document.tsx | Wires Chakra cookie-based color mode SSR. |
| src/config.ts | Adds dev-only small per-page options. |
| src/components/SearchBar/tests/SearchBar.test.tsx | Test timing adjustments to reduce runtime flakiness. |
| src/types.ts | Removes trailing whitespace line. |
Comments suppressed due to low confidence (1)
src/components/AbstractSearchForm/AbstractSearchForm.tsx:71
- PR description says the SearchModifierBar dropdown is shown on abstract pages, but
AbstractSearchFormcurrently renders onlySearchBarand doesn’t include the modifier UI. If abstract pages should allow changing search mode, addSearchModifierBarhere (and consider disabling it during submit/loading, consistent with the landing page behavior).
return (
<form method="get" action="/search" onSubmit={handleOnSubmit}>
<SearchBar query={query} showBackLinkAs="results" />
</form>
Comment on lines
+317
to
+324
| const merged = { ...existing, ...updates }; | ||
| Object.keys(merged).forEach((k) => merged[k] === undefined && delete merged[k]); | ||
| response.cookies.set('scix_prefs', JSON.stringify(merged), { | ||
| maxAge: PREFS_COOKIE_MAX_AGE, | ||
| path: '/', | ||
| sameSite: 'lax', | ||
| secure: process.env.NODE_ENV === 'production', | ||
| }); |
| log.info({ referer, duration: Date.now() - startTime }, 'Legacy ADS referrer redirect'); | ||
| return NextResponse.redirect(url); | ||
| const response = NextResponse.redirect(url); | ||
| setPrefsCookie(response, req, { mode: 'ASTROPHYSICS' }); |
Comment on lines
+78
to
+92
| // Show toast when middleware has auto-set ADS_COMPAT (cookie/GSSP path) | ||
| useEffect(() => { | ||
| if (router.query.fromADS !== 'true') { | ||
| return; | ||
| } | ||
| toast({ | ||
| status: 'info', | ||
| duration: 10000, | ||
| isClosable: true, | ||
| position: 'top', | ||
| title: 'ADS Compatibility mode enabled', | ||
| description: | ||
| "Looks like you came from ADS — we've switched to ADS Compatibility mode automatically. You can change this using the Search mode menu.", | ||
| }); | ||
| }, [router.query.fromADS, toast]); |
Comment on lines
197
to
+203
| <Box my={2}> | ||
| <SearchBar isLoading={isLoading} query={query} queryAddition={queryAddition} /> | ||
| {mode === AppMode.ASTROPHYSICS && ( | ||
| <Flex direction="row" mt={1}> | ||
| <SearchModifierBar onModeChange={(m) => setSearchMode(m)} ml="auto" /> | ||
| </Flex> | ||
| )} |
| const search = getSearchQuery(params, { mode }); | ||
| void router.push({ pathname: '/search', search }); | ||
| const urlParams = new URLSearchParams(search.startsWith('?') ? search.slice(1) : search); | ||
| urlParams.set(ADS_COMPAT_URL_PARAM, '1'); |
Comment on lines
+28
to
+33
| const updatedQuery = | ||
| newMode === SearchMode.ADS_COMPAT | ||
| ? { ...router.query, [ADS_COMPAT_URL_PARAM]: '1' } | ||
| : omit([ADS_COMPAT_URL_PARAM], router.query); | ||
| void router.push({ pathname: router.pathname, query: updatedQuery }, undefined, { shallow: true }); | ||
| } |
Comment on lines
+161
to
+170
| const [searchMode, setSearchMode] = useSearchMode(); | ||
|
|
||
| // Sync searchMode from URL ads_compat param — '1' means ADS_COMPAT, absent means skip. | ||
| const urlAdsCompat = (parsedParams as Record<string, unknown>)[ADS_COMPAT_URL_PARAM] as string | undefined; | ||
| useEffect(() => { | ||
| if (urlAdsCompat !== undefined) { | ||
| setSearchMode(urlAdsCompat === '1' ? SearchMode.ADS_COMPAT : SearchMode.ALL_RELEVANT); | ||
| } | ||
| }, [urlAdsCompat, setSearchMode]); | ||
|
|
Comment on lines
+84
to
+93
| NectarApp.getInitialProps = async (appContext: AppContext) => { | ||
| const appProps = await App.getInitialProps(appContext); | ||
| return { | ||
| ...appProps, | ||
| pageProps: { | ||
| ...appProps.pageProps, | ||
| cookies: appContext.ctx.req?.headers.cookie ?? '', | ||
| }, | ||
| }; | ||
| }; |
Comment on lines
+6
to
+13
| const COOKIE_NAME = 'scix_prefs'; | ||
| const MAX_AGE = 60 * 60 * 24 * 365; | ||
| const COOKIE_RE = new RegExp(`(?:^|; )${COOKIE_NAME}=([^;]*)`); | ||
|
|
||
| export const readPrefsCookie = (cookieSource?: string): SciXPrefs => { | ||
| const raw = cookieSource ?? (typeof document !== 'undefined' ? document.cookie : ''); | ||
| const match = raw.match(COOKIE_RE); | ||
| if (!match) { |
Comment on lines
+36
to
+53
| export const applySearchModeDefaults = (query: IADSApiSearchParams, mode: string | undefined): IADSApiSearchParams => { | ||
| if (mode === SearchMode.ADS_COMPAT) { | ||
| const withCollections = applyFiltersToQuery({ | ||
| query, | ||
| values: ['astronomy', 'physics'], | ||
| field: 'database', | ||
| logic: 'or', | ||
| }) as IADSApiSearchParams; | ||
| return { ...withCollections, sort: ADS_COMPAT_SORT }; | ||
| } | ||
|
|
||
| // Strip ADS-implied database filter if present and exactly matching ADS defaults. | ||
| // Only strip the exact values we would have set — leave user-configured filters alone. | ||
| if (query.fq_database === ADS_COMPAT_FQ_DATABASE) { | ||
| const fqWithout = (query.fq as string[] | undefined)?.filter((f) => f !== ADS_COMPAT_FQ_ENTRY) ?? []; | ||
| const withoutDb = omit(['fq_database'], query) as IADSApiSearchParams; | ||
| return fqWithout.length > 0 ? { ...withoutDb, fq: fqWithout } : (omit(['fq'], withoutDb) as IADSApiSearchParams); | ||
| } |
- middleware: seed searchMode: 'ADS_COMPAT' alongside mode: 'ASTROPHYSICS'
in the ADS referrer redirect cookie so the compatibility mode is actually
activated end-to-end (the toast on the landing page was accurate but the
store was never hydrated with the correct searchMode)
- searchMode: applySearchModeDefaults now also strips sort when leaving
ADS_COMPAT if it exactly matches the ADS default ('date desc'), preventing
results from remaining date-sorted after switching back to All relevant content
- SearchModifierBar: remove zero-width-space hack from the button label;
update tests to use getByRole('menuitem', { name: /.../ }) so queries are
unambiguous without leaking implementation details into production markup
- search/index: render SearchModifierBar on the results page with an
onNavigate handler that rebuilds the current query via buildSearchOutgoing
(adding/stripping ADS defaults) and pushes the new URL
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.
The app had no way for users to scope searches to ADS-style astronomy and physics content with date-sorted results. ADS users arriving via referrer also had no awareness that their experience was configured differently.
see https://adsabs.atlassian.net/browse/SCIX-867?search_id=b3c71d5d-2488-4a39-89d4-ee3ace7819d4 for screenshots and further description