✨ Theme i18n converted to use i18next (with feature flag) #23161
Conversation
WalkthroughThis change introduces a feature-flag-driven system for theme translation using i18next in place of the existing translation package. A new beta feature flag, Suggested labels
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (11)
🔇 Additional comments (13)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
ghost/core/core/frontend/services/theme-engine/i18next/ThemeI18n.js (1)
64-84: Consider using async/await consistently in error handling.While the implementation works, the nested try/catch blocks could be simplified using async/await with a more linear approach.
Consider refactoring the error handling to make it more linear:
- try { - // Verify the locale file exists - const localePath = path.join(themeLocalesPath, `${this._locale}.json`); - await fs.access(localePath); - - // Initialize i18n - this._i18n = i18nLib(this._locale, 'theme', {themePath: themeLocalesPath}); - } catch (err) { - // If the requested locale fails, try English as fallback - try { - const enPath = path.join(themeLocalesPath, 'en.json'); - await fs.access(enPath); - this._i18n = i18nLib('en', 'theme', {themePath: themeLocalesPath}); - } catch (enErr) { - // If both fail, use the key as the translation - this._i18n = { - t: key => key - }; - } - } + // Try the requested locale first + const localePath = path.join(themeLocalesPath, `${this._locale}.json`); + const localeExists = await fs.pathExists(localePath); + + if (localeExists) { + this._i18n = i18nLib(this._locale, 'theme', {themePath: themeLocalesPath}); + return; + } + + // If the requested locale doesn't exist, try English as fallback + const enPath = path.join(themeLocalesPath, 'en.json'); + const enExists = await fs.pathExists(enPath); + + if (enExists) { + this._i18n = i18nLib('en', 'theme', {themePath: themeLocalesPath}); + return; + } + + // If both fail, use the key as the translation + this._i18n = { + t: key => key + };ghost/i18n/lib/i18n.js (2)
96-107: Interpolation delimiter logic looks good, but maintain newsletter parity
The switch to single–brace interpolation for boththemeandnewsletternamespaces is correct. However, only thethemebranch later disables HTML escaping (escapeValue = false). If newsletter strings are also expected to contain raw HTML (previous implementation did so), you may want to disable escaping there as well––otherwise delimiter behaviour between the two namespaces is no longer symmetrical.
120-151: Synchronous disk I/O on every i18n initialisation can block the event loop
fs.readFileSyncis executed each timei18n('…','theme')is called. In production this may run for every request that triggers a new Handlebars render, causing synchronous blocking.Consider:
- Reading files asynchronously during theme activation and passing cached resources here.
- Memoising
resourcesperthemePath+lngpair so subsequent calls hit memory, not disk.Both approaches remove disk I/O from the hot path.
ghost/i18n/test/i18n.test.js (3)
43-52: Typo in test title
it('is uses default export if available' …)reads awkwardly. Changing to “uses default export if available” improves clarity when the test output is displayed.-it('is uses default export if available', async function () { +it('uses default export if available', async function () {
163-299: Great coverage, but consider deduplicating helper code
The theme-related tests repeat setup logic for creating temporary locale files, writing JSON, and instantiatingi18n. Extracting a small helper (e.g.,createTempThemeLocales(files: Record<lng, object>)) would shorten each test and make intent clearer.This is purely a readability/maintainability improvement—the tests are functionally solid.
250-277: Redundant interpolation tests
it('handles interpolation correctly')(250-258) andit('interpolates variables in theme translations')(260-277) overlap. Unless you intend to test distinct code paths, one consolidated test is enough and will speed up the suite slightly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
ghost/core/test/e2e-api/admin/__snapshots__/config.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (11)
apps/admin-x-settings/src/components/settings/advanced/labs/BetaFeatures.tsx(1 hunks)ghost/core/core/frontend/helpers/t.js(2 hunks)ghost/core/core/frontend/services/handlebars.js(1 hunks)ghost/core/core/frontend/services/theme-engine/active.js(2 hunks)ghost/core/core/frontend/services/theme-engine/i18next/ThemeI18n.js(1 hunks)ghost/core/core/frontend/services/theme-engine/i18next/index.js(1 hunks)ghost/core/core/shared/labs.js(1 hunks)ghost/core/test/unit/frontend/helpers/t-new.test.js(1 hunks)ghost/core/test/unit/frontend/services/theme-engine/i18next.test.js(1 hunks)ghost/i18n/lib/i18n.js(4 hunks)ghost/i18n/test/i18n.test.js(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
ghost/core/core/frontend/services/handlebars.js (1)
ghost/core/core/frontend/helpers/t.js (2)
require(13-13)require(14-14)
ghost/core/core/frontend/services/theme-engine/i18next/index.js (1)
ghost/core/core/frontend/helpers/t.js (3)
config(16-16)require(13-13)require(14-14)
ghost/core/test/unit/frontend/helpers/t-new.test.js (2)
ghost/core/core/frontend/helpers/t.js (3)
require(13-13)require(14-14)labs(15-15)ghost/core/core/frontend/services/theme-engine/i18next/ThemeI18n.js (1)
path(3-3)
🔇 Additional comments (26)
ghost/core/core/shared/labs.js (1)
37-38: Good addition of the feature flag for theme translationThe feature flag
themeTranslationis properly added to the PUBLIC_BETA_FEATURES array, making it available for users to enable or disable in the admin UI.ghost/core/core/frontend/services/handlebars.js (1)
21-22: Good export of the new theme translation serviceThe new
themeI18nextservice is appropriately exported alongside the existingthemeI18nservice, which aligns with the feature flag approach being implemented for theme translation.apps/admin-x-settings/src/components/settings/advanced/labs/BetaFeatures.tsx (1)
39-42: Good addition of the theme translation toggle in labs settingsThe UI toggle for the "Updated theme Translation (beta)" feature is well implemented, following the same pattern as other feature toggles. The description appropriately explains the purpose of the feature flag.
ghost/core/core/frontend/services/theme-engine/i18next/index.js (1)
1-6: Good implementation of the singleton pattern for ThemeI18nThe module correctly exports a singleton instance of ThemeI18n initialized with the appropriate base path from configuration. The ThemeI18n class is also re-exported, which provides flexibility for testing or advanced usage scenarios.
ghost/core/core/frontend/services/theme-engine/active.js (2)
21-22: Good addition of required importsThe new imports for
themeI18nextandlabsare correctly added to support the feature flag implementation.
106-113: Good implementation of feature flag conditional logicThe conditional initialization of either the new
themeI18nextservice or the legacythemeI18nservice based on the feature flag status is well implemented. This allows for a smooth transition between implementations as users opt into the beta feature.ghost/core/core/frontend/helpers/t.js (2)
13-17: Dependencies are correctly managed.Good job on adding the necessary imports for the feature flag implementation. The code correctly imports
themeI18next,labs,config, andsettingsCacheto support the conditional translation logic.
33-57: Feature flag implementation looks good.The implementation correctly uses the
themeTranslationfeature flag to conditionally choose between translation systems. I like how both paths have similar initialization patterns, making the code easy to understand and maintain.A few observations:
- Each translation service is initialized only when needed (lines 37-42 and 49-54)
- Both services receive the same configuration parameters
- Clear comments explain the purpose of each branch
ghost/core/test/unit/frontend/services/theme-engine/i18next.test.js (7)
1-5: Test setup imports look good.The test file properly imports the necessary dependencies, including the
ThemeI18nclass that's being tested.
6-17: Test setup is well-structured.Good job on the test setup, including:
- Clear description of what's being tested
- Proper initialization of the
ThemeI18ninstance with a test base path- Cleaning up sinon stubs after each test
18-25: Default locale tests are appropriate.These tests correctly verify the default locale behavior and the ability to set a different locale.
27-32: Translation initialization test looks good.This test properly verifies that translations are loaded and can be retrieved after initialization.
33-43: Fallback behavior tests are comprehensive.These tests verify two important fallback scenarios:
- Falling back to English when a translation is missing in the current locale
- Using the key itself as a fallback when no translation files exist
This ensures robustness of the translation system even when translations are incomplete.
45-55: Edge case handling tests are important.Good job on testing edge cases:
- Handling empty keys
- Ensuring proper error messages when the service is used before initialization
These tests help prevent subtle bugs in production.
57-62: Key fallback test is appropriate.This test verifies that unknown translation keys are returned as-is, which is the expected fallback behavior.
ghost/core/test/unit/frontend/helpers/t-new.test.js (6)
1-7: Imports are correctly set up.The test file properly imports all necessary dependencies, including the helper function being tested and the feature flag module.
8-24: Test setup is comprehensive.Good test setup that:
- Stubs the feature flag to ensure the new translation system is used
- Sets up a test path for theme fixtures
- Properly cleans up after tests
- Resets the i18n instance before each test to ensure test isolation
26-44: Locale-specific tests are thorough.These tests properly verify translation behavior for different locales (DE and EN), ensuring the translation helper works correctly with locale-specific content.
46-64: Fallback behavior tests are important.Good job testing fallback behavior:
- Falling back to English when a French translation is missing
- Using the key itself as fallback when no theme files exist
This ensures the translation system is robust even with incomplete translations.
66-80: Edge case handling is comprehensive.These tests properly verify handling of edge cases:
- Empty translation keys
- Missing translation keys
Both cases correctly return an empty string as expected.
82-88: Options handling test is important.This test verifies that the helper works even when no options are passed, which is an important edge case to support.
ghost/core/core/frontend/services/theme-engine/i18next/ThemeI18n.js (4)
1-5: Dependencies are properly imported.The code imports all necessary dependencies for translation, file system operations, and error handling.
6-31: Class structure and initialization are well-designed.The
ThemeI18nclass is well-structured with:
- Clear JSDoc comments explaining the parameters
- Proper validation of required parameters
- Sensible defaults (e.g., 'en' for locale)
- Getters and setters for testing purposes
33-85: Initialization method is robust with good fallback logic.The
initmethod:
- Validates required parameters
- Sets up the locale and active theme
- Checks if the theme has a locales directory
- Attempts to load the requested locale file
- Falls back to English if the requested locale is missing
- Falls back to using the key as translation if no locale files exist
This multi-level fallback approach ensures translation works even with incomplete or missing translations.
87-101: Translation method is properly implemented.The
tmethod:
- Checks if initialization has been performed
- Throws a clear error message if used before initialization
- Delegates to the i18n instance for translation
- Ensures the result is always a string, even if the underlying library returns something else
ghost/i18n/lib/i18n.js (1)
110-113: Potential XSS surface whenescapeValueis disabled
Turning offescapeValuefor the theme namespace means any HTML contained in translation files is rendered verbatim in templates. This is intentional for rich-text themes, but also opens the door for XSS if a theme bundles un-escaped user-supplied strings. Consider documenting this clearly and, if feasible, validating translation JSON during theme upload.
| let resources; | ||
| if (ns !== 'theme') { | ||
| resources = generateResources(SUPPORTED_LOCALES, ns); | ||
| } else { | ||
| // For theme namespace, we need to load translations from the theme's locales directory | ||
| resources = {}; | ||
| const themeLocalesPath = options.themePath; | ||
|
|
||
| if (themeLocalesPath) { | ||
| // Try to load the requested locale first | ||
| try { | ||
| const localePath = path.join(themeLocalesPath, `${lng}.json`); | ||
| const content = fs.readFileSync(localePath, 'utf8'); | ||
| resources[lng] = { | ||
| theme: JSON.parse(content) | ||
| }; | ||
| } catch (err) { | ||
| // If the requested locale fails, try English as fallback | ||
| try { | ||
| const enPath = path.join(themeLocalesPath, 'en.json'); | ||
| const content = fs.readFileSync(enPath, 'utf8'); | ||
| resources[lng] = { | ||
| theme: JSON.parse(content) | ||
| }; | ||
| } catch (enErr) { | ||
| // If both fail, use an empty object | ||
| resources[lng] = { | ||
| theme: {} | ||
| }; | ||
| } | ||
| } | ||
| } else { | ||
| // If no theme path provided, use empty translations | ||
| resources[lng] = { | ||
| theme: {} | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Theme namespace does not load true fallback resources – breaks partial translations
Only the requested locale is loaded into resources, and when that file is missing you copy the English file into the requested locale key.
This means that if fr.json exists but is incomplete, i18next cannot fall back to en for the missing keys, because resources.en is never registered. Users will instead see the key string.
A minimal fix is to always load English resources separately and attach them under the en key while still loading the requested locale (if present). Example patch:
- resources = {};
+ const loadLocaleJson = (locale) => {
+ try {
+ return JSON.parse(fs.readFileSync(path.join(themeLocalesPath, `${locale}.json`), 'utf8'));
+ } catch {
+ return {};
+ }
+ };
+
+ resources = {
+ en: {theme: loadLocaleJson('en')}
+ };
+
+ // merge requested locale on top of English so i18next can still fall back
+ resources[lng] = {
+ theme: {
+ ...resources.en.theme,
+ ...loadLocaleJson(lng)
+ }
+ };With this change the normal fallbackLng chain works and missing keys gracefully degrade to English without duplicating data.
Committable suggestion skipped: line range outside the PR's diff.
cathysarisky
changed the title
cathysarisky
changed the title
|
@coderabbitai, you wrote: |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 11799 @cathysarisky You're absolutely right. After examining the code in detail, I can confirm that the initialization only happens during boot, theme changes, and language changes - not on every page load or helper call. Looking at the implementation in // Only initializes if not already initialized
if (!themeI18next._i18n) {
themeI18next.init({
activeTheme: settingsCache.get('active_theme'),
locale: config.get('locale')
});
}The theme's translations are loaded during the Thank you for the correction and for verifying it with logging. I'll make sure to keep this in mind for future reviews. ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ghost/i18n/test/i18n.test.js (1)
289-296: Fix string quote style issueThere's an inconsistency in the string quote style that should be fixed to maintain code consistency.
- assert.equal(t('Welcome, {name}', {name: "<b>John O'Nolan</b>"}), 'Welcome, <b>John O\'Nolan</b>'); + assert.equal(t('Welcome, {name}', {name: '<b>John O\'Nolan</b>'}), 'Welcome, <b>John O\'Nolan</b>');🧰 Tools
🪛 ESLint
[error] 295-295: Strings must use singlequote.
(quotes)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ghost/core/core/frontend/services/theme-engine/i18next/ThemeI18n.js(1 hunks)ghost/i18n/test/i18n.test.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/core/core/frontend/services/theme-engine/i18next/ThemeI18n.js
🧰 Additional context used
🪛 ESLint
ghost/i18n/test/i18n.test.js
[error] 295-295: Strings must use singlequote.
(quotes)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Setup
🔇 Additional comments (14)
ghost/i18n/test/i18n.test.js (14)
4-4: Well-structured addition of fsExtra dependencyThe inclusion of fs-extra provides a cleaner API for file system operations, which is appropriate for creating and managing test directories and files.
163-179: Good test setup with proper cleanupThe test setup for theme resources creates isolated testing environments with a temporary directory that gets properly cleaned up after each test. This prevents test pollution and ensures reliable test execution.
181-198: Comprehensive translation loading testThis test properly validates that translations load correctly from the theme locales directory, which is a core functionality of the new i18next implementation.
200-211: Robust fallback mechanism testingThe fallback mechanism test ensures that the system gracefully falls back to English when translations are missing in the requested language, which is crucial for user experience.
213-217: Good empty case handlingTesting the behavior when no translation files exist ensures the system remains stable in edge cases and provides sensible defaults.
219-226: Excellent error handling verificationTesting how the system responds to invalid JSON files is important for ensuring robustness against corrupted translation files. This test helps prevent runtime errors for users.
228-248: Thorough i18next configuration validationThis test comprehensively verifies all aspects of the i18next configuration, ensuring the instance is initialized with the correct settings for theme translations.
250-267: Excellent interpolation testingThe test thoroughly validates variable interpolation in theme translations, covering both simple cases and multiple variables.
269-287: Good namespace-specific interpolation validationTesting the different interpolation formats across namespaces ensures compatibility with existing translation systems while supporting the new theme translation approach.
299-315: Thorough portal namespace initialization testThis test validates the correct initialization configuration for the portal namespace, covering essential settings like language, namespaces, and interpolation.
317-332: Well-structured theme namespace initialization testThe test ensures theme namespace is correctly configured with appropriate settings, particularly the single curly brace interpolation format which differs from portal.
334-349: Complete newsletter namespace initialization testThis test properly validates the newsletter namespace configuration, maintaining consistency with the rest of the test suite.
351-357: Appropriate fallback language chain testTesting the Norwegian language fallback chain ensures multi-language support works correctly across regional variants, which is a key aspect of internationalization.
359-364: Good edge case handling for empty theme pathTesting the case when no theme path is provided ensures the system degrades gracefully rather than throwing errors.
Goal: Replace the old theme i18n implementation with use of i18next, matching what we do for the apps and ghost core.
This PR:
I don't love having an i18n and i18next folder, but I figure that once we're sure we like the new i18next implementation, we can nuke the logic in the t helper, the theme-engine/i18n folder, and that set of tests.