🐛Reduce i18n reinitializations in EmailServiceWrapper #25586
Merged
Conversation
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
Contributor
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe EmailServiceWrapper.js change removes the Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on December 3
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ghost/core/core/server/services/email-service/EmailServiceWrapper.js(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: sagzy
Repo: TryGhost/Ghost PR: 24673
File: ghost/i18n/lib/i18n.js:34-35
Timestamp: 2025-11-24T11:12:15.712Z
Learning: In the Ghost i18n package (ghost/i18n/lib/i18n.js), changing existing locale codes requires backwards compatibility handling for users who have already configured those locales. Such changes should be done in a separate PR with migration logic rather than included in feature PRs.
Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 25118
File: apps/portal/src/actions.js:160-173
Timestamp: 2025-10-09T15:31:06.587Z
Learning: When reviewing PRs that introduce feature-flagged changes (e.g., `labs?.membersSigninOTCAlpha`), avoid suggesting modifications to non-flagged code paths unless they're directly related to the PR's objectives. Keep the scope focused on the feature-flag-specific changes only.
Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 25118
File: apps/portal/src/App.js:719-719
Timestamp: 2025-10-09T14:16:58.334Z
Learning: In Ghost Portal (apps/portal), avoid suggesting changes to translation strings (wrapped in `t()`) unless directly related to the PR's primary objective, as changing them invalidates existing translations and creates unnecessary churn. If a typo or improvement is found in a translation string, it should be fixed across all instances in a dedicated PR.
Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 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.
📚 Learning: 2025-10-30T17:13:26.190Z
Learnt from: sam-lord
Repo: TryGhost/Ghost PR: 25303
File: ghost/core/core/server/services/email-service/BatchSendingService.js:19-19
Timestamp: 2025-10-30T17:13:26.190Z
Learning: In ghost/core/core/server/services/email-service/BatchSendingService.js and similar files in the Ghost codebase, prefer using `{...options}` spread syntax without explicit guards like `...(options || {})` when spreading potentially undefined objects, as the maintainer prefers cleaner syntax over defensive patterns when the behavior is safe.
Applied to files:
ghost/core/core/server/services/email-service/EmailServiceWrapper.js
📚 Learning: 2025-11-24T11:12:15.712Z
Learnt from: sagzy
Repo: TryGhost/Ghost PR: 24673
File: ghost/i18n/lib/i18n.js:34-35
Timestamp: 2025-11-24T11:12:15.712Z
Learning: In the Ghost i18n package (ghost/i18n/lib/i18n.js), changing existing locale codes requires backwards compatibility handling for users who have already configured those locales. Such changes should be done in a separate PR with migration logic rather than included in feature PRs.
Applied to files:
ghost/core/core/server/services/email-service/EmailServiceWrapper.js
📚 Learning: 2025-01-29T15:27:29.391Z
Learnt from: cathysarisky
Repo: TryGhost/Ghost PR: 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.
Applied to files:
ghost/core/core/server/services/email-service/EmailServiceWrapper.js
📚 Learning: 2025-05-06T00:47:48.488Z
Learnt from: cathysarisky
Repo: TryGhost/Ghost PR: 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.
Applied to files:
ghost/core/core/server/services/email-service/EmailServiceWrapper.js
📚 Learning: 2025-11-25T14:28:50.351Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:28:50.351Z
Learning: Centralized i18n translations should be maintained in `ghost/i18n/locales/{locale}/{namespace}.json` files with 60+ supported locales
Applied to files:
ghost/core/core/server/services/email-service/EmailServiceWrapper.js
📚 Learning: 2025-11-25T14:28:50.351Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:28:50.351Z
Learning: Applies to ghost/i18n/locales/en/ghost.json : Add UI translations to `ghost/i18n/locales/en/ghost.json` for Admin UI features
Applied to files:
ghost/core/core/server/services/email-service/EmailServiceWrapper.js
🧬 Code graph analysis (1)
ghost/core/core/server/services/email-service/EmailServiceWrapper.js (2)
ghost/core/test/unit/server/services/email-service/email-renderer.test.js (1)
i18n(78-78)ghost/core/test/unit/server/services/email-service/email-helpers.test.js (1)
i18n(6-6)
⏰ 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). (6)
- GitHub Check: Legacy tests (Node 22.18.0, mysql8)
- GitHub Check: Acceptance tests (Node 22.18.0, sqlite3)
- GitHub Check: Acceptance tests (Node 22.18.0, mysql8)
- GitHub Check: Unit tests (Node 22.18.0)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Build & Push
🔇 Additional comments (1)
ghost/core/core/server/services/email-service/EmailServiceWrapper.js (1)
65-68: LGTM! Simplified locale change handler.The event handler now directly updates the i18n language whenever the locale setting changes, without conditionally checking the labs flag. This aligns with the PR objective of removing stale i18n feature flag logic now that i18n is GA.
ghost/core/core/server/services/email-service/EmailServiceWrapper.js
Outdated
Show resolved
Hide resolved
kevinansfield
approved these changes
Dec 10, 2025
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.
We had some stale code that fired whenever a labs setting changed. Since i18n is now GA, we don't need this check. Further, it actually runs every time any flag is changed, resulting in unneeded language changes (and debug log spam) for totally unrelated changes.
Note
Always use
settings.localefor i18n and update on locale changes only; remove labs-based checks and thesettings.labs.editedlistener.core/server/services/email-service/EmailServiceWrapper.js):settingsCache.get('locale') || 'en'(remove labs-gated fallback).settings.labs.editedlistener and related conditionalchangeLanguagelogic.settings.locale.edited, always calli18n.changeLanguagewith the new value (no labs checks).Written by Cursor Bugbot for commit 794a73c. This will update automatically on new commits. Configure here.