fix: ensure i18n objects are shallow copied#901
fix: ensure i18n objects are shallow copied#901mateo-leal wants to merge 1 commit intoWFCD:masterfrom
Conversation
📝 WalkthroughWalkthroughThe changes modify how i18n entries are processed when Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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.
🧹 Nitpick comments (1)
test/index.spec.mjs (1)
127-138: Nice regression test; consider adding anti-leak assertions.This already covers the core bug well. You can make it stricter by also asserting each run only has the requested locale.
Suggested test hardening
const esI18n = await getItem('es'); assert.ok(esI18n.es, 'should have i18n on object for es'); + assert.ok(!esI18n.ja, 'should not leak ja into es run'); const jaI18n = await getItem('ja'); assert.ok(jaI18n.ja, 'should have i18n on object for ja'); + assert.ok(!jaI18n.es, 'should not leak es into ja run');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/index.spec.mjs` around lines 127 - 138, The test should assert that each run returns only the requested locale to prevent cross-run leakage: update the test that uses getItem/wrapConstr (with i18nOnObject: true) to also assert that the returned i18n object contains the requested locale key (e.g., es or ja) and does NOT contain the other locale key, i.e., for esI18n assert es exists and ja does not, and for jaI18n assert ja exists and es does not; use the existing getItem, wrapConstr, firstItem, esI18n and jaI18n symbols to locate and modify the assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/index.spec.mjs`:
- Around line 127-138: The test should assert that each run returns only the
requested locale to prevent cross-run leakage: update the test that uses
getItem/wrapConstr (with i18nOnObject: true) to also assert that the returned
i18n object contains the requested locale key (e.g., es or ja) and does NOT
contain the other locale key, i.e., for esI18n assert es exists and ja does not,
and for jaI18n assert ja exists and es does not; use the existing getItem,
wrapConstr, firstItem, esI18n and jaI18n symbols to locate and modify the
assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e5a3369c-b14d-4900-a820-f6542888f353
📒 Files selected for processing (3)
index.jsindex.mjstest/index.spec.mjs
| const itemI18n = i18n[item.uniqueName]; | ||
| const raw = itemI18n ? { ...itemI18n } : undefined; |
There was a problem hiding this comment.
What other value besides an object or undefined itemI18n might be so it needs this check?
There was a problem hiding this comment.
None, but without that verification the branch coverage percentage requires a test that cannot be done on the if statement of line 111
2 participants
What did you fix?
When changing locales within the same runtime environment, the returned array did not include the i18n entries for the new locale.
This pull request replaces the reference to the global i18n object with a shallow copy to resolve this issue.
Reproduction steps
Evidence/screenshot/link to line
Considerations
Summary by CodeRabbit
Bug Fixes
Tests