Repository navigation
Conversation
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: reviewed the full PR diff against its merge base at d6ba4757, including localization lookup/fallback, plural conversion and remaining translation coverage. Feedback on this draft PR; this is a COMMENT review, not approval.
No new actionable code findings. This does not certify linguistic correctness across every imported language.
Validation performed: the 1,005 additions preserve existing resource values. All localization dictionaries parse with plutil; no added-key duplicate or placeholder/markup mismatch was detected. validate-translations.js reports 0 errors (1,902 warnings), and the plural generator successfully produces 69 native plural translations across 15 languages.
Remaining AI-assisted follow-up
Against 1,802 English source keys, missing nonempty values per locale are:
| Locale | Missing |
|---|---|
| ar | 994 |
| ca | 761 |
| cs | 602 |
| de | 607 |
| el | 906 |
| es-419 | 600 |
| es | 741 |
| fr | 591 |
| it | 637 |
| nl | 597 |
| pl | 592 |
| pt | 1,750 |
| pt-BR | 591 |
| ru | 612 |
The supported Pubky/Paykit flows remain the priority: every locale still misses all 62 profile__ keys, 53 contacts__ keys, 35 pubky_auth__ keys and 78 subscriptions__ keys; 54 of 55 payment-request keys are missing. These full namespace counts are broader than the release-feature subset in the PR description.
Arabic improves from 74 to 808 nonempty source keys, but still needs substantial work. Portuguese (pt) remains especially sparse at 52/1,802; Brazilian Portuguese is separate. Other shared backlogs include 71 hardware-wallet, 22 BTCPay and 19 watch-only-account keys per locale.
The raw backlog includes potentially unused English keys; establish usage before bulk translation. Preserve existing values and native plural/placeholder rules, and retain native-speaker review for payment, security and recovery wording. Arabic needs focused RTL, amount/placeholder and long-text UI checks.
The pre-existing whole-file overwrite risk and arb → ar collision in pull-translations.sh are unchanged here and should be fixed separately to protect the restored translations. Follow-up: #936. Companion: synonymdev/bitkit-android#1491.
Device/layout testing and native-speaker approval were not performed in this review. Build/unit/integration/E2E jobs are skipped on this draft; translation validation and plural generation are not a full app test pass.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
[Low impact] The translation changes appear safe to merge. SummaryRestores Arabic translations and fills translation gaps across the supported languages.
Acknowledged scope from Reviews (1) · Last reviewed commit: "chore: correct spanish spelling of passp..." · Reviewed by Greptile |
talosmachina
left a comment
There was a problem hiding this comment.
No findings. Merges 1,005 Transifex translations into 13 iOS locales, including restoring 734 Arabic strings lost in the February pull. Reviewed 98854b4, full tier. Reasoned from the diff and CI; iOS does not build on this reviewer's Linux host.
What I checked, and 2 candidates I ruled out
Checked mechanically, every added or changed string: the file parses (Android: XML; iOS: every line is a "key" = "value"; entry), no duplicate keys, every key exists in English, and each value carries the same %1$s / {name} placeholders and the same markup tags as its English source. All 13 files come out clean against en.lproj at the head, including the 734 Arabic lines.
Ran: node scripts/validate-translations.js at 98854b4: 0 errors.
CI: validate green; Run Tests, Run Integration Tests and build-local were still running.
Ruled out
- The 13 removed es / es-419 lines: replacements of the same keys, so no key is lost.
- Arabic plurals dropping
{INPUT}/{OUTPUT}: the PR skips those two, and neither is in the diff.
Merge confidence: 4/5, every added string passes the placeholder and markup checks and the validator; the test jobs had not finished.
|
Folded into #938, which now targets master and carries this pull plus the AI-assisted translations and the review fixes, so 2.6.0 translations land in one PR per platform. @piotr-iohk your review here found nothing actionable; the same checks apply to #938. |
Twin: synonymdev/bitkit-android#1491
Refs: #936
Description
ar(734 strings). The February pull had cut it from about 1,240 strings to 29 becausepull-translations.shlets the near-empty Transifexarblocale overwritear.lproj; the script itself is unchanged here.pt).{INPUT}/{OUTPUT}placeholder, and 75 keys no Swift code references.Out of Scope
scripts/pull-translations.sh: thearb→armapping that wiped Arabic.Design
N/A — no UI changes.
Preview
N/A — translation-only change.
QA Notes
Journeys
N/A — no user-visible behaviour change.
Manual Tests
N/A
Automated Checks
plutil -linton every changedLocalizable.strings— files are validgenerate-plural-localizations.swifton the localization folder — all 69 plural translations in 15 languages convert