fix(Relative routing): Default relative routing to current location #4978
Conversation
github-actions
bot
added
documentation
WalkthroughCentralizes origin resolution via new useActiveLocation hooks (React & Solid), updates Link and useNavigate to derive from via getFromPath and call router.navigate/router.preloadRoute with memoized options, changes router-core.buildLocation to compute/validate from/to resolution, and expands docs and tests for relative navigation semantics. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as Link / Caller
participant Active as useActiveLocation
participant Router as Router Instance
participant Core as Router Core
participant History as Browser History
UI->>Active: getFromPath(options.from?) -> resolvedFrom
UI->>Router: router.buildLocation({ to, from: resolvedFrom, ... })
Router->>Core: buildLocation(dest)
rect #e8f5e9
Core->>Core: compute fromPath (dest.from ? dest.from : lastMatch.fullPath or pathname)
Core->>Core: resolve nextTo, interpolate params, apply base
end
Core-->>Router: resolved location
UI->>Router: router.navigate(resolved location)
Router->>History: push/replace location
sequenceDiagram
participant Component as useNavigate caller
participant useNav as useNavigate hook
participant Active as useActiveLocation
participant Router as Router
Component->>useNav: invoke navigate(to, opts)
useNav->>Active: getFromPath(opts.from?) -> resolvedFrom
useNav->>Router: router.navigate({ to, from: resolvedFrom, ... })
Router->>Router: buildLocation + navigate -> history update
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Assessment against linked issues
Out-of-scope changes
Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 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 (
|
|
View your CI Pipeline Execution ↗ for commit 508ee37
☁️ 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: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (12)
docs/router/framework/react/guide/navigation.md (1)
217-227: Example equivalence looks correct; consider adding one-liner rationaleThese examples read well and match the new behavior. Consider a brief inline comment clarifying that
from="/posts"withto=".."produces/because/posts’s parent is root in that tree (helps readers reason about other hierarchies).packages/solid-router/src/link.tsx (1)
143-146: matchIndex fallback is fine; consider consolidating from-derivationThe
matchIndex()fallback is reasonable. To avoid duplication with useNavigate (and future drift), consider extracting a small helper (e.g. deriveFrom(options, router, matchIndex)) and sharing it across link/navigate.packages/solid-router/src/useNavigate.tsx (1)
25-37: from-derivation matches the new semantics; consider de-duplicating with LinkThe fallback chain
options.from ?? _defaultOpts?.from ?? last(currentRouteMatches)?.fullPath ?? router.state.matches[matchIndex()]!.fullPathis correct. To reduce duplication and keep parity with Link, consider extracting this into a shared helper.packages/react-router/src/useNavigate.tsx (1)
27-46: Remove ESLint disable for unknown rule or add the pluginCI shows “Definition for rule 'react-hooks/exhaustive-deps' was not found.” The inline disable at Line 45 references a rule that isn’t installed. Either:
- Remove the disable (recommended in this repo context), or
- Ensure eslint-plugin-react-hooks is installed and configured.
Also, given the absence of the rule, the dependency array can be simplified to avoid noise.
Minimal fix:
- // eslint-disable-next-line react-hooks/exhaustive-deps - [_defaultOpts?.from, router.navigate, router.latestLocation], + [_defaultOpts?.from, router.navigate, router.latestLocation],Optionally, if you install the plugin, consider adding
matchIndexto dependencies or keep the current behavior if you intentionally prefer a stable function.packages/react-router/src/link.tsx (4)
107-117: getFrom defaults align with the PR intent; ensure dependent memos re-run when it changesThe fallback chain (options.from → latestLocation’s last match → rendered match by index) is correct for “default to current active location.” However, getFrom is used inside memoized callbacks below but is not part of their dependency arrays. If matchIndex changes (or latestLocation changes without search changes), next/preload may use a stale “from.”
Add getFrom to the dependency arrays of both next and doPreload so they recompute when the resolved “from” origin changes.
- const next = React.useMemo( - () => router.buildLocation({ ...options, from: getFrom() } as any), + const next = React.useMemo( + () => router.buildLocation({ ...options, from: getFrom() } as any), // eslint-disable-next-line react-hooks/exhaustive-deps [ router, currentSearch, options._fromLocation, options.from, options.hash, options.to, options.search, options.params, options.state, options.mask, options.unsafeRelative, + getFrom, ], )And for doPreload:
const doPreload = React.useCallback( () => { router .preloadRoute({ ...options, from: getFrom() } as any) .catch((err) => { console.warn(err) console.warn(preloadWarning) }) }, // eslint-disable-next-line react-hooks/exhaustive-deps [ router, options.to, options._fromLocation, options.from, options.search, options.hash, options.params, options.state, options.mask, options.unsafeRelative, options.hashScrollIntoView, options.href, options.ignoreBlocker, options.reloadDocument, options.replace, options.resetScroll, options.viewTransition, + getFrom, ], )
119-135: Guard router.buildLocation against external URLs to avoid accidental throws/workWith isExternal handled later, next is currently computed unconditionally. If to is an absolute URL, router.buildLocation may parse it as a route path and could error. Even if it doesn’t throw, this is wasted work for external links.
Short-circuit next for external URLs so buildLocation is never called for them.
- const next = React.useMemo( - () => router.buildLocation({ ...options, from: getFrom() } as any), + const next = React.useMemo( + () => { + // Avoid constructing internal locations for external URLs + try { + new URL(to as any) + return null as any + } catch {} + return router.buildLocation({ ...options, from: getFrom() } as any) + }, // eslint-disable-next-line react-hooks/exhaustive-deps [ router, currentSearch, options._fromLocation, options.from, options.hash, options.to, options.search, options.params, options.state, options.mask, options.unsafeRelative, + getFrom, ], )Follow-up: This relies on the later early-return for external links to avoid dereferencing next. Please verify no code path attempts to read next.* when type === 'external'. The isActive selector already returns early for external, and the external branch returns before the final props object is built, so this should be safe.
195-203: Preload “from” propagation is correct; add getFrom to dependenciesPassing from: getFrom() to preloadRoute matches navigation behavior. Include getFrom in the dependency array so the callback refreshes when the “from” origin changes.
const doPreload = React.useCallback( () => { router .preloadRoute({ ...options, from: getFrom() } as any) .catch((err) => { console.warn(err) console.warn(preloadWarning) }) }, // eslint-disable-next-line react-hooks/exhaustive-deps [ router, options.to, options._fromLocation, options.from, options.search, options.hash, options.params, options.state, options.mask, options.unsafeRelative, options.hashScrollIntoView, options.href, options.ignoreBlocker, options.reloadDocument, options.replace, options.resetScroll, options.viewTransition, + getFrom, ], )
287-304: External link early-return placement is okay; consider moving it earlier to avoid unnecessary workWe now allocate handlers, compute next, isActive, etc., before bailing out for external links. This is functionally fine but prevents an easy exit path for external anchors. If we want to minimize work, consider returning external props earlier (before next is computed).
If you’d like, I can draft a refactor that hoists the external branch above next while keeping typing intact.
packages/solid-router/tests/link.test.tsx (1)
5140-5167: Fix testId typos to assert the intended elementsThese assertions use an extra leading quote in the testIds, which makes them always pass even if the elements are present. Drop the stray quote to assert the real elements.
- expect( - screen.queryByTestId("'post-info-heading"), - ).not.toBeInTheDocument() + expect( + screen.queryByTestId('post-info-heading'), + ).not.toBeInTheDocument() ... - expect( - screen.queryByTestId("'post-notes-heading"), - ).not.toBeInTheDocument() + expect( + screen.queryByTestId('post-notes-heading'), + ).not.toBeInTheDocument() ... - expect( - screen.queryByTestId("'post-detail-index-heading"), - ).not.toBeInTheDocument() + expect( + screen.queryByTestId('post-detail-index-heading'), + ).not.toBeInTheDocument()packages/react-router/tests/link.test.tsx (1)
5681-5687: Strengthen the assertion by re-enabling the DOM check forpost-id2The pathname assertion is good, but asserting the DOM change ensures the page actually re-rendered to the correct params.
Apply this diff:
- // expect(await screen.findByTestId('post-id2')).toBeInTheDocument() + expect(await screen.findByTestId('post-id2')).toBeInTheDocument()packages/react-router/tests/useNavigate.test.tsx (1)
1626-1650: Optional: avoid referencingpostsRoutebefore declaration in component bodiesWhile this works because the function runs after binding is set, it’s easier to read/less TDZ-prone if you either:
- Move
const postsRoute = createRoute(...)abovePostsComponent, or- Use
getRouteApi('/posts').useNavigate()for parity with other tests.packages/solid-router/tests/useNavigate.test.tsx (1)
29-32: Create/destroy testhistoryconsistentlyYou create a browser history in beforeEach but don’t destroy it in afterEach. This can leak listeners across tests. Either remove the per-test history if unused or destroy it in afterEach.
Update afterEach (outside this hunk) to destroy history:
afterEach(() => { history?.destroy() window.history.replaceState(null, 'root', '/') cleanup() })
📜 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 (10)
docs/router/framework/react/guide/navigation.md(3 hunks)packages/react-router/src/link.tsx(4 hunks)packages/react-router/src/useNavigate.tsx(2 hunks)packages/react-router/tests/link.test.tsx(4 hunks)packages/react-router/tests/useNavigate.test.tsx(4 hunks)packages/router-core/src/router.ts(1 hunks)packages/solid-router/src/link.tsx(1 hunks)packages/solid-router/src/useNavigate.tsx(1 hunks)packages/solid-router/tests/link.test.tsx(4 hunks)packages/solid-router/tests/useNavigate.test.tsx(6 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (8)
packages/solid-router/src/link.tsx (1)
packages/solid-router/src/useMatch.tsx (1)
useMatch(55-96)
packages/react-router/src/useNavigate.tsx (1)
packages/react-router/src/useRouter.tsx (1)
useRouter(6-15)
packages/solid-router/src/useNavigate.tsx (6)
packages/solid-router/src/index.tsx (3)
useRouter(318-318)useMatch(260-260)NavigateOptions(121-121)packages/react-router/src/useRouter.tsx (1)
useRouter(6-15)packages/solid-router/src/useRouter.tsx (1)
useRouter(6-15)packages/react-router/src/useMatch.tsx (1)
useMatch(78-119)packages/solid-router/src/useMatch.tsx (1)
useMatch(55-96)packages/router-core/src/link.ts (1)
NavigateOptions(289-295)
packages/router-core/src/router.ts (2)
packages/router-core/src/index.ts (1)
last(270-270)packages/router-core/src/utils.ts (1)
last(187-189)
packages/react-router/tests/link.test.tsx (2)
packages/react-router/src/index.tsx (4)
Link(154-154)createRoute(262-262)createRouter(280-280)getRouteApi(260-260)packages/react-router/src/router.ts (1)
createRouter(80-82)
packages/react-router/tests/useNavigate.test.tsx (2)
packages/react-router/src/useNavigate.tsx (1)
useNavigate(12-47)packages/react-router/src/router.ts (1)
createRouter(80-82)
packages/solid-router/tests/useNavigate.test.tsx (2)
packages/react-router/src/useNavigate.tsx (1)
useNavigate(12-47)packages/solid-router/src/useNavigate.tsx (1)
useNavigate(12-39)
packages/react-router/src/link.tsx (2)
packages/react-router/src/useMatch.tsx (1)
useMatch(78-119)packages/router-core/src/link.ts (1)
preloadWarning(706-706)
🪛 ESLint
packages/react-router/src/useNavigate.tsx
[error] 44-44: Definition for rule 'react-hooks/exhaustive-deps' was not found.
(react-hooks/exhaustive-deps)
packages/react-router/src/link.tsx
[error] 121-121: Definition for rule 'react-hooks/exhaustive-deps' was not found.
(react-hooks/exhaustive-deps)
[error] 204-204: Definition for rule 'react-hooks/exhaustive-deps' was not found.
(react-hooks/exhaustive-deps)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
🔇 Additional comments (21)
docs/router/framework/react/guide/navigation.md (1)
204-207: Good clarification on “.” and “..” semanticsClear, practical guidance: “.” reloads current (or provided from) and “..” navigates to the parent relative to the current (or provided from). This matches the implementation in router-core’s buildLocation now that
fromdefaults to the active location.packages/router-core/src/router.ts (2)
1446-1457: Defaulting fromPath to the active location is correct and aligns with the PR’s semanticsDeriving
defaultedFromPathfromcurrentLocation.pathnamefor path-relative calls and otherwisedest.from ?? lastMatch.fullPathis spot on. Resolving it throughresolvePathWithBase(..., '.')keeps basepath behavior consistent.
1464-1468: nextTo resolution reads cleanly and preserves basepath handlingUsing
resolvePathWithBase(fromPath, toOrDot)guarantees consistent basepath handling for both providedtoand default “.”. Nicely simplifies the old special-casing aroundto='.'.packages/solid-router/src/useNavigate.tsx (1)
18-19: Switch to using router instance directly is goodUsing
const router = useRouter()and callingrouter.navigatealigns with the broader refactor and keeps the API surface consistent with link logic.packages/react-router/src/useNavigate.tsx (1)
33-41: Using router.navigate and currentRouteMatches is consistent with the new default “from”The updated from-derivation and the switch to
router.navigateare aligned with the core changes. Looks good.packages/react-router/src/link.tsx (1)
102-106: Deriving matchIndex via useMatch looks goodSelecting the nearest match’s index with strict: false is a sensible, low-cost way to reference the current rendered route when computing “from.”
packages/solid-router/tests/link.test.tsx (5)
4594-4880: Great coverage for “relative links to current route” (incl. trailingSlash)This suite validates “to='.'” semantics against changing path/search in nested routes, and across trailingSlash modes. Nicely exercises param changes and preserves intent of defaulting to the active location.
4882-5183: Solid coverage for “relative links to from route”Tests correctly assert resolving relative paths off an explicit from (including param and child/sibling hops) and validate trailingSlash behavior.
5231-5233: Using route.fullPath as from is the right anchorSwitching to from={paramRoute.fullPath} makes the relative link resolution explicit and deterministic.
5252-5254: Likewise, anchoring to paramARoute.fullPath improves determinismThis aligns with the new “from” semantics and removes ambiguity from render-location vs. active-location.
5535-5537: Asserting final pathname after param change LGTMThis confirms param override via a “to='.'” link behaves as expected. Good end-to-end check.
packages/react-router/tests/link.test.tsx (5)
5077-5080: Good: usingfrom={paramRoute.fullPath}for relative link anchoringThis aligns with the new default-from semantics and makes the test unambiguous.
5099-5102: Good: explicitfrom={paramARoute.fullPath}for parent navigationClear and correct, avoids context ambiguity.
5416-5690: Robust coverage for “relative links to current route”Nice job exercising trailingSlash variants, search updates, and nested route transitions via
to=".".
6033-6039: Good: exercising RouteApi-backed LinkUsing
getRouteApi('/_layout/posts').Linkhere validates the new public API nicely.
27-27: Verified public export:getRouteApiis correctly exposedWe confirmed in packages/react-router/src/index.tsx that
getRouteApiis re-exported from./routeas part of the public API surface.packages/react-router/tests/useNavigate.test.tsx (2)
1061-1068: Good: wrapping navigation clicks inactThis removes act warnings and ensures state updates are flushed before assertions.
1369-1465: Solid coverage for “relative navigate to current route”Validates both search-only updates to current route via
to="."and trailingSlash handling. Looks correct.packages/solid-router/tests/useNavigate.test.tsx (3)
1391-1394: Good: passinghistoryintocreateRouterfor deterministic navigationThis makes path assertions deterministic across tests.
2317-2321: Good: usingfrom={paramRoute.fullPath}for relative navigation originMatches the new semantics and keeps origin explicit in tests.
2343-2347: Good: explicitfrom={paramARoute.fullPath}for parent navigationClear intent; reduces ambiguity in relative resolution.
# Conflicts: # packages/router-core/src/router.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
packages/solid-router/tests/useNavigate.test.tsx (2)
959-983: Fix false-positive error test: “Something went wrong!” is always renderedIn DetailsComponent, the error message is rendered unconditionally, so the test passes even if navigate does not throw. Gate the message on the error signal and assert via a test id.
Apply these diffs:
@@ const DetailsComponent = () => { const navigate = useNavigate() const [error, setError] = Solid.createSignal<unknown>() return ( <> <h1>Details!</h1> <button onClick={() => { try { navigate({ from: '/invoices', to: './$invoiceId', params: { invoiceId: 'id1' }, }) } catch (e) { setError(e) } }} > To Invoices </button> - <span>Something went wrong!</span> + {error() && ( + <span data-testid="navigate-error">Something went wrong!</span> + )} </> ) }@@ - expect(await screen.findByText('Something went wrong!')).toBeInTheDocument() + expect(await screen.findByTestId('navigate-error')).toBeInTheDocument()Also applies to: 1045-1058
446-449: Add missing matcher: bare expect(...) does nothingThese assertions don’t call any matcher, so they never assert anything. Append a matcher so failures are caught.
Apply these diffs:
@@ - expect(await screen.findByText('Params: id1')) + expect(await screen.findByText('Params: id1')).toBeInTheDocument()@@ - expect(await screen.findByText('Params: id1')) + expect(await screen.findByText('Params: id1')).toBeInTheDocument()Also applies to: 589-592
🧹 Nitpick comments (7)
packages/solid-router/src/link.tsx (1)
150-164: Consider memoizing computed “from” to avoid repeated matchRoutes callsWe call router.matchRoutes(activeLocation(), …) in multiple sites indirectly through _options(). You can compute “from” once via a Solid memo and reuse it, reducing repeated work and allocations.
Example shape:
+ const fromPath = Solid.createMemo(() => { + const currentRouteMatches = router.matchRoutes(activeLocation(), { + _buildLocation: false, + }) + return ( + options.from ?? + currentRouteMatches.slice(-1)[0]?.fullPath ?? + router.state.matches[matchIndex()]!.fullPath + ) + }) - const _options = () => { - const currentRouteMatches = router.matchRoutes(activeLocation(), { - _buildLocation: false, - }) - const from = - options.from ?? - currentRouteMatches.slice(-1)[0]?.fullPath ?? - router.state.matches[matchIndex()]!.fullPath - return { - ...options, - from, - } - } + const _options = () => ({ ...options, from: fromPath() })packages/solid-router/tests/link.test.tsx (1)
5185-5541: Optional: factor out helpers to reduce duplication in nested route testsThere’s a lot of repeated setup (route trees, history resets, tail logic). Extract small helpers/builders to cut boilerplate and speed up future maintenance.
packages/react-router/src/link.tsx (1)
196-226: Also make preloading reactive to active locationPreloading decisions using from: getFrom() should observe route changes to avoid using a stale origin when path changes without search changes. Extend the dependencies:
const doPreload = React.useCallback( () => { router .preloadRoute({ ...options, from: getFrom() } as any) .catch((err) => { console.warn(err) console.warn(preloadWarning) }) }, // eslint-disable-next-line react-hooks/exhaustive-deps [ router, options.to, options._fromLocation, options.from, options.search, options.hash, options.params, options.state, options.mask, options.unsafeRelative, options.hashScrollIntoView, options.href, options.ignoreBlocker, options.reloadDocument, options.replace, options.resetScroll, options.viewTransition, getFrom, + activeLocation, ], )packages/react-router/tests/useNavigate.test.tsx (1)
1057-1074: Minor: wrap click-then-assert in waitFor to avoid flakinessA few places use await act(fireEvent.click(...)) then immediate assertions. Prefer awaiting a visible DOM change (findBy*) or waitFor around the assertion to avoid timing flakiness.
Example:
- await act(() => fireEvent.click(invoicesButton)) - expect(consoleWarn).toHaveBeenCalledWith( + await act(() => fireEvent.click(invoicesButton)) + await waitFor(() => + expect(consoleWarn).toHaveBeenCalledWith( 'Could not find match for from: /invoices', -) + ), + )packages/solid-router/tests/useNavigate.test.tsx (3)
1154-1157: Eliminate unnecessary setTimeout and add a proper assertionThe extra microtask tick isn’t needed; findByText already waits. Also add a matcher so the assertion is effective.
Apply this diff:
- await new Promise((r) => setTimeout(r, 0)) - - expect(await screen.findByText('Params: id1')) + expect(await screen.findByText('Params: id1')).toBeInTheDocument()
2383-2392: Use the same test history for consistency in this suiteYou create a custom RouterHistory in beforeEach, but setupRouter() omits it, mixing two history mechanisms in the same test process. Pass the test history to createRouter here to avoid subtle interference.
Apply this diff:
return createRouter({ routeTree: rootRoute.addChildren([ indexRoute, aRoute.addChildren([bRoute]), paramRoute.addChildren([paramARoute, paramBRoute]), ]), basepath: basepath === '' ? undefined : basepath, + history, })
2373-2378: Remove redundant button (“functional”) that isn’t usedThis duplicate button has the same onClick as the previous one and isn’t exercised by any test. Removing it reduces noise.
Apply this diff:
- <button - onClick={() => navigate({ to: '..', params: { param: 'bar' } })} - > - Link to Parent with param:bar functional - </button>
📜 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 (9)
docs/router/framework/react/guide/navigation.md(4 hunks)packages/react-router/src/link.tsx(5 hunks)packages/react-router/src/useNavigate.tsx(2 hunks)packages/react-router/tests/link.test.tsx(4 hunks)packages/react-router/tests/useNavigate.test.tsx(4 hunks)packages/router-core/src/router.ts(1 hunks)packages/solid-router/src/link.tsx(1 hunks)packages/solid-router/tests/link.test.tsx(4 hunks)packages/solid-router/tests/useNavigate.test.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/router-core/src/router.ts
- docs/router/framework/react/guide/navigation.md
- packages/react-router/tests/link.test.tsx
🧰 Additional context used
🧬 Code Graph Analysis (5)
packages/solid-router/src/link.tsx (2)
packages/solid-router/src/useMatch.tsx (1)
useMatch(55-96)packages/solid-router/src/useRouterState.tsx (1)
useRouterState(20-36)
packages/react-router/src/link.tsx (2)
packages/react-router/src/useMatch.tsx (1)
useMatch(78-119)packages/router-core/src/link.ts (1)
preloadWarning(706-706)
packages/solid-router/tests/useNavigate.test.tsx (2)
packages/react-router/src/useNavigate.tsx (1)
useNavigate(12-47)packages/solid-router/src/useNavigate.tsx (1)
useNavigate(12-39)
packages/react-router/tests/useNavigate.test.tsx (2)
packages/react-router/src/useNavigate.tsx (1)
useNavigate(12-47)packages/react-router/src/router.ts (1)
createRouter(80-82)
packages/react-router/src/useNavigate.tsx (3)
packages/react-router/src/index.tsx (1)
useRouter(310-310)packages/solid-router/src/index.tsx (1)
useRouter(317-317)packages/react-router/src/useRouter.tsx (1)
useRouter(6-15)
🪛 ESLint
packages/react-router/src/link.tsx
[error] 121-121: Definition for rule 'react-hooks/exhaustive-deps' was not found.
(react-hooks/exhaustive-deps)
[error] 205-205: Definition for rule 'react-hooks/exhaustive-deps' was not found.
(react-hooks/exhaustive-deps)
packages/react-router/src/useNavigate.tsx
[error] 44-44: Definition for rule 'react-hooks/exhaustive-deps' was not found.
(react-hooks/exhaustive-deps)
🔇 Additional comments (7)
packages/solid-router/src/link.tsx (1)
148-165: Reactivity fix for “from” derivation looks solidDeriving “from” from the active location via useRouterState and touching activeLocation() inside next() ensures href/activeness recompute on route changes without relying on search changes. This addresses the stale href risk. Nice.
Also applies to: 166-171
packages/react-router/src/useNavigate.tsx (1)
27-46: Default “from” fallback mirrors core semanticsUsing the last currentRouteMatches entry and falling back to router.state.matches[matchIndex] ensures robust relative resolution. The Navigate component’s effect guard is also clean.
packages/solid-router/tests/link.test.tsx (1)
4594-5183: Great coverage for default-relative-to-current and explicit-from casesThe new suites comprehensively exercise to=".", params churn, trailingSlash modes, and basepath, including “from” targeting. This matches the PR objective and the reported issue.
packages/react-router/src/link.tsx (1)
289-306: External link branch move is fineRelocating the external branch below the click handler definition doesn’t change semantics. The returned object correctly omits internal navigation handlers for external URLs.
packages/react-router/tests/useNavigate.test.tsx (2)
1369-1785: Solid coverage for default-relative-to-current route semanticsThese tests validate to="." resolution, param churn, and trailingSlash variations using useNavigate. They align with the new default-from behavior.
1787-2280: Nice “relative to from route” coverage, including Route APIGood end-to-end coverage for from-based navigation, including Route.useNavigate and getRouteApi(...).useNavigate. This guards against regressions in derivation logic.
packages/solid-router/tests/useNavigate.test.tsx (1)
1323-1754: Great coverage for default-from semantics and trailingSlash handlingThe new suites exercise “to='.'” semantics from both current and explicit from origins across nested routes and with/without trailing slashes. They also validate search updates and param changes. This materially reduces regressions for #4842.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/react-router/src/link.tsx (1)
108-147: Reactive gap fixed: “from”/href/activeness now track active location changesTracking
activeLocationand deriving_optionsfromrouter.matchRoutes(activeLocation, ...)resolves the previously reported staleness when the pathname changed without a search change. Using_optionsfor buildLocation, preload, and navigate keeps everything in sync.
🧹 Nitpick comments (4)
packages/solid-router/src/useNavigate.tsx (2)
31-38: Minor hardening: avoid non-null assertion on matchIndex fallbackIn edge cases (e.g., atypical render trees),
router.state.matches[matchIndex()]could be undefined. This is unlikely, but we can make the fallback bulletproof.Consider this tweak:
- last(currentRouteMatches)?.fullPath ?? - router.state.matches[matchIndex()]!.fullPath, + last(currentRouteMatches)?.fullPath ?? + router.state.matches[matchIndex()]?.fullPath ?? + last(router.state.matches)?.fullPath!,This preserves current behavior but gracefully handles unexpected gaps.
42-58: Align Solid’s Navigate with new default-from semantics by using useNavigateReact’s Navigate now delegates to
useNavigate(which contains the new default-from logic). For parity and future-proofing, Solid’sNavigatecan do the same.Apply:
export function Navigate< TRouter extends AnyRouter = RegisteredRouter, const TFrom extends string = string, const TTo extends string | undefined = undefined, const TMaskFrom extends string = TFrom, const TMaskTo extends string = '', >(props: NavigateOptions<TRouter, TFrom, TTo, TMaskFrom, TMaskTo>): null { - const { navigate } = useRouter() + const navigate = useNavigate() Solid.onMount(() => { navigate({ ...props, }) })packages/react-router/src/link.tsx (1)
127-142: Nit: currentSearch isn’t used in _options; remove from deps
_optionsdoes not referencecurrentSearch, andactiveLocationalready captures search changes. DroppingcurrentSearchreduces needless recomputes.const _options = React.useMemo( () => { const currentRouteMatches = router.matchRoutes(activeLocation, { _buildLocation: false, }) const from = options.from ?? last(currentRouteMatches)?.fullPath ?? router.state.matches[matchIndex]!.fullPath return { ...options, from } }, // eslint-disable-next-line react-hooks/exhaustive-deps [ router, - currentSearch, activeLocation, options._fromLocation, options.from, options.hash, options.to, options.search, options.params, options.state, options.mask, options.unsafeRelative, ], )packages/react-router/src/useNavigate.tsx (1)
46-46: Nit: minimize callback churn by trimming dependenciesIncluding
router.latestLocation(androuter.navigate) forces the callback identity to change whenever those change, which is unnecessary sincerouter.latestLocationis read at call-time. Consider depending only on stable references.- [_defaultOpts?.from, router.navigate, router.latestLocation, matchIndex], + [_defaultOpts?.from, router, matchIndex],This keeps the callback stable across location changes while still observing
_defaultOpts?.fromand the nearest match index.
📜 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 (4)
packages/react-router/src/link.tsx(6 hunks)packages/react-router/src/useNavigate.tsx(3 hunks)packages/solid-router/src/link.tsx(2 hunks)packages/solid-router/src/useNavigate.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/solid-router/src/link.tsx
🧰 Additional context used
🧬 Code Graph Analysis (3)
packages/solid-router/src/useNavigate.tsx (4)
packages/solid-router/src/useRouter.tsx (1)
useRouter(6-15)packages/solid-router/src/useMatch.tsx (1)
useMatch(55-96)packages/router-core/src/link.ts (1)
NavigateOptions(289-295)packages/router-core/src/utils.ts (1)
last(187-189)
packages/react-router/src/link.tsx (4)
packages/react-router/src/useMatch.tsx (1)
useMatch(78-119)packages/react-router/src/useRouterState.tsx (1)
useRouterState(31-59)packages/router-core/src/utils.ts (1)
last(187-189)packages/router-core/src/link.ts (1)
preloadWarning(706-706)
packages/react-router/src/useNavigate.tsx (3)
packages/react-router/src/useRouter.tsx (1)
useRouter(6-15)packages/solid-router/src/useRouter.tsx (1)
useRouter(6-15)packages/router-core/src/utils.ts (1)
last(187-189)
🪛 ESLint
packages/react-router/src/link.tsx
[error] 127-127: Definition for rule 'react-hooks/exhaustive-deps' was not found.
(react-hooks/exhaustive-deps)
packages/react-router/src/useNavigate.tsx
[error] 45-45: Definition for rule 'react-hooks/exhaustive-deps' was not found.
(react-hooks/exhaustive-deps)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
🔇 Additional comments (5)
packages/solid-router/src/useNavigate.tsx (1)
27-38: Default “from” resolution is correct and aligns with PR intentUsing latestLocation’s matches with a fallback to the nearest match index achieves the desired “default to current active location” behavior for relative navigations.
packages/react-router/src/link.tsx (2)
274-291: External anchor early-return reordering is fineReturning the external anchor props after the click handler definition does not alter behavior and keeps external links bypassing internal logic as expected.
207-213: Preload now uses computed _options (good)Preloading the route with
_optionsensures consistent “from” semantics with the rendered href and navigation.packages/react-router/src/useNavigate.tsx (2)
30-39: Correct default “from” resolution using latestLocationDeriving
fromvialast(router.matchRoutes(router.latestLocation))?.fullPathand falling back to the nearest match index matches the PR’s objective and centralizes relative resolution cleanly.
50-74: Navigate delegates to useNavigate to reuse default-from logic (good)This avoids duplicate logic, prevents double-navigation under StrictMode, and keeps behavior consistent with Link/useNavigate.
There was a problem hiding this comment.
Couple of things to note:
I'm not a big fan of how both useLinkProps and useNavigate require separate (yet similar) implementations for how it determines what's the from value. That being said, I don't know if that's something to be touched in this PR or something separately entirely.
The diff was abit hard to follow. Other than the changes to used await act(...) did any of the existing content change.
For example, I noted this in the diff.
- <Link to="..">Link to .. from /param/foo/a</Link>
+ <Link from={paramARoute.fullPath} to="..">
+ Link to .. from /param/foo/a
+ </Link>Are we still testing for <Link to="..">Link to .. from /param/foo/a</Link>? Did I miss this in the diff?
The changes themselves LGTM, however I do think the tests could use some "ironing-out".
| return ( | ||
| <> | ||
| <h1>Posts</h1> | ||
| {linkVia()} |
@SeanCassiere thanks for the review, I'll add an extra test for With this PR not adding the Let me maybe explain in a bit more detail: Currently the Link in the test above is rendered in The current implementation works great, and has great merit, until you want to use a Link rendered in a layout route to apply on the current active route, for example a refresh/reload button or a back button. For example, if I wanted to have a back Link rendered in a layout route, say This became specifically problematic in a few issues for reloading the current route with changing search params, path params or just simply refreshing the current view i.e. We addressed these issues previously, but this fix resulted in In this PR we try and standardise the handling by making relative routes always relative to the current active location and not the route it is rendered in. To make it relative to the route it is rendered in, irrespective of the current active location a The alternative to this PR is to keep relative routing to the rendered route and enable something like Either of these options has its drawbacks, I like option 2 more, it changes the fix we applied previously, but keeps the current status quo for all other relative routing, however I do think that this current PR is easier to understand for most folks and it matches what has been communicated in the docs: |
Agreed there is quite a bit of noise, some of it necessary but overall can be improved. quite a bit of the changes to Link.test.tsx was to bring that up to date with the tests in useNavigate.test.tsx. This has been delegated to the bottom of the test file. This is also true for quite a bit of noise in the solid-router tests as these was not in line with the react-router tests when testing the relative routing. there was however also quite a bit of noise created in useNavigate.test.tsx when I wrapped the existing to "." tests into its own describe to clean it up a bit. I'm reverting that for now and will bring that and the "act" changes into a seperate PR to clean up these tests specifically. |
I made an attempt to strip this into a hook. works quite well in react and got it to work for useLinkProps in solid, but it breaks the history test when applying it on to useNavigate in Solid. my understanding of Solid is lacking to say the least so have not implemented that section. All tests are passing as intended. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/react-router/src/link.tsx (1)
102-130: From-origin is now reactive and centralized via useActiveLocation — resolves stale href/activenessSwitching to getFromPath and threading from through _options/next ties Link reactivity to the active location, fixing the prior gap where path changes without search updates didn’t recompute href/activeness.
🧹 Nitpick comments (6)
packages/solid-router/src/link.tsx (1)
138-141: Remove commented-out legacy path-derivation for clarityThe old useMatch-based “from” block is now superseded by useActiveLocation. Consider deleting it to reduce noise.
- // const from = useMatch({ - // strict: false, - // select: (match) => options.from ?? match.fullPath, - // })packages/solid-router/tests/link.test.tsx (1)
4973-5562: Comprehensive scenarios for “.”/“..” with active vs from origins; consider splitting into focused suitesThe breadth is great, but this file is very large. Consider splitting “relative links to current route” and “relative links to from route” into separate test files to reduce cognitive load and speed up targeted debugging.
packages/react-router/tests/useNavigate.test.tsx (1)
84-84: Consider usingawait act()for React state updates.While the test execution currently works, consider wrapping
fireEvent.click()calls withawait act()to ensure React state updates are properly flushed before assertions. This aligns with the pattern already used in some tests (lines 1855, 1861, 1868).Example refactor:
- fireEvent.click(postsButton) + await act(() => fireEvent.click(postsButton))This would make the tests more robust and consistent with React testing best practices.
Also applies to: 188-188, 195-195, 299-299, 434-434, 582-582, 728-728, 878-878, 1055-1055, 1325-1325, 1454-1454, 1809-1810, 1817-1818
packages/react-router/tests/link.test.tsx (1)
506-506: Consider usingawait act()consistently for navigation events.For consistency with React testing best practices and to match the pattern used elsewhere in the file, consider wrapping all
fireEvent.click()calls withawait act():- await act(() => fireEvent.click(postsLink)) + await act(async () => fireEvent.click(postsLink))This ensures proper React state flushing and makes tests more deterministic.
Also applies to: 568-568, 635-635, 723-723, 743-743, 834-834, 855-855, 1502-1503, 1588-1589, 1599-1600
packages/solid-router/tests/useNavigate.test.tsx (1)
1809-1810: Consider consistent async patterns for Solid.js navigation.While Solid.js doesn't require
act()like React, consider using consistent async patterns withawait waitFor()after navigation events to ensure DOM updates are complete before assertions:fireEvent.click(postButton) - - await waitFor(() => { + await waitFor(() => { expect(router.state.location.pathname).toBe(`/post${tail}`) })This would make the tests more robust and consistent throughout the file.
Also applies to: 1817-1818, 1826-1827, 1943-1944, 1952-1953, 1961-1962
packages/react-router/src/useNavigate.tsx (1)
31-33: Trim dependency churn by depending on a stable primitiveactiveLocationMatch is only used to trigger callback re-creation on location changes. Depending on its string path reduces re-creations caused by referential changes to the match object.
Apply this diff:
- // eslint-disable-next-line react-hooks/exhaustive-deps - [_defaultOpts?.from, router, activeLocationMatch], + // eslint-disable-next-line react-hooks/exhaustive-deps + [_defaultOpts?.from, router, activeLocationMatch?.fullPath],
📜 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 (9)
packages/react-router/src/link.tsx(6 hunks)packages/react-router/src/useActiveLocation.ts(1 hunks)packages/react-router/src/useNavigate.tsx(2 hunks)packages/react-router/tests/link.test.tsx(5 hunks)packages/react-router/tests/useNavigate.test.tsx(6 hunks)packages/solid-router/src/link.tsx(2 hunks)packages/solid-router/src/useActiveLocation.ts(1 hunks)packages/solid-router/tests/link.test.tsx(4 hunks)packages/solid-router/tests/useNavigate.test.tsx(7 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (8)
packages/solid-router/src/useActiveLocation.ts (1)
packages/react-router/src/useActiveLocation.ts (1)
useActiveLocation(12-36)
packages/react-router/src/useActiveLocation.ts (1)
packages/solid-router/src/useActiveLocation.ts (1)
useActiveLocation(14-42)
packages/react-router/src/link.tsx (2)
packages/react-router/src/useActiveLocation.ts (1)
useActiveLocation(12-36)packages/router-core/src/link.ts (1)
preloadWarning(706-706)
packages/solid-router/src/link.tsx (2)
packages/react-router/src/useActiveLocation.ts (1)
useActiveLocation(12-36)packages/solid-router/src/useActiveLocation.ts (1)
useActiveLocation(14-42)
packages/react-router/tests/link.test.tsx (2)
packages/react-router/src/index.tsx (8)
Link(153-153)RouterProvider(289-289)createRootRoute(264-264)Outlet(249-249)createRoute(261-261)createRouter(279-279)useParams(302-302)getRouteApi(259-259)packages/react-router/src/router.ts (1)
createRouter(80-82)
packages/solid-router/tests/useNavigate.test.tsx (2)
packages/react-router/src/useNavigate.tsx (1)
useNavigate(12-34)packages/solid-router/src/useNavigate.tsx (1)
useNavigate(13-40)
packages/react-router/tests/useNavigate.test.tsx (3)
packages/react-router/src/useNavigate.tsx (1)
useNavigate(12-34)packages/solid-router/src/useNavigate.tsx (1)
useNavigate(13-40)packages/react-router/src/router.ts (1)
createRouter(80-82)
packages/react-router/src/useNavigate.tsx (2)
packages/react-router/src/useRouter.tsx (1)
useRouter(6-15)packages/react-router/src/useActiveLocation.ts (1)
useActiveLocation(12-36)
🪛 ESLint
packages/react-router/src/link.tsx
[error] 110-110: Definition for rule 'react-hooks/exhaustive-deps' was not found.
(react-hooks/exhaustive-deps)
packages/react-router/src/useNavigate.tsx
[error] 31-31: Definition for rule 'react-hooks/exhaustive-deps' was not found.
(react-hooks/exhaustive-deps)
🔇 Additional comments (29)
packages/solid-router/src/link.tsx (2)
143-151: Reactivity restored: “from” now tracks the active location via useActiveLocationGood move to derive and inject a reactive from using useActiveLocation(). This removes the stale href/activeness risk that occurred when relying on a non-reactive latestLocation.
153-157: Memo now invalidates on route changesTouching from() inside next’s memo ensures href/activeness recompute on active-location changes, not just search updates. This aligns with the new default-relative-to-active-location semantics.
packages/react-router/src/useActiveLocation.ts (1)
12-36: Solid foundation for active-location origin resolutionThe hook correctly:
- Tracks the active location via useRouterState
- Falls back to the current rendered match
- Exposes a simple getFromPath(from?) API
This provides a clean, shared source of truth for relative origin across Link/useNavigate.
packages/solid-router/src/useActiveLocation.ts (1)
26-39: Correctly memoized activeLocationMatch and from-path Accessor
- activeLocationMatch is derived from the reactive location.
- getFromPath returns an Accessor that updates as location changes.
This matches Solid’s reactivity model and keeps downstream computations (e.g., href) fresh.
packages/react-router/src/link.tsx (4)
189-195: Preload respects the unified _optionsUsing the consolidated _options for preloadRoute keeps behavior consistent with navigation and href building. Console warnings maintain previous DX.
245-245: Navigation uses unified _optionsrouter.navigate spreading _options maintains parity with buildLocation/preload and the new “from” semantics.
256-273: External anchor early-return placement is fineReordering the external branch after handler declarations doesn’t change behavior and keeps the return payload minimal for external links.
110-110: Incorrect —react-hooks/exhaustive-depsIS configured in this repoShort summary: the rule and plugin are present — the disable comment in packages/react-router/src/link.tsx silences an active rule, it is not masking a missing ESLint rule.
Files/evidence:
- package.json (root) includes "eslint-plugin-react-hooks": "^5.1.0"
- packages/react-router/eslint.config.ts — imports
eslint-plugin-react-hooksand sets'react-hooks/exhaustive-deps': 'error'- Several other ESLint configs import/use the plugin and rule (examples): packages/react-start/eslint.config.js, packages/react-start-client/eslint.config.js, packages/react-start-server/eslint.config.js, packages/react-router-devtools/eslint.config.js, packages/router-devtools/eslint.config.js, packages/solid-start/eslint.config.js
- Disable occurrences found:
- packages/react-router/src/link.tsx — line ~110: // eslint-disable-next-line react-hooks/exhaustive-deps
- packages/react-router/src/useNavigate.tsx — line ~31: // eslint-disable-next-line react-hooks/exhaustive-deps
Snippet in question:
// eslint-disable-next-line react-hooks/exhaustive-depsRecommended action: keep the disable comment only if the suppression is intentional; otherwise remove it and fix the hook dependencies so the rule can run.
Likely an incorrect or invalid review comment.
packages/solid-router/tests/link.test.tsx (2)
4639-4641: Tests updated to explicitly set “from” where needed — aligns with new defaultUsing from={paramRoute.fullPath} and from={paramARoute.fullPath} captures the previous rendered-route-relative intent under the new default-to-active-location semantics. Good targeted adjustments.
Also applies to: 4661-4667
4843-4861: Adds coverage for active-location-based parent navigationThis verifies that to=".." resolves against the current active route when no “from” is provided. Matches the PR’s behavioral objective.
packages/react-router/tests/useNavigate.test.tsx (6)
1-41: LGTM! Well-structured test setup.The migration to use
createBrowserHistory()and the proper cleanup in lifecycle hooks ensures deterministic test execution and better isolation between tests.
1507-1508: Good: Top-leveluseNavigate()import used.The use of the public API
useNavigate()instead of route-scoped navigation aligns with the standardized relative routing behavior introduced in this PR.
1622-1623: Correct: useNavigate() from public API instead of route-scoped.This change properly reflects the new standardized behavior where relative navigations default to the current active location.
2316-2319: Good test coverage forfromroute semantics.These explicit
frompath specifications effectively test the PR's central feature: relative navigation from a specific route context. The addition of a variant that navigates from the current active route (data-testid="link-to-previous") provides comprehensive coverage of both behaviors.Also applies to: 2342-2345, 2349-2354
2505-2522: Excellent addition: Test for active location-based navigation.This new test case specifically validates the PR's core change - that relative navigation without an explicit
fromuses the current active location. This is a critical behavioral change that needed explicit test coverage.
1369-1464: Comprehensive test suites for from-route navigation scenarios.The addition of two large end-to-end test suites provides excellent coverage of:
- Multi-level route trees with trailingSlash variations
- Deep nesting scenarios (Layout -> Posts -> PostDetail -> PostInfo/PostNotes)
- Validation of pathname and search parameters across navigations
These tests effectively validate the new from-path resolution logic introduced in the PR.
Also applies to: 1780-2115
packages/react-router/tests/link.test.tsx (5)
27-27: LGTM! Good addition ofgetRouteApito public exports.The inclusion of
getRouteApiin the imports properly exposes this API for testing route-specific Link components, which aligns with the PR's goal of providing explicit from-context options.
5077-5079: Excellent: Explicitfrompath specifications for relative navigation.The changes to use
from={paramRoute.fullPath}andfrom={paramARoute.fullPath}properly test the new explicit from-context behavior. The addition of the "link-to-previous" variant that uses current active location provides comprehensive coverage of both navigation modes.Also applies to: 5099-5104
5291-5310: Great addition: Test validates active location-based navigation.This new test specifically verifies that links without an explicit
fromnavigate relative to the current active location, which is the core behavioral change of this PR.
5440-5998: Comprehensive test coverage for relative navigation scenarios.The new test suites provide excellent coverage of:
- Navigation to current route with "."
- From-route navigation with explicit
frompaths- Nested route structures with varying depths
- Search parameter preservation during navigation
- TrailingSlash and basepath variations
These tests thoroughly validate the PR's changes to relative routing behavior.
6049-6050: Good use ofgetRouteApifor route-scoped navigation.The RouteApi pattern demonstrates the proper way to get route-scoped Link components with a default
fromcontext, providing a clear alternative to the default active-location behavior.Also applies to: 6054-6054
packages/solid-router/tests/useNavigate.test.tsx (4)
16-16: LGTM! Proper browser history setup with public API.The use of
createBrowserHistory()as a public API and proper cleanup in lifecycle hooks ensures consistent test execution. This aligns well with the React Router test patterns.Also applies to: 25-32
1522-1526: Excellent: Comprehensivefrompath testing.The explicit
fromspecifications usingparamRoute.fullPathandparamARoute.fullPath, along with the "link-to-previous" variant for active location navigation, provide thorough coverage of the new from-path resolution behavior.Also applies to: 1548-1551, 1556-1560
1695-1710: Good addition: Test for active location-based navigation.This test validates that navigation without explicit
fromuses the current active location, which is crucial for verifying the PR's core behavior change.
1731-2525: Comprehensive test suites for relative navigation.The new test suites provide excellent coverage with:
- "relative navigate to current route" - Tests navigation with "." maintaining current location
- "relative navigate to from route" - Tests explicit from-context navigation
- Deep nesting scenarios with proper heading and pathname validation
- TrailingSlash variations for completeness
These tests effectively validate the Solid Router implementation of the new relative routing behavior.
packages/react-router/src/useNavigate.tsx (4)
3-3: Centralizing "from" resolution via useActiveLocation looks goodImporting useActiveLocation here aligns useNavigate with the new default-to-active-location semantics and keeps logic consistent with Link.
18-21: Hook composition is soundGrabbing router and useActiveLocation at the hook level is appropriate. This keeps navigation wired to the active location without duplicating matching logic.
31-31: ESLint “react-hooks” plugin is correctly configured for this package
Thereact-hooksplugin is imported and enabled in packages/react-router/eslint.config.ts (via the flat config’spluginsmapping), and the rulesreact-hooks/exhaustive-depsandreact-hooks/rules-of-hooksare explicitly set to “error.” No changes are needed here.Likely an incorrect or invalid review comment.
24-29: Confirmed:fromis strictly a URL path, not a route IDThe
FromPathOption<TRouter, TFrom>alias expands to
ConstrainLiteral<string, RoutePaths<TRouter['routeTree']>>
which resolves exactly to the union of literal full-path strings in your route tree. Route IDs aren’t part ofRoutePaths, so any attempt to pass an ID will be rejected at compile time. No further changes needed.
|
implementations have been standardised. if we are happy with the route this PR is taking then this should be ready for final review. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/react-router/src/link.tsx (1)
102-130: Reactive gap: href/activeness can go stale on path changes without search changesThe
_optionsandnextcomputations don't properly track location pathname changes. WhileactiveLocationis in the dependency array, it won't trigger re-computation becauseuseActiveLocationmaintains its own internal state that doesn't sync immediately with router state changes.The issue is that
activeLocationfromuseActiveLocationis a local state that updates asynchronously viauseEffect. When the router location changes, there's a timing gap beforeactiveLocationupdates, during which_optionsandnextretain stale values.Apply this fix to subscribe directly to router state:
+ // Track the router location directly for immediate updates + const routerLocation = useRouterState({ + select: (s) => s.location, + structuralSharing: true as any, + }) const _options = React.useMemo( () => { return { ...options, from } }, // eslint-disable-next-line react-hooks/exhaustive-deps [ router, - activeLocation, + routerLocation, currentSearch, from, options._fromLocation, options.hash, options.to, options.search, options.params, options.state, options.mask, options.unsafeRelative, ], )
🧹 Nitpick comments (7)
packages/solid-router/src/useActiveLocation.ts (5)
11-20: Don’t recreate useRouterState inside an effect; pass values (not accessors) to setters.You’re instantiating a new router state accessor inside the effect and passing the accessor function to setActiveLocation. While it works, it’s non-idiomatic, harder to read, and can create unnecessary subscriptions. Create a single routerLocation accessor and use its value in both initializers and the effect.
Apply this diff:
const router = useRouter() - const [activeLocation, setActiveLocation] = createSignal<ParsedLocation>( - location ?? useRouterState({ select: (state) => state.location })(), - ) - const [customActiveLocation, _setCustomActiveLocation] = - createSignal<ParsedLocation>( - location ?? useRouterState({ select: (state) => state.location })(), - ) + const routerLocation = useRouterState({ select: (state) => state.location }) + const [activeLocation, setActiveLocation] = createSignal<ParsedLocation>( + location ?? routerLocation(), + ) + const [customActiveLocation, _setCustomActiveLocation] = + createSignal<ParsedLocation>(location ?? routerLocation()) const [useCustomActiveLocation, setUseCustomActiveLocation] = createSignal(!!location) createEffect(() => { - if (!useCustomActiveLocation()) { - setActiveLocation(useRouterState({ select: (state) => state.location })) - } else { - setActiveLocation(customActiveLocation()) - } + setActiveLocation( + useCustomActiveLocation() ? customActiveLocation() : routerLocation(), + ) })Also applies to: 21-27
2-3: Import cleanup: prefer named imports over namespace usage.You’re mixing named imports and Solid namespace usage just to call createMemo. Import createMemo directly and drop the namespace to reduce cognitive load.
Apply this diff:
-import { createEffect, createSignal } from 'solid-js' -import * as Solid from 'solid-js' +import { createEffect, createMemo, createSignal } from 'solid-js'
39-41: Use the named createMemo import (matches the import cleanup).Follows from the previous import change.
Apply this diff:
- const getFromPath = (from?: string) => - Solid.createMemo(() => { + const getFromPath = (from?: string) => + createMemo(() => {
45-49: Guard against undefined match during edge cases.In rare transitional cases there might not be a current match. Consider a defensive optional chain or a clearer fallback to avoid a hard crash.
Possible tweak (if you want a safer fallback):
- currentRouteMatch().fullPath + currentRouteMatch()?.fullPath ?? last(router.state.matches)?.fullPathPlease confirm whether Solid’s useMatch with strict: false guarantees a match in all contexts where this hook is used (e.g., during Router initialization or teardown).
9-57: Overall: Solid parity with React and clear API surface.Nice addition. The active location override + from-path derivation matches the React hook’s behavior and should simplify Link/useNavigate. The small refactors above will improve clarity and resilience.
If you want, I can open a follow-up PR to align the React/Solid implementations side-by-side and add JSDoc indicating that getFromPath returns an Accessor.
packages/react-router/src/useNavigate.tsx (1)
31-33: ESLint configuration issue detectedThe ESLint disable comment references a missing rule definition. This suggests the project's ESLint configuration may be incomplete or outdated.
Consider fixing the ESLint configuration or using the correct rule name:
- // eslint-disable-next-line react-hooks/exhaustive-deps + // eslint-disable-next-line react-hooks/exhaustive-deps [_defaultOpts?.from, router, getFromPath, activeLocation],Also verify that
@eslint-plugin-react-hooksis properly installed and configured in your ESLint setup.packages/react-router/src/link.tsx (1)
257-274: Consider adding navigation handling for external linksExternal links bypass the router's navigation tracking and transition states. Users might expect consistent behavior for navigation events regardless of link type.
Consider emitting navigation events or providing hooks for external navigation:
if (isExternal) { + const handleExternalClick = (e: React.MouseEvent) => { + // Allow users to track external navigation + router.options.onExternalNavigation?.({ href: to, event: e }) + onClick?.(e) + } + return { ...propsSafeToSpread, ref: innerRef as React.ComponentPropsWithRef<'a'>['ref'], type, href: to, ...(children && { children }), ...(target && { target }), ...(disabled && { disabled }), ...(style && { style }), ...(className && { className }), - ...(onClick && { onClick }), + ...(onClick && { onClick: handleExternalClick }), ...(onFocus && { onFocus }), ...(onMouseEnter && { onMouseEnter }), ...(onMouseLeave && { onMouseLeave }), ...(onTouchStart && { onTouchStart }), } }
📜 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 (6)
packages/react-router/src/link.tsx(6 hunks)packages/react-router/src/useActiveLocation.ts(1 hunks)packages/react-router/src/useNavigate.tsx(2 hunks)packages/solid-router/src/link.tsx(2 hunks)packages/solid-router/src/useActiveLocation.ts(1 hunks)packages/solid-router/src/useNavigate.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/solid-router/src/link.tsx
- packages/solid-router/src/useNavigate.tsx
🧰 Additional context used
🧬 Code Graph Analysis (4)
packages/react-router/src/useActiveLocation.ts (3)
packages/solid-router/src/useActiveLocation.ts (1)
useActiveLocation(9-57)packages/history/src/index.ts (1)
location(162-164)packages/router-core/src/router.ts (1)
state(936-938)
packages/react-router/src/link.tsx (2)
packages/react-router/src/useActiveLocation.ts (1)
useActiveLocation(12-58)packages/router-core/src/link.ts (1)
preloadWarning(706-706)
packages/solid-router/src/useActiveLocation.ts (2)
packages/react-router/src/useActiveLocation.ts (1)
useActiveLocation(12-58)packages/router-core/src/router.ts (1)
state(936-938)
packages/react-router/src/useNavigate.tsx (2)
packages/react-router/src/useRouter.tsx (1)
useRouter(6-15)packages/react-router/src/useActiveLocation.ts (1)
useActiveLocation(12-58)
🪛 ESLint
packages/react-router/src/link.tsx
[error] 110-110: Definition for rule 'react-hooks/exhaustive-deps' was not found.
(react-hooks/exhaustive-deps)
packages/react-router/src/useNavigate.tsx
[error] 31-31: Definition for rule 'react-hooks/exhaustive-deps' was not found.
(react-hooks/exhaustive-deps)
🔇 Additional comments (1)
packages/react-router/src/useActiveLocation.ts (1)
22-28: Fix stale closure inuseEffectdependencyThe
useEffectreferencesuseCustomActiveLocationas a boolean value instead of the state value. This causes a stale closure and prevents the effect from re-running whenuseCustomActiveLocationchanges.Apply this diff to fix the stale closure:
useEffect(() => { - if (!useCustomActiveLocation) { + if (!useCustomActiveLocation) { setActiveLocation(state.location) } else { setActiveLocation(customActiveLocation) } }, [state.location, useCustomActiveLocation, customActiveLocation])Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/react-router/src/link.tsx (1)
108-116: Nice: reactive “from” and consistent option plumbingGood fixes:
- Using useActiveLocation/getFromPath to default “from” to the active location closes the stale href/activeness gap noted earlier.
- Centralizing navigation/preload/buildLocation usage on the memoized _options keeps behavior consistent across Link, preload, and isActive.
No action needed.
Also applies to: 133-136, 196-201, 249-254
🧹 Nitpick comments (6)
packages/solid-router/src/useActiveLocation.ts (4)
9-13: Unify type naming with React and broadengetFromPathinput for reactivityFor cross-package parity and to support reactive
fromvalues, consider:
- Renaming
UseLocationResult→UseActiveLocationResult(matches React).- Updating the
getFromPathsignature to accept astring | Accessor<string | undefined>so downstream callers can pass a reactivefrom.-export type UseLocationResult = { +export type UseActiveLocationResult = { activeLocation: Accessor<ParsedLocation> - getFromPath: (from?: string) => Accessor<string> + getFromPath: (from?: string | Accessor<string | undefined>) => Accessor<string> setActiveLocation: (location?: ParsedLocation) => void }And update the return type at the function signature accordingly (see lines 15-17).
15-17: Align return type with renamed interfaceIf you rename the interface per prior comment, update the function signature accordingly.
-export function useActiveLocation( - location?: ParsedLocation, -): UseLocationResult { +export function useActiveLocation( + location?: ParsedLocation, +): UseActiveLocationResult {
19-31: HoistuseRouterStateaccessor; avoid re-instantiating it and remove stale commentYou’re calling
useRouterState({ select: (s) => s.location })()in two places. Hoisting the accessor prevents repeated instantiation and clarifies dependencies. The existing comment becomes obsolete after this refactor.- // we are not using a variable here for router state location since we need to only calculate that if the location is not passed in. It can result in unnecessary history actions if we do that. - const [activeLocation, setActiveLocation] = createSignal<ParsedLocation>( - location ?? useRouterState({ select: (s) => s.location })(), - ) + // Hoist the router location accessor; read it reactively only when needed. + const routerLocation = useRouterState({ select: (s) => s.location }) + const [activeLocation, setActiveLocation] = createSignal<ParsedLocation>( + location ?? routerLocation(), + ) @@ createEffect(() => { - setActiveLocation( - customActiveLocation() ?? useRouterState({ select: (s) => s.location })(), - ) + setActiveLocation(customActiveLocation() ?? routerLocation()) })Alternative (more idiomatic Solid): derive
activeLocationas a memo and drop the effect:- const [activeLocation, setActiveLocation] = createSignal<ParsedLocation>( - location ?? routerLocation(), - ) - const [customActiveLocation, setCustomActiveLocation] = createSignal< - ParsedLocation | undefined - >(location) - - createEffect(() => { - setActiveLocation(customActiveLocation() ?? routerLocation()) - }) + const [customActiveLocation, setCustomActiveLocation] = createSignal< + ParsedLocation | undefined + >(location) + const activeLocation = createMemo<ParsedLocation>(() => { + return customActiveLocation() ?? routerLocation() + })
38-52: RefactorgetFromPathto support reactivefromand avoid per-call memosTwo improvements here:
- Make
fromreactive by accepting anAccessor<string | undefined>so callers can pass dynamic signals.- Stop allocating a new
createMemoon every call—either wrap it once insidegetFromPath(Option A) or return a plain accessor and let callers memoize if needed (Option B).— In packages/solid-router/src/useActiveLocation.ts around
getFromPath:Option A (keep memo, resolve reactive
from):-export type UseLocationResult = { - activeLocation: Accessor<ParsedLocation> - getFromPath: (from?: string) => Accessor<string> - setActiveLocation: (location?: ParsedLocation) => void -} +export type UseLocationResult = { + activeLocation: Accessor<ParsedLocation> + getFromPath: ( + from?: string | Accessor<string | undefined>, + ) => Accessor<string | undefined> + setActiveLocation: (location?: ParsedLocation) => void +} export function useActiveLocation( location?: ParsedLocation, ): UseLocationResult { @@ - const getFromPath = (from?: string) => - createMemo(() => { + const getFromPath = ( + from?: string | Accessor<string | undefined>, + ) => + createMemo(() => { + const resolvedFrom = + typeof from === 'function' + ? (from as Accessor<string | undefined>)() + : from const currentRouteMatches = router.matchRoutes( customActiveLocation() ?? activeLocation(), { _buildLocation: false }, ) return ( - from ?? + resolvedFrom ?? last(currentRouteMatches)?.fullPath ?? currentRouteMatch().fullPath )Option B (preferred—return a pure accessor; callers can
createMemoif they need caching):-export type UseLocationResult = { - activeLocation: Accessor<ParsedLocation> - getFromPath: (from?: string) => Accessor<string> - setActiveLocation: (location?: ParsedLocation) => void -} +export type UseLocationResult = { + activeLocation: Accessor<ParsedLocation> + getFromPath: ( + from?: string | Accessor<string | undefined>, + ) => Accessor<string | undefined> + setActiveLocation: (location?: ParsedLocation) => void +} export function useActiveLocation( location?: ParsedLocation, ): UseLocationResult { @@ - const getFromPath = (from?: string) => - createMemo(() => { + const getFromPath = ( + from?: string | Accessor<string | undefined>, + ) => + () => { + const resolvedFrom = + typeof from === 'function' + ? (from as Accessor<string | undefined>)() + : from + const currentRouteMatches = router.matchRoutes( + customActiveLocation() ?? activeLocation(), + { _buildLocation: false }, + ) + + return ( + resolvedFrom ?? + last(currentRouteMatches)?.fullPath ?? + currentRouteMatch().fullPath + ) + }• Don’t forget to update the exported
UseLocationResult.getFromPathsignature as shown.
• Verified all internal call sites inuseNavigate.tsxandlink.tsxtreat the return value as an accessor (from()).packages/react-router/src/link.tsx (2)
263-280: Harden external links opened in a new tabFor external links with target="_blank", consider defaulting rel="noopener noreferrer" when the caller didn’t provide rel to prevent reverse tabnabbing.
Example patch:
if (isExternal) { return { ...propsSafeToSpread, ref: innerRef as React.ComponentPropsWithRef<'a'>['ref'], type, href: to, ...(children && { children }), ...(target && { target }), + ...((target === '_blank' && !(propsSafeToSpread as any).rel) && { + rel: 'noopener noreferrer', + }), ...(disabled && { disabled }), ...(style && { style }), ...(className && { className }), ...(onClick && { onClick }), ...(onFocus && { onFocus }), ...(onMouseEnter && { onMouseEnter }), ...(onMouseLeave && { onMouseLeave }), ...(onTouchStart && { onTouchStart }), } }
102-106: Redundant routerLocation subscription and dependencyYou already derive from via useActiveLocation, which internally subscribes to state.location. Subscribing here to routerLocation and including it in _options deps seems redundant and can cause extra recalcs.
If nothing else depends on routerLocation directly, you can remove this subscription and drop it from the deps:
- // subscribe to location here to re-build fromPath if it changes - const routerLocation = useRouterState({ - select: (s) => s.location, - structuralSharing: true as any, - }) @@ - [ - router, - routerLocation, - currentSearch, - from, + [ + router, + currentSearch, + from, options._fromLocation, options.hash, options.to, options.search, options.params, options.state, options.mask, options.unsafeRelative, ],Keep it only if you need to force recomputation when options.from is explicitly provided and from won’t change. Otherwise, this is safe to drop.
Also applies to: 112-121
📜 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 (3)
packages/react-router/src/link.tsx(6 hunks)packages/react-router/src/useActiveLocation.ts(1 hunks)packages/solid-router/src/useActiveLocation.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react-router/src/useActiveLocation.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/solid-router/src/useActiveLocation.ts (1)
packages/react-router/src/useActiveLocation.ts (1)
useActiveLocation(14-53)
packages/react-router/src/link.tsx (3)
packages/react-router/src/useRouterState.tsx (1)
useRouterState(31-59)packages/react-router/src/useActiveLocation.ts (1)
useActiveLocation(14-53)packages/router-core/src/link.ts (1)
preloadWarning(706-706)
🪛 ESLint
packages/react-router/src/link.tsx
[error] 116-116: Definition for rule 'react-hooks/exhaustive-deps' was not found.
(react-hooks/exhaustive-deps)
🔇 Additional comments (2)
packages/solid-router/src/useActiveLocation.ts (2)
33-36: Good: using the current route match directly (avoids fragile indexing)This mirrors the React implementation and avoids reliance on
router.state.matchesindices during transitions.
54-59: Return shape looks goodClean, minimal API surface with clear responsibilities.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/react-router/src/useActiveLocation.ts (1)
18-29: Simplify: derive activeLocation instead of storing it in state + effectactiveLocation can be derived from customActiveLocation ?? routerLocation. Dropping the extra state/effect reduces re-renders and avoids the race fixed above.
Apply this diff:
const router = useRouter() const routerLocation = useRouterState({ select: (state) => state.location }) - const [activeLocation, setActiveLocation] = useState<ParsedLocation>( - location ?? routerLocation, - ) const [customActiveLocation, setCustomActiveLocation] = useState< ParsedLocation | undefined >(location) - useEffect(() => { - setActiveLocation(customActiveLocation ?? routerLocation) - }, [routerLocation, customActiveLocation]) + const activeLocation = customActiveLocation ?? routerLocationpackages/solid-router/src/useActiveLocation.ts (1)
19-31: Hoist routerLocation accessor; avoid creating a new subscription inside effectsCalling useRouterState inside createEffect creates a new accessor on each run. Hoist the accessor once and read it inside the effect for cleaner subscriptions and less churn.
Apply this diff:
- // we are not using a variable here for router state location since we need to only calculate that if the location is not passed in. It can result in unnecessary history actions if we do that. - const [activeLocation, setActiveLocation] = createSignal<ParsedLocation>( - location ?? useRouterState({ select: (s) => s.location })(), - ) + const routerLocation = useRouterState({ select: (s) => s.location }) + const [activeLocation, setActiveLocation] = createSignal<ParsedLocation>( + location ?? routerLocation(), + ) @@ - createEffect(() => { - setActiveLocation( - customActiveLocation() ?? useRouterState({ select: (s) => s.location })(), - ) - }) + createEffect(() => { + setActiveLocation(customActiveLocation() ?? routerLocation()) + })
📜 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 (2)
packages/react-router/src/useActiveLocation.ts(1 hunks)packages/solid-router/src/useActiveLocation.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/solid-router/src/useActiveLocation.ts (1)
packages/react-router/src/useActiveLocation.ts (1)
useActiveLocation(14-57)
packages/react-router/src/useActiveLocation.ts (2)
packages/solid-router/src/useActiveLocation.ts (1)
useActiveLocation(15-61)packages/router-core/src/router.ts (1)
state(936-938)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test
- GitHub Check: Preview
🔇 Additional comments (2)
packages/react-router/src/useActiveLocation.ts (1)
52-57: API surface LGTMThe returned shape (activeLocation, getFromPath, setActiveLocation) is cohesive and matches consumers’ needs. Once the above fixes are applied, this hook will be robust for the new relative routing semantics.
packages/solid-router/src/useActiveLocation.ts (1)
56-61: Parity and semantics look goodThe getFromPath contract and the active-location override match the React implementation, enabling consistent relative navigation defaults across frameworks.
When using relative routing we have 3 things to consider:
to="."andto=".."in layout routesWe have had a few issues where it hasn't been clear how relative routing is applied in various scenarios, and where the application differs, for example
to="."andto=".."was treated differently.This PR applies the following:
This is achieved by setting Link and useNavigate to default to the current active location instead of the rendered route location. This also simplifies the logic in buildLocation as we no longer specifically cater for
to=".".Included in this PR is the required changes to router-core (buildLocation), react-router and solid-router (Link and useNavigate), tests for both react-router and solid-router and an update to the docs to clarify the usage.
This closes #4842
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests