Space agent-markdown blocks through Streamdown's dir wrappers - #239
hardbeat920 merged 2 commits into
Conversation
A reply with several sections separated by blank lines show up as one dense block, no visible gap between them. The paragraph breaks are real in the text (each becomes its own <p>), they just have no space under them. Cause: Streamdown own heading, blockquote and hr components each carry a margin class already (mt-6/mb-2, my-4, my-6), but its default paragraph component is a plain <p> with no spacing class at all. So with margin reset by Tailwind preflight, every paragraph sit flush against the next one. Add margin-bottom to .agent-markdown p, and zero it on the last paragraph in a block so it does not add extra space before the next message. Test: no jsdom/component test exist in this repo yet (vitest only picks up src/**/*.test.ts and the harness modules are tested as pure functions), so the test check the shipped CSS rule directly against the stylesheet source instead of a rendered DOM. It fails without the fix and pass with it. Fixes hardbeat920#218
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe agent Markdown stylesheet now adds 1rem above paragraphs and lists that follow another block. The tests render Streamdown output and verify spacing between paragraphs, without spacing before the first or after the last block. ChangesMarkdown rendering
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to The Markdown spacing change preserves separation across paragraph, list, and heading transitions without adding a final trailing gap. No current merge-blocking risk was identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 1 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@bluzername can you share some screenshots to see the difference. I honestly couldn't find any but maybe i am missing sth 🙏 |
The first patch on this branch did nothing. I rendered it and measured it: the gap between paragraphs stayed 0px and the two screenshots came out byte identical. Cause: AgentMarkdown passes dir="auto", so Streamdown puts every block in its own <div dir="..." style="display: contents">. A display:contents box drops its own margins, so Streamdown's space-y-4 on the root, which targets exactly those wrappers, paints no gap. Headings, blockquotes and rules still space correctly because their margin sits on the element itself. Paragraphs and lists carry no margin at all, so they sit flush. That same wrapper is why the first patch was useless: every <p> is the only child of its wrapper, so .agent-markdown p:last-child matched all of them and zeroed the margin-bottom it had just added. Put the gap on the block inside the wrapper instead, top side only, so a reply never ends with a trailing gap. Measured in Chromium at 860px wide, dark theme: paragraph gap 0px before, 16px after, nothing added above the first block or below the last. Test: renders real Streamdown output (react-dom/server under happy-dom) and checks the shipped selectors against that DOM, so a rule that matches nothing, or matches every paragraph, fails. It is red on main and red on the first patch, green now. Fixes hardbeat920#218
bluzername
changed the title
|
thanks for checking. You are right, and I owe you an apology: my first patch did nothing at all. I rendered both and the two screenshots came out byte identical (same file size, Here is why, and what I found.
<div class="space-y-4 ... agent-markdown ...">
<div dir="ltr" style="display:contents"><p>First paragraph.</p></div>
<div dir="ltr" style="display:contents"></div>
<div dir="ltr" style="display:contents"><p>Second paragraph.</p></div>
</div>Two things follow from that wrapper:
New commit on the branch puts the gap on the block inside the wrapper instead, top side only, so no trailing gap: .agent-markdown > div + div > p,
.agent-markdown > div + div > [data-streamdown="unordered-list"],
.agent-markdown > div + div > [data-streamdown="ordered-list"] {
margin-top: 1rem;
}Before (main, and also my first patch, identical):
After (this branch now):
What is in the screenshots: the real Measured with
The test is rewritten too. It now renders real Streamdown output with Sorry for the round trip. Thank you for pushing back instead of merging it. |
|
@bluzername thank you this looks good. Merged :) |
2 participants
Fixes #218
Updated after @hardbeat920 could not see a difference. He was right: the first patch on this branch changed nothing. Details and screenshots are in the comment below.
What happens
A reply with several "\n\n"-separated sections shows up as one dense block in the transcript, no gap between the paragraphs, even though the breaks are really there in the stored text.
Cause
AgentMarkdownpassesdir="auto"to Streamdown. With that, Streamdown puts every block in its own wrapper:A
display: contentsbox drops its own margins. Streamdown'sspace-y-4on the root targets exactly those wrappers, so it paints no gap at all. Headings, blockquotes and rules still look fine because their margin sits on the element itself (mt-6 mb-2,my-4,my-6). Paragraphs and lists carry no margin, so they sit flush.The same wrapper is why my first patch did nothing: every
<p>is the only child of its wrapper, so.agent-markdown p:last-childmatched all of them and zeroed themargin-bottomit had just added.Fix
Put the gap on the block inside the wrapper, top side only, so a reply never ends with a trailing gap:
Lists are in there because they have the same problem, no margin of their own. Headings, blockquotes and rules are left alone, they already space themselves.
Measured in Chromium at 860px wide, dark theme, rendering the real
AgentMarkdownwith the realsrc/index.css:The heading row moves because the paragraph's new 16px collapses with the heading's own 8px bottom margin. That is the 1rem Streamdown meant to have there anyway.
Test
src/surfaces/agentMarkdownSpacing.test.tsnow renders real Streamdown output (react-dom/server, with// @vitest-environment happy-domin the file so the repo-wide node environment stays as it is) and checks the shipped selectors against that DOM. So a rule that matches nothing, or matches every paragraph like my first one did, fails. It is red on main, red on my first patch, green now.Ran
vitest run(2223 tests green) andtsc --noEmit(clean). Did not run the cargo half ofnpm run check, nothing here touches Rust.Summary by CodeRabbit
Style
Tests