-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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.
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
Clean up lastReadSequenceNumbers
and reportMaxSequenceNumbers
#9755
Conversation
Coming from this comment thread... I wanted to start a big conversation about this stuff, but as I started to write it all down it became clear that it might be easier to just suss out a few ideas in a PR review first... My ultimate goal is to get rid of this block of code here: App/src/libs/actions/Report.js Lines 1407 to 1420 in eb54f23
But getting rid of At a high level it looks like we do this:
Getting rid of
I can't really think of any reason why that would not work, but feel like I'm missing something. Regardless, I think some changes like I've got in this PR would be a good "stepping" stone since the |
Hey Marc, I like the direction this is doing in. It already feels cleaner and I love the removal of I know I'm not as close to this code as you are, so I don't have a full understanding over all the logic that exists. Hopefully, I can offer somewhat of an outsider perspective with my next suggestion.
This makes sense to me 👍
My out-of-the-box idea here is that maybe instead of pushing out the unread count for each participant, we push out an event that is basically "increment the unread count for report ID: X". That way there doesn't really need to be any math or calculations done. We just know that there is a new comment for the report, and until the comment is explicitly seen, we know it should just increment the participants counter by 1. That works if everyone is online and connected to pusher. If a participant is not connected to pusher, the next time they connect, the front-end (or maybe the server) should still be able to calculate the unread count properly. Thoughts about that approach? |
Same as Tim I agree with A. For B I like the event idea, but we'd also have to have an event for deleted chats no? Depending on whether you're deleting a chat that has been read already or not. Another idea is: The unread count should always be Basically we're optimistically predicting the changes in the sequenceNumber from the server response. |
Or even simpler, this might be a naive question, but why can't we just count the number of actions after the lastReadSequenceNumber that aren't deleted? |
Oh wow, nice, I really love both of these ideas! @tgolen I like this because we can essentially stop tracking
Very cool, so something like:
We could let the server send out these "increment" or "decrement" instructions that way we don't have to have specific events to track "new comment" vs "deleted comment" and could just do stuff like: [
'onyxMethod' => 'merge',
'key' => 'report_1',
'value' => [
'unreadActionCount' => '++',
],
], Not sure if we want something like that or something simpler like just having an event like [
'action' => 'increment'
'reportID' => 1,
], @madmax330 your idea sounds nice since we won't need specific events to tell the client to increment or decrement. But it seems like in practice we could have something like this... 0 - deleted and the last read would be The only potential issue I see with this is that when modifying the
The problem with counting the report actions themselves is that they might not exist at all locally. |
Can you elaborate on this? Why wouldn't they? |
Because they are paginated and we only download a subset of the chats you have when the app starts not all of your chats and chat messages. They can also get "evicted" if we run out of device storage but that is rarer now (especially on web) |
This is only an issue if you have more unreads than the page size though right?
Yeah I'm not sure how to deal with this. |
Yes, to use the reportActions alone we'd need to have all the chat messages for all the chats from the "last read" to the "most recent".
|
Just a quick thought on this... it actually sounds like that could be an improvement to our pagination. Instead of always getting the 50 most recent chats for the first page, the first page could be either the 50 most recent OR all the chats since "last read". I love this! ❤️ Slight tweak below in bold is needed (I think) Very cool, so something like: Fetch initial report -> user starts with base count from server
I'd vote for something like that ^, yeah. I think the other option just makes it a little too magical and I fear it could wreak havoc in unexpected ways at some point without knowing it. |
Oh yes, actually I think if an action is deleted by any user we would still need to look at the action deleted has a |
We could possibly send out paginated report actions with every new comment that gets added... e.g.
|
Oh, that's pretty interesting. Sounds a little more complex, but it sounds
like it could definitely work.
…On Thu, Jul 7, 2022 at 1:05 PM Marc Glasser ***@***.***> wrote:
Just a quick thought on this... it actually sounds like that could be an
improvement to our pagination. Instead of always getting the 50 most recent
chats for the first page, the first page could be either the 50 most recent
OR all the chats since "last read".
We could possibly send out paginated report actions with every new comment
that gets added...
e.g.
- User A has no chat messages locally stored for a chat with User B
(the chat itself has 200 historical messages)
- The last message User A read was sequenceNumber 50
- User B sends a new message with sequenceNumber 201
- In the server, we look at User A's last read recorded and send every
message from 50 - 201 to User A
- User A can now figure out that their unread count based on actions
alone
—
Reply to this email directly, view it on GitHub
<#9755 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJMAB6BRANB6O2ZVUGOYJDVS4S7TANCNFSM5233LPCA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Ah came across an edge case where we need to calculate the unreads in the client based on the deleted report actions 😄 When a user manually marks a comment as “unread” we will have to use the report actions to "optimistically determine" the unread count since we don’t know how many deleted comments are between the So I think that leaves us with:
Not sure how to get around that last one... cc @PauloGasparSv on this conversation too since I think they will run into this while refactoring the Edit / Delete comment stuff. |
Kill reportMaxSequenceNumbers Clean up some usages getUnreadActionCOunt remove getLastReadSequenceNumber Get rid of .then() calls in Report.js Remove ambiguous `updateReportActionMessage` method Remove last .then() in Report.js Fix lastMessageText bug Dont decrement count when handling deleted comment events from current user undo random/bad change Add Math.max() Update names in ReportActionsView Clear new marker when tapping floating message counter
ed52691
to
e9e72f9
Compare
lastReadSequenceNumbers
and reportMaxSequenceNumbers
lastReadSequenceNumbers
and reportMaxSequenceNumbers
I still need to test this on other platforms besides web, but pretty happy with where this changes landed and everything is feeling more consistent and testing well. Going to open this up for some reviews now. |
Yeah, this feels much better and it seems like it would be much more
testable.
Some of the stuff in that table is maybe a little bit off. For example, "New
action appears from a different user (report not visible)" mentions the
sequence number of "deleted" action. I think that's not quite right?
For that last use case (marking a comment as unread), it sounds very much
to me like exact same case as the first one (fetch initial report). Does
that help?
…On Thu, Jul 7, 2022 at 5:48 PM Marc Glasser ***@***.***> wrote:
I still need to test this on other platforms besides web, but pretty happy
with where this changes landed and everything is feeling more consistent
and testing well. Going to open this up for some reviews now.
—
Reply to this email directly, view it on GitHub
<#9755 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJMAB5TTASAXQBLXWTYM73VS5UDTANCNFSM5233LPCA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Nice catch, that should be "new" not "deleted". Updated. Thanks!
Yeah, they are pretty much the same except that when we are fetching the initial report the server is the one with the knowledge of any "deleted actions" and when we are marking a comment as "unread" it's the client that knows about them. caveat: Does seem like some of this will be changing in the future as there was a discussion here about possibly just tracking whether a report is read or unread and not the exact number of "unread" messages it has. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a few questions and comments, but other than that it looks really good! Thanks for cleaning this up a bit before the refactor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh cool, moment().unix()
does the job.
Hey @neil-marcellini thanks for the review! I ended up adding a test for this in a new branch and noticed some things that were off with my code so going to do one more update here (sorry). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a few questions that will help me understand everything that's happening here.
They're pretty basic but necessary none the less.
Ready for another review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests are passing and the changes look good.
Thanks @neil-marcellini! Gonna go for the merge to avoid conflicts 🙃. Will continue to look into improvements to the |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by @marcaaron in version: 1.1.83-1 🚀
|
🚀 Deployed to production by @chiragsalian in version: 1.1.84-13 🚀
|
Details
Quick draft to explore getting rid of these two "maps" of local variables that we have.
They are making it kind of hard to reason about our "optimistic" updates that need to be made and ideally we would be able to control these things from the server. So a first step is to keep referencing them on the report object instead of in these maps.
Also tried to clean up the logic around setting
unreadActionCount
as I noticed that thesetLocalLastRead()
method was being used sometimes to update thelastReadSequenceNumbers
directly (which just seems wrong) but it also performed a confusing side effect of updating theunreadActionCount
which also seems wrong and made it difficult to tell what things are updating theunreadActionCount
at all.Fixed Issues
(Related to both of these since the setting of unreads is pretty wonky atm)
https://github.com/Expensify/Expensify/issues/212871
https://github.com/Expensify/Expensify/issues/211601
Tests & QA
Test unread action shows
2022-07-07_14-49-29.mp4
Note: Also verify a browser notification shows on Desktop and Web (not mobile web)
Test unread action does not show when report is active
2022-07-07_14-53-37.mp4
Manually marking chat works as expected
2022-07-07_14-56-28.mp4
Deleted comments behave as expected when calculating unreads and lastMessageText
2022-07-07_15-09-52.mp4
Test tapping green message counter will reset new marker line and mark report as read
2022-07-07_15-14-16.mp4
PR Review Checklist
Contributor (PR Author) Checklist
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
filesSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
displayName
propertythis
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)PR Reviewer Checklist
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
).src/languages/*
filesSTYLE.md
) were followed/** comment above it */
displayName
propertythis
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Note: Tested on all platforms, but not posting videos since there are no UI changes.