feat: improve unified search content type results #2010
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Some suggested prefixes and emojis that may help to write clear, actionable code review comments:
Expand for comment prefix descriptions
|
📦 Next.js Bundle AnalysisThis analysis was generated by the next.js bundle analysis action 🤖 This PR introduced no changes to the javascript bundle 🙌 |
config/preview.json
Outdated
| @@ -4,6 +4,6 @@ | |||
| "max_static_paths": 10 | |||
| }, | |||
| "flags": { | |||
| "enable_unified_search": false | |||
| "enable_unified_search": true | |||
There was a problem hiding this comment.
🚧 Note: will set flags back to false before merging.
| * TODO: for non-specific `resultType`, filter to only show `integration` | ||
| * results for products with integrations config flags on. | ||
| */ | ||
| export function getAlgoliaFilters( |
There was a problem hiding this comment.
This function replaces get-algolia-product-filter-string. With separate content-type-specific search queries now running for each tab, I've expanded this function slightly to handle an optional contentType argument.
I've also expanded a bit on a TODO here related to the "integrations shown for a product that still have integrations flag off" issue identified in dev-portal#1986. Intent is to resolve that issue in a separate upcoming PR, which would be based off of the work in this PR.
| @@ -22,7 +29,7 @@ import { useSetUpAndCleanUpCommandState } from 'components/command-bar/hooks' | |||
| * when in that product context, Would typing-based filters, such as | |||
| * `product:<productSlug>`, be preferable to the "product tag" approach? | |||
| */ | |||
| export function useCommandBarProductTag() { | |||
| export function useCommandBarProductTag(): CommandBarProductTag | null { | |||
There was a problem hiding this comment.
Cleaned up the return type here, bit of a "side quest", but was adjacent to some other work in this PR.
| /** | ||
| * Render unified search results for the provided query input. | ||
| */ | ||
| function SearchResults({ |
There was a problem hiding this comment.
This file is where the bulk of the refactor occurred.
I've taken an approach that is in some ways similar to the existing separate-index tabs. This allows four separate searches to happen (one for each content type, plus on "global" without a content type filter), each returning filtered results up to the index's maximum single-page count.
The intent here is to thoroughly resolve the Skewed ranking on low-specificity queries issue mentioned at the end of the PR description of dev-portal#1986. That issue came up when executing a single search for the "all" tab, and then filtering down results client-side to populate every other tab.
Why this feels preferable
Apart from slightly worse performance (four queries instead of one), the only downside to this revised approach is that the results in the "All" tab are no longer simply the sum of the results in all other tabs. With this in mind, the count on the "All" tab has been removed.
These relatively minor compromises feel well worth avoiding the issue of relevant results for generic queries being lost to the abyss of skewed ranking criteria across content types.
Note: the "All" tab count removal, and this revised "separate search query per tab" approach was discussed with design last Friday, so I think we're good to go here on that front 🙇♂️
| * Transform unified search results into tab data that the | ||
| * `UnifiedHitsContainer` component can render. | ||
| */ | ||
| export function gatherSearchTabsData( |
There was a problem hiding this comment.
Slight refactor here to match some keys to make things easier, and to allow gather-search-tabs-content to accept a unified search results object rather than a single array of raw hits.
| * | ||
| * Note: this component needs to be used within an `InstantSearch` container | ||
| * imported from 'react-instantsearch-hooks-web'. That container provides | ||
| * the context from which `rawHits` are pulled. | ||
| */ | ||
| export function UnifiedHitsContainer({ |
There was a problem hiding this comment.
With tabsData logic now outside of this component, it's become much more purely presentational, taking the prepped tabsData and rendering the tabbed search results.
| <TabHeadingWithCount heading={heading} count={hitCount} /> | ||
| <TabHeadingWithCount | ||
| heading={heading} | ||
| count={type === 'global' ? undefined : hitCount} | ||
| /> |
There was a problem hiding this comment.
We hide the tab count for the global tab - it doesn't "add up" with the other tabs, so felt more confusing than helpful to include it.
| import type { Hit } from 'instantsearch.js' | ||
|
|
||
| // TODO: set up an enum for this | ||
| export type UnifiedSearchableContentType = |
There was a problem hiding this comment.
Added this types file to hold some types I found myself re-using in a couple spots.
There was a problem hiding this comment.
I think this is a totally reasonable approach. I'm not too concerned about performance RE: the 4 queries.
It looks like we already were managing these 4 indexes, and we didn't need to spin up any new indexes right? Just asking to ensure I shouldn't be looking anywhere else!
|
@BrandonRomano Re: the 4 queries, they're 4 queries all on the same unified index! So no new indexes needed 👍 As an adjacent note: in theory, and hopefully in practice in the near future, we should be able to transition |
🔗 Relevant links
🗒️ What
This PR refactors the "unified" search result tabs to ensure that relevant results are shown for all content types.
Why
Upstream, we execute a single search query against Algolia for the "all" tab, and then use client-side logic to split "all" results into separate content type tabs.
While efficient, and perhaps intuitive-feeling since the "all" tab truly represents the sum of results in all other tabs, this approach comes with a very bad downside. For generic search terms that return many results, the split of results across content-types is difficult if not impossible to control.
For context, for performance and experience reasons, we do not seem to want "exhaustive" search. Rather than return every record that matches a given query, we return only the most relevant records up to some limit we set in Algolia configuration.
As a concrete example, imagine the limit is
100record. The queryvaultwill return a full page of results in the "all" tab, so100results. But when split by content type, there is no reason to expect~ 33results in each of the separate content tabs. Since results are ranked by relevance, regardless of content type, some content types by nature of their authoring practices and metadata structure will "clobber" results from other content types for particular queries. Here are two screenshots of this exact behaviour happening:vaultupstreamwaypointupstream🛠️ How
In this PR, instead of executing a single query to Algolia, we execute four separate queries - one for the "all" tab, and one for each of the three separate content types.
This ensures that if there are any remotely relevant results in a particular content type, they'll show up in that content type tab.
📸 Design Screenshots
vaultthis PRwaypointthis PR🧪 Testing
vaultandwaypointshould return a relatively balanced number of results across content type tabs💭 Anything else?
Not at the moment!