Repository navigation
fix(core): stop effect-as-onmount flagging member-read reactivity; align effect-rule docs - #419
Conversation
…ign effect-rule docs
|
Warning Review limit reached
Next review available in: 43 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThe PR expands ChangesCORRECT003 refinement
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
…t wording, module-script test, {@Attach} version gate
Tightens "new-ed local" phrasing to "declared with a new …() initializer" everywhere it
appears (assignment-form `let m; m = new SvelteMap();` is still undetected, now documented
as a second known limitation instead of implied covered). Adds a fact-level test pinning the
<script module> import-folding path (mutation-confirmed: fails when the moduleProgram
collectImportedLocalNames/collectNewExprLocalNames calls are removed). Adds the {@Attach}
5.29+ version gate to the docs, matching the sibling reactivity/window 5.11.0+ note.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
An independent fresh-context review was run on this PR (the maintainer requested extra caution against regressions). Verdict: APPROVE — merging cannot make the tool worse for any rule other than the intended one. Key verifications by the reviewer:
Four non-blocking notes, all addressed in the latest commit: the "initialized with |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/correctness/effect-as-derived.md`:
- Around line 43-44: Update the documentation around $derived, $effect, and
canVibrate: state that $effect runs after component mount, and clarify that the
$derived expression is evaluated when read, including during SSR when it is
referenced. Preserve the guidance to suppress the check with
svelte-vitals-disable-next-line rather than replacing $effect with $derived.
In `@docs/src/content/docs/ja/rules/correctness/instance-browser-global.md`:
- Around line 22-32: Remove the SSR-unsafe `const width = window.innerWidth`
statement from the safe example in the Japanese and English
`instance-browser-global` documentation; keep it only in the preceding unsafe
example (or comment it out), leaving the fix block with the
`svelte/reactivity/window` import and `innerWidth.current` usage.
In `@docs/superpowers/specs/2026-07-02-correct003-effect-as-onmount-design.md`:
- Around line 162-165: Update the 2026-08-09 addendum in the specification to
use the correct review date and revise the related past-tense statements,
including “was refuted” and “Fixed,” so the document does not present a future
event as completed history.
- Around line 164-178: The earlier rule section covering the recommendation,
rationale, diagnostic, and “conservative — no false positives” claim must be
synchronized with the current contract in effect-as-onmount.ts. Update it to
include the current detection behavior and remaining false-positive cases, or
explicitly mark the outdated statements as historical; ensure the section no
longer presents onMount-only guidance or guarantees zero false positives.
In `@packages/core/src/component-parse.ts`:
- Around line 932-954: Replace text-only reactiveNames tracking with
lexical-scope binding resolution in the component analysis flow, including
collectImportedLocalNames and collectNewExprLocalNames. Mark identifier reads
reactive only when their resolved declaration is a selected import or a
new-expression declarator in an enclosing scope, excluding shadowing and
bindings declared inside the current effect callback. Add regression coverage
for effect-local shadowing and effect-local new bindings.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4f28944e-0247-4308-affd-ef8d60504080
📒 Files selected for processing (18)
.changeset/effect-rules-validity.mddocs/src/content/docs/ja/rules/correctness/effect-as-derived.mddocs/src/content/docs/ja/rules/correctness/effect-as-onmount.mddocs/src/content/docs/ja/rules/correctness/index.mdxdocs/src/content/docs/ja/rules/correctness/instance-browser-global.mddocs/src/content/docs/ja/rules/correctness/server-browser-global.mddocs/src/content/docs/ja/rules/index.mdxdocs/src/content/docs/rules/correctness/effect-as-derived.mddocs/src/content/docs/rules/correctness/effect-as-onmount.mddocs/src/content/docs/rules/correctness/index.mdxdocs/src/content/docs/rules/correctness/instance-browser-global.mddocs/src/content/docs/rules/correctness/server-browser-global.mddocs/src/content/docs/rules/index.mdxdocs/superpowers/specs/2026-07-02-correct003-effect-as-onmount-design.mdpackages/core/src/component-parse.tspackages/core/src/rules/correctness/effect-as-onmount.tspackages/core/test/component-parse.test.tspackages/core/test/correctness-rules.test.ts
…ble fix block split, shadowing clause - effect-as-derived.md (en+ja): "$effect runs one tick after mount" -> "runs only after the component has mounted to the DOM" (matches the official docs' framing); "$derived evaluates during SSR" unconditional claim qualified to "evaluates when read, and a template read happens during SSR too" — same imprecision existed in both locales, both fixed. - instance-browser-global.md (en+ja): the svelte/reactivity/window fix fence contained the ❌ window.innerWidth line ahead of the ✅ import — a verbatim copy would still crash SSR. Split into a standalone ❌ example and a fix fence containing only the import + usage. Both autofixer-verified clean. - 2026-07-02-correct003-effect-as-onmount-design.md: one-line inline pointer at the refuted "conservative — no false positives" claim; no other historical-section rewrites, per convention. - effect-as-onmount.md (en+ja) + changeset: added the shadowing-granularity clause — matching is by identifier text, not lexical scope (same as the rule's pre-existing rune-name handling), so a callback-local binding that shadows an imported/new-declared name still reads as reactive; this can only suppress a finding, never wrongly flag one. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
From the 2026-08-09 v1.0 rule-validity review (Priority-1 rows 1 and 5) — the review's highest-impact finding.
Finding 1:
correctness/effect-as-onmount's advice broke working codebodyReadsReactiverecognized reactivity only through same-file rune declarators, so four officially-recommended reactive idioms — class instances with$statefields,SvelteMap/SvelteSetreads, imported runes-module state,svelte/reactivity/window— were indistinguishable from the intended true positive. The "reads no reactive value — useonMountinstead" advice, applied to any of them, converts a re-running effect into run-once: a stale-UI bug. Verified empirically by two independent reviewers before this fix.Fix (detection narrowing, deliberately conservative): local names bound by non-type imports and locals initialized with
newexpressions are folded intoreactiveNames— an imported binding or class instance is opaque to static analysis and may carry hidden reactivity, so it suppresses the finding wherever referenced. The trade (member-form access on an imported binding — e.g.analytics.track()— now escapes detection; bare imported calls already did on both sides) is intentional: a missed finding is cheaper than wrong advice. The remaining blind spot (const c = createCounter()— no syntactic marker) is admitted in the code comment and the doc's new Known-limitation section; the design doc carries a dated addendum retiring its "no false positives" claim. Rule wording now names{@attach}/event handlers alongsideonMountper current Svelte guidance (MCP-verified).Finding 5: interlocking docs contradiction
The documented fixes in
server-browser-global.md/instance-browser-global.md($effect(() => { x = localStorage/window… })) were themselves flagged byeffect-as-derived, whose "$derived" advice would reintroduce the SSR ReferenceError those rules prevent. Both docs' fixes are nowonMount(SSR-inert, flagged by neither effect rule;svelte/reactivity/windowshown as the modern idiom for instance-browser-global),effect-as-derived.mdgains the browser-global-capture limitation beside its existing one, and its "updates synchronously" claim is corrected to push-pull/lazy. All shipped fix snippets autofixer-clean; the one deliberately-flagged illustration in the limitation section is labeled as the documented trap, not a fix.Verification
pnpm -r typecheck/pnpm test(core 1360, cli 875, vite 213) /pnpm lint/pnpm -r build/check:publishall green; rules-index regenerated (frontmatter description changed).@svelte-vitals/corepatch (false-positive removals + docs corrections).🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
onMountrecommendations for reactive class instances, collections, imported state, and browser-reactivity bindings.Documentation
{@Attach},onMount, or$derived.Tests