Repository navigation
refactor(client): inline the ANSI conversion, drop a dependency - #2442
Conversation
`ansi-html-community` was a third used and two thirds unused: the `tags`
getters and their pre-ES5 fallback, a `reset()` nobody called, and the
validation branches a fixed palette cannot reach. All of it shipped into every
consumer's browser bundle, from a package whose last release was 0.0.8 in
April 2022 — itself a fork of the abandoned `ansi-html`. Four production
dependencies now instead of five.
Checked against the package over a corpus before replacing it: identical for
every sequence a build actually produces. I confirmed what that is rather than
assuming — a real babel-loader failure with colour forced emits
`[0m [1m [22m [31m [33m [36m [39m [90m]`, all single-parameter.
Four things it got wrong, each now tested:
* `"transparent"`, which is how the palette says to leave the page's own
colour alone, became `color:#transparent`. Not a colour — so the reset
worked only because a browser drops an invalid declaration, and the
inverse sequence did nothing whatsoever.
* A sequence with more than one parameter (`\u001b[1;31m`) matched nothing,
leaving the escape in the output as text in front of the reader.
* `\u001b[m`, which is `\u001b[0m` written short, was left the same way.
* A closing sequence with nothing open emitted an unmatched `</span>`. The
highlighters wrap their own spans around this output, so a stray close
could end one of theirs early.
One trap worth recording: the package initialised its tag tables at module
load via `reset()`, and `setColors` here is only reached when `ansiColors` is
configured. Without an unconditional call at load, ANSI would have silently
stopped converting for everyone who never set a palette — which the corpus
comparison could not have caught, since it configures the palette itself.
The conversion had no test while it was a dependency. It has 19.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UjuMAuk9o6UazjHzcAQCTA
🦋 Changeset detectedLatest commit: 464d401 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 6 remain after this review. WalkthroughThe change adds a local ANSI SGR-to-HTML converter with configurable colors and declares its TypeScript interface. The overlay initializes the converter’s default palette and updates it when Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The local converter replaces the dependency while preserving overlay escaping and palette integration. No concrete merge-blocking issue is established; merge after normal checks pass. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to Problem text remains escaped before conversion, and the new converter keeps formatting state local to each message. No introduced security vulnerability was established. Palette values enter generated HTML separately from problem text, however, and their validation behavior has not been fully compared with the removed dependency. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 2305a1cd-214b-45a9-bf14-e9f1bb583269
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
.changeset/refactor-inline-ansi-html.mdclient-src/overlay.jsclient-src/utils/ansi-html.jspackage.jsontest/ansi-html.test.jstypes/client/utils/ansi-html.d.ts
💤 Files with no reviewable changes (1)
- package.json
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 6 remain after this review.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2442 +/- ##
==========================================
+ Coverage 96.22% 96.33% +0.10%
==========================================
Files 20 21 +1
Lines 2200 2265 +65
==========================================
+ Hits 2117 2182 +65
Misses 83 83 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The stack held SGR parameters and every close emitted `</span>`. So an element opened as a tag was closed as a span — `\u001b[3mx` gave `<i>x</span>` — and an interleaved sequence crossed its tags: `\u001b[3m\u001b[31mx\u001b[23m` gave `<i><span style="…">x</i></span>`. Both are inherited from the package this replaced, identically, but leaving them sat badly next to a changeset claiming to fix unmatched closes. This is the same class of defect. Each open element now carries its own closing tag. A close emits the innermost one, a repeated parameter closes back through everything opened inside it, and the final cleanup unwinds in reverse. Which parameter a closer belongs to is still not tracked — `[3m[31mx[23m` cannot close the italic without crossing the colour span, so the italic runs to the end of the message — but nothing this produces fails to nest, which is what matters when the highlighters wrap their own spans around it afterwards. Found by CodeRabbit on #2442. Five cases for the reported shapes, and one that walks every three-sequence combination of twelve sequences and checks each closing tag matches what is innermost. Six of the 25 fail without this. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UjuMAuk9o6UazjHzcAQCTA
1 participant
Four production dependencies instead of five. Independent of #2439 — branches
off
main.What we used of it
ansi-html-communityis 176 lines, of which this project touched about athird: the sequence regex, the open/close tag maps, the replace loop, and
building those maps from a palette. The rest was surface it never called — the
tagsgetters with their pre-ES5definePropertyfallback, areset()nobody used, the validation branches a fixed palette literal cannot reach, and
a
bin/. All of it shipped into every consumer's browser bundle, from apackage whose last release was 0.0.8 in April 2022, itself a fork of the
abandoned
ansi-html.It is
client-src/utils/ansi-html.jsnow, at 248 lines including thedocumentation of why each difference exists.
Verified against it, not assumed
I compared the two over a corpus before replacing anything: identical for
every sequence a build actually produces. And I checked what that is rather
than guessing — a real babel-loader failure with colour forced emits
[0m,[1m,[22m,[31m,[33m,[36m,[39m,[90m, allsingle-parameter, all handled the same way.
Four things it got wrong
"transparent"becamecolor:#transparent. That is how the overlay'spalette says to leave the page's own colour alone, and it was written into a
hex slot. Not a colour — so the reset worked only because browsers drop an
invalid declaration, and
\u001b[7m(inverse) did nothing whatsoever. Thedeclaration is now left out.
\u001b[1;31mstayed in theoutput as literal text, in front of the reader, and the following
[0mthenopened a span that wrapped the remainder. Parameters are applied one by one.
\u001b[m—\u001b[0mwritten short — was left the same way. Read as areset.
</span>. Thehighlighters run after this and wrap their own spans around its output, so a
stray close could end one of theirs early. Two corpus cases hit it:
[99m…[39mand[31ma[31mb[39mc, the latter giving…a</span>b</span>c.The trap
The package initialised its tag tables at module load, via
reset().setColorshere is reached only whenansiColorsis configured — so withoutan unconditional call at load, ANSI would have silently stopped converting
for everyone who never set a palette. The corpus comparison could not have
caught it, because the harness configures the palette itself. There is now a
call at load, where the palette is declared.
Tests
The conversion had no test of its own while it was a dependency — the only
ANSI-adjacent test passed
ansiColorsand checked the option was honoured,never feeding a sequence through. This is the one part of the overlay with real
edge cases, so it has 19 now: nesting, backgrounds, tag-style codes, an
unclosed sequence, an unknown parameter, a non-SGR escape left alone, each of
the four fixes, and a palette of one's own including hex, non-hex, one-value
resetand a missing colour.overlay + ansi suites 71 pass; the full run before splitting this out was e2e
146 across 12 suites and unit 6941 across 18. Lint, both typecheck passes,
spelling, the precompiled schema check and the full build clean.
Worth noting
ansi-html-communityis a direct dependency of webpack-dev-servertoo, for the overlay webpack/webpack-dev-server#5750 deletes — so it drops from
there as well once that lands.
🤖 Generated with Claude Code
https://claude.ai/code/session_01UjuMAuk9o6UazjHzcAQCTA
Generated by Claude Code
Summary by CodeRabbit