Conversation
… and NavigateOptions
- Make prefix optional: when undefined, passes through to raw useSearchParams
- Add NavigateOptions support on setter (e.g. { replace: true })
- Add equality checking to avoid unnecessary URL navigations
- Read from window.location.search for freshest URL state
- Extract helper functions for filtering and comparing params
…table URL param updates
- Add optional paramPrefix prop to FormikFilterForm, ConfigChangeTable,
ConfigsTable, FilterByCellValue, TagsFilterCell, ConfigListTagsCell,
MRTConfigListTagsCell, ConfigGroupByDropdown, and ConfigChangesDateRangeFilter
- Convert mrtConfigListColumns and configChangesColumn from constants to
functions accepting paramPrefix
- Replace direct useSearchParams with usePrefixedSearchParams across all
touched components and hooks
- Switch from mutable params.set()/setParams(params) to immutable callback
pattern: setParams(prev => { const next = new URLSearchParams(prev); ... return next })
- Add paramPrefix support to useAllConfigsQuery, useGetAllConfigsChangesQuery,
useConfigChangesArbitraryFilters, and useTimeRangeParams
No callers pass a prefix yet, so runtime behavior is unchanged.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughAdds optional URL search-parameter prefixing across hooks, tables, filters and forms by introducing Changes
Sequence Diagram(s)mermaid Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/hooks/usePrefixedSearchParams.ts`:
- Around line 82-95: The prefixed branch currently returns a fresh
URLSearchParams (baseParams) when the updater makes no effective change, causing
React Router to treat it as a change by reference; update the prefixed path to
perform the same no-op short-circuit as the non-prefixed path: after computing
updated (from baseParams) call areSearchParamsEqual(updated, baseParams) and if
true return currentParams (the original React Router state) instead of
baseParams so reference equality prevents an unnecessary update; ensure this fix
is applied for both prefixed occurrences around the code that uses prefix,
baseParams, updater, updated, and areSearchParamsEqual.
In `@src/ui/Dates/TimeRangePicker/useTimeRangeParams.tsx`:
- Around line 64-68: The code that clears old time range params in
useTimeRangeParams omits deleting the "range" key, leaving a stale param when
switching away from a "relative" type; update the cleanup block that currently
calls nextParams.delete("from"), .delete("to"), .delete("duration"),
.delete("timeRange") to also call nextParams.delete("range") so the "range"
param (which is set when relative is selected) is removed when switching to
absolute or mapped.
🧹 Nitpick comments (9)
src/hooks/usePrefixedSearchParams.ts (2)
120-123: Empty-value params are silently dropped.Line 121 filters out params where
!value || value.trim() === "". If a consumer's updater intentionally sets a param to""(e.g., to clear a value while keeping the key present), that intent will be silently discarded. This may be the desired behavior for URL cleanliness, but it's worth being explicit about in the JSDoc — especially since the non-prefixed path (Line 88) does not apply this filter, so behavior differs depending on whether a prefix is supplied.
82-85: Reading fromwindow.location.searchbypasses React Router state.This is a known workaround for stale closures when
setSearchParamsis called in rapid succession. The SSR fallback (typeof window !== "undefined") is appropriate. Just be aware this could diverge from React Router's internal state if other code modifies the URL outside ofsetSearchParams(e.g.,history.pushState).src/ui/DataTable/FilterByCellValue.tsx (1)
21-22: Unstable default[]forparamsToResetwill invalidateuseCallbackevery render.When no
paramsToResetis provided, the default[]creates a new array reference on each render, causing theonClickcallback to be recreated. Consider hoisting a constant:Proposed fix
+const EMPTY_ARRAY: string[] = []; + export function FilterByCellValue({ paramKey, children, filterValue, - paramsToReset = [], + paramsToReset = EMPTY_ARRAY, paramPrefix }: FilterByCellProps) {Also applies to: 58-58
src/components/Configs/ConfigsListFilters/ConfigGroupByDropdown.tsx (1)
49-55: Same unstable default[]pattern forparamsToReset.Same concern as in
FilterByCellValue.tsx— the default[]on Line 52 creates a new reference each render, invalidating thegroupByChangecallback. Consider hoisting a module-level constant.src/components/Forms/FormikFilterForm.tsx (3)
5-14:useStableStringArray: direct ref mutation during render.Writing to
cacheRef.currentduring render (Lines 9-10) is a side effect during the render phase. While this works in practice (idempotent, ref-based), React 18 Strict Mode in development will double-invoke render, which could cause subtle issues in theory. A safer alternative isuseMemo:Alternative using useMemo
function useStableStringArray(values: string[]) { const signature = values.join("\u001f"); - const cacheRef = useRef<{ signature: string; values: string[] }>(); - - if (!cacheRef.current || cacheRef.current.signature !== signature) { - cacheRef.current = { signature, values }; - } - - return cacheRef.current.values; + // eslint-disable-next-line react-hooks/exhaustive-deps + return useMemo(() => values, [signature]); }
42-73: Bidirectional sync: form→URL effect fires on everyvalueschange.This effect depends on
values(Line 74), so it runs on every form field change. Thechangedflag guard (Line 68) prevents unnecessary URL updates, which is good. However,setSearchParamsis still invoked each time (even if returningcurrentParams), which may trigger React Router's internal diffing. Consider debouncing for high-frequency inputs, or at minimum, moving the equality check before callingsetSearchParams:Proposed optimization — bail out before calling setSearchParams
You could pre-check whether any field differs from the current
searchParamsbefore invokingsetSearchParamsat all, avoiding the overhead of React Router's updater machinery:// Sync form values to URL params useEffect(() => { + // Quick check: bail out if no field differs from current URL params + const needsUpdate = filterFields.some((field) => { + const value = values[field]; + const currentValue = searchParams.get(field); + if (value && value.toLowerCase() !== "all") { + return currentValue !== value; + } + return currentValue !== null; + }); + if (!needsUpdate) return; + setSearchParams((currentParams) => {
111-147:initialValuesrecomputation is harmless but unnecessary after mount.
initialValues(Line 122) is recomputed wheneversearchParamschanges, but<Formik>only usesinitialValueson mount (noenableReinitialize). The ongoing URL→form sync is handled byFormikChangesListener. This is not a bug — just a slight inefficiency.src/ui/Dates/TimeRangePicker/useTimeRangeParams.tsx (1)
28-55: Defaults sync effect ignores theprevargument — potential for stale writes.On line 54,
setParams(() => nextParams)ignores theprevparameter supplied byusePrefixedSearchParams. ThenextParamswas built fromparamscaptured at effect-run time. If the URL changes between the effect scheduling and the updater executing, those intervening changes could be overwritten.Consider using the functional form properly:
Proposed fix
- const nextParams = new URLSearchParams(params); - - const defaultsParams = new URLSearchParams( - typeof defaults === "string" || defaults instanceof URLSearchParams - ? defaults - : Object.entries(defaults).flatMap(([key, value]) => - Array.isArray(value) ? value.map((v) => [key, v]) : [[key, value]] - ) - ); - defaultsParams.forEach((value, key) => { - if (!nextParams.has(key) && value != null) { - nextParams.set(key, String(value)); - changed = true; - } - }); - - if (!changed) { - return; - } - - setParams(() => nextParams); + const defaultsParams = new URLSearchParams( + typeof defaults === "string" || defaults instanceof URLSearchParams + ? defaults + : Object.entries(defaults).flatMap(([key, value]) => + Array.isArray(value) ? value.map((v) => [key, v]) : [[key, value]] + ) + ); + + // Check if any defaults are missing from current params + let changed = false; + defaultsParams.forEach((value, key) => { + if (!params.has(key) && value != null) { + changed = true; + } + }); + + if (!changed) { + return; + } + + setParams((prev) => { + const nextParams = new URLSearchParams(prev); + defaultsParams.forEach((value, key) => { + if (!nextParams.has(key) && value != null) { + nextParams.set(key, String(value)); + } + }); + return nextParams; + });src/api/query-hooks/useAllConfigsQuery.ts (1)
14-19: Loose index signature weakens type safety for query options.The
[key: string]: anyindex signature allows arbitrary untyped properties to pass through. Consider using a more specific type that extendsUseQueryOptions(or a picked subset) to preserve type safety for forwarded options, similar to whatuseGetAllConfigsChangesQuerydoes withUseQueryOptions<CatalogChangesSearchResponse>.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/hooks/usePrefixedSearchParams.ts`:
- Around line 171-182: The loop over updatedFiltered currently skips entries
with empty/whitespace values only in the prefixed branch, causing asymmetry with
the non-prefixed path; update the logic so empty values are handled
consistently: either remove the initial check (!value || value.trim() === "") so
both branches always set nextParams (use prefixedKey or raw key as currently
done), or move/apply the same empty-value guard to the non-prefixed branch that
uses GLOBAL_PARAM_KEYS and nextParams.set so both paths filter identically;
modify the block that references updatedFiltered, GLOBAL_PARAM_KEYS,
prefixWithSeparator and nextParams accordingly to ensure consistent behavior.
In `@src/ui/Dates/TimeRangePicker/useTimeRangeParams.tsx`:
- Around line 28-55: The effect in useTimeRangeParams re-runs on every URL
change because params is in the dependency array; change the logic so defaults
are applied only when defaults change by removing params from the useEffect deps
and performing the merge inside the setParams updater (setParams(prev => { ...
})) to read the current params safely; inside the updater compute a new
URLSearchParams from prev, iterate over defaults (handle
string/URLSearchParams/object the same way currently done), only set missing
keys and return prev if nothing changed, and keep defaults and setParams in the
effect deps so the effect runs only when defaults change.
🧹 Nitpick comments (5)
src/hooks/usePrefixedSearchParams.ts (1)
61-85:filterPrefixedParamsuses.set()which silently drops duplicate keys.If a prefixed
URLSearchParamsever contains multiple values for the same unprefixed key (e.g.,view__tag=a&view__tag=b),.set()on Line 80 will keep only the last value. If multi-value params are needed, use.append()instead. If multi-value is intentionally unsupported, a brief comment would help future readers.src/ui/Dates/TimeRangePicker/useTimeRangeParams.tsx (2)
36-42: Minor DRY concern: default-parsing logic duplicatestoURLSearchParamsfromusePrefixedSearchParams.ts.The conversion from
URLSearchParamsInittoURLSearchParams(Lines 36-42) replicates the same logic as thetoURLSearchParamsutility inusePrefixedSearchParams.ts. Consider importing and reusing it, or exporting it as a shared utility.
105-113: Redundant optional chaining and non-null assertion onrange.Line 111:
range?.toString()!—rangeis already confirmed truthy on Line 107, making both?.and!unnecessary.Proposed fix
- range: range?.toString()! + range: range.toString()src/components/Forms/FormikFilterForm.tsx (2)
44-77: Hardcoded"all"sentinel in a generic sync component.Line 53 treats
value.toLowerCase() !== "all"as "no value," which embeds domain-specific filter semantics into what is otherwise a reusable form↔URL sync mechanism. If another consumer passes a legitimate"all"value, it will be silently dropped from the URL.Consider extracting this check into a configurable predicate or documenting the assumption.
126-137:initialValuesmemo recomputes on URL changes but only matters at mount.
searchParamsin the dependency array (Line 136) causesinitialValuesto recompute on every URL change, but sinceFormikdoesn't haveenableReinitialize, only the first value is used. This is harmless but slightly wasteful.
adityathebe
force-pushed
the
refactor/prefixed-search-params
branch
from
e8e7e5a to
129a3d8
Compare
|
Review the changes in this branch. I'm expecting a fully backward compatible behavior. However, the app should behave exactly likebefore with no bheavioral change. |
|
Key aspects I'll examine:
I'll provide a comprehensive review of the implementation to confirm it's fully backward compatible and ready for the next phase of work related to issue ✅ Actions performedReview triggered.
|
1 similar comment
|
Key aspects I'll examine:
I'll provide a comprehensive review of the implementation to confirm it's fully backward compatible and ready for the next phase of work related to issue ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/api/query-hooks/useConfigChangesHooks.ts`:
- Around line 55-58: The defaults passed into usePrefixedSearchParams use the
wrong key name: change the property sortDirection to sortOrder in the default
param objects so the hook matches what useReactTableSortState expects
(sortOrderKey = "sortOrder"); update both occurrences where
usePrefixedSearchParams is called with defaults (the initial const [params] call
and the later call around the other default object at lines 104–109) so the
default sort uses sortOrder instead of sortDirection.
🧹 Nitpick comments (4)
src/api/query-hooks/useAllConfigsQuery.ts (2)
14-19: Loose index signature[key: string]: anyweakens type safety.This allows any arbitrary prop to be passed without type-checking, which can mask typos (e.g.,
staleTImeinstead ofstaleTime). Consider using a more precise type or an explicit intersection with the react-query options you intend to forward.
20-24: Inline defaults object is recreated every render, defeating theuseMemoinsideusePrefixedSearchParams.The
defaultsparameter is a dependency of the internaluseMemothat computesdefaultSearchParams. A new object reference each render causes that memo to recompute unnecessarily. Hoist it to module scope or wrap it inuseMemo.♻️ Proposed fix: hoist defaults to module scope
+const DEFAULT_SEARCH_PARAMS = { + sortBy: "type", + sortOrder: "asc", + groupBy: "type" +}; + export const useAllConfigsQuery = ({ ... }) => { - const [searchParams] = usePrefixedSearchParams(paramPrefix, false, { - sortBy: "type", - sortOrder: "asc", - groupBy: "type" - }); + const [searchParams] = usePrefixedSearchParams(paramPrefix, false, DEFAULT_SEARCH_PARAMS);src/components/Forms/FormikFilterForm.tsx (2)
62-66:valuesRefupdate runs as a separate effect — consider direct assignment during render.The
valuesRefis updated in its ownuseEffect, which means it lags one commit behind within the same render cycle's effect batch. Since this ref is read by the URL→form sync effect (Line 108), there's a narrow window where it could hold stale data if effect ordering shifts. Assigning during render (outside an effect) is the standard React pattern for keeping a ref in sync with a value.Proposed fix
- const valuesRef = useRef(values); - - useEffect(() => { - valuesRef.current = values; - }, [values]); + const valuesRef = useRef(values); + valuesRef.current = values;
146-162:searchParamsinuseMemodeps causesinitialValuesto recompute on every URL change.Since
Formikdoesn't haveenableReinitializeset (Line 165), the recomputedinitialValuesobject is effectively discarded after the first mount. The memo still produces a new object reference on each URL param change, which is harmless but wasteful. If this is intentional (to support a futureenableReinitializetoggle), a brief comment would help. Otherwise, consider droppingsearchParamsfrom the deps or memoizing more aggressively.
…chParams Replace raw useSearchParams with usePrefixedSearchParams in useReactTablePaginationState and useReactTableSortState, eliminating duplicate prefix logic and manual key construction. Use functional updater pattern in setters to avoid stale-state race conditions. Provide sort defaults declaratively via usePrefixedSearchParams instead of imperatively writing them to the URL in a useEffect.
1 participant
This PR replaces
useSearchParamswithusePrefixedSearchParamsto prepare for the changes required for #2842Summary by CodeRabbit
New Features
Refactor