No locale prefix for default locale#16050
Conversation
pettinarip
requested review from
corwintines,
minimalsm and
wackerow
as code owners
✅ Deploy Preview for ethereumorg ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
pettinarip
force-pushed
the
remove-fixed-locale
branch
from
dffb093 to
dfe2637
Compare
pettinarip
changed the title
|
@coderabbitai do your thing and review |
🧩 Analysis chainI'll review this PR that implements "as-needed" locale prefix configuration for next-intl routing. Let me analyze the changes first. 🏁 Script executed: Length of output: 6108 Now let me examine some key configuration files to better understand the implementation: 🏁 Script executed: Length of output: 2103 Let me check for the i18n config file in the correct location and examine any existing redirect configurations: 🏁 Script executed: Length of output: 2813 Let me examine the SEO-related implementations and understand the current locale configuration better: 🏁 Script executed: Length of output: 3096 Let me find and examine the package.json file and check for any SEO metadata implementations: 🏁 Script executed: Length of output: 5994 Let me find the package.json file and check for any metadata implementations in the codebase: 🏁 Script executed: Length of output: 3704 Now let me examine the metadata utilities and tutorial metadata to understand how SEO is currently implemented: 🏁 Script executed: Length of output: 7199 Let me now examine how the getFullUrl function was modified and check for any existing redirect configurations: 🏁 Script executed: Length of output: 11258 Based on my comprehensive analysis of the PR, I can now provide a thorough review of this important SEO migration. The changes implement "as-needed" locale prefix configuration for next-intl routing to remove unnecessary redirects for English users. @minimalsm: I've thoroughly reviewed PR #16050 implementing the "as-needed" locale prefix configuration. This is a well-structured SEO migration with several strong implementations, though I found some areas that need attention: ✅ Excellent Implementations1. Core Routing Configuration
2. SEO Metadata Updates
3. XML Sitemap Configuration
🔍 Issues Requiring Attention1. Matomo Analytics Inconsistency // This will track '/en/developers' for a page served at '/developers'
export const normalizePathForMatomo = (pathname: string): string => {
// ...
return `/${DEFAULT_LOCALE}${pathname}` // Adding /en prefix
}Recommendation: Consider whether you want analytics to track the actual served URLs ( 2. Missing Redirect Configuration
3. Testing Coverage
📋 SEO Migration Checklist VerificationBased on your TODO list, most items are completed in code:
🚀 Recommendations
The implementation is solid and follows Next.js + next-intl best practices. The code quality is high with proper type safety and clean abstractions. ✅ Actions performedReview triggered.
|
WalkthroughUpdates unify locale handling: default-locale stripping in sitemap, “as-needed” locale prefix in routing, and URL building that omits default locale. Matomo tracking now normalizes paths by ensuring a locale segment, with component and utility changes to send sanitized/normalized URLs to Matomo. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Router as Next Router
participant MatomoComp as Matomo Component
participant Utils as utils/matomo.normalizePathForMatomo
participant _paq as Matomo _paq Queue
Router->>MatomoComp: pathname change
MatomoComp->>Utils: normalize(previousPath)
Utils-->>MatomoComp: normalizedPreviousPath
MatomoComp->>Utils: normalize(pathname)
Utils-->>MatomoComp: normalizedPathname
MatomoComp->>_paq: setReferrerUrl(normalizedPreviousPath)
MatomoComp->>_paq: setCustomUrl(normalizedPathname)
MatomoComp->>_paq: deleteCustomVariables / trackPageView (delayed)
note right of _paq: URLs always include a locale segment
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ 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: 1
🧹 Nitpick comments (3)
src/i18n/routing.ts (1)
10-10: Good move: default locale without prefix via next-intl.This aligns routing with SEO expectations and reduces redirects.
Ensure 301 redirects exist for legacy
/en/*→/*to preserve link equity and avoid 404s. Want a quick_redirects/Next config snippet?next-sitemap.config.js (1)
4-4: Avoid drift: derive defaultLocale from i18n config.Hard-coding
"en"risks divergence from app constants.Apply:
-const defaultLocale = "en" +const defaultLocale = + (i18nConfig.find((l) => l.default)?.code) || "en"Also, nice touch stripping
/enand dropping the root entry. Consider adding explicit 301/en/*→/*at the edge to accelerate recrawl.Also applies to: 13-25, 27-27
src/components/Matomo.tsx (1)
8-8: Standardize Matomo URLs: send full URLs (not just paths).
trackCustomEventnow sends full URLs, while pageviews send only path—this creates inconsistency in reports.Apply:
- push(["setReferrerUrl", normalizedPreviousPath]) - push(["setCustomUrl", normalizedPathname]) + push(["setReferrerUrl", `${window.location.origin}${normalizedPreviousPath}`]) + push(["setCustomUrl", `${window.location.origin}${normalizedPathname}`])Also applies to: 44-49
📜 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 (5)
next-sitemap.config.js(1 hunks)src/components/Matomo.tsx(2 hunks)src/i18n/routing.ts(1 hunks)src/lib/utils/matomo.ts(2 hunks)src/lib/utils/url.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-18T21:22:45.937Z
Learnt from: CR
PR: ethereum/ethereum-org-website#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-18T21:22:45.937Z
Learning: Applies to {next-sitemap.js,next-sitemap.config.js} : Sitemap generation should use next-sitemap
Applied to files:
next-sitemap.config.js
🔇 Additional comments (2)
src/lib/utils/url.ts (1)
53-55: Check for absolute-path usage ofgetFullUrl
Our search didn’t identify any direct call sites passing an absolute path togetFullUrl. Manually verify there are no indirect or dynamic invocations that could trigger dropping the locale.src/lib/utils/matomo.ts (1)
50-54: LGTM: normalize and send full URL for events.Matches Matomo expectations and avoids query/hash noise.
| /** | ||
| * Normalizes paths to ensure consistent Matomo tracking. | ||
| * With localePrefix: "as-needed", English paths don't have /en prefix, | ||
| * but we want to track them as /en paths for analytics consistency. | ||
| */ | ||
| export const normalizePathForMatomo = (pathname: string): string => { | ||
| const hasLocalePrefix = LOCALES_CODES.some((locale) => | ||
| pathname.startsWith(`/${locale}/`) | ||
| ) | ||
|
|
||
| if (hasLocalePrefix) { | ||
| return pathname | ||
| } | ||
|
|
||
| // For paths without locale prefix (English content), add /en prefix | ||
| return `/${DEFAULT_LOCALE}${pathname}` | ||
| } |
There was a problem hiding this comment.
Edge case: locale root without trailing slash is misdetected.
startsWith(\/${locale}/`)misses/${locale}(no trailing slash), causing accidental/endouble-prefixing (e.g.,"/es"→"/en/es"`).
Apply:
-export const normalizePathForMatomo = (pathname: string): string => {
- const hasLocalePrefix = LOCALES_CODES.some((locale) =>
- pathname.startsWith(`/${locale}/`)
- )
+export const normalizePathForMatomo = (pathname: string): string => {
+ const hasLocalePrefix = LOCALES_CODES.some(
+ (locale) => pathname === `/${locale}` || pathname.startsWith(`/${locale}/`)
+ )📝 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.
| /** | |
| * Normalizes paths to ensure consistent Matomo tracking. | |
| * With localePrefix: "as-needed", English paths don't have /en prefix, | |
| * but we want to track them as /en paths for analytics consistency. | |
| */ | |
| export const normalizePathForMatomo = (pathname: string): string => { | |
| const hasLocalePrefix = LOCALES_CODES.some((locale) => | |
| pathname.startsWith(`/${locale}/`) | |
| ) | |
| if (hasLocalePrefix) { | |
| return pathname | |
| } | |
| // For paths without locale prefix (English content), add /en prefix | |
| return `/${DEFAULT_LOCALE}${pathname}` | |
| } | |
| export const normalizePathForMatomo = (pathname: string): string => { | |
| const hasLocalePrefix = LOCALES_CODES.some( | |
| (locale) => pathname === `/${locale}` || pathname.startsWith(`/${locale}/`) | |
| ) | |
| if (hasLocalePrefix) { | |
| return pathname | |
| } | |
| // For paths without locale prefix (English content), add /en prefix | |
| return `/${DEFAULT_LOCALE}${pathname}` | |
| } |
🤖 Prompt for AI Agents
In src/lib/utils/matomo.ts around lines 9 to 25, the locale detection uses
startsWith(`/${locale}/`) which misses the exact locale root (e.g., "/es") and
causes double-prefixing; update the hasLocalePrefix check to treat either an
exact match (`pathname === \`/${locale}\``) or a prefix match
(`pathname.startsWith(\`/${locale}/\`)`) as having a locale so the function
returns the original pathname for both `/locale` and `/locale/...` cases and
only prefixes `/${DEFAULT_LOCALE}` for truly locale-less paths.
There was a problem hiding this comment.
pathname is what it is, I don't think this matters... the helper function here just slips in the English prefix as-needed
4 participants
Intent to remove unnecessary redirect for most of the visitors.
Description
Configures next-intl routing to use "as-needed" locale prefix.
SEO Migration TODO
/en/*→/*hreflangs — English should now point to/(not/en)/enURLs./URLs for English.Summary by CodeRabbit
New Features
Refactor