Conversation
… splash screen display Convert SplashBuilder to StatefulWidget to track trigger-caused refreshes. Add _triggerCausedRefresh flag to distinguish between trigger changes (show splash) and other refreshes like errors or dependency changes (don't show splash). Update reactive task completion logic to skip splash when errors occur without trigger change. Set trigger flag on manual retry. Clear flag after reactive task completes using
Summary of ChangesHello @pckimlong, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the splash screen behavior in the application by introducing more granular control over when the splash screen is displayed during reactive task execution. Previously, errors or dependency reloads could inadvertently cause the splash screen to flash, disrupting the user experience. By converting the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses an issue where the splash screen was unnecessarily displayed during reactive task errors that were not caused by a trigger change. The conversion of SplashBuilder to a ConsumerStatefulWidget and the introduction of the _triggerCausedRefresh flag is a solid approach to distinguish between different refresh causes. The accompanying widget test is well-written and correctly verifies the fix.
My main feedback concerns the implementation of the state management for the _triggerCausedRefresh flag. The current approach involves side effects within the build method and state mutations without setState, which goes against Flutter's best practices and can lead to fragile, hard-to-maintain code. I've provided a detailed suggestion to refactor this using a more idiomatic Riverpod and Flutter pattern, which will improve the robustness and clarity of the implementation.
| ref.listen(_reactiveTaskTriggerProvider, (_, _) { | ||
| _triggerCausedRefresh = true; | ||
| ref.invalidate(_reactiveTaskRunProvider); | ||
| }); | ||
|
|
||
| final reactiveTaskRun = ref.watch(_reactiveTaskRunProvider); | ||
|
|
||
| // Clear trigger flag once reactive task completes (has value or error) | ||
| if ((reactiveTaskRun.hasValue || reactiveTaskRun.hasError) && _triggerCausedRefresh) { | ||
| // Use Future.microtask to avoid modifying state during build | ||
| Future.microtask(() { | ||
| if (mounted) { | ||
| _triggerCausedRefresh = false; | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
The current implementation for managing the _triggerCausedRefresh flag has a couple of architectural issues:
- Implicit State Updates: On line 32,
_triggerCausedRefreshis mutated directly. The UI updates correctly only becauseref.invalidatehappens to trigger a rebuild immediately after. This is fragile and hard to reason about. State changes that affect the UI should be explicit, usingsetState. - Side Effect in
build: Theifblock on lines 39-46 performs state mutation as a side effect of thebuildmethod. This is a significant violation of Flutter's declarative UI principles, which require thebuildmethod to be pure (i.e., free of side effects).
I recommend refactoring this logic to use ref.listen for both setting and resetting the flag. This is the idiomatic Riverpod approach for reacting to provider changes and performing side effects like updating widget state. This change will make the build method pure and the state management more robust and maintainable.
ref.listen<void>(_reactiveTaskTriggerProvider, (_, __) {
if (!mounted) return;
setState(() {
_triggerCausedRefresh = true;
});
ref.invalidate(_reactiveTaskRunProvider);
});
final reactiveTaskRun = ref.watch(_reactiveTaskRunProvider);
ref.listen<AsyncValue<void>>(_reactiveTaskRunProvider, (previous, next) {
final wasLoading = previous?.isLoading ?? false;
final isDone = next.hasValue || next.hasError;
if (wasLoading && isDone && _triggerCausedRefresh) {
// Use a post-frame callback to avoid calling setState during a build,
// as the listener can fire while a build is in progress.
WidgetsBinding.instance.addPostFrameCallback((_) {
if (mounted) {
setState(() {
_triggerCausedRefresh = false;
});
}
});
}
});1 participant
Convert SplashBuilder to StatefulWidget to track trigger-caused refreshes. Add _triggerCausedRefresh flag to distinguish between trigger changes (show splash) and other refreshes like errors or dependency changes (don't show splash). Update reactive task completion logic to skip splash when errors occur without trigger change. Set trigger flag on manual retry. Clear flag after reactive task completes using