[HOLD for payment 2022-10-31] [Bug] [$250] Desktop - Chat- New marker doesn't disappear after App minimize/restore

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Issue found when executing PR https://github.com/Expensify/App/pull/10582

Action Performed:

1. Run NewDot App
2. Open cha with unread message
3. Minimize/Restore
4. Check the chat

Expected Result:

The new marker and new message badge both disappear

Actual Result:

New marker is not disappeared after minimize/restore

Workaround:

Unknown

Platform:

Where is this issue occurring?

• Desktop App

Version Number: 1.1.96.0

Reproducible in staging?: Yes

Reproducible in production?: Yes

Email or phone of affected tester (no customers): any

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

Notes/Photos/Videos: Any additional supporting documentation

https://user-images.githubusercontent.com/93399543/188027791-3431cac2-889a-448a-bafa-b4be7a88adeb.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

[melvin-bot[bot]]

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

[melvin-bot[bot]]

@yuwenmemon Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[melvin-bot[bot]]

@yuwenmemon Eep! 4 days overdue now. Issues have feelings too...

[melvin-bot[bot]]

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

[Christinadobrzyn]

Created job in Upwork

External job posting - https://www.upwork.com/jobs/~01452789e88a3dd322 Internal job posting - https://www.upwork.com/ab/applicants/1567904682697019392/job-details

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[andrewlor]

Proposal

Context

ReportActionsView.js is listening to the change event on AppState to update the newMarkerSequenceNumber (essentially where the new marker should be displayed).

https://github.com/Expensify/App/blob/d3120758546153d25002866f6cd770d6b2b123c2/src/pages/home/report/ReportActionsView.js#L103-L115

However, it will not do so unless this.getIsReportFullyVisible() is true.

https://github.com/Expensify/App/blob/d3120758546153d25002866f6cd770d6b2b123c2/src/pages/home/report/ReportActionsView.js#L265-L271

This relies on the implementation of Visibility.isVisible, which is where the behaviour diverges between Electron and web.

https://github.com/Expensify/App/blob/d3120758546153d25002866f6cd770d6b2b123c2/src/libs/Visibility/index.js#L12-L16

Problem

The problem is that Electron's BrowserWindow.isFocused() is false after AppState changes to active. I found this out by debugging with these logs:

        this.appStateChangeListener = AppState.addEventListener('change', (state) => {
            console.log(`AppState: ${state}`);
            console.log(`Visibility.isVisible(): ${Visibility.isVisible()}`);
            ...
        }

https://user-images.githubusercontent.com/8475244/189752556-169f0659-f1eb-497e-9f56-70684c60938b.mov

However, document.visibilityState === 'visible' is true when AppState changes to active, which is why the browser implementation works.

Solution

Modify Visibility.isVisible() to only use BrowserWindow.isFocused() if the AppState is not active:

function isVisible() {
    return window.electron && AppState.currentState !== 'active'
        ? window.electron.sendSync(ELECTRON_EVENTS.REQUEST_VISIBILITY)
        : document.visibilityState === 'visible';
}

Reasoning

When the AppState is active, we want to use the regular document.visibilityState behaviour.

The only concern is this comment:

Electron supports document.visibilityState, but switching to another app while Electron is partially occluded will not trigger a state of hidden so we ask the main process synchronously whether the BrowserWindow.isFocused()

However the AppState should not be active in this case (because we're switching to another app), so my modification shouldn't have any behaviour changes in this case.

Result

https://user-images.githubusercontent.com/8475244/189756606-c447325a-4c76-42a2-aca0-cef1eddef67c.mov

[rushatgabhane]

@thienlnam I like @andrewlor's proposal. I couldn't find any flaws, and the root cause is really well explained.

C+ reviewed 🎀 👀 🎀

Side note: I think we should refactor isVisible() to use ifelse for better readability

[andrewlor]

Thanks @rushatgabhane. For the refactor, were you thinking something like this?

function isVisible() {
    if (window.electron && AppState.currentState !== 'active') {
        return window.electron.sendSync(ELECTRON_EVENTS.REQUEST_VISIBILITY);
    }

    return document.visibilityState === 'visible';
}

[rushatgabhane]

yepp precisely!

[Christinadobrzyn]

Hey @rushatgabhane we're good to move forward with hiring for this job?

[rushatgabhane]

@Christinadobrzyn yes, just waiting for @thienlnam's review as well

https://github.com/Expensify/App/issues/10767#issuecomment-1245783562

[thienlnam]

Sorry, was OOO but this proposal looks great 🟢 here

[Christinadobrzyn]

Hired @andrewlor for the fix and @rushatgabhane as C+ in Upwork - https://www.upwork.com/ab/applicants/1567904682697019392/job-details

[marcaaron]

Reasoning When the AppState is active, we want to use the regular document.visibilityState behaviour.

The only concern is this comment:

Electron supports document.visibilityState, but switching to another app while Electron is partially occluded will not trigger a state of hidden so we ask the main process synchronously whether the BrowserWindow.isFocused()

However the AppState should not be active in this case (because we're switching to another app), so my modification shouldn't have any behaviour changes in this case.

Coming in late here, but I am not 100% satisfied with this explanation or the proposed solution.

We are failing to answer this question:

Why does BrowserWindow.isFocused() not return the correct state?

Seems like you've proven that it can happen. But have not provided any reason as to why it happens?

[thienlnam]

I should have caught this in the review stage but I asked about it yesterday when reviewing the PR as well https://github.com/Expensify/App/pull/11150#discussion_r975824111

Please post an updated solution / explanation here @andrewlor

[marcaaron]

Nice, thanks @thienlnam !

[andrewlor]

@thienlnam @marcaaron After looking into this more, it seems like it's the order of events.

I believe BrowserWindow.isFocused() is correct, but it is false immediately after AppState becomes active because the window isn't actually focused yet. It instead becomes focused after the transition period when the window is expanding (the 1-2 seconds where the window is expanding through an animation, might be machine/os dependent how long that is).

AppState.addEventListener('change', () => {
    // When AppState becomes active after expanding a minimized window
    console.log(Visibility.isVisible()) // false
    setTimeout(() => console.log(Visibility.isVisible()), 50); // false
    setTimeout(() => console.log(Visibility.isVisible()), 2000); // true
})

This makes sense to me that a window shouldn't be focused while it's in the process of expanding from a minimized state, but instead when the transition is completed and the window is fully expanded.

I'll continue working and propose a modification to my solution.

[marcaaron]

Random thought.. but if electron has it's own sense for what is in the "background" or "focused" then maybe the chat page should use:

web: AppState listener mobile: AppState listener desktop: Some kind of similar functionality, but based on the electron API like these different events

Same as Visibility.isVisible() switches to request the focus when we are in electron - probably the replacement for AppState listener would do the same.

[andrewlor]

@thienlnam I just pushed changes to my PR implementing @marcaaron's idea https://github.com/Expensify/App/issues/10767#issuecomment-1254354995. Have a look guys and let me know if I understood it correctly. I just figured it was easier to push the changes to the PR rather than copy and paste a bunch of code into a comment here.

[thienlnam]

PR is in final stages of review!

[thienlnam]

Same as above

[mvtglobally]

Issue not reproducible during KI retests. (First week)

[thienlnam]

Regression hold before payment

[mvtglobally]

Issue not reproducible during KI retests. (Second week)

[melvin-bot[bot]]

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

• https://github.com/Expensify/App/pull/11150

If no regressions arise, payment will be issued on 2022-10-31. :confetti_ball:

[andrewlor]

While we're waiting for the regression period before payment, I'd like to request consideration for an increase in compensation. I feel like $1000 would be more appropriate for the work I put in, for a couple of reasons:

• I implemented a high quality and well explained solution. This involved lots of back and forth during the proposal/review stage to arrive at a solution that everyone is happy with.
• Aside from the intended bug fix, the solution makes improvements to the codebase that can be used/benefited from going forward. Eg. creating new desktop implementation for libs/Visibility, adding onVisibilityChange to all Visibility implementations, adding focus/blur channels to electron, error message cleanup in electron contextBridge.

Thanks to @thienlnam @marcaaron and @rushatgabhane for working on this with me, I received a lot of good feedback and guidance and would have not reached as high of quality solution without them. But I feel like my efforts here deserve a reconsideration in compensation and would also be greatly appreciated. Let me know what you think.

[Christinadobrzyn]

I'm going to be ooo on 10/31 so assigning to another teammate.

Here's the Upwork job for the next person- https://www.upwork.com/ab/applicants/1567904682697019392/job-details

[melvin-bot[bot]]

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