fix: deliver a cold start deep link once the app can open it - #692
Open
21Mill wants to merge 4 commits into
Open
fix: deliver a cold start deep link once the app can open it#69221Mill wants to merge 4 commits into
21Mill wants to merge 4 commits into
Conversation
Opening a mostro: link while the app was not running crashed it before anything rendered: 'package:go_router/src/match.dart': Failed assertion: line 245 pos 12: 'uriPathToCompare.startsWith(newMatchedLocationToCompare)': is not true. With no activity alive, Android hands the link over as the engine's defaultRouteName rather than through pushRouteInformation, and go_router prefers that over initialLocation whenever it is not '/'. So the router started up trying to match mostro:<id>?relays=..., which is an opaque URI: its path is the bare id, with no leading slash, and matching it against '/' fails the assertion. The link never reached DeepLinkInterceptor, which guards the other delivery path, and the redirect that sends custom schemes home never ran either, since matching asserts before redirects are consulted. createRouter now sets overridePlatformDefaultLocation when the platform default carries a scheme of ours, so the app starts at '/' and the initial link is left to the handler in MostroApp that already reads it through app_links. The override is conditional rather than always on because on web the platform default is the location the user asked for, and discarding it would break opening the app at a URL. The "is this one of our schemes" test existed twice, in the interceptor and in the redirect, and this adds a third caller, so it now lives in one place as DeepLinkInterceptor.isCustomSchemeUri / isCustomSchemeLocation. Covered by a test that fakes the platform default through TestPlatformDispatcher: against the unfixed router it reports the initial location as the mostro: link itself, which is the defect exactly.
isCustomSchemeUri asked whether the scheme starts with 'http', which the predicate it replaced already did in both of its copies. A scheme like httpfoo: passes that test, so such a link would be handed to go_router as an ordinary location and assert during a cold start, which is the failure this branch exists to remove. Uri normalises the scheme to lower case, so an exact comparison needs no case handling of its own; a test pins that rather than a defensive toLowerCase.
app_routes.dart importing DeepLinkInterceptor for two static predicates pointed the dependency the wrong way: routing is the lower layer here and the interceptor is one of its consumers. The predicate now lives on its own in deep_link_schemes.dart, which both callers import, and the private alias in the interceptor goes away with it. createRouter also logs the platform default it discards, so a link that goes missing on a cold start leaves a trace instead of nothing. The router test built the router inside a Consumer.builder, which may run more than once and leave routers nobody disposes; it now holds a single one for the test and disposes it on teardown.
A link that arrived before the router existed was dropped: the listener only forwarded it when _router was already set, and the initial link was handed to a post frame callback that requests no frame of its own, so on a slow start it ran too early and on a settled app it never ran at all. MostroApp now holds the link until there is a navigator to open it with, and hands it over as soon as there is one. Delivery is not a fire and forget: handleInitialDeepLink reports whether the app was in a state to attempt the link, and only the cases where it was not - no navigator, or another link being opened - put it back for another frame. Anything the user already saw an answer for, including a failed lookup and a declined Mostro switch, counts as done. The retry rides on consecutive frames and gives up after ten, so a link cannot surface minutes later in the middle of something else. The handler used to stamp the duplicate window before knowing whether it could go ahead, which turned a link it never attempted into a duplicate of itself; it stamps after the check now. app_links is no longer the only way back to a cold start link either: it falls back to the platform default location that createRouter discards.
Contributor
|
Warning Review limit reachedNext included review available in 51 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
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 |
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Depends on #673 — this branch sits on top of it, so its two commits show up in the diff. Review only
fix: deliver a cold start deep link once the app can open it; merge #673 first.Problem
#673 stops the cold start crash, but it does not make the link open. That part is still the code on
main, and it drops the link in two opposite timing windows:_processInitialDeepLinkhands the link toaddPostFrameCallbackand then waits100 msbefore checking_router != null. On a slow start the router is not there yet and the link is discarded with nothing in the log. On the device I measured 0.9 s between the link being detected and the app being able to open it, so this is not theoretical.addPostFrameCallbackrequests no frame of its own (flutter/lib/src/scheduler/binding.dart:792-797— "whenever that may be, if ever"). Once the app has settled, the callback simply never runs.customUrlStream) dropped a link outright when_router == null.Change
MostroAppkeeps the link until there is a navigator to open it with — not until a timer expires — and hands it over as soon as there is one. The condition isrouter.routerDelegate.navigatorKey.currentContext != null, which is exactly whatDeepLinkHandler._handleMostroDeepLinkneeds and aborts without.Delivery reports a result
handleInitialDeepLinknow returnsFuture<bool>: false means the app was not in a state to attempt the link, and only then is it kept for another frame.falsefalsetruetruetrueunsupportedLinkFormatprocessMostroLinkfailedtruetrueThis is the point @grunch raised in the last review: retaining on
catchwas dead code, becauseDeepLinkHandlerswallows every ordinary failure and returns normally. Retaining on any failure would be worse than useless — a link would reopen an order the user already dismissed. Only "the app could not try" is retained.The retry rides on the frames it asks for and gives up after ten, so a retained link cannot surface minutes later in the middle of something else.
Two smaller fixes in the handler
processingContext == nullearly return left the loading dialog up.Recovering the link the router throws away
createRouterdiscards the platform default location when it carries one of our schemes (#673). Ifapp_links.getInitialLink()returnsnullor throws, that discarded value is now read back inMostroAppas a fallback, so the two paths cannot both come up empty. Duplicates are absorbed by the handler's 2 s window — confirmed on device, where both paths fire and only one navigation happens.Scope note
_processInitialDeepLinkused to filter onscheme == 'mostro'and now accepts any non-web scheme, matching exactly whatcreateRouterdiscards. Nothing else can arrive today (AndroidManifest.xmlregisters onlymostroas an intent filter; iOS lists onlymostroinCFBundleURLSchemes), and alightning:link would reachDeepLinkHandler, which answers with the localizedunsupportedLinkFormatinstead of vanishing.Tests
test/core/app_deep_link_test.dartmounts the realMostroAppwithappInitializerProviderbehind aCompleter, theapp_linksmethod channel mocked and a recordingDeepLinkHandlerthat — like the real one — never throws and reports a result instead.Every test was checked by mutation; each one fails against the code without its fix:
_deliverPendingDeepLink()after_router ??= createRouter(ref)_pendingDeepLink ??= uritest/core/deep_link_handler_test.dartcovers the new contract on the real handler:falsewhen there is no navigator,truefor a scheme it answers with a message. The first fails if that early return is changed totrue.One case is not covered by a test: the
ensureVisualUpdate()on the retry path is unconditional now (it is already a no-op outsideidleandpostFrameCallbacks, perscheduler/binding.dart:906-917), which fixes the narrow== idleguard from the previous round. I could not build a test that distinguishes it — the navigator mounts in the same frame as the router — so it rests on the source contract, as @grunch described it.Test plan
flutter analyze lib/core test/core— only the two pre-existingcontainsSemanticsinfosflutter test test/core/— greenflutter test— 906 passing, the same 11 pre-existing load failures from the missing generated mocks (Dart run build_runner build fails on Flutter 3.44.0, source_gen 3.1.0 incompatible with analyzer 8.x #606)a2561e55-…:am force-stopfirst): detected22:15:06.477→ handled22:15:07.493→Navigating to: /take_sell/a2561e55-…22:15:09.468. The second delivery through the other path is collapsed by the duplicate window.22:16:54.866→ navigated22:16:54.933, 67 ms, no frame needed.