[bug][ios]: sheet breaks detach of modals on reload (pressing R)

[hirbod]

It seems like the sheet prevents modals and transparentModals from being detached correctly on a hard reload (R button), making the app unresponsive and forcing me to reboot the app many times during development. While this is not a huge deal since it does not affect production, it is super annoying in development when you work on sheets and have some reloads or hot refreshes that mount and unmount the transparent modal behind.

It works fine for modals if the sheet is not open, but as soon as the sheet is open, the modal is not detaching. See the two videos (red background added to my stack to demonstrate the problem).

No sheet, app responsive after reboot	With sheet not responsive anymore
b.mp4	a.mp4

Also, I tested the same behavior with multiple modals stacked on top of eachother. It works flawlessly, the problem only occurs with true sheet being open.
Kindly appreciate a fix here.

[lodev09]

Yeah, I'm not sure if there's a proper fix for that since it's how IOS handles viewcontrollers internally.

I think what's happening is that the Modal gets destroyed first leaving TrueSheet in the air. It's the same issue with RN navigation modals as well.

I tried finding a way to fix it before but couldn't find one. It's a deep rabbit hole when working with stacked modal view controllers.

Sorry.

[hirbod]

It is not happening with RN navigation modals, thats what I tried to showcase. The video with only red (without the sheet) gets removed, but when I have a modal, that opened the sheet, it does not work anymore.

The video showcases quite the opposite. On hot reload, the sheet gets removed and it breaks the RN Modal.

[lodev09]

Okay.. I guess I got confused :D. So, sheet gets removed first and then RN modal did not get the chance to detach. Again, this is a known issue and I'm suffering from it as well even with react-nativation/native. It's something to do with hot reloading + modals.

I wonder if it can be fixed here:

react-native-true-sheet/ios/TrueSheetView.swift

Lines 161 to 163 in d30b873

	func invalidate() {
	viewController.dismiss(animated: true)
	}

Try to disable animation on that line.

[lodev09]

Just curious, what's your reason in using RN modal? I assume it's the red one.

[hirbod]

I'll try to disable the animation to see if that fixes it. The main reason we're using the transparentModal is as I explained here #32. We're using react-navigation.

[hirbod]

Again, this is a known issue and I'm suffering from it as well even with react-nativation/native

This is not true. I opened two transparentModal on top of each other without using your library, and reloading successfully detaches them. If one of those modals contains your sheet, it breaks. We're not using import { Modal } from 'react-native'

    <Stack
      screenOptions={{
        headerShown: false,
        animation: 'none',
        presentation: 'transparentModal',
        contentStyle: { backgroundColor: 'transparent' },
      }}
    />

Summoning the debug king @intergalacticspacehighway

I tried disabling the animation, even traversing all UiViewController and trying to manually remove them, I wasn't successful unfortunately.

[hirbod]

I think this issue is related software-mansion/react-native-screens#2113

After inspecting the file, I did some testing out of curiosity:

func invalidate() {
    dismissAllPresentedViewControllers(from: UIApplication.shared.keyWindow?.rootViewController) {
        self.viewController.dismiss(animated: false) {
            self.viewController.view.removeFromSuperview()
        }
    }
}

private func dismissAllPresentedViewControllers(from viewController: UIViewController?, completion: @escaping () -> Void) {
    guard let viewController = viewController else {
        completion()
        return
    }
    
    if let presentedVC = viewController.presentedViewController {
        presentedVC.dismiss(animated: false) {
            self.dismissAllPresentedViewControllers(from: presentedVC, completion: completion)
        }
    } else {
        completion()
    }
}

and

//
//  Created by Jovanni Lo (@lodev09)
//  Copyright (c) 2024-present. All rights reserved.
//
//  This source code is licensed under the MIT license found in the
//  LICENSE file in the root directory of this source tree.
//

@objc(TrueSheetViewManager)
class TrueSheetViewManager: RCTViewManager {
  override var methodQueue: DispatchQueue! {
    return DispatchQueue.main
  }
  
  override static func requiresMainQueueSetup() -> Bool {
    return true
  }
  
  override func view() -> UIView? {
    return TrueSheetView(with: bridge)
  }
  
  private func getTrueSheetView(_ tag: NSNumber) -> TrueSheetView {
    return bridge.uiManager.view(forReactTag: tag) as! TrueSheetView
  }
  
  @objc
  func present(_ tag: NSNumber, index: Int, resolve: @escaping RCTPromiseResolveBlock, reject: @escaping RCTPromiseRejectBlock) {
    let trueSheetView = getTrueSheetView(tag)
    trueSheetView.present(at: index, promise: Promise(resolver: resolve, rejecter: reject))
  }
  
  @objc
  func dismiss(_ tag: NSNumber, resolve: @escaping RCTPromiseResolveBlock, reject: @escaping RCTPromiseRejectBlock) {
    let trueSheetView = getTrueSheetView(tag)
    trueSheetView.dismiss(promise: Promise(resolver: resolve, rejecter: reject))
  }
  
  @objc
  func invalidate(_ tag: NSNumber) {
    let trueSheetView = getTrueSheetView(tag)
    trueSheetView.invalidate()
  }
}

Normal closing:

Dismissing view controller: <UIViewController: 0x15ee09100>
Dismissing view controller: <RNSScreen: 0x348c84000>

Reload before my PoC:

Dismissing view controller: <UIViewController: 0x15ee09100>

After my changes

Dismissing view controller: <UIViewController: 0x15ee09100>
..... bunch of initalization
..... Running "main" with {"rootTag":11,"initialProps":{"concurrentRoot":false}}
Dismissing view controller: <RNSScreen: 0x348c84000>

So I am able to kill the attached screen. Obv all of these methods do not belong to this library, I just did some testing. I think this must be patched and fixed in RNSScreenStack.mm

ssMax.-.2024-06-08.at.11.51.54.mp4

[hirbod]

I was able to patch the issue in RNSScreenStack.mm

// updated this
- (void)invalidate {
    _invalidated = YES;
    [self dismissAllPresentedViewControllersFrom:_controller completion:^{
        // Ensure presented modals are removed and the controller is detached from its parent
        [_presentedModals removeAllObjects];
        [_controller willMoveToParentViewController:nil];
        [_controller removeFromParentViewController];
    }];
}

- (void)dismissOnReload {
    dispatch_async(dispatch_get_main_queue(), ^{
        [self invalidate];
    });
}

// added this
- (void)dismissAllPresentedViewControllersFrom:(UIViewController *)viewController completion:(void (^)(void))completion {
    if (viewController.presentedViewController) {
        [viewController.presentedViewController dismissViewControllerAnimated:NO completion:^{
            [self dismissAllPresentedViewControllersFrom:viewController completion:completion];
        }];
    } else {
        completion();
    }
}

fix-foreign-dismissal.mp4

[hirbod]

Fix is here software-mansion/react-native-screens#2175

[lodev09]

OMG thank you for fixing rn screens 😂

I'm actually traveling so I couldn't help much. I'll take a look at implementing the same fix on the sheet since it could also be an issue with stacked sheet.

[lodev09]

The main reason we're using the transparentModal is as I explained here #32. We're using react-navigation.

I see. An alternative solution I would suggest is capture route.params with the screen instead of implementing a transparent modal. See here

useEffect(() => {
  ;(async () =>  {
    if (id) {
      await sheet.current?.present()
      navigation.setParams({ id: undefined })
    }
  })()
}, [id])

[hirbod]

We rely on many lifecycle/focus/blur events; this approach wouldn’t work. Furthermore, your suggestion would require mounting all of the views in the root. Our sheets can be accessed from everywhere and are mounted “lazily” thanks to our implementation, and no extra code needed.

[lodev09]

Ah that makes sense. Definitely not good solution if you have lots of sheets that accepts links.

Good luck.

[hirbod]

Well everything works fine now with the patch :). Once we get your PR merged it's hopefully mounting a bit faster

[lodev09]

yep. The PR works for IOS but I'm seeing layout issues with android when orientation changes. I won't be able to fix them until I get back home in few days. However, I would appreciate if you could test it more and I can merge it when I get a chance then create separate PR for the layout issue :)

[hirbod]

Sounds good, if you consider it ready for testing I could give it a shot. Orientation change isn't an issue for us but maybe for others, but sounds good to address separately.

[wcastand]

i'm not sure it's related but on expo when i press R, my ios app crashes with this error

TrueSheet/TrueSheetView.swift:140: Fatal error: Unexpectedly found nil while implicitly unwrapping an Optional value

hope that can help, not sure if it's only a reload/dev thing or not tho yet

[hirbod]

Not related to this issue, but what Expo version are you using?

[wcastand]

using :

• "expo": "^51.0.11",
• "@lodev09/react-native-true-sheet": "^0.11.3",

[hirbod]

@wcastand it's weird, I mean I reloaded at least 50 times and never ran into this. Is that like happening constantly to you? Anyway, I can see the issue. RCTBridge is optional, because it gets invalidated on Reload. If the viewControllerDidChangeWidth is triggered in that moment (I couldn't reproduce), it could crash. I'll prepare a PR

Edit:
There you go: #35

[wcastand]

not 100% but almost everytime yeah, in my repo i had a scenario where i would get it 100%, but when i dev, it's not all the time.

i have a TrueSheet in my input component so it's present in a lot of screen so yeah, happens a lot.

nice for the Pr i will test as soon as it's possible then

[hirbod]

I guess you have a case that triggers a width change, thus the event is fired for you.
The PR should fix this effectively.

[lodev09]

@hirbod PR looks good with minor comments.

Closing this. Thank you!

[hirbod]

@lodev09 just an FYI that my PR was merged but they slightly changed the implementation logic: software-mansion/react-native-screens#2175

My PR made sure to close all modals recursively, including third party modals but they decided to limit this to RNS, which kinda makes sense but likely won't work as smooth as my initial idea. So we should try and test. Maybe we need appropriate destroy methods that close on dealloc within your library. Just as a little heads up.