Remove visibilityListeners when report gets unfocused #27642

staszekscp · 2023-09-18T06:55:08Z

Details

Currently on web and desktop there was an issue with setting multiple event listeners for report visibility- it was related to the fact that reports were not being unmounted at all, so the cleanup function that was supposed to remove listeners was not called. This PR attaches listeners-handling logic to useFocusedEffect hook, and tightly links visibilityListener to the focus of a screen.

Fixed Issues

$ #27502
PROPOSAL:
#27502 (comment)

Tests

Set listeners to see if the listener is created and removed correctly
Verify that no errors appear in the JS console (I noticed a hover error in the console after merging the newest main, but it seems to be unrelated to the PR)

Offline tests

N/A

QA Steps

See if opening and navigating across reports works correctly (there shouldn't be any logic change, the PR is related to event listeners, and the impact of it shouldn't be directly visible, there will be some performance gain when opening a huge number of reports, but it is not the main issue that causes performance problems on web/desktop)
Verify that no errors appear in the JS console (I noticed a hover error in the console after merging the newest main, but it seems to be unrelated to the PR)

PR Author Checklist

Screenshots/Videos

Web

Screen.Recording.2023-09-18.at.08.25.31.mov

Mobile Web - Chrome

Screen.Recording.2023-09-18.at.09.47.32.mov

Mobile Web - Safari

https://github.com/Expensify/App/assets/75020057/1e44c161-9e17-4c16-b199-da1ae90cd508

Desktop

Screen.Recording.2023-09-18.at.09.56.39.mov

iOS

Screen.Recording.2023-09-18.at.11.03.26.mov

Android

Screen.Recording.2023-09-18.at.11.10.07.mov

melvin-bot · 2023-09-18T06:55:12Z

@parasharrajat Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

parasharrajat · 2023-09-18T07:06:19Z

src/pages/home/ReportScreen.js

    useEffect(() => {
-        const unsubscribeVisibilityListener = Visibility.onVisibilityChange(() => {
-            const isTopMostReportID = Navigation.getTopmostReportId() === getReportID(route);
-            // If the report is not fully visible (AKA on small screen devices and LHR is open) or the report is optimistic (AKA not yet created)
-            // we don't need to call openReport
-            if (!getIsReportFullyVisible(isTopMostReportID) || report.isOptimisticReport) {
+        if (isFocused) {
+            unsubscribeVisibilityListener.current = Visibility.onVisibilityChange(() => {
+                const isTopMostReportID = Navigation.getTopmostReportId() === getReportID(route);
+                // If the report is not fully visible (AKA on small screen devices and LHR is open) or the report is optimistic (AKA not yet created)
+                // we don't need to call openReport
+                if (!getIsReportFullyVisible(isTopMostReportID) || report.isOptimisticReport) {
+                    return;
+                }
+
+                Report.openReport(report.reportID);
+            });
+        } else {
+            if (!unsubscribeVisibilityListener.current) {
                return;
            }
+            unsubscribeVisibilityListener.current();
+        }
+    }, [isFocused]);


let's use useFocusEffect hook instead.

Yep, I'm changing that right now

parasharrajat · 2023-09-18T07:17:02Z

src/pages/home/ReportScreen.js

            if (!unsubscribeVisibilityListener) {
                return;
            }


Suggested change

if (!unsubscribeVisibilityListener) {

return;

}

parasharrajat · 2023-09-18T07:27:45Z

src/pages/home/ReportScreen.js

@@ -271,14 +272,14 @@ function ReportScreen({
            Report.openReport(report.reportID);
        });

+        return () => unsubscribeVisibilityListener();
+        // The effect should run only on the first focus to attach listener


But this hook will run on each focus/blur event not just focus so this comment is misleading.

Not really, according to the documentation:

The useFocusEffect is analogous to React's useEffect hook. The only difference is that it only runs if the screen is currently focused.

The effect will run whenever the dependencies passed to React.useCallback change, i.e. it'll run on initial render (if the screen is focused) as well as on subsequent renders if the dependencies have changed. If you don't wrap your effect in React.useCallback, the effect will run every render if the screen is focused.

I mean to say that useFocusEffect will run on each focus to the screen. It will remove the handler on each blur as well.

So visibility event is being attached to each focus event.

Screen.Recording.2023-09-18.at.1.18.36.PM.mov

danieldoglas · 2023-09-18T07:28:56Z

src/pages/home/ReportScreen.js

@@ -259,7 +260,7 @@ function ReportScreen({
        [route],
    );

-    useEffect(() => {
+    useFocusEffect(useCallback(() => {
        const unsubscribeVisibilityListener = Visibility.onVisibilityChange(() => {
            const isTopMostReportID = Navigation.getTopmostReportId() === getReportID(route);


do we still need this logic if we're unsubscribing when we lose focus?

I didn't understand the question. Can you please rephrase?

since we will unsubscribe from the event when the component loses focus, do we need to check if the isTopMostReport? Like, this should only be called from the top most report

~~I guess unsubscribe happens regardless of the login inside that handler. Logic is only run when the visibility of the screen changes while the screen is focused.~~

Ok, now I understand it.

Yeah, I was considering that, but I decided to take the safer approach, since the Visibility lib is being used in some of the most crucial components of the app like Expensify.js, Form.js or TextInput.js

parasharrajat · 2023-09-18T07:29:09Z

You will also have to run prettier for linting

parasharrajat · 2023-09-18T08:10:48Z

I noticed that sometimes the openReport call is made multiple times not necessarily from this effect. Check the video from 0.15 sec.

Screen.Recording.2023-09-18.at.1.35.52.PM.mov

staszekscp · 2023-09-18T08:43:34Z

I noticed that sometimes the openReport call is made multiple times not necessarily from this effect. Check the video from 0.15 sec.

Do you know if this behaviour existed before? I think that the logic change in this PR wouldn't be the reason for those multiple requests, I'll have a look when I finish making videos if that's reproducible on main

staszekscp · 2023-09-18T09:23:05Z

I see that it is reproducible on main too, so this bug doesn't introduce a bug, however there is something going on. I think it may be related to react-native-onyx, because it happens only when one quickly switches between reports, which have not been opened before.

parasharrajat · 2023-09-18T09:34:38Z

I don't think it is related to changes in this PR. but that might be something to look at. I thought as we are targeting openreport api calls, we might be interested to fix this issue as well.

mountiny · 2023-09-18T09:41:57Z

Thanks for jumping onto this one so quickly

staszekscp · 2023-09-18T09:48:33Z

Agreed @parasharrajat. I can take care of that, but could we do that as a separate issue? The thing is that I have bronchitis, and I don't feel well, I just hoped to help with this task, cause I had an idea how to fix it, and on Friday I had promised to take care of that... So I could pick it up again on Thursday/Friday or (more probably) next week.

mountiny · 2023-09-18T10:25:58Z

I agree to keep this separate and I insist @staszekscp takes time off

parasharrajat · 2023-09-18T10:40:30Z

That works for me. Finishing off the checklist.

parasharrajat · 2023-09-18T11:40:55Z

Screenshots

🔲 iOS / native

Screen.Recording.2023-09-18.at.4.56.42.PM.mov

🔲 iOS / Safari

Screen.Recording.2023-09-18.at.5.09.17.PM.mov

🔲 MacOS / Desktop

Screen.Recording.2023-09-18.at.1.35.52.PM.mov

🔲 MacOS / Chrome

Screen.Recording.2023-09-18.at.1.30.46.PM.mov

🔲 Android / Chrome

Screen.Recording.2023-09-18.at.5.05.38.PM.mov

🔲 Android / native

Screen.Recording.2023-09-18.at.4.55.32.PM.mov

parasharrajat

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

🎀 👀 🎀 C+ reviewed

mountiny

Thanks for the changes and checklist

OSBotify · 2023-09-19T04:34:36Z

✋ 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 · 2023-09-20T08:45:33Z

🚀 Deployed to staging by https://github.com/danieldoglas in version: 1.3.72-0 🚀

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

OSBotify · 2023-09-22T13:27:28Z

🚀 Deployed to production by https://github.com/thienlnam in version: 1.3.72-11 🚀

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

remove visibilityListeners when report gets unfocused

47a0d69

staszekscp requested a review from a team as a code owner September 18, 2023 06:55

melvin-bot bot requested review from parasharrajat and removed request for a team September 18, 2023 06:55

parasharrajat reviewed Sep 18, 2023

View reviewed changes

staszekscp added 2 commits September 18, 2023 09:10

switch to useFocusEffect

afc6c42

removed unused import

432da39

parasharrajat reviewed Sep 18, 2023

View reviewed changes

staszekscp added 2 commits September 18, 2023 09:18

fix lint problems

b876aa0

simplify logic of return

49a32d8

parasharrajat reviewed Sep 18, 2023

View reviewed changes

danieldoglas reviewed Sep 18, 2023

View reviewed changes

prettier

55e9907

mountiny assigned staszekscp Sep 18, 2023

parasharrajat approved these changes Sep 18, 2023

View reviewed changes

melvin-bot bot requested a review from danieldoglas September 18, 2023 11:41

parasharrajat mentioned this pull request Sep 18, 2023

[HOLD for payment 2023-09-29] Find a better solution for removing visibilityListeners when changing the visible reports #27502

Closed

6 tasks

mountiny approved these changes Sep 19, 2023

View reviewed changes

danieldoglas merged commit 3bffd67 into Expensify:main Sep 19, 2023
3 checks passed

OSBotify mentioned this pull request Sep 20, 2023

Deploy Checklist: New Expensify 2023-09-20 #27837

Closed

hoangzinh mentioned this pull request Sep 29, 2023

[HOLD for payment 2023-11-09] [$1000] Web - API OpenReport shows after switch away from the app and back #27887

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove visibilityListeners when report gets unfocused #27642

Remove visibilityListeners when report gets unfocused #27642

staszekscp commented Sep 18, 2023 •

edited

Loading

melvin-bot bot commented Sep 18, 2023

parasharrajat Sep 18, 2023

staszekscp Sep 18, 2023

parasharrajat Sep 18, 2023

parasharrajat Sep 18, 2023

staszekscp Sep 18, 2023

parasharrajat Sep 18, 2023 •

edited

Loading

danieldoglas Sep 18, 2023

parasharrajat Sep 18, 2023 •

edited

Loading

danieldoglas Sep 18, 2023

parasharrajat Sep 18, 2023 •

edited

Loading

staszekscp Sep 18, 2023

parasharrajat commented Sep 18, 2023

parasharrajat commented Sep 18, 2023 •

edited

Loading

staszekscp commented Sep 18, 2023

staszekscp commented Sep 18, 2023

parasharrajat commented Sep 18, 2023

mountiny commented Sep 18, 2023

staszekscp commented Sep 18, 2023

mountiny commented Sep 18, 2023

parasharrajat commented Sep 18, 2023

parasharrajat commented Sep 18, 2023

parasharrajat left a comment

mountiny left a comment

OSBotify commented Sep 19, 2023

OSBotify commented Sep 20, 2023

OSBotify commented Sep 22, 2023

Remove visibilityListeners when report gets unfocused #27642

Remove visibilityListeners when report gets unfocused #27642

Conversation

staszekscp commented Sep 18, 2023 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Sep 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parasharrajat Sep 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parasharrajat Sep 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parasharrajat Sep 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parasharrajat commented Sep 18, 2023

parasharrajat commented Sep 18, 2023 • edited Loading

staszekscp commented Sep 18, 2023

staszekscp commented Sep 18, 2023

parasharrajat commented Sep 18, 2023

mountiny commented Sep 18, 2023

staszekscp commented Sep 18, 2023

mountiny commented Sep 18, 2023

parasharrajat commented Sep 18, 2023

parasharrajat commented Sep 18, 2023

Screenshots

🔲 iOS / native

🔲 iOS / Safari

🔲 MacOS / Desktop

🔲 MacOS / Chrome

🔲 Android / Chrome

🔲 Android / native

parasharrajat left a comment

Choose a reason for hiding this comment

Reviewer Checklist

mountiny left a comment

Choose a reason for hiding this comment

OSBotify commented Sep 19, 2023

OSBotify commented Sep 20, 2023

OSBotify commented Sep 22, 2023

staszekscp commented Sep 18, 2023 •

edited

Loading

parasharrajat Sep 18, 2023 •

edited

Loading

parasharrajat Sep 18, 2023 •

edited

Loading

parasharrajat Sep 18, 2023 •

edited

Loading

parasharrajat commented Sep 18, 2023 •

edited

Loading