Conversation
…cessary rebuilds Replace Set/Map-based state tracking with fixed-size lists for O(1) access. Add counters to cache loading state instead of iterating collections. Minimize setState calls by checking if state actually changed before updating. Cache SplashTaskError string representation. Add early returns in providers when no tasks exist. Use eagerError in Future.wait for faster failure detection. Make oneTimeTasks and reactiveTasks
perf(splash): optimize reactive task state management and reduce unnecessary rebuilds
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 focuses on optimizing the performance and enhancing the reliability of the splash screen system. It introduces a more efficient state management approach within the "SplashBuilder" and refines the task execution and error reporting mechanisms. A comprehensive suite of new tests has been added to validate these improvements and ensure the system's robustness, alongside routine dependency updates. 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 introduces significant performance optimizations and greatly expands the test suite, which is fantastic. The move from Maps/Sets to Lists with counters for state tracking in SplashBuilder is a smart way to reduce overhead and prevent unnecessary rebuilds. The optimizations in _oneTimeSplashTasksProvider and SplashTaskError are also valuable. The new tests are comprehensive and cover many edge cases, which significantly improves the robustness of the library. I have one minor suggestion regarding a comment in lib/src/splash_task.dart to ensure it aligns perfectly with the code's implementation. Overall, this is an excellent contribution.
| // Use more efficient Future.wait with growable: false | ||
| await Future.wait( | ||
| [for (final task in tasks) task(ref)], | ||
| eagerError: true, | ||
| ); |
There was a problem hiding this comment.
Great job on the performance optimizations in this file!
I noticed a small discrepancy between the comment and the code here. The comment mentions using a list with growable: false for efficiency, but the list comprehension [for ...] actually creates a growable list.
To align the code with the comment's intent and potentially gain a minor performance improvement, you could use .map with toList(growable: false).
| // Use more efficient Future.wait with growable: false | |
| await Future.wait( | |
| [for (final task in tasks) task(ref)], | |
| eagerError: true, | |
| ); | |
| // Use more efficient Future.wait with a fixed-length list | |
| await Future.wait( | |
| tasks.map((task) => task(ref)).toList(growable: false), | |
| eagerError: true, | |
| ); |
There was a problem hiding this comment.
Pull request overview
This PR prepares version 0.0.2 of the Riverboot package, primarily focusing on internal improvements, test coverage expansion, and performance optimizations. The changes include comprehensive test suite expansion, state management optimizations in SplashBuilder, and performance improvements in task execution logic.
Key changes:
- Expanded test suite from 2 to 30+ tests covering all major functionality
- Performance optimizations using Lists instead of Sets/Maps and counter-based tracking
- Added immutability guarantees for SplashConfig task lists with string caching for error messages
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/splash_task_test.dart | Comprehensive test suite expansion covering SplashConfig, SplashTaskError, providers, and SplashBuilder widget |
| lib/src/splash_task.dart | Performance optimizations (early returns, conditional stopwatch), string caching for SplashTaskError, and code formatting |
| lib/src/splash_builder.dart | Replaced Set/Map-based tracking with List-based tracking and counters for O(1) access and reduced setState calls |
| example/lib/main.dart | Code formatting changes (line consolidation) |
| example/pubspec.lock | Updated flutter_riverpod from 3.0.0 to 3.0.3 and version reference to 0.0.2 |
| pubspec.yaml | Version bump from 0.0.1 to 0.0.2 |
| CHANGELOG.md | Added version 0.0.2 entry with brief description |
| final future = container.read(oneTimeSplashTasksProvider.future); | ||
|
|
||
| // Allow tasks to start | ||
| await Future.delayed(const Duration(milliseconds: 10)); |
There was a problem hiding this comment.
[nitpick] This test relies on a 10ms delay to ensure both tasks have started, which could be flaky on slow CI systems or under heavy load. Consider using a more deterministic synchronization mechanism, such as having each task signal when it has started (e.g., via additional Completers), rather than relying on timing.
| if (tasks.isNotEmpty) { | ||
| if (config.runOneTimeTaskInParallel) { | ||
| await Future.wait([for (final task in tasks) task(ref)]); | ||
| // Use more efficient Future.wait with growable: false |
There was a problem hiding this comment.
The comment states "Use more efficient Future.wait with growable: false" but growable is not a parameter of Future.wait. Only eagerError is specified. This comment is misleading and should be updated to accurately reflect the optimization being made (e.g., "Use eagerError: true to fail fast on the first error")
| // Use more efficient Future.wait with growable: false | |
| // Use Future.wait with eagerError: true to fail fast on the first error |
Add retry parameter with exponential backoff strategy using retryCount-based Duration in seconds. This demonstrates the retry delay configuration feature in the example app.
…r` and `run`, add new tests, and update documentation.
…rallel task completion test reliability
…and optimizations
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| |---|---------|-----------------| | ||
| | **When** | Once at app start | When trigger changes | | ||
| | **Use for** | Init services, load config | User session data | | ||
| | **Shows splash** | Only on first run or manual retry | Every time trigger changes | |
There was a problem hiding this comment.
Missing article: "Only on first run or manual retry" should be "Only on first run or on manual retry" for consistent parallelism in the phrase structure.
| | **Shows splash** | Only on first run or manual retry | Every time trigger changes | | |
| | **Shows splash** | Only on first run or on manual retry | Every time trigger changes | |
| |---|---------|-----------------| | ||
| | **When** | Once at app start | When trigger changes | | ||
| | **Use for** | Init services, load config | User session data | | ||
| | **Shows splash** | Only on first run or manual retry | Every time trigger changes | |
There was a problem hiding this comment.
Missing article: "Every time trigger changes" should be "Every time the trigger changes" for proper grammar.
| | **Shows splash** | Only on first run or manual retry | Every time trigger changes | | |
| | **Shows splash** | Only on first run or manual retry | Every time the trigger changes | |
| /// The [run] function only receives [read] - no [watch] - to prevent | ||
| /// accidentally creating unwanted reactive dependencies. |
There was a problem hiding this comment.
Documentation states "The [run] function only receives [read] - no [watch]", but this is incorrect. The function signature shows Future<void> Function(Ref ref) run, which provides full Ref access including both ref.read() and ref.watch(). This is also confirmed by the tests and example code which use ref.watch() inside run.
| /// The [run] function only receives [read] - no [watch] - to prevent | |
| /// accidentally creating unwanted reactive dependencies. | |
| /// The [run] function receives a full [Ref] object, allowing both [read] and [watch]. | |
| /// You may use [ref.read] or [ref.watch] as needed within [run]. |
| tasks: [ | ||
| for (var i = 0; i < 10; i++) | ||
| (ref) async { | ||
| await Future.delayed(Duration(milliseconds: (i + 1) * 5)); // Variable delays |
There was a problem hiding this comment.
The comment states "Variable delays" but the actual implementation uses (i + 1) * 5 milliseconds, which creates delays of 5ms, 10ms, 15ms... 50ms. This pattern is actually consistent/predictable rather than variable. Consider either changing the comment to "Incremental delays" or using truly variable delays like Random().nextInt(50) + 1.
| name: riverboot | ||
| description: "App bootstrap & splash task system for Flutter with Riverpod" | ||
| version: 0.0.1 | ||
| version: 0.1.0 |
There was a problem hiding this comment.
Version inconsistency: pubspec.yaml shows version 0.1.0, but the CHANGELOG.md documents version 0.0.2, and example/pubspec.lock shows riverboot version 0.0.2. These versions should be consistent across the codebase.
| version: 0.1.0 | |
| version: 0.0.2 |
| // Ignoring=true allows taps to pass through to the app (if that's what you want) | ||
| // Ignoring=false blocks taps (if you want to freeze input during fade) | ||
| // Usually, during fade out, we want to BLOCK input until fully revealed: | ||
| ignoring: false, // Splash catches taps until it is removed |
There was a problem hiding this comment.
The comment states "Ignoring=true allows taps to pass through to the app" and then "Ignoring=false blocks taps", but the code sets ignoring: false with the comment "Splash catches taps until it is removed". This is contradictory - if ignoring: false, then the widget is NOT ignoring pointer events, meaning it blocks/catches taps (which is the desired behavior). The inline comment should clarify this more clearly, e.g., "ignoring: false means IgnorePointer does NOT ignore - it blocks input on the splash".
| ignoring: false, // Splash catches taps until it is removed | |
| ignoring: false, // IgnorePointer does NOT ignore taps: it blocks input on the splash until it is removed |
| ), | ||
| ``` | ||
|
|
||
| **Key behavior:** Only `trigger` changes show splash. Using `ref.watch()` in `run` keeps providers alive and re-runs silently without splash. |
There was a problem hiding this comment.
Documentation states "Using ref.watch() in run keeps providers alive and re-runs silently without splash." This is misleading. According to the implementation in splash_builder.dart (lines 30-31), reactive task changes show splash when isRefreshing is true, but isReloading (when run's own dependencies change) does not show splash. This distinction should be clarified to accurately reflect the behavior.
| **Key behavior:** Only `trigger` changes show splash. Using `ref.watch()` in `run` keeps providers alive and re-runs silently without splash. | |
| **Key behavior:** Only changes to `trigger` show splash (when `isRefreshing` is true). Dependencies watched via `ref.watch()` inside `run` will re-run the task silently (when `isReloading` is true) without showing splash. This distinction ensures that splash only appears for trigger changes, not for internal dependency updates. |
|
|
||
| | | `tasks` | `reactiveTask` | | ||
| |---|---------|-----------------| | ||
| | **When** | Once at app start | When trigger changes | |
There was a problem hiding this comment.
Missing article: "When trigger changes" should be "When the trigger changes" for proper grammar.
| | **When** | Once at app start | When trigger changes | | |
| | **When** | Once at app start | When the trigger changes | |
No description provided.