Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
154 changes: 112 additions & 42 deletions crates/biome_html_formatter/src/html/lists/element_list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -842,22 +842,97 @@ pub(crate) struct FormatMultilineChildren {

impl Format<HtmlFormatContext> for FormatMultilineChildren {
fn fmt(&self, f: &mut Formatter<HtmlFormatContext>) -> FormatResult<()> {
let format_inner = format_once(|f| {
let prefix = f.intern_vec(self.elements_prefix.take());
// We do not need the block ident when the list node is at the html root node
if self.is_root {
let format_inner = format_once(|f| {
let prefix = f.intern_vec(self.elements_prefix.take());

if let Some(elements) = f.intern_vec(self.elements.take()) {
match self.layout {
MultilineLayout::Fill => {
if let Some(prefix) = prefix {
f.write_elements([prefix])?;
}
f.write_elements([
FormatElement::Tag(Tag::StartFill),
elements,
FormatElement::Tag(Tag::EndFill),
])?;
}
MultilineLayout::NoFill => {
f.write_elements([FormatElement::Tag(Tag::StartGroup(
tag::Group::new().with_mode(GroupMode::Expand),
))])?;
if let Some(prefix) = prefix {
f.write_elements([prefix])?;
}
f.write_elements([elements, FormatElement::Tag(Tag::EndGroup)])?;
}
};
}

if let Some(elements) = f.intern_vec(self.elements.take()) {
match self.layout {
MultilineLayout::Fill => {
Ok(())
});
return write!(f, [format_inner]);
}

// For Fill layout, we need to ensure the Indent tags are inside the Fill tags
// to avoid tag mismatch errors when interned elements contain nested Indent tags.
match self.layout {
MultilineLayout::Fill => {
let prefix = self.elements_prefix.take();
let elements = self.elements.take();

let format_inner = format_once(|f| {
let prefix = f.intern_vec(prefix);

if let Some(elements) = f.intern_vec(elements) {
if let Some(prefix) = prefix {
f.write_elements([prefix])?;
}
f.write_elements([
FormatElement::Tag(Tag::StartFill),
elements,
FormatElement::Tag(Tag::EndFill),
])?;
// Put the Fill tags on the outside, with block_indent inside
// This ensures that when interned elements contain nested Indent tags,
// they are properly closed before the Fill tag is closed.
f.write_elements([FormatElement::Tag(Tag::StartFill)])?;
// Create a format closure for the interned elements and wrap it in block_indent
let elements_format = format_with(|f| {
f.write_elements([elements])
});
write!(f, [block_indent(&elements_format)])?;
f.write_elements([FormatElement::Tag(Tag::EndFill)])?;
}
MultilineLayout::NoFill => {

Ok(())
});
// This indent is wrapped with a group to ensure that the print mode is
// set to `Expanded` when the group prints and will guarantee that the
// content _does not_ fit when printed as part of a `Fill`. Example:
// <div>
// <span a b>
// <Foo />
// </span>{" "}
// ({variable})
// </div>
// The `<span>...</span>` is the element that gets wrapped in the group
// by this line. Importantly, it contains a hard line break, and because
// [FitsMeasurer::fits_element] considers all hard lines as `Fits::Yes`,
// it will cause the element and the following separator to be printed
// in flat mode due to the logic of `Fill`. But because the we know the
// item breaks over multiple lines, we want it to _not_ fit and print
// both the content and the separator in Expanded mode, keeping the
// formatting as shown above.
//
// The `group` here allows us to opt-in to telling the `FitsMeasurer`
// that content that breaks shouldn't be considered flat and should be
// expanded. This is in contrast to something like a concise array fill,
// which _does_ allow breaks to fit and preserves density.
write!(f, [group(&format_inner)])
}
MultilineLayout::NoFill => {
let format_inner = format_once(|f| {
let prefix = f.intern_vec(self.elements_prefix.take());

if let Some(elements) = f.intern_vec(self.elements.take()) {
f.write_elements([FormatElement::Tag(Tag::StartGroup(
tag::Group::new().with_mode(GroupMode::Expand),
))])?;
Expand All @@ -866,39 +941,34 @@ impl Format<HtmlFormatContext> for FormatMultilineChildren {
}
f.write_elements([elements, FormatElement::Tag(Tag::EndGroup)])?;
}
};
}

Ok(())
});
// We do not need the block ident when the list node is at the html root node
if self.is_root {
return write!(f, [format_inner]);
Ok(())
});
// This indent is wrapped with a group to ensure that the print mode is
// set to `Expanded` when the group prints and will guarantee that the
// content _does not_ fit when printed as part of a `Fill`. Example:
// <div>
// <span a b>
// <Foo />
// </span>{" "}
// ({variable})
// </div>
// The `<span>...</span>` is the element that gets wrapped in the group
// by this line. Importantly, it contains a hard line break, and because
// [FitsMeasurer::fits_element] considers all hard lines as `Fits::Yes`,
// it will cause the element and the following separator to be printed
// in flat mode due to the logic of `Fill`. But because the we know the
// item breaks over multiple lines, we want it to _not_ fit and print
// both the content and the separator in Expanded mode, keeping the
// formatting as shown above.
//
// The `group` here allows us to opt-in to telling the `FitsMeasurer`
// that content that breaks shouldn't be considered flat and should be
// expanded. This is in contrast to something like a concise array fill,
// which _does_ allow breaks to fit and preserves density.
write!(f, [group(&block_indent(&format_inner))])
}
}

// This indent is wrapped with a group to ensure that the print mode is
// set to `Expanded` when the group prints and will guarantee that the
// content _does not_ fit when printed as part of a `Fill`. Example:
// <div>
// <span a b>
// <Foo />
// </span>{" "}
// ({variable})
// </div>
// The `<span>...</span>` is the element that gets wrapped in the group
// by this line. Importantly, it contains a hard line break, and because
// [FitsMeasurer::fits_element] considers all hard lines as `Fits::Yes`,
// it will cause the element and the following separator to be printed
// in flat mode due to the logic of `Fill`. But because the we know the
// item breaks over multiple lines, we want it to _not_ fit and print
// both the content and the separator in Expanded mode, keeping the
// formatting as shown above.
//
// The `group` here allows us to opt-in to telling the `FitsMeasurer`
// that content that breaks shouldn't be considered flat and should be
// expanded. This is in contrast to something like a concise array fill,
// which _does_ allow breaks to fit and preserves density.
write!(f, [group(&block_indent(&format_inner))])
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
<div class="message" [class.me]="message.sender === peerService.localId()">
@if (message.status === "Failure") {
<div>
<mat-icon svgIcon="error" class="warn" matTooltip="Message sending failed!"></mat-icon>
</div>
}
<mat-card class="message-card mat-shadow-2">
<mat-card-header>
<mat-card-subtitle>{{ message.sender }}</mat-card-subtitle>
</mat-card-header>
<mat-card-content>
@switch (message.type) { @case ("Text") {<span class="content">{{ message.text }}</span>} @case
("File") {<span class="content">{{ message.fileName }}({{ message.fileSize | fileSize }})</span>
@if (isImage) {
<img [src]="imageUrl" alt="image" style="max-width: 100%">} } }<mat-divider></mat-divider>
</mat-card-content>
<mat-card-actions>
@switch (message.type) { @case ("Text") {
<button
(click)="onCopy(message)"
appThrottleButton
mat-icon-button
class="accent"
matTooltip="Copy to clipboard"
type="button"
>
<mat-icon svgIcon="content_copy"></mat-icon>
</button>
} @case ("File") {
<button
(click)="onSave(message)"
appThrottleButton
mat-icon-button
class="accent"
matTooltip="Save"
type="button"
>
<mat-icon svgIcon="download"></mat-icon>
</button>
} }<span class="time">{{ message.timestamp | date: "HH:mm" }}</span>
</mat-card-actions>
<mat-card-footer>
@if (message.status === "Queued") {
<mat-progress-bar class="accent" mode="indeterminate"></mat-progress-bar>}
</mat-card-footer>
</mat-card>
</div>

Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,11 @@ fn handle_collections(
options: &UseJsxKeyInIterableOptions,
) -> Vec<TextRange> {
let is_inside_jsx = node.parent::<JsxExpressionChild>().is_some();
// Only check for keys when the array is inside JSX context (used for rendering)
// Arrays outside JSX context (e.g., const arr = [<Hello />]) don't need keys
if !is_inside_jsx {
return vec![];
}
Comment on lines 130 to +135
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, find the biome_rowan crate and understand how parent::<T>() works
fd -t f "rowan" --type d | head -5

Repository: biomejs/biome

Length of output: 76


🏁 Script executed:

# Search for parent method definition in the codebase
rg -t rs "fn parent" --max-count 20 | grep -E "(pub|fn)\s+parent"

Repository: biomejs/biome

Length of output: 83


🏁 Script executed:

# Look at the test cases in the file to see what's covered
cat -n crates/biome_js_analyze/src/lint/correctness/use_jsx_key_in_iterable.rs | head -200

Repository: biomejs/biome

Length of output: 7995


🏁 Script executed:

# Find the AstNode trait definition in biome_rowan
fd -path "*/biome_rowan/*" -type f -name "*.rs" | xargs rg -l "fn parent"

Repository: biomejs/biome

Length of output: 1115


🏁 Script executed:

# Search for parent method signature specifically
rg "fn parent" crates/biome_rowan/ -A 2

Repository: biomejs/biome

Length of output: 2064


🏁 Script executed:

# Check if there are tests in the use_jsx_key_in_iterable file that cover wrapped arrays
tail -n +200 crates/biome_js_analyze/src/lint/correctness/use_jsx_key_in_iterable.rs | head -300

Repository: biomejs/biome

Length of output: 10131


🏁 Script executed:

# Look for test cases in the file
tail -n +500 crates/biome_js_analyze/src/lint/correctness/use_jsx_key_in_iterable.rs | head -500

Repository: biomejs/biome

Length of output: 974


🏁 Script executed:

# Check the AstNode trait definition in biome_rowan/src/ast/mod.rs
sed -n '1,50p' crates/biome_rowan/src/ast/mod.rs

Repository: biomejs/biome

Length of output: 1842


🏁 Script executed:

# Search for the unwrap_parenthesis function to understand how parentheses are handled elsewhere
rg "fn unwrap_parenthesis" crates/biome_js_analyze/src/lint/correctness/use_jsx_key_in_iterable.rs -A 5

Repository: biomejs/biome

Length of output: 340


🏁 Script executed:

# Search for test cases at the end of the file
tail -n +850 crates/biome_js_analyze/src/lint/correctness/use_jsx_key_in_iterable.rs

Repository: biomejs/biome

Length of output: 39


🏁 Script executed:

# Look at the AstNode trait's parent method more carefully - check if there's an ancestors method
rg "fn ancestors" crates/biome_rowan/src/ast/mod.rs -A 3

Repository: biomejs/biome

Length of output: 39


🏁 Script executed:

# Check if SyntaxNode has an ancestors method
rg "fn ancestors" crates/biome_rowan/src/syntax/node.rs -A 3

Repository: biomejs/biome

Length of output: 177


🏁 Script executed:

# Get test cases at the end of the file
wc -l crates/biome_js_analyze/src/lint/correctness/use_jsx_key_in_iterable.rs

Repository: biomejs/biome

Length of output: 132


🏁 Script executed:

# Get the test section
tail -n 200 crates/biome_js_analyze/src/lint/correctness/use_jsx_key_in_iterable.rs

Repository: biomejs/biome

Length of output: 6216


🏁 Script executed:

# Get the full test section at the end of the file
tail -n +330 crates/biome_js_analyze/src/lint/correctness/use_jsx_key_in_iterable.rs

Repository: biomejs/biome

Length of output: 6156


🏁 Script executed:

# Check line 11 to confirm JsxExpressionChild is imported correctly
sed -n '1,15p' crates/biome_js_analyze/src/lint/correctness/use_jsx_key_in_iterable.rs

Repository: biomejs/biome

Length of output: 873


🏁 Script executed:

# Find test cases for this rule
rg "use_jsx_key_in_iterable" --type f --include="*.rs" | grep -i test

Repository: biomejs/biome

Length of output: 498


🏁 Script executed:

# Look at the test file
fd "use_jsx_key_in_iterable" -path "*test*" -o -path "*tests*" | head -5

Repository: biomejs/biome

Length of output: 225


🏁 Script executed:

# Search for test cases in the codebase
rg "use_jsx_key_in_iterable" | grep -i "test\|check"

Repository: biomejs/biome

Length of output: 39


🏁 Script executed:

# Look for test directories related to lints
find crates/biome_js_analyze -type d -name "*test*" -o -name "*tests*"

Repository: biomejs/biome

Length of output: 351


🏁 Script executed:

# Check if tests are in the same file or a separate directory
ls -la crates/biome_js_analyze/src/lint/correctness/

Repository: biomejs/biome

Length of output: 4396


🏁 Script executed:

# Search for test specifications or snapshots
find crates/biome_js_analyze/tests -type f -name "*.json" | xargs grep -l "use_jsx_key_in_iterable" 2>/dev/null | head -5

Repository: biomejs/biome

Length of output: 39


🏁 Script executed:

# Look for how rules are tested
ls -la crates/biome_js_analyze/tests/

Repository: biomejs/biome

Length of output: 546


🏁 Script executed:

# Check the spec_tests.rs file
head -50 crates/biome_js_analyze/tests/spec_tests.rs

Repository: biomejs/biome

Length of output: 2132


🏁 Script executed:

# Find test specifications for use_jsx_key_in_iterable
find crates/biome_js_analyze/tests/specs -type f | xargs grep -l "use_jsx_key_in_iterable" 2>/dev/null

Repository: biomejs/biome

Length of output: 41


🏁 Script executed:

# Look for jsx-key or similar test files
ls -la crates/biome_js_analyze/tests/specs/*/

Repository: biomejs/biome

Length of output: 26782


🏁 Script executed:

# Search for test files related to JSX keys
find crates/biome_js_analyze/tests/specs -type f -name "*jsx*key*" -o -name "*key*"

Repository: biomejs/biome

Length of output: 231


🏁 Script executed:

# List test files for useJsxKeyInIterable
ls -la crates/biome_js_analyze/tests/specs/correctness/useJsxKeyInIterable/

Repository: biomejs/biome

Length of output: 660


🏁 Script executed:

# Check one of the test files to understand the format
head -100 crates/biome_js_analyze/tests/specs/correctness/useJsxKeyInIterable/invalid.jsx

Repository: biomejs/biome

Length of output: 2006


🏁 Script executed:

# Check the valid.jsx test file
cat -n crates/biome_js_analyze/tests/specs/correctness/useJsxKeyInIterable/valid.jsx

Repository: biomejs/biome

Length of output: 5522


🏁 Script executed:

# Now let me check the AstNode implementation more carefully - search for how parent::<T>() is defined
cat -n crates/biome_rowan/src/ast/mod.rs | sed -n '1,100p'

Repository: biomejs/biome

Length of output: 4311


🏁 Script executed:

# Search for the actual parent::<T> method implementation in the AstNode trait
rg "fn parent" crates/biome_rowan/src/ast/mod.rs -A 5

Repository: biomejs/biome

Length of output: 366


🏁 Script executed:

# Search for uses of .ancestors() in the js_analyze crate to see if there's a pattern for this
rg "\.ancestors\(" crates/biome_js_analyze/src/lint/ | head -10

Repository: biomejs/biome

Length of output: 1133


🏁 Script executed:

# Check if there's a test for parenthesized array in JSX expression
rg "\{\(\[" crates/biome_js_analyze/tests/specs/correctness/useJsxKeyInIterable/

Repository: biomejs/biome

Length of output: 39


Parenthesised arrays inside JSX won't be checked for keys.

node.parent::<JsxExpressionChild>() checks immediate parent only. For {([<Hello />])}, the array's parent is JsParenthesizedExpression, not JsxExpressionChild, so the rule skips the check. Use .ancestors() instead to walk up the tree. This pattern is already used elsewhere in the analyser for similar cases.

Suggested fix
-    let is_inside_jsx = node.parent::<JsxExpressionChild>().is_some();
+    let is_inside_jsx = node
+        .syntax()
+        .ancestors()
+        .any(|ancestor| JsxExpressionChild::can_cast(ancestor.kind()));
📝 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 is_inside_jsx = node.parent::<JsxExpressionChild>().is_some();
// Only check for keys when the array is inside JSX context (used for rendering)
// Arrays outside JSX context (e.g., const arr = [<Hello />]) don't need keys
if !is_inside_jsx {
return vec![];
}
let is_inside_jsx = node
.syntax()
.ancestors()
.any(|ancestor| JsxExpressionChild::can_cast(ancestor.kind()));
// Only check for keys when the array is inside JSX context (used for rendering)
// Arrays outside JSX context (e.g., const arr = [<Hello />]) don't need keys
if !is_inside_jsx {
return vec![];
}
🤖 Prompt for AI Agents
In @crates/biome_js_analyze/src/lint/correctness/use_jsx_key_in_iterable.rs
around lines 130 - 135, The check using node.parent::<JsxExpressionChild>() only
inspects the immediate parent and misses cases like parenthesised arrays (e.g.,
JsParenthesizedExpression) inside JSX; change the logic that sets is_inside_jsx
to walk ancestors instead (use node.ancestors() and detect any
JsxExpressionChild ancestor) so arrays wrapped in parentheses are correctly
recognized as being inside JSX; update the code around the is_inside_jsx
computation in use_jsx_key_in_iterable.rs to use an ancestors() search for
JsxExpressionChild rather than parent::<JsxExpressionChild>().

node.elements()
.iter()
.filter_map(|node| {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,5 @@
import React from "react";

[<Hello />, <Hello />, <Hello />];

[...[<Hello />, <Hello />], <Hello />];

[<Hello />, xyz ? <Hello />: <Hello />, <Hello />];

data.map(x => <Hello>{x}</Hello>);

data.map(x => <>{x}</>);
Expand All @@ -18,12 +12,13 @@ Array.from([1, 2, 3], (x) => {
return <Hello>{x}</Hello>
});

[React.createElement("h1"), React.createElement("h1"), React.createElement("h1")];

data.map(c => React.createElement("h1"));

React.Children.map(c => React.cloneElement(c));

// Standalone arrays (not inside JSX) should not trigger the rule
// These cases are now handled in valid.jsx

(<h1>{data.map(x => <h1>{x}</h1>)}</h1>)

(<h1>{[<h1></h1>, <h1></h1>, <h1></h1>]}</h1>)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,25 @@ const Valid = [<>
<p>Test 3</p>
</>]

// Arrays outside JSX context should not require keys
const components = [<Hello />, <Hello />, <Hello />];

const routes = [<Route path="/" />, <Route path="/about" />];

export const menuItems = [<MenuItem />, <MenuItem />];

let items = [<Item />];

var config = [<ConfigItem />, <ConfigItem />];

// Standalone array literals (not inside JSX) should not require keys
[<Hello />, <Hello />, <Hello />];

[...[<Hello />, <Hello />], <Hello />];

[<Hello />, xyz ? <Hello />: <Hello />, <Hello />];

[React.createElement("h1"), React.createElement("h1"), React.createElement("h1")];

// should not generate diagnostics
import { component$ } from "@builder.io/qwik";
Expand Down
Loading