Read tables back as Markdown instead of smearing their cells

internal/richtext/richtext.go

@@ -707,6 +707,23 @@ func HTMLToMarkdown(html string) string {
 		return convertCodeBlockHTML(s) + "\n\n"
 	})
 
+	// Tables — convert to GFM pipe tables while row and cell tags are still
+	// intact. Must run before the paragraph, line-break, and tag-stripping
+	// passes below, which would otherwise smear cell text together. The
+	// emitted Markdown is parked behind placeholders until every later pass
+	// has run: cell text is fully entity-decoded and escaped, so the
+	// document-level unescape would double-decode it (and could conjure
+	// unescaped pipes out of encoded ones).
+	var tables []string
+	html = reTableBlock.ReplaceAllStringFunc(html, func(s string) string {
+		md := convertTableHTML(s)
+		if md == "" {
+			return "\n\n"
+		}
+		tables = append(tables, md)
+		return "\x00tbl" + strconv.Itoa(len(tables)-1) + "\x00\n\n"
+	})
+
 	// Lists — use balanced-tag replacement to handle nesting correctly.
 	html = replaceBalancedListBlocks(html)
 
@@ -745,32 +762,7 @@ func HTMLToMarkdown(html string) string {
 	html = reHTMLStrike.ReplaceAllString(html, "~~$1~~")
 
 	// @-mentions: extract display text, render as bold (must fire before general attachment regex)
-	html = reMentionAttachment.ReplaceAllStringFunc(html, func(s string) string {
-		inner := ""
-		if match := reMentionAttachment.FindStringSubmatch(s); len(match) >= 2 {
-			inner = match[1]
-		}
-
-		name := ""
-		if match := reMentionFigcaption.FindStringSubmatch(inner); len(match) >= 2 {
-			name = strings.TrimSpace(unescapeHTML(reStripTags.ReplaceAllString(match[1], "")))
-		}
-		if name == "" {
-			if match := reMentionImgAlt.FindStringSubmatch(inner); len(match) >= 2 {
-				name = strings.TrimSpace(unescapeHTML(match[1]))
-			}
-		}
-		if name == "" {
-			name = strings.TrimSpace(unescapeHTML(reStripTags.ReplaceAllString(inner, "")))
-		}
-		if name == "" {
-			name = "mention"
-		}
-		if !strings.HasPrefix(name, "@") {
-			name = "@" + name
-		}
-		return "**" + name + "**"
-	})
+	html = reMentionAttachment.ReplaceAllStringFunc(html, mentionMarkdown)
 
 	// Basecamp attachments: <bc-attachment ... filename="report.pdf"> → 📎 report.pdf
 	html = reAttachment.ReplaceAllString(html, "\n📎 $1\n")
@@ -788,9 +780,179 @@ func HTMLToMarkdown(html string) string {
 	// Clean up multiple newlines
 	html = reMultiNewline.ReplaceAllString(html, "\n\n")
 
+	// Restore the parked tables now that no pass can touch their content.
+	for i, table := range tables {
+		html = strings.Replace(html, "\x00tbl"+strconv.Itoa(i)+"\x00", table, 1)
+	}
+
 	return strings.TrimSpace(html)
 }
 
+// mentionMarkdown converts one <bc-attachment> mention element to bold
+// Markdown, extracting the display name from the figcaption, the image alt,
+// or the remaining text, in that order.
+func mentionMarkdown(s string) string {
+	inner := ""
+	if match := reMentionAttachment.FindStringSubmatch(s); len(match) >= 2 {
+		inner = match[1]
+	}
+
+	name := ""
+	if match := reMentionFigcaption.FindStringSubmatch(inner); len(match) >= 2 {
+		name = strings.TrimSpace(unescapeHTML(reStripTags.ReplaceAllString(match[1], "")))
+	}
+	if name == "" {
+		if match := reMentionImgAlt.FindStringSubmatch(inner); len(match) >= 2 {
+			name = strings.TrimSpace(unescapeHTML(match[1]))
+		}
+	}
+	if name == "" {
+		name = strings.TrimSpace(unescapeHTML(reStripTags.ReplaceAllString(inner, "")))
+	}
+	if name == "" {
+		name = "mention"
+	}
+	if !strings.HasPrefix(name, "@") {
+		name = "@" + name
+	}
+	return "**" + name + "**"
+}
+
+// Pre-compiled regexes for HTML table conversion. BC3 rich text is sanitized
+// editor output, not arbitrary email HTML: tables are always flat
+// editor-authored grids (no nesting, no layout tables), so a non-greedy block
+// match is safe and every <table> converts — none are skipped.
+var (
+	reTableBlock     = regexp.MustCompile(`(?is)<table(?:\s[^>]*)?>.*?</table\s*>`)
+	reTableRowHTML   = regexp.MustCompile(`(?is)<tr(?:\s[^>]*)?>(.*?)</tr\s*>`)
+	reTableCellHTML  = regexp.MustCompile(`(?is)<t[hd]((?:\s[^>]*)?)>(.*?)</t[hd]\s*>`)
+	reTableCellAlign = regexp.MustCompile(`(?i)\balign="(left|center|right)"`)
+	reWhitespaceRun  = regexp.MustCompile(`\s+`)
+)
+
+// convertTableHTML converts one <table> block to a GFM pipe table. The first
+// row is the header whether its cells are <th> or <td> (GFM has no headerless
+// tables), with its align attributes — what MarkdownToHTML emits for GFM
+// column alignment — mapped back to :--- / :---: / ---: markers. The widest
+// row sizes the table and narrower rows are padded with empty cells. Cells
+// carrying colspan/rowspan emit as ordinary cells: a merged grid displays
+// better flattened than smeared, and editing such tables stays guarded by
+// HasComplexTableHTML.
+func convertTableHTML(table string) string {
+	var rows [][]string
+	var aligns []string
+	for _, row := range reTableRowHTML.FindAllStringSubmatch(table, -1) {
+		cells := reTableCellHTML.FindAllStringSubmatch(row[1], -1)
+		if len(cells) == 0 {
+			continue
+		}
+		texts := make([]string, 0, len(cells))
+		for _, cell := range cells {
+			texts = append(texts, cellMarkdown(cell[2]))
+		}
+		if rows == nil {
+			aligns = make([]string, 0, len(cells))
+			for _, cell := range cells {
+				align := ""
+				if m := reTableCellAlign.FindStringSubmatch(cell[1]); m != nil {
+					align = strings.ToLower(m[1])
+				}
+				aligns = append(aligns, align)
+			}
+		}
+		rows = append(rows, texts)
+	}
+	if rows == nil {
+		return ""
+	}
+
+	// Size the table to its widest row, not just the header: truncating a
+	// wider later row would silently drop cell data.
+	width := 0
+	for _, row := range rows {
+		width = max(width, len(row))
+	}
+	separators := make([]string, width)
+	for i := range separators {
+		switch {
+		case i < len(aligns) && aligns[i] == "left":
+			separators[i] = ":---"
+		case i < len(aligns) && aligns[i] == "center":
+			separators[i] = ":---:"
+		case i < len(aligns) && aligns[i] == "right":
+			separators[i] = "---:"
+		default:
+			separators[i] = "---"
+		}
+	}
+
+	pipeRow := func(cells []string) string {
+		for len(cells) < width {
+			cells = append(cells, "")
+		}
+		return "| " + strings.Join(cells, " | ") + " |"
+	}
+	lines := make([]string, 0, len(rows)+1)
+	lines = append(lines, pipeRow(rows[0]), pipeRow(separators))
+	for _, row := range rows[1:] {
+		lines = append(lines, pipeRow(row))
+	}
+	return strings.Join(lines, "\n")
+}
+
+// reCellEscape matches the characters that must be backslash-escaped in cell
+// text. Pipes would split the row. Backslashes must double: GFM processes
+// escapes left to right, so a lone literal `\` before an escaped pipe would
+// swallow its backslash and turn the pipe back into a delimiter.
+var reCellEscape = regexp.MustCompile(`[\\|]`)
+
+// cellMarkdown converts a table cell's inner HTML to single-line Markdown:
+// inline elements convert as usual, block boundaries (<p>, <br>, and any
+// other leftover tag) collapse to spaces, and backslashes and pipes are
+// escaped so cell text can't break the row. Code spans pass through as
+// placeholders: backslashes are literal inside code, so only their pipes are
+// escaped (GFM's row splitting honors `\|` inside code spans). Entities are
+// fully decoded here, after tags are gone and before escaping — escaping
+// must see the real characters (an encoded pipe is still a pipe, and
+// goldmark would decode it on the next render) — which is why HTMLToMarkdown
+// parks the emitted table out of reach of its document-level unescape pass.
+func cellMarkdown(inner string) string {
+	s := reMentionAttachment.ReplaceAllStringFunc(inner, mentionMarkdown)
+
+	var codes []string
+	s = reHTMLCode.ReplaceAllStringFunc(s, func(m string) string {
+		codes = append(codes, reHTMLCode.FindStringSubmatch(m)[1])
+		return "\x00" + strconv.Itoa(len(codes)-1) + "\x00"
+	})
+
+	s = reHTMLStrong.ReplaceAllString(s, "**$1**")
+	s = reHTMLB.ReplaceAllString(s, "**$1**")
+	s = reHTMLEm.ReplaceAllString(s, "*$1*")
+	s = reHTMLI.ReplaceAllString(s, "*$1*")
+	s = reHTMLLink.ReplaceAllString(s, "[$2]($1)")
+	s = reHTMLImgSA.ReplaceAllString(s, "![$2]($1)")
+	s = reHTMLImgAS.ReplaceAllString(s, "![$1]($2)")
+	s = reHTMLImgS.ReplaceAllString(s, "![]($1)")
+	s = reHTMLDel.ReplaceAllString(s, "~~$1~~")
+	s = reHTMLS.ReplaceAllString(s, "~~$1~~")
+	s = reHTMLStrike.ReplaceAllString(s, "~~$1~~")
+	s = reAttachment.ReplaceAllString(s, "📎 $1")
+	s = reAttachClose.ReplaceAllString(s, "")
+	s = reAttachNoFile.ReplaceAllString(s, "📎 attachment")
+	s = reStripTags.ReplaceAllString(s, " ")
+	s = strings.ReplaceAll(html.UnescapeString(s), "\u00a0", " ")
+	s = reCellEscape.ReplaceAllString(s, `\${0}`)
+	s = strings.TrimSpace(reWhitespaceRun.ReplaceAllString(s, " "))
+
+	for i, code := range codes {
+		code = reWhitespaceRun.ReplaceAllString(strings.TrimSpace(code), " ")
+		code = html.UnescapeString(code)
+		code = strings.ReplaceAll(code, "|", `\|`)
+		s = strings.Replace(s, "\x00"+strconv.Itoa(i)+"\x00", "`"+code+"`", 1)
+	}
+	return s
+}
+
 // reBRLine matches a <br> tag followed by an optional newline, collapsing
 // the pair to a single \n. goldmark's hard-break output is <br>\n; Trix API
 // content may have standalone <br>.
@@ -1506,15 +1668,96 @@ func IsHTML(s string) bool {
 // reTableHTML matches a real <table> tag — with attributes (`<table …>`), bare
 // (`<table>`), or self-closing (`<table/>`) — distinct from the Markdown table
 // detector. The trailing class requires a boundary after the name so longer
-// tags like <tablefoo> don't match. Used to gate the fail-closed TUI edit paths.
+// tags like <tablefoo> don't match.
 var reTableHTML = regexp.MustCompile(`(?i)<table[\s/>]`)
 
-// HasTableHTML reports whether s contains an HTML table element. The TUI in-place
-// editors use this to refuse table-bearing content: HTMLToMarkdown has no table
-// handling and would strip the structure, so those edits fail closed rather than
-// silently flatten the table on resubmit.
-func HasTableHTML(s string) bool {
-	return reTableHTML.MatchString(s)
+// reTableClose matches a closing </table> tag.
+var reTableClose = regexp.MustCompile(`(?i)</table\s*>`)
+
+// Complexity markers within a table block: merged cells; block elements,
+// captions, attachments, and images anywhere inside the table; and cells
+// whose content spans multiple paragraphs or lines. All are shapes a GFM pipe
+// table cannot represent — cellMarkdown flattens them to single-line text for
+// display, so resubmitting that text would lose the structure.
+var (
+	reTableMergedCell   = regexp.MustCompile(`(?i)\b(?:colspan|rowspan)\s*=`)
+	reTableComplexInner = regexp.MustCompile(`(?i)<(?:ul|ol|pre|blockquote|h[1-6]|caption|figure|img|bc-attachment)[\s/>]`)
+)
+
+// reTableContext matches the tags whose nesting decides whether a table sits
+// inside another block container (a blockquote or a list item). GFM pipe
+// tables are top-level only, so a table nested in either cannot round-trip.
+var reTableContext = regexp.MustCompile(`(?i)<(/?)(blockquote|li|table)[\s/>]`)
+
+// tableHasGrid reports whether a table block contains at least one row with
+// at least one cell — the minimum structure convertTableHTML can emit.
+func tableHasGrid(block string) bool {
+	for _, row := range reTableRowHTML.FindAllStringSubmatch(block, -1) {
+		if reTableCellHTML.MatchString(row[1]) {
+			return true
+		}
+	}
+	return false
+}
+
+// HasComplexTableHTML reports whether s contains a table that HTMLToMarkdown
+// cannot round-trip as a GFM pipe table: merged cells (colspan/rowspan), a
+// nested table, block content inside a cell, or the table itself nested in a
+// blockquote or list. The TUI in-place editors use this to gate edits —
+// HTMLToMarkdown still converts such tables for display, best-effort, but an
+// edit-and-resubmit would flatten the structure, so those edits fail closed.
+// Simple grids round-trip cleanly and stay editable.
+func HasComplexTableHTML(s string) bool {
+	depth := 0
+	for _, m := range reTableContext.FindAllStringSubmatch(s, -1) {
+		closing := m[1] == "/"
+		if strings.EqualFold(m[2], "table") {
+			if !closing && depth > 0 {
+				return true
+			}
+		} else if closing {
+			if depth > 0 {
+				depth--
+			}
+		} else {
+			depth++
+		}
+	}
+
+	for {
+		open := reTableHTML.FindStringIndex(s)
+		if open == nil {
+			return false
+		}
+		rest := s[open[1]:]
+		closeTag := reTableClose.FindStringIndex(rest)
+		// No closing tag: the converter can't parse the table, so nothing
+		// about the edit loop is safe. Fail closed.
+		if closeTag == nil {
+			return true
+		}
+		block := rest[:closeTag[0]]
+		if reTableHTML.MatchString(block) || reTableMergedCell.MatchString(block) {
+			return true
+		}
+		// A table the converter can't extract a grid from vanishes from the
+		// Markdown; if it holds any text, that vanishing is data loss.
+		if !tableHasGrid(block) && strings.TrimSpace(reStripTags.ReplaceAllString(block, "")) != "" {
+			return true
+		}
+		// Mentions are the one rich element cells may keep: they convert to
+		// **@Name** exactly as they do in body text. Strip them before
+		// scanning for content the conversion would lose.
+		if reTableComplexInner.MatchString(reMentionAttachment.ReplaceAllString(block, "")) {
+			return true
+		}
+		for _, cell := range reTableCellHTML.FindAllStringSubmatch(block, -1) {
+			if reBR.MatchString(cell[2]) || len(reOpeningP.FindAllString(cell[2], -1)) > 1 {
+				return true
+			}
+		}
+		s = rest[closeTag[1]:]
+	}
 }
 
 func isEscapedAt(s string, pos int) bool {