Skip to content

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

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.

Sign up for GitHub

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

Open
AlexV525 wants to merge 2 commits into
base: develop
Choose a base branch
from
+7 −5
Open

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cd2efdf
🐛 Check modal open state before calling navigator pop when closing
AlexV525 Jan 27, 2026
a0f7f02
Refactor closeModal to set disconnectOnClose after check
AlexV525 Jan 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions packages/reown_appkit/lib/modal/appkit_modal_impl.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1400,14 +1400,16 @@ class ReownAppKitModal

@override
void closeModal({bool disconnectSession = false}) async {
// If we aren't open, then we can't and shouldn't close.
// This is a necessary check to avoid unnecessary navigator pops.
if (!_isOpen) {
return;
}

_disconnectOnClose = disconnectSession;
// If we aren't open, then we can't and shouldn't close
_close();
if (_context != null) {
final canPop = Navigator.of(_context!, rootNavigator: true).canPop();
if (canPop) {
Navigator.of(_context!, rootNavigator: true).pop();
}
Navigator.maybeOf(_context!, rootNavigator: true)?.maybePop();
Copy link
Copy Markdown
Collaborator

@ignaciosantise ignaciosantise Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @AlexV525 👋 do we need this change?

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

Copy link
Copy Markdown
Contributor Author

@AlexV525 AlexV525 Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Collaborator

@ignaciosantise ignaciosantise Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
_notify();
}
Expand Down