refactor: render debug console output as styled spans - #1462
Conversation
Preview output reached the debug console as pre-rendered HTML: the creator-hub
side ran each line through `ansi-to-html` and the inspector dropped the result in
with `dangerouslySetInnerHTML`. So the escaping lived with the sender, one process
away from the element that rendered it, and the console's contract was "trust this
HTML".
The wire now carries the raw line. `parseAnsi` splits it into `{ text, style }`
segments and the console renders one `<span style>` per segment, so output is
React children and text stays text regardless of what a scene logs.
The parser handles the codes a preview actually emits — reset, bold, italic,
underline and their resets, 30-37/39, 90-97, 40-47/49, and the `38;5;n` /
`38;2;r;g;b` extended forms. Anything else is dropped, which costs colour and
nothing else. Non-SGR escape sequences are left in the text and render as
characters.
`DebugLogEntry.html` becomes `.text`, and `ansi-to-html` is no longer used by
anything, so it is dropped from creator-hub.
Each entry stays a single direct child span, since `.DebugConsole-logs > span`
is what makes entries block-level, and the wrapper stays inside `.DebugConsole`
because the copy handlers reach it with `.closest`.
Test @dcl/inspector package
|
Test this pull request on macos-latestDownload the correct version for your architecture:Click here if you don't know which version to downloadFor running this unsigned version of the app, you will need to run the xattr command on it:
|
Test this pull request on windows-latestDownload the correct version for your architecture: |
decentraland-bot
left a comment
There was a problem hiding this comment.
Code Review — PR #1462
refactor: render debug console output as styled spans
Great security improvement — replacing dangerouslySetInnerHTML with React children eliminates the XSS surface entirely. The ANSI parser is well-structured, the matchAll approach avoids the lastIndex pitfall, and the test coverage is solid. The store is now a clean data layer decoupled from rendering.
However, there's one correctness issue that needs fixing before merge.
Findings
| # | Sev | Category | Summary |
|---|---|---|---|
| 1 | P1 | Correctness | bold/italic/underline are not valid CSS properties — styling silently broken |
| 2 | P2 | Performance | parseAnsi re-parses all entries on every render (no memoization) |
| 3 | P2 | Test quality | Truncated-colour test asserts color: undefined as a present key |
P1 — Bold/italic/underline styling silently broken
In DebugConsole.tsx, the destructuring { text, ...style } spreads AnsiSegment fields directly into the style prop:
<span key={index} style={style}>{text}</span>The resulting object may contain { bold: true, italic: true, underline: true }, but these are not valid CSS property names. The browser silently ignores them. The correct CSS equivalents are:
| AnsiSegment field | Valid CSS property | Value |
|---|---|---|
bold: true |
fontWeight |
'bold' |
italic: true |
fontStyle |
'italic' |
underline: true |
textDecoration |
'underline' |
TypeScript doesn't catch this because excess property checking only applies to object literals, not to variables from destructuring.
Suggested fix — option A (change the parser output):
In ansi.ts, change AnsiSegment to emit CSS property names directly:
export type AnsiSegment = {
text: string;
color?: string;
backgroundColor?: string;
fontWeight?: 'bold';
fontStyle?: 'italic';
textDecoration?: 'underline';
};And update applyCodes accordingly (style.fontWeight = 'bold' instead of style.bold = true, etc.).
Suggested fix — option B (map in the component):
Keep the parser output semantic and map to CSS in DebugConsole.tsx:
const cssStyle = {
color: seg.color,
backgroundColor: seg.backgroundColor,
...(seg.bold && { fontWeight: 'bold' as const }),
...(seg.italic && { fontStyle: 'italic' as const }),
...(seg.underline && { textDecoration: 'underline' as const }),
};Option A is simpler since the segment type is only consumed by this one component.
P2 — No memoization of parseAnsi
parseAnsi is called for every entry on every render (line 83). With MAX_ENTRIES = 1000, each store update (every 100ms during active logging) triggers a full re-parse of all entries. The parser is lightweight so this isn't a blocker, but since entry.id is stable and monotonically increasing, a simple useMemo or caching parsed results by ID would eliminate redundant work entirely.
P2 — Test asserts color: undefined as present key
At ansi.spec.ts:57, the truncated-colour test expects { text: 'x', color: undefined }. The color key is present with value undefined, which propagates as style={{ color: undefined }}. React handles this gracefully (removes the property), but asserting a key exists with undefined is fragile. Consider either omitting the key or using expect(result[0]).not.toHaveProperty('color') / checking result[0].color is undefined explicitly.
Security Review
✅ No security issues found. This PR is a net security improvement:
dangerouslySetInnerHTMLis fully removed (verified by grep — none remain inpackages/inspectororpackages/creator-hub)- Text is rendered as React children, which inherently escapes HTML
- Style values come from a hardcoded palette or clamped numeric parsing (
Math.max(0, Math.min(255, n))→ hex) — no path to inject arbitrary strings - The regex
[0-9;]*has no catastrophic backtracking risk - XSS test cases in the spec confirm markup stays literal
Git Conventions (ADR-6) ✅
- PR title:
refactor: render debug console output as styled spans— correct semantic format - Branch:
refactor/debug-console-ansi— correct pattern
CI Status ✅
All checks passing (lint, typecheck, unit tests, E2E, builds).
Reviewed by Jarvis 🤖 · Requested by Gabriel Díaz (<@U03MGHMAJL8>) via Slack
| backgroundColor?: string; | ||
| bold?: boolean; | ||
| italic?: boolean; | ||
| underline?: boolean; |
There was a problem hiding this comment.
[P1] bold, italic, and underline are not valid CSS property names. When these are spread into a React style prop via { text, ...style } in DebugConsole.tsx, the browser silently ignores them.
The fix is to emit CSS-compatible names here:
| underline?: boolean; | |
| fontWeight?: 'bold'; | |
| fontStyle?: 'italic'; | |
| textDecoration?: 'underline'; |
Then update applyCodes to use style.fontWeight = 'bold' (line 104), style.fontStyle = 'italic' (line 105), style.textDecoration = 'underline' (line 106), and the corresponding delete calls and reset block.
| }); | ||
|
|
||
| it('should combine styles from a multi-parameter sequence', () => { | ||
| expect(parseAnsi(`${ESC}[1;4;32mok`)).toEqual([ |
There was a problem hiding this comment.
[P1] This test asserts bold: true and underline: true, which are not valid CSS properties. Once the type is fixed to use fontWeight/fontStyle/textDecoration, this test should be updated to match:
| expect(parseAnsi(`${ESC}[1;4;32mok`)).toEqual([ | |
| expect(parseAnsi(`${ESC}[1;4;32mok`)).toEqual([ | |
| { text: 'ok', fontWeight: 'bold', textDecoration: 'underline', color: '#0dbc79' }, | |
| ]); |
| }); | ||
|
|
||
| it('should drop a truncated extended-colour sequence rather than emit an invalid colour', () => { | ||
| expect(parseAnsi(`${ESC}[38;2;18mx`)).toEqual([{ text: 'x', color: undefined }]); |
There was a problem hiding this comment.
[P2] Asserting color: undefined means the key is present with value undefined. This works but is fragile — consider checking with expect(result[0].color).toBeUndefined() separately, or documenting that the key's presence is intentional (since color256 can return undefined for truncated sequences, and the spread copies it).
The segment style carried `bold` / `italic` / `underline` booleans, which the console spread into a `style` prop. Those are not CSS property names, so the browser ignored them and bold, italic and underline rendered as plain text — a regression, since the HTML this replaced used `<b>` / `<i>` / `<u>`. They are now `fontWeight: 'bold'`, `fontStyle: 'italic'` and `textDecoration: 'underline'`, and the style type is `Pick<CSSProperties, …>` rather than hand-written. That closes the class: `style.bold = true` is now a compile error. It typechecked before because the key was legal on a bespoke type and the spread is a rest object, which excess-property checking does not reach. A truncated extended-colour sequence no longer assigns `undefined`. It leaves the colour already in effect alone, so the key is genuinely absent instead of present with no value — which is also what the spec now asserts. Each line renders through a memoized `LogLine` declared at module scope, so a line parses once while mounted rather than on every render of a console holding up to `MAX_ENTRIES`. Its outer span stays a direct child of `.DebugConsole-logs`, which is what makes entries block-level.
|
Pushed P1 — bold/italic/underlineConfirmed and fixed. Now Why it compiled, since that's the part worth not repeating: the flags were legal keys on a hand-written export type AnsiStyle = Pick<
CSSProperties,
'color' | 'backgroundColor' | 'fontWeight' | 'fontStyle' | 'textDecoration'
>;Verified that closes the class rather than the instance — putting New tests assert each code maps to its CSS property, and that 22/23/24 turn the property off entirely rather than leaving it set. P2 — memoizationEach line now renders through a memoized Its outer span stays a direct child of P2 — test assertionFixed at the source rather than in the assertion. A truncated extended-colour sequence no longer assigns expect(segments).toEqual([{ text: 'x' }]);
expect(segments[0]).not.toHaveProperty('color');Plus a case for the behaviour that implies: a truncated sequence after a valid colour keeps the earlier colour. Verification
The manual items in the description are worth re-running given P1 — particularly that bold and underlined preview output now actually renders styled, which is the thing that was broken. |
decentraland-bot
left a comment
There was a problem hiding this comment.
Re-review — PR #1462
All three findings from the initial review have been addressed in 269353e3:
P1 — Bold/italic/underline styling ✅ Fixed
AnsiStyle is now Pick<CSSProperties, 'color' | 'backgroundColor' | 'fontWeight' | 'fontStyle' | 'textDecoration'> — derived directly from React's CSSProperties, which makes non-CSS keys impossible at the type level. The parser correctly emits fontWeight: 'bold', fontStyle: 'italic', textDecoration: 'underline'. New parameterized tests verify both the on-codes (1/3/4) and their resets (22/23/24).
P2 — Memoization ✅ Fixed
Rendering was refactored to a React.memo(LogLine) component. Each entry is parsed once while mounted rather than on every render cycle. Clean approach — LogLine is a pure function of text, and entries are immutable.
P2 — Truncated-colour test ✅ Fixed
The test now asserts not.toHaveProperty('color') instead of color: undefined. A new assign() helper only sets the key when the value is defined, so truncated sequences preserve the colour already in effect. An additional test case verifies this behavior ('should leave a colour already in effect alone when a later sequence is truncated').
CI
Lint, unit tests, and typechecking all pass. E2E pending at time of review.
LGTM — nice refactor. The XSS surface is gone, the type safety is tight, and the memoization makes it efficient.
Reviewed by Jarvis 🤖 · Requested by Gabriel Díaz (<@U03MGHMAJL8>) via Slack
Summary
Preview output reached the debug console as pre-rendered HTML: creator-hub ran each line through
ansi-to-html, and the inspector dropped the result in withdangerouslySetInnerHTML. The escaping therefore lived with the sender — one process away from the element doing the rendering — and the console's contract with its input was "trust this HTML".The wire now carries the raw line. A new
parseAnsisplits it into{ text, style }segments and the console renders one<span style>per segment, so the output is React children and text stays text regardless of what a scene logs.There is now no
dangerouslySetInnerHTMLanywhere inpackages/inspectororpackages/creator-hub— verified by grep, it was the only one.The parser
Handles the codes a preview actually emits: reset, bold, italic, underline and their resets, 30-37/39, 90-97, 40-47/49, and the
38;5;n/38;2;r;g;bextended forms, with xterm's first 16 colours plus the 6×6×6 cube and greyscale ramp. Anything unrecognised is dropped, which costs colour and nothing else. Non-SGR escape sequences are left in the text and render as characters.It uses
line.matchAll(SGR)rather than a loop overSGR.exec, so the module-level regex'slastIndexis never carried between calls —matchAlliterates from its own copy.Knock-on changes
DebugLogEntry.html→.text(debug-log-store.ts)useDebugLogForwardingpushes the line instead ofconvert.toHtml(line); theansi-to-htmlimport goesansi-to-htmlis dropped frompackages/creator-hub— nothing references it any moreTwo layout details preserved deliberately: each entry stays a single direct child span, because
.DebugConsole-logs > spanis what makes entries block-level; and the wrapper stays inside.DebugConsole, because the copy handlers reach it with.closest.Note on the lockfile
77 lines, and most of it isn't mine: 6 lines are the
ansi-to-htmlremoval, 46 are npm re-adding"peer": truemetadata to unrelated packages. I kept it rather than hand-trimming —package.jsonand the lockfile have to move together ornpm cifails, and editing a lockfile by hand to look tidy is how you get a broken install. Generated withnpm install --package-lock-onlyand verified withnpm ci --dry-run.Test plan
ansi.spec.ts, 16 cases: colour and style codes including the extended forms, resets, segment splitting, malformed/truncated sequences, and markup in a log line staying literal text (both bare and wrapped in colour codes).packages/inspector— 97 files, 866 testsnpm run test:unit— main 74, preload 44, renderer 131, shared 33make typecheck0 errors, lint and format clean<b>hi</b>shows those characters rather than bold text🤖 Generated with Claude Code
https://claude.ai/code/session_01CW4SCgagWDAxR3PKJCMp5d