Switched to time-based domain warmup#33
Conversation
WalkthroughRefactored the email warmup limit calculation system from a hard-coded scaling table to a configuration-driven exponential ramp approach. Updated the DomainWarmingService to accept labs and config dependencies, introduced a new WarmupVolumeOptions type, and replaced discrete scaling steps with a continuous growth formula based on elapsed days. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ghost/core/test/integration/services/email-service/domain-warming.test.js (1)
296-317: Match expected-limit rounding with the service (Math.floor).
The service usesMath.floor, soMath.roundhere will drift by +1 on early days (e.g., day 1) and make the test disagree with runtime behavior.🛠️ Suggested fix
- return Math.round(start * Math.pow(end / start, day / (totalDays - 1))); + return Math.floor(start * Math.pow(end / start, day / (totalDays - 1)));ghost/core/test/unit/server/services/email-service/domain-warming-service.test.ts (1)
170-187: Align expected values with the floor‑based formula.
getWarmupLimitfloors the computed limit, so day‑1 should be ~236 (not 237), and other entries can drift by 1. Derive expectations with the same formula to avoid mismatches.🛠️ Suggested fix
// Helper to create a date N days ago function daysAgo(days: number): string { const date = new Date('2024-01-15T12:00:00Z'); date.setDate(date.getDate() - days); return date.toISOString(); } + + const expectedLimit = (day: number) => Math.floor( + 200 * Math.pow(200000 / 200, day / (42 - 1)) + ); @@ - // Day 1: 200 * (1000)^(1/41) ≈ 237 - assert.equal(result, 237); + // Day 1: floor(200 * (1000)^(1/41)) ≈ 236 + assert.equal(result, expectedLimit(1)); @@ - const testCases = [ - {day: 0, expected: 200}, // Day 0: start value - {day: 1, expected: 237}, // Day 1 - {day: 5, expected: 464}, // Day 5 - {day: 10, expected: 1078}, // Day 10 - {day: 20, expected: 5814}, // Day 20 - {day: 21, expected: 6880}, // Day 21 (halfway) - {day: 30, expected: 31344}, // Day 30 - {day: 40, expected: 168989}, // Day 40 - {day: 41, expected: 200000} // Day 41: end value - ]; + const testDays = [0, 1, 5, 10, 20, 21, 30, 40, 41]; - for (const testCase of testCases) { + for (const day of testDays) { const EmailModel = createModelClass({ findAll: [{ csd_email_count: 100, - created_at: daysAgo(testCase.day) + created_at: daysAgo(day) }] }); @@ - const result = await service.getWarmupLimit(10000000); - assert.equal(result, testCase.expected, `Expected ${testCase.expected} for day ${testCase.day}, but got ${result}`); + const result = await service.getWarmupLimit(10000000); + const expected = expectedLimit(day); + assert.equal(result, expected, `Expected ${expected} for day ${day}, but got ${result}`); }Also applies to: 248-281
🤖 Fix all issues with AI agents
In `@ghost/core/core/server/services/email-service/DomainWarmingService.ts`:
- Around line 64-76: The `#getDaysSinceFirstEmail` function uses Math.ceil which
can count partial days as full days; change the calculation to use Math.floor
for whole-day elapsed calculation and clamp the result to a minimum of 0 (e.g.,
compute diff = Date.now() - new Date(res.data[0].get('created_at') as
string).getTime(), days = Math.floor(diff / (1000*60*60*24)), return Math.max(0,
days)). Ensure you update only the return expression in `#getDaysSinceFirstEmail`
to avoid negative values and prevent advancing warmup on the same calendar day.
- Around line 83-97: In getWarmupLimit of DomainWarmingService, change the
end-of-warmup check to use >= instead of > (i.e., if day >=
this.#warmupConfig.totalDays) so day indices starting at 0 correctly return
Infinity at totalDays, and add missing semicolons in the function block (after
the return Math.min(emailCount, limit) and other statement endings) to satisfy
ESLint; locate references to `#getDaysSinceFirstEmail` and this.#warmupConfig to
update the condition and punctuation.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ghost/core/core/server/services/email-service/DomainWarmingService.tsghost/core/test/integration/services/email-service/domain-warming.test.jsghost/core/test/unit/server/services/email-service/domain-warming-service.test.ts
🧰 Additional context used
🧬 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)
🪛 ESLint
ghost/core/core/server/services/email-service/DomainWarmingService.ts
[error] 84-85: Missing semicolon.
(semi)
[error] 86-87: Missing semicolon.
(semi)
[error] 95-96: Missing semicolon.
(semi)
🔇 Additional comments (6)
ghost/core/core/server/services/email-service/DomainWarmingService.ts (2)
18-28: Warmup options shape and defaults look solid.
Clear, minimal config surface for the new ramp.
34-45: Constructor wiring and warmup config init look good.ghost/core/test/integration/services/email-service/domain-warming.test.js (2)
199-203: Day‑1 warmup annotation is clear.
223-247: Multi‑day progression checks look good.ghost/core/test/unit/server/services/email-service/domain-warming-service.test.ts (2)
16-16: Deterministic clock setup anddaysAgohelper are solid.Also applies to: 31-36, 110-115
117-148: Expanded day‑based and edge‑case coverage looks good.Also applies to: 150-168, 190-224, 226-245, 284-320
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| 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.ceil((Date.now() - new Date(res.data[0].get('created_at') as string).getTime()) / (1000 * 60 * 60 * 24)); | ||
| } |
There was a problem hiding this comment.
Avoid counting partial days as full days.
Math.ceil bumps the day after any positive elapsed time, so a second send later the same day can advance the warmup unexpectedly. Consider floor-based whole-day calculation (and clamp at 0).
🛠️ Suggested fix
- return Math.ceil((Date.now() - new Date(res.data[0].get('created_at') as string).getTime()) / (1000 * 60 * 60 * 24));
+ const firstSentAt = new Date(res.data[0].get('created_at') as string).getTime();
+ const msPerDay = 1000 * 60 * 60 * 24;
+ const daysSince = Math.floor((Date.now() - firstSentAt) / msPerDay);
+ return Math.max(0, daysSince);📝 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.
| 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.ceil((Date.now() - new Date(res.data[0].get('created_at') as string).getTime()) / (1000 * 60 * 60 * 24)); | |
| } | |
| async `#getDaysSinceFirstEmail`(): Promise<number> { | |
| const res = await this.#emailModel.findPage({ | |
| filter: 'csd_email_count:-null', | |
| order: 'created_at ASC', | |
| limit: 1 | |
| }); | |
| if (!res.data.length) { | |
| return 0; | |
| } | |
| const firstSentAt = new Date(res.data[0].get('created_at') as string).getTime(); | |
| const msPerDay = 1000 * 60 * 60 * 24; | |
| const daysSince = Math.floor((Date.now() - firstSentAt) / msPerDay); | |
| return Math.max(0, daysSince); | |
| } |
🤖 Prompt for AI Agents
In `@ghost/core/core/server/services/email-service/DomainWarmingService.ts` around
lines 64 - 76, The `#getDaysSinceFirstEmail` function uses Math.ceil which can
count partial days as full days; change the calculation to use Math.floor for
whole-day elapsed calculation and clamp the result to a minimum of 0 (e.g.,
compute diff = Date.now() - new Date(res.data[0].get('created_at') as
string).getTime(), days = Math.floor(diff / (1000*60*60*24)), return Math.max(0,
days)). Ensure you update only the return expression in `#getDaysSinceFirstEmail`
to avoid negative values and prevent advancing warmup on the same calendar day.
| async getWarmupLimit(emailCount: number): Promise<number> { | ||
| const day = await this.#getDaysSinceFirstEmail() | ||
| if (day > this.#warmupConfig.totalDays) { | ||
| return Infinity | ||
| } | ||
|
|
||
| for (const threshold of WARMUP_SCALING_TABLE.thresholds.sort((a, b) => a.limit - b.limit)) { | ||
| if (lastCount <= threshold.limit) { | ||
| return Math.ceil(lastCount * threshold.scale); | ||
| } | ||
| } | ||
| const limit = Math.floor( | ||
| this.#warmupConfig.start * | ||
| Math.pow( | ||
| this.#warmupConfig.end / this.#warmupConfig.start, | ||
| day / (this.#warmupConfig.totalDays - 1) | ||
| ) | ||
| ) | ||
|
|
||
| // This should not be reached given the thresholds cover all cases up to highVolume.threshold | ||
| return Math.ceil(lastCount * WARMUP_SCALING_TABLE.highVolume.maxScale); | ||
| return Math.min(emailCount, limit) |
There was a problem hiding this comment.
End warmup at day >= totalDays (and fix lint semicolons).
With a day index starting at 0, > leaves an extra day and contradicts the day‑42 “Infinity” expectation. ESLint also flags missing semicolons in this block.
🛠️ Suggested fix
- const day = await this.#getDaysSinceFirstEmail()
- if (day > this.#warmupConfig.totalDays) {
- return Infinity
+ const day = await this.#getDaysSinceFirstEmail();
+ if (day >= this.#warmupConfig.totalDays) {
+ return Infinity;
}
@@
- )
+ );
- return Math.min(emailCount, limit)
+ return Math.min(emailCount, limit);📝 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.
| async getWarmupLimit(emailCount: number): Promise<number> { | |
| const day = await this.#getDaysSinceFirstEmail() | |
| if (day > this.#warmupConfig.totalDays) { | |
| return Infinity | |
| } | |
| for (const threshold of WARMUP_SCALING_TABLE.thresholds.sort((a, b) => a.limit - b.limit)) { | |
| if (lastCount <= threshold.limit) { | |
| return Math.ceil(lastCount * threshold.scale); | |
| } | |
| } | |
| const limit = Math.floor( | |
| this.#warmupConfig.start * | |
| Math.pow( | |
| this.#warmupConfig.end / this.#warmupConfig.start, | |
| day / (this.#warmupConfig.totalDays - 1) | |
| ) | |
| ) | |
| // This should not be reached given the thresholds cover all cases up to highVolume.threshold | |
| return Math.ceil(lastCount * WARMUP_SCALING_TABLE.highVolume.maxScale); | |
| return Math.min(emailCount, limit) | |
| async getWarmupLimit(emailCount: number): Promise<number> { | |
| const day = await this.#getDaysSinceFirstEmail(); | |
| if (day >= this.#warmupConfig.totalDays) { | |
| return Infinity; | |
| } | |
| const limit = Math.floor( | |
| this.#warmupConfig.start * | |
| Math.pow( | |
| this.#warmupConfig.end / this.#warmupConfig.start, | |
| day / (this.#warmupConfig.totalDays - 1) | |
| ) | |
| ); | |
| return Math.min(emailCount, limit); | |
| } |
🧰 Tools
🪛 ESLint
[error] 84-85: Missing semicolon.
(semi)
[error] 86-87: Missing semicolon.
(semi)
[error] 95-96: Missing semicolon.
(semi)
🤖 Prompt for AI Agents
In `@ghost/core/core/server/services/email-service/DomainWarmingService.ts` around
lines 83 - 97, In getWarmupLimit of DomainWarmingService, change the
end-of-warmup check to use >= instead of > (i.e., if day >=
this.#warmupConfig.totalDays) so day indices starting at 0 correctly return
Infinity at totalDays, and add missing semicolons in the function block (after
the return Math.min(emailCount, limit) and other statement endings) to satisfy
ESLint; locate references to `#getDaysSinceFirstEmail` and this.#warmupConfig to
update the condition and punctuation.
2 participants
Benchmark PR from qodo-benchmark#236
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.