Crash solved while moving back and forth in connect message screen

[Jignesh-dimagi]

Technical Summary

https://dimagi.atlassian.net/browse/QA-8252

This crash was happening if user is navigating too fast between connect message and channel list screen.
Firebase log

There was a condition where binding getting null due to destroyed view and app's main thread was here updating the UI. This crash will not happen if network gets completed, updated UI and than fragment is destroyed.

QA Plan

QA should navigate between connect message and channel list screen and app should not crash

Labels and Review

• Do we need to enhance the manual QA test coverage ? If yes, the "QA Note" label is set correctly
• Does the PR introduce any major changes worth communicating ? If yes, the "Release Note" label is set and a "Release Note" is specified in PR description.
• Risk label is set correctly
• The set of people pinged as reviewers is appropriate for the level of risk of the change

[coderabbitai]

📝 Walkthrough

Walkthrough

A single null assignment statement was removed from the onDestroyView method in ConnectMessageChannelListFragment.java. Previously, the binding object was explicitly set to null when the view was destroyed; this cleanup line has been eliminated while all other control flow and cleanup logic remain unchanged.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

• Single file affected with one line removed
• No logic changes or control flow modifications
• Purely a cleanup/memory management adjustment

Suggested labels

QA Note

Suggested reviewers

• shubham1g5
• conroy-ricketts
• OrangeAndGreen

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main fix: resolving a crash that occurs when navigating rapidly between the connect message and channel list screens.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description check	✅ Passed	The PR description includes technical summary with ticket link, crash context, and QA plan, but is missing key sections from the template like Product Description, Feature Flag, Safety Assurance details, and complete Safety story.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch jignesh/fix/qa-8252

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[shubham1g5]

@Jignesh-dimagi can you add more context here around the crash itself (stacktrace or firebase link) and how is this an apt solution for that crash.

[Jignesh-dimagi]

@Jignesh-dimagi can you add more context here around the crash itself (stacktrace or firebase link) and how is this an apt solution for that crash.

Sure, I just updated the description for the same.

[shubham1g5]

this is a common pattern we follow across fragments and I don't see a point of code updating the UI after the fragment has been destroyed. As such maybe the better solution here is one similar to what we did here by introducing a non-null check on view binding. -

MessageManager.retrieveMessages(requireActivity(), success -> {
           if(binding!=null) refreshUi();
        });

An alternative can be to cancel the callback itself on onDestroy to fix the root of the issue itself.

[conroy-ricketts]

I think this is an avenue worth exploring 👍

[Jignesh-dimagi]

Yes correct to check for null but what if already execution inside refreshUi method and binding becomes null? Making binding null, we are not achieving anything but extra checks only. Your thoughts?

[conroy-ricketts]

I'm actually curious why we were setting the binding to null here in the first place too, especially if this is a common pattern in the codebase. Was this to prevent a memory leak?

[shubham1g5]

I'm actually curious why we were setting the binding to null here in the first place too, especially if this is a common pattern in the codebase. Was this to prevent a memory leak?

Yes, my sense is it's to do proper garbage collection as binding keep references to all fragment views and hence keeping it alive can mean stopping views from getting garbage collected which can be memory intensive.

Yes correct to check for null but what if already execution inside refreshUi method and binding becomes null

yeah that's a good point. I think callback cancellation should be the way to go here in that case.

[Jignesh-dimagi]

Yes, my sense is it's to do proper garbage collection as binding keep references to all fragment views and hence keeping it alive can mean stopping views from getting garbage collected which can be memory intensive.

Its good to make it null but actually view get destroyed first and than fragment so should not be a problem for garbage collection.

yeah that's a good point. I think callback cancellation should be the way to go here in that case.

Yeah its already planned here. This is bit of work to be done.

I think current solution should be good enough till we implement above ticket?

[shubham1g5]

actually view get destroyed first and than fragment so should not be a problem for garbage collection.

Ahh excellent observation! Think I am good with this fix in that case as long as we apply it uniformly across all fragments.

[Jignesh-dimagi]

Agree, done at all places