Conversation
WalkthroughThis PR refactors the domain warmup logic in DomainWarmingService from a discrete threshold-based scaling approach to a continuous, time-based exponential formula. The service replaces the hardcoded WarmupScalingTable with a new WarmupVolumeOptions configuration and DefaultWarmupOptions constant. The warmup limit calculation now uses an exponential ramp formula ( Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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: 1
🧹 Nitpick comments (3)
ghost/core/test/unit/server/services/email-service/domain-warming-service.test.ts (1)
16-16: Explicit clock restoration might be redundant.The fake timer is explicitly restored on line 36, but
sinon.restore()on line 37 already handles cleanup of all Sinon modifications including fake timers. While this explicit restoration is not harmful, it may be unnecessary.Based on learnings, sinon.restore() in afterEach hooks handles cleanup of all sinon modifications including fake timers, so explicit restoration of individual sinon features is typically not needed when there's a top-level sinon.restore() call.
Consider simplifying to:
- afterEach(function () { - clock.restore(); - sinon.restore(); - }); + afterEach(function () { + sinon.restore(); + });Also applies to: 31-32, 36-36
ghost/core/test/integration/services/email-service/domain-warming.test.js (2)
199-207: Clarify the formula in comments.The comment on line 200 states
200 * (200000/200)^(1/41)but simplifies the ratio presentation. Consider showing the full calculation for clarity:- // Time-based warmup: limit = start * (end/start)^(day/(totalDays-1)) - // Day 1: 200 * (200000/200)^(1/41) ≈ 237 + // Time-based warmup: limit = start * (end/start)^(day/(totalDays-1)) + // Day 1: 200 * (200000/200)^(1/41) = 200 * (1000)^(1/41) ≈ 237
298-303: Consider extracting the helper for reusability.The
getExpectedLimithelper encapsulates the warmup formula calculation well. If this pattern is needed in other tests, consider extracting it to a shared test utility.For example, at the top of the test file:
// Helper: Calculate expected warmup limit for a given day function getExpectedLimit(day, totalCount = Infinity) { const start = 200; const end = 200000; const totalDays = 42; const limit = Math.round(start * Math.pow(end / start, day / (totalDays - 1))); return Math.min(totalCount, limit); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ghost/core/core/server/services/email-service/DomainWarmingService.ts(3 hunks)ghost/core/test/integration/services/email-service/domain-warming.test.js(4 hunks)ghost/core/test/unit/server/services/email-service/domain-warming-service.test.ts(11 hunks)
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/**/*.test.ts : Use factory pattern for all test data creation instead of hard-coded data or direct database manipulation
Applied to files:
ghost/core/test/unit/server/services/email-service/domain-warming-service.test.ts
📚 Learning: 2025-09-02T13:06:50.918Z
Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 24779
File: ghost/core/test/unit/server/services/members/members-api/controllers/RouterController.test.js:1388-1392
Timestamp: 2025-09-02T13:06:50.918Z
Learning: In Ghost's test files, sinon.restore() in afterEach hooks handles cleanup of all sinon modifications including fake timers, spies, and stubs, so explicit restoration of individual sinon features like clock.restore() is typically not needed when there's a top-level sinon.restore() call.
Applied to files:
ghost/core/test/unit/server/services/email-service/domain-warming-service.test.ts
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/**/*.test.ts : Never use hard-coded waits like `waitForTimeout()`
Applied to files:
ghost/core/test/unit/server/services/email-service/domain-warming-service.test.ts
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/**/*.test.ts : Use Playwright's auto-waiting capabilities and run tests multiple times to ensure stability
Applied to files:
ghost/core/test/unit/server/services/email-service/domain-warming-service.test.ts
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Each test receives fresh Ghost instance for automatic isolation
Applied to files:
ghost/core/test/unit/server/services/email-service/domain-warming-service.test.ts
📚 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/test/integration/services/email-service/domain-warming.test.jsghost/core/core/server/services/email-service/DomainWarmingService.ts
🧬 Code graph analysis (1)
ghost/core/test/integration/services/email-service/domain-warming.test.js (2)
ghost/core/test/unit/server/services/email-service/email-service.test.js (1)
assert(2-2)ghost/core/test/unit/server/services/email-service/batch-sending-service.test.js (5)
assert(4-4)sendEmail(96-102)sendEmail(125-131)sendEmail(152-164)sendEmail(193-199)
⏰ 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). (9)
- GitHub Check: Legacy tests (Node 22.18.0, mysql8)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Acceptance tests (Node 22.18.0, sqlite3)
- GitHub Check: Unit tests (Node 22.18.0)
- GitHub Check: Acceptance tests (Node 22.18.0, mysql8)
- GitHub Check: Lint
- GitHub Check: Legacy tests (Node 22.18.0, sqlite3)
- GitHub Check: Cursor Bugbot
- GitHub Check: Build & Push Docker Image
🔇 Additional comments (9)
ghost/core/test/unit/server/services/email-service/domain-warming-service.test.ts (3)
110-115: LGTM! Clean helper for generating test dates.The
daysAgohelper provides a clear, consistent way to generate timestamps for time-based testing. Good alignment with the fixed clock date.
248-282: LGTM! Comprehensive warmup progression validation.Excellent test coverage of the exponential warmup formula across key inflection points (day 0, 1, 5, 10, 20, 21, 30, 40, 41). The expected values are mathematically correct based on the formula
start * (end/start)^(day/(totalDays-1)).
284-320: LGTM! Proper validation of warmup completion behavior.These tests correctly verify that the warmup limit becomes
Infinityafter the 42-day period completes, ensuring no artificial limits are imposed on mature sending domains.ghost/core/test/integration/services/email-service/domain-warming.test.js (2)
223-247: LGTM! Clear multi-day progression validation.The test correctly validates the exponential warmup formula across multiple days with clear documentation of expected values at each step.
186-207: LGTM! Correct validation of day-to-day progression.The test properly verifies that the warmup limit increases from day 0 to day 1 according to the exponential formula, and correctly uses
setDay(1)to advance time.ghost/core/core/server/services/email-service/DomainWarmingService.ts (4)
18-28: LGTM! Clean configuration structure.The
WarmupVolumeOptionstype andDefaultWarmupOptionsconstant provide a clear, maintainable way to configure the warmup parameters. The defaults (200 → 200,000 over 42 days) align with the PR objectives.
75-75: LGTM! Correct day calculation with Date.now().Using
Date.now()instead ofnew Date()ensures compatibility with Sinon's fake timers during testing. TheMath.floorcorrectly rounds down partial days.
83-98: LGTM! Mathematically sound exponential warmup formula.The implementation correctly applies the exponential formula
limit = start * (end/start)^(day/(totalDays-1)):
- Day 0 produces the start value (200)
- Day 41 produces the end value (200,000)
- Day ≥ 42 returns Infinity (no limit)
- Result is capped at emailCount to prevent oversending
The use of
totalDays - 1in the denominator is correct for achieving the exact end value on the final day.
64-68: LGTM! Proper query filtering for warmup progression.The query correctly filters for emails with
csd_email_count:-null(emails sent via custom domain) and orders bycreated_at ASCto find the first email in the warmup sequence.
| async #getDaysSinceFirstEmail(): Promise<number> { | ||
| const res = await this.#emailModel.findPage({ | ||
| filter: 'csd_email_count:-null', | ||
| order: 'created_at ASC', | ||
| limit: 1 | ||
| }); | ||
|
|
||
| if (!result.data.length) { | ||
| if (!res.data.length) { | ||
| return 0; | ||
| } | ||
|
|
||
| const count = result.data[0].get('csd_email_count'); | ||
| return count || 0; | ||
| return Math.floor((Date.now() - new Date(res.data[0].get('created_at') as string).getTime()) / (1000 * 60 * 60 * 24)); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find Email model definition and check created_at field type
find . -type f \( -name "*.ts" -o -name "*.js" \) | xargs rg -l "class Email|interface Email|export.*Email" | head -20Repository: TryGhost/Ghost
Length of output: 1210
🏁 Script executed:
# Search for Email model in common locations
rg -nP --type=ts -C5 "class Email" --max-count=5Repository: TryGhost/Ghost
Length of output: 4088
🏁 Script executed:
# Look for Email model definition or interface
rg -nP --type=ts "created_at" -B2 -A2 | grep -A5 -B5 "Email\|model" | head -60Repository: TryGhost/Ghost
Length of output: 4957
🏁 Script executed:
# Search for Email model or EmailRepository with Bookshelf
rg -nP --type=ts -C3 "class.*Email.*extends|EmailRepository|Email.*model" | head -80Repository: TryGhost/Ghost
Length of output: 1202
🏁 Script executed:
# Look for Email model in core/server directory specifically
find ./ghost/core/core/server -type f -name "*mail*" -o -name "*email*" | grep -i "model\|repository" | head -20Repository: TryGhost/Ghost
Length of output: 429
🏁 Script executed:
# Search for where this.#emailModel is defined in DomainWarmingService
rg -nP --type=ts "emailModel\|Email.*Model" ./ghost/core/core/server/services/email-service/ -B3 -A3Repository: TryGhost/Ghost
Length of output: 40
🏁 Script executed:
# Read the Email model definition
cat -n ./ghost/core/core/server/models/email.js | head -100Repository: TryGhost/Ghost
Length of output: 3328
🏁 Script executed:
# Read the DomainWarmingService file to understand the context
cat -n ./ghost/core/core/server/services/email-service/DomainWarmingService.ts | head -100Repository: TryGhost/Ghost
Length of output: 3600
🏁 Script executed:
# Check what Bookshelf's get method returns for timestamps
rg -nP --type=ts "get\(" ./ghost/core/core/server/services -A1 -B1 | grep -A2 -B2 "created_at\|Date\|timestamp" | head -40Repository: TryGhost/Ghost
Length of output: 2670
🏁 Script executed:
# Search for how other services handle created_at from Bookshelf models
rg -nP --type=ts "\.get\('created_at'\)" -B2 -A2 | head -60Repository: TryGhost/Ghost
Length of output: 2822
Change type assertion from as string to as Date to match codebase conventions.
The created_at field from Bookshelf models is consistently cast as Date across similar repositories in the codebase (e.g., BookshelfClickEventRepository, BookshelfRecommendationRepository, BookshelfSubscribeEventRepository). Line 75 should use as Date instead of as string for consistency and correctness.
🤖 Prompt for AI Agents
In ghost/core/core/server/services/email-service/DomainWarmingService.ts around
lines 64 to 76, the created_at value is being asserted as a string then passed
to new Date; update the assertion to match codebase conventions by casting the
Bookshelf field to Date (as Date) and use its getTime() directly (e.g.
(res.data[0].get('created_at') as Date).getTime()) so the type reflects the
model and avoids reconstructing a Date from a string.
aileen
left a comment
aileen
left a comment
There was a problem hiding this comment.
So much simpler! Really nice. The tests helped me a lot as well, to understand the handling of the very first email to be send. 👍
2 participants
closes GVA-617
closes GVA-619
This uses the new domain warmup algorithm that makes the warmup static:
This simplifies the flow considerably, and we can experiment with the exact thresholds to make sure this is effective for most users of domain warming.
Note
Replaces threshold-based scaling with a time-based exponential warmup from 200 to 200k over 42 days, with Infinity after completion, and updates tests accordingly.
DomainWarmingService.ts)WarmupVolumeOptions(start=200,end=200000,totalDays=42).#getDaysSinceFirstEmail()querying firstcsd_email_countemail (findPagewithcsd_email_count:-null, ordered bycreated_at ASC).getWarmupLimitnow computes limit viastart * (end/start)^(day/(totalDays-1))and returnsInfinityonceday >= totalDays.Infinity.Written by Cursor Bugbot for commit 4d1012b. This will update automatically on new commits. Configure here.