Skip to content

Use Trix-compatible HTML for proper paragraph spacing#421

Open
nnemirovsky wants to merge 1 commit intobasecamp:mainfrom
nnemirovsky:fix/trix-compatible-html
Open

Use Trix-compatible HTML for proper paragraph spacing#421
nnemirovsky wants to merge 1 commit intobasecamp:mainfrom
nnemirovsky:fix/trix-compatible-html

Conversation

@nnemirovsky
Copy link
Copy Markdown
Contributor

@nnemirovsky nnemirovsky commented Apr 16, 2026

Summary

Rebased onto current main. The original three commits targeted the pre-#415 regex-based MarkdownToHTML pipeline, which #415 replaced with goldmark. This PR is now a single commit that re-implements the same Trix-compatibility goals on the goldmark architecture.

  • <div> instead of <p> — Trix stores paragraphs as <div> blocks. goldmark's default <p> output renders in Basecamp with the block margins of <p> plus the intervening <br>, producing double spacing where the user typed a single blank line.
  • <div><br></div> for blank lines — Trix's native empty-line shape, replacing the bare <br>\n the TrixBreak node previously emitted.
  • HTMLToMarkdown round-trip — accept <div> and <div><br></div> on input so content authored in Basecamp parses cleanly back to Markdown (and so MarkdownToHTML's own output round-trips).

Before: MarkdownToHTML("Line 1\n\nLine 2")<p>Line 1</p>\n<br>\n<p>Line 2</p> (double-spaced in Basecamp).

After: MarkdownToHTML("Line 1\n\nLine 2")<div>Line 1</div>\n<div><br></div>\n<div>Line 2</div> (correct single spacing).

Implementation notes

  • trixRenderer now registers renderParagraph so top-level paragraphs become <div>. trixTransformer already converts paragraphs inside lists and blockquotes into TextBlock nodes, so the override only fires where it should — list/blockquote output is unchanged.
  • renderTrixBreak emits <div><br></div>\n instead of <br>\n. renderHTMLBlock's raw-HTML wrapper matches.
  • HTMLToMarkdown gains reDivBr and reDiv passes running before the existing <p> pipeline, so documents containing either shape (or a mix) degrade cleanly.
  • blockquoteInnerToMarkdown also handles <div> / <div><br></div>, otherwise the inner <div><br></div> cascades through the outer blockquote line-splitter and produces extra > lines. TestEditLoopRoundTrip/multi-paragraph_blockquote covers this.
  • Inline <br> inside a paragraph (consecutive lines joined with a single \n) is unchanged — trixTransformer's soft→hard break conversion keeps working, matching Trix's own representation.

Test plan

  • All TestMarkdownToHTML* cases updated to the new <div> / <div><br></div> shape and green.
  • TestEditLoopRoundTrip round-trips including multi-paragraph blockquotes.
  • TestResolveMentions* inputs updated to <div> wrapper (representative of what MarkdownToHTML now emits and what Trix emits natively). Function behavior is input-shape-preserving so coverage is unchanged.
  • Cross-package: TestCheckinsAnswerCreateDefaultsDateToToday updated for <div> output.
  • make fmt-check, go vet, golangci-lint clean.
  • go test ./... full suite green.
  • End-to-end: posted a multi-paragraph comment via CLI, confirmed single-line paragraph spacing in Basecamp matches a hand-edited reference comment's HTML.

Copilot AI review requested due to automatic review settings April 16, 2026 10:42
@github-actions github-actions Bot added commands CLI command implementations tests Tests (unit and e2e) bug Something isn't working labels Apr 16, 2026
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates the rich-text Markdown→HTML conversion to match Basecamp/Trix’s native HTML structure so paragraph spacing and single line breaks render consistently in Basecamp.

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.

Changes:

  • Switch paragraph wrapper output from <p> to Trix-style <div>.
  • Represent blank lines as <div><br></div> instead of emitting a standalone <br>.
  • Preserve single newlines inside paragraphs by joining lines with <br> rather than spaces.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
internal/richtext/richtext.go Emits Trix-compatible <div> blocks and <div><br></div> separators; preserves single newlines via <br>.
internal/richtext/richtext_test.go Updates expected HTML outputs and adjusts round-trip behavior checks for the new line-break handling.
internal/commands/checkins_test.go Updates check-in answer creation test expectation to the new <div>-wrapped content.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread internal/richtext/richtext.go Outdated
Comment thread internal/richtext/richtext.go Outdated
Comment thread internal/richtext/richtext_test.go Outdated
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 3 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="internal/richtext/richtext.go">

<violation number="1" location="internal/richtext/richtext.go:178">
P1: MarkdownToHTML now emits `<div>` paragraphs, but HTMLToMarkdown only normalizes `<p>` blocks, causing paragraph-spacing loss in round-trip conversions.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

Comment thread internal/richtext/richtext.go Outdated
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread internal/richtext/richtext.go Outdated
Comment thread internal/richtext/richtext.go Outdated
Copilot AI review requested due to automatic review settings April 24, 2026 05:41
@nnemirovsky nnemirovsky force-pushed the fix/trix-compatible-html branch from 6aa2d47 to 4fd5294 Compare April 24, 2026 05:41
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread internal/richtext/richtext.go
Comment thread internal/richtext/richtext_test.go Outdated
@nnemirovsky nnemirovsky force-pushed the fix/trix-compatible-html branch from 4fd5294 to 0b19c43 Compare April 24, 2026 05:46
Copilot AI review requested due to automatic review settings April 24, 2026 05:51
@nnemirovsky nnemirovsky force-pushed the fix/trix-compatible-html branch from 0b19c43 to 96e1d94 Compare April 24, 2026 05:51
Trix (the editor behind Basecamp's rich-text fields) stores paragraphs as
<div>...</div> and blank-line separators as <div><br></div>. goldmark's
default output — <p>...</p>\n<br>\n<p>...</p> — renders with the block
margins of <p> *plus* the line break of the intervening <br>, producing
double spacing in Basecamp between what the user typed as a single blank
line between two paragraphs.

MarkdownToHTML changes:
- trixRenderer registers renderParagraph, emitting <div>...</div> for
  top-level paragraphs. trixTransformer already converts paragraphs
  inside lists and blockquotes into TextBlocks, so this only fires where
  it should.
- trixRenderer.renderTrixBreak emits <div><br></div>\n instead of <br>\n.
- trixRenderer.renderHTMLBlock wraps raw HTML blocks in <div> instead of
  <p> for consistency.
- The MarkdownToHTML error-path fallback ("<p>" + escaped + "</p>")
  mirrors the new shape.

HTMLToMarkdown changes (for round-trip fidelity with Trix-native HTML
coming from the API):
- Add reDivBr and reDiv patterns running before the existing <p>/<br>
  pipeline: <div><br></div> becomes a blank line, <div>...</div> becomes
  a paragraph.
- blockquoteInnerToMarkdown gains <div>/<div><br></div> handling so
  multi-paragraph blockquotes round-trip cleanly; without it, the inner
  <div><br></div> cascades through the outer blockquote line-splitter
  and produces extra ">" lines.

Test updates:
- MarkdownToHTML expected strings flip <p>→<div>, </p>→</div>, and the
  block-level break \n<br>\n → \n<div><br></div>\n. Applied via a
  targeted sweep, verified one by one against the rendered output.
- ResolveMentions test inputs flip <p>→<div> to stay representative of
  what MarkdownToHTML now produces and what Trix emits natively.
- TestEditLoopRoundTrip/multi-paragraph_blockquote: now passes cleanly
  because of the new blockquote <div> handling.
- TestCheckinsAnswerCreateDefaultsDateToToday in internal/commands
  asserted the wrapper shape; updated to <div>.

Inline <br> inside a paragraph (consecutive lines with single \n between
them) is unchanged: trixTransformer still converts soft-break → hard-break
inside <div>, matching Trix's own line-break representation.
@nnemirovsky nnemirovsky force-pushed the fix/trix-compatible-html branch from 96e1d94 to d086a88 Compare April 24, 2026 05:53
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

nnemirovsky added a commit to nnemirovsky/basecamp-cli that referenced this pull request Apr 24, 2026
Combines both PRs' branches for local use via `go install`:
- PR basecamp#421 (trix): <div>/<div><br></div> paragraph shape, soft→hard break
  in top-level paragraphs, HTMLToMarkdown round-trip for <div>.
- PR basecamp#447 (attachments): prefer download Href over preview URL in
  ParsedAttachment.DisplayURL; suggest --type on 404 from generic
  /recordings lookup.

Upstream each PR remains separately reviewable at basecamp/basecamp-cli.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working commands CLI command implementations tests Tests (unit and e2e)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants