Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HOLD on #15212] Bug: Mark as unread state vanishes after closing the app in IOS app @chauchausoup #11515

Closed
kavimuru opened this issue Oct 2, 2022 · 50 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Monthly KSv2
Projects

Comments

@kavimuru
Copy link

kavimuru commented Oct 2, 2022

On hold for navigation improvements through #11768

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 up IOS app
  2. Open any chat
  3. Click at Mark as Unread
  4. Go back in LHN and see that the bold is there, which means user has not read the message
  5. Click on home button / close the app

Expected Result:

Since it was marked as unread, user thinks that the message should be bold but it’s not showing that remark as expected.

Actual Result:

Bold remark in Unread message is vanished.

Workaround:

Unknown

Platform:

Where is this issue occurring?

  • iOS

Version Number: 1.2.11-0
Reproducible in staging?: Yes #11515 (comment)
Reproducible in production?: Needs reproduction
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

RPReplay_Final1664605493.MP4
KUTO7893.1.MP4

Expensify/Expensify Issue URL:
Issue reported by: @chauchausoup
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1664605579764649

View all open jobs on GitHub

@kavimuru kavimuru added the Needs Reproduction Reproducible steps needed label Oct 2, 2022
@melvin-bot melvin-bot bot added the Monthly KSv2 label Oct 5, 2022
@tylerkaraszewski tylerkaraszewski self-assigned this Oct 7, 2022
@puneetlath puneetlath added the Bug Something is broken. Auto assigns a BugZero manager. label Oct 19, 2022
@tylerkaraszewski
Copy link
Contributor

Reproduced in staging. It "fixes" itself once but not twice.

RPReplay_Final1667235547.mov

@tylerkaraszewski tylerkaraszewski removed their assignment Oct 31, 2022
@tylerkaraszewski tylerkaraszewski added the External Added to denote the issue can be worked on by a contributor label Oct 31, 2022
@melvin-bot
Copy link

melvin-bot bot commented Oct 31, 2022

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

@melvin-bot melvin-bot bot added Daily KSv2 and removed Monthly KSv2 labels Oct 31, 2022
@melvin-bot
Copy link

melvin-bot bot commented Oct 31, 2022

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 31, 2022
@melvin-bot
Copy link

melvin-bot bot commented Oct 31, 2022

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

@melvin-bot melvin-bot bot changed the title Bug: Mark as unread state vanishes after closing the app in IOS app @chauchausoup [$250] Bug: Mark as unread state vanishes after closing the app in IOS app @chauchausoup Oct 31, 2022
@tienifr
Copy link
Contributor

tienifr commented Nov 2, 2022

Proposal

Problem

This issue happens on both mWeb and native

Screen.Recording.2022-11-01.at.20.12.04.mp4

In

freeze={this.props.isSmallScreenWidth && this.props.isDrawerOpen}

We use Freeze component that prevents the components re-render whenever user uses small screen(isSmallScreenWidth = true) and be in home page(isDrawerOpen = true)

In ReportActionsView
we listen visibilityListener to detect visibility (switch tab or kill app then open again) and open report when user's still in chat(isDrawerOpen = true)

componentDidMount() {
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});
});

we just unsubscribe visibilityListener when unMount, but chat's still there when user goes back to home (home is just the drawer). Besides, Freeze prevents the components re-render => ReportActionsView will use the stale props, that makes isDrawerOpen = true even user goes back to home.

getIsReportFullyVisible() {
const isSidebarCoveringReportView = this.props.isSmallScreenWidth && this.props.isDrawerOpen;
return Visibility.isVisible() && !isSidebarCoveringReportView;
}

Solution

Solution 1:

In ReportScreen

Use setTimeout to trigger freeze => That makes ReportActionView has time to update props.

        this.state = {
            skeletonViewContainerHeight: reportActionsListViewHeight,
            viewportOffsetTop: 0,
            isBannerVisible: true,
+            freeze:false
        };
...
    componentDidUpdate(prevProps) {
+        const shouldEnableFreeze = this.props.isSmallScreenWidth && this.props.isDrawerOpen;
+        if(!this.state.freeze && shouldEnableFreeze){
+            setTimeout(()=>{
+                this.setState({freeze:true})
+            },100)
+        }else if(this.state.freeze && !shouldEnableFreeze){
+            this.setState({freeze:false})
+        }
        
        if (this.props.route.params.reportID === prevProps.route.params.reportID) {
            return;
        }

...

            <Freeze
-                freeze={this.props.isSmallScreenWidth && this.props.isDrawerOpen}
+                freeze={this.state.freeze}

Solution 2:

I'll create freeze state in ReportActionsView and ReportScreen.

In ReportActionsView
I just freeze ReportActionsView when its props is updated, then update freeze in ReportScreen to be true.

        this.state = {
            isFloatingMessageCounterVisible: false,
            newMarkerSequenceNumber: ReportUtils.isUnread(props.report)
                ? props.report.lastReadSequenceNumber + 1
                : 0,
+            freeze:false
        };
...
+    componentDidUpdate(prevProps) {
...
+       const shouldEnableFreeze = this.props.isSmallScreenWidth && this.props.isDrawerOpen;
+        if(!this.state.freeze && shouldEnableFreeze){
+            this.setState({freeze:true})
+            this.props.enableFreeze(true)
+        }else if(this.state.freeze && !shouldEnableFreeze){
+            this.setState({freeze:false})            
+        }
}

In ReportScreen
When user opens any chat I'll update freeze to be false

        this.state = {
            skeletonViewContainerHeight: reportActionsListViewHeight,
            viewportOffsetTop: 0,
            isBannerVisible: true,
+           freeze:false
        };

...

 componentDidUpdate(prevProps) {
+        const shouldEnableFreeze = this.props.isSmallScreenWidth && this.props.isDrawerOpen;
+        if(this.state.freeze && !shouldEnableFreeze){
+            this.setState({freeze:false})            
+       }
Screen.Recording.2022-11-01.at.20.10.21.mp4

@melvin-bot melvin-bot bot added the Overdue label Nov 3, 2022
@conorpendergrast
Copy link
Contributor

@rushatgabhane Over to you, for the proposal review!

@melvin-bot melvin-bot bot removed the Overdue label Nov 3, 2022
@conorpendergrast
Copy link
Contributor

Oh wait this needs to be reproduced on production too!

@rushatgabhane
Copy link
Member

rushatgabhane commented Nov 4, 2022

@flodnv i don't know if we're fixing the root cause in solution 2.

Are we using the Freeze component as intended?

Is there a better solution to abstract this logic? Otherwise we'll have to keep doing this whenever Freeze component is used

@melvin-bot melvin-bot bot added the Overdue label Nov 7, 2022
@flodnv
Copy link
Contributor

flodnv commented Nov 7, 2022

FWIW, this issue also seems to affect Android (I've tested on my phone)

I'll be honest, I don't really understand the problem. @tienifr can you please edit your problem statement description to be more readable (by someone who doesn't know anything)? Or @rushatgabhane if you understand the problem, can you rephrase it in your own words?

@melvin-bot melvin-bot bot removed the Overdue label Nov 7, 2022
@tienifr
Copy link
Contributor

tienifr commented Nov 7, 2022

@flodnv I just follow the lib docs here: https://www.npmjs.com/package/react-freeze
Screen Shot 2022-11-07 at 18 07 54
If freeze set to true, all renders for components from Freeze subtree will be suspended => ReportActionsView can't know the props changes (isDrawerOpen)

<ReportActionsView
reportActions={this.props.reportActions}
report={this.props.report}
session={this.props.session}
isComposerFullSize={this.props.isComposerFullSize}
isDrawerOpen={this.props.isDrawerOpen}
/>

=> This function:

getIsReportFullyVisible() {
const isSidebarCoveringReportView = this.props.isSmallScreenWidth && this.props.isDrawerOpen;
return Visibility.isVisible() && !isSidebarCoveringReportView;
}

still use the stale isDrawerOpen prop is false (that should be true when user click any chat) => getIsReportFullyVisible returns false.

So when user switches tabs, event onVisibilityChange is executed, this.openReportIfNecessary(); will be run => update unread state.

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});
});

@tienifr
Copy link
Contributor

tienifr commented Nov 7, 2022

@flodnv pls let me know if it's still hard to understand. Sorry about that, It's quite hard for me to explain this case

@flodnv
Copy link
Contributor

flodnv commented Nov 7, 2022

Ok, what I am not following is why the unread state isn't persisted then (If I close my app and reopen, it's still not marked as unread)? Is the API not getting called because something is frozen?

@tienifr
Copy link
Contributor

tienifr commented Nov 8, 2022

The API is still getting called but the props/states of frozen component is not updated

@flodnv flodnv changed the title [HOLD on #11768] Bug: Mark as unread state vanishes after closing the app in IOS app @chauchausoup [HOLD on #15212] Bug: Mark as unread state vanishes after closing the app in IOS app @chauchausoup May 22, 2023
@flodnv
Copy link
Contributor

flodnv commented May 22, 2023

This is also on HOLD for #15212

@melvin-bot melvin-bot bot removed the Overdue label May 22, 2023
@melvin-bot melvin-bot bot added the Overdue label Jun 23, 2023
@flodnv
Copy link
Contributor

flodnv commented Jun 26, 2023

Progress is happening in #15212

@melvin-bot melvin-bot bot removed the Overdue label Jun 26, 2023
@melvin-bot melvin-bot bot added the Overdue label Jul 27, 2023
@flodnv flodnv added Monthly KSv2 and removed Monthly KSv2 labels Jul 28, 2023
@melvin-bot melvin-bot bot removed the Overdue label Jul 28, 2023
@melvin-bot melvin-bot bot added the Overdue label Aug 28, 2023
@flodnv flodnv added Monthly KSv2 and removed Monthly KSv2 labels Aug 28, 2023
@melvin-bot melvin-bot bot removed the Overdue label Aug 28, 2023
@melvin-bot melvin-bot bot added the Overdue label Sep 29, 2023
@flodnv
Copy link
Contributor

flodnv commented Oct 4, 2023

Still waiting on continued progress in #15212

@melvin-bot melvin-bot bot removed the Overdue label Oct 4, 2023
@JmillsExpensify
Copy link

@chauchausoup do you mind re-testing this issue using your reproduction steps. I think we're to the point where this might now be fixed. Please share a video, similar to the original issue post.

@JmillsExpensify
Copy link

Waiting for re-test, same as above.

@MonilBhavsar
Copy link
Contributor

This issue should be fixed! We can retest and most probably close it

@flodnv
Copy link
Contributor

flodnv commented Nov 21, 2023

@kavimuru are you able to retest this to confirm please?

@JmillsExpensify
Copy link

That sounds good. Or @chauchausoup if you're free, please re-test and include a video and we can make sure you're paid out! Thanks.

@JmillsExpensify
Copy link

Same as above.

@chauchausoup
Copy link

Sorry guys for the delayed reply. I will retest the issue now.

Copy link

melvin-bot bot commented Dec 1, 2023

📣 @chauchausoup! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

  1. Make sure you've read and understood the contributing guidelines.
  2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
    Screen Shot 2022-11-16 at 4 42 54 PM
    Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

@JmillsExpensify
Copy link

Still waiting on confirmation.

@flodnv flodnv closed this as completed Jan 15, 2024
@melvin-bot melvin-bot bot removed the Overdue label Jan 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Monthly KSv2
Projects
Development

No branches or pull requests