-
Notifications
You must be signed in to change notification settings - Fork 283
Jared/cursor agent/long press warning #5950
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: develop
Are you sure you want to change the base?
Changes from all commits
f1357d7
534b5a3
303a373
854b747
404fbc0
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 | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -1,3 +1,7 @@ | ||||||||
| <resources> | ||||||||
| <string name="app_name">Edge</string> | ||||||||
| <string name="shortcut_contact_support_short">Contact Support</string> | ||||||||
| <string name="shortcut_contact_support_long">Contact support for help from a live agent</string> | ||||||||
|
Contributor
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. Warning: Inconsistent copy between platforms.
Same feature with different wording can be confusing for localization and UX. Recommendation: Pick one and use it on both platforms. |
||||||||
| <string name="shortcut_do_not_uninstall_short">⚠️ Save 2FA First!</string> | ||||||||
| <string name="shortcut_do_not_uninstall_long">⚠️ Login requires 2FA & credentials!</string> | ||||||||
|
Contributor
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. Critical: Unescaped ampersand in XML. In XML, Fix: <string name="shortcut_do_not_uninstall_long">⚠️ Login requires 2FA & credentials!</string>
Comment on lines
+5
to
+6
Contributor
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. Warning: Double emoji on Android vs single on iOS.
Android uses Recommendation: Remove |
||||||||
| </resources> | ||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| <?xml version="1.0" encoding="utf-8"?> | ||
| <shortcuts xmlns:android="http://schemas.android.com/apk/res/android"> | ||
| <shortcut | ||
|
Contributor
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. Warning: Shortcut order differs between platforms.
If the order is intentional for platform UX, add a comment explaining the rationale. Otherwise, align for consistency. |
||
| android:shortcutId="contact_support" | ||
| android:enabled="true" | ||
| android:icon="@mipmap/ic_launcher" | ||
|
Contributor
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. Suggestion: Both Android shortcuts use the same app launcher icon ( iOS uses distinct SF Symbols ( |
||
| android:shortcutShortLabel="@string/shortcut_contact_support_short" | ||
| android:shortcutLongLabel="@string/shortcut_contact_support_long"> | ||
| <intent | ||
| android:action="android.intent.action.VIEW" | ||
| android:data="https://support.edge.app/hc/en-us?chat=open" /> | ||
|
Contributor
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. Critical: Android shortcuts will not work — https URLs are not opened in the browser. The iOS implementation in if url.scheme == "https" || url.scheme == "http" {
UIApplication.shared.open(url)
return true
}However, Android has no equivalent native handling. The intent sends the URL to Recommended fix (consistent with iOS): Add native Android handling in Alternative design consideration: If the preference is to have
This would centralize URL handling logic in one place (the deep link infrastructure) rather than having platform-specific native handling. |
||
| </shortcut> | ||
| <shortcut | ||
| android:shortcutId="do_not_uninstall" | ||
| android:enabled="true" | ||
| android:icon="@mipmap/ic_launcher" | ||
| android:shortcutShortLabel="@string/shortcut_do_not_uninstall_short" | ||
| android:shortcutLongLabel="@string/shortcut_do_not_uninstall_long"> | ||
| <intent | ||
| android:action="android.intent.action.VIEW" | ||
| android:data="https://support.edge.app/hc/en-us/articles/24469866252443-Getting-a-new-phone" /> | ||
| </shortcut> | ||
| </shortcuts> | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,7 @@ import UserNotifications | |
| class AppDelegate: UIResponder, UIApplicationDelegate { | ||
| var window: UIWindow? | ||
| var securityView: UIView? | ||
| private var pendingShortcutItem: UIApplicationShortcutItem? | ||
|
Contributor
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. Suggestion: Consider adding a brief comment explaining why shortcut handling is deferred. The pattern of storing // Shortcut actions are deferred until React Native is fully initialized
private var pendingShortcutItem: UIApplicationShortcutItem?would clarify the intent for future readers. |
||
|
|
||
| var reactNativeDelegate: ReactNativeDelegate? | ||
| var reactNativeFactory: RCTReactNativeFactory? | ||
|
|
@@ -49,6 +50,10 @@ class AppDelegate: UIResponder, UIApplicationDelegate { | |
| _ application: UIApplication, | ||
| didFinishLaunchingWithOptions launchOptions: [UIApplication.LaunchOptionsKey: Any]? = nil | ||
| ) -> Bool { | ||
| if let shortcutItem = launchOptions?[.shortcutItem] as? UIApplicationShortcutItem { | ||
| pendingShortcutItem = shortcutItem | ||
| } | ||
|
|
||
| // Initialize SDK's: | ||
| initializeSentry() | ||
| FirebaseApp.configure() | ||
|
|
@@ -72,9 +77,35 @@ class AppDelegate: UIResponder, UIApplicationDelegate { | |
| launchOptions: launchOptions | ||
| ) | ||
|
|
||
| if let shortcutItem = pendingShortcutItem { | ||
| _ = handleShortcutItem(shortcutItem) | ||
| pendingShortcutItem = nil | ||
| } | ||
|
|
||
| return true | ||
| } | ||
|
|
||
| func application( | ||
| _ application: UIApplication, | ||
| performActionFor shortcutItem: UIApplicationShortcutItem, | ||
| completionHandler: @escaping (Bool) -> Void | ||
| ) { | ||
| let handled = handleShortcutItem(shortcutItem) | ||
| completionHandler(handled) | ||
| } | ||
|
|
||
| private func handleShortcutItem(_ shortcutItem: UIApplicationShortcutItem) -> Bool { | ||
| guard let url = URL(string: shortcutItem.type) else { return false } | ||
|
|
||
|
Contributor
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. Suggestion: UIApplication.shared.open(url, options: [:]) { success in
if !success {
// Log or handle failure
}
}Not critical, but could be useful for future troubleshooting. |
||
| // Open https/http URLs in the default browser, otherwise handle as deep link | ||
| if url.scheme == "https" || url.scheme == "http" { | ||
| UIApplication.shared.open(url) | ||
| return true | ||
| } | ||
|
|
||
| return RCTLinkingManager.application(UIApplication.shared, open: url, options: [:]) | ||
| } | ||
|
|
||
| /** | ||
| * Periodic background fetch logic. | ||
| * Edge addition. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -57,6 +57,29 @@ | |
| </array> | ||
| <key>CFBundleVersion</key> | ||
| <string>99999999</string> | ||
| <key>UIApplicationShortcutItems</key> | ||
|
Contributor
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. Localization: These shortcut strings are hardcoded in English only. The app supports 13 locales via React Native ( Recommendation: Use
Similarly for Android, add This could be deferred to a follow-up PR, but should at least be documented as a known limitation. |
||
| <array> | ||
| <dict> | ||
| <key>UIApplicationShortcutItemTitle</key> | ||
| <string>⚠️ Save 2FA First!</string> | ||
| <key>UIApplicationShortcutItemSubtitle</key> | ||
| <string>Login requires 2FA & credentials!</string> | ||
| <key>UIApplicationShortcutItemIconSymbolName</key> | ||
| <string>nosign</string> | ||
| <key>UIApplicationShortcutItemType</key> | ||
|
Contributor
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. Suggestion: Storing URLs there works, but it mixes identity with data. Consider using a type like |
||
| <string>https://support.edge.app/hc/en-us/articles/24469866252443-Getting-a-new-phone</string> | ||
| </dict> | ||
| <dict> | ||
| <key>UIApplicationShortcutItemTitle</key> | ||
| <string>Contact Support</string> | ||
| <key>UIApplicationShortcutItemSubtitle</key> | ||
| <string>Get help from our live support agents</string> | ||
| <key>UIApplicationShortcutItemIconSymbolName</key> | ||
| <string>message.fill</string> | ||
| <key>UIApplicationShortcutItemType</key> | ||
| <string>https://support.edge.app/hc/en-us?chat=open</string> | ||
| </dict> | ||
| </array> | ||
| <key>LSApplicationQueriesSchemes</key> | ||
| <array> | ||
| <string>https</string> | ||
|
|
||
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.
Warning: "Data loss" wording may be misleading.
The shortcut points to "Getting a new phone" and emphasizes 2FA and credentials, not funds or wallet data. "Data loss" could imply loss of crypto.
Suggestion: