Updated the scaling factors for domain warmup#41
Conversation
closes [GVA-605](https://linear.app/ghost/issue/GVA-605/adjust-the-warmup-numbers-to-reflect-joeys-suggestions) Our initial scaling factors were based on some preliminary research, and were designed to be as simple as possible. After further discussions, we've decided to avoid having such a fast ramp-up at the early stage, and limit the growth at the later stages to 75k+ per send. That should still prevent any large sites from going back into warm-up between sends, but is more likely to avoid damaging the reputation by scaling too aggressively.
WalkthroughThe changes refactor the domain warming service's email data access from single-record Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 101-104: The filter used in DomainWarmingService when calling
this.#emailModel.findPage currently uses `created_at:<=${new
Date().toISOString().split('T')[0]}` which can include today's emails; change
the comparator to a strict less-than so today is excluded (e.g., use
`created_at:<${...}` or equivalent) so the method's "excluding today" contract
is respected when querying via this.#emailModel.findPage.
- Around line 111-112: The null check using the != operator in
DomainWarmingService.ts should be replaced with nullish coalescing to satisfy
eqeqeq; change the return logic around the variable count (from
result.data[0].get('csd_email_count')) to use count ?? 0 (or inline the ?? when
assigning) so that null/undefined fall back to 0 without using !=.
📜 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 (12)
clock(901-901)clock(935-935)clock(1002-1002)clock(1051-1051)clock(1083-1083)clock(1729-1729)clock(1745-1745)clock(1761-1761)clock(1781-1781)clock(1867-1867)clock(1896-1896)sinon(3-3)
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(67-140)
🪛 ESLint
ghost/core/core/server/services/email-service/DomainWarmingService.ts
[error] 112-112: Expected '!==' and instead saw '!='.
(eqeqeq)
🔇 Additional comments (11)
ghost/core/core/server/services/email-service/DomainWarmingService.ts (3)
5-27: Type updates for paged email access look consistent.
The Email model and scaling table typing changes align with the new paging API and high-volume config.
30-65: Scaling table updates look coherent and well documented.
The new thresholds and high‑volume cap are clearly captured in the table and comments.
124-138: High‑volume cap + threshold progression logic looks good.
The early high‑volume clamp and ordered thresholds cover the intended ramp behavior.ghost/core/test/unit/server/services/email-service/domain-warming-service.test.ts (4)
10-21: Mock shape update matches the new paging API.
UsingfindPageandfindAllkeeps the unit tests aligned with the service.
66-161: Base-case and null/undefined handling tests look solid.
These cases provide good coverage for the updated paging data shape and edge counts.
163-184: Query-argument assertions are precise.
The test validates filter, ordering, and limit for the paging lookup.
186-226: Progression test cases reflect the new scaling model well.
The table-driven checks give clear coverage for each threshold.ghost/core/test/integration/services/email-service/domain-warming.test.js (4)
52-67: Deterministic time setup is a good test stabilization step.
The fixed base date with midday normalization should reduce flaky date transitions.
192-199: Updated expected-limit calculation matches the new 1.25× ramp.
The conditional assertions align with the revised scaling behavior.
220-241: Multi‑day progression assertions look consistent.
Day‑by‑day expectations correctly mirror the new scaling steps.
290-322: Helper-based scaling checks improve clarity.
The expected-scale helper makes the max‑limit test easier to reason about.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| const result = await this.#emailModel.findPage({ | ||
| filter: `created_at:<=${new Date().toISOString().split('T')[0]}`, | ||
| order: 'csd_email_count DESC', | ||
| limit: 1 |
There was a problem hiding this comment.
Exclude today’s emails per the method contract.
The docstring says “excluding today”, but created_at:<=${today} can include today’s records, which may allow same‑day ramping. Use a strict < (or equivalent) to exclude today.
🔧 Suggested fix
- filter: `created_at:<=${new Date().toISOString().split('T')[0]}`,
+ filter: `created_at:<${new Date().toISOString().split('T')[0]}`,🤖 Prompt for AI Agents
In `@ghost/core/core/server/services/email-service/DomainWarmingService.ts` around
lines 101 - 104, The filter used in DomainWarmingService when calling
this.#emailModel.findPage currently uses `created_at:<=${new
Date().toISOString().split('T')[0]}` which can include today's emails; change
the comparator to a strict less-than so today is excluded (e.g., use
`created_at:<${...}` or equivalent) so the method's "excluding today" contract
is respected when querying via this.#emailModel.findPage.
| const count = result.data[0].get('csd_email_count'); | ||
| return count != null ? count : 0; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "DomainWarmingService.ts" | head -20Repository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 155
🏁 Script executed:
# Check if file exists and read lines 111-112
file_path=$(find . -type f -name "DomainWarmingService.ts" | head -1)
if [ -n "$file_path" ]; then
echo "File found at: $file_path"
sed -n '105,115p' "$file_path"
fiRepository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 369
🏁 Script executed:
# Search for eslint configuration files
find . -type f \( -name ".eslintrc*" -o -name "eslint.config.*" \) | head -10Repository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 455
🏁 Script executed:
# Check package.json for eslint configuration
cat package.json | grep -A 20 "eslintConfig" || echo "No eslintConfig found in package.json"Repository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 121
🏁 Script executed:
# Search for eslint config in ghost/core directory
find ./ghost/core -type f \( -name ".eslintrc*" -o -name "eslint.config.*" \) | head -20Repository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 140
🏁 Script executed:
# Look at the ghost/core directory structure
ls -la ./ghost/core/ | grep -E "eslint|\.eslintrc"Repository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 212
🏁 Script executed:
# Check if there's a root level eslint config that applies to ghost/core
ls -la .eslintrc* 2>/dev/null || echo "No .eslintrc files in root"
cat .eslintignore 2>/dev/null | head -20 || echo "No .eslintignore"Repository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 110
🏁 Script executed:
# Read the root eslint config
cat .eslintrc.js | head -100Repository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 128
🏁 Script executed:
# Read ghost/core eslint config
cat ./ghost/core/.eslintrc.js | head -100Repository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 4069
🏁 Script executed:
# Search for eqeqeq rule in the ghost/core eslint config
grep -n "eqeqeq" ./ghost/core/.eslintrc.jsRepository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 66
🏁 Script executed:
# Check if there's a ghost eslint plugin package
find . -type f -path "*ghost*eslint*" -name "package.json" | head -5Repository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 66
🏁 Script executed:
# Search for eqeqeq rule in the entire codebase
rg "eqeqeq" --type js --type json | head -20Repository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 66
🏁 Script executed:
# Check if there's node_modules and look for ghost eslint plugin
find . -type f -path "*/node_modules/*ghost*eslint*" -name "index.js" | head -5Repository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 66
🏁 Script executed:
# Check the actual eslint dependencies in ghost/core
cat ./ghost/core/package.json | grep -A 5 -B 5 "eslint"Repository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 1637
🏁 Script executed:
# Search more broadly for eqeqeq mentions
rg "eqeqeq" --type-list | head -5
rg "eqeqeq" -t typescript -t javascript -t json 2>/dev/null | head -20Repository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 473
🏁 Script executed:
# Look for eslint plugins in ghost workspace
find ./ghost -type d -name "eslint-plugin*" | head -10Repository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 66
🏁 Script executed:
# Check if there are any ghost related eslint configs/packages
find ./ghost -type f -name "*.js" -path "*/eslint*" | head -20Repository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 66
🏁 Script executed:
# Search for where plugin:ghost/ts is defined
rg "plugin:ghost" --type javascript --type json | head -10Repository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 118
🏁 Script executed:
# Check root package.json for eslint plugin
cat ./package.json | grep -i eslint | head -20Repository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 221
🏁 Script executed:
# Look for eslint-plugin-ghost in node_modules
find . -type f -path "*/node_modules/eslint-plugin-ghost*" -name "*.js" | head -20Repository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 1391
🏁 Script executed:
# Check if eslint-plugin-ghost package is available
ls -la ./node_modules/eslint-plugin-ghost/ 2>/dev/null || echo "eslint-plugin-ghost not in node_modules"Repository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 804
🏁 Script executed:
# Search for the configs provided by eslint-plugin-ghost
rg "plugin:ghost" . --type json | head -10Repository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 269
🏁 Script executed:
# Let's check what the actual eqeqeq rule behavior is - search for any === or != in the file
sed -n '100,120p' ./ghost/core/core/server/services/email-service/DomainWarmingService.tsRepository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 822
🏁 Script executed:
# Check the ts.js config from eslint-plugin-ghost to see if eqeqeq is configured
cat ./node_modules/eslint-plugin-ghost/lib/config/ts.jsRepository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 529
🏁 Script executed:
# Also check shared-rules.js which might have eqeqeq
cat ./node_modules/eslint-plugin-ghost/lib/config/shared-rules.jsRepository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 3345
Fix eqeqeq lint error on null check.
The != operator violates the active eqeqeq rule (set to 'always'). Use nullish coalescing instead to maintain behavior and satisfy the linter.
🔧 Suggested fix
- const count = result.data[0].get('csd_email_count');
- return count != null ? count : 0;
+ const count = result.data[0].get('csd_email_count');
+ return count ?? 0;📝 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 count = result.data[0].get('csd_email_count'); | |
| return count != null ? count : 0; | |
| const count = result.data[0].get('csd_email_count'); | |
| return count ?? 0; |
🧰 Tools
🪛 ESLint
[error] 112-112: Expected '!==' and instead saw '!='.
(eqeqeq)
🤖 Prompt for AI Agents
In `@ghost/core/core/server/services/email-service/DomainWarmingService.ts` around
lines 111 - 112, The null check using the != operator in DomainWarmingService.ts
should be replaced with nullish coalescing to satisfy eqeqeq; change the return
logic around the variable count (from result.data[0].get('csd_email_count')) to
use count ?? 0 (or inline the ?? when assigning) so that null/undefined fall
back to 0 without using !=.
2 participants
Benchmark PR from agentic-review-benchmarks#3
Summary by CodeRabbit
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.