fix: Link onNavigate URL resolution for relative hrefs#483
Conversation
commit: |
|
CI failures are github being github |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 74994f9f18
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Good refactor. Centralizing URL resolution into url-utils.ts and fixing the onNavigate.url to match the actual navigation target for relative hrefs is the right approach. The test coverage is thorough — unit tests for each helper, SSR rendering tests under basePath, and E2E tests for both routers.
The Codex P1 about false basePath deduplication is a false positive — the new withBasePath does not deduplicate; it unconditionally prepends, and the test at line 448-452 explicitly validates the correct double-prefix behavior (/docs/getting-started → /docs/docs/getting-started).
A few minor observations below, none blocking.
| const resolved = resolveRelativeHref(href, currentUrl, basePath); | ||
|
|
||
| if (!basePath) { | ||
| return withBasePath(resolved, basePath); |
There was a problem hiding this comment.
Nit: when basePath is falsy, withBasePath(resolved, "") is a no-op (the first guard !basePath returns resolved unchanged). This works but is slightly misleading — a plain return resolved; would be clearer.
| return withBasePath(resolved, basePath); | |
| return resolved; |
| !basePath || | ||
| !path.startsWith("/") || | ||
| path.startsWith("http://") || | ||
| path.startsWith("https://") || |
There was a problem hiding this comment.
Nit: the path.startsWith("http://") and path.startsWith("https://") branches are unreachable — they're already caught by !path.startsWith("/") since those strings start with h. The "//" check is still reachable (protocol-relative URLs start with /), so that one is needed. Not a bug, just dead defensive code.
| const pathname = | ||
| basePath && resolved.pathname === basePath | ||
| ? "" | ||
| : basePath | ||
| ? stripBasePath(resolved.pathname, basePath) | ||
| : resolved.pathname; | ||
| return pathname + resolved.search + resolved.hash; |
There was a problem hiding this comment.
The empty-string sentinel for the basePath-root case (resolved.pathname === basePath → "") is correct but subtle — it relies on toBrowserNavigationHref catching resolved === "" two functions away. A comment here noting that contract would help future readers, e.g.:
// Returns "" when at basePath root so toBrowserNavigationHref can re-add it.
| href="?page=2" | ||
| id="link-relative-query" | ||
| onNavigate={(event) => { | ||
| const navEvent = event as typeof event & { url: URL }; |
There was a problem hiding this comment.
The cast event as typeof event & { url: URL } is needed because vinext exposes event.url but the onNavigate type only has preventDefault(). If/when the fixture typings are updated to include url: URL in the onNavigate event, this cast can be dropped. Fine for now as a test fixture.
|
Review submitted. Here's a summary: Approved with a few minor nits:
|
2 participants
Fix
next/linkonNavigateso its callback URL matches the actual navigation target for relative hrefs.This aligns
Linkcallback behavior with the browser/history destination for cases like?page=2,#hash, and other relative targets, including whenbasePathis configured.What changed
onNavigate.urlin thenext/linkshim to use the same resolved target as navigationbasePathbrowser URL resolution in shared URL helpersbasePathprefixes are not applied twicehrefbehavior underbasePathfor relative valuesnext/navigationandnext/routerto use the same shared browser-target resolution pathonNavigatetypings aligned with Next.js and use local casts where vinext exposesevent.urlTests
Added and updated regression coverage for:
basePath+ locale-prefixed browser targetsbasePathbasePathLink onNavigaterelative query navigationLink onNavigaterelative query navigationVerification
Ran targeted checks:
pnpm test tests/link.test.tsPLAYWRIGHT_PROJECT=pages-router pnpm exec playwright test tests/e2e/pages-router/link-advanced.spec.ts -g "onNavigate reports the resolved URL for relative query hrefs"PLAYWRIGHT_PROJECT=app-router pnpm exec playwright test tests/e2e/app-router/nextjs-compat/navigation.spec.ts -g "Link onNavigate reports the resolved URL for relative query hrefs"pnpm run typecheckpnpm run fmt