Fix: Incorrect unread actions counts by kidroca · Pull Request #5302 · Expensify/App

kidroca · 2021-09-16T18:38:38Z

Details

Include the existing maxSequenceNumber in the logic as we might already have a higher value

Fixed Issues

Tests

Note: the bug might have already caused to have incorrect unread notifications count stored for your account on the backed.
To ensure you're starting from a clean slate:

Log out
Sign in
If there are any chats with unread notifications - open them to clear unread messages (don't scroll up, just open the convo)
Finally log out and continue with the rest of the steps

For all platforms

Be logged out
Sign in
Open a chat with more than 50 messages
Scroll up to trigger loading older messages
Switch to a different chat
Switch back to the chat from 3)
Switch to another chat again
Refresh/Reload the app
- Cmd + R for web/mWeb/desktop
- Terminate and re-launch the app on iOS/Android
Following these steps should not create any "unread messages" proving the fix is working
- use the same steps on staging to see them recreate the bug

QA Steps

Follow the same tests steps as above. You can also try the rest of the discovered ways to recreate this issue - here #4273 (comment) and here #4273 (comment). These steps should no longer produce unread messages

Tested On

Screenshots

Web

Screen.Recording.2021-09-17.at.19.40.03.mov

Web: before: #4273 (comment)

Mobile Web

Screen.Recording.2021-09-17.at.19.36.38.mov

Desktop

Screen.Recording.2021-09-17.at.18.52.48.mov

Desktop before: #4273 (comment)

iOS

Screen.Recording.2021-09-17.at.19.24.15.mov

iOS before: #4273 (comment)

Android

Android.Emulator.-.Pixel_2_API_29_5554.2021-09-16.21-35-06.mp4

Android before: #4273 (comment)

Include the existing maxSequenceNumber in the logic as we might already have a higher max sequence number

marcaaron · 2021-09-17T17:31:08Z

+            const currentMaxSequenceNumber = reportMaxSequenceNumbers[reportID] || 0;
+            if (maxSequenceNumber > currentMaxSequenceNumber) {
+                Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, {maxSequenceNumber});
+            }


So, this change makes sense to me on the one hand because I'd expect things to get very screwed up when we call Report_GetHistory with paginated results. Since what comes back can be historical it possibly won't be the "max sequence number we have" so it makes sense to compare it with some previously fetched result. Great job identifying that.

On the other hand, it really feels like if Report_GetHistory can return some set of items that may or may not contain the "maxSequenceNumber" then maybe we should not be relying on Report_GetHistory at all to derive the "maxSequenceNumber".

It feels like it could be possible (although I haven't verified this) that we might:

have no local reportMaxSequenceNumbers[reportID] for a given report

then we get some older paginated history from this API

then we would set the max to the highest returned (which is possibly not the "true max")

This still feels wrong to me and I wanted to propose that maybe instead of setting the max when fetching history we should always use the reportSummaryList to do it. Hard to see from this without inspecting network requests, but we have a reportActionCount that is returned when we get the reportSummaryList here:

App/src/libs/actions/Report.js

Line 188 in 8d7b62f

maxSequenceNumber: lodashGet(report, 'reportActionCount', 0),

That said, I don't know if it's a perfect solution since we'd have to add a network request next to some places where we are fetching the report history. Gonna tag in @tgolen for his opinion on this.

Great observations @marcaaron
From my experience paging API's would usually respond with the total amount of items anytime you fetch a page
If that's added to the response we can just use it here

It feels like it could be possible (although I haven't verified this) that we might:

have no local reportMaxSequenceNumbers[reportID] for a given report

then we get some older paginated history from this API

I thought of that as well and did an investigation - it doesn't look likely, at least I couldn't find a way. fetchActions is called (with offset) only through ReportActionView paging and here:

App/src/libs/actions/Report.js

Lines 994 to 997 in 3b9a54f

_.each(reportIDsToFetchActions, (reportID) => {

const offset = dangerouslyGetReportActionsMaxSequenceNumber(reportID, false);

fetchActions(reportID, offset);

});

Since the above code runs in a timer I think we'll have a reportActionCount as you've pointed out is fetched with the reportSummaryList

The other time would be when a new report comes from pusher, but this also triggers a call that should fetch the correct data

I wanted to propose that maybe instead of setting the max when fetching history we should always use the reportSummaryList to do it

As I was reading through the comments, this is exactly the solution that came to my mind.

we'd have to add a network request next to some places where we are fetching the report history

This is the part that I'm not too sure about. Is this really necessary? I am just trying to think of any place where we:

Haven't called reportSummaryList

Are fetching the report history

Need the max sequence number

From my experience paging API's would usually respond with the total amount of items anytime you fetch a page

I also agree with Kidroca here, that it's very typical that pagination APIs will return the total number of records along with the paginated results, so I'd be fine with adding it here too.

Seems we're calling fetchChatReportsByIDs which in the end updates reportActionCount for these cases:

during init/refresh (for all reports)

for new report we learn about through pusher (receiving an action from a report we don't have)

when we're back online (for all reports)

So it's totally possible we don't need to update max count when we fetch actions, but I haven't tested.
I think we're missing the case where we resume a suspended app e.g.

AppState.addEventListener('change', this.onVisibilityChange);

There's no code that will fetch all reports when we bring focus back to the app, though it might not be necessary. ~~There's one listener in ReportActionsView that will refetch the current report~~ (wrong)
AFAIK the pusher connection is dropped when the app is suspended for more than 5-10 minutes (ios), so we might have a problem there

Is this possible: we've lost connection to pusher, so we miss some new messages, when we open a chat we always fetch the latest (no offset) so we'll fetch the new messages through http, but they will not update the maxSequenceNumber if we remove the logic

This is the part that I'm not too sure about. Is this really necessary?

We might be able to get rid of it entirely. Tbh, I am missing some context for why we started setting the maxSequenceNumber when fetching the history in the first place. I think it was added in the POC stages here.

Maybe we thought fetching the actions was a good place to poll for the max sequence number and update the unreads? And since we could guarantee that it would always include the highest was safe/convenient to do this here?

So, it sounds like our current options are:

Hold this PR on a backend change to return a reportActionCount with Report_GetHistory

Investigate whether we can stop updating the maxSequenceNumber here entirely and no backend change would be necessary

Merge this current change temporarily then follow up and do 2 and possibly 1 if we decide that we do want to set the maxSequenceNumber here.

I'm 👍 on doing 3 as long as we do a follow up after.

IMO we should continue updating the maxSequenceNumber in fetchActions - in case it fetches (new) actions we don't have locally. I think it can still happen sometime.

We're using fetchActions as a "message source", we're using Pusher as a "message source", when we receive new messages through any of these sources we should update the maxSequenceNumber. Pusher handles it in a different way since it would only receive new messages

Setting `maxSequenceNumber` during `fetchActions` is no longer necessary

kidroca · 2021-09-27T12:07:33Z

So, it sounds like our current options are:

Hold this PR on a backend change to return a reportActionCount with Report_GetHistory

Investigate whether we can stop updating the maxSequenceNumber here entirely and no backend change would be necessary

Merge this current change temporarily then follow up and do 2 and possibly 1 if we decide that we do want to set the maxSequenceNumber here.

I'm 👍 on doing 3 as long as we do a follow up after.

I did an investigation regarding 2)

We're calling fetchActions in 3 places

after fetchAllReports
paging - scrolling to past messages
when we open a report - ReportActinosView.componentDidMount

Overall it looks safe to drop setting of maxSequenceNumber in fetchActions

fetchAllReports gets the reports summary so we already have the latest count saved before fetchActions is called
paging - we don't need to update maxSequenceNumber while scrolling to older messages
when we open a report - this is the only time I'm not certain could there be some discrepancy. I lean towards "not a problem" - it doesn't seem to be a problem through normal usage. I had concerns about being offline but fetchAllReports is called when we resume connectivity so that should fix the count. My only remaining concern is losing connection to pusher - clicking on a report might load newer messages but we won't update maxSequenceNumber, not sure if this is possible though.

I've pushed an update that removes maxSequenceNumber from fetchActions, tested more and didn't experience any issues

tgolen

Thanks for the investigation! I agree with your conclusions and hopefully this is a step forward!

OSBotify · 2021-09-27T16:41:51Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2021-09-28T21:45:50Z

🚀 Deployed to staging by @marcaaron in version: 1.1.2-9 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2021-09-30T15:43:03Z

🚀 Deployed to production by @timszot in version: 1.1.3-1 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

fix: maxSequenceNumber is getting overwritten with lower values

f0a9776

Include the existing maxSequenceNumber in the logic as we might already have a higher max sequence number

kidroca mentioned this pull request Sep 16, 2021

[HOLD for payment 2021-10-07] Notification badge showed 100 unread messages when user only had 1 unread chat #4273

Closed

5 tasks

kidroca marked this pull request as ready for review September 17, 2021 16:41

kidroca requested a review from a team as a code owner September 17, 2021 16:41

MelvinBot requested review from marcaaron and removed request for a team September 17, 2021 16:41

marcaaron reviewed Sep 17, 2021

View reviewed changes

fix: maxSequenceNumber is getting overwritten with lower values

bc17f85

Setting `maxSequenceNumber` during `fetchActions` is no longer necessary

kidroca requested review from marcaaron and tgolen September 27, 2021 12:07

tgolen approved these changes Sep 27, 2021

View reviewed changes

marcaaron approved these changes Sep 27, 2021

View reviewed changes

marcaaron merged commit 1fdfbd7 into Expensify:main Sep 27, 2021

kidroca deleted the kidroca/maxSequenceNumber-fix branch September 27, 2021 17:18

kidroca mentioned this pull request Sep 27, 2021

Chat- Incorrect unread messages shown in green badge after "marked as unread" for any message #5106

Closed

OSBotify mentioned this pull request Sep 28, 2021

Deploy Checklist: New Expensify 2021-09-28 #5568

Closed

16 tasks

	_.each(reportIDsToFetchActions, (reportID) => {
	const offset = dangerouslyGetReportActionsMaxSequenceNumber(reportID, false);
	fetchActions(reportID, offset);
	});

Conversation

kidroca commented Sep 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

For all platforms

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Uh oh!

marcaaron Sep 17, 2021

Choose a reason for hiding this comment

Uh oh!

kidroca Sep 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tgolen Sep 17, 2021

Choose a reason for hiding this comment

Uh oh!

kidroca Sep 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kidroca Sep 17, 2021

Choose a reason for hiding this comment

Uh oh!

marcaaron Sep 18, 2021

Choose a reason for hiding this comment

Uh oh!

kidroca Sep 20, 2021

Choose a reason for hiding this comment

Uh oh!

kidroca Sep 20, 2021

Choose a reason for hiding this comment

Uh oh!

kidroca commented Sep 27, 2021

Uh oh!

tgolen left a comment

Choose a reason for hiding this comment

Uh oh!

OSBotify commented Sep 27, 2021

Uh oh!

OSBotify commented Sep 28, 2021

Uh oh!

OSBotify commented Sep 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kidroca commented Sep 16, 2021 •

edited

Loading

kidroca Sep 17, 2021 •

edited

Loading

kidroca Sep 17, 2021 •

edited

Loading