docs: clarify persisted state handling and add AsyncValue helper extensions - #4829
docs: clarify persisted state handling and add AsyncValue helper extensions#4829assassinaj602 wants to merge 14 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds ChangesPersisted state handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ProviderContainer
participant sortedTodosCleanProvider
participant todosProvider
ProviderContainer->>sortedTodosCleanProvider: read sortedTodosCleanProvider.future
sortedTodosCleanProvider->>todosProvider: watch AsyncValue<List<Todo>>
todosProvider-->>sortedTodosCleanProvider: loading, data, or error with optional value
sortedTodosCleanProvider-->>ProviderContainer: sorted List<Todo>
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: 4
🤖 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 `@packages/riverpod/example/persistence_example.dart`:
- Around line 39-52: Update the persisted-loading value reads in
packages/riverpod/example/persistence_example.dart:39-52 and both examples in
website/docs/concepts2/offline.mdx:192-205 and 243-262. Replace manual value ??
[] loading fallbacks with valueOr([]), or consistently use
valueIncludingLoading/hasValueIncludingLoading so retained cached values are
preserved during loading; keep the existing data and error behavior unchanged.
- Around line 17-30: Update the Todos persistence example by adding JsonPersist
configuration, its required storage provider, and the persist call in
Todos.build so saved state can be restored while loading. Make Todo serializable
for the chosen JSON mapper, including required JsonKey annotations, and preserve
the existing static Todo data as the persisted result.
In `@packages/riverpod/lib/src/core/async_value.dart`:
- Around line 163-171: Update the error callback in valueOrThrow to rethrow
error with its captured stack trace by using Error.throwWithStackTrace(error,
stack), preserving the original failure location for callers.
- Around line 144-156: Update the error branch of valueIncludingLoading in
AsyncValue so it returns the retained previous value when one exists, instead of
always returning null. Preserve null for error states without a retained value
and keep the existing data and loading behavior unchanged.
🪄 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 Plus
Run ID: 811804aa-c412-4ca1-9bc9-b13a61e09b08
📒 Files selected for processing (3)
packages/riverpod/example/persistence_example.dartpackages/riverpod/lib/src/core/async_value.dartwebsite/docs/concepts2/offline.mdx
| // ✅ CORRECT: Provider with persistence | ||
| @riverpod | ||
| class Todos extends _$Todos { | ||
| @override | ||
| FutureOr<List<Todo>> build() async { | ||
| // Simulate network delay | ||
| await Future.delayed(const Duration(seconds: 2)); | ||
|
|
||
| return [ | ||
| const Todo(id: 1, name: "Wash car"), | ||
| const Todo(id: 2, name: "Cook"), | ||
| const Todo(id: 3, name: "Clean house"), | ||
| ]; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep outline packages/riverpod/example/persistence_example.dart --items all --match Todos
rg -n -C 4 '`@JsonPersist`|persist\s*\(|storageProvider|toJson|fromJson' \
packages/riverpod/example/persistence_example.dartRepository: rrousselGit/riverpod
Length of output: 223
🏁 Script executed:
#!/bin/bash
set -u
echo "== file exists and size =="
wc -l packages/riverpod/example/persistence_example.dart
echo
echo "== relevant file contents =="
cat -n packages/riverpod/example/persistence_example.dart
echo
echo "== package/context files mentioning JsonPersist/Todo/brokenSortedTodos =="
rg -n -C 3 'JsonPersist|persist\(|toJson|fromJson|storageProvider|Todo|brokenSortedTodos|AsyncLoading' .Repository: rrousselGit/riverpod
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -u
echo "== target file line 1-120 =="
sed -n '1,120p' packages/riverpod/example/persistence_example.dart | cat -n
echo
echo "== target file line number counts for persistence markers =="
python3 - <<'PY'
from pathlib import Path
p = Path("packages/riverpod/example/persistence_example.dart")
text = p.read_text()
for label in ["`@JsonPersist`", "persist(", ".storage(", "storageProvider", "toJson", "fromJson", "Todo(", "`@JsonSerializable`", "JsonKey"]:
print(f"{label}: {text.count(label)}")
PY
echo
echo "== locate example-related docs/persistence README refs =="
rg -n -C 5 'persistence_example\.dart|brokenSortedTodos|`@JsonPersist`|`@riverpod`\n.*class Todos' . --glob '!packages/riverpod/**' || trueRepository: rrousselGit/riverpod
Length of output: 4526
🏁 Script executed:
#!/bin/bash
set -u
echo "== docs containing persistence example symbols (filtered aggressively) =="
rg -n -C 5 -U 'persistence_example\.dart|brokenSortedTodos|`@JsonPersist`|class Todos\n `@riverpod`' . \
--glob 'README.md' \
--glob '!packages/riverpod/**' || true
echo
echo "== all repo files containing persisted loading/API text (filtered by name) =="
rg -n -C 4 'persisted loading|AsyncLoading\\(value:|`@JsonPersist`\\(|`@riverpod`\\n.*class Todos|brokenSortedTodos' . \
--glob '!packages/riverpod/**' --glob '!**/.git/**' | head -n 300 || trueRepository: rrousselGit/riverpod
Length of output: 516
Use JsonPersist for this persistence example.
Todos currently only delays and returns a static Todo list; it has no @JsonPersist() annotation, storage provider, or persist call, so it cannot emit restored AsyncLoading(value: ...). Wire in storage/persistence and serializable Todo objects, including JsonKey support if using a generated mapper.
🤖 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/riverpod/example/persistence_example.dart` around lines 17 - 30,
Update the Todos persistence example by adding JsonPersist configuration, its
required storage provider, and the persist call in Todos.build so saved state
can be restored while loading. Make Todo serializable for the chosen JSON
mapper, including required JsonKey annotations, and preserve the existing static
Todo data as the persisted result.
| final todos = todosState.when( | ||
| data: (data) => data, | ||
| loading: () { | ||
| // Loading state may have persisted data | ||
| return todosState.value ?? []; | ||
| }, | ||
| error: (error, stack) { | ||
| // Error state may still have persisted data | ||
| if (todosState.hasValue) { | ||
| return todosState.value!; | ||
| } | ||
| throw error; | ||
| }, | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep outline packages/riverpod/lib/src/core/async_value.dart --items all --match AsyncValueExtensions
rg -n -C 3 'valueIncludingLoading|hasValueIncludingLoading|ValueT\? get value|bool get hasValue' \
packages/riverpod/lib/src/core/async_value.dartRepository: rrousselGit/riverpod
Length of output: 1171
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## async_value relevant implementations"
sed -n '1,230p' packages/riverpod/lib/src/core/async_value.dart
echo "## persistence example"
sed -n '30,60p' packages/riverpod/example/persistence_example.dart
echo "## offline docs relevant sections"
sed -n '178,268p' website/docs/concepts2/offline.mdx
echo "## references to loading-aware APIs in tracked docs/example"
rg -n 'valueIncludingLoading|hasValueIncludingLoading|isLoading|AsyncLoading|hasValue' packages/riverpod/example/persistence_example.dart website/docs/concepts2/offline.mdxRepository: rrousselGit/riverpod
Length of output: 12508
Use valueOr([]) or valueIncludingLoading for persisted loading states.
These examples label loading: () => todosState.value ?? [] as “correct”, but value is null on AsyncLoading(value: ...) when loading has no assigned value, so the loading branch returns [] instead of any retained cached value. Replace the manual loading fallback here with ref.watch(todosProvider).valueOr([]), or use valueIncludingLoading/hasValueIncludingLoading consistently where the retained value is read.
📍 Affects 2 files
packages/riverpod/example/persistence_example.dart#L39-L52(this comment)website/docs/concepts2/offline.mdx#L192-L205website/docs/concepts2/offline.mdx#L243-L262
🤖 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/riverpod/example/persistence_example.dart` around lines 39 - 52,
Update the persisted-loading value reads in
packages/riverpod/example/persistence_example.dart:39-52 and both examples in
website/docs/concepts2/offline.mdx:192-205 and 243-262. Replace manual value ??
[] loading fallbacks with valueOr([]), or consistently use
valueIncludingLoading/hasValueIncludingLoading so retained cached values are
preserved during loading; keep the existing data and error behavior unchanged.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@packages/riverpod/example/persistence_example.dart`:
- Line 5: Replace the pure-Dart riverpod import in the persistence example with
the flutter_riverpod import so TodoListWidget, WidgetRef, and ProviderScope
resolve from the Flutter package.
- Line 33: Update the persistence call in the example to invoke
JsonPersist.persist without awaiting its PersistResult, so restoration starts
without blocking build. Only await the returned result’s future if decode
completion is explicitly required.
🪄 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 Plus
Run ID: 3604a792-1050-48cb-8bca-49041aeb6299
📒 Files selected for processing (2)
packages/riverpod/example/persistence_example.dartpackages/riverpod/example/pubspec.yaml
| // ignore_for_file: avoid_dynamic_calls, invalid_use_of_protected_member, invalid_use_of_visible_for_testing_member | ||
|
|
||
| import 'package:flutter/material.dart'; | ||
| import 'package:riverpod/riverpod.dart'; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
sed -n '1,120p' packages/riverpod/lib/riverpod.dart
sed -n '1,140p' packages/flutter_riverpod/lib/flutter_riverpod.dartRepository: rrousselGit/riverpod
Length of output: 1677
🏁 Script executed:
#!/bin/bash
set -eu
sed -n '1,220p' packages/riverpod/example/persistence_example.dartRepository: rrousselGit/riverpod
Length of output: 5645
Import flutter_riverpod for this Flutter example.
TodoListWidget extends ConsumerWidget, uses WidgetRef, and wraps the app with ProviderScope; those Flutter widget types are exported from flutter_riverpod, so the pure-Dart riverpod import makes the example fail to compile.
Proposed fix
-import 'package:riverpod/riverpod.dart';
+import 'package:flutter_riverpod/flutter_riverpod.dart';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import 'package:riverpod/riverpod.dart'; | |
| import 'package:flutter_riverpod/flutter_riverpod.dart'; |
🤖 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/riverpod/example/persistence_example.dart` at line 5, Replace the
pure-Dart riverpod import in the persistence example with the flutter_riverpod
import so TodoListWidget, WidgetRef, and ProviderScope resolve from the Flutter
package.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@packages/riverpod/example/lib/persistence_example.dart`:
- Around line 64-68: Update the explanatory comment above
brokenSortedTodosProvider to clarify that ref.watch(todosProvider.future)
remains pending during AsyncLoading but may resolve when todosProvider later
emits AsyncData; remove the absolute claim that it will never resolve while
preserving the warning about persisted loading behavior.
- Around line 15-26: Update TodosNotifier.build to exercise the persistence flow
instead of always returning only the hard-coded list: configure the notifier's
persist call with a persistence fixture that decodes a cached Todos state,
allowing retained AsyncLoading(value: ...) and error states to be demonstrated.
If persistence cannot be wired into this example, explicitly reframe the snippet
as conceptual rather than presenting it as a working persistence example.
🪄 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 Plus
Run ID: 0d1dbe82-c94e-40f5-aba2-eb1b6aa670c1
📒 Files selected for processing (2)
packages/riverpod/example/lib/persistence_example.dartpackages/riverpod/lib/src/core/async_value.dart
| /// - In loading state with no value | ||
| /// - In error state with no value | ||
| /// - In data state with null value | ||
| ValueT? get valueIncludingLoading { |
There was a problem hiding this comment.
I have no plan on adding any new API such as this
| part 'persistence_example.g.dart'; | ||
|
|
||
| // Model | ||
| class Todo { |
There was a problem hiding this comment.
I don't think such an example would be very useful. Few people run examples to begin with.
|
|
||
| ### Correct Way to Handle Persisted Data | ||
|
|
||
| #### ❌ Wrong: Using `.future` |
There was a problem hiding this comment.
That's not wrong. Providers can opt-in to have their persisted state be AsyncData instead of AsyncLoading by calling await persist(...); state = ..
- Remove rejected AsyncValue extensions - Remove example file (not useful per maintainer feedback) - Update offline.mdx to show both handling approaches: - Approach 1: Handle AsyncLoading with value using when() - Approach 2: Promote persisted data to AsyncData - Clarify that .future is NOT wrong when used correctly Ref: rrousselGit#4802
400cc9a to
166350d
Compare
|
@rrousselGit Thank you for the feedback. I've updated the PR based on your comments: Changes Made❌ Removed
✅ Kept & Fixed
🔄 Corrected
This is now a documentation-only PR that clarifies the existing behavior without adding any new APIs. PR ready for review. |
|
@rrousselGit kindly take a look at my PR's when you become free, thanks |
- Add emitAsLoadingState parameter to persist() in core and generator - Allow emitting restored cache state as AsyncData when false - Update offline documentation and add unit test Fixes rrousselGit#4802
|
@rrousselGit I've updated the PR as suggested:
I have run the full test suite locally (dart test), and all 120 tests pass with no new failures. dart analyze and dart format are also clean. Could you please run the CI checks on your side and verify that everything works as expected? If you spot any edge cases I've missed, I'm happy to adjust. Ready for re-review! |
… changelog - Commit updated generated *.g.dart files for emitAsLoadingState parameter - Fix unnecessary underscores lint in offline_test.dart - Add CHANGELOG entry for emitAsLoadingState
2 participants
Description
This PR addresses the confusion around persisted state not being available when using .future\ on providers with \JsonPersist.
Problem
When using @JsonPersist(), the state is emitted as \AsyncLoading(value: ...)\ rather than \AsyncData. This causes .future\ to never resolve because it only listens for \AsyncData\ state. Developers expect persisted data to be available immediately.
Root Cause
Solution
1. Added Helper Extensions to AsyncValue
\\dart
extension AsyncValueExtensions on AsyncValue {
T? get valueIncludingLoading;
T get valueOrThrow;
T valueOr(T fallback);
bool get hasValueIncludingLoading;
}
\\
2. Updated Documentation
3. Added Examples
Changes Made
Testing
Related Issues
Fixes #4802
Related: #4286
Summary by CodeRabbit
New Features
Documentation
.futurein favor of state-aware reads.