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 })