Conversation
🦋 Changeset detectedLatest commit: ddbe360 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 |
WalkthroughAdds a registry-driven embed detection system and types: HostLanguage, GuestLanguage, EmbedCandidate/EmbedContent, TemplateTagKind; introduces a const-constructible Possibly related PRs
Suggested labels
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)
Comment |
![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_service/src/file_handlers/html.rs (1)
546-564: Clarification: First-JS-capture strategyThe code captures
embedded_file_sourcefrom the first successful JS parse (Line 559-561). If a Vue file has multiple<script>tags with differentlangattributes (e.g., one plain JS and one TS), only the first source type is used for subsequent text expressions. This appears intentional for consistency, but worth documenting if not already done elsewhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_service/src/file_handlers/html.rs` around lines 546 - 564, The code currently sets embedded_file_source from the first successful JS parse (see JsFileSource::js_module() initialization and the assignment inside the loop where parse_matched_embed returns parsed.js_file_source), which means in multi-<script> Vue files subsequent scripts with different lang (e.g., TS) are ignored; either document this first-js-capture strategy clearly above the loop (mention build_html_candidate, EmbedDetectorsRegistry::detect_match, parse_matched_embed, elements, nodes, and ctx) so future readers know this is intentional, or change the logic to choose the most specific/accurate source (e.g., prefer TS over JS) by comparing parsed.js_file_source values instead of taking the first—update comments/tests accordingly.
🤖 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_service/src/file_handlers/html.rs`:
- Around line 546-564: The code currently sets embedded_file_source from the
first successful JS parse (see JsFileSource::js_module() initialization and the
assignment inside the loop where parse_matched_embed returns
parsed.js_file_source), which means in multi-<script> Vue files subsequent
scripts with different lang (e.g., TS) are ignored; either document this
first-js-capture strategy clearly above the loop (mention build_html_candidate,
EmbedDetectorsRegistry::detect_match, parse_matched_embed, elements, nodes, and
ctx) so future readers know this is intentional, or change the logic to choose
the most specific/accurate source (e.g., prefer TS over JS) by comparing
parsed.js_file_source values instead of taking the first—update comments/tests
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 726e3e85-62ca-4073-99d9-e920bb1e521f
📒 Files selected for processing (2)
crates/biome_service/src/file_handlers/html.rscrates/biome_service/src/file_handlers/javascript.rs
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_service/src/file_handlers/html.rs`:
- Around line 850-868: The loop only handles AnySvelteDirective which excludes
Svelte "on:" event directives, so event handlers like on:click are never parsed;
update the branch in the Pass 4 loop to also detect and handle Svelte event
directives by attempting a cast for the event-specific node (e.g.,
AnySvelteEventDirective or AnySvelteOnDirective depending on your syntax types)
in addition to AnySvelteDirective, then feed that directive's initializer
through build_svelte_directive_candidate, EmbedDetectorsRegistry::detect_match,
and parse_matched_embed exactly as existing code does (push parsed.node into
nodes) so "on:" handlers are parsed/linted/formatted like other directives.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ab92c01c-aa97-49ba-986f-f4dec067e920
📒 Files selected for processing (1)
crates/biome_service/src/file_handlers/html.rs
Oh, ok. No, the issue belongs to the formatter and overlaps with the embedded snippet range. There's some issue there. I can fix it. |
github-actions
bot
added
A-Formatter
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/biome_js_formatter/src/js/auxiliary/template_chunk_element.rs (1)
51-58: Consider centralizing embed detector names to prevent silent formatting failures.The hardcoded constants
KNOWN_EMBED_TAGSandKNOWN_EMBED_OBJECTSduplicate the detector definitions fromJS_DETECTORS, creating a cross-crate maintenance burden. If someone adds a detector without updating these constants, embeds will silently fail to format.Given that
EmbedDetectortypes andJS_DETECTORSare not publicly exported from biome_service, consider either:
- Adding cross-references in
JS_DETECTORScomments pointing to where these constants are duplicated- Exporting detector definitions to allow biome_js_formatter to derive these constants
At minimum, a test validating alignment would catch future drift.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_js_formatter/src/js/auxiliary/template_chunk_element.rs` around lines 51 - 58, The duplicated hardcoded arrays KNOWN_EMBED_TAGS and KNOWN_EMBED_OBJECTS in template_chunk_element.rs can drift from the source JS_DETECTORS in biome_service; update the crate to remove duplication by either (1) importing/exporting the detector definitions from biome_service (expose JS_DETECTORS or a small public list of tag/object names) and derive KNOWN_EMBED_TAGS/KNOWN_EMBED_OBJECTS from that, or (2) if exporting is not possible, add a unit/integration test that iterates JS_DETECTORS (or the internal EmbedDetector definitions in biome_service) and asserts exact equality with these constants (and add a clear comment linking JS_DETECTORS to these constants) so future changes fail the test; reference the symbols KNOWN_EMBED_TAGS, KNOWN_EMBED_OBJECTS, JS_DETECTORS, and EmbedDetector when implementing the fix.
🤖 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_js_formatter/src/js/auxiliary/template_chunk_element.rs`:
- Around line 51-58: The duplicated hardcoded arrays KNOWN_EMBED_TAGS and
KNOWN_EMBED_OBJECTS in template_chunk_element.rs can drift from the source
JS_DETECTORS in biome_service; update the crate to remove duplication by either
(1) importing/exporting the detector definitions from biome_service (expose
JS_DETECTORS or a small public list of tag/object names) and derive
KNOWN_EMBED_TAGS/KNOWN_EMBED_OBJECTS from that, or (2) if exporting is not
possible, add a unit/integration test that iterates JS_DETECTORS (or the
internal EmbedDetector definitions in biome_service) and asserts exact equality
with these constants (and add a clear comment linking JS_DETECTORS to these
constants) so future changes fail the test; reference the symbols
KNOWN_EMBED_TAGS, KNOWN_EMBED_OBJECTS, JS_DETECTORS, and EmbedDetector when
implementing the fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dda92488-4002-4e84-a000-93cced4d4beb
⛔ Files ignored due to path filters (1)
crates/biome_service/src/workspace/snapshots/biome_service__workspace__server__tests__issue_9131.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (3)
.changeset/fix-embedded-template-crash.mdcrates/biome_js_formatter/src/js/auxiliary/template_chunk_element.rscrates/biome_service/src/workspace/server.tests.rs
✅ Files skipped from review due to trivial changes (1)
- .changeset/fix-embedded-template-crash.md
Merging this PR will not alter performance
Comparing 
Footnotes
|
|
Done, it's fixed @Netail :) |
Legend 🙌 |
dyc3
left a comment
dyc3
left a comment
There was a problem hiding this comment.
This approach makes a bit more sense. LGTM
3 participants
Summary
Closes #9131
Closes #9112
Closes #9166
This PR refines the detection of embedded snippets to be language-agnostic and const-friendly.
The new code completely abstracts from
DocumentFileService, and creates a few new concepts:HostLanguage- the language that can contain embeds - andGuestLanguage- the language that is hosted as a snippetEmbedDetectorwhich allows the creation of "detectors". They describe where, inside a host language, we can find possible embeds. They are generic constructs. They don't have logic on their own, other than exposing a function calledtry_matchwhich runs the logic against a candidate. The detector expose the concept of target, which can be either static or dynamic. The dynamic detector was created to fill cases where we have<script lang="ts>, for example.EmbedDetectorsRegistry, which is essentially the repository of detectors, and we usedetect_matchto determine if anEmbedCandidateis a hit.EmbedCandidateis a type that says "here we could have a possible embedded snippet". Contains a bunch of information that are needed to run a match, and eventually create an embedded snippet. The candidate is like a counterpart of the detector, but it's dynamic, because it needs to be created when we inspect the AST of a host language.detect_matchwhich returns anEmbedMatchwhich contains theGuestLanguage, which is used to tell the workspace "that's how you need to parse the snippet".I used AI to write the trivial code and docstring, but I wrote the whole architecture. Let me know if anything isn't clear.
Test Plan
Current tests should pass
Docs
N/A