Rebuild unread reports without sequence numbers #13419

puneetlath · 2022-12-07T22:47:05Z

On HOLD until https://github.com/Expensify/Web-Expensify/pull/35907 is live.

Details

This PR rebuilds the unread functionality to use reporAction IDs and timestamps instead of sequence numbers. At a high-level:

Every report action has a unique reportActionID (non-sequential)
Every report action has a created field with millisecond precision
Every report has a lastReadTime indicating the last time that report was read
Every report was a lastActionCreated indicating the reportAction.created of the last visible report action on the report
A report is unread if lastReadTimes < lastActionCreated
To mark a given action as unread, we set the lastReadTime to 1 millisecond before the reportAction.created of that action
We display the "new" message indicator above the oldest action whose reportAction.created is after the lastReadTimestamp

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/240916
$ #13472
$ #13684

Tests

Create two accounts, A and B, that have never chatted before.
Send a DM from B to A. The chat should be:
- Unread for A
- Read for B
User A opens the chat. The chat should become Read
User A sends a message in the chat. It should remain Read
When user A has the chat fully visible and scrolled to the bottom, user B sends a message to user A. The chat should remain Read for both users.
User A sends a bunch of messages, such that they can scroll back in the chat and the bottom messages aren't visible. User A then scrolls back up in the chat.
User B sends a message to user A. The chat should become unread for user A.
User B sends two more messages to user. The chat should remain unread.
User A scrolls back down to the bottom of the chat. It should stay unread for now, and a new message indicator should appear above the first message that user B sent while user A was scrolled up.
(narrow screens) User A goes to the LHN, opens a different chat, then goes back to the LHN. The chat with user B should now show as read.
(narrow screens) User A returns to the DM with user B and starts uploading a large attachment.
(narrow screens) Before the upload finishes, User A goes back to the LHN.
(narrow screens) Wait for a bit until the upload completes. Verify that the chat remains read and does not become unread.
- Note: The past three steps cover this issue
User A navigates to a different chat.
User B sends a message to User A. The chat should become unread for User A.
User A opens the chat from user B, then navigates to a different chat, then comes back. It should now be read.
User A scrolls up, marks a given message as Unread. The chat should become unread and an unread indicator should appear above the message that was marked as unread.
User A leaves the chat, opens a different one, then comes back. The unread marker should be where it was in the previous step, and the chat should become read.
User A opens a different chat, then comes back. The unread marker should be gone.
User A scrolls back, then marks one of their own messages in the middle of the chat as unread. The chat should become unread and a new message indicator should appear above that message.
User A deletes the message they marked as unread.
User A opens a different chat, then comes back. The new message indicator should be above the message after the deleted message, and the chat should become read.
User A opens a different chat, then comes back. The new message indicator should be gone.
User A sends a message in the chat, then marks it (i.e: the newest message in the chat) as unread. The chat should become unread and the new message indicator should appear above that message.
User A then deletes the message they marked as unread. The chat should become read and the new message indicator should disappear.
User A sends a message in the chat, then marks it (i.e: the newest message in the chat) as unread. The chat should become unread (bolded) and the new message indicator should appear above that message.
User A navigates to a different chat, closes the app completely (by closing the tab (web), pressing the home button (mobile), or exiting electron (desktop)) and reopens it. The chat/message from the previous step should still be unread.
User A opens the chat, opens a different chat, then comes back, and the message should be read.
User A closes the app completely and reopens it. The chat should remain read.

Verify that no errors appear in the JS console

Offline tests

User A goes offline.
User B sends a DM to user A. User A is offline so their chat should remain unchanged.
User A sends an (offline) DM to User B. User A should see the chat as read.
User A comes back online. Since their message was sent most recently, the chat should appear as read.
- Potential improvement: As described here, we could change the unread feature such that the chat appears unread for User A in this scenario, but that's out-of-scope for now.

QA Steps

Run through any existing regression tests that deal with unread chat functionality.

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

Web.mov

Mobile Web - Chrome

android.chrome.mov

Mobile Web - Safari

ios.safari.mov

Desktop

desktop.mov

iOS

ios.mov

Android

android.mov

melvin-bot · 2022-12-14T22:43:05Z

@rushatgabhane @deetergp One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

src/pages/home/report/ReportActionsView.js

puneetlath · 2022-12-14T22:45:19Z

@rushatgabhane FYI you won't be able to test this yet as it requires some back-end changes that you don't yet have access to.

src/libs/actions/Report.js

marcaaron · 2023-01-02T22:13:50Z

I think we should re-write these test steps so that there is less ambiguity about what we're looking for. I generally refer to any indication that a report is unread as an "unread indicator" but I think maybe you are using it to refer to the "new marker line" e.g. in step 22

User A leaves the report, opens a different one, then comes back. The unread marker should be where it was in the previous step, and the report should become read.

If I understand this step correctly it is not the expected behavior. If you navigate away from a report that had the "new marker line" set then it should be cleared when you navigate away from the report. It shouldn't matter if you open a different report or not.

marcaaron · 2023-01-02T22:17:12Z

Step 25 - Again, we should move failing tests out of the tests or test the expected behavior (even if we expect it to fail). I'd probably say we should avoid testing behaviors we know will fail because sometimes QA uses these tests in the future to assume a correct behavior.

marcaaron · 2023-01-02T22:22:12Z

I think step 26 is also hard to test because it's dependent on 25.

User A opens a different report, then comes back. The new message indicator should be where is was before, and the chat should become read.

What state should we be in for this to work? I think it's right after we mark a message as unread? But in the tests it is after we delete a report action.

marcaaron · 2023-01-02T22:31:55Z

Step 31 is a little ambiguous too

User A closes the app completely and reopens it. The message from the previous step should still be unread.

Close the app how? Can we be more specific and if there are platform specific things we are doing?

marcaaron

Code LGTM! Ran into some trouble following the test steps. I get the sense that whatever is broken about the unread indicators at this point is consistent with staging or some possible recent regressions. I haven't looked into the source of those regressions, but certainly doesn't seem like this PR is going to make things any worse.

puneetlath · 2023-01-02T23:26:19Z

Thanks @marcaaron! The tests have been updated. The steps that are in the PR now should bring us up to parity with main and should all be passing.

marcaaron · 2023-01-02T23:52:11Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

marcaaron · 2023-01-02T23:59:47Z

User A opens the report, navigates to a different one, then comes back. It should now be read.

should say

User A opens the report, navigates to LHN?, selects a different report, navigates back to the LHN. Verify the chat in the LHN is read

?

marcaaron · 2023-01-03T00:28:13Z

Found this while testing on iOS (maybe it is a known issue)

Mark chat message as unread
Navigate to LHN
Close app
Reopen app
Unread status of report flashes from "read" to "unread" then eventually "read" 🐛
Navigate to chat
No "new marker" present 🐛

2023-01-02_14-24-35.mp4

marcaaron · 2023-01-03T00:45:07Z

Sorry this one is pretty wonky, but on mobile web (Safari) I performed these steps:

Visit a report where there was just one new comment
New marker shows in the correct spot ✅
Scroll up in the chat
Pull down to refresh
LHN will show (report is now "read") ✅
Tap back into chat and scroll up
Chat messages disappear briefly 🤔
When they return the new marker is in the wrong spot 💥

2023-01-02_14-40-41.mp4

Didn't see this on any other platforms so far...

marcaaron · 2023-01-03T00:49:28Z

Minor issue while testing mobile Safari (Edit: also happens on Android)

Mark last comment as read
Close Safari
Reopen Safari
New marker is not shown but appears a short time later 🐛

2023-01-02_14-47-54.mp4

marcaaron · 2023-01-03T00:59:55Z

I don't think it was introduced here but for transparency there is a propType warning popping up when you long press on a comment (found while testing Android)

marcaaron · 2023-01-03T01:20:10Z

Great work @puneetlath!

Completed testing on various platforms. Turned up a few issues (minor I think). I'm also not certain if those are on staging or not (sorry testing took several hours). My personal opinion on this is that the things turned up are not urgent. There are a few open issues related to the unread indicators floating around and I don't think having this PR open any longer will help us solve those issues. So seeing as nothing major is broken I am 👍

That said, it does seem like something has recently regressed related to setting the "new marker" and unread status in the LHN. This stuff was working (definitely not perfectly, but I think better than it is working now) and we should commit to exploring improvements in the ReportScreen 2.0 conversation started by @roryabraham here.

puneetlath · 2023-01-03T18:01:51Z

Thanks for the super thorough testing!

Found this while testing on iOS (maybe it is a known issue)

Mark chat message as unread
Navigate to LHN
Close app
Reopen app
Unread status of report flashes from "read" to "unread" then eventually "read" 🐛
Navigate to chat
No "new marker" present 🐛

This one is known and exists on main too. This is the same issue as this: #13799

Sorry this one is pretty wonky, but on mobile web (Safari) I performed these steps:

Visit a report where there was just one new comment
New marker shows in the correct spot ✅
Scroll up in the chat
Pull down to refresh
LHN will show (report is now "read") ✅
Tap back into chat and scroll up
Chat messages disappear briefly 🤔
When they return the new marker is in the wrong spot 💥

I wasn't able to reproduce this one on main. I'll open an issue for it. It feels sufficiently niche to not block over.

Minor issue while testing mobile Safari (Edit: also happens on Android)

Mark last comment as read
Close Safari
Reopen Safari
New marker is not shown but appears a short time later 🐛

This one also exists on main. I'll open an issue.

I don't think it was introduced here but for transparency there is a propType warning popping up when you long press on a comment (found while testing Android)

Confirmed this is also happening on main. I'll open an issue. (link)

chiragsalian · 2023-01-03T23:27:43Z

Discussed on slack, i'm proceeding to merge this.

stale review and it was tackled

OSBotify · 2023-01-03T23:34:22Z

✋ 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 · 2023-01-04T03:35:32Z

🚀 Deployed to staging by @chiragsalian in version: 1.2.48-0 🚀

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

OSBotify · 2023-01-05T16:35:41Z

🚀 Deployed to production by @luacmartins in version: 1.2.48-2 🚀

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

OSBotify · 2023-01-06T19:28:40Z

🚀 Deployed to production by @luacmartins in version: 1.2.49-0 🚀

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

s77rt · 2024-05-16T01:20:23Z

src/libs/ReportUtils.js

+    // lastActionCreated and lastReadTime are both datetime strings and can be compared directly
+    const lastActionCreated = report.lastActionCreated || '';
+    const lastReadTime = report.lastReadTime || '';
+    return lastReadTime < lastActionCreated;


Edge case: If a chat report is empty (has no report actions except the CREATED action) then lastReadTime wouldn't be set and we fallback to an empty string and '' < lastActionCreated will be true making an empty chat unread (#38368).

refactor isUnread to use lastMessageTimestamp and lastReadTimestamp

73854ba

puneetlath self-assigned this Dec 7, 2022

puneetlath added 9 commits December 8, 2022 15:50

Refactor markCommentAsUnread to use reportActionTimestamp

25771ea

Merge main into branch

ed866d3

Refactor markCommentAsUnread to use reportActionTimestamp

9b5e726

Make sure lastReadTimestamp is in UTC

b85a5f7

Refactor viewNewReportAction to use timestamp instead of sequence number

377e9f0

Refactor unread marker to use timestamps instead of sequence numbers

cec458a

use reportActionTimestamp instead of timestamp

8ead297

Merge main into branch

d2d619d

hanlde cases where reportActionID or reportActionTimestamp arent set

34907b7

puneetlath mentioned this pull request Dec 14, 2022

[HOLD] Rebuilding the unread reports without the sequenceNumbers #12948

Closed

98 tasks

puneetlath changed the title ~~Rebuild unread reports without sequence numbers~~ [WIP] Rebuild unread reports without sequence numbers Dec 14, 2022

puneetlath marked this pull request as ready for review December 14, 2022 22:42

puneetlath requested a review from a team as a code owner December 14, 2022 22:42

melvin-bot bot requested review from deetergp and rushatgabhane and removed request for a team December 14, 2022 22:43

puneetlath commented Dec 14, 2022

View reviewed changes

src/pages/home/report/ReportActionsView.js Outdated Show resolved Hide resolved

puneetlath requested review from roryabraham and removed request for rushatgabhane December 15, 2022 14:46

puneetlath commented Dec 15, 2022

View reviewed changes

src/libs/actions/Report.js Outdated Show resolved Hide resolved

puneetlath added 3 commits December 15, 2022 14:06

Always set updatedReportObject.lastMessageTimestamp

8f63d20

Use reportActionTimestamp instead of timestamp

87ba2d1

Update lastMessageTimestamp after deleting a message

171834c

puneetlath requested review from marcaaron and chiragsalian December 16, 2022 15:44

Merge branch 'main' into unread-refactor

9cfb7d5

marcaaron self-requested a review January 2, 2023 22:34

marcaaron approved these changes Jan 2, 2023

View reviewed changes

chiragsalian approved these changes Jan 3, 2023

View reviewed changes

sketchydroide mentioned this pull request Jan 3, 2023

Edit command is slow #13742

Closed

puneetlath changed the title ~~[HOLD web-e #35907] Rebuild unread reports without sequence numbers~~ Rebuild unread reports without sequence numbers Jan 3, 2023

chiragsalian merged commit 9a0f817 into main Jan 3, 2023

chiragsalian deleted the unread-refactor branch January 3, 2023 23:29

OSBotify mentioned this pull request Jan 4, 2023

Deploy Checklist: New Expensify 2023-01-04 #13976

Closed

18 tasks

MariaHCD mentioned this pull request Apr 12, 2023

[$1000] Unread messages are not synced between desktop and android devices #17100

Closed

6 tasks

s77rt reviewed May 16, 2024

View reviewed changes

s77rt mentioned this pull request May 16, 2024

[HOLD for payment 2024-05-22] CRITICAL: [$500] Random empty chats are showing in LHN in focus mode #38368

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rebuild unread reports without sequence numbers #13419

Rebuild unread reports without sequence numbers #13419

puneetlath commented Dec 7, 2022 •

edited

Loading

melvin-bot bot commented Dec 14, 2022

puneetlath commented Dec 14, 2022

marcaaron commented Jan 2, 2023

marcaaron commented Jan 2, 2023

marcaaron commented Jan 2, 2023

marcaaron commented Jan 2, 2023

marcaaron left a comment

puneetlath commented Jan 2, 2023

marcaaron commented Jan 2, 2023 •

edited

Loading

marcaaron commented Jan 2, 2023

marcaaron commented Jan 3, 2023

marcaaron commented Jan 3, 2023

marcaaron commented Jan 3, 2023 •

edited

Loading

marcaaron commented Jan 3, 2023

marcaaron commented Jan 3, 2023

puneetlath commented Jan 3, 2023 •

edited

Loading

chiragsalian commented Jan 3, 2023 •

edited

Loading

OSBotify commented Jan 3, 2023

OSBotify commented Jan 4, 2023

OSBotify commented Jan 5, 2023

OSBotify commented Jan 6, 2023

s77rt May 16, 2024

Rebuild unread reports without sequence numbers #13419

Rebuild unread reports without sequence numbers #13419

Conversation

puneetlath commented Dec 7, 2022 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Dec 14, 2022

puneetlath commented Dec 14, 2022

marcaaron commented Jan 2, 2023

marcaaron commented Jan 2, 2023

marcaaron commented Jan 2, 2023

marcaaron commented Jan 2, 2023

marcaaron left a comment

Choose a reason for hiding this comment

puneetlath commented Jan 2, 2023

marcaaron commented Jan 2, 2023 • edited Loading

Reviewer Checklist

Screenshots/Videos

marcaaron commented Jan 2, 2023

marcaaron commented Jan 3, 2023

marcaaron commented Jan 3, 2023

marcaaron commented Jan 3, 2023 • edited Loading

marcaaron commented Jan 3, 2023

marcaaron commented Jan 3, 2023

puneetlath commented Jan 3, 2023 • edited Loading

chiragsalian commented Jan 3, 2023 • edited Loading

OSBotify commented Jan 3, 2023

OSBotify commented Jan 4, 2023

OSBotify commented Jan 5, 2023

OSBotify commented Jan 6, 2023

s77rt May 16, 2024

Choose a reason for hiding this comment

puneetlath commented Dec 7, 2022 •

edited

Loading

marcaaron commented Jan 2, 2023 •

edited

Loading

marcaaron commented Jan 3, 2023 •

edited

Loading

puneetlath commented Jan 3, 2023 •

edited

Loading

chiragsalian commented Jan 3, 2023 •

edited

Loading