-
Notifications
You must be signed in to change notification settings - Fork 3
fix(instant): make forced conversion sheet non-dismissible #125
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
Changes from all commits
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 |
|---|---|---|
|
|
@@ -24,6 +24,26 @@ class BottomSheetViewController: UIViewController { | |
| var latestTargetHeight: CGFloat = 0.9 | ||
| var isKeyboardOpen = false | ||
|
|
||
| // When true, swipe-to-dismiss and tap-outside-to-dismiss are disabled for this presentation, | ||
| // and the Hub's `can_touch_background_to_dismiss` message cannot re-enable them. Programmatic | ||
| // dismissal (e.g. after a successful sign-in) is unaffected. Reset on viewDidDisappear. | ||
| // The didSet applies the change to a live sheetController, so toggling the lock after | ||
| // presentation still takes effect (and unlock restores the Hub's most recent preference). | ||
| var isUserDismissalDisabled: Bool = false { | ||
| didSet { | ||
| guard isUserDismissalDisabled != oldValue, let sheetController = sheetController else { return } | ||
| if isUserDismissalDisabled { | ||
| sheetController.setCanTouchDimmingBackgroundToDismiss(false) | ||
| } else { | ||
| sheetController.setCanTouchDimmingBackgroundToDismiss(hubRequestedCanTouchToDismiss) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Tracks the most recent Hub-requested dismissibility state so that releasing the lock | ||
| // restores whatever the Hub last asked for instead of unconditionally re-enabling. | ||
| private var hubRequestedCanTouchToDismiss: Bool = true | ||
|
|
||
| override func viewWillAppear(_ animated: Bool) { | ||
| super.viewWillAppear(animated) | ||
| guard let controller = controller else { | ||
|
|
@@ -54,6 +74,9 @@ class BottomSheetViewController: UIViewController { | |
|
|
||
| sheetController = presentAsBottomSheet(controller, theme: theme, behavior: behavior) | ||
|
|
||
| if isUserDismissalDisabled { | ||
| sheetController?.setCanTouchDimmingBackgroundToDismiss(false) | ||
| } | ||
| } | ||
|
|
||
| override func viewDidLoad() { | ||
|
|
@@ -69,6 +92,8 @@ class BottomSheetViewController: UIViewController { | |
| override func viewDidDisappear(_ animated: Bool) { | ||
| super.viewDidDisappear(animated) | ||
| self.controller = nil | ||
| self.isUserDismissalDisabled = false | ||
| self.hubRequestedCanTouchToDismiss = true | ||
| } | ||
|
|
||
| func updateBottomSheetHeight(_ number: CGFloat) { | ||
|
|
@@ -77,6 +102,10 @@ class BottomSheetViewController: UIViewController { | |
| } | ||
|
|
||
| public func hideBottomSheet(_ completion: (() -> Void)? = nil) { | ||
| if isUserDismissalDisabled { | ||
|
Contributor
Author
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. Nit — completion silently dropped. When locked, the
Contributor
Author
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. Leaving as-is. The only caller passing a completion ( |
||
| logger.debug("Ignoring hideBottomSheet: forced-conversion lock is active") | ||
| return | ||
| } | ||
| sheetController?.dismiss(completion) | ||
| } | ||
|
|
||
|
|
@@ -99,6 +128,10 @@ class BottomSheetViewController: UIViewController { | |
| } | ||
|
|
||
| func canTouchDimmingBackgroundToDismiss(_ enable: Bool) { | ||
| hubRequestedCanTouchToDismiss = enable | ||
| if isUserDismissalDisabled && enable { | ||
| return | ||
| } | ||
| if let sheetController = sheetController { | ||
| sheetController.setCanTouchDimmingBackgroundToDismiss(enable) | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -182,11 +182,16 @@ public class HubViewController: UIViewController, HubViewProtocol, BottomSheetHo | |
| self.dismiss(animated: true) | ||
| return | ||
| } | ||
|
|
||
|
|
||
| if bottomSheetController.isUserDismissalDisabled { | ||
|
Contributor
Author
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. Bug — lock bypass. The new check guards hostController.dismiss(animated: true)…on the bottom-sheet host. That path neither checks Suggest adding the same early-return on the lock at line 141, or routing all dismissals through
Contributor
Author
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. Leaving as-is per offline discussion. The viewWillDisappear path requires something to disappear the Hub VC from outside the SDK, which our customers shouldn't be doing on the forced-conversion flow. Worth revisiting if we see field reports of escape via this path. |
||
| logger.debug("Ignoring HubViewController.hide(): forced-conversion lock is active") | ||
| return | ||
| } | ||
|
|
||
| if (isBottomSheetDismissing) { | ||
| return | ||
| } | ||
|
|
||
| isBottomSheetDismissing = true | ||
| bottomSheetController.hideBottomSheet({ | ||
| self.dismiss(animated: true) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,7 @@ import Combine | |
| class InstantUsers { | ||
| private let context: Context | ||
| private var cancellables = Set<AnyCancellable>() | ||
| private var hasTriggeredConversion: Bool = false | ||
|
|
||
| init( | ||
| context: Context | ||
|
|
@@ -36,24 +37,26 @@ class InstantUsers { | |
| .removeDuplicates( | ||
| by: == | ||
| ) | ||
| .first { | ||
| isAuthenticated, | ||
| authLevel in | ||
| isAuthenticated && authLevel == .instant | ||
| } | ||
| .sink { | ||
| isAuthenticated, | ||
| authLevel in | ||
|
|
||
| var signInOptions = RowndSignInOptions() | ||
| signInOptions.title = "Add a sign-in method" | ||
| signInOptions.subtitle = "To make sure you can always access your account, please add a sign-in method." | ||
| signInOptions.intent = .signUp | ||
| Rownd | ||
| .requestSignIn(signInOptions) | ||
|
|
||
| subscriber | ||
| .unsubscribe() | ||
| .sink { [weak self] isAuthenticated, authLevel in | ||
| guard let self = self, isAuthenticated else { return } | ||
|
|
||
| if !self.hasTriggeredConversion && authLevel == .instant { | ||
| self.hasTriggeredConversion = true | ||
|
|
||
| var signInOptions = RowndSignInOptions() | ||
| signInOptions.title = "Add a sign-in method" | ||
| signInOptions.subtitle = "To make sure you can always access your account, please add a sign-in method." | ||
| signInOptions.intent = .signUp | ||
| Rownd.requestSignInForcedConversion(signInOptions) | ||
| return | ||
| } | ||
|
|
||
| // User has converted to a non-instant auth level (verified, unverified, guest). | ||
| // Release the lock so the Hub's post-success auto-close can proceed. | ||
| if self.hasTriggeredConversion && authLevel != .instant && authLevel != .unknown { | ||
|
Contributor
Author
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. Race — sheet can stay open after successful conversion.
store.dispatch(store.state.auth.onReceiveAuthTokens(
AuthState(accessToken: authMessage.accessToken, refreshToken: authMessage.refreshToken)
))
...
DispatchQueue.main.asyncAfter(deadline: .now() + 1.5) { [weak self] in
if initialJsFunctionArgsAsJson == self?.jsFunctionArgsAsJson {
self?.hubViewController?.hide()
}
}The
The user sees a sheet that won't close on its own. They can swipe-dismiss it (because the lock is released), but that's an unexpected UX. Suggest re-triggering dismissal when releasing the lock, e.g.: Rownd.releaseForcedConversionLock()
// Re-attempt the post-auth close that may have been blocked by the lock.
(Rownd.inst.bottomSheetController.controller as? HubViewProtocol)?.hide()
subscriber.unsubscribe()(Or add a
Contributor
Author
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. Fixed in 9ea56d0.
Contributor
Author
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. Question —
Contributor
Author
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. Keeping current behavior ( |
||
| Rownd.releaseForcedConversionLock() | ||
| subscriber.unsubscribe() | ||
| } | ||
| }.store( | ||
| in: &self.cancellables | ||
| ) | ||
|
|
||
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.
1. Lock not applied live
🐞 Bug≡ CorrectnessAgent Prompt
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation toolsThere 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.
Addressed in 9ea56d0.
isUserDismissalDisablednow has adidSetthat pushes through to the livesheetControllerviasetCanTouchDimmingBackgroundToDismiss(...), so toggling the flag after presentation takes effect immediately. Same path covers your second comment — releasing the lock restores the Hub's most recent dismissibility request (tracked in a separatehubRequestedCanTouchToDismissso we don't unconditionally forcetrueon unlock).