Added size warning to email preview #25717
Conversation
WalkthroughAdds a presentational email-size warning UI and wires it into the editor preview and publish-flow options. Introduces a preview clip template and styles, a reusable EmailSizeWarning template with block and non-block rendering, and template insertions in preview and publish options. Refactors the EmailSizeWarning component to expose Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
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: 1
🧹 Nitpick comments (1)
ghost/admin/app/styles/layouts/preview-email.css (1)
287-321: Check iframe height calculation after inserting the new clip containerThe new
.gh-email-preview-clip-containeradds fixed-height content above the iframe, but the iframe still usesheight: calc(100% - 67px)based on the old layout. That combination (plusoverflow: hiddenon the mockup) risks cropping the bottom of the email preview, especially on smaller viewports.Consider either:
- converting the mockup content into a flex column and letting the iframe take
flex: 1 1 autoinstead of relying on magic numbers, or- updating the height calculation to account for the new banner's height and QA’ing across viewport sizes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ghost/admin/app/components/editor/modals/preview/email.hbs(1 hunks)ghost/admin/app/styles/layouts/preview-email.css(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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/preview-email.cssghost/admin/app/components/editor/modals/preview/email.hbs
📚 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/preview-email.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). (3)
- GitHub Check: Admin tests - Chrome
- GitHub Check: Unit tests (Node 22.18.0)
- GitHub Check: Build & Push Docker Image
kevinansfield
force-pushed
the
email-size-warning-preview
branch
2 times, most recently
from
4ca47c1 to
d847740
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ghost/admin/app/styles/layouts/editor.css (1)
425-517: Editor styles for email size warning integrate safely with existing layoutThe new container, icon, and popup styles line up with the template’s
data-warning-activeattribute and DOM structure, keeping the default state invisible and only expanding when a warning is present. As a future enhancement, you might consider exposing the popup via focus/keyboard as well as hover for better accessibility, but the current behavior is consistent with existing editor affordances.ghost/admin/app/services/email-size-warning.js (1)
55-65: Newsletter is cached indefinitely.The newsletter is cached for the lifetime of the service (entire session). If the active newsletter changes during the session, the cached value becomes stale. This is likely acceptable for typical use, but consider adding cache invalidation if this causes issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
ghost/admin/public/assets/icons/email-warning.svgis excluded by!**/*.svgghost/core/test/e2e-api/admin/__snapshots__/config.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (10)
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/components/editor/modals/preview/email.hbs(1 hunks)ghost/admin/app/services/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/styles/layouts/preview-email.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 (1)
- ghost/admin/app/styles/layouts/preview-email.css
🧰 Additional context used
🧠 Learnings (8)
📓 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.
📚 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/services/feature.jsghost/core/core/shared/labs.jsapps/admin-x-settings/src/components/settings/advanced/labs/private-features.tsxghost/admin/app/components/editor/modals/preview/email.hbs
📚 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
📚 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:
ghost/core/core/shared/labs.jsapps/admin-x-settings/src/components/settings/advanced/labs/private-features.tsx
📚 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.hbsghost/admin/app/components/editor/email-size-warning.jsghost/admin/app/templates/lexical-editor.hbsghost/admin/app/services/email-size-warning.jsghost/admin/app/styles/layouts/editor.cssghost/admin/app/components/editor/modals/preview/email.hbs
📚 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-10-06T14:19:50.385Z
Learnt from: sam-lord
Repo: TryGhost/Ghost PR: 24966
File: ghost/core/test/unit/server/services/mail/GhostMailer.test.js:330-421
Timestamp: 2025-10-06T14:19:50.385Z
Learning: The GhostMailer class in `ghost/core/core/server/services/mail/GhostMailer.js` does not accept external Mailgun-specific parameters (like `o:tag`) from callers. Tags are generated internally via the `getTags()` method. This design keeps the class generic and avoids mixing transport-specific concerns into the public interface.
Applied to files:
ghost/admin/app/services/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
🧬 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 (12)
ghost/core/core/shared/labs.js (1)
57-59: Private labs flag registration foremailSizeWarningsis consistentThe new
emailSizeWarningsentry sits correctly inPRIVATE_FEATURESand will be exposed via bothgetAllFlagsandWRITABLE_KEYS_ALLOWLIST. No further changes needed here.ghost/admin/app/services/feature.js (1)
82-99: Feature service exposure ofemailSizeWarningsmatches existing patternRegistering
emailSizeWarningsvia@feature('emailSizeWarnings')aligns with the other labs flags and makes the flag available to consuming components. Looks good as-is.ghost/admin/app/templates/lexical-editor.hbs (1)
105-111: Editor integration points forEditor::EmailSizeWarninglook correctPassing
@post={{this.post}}in both the desktop and mobile wordcount areas follows the component’s API and provides consistent exposure of the warning across layouts without disturbing existing structure.Also applies to: 122-125
apps/admin-x-settings/src/components/settings/advanced/labs/private-features.tsx (1)
51-66: Private labs listing for “Email Size Warnings” is wired correctlyThe new entry’s
flag: 'emailSizeWarnings'matches the backend and feature service keys, and the description accurately reflects the behavior. No additional wiring changes needed here.ghost/admin/app/components/editor/modals/preview/email.hbs (1)
91-102: Preview clip banner now correctly driven by shared email-size stateUsing the block form of
Editor::EmailSizeWarningwithwarningLevelandemailSizeKbensures the banner only appears when a warning is needed and shows the real computed size, keeping preview behavior aligned with the editor clipping indicator.ghost/admin/app/components/editor/email-size-warning.hbs (1)
1-24: EmailSizeWarning template cleanly supports both inline and block usageThe
isEnabledguard,did-updatehook on@post.updatedAtUTC, and the yieldedwarningLevel/emailSizeKbgive a flexible API for both preview and editor placements, while the default UI stays minimal until a warning is active. This looks solid.ghost/admin/app/components/editor/email-size-warning.js (1)
1-47: Component logic for fetching and gating email size warnings is well-structuredThe enablement conditions, initial fetch in the constructor, and restartable task give you a clean, race-free way to populate
warningLevelandemailSizeKbonly when the feature is relevant. Please just double-check that excluding posts with an existingpost.emailand sites witheditorDefaultEmailRecipients === 'disabled'matches the intended product behavior for resend flows and non-default-email setups.ghost/admin/app/services/email-size-warning.js (5)
1-11: Well-documented constants and proper imports.The threshold constants and URL path length are clearly documented with their expected values and format.
17-27: Standard Ember service pattern with appropriate state tracking.The service injections and state variables for cache management are correctly implemented.
36-53: Solid deduplication logic for concurrent requests.The implementation correctly prevents redundant API calls when multiple components request data for the same post version. The handling of
updatedAtUTCas either a Date or string is a good defensive approach.
67-105: Well-implemented link rewriting size estimation.The method correctly isolates post content using markers and estimates the size impact of URL rewriting. The filtering logic properly excludes placeholders, hash links, and non-rewritable URLs.
107-145: Robust implementation with proper error handling.Good use of
Blobfor accurate UTF-8 byte size measurement andMath.max(0, ...)for defensive handling of edge cases. The graceful degradation on errors returning null values is appropriate for a non-critical warning feature.
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-8: Consider the class name whenwarningLevelis empty.When
warningLevelis falsy, the class on line 8 will begh-editor-email-size-warning--with a trailing double-dash. If this is unintentional, consider using a conditional to only append the modifier whenwarningLevelis present.Example fix:
However, if the trailing double-dash is intentional for styling purposes (e.g., as a base state), then the current implementation is fine.
ghost/admin/app/styles/layouts/preview-email.css (1)
315-315: Consider browser compatibility fortext-wrap: pretty.The
text-wrap: prettyproperty is supported in Chrome 117+, Edge 117+, Safari 26+, and most mobile browsers (~76-78% global support), but Firefox does not support it. The property gracefully degrades in unsupported browsers. For an admin preview panel, this is typically acceptable; however, if Firefox compatibility for admin users is important, consider adding a fallback or removing this property.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ghost/admin/app/components/editor/email-size-warning.hbs(1 hunks)ghost/admin/app/components/editor/modals/preview/email.hbs(1 hunks)ghost/admin/app/styles/layouts/preview-email.css(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/admin/app/components/editor/modals/preview/email.hbs
🧰 Additional context used
🧠 Learnings (2)
📚 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/preview-email.cssghost/admin/app/components/editor/email-size-warning.hbs
📚 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/preview-email.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). (4)
- GitHub Check: Admin tests - Chrome
- GitHub Check: Unit tests (Node 22.18.0)
- GitHub Check: Lint
- GitHub Check: Build & Push Docker Image
🔇 Additional comments (2)
ghost/admin/app/components/editor/email-size-warning.hbs (2)
2-5: LGTM! Block-style support enables flexible rendering.The
has-blockcheck allows the component to be used both as a block (yielding context) and inline (original behavior), which enables the email preview modal to render a custom warning UI while maintaining backward compatibility for existing usages.
15-21: LGTM! Popup content is properly structured.The popup content is now conditionally rendered inside the warning container and provides clear guidance about email size limits. The structure and messaging are appropriate for alerting users about potential email clipping.
peterzimon
force-pushed
the
email-size-warning-preview
branch
from
d847740 to
459a02d
Compare
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 (4)
ghost/admin/app/components/editor/email-size-warning.hbs(1 hunks)ghost/admin/app/components/editor/modals/preview/email.hbs(1 hunks)ghost/admin/app/styles/layouts/editor.css(1 hunks)ghost/admin/app/styles/layouts/preview-email.css(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- ghost/admin/app/styles/layouts/preview-email.css
- ghost/admin/app/components/editor/modals/preview/email.hbs
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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/admin/app/components/editor/email-size-warning.hbs
🔇 Additional comments (3)
ghost/admin/app/styles/layouts/editor.css (1)
461-463: LGTM!The fixed yellow stroke color ensures consistent warning icon styling, aligning with the PR's goal of making email size warnings more visible.
ghost/admin/app/components/editor/email-size-warning.hbs (2)
2-5: LGTM on block mode support.Clean pattern for yielding
warningLevelandemailSizeKbto consumer components like the email preview modal.
17-17: No action needed—theyellowutility class is properly defined.The class exists in
ghost/admin/app/styles/spirit/_skins.cssas part of Ghost's design system, with the CSS variable--yellowdefined inghost/admin/app/styles/patterns/global.css. The span will be styled correctly.
| <span {{did-update this.checkEmailSize @post.updatedAtUTC}} class="gh-editor-email-size-warning-container"> | ||
| <span class="gh-editor-email-size-warning gh-editor-email-size-warning--{{this.warningLevel}}" data-warning-active={{if this.warningLevel "true" "false"}}> | ||
| {{#if this.warningLevel}} | ||
| {{svg-jar "email-warning"}} | ||
| </span> | ||
| {{/if}} | ||
| </span> | ||
| {{#if this.warningLevel}} | ||
| <div class="gh-editor-email-size-popup"> | ||
| <div class="gh-editor-email-size-popup-title">Looks like this is a long post</div> | ||
| <div class="gh-editor-email-size-popup-text">Email newsletters may get clipped in the inbox behind a "View entire message" link when they're over 100kB.</div> | ||
| <div class="gh-editor-email-size-popup-used">You've used: <span class="yellow">{{this.emailSizeKb}}kB</span></div> | ||
| </div> | ||
| {{/if}} |
There was a problem hiding this comment.
Consider keyboard accessibility for the warning popup.
The popup is triggered only via CSS :hover. Keyboard users cannot access the warning details since the warning icon isn't focusable. Consider adding tabindex="0" and a :focus-within selector, or wrapping the icon in a button element.
- <span class="gh-editor-email-size-warning gh-editor-email-size-warning--{{this.warningLevel}}" data-warning-active={{if this.warningLevel "true" "false"}}>
+ <span class="gh-editor-email-size-warning gh-editor-email-size-warning--{{this.warningLevel}}" data-warning-active={{if this.warningLevel "true" "false"}} tabindex={{if this.warningLevel "0"}}>Also add a :focus-within rule in the CSS to show the popup on focus.
📝 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.
| <span {{did-update this.checkEmailSize @post.updatedAtUTC}} class="gh-editor-email-size-warning-container"> | |
| <span class="gh-editor-email-size-warning gh-editor-email-size-warning--{{this.warningLevel}}" data-warning-active={{if this.warningLevel "true" "false"}}> | |
| {{#if this.warningLevel}} | |
| {{svg-jar "email-warning"}} | |
| </span> | |
| {{/if}} | |
| </span> | |
| {{#if this.warningLevel}} | |
| <div class="gh-editor-email-size-popup"> | |
| <div class="gh-editor-email-size-popup-title">Looks like this is a long post</div> | |
| <div class="gh-editor-email-size-popup-text">Email newsletters may get clipped in the inbox behind a "View entire message" link when they're over 100kB.</div> | |
| <div class="gh-editor-email-size-popup-used">You've used: <span class="yellow">{{this.emailSizeKb}}kB</span></div> | |
| </div> | |
| {{/if}} | |
| <span {{did-update this.checkEmailSize @post.updatedAtUTC}} class="gh-editor-email-size-warning-container"> | |
| <span class="gh-editor-email-size-warning gh-editor-email-size-warning--{{this.warningLevel}}" data-warning-active={{if this.warningLevel "true" "false"}} tabindex={{if this.warningLevel "0"}}> | |
| {{#if this.warningLevel}} | |
| {{svg-jar "email-warning"}} | |
| {{/if}} | |
| </span> | |
| {{#if this.warningLevel}} | |
| <div class="gh-editor-email-size-popup"> | |
| <div class="gh-editor-email-size-popup-title">Looks like this is a long post</div> | |
| <div class="gh-editor-email-size-popup-text">Email newsletters may get clipped in the inbox behind a "View entire message" link when they're over 100kB.</div> | |
| <div class="gh-editor-email-size-popup-used">You've used: <span class="yellow">{{this.emailSizeKb}}kB</span></div> | |
| </div> | |
| {{/if}} |
🤖 Prompt for AI Agents
In ghost/admin/app/components/editor/email-size-warning.hbs around lines 7 to
19, the warning popup is only revealed on CSS :hover and the icon is not
keyboard focusable; make the icon focusable and ensure the popup appears on
keyboard focus: wrap the icon span in a semantic button (or add tabindex="0" to
the existing container), add appropriate aria attributes (aria-expanded and
aria-haspopup) on the button, move the popup markup so it is a
focusable/focus-within child of that button/container, and update the CSS to
include a :focus-within rule that reveals the popup the same way as :hover.
Ensure focus styling remains visible and that keyboard activation toggles/keeps
the popup visible as expected.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ghost/admin/app/styles/components/publishmenu.css (1)
451-458: Optimize transition and clarify "open" state behavior.Two recommendations for this animation:
- Specify transition properties: Using
transition: all 0.3s easecan impact performance. Specify only the properties that animate:.gh-publish-setting-email-warning { - transition: all 0.3s ease; + transition: transform 0.3s ease, margin-bottom 0.3s ease; }
- Clarify the "open" state logic: The warning moves up 12px when the "publishType" section is expanded. This coupling seems questionable—why should the warning's position depend on whether the publish type section is open? Consider whether the warning should be independently positioned or if this interaction is intentional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ghost/admin/app/components/editor/modals/publish-flow/options.hbs(1 hunks)ghost/admin/app/styles/components/publishmenu.css(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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/components/publishmenu.cssghost/admin/app/components/editor/modals/publish-flow/options.hbs
📚 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/components/publishmenu.cssghost/admin/app/components/editor/modals/publish-flow/options.hbs
📚 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/admin/app/styles/components/publishmenu.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). (3)
- GitHub Check: Unit tests (Node 22.18.0)
- GitHub Check: Admin tests - Chrome
- GitHub Check: Build & Push Docker Image
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
ghost/admin/app/components/editor/email-size-warning.hbs (1)
7-19: Keyboard accessibility issue remains unaddressed.As noted in previous review comments, the popup is only triggered via CSS
:hover, making it inaccessible to keyboard users. The warning icon should be focusable (e.g., viatabindex="0"or wrapping in a button element), and the CSS should include:focus-withinrules to show the popup on keyboard focus.
🧹 Nitpick comments (2)
ghost/admin/app/components/editor/email-size-warning.hbs (1)
8-8: Boolean value directly interpolated into CSS class name.Line 8 interpolates
this.overLimit(a boolean) directly into the class name, resulting ingh-editor-email-size-warning--trueorgh-editor-email-size-warning--false. While this may work if the CSS is written to expect these exact strings, it's unconventional and could be confusing.Consider using a helper or computed property to map the boolean to a more semantic class name:
-<span class="gh-editor-email-size-warning gh-editor-email-size-warning--{{this.overLimit}}" data-warning-active={{if this.overLimit "true" "false"}}> +<span class="gh-editor-email-size-warning {{if this.overLimit "gh-editor-email-size-warning--active"}}" data-warning-active={{if this.overLimit "true" "false"}}>ghost/admin/app/services/email-size-warning.js (1)
33-37: Consider returningfalseinstead ofnullfor invalid posts.Line 37 returns
{overLimit: null, emailSizeKb: null}for invalid posts, but the component initializesoverLimittofalse. While the component'sisEnabledcheck likely prevents this code path from executing in practice, returningfalsewould be more consistent with the component's type expectations and the service's other return paths (lines 110, 141).-return Promise.resolve({overLimit: null, emailSizeKb: null}); +return Promise.resolve({overLimit: false, emailSizeKb: null});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
ghost/admin/app/components/editor/email-size-warning.hbs(1 hunks)ghost/admin/app/components/editor/email-size-warning.js(2 hunks)ghost/admin/app/components/editor/modals/preview/email.hbs(1 hunks)ghost/admin/app/components/editor/modals/publish-flow/options.hbs(1 hunks)ghost/admin/app/services/email-size-warning.js(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/admin/app/components/editor/modals/preview/email.hbs
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
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.
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-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/modals/publish-flow/options.hbsghost/admin/app/components/editor/email-size-warning.hbsghost/admin/app/components/editor/email-size-warning.jsghost/admin/app/services/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/modals/publish-flow/options.hbsghost/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/modals/publish-flow/options.hbsghost/admin/app/components/editor/email-size-warning.hbs
📚 Learning: 2025-11-06T05:35:41.162Z
Learnt from: danielraffel
Repo: TryGhost/Ghost PR: 25366
File: apps/admin/src/layout/app-sidebar/NavHeader.tsx:13-23
Timestamp: 2025-11-06T05:35:41.162Z
Learning: In apps/admin/src/layout/app-sidebar/NavHeader.tsx, the React component dispatches a synthetic KeyboardEvent to trigger the Ember keymaster.js search modal shortcut. This approach is known to have cross-browser reliability issues but was deferred for architectural refactoring in a separate PR. The recommended fix is to expose a global function or custom DOM event from the Ember app instead of relying on synthetic keyboard events with keymaster.js.
Applied to files:
ghost/admin/app/components/editor/email-size-warning.hbs
⏰ 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). (4)
- GitHub Check: Unit tests (Node 22.18.0)
- GitHub Check: Admin tests - Chrome
- GitHub Check: Lint
- GitHub Check: Build & Push Docker Image
🔇 Additional comments (9)
ghost/admin/app/components/editor/modals/publish-flow/options.hbs (1)
41-50: LGTM! Email size warning correctly integrated.The implementation properly uses the
EmailSizeWarningcomponent to dynamically calculate and display the email size. The conditional rendering based onoverLimitensures the warning only appears when the post exceeds 100kB, and the dynamic{{emailSizeKb}}value correctly replaces the previously hardcoded size.ghost/admin/app/components/editor/email-size-warning.hbs (1)
2-5: LGTM! Block rendering path correctly implemented.The block-form rendering uses
has-blockto detect when a block is provided and correctly yieldsoverLimitandemailSizeKbvalues. Thedid-updatemodifier ensures the check runs when the post is updated.ghost/admin/app/components/editor/email-size-warning.js (4)
12-13: LGTM! Property renamed to better reflect boolean nature.The rename from
warningLeveltooverLimitand initialization tofalsemakes the component state clearer and aligns with the simplified service API.
15-21: LGTM! Simplified post access.Directly accessing
this.args.postinstead of using a getter is cleaner and the conditions correctly check for valid post state.
23-28: Good improvement: Auto-check on construction.Adding the constructor to automatically check email size when the component is enabled improves the user experience by eliminating the delay before the warning appears.
37-42: LGTM! Task correctly uses updated API.The task properly fetches email size using
this.args.postand correctly maps the newresult.overLimitboolean to the tracked property.ghost/admin/app/services/email-size-warning.js (3)
5-5: LGTM! Simplified threshold constant.Replacing separate yellow/red thresholds with a single
LIMIT_THRESHOLDaligns with the feature's simplified binary warning approach (over limit or not).
106-111: LGTM! Appropriate fallback for missing newsletter.Returning
{overLimit: false, emailSizeKb: null}when no newsletter is available is correct, as no email will be sent in this case.
129-142: LGTM! Threshold logic correctly implemented.The calculation properly determines when the email size exceeds 100kB and returns the boolean
overLimitvalue. The error fallback returning{overLimit: false, emailSizeKb: null}is appropriate and consistent.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
ghost/admin/app/components/editor/email-size-warning.hbs (1)
7-19: Keyboard accessibility issue noted in previous review.The warning icon and popup remain inaccessible to keyboard users as flagged in the previous review. Consider addressing the
:hover-only interaction and missing focus management.
🧹 Nitpick comments (1)
ghost/admin/app/components/editor/email-size-warning.hbs (1)
8-8: Consider using a semantic modifier class instead of the boolean value.The template uses
{{this.overLimit}}directly in the class name, which outputsgh-editor-email-size-warning--trueorgh-editor-email-size-warning--false. BEM conventions typically favor semantic modifier names like--activeor conditionally adding the modifier only when true.Consider one of these approaches:
-<span class="gh-editor-email-size-warning gh-editor-email-size-warning--{{this.overLimit}}" data-warning-active={{if this.overLimit "true" "false"}}> +<span class="gh-editor-email-size-warning {{if this.overLimit 'gh-editor-email-size-warning--active'}}" data-warning-active={{if this.overLimit "true" "false"}}>Or if you prefer to keep the modifier always present with a semantic name:
-<span class="gh-editor-email-size-warning gh-editor-email-size-warning--{{this.overLimit}}" data-warning-active={{if this.overLimit "true" "false"}}> +<span class="gh-editor-email-size-warning gh-editor-email-size-warning--{{if this.overLimit 'active' 'inactive'}}" data-warning-active={{if this.overLimit "true" "false"}}>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ghost/admin/app/components/editor/email-size-warning.hbs(1 hunks)ghost/admin/app/components/editor/modals/preview/email.hbs(1 hunks)ghost/admin/app/components/editor/modals/publish-flow/options.hbs(1 hunks)ghost/admin/app/styles/layouts/preview-email.css(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- ghost/admin/app/components/editor/modals/publish-flow/options.hbs
- ghost/admin/app/components/editor/modals/preview/email.hbs
- ghost/admin/app/styles/layouts/preview-email.css
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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.hbs
📚 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.hbs
📚 Learning: 2025-11-06T05:35:41.162Z
Learnt from: danielraffel
Repo: TryGhost/Ghost PR: 25366
File: apps/admin/src/layout/app-sidebar/NavHeader.tsx:13-23
Timestamp: 2025-11-06T05:35:41.162Z
Learning: In apps/admin/src/layout/app-sidebar/NavHeader.tsx, the React component dispatches a synthetic KeyboardEvent to trigger the Ember keymaster.js search modal shortcut. This approach is known to have cross-browser reliability issues but was deferred for architectural refactoring in a separate PR. The recommended fix is to expose a global function or custom DOM event from the Ember app instead of relying on synthetic keyboard events with keymaster.js.
Applied to files:
ghost/admin/app/components/editor/email-size-warning.hbs
⏰ 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). (4)
- GitHub Check: Lint
- GitHub Check: Unit tests (Node 22.18.0)
- GitHub Check: Admin tests - Chrome
- GitHub Check: Build & Push Docker Image
🔇 Additional comments (1)
ghost/admin/app/components/editor/email-size-warning.hbs (1)
2-5: LGTM! Good reusability pattern.The block rendering path cleanly separates data provision from presentation. Yielding
overLimitandemailSizeKballows consumers to customize the warning UI while the component handles the size check logic.
ref https://linear.app/ghost/issue/FEA-479/clipping-meter-that-alerts-you-when-html-is-getting-too-large