[HOLD for payment 2022-01-21] User is not navigated last active conversation after loading Home URL - Reported by: @parasharrajat

[isagoico]

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

---

Action Performed:

1. Open staging app.
2. Go to any report.
3. Close the app.
4. open the app again from the Home URL (which does not have report id).

Expected Result:

User should have been navigated to last active report

Actual Result:

User is navigated to the previous active report.

Workaround:

No need for workaround. User can navigate to any conversation in LHN

Platform:

Where is this issue occurring?

• Web

Version Number: 1.1.16-6

Reproducible in staging?: Yes
Reproducible in production?: Yes

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

output_file.3.mp4

Expensify/Expensify Issue URL:

Issue reported by: @parasharrajat
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1637770185436600

View all open jobs on GitHub

[MelvinBot]

Triggered auto assignment to @flodnv (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[flodnv]

@isagoico @parasharrajat is this problem only on staging? If yes, does this need to be a deploy blocker?

[parasharrajat]

@flodnv It's on production too.

[MelvinBot]

Triggered auto assignment to @MitchExpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[MitchExpensify]

Upwork job

[MelvinBot]

Triggered auto assignment to @parasharrajat (Exported)

[MelvinBot]

Triggered auto assignment to @marcaaron (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[marcaaron]

Back off Melv' we're waiting for proposals!

[parasharrajat]

Still waiting for proposals.

[K4tsuki]

proposal

App/src/libs/reportUtils.js

Line 112 in 7e0717f

function findLastAccessedReport(reports, ignoreDefaultRooms) {

The cause is findLastAccessedReport currently returning local reports data by lastVisitedTimestamp value. But the lastVisitedTimestamp isn't updated when we navigate between rooms (I think it is updated only when we navigate to room that has unread message). My proposal is to use Onyx value CURRENTLY_VIEWED_REPORTID. This value is updated when user navigate between room.

let lastVisitedReportId;
Onyx.connect({
    key: ONYXKEYS.CURRENTLY_VIEWED_REPORTID,
    callback: val => lastVisitedReportId = val ? val : null,
});

This findLastAccessedReport function is used by MainDrawerNavigator when we visit the server by HOME url.

[parasharrajat]

The cause is findLastAccessedReport currently returning local reports data by lastVisitedTimestamp value. But the lastVisitedTimestamp isn't updated when we navigate between rooms (I think it is updated only when we navigate to a room that has an unread message

Seems like @K4tsuki 's hunch is correct here.

My proposal is to use Onyx value CURRENTLY_VIEWED_REPORTID

But if do this, it will cause a bug which is resolved by this PR #5338. Do you think we can fix this without causing that bug?

[marcaaron]

Seems like we just need to update the last visited timestamp. The early return here is likely preventing it from happening.

App/src/libs/actions/Report.js

Lines 1270 to 1275 in bfaabd9

	// We call this method in many cases where there's nothing to update because we already updated it, so we avoid
	// doing an unnecessary server call if the last read is the same one we had already
	if (lastReadSequenceNumbers[reportID] === lastReadSequenceNumber) {
	return;
	}
	setLocalLastRead(reportID, lastReadSequenceNumber);

Doesn't seem like there will be any negative effects if we simply move it above the return? (someone will need to verify that is correct)

[parasharrajat]

@K4tsuki We value your time and contributions but based on the contribution guidelines, you can only work on one issue at a time as you are a new contributor and already hired for another job. So we will have to hold your proposal, if any.

[K4tsuki]

Why? I working hard on this. @mallenexpensify @puneetlath am I really cannot submit many proposal at a times. I have been working hard on some issues.

[marcaaron]

@K4tsuki Sorry I'm not following the explanation or what the proposed fix is.

I've explained in my comment that there's a PR where we stopped updating the lastVisitedTimestamp (which happens when we call the Report_UpdateLastRead API). I am sure we just need to revert that change and then see if the API is called "too many times" by looking into what calls it.

@parasharrajat This doesn't seem too urgent as we've had the issue for 5 months. Maybe we can hire @K4tsuki and wait until they are finished with their other issue before working on this one.

[parasharrajat]

Yup, That can be done as there is one proposal.

🎀 👀 🎀 C+ reviewed

[K4tsuki]

My original proposal is to update calling lastVisitedTimestamp on each time ReportActionsView didMount. This will update lastVisitedTimestamp of a report each time we visit a room.

But as you said it be done using updateLastReadActionID, this function is called each time ReportActionsView didMount. I need to figure out to solve the issue #4077 with different method and make updateLastReadActionID success on each ReportActionsView didMount. I am researching on this.

[K4tsuki]

updateLastReadActionID is called four times on making a comment because of:
ReportActionsView is updated twice. We use temporary optimisticReportActionID sequence number when sending new comment:

1. Updated on new comment with optimisticReportActionID
2. Updated when that comment optimistic id is changed to sequence number from server.

The other two call is from scrollToListBottom:

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

Line 213 in 3f13de8

this.scrollToListBottom();

---

• My solution is to check if new report action has optimistic id in componentDidUpdate
• add parameter to scrollToListBottom and check whether we must call updateLastReadActionID

[marcaaron]

ReportActionsView is updated twice. We use temporary optimisticReportActionID sequence number when sending new comment

Two calls does not seem so bad to me.

The other two call is from scrollToListBottom:

Got it. So there are some cases where we want to record the last read and ones where we do not.

How about we rename the existing method to scrollToBottomAndUpdateLastRead() and replace all usages of scrollToListBottom() except the one here:

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

Lines 209 to 214 in 3f13de8

	// If a new comment is added and it's from the current user scroll to the bottom otherwise
	// leave the user positioned where they are now in the list.
	const lastAction = CollectionUtils.lastItem(this.props.reportActions);
	if (lastAction && (lastAction.actorEmail === this.props.session.email)) {
	this.scrollToListBottom();
	}

This one can call ReportScrollManager.scrollToBottom() and that way it will be clear that whenever we scroll using this method it has the side effect to update the last read.

[K4tsuki]

How about we rename the existing method to scrollToBottomAndUpdateLastRead() and replace all usages of scrollToListBottom() except the one here:
This one can call ReportScrollManager.scrollToBottom() and that way it will be clear that whenever we scroll using this method it has the side effect to update the last read.

All right. I think this is good.

But calling

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

Line 230 in 3f13de8

Report.updateLastReadActionID(this.props.reportID);

twice, Is it okay?
@marcaaron

[marcaaron]

But calling Report.updateLastReadActionID(this.props.reportID); twice, Is it okay?

Yes, if one of those calls is the optimistic update then it seems fine to me.

@MitchExpensify I think we can hire @K4tsuki at this point seems like there is enough context here for us to address the issue.

[MelvinBot]

📣 @K4tsuki You have been assigned to this job by @marcaaron!
Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[MitchExpensify]

Thanks @marcaaron !

@K4tsuki Mind making a proposal on the Upwork job so I can hire you? I can't see you via Upwork search

[MitchExpensify]

Done, thanks @K4tsuki !

[K4tsuki]

Thank you.

[botify]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.29-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Allow App to Navigate to Last Visited Room On visiting with HOME url #6978

If no regressions arise, payment will be issued on 2022-01-21. 🎊

[MitchExpensify]

Paid @K4tsuki!

@parasharrajat I've sent an offer on a reposting of the job (Upwork automatically closed the previous one) so I can pay your reporting fee!

[parasharrajat]

@MitchExpensify Could you share the URL for the job? I am seeing a very different title from this one. Also, I should be eligible for reporting and C+.

[K4tsuki]

Thank you @MitchExpensify, I really appreciate it.

[MitchExpensify]

Here you go @parasharrajat. I will include C+ amount too when paying

[parasharrajat]

@MitchExpensify This is private. You will have to invite me.

[MitchExpensify]

My mistake. I've made it public now! Thanks @parasharrajat

[MitchExpensify]

Thanks for your patience @parasharrajat, C+ & reporting bonus paid