-
Notifications
You must be signed in to change notification settings - Fork 28
fix: do not let a cold-start deep link become the router's location #673
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -62,9 +62,16 @@ class DeepLinkInterceptor extends WidgetsBindingObserver { | |||||||
| } | ||||||||
|
|
||||||||
| /// Check if the URI uses a custom scheme | ||||||||
| bool _isCustomScheme(Uri uri) { | ||||||||
| return uri.scheme == 'mostro' || | ||||||||
| (!uri.scheme.startsWith('http') && uri.scheme.isNotEmpty); | ||||||||
| bool _isCustomScheme(Uri uri) => isCustomSchemeUri(uri); | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Refactor suggestion | Medium The router now depends on the interceptor for a pure predicate, and this wrapper is dead weight. Deduplicating the three copies is the right call — the home isn't. Extracting to |
||||||||
|
|
||||||||
| /// Whether the URI uses a scheme the app resolves itself, such as `mostro:` | ||||||||
| static bool isCustomSchemeUri(Uri uri) => | ||||||||
| uri.scheme.isNotEmpty && !uri.scheme.startsWith('http'); | ||||||||
|
|
||||||||
| /// [isCustomSchemeUri] for an unparsed location; unparseable means no | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 Nitpick The doc comment is cut off mid-sentence —
Suggested change
|
||||||||
| static bool isCustomSchemeLocation(String location) { | ||||||||
| final uri = Uri.tryParse(location); | ||||||||
| return uri != null && isCustomSchemeUri(uri); | ||||||||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||||||||
| } | ||||||||
|
|
||||||||
| /// Dispose the interceptor | ||||||||
|
|
||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,85 @@ | ||
| import 'package:flutter/material.dart'; | ||
| import 'package:flutter_riverpod/flutter_riverpod.dart'; | ||
| import 'package:flutter_test/flutter_test.dart'; | ||
| import 'package:go_router/go_router.dart'; | ||
| import 'package:mostro_mobile/core/app_routes.dart'; | ||
| import 'package:mostro_mobile/shared/providers/storage_providers.dart'; | ||
| import 'package:shared_preferences/shared_preferences.dart'; | ||
| import 'package:shared_preferences_platform_interface/in_memory_shared_preferences_async.dart'; | ||
| import 'package:shared_preferences_platform_interface/shared_preferences_async_platform_interface.dart'; | ||
|
|
||
| const _mostroLink = | ||
| 'mostro:8927bb1d-da68-491e-b0e2-db0ed548d52c?relays=wss://relay.mostro.network'; | ||
|
|
||
| /// Builds the app's real router inside a scope that can resolve it, and hands | ||
| /// it back without mounting any screen. | ||
| Future<GoRouter> buildRouter(WidgetTester tester) async { | ||
| late GoRouter router; | ||
| await tester.pumpWidget( | ||
| ProviderScope( | ||
| overrides: [ | ||
| sharedPreferencesProvider.overrideWithValue(SharedPreferencesAsync()), | ||
| ], | ||
| child: Consumer( | ||
| builder: (context, ref, _) { | ||
| router = createRouter(ref); | ||
| return const SizedBox.shrink(); | ||
| }, | ||
| ), | ||
| ), | ||
| ); | ||
| return router; | ||
| } | ||
|
Comment on lines
+31
to
+44
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 Nitpick
A small |
||
|
|
||
| void main() { | ||
| setUp(() { | ||
| SharedPreferencesAsyncPlatform.instance = | ||
| InMemorySharedPreferencesAsync.empty(); | ||
| }); | ||
|
|
||
| group('createRouter initial location', () { | ||
| // Regression test for #670: go_router preferred the cold-start deep link | ||
| // over initialLocation and asserted while matching it. | ||
| testWidgets('ignores a custom scheme handed over by the platform', | ||
| (tester) async { | ||
| tester.binding.platformDispatcher.defaultRouteNameTestValue = _mostroLink; | ||
| addTearDown( | ||
| tester.binding.platformDispatcher.clearDefaultRouteNameTestValue); | ||
|
|
||
| final router = await buildRouter(tester); | ||
|
|
||
| expect( | ||
| router.routeInformationProvider.value.uri.toString(), | ||
| '/', | ||
| ); | ||
| expect(tester.takeException(), isNull); | ||
| }); | ||
|
|
||
| testWidgets('starts at the root on an ordinary launch', (tester) async { | ||
| tester.binding.platformDispatcher.defaultRouteNameTestValue = '/'; | ||
| addTearDown( | ||
| tester.binding.platformDispatcher.clearDefaultRouteNameTestValue); | ||
|
|
||
| final router = await buildRouter(tester); | ||
|
|
||
| expect(router.routeInformationProvider.value.uri.toString(), '/'); | ||
| expect(tester.takeException(), isNull); | ||
| }); | ||
|
|
||
| // On web the platform default is a real location and must still win. | ||
| testWidgets('honours a real location handed over by the platform', | ||
| (tester) async { | ||
| tester.binding.platformDispatcher.defaultRouteNameTestValue = '/settings'; | ||
| addTearDown( | ||
| tester.binding.platformDispatcher.clearDefaultRouteNameTestValue); | ||
|
|
||
| final router = await buildRouter(tester); | ||
|
|
||
| expect( | ||
| router.routeInformationProvider.value.uri.toString(), | ||
| '/settings', | ||
| ); | ||
| expect(tester.takeException(), isNull); | ||
| }); | ||
| }); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| import 'package:flutter_test/flutter_test.dart'; | ||
| import 'package:mostro_mobile/core/deep_link_interceptor.dart'; | ||
|
|
||
| void main() { | ||
| group('DeepLinkInterceptor.isCustomSchemeLocation', () { | ||
| test('claims mostro links', () { | ||
| expect( | ||
| DeepLinkInterceptor.isCustomSchemeLocation( | ||
| 'mostro:8927bb1d-da68-491e-b0e2-db0ed548d52c' | ||
| '?relays=wss://relay.mostro.network', | ||
| ), | ||
| isTrue, | ||
| ); | ||
| expect(DeepLinkInterceptor.isCustomSchemeLocation('mostro:'), isTrue); | ||
| }); | ||
|
|
||
| test('claims other non-web schemes', () { | ||
| expect( | ||
| DeepLinkInterceptor.isCustomSchemeLocation('lightning:lnbc1...'), | ||
| isTrue, | ||
| ); | ||
| }); | ||
|
|
||
| test('leaves app locations alone', () { | ||
| expect(DeepLinkInterceptor.isCustomSchemeLocation('/'), isFalse); | ||
| expect( | ||
| DeepLinkInterceptor.isCustomSchemeLocation('/take_sell/order-1'), | ||
| isFalse, | ||
| ); | ||
| expect( | ||
| DeepLinkInterceptor.isCustomSchemeLocation('/settings?tab=relays'), | ||
| isFalse, | ||
| ); | ||
| expect(DeepLinkInterceptor.isCustomSchemeLocation(''), isFalse); | ||
| }); | ||
|
|
||
| test('leaves web locations alone', () { | ||
| expect( | ||
| DeepLinkInterceptor.isCustomSchemeLocation('https://mostro.network/x'), | ||
| isFalse, | ||
| ); | ||
| expect( | ||
| DeepLinkInterceptor.isCustomSchemeLocation('http://localhost:8080/'), | ||
| isFalse, | ||
| ); | ||
| }); | ||
|
|
||
| test('treats an unparseable location as an ordinary one', () { | ||
| // Nothing we could hand to the deep link handler either. | ||
| expect(DeepLinkInterceptor.isCustomSchemeLocation('::::'), isFalse); | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Refactor suggestion | Medium
The discarded platform default is never used as a fallback.
Once the override kicks in, recovering the link rests entirely on
app_links.getInitialLink()returning the same thing. If that call fails or returnsnullon a cold start, the link is gone with no trace: it used to crash, now it's silent.At minimum log it here; better, hand
platformDefaultLocationto_queueDeepLinkas a backup source so the two paths can't both come up empty.