fix(i18n): remove duplicate locale keys and guard with a regression test#2789
Open
rsnetworkinginc wants to merge 1 commit into
Open
fix(i18n): remove duplicate locale keys and guard with a regression test#2789rsnetworkinginc wants to merge 1 commit into
rsnetworkinginc wants to merge 1 commit into
Conversation
YAML resolves duplicate keys by silently letting the last occurrence
win, so every duplicated key hides an earlier definition without any
warning:
- transactions/{en,de,nl,hu}.yml defined `transactions.merge_duplicate`
twice: once as a stale flat string ("Yes, merge them") and once as the
`success`/`failure` mapping the controllers actually read. The flat
string is dead either way (the show view reads
`transactions.show.merge_duplicate`), so drop it.
- securities/fr.yml listed five providers (tiingo, eodhd, alpha_vantage,
mfapi, binance_public) twice inside `securities.providers`.
- settings/api_keys/hu.yml defined
`settings.api_keys.created.usage_instructions_title` twice.
- imports/en.yml defined `imports.create.ndjson_uploaded` twice.
All removals keep the currently-winning value, so rendered output is
unchanged; this only makes the files match what I18n already loads.
Add a non-skipped I18nTest case that walks the raw Psych AST of every
file under config/locales and fails on same-level duplicate keys, so
shadowed translations can't sneak back in.
Fixes we-promise#1506
Fixes we-promise#1502
Contributor
📝 WalkthroughWalkthroughLocale translation entries were reordered, removed, or expanded across import, securities, API-key, and transaction views. A new i18n test recursively inspects YAML ASTs to detect duplicate keys and report their locations. ChangesLocale maintenance
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/i18n_test.rb`:
- Around line 63-90: Dedent the private helper method duplicate_key_offenses and
its entire body by two spaces so its indentation matches the surrounding class
and follows the project’s two-space Ruby style.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6c22e89e-4272-4990-a539-162ccecb4fe9
📒 Files selected for processing (8)
config/locales/views/imports/en.ymlconfig/locales/views/securities/fr.ymlconfig/locales/views/settings/api_keys/hu.ymlconfig/locales/views/transactions/de.ymlconfig/locales/views/transactions/en.ymlconfig/locales/views/transactions/hu.ymlconfig/locales/views/transactions/nl.ymltest/i18n_test.rb
💤 Files with no reviewable changes (7)
- config/locales/views/transactions/de.yml
- config/locales/views/transactions/nl.yml
- config/locales/views/settings/api_keys/hu.yml
- config/locales/views/transactions/en.yml
- config/locales/views/securities/fr.yml
- config/locales/views/transactions/hu.yml
- config/locales/views/imports/en.yml
Comment on lines
+63
to
+90
| def duplicate_key_offenses(node, path, file) | ||
| offenses = [] | ||
|
|
||
| case node | ||
| when Psych::Nodes::Mapping | ||
| first_definition_lines = {} | ||
|
|
||
| node.children.each_slice(2) do |key_node, value_node| | ||
| if key_node.is_a?(Psych::Nodes::Scalar) | ||
| key_path = path + [ key_node.value ] | ||
|
|
||
| if (first_line = first_definition_lines[key_node.value]) | ||
| offenses << "#{file}:#{key_node.start_line + 1} duplicate key `#{key_path.join(".")}` (first defined on line #{first_line})" | ||
| else | ||
| first_definition_lines[key_node.value] = key_node.start_line + 1 | ||
| end | ||
|
|
||
| offenses.concat(duplicate_key_offenses(value_node, key_path, file)) | ||
| else | ||
| offenses.concat(duplicate_key_offenses(value_node, path, file)) | ||
| end | ||
| end | ||
| when Psych::Nodes::Sequence | ||
| node.children.each { |child| offenses.concat(duplicate_key_offenses(child, path, file)) } | ||
| end | ||
|
|
||
| offenses | ||
| end |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use two-space indentation for the private helper.
duplicate_key_offenses and its body are indented one level too far; dedent this block by two spaces to match the rest of the class. As per coding guidelines, Ruby code should use 2-space indent.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/i18n_test.rb` around lines 63 - 90, Dedent the private helper method
duplicate_key_offenses and its entire body by two spaces so its indentation
matches the surrounding class and follows the project’s two-space Ruby style.
Source: Coding guidelines
Collaborator
|
Reviewed the diff and verified the fix empirically (checked out both the base commit and the PR head into a scratch worktree and ran the new duplicate-key scan against each). Removed duplicates — all verified as true, value-identical duplicates:
Regression test:
Minor nit (non-blocking, already flagged by CodeRabbit): the No functional issues found. The scope is a bit broader than the linked issue (touches imports/securities/api_keys locales in addition to the transactions duplicate), but each change is independently a genuine duplicate-key fix, so I don't see a correctness concern with bundling them. Generated by Claude Code |
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
2 participants
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.
PR body — we-promise/sure — account: rsnetworkinginc
Branch:
fix/i18n-duplicate-locale-keys(in WSL clone/root/sure, commitdfd95b55, based onmain@40b18e84)Title:
fix(i18n): remove duplicate locale keys that shadow translationsSummary
Fixes #1506 (and its duplicate #1502).
YAML resolves duplicate keys by silently letting the last occurrence win, so every duplicated key in a locale file hides an earlier definition without any warning. A scan of
config/locales/**/*.ymlfound 11 same-level duplicates across 7 files:config/locales/views/transactions/{en,de,nl,hu}.ymltransactions.merge_duplicate— a stale flat string ("Yes, merge them") next to themerge_duplicate.success/failuremapping the controllers actually readconfig/locales/views/securities/fr.ymlsecurities.providers.{tiingo,eodhd,alpha_vantage,mfapi,binance_public}each defined twiceconfig/locales/views/settings/api_keys/hu.ymlsettings.api_keys.created.usage_instructions_titledefined twiceconfig/locales/views/imports/en.ymlimports.create.ndjson_uploadeddefined twiceWhat changed
transactions.merge_duplicatestring is dead either way: the merge button readstransactions.show.merge_duplicate, and the controllers readtransactions.merge_duplicate.success/.failure(transactions_controller.rb:199-209,pending_duplicate_merges_controller.rb:58).I18nTest#test_no_duplicate_keys_within_locale_filesthat walks the raw Psych AST of every file underconfig/localesand fails on same-level duplicate keys, so shadowed translations can't sneak back in. (The existing i18n-tasks checks in that file are allskipped; this one is fast — pure parsing, ~0.2 s — and runs.)Test plan
bin/rails test test/i18n_test.rb— new test passes on this branch; onmainit correctly reports all 11 duplicates listed above.bin/rails test— full unit/integration suite green.bin/rubocop— clean.Checklist for opening: base =
main, allow edits from maintainers ON.Summary by CodeRabbit
Bug Fixes
Tests