feat(lint): port useValidAnchor to html#8987
feat(lint): port useValidAnchor to html#8987DerTimonius wants to merge 13 commits intobiomejs:nextfrom
Conversation
🦋 Changeset detectedLatest commit: ef28996 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
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new HTML accessibility lint rule UseValidAnchor that inspects elements and reports three states: MissingHrefAttribute (no Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@crates/biome_html_analyze/src/lint/a11y/use_valid_anchor.rs`:
- Around line 84-91: The doc comment for the UseValidAnchorState::CantBeAnchor
enum variant is incorrect — it currently says "The element has `href` and
`onClick`" but the variant is emitted when the element has no `href` and only an
`onclick` handler; update the comment on CantBeAnchor to accurately state that
it represents an element with no href and an onclick (or interactive-only)
handler so the semantics match its use in the validation logic referencing
UseValidAnchorState::CantBeAnchor.
- Around line 120-123: In the match arm for Self::IncorrectHref(_) in
use_valid_anchor.rs, fix the typo in the message string by removing the extra
article so it reads "The href attribute should be a valid URL" instead of "The
href attribute should be a valid a URL"; update the string literal produced by
the markup! invocation (the Self::IncorrectHref(_) arm) accordingly so the
returned ownership (.to_owned()) contains the corrected text.
- Around line 10-78: Fix two small typos in the UseValidAnchor rule docs: in the
doc comment for the declare_lint_rule! block (the UseValidAnchor rule) replace
the double space in the phrase "it's now easier" with a single space and change
"it can source of invalid links" to "it can be a source of invalid links" so the
documentation reads correctly; update the comment text inside the
declare_lint_rule! macro where the rule documentation is written.
- Around line 160-167: Replace the current static-value check that uses
anchor_attribute.value() and is_none_or(...) with a call to as_static_value()?
so dynamic hrefs are not treated as errors (i.e., if let Some(const_str) =
anchor_attribute.as_static_value()? { ... } ), and when returning the error
produce UseValidAnchorState::IncorrectHref(anchor_attribute.range()) only for
truly invalid static values; also change the javascript protocol check from
contains("javascript:") to a prefix check after trimming (e.g.,
trimmed.starts_with("javascript:")) so queries containing "javascript:" do not
falsely match.
In `@crates/biome_html_analyze/tests/specs/a11y/useValidAnchor/invalid.svelte`:
- Around line 1-4: Add a fixture that includes a bare anchor element to trigger
the MissingHrefAttribute variant: insert a plain "<a>no attributes</a>" into one
of the invalid test files (e.g.,
crates/biome_html_analyze/tests/specs/a11y/useValidAnchor/invalid.svelte) so the
rule's (None, None) branch (MissingHrefAttribute at line 174) is exercised; also
update the test expectations/diagnostics for that fixture to assert the
MissingHrefAttribute diagnostic is produced.
🧹 Nitpick comments (1)
.changeset/cold-pianos-try.md (1)
1-5: Changeset could be more informative.Per project conventions, changesets for new lint rules should include:
- A link to the rule's documentation page.
- An example of an invalid case (inline code or code block).
Something along these lines would do:
Ported the [useValidAnchor](https://biomejs.dev/linter/rules/use-valid-anchor/) rule to HTML. Example of invalid code: `<a href="javascript:void(0)">navigate here</a>` It is not currently possible to detect whether a given variable is `null` or `undefined`, so those cases are not covered.Based on learnings: "For new lint rules in changesets, show an example of invalid case in inline code or code block" and "In changesets, reference rules and assists with links to the website documentation".
crates/biome_html_analyze/tests/specs/a11y/useValidAnchor/invalid.svelte
Show resolved
Hide resolved
Merging this PR will degrade performance by 7.46%
Performance Changes
Comparing 
Footnotes
|
.changeset/cold-pianos-try.md
Outdated
| "@biomejs/biome": minor | ||
| --- | ||
|
|
||
| This PR ports the `useValidAnchor` rule to HTML. As it's not possible at the moment to detect, whether or not a given variable is `null` or `undefined`, those cases are not covered. |
There was a problem hiding this comment.
As it's not possible at the moment to detect...
This is not worth calling out in the changeset, imo.
.changeset/cold-pianos-try.md
Outdated
| "@biomejs/biome": minor | ||
| --- | ||
|
|
||
| This PR ports the `useValidAnchor` rule to HTML. As it's not possible at the moment to detect, whether or not a given variable is `null` or `undefined`, those cases are not covered. |
There was a problem hiding this comment.
- needs to link to the rule docs.
- needs to describe the change as it should appear in the changelog
take a look at .changeset/html-no-positive-tabindex.md to see a good example
| fn message(&self) -> MarkupBuf { | ||
| match self { | ||
| Self::MissingHrefAttribute(_) => { | ||
| (markup! { |
There was a problem hiding this comment.
nit: would prefer for these to be in the fn diagnostic()
ematipico
left a comment
ematipico
left a comment
There was a problem hiding this comment.
The rule has some bugs, which I highlighted in some comments. We should add a .vue file too in the tests
| /// :::note | ||
| /// At the moment, it is not possible to check whether or not a specified variable in the `href` attribute resolves | ||
| /// to `null` or `undefined`. | ||
| /// ::: |
There was a problem hiding this comment.
I don't think Biome will ever be able to do, so maybe it's not worth mentioning it, and just assume that rule works without that
| let node = ctx.query(); | ||
| let name = node.name().ok()?.token_text_trimmed(); | ||
|
|
||
| if name.is_some_and(|n| n == "a") { |
There was a problem hiding this comment.
Bug: the name of a tag is case-insensitive in .html but case-sensitive in files like .vue, .svelte and .astro.
Here's an example of implementation
biome/crates/biome_html_analyze/src/lint/a11y/no_redundant_alt.rs
Lines 59 to 64 in 0a147f1
| if static_value.is_none_or(|const_str| { | ||
| const_str.is_empty() | ||
| || const_str == "#" | ||
| || const_str.contains("javascript:") |
There was a problem hiding this comment.
This is incorrect. It should use starts_with()
| @@ -0,0 +1,4 @@ | |||
| <!-- should not generate diagnostics --> | |||
| <a href="#id">ok</a> | |||
There was a problem hiding this comment.
This test should contain components too
| @@ -0,0 +1,4 @@ | |||
| <!-- should generate diagnostics --> | |||
| <a href="javascript:void(0)">invalid</a> | |||
There was a problem hiding this comment.
This test should contain components too
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.changeset/cold-pianos-try.md:
- Line 5: Fix the subject-verb agreement in the sentence describing the rule:
change "This rule enforce that all anchors are valid and that they are navigable
elements." to "This rule enforces that all anchors are valid and that they are
navigable elements." — update the line in .changeset/cold-pianos-try.md where
that sentence appears.
🧹 Nitpick comments (1)
.changeset/cold-pianos-try.md (1)
5-5: Consider adding an example of invalid code.The description would benefit from showing what specific patterns are now flagged (e.g., anchors with missing
href, emptyhref, oronclickwithouthref). Based on learnings, new lint rules in changesets should demonstrate invalid cases.💡 Suggested enhancement
-Ports the [`useValidAnchor`](https://biomejs.dev/linter/rules/use-valid-anchor/) rule to HTML. This rule enforces that all anchors are valid and that they are navigable elements. +Ports the [`useValidAnchor`](https://biomejs.dev/linter/rules/use-valid-anchor/) rule to HTML. This rule now flags invalid anchors such as `<a>` without `href`, `<a href="#">`, or `<a onclick="...">` without a valid `href`.Based on learnings: For new lint rules in changesets, show an example of invalid case in inline code or code block.
|
hey @dyc3 & @ematipico @dyc3 regarding your nits: as I have tried to stick to the original JS rule, should I apply the same changes there as well? |
.changeset/cold-pianos-try.md
Outdated
| "@biomejs/biome": minor | ||
| --- | ||
|
|
||
| Ports the [`useValidAnchor`](https://biomejs.dev/linter/rules/use-valid-anchor/) rule to HTML. This rule enforce that all anchors are valid and that they are navigable elements. |
There was a problem hiding this comment.
| Ports the [`useValidAnchor`](https://biomejs.dev/linter/rules/use-valid-anchor/) rule to HTML. This rule enforce that all anchors are valid and that they are navigable elements. | |
| Ports the [`useValidAnchor`](https://biomejs.dev/linter/rules/use-valid-anchor/) rule to HTML. This rule enforces that all anchors are valid and that they are navigable elements. |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
ematipico
left a comment
ematipico
left a comment
There was a problem hiding this comment.
Looks good. The bot left some good comments to address
There was a problem hiding this comment.
♻️ Duplicate comments (5)
crates/biome_html_analyze/src/lint/a11y/use_valid_anchor.rs (5)
79-86:UseValidAnchorStateenum sits betweendeclare_lint_rule!andimpl Rule— dyc3 already asked to move it.The
impl Ruleblock should immediately followdeclare_lint_rule!. Move both the enum declaration and itsimplblock below the closing}ofimpl Rule.Based on learnings: "In crates/biome_analyze/**/*.rs rule files, all helper functions, structs, and enums must be placed below the
impl Ruleblock."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_html_analyze/src/lint/a11y/use_valid_anchor.rs` around lines 79 - 86, The UseValidAnchorState enum (and its associated impl block) is declared between declare_lint_rule! and the impl Rule block; move the entire enum declaration and its impl for UseValidAnchorState so they appear after the closing brace of impl Rule (i.e., relocate UseValidAnchorState and its impl below the impl Rule block) to comply with the project convention that helper types/functions live after impl Rule.
112-119:⚠️ Potential issue | 🟠 Major
is_none_orstill causes false positives on dynamichrefvalues — this was flagged and still not fixed.
value()returnsNonefor expressions likehref={somewhere}.is_none_ortreatsNoneas matching the predicate, so valid dynamic hrefs are incorrectly reported asIncorrectHref. Replace with an explicitif let Some(...)guard soNone(dynamic) is simply skipped.🐛 Proposed fix
- let static_value = anchor_attribute.value(); - if static_value.is_none_or(|const_str| { - const_str.is_empty() - || const_str == "#" - || const_str.starts_with("javascript:") - }) { - return Some(UseValidAnchorState::IncorrectHref(anchor_attribute.range())); - } + if let Some(const_str) = anchor_attribute.value() { + if const_str.is_empty() + || const_str == "#" + || const_str.starts_with("javascript:") + { + return Some(UseValidAnchorState::IncorrectHref(anchor_attribute.range())); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_html_analyze/src/lint/a11y/use_valid_anchor.rs` around lines 112 - 119, The current check uses anchor_attribute.value().is_none_or(...) which treats None (dynamic hrefs like href={somewhere}) as failing and yields false positives; change the logic in use_valid_anchor.rs by replacing the is_none_or call with an explicit if let Some(static_value) = anchor_attribute.value() { ... } guard and only run the empty/"#"/"javascript:" checks inside that block so None values are skipped and not reported by UseValidAnchorState::IncorrectHref(anchor_attribute.range()).
17-30:⚠️ Potential issue | 🟡 MinorUnresolved doc typos from the previous review round.
Two issues still present:
- Line 17: double space —
"it's now easier"→"it's now easier"- Line 30: missing word —
"it can source of invalid links"→"it can be a source of invalid links"✏️ Proposed fix
- /// it's now easier to attach logic to any HTML element, anchors included. + /// it's now easier to attach logic to any HTML element, anchors included.- /// - it can source of invalid links, and crawlers can't navigate the website, risking to penalize + /// - it can be a source of invalid links, and crawlers can't navigate the website, risking to penalize🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_html_analyze/src/lint/a11y/use_valid_anchor.rs` around lines 17 - 30, Fix the two typos in the documentation comment at the top of the use_valid_anchor lint: change "it's now easier" to "it's now easier" and change "it can source of invalid links" to "it can be a source of invalid links" in the module-level docstring inside crates/biome_html_analyze/src/lint/a11y/use_valid_anchor.rs (the doc comment above the rule implementation for use_valid_anchor).
163-166:⚠️ Potential issue | 🟡 MinorTypo
"valid a URL"— still unresolved.The extra article has been flagged before. Quick fix:
✏️ Proposed fix
- Self::State::IncorrectHref(_) => (markup! { - "The href attribute should be a valid a URL" - }) + Self::State::IncorrectHref(_) => (markup! { + "The href attribute should be a valid URL" + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_html_analyze/src/lint/a11y/use_valid_anchor.rs` around lines 163 - 166, Fix the typo in the message produced for the Self::State::IncorrectHref(_) branch: replace the string "The href attribute should be a valid a URL" with a corrected message such as "The href attribute should be a valid URL" inside the Self::State::IncorrectHref(_) match arm (the markup! { ... }.to_owned() expression).
34-34:⚠️ Potential issue | 🟡 Minor
<https://...>autolink syntax breaks the Biome website codegen.dyc3 already flagged this. The raw
<URL>rustdoc autolink form isn't accepted by the website generator — use a Markdown link instead, e.g.[text](url).✏️ Proposed fix
- /// For a detailed explanation, check out <https://marcysutton.com/links-vs-buttons-in-modern-web-applications> + /// For a detailed explanation, check out [this article](https://marcysutton.com/links-vs-buttons-in-modern-web-applications)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_html_analyze/src/lint/a11y/use_valid_anchor.rs` at line 34, The rustdoc autolink using raw angle-bracket form "<https://marcysutton.com/links-vs-buttons-in-modern-web-applications>" in crates/biome_html_analyze/src/lint/a11y/use_valid_anchor.rs breaks the site codegen; replace it with a Markdown link like [Links vs Buttons](https://marcysutton.com/links-vs-buttons-in-modern-web-applications) inside the same doc comment so the generator accepts it (edit the doc comment in use_valid_anchor.rs where that URL appears).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/biome_html_analyze/src/lint/a11y/use_valid_anchor.rs`:
- Around line 79-86: The UseValidAnchorState enum (and its associated impl
block) is declared between declare_lint_rule! and the impl Rule block; move the
entire enum declaration and its impl for UseValidAnchorState so they appear
after the closing brace of impl Rule (i.e., relocate UseValidAnchorState and its
impl below the impl Rule block) to comply with the project convention that
helper types/functions live after impl Rule.
- Around line 112-119: The current check uses
anchor_attribute.value().is_none_or(...) which treats None (dynamic hrefs like
href={somewhere}) as failing and yields false positives; change the logic in
use_valid_anchor.rs by replacing the is_none_or call with an explicit if let
Some(static_value) = anchor_attribute.value() { ... } guard and only run the
empty/"#"/"javascript:" checks inside that block so None values are skipped and
not reported by UseValidAnchorState::IncorrectHref(anchor_attribute.range()).
- Around line 17-30: Fix the two typos in the documentation comment at the top
of the use_valid_anchor lint: change "it's now easier" to "it's now easier" and
change "it can source of invalid links" to "it can be a source of invalid links"
in the module-level docstring inside
crates/biome_html_analyze/src/lint/a11y/use_valid_anchor.rs (the doc comment
above the rule implementation for use_valid_anchor).
- Around line 163-166: Fix the typo in the message produced for the
Self::State::IncorrectHref(_) branch: replace the string "The href attribute
should be a valid a URL" with a corrected message such as "The href attribute
should be a valid URL" inside the Self::State::IncorrectHref(_) match arm (the
markup! { ... }.to_owned() expression).
- Line 34: The rustdoc autolink using raw angle-bracket form
"<https://marcysutton.com/links-vs-buttons-in-modern-web-applications>" in
crates/biome_html_analyze/src/lint/a11y/use_valid_anchor.rs breaks the site
codegen; replace it with a Markdown link like [Links vs
Buttons](https://marcysutton.com/links-vs-buttons-in-modern-web-applications)
inside the same doc comment so the generator accepts it (edit the doc comment in
use_valid_anchor.rs where that URL appears).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
crates/biome_html_analyze/src/lint/a11y/use_valid_anchor.rs (2)
76-86: MoveUseValidAnchorStatebelow theimpl Ruleblock.
Keeping helpers after the rule implementation matches the repo’s rule-file layout.Based on learnings: “In crates/biome_analyze/**/*.rs rule files, all helper functions, structs, and enums must be placed below the
impl Ruleblock.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_html_analyze/src/lint/a11y/use_valid_anchor.rs` around lines 76 - 86, Move the helper enum UseValidAnchorState so it appears after the impl Rule block in this file: locate the enum declaration UseValidAnchorState and cut/paste it below the impl Rule { ... } implementation for the rule (keeping its exact name and variants MissingHrefAttribute, IncorrectHref, CantBeAnchor and their TextRange payloads unchanged); ensure any references to UseValidAnchorState within the impl Rule still compile (adjust visibility if needed) and run cargo build/tests to verify no missing imports or visibility issues.
33-35: Swap the rustdoc autolink for a Markdown link.
Line 34 uses the<https://…>form, which the docs codegen rejects. Please use a standard Markdown link instead.✏️ Suggested change
- /// For a detailed explanation, check out <https://marcysutton.com/links-vs-buttons-in-modern-web-applications> + /// For a detailed explanation, check out [this article](https://marcysutton.com/links-vs-buttons-in-modern-web-applications)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_html_analyze/src/lint/a11y/use_valid_anchor.rs` around lines 33 - 35, The rustdoc comment in use_valid_anchor.rs contains an autolink using the <https://...> form which is rejected by docs codegen; update that doc comment to use a standard Markdown link (e.g., [Links vs Buttons](https://marcysutton.com/links-vs-buttons-in-modern-web-applications)) instead. Locate the documentation block in the use_valid_anchor lint (look for the comment above the use_valid_anchor lint implementation or module) and replace the angle-bracket autolink with a Markdown-style link so the docs generator will accept it.
🤖 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/lint/a11y/use_valid_anchor.rs`:
- Around line 117-123: The check for "javascript:" in the anchor href is
case-sensitive and allows mixed-case schemes; update the conditional that
inspects static_value (the trimmed attribute_value) to perform an ASCII
case-insensitive comparison (e.g., call
static_value.to_ascii_lowercase().starts_with("javascript:") or compare a
lowercase slice) so UseValidAnchorState::IncorrectHref(anchor_attribute.range())
is returned for any ASCII-cased "javascript:" scheme.
---
Duplicate comments:
In `@crates/biome_html_analyze/src/lint/a11y/use_valid_anchor.rs`:
- Around line 76-86: Move the helper enum UseValidAnchorState so it appears
after the impl Rule block in this file: locate the enum declaration
UseValidAnchorState and cut/paste it below the impl Rule { ... } implementation
for the rule (keeping its exact name and variants MissingHrefAttribute,
IncorrectHref, CantBeAnchor and their TextRange payloads unchanged); ensure any
references to UseValidAnchorState within the impl Rule still compile (adjust
visibility if needed) and run cargo build/tests to verify no missing imports or
visibility issues.
- Around line 33-35: The rustdoc comment in use_valid_anchor.rs contains an
autolink using the <https://...> form which is rejected by docs codegen; update
that doc comment to use a standard Markdown link (e.g., [Links vs
Buttons](https://marcysutton.com/links-vs-buttons-in-modern-web-applications))
instead. Locate the documentation block in the use_valid_anchor lint (look for
the comment above the use_valid_anchor lint implementation or module) and
replace the angle-bracket autolink with a Markdown-style link so the docs
generator will accept it.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
crates/biome_html_analyze/tests/specs/a11y/useValidAnchor/invalid.astro.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/a11y/useValidAnchor/invalid.html.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/a11y/useValidAnchor/invalid.svelte.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/a11y/useValidAnchor/invalid.vue.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (6)
.changeset/cold-pianos-try.mdcrates/biome_html_analyze/src/lint/a11y/use_valid_anchor.rscrates/biome_html_analyze/tests/specs/a11y/useValidAnchor/invalid.astrocrates/biome_html_analyze/tests/specs/a11y/useValidAnchor/invalid.htmlcrates/biome_html_analyze/tests/specs/a11y/useValidAnchor/invalid.sveltecrates/biome_html_analyze/tests/specs/a11y/useValidAnchor/invalid.vue
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/biome_html_analyze/tests/specs/a11y/useValidAnchor/invalid.html
- .changeset/cold-pianos-try.md
| let static_value = attribute_value.trim(); | ||
|
|
||
| if static_value.is_empty() | ||
| || static_value == "#" | ||
| || static_value.starts_with("javascript:") | ||
| { | ||
| return Some(UseValidAnchorState::IncorrectHref(anchor_attribute.range())); |
There was a problem hiding this comment.
Make the javascript: check ASCII case‑insensitive.
Right now JavaScript: or mixed‑case schemes slip through.
🔧 Suggested fix
- if static_value.is_empty()
- || static_value == "#"
- || static_value.starts_with("javascript:")
+ let is_javascript = static_value
+ .as_bytes()
+ .get(.. "javascript:".len())
+ .map(|prefix| prefix.eq_ignore_ascii_case(b"javascript:"))
+ .unwrap_or(false);
+
+ if static_value.is_empty()
+ || static_value == "#"
+ || is_javascript
{
return Some(UseValidAnchorState::IncorrectHref(anchor_attribute.range()));
}📝 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.
| let static_value = attribute_value.trim(); | |
| if static_value.is_empty() | |
| || static_value == "#" | |
| || static_value.starts_with("javascript:") | |
| { | |
| return Some(UseValidAnchorState::IncorrectHref(anchor_attribute.range())); | |
| let static_value = attribute_value.trim(); | |
| let is_javascript = static_value | |
| .as_bytes() | |
| .get(.. "javascript:".len()) | |
| .map(|prefix| prefix.eq_ignore_ascii_case(b"javascript:")) | |
| .unwrap_or(false); | |
| if static_value.is_empty() | |
| || static_value == "#" | |
| || is_javascript | |
| { | |
| return Some(UseValidAnchorState::IncorrectHref(anchor_attribute.range())); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/biome_html_analyze/src/lint/a11y/use_valid_anchor.rs` around lines 117
- 123, The check for "javascript:" in the anchor href is case-sensitive and
allows mixed-case schemes; update the conditional that inspects static_value
(the trimmed attribute_value) to perform an ASCII case-insensitive comparison
(e.g., call static_value.to_ascii_lowercase().starts_with("javascript:") or
compare a lowercase slice) so
UseValidAnchorState::IncorrectHref(anchor_attribute.range()) is returned for any
ASCII-cased "javascript:" scheme.
| /// <a href="javascript:void(0)">navigate here</a> | ||
| /// ``` | ||
| /// ```html,expect_diagnostic | ||
| /// <a onclick={something}>navigate here</a> |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
crates/biome_html_analyze/src/lint/a11y/use_valid_anchor.rs (2)
104-108:⚠️ Potential issue | 🟡 Minor
javascript:scheme check is still case-sensitive.
Javascript:void(0)orJAVASCRIPT:void(0)would not be caught. URL schemes are case-insensitive per RFC 3986 §3.1.🔧 Proposed fix
- || static_value.starts_with("javascript:") + || static_value + .get(.. "javascript:".len()) + .is_some_and(|prefix| prefix.eq_ignore_ascii_case("javascript:"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_html_analyze/src/lint/a11y/use_valid_anchor.rs` around lines 104 - 108, The current check in use_valid_anchor (around the static_value handling where it returns UseValidAnchorState::IncorrectHref(anchor_attribute.range())) treats "javascript:" case-sensitively; change the check to perform a case-insensitive scheme test (e.g., compare static_value.to_ascii_lowercase().starts_with("javascript:") or otherwise compare the scheme lowercased) so values like "Javascript:void(0)" or "JAVASCRIPT:void(0)" are caught; keep the other checks for empty and "#" as-is and still return UseValidAnchorState::IncorrectHref(anchor_attribute.range()) when matched.
47-47:⚠️ Potential issue | 🟡 MinorInvalid HTML in doc example still present.
onclick={something}is JSX syntax — it's not valid HTML. A plain-HTML doctest should use something likeonclick="someFunction()".🔧 Proposed fix
-/// <a onclick={something}>navigate here</a> +/// <a onclick="someFunction()">navigate here</a>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_html_analyze/src/lint/a11y/use_valid_anchor.rs` at line 47, The doc-example in use_valid_anchor.rs contains invalid JSX-style HTML (`onclick={something}`); replace that snippet in the doc comment with valid plain HTML (e.g. change `<a onclick={something}>navigate here</a>` to something like `<a onclick="someFunction()">navigate here</a>`) so the doctest is valid HTML and compiles as a plain-HTML example in the UseValidAnchor lint's documentation.
🧹 Nitpick comments (1)
crates/biome_html_analyze/src/lint/a11y/use_valid_anchor.rs (1)
185-192:range()returns&TextRange, butTextRangeisCopy.Returning a reference to a
Copytype is slightly un-idiomatic. Callers get the same value either way (auto-deref), but returning by value is cleaner.♻️ Proposed refactor
- fn range(&self) -> &TextRange { + fn range(&self) -> TextRange { match self { Self::MissingHrefAttribute(range) | Self::CantBeAnchor(range) - | Self::IncorrectHref(range) => range, + | Self::IncorrectHref(range) => *range, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_html_analyze/src/lint/a11y/use_valid_anchor.rs` around lines 185 - 192, The method UseValidAnchorState::range currently returns &TextRange even though TextRange is Copy; change the signature to return TextRange (by value) and update the match arms to return the copied range (not references) so callers receive TextRange directly; modify fn range(&self) -> TextRange and have the match return range for each variant (Self::MissingHrefAttribute(range) | Self::CantBeAnchor(range) | Self::IncorrectHref(range) => *range or simply range if binding by value) to remove the unnecessary reference and adhere to idiomatic use of Copy types.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/biome_html_analyze/src/lint/a11y/use_valid_anchor.rs`:
- Around line 104-108: The current check in use_valid_anchor (around the
static_value handling where it returns
UseValidAnchorState::IncorrectHref(anchor_attribute.range())) treats
"javascript:" case-sensitively; change the check to perform a case-insensitive
scheme test (e.g., compare
static_value.to_ascii_lowercase().starts_with("javascript:") or otherwise
compare the scheme lowercased) so values like "Javascript:void(0)" or
"JAVASCRIPT:void(0)" are caught; keep the other checks for empty and "#" as-is
and still return UseValidAnchorState::IncorrectHref(anchor_attribute.range())
when matched.
- Line 47: The doc-example in use_valid_anchor.rs contains invalid JSX-style
HTML (`onclick={something}`); replace that snippet in the doc comment with valid
plain HTML (e.g. change `<a onclick={something}>navigate here</a>` to something
like `<a onclick="someFunction()">navigate here</a>`) so the doctest is valid
HTML and compiles as a plain-HTML example in the UseValidAnchor lint's
documentation.
---
Nitpick comments:
In `@crates/biome_html_analyze/src/lint/a11y/use_valid_anchor.rs`:
- Around line 185-192: The method UseValidAnchorState::range currently returns
&TextRange even though TextRange is Copy; change the signature to return
TextRange (by value) and update the match arms to return the copied range (not
references) so callers receive TextRange directly; modify fn range(&self) ->
TextRange and have the match return range for each variant
(Self::MissingHrefAttribute(range) | Self::CantBeAnchor(range) |
Self::IncorrectHref(range) => *range or simply range if binding by value) to
remove the unnecessary reference and adhere to idiomatic use of Copy types.
DerTimonius
force-pushed
the
feat/html-use-valid-anchor
branch
from
c9fd6c3 to
ef28996
Compare
|
@DerTimonius there's still this #8987 (comment) |
3 participants
Summary
This PR ports the
useValidAnchora11y rule to HTML as part of #8155. It was mostly a 1:1 port of the original rule with small tweaks.As noted in the original issue, we are unable to detect whether or not the given JS variables are
nullorundefined, so I have not added checks for those cases.I have not used any AI assistance for this PR.
Test Plan
.html,.astroand.sveltefilesDocs
Doc comments have been added, with a note regarding the aforementioned issue with
nullandundefined