Repository navigation
feat(core): CORRECT003 — $effect used as onMount - #76
Merged
Merged
Conversation
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR adds CORRECT003, which flags ChangesCORRECT003 effect-as-onMount rule
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 2
🧹 Nitpick comments (1)
packages/cli/src/providers/source/parse.ts (1)
446-462: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNon-computed member/property names can be misread as reactive references.
bodyReadsReactiverelies on the genericwalkEstree, which recurses into every object key includingMemberExpression.propertyandProperty.keyeven whencomputedisfalse. A property name that happens to match a reactive binding name (or starts with$) is then wrongly treated as a reactive read, suppressing a legitimate CORRECT003 flag.Example:
let title = $state('t'); $effect(() => { el.title = 'x'; });—titlehere is just a property name, not a reference to thetitlestate, yet it forcesmountOnlytofalse.This is a conservative-direction bug (under-flagging only), but worth tightening since common property names (
value,id,name,title,count, …) are likely to collide with reactive bindings.♻️ Possible fix sketch
function bodyReadsReactive(fn: Node, reactiveNames: Set<string>): boolean { let reads = false; - walkEstree(fn.body, (n: Node) => { - if (reads) return; - if (n?.type === 'Identifier') { - if (reactiveNames.has(n.name)) reads = true; - else if (n.name.startsWith('$') && !RUNE_NAMES.has(n.name)) reads = true; - } else if (n?.type === 'CallExpression' && n.callee?.type === 'Identifier') { - reads = true; - } - }); + const visit = (n: Node): void => { + if (reads || !n || typeof n !== 'object' || typeof n.type !== 'string') return; + if (n.type === 'Identifier') { + if (reactiveNames.has(n.name)) reads = true; + else if (n.name.startsWith('$') && !RUNE_NAMES.has(n.name)) reads = true; + } else if (n.type === 'CallExpression' && n.callee?.type === 'Identifier') { + reads = true; + } else if (n.type === 'MemberExpression') { + visit(n.object); + if (n.computed) visit(n.property); + return; // skip non-computed property key + } else if (n.type === 'Property' && !n.computed) { + visit(n.value); + return; // skip non-computed property key + } + for (const key of Object.keys(n)) { + if (key === 'type' || key === 'start' || key === 'end' || key === 'loc' || key === 'range') continue; + visit((n as any)[key]); + } + }; + visit(fn.body); return reads; }🤖 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/parse.ts` around lines 446 - 462, The bodyReadsReactive helper is overcounting reactive reads because walkEstree also visits non-computed member/property keys, so identifiers used as plain property names can falsely match reactive bindings. Update bodyReadsReactive in parse.ts to ignore MemberExpression.property and Property.key when computed is false (or otherwise skip non-value positions) while still detecting real reactive identifiers, $-prefixed stores, and bare-identifier calls. Use the existing bodyReadsReactive and walkEstree logic as the main place to tighten this check.
🤖 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/src/content/docs/ja/rules/correct003.md`:
- Line 10: The description in the `$effect`/`$effect.pre` rule is too narrow
because it says only “local function calls,” while the parser treats any bare
identifier call as a reactive read. Update the wording in the rule doc to match
the behavior from `parse.ts` by referring to “bare function calls” or
equivalent, and keep the rest of the reactive-read examples (`$state`,
`$derived`, `$props`, store subscriptions) unchanged.
In `@docs/src/content/docs/rules/correct003.md`:
- Line 10: The CORRECT003 description is too narrow because it says “local
function call,” but the parser in parse.ts treats any bare identifier call as a
valid reactive read. Update the rule text in correct003.md to describe a “bare
function call” instead of only local helpers, and keep the rest of the
effect/readability wording aligned with the static analysis behavior.
---
Nitpick comments:
In `@packages/cli/src/providers/source/parse.ts`:
- Around line 446-462: The bodyReadsReactive helper is overcounting reactive
reads because walkEstree also visits non-computed member/property keys, so
identifiers used as plain property names can falsely match reactive bindings.
Update bodyReadsReactive in parse.ts to ignore MemberExpression.property and
Property.key when computed is false (or otherwise skip non-value positions)
while still detecting real reactive identifiers, $-prefixed stores, and
bare-identifier calls. Use the existing bodyReadsReactive and walkEstree logic
as the main place to tighten this check.
🪄 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: 02aefdbe-f2d7-4b56-8c20-859a16fe716f
📒 Files selected for processing (12)
.changeset/correct003-effect-onmount.mddocs/src/content/docs/ja/rules/correct003.mddocs/src/content/docs/rules/correct003.mddocs/superpowers/plans/2026-07-02-correct003-effect-as-onmount.mddocs/superpowers/specs/2026-07-02-correct003-effect-as-onmount-design.mdpackages/cli/src/providers/source/parse.tspackages/cli/test/parse-component-facts.test.tspackages/core/src/component.tspackages/core/src/index.tspackages/core/src/rules/correctness/correct001-002.tspackages/core/src/rules/index.tspackages/core/test/correctness-rules.test.ts
…re call (PR #76 review) bodyReadsReactive now uses a dedicated walk that skips non-computed MemberExpression.property and Property.key, so a property named like a reactive binding (obj.count) no longer suppresses a legitimate flag; computed access (obj[count]) and reactive names in call arguments still count as reads. Docs (en/ja) reworded 'local function call' -> 'bare function call'. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Owner
Author
|
Addressed the review in 3c58162:
Green: parse-component-facts 39/39, correctness-rules 10/10, typecheck + prettier clean. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a new correctness rule CORRECT003 to detect $effect/$effect.pre bodies that never read reactive values (i.e., they effectively run once after mount), and recommends using onMount to make intent explicit. It extends the CLI’s component-fact extraction with a new EffectFact.mountOnly classification, registers the rule in core exports, and documents/releases it.
Changes:
- Add
EffectFact.mountOnlycomputed by the CLI parser via a conservative “reads reactive” scan. - Introduce and register CORRECT003 (
correct003EffectAsOnMount) and add core/cli tests covering the new behavior. - Add docs (en/ja), spec/plan artifacts, and a changeset bumping
@svelte-vitals/core,svelte-vitals, and@svelte-vitals/mcp(minor).
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/cli/src/providers/source/parse.ts | Computes mountOnly by scanning effect bodies for reactive reads / store subs / bare calls. |
| packages/core/src/component.ts | Extends EffectFact with mountOnly. |
| packages/core/src/rules/correctness/correct001-002.ts | Adds correct003EffectAsOnMount rule based on mountOnly. |
| packages/core/src/rules/index.ts | Registers and re-exports CORRECT003 in allRules and exports. |
| packages/core/src/index.ts | Re-exports CORRECT003 from the package entrypoint. |
| packages/cli/test/parse-component-facts.test.ts | Updates existing effect expectations and adds CORRECT003 capture tests for mountOnly. |
| packages/core/test/correctness-rules.test.ts | Updates CORRECT002 fixtures for the new field and adds CORRECT003 rule tests. |
| docs/src/content/docs/rules/correct003.md | Adds English rule reference page for CORRECT003. |
| docs/src/content/docs/ja/rules/correct003.md | Adds Japanese rule reference page for CORRECT003. |
| .changeset/correct003-effect-onmount.md | Minor version changeset for core/app/mcp plus release notes for CORRECT003. |
| docs/superpowers/specs/2026-07-02-correct003-effect-as-onmount-design.md | Design spec for the new mount-only effect detection and rule. |
| docs/superpowers/plans/2026-07-02-correct003-effect-as-onmount.md | Implementation plan covering capture, rule, docs, and verification steps. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Props/state/derived with defaults ({ title = 'x' }), nested patterns
({ a: { b } }), arrays, and rest were dropped from reactiveNames, which could
mark an reading such a binding as mountOnly — a false-positive CORRECT003.
addBoundNames now recurses the full binding-pattern grammar.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/cli/test/parse-component-facts.test.ts (1)
202-209: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding array/rest destructuring coverage.
The commit message states the destructuring fix tracks reactive bindings "inside defaults, nested patterns, arrays, and rest," but this block only tests default values and nested object destructuring (
{ a: { b } }). No test exercises array destructuring (let [a] = $props()-style, if applicable) or rest patterns (let { ...rest } = $props()). GivenaddBoundNamespresumably now handles these per the commit description, adding assertions here would close the gap between the stated fix scope and test coverage.🤖 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/test/parse-component-facts.test.ts` around lines 202 - 209, The destructuring coverage test is missing cases for array and rest bindings even though the fix is meant to handle them. Extend the existing facts() assertions in parse-component-facts.test.ts to cover reactive names produced by addBoundNames from array patterns and rest patterns, alongside the current default and nested object destructuring checks, so the behavior is verified for all mentioned binding forms.
🤖 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.
Nitpick comments:
In `@packages/cli/test/parse-component-facts.test.ts`:
- Around line 202-209: The destructuring coverage test is missing cases for
array and rest bindings even though the fix is meant to handle them. Extend the
existing facts() assertions in parse-component-facts.test.ts to cover reactive
names produced by addBoundNames from array patterns and rest patterns, alongside
the current default and nested object destructuring checks, so the behavior is
verified for all mentioned binding forms.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5520b204-8e18-4337-933f-90ef50355c0a
📒 Files selected for processing (4)
docs/src/content/docs/ja/rules/correct003.mddocs/src/content/docs/rules/correct003.mdpackages/cli/src/providers/source/parse.tspackages/cli/test/parse-component-facts.test.ts
✅ Files skipped from review due to trivial changes (2)
- docs/src/content/docs/ja/rules/correct003.md
- docs/src/content/docs/rules/correct003.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/cli/src/providers/source/parse.ts
1 task
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
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.
The Correctness/reactivity slice of #69. Adds CORRECT003, flagging an
$effect/$effect.prewhose non-empty body reads no reactive value — such an effect runs once after mount and never re-runs, so it is anonMountin disguise.warningseverity, matching CORRECT002.Distinct from CORRECT002 (an
$effectthat only assigns$state→ use$derived): CORRECT003 is an$effectthat neither reads nor derives reactive state → useonMount.What's flagged
How it works (precision-first)
A new computed
EffectFact.mountOnlyboolean, set by the CLI parser. An effect is not flagged (conservative) when its body:reactiveNames=$state∪$derived∪$propsbindings, incl. destructured + rest),$-prefixed non-rune identifier (store subscription), orfoo()(a local/imported fn may read state internally).Member calls (
el.focus(),console.log('x')) are not suppressive on their own, but a reactive name in a call argument is caught by rule 1. Empty bodies are never flagged. The cardinal sin — flagging an effect that legitimately reacts — was scrutinized hardest in review; no false-positive path found.The
componentRule-built rule filtersmountOnlyand no-ops in rendered mode.assignsOnlyState/stateNames(CORRECT002) are untouched.Docs & release
@svelte-vitals/core+svelte-vitals+@svelte-vitals/mcpminor (CLI/static rule; not@svelte-vitals/vite).Validation
pnpm -r test(554: core 250 / vite 76 / cli 219 / mcp 9),pnpm -r typecheck,pnpm lint,pnpm --filter docs build(111 pages incl. the 2 new) — all green.Process
spec → plan → subagent-driven (3 impl tasks, each spec + quality reviewed) + a whole-branch review on Opus (verdict: ready to merge). Zero review-loop fixes needed.
🤖 Generated with Claude Code
Summary by CodeRabbit
$effect/$effect.preused as one-time, mount-like side effects, encouraging use ofonMount.