feat(css_formatter): improve SCSS map formatting parity - #10076

crates/biome_css_formatter/src/scss/auxiliary/parenthesized_expression.rs (2)

32-33: Tiny nit: redundant conjunct.

is_outer_parenthesized_map_value is true iff outer_payload_kind.is_some(), so is_outer_parenthesized_map_value && outer_payload_kind == Some(Map) simplifies to the second check alone. Same redundancy on line 49.

♻️ Proposed simplification

-        let should_expand = is_outer_parenthesized_map_value
-            && outer_payload_kind == Some(ScssMapOuterParenthesizedValuePayloadKind::Map);
+        let should_expand =
+            outer_payload_kind == Some(ScssMapOuterParenthesizedValuePayloadKind::Map);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/biome_css_formatter/src/scss/auxiliary/parenthesized_expression.rs`
around lines 32 - 33, The boolean expression setting should_expand is redundant:
remove the unnecessary is_outer_parenthesized_map_value conjunct and replace the
conjunction with a direct comparison of outer_payload_kind to
Some(ScssMapOuterParenthesizedValuePayloadKind::Map) (i.e., set should_expand =
outer_payload_kind == Some(...)); apply the identical simplification to the
other occurrence that currently combines is_outer_parenthesized_map_value with
outer_payload_kind == Some(...).

---

32-59: Trailing comma block is unreachable — intentional, but could simplify.

Your analysis is spot on: given only three ScssMapOuterParenthesizedValuePayloadKind variants (Scalar, List, Map), every case blocks should_print_trailing_comma:

• Scalar → blocked by outer_payload_kind != Some(Scalar)
• List/Map → blocked by inner_expression_owns_trailing_comma

The comments (lines 35–40) make clear this is intentional: Scalar payloads must never gain a trailing comma (singleton list semantics), and List/Map handle their own trailing commas elsewhere. So the trailing_comma block and if_group_breaks machinery are dead code.

If this is scaffolding for a future variant, a brief note would help. Otherwise, consider removing the trailing_comma block or simplifying the condition for clarity.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/biome_css_formatter/src/scss/auxiliary/parenthesized_expression.rs`
around lines 32 - 59, The trailing-comma branch is unreachable given the three
ScssMapOuterParenthesizedValuePayloadKind variants; remove the dead
`trailing_comma` block (and its use of `if_group_breaks`/`with_group_id`) or
replace it with a single-line explanatory comment to indicate this is
intentional scaffolding; update or remove the `should_print_trailing_comma`
and/or `inner_expression_owns_trailing_comma` locals as needed to avoid unused
variable warnings and keep references to
`ScssMapOuterParenthesizedValuePayloadKind`,
`inner_expression_owns_trailing_comma`, `should_print_trailing_comma`, and
`trailing_comma` to locate the code to change.

crates/biome_css_formatter/src/scss/auxiliary/map_expression_pair.rs (1)

63-77: Nitpick: import AnyScssExpressionItem for readability.

The fully-qualified paths on lines 72-74 are a touch noisy — AnyScssExpression is already imported on line 4, so bringing in AnyScssExpressionItem alongside keeps the match arms tidy.

♻️ Suggestion

-use biome_css_syntax::{AnyScssExpression, ScssMapExpressionPair, ScssMapExpressionPairFields};
+use biome_css_syntax::{
+    AnyScssExpression, AnyScssExpressionItem, ScssMapExpressionPair, ScssMapExpressionPairFields,
+};
@@
-    ) || unwrap_single_expression_item(value).is_some_and(|item| {
-        matches!(
-            item,
-            biome_css_syntax::AnyScssExpressionItem::ScssListExpression(_)
-                | biome_css_syntax::AnyScssExpressionItem::ScssMapExpression(_)
-                | biome_css_syntax::AnyScssExpressionItem::ScssParenthesizedExpression(_)
-        )
-    })
+    ) || unwrap_single_expression_item(value).is_some_and(|item| {
+        matches!(
+            item,
+            AnyScssExpressionItem::ScssListExpression(_)
+                | AnyScssExpressionItem::ScssMapExpression(_)
+                | AnyScssExpressionItem::ScssParenthesizedExpression(_)
+        )
+    })

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/biome_css_formatter/src/scss/auxiliary/map_expression_pair.rs` around
lines 63 - 77, Import biome_css_syntax::AnyScssExpressionItem and replace the
fully-qualified uses in value_manages_its_own_breaking (the match inside
unwrap_single_expression_item closure) with the shorter
AnyScssExpressionItem::ScssListExpression, ::ScssMapExpression, and
::ScssParenthesizedExpression variants to improve readability while leaving
logic intact.

crates/biome_css_formatter/src/scss/auxiliary/map_expression.rs (2)

61-69: Nitpick: compute dangling_comments once.

dangling_comments(self.node.syntax()) is called twice in this predicate (plus is_empty() is redundant with the all check — an empty iterator would make all vacuously true, but you already guard with pairs().len() > 0).

♻️ Suggestion

 fn has_inline_closing_comments(&self, f: &CssFormatter) -> bool {
-    self.node.pairs().len() > 0
-        && !f.context().comments().dangling_comments(self.node.syntax()).is_empty()
-        && f.context()
-            .comments()
-            .dangling_comments(self.node.syntax())
-            .iter()
-            .all(|comment| comment.kind().is_inline() && comment.lines_before() == 0)
+    if self.node.pairs().len() == 0 {
+        return false;
+    }
+    let comments = f.context().comments();
+    let dangling = comments.dangling_comments(self.node.syntax());
+    !dangling.is_empty()
+        && dangling
+            .iter()
+            .all(|comment| comment.kind().is_inline() && comment.lines_before() == 0)
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/biome_css_formatter/src/scss/auxiliary/map_expression.rs` around lines
61 - 69, In has_inline_closing_comments, avoid calling
f.context().comments().dangling_comments(self.node.syntax()) multiple times and
drop the redundant is_empty() check; compute let dangling =
f.context().comments().dangling_comments(self.node.syntax()) once, then check
self.node.pairs().len() > 0 && dangling.iter().all(|comment|
comment.kind().is_inline() && comment.lines_before() == 0) so the predicate uses
the cached dangling_comments variable (referenced in the
has_inline_closing_comments method and self.node.pairs()).

---

13-27: Tiny inefficiency: GroupId allocated but unused here.

handles_dangling_comments never touches self.group_id, yet fmt_dangling_comments still allocates one via f.group_id(...). Not a bug, but a new ScssMapLayout constructor (or free function) that takes only the node would avoid the churn.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/biome_css_formatter/src/scss/auxiliary/map_expression.rs` around lines
13 - 27, The call in fmt_dangling_comments creates a GroupId via f.group_id(...)
only to pass it into ScssMapLayout::new even though handles_dangling_comments
does not use that GroupId; change ScssMapLayout API or call site so no GroupId
is allocated: add a constructor or helper like
ScssMapLayout::new_from_node(node) or ScssMapLayout::for_node(node) (or a free
function) that accepts only &ScssMapExpression and use that in
fmt_dangling_comments when invoking handles_dangling_comments, leaving the
existing GroupId-taking constructor for callers that need grouping.

crates/biome_css_formatter/src/comments.rs (1)

101-120: Optional: dedupe the three-identical-dispatch-arms.

All three text-position arms run the exact same handler chain. Since this file already had the pattern, it's no regression, but the duplication grows with every new handler. A tiny refactor would pay off next time someone adds one.

♻️ Suggestion

// Outside the match, or via a single arm:
match comment.text_position() {
    CommentTextPosition::EndOfLine
    | CommentTextPosition::OwnLine
    | CommentTextPosition::SameLine => handle_scss_map_trailing_separator_comment(comment)
        .or_else(handle_function_comment)
        .or_else(handle_generic_property_comment)
        .or_else(handle_declaration_name_comment)
        .or_else(handle_complex_selector_comment)
        .or_else(handle_global_suppression),
}

🤖 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 101 - 120, The three
match arms for comment.text_position() (CommentTextPosition::EndOfLine,
::OwnLine, ::SameLine) all invoke the same handler chain
(handle_scss_map_trailing_separator_comment, handle_function_comment,
handle_generic_property_comment, handle_declaration_name_comment,
handle_complex_selector_comment, handle_global_suppression); refactor by
collapsing those three arms into a single pattern (e.g.,
CommentTextPosition::EndOfLine | CommentTextPosition::OwnLine |
CommentTextPosition::SameLine) that calls the shared chain, or alternatively
assign the chain to a local variable or helper function and call it from the
single arm to avoid duplication while preserving the same behavior.

crates/biome_css_formatter/src/scss/auxiliary/map_expression.rs (2)

66-78: Two dangling_comments lookups where one will do.

is_empty() followed by iter().all(..) fetches the dangling slice twice. Bind once:

♻️ Optional tidy

-    fn has_inline_closing_comments(&self, f: &CssFormatter) -> bool {
-        self.node.pairs().len() > 0
-            && !f
-                .context()
-                .comments()
-                .dangling_comments(self.node.syntax())
-                .is_empty()
-            && f.context()
-                .comments()
-                .dangling_comments(self.node.syntax())
-                .iter()
-                .all(|comment| comment.kind().is_inline() && comment.lines_before() == 0)
-    }
+    fn has_inline_closing_comments(&self, f: &CssFormatter) -> bool {
+        if self.node.pairs().len() == 0 {
+            return false;
+        }
+        let dangling = f.context().comments().dangling_comments(self.node.syntax());
+        !dangling.is_empty()
+            && dangling
+                .iter()
+                .all(|c| c.kind().is_inline() && c.lines_before() == 0)
+    }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/biome_css_formatter/src/scss/auxiliary/map_expression.rs` around lines
66 - 78, In has_inline_closing_comments, avoid calling
f.context().comments().dangling_comments(self.node.syntax()) twice: store that
slice in a local let (e.g., let dangling =
f.context().comments().dangling_comments(self.node.syntax())) and then use
dangling.is_empty() and dangling.iter().all(...) so the dangling_comments lookup
only occurs once; update the function body to reference that local variable
instead of repeating the call.

---

9-27: Double ScssMapLayout::new — tiny redundancy.

fmt_fields and fmt_dangling_comments both build a fresh ScssMapLayout (and each in turn invokes has_inline_closing_comments, which calls dangling_comments(..) twice). group_id("scss_map_expression") returns the same id, so behaviour is identical — only cosmetic. If you feel like reducing the comment-lookup fan-out, caching the dangling comments once inside a single layout instance would pay for itself.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/biome_css_formatter/src/scss/auxiliary/map_expression.rs` around lines
9 - 27, Both fmt_fields and fmt_dangling_comments recreate ScssMapLayout (via
ScssMapLayout::new(node, f.group_id("scss_map_expression"))) causing duplicate
work and repeated calls to has_inline_closing_comments / dangling_comments; fix
by creating a single ScssMapLayout instance and reusing it (e.g., instantiate
layout = ScssMapLayout::new(node, f.group_id("scss_map_expression")) and use
layout.fmt(f) in fmt_fields and layout.handles_dangling_comments(f) /
layout.fmt(...) in fmt_dangling_comments), or alternatively move the
dangling_comments caching into ScssMapLayout so its dangling_comments() is
computed once and reused by has_inline_closing_comments and other callers.

crates/biome_css_formatter/src/comments.rs (1)

101-121: Tiny tidy: the OwnLine arm never benefits from the new handler.

handle_scss_map_trailing_separator_comment short-circuits to Default whenever comment.text_position().is_own_line() (Line 138), so listing it in the OwnLine arm is a guaranteed no-op. Not a bug — just a bit of sleight of hand that will puzzle the next reader. Feel free to drop it there, or consolidate the three arms since they're now identical anyway.

♻️ Optional tidy

-            CommentTextPosition::EndOfLine => handle_scss_map_trailing_separator_comment(comment)
-                .or_else(handle_function_comment)
-                .or_else(handle_generic_property_comment)
-                .or_else(handle_declaration_name_comment)
-                .or_else(handle_complex_selector_comment)
-                .or_else(handle_global_suppression),
-            CommentTextPosition::OwnLine => handle_scss_map_trailing_separator_comment(comment)
-                .or_else(handle_function_comment)
-                .or_else(handle_generic_property_comment)
-                .or_else(handle_declaration_name_comment)
-                .or_else(handle_complex_selector_comment)
-                .or_else(handle_global_suppression),
-            CommentTextPosition::SameLine => handle_scss_map_trailing_separator_comment(comment)
-                .or_else(handle_function_comment)
-                .or_else(handle_generic_property_comment)
-                .or_else(handle_declaration_name_comment)
-                .or_else(handle_complex_selector_comment)
-                .or_else(handle_global_suppression),
+            CommentTextPosition::EndOfLine
+            | CommentTextPosition::OwnLine
+            | CommentTextPosition::SameLine => {
+                handle_scss_map_trailing_separator_comment(comment)
+                    .or_else(handle_function_comment)
+                    .or_else(handle_generic_property_comment)
+                    .or_else(handle_declaration_name_comment)
+                    .or_else(handle_complex_selector_comment)
+                    .or_else(handle_global_suppression)
+            }

🤖 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 101 - 121, The
OwnLine match arm includes handle_scss_map_trailing_separator_comment which
always returns Default for comments where comment.text_position().is_own_line(),
so remove that no-op to avoid confusion: update the match over
comment.text_position() (matching CommentTextPosition::EndOfLine, ::OwnLine,
::SameLine) by either (a) dropping handle_scss_map_trailing_separator_comment
from the CommentTextPosition::OwnLine arm so it mirrors the effective logic of
that position, or (b) consolidate the three arms into a single shared chain of
handlers (handle_scss_map_trailing_separator_comment, handle_function_comment,
handle_generic_property_comment, handle_declaration_name_comment,
handle_complex_selector_comment, handle_global_suppression) to eliminate the
redundant listing; adjust based on preference and ensure behavior of
handle_function_comment, handle_generic_property_comment,
handle_declaration_name_comment, handle_complex_selector_comment, and
handle_global_suppression remains unchanged.

crates/biome_css_formatter/src/scss/auxiliary/map_expression.rs (1)

13-26: Throwaway group_id in fmt_dangling_comments.

handles_dangling_comments never touches self.group_id, so calling f.group_id("scss_map_expression") here just burns a counter each time. Consider extracting the check as a free function (or associated fn) that takes &ScssMapExpression and &CssFormatter, so only fmt_fields pays for the group-id allocation.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/biome_css_formatter/src/scss/auxiliary/map_expression.rs` around lines
13 - 26, The call in fmt_dangling_comments is wasting a group-id by invoking
f.group_id("scss_map_expression") even though handles_dangling_comments doesn't
use the group id; refactor by adding a free function or associated function
(e.g., ScssMapLayout::handles_dangling_comments_for(node: &ScssMapExpression, f:
&CssFormatter) -> bool) that performs the same check without allocating a group
id, update fmt_dangling_comments to call that new function (or pass f directly)
and ensure only fmt_fields (where the group id is actually needed) calls
f.group_id("scss_map_expression") to pay for the allocation.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

crates/biome_css_formatter/src/comments.rs (1)

108-113: Tiny redundancy: handler is a no-op in the OwnLine arm.

handle_scss_map_trailing_separator_comment bails out early via comment.text_position().is_own_line() at line 138, so chaining it into the OwnLine branch will always fall through to handle_function_comment. Harmless, but you can drop it from that arm (or drop the is_own_line() guard) to avoid misleading readers about where this handler can fire.

🤖 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 108 - 113, The
OwnLine match arm currently calls handle_scss_map_trailing_separator_comment
even though handle_scss_map_trailing_separator_comment immediately returns early
when comment.text_position().is_own_line(), so remove that handler from the
CommentTextPosition::OwnLine arm (leaving handle_function_comment,
handle_generic_property_comment, handle_declaration_name_comment,
handle_complex_selector_comment, handle_global_suppression) to avoid the
misleading no-op, or alternatively remove the internal is_own_line() guard
inside handle_scss_map_trailing_separator_comment if the intention is that it
should run for OwnLine; reference CommentTextPosition::OwnLine and
handle_scss_map_trailing_separator_comment when making the change.