From 4ddcb24bcd8ae09af4828cab725935ab5daa3a9a Mon Sep 17 00:00:00 2001 From: Matthew Meszaros Date: Fri, 28 Aug 2026 21:54:29 -0700 Subject: [PATCH 1/2] 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 2/2] 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 })