Skip to content

[#598] Fix email delivery, email framing, and mobile dashboard entry - #599

Merged
Adr1an04 merged 3 commits into
mainfrom
emailfix
Oct 8, 2026
Merged

Adr1an04 merged 3 commits into
mainfrom
emailfix

Conversation

@Adr1an04

@Adr1an04 Adr1an04 commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

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

  • Serialize subscriber updates and cleanup across processes with PostgreSQL advisory locks, bound provider requests, and retain prepared campaign identity through polling failures.
  • Report partial, paused, bounced, and over-counted sends explicitly; prevent whole-campaign retries after delivery may have started.
  • Use a Forge-owned content-only campaign wrapper, preserving personalization, text alternatives, unsubscribe/browser-view links, and tracking. Explicit custom provider wrappers retain their behavior.
  • Replace Apply with Log into Dashboard, adjust mobile grass/button placement by 10svh, preserve more canopy/robot visibility, and expose the desktop link to assistive technology.
  • Make the email browser test self-contained by seeding a recipient that remains selected.

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

  • Passed: frozen-lockfile install; root format, lint, workspace lint, typecheck, React analysis, and production builds using CI's Node 25.6.1 and pnpm 9.12.1. Lint warnings remain.
  • Passed: all 3,011 unit/integration tests, including 102 email and 1,096 API tests; migration generation with no diff; fresh migration and repeated migration on disposable PostgreSQL 16.
  • Passed: corrected email browser flow (create, publish, schedule, cancel), all 19 archive browser tests with four real Docker builds, and static checks for all 13 Dockerfiles.
  • Delivery verification: a 10,000-message local SMTP capture test produced every expected campaign/recipient pair exactly once. Authorized isolated live tests were inspected separately; SMTP acceptance is not proof of inbox delivery.
  • Provider-rendered email preview inspected at mobile and desktop widths, plus one accepted visual test sent to the authorized directors mailbox without changing subscription state. The latest preview also prevents a tracking pixel from adding an empty line; that cosmetic refinement was not resent.
  • Mobile hero screenshots and keyboard checks cover 320–430px mobile layouts and desktop. The mobile link has a 56px target and 13:1 text contrast.

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:

Email without the provider's white card and gray gutters

Dark email footer with unsubscribe and browser-view links

Mobile hero with visible robot and dashboard link over the grass

Desktop hero

Checklist

  • Database: No schema changes.
  • Environment Variables: No environment variables changed.

Co-authored-by: Codex <codex@openai.com>
@Adr1an04 Adr1an04 added Bug Something isn't working Major Big change - 2+ reviewers required Blade Change modifies code in Blade app Hack Sites Change modifies code in a Hackathon app (ex. 2025) API Change modifies code in the global API/tRPC package labels Oct 8, 2026
@Adr1an04 Adr1an04 self-assigned this Oct 8, 2026
@Adr1an04
Adr1an04 marked this pull request as ready for review October 8, 2026 06:27
Co-authored-by: Codex <codex@openai.com>
@Adr1an04
Adr1an04 enabled auto-merge October 8, 2026 06:27
@coderabbitai

coderabbitai Bot commented Oct 8, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Warning

Review limit reached

You'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.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Repository: KnightHacks/forge/.coderabbit.yml
  • Review profile: QUIET
  • Plan: Advanced
  • Run ID: be4017c0-6b78-4e30-a456-864eceb465c8
📥 Commits

Reviewing files that changed from the base of the PR and between f59d090 and 2c04816.

📒 Files selected for processing (7)
  • .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/status.md
  • packages/api/src/tests/email/delivery.integration.test.ts
  • packages/api/src/tests/email/recipient-lock.test.ts
  • packages/api/src/utils/email/recipient-lock.ts
📝 Walkthrough

Walkthrough

Email 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 f59d0

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)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning 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:… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (7 passed)
Check name Status Explanation
Title check ✅ Passed The title starts with the required issue number, is 68 characters long, and accurately summarizes the email, framing, and mobile dashboard changes.
Description check ✅ Passed The description directly explains the email delivery, email framing, and mobile dashboard changes, including scope and test results.
Linked Issues check ✅ Passed Issue #598 requires coordinated subscriber updates, reliable delivery-state reporting, polling recovery, safe retry behavior, and preserved email controls. recipient-lock.ts, provider.ts, and `del…
Out of Scope Changes check ✅ Passed The changes stay within issue #598 scope: shared email/API delivery code, Blade email tooling, the 2026 hero, and supporting tests and feature documentation. The combined email and hero work is explic…
No Hardcoded Secrets ✅ Passed No hardcoded credential was introduced. The only added token-like literal is LISTMONK_TOKEN: "synthetic-token" in a mocked test environment, paired with example.test values. It is synthetic test d…
Validated Env Access ✅ Passed The pull-request diff adds no direct process.env usage. The changed TypeScript and JavaScript files contain no raw process.env access, so the validated env access condition is not violated.
No Typescript Escape Hatches ✅ Passed No added TypeScript usage of the any type, @ts-ignore, @ts-expect-error, or non-null assertions (!.) appears in the pull-request diff. Added ?. expressions are optional chaining.
Full details: Docstring Coverage

Explanation

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 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

A polled paused campaign can never expire, and updatedAt churn affects ordering.

The paused path leaves status as failed with terminalAt: 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 set terminalAt after a maximum age.

packages/api/src/routers/email.ts (1)

242-242: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win

getSend now reconciles every send that has a campaign, including terminal sends.

getSend now 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 rewrites updatedAt and safeError. Limit the reconcile to the same statuses that the filter in listSends uses.

.forge/features/mobile-dashboard-cta/test-cases.md (1)

3-3: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Update the browser-verification status.

status.md records 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 & Availability

Inspect the connection pool capacity before reducing concurrency.

withRecipientLock can hold one database connection while syncRecipient performs its provider requests. However, the available evidence does not establish the 20 × 25 exhaustion claim or the required pool capacity. The inspected pool configuration only shows new 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 counts to provider-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
📥 Commits

Reviewing files that changed from the base of the PR and between 20dca5a and f59d090.

⛔ Files ignored due to path filters (4)
  • .forge/features/email-delivery-reliability/screenshots/mobile-email.png is excluded by !**/*.png
  • .forge/features/email-delivery-reliability/screenshots/mobile-footer.png is excluded by !**/*.png
  • .forge/features/mobile-dashboard-cta/screenshots/desktop.png is excluded by !**/*.png
  • .forge/features/mobile-dashboard-cta/screenshots/mobile.png is 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.md
  • apps/2026/src/app/_components/sections/hero/Hero.module.css
  • apps/2026/src/app/_components/sections/hero/Hero.tsx
  • apps/2026/src/app/_components/sections/hero/HeroTitle.tsx
  • apps/2026/src/app/page.module.css
  • apps/blade/src/app/_components/admin/email/email-portal-types.ts
  • apps/blade/src/app/_components/admin/email/email-portal-workspace.tsx
  • apps/blade/src/app/_components/admin/email/email-send-status.ts
  • apps/blade/src/tests/admin/email-portal-workspace.test.tsx
  • apps/blade/src/tests/admin/email-send-status.test.ts
  • apps/blade/src/tests/e2e/email-portal.spec.ts
  • packages/api/src/routers/email.ts
  • packages/api/src/tests/email/delivery.integration.test.ts
  • packages/api/src/utils/email/delivery.ts
  • packages/api/src/utils/email/recipient-lock.ts
  • packages/email/src/campaign-content.ts
  • packages/email/src/index.ts
  • packages/email/src/provider.ts
  • packages/email/src/tests/campaign-content.test.ts
  • packages/email/src/tests/delivery-mode.test.ts
  • packages/email/src/tests/http-timeout.test.ts
  • packages/email/src/tests/listmonk-gateway.test.ts
  • packages/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.

Comment thread packages/email/src/provider.ts
Co-authored-by: Codex <codex@openai.com>
@Adr1an04
Adr1an04 disabled auto-merge October 8, 2026 07:02
@Adr1an04
Adr1an04 merged commit b2228c5 into main Oct 8, 2026
12 checks passed
@Adr1an04
Adr1an04 deleted the emailfix branch October 8, 2026 07:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

API Change modifies code in the global API/tRPC package Blade Change modifies code in Blade app Bug Something isn't working Hack Sites Change modifies code in a Hackathon app (ex. 2025) Major Big change - 2+ reviewers required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix email delivery reliability, email framing, and mobile dashboard entry

1 participant