Conversation
📝 WalkthroughWalkthroughUpdates error payload fields from Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Middleware as i18n<br/>Handler
participant Store as Event<br/>Store
participant i18nInstance as i18n<br/>Instance
Note over Client,i18nInstance: Routup v5 Event-Based Flow (New)
Client->>Middleware: HTTP Request (event)
Middleware->>Store: Retrieve instance from<br/>event.store[symbol]
Middleware->>Middleware: Resolve locale<br/>locale(event)
Middleware->>Store: Write instance & locale to<br/>event.store[symbols]
Middleware->>Client: Return event.next()
Client->>i18nInstance: Later: useTranslator(event)<br/>retrieves from event.store
i18nInstance->>Client: Return translator
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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.
🧹 Nitpick comments (3)
packages/i18n/src/module.ts (1)
50-67: Variable shadowing:localesdeclared twice.On line 51,
const locales = await resolveLocales(instance)shadows the outerlocalesvariable declared on line 12. While this works correctly due to scoping, it can be confusing during maintenance.♻️ Suggested rename for clarity
- router.use(defineCoreHandler(async (event) => { - const locales = await resolveLocales(instance); + router.use(defineCoreHandler(async (event) => { + const availableLocales = await resolveLocales(instance); event.store[REQUEST_INSTANCE_SYMBOL] = instance; let reqLocale : string | undefined; if (typeof locale === 'function') { reqLocale = await locale(event); } if (typeof reqLocale === 'undefined') { const acceptLanguage = event.headers.get('accept-language'); - reqLocale = negotiateLanguage(acceptLanguage, locales); + reqLocale = negotiateLanguage(acceptLanguage, availableLocales); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/i18n/src/module.ts` around lines 50 - 67, The inner variable name `locales` inside the router.use handler shadows the outer `locales` variable; rename the inner binding returned by resolveLocales (e.g., to `resolvedLocales` or `availableLocales`) to avoid confusion. Update the reference used by negotiateLanguage(acceptLanguage, ...) to use the new name and keep all other symbols unchanged (router.use, defineCoreHandler, resolveLocales, REQUEST_INSTANCE_SYMBOL, REQUEST_LOCALE_SYMBOL, locale).packages/i18n/test/unit/module.spec.ts (1)
14-108: Tests look good, consider adding edge case coverage.The tests properly validate the migration to routup v5's event-based API using
router.fetch()anddefineCoreHandler. The three test cases cover the core functionality well.For more robust coverage, consider adding tests for edge cases:
- Empty or missing
accept-languageheader- No matching locale in the store (fallback behavior)
- Locale function returning
undefined🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/i18n/test/unit/module.spec.ts` around lines 14 - 108, Add unit tests covering edge cases: create new it() cases using Router, MemoryStore, i18n, defineCoreHandler and useTranslator to verify behavior when (1) the Accept-Language header is missing or empty — assert default/fallback locale result and 200 response; (2) the store has no matching locale — assert fallback string/behavior (e.g., key returned or error handling) and response status; and (3) the provided locale function returns undefined — assert that i18n falls back to default locale behavior. Reuse router.fetch and createTestRequest patterns from existing tests and assert response.status and response.text for each edge case.packages/i18n/src/utils.ts (1)
6-37: Consider validating quality bounds per RFC 7231.The
qquality value should be between 0 and 1 per HTTP specification. Currently, values outside this range (e.g.,q=2orq=-1) are accepted as-is. This is unlikely to cause issues in practice since malformed headers are rare, but clamping might improve robustness.♻️ Optional: clamp quality to valid range
for (const segment of segments.slice(1)) { const param = segment.trim(); if (param.startsWith('q=')) { quality = Number.parseFloat(param.substring(2)); if (Number.isNaN(quality)) { quality = 0; } + quality = Math.max(0, Math.min(1, quality)); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/i18n/src/utils.ts` around lines 6 - 37, The parseAcceptLanguage function currently accepts q values outside RFC 7231 bounds; after parsing the q value in parseAcceptLanguage (function name) and after the NaN check, clamp quality into the valid range [0,1] (e.g., quality = Math.min(Math.max(quality, 0), 1)) before using or pushing the entry into the entries array (LanguageEntry objects), so any values like q=2 or q=-1 are constrained to 1 and 0 respectively.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/i18n/src/module.ts`:
- Around line 50-67: The inner variable name `locales` inside the router.use
handler shadows the outer `locales` variable; rename the inner binding returned
by resolveLocales (e.g., to `resolvedLocales` or `availableLocales`) to avoid
confusion. Update the reference used by negotiateLanguage(acceptLanguage, ...)
to use the new name and keep all other symbols unchanged (router.use,
defineCoreHandler, resolveLocales, REQUEST_INSTANCE_SYMBOL,
REQUEST_LOCALE_SYMBOL, locale).
In `@packages/i18n/src/utils.ts`:
- Around line 6-37: The parseAcceptLanguage function currently accepts q values
outside RFC 7231 bounds; after parsing the q value in parseAcceptLanguage
(function name) and after the NaN check, clamp quality into the valid range
[0,1] (e.g., quality = Math.min(Math.max(quality, 0), 1)) before using or
pushing the entry into the entries array (LanguageEntry objects), so any values
like q=2 or q=-1 are constrained to 1 and 0 respectively.
In `@packages/i18n/test/unit/module.spec.ts`:
- Around line 14-108: Add unit tests covering edge cases: create new it() cases
using Router, MemoryStore, i18n, defineCoreHandler and useTranslator to verify
behavior when (1) the Accept-Language header is missing or empty — assert
default/fallback locale result and 200 response; (2) the store has no matching
locale — assert fallback string/behavior (e.g., key returned or error handling)
and response status; and (3) the provided locale function returns undefined —
assert that i18n falls back to default locale behavior. Reuse router.fetch and
createTestRequest patterns from existing tests and assert response.status and
response.text for each edge case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 42fd513d-2105-44d5-ad7e-1cda62af9817
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
packages/body/src/helpers/parse-json.tspackages/body/src/helpers/parse-url-encoded.tspackages/body/src/helpers/read-raw.tspackages/body/src/helpers/read-stream.tspackages/body/src/utils/decompress.tspackages/i18n/package.jsonpackages/i18n/src/constants.tspackages/i18n/src/module.tspackages/i18n/src/types.tspackages/i18n/src/use-translator.tspackages/i18n/src/utils.tspackages/i18n/test/unit/module.spec.ts
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/i18n/test/unit/module.spec.ts (1)
31-31: Consider deduplicating repeatedaccept-languagefixtures.The long header values are repeated across tests; extracting constants would reduce noise and ease updates.
♻️ Optional cleanup
+const ACCEPT_LANGUAGE_DE = 'de-CH,de-DE;q=0.9,de;q=0.8,en-US;q=0.7,en;q=0.6'; +const ACCEPT_LANGUAGE_EN = 'en-US;q=0.9,en-GB;q=0.8'; ... - let response = await router.fetch(createTestRequest('/', { headers: { 'accept-language': 'de-CH,de-DE;q=0.9,de;q=0.8,en-US;q=0.7,en;q=0.6' } })); + let response = await router.fetch(createTestRequest('/', { headers: { 'accept-language': ACCEPT_LANGUAGE_DE } })); ... - response = await router.fetch(createTestRequest('/', { headers: { 'accept-language': 'en-US;q=0.9,en-GB;q=0.8' } })); + response = await router.fetch(createTestRequest('/', { headers: { 'accept-language': ACCEPT_LANGUAGE_EN } }));Also applies to: 63-63, 94-94
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/i18n/test/unit/module.spec.ts` at line 31, Several tests duplicate the long accept-language header string; extract these repeated fixtures into named constants (e.g., ACCEPT_LANGUAGE_DE_CH or ACCEPT_LANGUAGE_DE_DE) and replace inline usages in the calls to createTestRequest(...) / router.fetch(...) in module.spec.ts to reduce duplication and simplify updates. Locate the repeated strings used in the createTestRequest calls (see the invocation with headers: { 'accept-language': ... }) and define the constants near the top of the test file (or in a shared test helper), then update the three occurrences to reference those constants.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/i18n/test/unit/module.spec.ts`:
- Line 31: Several tests duplicate the long accept-language header string;
extract these repeated fixtures into named constants (e.g.,
ACCEPT_LANGUAGE_DE_CH or ACCEPT_LANGUAGE_DE_DE) and replace inline usages in the
calls to createTestRequest(...) / router.fetch(...) in module.spec.ts to reduce
duplication and simplify updates. Locate the repeated strings used in the
createTestRequest calls (see the invocation with headers: { 'accept-language':
... }) and define the constants near the top of the test file (or in a shared
test helper), then update the three occurrences to reference those constants.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ce5e5f31-a64e-4b4e-acc1-7ef737ed275d
📒 Files selected for processing (2)
packages/i18n/src/module.tspackages/i18n/test/unit/module.spec.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes #746
Summary by CodeRabbit
Bug Fixes
Chores