Closed
Conversation
- i18n support was added to themes a _really_ long time ago, and at the time there weren't great libraries - Since then, i18n has been added to more parts of Ghost, but leveraging the awesome i18next library - This PR brings theme i18n into line with the rest of Ghost by swapping out the custom code with i18next - The change exists behind a labs flag, with duplicated logic to make it easier to clean up later
Contributor
|
Warning Rate limit exceeded@ErisDS has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 26 minutes and 31 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughThis change introduces a new feature flag, Possibly related PRs
Suggested labels
✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
ghost/i18n/lib/i18n.js (2)
2-2: Unnecessary fs-extra importThe code uses
fs.readFileSyncfrom the built-infsmodule, notfs-extra. Consider removing this import if it's not used elsewhere in the file.-const fs = require('fs-extra'); +const fs = require('fs');
118-128: Consider using asynchronous file operationsThe synchronous
fs.readFileSyncoperations could block the event loop, especially if translation files are large or the filesystem is slow. Since this appears to be called during initialization, consider if the calling code could be refactored to support async operations.ghost/core/test/unit/frontend/helpers/t-new.test.js (1)
9-9: Consider using a more robust approach for test setupThe test directly manipulates module properties (
basePathand_i18n). This approach is brittle and may break if the internal implementation changes. Consider adding test-specific methods or using dependency injection for better testability.Also applies to: 13-13, 18-18, 23-23
ghost/i18n/test/i18n.test.js (1)
296-304: Important security consideration verifiedGood test verifying that HTML is not escaped in theme translations. This is intentional for theme flexibility but requires theme developers to be aware of potential XSS risks when using user-provided content in translations.
Consider documenting this behavior prominently in theme development documentation to ensure developers are aware of the security implications.
📜 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(3 hunks)ghost/i18n/test/i18n.test.js(4 hunks)
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
Learnt from: cathysarisky
PR: TryGhost/Ghost#0
File: :0-0
Timestamp: 2025-05-06T00:47:48.488Z
Learning: The theme i18n initialization in Ghost only happens during server boot, theme changes, and language changes - not on every page load or `t` helper call. The initialization check in the `t` helper is a safeguard that doesn't execute during normal page rendering once the theme is loaded.
Learnt from: cathysarisky
PR: TryGhost/Ghost#0
File: :0-0
Timestamp: 2025-05-20T21:08:21.026Z
Learning: In the Ghost project, translation files (ghost/i18n/locales/*/*.json) commonly have blank values for translations, and this is normal behavior that should not be flagged in reviews. These empty translations will be filled in separate PRs.
Learnt from: cathysarisky
PR: TryGhost/Ghost#21982
File: ghost/i18n/locales/hu v2/portal.json:70-70
Timestamp: 2025-01-29T15:23:58.658Z
Learning: Typos and spelling errors within localization files (ghost/i18n/locales/*.json) must be treated as important problems, not nitpicks, as they directly affect the user experience for speakers of that language. These issues should be flagged with high priority during code review.
Learnt from: cathysarisky
PR: TryGhost/Ghost#0
File: :0-0
Timestamp: 2025-01-29T15:27:29.391Z
Learning: When reviewing translation files in ghost/i18n/locales, translators should only modify the translated values and must not change the keys or placeholder styles (e.g., %%{status}%%, {site}, etc.) as they serve specific purposes like Mailgun string replacement.
Learnt from: cathysarisky
PR: TryGhost/Ghost#22175
File: ghost/i18n/locales/pt-BR/portal.json:18-18
Timestamp: 2025-02-12T20:09:12.731Z
Learning: In the Ghost project, new translation keys are first added with empty string values, and the actual translations are added later in separate PRs to maintain better organization of translation work.
Learnt from: cathysarisky
PR: TryGhost/Ghost#23070
File: ghost/i18n/locales/hu/comments.json:45-45
Timestamp: 2025-04-30T16:27:03.542Z
Learning: In the Ghost project, new translation string keys are intentionally added with empty values initially and will be translated in a separate PR. Do not report missing translations for entirely new string additions.
Learnt from: kevinansfield
PR: TryGhost/Ghost#23770
File: apps/admin-x-settings/src/components/settings/email/newsletters/NewsletterDetailModalLabs.tsx:435-450
Timestamp: 2025-06-10T11:07:10.800Z
Learning: In Ghost's newsletter customization features, when promoting a feature from alpha to beta status, the feature flag guards are updated to make the feature available under both the `emailCustomization` (beta) and `emailCustomizationAlpha` (alpha) flags. This is done by either removing conditional guards entirely when the component is already behind both flags, or by updating conditionals to check for either flag.
Learnt from: SebastianSchroeder
PR: TryGhost/Ghost#23602
File: ghost/i18n/locales/vi/ghost.json:3-4
Timestamp: 2025-05-30T11:41:47.336Z
Learning: SebastianSchroeder prefers to handle missing translations in separate PRs rather than including them in the same PR that adds the i18n infrastructure, to keep PRs focused on specific functionality.
Learnt from: cathysarisky
PR: TryGhost/Ghost#22175
File: ghost/i18n/locales/lt/portal.json:16-16
Timestamp: 2025-02-12T20:23:47.777Z
Learning: When reviewing localization changes, verify the actual history of translations before flagging potential deletions. Empty translation strings may be intentional placeholders rather than deleted content.
ghost/core/core/shared/labs.js (4)
Learnt from: kevinansfield
PR: TryGhost/Ghost#23770
File: apps/admin-x-settings/src/components/settings/email/newsletters/NewsletterDetailModalLabs.tsx:435-450
Timestamp: 2025-06-10T11:07:10.800Z
Learning: In Ghost's newsletter customization features, when promoting a feature from alpha to beta status, the feature flag guards are updated to make the feature available under both the `emailCustomization` (beta) and `emailCustomizationAlpha` (alpha) flags. This is done by either removing conditional guards entirely when the component is already behind both flags, or by updating conditionals to check for either flag.
Learnt from: cathysarisky
PR: TryGhost/Ghost#23070
File: ghost/i18n/locales/hu/comments.json:45-45
Timestamp: 2025-04-30T16:27:03.542Z
Learning: In the Ghost project, new translation string keys are intentionally added with empty values initially and will be translated in a separate PR. Do not report missing translations for entirely new string additions.
Learnt from: cathysarisky
PR: TryGhost/Ghost#0
File: :0-0
Timestamp: 2025-05-20T21:08:21.026Z
Learning: In the Ghost project, translation files (ghost/i18n/locales/*/*.json) commonly have blank values for translations, and this is normal behavior that should not be flagged in reviews. These empty translations will be filled in separate PRs.
Learnt from: cathysarisky
PR: TryGhost/Ghost#22175
File: ghost/i18n/locales/pt-BR/portal.json:18-18
Timestamp: 2025-02-12T20:09:12.731Z
Learning: In the Ghost project, new translation keys are first added with empty string values, and the actual translations are added later in separate PRs to maintain better organization of translation work.
ghost/core/core/frontend/services/handlebars.js (2)
Learnt from: cathysarisky
PR: TryGhost/Ghost#0
File: :0-0
Timestamp: 2025-05-06T00:47:48.488Z
Learning: The theme i18n initialization in Ghost only happens during server boot, theme changes, and language changes - not on every page load or `t` helper call. The initialization check in the `t` helper is a safeguard that doesn't execute during normal page rendering once the theme is loaded.
Learnt from: cathysarisky
PR: TryGhost/Ghost#0
File: :0-0
Timestamp: 2025-01-29T15:27:29.391Z
Learning: When reviewing translation files in ghost/i18n/locales, translators should only modify the translated values and must not change the keys or placeholder styles (e.g., %%{status}%%, {site}, etc.) as they serve specific purposes like Mailgun string replacement.
ghost/core/core/frontend/services/theme-engine/i18next/index.js (1)
Learnt from: cathysarisky
PR: TryGhost/Ghost#0
File: :0-0
Timestamp: 2025-05-06T00:47:48.488Z
Learning: The theme i18n initialization in Ghost only happens during server boot, theme changes, and language changes - not on every page load or `t` helper call. The initialization check in the `t` helper is a safeguard that doesn't execute during normal page rendering once the theme is loaded.
apps/admin-x-settings/src/components/settings/advanced/labs/BetaFeatures.tsx (2)
Learnt from: kevinansfield
PR: TryGhost/Ghost#23770
File: apps/admin-x-settings/src/components/settings/email/newsletters/NewsletterDetailModalLabs.tsx:435-450
Timestamp: 2025-06-10T11:07:10.800Z
Learning: In Ghost's newsletter customization features, when promoting a feature from alpha to beta status, the feature flag guards are updated to make the feature available under both the `emailCustomization` (beta) and `emailCustomizationAlpha` (alpha) flags. This is done by either removing conditional guards entirely when the component is already behind both flags, or by updating conditionals to check for either flag.
Learnt from: sagzy
PR: TryGhost/Ghost#24346
File: apps/admin-x-settings/src/components/settings/growth/Network.tsx:8-12
Timestamp: 2025-07-14T12:20:35.268Z
Learning: The Network component toggle in `apps/admin-x-settings/src/components/settings/growth/Network.tsx` is intentionally implemented as static UI with a no-op onChange handler, as part of a UI-first development approach before connecting actual ActivityPub functionality.
ghost/core/core/frontend/services/theme-engine/active.js (3)
Learnt from: cathysarisky
PR: TryGhost/Ghost#0
File: :0-0
Timestamp: 2025-05-06T00:47:48.488Z
Learning: The theme i18n initialization in Ghost only happens during server boot, theme changes, and language changes - not on every page load or `t` helper call. The initialization check in the `t` helper is a safeguard that doesn't execute during normal page rendering once the theme is loaded.
Learnt from: kevinansfield
PR: TryGhost/Ghost#23770
File: apps/admin-x-settings/src/components/settings/email/newsletters/NewsletterDetailModalLabs.tsx:435-450
Timestamp: 2025-06-10T11:07:10.800Z
Learning: In Ghost's newsletter customization features, when promoting a feature from alpha to beta status, the feature flag guards are updated to make the feature available under both the `emailCustomization` (beta) and `emailCustomizationAlpha` (alpha) flags. This is done by either removing conditional guards entirely when the component is already behind both flags, or by updating conditionals to check for either flag.
Learnt from: cathysarisky
PR: TryGhost/Ghost#23070
File: ghost/i18n/locales/hu/comments.json:45-45
Timestamp: 2025-04-30T16:27:03.542Z
Learning: In the Ghost project, new translation string keys are intentionally added with empty values initially and will be translated in a separate PR. Do not report missing translations for entirely new string additions.
ghost/core/core/frontend/helpers/t.js (7)
Learnt from: cathysarisky
PR: TryGhost/Ghost#0
File: :0-0
Timestamp: 2025-05-06T00:47:48.488Z
Learning: The theme i18n initialization in Ghost only happens during server boot, theme changes, and language changes - not on every page load or `t` helper call. The initialization check in the `t` helper is a safeguard that doesn't execute during normal page rendering once the theme is loaded.
Learnt from: kevinansfield
PR: TryGhost/Ghost#23770
File: apps/admin-x-settings/src/components/settings/email/newsletters/NewsletterDetailModalLabs.tsx:435-450
Timestamp: 2025-06-10T11:07:10.800Z
Learning: In Ghost's newsletter customization features, when promoting a feature from alpha to beta status, the feature flag guards are updated to make the feature available under both the `emailCustomization` (beta) and `emailCustomizationAlpha` (alpha) flags. This is done by either removing conditional guards entirely when the component is already behind both flags, or by updating conditionals to check for either flag.
Learnt from: cathysarisky
PR: TryGhost/Ghost#0
File: :0-0
Timestamp: 2025-01-29T15:27:29.391Z
Learning: When reviewing translation files in ghost/i18n/locales, translators should only modify the translated values and must not change the keys or placeholder styles (e.g., %%{status}%%, {site}, etc.) as they serve specific purposes like Mailgun string replacement.
Learnt from: cathysarisky
PR: TryGhost/Ghost#0
File: :0-0
Timestamp: 2025-05-20T21:08:21.026Z
Learning: In the Ghost project, translation files (ghost/i18n/locales/*/*.json) commonly have blank values for translations, and this is normal behavior that should not be flagged in reviews. These empty translations will be filled in separate PRs.
Learnt from: cathysarisky
PR: TryGhost/Ghost#23070
File: ghost/i18n/locales/hu/comments.json:45-45
Timestamp: 2025-04-30T16:27:03.542Z
Learning: In the Ghost project, new translation string keys are intentionally added with empty values initially and will be translated in a separate PR. Do not report missing translations for entirely new string additions.
Learnt from: cathysarisky
PR: TryGhost/Ghost#22175
File: ghost/i18n/locales/pt-BR/portal.json:18-18
Timestamp: 2025-02-12T20:09:12.731Z
Learning: In the Ghost project, new translation keys are first added with empty string values, and the actual translations are added later in separate PRs to maintain better organization of translation work.
Learnt from: cathysarisky
PR: TryGhost/Ghost#23161
File: ghost/i18n/lib/i18n.js:107-144
Timestamp: 2025-05-14T14:39:09.136Z
Learning: In Ghost's i18n system, translation keys are the actual English phrases, so when a translation is missing, displaying the key itself serves as the English fallback, eliminating the need for explicit fallback resource loading.
ghost/core/test/unit/frontend/services/theme-engine/i18next.test.js (7)
Learnt from: cathysarisky
PR: TryGhost/Ghost#0
File: :0-0
Timestamp: 2025-05-06T00:47:48.488Z
Learning: The theme i18n initialization in Ghost only happens during server boot, theme changes, and language changes - not on every page load or `t` helper call. The initialization check in the `t` helper is a safeguard that doesn't execute during normal page rendering once the theme is loaded.
Learnt from: mike182uk
PR: TryGhost/Ghost#22471
File: apps/admin-x-activitypub/src/utils/pending-activity.ts:13-71
Timestamp: 2025-03-13T09:00:20.205Z
Learning: The pending activity utilities in the Ghost ActivityPub module are covered by tests in the file `apps/admin-x-activitypub/test/unit/utils/pending-activity.ts`.
Learnt from: cathysarisky
PR: TryGhost/Ghost#23070
File: ghost/i18n/locales/hu/comments.json:45-45
Timestamp: 2025-04-30T16:27:03.542Z
Learning: In the Ghost project, new translation string keys are intentionally added with empty values initially and will be translated in a separate PR. Do not report missing translations for entirely new string additions.
Learnt from: cathysarisky
PR: TryGhost/Ghost#0
File: :0-0
Timestamp: 2025-05-20T21:08:21.026Z
Learning: In the Ghost project, translation files (ghost/i18n/locales/*/*.json) commonly have blank values for translations, and this is normal behavior that should not be flagged in reviews. These empty translations will be filled in separate PRs.
Learnt from: mike182uk
PR: TryGhost/Ghost#22471
File: apps/admin-x-activitypub/src/utils/pending-activity.ts:13-71
Timestamp: 2025-03-13T09:00:20.205Z
Learning: The pending activity utilities in Ghost's ActivityPub module are thoroughly tested in `apps/admin-x-activitypub/test/unit/utils/pending-activity.ts`, which covers `generatePendingActivityId`, `isPendingActivity`, and `generatePendingActivity` functions.
Learnt from: cathysarisky
PR: TryGhost/Ghost#0
File: :0-0
Timestamp: 2025-01-29T15:27:29.391Z
Learning: When reviewing translation files in ghost/i18n/locales, translators should only modify the translated values and must not change the keys or placeholder styles (e.g., %%{status}%%, {site}, etc.) as they serve specific purposes like Mailgun string replacement.
Learnt from: cathysarisky
PR: TryGhost/Ghost#23161
File: ghost/i18n/lib/i18n.js:107-144
Timestamp: 2025-05-14T14:39:09.136Z
Learning: In Ghost's i18n system, translation keys are the actual English phrases, so when a translation is missing, displaying the key itself serves as the English fallback, eliminating the need for explicit fallback resource loading.
ghost/core/test/unit/frontend/helpers/t-new.test.js (8)
Learnt from: cathysarisky
PR: TryGhost/Ghost#0
File: :0-0
Timestamp: 2025-05-06T00:47:48.488Z
Learning: The theme i18n initialization in Ghost only happens during server boot, theme changes, and language changes - not on every page load or `t` helper call. The initialization check in the `t` helper is a safeguard that doesn't execute during normal page rendering once the theme is loaded.
Learnt from: mike182uk
PR: TryGhost/Ghost#22471
File: apps/admin-x-activitypub/src/utils/pending-activity.ts:13-71
Timestamp: 2025-03-13T09:00:20.205Z
Learning: The pending activity utilities in the Ghost ActivityPub module are covered by tests in the file `apps/admin-x-activitypub/test/unit/utils/pending-activity.ts`.
Learnt from: mike182uk
PR: TryGhost/Ghost#22471
File: apps/admin-x-activitypub/src/utils/pending-activity.ts:13-71
Timestamp: 2025-03-13T09:00:20.205Z
Learning: The pending activity utilities in Ghost's ActivityPub module are thoroughly tested in `apps/admin-x-activitypub/test/unit/utils/pending-activity.ts`, which covers `generatePendingActivityId`, `isPendingActivity`, and `generatePendingActivity` functions.
Learnt from: cathysarisky
PR: TryGhost/Ghost#23070
File: ghost/i18n/locales/hu/comments.json:45-45
Timestamp: 2025-04-30T16:27:03.542Z
Learning: In the Ghost project, new translation string keys are intentionally added with empty values initially and will be translated in a separate PR. Do not report missing translations for entirely new string additions.
Learnt from: cathysarisky
PR: TryGhost/Ghost#0
File: :0-0
Timestamp: 2025-05-20T21:08:21.026Z
Learning: In the Ghost project, translation files (ghost/i18n/locales/*/*.json) commonly have blank values for translations, and this is normal behavior that should not be flagged in reviews. These empty translations will be filled in separate PRs.
Learnt from: cathysarisky
PR: TryGhost/Ghost#22175
File: ghost/i18n/locales/pt-BR/portal.json:18-18
Timestamp: 2025-02-12T20:09:12.731Z
Learning: In the Ghost project, new translation keys are first added with empty string values, and the actual translations are added later in separate PRs to maintain better organization of translation work.
Learnt from: cathysarisky
PR: TryGhost/Ghost#0
File: :0-0
Timestamp: 2025-01-29T15:27:29.391Z
Learning: When reviewing translation files in ghost/i18n/locales, translators should only modify the translated values and must not change the keys or placeholder styles (e.g., %%{status}%%, {site}, etc.) as they serve specific purposes like Mailgun string replacement.
Learnt from: ErisDS
PR: TryGhost/Ghost#23588
File: ghost/core/test/e2e-api/admin/backup.test.js:136-148
Timestamp: 2025-05-29T10:37:26.369Z
Learning: In the Ghost test framework, when testing CSV exports via the admin API, the response `body` field is empty while the actual CSV data is provided through the `text` field. Tests should use `expectEmptyBody()` and then validate the CSV content via `.expect(({text}) => ...)` - this is not contradictory behavior.
ghost/i18n/lib/i18n.js (4)
Learnt from: cathysarisky
PR: TryGhost/Ghost#0
File: :0-0
Timestamp: 2025-05-06T00:47:48.488Z
Learning: The theme i18n initialization in Ghost only happens during server boot, theme changes, and language changes - not on every page load or `t` helper call. The initialization check in the `t` helper is a safeguard that doesn't execute during normal page rendering once the theme is loaded.
Learnt from: cathysarisky
PR: TryGhost/Ghost#0
File: :0-0
Timestamp: 2025-01-29T15:27:29.391Z
Learning: When reviewing translation files in ghost/i18n/locales, translators should only modify the translated values and must not change the keys or placeholder styles (e.g., %%{status}%%, {site}, etc.) as they serve specific purposes like Mailgun string replacement.
Learnt from: cathysarisky
PR: TryGhost/Ghost#23161
File: ghost/i18n/lib/i18n.js:107-144
Timestamp: 2025-05-14T14:39:09.136Z
Learning: In Ghost's i18n system, translation keys are the actual English phrases, so when a translation is missing, displaying the key itself serves as the English fallback, eliminating the need for explicit fallback resource loading.
Learnt from: cathysarisky
PR: TryGhost/Ghost#0
File: :0-0
Timestamp: 2025-05-20T21:08:21.026Z
Learning: In the Ghost project, translation files (ghost/i18n/locales/*/*.json) commonly have blank values for translations, and this is normal behavior that should not be flagged in reviews. These empty translations will be filled in separate PRs.
ghost/i18n/test/i18n.test.js (13)
Learnt from: cathysarisky
PR: TryGhost/Ghost#0
File: :0-0
Timestamp: 2025-05-06T00:47:48.488Z
Learning: The theme i18n initialization in Ghost only happens during server boot, theme changes, and language changes - not on every page load or `t` helper call. The initialization check in the `t` helper is a safeguard that doesn't execute during normal page rendering once the theme is loaded.
Learnt from: cathysarisky
PR: TryGhost/Ghost#0
File: :0-0
Timestamp: 2025-05-20T21:08:21.026Z
Learning: In the Ghost project, translation files (ghost/i18n/locales/*/*.json) commonly have blank values for translations, and this is normal behavior that should not be flagged in reviews. These empty translations will be filled in separate PRs.
Learnt from: cathysarisky
PR: TryGhost/Ghost#0
File: :0-0
Timestamp: 2025-01-29T15:27:29.391Z
Learning: When reviewing translation files in ghost/i18n/locales, translators should only modify the translated values and must not change the keys or placeholder styles (e.g., %%{status}%%, {site}, etc.) as they serve specific purposes like Mailgun string replacement.
Learnt from: mike182uk
PR: TryGhost/Ghost#22471
File: apps/admin-x-activitypub/src/utils/pending-activity.ts:13-71
Timestamp: 2025-03-13T09:00:20.205Z
Learning: The pending activity utilities in the Ghost ActivityPub module are covered by tests in the file `apps/admin-x-activitypub/test/unit/utils/pending-activity.ts`.
Learnt from: cathysarisky
PR: TryGhost/Ghost#23070
File: ghost/i18n/locales/hu/comments.json:45-45
Timestamp: 2025-04-30T16:27:03.542Z
Learning: In the Ghost project, new translation string keys are intentionally added with empty values initially and will be translated in a separate PR. Do not report missing translations for entirely new string additions.
Learnt from: cathysarisky
PR: TryGhost/Ghost#21982
File: ghost/i18n/locales/hu v2/portal.json:70-70
Timestamp: 2025-01-29T15:23:58.658Z
Learning: Typos and spelling errors within localization files (ghost/i18n/locales/*.json) must be treated as important problems, not nitpicks, as they directly affect the user experience for speakers of that language. These issues should be flagged with high priority during code review.
Learnt from: cathysarisky
PR: TryGhost/Ghost#23161
File: ghost/i18n/lib/i18n.js:107-144
Timestamp: 2025-05-14T14:39:09.136Z
Learning: In Ghost's i18n system, translation keys are the actual English phrases, so when a translation is missing, displaying the key itself serves as the English fallback, eliminating the need for explicit fallback resource loading.
Learnt from: cathysarisky
PR: TryGhost/Ghost#21651
File: ghost/i18n/locales/ar/comments.json:1-1
Timestamp: 2025-01-29T15:35:26.447Z
Learning: All JSON files containing translation strings in ghost/i18n/locales must include both opening and closing braces to maintain valid JSON structure.
Learnt from: mike182uk
PR: TryGhost/Ghost#22471
File: apps/admin-x-activitypub/src/utils/pending-activity.ts:13-71
Timestamp: 2025-03-13T09:00:20.205Z
Learning: The pending activity utilities in Ghost's ActivityPub module are thoroughly tested in `apps/admin-x-activitypub/test/unit/utils/pending-activity.ts`, which covers `generatePendingActivityId`, `isPendingActivity`, and `generatePendingActivity` functions.
Learnt from: cathysarisky
PR: TryGhost/Ghost#22075
File: ghost/i18n/locales/pt-BR/portal.json:0-0
Timestamp: 2025-01-30T15:11:25.199Z
Learning: When reviewing translation files in i18n/locales/*, focus on identifying spelling errors in the translation values (right side) of the JSON key-value pairs, not the keys (left side).
Learnt from: cathysarisky
PR: TryGhost/Ghost#22007
File: ghost/i18n/locales/sl/newsletter.json:18-18
Timestamp: 2025-01-28T13:58:08.393Z
Learning: When reviewing translations of newsletter.json, check that the translations of status words (free/complimentary/paid/trialing) will form grammatically correct sentences when substituted into the message template. The status words should agree grammatically with the word "subscriber" in the target language.
Learnt from: cathysarisky
PR: TryGhost/Ghost#22175
File: ghost/i18n/locales/pt-BR/portal.json:18-18
Timestamp: 2025-02-12T20:09:12.731Z
Learning: In the Ghost project, new translation keys are first added with empty string values, and the actual translations are added later in separate PRs to maintain better organization of translation work.
Learnt from: cathysarisky
PR: TryGhost/Ghost#21982
File: ghost/i18n/locales/hu/portal.json:207-207
Timestamp: 2025-02-15T16:00:03.347Z
Learning: In Hungarian translations for Ghost, ensure that all variables like {{title}} are preserved in the translated strings and not omitted.
ghost/core/core/frontend/services/theme-engine/i18next/ThemeI18n.js (6)
Learnt from: cathysarisky
PR: TryGhost/Ghost#0
File: :0-0
Timestamp: 2025-05-06T00:47:48.488Z
Learning: The theme i18n initialization in Ghost only happens during server boot, theme changes, and language changes - not on every page load or `t` helper call. The initialization check in the `t` helper is a safeguard that doesn't execute during normal page rendering once the theme is loaded.
Learnt from: cathysarisky
PR: TryGhost/Ghost#23161
File: ghost/i18n/lib/i18n.js:107-144
Timestamp: 2025-05-14T14:39:09.136Z
Learning: In Ghost's i18n system, translation keys are the actual English phrases, so when a translation is missing, displaying the key itself serves as the English fallback, eliminating the need for explicit fallback resource loading.
Learnt from: cathysarisky
PR: TryGhost/Ghost#0
File: :0-0
Timestamp: 2025-01-29T15:27:29.391Z
Learning: When reviewing translation files in ghost/i18n/locales, translators should only modify the translated values and must not change the keys or placeholder styles (e.g., %%{status}%%, {site}, etc.) as they serve specific purposes like Mailgun string replacement.
Learnt from: cathysarisky
PR: TryGhost/Ghost#23070
File: ghost/i18n/locales/hu/comments.json:45-45
Timestamp: 2025-04-30T16:27:03.542Z
Learning: In the Ghost project, new translation string keys are intentionally added with empty values initially and will be translated in a separate PR. Do not report missing translations for entirely new string additions.
Learnt from: aileen
PR: TryGhost/Ghost#24073
File: apps/admin-x-settings/src/components/settings/site/DesignAndThemeModal.tsx:68-85
Timestamp: 2025-06-26T14:53:58.238Z
Learning: In Ghost's theme limit checking functionality (useCheckThemeLimitError hook), the `checkThemeLimitError` function is designed to always resolve and return either an error message string if the theme is disallowed or null if allowed. It does not throw/reject promises - any internal errors are caught within the hook and converted to return values, so `.catch()` blocks are not needed when calling this function.
Learnt from: cathysarisky
PR: TryGhost/Ghost#21651
File: ghost/i18n/locales/ar/comments.json:1-1
Timestamp: 2025-01-29T15:35:26.447Z
Learning: All JSON files containing translation strings in ghost/i18n/locales must include both opening and closing braces to maintain valid JSON structure.
🧬 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/test/unit/frontend/services/theme-engine/i18next.test.js (3)
ghost/core/test/unit/frontend/helpers/t-new.test.js (3)
should(1-1)sinon(3-3)path(2-2)ghost/core/core/frontend/services/theme-engine/i18next/index.js (1)
ThemeI18n(3-3)ghost/core/core/frontend/services/theme-engine/i18next/ThemeI18n.js (1)
path(3-3)
ghost/core/test/unit/frontend/helpers/t-new.test.js (3)
ghost/core/test/unit/frontend/services/theme-engine/i18next.test.js (3)
should(1-1)path(4-4)sinon(2-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/active.js (2)
themeI18next(21-21)labs(22-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Setup
🔇 Additional comments (18)
ghost/core/core/shared/labs.js (1)
37-38: LGTM! Feature flag correctly added to beta features.The
themeTranslationflag is properly positioned and follows the established naming convention for feature flags.ghost/core/core/frontend/services/handlebars.js (1)
22-22: LGTM! New theme translation service properly exported.The
themeI18nextexport is correctly added alongside the legacythemeI18nto support the feature flag-controlled transition to the new translation system.apps/admin-x-settings/src/components/settings/advanced/labs/BetaFeatures.tsx (1)
74-77: LGTM! Feature toggle correctly implemented in admin UI.The new theme translation toggle follows the established pattern and provides clear messaging about the feature's purpose.
ghost/core/core/frontend/services/theme-engine/i18next/index.js (1)
1-6: LGTM! Clean module structure with proper configuration.The module correctly exports both an initialized instance and the class itself, with appropriate configuration for the themes directory.
ghost/core/core/frontend/services/theme-engine/active.js (2)
21-22: LGTM! Required imports added for feature flag functionality.The new imports for
themeI18nextandlabsare correctly added to support the conditional translation system initialization.
106-112: LGTM! Feature flag-controlled translation system initialization implemented correctly.The conditional logic properly switches between the new i18next-based and legacy translation systems based on the
themeTranslationfeature flag, maintaining backward compatibility during the transition.ghost/core/core/frontend/services/theme-engine/i18next/ThemeI18n.js (4)
1-20: LGTM! Constructor properly validates required parameters.The constructor correctly validates the required
basePathparameter and initializes all properties with sensible defaults. Error handling using Ghost'sIncorrectUsageErroris appropriate.
22-31: LGTM! Simple getter/setter for testing purposes.The basePath getter/setter implementation is clean and serves its intended purpose for testing scenarios.
41-86: Robust fallback logic with comprehensive error handling.The initialization method implements a well-thought-out fallback strategy:
- Theme locales directory missing → no-op translation
- Requested locale missing → fallback to English
- English missing → no-op translation
The async file existence checks before calling
i18nLibare prudent given the synchronous nature of the underlying library.
95-101: Good safeguard against uninitialized usage.The method correctly validates that initialization has occurred and ensures a consistent string return type. The error message including the translation key will be helpful for debugging.
ghost/core/test/unit/frontend/services/theme-engine/i18next.test.js (2)
27-44: Well-structured translation and fallback testsGood coverage of translation retrieval scenarios including successful translation, fallback to English, and fallback to key when no translation files exist.
51-56: Excellent error handling testGood test for the defensive check that prevents usage before initialization. The error message helpfully includes the key that triggered the error.
ghost/i18n/lib/i18n.js (1)
107-144: Well-structured resource loading with appropriate fallbacksThe implementation correctly handles theme namespace resource loading with a sensible fallback chain (requested locale → English → empty object). The error handling ensures the system remains functional even when translation files are missing or malformed.
ghost/core/test/unit/frontend/helpers/t-new.test.js (2)
26-65: Comprehensive translation tests with good coverageExcellent test coverage for different locales and fallback scenarios. The tests properly verify the integration between the helper and the theme translation service.
66-81: Confirmed: early return handles empty/undefined keys without initializationThe
thelper’s very first checkif (!text || text.length === 0) { return ''; }returns an empty string before any call to
themeI18next.init(). The existing tests for''andundefinedkeys are therefore valid and don’t require an explicitinit()call.No changes needed.
ghost/i18n/test/i18n.test.js (3)
170-187: Well-structured test setup with proper cleanupGood use of temporary directories and proper cleanup to ensure test isolation. The beforeEach/afterEach pattern ensures tests don't interfere with each other.
188-275: Comprehensive theme translation test coverageExcellent test suite covering:
- Translation loading from theme directories
- Fallback behavior (missing translations, missing files)
- Error handling (invalid JSON)
- Configuration verification
- Variable interpolation
The tests are well-structured and cover both happy paths and edge cases.
306-373: Thorough i18next configuration testsExcellent coverage of i18next initialization including:
- Configuration verification for all namespaces
- Fallback language chain testing
- Edge case handling for missing theme paths
These tests ensure the i18next instances are configured consistently and correctly.
Comment on lines
+36
to
+42
| // Initialize only if needed | ||
| if (!themeI18next._i18n) { | ||
| themeI18next.init({ | ||
| activeTheme: settingsCache.get('active_theme'), | ||
| locale: config.get('locale') | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
Fix initialization logic to avoid race conditions and encapsulation violations
The current implementation has several issues:
- Accessing the private
_i18nproperty violates encapsulation - The initialization could race if multiple requests arrive before completion
- The
init()call appears to be async but isn't awaited
Based on the learning that theme i18n initialization only happens during server boot, theme changes, and language changes, this initialization should not be in the helper function.
Consider moving the initialization to the appropriate lifecycle hooks (server boot, theme change, language change) and ensure the helper assumes themeI18next is already initialized:
if (labs.isSet('themeTranslation')) {
// Use the new translation package when feature flag is enabled
-
- // Initialize only if needed
- if (!themeI18next._i18n) {
- themeI18next.init({
- activeTheme: settingsCache.get('active_theme'),
- locale: config.get('locale')
- });
- }
-
return themeI18next.t(text, bindings);
}The initialization should be handled in the theme engine's active theme service or during application bootstrap.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Initialize only if needed | |
| if (!themeI18next._i18n) { | |
| themeI18next.init({ | |
| activeTheme: settingsCache.get('active_theme'), | |
| locale: config.get('locale') | |
| }); | |
| } | |
| if (labs.isSet('themeTranslation')) { | |
| // Use the new translation package when feature flag is enabled | |
| return themeI18next.t(text, bindings); | |
| } |
🤖 Prompt for AI Agents
In ghost/core/core/frontend/helpers/t.js around lines 36 to 42, remove the
initialization logic that checks and initializes themeI18next inside the helper
function to avoid accessing the private _i18n property, prevent race conditions,
and handle async initialization properly. Instead, move the themeI18next
initialization to the appropriate lifecycle hooks such as server boot, theme
changes, or language changes within the theme engine's active theme service or
application bootstrap. Update the helper to assume themeI18next is already
initialized and remove any initialization code from it.
Comment on lines
+19
to
+20
| i18n._locale.should.eql('en'); | ||
| }); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid accessing private properties in tests
The test directly accesses the private property _locale. Consider adding a public getter method to the ThemeI18n class to expose the current locale, making the test less brittle to implementation changes.
🤖 Prompt for AI Agents
In ghost/core/test/unit/frontend/services/theme-engine/i18next.test.js around
lines 19 to 20, the test accesses the private property _locale directly, which
is brittle. Modify the ThemeI18n class to add a public getter method that
returns the current locale, then update the test to use this getter instead of
accessing _locale directly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is the OG change PR'd with browser-tests on