FEATURE: Localization fallbacks (server-side)
discourse/discourse@ecfa17b5
Golden recall: 0%
Extra findings: 9
Golden Comments (0/2 found)
| # | Comment | Severity | Status | FriendlyReviewer Detail |
|---|---|---|---|---|
| #1 | Thread-safety issue with lazy @loaded_locales |
LOW | ✗ Not found | The review covers the same files from different angles — the accelerator only loading config.locale for non-controller contexts, the blank :"" locale at boot, the LRU cache key omitting the fallback source, and enforce_available_locales never set — but no candidate targets the lazy @loaded_locales memoization itself, which is read and mutated without synchronization on first load. See lib/freedom_patches/translate_accelerator.rb |
| #2 | Consider normalizing the input locale (e.g., to a symbol) when checking/loading here to avoid double-loading if the same locale is passed as a String vs Symbol (also applies to other locations in the PR). | LOW | ✗ Not found | No candidate flags the String-vs-Symbol double load: every lookup path in the review assumes a normalized locale, and none mentions that the same locale passed as a String and as a Symbol loads twice. See lib/freedom_patches/translate_accelerator.rb |
Supplementary Findings (9 findings)
| Finding | File | Severity | Legitimate? |
|---|---|---|---|
translate still only loads config.locale — the fallback chain is pre-loaded solely from application_controller#set_locale, so I18n.t outside controller requests (Sidekiq, rake, I18n.with_locale) hits I18n::MissingTranslation |
translate_accelerator.rb:68 | HIGH | ✓ Real: fallbacks silently broken for every non-controller path — the most substantive finding of this review |
[locale, SiteSetting.default_locale.to_sym, :en].uniq.compact can yield :"" when default_locale is blank during early boot — load_locale then builds a regex matching every yml file, defeating on-demand loading |
config/initializers/i18n.rb:17 | MEDIUM | ✓ Real boot-order hazard; edge-case trigger |
The LRU cache key omits the resolved fallback source — cached translations stay stale after a runtime default_locale change without a reload! |
translate_accelerator.rb:72 | MEDIUM | ✓ Real staleness window; needs a locale change at runtime to manifest |
I18n.enforce_available_locales is never set — deprecation warnings or I18n::InvalidLocale depending on the i18n gem version, now that set_locale accepts an unvalidated stored string |
config/initializers/i18n.rb:24 | MEDIUM | ✓ Borderline: version-dependent behavior; worth pinning explicitly either way |
Removing config.i18n.fallbacks = true from the environment files silently enables fallbacks in development and test too — a behavioral change for the test suite |
config/environments/production.rb:27 | MEDIUM | ✓ Real: specs now fall back to site default / :en for missing keys, unconfirmed as desired |
The fallback chain always terminates in :en even when the site default locale is not English — every locale is forced back to English if incomplete |
config/initializers/i18n.rb:17 | LOW | ✓ Borderline: acceptable as designed per the comment; worth confirming :en is always fully loaded |
Pluralization is included into I18n::Backend::Simple while Fallbacks goes into I18n.backend.class — the two coincide today but diverge if the backend is replaced |
config/initializers/i18n.rb:9 | LOW | ✓ Borderline: latent coupling, no current failure |
| Environment files now contribute nothing to fallback configuration — the effective chain is entirely the initializer's, removing any per-environment override hook | config/environments/profile.rb:17 | LOW | ✓ Borderline: delegation is simpler but drops flexibility (e.g. locale-aware chains) |
| The Cloud66 override removes the same fallback line, keeping it consistent with production — the initializer covers it on boot | config/cloud/cloud66/files/production.rb:22 | LOW | ✓ Borderline: consistent no-op; the review itself marks it "no change required" |
Honest Analysis
FriendlyReviewer's full review
As posted on the PR — the complete output from FriendlyReviewer.
Hi there 👋
| 🌥️ Tech | 🌤️ Feat |
|---|---|
| 1 high, 4 medium, 4 low | 1 mitigated |
The MR introduces server-side localization fallbacks via a new config/initializers/i18n.rb (fallback chain [locale, default_locale, :en]) and updates application_controller#set_locale to call I18n.fallbacks.ensure_loaded!. Environment files consistently delegate fallback configuration to the initializer, and the Cloud66 deployment remains consistent with production. However, the feature is not fully robust: the translate accelerator still loads only the current locale, so fallbacks silently break outside controller requests (e.g. Sidekiq, rake, I18n.with_locale); the LRU cache key omits the fallback chain and can serve stale values when default_locale changes; a blank SiteSetting.default_locale during early boot can produce an empty-symbol locale that loads every yml file; I18n.enforce_available_locales is not addressed; and fallback behavior is now silently enabled in development/test environments. These issues should be addressed before merge.
These might need a close look
- 🔴 lib/freedom_patches/translate_accelerator.rb (L68)
translatestill only loadsconfig.localeviaload_locale(config.locale). The fallback locales defined in the i18n initializer are only pre-loaded byI18n.fallbacks.ensure_loaded!, which is called solely fromapplication_controller#set_locale. AnyI18n.texecuted outside a controller request (e.g.I18n.with_localein post_alerter/post_destroyer, Sidekiq jobs, rake tasks) will not have the fallback chain loaded, causingI18n::MissingTranslationinstead of the intended fallback. Recommend callingensure_loaded!(config.locale)intranslateto pre-load the full chain.
Worth checking
- 🟡 config/initializers/i18n.rb (L17)
[locale, SiteSetting.default_locale.to_sym, :en].uniq.compactcan yield:""whenSiteSetting.default_localeis blank/nil during early boot (before site settings are initialized).FallbackLocaleList#ensure_loaded!then callsI18n.ensure_loaded!(:""), andload_localebuilds a regex matching every.ymlfile, defeating on-demand loading. Guard the value (e.g.(SiteSetting.default_locale.presence || :en).to_sym) or skip blank locales inensure_loaded!. - 🟡 lib/freedom_patches/translate_accelerator.rb (L72)
The LRU cache key"#{key}#{config.locale}#{config.backend.object_id}"does not include the resolved fallback source. IfSiteSetting.default_localechanges at runtime (e.g.en→fr) without areload!, cached translations resolved via the old fallback chain remain keyed to(key, locale)and return stale values. Consider keying on the fallback chain (e.g.config.fallbacks[config.locale].join('-')) or invalidating the cache when default_locale changes. - 🟡 config/initializers/i18n.rb (L24)
The MR configuresI18n.fallbacksand overridesavailable_localesin translate_accelerator, but never setsI18n.enforce_available_locales. Depending on the i18n gem version this emits deprecation warnings or raisesI18n::InvalidLocale, especially now thatset_localecan setI18n.localefrom a stored user string not validated againstsupported_locales. Recommend explicitly settingI18n.enforce_available_locales = false(or validating the effective locale). - 🟡 config/environments/production.rb (L27)
Removingconfig.i18n.fallbacks = truefrom production/profile/cloud66 moves fallback enabling entirely to the environment-independent initializer, silently enabling fallbacks in development and test environments as well. This behavioral change for the test environment (specs will now fall back to site default locale /:enfor missing keys) should be confirmed as desired, or explicitly reverted/documented for tests.
Small things (take or leave)
- 🔵 config/initializers/i18n.rb (L17)
The fallback chain always terminates in:en, even when the site default locale is itself not English. This forces every locale to fall back to English if incomplete relative to the site default, and the available-locale set does not gate the terminal fallback. Acceptable as designed per the comment, but worth confirming:enis always a guaranteed, fully-loaded locale. - 🔵 config/initializers/i18n.rb (L9)
Pluralizationis included intoI18n::Backend::SimplewhileFallbacksis included intoI18n.backend.class. These coincide today but diverge if the backend is ever replaced with a chain or another class. Recommend including both into the same target (e.g.I18n.backend.class.send(:include, ...)for both) to keep pluralization and fallbacks on the live backend. - 🔵 config/environments/profile.rb (L17)
The environment files now contribute nothing to the fallback configuration; the effective fallback chain is entirely the initializer's[locale, SiteSetting.default_locale.to_sym, :en]. This depends on DB-backed site settings at runtime and removes any per-environment override hook (e.g. for a locale-aware chain likezh_TW → zh_CN). Confirm this delegation is intended and thatSiteSetting.default_localeis reliably initialized at the point of use. - 🔵 config/cloud/cloud66/files/production.rb (L22)
The Cloud66 deployment override removes the sameconfig.i18n.fallbacks = trueline, keeping it consistent withenvironments/production.rb. Since the initializer runs on boot regardless of deployment, the Cloud66 production deployment picks up the intended fallback configuration without explicit setting. No change required.
✅ Feature-level checklist
Partially covered
- ⚠️ FEATURE: Localization fallbacks (server-side)
The server-side fallback chain is implemented and works for controller requests, but is not fully robust: fallbacks silently break outside controller requests because the translate accelerator does not load fallback locales; the LRU cache can serve stale values when default_locale changes; and a blank default_locale during early boot can cause all locales to load. These caveats mean the feature is only partially satisfied as intended.
This is a 0/2 result, and the miss is not trivial. Golden #1 targets the lazy
@loaded_localesmemoization intranslate_accelerator.rb: the instance variable is mutated on first access with no synchronization, a genuine thread-safety hazard in a multi-threaded Rails server. Golden #2 asks for locale normalization (String vs Symbol) to avoid double-loading the same locale. Neither appears anywhere in the review's candidates — the review spent its attention on adjacent questions and never touched these two.What the review did produce is mostly real: one high — the
translateaccelerator still loads onlyconfig.locale, so the fallback chain silently breaks outside controller requests (Sidekiq, rake,I18n.with_locale) — plus a blank-locale:""hazard at boot, a stale LRU cache whendefault_localechanges, and fallbacks silently enabled in dev/test. The high is arguably more severe than both goldens combined: it breaks the feature's whole point in non-request contexts. But it does not excuse the miss: both goldens are precisely the class of latent bug that broader angles don't catch, because there is no local evidence of concurrency in the diff — no visible thread, mutex, or shared-state write next to the lazy memoization.The "partially covered" verdict is fair on the letter of the feature — fallbacks do work for controller requests — but generous on the spirit: both goldens will surface in production (rare races, doubled loads) rather than in tests. This is a weak recall case with salvageable findings: the supplementary set is legitimate and the high is a genuine blocker, but none of the nine candidates addresses the two exact weaknesses the dataset isolated.