Started processing automation email analytics#29333
Conversation
WalkthroughThe email analytics system now exposes separate newsletter and automation services with injected job names, cursor seeds, event processors, tags, and lifecycle events. Newsletter processing moved into a dedicated batch processor supporting sequential or batched handling and aggregation. Automation processing resolves Mailgun message IDs to recipients, records delivery/open timestamps, and increments revision open counts transactionally. Endpoints and existing tests now use the newsletter-scoped service. Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run ghost:test:ci:integration |
✅ Succeeded | 2m 52s | View ↗ |
nx run ghost:test:integration |
✅ Succeeded | 3m | View ↗ |
nx run ghost:test:legacy |
✅ Succeeded | 3m 3s | View ↗ |
nx run ghost:test:e2e |
✅ Succeeded | 2m 36s | View ↗ |
nx run ghost-monorepo:lint:boundaries |
✅ Succeeded | 20s | View ↗ |
nx run-many -t test:unit -p ghost |
✅ Succeeded | 29s | View ↗ |
nx run-many -t lint -p ghost,ghost-monorepo |
✅ Succeeded | 3s | View ↗ |
nx run @tryghost/admin:build |
✅ Succeeded | 3s | View ↗ |
nx run-many --target=build --projects=tag:publi... |
✅ Succeeded | <1s | View ↗ |
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗
☁️ Nx Cloud last updated this comment at 2026-07-16 01:05:54 UTC
We'll need this later.
We'll use this soon.
We'll use this soon.
This will let us inject automation's behavior later. This change looks huge, but most of it is moving things out of `EmailAnalyticsService` and into `NewsletterEmailAnalyticsProcessor`.
I recommend reviewing `core/server/services/email-analytics/index.ts` first, then looking at the rest.
This will make it possible to attach automations in an upcoming change.
This is similar to the previous commit. This will make it possible to attach automations in an upcoming change.
EvanHahn
force-pushed
the
2026-07-14-NY-1447
branch
from
6f3309e to
71c5ad0
Compare
![coderabbitai[bot]](https://cdn.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ghost/core/core/server/services/email-analytics/email-analytics-service.js (1)
441-458: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve
processingFailuresin the cumulative result.
processBatchmay incrementprocessingResult.processingFailures, butbatchDeltaomits it. The returned result therefore loses this count, particularly after intermediate aggregation resets the working result.Proposed fix
const beforeCounts = { opened: processingResult.opened, delivered: processingResult.delivered, temporaryFailed: processingResult.temporaryFailed, permanentFailed: processingResult.permanentFailed, unsubscribed: processingResult.unsubscribed, complained: processingResult.complained, unhandled: processingResult.unhandled, - unprocessable: processingResult.unprocessable + unprocessable: processingResult.unprocessable, + processingFailures: processingResult.processingFailures }; const batchDelta = new EventProcessingResult({ opened: processingResult.opened - beforeCounts.opened, delivered: processingResult.delivered - beforeCounts.delivered, temporaryFailed: processingResult.temporaryFailed - beforeCounts.temporaryFailed, permanentFailed: processingResult.permanentFailed - beforeCounts.permanentFailed, unsubscribed: processingResult.unsubscribed - beforeCounts.unsubscribed, complained: processingResult.complained - beforeCounts.complained, unhandled: processingResult.unhandled - beforeCounts.unhandled, unprocessable: processingResult.unprocessable - beforeCounts.unprocessable, + processingFailures: processingResult.processingFailures - beforeCounts.processingFailures, emailIds: processingResult.emailIds.filter(id => !beforeEmailIds.has(id)), memberIds: processingResult.memberIds.filter(id => !beforeMemberIds.has(id)) });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ghost/core/core/server/services/email-analytics/email-analytics-service.js` around lines 441 - 458, The batch delta built in the event-processing aggregation must include the processingFailures count from processingResult relative to beforeCounts. Update the EventProcessingResult construction in the cumulative result flow to calculate and preserve this delta, ensuring cumulativeResult.merge(batchDelta) retains failures across intermediate resets.
🧹 Nitpick comments (1)
ghost/core/core/server/services/email-analytics/event-processor.ts (1)
3-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign
EventProcessor.processBatchwithReadonlyArray<EmailAnalyticsEvent>ghost/core/core/server/services/email-analytics/event-processor.ts:4
any[]weakens the shared contract and allows mutation; use the same typed readonly array as the processor implementation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ghost/core/core/server/services/email-analytics/event-processor.ts` around lines 3 - 8, Update EventProcessor.processBatch to accept ReadonlyArray<EmailAnalyticsEvent> instead of any[], matching the processor implementation and preventing event mutation. Preserve the existing result, fetchData, and Promise<void> contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@ghost/core/test/integration/services/email-analytics/automation-email-analytics.test.js`:
- Around line 19-24: Update clearInMemoryCursors to skip null or undefined
entries returned by emailAnalytics.automations.service.getStatus() before
deleting lastEventTimestamp and lastBegin, while preserving cursor clearing for
valid status objects.
---
Outside diff comments:
In `@ghost/core/core/server/services/email-analytics/email-analytics-service.js`:
- Around line 441-458: The batch delta built in the event-processing aggregation
must include the processingFailures count from processingResult relative to
beforeCounts. Update the EventProcessingResult construction in the cumulative
result flow to calculate and preserve this delta, ensuring
cumulativeResult.merge(batchDelta) retains failures across intermediate resets.
---
Nitpick comments:
In `@ghost/core/core/server/services/email-analytics/event-processor.ts`:
- Around line 3-8: Update EventProcessor.processBatch to accept
ReadonlyArray<EmailAnalyticsEvent> instead of any[], matching the processor
implementation and preventing event mutation. Preserve the existing result,
fetchData, and Promise<void> contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 435fc591-5b4e-4321-8168-42ef4e09c8b9
📒 Files selected for processing (24)
ghost/core/core/server/api/endpoints/emails.jsghost/core/core/server/services/automations/automations-api.tsghost/core/core/server/services/automations/automations-repository.tsghost/core/core/server/services/automations/database-automations-repository.tsghost/core/core/server/services/automations/mailgun-message-id.tsghost/core/core/server/services/email-analytics/automation-email-analytics-processor.tsghost/core/core/server/services/email-analytics/email-analytics-service-wrapper.jsghost/core/core/server/services/email-analytics/email-analytics-service.jsghost/core/core/server/services/email-analytics/event-processing-result.tsghost/core/core/server/services/email-analytics/event-processor.tsghost/core/core/server/services/email-analytics/index.tsghost/core/core/server/services/email-analytics/lib/queries.jsghost/core/core/server/services/email-analytics/newsletter-email-analytics-processor.jsghost/core/test/integration/services/email-analytics/automation-email-analytics.test.jsghost/core/test/integration/services/email-service/email-event-storage.test.jsghost/core/test/integration/services/mailgun-email-suppression-list.test.jsghost/core/test/unit/server/services/automations/automations-repository.test.tsghost/core/test/unit/server/services/automations/mailgun-message-id.test.tsghost/core/test/unit/server/services/email-analytics/automation-email-analytics-processor.test.tsghost/core/test/unit/server/services/email-analytics/email-analytics-service-wrapper.test.jsghost/core/test/unit/server/services/email-analytics/email-analytics-service.test.jsghost/core/test/unit/server/services/email-analytics/event-processing-result.test.jsghost/core/test/unit/server/services/email-analytics/newsletter-email-analytics-processor.test.jsghost/core/test/utils/fixture-utils.js
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #29333 +/- ##
==========================================
+ Coverage 74.03% 74.11% +0.08%
==========================================
Files 1578 1581 +3
Lines 137430 137903 +473
Branches 16650 16716 +66
==========================================
+ Hits 101744 102212 +468
- Misses 34641 34677 +36
+ Partials 1045 1014 -31
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
cmraible
left a comment
cmraible
left a comment
There was a problem hiding this comment.
Overall looks good to me!
I've smoke tested locally and gone through all the code in detail. Some parts, especially where we create an initialize the wrappers in ghost/core/core/server/services/email-analytics/index.ts are breaking my brain a little bit, but I don't really have any better suggestions and I think it doesn't make it worse than the current state.
I still want to do a bit more stress testing (hence comment instead of approve), but figured I'd share my thoughts on it so far.
| assert.equal(await getOpenedCount(firstRevisionId), null, 'a recipient that had already opened should not be counted again'); | ||
| }); | ||
|
|
||
| it('overwrites delivered and opened timestamps that are later than the new ones', async function () { |
There was a problem hiding this comment.
thought: do we need this? do we do this today for newsletter analytics?
There was a problem hiding this comment.
I think so, for two reasons:
-
I think it makes more sense.
-
It's buried, but we do this for newsletter analytics here:
(That's for
deliveredevents. The same code exists for other events, such asopened.)
| const newsletterEmailEventProcessor = new EmailEventProcessor({ | ||
| domainEvents, | ||
| db, | ||
| eventStorage: new EmailEventStorage({ | ||
| db, | ||
| membersRepository: membersService.api.members, | ||
| models: { | ||
| Email, | ||
| EmailRecipientFailure, | ||
| EmailSpamComplaintEvent | ||
| }, | ||
| emailSuppressionList, | ||
| prometheusClient | ||
| }), | ||
| prometheusClient | ||
| }); | ||
|
|
||
| const newsletterMailgunTags = ['bulk-email']; | ||
| if (config.get('bulkEmail:mailgun:tag')) { | ||
| newsletterMailgunTags.push(config.get('bulkEmail:mailgun:tag')); | ||
| } | ||
|
|
||
| newsletters.init({ | ||
| event: StartEmailAnalyticsJobEvent, | ||
| mailgunTags: newsletterMailgunTags, | ||
| createEventProcessor: () => ( | ||
| new NewsletterEmailAnalyticsProcessor({ | ||
| config, | ||
| emailEventProcessor: newsletterEmailEventProcessor, | ||
| prometheusClient, | ||
| queries | ||
| }) | ||
| ) | ||
| }); |
There was a problem hiding this comment.
thought: this is confusing me.
I think there's a subtle distinction between an "event processor" and an "analytics processor", but my dumb human brain sees "processor" for both and thinks they're the same thing, which makes this feel a bit circular. Codex suggested a potential renaming here — I don't love it, but maybe it's more clear? Although it does add another noun 😅
- NewsletterEmailAnalyticsProcessor -> NewsletterAnalyticsBatchProcessor
- EmailEventProcessor -> EmailEventWriter
- newsletterEmailEventProcessor -> newsletterEmailEventWriter
Feel free to not do anything about this; I think it's not worse than the current state, just wanted to flag that my brain is struggling to comprehend this bit.
There was a problem hiding this comment.
Addressing these in three ways:
- In 922237f, renamed
EventProcessor→BatchEventProcessor,AutomationEmailAnalyticsProcessor→AutomationEmailAnalyticsBatchProcessor, andNewsletterEmailAnalyticsProcessor→NewsletterEmailAnalyticsBatchProcessor. - Filed NY-1474 to rename
EmailEventStoragetoNewsletterEmailEventStorage(simple rename) - Filed NY-1475 to integrate
EmailEventProcessorclass intoNewsletterEmailAnalyticsBatchProcessor
| ); | ||
| }); | ||
|
|
||
| it('counts events with no provider id as unprocessable', async function () { |
There was a problem hiding this comment.
question: is it possible to have an event with no provider ID?
I thought the "provider id" is mailgun's message id, which I think should be available for all events?
There was a problem hiding this comment.
Yeah, you're right. Addressed in 7327c92.
This was converted to TypeScript in b837735.
ref #29333 (comment) `EventProcessor` -> `BatchEventProcessor` `AutomationEmailAnalyticsProcessor` -> `AutomationEmailAnalyticsBatchProcessor` `NewsletterEmailAnalyticsProcessor` -> `NewsletterEmailAnalyticsBatchProcessor`
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
ghost/core/core/server/services/email-analytics/newsletter-email-analytics-batch-processor.js (2)
118-121: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix typographical error in JSDoc.
There is a small typo in the
@paramJSDoc for theerrorobject (enhandedCodeshould beenhancedCode).📝 Proposed fix
- * `@param` {{id: string, type: any; severity: any; recipientEmail: any; emailId?: string; providerId: string; timestamp: Date; error: {code: number; message: string; enhandedCode: string|number} | null}} event + * `@param` {{id: string, type: any; severity: any; recipientEmail: any; emailId?: string; providerId: string; timestamp: Date; error: {code: number; message: string; enhancedCode: string|number} | null}} event🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ghost/core/core/server/services/email-analytics/newsletter-email-analytics-batch-processor.js` around lines 118 - 121, Correct the `error` object property name in the JSDoc for the event parameter from `enhandedCode` to `enhancedCode`, leaving the surrounding type annotation unchanged.
123-209: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReduce code duplication in
#processEvent.The event dispatching logic repeats the same extraction, handling, and result-mapping boilerplate for every event type. You can significantly simplify this method by extracting the common identification object and consolidating the
EventProcessingResultconstruction.♻️ Proposed refactor
async `#processEvent`(event, recipientCache) { const identification = { emailId: event.emailId, providerId: event.providerId, email: event.recipientEmail }; let recipient; let resultKey = event.type; switch (event.type) { case 'delivered': recipient = await this.#emailEventProcessor.handleDelivered(identification, event.timestamp, recipientCache); break; case 'opened': recipient = await this.#emailEventProcessor.handleOpened(identification, event.timestamp, recipientCache); break; case 'unsubscribed': recipient = await this.#emailEventProcessor.handleUnsubscribed(identification, event.timestamp, recipientCache); break; case 'complained': recipient = await this.#emailEventProcessor.handleComplained(identification, event.timestamp, recipientCache); break; case 'failed': const errorData = {id: event.id, timestamp: event.timestamp, error: event.error}; if (event.severity === 'permanent') { recipient = await this.#emailEventProcessor.handlePermanentFailed(identification, errorData, recipientCache); resultKey = 'permanentFailed'; } else { recipient = await this.#emailEventProcessor.handleTemporaryFailed(identification, errorData, recipientCache); resultKey = 'temporaryFailed'; } break; default: return new EventProcessingResult({unhandled: 1}); } if (recipient) { return new EventProcessingResult({ [resultKey]: 1, emailIds: [recipient.emailId], memberIds: [recipient.memberId] }); } return new EventProcessingResult({unprocessable: 1}); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ghost/core/core/server/services/email-analytics/newsletter-email-analytics-batch-processor.js` around lines 123 - 209, Refactor `#processEvent` to eliminate repeated identification and EventProcessingResult construction: create one identification object, dispatch event types through a switch to the corresponding `#emailEventProcessor` handler, and use a resultKey for the result counter. Preserve failed-event severity handling with shared error data, and retain unhandled and unprocessable results.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@ghost/core/core/server/services/email-analytics/automation-email-analytics-batch-processor.ts`:
- Around line 18-24: Update getMailgunMessageIds and getRecipient to validate
providerId before calling normalizeMailgunMessageId or using it; skip or safely
classify events with missing IDs so they are counted as unprocessable, while
continuing to process valid events without throwing or terminating the batch.
---
Nitpick comments:
In
`@ghost/core/core/server/services/email-analytics/newsletter-email-analytics-batch-processor.js`:
- Around line 118-121: Correct the `error` object property name in the JSDoc for
the event parameter from `enhandedCode` to `enhancedCode`, leaving the
surrounding type annotation unchanged.
- Around line 123-209: Refactor `#processEvent` to eliminate repeated
identification and EventProcessingResult construction: create one identification
object, dispatch event types through a switch to the corresponding
`#emailEventProcessor` handler, and use a resultKey for the result counter.
Preserve failed-event severity handling with shared error data, and retain
unhandled and unprocessable results.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f90b77c2-1de3-4439-9593-829db2851718
📒 Files selected for processing (9)
ghost/core/core/server/services/email-analytics/automation-email-analytics-batch-processor.tsghost/core/core/server/services/email-analytics/batch-event-processor.tsghost/core/core/server/services/email-analytics/email-analytics-service-wrapper.jsghost/core/core/server/services/email-analytics/email-analytics-service.jsghost/core/core/server/services/email-analytics/index.tsghost/core/core/server/services/email-analytics/newsletter-email-analytics-batch-processor.jsghost/core/test/unit/server/services/automations/automations-repository.test.tsghost/core/test/unit/server/services/email-analytics/automation-email-analytics-batch-processor.test.tsghost/core/test/unit/server/services/email-analytics/newsletter-email-analytics-batch-processor.test.js
🚧 Files skipped from review as they are similar to previous changes (4)
- ghost/core/test/unit/server/services/automations/automations-repository.test.ts
- ghost/core/core/server/services/email-analytics/email-analytics-service-wrapper.js
- ghost/core/core/server/services/email-analytics/index.ts
- ghost/core/core/server/services/email-analytics/email-analytics-service.js
| const getMailgunMessageIds = (events: Iterable<EmailAnalyticsEvent>): Set<string> => { | ||
| const result = new Set<string>(); | ||
| for (const {providerId} of events) { | ||
| result.add(normalizeMailgunMessageId(providerId)); | ||
| } | ||
| return result; | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard against missing providerId to prevent batch poisoning.
If an incoming event from the webhook lacks a providerId, calling normalizeMailgunMessageId will throw a TypeError (attempting to execute .trim() on undefined). Because this occurs inside a batch processor, a single malformed event will crash the entire loop, poison the queue, and prevent any other valid events in the batch from being processed.
Please add defensive checks to safely handle missing provider IDs so they are natively counted as unprocessable without crashing.
🛡️ Proposed defensive checks
Update getMailgunMessageIds:
const getMailgunMessageIds = (events: Iterable<EmailAnalyticsEvent>): Set<string> => {
const result = new Set<string>();
for (const {providerId} of events) {
- result.add(normalizeMailgunMessageId(providerId));
+ if (providerId) {
+ result.add(normalizeMailgunMessageId(providerId));
+ }
}
return result;
};Update getRecipient:
const getRecipient = () => {
+ if (!event.providerId) {
+ return undefined;
+ }
const mailgunMessageId = normalizeMailgunMessageId(event.providerId);
return automatedEmailRecipientsByMessageId.get(mailgunMessageId);
};Also applies to: 94-97
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@ghost/core/core/server/services/email-analytics/automation-email-analytics-batch-processor.ts`
around lines 18 - 24, Update getMailgunMessageIds and getRecipient to validate
providerId before calling normalizeMailgunMessageId or using it; skip or safely
classify events with missing IDs so they are counted as unprocessable, while
continuing to process valid events without throwing or terminating the batch.
2 participants
towards https://linear.app/ghost/issue/NY-1436
closes https://linear.app/ghost/issue/NY-1447
ref #29211
ref #29229
ref #29286
ref #29289
ref #29290
ref #29291
I recommend reviewing this commit-by-commit.