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" | ||
| }) |
There was a problem hiding this comment.
Flagged for human review — declining as converter-wide normalization rather than a table defect: every adjacent block pair normalizes to the durable separator on a round trip (adjacent
s gain
![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".
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*=`)
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".
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
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*=`)
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 👍 / 👎.
There was a problem hiding this comment.
Flagged for human review — declining table-specific metacharacter escaping: literal Markdown punctuation round-trips unescaped everywhere in this converter (body paragraphs included), and after the inline passes run, generated syntax can no longer be distinguished from literal text in the regex chain. A fix belongs at the converter level, not per-cell.
| 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 👍 / 👎.
There was a problem hiding this comment.
Flagged for human review — declining table-specific link-title handling: reHTMLLink drops titles converter-wide (body links too), so preserving or guarding them only inside table cells would diverge from established behavior. Worth a converter-wide follow-up if titles matter.
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 \|, 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, interior spaces are preserved (only non-space whitespace collapses), 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. 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 — the parked-table path would leave the quote on only the first line. 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 or block elements inside the table, cells spanning multiple paragraphs, divs, 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.
cceb8a7 to
d7fd9c2
Compare
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.\|, 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 decode a second time on the next render. Code spans pass through as placeholders — backslashes are literal inside code, so only their pipes are escaped, interior spaces are preserved, and the emitted fence is one backtick longer than the longest backtick run in the content (CommonMark space padding included).|) 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.>prefix.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 against cell attributes — prose that merely mentionscolspan=stays editable)> | a | b |markdown)ul/ol/pre/blockquote/headings) inside the table<br>(the converter flattens those to one line for display; a single wrapper element stays editable)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.