Conversation
🦋 Changeset detectedLatest commit: 5f77a1f 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 |
Parser conformance results onjs/262
jsx/babel
markdown/commonmark
symbols/microsoft
ts/babel
ts/microsoft
|
dyc3
force-pushed
the
dyc3/grit-native-node-field-names
branch
from
4c0e707 to
da741be
Compare
Merging this PR will not alter performance
Comparing 
Footnotes
|
dyc3
force-pushed
the
dyc3/grit-native-node-field-names
branch
2 times, most recently
from
dfbfc4e to
4ab3417
Compare
|
cc @arendjr was this intended to be implemented originally? |
|
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:
WalkthroughThis PR separates legacy Tree‑sitter and native Biome node-name resolution for Grit patterns. It adds a public Possibly related PRs
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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/biome_grit_patterns/src/grit_target_language/json_target_language.rs (1)
59-69:⚠️ Potential issue | 🟠 MajorAdd native slot support for JSON to match JS/CSS architecture.
The
sourceparameter is ignored, and this always returns legacy TreeSitter slots. Unlike JS and CSS implementations, there's no routing tonative_slots_for_name(node_name)whensource == Native.Critically, JSON's native patterns like
JsonMemberName,JsonMemberList, andJsonArrayElementListhave no legacy TreeSitter equivalents, so they silently return empty slots. This prevents named field matching for native JSON patterns and creates an architectural inconsistency—JS and CSS both properly route based on source; JSON should too.To fix this, you'll need to generate
native_slots_for_namein the JSON generated mappings and updatenamed_slots_for_nodeto route based on the source parameter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_grit_patterns/src/grit_target_language/json_target_language.rs` around lines 59 - 69, The current named_slots_for_node implementation always returns legacy_treesitter_slots_for_kind and ignores the _source parameter, which prevents native JSON patterns from returning slots; update named_slots_for_node (in json_target_language.rs) to match the JS/CSS pattern: check kind.as_json_kind(), then match on the provided source (GritNodePatternSource) and, when source == Native, call native_slots_for_name(node_name) (ensure native_slots_for_name is generated into the JSON mappings), otherwise fall back to legacy_treesitter_slots_for_kind(kind); this will expose native slots for JsonMemberName/JsonMemberList/JsonArrayElementList and keep legacy behavior for TreeSitter source.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/thirty-lines-sit.md:
- Line 5: Update the changeset description to include the GitHub issue reference
at the start using the required format: prefix the existing line "Fixed Grit
queries that use native Biome AST node names with native field names. Queries
such as `JsConditionalExpression(consequent = $cons, alternate = $alt)` now
compile successfully in `biome search`." with "Fixed
[`#9734`](https://github.com/biomejs/biome/issues/9734):" so the final description
begins "Fixed [`#9734`](https://github.com/biomejs/biome/issues/9734): Fixed Grit
queries..." ensuring the issue number and link are present as per guidelines.
---
Outside diff comments:
In `@crates/biome_grit_patterns/src/grit_target_language/json_target_language.rs`:
- Around line 59-69: The current named_slots_for_node implementation always
returns legacy_treesitter_slots_for_kind and ignores the _source parameter,
which prevents native JSON patterns from returning slots; update
named_slots_for_node (in json_target_language.rs) to match the JS/CSS pattern:
check kind.as_json_kind(), then match on the provided source
(GritNodePatternSource) and, when source == Native, call
native_slots_for_name(node_name) (ensure native_slots_for_name is generated into
the JSON mappings), otherwise fall back to
legacy_treesitter_slots_for_kind(kind); this will expose native slots for
JsonMemberName/JsonMemberList/JsonArrayElementList and keep legacy behavior for
TreeSitter source.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 06559229-fa55-45b3-90c4-fcf9c9dde786
⛔ Files ignored due to path filters (11)
crates/biome_grit_patterns/tests/specs/css/declaration_slot_names.snapis excluded by!**/*.snapand included by**crates/biome_grit_patterns/tests/specs/css/function_slot_names.snapis excluded by!**/*.snapand included by**crates/biome_grit_patterns/tests/specs/css/native_slot_names.snapis excluded by!**/*.snapand included by**crates/biome_grit_patterns/tests/specs/css/selector_slot_names.snapis excluded by!**/*.snapand included by**crates/biome_grit_patterns/tests/specs/ts/if_slot_names.snapis excluded by!**/*.snapand included by**crates/biome_grit_patterns/tests/specs/ts/legacy_conditional_slots.snapis excluded by!**/*.snapand included by**crates/biome_grit_patterns/tests/specs/ts/member_slot_names.snapis excluded by!**/*.snapand included by**crates/biome_grit_patterns/tests/specs/ts/mixed_slot_names.snapis excluded by!**/*.snapand included by**crates/biome_grit_patterns/tests/specs/ts/native_slot_aliases.snapis excluded by!**/*.snapand included by**crates/biome_grit_patterns/tests/specs/ts/while_slot_names.snapis excluded by!**/*.snapand included by**crates/biome_grit_patterns/tests/specs/tsx/jsx_attribute_slot_names.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (32)
.changeset/thirty-lines-sit.md.claude/skills/changeset/SKILL.mdcrates/biome_grit_patterns/src/grit_target_language.rscrates/biome_grit_patterns/src/grit_target_language/css_target_language.rscrates/biome_grit_patterns/src/grit_target_language/css_target_language/generated_mappings.rscrates/biome_grit_patterns/src/grit_target_language/js_target_language.rscrates/biome_grit_patterns/src/grit_target_language/js_target_language/generated_mappings.rscrates/biome_grit_patterns/src/grit_target_language/json_target_language.rscrates/biome_grit_patterns/src/pattern_compiler/node_like_compiler.rscrates/biome_grit_patterns/tests/specs/css/declaration_slot_names.csscrates/biome_grit_patterns/tests/specs/css/declaration_slot_names.gritcrates/biome_grit_patterns/tests/specs/css/function_slot_names.csscrates/biome_grit_patterns/tests/specs/css/function_slot_names.gritcrates/biome_grit_patterns/tests/specs/css/native_slot_names.csscrates/biome_grit_patterns/tests/specs/css/native_slot_names.gritcrates/biome_grit_patterns/tests/specs/css/selector_slot_names.csscrates/biome_grit_patterns/tests/specs/css/selector_slot_names.gritcrates/biome_grit_patterns/tests/specs/ts/if_slot_names.gritcrates/biome_grit_patterns/tests/specs/ts/if_slot_names.tscrates/biome_grit_patterns/tests/specs/ts/legacy_conditional_slots.gritcrates/biome_grit_patterns/tests/specs/ts/legacy_conditional_slots.tscrates/biome_grit_patterns/tests/specs/ts/member_slot_names.gritcrates/biome_grit_patterns/tests/specs/ts/member_slot_names.tscrates/biome_grit_patterns/tests/specs/ts/mixed_slot_names.gritcrates/biome_grit_patterns/tests/specs/ts/mixed_slot_names.tscrates/biome_grit_patterns/tests/specs/ts/native_slot_aliases.gritcrates/biome_grit_patterns/tests/specs/ts/native_slot_aliases.tscrates/biome_grit_patterns/tests/specs/ts/while_slot_names.gritcrates/biome_grit_patterns/tests/specs/ts/while_slot_names.tscrates/biome_grit_patterns/tests/specs/tsx/jsx_attribute_slot_names.gritcrates/biome_grit_patterns/tests/specs/tsx/jsx_attribute_slot_names.tsxxtask/codegen/src/generate_grit_mappings.rs
|
@dyc3 yeah, this was intended. I prefer not to review AI generated PRs though, so I hope you can finish this one without my involvement. |
dyc3
force-pushed
the
dyc3/grit-native-node-field-names
branch
from
4ab3417 to
aea0fef
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
xtask/codegen/src/generate_grit_mappings.rs (1)
118-159:⚠️ Potential issue | 🟠 MajorTeach codegen about the JSON aliases before xtask bins them.
crates/biome_grit_patterns/src/grit_target_language/json_target_language/generated_mappings.rsLines 95-107 now accept aliases such asjson_root,json_member_name, andkey, but this generator can only emit onenameperTreeSitterPattern. The next regeneration will drop those arms and quietly regress JSON queries. Please encode aliases in the generator so the generated file stays reproducible.💡 Sketch
struct TreeSitterPattern { name: &'static str, + aliases: &'static [&'static str], biome_kind: &'static str, slots: &'static [(&'static str, u32)], } - .map(|p| { - format!( - r#" "{}" => Some({syntax_kind_type}::{}),"#, - p.name, - p.biome_kind, - syntax_kind_type = lang.syntax_kind_type - ) - }) + .flat_map(|p| { + std::iter::once(p.name) + .chain(p.aliases.iter().copied()) + .map(move |name| { + format!( + r#" "{}" => Some({syntax_kind_type}::{}),"#, + name, + p.biome_kind, + syntax_kind_type = lang.syntax_kind_type + ) + }) + })Then add the JSON aliases to
JSON_TREESITTER_PATTERNS— including aJSON_MEMBER_NAMEentry forproperty_name/key.Also applies to: 251-255, 405-451
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xtask/codegen/src/generate_grit_mappings.rs` around lines 118 - 159, The generator currently emits only a single name per TreeSitterPattern which drops JSON aliases (e.g., json_root, json_member_name, key) and causes non-reproducible generated_mappings.rs; update the code that builds legacy_kind_mappings / legacy_kind_by_name so it iterates over all alias names for each pattern (not just p.name) and emits a match arm for each alias, and also update the JSON source data used by the generator (JSON_TREESITTER_PATTERNS and add JSON_MEMBER_NAME entries) so the aliases (property_name/key, json_root, json_member_name, etc.) are present for generation; locate and change the mapping construction in generate_grit_mappings.rs (symbols legacy_kind_mappings and legacy_kind_by_name) to loop through pattern.aliases (or the equivalent field) and format a separate '"alias" => Some({syntax_kind_type}::{biome_kind}),' arm for each alias.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@xtask/codegen/src/generate_grit_mappings.rs`:
- Around line 118-159: The generator currently emits only a single name per
TreeSitterPattern which drops JSON aliases (e.g., json_root, json_member_name,
key) and causes non-reproducible generated_mappings.rs; update the code that
builds legacy_kind_mappings / legacy_kind_by_name so it iterates over all alias
names for each pattern (not just p.name) and emits a match arm for each alias,
and also update the JSON source data used by the generator
(JSON_TREESITTER_PATTERNS and add JSON_MEMBER_NAME entries) so the aliases
(property_name/key, json_root, json_member_name, etc.) are present for
generation; locate and change the mapping construction in
generate_grit_mappings.rs (symbols legacy_kind_mappings and legacy_kind_by_name)
to loop through pattern.aliases (or the equivalent field) and format a separate
'"alias" => Some({syntax_kind_type}::{biome_kind}),' arm for each alias.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: be573e67-9b19-4a99-b1ee-899faca5776c
⛔ Files ignored due to path filters (11)
crates/biome_grit_patterns/tests/specs/css/declaration_slot_names.snapis excluded by!**/*.snapand included by**crates/biome_grit_patterns/tests/specs/css/function_slot_names.snapis excluded by!**/*.snapand included by**crates/biome_grit_patterns/tests/specs/css/native_slot_names.snapis excluded by!**/*.snapand included by**crates/biome_grit_patterns/tests/specs/css/selector_slot_names.snapis excluded by!**/*.snapand included by**crates/biome_grit_patterns/tests/specs/ts/if_slot_names.snapis excluded by!**/*.snapand included by**crates/biome_grit_patterns/tests/specs/ts/legacy_conditional_slots.snapis excluded by!**/*.snapand included by**crates/biome_grit_patterns/tests/specs/ts/member_slot_names.snapis excluded by!**/*.snapand included by**crates/biome_grit_patterns/tests/specs/ts/mixed_slot_names.snapis excluded by!**/*.snapand included by**crates/biome_grit_patterns/tests/specs/ts/native_slot_aliases.snapis excluded by!**/*.snapand included by**crates/biome_grit_patterns/tests/specs/ts/while_slot_names.snapis excluded by!**/*.snapand included by**crates/biome_grit_patterns/tests/specs/tsx/jsx_attribute_slot_names.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (33)
.changeset/thirty-lines-sit.md.claude/skills/changeset/SKILL.mdcrates/biome_grit_patterns/src/grit_target_language.rscrates/biome_grit_patterns/src/grit_target_language/css_target_language.rscrates/biome_grit_patterns/src/grit_target_language/css_target_language/generated_mappings.rscrates/biome_grit_patterns/src/grit_target_language/js_target_language.rscrates/biome_grit_patterns/src/grit_target_language/js_target_language/generated_mappings.rscrates/biome_grit_patterns/src/grit_target_language/json_target_language.rscrates/biome_grit_patterns/src/grit_target_language/json_target_language/generated_mappings.rscrates/biome_grit_patterns/src/pattern_compiler/node_like_compiler.rscrates/biome_grit_patterns/tests/specs/css/declaration_slot_names.csscrates/biome_grit_patterns/tests/specs/css/declaration_slot_names.gritcrates/biome_grit_patterns/tests/specs/css/function_slot_names.csscrates/biome_grit_patterns/tests/specs/css/function_slot_names.gritcrates/biome_grit_patterns/tests/specs/css/native_slot_names.csscrates/biome_grit_patterns/tests/specs/css/native_slot_names.gritcrates/biome_grit_patterns/tests/specs/css/selector_slot_names.csscrates/biome_grit_patterns/tests/specs/css/selector_slot_names.gritcrates/biome_grit_patterns/tests/specs/ts/if_slot_names.gritcrates/biome_grit_patterns/tests/specs/ts/if_slot_names.tscrates/biome_grit_patterns/tests/specs/ts/legacy_conditional_slots.gritcrates/biome_grit_patterns/tests/specs/ts/legacy_conditional_slots.tscrates/biome_grit_patterns/tests/specs/ts/member_slot_names.gritcrates/biome_grit_patterns/tests/specs/ts/member_slot_names.tscrates/biome_grit_patterns/tests/specs/ts/mixed_slot_names.gritcrates/biome_grit_patterns/tests/specs/ts/mixed_slot_names.tscrates/biome_grit_patterns/tests/specs/ts/native_slot_aliases.gritcrates/biome_grit_patterns/tests/specs/ts/native_slot_aliases.tscrates/biome_grit_patterns/tests/specs/ts/while_slot_names.gritcrates/biome_grit_patterns/tests/specs/ts/while_slot_names.tscrates/biome_grit_patterns/tests/specs/tsx/jsx_attribute_slot_names.gritcrates/biome_grit_patterns/tests/specs/tsx/jsx_attribute_slot_names.tsxxtask/codegen/src/generate_grit_mappings.rs
✅ Files skipped from review due to trivial changes (24)
- crates/biome_grit_patterns/tests/specs/ts/legacy_conditional_slots.grit
- crates/biome_grit_patterns/tests/specs/ts/native_slot_aliases.ts
- crates/biome_grit_patterns/tests/specs/ts/if_slot_names.ts
- .claude/skills/changeset/SKILL.md
- crates/biome_grit_patterns/tests/specs/css/function_slot_names.css
- crates/biome_grit_patterns/tests/specs/css/selector_slot_names.grit
- crates/biome_grit_patterns/tests/specs/css/selector_slot_names.css
- crates/biome_grit_patterns/tests/specs/css/native_slot_names.css
- crates/biome_grit_patterns/tests/specs/ts/member_slot_names.ts
- .changeset/thirty-lines-sit.md
- crates/biome_grit_patterns/tests/specs/css/function_slot_names.grit
- crates/biome_grit_patterns/tests/specs/ts/native_slot_aliases.grit
- crates/biome_grit_patterns/tests/specs/ts/member_slot_names.grit
- crates/biome_grit_patterns/tests/specs/css/declaration_slot_names.grit
- crates/biome_grit_patterns/tests/specs/ts/while_slot_names.ts
- crates/biome_grit_patterns/tests/specs/css/declaration_slot_names.css
- crates/biome_grit_patterns/tests/specs/ts/legacy_conditional_slots.ts
- crates/biome_grit_patterns/tests/specs/ts/mixed_slot_names.ts
- crates/biome_grit_patterns/tests/specs/ts/if_slot_names.grit
- crates/biome_grit_patterns/tests/specs/tsx/jsx_attribute_slot_names.tsx
- crates/biome_grit_patterns/tests/specs/css/native_slot_names.grit
- crates/biome_grit_patterns/tests/specs/tsx/jsx_attribute_slot_names.grit
- crates/biome_grit_patterns/tests/specs/ts/mixed_slot_names.grit
- crates/biome_grit_patterns/tests/specs/ts/while_slot_names.grit
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/biome_grit_patterns/src/grit_target_language/css_target_language.rs
- crates/biome_grit_patterns/src/grit_target_language/json_target_language.rs
- crates/biome_grit_patterns/src/grit_target_language/js_target_language.rs
dyc3
force-pushed
the
dyc3/grit-native-node-field-names
branch
from
dd3beb2 to
80e2dfe
Compare
dyc3
force-pushed
the
dyc3/grit-native-node-field-names
branch
from
80e2dfe to
1eca78b
Compare
ematipico
left a comment
ematipico
left a comment
There was a problem hiding this comment.
This doesn't feel like a bug fix. This PR unblocks new things that users weren't able able to do before.
It needs a more descriptive changeset. This doesn't affect only the search command, but plugins too. Also, we need to update the documentation, and provide new examples.
Ironically, the changset mentions the search command, but no tests were added. We need tests for it too.
| /// Returns the native Biome slot mappings for a node name. | ||
| pub fn native_slots_for_name(node_name: &str) -> &'static [(&'static str, u32)] { | ||
| match node_name { | ||
| "CssAtRule" => &[("rule", 1)], |
There was a problem hiding this comment.
Can you point to me where the indexes are used?
There was a problem hiding this comment.
Essentially, it eventually gets wired into GritNodePatternArg::new(slot_index, pattern).
My understanding is that every node has slots, and some of them can be replaced with grit metavariables. During execution the index is used to look up which child is being referenced.
While I somewhat agree, we tell our users to refer to the
There are plenty of snapshot tests that prove these new queries can compile and execute. I can have the agent make more snapshot tests for this, but it's probably going to be low signal. Are you saying we need cli tests? We already know the plumbing of search and plugins works. |
dyc3
force-pushed
the
dyc3/grit-native-node-field-names
branch
from
db9cdbb to
330c6b5
Compare
Yes, at least one so that we prove that the diagnostics correctly map to the new nodes. As for the entity of the change: that's fine, but I hope you agree with me that we need new docs, since we're enabling new queries that before weren't possible. There's little to no value saying "we fixed this bug" in the changset without properly showing what users can do. And that's why, to me is a new feature. However, I don't want to rush this PR as a |
|
Yup, agree completely. I'll have a docs pr up soon. |
|
done biomejs/website#4129 |
dyc3
force-pushed
the
dyc3/grit-native-node-field-names
branch
from
330c6b5 to
5f77a1f
Compare
3 participants
Summary
This makes it so that users can use our field names in grit queries for things like:
generated by gpt 5.4, but had a really hard time wrangling the agent for this because its more of an added feature rather than a fix.
Essentially, we now grab the
AstSrcfrom the parsed.ungramfile to generate the metadata.#9734 brought it to my attention.
Test Plan
added snapshot tests
Docs
biomejs/website#4129