EXP-5874 Add strings for Rollouts toggle and update Studies strings #27
Conversation
WalkthroughThese changes introduce v148 localization support for Studies and Rollouts feature toggles across Firefox iOS and Focus iOS, including linter configuration updates, new localized strings across multiple languages, and a new text formatting utility class. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 1
🤖 Fix all issues with AI agents
In `@firefox-ios/Shared/Strings.swift`:
- Around line 6258-6261: The localized string for key
"Settings.Rollouts.Message.v148" uses a non-positional specifier ("%@") but the
comment documents a positional placeholder ("%1$@"); update the value to use a
positional specifier (replace "%@" with "%1$@") so it matches the comment and
supports reordering in translations; ensure you only change the value string for
that key in Strings.swift (or alternatively update the comment if you
intentionally want a non-positional specifier).
🧹 Nitpick comments (1)
focus-ios/Blockzilla/UIComponents/UIConstants.swift (1)
417-430: Consider using static methods instead of singleton.Since
SettingsTextFormatteris stateless and the methods are simpleString(format:)wrappers, static methods would be simpler and eliminate the singleton overhead:Alternative approach
-class SettingsTextFormatter { - static let shared = SettingsTextFormatter() - - private init() {} - - func formatStudiesText(appName: String) -> String { +enum SettingsTextFormatter { + static func formatStudiesText(appName: String) -> String { return String(format: strings.detailTextStudies, appName) } - func formatRolloutsText(appName: String) -> String { + static func formatRolloutsText(appName: String) -> String { return String(format: strings.detailTextRollouts, appName) } }Usage would change from
SettingsTextFormatter.shared.formatStudiesText(...)toSettingsTextFormatter.formatStudiesText(...).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/l10n/linter_config_ios.jsonfirefox-ios/Shared/Strings.swiftfirefox-ios/Shared/Supporting Files/en-CA.lproj/Settings.stringsfirefox-ios/Shared/Supporting Files/en-GB.lproj/Settings.stringsfocus-ios/Blockzilla/UIComponents/UIConstants.swift
🧰 Additional context used
🧬 Code graph analysis (1)
firefox-ios/Shared/Strings.swift (1)
firefox-ios/Client/Assets/CC_Script/TranslationsUtils.mjs (1)
key(118-118)
🔇 Additional comments (6)
focus-ios/Blockzilla/UIComponents/UIConstants.swift (3)
202-203: LGTM!The V2 localization key follows the versioning pattern for localization migration. The escaped quotes in comments properly document the placeholder usage.
205-209: LGTM!The new V2 Studies strings and Rollouts strings are well-defined with appropriate localization keys and clear user-facing descriptions.
423-425: Verify: Should this use the V2 string key?The formatter uses
strings.detailTextStudies(the original key), but this PR introducesdetailTextStudiesV2with updated wording. If the V2 strings are intended for new UI, consider usingdetailTextStudiesV2here for consistency:Suggested change
func formatStudiesText(appName: String) -> String { - return String(format: strings.detailTextStudies, appName) + return String(format: strings.detailTextStudiesV2, appName) }firefox-ios/Shared/Supporting Files/en-CA.lproj/Settings.strings (1)
238-261: LGTM!The new v148 localization entries for Studies and Rollouts are well-structured:
- Comments clearly describe each string's purpose
- The
%@placeholder for app name is consistent with existing patterns- Keys follow the established naming convention (
Settings.<Feature>.<Property>.v<version>)firefox-ios/Shared/Supporting Files/en-GB.lproj/Settings.strings (1)
238-261: LGTM!The new v148 localization entries for Studies and Rollouts are consistent with the en-CA locale and follow the established patterns. The British English locale appropriately uses the same text for these settings strings.
.github/l10n/linter_config_ios.json (1)
8-10: Document the rationale for excludingSettings.Studies.Message.v148from quote validation.The exclusion exists in the linter config but lacks any explanation. Add a comment or update documentation clarifying why this string requires bypass from quote checks, to help maintainers understand the configuration decision.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| key: "Settings.Rollouts.Message.v148", | ||
| tableName: "Settings", | ||
| value: "%@ will improve features, performance, and stability between updates. Changes applied remotely.", | ||
| comment: "A short description that explains that Mozilla may change features remotely. %1$@ is the app name (e.g. Firefox).") |
There was a problem hiding this comment.
Align placeholder spec with the comment (positional).
The comment documents %1$@, but the value uses %@. Use a positional specifier (or update the comment) to avoid confusion and allow reordering in localized strings.
🛠️ Proposed fix
- value: "%@ will improve features, performance, and stability between updates. Changes applied remotely.",
+ value: "%1$@ will improve features, performance, and stability between updates. Changes applied remotely.",📝 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.
| key: "Settings.Rollouts.Message.v148", | |
| tableName: "Settings", | |
| value: "%@ will improve features, performance, and stability between updates. Changes applied remotely.", | |
| comment: "A short description that explains that Mozilla may change features remotely. %1$@ is the app name (e.g. Firefox).") | |
| key: "Settings.Rollouts.Message.v148", | |
| tableName: "Settings", | |
| value: "%1$@ will improve features, performance, and stability between updates. Changes applied remotely.", | |
| comment: "A short description that explains that Mozilla may change features remotely. %1$@ is the app name (e.g. Firefox).") |
🤖 Prompt for AI Agents
In `@firefox-ios/Shared/Strings.swift` around lines 6258 - 6261, The localized
string for key "Settings.Rollouts.Message.v148" uses a non-positional specifier
("%@") but the comment documents a positional placeholder ("%1$@"); update the
value to use a positional specifier (replace "%@" with "%1$@") so it matches the
comment and supports reordering in translations; ensure you only change the
value string for that key in Strings.swift (or alternatively update the comment
if you intentionally want a non-positional specifier).
Benchmark PR from agentic-review-benchmarks#1
Summary by CodeRabbit
New Features
Localization
✏️ Tip: You can customize this high-level summary in your review settings.