Repository navigation
Conversation
Co-authored-by: Codex <codex@openai.com>
Co-authored-by: Codex <codex@openai.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 35 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (7)
📝 WalkthroughWalkthroughEmail changes coordinate subscriber updates, retain campaign identity after errors, expand campaign reconciliation, and hide Retry when provider delivery may have started. The changes also update email campaign HTML and add provider and database tests. The 2026 homepage changes the mobile hero CTA to “Log into Dashboard,” links it to the configured dashboard route, and adjusts its layout. Priority: ➖ Normal Estimated code review effort: Complex — 45 minutes. Severity of issue fixed: Medium Merge Risk: 🟡 Moderate · up to Resolve database-pool contention and the paused-send retention gap before merging. The remaining confirmed issues cause unnecessary work on old sends and misreport mobile verification progress. 🚥 Pre-merge checks | ✅ 7 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (7 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 4.76% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 21 functions across 20 files. (10 skipped: 10 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (3)
packages/api/src/utils/email/delivery.ts (1)
463-464: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winA polled paused campaign can never expire, and
updatedAtchurn affects ordering.The paused path leaves
statusasfailedwithterminalAt: null. The candidate query keeps polling that row with no end. The retention sweep at Line 560 also never removes it. If the campaign stays paused or is abandoned in Listmonk, the send and its recipient PII are kept past 90 days. Add a time limit to the paused state, or setterminalAtafter a maximum age.packages/api/src/routers/email.ts (1)
242-242: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
getSendnow reconciles every send that has a campaign, including terminal sends.
getSendnow calls Listmonk for every send with a campaign, including completed and cancelled sends. Each detail view of an old send triggers a provider call. Each call also rewritesupdatedAtandsafeError. Limit the reconcile to the same statuses that the filter inlistSendsuses..forge/features/mobile-dashboard-cta/test-cases.md (1)
3-3: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the browser-verification status.
status.mdrecords several browser checks as complete, but also states that full sign-in remains untested. Change the heading to reflect partial completion.Suggested fix
-Status: Manual/browser verification planned +Status: Manual/browser verification partially completed; full sign-in remains untested
🔇 Additional comments (18)
packages/email/src/index.ts (1)
95-116: LGTM!packages/email/src/campaign-content.ts (1)
1-26: LGTM!packages/email/src/tests/campaign-content.test.ts (1)
1-39: LGTM!packages/email/src/tests/delivery-mode.test.ts (1)
20-32: LGTM!packages/email/src/tests/http-timeout.test.ts (1)
1-32: LGTM!packages/email/src/tests/listmonk-gateway.test.ts (1)
52-96: LGTM!packages/email/src/tests/subscriber-concurrency.test.ts (1)
1-162: LGTM!packages/api/src/utils/email/recipient-lock.ts (1)
11-17: LGTM!packages/api/src/tests/email/delivery.integration.test.ts (1)
1-300: LGTM!apps/blade/src/app/_components/admin/email/email-portal-types.ts (1)
26-26: LGTM!apps/blade/src/app/_components/admin/email/email-portal-workspace.tsx (1)
1408-1420: LGTM!apps/blade/src/app/_components/admin/email/email-send-status.ts (1)
15-15: LGTM!apps/blade/src/tests/admin/email-portal-workspace.test.tsx (1)
25-46: LGTM!apps/blade/src/tests/admin/email-send-status.test.ts (1)
46-46: LGTM!apps/blade/src/tests/e2e/email-portal.spec.ts (1)
21-23: LGTM!Also applies to: 67-69, 93-98, 118-120, 139-153
packages/email/src/provider.ts (1)
695-703: 🩺 Stability & AvailabilityInspect the connection pool capacity before reducing concurrency.
withRecipientLockcan hold one database connection whilesyncRecipientperforms its provider requests. However, the available evidence does not establish the20 × 25exhaustion claim or the required pool capacity. The inspected pool configuration only showsnew Pool({ connectionString: env.DATABASE_URL }); it does not show the effective pool limit..forge/features/email-delivery-reliability/srd.md-13-13 (1)
13-13: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
⚠️ Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.Hyphenate the compound modifier. Change
provider sent countstoprovider-sent counts.Source: Linters/SAST tools
.forge/features/email-delivery-reliability/status.md-45-45 (1)
45-45: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
⚠️ Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.Vary the repeated sentence openings. Three consecutive sentences begin with
All; revise one opening.Source: Linters/SAST tools
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: KnightHacks/forge/.coderabbit.yml
- Review profile: QUIET
- Plan: Advanced
- Run ID:
507bfe55-1d5e-43a8-95b3-cc58663041d1
⛔ Files ignored due to path filters (4)
.forge/features/email-delivery-reliability/screenshots/mobile-email.pngis excluded by!**/*.png.forge/features/email-delivery-reliability/screenshots/mobile-footer.pngis excluded by!**/*.png.forge/features/mobile-dashboard-cta/screenshots/desktop.pngis excluded by!**/*.png.forge/features/mobile-dashboard-cta/screenshots/mobile.pngis excluded by!**/*.png
📒 Files selected for processing (30)
.forge/features/email-delivery-reliability/spec.md.forge/features/email-delivery-reliability/srd.md.forge/features/email-delivery-reliability/status.md.forge/features/email-delivery-reliability/test-cases.md.forge/features/mobile-dashboard-cta/spec.md.forge/features/mobile-dashboard-cta/srd.md.forge/features/mobile-dashboard-cta/status.md.forge/features/mobile-dashboard-cta/test-cases.mdapps/2026/src/app/_components/sections/hero/Hero.module.cssapps/2026/src/app/_components/sections/hero/Hero.tsxapps/2026/src/app/_components/sections/hero/HeroTitle.tsxapps/2026/src/app/page.module.cssapps/blade/src/app/_components/admin/email/email-portal-types.tsapps/blade/src/app/_components/admin/email/email-portal-workspace.tsxapps/blade/src/app/_components/admin/email/email-send-status.tsapps/blade/src/tests/admin/email-portal-workspace.test.tsxapps/blade/src/tests/admin/email-send-status.test.tsapps/blade/src/tests/e2e/email-portal.spec.tspackages/api/src/routers/email.tspackages/api/src/tests/email/delivery.integration.test.tspackages/api/src/utils/email/delivery.tspackages/api/src/utils/email/recipient-lock.tspackages/email/src/campaign-content.tspackages/email/src/index.tspackages/email/src/provider.tspackages/email/src/tests/campaign-content.test.tspackages/email/src/tests/delivery-mode.test.tspackages/email/src/tests/http-timeout.test.tspackages/email/src/tests/listmonk-gateway.test.tspackages/email/src/tests/subscriber-concurrency.test.ts
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
Co-authored-by: Codex <codex@openai.com>
Why
Concurrent campaigns could overwrite shared subscriber data and list memberships, while incomplete sends and polling failures could be reported incorrectly or leave delivery stranded. Forge's complete HTML documents were also nested inside Listmonk's default white card and gray gutters. The 2026 mobile hero needed a readable dashboard link and adjusted foreground placement.
What
Closes: #598
The email/API/Blade work and 2026 hero changes share this branch at the requester's explicit direction. No schema, dependency, environment, or SMTP configuration changes. Deploy Blade/API and cron together so all subscriber writers participate in the lock.
Test Plan
Broader E2E is not fully green: the complete Blade run had 63 passing, 19 failing, and 31 unrun tests; its email fixture was subsequently corrected and passed independently. Remaining failures concern unrelated resume-storage prerequisites, UI expectations, responsive flows, and eight visual baselines. Guild passed three tests but its team-filter test requires seeded team data. The 2026 and Club E2E scripts discover Vitest files without a Playwright configuration. These unrelated issues remain out of scope. Hosted CI is pending this draft PR; production deployment/migrations have not been run.
Screenshots
Browser previews, not screenshots of recipient inboxes:
Desktop hero
Checklist