New line indicator working fine #6764

mateusbra · 2021-12-14T23:39:56Z

Details

Now new line indicator shows up when opening report by URL.

Fixed Issues

$ #6471

Tests

Copy the report url.
Close the app.
Go to device 2 and send few messages to that report.
Go back to device 1 and open that report via url.
Note that now New line indicator shows up.

QA Steps

Web, Mobile Web:

Copy the report url.
Close the app.
Go to device 2 and send few messages to that report.
Go back to device 1 and open that report via url.
Note that now New line indicator shows up.

Desktop:

Go to device 1 (Desktop app) and open a report.
Run command+R to reload the Desktop app.
While it is reloading, quickly go to device 2 and send few messages to that report. (We are simulating the report opened directly by URL within desktop app)
At this point we should have new pending messages when desktop app is about to load.
The reload will show up the opened report with the pending messages.
Note that now New line indicator shows up.

Android,iOS:

Go to device 2 and send few messages to a report.
Go to device 1 and open the app.
Within device 1 open that report.
Note that New line indicator shows up normally.

NOTE: We still have a delay on showing NEW marker, we will open a new issue separatelly for this.

Tested On

Videos

Web

2022-03-14.17-49-10.mp4

Mobile Web

WhatsApp.Video.2022-03-14.at.18.33.46.mp4

Android

2022-03-14.19-24-19.mp4

Desktop

WhatsApp.Video.2022-03-14.at.23.47.17.mp4

iOS

whatsapp-video-2022-03-15-at-004509_3JXAnGo9.mp4

parasharrajat · 2021-12-15T12:56:27Z

@mateusbra Could you please upload videos for the rest of the platforms?

parasharrajat · 2021-12-15T18:24:28Z

There is a regression from your changes. When we foreground the app from the background on Tablet the newline indicator is visible only for short period of time.

Now

output_file.mp4

There could be more regressions like this. Please test it carefully before requesting another reivew.

mateusbra · 2021-12-15T21:10:08Z

Hey @parasharrajat, thanks for the feedback!
I'm trying to reproduce this behavior from my side but so far I couldn't reproduce it.
Did you tested it with some extra conditions?(opening keyboard or something like that?)
I'll keep trying testing to find out what is happening.

parasharrajat · 2021-12-16T08:23:54Z

There were straight actions.

Open the app on Android Tab.
Click on the report.
Press the Home(middle) button to minimize.
Open the app on the web.
Open the same report but log in with another participant.
Send a message.
Go to the android tab.
Press the Recent key to show all minimized app.
Click to open NewDot.
New line shows shortly and then vanishes.

parasharrajat · 2021-12-20T11:42:45Z

Any update for us.

mateusbra · 2021-12-20T21:26:12Z

I tested multiple times and still couldn't reproduce it:

Gravacao.de.Tela.2021-12-20.as.15.47.30.mov

@parasharrajat do you think I'm missing something?

@NikkiWines could you test and see if you can reproduce the behavior @parasharrajat had?

In all my tests it requested ReportData only once(when it loads the report view for the first time)

parasharrajat · 2021-12-20T21:56:49Z

Yes, you are @mateusbra. I meant to open the Android app not App on the web browser on an Android tablet.

NikkiWines · 2021-12-21T01:26:01Z

@mateusbra, it looks like I can reproduce the bug @parasharrajat mentioned, though only on an Android tablet, not on the phone sim, weirdly.

Screen.Recording.2021-12-20.at.17.09.47.mov

Screen.Recording.2021-12-20.at.17.23.36.mov

mateusbra · 2021-12-21T03:42:18Z

Thanks both of you for the feedback, I'll try to reproduce it, sorry for the missunderstanding😅

mateusbra · 2021-12-23T19:27:16Z

The reason it wasn't working on tablet is:
On the tablet, it called the API multiple times, making our changes trigger the function this.updateLocalUnreadActionCount(); unnecessarily.

I created a new state(isReportDataLoaded), this way we know when we first receive the report data.
This way we don't have to call this.updateLocalUnreadActionCount(); unnecessarily.
We just call it once when the data is received.

mateusbra · 2021-12-23T19:29:31Z

I'm getting an eslint error:

error  Do not use setState in componentDidUpdate  react/no-did-update-set-state

Should I create a function just for the purpose of setting its state?
What you think we should do to solve this?

parasharrajat · 2021-12-27T20:03:09Z

Back from vacation, on my list. I will review your change @mateusbra asap.

mateusbra · 2021-12-27T20:28:09Z

@parasharrajat I think I have another approach:
What you think if instead of using ONYXKEYS:IS_LOADING_REPORT_DATA we create another ONYXKEY that works like ONYXKEYS:IS_INITIAL_REPORT_DATA_LOADED.

App/src/libs/actions/Report.js

Line 1020 in 22a7baf

Onyx.set(ONYXKEYS.INITIAL_REPORT_DATA_LOADED, true);

ONYXKEYS.INITIAL_REPORT_DATA_LOADED:This flag is supposed to be true and remain true after you have downloaded initial data and have some reports stored locally.
I think we could create a new ONYXKEY:IS_REPORT_DATA_LOADED, this key would have its initial value as false and would become true when the report data arrives. This way we wouldn't have to control when data arrives at ReportActionsView.js we would have it setted by our new ONYXKEY flag

parasharrajat · 2021-12-30T15:08:37Z

Finally, I got time to review this.

ONYXKEYS.IS_LOADING_REPORT_DATA is key will be true multiple times as it indicates whenever report data is refreshed and we refresh the report data multiple times during the app lifecycle.

I think it would be better to use another Onyx key to detect the first-time load but all these keys could make the logic confusing.

Now there are two options

Either we create a HOC that subscribes to this key and updates the state only the first time key is set to true.
Or we use another Onyx key.

The prime motive should always be to keep the ReportActionsView.js as light as possible.

@mateusbra Let's get more eyes on this. Could you please raise a discussion in Slack?

mateusbra · 2022-01-03T18:02:04Z

Ok @parasharrajat, sorry for the late reply, I'll rise a discussion about it in Slack

marcaaron · 2022-01-03T18:57:26Z

Can someone break down what problem is solved by having a new key that tracks when a report is loaded for the "first time"?

I see a few issues with the current approach, but think it would be more productive to discuss alternatives before improving what is currently presented.

mateusbra · 2022-01-03T21:44:10Z

@marcaaron the main problem here is that we are trying to access report data when its not ready for being accessed(its getting undefined), the solution we are proposing here is to access report data when its ready for being accessed(when we have a response from API).

here:

App/src/libs/actions/Report.js

Lines 1137 to 1139 in de5d2ba

    
           function getLastReadSequenceNumber(reportID) { 
        
               return lastReadSequenceNumbers[reportID]; 
        
           }

lastReadSequenceNumbers[reportID] returns undefined if we try to access it before report data is ready, we need it to show our New line indicator.

we are calling for this.updateUnreadIndicatorPosition(this.props.report.unreadActionCount); before report data is ready for being accessed inside componentDidMount in ReportActionsView.js:

App/src/pages/home/report/ReportActionsView.js

Line 146 in de5d2ba

this.updateUnreadIndicatorPosition(this.props.report.unreadActionCount);

our solution:

I tried first to solve it by subscribing to ONYXKEYS.IS_LOADING_REPORT_DATA, refreshing the New line indicator when data is ready for being accessed( this way we dont get undefined from lastReadSequenceNumbers[reportID] and it works fine) by calling this.updateLocalUnreadActionCount(); when report data is received
But @parasharrajat found a regresion from it. We call API multiple times and the key ONYXKEYS.IS_LOADING_REPORT_DATA is setted to true multiple times, making it call this.updateLocalUnreadActionCount(); multiple times, even when it shouldnt call it.
So now I think our solution would be to call this.updateLocalUnreadActionCount(); only once when data is ready for being accessed.

marcaaron · 2022-01-03T22:43:39Z

we are calling for this.updateUnreadIndicatorPosition(this.props.report.unreadActionCount); before report data is ready for being accessed inside componentDidMount in ReportActionsView.js:

Not sure I understand why knowing that the data is ready to be accessed is important...

Could we try to wait for this value to become available in componentDidUpdate() and then it should only be called once if it was undefined before?

if (!prevProps.report.unreadActionCount && this.props.report.unreadActionCount) {
    this.updateUnreadIndicatorPosition();
}

mateusbra · 2022-01-04T01:54:59Z

@marcaaron I'll try to test it and give you a feedback, thanks 😄

mateusbra · 2022-01-07T00:52:35Z

@marcaaron I couldn't make it work with your proposed changes.

this.props.report.unreadActionCount isn't getting undefined, it won't enter in the if statement...

We are getting undefined when inside the function this.updateUnreadIndicatorPosition(this.props.report.unreadActionCount); in componentDidMount():

App/src/pages/home/report/ReportActionsView.js

Line 146 in de5d2ba

this.updateUnreadIndicatorPosition(this.props.report.unreadActionCount);

then, in the called function we have:

App/src/pages/home/report/ReportActionsView.js

Lines 381 to 391 in de5d2ba

    
               /** 
        
                * Updates NEW marker position 
        
                * @param {Number} unreadActionCount 
        
                */ 
        
               updateUnreadIndicatorPosition(unreadActionCount) { 
        
                   // Since we want the New marker to remain in place even if newer messages come in, we set it once on mount. 
        
                   // We determine the last read action by deducting the number of unread actions from the total number. 
        
                   // Then, we add 1 because we want the New marker displayed over the oldest unread sequence. 
        
                   const oldestUnreadSequenceNumber = unreadActionCount === 0 ? 0 : Report.getLastReadSequenceNumber(this.props.report.reportID) + 1; 
        
                   Report.setNewMarkerPosition(this.props.reportID, oldestUnreadSequenceNumber); 
        
               }

when we call Report.getLastReadSequenceNumber(this.props.report.reportID) if report data is not ready we get undefined from it

marcaaron · 2022-01-07T01:09:19Z

We are getting undefined when inside the function this.updateUnreadIndicatorPosition(this.props.report.unreadActionCount); in componentDidMount():

Thanks, I think I understood that the first time it was mentioned. But if it is undefined when the component mounts then eventually it should have a value? And that is why "waiting for it to appear" resolves the issue. So, if that's the case, why
does the value not appear in componentDidUpdate()?

when we call Report.getLastReadSequenceNumber(this.props.report.reportID) if report data is not ready we get undefined from it

Yes, we need to figure out why? Are you able to look some more and return with an explanation or some alternative?

mateusbra · 2022-01-10T18:35:48Z

Sorry for the late reply again, I'll be able to give more attention for this PR for now on.

But if it is undefined when the component mounts then eventually it should have a value?

Yes, it will have a value when the data is received from the API

So, if that's the case, why does the value not appear in componentDidUpdate()?

I don't know if I understood you well in this part, but I think we can call it inside componentDidUpdate(); if report data has already been received and it should work fine, but the problem I think we are facing here is that we don't know when data is ready for being accessed in ReportActionsView.js and we need to know it, so we can access it with available data to read(let me know what you think of this, you think could be another alternative way to do it?).

Yes, we need to figure out why? Are you able to look some more and return with an explanation or some alternative?

I think Its because inside Report.js we only fill it with values after the API data is receive(before it we don't have a lastReadSequenceNumbers[reportID], we just have an empty array)
I'll keep working on it and trying to find an alternative way to do it

parasharrajat · 2022-01-13T18:25:30Z

@mateusbra Any update? Have you found any other way to fix it?

parasharrajat · 2022-02-18T09:26:57Z

Thanks @mateusbra. I will help you close this ticket in next week. I will try to debug the whole cycle of data loading for report and see where are we lacking and how will your changes affect it.

parasharrajat · 2022-02-24T17:09:53Z

I debugged this but it is confusing and complex logic. I am will debug more on this. Mostly try to see how it was designed in old PRs.

parasharrajat · 2022-02-28T16:37:31Z

Looking into this today..

mateusbra · 2022-03-03T23:15:01Z

Now we have the New marker appearing correctly and the report being marked as read as soon as we get isLoadingReportData=true, but we still have a delay from creating all report data (fetchAllReports() function)

parasharrajat · 2022-03-07T10:43:15Z

Got it. but did you check with the changes that I suggested?

parasharrajat · 2022-03-08T19:52:44Z

At this stage, I say we are good with the delay. It only affects page load on web | Desktop. This PR has taken a serious amount of time and I don't see any possible improvement so @NikkiWines Do you think we should merge this and open a new fresh issue for the delay.

mateusbra · 2022-03-08T22:28:26Z

At this stage, I say we are good with the delay. It only affects page load on web | Desktop. This PR has taken a serious amount of time and I don't see any possible improvement so @NikkiWines Do you think we should merge this and open a new fresh issue for the delay.

I agree with that...

I think we also gonna solve #7837 with this PR.

NikkiWines · 2022-03-10T01:13:09Z

It's definitely not ideal to merge a PR with a known issue like this but I do think we're in a holding pattern for the moment trying to find the perfect solution. So, I agree we should re-review and merge without trying to fix the delay. I also think it's a good idea to make a new issue for the delay so we can try and get that resolved as well 👍

@mateusbra could you merge main into your branch and retest to confirm everything still works as expected? We also need videos for each device, not only for web.

mateusbra · 2022-03-14T05:19:39Z

I'll work on it today to provide the test videos...

mateusbra · 2022-03-15T03:55:35Z

#6764 (comment) updated providing all platform tests

parasharrajat · 2022-03-15T10:21:15Z

@mateusbra Desktop Video is wrong. Please mention the platform in your QA steps. Also, add steps for testing the new line indicator on all platforms.

mateusbra · 2022-03-15T11:23:13Z

@parasharrajat sorry, I think I mixed up the correctly video for desktop, already changed it....

Please mention the platform in your QA steps. Also, add steps for testing the new line indicator on all platforms.

You're saying I need to cover every single platform with a different QA steps for it? Or I just should add which platforms the issue was occurring?

parasharrajat · 2022-03-15T11:58:21Z

As this PR touches newline functionality it becomes important to get that tested on each platform. Thus, I am asking if a platform has different testing steps then please add seperate steps for it.

Thank you.

mateusbra · 2022-03-15T12:53:39Z

Did it, please let me know if you think there's something missing.

parasharrajat

LGTM.

CC: @NikkiWines

🎀 👀 🎀 C+ reviewed

NikkiWines · 2022-03-17T01:17:21Z

Sorry guys, been totally swamped with work this week - I'll review this first thing tomorrow

NikkiWines

👍 This looks good - I would add in above the QA steps that the delay is expected and that we'll create a new issue to resolve it.

NikkiWines · 2022-03-17T18:16:58Z

@marcaaron can I dismiss your old review or do you want to give this another once over.

mateusbra · 2022-03-17T19:33:37Z

Thanks you for the review @parasharrajat @NikkiWines!

I would add in above the QA steps that the delay is expected and that we'll create a new issue to resolve it.

Added!

OSBotify · 2022-03-18T00:11:13Z

✋ 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 · 2022-03-24T21:01:40Z

🚀 Deployed to staging by @marcaaron in version: 1.1.46-0 🚀

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

OSBotify · 2022-03-30T19:27:59Z

🚀 Deployed to production by @timszot in version: 1.1.46-3 🚀

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

New line indicator working fine

b35add4

mateusbra requested a review from a team as a code owner December 14, 2021 23:39

MelvinBot requested review from NikkiWines and parasharrajat and removed request for a team December 14, 2021 23:39

NikkiWines assigned mateusbra Dec 15, 2021

ätrack report data to know when it was loaded first time

f762a9e

mateusbra added 2 commits March 3, 2022 19:56

reverting previously changes

153f980

Merge branch 'main' into new-line

fb0fbc2

Merge branch 'main' into new-line

ca96c21

mateusbra force-pushed the new-line branch from 153d99b to ca96c21 Compare March 14, 2022 05:18

parasharrajat approved these changes Mar 15, 2022

View reviewed changes

marcaaron mentioned this pull request Mar 17, 2022

[HOLD OFFLINE + Report Logic Refactor] Unread messages stays unread on opening the chat when offline - Reported by @thesahindia #7727

Closed

NikkiWines approved these changes Mar 17, 2022

View reviewed changes

marcaaron approved these changes Mar 18, 2022

View reviewed changes

marcaaron merged commit c0b3181 into Expensify:main Mar 18, 2022

OSBotify mentioned this pull request Mar 24, 2022

Deploy Checklist: New Expensify 2022-03-24 #8305

Closed

69 tasks

New line indicator working fine #6764

New line indicator working fine #6764

Uh oh!

Conversation

mateusbra commented Dec 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

QA Steps

Tested On

Videos

Web

Mobile Web

Android

Desktop

iOS

Uh oh!

parasharrajat commented Dec 15, 2021

Uh oh!

parasharrajat commented Dec 15, 2021

Previous

Now

Uh oh!

mateusbra commented Dec 15, 2021

Uh oh!

parasharrajat commented Dec 16, 2021

Uh oh!

parasharrajat commented Dec 20, 2021

Uh oh!

mateusbra commented Dec 20, 2021

Uh oh!

parasharrajat commented Dec 20, 2021

Uh oh!

NikkiWines commented Dec 21, 2021

Uh oh!

mateusbra commented Dec 21, 2021

Uh oh!

mateusbra commented Dec 23, 2021

Uh oh!

mateusbra commented Dec 23, 2021

Uh oh!

parasharrajat commented Dec 27, 2021

Uh oh!

mateusbra commented Dec 27, 2021

Uh oh!

parasharrajat commented Dec 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mateusbra commented Jan 3, 2022

Uh oh!

marcaaron commented Jan 3, 2022

Uh oh!

mateusbra commented Jan 3, 2022

Uh oh!

marcaaron commented Jan 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mateusbra commented Jan 4, 2022

Uh oh!

mateusbra commented Jan 7, 2022

Uh oh!

marcaaron commented Jan 7, 2022

Uh oh!

mateusbra commented Jan 10, 2022

Uh oh!

parasharrajat commented Jan 13, 2022

Uh oh!

parasharrajat commented Feb 18, 2022

Uh oh!

parasharrajat commented Feb 24, 2022

Uh oh!

parasharrajat commented Feb 28, 2022

Uh oh!

mateusbra commented Mar 3, 2022

Uh oh!

parasharrajat commented Mar 7, 2022

Uh oh!

parasharrajat commented Mar 8, 2022

Uh oh!

mateusbra commented Dec 14, 2021 •

edited

Loading

parasharrajat commented Dec 30, 2021 •

edited

Loading

marcaaron commented Jan 3, 2022 •

edited

Loading

NikkiWines commented Mar 10, 2022 •

edited

Loading

parasharrajat commented Mar 15, 2022 •

edited

Loading