Update Expo linking, simplify popup deeplink callback #16769
Conversation
WalkthroughThe changes involve modifications to the Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
🪧 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
CodeRabbit Configuration File (
|
✅ Actions performedReview triggered.
|
martykan
added
the
no-project
![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
🧹 Nitpick comments (2)
suite-native/module-connect-popup/src/hooks/useConnectPopupNavigation.ts (2)
31-32: Track device connection/unlock state handling.The TODO comments highlight an important edge case that needs to be addressed. Similar patterns exist in other modals (biometrics, coin enabled) that could serve as references.
Would you like me to:
- Create an issue to track this technical debt?
- Generate a proposal for handling device connection/unlock states based on existing modal patterns?
39-44: Consider enhancing error handling and readability.While the implementation is cleaner, consider these improvements:
- Combine the early returns for better readability
- Add error handling for URL parsing
- Memoize the navigation callback
Here's a suggested implementation:
useEffect(() => { - if (!featureFlagEnabled) return; - if (!url || !isConnectPopupUrl(url)) return; - const parsedUrl = Linking.parse(url); - navigation.navigate(RootStackRoutes.ConnectPopup, { parsedUrl }); + if (!featureFlagEnabled || !url || !isConnectPopupUrl(url)) return; + + try { + const parsedUrl = Linking.parse(url); + navigation.navigate(RootStackRoutes.ConnectPopup, { parsedUrl }); + } catch (error) { + console.error('Failed to parse Connect Popup URL:', error); + } }, [url, navigation, featureFlagEnabled]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
suite-native/module-connect-popup/src/hooks/useConnectPopupNavigation.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Unit Tests
- GitHub Check: Releases revision Checks
- GitHub Check: Other Checks
- GitHub Check: Type Checking
- GitHub Check: Linting and formatting
- GitHub Check: Build libs for publishing
- GitHub Check: prepare_android_test_app
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (2)
suite-native/module-connect-popup/src/hooks/useConnectPopupNavigation.ts (2)
1-29: LGTM! Well-structured imports and robust URL validation.The imports are well-organized, types are properly defined, and the URL validation logic effectively handles both development and production environments.
37-37: LGTM! Good use of theuseURLhook.Using
Linking.useURL()simplifies the URL handling by leveraging React's built-in state management.
martykan
force-pushed
the
chore/mobile-connect-popup-linking-logic
branch
from
dc05200 to
09dac31
Compare
martykan
changed the title
|
🚀 Expo preview is ready!
|
martykan
removed
the
no-project
suite-native/module-connect-popup/src/hooks/useConnectPopupNavigation.ts
Show resolved
Hide resolved
suite-native/module-connect-popup/src/hooks/useConnectPopupNavigation.ts
Outdated
Show resolved
Hide resolved
martykan
force-pushed
the
chore/mobile-connect-popup-linking-logic
branch
from
09dac31 to
cbca225
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
suite-native/module-connect-popup/src/hooks/useConnectPopupNavigation.ts (1)
42-47: Consider enhancing error reporting.While the try-catch implementation is good, consider using a proper error tracking solution instead of just
console.warnfor better visibility of issues in production.- console.warn('Invalid deeplink URL', { error, url }); + // Use your error tracking solution (e.g., Sentry) + reportError(new Error('Invalid deeplink URL'), { error, url });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
suite-native/module-connect-popup/src/hooks/useConnectPopupNavigation.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: build-deploy
- GitHub Check: build-deploy
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: EAS Update
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: transport-e2e-test
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: build
- GitHub Check: prepare_android_test_app
- GitHub Check: build-web
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: test
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (2)
suite-native/module-connect-popup/src/hooks/useConnectPopupNavigation.ts (2)
37-37: LGTM! Good simplification using theuseURLhook.The change to use
Linking.useURL()effectively simplifies the URL handling by automatically managing both initial URLs and subsequent URL changes.
39-48: Verify edge cases and optimize URL validation.Based on previous concerns about edge cases:
- Verify the behavior when rapidly receiving multiple URLs.
- Consider validating the URL before checking the feature flag for better performance.
useEffect(() => { - if (!featureFlagEnabled) return; - if (!url || !isConnectPopupUrl(url)) return; + if (!url) return; + if (!isConnectPopupUrl(url)) return; + if (!featureFlagEnabled) return; try { const parsedUrl = Linking.parse(url); navigation.navigate(RootStackRoutes.ConnectPopup, { parsedUrl });Let's verify the behavior with multiple URLs:
Description
There was an issue in
expo-linkingon iOS that was fixed in the latest version expo/expo#33031This PR also refactors the logic around
expo-linkingto use theiruseURLhook instead of manual callbacks.