Some playground improvements - #4213
Conversation
| let text = `${start.line}:${start.col}-${end.line}:${end.col}`; | ||
|
|
||
| if (includeSlice) { | ||
| const slice = source.slice(loc.startOffset, loc.startOffset + loc.length); |
There was a problem hiding this comment.
So, this is not correct. slice does by character but the offsets are in bytes and I don't know how you'd handle this in javascript. It's already the same problem with offsetToLineCol but here it would be a bit more noticable I think
There was a problem hiding this comment.
🟡 Changes recommended
The current implementation introduces a DOM injection risk when rendering location slices and has correctness issues in location column mapping and string-wrapper detection that can break navigation/rendering.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR improves the Prism playground’s AST/diagnostics rendering by enhancing how locations and certain value types are displayed, aiming to better match Ruby’s inspect-style output and improve usability when navigating locations.
Changes:
- Reuse shared
TextEncoder/TextDecoderinstances and switch location calculations to use UTF-8 bytes. - Improve AST rendering: show
location:prefix, include slices for location fields, and handle Prism string wrapper objects. - Adjust styling for empty (
∅) locations.
File summaries
| File | Description |
|---|---|
| doc/playground.js | Updates AST/diagnostic rendering, location formatting, and string-type display; adds shared encoder/decoder usage. |
| doc/playground.css | Changes the color used for empty (∅) values in the AST tree. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
ed00ad8 to
59ead1c
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The updated byte-based line/column calculation produces incorrect columns for non-ASCII text, which will misalign Monaco cursor/highlighting and displayed locations.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
doc/playground.js:252
offsetToLineColcurrently incrementscolper UTF-8 byte. Monaco columns (and the displayed column numbers) are based on the decoded string/UTF-16 code units, so offsets after non-ASCII characters will produce incorrect column values and misaligned highlights.
function offsetToLineCol(utf8Bytes, offset) {
let line = 1, col = 0;
for (let i = 0; i < offset && i < utf8Bytes.length; i++) {
// Check for newline
if (utf8Bytes[i] === 10) { line++; col = 0; }
doc/playground.css:272
- Missing trailing semicolon in this declaration; the surrounding CSS uses semicolons consistently, and keeping that convention reduces the risk of mistakes when adding more properties later.
.tree-number {
color: #098658
}
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
* Add color to empty locations, numbers * Format constant type like ruby symbol * Fix display of string type as `[Object Object]` * Include slice for location fields * Add `location: ` to node locations like in ruby inspect output
Javascript strings are always utf16, TextEncoder converts it to utf-8 bytes. Doing it like this seems like a fine solution
59ead1c to
b4a5db9
Compare
[Object Object]location:to node locations like in ruby inspect outputBefore:
After: