FEATURE: Localization fallbacks (server-side)

[akshayutture-augment]

Test 9

---

Replicated from ai-code-review-evaluation/discourse-greptile#9

[akshayutture-augment]

@claude review this PR

[akshayutture-augment]

@claude review this PR

[claude]

Claude finished @akshayutture-augment's task —— View job

---

Code Review Complete ✓

Tasks:

• Check for repository guidelines (CLAUDE.md)
• Read all changed files
• Analyze the diff to understand changes
• Review implementation for correctness and best practices
• Provide detailed feedback

---

Summary

This PR implements server-side localization fallbacks for the Discourse application. The implementation is solid and well-designed with good architectural decisions. The changes properly configure I18n to fall back through a chain of locales (user locale → site default locale → English) when translations are missing.

✅ Strengths

1. Clean Architecture: The FallbackLocaleList class is elegantly designed, inheriting from Hash to integrate seamlessly with I18n's fallback system.

2. Proper Initialization Order: The comment # order: after 02-freedom_patches.rb ensures the initializer runs after the translate accelerator patch is loaded.

3. Good Code Consolidation: Merging the pluralization setup from the separate file into the main i18n initializer improves maintainability.

4. Correct Cleanup: Removing the useless config.i18n.fallbacks = true declarations that weren't actually doing anything (since I18n.default_locale was set to SiteSetting.default_locale).

5. Performance Awareness: The ensure_loaded! call ensures all fallback locales are preloaded, which is critical given the lazy-loading nature of the translate_accelerator patch.

🔍 Issues & Concerns

1. Potential Thread Safety Issue (Medium Severity)

Location: config/initializers/i18n.rb:20-22

def ensure_loaded!
  self[I18n.locale].each { |l| I18n.ensure_loaded! l }
end

The ensure_loaded! method accesses I18n.locale which can vary per-thread in a multi-threaded environment. If this is called from the initializer context (not per-request), it might use the wrong locale.

Recommendation: Consider passing the locale as a parameter:

def ensure_loaded!(locale = I18n.locale)
  self[locale].each { |l| I18n.ensure_loaded! l }
end

2. Redundant compact Call (Low Severity)

Location: config/initializers/i18n.rb:17

[locale, SiteSetting.default_locale.to_sym, :en].uniq.compact

Since you're constructing the array yourself and all values are symbols or will be converted to symbols, the compact is unnecessary (there are no nil values unless SiteSetting.default_locale returns nil, which would be a configuration error).

Recommendation: Either remove compact or add a comment explaining why it's needed for defensive programming.

3. Missing Error Handling (Low-Medium Severity)

Location: config/initializers/i18n.rb:17

If SiteSetting.default_locale raises an exception or returns an unexpected value during initialization, the application could fail to start. Consider adding error handling or validation.

4. TODO Should Be Addressed (Low Severity)

Location: config/initializers/i18n.rb:15-16

# TODO - this can be extended to be per-language for a better user experience
# (e.g. fallback zh_TW to zh_CN / vice versa)

This is a good enhancement idea. Consider creating a follow-up issue to track this feature request for language-family-aware fallbacks.

🎯 Code Quality

Positive:

• Clean, readable code
• Good use of Ruby idioms (inheriting from Hash)
• Appropriate comments explaining the "why"
• Consistent code style

Minor Style Notes:

• The FallbackLocaleList class could benefit from a brief class-level comment explaining its purpose
• Consider adding a module namespace to avoid polluting the global namespace

🔒 Security

No security concerns identified. The changes don't introduce user input handling or external data processing.

⚡ Performance

The implementation is performance-conscious:

• Uses uniq to avoid duplicate locale loading
• Preloads all fallback locales upfront with ensure_loaded!
• Works well with the existing LRU cache in translate_accelerator

📝 Testing Recommendations

Consider adding tests for:

1. Fallback chain order (user locale → site locale → en)
2. Behavior when user locale equals site locale (should not duplicate)
3. The ensure_loaded! method ensures all locales in the chain are loaded
4. Edge cases (nil locale, invalid locale, etc.)

Verdict

APPROVE with minor suggestions. This is a well-thought-out implementation that addresses a real architectural issue (the previous fallback configuration wasn't actually working). The main concern is the potential thread-safety issue with ensure_loaded!, but this is likely fine given how it's being called in practice.

The removal of the non-functional config.i18n.fallbacks = true lines and the consolidation into a proper custom fallback system is exactly the right approach.

---