feat: add shared localization infrastructure (xtask generate-strings)#703
feat: add shared localization infrastructure (xtask generate-strings)#703Vineet1101 wants to merge 4 commits intobitcoinppl:masterfrom
Conversation
📝 WalkthroughWalkthroughAdds a single-source JSON for localization and an xtask generator plus just recipe to produce platform-native localization files; generates Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as "Dev / CI"
participant Just as "just (gs)"
participant Xtask as "xtask (generate-strings)"
participant JSON as "localization/strings.json"
participant FS as "File System"
participant iOS as "iOS Localizable.strings"
participant Android as "Android generated_strings.xml"
Dev->>Just: run gs / just generate-strings
Just->>Xtask: invoke generate-strings
Xtask->>JSON: read & validate strings.json
Xtask->>Xtask: flatten keys & escape platform chars
Xtask->>FS: write iOS Localizable.strings
FS-->>iOS: file created
Xtask->>FS: write Android generated_strings.xml
FS-->>Android: file created
Xtask->>Dev: report emitted string count
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Greptile SummaryThis PR adds a centralized localization system: a shared
Confidence Score: 4/5Safe to merge after fixing the two header-string mismatches; the generator will produce dirty diffs otherwise. Two P1 findings: the iOS and Android header strings in localization.rs don't match the committed generated files. Running rust/xtask/src/localization.rs lines 660 and 720–721 (header string literals) Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[localization/strings.json\nsingle source of truth] --> B[cargo xtask generate-strings\nrust/xtask/src/localization.rs]
B --> C[flatten_to_ios\ndot-separated keys]
B --> D[flatten_to_android\nunderscore-separated keys]
C --> E[ios/Cove/Resources/en.lproj/\nLocalizable.strings]
D --> F[android/app/src/main/res/values/\ngenerated_strings.xml]
G[justfile: generate-strings / gs] --> B
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -0,0 +1,207 @@ | |||
| { | |||
| "$schema": "./strings.schema.json", | |||
There was a problem hiding this comment.
strings.json declares "$schema": "./strings.schema.json", but no strings.schema.json exists in the repository. IDE validation will either error or silently skip schema enforcement. Either commit the schema file or remove the reference until it's ready.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@localization/strings.json`:
- Line 132: Update the localization entry for the "body" key to remove the comma
after "recovery words" so the sentence reads "Whoever has your recovery words
controls your Bitcoin." — locate the "body" string in strings.json and replace
the current sentence fragment "Whoever has your recovery words, controls your
Bitcoin." with the corrected punctuation and wording.
- Line 2: The "$schema" reference in localization/strings.json points to a
missing file ("./strings.schema.json"); fix by either committing/adding the
referenced schema file to the repo (strings.schema.json) or removing the
"$schema" entry from localization/strings.json so the JSON no longer references
a non-existent schema; update localization/strings.json's "$schema" key or add
the schema file accordingly and ensure the name matches exactly.
In `@rust/xtask/src/localization.rs`:
- Around line 55-57: The generated file header written into the mutable String
(see variable output and the output.push_str calls in localization generator)
must be made deterministic and match the committed “— DO NOT EDIT” header;
replace the current push_str content at the header sites with a single constant
header string (no timestamps, no random data) that exactly matches the committed
header (including the “— DO NOT EDIT” line), and ensure the same constant is
reused for the second header location (the other push_str block around the &str
generation) so running just generate-strings won’t change the files.
- Around line 14-16: Replace runtime lookups of CARGO_MANIFEST_DIR with the
compile-time env! macro and centralize it in a small helper: create a fn
manifest_dir() -> &'static str { env!("CARGO_MANIFEST_DIR") } and use
Path::new(manifest_dir()) wherever std::env::var("CARGO_MANIFEST_DIR") is
currently used (e.g., the code that builds json_path and the other occurrences
referenced around lines 45-48 and 103-107), removing the expect() panic call and
anchoring paths with the compile-time string.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6d17d5f1-1162-4425-8d61-56bbe0a9a40e
⛔ Files ignored due to path filters (1)
rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
android/app/src/main/res/values/generated_strings.xmlios/Cove/Resources/en.lproj/Localizable.stringsjustfilelocalization/strings.jsonrust/xtask/Cargo.tomlrust/xtask/src/localization.rsrust/xtask/src/main.rs
| "recoveryWords": { | ||
| "title": "Recovery Words", | ||
| "secureNote": "Please save these words in a secure location.", | ||
| "body": "Your secret recovery words are the only way to recover your wallet if you lose your phone or switch to a different wallet. Whoever has your recovery words, controls your Bitcoin.", |
There was a problem hiding this comment.
Remove the comma in the recovery warning.
This should read more naturally as “Whoever has your recovery words controls your Bitcoin.”
✏️ Proposed copy fix
- "body": "Your secret recovery words are the only way to recover your wallet if you lose your phone or switch to a different wallet. Whoever has your recovery words, controls your Bitcoin.",
+ "body": "Your secret recovery words are the only way to recover your wallet if you lose your phone or switch to a different wallet. Whoever has your recovery words controls your Bitcoin.",📝 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.
| "body": "Your secret recovery words are the only way to recover your wallet if you lose your phone or switch to a different wallet. Whoever has your recovery words, controls your Bitcoin.", | |
| "body": "Your secret recovery words are the only way to recover your wallet if you lose your phone or switch to a different wallet. Whoever has your recovery words controls your Bitcoin.", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@localization/strings.json` at line 132, Update the localization entry for the
"body" key to remove the comma after "recovery words" so the sentence reads
"Whoever has your recovery words controls your Bitcoin." — locate the "body"
string in strings.json and replace the current sentence fragment "Whoever has
your recovery words, controls your Bitcoin." with the corrected punctuation and
wording.
Add a centralized localization system that generates platform-specific string files from a single JSON source of truth. New files: - localization/strings.json: shared source with 162 strings organized by feature area (common, wallet, send, transaction, etc.) - rust/xtask/src/localization.rs: generator that produces iOS Localizable.strings and Android generated_strings.xml - ios/Cove/Resources/en.lproj/Localizable.strings: generated iOS strings - android/app/src/main/res/values/generated_strings.xml: generated Android strings Modified: - rust/xtask/src/main.rs: wire GenerateStrings command - rust/xtask/Cargo.toml: add serde_json dependency - justfile: add 'generate-strings' recipe (alias: gs) Includes 7 unit tests covering iOS/Android flattening, XML/quote escaping, nested keys, and $schema key filtering. Closes bitcoinppl#538 (Phase 1)
Vineet1101
force-pushed
the
feature/shared-localization
branch
from
2a6bcf2 to
0dd7f4c
Compare
Move sort_date, sort_name, sort_amount, sort_change, and utxo_description from the hand-written strings.xml into the generated file. These are now managed by localization/strings.json as the single source of truth.
The justfile recipe used 'test -f' to skip rebuilding, which ran stale binaries when source was modified. Now always runs cargo build (which is a sub-second no-op when nothing changed).
|
hey @praveenperera can you please review it |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
rust/xtask/src/localization.rs (1)
68-68: Nit: inconsistent em-dash encoding across headers.Line 68 uses
\u{2014}while line 126 uses the literal—. Output bytes are identical, but making the source consistent (either both literal or both escaped) avoids future confusion.♻️ Proposed tweak
- output.push_str("/* Generated from localization/strings.json \u{2014} DO NOT EDIT */\n\n"); + output.push_str("/* Generated from localization/strings.json — DO NOT EDIT */\n\n");Also applies to: 126-126
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/xtask/src/localization.rs` at line 68, Two header string literals in localization.rs are inconsistent: one uses the escaped em-dash (\u{2014}) and another uses the literal ‘—’; update both occurrences of the generated-file comment (the output.push_str calls that start with "/* Generated from localization/strings.json") to use the same encoding—either replace \u{2014} with the literal — in the first occurrence or replace the literal — with \u{2014} in the other—so both header strings match exactly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust/xtask/src/localization.rs`:
- Around line 162-168: The blanket percent-escaping in the localized string
pipeline (the let escaped = s.replace(...).replace('%', '%%') chain) prevents
Android/Java format placeholders like "%1$s" from surviving; change the logic to
only double '%' when the string is not intended to be a format string — e.g. add
a per-key boolean flag (formatted or raw) in the source metadata and, in the
code that computes `escaped`, conditionally apply the `replace('%','%%')` step
only when formatted == false (or alternatively detect format placeholders with a
regex like /%(\d+\$)?[sdf]/ and skip escaping if matched). Update the code that
constructs/reads keys so callers can mark keys that expect String.format-style
placeholders.
- Around line 160-175: The Android escaping routine in the Value::String arm
must first escape backslashes so later replacements (like apostrophe and
newline) don't double-count introduced backslashes; in the function handling
string serialization (the Value::String branch that builds `escaped` and pushes
to `output`), add a `.replace('\\', "\\\\")` as the first replace in the chain
(before .replace('\'', "\\'") and .replace('\n', "\\n")), and add a unit test
(next to test_flatten_android_escapes_apostrophe) that supplies a raw backslash
(e.g., "C:\\path") and asserts the produced Android string contains `\\` to
verify correct escaping.
---
Nitpick comments:
In `@rust/xtask/src/localization.rs`:
- Line 68: Two header string literals in localization.rs are inconsistent: one
uses the escaped em-dash (\u{2014}) and another uses the literal ‘—’; update
both occurrences of the generated-file comment (the output.push_str calls that
start with "/* Generated from localization/strings.json") to use the same
encoding—either replace \u{2014} with the literal — in the first occurrence or
replace the literal — with \u{2014} in the other—so both header strings match
exactly.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f8fc27d0-c75a-4d15-bdc9-ec3866d15108
⛔ Files ignored due to path filters (1)
rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
justfilerust/xtask/src/localization.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- justfile
| Value::String(s) => { | ||
| // escape XML special characters and Android format specifiers | ||
| let escaped = s | ||
| .replace('&', "&") | ||
| .replace('<', "<") | ||
| .replace('"', """) | ||
| .replace('\'', "\\'") | ||
| .replace('%', "%%") | ||
| .replace('\n', "\\n"); | ||
|
|
||
| output.push_str(&format!(" <string name=\"{prefix}\">{escaped}</string>\n")); | ||
| *count += 1; | ||
| } | ||
| _ => {} | ||
| } | ||
| } |
There was a problem hiding this comment.
Android escaping is missing backslash handling.
In Android string resources, \ is the escape prefix, so a literal backslash in the source JSON must be emitted as \\. The current chain never escapes \, and then introduces backslashes itself via .replace('\'', "\\'") and .replace('\n', "\\n"). Consequences:
- A source string containing
C:\pathemits<string>...C:\path</string>— Android will silently drop the\(or interpret\pas an unknown escape). - If you ever prepend a
\\escape step after the apostrophe/newline steps, the introduced\'/\ntokens would get doubled. So the fix must be the first replacement.
🐛 Proposed fix
let escaped = s
- .replace('&', "&")
+ .replace('\\', "\\\\")
+ .replace('&', "&")
.replace('<', "<")
.replace('"', """)
.replace('\'', "\\'")
.replace('%', "%%")
.replace('\n', "\\n");Worth adding a test alongside test_flatten_android_escapes_apostrophe that feeds a raw \ and asserts the output contains \\.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/xtask/src/localization.rs` around lines 160 - 175, The Android escaping
routine in the Value::String arm must first escape backslashes so later
replacements (like apostrophe and newline) don't double-count introduced
backslashes; in the function handling string serialization (the Value::String
branch that builds `escaped` and pushes to `output`), add a `.replace('\\',
"\\\\")` as the first replace in the chain (before .replace('\'', "\\'") and
.replace('\n', "\\n")), and add a unit test (next to
test_flatten_android_escapes_apostrophe) that supplies a raw backslash (e.g.,
"C:\\path") and asserts the produced Android string contains `\\` to verify
correct escaping.
| let escaped = s | ||
| .replace('&', "&") | ||
| .replace('<', "<") | ||
| .replace('"', """) | ||
| .replace('\'', "\\'") | ||
| .replace('%', "%%") | ||
| .replace('\n', "\\n"); |
There was a problem hiding this comment.
Heads-up: blanket % → %% blocks Android format strings.
This escapes every % unconditionally, which is safe for non-formatted strings but means keys intended for getString(id, args...) / String.format (e.g. "%1$s sent") can't be expressed in the JSON source — they'll be emitted as %%1$s and printed literally. If phase 1 doesn't need format args this is fine; flagging so the limitation is documented or addressed when you migrate platform strings that do use formatting (e.g. via a formatted="false" attribute or a per-key opt-out).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/xtask/src/localization.rs` around lines 162 - 168, The blanket
percent-escaping in the localized string pipeline (the let escaped =
s.replace(...).replace('%', '%%') chain) prevents Android/Java format
placeholders like "%1$s" from surviving; change the logic to only double '%'
when the string is not intended to be a format string — e.g. add a per-key
boolean flag (formatted or raw) in the source metadata and, in the code that
computes `escaped`, conditionally apply the `replace('%','%%')` step only when
formatted == false (or alternatively detect format placeholders with a regex
like /%(\d+\$)?[sdf]/ and skip escaping if matched). Update the code that
constructs/reads keys so callers can mark keys that expect String.format-style
placeholders.
1 participant
Add a centralized localization system that generates platform-specific string files from a single JSON source of truth.
New files:
Modified:
Includes 7 unit tests covering iOS/Android flattening, XML/quote escaping, nested keys, and $schema key filtering.
Closes #538 (Phase 1)
Summary
Testing
Platform Coverage
Checklist
Summary by CodeRabbit