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 2 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
12 changes: 8 additions & 4 deletions src/pages/home/ReportScreen.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import DragAndDropProvider from '../../components/DragAndDrop/Provider';
import usePrevious from '../../hooks/usePrevious';
import CONST from '../../CONST';
import withCurrentReportID, {withCurrentReportIDPropTypes, withCurrentReportIDDefaultProps} from '../../components/withCurrentReportID';
import {useIsFocused, useFocusEffect} from '@react-navigation/native';

const propTypes = {
/** Navigation route context info provided by react navigation */
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,17 @@ function ReportScreen({
Report.openReport(report.reportID);
});

fetchReportIfNeeded();
ComposerActions.setShouldShowComposeInput(true);
return () => {
if (!unsubscribeVisibilityListener) {
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (!unsubscribeVisibilityListener) {
return;
}

unsubscribeVisibilityListener();
};
}
}, []))

useEffect(() => {
fetchReportIfNeeded();
ComposerActions.setShouldShowComposeInput(true);
// 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