FEATURE: Localization fallbacks (server-side)

[ShashankFC]

Test 9nn---nReplicated from ai-code-review-evaluation/discourse-greptile#9

[ShashankFC]

@claude

[claude]

Claude finished @ShashankFC's task in 1m 37s —— View job

---

PR Review

Tasks

• Read repository guidelines (CLAUDE.md)
• Review changed files
• Analyze code quality and correctness
• Provide feedback

---

Summary

This PR implements server-side localization fallbacks for Discourse. The implementation consolidates i18n configuration into a dedicated initializer and adds a custom fallback chain: user locale → site default locale → English.

Key Changes

New: config/initializers/i18n.rb (24 lines added)

• Includes pluralization and fallbacks modules
• Introduces FallbackLocaleList class for custom fallback logic
• Implements ensure_loaded! method to pre-load all fallback locales

Modified: app/controllers/application_controller.rb (+2 lines)

• Calls I18n.fallbacks.ensure_loaded! after setting locale in set_locale method

Modified: lib/freedom_patches/translate_accelerator.rb (+5 lines)

• Adds ensure_loaded! method to support the fallback system

Removed: config/initializers/pluralization.rb (-2 lines)

• Consolidated into new i18n.rb initializer

Modified: Production configs (-13 lines total)

• Removed ineffective config.i18n.fallbacks = true declarations from production.rb, profile.rb, and cloud66/production.rb

---

Code Review Findings

✅ Strengths

1. Well-explained rationale: The commit message clearly explains why the previous config.i18n.fallbacks = true was ineffective
2. Proper consolidation: Moving pluralization setup into the i18n initializer improves organization
3. Lazy loading integration: The ensure_loaded! method integrates well with the existing translate_accelerator patch
4. Sensible fallback chain: user locale → site default → English is a logical progression

⚠️ Issues & Concerns

1. Potential Performance Impact (Medium severity)

• Location: app/controllers/application_controller.rb:159
• Issue: I18n.fallbacks.ensure_loaded! is called on every request in the set_locale before_filter
• Impact: This iterates through all fallback locales and calls I18n.ensure_loaded! for each, potentially loading multiple locale files on every request
• Details: For a user with locale fr, this would load fr, then check SiteSetting.default_locale, then check en - on every single HTTP request
• Suggestion: Consider caching or memoizing loaded locale combinations, or move this to only trigger when the locale actually changes

2. Thread Safety Concerns (Low-Medium severity)

• Location: config/initializers/i18n.rb:20-22
• Issue: The ensure_loaded! method calls I18n.ensure_loaded! without synchronization, but translate_accelerator.rb uses a LOAD_MUTEX for thread safety
• Risk: Potential race conditions during concurrent locale loading, though the underlying ensure_loaded! in translate_accelerator does use the mutex
• Note: May be safe due to the mutex in the underlying implementation, but worth verifying

3. Code Duplication (Low severity)

• Location: config/initializers/i18n.rb:20-22 and lib/freedom_patches/translate_accelerator.rb:62-65
• Issue: Two ensure_loaded! implementations exist - one in FallbackLocaleList and one in the I18n module
• Concern: The FallbackLocaleList#ensure_loaded! calls I18n.ensure_loaded! which is defined in the translate_accelerator patch. The loading order dependency is explicitly noted in the comment # order: after 02-freedom_patches.rb
• Impact: Fragile dependency on initialization order

4. Missing Error Handling (Low severity)

• Location: config/initializers/i18n.rb:17
• Issue: If SiteSetting.default_locale is nil or invalid, .to_sym will still proceed
• Impact: Could result in attempting to load invalid locales, though .compact would filter out nils

5. Array Creation on Every Lookup (Low severity)

• Location: config/initializers/i18n.rb:17
• Issue: A new array is created every time FallbackLocaleList#[] is called
• Impact: Minor performance overhead and GC pressure
• Suggestion: Consider memoizing the result per locale combination

6. Inconsistent Comments (Cosmetic)

• Location: config/initializers/i18n.rb:1
• Issue: Comment says # order: after 02-freedom_patches.rb but Rails initializer loading order is alphabetical by default. The dependency on translate_accelerator being loaded first is not guaranteed.
• Concern: If initializers load in unexpected order, I18n.ensure_loaded! won't exist when FallbackLocaleList tries to call it

---

Questions

1. Has this been performance tested with multiple locales? The per-request overhead of ensure_loaded! could add up
2. Are there integration tests covering the fallback chain (user locale → site default → English)?
3. What happens if a user's locale is the same as SiteSetting.default_locale? The .uniq call handles this, but worth confirming the behavior is tested

---

Recommendations

Priority fixes:

1. Optimize the ensure_loaded! call - Consider moving it to only execute when locale changes, not every request
2. Verify initializer load order - Ensure translate_accelerator loads before i18n.rb or handle the dependency more explicitly

Nice-to-haves:
3. Add memoization to FallbackLocaleList#[] to avoid array creation on every call
4. Add integration tests for the complete fallback chain
5. Consider adding metrics/logging to track fallback usage patterns

---

Overall Assessment

The implementation is generally solid with a clear purpose and good architectural decisions. The main concern is the performance impact of calling ensure_loaded! on every request. This should be addressed before merging, or at minimum, performance tested under realistic load.

The code is readable and well-commented, and the consolidation of i18n configuration is a good improvement. With the performance optimization, this would be ready to merge.

---