test: added (currently failing) unit test to reproduce regression introduced in 1.123.2 #4726
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| const router = createRouter({ routeTree }) | ||
|
|
||
| render(<RouterProvider router={router} />) | ||
| await act(async () => { |
There was a problem hiding this comment.
replace
await act(async () => {
/* no-op */
})by
await router.load()There was a problem hiding this comment.
this applies to all occurences
There was a problem hiding this comment.
for some reason when I change it to await router.load() for the index route, the test fails there – it doesn't render the navbar as it did with the act call.
when I use await router.load() for the dashboard route part, it doesn't render anything at all:
| await act(async () => { | ||
| /* no-op */ | ||
| }) | ||
| expect(screen.getByRole('heading', { name: 'Index' })).toBeInTheDocument() |
There was a problem hiding this comment.
please use getByTestId and use data-testid="..."
we try not to use getByRole in new tests anymore
There was a problem hiding this comment.
got it – why's that? (just curious)
There was a problem hiding this comment.
it allows to change text, tags etc without having to update the getByRole calls
you just need to make sure the test-id is correctly specified
|
View your CI Pipeline Execution ↗ for commit d9b8213
☁️ Nx Cloud last updated this comment at |
More templates
@tanstack/arktype-adapter
@tanstack/directive-functions-plugin
@tanstack/eslint-plugin-router
@tanstack/history
@tanstack/react-router
@tanstack/react-router-devtools
@tanstack/react-router-ssr-query
@tanstack/react-start
@tanstack/react-start-client
@tanstack/react-start-plugin
@tanstack/react-start-server
@tanstack/router-cli
@tanstack/router-core
@tanstack/router-devtools
@tanstack/router-devtools-core
@tanstack/router-generator
@tanstack/router-plugin
@tanstack/router-ssr-query-core
@tanstack/router-utils
@tanstack/router-vite-plugin
@tanstack/server-functions-plugin
@tanstack/solid-router
@tanstack/solid-router-devtools
@tanstack/solid-start
@tanstack/solid-start-client
@tanstack/solid-start-plugin
@tanstack/solid-start-server
@tanstack/start-client-core
@tanstack/start-plugin-core
@tanstack/start-server-core
@tanstack/start-server-functions-client
@tanstack/start-server-functions-fetcher
@tanstack/start-server-functions-server
@tanstack/start-storage-context
@tanstack/valibot-adapter
@tanstack/virtual-file-routes
@tanstack/zod-adapter
commit: |
|
@schiller-manuel Is it possible for me to re-run the CI for this PR? It's possible the bug has been fixed |
WalkthroughAdds a new test in loaders.test.tsx reproducing auth-aware navigation rendering with beforeLoad, loader, Suspense, and memory-history navigation. Also exposes createMemoryHistory from the react-router src for use in tests, enabling initialEntries-based routing scenarios. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor T as Test Runner
participant RR as Router
participant H as MemoryHistory
participant R as Root Route
participant D as Dashboard Route
T->>RR: createRouter(routeTree)
T->>H: createMemoryHistory({ initialEntries })
T->>RR: <RouterProvider history=H />
activate RR
RR->>R: beforeLoad(ctx)
R-->>RR: ctx' { isAuthenticated, isAdmin }
RR->>R: loader(ctx')
R-->>RR: loaderData(ctx')
RR-->>T: render Root (nav if authenticated)
T->>H: navigate('/dashboard?page=0')
RR->>D: validateSearch()
D-->>RR: { page: 0 }
RR-->>T: render Dashboard (with nav)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/react-router/tests/loaders.test.tsx (1)
19-24: ExportcreateMemoryHistoryor update tests to import it directly
Tests inpackages/react-router/tests/loaders.test.tsximportcreateMemoryHistoryfrom../src, but that symbol isn’t currently re-exported inpackages/react-router/src/index.ts. Either:
- Re-export it in the public API (e.g.
export { createMemoryHistory } from '@remix-run/router'),
or- Change the tests to import
createMemoryHistorydirectly from its source package (history/@remix-run/router).
♻️ Duplicate comments (1)
packages/react-router/tests/loaders.test.tsx (1)
791-795: Replace no-opactwith an awaited router load.No-op
actcan race withwrapInSuspense/loader resolution. Await the router’s load to stabilize the DOM before asserting.- await act(async () => {}) - expect(screen.getByTestId('heading-index')).toBeInTheDocument() - expect(screen.getByTestId('nav')).toBeInTheDocument() + await act(() => router.latestLoadPromise) + expect(await screen.findByTestId('heading-index')).toBeInTheDocument() + expect(await screen.findByTestId('nav')).toBeInTheDocument()
🧹 Nitpick comments (1)
packages/react-router/tests/loaders.test.tsx (1)
796-807: Preferrender().unmount()over mid-testcleanup()and destroy histories.Mid-test
cleanup()is heavy-handed and can interact oddly with globalafterEach(cleanup). Unmount the specific render and, if you keep creating explicit histories, callhistory.destroy()to avoid leaks.Example pattern:
const { unmount } = render(<RouterProvider router={router2} />) await act(() => router2.latestLoadPromise) unmount() // optionally: router2.options.history?.destroy?.()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/react-router/tests/loaders.test.tsx(2 hunks)
🔇 Additional comments (1)
packages/react-router/tests/loaders.test.tsx (1)
734-747: Repro scaffolding looks good.Clear use of
beforeLoad,loader, and Suspense to gate the navbar on auth. No changes requested.
Comment on lines
+798
to
+805
| const historyWithSearchParam = createMemoryHistory({ | ||
| initialEntries: ['/dashboard?page=0'], | ||
| }) | ||
| render(<RouterProvider history={historyWithSearchParam} router={router} />) | ||
| await act(async () => {}) | ||
| expect(screen.getByTestId('heading-dashboard')).toBeInTheDocument() | ||
| expect(screen.getByTestId('nav')).toBeInTheDocument() | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t reuse the same router while swapping histories via RouterProvider; create a new router per history.
Passing history into RouterProvider while keeping the same router instance can desynchronize the router’s internal history from the Provider and cause loaders/Suspense not to resolve as expected. Build a fresh router with the target memory history for each scenario and await latestLoadPromise.
- const historyWithSearchParam = createMemoryHistory({
- initialEntries: ['/dashboard?page=0'],
- })
- render(<RouterProvider history={historyWithSearchParam} router={router} />)
- await act(async () => {})
- expect(screen.getByTestId('heading-dashboard')).toBeInTheDocument()
- expect(screen.getByTestId('nav')).toBeInTheDocument()
+ const router2 = createRouter({
+ routeTree,
+ history: createMemoryHistory({ initialEntries: ['/dashboard?page=0'] }),
+ })
+ render(<RouterProvider router={router2} />)
+ await act(() => router2.latestLoadPromise)
+ expect(await screen.findByTestId('heading-dashboard')).toBeInTheDocument()
+ expect(await screen.findByTestId('nav')).toBeInTheDocument()📝 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.
Suggested change
| const historyWithSearchParam = createMemoryHistory({ | |
| initialEntries: ['/dashboard?page=0'], | |
| }) | |
| render(<RouterProvider history={historyWithSearchParam} router={router} />) | |
| await act(async () => {}) | |
| expect(screen.getByTestId('heading-dashboard')).toBeInTheDocument() | |
| expect(screen.getByTestId('nav')).toBeInTheDocument() | |
| const router2 = createRouter({ | |
| routeTree, | |
| history: createMemoryHistory({ initialEntries: ['/dashboard?page=0'] }), | |
| }) | |
| render(<RouterProvider router={router2} />) | |
| await act(() => router2.latestLoadPromise) | |
| expect(await screen.findByTestId('heading-dashboard')).toBeInTheDocument() | |
| expect(await screen.findByTestId('nav')).toBeInTheDocument() |
🤖 Prompt for AI Agents
In packages/react-router/tests/loaders.test.tsx around lines 798 to 805, the
test reuses the same router instance while swapping in a new memory history via
RouterProvider which can desynchronize the router's internal history and cause
loaders/Suspense to not resolve; instead construct a fresh router for each
history scenario (createMemoryHistory with the desired initialEntries, build a
new router using that history), render RouterProvider with that new router, and
after rendering await the router's latestLoadPromise (or equivalent promise that
indicates loading finished) before asserting that elements like
heading-dashboard and nav are in the document.
Comment on lines
+806
to
+816
| cleanup() | ||
|
|
||
| const historyWithoutSearchParam = createMemoryHistory({ | ||
| initialEntries: ['/dashboard'], | ||
| }) | ||
| render(<RouterProvider history={historyWithoutSearchParam} router={router} />) | ||
| await act(async () => {}) | ||
| expect(screen.getByTestId('heading-dashboard')).toBeInTheDocument() | ||
| // Fails here! | ||
| expect(screen.getByTestId('nav')).toBeInTheDocument() | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Apply the same fix for the no-search-param case.
Create a third router with /dashboard as the initial entry and await load before asserting. This addresses the current failure.
- const historyWithoutSearchParam = createMemoryHistory({
- initialEntries: ['/dashboard'],
- })
- render(<RouterProvider history={historyWithoutSearchParam} router={router} />)
- await act(async () => {})
- expect(screen.getByTestId('heading-dashboard')).toBeInTheDocument()
- // Fails here!
- expect(screen.getByTestId('nav')).toBeInTheDocument()
+ const router3 = createRouter({
+ routeTree,
+ history: createMemoryHistory({ initialEntries: ['/dashboard'] }),
+ })
+ render(<RouterProvider router={router3} />)
+ await act(() => router3.latestLoadPromise)
+ expect(await screen.findByTestId('heading-dashboard')).toBeInTheDocument()
+ expect(await screen.findByTestId('nav')).toBeInTheDocument()📝 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.
Suggested change
| cleanup() | |
| const historyWithoutSearchParam = createMemoryHistory({ | |
| initialEntries: ['/dashboard'], | |
| }) | |
| render(<RouterProvider history={historyWithoutSearchParam} router={router} />) | |
| await act(async () => {}) | |
| expect(screen.getByTestId('heading-dashboard')).toBeInTheDocument() | |
| // Fails here! | |
| expect(screen.getByTestId('nav')).toBeInTheDocument() | |
| }) | |
| cleanup() | |
| const router3 = createRouter({ | |
| routeTree, | |
| history: createMemoryHistory({ initialEntries: ['/dashboard'] }), | |
| }) | |
| render(<RouterProvider router={router3} />) | |
| await act(() => router3.latestLoadPromise) | |
| expect(await screen.findByTestId('heading-dashboard')).toBeInTheDocument() | |
| expect(await screen.findByTestId('nav')).toBeInTheDocument() | |
| }) |
🤖 Prompt for AI Agents
In packages/react-router/tests/loaders.test.tsx around lines 806 to 816, the
"no-search-param" test renders RouterProvider with a history whose initial entry
is '/dashboard' but does not await the router load, causing the nav assertion to
fail; fix by creating a dedicated router instance for the no-search-param case
using '/dashboard' as the initial entry (i.e., a third router), render
RouterProvider with that router, then await the router/load to finish (use act
or await the navigation promise) before asserting that the heading and nav
elements are in the document.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As requested by @schiller-manuel for this reported issue: #4696
Summary by CodeRabbit