[Feature] PopupBridge++#90
Conversation
Add app switch toggle for backwards compatibility
| /// instead of opening a browser. Defaults to false for backward compatibility. | ||
| /// This is specific to the popup bridge flow and is separate from the JS SDK's | ||
| /// appSwitchWhenAvailable which controls non-webview mobile browser app switch. | ||
| public init(webView: WKWebView, prefersEphemeralWebBrowserSession: Bool = true, enablePopupBridgeAppSwitch: Bool = false) { |
There was a problem hiding this comment.
why does this take a different approach vs venmo?
There was a problem hiding this comment.
This PR follows the existing Popup Bridge pattern and only adds PayPal-specific app-switch handling behind the feature flag. I kept Venmo unchanged here to avoid expanding scope, since the HLD and QA for this work were focused on the PayPal path.
| guard let self, | ||
| let url = notification.userInfo?["url"] as? URL else { |
There was a problem hiding this comment.
| guard let self, | |
| let url = notification.userInfo?["url"] as? URL else { | |
| guard let self, let url = notification.userInfo?["url"] as? URL else { |
There was a problem hiding this comment.
What if self is nil here?
There was a problem hiding this comment.
Hey Jax, if self is nil, the bridge has already been deallocated and there’s nothing left to handle, so returning early is the correct behavior. I split the guards to make that explicit.
|
|
||
| if let returnUrlScheme { | ||
| let deepLinkReturnUrlPrefix = "\(returnUrlScheme)://\(host)/" | ||
| deepLinkJs = """ |
There was a problem hiding this comment.
What does deepLinkJs mean/do?
There was a problem hiding this comment.
deepLinkJs injects the getDeepLinkReturnUrlPrefix() JavaScript function onto window.popupBridge
| return '\(deepLinkReturnUrlPrefix)'; | ||
| }; | ||
| """ | ||
| deepLinkProperty = """ |
There was a problem hiding this comment.
What does deepLinkProperty mean/do?
There was a problem hiding this comment.
deepLinkProperty injects the deepLinkReturnUrlPrefix JavaScript property onto window.popupBridge
buzzamus
left a comment
There was a problem hiding this comment.
It doesn't appear this PR builds. Looks like there are issues around launchAppReturnObserver. Once this builds and has been thoroughly tested please let us know when this is ready for review
| Self.analyticsService.sendAnalyticsEvent(PopupBridgeAnalytics.succeeded, sessionID: sessionID) | ||
|
|
||
| let script = """ | ||
| ;(function() { |
There was a problem hiding this comment.
I am assuming there is a specific reason this starts with a semicolon, but I am just curious what the reason is
- Add missing convenience init - Fix compiler error - Fix failing test - Add code comment explaining the leading semicolon - Add `## unreleased` CHANGELOG entry for `enablePopupBridgeAppSwitch` and new JS APIs
* docs: create SECURITY.md * docs: update SECURITY.md
* Add integration test coverage
buzzamus
left a comment
There was a problem hiding this comment.
I think the git history got messed up somewhere since it has previous PR changes as part of this PR's commit history and also left a few other comments.
| > List GitHub usernames for everyone who contributed to this pull request. | ||
|
|
||
| - | ||
| - |
There was a problem hiding this comment.
I don't think this file is from this PR. I see several files committed from previous PRs, so I think your git history is screwed up somehow.
| codeCoverageEnabled = "YES"> | ||
| <Testables> | ||
| <TestableReference | ||
| skipped = "NO"> |
There was a problem hiding this comment.
Same here. I don't think this change is meant for this PR as it was already merged into main
| get { stubBody } | ||
| set { stubBody = newValue } | ||
| } | ||
| } |
There was a problem hiding this comment.
Same with this file. This is already merged into main
| startWasCalled = true | ||
| capturedURL = url | ||
| } | ||
| } |
| } | ||
|
|
||
| override func stopLoading() {} | ||
| } |
| @@ -0,0 +1,145 @@ | |||
| import AuthenticationServices | |||
| @@ -0,0 +1,53 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
| 085C05B32F9A78E000460CD9 /* MockWebAuthSession.swift in Sources */ = {isa = PBXBuildFile; fileRef = 085C05B22F9A78E000460CD9 /* MockWebAuthSession.swift */; }; | ||
| 085C05B82F9A79DA00460CD9 /* MockScriptMessage.swift in Sources */ = {isa = PBXBuildFile; fileRef = 085C05B72F9A79DA00460CD9 /* MockScriptMessage.swift */; }; | ||
| 085C05BA2F9A7AB900460CD9 /* StubURLProtocol.swift in Sources */ = {isa = PBXBuildFile; fileRef = 085C05B92F9A7AB900460CD9 /* StubURLProtocol.swift */; }; | ||
| 451348272D8CAD7C009265C9 /* WebViewScriptHandler.swift in Sources */ = {isa = PBXBuildFile; fileRef = 451348262D8CAD6D009265C9 /* WebViewScriptHandler.swift */; }; |
There was a problem hiding this comment.
This project file has all of the changes from recent PRs in it, so this will also need to be resolved. Not sure if this is from a bad merge or a rebase screwed up the history, but these need to be removed. The best way will be to instead of manually removing see if you can fix the git history
| - Avoid accessing, modifying, or deleting data that does not belong to you | ||
| - Make a good faith effort to avoid disruption to production systems | ||
|
|
||
| We appreciate responsible disclosure and your efforts to keep Braintree SDK users safe. |
There was a problem hiding this comment.
This is another previously merged change
|
|
||
| /// Exposed for testing | ||
| convenience init( | ||
| webView: WKWebView, |
There was a problem hiding this comment.
There are quite a few initializers now in this file, which to me is a big code smell. I think we should try and find a different way to handle. If we are using default values this wouldn't be a breaking change so, I am not sure if we need so many new ones created
|
Closing in favor of #95 |
Summary of changes
enablePopupBridgeAppSwitchflag toPOPPopupBridgethat, when enabled, allows the SDK to launch the native PayPal app for checkout instead of opening a browser session.NotificationCenterobserver handles the deep link return URL and injects the result back into the WebView. If the app launch fails, the flow falls back to the existingWebAuthenticationSessionbehavior.isPayPalInstalled,launchApp(), andgetDeepLinkReturnUrlPrefix()to JavaScript, and fires analytics events for app detection and launch outcomes.Checklist
Authors