fix(logger): Preserve box alignment for ANSI-styled warnings/errors#105
fix(logger): Preserve box alignment for ANSI-styled warnings/errors#105SergioChan wants to merge 7 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughStdOutLogger now strips ANSI escape sequences from messages before boxing to compute correct widths, then reapplies level-specific ANSI styling; tests and test utilities were updated to support ANSI-aware stdout mocking and to validate boxed output with ANSI and control sequences. Changes
Sequence Diagram(s)(omitted — changes are local to logger and tests, not multi-component sequential flows) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/cli_tools/lib/src/logger/loggers/std_out_logger.dart (1)
243-251: Consider consolidating duplicate styling logic.This switch statement is identical to the one in the
log()method (lines 97-103). Using_styleByLevelin both places would eliminate the duplication and reduce the risk of the two diverging if styling requirements change.♻️ Proposed refactor to consolidate styling
void log( final String message, final LogLevel level, { final bool newParagraph = false, final LogType type = TextLogType.normal, }) { if (ansiSupported) { - final ansiMessage = switch (level) { - LogLevel.debug => AnsiStyle.darkGray.wrap(message), - LogLevel.info => message, - LogLevel.warning => AnsiStyle.yellow.wrap(message), - LogLevel.error => AnsiStyle.red.wrap(message), - LogLevel.nothing => message, - }; + final ansiMessage = _styleByLevel(message, level); _log(ansiMessage, level, newParagraph, type); } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli_tools/lib/src/logger/loggers/std_out_logger.dart` around lines 243 - 251, The switch-based styling logic duplicated in the log() method should be consolidated to use the existing helper _styleByLevel(String message, LogLevel level); update the log() implementation to call _styleByLevel(message, level) instead of repeating the switch, and remove the redundant switch block so styling is centralized in _styleByLevel (ensure imports/signatures remain compatible with LogLevel and AnsiStyle).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/cli_tools/lib/src/logger/loggers/std_out_logger.dart`:
- Around line 243-251: The switch-based styling logic duplicated in the log()
method should be consolidated to use the existing helper _styleByLevel(String
message, LogLevel level); update the log() implementation to call
_styleByLevel(message, level) instead of repeating the switch, and remove the
redundant switch block so styling is centralized in _styleByLevel (ensure
imports/signatures remain compatible with LogLevel and AnsiStyle).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: da9b3afa-e54a-458d-a516-b4f651258285
📒 Files selected for processing (1)
packages/cli_tools/lib/src/logger/loggers/std_out_logger.dart
SergioChan
changed the title
|
Thanks for the review — addressed. Updates pushed
Please re-check when convenient. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/cli_tools/lib/src/logger/loggers/std_out_logger.dart (1)
153-161: Core fix looks correct.The approach of stripping ANSI codes before computing box dimensions and re-applying styling afterward properly addresses the alignment issue.
Minor edge case: The
titlepassed to_formatAsBox(line 157) is not stripped of ANSI codes. If a title contains ANSI sequences,title.lengthat line 320 would include escape characters, potentially causing the same misalignment in the title row. Consider applying_stripAnsiCodesto the title as well for completeness, or document that titles should be plain text.,
🔧 Optional fix for title consistency
if (type is BoxLogType) { message = _formatAsBox( wrapColumn: wrapTextColumn ?? _defaultColumnWrap, message: _stripAnsiCodes(message), - title: type.title, + title: type.title != null ? _stripAnsiCodes(type.title!) : null, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli_tools/lib/src/logger/loggers/std_out_logger.dart` around lines 153 - 161, The title passed to _formatAsBox can contain ANSI codes which will skew box width calculations; update the BoxLogType handling so the title argument is passed through _stripAnsiCodes (similar to message) before calling _formatAsBox, i.e., apply _stripAnsiCodes to the title variable in the block where _formatAsBox is invoked (the same block that later re-applies styling with _styleByLevel) to ensure title length is measured against plain text.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/cli_tools/lib/src/logger/loggers/std_out_logger.dart`:
- Around line 153-161: The title passed to _formatAsBox can contain ANSI codes
which will skew box width calculations; update the BoxLogType handling so the
title argument is passed through _stripAnsiCodes (similar to message) before
calling _formatAsBox, i.e., apply _stripAnsiCodes to the title variable in the
block where _formatAsBox is invoked (the same block that later re-applies
styling with _styleByLevel) to ensure title length is measured against plain
text.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 02ed9464-138f-4531-ab3f-34337671ebee
📒 Files selected for processing (1)
packages/cli_tools/lib/src/logger/loggers/std_out_logger.dart
|
Thanks for the follow-up review — good catch. I pushed a small update to strip ANSI sequences from
This keeps box title width calculations consistent even if a styled title is ever passed. Validation:
|
christerswahn
left a comment
christerswahn
left a comment
There was a problem hiding this comment.
Thank you for contributing!
You need to add tests. See std_out_logger_test.dart. Follow the given - when - then test structure convention. I see now that there aren't any existing test cases for calling log() with the BoxLogType, please add for both the normal cases and your fixed case.
| wrapColumn: wrapTextColumn ?? _defaultColumnWrap, | ||
| message: message, | ||
| title: type.title, | ||
| message: _stripAnsiCodes(message), |
There was a problem hiding this comment.
Suggest moving the ansi code wrapping to the _log() method, and avoid wrapping, unwrapping, and wrapping again.
|
Thanks — added the requested tests in Added coverage
Notes on validation
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/cli_tools/lib/src/logger/loggers/std_out_logger.dart (1)
247-249: Broaden_stripAnsiCodesor narrow its name.This regex only removes SGR color sequences (
...m). Other ANSI control sequences will still count toward width and can reintroduce box misalignment if callers pass richer terminal escapes.Suggested change
String _stripAnsiCodes(final String input) { - final ansiRegex = RegExp(r'\x1B\[[0-9;]*m'); + final ansiRegex = RegExp(r'\x1B(?:[`@-Z`\\-_]|\[[0-?]*[ -/]*[`@-`~])'); return input.replaceAll(ansiRegex, ''); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli_tools/lib/src/logger/loggers/std_out_logger.dart` around lines 247 - 249, The _stripAnsiCodes function currently only removes SGR color sequences and should either be renamed to reflect that limitation or expanded to strip all common ANSI control sequences; update _stripAnsiCodes to remove not just SGR ("\x1B[...m") but also CSI sequences ending with bytes in the range `@-`~, OSC sequences (ESC ] ... BEL or ESC \ terminators), single-character escapes (ESC followed by control byte), and other ESC sequences so terminal control codes no longer contribute to visible width, then run/update any tests or callers that depend on box alignment to validate the fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli_tools/test/std_out_logger_test.dart`:
- Around line 160-176: The test is non-deterministic across ANSI environments;
update it to exercise both paths by calling collectOutput twice (or passing an
ansiSupported=true flag if collectOutput supports it) around logger.warning with
BoxLogType so you assert both behaviors: (1) the non-ANSI output from stdout
after the first collectOutput has no '\x1B[' and matches the plain boxed string,
and (2) the ANSI-enabled output from a second collectOutput contains ANSI
sequences and still renders with the same box width (e.g., verify it contains
'\x1B[' and, after stripping ANSI codes, equals the same boxed string). Target
the test, collectOutput, logger.warning and BoxLogType identifiers when making
the change.
---
Nitpick comments:
In `@packages/cli_tools/lib/src/logger/loggers/std_out_logger.dart`:
- Around line 247-249: The _stripAnsiCodes function currently only removes SGR
color sequences and should either be renamed to reflect that limitation or
expanded to strip all common ANSI control sequences; update _stripAnsiCodes to
remove not just SGR ("\x1B[...m") but also CSI sequences ending with bytes in
the range `@-`~, OSC sequences (ESC ] ... BEL or ESC \ terminators),
single-character escapes (ESC followed by control byte), and other ESC sequences
so terminal control codes no longer contribute to visible width, then run/update
any tests or callers that depend on box alignment to validate the fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 47eafb58-0bdb-46d3-89e2-923a58ecf105
📒 Files selected for processing (2)
packages/cli_tools/lib/src/logger/loggers/std_out_logger.dartpackages/cli_tools/test/std_out_logger_test.dart
|
Thanks for the additional review. I pushed a follow-up commit to address ANSI-stripping robustness in boxed logs:
Validation:
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/cli_tools/test/std_out_logger_test.dart (1)
160-199:⚠️ Potential issue | 🟠 MajorExercise the ANSI-enabled branch in these box tests.
These cases currently require
stdout.outputto contain no escape sequences, so they only validate the non-ANSI path. The bug fixed in this PR is on the ANSI-enabled path, where the box is restyled after formatting, so that regression could come back without failing these tests. Add an ANSI-enabled assertion and compare the de-ANSI'd output to the same expected box.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli_tools/test/std_out_logger_test.dart` around lines 160 - 199, The tests currently only assert the non-ANSI path; to exercise the ANSI-enabled branch capture the raw output from collectOutput when calling logger.warning (with BoxLogType) and first assert that stdout.output contains ANSI escape sequences (e.g., contains '\x1B[' or other control sequences), then strip ANSI/control sequences from that raw output and compare the stripped result to the same expected boxed string used currently; update both test cases that use collectOutput/logger.warning/BoxLogType to include this additional assertion so the ANSI-enabled branch is exercised and the de-ANSI'd output equals the expected box.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/cli_tools/test/std_out_logger_test.dart`:
- Around line 160-199: The tests currently only assert the non-ANSI path; to
exercise the ANSI-enabled branch capture the raw output from collectOutput when
calling logger.warning (with BoxLogType) and first assert that stdout.output
contains ANSI escape sequences (e.g., contains '\x1B[' or other control
sequences), then strip ANSI/control sequences from that raw output and compare
the stripped result to the same expected boxed string used currently; update
both test cases that use collectOutput/logger.warning/BoxLogType to include this
additional assertion so the ANSI-enabled branch is exercised and the de-ANSI'd
output equals the expected box.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 99dfbba5-15cf-4456-b3bb-59a174d46dd0
📒 Files selected for processing (2)
packages/cli_tools/lib/src/logger/loggers/std_out_logger.dartpackages/cli_tools/test/std_out_logger_test.dart
| message = _formatAsBox( | ||
| wrapColumn: wrapTextColumn ?? _defaultColumnWrap, | ||
| message: message, | ||
| title: type.title, | ||
| message: _stripAnsiCodes(message), | ||
| title: type.title != null ? _stripAnsiCodes(type.title!) : null, | ||
| ); | ||
| if (ansiSupported) { | ||
| message = _styleByLevel(message, logLevel); |
There was a problem hiding this comment.
Don't drop caller-supplied ANSI styling from boxed logs.
Lines 156-160 build the box from stripped message/title and only reapply _styleByLevel afterward. That removes any inline styling the caller passed in, and LogLevel.info / LogLevel.nothing boxes become plain text on ANSI-capable terminals. Use stripped text for width calculation only; the final render should preserve the original styled content.
|
Thanks — addressed the latest ANSI-path test feedback. What I changed
Commit
Validation
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/cli_tools/test/std_out_logger_test.dart (1)
171-174: Extract the duplicated ANSI-stripping helper.Lines 171-174 and 198-201 repeat the same large regex. A local helper would make these tests easier to read and keep the normalization logic in one place.
Refactor sketch
- final stripped = stdout.output.replaceAll( - RegExp(r'\x1B\[[0-?]*[ -/]*[`@-`~]|\x1B\][^\x07\x1B]*(?:\x07|\x1B\\\\)|\x1B[`@-Z`\\\\-_]'), - '', - ); + final stripped = _stripAnsi(stdout.output); ... - final stripped = stdout.output.replaceAll( - RegExp(r'\x1B\[[0-?]*[ -/]*[`@-`~]|\x1B\][^\x07\x1B]*(?:\x07|\x1B\\\\)|\x1B[`@-Z`\\\\-_]'), - '', - ); + final stripped = _stripAnsi(stdout.output);final _ansiRegex = RegExp( r'\x1B\[[0-?]*[ -/]*[`@-`~]|\x1B\][^\x07\x1B]*(?:\x07|\x1B\\\\)|\x1B[`@-Z`\\\\-_]', ); String _stripAnsi(final String input) => input.replaceAll(_ansiRegex, '');Also applies to: 198-201
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli_tools/test/std_out_logger_test.dart` around lines 171 - 174, Extract the duplicated ANSI-stripping regex into a shared RegExp (e.g. final _ansiRegex = RegExp(r'\x1B\[[0-?]*[ -/]*[`@-`~]|\x1B\][^\x07\x1B]*(?:\x07|\x1B\\\\)|\x1B[`@-Z`\\\\-_]',);) and add a helper String _stripAnsi(String input) => input.replaceAll(_ansiRegex, ''); then replace the two inline replaceAll(RegExp(...), '') usages (the ones operating on stdout.output) with calls to _stripAnsi(stdout.output) so the normalization logic is centralized and the tests are clearer.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli_tools/test/test_utils/io_helper.dart`:
- Around line 12-15: The stderr MockStdout isn't receiving the ansiSupported
flag, so collectOutput(..., ansiSupported: true) still yields
supportsAnsiEscapes == false on standardError; update the creation of
standardError (in the collectOutput helper) to pass ansiSupported: ansiSupported
(same as standardOut) so both MockStdout instances reflect the requested ANSI
capability and allow exercising ANSI-capable stderr paths like
logToStderrLevelThreshold.
---
Nitpick comments:
In `@packages/cli_tools/test/std_out_logger_test.dart`:
- Around line 171-174: Extract the duplicated ANSI-stripping regex into a shared
RegExp (e.g. final _ansiRegex = RegExp(r'\x1B\[[0-?]*[
-/]*[`@-`~]|\x1B\][^\x07\x1B]*(?:\x07|\x1B\\\\)|\x1B[`@-Z`\\\\-_]',);) and add a
helper String _stripAnsi(String input) => input.replaceAll(_ansiRegex, ''); then
replace the two inline replaceAll(RegExp(...), '') usages (the ones operating on
stdout.output) with calls to _stripAnsi(stdout.output) so the normalization
logic is centralized and the tests are clearer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4ffe7b0b-95bd-427e-8543-da946ba95cd4
📒 Files selected for processing (3)
packages/cli_tools/test/std_out_logger_test.dartpackages/cli_tools/test/test_utils/io_helper.dartpackages/cli_tools/test/test_utils/mock_stdout.dart
|
Thanks — addressed the latest review notes with a small follow-up commit. Updates pushed
Commit
Validation
|
2 participants
Summary
_formatAsBoxTesting
dartis not available in this execution environment (dart: command not found), so I could not executedart test.Related
Fixes #104
Summary by CodeRabbit