Conversation
|
@larryaasen could you please review this PR? |
| } | ||
| } | ||
| return widget.child ?? const SizedBox.shrink(); | ||
| return Stack( |
There was a problem hiding this comment.
@haashem This code change impacts all users not just for the use case you are trying to solve. Why is a Stack created (and all of those other widgets) when widget.child is null? Do all existing use cases that already work properly need this change?
There was a problem hiding this comment.
Sorry for my late reply, Good point. Stack is only needed when UpgraderAlert wraps app content so then it host dialog above subsequent page pushes.
I reverted to original implementation when child is null.
There was a problem hiding this comment.
Pull request overview
Fixes the issue where UpgradeAlert could be obscured by subsequent route pushes (e.g., navigating from a splash screen to home) by ensuring the upgrade dialog is presented above later navigation changes.
Changes:
- Add an overlay
NavigatorinsideUpgradeAlertso dialogs are shown on a dedicated top-layer navigator (stays above later pushes). - Adjust dialog presentation to use the overlay navigator context (
useRootNavigator: false). - Add a widget test that reproduces a later route push and asserts the alert remains visible.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| lib/src/upgrade_alert.dart | Presents the upgrade dialog via a dedicated overlay navigator to keep it above subsequent navigation pushes. |
| test/upgrade_alert_test.dart | Adds a regression test covering route push behavior while the alert is displayed. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
larryaasen
added
enhancement
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
lib/src/upgrade_alert.dart:148
- The private
Navigatoris not part ofWidgetsApp's platform-back dispatch path. Android back is sent to the app navigator, while this dialog'sPopScopeis registered on a route in_dialogNavigatorKey, soshouldPopScopeis no longer invoked and enabled back-button dismissal stops working. The existing regression test attest/upgrader_test.dart:640-659exercises this path. The nested navigator needs explicit back delegation, or the dialog must remain on a navigator that receives platform pop events.
child: HeroControllerScope.none(
child: Navigator(
key: _dialogNavigatorKey,
| Positioned.fill( | ||
| child: HeroControllerScope.none( | ||
| child: Navigator( |
There was a problem hiding this comment.
Good catch, I could reproduce it! Pushed a fix:
UpgradeAlert now uses its private navigator only when wrapping the app above its navigator, such as MaterialApp.builder.
Subtree placements like Scaffold(body: UpgradeAlert(...)) use the root navigator.
| Before | After |
|---|---|
 |
 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #541 +/- ##
==========================================
- Coverage 92.82% 92.22% -0.61%
==========================================
Files 12 12
Lines 1296 1337 +41
==========================================
+ Hits 1203 1233 +30
- Misses 93 104 +11 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Resolves #540
upgrader-before.mov
upgrader-after.mov