Skip to content

fix: render html in pull request comment bodies as markdown - #808

Merged
leynier merged 3 commits into
mainfrom
fix/comments-markdown-rendering
Sep 17, 2026
Merged

leynier merged 3 commits into
mainfrom
fix/comments-markdown-rendering

Conversation

@leynier

@leynier leynier commented Sep 17, 2026

Copy link
Copy Markdown
Owner

Pull request comment bodies often contain a GitHub HTML subset (bot footers with <picture>, <img>, <a>, <sub>, <br>, HTML comments), which GptMarkdown does not render and showed up as literal text.

What changed:

  • Add sanitizePullRequestCommentBody in lib/src/features/pull_requests/domain/pull_request_comment_body.dart to convert that HTML subset to Markdown before rendering, and wire it into PullRequestCommentMarkdown.
  • Mirror the same sanitizer in mobile/lib/src/features/workbench/domain/pull_request_comment_body.dart and wire it into pull_request_conversation_section.dart.
  • Handle images, dark-mode <picture> sources, links, inline formatting, blocks, breaks, <pre>, entities, and HTML comments while leaving fenced code blocks and inline code spans untouched.
  • Cover desktop behavior with unit and widget tests, plus a mobile drift test for the shared footer and code-fence cases.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Important

HTML <blockquote> in a comment body throws while sanitizing, so that comment fails to render.

Reviewed changes

This pass reviewed the HTML-to-markdown sanitizer added for pull request comment bodies on desktop and mobile, plus the tests that pin bot-footer rendering.

  • Comment HTML sanitizer — Converts the GitHub HTML subset (<picture>, <img>, <a>, formatting, blocks, entities) to markdown before GptMarkdown, leaving fenced and inline code untouched.
  • Desktop and mobile wiring — PullRequestCommentMarkdown and the mobile conversation _CommentBody now sanitize immediately before render; the mobile file is the expected duplicate because alera_mobile cannot import the root package.
  • Tests — Desktop unit tests pin exact conversions for the footer, images, dark <picture>, javascript: hrefs, entities, and fences; a widget test pumps a bot footer; mobile covers the footer and fence cases against drift.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using Grok | 𝕏

Comment thread lib/src/features/pull_requests/domain/pull_request_comment_body.dart Outdated

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ No new issues found.

Reviewed changes

This pass reviewed the follow-up commit that fixes the blockquote sanitizer crash and tightens CRLF / <br> handling, plus the tests that pin those arms.

  • Blockquote capture group — Both desktop and mobile converters now read group 1, matching the one-capture regex, so HTML <blockquote> no longer throws in build().
  • CRLF and breaks — Prose segments drop a trailing \r, and <br> consumes one following newline so a hard break does not become an extra paragraph.
  • Inline picture pass — _sanitizeInline no longer re-walks <picture>; the block pass already converted complete pictures, and the empty-src fallback still uses the inner <img>.
  • Coverage — Unit tests now pin blockquotes (including empty), <pre>, strikethrough, inline <code>, headings, summaries, lists, Windows line endings, and pictures without a usable src.

Pullfrog  | View workflow run | Using Grok | 𝕏

- GitHub review bodies embed `<picture>`, `<img>`, `<a>`, and `<sub>` tags that `GptMarkdown` showed as literal text
- Sanitize comment HTML to Markdown on desktop and mobile, preserving fenced and inline code
- Prefer dark picture sources, convert anchors and formatting, and decode entities
- blockquote handler read group(2) on a one-group regex and threw on any <blockquote>
- drop the unreachable picture arm from inline sanitizing
- normalize CRLF in prose segments and fold one newline into <br>
- extend unit tests to hold 100% domain coverage
- cli_tests.rs grew to 567 lines after workspace section tests landed,
  tripping the 500-line max-lines ratchet on every PR
- move each command area into its own cli_*_tests.rs module behind the
  existing path-attribute pattern; cli_tests.rs keeps only declarations
- give cli_issue_tests.rs explicit imports instead of super::*
@leynier
leynier force-pushed the fix/comments-markdown-rendering branch from 693e0ac to 57f785a Compare September 17, 2026 08:00
@leynier
leynier merged commit d80d6f9 into main Sep 17, 2026
34 of 36 checks passed
@leynier
leynier deleted the fix/comments-markdown-rendering branch September 17, 2026 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant