Pending bell icon is not shown when a new un-approved request is created

[Pavneet-Sing]

Description

Pending request bell icon not shown when coming back to Portfolio fragment after creating a new Pending transaction.

Steps to reproduce

1. Make sure there are no pending requests then create an unapproved/pending request.
2. Go back to Portfolio screen, note no bell button is shown.

Actual result

Should show bell icon when a new pending request is created.

Expected result

Should show the pending request bell button

Issue reproduces how often

Easy

Version/Channel Information:

• Can you reproduce this issue with the current Play Store version? NA (doesn't have it yet)
• Can you reproduce this issue with the current Play Store Beta version? NA (doesn't have it yet)
• Can you reproduce this issue with the current Play Store Nightly version? Yes

Device details

• Install type (ARM, x86): All
• Device type (Phone, Tablet, Phablet):All
• Android version:All

Brave version

1.38.40

Website problems only

• Does the issue resolve itself when disabling Brave Shields? NA
• Does the issue resolve itself when disabling Brave Rewards? NA
• Is the issue reproducible on the latest version of Chrome? NA

Additional information

Perhaps move the logic to check for pending requests in OnResume.

cc: @wchen342

[wchen342]

It is known, the underlying issue is /issues/21612.

[Pavneet-Sing]

I saw that so couple of doubts:

1. It can be done without the global observer, as mentioned, by moving the logic of pending check in on resume?
2. The liked issue has qa no and sounds more like a universal implementation than a bug?

We might not need to make it global unless we wanna show the button across the app. WDYT?

[wchen342]

by moving the logic of pending check in on resume

I personally think the update is more logically nature to put in the observer but yes, it is possible to put that in onResume. There needs to be a new overload for updatePortfolioGetPendingTx, otherwise you will end up with two getKeyringInfo and two getAllNetworks calls every resume.

We might not need to make it global unless we wanna show the button across the app

I think it's not just for the button. ApprovedTxObserver is implemented on a bunch of fragments so I supposed it is intended for more than just the notification.

[Pavneet-Sing]

I personally think the update is more logically nature to put in the observer but yes, it is possible to put that in onResume. There needs to be a new overload for updatePortfolioGetPendingTx, otherwise you will end up with two getKeyringInfo and two getAllNetworks calls every resume.

Wait if updatePortfolioGetPendingTx is already being called then it should be able to check if there's a pending request and show the bell icon? probably retain the required data getAllNetworks etc and reuse.

I think it's not just for the button. ApprovedTxObserver is implemented on a bunch of fragments so I supposed it is intended for more than just the notification.

A global ApprovedTxObserver could be useful for simultaneous actions but not sure how long it will take to implement this? though would love to have observer patterns for data changes and is something I have been thinking to do so, probably we can discuss the approach to refactor service pattern to reactive pattern with team because it should be life cycle aware to subscribe in onResume and unsubscribe in onPause or something to avoid updating the UI in the background.

We need QA to validate this issue so feel free to link both issues in one PR or probably wanna add more details in 21612 if issues like these are known, to avoid duplicate issues.

[wchen342]

if updatePortfolioGetPendingTx is already being called

Actually it's not, updateNetwork will return early because chainId will be the same, but the mojo calls are still there.

probably retain the required data getAllNetworks etc and reuse

Yes, that's what I meant by "a new overload for updatePortfolioGetPendingTx".

it should be life cycle aware to subscribe in onResume and unsubscribe in onPause or something to avoid updating the UI in the background

That's an interesting point, I am not sure what problem it will cause if we update UI in background, or is it just anti-pattern?

link both issues in one PR

Yes, if I get to this I will do just that so QA can verify this issue.

[Pavneet-Sing]

That's an interesting point, I am not sure what problem it will cause if we update UI in background, or is it just anti-pattern?

Indeed, for example, a fragment can receive the callbacks from ApprovedTxObserver in the background when the view has been destroyed so any attempt to update the view will result in crash or will require additional null and state checks. There can be n number of listeners (fragments, activities or objects) trying to update the UI in background, causing unexpected issues and rendering.

[srirambv]

Verification passed on the following devices running 1.39.96 x64 Beta build

• Verified steps from https://github.com/brave/brave-core/pull/12712
• Verified bell icon is shown when there is an unapproved request pending
• Verified clicking on the bell icon loads the transaction approval prompt
• Verified approve/reject causes the bell icon to be removed from the portfolio page

Oppo Reno 5 (Android 12)	Samsung Tab A (Android 10)
21654-ARM.mp4	21654-Tab.mp4