FEATURE: Localization fallbacks (server-side)#9
Conversation
The FallbackLocaleList object tells I18n::Backend::Fallbacks what order the languages should be attempted in. Because of the translate_accelerator patch, the SiteSetting.default_locale is *not* guaranteed to be fully loaded after the server starts, so a call to ensure_loaded! is added after the locale is set for the current user. The declarations of config.i18n.fallbacks = true in the environment files were actually garbage, because the I18n.default_locale was SiteSetting.default_locale, so there was nothing to fall back to. *derp*
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThe changes refactor I18n locale fallback handling by introducing a custom FallbackLocaleList class to compute fallback chains dynamically, adding on-demand locale loading via ensure_loaded! methods, and removing explicit fallback and pluralization configuration from environment files. Changes
Sequence DiagramsequenceDiagram
participant Controller as Request Handler
participant Fallback as FallbackLocaleList
participant I18n as I18n Module
participant Loader as Locale Loader
Controller->>Fallback: ensure_loaded!()
Fallback->>Fallback: [locale, default_locale, :en]
Fallback->>I18n: ensure_loaded!(computed_locales)
I18n->>I18n: `@loaded_locales` ||= Set.new
alt Locale not in `@loaded_locales`
I18n->>Loader: load_locale(locale)
Loader-->>I18n: Locale loaded
I18n->>I18n: Add to `@loaded_locales`
end
I18n-->>Fallback: Complete
Fallback-->>Controller: Ready for translation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
config/initializers/i18n.rb (3)
3-5: Prefer directincludeoversend(:include, ...).Per static analysis (RuboCop), Ruby 2.1+ allows calling
includedirectly on modules.♻️ Suggested refactor
# Include pluralization module require 'i18n/backend/pluralization' -I18n::Backend::Simple.send(:include, I18n::Backend::Pluralization) +I18n::Backend::Simple.include(I18n::Backend::Pluralization)
7-9: Same refactor applies here.♻️ Suggested refactor
# Include fallbacks module require 'i18n/backend/fallbacks' -I18n.backend.class.send(:include, I18n::Backend::Fallbacks) +I18n.backend.class.include(I18n::Backend::Fallbacks)
12-18: Consider defensive nil handling forSiteSetting.default_locale.If
SiteSetting.default_localereturnsnilduring early initialization or edge cases, calling.to_symwill raiseNoMethodError. Thecompacton line 17 removes nils from the array but executes afterto_sym.🛡️ Suggested defensive change
class FallbackLocaleList < Hash def [](locale) # user locale, site locale, english # TODO - this can be extended to be per-language for a better user experience # (e.g. fallback zh_TW to zh_CN / vice versa) - [locale, SiteSetting.default_locale.to_sym, :en].uniq.compact + [locale, SiteSetting.default_locale&.to_sym, :en].uniq.compact end
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/controllers/application_controller.rbconfig/cloud/cloud66/files/production.rbconfig/environments/production.rbconfig/environments/profile.rbconfig/initializers/i18n.rbconfig/initializers/pluralization.rblib/freedom_patches/translate_accelerator.rb
💤 Files with no reviewable changes (4)
- config/environments/profile.rb
- config/cloud/cloud66/files/production.rb
- config/environments/production.rb
- config/initializers/pluralization.rb
🧰 Additional context used
🧬 Code graph analysis (2)
app/controllers/application_controller.rb (2)
config/initializers/i18n.rb (1)
ensure_loaded!(20-22)lib/freedom_patches/translate_accelerator.rb (1)
ensure_loaded!(62-65)
lib/freedom_patches/translate_accelerator.rb (1)
config/initializers/i18n.rb (3)
ensure_loaded!(20-22)locale(12-23)locale(13-18)
🪛 Brakeman (7.1.1)
app/controllers/application_controller.rb
[medium] protect_from_forgery should be configured with with: :exception
Type: Cross-Site Request Forgery
Confidence: Medium
More info: https://brakemanscanner.org/docs/warning_types/cross-site_request_forgery/
(Cross-Site Request Forgery)
🪛 RuboCop (1.82.1)
config/initializers/i18n.rb
[warning] 5-5: Use include I18n::Backend::Pluralization instead of send(:include, I18n::Backend::Pluralization).
(Lint/SendWithMixinArgument)
🔇 Additional comments (3)
lib/freedom_patches/translate_accelerator.rb (1)
62-65: LGTM!The
ensure_loaded!method follows the same pattern as the existingtranslatemethod (line 68), withload_localeproviding thread-safe double-checking internally viaLOAD_MUTEX. This provides a clean public API for on-demand locale loading.config/initializers/i18n.rb (1)
20-24: LGTM!The
ensure_loaded!method correctly iterates over the computed fallback chain and loads each locale via the patchedI18n.ensure_loaded!. The assignment at line 24 properly wires up the custom fallback behavior.app/controllers/application_controller.rb (1)
152-160: LGTM!The
ensure_loaded!call is correctly placed after determining the locale, ensuring all fallback locales (user locale → site default → English) are loaded before any translations occur in the request lifecycle. This aligns well with the on-demand loading strategy introduced in this PR.
2 participants
Test 9
Summary by CodeRabbit
Release Notes
Refactor
Chore
✏️ Tip: You can customize this high-level summary in your review settings.
Replicated from ai-code-review-evaluation/discourse-coderabbit#9