Migrated from uni_links to app_links package #2710
Conversation
WalkthroughThis pull request focuses on migrating the deep linking functionality from the discontinued Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
Other🎯 Please be considerate of our volunteers' time. Contacting the person who assigned the reviewers is not advised unless they ask for your input. Do not @ the person who did the assignment otherwise. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
lib/splash_screen.dart (1)
Deep linking functionality needs to be restored
The codebase shows evidence of an ongoing migration from a custom deep linking implementation to the AppLinks package:
- The AppLinks integration is present and well-tested
- The old deep linking logic is commented out in multiple files
- The invite handling functionality appears to be missing in the new implementation
Please restore the deep linking functionality by:
- Migrating the invite handling logic from the commented code to work with AppLinks
- Updating the
fromInviteLinkmethod in navigation_service.dart- Re-implementing the invite URL parameter handling
🔗 Analysis chain
Line range hint
78-142: Verify if deep linking functionality should be restored.There's a large section of commented-out code handling deep links (
_handleDeepLinksmethod). This might indicate missing functionality that needs to be migrated to work with the new AppLinks package.Let me help verify if this functionality is still needed:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any references to deep linking functionality rg -l "invite|eventInvite|fromInviteLink" --type dartLength of output: 844
Script:
#!/bin/bash # Search for AppLinks usage and related deep linking implementation rg "AppLinks|uni_links" --type dart -A 5 # Search for TODOs related to deep linking rg "TODO.*(?i)(deep.*link|applinks|uni_links)" --type dart # Search for invite/eventInvite handling implementation ast-grep --pattern 'if ($_.contains("invite") || $_.contains("eventInvite")) { $$$ }'Length of output: 11601
Script:
#!/bin/bash # Check navigation service for invite handling rg "fromInviteLink|handleDeepLinks?" lib/services/navigation_service.dart -A 5 # Look for any invite handling in view models rg "fromInviteLink|handleDeepLinks?" lib/view_model/ -A 5Length of output: 436
🧹 Nitpick comments (2)
test/widget_tests/pre_auth_screens/splash_screen_test.dart (2)
110-289: Good use of runAsync for handling asynchronous operations!The widget tests have been properly updated to handle asynchronous operations. However, consider adding timeout parameters to
tester.pumpAndSettle()calls to prevent infinite loops in case of continuous animations.-await tester.pumpAndSettle(); +await tester.pumpAndSettle(const Duration(seconds: 5));
293-389: Enhance assertions in URI handling tests.While the test coverage is comprehensive, consider strengthening the assertions:
- For initial URI test:
testWidgets("should handle initial URI successfully", (tester) async { await tester.runAsync(() async { final uri = Uri.parse('talawa://example.com'); when(mockAppLinks.getInitialLink()).thenAnswer((_) async => uri); when(mockAppLinks.uriLinkStream).thenAnswer((_) => Stream.value(uri)); await tester.pumpWidget(createSplashScreenLight()); await tester.pumpAndSettle(); verify(mockAppLinks.getInitialLink()).called(1); + verify(mockAppLinks.uriLinkStream).called(1); expect(find.byKey(const Key('SplashScreenScaffold')), findsOneWidget); }); });
- For error handling tests:
testWidgets("should handle PlatformException when getting initial URI", (tester) async { await tester.runAsync(() async { when(mockAppLinks.getInitialLink()) .thenThrow(PlatformException(code: 'TEST_ERROR')); when(mockAppLinks.uriLinkStream) .thenAnswer((_) => const Stream.empty()); await tester.pumpWidget(createSplashScreenLight()); await tester.pumpAndSettle(); verify(mockAppLinks.getInitialLink()).called(1); + verify(mockAppLinks.uriLinkStream).called(1); + expect(find.byKey(const Key('SplashScreenScaffold')), findsOneWidget); }); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
lib/locator.dart(2 hunks)lib/splash_screen.dart(3 hunks)pubspec.yaml(1 hunks)test/helpers/test_locator.dart(2 hunks)test/widget_tests/pre_auth_screens/splash_screen_test.dart(2 hunks)test/widget_tests/pre_auth_screens/splash_screen_test.mocks.dart(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Checking codebase
🔇 Additional comments (7)
test/helpers/test_locator.dart (1)
4-4: LGTM! Test locator setup mirrors production code.The test locator correctly registers AppLinks as a singleton, maintaining parity with the production code.
Also applies to: 153-155
lib/locator.dart (1)
1-1: LGTM! AppLinks singleton registration is properly implemented.The AppLinks registration follows the established pattern for singleton registration in the service locator.
Also applies to: 181-183
lib/splash_screen.dart (1)
46-48: LGTM! AppLinks integration is properly implemented.The migration to AppLinks is done correctly:
- AppLinks instance is retrieved from the service locator
- URI stream subscription uses the new API
- Initial link retrieval uses the new API
Also applies to: 65-66
test/widget_tests/pre_auth_screens/splash_screen_test.mocks.dart (1)
1-327: LGTM! Mock implementations are correctly generated.The Mockito-generated mocks for AppLinks include all necessary methods with appropriate default behaviors and error handling.
test/widget_tests/pre_auth_screens/splash_screen_test.dart (1)
6-7: Well-structured mock setup for AppLinks migration!The mock setup follows testing best practices with proper registration and cleanup of dependencies.
Also applies to: 18-18, 23-23, 25-25, 73-101
pubspec.yaml (2)
22-23: LGTM! Appropriate versions selected for app_links.The selected versions are recent, stable, and compatible with the platform interface.
Line range hint
17-20: Verify the removal of analyzer-related dependencies.The removal of
_fe_analyzer_shared,analyzer, andanalyzer_pluginseems unrelated to the uni_links to app_links migration. Please confirm if this was intentional.✅ Verification successful
Removal of analyzer dependencies is safe and intentional
The analyzer-related dependencies are correctly declared in
talawa_lint/pubspec.yamlwhere they are actually needed. Their removal from the mainpubspec.yamlis appropriate as these dependencies are only required by the lint package, and having them in both places could potentially cause version conflicts.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if these dependencies are used elsewhere in the project rg --type dart "_fe_analyzer_shared|analyzer[^a-zA-Z]|analyzer_plugin" -lLength of output: 424
Script:
#!/bin/bash # Check both pubspec files echo "=== Main pubspec.yaml ===" cat pubspec.yaml echo -e "\n=== talawa_lint/pubspec.yaml ===" cat talawa_lint/pubspec.yamlLength of output: 3475
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
test/widget_tests/pre_auth_screens/splash_screen_test.dart (3)
121-133: Consider enhancing widget tests with additional assertions.While the async wrapper implementation is correct, consider adding assertions for:
- Initial loading state
- State transitions during deep link handling
- Error states in the UI
Also applies to: 136-148, 151-168, 171-190, 193-208, 213-225, 228-240, 243-260, 263-282, 285-300
304-320: Add assertions for URI parsing and navigation.While the initial URI handling test is good, consider adding assertions for:
- Correct URI parsing
- Navigation to the appropriate screen based on the URI
// Assert verify(mockAppLinks.getInitialLink()).called(1); expect(find.byKey(const Key('SplashScreenScaffold')), findsOneWidget); +// Add assertions for URI parsing +expect(uri.scheme, equals('talawa')); +expect(uri.host, equals('example.com')); +// Add navigation verification +verify(mockUserConfig.getInitialScreen(uri)).called(1);
336-352: Enhance error handling test coverage.Consider adding tests for:
- Different types of PlatformExceptions
- Malformed URIs
- Network-related errors
Also applies to: 353-369, 370-384
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
pubspec.yaml(2 hunks)test/widget_tests/pre_auth_screens/splash_screen_test.dart(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pubspec.yaml
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Checking codebase
🔇 Additional comments (5)
test/widget_tests/pre_auth_screens/splash_screen_test.dart (5)
6-7: LGTM! Required imports added for app_links migration.The new imports correctly support the migration from uni_links to app_links and include necessary testing utilities.
Also applies to: 9-9, 19-19
74-79: LGTM! Stack trace demangling configuration added.The stack trace demangling configuration is correctly implemented to support non-standard environments like CI.
81-102: LGTM! Well-structured mock setup with proper initialization.The mock setup follows best practices:
- Proper registration/unregistration of dependencies
- Default mock behavior configured
- Clean state ensured for each test
104-112: LGTM! Thorough cleanup in tearDown.Proper cleanup of registered mocks in tearDown prevents test interference.
385-399: LGTM! Proper stream cleanup verification.The test correctly verifies stream subscription cleanup on widget disposal.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/widget_tests/pre_auth_screens/splash_screen_test.dart (2)
Line range hint
28-73: Consider reducing code duplication in test helper functions.The
createSplashScreenLightandcreateSplashScreenDarkfunctions share significant code. Consider refactoring to a single function with a theme parameter.-Widget createSplashScreenLight({ThemeMode themeMode = ThemeMode.light}) => - BaseView<AppLanguage>( - onModelReady: (model) => model.initialize(), - builder: (context, model, child) { - return MaterialApp( - locale: const Locale('en'), - localizationsDelegates: [ - const AppLocalizationsDelegate(isTest: true), - GlobalMaterialLocalizations.delegate, - GlobalWidgetsLocalizations.delegate, - ], - key: const Key('Root'), - themeMode: themeMode, - theme: TalawaTheme.lightTheme, - home: const SplashScreen( - key: Key('SplashScreen'), - ), - navigatorKey: navigationService.navigatorKey, - onGenerateRoute: router.generateRoute, - ); - }, - ); - -Widget createSplashScreenDark({ThemeMode themeMode = ThemeMode.dark}) => +Widget createSplashScreen({ + ThemeMode themeMode = ThemeMode.light, + ThemeData? theme, + ThemeData? darkTheme, +}) => BaseView<AppLanguage>( onModelReady: (model) => model.initialize(), builder: (context, model, child) { return MaterialApp( locale: const Locale('en'), localizationsDelegates: [ const AppLocalizationsDelegate(isTest: true), GlobalMaterialLocalizations.delegate, GlobalWidgetsLocalizations.delegate, ], key: const Key('Root'), themeMode: themeMode, - darkTheme: TalawaTheme.darkTheme, + theme: theme ?? TalawaTheme.lightTheme, + darkTheme: darkTheme ?? TalawaTheme.darkTheme, home: const SplashScreen( key: Key('SplashScreen'), ), navigatorKey: navigationService.navigatorKey, onGenerateRoute: router.generateRoute, ); }, );
127-306: Consider parameterizing theme-based tests to reduce duplication.The light and dark mode test groups share identical test cases with different themes. Consider using parameterized tests to reduce code duplication.
Example approach:
for (final theme in [ (name: 'light', mode: ThemeMode.light, data: TalawaTheme.lightTheme), (name: 'dark', mode: ThemeMode.dark, data: TalawaTheme.darkTheme), ]) { group('Splash Screen Widget Test in ${theme.name} mode', () { // Your test cases here, using theme.mode and theme.data }); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/widget_tests/pre_auth_screens/splash_screen_test.dart(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Checking codebase
🔇 Additional comments (4)
test/widget_tests/pre_auth_screens/splash_screen_test.dart (4)
6-7: LGTM! Well-structured mock setup.The mock setup using
@GenerateNiceMocksfollows best practices for testing Flutter widgets.Also applies to: 20-20, 27-27
75-81: LGTM! Proper test environment configuration.The stack trace demangling configuration improves test output readability in CI environments.
83-104: LGTM! Well-structured test setup.The mock setup and teardown logic ensures proper test isolation by:
- Registering fresh mock instances for each test
- Properly initializing default behaviors
310-406: LGTM! Comprehensive URI handling test coverage.The tests thoroughly cover:
- Success scenarios for initial URI and stream updates
- Error handling for various exceptions
- Proper cleanup of stream subscriptions
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
test/widget_tests/pre_auth_screens/splash_screen_test.dart (4)
75-80: Consider adding a comment explaining the stack trace setup.While the code is correct, adding a brief comment explaining why stack trace demangling is disabled would improve maintainability.
// Disable stack trace demangling for non-standard environments (e.g., CI) + // This ensures consistent stack traces across different environments and + // makes test failures more debuggable in CI pipelines FlutterError.demangleStackTrace = (StackTrace stack) {
Line range hint
28-73: Consider reducing code duplication in test helper functions.The
createSplashScreenLightandcreateSplashScreenDarkfunctions share significant code. Consider extracting the common logic into a shared helper function.+Widget _createSplashScreen({ + required ThemeMode themeMode, + ThemeData? theme, + ThemeData? darkTheme, +}) => BaseView<AppLanguage>( + onModelReady: (model) => model.initialize(), + builder: (context, model, child) { + return MaterialApp( + locale: const Locale('en'), + localizationsDelegates: [ + const AppLocalizationsDelegate(isTest: true), + GlobalMaterialLocalizations.delegate, + GlobalWidgetsLocalizations.delegate, + ], + key: const Key('Root'), + themeMode: themeMode, + theme: theme, + darkTheme: darkTheme, + home: const SplashScreen( + key: Key('SplashScreen'), + ), + navigatorKey: navigationService.navigatorKey, + onGenerateRoute: router.generateRoute, + ); + }, +); + +Widget createSplashScreenLight({ThemeMode themeMode = ThemeMode.light}) => + _createSplashScreen( + themeMode: themeMode, + theme: TalawaTheme.lightTheme, + ); + +Widget createSplashScreenDark({ThemeMode themeMode = ThemeMode.dark}) => + _createSplashScreen( + themeMode: themeMode, + darkTheme: TalawaTheme.darkTheme, + );
124-173: Consider improving test descriptions and assertion grouping.While the tests are comprehensive, consider:
- Using more consistent test descriptions (e.g., "should show splash screen" instead of "Testing if Splash Screen shows up")
- Grouping related assertions using
describeblocksExample of improved test structure:
describe('app name', () { testWidgets("should display with correct styling", (tester) async { await tester.runAsync(() async { await tester.pumpWidget(createSplashScreenLight()); await tester.pumpAndSettle(); final text = tester.firstWidget(find.text('TALAWA')) as Text; expect(text.style!.color, TalawaTheme.lightTheme.textTheme.headlineMedium!.color); expect(text.style!.fontFamily, TalawaTheme.lightTheme.textTheme.headlineMedium!.fontFamily); expect(text.style!.fontSize, TalawaTheme.lightTheme.textTheme.headlineMedium!.fontSize); }); }); });
216-305: Consider using parameterized tests to reduce duplication.The dark mode tests duplicate the light mode test logic. Consider using parameterized tests to reduce code duplication and make maintenance easier.
for (final theme in [ (mode: ThemeMode.light, creator: createSplashScreenLight, theme: TalawaTheme.lightTheme), (mode: ThemeMode.dark, creator: createSplashScreenDark, theme: TalawaTheme.darkTheme), ]) { group('Splash Screen Widget Test in ${theme.mode} mode', () { testWidgets("should show splash screen", (tester) async { await tester.runAsync(() async { await tester.pumpWidget(theme.creator()); // ... rest of the test }); }); // ... other tests }); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/widget_tests/pre_auth_screens/splash_screen_test.dart(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Checking codebase
🔇 Additional comments (3)
test/widget_tests/pre_auth_screens/splash_screen_test.dart (3)
6-7: LGTM! Well-organized imports and mock setup.The imports are properly organized and include all necessary packages for the migration from uni_links to app_links.
Also applies to: 9-9, 12-15, 20-20, 25-25, 27-27
82-103: LGTM! Well-structured mock setup with proper cleanup.The mock setup follows best practices:
- Proper registration/unregistration of mocks
- Default behavior setup for AppLinks
- Clean isolation between tests
309-405: LGTM! Comprehensive URI link handling tests.Excellent test coverage for the app_links migration:
- Success cases for initial URI and stream updates
- Error handling for PlatformException and FormatException
- Stream error handling
- Proper cleanup verification
|
@palisadoes |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #2710 +/- ##
====================================================
+ Coverage 96.55% 96.80% +0.25%
====================================================
Files 189 189
Lines 9994 9996 +2
====================================================
+ Hits 9650 9677 +27
+ Misses 344 319 -25 ☔ View full report in Codecov by Sentry. |
palisadoes
added
the
ignore-sensitive-files-pr
|
Please do a minor commit to restart so the tests |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
test/widget_tests/pre_auth_screens/splash_screen_test.dart (4)
3-3: Consider removing unused imports instead of suppressing the warning.While the
unused_importdirective suppresses the warning, it's better to maintain clean code by removing unused imports. This helps reduce compilation time and keeps the codebase maintainable.
68-109: LGTM! Comprehensive test setup with proper isolation.The test setup is well-implemented with proper registration and cleanup of mocks. The stack trace demangling configuration improves test debugging in CI environments.
Consider adding a brief comment explaining why
mockAppLinksandmockUserConfigare initialized with default behaviors insetUp:setUp(() { mockAppLinks = MockAppLinks(); mockUserConfig = MockUserConfig(); + // Set up default behaviors to prevent null responses in basic widget tests when(mockAppLinks.getInitialLink()).thenAnswer((_) async => null); when(mockAppLinks.uriLinkStream).thenAnswer((_) => const Stream.empty()); });
305-401: Enhance URI link handling tests with additional validations.While the URI link handling tests cover the basic scenarios, consider these improvements:
- Add validation for the URI scheme:
testWidgets("should handle initial URI successfully", (tester) async { await tester.runAsync(() async { // Arrange final uri = Uri.parse('talawa://example.com'); when(mockAppLinks.getInitialLink()).thenAnswer((_) async => uri); when(mockAppLinks.uriLinkStream).thenAnswer((_) => Stream.value(uri)); // Act: Pump the widget await tester.pumpWidget(createSplashScreenLight()); await tester.pumpAndSettle(); // Assert verify(mockAppLinks.getInitialLink()).called(1); + expect(uri.scheme, equals('talawa'), 'URI scheme should be "talawa"'); expect(find.byKey(const Key('SplashScreenScaffold')), findsOneWidget); }); });
- Add assertions for error handling tests:
testWidgets("should handle PlatformException when getting initial URI", (tester) async { await tester.runAsync(() async { // Arrange - when(mockAppLinks.getInitialLink()) - .thenThrow(PlatformException(code: 'TEST_ERROR')); + const errorCode = 'TEST_ERROR'; + const errorMessage = 'Test error message'; + when(mockAppLinks.getInitialLink()) + .thenThrow(PlatformException(code: errorCode, message: errorMessage)); when(mockAppLinks.uriLinkStream) .thenAnswer((_) => const Stream.empty()); // Act await tester.pumpWidget(createSplashScreenLight()); await tester.pumpAndSettle(); // Assert verify(mockAppLinks.getInitialLink()).called(1); + // Verify the widget still renders without crashing + expect(find.byKey(const Key('SplashScreenScaffold')), findsOneWidget); }); });
122-134: Consider adding timeout handling for async operations.While the tests properly use
runAsyncfor async operations, they might benefit from timeout handling to prevent hanging tests:testWidgets("Testing if Splash Screen shows up", (tester) async { await tester.runAsync(() async { + // Add a timeout to prevent hanging tests + const timeout = Duration(seconds: 5); + final completer = Completer<void>(); + + // Setup a timeout future + Future.delayed(timeout).then((_) { + if (!completer.isCompleted) { + completer.completeError(TimeoutException( + 'Test timed out after ${timeout.inSeconds} seconds', + )); + } + }); + await tester.pumpWidget(createSplashScreenLight()); await tester.pumpAndSettle(); // ... rest of the test + completer.complete(); + await completer.future; }); });Also applies to: 214-226, 307-321
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/widget_tests/pre_auth_screens/splash_screen_test.dart(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Checking codebase
🔇 Additional comments (1)
test/widget_tests/pre_auth_screens/splash_screen_test.dart (1)
Line range hint
28-65: LGTM! Well-structured widget creation helpers.The helper functions are well-organized, follow the DRY principle, and provide good test isolation with different theme modes.
|
@noman2002 please review. |
|
@noman2002 Please review. |
palisadoes
merged commit a144878
into
PalisadoesFoundation:develop-postgres
What kind of change does this PR introduce?
Issue Number:
uni_linkstoapp_linksdue to Discontinuation #2706Did you add tests for your changes?
Yes
Tests are written for all changes made in this PR.
Test coverage meets or exceeds the current coverage (~90/95%).
Snapshots/Videos:
Summary
Does this PR introduce a breaking change?
Checklist for Repository Standards
coderaabbitaireview suggestions?Have you read the contributing guide?
Summary by CodeRabbit
Release Notes
Dependencies
uni_linkspackage withapp_linksfor improved deep linking functionalityDeep Linking
Testing