🐛 Check modal open state before calling Navigator.pop when closing

[AlexV525]

Description

Adding _isOpen check when calling Navigator.pop in closeModal to avoid unnecessary navigation changes.

Resolves #331

[Copilot]

Pull request overview

This PR aims to prevent Navigator.pop() from being called when the AppKit modal was never opened, addressing issue #331.

Changes:

• Add an _isOpen guard before attempting to Navigator.pop() in closeModal.

Comments suppressed due to low confidence (1)

packages/reown_appkit/lib/modal/appkit_modal_impl.dart:1411

• closeModal calls _close() before checking _isOpen. Since _close() sets _isOpen = false when the modal was open, the new guard if (_isOpen && _context != null) will always be false in the open case, so Navigator.pop() will never run and the modal route may remain on the stack. Capture the previous open state (e.g., final wasOpen = _isOpen;) before _close(), or move the pop logic into _close() before toggling _isOpen so you only pop when it was actually open.

    // If we aren't open, then we can't and shouldn't close
    _close();
    if (_isOpen && _context != null) {
      final canPop = Navigator.of(_context!, rootNavigator: true).canPop();
      if (canPop) {
        Navigator.of(_context!, rootNavigator: true).pop();
      }
    }

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[ignaciosantise]

hey @AlexV525 👋 do we need this change?

closeModal should close the modal unconditionally, and maybePop could refuse to close it in some cases, right?

[AlexV525]

This is a regular practise navigation op for packages with my experience. Using maybePop will call predicate normally if users have customized the Navigator.

[ignaciosantise]

Hey @AlexV525 sorry the delay 🙏

Agreed it’s good practice for general navigation, but _close() mutates internal state (sets _isOpen = false, clears widget stack) before the pop. If maybePop gets vetoed, the navigator and SDK state diverge — the modal is still on screen but the SDK considers it closed, and it won’t retry. Since this is our own programmatic dismiss, canPop() + pop() keeps things consistent.

Happy to keep just the !_isOpen guard — that’s the real fix.