feat(tui): improve fenced code block readability - #75783
Conversation
|
Visual validation artifacts were generated from the same harness on WSL2:
The after sheet covers normal-width Python, diff, narrow Korean/emoji wrapping, and default/slate themes on dark/light terminal backgrounds. GitHub CLI does not support uploading local image attachments directly, so these paths are recorded for manual attachment to the PR description. |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused TUI treatment and the width-oriented renderer coverage. The main idea addresses a real current-main gap: fenced code is still rendered as an indented column in ui-tui/src/components/markdown.tsx:812-848.
Problems
ui-tui/src/components/markdown.tsx:584sends arbitrary fence-info text toborderText. Ink detects width withstringWidth, but its overflow path truncates withsubstring(ui-tui/packages/hermes-ink/src/ink/render-border.ts:43-45); wide CJK/emoji labels can therefore write past a 20-column panel. Please display-width-truncate the label and cover wide labels, not only wide code content.ui-tui/src/components/markdown.tsx:897adds a bordered, four-cells-narrower code body, whileui-tui/src/lib/virtualHeights.ts:106-116continues estimating raw text at full body width with no fenced-panel adjustment. Add a width-aware fenced-code estimate and regression coverage so virtual transcript spacers are correct before Yoga convergence.
Suggested changes
- Add CJK/emoji fence-info tests at the normal-width threshold.
- Add a virtual-height test for a long unbroken fenced line.
Automated hermes-sweeper review.
The original implementation in NousResearch#75783 had two width-correctness blockers: * `CodeBlock` passed the raw `lang` string to Ink `borderText`. Ink's border-embedding path (packages/hermes-ink/src/ink/render-border.ts::embedTextInBorder)'s JS-substring truncation corrupts CJK / emoji labels mid-glyph whenever `stringWidth(text) >= borderLength - 2`. * `lib/virtualHeights.ts` still estimated the raw fenced source at the full body width, while the rendered code panel uses the narrower inner width. A 30-char source line that fits at body width 30 (= 1 wrapped row) actually wraps to 2 rows inside a normal panel, so the virtual transcript spacer was undercounting and snapping on first mount. This commit: * Adds `ui-tui/src/lib/codeBlockLayout.ts`, a small shared module that holds the panel constants, the mode judgment, the inner-width and border-label-budget formulas, a grapheme-safe `truncateToWidth` (Intl.Segmenter with `Array.from` fallback), and the fence open/close regexes that the renderer and the estimator must agree on. * Updates `CodeBlock` to keep the original `lang` for syntax-highlight detection and to derive a `displayLang` via `truncateToWidth` for the border label (normal mode) and the language row (narrow / compact mode). The ellipsis is included inside the budget so `stringWidth(result) <= borderLabelWidth(cols)` always holds. * Adds `estimateBodyHeight` in `lib/virtualHeights.ts` that walks the source text linearly (no split, no pre-built fence-span list) and counts rows the way the renderer paints them: fenced code uses the panel's inner width via `fenceWrapWidth`, plus `chromeRows` (2 for normal, `hasLang ? 1 : 0` for narrow). The walk is bounded by the same `MAX_ESTIMATE_LINES` cap the prose estimator uses, so a 1M-char single line still returns in O(width) time. A single trailing newline is stripped from each fence body so the existing `wrappedLines` impl does not inflate the count with a phantom empty line. Tests: * `markdown.test.ts` adds 6 width-safe label regressions: long Korean label at the 20-col threshold, emoji label, mixed Korean/emoji/ASCII, label wider than the panel, no broken surrogate pairs, and width-safety at the 19 / 20 / 21 col boundary. * `virtualHeights.test.ts` adds 8 fence-aware regressions: a long unbroken fenced line that undercounted at the old body width, normal vs narrow at the threshold, compact mode, no-language fence, mixed prose + fence, empty fence, unclosed fence, the 1M-char giant-fence performance cap, and the full `estimatedMsgHeight` long-fence regression. Pre-fix borderText overflow and pre-fix virtualHeights undercount are both fixed without changing the existing normal / narrow / compact panel design, syntax / diff coloring, or wrap behavior. No dependencies added, no config option introduced. The original 2-commit branch is rebased onto current upstream main and the new work is a single focused commit on top.
9297520 to
291c942
Compare
Final review pass on NousResearch#75783 surfaced three follow-ups. All three are small, contained changes on top of the previous fix commit. 1. `md` / `markdown` fence height. The renderer recurses those fences through `<Md cols={cols}>` instead of painting the rounded CodeBlock panel, so the body is prose at the full body width and the panel's top + bottom border rows don't apply. The previous estimator tracked only `fenceHasLang: boolean` and treated every fence as a code panel, which overcounted md/markdown fences by 2 rows plus any inner-width vs. body-width difference. `estimateBodyHeight` now stores the normalized lang string and, when it is `md` or `markdown`, counts the body at `bodyWidth` with no chrome rows. Closed and unclosed fences both go through this branch. The renderer is unchanged. 2. Real regex sharing. The renderer had its own local `FENCE_RE` and `FENCE_CLOSE_RE`; `codeBlockLayout.ts` had its own `FENCE_OPEN_RE` and `FENCE_CLOSE_RE`. Both call sites now use the shared exports from `ui-tui/src/lib/codeBlockLayout.ts`. The renderer's local copies are removed. 3. Trim `codeBlockLayout.ts`. Dropped `FenceMatch`, `isFenceOpenLine`, `fenceLangOf` (none had callers) and `fenceWrapWidth` (a one-line wrapper around `innerContentWidth` that the estimator now calls directly). Module is down from 145 to 103 lines with the long comments compressed. The renderer still imports only the four symbols it actually uses (`borderLabelWidth`, `FENCE_CLOSE_RE`, `FENCE_OPEN_RE`, `innerContentWidth`, `isNarrowPanel`, `truncateToWidth`). Tests: * `virtualHeights.test.ts` adds one md/markdown regression: a 30-char source line at body width 30 returns 1 row for `md` / `markdown` (body width, no chrome) and 4 rows for `python` (panel path). Same body at narrow width 18 returns 2 rows for `md` (no chrome) and the panel path still produces 3 rows. Existing 63 tests still pass; new total 64. No new branch, no new PR, no new issue. No dependencies added, no config option introduced. Draft state preserved.
SummaryOne PR addresses Issue #75781. #75783 replaces indented fenced-code rendering with a theme-aware rounded panel, adds a narrow/compact left-accent fallback, preserves syntax and diff coloring, and aligns wrapping and virtual-height estimation with the panel layout. Related pull requests
Suggested consolidationKeep #75783 open with a salvage path: retain the CodeBlock treatment, shared width helpers, fence-aware height estimator, and Unicode/narrow-layout regressions, then obtain contributor re-review of the changes addressing the visible keep_open review and validate the remaining issue requirements for streaming and exact copy semantics. There are no duplicate PRs to close. Complex graphflowchart LR
classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
classDef best stroke-width:3px,stroke:#b45309
classDef target stroke-width:3px,stroke:#4338ca
I75781(["issue #75781 (open)"])
P75783["PR #75783 (open)"]
P75783 -->|best fix| I75781
class I75781 open
class P75783 open
class P75783 best
class P75783 target
click I75781 "https://github.com/NousResearch/hermes-agent/issues/75781"
click P75783 "https://github.com/NousResearch/hermes-agent/pull/75783"
Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label). Cross-PR triage: Reviewed 1 pull request and 1 issue in this complex. Each diff was read against this issue; Assessment working set: 37 kB of PR diffs, 18 kB of issue/PR text, 9 kB of discussion (7 comments), 3 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch. |
|
Follow-up on the automated triage note:
An earlier run from a non-clean local environment was not reproducible in the clean worktree. No code changes were made in this follow-up. A contributor re-review of the current head would be appreciated. |
What does this PR do?
Fenced code blocks in the Ink TUI previously relied on indentation and a small language separator, so code could blend into surrounding prose in long technical responses.
This PR gives fenced code a restrained, theme-aware panel at normal widths:
At widths below 20 columns, and whenever compact mode is active, it falls back to a left accent and compact language label. That keeps useful code width and avoids making a four-sided outline dominate heavily wrapped content.
The follow-up commit
695df6881(on top of the previous two) addresses the final-review findings:md/markdownfence height. The renderer recurses those fences through<Md cols={cols}>instead of painting the rounded CodeBlock panel, so the body is prose at the full body width and the panel's top + bottom border rows don't apply.estimateBodyHeightnow stores the normalized lang string and, when it ismdormarkdown, counts the body atbodyWidthwith no chrome rows. Closed and unclosed fences both go through this branch. The renderer is unchanged.FENCE_RE/FENCE_CLOSE_RE;codeBlockLayout.tshad its ownFENCE_OPEN_RE/FENCE_CLOSE_RE. Both call sites now use the shared exports fromui-tui/src/lib/codeBlockLayout.ts. The renderer's local copies are removed.codeBlockLayout.ts. DroppedFenceMatch,isFenceOpenLine,fenceLangOf(none had callers) andfenceWrapWidth(a one-line wrapper aroundinnerContentWidththat the estimator now calls directly). The module is down from 145 to 103 lines with the long comments compressed.The earlier commit
291c9422ccovers the original two review blockers:CodeBlockwas passing the rawlangstring to InkborderText. Ink's border-embedding path (packages/hermes-ink/src/ink/render-border.ts::embedTextInBorder) takes a JS-substringfallback wheneverstringWidth(text) >= borderLength - 2, which corrupts a CJK / emoji / mixed label mid-glyph.CodeBlocknow derives adisplayLangvia a grapheme-safetruncateToWidth(Intl.Segmenter withArray.fromfallback) and uses that for both the border label and the narrow / compact language row. The originallangis preserved for syntax-highlight detection.virtualHeights.tsdid not account for the panel's chrome rows or reduced content width. A source line that fits at the body width can wrap inside the panel's narrower inner width, so the original estimator undercounted and the virtual transcript spacer snapped on first mount.estimateBodyHeightwalks the source linearly (nosplit, no pre-built fence-span list), recognizes the same backtick / tilde fence forms as the renderer, and counts each fence body at the panel's inner width plus the panel's chrome rows. The walk is bounded by the sameMAX_ESTIMATE_LINEScap the prose estimator uses, so a 1M-char single line still returns in O(width) time.Related issue
Closes #75781
Related to #12130 and #46905. Builds on the width-safe TUI rendering discussion in #15534 and #17114. Adjacent but non-duplicative work includes #71849 and #48095. PRs #5617 and #75326 address decorated fenced-code rendering in the classic Python interactive CLI rather than this Ink TUI renderer.
Type of change
Changes made
CodeBlockrenderer insideui-tui/src/components/markdown.tsx.t.color.borderandt.color.muted; no hard-coded colors or new theme token.wrap="wrap-char", so long unbroken source stays within the allocated Markdown body width.ui-tui/src/lib/codeBlockLayout.ts(103 lines after the final-review trim) — a small shared module that holdsCODE_PANEL_MIN_WIDTH, theisNarrowPanel/innerContentWidth/borderLabelWidth/chromeRowshelpers, the grapheme-safetruncateToWidth, and theFENCE_OPEN_RE/FENCE_CLOSE_REregexes that the renderer and the virtual-height estimator both import.ui-tui/src/lib/virtualHeights.tswithestimateBodyHeightthat recognizes fenced blocks in a single linear pass, counts their body rows at the panel's inner width (or at the full body width formd/markdownfences), and adds the matching chrome rows (or none formd/markdownfences).langfor syntax-highlight detection; only the displayed label is truncated.Design notes
A full outline was retained for normal widths because the current
@hermes/inkborder renderer subtracts visible left and right border cells from its measured Yoga width, and theCodeBlockreceivescols, the actual transcript-body allocation, rather than reading the full terminal width. Its one-cell horizontal padding is consequently part of the same width budget.The table precedent in #17114 deliberately avoided a full outline when display-width measurement was unreliable. The current TUI now uses its own
stringWidthimplementation and the focused tests exercise Korean and emoji widths. Even so, a full outline costs four horizontal cells after borders and padding, so constrained and compact layouts deliberately use only a left accent.The width budget for the trimmed
borderTextcontent iscols - 5: the two corner cells, the two surrounding spaces inside the border (${label}), and one cell for the leading─after the╭. The truncation grapheme-walks the original label withIntl.Segmenter(undefined, { granularity: 'grapheme' })(falling back toArray.fromfor environments without the Segmenter) and includes the ellipsis in the budget sostringWidth(result) <= borderLabelWidth(cols)always holds.For the virtual-height estimator, the renderer-facing chrome rules are mirrored exactly:
bodyWidth - 4content width, 2 chrome rows (top + bottom border);bodyWidth - 2content width,hasLang ? 1 : 0chrome rows (language row only, no border);md/markdownfence: body is recursed with<Md>, so the estimator counts it as prose at the full body width with no chrome rows.The opener / closer fence lines themselves are not counted as visible rows, matching the renderer. Empty fences still receive a minimum 1 code row so the renderer-side
<Text> </Text>placeholder isn't undercounted.Streaming remains unchanged:
StreamingMdcontinues to hold open fences in its mutable tail and freezes only complete Markdown blocks. The sameMdrenderer draws partial and completed fenced blocks, so there is no second streaming-specific style path.No user-facing configuration was added because this is a focused rendering treatment with a deterministic narrow-width fallback.
How to test
Validation results
npm run build:ink— clean (esbuild,dist/entry-exports.js433.8kb).npm test --run src/__tests__/markdown.test.ts src/__tests__/streamingMarkdown.test.ts src/__tests__/syntax.test.ts src/__tests__/virtualHeights.test.ts— 64 / 64 passed in the two changed test files (markdown.test.ts44,virtualHeights.test.ts20). The +1 over the prior 63 is the newmd/markdownregression.npm test --run(full TUI suite) — 1,464 / 1,480 passed, 8 skipped, 8 failed. The 8 failures reproduce identically on clean upstreammain(commit3572d4bca) and are ineditor.test.ts,terminalParity.test.ts, andterminalSetup.test.ts— all environment-dependent (PATH editor lookups, VS Code config dir detection) and unrelated to this PR.npm run typecheck— clean.npx eslinton the touched files — 0 errors, 0 warnings.npm run visual— wroteZ:\Temp\hermes-tui-visual\tui-visual.htmlandtui-visual.png; standard inputs (Python / diff / narrow Korean+emoji / default + slate themes on dark + light) unchanged, so the existingafter.pngis not regenerated.npm run build—dist/entry.js3.5mb.git diff --check— clean.CI
gh pr checks 75783currently reports 1 check inaction_requiredon the current head695df6881. This is the fork workflow approval gate — the upstreamci.ymlworkflow is configured to require explicit maintainer approval for runs on a contributor fork, and a first-time contributor is gated behind that approval. It is not a test failure introduced by either of the fix commits. Once a maintainer approves the workflow run the checks proceed normally.Tests added
ui-tui/src/__tests__/markdown.test.ts(in thefenced code panelsdescribe block):…,stringWidth(label) <= 15.U+FFFDreplacement char, every linestringWidth(line) <= width.U+FFFD, every line within width.╭…╮, contains…, noU+FFFD.lines.every(line => stringWidth(line) <= width)invariant asserted at widths 19 (narrow), 20 (normal threshold), 21 (normal).ui-tui/src/__tests__/virtualHeights.test.ts:bodyWidth - 4width proof via the 22-char line at body width 24 (normal: 2 + 2 = 4) and body width 18 (narrow: 2 + 1 = 3).bodyWidth 30, compact: true) routes through narrow layout regardless of width.<= 800and runs in under 50 ms.estimatedMsgHeight(msg, 35, { compact: false, details: false })regression on a 30-char source line — must be>= 4(was 3 before).md/markdownfence regression: a 30-char source line at body width 30 returns 1 row formd/markdown(full body width, no chrome) and 4 rows forpython(panel path). Same body at narrow width 18 returns 2 rows formdand the panel path still produces the expected count.Files changed
ui-tui/src/lib/codeBlockLayout.ts(new, 103 lines after the final-review trim)ui-tui/src/components/markdown.tsx(modified; now importsFENCE_OPEN_RE/FENCE_CLOSE_REfrom the shared module)ui-tui/src/lib/virtualHeights.ts(modified;estimateBodyHeightwithmd/markdownaware chrome and width)ui-tui/src/__tests__/markdown.test.ts(modified; 6 width-safe label regressions)ui-tui/src/__tests__/virtualHeights.test.ts(modified; 8 fence-aware regressions + 1md/markdownregression + the 1M-char giant-fence performance cap)The pre-existing entries in this PR remain as well:
ui-tui/scripts/visual/render.tsx(visual harness extension for the new code-block scene).github/pr-screenshots/75783/before.pngandafter.png(visual fixtures)ui-tui/src/components/markdown.tsxand the existing renderer tests inui-tui/src/__tests__/markdown.test.ts(kept; the fix commits only add width-safe label handling,md/markdownfence handling, and additional regression tests on top of them).Commits on this branch
34c54c3b4 feat(tui): improve fenced code block readability77a67cf25 docs(tui): add code block visual comparison291c9422c fix(tui): make fenced code panels width-safe— width-safe border label + fence-aware virtual-height estimator (blockers 1 and 2 from the first review pass)695df6881 fix(tui): align estimator with md/markdown fence recursion—md/markdownfence handling, real regex sharing,codeBlockLayout.tstrim (final-review follow-ups)The branch is rebased onto current upstream
main(3572d4bca); pushing was done with--force-with-lease.Checklist
Code
Documentation and housekeeping
cli-config.yaml.example: not applicableCONTRIBUTING.md/AGENTS.md: not applicableScreenshots
Before
After
Both sheets use the same examples and cover normal-width Python, diff, narrow Korean/emoji wrapping, and default/slate themes on dark/light terminal backgrounds.