feat(assist): implement useSortedAttributes for HTML#9547
feat(assist): implement useSortedAttributes for HTML#9547mujpao wants to merge 2 commits intobiomejs:nextfrom
Conversation
🦋 Changeset detectedLatest commit: 6cce591 The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 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 |
github-actions
bot
added
A-Linter
WalkthroughPorts the existing JSX 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can customize the high-level summary generated by CodeRabbit.Configure the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/biome_html_analyze/src/assist/source/use_sorted_attributes.rs (1)
102-105: Optional: Consider extracting comparator selection into a helper.The comparator creation is duplicated between
run()andaction(). A small helper function could reduce this duplication.♻️ Suggested helper function
fn get_comparator( sort_order: SortOrder, ) -> fn(&SortableHtmlAttribute, &SortableHtmlAttribute) -> Ordering { match sort_order { SortOrder::Natural => SortableHtmlAttribute::ascii_nat_cmp, SortOrder::Lexicographic => SortableHtmlAttribute::lexicographic_cmp, } }Then use
get_comparator(options.sort_order.unwrap_or_default())in both places.Also applies to: 161-164
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_html_analyze/src/assist/source/use_sorted_attributes.rs` around lines 102 - 105, Extract the duplicated comparator selection into a small helper function (e.g., get_comparator) that takes SortOrder and returns the comparator fn(&SortableHtmlAttribute, &SortableHtmlAttribute) -> Ordering; replace the match blocks in both run() and action() with calls to get_comparator(options.sort_order.unwrap_or_default()) (or the appropriate sort_order variable) so both places use the same helper and remove duplicated match logic over SortOrder::Natural / SortOrder::Lexicographic mapping to SortableHtmlAttribute::ascii_nat_cmp and ::lexicographic_cmp.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/biome_html_analyze/src/assist/source/use_sorted_attributes.rs`:
- Around line 102-105: Extract the duplicated comparator selection into a small
helper function (e.g., get_comparator) that takes SortOrder and returns the
comparator fn(&SortableHtmlAttribute, &SortableHtmlAttribute) -> Ordering;
replace the match blocks in both run() and action() with calls to
get_comparator(options.sort_order.unwrap_or_default()) (or the appropriate
sort_order variable) so both places use the same helper and remove duplicated
match logic over SortOrder::Natural / SortOrder::Lexicographic mapping to
SortableHtmlAttribute::ascii_nat_cmp and ::lexicographic_cmp.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aa4467ce-c90c-4f70-952c-330f8399b890
⛔ Files ignored due to path filters (13)
Cargo.lockis excluded by!**/*.lockand included by**crates/biome_html_analyze/tests/specs/source/useSortedAttributes/astro/sorted.astro.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/source/useSortedAttributes/astro/unsorted.astro.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/source/useSortedAttributes/html/sorted-lexicographic.html.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/source/useSortedAttributes/html/sorted.html.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/source/useSortedAttributes/html/unsorted-lexicographic.html.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/source/useSortedAttributes/html/unsorted.html.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/source/useSortedAttributes/svelte/sorted.svelte.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/source/useSortedAttributes/svelte/unsorted.svelte.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/source/useSortedAttributes/vue/sorted.vue.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/source/useSortedAttributes/vue/unsorted.vue.snapis excluded by!**/*.snapand 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 (18)
.changeset/eleven-baths-wave.mdcrates/biome_analyze/Cargo.tomlcrates/biome_analyze/src/shared/mod.rscrates/biome_analyze/src/shared/sort_attributes.rscrates/biome_html_analyze/src/assist/source/use_sorted_attributes.rscrates/biome_html_analyze/tests/specs/source/useSortedAttributes/astro/sorted.astrocrates/biome_html_analyze/tests/specs/source/useSortedAttributes/astro/unsorted.astrocrates/biome_html_analyze/tests/specs/source/useSortedAttributes/html/sorted-lexicographic.htmlcrates/biome_html_analyze/tests/specs/source/useSortedAttributes/html/sorted-lexicographic.options.jsoncrates/biome_html_analyze/tests/specs/source/useSortedAttributes/html/sorted.htmlcrates/biome_html_analyze/tests/specs/source/useSortedAttributes/html/unsorted-lexicographic.htmlcrates/biome_html_analyze/tests/specs/source/useSortedAttributes/html/unsorted-lexicographic.options.jsoncrates/biome_html_analyze/tests/specs/source/useSortedAttributes/html/unsorted.htmlcrates/biome_html_analyze/tests/specs/source/useSortedAttributes/svelte/sorted.sveltecrates/biome_html_analyze/tests/specs/source/useSortedAttributes/svelte/unsorted.sveltecrates/biome_html_analyze/tests/specs/source/useSortedAttributes/vue/sorted.vuecrates/biome_html_analyze/tests/specs/source/useSortedAttributes/vue/unsorted.vuecrates/biome_js_analyze/src/assist/source/use_sorted_attributes.rs
Merging this PR will not alter performance
Comparing 
Footnotes
|
| i Safe fix: Sort the HTML attributes. | ||
|
|
||
| 1 │ - <p·v-text="msg"·v-show="ok"·dir="auto"·v-foo:bar.baz·id="hello"·class="flex"></p> | ||
| 1 │ + <p·v-text="msg"·v-show="ok"·dir="auto"·v-foo:bar.baz·class="flex"id="hello"·></p> | ||
| 2 2 │ <div v-slot:default v-on:click="doThis" v-once v-bind="{ id: someProp, 'other-attr': otherProp } " | ||
| 3 3 │ v-bind:src="'/path/to/images/' + fileName" |
There was a problem hiding this comment.
we should discuss the order of vue directives. I personally prefer vue directives to generally be after all normal attributes, but there might be some prior art out there.
| 9 9 │ onswiperight={prev} | ||
| 10 10 │ {@attach myAttachment} | ||
| 11 │ - ····spellcheck="true" | ||
| 12 │ - ····tabindex="-1" | ||
| 13 │ - ····dir="auto" | ||
| 11 │ + ····dir="auto" | ||
| 12 │ + ····spellcheck="true" | ||
| 13 │ + ····tabindex="-1" | ||
| 14 14 │ animate:flip | ||
| 15 15 │ style:color="red" |
There was a problem hiding this comment.
same here, there might be prior art or popular conventions to consider here for svelte directives
There was a problem hiding this comment.
Hi! For prior art on Svelte directive ordering, the official eslint-plugin-svelte has a well-established sort-attributes rule with a default order that groups Svelte-specific directives by type.
It might make sense to align with this convention since it's what the Svelte ecosystem already uses.
There was a problem hiding this comment.
What if the alphabetical sorting of HTML attributes conflicts with eslint-plugin-svelte's ordering? For instance, eslint-plugin-svelte's sort order puts id before class.
There was a problem hiding this comment.
For now I would say let's ignore conflicting logic for base html attributes, and just focus on svelte specific directives
| client:load | ||
| class:list={classes} | ||
| set:text={text} | ||
| is:raw | ||
| dir="auto" | ||
| spellcheck="true" | ||
| tabindex="-1" | ||
| define:vars={vars} | ||
| server:defer | ||
| id="myid" |
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_html_analyze/src/assist/source/use_sorted_attributes.rs`:
- Around line 145-151: text_range currently ignores the provided _state and
returns the whole element range, causing duplicate diagnostics; update
text_range(ctx: &RuleContext<Self>, state: &Self::State) to compute and return a
narrower TextRange covering only the current attribute group (use state.attrs:
get the first and last attribute nodes from state.attrs, compute their combined
span from first.range().start() to last.range().end(), and return that
TextRange) instead of the HtmlOpeningElement/HtmlSelfClosingElement
element.range(); this ensures the diagnostic is tied to the specific group
rather than the entire element.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d5a4de83-7c63-4acb-bc99-3a8bba45b1be
📒 Files selected for processing (1)
crates/biome_html_analyze/src/assist/source/use_sorted_attributes.rs
| fn text_range(ctx: &RuleContext<Self>, _state: &Self::State) -> Option<TextRange> { | ||
| ctx.query().syntax().ancestors().skip(1).find_map(|node| { | ||
| HtmlOpeningElement::cast_ref(&node) | ||
| .map(|element| element.range()) | ||
| .or_else(|| HtmlSelfClosingElement::cast_ref(&node).map(|element| element.range())) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Use the state-specific range to avoid duplicate diagnostics.
text_range currently ignores state, so two unsorted groups in the same element can emit duplicate diagnostics on the same element-wide span. Please narrow the range to the current group (for example, from first to last attribute in state.attrs).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/biome_html_analyze/src/assist/source/use_sorted_attributes.rs` around
lines 145 - 151, text_range currently ignores the provided _state and returns
the whole element range, causing duplicate diagnostics; update text_range(ctx:
&RuleContext<Self>, state: &Self::State) to compute and return a narrower
TextRange covering only the current attribute group (use state.attrs: get the
first and last attribute nodes from state.attrs, compute their combined span
from first.range().start() to last.range().end(), and return that TextRange)
instead of the HtmlOpeningElement/HtmlSelfClosingElement element.range(); this
ensures the diagnostic is tied to the specific group rather than the entire
element.
3 participants
Summary
Adds assist rule
useSortedAttributesfor HTML. Also refactors some of the code from the JSX version ofuseSortedAttributesto reduce code duplication.Closes #9334
Test Plan
Snapshots tests in
crates/biome_html_analyze/tests/specs/source/useSortedAttributesDocs
There is documentation in
crates/biome_html_analyze/src/assist/source/use_sorted_attributes.rs