feat: Move Main Page Search to Global Navigation & Simplify Search UI/UX#4160
Merged
kasya merged 15 commits intoOWASP:mainfrom Mar 8, 2026
Merged
feat: Move Main Page Search to Global Navigation & Simplify Search UI/UX#4160kasya merged 15 commits intoOWASP:mainfrom
kasya merged 15 commits intoOWASP:mainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaces the in-page MultiSearch with a new GlobalSearch component (added to Header), removes the MultiSearch component and its unit/a11y tests, updates the home page to show a keyboard-shortcut hint, adds a GlobalSearch unit test, and removes MultiSearchBarProps type. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
2 issues found across 10 files
Confidence score: 3/5
- There is some merge risk: both findings are medium severity (6/10) with high confidence, and they affect core search behavior in
frontend/src/components/GlobalSearch.tsx. - The most impactful issue is the debounced async search race condition in
frontend/src/components/GlobalSearch.tsx, where older responses can overwrite newer queries and show stale suggestions to users. frontend/src/components/GlobalSearch.tsxalso omits theeventsindex, which creates a concrete functional gap—event results cannot appear even though event-specific UI/navigation exists.- Pay close attention to
frontend/src/components/GlobalSearch.tsx- async request ordering and missingeventsindex can cause incorrect or incomplete search results.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="frontend/src/components/GlobalSearch.tsx">
<violation number="1" location="frontend/src/components/GlobalSearch.tsx:20">
P2: Global search excludes the `events` index, so event results can never be returned despite event-specific UI/navigation logic.</violation>
<violation number="2" location="frontend/src/components/GlobalSearch.tsx:91">
P2: Debounced async search updates state without request versioning/cancellation, so older responses can overwrite newer query suggestions.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
frontend/__tests__/unit/pages/Header.test.tsx (1)
97-101: Add an explicit integration assertion forGlobalSearch.You mock
components/GlobalSearchat Line 97, but there’s no assertion that Header actually renders it. A small assertion ondata-testid="global-search"would prevent silent regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/pages/Header.test.tsx` around lines 97 - 101, The test mocks components/GlobalSearch but never asserts Header renders it; update the Header unit test (where jest.mock('components/GlobalSearch') is declared) to add an explicit assertion that the mocked GlobalSearch is present by checking for data-testid="global-search" (e.g., using getByTestId or queryByTestId against the rendered Header output) so the test fails if Header stops rendering the GlobalSearch component.frontend/__tests__/unit/components/GlobalSearch.test.tsx (1)
81-88: AddCtrl+Kcoverage for non-macOS shortcut behavior.This test only validates
metaKey(Cmd). Since the component supports both keys, add actrlKey: truecase to protect cross-platform behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/components/GlobalSearch.test.tsx` around lines 81 - 88, Update the unit test for GlobalSearch to also assert the Ctrl+K path: in the test (the one rendering GlobalSearch and using fireEvent.keyDown on document), add an additional fireEvent.keyDown(document, { key: 'k', ctrlKey: true }) scenario (or convert to a parameterized test) and reuse the same waitFor/assert (screen.getByRole('dialog')) so the test covers both metaKey and ctrlKey keyboard shortcuts across platforms.frontend/__tests__/unit/pages/Home.test.tsx (1)
132-137: Strengthen the shortcut hint assertion.At Line 136 and Line 406, the regex only checks the trailing sentence. Add an explicit assertion for the keyboard token so UI regressions in the key hint are caught.
Also applies to: 406-406
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/pages/Home.test.tsx` around lines 132 - 137, The current test "renders search keyboard shortcut hint" in Home.test.tsx only asserts the trailing hint text; update the test to also assert the actual keyboard token is rendered by adding an explicit expectation for the keyboard hint (for example verify the visible kbd token such as "⌘K" or "Ctrl+K" or query for a <kbd> element containing "K") using screen.getByText or screen.getByRole inside the same waitFor; apply the same change to the other similar assertion at the other spot (around line 406) so UI regressions in the key hint are caught (refer to the test name "renders search keyboard shortcut hint", the Home component render call, and existing waitFor/screen.getByText usage to locate where to insert the extra assertion).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/GlobalSearch.tsx`:
- Around line 20-21: INDEXES currently omits "events", making the event-specific
search branches and UI (the event search paths and event labeling/icon logic in
GlobalSearch) unreachable; update the INDEXES constant (and any place that
builds searchable index lists) to include 'events' so the event search code
paths and rendering logic are executed, and then run the component's search flow
to confirm event suggestions appear correctly.
- Around line 74-78: The current telemetry call in GlobalSearch.tsx sends raw
search text (query) via sendGAEvent(event: 'globalSearch'), which may leak
sensitive user input; change the payload to avoid raw content by replacing the
value field with a non-sensitive metric (e.g., query length, result count, or a
hashed/bucketed representation of the query) and update callers
accordingly—modify the sendGAEvent call in the GlobalSearch component to send
something like { event: 'globalSearch', path: globalThis.location.pathname,
value: query.length } or compute a stable hash/bucket of query before passing
it, ensuring the variable query is never sent in plain text.
- Around line 72-92: The Promise.all call inside the debounce callback causes
all requests to fail if any fetchAlgoliaData rejects; change the per-index fetch
to isolate failures by using Promise.allSettled or mapping INDEXES to async
functions that catch errors (e.g., in the async (index) => { try { const data =
await fetchAlgoliaData(...) } catch (err) { return { indexName: index, hits: [],
totalPages: 0 } } }) so a failed index returns an empty result instead of
rejecting the whole Promise; keep the rest of the flow (setSuggestions and
setShowSuggestions) the same and ensure the final results filter still works
with the fallback objects.
- Line 256: In the GlobalSearch component update the JSX className that
currently contains "z-100" to use Tailwind's arbitrary value syntax "z-[100]"
(e.g., change className="fixed inset-0 z-100 flex items-start justify-center
px-3 pt-[10vh] sm:px-0 sm:pt-[15vh]" to use z-[100]) so the modal stacks above
elements with z-50; locate the className on the modal root in the GlobalSearch
component and replace the token accordingly.
---
Nitpick comments:
In `@frontend/__tests__/unit/components/GlobalSearch.test.tsx`:
- Around line 81-88: Update the unit test for GlobalSearch to also assert the
Ctrl+K path: in the test (the one rendering GlobalSearch and using
fireEvent.keyDown on document), add an additional fireEvent.keyDown(document, {
key: 'k', ctrlKey: true }) scenario (or convert to a parameterized test) and
reuse the same waitFor/assert (screen.getByRole('dialog')) so the test covers
both metaKey and ctrlKey keyboard shortcuts across platforms.
In `@frontend/__tests__/unit/pages/Header.test.tsx`:
- Around line 97-101: The test mocks components/GlobalSearch but never asserts
Header renders it; update the Header unit test (where
jest.mock('components/GlobalSearch') is declared) to add an explicit assertion
that the mocked GlobalSearch is present by checking for
data-testid="global-search" (e.g., using getByTestId or queryByTestId against
the rendered Header output) so the test fails if Header stops rendering the
GlobalSearch component.
In `@frontend/__tests__/unit/pages/Home.test.tsx`:
- Around line 132-137: The current test "renders search keyboard shortcut hint"
in Home.test.tsx only asserts the trailing hint text; update the test to also
assert the actual keyboard token is rendered by adding an explicit expectation
for the keyboard hint (for example verify the visible kbd token such as "⌘K" or
"Ctrl+K" or query for a <kbd> element containing "K") using screen.getByText or
screen.getByRole inside the same waitFor; apply the same change to the other
similar assertion at the other spot (around line 406) so UI regressions in the
key hint are caught (refer to the test name "renders search keyboard shortcut
hint", the Home component render call, and existing waitFor/screen.getByText
usage to locate where to insert the extra assertion).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
frontend/__tests__/a11y/components/MultiSearch.a11y.test.tsxfrontend/__tests__/unit/components/GlobalSearch.test.tsxfrontend/__tests__/unit/components/MultiSearch.test.tsxfrontend/__tests__/unit/pages/Header.test.tsxfrontend/__tests__/unit/pages/Home.test.tsxfrontend/src/app/page.tsxfrontend/src/components/GlobalSearch.tsxfrontend/src/components/Header.tsxfrontend/src/components/MultiSearch.tsxfrontend/src/types/search.ts
💤 Files with no reviewable changes (4)
- frontend/tests/a11y/components/MultiSearch.a11y.test.tsx
- frontend/src/types/search.ts
- frontend/src/components/MultiSearch.tsx
- frontend/tests/unit/components/MultiSearch.test.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/GlobalSearch.tsx`:
- Around line 343-389: The dialog currently lacks a focus trap so keyboard users
can tab out of the modal; implement a focus-trap when the dialog is open by
wiring a keydown handler on the dialog (or on panelRef) that captures
Tab/Shift+Tab and keeps focus within the modal's focusable elements (use
panelRef to query focusable selectors), preventDefault and cycle focus to
first/last as needed, ensure inputRef is focused on open and save/restore the
previously focused element on open/close, and make sure closing via
setIsOpen(false) restores focus to the prior element; attach this logic near
where panelRef, inputRef, setIsOpen, handleClearSearch, and renderSearchContent
are defined.
- Around line 83-114: When query is cleared or suggestions are hidden we must
invalidate any in-flight searches by bumping searchVersionRef so late responses
are ignored; modify the else branch (where setSuggestions([]) and
setShowSuggestions(false) are called) and any modal close/clear handlers to
increment searchVersionRef.current (same mechanism used at the top of the
non-empty branch) so results from Promise.all for fetchAlgoliaData won't
repopulate suggestions after the input is cleared or modal closed; ensure this
same incrementing is used consistently wherever setShowSuggestions(false) or
input clear logic lives.
- Line 90: The call to fetchAlgoliaData in GlobalSearch (const data = await
fetchAlgoliaData(index, query, 1, SUGGESTION_COUNT)) uses page=1 but Algolia
pages are zero-based; change the third argument from 1 to 0 so
fetchAlgoliaData(index, query, 0, SUGGESTION_COUNT) returns the first page of
results for suggestions. Ensure any other calls or tests expecting page=1 are
updated accordingly to use 0-based pagination where appropriate.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/__tests__/unit/components/GlobalSearch.test.tsxfrontend/src/app/page.tsxfrontend/src/components/GlobalSearch.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/tests/unit/components/GlobalSearch.test.tsx
- frontend/src/app/page.tsx
Contributor
There was a problem hiding this comment.
4 issues found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="frontend/__tests__/unit/components/GlobalSearch.test.tsx">
<violation number="1" location="frontend/__tests__/unit/components/GlobalSearch.test.tsx:394">
P2: Stale-response test is brittle because it hard-codes a fixed number of `fetchAlgoliaData` calls tied to current index fan-out.</violation>
</file>
<file name="frontend/src/components/GlobalSearch.tsx">
<violation number="1" location="frontend/src/components/GlobalSearch.tsx:84">
P2: Race condition: `searchVersionRef` is only incremented when `query.length > 0`. If the user clears the input while searches are in-flight, late responses can still pass the `version !== searchVersionRef.current` guard and repopulate suggestions after they were cleared. Move `++searchVersionRef.current` before the `if (query.length > 0)` branch so clearing the query also invalidates pending requests.</violation>
<violation number="2" location="frontend/src/components/GlobalSearch.tsx:90">
P1: Algolia's search API uses zero-based pagination (`page=0` is the first page). Passing `1` here skips the most relevant results and instead returns the second page. This should be `0` to show users the top-ranked matches in the suggestions.</violation>
<violation number="3" location="frontend/src/components/GlobalSearch.tsx:105">
P2: `searchError` is not cleared when query is emptied, so error UI can persist after user clears search input.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
b24e8b8 to
a891888
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
frontend/src/components/GlobalSearch.tsx (3)
90-90:⚠️ Potential issue | 🟠 MajorUse first-page pagination for suggestions.
At Line [90],
currentPageis passed as1, which skips the initial page of results. The local helper defaults first page to0, so suggestions should request0.Proposed fix
-const data = await fetchAlgoliaData(index, query, 1, SUGGESTION_COUNT) +const data = await fetchAlgoliaData(index, query, 0, SUGGESTION_COUNT)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/GlobalSearch.tsx` at line 90, The suggestions call uses the wrong page index: change the call to fetchAlgoliaData in GlobalSearch (currently passing 1) to pass page 0 so it requests the first page that the local helper expects; update the invocation that supplies currentPage (the call with fetchAlgoliaData(index, query, 1, SUGGESTION_COUNT)) to use 0 instead.
63-71:⚠️ Potential issue | 🟠 MajorInvalidate pending searches when clearing or closing the dialog.
searchVersionRefis not bumped (and debounce not canceled) on clear/close, so late responses can repopulate stale suggestions after Line [65]-Line [69] resets.Proposed fix
useEffect(() => { if (!isOpen) { + searchVersionRef.current += 1 + debouncedSearch.cancel() setSearchQuery('') setSuggestions([]) setShowSuggestions(false) setHighlightedIndex(null) setSearchError(false) } -}, [isOpen]) +}, [isOpen, debouncedSearch]) const handleClearSearch = () => { + searchVersionRef.current += 1 + debouncedSearch.cancel() setSearchQuery('') setSuggestions([]) setShowSuggestions(false) setHighlightedIndex(null) + setSearchError(false) inputRef.current?.focus() }Also applies to: 111-114, 204-210
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/GlobalSearch.tsx` around lines 63 - 71, When the dialog is closed or cleared in the useEffect and other reset locations, bump the searchVersionRef and cancel any pending debounced search so late async responses can't repopulate suggestions: update the useEffect that resets state (and the other reset sites around lines with setSearchQuery/setSuggestions) to increment searchVersionRef.current (or set to a new token) and call the debounce cancel method (the debounced search function used by GlobalSearch) so any in-flight timers or callbacks are invalidated before clearing state.
357-403:⚠️ Potential issue | 🟠 MajorAdd a focus trap while the dialog is open.
The dialog gets initial focus, but keyboard focus can still escape to background controls via Tab/Shift+Tab. This is an accessibility blocker for modal behavior.
Proposed fix
+const previouslyFocusedRef = useRef<HTMLElement | null>(null) + useEffect(() => { if (isOpen) { + previouslyFocusedRef.current = document.activeElement as HTMLElement | null const timer = setTimeout(() => inputRef.current?.focus(), 50) document.body.style.overflow = 'hidden' return () => { clearTimeout(timer) document.body.style.overflow = '' + previouslyFocusedRef.current?.focus() } } else { document.body.style.overflow = '' } }, [isOpen]) + +useEffect(() => { + if (!isOpen || !panelRef.current) return + const panel = panelRef.current + const selector = + 'a[href], button:not([disabled]), textarea, input, select, [tabindex]:not([tabindex="-1"])' + + const onKeyDown = (e: KeyboardEvent) => { + if (e.key !== 'Tab') return + const focusables = panel.querySelectorAll<HTMLElement>(selector) + if (!focusables.length) return + const first = focusables[0] + const last = focusables[focusables.length - 1] + const active = document.activeElement as HTMLElement | null + if (e.shiftKey && active === first) { + e.preventDefault() + last.focus() + } else if (!e.shiftKey && active === last) { + e.preventDefault() + first.focus() + } + } + + document.addEventListener('keydown', onKeyDown) + return () => document.removeEventListener('keydown', onKeyDown) +}, [isOpen])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/GlobalSearch.tsx` around lines 357 - 403, Add a focus trap to the modal so keyboard focus cannot escape background controls while the dialog is open: when opening (isOpen true) set focus to inputRef and attach a keydown handler on the panelRef (or dialog) that intercepts Tab/Shift+Tab, computes the list of focusable elements inside the panel (querySelectorAll for a, button, input, textarea, select, [tabindex]:not([tabindex="-1"])), and moves focus to first/last accordingly to wrap; also prevent propagation of Tab so background elements are not focused. Ensure the handler is removed on close (setIsOpen false) and restore previously focused element; reference inputRef, panelRef, renderSearchContent, setIsOpen, handleClearSearch and handleSearchChange when locating where to add the trap and cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@frontend/src/components/GlobalSearch.tsx`:
- Line 90: The suggestions call uses the wrong page index: change the call to
fetchAlgoliaData in GlobalSearch (currently passing 1) to pass page 0 so it
requests the first page that the local helper expects; update the invocation
that supplies currentPage (the call with fetchAlgoliaData(index, query, 1,
SUGGESTION_COUNT)) to use 0 instead.
- Around line 63-71: When the dialog is closed or cleared in the useEffect and
other reset locations, bump the searchVersionRef and cancel any pending
debounced search so late async responses can't repopulate suggestions: update
the useEffect that resets state (and the other reset sites around lines with
setSearchQuery/setSuggestions) to increment searchVersionRef.current (or set to
a new token) and call the debounce cancel method (the debounced search function
used by GlobalSearch) so any in-flight timers or callbacks are invalidated
before clearing state.
- Around line 357-403: Add a focus trap to the modal so keyboard focus cannot
escape background controls while the dialog is open: when opening (isOpen true)
set focus to inputRef and attach a keydown handler on the panelRef (or dialog)
that intercepts Tab/Shift+Tab, computes the list of focusable elements inside
the panel (querySelectorAll for a, button, input, textarea, select,
[tabindex]:not([tabindex="-1"])), and moves focus to first/last accordingly to
wrap; also prevent propagation of Tab so background elements are not focused.
Ensure the handler is removed on close (setIsOpen false) and restore previously
focused element; reference inputRef, panelRef, renderSearchContent, setIsOpen,
handleClearSearch and handleSearchChange when locating where to add the trap and
cleanup.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/__tests__/unit/components/GlobalSearch.test.tsxfrontend/src/app/page.tsxfrontend/src/components/GlobalSearch.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/tests/unit/components/GlobalSearch.test.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
frontend/src/components/GlobalSearch.tsx (2)
67-76:⚠️ Potential issue | 🟠 MajorCancel pending debounced searches on clear/close to prevent stale repopulation.
searchVersionRefis incremented, but a queued debounced callback can still run later and repopulate suggestions after clear/close.🐛 Proposed fix
useEffect(() => { if (!isOpen) { + debouncedSearch.cancel() searchVersionRef.current++ setSearchQuery('') setSuggestions([]) setShowSuggestions(false) setHighlightedIndex(null) setSearchError(false) } - }, [isOpen]) + }, [isOpen, debouncedSearch]) const handleClearSearch = () => { + debouncedSearch.cancel() searchVersionRef.current++ setSearchQuery('') setSuggestions([]) setShowSuggestions(false) setHighlightedIndex(null) inputRef.current?.focus() }Also applies to: 78-121, 241-248
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/GlobalSearch.tsx` around lines 67 - 76, The clear/close useEffect increments searchVersionRef and resets state but does not cancel any queued debounced search, so a pending debounced callback can later repopulate suggestions; update the same effect (and the other clear paths mentioned) to cancel the debounced search before resetting state by calling the debouncer’s cancel method (e.g., fetchSuggestionsDebounced.cancel()) or aborting the associated AbortController, and also ensure any debounced callback checks searchVersionRef (captured vs current) before calling setSuggestions/setShowSuggestions/setHighlightedIndex/setSearchError to avoid stale updates.
95-95:⚠️ Potential issue | 🟠 MajorUse page
0for first-page suggestions.Algolia's Search API uses zero-based page indexing (first page =
page=0). Passing1here fetches the second page and skips the highest-ranked results, degrading autocomplete quality.🐛 Proposed fix
- const data = await fetchAlgoliaData(index, query, 1, SUGGESTION_COUNT) + const data = await fetchAlgoliaData(index, query, 0, SUGGESTION_COUNT)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/GlobalSearch.tsx` at line 95, The call to fetchAlgoliaData in GlobalSearch (const data = await fetchAlgoliaData(index, query, 1, SUGGESTION_COUNT)) uses page=1 but Algolia is zero-based, so change the page argument to 0 to request the first page; update the invocation in GlobalSearch.tsx (the fetchAlgoliaData call) to pass 0 instead of 1 so autocomplete returns the top-ranked first-page suggestions.
🧹 Nitpick comments (1)
frontend/__tests__/unit/components/GlobalSearch.test.tsx (1)
393-407: Make stale-response test independent of index count.
callCount <= 5hard-codes the current number of indexes, so this test can break for unrelated index-list changes. Prefer branching byqueryarg ('a'vs'ab') to keep the test stable.♻️ Suggested change
- let callCount = 0 - ;(fetchAlgoliaData as jest.Mock).mockImplementation((index: string) => { - callCount++ - // First batch of calls (first keystroke) returns a slow, pending promise - if (callCount <= 5) { + ;(fetchAlgoliaData as jest.Mock).mockImplementation((index: string, query: string) => { + // First query batch returns a slow, pending promise + if (query === 'a') { return firstPromise } - // Second batch (second keystroke) resolves immediately + // Second query batch resolves immediately if (index === 'projects') { return Promise.resolve({ hits: [{ key: 'second-result', name: 'Second Result' }], totalPages: 1, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/components/GlobalSearch.test.tsx` around lines 393 - 407, The test currently uses callCount in the fetchAlgoliaData mock to decide when to return a slow pending promise (callCount <= 5), which couples the test to the current number of indexes; change the mock implementation of fetchAlgoliaData to branch on the query argument instead: when the query arg is 'a' return the slow firstPromise, and when the query arg is 'ab' return the immediate resolved result (and keep the existing per-index results for 'projects' etc.), so the stale-response behavior is driven by query values rather than call order; update the mock function (the jest.Mock for fetchAlgoliaData) accordingly and remove reliance on callCount.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/GlobalSearch.tsx`:
- Around line 139-141: In GlobalSearch.tsx inside the switch branch handling
case 'events' (the code using suggestion as Event and calling globalThis.open),
validate the event URL before calling globalThis.open: ensure the URL is a
string and its protocol is either 'http://' or 'https://' (e.g., check
startsWith('http://') || startsWith('https://')); only call globalThis.open when
that check passes, otherwise skip opening (and optionally log or show a safe
fallback). This prevents opening values with other protocols.
---
Duplicate comments:
In `@frontend/src/components/GlobalSearch.tsx`:
- Around line 67-76: The clear/close useEffect increments searchVersionRef and
resets state but does not cancel any queued debounced search, so a pending
debounced callback can later repopulate suggestions; update the same effect (and
the other clear paths mentioned) to cancel the debounced search before resetting
state by calling the debouncer’s cancel method (e.g.,
fetchSuggestionsDebounced.cancel()) or aborting the associated AbortController,
and also ensure any debounced callback checks searchVersionRef (captured vs
current) before calling
setSuggestions/setShowSuggestions/setHighlightedIndex/setSearchError to avoid
stale updates.
- Line 95: The call to fetchAlgoliaData in GlobalSearch (const data = await
fetchAlgoliaData(index, query, 1, SUGGESTION_COUNT)) uses page=1 but Algolia is
zero-based, so change the page argument to 0 to request the first page; update
the invocation in GlobalSearch.tsx (the fetchAlgoliaData call) to pass 0 instead
of 1 so autocomplete returns the top-ranked first-page suggestions.
---
Nitpick comments:
In `@frontend/__tests__/unit/components/GlobalSearch.test.tsx`:
- Around line 393-407: The test currently uses callCount in the fetchAlgoliaData
mock to decide when to return a slow pending promise (callCount <= 5), which
couples the test to the current number of indexes; change the mock
implementation of fetchAlgoliaData to branch on the query argument instead: when
the query arg is 'a' return the slow firstPromise, and when the query arg is
'ab' return the immediate resolved result (and keep the existing per-index
results for 'projects' etc.), so the stale-response behavior is driven by query
values rather than call order; update the mock function (the jest.Mock for
fetchAlgoliaData) accordingly and remove reliance on callCount.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/__tests__/unit/components/GlobalSearch.test.tsxfrontend/src/components/GlobalSearch.tsx
660304d to
add57b0
Compare
Contributor
Author
|
Hi @arkid15r, The PR is ready for review let me know if any changes required. |
kasya
requested changes
Mar 5, 2026
Collaborator
kasya
left a comment
There was a problem hiding this comment.
@mrkeshav-05 this looks awesome! Thank you!
Left a couple of requests ⬇️
Collaborator
There was a problem hiding this comment.
@mrkeshav-05 can you also add these tests back under GlobalSearch component tests now? Similar to what you did with components tests.
Comment on lines
+147
to
+157
| <p className="mt-4 text-sm text-gray-400 dark:text-gray-500"> | ||
| Press{' '} | ||
| <kbd className="rounded border border-gray-300 bg-gray-100 px-1.5 py-0.5 text-xs dark:border-gray-600 dark:bg-gray-700"> | ||
| ⌘K | ||
| </kbd>{' '} | ||
| /{' '} | ||
| <kbd className="rounded border border-gray-300 bg-gray-100 px-1.5 py-0.5 text-xs dark:border-gray-600 dark:bg-gray-700"> | ||
| Ctrl+K | ||
| </kbd>{' '} | ||
| to search across projects, chapters, and more. | ||
| </p> |
Collaborator
There was a problem hiding this comment.
Nice idea, but it does look a bit weird. Since we already show ⌘K on the search field, maybe we can get rid of this? And reduce padding a little bit between the subtitle and the Upcoming events block
Contributor
Author
|
Hi @kasya, The PR is ready for review. |
Contributor
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="frontend/src/components/GlobalSearch.tsx">
<violation number="1" location="frontend/src/components/GlobalSearch.tsx:389">
P2: Conflicting Tailwind dark-mode utilities on the search trigger create ambiguous effective styling and indicate likely accidental class duplication.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4160 +/- ##
==========================================
- Coverage 99.75% 99.43% -0.33%
==========================================
Files 519 519
Lines 16068 16145 +77
Branches 2149 2206 +57
==========================================
+ Hits 16029 16054 +25
- Misses 30 69 +39
- Partials 9 22 +13
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 5 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
kasya
previously approved these changes
Mar 7, 2026
Collaborator
kasya
left a comment
There was a problem hiding this comment.
@mrkeshav-05 thanks for working on this 👍🏼
Love our new Global Search!
I pushed a small change to update light theme colors - they were really really off.
|
kasya
approved these changes
Mar 8, 2026
Contributor
Author
Thank you @kasya for merging it! |
Merged
This was referenced Mar 24, 2026
Closed
This was referenced Mar 31, 2026
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
2 participants
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.
Proposed change
This PR moves the main page search bar into the global navigation bar, making search accessible across all pages.
The goal is to unify and simplify the search experience while avoiding redundancy between global search and page-specific search components (e.g., Chapters, Projects).
Additionally, a keyboard shortcut (Command + K) has been introduced to quickly activate the global search input.
Resolves: #4087
Screen.Recording.2026-03-03.at.5.15.43.PM.mov
Checklist
make check-testlocally: all warnings addressed, tests passed