fix(ui/dashboard): experiment filter search params - #2123
Conversation
steveninhle
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR fixes search parameter handling for experiment filters in the dashboard UI, addressing issues with filter state management and URL synchronization.
- Updates
useSearchParamshook dependency array to includenavigateandlocationfor proper re-computation - Refactors experiment filter handling to improve state consistency and remove redundant optional chaining
- Simplifies filter update logic in
onChangeFilterscallback
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| ui/dashboard/src/utils/search-params.ts | Updates useMemo dependencies to fix search params reactivity |
| ui/dashboard/src/pages/experiments/page-content.tsx | Refactors filter handling logic and removes unnecessary optional chaining |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| const searchOptions = useMemo<SearchParams>((): SearchParams => { | ||
| return queryString.parse(location.search); | ||
| }, [location.search]); | ||
| }, [navigate, location]); |
There was a problem hiding this comment.
The dependency array includes navigate but this function is typically stable and doesn't need to be a dependency. Including it may cause unnecessary re-computations. Consider removing navigate from the dependencies and keeping only location.search to maintain the original intent of re-parsing when the search string changes.
| }, [navigate, location]); | |
| }, [location.search]); |
| const merged = { ...filters, ...values, page: nextPage }; | ||
| const options = pickBy(merged, v => isNotEmpty(v)); | ||
| if (isChangeParams) onChangSearchParams(options); | ||
| setFilters(merged); |
There was a problem hiding this comment.
The setFilters call uses merged which includes all filter properties, but the original code used { ...values } which only set the changed values. This change could overwrite filter state that wasn't intended to be modified, potentially causing unexpected behavior when partial updates are made.
| setFilters(merged); | |
| setFilters(values); |
No description provided.