Open
Conversation
Contributor
WalkthroughThis PR moves the search functionality from the home page to the global navigation header, making search accessible across all pages. It adds scope labels to page-specific searches to provide context, removes the redundant home page search bar, and introduces UI customization props for the MultiSearchBar component to support both mobile and desktop layouts. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Possibly related PRs
Suggested labels
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 |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/src/components/MultiSearch.tsx (1)
166-166:⚠️ Potential issue | 🟡 MinorMissing
showSuggestionsin dependency array.The
handleKeyDownfunction referencesshowSuggestions(line 123), but it's not included in theuseEffectdependency array. This could cause stale closure issues where the handler doesn't react toshowSuggestionschanges correctly.🔧 Proposed fix
- }, [searchQuery, suggestions, highlightedIndex, handleSuggestionClick]) + }, [searchQuery, suggestions, highlightedIndex, handleSuggestionClick, showSuggestions])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/MultiSearch.tsx` at line 166, The effect that registers the keyboard handler uses handleKeyDown which reads showSuggestions but showSuggestions is missing from the dependency array; update the useEffect in the MultiSearch component to include showSuggestions alongside searchQuery, suggestions, highlightedIndex, and handleSuggestionClick (or alternatively recreate/register handleKeyDown inside the effect) so the handler never captures a stale showSuggestions value.frontend/src/components/Header.tsx (1)
76-76:⚠️ Potential issue | 🟠 MajorMissing
mobileSearchOpenin dependency array.The
handleOutsideClickfunction referencesmobileSearchOpen(line 59), but onlymobileMenuOpenis in the dependency array. This will cause stale closure issues where outside clicks won't correctly close the mobile search panel.🔧 Proposed fix
- }, [mobileMenuOpen]) + }, [mobileMenuOpen, mobileSearchOpen])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/Header.tsx` at line 76, The useEffect that registers handleOutsideClick currently lists only mobileMenuOpen in its dependency array, but handleOutsideClick reads mobileSearchOpen, which can cause stale closures; update the dependency array for the effect that registers handleOutsideClick to include mobileSearchOpen (in addition to mobileMenuOpen) so the handler sees the latest mobileSearchOpen state, or alternatively memoize handleOutsideClick with useCallback using [mobileMenuOpen, mobileSearchOpen] and then use that stable callback in the effect.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@frontend/src/components/Header.tsx`:
- Line 76: The useEffect that registers handleOutsideClick currently lists only
mobileMenuOpen in its dependency array, but handleOutsideClick reads
mobileSearchOpen, which can cause stale closures; update the dependency array
for the effect that registers handleOutsideClick to include mobileSearchOpen (in
addition to mobileMenuOpen) so the handler sees the latest mobileSearchOpen
state, or alternatively memoize handleOutsideClick with useCallback using
[mobileMenuOpen, mobileSearchOpen] and then use that stable callback in the
effect.
In `@frontend/src/components/MultiSearch.tsx`:
- Line 166: The effect that registers the keyboard handler uses handleKeyDown
which reads showSuggestions but showSuggestions is missing from the dependency
array; update the useEffect in the MultiSearch component to include
showSuggestions alongside searchQuery, suggestions, highlightedIndex, and
handleSuggestionClick (or alternatively recreate/register handleKeyDown inside
the effect) so the handler never captures a stale showSuggestions value.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
frontend/src/app/chapters/page.tsxfrontend/src/app/committees/page.tsxfrontend/src/app/contribute/page.tsxfrontend/src/app/members/page.tsxfrontend/src/app/mentorship/programs/page.tsxfrontend/src/app/my/mentorship/page.tsxfrontend/src/app/organizations/page.tsxfrontend/src/app/page.tsxfrontend/src/app/projects/page.tsxfrontend/src/components/Header.tsxfrontend/src/components/MultiSearch.tsxfrontend/src/components/SearchPageLayout.tsxfrontend/src/types/search.ts
💤 Files with no reviewable changes (1)
- frontend/src/app/page.tsx
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 13 files
Confidence score: 3/5
- There’s some user-facing risk: moving
MultiSearchBarto a global header can drop event search results because it no longer receiveseventDatafrom the page query infrontend/src/app/page.tsx. - AutoFocus can fail to trigger after load because the effect in
frontend/src/components/MultiSearch.tsxdoesn’t re-run whenisLoadedflips, so the input may never get focus. - Pay close attention to
frontend/src/app/page.tsx,frontend/src/components/MultiSearch.tsx- search data wiring and focus timing depend on mount/load order.
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/app/page.tsx">
<violation number="1" location="frontend/src/app/page.tsx:149">
P1: Event search functionality will be lost when MultiSearchBar is moved to global Header. The component relies on `eventData={data?.upcomingEvents ?? []}` from page-specific GraphQL query, but the relocated component in Header.tsx does not receive this prop. Since events are not in the Algolia `indexes` array, search will silently return no event results.</violation>
</file>
<file name="frontend/src/components/MultiSearch.tsx">
<violation number="1" location="frontend/src/components/MultiSearch.tsx:182">
P1: The autoFocus feature fails when the component mounts with `isLoaded=false`. The useEffect only depends on `[autoFocus]` but the input element is conditionally rendered based on `isLoaded`. When `isLoaded` becomes `true`, the effect won't re-run because `autoFocus` hasn't changed, leaving the input unfocused.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| /> | ||
| </div> | ||
| </div> | ||
| <SecondaryCard |
Contributor
There was a problem hiding this comment.
P1: Event search functionality will be lost when MultiSearchBar is moved to global Header. The component relies on eventData={data?.upcomingEvents ?? []} from page-specific GraphQL query, but the relocated component in Header.tsx does not receive this prop. Since events are not in the Algolia indexes array, search will silently return no event results.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/src/app/page.tsx, line 149:
<comment>Event search functionality will be lost when MultiSearchBar is moved to global Header. The component relies on `eventData={data?.upcomingEvents ?? []}` from page-specific GraphQL query, but the relocated component in Header.tsx does not receive this prop. Since events are not in the Algolia `indexes` array, search will silently return no event results.</comment>
<file context>
@@ -146,14 +145,6 @@ export default function Home() {
- />
- </div>
</div>
<SecondaryCard
icon={FaCalendarAlt}
</file context>
Comment on lines
+182
to
+185
| useEffect(() => { | ||
| if (autoFocus) { | ||
| inputRef.current?.focus() | ||
| } |
Contributor
There was a problem hiding this comment.
P1: The autoFocus feature fails when the component mounts with isLoaded=false. The useEffect only depends on [autoFocus] but the input element is conditionally rendered based on isLoaded. When isLoaded becomes true, the effect won't re-run because autoFocus hasn't changed, leaving the input unfocused.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/src/components/MultiSearch.tsx, line 182:
<comment>The autoFocus feature fails when the component mounts with `isLoaded=false`. The useEffect only depends on `[autoFocus]` but the input element is conditionally rendered based on `isLoaded`. When `isLoaded` becomes `true`, the effect won't re-run because `autoFocus` hasn't changed, leaving the input unfocused.</comment>
<file context>
@@ -173,6 +179,12 @@ const MultiSearchBar: React.FC<MultiSearchBarProps> = ({
}
}, [])
+ useEffect(() => {
+ if (autoFocus) {
+ inputRef.current?.focus()
</file context>
Suggested change
| useEffect(() => { | |
| if (autoFocus) { | |
| inputRef.current?.focus() | |
| } | |
| useEffect(() => { | |
| if (autoFocus && isLoaded) { | |
| inputRef.current?.focus() | |
| } | |
| }, [autoFocus, isLoaded]) |
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
1 participant
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
Resolves #4087
This PR completes the migration of the main page search into the global navigation bar and improves search clarity across scoped pages. The update ensures a consistent, accessible, and non-redundant search experience while clearly distinguishing between:
Checklist
make check-testlocally: all warnings addressed, tests passed