Repository navigation
i18n_audit: catch UI literals hidden behind if/else, when, and .let (#540) - #558
Conversation
…yanbr#540) The Android regex only matched a literal sitting immediately after Text(/title=/etc, so Text(if (saved) "Saved" else "Save") slid straight past and the audit reported "clean" while shipping English regardless of device language. The Apple side had the identical blind spot, and it's a real functional bug there, not just a tooling gap: Text(cond ? "a" : "b") resolves to SwiftUI's non-localizing Text<S: StringProtocol> overload, so those sites are always-English on-device too. Rewrites both scanners to walk each call's/kwarg's own argument span instead of matching a fixed-position literal: transparent through (/[ always, and through { for the Kotlin idioms this codebase actually uses to conditionally pick a string (if/when/catch/else/try/finally, and .let as a null-coalescing ternary substitute) — opaque for everything else, so an unrelated nested composable slot lambda isn't swept into the same scan (an early draft that didn't distinguish these produced 489 spurious findings instead of the real ~250). Also fixes two bugs the rewrite surfaced: Kotlin's ${...} string-template interpolation corrupting the naive quote-matching, and title=/text=/etc. matching a local `val` declaration instead of only an actual call argument. Deliberately does not flag `+`-concatenated literals (typically uiString(R.string.x) + "hardcoded suffix") — a different, larger bug (tracked separately in ryanbr#557) that needs new translation content per site rather than reusing an existing value. The corrected scanners find real, previously-invisible findings: 170 distinct Android literals, 64 distinct Apple ones. Far more than one PR should close with rushed translations (see ryanbr#543 on what that produces), so this ships with Tools/i18n_audit_baseline.json: the CI gate now blocks any NEW hardcoded literal from this point on, while the pre-existing backlog is closed incrementally in separate, appropriately-sized follow-ups. An entry that stops appearing in a scan is simply inert; `--update-baseline` regenerates the file. Adds Tools/test_i18n_audit.py covering the conditional shapes above, the nested-slot-lambda non-explosion, the interpolation and local-declaration fixes, and the baseline diffing logic.
|
Reviewed, and since this is a Python tool I could run it end-to-end rather than read it — which I did, including driving the real scan path directly on crafted inputs. It genuinely fixes #540, and this is the fix for a blind spot I hit three separate times reviewing other PRs this session, so I'm glad to see it land. Verified working (not just claimed)
The baseline-not-rush decision is the correct one and well-executed. Two residual gaps — neither blocks this, both are follow-up material1. A kwarg set inside a lambda is still invisible — including the standard Compose a11y idiom. 2. The extraction tests bypass span-detection. The Minor (mention-only)
Solid, correct, honestly scoped, and it closes a real recurring hole. The two follow-ups (semantics-lambda a11y; a span-detection test) are worth filing but shouldn't hold the merge — the backlog is already tracked and the gate is sound. |
…#571) (#581) #558 fixed conditional literals but left a different axis invisible: a UI-text kwarg assigned INSIDE a lambda. `contentDescription` is in ANDROID_KWARG_PATTERN, but that pass is gated on an arg boundary (`(` or `,`), so the standard Compose a11y idiom Modifier.semantics { contentDescription = if (hr) "Ready" else "No data" } was skipped — its `{` isn't an arg boundary — and the audit reported clean while TalkBack read English regardless of device language. Adds a dedicated assignment pass: a bare `contentDescription =` (not a `==` comparison, a `.member` read, or a `val`/`var` local decl) is a UI-text site wherever it's assigned. It reads only its own value span via _argument_span_end (which stops at the enclosing `}`), so it does NOT reopen unrelated slot lambdas — the 489-explosion guard still holds. Surfaces 22 pre-existing hardcoded a11y strings across 9 screens (AppRoot, CoachScreen, FusedRecordScreen, HealthScreen, PipBar, SettingsScreen, StepsCalibrationScreen, UpdatesInboxScreen, WorkoutsScreen). Per the #558 pattern these land in the baseline as tracked backlog (170 -> 192 android) rather than forcing 22 rushed translations; --ci stays green and they get localized incrementally. NO new hardcoded literal can still land. Tested (Tools-only Python, fully runnable — no app-target/CI gap): python3 -m unittest test_i18n_audit -> 26/26 (4 new: semantics idiom, multi-line, decl/member exclusion, and the no-reopen-unrelated-slot guard) python3 Tools/i18n_audit.py --ci origin/main -> exit 0
…oach UI Merging upstream/main pulled in a substantially rewritten Tools/i18n_audit.py (ryanbr#540, ryanbr#558 and others): it now descends through ternaries and nested calls to find literals reachable in a call's first argument (closing exactly the "Text(cond ? "a" : "b") is invisible" blind spot P14 worked around by hand), and now also scans .accessibilityLabel(...), .navigationTitle(...) and a few more call kinds it previously ignored. Re-running it against all the P1-P15 coach work surfaced 44 genuinely un-cataloged literals the old tool could never have seen. Two kinds of fix: - The overwhelming majority were real gaps: accessibility labels on the coach chat header, plan cards, goal editor, memory-facts list, and settings toggles that were always plain English regardless of system language (63 catalog entries added — de/es/fr, matching the app's established register). - CoachSettingsView's token-usage accessibility label used `+` string concatenation, which types the whole expression as plain `String` and can never localize even the static wrapper text around it — restructured to String(localized:) interpolation. ModelSearchSheet's row label mixed a literal branch with a bare-variable branch in one ternary, which has the same effect — same fix, leaving the (untranslated, technical) model id exactly as typed. Also found and fixed 6 false positives: doc comments containing a quoted phrase (`"a plan with a time is a plan you keep"`, `"no key"`, …) that the scanner — which doesn't understand `///` syntax — mistook for part of a later call's literal argument. Reworded those comments to drop the literal quotes; no behavior change. python3 Tools/i18n_audit.py --ci main now exits 0. Both targets build; full StrandTests green (1210, up from 1195 — the merge brought in 15 upstream tests, none touching coach code). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…yanbr#540) (ryanbr#558) The Android regex only matched a literal sitting immediately after Text(/title=/etc, so Text(if (saved) "Saved" else "Save") slid straight past and the audit reported "clean" while shipping English regardless of device language. The Apple side had the identical blind spot, and it's a real functional bug there, not just a tooling gap: Text(cond ? "a" : "b") resolves to SwiftUI's non-localizing Text<S: StringProtocol> overload, so those sites are always-English on-device too. Rewrites both scanners to walk each call's/kwarg's own argument span instead of matching a fixed-position literal: transparent through (/[ always, and through { for the Kotlin idioms this codebase actually uses to conditionally pick a string (if/when/catch/else/try/finally, and .let as a null-coalescing ternary substitute) — opaque for everything else, so an unrelated nested composable slot lambda isn't swept into the same scan (an early draft that didn't distinguish these produced 489 spurious findings instead of the real ~250). Also fixes two bugs the rewrite surfaced: Kotlin's ${...} string-template interpolation corrupting the naive quote-matching, and title=/text=/etc. matching a local `val` declaration instead of only an actual call argument. Deliberately does not flag `+`-concatenated literals (typically uiString(R.string.x) + "hardcoded suffix") — a different, larger bug (tracked separately in ryanbr#557) that needs new translation content per site rather than reusing an existing value. The corrected scanners find real, previously-invisible findings: 170 distinct Android literals, 64 distinct Apple ones. Far more than one PR should close with rushed translations (see ryanbr#543 on what that produces), so this ships with Tools/i18n_audit_baseline.json: the CI gate now blocks any NEW hardcoded literal from this point on, while the pre-existing backlog is closed incrementally in separate, appropriately-sized follow-ups. An entry that stops appearing in a scan is simply inert; `--update-baseline` regenerates the file. Adds Tools/test_i18n_audit.py covering the conditional shapes above, the nested-slot-lambda non-explosion, the interpolation and local-declaration fixes, and the baseline diffing logic.
…ryanbr#571) (ryanbr#581) ryanbr#558 fixed conditional literals but left a different axis invisible: a UI-text kwarg assigned INSIDE a lambda. `contentDescription` is in ANDROID_KWARG_PATTERN, but that pass is gated on an arg boundary (`(` or `,`), so the standard Compose a11y idiom Modifier.semantics { contentDescription = if (hr) "Ready" else "No data" } was skipped — its `{` isn't an arg boundary — and the audit reported clean while TalkBack read English regardless of device language. Adds a dedicated assignment pass: a bare `contentDescription =` (not a `==` comparison, a `.member` read, or a `val`/`var` local decl) is a UI-text site wherever it's assigned. It reads only its own value span via _argument_span_end (which stops at the enclosing `}`), so it does NOT reopen unrelated slot lambdas — the 489-explosion guard still holds. Surfaces 22 pre-existing hardcoded a11y strings across 9 screens (AppRoot, CoachScreen, FusedRecordScreen, HealthScreen, PipBar, SettingsScreen, StepsCalibrationScreen, UpdatesInboxScreen, WorkoutsScreen). Per the ryanbr#558 pattern these land in the baseline as tracked backlog (170 -> 192 android) rather than forcing 22 rushed translations; --ci stays green and they get localized incrementally. NO new hardcoded literal can still land. Tested (Tools-only Python, fully runnable — no app-target/CI gap): python3 -m unittest test_i18n_audit -> 26/26 (4 new: semantics idiom, multi-line, decl/member exclusion, and the no-reopen-unrelated-slot guard) python3 Tools/i18n_audit.py --ci origin/main -> exit 0
…oach UI Merging upstream/main pulled in a substantially rewritten Tools/i18n_audit.py (ryanbr#540, ryanbr#558 and others): it now descends through ternaries and nested calls to find literals reachable in a call's first argument (closing exactly the "Text(cond ? "a" : "b") is invisible" blind spot P14 worked around by hand), and now also scans .accessibilityLabel(...), .navigationTitle(...) and a few more call kinds it previously ignored. Re-running it against all the P1-P15 coach work surfaced 44 genuinely un-cataloged literals the old tool could never have seen. Two kinds of fix: - The overwhelming majority were real gaps: accessibility labels on the coach chat header, plan cards, goal editor, memory-facts list, and settings toggles that were always plain English regardless of system language (63 catalog entries added — de/es/fr, matching the app's established register). - CoachSettingsView's token-usage accessibility label used `+` string concatenation, which types the whole expression as plain `String` and can never localize even the static wrapper text around it — restructured to String(localized:) interpolation. ModelSearchSheet's row label mixed a literal branch with a bare-variable branch in one ternary, which has the same effect — same fix, leaving the (untranslated, technical) model id exactly as typed. Also found and fixed 6 false positives: doc comments containing a quoted phrase (`"a plan with a time is a plan you keep"`, `"no key"`, …) that the scanner — which doesn't understand `///` syntax — mistook for part of a later call's literal argument. Reworded those comments to drop the literal quotes; no behavior change. python3 Tools/i18n_audit.py --ci main now exits 0. Both targets build; full StrandTests green (1210, up from 1195 — the merge brought in 15 upstream tests, none touching coach code). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…yanbr#540) (ryanbr#558) The Android regex only matched a literal sitting immediately after Text(/title=/etc, so Text(if (saved) "Saved" else "Save") slid straight past and the audit reported "clean" while shipping English regardless of device language. The Apple side had the identical blind spot, and it's a real functional bug there, not just a tooling gap: Text(cond ? "a" : "b") resolves to SwiftUI's non-localizing Text<S: StringProtocol> overload, so those sites are always-English on-device too. Rewrites both scanners to walk each call's/kwarg's own argument span instead of matching a fixed-position literal: transparent through (/[ always, and through { for the Kotlin idioms this codebase actually uses to conditionally pick a string (if/when/catch/else/try/finally, and .let as a null-coalescing ternary substitute) — opaque for everything else, so an unrelated nested composable slot lambda isn't swept into the same scan (an early draft that didn't distinguish these produced 489 spurious findings instead of the real ~250). Also fixes two bugs the rewrite surfaced: Kotlin's ${...} string-template interpolation corrupting the naive quote-matching, and title=/text=/etc. matching a local `val` declaration instead of only an actual call argument. Deliberately does not flag `+`-concatenated literals (typically uiString(R.string.x) + "hardcoded suffix") — a different, larger bug (tracked separately in ryanbr#557) that needs new translation content per site rather than reusing an existing value. The corrected scanners find real, previously-invisible findings: 170 distinct Android literals, 64 distinct Apple ones. Far more than one PR should close with rushed translations (see ryanbr#543 on what that produces), so this ships with Tools/i18n_audit_baseline.json: the CI gate now blocks any NEW hardcoded literal from this point on, while the pre-existing backlog is closed incrementally in separate, appropriately-sized follow-ups. An entry that stops appearing in a scan is simply inert; `--update-baseline` regenerates the file. Adds Tools/test_i18n_audit.py covering the conditional shapes above, the nested-slot-lambda non-explosion, the interpolation and local-declaration fixes, and the baseline diffing logic.
…ryanbr#571) (ryanbr#581) ryanbr#558 fixed conditional literals but left a different axis invisible: a UI-text kwarg assigned INSIDE a lambda. `contentDescription` is in ANDROID_KWARG_PATTERN, but that pass is gated on an arg boundary (`(` or `,`), so the standard Compose a11y idiom Modifier.semantics { contentDescription = if (hr) "Ready" else "No data" } was skipped — its `{` isn't an arg boundary — and the audit reported clean while TalkBack read English regardless of device language. Adds a dedicated assignment pass: a bare `contentDescription =` (not a `==` comparison, a `.member` read, or a `val`/`var` local decl) is a UI-text site wherever it's assigned. It reads only its own value span via _argument_span_end (which stops at the enclosing `}`), so it does NOT reopen unrelated slot lambdas — the 489-explosion guard still holds. Surfaces 22 pre-existing hardcoded a11y strings across 9 screens (AppRoot, CoachScreen, FusedRecordScreen, HealthScreen, PipBar, SettingsScreen, StepsCalibrationScreen, UpdatesInboxScreen, WorkoutsScreen). Per the ryanbr#558 pattern these land in the baseline as tracked backlog (170 -> 192 android) rather than forcing 22 rushed translations; --ci stays green and they get localized incrementally. NO new hardcoded literal can still land. Tested (Tools-only Python, fully runnable — no app-target/CI gap): python3 -m unittest test_i18n_audit -> 26/26 (4 new: semantics idiom, multi-line, decl/member exclusion, and the no-reopen-unrelated-slot guard) python3 Tools/i18n_audit.py --ci origin/main -> exit 0
Summary
Fixes #540:
Tools/i18n_audit.py's Android regex only matched a literal sitting immediately afterText(/title=/etc, soText(if (saved) "Saved" else "Save")slid straight past and the audit reported "clean" while shipping English regardless of device language.While fixing this I checked the Apple side for parity and found the identical blind spot — and there it's a real functional bug, not just a tooling gap:
Text(cond ? "a" : "b")resolves to SwiftUI's non-localizingText<S: StringProtocol>overload, so those sites are always-English on-device too, independent of this audit.What changed
Both scanners now walk each call's/kwarg's own argument span instead of matching a literal at a fixed position:
(/[always (so ternaries and nested calls are visible).{only for the Kotlin idioms this codebase actually uses to conditionally pick a string (if/when/catch/else/try/finally, and.let {}as a null-coalescing ternary substitute paired with?:). Opaque for everything else, so an unrelated nested composable slot lambda (e.g.AlertDialog(text = { Column { /* a separate, unrelated subtree */ } })) doesn't get swept into the same scan — that nested content is still found, just on its own independent pass when the file-wide scan reaches it directly.An early draft that didn't distinguish those two cases produced 489 spurious findings instead of the real backlog. Two more bugs surfaced along the way, fixed in the same pass since they'd otherwise produce un-actionable noise:
${...}string-template interpolation (the"${if (n==1) "day" else "days"}"pluralization idiom) corrupted the naive quote-matching.title=/text=/etc. also matched a plain localval text = when {...}declaration, not just an actual call argument.Deliberately does not flag
+-concatenated literals (typicallyuiString(R.string.x) + "hardcoded suffix") — found 86 of those across 28 files, but it's a different, larger bug (partial localization via an engineered prefix, needs real new translation content per site rather than reusing an existing value) — filed separately as #557.The backlog problem, and the baseline
The corrected scanners find real, previously-invisible findings: 170 distinct Android literals, 64 distinct Apple ones. Far more than one PR should close with rushed translations (see #543 on what that produces when strings get "fixed" without native review).
Since
i18n-coverage.yml's--ciis a whole-tree gate (not diff-scoped), the fix and some resolution of what it newly surfaces have to land together or every subsequent PR breaks. Rather than rush ~234 translations, this ships withTools/i18n_audit_baseline.json: a snapshot of the pre-existing findings, keyed by(path, literal)so it survives unrelated line-number drift.--cinow only fails on a new hardcoded literal from this point forward; the baseline backlog is closed incrementally in separate, appropriately-sized follow-up PRs, chipped away by area.--update-baselineregenerates the file — an entry that stops appearing in a fresh scan is simply inert, not an error, so it naturally shrinks as follow-ups land.Testing
Tools/test_i18n_audit.py(new): the conditional shapes above (ternary, block-if/else,when,.let), the nested-slot-lambda non-explosion regression guard, the interpolation and local-declaration fixes, comment/format-spec false-positive exclusion, and the baseline diff logic (missing file, round-trip, line-number-drift resilience).python3 -m unittest test_i18n_audit -v— 22/22 pass.python3 Tools/i18n_audit.py --ci origin/main— exit 0 (clean against the new baseline).--ciwith a clear message, then confirmed the repo is unaffected (temporary local-only edit, reverted,git statusclean before this PR).Tools/i18n_audit.pyisn't part of any Xcode/Gradle target) — only the Python audit tool, its new baseline snapshot, and its test file.xcodebuild/./gradlew compile*weren't needed for this change.Follow-ups tracked separately
uiString(...) + "hardcoded suffix"concatenation bug found while investigating this.Tools/i18n_audit_baseline.json's 234 pre-existing entries, screen/area by screen/area.