Switched to time-based domain warmup#47
Conversation
WalkthroughThe domain warming service refactors from a rigid lookup table model to a dynamic time-based exponential growth algorithm. The warmup limit now scales exponentially between configured start and end values over a specified number of days, computed from elapsed time since the first email. 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
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
🤖 Fix all issues with AI agents
In `@ghost/core/core/server/services/email-service/DomainWarmingService.ts`:
- Around line 83-98: The getWarmupLimit method is missing semicolons causing
ESLint style failures; update the function (DomainWarmingService.getWarmupLimit)
to add semicolons after the const day declaration, after the if-block's return
Infinity statement, and after the const limit assignment/return lines so that
statements like "const day = await this.#getDaysSinceFirstEmail()", the limit
calculation assignment, and the final "return Math.min(emailCount, limit)" end
with semicolons; ensure all newly added semicolons match the codebase style.
In `@ghost/core/test/integration/services/email-service/domain-warming.test.js`:
- Around line 296-303: The test helper getExpectedLimit uses Math.round but the
implementation in DomainWarmingService.getWarmupLimit uses Math.floor, causing
possible mismatches; update the helper getExpectedLimit to use Math.floor (same
start/end/totalDays formula) so both functions compute limits identically, and
run the test to ensure assertions align with
DomainWarmingService.getWarmupLimit.
📜 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/unit/server/services/email-service/domain-warming-service.test.ts (1)
ghost/core/core/server/services/email-service/DomainWarmingService.ts (1)
DomainWarmingService(30-99)
🪛 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 (10)
ghost/core/test/unit/server/services/email-service/domain-warming-service.test.ts (5)
16-38: LGTM! Proper fake timer setup for deterministic tests.The clock initialization with a fixed date and proper cleanup in
afterEachensures consistent, reproducible test results for the time-based warmup logic.
110-115: LGTM! Clean helper for generating test dates.The
daysAgohelper correctly uses the same fixed date as the fake timer, ensuring consistent day calculations across all tests.
248-282: LGTM! Comprehensive warmup progression test.The test cases cover the full exponential curve from start (200) to end (200000) with well-chosen intermediate checkpoints. The expected values align with the formula
start * (end/start)^(day/(totalDays-1)).
284-320: LGTM! Good boundary and post-warmup coverage.Testing both the exact boundary (day 42) and a value well beyond (day 100) ensures the Infinity return is robust.
226-246: LGTM! Query behavior verification is correct.The test properly validates the filter, ordering, and limit parameters for fetching the first qualifying email.
ghost/core/core/server/services/email-service/DomainWarmingService.ts (2)
18-28: LGTM! Clean warmup configuration type and defaults.The
WarmupVolumeOptionstype andDefaultWarmupOptionsconstant provide a clear, configurable foundation for the time-based warmup algorithm.
64-76: LGTM! Day calculation logic is sound.Using
Math.ceilensures that even partial days count as a full day elapsed, which is appropriate for a warmup progression that should advance once any time has passed since the first email.ghost/core/test/integration/services/email-service/domain-warming.test.js (3)
199-207: LGTM! Day 2 test correctly uses time-based warmup formula.The expected limit of 237 for day 1 aligns with the exponential formula and unit test expectations.
223-247: LGTM! Multi-day progression test validates the exponential curve.The expected values for days 0, 1, and 2 are mathematically correct and consistent with the unit tests.
305-323: LGTM! Comprehensive maximum limit test loop.The 5-day progression test with non-decreasing validation is well-structured. Just ensure the
getExpectedLimithelper is fixed per the previous comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| 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.
Add missing semicolons for code style consistency.
ESLint flags missing semicolons at lines 84, 86, and 95. While JavaScript/TypeScript allows semicolon omission, the codebase appears to use semicolons consistently.
🔧 Proposed fix
async getWarmupLimit(emailCount: number): Promise<number> {
- const day = await this.#getDaysSinceFirstEmail()
+ const day = await this.#getDaysSinceFirstEmail();
if (day > this.#warmupConfig.totalDays) {
- return Infinity
+ 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)
+ 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)
[error] 97-98: Missing semicolon.
(semi)
🤖 Prompt for AI Agents
In `@ghost/core/core/server/services/email-service/DomainWarmingService.ts` around
lines 83 - 98, The getWarmupLimit method is missing semicolons causing ESLint
style failures; update the function (DomainWarmingService.getWarmupLimit) to add
semicolons after the const day declaration, after the if-block's return Infinity
statement, and after the const limit assignment/return lines so that statements
like "const day = await this.#getDaysSinceFirstEmail()", the limit calculation
assignment, and the final "return Math.min(emailCount, limit)" end with
semicolons; ensure all newly added semicolons match the codebase style.
| // Time-based warmup: limit = start * (end/start)^(day/(totalDays-1)) | ||
| // With start=200, end=200000, totalDays=42 | ||
| const getExpectedLimit = (day) => { | ||
| const start = 200; | ||
| const end = 200000; | ||
| const totalDays = 42; | ||
| return Math.round(start * Math.pow(end / start, day / (totalDays - 1))); | ||
| }; |
There was a problem hiding this comment.
Inconsistent rounding: Math.round vs Math.floor in implementation.
The getExpectedLimit helper uses Math.round, but the actual DomainWarmingService.getWarmupLimit implementation uses Math.floor. This could cause test assertions to fail when the fractional part is ≥ 0.5.
🔧 Proposed fix
const getExpectedLimit = (day) => {
const start = 200;
const end = 200000;
const totalDays = 42;
- return Math.round(start * Math.pow(end / start, day / (totalDays - 1)));
+ return Math.floor(start * Math.pow(end / start, day / (totalDays - 1)));
};📝 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.
| // Time-based warmup: limit = start * (end/start)^(day/(totalDays-1)) | |
| // With start=200, end=200000, totalDays=42 | |
| const getExpectedLimit = (day) => { | |
| const start = 200; | |
| const end = 200000; | |
| const totalDays = 42; | |
| return Math.round(start * Math.pow(end / start, day / (totalDays - 1))); | |
| }; | |
| // Time-based warmup: limit = start * (end/start)^(day/(totalDays-1)) | |
| // With start=200, end=200000, totalDays=42 | |
| const getExpectedLimit = (day) => { | |
| const start = 200; | |
| const end = 200000; | |
| const totalDays = 42; | |
| return Math.floor(start * Math.pow(end / start, day / (totalDays - 1))); | |
| }; |
🤖 Prompt for AI Agents
In `@ghost/core/test/integration/services/email-service/domain-warming.test.js`
around lines 296 - 303, The test helper getExpectedLimit uses Math.round but the
implementation in DomainWarmingService.getWarmupLimit uses Math.floor, causing
possible mismatches; update the helper getExpectedLimit to use Math.floor (same
start/end/totalDays formula) so both functions compute limits identically, and
run the test to ensure assertions align with
DomainWarmingService.getWarmupLimit.
2 participants
Benchmark PR from agentic-review-benchmarks#9
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.