Conversation
PR SummaryEnhanced deep linking capabilities by adding support for Telegram deep links with "tg" scheme and improved WalletConnect URI handling. Added window color customization through JavaScript interface and included error handling for Telegram app availability. 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.
✅ LGTM!
Review Summary
Commits Considered (1)
- aa21958: Hot fix - telegram deep linking
Files Processed (2)
- app/src/main/AndroidManifest.xml (1 hunk)
- app/src/main/java/com/flowfoundation/wallet/page/browser/widgets/LilicoWebView.kt (1 hunk)
Actionable Comments (0)
Skipped Comments (2)
-
app/src/main/java/com/flowfoundation/wallet/page/browser/widgets/LilicoWebView.kt [274-281]
best practice: "Exception handling could be more specific"
-
app/src/main/java/com/flowfoundation/wallet/page/browser/widgets/LilicoWebView.kt [279-279]
best practice: "Consider logging error details"
|
Case 1: User click Crypto Kitties button, Telegram is not installed on the phone -> redirect to Play Store Screen.Recording.2025-05-16.at.4.35.14.PM.mov |
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (1)
- 7a2ab7e: Hot fix - telegram deep linking
Files Processed (3)
- app/src/main/java/com/flowfoundation/wallet/page/browser/widgets/LilicoWebView.kt (10 hunks)
- app/src/main/java/com/flowfoundation/wallet/page/component/deeplinking/Utils.kt (1 hunk)
- app/src/main/java/com/flowfoundation/wallet/widgets/webview/JsInterface.kt (1 hunk)
Actionable Comments (1)
-
app/src/main/java/com/flowfoundation/wallet/page/browser/widgets/LilicoWebView.kt [278-281]
security: "Potential security issue with package validation"
Skipped Comments (2)
-
app/src/main/java/com/flowfoundation/wallet/page/browser/widgets/LilicoWebView.kt [332-341]
possible issue: "Missing error handling for web intent"
-
app/src/main/java/com/flowfoundation/wallet/widgets/webview/JsInterface.kt [54-56]
possible bug: "Input validation missing for color parsing"
| val pkg = ri.activityInfo.packageName | ||
| if (pkg != "android" && pkg != context.packageName) { | ||
| return true // found a 3rd-party app that can open tg: | ||
| } |
There was a problem hiding this comment.
The current package validation for Telegram deep linking is insufficient. A malicious app could register the tg: scheme and intercept these links. Consider explicitly checking for the official Telegram package name (org.telegram.messenger) instead of accepting any non-system package.
There was a problem hiding this comment.
org.telegram.messenger didn't seem to open the app when it is installed from Telegram website (https://telegram.org/android) instead of PlayStore, so I removed this check
Screen.Recording.2025-05-16.at.4.50.16.PM.movCase 2: Telegram is installed |
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (1)
- dbf897f: Hot fix - telegram deep linking
Files Processed (2)
- app/src/main/AndroidManifest.xml (2 hunks)
- app/src/main/java/com/flowfoundation/wallet/page/browser/widgets/LilicoWebView.kt (10 hunks)
Actionable Comments (2)
-
app/src/main/java/com/flowfoundation/wallet/page/browser/widgets/LilicoWebView.kt [299-319]
possible bug: "Potential crash when handling Telegram deep links"
-
app/src/main/java/com/flowfoundation/wallet/page/browser/widgets/LilicoWebView.kt [295-295]
security: "Potential security risk with deep link handling"
Skipped Comments (1)
-
app/src/main/java/com/flowfoundation/wallet/page/browser/widgets/LilicoWebView.kt [321-324]
possible issue: "Potential state inconsistency in WebView"
| e.printStackTrace() | ||
| false | ||
| } | ||
| } else if (it.scheme == "tg") { |
There was a problem hiding this comment.
The code accepts any URI with the 'tg' scheme without validating the host or path. This could potentially be exploited if a malicious site provides a crafted 'tg://' URL. Consider adding validation for expected Telegram deep link formats before processing them.
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (1)
- 3a3501f: Hot fix - telegram deep linking
Files Processed (2)
- app/src/main/AndroidManifest.xml (1 hunk)
- app/src/main/java/com/flowfoundation/wallet/page/browser/widgets/LilicoWebView.kt (10 hunks)
Actionable Comments (1)
-
app/src/main/java/com/flowfoundation/wallet/page/browser/widgets/LilicoWebView.kt [296-297]
security: "Potential security risk with FLAG_ACTIVITY_NEW_TASK"
Skipped Comments (1)
-
app/src/main/java/com/flowfoundation/wallet/page/browser/widgets/LilicoWebView.kt [299-319]
best practice: "Missing error logging for Telegram app launch failure"
| val openTg = Intent(Intent.ACTION_VIEW, it) | ||
| .addFlags(Intent.FLAG_ACTIVITY_NEW_TASK) |
There was a problem hiding this comment.
Using FLAG_ACTIVITY_NEW_TASK with an Intent that handles external data (deep links) could potentially lead to task hijacking. Consider using FLAG_ACTIVITY_RESET_TASK_IF_NEEDED in combination or evaluating if the flag is truly necessary for your use case.
| } | ||
| } | ||
|
|
||
| @JavascriptInterface |
There was a problem hiding this comment.
This is an unrelated issue that was being thrown to the console from internal web browser, so I added handling for it
| request?.url?.let { | ||
| logd(TAG, "shouldOverrideUrlLoading URL: $it, scheme: ${it.scheme}") | ||
|
|
||
| if (it.scheme == "wc") { |
There was a problem hiding this comment.
I think you should handle frw://, fcw:// and all universal link in here. and check if it's wc connection url
There was a problem hiding this comment.
Yeah we currently have one check for wc URLs above the tg check. I realize we are missing the frw and fcw cases here, will add this in a separate PR to address the middle window appearing during the wallet session auth.
| context.startActivity(openTg) | ||
| } catch (e: Exception) { | ||
| // Nothing handled it → send user to Play Store (or web fallback) | ||
| val pkgName = "org.telegram.messenger" |
There was a problem hiding this comment.
this is wired, if we can't open just show a toast. No need go to play store.
There was a problem hiding this comment.
Screen.Recording.2025-05-16.at.5.17.30.PM.mov
Updated:
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Commits Considered (1)
- a29c063: Hot fix - telegram deep linking
Files Processed (2)
- app/src/main/java/com/flowfoundation/wallet/page/browser/widgets/LilicoWebView.kt (11 hunks)
- app/src/main/res/values/strings.xml (1 hunk)
Actionable Comments (0)
Skipped Comments (2)
-
app/src/main/java/com/flowfoundation/wallet/page/browser/widgets/LilicoWebView.kt [302-304]
best practice: "Generic exception handling could mask specific errors"
-
app/src/main/java/com/flowfoundation/wallet/page/browser/widgets/LilicoWebView.kt [306-308]
enhancement: "WebView history manipulation could affect user experience"
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Commits Considered (1)
- a69552b: Hot fix - telegram deep linking
Files Processed (0)
Actionable Comments (0)
Skipped Comments (0)
| view?.stopLoading() | ||
| view?.clearHistory() | ||
| return true | ||
| } else if (it.host == "link.lilico.app" || it.host == "frw-link.lilico.app" || it |
There was a problem hiding this comment.
let's register all universal link as a enum class, then check the host in the list of enum.
enum UniversaLinks: String {
case frw = ""
case fcw = ""
case wallet = ""
}
enum Deeplinks: {
case frw
case fcw
}
UniversaLinks.values.contains(host)
Deeplinks.values.contains(scheme)
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (1)
- 906f29e: WC deep linking
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/Utils.kt (1 hunk)
Actionable Comments (1)
-
app/src/main/java/com/flowfoundation/wallet/page/component/deeplinking/Utils.kt [188-194]
possible bug: "Potential null pointer exception in URL processing"
Skipped Comments (1)
-
app/src/main/java/com/flowfoundation/wallet/page/browser/widgets/LilicoWebView.kt [304-312]
best practice: "Missing error handling specificity in Telegram deep linking"
| if (uri.host?.contains("lilico.app") == true && uri.path == "/wc") { | ||
| logd(TAG, "Processing WalletConnect link URL: $uri") | ||
| val wcUriParam = uri.getQueryParameter("uri") | ||
| if (wcUriParam != null) { | ||
| return@runCatching URLDecoder.decode(wcUriParam, StandardCharsets.UTF_8.name()) | ||
| } | ||
| } |
There was a problem hiding this comment.
The code checks if the host contains 'lilico.app' but doesn't verify that the host itself isn't null before the contains check. While the null check is there with the safe call operator, restructuring the condition to uri.host?.contains("lilico.app") == true && uri.path == "/wc" would be more robust and prevent potential NPEs.
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (1)
- b18d701: WC deep linking
Files Processed (1)
- app/src/main/java/com/flowfoundation/wallet/page/browser/widgets/LilicoWebView.kt (11 hunks)
Actionable Comments (1)
-
app/src/main/java/com/flowfoundation/wallet/page/browser/widgets/LilicoWebView.kt [322-326]
possible issue: "Potential null pointer dereference in wallet link handling"
Skipped Comments (1)
-
app/src/main/java/com/flowfoundation/wallet/page/browser/widgets/LilicoWebView.kt [307-311]
best practice: "Improve error handling for Telegram deep links"
| val wcUri = getWalletConnectUri(it) | ||
| wcUri?.let { uri -> | ||
| logd(TAG, "Wallet Connect URI: $uri") | ||
| WalletConnect.get().pair(uri) | ||
| } |
There was a problem hiding this comment.
The code calls toString() on the result of getWalletConnectUri() without null checking in the existing code, but now properly handles the null case. However, if wcUri is null, the code silently fails without any error handling or user feedback. Consider adding appropriate error handling:
val wcUri = getWalletConnectUri(it)
if (wcUri != null) {
logd(TAG, "Wallet Connect URI: $wcUri")
WalletConnect.get().pair(wcUri)
} else {
logd(TAG, "Failed to parse Wallet Connect URI")
// Show appropriate error to user
}
* Hot fix - telegram deep linking (#1074) * Hot fix - telegram deep linking * Hot fix - telegram deep linking * Hot fix - telegram deep linking * Hot fix - telegram deep linking * Hot fix - telegram deep linking * Hot fix - telegram deep linking * WC deep linking * WC deep linking * fix: update the version code --------- Co-authored-by: Lea Lobanov <44328396+lealobanov@users.noreply.github.com>
2 participants
Related Issue
Summary of Changes
Support deep links with "tg" scheme.
Need Regression Testing
Risk Assessment
Additional Notes
Screenshots (if applicable)