fix(ace-editor): carry token classes onto emoji boxes#41821
Conversation
Follow-up to #41697: the emoji spans were appended outside the token-class wrapper ace puts around non-text tokens, so token styling that isn't glyph color (comment italics, invalid-token backgrounds) stopped applying to emoji segments. Mirror ace's text_util token-class handling directly on the emoji box. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Code Review Agent Run #066846Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #41821 +/- ##
==========================================
- Coverage 64.77% 64.76% -0.01%
==========================================
Files 2688 2688
Lines 148877 148881 +4
Branches 34359 34361 +2
==========================================
- Hits 96428 96426 -2
- Misses 50682 50688 +6
Partials 1767 1767
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Verified at HEAD 0f51c61: CI green (all required checks pass), no prior review threads to address. Confirmed the TEXT_TOKENS set and ace_+dotted-type class construction exactly mirror ace-builds' own text_util.isTextToken/$renderToken (checked against the vendored ace source), the two new tests exercise the real ace TextLayer (not a mock) and fail without the fix, no perf/XSS concerns (gated behind existing emoji match, classes come from the tokenizer not user text), and non-emoji tokens are unaffected. No findings.
2 participants
SUMMARY
Follow-up to #41697, addressing a fair CodeAnt catch that landed after merge: the emoji width patch appends its forced-width
.ace_cjkboxes directly to the line, outside the token-class wrapper ace puts around non-text tokens. Emoji glyphs carry their own color so the visible impact is small, but token styling that isn't glyph color — comment italics,ace_invalidbackgrounds, and any theme's token-level styling — stopped applying to emoji segments inside those tokens.Fix: mirror ace's
text_util.isTextToken/$renderTokenclass construction directly on the emoji box (ace_cjk ace_constant ace_languagefor aconstant.languagetoken, bareace_cjkfor text tokens). Same cascade as ace's own nesting, no extra DOM.Two new tests pin the class carrying for dotted token types and the bare-text case;
emojiWidthPatch.tsstays at 100% coverage.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — token-styling fidelity fix; caret behavior unchanged.
TESTING INSTRUCTIONS
13 passed, 100% coverage on the patch module.
ADDITIONAL INFORMATION
🤖 Generated with Claude Code