From c28ef3e82aaa0afd6f1adea8fa86f334cf55cb93 Mon Sep 17 00:00:00 2001 From: Matthew Meszaros Date: Fri, 28 Aug 2026 21:26:57 -0700 Subject: [PATCH 1/9] feat: keep a restricted or suspended workspace out of the premium warmup pool by checking the organization's risk posture before the stored warmup_pool_type in resolveWarmupPoolType (tasks and email services), which is never empty so the restricted branch below it was dead code, make the scheduler's recipient-capacity count risk-aware the same way, wire the org risk repository into the email service, and refuse the premium membership move in UpdateEmailAccountWarmupPoolType while the owning organization is restricted so a worker rebalance cannot readmit a risky tenant --- cmd/backend/main.go | 4 + internal/app/email/handler.go | 24 +++- internal/app/email/service.go | 17 +++ internal/app/email/warmup_pool_type_test.go | 59 ++++++++++ internal/repository/pg_worker.go | 12 +- internal/scheduler/warmup_pool_type_test.go | 66 +++++++++++ internal/scheduler/warmup_scheduler.go | 15 ++- internal/tasks/email_task.go | 10 +- internal/tasks/warmup_pool_type_test.go | 116 ++++++++++++++++++++ 9 files changed, 312 insertions(+), 11 deletions(-) create mode 100644 internal/app/email/warmup_pool_type_test.go create mode 100644 internal/scheduler/warmup_pool_type_test.go create mode 100644 internal/tasks/warmup_pool_type_test.go diff --git a/cmd/backend/main.go b/cmd/backend/main.go index e99dfa78..de2237d7 100644 --- a/cmd/backend/main.go +++ b/cmd/backend/main.go @@ -1148,6 +1148,10 @@ func main() { if instanceSettings != nil { emailService.WireSyncBudget(instanceSettings) } + // A restricted organization's mailboxes join the free pool on connect. + if aware, ok := emailService.(email.OrgRiskAware); ok { + aware.WireOrgRisk(orgRiskRepository) + } analyticsRepository := repository.NewAnalyticsRepository(primaryDB) emailAccountErrorRepository := repository.NewEmailAccountErrorRepository(primaryDB) analyticsService = analytics.NewService(analyticsRepository, emailRepostory, campaignRepostory, emailAccountErrorRepository, warmupRepository) diff --git a/internal/app/email/handler.go b/internal/app/email/handler.go index 962414c3..adc0fd70 100644 --- a/internal/app/email/handler.go +++ b/internal/app/email/handler.go @@ -359,18 +359,36 @@ func (s *emailService) canUseWarmupPool(ctx context.Context, account *models.Ema return err == nil && canWarmup } +// orgSuspendedOrRestricted reports whether the workspace's posture bars the +// paid warmup pool. Fails open, like every other risk read. +func (s *emailService) orgSuspendedOrRestricted(ctx context.Context, orgID uuid.UUID) bool { + if s.orgRiskRepo == nil { + return false + } + states, err := s.orgRiskRepo.GetOrgRiskStates(ctx, []uuid.UUID{orgID}) + if err != nil { + return false + } + return states[orgID].ForcesFreeWarmupPool() +} + func (s *emailService) resolveWarmupPoolType(ctx context.Context, account *models.Email) string { if account == nil { return "premium" } - if account.WarmupPoolType != "" { - return account.WarmupPoolType - } // No organization means no entitlement to check, so the mailbox gets the // lower-trust pool rather than defaulting into the paid one. if account.OrganizationID == nil { return "free" } + // A restricted organization warms in the free pool whatever it pays; the + // stored tier is checked after, since it is always set (issue #242). + if s.orgSuspendedOrRestricted(ctx, *account.OrganizationID) { + return "free" + } + if account.WarmupPoolType != "" { + return account.WarmupPoolType + } if s.featureGate != nil { isPaid, err := s.featureGate.IsPaidOrganization(ctx, *account.OrganizationID) if err == nil && !isPaid { diff --git a/internal/app/email/service.go b/internal/app/email/service.go index 13f74976..1b41df63 100644 --- a/internal/app/email/service.go +++ b/internal/app/email/service.go @@ -108,8 +108,25 @@ type emailService struct { // (email_account.connected, email_account.removed) are dispatched to // subscribed customer webhooks. webhookService webhook.Service + // orgRiskRepo bars a restricted organization from the paid warmup pool. + // Optional/nil-safe. + orgRiskRepo repository.OrgRiskRepository } +// WireOrgRisk attaches the organization risk posture. +func (s *emailService) WireOrgRisk(r repository.OrgRiskRepository) { + s.orgRiskRepo = r +} + +// OrgRiskAware is the optional capability the caller uses to attach org risk. +type OrgRiskAware interface { + WireOrgRisk(r repository.OrgRiskRepository) +} + +// The backend attaches the posture through a type assertion, so a silently +// unsatisfied interface would leave restricted workspaces in the paid pool. +var _ OrgRiskAware = (*emailService)(nil) + // SyncBudgetSource is the operator-editable sync fair-use section, satisfied // by instancesettings.Service. Injected post-construction; when unset the // loader ships compiled defaults. diff --git a/internal/app/email/warmup_pool_type_test.go b/internal/app/email/warmup_pool_type_test.go new file mode 100644 index 00000000..6128daa5 --- /dev/null +++ b/internal/app/email/warmup_pool_type_test.go @@ -0,0 +1,59 @@ +package email + +import ( + "context" + "errors" + "testing" + + "github.com/google/uuid" + + "github.com/warmbly/warmbly/internal/models" + "github.com/warmbly/warmbly/internal/repository" +) + +type poolTypeRiskRepo struct { + repository.OrgRiskRepository + states map[uuid.UUID]models.OrgRiskState + err error +} + +func (r *poolTypeRiskRepo) GetOrgRiskStates(_ context.Context, _ []uuid.UUID) (map[uuid.UUID]models.OrgRiskState, error) { + return r.states, r.err +} + +// A mailbox connected under a restricted workspace joins the free pool, not the +// tier its worker assignment wrote (issue #242). +func TestResolveWarmupPoolTypeDemotesARestrictedOrganization(t *testing.T) { + org := uuid.New() + account := &models.Email{ID: uuid.New(), OrganizationID: &org, Status: "active", WarmupPoolType: "premium"} + + for _, tc := range []struct { + name string + risk *poolTypeRiskRepo + wants string + }{ + {"restricted", &poolTypeRiskRepo{states: map[uuid.UUID]models.OrgRiskState{org: models.OrgRiskRestricted}}, "free"}, + {"suspended", &poolTypeRiskRepo{states: map[uuid.UUID]models.OrgRiskState{org: models.OrgRiskSuspended}}, "free"}, + {"trusted", &poolTypeRiskRepo{states: map[uuid.UUID]models.OrgRiskState{org: models.OrgRiskTrusted}}, "premium"}, + {"unknown org", &poolTypeRiskRepo{states: map[uuid.UUID]models.OrgRiskState{}}, "premium"}, + {"lookup failure fails open", &poolTypeRiskRepo{err: errors.New("connection reset by peer")}, "premium"}, + } { + t.Run(tc.name, func(t *testing.T) { + svc := &emailService{orgRiskRepo: tc.risk} + if got := svc.resolveWarmupPoolType(context.Background(), account); got != tc.wants { + t.Fatalf("resolved %q, want %q", got, tc.wants) + } + }) + } +} + +// Without the risk repository wired (jobs, tests) the stored tier stands. +func TestResolveWarmupPoolTypeWithoutRiskUsesTheStoredTier(t *testing.T) { + org := uuid.New() + svc := &emailService{} + account := &models.Email{ID: uuid.New(), OrganizationID: &org, Status: "active", WarmupPoolType: "premium"} + + if got := svc.resolveWarmupPoolType(context.Background(), account); got != "premium" { + t.Fatalf("resolved %q, want premium", got) + } +} diff --git a/internal/repository/pg_worker.go b/internal/repository/pg_worker.go index 81ae7d55..535cdfbc 100644 --- a/internal/repository/pg_worker.go +++ b/internal/repository/pg_worker.go @@ -496,6 +496,9 @@ func (r *workerRepository) ClearEmailAccountWorker(ctx context.Context, emailAcc // UpdateEmailAccountWarmupPoolType writes the tier and moves the mailbox's pool membership to // match in one transaction: they record the same fact, and updating only the column left // downgraded mailboxes in the premium pool (issue #211). A mailbox in no pool stays in none. +// The membership move into premium is refused while the owning organization is restricted or +// suspended, so a worker rebalance cannot readmit a risky tenant to the paid pool (issue #242). +// The tier column is still written: it records what the workspace pays for, not where it warms. func (r *workerRepository) UpdateEmailAccountWarmupPoolType(ctx context.Context, emailAccountID uuid.UUID, poolType string) error { tx, err := r.db.Begin(ctx) if err != nil { @@ -515,7 +518,14 @@ func (r *workerRepository) UpdateEmailAccountWarmupPoolType(ctx context.Context, FROM warmup_pools wp WHERE wp.pool_type = $1::warmup_pool_type AND wpp.email_account_id = $2::uuid - AND wpp.pool_id <> wp.id`, + AND wpp.pool_id <> wp.id + AND ($1::text <> 'premium' OR NOT EXISTS ( + SELECT 1 + FROM email_accounts ea + JOIN organizations o ON o.id = ea.organization_id + WHERE ea.id = $2::uuid + AND o.risk_state IN ('restricted', 'suspended') + ))`, poolType, emailAccountID); err != nil { return err } diff --git a/internal/scheduler/warmup_pool_type_test.go b/internal/scheduler/warmup_pool_type_test.go new file mode 100644 index 00000000..2cfabcda --- /dev/null +++ b/internal/scheduler/warmup_pool_type_test.go @@ -0,0 +1,66 @@ +package scheduler + +import ( + "context" + "errors" + "testing" + + "github.com/google/uuid" + + "github.com/warmbly/warmbly/internal/models" + "github.com/warmbly/warmbly/internal/repository" +) + +type poolTypeRiskRepo struct { + repository.OrgRiskRepository + states map[uuid.UUID]models.OrgRiskState + err error +} + +func (r *poolTypeRiskRepo) GetOrgRiskStates(_ context.Context, _ []uuid.UUID) (map[uuid.UUID]models.OrgRiskState, error) { + return r.states, r.err +} + +// The recipient-capacity count has to be taken against the pool the mailbox +// will actually send into, or a restricted mailbox sizes its day off the +// premium pool it is no longer in (issue #242). +func TestWarmupPoolTypeForAccountFollowsTheOrganizationsPosture(t *testing.T) { + org := uuid.New() + account := &models.Email{ID: uuid.New(), OrganizationID: &org, WarmupPoolType: "premium"} + + for _, tc := range []struct { + name string + risk *poolTypeRiskRepo + want string + }{ + {"restricted", &poolTypeRiskRepo{states: map[uuid.UUID]models.OrgRiskState{org: models.OrgRiskRestricted}}, "free"}, + {"suspended", &poolTypeRiskRepo{states: map[uuid.UUID]models.OrgRiskState{org: models.OrgRiskSuspended}}, "free"}, + {"watch", &poolTypeRiskRepo{states: map[uuid.UUID]models.OrgRiskState{org: models.OrgRiskWatch}}, "premium"}, + {"trusted", &poolTypeRiskRepo{states: map[uuid.UUID]models.OrgRiskState{org: models.OrgRiskTrusted}}, "premium"}, + {"lookup failure fails open", &poolTypeRiskRepo{err: errors.New("connection reset by peer")}, "premium"}, + } { + t.Run(tc.name, func(t *testing.T) { + s := &schedulerService{orgRiskRepo: tc.risk} + if got := s.warmupPoolTypeForAccount(context.Background(), account); got != tc.want { + t.Fatalf("resolved %q, want %q", got, tc.want) + } + }) + } +} + +// Without the risk repository wired the stored tier stands, and a mailbox with +// no tier recorded keeps the historical premium default. +func TestWarmupPoolTypeForAccountWithoutRiskWired(t *testing.T) { + s := &schedulerService{} + org := uuid.New() + + if got := s.warmupPoolTypeForAccount(context.Background(), &models.Email{OrganizationID: &org, WarmupPoolType: "free"}); got != "free" { + t.Fatalf("resolved %q, want free", got) + } + if got := s.warmupPoolTypeForAccount(context.Background(), &models.Email{OrganizationID: &org}); got != "premium" { + t.Fatalf("resolved %q, want premium", got) + } + if got := s.warmupPoolTypeForAccount(context.Background(), nil); got != "premium" { + t.Fatalf("nil account resolved %q, want premium", got) + } +} diff --git a/internal/scheduler/warmup_scheduler.go b/internal/scheduler/warmup_scheduler.go index 20a6b8fc..24c815db 100644 --- a/internal/scheduler/warmup_scheduler.go +++ b/internal/scheduler/warmup_scheduler.go @@ -47,8 +47,17 @@ func adjustmentFor(state models.WarmupHealthState) healthAdjustment { } } -func warmupPoolTypeForAccount(account *models.Email) string { - if account != nil && account.WarmupPoolType != "" { +// warmupPoolTypeForAccount is the pool the mailbox actually warms in. A +// restricted organization is held in the free pool whatever tier its mailbox +// carries, matching the task service's resolution (issue #242). +func (s *schedulerService) warmupPoolTypeForAccount(ctx context.Context, account *models.Email) string { + if account == nil { + return "premium" + } + if s.orgRiskState(ctx, account.OrganizationID).ForcesFreeWarmupPool() { + return "free" + } + if account.WarmupPoolType != "" { return account.WarmupPoolType } return "premium" @@ -232,7 +241,7 @@ func (s *schedulerService) CalculateNextWarmupTime(ctx context.Context, accountI // here, so operators can add inbound capacity without making those // mailboxes warmup senders. if s.warmupRepo != nil { - eligibleRecipients, err := s.warmupRepo.CountEligibleRecipients(ctx, warmupPoolTypeForAccount(account), accountID) + eligibleRecipients, err := s.warmupRepo.CountEligibleRecipients(ctx, s.warmupPoolTypeForAccount(ctx, account), accountID) if err == nil { if eligibleRecipients <= 0 { return recipientRecheckTime(), nil diff --git a/internal/tasks/email_task.go b/internal/tasks/email_task.go index dd4e788c..aa699873 100644 --- a/internal/tasks/email_task.go +++ b/internal/tasks/email_task.go @@ -739,9 +739,6 @@ func (s *tasksService) resolveWarmupPoolType(ctx context.Context, account *Email if account == nil { return "premium" } - if account.WarmupPoolType != "" { - return account.WarmupPoolType - } // No organization means no entitlement to check, so the mailbox gets the // lower-trust pool rather than defaulting into the paid one. if account.OrganizationID == nil { @@ -749,10 +746,15 @@ func (s *tasksService) resolveWarmupPoolType(ctx context.Context, account *Email } // A restricted organization leaves the paid pool whatever it pays: the // shared reputation paying customers depend on is not for spending on a - // risky tenant. + // risky tenant. Checked before the stored tier, which is always set + // (schema default 'free', worker assignment writes it) and so would + // otherwise short-circuit this gate (issue #242). if s.orgSuspendedOrRestricted(ctx, *account.OrganizationID) { return "free" } + if account.WarmupPoolType != "" { + return account.WarmupPoolType + } if s.featureGate != nil { isPaid, xerr := s.featureGate.IsPaidOrganization(ctx, *account.OrganizationID) if xerr == nil && !isPaid { diff --git a/internal/tasks/warmup_pool_type_test.go b/internal/tasks/warmup_pool_type_test.go new file mode 100644 index 00000000..791a3d82 --- /dev/null +++ b/internal/tasks/warmup_pool_type_test.go @@ -0,0 +1,116 @@ +package tasks + +import ( + "context" + "errors" + "testing" + + "github.com/google/uuid" + + "github.com/warmbly/warmbly/internal/errx" + "github.com/warmbly/warmbly/internal/models" + "github.com/warmbly/warmbly/internal/repository" +) + +type poolTypeRiskRepo struct { + repository.OrgRiskRepository + states map[uuid.UUID]models.OrgRiskState + err error +} + +func (r *poolTypeRiskRepo) GetOrgRiskStates(_ context.Context, _ []uuid.UUID) (map[uuid.UUID]models.OrgRiskState, error) { + return r.states, r.err +} + +type poolTypeGate struct { + poolReconcileGate + paid map[uuid.UUID]bool +} + +func (g *poolTypeGate) IsPaidOrganization(_ context.Context, orgID uuid.UUID) (bool, *errx.Error) { + return g.paid[orgID], nil +} + +func poolTypeAccount(orgID uuid.UUID, tier string) *Email { + return &Email{ID: uuid.New(), OrganizationID: &orgID, Status: "active", WarmupPoolType: tier} +} + +// The headline defect (issue #242): the stored tier is always set, so checking +// it first meant a restricted organization never left the premium pool. +func TestResolveWarmupPoolTypeDemotesARestrictedOrganizationWhateverItsTier(t *testing.T) { + for _, state := range []models.OrgRiskState{models.OrgRiskRestricted, models.OrgRiskSuspended} { + t.Run(string(state), func(t *testing.T) { + org := uuid.New() + svc := &tasksService{orgRiskRepo: &poolTypeRiskRepo{states: map[uuid.UUID]models.OrgRiskState{org: state}}} + + if got := svc.resolveWarmupPoolType(context.Background(), poolTypeAccount(org, "premium")); got != "free" { + t.Fatalf("resolved %q for a %s organization, want free", got, state) + } + }) + } +} + +// Lifting the restriction hands the mailbox its paid tier back on the next resolution. +func TestResolveWarmupPoolTypeKeepsTheStoredTierForATrustedOrganization(t *testing.T) { + org := uuid.New() + svc := &tasksService{orgRiskRepo: &poolTypeRiskRepo{states: map[uuid.UUID]models.OrgRiskState{org: models.OrgRiskWatch}}} + + if got := svc.resolveWarmupPoolType(context.Background(), poolTypeAccount(org, "premium")); got != "premium" { + t.Fatalf("resolved %q, want the stored premium tier", got) + } + if got := svc.resolveWarmupPoolType(context.Background(), poolTypeAccount(org, "free")); got != "free" { + t.Fatalf("resolved %q, want the stored free tier", got) + } +} + +// An unreadable posture must not demote: the risk read fails open everywhere else too. +func TestResolveWarmupPoolTypeFailsOpenWhenRiskCannotBeRead(t *testing.T) { + org := uuid.New() + svc := &tasksService{orgRiskRepo: &poolTypeRiskRepo{err: errors.New("connection reset by peer")}} + + if got := svc.resolveWarmupPoolType(context.Background(), poolTypeAccount(org, "premium")); got != "premium" { + t.Fatalf("resolved %q on a risk lookup failure, want the stored tier", got) + } +} + +// With no stored tier the subscription decides, and no organization means free. +func TestResolveWarmupPoolTypeFallsBackToEntitlementWithoutAStoredTier(t *testing.T) { + paid, trial := uuid.New(), uuid.New() + svc := &tasksService{featureGate: &poolTypeGate{paid: map[uuid.UUID]bool{paid: true, trial: false}}} + + if got := svc.resolveWarmupPoolType(context.Background(), poolTypeAccount(paid, "")); got != "premium" { + t.Fatalf("paid organization resolved %q, want premium", got) + } + if got := svc.resolveWarmupPoolType(context.Background(), poolTypeAccount(trial, "")); got != "free" { + t.Fatalf("trial organization resolved %q, want free", got) + } + if got := svc.resolveWarmupPoolType(context.Background(), &Email{ID: uuid.New(), WarmupPoolType: "premium"}); got != "free" { + t.Fatalf("mailbox with no organization resolved %q, want free", got) + } +} + +// The reconciler is what moves a mailbox that is not actively warming (a +// recipient-only member, say), so restriction has to reach it there too. +func TestPoolReconcileMovesARestrictedOrganizationToTheFreePool(t *testing.T) { + f := newPoolReconcileFixture() + restricted := f.mailbox(f.orgPaid, "premium", "premium") + participants := []uuid.UUID{restricted} + f.svc = &tasksService{ + warmupRepo: &poolReconcileWarmupRepo{participants: participants}, + emailRepo: f.emails, + featureGate: f.gate, + warmupHealth: f.warmup, + orgRiskRepo: &poolTypeRiskRepo{states: map[uuid.UUID]models.OrgRiskState{f.orgPaid: models.OrgRiskRestricted}}, + } + + moved, removed, err := f.svc.ReconcileWarmupPoolMembership(context.Background()) + if err != nil { + t.Fatalf("reconcile: %v", err) + } + if moved != 1 || removed != 0 { + t.Fatalf("moved %d removed %d, want 1 and 0", moved, removed) + } + if f.warmup.moved[restricted] != "free" { + t.Fatalf("moved to %q, want free", f.warmup.moved[restricted]) + } +} From f518c950d7a7f189d60412119c8f8c23cc3b3c7e Mon Sep 17 00:00:00 2001 From: Matthew Meszaros Date: Fri, 28 Aug 2026 21:30:52 -0700 Subject: [PATCH 2/9] feat: stop an unattributable warmup spam placement demoting every custom-domain partner, key the admin pool rollup to who runs the recipient's mail rather than the connect method, and cover issue #143's routing feedback end to end with a live selectWarmupPartner test that proves the per-provider signal reaches the partner the selector returns --- internal/models/warmup.go | 6 +- internal/repository/pg_warmup.go | 25 +- .../warmup_provider_routing_live_test.go | 90 ++++++- .../tasks/warmup_partner_routing_live_test.go | 220 ++++++++++++++++++ 4 files changed, 324 insertions(+), 17 deletions(-) create mode 100644 internal/tasks/warmup_partner_routing_live_test.go diff --git a/internal/models/warmup.go b/internal/models/warmup.go index 66f3699a..ef26941e 100644 --- a/internal/models/warmup.go +++ b/internal/models/warmup.go @@ -108,9 +108,9 @@ type WarmupPoolHealthSummary struct { ByState map[string]int `json:"by_state"` AvgSpamScore float64 `json:"avg_spam_score"` AvgSpamPlacement float64 `json:"avg_spam_placement_rate"` - // SpamPlacementByProvider breaks recent spam-placement counts down by the - // recipient provider so the admin can see where warmup mail is being - // filtered (e.g. mostly at Outlook vs Gmail) rather than one flat rate. + // SpamPlacementByProvider breaks recent spam-placement counts down by who + // runs the recipient's mail, the same vocabulary partner routing reads, so + // the admin sees where warmup mail is filtered rather than one flat rate. SpamPlacementByProvider map[string]int `json:"spam_placement_by_provider"` BlockedCount int `json:"blocked_count"` AtRiskCount int `json:"at_risk_count"` diff --git a/internal/repository/pg_warmup.go b/internal/repository/pg_warmup.go index a42558ff..c736a2bf 100644 --- a/internal/repository/pg_warmup.go +++ b/internal/repository/pg_warmup.go @@ -862,12 +862,13 @@ func (r *warmupRepository) PoolSpamPlacementRate(ctx context.Context, since time return float64(placements) / float64(sent) * 100, nil } -// PoolSpamPlacementsByProvider returns spam-placement counts grouped by the -// recipient provider over the window, so the admin overview can show where -// warmup mail is being filtered (e.g. mostly at Outlook vs Gmail). +// PoolSpamPlacementsByProvider returns spam-placement counts over the window +// keyed by who RUNS the recipient's mail, the same vocabulary partner routing +// reads. The stored recipient_provider column is the connect method, which +// files every custom-domain Microsoft 365 mailbox under smtp_imap. func (r *warmupRepository) PoolSpamPlacementsByProvider(ctx context.Context, since time.Time) (map[string]int, error) { query := ` - SELECT COALESCE(NULLIF(recipient_provider, ''), 'unknown'), COUNT(*) + SELECT recipient_domain, COUNT(*) FROM warmup_spam_reports WHERE report_type = 'spam_placement' AND created_at >= $1 GROUP BY 1 @@ -880,12 +881,18 @@ func (r *warmupRepository) PoolSpamPlacementsByProvider(ctx context.Context, sin out := make(map[string]int) for rows.Next() { - var provider string + var domain string var n int - if err := rows.Scan(&provider, &n); err != nil { + if err := rows.Scan(&domain, &n); err != nil { return nil, err } - out[provider] = n + // Unattributable rows stay their own bucket rather than inflating + // custom, which would read as a real provider surface. + key := "unknown" + if domain != "" { + key = string(models.ClassifyProvider(domain)) + } + out[key] += n } return out, rows.Err() } @@ -938,10 +945,14 @@ func (r *warmupRepository) SenderPlacementByProvider(ctx context.Context, sender return nil, err } + // A blank recipient domain is unattributable, and the send side can never + // produce one, so counting it would charge a numerator with no denominator + // to ProviderCustom and demote every custom-domain partner for it. placementRows, err := r.db.Query(ctx, ` SELECT recipient_domain, COUNT(*) FROM warmup_spam_reports WHERE reported_account_id = $1 AND report_type = 'spam_placement' AND created_at >= $2 + AND recipient_domain <> '' GROUP BY 1 `, senderAccountID, since) if err != nil { diff --git a/internal/repository/warmup_provider_routing_live_test.go b/internal/repository/warmup_provider_routing_live_test.go index e0e90672..a0f3fbb8 100644 --- a/internal/repository/warmup_provider_routing_live_test.go +++ b/internal/repository/warmup_provider_routing_live_test.go @@ -27,6 +27,7 @@ type providerRoutingFixture struct { sender uuid.UUID atGoogle uuid.UUID atMSGraph uuid.UUID + atCustom uuid.UUID } func newProviderRoutingFixture(t *testing.T) *providerRoutingFixture { @@ -41,7 +42,7 @@ func newProviderRoutingFixture(t *testing.T) *providerRoutingFixture { f := &providerRoutingFixture{ pool: pool, user: uuid.New(), org: uuid.New(), - sender: uuid.New(), atGoogle: uuid.New(), atMSGraph: uuid.New(), + sender: uuid.New(), atGoogle: uuid.New(), atMSGraph: uuid.New(), atCustom: uuid.New(), } exec := func(sql string, args ...any) { t.Helper() @@ -65,15 +66,16 @@ func newProviderRoutingFixture(t *testing.T) *providerRoutingFixture { {f.sender, "smtp_imap", "test.local"}, {f.atGoogle, "gmail", "gmail.com"}, {f.atMSGraph, "smtp_imap", "outlook.com"}, + {f.atCustom, "smtp_imap", "acme.test"}, } { exec(`INSERT INTO email_accounts (id, user_id, organization_id, email, name, signature_plain, signature_html, provider, status, campaign_limit, min_wait_time, timezone) VALUES ($1, $2, $3, $4, 'Route', '', '', $5, 'active', 50, 600, 'UTC')`, m.id, f.user, f.org, "route-"+m.id.String()[:8]+"@"+m.domain, m.provider) } - // Only the two recipients join the pool; the sender does not, so the - // participants map must contain exactly them. - for _, id := range []uuid.UUID{f.atGoogle, f.atMSGraph} { + // Only the recipients join the pool; the sender does not, so it must never + // appear in the participants map. + for _, id := range []uuid.UUID{f.atGoogle, f.atMSGraph, f.atCustom} { exec(`INSERT INTO warmup_pool_participants (pool_id, email_account_id, participant_role, health_state) VALUES ($1, $2, 'sender_receiver', 'healthy')`, premiumPoolID, id) } @@ -121,13 +123,20 @@ func (f *providerRoutingFixture) send(t *testing.T, recipient uuid.UUID, n int, } func (f *providerRoutingFixture) placement(t *testing.T, domain string, n int) { + t.Helper() + f.placementFrom(t, "smtp_imap", domain, n) +} + +// placementFrom writes the row the way the consumer does: recipient_provider is +// the connect method the mailbox uses, recipient_domain is who its mail is at. +func (f *providerRoutingFixture) placementFrom(t *testing.T, connectMethod, domain string, n int) { t.Helper() for i := 0; i < n; i++ { if _, err := f.pool.Exec(context.Background(), `INSERT INTO warmup_spam_reports (id, reporter_account_id, reported_account_id, message_id, - report_type, recipient_domain, created_at) - VALUES (gen_random_uuid(), $1, $1, $2, 'spam_placement', $3, NOW())`, - f.sender, "m-"+uuid.New().String(), domain); err != nil { + report_type, recipient_provider, recipient_domain, created_at) + VALUES (gen_random_uuid(), $1, $1, $2, 'spam_placement', $3, $4, NOW())`, + f.sender, "m-"+uuid.New().String(), connectMethod, domain); err != nil { t.Fatalf("insert placement: %v", err) } } @@ -222,3 +231,70 @@ func TestLiveProviderRoutingIgnoresFailedSends(t *testing.T) { t.Errorf("rate = %v, want 0.5; counting the failures would have read 0.05", m.Rate()) } } + +// An unattributed placement (the recipient account could not be resolved when +// it was recorded) has no domain, so it belongs to no provider. Charging it to +// ProviderCustom would demote every custom-domain partner for a failure that +// was never theirs, and the send side can never produce a blank domain to put +// underneath it. +func TestLiveProviderRoutingIgnoresUnattributedPlacement(t *testing.T) { + handle, _ := liveContactDB(t) + f := newProviderRoutingFixture(t) + repo := NewWarmupRepository(handle.Pool) + + f.send(t, f.atCustom, 10, "completed") + f.placement(t, "", 5) + + got, err := repo.SenderPlacementByProvider(context.Background(), f.sender, time.Now().Add(-24*time.Hour)) + if err != nil { + t.Fatalf("SenderPlacementByProvider: %v", err) + } + c := got["custom"] + if c.Sends != 10 { + t.Errorf("sends = %d, want the 10 that went to the custom domain", c.Sends) + } + if c.Placements != 0 || c.Rate() != 0 { + t.Errorf("custom = %+v (rate %v), want the domainless placements ignored", c, c.Rate()) + } +} + +// The admin overview has to name the same providers routing does. The stored +// recipient_provider column is the connect method, which files a custom-domain +// Microsoft 365 mailbox under smtp_imap: the bucket an operator most needs +// split, and one routing never uses. +func TestLivePoolPlacementsUseTheRoutingVocabulary(t *testing.T) { + handle, _ := liveContactDB(t) + f := newProviderRoutingFixture(t) + repo := NewWarmupRepository(handle.Pool) + ctx := context.Background() + since := time.Now().Add(-24 * time.Hour) + + // The rollup is pool-wide, so measure what these rows added to it. + before, err := repo.PoolSpamPlacementsByProvider(ctx, since) + if err != nil { + t.Fatalf("PoolSpamPlacementsByProvider: %v", err) + } + + f.placementFrom(t, "smtp_imap", "outlook.com", 3) + f.placementFrom(t, "gmail", "", 2) + + after, err := repo.PoolSpamPlacementsByProvider(ctx, since) + if err != nil { + t.Fatalf("PoolSpamPlacementsByProvider: %v", err) + } + delta := func(k string) int { return after[k] - before[k] } + + if got := delta("microsoft"); got != 3 { + t.Errorf("microsoft delta = %d, want 3: mail run by Microsoft over plain IMAP", got) + } + if got := delta("smtp_imap"); got != 0 { + t.Errorf("smtp_imap delta = %d, want 0: the connect method is not a provider", got) + } + // Domainless rows stay their own bucket rather than inflating custom. + if got := delta("unknown"); got != 2 { + t.Errorf("unknown delta = %d, want 2", got) + } + if got := delta("custom"); got != 0 { + t.Errorf("custom delta = %d, want 0", got) + } +} diff --git a/internal/tasks/warmup_partner_routing_live_test.go b/internal/tasks/warmup_partner_routing_live_test.go new file mode 100644 index 00000000..4faacf69 --- /dev/null +++ b/internal/tasks/warmup_partner_routing_live_test.go @@ -0,0 +1,220 @@ +package tasks + +import ( + "context" + "os" + "testing" + + "github.com/google/uuid" + "github.com/jackc/pgx/v5/pgxpool" + + "github.com/warmbly/warmbly/internal/infrastructure/db" + "github.com/warmbly/warmbly/internal/models" + "github.com/warmbly/warmbly/internal/pkg/encrypt" + "github.com/warmbly/warmbly/internal/repository" +) + +// Issue #143 end to end: the per-provider placement signal has to reach the +// partner the selector actually returns, not just the query that computes it. +// The repository tests prove the numbers; this proves selectWarmupPartner +// wires them into the weight. Skipped unless WARMBLY_TEST_DB is set: +// +// WARMBLY_TEST_DB=postgres://warmbly:warmbly@localhost:15432/warmbly_dev?sslmode=disable \ +// go test ./internal/tasks/ -run LiveWarmupPartner -v + +// freePoolID is the seeded free pool. It is used here rather than the premium +// one because the selector reads EVERY participant of the pool, and the free +// pool is the one no fixture or seed puts mailboxes in. +const freePoolID = "77777777-aaaa-0000-0000-000000000001" + +type partnerRoutingFixture struct { + pool *pgxpool.Pool + svc *tasksService + sender models.Email + user uuid.UUID + org uuid.UUID + atGoogle uuid.UUID + atMS uuid.UUID +} + +func newPartnerRoutingFixture(t *testing.T) *partnerRoutingFixture { + t.Helper() + dsn := os.Getenv("WARMBLY_TEST_DB") + if dsn == "" { + t.Skip("WARMBLY_TEST_DB not set") + } + ctx := context.Background() + handle, err := db.New(ctx, dsn) + if err != nil { + t.Fatalf("connect: %v", err) + } + t.Cleanup(func() { handle.Pool.Close() }) + + var pools int + if err := handle.Pool.QueryRow(ctx, `SELECT count(*) FROM warmup_pools WHERE id = $1`, freePoolID).Scan(&pools); err != nil || pools == 0 { + t.Skip("free warmup pool not seeded in this database") + } + // A pick is weighted across the WHOLE pool, so a stray participant would + // dilute the measurement into a meaningless pass. + var occupied int + if err := handle.Pool.QueryRow(ctx, `SELECT count(*) FROM warmup_pool_participants WHERE pool_id = $1`, freePoolID).Scan(&occupied); err != nil { + t.Fatalf("count free pool: %v", err) + } + if occupied != 0 { + t.Skip("free pool already has participants; cannot isolate the measurement") + } + + f := &partnerRoutingFixture{ + pool: handle.Pool, user: uuid.New(), org: uuid.New(), + atGoogle: uuid.New(), atMS: uuid.New(), + } + senderID := uuid.New() + exec := func(sql string, args ...any) { + t.Helper() + if _, err := handle.Pool.Exec(ctx, sql, args...); err != nil { + t.Fatalf("fixture %q: %v", sql[:min(60, len(sql))], err) + } + } + exec(`INSERT INTO users (id, email, first_name, last_name) VALUES ($1, $2, 'Pick', 'Test')`, + f.user, "pick-"+f.user.String()[:8]+"@test.local") + exec(`INSERT INTO organizations (id, name, slug, owner_user_id) VALUES ($1, 'Pick Test', $2, $3)`, + f.org, "pick-"+f.org.String()[:8], f.user) + for _, m := range []struct { + id uuid.UUID + domain string + }{ + {senderID, "test.local"}, + {f.atGoogle, "gmail.com"}, + {f.atMS, "outlook.com"}, + } { + exec(`INSERT INTO email_accounts (id, user_id, organization_id, email, name, signature_plain, + signature_html, provider, status, campaign_limit, min_wait_time, timezone) + VALUES ($1, $2, $3, $4, 'Pick', '', '', 'smtp_imap', 'active', 50, 600, 'UTC')`, + m.id, f.user, f.org, "pick-"+m.id.String()[:8]+"@"+m.domain) + } + for _, id := range []uuid.UUID{f.atGoogle, f.atMS} { + exec(`INSERT INTO warmup_pool_participants (pool_id, email_account_id, participant_role, health_state) + VALUES ($1, $2, 'sender_receiver', 'healthy')`, freePoolID, id) + } + + t.Cleanup(func() { + c := context.Background() + for _, step := range []struct { + sql string + arg any + }{ + {`DELETE FROM warmup_spam_reports WHERE reported_account_id = $1`, senderID}, + {`DELETE FROM warmup_tokens WHERE sender_account_id = $1`, senderID}, + {`DELETE FROM tasks WHERE email_account_id = $1`, senderID}, + {`DELETE FROM warmup_pool_participants WHERE email_account_id IN (SELECT id FROM email_accounts WHERE organization_id = $1)`, f.org}, + {`DELETE FROM email_accounts WHERE organization_id = $1`, f.org}, + {`DELETE FROM organizations WHERE id = $1`, f.org}, + {`DELETE FROM users WHERE id = $1`, f.user}, + } { + if _, err := handle.Pool.Exec(c, step.sql, step.arg); err != nil { + t.Errorf("cleanup %q: %v", step.sql, err) + } + } + }) + + enc, err := encrypt.NewEncrypter([]byte("0123456789abcdef0123456789abcdef")) + if err != nil { + t.Fatalf("encrypter: %v", err) + } + f.svc = &tasksService{ + warmupRepo: repository.NewWarmupRepository(handle.Pool), + emailRepo: repository.NewEmailRepostory(handle, enc), + } + // WarmupPoolType short-circuits entitlement resolution, so the selector + // runs without a feature gate or org-risk repository. + f.sender = models.Email{ID: senderID, OrganizationID: &f.org, WarmupPoolType: "free"} + return f +} + +// history writes n completed warmup sends to one partner, backdated two days: +// inside the seven-day placement window, but outside the same-day exclusion +// that would drop both candidates before weighting ever runs. +func (f *partnerRoutingFixture) history(t *testing.T, recipient uuid.UUID, n int) { + t.Helper() + for i := 0; i < n; i++ { + taskID := uuid.New() + if _, err := f.pool.Exec(context.Background(), + `INSERT INTO tasks (id, task_type, email_account_id, status, message_id) + VALUES ($1, 'warmup', $2, 'completed', '')`, taskID, f.sender.ID); err != nil { + t.Fatalf("insert task: %v", err) + } + if _, err := f.pool.Exec(context.Background(), + `INSERT INTO warmup_tokens (token, task_id, sender_account_id, recipient_account_id, created_at) + VALUES (gen_random_uuid(), $1, $2, $3, NOW() - INTERVAL '2 days')`, + taskID, f.sender.ID, recipient); err != nil { + t.Fatalf("insert token: %v", err) + } + } +} + +func (f *partnerRoutingFixture) junked(t *testing.T, domain string, n int) { + t.Helper() + for i := 0; i < n; i++ { + if _, err := f.pool.Exec(context.Background(), + `INSERT INTO warmup_spam_reports (id, reporter_account_id, reported_account_id, message_id, + report_type, recipient_domain, created_at) + VALUES (gen_random_uuid(), $1, $1, $2, 'spam_placement', $3, NOW() - INTERVAL '1 day')`, + f.sender.ID, "m-"+uuid.New().String(), domain); err != nil { + t.Fatalf("insert placement: %v", err) + } + } +} + +// picks runs the real selector n times and reports how often each partner won. +func (f *partnerRoutingFixture) picks(t *testing.T, n int) (google, microsoft int) { + t.Helper() + ctx := context.Background() + for i := 0; i < n; i++ { + partner, err := f.svc.selectWarmupPartner(ctx, f.sender) + if err != nil { + t.Fatalf("selectWarmupPartner: %v", err) + } + switch partner.ID { + case f.atGoogle: + google++ + case f.atMS: + microsoft++ + default: + t.Fatalf("selector returned a mailbox outside the fixture: %s", partner.ID) + } + } + return google, microsoft +} + +// The whole point of #143: a sender landing in junk only at Microsoft stops +// being handed Microsoft partners, without an aggregate health band tripping. +func TestLiveWarmupPartnerRoutesAwayFromTheProviderItLandsInJunkAt(t *testing.T) { + f := newPartnerRoutingFixture(t) + const rounds = 200 + + // Equal history on both sides, so the domain-diversity weight cannot be + // what moves the split. + f.history(t, f.atGoogle, 10) + f.history(t, f.atMS, 10) + + baseGoogle, baseMS := f.picks(t, rounds) + if baseGoogle < rounds*35/100 || baseGoogle > rounds*65/100 { + t.Fatalf("baseline split is not even: google %d, microsoft %d of %d", baseGoogle, baseMS, rounds) + } + + // 6 of the 10 Microsoft sends were filtered into junk. Nothing about the + // Google side changed. + f.junked(t, "outlook.com", 6) + + google, microsoft := f.picks(t, rounds) + // weight ratio is 1 : 1/(1+4*0.6), so google should take ~77%. + if google <= rounds*60/100 { + t.Errorf("placement signal did not reach the selector: google %d, microsoft %d of %d (baseline was %d/%d)", + google, microsoft, rounds, baseGoogle, baseMS) + } + // Downweighted, never excluded: a sender that stops mailing a provider + // entirely can never discover it recovered there. + if microsoft == 0 { + t.Errorf("microsoft was excluded outright over %d picks; the penalty must only downweight", rounds) + } +} From 230d80db64927075d80c58504114fe8a35cdc1bc Mon Sep 17 00:00:00 2001 From: Matthew Meszaros Date: Fri, 28 Aug 2026 21:31:06 -0700 Subject: [PATCH 3/9] feat: condense the new warmup pool comments in email_task.go, handler.go, service.go, pg_worker.go and warmup_scheduler.go to the one-line form the repo convention asks for, keeping the non-obvious constraints (the stored tier is never empty, the tier column records what is paid for rather than where the mailbox warms) and dropping the narration --- internal/app/email/handler.go | 6 +++--- internal/app/email/service.go | 3 +-- internal/repository/pg_worker.go | 5 ++--- internal/scheduler/warmup_scheduler.go | 5 ++--- internal/tasks/email_task.go | 7 ++----- 5 files changed, 10 insertions(+), 16 deletions(-) diff --git a/internal/app/email/handler.go b/internal/app/email/handler.go index adc0fd70..87cfb1df 100644 --- a/internal/app/email/handler.go +++ b/internal/app/email/handler.go @@ -360,7 +360,7 @@ func (s *emailService) canUseWarmupPool(ctx context.Context, account *models.Ema } // orgSuspendedOrRestricted reports whether the workspace's posture bars the -// paid warmup pool. Fails open, like every other risk read. +// paid warmup pool. Fails open. func (s *emailService) orgSuspendedOrRestricted(ctx context.Context, orgID uuid.UUID) bool { if s.orgRiskRepo == nil { return false @@ -381,8 +381,8 @@ func (s *emailService) resolveWarmupPoolType(ctx context.Context, account *model if account.OrganizationID == nil { return "free" } - // A restricted organization warms in the free pool whatever it pays; the - // stored tier is checked after, since it is always set (issue #242). + // A restricted organization leaves the paid pool whatever it pays. Checked + // before the stored tier, which is never empty and would short-circuit it. if s.orgSuspendedOrRestricted(ctx, *account.OrganizationID) { return "free" } diff --git a/internal/app/email/service.go b/internal/app/email/service.go index 1b41df63..8f28c752 100644 --- a/internal/app/email/service.go +++ b/internal/app/email/service.go @@ -123,8 +123,7 @@ type OrgRiskAware interface { WireOrgRisk(r repository.OrgRiskRepository) } -// The backend attaches the posture through a type assertion, so a silently -// unsatisfied interface would leave restricted workspaces in the paid pool. +// main.go attaches the posture by type assertion, which fails silently. var _ OrgRiskAware = (*emailService)(nil) // SyncBudgetSource is the operator-editable sync fair-use section, satisfied diff --git a/internal/repository/pg_worker.go b/internal/repository/pg_worker.go index 535cdfbc..4cadd298 100644 --- a/internal/repository/pg_worker.go +++ b/internal/repository/pg_worker.go @@ -496,9 +496,8 @@ func (r *workerRepository) ClearEmailAccountWorker(ctx context.Context, emailAcc // UpdateEmailAccountWarmupPoolType writes the tier and moves the mailbox's pool membership to // match in one transaction: they record the same fact, and updating only the column left // downgraded mailboxes in the premium pool (issue #211). A mailbox in no pool stays in none. -// The membership move into premium is refused while the owning organization is restricted or -// suspended, so a worker rebalance cannot readmit a risky tenant to the paid pool (issue #242). -// The tier column is still written: it records what the workspace pays for, not where it warms. +// The move into premium is refused while the organization is restricted (issue #242); the tier +// column is still written, since it records what the workspace pays for, not where it warms. func (r *workerRepository) UpdateEmailAccountWarmupPoolType(ctx context.Context, emailAccountID uuid.UUID, poolType string) error { tx, err := r.db.Begin(ctx) if err != nil { diff --git a/internal/scheduler/warmup_scheduler.go b/internal/scheduler/warmup_scheduler.go index 24c815db..d8ce675d 100644 --- a/internal/scheduler/warmup_scheduler.go +++ b/internal/scheduler/warmup_scheduler.go @@ -47,9 +47,8 @@ func adjustmentFor(state models.WarmupHealthState) healthAdjustment { } } -// warmupPoolTypeForAccount is the pool the mailbox actually warms in. A -// restricted organization is held in the free pool whatever tier its mailbox -// carries, matching the task service's resolution (issue #242). +// warmupPoolTypeForAccount is the pool the mailbox actually warms in: a restricted +// organization is held in free whatever tier it carries. func (s *schedulerService) warmupPoolTypeForAccount(ctx context.Context, account *models.Email) string { if account == nil { return "premium" diff --git a/internal/tasks/email_task.go b/internal/tasks/email_task.go index aa699873..725b49a6 100644 --- a/internal/tasks/email_task.go +++ b/internal/tasks/email_task.go @@ -744,11 +744,8 @@ func (s *tasksService) resolveWarmupPoolType(ctx context.Context, account *Email if account.OrganizationID == nil { return "free" } - // A restricted organization leaves the paid pool whatever it pays: the - // shared reputation paying customers depend on is not for spending on a - // risky tenant. Checked before the stored tier, which is always set - // (schema default 'free', worker assignment writes it) and so would - // otherwise short-circuit this gate (issue #242). + // A restricted organization leaves the paid pool whatever it pays. Checked + // before the stored tier, which is never empty and would short-circuit it. if s.orgSuspendedOrRestricted(ctx, *account.OrganizationID) { return "free" } From 4ddcb24bcd8ae09af4828cab725935ab5daa3a9a Mon Sep 17 00:00:00 2001 From: Matthew Meszaros Date: Fri, 28 Aug 2026 21:54:29 -0700 Subject: [PATCH 4/9] feat: make the campaign content check actually fire, fixing a link cap that counted zero links on every HTML email because the URLs live in href attributes that tag-stripping discards, a preflight that scored wait and action nodes as copy and so failed every campaign using one at 55/100 because GetSequencesByCampaignID never selected the kind column, a content_warning log level of "warning" that never matched the dashboard's "warn" amber tier, an unvalidated min_content_score an API caller could set to 5000, attachments the send path scored but preflight ignored, and a docs claim of live editor scoring that was really a manual button --- cmd/backend/main.go | 5 + .../docs/api/reference/deliverability-ops.mdx | 4 + docs/content/docs/guides/campaigns.mdx | 6 +- internal/app/advanced/content_score_test.go | 97 +++++++++++++++++++ internal/app/advanced/service.go | 86 +++++++++++++--- internal/models/advanced_outreach.go | 16 ++- internal/models/advanced_outreach_test.go | 33 +++++++ internal/pkg/warmlint/lint.go | 25 +++-- internal/pkg/warmlint/score_test.go | 47 +++++++++ internal/repository/pg_campaign.go | 4 +- .../repository/sequence_kind_live_test.go | 55 +++++++++++ internal/tasks/content_gate.go | 17 +++- web/src/app/app/settings/sending/page.tsx | 4 +- .../components/app/campaigns/ContentScore.tsx | 67 +++++++++---- .../hooks/app/campaigns/useScoreTemplate.ts | 12 --- 15 files changed, 411 insertions(+), 67 deletions(-) create mode 100644 internal/app/advanced/content_score_test.go create mode 100644 internal/models/advanced_outreach_test.go create mode 100644 internal/repository/sequence_kind_live_test.go delete mode 100644 web/src/lib/api/hooks/app/campaigns/useScoreTemplate.ts diff --git a/cmd/backend/main.go b/cmd/backend/main.go index e99dfa78..b560d610 100644 --- a/cmd/backend/main.go +++ b/cmd/backend/main.go @@ -1289,6 +1289,11 @@ func main() { if aware, ok := advancedService.(advanced.AudienceAware); ok { aware.WireAudience(campaignAudienceRepository) } + // Preflight's content check weighs attachments the way the send path + // does, so the launch dialog and the campaign feed agree on the score. + if aware, ok := advancedService.(advanced.AttachmentAware); ok { + aware.WireAttachments(attachmentRepoForHandler) + } // Shared AI tool registry: every tool calls a service-layer function as // the invoking user, so the dashboard agent (M3) and MCP server (M8) can diff --git a/docs/content/docs/api/reference/deliverability-ops.mdx b/docs/content/docs/api/reference/deliverability-ops.mdx index 019c647c..c783b674 100644 --- a/docs/content/docs/api/reference/deliverability-ops.mdx +++ b/docs/content/docs/api/reference/deliverability-ops.mdx @@ -79,6 +79,10 @@ Returns the `AdvancedOutreachSettings` object directly (not wrapped in an envelo } ``` + +`preflight.min_content_score` is stored clamped to `1`-`100`. A value outside that range is corrected on write rather than rejected, so a floor above `100` cannot flag every campaign permanently. Set `preflight.check_content_score` to `false` to turn the check off; the score never blocks or delays a send either way. See [Content checks](/guides/campaigns/). + + `send_time_optimization.enabled` defaults to `false`. Set it to `true` and campaign scheduling holds each send until the recipient's local clock reaches one of `preferred_hours`, resolving the recipient's timezone from the contact's `timezone` custom field, then the country-code suffix of its email domain, then `default_contact_timezone`. It can only delay a send: the campaign window, the mailbox's sending profile, its daily cap, and the campaign end date all still bind. See [Sending behavior](/guides/sending-behavior/). diff --git a/docs/content/docs/guides/campaigns.mdx b/docs/content/docs/guides/campaigns.mdx index bb007cba..6a1121ce 100644 --- a/docs/content/docs/guides/campaigns.mdx +++ b/docs/content/docs/guides/campaigns.mdx @@ -138,13 +138,13 @@ Warmbly scores each step's copy out of 100 for the signals spam filters weight: You see it in three places: -- **In the editor**, live as you write a step. -- **In the launch dialog**, alongside the other pre-send checks, with the lowest-scoring step named. +- **In the editor**, re-scored as you write a step. +- **In the launch dialog**, alongside the other pre-send checks, with the lowest-scoring step named. Only email steps are scored; wait and action steps carry no copy. - **In the campaign activity feed**, if the copy that actually goes out scores below your floor. That last one catches what the first two cannot. The editor scores the template; the send path scores the message after merge fields, spintax, A/B selection and AI blocks have resolved, which is where a clean template becomes "Hi ," or picks the one spammy spintax branch. It logs once per step per day, not once per recipient. -**Settings** > **Sending** > **Content checks** turns it off or moves the floor, which defaults to `60`. +**Settings** > **Sending** > **Content checks** turns it off or moves the floor, which defaults to `60` and accepts `1` to `100`. A low score never blocks or delays a send. It is a signal to rewrite, not a verdict: a legitimate email can score badly, and a well-scoring one sent to a bad list will still fail. diff --git a/internal/app/advanced/content_score_test.go b/internal/app/advanced/content_score_test.go new file mode 100644 index 00000000..843ce8f6 --- /dev/null +++ b/internal/app/advanced/content_score_test.go @@ -0,0 +1,97 @@ +package advanced + +import ( + "strings" + "testing" + + "github.com/warmbly/warmbly/internal/models" +) + +func emailStep(pos int, subject, body string) models.Sequence { + return models.Sequence{ + Kind: "email", + Position: pos, + Subject: subject, + BodyHTML: "

" + body + "

", + BodyPlain: body, + } +} + +// A wait or action node has no subject and no body. Scoring it as copy made +// every campaign that used one fail preflight with "scores 55/100 for spam +// signals" about a step that was never an email. +func TestWorstStepContentScoreSkipsNonEmailSteps(t *testing.T) { + good := strings.Repeat("A real sentence about the recipient's work. ", 5) + seqs := []models.Sequence{ + emailStep(0, "Quick question about hiring", good), + {Kind: "wait", Position: 1}, + {Kind: "action", Position: 2}, + emailStep(3, "Following up on my note", good), + } + + worst, _, _, scored := worstStepContentScore(seqs, 0) + if scored != 2 { + t.Errorf("scored %d steps, want the 2 email steps", scored) + } + if worst != 100 { + t.Errorf("clean campaign scored %d, want 100", worst) + } +} + +// A campaign of nothing but control nodes has no copy to judge, which must read +// as "nothing to score" rather than as a perfect or a failing score. +func TestWorstStepContentScoreReportsNothingToScore(t *testing.T) { + _, _, _, scored := worstStepContentScore([]models.Sequence{ + {Kind: "wait", Position: 0}, + {Kind: "action", Position: 1}, + }, 0) + if scored != 0 { + t.Errorf("scored %d steps, want 0", scored) + } +} + +// The reported step number must be the step's position, so preflight and the +// per-send warning name the same step. +func TestWorstStepContentScoreReportsThePositionOfTheWorstStep(t *testing.T) { + good := strings.Repeat("A real sentence about the recipient's work. ", 5) + seqs := []models.Sequence{ + emailStep(0, "Quick question about hiring", good), + {Kind: "wait", Position: 1}, + emailStep(2, "FREE CASH PRIZE GUARANTEED!!!", "Act now, click here, 100% free, risk free."), + } + + worst, step, issue, scored := worstStepContentScore(seqs, 0) + if scored != 2 { + t.Fatalf("scored %d steps, want 2", scored) + } + if step != 3 { + t.Errorf("worst step reported as %d, want 3 (position 2)", step) + } + if worst >= 60 { + t.Errorf("obviously spammy copy scored %d, want it below the default floor", worst) + } + if issue == "" { + t.Error("no leading issue reported for the worst step") + } +} + +// An empty step list falls out with nothing scored: the caller reports that +// rather than treating it as passing content. +func TestWorstStepContentScoreOnEmptyCampaign(t *testing.T) { + if _, _, _, scored := worstStepContentScore(nil, 0); scored != 0 { + t.Errorf("scored %d steps on an empty campaign, want 0", scored) + } +} + +// Attachments are campaign-wide, so preflight weighs them the way the send path +// does instead of reporting a score the activity feed later contradicts. +func TestWorstStepContentScoreCountsAttachments(t *testing.T) { + good := strings.Repeat("A real sentence about the recipient's work. ", 5) + seqs := []models.Sequence{emailStep(0, "Quick question about hiring", good)} + + clean, _, _, _ := worstStepContentScore(seqs, 0) + withAtt, _, _, _ := worstStepContentScore(seqs, 2) + if withAtt >= clean { + t.Errorf("attachment score %d not below the clean %d", withAtt, clean) + } +} diff --git a/internal/app/advanced/service.go b/internal/app/advanced/service.go index 15fef5b4..615d4557 100644 --- a/internal/app/advanced/service.go +++ b/internal/app/advanced/service.go @@ -165,7 +165,11 @@ type service struct { dispatcher EventDispatcher // audienceRepo measures a campaign's list for the preflight report. // Optional/nil-safe: without it the list check is simply absent. - audienceRepo repository.CampaignAudienceRepository + audienceRepo repository.CampaignAudienceRepository + // attachmentRepo counts a campaign's attachments so preflight scores copy + // the way the send path does. Optional/nil-safe: without it the content + // check simply scores zero attachments. + attachmentRepo repository.AttachmentRepository notifier Notifier realtime ReplyRealtimePublisher automationRunner AutomationRunner @@ -222,6 +226,7 @@ func (s *service) UpdateOrganizationSettings(ctx context.Context, organizationID if settings == nil { return errx.New(errx.BadRequest, "settings are required") } + settings.Normalize() if err := s.repo.UpsertOutreachSettings(ctx, organizationID, updatedBy, settings); err != nil { return toErrx(err) } @@ -247,6 +252,7 @@ func (s *service) UpdateCampaignSettings(ctx context.Context, campaignID uuid.UU if settings == nil { return errx.New(errx.BadRequest, "settings are required") } + settings.Normalize() if err := s.repo.UpsertCampaignAdvancedSettings(ctx, campaignID, settings); err != nil { return toErrx(err) } @@ -1760,11 +1766,43 @@ func (s *service) ProcessRetryableDeadLetters(ctx context.Context) (int, *errx.E return retried, nil } +// worstStepContentScore scores each email step's copy and returns the lowest +// score, that step's number, its leading issue, and how many steps were scored. +// Only email steps carry copy: a wait or action node has no subject or body and +// would otherwise score as the campaign's worst content. Step numbers are the +// step's position, the same number the per-send warning reports. +func worstStepContentScore(seqs []models.Sequence, attachments int) (worst, worstStep int, issue string, scored int) { + worst = 101 + for _, seq := range seqs { + if seq.Kind != "" && seq.Kind != "email" { + continue + } + scored++ + r := warmlint.ScoreWithAttachments(seq.Subject, seq.BodyHTML, seq.BodyPlain, attachments) + if r.Score >= worst { + continue + } + worst, worstStep, issue = r.Score, seq.Position+1, "" + for _, is := range r.Issues { + if is.Severity == "high" { + issue = is.Message + break + } + } + if issue == "" && len(r.Issues) > 0 { + issue = r.Issues[0].Message + } + } + return worst, worstStep, issue, scored +} + // contentScoreCheck scores every step's copy and reports the worst. A step list // it could not read reports as FAILED, not passed: a check that did not run // must never look like one that succeeded. func (s *service) contentScoreCheck(ctx context.Context, campaignID uuid.UUID, floor int, recommendations *[]string) models.PreflightCheckResult { - if floor <= 0 { + if floor <= 0 || floor > 100 { + // Out of range means a row written before the floor was clamped; fall + // back to the default rather than honoring a floor nothing can clear. floor = 60 } seqs, err := s.campaignRepo.GetSequencesByCampaignID(ctx, campaignID) @@ -1787,21 +1825,22 @@ func (s *service) contentScoreCheck(ctx context.Context, campaignID uuid.UUID, f } } - worst, worstStep, issue := 101, 0, "" - for i, seq := range seqs { - r := warmlint.Score(seq.Subject, seq.BodyHTML, seq.BodyPlain) - if r.Score >= worst { - continue + // Attachments are campaign-wide and the send path scores them, so preflight + // weighs them too rather than reporting a score the feed later contradicts. + attachments := 0 + if s.attachmentRepo != nil { + if atts, aerr := s.attachmentRepo.ListByCampaign(ctx, campaignID); aerr == nil { + attachments = len(atts) } - worst, worstStep, issue = r.Score, i+1, "" - for _, is := range r.Issues { - if is.Severity == "high" { - issue = is.Message - break - } - } - if issue == "" && len(r.Issues) > 0 { - issue = r.Issues[0].Message + } + + worst, worstStep, issue, scored := worstStepContentScore(seqs, attachments) + if scored == 0 { + return models.PreflightCheckResult{ + Key: "content_score", + Passed: true, + Severity: "info", + Message: "No email steps to score yet.", } } @@ -1862,3 +1901,18 @@ func (s *service) WireAudience(r repository.CampaignAudienceRepository) { type AudienceAware interface { WireAudience(r repository.CampaignAudienceRepository) } + +// WireAttachments attaches the campaign attachment counter the content check +// scores with, so preflight and the send path weigh attachments the same way. +func (s *service) WireAttachments(r repository.AttachmentRepository) { + s.attachmentRepo = r +} + +// AttachmentAware is the optional capability the caller uses to attach it. +type AttachmentAware interface { + WireAttachments(r repository.AttachmentRepository) +} + +// The wiring in main is a type assertion, so a receiver change would silently +// stop attaching the repository rather than fail the build. +var _ AttachmentAware = (*service)(nil) diff --git a/internal/models/advanced_outreach.go b/internal/models/advanced_outreach.go index 614eea26..4cb38d51 100644 --- a/internal/models/advanced_outreach.go +++ b/internal/models/advanced_outreach.go @@ -61,10 +61,24 @@ type PreflightValidationSettings struct { // and again per send against the rendered text. Advisory: it warns, it // never blocks a send. CheckContentScore bool `json:"check_content_score"` - // MinContentScore is the 0-100 floor below which copy is flagged. + // MinContentScore is the 1-100 floor below which copy is flagged. MinContentScore int `json:"min_content_score"` } +// Normalize clamps the settings an API caller can put out of range, so a stored +// value can never make every campaign fail the check or none of them. +func (s *AdvancedOutreachSettings) Normalize() { + if s == nil { + return + } + if s.Preflight.MinContentScore > 100 { + s.Preflight.MinContentScore = 100 + } + if s.Preflight.MinContentScore < 1 { + s.Preflight.MinContentScore = 1 + } +} + type DeliverabilityDashboardSettings struct { Enabled bool `json:"enabled"` ShowSuppressionLog bool `json:"show_suppression_log"` diff --git a/internal/models/advanced_outreach_test.go b/internal/models/advanced_outreach_test.go new file mode 100644 index 00000000..db20d61d --- /dev/null +++ b/internal/models/advanced_outreach_test.go @@ -0,0 +1,33 @@ +package models + +import "testing" + +// The content-score floor reaches the API as a plain integer. Left unclamped, a +// floor above 100 flags every campaign forever and a floor at or below 0 is a +// control that does nothing, since the readers fall back to the default. +func TestNormalizeClampsTheContentScoreFloor(t *testing.T) { + for _, tc := range []struct{ in, want int }{ + {-40, 1}, + {0, 1}, + {1, 1}, + {60, 60}, + {100, 100}, + {5000, 100}, + } { + s := DefaultAdvancedOutreachSettings() + s.Preflight.MinContentScore = tc.in + s.Normalize() + if s.Preflight.MinContentScore != tc.want { + t.Errorf("floor %d normalized to %d, want %d", tc.in, s.Preflight.MinContentScore, tc.want) + } + } +} + +func TestNormalizeLeavesTheDefaultsAlone(t *testing.T) { + s := DefaultAdvancedOutreachSettings() + before := s + s.Normalize() + if s.Preflight != before.Preflight { + t.Errorf("defaults changed under Normalize: %+v -> %+v", before.Preflight, s.Preflight) + } +} diff --git a/internal/pkg/warmlint/lint.go b/internal/pkg/warmlint/lint.go index 44c7ec46..83481e09 100644 --- a/internal/pkg/warmlint/lint.go +++ b/internal/pkg/warmlint/lint.go @@ -14,6 +14,7 @@ var ( stackedPunct = regexp.MustCompile(`[!?]{2,}`) wordToken = regexp.MustCompile(`[a-z0-9%]+`) linkPattern = regexp.MustCompile(`https?://`) + hrefPattern = regexp.MustCompile(`(?i)href\s*=\s*["']?\s*https?://`) htmlTag = regexp.MustCompile(`(?i)<[a-z!/][^>]*>`) imgTag = regexp.MustCompile(`(?i)]*>`) ) @@ -99,7 +100,7 @@ func Score(subject, bodyHTML, bodyPlain string) ScoreResult { if subj == "" { deduct(20, "high", "empty_subject", "Subject is empty.") } else if isAllCaps(subj) { - deduct(15, "high", "all_caps_subject", "Subject is all caps — a strong spam signal.") + deduct(15, "high", "all_caps_subject", "Subject is all caps, a strong spam signal.") } if stackedPunct.MatchString(combined) { deduct(10, "warn", "stacked_punctuation", "Stacked punctuation (e.g. !!! or ?!) reads as promotional.") @@ -115,12 +116,12 @@ func Score(subject, bodyHTML, bodyPlain string) ScoreResult { } deduct(d, severity, "spam_trigger_terms", fmt.Sprintf("%d spam-trigger term(s) found in subject/body.", n)) } - if links := len(linkPattern.FindAllString(combined, -1)); links > 3 { + if links := countLinks(combined, bodyHTML); links > 3 { d := (links - 3) * 5 if d > 20 { d = 20 } - deduct(d, "warn", "too_many_links", fmt.Sprintf("%d links — keep cold-email link count low.", links)) + deduct(d, "warn", "too_many_links", fmt.Sprintf("%d links. Keep the link count low in cold email.", links)) } if strings.TrimSpace(body) == "" { deduct(25, "high", "empty_body", "Body has no text content (image-only or empty body hurts deliverability).") @@ -134,13 +135,13 @@ func Score(subject, bodyHTML, bodyPlain string) ScoreResult { switch { case len(strings.TrimSpace(body)) < 200 && images >= 1: deduct(20, "high", "image_heavy", - "Almost all of this email is images — filters cannot read it and treat that as evasion.") + "Almost all of this email is images. Filters cannot read it and treat that as evasion.") case images > 3: d := (images - 3) * 5 if d > 15 { d = 15 } - deduct(d, "warn", "many_images", fmt.Sprintf("%d images — cold email from a person rarely has many.", images)) + deduct(d, "warn", "many_images", fmt.Sprintf("%d images. Cold email from a person rarely has many.", images)) } } @@ -164,7 +165,7 @@ func ScoreWithAttachments(subject, bodyHTML, bodyPlain string, attachments int) res.Issues = append(res.Issues, Issue{ Severity: "warn", Code: "has_attachments", - Message: fmt.Sprintf("%d attachment(s) on a cold email — link to the file instead.", attachments), + Message: fmt.Sprintf("%d attachment(s) on a cold email. Link to the file instead.", attachments), }) } return res @@ -187,6 +188,18 @@ func isAllCaps(s string) bool { return letters >= 4 } +// countLinks counts the links the recipient can click. An anchor carries its +// URL in the href, which stripping tags throws away, so the text alone reports +// zero links for a normal HTML email; take the larger of the two counts so a +// URL used as its own anchor text is not counted twice. +func countLinks(text, bodyHTML string) int { + n := len(linkPattern.FindAllString(text, -1)) + if h := len(hrefPattern.FindAllString(bodyHTML, -1)); h > n { + n = h + } + return n +} + func countTriggerTerms(text string) int { lower := strings.ToLower(text) found := map[string]struct{}{} diff --git a/internal/pkg/warmlint/score_test.go b/internal/pkg/warmlint/score_test.go index a93fff82..3d23119e 100644 --- a/internal/pkg/warmlint/score_test.go +++ b/internal/pkg/warmlint/score_test.go @@ -83,3 +83,50 @@ func TestScoreNeverGoesNegative(t *testing.T) { t.Error("obviously spammy copy produced no issues") } } + +func TestScoreCountsLinksInHTMLAnchors(t *testing.T) { + // A normal HTML email carries its URLs in href attributes. Stripping tags + // throws those away, so counting the text alone reported zero links and the + // cap never fired for the case it exists for. + body := strings.Repeat("A real sentence about the recipient's work. ", 10) + html := "

" + body + "

" + + strings.Repeat(`see this `, 6) + "

" + + res := Score("Quick question", html, "") + if !hasIssue(res, "too_many_links") { + t.Errorf("six anchors in an HTML body were not counted: %+v", res.Issues) + } + + // The editor stores a plain-text body derived from the HTML, which also + // drops the hrefs. The score must not depend on which one is present. + res = Score("Quick question", html, body) + if !hasIssue(res, "too_many_links") { + t.Errorf("six anchors alongside a link-free plain body were not counted: %+v", res.Issues) + } +} + +func TestScoreDoesNotDoubleCountSelfLinkingAnchors(t *testing.T) { + // A URL used as its own anchor text appears in both the href and the text. + // Counting both would flag three links as six. + body := strings.Repeat("A real sentence about the recipient's work. ", 10) + html := "

" + body + "

" + + `https://a.com/1 ` + + `https://b.com/2 ` + + `https://c.com/3` + "

" + plain := body + " https://a.com/1 https://b.com/2 https://c.com/3" + + res := Score("Quick question", html, plain) + if hasIssue(res, "too_many_links") { + t.Errorf("three self-linking anchors were counted as more: %+v", res.Issues) + } +} + +func TestScoreStillCountsPlainTextURLs(t *testing.T) { + body := strings.Repeat("A real sentence about the recipient's work. ", 10) + plain := body + " https://a.com/1 https://b.com/2 https://c.com/3 https://d.com/4 https://e.com/5" + + res := Score("Quick question", "", plain) + if !hasIssue(res, "too_many_links") { + t.Errorf("five bare URLs in a plain body were not counted: %+v", res.Issues) + } +} diff --git a/internal/repository/pg_campaign.go b/internal/repository/pg_campaign.go index 49e00f2f..5cfe42f9 100644 --- a/internal/repository/pg_campaign.go +++ b/internal/repository/pg_campaign.go @@ -1265,7 +1265,7 @@ func (r *campaignRepository) GetSequenceByID(ctx context.Context, sequenceID uui // GetSequencesByCampaignID retrieves all sequences for a campaign ordered by position func (r *campaignRepository) GetSequencesByCampaignID(ctx context.Context, campaignID uuid.UUID) ([]models.Sequence, error) { query := ` - SELECT id, name, subject, body_plain, body_html, body_sync, body_code, wait_after, position, updated_at, created_at + SELECT id, name, subject, body_plain, body_html, body_sync, body_code, wait_after, position, kind, updated_at, created_at FROM sequences WHERE campaign_id = $1 ORDER BY position ASC, created_at ASC @@ -1283,7 +1283,7 @@ func (r *campaignRepository) GetSequencesByCampaignID(ctx context.Context, campa var seq models.Sequence err := rows.Scan( &seq.ID, &seq.Name, &seq.Subject, &seq.BodyPlain, &seq.BodyHTML, - &seq.BodySync, &seq.BodyCode, &seq.WaitAfter, &seq.Position, &seq.UpdatedAt, &seq.CreatedAt, + &seq.BodySync, &seq.BodyCode, &seq.WaitAfter, &seq.Position, &seq.Kind, &seq.UpdatedAt, &seq.CreatedAt, ) if err != nil { db.CaptureError(err, "", nil, "scan") diff --git a/internal/repository/sequence_kind_live_test.go b/internal/repository/sequence_kind_live_test.go new file mode 100644 index 00000000..4beed58f --- /dev/null +++ b/internal/repository/sequence_kind_live_test.go @@ -0,0 +1,55 @@ +package repository + +import ( + "context" + "testing" + + "github.com/google/uuid" +) + +// GetSequencesByCampaignID did not select `kind`, so every step came back +// looking like an email. Preflight's content check then scored wait and action +// nodes as copy and reported their empty subject and body as the campaign's +// worst content. +// +// WARMBLY_TEST_DB=postgres://warmbly:warmbly@localhost:15432/warmbly_dev?sslmode=disable \ +// go test ./internal/repository/ -run LiveSequenceKind -v +func TestLiveSequenceKindSurvivesTheRoundTrip(t *testing.T) { + handle, pool := liveContactDB(t) + f := newSharedOrgFixture(t, pool) + repo := NewCampaignRepostory(handle) + ctx := context.Background() + + t.Cleanup(func() { + if _, err := pool.Exec(context.Background(), + `DELETE FROM sequences WHERE campaign_id = $1`, f.campaign); err != nil { + t.Errorf("cleanup sequences: %v", err) + } + }) + + for _, step := range []struct { + pos int + kind string + name string + }{{0, "email", "Intro"}, {1, "wait", "Hold"}, {2, "action", "Tag"}} { + if _, err := pool.Exec(ctx, + `INSERT INTO sequences (id, campaign_id, organization_id, name, subject, body_plain, body_html, wait_after, position, kind) + VALUES ($1, $2, $3, $4, '', '', '', 0, $5, $6)`, + uuid.New(), f.campaign, f.org, step.name, step.pos, step.kind); err != nil { + t.Fatalf("insert %s step: %v", step.kind, err) + } + } + + seqs, err := repo.GetSequencesByCampaignID(ctx, f.campaign) + if err != nil { + t.Fatalf("get sequences: %v", err) + } + if len(seqs) != 3 { + t.Fatalf("got %d steps, want 3", len(seqs)) + } + for i, want := range []string{"email", "wait", "action"} { + if seqs[i].Kind != want { + t.Errorf("step %d kind = %q, want %q", i, seqs[i].Kind, want) + } + } +} diff --git a/internal/tasks/content_gate.go b/internal/tasks/content_gate.go index 20959763..df313697 100644 --- a/internal/tasks/content_gate.go +++ b/internal/tasks/content_gate.go @@ -23,6 +23,13 @@ func (s *tasksService) warnOnWeakContent(ctx context.Context, orgID, campaignID, if s.advanced == nil || s.campaignLogRepo == nil { return } + // Score first: it is pure CPU, and a clean 100 cannot fall below any floor + // (they are clamped to 100), so the common case never touches the database. + res := warmlint.ScoreWithAttachments(subject, bodyHTML, bodyPlain, attachments) + if res.Score >= 100 { + return + } + // Campaign-effective, not org-only: a campaign that turned the check off or // moved its floor must be honored here as it is at preflight. settings, xerr := s.advanced.EffectiveSettings(ctx, orgID, campaignID) @@ -30,11 +37,11 @@ func (s *tasksService) warnOnWeakContent(ctx context.Context, orgID, campaignID, return } floor := settings.Preflight.MinContentScore - if floor <= 0 { + if floor <= 0 || floor > 100 { + // Out of range means a row written before the floor was clamped; fall + // back to the default rather than honoring a floor nothing can clear. floor = 60 } - - res := warmlint.ScoreWithAttachments(subject, bodyHTML, bodyPlain, attachments) if res.Score >= floor { return } @@ -62,7 +69,9 @@ func (s *tasksService) warnOnWeakContent(ctx context.Context, orgID, campaignID, Message: fmt.Sprintf("Step %d's copy scores %d/100 for spam signals as sent (floor %d).%s", step, res.Score, floor, detail), Metadata: map[string]interface{}{ - "level": "warning", + // "warn" is the dashboard's amber tier; "warning" would fall + // through to the neutral info styling. + "level": "warn", "sequence_id": seq, "score": res.Score, "floor": floor, diff --git a/web/src/app/app/settings/sending/page.tsx b/web/src/app/app/settings/sending/page.tsx index af9bf270..e51e1f37 100644 --- a/web/src/app/app/settings/sending/page.tsx +++ b/web/src/app/app/settings/sending/page.tsx @@ -222,12 +222,12 @@ function SendingSettings() { description="Copy scoring below this out of 100 is flagged. Higher is stricter." > patchPreflight({ - min_content_score: Number.isFinite(n) ? Math.min(100, Math.max(0, n)) : 60, + min_content_score: Number.isFinite(n) ? Math.min(100, Math.max(1, n)) : 60, }) } className="w-20" diff --git a/web/src/components/app/campaigns/ContentScore.tsx b/web/src/components/app/campaigns/ContentScore.tsx index e778256f..cb30e476 100644 --- a/web/src/components/app/campaigns/ContentScore.tsx +++ b/web/src/components/app/campaigns/ContentScore.tsx @@ -1,10 +1,13 @@ -// Advisory campaign-template content check. A "Check content" button POSTs the -// current subject + body to /templates/score and renders a 0-100 score (higher -// = safer) plus a list of non-blocking issues. Purely advisory — it never -// blocks saving or sending, it just surfaces deliverability hints. +// Advisory campaign-template content check. Scores the current subject + body +// against /templates/score and renders a 0-100 score (higher = safer) plus a +// list of non-blocking issues. It re-scores as the copy changes, on the same +// debounce the composer's live preview uses. Purely advisory: it never blocks +// saving or sending, it just surfaces deliverability hints. +import * as React from "react"; import { ShieldCheckIcon, AlertTriangleIcon, AlertCircleIcon } from "lucide-react"; -import useScoreTemplate from "@/lib/api/hooks/app/campaigns/useScoreTemplate"; +import scoreTemplate from "@/lib/api/client/app/campaigns/scoreTemplate"; +import type TemplateScore from "@/lib/api/models/app/campaigns/TemplateScore"; import type { TemplateScoreIssue } from "@/lib/api/models/app/campaigns/TemplateScore"; import { Loading } from "@/components/loader"; import { cn } from "@/lib/utils"; @@ -39,11 +42,41 @@ export default function ContentScore({ bodyHtml: string; bodyPlain: string; }) { - const score = useScoreTemplate(); - const data = score.data; + const [data, setData] = React.useState(null); + const [pending, setPending] = React.useState(false); + const [failed, setFailed] = React.useState(false); - const run = () => - score.mutate({ subject, body_html: bodyHtml, body_plain: bodyPlain }); + React.useEffect(() => { + // A step with nothing written yet is not a content problem, so hold the + // panel quiet rather than scoring an empty draft as spam. + if (!subject.trim() && !bodyPlain.trim()) { + setData(null); + setFailed(false); + return; + } + let cancelled = false; + // Pending is set inside the timer, not on every keystroke, so the + // spinner marks a request in flight rather than flickering as you type. + const t = setTimeout(() => { + setPending(true); + scoreTemplate({ subject, body_html: bodyHtml, body_plain: bodyPlain }) + .then((res) => { + if (cancelled) return; + setData(res); + setFailed(false); + }) + .catch(() => { + if (!cancelled) setFailed(true); + }) + .finally(() => { + if (!cancelled) setPending(false); + }); + }, 600); + return () => { + cancelled = true; + clearTimeout(t); + }; + }, [subject, bodyHtml, bodyPlain]); const tone = data ? scoreTone(data.score) : null; @@ -52,21 +85,13 @@ export default function ContentScore({
Content check
-

Advisory deliverability score — never blocks sending.

+

Advisory deliverability score. It never blocks sending.

- + {pending && }
- {score.isError && ( -
Couldn't score this template. Try again.
+ {failed && ( +
Couldn't score this template.
)} {data && tone && ( diff --git a/web/src/lib/api/hooks/app/campaigns/useScoreTemplate.ts b/web/src/lib/api/hooks/app/campaigns/useScoreTemplate.ts deleted file mode 100644 index c889cb97..00000000 --- a/web/src/lib/api/hooks/app/campaigns/useScoreTemplate.ts +++ /dev/null @@ -1,12 +0,0 @@ -import { useMutation } from "@tanstack/react-query"; -import scoreTemplate from "@/lib/api/client/app/campaigns/scoreTemplate"; -import type { ScoreTemplateRequest } from "@/lib/api/models/app/campaigns/TemplateScore"; - -// On-demand advisory content score for a campaign template. A mutation rather -// than a query because it's run explicitly via a "Check content" button, not -// on every keystroke. -export default function useScoreTemplate() { - return useMutation({ - mutationFn: (body: ScoreTemplateRequest) => scoreTemplate(body), - }); -} From ec7b7bbed34ea4bf801b91fa12f59d45568264d6 Mon Sep 17 00:00:00 2001 From: Matthew Meszaros Date: Fri, 28 Aug 2026 21:59:53 -0700 Subject: [PATCH 5/9] feat: count anchors and bare URLs together instead of taking the larger of the two, so three labeled links plus three written-out URLs no longer score a clean 100, clear the editor's pending flag when the draft is emptied mid-request since the cancelled request's finally can no longer do it, fail the preflight content check when a campaign's attachments cannot be read rather than scoring as if there were none and passing copy the send path then warns about, and condense the new comments to the one-line form the repo convention asks for --- internal/app/advanced/service.go | 33 +++++++++++-------- internal/pkg/warmlint/lint.go | 31 ++++++++++++----- internal/pkg/warmlint/score_test.go | 29 ++++++++++++++++ internal/tasks/content_gate.go | 9 ++--- .../components/app/campaigns/ContentScore.tsx | 14 ++++---- 5 files changed, 80 insertions(+), 36 deletions(-) diff --git a/internal/app/advanced/service.go b/internal/app/advanced/service.go index 615d4557..d1dbc965 100644 --- a/internal/app/advanced/service.go +++ b/internal/app/advanced/service.go @@ -166,9 +166,8 @@ type service struct { // audienceRepo measures a campaign's list for the preflight report. // Optional/nil-safe: without it the list check is simply absent. audienceRepo repository.CampaignAudienceRepository - // attachmentRepo counts a campaign's attachments so preflight scores copy - // the way the send path does. Optional/nil-safe: without it the content - // check simply scores zero attachments. + // attachmentRepo lets preflight weigh attachments as the send path does. + // Optional/nil-safe: without it the content check scores none. attachmentRepo repository.AttachmentRepository notifier Notifier realtime ReplyRealtimePublisher @@ -1766,11 +1765,9 @@ func (s *service) ProcessRetryableDeadLetters(ctx context.Context) (int, *errx.E return retried, nil } -// worstStepContentScore scores each email step's copy and returns the lowest -// score, that step's number, its leading issue, and how many steps were scored. -// Only email steps carry copy: a wait or action node has no subject or body and -// would otherwise score as the campaign's worst content. Step numbers are the -// step's position, the same number the per-send warning reports. +// worstStepContentScore returns the lowest-scoring email step's score, number, +// leading issue, and how many steps were scored. Only email steps carry copy: a +// wait or action node would otherwise score as the campaign's worst content. func worstStepContentScore(seqs []models.Sequence, attachments int) (worst, worstStep int, issue string, scored int) { worst = 101 for _, seq := range seqs { @@ -1800,9 +1797,8 @@ func worstStepContentScore(seqs []models.Sequence, attachments int) (worst, wors // it could not read reports as FAILED, not passed: a check that did not run // must never look like one that succeeded. func (s *service) contentScoreCheck(ctx context.Context, campaignID uuid.UUID, floor int, recommendations *[]string) models.PreflightCheckResult { + // Out of range means a row written before the floor was clamped. if floor <= 0 || floor > 100 { - // Out of range means a row written before the floor was clamped; fall - // back to the default rather than honoring a floor nothing can clear. floor = 60 } seqs, err := s.campaignRepo.GetSequencesByCampaignID(ctx, campaignID) @@ -1829,9 +1825,19 @@ func (s *service) contentScoreCheck(ctx context.Context, campaignID uuid.UUID, f // weighs them too rather than reporting a score the feed later contradicts. attachments := 0 if s.attachmentRepo != nil { - if atts, aerr := s.attachmentRepo.ListByCampaign(ctx, campaignID); aerr == nil { - attachments = len(atts) + atts, aerr := s.attachmentRepo.ListByCampaign(ctx, campaignID) + if aerr != nil { + // Scoring as none would pass copy the send path then warns about. + *recommendations = append(*recommendations, "Re-run preflight; the campaign's attachments could not be read.") + return models.PreflightCheckResult{ + Key: "content_score", + Passed: false, + Severity: "warning", + Message: "Could not read the campaign's attachments to score its copy.", + Remediation: "Re-run preflight.", + } } + attachments = len(atts) } worst, worstStep, issue, scored := worstStepContentScore(seqs, attachments) @@ -1913,6 +1919,5 @@ type AttachmentAware interface { WireAttachments(r repository.AttachmentRepository) } -// The wiring in main is a type assertion, so a receiver change would silently -// stop attaching the repository rather than fail the build. +// main attaches this by type assertion, which fails silently, so pin it here. var _ AttachmentAware = (*service)(nil) diff --git a/internal/pkg/warmlint/lint.go b/internal/pkg/warmlint/lint.go index 83481e09..2931bef8 100644 --- a/internal/pkg/warmlint/lint.go +++ b/internal/pkg/warmlint/lint.go @@ -13,8 +13,8 @@ import ( var ( stackedPunct = regexp.MustCompile(`[!?]{2,}`) wordToken = regexp.MustCompile(`[a-z0-9%]+`) - linkPattern = regexp.MustCompile(`https?://`) - hrefPattern = regexp.MustCompile(`(?i)href\s*=\s*["']?\s*https?://`) + linkPattern = regexp.MustCompile(`https?://[^\s"'<>)\]]*`) + hrefPattern = regexp.MustCompile(`(?i)href\s*=\s*["']?\s*(https?://[^\s"'<>]*)`) htmlTag = regexp.MustCompile(`(?i)<[a-z!/][^>]*>`) imgTag = regexp.MustCompile(`(?i)]*>`) ) @@ -188,18 +188,31 @@ func isAllCaps(s string) bool { return letters >= 4 } -// countLinks counts the links the recipient can click. An anchor carries its -// URL in the href, which stripping tags throws away, so the text alone reports -// zero links for a normal HTML email; take the larger of the two counts so a -// URL used as its own anchor text is not counted twice. +// countLinks counts every anchor plus any bare URL in the text that is not +// already an anchor's destination. Stripping tags throws hrefs away, so the text +// alone reports zero links for an HTML email; matching destinations keeps a URL +// used as its own anchor text from counting twice. func countLinks(text, bodyHTML string) int { - n := len(linkPattern.FindAllString(text, -1)) - if h := len(hrefPattern.FindAllString(bodyHTML, -1)); h > n { - n = h + destinations := map[string]struct{}{} + n := 0 + for _, m := range hrefPattern.FindAllStringSubmatch(bodyHTML, -1) { + destinations[trimURL(m[1])] = struct{}{} + n++ + } + for _, u := range linkPattern.FindAllString(text, -1) { + if _, seen := destinations[trimURL(u)]; !seen { + n++ + } } return n } +// trimURL drops the sentence punctuation a URL picks up in prose, so the same +// link matches whether it was written inline or as an anchor's destination. +func trimURL(u string) string { + return strings.TrimRight(u, ".,;:!?)]}\"'") +} + func countTriggerTerms(text string) int { lower := strings.ToLower(text) found := map[string]struct{}{} diff --git a/internal/pkg/warmlint/score_test.go b/internal/pkg/warmlint/score_test.go index 3d23119e..a0dc2a5a 100644 --- a/internal/pkg/warmlint/score_test.go +++ b/internal/pkg/warmlint/score_test.go @@ -130,3 +130,32 @@ func TestScoreStillCountsPlainTextURLs(t *testing.T) { t.Errorf("five bare URLs in a plain body were not counted: %+v", res.Issues) } } + +func TestScoreCountsAnchorsAndBareURLsTogether(t *testing.T) { + // Labeled anchors and bare URLs are different destinations. Counting only + // the larger of the two sets let six distinct links score a clean 100. + body := strings.Repeat("A real sentence about the recipient's work. ", 10) + html := "

" + body + "

" + + `one two three ` + + "https://d.com/4 https://e.com/5 https://f.com/6

" + plain := body + " one two three https://d.com/4 https://e.com/5 https://f.com/6" + + res := Score("Quick question", html, plain) + if !hasIssue(res, "too_many_links") { + t.Errorf("three anchors plus three bare URLs were not counted as six: %+v", res.Issues) + } +} + +func TestScoreIgnoresTrailingPunctuationWhenMatchingAnchors(t *testing.T) { + // A URL that ends a sentence in the plain text is the same link as the + // anchor's destination, so it must not count a second time. + body := strings.Repeat("A real sentence about the recipient's work. ", 10) + html := "

" + body + "

" + + `https://a.com/1, https://b.com/2.` + "

" + plain := body + " https://a.com/1, https://b.com/2." + + res := Score("Quick question", html, plain) + if hasIssue(res, "too_many_links") { + t.Errorf("two self-linking anchors counted as more than two: %+v", res.Issues) + } +} diff --git a/internal/tasks/content_gate.go b/internal/tasks/content_gate.go index df313697..f5215679 100644 --- a/internal/tasks/content_gate.go +++ b/internal/tasks/content_gate.go @@ -23,8 +23,7 @@ func (s *tasksService) warnOnWeakContent(ctx context.Context, orgID, campaignID, if s.advanced == nil || s.campaignLogRepo == nil { return } - // Score first: it is pure CPU, and a clean 100 cannot fall below any floor - // (they are clamped to 100), so the common case never touches the database. + // A clean 100 clears every floor, so the common case never reads settings. res := warmlint.ScoreWithAttachments(subject, bodyHTML, bodyPlain, attachments) if res.Score >= 100 { return @@ -37,9 +36,8 @@ func (s *tasksService) warnOnWeakContent(ctx context.Context, orgID, campaignID, return } floor := settings.Preflight.MinContentScore + // Out of range means a row written before the floor was clamped. if floor <= 0 || floor > 100 { - // Out of range means a row written before the floor was clamped; fall - // back to the default rather than honoring a floor nothing can clear. floor = 60 } if res.Score >= floor { @@ -69,8 +67,7 @@ func (s *tasksService) warnOnWeakContent(ctx context.Context, orgID, campaignID, Message: fmt.Sprintf("Step %d's copy scores %d/100 for spam signals as sent (floor %d).%s", step, res.Score, floor, detail), Metadata: map[string]interface{}{ - // "warn" is the dashboard's amber tier; "warning" would fall - // through to the neutral info styling. + // "warn" is the dashboard's amber tier; anything else reads as info. "level": "warn", "sequence_id": seq, "score": res.Score, diff --git a/web/src/components/app/campaigns/ContentScore.tsx b/web/src/components/app/campaigns/ContentScore.tsx index cb30e476..5c45eb57 100644 --- a/web/src/components/app/campaigns/ContentScore.tsx +++ b/web/src/components/app/campaigns/ContentScore.tsx @@ -1,8 +1,7 @@ -// Advisory campaign-template content check. Scores the current subject + body -// against /templates/score and renders a 0-100 score (higher = safer) plus a -// list of non-blocking issues. It re-scores as the copy changes, on the same -// debounce the composer's live preview uses. Purely advisory: it never blocks -// saving or sending, it just surfaces deliverability hints. +// Advisory campaign-template content check: scores the current subject + body +// against /templates/score and renders a 0-100 score (higher = safer) plus the +// non-blocking issues found, re-scored on the debounce the composer's preview +// uses. It never blocks saving or sending. import * as React from "react"; import { ShieldCheckIcon, AlertTriangleIcon, AlertCircleIcon } from "lucide-react"; @@ -50,13 +49,14 @@ export default function ContentScore({ // A step with nothing written yet is not a content problem, so hold the // panel quiet rather than scoring an empty draft as spam. if (!subject.trim() && !bodyPlain.trim()) { + // Clears pending too: a cancelled request can no longer do it. setData(null); + setPending(false); setFailed(false); return; } let cancelled = false; - // Pending is set inside the timer, not on every keystroke, so the - // spinner marks a request in flight rather than flickering as you type. + // Set inside the timer so the spinner marks a request, not a keystroke. const t = setTimeout(() => { setPending(true); scoreTemplate({ subject, body_html: bodyHtml, body_plain: bodyPlain }) From 6da2c3dfaade7f8d0dc81ba87459f165a6cdcd97 Mon Sep 17 00:00:00 2001 From: Matthew Meszaros Date: Fri, 28 Aug 2026 22:05:41 -0700 Subject: [PATCH 6/9] feat: fix the three mailbox Settings bugs from #247: patch both cache shapes under ["emails", "list"] (paginated pages and the flat directory array) through one patchEmailLists helper so saving, removing, warmup lifecycle and tracking-domain writes no longer crash on undefined.map, stop rewriting the signature editor's contentEditable innerHTML on every keystroke so the caret stays where the user is typing, and scope the auth-check GET/POST handlers by organization instead of user id so the SPF/DKIM/DMARC check stops returning 404 --- internal/api/handler/email_authcheck.go | 18 +++++----- internal/app/email/handler.go | 19 +++++----- internal/app/email/service.go | 4 +-- web/src/components/app/EmailEditor.tsx | 12 +++++-- .../hooks/app/emails/patchEmailLists.test.ts | 24 +++++++++++++ .../api/hooks/app/emails/patchEmailLists.ts | 32 +++++++++++++++++ .../api/hooks/app/emails/useRemoveEmail.ts | 20 ++--------- .../api/hooks/app/emails/useUpdateEmail.ts | 20 ++--------- .../emails/useUpdateEmailTrackingDomain.ts | 36 ++++++++----------- .../hooks/app/emails/useWarmupLifecycle.ts | 20 ++--------- 10 files changed, 111 insertions(+), 94 deletions(-) create mode 100644 web/src/lib/api/hooks/app/emails/patchEmailLists.test.ts create mode 100644 web/src/lib/api/hooks/app/emails/patchEmailLists.ts diff --git a/internal/api/handler/email_authcheck.go b/internal/api/handler/email_authcheck.go index a6aa4182..53e08999 100644 --- a/internal/api/handler/email_authcheck.go +++ b/internal/api/handler/email_authcheck.go @@ -17,13 +17,15 @@ import ( // Read-only: it reports what DNS says right now and leaves the mailbox's stored // auth_state alone. Use RefreshEmailAuthCheck to record the verdict. func (h *Handler) GetEmailAuthCheck(c *gin.Context) { - userID, err := middleware.GetUserUUID(c) - if err != nil { - errx.JSON(c, errx.ErrUnauthorized) + // Mailboxes are workspace assets and the lookup behind this is scoped by + // organization, so the caller's user id would 404 for everyone. + orgID := middleware.GetOrganizationID(c) + if orgID == nil { + errx.JSON(c, errx.New(errx.BadRequest, "no organization selected")) return } - res, xerr := h.EmailService.CheckDomainAuth(c.Request.Context(), userID.String(), c.Param("id")) + res, xerr := h.EmailService.CheckDomainAuth(c.Request.Context(), orgID.String(), c.Param("id")) if xerr != nil { errx.JSON(c, xerr) return @@ -42,13 +44,13 @@ func (h *Handler) GetEmailAuthCheck(c *gin.Context) { // to change it. No Idempotency-Key: the write is derived entirely from public // DNS with no caller input, so repeating it converges on the same row. func (h *Handler) RefreshEmailAuthCheck(c *gin.Context) { - userID, err := middleware.GetUserUUID(c) - if err != nil { - errx.JSON(c, errx.ErrUnauthorized) + orgID := middleware.GetOrganizationID(c) + if orgID == nil { + errx.JSON(c, errx.New(errx.BadRequest, "no organization selected")) return } - res, xerr := h.EmailService.RefreshDomainAuth(c.Request.Context(), userID.String(), c.Param("id")) + res, xerr := h.EmailService.RefreshDomainAuth(c.Request.Context(), orgID.String(), c.Param("id")) if xerr != nil { errx.JSON(c, xerr) return diff --git a/internal/app/email/handler.go b/internal/app/email/handler.go index 962414c3..a53cc045 100644 --- a/internal/app/email/handler.go +++ b/internal/app/email/handler.go @@ -216,8 +216,8 @@ func (s *emailService) resolveTrackingDomain(ctx context.Context, domain string) // CheckDomainAuth runs a live SPF/DKIM/DMARC lookup for a mailbox's sending // domain and reports it without writing anything. -func (s *emailService) CheckDomainAuth(ctx context.Context, userID, emailAccountID string) (*dnsauth.Result, *errx.Error) { - _, res, xerr := s.resolveDomainAuth(ctx, userID, emailAccountID) +func (s *emailService) CheckDomainAuth(ctx context.Context, orgID, emailAccountID string) (*dnsauth.Result, *errx.Error) { + _, res, xerr := s.resolveDomainAuth(ctx, orgID, emailAccountID) return res, xerr } @@ -229,8 +229,8 @@ func (s *emailService) CheckDomainAuth(ctx context.Context, userID, emailAccount // their DNS would keep being blocked until the background sweep next reached // their domain, which can be a day away, and "I fixed it and nothing happened" // is how a correct gate still becomes a support incident. -func (s *emailService) RefreshDomainAuth(ctx context.Context, userID, emailAccountID string) (*dnsauth.Result, *errx.Error) { - domain, res, xerr := s.resolveDomainAuth(ctx, userID, emailAccountID) +func (s *emailService) RefreshDomainAuth(ctx context.Context, orgID, emailAccountID string) (*dnsauth.Result, *errx.Error) { + domain, res, xerr := s.resolveDomainAuth(ctx, orgID, emailAccountID) if xerr != nil { return nil, xerr } @@ -247,11 +247,12 @@ func (s *emailService) RefreshDomainAuth(ctx context.Context, userID, emailAccou return res, nil } -// resolveDomainAuth loads the caller's mailbox and runs the DNS lookup for its -// sending domain, returning the domain alongside the result so the persisting -// caller does not re-derive it. -func (s *emailService) resolveDomainAuth(ctx context.Context, userID, emailAccountID string) (string, *dnsauth.Result, *errx.Error) { - account, xerr := s.emailRepository.Get(ctx, userID, emailAccountID) +// resolveDomainAuth loads the organization's mailbox and runs the DNS lookup +// for its sending domain, returning the domain alongside the result so the +// persisting caller does not re-derive it. Get is organization-scoped: handing +// it a user id made every check 404. +func (s *emailService) resolveDomainAuth(ctx context.Context, orgID, emailAccountID string) (string, *dnsauth.Result, *errx.Error) { + account, xerr := s.emailRepository.Get(ctx, orgID, emailAccountID) if xerr != nil { return "", nil, xerr } diff --git a/internal/app/email/service.go b/internal/app/email/service.go index 13f74976..2f7dee1d 100644 --- a/internal/app/email/service.go +++ b/internal/app/email/service.go @@ -48,11 +48,11 @@ type EmailService interface { StartTrackingDomainSweep(ctx context.Context, interval, staleAfter time.Duration) // CheckDomainAuth runs a live SPF/DKIM/DMARC lookup for a mailbox's // sending domain and returns it without touching stored state. - CheckDomainAuth(ctx context.Context, userID, emailAccountID string) (*dnsauth.Result, *errx.Error) + CheckDomainAuth(ctx context.Context, orgID, emailAccountID string) (*dnsauth.Result, *errx.Error) // RefreshDomainAuth does the same and PERSISTS the verdict. That write can // lift the cold-send and warmup gate, so it sits behind the write // permission while CheckDomainAuth stays readable. - RefreshDomainAuth(ctx context.Context, userID, emailAccountID string) (*dnsauth.Result, *errx.Error) + RefreshDomainAuth(ctx context.Context, orgID, emailAccountID string) (*dnsauth.Result, *errx.Error) Delete(ctx context.Context, userID, emailAccountID string) *errx.Error // Onboarding flow diff --git a/web/src/components/app/EmailEditor.tsx b/web/src/components/app/EmailEditor.tsx index 143980eb..98a1b00e 100644 --- a/web/src/components/app/EmailEditor.tsx +++ b/web/src/components/app/EmailEditor.tsx @@ -7,7 +7,7 @@ import { RiText, RiCodeView, } from "@remixicon/react"; -import { useRef, useState } from "react"; +import { useEffect, useRef, useState } from "react"; import { cn } from "@/lib/utils"; import { TextInput } from "@/components/ui/field"; import { @@ -53,6 +53,14 @@ export default function EmailEditor({ }: EmailEditorProps) { const editorRef = useRef(null); const [activeTab, setActiveTab] = useState<"html" | "plain">("html"); + // The visual editor owns its DOM while the user types: rewriting innerHTML + // from the prop on every render resets the caret to the start, so only + // push htmlText in when it differs from what the element already holds + // (mount, switching back from the source view, an outside reset). + useEffect(() => { + const el = editorRef.current; + if (el && el.innerHTML !== htmlText) el.innerHTML = htmlText; + }, [htmlText, activeTab, code]); const [urlPopover, setUrlPopover] = useState<"link" | "image" | null>(null); const [url, setUrl] = useState(""); // The contentEditable selection is lost as soon as the popover's text @@ -263,9 +271,9 @@ export default function EmailEditor({ ref={editorRef} id={id} contentEditable + suppressContentEditableWarning onInput={(e) => commitHtml(e.currentTarget.innerHTML)} className="min-h-[120px] px-3 py-2.5 text-[13px] text-slate-800 outline-none prose prose-sm max-w-none" - dangerouslySetInnerHTML={{ __html: htmlText }} /> )} diff --git a/web/src/lib/api/hooks/app/emails/patchEmailLists.test.ts b/web/src/lib/api/hooks/app/emails/patchEmailLists.test.ts new file mode 100644 index 00000000..425a03ba --- /dev/null +++ b/web/src/lib/api/hooks/app/emails/patchEmailLists.test.ts @@ -0,0 +1,24 @@ +import { describe, expect, it } from "vitest"; +import { QueryClient } from "@tanstack/react-query"; +import patchEmailLists from "./patchEmailLists"; +import type Inbox from "@/lib/api/models/app/emails/Inbox"; + +const row = (id: string, name = id) => ({ id, name, email: `${id}@x.test` }) as unknown as Inbox; + +describe("patchEmailLists", () => { + it("patches the paginated list and the flat directory without crashing on either shape", () => { + const qc = new QueryClient(); + qc.setQueryData(["emails", "list", "", "", 20], { + pages: [{ data: [row("a"), row("b")], pagination: { has_more: false } }], + pageParams: [null], + }); + qc.setQueryData(["emails", "list", "directory"], [row("a"), row("b")]); + + patchEmailLists(qc, (rows) => rows.map((c) => (c.id === "a" ? row("a", "renamed") : c))); + + const list = qc.getQueryData<{ pages: { data: Inbox[] }[] }>(["emails", "list", "", "", 20]); + expect(list?.pages[0].data.map((c) => c.name)).toEqual(["renamed", "b"]); + const dir = qc.getQueryData(["emails", "list", "directory"]); + expect(dir?.map((c) => c.name)).toEqual(["renamed", "b"]); + }); +}); diff --git a/web/src/lib/api/hooks/app/emails/patchEmailLists.ts b/web/src/lib/api/hooks/app/emails/patchEmailLists.ts new file mode 100644 index 00000000..2a7eeee9 --- /dev/null +++ b/web/src/lib/api/hooks/app/emails/patchEmailLists.ts @@ -0,0 +1,32 @@ +import type GetEmails from "@/lib/api/models/app/emails/GetEmails"; +import type Inbox from "@/lib/api/models/app/emails/Inbox"; +import type { InfiniteData, QueryClient } from "@tanstack/react-query"; + +type EmailListCache = InfiniteData | Inbox[]; + +// Two shapes live under ["emails", "list"]: the paginated accounts list +// (InfiniteData pages) and the store's flat mailbox directory (Inbox[]). +// Patching both through one helper is what keeps a mutation's cache write +// from assuming one shape and crashing on the other. +export default function patchEmailLists(queryClient: QueryClient, patch: (rows: Inbox[]) => Inbox[]) { + const allLists = queryClient.getQueriesData({ queryKey: ["emails", "list"] }); + + for (const [key, oldData] of allLists) { + if (!oldData) continue; + + if (Array.isArray(oldData)) { + queryClient.setQueryData(key, patch(oldData)); + continue; + } + + if (!Array.isArray(oldData.pages)) continue; + + queryClient.setQueryData(key, { + ...oldData, + pages: oldData.pages.map((page) => ({ + ...page, + data: patch(page.data ?? []), + })), + }); + } +} diff --git a/web/src/lib/api/hooks/app/emails/useRemoveEmail.ts b/web/src/lib/api/hooks/app/emails/useRemoveEmail.ts index 82f8391c..ee0ef9da 100644 --- a/web/src/lib/api/hooks/app/emails/useRemoveEmail.ts +++ b/web/src/lib/api/hooks/app/emails/useRemoveEmail.ts @@ -1,6 +1,6 @@ import removeEmail from "@/lib/api/client/app/emails/removeEmail"; -import type GetEmails from "@/lib/api/models/app/emails/GetEmails"; -import { useMutation, useQueryClient, type InfiniteData } from "@tanstack/react-query"; +import { useMutation, useQueryClient } from "@tanstack/react-query"; +import patchEmailLists from "./patchEmailLists"; export default function useRemoveEmail(id: string) { const queryClient = useQueryClient(); @@ -8,21 +8,7 @@ export default function useRemoveEmail(id: string) { return useMutation({ mutationFn: () => removeEmail(id), onSuccess: () => { - const allLists = queryClient.getQueriesData>({ - queryKey: ["emails", "list"], - }); - - for (const [key, oldData] of allLists) { - if (!oldData) continue; - - queryClient.setQueryData(key, { - ...oldData, - pages: oldData.pages.map((page) => ({ - ...page, - data: page.data.filter((c) => c.id !== id), - })), - }); - } + patchEmailLists(queryClient, (rows) => rows.filter((c) => c.id !== id)); queryClient.invalidateQueries({ queryKey: ["emails", id] diff --git a/web/src/lib/api/hooks/app/emails/useUpdateEmail.ts b/web/src/lib/api/hooks/app/emails/useUpdateEmail.ts index fe78dfd9..9cd76c99 100644 --- a/web/src/lib/api/hooks/app/emails/useUpdateEmail.ts +++ b/web/src/lib/api/hooks/app/emails/useUpdateEmail.ts @@ -1,7 +1,7 @@ import updateEmail from "@/lib/api/client/app/emails/updateEmail"; -import type GetEmails from "@/lib/api/models/app/emails/GetEmails"; import type Inbox from "@/lib/api/models/app/emails/Inbox"; -import { useMutation, useQueryClient, type InfiniteData } from "@tanstack/react-query"; +import { useMutation, useQueryClient } from "@tanstack/react-query"; +import patchEmailLists from "./patchEmailLists"; export default function useUpdateEmail(id: string) { const queryClient = useQueryClient(); @@ -9,21 +9,7 @@ export default function useUpdateEmail(id: string) { return useMutation({ mutationFn: (inbox: Partial) => updateEmail(id, inbox), onSuccess: (data) => { - const allLists = queryClient.getQueriesData>({ - queryKey: ["emails", "list"], - }); - - for (const [key, oldData] of allLists) { - if (!oldData) continue; - - queryClient.setQueryData(key, { - ...oldData, - pages: oldData.pages.map((page) => ({ - ...page, - data: page.data.map((c) => c.id === id ? data : c), - })), - }); - } + patchEmailLists(queryClient, (rows) => rows.map((c) => (c.id === id ? data : c))); queryClient.setQueryData( ["emails", id], diff --git a/web/src/lib/api/hooks/app/emails/useUpdateEmailTrackingDomain.ts b/web/src/lib/api/hooks/app/emails/useUpdateEmailTrackingDomain.ts index e532b969..b5ee69cb 100644 --- a/web/src/lib/api/hooks/app/emails/useUpdateEmailTrackingDomain.ts +++ b/web/src/lib/api/hooks/app/emails/useUpdateEmailTrackingDomain.ts @@ -1,7 +1,7 @@ import updateEmailTrackingDomain from "@/lib/api/client/app/emails/updateEmailTrackingDomain"; -import type GetEmails from "@/lib/api/models/app/emails/GetEmails"; import type Inbox from "@/lib/api/models/app/emails/Inbox"; -import { useMutation, useQueryClient, type InfiniteData } from "@tanstack/react-query"; +import { useMutation, useQueryClient } from "@tanstack/react-query"; +import patchEmailLists from "./patchEmailLists"; export default function useUpdateEmailTrackingDomain(id: string) { const queryClient = useQueryClient(); @@ -9,26 +9,18 @@ export default function useUpdateEmailTrackingDomain(id: string) { return useMutation({ mutationFn: (tracking_domain: string) => updateEmailTrackingDomain(id, tracking_domain), onSuccess: (data) => { - const allLists = queryClient.getQueriesData>({ - queryKey: ["emails", "list"], - }); - - for (const [key, oldData] of allLists) { - if (!oldData) continue; - - queryClient.setQueryData(key, { - ...oldData, - pages: oldData.pages.map((page) => ({ - ...page, - data: page.data.map((c) => c.id === id ? { - ...c, - tracking_domain: data.tracking_domain, - tracking_domain_verified: data.tracking_domain_verified, - tracking_domain_verified_at: data.tracking_domain_verified_at, - } : c), - })), - }); - } + patchEmailLists(queryClient, (rows) => + rows.map((c) => + c.id === id + ? { + ...c, + tracking_domain: data.tracking_domain, + tracking_domain_verified: data.tracking_domain_verified, + tracking_domain_verified_at: data.tracking_domain_verified_at, + } + : c, + ), + ); // The card reads its target and diagnostic from this query. queryClient.setQueryData(["emails", id, "tracking-domain"], data); diff --git a/web/src/lib/api/hooks/app/emails/useWarmupLifecycle.ts b/web/src/lib/api/hooks/app/emails/useWarmupLifecycle.ts index c6baf0b0..be5fda20 100644 --- a/web/src/lib/api/hooks/app/emails/useWarmupLifecycle.ts +++ b/web/src/lib/api/hooks/app/emails/useWarmupLifecycle.ts @@ -1,7 +1,7 @@ import warmupLifecycle, { type WarmupAction } from "@/lib/api/client/app/emails/warmupLifecycle"; -import type GetEmails from "@/lib/api/models/app/emails/GetEmails"; import type Inbox from "@/lib/api/models/app/emails/Inbox"; -import { useMutation, useQueryClient, type InfiniteData } from "@tanstack/react-query"; +import { useMutation, useQueryClient } from "@tanstack/react-query"; +import patchEmailLists from "./patchEmailLists"; // Drives the flame-icon dropdown + the warmup tab's enable/pause/resume // control. Patches the mailbox into every emails list page and the single @@ -13,21 +13,7 @@ export default function useWarmupLifecycle(id: string) { return useMutation({ mutationFn: (action: WarmupAction) => warmupLifecycle(id, action), onSuccess: (data) => { - const allLists = queryClient.getQueriesData>({ - queryKey: ["emails", "list"], - }); - - for (const [key, oldData] of allLists) { - if (!oldData) continue; - - queryClient.setQueryData(key, { - ...oldData, - pages: oldData.pages.map((page) => ({ - ...page, - data: page.data.map((c) => (c.id === id ? data : c)), - })), - }); - } + patchEmailLists(queryClient, (rows) => rows.map((c) => (c.id === id ? data : c))); queryClient.setQueryData(["emails", id], data); void queryClient.invalidateQueries({ queryKey: ["analytics", "accounts", id] }); From 3d72c7386e6422f387ea5de8c996c23a9128b475 Mon Sep 17 00:00:00 2001 From: Matthew Meszaros Date: Fri, 28 Aug 2026 22:09:18 -0700 Subject: [PATCH 7/9] feat: condense the explanatory comments on the signature editor DOM sync, the patchEmailLists cache helper and the org-scoped auth-check lookup to the one-line form the repo convention asks for --- internal/api/handler/email_authcheck.go | 3 +-- internal/app/email/handler.go | 6 ++---- web/src/components/app/EmailEditor.tsx | 5 +---- web/src/lib/api/hooks/app/emails/patchEmailLists.ts | 5 +---- 4 files changed, 5 insertions(+), 14 deletions(-) diff --git a/internal/api/handler/email_authcheck.go b/internal/api/handler/email_authcheck.go index 53e08999..9b00dd71 100644 --- a/internal/api/handler/email_authcheck.go +++ b/internal/api/handler/email_authcheck.go @@ -17,8 +17,7 @@ import ( // Read-only: it reports what DNS says right now and leaves the mailbox's stored // auth_state alone. Use RefreshEmailAuthCheck to record the verdict. func (h *Handler) GetEmailAuthCheck(c *gin.Context) { - // Mailboxes are workspace assets and the lookup behind this is scoped by - // organization, so the caller's user id would 404 for everyone. + // The mailbox lookup is organization-scoped; a user id here 404s for everyone. orgID := middleware.GetOrganizationID(c) if orgID == nil { errx.JSON(c, errx.New(errx.BadRequest, "no organization selected")) diff --git a/internal/app/email/handler.go b/internal/app/email/handler.go index a53cc045..27eefb86 100644 --- a/internal/app/email/handler.go +++ b/internal/app/email/handler.go @@ -247,10 +247,8 @@ func (s *emailService) RefreshDomainAuth(ctx context.Context, orgID, emailAccoun return res, nil } -// resolveDomainAuth loads the organization's mailbox and runs the DNS lookup -// for its sending domain, returning the domain alongside the result so the -// persisting caller does not re-derive it. Get is organization-scoped: handing -// it a user id made every check 404. +// resolveDomainAuth loads the organization's mailbox (Get is org-scoped, never user-scoped) +// and runs the DNS lookup, returning the domain so the persisting caller does not re-derive it. func (s *emailService) resolveDomainAuth(ctx context.Context, orgID, emailAccountID string) (string, *dnsauth.Result, *errx.Error) { account, xerr := s.emailRepository.Get(ctx, orgID, emailAccountID) if xerr != nil { diff --git a/web/src/components/app/EmailEditor.tsx b/web/src/components/app/EmailEditor.tsx index 98a1b00e..acd8e0b9 100644 --- a/web/src/components/app/EmailEditor.tsx +++ b/web/src/components/app/EmailEditor.tsx @@ -53,10 +53,7 @@ export default function EmailEditor({ }: EmailEditorProps) { const editorRef = useRef(null); const [activeTab, setActiveTab] = useState<"html" | "plain">("html"); - // The visual editor owns its DOM while the user types: rewriting innerHTML - // from the prop on every render resets the caret to the start, so only - // push htmlText in when it differs from what the element already holds - // (mount, switching back from the source view, an outside reset). + // Only rewrite innerHTML when the prop diverges from the DOM; rewriting it every render resets the caret. useEffect(() => { const el = editorRef.current; if (el && el.innerHTML !== htmlText) el.innerHTML = htmlText; diff --git a/web/src/lib/api/hooks/app/emails/patchEmailLists.ts b/web/src/lib/api/hooks/app/emails/patchEmailLists.ts index 2a7eeee9..fd211105 100644 --- a/web/src/lib/api/hooks/app/emails/patchEmailLists.ts +++ b/web/src/lib/api/hooks/app/emails/patchEmailLists.ts @@ -4,10 +4,7 @@ import type { InfiniteData, QueryClient } from "@tanstack/react-query"; type EmailListCache = InfiniteData | Inbox[]; -// Two shapes live under ["emails", "list"]: the paginated accounts list -// (InfiniteData pages) and the store's flat mailbox directory (Inbox[]). -// Patching both through one helper is what keeps a mutation's cache write -// from assuming one shape and crashing on the other. +// ["emails", "list"] holds both the paginated list (InfiniteData) and the flat directory (Inbox[]); patch each by shape. export default function patchEmailLists(queryClient: QueryClient, patch: (rows: Inbox[]) => Inbox[]) { const allLists = queryClient.getQueriesData({ queryKey: ["emails", "list"] }); From 771bbe04e49e4347d4f9812a5060831e95ee169a Mon Sep 17 00:00:00 2001 From: Matthew Meszaros Date: Fri, 28 Aug 2026 22:42:59 -0700 Subject: [PATCH 8/9] feat: correct the comment on the partner-routing fixture's service wiring, since the org-risk read now runs ahead of the stored warmup tier and it is that read failing open, not the tier short-circuiting, that lets the selector run without an org-risk repository --- internal/tasks/warmup_partner_routing_live_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/tasks/warmup_partner_routing_live_test.go b/internal/tasks/warmup_partner_routing_live_test.go index 4faacf69..ebb1f197 100644 --- a/internal/tasks/warmup_partner_routing_live_test.go +++ b/internal/tasks/warmup_partner_routing_live_test.go @@ -125,8 +125,8 @@ func newPartnerRoutingFixture(t *testing.T) *partnerRoutingFixture { warmupRepo: repository.NewWarmupRepository(handle.Pool), emailRepo: repository.NewEmailRepostory(handle, enc), } - // WarmupPoolType short-circuits entitlement resolution, so the selector - // runs without a feature gate or org-risk repository. + // The stored tier picks the pool outright, and the risk read ahead of it + // fails open, so the selector runs with no feature gate or org-risk repo. f.sender = models.Email{ID: senderID, OrganizationID: &f.org, WarmupPoolType: "free"} return f } From 74fdd5002db0a7c8b55f08968bc222617e96ab81 Mon Sep 17 00:00:00 2001 From: Matthew Meszaros Date: Fri, 28 Aug 2026 22:46:25 -0700 Subject: [PATCH 9/9] feat: condense the new comments on PoolSpamPlacementsByProvider, its unknown bucket, the blank-domain placement filter and the summary model field to the one-line form the repo convention asks for, keeping the non-obvious constraint (the stored recipient_provider is the connect method, and a domainless placement belongs to no provider) and dropping the narration --- internal/models/warmup.go | 4 +--- internal/repository/pg_warmup.go | 14 +++++--------- 2 files changed, 6 insertions(+), 12 deletions(-) diff --git a/internal/models/warmup.go b/internal/models/warmup.go index ef26941e..758445f8 100644 --- a/internal/models/warmup.go +++ b/internal/models/warmup.go @@ -108,9 +108,7 @@ type WarmupPoolHealthSummary struct { ByState map[string]int `json:"by_state"` AvgSpamScore float64 `json:"avg_spam_score"` AvgSpamPlacement float64 `json:"avg_spam_placement_rate"` - // SpamPlacementByProvider breaks recent spam-placement counts down by who - // runs the recipient's mail, the same vocabulary partner routing reads, so - // the admin sees where warmup mail is filtered rather than one flat rate. + // Keyed by who runs the recipient's mail, the vocabulary routing reads. SpamPlacementByProvider map[string]int `json:"spam_placement_by_provider"` BlockedCount int `json:"blocked_count"` AtRiskCount int `json:"at_risk_count"` diff --git a/internal/repository/pg_warmup.go b/internal/repository/pg_warmup.go index c736a2bf..acf22dd5 100644 --- a/internal/repository/pg_warmup.go +++ b/internal/repository/pg_warmup.go @@ -862,10 +862,8 @@ func (r *warmupRepository) PoolSpamPlacementRate(ctx context.Context, since time return float64(placements) / float64(sent) * 100, nil } -// PoolSpamPlacementsByProvider returns spam-placement counts over the window -// keyed by who RUNS the recipient's mail, the same vocabulary partner routing -// reads. The stored recipient_provider column is the connect method, which -// files every custom-domain Microsoft 365 mailbox under smtp_imap. +// PoolSpamPlacementsByProvider counts spam placements in the window keyed by who +// runs the recipient's mail, not the stored connect method. func (r *warmupRepository) PoolSpamPlacementsByProvider(ctx context.Context, since time.Time) (map[string]int, error) { query := ` SELECT recipient_domain, COUNT(*) @@ -886,8 +884,7 @@ func (r *warmupRepository) PoolSpamPlacementsByProvider(ctx context.Context, sin if err := rows.Scan(&domain, &n); err != nil { return nil, err } - // Unattributable rows stay their own bucket rather than inflating - // custom, which would read as a real provider surface. + // A domainless row belongs to no provider, so it stays out of custom. key := "unknown" if domain != "" { key = string(models.ClassifyProvider(domain)) @@ -945,9 +942,8 @@ func (r *warmupRepository) SenderPlacementByProvider(ctx context.Context, sender return nil, err } - // A blank recipient domain is unattributable, and the send side can never - // produce one, so counting it would charge a numerator with no denominator - // to ProviderCustom and demote every custom-domain partner for it. + // A blank domain is unattributable and the send side never produces one, so + // counting it would demote every custom-domain partner for nobody's failure. placementRows, err := r.db.Query(ctx, ` SELECT recipient_domain, COUNT(*) FROM warmup_spam_reports