-
Notifications
You must be signed in to change notification settings - Fork 339
feat: improve search with filters and infinite scroll #2386
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
base: main
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughThe search functionality has been refactored to support paginated, filterable results across multiple data sources. A new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as MobileSearch/<br/>DesktopSearch
participant StateHook as useSearchUIState
participant InfiniteQ as useInfiniteQuery
participant FilteredQ as useFilteredDefaultResults
participant Observer as Intersection<br/>Observer
participant API
User->>UI: Select filter type
UI->>StateHook: Update filter selection
StateHook->>InfiniteQ: Trigger with new filterType
StateHook->>FilteredQ: Trigger with new filterType
par Live & Default Results
InfiniteQ->>API: Fetch live results (filter applied)
API-->>InfiniteQ: Return paginated results
FilteredQ->>API: Fetch default results (filter applied)
API-->>FilteredQ: Return paginated results
end
InfiniteQ-->>StateHook: Data + pagination state
FilteredQ-->>StateHook: Data + pagination state
StateHook-->>UI: Render combined results + UI state
UI->>User: Display filtered results + load-more sentinel
Observer->>UI: Detect scroll near sentinel
UI->>StateHook: User scrolled to end
StateHook->>InfiniteQ: Invoke fetchNextPage()
InfiniteQ->>API: Fetch next page (offset)
API-->>InfiniteQ: Return next batch
InfiniteQ-->>StateHook: Append new data
StateHook-->>UI: Render additional results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Comment |
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.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/Search/index.tsx (2)
44-56: Security: API token exposed in client-side code.The Bearer token at line 48 is hardcoded and will be visible in browser dev tools and bundled source. Even for read-only public APIs, consider proxying requests through a Next.js API route to keep the token server-side.
406-406: Bug:onClickhandler receives event, not previous state.The
onClickcallback receives aMouseEvent, not the previous state value. Currently!prevevaluates tofalse(sinceMouseEventis truthy), so clicking always closes the search instead of toggling.🔎 Proposed fix
-<button onClick={(prev) => setOpen(!prev)} className="absolute top-0 bottom-0 left-2 my-auto opacity-50"> +<button onClick={() => setOpen((prev) => !prev)} className="absolute top-0 bottom-0 left-2 my-auto opacity-50">
🧹 Nitpick comments (3)
src/components/Search/index.tsx (3)
163-173: Consider addingtype="button"for defensive coding.While not currently an issue, adding
type="button"prevents accidental form submission if this component is ever rendered within a form context.🔎 Suggested fix
<button key={filter.value} + type="button" onClick={() => setSearchFilter(filter.value)}
287-313: Minor inconsistency: sentinel element styling differs between branches.Line 287 has
className="h-1"but line 313 has no className. Consider making them consistent for predictable scroll behavior.🔎 Suggested fix
-{hasNextPage && <div ref={loadMoreRef} />} +{hasNextPage && <div ref={loadMoreRef} className="h-1" />}
458-484: Same inconsistency: sentinel elements lack height.Both sentinel
divelements (lines 458 and 484) have no explicit height, unlike MobileSearch line 287. The IntersectionObserver may not trigger reliably for zero-height elements depending on browser behavior.🔎 Suggested fix
-{hasNextPage && <div ref={loadMoreRef} />} +{hasNextPage && <div ref={loadMoreRef} className="h-1" />}Apply to both lines 458 and 484.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Search/index.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/Search/index.tsx (2)
src/components/Search/types.ts (1)
ISearchItem(3-16)src/components/Loaders.tsx (1)
LoadingDots(1-9)
🔇 Additional comments (6)
src/components/Search/index.tsx (6)
1-14: LGTM!Imports and constants are appropriate. The
SEARCH_PAGE_SIZEconstant is well-placed for reuse across pagination logic.
72-83: LGTM!The
SearchFiltertype andFILTER_OPTIONSarray are well-structured with clear value-to-label mappings.
87-151: LGTM!The
useSearchUIStatehook effectively orchestrates multiple data sources (live search, default results, recents) with appropriate pagination controls. The conditional logic at lines 131-134 correctly switches between live search pagination and filtered default pagination.
203-223: LGTM!The IntersectionObserver setup is correct with proper cleanup. The dependency array appropriately triggers re-creation when pagination state changes.
513-600: LGTM!The
SearchItemcomponent,setRecentSearchhelper, anduseDefaultSearchListhook are well-implemented. The use ofuseSyncExternalStorefor localStorage synchronization is the correct React 18+ pattern.
602-634: LGTM!The
useSearchanduseFilteredDefaultResultshooks correctly implement infinite query patterns with proper page param calculation. Theenabledconditions appropriately gate network requests.
What
Why
Screenshots
Before:

After:

Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.