[Due payment 2024-10-03][$250] Thread - Unread marker not displayed when opening thread with unread comments #47817

IuliiaHerets · 2024-08-21T20:08:17Z

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Version Number: 9.0.23-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4882295
Email or phone of affected tester (no customers):
Issue reported by: Applause Internal Team

Action Performed:

Open the app and log in
Navigate to any chat
Send a message
Start a thread and send some messages
Make any comment as unread
Navigate to the LHN
Open the the thread

Expected Result:

The new message marker is displayed

Actual Result:

The new message marker is not displayed

Workaround:

Unknown

Platforms:

Screenshots/Videos

Bug6578545_1724265464850.video_2024-08-21_14-36-03.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~0166496cbdb268ee61
Upwork Job ID: 1827933307174227915
Last Price Increase: 2024-09-02

Issue Owner

Current Issue Owner: @kadiealexander

melvin-bot · 2024-08-21T20:08:26Z

Triggered auto assignment to @kadiealexander (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

IuliiaHerets · 2024-08-21T20:09:17Z

@kadiealexander FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

bernhardoj · 2024-08-22T04:52:54Z

Edited by proposal-police: This proposal was edited at 2024-08-22 04:54:49 UTC.

Proposal

Please re-state the problem that we are trying to solve in this issue.

The unread marker doesn't show when manually marked as unread and reopen the report. This happens to all report, not limited to thread.

What is the root cause of that problem?

The logic to display the unread marker checks whether the message is from the current user or not.

App/src/pages/home/report/ReportActionsList.tsx

Lines 225 to 239 in ad7c1d5

    
           const shouldDisplayNewMarker = (reportAction: OnyxTypes.ReportAction, index: number): boolean => { 
        
               const nextMessage = sortedVisibleReportActions[index + 1]; 
        
               const isCurrentMessageUnread = isMessageUnread(reportAction, unreadMarkerTime); 
        
               const isNextMessageRead = !nextMessage || !isMessageUnread(nextMessage, unreadMarkerTime); 
        
               let shouldDisplay = isCurrentMessageUnread && isNextMessageRead && !ReportActionsUtils.shouldHideNewMarker(reportAction); 
        
               if (shouldDisplay && !messageManuallyMarkedUnread) { 
        
                   // Prevent displaying a new marker line when report action is of type "REPORT_PREVIEW" and last actor is the current user 
        
                   const isFromCurrentUser = accountID === (ReportActionsUtils.isReportPreviewAction(reportAction) ? !reportAction.childLastActorAccountID : reportAction.actorAccountID); 
        
                   const isWithinVisibleThreshold = scrollingVerticalOffset.current < MSG_VISIBLE_THRESHOLD ? reportAction.created < (userActiveSince.current ?? '') : true; 
        
                   shouldDisplay = !isFromCurrentUser && isWithinVisibleThreshold; 
        
               } 
        
               return shouldDisplay; 
        
           };

If it's from the current user, we won't show the unread marker, but only if the message is not marked manually (messageManuallyMarkedUnread).

App/src/pages/home/report/ReportActionsList.tsx

Line 172 in ad7c1d5

    
           const [messageManuallyMarkedUnread, setMessageManuallyMarkedUnread] = useState(0);

When we mark it as unread manually, we set the state to the current date.

App/src/pages/home/report/ReportActionsList.tsx

Lines 256 to 258 in ad7c1d5

    
           const unreadActionSubscription = DeviceEventEmitter.addListener(`unreadAction_${report.reportID}`, (newLastReadTime: string) => { 
        
               setUnreadMarkerTime(newLastReadTime); 
        
               setMessageManuallyMarkedUnread(new Date().getTime());

But when we switch the report or close and reopen it, the new report is mounted, so the state is reset.

What changes do you think we should make in order to solve the problem?

We need to store the mark manually state outside the component lifecycle. (OR using Onyx so it's persisted even when closing/refreshing the app)

App/src/pages/home/report/ReportActionsList.tsx

Lines 135 to 136 in ad7c1d5


	function ReportActionsList({

const messageManuallyMarkedUnreadCache = {};

Then, initialize the state with the cached value.

App/src/pages/home/report/ReportActionsList.tsx

Line 172 in ad7c1d5

    
           const [messageManuallyMarkedUnread, setMessageManuallyMarkedUnread] = useState(0);

const [messageManuallyMarkedUnread, _setMessageManuallyMarkedUnread] = useState(messageManuallyMarkedUnreadCache[report.reportID]);

Next, we create a function to update the state and cache.

const setMessageManuallyMarkedUnread = (isManuallyMarkedUnread: boolean) => {
    _setMessageManuallyMarkedUnread(isManuallyMarkedUnread);
    messageManuallyMarkedUnreadCache[report.reportID] = isManuallyMarkedUnread;
}

Last, we need to update the clear code here to clear the cache only.

App/src/pages/home/report/ReportActionsList.tsx

Lines 214 to 219 in ad7c1d5

    
           useEffect(() => { 
        
               setUnreadMarkerTime(report.lastReadTime ?? ''); 
        
               setMessageManuallyMarkedUnread(0); 
        
               // eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps 
        
           }, [report.reportID]);

messageManuallyMarkedUnreadCache[report.reportID] = false;

Note: the above solution changes the type from Date to boolean

melvin-bot · 2024-08-26T04:57:19Z

Job added to Upwork: https://www.upwork.com/jobs/~0166496cbdb268ee61

melvin-bot · 2024-08-26T04:57:24Z

Triggered auto assignment to Contributor-plus team member for initial proposal review - @mollfpr (External)

kadiealexander · 2024-08-26T05:00:23Z

@mollfpr please review the proposal above!

klajdipaja · 2024-08-26T12:02:48Z

Proposal

Please re-state the problem that we are trying to solve in this issue.

The unread marker does not show when the user marks unread a message that they themselves have sent

What is the root cause of that problem?

The handling of the unread marker depends on the state of messageManuallyMarkedUnread which is lost when the user leaves the screen due to the component itself getting destroyed.

What changes do you think we should make in order to solve the problem?

When the message is marked as unread a BE action is triggered that updates the Onyx state appropriately. We can rely on that to display the marker line instead of the component state.
To do so we need to change the logic of the shouldDisplayNewMarker to the one below:

const shouldDisplayNewMarker = (reportAction: OnyxTypes.ReportAction, index: number): boolean => {
            const nextMessage = sortedVisibleReportActions[index + 1];
            const isCurrentMessageUnread = isMessageUnread(reportAction, unreadMarkerTime);
            const isNextMessageRead = !nextMessage || !isMessageUnread(nextMessage, unreadMarkerTime);
            const shouldDisplay = isCurrentMessageUnread && isNextMessageRead && !ReportActionsUtils.shouldHideNewMarker(reportAction);
            const isWithinVisibleThreshold = scrollingVerticalOffset.current < MSG_VISIBLE_THRESHOLD ? reportAction.created < (userActiveSince.current ?? '') : true;
            return shouldDisplay && isWithinVisibleThreshold;
        };

Along with it we also need to remove messageManuallyMarkedUnread from the component as it becomes unused.

What alternative solutions did you explore? (Optional)

melvin-bot · 2024-08-29T18:25:07Z

@mollfpr, @kadiealexander Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot · 2024-09-02T16:00:10Z

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

mollfpr · 2024-09-02T17:19:58Z

Sorry for the delay!

Reviewing 👀

melvin-bot · 2024-09-04T17:59:54Z

@mollfpr @kadiealexander this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

melvin-bot · 2024-09-05T18:09:40Z

@mollfpr, @kadiealexander Whoops! This issue is 2 days overdue. Let's get this updated quick!

mollfpr · 2024-09-05T18:28:00Z

I am still able to reproduce the issue.

I agree with the RCA in both proposals but @bernhardoj did a great job elaborating on the root cause. I onboard with @bernhardoj solution and I think we don't need to use onyx to accommodate our need here.

🎀 👀 🎀 C+ reviewed!

melvin-bot · 2024-09-05T18:28:13Z

Triggered auto assignment to @neil-marcellini, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

klajdipaja · 2024-09-06T07:19:24Z

@mollfpr Wouldn't it be better to rely on the backend information instead of a state in the local storage so that the information is consistent across different devices? If I as a user mark unread a message in my mobile app I would expect that to show as unread if I open expensify in the browser as well.

melvin-bot · 2024-09-13T17:39:22Z

📣 @klajdipaja You have been assigned to this job!
Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs!
Keep in mind: Code of Conduct | Contributing 📖

neil-marcellini · 2024-09-13T17:41:05Z

I also found a bug in the existing logic of the unread marker that comes from isWithinVisibleThreshold that can be tested by openning a chat with between two users with the following steps:

Open chat in user 1

Do nothing for a bit, like a minute.

Open chat in user 2 and send a message to the chat with user 1

Expected result: In user 1 chat the user sees a new message with a new message marker line
Actual result: In user 1 chat the user sees a new message but no marker line

Ok interesting. Would you like to fix that in your PR for this issue, or do you think it would be best to keep separate? We should probably do that to keep the PRs small and focused, unless the solution is a small change. If you plan to keep it separate please report the bug in #expensify-bugs and tag me.

klajdipaja · 2024-09-14T07:41:15Z

@neil-marcellini I will work on the PR on Monday.
I have an idea how to fix the second bug that can fit in the same PR but if that's not the case it would be better to create a separate issue

…ndling of the unread marker so that we rely on the BE information about the last unread action

klajdipaja · 2024-09-18T08:11:27Z

@neil-marcellini Sorry for the delay in creating the PR. It took me more than I thought it would to revive my old MacOs environment for the iOS testing.
Should I add other reviewers to the PR, I'm thinking @arosiclair since he worked on the original code?

arosiclair · 2024-09-18T12:54:23Z

Should I add other reviewers to the PR, I'm thinking @arosiclair since he worked on the original code?

Yeah please add me for review I'd like to take a look.

I also found a bug in the existing logic of the unread marker that comes from isWithinVisibleThreshold that can be tested by openning a chat with between two users with the following steps:

Open chat in user 1
Do nothing for a bit, like a minute.
Open chat in user 2 and send a message to the chat with user 1
Expected result: In user 1 chat the user sees a new message with a new message marker line
Actual result: In user 1 chat the user sees a new message but no marker line

I'm pretty sure this is not a bug. If a user has the report open and is scrolled near the bottom (the scroll view is below the threshold - scrollingVerticalOffset.current < MSG_VISIBLE_THRESHOLD), then newly received messages should not show the unread marker since the user is already reading them.

klajdipaja · 2024-09-19T15:34:43Z

I am wondering if the process for hiring on upwork has failed or maybe it has changed because the job has not been assigned to me on UpWork as should be based on this description:

If your proposal is accepted by the Expensify engineer assigned to the issue, Expensify will hire you on Upwork and assign the GitHub issue to you.

…not_displayed_when_opening_thread_with_unread_comments [GH-47817]- remove messageManuallyMarkedUnread and manual handling of…

melvin-bot · 2024-09-25T23:39:56Z

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

…dling of…"

kadiealexander · 2024-10-02T06:16:19Z

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

[@mollfpr] The PR that introduced the bug has been identified. Link to the PR: [Due payment 2024-10-03][$250] Thread - Unread marker not displayed when opening thread with unread comments #47817 (comment)
[@mollfpr] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: [Due payment 2024-10-03][$250] Thread - Unread marker not displayed when opening thread with unread comments #47817 (comment)
[@mollfpr] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: [Due payment 2024-10-03][$250] Thread - Unread marker not displayed when opening thread with unread comments #47817 (comment)
[@mollfpr] Determine if we should create a regression test for this bug.
[@mollfpr] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
[@kadiealexander] Link the GH issue for creating/updating the regression test once above steps have been agreed upon: https://github.com/Expensify/Expensify/issues/435372

kadiealexander · 2024-10-02T06:17:13Z

Payouts due:

Contributor: $250 @klajdipaja (offer sent)
Contributor+: $250 @mollfpr (via manual requests)

Upwork job is here.

@neil-marcellini is this a regression?

klajdipaja · 2024-10-07T19:49:05Z

@kadiealexander I have accepted the Upwork offer but still has not received the payment. Can you let me know if there was an issue or when I can expect the payment to be made?

kadiealexander · 2024-10-08T03:06:35Z

@klajdipaja just confirming from @neil-marcellini whether there was a regression, and then we'll issue payment. Sorry for the delay!

neil-marcellini · 2024-10-08T18:13:55Z

@kadiealexander Wow yes this one was a bit tricky. There were some regressions and the PR to fix this issue was mentioned, and maybe revealed some other issues, but it was not reverted so I think we can say that it did not directly cause any regressions. I think it's best explained in this comment. Let's pay the full amount.

We can discuss more if anyone passionately disagrees.

kadiealexander · 2024-10-08T20:20:44Z

Sounds great to me, thanks @neil-marcellini!

kadiealexander · 2024-10-08T20:24:50Z

@mollfpr the only steps left are this checklist :)

mollfpr · 2024-10-09T03:56:54Z

[@mollfpr] The PR that introduced the bug has been identified. Link to the PR:
[@mollfpr] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:

I couldn't determined which PR causing the issue.

[@mollfpr] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:

I think the regression step is enough.

[@mollfpr] Determine if we should create a regression test for this bug.
[@mollfpr] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Mark unread user messages

Open a chat with a workspace or another user
Send a few messages in the chat
Mark a message as unread
Open a different chat
Open the same chat again
Verify the "new message" marker line is shown on the message that was marked unread

Unread messages in the thread

Open a chat with a workspace or another user
Send a message in the chat
Start a new thread in the chat and send a few messages
Mark one of the messages as unread
Leave the thread
Open the thread again
Verify the "new message" marker line is shown on the message that was marked unread

Unread messages between devices

Run all the steps for one of the previous tests in the desktop app
Open a different device (mweb, android, ios)
Open the same chat where you marked a message unread
Verify the "new message" marker line is shown on the message that was marked unread

garrettmknight · 2024-10-15T01:15:45Z

$250 approved for @mollfpr

IuliiaHerets added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 21, 2024

melvin-bot bot assigned kadiealexander Aug 21, 2024

kadiealexander added the External Added to denote the issue can be worked on by a contributor label Aug 26, 2024

melvin-bot bot changed the title ~~Thread - Unread marker not displayed when opening thread with unread comments~~ [$250] Thread - Unread marker not displayed when opening thread with unread comments Aug 26, 2024

melvin-bot bot added Overdue Help Wanted Apply this label when an issue is open to proposals by contributors labels Aug 26, 2024

kadiealexander added this to [#whatsnext] #vip-vsb Aug 26, 2024

melvin-bot bot assigned mollfpr Aug 26, 2024

melvin-bot bot removed the Overdue label Aug 26, 2024

melvin-bot bot added the Overdue label Aug 29, 2024

melvin-bot bot removed the Overdue label Sep 2, 2024

melvin-bot bot added the Overdue label Sep 4, 2024

melvin-bot bot removed the Overdue label Sep 5, 2024

melvin-bot bot assigned neil-marcellini Sep 5, 2024

neil-marcellini added Weekly KSv2 and removed Daily KSv2 labels Sep 13, 2024

klajdipaja added a commit to klajdipaja/Expensify-App that referenced this issue Sep 16, 2024

[ExpensifyGH-47817]- remove messageManuallyMarkedUnread and manual ha…

942876f

…ndling of the unread marker so that we rely on the BE information about the last unread action

klajdipaja mentioned this issue Sep 18, 2024

[GH-47817]- remove messageManuallyMarkedUnread and manual handling of… #49367

Merged

48 tasks

melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Weekly KSv2 labels Sep 18, 2024

klajdipaja mentioned this issue Sep 24, 2024

[HOLD for payment 2024-10-04] [$250] Chat - New message line marker shown in the wrong position #49525

Closed

6 tasks

neil-marcellini added a commit that referenced this issue Sep 24, 2024

Merge pull request #49367 from klajdipaja/bugfix/47817_Unread_marker_…

4953d89

…not_displayed_when_opening_thread_with_unread_comments [GH-47817]- remove messageManuallyMarkedUnread and manual handling of…

marcaaron added a commit that referenced this issue Sep 25, 2024

Revert "[GH-47817]- remove messageManuallyMarkedUnread and manual han…

0d674d0

…dling of…"

kadiealexander changed the title ~~[$250] Thread - Unread marker not displayed when opening thread with unread comments~~ [Due payment 2024-10-03][$250] Thread - Unread marker not displayed when opening thread with unread comments Oct 2, 2024

kadiealexander added Daily KSv2 and removed Weekly KSv2 labels Oct 8, 2024

kadiealexander closed this as completed Oct 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Due payment 2024-10-03][$250] Thread - Unread marker not displayed when opening thread with unread comments #47817

[Due payment 2024-10-03][$250] Thread - Unread marker not displayed when opening thread with unread comments #47817

IuliiaHerets commented Aug 21, 2024 •

edited by kadiealexander

Loading

melvin-bot bot commented Aug 21, 2024

IuliiaHerets commented Aug 21, 2024

bernhardoj commented Aug 22, 2024 •

edited by github-actions bot

Loading

melvin-bot bot commented Aug 26, 2024

melvin-bot bot commented Aug 26, 2024

kadiealexander commented Aug 26, 2024

klajdipaja commented Aug 26, 2024

melvin-bot bot commented Aug 29, 2024

melvin-bot bot commented Sep 2, 2024

mollfpr commented Sep 2, 2024

melvin-bot bot commented Sep 4, 2024

melvin-bot bot commented Sep 5, 2024

mollfpr commented Sep 5, 2024

melvin-bot bot commented Sep 5, 2024

klajdipaja commented Sep 6, 2024

melvin-bot bot commented Sep 13, 2024

neil-marcellini commented Sep 13, 2024

klajdipaja commented Sep 14, 2024

klajdipaja commented Sep 18, 2024

arosiclair commented Sep 18, 2024

klajdipaja commented Sep 19, 2024

melvin-bot bot commented Sep 25, 2024

kadiealexander commented Oct 2, 2024 •

edited

Loading

kadiealexander commented Oct 2, 2024 •

edited

Loading

klajdipaja commented Oct 7, 2024

kadiealexander commented Oct 8, 2024

neil-marcellini commented Oct 8, 2024

kadiealexander commented Oct 8, 2024

kadiealexander commented Oct 8, 2024

mollfpr commented Oct 9, 2024

garrettmknight commented Oct 15, 2024

[Due payment 2024-10-03][$250] Thread - Unread marker not displayed when opening thread with unread comments #47817

[Due payment 2024-10-03][$250] Thread - Unread marker not displayed when opening thread with unread comments #47817

Comments

IuliiaHerets commented Aug 21, 2024 • edited by kadiealexander Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

melvin-bot bot commented Aug 21, 2024

IuliiaHerets commented Aug 21, 2024

bernhardoj commented Aug 22, 2024 • edited by github-actions bot Loading

Proposal

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

What changes do you think we should make in order to solve the problem?

melvin-bot bot commented Aug 26, 2024

melvin-bot bot commented Aug 26, 2024

kadiealexander commented Aug 26, 2024

klajdipaja commented Aug 26, 2024

Proposal

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

What changes do you think we should make in order to solve the problem?

What alternative solutions did you explore? (Optional)

melvin-bot bot commented Aug 29, 2024

melvin-bot bot commented Sep 2, 2024

mollfpr commented Sep 2, 2024

melvin-bot bot commented Sep 4, 2024

melvin-bot bot commented Sep 5, 2024

mollfpr commented Sep 5, 2024

melvin-bot bot commented Sep 5, 2024

klajdipaja commented Sep 6, 2024

melvin-bot bot commented Sep 13, 2024

neil-marcellini commented Sep 13, 2024

klajdipaja commented Sep 14, 2024

klajdipaja commented Sep 18, 2024

arosiclair commented Sep 18, 2024

klajdipaja commented Sep 19, 2024

melvin-bot bot commented Sep 25, 2024

kadiealexander commented Oct 2, 2024 • edited Loading

kadiealexander commented Oct 2, 2024 • edited Loading

klajdipaja commented Oct 7, 2024

kadiealexander commented Oct 8, 2024

neil-marcellini commented Oct 8, 2024

kadiealexander commented Oct 8, 2024

kadiealexander commented Oct 8, 2024

mollfpr commented Oct 9, 2024

garrettmknight commented Oct 15, 2024

IuliiaHerets commented Aug 21, 2024 •

edited by kadiealexander

Loading

bernhardoj commented Aug 22, 2024 •

edited by github-actions bot

Loading

kadiealexander commented Oct 2, 2024 •

edited

Loading

kadiealexander commented Oct 2, 2024 •

edited

Loading