fix(css/formatter): comment placement in lists#9261
Conversation
🦋 Changeset detectedLatest commit: c7fa905 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds a changeset entry and fixes CSS comment placement in the formatter. Introduces a handler for generic property comments, updates generic property formatting to emit trailing comments inline after the colon (moving the value to the next line when needed), and adds a new value-list layout variant to handle dangling comments between colon and values. Adds tests covering comments in property values and around property names. No public API signatures were changed aside from an added internal enum variant and private helpers. Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/biome_css_formatter/src/css/properties/generic_property.rs (1)
23-28: Semantic naming: usingFormatCssLeadingCommentfor trailing comments.Using
FormatCssLeadingCommentto format trailing comments works functionally but is semantically confusing. If the formatting logic is identical for both leading and trailing comments, consider renaming to something generic likeFormatCssComment, or add a brief comment explaining the reuse.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_css_formatter/src/css/properties/generic_property.rs` around lines 23 - 28, The code uses FormatCssLeadingComment to format trailing_comments in the loop (see FormatRefWithRule::new(comment, FormatCssLeadingComment::default()) and comment.mark_formatted()), which is semantically confusing; rename the formatter to a neutral name (e.g., FormatCssComment) or introduce a new alias/type that wraps/re-exports FormatCssLeadingComment with a clear name, update all references (including where FormatRefWithRule is constructed for both leading and trailing comments) to use the new name, or alternatively add a one-line comment above the loop explaining that the leading-comment formatter is intentionally reused for trailing comments; ensure the change preserves existing behavior and updates any imports/uses of FormatCssLeadingComment accordingly.crates/biome_css_formatter/src/comments.rs (1)
140-147: Consider alternative matching strategy for trivia comparison.The use of
piece.text() == comment_piece.text()is the only text comparison for trivia matching in the codebase. If identical comments appear multiple times in trailing trivia, this could incorrectly match the wrong piece. Explore whether comparing trivia ranges, offsets, or leveraging structural equality would be more robust than text equality.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_css_formatter/src/comments.rs` around lines 140 - 147, The current comparison in the trailing trivia loop (inside the block that calls name.syntax().last_token()) only checks piece.text() == comment_piece.text(), which can misidentify identical comment text occurring multiple times; update the match to use a positional or structural comparison instead—for example compare the trivia piece's text range/offset (e.g., piece.text_range() or piece.range()) against comment_piece's range, or use a structural equality method on TriviaPiece if available, so the loop in comments.rs returns CommentPlacement::trailing(name.into_syntax(), comment) only when the exact trivia instance (not just identical text) matches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/fix-css-comment-placement.md:
- Around line 17-22: The "After" example has inconsistent indentation: the
'sans-serif' token is indented further than 'Hiragino Sans' in the font-family
block for the [lang]:lang(ja) selector; fix by aligning 'sans-serif' to the same
indentation level as 'Hiragino Sans' in .changeset/fix-css-comment-placement.md
(or, if this reflects real formatter output from your CSS formatter, adjust the
formatter/config or add a regression test to ensure comment placement doesn't
change indentation). Ensure the font-family lines under the [lang]:lang(ja)
block use consistent indentation.
In `@crates/biome_css_formatter/src/utils/component_value_list.rs`:
- Around line 197-204: Remove the temporary dbg!() calls left in
component_value_list.rs around the conditional that chooses between
hard_line_break() and space(): delete the dbg!("here1") and dbg!("here2")
invocations so they no longer print to stdout; keep the surrounding logic that
calls write!(f, [hard_line_break()])? and write!(f, [space()])? intact and run
tests/formatting to confirm no behavior change. Use the nearby identifiers
(hard_line_break(), space(), at_group_boundary, and the write!(f, ...) calls) to
locate the exact spots to clean up.
---
Nitpick comments:
In `@crates/biome_css_formatter/src/comments.rs`:
- Around line 140-147: The current comparison in the trailing trivia loop
(inside the block that calls name.syntax().last_token()) only checks
piece.text() == comment_piece.text(), which can misidentify identical comment
text occurring multiple times; update the match to use a positional or
structural comparison instead—for example compare the trivia piece's text
range/offset (e.g., piece.text_range() or piece.range()) against comment_piece's
range, or use a structural equality method on TriviaPiece if available, so the
loop in comments.rs returns CommentPlacement::trailing(name.into_syntax(),
comment) only when the exact trivia instance (not just identical text) matches.
In `@crates/biome_css_formatter/src/css/properties/generic_property.rs`:
- Around line 23-28: The code uses FormatCssLeadingComment to format
trailing_comments in the loop (see FormatRefWithRule::new(comment,
FormatCssLeadingComment::default()) and comment.mark_formatted()), which is
semantically confusing; rename the formatter to a neutral name (e.g.,
FormatCssComment) or introduce a new alias/type that wraps/re-exports
FormatCssLeadingComment with a clear name, update all references (including
where FormatRefWithRule is constructed for both leading and trailing comments)
to use the new name, or alternatively add a one-line comment above the loop
explaining that the leading-comment formatter is intentionally reused for
trailing comments; ensure the change preserves existing behavior and updates any
imports/uses of FormatCssLeadingComment accordingly.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (11)
crates/biome_css_formatter/tests/specs/css/atrule/import.css.snapis excluded by!**/*.snapand included by**crates/biome_css_formatter/tests/specs/css/comments/property-value-comment.css.snapis excluded by!**/*.snapand included by**crates/biome_css_formatter/tests/specs/css/scss/declaration/mixed.scss.snapis excluded by!**/*.snapand included by**crates/biome_css_formatter/tests/specs/css/scss/declaration/nested-properties-empty-value.scss.snapis excluded by!**/*.snapand included by**crates/biome_css_formatter/tests/specs/css/scss/declaration/parent-and-colon-values.scss.snapis excluded by!**/*.snapand included by**crates/biome_css_formatter/tests/specs/css/scss/expression/list-map-paren.scss.snapis excluded by!**/*.snapand included by**crates/biome_css_formatter/tests/specs/css/scss/expression/precedence.scss.snapis excluded by!**/*.snapand included by**crates/biome_css_formatter/tests/specs/css/unary-precedence.css.snapis excluded by!**/*.snapand included by**crates/biome_css_formatter/tests/specs/prettier/css/comments/at-rules.css.snapis excluded by!**/*.snapand included by**crates/biome_css_formatter/tests/specs/prettier/css/comments/custom-properties.css.snapis excluded by!**/*.snapand included by**crates/biome_css_formatter/tests/specs/prettier/css/comments/declaration.css.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (5)
.changeset/fix-css-comment-placement.mdcrates/biome_css_formatter/src/comments.rscrates/biome_css_formatter/src/css/properties/generic_property.rscrates/biome_css_formatter/src/utils/component_value_list.rscrates/biome_css_formatter/tests/specs/css/comments/property-value-comment.css
| dbg!("here1"); | ||
| write!(f, [hard_line_break()])?; | ||
| } else { | ||
| write!(f, [space()])?; | ||
| } | ||
| } else if at_group_boundary { | ||
| dbg!("here2"); | ||
| write!(f, [hard_line_break()])?; |
There was a problem hiding this comment.
Remove debug statements before merge.
These dbg!() calls appear to be temporary debugging hooks left in the code. While the coding guidelines do mention using dbg!() for debugging, these should be removed before the PR is merged to avoid noisy output during formatting.
🧹 Proposed fix
if has_leading_newline {
- dbg!("here1");
write!(f, [hard_line_break()])?;
} else {
write!(f, [space()])?;
}
} else if at_group_boundary {
- dbg!("here2");
write!(f, [hard_line_break()])?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| dbg!("here1"); | |
| write!(f, [hard_line_break()])?; | |
| } else { | |
| write!(f, [space()])?; | |
| } | |
| } else if at_group_boundary { | |
| dbg!("here2"); | |
| write!(f, [hard_line_break()])?; | |
| write!(f, [hard_line_break()])?; | |
| } else { | |
| write!(f, [space()])?; | |
| } | |
| } else if at_group_boundary { | |
| write!(f, [hard_line_break()])?; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/biome_css_formatter/src/utils/component_value_list.rs` around lines
197 - 204, Remove the temporary dbg!() calls left in component_value_list.rs
around the conditional that chooses between hard_line_break() and space():
delete the dbg!("here1") and dbg!("here2") invocations so they no longer print
to stdout; keep the surrounding logic that calls write!(f, [hard_line_break()])?
and write!(f, [space()])? intact and run tests/formatting to confirm no behavior
change. Use the nearby identifiers (hard_line_break(), space(),
at_group_boundary, and the write!(f, ...) calls) to locate the exact spots to
clean up.
Merging this PR will not alter performance
Comparing 
Footnotes
|
|
Does the regression happen only with a list with comments? |
Only those lists where we use |
f0bc401 to
4fa0c9e
Compare
Summary
Closes #8409
I added a new layout so that comments are properly printed.
Test Plan
Added new tests
Docs
N/A