Merged
Conversation
🦋 Changeset detectedLatest commit: a24e4ea 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 |
Merging this PR will not alter performance
Comparing Footnotes
|
2f8e520 to
81dc94d
Compare
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR adds a nursery lint rule 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/biome_css_semantic/src/events.rs (1)
272-285:⚠️ Potential issue | 🟠 MajorKeep
:roottied to its owning rule, not every nested at-rule.This now ends the root-selector scope when an inner
@supports,@container, or@starting-styleexits. In:root {@supports(...) { ... } color: red; }, the declaration after the at-rule loses root context. Please track which rule opened:root(for example with rule depth/stack) and only emitRootSelectorEndwhen that rule closes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_css_semantic/src/events.rs` around lines 272 - 285, The current logic uses is_in_root_selector boolean and emits SemanticEvent::RootSelectorEnd for every rule end, which closes a :root scope even when an inner at-rule ends; instead record which rule started the :root (e.g., push the current rule depth or rule identifier onto a root-selector stack when you emit SemanticEvent::RootSelectorStart) and only emit SemanticEvent::RootSelectorEnd when the closing rule matches that recorded rule (i.e., when popping the same rule id/depth), updating stash pushes (SemanticEvent::RuleEnd and conditional SemanticEvent::RootSelectorEnd) to check the top of that stack rather than is_in_root_selector; update any code paths that set/reset is_in_root_selector (replace with stack push/pop) so nested at-rules won’t prematurely end the :root scope.
🧹 Nitpick comments (5)
.changeset/add-no-duplicate-selectors.md (1)
5-5: Minor grammar nit.The comma after the rule name creates a slightly awkward restrictive clause. Consider: "Added a new nursery CSS rule
noDuplicateSelectorsthat disallows..." (no comma).📝 Suggested fix
-Added a new nursery CSS rule `noDuplicateSelectors`, that disallows duplicate selector lists within the same at-rule context. +Added a new nursery CSS rule `noDuplicateSelectors` that disallows duplicate selector lists within the same at-rule context.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.changeset/add-no-duplicate-selectors.md at line 5, Remove the extra comma after the rule name in the changelog sentence so it reads: "Added a new nursery CSS rule `noDuplicateSelectors` that disallows duplicate selector lists within the same at-rule context." Locate the sentence mentioning `noDuplicateSelectors` in the .changeset description and delete the comma immediately following the inline code symbol.crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs (2)
87-96: Consider moving struct belowimpl Ruleblock.Per codebase conventions, helper structs should generally be placed below the
impl Ruleblock. TheDuplicateSelectorListstate type could be moved after line 216.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs` around lines 87 - 96, Move the helper state struct DuplicateSelectorList so it appears after the impl Rule block (i.e., place its definition below the Rule implementation), keeping its fields, derives, and privacy unchanged; ensure any references to DuplicateSelectorList within the file still resolve (no forward-declaration changes needed) and run tests/formatting to confirm no references break after reordering.
173-173: Minor allocation on each selector check.
at_rule_context.clone()allocates a newVecfor every qualified rule. For typical stylesheets this is fine, but for very large files it could add up. A potential optimisation would be to use a hash of the context or intern the context vectors, but this is likely premature optimisation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs` at line 173, The code currently creates a new Vec clone for every qualified rule via at_rule_context.clone() when building context_key; avoid that allocation by using references or an interned/keyed representation instead—e.g. build context_key as a tuple of references to at_rule_context and list_key (or use a stable hash/intern for at_rule_context) so you stop cloning the Vec on each check; update usages that expect an owned key accordingly (or convert to a hashed key) in no_duplicate_selectors.rs where context_key, at_rule_context, and list_key are used.crates/biome_css_semantic/src/tests/selector.rs (1)
162-192: Ignored test documents a known edge case.The
#[ignore]attribute is appropriate here. Consider adding a brief comment or linking to an issue explaining why> liresolution isn't working yet.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_css_semantic/src/tests/selector.rs` around lines 162 - 192, The test test_resolve_selector_with_complex_parent is intentionally marked #[ignore] because resolving nested child-only selectors like "> li" against a complex parent (resolved selector ".menu > ul") is a known edge case; keep the #[ignore] but add a one-line explanatory comment above the test (or a TODO) referencing the root cause (selector resolution of child combinators) and link to the tracker/issue number (or create one) so future readers know why it’s skipped and where to follow progress.crates/biome_css_semantic/src/tests/specificity.rs (1)
370-380: Known limitations documented.The TODO comments here acknowledge specificity calculation bugs for deeply nested rules. Good practice to document these as tracked issues rather than silently skipping them.
Would you like me to open an issue to track these specificity bugs for deeper nesting levels?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_css_semantic/src/tests/specificity.rs` around lines 370 - 380, Replace the inline TODOs in the tests with a tracked issue reference: open a repository issue describing the correctness bug in specificity calculation for nested selectors, include the failing examples from crates/biome_css_semantic/src/tests/specificity.rs (the commented cases using class_rule/class_selector and id_rule/id_selector that expect Specificity(0,1,2) and Specificity(1,1,2)), attach the actual vs expected specificity values and test snippet, and add the new issue ID or URL into the test file comment so the skipped assertions point to the tracked issue for follow-up.
🤖 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/add-no-duplicate-selectors.md:
- Line 7: Replace the typo "trigges" with "triggers" in the sentence that starts
"For example, the following snippet trigges the rule..." so the sentence reads
"For example, the following snippet triggers the rule because the second
selector and the first selector are the same:"; update only that word where it
appears.
In `@crates/biome_css_semantic/src/semantic_model/mod.rs`:
- Around line 235-236: Update the inaccurate test comment that currently says
"'blue' in '.child'" to reference the actual CSS token used (`var(--foo)`);
locate the nearby TextRange::new(60.into(), 64.into()) instantiation and the
comment above the `range` variable and change the text to mention the range of
the declaration `var(--foo)` in '.child' so the comment matches the CSS fixture.
---
Outside diff comments:
In `@crates/biome_css_semantic/src/events.rs`:
- Around line 272-285: The current logic uses is_in_root_selector boolean and
emits SemanticEvent::RootSelectorEnd for every rule end, which closes a :root
scope even when an inner at-rule ends; instead record which rule started the
:root (e.g., push the current rule depth or rule identifier onto a root-selector
stack when you emit SemanticEvent::RootSelectorStart) and only emit
SemanticEvent::RootSelectorEnd when the closing rule matches that recorded rule
(i.e., when popping the same rule id/depth), updating stash pushes
(SemanticEvent::RuleEnd and conditional SemanticEvent::RootSelectorEnd) to check
the top of that stack rather than is_in_root_selector; update any code paths
that set/reset is_in_root_selector (replace with stack push/pop) so nested
at-rules won’t prematurely end the :root scope.
---
Nitpick comments:
In @.changeset/add-no-duplicate-selectors.md:
- Line 5: Remove the extra comma after the rule name in the changelog sentence
so it reads: "Added a new nursery CSS rule `noDuplicateSelectors` that disallows
duplicate selector lists within the same at-rule context." Locate the sentence
mentioning `noDuplicateSelectors` in the .changeset description and delete the
comma immediately following the inline code symbol.
In `@crates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rs`:
- Around line 87-96: Move the helper state struct DuplicateSelectorList so it
appears after the impl Rule block (i.e., place its definition below the Rule
implementation), keeping its fields, derives, and privacy unchanged; ensure any
references to DuplicateSelectorList within the file still resolve (no
forward-declaration changes needed) and run tests/formatting to confirm no
references break after reordering.
- Line 173: The code currently creates a new Vec clone for every qualified rule
via at_rule_context.clone() when building context_key; avoid that allocation by
using references or an interned/keyed representation instead—e.g. build
context_key as a tuple of references to at_rule_context and list_key (or use a
stable hash/intern for at_rule_context) so you stop cloning the Vec on each
check; update usages that expect an owned key accordingly (or convert to a
hashed key) in no_duplicate_selectors.rs where context_key, at_rule_context, and
list_key are used.
In `@crates/biome_css_semantic/src/tests/selector.rs`:
- Around line 162-192: The test test_resolve_selector_with_complex_parent is
intentionally marked #[ignore] because resolving nested child-only selectors
like "> li" against a complex parent (resolved selector ".menu > ul") is a known
edge case; keep the #[ignore] but add a one-line explanatory comment above the
test (or a TODO) referencing the root cause (selector resolution of child
combinators) and link to the tracker/issue number (or create one) so future
readers know why it’s skipped and where to follow progress.
In `@crates/biome_css_semantic/src/tests/specificity.rs`:
- Around line 370-380: Replace the inline TODOs in the tests with a tracked
issue reference: open a repository issue describing the correctness bug in
specificity calculation for nested selectors, include the failing examples from
crates/biome_css_semantic/src/tests/specificity.rs (the commented cases using
class_rule/class_selector and id_rule/id_selector that expect Specificity(0,1,2)
and Specificity(1,1,2)), attach the actual vs expected specificity values and
test snippet, and add the new issue ID or URL into the test file comment so the
skipped assertions point to the tracked issue for follow-up.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fba70917-d3a2-4a17-a86a-9c5c56723d7a
⛔ Files ignored due to path filters (7)
Cargo.lockis excluded by!**/*.lockand included by**crates/biome_configuration/src/analyzer/linter/rules.rsis excluded by!**/rules.rsand included by**crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.css.snapis excluded by!**/*.snapand included by**crates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.css.snapis excluded by!**/*.snapand included by**crates/biome_diagnostics_categories/src/categories.rsis excluded by!**/categories.rsand included by**packages/@biomejs/backend-jsonrpc/src/workspace.tsis excluded by!**/backend-jsonrpc/src/workspace.tsand included by**packages/@biomejs/biome/configuration_schema.jsonis excluded by!**/configuration_schema.jsonand included by**
📒 Files selected for processing (16)
.changeset/add-no-duplicate-selectors.mdcrates/biome_css_analyze/src/lint/nursery/no_duplicate_selectors.rscrates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/invalid.csscrates/biome_css_analyze/tests/specs/nursery/noDuplicateSelectors/valid.csscrates/biome_css_semantic/Cargo.tomlcrates/biome_css_semantic/src/events.rscrates/biome_css_semantic/src/format_semantic_model.rscrates/biome_css_semantic/src/lib.rscrates/biome_css_semantic/src/semantic_model/builder.rscrates/biome_css_semantic/src/semantic_model/mod.rscrates/biome_css_semantic/src/semantic_model/model.rscrates/biome_css_semantic/src/tests/mod.rscrates/biome_css_semantic/src/tests/selector.rscrates/biome_css_semantic/src/tests/specificity.rscrates/biome_rule_options/src/lib.rscrates/biome_rule_options/src/no_duplicate_selectors.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/biome_css_semantic/src/tests/selector.rs`:
- Around line 156-158: The ignored test
test_resolve_selector_with_complex_parent must be un-ignored by implementing
proper handling of relative child selectors (e.g., "> li") in the selector
resolution/flattening pipeline: locate the selector resolution function
(resolve_selector or the selector flattening routine used before
noDuplicateSelectors) and add logic to correctly splice/interpret leading child
combinators against complex parent selectors so that "> li" produces the
expected flattened selectors; run the test and adjust the noDuplicateSelectors
deduplication step if it assumes fully-resolved selectors to ensure the
newly-flattened results are accepted by the deduper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 983078c0-cefd-4188-b2ef-344ca1f17899
📒 Files selected for processing (2)
crates/biome_css_semantic/src/tests/selector.rscrates/biome_css_semantic/src/tests/specificity.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/biome_css_semantic/src/tests/specificity.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/biome_css_semantic/src/tests/specificity.rs (1)
7-19: Consider extracting a test helper to reduce boilerplate.The test setup pattern (parse → tree → model → rule → selector) repeats across all 21 tests. A small helper could tidy things up:
fn get_first_selector(css_code: &str) -> ResolvedSelector { let parse = parse_css(css_code, CssFileSource::css(), CssParserOptions::default()); let root = parse.tree(); let model = semantic_model(&root); let rule = model.rules().first().unwrap(); rule.selectors[0].clone() }This would let tests focus on the assertion logic. Entirely optional — the current approach is perfectly readable.
Also applies to: 21-33, 35-47
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_css_semantic/src/tests/specificity.rs` around lines 7 - 19, Extract a small test helper to remove duplicated setup in the specificity tests: add a function (e.g., get_first_selector(css_code: &str) -> ResolvedSelector) that calls parse_css(...), gets .tree(), calls semantic_model(&root), takes model.rules().first().unwrap() and returns rule.selectors[0].clone(); then replace the repeated parse→tree→model→rule→selector sequence in each test with a single call to get_first_selector and keep the existing assertions (references: parse_css, CssFileSource::css, CssParserOptions::default, semantic_model, model.rules, ResolvedSelector).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/biome_css_semantic/src/tests/specificity.rs`:
- Around line 345-347: Update the panic message on the get_rule_by_id call to
match the actual selector being tested: change the expect string for the
retrieval of class_rule (the call to
model.get_rule_by_id(class_rule_id).expect(...)) to reference ".class" instead
of "span.class" so the message accurately reflects the selector under test.
---
Nitpick comments:
In `@crates/biome_css_semantic/src/tests/specificity.rs`:
- Around line 7-19: Extract a small test helper to remove duplicated setup in
the specificity tests: add a function (e.g., get_first_selector(css_code: &str)
-> ResolvedSelector) that calls parse_css(...), gets .tree(), calls
semantic_model(&root), takes model.rules().first().unwrap() and returns
rule.selectors[0].clone(); then replace the repeated
parse→tree→model→rule→selector sequence in each test with a single call to
get_first_selector and keep the existing assertions (references: parse_css,
CssFileSource::css, CssParserOptions::default, semantic_model, model.rules,
ResolvedSelector).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 37cc059f-7b94-4c38-aa7c-6386f11ef1ce
📒 Files selected for processing (1)
crates/biome_css_semantic/src/tests/specificity.rs
Comment on lines
+345
to
+347
| let class_rule = model | ||
| .get_rule_by_id(class_rule_id) | ||
| .expect("Expected to retrieve 'span.class' rule"); |
Contributor
There was a problem hiding this comment.
Minor comment inconsistency.
The comment says "Expected to retrieve 'span.class' rule" but the selector is .class, not span.class.
Proposed fix
let class_rule = model
.get_rule_by_id(class_rule_id)
- .expect("Expected to retrieve 'span.class' rule");
+ .expect("Expected to retrieve '.class' rule");📝 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.
Suggested change
| let class_rule = model | |
| .get_rule_by_id(class_rule_id) | |
| .expect("Expected to retrieve 'span.class' rule"); | |
| let class_rule = model | |
| .get_rule_by_id(class_rule_id) | |
| .expect("Expected to retrieve '.class' rule"); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/biome_css_semantic/src/tests/specificity.rs` around lines 345 - 347,
Update the panic message on the get_rule_by_id call to match the actual selector
being tested: change the expect string for the retrieval of class_rule (the call
to model.get_rule_by_id(class_rule_id).expect(...)) to reference ".class"
instead of "span.class" so the message accurately reflects the selector under
test.
Netail
reviewed
Mar 23, 2026
dyc3
approved these changes
Mar 23, 2026
Comment on lines
+31
to
+33
| /* 7. Type selector case-insensitivity: DIV == div */ | ||
| DIV { color: red; } | ||
| div { color: blue; } |
Contributor
There was a problem hiding this comment.
wouldn't this conflict with the selectors from case 2?
Member
Author
There was a problem hiding this comment.
Yes, in fact the diagnostic points to line 8
Co-authored-by: Carson McManus <[email protected]>
Co-authored-by: Maikel van Dort <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR superseeds #4658
This PR closes #2514
This PR closes #2511 (the main stylelint tracking umbrella)
The implementation of the rule has been done with AI, as well as the rest of the semantic model.
There's a cost to this rule, and what the semantic model does, we can't avoid it. In order to flatten the selectors, we need to allocate strings.
Test Plan
Added new tests
Docs