fix(formatter): preserve space after svelte interpolation#8594
fix(formatter): preserve space after svelte interpolation#8594Carbophile wants to merge 1 commit intobiomejs:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 2519f94 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-Formatter
WalkthroughThis PR fixes the HTML formatter's handling of spacing after interpolation variables in Svelte templates. It introduces a new Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (4 passed)
✨ 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: 0
🧹 Nitpick comments (2)
crates/biome_html_formatter/src/utils/children.rs (2)
405-426: Minor:peekable()is unused.Line 409 creates a peekable iterator but
peek()is never called. A plain iterator would suffice.🔎 Suggested fix
- let mut trailing_trivia = last_token.trailing_trivia().pieces().peekable(); + let trailing_trivia = last_token.trailing_trivia().pieces(); - while let Some(piece) = trailing_trivia.next() { + for piece in trailing_trivia {
450-478: The unreachable else branch is correctly documented.The comment on line 466 correctly identifies that this branch should be unreachable. Given the outer match pattern guarantees
lastis one ofEmptyLine|Newline|Whitespace|TrailingWhitespace, and lines 461-464 cover all four cases, the else branch at line 465-467 is indeed dead code.If you want to be explicit, you could use
unreachable!()instead:🔎 Optional: make the unreachable path explicit
} else if matches!(last, HtmlChild::Whitespace | HtmlChild::Newline | HtmlChild::EmptyLine) { // Keep the existing whitespace/newline as it's more significant (allows breaking) } else { - // Should be unreachable if logic is correct (last is one of the matched types) - *last = child; + unreachable!("last must be one of EmptyLine, Newline, Whitespace, or TrailingWhitespace") }That said, the defensive assignment is harmless and the code works correctly.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (9)
crates/biome_html_formatter/tests/specs/html/elements/inline/mixed-block-inline.html.snapis excluded by!**/*.snapand included by**crates/biome_html_formatter/tests/specs/html/elements/inline/tags-hug-content-longer-w-attr.html.snapis excluded by!**/*.snapand included by**crates/biome_html_formatter/tests/specs/html/svelte/each_with_destructuring.svelte.snapis excluded by!**/*.snapand included by**crates/biome_html_formatter/tests/specs/html/svelte/interpolation_spacing.svelte.snapis excluded by!**/*.snapand included by**crates/biome_html_formatter/tests/specs/prettier/html/cdata/example.html.snapis excluded by!**/*.snapand included by**crates/biome_html_formatter/tests/specs/prettier/html/svg/svg.html.snapis excluded by!**/*.snapand included by**crates/biome_html_formatter/tests/specs/prettier/html/tags/tags.html.snapis excluded by!**/*.snapand included by**crates/biome_html_formatter/tests/specs/prettier/html/whitespace/inline-nodes.html.snapis excluded by!**/*.snapand included by**crates/biome_html_formatter/tests/specs/prettier/vue/html-vue/elastic-header.html.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (4)
.changeset/good-fix-interpolation-spacing.mdcrates/biome_html_formatter/src/html/lists/element_list.rscrates/biome_html_formatter/src/utils/children.rscrates/biome_html_formatter/tests/specs/html/svelte/interpolation_spacing.svelte
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.rs: Use inline rustdoc documentation for rules, assists, and their options
Use thedbg!()macro for debugging output in Rust tests and code
Use doc tests (doctest) format with code blocks in rustdoc comments; ensure assertions pass in tests
Files:
crates/biome_html_formatter/src/html/lists/element_list.rscrates/biome_html_formatter/src/utils/children.rs
🧠 Learnings (10)
📓 Common learnings
Learnt from: dyc3
Repo: biomejs/biome PR: 8291
File: crates/biome_html_formatter/tests/specs/prettier/vue/html-vue/elastic-header.html:10-10
Timestamp: 2025-12-04T13:29:49.287Z
Learning: Files under `crates/biome_html_formatter/tests/specs/prettier` are test fixtures synced from Prettier and should not receive detailed code quality reviews (e.g., HTTP vs HTTPS, formatting suggestions, etc.). These files are test data meant to validate formatter behavior and should be preserved as-is.
📚 Learning: 2025-12-04T13:29:49.287Z
Learnt from: dyc3
Repo: biomejs/biome PR: 8291
File: crates/biome_html_formatter/tests/specs/prettier/vue/html-vue/elastic-header.html:10-10
Timestamp: 2025-12-04T13:29:49.287Z
Learning: Files under `crates/biome_html_formatter/tests/specs/prettier` are test fixtures synced from Prettier and should not receive detailed code quality reviews (e.g., HTTP vs HTTPS, formatting suggestions, etc.). These files are test data meant to validate formatter behavior and should be preserved as-is.
Applied to files:
.changeset/good-fix-interpolation-spacing.mdcrates/biome_html_formatter/tests/specs/html/svelte/interpolation_spacing.svelte
📚 Learning: 2025-12-21T21:15:03.796Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-12-21T21:15:03.796Z
Learning: For formatter changes in changesets, show formatting changes using diff code blocks
Applied to files:
.changeset/good-fix-interpolation-spacing.md
📚 Learning: 2025-11-24T18:05:20.371Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:05:20.371Z
Learning: Use the `just gen-formatter` command to generate initial formatter implementations from the grammar, which will use `format_verbatim_node` that must be replaced with proper `biome_formatter` utilities
Applied to files:
.changeset/good-fix-interpolation-spacing.md
📚 Learning: 2025-11-24T18:06:03.545Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_parser/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:06:03.545Z
Learning: Applies to crates/biome_parser/**/src/**/*.rs : Use `ParseSeparatedList` and `ParseNodeList` for parsing lists with error recovery to avoid infinite loops
Applied to files:
crates/biome_html_formatter/src/html/lists/element_list.rs
📚 Learning: 2025-09-13T16:16:06.459Z
Learnt from: ematipico
Repo: biomejs/biome PR: 7467
File: crates/biome_service/src/file_handlers/html.rs:456-466
Timestamp: 2025-09-13T16:16:06.459Z
Learning: In biome_formatter, the printer correctly handles consecutive LineMode::Hard elements without creating extra blank lines. Multiple consecutive FormatElement::Line(LineMode::Hard) elements in the formatting code do not result in duplicate blank lines in the output because the printer has logic to track line states and handle indentation properly.
Applied to files:
crates/biome_html_formatter/src/html/lists/element_list.rs
📚 Learning: 2025-09-13T16:16:06.459Z
Learnt from: ematipico
Repo: biomejs/biome PR: 7467
File: crates/biome_service/src/file_handlers/html.rs:456-466
Timestamp: 2025-09-13T16:16:06.459Z
Learning: In biome_formatter, consecutive LineMode::Hard elements are automatically collapsed to a single newline by the printer. The printer has a test "it_prints_consecutive_hard_lines_as_one" that demonstrates this behavior - multiple consecutive hard line breaks result in only one newline in the output, not extra blank lines.
Applied to files:
crates/biome_html_formatter/src/html/lists/element_list.rs
📚 Learning: 2025-11-24T18:05:20.371Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:05:20.371Z
Learning: Define `FormatHtmlSyntaxNode` struct in a `cst.rs` file implementing `FormatRule<HtmlSyntaxNode>`, `AsFormat<HtmlFormatContext>`, and `IntoFormat<HtmlFormatContext>` traits using the provided boilerplate code
Applied to files:
crates/biome_html_formatter/src/utils/children.rs
📚 Learning: 2025-11-24T18:05:20.371Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:05:20.371Z
Learning: Applies to crates/biome_formatter/**/biome_*_formatter/tests/spec_tests.rs : Use the `tests_macros::gen_tests!` macro in `spec_tests.rs` to generate test functions for each specification file matching the pattern `tests/specs/<language>/**/*.<ext>`
Applied to files:
crates/biome_html_formatter/tests/specs/html/svelte/interpolation_spacing.svelte
📚 Learning: 2025-11-24T18:05:20.371Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:05:20.371Z
Learning: Applies to crates/biome_formatter/**/biome_*_formatter/tests/language.rs : Implement `TestFormatLanguage` trait in `tests/language.rs` for the formatter's test language
Applied to files:
crates/biome_html_formatter/tests/specs/html/svelte/interpolation_spacing.svelte
🧬 Code graph analysis (2)
crates/biome_html_formatter/src/html/lists/element_list.rs (3)
crates/biome_formatter/src/format_element.rs (2)
is_hard(102-104)last(408-413)crates/biome_html_formatter/src/utils/metadata.rs (1)
is_element_whitespace_sensitive_from_element(778-794)crates/biome_formatter/src/builders.rs (2)
hard_line_break(99-101)soft_line_break_or_space(188-190)
crates/biome_html_formatter/src/utils/children.rs (1)
crates/biome_rowan/src/syntax/node.rs (1)
last_token(327-329)
🔇 Additional comments (6)
crates/biome_html_formatter/tests/specs/html/svelte/interpolation_spacing.svelte (1)
1-2: LGTM!Good test coverage for the interpolation spacing fix. Both cases (Svelte interpolation
{variable}and inline element<span>) are covered to ensure trailing spaces are preserved..changeset/good-fix-interpolation-spacing.md (1)
1-5: LGTM!Changeset accurately describes the fix. Patch level is appropriate for this bug fix.
crates/biome_html_formatter/src/html/lists/element_list.rs (3)
260-265: LGTM!Correctly extends the match arm to include
TrailingWhitespace, ensuring words followed by trailing whitespace don't get an extra separator.
336-379: Logic looks correct for the interpolation spacing fix.The conditional logic properly preserves trailing whitespace as a space (via
soft_line_break_or_space) when the preceding element is whitespace-sensitive, which addresses the core issue. The fallback tohard_line_breakfor non-whitespace-sensitive elements maintains proper formatting.One observation: the nested conditionals are a bit dense. Consider extracting a helper function if this grows further, but it's acceptable for now.
529-534: LGTM!Correctly defers separator handling to the
TrailingWhitespacematch arm when aNonTextelement is followed by trailing whitespace.crates/biome_html_formatter/src/utils/children.rs (1)
129-136: LGTM!Good rustdoc documentation explaining the semantic purpose of
TrailingWhitespaceand how it differs from regularWhitespace. This aligns with the coding guidelines for inline rustdoc.
|
Superseded by #8595 |
ematipico
left a comment
ematipico
left a comment
There was a problem hiding this comment.
Yeah this solution is nonsense. It affected code that isn't related to svelte too.
I mean, it's the same underlying issue, now that I look at it it isn't even Svelte exclusive. But fair enough |
2 participants
The solution was architected by me and implemented by Google Jules (an LLM).
Summary
This pull request makes sure Biome doesn't remove contenful spaces in HTML like those after Svelte interopelation.
Closes #8584
Test Plan
A test has been implemented for this case.
Docs
N/A