Skip to content

Guard success-path setState with mounted check in review send#525

Merged
n13 merged 1 commit into
mainfrom
fix/review-send-mounted-guard
Jun 15, 2026
Merged

Guard success-path setState with mounted check in review send#525
n13 merged 1 commit into
mainfrom
fix/review-send-mounted-guard

Conversation

@n13

@n13 n13 commented Jun 14, 2026

Copy link
Copy Markdown
Collaborator

Summary

Follow-up to #524. Now that the send flow blocks on the RPC submission before navigating, the await in _confirmSend can take a real network round-trip (plus retries). This widens the window for the screen to unmount mid-submission.

The success-path setState was not guarded by mounted. If the widget unmounted during submission, that setState threw, the exception was caught by the failure handler, and a successful send was reported as failed (sendReviewSubmitFailed) while navigation to the success screen was skipped.

This adds an early if (!mounted) return; before the success setState and removes the now-redundant if (mounted) wrapper around Navigator.push (no awaits between the guard and the push). The failure handler already guards its setState with mounted.

Test plan

  • Send a transfer normally → success screen still shows.
  • Trigger a submission failure → error message shows, no navigation.
  • Navigate away (back) while the confirm button is loading → no setState after dispose, no false failure logged.

The blocking submit (#524) widens the unmount window while awaiting the
RPC. If the screen unmounts mid-submission, the unguarded setState threw
and was caught by the failure handler, reporting a successful send as
failed and skipping navigation to the success screen.

@dewabisma dewabisma left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Actually, this is redundant checking the mounted. As you can see on line 62, we already checked it.

And after seeing it, it's kinda weird we set state on not mounted.

@n13

n13 commented Jun 15, 2026

Copy link
Copy Markdown
Collaborator Author

@dewabisma The line 62 mounted check doesn't cover the success-path setState, because mounted is only valid until the next await.

Between line 62 and the success setState there are real async gaps:

  • await settings.getActiveRegularAccount()
  • await submissionService.balanceTransfer(...) — the network RPC, plus retries

The widget can unmount during that balanceTransfer await (user hits back), so by the time we reach the success setState the line 62 result is stale. This is the standard use_build_context_synchronously rule: re-check mounted after every async gap before setState/context.

On the second point — if (!mounted) return; does the opposite of setting state while unmounted: it skips the setState when unmounted. Without it the unguarded setState throws after dispose, the catch swallows it, and a successful send gets reported as sendReviewSubmitFailed with no navigation. That false-failure-on-success is exactly the bug this PR fixes (the failure-path setState is already mounted-guarded for the same reason).

The removed if (mounted) around Navigator.push is fine to drop: there's no await between the guard and the push, so the single check covers both.

@n13

n13 commented Jun 15, 2026

Copy link
Copy Markdown
Collaborator Author

This is a small thing we can just fix it and move on

@n13 n13 requested a review from dewabisma June 15, 2026 06:04
@n13

n13 commented Jun 15, 2026

Copy link
Copy Markdown
Collaborator Author

AI explanation above ;)

@dewabisma dewabisma left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM!

@n13 n13 merged commit 3226709 into main Jun 15, 2026
1 check passed
@dewabisma dewabisma deleted the fix/review-send-mounted-guard branch June 22, 2026 03:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants