[3.x backport] fix(instant): make forced conversion sheet non-dismissible#126
Merged
mhamann merged 1 commit intoMay 12, 2026
Merged
Conversation
* fix(instant): make forced conversion sheet non-dismissible When `forceInstantUserConversion` is enabled, the sign-in sheet must be non-dismissible until the user actually adds an identifier. Previously the sheet defaulted to dismissible via swipe, tap-outside, and any Hub close message, which left customers with large populations of unconverted instant users. - BottomSheetViewController gains an `isUserDismissalDisabled` flag that disables swipe-to-dismiss and tap-outside-to-dismiss at presentation, and that the Hub's `can_touch_background_to_dismiss` message cannot re-enable. - `hideBottomSheet` and `HubViewController.hide()` honor the same lock, so Hub-dispatched `closeHubViewController`/`signOut` messages cannot close the sheet either. - InstantUsers observes the post-conversion auth-level transition and releases the lock once the user is no longer `.instant`, so the standard post-auth auto-close path still runs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(instant): apply lock live and re-trigger close on release Addresses review feedback from PR #125: - BottomSheetViewController: `isUserDismissalDisabled` now has a `didSet` that updates the live `sheetController` so the lock takes effect on an already-presented sheet. Track the Hub's last-requested dismissibility separately so releasing the lock restores it rather than unconditionally re-enabling (Qodo #1, #2). - Rownd: `releaseForcedConversionLock` now re-invokes `hide()` on the active HubViewProtocol so a post-auth auto-close that lost the race against `UserData.fetch` propagating `authLevel` still closes the sheet. Guarded on the prior locked state so unlocking a non-held lock is a no-op (avoids side effects in tests). - Add `Rownd._bottomSheetIsLocked` internal accessor for tests. - Tests: cover lock engagement on `.instant`, release on `.verified`, and the once-per-session gate after release. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reviewer's GuideBackport of the instant-user forced conversion behavior to the 3.x line, introducing a lock around the sign-in bottom sheet so it cannot be user- or Hub-dismissed during forced conversion, and adding tests and state management to ensure the lock engages/release correctly and only once per session. Sequence diagram for forced conversion bottom sheet lock during instant user conversionsequenceDiagram
participant InstantUsers
participant Rownd
participant BottomSheetController
participant HubTimer
participant HubViewController
InstantUsers->>Rownd: requestSignInForcedConversion(signInOptions)
Rownd->>BottomSheetController: isUserDismissalDisabled = true
Rownd->>Rownd: requestSignIn(signInOptions)
HubTimer->>HubViewController: hide()
HubViewController->>BottomSheetController: hideBottomSheet()
alt [isUserDismissalDisabled]
HubViewController->>HubViewController: return
end
InstantUsers->>Rownd: releaseForcedConversionLock()
Rownd->>BottomSheetController: isUserDismissalDisabled = false
Rownd->>HubViewController: hide()
HubViewController->>BottomSheetController: hideBottomSheet()
BottomSheetController->>HubViewController: dismiss()
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Review Summary by QodoMake forced conversion sheet non-dismissible until user converts
WalkthroughsDescription• Make forced instant user conversion sheet non-dismissible until conversion completes • Add lock mechanism to prevent swipe, tap-outside, and Hub-dispatched dismissals • Auto-release lock when user transitions from instant to verified auth level • Retry post-auth close on lock release to handle race conditions with Hub timer Diagramflowchart LR
A["User triggers forced conversion"] --> B["Lock engaged on bottom sheet"]
B --> C["Sheet blocks swipe/tap-outside/Hub close"]
D["User adds identifier"] --> E["Auth level transitions to verified"]
E --> F["Lock released"]
F --> G["Post-auth auto-close proceeds"]
File Changes1. Sources/Rownd/Rownd.swift
|
Code Review by Qodo
1. Auto-close can be skipped
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Several of the new instant-user tests share nearly identical setup (store/context creation, config flags, initial dispatches, and delays); consider extracting a small helper or fixture to centralize this logic and make the tests easier to maintain.
- The tests rely on fixed
Task.sleepdelays (e.g., 50ms/200ms) before asserting lock state, which could be flaky under load; you might instead rely solely on thewaitUntilhelper (or a similar condition-based wait) to synchronize on the actual state change rather than elapsed time.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Several of the new instant-user tests share nearly identical setup (store/context creation, config flags, initial dispatches, and delays); consider extracting a small helper or fixture to centralize this logic and make the tests easier to maintain.
- The tests rely on fixed `Task.sleep` delays (e.g., 50ms/200ms) before asserting lock state, which could be flaky under load; you might instead rely solely on the `waitUntil` helper (or a similar condition-based wait) to synchronize on the actual state change rather than elapsed time.
## Individual Comments
### Comment 1
<location path="Sources/Rownd/Views/BottomSheetViewController.swift" line_range="27-36" />
<code_context>
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.
</code_context>
<issue_to_address>
**issue (bug_risk):** The lock currently also blocks programmatic dismissals, which contradicts the comment and may be too strict.
The docstring says programmatic dismissal is unaffected, but `hideBottomSheet` now returns early when the lock is active:
```swift
def hideBottomSheet(_ completion: (() -> Void)? = nil) {
if isUserDismissalDisabled {
logger.debug("Ignoring hideBottomSheet: forced-conversion lock is active")
return
}
sheetController?.dismiss(completion)
}
```
So all calls into `hideBottomSheet` are blocked while locked, and `HubViewController.hide()` already checks `isUserDismissalDisabled`, making this double enforcement. If the goal is to block only user-driven dismissals, consider restricting the lock to user-interaction paths (e.g. `canTouchDimmingBackgroundToDismiss` / gesture logic) and let `hideBottomSheet` always dismiss, or provide a separate API that bypasses the lock for trusted, SDK-initiated closes.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+27
to
+36
| // 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) |
There was a problem hiding this comment.
issue (bug_risk): The lock currently also blocks programmatic dismissals, which contradicts the comment and may be too strict.
The docstring says programmatic dismissal is unaffected, but hideBottomSheet now returns early when the lock is active:
def hideBottomSheet(_ completion: (() -> Void)? = nil) {
if isUserDismissalDisabled {
logger.debug("Ignoring hideBottomSheet: forced-conversion lock is active")
return
}
sheetController?.dismiss(completion)
}So all calls into hideBottomSheet are blocked while locked, and HubViewController.hide() already checks isUserDismissalDisabled, making this double enforcement. If the goal is to block only user-driven dismissals, consider restricting the lock to user-interaction paths (e.g. canTouchDimmingBackgroundToDismiss / gesture logic) and let hideBottomSheet always dismiss, or provide a separate API that bypasses the lock for trusted, SDK-initiated closes.
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+188
to
+196
| internal static func releaseForcedConversionLock() { | ||
| let wasLocked = inst.bottomSheetController.isUserDismissalDisabled | ||
| inst.bottomSheetController.isUserDismissalDisabled = false | ||
| guard wasLocked else { return } | ||
| // The post-auth auto-close may have fired and been blocked while the lock was held | ||
| // (Hub schedules hide() 1.5s after `.authentication` but `authLevel` propagates from a | ||
| // separate UserData.fetch — the timer can lose the race). Retry the close now. | ||
| (inst.bottomSheetController.controller as? HubViewProtocol)?.hide() | ||
| } |
There was a problem hiding this comment.
1. Auto-close can be skipped 🐞 Bug ☼ Reliability
Rownd.releaseForcedConversionLock() retries closing the Hub by calling hide() on bottomSheetController.controller, but that controller is assigned asynchronously via Task in displayHub/displayViewControllerOnTop. If authLevel transitions out of .instant before controller is assigned, the cast to HubViewProtocol fails and the Hub may stay open after conversion.
Agent Prompt
## Issue description
`releaseForcedConversionLock()` attempts a single immediate close by calling `hide()` on `inst.bottomSheetController.controller`, but `controller` is only assigned later inside `Task { @MainActor in ... }` in `displayHub()`/`displayViewControllerOnTop()`. If forced-conversion unlock happens before that assignment, the optional cast to `HubViewProtocol` yields `nil` and the auto-close retry becomes a no-op.
## Issue Context
This PR’s goal includes reliably auto-closing after `authLevel` transitions out of `.instant`. The current implementation depends on `bottomSheetController.controller` being set at the time of unlock, which is not guaranteed due to the async Task-based presentation.
## Fix Focus Areas
- Sources/Rownd/Rownd.swift[181-196]
- Sources/Rownd/Rownd.swift[416-455]
### Implementation guidance
Pick one of these robust patterns:
1. **Two-phase hide:** keep the current immediate hide, and also schedule a retry on the next MainActor turn (e.g., `Task { @MainActor in await Task.yield(); (controller as? HubViewProtocol)?.hide() }`).
2. **Pending-hide flag:** store a `pendingForcedConversionHide` flag when unlock happens and `controller` is nil; then consume it in `displayHub()` after `bottomSheetController.controller` is assigned.
3. **Dismiss via presenter state:** if `inst.bottomSheetController.presentingViewController != nil`, dismiss the presented sheet controller directly instead of relying on `controller` casting.
Whichever approach you choose, ensure the Hub closes even when unlock occurs before `displayHub()` has assigned/presented the controller.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Backports #125 to the v3.x maintenance line.
Cherry-picked from
8731aac(the squash-merge of #125 intomain) onto a newrelease/3.xbranch cut from tag3.14.10. The cherry-pick auto-merged a single context overlap inRownd.swift(addedregisterSuperTokensSyncEventHandler()call from #124 isn't present here, since #124 is the v4 breaking change we're deliberately leaving out).Summary
When
Rownd.config.forceInstantUserConversionis enabled, the sign-in sheet now:closeHubViewController/signOutmessages.authLeveltransitions out of.instant), including a retry of the post-authhide()to handle the race between the Hub's 1.5s timer andUserData.fetchpropagatingauthLevel.No Hub-side changes required.
Test plan
RowndTests/InstantUsersTests— all 7 tests pass (4 existing + 3 new for this fix).generic/platform=iOS Simulator.forceInstantUserConversion = truein the example AppDelegate.Release path
This PR targets
release/3.x(newly created from3.14.10). Merge → run the project's release tooling againstrelease/3.xto cut e.g.3.14.11.🤖 Generated with Claude Code
Summary by Sourcery
Enforce a non-dismissible sign-in sheet for forced instant user conversion and ensure it auto-closes once conversion completes.
Bug Fixes:
Enhancements:
Tests: