-
Notifications
You must be signed in to change notification settings - Fork 85
fix(browser-repl): strip ansi codes from syntax errors COMPASS-9921 #2550
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR addresses a visual issue in the browser REPL where ANSI escape sequences appear in syntax error messages in Compass on Windows. The fix strips ANSI codes from syntax error messages and stack traces to improve readability.
- Adds
strip-ansidependency to remove ANSI escape sequences from syntax errors - Modifies error rendering to conditionally strip ANSI codes only for SyntaxError types
- Includes comprehensive tests for both collapsed and expanded error display modes
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/browser-repl/src/components/types/error-output.tsx | Imports strip-ansi and applies it conditionally to SyntaxError messages and stack traces |
| packages/browser-repl/src/components/types/error-output.spec.tsx | Adds tests verifying ANSI codes are stripped from syntax errors in both collapsed and expanded views |
| packages/browser-repl/package.json | Adds strip-ansi dependency for removing ANSI escape sequences |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| "react-refresh": "^0.14.0", | ||
| "rimraf": "^3.0.2", | ||
| "stream-browserify": "^3.0.0", | ||
| "strip-ansi": "^6.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
latest for this is 7.1.2, any reason for earlier version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, I see, we already use this package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same we use in the rest of the project
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL: Node apparently has https://nodejs.org/api/util.html#utilstripvtcontrolcharactersstr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice :) I think it's fine to just leave a mental note to prefer that in the future, keeping things working as they do for the CLI seems like a perfectly fine approach for now
We're also doing something similar for cli-repl.
At some point babel switched from chalk to picocolors: babel/babel#16359
where color detection seems to be broken: alexeyraspopov/picocolors#85
and there are some pretty complicated attempts at trying to fix it: alexeyraspopov/picocolors#87
with relatively frequent bugs and complaints: alexeyraspopov/picocolors#41
This results in syntax errors in the embedded shell in Compass on Windows having ansi escape sequences for syntax hilighting that's not supported. The workaround here is to just strip any ansi sequences from the message and stack.
I tested this manually on windows using browser-repl's sync-to-compass.js script and it seems to work.