hotwired / turbo-ios

iOS framework for making Turbo native apps
MIT License
874 stars 88 forks source link

Check for presenting instead of presented #212

Closed olivaresf closed 3 months ago

olivaresf commented 4 months ago

I found another small issue. It's possible that users present a screen modally outside of TurboNavigator on TurboNavigator's main navigation controller.

For example:

  1. A URL request fails.
  2. A custom alert ("Cancel", "Settings") is presented in turboNavigator.rootViewController.
  3. "Settings" is tapped.
  4. route(url: settings) is proposed.

Expected: 5. turboNavigator.modalRootViewController is presented with Settings as its root view controller. Current: 5. turboNavigator.rootViewController is already presenting (the alert) so Settings is pushed to turboNavigator.modalRootViewController which exists but is not presented.

The current behavior assumes TurboNavigator itself has presented the custom alert (2.) but no modal has actually been presented via route(url:). This causes future modals proposed through route(url:) (4.) to never be presented. Instead, they're pushed/replaced on modalRootViewController since TurboNavigator believes it is already visible.

Instead of checking whether the main nav controller has a presented modal, check if TurboNavigator's modal stack is being presented. This way, manually presenting a modal does not interfere with TurboNavigator's modal stack.

joemasilotti commented 4 months ago

I'm not seeing the current behavior you're experiencing. Applying the diff below, I see the "settings" page (actually /new) presented correctly as a modal. Can you help me understand what I'm missing?

  1. Apply the diff below
  2. Tap "Hit a HTTP 404 error"
  3. Tap "Settings" from the native alert
diff --git a/Demo/SceneController.swift b/Demo/SceneController.swift
index 0ad7ee4..1413775 100644
--- a/Demo/SceneController.swift
+++ b/Demo/SceneController.swift
@@ -93,11 +93,12 @@ extension SceneController: TurboNavigatorDelegate {
     func visitableDidFailRequest(_ visitable: Visitable, error: Error, retryHandler: RetryBlock?) {
         if let turboError = error as? TurboError, case let .http(statusCode) = turboError, statusCode == 401 {
             promptForAuthentication()
-        } else if let errorPresenter = visitable as? ErrorPresenter {
-            errorPresenter.presentError(error, retryHandler: retryHandler)
         } else {
             let alert = UIAlertController(title: "Visit failed!", message: error.localizedDescription, preferredStyle: .alert)
-            alert.addAction(UIAlertAction(title: "OK", style: .default, handler: nil))
+            alert.addAction(UIAlertAction(title: "Settings", style: .default) { [unowned self] _ in
+                navigator.route(rootURL.appendingPathComponent("new"))
+            })
+            alert.addAction(UIAlertAction(title: "Cancel", style: .cancel, handler: nil))
             navigator.activeNavigationController.present(alert, animated: true)
         }
     }
olivaresf commented 3 months ago

I'm not seeing the current behavior you're experiencing. Applying the diff below, I see the "settings" page (actually /new) presented correctly as a modal. Can you help me understand what I'm missing?

In the diff you shared, Turbo will present a UIAlertController which immediately dismisses itself when a button is tapped. That's definitely on me since I used that as an example, 🤦🏻‍♂️ sorry.

However, @svara confirmed my suspicion. Even though (in my mind) it makes more sense to check if the modal controller is presented than checking if the root controller is presenting, the real smell here is that if you're presenting outside of Turbo, it's your responsibility to make sure you dismiss too.

So, for now, I'm closing this one. Thanks both for your feedback!