Added warning icon to editor when email size exceeds clipping threshold #25689
Conversation
WalkthroughIntroduces an email size warning feature: adds a feature flag Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
![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 (2)
ghost/admin/app/components/editor/email-size-warning.hbs (1)
8-10: Consider clarifying the threshold messaging.The popup mentions "over 100kb" as the cutoff, but warnings appear at 85kb (yellow) and 95kb (red). This is intentional to warn users before reaching the limit, but the messaging could be clearer about why they're seeing a warning before the 100kb mark.
Consider rewording to something like: "...when they're over 100kb. Warning shown at 85kb."
ghost/admin/app/components/editor/email-size-warning.js (1)
79-84: Add defensive check for response structure.The destructuring
const [emailPreview] = response.email_previewscould throw ifresponseis undefined oremail_previewsis not an array. Consider adding a guard.const response = yield this.ajax.request(url.href); - const [emailPreview] = response.email_previews; + const emailPreview = response?.email_previews?.[0]; if (emailPreview?.html) {
📜 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 (7)
apps/admin-x-settings/src/components/settings/advanced/labs/private-features.tsx(1 hunks)ghost/admin/app/components/editor/email-size-warning.hbs(1 hunks)ghost/admin/app/components/editor/email-size-warning.js(1 hunks)ghost/admin/app/services/feature.js(2 hunks)ghost/admin/app/styles/layouts/editor.css(1 hunks)ghost/admin/app/templates/lexical-editor.hbs(2 hunks)ghost/core/core/shared/labs.js(1 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
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.
Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 23824
File: ghost/core/core/server/services/email-service/email-templates/partials/styles.hbs:919-926
Timestamp: 2025-06-13T11:57:58.226Z
Learning: In `ghost/core/core/server/services/email-service/email-templates/partials/styles.hbs`, some style blocks (e.g., `.latest-post p` and `.latest-post p a`) still use the legacy colour `#73818c` on purpose; they are later overridden by `emailCustomization` feature rules, as noted by inline TODO comments. These occurrences should not be flagged as inconsistencies.
📚 Learning: 2025-06-13T11:57:58.226Z
Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 23824
File: ghost/core/core/server/services/email-service/email-templates/partials/styles.hbs:919-926
Timestamp: 2025-06-13T11:57:58.226Z
Learning: In `ghost/core/core/server/services/email-service/email-templates/partials/styles.hbs`, some style blocks (e.g., `.latest-post p` and `.latest-post p a`) still use the legacy colour `#73818c` on purpose; they are later overridden by `emailCustomization` feature rules, as noted by inline TODO comments. These occurrences should not be flagged as inconsistencies.
Applied to files:
ghost/admin/app/styles/layouts/editor.cssghost/core/core/shared/labs.jsghost/admin/app/components/editor/email-size-warning.hbsghost/admin/app/templates/lexical-editor.hbsghost/admin/app/components/editor/email-size-warning.js
📚 Learning: 2025-08-14T20:54:20.921Z
Learnt from: troyciesco
Repo: TryGhost/Ghost PR: 24665
File: ghost/core/core/server/services/koenig/node-renderers/header-v2-renderer.js:113-119
Timestamp: 2025-08-14T20:54:20.921Z
Learning: In Ghost's header card email templates, .kg-header-card-content has 40px padding applied via CSS. When using Outlook VML textbox inset for background images, the inset values must match this padding (approximately 30pt,30pt,30pt,30pt) to maintain consistent content positioning across email clients.
Applied to files:
ghost/admin/app/styles/layouts/editor.css
📚 Learning: 2025-06-10T11:07:10.800Z
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.
Applied to files:
ghost/admin/app/styles/layouts/editor.cssghost/core/core/shared/labs.jsghost/admin/app/components/editor/email-size-warning.hbsghost/admin/app/components/editor/email-size-warning.jsapps/admin-x-settings/src/components/settings/advanced/labs/private-features.tsxghost/admin/app/services/feature.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/shared/labs.jsghost/admin/app/components/editor/email-size-warning.hbs
📚 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/shared/labs.js
📚 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/shared/labs.jsghost/admin/app/services/feature.js
📚 Learning: 2025-10-09T15:31:06.587Z
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.
Applied to files:
apps/admin-x-settings/src/components/settings/advanced/labs/private-features.tsx
📚 Learning: 2025-11-03T12:33:31.093Z
Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 25320
File: apps/admin/src/layout/app-sidebar/NavHeader.tsx:10-10
Timestamp: 2025-11-03T12:33:31.093Z
Learning: The Ghost admin apps (apps/admin/**) do not use SSR, so accessing browser APIs like `navigator`, `window`, or `document` at module load time is safe and does not require typeof guards.
Applied to files:
ghost/admin/app/services/feature.js
⏰ 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). (10)
- GitHub Check: Acceptance tests (Node 22.18.0, mysql8)
- GitHub Check: Acceptance tests (Node 22.18.0, sqlite3)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Admin-X Settings tests
- GitHub Check: Admin tests - Chrome
- GitHub Check: Legacy tests (Node 22.18.0, mysql8)
- GitHub Check: Unit tests (Node 22.18.0)
- GitHub Check: Legacy tests (Node 22.18.0, sqlite3)
- GitHub Check: Lint
- GitHub Check: Build & Push Docker Image
🔇 Additional comments (9)
ghost/core/core/shared/labs.js (1)
56-57: LGTM!The new
emailSizeWarningsfeature flag is correctly added toPRIVATE_FEATURESwith proper trailing comma formatting.ghost/admin/app/templates/lexical-editor.hbs (1)
109-109: LGTM!The
EmailSizeWarningcomponent is correctly integrated in both desktop and mobile editor layouts, passing the post reference as expected.Also applies to: 123-123
apps/admin-x-settings/src/components/settings/advanced/labs/private-features.tsx (1)
59-64: LGTM!The feature entry is correctly added with appropriate title, description, and flag name matching the backend configuration.
ghost/admin/app/services/feature.js (1)
97-97: LGTM!The
emailSizeWarningsfeature flag is correctly exposed following the established pattern for labs flags.ghost/admin/app/styles/layouts/editor.css (1)
425-468: LGTM!The CSS styling is well-structured, using appropriate CSS variables for theming consistency. The hover-triggered popup with proper z-index layering follows established patterns in the codebase.
ghost/admin/app/components/editor/email-size-warning.js (4)
7-8: LGTM on threshold constants.Clear constants with comments explaining the thresholds. The values (85KB yellow, 95KB red) provide appropriate early warnings before the Gmail 102KB clipping threshold.
22-29: Well-designed enablement logic.The
isEnabledgetter correctly gates the feature behind the feature flag and appropriately disables for: newsletters disabled, missing post, already-sent posts, and new/unsaved posts.
60-98: Overall task implementation looks solid.The use of
restartabletask is appropriate to cancel previous in-flight requests when new checks are triggered. TheBlobapproach for measuring byte size is a reliable cross-browser method. Error handling gracefully fails without disrupting the user's editing experience.
49-58: Newsletter selection ordering is inconsistent across similar use cases.The
order: 'sort_order DESC'withlimit: 1uniquely selects the newsletter with the highest sort_order value. However, the similaremail-preview.jscomponent selects a newsletter with justlimit: 1(no explicit ordering), and all other newsletter queries in the codebase omit the order clause entirely. Additionally, other parts of the system (e.g., link-redirection) usesort_order ASCordering. Clarify whether DESC is intentional for this specific preview use case or if ordering should be removed to match the pattern used elsewhere.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #25689 +/- ##
==========================================
- Coverage 72.10% 72.08% -0.03%
==========================================
Files 1536 1539 +3
Lines 117532 117719 +187
Branches 14072 14102 +30
==========================================
+ Hits 84748 84859 +111
- Misses 31759 31845 +86
+ Partials 1025 1015 -10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
React E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 20107950686 -n playwright-report-react -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
kevinansfield
force-pushed
the
email-size-warning
branch
from
862e8ee to
d1e97e0
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
ghost/admin/app/components/editor/email-size-warning.hbs (1)
1-15: Add accessibility attributes for screen readers and keyboard navigation.The warning popup lacks accessibility features:
- The trigger has no
role,aria-label, oraria-describedbyattributes- The popup is only visible on hover, making it inaccessible via keyboard navigation
- There's no way for keyboard users to access the warning information
Consider adding accessibility attributes and keyboard support:
{{#if this.isEnabled}} {{! Watch for post.updatedAt changes to trigger recalculation after saves }} <span {{did-update this.checkEmailSize @post.updatedAtUTC}}> {{#if this.warningLevel}} - <span class="gh-editor-email-size-warning gh-editor-email-size-warning--{{this.warningLevel}}"> + <span + class="gh-editor-email-size-warning gh-editor-email-size-warning--{{this.warningLevel}}" + role="button" + tabindex="0" + aria-label="Email size warning: {{this.emailSizeKb}}kb" + aria-describedby="email-size-popup-{{this.elementId}}"> {{svg-jar "email" class=(concat "fill-" this.warningLevel)}} - <div class="gh-editor-email-size-popup"> + <div class="gh-editor-email-size-popup" id="email-size-popup-{{this.elementId}}" role="tooltip"> <span class="gh-editor-email-size-popup-title">Looks like this is a long post</span> <span class="gh-editor-email-size-popup-text">Email newsletters may get cut off in the inbox behind a "View entire message" link when they're over 100kb.</span> <span class="gh-editor-email-size-popup-title">You've used: <span class={{this.warningLevel}}>{{this.emailSizeKb}}kb</span></span> </div> </span> {{/if}} </span> {{/if}}You'll also need to add CSS for
:focusstate and potentially JavaScript for keyboard interaction (Enter/Space to toggle popup).ghost/admin/app/styles/layouts/editor.css (3)
425-431: Add:focusstyles for keyboard accessibility.The trigger element has a
pointercursor and hover states, but lacks focus styles for keyboard navigation. This makes it inaccessible to keyboard users.Add focus styles after the trigger definition:
.gh-editor-email-size-warning { position: relative; display: flex; align-items: center; margin-right: 8px; cursor: pointer; } + +.gh-editor-email-size-warning:focus { + outline: 2px solid var(--blue); + outline-offset: 2px; + border-radius: 2px; +}
438-451: Consider mobile viewport constraints for the popup.The popup has a fixed width of 200px and is positioned absolutely with
right: 0. On smaller screens or when the trigger is near the left edge, the popup could overflow the viewport or be cut off.Consider adding a responsive adjustment or using a max-width with proper positioning:
.gh-editor-email-size-popup { position: absolute; bottom: calc(100% + 8px); right: 0; display: none; flex-direction: column; gap: 4px; - width: 200px; + width: 200px; + max-width: calc(100vw - 32px); padding: 12px; background: var(--white); border-radius: 4px; box-shadow: 0 0 1px rgba(0, 0, 0, .32), 0 8px 16px -4px rgba(0, 0, 0, .3); z-index: 1000; }Alternatively, you could add a media query to adjust positioning on mobile if needed.
453-455: Consider adding a transition for smoother popup appearance.The popup appears instantly on hover, which can feel abrupt. A subtle fade-in transition would improve the user experience.
Add a transition to the popup:
.gh-editor-email-size-popup { position: absolute; bottom: calc(100% + 8px); right: 0; display: none; flex-direction: column; gap: 4px; width: 200px; padding: 12px; background: var(--white); border-radius: 4px; box-shadow: 0 0 1px rgba(0, 0, 0, .32), 0 8px 16px -4px rgba(0, 0, 0, .3); z-index: 1000; + opacity: 0; + transition: opacity 0.2s ease-in-out; + pointer-events: none; } .gh-editor-email-size-warning:hover .gh-editor-email-size-popup { display: flex; + opacity: 1; + pointer-events: auto; }Note: You'll need to change the show/hide mechanism from
display: none/flexto usingopacityandpointer-eventsfor the transition to work.ghost/admin/app/components/editor/email-size-warning.js (2)
43-48: Potential race condition if component unmounts before task completes.The constructor immediately starts the async task without checking if the component is still mounted when the task completes. While ember-concurrency typically handles task cancellation on component destruction, this could lead to issues if the tracked properties are updated after the component is destroyed.
Consider adding a check or using the component's lifecycle more explicitly:
constructor() { super(...arguments); - if (this.isEnabled) { - this.checkEmailSizeTask.perform(); - } + // Defer task execution to avoid potential race conditions + if (this.isEnabled) { + // Use next tick to ensure component is fully initialized + Promise.resolve().then(() => { + if (!this.isDestroying && !this.isDestroyed) { + this.checkEmailSizeTask.perform(); + } + }); + } }Alternatively, move the initial check to a modifier in the template, but the current approach is acceptable if ember-concurrency's built-in cancellation is reliable.
103-103: Regex may not handle all HTML edge cases.The regex
/href="([^"]+)"/gassumes well-formed HTML with double-quoted href attributes. It won't match:
- Single-quoted hrefs:
href='...'- Unquoted hrefs (valid in HTML5):
href=...- Attributes with spaces around the equals sign
While this might be acceptable if the email preview HTML is always well-formed, it could lead to inaccurate size estimates.
Consider using a more robust pattern or documenting this assumption:
- // We use a simple regex to extract href values - this matches the approach used - // in the backend link-replacer.js + // We use a simple regex to extract href values with double quotes. + // This matches the approach used in the backend link-replacer.js and is sufficient + // because email preview HTML is always well-formed with double-quoted attributes. const hrefRegex = /href="([^"]+)"/g;If you need to handle other cases:
// More comprehensive pattern (use carefully as it's more complex) const hrefRegex = /href=["']?([^"'\s>]+)["']?/gi;
📜 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 (7)
apps/admin-x-settings/src/components/settings/advanced/labs/private-features.tsx(1 hunks)ghost/admin/app/components/editor/email-size-warning.hbs(1 hunks)ghost/admin/app/components/editor/email-size-warning.js(1 hunks)ghost/admin/app/services/feature.js(2 hunks)ghost/admin/app/styles/layouts/editor.css(1 hunks)ghost/admin/app/templates/lexical-editor.hbs(2 hunks)ghost/core/core/shared/labs.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- ghost/admin/app/services/feature.js
- apps/admin-x-settings/src/components/settings/advanced/labs/private-features.tsx
- ghost/core/core/shared/labs.js
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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.
Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 23824
File: ghost/core/core/server/services/email-service/email-templates/partials/styles.hbs:919-926
Timestamp: 2025-06-13T11:57:58.226Z
Learning: In `ghost/core/core/server/services/email-service/email-templates/partials/styles.hbs`, some style blocks (e.g., `.latest-post p` and `.latest-post p a`) still use the legacy colour `#73818c` on purpose; they are later overridden by `emailCustomization` feature rules, as noted by inline TODO comments. These occurrences should not be flagged as inconsistencies.
📚 Learning: 2025-06-13T11:57:58.226Z
Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 23824
File: ghost/core/core/server/services/email-service/email-templates/partials/styles.hbs:919-926
Timestamp: 2025-06-13T11:57:58.226Z
Learning: In `ghost/core/core/server/services/email-service/email-templates/partials/styles.hbs`, some style blocks (e.g., `.latest-post p` and `.latest-post p a`) still use the legacy colour `#73818c` on purpose; they are later overridden by `emailCustomization` feature rules, as noted by inline TODO comments. These occurrences should not be flagged as inconsistencies.
Applied to files:
ghost/admin/app/templates/lexical-editor.hbsghost/admin/app/components/editor/email-size-warning.hbsghost/admin/app/components/editor/email-size-warning.jsghost/admin/app/styles/layouts/editor.css
📚 Learning: 2025-06-10T11:07:10.800Z
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.
Applied to files:
ghost/admin/app/components/editor/email-size-warning.hbsghost/admin/app/components/editor/email-size-warning.js
📚 Learning: 2025-08-14T20:54:20.921Z
Learnt from: troyciesco
Repo: TryGhost/Ghost PR: 24665
File: ghost/core/core/server/services/koenig/node-renderers/header-v2-renderer.js:113-119
Timestamp: 2025-08-14T20:54:20.921Z
Learning: In Ghost's header card email templates, .kg-header-card-content has 40px padding applied via CSS. When using Outlook VML textbox inset for background images, the inset values must match this padding (approximately 30pt,30pt,30pt,30pt) to maintain consistent content positioning across email clients.
Applied to files:
ghost/admin/app/styles/layouts/editor.css
⏰ 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). (10)
- GitHub Check: Legacy tests (Node 22.18.0, sqlite3)
- GitHub Check: Admin-X Settings tests
- GitHub Check: Acceptance tests (Node 22.18.0, mysql8)
- GitHub Check: Acceptance tests (Node 22.18.0, sqlite3)
- GitHub Check: Unit tests (Node 22.18.0)
- GitHub Check: Legacy tests (Node 22.18.0, mysql8)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Admin tests - Chrome
- GitHub Check: Lint
- GitHub Check: Build & Push Docker Image
🔇 Additional comments (4)
ghost/admin/app/components/editor/email-size-warning.js (4)
30-37: LGTM! Well-designed enablement guard.The
isEnabledgetter appropriately checks multiple conditions:
- Feature flag is enabled
- Newsletters are not disabled globally
- Post exists and hasn't been sent yet
- Post is not new/unsaved
This ensures the warning only shows when it's relevant and actionable.
110-120: LGTM! Comprehensive URL filtering logic.The filtering correctly excludes:
- Handlebars variables:
%%{...}%%- Fragment-only links:
#- Non-HTTP/relative URLs
This should match the backend's link replacement behavior and prevent false positives in the size calculation.
150-167: Email size calculation logic is well-implemented.The task correctly:
- Uses
Blobto calculate byte size (matches how emails are measured)- Applies the link rewriting adjustment to account for URL tracking
- Uses
Math.max(0, ...)to prevent negative sizes- Rounds to KB for display
- Sets appropriate warning levels based on thresholds
The yellow/red threshold values (85KB/95KB) provide good early warning before Gmail's ~102KB clipping threshold.
168-171: Error handling appropriately balances visibility and user experience.Silent failure with console logging is appropriate here - email size warnings are a nice-to-have feature and shouldn't disrupt editing if the API call fails. The console.error provides debugging information without an intrusive error modal.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ghost/admin/app/styles/layouts/editor.css (1)
456-463: Clarify the necessity of the!importantdeclaration on SVG stroke.Line 462 uses
stroke: currentColor !important;to override upstream styles. This suggests potential CSS specificity conflicts.Verify that the
!importantis necessary. If upstream styles are too specific, consider refactoring those selectors instead of using!importanthere.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ghost/admin/app/components/editor/email-size-warning.hbs(1 hunks)ghost/admin/app/styles/layouts/editor.css(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/admin/app/components/editor/email-size-warning.hbs
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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.
Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 23824
File: ghost/core/core/server/services/email-service/email-templates/partials/styles.hbs:919-926
Timestamp: 2025-06-13T11:57:58.226Z
Learning: In `ghost/core/core/server/services/email-service/email-templates/partials/styles.hbs`, some style blocks (e.g., `.latest-post p` and `.latest-post p a`) still use the legacy colour `#73818c` on purpose; they are later overridden by `emailCustomization` feature rules, as noted by inline TODO comments. These occurrences should not be flagged as inconsistencies.
📚 Learning: 2025-06-13T11:57:58.226Z
Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 23824
File: ghost/core/core/server/services/email-service/email-templates/partials/styles.hbs:919-926
Timestamp: 2025-06-13T11:57:58.226Z
Learning: In `ghost/core/core/server/services/email-service/email-templates/partials/styles.hbs`, some style blocks (e.g., `.latest-post p` and `.latest-post p a`) still use the legacy colour `#73818c` on purpose; they are later overridden by `emailCustomization` feature rules, as noted by inline TODO comments. These occurrences should not be flagged as inconsistencies.
Applied to files:
ghost/admin/app/styles/layouts/editor.css
📚 Learning: 2025-08-14T20:54:20.921Z
Learnt from: troyciesco
Repo: TryGhost/Ghost PR: 24665
File: ghost/core/core/server/services/koenig/node-renderers/header-v2-renderer.js:113-119
Timestamp: 2025-08-14T20:54:20.921Z
Learning: In Ghost's header card email templates, .kg-header-card-content has 40px padding applied via CSS. When using Outlook VML textbox inset for background images, the inset values must match this padding (approximately 30pt,30pt,30pt,30pt) to maintain consistent content positioning across email clients.
Applied to files:
ghost/admin/app/styles/layouts/editor.css
🔇 Additional comments (1)
ghost/admin/app/styles/layouts/editor.css (1)
425-516: CSS styling for email size warning looks well-structured.The overall implementation is clean:
- Flexbox layout is appropriate for the container and trigger positioning
- Smooth transitions on
max-width,opacity, andmargin-leftfor visibility toggle (lines 439)- Data attribute selector pattern (
data-warning-active="true") is idiomatic for conditional styling- Popup fade-in animation is performant and well-defined
- Typography and spacing for popup content are consistent with the design system
The CSS integrates well with the feature gating via the
emailSizeWarningsflag mentioned in the PR summary.
| .gh-editor-email-size-warning { | ||
| display: flex; | ||
| align-items: center; | ||
| margin-left: 0; | ||
| cursor: pointer; | ||
| max-width: 0; | ||
| overflow: hidden; | ||
| opacity: 0; | ||
| transition: max-width 0.3s ease, opacity 0.3s ease, margin-left 0.3s ease; | ||
| } | ||
|
|
||
| .gh-editor-email-size-warning[data-warning-active="true"] { | ||
| max-width: 40px; | ||
| opacity: 1; | ||
| margin-left: 8px; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f \( -name "*.ts" -o -name "*.tsx" -o -name "*.js" \) | xargs rg -l "emailSizeWarning|email-size-warning" | head -20Repository: TryGhost/Ghost
Length of output: 269
🏁 Script executed:
cat -n ./ghost/admin/app/components/editor/email-size-warning.jsRepository: TryGhost/Ghost
Length of output: 7424
🏁 Script executed:
find . -path "*/editor/*" -name "*email-size-warning*" -type fRepository: TryGhost/Ghost
Length of output: 174
🏁 Script executed:
cat -n ./ghost/admin/app/components/editor/email-size-warning.hbsRepository: TryGhost/Ghost
Length of output: 1368
🏁 Script executed:
sed -n '431,520p' ./ghost/admin/app/styles/layouts/editor.cssRepository: TryGhost/Ghost
Length of output: 1937
Add keyboard focus and touch handlers to make the popup accessible.
The popup visibility relies on CSS hover state (.gh-editor-email-size-warning:hover ~ .gh-editor-email-size-popup), which excludes keyboard users (Tab navigation) and touch users (no click handler on mobile).
The accompanying JavaScript component must handle:
focus/blurstates to show/hide the popup on keyboard navigationclick/touchstarthandlers for mobile devices- Proper ARIA attributes (
aria-expanded,role="button",aria-label) on the trigger element
Without these, keyboard and touch users cannot access the warning information.
🤖 Prompt for AI Agents
In ghost/admin/app/styles/layouts/editor.css around lines 431 to 446, the
email-size-warning currently relies on CSS hover which prevents keyboard and
touch users from accessing the popup; update the corresponding JS component to:
add focus and blur event handlers that set/unset the same data-warning-active
attribute or class used by CSS so Tab/Shift+Tab reveals/hides the popup; add
click and touchstart handlers that toggle the popup on touch devices (prevent
duplicate events); and add proper ARIA attributes to the trigger element
(role="button", aria-expanded reflecting the active state, and a descriptive
aria-label) so screen readers and keyboard users can discover and control the
popup. Ensure event handlers clean up on unmount to avoid leaks.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ghost/admin/app/services/email-size-warning.js (1)
41-53: Consider limiting deduplication to in‑flight task instances to allow retries for the same post version
fetchEmailSizealways reusesthis._fetchTask.lastwheneverpostIdandupdatedAtmatch, even if the previous task has already finished (including transient network/API failures). That means a failed or stale result for a given post version is effectively cached forever and a user‑initiated "check again" cannot trigger a fresh request untilupdatedAtUTCchanges.If you only want to dedupe concurrent requests while still allowing retries for the same version, consider returning the last task instance only when it's still running, otherwise start a new one:
const postId = post.id; const updatedAt = post.updatedAtUTC?.toISOString?.() || post.updatedAtUTC; - // Return existing task instance if we have one for this exact version - if (this._lastPostId === postId && this._lastUpdatedAt === updatedAt && this._fetchTask.last) { - return this._fetchTask.last; - } + // Share an in-flight task instance for this exact post version + const lastInstance = this._fetchTask.last; + if ( + this._lastPostId === postId && + this._lastUpdatedAt === updatedAt && + lastInstance && + lastInstance.isRunning + ) { + return lastInstance; + } this._lastPostId = postId; this._lastUpdatedAt = updatedAt; return this._fetchTask.perform(post);This keeps the deduplication benefit for overlapping calls while still allowing retries if the previous check completed with an error or if the backend behaviour changes without a post save.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ghost/admin/app/components/editor/email-size-warning.js(1 hunks)ghost/admin/app/services/email-size-warning.js(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
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.
Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 23824
File: ghost/core/core/server/services/email-service/email-templates/partials/styles.hbs:919-926
Timestamp: 2025-06-13T11:57:58.226Z
Learning: In `ghost/core/core/server/services/email-service/email-templates/partials/styles.hbs`, some style blocks (e.g., `.latest-post p` and `.latest-post p a`) still use the legacy colour `#73818c` on purpose; they are later overridden by `emailCustomization` feature rules, as noted by inline TODO comments. These occurrences should not be flagged as inconsistencies.
📚 Learning: 2025-06-13T11:57:58.226Z
Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 23824
File: ghost/core/core/server/services/email-service/email-templates/partials/styles.hbs:919-926
Timestamp: 2025-06-13T11:57:58.226Z
Learning: In `ghost/core/core/server/services/email-service/email-templates/partials/styles.hbs`, some style blocks (e.g., `.latest-post p` and `.latest-post p a`) still use the legacy colour `#73818c` on purpose; they are later overridden by `emailCustomization` feature rules, as noted by inline TODO comments. These occurrences should not be flagged as inconsistencies.
Applied to files:
ghost/admin/app/components/editor/email-size-warning.jsghost/admin/app/services/email-size-warning.js
📚 Learning: 2025-06-10T11:07:10.800Z
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.
Applied to files:
ghost/admin/app/components/editor/email-size-warning.js
📚 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/admin/app/components/editor/email-size-warning.jsghost/admin/app/services/email-size-warning.js
📚 Learning: 2025-09-03T12:28:11.174Z
Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 24779
File: ghost/core/core/server/services/members/members-api/controllers/RouterController.js:34-51
Timestamp: 2025-09-03T12:28:11.174Z
Learning: The extractRefererOrRedirect function in Ghost's RouterController (ghost/core/core/server/services/members/members-api/controllers/RouterController.js) is working as intended according to maintainer kevinansfield. The current handling of autoRedirect, redirect parameter parsing, and referrer logic does not need the security-related changes suggested around autoRedirect coercion, relative URL handling, or same-origin checks.
Applied to files:
ghost/admin/app/components/editor/email-size-warning.js
📚 Learning: 2025-04-23T15:44:52.549Z
Learnt from: 9larsons
Repo: TryGhost/Ghost PR: 21866
File: ghost/core/core/server/web/admin/middleware/serve-auth-frame-file.js:10-19
Timestamp: 2025-04-23T15:44:52.549Z
Learning: The existing implementation in `ghost/core/core/server/web/admin/middleware/serve-auth-frame-file.js` using `path.parse(req.url).base` is secure against path traversal attacks as it properly extracts only the filename component without any directory parts.
Applied to files:
ghost/admin/app/components/editor/email-size-warning.js
🧬 Code graph analysis (1)
ghost/admin/app/services/email-size-warning.js (1)
ghost/core/test/legacy/models/model_users.test.js (1)
updatedAt(186-186)
🔇 Additional comments (2)
ghost/admin/app/services/email-size-warning.js (1)
55-145: Service design, error handling, and size calculation look soundNewsletter loading, link adjustment, and size/threshold calculation are defensive and side‑effect free (falling back to
{warningLevel: null, emailSizeKb: null}on any error or missing data), and the URL construction for/email_previews/postswith a newsletter slug is consistent and safe. Overall the service wiring is solid and matches the feature’s goals.ghost/admin/app/components/editor/email-size-warning.js (1)
7-46: Component wiring and enablement logic align well with the feature requirementsThe service injections,
isEnabledconditions (feature flag, newsletters enabled, existing unsent post), and the restartablecheckEmailSizeTaskall work together cleanly. Triggering the task from the constructor and the explicitcheckEmailSizeaction ensures the warning stays in sync with saves while keeping the UI inert when the feature or email sending is disabled.
ref https://linear.app/ghost/issue/FEA-479/ - added `emailSizeWarnings` labs flag for gating the warning component - added `<EmailSizeWarning>` component that watches a post for saves, fetches the email preview for the post, and calculates whether the email size will be in danger of hitting the clipping threshold for populate email clients like Gmail - used in the editor footer next to the word count - invisible until a threshold is met - disabled for new+unsaved posts, already sent posts, or when newsletters have been disabled # Conflicts: # apps/admin-x-settings/src/components/settings/advanced/labs/private-features.tsx # ghost/core/core/shared/labs.js
kevinansfield
force-pushed
the
email-size-warning
branch
from
3899580 to
8867ad5
Compare
ref https://linear.app/ghost/issue/FEA-479/
emailSizeWarningslabs flag for gating the warning componentemailSizeWarningservice that fetches a post email preview and calculates the email size<EmailSizeWarning>component that usesemailSizeWarningservice to fetch email size and re-fetch on save