fix(lint): detect array index in any position of template string#8947
fix(lint): detect array index in any position of template string#8947murataslan1 wants to merge 2 commits intobiomejs:mainfrom
Conversation
Fixes biomejs#8812 The `noArrayIndexKey` rule was not detecting array index usage when the index appeared before other expressions in a template string. Before: - `${item}-${index}` - correctly flagged - `${index}-${item}` - NOT flagged (false negative) The issue was that the loop overwrote `capture_array_index` for each template element. If the last element wasn't an identifier expression (e.g., an object property access), the captured index was lost. Fix: Stop searching after finding the first identifier expression in the template string, which is the potential array index. Co-authored-by: Cursor <cursoragent@cursor.com>
🦋 Changeset detectedLatest commit: 9f3a509 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 |
github-actions
bot
added
A-Linter
There was a problem hiding this comment.
Pull request overview
Fixes a false negative in lint/suspicious/noArrayIndexKey when an array index is used inside a template literal key but not in the last ${...} expression.
Changes:
- Updates template-literal scanning logic in
noArrayIndexKeyto avoid losing a previously captured identifier when later template expressions aren’t simple identifiers. - Adds invalid test cases for
key={${index}-${item}}andkey={${index}-${item.title}}.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| crates/biome_js_analyze/src/lint/suspicious/no_array_index_key.rs | Adjusts how identifier expressions are extracted from template literals for key analysis. |
| crates/biome_js_analyze/tests/specs/suspicious/noArrayIndexKey/invalid.jsx | Adds regression tests for index usage at the start of a template string and before property access. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
crates/biome_js_analyze/tests/specs/suspicious/noArrayIndexKey/invalid.jsx
Show resolved
Hide resolved
crates/biome_js_analyze/tests/specs/suspicious/noArrayIndexKey/invalid.jsx
Show resolved
Hide resolved
crates/biome_js_analyze/tests/specs/suspicious/noArrayIndexKey/invalid.jsx
Show resolved
Hide resolved
WalkthroughUpdates the lint rule that detects use of array indices in React keys inside template literals: the template-element handling now explicitly captures the first identifier found and breaks the search, ensuring index usage is detected regardless of its position in the template. Adds three invalid test components covering reordered index/value positions in template keys. Adds a patch changelog entry documenting the fix to noArrayIndexKey. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/biome_js_analyze/src/lint/suspicious/no_array_index_key.rs`:
- Around line 153-162: The current logic breaks early on the first identifier
expression in a template (the block using
template_element.expression().ok()?.as_js_identifier_expression() and setting
capture_array_index), which causes templates like `${item}-${index}` to capture
the wrong identifier; instead, collect all identifier expressions from the
template (or continue scanning instead of breaking) and then choose the
identifier that satisfies is_array_method_index, or iterate candidates calling
is_array_method_index until one returns true before assigning
capture_array_index; update the loop around template_element/identifier_expr to
accumulate candidates and only set capture_array_index when
is_array_method_index confirms the identifier is the array index.
🧹 Nitpick comments (1)
crates/biome_js_analyze/tests/specs/suspicious/noArrayIndexKey/invalid.jsx (1)
136-144: Add a regression case for value‑first template keysNice additions. To keep the fix order‑agnostic, consider adding a case like
${item}-${index}(or${item.title}-${index}) so a future change doesn’t quietly reintroduce order sensitivity.➕ Suggested test snippet
+// Issue `#8812`: value-first order should still trigger +function Component13() { + return things.map((item, index) => <div key={`${item}-${index}`}>{item}</div>); +}
| // Check if this element is an identifier expression (potential array index) | ||
| if let Some(identifier_expr) = | ||
| template_element.expression().ok()?.as_js_identifier_expression() | ||
| { | ||
| if let Ok(name) = identifier_expr.name() { | ||
| // Found a potential array index reference, use it and stop searching | ||
| // This fixes the issue where index position in template doesn't matter | ||
| capture_array_index = Some(name); | ||
| break; | ||
| } |
There was a problem hiding this comment.
Potential regression: template literal now ignores later identifiers
Lines 153-162: breaking on the first identifier means ${item}-${index} will capture item and miss index, so order dependence creeps back in. Please consider collecting all identifier expressions in the template and selecting the one that resolves to the array index parameter (or iterating candidates until one passes is_array_method_index).
🤖 Prompt for AI Agents
In `@crates/biome_js_analyze/src/lint/suspicious/no_array_index_key.rs` around
lines 153 - 162, The current logic breaks early on the first identifier
expression in a template (the block using
template_element.expression().ok()?.as_js_identifier_expression() and setting
capture_array_index), which causes templates like `${item}-${index}` to capture
the wrong identifier; instead, collect all identifier expressions from the
template (or continue scanning instead of breaking) and then choose the
identifier that satisfies is_array_method_index, or iterate candidates calling
is_array_method_index until one returns true before assigning
capture_array_index; update the loop around template_element/identifier_expr to
accumulate candidates and only set capture_array_index when
is_array_method_index confirms the identifier is the array index.
- Add changeset for noArrayIndexKey template string fix
- Add test case for value-first order (${item}-${index})
Co-authored-by: Cursor <cursoragent@cursor.com>
|
@murataslan1 Please don't ask copilot for a review. It doesn't have context about our repo like coderabbit does, and it just makes the PR harder to navigate. |
| { | ||
| if let Ok(name) = identifier_expr.name() { | ||
| // Found a potential array index reference, use it and stop searching | ||
| // This fixes the issue where index position in template doesn't matter |
There was a problem hiding this comment.
nit
| // This fixes the issue where index position in template doesn't matter |
2 participants
Fixes #8812
Problem
The
noArrayIndexKeyrule was not detecting array index usage when the index appeared before other expressions in a template string.Before:
Root Cause
The loop that processes template elements was overwriting
capture_array_indexfor each element. If the last element wasn't a simple identifier expression (e.g., it was an object property access likeitem.title), the previously captured index was lost.Solution
Stop searching after finding the first identifier expression in the template string. Once we find a potential array index, we use it and break out of the loop.
Testing
Added test cases for:
${index}-${item}- index at start${index}-${item.title}- index followed by object property accessMade with Cursor