From 3aed904a49bf00e42d15fb62515b64eac65e1608 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 \|, 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 — and ampersands escape so decoded text that still looks like an entity can't be decoded a second time on the next render. Cell entities are fully decoded before escaping (an encoded pipe is still a pipe), and the emitted tables are parked behind placeholders until HTMLToMarkdown's document-level unescape pass has run, so nothing double-decodes. Code spans pass through as placeholders since backslashes are literal inside code: only their pipes are escaped, entities decode before whitespace collapses (an encoded newline must not split the row), interior and edge spaces are preserved (only non-space whitespace collapses), zero-gap adjacent code elements coalesce into one span (GFM can't express them separately and one span renders identically), and the emitted fence is one backtick longer than the longest backtick run in the content, space-padded per CommonMark when the content starts or ends with a backtick or a significant space. A goldmark-backed test proves the parsed cell content survives the full HTML → Markdown → HTML cycle (backslash-pipe text, code spans with pipes, backslashes, backticks, and double spaces, entity-encoded pipes and backslashes in text and code, entity-looking literals, pipes in link destinations, adjacent tables), and pipe tables round-trip byte-identical through MarkdownToHTML → HTMLToMarkdown, alignment and the #648 blank-line separator included. Tables inside blockquotes convert within the blockquote pass, where each pipe row picks up its > prefix, and tables inside list items convert within the list pass, where each row picks up the item indent — the parked-table path would leave the quote or indent on only the first line. Captions have no GFM form, so their text surfaces as a paragraph above the grid instead of vanishing (caption tables are display-only). 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 (matched against cell attributes, so prose that mentions colspan= stays editable), captions, header cells beyond the first row, nested tables, attachments/images, styled tags (BC3 preserves color/background styles the conversion would strip), or block elements (hr included) inside the table, cells with flattened line structure (a
, multiple

/

blocks, or a partial wrapper that doesn't span the whole cell), a body cell whose align differs from its column's (GFM alignment is a header-declared column property; our own output aligns every cell with its column, so real CLI tables pass), 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 | 454 ++++++++++++++++++-- internal/richtext/richtext_test.go | 367 +++++++++++++++- 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 | 17 +- 7 files changed, 864 insertions(+), 69 deletions(-) diff --git a/internal/richtext/richtext.go b/internal/richtext/richtext.go index 1c649c441..90a4a213a 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 e1d8aa003..4031b3027 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: "
  • a
    b
", + 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
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

", 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..97b9055d4 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