Conversation
PR SummaryConsolidated deep linking logic by introducing a centralized URI handling system. Added new data structures and utility methods to manage different types of deep links (WalletConnect, Telegram, universal links) more efficiently. Improved error handling and logging throughout the deep linking process. Changes
autogenerated by presubmit.ai |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (1)
- 1636e84: WC deep linking
Files Processed (6)
- app/src/main/AndroidManifest.xml (1 hunk)
- app/src/main/java/com/flowfoundation/wallet/page/browser/widgets/LilicoWebView.kt (2 hunks)
- app/src/main/java/com/flowfoundation/wallet/page/component/deeplinking/DeepLinkType.kt (1 hunk)
- app/src/main/java/com/flowfoundation/wallet/page/component/deeplinking/DeepLinkingActivity.kt (3 hunks)
- app/src/main/java/com/flowfoundation/wallet/page/component/deeplinking/UriHandler.kt (1 hunk)
- app/src/main/java/com/flowfoundation/wallet/page/component/deeplinking/Utils.kt (4 hunks)
Actionable Comments (2)
-
app/src/main/java/com/flowfoundation/wallet/page/component/deeplinking/UriHandler.kt [61-64]
security: "Potential security risk with Intent flags"
-
app/src/main/java/com/flowfoundation/wallet/page/component/deeplinking/UriHandler.kt [79-85]
possible bug: "Potential race condition in WalletConnect initialization"
Skipped Comments (1)
-
app/src/main/java/com/flowfoundation/wallet/page/component/deeplinking/UriHandler.kt [157-157]
possible issue: "Missing return value documentation"
| try { | ||
| val intent = Intent(Intent.ACTION_VIEW, uri) | ||
| .addFlags(Intent.FLAG_ACTIVITY_NEW_TASK) | ||
| context.startActivity(intent) |
There was a problem hiding this comment.
Using FLAG_ACTIVITY_NEW_TASK alone for external activities can lead to task hijacking. Consider adding FLAG_ACTIVITY_CLEAR_TOP and validating the target package to ensure the intent only opens the official Telegram app.
| return try { | ||
| if (WalletConnect.isInitialized()) { | ||
| WalletConnect.get().pair(wcUri) | ||
| true | ||
| } else { | ||
| logd(TAG, "WalletConnect not initialized") | ||
| // Save for later processing or initialize |
There was a problem hiding this comment.
The code checks if WalletConnect is initialized but doesn't handle the case where it becomes initialized right after the check. Consider using a synchronized block or implementing a proper initialization callback mechanism.
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Commits Considered (1)
- 8cfabd7: WC deep linking
Files Processed (0)
Actionable Comments (0)
Skipped Comments (0)
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (7)
Files Processed (2)
- app/src/main/java/com/flowfoundation/wallet/page/browser/widgets/LilicoWebView.kt (11 hunks)
- app/src/main/java/com/flowfoundation/wallet/page/component/deeplinking/DeepLinkingActivity.kt (3 hunks)
Actionable Comments (2)
-
app/src/main/java/com/flowfoundation/wallet/page/browser/widgets/LilicoWebView.kt [283-308]
best practice: "Missing error handling for failed deep link actions"
-
app/src/main/java/com/flowfoundation/wallet/page/browser/widgets/LilicoWebView.kt [292-296]
possible bug: "Potential null pointer dereference"
Skipped Comments (1)
-
app/src/main/java/com/flowfoundation/wallet/page/component/deeplinking/DeepLinkingActivity.kt [36-41]
maintainability: "Early return without proper cleanup"
| when (it.scheme) { | ||
| DeepLinkScheme.WC.scheme -> { | ||
| logd(TAG, "Handling WalletConnect URI") | ||
| WalletConnect.get().pair(it.toString()) | ||
| return true | ||
| } | ||
| DeepLinkScheme.FRW.scheme, DeepLinkScheme.FCW.scheme -> { | ||
| if (it.path?.startsWith("/wc") == true) { | ||
| logd(TAG, "Handling ${it.scheme}://wc URI") | ||
| val uri = getWalletConnectUri(it) | ||
| uri?.let { wcUri -> | ||
| WalletConnect.get().pair(wcUri.toString()) | ||
| } | ||
| return true | ||
| } | ||
| true | ||
| } catch (e: URISyntaxException) { | ||
| e.printStackTrace() | ||
| false | ||
| } | ||
| } else if (it.host == "link.lilico.app" || it.host == "frw-link.lilico.app" || it | ||
| .host == "fcw-link.lilico.app" || it.host == "link.wallet.flow.com") { | ||
| DeepLinkScheme.TG.scheme -> { | ||
| logd(TAG, "Handling Telegram URI") | ||
| UriHandler.processUri(context, it) | ||
|
|
||
| // Stop the WebView navigation to avoid looping | ||
| view?.stopLoading() | ||
| view?.clearHistory() | ||
| return true | ||
| } | ||
| "intent" -> { |
There was a problem hiding this comment.
The deep link handling code should include error handling for cases where the actions fail. For example, when WalletConnect.pair() fails or when UriHandler.processUri() encounters an error. Consider wrapping these calls in try-catch blocks and showing appropriate error messages to users via Toast or other UI feedback mechanisms.
| val uri = getWalletConnectUri(it) | ||
| uri?.let { wcUri -> | ||
| WalletConnect.get().pair(wcUri.toString()) | ||
| } | ||
| return true |
There was a problem hiding this comment.
The code doesn't handle the case where getWalletConnectUri(it) returns null. The current implementation might lead to a potential null pointer exception if the URI parsing fails. Consider adding an else branch to handle the null case and provide appropriate user feedback.
lealobanov
changed the title
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (1)
- fa6e8c1: WC deep linking
Files Processed (3)
- app/src/main/java/com/flowfoundation/wallet/page/component/deeplinking/DeepLinkType.kt (1 hunk)
- app/src/main/java/com/flowfoundation/wallet/page/component/deeplinking/UriHandler.kt (1 hunk)
- app/src/main/java/com/flowfoundation/wallet/page/component/deeplinking/Utils.kt (4 hunks)
Actionable Comments (2)
-
app/src/main/java/com/flowfoundation/wallet/page/component/deeplinking/UriHandler.kt [161-161]
possible issue: "Missing error handling for pending action storage"
-
app/src/main/java/com/flowfoundation/wallet/page/component/deeplinking/DeepLinkType.kt [14-14]
possible bug: "Case-sensitive host comparison may cause issues"
Skipped Comments (1)
-
app/src/main/java/com/flowfoundation/wallet/page/component/deeplinking/UriHandler.kt [169-169]
best practice: "Use more specific exception handling"
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (1)
- b00f0ba: WC deep linking
Files Processed (1)
- app/src/main/java/com/flowfoundation/wallet/page/browser/widgets/LilicoWebView.kt (2 hunks)
Actionable Comments (2)
-
app/src/main/java/com/flowfoundation/wallet/page/browser/widgets/LilicoWebView.kt [279-279]
possible bug: "Potential null pointer dereference in URL handling"
-
app/src/main/java/com/flowfoundation/wallet/page/browser/widgets/LilicoWebView.kt [288-295]
possible issue: "Potential race condition in URL blocking check"
Skipped Comments (1)
-
app/src/main/java/com/flowfoundation/wallet/page/browser/widgets/LilicoWebView.kt [298-307]
possible bug: "Inconsistent WebView state management"
| isLoading = true | ||
| request?.url?.let { | ||
| logd(TAG, "shouldOverrideUrlLoading URL: $it, scheme: ${it.scheme}") | ||
| request?.url?.let { uri -> |
There was a problem hiding this comment.
The request?.url?.let block assumes the URL is non-null, but doesn't handle the case where request or url is null. Consider adding an else clause to handle null cases gracefully:
request?.url?.let { uri ->
// existing code
} ?: run {
return super.shouldOverrideUrlLoading(view, request)
}| uiScope { | ||
| if (BlockManager.isBlocked(it.toString())) { | ||
| logd(TAG, "URL blocked: $url") | ||
| showBlockedViewLayout(it.toString()) | ||
| if (BlockManager.isBlocked(uri.toString())) { | ||
| logd(TAG, "URL blocked: $uri") | ||
| showBlockedViewLayout(uri.toString()) | ||
| loadUrl("about:blank#blocked") | ||
| isLoading = false | ||
| return@uiScope | ||
| } | ||
| } |
There was a problem hiding this comment.
The URL blocking check is performed asynchronously in uiScope, but the method continues execution without waiting for the result. This could lead to a race condition where the URL starts loading before the blocking check completes. Consider restructuring the code to ensure the blocking check completes before proceeding with URL handling.
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (1)
- d6aeb7e: WC deep linking
Files Processed (2)
- app/src/main/java/com/flowfoundation/wallet/page/component/deeplinking/DeepLinkType.kt (1 hunk)
- app/src/main/java/com/flowfoundation/wallet/page/component/deeplinking/PendingActionHelper.kt (1 hunk)
Actionable Comments (2)
-
app/src/main/java/com/flowfoundation/wallet/page/component/deeplinking/DeepLinkType.kt [14-16]
possible bug: "Potential null pointer exception in host comparison"
-
app/src/main/java/com/flowfoundation/wallet/page/component/deeplinking/PendingActionHelper.kt [46-46]
possible bug: "Potential URI parsing exception not handled"
Skipped Comments (1)
-
app/src/main/java/com/flowfoundation/wallet/page/component/deeplinking/PendingActionHelper.kt [26-26]
performance: "Consider using apply() instead of commit() for better performance"
| fun fromHost(host: String?): UniversalLinkHost? = values().firstOrNull { | ||
| it.host.equals(host, ignoreCase = true) | ||
| } |
There was a problem hiding this comment.
The fromHost method compares a potentially null host parameter with it.host. While Kotlin's null safety would prevent a direct NPE, the current implementation might miss valid matches when host is null. Consider adding an early return for null host:
fun fromHost(host: String?): UniversalLinkHost? =
if (host == null) null
else values().firstOrNull { it.host.equals(host, ignoreCase = true) }| val deepLinkStr = getPrefs(context).getString(KEY_PENDING_DEEPLINK, null) ?: return null | ||
| return Uri.parse(deepLinkStr) | ||
| try { | ||
| val deepLinkStr = getPrefs(context).getString(KEY_PENDING_DEEPLINK, null) ?: return null |
There was a problem hiding this comment.
The Uri.parse() call could throw an IllegalArgumentException if the stored URI string is malformed. Consider wrapping the parsing in a try-catch block to handle malformed URIs gracefully:
try {
val deepLinkStr = getPrefs(context).getString(KEY_PENDING_DEEPLINK, null) ?: return null
return Uri.parse(deepLinkStr)
} catch (e: IllegalArgumentException) {
loge(TAG, "Invalid URI format: ${e.message}")
return null
}
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Commits Considered (1)
- 53289ee: WC deep linking
Files Processed (1)
- app/src/main/java/com/flowfoundation/wallet/page/component/deeplinking/PendingActionHelper.kt (1 hunk)
Actionable Comments (0)
Skipped Comments (3)
-
app/src/main/java/com/flowfoundation/wallet/page/component/deeplinking/PendingActionHelper.kt [26-26]
performance: "Performance improvement for SharedPreferences write operations"
-
app/src/main/java/com/flowfoundation/wallet/page/component/deeplinking/PendingActionHelper.kt [65-65]
performance: "Performance improvement for SharedPreferences write operations"
-
app/src/main/java/com/flowfoundation/wallet/page/component/deeplinking/PendingActionHelper.kt [29-31]
best practice: "Consolidate error logging statements"
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (1)
- ad1486c: WC deep linking
Files Processed (3)
- app/src/main/java/com/flowfoundation/wallet/page/browser/widgets/LilicoWebView.kt (4 hunks)
- app/src/main/java/com/flowfoundation/wallet/page/component/deeplinking/DeepLinkType.kt (1 hunk)
- app/src/main/java/com/flowfoundation/wallet/page/component/deeplinking/DeepLinkingActivity.kt (3 hunks)
Actionable Comments (1)
-
app/src/main/java/com/flowfoundation/wallet/page/component/deeplinking/DeepLinkType.kt [14-16]
possible bug: "Potential null pointer exception in host comparison"
Skipped Comments (2)
-
app/src/main/java/com/flowfoundation/wallet/page/browser/widgets/LilicoWebView.kt [300-303]
possible issue: "Potential race condition in WebView navigation handling"
-
app/src/main/java/com/flowfoundation/wallet/page/browser/widgets/LilicoWebView.kt [276-279]
enhancement: "Early return optimization for null request handling"
| fun fromHost(host: String?): UniversalLinkHost? = entries.firstOrNull { | ||
| it.host.equals(host, ignoreCase = true) | ||
| } |
There was a problem hiding this comment.
The fromHost() method accepts a nullable String parameter but doesn't handle the null case before using it in the equals() comparison. Consider adding a null check at the start of the method to avoid potential NPE:
fun fromHost(host: String?): UniversalLinkHost? =
if (host == null) null
else entries.firstOrNull { it.host.equals(host, ignoreCase = true) }
| FRW("frw"), | ||
| FCW("fcw"), | ||
| LILICO("lilico"), | ||
| TG("tg"), |
There was a problem hiding this comment.
I think the tg, http, https are not required in here.
2 participants
Related Issue
Summary of Changes
Need Regression Testing
Risk Assessment
Additional Notes
Screenshots (if applicable)