Read tables back as Markdown instead of smearing their cells - #650
Conversation
github-actions
Bot
added
tui
There was a problem hiding this comment.
Pull request overview
Converts Basecamp HTML tables back to GFM Markdown and permits safe table editing in the TUI.
Changes:
- Adds HTML-table conversion with formatting, alignment, and round-trip tests.
- Replaces blanket table guards with complex-table detection.
- Updates TUI tests and skill guidance.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
skills/basecamp/SKILL.md |
Documents table conversion and editing boundaries. |
internal/tui/workspace/views/todos.go |
Allows simple table description editing. |
internal/tui/workspace/views/todos_test.go |
Tests to-do table guards. |
internal/tui/workspace/views/detail.go |
Updates message and comment table guards. |
internal/tui/workspace/views/detail_test.go |
Tests detail-view table editing. |
internal/richtext/richtext.go |
Implements conversion and complexity detection. |
internal/richtext/richtext_test.go |
Adds conversion and round-trip coverage. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| html = reTableBlock.ReplaceAllStringFunc(html, func(s string) string { | ||
| return convertTableHTML(s) + "\n\n" | ||
| }) |
| // Markdown edit loop would flatten them. | ||
| var ( | ||
| reTableMergedCell = regexp.MustCompile(`(?i)\b(?:colspan|rowspan)\s*=`) | ||
| reTableBlockInCell = regexp.MustCompile(`(?i)<(?:ul|ol|pre|blockquote|h[1-6])[\s/>]`) |
| if closeTag != nil { | ||
| block = rest[:closeTag[0]] | ||
| } | ||
| if reTableHTML.MatchString(block) || reTableMergedCell.MatchString(block) || reTableBlockInCell.MatchString(block) { |
| // inside cells. Both are shapes a GFM pipe table cannot represent, so a | ||
| // Markdown edit loop would flatten them. | ||
| var ( | ||
| reTableMergedCell = regexp.MustCompile(`(?i)\b(?:colspan|rowspan)\s*=`) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a4f14d597
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Fail closed on complex tables (merged cells, nested tables, blocks in | ||
| // cells): HTMLToMarkdown flattens those shapes, so an edit-and-resubmit | ||
| // would lose structure. Simple grids round-trip and stay editable. | ||
| if richtext.HasComplexTableHTML(description) { |
There was a problem hiding this comment.
When a simple table is edited as a to-do description, this newly relaxed guard lets execution reach descComposer.Reset(), which switches the composer to single-line quick mode; the subsequent SetValue therefore flattens the generated pipe-table newlines. Saving without making any changes sends a single line such as | Foo | | --- | | Baz |, which MarkdownToHTML no longer recognizes as a table and silently replaces the stored table with a paragraph. Restore rich mode before populating the composer or continue blocking tables on this edit path.
Useful? React with 👍 / 👎.
| // Markdown edit loop would flatten them. | ||
| var ( | ||
| reTableMergedCell = regexp.MustCompile(`(?i)\b(?:colspan|rowspan)\s*=`) | ||
| reTableBlockInCell = regexp.MustCompile(`(?i)<(?:ul|ol|pre|blockquote|h[1-6])[\s/>]`) |
There was a problem hiding this comment.
For a table cell containing <p> or <br> content, this predicate returns false even though cellMarkdown collapses those boundaries to spaces. The message, comment, and to-do editors consequently allow the table to be saved, and even an unchanged submission irreversibly rewrites <p>one</p><p>two</p> or one<br>two as one two. Include these lossy cell shapes in the complexity check, or preserve their boundaries in the emitted Markdown.
Useful? React with 👍 / 👎.
| if len(row) > width { | ||
| row = row[:width] |
There was a problem hiding this comment.
When a table has a later row with more cells than its first row, this conversion silently discards every extra cell, while HasComplexTableHTML still classifies the table as editable. Opening and saving such a message, comment, or to-do therefore permanently deletes cell contents even if the user makes no changes. Treat inconsistent row widths as complex for edit guarding, or expand the emitted header width instead of truncating data.
Useful? React with 👍 / 👎.
5a4f14d to
fc1e379
Compare
fc1e379 to
f5514c5
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Suppressed comments (2)
internal/richtext/richtext.go:932
- Using a single backtick delimiter corrupts valid code-span cells that contain a literal backtick. For example, HTML containing
<code>abis emitted as ``ab``, which no longer parses as the original code span; becauseHasComplexTableHTMLpermits inline code, an unchanged edit can lose formatting. Choose a delimiter longer than the longest backtick run in the content and apply CommonMark code-span padding rules.
for i, code := range codes {
code = reWhitespaceRun.ReplaceAllString(strings.TrimSpace(code), " ")
code = strings.ReplaceAll(code, "|", `\|`)
s = strings.Replace(s, "\x00"+strconv.Itoa(i)+"\x00", "`"+code+"`", 1)
internal/richtext/richtext.go:1664
- This pattern scans the entire table block, so ordinary cell text such as
set colspan=2is misclassified as a merged cell and the otherwise-simple table is refused by every editor. Restrict the match toth/tdstart-tag attributes (or apply an attribute-only pattern tocell[1]) rather than matching text content.
reTableMergedCell = regexp.MustCompile(`(?i)\b(?:colspan|rowspan)\s*=`)
| // 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) { |
| // column alignment — mapped back to :--- / :---: / ---: markers. Later rows | ||
| // are padded or truncated to the header's width. Cells carrying | ||
| // colspan/rowspan emit as ordinary cells: a merged grid displays better | ||
| // flattened than smeared, and editing such tables stays guarded by |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f5514c5cab
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for i, code := range codes { | ||
| code = reWhitespaceRun.ReplaceAllString(strings.TrimSpace(code), " ") | ||
| code = strings.ReplaceAll(code, "|", `\|`) | ||
| s = strings.Replace(s, "\x00"+strconv.Itoa(i)+"\x00", "`"+code+"`", 1) |
There was a problem hiding this comment.
When a simple table contains a code span with a literal backtick, such as HTML generated from ``a`b``, this always re-emits the cell as `a`b`, so Goldmark closes the span at the embedded backtick. Opening and saving the table unchanged therefore changes the code contents and markup; select a delimiter longer than the longest backtick run and apply the required padding.
Useful? React with 👍 / 👎.
| // 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/>]`) |
There was a problem hiding this comment.
The new paragraph/line checks still let a cell such as <td><div>one</div><div>two</div></td> through because div is absent from this pattern, even though it is recognized as safe rich-text HTML elsewhere. cellMarkdown then replaces both tags with spaces, and an unchanged edit resubmits one two, permanently losing the block boundary; classify these block containers as complex or preserve their boundaries.
Useful? React with 👍 / 👎.
| // 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*=`) |
There was a problem hiding this comment.
A simple table is incorrectly blocked from all TUI edit paths whenever ordinary cell text contains colspan= or rowspan=—for example, <td>Set colspan=2 in the template</td> matches this unscoped regex even though no cell is merged. Limit the check to attributes on opening th/td tags so textual mentions of these attribute names do not force users to edit the content on the web.
Useful? React with 👍 / 👎.
| html = reTableBlock.ReplaceAllStringFunc(html, func(s string) string { | ||
| return convertTableHTML(s) + "\n\n" | ||
| }) |
There was a problem hiding this comment.
When a table is nested in a blockquote, the blockquote pass runs first and prefixes the still-HTML table as a single line; this later replacement inserts a multiline pipe table while leaving > only on its first row. The separator and body rows therefore escape the quote and Goldmark cannot parse the result as the nested table that produced the HTML, so CLI/TUI display no longer renders the promised best-effort grid. Convert the table within the blockquote before splitting and prefixing its lines.
Useful? React with 👍 / 👎.
| for _, cell := range reTableCellHTML.FindAllStringSubmatch(block, -1) { | ||
| if reBR.MatchString(cell[2]) || len(reOpeningP.FindAllString(cell[2], -1)) > 1 { | ||
| return true | ||
| } |
There was a problem hiding this comment.
A valid table such as <thead><tr><th>A</th></tr><tr><th>B</th></tr></thead> passes this complexity scan, but convertTableHTML promotes only the first row to the GFM header. Saving an unchanged edit consequently recreates the second row with <td> cells in <tbody>, permanently losing its header semantics; detect additional header rows (or any later th cells) and keep these tables on the complex-table path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Suppressed comments (2)
internal/tui/workspace/views/todos.go:1049
- This guard now admits simple tables, but this editor immediately calls
Reset(), which switches the composer to quick mode, and thenSetValue()puts the multiline pipe table into the single-line text input. The line breaks are flattened, so submitting without making any edit no longer parses as a table and destroys it. Restore rich mode before pre-populating Markdown (for example, useInsertPaste, which expands on multiline/Markdown input) before allowing this path.
if richtext.HasComplexTableHTML(description) {
internal/richtext/richtext.go:823
- This doc comment contradicts the implementation below: rows are no longer truncated to the header width; the widest row determines the width and narrower rows are padded. Update it so callers do not infer that cell data may be discarded.
// column alignment — mapped back to :--- / :---: / ---: markers. Later rows
// are padded or truncated to the header's width. Cells carrying
// colspan/rowspan emit as ordinary cells: a merged grid displays better
| for _, cell := range reTableCellHTML.FindAllStringSubmatch(block, -1) { | ||
| if reBR.MatchString(cell[2]) || len(reOpeningP.FindAllString(cell[2], -1)) > 1 { | ||
| return true | ||
| } | ||
| } |
| for i, code := range codes { | ||
| code = reWhitespaceRun.ReplaceAllString(strings.TrimSpace(code), " ") | ||
| code = strings.ReplaceAll(code, "|", `\|`) | ||
| s = strings.Replace(s, "\x00"+strconv.Itoa(i)+"\x00", "`"+code+"`", 1) |
HTMLToMarkdown had no table handling: <table> 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 <table> 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 <th> or <td> (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.
f5514c5 to
cceb8a7
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Suppressed comments (1)
internal/richtext/richtext.go:1683
- This pattern scans the entire table body, so ordinary cell text such as
document colspan= hereis mistaken for a merged-cell attribute and the TUI unnecessarily blocks an otherwise simple table. Restrict the match toth/tdopening tags.
reTableMergedCell = regexp.MustCompile(`(?i)\b(?:colspan|rowspan)\s*=`)
| // 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(`[\\|]`) |
| 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) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cceb8a7010
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| s = strings.ReplaceAll(html.UnescapeString(s), "\u00a0", " ") | ||
| s = reCellEscape.ReplaceAllString(s, `\${0}`) |
There was a problem hiding this comment.
When a simple cell contains literal Markdown punctuation, such as HTML produced from the valid GFM input \*literal\*, this pass emits *literal* because it escapes only backslashes and pipes. Opening and saving the table unchanged therefore converts the literal asterisks into emphasis; angle-bracket text can similarly become raw HTML. Escape Markdown metacharacters in text nodes while preserving the Markdown deliberately generated for inline elements.
Useful? React with 👍 / 👎.
| for i, code := range codes { | ||
| code = reWhitespaceRun.ReplaceAllString(strings.TrimSpace(code), " ") | ||
| code = html.UnescapeString(code) |
There was a problem hiding this comment.
For a simple table containing an inline code span with repeated whitespace, such as <code>a b</code>, this collapses the code content to a b. GFM code spans preserve interior repeated spaces, so an unchanged TUI edit permanently changes the code value; normalize line endings as required for code spans, but do not collapse all whitespace runs.
Useful? React with 👍 / 👎.
| s = reHTMLB.ReplaceAllString(s, "**$1**") | ||
| s = reHTMLEm.ReplaceAllString(s, "*$1*") | ||
| s = reHTMLI.ReplaceAllString(s, "*$1*") | ||
| s = reHTMLLink.ReplaceAllString(s, "[$2]($1)") |
There was a problem hiding this comment.
When a simple table originates from valid GFM such as [docs](https://example.com "API docs"), MarkdownToHTML stores the title on the anchor, but this replacement reconstructs only its label and destination. Opening and saving the table unchanged therefore permanently removes the link title; include the optional title when emitting the Markdown link or classify such anchors as complex.
Useful? React with 👍 / 👎.
Stacked on #651 — the composer Reset fix is a prerequisite: without it, the todos description composer drops to single-line mode on
Reset()and flattens the freshly-converted pipe table onSetValue(). This PR's base iscomposer-reset-keeps-mode; merge #651 first.Follow-up to #637/#648.
HTMLToMarkdownhad no table handling:<table>markup survived every conversion pass untouched until the final tag-stripping regex deleted the tags and ran the cell text together. Any table-bearing message, comment, or to-do description displayed as one smeared line, and the TUI in-place editors refused table-bearing content wholesale to avoid destroying it on resubmit.What changed
Converter. A new pass converts each
<table>block to a GFM pipe table while row/cell tags are still intact. BC3 rich text is sanitized editor output — always flat editor-authored grids, no nesting, no layout tables — so a non-greedy regex extractor is consistent and sufficient; no DOM walker, no new dependency. Emission shape:<th>or<td>(GFM has no headerless tables); the widest row sizes the table and narrower rows are padded — no row is ever truncated, so no cell data is dropped.alignattributes — exactly whatMarkdownToHTMLemits for GFM column alignment viaTableCellAlignAttribute— map back to:---/:---:/---:.bc-attachmentmentions →**@Name**); block boundaries collapse to spaces.\|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 — backslashes are literal inside code, so only their pipes are escaped.|) is still a pipe — goldmark decodes it on the next render — so cell text is fully entity-decoded once tags are gone, then escaped. The emitted tables are parked behind placeholders untilHTMLToMarkdown's document-level unescape pass has run, so nothing double-decodes.MarkdownToHTML→HTMLToMarkdown(asserted inTestEditLoopRoundTrip, alignment and the Restore the lost blank line before Markdown tables #648 separator included), and a goldmark-backed test (TestHTMLToMarkdownTableGoldmarkRoundTrip) verifies the parsed cell content survives the full HTML → Markdown → HTML cycle: backslash-before-pipe text, code spans containing pipes and backslashes, pipes in link destinations, adjacent tables.Guards. The blanket
HasTableHTMLgate on the three TUI in-place editors is replaced byHasComplexTableHTML, which fails closed on every shape the pipe-table round trip can't preserve — checking every table in the content, not just the first:colspan/rowspan, matched case- and whitespace-insensitively)> | a | b |markdown)ul/ol/pre/blockquote/headings) inside the table<br>(the converter flattens those to one line for display)Mentions are the one rich element cells may keep: they convert to
**@Name**exactly as they already do in body text, so editing them loses no more than any body-text edit does. Simple grids now open for editing like any other content, and the guard message names the real blocker ("too complex to edit as Markdown").The mention-conversion closure is extracted to a named
mentionMarkdownfunction so cell conversion reuses it — behavior unchanged. The skill's "Table boundary" paragraph is updated to match the new behavior.Testing
TestHTMLToMarkdownTable: header promotion, alignment, escaping (pipe, backslash-before-pipe, code spans), inline formatting and mentions in cells, multi-paragraph/<br>cells (display), ragged rows padded and wide rows widening the table,<p><br></p>separators, best-effort colspan display, empty table.TestHTMLToMarkdownTableGoldmarkRoundTrip: parsed-content verification through goldmark, not just Markdown bytes — including entity-encoded pipes and backslashes in text and in code spans.TestEditLoopRoundTrip: five new byte-identical cases (plain, aligned, paragraph-then-table, escaped-pipe-after-backslash, code-span-with-pipe).TestHasComplexTableHTMLreplacesTestHasTableHTML(the blanket predicate is folded into the new one) with cases covering both halves of every rule.bin/cigreen.