fix(ui): give transcript paragraphs a full block step - #3436
Open
Astro-Han wants to merge 3 commits into
Open
Conversation
Two paragraphs in a chat turn sat 8px apart against 20px body leading, so a paragraph break was 0.4 of a line — narrower than the line break inside a paragraph, which is the one vertical distance a reader already knows. A long answer read as one slab. Take the block rung to 12px, which is Astryx's own paragraph rhythm at document density (spacingParagraphDefault, --spacing-3). The transcript keeps its own heading scale and its own 4px list rung and only stops being denser than the design system between two paragraphs. Where the fix does NOT belong, because the obvious reading is wrong: `density="compact"` in chat-turn.tsx has not owned transcript block spacing since #2348. That table zeroes every Astryx margin on the document's children and declares the four gaps itself, in @layer components — a Markdown density token, or a makaTheme.ts override of `astryx-markdown-paragraph`, lands in an earlier layer and is inert. `--md-gap-block` is the only authority, so the call site now says so rather than leaving the next reader to re-derive it from commit history. One rung moves, so code blocks and blockquotes move with it: they are already carried by the same `> * + *` step (Astryx skips its codeblock wrapper whenever `components.code` is set, which Maka always does), and splitting a per-element rung out would re-introduce the multi-value spacing this table exists to collapse. What that costs is contrast at the top of the ladder — the section step falls from 2x the block gap to 1.33x — and the heading rungs stay put anyway, because heading spacing is #1857's decision and not this change's business. Measured in Storybook against live computed styles, before and after, on Product/Markdown → TranscriptTurn and Product/Shell Official AppShell → Native Conversation: list rows 4px 4px unchanged blocks 8px 12px paragraph, list, quote, code block section 16px 16px unchanged (h3-h6) chapter 24px 24px unchanged (h1, h2) Daily Review renders at density="default" and is out of the selector, so its ladder is byte-for-byte what it was: 12/12 paragraphs, 16/16 quote and table, straight from Astryx. Generated-by: Claude Code
Astro-Han
force-pushed
the
fix/transcript-paragraph-spacing
branch
from
August 21, 2026 15:46
86c1e5f to
b5f2a96
Compare
6 tasks
styles.css told readers the "compact == transcript" assumption was held by `__tests__/markdown-rhythm-dom-contract.test.tsx`. That file was deleted in #2462 and its stylesheet-side sibling in #2425, so the comment has been citing a guard that does not exist — the worst state to leave an assumption in, since the next person reads the citation and stops looking. Restore only what cannot be noticed without a test. Both failure modes here are silent: no error, no failing check, just spacing that quietly stops being what the table declares, because a CSS selector that matches nothing never complains. - The table selects entirely on DOM Astryx generates at runtime — data-density, astryx-markdown-heading + data-level, astryx-list-item. Those names have one upstream owner and appear in Maka only inside selectors, so a rename kills every rule at once. Astryx is bumped regularly (0.4.0 in #2983, 0.4.3 in flight, plus the Dependabot minor group), so this is a recurring event, not a hypothetical. - Astryx's ListItem carries CONTROL row padding that `density` cannot reach from outside. That padding is what inverted the ladder in #2348 — list items ~10px apart against 4px paragraphs — and one rule neutralizes it. Lose the rule and the original defect returns, silently. - Plus the assumption the comment actually names: compact markdown still has exactly one caller. The table carries heading TYPOGRAPHY on a density key, which Astryx's own RFC says density must not do, so the key is honest only while `compact` and "transcript" are the same set. Deliberately NOT pinned: the ladder's declared values and their order. A first draft asserted them, and it was the wrong instinct — a reversed ladder has to be typed on purpose into four adjacent lines under a comment explaining the order, and it is visible the moment anyone looks at a transcript. It also would not have caught the defect it claimed to guard: #2348's inversion came from the ListItem padding above, which a test reading only the declared variables cannot see. Same for the adjacent-sibling gap form, the `hr` rung and the two heading size tiers — those are how the table is written, not what it promises. Why this is not what #2425/#2462 retired. Those PRs removed markup/copy pins, duplicate presentation assertions, and CSS-structure suites that asserted how a rule was written. Two of the three assertions here render the real component and check the join between Astryx's output and Maka's selectors; the third asserts an outcome the table promises, not a syntax. The stylesheet-reading half does not revive the deleted desktop css-test-helpers module or the renderer-wide CSS aggregate it read — it reads packages/ui/src/styles.css directly, in the package that owns it. Six mutations, each verified to fail the assertion that describes it: deleting the ListItem padding reset; un-zeroing it; un-zeroing it while naming the reset in a comment (this is why comments are stripped first); renaming the contract wrapper; cutting density off before it reaches Astryx; and adding a second compact caller. Generated-by: Claude Code
Astro-Han
force-pushed
the
fix/transcript-paragraph-spacing
branch
from
August 21, 2026 16:00
b5f2a96 to
c43fbd0
Compare
Astro-Han
marked this pull request as ready for review
August 21, 2026 16:18
hqhq1025
reviewed
Aug 21, 2026
hqhq1025
left a comment
hqhq1025
left a comment
Contributor
There was a problem hiding this comment.
One actionable P2 finding remains on the reviewed head.
Problem and mechanism: the PR correctly identifies that the compact transcript rhythm table, not Astryx Markdown density tokens, owns top-level paragraph spacing. Raising the block rung from 8px to 12px is a small and coherent readability change if the affected surface is actually the transcript.
The scope assumption is not currently true, however. Desktop Artifact Preview has long rendered exported MarkdownBody with density="compact", so it receives the same selector, transcript heading typography, and this new 12px block gap. The added caller test scans only packages/ui/src and only raw <Markdown> tags, so it misses both the cross-workspace caller and the <MarkdownBody> component form and passes while its transcript-only assertion is already false.
First principles and optimality: surface semantics and density are separate concerns. If these typography and block-rhythm overrides are transcript-specific, key them on an explicit transcript surface marker rather than treating every compact document as a transcript. If Artifact Preview intentionally shares the same ladder, include it in the contract and visually verify that surface instead of claiming a single caller.
Deletion and tests: replace or delete the source-scanning transcript-only test in its current form; it provides false confidence. The rendered DOM-hook test and ListItem padding regression remain useful. A replacement should exercise the explicit surface boundary or enumerate real exported MarkdownBody/Markdown callers across the workspace without ad-hoc JSX regex assumptions.
Merge verdict: not ready to merge until the Artifact Preview blast radius is made intentional and the false transcript-only guard is corrected. This is a normal COMMENT review under the maintainer comment-only policy; no REQUEST_CHANGES state is used.
Verification: reviewed head c43fbd0; required CI is green; npm run build:test passed; the new 3/3 rhythm suite passes despite the existing Artifact Preview caller; Astryx theme drift and git diff --check pass.
Astro-Han
added a commit
that referenced
this pull request
Aug 21, 2026
Review on #3436 caught the caller-scan test asserting something already false. Two surfaces render MarkdownBody at compact density — the transcript and the desktop Artifact Preview (apps/desktop, since #2506) — and the test passed anyway, for two independent reasons: it walked only packages/ui/src, and its `<Markdown\s` pattern never matched the `<MarkdownBody>` form the second caller uses. Either bug alone would have surfaced the other caller. Delete it rather than widen it. The set it wants to pin spans workspaces, so holding it means a library test grepping application source — the dependency backwards — and the mechanism already proved it can be wrong and silent on its first commit. A guard that lies is worse than a documented gap. What the deletion gives up is recorded where the sharing matters, next to the heading rules: compact carries transcript TYPOGRAPHY, which Astryx's density RFC says density should not, so a third compact surface inherits it with nothing to catch that. Left shared deliberately for the preview — it is a narrow pane showing a document a few lines at a time, the shape the flattening was built for. Splitting the key is a visible change to that surface and belongs in its own PR, argued on its own screenshots. The 12px block rung reaches the preview too, and there it is a move toward the design system rather than away: 12px is Astryx's own document paragraph rhythm. The two remaining assertions are untouched — both read rendered DOM and the stylesheet itself, so neither can be wrong about scope. Generated-by: Claude Code
Review on #3436 caught the caller-scan test asserting something already false. Two surfaces render MarkdownBody at compact density — the transcript and the desktop Artifact Preview (apps/desktop, since #2506) — and the test passed anyway, for two independent reasons: it walked only packages/ui/src, and its `<Markdown\s` pattern never matched the `<MarkdownBody>` form the second caller uses. Either bug alone would have surfaced the other caller. Delete it rather than widen it. The set it wants to pin spans workspaces, so holding it means a library test grepping application source — the dependency backwards — and the mechanism already proved it can be wrong and silent on its first commit. A guard that lies is worse than a documented gap. What the deletion gives up is recorded where the sharing matters, next to the heading rules: compact carries transcript TYPOGRAPHY, which Astryx's density RFC says density should not, so a third compact surface inherits it with nothing to catch that. Left shared deliberately for the preview — it is a narrow pane showing a document a few lines at a time, the shape the flattening was built for. Splitting the key is a visible change to that surface and belongs in its own PR, argued on its own screenshots. The 12px block rung reaches the preview too, and there it is a move toward the design system rather than away: 12px is Astryx's own document paragraph rhythm. The two remaining assertions are untouched — both read rendered DOM and the stylesheet itself, so neither can be wrong about scope. Generated-by: Claude Code
Astro-Han
force-pushed
the
fix/transcript-paragraph-spacing
branch
from
August 21, 2026 16:56
567933b to
d8b2189
Compare
Contributor
Author
|
Confirmed — fixed in 567933b. The test was false on the commit that introduced it. Deleted rather than widened (reasoning in the inline reply), and the sharing is now documented next to the heading rules. On the blast radius, split in two:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two paragraphs in a chat turn sat 8px apart against 20px body leading — a paragraph break narrower than the line break inside a paragraph, so a long answer read as one slab. The block rung goes to 12px, Astryx's own paragraph rhythm at document density.
Not where it looks like it belongs:
density="compact"on the transcript's<Markdown>has not owned block spacing since #2348. The rhythm table instyles.csszeroes Astryx's margins and declares the four gaps itself in@layer components, so a Markdown density token — or amakaTheme.tsoverride — lands in an earlier layer and is inert.--md-gap-blockis the only authority; the call site now says so.Code blocks and blockquotes ride the same rung and move with it. The heading rungs stay put, which is #1857's and #2348's decision, and the cost is accepted: the section step falls from 2x the block gap to 1.33x. 12px is the largest step that keeps the ladder ordered without moving a heading rung — at 16px a paragraph break would be indistinguishable from the space before a section heading.
Second commit.
styles.csscited a test as holding the "compact == transcript" assumption; that file was deleted in #2462 and its sibling in #2425, so nothing would have caught a bad value here. The restored test pins only what a reader cannot notice on sight — an Astryx rename (the table selects entirely on runtime Astryx DOM, and a selector matching nothing never complains) and losing the ListItem padding reset (the actual cause of #2348's inversion) — plus the single-caller assumption the comment names. The ladder's values are deliberately left unpinned: a wrong number is visible the moment anyone looks at a transcript.Refs #1857, #2348, #2425, #2462
Verification
Live computed styles in Storybook (
Product/Markdown → TranscriptTurn,Product/Shell Official AppShell → Native Conversation). Daily Review renders atdensity="default", outside the selector — unchanged.The contract test was mutation-tested: six mutations (delete / un-zero / comment-out the ListItem reset, rename the contract wrapper, cut
densitybefore Astryx, add a second compact caller), each verified to fail the assertion that describes it. Also run: the new suite (3/3),@maka/uitypecheck,npm run lint,npm run format:check,npm run astryx:theme -- --check. Full repository suite left to CI.AI use
Select exactly one:
Tool(s) and scope: Claude Code — found the real spacing authority, made the change, found the dangling test citation, wrote the test and comments, and ran the measurements and mutation sweep. The 12px value, leaving the heading rungs alone, and the restore-vs-drop scope call were reviewed by the human contributor.
Checklist
Does this PR entail a change in behavior?