[CDX-310]: added recent searches to autocomplete dropdown#267
[CDX-310]: added recent searches to autocomplete dropdown#267
Conversation
Code Review Results✅ StrengthsThe feature is well-structured following established patterns in the codebase — new type guards, a dedicated hook, and proper integration into the section pipeline are clean and consistent. 🚨 Critical Issues
|
There was a problem hiding this comment.
Pull request overview
Adds a new recentSearches section type to the autocomplete system so that previously-submitted queries can be displayed in the dropdown (including section rendering + styling), and wires selection of a recent search to submit tracking.
Changes:
- Introduces
recentSearchesas a first-class section type (types, guards, section result aggregation, rendering behavior). - Adds a new hook (
useGetRecentSearches) to pull recent searches from storage and format them as dropdown items. - Adds styling and selection handling so recent-search items render appropriately and trigger
trackSearchSubmiton click.
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/tracking.ts | Types term for trackSearchSubmit; continues persisting recent searches on submit. |
| src/utils/helpers.ts | Allows getActiveSectionsWithData to source data for recentSearches sections by displayName. |
| src/utils/beaconUtils.ts | Adds typings for recent-search helpers (getRecentSearches, storeRecentSearch). |
| src/types.ts | Extends section/type model to include recentSearches and defines RecentSearches shape. |
| src/typeGuards.ts | Adds isRecentSearchesSection guard. |
| src/styles.css | Adds styling for recent-searches section items; minor formatting cleanup. |
| src/hooks/useSections/useSectionsResults.ts | Fetches recent-searches “results” via the new hook and returns them alongside other section results. |
| src/hooks/useSections/index.ts | Merges recent-searches results into the combined sectionsResults. |
| src/hooks/useGetRecentSearches.ts | New hook: reads recent searches from storage and maps to dropdown item shape. |
| src/hooks/useDownShift.ts | Handles selection of recent-search items by setting query + firing trackSearchSubmit. |
| src/hooks/useCioAutocomplete.ts | Ensures section titles / section class naming logic supports recentSearches. |
| src/components/Autocomplete/SectionItemsList/SectionItemsList.tsx | Adds recentSearches handling for section titles. |
| src/components/Autocomplete/AutocompleteResults/AutocompleteResults.tsx | Uses a stable-ish key for recentSearches sections. |
| .gitignore | Ignores .idea directory. |
Comments suppressed due to low confidence (1)
src/utils/beaconUtils.ts:56
getRecentSearchesnow claims to returnRecentSearches[], but the legacy-upgrade path converts string entries into{ term, ts }objects without adatafield. This makes the stored shape inconsistent with the newRecentSearchestype and can lead todatabeing unexpectedly missing at runtime. Consider normalizing upgraded items to includedata: {}(and optionally persisting the upgraded array back to storage so timestamps/ids are stable across calls).
export const getRecentSearches = (): RecentSearches[] => {
const recentSearches = storageGetArray(CONSTANTS.RECENT_SEARCHES_STORAGE_KEY) || [];
// upgrade the array to store timestamps if it isn't already
recentSearches.forEach((item, i) => {
if (typeof item === 'string') {
recentSearches[i] = {
term: item,
ts: Date.now(),
};
}
});
return recentSearches;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Code Review
This PR adds first-party recent searches support to the autocomplete dropdown as a new recentSearches zero-state section type, reading from localStorage and integrating with the existing tracking pipeline.
Inline comments: 7 discussions added
Overall Assessment:
| if (selectedItem?.section === 'Search Suggestions') { | ||
| if ( | ||
| selectedItem?.section === 'Search Suggestions' || | ||
| selectedItem?.section === 'recent-searches' |
There was a problem hiding this comment.
Critical Issue: There is a logic bug here. trackSearchSubmit is called for recent-searches items in the condition above (lines 41-49), AND then trackAutocompleteSelect is called again for recent-searches items in the else if branch below (lines 78-84). This means selecting a recent search item fires both trackSearchSubmit AND trackAutocompleteSelect, double-tracking the event. The else if (selectedItem.section === 'recent-searches') block at line 78 is dead/unreachable for the trackAutocompleteSelect call that was intentionally added to track recent searches as Search Suggestions — the item was already handled by the first if branch. Remove the || selectedItem?.section === 'recent-searches' from the first condition and handle recent searches exclusively in the dedicated else if block, or restructure the conditionals so the paths are mutually exclusive.
There was a problem hiding this comment.
Intended, the block is reachable
| }); | ||
|
|
||
| setRecentSearches(recentSearchesResults); | ||
| }, [recentSearchesSections]); |
There was a problem hiding this comment.
Important Issue: The useEffect dependency on recentSearchesSections will cause re-execution on every render because activeRecentSearchesSections is derived via useMemo in useSectionsResults.ts, but activeSections itself may produce a new array reference each render. More critically, recentSearchesFromStore is read once at mount (or when the section config changes), so it will not reflect localStorage changes made mid-session (e.g. after a search submit stores a new term). Consider using a storage event listener or at least reading localStorage on every focus/open event so the list stays fresh within a session.
There was a problem hiding this comment.
it doesn't re-execute on every render as the activeSections is not a new reference every render.
New searches get added mid-session
| const recentSearchesResults: SectionsData = {}; | ||
|
|
||
| recentSearchesSections.forEach(({ displayName, numResults }) => { | ||
| recentSearchesResults[displayName] = recentSearchesFromStore |
There was a problem hiding this comment.
Important Issue: The section results are keyed by displayName (a user-controlled string), which is the same key used to look up data in getActiveSectionsWithData (sectionsResults[sectionConfig.displayName]). If two recentSearches sections are configured with the same displayName, or if a custom section happens to have the same displayName as a recentSearches section, results will silently collide and overwrite. A more collision-resistant key (e.g. a stable internal ID or a namespaced key like recentSearches:${displayName}) would be safer.
There was a problem hiding this comment.
It's unreasonable to add multiple recent searches sections so displayName is used as a key here
| // so there is no query and no term highlighting. | ||
| if (getSearchResultsUrl) { | ||
| defaultChildren = ( | ||
| <a className='suggestion-link' href={getSearchResultsUrl(item)}> |
There was a problem hiding this comment.
Suggestion: The <a> element rendered for recent search items is structurally identical to the one rendered for SearchSuggestion items in SearchSuggestionItem. Consider extracting a shared SuggestionLink component (or reusing SearchSuggestionItem) to avoid the duplication and keep the two render paths in sync when one is updated in the future.
There was a problem hiding this comment.
Intentionally separated for better maintenance. Search suggestions might behave differently from recent searches
| { term: 'blue jeans', ts: Date.now() - 2000 }, | ||
| { term: 'white sneakers', ts: Date.now() - 1000 }, | ||
| ]; | ||
| localStorage.setItem(recentSearchesStorageKey, JSON.stringify(mockRecentSearches)); |
There was a problem hiding this comment.
Suggestion: mockRecentSearches is populated and written to localStorage at module evaluation time (top-level, outside any story play function). This is a side effect that will affect every other story in the file that runs in the same browser context. The setup should be confined entirely to the play function of each story (or a loaders hook), just as is already done inside the play functions themselves. Remove the top-level localStorage.setItem call.
No description provided.