feat: auto-dispose effect/scope on setup failure, upgrade alien_signals to 2.3.1#42
Conversation
…ls to 2.3.1 - Upgrade alien_signals from ^2.2.0 to ^2.3.1 - Add auto-dispose catch blocks in _createEffect/_createEffectScope - _OrefEffectScope extends EffectScopeNode (new in 2.3.1) instead of ReactiveNode - Rename _OrefEffect.cleanup -> _userCleanup to avoid shadowing EffectNode.cleanup - Simplify call() methods: delegate child disposal to alien.stop(), removing manual deps iteration - Add 5 auto-dispose tests covering Dart and Flutter widget contexts
|
Warning Review limit reached
More reviews will be available in 12 minutes and 23 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughEffect and effect-scope setup now auto-dispose created resources when their setup callbacks throw. Internal cleanup storage was renamed, effect execution gets a try/catch to trigger disposal on throw, effect-scope base class changed to ChangesAuto-dispose on setup failure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f1893151c4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
test/core/effect_test.dart (3)
646-681: 💤 Low valueConsider whether explicit type annotation and cast are necessary.
The test uses
signal<Object?>(null, 0)with an explicit type parameter, then later casts toWritableSignal<Object?>when calling.set(1)(line 677). In contrast, the non-widget tests (line 569) usesignal(null, 0)and call.set()directly without casting (line 583).The explicit type and cast may be required due to type inference in the widget builder context, or they might be unnecessary verbosity. Consider testing whether this simpler form works:
final source = signal(null, 0); // ... source.set(1);If the cast is genuinely required, a brief comment explaining why would help future maintainers.
🤖 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 `@test/core/effect_test.dart` around lines 646 - 681, The test creates `source` as signal<Object?>(null, 0) then casts to `WritableSignal<Object?>` to call `.set(1)`; simplify by letting type inference pick the signal type (use `final source = signal(null, 0)`) and call `source.set(1)` directly in the failing-effect widget test (`testWidgets('failed effect setup in widget does not leave subscription'...)`), and if the cast is actually required by the widget builder context keep the current explicit type but add a short comment explaining why the `WritableSignal` cast is necessary to aid future maintainers.
611-642: 💤 Low valueConsider moving this test to a separate group.
This test verifies that stopping an effect during its run prevents subsequent signal reads from creating subscriptions. While the test is correct and valuable, it doesn't test "setup failure" — it tests mid-run disposal behavior. The group name "Effect auto-dispose on setup failure" suggests all tests should verify setup-time error handling.
Consider either:
- Moving this test to a new group like "Effect disposal during execution"
- Renaming the group to "Effect auto-dispose behavior"
🤖 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 `@test/core/effect_test.dart` around lines 611 - 642, The test named 'stopped effect does not subscribe to signals read later in the same run' belongs to a different behavior than "setup failure" — move this test (the test block that creates rerun, readAfterStop, stopDuringRun and calls effect/stop) into a new test group (e.g., "Effect disposal during execution") or rename the existing group to a more general title like "Effect auto-dispose behavior" so the test's intent (stopping an effect mid-run preventing later subscriptions) matches its group; update the surrounding group(...) or group name accordingly to keep tests organized and semantically correct.
683-721: 💤 Low valueSame type annotation and cast pattern as previous test.
This test has the same explicit type annotation and cast pattern as the previous test (lines 686, 717). See the previous comment for suggestions on potential simplification.
🤖 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 `@test/core/effect_test.dart` around lines 683 - 721, The test uses an explicit generic on signal and later casts it to WritableSignal; replace the explicit type and cast by letting type inference produce a writable signal: change the declaration using signal(...) (e.g. var source = signal(null, 0); or final source = signal(null, 0);) and remove the explicit (source as WritableSignal<Object?>) cast before calling set, using source.set(1) directly; locate usages of signal(...) and the cast in this test to update them accordingly.
🤖 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 `@lib/src/core/effect.dart`:
- Around line 111-112: The onEffectCleanup callback stored in
_OrefEffect._userCleanup must be invoked during the stop/teardown path because
_OrefEffect.call() (used by stop and the setup-failure catch in _createEffect())
currently does not run _userCleanup; change _OrefEffect.call() (or the
stop/teardown branch it uses) to drain and invoke _userCleanup with the same
semantics as the drain in _wrapEffectCallback() so user-managed resources are
cleaned on stop or setup failure, or alternatively wire _userCleanup into the
existing inherited cleanup mechanism so the cleanup runs immediately when
stop/teardown executes.
---
Nitpick comments:
In `@test/core/effect_test.dart`:
- Around line 646-681: The test creates `source` as signal<Object?>(null, 0)
then casts to `WritableSignal<Object?>` to call `.set(1)`; simplify by letting
type inference pick the signal type (use `final source = signal(null, 0)`) and
call `source.set(1)` directly in the failing-effect widget test
(`testWidgets('failed effect setup in widget does not leave subscription'...)`),
and if the cast is actually required by the widget builder context keep the
current explicit type but add a short comment explaining why the
`WritableSignal` cast is necessary to aid future maintainers.
- Around line 611-642: The test named 'stopped effect does not subscribe to
signals read later in the same run' belongs to a different behavior than "setup
failure" — move this test (the test block that creates rerun, readAfterStop,
stopDuringRun and calls effect/stop) into a new test group (e.g., "Effect
disposal during execution") or rename the existing group to a more general title
like "Effect auto-dispose behavior" so the test's intent (stopping an effect
mid-run preventing later subscriptions) matches its group; update the
surrounding group(...) or group name accordingly to keep tests organized and
semantically correct.
- Around line 683-721: The test uses an explicit generic on signal and later
casts it to WritableSignal; replace the explicit type and cast by letting type
inference produce a writable signal: change the declaration using signal(...)
(e.g. var source = signal(null, 0); or final source = signal(null, 0);) and
remove the explicit (source as WritableSignal<Object?>) cast before calling set,
using source.set(1) directly; locate usages of signal(...) and the cast in this
test to update them accordingly.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 21606d44-a89a-4174-894c-cc18aa4b4149
⛔ Files ignored due to path filters (1)
example/pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
lib/src/core/effect.dartlib/src/core/effect_scope.dartpubspec.yamltest/core/effect_test.dart
- Remove unnecessary explicit type annotations and WritableSignal casts in widget tests - Rename group from 'auto-dispose on setup failure' to 'auto-dispose behavior' since it also covers mid-run disposal
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
Previously _OrefEffect.call() skipped _userCleanup, so cleanup registered via onEffectCleanup() during setup was silently dropped when setup failed and auto-dispose fired. Drain _userCleanup before onDispose in the stop path to match alien_signals 2.3.1's stopEffect() semantics.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02bb160ef6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
When an effect calls its own disposer inside the callback, activeSub is still the effect until _wrapEffectCallback returns. In alien_signals 2.3.1, SignalNode.get() unconditionally links any non-null activeSub, so signals read after alien.stop(this) would re-link to the disposed effect. This didn't cause re-execution (run() guards against it), but caused stale dependency links and unnecessary propagation on every signal change. Fix: check if activeSub === this after stop, and clear it if so.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Lines 150 to 151 in 3584b17
Mark active parents when linking child effects
When this wrapper creates an effect under another active effect/scope, it only calls alien.link(effect, prevSub, 0), but alien_signals 2.3.1's own surface API also sets the parent's child-effect flag. Without that flag, alien.run() does not dispose child deps before a parent effect re-runs, so child effects from the previous run remain subscribed until purgeDeps; if the parent callback writes a signal during setup, those stale children can fire or accumulate duplicate work. Please set the parent child-effect flag when linking child effects, and apply the same pattern in _createEffectScope().
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
In alien_signals 2.3.1, both effect() and effectScope() set the hasChildEffect flag (64) on the parent when linking a child. Without this flag, run() does not call disposeChildDepsInReverse() before the parent re-executes, so stale child effects from a previous run stay linked and may trigger during the parent's callback — causing duplicate work or side effects. Reproduced by test: parent creates child on first run, then writes to the child's dependency during re-run. Without hasChildEffect, the stale child fires (childRuns=2). With the flag, it's disposed first (childRuns=1).
1 participant
Summary
Upgrade alien_signals to 2.3.1 and implement auto-dispose on setup failure at the oref layer.
Context
Upstream alien-signals-dart#38 removed the Dart-only auto-dispose-on-setup-failure behavior, treating it as a lifecycle policy decision for downstream code. This PR implements that policy in oref's wrapper layer and leverages new 2.3.1 features to simplify the codebase.
Changes
Dependency
alien_signals:^2.2.0→^2.3.1Auto-dispose on setup failure
catchblocks in_createEffect()and_createEffectScope()that auto-dispose partially-initialized nodes when the callback throwsSimplifications (leveraging alien_signals 2.3.1)
_OrefEffectScopenow extendsEffectScopeNode(new in 2.3.1) instead ofReactiveNode, giving proper propagation supportcall()methods simplified: removed 28 lines of manualdepsiteration + switch/case dispatch, replaced withalien.stop(this)which dispatches to 2.3.1's built-instopEffect()/stopScope()cleanup→_userCleanupto avoid shadowingEffectNode.cleanup(new built-in cleanup mechanism in 2.3.1)Tests
context: null) and Flutter widget (BuildContext) contextsDiff
-28lines of business code (effect.dart -11, effect_scope.dart -11 + catch blocks)Summary by CodeRabbit
Bug Fixes
Tests
Chores