Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/curly-sloths-end.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"react-router": patch
---

Pass a copy of `searchParams` to the `setSearchParams` callback function to avoid muations of the internal `searchParams` instance. This was an issue when navigations were blocked because the internal instance be out of sync with `useLocation().search`.
1 change: 1 addition & 0 deletions contributors.yml
Original file line number Diff line number Diff line change
Expand Up @@ -403,6 +403,7 @@
- yionr
- yracnet
- ytori
- yuhwan-park
- yuleicul
- zeevick10
- zeromask1337
Expand Down
114 changes: 113 additions & 1 deletion packages/react-router/__tests__/dom/search-params-test.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,16 @@
import * as React from "react";
import * as ReactDOM from "react-dom/client";
import { act } from "@testing-library/react";
import { MemoryRouter, Routes, Route, useSearchParams } from "../../index";
import {
MemoryRouter,
Routes,
Route,
useSearchParams,
createBrowserRouter,
useBlocker,
RouterProvider,
useLocation,
} from "../../index";

describe("useSearchParams", () => {
let node: HTMLDivElement;
Expand Down Expand Up @@ -182,4 +191,107 @@ describe("useSearchParams", () => {
`"<p>value=initial&amp;a=1&amp;b=2</p>"`
);
});

it("does not reflect functional update mutation when navigation is blocked", () => {
let router = createBrowserRouter([
{
path: "/",
Component() {
let location = useLocation();
let [searchParams, setSearchParams] = useSearchParams();
let [shouldBlock, setShouldBlock] = React.useState(false);
let b = useBlocker(shouldBlock);
return (
<>
<pre id="output">
{`location.search=${location.search}`}
{`searchParams=${searchParams.toString()}`}
{`blocked=${b.state}`}
</pre>
<button
id="toggle-blocking"
onClick={() => setShouldBlock(!shouldBlock)}
>
Toggle Blocking
</button>
<button
id="navigate1"
onClick={() => {
setSearchParams((prev) => {
prev.set("foo", "bar");
return prev;
});
}}
>
Navigate 1
</button>
<button
id="navigate2"
onClick={() => {
setSearchParams((prev) => {
prev.set("foo", "baz");
return prev;
});
}}
>
Navigate 2
</button>
</>
);
},
},
]);

act(() => {
ReactDOM.createRoot(node).render(<RouterProvider router={router} />);
});

expect(node.querySelector("#output")).toMatchInlineSnapshot(`
<pre
id="output"
>
location.search=
searchParams=
blocked=unblocked
</pre>
`);

act(() => {
node
.querySelector("#navigate1")!
.dispatchEvent(new Event("click", { bubbles: true }));
});

expect(node.querySelector("#output")).toMatchInlineSnapshot(`
<pre
id="output"
>
location.search=?foo=bar
searchParams=foo=bar
blocked=unblocked
</pre>
`);

act(() => {
node
.querySelector("#toggle-blocking")!
.dispatchEvent(new Event("click", { bubbles: true }));
});

act(() => {
node
.querySelector("#navigate2")!
.dispatchEvent(new Event("click", { bubbles: true }));
});

expect(node.querySelector("#output")).toMatchInlineSnapshot(`
<pre
id="output"
>
location.search=?foo=bar
searchParams=foo=bar
blocked=blocked
</pre>
`);
});
});
4 changes: 3 additions & 1 deletion packages/react-router/lib/dom/lib.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -1424,7 +1424,9 @@ export function useSearchParams(
let setSearchParams = React.useCallback<SetURLSearchParams>(
(nextInit, navigateOptions) => {
const newSearchParams = createSearchParams(
typeof nextInit === "function" ? nextInit(searchParams) : nextInit
typeof nextInit === "function"
? nextInit(new URLSearchParams(searchParams))
: nextInit
Comment on lines +1427 to +1429
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if we want to add the URLSearchParams call here or in getSearchParamsForLocation 🤔

CC/ @brophdawg11

Copy link
Contributor Author

@yuhwan-park yuhwan-park Apr 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The getSearchParamsForLocation function already returns a new URLSearchParams object each time.
Here's what's happening in the flow that causes the bug:

  1. setSearchParams executes (this is where searchParams changes)
setSearchParams((prev) => {
    prev.set("mode", "edit");
    return prev;
  })
  1. router.navigate() runs
  2. It gets blocked by useBlocker
  3. Even though the location.pathname hasn't changed, the searchParams returned by useSearchParams remains in the modified state

Because of this flow, I think we need to create a deep copy of the URLSearchParams before passing it to the nextInit function - regardless of how nextInit is implemented - to ensure it behaves as intended.

);
hasSetSearchParamsRef.current = true;
navigate("?" + newSearchParams, navigateOptions);
Expand Down