Embed filter selection in query params #276
Conversation
WalkthroughThis update synchronizes the filter selection state in the filter panel with the URL query parameters, enabling deep linking and state persistence across reloads and navigation. The Changes
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
CHANGELOG.md (1)
10-12: Minor wording nitTo stay consistent with Keep-a-Changelog style (“Added / Changed / Fixed … : …”), consider trimming the redundant “Changed” prefix:
-### Changed -- Changed the filter panel to embed the filter selection state in the query params. +### Changed +- The filter panel now embeds the filter-selection state in the query params.🧰 Tools
🪛 LanguageTool
[duplication] ~10-~10: Possible typo: you repeated a word.
Context: ...pec/v2.0.0.html). ## [Unreleased] ### Changed - Changed the filter panel to embed the filter se...(ENGLISH_WORD_REPEAT_RULE)
packages/web/src/app/[domain]/search/components/filterPanel/index.tsx (2)
100-115: Extraneous dependencies trigger superfluous re-filtering
routeris listed in the dependency array but isn’t referenced in the effect body; including it causes the effect to run on every navigation event even when filter state didn’t change.-}, [matches, repos, languages, onFilterChanged, searchParams, router]); +}, [matches, repos, languages, onFilterChanged, searchParams]);
116-139: Safer URL replacement – keep explicit pathnameUsing
router.replace('?…')relies on implicit relative resolution.
When the current route contains optional catch-all segments it can break (e.g./org/foo/search). Prepend the existing pathname for robustness:- router.replace(`?${newParams.toString()}`, { scroll: false }); + router.replace(`${window.location.pathname}?${newParams.toString()}`, { scroll: false });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CHANGELOG.md(1 hunks)packages/web/src/app/[domain]/search/components/filterPanel/index.tsx(4 hunks)packages/web/src/app/[domain]/search/page.tsx(3 hunks)packages/web/src/hooks/useNonEmptyQueryParam.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/web/src/app/[domain]/search/components/filterPanel/index.tsx (1)
packages/web/src/lib/types.ts (2)
SearchResultFile(11-11)Repository(20-20)
🪛 LanguageTool
CHANGELOG.md
[duplication] ~10-~10: Possible typo: you repeated a word.
Context: ...pec/v2.0.0.html). ## [Unreleased] ### Changed - Changed the filter panel to embed the filter se...
(ENGLISH_WORD_REPEAT_RULE)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (4)
packages/web/src/hooks/useNonEmptyQueryParam.ts (1)
20-24: Readable rename – looks goodChanging
inviteId→paramValuemakes the variable purpose clearer without touching behaviour. No further action required.packages/web/src/app/[domain]/search/page.tsx (3)
50-63: Clearer loading-state names – approvedRenaming to
isSearchLoading&isRepoMetadataLoadingeliminates ambiguity and makes the below conditional far easier to follow.
94-108: Consistent naming adopted in repo-metadata queryThe rename propagates correctly here; logic remains unchanged.
🧰 Tools
🪛 Biome (1.9.4)
[error] 102-102: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
197-203: Improved UX – spinner covers both async callsShowing the loader while either query is inflight prevents flashes of un-initialised UI. Nice touch.
| const router = useRouter(); | ||
| const searchParams = useSearchParams(); | ||
|
|
||
| // Helper to parse query params into sets | ||
| const getSelectedFromQuery = (param: string) => { | ||
| const value = searchParams.get(param); | ||
| return value ? new Set(value.split(',')) : new Set(); | ||
| }; | ||
|
|
||
| const [repos, setRepos] = useState<Record<string, Entry>>(() => { |
There was a problem hiding this comment.
State desynchronises when the URL is changed via browser navigation
repos and languages are initialised from the query string only once (the useState initialiser).
If the user:
- toggles filters (pushes a new URL),
- presses the browser “Back” button,
the query params revert but the repos / languages state does not, so the UI and filtering logic become inconsistent.
Add an effect that re-hydrates the state whenever searchParams changes:
+// Keep local state in sync if the URL changes externally
+useEffect(() => {
+ setRepos(prev => {
+ const selected = getSelectedFromQuery(REPOS_QUERY_PARAM);
+ return Object.fromEntries(
+ Object.entries(prev).map(([k, v]) => [
+ k,
+ { ...v, isSelected: selected.has(k) }
+ ]),
+ );
+ });
+ setLanguages(prev => {
+ const selected = getSelectedFromQuery(LANGUAGES_QUERY_PARAM);
+ return Object.fromEntries(
+ Object.entries(prev).map(([k, v]) => [
+ k,
+ { ...v, isSelected: selected.has(k) }
+ ]),
+ );
+ });
+}, [searchParams]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const router = useRouter(); | |
| const searchParams = useSearchParams(); | |
| // Helper to parse query params into sets | |
| const getSelectedFromQuery = (param: string) => { | |
| const value = searchParams.get(param); | |
| return value ? new Set(value.split(',')) : new Set(); | |
| }; | |
| const [repos, setRepos] = useState<Record<string, Entry>>(() => { | |
| const router = useRouter(); | |
| const searchParams = useSearchParams(); | |
| // Helper to parse query params into sets | |
| const getSelectedFromQuery = (param: string) => { | |
| const value = searchParams.get(param); | |
| return value ? new Set(value.split(',')) : new Set(); | |
| }; | |
| const [repos, setRepos] = useState<Record<string, Entry>>(() => { | |
| // initial repos setup... | |
| }); | |
| const [languages, setLanguages] = useState<Record<string, Entry>>(() => { | |
| // initial languages setup... | |
| }); | |
| // Keep local state in sync if the URL changes externally | |
| useEffect(() => { | |
| setRepos(prev => { | |
| const selected = getSelectedFromQuery(REPOS_QUERY_PARAM); | |
| return Object.fromEntries( | |
| Object.entries(prev).map(([k, v]) => [ | |
| k, | |
| { ...v, isSelected: selected.has(k) } | |
| ]), | |
| ); | |
| }); | |
| setLanguages(prev => { | |
| const selected = getSelectedFromQuery(LANGUAGES_QUERY_PARAM); | |
| return Object.fromEntries( | |
| Object.entries(prev).map(([k, v]) => [ | |
| k, | |
| { ...v, isSelected: selected.has(k) } | |
| ]), | |
| ); | |
| }); | |
| }, [searchParams]); |
This PR changes the filter panel to embed the filter selection into query parameters to make it easier to share search results. For example:
http://localhost:3000/~/search?query=hello&repos=github.com%2Fsourcebot-dev%2Fsourcebot&langs=JSON%2CTSX#166
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Style