feat(watchos): system parity (preferences/keychain/locale) + i18n runtime locale selection + t() interpolation fix - #6201
Merged
Conversation
added 7 commits
July 4, 2026 22:16
Intl.ListFormat with locale "es"/"es-*" and type "unit" must join with the
Spanish connectors from CLDR:
- long: "foo, bar y baz" (last boundary " y ", pair " y ")
- short: "foo, bar, baz" (comma-joined for 3+, pair still " y ")
- narrow: "foo bar baz" (space-joined, same as the base — unchanged)
list_separators was hardcoded to the en-US patterns, so es lists came out
comma-joined ("foo, bar" instead of "foo y bar").
Thread the resolved locale through list_format_parts / list_separators and
add the es unit branch. The single DurationFormat caller passes "en-US"
explicitly to preserve its existing (locale-agnostic) output.
Fixes test262 intl402/ListFormat/prototype/{format,formatToParts}/
{es-es-long,es-es-short}.js (4 cases); es-es-narrow stays passing.
…ults + SecItem + NSLocale impls Replace the empty perry_system_preferences_set/get, perry_system_keychain_save/get/delete and perry_system_get_locale stubs in perry-ui-watchos with real implementations in a new src/system.rs, ported from perry-ui-ios (ffi/camera.rs preferences, ffi/security_notifications.rs keychain + locale). Foundation and Security.framework are already on the watch link line; exported extern signatures match what the stubs declared, so no dispatch changes. arm64_32 (ILP32) note: the runtime externs are declared with the real perry-runtime signatures (js_string_from_bytes(ptr, len: u32) -> *mut u8) and pointers only widen to i64 via zero-extending casts at the NaN-box boundary; NSStringEncoding is passed as usize (NSUInteger is 32-bit on arm64_32). Checked for aarch64-apple-watchos-sim, aarch64-apple-watchos and arm64_32-apple-watchos.
detect_apple_locale (NSBundle preferredLocalizations with a CFLocaleCopyCurrent fallback) was gated to macos+ios, so the other three Apple targets silently fell through to LANG/LC_* env vars that GUI apps never have, and perry/i18n apps always rendered locale index 0. Both APIs exist on every Apple OS; widen the two cfg gates. No link-line change needed: the non-game-loop watch path links with swiftc which pulls in libobjc implicitly (verified: _objc_getClass / _sel_registerName bind from libobjc.A.dylib in a linked simulator app), and the game-loop path already passes -lobjc explicitly.
… repaint on attribute-only mutations Two gaps in the SwiftUI tree renderer: 1. buttonView rendered Button(nodeText) only. The default watchOS button style paints its own opaque chrome over anything CommonModifiers hangs outside the Button, so font size/weight, buttonSetTextColor, widgetSetBackgroundColor and setCornerRadius never showed. When a node carries any of those, render a .plain-style button and style the label directly (same fontSize > 0 / fontWeight >= 0 fallback logic as textView), full-width with a stock-like pill fallback so styled buttons keep filling their cell; unstyled buttons keep the stock system path unchanged. CommonModifiers skips fg/bg/cornerRadius for button nodes so nothing double-applies. 2. CommonModifiers' only stored property was nodeId, which compares equal across render passes, so SwiftUI never re-ran its body on tree version bumps: attribute-only mutations (bg/fg color, corner radius, opacity, frame...) were recorded in the node model (with_node_mut does bump the version) but only became visible after a structural rebuild (e.g. widgetSetHidden toggle). Thread bridge.version through the modifier so its value changes every bump and body re-runs.
…wering
t("Day streak: {days}", { days: 5 }) printed the raw "{days}"
placeholder for EVERY params object. Root cause: closed-shape object
literals are rewritten during HIR lowering — before the i18n transform
runs — into Expr::New on a synthesized __AnonShape_<hash> class whose
constructor takes the field values positionally (perry-hir
lower/context.rs::synthesize_anon_shape_class). extract_params only
recognized Expr::Object, so it returned no params, and codegen's
fragment plan (dyn_extern_i18n) deliberately emits the literal {name}
text for placeholders missing from lowered_params.
Fix: apply_i18n builds a map of every __AnonShape_* class to its
ordered field names (declaration order == constructor arg order, same
across modules thanks to content-addressed names) and extract_params
zips those names with the New args to recover the name→value pairs.
Also covers the localizable perry/ui widget path (Text/Button/...),
which shares extract_params.
New e2e test: literal / variable / property-get / multi-param string
forms all interpolate.
The watch bundler wrote no CFBundleLocalizations / CFBundleDevelopmentRegion and no .lproj directories, so NSBundle.mainBundle.preferredLocalizations — the primary source in perry_runtime::i18n::detect_apple_locale — negotiated against an unlocalized bundle and always answered the development language. A German watch (or per-app language override) still rendered the default locale. Mirror the visionOS bundler: bundle_for_watchos now takes the i18n table + config, declares CFBundleDevelopmentRegion (default_locale) and CFBundleLocalizations (all configured locales) in Info.plist, and emits <locale>.lproj/Localizable.strings via the shared write_lproj_localized_strings helper — the .lproj directories are what NSBundle actually scans during language negotiation.
t() / localized-widget strings were resolved at COMPILE TIME against the default locale (I18nString lowering used default_locale_idx only), so a German device still rendered English on every platform. The runtime's locale plumbing (perry_i18n_init, LOCALE_INDEX, detect_apple_locale) existed but nothing in emitted code ever called it — dead code since it landed. Wire it up end to end: - runtime: new perry_i18n_locale_index_for(locales_header, default_idx) — lazily detects the system locale on first call (NSBundle/CFLocale on Apple, Win32 / Android props / env vars elsewhere), matches it against the comma-separated configured locale list, caches the row index in LOCALE_INDEX. An explicit perry_i18n_set_locale_index or perry_i18n_init beats the lazy path (LOCALE_RESOLVED flag). - codegen: I18nLowerCtx carries the locale codes; the I18nString lowering resolves EVERY locale's template row. Keys translated identically everywhere (and single-locale builds) keep the old compile-time fast path with zero runtime cost; keys that differ emit one perry_i18n_locale_index_for call + per-locale branches that each build that row's fragment plan (params are lowered exactly once, up front, so side effects don't duplicate across branches). Template emission is factored into emit_i18n_template. Verified on the watch simulator (device language German): a [i18n] en/de/fr app renders 'Gut gemacht!' / 'Serie: 5' with interpolation, while an English host still renders the en rows. The e2e test asserts the multi-locale switch compiles, runs, and interpolates without pinning the host language.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds locale-aware i18n lowering and runtime locale selection, anon-shape param reconstruction in the transform pass, watchOS system FFI and bundle localization output, and SwiftUI watchOS button/modifier updates. ChangesMulti-locale i18n interpolation and runtime resolution
Anon-shape params support in i18n transform pass
watchOS system APIs and UI updates
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Codegen
participant Runtime
participant EmitI18nTemplate
Codegen->>Codegen: resolve per-locale templates from table
Codegen->>Runtime: call perry_i18n_locale_index_for
Runtime-->>Codegen: locale index
Codegen->>EmitI18nTemplate: emit selected locale template
EmitI18nTemplate-->>Codegen: boxed nan-string result
sequenceDiagram
participant JSRuntime
participant SystemRS as system.rs
participant SecItem as Security.framework
JSRuntime->>SystemRS: perry_system_keychain_save(key, value)
SystemRS->>SecItem: SecItemUpdate(query)
SecItem-->>SystemRS: errSecItemNotFound
SystemRS->>SecItem: SecItemAdd(query)
SecItem-->>SystemRS: status
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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.
Actionable comments posted: 2
🧹 Nitpick comments (4)
crates/perry-ui-watchos/swift/PerryWatchApp.swift (1)
316-327: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
styledButtonTextduplicatestextViewfont logic.The font fallback logic (fontSize > 0 → fontWeight >= 0 gate) is identical to
textView(lines 219–233). Extracting a shared helper would prevent these from diverging over time.♻️ Optional refactor: shared font helper
+ /// Shared font logic for textView and styledButtonText. + `@ViewBuilder` func styledText( + _ t: String, fontSize: Double, fontWeight: Double + ) -> some View { + if fontSize > 0 { + if fontWeight >= 0 { + Text(t).font(.system(size: fontSize, weight: swiftWeight(fontWeight))) + } else { + Text(t).font(.system(size: fontSize)) + } + } else { + Text(t) + } + }Then
textViewbecomesstyledText(nodeText, fontSize: ..., fontWeight: ...)andstyledButtonTextdelegates to the same helper.🤖 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 `@crates/perry-ui-watchos/swift/PerryWatchApp.swift` around lines 316 - 327, `styledButtonText` is duplicating the same font fallback rules already used by `textView`, so extract that shared font-building logic into one helper and have both `textView` and `styledButtonText` delegate to it. Keep the `fontSize > 0` and `fontWeight >= 0` behavior in the shared helper, and update both call sites to use the new helper so the logic stays consistent and won’t drift.crates/perry-runtime/src/intl/list_relative_plural.rs (1)
126-159: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding unit tests for the new Spanish separator branch.
list_separatorsis a pure function with no I/O, making it cheap to unit-test directly (e.g. asserting the tuple returned for("es", "unit", "long"),("es-MX", "unit", "short"), and the narrow-falls-through-to-base case). This locks in the CLDR-derived behavior described in the comment and guards against regressions when more locales are added later.🤖 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 `@crates/perry-runtime/src/intl/list_relative_plural.rs` around lines 126 - 159, Add unit tests for the new Spanish branch in list_separators to lock in the CLDR behavior. Cover the es and es-* cases for list_type "unit" with style "long" and "short", and verify the "narrow" case falls through to the base separator behavior. Keep the tests focused on the pure list_separators function so future locale changes don’t regress this logic.crates/perry/tests/i18n_interpolation_params.rs (1)
106-179: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest accepts either locale row — doesn't actually verify runtime locale selection picked the right one.
The comment acknowledges the host's language can't be pinned, but the assertion accepting either
"Day streak: 9"or"Serie: 9 Tage"means this test would still pass even if locale detection always fell back to the default locale and never actually selected "de" — it only proves the runtime-switch codegen compiles and interpolates params correctly for whichever branch happens to run.Consider setting
LANG/LC_ALLon the child process (viaCommand::env(...)) when spawningrun, rather than relying on the host's ambient locale. That pins only the compiled binary's environment (not the host machine, respecting the stated constraint) and would let the test deterministically assert the "de" row is selected.♻️ Sketch of a more deterministic assertion
- let run = Command::new(&output).output().expect("run compiled binary"); + let run = Command::new(&output) + .env("LANG", "de_DE.UTF-8") + .env("LC_ALL", "de_DE.UTF-8") + .output() + .expect("run compiled binary");Effectiveness may depend on how the runtime reads the OS locale on the CI platform (env-var based on Linux vs. NSLocale-based on Apple targets per the PR description), so this is worth confirming before tightening the assertion to require the "de" row specifically.
🤖 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 `@crates/perry/tests/i18n_interpolation_params.rs` around lines 106 - 179, The test in t_multi_locale_runtime_selection_interpolates is too permissive because it accepts either locale row, so it never proves runtime locale selection actually chooses the non-default branch. Update the run invocation using Command::env to set the child process locale (for example LANG/LC_ALL) before executing the compiled binary, and then tighten the assertion to expect the intended locale row with {days} interpolated. Use the existing t_multi_locale_runtime_selection_interpolates test and its Command::new(&output) spawn point to locate the change.crates/perry-transform/src/i18n.rs (1)
501-528: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider guarding against field/arg count mismatch in
extract_params.
zipsilently truncates iffield_namesandargsdiffer in length, dropping params without any signal. Given the anon-shape synthesis contract this should never happen, but a debug assertion would catch HIR-lowering regressions early instead of producing silently incorrect interpolation.♻️ Proposed debug assertion
Expr::New { class_name, args, .. } => match anon_shapes.get(class_name) { Some(field_names) => { + debug_assert_eq!( + field_names.len(), + args.len(), + "anon-shape field count mismatch for {}: {} fields, {} args", + class_name, + field_names.len(), + args.len() + ); field_names .iter() .zip(args.iter()) .map(|(name, value)| (name.clone(), Box::new(value.clone()))) .collect() } None => Vec::new(), },🤖 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 `@crates/perry-transform/src/i18n.rs` around lines 501 - 528, Add a debug-time guard in extract_params for the Expr::New branch so mismatched anon-shape field_names and args lengths are caught instead of silently truncated by zip. Use the anon_shapes lookup and the class_name/args path to assert the counts match before mapping, and keep the existing field-name-to-arg pairing logic unchanged when they do.
🤖 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 `@crates/perry-codegen/src/expr/dyn_extern_i18n.rs`:
- Around line 234-279: The literal accumulation logic in the template parser
inside the byte-scanning loop is incorrectly using `buf.push(b as char)`, which
can mangle non-ASCII text when placeholders are present. Update the parser in
`dyn_extern_i18n.rs` so it buffers raw bytes for each literal segment and only
converts to UTF-8 once when flushing into `Part::Lit`, while keeping the
existing `Part::Param` handling and escape cases (`{{`, `}}`) intact.
In `@crates/perry-ui-watchos/src/system.rs`:
- Around line 184-206: The successful keychain lookup path in
perry_system_keychain_get is leaking the returned CFData from
SecItemCopyMatching. After converting the bytes with js_string_from_bytes and
before returning, explicitly release the result object when status is 0 and
result is non-null so the owned Core Foundation object does not accumulate on
repeated reads.
---
Nitpick comments:
In `@crates/perry-runtime/src/intl/list_relative_plural.rs`:
- Around line 126-159: Add unit tests for the new Spanish branch in
list_separators to lock in the CLDR behavior. Cover the es and es-* cases for
list_type "unit" with style "long" and "short", and verify the "narrow" case
falls through to the base separator behavior. Keep the tests focused on the pure
list_separators function so future locale changes don’t regress this logic.
In `@crates/perry-transform/src/i18n.rs`:
- Around line 501-528: Add a debug-time guard in extract_params for the
Expr::New branch so mismatched anon-shape field_names and args lengths are
caught instead of silently truncated by zip. Use the anon_shapes lookup and the
class_name/args path to assert the counts match before mapping, and keep the
existing field-name-to-arg pairing logic unchanged when they do.
In `@crates/perry-ui-watchos/swift/PerryWatchApp.swift`:
- Around line 316-327: `styledButtonText` is duplicating the same font fallback
rules already used by `textView`, so extract that shared font-building logic
into one helper and have both `textView` and `styledButtonText` delegate to it.
Keep the `fontSize > 0` and `fontWeight >= 0` behavior in the shared helper, and
update both call sites to use the new helper so the logic stays consistent and
won’t drift.
In `@crates/perry/tests/i18n_interpolation_params.rs`:
- Around line 106-179: The test in t_multi_locale_runtime_selection_interpolates
is too permissive because it accepts either locale row, so it never proves
runtime locale selection actually chooses the non-default branch. Update the run
invocation using Command::env to set the child process locale (for example
LANG/LC_ALL) before executing the compiled binary, and then tighten the
assertion to expect the intended locale row with {days} interpolated. Use the
existing t_multi_locale_runtime_selection_interpolates test and its
Command::new(&output) spawn point to locate the change.
🪄 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: f6efd531-f171-489e-84a4-3ea47b29be19
📒 Files selected for processing (14)
crates/perry-codegen/src/codegen/mod.rscrates/perry-codegen/src/expr/dyn_extern_i18n.rscrates/perry-codegen/src/expr/mod.rscrates/perry-codegen/src/runtime_decls/mod.rscrates/perry-runtime/src/i18n.rscrates/perry-runtime/src/intl/duration_format.rscrates/perry-runtime/src/intl/list_relative_plural.rscrates/perry-transform/src/i18n.rscrates/perry-ui-watchos/src/lib.rscrates/perry-ui-watchos/src/system.rscrates/perry-ui-watchos/swift/PerryWatchApp.swiftcrates/perry/src/commands/compile/bundle_apple.rscrates/perry/src/commands/compile/run_pipeline.rscrates/perry/tests/i18n_interpolation_params.rs
added 2 commits
July 9, 2026 20:55
…; release keychain CFData
- emit_i18n_template buffered literal bytes as `b as char`, re-encoding each
byte as a Unicode scalar and mangling non-ASCII text around placeholders
(e.g. `für {name}`). Buffer raw bytes, decode once per fragment. New
non-ASCII case in the interpolation regression test.
- keychain_get leaked the +1 CFData from SecItemCopyMatching/kSecReturnData;
release it after copying the bytes.
Addresses CodeRabbit review on #6201.
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
1 participant
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.
What
watchOS system parity, plus three cross-platform fixes surfaced while building a real standalone watch app (a kids' times-table trainer) against Perry.
The watchOS backend had
preferences,keychain, andgetLocalestubbed. This makes them real by porting the iOS implementations, then fixes three issues the app exposed:preferences/keychain/getLocale— realNSUserDefaults/SecItem/NSLocalebacking in a newcrates/perry-ui-watchos/src/system.rs(six empty stubs removed fromlib.rs). ILP32-hardened for arm64_32 (true runtime extern signatures, zero-extending pointer casts,usizeforNSUInteger).#[cfg]gates incrates/perry-runtime/src/i18n.rs(the Apple detector was gated to macOS/iOS only, so a German watch rendered English).PerryWatchApp.swift—CommonModifiers' only stored property wasnodeId, so SwiftUI value-diffed it as unchanged and never re-ran its body on version bumps; attribute-only mutations (bg/fg/font) never repainted, and styled buttons needed a structural rebuild to show. Fixed by threadingbridge.versionthrough the modifier and rendering styled buttons as.plainwith the style applied to the label.t(){param}interpolation (was broken on every platform) — closed-shape object literals lower toExpr::Newon synthesized__AnonShape_*classes before the i18n transform runs, soextract_params(which only matchedExpr::Object) saw no params and emitted the literal{name}. Fixed by mapping anon-shape classes to their ordered field names.bundle_for_watchosnow writesCFBundleDevelopmentRegion+CFBundleLocalizationsand emits<locale>.lproj/Localizable.strings(mirrors iOS/visionOS), soNSBundlelanguage negotiation actually sees the app's locales.t()translations were baked to the default locale at compile time on all platforms (perry_i18n_init/LOCALE_INDEXwere dead code). Addedperry_i18n_locale_index_for(lazy detect+match+cache); theI18nStringlowering now resolves every locale's row and branches at runtime when they differ, keeping the compile-time fast path when identical.Verification
cargo +nightly check -Z build-stdclean on watchos-sim, watchos (arm64), and arm64_32.perry-dispatch(5/5) andperry-api-manifestincl. stub_inventory (37/37, no count change). Two new e2e i18n tests. Verified end-to-end on the German watch simulator: strings render in German with{param}interpolation; prefs persist across relaunch.Caveats
-34018) — same pre-existing iOS-sim behavior; works on signed device builds.Note for reviewers
Touches
bundle_for_watchosinbundle_apple.rs, which also changes in the App Groups PR (#watchos-appgroups) — trivial rebase (localization writes before codesign).🤖 Generated with Claude Code
Summary by CodeRabbit
Localizable.stringsgeneration in app bundles.