Repository navigation
QueryParamTabs: don't show tab until loader is done - #3430
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // Copy rather than mutate: the hook's object is what we read `value` from, | ||
| // so mutating it switches the tab on the next render, before the navigation | ||
| // (and the loader it triggers) has committed. | ||
| const nextParams = new URLSearchParams(searchParams) | ||
| if (newValue === props.defaultValue) { | ||
| searchParams.delete('tab') | ||
| nextParams.delete('tab') | ||
| } else { | ||
| searchParams.set('tab', String(newValue)) | ||
| nextParams.set('tab', String(newValue)) | ||
| } | ||
| setSearchParams(searchParams, { replace: true }) | ||
| setSearchParams(nextParams, { replace: true }) |
There was a problem hiding this comment.
🟡 (optional) On the four existing tab pages, switching tabs now waits on network refetches, where before it switched instantly. Changing ?tab= revalidates every matched clientLoader, and the default staleTime: 2000 makes their prefetches refetch. IpPoolPage alone refetches 4-5 endpoints. The tab now waits for those because value no longer reads the mutated params. Fix: tab-only changes on these pages should not wait on a refetch. Export a shouldRevalidate from IpPoolPage, SubnetPoolPage, AlertReceiverPage and UtilizationPage that returns false when only tab changed. Keep revalidation for path changes and other params.
Why this was flagged
A user stays on /system/networking/ip-pools/:pool for more than 2s, then clicks the "Members"/"Linked silos" tab. That calls onValueChange, and setSearchParams(nextParams) starts a navigation. React Router revalidates by default when the search string changes. No route here exports shouldRevalidate (only AuthenticatedLayout does, and it returns true). So IpPoolPage clientLoader (IpPoolPage.tsx:83) runs again. With queryClient staleTime: 2000 (app/api/client.ts:74), it refetches ipPoolView, ipPoolSiloList, ipPoolRangeList, ipPoolUtilizationView and siloList. On the base branch, the mutated memoized params let the nav-state rerender show the new tab at once while the loader ran in the background. With the copy at QueryParamTabs.tsx:26, value stays on the old tab until every request resolves. So each tab click on IpPoolPage, SubnetPoolPage, AlertReceiverPage and UtilizationPage lags by a network round trip.
Verification: setSearchParams(nextParams, { replace: true }) (QueryParamTabs.tsx:32) starts a navigation that changes only the search string. The only shouldRevalidate in app/ is AuthenticatedLayout.tsx:16, and it returns true. With staleTime: 2000 (app/api/client.ts:74), the prefetchQuery/fetchQuery calls refetch. Now value changes only when the navigation commits, so every tab click waits for a round trip.
There was a problem hiding this comment.
Correct and a good catch, but this is actually an improvement. It makes sense for these to wait for the loading to finish before loading the tab. On main, if you change tabs outside of the 2 second staleTime, you get an immediate nav plus a loading bar while the loader reruns. Weird experience, would rather have it get the data up to date before flipping.
I hit this while working on Active Problems. A query param tab with a loader gets rendered before its loader is actually done, and if it uses
usePrefetchedQuery, that render triggers an error page which, oddly, goes away after the nav completes. This PR fixes it so the current tab stays on screen until the new tab's data has loaded. None of our existingQueryParamTabspages run into this because their loaders fetch data for every tab up front.2026-10-29-query-param-tabs-bug.mp4
The bug depends on the way RR's
useSearchParamsworks internally. The return value ofuseSearchParamsis memoized based onuseLocation().search, so every render gets the same object back until the location changes. When you click a tab,QueryParamTabscallsset('tab', ...)on that object, changing it in place, and then passes it tosetSearchParams. That starts a navigation: the loader runs for the new URL, and the URL changes once it finishes. But as soon as the navigation starts, React Router rerenders the page to update the navigation state from idle to loading. In that render,useLocation().searchhasn't changed (because the nav hasn't completed, because there's a loader), souseSearchParamsreturns the same memoized object, which now has the newtabvalue in it, which is whatQueryParamTabsuses to decide which tab to render. So the new tab renders immediately instead of waiting for the loader.usePrefetchedQueryerrors, and the error page shows until the navigation finishes.The fix is to pass a copy to
setSearchParamsinstead, which leaves the memoized return value alone.