[go_router] Fixes deep-link with no path on cold start #5113

hrishikesh-kadam · 2023-10-11T16:01:13Z

List which issues are fixed by this PR. You must list at least one issue.

[go_router] Page Not Found when cold opening an app from a deep link with no path flutter#133928

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

No breaking changes

Pre-launch Checklist

Fixes flutter/flutter#133928

chunhtai · 2023-10-11T18:28:12Z

packages/go_router/lib/src/configuration.dart

    final List<RouteMatch>? result = _getLocRouteRecursively(
-      location: uri.path,
-      remainingLocation: uri.path,
+      location: uri.path.isEmpty ? '/' : uri.path,


Instead of fixing this in the parsing, this should probably be fixed in GoRouteInformationProvider to normalize the uri when it receives deeplink from engine.

Following are the three call stacks when the breakpoint is set at the above change:

On a normal launch

Call stack

RouteConfiguration._getLocRouteMatches (.../.pub-cache/hosted/pub.dev/go_router-11.1.3/lib/src/configuration.dart:304) RouteConfiguration.findMatch (.../.pub-cache/hosted/pub.dev/go_router-11.1.3/lib/src/configuration.dart:281) GoRouteInformationParser.parseRouteInformationWithDependencies (.../.pub-cache/hosted/pub.dev/go_router-11.1.3/lib/src/parser.dart:87) _RouterState._processRouteInformation (.../flutter/stable/packages/flutter/lib/src/widgets/router.dart:744) _RouterState.restoreState (.../flutter/stable/packages/flutter/lib/src/widgets/router.dart:622) RestorationMixin._doRestore (.../flutter/stable/packages/flutter/lib/src/widgets/restoration.dart:923) RestorationMixin.didChangeDependencies (.../flutter/stable/packages/flutter/lib/src/widgets/restoration.dart:909) _RouterState.didChangeDependencies (.../flutter/stable/packages/flutter/lib/src/widgets/router.dart:692) StatefulElement._firstBuild (.../flutter/stable/packages/flutter/lib/src/widgets/framework.dart:5448) ComponentElement.mount (.../flutter/stable/packages/flutter/lib/src/widgets/framework.dart:5273) Element.inflateWidget (.../flutter/stable/packages/flutter/lib/src/widgets/framework.dart:4182) Element.updateChild (.../flutter/stable/packages/flutter/lib/src/widgets/framework.dart:3707) ComponentElement.performRebuild (.../flutter/stable/packages/flutter/lib/src/widgets/framework.dart:5322) Element.rebuild (.../flutter/stable/packages/flutter/lib/src/widgets/framework.dart:5016) ComponentElement._firstBuild (.../flutter/stable/packages/flutter/lib/src/widgets/framework.dart:5279) ComponentElement.mount (.../flutter/stable/packages/flutter/lib/src/widgets/framework.dart:5273) Element.inflateWidget (.../flutter/stable/packages/flutter/lib/src/widgets/framework.dart:4182) Element.updateChild (.../flutter/stable/packages/flutter/lib/src/widgets/framework.dart:3707) ComponentElement.performRebuild (.../flutter/stable/packages/flutter/lib/src/widgets/framework.dart:5322) Element.rebuild (.../flutter/stable/packages/flutter/lib/src/widgets/framework.dart:5016)

Launching deep-link with Uri containing scheme and authority using adb shell, when the app is closed by clicking the system back button (To mock deep-link on cold start)

Call stack

RouteConfiguration._getLocRouteMatches (.../.pub-cache/hosted/pub.dev/go_router-11.1.3/lib/src/configuration.dart:304) RouteConfiguration.findMatch (.../.pub-cache/hosted/pub.dev/go_router-11.1.3/lib/src/configuration.dart:281) GoRouteInformationParser.parseRouteInformationWithDependencies (.../.pub-cache/hosted/pub.dev/go_router-11.1.3/lib/src/parser.dart:87) _RouterState._processRouteInformation (.../flutter/stable/packages/flutter/lib/src/widgets/router.dart:744) _RouterState.restoreState (.../flutter/stable/packages/flutter/lib/src/widgets/router.dart:622) RestorationMixin._doRestore (.../flutter/stable/packages/flutter/lib/src/widgets/restoration.dart:923) RestorationMixin.didChangeDependencies (.../flutter/stable/packages/flutter/lib/src/widgets/restoration.dart:909) _RouterState.didChangeDependencies (.../flutter/stable/packages/flutter/lib/src/widgets/router.dart:692) StatefulElement._firstBuild (.../flutter/stable/packages/flutter/lib/src/widgets/framework.dart:5448) ComponentElement.mount (.../flutter/stable/packages/flutter/lib/src/widgets/framework.dart:5273) Element.inflateWidget (.../flutter/stable/packages/flutter/lib/src/widgets/framework.dart:4182) Element.updateChild (.../flutter/stable/packages/flutter/lib/src/widgets/framework.dart:3707) ComponentElement.performRebuild (.../flutter/stable/packages/flutter/lib/src/widgets/framework.dart:5322) Element.rebuild (.../flutter/stable/packages/flutter/lib/src/widgets/framework.dart:5016) ComponentElement._firstBuild (.../flutter/stable/packages/flutter/lib/src/widgets/framework.dart:5279) ComponentElement.mount (.../flutter/stable/packages/flutter/lib/src/widgets/framework.dart:5273) Element.inflateWidget (.../flutter/stable/packages/flutter/lib/src/widgets/framework.dart:4182) Element.updateChild (.../flutter/stable/packages/flutter/lib/src/widgets/framework.dart:3707) ComponentElement.performRebuild (.../flutter/stable/packages/flutter/lib/src/widgets/framework.dart:5322) Element.rebuild (.../flutter/stable/packages/flutter/lib/src/widgets/framework.dart:5016)

Launching deep-link with Uri containing scheme and authority using adb shell, when the app is in foreground or backgrounded by clicking the home button

Call stack

RouteConfiguration._getLocRouteMatches (.../.pub-cache/hosted/pub.dev/go_router-11.1.3/lib/src/configuration.dart:304) RouteConfiguration.findMatch (.../.pub-cache/hosted/pub.dev/go_router-11.1.3/lib/src/configuration.dart:281) GoRouteInformationParser.parseRouteInformationWithDependencies (.../.pub-cache/hosted/pub.dev/go_router-11.1.3/lib/src/parser.dart:87) _RouterState._processRouteInformation (.../flutter/stable/packages/flutter/lib/src/widgets/router.dart:744) _RouterState._handleRouteInformationProviderNotification (.../flutter/stable/packages/flutter/lib/src/widgets/router.dart:762) ChangeNotifier.notifyListeners (.../flutter/stable/packages/flutter/lib/src/foundation/change_notifier.dart:403) GoRouteInformationProvider.notifyListeners (.../.pub-cache/hosted/pub.dev/go_router-11.1.3/lib/src/information_provider.dart:147) GoRouteInformationProvider._platformReportsNewRouteInformation (.../.pub-cache/hosted/pub.dev/go_router-11.1.3/lib/src/information_provider.dart:246) GoRouteInformationProvider.didPushRouteInformation (.../.pub-cache/hosted/pub.dev/go_router-11.1.3/lib/src/information_provider.dart:277) WidgetsBinding._handlePushRouteInformation (.../flutter/stable/packages/flutter/lib/src/widgets/binding.dart:704) <asynchronous gap> (Unknown Source:0) MethodChannel._handleAsMethodCall (.../flutter/stable/packages/flutter/lib/src/services/platform_channel.dart:547) <asynchronous gap> (Unknown Source:0) _DefaultBinaryMessenger.setMessageHandler.<anonymous closure> (.../flutter/stable/packages/flutter/lib/src/services/binding.dart:567) <asynchronous gap> (Unknown Source:0)

You can see, GoRouteInformationProvider gets involved only in the 3rd case.

for 1 and 2, it would hit this line
https://github.com/flutter/flutter/blob/38f318b46547a6c96715b1d8454c36606acddd42/packages/flutter/lib/src/widgets/router.dart#L619

We should make sure the initialroute is normalized in here

packages/packages/go_router/lib/src/router.dart

Line 523 in 30b1094

String _effectiveInitialLocation(String? initialLocation) {

Incorporated suggested changes in the 6cc2fc4 commit.

Before resolving this conversation, please note that the difference between 49f39ad and 6cc2fc4 is that former tries to fix the issue as late as possible and later as early as possible.
This might create issues in the future when the package handles other schemes like file:///.

chunhtai · 2023-10-26T21:49:47Z

packages/go_router/lib/src/router.dart

+    final Uri platformDefaultUri =
+        Uri.parse(WidgetsBinding.instance.platformDispatcher.defaultRouteName);
    final String platformDefault =
-        WidgetsBinding.instance.platformDispatcher.defaultRouteName;


What if the deeplink is called when the app is already running? wasn't the path be missing in that case? I think you need to also modify the logic in _platformReportsNewRouteInformation

For now, the change is not required because the RouteInformation.location getter in _platformReportsNewRouteInformation() handles the empty path.

Once the package maintainers decide to migrate from RouteInformation.location to RouteInformation.uri, then it would be the right time to edit.
Also, the care should be taken about where to strip the scheme and authority, either early or late in the code as mentioned in this comment.

ah interesting, thanks for reminding. Can you add a todo somewhere to remind us about location to uri migration? That way we won't break the code when that happens

chunhtai

Code LGTM, just we should add a todo to remind ourself once the migration happens

chunhtai · 2023-10-27T17:23:39Z

packages/go_router/lib/src/router.dart

+    final Uri platformDefaultUri =
+        Uri.parse(WidgetsBinding.instance.platformDispatcher.defaultRouteName);
    final String platformDefault =
-        WidgetsBinding.instance.platformDispatcher.defaultRouteName;


ah interesting, thanks for reminding. Can you add a todo somewhere to remind us about location to uri migration? That way we won't break the code when that happens

hannah-hyj

LGTM

…er/packages#5113)

flutter/packages@2af6954...c9fec61 2023-10-31 [email protected] [image_picker] Prevent multiple active calls on iOS (flutter/packages#5272) 2023-10-30 [email protected] [in_app_purchase_android] Add missing response code to BillingResponse enum (flutter/packages#5120) 2023-10-30 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 2.22.4 to 2.22.5 (flutter/packages#5263) 2023-10-30 [email protected] Bump play-service-maps dependency from version 18.1.0 to 18.2.0 (flutter/packages#5243) 2023-10-30 [email protected] Roll Flutter from a4ec627 to e12d1a7 (3 revisions) (flutter/packages#5267) 2023-10-30 [email protected] [camerax] Loosen restrictions of fallback strategies for choosing resolutions (flutter/packages#5239) 2023-10-30 [email protected] [pigeon] Don't wrap non-nullable primitives in Obj-C (flutter/packages#5214) 2023-10-30 [email protected] [flutter_lints] Replace "flutter pub add --dev" with "dev:" (flutter/packages#5260) 2023-10-28 [email protected] Roll Flutter from 5907c97 to a4ec627 (18 revisions) (flutter/packages#5255) 2023-10-27 [email protected] [go_router] Fixes deep-link with no path on cold start (flutter/packages#5113) 2023-10-27 [email protected] [two_dimensional_scrollables] Fix pinned row painting when one axis is reversed (flutter/packages#5187) 2023-10-27 [email protected] Roll Flutter from c555599 to 5907c97 (45 revisions) (flutter/packages#5252) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

[go_router] Fixes deep-link with no path on cold start

49f39ad

Fixes flutter/flutter#133928

hrishikesh-kadam requested a review from chunhtai as a code owner October 11, 2023 16:01

github-actions bot added the p: go_router label Oct 11, 2023

chunhtai reviewed Oct 11, 2023

View reviewed changes

chunhtai requested a review from hannah-hyj October 12, 2023 18:24

chunhtai self-requested a review October 19, 2023 22:19

hrishikesh-kadam added 2 commits October 25, 2023 21:02

Merge upstream/main and bump version

795f7c0

[go_router] Resolves flutter#5113 (comment)

6cc2fc4

chunhtai reviewed Oct 26, 2023

View reviewed changes

chunhtai approved these changes Oct 27, 2023

View reviewed changes

[go_router] Resolves flutter#5113 (comment)

e32b56e

hannah-hyj approved these changes Oct 27, 2023

View reviewed changes

hannah-hyj added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 27, 2023

auto-submit bot merged commit 6bcb747 into flutter:main Oct 27, 2023

hrishikesh-kadam deleted the fix-deep-link-cold-start branch October 28, 2023 17:23

engine-flutter-autoroll mentioned this pull request Oct 30, 2023

Roll Packages from 2af69541a214 to fe4c86bf2091 (4 revisions) flutter/flutter#137541

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 30, 2023

6bcb747 [go_router] Fixes deep-link with no path on cold start (flutt…

4a1f35b

…er/packages#5113)

engine-flutter-autoroll mentioned this pull request Oct 30, 2023

Roll Packages from 2af69541a214 to 35b15de176a9 (5 revisions) flutter/flutter#137549

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 30, 2023

6bcb747 [go_router] Fixes deep-link with no path on cold start (flutt…

050a50b

…er/packages#5113)

engine-flutter-autoroll mentioned this pull request Oct 30, 2023

Roll Packages from 2af69541a214 to 52202abec091 (7 revisions) flutter/flutter#137551

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 30, 2023

6bcb747 [go_router] Fixes deep-link with no path on cold start (flutt…

2227268

…er/packages#5113)

engine-flutter-autoroll mentioned this pull request Oct 31, 2023

Roll Packages from 2af69541a214 to c9fec61b6cde (12 revisions) flutter/flutter#137623

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 31, 2023

6bcb747 [go_router] Fixes deep-link with no path on cold start (flutt…

12c4f69

…er/packages#5113)

hrishikesh-kadam mentioned this pull request Nov 2, 2023

[go_router] Page Not Found when cold opening an app from a deep link with no path flutter/flutter#133928

Closed

2 tasks

reidbaker mentioned this pull request Jan 12, 2024

Dialog Debug #5881

Closed

reidbaker mentioned this pull request Jan 19, 2024

[in_app_purchase] Add play country code api #5941

Merged

12 tasks

reidbaker mentioned this pull request Feb 19, 2024

[in_app_purchase_android] Add UserChoiceBilling mode. #6162

Merged

16 tasks

[go_router] Fixes deep-link with no path on cold start #5113

[go_router] Fixes deep-link with no path on cold start #5113

Uh oh!

Conversation

hrishikesh-kadam commented Oct 11, 2023

Pre-launch Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hrishikesh-kadam Oct 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hrishikesh-kadam Oct 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chunhtai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hannah-hyj left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hrishikesh-kadam Oct 11, 2023 •

edited

Loading

hrishikesh-kadam Oct 27, 2023 •

edited

Loading