Add manual invalidation support to provider invalidation logic#4693
Conversation
📝 WalkthroughWalkthroughAdds a manual invalidation path and a public Changes
Sequence DiagramsequenceDiagram
actor User
participant Container as ProviderContainer
participant Element as ProviderElement
participant Ref as Ref
participant Callbacks as Manual Callbacks
User->>Container: invalidate(provider)
Container->>Element: invalidateSelf(asReload: false, manual: true)
Element->>Ref: _runManualInvalidationCallbacks(container, ref)
rect rgba(100,150,200,0.5)
Note over Ref,Callbacks: Manual Invalidation Phase
Ref->>Callbacks: execute registered callbacks
Callbacks->>Ref: may call ref.invalidate(dependency) to forward
Callbacks-->>Ref: callback complete
end
Note over Element: manual callbacks complete
Element->>Element: runOnDispose() (standard disposal)
Element-->>Container: invalidation complete
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 |
…ort for async and stream providers
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/riverpod/lib/src/core/ref.dart (1)
785-803: Consider iterating over a snapshot of the callbacks list to preventConcurrentModificationError.Unlike
_runCallbacks(line 765), which increments_debugCallbackStackto block further Ref operations,_runManualInvalidationCallbacksexplicitly allowsinvalidatecalls within callbacks. If any callback directly or indirectly triggers removal from_onManualInvalidationListeners(e.g., by calling a storedRemoveListenerfunction), this would mutate the list during iteration.The existing
_runCallbackshas the same direct-iteration pattern, but its_debugCallbackStackguard makes list mutation far less likely. Here, the guard is intentionally loosened, increasing the surface for this edge case.🛡️ Defensive copy before iteration
void _runManualInvalidationCallbacks(ProviderContainer container, Ref? ref) { final callbacks = ref?._onManualInvalidationListeners; if (ref == null || callbacks == null) return; - for (final cb in callbacks) { + for (final cb in [...callbacks]) { try { if (kDebugMode) { _debugCallbackStack++; } ref._inManualInvalidationCallback = true; container.runGuarded(cb); } finally { ref._inManualInvalidationCallback = false; if (kDebugMode) { _debugCallbackStack--; } } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/riverpod/lib/src/core/ref.dart` around lines 785 - 803, In _runManualInvalidationCallbacks, avoid iterating the live _onManualInvalidationListeners list to prevent ConcurrentModificationError by making a defensive snapshot (e.g., final callbacks = ref?._onManualInvalidationListeners?.toList() or List.of(...)) and iterating that copy; keep the existing null/ref guard, per-callback setting of ref._inManualInvalidationCallback, the container.runGuarded(cb) call, and the try/finally that clears the flag and (in kDebugMode) adjusts _debugCallbackStack.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/riverpod/lib/src/core/ref.dart`:
- Around line 785-803: In _runManualInvalidationCallbacks, avoid iterating the
live _onManualInvalidationListeners list to prevent ConcurrentModificationError
by making a defensive snapshot (e.g., final callbacks =
ref?._onManualInvalidationListeners?.toList() or List.of(...)) and iterating
that copy; keep the existing null/ref guard, per-callback setting of
ref._inManualInvalidationCallback, the container.runGuarded(cb) call, and the
try/finally that clears the flag and (in kDebugMode) adjusts
_debugCallbackStack.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/riverpod/lib/src/core/ref.dart (1)
787-788: Redundantref == nullbranch in early-return condition.If
refisnull,callbacksis alreadynull(fromref?._onManualInvalidationListeners?.toList()), so theref == nullsub-expression in the guard is unreachable.♻️ Proposed simplification
void _runManualInvalidationCallbacks(ProviderContainer container, Ref? ref) { final callbacks = ref?._onManualInvalidationListeners?.toList(); - if (ref == null || callbacks == null) return; + if (callbacks == null) return;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/riverpod/lib/src/core/ref.dart` around lines 787 - 788, The early-return condition should not redundantly check ref == null because callbacks is derived with ref? and will already be null if ref is null; update the guard to only test callbacks (i.e., remove the ref == null sub-expression). Locate the assignment to callbacks (final callbacks = ref?._onManualInvalidationListeners?.toList()) and the following if-check, and change the condition to if (callbacks == null) return; leaving the callbacks assignment and the _onManualInvalidationListeners reference intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/riverpod/lib/src/core/ref.dart`:
- Around line 338-346: The debug assertion fires because _invalidateSelf calls
_throwIfInvalidUsage() without propagating the skipAssert flag used for manual
invalidation callbacks; update _invalidateSelf (and thus invalidateSelf) to call
_throwIfInvalidUsage with skipAssert: _inManualInvalidationCallback (the same
pattern used by invalidate(other)) so that when inside onManualInvalidation the
assertion is suppressed and the allowed invalidateSelf path doesn't produce the
misleading debug message.
---
Nitpick comments:
In `@packages/riverpod/lib/src/core/ref.dart`:
- Around line 787-788: The early-return condition should not redundantly check
ref == null because callbacks is derived with ref? and will already be null if
ref is null; update the guard to only test callbacks (i.e., remove the ref ==
null sub-expression). Locate the assignment to callbacks (final callbacks =
ref?._onManualInvalidationListeners?.toList()) and the following if-check, and
change the condition to if (callbacks == null) return; leaving the callbacks
assignment and the _onManualInvalidationListeners reference intact.
… pr/TekExplorer/4693
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/riverpod/lib/src/core/ref.dart (1)
338-346:⚠️ Potential issue | 🟡 Minor
invalidateSelf()called inside anonManualInvalidationcallback fires a misleading debug assertion.
_invalidateSelfcalls_throwIfInvalidUsage()without forwardingskipAssert, while the sibling methodinvalidate(...)at line 328 correctly usesskipAssert: _inManualInvalidationCallback. As a result, callingref.invalidateSelf()from within anonManualInvalidationcallback trips the_debugCallbackStack == 0assertion in debug mode with the generic "Cannot use Ref or modify other providers inside life-cycles/selectors" message — even thoughinvalidate(otherProvider)is explicitly allowed in the same context.The actual recursion is already prevented by
_mustRecomputeStateat element.dart:758, so this is purely a debug-mode UX issue. Suggested fix:🔧 Proposed fix
void _invalidateSelf({required bool asReload, required bool manual}) { - _throwIfInvalidUsage(); + _throwIfInvalidUsage(skipAssert: _inManualInvalidationCallback); _element.invalidateSelf(asReload: asReload, manual: manual); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/riverpod/lib/src/core/ref.dart` around lines 338 - 346, The debug assertion is triggered because _invalidateSelf doesn't pass the skipAssert flag to _throwIfInvalidUsage; update _invalidateSelf({required bool asReload, required bool manual}) so it calls _throwIfInvalidUsage(skipAssert: _inManualInvalidationCallback) (matching how invalidate(...) does skipAssert: _inManualInvalidationCallback) before delegating to _element.invalidateSelf(...), ensuring calls to invalidateSelf() inside an onManualInvalidation callback won't hit the misleading debug assertion.
🧹 Nitpick comments (6)
packages/riverpod/lib/src/core/element.dart (1)
754-780: Ordering looks correct, but consider documenting the re-entrancy contract.Setting
_mustRecomputeState = true(line 760) before calling_runManualInvalidationCallbacks(line 763) is important: it ensures that any nestedinvalidateSelftriggered inside a manual-invalidation callback short-circuits at line 758 instead of recursing. Likewise, manual callbacks are invoked beforerunOnDispose()so that listeners are still present (they get nulled insiderunOnDisposeat line 1205).These constraints are subtle and easy to break in future refactors. A short comment on lines 760–765 explaining the required ordering (set guard → run manual callbacks → dispose cleanup) would help protect this invariant.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/riverpod/lib/src/core/element.dart` around lines 754 - 780, Add a short clarifying comment in invalidateSelf explaining the re-entrancy/ordering contract: set the guard _mustRecomputeState before invoking manual callbacks so nested invalidateSelf short-circuits, invoke _runManualInvalidationCallbacks while listeners still exist, then call runOnDispose/mayNeedDispose to perform cleanup (which can null listeners); reference these symbols (_mustRecomputeState, _runManualInvalidationCallbacks, runOnDispose, mayNeedDispose, invalidateSelf) in the comment so future editors know the reason for the specific ordering and must not reorder these steps.packages/riverpod/test/src/core/ref_test.dart (3)
2631-2692: Duplicate test:container.invalidate(provider, asReload: true)is exercised twice.The test at lines 2631–2649 ("is called when invalidate is called with asReload: true") and the test at lines 2671–2692 ("is called when container.invalidate is called with asReload: true") perform the same setup, the same
container.invalidate(provider, asReload: true)call, and the same single-call verification. One of them should be removed (or replaced with a distinct scenario, e.g., asserting non-asReloadcontainer.invalidatebehavior, or covering family invalidation withasReload).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/riverpod/test/src/core/ref_test.dart` around lines 2631 - 2692, The two tests "is called when invalidate is called with asReload: true" and "is called when container.invalidate is called with asReload: true" are duplicates (both set up Provider/Ref with ref.onManualInvalidation(listener.call) and call container.invalidate(provider, asReload: true)); remove or modify one: either delete the first duplicate test (keep the later one) or change it to a distinct scenario (for example, call container.invalidate(provider) without asReload to assert the same listener behavior or test family invalidation with asReload), updating references to container.invalidate(provider, asReload: true) accordingly and keeping the same listener verification (verify(listener()).called(1)).
3132-3189: Async forwarding scenario only assertsreturnsNormally— weak coverage.This "complex forwarding scenario with async providers" wires up several async providers but only verifies that
container.invalidate(complexAsyncDerived)doesn't throw. It doesn't assert that downstream async providers were actually invalidated/rebuilt. Consider strengthening this test by:
- Adding
OnManualInvalidationmocks on each forwarded provider and verifyingverify(...).called(n).- Tracking build counts for
asyncComputed,streamFamily(...), andasyncNotifierProvider, and asserting they incremented after the manual invalidation.Otherwise, regressions in async forwarding semantics could pass undetected.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/riverpod/test/src/core/ref_test.dart` around lines 3132 - 3189, The test only asserts that invalidating complexAsyncDerived returns normally; enhance it to verify downstream providers were actually invalidated and rebuilt: add build counters or mock callbacks for asyncComputed, streamFamily(1)/streamFamily(2), and asyncNotifierProvider (referencing asyncComputed, streamFamily, asyncNotifierProvider, complexAsyncDerived, baseState, and TestAsyncNotifier) when defining them, capture initial build counts after the first pump, then call container.invalidate(complexAsyncDerived) and pump again and assert each counter incremented (or use verify on onManualInvalidation mocks) to ensure forwarding actually triggered rebuilds; also assert the resulting complexAsyncDerived value changes accordingly.
2407-2426: Remove or rename the "debug: basic functionality check" test.This test appears to be a leftover development/debugging placeholder — the
debug:prefix is unusual for a test name, and the assertions it makes (callback invoked once onref.invalidateSelf()) are already fully covered by the next test, "is called when invalidateSelf is called". Consider deleting it, or at minimum giving it a meaningful name.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/riverpod/test/src/core/ref_test.dart` around lines 2407 - 2426, Remove or rename the redundant test named 'debug: basic functionality check' in the test that constructs a Provider and uses ref.onManualInvalidation/ref.invalidateSelf; either delete the entire test block (the test(...) that sets up Provider, container.listen, calls ref.invalidateSelf and asserts callCount == 1, then sub.close()) or give it a meaningful name and ensure it doesn't duplicate the existing "is called when invalidateSelf is called" test; locate the test by the string literal 'debug: basic functionality check' and by references to Provider, ref.onManualInvalidation, and ref.invalidateSelf to make the change.packages/riverpod/lib/src/core/ref.dart (2)
348-393: API and doc comment look good.@experimentalis appropriate while semantics settle.The doc nicely calls out which paths fire the callback (
invalidateSelf,invalidate,refresh) and which don't (dependency changes viawatch). One small nit: the bullet list under "This callback is triggered when:" lists "[refresh] on this provider" without a verb — consider "[refresh] is called on this provider" for parallel structure with the other bullets.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/riverpod/lib/src/core/ref.dart` around lines 348 - 393, Update the doc comment for onManualInvalidation to make the bullet list parallel: change the third triggered bullet from "[refresh] on this provider" to "[refresh] is called on this provider" (referencing the onManualInvalidation method and its RemoveListener return) so all bullets use consistent verb phrases.
790-809: Snapshot semantics of_runManualInvalidationCallbacksdeserve a brief comment.Two subtle behaviors that are correct today but undocumented:
callbacks.toList()snapshots the list, so callbacks registered during invalidation (viaref.onManualInvalidationfrom inside another callback) will not run for the current invalidation — they'll only run on the next one.ref._inManualInvalidationCallbackis set/cleared per-callback. If callback N triggersref.invalidate(otherProvider), that other provider's manual callbacks run with their own ref's flag set; this works because the flag lives on eachRefinstance.A short doc comment capturing (1) above would help future maintainers, since the snapshot vs. live-list trade-off is a common source of bugs.
Additionally, the loop unconditionally writes
ref._inManualInvalidationCallback = falseinfinally. This is fine today (no nested manual invalidation on the same ref is possible due to_mustRecomputeState), but if that invariant ever changes the flag could be cleared prematurely. Worth a comment, or alternatively save/restore the previous value.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/riverpod/lib/src/core/ref.dart` around lines 790 - 809, Add a short doc comment above _runManualInvalidationCallbacks explaining that callbacks.toList() snapshots the listener list so handlers added during the current invalidation won't run until the next invalidation, and that ref._inManualInvalidationCallback is set/cleared per-callback (it lives on each Ref) so other providers' callbacks can run with their own flags; then either (a) change the per-callback flag handling to save and restore the previous value around container.runGuarded (e.g. final prev = ref._inManualInvalidationCallback; ref._inManualInvalidationCallback = true; ... ref._inManualInvalidationCallback = prev) to guard against future invariants changing, or (b) keep the existing set-to-false-in-finally but add a comment referencing the current invariant (_mustRecomputeState prevents nested manual invalidation on the same ref) so future maintainers understand why unconditional clearing is safe.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/riverpod/lib/src/core/ref.dart`:
- Around line 338-346: The debug assertion is triggered because _invalidateSelf
doesn't pass the skipAssert flag to _throwIfInvalidUsage; update
_invalidateSelf({required bool asReload, required bool manual}) so it calls
_throwIfInvalidUsage(skipAssert: _inManualInvalidationCallback) (matching how
invalidate(...) does skipAssert: _inManualInvalidationCallback) before
delegating to _element.invalidateSelf(...), ensuring calls to invalidateSelf()
inside an onManualInvalidation callback won't hit the misleading debug
assertion.
---
Nitpick comments:
In `@packages/riverpod/lib/src/core/element.dart`:
- Around line 754-780: Add a short clarifying comment in invalidateSelf
explaining the re-entrancy/ordering contract: set the guard _mustRecomputeState
before invoking manual callbacks so nested invalidateSelf short-circuits, invoke
_runManualInvalidationCallbacks while listeners still exist, then call
runOnDispose/mayNeedDispose to perform cleanup (which can null listeners);
reference these symbols (_mustRecomputeState, _runManualInvalidationCallbacks,
runOnDispose, mayNeedDispose, invalidateSelf) in the comment so future editors
know the reason for the specific ordering and must not reorder these steps.
In `@packages/riverpod/lib/src/core/ref.dart`:
- Around line 348-393: Update the doc comment for onManualInvalidation to make
the bullet list parallel: change the third triggered bullet from "[refresh] on
this provider" to "[refresh] is called on this provider" (referencing the
onManualInvalidation method and its RemoveListener return) so all bullets use
consistent verb phrases.
- Around line 790-809: Add a short doc comment above
_runManualInvalidationCallbacks explaining that callbacks.toList() snapshots the
listener list so handlers added during the current invalidation won't run until
the next invalidation, and that ref._inManualInvalidationCallback is set/cleared
per-callback (it lives on each Ref) so other providers' callbacks can run with
their own flags; then either (a) change the per-callback flag handling to save
and restore the previous value around container.runGuarded (e.g. final prev =
ref._inManualInvalidationCallback; ref._inManualInvalidationCallback = true; ...
ref._inManualInvalidationCallback = prev) to guard against future invariants
changing, or (b) keep the existing set-to-false-in-finally but add a comment
referencing the current invariant (_mustRecomputeState prevents nested manual
invalidation on the same ref) so future maintainers understand why unconditional
clearing is safe.
In `@packages/riverpod/test/src/core/ref_test.dart`:
- Around line 2631-2692: The two tests "is called when invalidate is called with
asReload: true" and "is called when container.invalidate is called with
asReload: true" are duplicates (both set up Provider/Ref with
ref.onManualInvalidation(listener.call) and call container.invalidate(provider,
asReload: true)); remove or modify one: either delete the first duplicate test
(keep the later one) or change it to a distinct scenario (for example, call
container.invalidate(provider) without asReload to assert the same listener
behavior or test family invalidation with asReload), updating references to
container.invalidate(provider, asReload: true) accordingly and keeping the same
listener verification (verify(listener()).called(1)).
- Around line 3132-3189: The test only asserts that invalidating
complexAsyncDerived returns normally; enhance it to verify downstream providers
were actually invalidated and rebuilt: add build counters or mock callbacks for
asyncComputed, streamFamily(1)/streamFamily(2), and asyncNotifierProvider
(referencing asyncComputed, streamFamily, asyncNotifierProvider,
complexAsyncDerived, baseState, and TestAsyncNotifier) when defining them,
capture initial build counts after the first pump, then call
container.invalidate(complexAsyncDerived) and pump again and assert each counter
incremented (or use verify on onManualInvalidation mocks) to ensure forwarding
actually triggered rebuilds; also assert the resulting complexAsyncDerived value
changes accordingly.
- Around line 2407-2426: Remove or rename the redundant test named 'debug: basic
functionality check' in the test that constructs a Provider and uses
ref.onManualInvalidation/ref.invalidateSelf; either delete the entire test block
(the test(...) that sets up Provider, container.listen, calls ref.invalidateSelf
and asserts callCount == 1, then sub.close()) or give it a meaningful name and
ensure it doesn't duplicate the existing "is called when invalidateSelf is
called" test; locate the test by the string literal 'debug: basic functionality
check' and by references to Provider, ref.onManualInvalidation, and
ref.invalidateSelf to make the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c39fe0d0-dfcd-4210-8e82-f521e7ba31d1
📒 Files selected for processing (6)
packages/riverpod/CHANGELOG.mdpackages/riverpod/lib/src/core/element.dartpackages/riverpod/lib/src/core/provider_container.dartpackages/riverpod/lib/src/core/ref.dartpackages/riverpod/test/src/core/ref_test.dartpackages/riverpod/test/src/utils.dart
✅ Files skipped from review due to trivial changes (2)
- packages/riverpod/test/src/utils.dart
- packages/riverpod/CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/riverpod/lib/src/core/provider_container.dart
|
LGTM |
2 participants
Related Issues
fixes #3675
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]).I have updated the
CHANGELOG.mdof the relevant packages.Changelog files must be edited under the form:
If this contains new features or behavior changes,
I have updated the documentation to match those changes.
Summary by CodeRabbit
New Features
Behavior
Documentation
Tests