From cceb8a7010c660a8a25bd86bcd225bd1a4cf9f9f Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 22 Aug 2026 01:27:41 -0700 Subject: [PATCH] Read tables back as Markdown instead of smearing their cells MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit HTMLToMarkdown had no table handling: markup survived every pass untouched until the final tag-stripping regex deleted the tags and ran the cell text together. Display of any table-bearing message, comment, or to-do description came out as one smeared line, and the TUI in-place editors had to refuse table-bearing content wholesale to avoid destroying it on resubmit. Convert tables to GFM pipe tables instead. A new pass runs while row and cell tags are still intact: each
block is extracted (BC3 rich text is sanitized editor output — always flat editor-authored grids, so a non-greedy block match is safe), rows and cells are pulled out by regex, and the block is emitted as a pipe table. The first row is the header whether its cells are `}, + wantTables: 1, + }, + { + name: "code span containing a pipe", + input: "
or (GFM has no headerless tables); align attributes — exactly what MarkdownToHTML emits for GFM column alignment — map back to :--- / :---: / ---: markers; the widest row sizes the table, with narrower rows padded, so no row is ever truncated. Cell content runs through the same inline conversions as body text (bold, italic, code, links, strikethrough, mentions, attachments) and block boundaries collapse to spaces. Escaping is GFM-exact: pipes escape as \| and literal backslashes double — GFM processes escapes left to right, so a lone \ before an escaped pipe would swallow its backslash and turn the pipe back into a delimiter. Code spans pass through as placeholders since backslashes are literal inside code; only their pipes are escaped. Cell entities are fully decoded before escaping — an encoded pipe is still a pipe, and goldmark would decode | on the next render — and the emitted tables are parked behind placeholders until HTMLToMarkdown's document-level unescape pass has run, so nothing double-decodes. A goldmark-backed test proves the parsed cell content survives the full HTML → Markdown → HTML cycle (backslash-pipe text, code spans with pipes and backslashes, entity-encoded pipes and backslashes in text and code, pipes in link destinations, adjacent tables), and pipe tables round-trip byte-identical through MarkdownToHTML → HTMLToMarkdown, alignment and the #648 blank-line separator included. Shapes GFM can't represent still display, best-effort — colspan/rowspan cells emit as ordinary cells, a merged grid displaying better flattened than smeared — but editing them stays blocked: the blanket HasTableHTML gate on the three TUI in-place editors is replaced by HasComplexTableHTML, which fails closed on merged cells, captions, nested tables, attachments/images or block elements inside the table, cells spanning multiple paragraphs or lines, a table nested in a blockquote or list, unclosed tables, and tables the converter can't extract a grid from (they'd otherwise vanish with their content) — checking every table in the content, not just the first. Mentions are the one rich element cells may keep: they convert to **@Name** exactly as they do in body text. Simple grids open for editing like any other content. The mention-conversion closure moves to a named mentionMarkdown function so cell conversion can reuse it; behavior is unchanged. Editing simple tables in the todos view requires the composer Reset fix (previous commit): without it, Reset dropped the description composer to single-line mode and SetValue flattened the freshly-converted pipe table. The todos test asserts the exact multiline table to prove the pairing. --- internal/richtext/richtext.go | 309 +++++++++++++++++--- internal/richtext/richtext_test.go | 257 +++++++++++++++- internal/tui/workspace/views/detail.go | 16 +- internal/tui/workspace/views/detail_test.go | 41 ++- internal/tui/workspace/views/todos.go | 10 +- internal/tui/workspace/views/todos_test.go | 28 +- skills/basecamp/SKILL.md | 16 +- 7 files changed, 608 insertions(+), 69 deletions(-) diff --git a/internal/richtext/richtext.go b/internal/richtext/richtext.go index 1c649c441..910cd1fd9 100644 --- a/internal/richtext/richtext.go +++ b/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: → 📎 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 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)"`) + 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 { + 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 (

,
, 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
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
. @@ -1506,15 +1668,96 @@ 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; 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 { diff --git a/internal/richtext/richtext_test.go b/internal/richtext/richtext_test.go index e1d8aa003..7e8d8491c 100644 --- a/internal/richtext/richtext_test.go +++ b/internal/richtext/richtext_test.go @@ -922,6 +922,33 @@ 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` |", + }, } for _, tt := range tests { @@ -935,6 +962,194 @@ 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 | |", + }, + { + 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
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, + }, + { + 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: "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

", expected: false}, + {name: "block after simple table", input: "
x
", 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: "", 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 6a68b3a26..6966338c1 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 1f743d019..a13b39902 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 069359965..88ced03b0 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 a876b14dc..66a45877c 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 59b7b3094..14e29500e 100644 --- a/skills/basecamp/SKILL.md +++ b/skills/basecamp/SKILL.md @@ -97,12 +97,16 @@ 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, 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