diff --git a/internal/richtext/richtext.go b/internal/richtext/richtext.go index 1c649c44..90a4a213 100644 --- a/internal/richtext/richtext.go +++ b/internal/richtext/richtext.go @@ -708,8 +708,28 @@ func HTMLToMarkdown(html string) string { }) // Lists ā€” use balanced-tag replacement to handle nesting correctly. + // Runs before the parked table pass below: list items convert their + // tables inline (see formatListItem) so each pipe row picks up the item + // indent a parked one-line placeholder would deny it. html = replaceBalancedListBlocks(html) + // 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" + }) + // Paragraphs html = reP.ReplaceAllString(html, "$1\n\n") @@ -745,32 +765,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: ā†’ šŸ“Ž report.pdf html = reAttachment.ReplaceAllString(html, "\nšŸ“Ž $1\n") @@ -788,9 +783,242 @@ 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 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 converts ā€” none are skipped. +var ( + reTableBlock = regexp.MustCompile(`(?is)]*)?>.*?`) + reTableRowHTML = regexp.MustCompile(`(?is)]*)?>(.*?)`) + reTableCellHTML = regexp.MustCompile(`(?is)]*)?)>(.*?)`) + reTableCellAlign = regexp.MustCompile(`(?i)\balign="(left|center|right)"`) + reTableCaption = regexp.MustCompile(`(?is)]*)?>(.*?)`) + reWhitespaceRun = regexp.MustCompile(`\s+`) +) + +// convertTableHTML converts one
block to a GFM pipe table. The first +// row is the header whether its cells are : the first row is promoted to + // the header ā€” GFM has no headerless tables. + name: "td-only table", + input: "
or (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 { + caption := "" + if m := reTableCaption.FindStringSubmatch(table); m != nil { + caption = cellMarkdown(m[1]) + } + + 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 { + aligns = append(aligns, cellAlign(cell[1])) + } + } + rows = append(rows, texts) + } + if rows == nil { + return caption + } + + // 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)) + } + out := strings.Join(lines, "\n") + // GFM has no table captions; emit the text as a paragraph above the grid + // rather than dropping user-visible content. Caption-bearing tables stay + // display-only (HasComplexTableHTML), so this never has to round-trip. + if caption != "" { + out = caption + "\n\n" + out + } + return out +} + +// cellAlign extracts the whitelisted align attribute value from a cell's +// open-tag attributes, or "" when unaligned. +func cellAlign(attrs string) string { + if m := reTableCellAlign.FindStringSubmatch(attrs); m != nil { + return strings.ToLower(m[1]) + } + return "" +} + +// 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. Ampersands +// would let decoded text that still looks like an entity (say a literal +// |) be decoded a second time by goldmark on the next render. +var reCellEscape = regexp.MustCompile(`[\\|&]`) + +// reCodeNonSpaceWS matches whitespace other than plain spaces. Code-span cell +// content must be one line, but interior spaces are significant in GFM code +// spans, so only these collapse. +var reCodeNonSpaceWS = regexp.MustCompile(`[\t\n\r\f\v]+`) + +// reAdjacentCode matches the boundary between two code elements with nothing +// separating them. +var reAdjacentCode = regexp.MustCompile(`(?i)]*)?>`) + +// codeSpanMarkdown wraps code content in a backtick fence long enough to +// survive backticks in the content, with CommonMark's space padding when the +// content begins or ends with a backtick ā€” or with a space, since the parser +// strips exactly one leading/trailing space pair from padded spans and would +// otherwise eat a significant edge space. All-space content needs no padding: +// the strip rule exempts it. +func codeSpanMarkdown(content string) string { + longest, run := 0, 0 + for _, r := range content { + if r == '`' { + run++ + longest = max(longest, run) + } else { + run = 0 + } + } + delim := strings.Repeat("`", longest+1) + pad := "" + edgeSpace := (strings.HasPrefix(content, " ") || strings.HasSuffix(content, " ")) && + strings.TrimSpace(content) != "" + if strings.HasPrefix(content, "`") || strings.HasSuffix(content, "`") || edgeSpace { + pad = " " + } + return delim + pad + content + pad + delim +} + +// cellMarkdown converts a table cell's inner HTML to single-line Markdown: +// inline elements convert as usual, block boundaries (

,
, 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) + + // Adjacent code elements with no gap coalesce into one span: GFM cannot + // express them separately (`a``b` pairs the outer fences instead), and + // the rendered output of one span is identical. + s = reAdjacentCode.ReplaceAllString(s, "") + + 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 { + // Decode before collapsing so entity-encoded newlines ( ) can't + // slip through and split the row. No TrimSpace: edge spaces are + // significant in code spans ā€” codeSpanMarkdown pads so the parser's + // strip restores them. + code = reCodeNonSpaceWS.ReplaceAllString(html.UnescapeString(code), " ") + code = strings.ReplaceAll(code, "|", `\|`) + s = strings.Replace(s, "\x00"+strconv.Itoa(i)+"\x00", codeSpanMarkdown(code), 1) + } + return s +} + // reBRLine matches a
tag followed by an optional newline, collapsing // the pair to a single \n. goldmark's hard-break output is
\n; Trix API // content may have standalone
. @@ -799,6 +1027,16 @@ var reBRLine = regexp.MustCompile(`(?i)\n?`) // formatListItem converts a list item's HTML content to Markdown, handling //
tags as indented continuation lines. func formatListItem(prefix, indent, content string) string { + // Tables inside list items convert here, like quoted tables convert in + // the blockquote pass: the indentation below applies per line, so the + // pipe rows must already be in place. Editing stays blocked by + // HasComplexTableHTML. + content = reTableBlock.ReplaceAllStringFunc(content, func(s string) string { + if md := convertTableHTML(s); md != "" { + return "\n" + md + "\n" + } + return "" + }) content = strings.TrimSpace(content) content = reBRLine.ReplaceAllString(content, "\n") lines := strings.Split(content, "\n") @@ -1006,6 +1244,19 @@ func blockquoteInnerToMarkdown(inner string) string { content = reCodeBlock.ReplaceAllStringFunc(content, func(s string) string { return convertCodeBlockHTML(s) + "\n\n" }) + // Quoted tables convert here, not in HTMLToMarkdown's parked table pass: + // the quote prefixes each line below, so the pipe rows must already be in + // place. The emitted cells then pass through the document-level unescape, + // which is inert for them ā€” cell text is decoded once here and its + // ampersands are escaped, so no entity survives to decode again. Editing + // quoted tables stays blocked by HasComplexTableHTML regardless. + content = reTableBlock.ReplaceAllStringFunc(content, func(s string) string { + md := convertTableHTML(s) + if md == "" { + return "\n\n" + } + return md + "\n\n" + }) content = replaceBalancedListBlocks(content) // Replace

with double newline (paragraph break) to separate adjacent blocks, // then strip

openers. Two passes so

para1

para2

produces @@ -1506,15 +1757,152 @@ func IsHTML(s string) bool { // reTableHTML matches a real tag ā€” with attributes (`
`), bare // (`
`), or self-closing (`
`) ā€” distinct from the Markdown table // detector. The trailing class requires a boundary after the name so longer -// tags like don't match. Used to gate the fail-closed TUI edit paths. +// tags like don't match. var reTableHTML = regexp.MustCompile(`(?i)]`) -// 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
tag. +var reTableClose = regexp.MustCompile(`(?i)`) + +// Complexity markers within a table block: merged cells (matched against a +// cell's open-tag attributes, so prose that merely mentions colspan= stays +// simple); block elements, captions, attachments, and images anywhere inside +// the table; header cells beyond the first row (GFM has exactly one header +// row); and cells whose content spans multiple paragraphs, divs, or lines. +// All are shapes a GFM pipe table cannot represent ā€” cellMarkdown flattens +// them to single-line text for display, so resubmitting would lose the +// structure. +var ( + reTableMergedCell = regexp.MustCompile(`(?i)\b(?:colspan|rowspan)\s*=`) + reTableComplexInner = regexp.MustCompile(`(?i)<(?:ul|ol|pre|blockquote|h[1-6]|hr|caption|figure|img|bc-attachment)[\s/>]`) + reTableTH = regexp.MustCompile(`(?i)]`) + reOpeningDiv = regexp.MustCompile(`(?i)]*)?>`) + // BC3's sanitizer preserves color/background-color styles; conversion + // strips them, so a styled tag anywhere in the table is data an edit + // would lose. + reTableStyledTag = regexp.MustCompile(`(?i)<[^>]*\bstyle\s*=`) +) + +// reWholeCellWrapper matches cell content that is exactly one

or

+// element wrapping everything ā€” the shape whose boundary costs nothing to +// flatten. +var reWholeCellWrapper = regexp.MustCompile(`(?is)^(?:]*)?>.*|]*)?>.*)$`) + +// cellFlattensLineStructure reports whether a cell's inner HTML carries line +// structure cellMarkdown would flatten to spaces: any
, more than one +// block wrapper, or a single

/

that doesn't wrap the entire cell. +func cellFlattensLineStructure(inner string) bool { + if reBR.MatchString(inner) { + return true + } + blocks := len(reOpeningP.FindAllString(inner, -1)) + len(reOpeningDiv.FindAllString(inner, -1)) + if blocks > 1 { + return true + } + return blocks == 1 && !reWholeCellWrapper.MatchString(strings.TrimSpace(inner)) +} + +// 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) { + 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. + stripped := reMentionAttachment.ReplaceAllString(block, "") + if reTableComplexInner.MatchString(stripped) || reTableStyledTag.MatchString(stripped) { + return true + } + var headerAligns []string + for ri, row := range reTableRowHTML.FindAllStringSubmatch(block, -1) { + // convertTableHTML promotes only the first row to the GFM header; + // a
in any later row would be demoted to a plain cell. + if ri > 0 && reTableTH.MatchString(row[1]) { + return true + } + for ci, cell := range reTableCellHTML.FindAllStringSubmatch(row[1], -1) { + if reTableMergedCell.MatchString(cell[1]) { + return true + } + if cellFlattensLineStructure(cell[2]) { + return true + } + // GFM alignment is a column property declared by the header + // row; a later cell whose align differs from its column's + // cannot round-trip. Our own MarkdownToHTML output aligns + // every cell with its column, so real CLI tables pass. + align := cellAlign(cell[1]) + if ri == 0 { + headerAligns = append(headerAligns, align) + } else { + want := "" + if ci < len(headerAligns) { + want = headerAligns[ci] + } + if align != want { + return true + } + } + } + } + s = rest[closeTag[1]:] + } } func isEscapedAt(s string, pos int) bool { diff --git a/internal/richtext/richtext_test.go b/internal/richtext/richtext_test.go index e1d8aa00..4031b302 100644 --- a/internal/richtext/richtext_test.go +++ b/internal/richtext/richtext_test.go @@ -922,6 +922,48 @@ func TestEditLoopRoundTrip(t *testing.T) { markdown: "# Title\n\nSome **bold** text.\n\n- Item 1\n- Item 2\n\n> A quote\n\n```\ncode\n```", expected: "# Title\n\nSome **bold** text.\n\n- Item 1\n- Item 2\n\n> A quote\n\n```\ncode\n```", }, + { + name: "table", + markdown: "| Foo | Bar |\n| --- | --- |\n| Baz | Qux |", + expected: "| Foo | Bar |\n| --- | --- |\n| Baz | Qux |", + }, + { + name: "table with alignment", + markdown: "| L | C | R |\n| :--- | :---: | ---: |\n| a | b | c |", + expected: "| L | C | R |\n| :--- | :---: | ---: |\n| a | b | c |", + }, + { + // The #648 separator (


) before the table must come back + // as the blank line it encodes. + name: "paragraph then table", + markdown: "Intro.\n\n| a | b |\n| --- | --- |\n| c | d |", + expected: "Intro.\n\n| a | b |\n| --- | --- |\n| c | d |", + }, + { + name: "table cell with escaped pipe after backslash", + markdown: "| h |\n| --- |\n| a\\\\\\|b |", + expected: "| h |\n| --- |\n| a\\\\\\|b |", + }, + { + name: "table cell with code span containing a pipe", + markdown: "| h |\n| --- |\n| `a\\|b` |", + expected: "| h |\n| --- |\n| `a\\|b` |", + }, + { + name: "table cell with ampersand", + markdown: "| h |\n| --- |\n| AT\\&T |", + expected: "| h |\n| --- |\n| AT\\&T |", + }, + { + name: "table cell with code span containing a backtick", + markdown: "| h |\n| --- |\n| ``a`b`` |", + expected: "| h |\n| --- |\n| ``a`b`` |", + }, + { + name: "table cell with space-padded code span", + markdown: "| h |\n| --- |\n| ` a ` |", + expected: "| h |\n| --- |\n| ` a ` |", + }, } for _, tt := range tests { @@ -935,6 +977,279 @@ func TestEditLoopRoundTrip(t *testing.T) { } } +func TestHTMLToMarkdownTable(t *testing.T) { + tests := []struct { + name string + input string + expected string + }{ + { + name: "thead th and tbody td", + input: "\n\n\n\n\n\n\n\n\n\n\n\n\n
FooBar
BazQux
", + expected: "| Foo | Bar |\n| --- | --- |\n| Baz | Qux |", + }, + { + // Trix-style grid with no
ab
cd
", + expected: "| a | b |\n| --- | --- |\n| c | d |", + }, + { + name: "alignment attributes", + input: `
LCR
abc
`, + expected: "| L | C | R |\n| :--- | :---: | ---: |\n| a | b | c |", + }, + { + name: "pipe in cell text is escaped", + input: "
a|bc
", + expected: "| a\\|b | c |\n| --- | --- |", + }, + { + name: "inline formatting in cells", + input: `
WhoWhat
bold and italiccode and link
`, + expected: "| Who | What |\n| --- | --- |\n| **bold** and *italic* | `code` and [link](https://example.com) |", + }, + { + name: "mention in cell", + input: `
Owner
Jane Doe
Jane Doe
`, + expected: "| Owner |\n| --- |\n| **@Jane Doe** |", + }, + { + name: "multi-paragraph cell joins with spaces", + input: "
Notes

one

two

", + expected: "| Notes |\n| --- |\n| one two |", + }, + { + name: "br in cell joins with spaces", + input: "
Notes
one
two
", + expected: "| Notes |\n| --- |\n| one two |", + }, + { + name: "ragged row padded to header width", + input: "
ab
c
", + expected: "| a | b |\n| --- | --- |\n| c | |", + }, + { + // Truncating would silently drop cell data, so the widest row + // sizes the table. + name: "row wider than header widens the table", + input: "
a
bc
", + expected: "| a | |\n| --- | --- |\n| b | c |", + }, + { + // A literal backslash must double, or GFM's left-to-right escape + // processing would pair it with the escaped pipe's backslash and + // turn the pipe back into a delimiter. + name: "backslash before pipe in cell text", + input: `
a\|bc
`, + expected: "| a\\\\\\|b | c |\n| --- | --- |", + }, + { + // Backslashes are literal inside code spans, so only the pipe is + // escaped there. + name: "code span with pipe and backslash", + input: `
a|bc\d
`, + expected: "| `a\\|b` | `c\\d` |\n| --- | --- |", + }, + { + name: "table between paragraphs", + input: "

Intro.

\n


\n\n\n\n\n\n\n\n\n\n\n\n
a
b
\n


\n

After.

", + expected: "Intro.\n\n| a |\n| --- |\n| b |\n\nAfter.", + }, + { + // Best-effort display of shapes GFM can't represent: merged cells + // emit as ordinary cells (editing stays guarded by + // HasComplexTableHTML). + name: "colspan cell emits as ordinary cell", + input: `
ab
wide
`, + expected: "| a | b |\n| --- | --- |\n| wide | |", + }, + { + // Quoted tables convert inside the blockquote pass so every pipe + // row carries the quote prefix; editing them stays guarded. + name: "table inside blockquote keeps the quote", + input: "
a
b
", + expected: "> | a |\n> | --- |\n> | b |", + }, + { + // List-nested tables convert inside the list pass so every pipe + // row carries the item indent; editing them stays guarded. + name: "table inside list item keeps the indent", + input: "", + expected: "- | a |\n | --- |\n | b |", + }, + { + // GFM has no captions; the text surfaces as a paragraph above the + // grid instead of vanishing. Editing stays guarded. + name: "caption becomes a leading paragraph", + input: "
Quarterly results
a
b
", + expected: "Quarterly results\n\n| a |\n| --- |\n| b |", + }, + { + name: "empty table vanishes", + input: "

Before.

After.

", + expected: "Before.\n\nAfter.", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := HTMLToMarkdown(tt.input) + if result != tt.expected { + t.Errorf("HTMLToMarkdown(%q)\ngot: %q\nwant: %q", tt.input, result, tt.expected) + } + }) + } +} + +// TestHTMLToMarkdownTableGoldmarkRoundTrip verifies cell CONTENT survives a +// full HTML -> Markdown -> HTML cycle by checking what goldmark parses back +// out of the emitted pipe table ā€” not just the Markdown bytes. This is what +// proves the escaping actually escapes: a broken escape still produces +// plausible-looking Markdown, but goldmark splits the row differently. +func TestHTMLToMarkdownTableGoldmarkRoundTrip(t *testing.T) { + tests := []struct { + name string + input string + wantInHTML []string + wantTables int + }{ + { + name: "backslash before pipe in text", + input: `
h
a\|b
`, + wantInHTML: []string{`a\|b`}, + wantTables: 1, + }, + { + name: "code span containing a pipe", + input: "
h
a|b
", + wantInHTML: []string{"a|b"}, + wantTables: 1, + }, + { + name: "code span containing a backslash", + input: `
h
a\b
`, + wantInHTML: []string{`a\b`}, + wantTables: 1, + }, + { + // goldmark percent-encodes the escaped pipe in the destination; + // the URL is equivalent and the row stays intact. + name: "link destination containing a pipe", + input: `
h
t
`, + wantInHTML: []string{`t`}, + wantTables: 1, + }, + { + // An encoded pipe is still a pipe: goldmark decodes the entity on + // the next render, so cellMarkdown must decode-then-escape or the + // entity smuggles an unescaped pipe into the cell. + name: "entity-encoded pipe in text", + input: "
h
a|b
", + wantInHTML: []string{"a|b"}, + wantTables: 1, + }, + { + name: "entity-encoded backslash before pipe", + input: "
h
a\|b
", + wantInHTML: []string{`a\|b`}, + wantTables: 1, + }, + { + name: "entity-encoded pipe inside code", + input: "
h
a|b
", + wantInHTML: []string{"a|b"}, + wantTables: 1, + }, + { + // A decoded literal that still looks like an entity must not be + // decoded a second time: the escaped ampersand keeps it literal. + name: "entity-looking literal survives", + input: "
h
&#124;
", + wantInHTML: []string{"&#124;"}, + wantTables: 1, + }, + { + name: "code span containing a backtick", + input: "
h
a`b
", + wantInHTML: []string{"a`b"}, + wantTables: 1, + }, + { + name: "code span keeps interior spaces", + input: "
h
a b
", + wantInHTML: []string{"a b"}, + wantTables: 1, + }, + { + // Edge spaces are significant in code spans: the emission pads so + // the parser's one-space strip restores the original content. + name: "code span keeps edge spaces", + input: "
h
a
", + wantInHTML: []string{" a "}, + wantTables: 1, + }, + { + name: "code span keeps a trailing space", + input: "
h
a
", + wantInHTML: []string{"a "}, + wantTables: 1, + }, + { + name: "all-space code span survives", + input: "
h
", + wantInHTML: []string{" "}, + wantTables: 1, + }, + { + // Decode-then-collapse: an entity-encoded newline must not split + // the row. + name: "code span with entity-encoded newline", + input: "
h
a b
", + wantInHTML: []string{"a b"}, + wantTables: 1, + }, + { + // Zero-gap adjacent code elements coalesce: GFM can't express + // them separately, and one span renders identically. + name: "adjacent code spans coalesce", + input: "
h
ab
", + wantInHTML: []string{"ab"}, + wantTables: 1, + }, + { + // goldmark honors the escaped pipe inside code spans, backslash + // context included ā€” the row must not split here. + name: "code span with backslash before pipe", + input: `
h
a\|b
`, + wantInHTML: []string{`a\|b`}, + wantTables: 1, + }, + { + name: "adjacent tables stay separate", + input: "
one
\n


\n
two
", + wantInHTML: []string{"one", "two"}, + wantTables: 2, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + md := HTMLToMarkdown(tt.input) + html := MarkdownToHTML(md) + for _, want := range tt.wantInHTML { + if !strings.Contains(html, want) { + t.Errorf("round-trip HTML missing %q\nmarkdown: %q\nhtml: %q", want, md, html) + } + } + if got := strings.Count(html, "x", expected: true}, - {name: "uppercase table tag with attrs", input: `
`, expected: true}, - {name: "self-closing table tag", input: "", expected: true}, - {name: "wrapped table", input: "
", expected: true}, - {name: "plain text", input: "just some text", expected: false}, - {name: "pipe markdown", input: "| a | b |\n| --- | --- |\n| 1 | 2 |", expected: false}, + {name: "colspan cell", input: `
x
`, expected: true}, + {name: "rowspan cell", input: `
xy
`, expected: true}, + {name: "uppercase colspan", input: `
x
`, expected: true}, + {name: "whitespace around colspan equals", input: `
x
`, expected: true}, + {name: "colspan mentioned in cell text stays simple", input: "
Set colspan=2 here
", expected: false}, + {name: "second header row", input: "
A
B
", expected: true}, + {name: "div-separated cell", input: "
one
two
", expected: true}, + {name: "single-div cell stays simple", input: "
one
", expected: false}, + {name: "partially wrapped cell", input: "
before

inside

after
", expected: true}, + {name: "hr in cell", input: "
a
b
", expected: true}, + {name: "body cell align conflicts with column", input: `
h
x
`, expected: true}, + {name: "body cell align matches column stays simple", input: `
h
x
`, expected: false}, + {name: "styled span in cell", input: `
urgent
`, expected: true}, + {name: "plain span in cell stays simple", input: "
plain
", expected: false}, + {name: "nested table", input: "
x
", expected: true}, + {name: "list in cell", input: "
  • x
", expected: true}, + {name: "ordered list in cell", input: "
  1. x
", expected: true}, + {name: "code block in cell", input: "
x
", expected: true}, + {name: "blockquote in cell", input: "
x
", expected: true}, + {name: "heading in cell", input: "

x

", expected: true}, + {name: "plain grid", input: "
a
x
", expected: false}, + {name: "grid with inline formatting", input: "
x and y
", expected: false}, + {name: "no table at all", input: "

colspan= is mentioned outside a table

  • x
", expected: false}, + {name: "block after simple table", input: "
x
  • y
", expected: false}, + {name: "second table is complex", input: `
x
y
`, expected: true}, + {name: "caption", input: "
c
x
", expected: true}, + {name: "rowless table with content", input: "
orphan
", expected: true}, + {name: "cellless row with content", input: "orphan
", expected: true}, + {name: "empty table stays simple", input: "
", expected: false}, + {name: "unclosed table", input: "", expected: true}, + {name: "image in cell", input: `
x
a
`, expected: true}, + {name: "attachment in cell", input: `
`, expected: true}, + {name: "mention in cell stays simple", input: `
Jane
Jane
`, expected: false}, + {name: "multi-paragraph cell", input: "

a

b

", expected: true}, + {name: "br in cell", input: "
a
b
", expected: true}, + {name: "single-paragraph cell stays simple", input: "

a

", expected: false}, + {name: "table inside blockquote", input: "
x
", expected: true}, + {name: "table inside list item", input: "
  • x
", expected: true}, + {name: "blockquote then separate table", input: "
quote
x
", expected: false}, {name: "word starting with table", input: "the tablet is here", expected: false}, + {name: "pipe markdown", input: "| a | b |\n| --- | --- |\n| 1 | 2 |", expected: false}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if got := HasTableHTML(tt.input); got != tt.expected { - t.Errorf("HasTableHTML(%q) = %v, want %v", tt.input, got, tt.expected) + if got := HasComplexTableHTML(tt.input); got != tt.expected { + t.Errorf("HasComplexTableHTML(%q) = %v, want %v", tt.input, got, tt.expected) } }) } diff --git a/internal/tui/workspace/views/detail.go b/internal/tui/workspace/views/detail.go index 6a68b3a2..6966338c 100644 --- a/internal/tui/workspace/views/detail.go +++ b/internal/tui/workspace/views/detail.go @@ -757,9 +757,9 @@ func (v *Detail) startCommentEdit() tea.Cmd { return nil } c := v.data.comments[v.focusedComment] - // Fail closed on table-bearing content (see startEditBody). - if richtext.HasTableHTML(c.content) { - return workspace.SetStatus("This comment contains a table ā€” edit it on Basecamp web", true) + // Fail closed on complex tables (see startEditBody). + if richtext.HasComplexTableHTML(c.content) { + return workspace.SetStatus("This comment contains a table too complex to edit as Markdown ā€” edit it on Basecamp web", true) } v.editingComment = true v.commentEditComposer = widget.NewComposer(v.styles, @@ -1078,10 +1078,12 @@ func (v *Detail) startEditBody() tea.Cmd { if v.data == nil { return nil } - // Fail closed on table-bearing content: HTMLToMarkdown has no table handling, - // so entering edit mode and resubmitting would strip the table. Block the edit. - if richtext.HasTableHTML(v.data.content) { - return workspace.SetStatus("This message contains a table ā€” edit it on Basecamp web", true) + // Fail closed on complex tables ā€” shapes a GFM pipe table can't + // represent (see richtext.HasComplexTableHTML): HTMLToMarkdown flattens + // them, so an edit-and-resubmit would lose structure. Simple grids + // round-trip and stay editable. + if richtext.HasComplexTableHTML(v.data.content) { + return workspace.SetStatus("This message contains a table too complex to edit as Markdown ā€” edit it on Basecamp web", true) } v.editingBody = true v.bodyEditComposer = widget.NewComposer(v.styles, diff --git a/internal/tui/workspace/views/detail_test.go b/internal/tui/workspace/views/detail_test.go index 1f743d01..a13b3990 100644 --- a/internal/tui/workspace/views/detail_test.go +++ b/internal/tui/workspace/views/detail_test.go @@ -573,12 +573,18 @@ func TestDetail_CommentEdit_Ignored_WhenNoFocus(t *testing.T) { assert.False(t, v.editingComment) } -const tableHTML = "
" + +// A simple grid round-trips through HTMLToMarkdown and stays editable; a +// merged-cell grid cannot be represented as a GFM pipe table, so its edits +// fail closed. +const simpleTableHTML = "
Foo
" + "
Foo
Baz
" -func TestDetail_EditBody_BlockedForTable(t *testing.T) { +const complexTableHTML = "
" + + "
FooBar
Baz
" + +func TestDetail_EditBody_BlockedForComplexTable(t *testing.T) { v := testDetailWithSession("Message", false) - v.data.content = tableHTML + v.data.content = complexTableHTML cmd := v.startEditBody() assert.False(t, v.editingBody, "must not enter edit mode on table content") @@ -601,10 +607,35 @@ func TestDetail_EditBody_EntersForNonTable(t *testing.T) { assert.NotNil(t, cmd) } -func TestDetail_CommentEdit_BlockedForTable(t *testing.T) { +func TestDetail_EditBody_EntersForSimpleTable(t *testing.T) { + v := testDetailWithSession("Message", false) + v.data.content = simpleTableHTML + + cmd := v.startEditBody() + assert.True(t, v.editingBody, "should enter edit mode on simple-table content") + require.NotNil(t, v.bodyEditComposer, "composer should be built") + assert.NotNil(t, cmd) + assert.Equal(t, "| Foo |\n| --- |\n| Baz |", v.bodyEditComposer.Value(), + "composer should hold the table as Markdown") +} + +func TestDetail_CommentEdit_EntersForSimpleTable(t *testing.T) { + v := detailWithComments() + v.focusedComment = 0 + v.data.comments[0].content = simpleTableHTML + + cmd := v.startCommentEdit() + assert.True(t, v.editingComment, "should enter edit mode on simple-table content") + require.NotNil(t, v.commentEditComposer, "composer should be built") + assert.NotNil(t, cmd) + assert.Equal(t, "| Foo |\n| --- |\n| Baz |", v.commentEditComposer.Value(), + "composer should hold the table as Markdown") +} + +func TestDetail_CommentEdit_BlockedForComplexTable(t *testing.T) { v := detailWithComments() v.focusedComment = 0 - v.data.comments[0].content = tableHTML + v.data.comments[0].content = complexTableHTML cmd := v.startCommentEdit() assert.False(t, v.editingComment, "must not enter edit mode on table content") diff --git a/internal/tui/workspace/views/todos.go b/internal/tui/workspace/views/todos.go index 06935996..88ced03b 100644 --- a/internal/tui/workspace/views/todos.go +++ b/internal/tui/workspace/views/todos.go @@ -1042,10 +1042,12 @@ func (v *Todos) startEditDescription() tea.Cmd { } } - // Fail closed on table-bearing content: HTMLToMarkdown has no table handling, - // so entering edit mode and resubmitting would strip the table. Block the edit. - if richtext.HasTableHTML(description) { - return workspace.SetStatus("This to-do description contains a table ā€” edit it on Basecamp web", true) + // Fail closed on complex tables ā€” shapes a GFM pipe table can't + // represent (see richtext.HasComplexTableHTML): HTMLToMarkdown flattens + // them, so an edit-and-resubmit would lose structure. Simple grids + // round-trip and stay editable. + if richtext.HasComplexTableHTML(description) { + return workspace.SetStatus("This to-do description contains a table too complex to edit as Markdown ā€” edit it on Basecamp web", true) } v.editingDesc = true diff --git a/internal/tui/workspace/views/todos_test.go b/internal/tui/workspace/views/todos_test.go index a876b14d..66a45877 100644 --- a/internal/tui/workspace/views/todos_test.go +++ b/internal/tui/workspace/views/todos_test.go @@ -942,20 +942,23 @@ func TestTodos_BoostTarget_IncludesAccountID(t *testing.T) { assert.Equal(t, int64(42), picker.Target.ProjectID) } -// --- Edit description: table fail-closed guard --- +// --- Edit description: complex-table fail-closed guard --- -const todoTableHTML = "
" + +const todoSimpleTableHTML = "
Foo
" + "
Foo
Baz
" -func TestTodos_EditDescription_BlockedForTable(t *testing.T) { +const todoComplexTableHTML = "
" + + "
FooBar
Baz
" + +func TestTodos_EditDescription_BlockedForComplexTable(t *testing.T) { v := testTodosViewWithTodos() todos := sampleTodos() - todos[0].Description = todoTableHTML + todos[0].Description = todoComplexTableHTML v.session.Hub().Todos(42, 10).Set(todos) cmd := v.startEditDescription() - assert.False(t, v.editingDesc, "must not enter edit mode on table content") + assert.False(t, v.editingDesc, "must not enter edit mode on complex-table content") require.NotNil(t, cmd, "should return a status command") status, ok := cmd().(workspace.StatusMsg) @@ -977,6 +980,21 @@ func TestTodos_EditDescription_EntersForNonTable(t *testing.T) { assert.NotNil(t, cmd) } +func TestTodos_EditDescription_EntersForSimpleTable(t *testing.T) { + v := testTodosViewWithTodos() + v.descComposer = widget.NewComposer(v.styles, widget.WithMode(widget.ComposerRich)) + + todos := sampleTodos() + todos[0].Description = todoSimpleTableHTML + v.session.Hub().Todos(42, 10).Set(todos) + + cmd := v.startEditDescription() + assert.True(t, v.editingDesc, "should enter edit mode on simple-table content") + assert.NotNil(t, cmd) + assert.Equal(t, "| Foo |\n| --- |\n| Baz |", v.descComposer.Value(), + "composer should hold the table as Markdown, line structure intact") +} + // newTextInputWithValue creates a textinput with a preset value for testing. func newTextInputWithValue(val string) textinput.Model { ti := textinput.New() diff --git a/skills/basecamp/SKILL.md b/skills/basecamp/SKILL.md index 59b7b309..97b9055d 100644 --- a/skills/basecamp/SKILL.md +++ b/skills/basecamp/SKILL.md @@ -97,12 +97,17 @@ Full CLI coverage: 155 endpoints across todos, cards, messages, files, schedule, - **`@Name` / `@First.Last`** ā€” fuzzy name resolution (may be ambiguous) For todos, documents, and cards, content is sent as-is ā€” use plain text or HTML directly. - **Table boundary:** GFM tables render in message/comment bodies, but the TUI - in-place editors **refuse to open** table-bearing content (edit it on Basecamp - web, or replace the whole field via `messages update` / `comments update` / - `todos update --description`, which take fresh content and are unaffected), and - human-readable CLI/TUI **display** of such content may lose table structure ā€” - both pending server-side Markdown support (BC3 #11986). + **Table boundary:** GFM tables round-trip: they render in message/comment + bodies, display converts them back to pipe tables, and the TUI in-place + editors open simple grids for editing. Only **complex** tables ā€” merged + cells (colspan/rowspan), captions, extra header rows, nested tables, + attachments/images or block content inside cells, multi-paragraph or + multi-line cells, or a table inside a blockquote or list ā€” refuse to open, + since a GFM pipe table can't represent those shapes (edit them on Basecamp + web, or replace the + whole field via `messages update` / `comments update` / `todos update + --description`, which take fresh content and are unaffected). Complex + tables still **display** best-effort, flattened to a plain grid. **Multiline / non-ASCII content:** do not rely on bash ANSI-C quoting (`$'...\n...'`) ā€” it is a bash/zsh extension. Under a POSIX `/bin/sh` (dash, busybox-ash, common in sandboxes) the `$` is passed through literally and posts a stray leading `$`, and `\n` stays a literal backslash-n. Pipe the content via stdin instead, using `-` as the content argument: ```bash