fix: fix redirect to localized route if i18n is disable for single route and there is a catch all route defined#3911
fix: fix redirect to localized route if i18n is disable for single route and there is a catch all route defined#3911fedetorre wants to merge 2 commits intonuxt-modules:mainfrom
Conversation
WalkthroughAdds a new exported constant Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pages.ts (1)
101-119: Avoid marking partially-localized routes as fully disabled.
notLocalizedMapInvertis populated whenever any locale lacks a path. This will treat routes with a subset of locales (e.g.,locales: ['en']) as fully disabled, soisExistingNuxtRoutewill skip valid localized routes. Only mark a route as disabled when all locales are false.🔧 Suggested fix
- const localizedMapInvert: Record<string, string> = {} - const notLocalizedMapInvert: Record<string, string> = {} + const localizedMapInvert: Record<string, string> = {} + const notLocalizedMapInvert: Record<string, string> = {} for (const [path, localeConfig] of Object.entries(ctx.pathToConfig)) { const resPath = resolveRoutePath(path) invertedMap[resPath] ??= {} + let hasLocalized = false for (const [locale, localePath] of Object.entries(localeConfig!)) { const localized = localePath === true ? path : localePath - invertedMap[resPath][locale] = localized && resolveRoutePath(localized) - if (invertedMap[resPath][locale]) { - localizedMapInvert[invertedMap[resPath][locale]] = resPath - } else { - notLocalizedMapInvert[resPath] = resPath - } + const resolvedLocalized = localized && resolveRoutePath(localized) + invertedMap[resPath][locale] = resolvedLocalized + if (resolvedLocalized) { + localizedMapInvert[resolvedLocalized] = resPath + hasLocalized = true + } } + if (!hasLocalized) { + notLocalizedMapInvert[resPath] = resPath + } }
…ute and there is a catch all route defined
|
Thank you for your PR! Could you add a test so we can prevent regressions in the future? 🙏 |
@BobbieGoede I create the e2e test for the issue #3910 tell me if is it ok for you! |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@specs/issues/3910.spec.ts`:
- Around line 5-18: The test title claims both SSR and CSR but only performs an
SSR fetch (see test 'should not redirect i18n disable routes', the URL_PATHNAME
constant and the fetch(url(URL_PATHNAME)) call); either add a CSR check by
creating a browser page (use createPage() to navigate to URL_PATHNAME, assert
location/pathname and content) to cover client-side navigation, or update the
test description string to state "SSR-only" or similar so it matches the current
fetch-based assertions. Ensure you modify the same test block (the 'should not
redirect i18n disable routes' test) and keep the existing assertions for SSR if
you add CSR by duplicating equivalent checks for the page navigation.
specs/issues/3910.spec.ts
Outdated
| describe('#3910 - i18n disable for route needs priority to catch-all route on both SSR and CSR', async () => { | ||
| await setup({ | ||
| rootDir: fileURLToPath(new URL(`../fixtures/issues/3910`, import.meta.url)), | ||
| }) | ||
|
|
||
| const URL_PATHNAME = '/test'; | ||
|
|
||
| test('should not redirect i18n disable routes', async () => { | ||
| const res = await fetch(url(URL_PATHNAME)); | ||
|
|
||
| const u = new URL(res.url); | ||
| expect(res.status).toBe(200); | ||
| expect(u.pathname).toBe(URL_PATHNAME); | ||
| expect(await res.text()).toContain('Test page not localized'); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the complete test file to see if there's CSR coverage beyond the snippet
cat -n specs/issues/3910.spec.tsRepository: nuxt-modules/i18n
Length of output: 879
🏁 Script executed:
# Now let's check what utilities are available for CSR testing
cat -n specs/utils.ts | head -100Repository: nuxt-modules/i18n
Length of output: 107
🏁 Script executed:
# Find the test utility files
fd -e ts -e js specs/ | grep -i utilRepository: nuxt-modules/i18n
Length of output: 390
🏁 Script executed:
# Find test utility files correctly
fd . specs/ | grep -i utilRepository: nuxt-modules/i18n
Length of output: 372
🏁 Script executed:
# Check what utilities are exported
cat -n specs/utils/index.tsRepository: nuxt-modules/i18n
Length of output: 250
🏁 Script executed:
# Check the browser utilities for CSR testing capabilities
cat -n specs/utils/browser.tsRepository: nuxt-modules/i18n
Length of output: 3709
🏁 Script executed:
# Check if there are other test files for issue 3910
fd . specs/ | grep -i 3910Repository: nuxt-modules/i18n
Length of output: 483
🏁 Script executed:
# Check what the fetch utility is (server or client)
cat -n specs/utils/server.tsRepository: nuxt-modules/i18n
Length of output: 4956
🏁 Script executed:
# Check other issue test files to see the pattern for testing both SSR and CSR
fd . specs/issues/ -name "*.spec.ts" | head -5 | xargs -I {} sh -c 'echo "=== {} ===" && cat -n {} | head -40'Repository: nuxt-modules/i18n
Length of output: 291
🏁 Script executed:
# Check other issue test files to see the pattern for testing both SSR and CSR
ls specs/issues/*.spec.ts | head -5 | while read file; do echo "=== $file ===" && head -40 "$file"; doneRepository: nuxt-modules/i18n
Length of output: 5522
🏁 Script executed:
# Verify the entire 3910.spec.ts file - check if there are more tests beyond line 20
wc -l specs/issues/3910.spec.tsRepository: nuxt-modules/i18n
Length of output: 89
Add CSR coverage or update test description to SSR‑only.
The test description claims both SSR and CSR coverage, but only server-side rendering is tested (via fetch()). Other tests in this suite use createPage() for browser-based CSR navigation. Either add a client‑side navigation test using createPage() or revise the description to accurately reflect SSR‑only coverage.
🤖 Prompt for AI Agents
In `@specs/issues/3910.spec.ts` around lines 5 - 18, The test title claims both
SSR and CSR but only performs an SSR fetch (see test 'should not redirect i18n
disable routes', the URL_PATHNAME constant and the fetch(url(URL_PATHNAME))
call); either add a CSR check by creating a browser page (use createPage() to
navigate to URL_PATHNAME, assert location/pathname and content) to cover
client-side navigation, or update the test description string to state
"SSR-only" or similar so it matches the current fetch-based assertions. Ensure
you modify the same test block (the 'should not redirect i18n disable routes'
test) and keep the existing assertions for SSR if you add CSR by duplicating
equivalent checks for the page navigation.
3 participants
🔗 Linked issue
resolves #3910
📚 Description
There is a bug in the latest version of nuxt-i18n. If you have a catch‑all route (e.g. [...,slug].vue) defined in the pages directory, any routes that have i18n: false in their meta cannot be reached because the catch‑all route matches them first and triggers a redirect.
The expected behavior is that routes with i18n: false should take precedence over localized routes.
Further details and a reproduction are available in the related issue: #3910
Summary by CodeRabbit
Refactor
Bug Fixes
Tests
Chores