Skip to content
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

Only update lastMessageText from pusher event from other users #1781

Merged
merged 2 commits into from
Mar 16, 2021

Conversation

tgolen
Copy link
Contributor

@tgolen tgolen commented Mar 15, 2021

cc @marcaaron

Details

This will do a better job of keeping the last comment text in the LHN in sync.

  • It adds updates the text when the optimistic comment is first added to Onyx
  • It only updates the text when the pusher event comes from the non-active chat participant

Fixed Issues

Fixes https://github.com/Expensify/Expensify/issues/157488

Tests

You need to have two chat clients running with two different accounts (use incognito or a different platform with a different user logged in)

  1. Open a chat with both users
  2. Make comments AS FAST AS YOU CAN by one user
  3. Ensure the last message text is always up to date in the LHN
  4. Make comments AS FAST AS YOU CAN by the other user
  5. Ensure the last message text is always up to date in the LHN

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

image

Mobile Web

image
image

Desktop

image

iOS

image
image

Android

image
image

@tgolen tgolen requested a review from a team March 15, 2021 21:26
@tgolen tgolen self-assigned this Mar 15, 2021
@botify botify requested review from francoisl and removed request for a team March 15, 2021 21:26
@francoisl
Copy link
Contributor

Looks like tests didn't even start when GH Actions was down? I can't seem to manually retrigger them either, maybe you'll need to make a blank commit?

Copy link
Contributor

@marcaaron marcaaron left a comment

Choose a reason for hiding this comment

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

LGTM only have a couple of small comments


// If the report action from pusher is a higher sequence number than we know about (meaning it has come from
// a chat participant in another application), then the last message text and author needs to be updated as well
if (newMaxSequenceNumber > (lastReadSequenceNumbers[reportID] || 0)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

NAB, we could maybe do a

const lastReadSequenceNumber = lastReadSequenceNumbers[reportID] || 0;

above the const updatedAction = { line and use it here and when calculating the unreadActionCount

@@ -257,14 +257,21 @@ function updateReportWithNewAction(reportID, reportAction) {
// Always merge the reportID into Onyx
// If the report doesn't exist in Onyx yet, then all the rest of the data will be filled out
// by handleReportChanged
Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, {
const updatedAction = {
Copy link
Contributor

Choose a reason for hiding this comment

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

updatedReportObject ?

@marcaaron
Copy link
Contributor

Works great! 🎉

@tgolen tgolen requested a review from a team as a code owner March 16, 2021 15:12
@botify botify requested review from johnmlee101 and removed request for a team March 16, 2021 15:12
@tgolen
Copy link
Contributor Author

tgolen commented Mar 16, 2021

Updated! Tests should run now, hopefully.

Copy link
Contributor

@francoisl francoisl left a comment

Choose a reason for hiding this comment

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

Works well

@marcaaron marcaaron self-requested a review March 16, 2021 19:11
Copy link
Contributor

@marcaaron marcaaron left a comment

Choose a reason for hiding this comment

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

🎉

@marcaaron marcaaron merged commit ac813fa into master Mar 16, 2021
@marcaaron marcaaron deleted the tgolen-fix-lhn-sync branch March 16, 2021 19:11
@github-actions github-actions bot locked and limited conversation to collaborators Mar 16, 2021
@OSBotify
Copy link
Contributor

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants