diff --git a/internal/richtext/richtext.go b/internal/richtext/richtext.go index 1c649c44..910cd1fd 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 e1d8aa00..7e8d8491 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`}, + 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, + }, + { + 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

  • 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..14e29500 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