Skip to content

refactor: render debug console output as styled spans - #1462

Open
cyaiox wants to merge 2 commits into
mainfrom
refactor/debug-console-ansi
Open

refactor: render debug console output as styled spans#1462
cyaiox wants to merge 2 commits into
mainfrom
refactor/debug-console-ansi

Conversation

@cyaiox

@cyaiox cyaiox commented Aug 4, 2026

Copy link
Copy Markdown
Member

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 with dangerouslySetInnerHTML. 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 parseAnsi splits 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 dangerouslySetInnerHTML anywhere in packages/inspector or packages/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;b extended 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 over SGR.exec, so the module-level regex's lastIndex is never carried between calls — matchAll iterates from its own copy.

Knock-on changes

  • DebugLogEntry.html.text (debug-log-store.ts)
  • useDebugLogForwarding pushes the line instead of convert.toHtml(line); the ansi-to-html import goes
  • ansi-to-html is dropped from packages/creator-hub — nothing references it any more

Two layout details preserved deliberately: each entry stays a single direct child span, because .DebugConsole-logs > span is 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-html removal, 46 are npm re-adding "peer": true metadata to unrelated packages. I kept it rather than hand-trimming — package.json and the lockfile have to move together or npm ci fails, and editing a lockfile by hand to look tidy is how you get a broken install. Generated with npm install --package-lock-only and verified with npm 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 tests
  • npm run test:unit — main 74, preload 44, renderer 131, shared 33
  • make typecheck 0 errors, lint and format clean
  • run a scene with the debug panel open — coloured output still renders with the same colours
  • a log line containing <b>hi</b> shows those characters rather than bold text
  • entries are still one per line (block layout intact) and Copy still works
  • the mobile debug console is unaffected

🤖 Generated with Claude Code

https://claude.ai/code/session_01CW4SCgagWDAxR3PKJCMp5d

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`.
@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Test @dcl/inspector package

  • Preview: link
  • Install via NPM:
    npm install "https://sdk-team-cdn.decentraland.org/creator-hub/branch/refactor/debug-console-ansi/@dcl/inspector/dcl-inspector-7.37.2-commit-1fb12f6ecec292b9f81a77075f8ca883d228cbdd.tgz"

@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Test this pull request on macos-latest

Download the correct version for your architecture:

mac-x64
mac-arm64

Click here if you don't know which version to download

For running this unsigned version of the app, you will need to run the xattr command on it:

  1. Extract the app from the downloaded .dmg file (double-click it)
  2. Place the extracted app anywhere you like in your file system
  3. Open a terminal on the directory where the app is
  4. Run xattr -c app-name, replacing "app-name" for the actual name of the app
  5. Double-click the app ✅

@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Test this pull request on windows-latest

Download the correct version for your architecture:

win-x64

@decentraland-bot decentraland-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  • dangerouslySetInnerHTML is fully removed (verified by grep — none remain in packages/inspector or packages/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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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:

Suggested change
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([

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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:

Suggested change
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 }]);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.
@cyaiox

cyaiox commented Aug 4, 2026

Copy link
Copy Markdown
Member Author

Pushed 269353e3. All three taken — the P1 was a real regression, thanks for catching it.

P1 — bold/italic/underline

Confirmed and fixed. bold / italic / underline were spread into a style prop where they aren't CSS property names, so the browser dropped them silently. And it was a regression: the HTML this replaced used <b> / <i> / <u>, so those styles worked before.

Now fontWeight: 'bold', fontStyle: 'italic', textDecoration: 'underline'.

Why it compiled, since that's the part worth not repeating: the flags were legal keys on a hand-written AnsiSegment, and the spread is a rest object rather than an object literal — so excess-property checking never applied at the style={style} site. The style type is now derived instead of hand-written:

export type AnsiStyle = Pick<
  CSSProperties,
  'color' | 'backgroundColor' | 'fontWeight' | 'fontStyle' | 'textDecoration'
>;

Verified that closes the class rather than the instance — putting style.bold = true back now fails typecheck:

src/lib/logic/ansi.ts(117,34): error TS2339: Property 'bold' does not exist on type 'AnsiStyle'.

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 — memoization

Each line now renders through a memoized LogLine, declared at module scope (the repo's coding standards call out memoized components built during render as an antipattern). Entries are immutable and keyed by id, so a line parses once while mounted rather than on every console render.

Its outer span stays a direct child of .DebugConsole-logs — that selector is what makes entries block-level, so flattening the segments into the list would have collapsed every entry onto one line.

P2 — test assertion

Fixed at the source rather than in the assertion. A truncated extended-colour sequence no longer assigns undefined; it leaves the colour already in effect alone. So the key is genuinely absent, and the spec says so explicitly:

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

  • packages/inspector — 873 tests passing (23 in ansi.spec.ts, was 16)
  • make typecheck 0 errors, lint and format clean

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 decentraland-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants