Switched to time-based domain warmup#9
Conversation
📝 WalkthroughWalkthroughThe email warming service's domain warmup logic is refactored from a hard-coded scaling table to a time-based progression model. The new system ramps email volume from a configurable start value to an end value over a defined number of days, replacing the previous manual scaling structure and simplifying the calculation approach. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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
🧪 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 review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
🤖 Fix all issues with AI agents
In `@ghost/core/core/server/services/email-service/DomainWarmingService.ts`:
- Around line 83-97: In getWarmupLimit, the warmup completion check and rounding
are off: change the completion test from using ">" to ">=" when comparing day to
this.#warmupConfig.totalDays (so the ramp stops on the completion day), switch
Math.floor to Math.round when computing limit so intermediate expected values
aren’t undercut, and add missing semicolons to the function body; update
references in the calculations that use this.#warmupConfig.start,
this.#warmupConfig.end, and this.#warmupConfig.totalDays and keep the final
return as Math.min(emailCount, limit).
- Around line 64-76: The day-count calculation in `#getDaysSinceFirstEmail` uses
Math.ceil which increments the day on any positive difference; change it to use
Math.floor on (Date.now() - new Date(res.data[0].get('created_at') as
string).getTime()) / (1000 * 60 * 60 * 24) and clamp the result to a minimum of
0 (e.g., Math.max(0, Math.floor(...))) so emails sent later the same day do not
advance the warmup day.
🧹 Nitpick comments (1)
ghost/core/test/integration/services/email-service/domain-warming.test.js (1)
199-203: Consider deriving expected limits from a shared helper to avoid magic numbers.
Hardcoding 237/280/etc. can drift if warmup defaults change. You already compute expected limits later; extracting a reusable helper would keep all tests aligned.Also applies to: 223-247
| 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.
Day calculation advances within the same day; use floor/clamp.
Using Math.ceil means any positive diff becomes day 1, so a second send minutes later bumps the warmup. That contradicts same‑day stability and can overshoot early.
🛠️ Proposed fix
- return Math.ceil((Date.now() - new Date(res.data[0].get('created_at') as string).getTime()) / (1000 * 60 * 60 * 24));
+ const diffMs = Date.now() - new Date(res.data[0].get('created_at') as string).getTime();
+ const diffDays = Math.max(0, Math.floor(diffMs / (1000 * 60 * 60 * 24)));
+ return diffDays;🤖 Prompt for AI Agents
In `@ghost/core/core/server/services/email-service/DomainWarmingService.ts` around
lines 64 - 76, The day-count calculation in `#getDaysSinceFirstEmail` uses
Math.ceil which increments the day on any positive difference; change it to use
Math.floor on (Date.now() - new Date(res.data[0].get('created_at') as
string).getTime()) / (1000 * 60 * 60 * 24) and clamp the result to a minimum of
0 (e.g., Math.max(0, Math.floor(...))) so emails sent later the same day do not
advance the warmup 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.
Warmup completion and rounding are off by one (and lint).
day > totalDays keeps the ramp active on the completion day, and Math.floor undercuts the expected progression values. Also, ESLint flags missing semicolons here.
🛠️ Proposed 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;
}
- const limit = Math.floor(
+ const limit = Math.round(
this.#warmupConfig.start *
Math.pow(
this.#warmupConfig.end / this.#warmupConfig.start,
day / (this.#warmupConfig.totalDays - 1)
)
- )
+ );
- 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.round( | |
| 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, the warmup completion check and rounding are
off: change the completion test from using ">" to ">=" when comparing day to
this.#warmupConfig.totalDays (so the ramp stops on the completion day), switch
Math.floor to Math.round when computing limit so intermediate expected values
aren’t undercut, and add missing semicolons to the function body; update
references in the calculations that use this.#warmupConfig.start,
this.#warmupConfig.end, and this.#warmupConfig.totalDays and keep the final
return as Math.min(emailCount, limit).
3 participants
Benchmark PR from agentic-review-benchmarks#9
Summary by CodeRabbit