fix: skip locale prefix for absolute and protocol-relative Link hrefs#455
Conversation
applyLocaleToHref() blindly prepended /${locale} to any href string,
producing malformed URLs like /fr/https://example.com/about or
/fr///example.com/about for absolute and protocol-relative hrefs.
Guard against http://, https://, and // URLs before applying the
locale prefix — these are external or same-origin absolute URLs that
should not be rewritten.
commit: |
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean, correct fix. The guard is in the right place and the tests cover all three URL patterns.
One thing to flag: applyNavigationLocale() in router.ts:125 is the sibling function used by Router.push() / Router.replace() and has the exact same bug — it will prepend /${locale} to absolute URLs. Should be fixed in this PR or a follow-up. See inline comment.
| // Absolute and protocol-relative URLs must not be prefixed — locale | ||
| // only applies to local paths. | ||
| if (href.startsWith("http://") || href.startsWith("https://") || href.startsWith("//")) { | ||
| return href; |
There was a problem hiding this comment.
The fix looks correct and is well-placed — right after the undefined/false early returns and before the locale-prefixing logic.
Note: the sibling function applyNavigationLocale() in router.ts:125-132 has the same bug. It's used by Router.push() and Router.replace() when a locale option is passed. Consider adding the same guard there (and a corresponding test) — either in this PR or as an immediate follow-up:
| return href; | |
| // Absolute and protocol-relative URLs must not be prefixed — locale | |
| // only applies to local paths. | |
| if (/^https?:\/\//.test(href) || href.startsWith("//")) { | |
| return href; | |
| } |
(The startsWith version you have is perfectly fine too — just showing the regex alternative for brevity. No change needed here.)
| React.createElement(Link, { href: "http://example.com/path", locale: "de" } as any, "x"), | ||
| ); | ||
| expect(html).toContain('href="http://example.com/path"'); | ||
| }); |
There was a problem hiding this comment.
Good coverage — three distinct URL patterns (https, protocol-relative, http) with clear comments explaining the expected behavior.
Minor: might be worth adding a test for mailto: or other non-http schemes to document whether those are handled (they'd currently get locale-prefixed, but that's a separate issue).
|
Review submitted. Here's the summary: Approved the PR. The fix is correct and well-placed — it adds an early return in One follow-up flagged: The sibling function |
…plyNavigationLocale
applyNavigationLocale() in router.ts (used by Router.push() / Router.replace())
had the same bug as applyLocaleToHref() in link.tsx — it would prepend /{locale}
to absolute and protocol-relative URLs, producing malformed hrefs like
/fr/https://example.com/about or /fr///cdn.example.com/img.png.
Add the same guard that was applied to applyLocaleToHref(), and add
corresponding tests covering https://, http://, and // URL patterns.
james-elicx
force-pushed
the
fix/link-locale-absolute-urls
branch
from
0de685d to
ba458ab
Compare
2 participants
Summary
applyLocaleToHref()blindly prepended/${locale}to any href, producing malformed URLs like/fr/https://example.com/aboutor/fr///example.com/aboutfor absolute and protocol-relative hrefshttp://,https://, and//URLs before applying the locale prefix — these should pass through unchangedTest plan