feat: address the CodeRabbit review by carrying every stretch of stylesheet the CSS parser cannot read through as a verbatim item, since the sheet is rewritten from parsed items the moment any rule inlines and an unrelated match was deleting the rest, keeping a link's destination out of the content score now that the text renderer emits it so a CTA pointing at a free-trial page stops costing eight points, promising inlining in the editor only for a stylesheet that is actually eligible for it, skipping the client notes entirely for a plain-text campaign that ships no HTML part, switching a step into HTML mode when a template replaces its body with document markup rather than waiting for the next visual edit to gut it, accepting a pasted background shorthand only when it is a single colour so Word's "yellow none repeat scroll" stops becoming an invalid longhand, listing in SCHEMA_TAGS only the tags the mounted schema actually keeps so the warning fires for h1, font, center, thead and caption instead of staying silent while they are dropped, and correcting the guide's byte-for-byte claim and its unconditional plain-text claim

2026-09-12 08:04:41 +00:00 · 2026-09-09 08:30:49 -07:00
parent 538dc29575
commit f5eeac7b2a
11 changed files with 162 additions and 36 deletions
@@ -52,7 +52,7 @@ Cold email from a real person rarely has images. An image-heavy body reads as a

 #### Writing the HTML yourself

-The `</>` button on the right of the toolbar swaps the body for its HTML. What you type there is byte for byte what the step sends, merge fields and conditions included, so a template built elsewhere sends exactly as its designer wrote it.
+The `</>` button on the right of the toolbar swaps the body for its HTML. What you type there is stored exactly as you wrote it, merge fields and conditions included, so a template built elsewhere keeps its markup. The only thing that changes on the way out is what the send path adds or resolves: merge fields fill in, a `<style>` block is inlined onto the elements it matches, and the signature, opt-out footer and tracking are appended.

 HTML is a property of the step, not of the tab you happen to be on. A step written as markup opens as markup the next time anyone edits it, so a design cannot be quietly flattened by being reopened. Paste a whole HTML email into the visual editor and the step switches to HTML on its own and keeps the markup verbatim, because a document with its own `<head>` and `<style>` block is not something any visual editor can hold faithfully. The same happens if you copy the source of an email out of a file and paste it as text.

@@ -81,7 +81,9 @@ None of it blocks a send. It is there because writing email HTML means writing f

 Every HTML email ships with a plain-text alternative beside it, and inbox providers read it.

-For a step written in HTML, Warmbly renders that half when the email sends rather than storing one: headings and paragraphs keep their line breaks, list items are bulleted, table rows become lines, each link keeps its destination in brackets after its text, and the stylesheet and any hidden preheader are left out. The mailbox's plain-text signature and the opt-out line follow it. The Preview tab shows the result, so you can read what a text-only client gets.
+For a step written in HTML in the dashboard, Warmbly stores no plain-text half and renders one when the email sends: headings and paragraphs keep their line breaks, list items are bulleted, table rows become lines, each link keeps its destination in brackets after its text, and the stylesheet and any hidden preheader are left out. The mailbox's plain-text signature and the opt-out line follow it. The Preview tab shows the result, so you can read what a text-only client gets.
+
+This only applies when there is no plain-text body to send. Set `body_plain` through the API and that text is sent as written; leave it empty and the send path renders one from the HTML.

 ### Preview and test

@@ -7,9 +7,14 @@ import "strings"
 // @font-face, @keyframes, @supports, @import) has no single element to attach
 // to and can only stay in the <style> block.
 type cssItem struct {
-	// atRule holds the whole rule verbatim, prelude and block, when this item
-	// is an at-rule. Empty for a plain rule.
-	atRule string
+	// verbatim holds text that is carried through exactly as written: an
+	// at-rule, or a stretch the parser could not read. Empty for a plain rule.
+	//
+	// Unreadable text has to become an item rather than being dropped. The
+	// sheet is rewritten from these items as soon as one rule moves onto an
+	// element, so anything not represented here is deleted from the author's
+	// stylesheet the moment any other rule inlines.
+	verbatim string
 	// selectors is the raw selector list of a plain rule ("h1, .lead > p").
 	selectors string
 	decls     []cssDecl
@@ -35,20 +40,22 @@ func parseStylesheet(css string) []cssItem {
 			start := i
 			prelude := scanCSSUntil(css, i, "{;")
 			if prelude >= len(css) {
+				items = appendVerbatim(items, css[start:])
 				break
 			}
 			if css[prelude] == ';' {
-				items = append(items, cssItem{atRule: strings.TrimSpace(css[start : prelude+1])})
+				items = append(items, cssItem{verbatim: strings.TrimSpace(css[start : prelude+1])})
 				i = prelude + 1
 				continue
 			}
 			end := scanCSSBlock(css, prelude)
-			items = append(items, cssItem{atRule: strings.TrimSpace(css[start:end])})
+			items = append(items, cssItem{verbatim: strings.TrimSpace(css[start:end])})
 			i = end
 			continue
 		}
 		selEnd := scanCSSUntil(css, i, "{")
 		if selEnd >= len(css) {
+			items = appendVerbatim(items, css[i:])
 			break
 		}
 		blockEnd := scanCSSBlock(css, selEnd)
@@ -60,16 +67,29 @@ func parseStylesheet(css string) []cssItem {
 			innerEnd--
 		}
 		inner := css[selEnd+1 : innerEnd]
-		if selectors != "" {
-			if decls := parseDeclarations(inner); len(decls) > 0 {
-				items = append(items, cssItem{selectors: selectors, decls: decls})
-			}
+		decls := parseDeclarations(inner)
+		switch {
+		case selectors != "" && len(decls) > 0:
+			items = append(items, cssItem{selectors: selectors, decls: decls})
+		default:
+			// A rule with no selector, or one whose declarations we could not
+			// read: kept as written rather than dropped.
+			items = appendVerbatim(items, css[i:blockEnd])
 		}
 		i = blockEnd
 	}
 	return items
 }

+// appendVerbatim adds a stretch of unreadable stylesheet text, ignoring one
+// that is only whitespace.
+func appendVerbatim(items []cssItem, text string) []cssItem {
+	if t := strings.TrimSpace(text); t != "" {
+		items = append(items, cssItem{verbatim: t})
+	}
+	return items
+}
+
 // parseDeclarations reads "prop: value; prop: value" into ordered pairs.
 func parseDeclarations(block string) []cssDecl {
 	var decls []cssDecl
@@ -75,13 +75,7 @@ func InlineCSS(body string) string {
 	pending := map[*html.Node]map[string]staged{}
 	order := 0
 	for _, sheet := range sheets {
-		if mediaAttr(sheet) != "" {
-			// A print or device-scoped sheet is not what this reader sees.
-			continue
-		}
-		// The author's opt out, per stylesheet rather than per campaign: a
-		// sheet marked this way ships exactly as written.
-		if strings.EqualFold(attrOf(sheet, "data-warmbly-inline"), "false") {
+		if !sheetEligible(sheet) {
 			continue
 		}
 		items := parseStylesheet(textOf(sheet))
@@ -92,8 +86,8 @@ func InlineCSS(body string) string {
 		movedAny := false
 		var kept []string
 		for _, item := range items {
-			if item.atRule != "" {
-				kept = append(kept, item.atRule)
+			if item.verbatim != "" {
+				kept = append(kept, item.verbatim)
 				continue
 			}
 			// A selector list is split on commas, which is only safe once
@@ -170,6 +164,15 @@ func InlineCSS(body string) string {
 	return out
 }

+// sheetEligible reports whether a <style> block is one this pass will take
+// rules out of. A print or device-scoped sheet is not what the reader sees,
+// and data-warmbly-inline="false" is the author's opt out, per stylesheet
+// rather than per campaign. Shared with Lint so the editor never promises
+// inlining for a sheet that will ship exactly as written.
+func sheetEligible(sheet *html.Node) bool {
+	return mediaAttr(sheet) == "" && !strings.EqualFold(attrOf(sheet, "data-warmbly-inline"), "false")
+}
+
 // staged is the winning declaration for one property on one element, with the
 // cascade key that let it win.
 type staged struct {
@@ -107,13 +107,15 @@ func Lint(bodyHTML string, wireBytes int) []Finding {
 					externalSheet = true
 				}
 			case atom.Style:
-				hasStyle = true
+				// Only a sheet this send will actually inline; promising it
+				// for one that ships as written would be a lie in the editor.
+				hasStyle = hasStyle || sheetEligible(n)
 				for _, item := range parseStylesheet(textOf(n)) {
-					if item.atRule == "" {
+					if item.verbatim == "" {
 						noteUnsupported(unsupported, item.decls)
 						continue
 					}
-					lower := strings.ToLower(item.atRule)
+					lower := strings.ToLower(item.verbatim)
 					if strings.HasPrefix(lower, "@font-face") {
 						webFont = true
 					}
@@ -353,3 +353,38 @@ func TestInlineCSSKeepsARuleThatMatchesNothing(t *testing.T) {
 		t.Errorf("a rule matching nothing was deleted:\n%s", out)
 	}
 }
+
+// The sheet is rewritten from parsed items as soon as one rule inlines, so
+// anything the parser could not read has to survive as an item of its own or
+// it is deleted from the author's stylesheet by an unrelated rule matching.
+func TestInlineCSSKeepsUnreadableTextBesideARuleThatInlines(t *testing.T) {
+	out := InlineCSS("<style>.a{color:red}\n@weird-at-rule-we-do-not-know</style><p class=\"a\">hi</p>")
+	if !strings.Contains(out, "@weird-at-rule-we-do-not-know") {
+		t.Errorf("unreadable text was deleted once another rule inlined:\n%s", out)
+	}
+	if !strings.Contains(out, `style="color: red"`) {
+		t.Errorf("the readable rule did not inline:\n%s", out)
+	}
+}
+
+// The editor promises inlining from this finding, so it must not claim it for
+// a sheet InlineCSS will leave exactly as written.
+func TestLintPromisesInliningOnlyForSheetsThatGetIt(t *testing.T) {
+	has := func(html string) bool {
+		for _, f := range Lint(html, 500) {
+			if f.Code == "stylesheet_inlined" {
+				return true
+			}
+		}
+		return false
+	}
+	if !has(`<style>.a{color:red}</style><p class="a">x</p>`) {
+		t.Error("an ordinary stylesheet is inlined and should say so")
+	}
+	if has(`<style media="print">.a{color:red}</style><p class="a">x</p>`) {
+		t.Error("a print stylesheet is never inlined")
+	}
+	if has(`<style data-warmbly-inline="false">.a{color:red}</style><p class="a">x</p>`) {
+		t.Error("an opted-out stylesheet is never inlined")
+	}
+}
@@ -60,7 +60,7 @@ func Check(subject, body string, isReply bool) error {
 	if stackedPunct.MatchString(combined) {
 		return fmt.Errorf("stacked punctuation")
 	}
-	if n := countTriggerTerms(combined); n >= 3 {
+	if n := countTriggerTerms(withoutURLs(combined)); n >= 3 {
 		return fmt.Errorf("content has %d spam-trigger terms", n)
 	}
 	return nil
@@ -106,7 +106,7 @@ func Score(subject, bodyHTML, bodyPlain string) ScoreResult {
 	if stackedPunct.MatchString(combined) {
 		deduct(10, "warn", "stacked_punctuation", "Stacked punctuation (e.g. !!! or ?!) reads as promotional.")
 	}
-	if n := countTriggerTerms(combined); n > 0 {
+	if n := countTriggerTerms(withoutURLs(combined)); n > 0 {
 		d := n * 8
 		if d > 40 {
 			d = 40
@@ -177,6 +177,14 @@ func ScoreWithAttachments(subject, bodyHTML, bodyPlain string, attachments int)
 // It renders rather than strips tags: a regex left a <style> block's CSS
 // behind as body text, so a class named .free-trial-banner cost a designed
 // email eight points for a spam-trigger term nobody would ever read.
+// withoutURLs removes link destinations before the words are scored. The text
+// renderer keeps a link's target so the plain-text part stays usable, which
+// put URL slugs in front of the trigger-term list: a CTA pointing at
+// /free-trial cost eight points for a word no reader ever sees.
+func withoutURLs(s string) string {
+	return linkPattern.ReplaceAllString(s, " ")
+}
+
 func stripTags(s string) string {
 	return strings.TrimSpace(mailhtml.ToPlainText(s))
 }
@@ -159,3 +159,24 @@ func TestScoreIgnoresTrailingPunctuationWhenMatchingAnchors(t *testing.T) {
 		t.Errorf("two self-linking anchors counted as more than two: %+v", res.Issues)
 	}
 }
+
+// The text renderer keeps a link's destination so the plain-text part stays
+// usable, which put URL slugs in front of the trigger-term list: a CTA
+// pointing at /free-trial cost eight points for a word no reader ever sees.
+func TestScoreIgnoresWordsInsideALinkDestination(t *testing.T) {
+	plain := Score("Quick question", `<p>Hi Ana, worth a look?</p>`, "")
+	slug := Score("Quick question", `<p>Hi Ana, <a href="https://example.com/free-trial">worth a look?</a></p>`, "")
+	if slug.Score != plain.Score {
+		t.Errorf("a URL slug changed the content score: %d vs %d (%v)", slug.Score, plain.Score, slug.Issues)
+	}
+}
+
+// A stylesheet is markup machinery, never copy: a class named for a trigger
+// term must not cost a designed email anything.
+func TestScoreIgnoresAStylesheet(t *testing.T) {
+	clean := Score("Quick question", "<p>Hi Ana, ten minutes on Thursday?</p>", "")
+	styled := Score("Quick question", `<style>.free-trial-banner{color:red}</style><p>Hi Ana, ten minutes on Thursday?</p>`, "")
+	if styled.Score != clean.Score {
+		t.Errorf("a stylesheet changed the content score: %d vs %d (%v)", styled.Score, clean.Score, styled.Issues)
+	}
+}
@@ -70,8 +70,12 @@ func (s *tasksService) PreviewEmail(ctx context.Context, orgID uuid.UUID, in Ema
 	out := &EmailPreview{TemplatePreview: previewTemplatesWith(in.Subject, in.BodyHTML, in.BodyPlain, in.Contact, unsubURL)}
 	out.BodyHTML, out.BodyPlain = finishBody(out.BodyHTML, out.BodyPlain, textOnly, in.Account, optOut, unsubURL)
 	// Linted on what the author wrote, sized on what ships: the findings have
-	// to name the markup they can go and fix, but Gmail measures the wire.
-	out.HTMLFindings = mailhtml.Lint(in.BodyHTML, len(out.BodyHTML))
+	// to name the markup they can go and fix, but Gmail measures the wire. A
+	// plain-text campaign sends no HTML part at all, so there is no client
+	// left to be incompatible with and the notes would only be noise.
+	if !textOnly {
+		out.HTMLFindings = mailhtml.Lint(in.BodyHTML, len(out.BodyHTML))
+	}

 	if in.Account != nil {
 		out.From = &EmailPreviewFrom{Name: strings.TrimSpace(in.Account.Name), Email: in.Account.Email}
@@ -92,6 +92,16 @@ export default function EmailContentEditor({
    const code = onBodyCodeChange ? bodyCode : localCode;
    const setCode = onBodyCodeChange ?? setLocalCode;

+    // A body can also become a document after mount: applying a template
+    // replaces it wholesale. Whichever mode the step is in, it has to switch
+    // before the editor parses that markup through its schema, or the next
+    // visual edit saves the gutted version. The visual editor cannot produce
+    // document markup itself, so this only ever fires on a body from outside.
+    React.useEffect(() => {
+        if (!code && isDocumentBody(bodyHtml)) setCode(true);
+        // eslint-disable-next-line react-hooks/exhaustive-deps
+    }, [bodyHtml, code]);
+
    // Preview context: null contact = the built-in sample; the mailbox defaults
    // to the campaign's first enabled sender once the pool has loaded.
    const [previewContact, setPreviewContact] = React.useState<Contact | null>(null);
@@ -381,14 +381,18 @@ function prettyHTML(html: string): string {

 // The tags the visual editor's schema can hold. Anything else in HTML mode is
 // dropped the moment the editor parses it, so the user is told which ones
-// before that happens rather than after. Tables, divs and styled spans are in
-// the schema now; what is left is the machinery of a whole document, which no
-// editor schema can be faithful to.
+// before that happens rather than after.
+//
+// This list has to match the extensions actually mounted above, or the warning
+// stays silent while the switch destroys something. Headings are configured to
+// levels 2 and 3, so h1 and h4-h6 become paragraphs. Nothing mounted parses
+// font or center. The table extensions know only table, tr, td and th: thead
+// and tfoot lose their section, colgroup and col are dropped, and a caption
+// comes back as an extra row.
 const SCHEMA_TAGS = new Set([
    "p", "br", "strong", "b", "em", "i", "u", "s", "strike", "del",
-    "h1", "h2", "h3", "h4", "h5", "h6", "ul", "ol", "li", "a", "img",
-    "span", "div", "font", "center",
-    "table", "thead", "tbody", "tfoot", "tr", "td", "th", "caption", "colgroup", "col",
+    "h2", "h3", "ul", "ol", "li", "a", "img", "span", "div",
+    "table", "tbody", "tr", "td", "th",
 ]);

 function unsupportedTags(html: string): string[] {
@@ -109,6 +109,21 @@ const DEFAULT_TEXT_COLOURS = new Set([
    "rgb(0,0,0)", "rgb(17,17,17)", "rgb(34,34,34)", "rgb(51,51,51)",
 ]);

+// isColourValue accepts a single colour token: a name, a hex code, or one
+// functional form. Anything with a second top-level token is a shorthand
+// carrying more than a colour.
+function isColourValue(value: string): boolean {
+    const v = value.trim();
+    if (!v || /url\(|gradient/i.test(v)) return false;
+    let depth = 0;
+    for (const ch of v) {
+        if (ch === "(") depth++;
+        else if (ch === ")") depth = Math.max(0, depth - 1);
+        else if (/\s/.test(ch) && depth === 0) return false;
+    }
+    return true;
+}
+
 function isDefaultColour(value: string): boolean {
    return DEFAULT_TEXT_COLOURS.has(value.replace(/\s+/g, "").toLowerCase());
 }
@@ -124,9 +139,11 @@ function keptInlineStyle(el: Element): string {
        const value = decl.slice(at + 1).trim();
        if (!prop || !value) continue;
        if (prop === "color" && isDefaultColour(value)) continue;
-        // A background shorthand holding an image or a gradient is not a
-        // highlight, and the schema has nowhere to put it.
-        if (prop === "background-color" && /url\(|gradient/i.test(value)) continue;
+        // The background shorthand is only a highlight when it is nothing but
+        // a colour. Word and Outlook paste "background: yellow none repeat
+        // scroll 0% 0%", and copying that whole value into the longhand writes
+        // a declaration every client drops, losing the highlight entirely.
+        if (prop === "background-color" && !isColourValue(value)) continue;
        kept.set(prop, value);
    }
    // <font color> says the same thing in the older spelling.