Skip to content

[CDX-310]: added recent searches to autocomplete dropdown#267

Open
niizom wants to merge 5 commits intomainfrom
cdx-310-ac-ui-feature-recently-searched-terms
Open

[CDX-310]: added recent searches to autocomplete dropdown#267
niizom wants to merge 5 commits intomainfrom
cdx-310-ac-ui-feature-recently-searched-terms

Conversation

@niizom
Copy link
Copy Markdown

@niizom niizom commented Mar 13, 2026

No description provided.

@niizom niizom requested review from Mudaafi and esezen as code owners March 13, 2026 19:32
Copilot AI review requested due to automatic review settings March 13, 2026 19:32
@constructor-claude-bedrock
Copy link
Copy Markdown

Code Review Results

✅ Strengths

The 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

  • [File: src/hooks/useGetRecentSearches.ts Line: 28] The useEffect dependency array contains recentSearchesSection (an array), which will be a new reference on every render since it is derived from useMemo in useSectionsResults. This will cause the effect to re-run unnecessarily on every render, re-reading from storage and calling setRecentSearches on every cycle. The effect should either depend on a stable primitive (e.g., serialize or use a ref) or the array comparison should be stabilized. Additionally, the effect does not update when the underlying localStorage data changes (e.g., after a new search is submitted), so freshly stored searches will not appear until a re-render driven by something else. Consider reading from storage on every render (without the effect) or subscribing to storage events.

  • [File: src/hooks/useGetRecentSearches.ts Line: 12] recentSearchesResults is typed as {} (plain object / implicit any index signature) instead of SectionsData. Replace with const recentSearchesResults: SectionsData = {}; to maintain full type safety throughout.

  • [File: src/hooks/useGetRecentSearches.ts Line: 16] When numResults is undefined (it is optional in SectionConfiguration), slice(0, undefined) returns the full array. This is probably unintentional — it means omitting numResults shows all stored recent searches (up to 100) rather than applying a sensible default. Add a fallback: slice(0, numResults ?? DEFAULT_NUM_RESULTS).

⚠️ Important Issues

  • [File: src/hooks/useDownShift.ts Lines: 41-53] The 'Search Suggestions' block (lines 41–46) and the new 'recent-searches' block (lines 48–53) are identical in behaviour. They can be merged into a single condition:

    if (selectedItem?.section === 'Search Suggestions' || selectedItem?.section === 'recent-searches') {
      setQuery(selectedItem.value || '');
      trackSearchSubmit(cioClient, selectedItem.value, { originalQuery: previousQuery });
    }

    The duplication will become a maintenance burden.

  • [File: src/hooks/useGetRecentSearches.ts Line: 23] The string literal 'recent-searches' (kebab-case) is used as the section value for items, but the section type identifier in types.ts is 'recentSearches' (camelCase). Using two different strings for the same concept across the codebase (section.type === 'recentSearches' in type guards vs item.section === 'recent-searches' in downshift and styles) is fragile. Extract both into shared constants to prevent future mismatches.

  • [File: src/utils/beaconUtils.ts Line: 47-54] getRecentSearches mutates the array returned from storage (recentSearches[i] = ...) in the upgrade path. Functions that read from a store should not have side effects — the mutation silently modifies the in-memory array without persisting the upgraded format back to storage. Either persist the upgraded entries or return a new mapped array without mutating.

  • [File: src/utils/beaconUtils.ts Line: 62] storeRecentSearch(term: string, suggestionData)suggestionData is still untyped. Given the RecentSearches type was just added, type this parameter as Record<string, unknown> (matching RecentSearches.data) or a dedicated type.

  • [General] No tests were added for the new useGetRecentSearches hook or for the recentSearches section flow end-to-end. Per the review standards, all code changes must be covered by tests. At minimum, unit tests for useGetRecentSearches covering: empty section array, numResults cap, and the item shape produced (especially section: 'recent-searches'), and integration-level coverage of the recentSearches case in useDownShift's onSelectedItemChange.

💡 Suggestions

  • [File: src/components/Autocomplete/SectionItemsList/SectionItemsList.tsx and AutocompleteResults.tsx] The case 'recentSearches': sectionTitle = section.displayName; break; block is identical to the existing case 'custom' block in both files. Consider combining them (case 'custom': case 'recentSearches':) to avoid repeating the same logic.

  • [File: src/hooks/useCioAutocomplete.ts Lines: 280-285] Same deduplication opportunity: the case 'recentSearches': sectionTitle = section.displayName; break; inside getDeprecatedClassNames is identical to case 'custom'. Fall-through cases would keep this DRY.

  • [File: src/types.ts Line: 307-311] The RecentSearches type has a data field typed as Record<string, unknown>, but storeRecentSearch is called with {} (empty object) from trackSearchSubmit. If data is always empty when coming from a search submit, consider making it optional (data?: Record<string, unknown>) to reflect actual usage.

Overall Assessment: ⚠️ Needs Work

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

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 recentSearches as 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 trackSearchSubmit on 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

  • getRecentSearches now claims to return RecentSearches[], but the legacy-upgrade path converts string entries into { term, ts } objects without a data field. This makes the stored shape inconsistent with the new RecentSearches type and can lead to data being unexpectedly missing at runtime. Consider normalizing upgraded items to include data: {} (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.

Comment thread src/utils/beaconUtils.ts
Comment thread src/hooks/useGetRecentSearches.ts
Comment thread src/hooks/useGetRecentSearches.ts Outdated
Comment thread src/hooks/useGetRecentSearches.ts
Comment thread src/types.ts Outdated
@niizom niizom changed the title [CDX-310]: added recent searches to autocomplete dropdown DRAFT: [CDX-310]: added recent searches to autocomplete dropdown Mar 16, 2026
@esezen esezen requested a review from a team March 16, 2026 15:28
constructor-claude-bedrock[bot]

This comment was marked as outdated.

@TarekAlQaddy TarekAlQaddy changed the title DRAFT: [CDX-310]: added recent searches to autocomplete dropdown [CDX-310]: added recent searches to autocomplete dropdown Apr 6, 2026
constructor-claude-bedrock[bot]

This comment was marked as outdated.

constructor-claude-bedrock[bot]

This comment was marked as outdated.

Copy link
Copy Markdown

@constructor-claude-bedrock constructor-claude-bedrock Bot left a comment

Choose a reason for hiding this comment

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

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: ⚠️ Needs Work

Comment thread src/hooks/useDownShift.ts
if (selectedItem?.section === 'Search Suggestions') {
if (
selectedItem?.section === 'Search Suggestions' ||
selectedItem?.section === 'recent-searches'
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

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.

Intended, the block is reachable

});

setRecentSearches(recentSearchesResults);
}, [recentSearchesSections]);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

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.

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
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

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.

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)}>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

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.

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));
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

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