-
Notifications
You must be signed in to change notification settings - Fork 14
Stabilize Algolia search feedback #1069
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,7 @@ | |
|
|
||
| import { liteClient as algoliasearch } from "algoliasearch/lite"; | ||
| import { Search } from "lucide-react"; | ||
| import { useEffect, useState } from "react"; | ||
| import { useCallback, useEffect, useRef, useState } from "react"; | ||
| import { | ||
| Configure, | ||
| Highlight, | ||
|
|
@@ -41,6 +41,82 @@ const indexName = process.env.NEXT_PUBLIC_ALGOLIA_INDEX_NAME; | |
| const searchClient = | ||
| appId && searchKey ? algoliasearch(appId, searchKey) : null; | ||
|
|
||
| export const ALGOLIA_SEARCH_CONFIG = { | ||
| attributesToSnippet: ["content:20"], | ||
| distinct: true, | ||
| highlightPreTag: "__ais-highlight__", | ||
| highlightPostTag: "__/ais-highlight__", | ||
| hitsPerPage: 15, | ||
| snippetEllipsisText: "…", | ||
| }; | ||
| export const ALGOLIA_SEARCH_DEBOUNCE_MS = 150; | ||
|
|
||
| type SearchStatus = "idle" | "loading" | "stalled" | "error"; | ||
| type SearchTimer = ReturnType<typeof setTimeout>; | ||
|
|
||
| export function getSearchErrorMessage(status: SearchStatus): string | null { | ||
| return status === "error" | ||
| ? "Search failed. Check your connection and try again." | ||
| : null; | ||
| } | ||
|
|
||
| export function searchErrorIsCurrent( | ||
| query: string, | ||
| dispatchedQuery: string, | ||
| status: SearchStatus | ||
| ): boolean { | ||
| return ( | ||
| status === "error" && | ||
| query.trim().length > 0 && | ||
| dispatchedQuery.trim() === query.trim() | ||
| ); | ||
| } | ||
|
|
||
| type ScheduleSearchOptions = { | ||
| query: string; | ||
| search: (nextQuery: string) => void; | ||
| setTypedQuery: (nextQuery: string) => void; | ||
| setDispatchedQuery?: (nextQuery: string) => void; | ||
| currentTimer: SearchTimer | null; | ||
| delayMs?: number; | ||
| }; | ||
|
|
||
| export function scheduleSearch({ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Replacing with the existing hook would remove the manual timer management, eliminate the exported |
||
| query, | ||
| search, | ||
| setTypedQuery, | ||
| setDispatchedQuery, | ||
| currentTimer, | ||
| delayMs = ALGOLIA_SEARCH_DEBOUNCE_MS, | ||
| }: ScheduleSearchOptions): SearchTimer | null { | ||
| setTypedQuery(query); | ||
| if (currentTimer) { | ||
| clearTimeout(currentTimer); | ||
| } | ||
| if (!query.trim()) { | ||
| setDispatchedQuery?.(query); | ||
| search(query); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unnecessary Algolia request on every input clear The empty-query fast path calls |
||
| return null; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Whitespace skips clear searchLow Severity
Additional Locations (1)Reviewed by Cursor Bugbot for commit 5cf7f8b. Configure here. |
||
| } | ||
| return setTimeout(() => { | ||
| setDispatchedQuery?.(query); | ||
| search(query); | ||
| }, delayMs); | ||
| } | ||
|
|
||
| export function searchResultsAreCurrent( | ||
| query: string, | ||
| resultsQuery: string, | ||
| status: SearchStatus | ||
| ): boolean { | ||
| const normalizedQuery = query.trim(); | ||
| return ( | ||
| normalizedQuery.length > 0 && | ||
| status === "idle" && | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: React InstantSearch sets Suggested fix: return (
normalizedQuery.length > 0 &&
(status === "idle" || status === "stalled") &&
resultsQuery.trim() === normalizedQuery
); |
||
| resultsQuery.trim() === normalizedQuery | ||
| ); | ||
| } | ||
|
|
||
| function safeHref(url: string | undefined): string { | ||
| if (!url) { | ||
| return "/"; | ||
|
|
@@ -138,28 +214,117 @@ function SearchHit({ hit }: { hit: DocSearchRecord }) { | |
| ); | ||
| } | ||
|
|
||
| function EmptyQuery() { | ||
| const { indexUiState } = useInstantSearch(); | ||
| if (indexUiState.query) { | ||
| return null; | ||
| function SearchResults({ | ||
| query, | ||
| dispatchedQuery, | ||
| }: { | ||
| query: string; | ||
| dispatchedQuery: string; | ||
| }) { | ||
| const { results, status } = useInstantSearch({ catchError: true }); | ||
| if (!query.trim()) { | ||
| return ( | ||
| <p className="px-4 py-8 text-center text-sm text-muted-foreground"> | ||
| Start typing to search the docs… | ||
| </p> | ||
| ); | ||
| } | ||
|
|
||
| const errorMessage = searchErrorIsCurrent(query, dispatchedQuery, status) | ||
| ? getSearchErrorMessage(status) | ||
| : null; | ||
| if (errorMessage) { | ||
| return ( | ||
| <p | ||
| className="px-4 py-8 text-center text-sm text-muted-foreground" | ||
| role="alert" | ||
| > | ||
| {errorMessage} | ||
| </p> | ||
| ); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Stale error during debounced typingMedium Severity After a search fails, the Reviewed by Cursor Bugbot for commit 5cf7f8b. Configure here. |
||
| } | ||
|
|
||
| if ( | ||
| !(results && searchResultsAreCurrent(query, results.query ?? "", status)) | ||
| ) { | ||
| return ( | ||
| <p className="px-4 py-8 text-center text-sm text-muted-foreground"> | ||
| Searching… | ||
| </p> | ||
| ); | ||
| } | ||
|
|
||
| if (results.nbHits === 0) { | ||
| return ( | ||
| <p className="px-4 py-8 text-center text-sm text-muted-foreground"> | ||
| No results for{" "} | ||
| <strong className="text-foreground">"{results.query}"</strong> | ||
| </p> | ||
| ); | ||
| } | ||
|
|
||
| return ( | ||
| <p className="px-4 py-8 text-center text-sm text-muted-foreground"> | ||
| Start typing to search the docs… | ||
| </p> | ||
| <Hits | ||
| classNames={{ item: "", list: "space-y-0.5", root: "" }} | ||
| hitComponent={({ hit }) => ( | ||
| <SearchHit hit={hit as unknown as DocSearchRecord} /> | ||
| )} | ||
| /> | ||
| ); | ||
| } | ||
|
|
||
| function NoResults() { | ||
| const { results } = useInstantSearch(); | ||
| if (!results?.query || results.nbHits > 0) { | ||
| return null; | ||
| } | ||
| type SearchQueryHook = ( | ||
| query: string, | ||
| search: (nextQuery: string) => void | ||
| ) => void; | ||
|
|
||
| function SearchContent() { | ||
| const [typedQuery, setTypedQuery] = useState(""); | ||
| const [dispatchedQuery, setDispatchedQuery] = useState(""); | ||
| const searchTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null); | ||
| const queryHook = useCallback<SearchQueryHook>((query, search) => { | ||
| searchTimerRef.current = scheduleSearch({ | ||
| query, | ||
| search, | ||
| setTypedQuery, | ||
| setDispatchedQuery, | ||
| currentTimer: searchTimerRef.current, | ||
| }); | ||
| }, []); | ||
|
|
||
| useEffect( | ||
| () => () => { | ||
| if (searchTimerRef.current) { | ||
| clearTimeout(searchTimerRef.current); | ||
| } | ||
| }, | ||
| [] | ||
| ); | ||
|
|
||
| return ( | ||
| <p className="px-4 py-8 text-center text-sm text-muted-foreground"> | ||
| No results for{" "} | ||
| <strong className="text-foreground">"{results.query}"</strong> | ||
| </p> | ||
| <> | ||
| <Configure {...ALGOLIA_SEARCH_CONFIG} /> | ||
| <div className="flex items-center border-b border-border px-4"> | ||
| <Search className="size-4 shrink-0 text-muted-foreground" /> | ||
| <SearchBox | ||
| autoFocus | ||
| classNames={{ | ||
| form: "flex flex-1", | ||
| input: | ||
| "w-full bg-transparent px-3 py-4 text-sm text-foreground placeholder:text-muted-foreground outline-none", | ||
| loadingIndicator: "hidden", | ||
| reset: "hidden", | ||
| root: "flex-1", | ||
| submit: "hidden", | ||
| }} | ||
| placeholder="Search docs…" | ||
| queryHook={queryHook} | ||
| /> | ||
| </div> | ||
| <div className="max-h-[60vh] min-h-24 overflow-y-auto p-2"> | ||
| <SearchResults dispatchedQuery={dispatchedQuery} query={typedQuery} /> | ||
| </div> | ||
| </> | ||
| ); | ||
| } | ||
|
|
||
|
|
@@ -222,38 +387,7 @@ export function AlgoliaSearch() { | |
| <div className="relative z-10 w-full max-w-2xl overflow-hidden rounded-xl border border-border bg-popover shadow-2xl"> | ||
| {searchClient && indexName ? ( | ||
| <InstantSearch indexName={indexName} searchClient={searchClient}> | ||
| <Configure | ||
| attributesToSnippet={["content:20"]} | ||
| distinct={true} | ||
| hitsPerPage={15} | ||
| snippetEllipsisText="…" | ||
| /> | ||
| <div className="flex items-center border-b border-border px-4"> | ||
| <Search className="size-4 shrink-0 text-muted-foreground" /> | ||
| <SearchBox | ||
| autoFocus | ||
| classNames={{ | ||
| form: "flex flex-1", | ||
| input: | ||
| "w-full bg-transparent px-3 py-4 text-sm text-foreground placeholder:text-muted-foreground outline-none", | ||
| loadingIndicator: "hidden", | ||
| reset: "hidden", | ||
| root: "flex-1", | ||
| submit: "hidden", | ||
| }} | ||
| placeholder="Search docs…" | ||
| /> | ||
| </div> | ||
| <div className="max-h-[60vh] overflow-y-auto p-2"> | ||
| <EmptyQuery /> | ||
| <NoResults /> | ||
| <Hits | ||
| classNames={{ item: "", list: "space-y-0.5", root: "" }} | ||
| hitComponent={({ hit }) => ( | ||
| <SearchHit hit={hit as unknown as DocSearchRecord} /> | ||
| )} | ||
| /> | ||
| </div> | ||
| <SearchContent /> | ||
| </InstantSearch> | ||
| ) : ( | ||
| <SearchUnavailable /> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| import { afterEach, describe, expect, test, vi } from "vitest"; | ||
| import { | ||
| ALGOLIA_SEARCH_CONFIG, | ||
| ALGOLIA_SEARCH_DEBOUNCE_MS, | ||
| getSearchErrorMessage, | ||
| scheduleSearch, | ||
| searchErrorIsCurrent, | ||
| searchResultsAreCurrent, | ||
| } from "../app/_components/algolia-search"; | ||
|
|
||
| describe("Algolia search configuration", () => { | ||
| afterEach(() => { | ||
| vi.useRealTimers(); | ||
| }); | ||
|
|
||
| test("uses the highlight markers expected by React InstantSearch", () => { | ||
| expect(ALGOLIA_SEARCH_CONFIG.highlightPreTag).toBe("__ais-highlight__"); | ||
| expect(ALGOLIA_SEARCH_CONFIG.highlightPostTag).toBe("__/ais-highlight__"); | ||
| }); | ||
|
|
||
| test("debounces search requests while typing", () => { | ||
| vi.useFakeTimers(); | ||
| const search = vi.fn(); | ||
| const setDispatchedQuery = vi.fn(); | ||
| const setTypedQuery = vi.fn(); | ||
| let timer = scheduleSearch({ | ||
| query: "g", | ||
| search, | ||
| setDispatchedQuery, | ||
| setTypedQuery, | ||
| currentTimer: null, | ||
| }); | ||
| timer = scheduleSearch({ | ||
| query: "github", | ||
| search, | ||
| setDispatchedQuery, | ||
| setTypedQuery, | ||
| currentTimer: timer, | ||
| }); | ||
|
|
||
| expect(setTypedQuery).toHaveBeenLastCalledWith("github"); | ||
| expect(search).not.toHaveBeenCalled(); | ||
| vi.advanceTimersByTime(ALGOLIA_SEARCH_DEBOUNCE_MS); | ||
| expect(search).toHaveBeenCalledOnce(); | ||
| expect(search).toHaveBeenCalledWith("github"); | ||
| expect(setDispatchedQuery).toHaveBeenCalledWith("github"); | ||
|
|
||
| scheduleSearch({ | ||
| query: "", | ||
| search, | ||
| setDispatchedQuery, | ||
| setTypedQuery, | ||
| currentTimer: timer, | ||
| }); | ||
| expect(search).toHaveBeenLastCalledWith(""); | ||
| }); | ||
|
|
||
| test("does not render stale results while a new query is pending", () => { | ||
| expect(searchResultsAreCurrent("slack", "github", "idle")).toBe(false); | ||
| expect(searchResultsAreCurrent("slack", "slack", "loading")).toBe(false); | ||
| expect(searchResultsAreCurrent("slack", "slack", "stalled")).toBe(false); | ||
| expect(searchResultsAreCurrent("slack", "slack", "idle")).toBe(true); | ||
| }); | ||
|
|
||
| test("surfaces failed searches instead of leaving a loading state", () => { | ||
| expect(getSearchErrorMessage("error")).toBe( | ||
| "Search failed. Check your connection and try again." | ||
| ); | ||
| expect(getSearchErrorMessage("loading")).toBeNull(); | ||
| }); | ||
|
|
||
| test("does not show an old error for a newly typed query", () => { | ||
| expect(searchErrorIsCurrent("slack", "github", "error")).toBe(false); | ||
| expect(searchErrorIsCurrent("slack", "slack", "error")).toBe(true); | ||
| }); | ||
| }); |


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Misleading optional on a consequence-bearing parameter
setDispatchedQueryis typed as optional (?), so external callers of the exportedscheduleSearchcan legally omit it. When omitted,dispatchedQuerystays at"", makingsearchErrorIsCurrentalways returnfalse— Algolia errors are silently swallowed and the UI shows an indefinite loading state.Since this parameter is mandatory for correct error display, consider removing the
?or adding a JSDoc warning.