Switched to time-based domain warmup#20
Conversation
WalkthroughThe pull request replaces the email domain warming service's scaling-based model with a time-based progression system. The 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: 3
🤖 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 on several
statements causing ESLint failures; update the function (method getWarmupLimit
and any statements using this.#getDaysSinceFirstEmail() and the const limit
calculation that references this.#warmupConfig) to terminate each statement with
a semicolon (after the const day assignment, after the if block return Infinity
line, and after the return Math.min(emailCount, limit) line) so the file
conforms to the project's semicolon linting rule.
In `@ghost/core/test/integration/services/email-service/domain-warming.test.js`:
- Around line 298-303: The test helper getExpectedLimit uses Math.round but
DomainWarmingService.getWarmupLimit uses Math.floor, causing off-by-one
mismatches; update getExpectedLimit to use Math.floor (keep same
start/end/totalDays formula) so the helper mirrors
DomainWarmingService.getWarmupLimit's rounding behavior when calculating
expected limits.
In
`@ghost/core/test/unit/server/services/email-service/domain-warming-service.test.ts`:
- Around line 284-301: The test expects warmup to be complete at 42 days but the
implementation in DomainWarmingService uses a strict greater-than check (day >
this.#warmupConfig.totalDays), causing day=42 to miss the completion branch;
update the boundary check in getWarmupLimit to use >=
this.#warmupConfig.totalDays (or equivalent) so that when the elapsed days is
equal to warmupConfig.totalDays the method returns Infinity as the test expects.
🧹 Nitpick comments (1)
ghost/core/core/server/services/email-service/DomainWarmingService.ts (1)
64-76: ConsiderMath.floorinstead ofMath.ceilfor day calculation.Using
Math.ceilmeans an email sent a few hours ago counts as "1 day ago," which could cause warmup to progress faster than intended. For example, an email sent 8 hours ago would yieldMath.ceil(0.33) = 1.Consider whether
Math.floorbetter matches the business intent of counting complete days elapsed.
📜 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 (2)
ghost/core/test/integration/services/email-service/domain-warming.test.js (1)
ghost/core/test/unit/server/services/email-service/batch-sending-service.test.js (4)
sendEmail(96-102)sendEmail(125-131)sendEmail(152-164)sendEmail(193-199)
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 (7)
ghost/core/core/server/services/email-service/DomainWarmingService.ts (2)
85-87: Potential off-by-one in warmup completion check.The condition
day > this.#warmupConfig.totalDaysreturnsInfinitystarting from day 43, not day 42. WithtotalDays=42:
- Day 41: calculates limit (200,000 - the end value)
- Day 42:
42 > 42is false, so it calculates200 * 1000^(42/41) ≈ 236,798(exceeds configured end)- Day 43: returns
InfinityIf the intent is that warmup completes after 42 days (days 0–41), the condition should be
>=:Suggested fix if warmup should complete on day 42
- if (day > this.#warmupConfig.totalDays) { + if (day >= this.#warmupConfig.totalDays) {Please verify the intended behavior: should day 42 return
Infinityor calculate a limit exceeding the configured end value?
18-28: LGTM on the warmup configuration design.The
WarmupVolumeOptionstype andDefaultWarmupOptionsconstant provide a clean, configurable approach to the time-based warmup. The exponential formulastart * (end/start)^(day/(totalDays-1))creates a smooth progression from 200 to 200,000 over the warmup period.ghost/core/test/unit/server/services/email-service/domain-warming-service.test.ts (2)
109-115: Well-structured test helper and time management.The
daysAgohelper and fake timer setup provide deterministic test behavior. The approach of fixing the clock to a specific date and computing relative dates ensures consistent results across test runs.
248-282: Comprehensive day progression test coverage.The progression test thoroughly validates the exponential formula across multiple days (0, 1, 5, 10, 20, 21, 30, 40, 41) with mathematically verified expected values. Good inclusion of the error message showing both expected and actual values for debugging.
ghost/core/test/integration/services/email-service/domain-warming.test.js (3)
166-184: Good integration test for initial warmup behavior.The test properly validates that when total recipients (104) are below the warmup limit (200), all emails go through the custom domain. The assertions verify both the email model counts and the actual batch distribution.
109-163: Thorough test cleanup ensures isolation.The
afterEachcleanup properly handles clock restoration, stub cleanup, and database cleanup for test-created members, emails, batches, and recipients. This prevents test pollution and maintains reliable test runs.
220-247: Multi-day progression test validates time-based warmup correctly.The test verifies the exponential growth formula across days 0, 1, and 2 with specific expected limits (200, 237, 280). Comments clearly document the formula being tested.
✏️ 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.
Missing semicolons flagged by ESLint.
Lines 84, 86, and 95 are missing semicolons, which violates the project's linting rules.
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);
}🧰 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 on several
statements causing ESLint failures; update the function (method getWarmupLimit
and any statements using this.#getDaysSinceFirstEmail() and the const limit
calculation that references this.#warmupConfig) to terminate each statement with
a semicolon (after the const day assignment, after the if block return Infinity
line, and after the return Math.min(emailCount, limit) line) so the file
conforms to the project's semicolon linting rule.
| 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 implementation's Math.floor.
The getExpectedLimit helper uses Math.round, but the actual DomainWarmingService.getWarmupLimit implementation uses Math.floor. This could cause test failures for edge cases where the calculated value has a fractional part near 0.5.
Proposed fix to match implementation
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.
| const getExpectedLimit = (day) => { | |
| const start = 200; | |
| const end = 200000; | |
| const totalDays = 42; | |
| return Math.round(start * Math.pow(end / start, day / (totalDays - 1))); | |
| }; | |
| 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 298 - 303, The test helper getExpectedLimit uses Math.round but
DomainWarmingService.getWarmupLimit uses Math.floor, causing off-by-one
mismatches; update getExpectedLimit to use Math.floor (keep same
start/end/totalDays formula) so the helper mirrors
DomainWarmingService.getWarmupLimit's rounding behavior when calculating
expected limits.
| it('should return Infinity after warmup period is complete', async function () { | ||
| // After 42 days, warmup is complete | ||
| Email = createModelClass({ | ||
| findAll: [{ | ||
| csd_email_count: 100, | ||
| created_at: daysAgo(42) | ||
| }] | ||
| }); | ||
|
|
||
| const service = new DomainWarmingService({ | ||
| models: {Email}, | ||
| labs, | ||
| config | ||
| }); | ||
|
|
||
| const result = await service.getWarmupLimit(1000000); | ||
| assert.equal(result, Infinity); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the domain-warming-service implementation file
find . -name "domain-warming-service.ts" -type f | grep -v test | head -5Repository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 66
🏁 Script executed:
# Verify the exact boundary condition in the implementation
ast-grep --pattern 'if (day > this.#warmupConfig.totalDays) {
$$$
}'Repository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 390
🏁 Script executed:
# Also check for >= condition as alternative
ast-grep --pattern 'if (day >= this.#warmupConfig.totalDays) {
$$$
}'Repository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 66
🏁 Script executed:
# Search the test file for warmupConfig or totalDays configuration
rg -A 5 -B 5 "warmupConfig|totalDays" ghost/core/test/unit/server/services/email-service/domain-warming-service.test.tsRepository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 681
🏁 Script executed:
# Also check the implementation to see the default warmupConfig
cat -n ghost/core/core/server/services/email-service/DomainWarmingService.ts | head -100Repository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 3620
🏁 Script executed:
# Look at the full test file around the boundary test to see context
sed -n '270,310p' ghost/core/test/unit/server/services/email-service/domain-warming-service.test.tsRepository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 1491
Test expectation at day 42 conflicts with implementation boundary condition.
The test expects Infinity when the first email was 42 days ago. However, the implementation uses day > this.#warmupConfig.totalDays where totalDays is 42, meaning 42 > 42 evaluates to false. The test will fail because a calculated limit will be returned instead of Infinity.
The implementation condition should be >= to match the test assertion that "after 42 days, warmup is complete."
🤖 Prompt for AI Agents
In
`@ghost/core/test/unit/server/services/email-service/domain-warming-service.test.ts`
around lines 284 - 301, The test expects warmup to be complete at 42 days but
the implementation in DomainWarmingService uses a strict greater-than check (day
> this.#warmupConfig.totalDays), causing day=42 to miss the completion branch;
update the boundary check in getWarmupLimit to use >=
this.#warmupConfig.totalDays (or equivalent) so that when the elapsed days is
equal to warmupConfig.totalDays the method returns Infinity as the test expects.
2 participants
Benchmark PR from qodo-benchmark#236
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.