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.

Sign up for GitHub

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

Remove visibilityListeners when report gets unfocused #27642

Merged
merged 6 commits into from
Sep 19, 2023
Merged

Remove visibilityListeners when report gets unfocused #27642

Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
47a0d69
remove visibilityListeners when report gets unfocused
staszekscp Sep 18, 2023
afc6c42
switch to useFocusEffect
staszekscp Sep 18, 2023
432da39
removed unused import
staszekscp Sep 18, 2023
b876aa0
fix lint problems
staszekscp Sep 18, 2023
49a32d8
simplify logic of return
staszekscp Sep 18, 2023
55e9907
prettier
staszekscp Sep 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions src/pages/home/ReportScreen.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import React, {useRef, useState, useEffect, useMemo, useCallback} from 'react';
import {withOnyx} from 'react-native-onyx';
import {useFocusEffect} from '@react-navigation/native';
import PropTypes from 'prop-types';
import {View} from 'react-native';
import lodashGet from 'lodash/get';
Expand Down Expand Up @@ -259,7 +260,7 @@ function ReportScreen({
[route],
);

useEffect(() => {
useFocusEffect(useCallback(() => {
const unsubscribeVisibilityListener = Visibility.onVisibilityChange(() => {
const isTopMostReportID = Navigation.getTopmostReportId() === getReportID(route);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@parasharrajat parasharrajat Sep 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@parasharrajat parasharrajat Sep 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// 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)
Expand All @@ -271,14 +272,14 @@ function ReportScreen({
Report.openReport(report.reportID);
});

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@parasharrajat parasharrajat Sep 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// eslint-disable-next-line react-hooks/exhaustive-deps
}, []))

useEffect(() => {
fetchReportIfNeeded();
ComposerActions.setShouldShowComposeInput(true);
return () => {
if (!unsubscribeVisibilityListener) {
return;
}
unsubscribeVisibilityListener();
};
// I'm disabling the warning, as it expects to use exhaustive deps, even though we want this useEffect to run only on the first render.
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);
Expand Down