feat(settings): adopt contract language catalogs - #154
Conversation
📝 WalkthroughWalkthroughLanguage settings now use revisioned catalog metadata, platform-localized labels, canonical language identities, and server-provided suggestions. Profile state propagates these suggestions to Android and TV pickers, which use keyed option and wire-value mappings. ChangesLanguage settings presentation and suggestions
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant EffectiveSettingsAPI
participant ProfileSettingsController
participant SettingsViewModel
participant LanguageOptions
participant SettingsPicker
EffectiveSettingsAPI->>ProfileSettingsController: Return effective values and suggested_values
ProfileSettingsController->>SettingsViewModel: Provide language suggestions in snapshot
SettingsViewModel->>SettingsPicker: Pass current values and suggestions
SettingsPicker->>LanguageOptions: Build keyed options and labels
LanguageOptions-->>SettingsPicker: Return localized labels and wire mappings
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
shared/src/androidUnitTest/kotlin/org/siloserver/silo/model/settings/SettingsConformanceTest.kt (1)
190-207: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider also asserting the reverse direction.
The loop only walks generated
DEFINITIONS, so a manifest definition that carriessuggested_options/unset_labelbut is absent from the generated bindings passes silently — the same vendoring drifttheVendoredManifestCoversTheGeneratedBindingsexists to catch.♻️ Suggested addition
) } + + val manifestPresentationKeys = manifest.definitions + .filter { it.suggestedOptions != null || it.unsetLabel != null } + .map { it.key } + .toSet() + assertEquals( + manifestPresentationKeys, + SettingPresentationMetadata.DEFINITIONS.keys, + "the manifest and the generated presentation metadata were vendored from " + + "different server commits", + ) }🤖 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 `@shared/src/androidUnitTest/kotlin/org/siloserver/silo/model/settings/SettingsConformanceTest.kt` around lines 190 - 207, Extend generatedPresentationMetadataMatchesTheVendoredManifest to also iterate manifest definitions and verify that every definition containing suggested_options or unset_label has a corresponding entry in SettingPresentationMetadata.DEFINITIONS, reusing the existing generated-binding coverage assertion pattern where appropriate.shared/src/commonMain/kotlin/org/siloserver/silo/model/settings/LanguageOptions.kt (2)
82-87: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winHoist the
Regexout ofisPreservableTag; it is recompiled on every candidate.
namedOptionscalls this once per catalog entry, runtime value, and current value, so each picker build recompiles the pattern ~40 times. Also note the pattern accepts_separators, so an underscore-spelled tag is kept verbatim invaluesand can be written back to the wire — consider normalizing to-inadd.♻️ Proposed change
- private fun isPreservableTag(value: String): Boolean = - value.isNotBlank() && - !value.equals("Off", ignoreCase = true) && - !value.equals("Default", ignoreCase = true) && - Regex("^[a-zA-Z]{2,3}([-_][a-zA-Z0-9]{1,8})*$").matches(value) && - canonicalSubtitleLanguage(value) != null + private val TAG_SHAPE = Regex("^[a-zA-Z]{2,3}([-_][a-zA-Z0-9]{1,8})*$") + + private fun isPreservableTag(value: String): Boolean = + value.isNotBlank() && + !value.equals("Off", ignoreCase = true) && + !value.equals("Default", ignoreCase = true) && + TAG_SHAPE.matches(value) && + canonicalSubtitleLanguage(value) != null🤖 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 `@shared/src/commonMain/kotlin/org/siloserver/silo/model/settings/LanguageOptions.kt` around lines 82 - 87, Hoist the language-tag Regex used by isPreservableTag into a reusable class-level or file-level constant so picker construction does not recompile it for each candidate. Preserve the existing validation behavior, including support for both hyphen and underscore separators; do not change add normalization unless required elsewhere.
64-66: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDisplay labels are used as the identity for wire resolution. Localized labels are CLDR-derived and locale-dependent, so they are not a reliable key: colliding labels resolve to the wrong tag and a label formatted under a different default locale falls back to
UNSET.
shared/src/commonMain/kotlin/org/siloserver/silo/model/settings/LanguageOptions.kt#L64-L66: resolve the selection by index or wire value instead of matchingit.second == label.shared/src/commonTest/kotlin/org/siloserver/silo/model/settings/LanguageOptionsTest.kt#L64-L81: once the API is wire/index-keyed, assert the round trip on wire values so the test no longer depends on the test JVM's default locale.🤖 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 `@shared/src/commonMain/kotlin/org/siloserver/silo/model/settings/LanguageOptions.kt` around lines 64 - 66, The wireValue function in shared/src/commonMain/kotlin/org/siloserver/silo/model/settings/LanguageOptions.kt lines 64-66 must resolve selections by stable wire value or index, not the localized display label; update its callers/API as needed while preserving UNSET for invalid selections. Update shared/src/commonTest/kotlin/org/siloserver/silo/model/settings/LanguageOptionsTest.kt lines 64-81 to verify round trips using wire values and remove dependence on the default locale.shared/src/androidMain/kotlin/org/siloserver/silo/model/settings/LanguagePresentation.android.kt (1)
11-28: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMemoize the ISO-3 → ISO-2 lookup; the dead
isEmpty()branch can go.Each 3-letter tag scans all ~188
Locale.getISOLanguages()entries and constructs aLocaleper candidate insiderunCatching, and this runs once per picker candidate.splitnever yields an empty list, so line 13 is unreachable.♻️ Proposed change
+private val iso3ToIso2: Map<String, String> by lazy { + Locale.getISOLanguages().mapNotNull { two -> + runCatching { Locale(two).isO3Language }.getOrNull() + ?.takeIf { it.isNotEmpty() } + ?.lowercase(Locale.ROOT) + ?.let { it to two } + }.toMap() +} + internal actual fun canonicalLanguageIdentity(tag: String): String { val parts = tag.replace('_', '-').split('-').toMutableList() - if (parts.isEmpty()) return tag.lowercase(Locale.ROOT) - val primary = parts.first().lowercase(Locale.ROOT) - val canonicalPrimary = if (primary.length == 3) { - Locale.getISOLanguages().firstOrNull { twoLetter -> - runCatching { - Locale.forLanguageTag(twoLetter).isO3Language.equals(primary, ignoreCase = true) - } - .getOrDefault(false) - } ?: primary - } else { - primary - } - parts[0] = canonicalPrimary + parts[0] = if (primary.length == 3) iso3ToIso2[primary] ?: primary else primary return parts.joinToString("-").lowercase(Locale.ROOT) }🤖 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 `@shared/src/androidMain/kotlin/org/siloserver/silo/model/settings/LanguagePresentation.android.kt` around lines 11 - 28, Update canonicalLanguageIdentity so the ISO-3-to-ISO-2 lookup is memoized and reused across calls instead of scanning Locale.getISOLanguages() and constructing candidate Locales for every 3-letter tag. Remove the unreachable parts.isEmpty() branch, while preserving the existing fallback to the primary tag and canonical lowercase output.
🤖 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.
Nitpick comments:
In
`@shared/src/androidMain/kotlin/org/siloserver/silo/model/settings/LanguagePresentation.android.kt`:
- Around line 11-28: Update canonicalLanguageIdentity so the ISO-3-to-ISO-2
lookup is memoized and reused across calls instead of scanning
Locale.getISOLanguages() and constructing candidate Locales for every 3-letter
tag. Remove the unreachable parts.isEmpty() branch, while preserving the
existing fallback to the primary tag and canonical lowercase output.
In
`@shared/src/androidUnitTest/kotlin/org/siloserver/silo/model/settings/SettingsConformanceTest.kt`:
- Around line 190-207: Extend
generatedPresentationMetadataMatchesTheVendoredManifest to also iterate manifest
definitions and verify that every definition containing suggested_options or
unset_label has a corresponding entry in
SettingPresentationMetadata.DEFINITIONS, reusing the existing generated-binding
coverage assertion pattern where appropriate.
In
`@shared/src/commonMain/kotlin/org/siloserver/silo/model/settings/LanguageOptions.kt`:
- Around line 82-87: Hoist the language-tag Regex used by isPreservableTag into
a reusable class-level or file-level constant so picker construction does not
recompile it for each candidate. Preserve the existing validation behavior,
including support for both hyphen and underscore separators; do not change add
normalization unless required elsewhere.
- Around line 64-66: The wireValue function in
shared/src/commonMain/kotlin/org/siloserver/silo/model/settings/LanguageOptions.kt
lines 64-66 must resolve selections by stable wire value or index, not the
localized display label; update its callers/API as needed while preserving UNSET
for invalid selections. Update
shared/src/commonTest/kotlin/org/siloserver/silo/model/settings/LanguageOptionsTest.kt
lines 64-81 to verify round trips using wire values and remove dependence on the
default locale.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c23b2936-9e7f-4d32-9ad1-65f5053df2ce
📒 Files selected for processing (19)
androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/settings/PlaybackSettings.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/settings/SettingsScreen.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/settings/SettingsViewModel.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/settings/SubtitleSettings.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/settings/TvSettingsScreen.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/settings/TvSettingsViewModel.ktshared/src/androidMain/kotlin/org/siloserver/silo/model/settings/LanguagePresentation.android.ktshared/src/androidUnitTest/kotlin/org/siloserver/silo/model/settings/SettingsConformanceTest.ktshared/src/commonMain/kotlin/org/siloserver/silo/domain/settings/ProfileSettingsController.ktshared/src/commonMain/kotlin/org/siloserver/silo/model/settings/LanguageOptions.ktshared/src/commonMain/kotlin/org/siloserver/silo/model/settings/SettingKeys.ktshared/src/commonMain/kotlin/org/siloserver/silo/model/settings/SettingValueModels.ktshared/src/commonTest/kotlin/org/siloserver/silo/domain/settings/ProfileSettingsControllerTest.ktshared/src/commonTest/kotlin/org/siloserver/silo/model/settings/LanguageOptionsTest.ktshared/src/commonTest/kotlin/org/siloserver/silo/model/settings/SettingsManifest.ktshared/src/commonTest/kotlin/org/siloserver/silo/network/api/SettingsApiValuesTest.ktshared/src/commonTest/resources/settings/v1/SOURCEshared/src/commonTest/resources/settings/v1/conformance.jsonshared/src/commonTest/resources/settings/v1/manifest.json
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21d70afada
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } ?: primary | ||
| } else { | ||
| primary |
There was a problem hiding this comment.
When a current or runtime value uses a deprecated two-letter alias such as iw, this branch preserves iw as a different identity from the generated he option, even though Android's Locale normalizes both and displays both as “Hebrew.” Both phone and TV therefore render duplicate labels, and wireValue() resolves either label to the first row (he), so the exact current iw row cannot round-trip as intended. Normalize two-letter aliases through Locale.forLanguageTag(...).language before deduplication.
Useful? React with 👍 / 👎.
| Regex("^[a-zA-Z]{2,3}([-_][a-zA-Z0-9]{1,8})*$").matches(value) && | ||
| canonicalSubtitleLanguage(value) != null |
There was a problem hiding this comment.
When the server or another client supplies und, which is a valid BCP 47 language tag, this predicate rejects it because canonicalSubtitleLanguage() deliberately treats und as null for track-selection semantics. The picker consequently omits the exact current value and displays the unset label even though the server still holds a nonempty setting, violating the open language_tag behavior this adapter is intended to preserve. Validate setting tags independently of subtitle track-selection normalization.
Useful? React with 👍 / 👎.
1 participant
Problem
Android phone and TV used a small hand-maintained language table, so metadata and subtitle pickers exposed fewer choices than the web app. The duplicated lists also made future drift between phone, TV, Apple, and web likely.
Approach
025083159f9624269483479cc05de02a822c6cd2suggested_values, and the exact current wire valueCanonical Server contract: Silo-Server/silo-server#521
Companion PRs
Verification
:shared:testDebugUnitTest— passed:androidApp:testDebugUnitTest— passed:androidApp:assembleDebug— passed:androidTvApp:testDebugUnitTest— passed:androidTvApp:assembleDebug— passedBUILD SUCCESSFULin 3m 22s, 152 actionable tasks0250831502508315Risks and follow-up
AI Disclosure
Summary by CodeRabbit
New Features
Bug Fixes