Feat: force dismiss captive signing - #2
Conversation
Expo AsyncFunction handlers are dispatched on AsyncFunctionQueue (a non-main queue). DSMManager APIs (clearAllWebCookies, logout) must run on the main thread, so endSigningSession could touch them off-main and intermittently hang or crash. Hop to main before calling clearWebCookiesAsync. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
presentCaptiveSigningWithUrl presents the DocuSign signing WebView as a native modal above the app's view hierarchy. The SDK only dismisses it on the user's own Finish/Cancel — so a host app that tears down the session programmatically (e.g. a remote "end consultation" signal) cannot remove the modal: reset() resolved the JS promise but left the WebView on screen. reset() now dismisses the presented modal on the key window's root view controller. Gated on an in-flight signing (hadPending) so reset() never tears down an unrelated modal when no signing is active. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
IronTony
left a comment
IronTony
left a comment
There was a problem hiding this comment.
The main-thread hop is a correct catch. The dismissal has two problems, and one gap means the change never runs for most consumers.
The path that motivated this PR is not covered. useDocuSignSigning.reset() calls endSigningSession(), not the native reset() (src/useDocuSignSigning.ts:165). A remote end-of-session signal arriving mid-signing goes through the hook, so hook consumers still get a stranded modal after this lands. endSigningSession already resolves the pending promise, so the dismissal belongs there too.
No Android counterpart. Android's reset() leaves CaptiveSigningActivity on screen, which opens a parity gap against a README that leads with an iOS/Android parity section. A follow-up is fine, let's just note it here.
Housekeeping: the commits carry Co-Authored-By trailers for an AI assistant. I'll squash-merge with a rewritten message.
| if hadPending { | ||
| DispatchQueue.main.async { | ||
| let root = UIApplication.shared.connectedScenes | ||
| .compactMap { $0 as? UIWindowScene } | ||
| .flatMap { $0.windows } | ||
| .first(where: { $0.isKeyWindow })?.rootViewController | ||
| if root?.presentedViewController != nil { | ||
| root?.dismiss(animated: true) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This dismisses every modal above the root view controller, including ones the host app owns.
presentCaptiveSigning and presentCaptiveSigningWithUrl both present from Self.topmostViewController(), which walks the presentation chain. If the host app already has a modal on screen (signing started from a sheet, or from a modally presented screen), the DocuSign controller is presented by that modal. Dismissing the key window's root controller then tears down the app's modal along with DocuSign's. The hadPending gate does not prevent that, contrary to the comment above it.
The SDK hands you the controller you actually want. Both presentCaptiveSigning completion closures receive it and discard it: { [weak self] (_: UIViewController?, error: Error?) in. Store it in a private weak var presentedSigningVC: UIViewController?, dismiss through presentedSigningVC?.presentingViewController?.dismiss(animated:completion:), and clear it in resolvePending. A weak ref is nil when nothing is presented, so hadPending drops out, and so does the key window walk on 607-610, which duplicates the default argument of topmostViewController().
| .flatMap { $0.windows } | ||
| .first(where: { $0.isKeyWindow })?.rootViewController | ||
| if root?.presentedViewController != nil { | ||
| root?.dismiss(animated: true) |
There was a problem hiding this comment.
The dismissal is not sequenced with the teardown or with the promise.
There is no completion handler here, so reset() runs straight into clearWebCookiesAsync. That wipes the whole WKWebsiteDataStore while the DocuSign WKWebView is still on screen mid-animation, and the JS promise resolves before the modal is gone. Call reset() and then immediately start a new signing: topmostViewController() returns the dismissing controller, and you get "already presenting" or a presentation on a detached VC. Sequence it as dismiss, then wipe, then resolve.
Small thing on the same lines: if let root, root.presentedViewController != nil avoids optional-chaining root twice.
| // DSMManager APIs (clearAllWebCookies, logout) must run on the main thread. | ||
| // Expo async functions are dispatched on AsyncFunctionQueue (non-main), so | ||
| // we must hop to main before touching any DSMManager API. | ||
| guard Thread.isMainThread else { |
There was a problem hiding this comment.
The guard sits after the state mutation, so the stateQueue.sync block above runs twice: once on the caller's thread, once on the main-queue re-entry. If a presentCaptiveSigning lands in between, the second pass cancels that fresh session.
Hop to main at the top of the function. Better still, put the guard inside clearWebCookiesAsync. That is the only place touching DSMManager.clearAllWebCookies() and WKWebsiteDataStore off-main, so one guard covers every caller now and later. DSMManager.logout() below was already safe: it runs from clearWebCookiesAsync's completion, which is dispatched on main.
| // we must hop to main before touching any DSMManager API. | ||
| guard Thread.isMainThread else { | ||
| DispatchQueue.main.async { [weak self] in | ||
| self?.endSigningSession(completion: completion) |
There was a problem hiding this comment.
completion is dropped when self is nil and the JS promise never settles. The equivalent hop in reset() handles it: guard let self = self else { completion(); return }. Latent today because the manager is a singleton, but let's keep the two paths consistent.
| @@ -543,6 +543,16 @@ internal final class DocuSignManager: NSObject { | |||
| } | |||
| _ = pendingResolved // silence unused-warning; kept for future telemetry | |||
There was a problem hiding this comment.
Dead code in the function you are already touching. var pendingResolved on 529 is assigned and never read, and this line exists only to silence the warning. Drop both.
2 participants
No description provided.