Use native iOS localization selection - #291
Conversation
|
Warning Review limit reached
Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesLocalization context
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SettingsContext
participant LocalizationContext
participant Bundle
SettingsContext->>LocalizationContext: provide selected locale
LocalizationContext->>Bundle: resolve supported localization
Bundle-->>LocalizationContext: return locale bundle
LocalizationContext->>SettingsContext: store validated locale
LocalizationContext->>Bundle: request localized string
Bundle-->>LocalizationContext: return active or development translation
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
@JJGatchalian This makes the automatic translation selection based on device language and region more intelligent by using the built-in iOS selection logic. MS Soundscape had it's own, much more basic selection logic, that would fall back to English or possibly the wrong dialect if an exact match wasn't found. E.g. it should now choose es-419 when the device is set to es-US or es-MX. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/ios/GuideDogs/Code/Language/Localization/LocalizationContext.swift (1)
87-121: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUnsynchronized static cache mutated inside a computed-property getter.
currentAppLocale's getter mutates the shared static_currentAppLocale/currentAppLocaleBundleon every access, andlocalizedString(_:)(line 215) forces this getter to run on every single localized-string lookup. If any callout/TTS generation or other work happens off the main thread whilelocalizedStringis also invoked elsewhere concurrently, these unsynchronized writes to shared static state are a data race that can corrupt the cachedBundlereference or cause crashes under contention. Relying on getter side effects to "warm" the cache (as done at line 215's_ = currentAppLocale) also makes the dependency implicit and easy to break in a future refactor.Consider guarding the read/update of
_currentAppLocale/currentAppLocaleBundlewith a lock (or a serial queue), and exposing the "resolve + cache" step as an explicit function rather than as a side effect of reading a property.🤖 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 `@apps/ios/GuideDogs/Code/Language/Localization/LocalizationContext.swift` around lines 87 - 121, Make the currentAppLocale cache thread-safe by moving the resolve-and-cache logic out of the computed-property getter into an explicit function, such as a cache-refresh helper, and guard reads and updates of _currentAppLocale and currentAppLocaleBundle with a lock or serial queue. Update localizedString(_:) and other callers to invoke this helper explicitly instead of relying on `_ = currentAppLocale`, while preserving the existing locale resolution and bundle fallback behavior.
🧹 Nitpick comments (2)
apps/ios/GuideDogs/Code/Language/Localization/LocalizationContext.swift (2)
134-137: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winForce-unwrapped
Bundle(locale:)for the development bundle.
defaultDevelopmentBundle = Bundle(locale: developmentLocale)!crashes the app on first access if the development-locale.lprojbundle can't be resolved (e.g. resource-path or naming mismatch). Since this bundle is the ultimate fallback used bylocalizedString, a resolution failure here is exactly the scenario that fallback is supposed to survive. Consider falling back toBundle.main(with a logged error) instead of force-unwrapping.🤖 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 `@apps/ios/GuideDogs/Code/Language/Localization/LocalizationContext.swift` around lines 134 - 137, Update defaultDevelopmentBundle in LocalizationContext to avoid force-unwrapping Bundle(locale: developmentLocale); resolve the development bundle safely, log a resolution failure, and fall back to Bundle.main so localizedString remains usable when the development-locale bundle is unavailable.
213-222: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGetter side-effect dependency is implicit.
_ = currentAppLocaleis used purely to trigger the getter's cache-update side effect before readingcurrentAppLocaleBundle. This works but is easy to silently break (e.g. if the caching logic is later moved out of the getter). Consider a dedicatedresolveCurrentBundle()helper that makes the dependency explicit instead of discarding the getter's return value.🤖 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 `@apps/ios/GuideDogs/Code/Language/Localization/LocalizationContext.swift` around lines 213 - 222, Replace the discarded currentAppLocale getter call in the localization lookup flow with a dedicated resolveCurrentBundle() helper that explicitly performs the locale and bundle cache resolution before reading currentAppLocaleBundle. Preserve the existing localizedString fallback behavior and ensure the helper owns the required cache-update dependency.
🤖 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.
Outside diff comments:
In `@apps/ios/GuideDogs/Code/Language/Localization/LocalizationContext.swift`:
- Around line 87-121: Make the currentAppLocale cache thread-safe by moving the
resolve-and-cache logic out of the computed-property getter into an explicit
function, such as a cache-refresh helper, and guard reads and updates of
_currentAppLocale and currentAppLocaleBundle with a lock or serial queue. Update
localizedString(_:) and other callers to invoke this helper explicitly instead
of relying on `_ = currentAppLocale`, while preserving the existing locale
resolution and bundle fallback behavior.
---
Nitpick comments:
In `@apps/ios/GuideDogs/Code/Language/Localization/LocalizationContext.swift`:
- Around line 134-137: Update defaultDevelopmentBundle in LocalizationContext to
avoid force-unwrapping Bundle(locale: developmentLocale); resolve the
development bundle safely, log a resolution failure, and fall back to
Bundle.main so localizedString remains usable when the development-locale bundle
is unavailable.
- Around line 213-222: Replace the discarded currentAppLocale getter call in the
localization lookup flow with a dedicated resolveCurrentBundle() helper that
explicitly performs the locale and bundle cache resolution before reading
currentAppLocaleBundle. Preserve the existing localizedString fallback behavior
and ensure the helper owns the required cache-update dependency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0be1edee-2221-4b74-a819-64fc4e9242d5
📒 Files selected for processing (3)
apps/ios/GuideDogs.xcodeproj/project.pbxprojapps/ios/GuideDogs/Code/Language/Localization/LocalizationContext.swiftapps/ios/UnitTests/Language/LocalizationContextTest.swift
1 participant
Summary
en-USdevelopment localizationWhy
The existing fallback selected the first alphabetically sorted localization sharing a language code. That could disagree with iOS's regional matching. Delegating to
Bundle.main.preferredLocalizationsgives the system enough context to choose the appropriate supported localization.This deliberately does not add an
es-USbundle. With Soundscape's currentes-ESandes-419resources, Foundation selectses-419for anes-USpreference. Adding an empty exact-match bundle would prevent that behavior.Validation
LocalizationContextTest: 6 tests passed on iPhone 17 Pro simulator, iOS 26.4en-UScomment/whitespace warningsplutil -lintgit diff --checkpassesSummary by CodeRabbit
Bug Fixes
Tests