Separate contiguous paragraphs in HTML passed to the CLI

internal/richtext/richtext.go

@@ -49,6 +49,9 @@ var (
 	reP  = regexp.MustCompile(`(?is)<p(?:\s[^>]*)?>(.*?)</p>`)
 	reBR = regexp.MustCompile(`(?i)<br\s*/?\s*>`)
 	reHR = regexp.MustCompile(`(?i)<hr\s*/?\s*>`)
+	// reNbsp matches non-breaking-space entities used as visual filler in
+	// otherwise-empty paragraphs (&nbsp;, &#160;, &#xa0; / &#xA0;).
+	reNbsp = regexp.MustCompile(`(?i)&nbsp;|&#160;|&#xa0;`)
 )
 
 // Pre-compiled regexes for HTMLToMarkdown inline elements
@@ -344,14 +347,16 @@ func (r *trixRenderer) renderEscapedAt(w util.BufWriter, _ []byte, _ ast.Node, e
 
 // MarkdownToHTML converts Markdown text to HTML suitable for Basecamp's rich text fields.
 // It uses goldmark with custom AST transformations for Trix editor compatibility.
-// If the input already appears to be HTML, it is returned unchanged to preserve existing formatting.
+// If the input already appears to be HTML, it is passed through with existing
+// formatting preserved, except that a <br> separator is inserted between
+// directly adjacent paragraph blocks (see insertParagraphSeparators).
 func MarkdownToHTML(md string) string {
 	if md == "" {
 		return ""
 	}
 
 	if IsHTML(md) {
-		return md
+		return insertParagraphSeparators(md)
 	}
 
 	md = strings.ReplaceAll(md, "\r\n", "\n")
@@ -365,6 +370,77 @@ func MarkdownToHTML(md string) string {
 	return strings.TrimSpace(buf.String())
 }
 
+// insertParagraphSeparators inserts a <br> between directly adjacent, non-empty
+// paragraph blocks so that HTML supplied to the CLI renders with visible
+// paragraph spacing.
+//
+// Basecamp's rich text relies on explicit separator nodes for paragraph
+// spacing, not CSS margins: contiguous <p>A</p><p>B</p> renders squished. The
+// Markdown pipeline already inserts a separator between blank-line-separated
+// paragraphs (via TrixBreak), so this brings the raw-HTML passthrough into line
+// with that behavior. Unlike Basecamp's editor, the CLI has no concept of an
+// intentionally-tight single-line-break paragraph (its edit loop collapses that
+// distinction), so contiguous paragraphs from HTML input are treated as
+// separate paragraphs.
+//
+// The transform is byte-preserving apart from the inserted separators and is
+// idempotent: a boundary that already carries a separator — a bare <br> between
+// the paragraphs, or an empty separator paragraph (<p><br></p> or <p></p>) on
+// either side — is left untouched, so running it on already-separated content
+// (including Basecamp editor output) is a no-op. Only directly adjacent <p>
+// blocks are separated; anything between them (whitespace excepted), such as a
+// heading, list, or attachment, already provides its own break and is left
+// alone.
+func insertParagraphSeparators(s string) string {
+	locs := reP.FindAllStringIndex(s, -1)
+	if len(locs) < 2 {
+		return s
+	}
+
+	empty := make([]bool, len(locs))
+	for i, loc := range locs {
+		empty[i] = isEmptyParagraph(s[loc[0]:loc[1]])
+	}
+
+	var b strings.Builder
+	b.Grow(len(s) + len(locs)*4)
+	cursor := 0
+	for i := 0; i < len(locs); i++ {
+		end := locs[i][1]
+		b.WriteString(s[cursor:end])
+		cursor = end
+
+		if i+1 == len(locs) {
+			break
+		}
+
+		nextStart := locs[i+1][0]
+		gap := s[end:nextStart]
+		if !empty[i] && !empty[i+1] && strings.TrimSpace(gap) == "" {
+			b.WriteString(gap)
+			b.WriteString("<br>")
+			cursor = nextStart
+		}
+	}
+	b.WriteString(s[cursor:])
+	return b.String()
+}
+
+// isEmptyParagraph reports whether a <p>...</p> block has no visible content —
+// i.e. it is empty or contains only <br> tags and whitespace, including
+// non-breaking-space entities (&nbsp;, &#160;, &#xa0;) that rich text editors
+// commonly use for blank separator lines. Such paragraphs act as separators, so
+// no additional <br> is inserted adjacent to them.
+func isEmptyParagraph(block string) bool {
+	m := reP.FindStringSubmatch(block)
+	if m == nil {
+		return false
+	}
+	inner := reBR.ReplaceAllString(m[1], "")
+	inner = reNbsp.ReplaceAllString(inner, "")
+	return strings.TrimSpace(inner) == ""
+}
+
 // escapeHTML escapes special HTML characters.
 func escapeHTML(s string) string {
 	s = strings.ReplaceAll(s, "&", "&amp;")

internal/richtext/richtext_test.go

@@ -2213,3 +2213,142 @@ func TestParsedAttachmentDisplayURL(t *testing.T) {
 		})
 	}
 }
+
+func TestMarkdownToHTMLInsertsParagraphSeparators(t *testing.T) {
+	tests := []struct {
+		name     string
+		input    string
+		expected string
+	}{
+		{
+			name:     "two contiguous paragraphs get a separator",
+			input:    "<p>Line 1</p><p>Line 2</p>",
+			expected: "<p>Line 1</p><br><p>Line 2</p>",
+		},
+		{
+			name:     "three contiguous paragraphs get separators between each",
+			input:    "<p>A</p><p>B</p><p>C</p>",
+			expected: "<p>A</p><br><p>B</p><br><p>C</p>",
+		},
+		{
+			name:     "paragraphs with attributes are separated",
+			input:    `<p dir="auto">A</p><p dir="auto">B</p>`,
+			expected: `<p dir="auto">A</p><br><p dir="auto">B</p>`,
+		},
+		{
+			name:     "whitespace-only gap is preserved and separator added",
+			input:    "<p>A</p>\n<p>B</p>",
+			expected: "<p>A</p>\n<br><p>B</p>",
+		},
+		{
+			name:     "existing bare br separator is left untouched (idempotent)",
+			input:    "<p>A</p><br><p>B</p>",
+			expected: "<p>A</p><br><p>B</p>",
+		},
+		{
+			name:     "empty separator paragraph is left untouched (Lexxy canonical)",
+			input:    "<p>A</p><p><br></p><p>B</p>",
+			expected: "<p>A</p><p><br></p><p>B</p>",
+		},
+		{
+			name:     "empty paragraph separator is left untouched",
+			input:    "<p>A</p><p></p><p>B</p>",
+			expected: "<p>A</p><p></p><p>B</p>",
+		},
+		{
+			name:     "nbsp entity separator paragraph is left untouched",
+			input:    "<p>A</p><p>&nbsp;</p><p>B</p>",
+			expected: "<p>A</p><p>&nbsp;</p><p>B</p>",
+		},
+		{
+			name:     "numeric nbsp separator paragraph is left untouched",
+			input:    "<p>A</p><p>&#160;</p><p>B</p>",
+			expected: "<p>A</p><p>&#160;</p><p>B</p>",
+		},
+		{
+			name:     "hex nbsp separator paragraph is left untouched",
+			input:    "<p>A</p><p>&#xa0;</p><p>B</p>",
+			expected: "<p>A</p><p>&#xa0;</p><p>B</p>",
+		},
+		{
+			name:     "single paragraph is unchanged",
+			input:    "<p>Only one</p>",
+			expected: "<p>Only one</p>",
+		},
+		{
+			name:     "heading between paragraphs is left alone",
+			input:    "<p>A</p><h3>H</h3><p>B</p>",
+			expected: "<p>A</p><h3>H</h3><p>B</p>",
+		},
+		{
+			name:     "list between paragraphs is left alone",
+			input:    "<p>A</p><ul><li>x</li></ul><p>B</p>",
+			expected: "<p>A</p><ul><li>x</li></ul><p>B</p>",
+		},
+		{
+			name:     "non-paragraph HTML is unchanged",
+			input:    "<div><strong>Hi</strong></div>",
+			expected: "<div><strong>Hi</strong></div>",
+		},
+		{
+			name:     "paragraph with inline br is still non-empty and separated",
+			input:    "<p>A<br>C</p><p>B</p>",
+			expected: "<p>A<br>C</p><br><p>B</p>",
+		},
+		{
+			name:     "leading empty paragraph is left alone",
+			input:    "<p></p><p>A</p>",
+			expected: "<p></p><p>A</p>",
+		},
+		{
+			name:     "mix of separated and contiguous only fills the gap that lacks a separator",
+			input:    "<p>A</p><p>B</p><br><p>C</p>",
+			expected: "<p>A</p><br><p>B</p><br><p>C</p>",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result := MarkdownToHTML(tt.input)
+			if result != tt.expected {
+				t.Errorf("MarkdownToHTML(%q)\ngot:  %q\nwant: %q", tt.input, result, tt.expected)
+			}
+		})
+	}
+}
+
+// Running the separator insertion on its own output must be a no-op.
+func TestMarkdownToHTMLParagraphSeparatorsIdempotent(t *testing.T) {
+	inputs := []string{
+		"<p>Line 1</p><p>Line 2</p>",
+		"<p>A</p><p>B</p><p>C</p>",
+		`<p dir="auto">A</p><p dir="auto">B</p>`,
+		"<p>A</p>\n<p>B</p>",
+		"<p>A</p><p><br></p><p>B</p>",
+		"<p>A</p><h3>H</h3><p>B</p>",
+	}
+
+	for _, in := range inputs {
+		t.Run(in, func(t *testing.T) {
+			once := MarkdownToHTML(in)
+			twice := MarkdownToHTML(once)
+			if once != twice {
+				t.Errorf("not idempotent for %q\nonce:  %q\ntwice: %q", in, once, twice)
+			}
+		})
+	}
+}
+
+// A blank-line Markdown paragraph break and the equivalent contiguous-<p> HTML
+// must converge on the same separated shape.
+func TestMarkdownToHTMLParagraphSeparatorsMatchMarkdownPath(t *testing.T) {
+	fromMarkdown := MarkdownToHTML("Line 1\n\nLine 2")
+	fromHTML := MarkdownToHTML("<p>Line 1</p><p>Line 2</p>")
+
+	if !strings.Contains(fromMarkdown, "<br>") {
+		t.Fatalf("markdown path unexpectedly produced no <br>: %q", fromMarkdown)
+	}
+	if fromHTML != "<p>Line 1</p><br><p>Line 2</p>" {
+		t.Errorf("HTML path = %q, want %q", fromHTML, "<p>Line 1</p><br><p>Line 2</p>")
+	}
+}