fix(tui): plus-key bindings never match, NFD combining-mark width over-count, theme-dependent nested-list mis-render - #11633
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…ent nested-list detection
- keys.ts: parseKeyId() returned null for any keyId whose base key is the
literal "+" symbol ("+", "ctrl++", ...) because split("+") yields a
trailing empty part, so those bindings could never match despite being
valid KeyIds. Resolve a trailing "+" separator to the "+" key.
- utils.ts: couldBeEmoji() treated every grapheme longer than 2 code units
as an emoji (width 2), so decomposed (NFD) characters with two combining
marks (e.g. Vietnamese ế as e+U+0302+U+0301) measured width 2 while
their NFC form measured 1, breaking truncation/padding/wrap alignment for
NFD text. Detect multi-codepoint emoji via ZWJ instead; skin tones, flags,
keycaps and ZWJ families are still caught by the existing range/VS16 checks.
- markdown/list-renderer.ts: nested-list detection sniffed rendered lines
for a hardcoded cyan SGR prefix (\x1b[36m). With colors disabled
(NO_COLOR, piped output) or any non-cyan listBullet theme, nested lists
got double indentation and items starting with a nested list grew an
extra bullet ("- - b"). Tag nested-list lines structurally instead of
sniffing ANSI codes.
30c34fd to
de90b9f
Compare
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
lalalune
left a comment
lalalune
left a comment
There was a problem hiding this comment.
Reviewed the rebased diff and validated the TUI fixes locally.
Evidence:
bunx vitest run test/keys.test.ts test/visible-width.test.ts test/markdown.test.tsinpackages/tui: 3 files / 90 tests passed.bun run testinpackages/tui: 26 files / 496 tests passed.bun run typecheckinpackages/tui: passed.git diff --check origin/develop...HEAD && git diff --check: passed.bunx @biomejs/biome@2.5.2 check src/components/markdown/list-renderer.ts src/utils.ts test/visible-width.test.ts test/keys.test.ts: passed.
I also ran Biome across all touched files; it reports existing diagnostics in src/keys.ts and test/markdown.test.ts that are already present on develop (control-regex/non-null assertions), and the PR's new hunks do not add those patterns.
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
2 participants
Summary
Three provable rendering/input bugs in
@elizaos/tui(used by the agent CLI and the eliza-code cockpit surface), each with a concrete failing input, a minimal fix, and a mutation-checked regression test.1.
keys.ts— bindings on the+key can never match"+"is a validBaseKey(theKeyIdtype advertises"ctrl++"etc. andKey.plusexists), butparseKeyId()splits the keyId on"+", so the base key parses as the empty string and the function returnsnull.Failure (pre-fix, reproduced live):
matchesKey("+", "+")→false(typing a plus sign never matches a+binding)matchesKey("\x1b[43;5u", "ctrl++")→false(Kitty ctrl+plus, codepoint 43 mod 5)Fix: a keyId whose last split part is empty and that ends with
"+"resolves its base key to the literal"+"."ctrl+-"and every other keyId are untouched (asserted).2.
utils.ts—visibleWidth()counts NFD graphemes with 2+ combining marks as width-2 emojicouldBeEmoji()treated any grapheme longer than 2 code units as an emoji (width 2). A base letter with two combining marks — e.g. Vietnameseếin NFD (e+ U+0302 + U+0301, which is exactly what macOS NFD filenames contain) — is 3 code units, so:Failure (pre-fix, reproduced live):
visibleWidth("ế")= 2 butvisibleWidth("ế")(the identical visible character, NFC) = 1. The same glyph measured two different widths, corruptingtruncateToWidthpadding, wrap points, and markdown table column alignment for any NFD input.Fix: replace the
length > 2heuristic with a ZWJ (U+200D) check. Multi-codepoint emoji remain covered: skin-tone modifiers, regional-indicator flags and ZWJ families start with a codepoint in the existing emoji ranges, keycaps carry VS16 — all re-asserted at width 2 in the new tests.3.
markdown/list-renderer.ts— nested-list detection hardcodes the cyan escape\x1b[36mrenderList()classified already-rendered lines as "nested list" by regex-matching a hardcoded basic-cyan SGR prefix. With colors disabled (NO_COLOR, piped/non-TTY output → chalk level 0) or any theme whoselistBulletisn't exactly\x1b[36m(e.g. truecolor), detection fails.Failure (pre-fix, reproduced live with an identity theme):
- a\n - b→["- a", " - b"](nested item double-indented)- - b(marked genuinely emits an item whose first token is a nested list) →"- - b"(extra parent bullet, garbage output)Fix: tag lines structurally at the source — internal
renderListItemLines()returns{ text, nested }sorenderList()no longer sniffs theme-specific ANSI. The exportedrenderListItem()keeps itsstring[]signature (delegates), so no API break.Tests
test/keys.test.ts: +5 tests (typed+, Kittyctrl++/ctrl+shift++, negatives,ctrl+-guard)test/visible-width.test.ts(new): +9 tests (NFC/NFD equality, two-combining-marks width, NFD string equality, NFD truncate+pad alignment; emoji guards: single-codepoint, ZWJ family, skin tone, flag, keycap)test/markdown.test.ts: +3 tests (NO_COLOR nested indent, no extra bullet on nested-first item, continuation indent)Evidence
bunx vitest run packages/tui/test)bun run --cwd packages/tui typecheckcleankeys.tsfix → 3 failures inkeys.test.ts✅ caughtutils.tsfix → 3 failures invisible-width.test.ts✅ caughtlist-renderer.tsfix → 2 failures inmarkdown.test.ts✅ caughtkeys.ts/markdown.test.tspre-exist byte-identical onorigin/develop(tui has no lint gate)packages/appUI surface; deterministic string-level renderer output asserted directly in unit tests (before/after rendered lines shown above)Only
packages/tui/src+packages/tui/testtouched.packages/examples/codeuntouched.Authored end-to-end by a Fable-5 agent (find + fix + tests + mutation-checks + push, single pass). Independently re-verified by the main loop: 6-file merge-base diff, 90/90 tests pass on a fresh run, and the utils.ts NFD-width mutation reproduced independently (revert → 3 fail; restore → green). —
[phone-ui]