fix(maestro): enrich template component intellisense#2736
Merged
Conversation
📝 WalkthroughWalkthroughAdds LSP diagnostics for missing required component props with service wiring and tests, enriches hover output for component props/tags with usage examples and doc links, and introduces token-aware text-edit completions for open tags, directives, and events, with refactored/added tests. ChangesRequired Component Props Diagnostics
Hover Enhancements for Component Props and Tags
Completion Text-Edit and Documentation Improvements
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant DiagnosticService
participant Collector as collect_component_required_prop_diagnostics
participant Template as SFC Template Parser
Client->>DiagnosticService: collect(uri, content)
DiagnosticService->>Collector: collect_component_required_prop_diagnostics(state, uri, content, descriptor)
Collector->>Template: parse template and analyze usages
Template-->>Collector: component usages
Collector->>Collector: resolve props, check missing required
Collector-->>DiagnosticService: diagnostics list
DiagnosticService-->>Client: aggregated diagnostics
sequenceDiagram
participant Editor
participant CompleteTemplate as complete_template
participant PrepareEdits as prepare_open_tag_completions
participant Replacement as replacement_text_for_open_tag_completion
Editor->>CompleteTemplate: request completion at cursor
CompleteTemplate->>PrepareEdits: prepare_open_tag_completions(ctx, tag_ctx, items)
PrepareEdits->>Replacement: compute replacement text for token
Replacement-->>PrepareEdits: normalized new_text
PrepareEdits->>PrepareEdits: build CompletionTextEdit::Edit with range
PrepareEdits-->>CompleteTemplate: items with text edits
CompleteTemplate-->>Editor: completion response
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
vize
@vizejs/fresco
@vizejs/musea-mcp-server
oxlint-plugin-vize
@vizejs/rspack-plugin
@vizejs/unplugin
@vizejs/vite-plugin
@vizejs/vite-plugin-musea
@vizejs/musea-nuxt
@vizejs/nuxt
commit: |
Contributor
PR BenchmarkBase:
Raw run timesCompile SFC
Lint
Format
Type check (1T)
Type check (max)
|
Contributor
Detailed Test ReportCommit: Area Summary
Test InventoryTotal tracked cases: 8628 across 1213 files.
Files
Comment truncated at 64000 characters. Open the workflow run for the full job log. |
ubugeeei
force-pushed
the
fix/template-component-intellisense
branch
from
d4c01b8 to
a732177
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/vize_maestro/src/ide/diagnostics/collectors.rs`:
- Around line 47-58: The diagnostics collector in collect() is re-parsing the
same template in collect_template_diagnostics and again in the Drawer flow,
doubling work on the typecheck-enabled path. Refactor the collectors in
collectors.rs so the already-parsed AST or the finished Drawer/croquis from
collect_template_diagnostics is threaded into the template diagnostics
collection instead of calling vize_armature::parse a second time, and reuse the
existing Bump/Drawer state where possible.
- Around line 670-672: The duplicated prop_names_match helper in the diagnostics
collector should be consolidated into a shared helper so its matching behavior
stays consistent with hover/component_tag.rs. Move the logic into
crate::ide::definition::helpers alongside kebab_to_camel, then update both
collectors to call the shared function instead of keeping separate copies. Use
the existing prop_names_match references in collectors.rs and
hover/component_tag.rs to replace the local implementations with the shared
helper.
In `@crates/vize_maestro/src/ide/hover/component_prop.rs`:
- Around line 30-40: Extract the duplicated Vue docs label and URL used in
`hover_component_prop` into a shared constant or small helper so it can be
reused by `hover_component_tag` in `component_tag.rs` as well. Update the
`.link(...)` call in this builder to reference the shared symbol instead of
hardcoding `"Vue Component Props"` and the Vue props URL, keeping both hover
paths consistent and preventing drift if the docs change.
In `@crates/vize_maestro/src/ide/hover/component_tag.rs`:
- Around line 85-96: Add hover test coverage for the new required-props
rendering paths in hover_component_tag so both the “Required props” bullet
output and the vue missing-required example are exercised; update or add tests
alongside the existing component hover tests to assert these branches. Also
remove the duplicated "Vue Component Props" link literal by reusing the shared
link constant used in component_prop.rs so the label/URL stay consistent in one
place.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 78c22428-7164-44f9-b277-4e0965296cdb
📒 Files selected for processing (12)
crates/vize_maestro/src/ide/completion/component_props_tests.rscrates/vize_maestro/src/ide/completion/items.rscrates/vize_maestro/src/ide/completion/template.rscrates/vize_maestro/src/ide/completion/template/directives.rscrates/vize_maestro/src/ide/completion/template/tag_context.rscrates/vize_maestro/src/ide/completion/tests.rscrates/vize_maestro/src/ide/diagnostics.rscrates/vize_maestro/src/ide/diagnostics/collectors.rscrates/vize_maestro/src/ide/diagnostics/service.rscrates/vize_maestro/src/ide/diagnostics/tests.rscrates/vize_maestro/src/ide/hover/component_prop.rscrates/vize_maestro/src/ide/hover/component_tag.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (9)
- GitHub Check: dialect-guard
- GitHub Check: clippy-and-test
- GitHub Check: vue-parity
- GitHub Check: criterion-ab
- GitHub Check: build-js-packages
- GitHub Check: test-js-packages
- GitHub Check: test-scripts
- GitHub Check: pr-benchmark
- GitHub Check: tool-benchmark
🧰 Additional context used
📓 Path-based instructions (1)
crates/**
⚙️ CodeRabbit configuration file
crates/**: Focus on parser/compiler correctness, source locations, UTF-8/UTF-16 offset handling, and panic-free LSP behavior. Flag changes that only work for small fixtures but break real Vue/Nuxt projects.
Files:
crates/vize_maestro/src/ide/diagnostics.rscrates/vize_maestro/src/ide/hover/component_prop.rscrates/vize_maestro/src/ide/completion/tests.rscrates/vize_maestro/src/ide/diagnostics/tests.rscrates/vize_maestro/src/ide/completion/template/directives.rscrates/vize_maestro/src/ide/diagnostics/service.rscrates/vize_maestro/src/ide/completion/component_props_tests.rscrates/vize_maestro/src/ide/completion/items.rscrates/vize_maestro/src/ide/diagnostics/collectors.rscrates/vize_maestro/src/ide/completion/template/tag_context.rscrates/vize_maestro/src/ide/hover/component_tag.rscrates/vize_maestro/src/ide/completion/template.rs
🔇 Additional comments (18)
crates/vize_maestro/src/ide/hover/component_prop.rs (1)
144-148: LGTM!crates/vize_maestro/src/ide/hover/component_tag.rs (2)
50-65: LGTM!
234-241: LGTM!crates/vize_maestro/src/ide/completion/items.rs (3)
12-13: LGTM!
94-114: LGTM!
116-123: 🎯 Functional CorrectnessShorthand directive labels are already used.
crates/vize_maestro/src/ide/completion/template/directives.rspasses"@",":", and"#"intodirective_item, so the shorthand branches indirective_completion_kindare reachable and the icon mapping is intentional.> Likely an incorrect or invalid review comment.crates/vize_maestro/src/ide/completion/template/directives.rs (2)
4-5: LGTM!
56-75: 🎯 Functional CorrectnessNo issue: event shorthand completions already get replacement text edits in open tags.
contextual_directive_completions()is wrapped byprepare_open_tag_completions(), soevent_shorthand_completions()items also pass throughwith_open_tag_text_edits()and replace the typed prefix instead of duplicating it.> Likely an incorrect or invalid review comment.crates/vize_maestro/src/ide/diagnostics.rs (1)
43-43: LGTM!crates/vize_maestro/src/ide/diagnostics/collectors.rs (2)
12-46: LGTM!Also applies to: 86-116
64-83: 🚀 Performance & ScalabilityNo issue:
component_metadatais already cached per file. It reusesServerState::component_metadata_cachekeyed by resolved path and invalidates on file length/mtime changes, so repeated component usages here do not cause repeated disk reads.> Likely an incorrect or invalid review comment.crates/vize_maestro/src/ide/diagnostics/service.rs (1)
154-167: LGTM!crates/vize_maestro/src/ide/diagnostics/tests.rs (1)
257-361: LGTM!crates/vize_maestro/src/ide/completion/template/tag_context.rs (1)
7-13: LGTM!Also applies to: 44-55, 79-93
crates/vize_maestro/src/ide/completion/template.rs (2)
23-23: LGTM!Also applies to: 40-86, 103-110, 139-169, 186-208, 216-226
112-137: 🩺 Stability & AvailabilityNo clamp needed here The range helper already bounds
offsettocontent.len(), soctx.offsetcan’t drive an out-of-range position lookup.> Likely an incorrect or invalid review comment.crates/vize_maestro/src/ide/completion/component_props_tests.rs (1)
3-3: LGTM!Also applies to: 50-101, 193-203
crates/vize_maestro/src/ide/completion/tests.rs (1)
9-11: LGTM!Also applies to: 559-597
Comment on lines
+47
to
+58
| let allocator = vize_carton::Bump::new(); | ||
| let (root, _) = vize_armature::parse(&allocator, template_content); | ||
| let mut drawer = Drawer::with_options(DrawerOptions { | ||
| analyze_template_scopes: true, | ||
| track_usage: true, | ||
| ..Default::default() | ||
| }); | ||
| drawer.draw_template(&root); | ||
| let croquis = drawer.finish(); | ||
| if croquis.component_usages.is_empty() { | ||
| return Vec::new(); | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy lift
Template is re-parsed here even though collect_template_diagnostics already parses the same template in the same collect() pass.
Every call to collect() with features.typecheck enabled now parses the template with vize_armature::parse twice (once for parser diagnostics, once here for the Drawer), plus builds a fresh Drawer/Bump allocator. For large real-world templates (as opposed to small fixtures), this doubles per-keystroke parse cost on the typecheck-enabled path.
Consider threading the already-parsed AST (or the Drawer/croquis result) from collect_template_diagnostics into this collector instead of re-parsing, to keep the diagnostics pipeline scaling with real Vue/Nuxt-sized templates.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/vize_maestro/src/ide/diagnostics/collectors.rs` around lines 47 - 58,
The diagnostics collector in collect() is re-parsing the same template in
collect_template_diagnostics and again in the Drawer flow, doubling work on the
typecheck-enabled path. Refactor the collectors in collectors.rs so the
already-parsed AST or the finished Drawer/croquis from
collect_template_diagnostics is threaded into the template diagnostics
collection instead of calling vize_armature::parse a second time, and reuse the
existing Bump/Drawer state where possible.
Comment on lines
+670
to
+672
| fn prop_names_match(passed_name: &str, declared_name: &str) -> bool { | ||
| passed_name == declared_name || helpers::kebab_to_camel(passed_name) == declared_name | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Duplicate prop_names_match helper.
This is structurally identical to prop_names_match already defined in hover/component_tag.rs. The doc comment on this collector (line 31-32) explicitly states it "uses the same component metadata and usage analysis as component hover" — keeping two copies risks the matching semantics silently drifting apart (e.g. if kebab/camel handling is extended in one copy but not the other).
♻️ Suggested consolidation
Move prop_names_match into crate::ide::definition::helpers (alongside kebab_to_camel, which is already imported here) and have both hover/component_tag.rs and this collector call the shared helper.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/vize_maestro/src/ide/diagnostics/collectors.rs` around lines 670 -
672, The duplicated prop_names_match helper in the diagnostics collector should
be consolidated into a shared helper so its matching behavior stays consistent
with hover/component_tag.rs. Move the logic into crate::ide::definition::helpers
alongside kebab_to_camel, then update both collectors to call the shared
function instead of keeping separate copies. Use the existing prop_names_match
references in collectors.rs and hover/component_tag.rs to replace the local
implementations with the shared helper.
Comment on lines
+30
to
+40
| .section("Requirement", requirement) | ||
| .code( | ||
| "vue", | ||
| &format!( | ||
| "<{component_name} {attr_name}=\"...\" />\n<{component_name} :{attr_name}=\"value\" />" | ||
| ), | ||
| ) | ||
| .link( | ||
| "Vue Component Props", | ||
| "https://vuejs.org/guide/components/props.html", | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Consider extracting the shared Vue docs link into a constant.
The "Vue Component Props" / "https://vuejs.org/guide/components/props.html" pair is duplicated verbatim in component_tag.rs (hover_component_tag). A shared const/helper would avoid drift if the URL or label ever changes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/vize_maestro/src/ide/hover/component_prop.rs` around lines 30 - 40,
Extract the duplicated Vue docs label and URL used in `hover_component_prop`
into a shared constant or small helper so it can be reused by
`hover_component_tag` in `component_tag.rs` as well. Update the `.link(...)`
call in this builder to reference the shared symbol instead of hardcoding `"Vue
Component Props"` and the Vue props URL, keeping both hover paths consistent and
preventing drift if the docs change.
Comment on lines
+85
to
+96
| if !usage.has_spread_attrs { | ||
| builder = builder.code( | ||
| "vue", | ||
| &missing_required_example(tag_name.as_str(), &missing_required), | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| builder = builder.link( | ||
| "Vue Component Props", | ||
| "https://vuejs.org/guide/components/props.html", | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add hover test coverage for the new required-props sections.
This adds two new rendering branches (Required props bullets and the vue missing-required example) to hover_component_tag, but no accompanying test exercises them (unlike the sibling component_prop.rs change, which did add assertions). A regression here would go unnoticed.
Also duplicates the "Vue Component Props" link constant already flagged in component_prop.rs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/vize_maestro/src/ide/hover/component_tag.rs` around lines 85 - 96, Add
hover test coverage for the new required-props rendering paths in
hover_component_tag so both the “Required props” bullet output and the vue
missing-required example are exercised; update or add tests alongside the
existing component hover tests to assert these branches. Also remove the
duplicated "Vue Component Props" link literal by reusing the shared link
constant used in component_prop.rs so the label/URL stay consistent in one
place.
Contributor
PR BenchmarkBase:
Raw run timesCompile SFC
Lint
Format
Type check (1T)
Type check (max)
|
Contributor
Detailed Test ReportCommit: Area Summary
Test InventoryTotal tracked cases: 8628 across 1215 files.
Files
Comment truncated at 64000 characters. Open the workflow run for the full job log. |
Contributor
Tool BenchmarkMeasured: 2026-07-08T19:51:51.879Z
Fairness notes:
Commands: gh workflow run tool-benchmark.yml --ref <branch> -f file_count=3000 -f check_file_count=500 -f vite_file_count=1000 -f nuxt_file_count=250 -f large_blocks=300 -f runs=3 -f warmups=1 -f commit_results=true
node bench/generate.mjs 3000
node bench/compare-tools.mjs --input bench/__in__ --vize-bin target/release/vize --runs 3 --warmups 1 --check-file-count 500 --vite-file-count 1000 --nuxt-file-count 250 --large-blocks 300 --runner-label "blacksmith-32vcpu-ubuntu-2404" --out tool-benchmark-summary.md --json tool-benchmark-results.json --doc performance-blacksmith.mdVariant details and raw run timesSFC compile
Large SFC compile
Large SFC type check
Lint
Format
Type check
Vite build (end-to-end)
Nuxt SPA build (end-to-end)
|
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/vize_maestro/src/ide/completion/template/tag_context.rs (1)
79-93: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse Unicode whitespace for open-tag token boundaries.
is_ascii_whitespace()misses cases like NBSP, so the token scan can span multiple attributes whiletrim_start()hides only the leading space. That breaks completion prefix matching and can widen the LSP replace range.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/vize_maestro/src/ide/completion/template/tag_context.rs` around lines 79 - 93, The open-tag token scan in current_open_tag_token should use Unicode-aware whitespace boundaries instead of is_ascii_whitespace(), since ASCII-only checks miss characters like NBSP and can cause the token to span too far. Update the loop over slice.char_indices() to treat any Unicode whitespace as a boundary, and keep the token start aligned with that same rule so content[token_start..cursor] and the returned prefix stay consistent. Make sure the behavior around '<' in current_open_tag_token still works the same while preventing completion prefix and replace-range widening.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/vize_maestro/src/ide/completion/template_event_tests.rs`:
- Around line 60-76: The helper functions completion_labels, completion_items,
and has_label in template_event_tests duplicate the identical test utilities
already defined in component_props_tests. Extract these shared helpers into a
common test-support module (for example, a cfg(test) module under the completion
tests) and update both test files to import them instead of keeping separate
copies. Use the existing function names as the shared API so the test assertions
stay unchanged and future completion tests can reuse the same utilities.
In `@crates/vize_maestro/src/ide/diagnostics/component_props.rs`:
- Around line 100-119: The static missing-required-props diagnostic in
component_props.rs is too aggressive as an ERROR for cases that can be satisfied
at runtime or through higher-order patterns. Update the Diagnostic construction
in the required-props reporting path to use a less severe level such as WARNING
or HINT, or add a suppression mechanism for known false positives, so the logic
around required_props_message, has_spread_attrs, and the component props check
does not produce hard red squiggles for legitimate Vue/Nuxt component patterns.
- Around line 54-62: The template is being parsed and analyzed a second time in
the diagnostics flow, duplicating work already done by
collect_template_diagnostics. Update the component_props diagnostics path to
reuse the already-parsed AST or Croquis produced earlier by collect() instead of
calling vize_armature::parse and running a new
Drawer::with_options/draw_template pass again. Keep the existing behavior for
parse-error short-circuiting, but thread the prior result through the
diagnostics collector so large templates are not reprocessed on every keystroke.
- Around line 71-93: The diagnostics loop in component_props.rs resolves
component metadata repeatedly for the same component name, which causes
redundant lookups when croquis.component_usages contains multiple instances of
one component. Add memoization within the loop around component_metadata so
repeated usage.name.as_str() values reuse the previously resolved metadata
instead of calling component_metadata again, and keep the existing required-prop
missing check logic in place using the cached result.
- Line 2: The module-level clippy allow in component_props currently suppresses
legitimate uses of disallowed methods/macros across the whole file. Narrow the
allowance by moving the allow attributes to the specific functions or statements
in component_props, especially around content.to_string(),
"component-required-props".to_string(), sources::COMPONENTS.to_string(), and
required_props_message, so future additions still trigger clippy.
---
Outside diff comments:
In `@crates/vize_maestro/src/ide/completion/template/tag_context.rs`:
- Around line 79-93: The open-tag token scan in current_open_tag_token should
use Unicode-aware whitespace boundaries instead of is_ascii_whitespace(), since
ASCII-only checks miss characters like NBSP and can cause the token to span too
far. Update the loop over slice.char_indices() to treat any Unicode whitespace
as a boundary, and keep the token start aligned with that same rule so
content[token_start..cursor] and the returned prefix stay consistent. Make sure
the behavior around '<' in current_open_tag_token still works the same while
preventing completion prefix and replace-range widening.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 5aaeaf9b-87f9-46b6-b31a-b5319191de9a
📒 Files selected for processing (13)
crates/vize_maestro/src/ide/completion.rscrates/vize_maestro/src/ide/completion/component_props_tests.rscrates/vize_maestro/src/ide/completion/items.rscrates/vize_maestro/src/ide/completion/template.rscrates/vize_maestro/src/ide/completion/template/directives.rscrates/vize_maestro/src/ide/completion/template/tag_context.rscrates/vize_maestro/src/ide/completion/template_event_tests.rscrates/vize_maestro/src/ide/diagnostics.rscrates/vize_maestro/src/ide/diagnostics/component_props.rscrates/vize_maestro/src/ide/diagnostics/component_props_tests.rscrates/vize_maestro/src/ide/diagnostics/service.rscrates/vize_maestro/src/ide/hover/component_prop.rscrates/vize_maestro/src/ide/hover/component_tag.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (8)
- GitHub Check: pr-benchmark-comment
- GitHub Check: pr-benchmark-budget
- GitHub Check: playground-test
- GitHub Check: dialect-guard
- GitHub Check: criterion-ab
- GitHub Check: tool-benchmark
- GitHub Check: test-scripts
- GitHub Check: clippy-and-test
🧰 Additional context used
📓 Path-based instructions (1)
crates/**
⚙️ CodeRabbit configuration file
crates/**: Focus on parser/compiler correctness, source locations, UTF-8/UTF-16 offset handling, and panic-free LSP behavior. Flag changes that only work for small fixtures but break real Vue/Nuxt projects.
Files:
crates/vize_maestro/src/ide/completion.rscrates/vize_maestro/src/ide/completion/template_event_tests.rscrates/vize_maestro/src/ide/completion/template/directives.rscrates/vize_maestro/src/ide/diagnostics.rscrates/vize_maestro/src/ide/completion/items.rscrates/vize_maestro/src/ide/diagnostics/component_props_tests.rscrates/vize_maestro/src/ide/completion/template/tag_context.rscrates/vize_maestro/src/ide/hover/component_prop.rscrates/vize_maestro/src/ide/hover/component_tag.rscrates/vize_maestro/src/ide/diagnostics/component_props.rscrates/vize_maestro/src/ide/completion/template.rscrates/vize_maestro/src/ide/diagnostics/service.rscrates/vize_maestro/src/ide/completion/component_props_tests.rs
🔇 Additional comments (20)
crates/vize_maestro/src/ide/hover/component_prop.rs (2)
37-40: 📐 Maintainability & Code QualityDuplicated Vue docs link literal.
Same
"Vue Component Props"/ URL pair duplicated incomponent_tag.rs. Previously flagged for extraction into a shared constant.
144-148: LGTM!crates/vize_maestro/src/ide/hover/component_tag.rs (2)
69-80: 📐 Maintainability & Code QualityMissing test coverage for required-props hover branches; duplicated link literal.
No test exercises the new "Required props" example/link branches here (unlike
component_prop.rs). Also duplicates the"Vue Component Props"link literal already flagged incomponent_prop.rs.
218-225: LGTM!crates/vize_maestro/src/ide/completion/items.rs (3)
12-13: LGTM!
94-114: LGTM!
116-123: 🎯 Functional CorrectnessNo change needed for directive kinds.
directive_itemonly receives bare directive shorthands (v-on,v-bind,v-model,v-slot) and shorthand symbols (@,:,#); compound labels are handled byevent_itemelsewhere.> Likely an incorrect or invalid review comment.crates/vize_maestro/src/ide/completion/template/directives.rs (2)
4-5: LGTM!
61-71: LGTM!crates/vize_maestro/src/ide/diagnostics.rs (1)
12-14: LGTM!Also applies to: 46-46
crates/vize_maestro/src/ide/diagnostics/component_props.rs (2)
126-146: LGTM!
95-119: 🩺 Stability & AvailabilityNo issue: the component span and line mapping are already safe
usage.startis the byte offset of the element start, so+ 1correctly lands on the tag name.LineIndex::line_colalso accepts any in-bounds byte offset and handles mid-character offsets without panicking, so the UTF-8 panic concern doesn’t apply here.> Likely an incorrect or invalid review comment.crates/vize_maestro/src/ide/diagnostics/service.rs (1)
142-150: Wiring is correctly placed after the parse-error short-circuit.Gating on
features.typecheck(rather thanfeatures.lint) is a deliberate, tested choice (seecomponent_props_tests.rs), and the call happens after script/template parse errors already return early, so the collector only runs on a structurally valid template.crates/vize_maestro/src/ide/diagnostics/component_props_tests.rs (1)
22-125: LGTM!Tests clearly cover the happy path (missing required prop reported with correct source/code/message/range) and the spread-attrs suppression case.
crates/vize_maestro/src/ide/completion/template/tag_context.rs (1)
8-13: LGTM!Also applies to: 44-55
crates/vize_maestro/src/ide/completion/template.rs (2)
23-23: LGTM!Also applies to: 55-66, 103-110, 139-169, 171-185, 216-226
112-137: 🎯 Functional CorrectnessSet
filter_texton open-tag completions. These items only rewriteinsert_text, so some LSP clients can re-filter on the edited:prop/@eventprefix and drop the suggestion as typing continues.crates/vize_maestro/src/ide/completion/component_props_tests.rs (1)
3-3: LGTM!Also applies to: 50-101, 193-204
crates/vize_maestro/src/ide/completion/template_event_tests.rs (1)
1-48: LGTM!crates/vize_maestro/src/ide/completion.rs (1)
25-27: 🎯 Functional CorrectnessNo issue:
template_event_testsis already test-gated.#[cfg(test)]applies to the following module item here, somod template_event_tests;is not compiled in non-test builds.> Likely an incorrect or invalid review comment.
Comment on lines
+60
to
+76
| fn completion_labels(response: CompletionResponse) -> Vec<String> { | ||
| completion_items(response) | ||
| .into_iter() | ||
| .map(|item| item.label) | ||
| .collect() | ||
| } | ||
|
|
||
| fn completion_items(response: CompletionResponse) -> Vec<tower_lsp::lsp_types::CompletionItem> { | ||
| match response { | ||
| CompletionResponse::Array(items) => items, | ||
| CompletionResponse::List(list) => list.items, | ||
| } | ||
| } | ||
|
|
||
| fn has_label(labels: &[String], expected: &str) -> bool { | ||
| labels.iter().any(|label| label == expected) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Duplicate test helpers already present in component_props_tests.rs.
completion_labels, completion_items, and has_label here are identical, verbatim copies of the functions defined in crates/vize_maestro/src/ide/completion/component_props_tests.rs (Lines 192-208 there). Consider extracting these into a shared test-support module used by both files to avoid divergence as more completion test files get added.
♻️ Suggested direction
// e.g. crates/vize_maestro/src/ide/completion/test_support.rs (cfg(test) only)
pub(super) fn completion_items(response: CompletionResponse) -> Vec<CompletionItem> { ... }
pub(super) fn completion_labels(response: CompletionResponse) -> Vec<String> { ... }
pub(super) fn has_label(labels: &[String], expected: &str) -> bool { ... }Then use super::test_support::{completion_items, completion_labels, has_label}; from both test files.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/vize_maestro/src/ide/completion/template_event_tests.rs` around lines
60 - 76, The helper functions completion_labels, completion_items, and has_label
in template_event_tests duplicate the identical test utilities already defined
in component_props_tests. Extract these shared helpers into a common
test-support module (for example, a cfg(test) module under the completion tests)
and update both test files to import them instead of keeping separate copies.
Use the existing function names as the shared API so the test assertions stay
unchanged and future completion tests can reuse the same utilities.
| @@ -0,0 +1,146 @@ | |||
| //! Component-surface diagnostics that need template usage plus imported metadata. | |||
| #![allow(clippy::disallowed_methods, clippy::disallowed_macros)] | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check clippy.toml for what disallowed_methods/macros are configured, and see if any listed
# item is actually used in this file.
fd -e toml clippy --exec cat {}
rg -n 'disallowed-methods|disallowed-macros' -A5Repository: ubugeeei-prod/vize
Length of output: 915
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file and any nearby usage of the disallowed clippy items.
wc -l crates/vize_maestro/src/ide/diagnostics/component_props.rs
cat -n crates/vize_maestro/src/ide/diagnostics/component_props.rs
printf '\n-- usages in file --\n'
rg -n '\bformat!\b|\.to_string\(\)|disallowed_methods|disallowed_macros' crates/vize_maestro/src/ide/diagnostics/component_props.rs || true
printf '\n-- project-wide uses of disallowed items in this file path context --\n'
rg -n 'format!\(|\.to_string\(\)' crates/vize_maestro/src/ide/diagnostics/component_props.rs crates/vize_maestro/src/ide -g '!**/target/**' || trueRepository: ubugeeei-prod/vize
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file and any nearby usage of the disallowed clippy items.
wc -l crates/vize_maestro/src/ide/diagnostics/component_props.rs
cat -n crates/vize_maestro/src/ide/diagnostics/component_props.rs
printf '\n-- usages in file --\n'
rg -n '\bformat!\b|\.to_string\(\)|disallowed_methods|disallowed_macros' crates/vize_maestro/src/ide/diagnostics/component_props.rs || true
printf '\n-- project-wide uses of disallowed items in this file path context --\n'
rg -n 'format!\(|\.to_string\(\)' crates/vize_maestro/src/ide/component_props.rs crates/vize_maestro/src/ide -g '!**/target/**' || trueRepository: ubugeeei-prod/vize
Length of output: 50376
Narrow the clippy allow in crates/vize_maestro/src/ide/diagnostics/component_props.rs The module-level #![allow(...)] covers several actual format!/to_string() sites here (content.to_string(), "component-required-props".to_string(), sources::COMPONENTS.to_string(), and format! in required_props_message). If these calls are intentional, scope the allow to the specific statements/functions instead of the whole file so the lint still catches new additions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/vize_maestro/src/ide/diagnostics/component_props.rs` at line 2, The
module-level clippy allow in component_props currently suppresses legitimate
uses of disallowed methods/macros across the whole file. Narrow the allowance by
moving the allow attributes to the specific functions or statements in
component_props, especially around content.to_string(),
"component-required-props".to_string(), sources::COMPONENTS.to_string(), and
required_props_message, so future additions still trigger clippy.
Comment on lines
+54
to
+62
| let allocator = vize_carton::Bump::new(); | ||
| let (root, _) = vize_armature::parse(&allocator, template_content); | ||
| let mut drawer = Drawer::with_options(DrawerOptions { | ||
| analyze_template_scopes: true, | ||
| track_usage: true, | ||
| ..Default::default() | ||
| }); | ||
| drawer.draw_template(&root); | ||
| let croquis = drawer.finish(); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy lift
Template re-parsed independently of collect_template_diagnostics.
vize_armature::parse plus a full Drawer pass run again here even though service.rs's collect() already parsed and analyzed the same template a few lines earlier (collect_template_diagnostics) and short-circuits on parse errors before this call runs. For large real-world templates re-parsed on every keystroke, this doubles parse/analysis cost for this diagnostic pass alone (on top of script/template/sfc-compile parses already happening).
Longer-term, consider threading the already-parsed AST/Croquis result through instead of re-parsing, to keep collect() scaling on large SFCs.
As per path instructions, "Flag changes that only work for small fixtures but break real Vue/Nuxt projects" for crates/**.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/vize_maestro/src/ide/diagnostics/component_props.rs` around lines 54 -
62, The template is being parsed and analyzed a second time in the diagnostics
flow, duplicating work already done by collect_template_diagnostics. Update the
component_props diagnostics path to reuse the already-parsed AST or Croquis
produced earlier by collect() instead of calling vize_armature::parse and
running a new Drawer::with_options/draw_template pass again. Keep the existing
behavior for parse-error short-circuiting, but thread the prior result through
the diagnostics collector so large templates are not reprocessed on every
keystroke.
Source: Path instructions
Comment on lines
+71
to
+93
| for usage in croquis.component_usages { | ||
| if usage.has_spread_attrs { | ||
| continue; | ||
| } | ||
|
|
||
| let Some(metadata) = component_metadata(&metadata_ctx, usage.name.as_str()) else { | ||
| continue; | ||
| }; | ||
| let missing = metadata | ||
| .props | ||
| .iter() | ||
| .filter(|prop| prop.required) | ||
| .filter(|prop| { | ||
| !usage | ||
| .props | ||
| .iter() | ||
| .any(|passed| prop_names_match(passed.name.as_str(), prop.name.as_str())) | ||
| }) | ||
| .map(|prop| prop.name.clone()) | ||
| .collect::<Vec<_>>(); | ||
| if missing.is_empty() { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
No memoization of component_metadata across repeated usages of the same component.
component_metadata(&metadata_ctx, usage.name.as_str()) is resolved fresh for every usage in croquis.component_usages. Real templates frequently render the same component many times (e.g., inside v-for), so a list template with N instances of the same child component triggers N independent metadata resolutions (each potentially involving cross-file lookups/parsing) per diagnostics pass.
Consider caching resolved metadata by component name for the duration of this loop:
♻️ Suggested caching
+ let mut metadata_cache: std::collections::HashMap<&str, Option<_>> = Default::default();
for usage in croquis.component_usages {
if usage.has_spread_attrs {
continue;
}
- let Some(metadata) = component_metadata(&metadata_ctx, usage.name.as_str()) else {
+ let metadata = metadata_cache
+ .entry(usage.name.as_str())
+ .or_insert_with(|| component_metadata(&metadata_ctx, usage.name.as_str()));
+ let Some(metadata) = metadata else {
continue;
};(Exact types depend on component_metadata's return type — adjust as needed.)
As per path instructions, flag changes that "break real Vue/Nuxt projects" beyond small fixtures for crates/**.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/vize_maestro/src/ide/diagnostics/component_props.rs` around lines 71 -
93, The diagnostics loop in component_props.rs resolves component metadata
repeatedly for the same component name, which causes redundant lookups when
croquis.component_usages contains multiple instances of one component. Add
memoization within the loop around component_metadata so repeated
usage.name.as_str() values reuse the previously resolved metadata instead of
calling component_metadata again, and keep the existing required-prop missing
check logic in place using the cached result.
Source: Path instructions
Comment on lines
+100
to
+119
| diagnostics.push(Diagnostic { | ||
| range: Range { | ||
| start: Position { | ||
| line: start_line, | ||
| character: start_col, | ||
| }, | ||
| end: Position { | ||
| line: end_line, | ||
| character: end_col, | ||
| }, | ||
| }, | ||
| severity: Some(DiagnosticSeverity::ERROR), | ||
| code: Some(NumberOrString::String( | ||
| "component-required-props".to_string(), | ||
| )), | ||
| code_description: vue_props_code_description(), | ||
| source: Some(sources::COMPONENTS.to_string()), | ||
| message: required_props_message(usage.name.as_str(), &missing), | ||
| ..Default::default() | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
ERROR severity for statically-inferred missing required props risks false positives on real components.
Static template analysis cannot see every legitimate way a required prop can be satisfied at runtime: inheritAttrs: false + manual $attrs application in a render function, mixins/extends supplying props, provide/inject-based wrapper components, or a slot-forwarding higher-order component. The has_spread_attrs check only covers the v-bind="obj" case; it won't catch these other common design-system/HOC patterns seen in real Vue/Nuxt codebases.
Marking these as hard ERROR squiggles (rather than WARNING/HINT) means any component relying on such patterns will show persistent, unfixable-looking red diagnostics for correct code — a pattern the path guidance specifically warns against (fixture-only correctness that doesn't hold up on real projects).
Consider downgrading to DiagnosticSeverity::WARNING (or HINT) given the inherent false-positive surface of static analysis here, or provide an explicit suppression mechanism (e.g., a directive comment) for known false positives.
As per path instructions, "Flag changes that only work for small fixtures but break real Vue/Nuxt projects" for crates/**.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/vize_maestro/src/ide/diagnostics/component_props.rs` around lines 100
- 119, The static missing-required-props diagnostic in component_props.rs is too
aggressive as an ERROR for cases that can be satisfied at runtime or through
higher-order patterns. Update the Diagnostic construction in the required-props
reporting path to use a less severe level such as WARNING or HINT, or add a
suppression mechanism for known false positives, so the logic around
required_props_message, has_spread_attrs, and the component props check does not
produce hard red squiggles for legitimate Vue/Nuxt component patterns.
Source: Path instructions
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Validation
Note: cargo clippy -p vize_maestro --all-targets -- -D warnings still fails on existing test-target disallowed macro/type lints outside this change.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by CodeRabbit
New Features
Bug Fixes