From 6b1ddd8bb838b9df2aa656d5553dad49839049a2 Mon Sep 17 00:00:00 2001 From: Matthew Meszaros Date: Thu, 3 Sep 2026 01:35:24 -0700 Subject: [PATCH] feat: address the CodeRabbit review on the one-time email preset: rate-limit POST /campaigns-estimate as a read, count a mailbox whose sent-today counter fails as having nothing left today instead of untouched, lock the campaign row FOR UPDATE before counting email steps so concurrent inserts cannot give a one-time campaign two messages, keep a plain-text test email free of the HTML signature, and make the wizard's estimate panel say an audience beyond the two-year horizon cannot be projected instead of reading the null as one sending day --- internal/api/routes.go | 2 +- internal/app/campaign/handlers.go | 7 +++--- internal/repository/pg_sequence.go | 3 +++ internal/tasks/test_email.go | 6 ++++- .../app/campaigns/NewCampaignDialog.tsx | 25 +++++++++++++------ 5 files changed, 31 insertions(+), 12 deletions(-) diff --git a/internal/api/routes.go b/internal/api/routes.go index ec22ccef..8a09ff56 100644 --- a/internal/api/routes.go +++ b/internal/api/routes.go @@ -462,7 +462,7 @@ func Run( // Audience-versus-pool projection for the campaign wizard (no // campaign id yet). Read-level: it writes nothing. - protected.POST("/campaigns-estimate", m.RequireOrganization(), m.RequireAccess(models.PermViewCampaigns, models.APIPermReadCampaigns), h.EstimateCampaign) + protected.POST("/campaigns-estimate", m.RateLimitMiddleware(models.RateLimitRead), m.RequireOrganization(), m.RequireAccess(models.PermViewCampaigns, models.APIPermReadCampaigns), h.EstimateCampaign) campaigns := protected.Group("/campaigns") campaigns.Use(m.RateLimitMiddleware(models.RateLimitWrite)) diff --git a/internal/app/campaign/handlers.go b/internal/app/campaign/handlers.go index b9595c09..0f062ad8 100644 --- a/internal/app/campaign/handlers.go +++ b/internal/app/campaign/handlers.go @@ -932,9 +932,10 @@ func (s *campaignService) Estimate(ctx context.Context, orgID uuid.UUID, in *mod out.DailyCapacity += lim sent, err := s.taskRepo.CountCampaignEmailsSentToday(ctx, acct.ID) if err != nil { - // A counter blip must not blank the whole estimate; today then - // reads as untouched, which is the optimistic side. - sent = 0 + // A counter blip must not blank the whole estimate, but it must + // not flatter it either: a mailbox whose sends today are unknown + // contributes nothing to today and only counts from tomorrow. + continue } out.RemainingToday += max(0, lim-sent) } diff --git a/internal/repository/pg_sequence.go b/internal/repository/pg_sequence.go index 4181735c..01b2de71 100644 --- a/internal/repository/pg_sequence.go +++ b/internal/repository/pg_sequence.go @@ -129,9 +129,12 @@ func (r *sequenceRepository) Create(ctx context.Context, userID string, campaign } defer tx.Rollback(ctx) + // FOR UPDATE serialises step creation per campaign, so two concurrent + // inserts on a one-time campaign cannot both see zero email steps. query := ` SELECT user_id, organization_id, kind FROM campaigns WHERE id = $1 + FOR UPDATE ` params := []any{ diff --git a/internal/tasks/test_email.go b/internal/tasks/test_email.go index 5515e8aa..32db3f3c 100644 --- a/internal/tasks/test_email.go +++ b/internal/tasks/test_email.go @@ -54,8 +54,12 @@ func (s *tasksService) SendTestEmail(ctx context.Context, userID string, account subject = "[TEST] " + subject // Add signature if enabled + // Same guard as the campaign send path: a plain-text test must not grow + // an HTML part back through the signature. if account.SignatureSync { - bodyHTML = AddSignature(bodyHTML, account.SignatureHTML, true) + if bodyHTML != "" { + bodyHTML = AddSignature(bodyHTML, account.SignatureHTML, true) + } bodyPlain = AddSignature(bodyPlain, account.SignaturePlain, false) } diff --git a/web/src/components/app/campaigns/NewCampaignDialog.tsx b/web/src/components/app/campaigns/NewCampaignDialog.tsx index 2f9aef4a..75936e71 100644 --- a/web/src/components/app/campaigns/NewCampaignDialog.tsx +++ b/web/src/components/app/campaigns/NewCampaignDialog.tsx @@ -1377,16 +1377,27 @@ function EstimatePanel({ ); tone = "warn"; + } else if (sendingDays === null || finish === null) { + // The backend leaves both null when the audience is not covered + // inside its two-year horizon, which must not read as "one day". + headline = "Longer than this estimate can project"; + detail = ( + <> + {recipients.toLocaleString()} recipients at up to {dailyCapacity.toLocaleString()} a day would take + years.{" "} + + , or narrow the audience. + + ); + tone = "warn"; } else { - const days = sendingDays ?? 0; + const days = sendingDays; headline = days <= 1 - ? finish - ? `Finishes ${startsAt ? "on" : "today,"} ${fmtDate(finish)}` - : "Finishes in one sending day" - : finish - ? `About ${days} sending days, finishing around ${fmtDate(finish)}` - : `More than ${days} sending days`; + ? `Finishes ${startsAt ? "on" : "today,"} ${fmtDate(finish)}` + : `About ${days} sending days, finishing around ${fmtDate(finish)}`; detail = ( <> {recipients.toLocaleString()} recipient{recipients === 1 ? "" : "s"} across {mailboxes} mailbox