Repository navigation
Give vite build mode component-scoped rule coverage (Correctness/Security/Architecture/PERF009-010) - #112
Conversation
Extract parseComponentFacts (+ its shared AST utilities) from the CLI package into @svelte-vitals/core so the vite plugin's build mode can add CORRECT001-004, SEC001-002, ARCH001-002, and PERF009-010 support without duplicating ~350 lines of AST-walking logic.
…rser Also fixes packages/cli/test/parse-file.test.ts's attrValueOf import, which the task brief's file list omitted but has the same dependency on the relocated svelte-ast helper. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015qoHchC2eWjmw2rdm91kLq
…dency Task 2 added `svelte` to packages/core/package.json but the lockfile update was never committed alongside it.
… label (task 6 review) ja docs mixed a bare English "component スコープ"/"componentルール" into Japanese prose instead of the コンポーネントスコープ convention already established in cli.md's suppression-directive section. Also replaced the one-off "Bundle-Performance rules" label (used nowhere else in the docs) with "component-scoped Performance rules", matching plugin-mode.md and choosing-a-package.md's existing wording for the same PERF009/PERF010 pair.
|
Warning Review limit reached
Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR moves shared component-facts parsing and Svelte AST helpers into ChangesComponent-facts relocation and Vite build-mode wiring
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 3
🧹 Nitpick comments (5)
packages/vite/src/providers/source/components.ts (1)
20-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated "empty ComponentFacts" default object.
This default fallback object duplicates the shape defined by
ComponentFactsin@svelte-vitals/core(and likely mirrors an equivalent default in the CLI's collector). Extracting a sharedemptyComponentFacts(file)helper (or default constant) in core would keep both packages in sync if the interface gains a field.🤖 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 `@packages/vite/src/providers/source/components.ts` around lines 20 - 32, The fallback “empty ComponentFacts” object is duplicated here and should be centralized to avoid drift from the shared ComponentFacts shape. Extract a shared helper or default factory such as emptyComponentFacts(file) in `@svelte-vitals/core`, then update the components.ts fallback to use it so this source and any CLI collector stay in sync if ComponentFacts changes.packages/vite/src/analyze.ts (1)
46-50: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueIndependent collectors run sequentially.
collectComponentFacts(cwd)doesn't depend onproject/htmlLang, so it could run concurrently withcollectRenderedProjectviaPromise.all. Given this runs once per build, the reward is marginal, so treating this as optional.🤖 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 `@packages/vite/src/analyze.ts` around lines 46 - 50, The collectors in analyze() are independent, and collectComponentFacts(cwd) does not depend on the htmlLang/project result. Update the analyze flow to run collectRenderedProject(cwd, htmlLang) and collectComponentFacts(cwd) concurrently with Promise.all, while keeping collectRenderedHeads(prerenderPagesDir) before them and preserving the same data passed into runRules and applyRuleSeverities.packages/core/test/svelte-ast.test.ts (1)
54-70: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a mixed-content regression test for
attrText.Once the
attrTextfix (flagged insvelte-ast.ts) lands, add a case likeattrText([{ type: 'Attribute', name: 'name', value: [{ type: 'Text', data: 'prefix' }, { type: 'ExpressionTag' }] }], 'name')assertingundefined, to prevent regressions.🤖 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 `@packages/core/test/svelte-ast.test.ts` around lines 54 - 70, Add a regression test in the attrText spec to cover mixed-content attributes. Update the existing attrText test block in svelte-ast.test.ts to assert that when an Attribute value contains both Text and an ExpressionTag, attrText returns undefined rather than a string. Use the attrText helper and the mixed-value Attribute shape to target the same behavior covered by the svelte-ast.ts fix.docs/src/content/docs/guides/plugin-mode.md (1)
8-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClarify the scan scope wording.
collectComponentFacts()walkssrc/**/*.svelte, so "directly undersrc/" reads as top-level-only and understates the actual coverage for nested components. Please reword this to say "all.sveltefiles undersrc/" or "recursively undersrc/".♻️ Proposed wording tweak
-Build mode additionally scans your `.svelte` source directly under `src/` for ... +Build mode additionally scans all `.svelte` files under `src/` recursively for ...🤖 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 `@docs/src/content/docs/guides/plugin-mode.md` at line 8, The build-mode scan scope wording in the plugin-mode docs is too narrow: `collectComponentFacts()` actually walks all `.svelte` files recursively under `src/`, not just files directly under `src/`. Update the description near the `@svelte-vitals/vite` overview to say “all `.svelte` files under `src/`” or “recursively under `src/`” so the documented coverage matches the `collectComponentFacts()` behavior.packages/cli/src/providers/source/adapters/svelte-meta-tags.ts (1)
9-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate
findAttrwith the core export.This file (and
svelte-seo.ts) still defines a localfindAttrthat is functionally identical tofindAttrnow exported from@svelte-vitals/core(already used directly inparse.ts, e.g. findAttr(node.attributes, 'as')). Since this PR's goal is centralizing shared AST helpers, both adapters could importfindAttrfrom core instead of re-declaring it, avoiding future drift between the two copies.♻️ Proposed consolidation
-import { attrValueOf, attrTextOf } from '`@svelte-vitals/core`'; +import { attrValueOf, attrTextOf, findAttr } from '`@svelte-vitals/core`'; -function findAttr(attributes: Node[], name: string): Node | undefined { - return attributes.find((a) => a?.type === 'Attribute' && a.name === name); -}Please confirm core's
findAttrimplementation matches this local one exactly (sametype === 'Attribute'check) before removing the duplicates.🤖 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 `@packages/cli/src/providers/source/adapters/svelte-meta-tags.ts` around lines 9 - 11, The local findAttr helper is duplicated in the Svelte adapter and should be consolidated with the shared core export. Update the adapter to import findAttr from `@svelte-vitals/core` instead of redefining it, and make the same change in the matching svelte-seo.ts adapter so both use the single shared helper. Before removing the local copies, verify the core findAttr behavior matches the current Node[] lookup and type === 'Attribute' check used by findAttr.
🤖 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 `@docs/superpowers/specs/2026-07-05-vite-component-rules-design.md`:
- Around line 201-205: The fenced snippet in the spec is missing a language tag,
causing the markdownlint MD040 warning. Update the fenced block around the
analyzed output text to use the appropriate tag (ts) so the documentation stays
lint-clean. Locate the affected fence in the spec section containing the
“Analyzed ${heads.length} prerendered route(s)” text and change only the fence
marker, not the snippet contents.
In `@packages/core/src/svelte-ast.ts`:
- Around line 55-67: The attrText helper is treating mixed static/dynamic
attributes as fully static because it only collects Text nodes from the
attr.value array. Update attrText in svelte-ast.ts to use the same mixed-content
check as attrTextOf/valueFromNodes, so any ExpressionTag anywhere in the array
causes undefined to be returned. If helpful, refactor attrText to delegate to
attrTextOf to remove duplication, and ensure attrTextOf is defined before it is
called or reorder the helpers accordingly.
In `@packages/vite/src/providers/source/components.ts`:
- Around line 16-34: The catch in collectComponentFacts is swallowing read/parse
failures and returning an empty fact object, which makes failed files look
successfully scanned. Update collectComponentFacts and the CLI mirror to surface
the error (for example via logging or an error collection) when readFile or
parseComponentFacts fails, and do not treat the file as a clean component in the
normal results. Keep using the existing collectComponentFacts flow and its
component file handling, but ensure failures are visible instead of silently
producing empty facts.
---
Nitpick comments:
In `@docs/src/content/docs/guides/plugin-mode.md`:
- Line 8: The build-mode scan scope wording in the plugin-mode docs is too
narrow: `collectComponentFacts()` actually walks all `.svelte` files recursively
under `src/`, not just files directly under `src/`. Update the description near
the `@svelte-vitals/vite` overview to say “all `.svelte` files under `src/`” or
“recursively under `src/`” so the documented coverage matches the
`collectComponentFacts()` behavior.
In `@packages/cli/src/providers/source/adapters/svelte-meta-tags.ts`:
- Around line 9-11: The local findAttr helper is duplicated in the Svelte
adapter and should be consolidated with the shared core export. Update the
adapter to import findAttr from `@svelte-vitals/core` instead of redefining it,
and make the same change in the matching svelte-seo.ts adapter so both use the
single shared helper. Before removing the local copies, verify the core findAttr
behavior matches the current Node[] lookup and type === 'Attribute' check used
by findAttr.
In `@packages/core/test/svelte-ast.test.ts`:
- Around line 54-70: Add a regression test in the attrText spec to cover
mixed-content attributes. Update the existing attrText test block in
svelte-ast.test.ts to assert that when an Attribute value contains both Text and
an ExpressionTag, attrText returns undefined rather than a string. Use the
attrText helper and the mixed-value Attribute shape to target the same behavior
covered by the svelte-ast.ts fix.
In `@packages/vite/src/analyze.ts`:
- Around line 46-50: The collectors in analyze() are independent, and
collectComponentFacts(cwd) does not depend on the htmlLang/project result.
Update the analyze flow to run collectRenderedProject(cwd, htmlLang) and
collectComponentFacts(cwd) concurrently with Promise.all, while keeping
collectRenderedHeads(prerenderPagesDir) before them and preserving the same data
passed into runRules and applyRuleSeverities.
In `@packages/vite/src/providers/source/components.ts`:
- Around line 20-32: The fallback “empty ComponentFacts” object is duplicated
here and should be centralized to avoid drift from the shared ComponentFacts
shape. Extract a shared helper or default factory such as
emptyComponentFacts(file) in `@svelte-vitals/core`, then update the components.ts
fallback to use it so this source and any CLI collector stay in sync if
ComponentFacts changes.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 037edd11-5630-4a13-9600-27c8cad402db
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (28)
.changeset/vite-component-rules-cli.md.changeset/vite-component-rules-core.md.changeset/vite-component-rules-vite.mddocs/src/content/docs/guides/choosing-a-package.mddocs/src/content/docs/guides/dev-overlay.mddocs/src/content/docs/guides/plugin-mode.mddocs/src/content/docs/ja/guides/choosing-a-package.mddocs/src/content/docs/ja/guides/dev-overlay.mddocs/src/content/docs/ja/guides/plugin-mode.mddocs/superpowers/plans/2026-07-05-vite-component-rules.mddocs/superpowers/specs/2026-07-05-vite-component-rules-design.mdpackages/cli/src/providers/source/adapters/svelte-meta-tags.tspackages/cli/src/providers/source/adapters/svelte-seo.tspackages/cli/src/providers/source/components.tspackages/cli/src/providers/source/parse.tspackages/cli/test/collect-component-facts.test.tspackages/cli/test/parse-file.test.tspackages/cli/test/suppression-e2e.test.tspackages/core/package.jsonpackages/core/src/component-parse.tspackages/core/src/index.tspackages/core/src/svelte-ast.tspackages/core/test/component-parse.test.tspackages/core/test/svelte-ast.test.tspackages/vite/src/analyze.tspackages/vite/src/providers/source/components.tspackages/vite/test/analyze.test.tspackages/vite/test/collect-component-facts.test.ts
attrText's array branch filtered for Text nodes but never checked for
an ExpressionTag alongside them, so href="prefix{expr}" incorrectly
returned "prefix" instead of undefined — inconsistent with its own doc
comment and with the sibling attrTextOf, which already guards this
case. Also fixes a missing MD040 language tag on a fenced code block
in the design doc.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015qoHchC2eWjmw2rdm91kLq
There was a problem hiding this comment.
Pull request overview
This PR extends @svelte-vitals/vite build-mode analysis to include component-scoped rules by scanning .svelte source under src/ and wiring the resulting components facts into the core rule pipeline. To avoid duplication, it extracts the CLI’s component-facts parser (and shared Svelte AST helpers) into @svelte-vitals/core, then updates CLI/vite to consume the shared implementation and documents the new coverage.
Changes:
- Extract
.svelteAST utilities +parseComponentFacts()into@svelte-vitals/core(with new tests and a newsveltedependency). - Add a new vite build-mode source collector (
src/**/*.svelte) and passcomponentsintorunRules(); extend console coverage output and add integration/unit tests. - Update CLI import paths, add changesets, and update English/Japanese docs to reflect build-mode vs dev-overlay scope.
Reviewed changes
Copilot reviewed 28 out of 29 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Locks the new svelte dependency for packages/core. |
| packages/vite/test/collect-component-facts.test.ts | Adds filesystem-based unit tests for the new vite component collector. |
| packages/vite/test/analyze.test.ts | Extends analyze integration test to assert a real component-scoped finding is produced. |
| packages/vite/src/providers/source/components.ts | New vite build-mode collector that globs + parses src/**/*.svelte. |
| packages/vite/src/analyze.ts | Wires collected component facts into rule execution and updates coverage note output. |
| packages/core/test/svelte-ast.test.ts | Adds unit tests for extracted Svelte AST helper utilities. |
| packages/core/test/component-parse.test.ts | Moves/creates tests for parseComponentFacts() in core. |
| packages/core/src/svelte-ast.ts | New shared AST helper module exported from core. |
| packages/core/src/index.ts | Re-exports parseComponentFacts and AST helpers from the core package root. |
| packages/core/src/component-parse.ts | New shared parseComponentFacts() implementation in core. |
| packages/core/package.json | Adds svelte as a core dependency (for svelte/compiler). |
| packages/cli/test/suppression-e2e.test.ts | Updates parseComponentFacts import to come from @svelte-vitals/core. |
| packages/cli/test/parse-file.test.ts | Updates tests to import attrValueOf from core (no longer from CLI parse module). |
| packages/cli/test/collect-component-facts.test.ts | Adds a focused CLI test for the Runtime-based component collector. |
| packages/cli/src/providers/source/parse.ts | Removes duplicated AST/component parsing helpers; consumes shared core helpers for head parsing. |
| packages/cli/src/providers/source/components.ts | Switches component parsing to @svelte-vitals/core export. |
| packages/cli/src/providers/source/adapters/svelte-seo.ts | Switches attrValueOf/attrTextOf imports to core. |
| packages/cli/src/providers/source/adapters/svelte-meta-tags.ts | Switches attrValueOf/attrTextOf imports to core. |
| docs/superpowers/specs/2026-07-05-vite-component-rules-design.md | Adds the approved design spec documenting the approach and constraints. |
| docs/superpowers/plans/2026-07-05-vite-component-rules.md | Adds the implementation plan document. |
| docs/src/content/docs/ja/guides/plugin-mode.md | Documents new build-mode source scanning and component-scoped rule coverage (JA). |
| docs/src/content/docs/ja/guides/dev-overlay.md | Clarifies dev overlay remains rendered-only and excludes component-scoped rules (JA). |
| docs/src/content/docs/ja/guides/choosing-a-package.md | Updates comparison table and explanation of coverage differences (JA). |
| docs/src/content/docs/guides/plugin-mode.md | Documents new build-mode source scanning and component-scoped rule coverage (EN). |
| docs/src/content/docs/guides/dev-overlay.md | Clarifies dev overlay remains rendered-only and excludes component-scoped rules (EN). |
| docs/src/content/docs/guides/choosing-a-package.md | Updates comparison table and explanation of coverage differences (EN). |
| .changeset/vite-component-rules-vite.md | Declares a minor release for new default build-mode coverage in the vite plugin. |
| .changeset/vite-component-rules-core.md | Declares a minor release for new core exports + svelte dependency. |
| .changeset/vite-component-rules-cli.md | Declares a patch release for the CLI internal refactor to shared core parser. |
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** Static string of an attribute (e.g. name="description"), or undefined if dynamic/absent. */ | ||
| export function attrText(attributes: Node[], name: string): string | undefined { | ||
| const attr = findAttr(attributes, name); | ||
| if (!attr) return undefined; | ||
| const v = attr.value; | ||
| if (v === true) return ''; | ||
| if (Array.isArray(v)) { | ||
| if (v.some((n: Node) => n?.type === 'ExpressionTag')) return undefined; | ||
| return v | ||
| .filter((n: Node) => n?.type === 'Text') | ||
| .map((n: Node) => String(n.data ?? '')) | ||
| .join(''); | ||
| } | ||
| return undefined; // single ExpressionTag → not a literal |
There was a problem hiding this comment.
Good catch, and you're right — this is a genuine user-visible CLI behavior change, not purely internal to this PR's refactor. Updated the PR description to call it out explicitly, and added a CLI-level regression test in parse-link-attrs.test.ts (commit a161cdb) covering <link href="/base/{slug}">: before the fix this returned the truncated "/base/" as if it were a complete literal URL (which could have fed wrong data into PERF008's origin analysis); now it correctly returns undefined so PERF008 skips it as non-literal. The stricter behavior is intentional — it's a correctness fix for a pre-existing bug, not a new feature.
Regression test for the attrText fix in 94990ed, at the CLI's actual <svelte:head> parsing boundary (PERF008 origin analysis reads link.href from this path). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015qoHchC2eWjmw2rdm91kLq
Summary
@svelte-vitals/vite's build mode previously only ran SEO/Performance rules against rendered HTML — it never populatedRuleContext.components, so CORRECT001–004, SEC001–002, ARCH001–002, and PERF009–010 (component-scoped rules) silently emitted nothing.svelte-source parser (parseComponentFacts+ shared AST utilities, ~350 lines) fromsvelte-vitalsinto@svelte-vitals/core(svelte-ast.ts,component-parse.ts) so it can be reused without duplication@svelte-vitals/vite(directtinyglobby+node:fs, noRuntimeabstraction needed) and wired it intoanalyze()rulesoptionplugin-mode.md,dev-overlay.md,choosing-a-package.mdDesign
See
docs/superpowers/specs/2026-07-05-vite-component-rules-design.md(approved design) anddocs/superpowers/plans/2026-07-05-vite-component-rules.md(implementation plan).Post-review fix (user-visible CLI behavior change)
Code review (94990ed) found and fixed a pre-existing bug in
attrText(carried over verbatim from the original CLI code during extraction, not introduced by this PR): a mixed static/dynamic attribute value likehref="/base/{slug}"was incorrectly treated as fully literal, returning the truncated static prefix ("/base/") instead ofundefined. This is used by the CLI's<svelte:head>parsing formeta[content](description/robots),link[rel|as|hreflang|href](incl. PERF008 origin analysis), andscript[type|src]. The fix makesattrTextcorrectly returnundefinedfor any such mixed value, matching its own doc comment and the siblingattrTextOfhelper's existing behavior. Added a CLI-level regression test (parse-link-attrs.test.ts) locking in the corrected semantics. This is a correctness fix, not a feature change — the original (buggy) behavior was never intentional.Test plan
pnpm test— all packages green (core 343, cli 284, vite 79, mcp 9)pnpm typecheck— zero errorspnpm lint— cleanpnpm --filter docs build— succeedspnpm build— all packages build cleanly (core's newsveltedependency resolves correctly,dist/verified consistent)packages/vite/test/analyze.test.ts) traces a real unkeyed{#each}through the vite collector →parseComponentFacts→analyze()→ a realCORRECT001finding in the console report — not mocked at any layerattrTextbug fix described above, which is intentional and now covered by a regression test🤖 Generated with Claude Code
Summary by CodeRabbit
.sveltesource files undersrc/(in addition to prerendered HTML) and applies component-scoped rules alongside existing checks, with the console summary including the number of components scanned.parseComponentFactsand additional Svelte AST helper utilities for reuse.