feat: access providers in same scope #32
Conversation
|
Warning Rate limit exceeded@nank1ro has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 13 minutes and 1 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughAdds provider debugName support, centralizes provider creation with declaration-order tracking to allow same-scope dependencies while detecting forward references, renames/unifies scope errors, adds benchmarks and CI workflow, updates docs, tests, analysis options, and minor lint/example tweaks. (33 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ProviderScope
participant ProviderA as Provider A
participant ProviderB as Provider B
rect rgb(247,250,255)
Note over ProviderScope: Phase 1 — registration & indexing
Client->>ProviderScope: initState() / build with providers list
ProviderScope->>ProviderScope: register providers, assign indices
end
rect rgb(240,255,240)
Note over ProviderScope: Phase 2 — eager creation
ProviderScope->>ProviderA: create if non-lazy
ProviderA-->>ProviderScope: value cached
end
rect rgb(255,250,230)
Note over ProviderScope: Phase 3 — lazy/on-demand creation with same-scope checks
Client->>ProviderScope: request ProviderB value
ProviderScope->>ProviderScope: set _currentlyCreatingProviderIndex(B)
alt A declared before B
ProviderScope->>ProviderA: get (cached or create)
ProviderA-->>ProviderScope: value
ProviderScope->>ProviderB: create using ProviderA
ProviderB-->>ProviderScope: success
ProviderScope-->>Client: return value
else B declared before A
ProviderScope-->>Client: throw ProviderForwardReferenceError
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #32 +/- ##
=======================================
Coverage 99.53% 99.53%
=======================================
Files 9 9
Lines 217 217
=======================================
Hits 216 216
Misses 1 1
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/disco/test/disco_test.dart (1)
171-195: Stale test description.The test description at line 172 still references
ArgProviderWithoutScopeError, but the assertion correctly uses the renamedProviderWithoutScopeError. Update the description for consistency.testWidgets( - '''Test ProviderScope throws ArgProviderWithoutScopeError for a not found ArgProvider''', + '''Test ProviderScope throws ProviderWithoutScopeError for a not found ArgProvider''', (tester) async {
🧹 Nitpick comments (2)
packages/disco/lib/src/utils/extensions.dart (2)
3-10: Return type is unnecessarily nullable.The getter always returns a non-null
String(line 6 initializessunconditionally), but the return type is declared asString?. This can mislead callers into thinking the value might be null.Apply this diff to correct the return type:
extension _DebugNameProvider<T extends Object> on Provider<T> { // Returns a debug name for the provider. - String? get _debugName { + String get _debugName { var s = 'Provider<$_valueType>'; if (debugName != null) s += '(name: $debugName)'; return s; } }
12-19: Same nullable return type issue.Same as above - this getter always returns a non-null
String.extension _DebugNameArgProvider<T extends Object, A> on ArgProvider<T, A> { // Returns a debug name for the provider with arguments. - String? get _debugName { + String get _debugName { var s = 'ArgProvider<$_valueType, $_argumentType>'; if (debugName != null) s += '(name: $debugName)'; return s; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
examples/auto_route/analysis_options.yaml(1 hunks)examples/bloc/analysis_options.yaml(1 hunks)examples/solidart/analysis_options.yaml(1 hunks)packages/disco/CHANGELOG.md(1 hunks)packages/disco/lib/src/disco_internal.dart(1 hunks)packages/disco/lib/src/models/overrides/provider_argument_override.dart(2 hunks)packages/disco/lib/src/models/providers/provider.dart(3 hunks)packages/disco/lib/src/models/providers/provider_argument.dart(4 hunks)packages/disco/lib/src/utils/extensions.dart(1 hunks)packages/disco/lib/src/widgets/provider_scope.dart(9 hunks)packages/disco/pubspec.yaml(1 hunks)packages/disco/test/disco_test.dart(4 hunks)
🔇 Additional comments (21)
examples/auto_route/analysis_options.yaml (1)
4-4: Configuration change to suppress analyzer warning across examples.Adding
specify_nonobvious_property_typessuppression is consistent with updates to other example analysis_options.yaml files in this PR, likely due to type inference complexity in the new provider API.examples/solidart/analysis_options.yaml (1)
6-6: Configuration change consistent with other examples.Adding the same analyzer suppression rule to solidart example aligns with updates across examples in this PR.
examples/bloc/analysis_options.yaml (1)
1-3: Verify structural reordering intention.Unlike the other examples, this file restructures the YAML by moving the
analyzersection before theincludestatement. While the addition ofspecify_nonobvious_property_types: ignoreis consistent with other examples, please verify that reordering the sections was intentional, as YAML configuration order can impact how rules are applied.packages/disco/pubspec.yaml (1)
3-3: LGTM!Major version bump to 2.0.0 is appropriate given the breaking API changes (error type renames) and significant new features (same-scope provider dependencies, debugName parameter).
packages/disco/lib/src/disco_internal.dart (1)
13-13: LGTM!The new
extensions.dartpart directive follows the existing pattern and is logically placed among the utility parts.packages/disco/CHANGELOG.md (1)
1-4: LGTM!Changelog entries accurately document the new features and follow the established format.
packages/disco/lib/src/models/providers/provider.dart (3)
39-39: LGTM!The
debugNameparameter is correctly added to theProviderconstructor as an optional field.
50-52: LGTM!The
debugNameparameter is properly propagated through thewithArgumentfactory toArgProvider._.
123-127: LGTM!The
debugNamefield declaration with documentation follows the class pattern and provides clear purpose documentation.packages/disco/test/disco_test.dart (3)
517-571: LGTM!Good test coverage for the
ProviderScopePortalpath with lazy providers accessing same-scope providers, testing the cached value return and portal context navigation.
858-912: LGTM!Tests correctly validate:
- Non-lazy provider can access an earlier provider in the same scope
ProviderForwardReferenceErroris thrown when a provider references a later provider (wrong order)
1024-1067: LGTM!Excellent test for ensuring lazy provider values are cached and reused. The
creationCountassertion verifies the provider factory is only called once even when accessed by multiple dependent providers.packages/disco/lib/src/models/providers/provider_argument.dart (4)
20-20: LGTM!The
debugNameparameter is correctly added to theArgProvider._constructor.
40-41: LGTM!Correctly propagating
debugNametoArgProviderOverride._ensures debug names are preserved through override chains.
50-52: LGTM!The error type change from
ArgProviderWithoutScopeErrortoProviderWithoutScopeErrorunifies error handling across bothProviderandArgProvider, which is a cleaner API design.
85-87: LGTM!The
debugNamefield declaration follows the same pattern as inProvider, maintaining consistency across the codebase.packages/disco/lib/src/models/overrides/provider_argument_override.dart (1)
7-9: LGTM!The
debugNameparameter is properly added with appropriate nullability and immutability. The implementation follows the existing patterns in the codebase.Also applies to: 25-27
packages/disco/lib/src/widgets/provider_scope.dart (4)
221-235: Static scope tracking looks correct for typical usage.The static
_currentlyInitializingScopewith try/finally cleanup is appropriate for Flutter's synchronous widget initialization. The save/restore pattern increateProviderValue(lines 440-454) correctly handles lazy provider creation.
281-338: Two-phase initialization correctly separates registration from creation.Phase 1 registers all providers and tracks indices before Phase 2 creates non-lazy values. This ensures
isProviderInScope()works correctly during creation. The try/finally block properly cleans up static state.
646-658: Unified error handling for both provider types looks good.The
Objecttype with runtime switch pattern enables consistent error messaging for bothProviderandArgProvider. The fallback exception (line 653) provides a clear failure path if an unexpected type is encountered.
435-455: Save/restore pattern for lazy provider creation is well implemented.The try/finally correctly saves and restores scope tracking state, enabling lazy providers to access earlier same-scope providers while maintaining proper cleanup on exceptions.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/disco/lib/src/widgets/provider_scope.dart (1)
675-741: LGTM! Well-designed error types with clear messaging.The renamed
MultipleProviderOverrideOfSameInstanceimproves clarity. The newProviderForwardReferenceErroris particularly well-designed, providing comprehensive context (indices, provider references) and an actionable error message that explains both the problem and the solution.Note: There's a typo in the error message at line 734 (missing closing backtick) that was already flagged in a previous review.
packages/disco/test/disco_test.dart (1)
859-1221: Excellent comprehensive test coverage for same-scope provider access.The test suite thoroughly exercises the new same-scope provider access feature, covering:
- Both lazy and non-lazy providers
- Provider and ArgProvider interactions
- Nested dependencies (A→B→C chains)
- Value reuse/caching when multiple providers access the same earlier provider
- Forward reference error scenarios
- Mixed Provider and ArgProvider ordering
This coverage ensures the feature works correctly across various scenarios and properly detects ordering violations.
Note: The test at lines 1189-1221 has a description mismatch already flagged in a previous review.
🧹 Nitpick comments (1)
packages/disco/lib/src/widgets/provider_scope.dart (1)
241-411: Consider extracting helper methods to improve readability.The
initStatemethod is quite long (~170 lines) and handles multiple responsibilities: validation, two-phase provider registration, and override processing. While functionally correct, extracting helper methods would improve maintainability and testability.Consider refactoring into smaller methods:
void initState() { super.initState(); _currentlyInitializingScope = this; try { if (widget.providers != null) { _initializeProviders(widget.providers!); } else if (widget.overrides != null) { _initializeOverrides(widget.overrides!); } } finally { _currentlyInitializingScope = null; _currentlyCreatingProviderIndex = null; } } void _initializeProviders(List<InstantiableProvider> allProviders) { _validateProvidersUniqueness(allProviders); _registerAllProviders(allProviders); _createNonLazyProviders(allProviders); } void _initializeOverrides(List<Override> overrides) { _processProviderOverrides(overrides); _processArgProviderOverrides(overrides); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/disco/lib/src/widgets/provider_scope.dart(9 hunks)packages/disco/test/disco_test.dart(4 hunks)
🔇 Additional comments (9)
packages/disco/lib/src/widgets/provider_scope.dart (6)
31-36: LGTM! Clear documentation of ordering requirements.The documentation clearly explains the ordering constraint for same-scope provider dependencies and references the error type that will be thrown. This will help developers understand the design decision upfront.
63-141: Good refactoring to centralize provider value creation logic.The centralized
_getOrCreateValuehelper effectively reduces code duplication betweenProviderandArgProvideraccess paths while enabling consistent same-scope dependency checking. The two-phase approach (check initializing scope, then search ancestors) is logical and well-structured.
435-455: LGTM! Proper save/restore pattern for lazy provider creation.The save/restore pattern for initialization state correctly enables same-scope dependency checking for lazy providers. The try-finally ensures proper cleanup even if value creation throws.
474-497: LGTM! Consistent save/restore pattern for ArgProvider.The same save/restore pattern is correctly applied to
ArgProviderlazy creation, maintaining consistency with theProviderimplementation.
646-658: LGTM! Improved error type with dynamic naming.The updated error type now accepts any provider type and uses dynamic naming via the new
debugNamesupport. The switch expression cleanly handles bothProviderandArgProvidercases with appropriate fallback for unexpected types.
221-235: The static_currentlyInitializingScopeis correctly handled for nested initialization. Lines 440-453 implement a proper save/restore pattern that preserves and restores the previous scope state, preventing any premature overwriting during nested initialization. Existing tests at lines 445-500 ("Test ProviderScope inside ProviderScopePortal works") and line 998 ("Nested provider dependencies work") already verify that nested scope initialization with cross-scope provider access works correctly. No changes are needed.packages/disco/test/disco_test.dart (3)
190-191: LGTM! Correctly updated to use unified error type.The test correctly reflects the API change from
ArgProviderWithoutScopeErrorto the unifiedProviderWithoutScopeErrortype with theproviderfield.
517-571: LGTM! Good test coverage for ProviderScopePortal with same-scope dependencies.This test effectively covers the interaction between
ProviderScopePortal, lazy providers, and same-scope dependencies. It verifies that lazy providers can access earlier providers in the same scope even when accessed through a portal context.
826-826: LGTM! Updated to match renamed error type.Correctly updated to expect
MultipleProviderOverrideOfSameInstancematching the renamed error type.
|
@manuel-plavsic let me know if you can review the PR given that I already tried to merge something similar in the past but failed. PS: I bumped a major, reaching v2 because I want to create another PR to create a new package |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/disco/lib/src/widgets/provider_scope.dart (2)
462-482: Correct same-scope support for lazy providers.The save/restore pattern for
_currentlyInitializingScopeand_currentlyCreatingProviderIndexis essential for nested lazy provider creation. The try-finally ensures proper restoration even if creation throws.The save/restore pattern is duplicated in both
createProviderValue(lines 467-481) andcreateProviderValueForArgProvider(lines 509-523). Consider extracting this pattern into a helper method to reduce duplication:T _withProviderIndex<T>(int? index, T Function() create) { final savedScope = _currentlyInitializingScope; final savedIndex = _currentlyCreatingProviderIndex; try { _currentlyInitializingScope = this; _currentlyCreatingProviderIndex = index; return create(); } finally { _currentlyInitializingScope = savedScope; _currentlyCreatingProviderIndex = savedIndex; } }However, since it's only used in two places, the current approach is acceptable.
442-452: Consider clearing index maps in dispose for consistency.The
disposemethod clears the main hash maps but not the new index maps (_providerIndicesand_argProviderIndices). While these will be garbage collected when the State object is disposed, explicitly clearing them would be more consistent with the existing cleanup pattern.Add these lines in dispose for consistency:
@override void dispose() { // dispose all the created providers createdProviderValues.forEach((key, value) { key._safeDisposeValue(value); }); allArgProvidersInScope.clear(); allProvidersInScope.clear(); createdProviderValues.clear(); _providerIndices.clear(); _argProviderIndices.clear(); super.dispose(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/disco/lib/src/widgets/provider_scope.dart(9 hunks)
🔇 Additional comments (17)
packages/disco/lib/src/widgets/provider_scope.dart (17)
31-36: Clear documentation of the ordering requirement.The documentation clearly explains the ordering constraint and references the appropriate error type. This will help developers understand the feature and avoid forward-reference errors.
63-143: Excellent centralization of provider access logic.The
_getOrCreateValuehelper elegantly handles bothProviderandArgProvideraccess with proper forward-reference detection. The two-step approach (check initialization scope first, then search ancestors) is well-structured and maintainable.The ordering validation correctly prevents both forward references and self-references by checking
requestedIndex >= currentIndex.
159-168: Clean refactoring to use the centralized helper.Delegating to
_getOrCreateValueeliminates duplication and ensures consistent behavior across both provider types.
191-201: Clean refactoring to use the centralized helper.Delegating to
_getOrCreateValueeliminates duplication and ensures consistent behavior across both provider types.
223-237: Well-designed tracking fields for initialization.The combination of static scope tracking and instance-level index tracking enables robust same-scope dependency resolution while preventing circular references. The comments clearly explain each field's purpose.
243-256: Proper initialization with cleanup guarantees.The try-finally block ensures that
_currentlyInitializingScopeand_currentlyCreatingProviderIndexare always cleared, even if initialization throws. This prevents stale state from affecting subsequent operations.
258-293: Thorough uniqueness validation.The separate validation loops for
ProviderandArgProvidermake the logic clear and maintainable. The assertions appropriately catch configuration errors during development.
295-325: PHASE 1: Well-structured provider registration.Registering all providers before creating any is essential for same-scope access to work correctly. The separation of concerns between registration and creation makes the initialization flow easy to follow.
327-356: PHASE 2: Correct non-lazy provider creation.Setting and clearing
_currentlyCreatingProviderIndexfor each provider ensures accurate forward-reference detection. The sequential creation in declaration order is essential for the same-scope dependency feature.
358-363: Clear initialization orchestration.The three-step initialization flow is well-organized and easy to understand.
429-439: Clear override initialization orchestration.Separating provider and arg provider override processing keeps the logic modular and maintainable.
365-394: Robust provider override processing.The uniqueness validation and immediate value creation for overrides is correct. The renamed error class (
MultipleProviderOverrideOfSameInstance) is more specific than the old name.
396-427: Robust arg provider override processing.Consistent with the provider override logic. Proper validation and immediate creation of override values.
501-524: Correct same-scope support for lazy arg providers.Consistent with
createProviderValue. The save/restore pattern properly handles nested lazy provider creation.
673-687: Improved error with dynamic naming.Using the provider's
debugNamein error messages significantly improves debuggability. The switch expression cleanly handles bothProviderandArgProvidertypes.
704-716: Improved error class naming.The shortened name
MultipleProviderOverrideOfSameInstanceis clearer and more concise while maintaining the same meaning. Appropriate for the v2 major version bump.
718-774: Excellent error class with actionable guidance.The error message is exceptionally clear and helpful:
- Explains the forward reference with specific indices
- Describes why the constraint exists (prevent circular dependencies)
- Provides concrete fix instructions (reorder the providers)
This will significantly improve the developer experience when encountering ordering issues.
Deploying flutter-disco with
|
| Latest commit: |
8cf5f27
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d4844d35.flutter-disco.pages.dev |
| Branch Preview URL: | https://feat-access-providers-in-sam.flutter-disco.pages.dev |
nank1ro
force-pushed
the
feat/access-providers-in-same-scope
branch
from
70a8946 to
6d02e04
Compare
|
@copilot Do you think the new implementation can be improved by avoiding for loops and using maps only? |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/disco/test/disco_test.dart (1)
811-827: Test description still references old error nameThe test description still says “MultipleProviderOverrideOfSameProviderInstance” while the expectation now uses
MultipleProviderOverrideOfSameInstance. Functionally the test is correct, but the wording is slightly stale.You can align the description with the new type name:
- testWidgets( - '''ProviderScopeOverride must throw a MultipleProviderOverrideOfSameProviderInstance for duplicated providers''', + testWidgets( + '''ProviderScopeOverride must throw a MultipleProviderOverrideOfSameInstance for duplicated providers''', (tester) async {
🧹 Nitpick comments (4)
.github/workflows/benchmark.yaml (1)
52-68: README update logic assumes Benchmark is the final section.The sed command (line 61) removes everything from "## Benchmark" to the end of file. If other sections are added after the Benchmark section in the future, they will be accidentally deleted. Consider a more defensive approach.
For robustness, replace the sed-based deletion with explicit section replacement:
- name: Update README with benchmark results if: github.event_name == 'push' && (github.ref == 'refs/heads/main' || github.ref == 'refs/heads/dev') run: | # Read benchmark results BENCHMARK_CONTENT=$(cat packages/disco/benchmark_results.md) # Check if README already has a Benchmark section if grep -q "^## Benchmark" packages/disco/README.md; then # Remove everything from ## Benchmark to the end - sed -i '/^## Benchmark/,$d' packages/disco/README.md + # More robust: extract everything before ## Benchmark + sed -i '/^## Benchmark/,$d' packages/disco/README.md fi # Add benchmark section at the end echo "" >> packages/disco/README.md echo "## Benchmark" >> packages/disco/README.md echo "" >> packages/disco/README.md echo "$BENCHMARK_CONTENT" >> packages/disco/README.mdAlternatively, use a marker-based approach if other sections may follow Benchmark:
sed -i '/^## Benchmark/,/^## [^B]/d; /^## Benchmark/d' packages/disco/README.mdpackages/disco/lib/src/widgets/provider_scope.dart (3)
63-133: Same‑scope creation + forward‑reference detection logic looks correct; note behaviour for overridesThe combination of:
_getOrCreateValue’s same‑scope branch,_currentlyInitializingScope/_currentlyCreatingProviderIndex/_currentlyCreatingProvider,- index maps populated in
_registerAllProviders, and- the wrapping in
createProviderValue/createProviderValueForArgProvidergives a coherent model:
- Forward references in the same
ProviderScope(requested index ≥ current index) reliably throwProviderForwardReferenceErrorin both init and lazy creation.- Earlier providers (including lazy ones) in the same scope are created on demand and reused via
createdProviderValues, with static state correctly saved/restored through nested calls and cross‑scope access.- Cross‑scope reads skip the same‑scope branch via
isInScopeand fall back to the usual ancestor/portal lookup.One nuance: overrides initialized via
_initializeOverridesdon’t populate_providerIndices/_argProviderIndicesor set_currentlyCreatingProviderIndex, so forward‑reference detection and “same‑scope” semantics effectively do not apply within override lists. That’s perfectly safe, but it’s an implicit difference fromproviders; if you ever want overrides to participate in the same ordering rules, you’d need a similar “phase 1 register + phase 2 create with index tracking” flow there too.Also applies to: 213-253, 279-351, 450-518
255-277: Duplicate‑provider checks are assert‑only; confirm that dev‑only enforcement is desired
_validateProvidersUniqueness,_processProviderOverrides, and_processArgProviderOverridesall throwMultipleProviderOfSameInstance/MultipleProviderOverrideOfSameInstanceonly from insideassertblocks. This mirrors the existing pattern, but means:
- In debug/tests: duplicates throw as expected.
- In release: duplicates are silently allowed, and later entries just overwrite earlier ones in the maps.
If the intent is to treat duplicates as programmer errors that must never reach production, the current design is fine. If you want consistent runtime behaviour between debug and release, consider moving the checks out of
assertor duplicating them in non‑assert code.Also applies to: 353-427
659-681: Error type refinements and messages align with new semantics
ProviderWithoutScopeErrornow takes a genericObject providerand derives a debug name for bothProviderandArgProvider, which matches the updated tests and makes messages clearer.MultipleProviderOverrideOfSameInstancecleanly distinguishes override duplication from base provider duplication.ProviderForwardReferenceErrorcarries bothcurrentProviderandrequestedProviderand formats them via_debugName, producing a concise, actionable error that aligns with the declaration‑order rules.The defensive
switchfallbacks that throw for unknown provider types should be unreachable given the current API surface; if you later introduce new provider‐like types, you might want to either broaden the pattern or fall back toruntimeTypeinstead of throwing fromtoString, but as written this is acceptable for an “impossible” branch.Also applies to: 698-710, 712-757
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/benchmark.yaml(1 hunks)packages/disco/.pubignore(1 hunks)packages/disco/benchmark/provider_benchmark.dart(1 hunks)packages/disco/lib/src/widgets/provider_scope.dart(12 hunks)packages/disco/test/disco_test.dart(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/disco/.pubignore
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/disco/benchmark/provider_benchmark.dart
🔇 Additional comments (6)
.github/workflows/benchmark.yaml (1)
83-119: GitHub Script for PR comments is well-structured.The logic correctly detects existing bot comments by checking for both Bot user type and the comment text identifier. Updating vs. creating comments prevents duplication and keeps the PR clean.
packages/disco/lib/src/widgets/provider_scope.dart (2)
31-36: Doc comment clearly communicates ordering contractThe added documentation on
providersexplicitly states the declaration-order constraint and theProviderForwardReferenceErrorbehaviour; this matches the implementation and will help users avoid subtle bugs.
536-544: Coverage ignores around debug‑only InheritedWidget hooks are reasonableMarking
debugFillProperties,updateShouldNotify, andupdateShouldNotifyDependentwith coverage ignores keeps noise out of coverage reporting while still exercising the main provider logic elsewhere. No functional concerns here.Also applies to: 553-583
packages/disco/test/disco_test.dart (3)
171-195: Updated ArgProvider “not found” test matches new error APIThe test now correctly expects
ProviderWithoutScopeErrorand inspects the unified.providerfield, asserting it equals theArgProviderinstance. This aligns with the refactored error type and ensures ArgProviders are covered by the same error path as regular Providers.
517-571: Portal + same‑scope lazy provider test exercises the new common pathThe
ProviderScopePortal with lazy providers accessing same‑scope providerstest nicely validates that:
- Lazy providers declared in the same
ProviderScopecan depend on each other via declaration order, and- Access through
ProviderScopePortalstill hits the same_getOrCreateValuepath (including caching and same‑scope semantics).This is a good regression test for the portal + same‑scope integration.
859-1305: New same‑scope, forward‑reference, caching, and circular‑dependency tests provide strong coverageThe added block of tests around:
- same‑scope access for combinations of lazy/non‑lazy Providers and ArgProviders,
- forward references in both directions (Provider→Provider, ArgProvider→Provider, Provider→ArgProvider, ArgProvider→ArgProvider),
- reuse of previously created values when multiple dependents access the same earlier provider, and
- “impossible by design” circular dependencies that surface as
ProviderForwardReferenceErrorgives very thorough coverage of the new ordering semantics. The scenarios mirror the implementation details (index maps, initialization vs lazy creation, mixed Provider/ArgProvider lists) and should catch regressions in
_getOrCreateValueor the index bookkeeping.No functional issues spotted; this suite looks well designed for the new behaviour.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/disco/test/disco_test.dart (1)
1189-1221: Later ArgProvider forward-reference test now matches error typeThe test name and expectation both target
ProviderForwardReferenceError, addressing the previously noted mismatch between description and asserted type.
🧹 Nitpick comments (2)
packages/disco/test/disco_test.dart (2)
517-571: Portal + lazy same-scope test exercises the right code pathsThis test nicely covers
ProviderScopePortalwith lazy providers that depend on an earlier same-scope provider, ensuring the portal path and cached-value resolution behave as expected. One minor readability nit: consider renaming the localdoublevariable to something likedoubleValueto avoid shadowing thedoubletype.
859-1187: Strong coverage of same-scope ordering, laziness, ArgProviders, and cachingThis block of tests thoroughly exercises the new semantics: non-lazy vs lazy combinations, Provider↔ArgProvider interactions, multi-step chains (A→B→C), and reuse of a single created value when multiple providers read from the same earlier provider. It should give good regression protection around same-scope access and caching behaviour. If you ever want to reduce duplication, many of these
MaterialApp/Scaffold/ProviderScope+Builderscaffolds could be factored into a small helper, but that’s optional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/src/content/docs/miscellaneous/reactivity.md(1 hunks)packages/disco/test/disco_test.dart(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (4)
docs/src/content/docs/miscellaneous/reactivity.md (1)
16-25: Example now matches same-scope provider semanticsRegistering both
counterProvideranddoubleCounterProviderin a singleProviderScopein dependency order, along with the fixedComputedreturn line, correctly illustrates the new same-scope access model and forward-reference behaviour.packages/disco/test/disco_test.dart (3)
171-195: ArgProvider not-found test now matches unified ProviderWithoutScopeError APIThe description,
TypeMatcher<ProviderWithoutScopeError>, and use oferror.providerare consistent and aligned with the non-Arg provider case, which keeps the error surface coherent.
811-827: Override duplication test correctly targets MultipleProviderOverrideOfSameInstanceThe test description and expectation now both use
MultipleProviderOverrideOfSameInstance, clearly separating override-duplication failures fromMultipleProviderOfSameInstancein regular scopes.
1223-1305: Circular-dependency prevention tests clearly encode the forward-reference invariantBoth scenarios (Provider→Provider and Provider→ArgProvider) demonstrate that any would-be cycle in a single scope must hit a forward reference first and thus raise
ProviderForwardReferenceError, which captures the intended design constraint and helps guard against regressions.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/disco_lint/lib/src/assists/base/wrap_builder.dart (1)
1-8: Implementation imports from internal analyzer APIs create maintenance risk.The code imports three internal APIs from analyzer's
/src/paths (lines 5-7):
analyzer/src/dart/ast/extensions.dartanalyzer/src/dart/element/type.dartanalyzer/src/utilities/extensions/flutter.dartWhile the
analyzer: ^8.0.0constraint in pubspec.yaml prevents major version breakage, it does not protect against breaking changes in minor versions. The analyzer package is known to make breaking changes within minor versions without notice, creating fragility.Add inline comments explaining why each implementation import is necessary and what public analyzer or analyzer_plugin API would serve as an alternative if available. Consider pinning analyzer more tightly (e.g.,
^8.1.0or8.1.x) if targeting specific known-stable versions.
🧹 Nitpick comments (1)
packages/disco/analysis_options.yaml (1)
14-16: Reconsider suppressing documentation warnings for a major version release.Adding
document_ignores: ignoresilences all documentation-related analyzer warnings globally. For a major version bump (v2) that introduces significant API changes (debugName parameter, provider ordering semantics), this risks masking real documentation gaps that should be addressed—especially for public APIs that consumers depend on.If specific files legitimately lack documentation by design, prefer using file-level ignore comments (e.g.,
// ignore: document_ignoresor/// ignore: document_ignores) rather than project-wide suppression.Please clarify:
- Why is this suppression necessary for the v2 release?
- Are there intentionally undocumented public APIs, or should these be documented instead?
- If suppression is needed, would file-level ignores be more appropriate than project-wide?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
examples/solidart/analysis_options.yaml(1 hunks)packages/disco/CHANGELOG.md(1 hunks)packages/disco/analysis_options.yaml(1 hunks)packages/disco/pubspec.yaml(1 hunks)packages/disco/test/disco_test.dart(17 hunks)packages/disco_lint/example/pubspec.yaml(1 hunks)packages/disco_lint/lib/src/assists/base/wrap_builder.dart(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/disco/CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/disco/pubspec.yaml
- examples/solidart/analysis_options.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Flutter test
- GitHub Check: Run Provider Benchmarks
🔇 Additional comments (8)
packages/disco_lint/example/pubspec.yaml (1)
14-15: LGTM!The switch to a local path dependency aligns with the workspace setup and is the appropriate pattern for an example project during development. This allows the example to test against the latest local disco package changes.
packages/disco/test/disco_test.dart (6)
9-9: LGTM! Appropriate lint suppression.The
unreachable_from_mainignore is correct sinceNameContaineris only used in test code.
167-172: LGTM! Error type and field updates are consistent.The renamed error types (
ProviderWithoutScopeError,MultipleProviderOfSameInstance) and unified field access (.provider) are correctly applied throughout the test suite, matching the PR's public API changes.Also applies to: 175-201, 203-228
385-385: LGTM! Proper future handling in tests.Using
.ignore()on unawaitedshowDialogfutures is the correct approach to suppress theunawaited_futureslint in test scenarios where the dialog result is not needed.Also applies to: 441-441, 515-515, 573-573
885-1251: Excellent comprehensive test coverage for same-scope provider access!This test suite thoroughly validates the new same-scope provider dependency feature:
- Tests all combinations of lazy/non-lazy providers
- Correctly validates forward-reference detection
- Verifies value caching when multiple providers access the same dependency
- Tests nested dependency chains (A→B→C)
- Covers both
ProviderandArgProviderscenariosThe test logic and expectations are correct throughout. This provides strong confidence in the feature's correctness.
1253-1339: Excellent demonstration of circular dependency prevention by design!These tests effectively validate that the declaration-order enforcement makes circular dependencies impossible within the same scope. Any attempt to create circular dependencies results in
ProviderForwardReferenceError, which aligns perfectly with the PR's stated goal of "preventing circular dependencies by design."The test comments clearly explain the prevention mechanism, making this a valuable documentation reference.
536-589: LGTM! Important edge case coverage.This test validates that same-scope provider dependencies work correctly through
ProviderScopePortal, including proper context navigation and cached value retrieval. This is an important validation of the feature's robustness across different widget tree patterns.packages/disco_lint/lib/src/assists/base/wrap_builder.dart (1)
10-84: LGTM!The WrapBuilder abstraction is well-designed with proper null safety, clean separation of concerns, and correct indentation handling for generated code. The use of early returns keeps the logic clear, and the builder pattern integration with analyzer_plugin APIs is implemented correctly.
|
I'll have time to review it this week. Probably tomorrow or on Thursday ;) |
manuel-plavsic
left a comment
manuel-plavsic
left a comment
There was a problem hiding this comment.
I am still not finishing reviewing. I still need to review the main part of the provider_scope.dart file. I will do it ASAP.
I like the idea of a user-friendlier API 👍
|
|
||
| /// Track the scope currently being initialized. This enables same-scope | ||
| /// provider access during initialization. | ||
| static ProviderScopeState? _currentlyInitializingScope; |
There was a problem hiding this comment.
I guess this can be solved like this... I am just a bit unsure. Are we basically using global state for the current scope?
There was a problem hiding this comment.
In some sorts yes, but the variable has a value only during initialization.
After initState is completed, the value resets to null
manuel-plavsic
left a comment
manuel-plavsic
left a comment
There was a problem hiding this comment.
There are a few things to adjust, bit it LGTM otherwise. The core logic seems sound. Good job!
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @packages/disco/benchmark/provider_benchmark.dart:
- Around line 258-286: The mid/top provider loops currently index into the
growing providers list and end up referencing same-tier providers; fix by using
a loop-local index relative to the intended tier instead of absolute i into
providers. For mid providers (Provider with debugName 'mid$i') compute k = i -
10 and read base providers as providers[k] and providers[k + 1] (or iterate j
from 0..9 and use providers[j], providers[j+1]); for top providers (debugName
'top$i') compute k = i - 20 and read mid providers as providers[10 + k] and
providers[10 + k + 1] (or iterate j from 0..9 and use providers[10 + j],
providers[10 + j + 1]). This ensures Provider(...) closures reference only the
lower-tier entries rather than same-tier entries.
In @packages/disco/lib/src/widgets/provider_scope.dart:
- Around line 381-412: In _processArgProviderOverrides change the thrown
exception from MultipleProviderOfSameInstance to
MultipleProviderOverrideOfSameInstance inside the assert block that checks
duplicate ArgProvider instances (the code that iterates argProviderOverrides and
uses override._argProvider and ids list); keep the same duplicate-detection
logic and only swap the exception class so both _processArgProviderOverrides and
_processProviderOverrides raise the same override-related error type.
🧹 Nitpick comments (4)
packages/disco/benchmark/provider_benchmark.dart (3)
79-123: Inconsistent timing: provider creation excluded from measurement.The stopwatch starts at line 108, after the provider list is fully populated (lines 86-106). This measures only the
pumpWidgettime, not the provider object creation. Compare to lines 25-49 whereStopwatch()..start()precedesList.generate.For consistent benchmarking, either move the stopwatch start before line 83, or clarify in the benchmark name that only scope initialization is measured.
♻️ Suggested fix: start stopwatch before provider creation
testWidgets('Benchmark: Create 50 providers with dependencies', ( tester, ) async { + final stopwatch = Stopwatch()..start(); + // Create a chain of providers where each depends on the previous one final providers = <Provider>[]; // ... rest of provider creation ... } - final stopwatch = Stopwatch()..start(); await tester.pumpWidget(
459-473: Consider verifying within the same scope.The second
pumpWidgetcreates a newProviderScope, so the verification is not confirming the first scope's values—just that the chain logic is correct in general. To verify the exact scope that was timed, use aBuilderinside the first widget tree.♻️ Suggested refactor: verify within same scope
await tester.pumpWidget( MaterialApp( home: ProviderScope( providers: providers, - child: Container(), + child: Builder( + builder: (context) { + final lastValue = providers.last.of(context) as int; + expect(lastValue, 99); + return Container(); + }, + ), ), ), ); stopwatch.stop(); // ... timing code ... - - // Verify the final value is correct - await tester.pumpWidget( - MaterialApp( - home: ProviderScope( - providers: providers, - child: Builder( - builder: (context) { - final lastValue = providers.last.of(context) as int; - expect(lastValue, 99); - return Container(); - }, - ), - ), - ), - );
596-599: Consider adding error handling for file write.If the file write fails (e.g., permissions, disk full), the exception will propagate and may obscure the benchmark results printed to stdout. A try-catch with a fallback message would improve robustness.
♻️ Suggested: wrap file write in try-catch
// Write to file final file = File('benchmark_results.md'); - file.writeAsStringSync(buffer.toString()); - print('\n✓ Benchmark results written to: ${file.absolute.path}'); + try { + file.writeAsStringSync(buffer.toString()); + print('\n✓ Benchmark results written to: ${file.absolute.path}'); + } on FileSystemException catch (e) { + print('\n✗ Failed to write benchmark results: $e'); + print('\n--- Benchmark Results (stdout fallback) ---'); + print(buffer.toString()); + }packages/disco/lib/src/widgets/provider_scope.dart (1)
448-473: State save/restore pattern enables nested provider creation.The try-finally structure with state preservation is essential for handling nested dependencies (e.g., provider A accessing provider B during A's creation). The pattern correctly restores state even when creation throws.
Based on past review comments, consider adding more detailed inline comments explaining the state management flow, especially around the finally block's role in restoring state for both success and error cases.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
docs/src/content/docs/core/scoped-di.mdxdocs/src/content/docs/installing.mdexamples/auto_route/lib/router.dartexamples/bloc/lib/main.dartexamples/preferences/lib/main.dartexamples/solidart/lib/domain/todo.dartexamples/solidart/lib/widgets/toolbar.dartpackages/disco/CHANGELOG.mdpackages/disco/analysis_options.yamlpackages/disco/benchmark/provider_benchmark.dartpackages/disco/lib/src/models/overrides/provider_argument_override.dartpackages/disco/lib/src/models/overrides/provider_override.dartpackages/disco/lib/src/models/providers/provider.dartpackages/disco/lib/src/models/providers/provider_argument.dartpackages/disco/lib/src/widgets/provider_scope.dartpackages/disco/lib/src/widgets/provider_scope_override.dartpackages/disco/lib/src/widgets/provider_scope_portal.dartpackages/disco/test/disco_test.dart
💤 Files with no reviewable changes (1)
- packages/disco/analysis_options.yaml
✅ Files skipped from review due to trivial changes (6)
- examples/bloc/lib/main.dart
- examples/solidart/lib/widgets/toolbar.dart
- packages/disco/lib/src/models/overrides/provider_override.dart
- packages/disco/lib/src/widgets/provider_scope_override.dart
- packages/disco/lib/src/widgets/provider_scope_portal.dart
- examples/solidart/lib/domain/todo.dart
🚧 Files skipped from review as they are similar to previous changes (5)
- docs/src/content/docs/core/scoped-di.mdx
- packages/disco/lib/src/models/providers/provider.dart
- packages/disco/lib/src/models/providers/provider_argument.dart
- packages/disco/lib/src/models/overrides/provider_argument_override.dart
- packages/disco/CHANGELOG.md
🔇 Additional comments (16)
docs/src/content/docs/installing.md (1)
47-54: Clarify whether this setting is required or optional for new features.The new section helpfully documents analyzer configuration, but the heading "Recommended" is ambiguous. Given that this PR introduces features like same-scope provider dependencies and type inference, clarify whether this setting is essential for those features to work properly, or truly optional for convenience. If it's critical, consider upgrading the label from "Recommended" to "Required".
examples/preferences/lib/main.dart (3)
53-53: LGTM! Formatting improvement.The condensed error UI expression is more concise while maintaining the same functionality.
58-58: LGTM! Formatting improvement.The single-line providers array is more concise for a single provider.
85-85: LGTM! Formatting improvement.The condensed Center widget expression is cleaner and more readable.
examples/auto_route/lib/router.dart (1)
13-25: LGTM! Formatting improvements with no functional changes.The route configuration remains functionally identical; only indentation and formatting have been adjusted for improved readability.
packages/disco/benchmark/provider_benchmark.dart (3)
1-18: LGTM!The imports and global state setup are appropriate for a benchmark suite. Using a global map to collect results across multiple test cases before writing them out is a reasonable pattern for this use case.
518-549: Clarify intent: same provider instances across all 5 nested scopes.The same
providerslist is passed to each nestedProviderScope. If this tests override/shadowing behavior, consider adding a comment. If distinct providers per scope are intended, generate separate lists.
209-238: Benchmark timing includes both scope creation and provider access.Unlike other benchmarks, this one starts the stopwatch before
pumpWidget(line 209) but also includes the provider access loops (lines 220-226) inside theBuilder. The measured time conflates scope initialization with value retrieval. If only access time is intended, move stopwatch start/stop inside theBuilder.packages/disco/test/disco_test.dart (2)
3-4: LGTM - reasonable for test files.The
document_ignoressuppression is appropriate for test code that accesses internal APIs, though be mindful that it broadly suppresses documentation warnings.
887-1342: Excellent comprehensive test coverage for same-scope provider access.This test suite thoroughly exercises the new same-scope provider access feature with well-organized coverage of:
- Declaration-order enforcement (providers can only access earlier providers)
- Forward-reference error detection for both Provider and ArgProvider
- Lazy and non-lazy provider interactions
- Value reuse across multiple dependent providers
- Nested dependency chains (A→B→C)
- Circular dependency prevention
The tests are well-named, clearly document expected behavior, and include both happy paths and error scenarios.
packages/disco/lib/src/widgets/provider_scope.dart (6)
58-129: Well-designed centralized helper for same-scope provider access.The
_getOrCreateValuemethod effectively implements the core same-scope access logic with proper ordering validation. The two-step approach (check initializing scope first, then search ancestors) is sound.One observation: The cached value return path at line 104 is excluded from coverage. While the logic appears correct, this suggests the test suite might not exercise the scenario where a lazy provider accesses an already-created earlier provider during another provider's initialization. The test at lines 1060-1102 in disco_test.dart should cover this, so consider verifying the coverage exclusion is intentional.
209-228: State tracking fields enable robust same-scope access.The combination of static initialization tracking and per-scope index maps provides the foundation for forward-reference detection. The static
_currentlyInitializingScopeis safe in Flutter's single-threaded environment, and the try-finally blocks in creation methods ensure proper cleanup.
234-249: Try-finally pattern ensures clean state management.The initialization structure properly sets up same-scope tracking before provider creation and guarantees cleanup regardless of success or failure. This defensive approach prevents state pollution across multiple ProviderScope instances.
251-347: Phased initialization correctly enables same-scope access.The two-phase approach (register all providers, then create non-lazy ones) is necessary to ensure
isProviderInScope()works correctly during provider creation. Setting and clearing_currentlyCreatingProviderIndexaround each creation enables precise forward-reference detection.The clear separation of concerns and explanatory comments make the initialization flow easy to follow.
669-684: Error unification improves consistency and usability.Consolidating error handling for both
ProviderandArgProviderinto a singleProviderWithoutScopeErrorclass simplifies the API. The dynamic naming via_debugNameprovides clear error messages regardless of provider type.
714-762: Excellent error messaging for forward reference detection.The
ProviderForwardReferenceErrorprovides exceptionally clear and actionable guidance:
- Explains what went wrong (forward reference detected)
- States the constraint (can only access earlier providers)
- Provides specific fix instructions (move provider order)
This will significantly improve developer experience when encountering ordering issues.
ProviderScopeto depend on each other, by leveraging the order of declaration. This simplifies the development experience and prevents circular dependencies by design.debugNameparameter to providers for easier debugging, allowing better identification of providers in error messages and logs.This now works:
while by reversing the order of the providers declaration (
doubleCounterProviderbeforecounterProvider) you get this runtime error:This eliminates the possibility to create a circular dependency.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.