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

Jump to bottom

Replace global variables #47076

Merged
merged 14 commits into from
Sep 11, 2024
Merged

Replace global variables #47076

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
5a175d9
fix undefined role route
BrtqKr Aug 8, 2024
0d3cc6e
move search state to onyx wip
BrtqKr Aug 8, 2024
75654cf
apply onyx search persist to the members flow
BrtqKr Aug 8, 2024
f06810d
cleanup
BrtqKr Aug 8, 2024
d6df1ba
remove persist from workspace invite page
BrtqKr Aug 8, 2024
30ff249
cleanup
BrtqKr Aug 8, 2024
a311867
cleanup
BrtqKr Aug 8, 2024
ba8b960
review fixes
BrtqKr Aug 8, 2024
07a3dce
Merge remote-tracking branch 'origin/main' into brtqkr/repalce-global…
BrtqKr Aug 26, 2024
aff91e1
Merge remote-tracking branch 'origin/main' into brtqkr/repalce-global…
BrtqKr Aug 28, 2024
86db79c
cleanup
BrtqKr Aug 28, 2024
902e65c
Merge remote-tracking branch 'origin/main' into brtqkr/repalce-global…
BrtqKr Sep 5, 2024
043056c
Merge remote-tracking branch 'origin/main' into brtqkr/repalce-global…
BrtqKr Sep 10, 2024
dd11582
replace search input manager for the updated screens
BrtqKr Sep 10, 2024
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
4 changes: 4 additions & 0 deletions src/ONYXKEYS.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -400,6 +400,9 @@ const ONYXKEYS = {
/** Stores the information about currently edited advanced approval workflow */
APPROVAL_WORKFLOW: 'approvalWorkflow',

/** Stores the user search value for persistance across the screens */
ROOM_MEMBERS_USER_SEARCH_PHRASE: 'roomMembersUserSearchPhrase',

/** Collection Keys */
COLLECTION: {
DOWNLOAD: 'download_',
Expand Down Expand Up @@ -879,6 +882,7 @@ type OnyxValuesMapping = {
[ONYXKEYS.NVP_WORKSPACE_TOOLTIP]: OnyxTypes.WorkspaceTooltip;
[ONYXKEYS.NVP_PRIVATE_CANCELLATION_DETAILS]: OnyxTypes.CancellationDetails[];
[ONYXKEYS.APPROVAL_WORKFLOW]: OnyxTypes.ApprovalWorkflow;
[ONYXKEYS.ROOM_MEMBERS_USER_SEARCH_PHRASE]: string;
};

type OnyxValues = OnyxValuesMapping & OnyxCollectionValuesMapping & OnyxFormValuesMapping & OnyxFormDraftValuesMapping;
Expand Down
5 changes: 4 additions & 1 deletion src/ROUTES.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,7 +355,10 @@ const ROUTES = {
},
ROOM_INVITE: {
route: 'r/:reportID/invite/:role?',
getRoute: (reportID: string, role?: string) => `r/${reportID}/invite/${role}` as const,
getRoute: (reportID: string, role?: string) => {
const route = role ? (`r/${reportID}/invite/${role}` as const) : (`r/${reportID}/invite` as const);
Copy link
Contributor

Choose a reason for hiding this comment

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

How about a shorter version?

Suggested change
const route = role ? (`r/${reportID}/invite/${role}` as const) : (`r/${reportID}/invite` as const);
getRoute: (reportID: string, role?: string) => `r/${reportID}/invite${role ? `/${role}`: ''}` as const,

Copy link
Contributor

Choose a reason for hiding this comment

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

@BrtqKr Could you please help to detail which problem here?

Fixed role undefined case on room members invite

Copy link
Contributor Author

@BrtqKr BrtqKr Aug 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

when someone didn't have a role inside of a report we ended up with /undefined inside of the url. It just seems like someone forgot to put a proper fallback in there

Copy link
Contributor

Choose a reason for hiding this comment

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

@BrtqKr With previous code, we added a fallback value is empty string. Why shouldn't use ?? operator?

Copy link
Contributor Author

@BrtqKr BrtqKr Aug 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure what you meant exactly, but two explanations:
1.
getRoute: (reportID: string, role?: string) =>r/${reportID}/invite/${role} as const,

In the previous version, skipping the role meant getting undefined, which was getting stringified, which resulted in for example
r/151422415264937/invite/undefined in the url.

?? operator works only as a fallback for null and undefined and in this case we want to include empty string in the same group to remove slash for this scenario

return route;
},
},
MONEY_REQUEST_HOLD_REASON: {
route: ':type/edit/reason/:transactionID?',
Expand Down
3 changes: 0 additions & 3 deletions src/libs/Navigation/AppNavigator/AuthScreens.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ import * as SessionUtils from '@libs/SessionUtils';
import ConnectionCompletePage from '@pages/ConnectionCompletePage';
import NotFoundPage from '@pages/ErrorPage/NotFoundPage';
import DesktopSignInRedirectPage from '@pages/signin/DesktopSignInRedirectPage';
import SearchInputManager from '@pages/workspace/SearchInputManager';
import * as App from '@userActions/App';
import * as Download from '@userActions/Download';
import * as Modal from '@userActions/Modal';
Expand Down Expand Up @@ -174,8 +173,6 @@ const modalScreenListeners = {
Modal.setModalVisibility(false);
},
beforeRemove: () => {
// Clear search input (WorkspaceInvitePage) when modal is closed
SearchInputManager.searchInput = '';
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we clear it here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, because we've removed the usage of persist on that page. Apparently navigation patterns have changed and this is no longer necessary.

Modal.setModalVisibility(false);
Modal.willAlertModalBecomeVisible(false);
},
Expand Down
15 changes: 15 additions & 0 deletions src/libs/actions/RoomMembersUserSearchPhrase.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import Onyx from 'react-native-onyx';
import ONYXKEYS from '@src/ONYXKEYS';

function clearUserSearchPhrase() {
Onyx.set(ONYXKEYS.ROOM_MEMBERS_USER_SEARCH_PHRASE, '');
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use merge here as well

}

/**
* Persists user search phrase from the serch input across the screens.
*/
function updateUserSearchPhrase(value: string) {
Onyx.merge(ONYXKEYS.ROOM_MEMBERS_USER_SEARCH_PHRASE, value);
}

export {clearUserSearchPhrase as clearUserSearchValue, updateUserSearchPhrase};
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
export {clearUserSearchPhrase as clearUserSearchValue, updateUserSearchPhrase};
export {clearUserSearchPhrase, updateUserSearchPhrase};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

leftover, sorry

15 changes: 6 additions & 9 deletions src/pages/RoomInvitePage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import useDebouncedState from '@hooks/useDebouncedState';
import useLocalize from '@hooks/useLocalize';
import useThemeStyles from '@hooks/useThemeStyles';
import * as ReportActions from '@libs/actions/Report';
import * as UserSearchPhraseActions from '@libs/actions/RoomMembersUserSearchPhrase';
import {READ_COMMANDS} from '@libs/API/types';
import * as DeviceCapabilities from '@libs/DeviceCapabilities';
import HttpUtils from '@libs/HttpUtils';
Expand All @@ -38,7 +39,6 @@ import type {Policy} from '@src/types/onyx';
import {isEmptyObject} from '@src/types/utils/EmptyObject';
import type {WithReportOrNotFoundProps} from './home/report/withReportOrNotFound';
import withReportOrNotFound from './home/report/withReportOrNotFound';
import SearchInputManager from './workspace/SearchInputManager';

type RoomInvitePageProps = WithReportOrNotFoundProps & WithNavigationTransitionEndProps & StackScreenProps<RoomInviteNavigatorParamList, typeof SCREENS.ROOM_INVITE_ROOT>;

Expand All @@ -53,15 +53,12 @@ function RoomInvitePage({
}: RoomInvitePageProps) {
const styles = useThemeStyles();
const {translate} = useLocalize();
const [searchTerm, debouncedSearchTerm, setSearchTerm] = useDebouncedState('');
const [userSearchPhrase] = useOnyx(ONYXKEYS.ROOM_MEMBERS_USER_SEARCH_PHRASE);
const [searchTerm, debouncedSearchTerm, setSearchTerm] = useDebouncedState(userSearchPhrase ?? '');
const [selectedOptions, setSelectedOptions] = useState<ReportUtils.OptionData[]>([]);
const [isSearchingForReports] = useOnyx(ONYXKEYS.IS_SEARCHING_FOR_REPORTS, {initWithStoredValues: false});
const {options, areOptionsInitialized} = useOptionsList();

useEffect(() => {
setSearchTerm(SearchInputManager.searchInput);
// eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps
}, []);
const {options, areOptionsInitialized} = useOptionsList();

// Any existing participants and Expensify emails should not be eligible for invitation
const excludedUsers = useMemo(() => {
Expand Down Expand Up @@ -207,7 +204,7 @@ function RoomInvitePage({
if (reportID) {
Report.inviteToRoom(reportID, invitedEmailsToAccountIDs);
}
SearchInputManager.searchInput = '';
UserSearchPhraseActions.clearUserSearchValue();
Navigation.navigate(backRoute);
}, [selectedOptions, backRoute, reportID, validate]);

Expand Down Expand Up @@ -239,6 +236,7 @@ function RoomInvitePage({
}, [debouncedSearchTerm, inviteOptions.userToInvite, inviteOptions.personalDetails, excludedUsers, translate, reportName]);

useEffect(() => {
UserSearchPhraseActions.updateUserSearchPhrase(debouncedSearchTerm);
ReportActions.searchInServer(debouncedSearchTerm);
}, [debouncedSearchTerm]);

Expand All @@ -265,7 +263,6 @@ function RoomInvitePage({
textInputLabel={translate('selectionList.nameEmailOrPhoneNumber')}
textInputValue={searchTerm}
onChangeText={(value) => {
SearchInputManager.searchInput = value;
setSearchTerm(value);
}}
headerMessage={headerMessage}
Expand Down
19 changes: 12 additions & 7 deletions src/pages/RoomMembersPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import {useIsFocused} from '@react-navigation/native';
import type {StackScreenProps} from '@react-navigation/stack';
import React, {useCallback, useEffect, useMemo, useState} from 'react';
import {View} from 'react-native';
import {withOnyx} from 'react-native-onyx';
import {useOnyx, withOnyx} from 'react-native-onyx';
import type {OnyxEntry} from 'react-native-onyx';
import FullPageNotFoundView from '@components/BlockingViews/FullPageNotFoundView';
import Button from '@components/Button';
Expand All @@ -16,8 +16,10 @@ import type {ListItem} from '@components/SelectionList/types';
import UserListItem from '@components/SelectionList/UserListItem';
import type {WithCurrentUserPersonalDetailsProps} from '@components/withCurrentUserPersonalDetails';
import withCurrentUserPersonalDetails from '@components/withCurrentUserPersonalDetails';
import useDebouncedState from '@hooks/useDebouncedState';
import useLocalize from '@hooks/useLocalize';
import useThemeStyles from '@hooks/useThemeStyles';
import * as UserSearchPhraseActions from '@libs/actions/RoomMembersUserSearchPhrase';
import * as DeviceCapabilities from '@libs/DeviceCapabilities';
import localeCompare from '@libs/LocaleCompare';
import Log from '@libs/Log';
Expand All @@ -37,7 +39,6 @@ import type {Session} from '@src/types/onyx';
import {isEmptyObject} from '@src/types/utils/EmptyObject';
import type {WithReportOrNotFoundProps} from './home/report/withReportOrNotFound';
import withReportOrNotFound from './home/report/withReportOrNotFound';
import SearchInputManager from './workspace/SearchInputManager';

type RoomMembersPageOnyxProps = {
session: OnyxEntry<Session>;
Expand All @@ -53,7 +54,8 @@ function RoomMembersPage({report, session, policies}: RoomMembersPageProps) {
const {formatPhoneNumber, translate} = useLocalize();
const [selectedMembers, setSelectedMembers] = useState<number[]>([]);
const [removeMembersConfirmModalVisible, setRemoveMembersConfirmModalVisible] = useState(false);
const [searchValue, setSearchValue] = useState('');
const [userSearchPhrase] = useOnyx(ONYXKEYS.ROOM_MEMBERS_USER_SEARCH_PHRASE);
const [searchValue, debouncedSearchTerm, setSearchValue] = useDebouncedState('');
const [didLoadRoomMembers, setDidLoadRoomMembers] = useState(false);
const personalDetails = usePersonalDetails() || CONST.EMPTY_OBJECT;
const policy = useMemo(() => policies?.[`${ONYXKEYS.COLLECTION.POLICY}${report?.policyID ?? ''}`], [policies, report?.policyID]);
Expand All @@ -62,16 +64,20 @@ function RoomMembersPage({report, session, policies}: RoomMembersPageProps) {
const isFocusedScreen = useIsFocused();

useEffect(() => {
setSearchValue(SearchInputManager.searchInput);
}, [isFocusedScreen]);
setSearchValue(userSearchPhrase ?? '');
}, [isFocusedScreen, setSearchValue, userSearchPhrase]);

useEffect(
() => () => {
SearchInputManager.searchInput = '';
UserSearchPhraseActions.clearUserSearchValue();
},
[],
);

useEffect(() => {
UserSearchPhraseActions.updateUserSearchPhrase(debouncedSearchTerm);
}, [debouncedSearchTerm]);

/**
* Get members for the current room
*/
Expand Down Expand Up @@ -295,7 +301,6 @@ function RoomMembersPage({report, session, policies}: RoomMembersPageProps) {
disableKeyboardShortcuts={removeMembersConfirmModalVisible}
textInputValue={searchValue}
onChangeText={(value) => {
SearchInputManager.searchInput = value;
setSearchValue(value);
}}
headerMessage={headerMessage}
Expand Down
5 changes: 0 additions & 5 deletions src/pages/workspace/SearchInputManager.ts
View file
Open in desktop

This file was deleted.

2 changes: 0 additions & 2 deletions src/pages/workspace/WorkspaceInviteMessagePage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ import INPUT_IDS from '@src/types/form/WorkspaceInviteMessageForm';
import type {InvitedEmailsToAccountIDs, PersonalDetailsList} from '@src/types/onyx';
import {isEmptyObject} from '@src/types/utils/EmptyObject';
import AccessOrNotFoundWrapper from './AccessOrNotFoundWrapper';
import SearchInputManager from './SearchInputManager';
import withPolicyAndFullscreenLoading from './withPolicyAndFullscreenLoading';
import type {WithPolicyAndFullscreenLoadingProps} from './withPolicyAndFullscreenLoading';

Expand Down Expand Up @@ -110,7 +109,6 @@ function WorkspaceInviteMessagePage({
// Please see https://github.com/Expensify/App/blob/main/README.md#Security for more details
Member.addMembersToWorkspace(invitedEmailsToAccountIDsDraft ?? {}, `${welcomeNoteSubject}\n\n${welcomeNote}`, route.params.policyID);
debouncedSaveDraft(null);
SearchInputManager.searchInput = '';
Navigation.dismissModal();
};

Expand Down
3 changes: 0 additions & 3 deletions src/pages/workspace/WorkspaceInvitePage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ import type {Beta, InvitedEmailsToAccountIDs} from '@src/types/onyx';
import type {Errors} from '@src/types/onyx/OnyxCommon';
import {isEmptyObject} from '@src/types/utils/EmptyObject';
import AccessOrNotFoundWrapper from './AccessOrNotFoundWrapper';
import SearchInputManager from './SearchInputManager';
import withPolicyAndFullscreenLoading from './withPolicyAndFullscreenLoading';
import type {WithPolicyAndFullscreenLoadingProps} from './withPolicyAndFullscreenLoading';

Expand Down Expand Up @@ -77,7 +76,6 @@ function WorkspaceInvitePage({route, betas, invitedEmailsToAccountIDsDraft, poli
});

useEffect(() => {
setSearchTerm(SearchInputManager.searchInput);
return () => {
Member.setWorkspaceInviteMembersDraft(route.params.policyID, {});
};
Expand Down Expand Up @@ -338,7 +336,6 @@ function WorkspaceInvitePage({route, betas, invitedEmailsToAccountIDsDraft, poli
textInputLabel={translate('selectionList.nameEmailOrPhoneNumber')}
textInputValue={searchTerm}
onChangeText={(value) => {
SearchInputManager.searchInput = value;
setSearchTerm(value);
}}
headerMessage={headerMessage}
Expand Down
Loading