App
App copied to clipboard
Expensify
[$8000] Mark as Unread functionality not working with offline mode - reported by @gadhiyamanan
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!
Action Performed:
- Go any chat
- click on any message and mark that message as unread
- kill the app
- reopen app and check chat
Expected Result:
Chat Should be mark as unread
Actual Result:
Chat not mark as unread
Workaround:
Can the user still use Expensify without this being fixed? Have you informed them of the workaround?
Platform:
Where is this issue occurring?
- iOS
- Android
Version Number: 1.1.99-0 Reproducible in staging?: Y Reproducible in production?: Y Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Any additional supporting documentation
https://user-images.githubusercontent.com/43995119/190949830-5a7b8e9d-cbe4-4df0-a9c3-fa2eb6da3f0b.mp4
Upwork job URL: https://www.upwork.com/jobs/~01f5eb4b76fce52748 Issue reported by: @gadhiyamanan Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1661600613194619
Triggered auto assignment to @MitchExpensify (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.
![melvin-bot[bot] avatar](https://avatars.githubusercontent.com/in/179547?v=4 "Published by a pub.dev verified publisher"){kind=small}
Triggered auto assignment to @Luke9389 (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.
Triggered auto assignment to @michaelhaxhiu (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.
Triggered auto assignment to Contributor-plus team member for initial proposal review - @rushatgabhane (External)
Current assignee @Luke9389 is eligible for the External assigner, not assigning anyone new.
Interested to see what comes up here. Not sure if we'll get a proposal that fits perfectly but might as well hear some.
@michaelhaxhiu, @rushatgabhane, @Luke9389 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
@michaelhaxhiu, @rushatgabhane, @Luke9389 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
@michaelhaxhiu i think it was fixed because i am not able to reproduce it , are you still able to reproduce it ?
@gadhiyamanan this was hasty of me, and thanks for your quick comment. Let me try to reproduce (something I should've done before doubling price again... oopsie daisy 🥀 ).
Tested and it does appear to be working now. Can we identify which PR fixed this cc @rushatgabhane? Reason I ask if because I'll payout the bug reporting bonus after 7 days of that PR merging to production.
@michaelhaxhiu I can still reporduce this on mWeb safari (main)
https://user-images.githubusercontent.com/29673073/196245069-12f9f4b7-0a22-4f30-877f-311adc6edcaa.mov
cc @mvtglobally and @gadhiyamanan I'm having a hard time reproducing. Are either of you able to reproduce this one still? Mind trying and including a little video?
Closing this job, and won't reopen unless @mvtglobally can reproduce.
Issue is reproducible in Android app with build 1.2.24.4
https://user-images.githubusercontent.com/93399543/200600847-7516a318-d91a-415c-a3dc-0e4037f564ad.mp4
Reproducible in IOS app with build 1.2.24.4
https://user-images.githubusercontent.com/93399543/200600938-d0522b1d-f53e-4070-84d6-42c989bbe913.MP4
Proposal
1. Reproducable
I could reproduce this issue like below:
-
Mark as Unreadissue (video is attached in the issue description). - User name is not changed to bold when user is on LHN page like here
2. Why the issue is produced?
We need to focus on 2 problems to dig the issue.
-
There are 3 listeners (mainly, don't need to care for keyboardEvent here) that were defined in the
componentDidMountofReportActionsView.js. Actually, this component is the view of the chat history, which will be frozen in the mobile view when LHN is opened. (This component was wrapped byFreezeand will be frozen when the condition isthis.props.isSmallScreenWidth && this.props.isDrawerOpen.) If a component is frozen, no state/props changing will affect to render, but event listeners will be kept to be alive. (You can see the related discussion here). -
Another one is that
MarkAsUnreadapi parameter has a problem.
3. Solution
- Need to disable
OpenReportin theVisibilityListenerand resolve theMarkAsUnreadAPI issue. (issue 1)
this.unsubscribeVisibilityListener = Visibility.onVisibilityChange(() => {
if (!this.getIsReportFullyVisible()) {
return;
}
// If the app user becomes active and they have no unread actions we clear the new marker to sync their device
// e.g. they could have read these messages on another device and only just become active here
this.openReportIfNecessary();
this.setState({newMarkerSequenceNumber: 0});
});
First, we should not call this.openReportIfNecessary() when user is out of this component.
Also, We don't need to call this.openReportIfNecessary() in the Visibility.onVisibilityChange because this listener is needed to check the activity of the AppState (active/inactive), but full reports will be collected when OpenApp or ReconnectApp is called like here.
OpenApp or ReconnectApp is called whenever app is activated.
We can debug the responses of OpenApp or ReconnectApp in the middleware (e.g. SaveResponseInOnyx middleware).
So, don't need to get reports inside of this listener, and will remove like below. (If we need to keep it, this should be prevented from the parent component like issue 2 solution)
this.unsubscribeVisibilityListener = Visibility.onVisibilityChange(() => {
if (!this.getIsReportFullyVisible()) {
return;
}
// If the app user becomes active and they have no unread actions we clear the new marker to sync their device
// e.g. they could have read these messages on another device and only just become active here
+ // Calling openReportIfNecessary method is not needed in this listener because full reports will be collecetd when +`OpenApp` and `ReconnectApp` are finished.
+ // We can check the response of them inside middleware.
+ //this.openReportIfNecessary();
this.setState({newMarkerSequenceNumber: 0});
});
Also, I've noticed that MarkAsUnread api parameter has a problem.
function markCommentAsUnread(reportID, sequenceNumber) {
const newLastReadSequenceNumber = sequenceNumber - 1;
API.write('MarkAsUnread',
{
reportID,
sequenceNumber,
},
{
In this code, sequenceNumber is maxSequenceNumber.
If we save in this way, all of the getting report commands (OpenApp, ReconnectApp, OpenReport, ...) will return the wrong report, which target report has lastSequenceNumber == maxSequenceNumber even though MarkAsUnRead is finished.
I don't know the backend code, but looks like data is saved somewhere after changing the Onyx data.
So, I can resolve this issue by changing like below:
function markCommentAsUnread(reportID, sequenceNumber) {
const newLastReadSequenceNumber = sequenceNumber - 1;
API.write('MarkAsUnread',
{
reportID,
- sequenceNumber,
+ sequenceNumber: newLastReadSequenceNumber,
},
{
By doing this, I can get the correct lastSequenceNumber (which is same as maxSequenceNumber - 1) after calling MarkAsUnRead -> <Getting Report Commands>.
By doing this, issue 1 will be resolved.
- Need to call
ReadNewestActionin the parent level. (issue 2)
...
if (this.currentScrollOffset === 0) {
Report.readNewestAction(this.props.report.reportID);
this.setState({newMarkerSequenceNumber: 0});
} else if (this.state.newMarkerSequenceNumber === 0) {
...
Issue 2 is produced because unsubscribeFromNewActionEvent is alive and Report.readNewestAction is called when user is on LHN page.
So, need to prevent to call of this method when user is on LHN page.
But we can't use any updated props variable when it is frozen.
So, need to call this method in the parent.
I am writing code like below.
In the pages/home/ReportScreen.js
...
this.dismissBanner = this.dismissBanner.bind(this);
+ this.readNewestReportAction = this.readNewestReportAction.bind(this);
this.removeViewportResizeListener = () => {};
....
chatWithAccountManager() {
Navigation.navigate(ROUTES.getReportRoute(this.props.accountManagerReportID));
}
+ /**
+ * read newest reports actions, but will not read when freeze
+ * @param {string} reportID
+ */
+ readNewestReportAction(reportID) {
+ if (this.props.isSmallScreenWidth && this.props.isDrawerOpen) {
+ return;
+ }
+ Report.readNewestAction(reportID);
+ }
render() {
...
{this.isReportReadyForDisplay() && (
<ReportActionsView
reportActions={this.props.reportActions}
report={this.props.report}
session={this.props.session}
isComposerFullSize={this.props.isComposerFullSize}
isDrawerOpen={this.props.isDrawerOpen}
parentViewHeight={this.state.skeletonViewContainerHeight}
+ readNewestReportAction={this.readNewestReportAction}
/>
)}
...
In the pages/home/report/ReportActionsView.js
...
+ /** Read newest report action by considering freeze */
+ readNewestReportAction: PropTypes.func,
...
// stays in it's previous position.
if (this.currentScrollOffset === 0) {
- Report.readNewestAction(this.props.report.reportID);
+ this.props.readNewestReportAction(this.props.report.reportID);
this.setState({newMarkerSequenceNumber: 0});
} else if (this.state.newMarkerSequenceNumber === 0) {
this.setState({newMarkerSequenceNumber: currentLastSequenceNumber});
}
....
scrollToUnreadMsgAndMarkReportAsRead() {
ReportScrollManager.scrollToIndex({animated: true, index: this.props.report.maxSequenceNumber -
this.state.newMarkerSequenceNumber}, false);
- Report.readNewestAction(this.props.report.reportID);
+ this.props.readNewestReportAction(this.props.report.reportID)
}
Finally, we can get resolved like here
Looks like something related to react-navigation may have been mentioned in this issue discussion.
As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.
Feel free to drop a note in #expensify-open-source with any questions.
@bondydaa I see you commented before we got a proposal (protip: when you assign yourself, remove the Help Wanted label in the future). For now, probably makes just as much sense to have @rushatgabhane review and see where we get.
