fix(css/formatter): comment placement in lists#9261
Conversation
🦋 Changeset detectedLatest commit: f0bc401 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 |
WalkthroughThis PR fixes a CSS formatter bug where comments after a colon in property declarations were incorrectly positioned before the property name. It introduces new comment handling logic for CSS generic properties, modifies trailing comment formatting in property declarations, and adds a new layout variant for handling dangling comments in value lists. The changes ensure comments remain correctly placed relative to property colons and values. Possibly related PRs
Suggested labels
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
| /* After (correct) */ | ||
| [lang]:lang(ja) { | ||
| font-family: /* system-ui,*/ | ||
| Hiragino Sans, | ||
| sans-serif; | ||
| } |
There was a problem hiding this comment.
Inconsistent indentation in the "After" example.
Line 21 has extra leading whitespace (sans-serif is indented more than Hiragino Sans). If this reflects actual formatter output, it may indicate the regression mentioned in the PR description. If it's a typo in the changeset, it should be corrected.
📝 If this is a typo, proposed fix
/* After (correct) */
[lang]:lang(ja) {
font-family: /* system-ui,*/
Hiragino Sans,
- sans-serif;
+ sans-serif;
}📝 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.
| /* After (correct) */ | |
| [lang]:lang(ja) { | |
| font-family: /* system-ui,*/ | |
| Hiragino Sans, | |
| sans-serif; | |
| } | |
| /* After (correct) */ | |
| [lang]:lang(ja) { | |
| font-family: /* system-ui,*/ | |
| Hiragino Sans, | |
| sans-serif; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.changeset/fix-css-comment-placement.md around lines 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.
| 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
|
1 participant
Summary
Closes #8409
It's a weird issue because it fixes the comment placement, however it introduces a small regression in the printing of the list. It's weird because the fill IR is still the same... I suspect there's some weird bug inside the printer.
We can leave it like this, or I will try to understand if there's a bug in the printer.
Test Plan
Added new tests
Docs
N/A