feat(mobile): add i18n infrastructure and language settings - #2069
Conversation
Introduce the shared localization foundation for the mobile app: - i18next/react-i18next instance with bundled en/pl resources and an explicit English-fallback contract (every user-facing t() call must pass a fallback string or defaultValue). - App language model (system/en/pl) with persisted preference in appPreferencesStore, device-locale resolution (pl-* -> pl, en-* -> en, unsupported -> en) and splash-safe bootstrap that resolves the effective locale before the first screen renders. - One-time AppCompat migration marker for the later native App Language sync (@SparkyFitness/app-language-migration). - useScreenHeader primary Save labels fall back to localized common.save/common.saving with English fallbacks. - Representative language settings UI in AppSettingsScreen plus shell strings (Settings / App Settings) in App.tsx. - i18n:audit tooling (missing keys, duplicate keys, placeholder and plural mismatches, dynamic t(), hardcoded UI text baseline, and the new missing-English-fallback rule) with a fresh baseline captured from current main + this PR (zero new findings). - Jest mocks for expo-localization / expo-winter and localization, bootstrap, foreground-sync, audit and native-config test suites. Assisted-by: Open WebUI
Wire the native app-language surface for Android App Languages and iOS InfoPlist localization: - Expo config plugin (withAppLanguage) that adds the AppCompat dependency, registers AppLocalesMetadataHolderService with autoStoreLocales, installs the AppLanguage native module, and copies the Kotlin sources into the generated project (idempotent). - Native AppLanguageModule over AppCompatDelegate.setApplicationLocales: system clears the override, en/pl apply the locale, effective language is read from the configuration. - expo-localization config plugin declares supportedLocales (en/pl) for Android/iOS; app.config.ts wires the plugin and the iOS locales map. - Minimal en/pl native permission strings for iOS InfoPlist (camera, HealthKit read/write, local network). - Foreground reconciliation contract is implemented in the JS layer (useAppLanguageForegroundSync + syncAppLanguageFromSystem); this commit adds its config/native test coverage. Assisted-by: Open WebUI
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe mobile app adds English and Polish localization, manual language selection, Android native language integration, localized startup and navigation behavior, and an automated i18n audit with validation coverage. ChangesMobile localization
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
PR Validation ResultsChange Detection
|
AppSettingsScreen now renders the Language row with the explicit English fallback copy, exercising the in-app selector surface that Closes CodeWithCJ#1490. Assisted-by: Open WebUI
Remove unused variables in the audit test and use the project's T[] array syntax in the withAppLanguage manifest types so the mobile lint gate passes with --max-warnings 0. Assisted-by: Open WebUI
Correction round requested in review of the i18n infrastructure PR: - Remove the 24.6k-line hardcoded-UI baseline snapshot from the audit. Hardcoded English strings are now reported as informational only and never block i18n:audit (full inventory/migration moves to PR5). The PR drops from ~29.5k insertions to ~4.5k and becomes reviewable. - Keep every blocking rule intact: missing English fallback, dynamic t(), unsafe template-literal keys, missing locale keys, EN/PL structure mismatch, placeholder mismatch, plural mismatch, duplicate keys, forbidden legacy files, and invalid suppressions. - useScreenHeader: the default accessibility label for kind:'primary' now mirrors the resolved, localized visible label instead of the hard-coded English SAVE_LABEL (explicit caller accessibilityLabel still wins). Native busy path mirrors the busy label too. - Add useScreenHeader regression tests: EN Save/Save, PL Zapisz/Zapisz, busy disabled states, explicit accessibilityLabel override, and the native-path busy contract. Assisted-by: Open WebUI
CI lint (no-unused-vars) flags the unused useAppPreferencesStore import in the new useScreenHeader regression suite; the store reset helper is the only thing used there. Assisted-by: Open WebUI
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (13)
SparkyFitnessMobile/scripts/i18n-audit/core.cjs (1)
10-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive both forbidden lists from one source.
FORBIDDEN_FILESand the inline default insidecheckForbiddenFileslist the same three paths. The two lists can diverge when a new forbidden file is added. Define the relative names once and build both lists from them.♻️ Proposed refactor
+const FORBIDDEN_RELATIVE_FILES = [ + ['src', 'localization', 'mobile.pl.json'], + ['src', 'localization', 'mobile.pl.overrides.json'], + ['scripts', 'populate-mobile-polish.mjs'], +]; + +function defaultForbiddenFiles(rootDir) { + return FORBIDDEN_RELATIVE_FILES.map((parts) => path.join(rootDir, ...parts)); +} + -const FORBIDDEN_FILES = [ - path.join(MOBILE_ROOT, 'src', 'localization', 'mobile.pl.json'), - path.join(MOBILE_ROOT, 'src', 'localization', 'mobile.pl.overrides.json'), - path.join(MOBILE_ROOT, 'scripts', 'populate-mobile-polish.mjs'), -]; +const FORBIDDEN_FILES = defaultForbiddenFiles(MOBILE_ROOT); const SOURCE_ROOTS = [path.join(MOBILE_ROOT, 'src')]; function checkForbiddenFiles(rootDir, forbiddenFiles) { const errors = []; const files = forbiddenFiles && forbiddenFiles.length > 0 ? forbiddenFiles - : [ - path.join(rootDir, 'src', 'localization', 'mobile.pl.json'), - path.join(rootDir, 'src', 'localization', 'mobile.pl.overrides.json'), - path.join(rootDir, 'scripts', 'populate-mobile-polish.mjs'), - ]; + : defaultForbiddenFiles(rootDir);🤖 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 `@SparkyFitnessMobile/scripts/i18n-audit/core.cjs` around lines 10 - 26, Define the three forbidden file paths once as relative names, then derive both the module-level FORBIDDEN_FILES and checkForbiddenFiles’s fallback list from that shared source using the applicable root directory. Remove the duplicated inline path list while preserving the existing custom forbiddenFiles override behavior.SparkyFitnessMobile/scripts/audit-mobile-i18n.mjs (1)
7-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReport the output path when
--jsonis absent.If a user passes an output path without
--json, the script prints the human report and ignores the path. No message explains this. Consider writing the human report to the file, or printing a warning.🤖 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 `@SparkyFitnessMobile/scripts/audit-mobile-i18n.mjs` around lines 7 - 9, Update the argument handling and reporting flow in the audit script so a provided outputFile is not silently ignored when showJson is false. Either write the human-readable report to outputFile or emit a clear warning explaining that the path requires JSON output, while preserving the existing JSON flag behavior.SparkyFitnessMobile/scripts/i18n-audit/sourceScanner.cjs (1)
158-161: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove or use the unused scanner exports.
KNOWN_ICONSis exported but no code reads it;isLikelyFalsePositivedoes not consult it, so icon names pass only through the other heuristics.getSuppressionWithoutJustificationFindingsis defined but neither exported nor called. Remove both, or wire them into the audit.Also applies to: 514-518
🤖 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 `@SparkyFitnessMobile/scripts/i18n-audit/sourceScanner.cjs` around lines 158 - 161, Remove the unused KNOWN_ICONS export and the unreferenced getSuppressionWithoutJustificationFindings function from the scanner, unless they are intentionally integrated into the audit flow. If retained, update isLikelyFalsePositive to consult KNOWN_ICONS and invoke/export getSuppressionWithoutJustificationFindings where appropriate.SparkyFitnessMobile/scripts/i18n-audit/localeValidator.cjs (2)
100-131: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused helper and the unused parameter.
pluralFormsForis never called and is not exported.detectSingularPluralCollisionnever reads itsdataparameter; the caller at line 196 still passesenData/plData. Remove both to keep the audit module minimal.♻️ Proposed refactor
-/** - * Returns an array of plural-form suffixes (e.g. '_one', '_other') used in a - * locale for the given base key, when the key is a plural group. - */ -function pluralFormsFor(keys, base) { - const forms = []; - for (const key of keys) { - const kBase = getPluralBase(key); - if (kBase === base) { - forms.push(key.slice(base.length)); - } - } - return forms; -} - /** * Detects a plain (singular) key sharing its base with a plural group in the * same locale, e.g. both `item` and `item_one`/`item_other`. This is ambiguous * for i18next lookups and is a structural error that cannot be suppressed. */ -function detectSingularPluralCollision(data, groups, localeName) { +function detectSingularPluralCollision(groups, localeName) {Update the call site at lines 192-200 accordingly:
for (const localeName of ['en', 'pl']) { const isEn = localeName === 'en'; const groups = isEn ? enGroups : plGroups; - const data = isEn ? enData : plData; - const collisionErrors = detectSingularPluralCollision(data, groups, localeName); + const collisionErrors = detectSingularPluralCollision(groups, localeName);🤖 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 `@SparkyFitnessMobile/scripts/i18n-audit/localeValidator.cjs` around lines 100 - 131, Remove the unused pluralFormsFor helper, remove the unused data parameter from detectSingularPluralCollision, and update every call site to pass only the arguments the function reads, including the caller currently passing enData/plData.
366-378: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
samePlaceholderMultisetin the singular branches.Both blocks re-implement the length-plus-index comparison that
samePlaceholderMultisetalready provides. Call the helper to keep one comparison rule.♻️ Proposed refactor
- if (enPlaceholders.length !== plPlaceholders.length || - !enPlaceholders.every((p, i2) => p === plPlaceholders[i2])) { + if (!samePlaceholderMultiset(enPlaceholders, plPlaceholders)) {- if (enPlaceholders.length !== plPlaceholders.length || - !enPlaceholders.every((p, i) => p === plPlaceholders[i])) { + if (!samePlaceholderMultiset(enPlaceholders, plPlaceholders)) {Also applies to: 391-404
🤖 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 `@SparkyFitnessMobile/scripts/i18n-audit/localeValidator.cjs` around lines 366 - 378, Update the singular placeholder-mismatch checks in both branches around the visible comparison and the corresponding block near the alternate branch to call samePlaceholderMultiset instead of manually comparing lengths and indexed placeholder values. Preserve the existing error objects, keys, and messages.SparkyFitnessMobile/__tests__/scripts/i18nAudit.test.ts (2)
22-65: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
createFixtureStructureandcleanupFixturedo not needasync.Both functions use only synchronous
fscalls and contain noawait. Making them synchronous removes theawaitat every call site and shortens the tests.🤖 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 `@SparkyFitnessMobile/__tests__/scripts/i18nAudit.test.ts` around lines 22 - 65, Remove the async modifier from createFixtureStructure and cleanupFixture, since both use only synchronous filesystem operations. Update every call site to remove the corresponding await while preserving fixture setup and cleanup behavior.
723-750: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe forbidden-file tests depend on an ambiguous empty-array fallback.
auditRunpassesforbiddenFiles: [].checkForbiddenFilesinSparkyFitnessMobile/scripts/i18n-audit/core.cjsat line 20 treats an empty array as "not provided" and falls back to the default list built fromrootDir. These two tests therefore pass only because of that fallback, and no test can express "check no forbidden files". Pass the explicit fixture paths in these tests so the intent is clear.♻️ Proposed change for the first test
- const result = auditRun(tmpDir); + const result = auditRun(tmpDir, { + forbiddenFiles: [path.join(tmpDir, 'src', 'localization', 'mobile.pl.json')], + });🤖 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 `@SparkyFitnessMobile/__tests__/scripts/i18nAudit.test.ts` around lines 723 - 750, Update both tests in the “Forbidden files” suite to pass the relevant fixture path explicitly to auditRun/checkForbiddenFiles instead of relying on the empty-array fallback: use the created mobile.pl.json path in the first test and populate-mobile-polish.mjs path in the second. Preserve the existing assertions that each file is reported as forbidden.SparkyFitnessMobile/src/localization/appLanguage.ts (1)
23-25: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse a single preference normalizer.
i18n.tslines 46-51 define the same logic. Export it fromi18n.tsand import it here to keep one definition.🤖 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 `@SparkyFitnessMobile/src/localization/appLanguage.ts` around lines 23 - 25, Remove the duplicate normalizePreference implementation from appLanguage.ts, export the existing normalizer from i18n.ts, and import and reuse it in the app language flow while preserving the current fallback to 'system'.SparkyFitnessMobile/__tests__/localization/i18n.test.ts (1)
184-194: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTighten the English-branch assertion.
Line 189 uses
.not.toContain('missing.key'). That passes for many wrong return values. The Polish branch at Line 192 already uses an exact match. Use the same exact assertion for the English branch.💚 Proposed change
- expect(i18n.t('missing.key.with.fallback', 'Readable text')).not.toContain('missing.key'); + expect(i18n.t('missing.key.with.fallback', 'Readable text')).toBe('Readable text');🤖 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 `@SparkyFitnessMobile/__tests__/localization/i18n.test.ts` around lines 184 - 194, Update the English assertion in the `never leaks a raw translation key into the UI` test to require an exact `Readable text` result from `i18n.t`, matching the existing Polish assertion, rather than only checking that the value does not contain `missing.key`.SparkyFitnessMobile/__tests__/localization/appLanguage.test.ts (1)
111-177: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for a rejected native read after migration.
The suite covers a rejected
setApplicationLanguageduring migration at Lines 99-108. It does not cover a rejectedgetApplicationLanguageon the post-migration adopt path. That path currently has no error handling inadoptNativeState; see the comment onsrc/localization/appLanguage.tsLines 122-133.💚 Proposed test
+ it('keeps startup usable when the native read fails', async () => { + useAppPreferencesStore.setState({ languagePreference: 'pl' }); + mockNative.getApplicationLanguage.mockRejectedValue(new Error('bridge failure')); + + await expect(initializeAppLanguage()).resolves.toBe('pl'); + expect(i18n.resolvedLanguage).toBe('pl'); + });🤖 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 `@SparkyFitnessMobile/__tests__/localization/appLanguage.test.ts` around lines 111 - 177, Add a post-migration test in the “bootstrap after migration” suite that makes mockNative.getApplicationLanguage reject, calls initializeAppLanguage, and verifies the rejection is handled without failing initialization while preserving the store and i18n language state. Anchor the test to the existing initializeAppLanguage flow and use the established rejected-native-operation expectations from the migration tests.SparkyFitnessMobile/src/localization/i18n.ts (2)
117-126: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow the exported language-change surface.
applyLanguagePreferencechanges i18next without updating the preferences store or AppCompat.setAppLanguagePreferenceinappLanguage.tsis the intended entry point and keeps all three in sync. A future caller that picks this function desynchronizes them.Mark this function as internal, or remove the export and keep
applyEffectiveLanguageinappLanguage.tsas the only applier.🤖 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 `@SparkyFitnessMobile/src/localization/i18n.ts` around lines 117 - 126, Restrict the public language-change API around applyLanguagePreference so callers use setAppLanguagePreference, which synchronizes i18n, preferences, and AppCompat. Either remove its export while retaining internal use, or mark it internal, and preserve applyEffectiveLanguage as the only externally accessible direct language applier.
13-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport the persist key instead of duplicating it.
STORE_KEYis duplicated inappPreferencesStore.tsandi18n.ts. Export it fromappPreferencesStore.tsand import it here so both modules use the same AsyncStorage key.🤖 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 `@SparkyFitnessMobile/src/localization/i18n.ts` at line 13, Update STORE_KEY ownership in appPreferencesStore.ts by exporting the existing constant, then remove the duplicate declaration in i18n.ts and import the shared symbol there so both modules use the same AsyncStorage key.SparkyFitnessMobile/__tests__/config/withAppLanguage.test.ts (1)
75-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert meta-data after the second call.
The idempotency test checks only
servicelength on the second call. The plugin's existing-service branch replaces$but does not rewritemeta-data. A future regression that dropsautoStoreLocaleson re-application would still pass.♻️ Proposed additional assertion
expect(twice?.service).toHaveLength(1); + expect(twice?.service?.[0]?.['meta-data']?.[0]?.$?.['android:name']).toBe('autoStoreLocales'); + expect(twice?.service?.[0]?.['meta-data']?.[0]?.$?.['android:value']).toBe('true');🤖 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 `@SparkyFitnessMobile/__tests__/config/withAppLanguage.test.ts` around lines 75 - 88, Extend the idempotency test around addAppLocalesService to assert that the second result, twice, still contains the APP_LOCALES_SERVICE metadata with android:name "autoStoreLocales" and android:value "true". Keep the existing first-call assertions and service-length check unchanged.
🤖 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 `@SparkyFitnessMobile/__tests__/scripts/i18nAudit.test.ts`:
- Around line 8-16: Replace all any annotations in i18nAudit.test.ts with
explicit local audit types: define interfaces for AuditFinding, AuditError,
AuditResult, and AuditOptions, type the required LocaleValidator,
groupPluralKeys, collectFindings, and runAudit symbols accordingly, and change
assertion callback parameters to AuditError or AuditFinding plus extra to
Partial<AuditOptions>; do not suppress the no-explicit-any rule.
In `@SparkyFitnessMobile/AGENTS.md`:
- Line 67: Update the startup ownership bullet in AGENTS.md to state that
App.tsx’s useAppBootstrap owns language initialization, initial-route selection,
linking state, and splash hiding, replacing the outdated
useInitialRoute/useAppStartup guidance.
In `@SparkyFitnessMobile/scripts/i18n-audit/sourceScanner.cjs`:
- Around line 345-367: Remove the redundant ts.isJsxElement check from the outer
guard in the JSX scanning block, relying on isTextLikeElement(node) for element
validation. In the child-expression branch, treat child itself as the
JsxExpression node, validate it with ts.isJsxExpression(child), and pass
child.expression to literalText so expressions such as <Text>{'Hardcoded
English'}</Text> are recorded. Add or update a fixture covering this
expression-child form.
In `@SparkyFitnessMobile/src/hooks/useAppBootstrap.ts`:
- Around line 39-42: Remove the early return from the finally block in the
bootstrap flow around SplashScreen.hideAsync. Preserve the cancelled condition
while allowing finally to complete normally, and keep the resulting TypeScript
strict and Biome-compliant.
In `@SparkyFitnessMobile/src/hooks/useScreenHeader.tsx`:
- Around line 615-616: Update the native left-item signature near
resolveItemLabel and resolveItemBusyLabel to include the localized label value
for left text/primary items, matching the right-item signature behavior so
language changes rebuild the native item. Add a native-path test covering a
localized left text or primary item and verifying it updates after the locale
changes.
In `@SparkyFitnessMobile/src/localization/appLanguage.ts`:
- Around line 122-133: The native language read in adoptNativeState must handle
rejection by catching getApplicationLanguage failures and falling back to the
stored preference, matching runMigration’s resilience; update
SparkyFitnessMobile/src/localization/appLanguage.ts lines 122-133. Make no
direct change to SparkyFitnessMobile/src/localization/appLanguage.ts lines
201-214; confirm it is covered once adoptNativeState catches errors. Add a test
in SparkyFitnessMobile/__tests__/localization/appLanguage.test.ts lines 111-177
that rejects getApplicationLanguage after the migration marker exists and
verifies initializeAppLanguage resolves with the stored preference.
In `@SparkyFitnessMobile/src/localization/i18n.ts`:
- Around line 97-115: Update the failure handling in initializeI18n so that
after the primary and English fallback initialization both fail, it clears
initPromise when i18n.isInitialized remains false. Preserve the existing error
logging and fallback behavior, while allowing subsequent initializeI18n calls to
start a fresh initialization attempt.
In `@SparkyFitnessMobile/src/screens/AppSettingsScreen.tsx`:
- Around line 94-100: Update BottomSheetPicker to accept an accessibilityHint
prop and use it instead of the hardcoded default when provided. In
AppSettingsScreen’s language picker, pass the localized settings translation for
the hint, and add or update the relevant test to verify the Polish locale
announces the Polish hint.
---
Nitpick comments:
In `@SparkyFitnessMobile/__tests__/config/withAppLanguage.test.ts`:
- Around line 75-88: Extend the idempotency test around addAppLocalesService to
assert that the second result, twice, still contains the APP_LOCALES_SERVICE
metadata with android:name "autoStoreLocales" and android:value "true". Keep the
existing first-call assertions and service-length check unchanged.
In `@SparkyFitnessMobile/__tests__/localization/appLanguage.test.ts`:
- Around line 111-177: Add a post-migration test in the “bootstrap after
migration” suite that makes mockNative.getApplicationLanguage reject, calls
initializeAppLanguage, and verifies the rejection is handled without failing
initialization while preserving the store and i18n language state. Anchor the
test to the existing initializeAppLanguage flow and use the established
rejected-native-operation expectations from the migration tests.
In `@SparkyFitnessMobile/__tests__/localization/i18n.test.ts`:
- Around line 184-194: Update the English assertion in the `never leaks a raw
translation key into the UI` test to require an exact `Readable text` result
from `i18n.t`, matching the existing Polish assertion, rather than only checking
that the value does not contain `missing.key`.
In `@SparkyFitnessMobile/__tests__/scripts/i18nAudit.test.ts`:
- Around line 22-65: Remove the async modifier from createFixtureStructure and
cleanupFixture, since both use only synchronous filesystem operations. Update
every call site to remove the corresponding await while preserving fixture setup
and cleanup behavior.
- Around line 723-750: Update both tests in the “Forbidden files” suite to pass
the relevant fixture path explicitly to auditRun/checkForbiddenFiles instead of
relying on the empty-array fallback: use the created mobile.pl.json path in the
first test and populate-mobile-polish.mjs path in the second. Preserve the
existing assertions that each file is reported as forbidden.
In `@SparkyFitnessMobile/scripts/audit-mobile-i18n.mjs`:
- Around line 7-9: Update the argument handling and reporting flow in the audit
script so a provided outputFile is not silently ignored when showJson is false.
Either write the human-readable report to outputFile or emit a clear warning
explaining that the path requires JSON output, while preserving the existing
JSON flag behavior.
In `@SparkyFitnessMobile/scripts/i18n-audit/core.cjs`:
- Around line 10-26: Define the three forbidden file paths once as relative
names, then derive both the module-level FORBIDDEN_FILES and
checkForbiddenFiles’s fallback list from that shared source using the applicable
root directory. Remove the duplicated inline path list while preserving the
existing custom forbiddenFiles override behavior.
In `@SparkyFitnessMobile/scripts/i18n-audit/localeValidator.cjs`:
- Around line 100-131: Remove the unused pluralFormsFor helper, remove the
unused data parameter from detectSingularPluralCollision, and update every call
site to pass only the arguments the function reads, including the caller
currently passing enData/plData.
- Around line 366-378: Update the singular placeholder-mismatch checks in both
branches around the visible comparison and the corresponding block near the
alternate branch to call samePlaceholderMultiset instead of manually comparing
lengths and indexed placeholder values. Preserve the existing error objects,
keys, and messages.
In `@SparkyFitnessMobile/scripts/i18n-audit/sourceScanner.cjs`:
- Around line 158-161: Remove the unused KNOWN_ICONS export and the unreferenced
getSuppressionWithoutJustificationFindings function from the scanner, unless
they are intentionally integrated into the audit flow. If retained, update
isLikelyFalsePositive to consult KNOWN_ICONS and invoke/export
getSuppressionWithoutJustificationFindings where appropriate.
In `@SparkyFitnessMobile/src/localization/appLanguage.ts`:
- Around line 23-25: Remove the duplicate normalizePreference implementation
from appLanguage.ts, export the existing normalizer from i18n.ts, and import and
reuse it in the app language flow while preserving the current fallback to
'system'.
In `@SparkyFitnessMobile/src/localization/i18n.ts`:
- Around line 117-126: Restrict the public language-change API around
applyLanguagePreference so callers use setAppLanguagePreference, which
synchronizes i18n, preferences, and AppCompat. Either remove its export while
retaining internal use, or mark it internal, and preserve applyEffectiveLanguage
as the only externally accessible direct language applier.
- Line 13: Update STORE_KEY ownership in appPreferencesStore.ts by exporting the
existing constant, then remove the duplicate declaration in i18n.ts and import
the shared symbol there so both modules use the same AsyncStorage key.
🪄 Autofix
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: c12c91ae-7754-4f58-9922-3b63e3ea418a
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (36)
SparkyFitnessMobile/AGENTS.mdSparkyFitnessMobile/App.tsxSparkyFitnessMobile/__mocks__/expo-winter.jsSparkyFitnessMobile/__tests__/config/nativeLocales.test.tsSparkyFitnessMobile/__tests__/config/withAppLanguage.test.tsSparkyFitnessMobile/__tests__/hooks/useAppBootstrap.test.tsxSparkyFitnessMobile/__tests__/hooks/useAppLanguageForegroundSync.test.tsxSparkyFitnessMobile/__tests__/hooks/useScreenHeader.test.tsxSparkyFitnessMobile/__tests__/localization/appLanguage.test.tsSparkyFitnessMobile/__tests__/localization/i18n.test.tsSparkyFitnessMobile/__tests__/screens/AppSettingsScreen.test.tsxSparkyFitnessMobile/__tests__/scripts/i18nAudit.test.tsSparkyFitnessMobile/__tests__/stores/appPreferencesStore.test.tsSparkyFitnessMobile/app.config.tsSparkyFitnessMobile/jest.setup.jsSparkyFitnessMobile/locales/en.jsonSparkyFitnessMobile/locales/pl.jsonSparkyFitnessMobile/package.jsonSparkyFitnessMobile/plugins/withAppLanguage.tsSparkyFitnessMobile/scripts/audit-mobile-i18n.mjsSparkyFitnessMobile/scripts/i18n-audit/core.cjsSparkyFitnessMobile/scripts/i18n-audit/localeValidator.cjsSparkyFitnessMobile/scripts/i18n-audit/sourceScanner.cjsSparkyFitnessMobile/src/hooks/useAppBootstrap.tsSparkyFitnessMobile/src/hooks/useAppLanguageForegroundSync.tsSparkyFitnessMobile/src/hooks/useScreenHeader.tsxSparkyFitnessMobile/src/localization/appLanguage.tsSparkyFitnessMobile/src/localization/i18n.tsSparkyFitnessMobile/src/localization/index.tsSparkyFitnessMobile/src/localization/locales/en/translation.jsonSparkyFitnessMobile/src/localization/locales/pl/translation.jsonSparkyFitnessMobile/src/screens/AppSettingsScreen.tsxSparkyFitnessMobile/src/services/appLanguageNative.tsSparkyFitnessMobile/src/stores/appPreferencesStore.tsSparkyFitnessMobile/targets/android-language/kotlin/com/sparkyapps/sparkyfitness/language/AppLanguageModule.ktSparkyFitnessMobile/targets/android-language/kotlin/com/sparkyapps/sparkyfitness/language/AppLanguagePackage.kt
apedley
left a comment
apedley
left a comment
There was a problem hiding this comment.
- Dead code?
applyLanguagePreference, formatLocalizedNumber, getAppLocale, resolveLanguagePreference, getNativeApplicationLanguage have no callers. I checked the other PR and nothing in there calls them either. Is there yet another PR or is this dead code? applyLanguagePreference is also the only caller of the no-arg initializeI18n() path, so the AsyncStorage-parsing branch at i18n.ts:81-91 is dead in production and duplicates the store's persist shape. Make initializeI18n take a required language and drop the helpers
Same in the audit scripts: buildFingerprint, buildMigrationFingerprint, KNOWN_ICONS, and getSuppressionWithoutJustificationFindings in sourceScanner.cjs have zero consumers and look like leftovers of the removed baseline feature. Also withFileExtensions: true at sourceScanner.cjs:470 isn't a real readdirSync option, and the ts.isJsxElement && on line 345 is a truthy function check. The forbidden-files rule in core.cjs guards files that have never existed in this repo, that can go too.
- Language names are shown as endonyms not as english names.. so Polski instead Polish in the english ui
Review round for the i18n infrastructure PR (apedley + CodeRabbit): - initializeI18n(language) now requires the language; the dead AsyncStorage/Zustand-blob parsing branch and its STORE_KEY duplicate are removed. One owner for preference persistence (appPreferencesStore), one owner for native -> effective -> i18next (appLanguage.ts). - Init failure is retryable: if both the requested language and the English fallback fail, initPromise is cleared so a later bootstrap/foreground operation retries instead of permanently poisoning the session with a resolved failed promise. - Runtime errors use LogService (addLog ERROR/WARNING) instead of console.error/warn. - useAppBootstrap: language init and server-config loading are independent failure domains. A rejected initializeAppLanguage() is logged and never changes the route; a config failure still lands on Onboarding. Splash hiding is the last step, failure is logged and never rejects the unobserved determine() promise. - useInitialRoute removed from useAppStartup (dead after useAppBootstrap); AGENTS.md now documents App.tsx + useAppBootstrap as the owners of language init, initial route, linking state, and splash hiding. - Native language reads are resilient: getApplicationLanguage and getEffectiveLanguage rejections fall back to the stored preference / expo-localization device locale and are logged. - Foreground sync catches and logs resync rejections so an AppState callback can never leak an unhandled promise rejection. - Endonym labels: English catalog renders "Polski" (not "Polish"). - Settings language selection handles native-set failure: the previous stored/effective language is preserved, the error is logged, and a minimal settings.language.changeFailed error toast is shown. Assisted-by: Open WebUI
Android architecture correction per maintainer agreement (CodeWithCJ#1774): - Android 13+ (API 33+) uses the platform per-app language API via android.app.LocaleManager / applicationLocales in the native AppLanguageModule (thin bridge, SDK_INT-guarded). TS exposes supportsNativePerAppLanguage (Android && API >= 33 && module exists) and never calls native set/get on Android <=12. - Android 12 and below: no AppCompat locale APIs, no AppLocalesMetadataHolderService, no autoStoreLocales. The stored preference stays authoritative; manual en/pl work through i18next and `system` resolves through expo-localization. - The migration marker now applies only where native per-app language support exists: a stored manual preference seeds the platform locale exactly once after a successful handoff, and a failed handoff leaves the marker unset so it retries next bootstrap. A user who picked Polish on Android 12 keeps it after upgrading to Android 13. - Config plugin keeps only what is still needed: copying the Kotlin bridge sources and registering AppLanguagePackage in MainApplication (idempotent — verified with two consecutive `expo prebuild --clean --platform android` runs; no AppCompat dependency, service, or metadata is generated). - BottomSheetPicker gains an optional accessibilityHint prop threaded into PickerTrigger; the language picker passes the localized settings.language.pickerHint (EN/PL resources added). - useScreenHeader native left signature now carries the resolved localized label/busyLabel so a language change rebuilds the native left item; explicit caller accessibilityLabel still wins. Native-path tests cover EN/PL left primary items and busy labels. Assisted-by: Open WebUI
- sourceScanner: fix the JSX guard (was a truthy function check) and
the expression-child branch (child is the JsxExpression; pass
child.expression to literalText) so <Text>{'…'}</Text> and
<Text>{`…`}</Text> are inventoried like plain text. Fixtures cover
all three forms.
- Audit fails closed on scan failures: a source file that cannot be
read/parsed records a blocking `source-scan-error` (file + message)
and runAudit returns hasErrors=true; a broken-symlink fixture proves
coverage cannot silently shrink.
- Explicit-fallback validator now requires a statically readable
English fallback VALUE: literal second arg, defaultValue literal, or
defaultValue template literal. Dynamic defaultValue variables /
positional variables are rejected (tests added).
- runAudit derives default source roots from the ACTUAL rootDir
([rootDir/src]); custom-root fixture test added.
- Removed dead audit code: buildFingerprint, buildMigrationFingerprint,
KNOWN_ICONS, getSuppressionWithoutJustificationFindings, and the
invalid `withFileExtensions` readdirSync option.
- localeValidator: removed unused pluralFormsFor and the unused data
parameter from detectSingularPluralCollision; singular placeholder
checks now reuse samePlaceholderMultiset.
- Removed the forbidden-files legacy rule entirely (FORBIDDEN_FILES,
checkForbiddenFiles, report.forbidden, CLI output, tests) per apedley
— those files never existed in this repo.
- i18nAudit.test.ts: no `any` (explicit local audit types), sync
fixture helpers without async/await, exact toBe('Readable text')
assertion.
- CLI: an output file without --json now prints an explicit error
instead of silently ignoring the path; forbidden output removed.
Assisted-by: Open WebUI
|
@coderabbitai review |
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
SparkyFitnessMobile/__tests__/localization/appLanguage.test.ts (1)
48-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
mockReset()forsetApplicationLanguageto match the sibling mocks.Line 49 uses
mockClear(), while lines 50-51 usemockReset().mockClear()clears call records only. It does not drain queuedmockRejectedValueOnceimplementations. Tests at lines 149 and 171 queue one-time rejections. If a future test queues a one-time rejection that is not consumed, the rejection leaks into the next test and creates an order-dependent failure.mockReset()removes the queue, and line 55 re-installs the tracking implementation immediately afterwards.♻️ Proposed change
- mockNative.setApplicationLanguage.mockClear(); + mockNative.setApplicationLanguage.mockReset(); mockNative.getApplicationLanguage.mockReset();🤖 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 `@SparkyFitnessMobile/__tests__/localization/appLanguage.test.ts` around lines 48 - 57, Replace mockNative.setApplicationLanguage.mockClear() with mockReset() in the test setup, preserving the existing implementation installed immediately afterward so each test starts without queued one-time behaviors.SparkyFitnessMobile/scripts/i18n-audit/core.cjs (1)
184-186: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShare the
source-scan-errorrule name instead of duplicating the literal.
buildSummaryrecognizes scan errors by the string'source-scan-error'.collectFindingsinSparkyFitnessMobile/scripts/i18n-audit/sourceScanner.cjsproduces that same string independently. If the scanner rule name changes, this counter silently reports0, and the audit still blocks throughlocaleStructuralErrors.length, so no test fails. Export the rule name fromsourceScanner.cjsand reference it here.♻️ Proposed change
- sourceScanErrors: report.localeStructuralErrors.filter( - (e) => e.rule === 'source-scan-error', - ).length, + sourceScanErrors: report.localeStructuralErrors.filter( + (e) => e.rule === SOURCE_SCAN_ERROR_RULE, + ).length,Add the shared constant in
SparkyFitnessMobile/scripts/i18n-audit/sourceScanner.cjsand import it at the top ofcore.cjs:// sourceScanner.cjs const SOURCE_SCAN_ERROR_RULE = 'source-scan-error'; module.exports = { /* ...existing exports, */ SOURCE_SCAN_ERROR_RULE };🤖 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 `@SparkyFitnessMobile/scripts/i18n-audit/core.cjs` around lines 184 - 186, Define and export a shared SOURCE_SCAN_ERROR_RULE constant from sourceScanner.cjs, then import and use it in buildSummary’s sourceScanErrors filter instead of the duplicated 'source-scan-error' literal. Preserve the existing counting behavior and all other exports.SparkyFitnessMobile/__tests__/scripts/i18nAudit.test.ts (2)
787-838: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the suppression tests into their own
describeblock.These six tests cover suppression directives, not static key resolution. They currently sit inside
describe('Static key resolution'), which opens at line 720. Test-report grouping is misleading as a result. Close the static-key block before line 787 and open adescribe('Suppressions')block.🤖 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 `@SparkyFitnessMobile/__tests__/scripts/i18nAudit.test.ts` around lines 787 - 838, The six suppression-related tests should be grouped separately from static key resolution. Close the existing `describe('Static key resolution')` block before the suppression tests, then wrap the tests from “hardcoded suppression works” through “suppression does not hide a missing static key” in a new `describe('Suppressions')` block.
9-11: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueType
groupPluralKeyslike the other imported audit symbols.Lines 9, 14, and 20 give explicit types to
LocaleValidator,collectFindings, andrunAudit. Line 11 leavesgroupPluralKeyswith the implicitanythatrequirereturns, so the plural-grouping test at lines 917-927 is unchecked. Add an explicit signature.As per path instructions: "Keep TypeScript strict, type-safe, and compiling cleanly."
♻️ Proposed change
-const groupPluralKeys = localeMod.groupPluralKeys; +const groupPluralKeys = localeMod.groupPluralKeys as ( + keys: string[], +) => Record<string, string[]>;Adjust the return type to match the implementation in
SparkyFitnessMobile/scripts/i18n-audit/localeValidator.cjs.🤖 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 `@SparkyFitnessMobile/__tests__/scripts/i18nAudit.test.ts` around lines 9 - 11, Explicitly type the groupPluralKeys import in the i18n audit test, matching the implementation signature in localeValidator.cjs and the typing style used for LocaleValidator, collectFindings, and runAudit. Ensure the plural-grouping test receives a type-safe function rather than the implicit any from require.Source: Path instructions
🤖 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 `@SparkyFitnessMobile/__tests__/screens/AppSettingsScreen.test.tsx`:
- Around line 130-149: Await the asynchronous language-selection handler
returned by picker().onSelect?.('pl') inside the act callback, ensuring the
rejected native write and resulting toast/state updates complete before the
assertions. Keep the existing failure expectations unchanged.
In `@SparkyFitnessMobile/src/components/BottomSheetPicker.tsx`:
- Line 41: Update the default accessibilityHint in BottomSheetPicker so omitted
values use the existing i18n translation mechanism instead of hardcoded English,
while preserving caller-provided hints; alternatively require localized hints
from every caller, including the Theme picker in AppSettingsScreen, and add an
assertion covering the Polish fallback.
---
Nitpick comments:
In `@SparkyFitnessMobile/__tests__/localization/appLanguage.test.ts`:
- Around line 48-57: Replace mockNative.setApplicationLanguage.mockClear() with
mockReset() in the test setup, preserving the existing implementation installed
immediately afterward so each test starts without queued one-time behaviors.
In `@SparkyFitnessMobile/__tests__/scripts/i18nAudit.test.ts`:
- Around line 787-838: The six suppression-related tests should be grouped
separately from static key resolution. Close the existing `describe('Static key
resolution')` block before the suppression tests, then wrap the tests from
“hardcoded suppression works” through “suppression does not hide a missing
static key” in a new `describe('Suppressions')` block.
- Around line 9-11: Explicitly type the groupPluralKeys import in the i18n audit
test, matching the implementation signature in localeValidator.cjs and the
typing style used for LocaleValidator, collectFindings, and runAudit. Ensure the
plural-grouping test receives a type-safe function rather than the implicit any
from require.
In `@SparkyFitnessMobile/scripts/i18n-audit/core.cjs`:
- Around line 184-186: Define and export a shared SOURCE_SCAN_ERROR_RULE
constant from sourceScanner.cjs, then import and use it in buildSummary’s
sourceScanErrors filter instead of the duplicated 'source-scan-error' literal.
Preserve the existing counting behavior and all other exports.
🪄 Autofix
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: adc1288f-68e8-4d35-ace1-5fa90f83bbfe
📒 Files selected for processing (28)
SparkyFitnessMobile/AGENTS.mdSparkyFitnessMobile/__tests__/config/withAppLanguage.test.tsSparkyFitnessMobile/__tests__/hooks/useAppBootstrap.test.tsxSparkyFitnessMobile/__tests__/hooks/useAppLanguageForegroundSync.test.tsxSparkyFitnessMobile/__tests__/hooks/useAppStartup.test.tsSparkyFitnessMobile/__tests__/hooks/useScreenHeader.test.tsxSparkyFitnessMobile/__tests__/localization/appLanguage.test.tsSparkyFitnessMobile/__tests__/localization/i18n.test.tsSparkyFitnessMobile/__tests__/screens/AppSettingsScreen.test.tsxSparkyFitnessMobile/__tests__/scripts/i18nAudit.test.tsSparkyFitnessMobile/plugins/withAppLanguage.tsSparkyFitnessMobile/scripts/audit-mobile-i18n.mjsSparkyFitnessMobile/scripts/i18n-audit/core.cjsSparkyFitnessMobile/scripts/i18n-audit/localeValidator.cjsSparkyFitnessMobile/scripts/i18n-audit/sourceScanner.cjsSparkyFitnessMobile/src/components/BottomSheetPicker.tsxSparkyFitnessMobile/src/hooks/useAppBootstrap.tsSparkyFitnessMobile/src/hooks/useAppLanguageForegroundSync.tsSparkyFitnessMobile/src/hooks/useAppStartup.tsSparkyFitnessMobile/src/hooks/useScreenHeader.tsxSparkyFitnessMobile/src/localization/appLanguage.tsSparkyFitnessMobile/src/localization/i18n.tsSparkyFitnessMobile/src/localization/index.tsSparkyFitnessMobile/src/localization/locales/en/translation.jsonSparkyFitnessMobile/src/localization/locales/pl/translation.jsonSparkyFitnessMobile/src/screens/AppSettingsScreen.tsxSparkyFitnessMobile/src/services/appLanguageNative.tsSparkyFitnessMobile/targets/android-language/kotlin/com/sparkyapps/sparkyfitness/language/AppLanguageModule.kt
🚧 Files skipped from review as they are similar to previous changes (9)
- SparkyFitnessMobile/src/localization/locales/pl/translation.json
- SparkyFitnessMobile/src/screens/AppSettingsScreen.tsx
- SparkyFitnessMobile/src/localization/locales/en/translation.json
- SparkyFitnessMobile/scripts/audit-mobile-i18n.mjs
- SparkyFitnessMobile/src/hooks/useAppLanguageForegroundSync.ts
- SparkyFitnessMobile/scripts/i18n-audit/localeValidator.cjs
- SparkyFitnessMobile/src/hooks/useAppBootstrap.ts
- SparkyFitnessMobile/src/hooks/useScreenHeader.tsx
- SparkyFitnessMobile/scripts/i18n-audit/sourceScanner.cjs
- Await the async language-selection handler inside act in the AppSettingsScreen failure test so the toast/state assertions run after the rejected native write settles. - Use mockReset() for setApplicationLanguage in the appLanguage test setup to avoid leaking queued one-time rejections across tests. - Share the SOURCE_SCAN_ERROR_RULE constant between sourceScanner.cjs and core.cjs instead of duplicating the literal. - Type groupPluralKeys explicitly in i18nAudit.test.ts (no implicit any from require). Assisted-by: Open WebUI
|
Disposition of the four nitpick comments from the fresh CodeRabbit review (review id 4896297245):
|
|
@apedley — follow-up on your review; everything you flagged is addressed on this new HEAD (
Please wait with review. I need first build apk and test it on device. |
Final SPLIT-3 correctness pass: - Migration precedence on Android 13+: runMigration now reads the platform app language FIRST. An explicit Android Settings choice (en/pl) wins over the legacy stored preference and is adopted with no native write (cases A/B). A legacy explicit preference seeds the platform locale only when Android still follows System (cases C/D); system/system performs no needless write (case E). If the initial native read fails, the stored preference is used locally, the marker stays absent, and the migration retries next launch (case F) — native state is never overwritten when it could not be read. Unsupported native values are repaired to system explicitly and never written into the store. - setAppLanguagePreference is now transactional: previous store preference, previous effective language and (on Android 13+) previous native preference are snapshotted; i18n is applied and the store is committed LAST. A failed i18n apply rolls the native value back best-effort; if the rollback itself fails, the real native state is re-read and the store/i18n are reconciled to it so store/native/i18n can never knowingly contradict each other. Android <=12/iOS keep the store uncommitted on i18n failure. - AppSettingsScreen only awaits setAppLanguagePreference and shows the existing localized error toast (stale lower-layer comment fixed). Tests: explicit-native-wins (system/en/pl stores), legacy-seeds-system, system/system no-write, native-read-failure fallback, i18n-failure rollback on Android 13+, i18n-failure store-preservation on Android <=12, and rollback-failure reconciliation. Assisted-by: Open WebUI
|
@coderabbitai review |
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
SparkyFitnessMobile/src/localization/appLanguage.ts (1)
315-317: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow
i18n.resolvedLanguageinstead of asserting it.
resolvedLanguageis typed as a plain string. The assertion at Line 316 lets any resolved value flow intoapplyEffectiveLanguageduring post-rollback restoration.SUPPORTED_LANGUAGESandnormalizeLanguagealready exist insrc/localization/i18n.ts, so a guarded narrow removes the unchecked assertion without changing behavior for'en'/'pl'.♻️ Proposed narrowing
- const previousEffective = i18n.resolvedLanguage as SupportedLanguage | undefined; + const resolved = i18n.resolvedLanguage; + const previousEffective: SupportedLanguage | undefined = + resolved && (SUPPORTED_LANGUAGES as readonly string[]).includes(resolved) + ? (resolved as SupportedLanguage) + : undefined;Import
SUPPORTED_LANGUAGESfrom./i18nif it is not imported yet.🤖 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 `@SparkyFitnessMobile/src/localization/appLanguage.ts` around lines 315 - 317, Update the previousEffective assignment in the rollback restoration flow to narrow i18n.resolvedLanguage against SUPPORTED_LANGUAGES instead of asserting SupportedLanguage. Import SUPPORTED_LANGUAGES from ./i18n if needed, and ensure unsupported or absent values become undefined before being passed to applyEffectiveLanguage while preserving valid en/pl behavior.
🤖 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 `@SparkyFitnessMobile/src/localization/appLanguage.ts`:
- Around line 117-126: Update the doc comment for readNativePreference to state
that unsupported native language tags map to 'unsupported', while rejections
from AppLanguageNative.getApplicationLanguage() propagate to callers. Preserve
the existing try/catch handling at all call sites.
---
Nitpick comments:
In `@SparkyFitnessMobile/src/localization/appLanguage.ts`:
- Around line 315-317: Update the previousEffective assignment in the rollback
restoration flow to narrow i18n.resolvedLanguage against SUPPORTED_LANGUAGES
instead of asserting SupportedLanguage. Import SUPPORTED_LANGUAGES from ./i18n
if needed, and ensure unsupported or absent values become undefined before being
passed to applyEffectiveLanguage while preserving valid en/pl behavior.
🪄 Autofix
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: a9e5ecf5-55f9-4814-b331-60b8d3d493d0
📒 Files selected for processing (3)
SparkyFitnessMobile/__tests__/localization/appLanguage.test.tsSparkyFitnessMobile/src/localization/appLanguage.tsSparkyFitnessMobile/src/screens/AppSettingsScreen.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- SparkyFitnessMobile/src/screens/AppSettingsScreen.tsx
- Correct the readNativePreference doc comment: unsupported values map to 'unsupported', while rejected native reads propagate to callers (which all handle them) — the old wording implied the helper catches. - Narrow i18n.resolvedLanguage through SUPPORTED_LANGUAGES instead of an unchecked type assertion when snapshotting the previous effective language for rollback. Assisted-by: Open WebUI
|
Disposition of CodeRabbit review 4897318270 (SHA 47d26d5):
|
|
@apedley — one final Android 13+ migration edge case was found during review: an existing per-app language selected in Android Settings could be overwritten by the first legacy-preference handoff. The migration now reads native state first — an explicit Android App Language wins, while a legacy local en/pl preference seeds the native setting only when Android is still following System. I also made Settings language changes transactional so a failed i18n apply cannot leave store/native/i18n knowingly out of sync (rollback + reconciliation to actual state if the rollback itself fails). Scope remains infrastructure-only; the fresh dev-release APK for the final commit is at run 31394733850 ( |
|
@apedley tests done in real device. I don't have application language settings in my Redmi Note so I can't test that. |
|
@Dragonk THank you very much for splitting the PR. Two small things from claude. Could you take a look. @apedley might be on vacation. So, I will merge this PR for now once you take a look at the the below two items. 1. The endonym change only landed in
2. iOS advertises a language switcher we don't honor.
Can we drop |
|
@CodeWithCJ I’ll fix the endonym issue so both catalogs consistently show For iOS, I agree the current implementation can diverge: iOS exposes a per-app Language setting, while SparkyFitness may still retain a separate local The intended model was:
Because iOS does not provide a public equivalent of Android’s language setter, I propose to keep I’ll also preserve the existing safeguards: native state wins over stale local state, operations remain serialized, redundant writes are skipped, store updates happen only after successful native/i18next changes, Android writes retain rollback behavior, and foreground sync adopts external changes without creating a write-back loop. I don’t have access to an iPhone, Mac, or Xcode, so I can implement and cover the behavior with config and unit tests, but I’ll need help validating the final iOS build on a simulator or physical device before merge. |
|
Thanks. Sure, I can review through both simulator and a real iPhone when you are ready |
|
please tag me once when it is ready for iPhone testing |
|
Implemented in fb430c6. Typecheck, lint, the i18n audit (all blocking counters 0), I still do not have access to an iPhone, Mac, or Xcode, so I would appreciate help validating the final iOS behavior on a simulator or a physical device. |
|
I tested through Simulator & real iPhone and it worked: Settings-->SparkyFitenss-->Language 
SParkyFitness now renders Polish language. |
|
Let me know if I can merge this PR |
|
Claude flagged this. I know translation is not complete, but have a look. notifications.title = "Powiadomienia" exists in the resource file, but AppSettingsScreen.tsx:167 passes title="Notifications" as a literal instead of using it. That's a dead key — the translation is shipped but never rendered, which matches the English "Notifications" in your screenshot. Either the row should call t('notifications.title', 'Notifications') or the key should be dropped from both locale files. |
|
@CodeWithCJ I also saw that you tested the iOS build — thank you for covering that platform. Regarding the orphaned translation, I don’t think it needs to block this infrastructure PR. After this PR and the native widget localization PR, I plan to open a separate PR translating all remaining user-facing strings in the mobile app. I’ll clean up isolated or orphaned translation entries as part of that complete catalog pass. Android screenshots: 
|
3 participants
Description
What problem does this PR solve?
The mobile app has no shared i18n infrastructure and no way to follow the system language or pick a manual one. This PR introduces the foundation: i18next/react-i18next, a splash-safe language bootstrap, native per-app language support (Android 13+ reads/writes the platform per-app language with two-way sync; iOS reads the OS-owned per-app Language and opens Settings for changes; Android ≤12 keeps the local fallback), and an audit that enforces an explicit English-fallback contract.
How did you implement the solution?
en/plresources (a deliberately minimal, representative key set — not a full translation).src/localization/exposes the app-language model (system/en/pl), device-locale resolution (pl-*→pl,en-*→en, unsupported → English), persistence throughappPreferencesStore, and a one-time Android 13+ legacy-preference handoff marker (@SparkyFitness/app-language-migration).useAppBootstrap): the effective locale is resolved before the first screen renders — no English flash followed by a jump to Polish.android.app.LocaleManager/applicationLocalesreads/writes the system per-app language (systemclears the override,en/plapply the locale). On Android 12 and below no native locale API is called: the persisted in-app preference is authoritative, manualen/plwork through i18next andsystemfollows the device locale viaexpo-localization. No AppCompat dependency,AppLocalesMetadataHolderService, orautoStoreLocalesis used on any API level.expo-localizationbefore the first screen renders and re-reads it during bootstrap/foreground reconciliation; it never maintains a competing explicit override.supportedLocales.ios: ['en','pl']and thelocalesmap generate the localized native metadata and permission strings (camera / HealthKit read / HealthKit write / local network);UIPrefersShowingLanguageSettings: truekeeps the app-specific Language entry visible in iOS Settings even without multiple preferred system languages, andCFBundleAllowMixedLocalizationsis enabled for the localized InfoPlist strings. The in-app row shows the effective language (endonym) and opens iOS Settings viaLinking.openSettings(); a stale persisted explicit preference is normalized tosystemso it can never override the native value, and a failedopenSettingsleaves all language state unchanged. No privateAppleLanguages/UserDefaults/App-Prefs:hacks are used — there is no public API to write the iOS per-app language, so the app reads it and opens Settings for changes.t()passes a fallback string ordefaultValue.pnpm run i18n:auditenforces this plus missing keys, placeholder/plural mismatches, duplicate keys, dynamict(), and unsafe template-literal keys.Linked Issue: Related to #1774 · Closes #1490
How to Test
cd SparkyFitnessMobile && pnpm install && pnpm run i18n:audit→ expectuser-facing t() without English fallback: 0,dynamic t() keys: 0, all structural counts0, exit 0.npx jest --runInBand __tests__/localization __tests__/hooks/useAppLanguageForegroundSync.test.tsx __tests__/screens/AppSettingsScreen.test.tsx __tests__/config→ 8 suites / 81 tests pass (focused native-language regression set); bootstrap/startup/store set (useAppBootstrap,useAppStartup,appPreferencesStore) → 3 suites / 23 tests pass. Full mobile Jest suite on the final HEADfb430c63: 291 suites / 4861 tests, 0 failed;pnpm run typecheck,pnpm run lint,pnpm run i18n:auditandgit diff --checkclean,expo config --type publicgeneration verified, all i18n:audit blockers 0Zapisz— the same localized text as the visible button.PR Type
Checklist
All PRs:
New features only:
Mobile changes (
SparkyFitnessMobile/):android.ymlworkflow on this branch (fb430c63); device validation remains pending on the final artifact.)Screenshots
Click to expand
The UI diff is limited to the language row in App Settings (BottomSheetPicker with System / English / Polski), the App Settings shell header, and the header Save fallback labels — all of which resolve through the audited fallback contract and are covered by the targeted Jest suites.
Notes for Reviewers
medications.types.*, widgets, Live Activity localization and Weblate/translation-repo sync are explicitly not included here (PR5 / later).i18n:auditreports them as informational only (currently 2358 against this branch) and never blocks — the full hardcoded-UI inventory and its mass migration belong to PR5, where a fresh baseline can be captured if still needed.Zapiszin Polish); an explicit calleraccessibilityLabelstill wins.expo prebuild --clean --platform androidwas run twice and is idempotent (the Kotlin bridge sources andAppLanguagePackageregistration are generated exactly once; no AppCompat dependency,AppLocalesMetadataHolderService, orautoStoreLocalesis generated). A release APK + AAB build is generated by the fork'sandroid.yml(Build Android APK) workflow on this branch (fb430c63): run 31937551546 / 31937533353. Upstream CI on the final HEAD (fb430c63): Mobile Tests, Server Tests, Detect Changes, Validate & Label, GitGuardian and CodeRabbit all pass; the only remaining gate is the pr-validation bot's mandatory device/emulator checkbox, which stays[ ]until a real device pass on this APK.supportedLocales.iosis intentional and remains. Runtime/Xcode validation is still pending (no Mac available here); config generation, locale declarations and the TS lifecycle are covered by tests.mobile/en/translation.json,mobile/pl/translation.jsonin the translations repo).Summary by CodeRabbit
New Features
Bug Fixes