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.

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

Minimal server search / enable improved focus mode #27819

Merged
merged 38 commits into from
Oct 13, 2023
Merged
Show file tree
Hide file tree
Changes from 32 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
4023821
Only search when online
marcaaron Sep 20, 2023
dae32f2
Remove Text
marcaaron Sep 20, 2023
c0e5658
Add missing file
marcaaron Sep 20, 2023
61e4199
Run prettier
marcaaron Sep 20, 2023
0d9a313
Slow down server search
marcaaron Sep 21, 2023
470e323
Merge branch 'main' into marcaaron-minimalSearch
marcaaron Sep 22, 2023
630a6fd
Make NewChatPage also search
marcaaron Sep 25, 2023
b881942
Merge branch 'main' into marcaaron-minimalSearch
marcaaron Sep 28, 2023
a66dbd9
Update translations
marcaaron Sep 28, 2023
8e94c93
Merge branch 'main' into marcaaron-minimalSearch
marcaaron Sep 29, 2023
1bbde1a
Use debounce
marcaaron Sep 29, 2023
ad952ae
Make some requested changes
marcaaron Sep 29, 2023
98724e9
Make requested changes
marcaaron Oct 2, 2023
9a8387a
Fix conflicts
marcaaron Oct 2, 2023
3ab1772
add missing useCallback
marcaaron Oct 3, 2023
3bdf7bb
Merge branch 'main' into marcaaron-minimalSearch
marcaaron Oct 3, 2023
97e5bb1
Change UX so we are showing loading row below options instead of above
marcaaron Oct 3, 2023
af3fec7
Remove unneeded changes. Fix propTypes
marcaaron Oct 3, 2023
ea4a0f1
Undo bad whitespace change
marcaaron Oct 3, 2023
06531cb
undo another whitespace change
marcaaron Oct 3, 2023
95d436b
Fix UI race issue with No results found
marcaaron Oct 3, 2023
16474cd
Fix conflicts
marcaaron Oct 3, 2023
3d10710
Add loadingRow to NewChat page
marcaaron Oct 3, 2023
4c801f7
Clean up
marcaaron Oct 3, 2023
92c0ed4
remove new line
marcaaron Oct 3, 2023
b101e17
Style
marcaaron Oct 4, 2023
007d2d3
Show loading spinner instead of skeleton
marcaaron Oct 4, 2023
4e6826a
Undo skeleton row changes
marcaaron Oct 4, 2023
96400ed
run prettier
marcaaron Oct 4, 2023
15f9492
Call OpenApp when the priorityMode changes
marcaaron Oct 5, 2023
94411ae
Fix loading when offline
marcaaron Oct 6, 2023
3816569
Add constant for debounce time
marcaaron Oct 6, 2023
b09bad3
Fix conflicts and propTypes
marcaaron Oct 10, 2023
5719736
make requested comment change
marcaaron Oct 10, 2023
1ea9f28
Merge branch 'main' into marcaaron-minimalSearch
marcaaron Oct 11, 2023
992ae36
Update src/libs/actions/App.js
marcaaron Oct 12, 2023
352d376
Use withNetwork. propType comment
marcaaron Oct 12, 2023
6df2efa
Merge branch 'marcaaron-minimalSearch' of https://github.com/Expensif…
marcaaron Oct 12, 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
1 change: 1 addition & 0 deletions src/CONST.ts
Original file line number Diff line number Diff line change
Expand Up @@ -664,6 +664,7 @@ const CONST = {
TOOLTIP_SENSE: 1000,
TRIE_INITIALIZATION: 'trie_initialization',
COMMENT_LENGTH_DEBOUNCE_TIME: 500,
SEARCH_FOR_REPORTS_DEBOUNCE_TIME: 300,
},
PRIORITY_MODE: {
GSD: 'gsd',
Expand Down
3 changes: 3 additions & 0 deletions src/ONYXKEYS.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@ const ONYXKEYS = {
/** Boolean flag set whenever the sidebar has loaded */
IS_SIDEBAR_LOADED: 'isSidebarLoaded',

/** Boolean flag set whenever we are searching for reports in the server */
IS_SEARCHING_FOR_REPORTS: 'isSearchingForReports',

/** Note: These are Persisted Requests - not all requests in the main queue as the key name might lead one to believe */
PERSISTED_REQUESTS: 'networkRequestQueue',

Expand Down
5 changes: 4 additions & 1 deletion src/components/OptionsList/BaseOptionsList.js
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ function BaseOptionsList({
isDisabled,
innerRef,
isRowMultilineSupported,
isLoadingNewOptions,
}) {
const flattenedData = useRef();
const previousSections = usePrevious(sections);
Expand Down Expand Up @@ -243,7 +244,9 @@ function BaseOptionsList({
<OptionsListSkeletonView shouldAnimate />
) : (
<>
{headerMessage ? (
{/* If we are loading new options we will avoid showing any header message. This is mostly because one of the header messages says there are no options. */}
{/* This is confusing because we might be in the process of loading fresh options from the server. */}
Beamanator marked this conversation as resolved.
Show resolved Hide resolved
{!isLoadingNewOptions && headerMessage ? (
<View style={[styles.ph5, styles.pb5]}>
<Text style={[styles.textLabel, styles.colorMuted]}>{headerMessage}</Text>
</View>
Expand Down
4 changes: 4 additions & 0 deletions src/components/OptionsList/optionsListPropTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,9 @@ const propTypes = {

/** Whether to wrap large text up to 2 lines */
isRowMultilineSupported: PropTypes.bool,

/** Whether we are loading new options */
isLoadingNewOptions: PropTypes.bool,
};

const defaultProps = {
Expand All @@ -109,6 +112,7 @@ const defaultProps = {
shouldDisableRowInnerPadding: false,
showScrollIndicator: false,
isRowMultilineSupported: false,
isLoadingNewOptions: true,
};

export {propTypes, defaultProps};
10 changes: 10 additions & 0 deletions src/components/OptionsSelector/BaseOptionsSelector.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {propTypes as optionsSelectorPropTypes, defaultProps as optionsSelectorDe
import setSelection from '../../libs/setSelection';
import compose from '../../libs/compose';
import getPlatform from '../../libs/getPlatform';
import FormHelpMessage from '../FormHelpMessage';

const propTypes = {
/** padding bottom style of safe area */
Expand Down Expand Up @@ -380,6 +381,7 @@ class BaseOptionsSelector extends Component {
blurOnSubmit={Boolean(this.state.allOptions.length)}
spellCheck={false}
shouldInterceptSwipe={this.props.shouldTextInputInterceptSwipe}
isLoading={this.props.isLoadingNewOptions}
/>
);
const optionsList = (
Expand Down Expand Up @@ -416,6 +418,7 @@ class BaseOptionsSelector extends Component {
isLoading={!this.props.shouldShowOptions}
showScrollIndicator={this.props.showScrollIndicator}
isRowMultilineSupported={this.props.isRowMultilineSupported}
isLoadingNewOptions={this.props.isLoadingNewOptions}
/>
);
return (
Expand All @@ -440,6 +443,13 @@ class BaseOptionsSelector extends Component {
<View style={this.props.shouldUseStyleForChildren ? [styles.ph5, styles.pb3] : []}>
{this.props.children}
{this.props.shouldShowTextInput && textInput}
{Boolean(this.props.textInputAlert) && (
<FormHelpMessage
message={this.props.textInputAlert}
style={[styles.mb3]}
isError={false}
/>
)}
</View>
{optionsList}
</>
Expand Down
9 changes: 8 additions & 1 deletion src/components/TextInput/BaseTextInput.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import _ from 'underscore';
import React, {useState, useRef, useEffect, useCallback, useMemo} from 'react';
import {Animated, View, StyleSheet} from 'react-native';
import {Animated, View, StyleSheet, ActivityIndicator} from 'react-native';
import Str from 'expensify-common/lib/str';
import RNTextInput from '../RNTextInput';
import TextInputLabel from './TextInputLabel';
Expand Down Expand Up @@ -368,6 +368,13 @@ function BaseTextInput(props) {
// `dataset.submitOnEnter` is used to indicate that pressing Enter on this input should call the submit callback.
dataSet={{submitOnEnter: isMultiline && props.submitOnEnter}}
/>
{props.isLoading && (
<ActivityIndicator
size="small"
color={themeColors.iconSuccessFill}
style={[styles.mt4, styles.ml1]}
/>
)}
{Boolean(props.secureTextEntry) && (
<Checkbox
style={[styles.flex1, styles.textInputIconContainer]}
Expand Down
3 changes: 3 additions & 0 deletions src/languages/en.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1568,6 +1568,9 @@ export default {
screenShare: 'Screen share',
screenShareRequest: 'Expensify is inviting you to a screen share',
},
search: {
resultsAreLimited: 'Search results are limited.',
},
genericErrorPage: {
title: 'Uh-oh, something went wrong!',
body: {
Expand Down
3 changes: 3 additions & 0 deletions src/languages/es.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1591,6 +1591,9 @@ export default {
screenShare: 'Compartir pantalla',
screenShareRequest: 'Expensify te está invitando a compartir la pantalla',
},
search: {
resultsAreLimited: 'Los resultados de búsqueda están limitados.',
},
genericErrorPage: {
title: '¡Uh-oh, algo salió mal!',
body: {
Expand Down
16 changes: 15 additions & 1 deletion src/libs/actions/App.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,19 @@ Onyx.connect({
callback: (val) => (preferredLocale = val),
});

let priorityMode;
Onyx.connect({
key: ONYXKEYS.NVP_PRIORITY_MODE,
callback: (nextPriorityMode) => {
// When someone switches their priority mode we need to fetch all their chats. This is only possible via the OpenApp command.
marcaaron marked this conversation as resolved.
Show resolved Hide resolved
if (nextPriorityMode === CONST.PRIORITY_MODE.DEFAULT && priorityMode === CONST.PRIORITY_MODE.GSD) {
// eslint-disable-next-line no-use-before-define
openApp();
}
priorityMode = nextPriorityMode;
},
});

let resolveIsReadyPromise;
const isReadyToOpenApp = new Promise((resolve) => {
resolveIsReadyPromise = resolve;
Expand Down Expand Up @@ -204,7 +217,8 @@ function getOnyxDataForOpenOrReconnect(isOpenApp = false) {
*/
function openApp() {
getPolicyParamsForOpenOrReconnect().then((policyParams) => {
API.read('OpenApp', policyParams, getOnyxDataForOpenOrReconnect(true));
const params = {enablePriorityModeFilter: true, ...policyParams};
API.read('OpenApp', params, getOnyxDataForOpenOrReconnect(true));
});
}

Expand Down
57 changes: 57 additions & 0 deletions src/libs/actions/Report.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import {InteractionManager} from 'react-native';
import _ from 'underscore';
import lodashGet from 'lodash/get';
import lodashDebounce from 'lodash/debounce';
import ExpensiMark from 'expensify-common/lib/ExpensiMark';
import Onyx from 'react-native-onyx';
import Str from 'expensify-common/lib/str';
Expand Down Expand Up @@ -2167,7 +2168,63 @@ function clearPrivateNotesError(reportID, accountID) {
Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, {privateNotes: {[accountID]: {errors: null}}});
}

/**
* @private
* @param {string} searchInput
*/
function searchForReports(searchInput) {
// We do not try to make this request while offline because it sets a loading indicator optimistically
if (isNetworkOffline) {
Onyx.set(ONYXKEYS.IS_SEARCHING_FOR_REPORTS, false);
return;
}

API.read(
'SearchForReports',
{searchInput},
{
successData: [
{
onyxMethod: Onyx.METHOD.MERGE,
key: ONYXKEYS.IS_SEARCHING_FOR_REPORTS,
value: false,
},
],
failureData: [
{
onyxMethod: Onyx.METHOD.MERGE,
key: ONYXKEYS.IS_SEARCHING_FOR_REPORTS,
value: false,
},
],
},
);
}

/**
* @private
* @param {string} searchInput
*/
const debouncedSearchInServer = lodashDebounce(searchForReports, CONST.TIMING.SEARCH_FOR_REPORTS_DEBOUNCE_TIME, {leading: false});

/**
* @param {string} searchInput
*/
function searchInServer(searchInput) {
Copy link
Contributor

Choose a reason for hiding this comment

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

NAB

Suggested change
function searchInServer(searchInput) {
function searchForReportsInServer(searchInput) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's already Report.searchInServer() so you would have Report.searchForReportInServer() 🤷

Copy link
Contributor

Choose a reason for hiding this comment

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

true that reads a bit funky

if (isNetworkOffline) {
Onyx.set(ONYXKEYS.IS_SEARCHING_FOR_REPORTS, false);
return;
}

// Why not set this in optimistic data? It won't run until the API request happens and while the API request is debounced
// we want to show the loading state right away. Otherwise, we will see a flashing UI where the client options are sorted and
// tell the user there are no options, then we start searching, and tell them there are no options again.
Onyx.set(ONYXKEYS.IS_SEARCHING_FOR_REPORTS, true);
debouncedSearchInServer(searchInput);
}

export {
searchInServer,
addComment,
addAttachment,
reconnect,
Expand Down
21 changes: 18 additions & 3 deletions src/pages/NewChatPage.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import _ from 'underscore';
import React, {useState, useEffect, useMemo} from 'react';
import React, {useState, useEffect, useMemo, useCallback} from 'react';
import {View} from 'react-native';
import PropTypes from 'prop-types';
import {withOnyx} from 'react-native-onyx';
Expand All @@ -20,6 +20,7 @@ import compose from '../libs/compose';
import personalDetailsPropType from './personalDetailsPropType';
import reportPropTypes from './reportPropTypes';
import variables from '../styles/variables';
import useNetwork from '../hooks/useNetwork';

const propTypes = {
/** Beta features list */
Expand All @@ -44,12 +45,13 @@ const defaultProps = {

const excludedGroupEmails = _.without(CONST.EXPENSIFY_EMAILS, CONST.EMAIL.CONCIERGE);

function NewChatPage({betas, isGroupChat, personalDetails, reports, translate}) {
function NewChatPage({betas, isGroupChat, personalDetails, reports, translate, isSearchingForReports}) {
Beamanator marked this conversation as resolved.
Show resolved Hide resolved
const [searchTerm, setSearchTerm] = useState('');
const [filteredRecentReports, setFilteredRecentReports] = useState([]);
const [filteredPersonalDetails, setFilteredPersonalDetails] = useState([]);
const [filteredUserToInvite, setFilteredUserToInvite] = useState();
const [selectedOptions, setSelectedOptions] = useState([]);
const {isOffline} = useNetwork();

const maxParticipantsReached = selectedOptions.length === CONST.REPORT.MAXIMUM_PARTICIPANTS;
const headerMessage = OptionsListUtils.getHeaderMessage(
Expand Down Expand Up @@ -167,6 +169,13 @@ function NewChatPage({betas, isGroupChat, personalDetails, reports, translate})
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [reports, personalDetails, searchTerm]);

// When search term updates we will fetch any reports
const setSearchTermAndSearchInServer = useCallback((text = '') => {
if (text.length) {
Report.searchInServer(text);
}
setSearchTerm(text);
}, []);
marcaaron marked this conversation as resolved.
Show resolved Hide resolved
return (
<ScreenWrapper
shouldEnableKeyboardAvoidingView={false}
Expand Down Expand Up @@ -195,16 +204,18 @@ function NewChatPage({betas, isGroupChat, personalDetails, reports, translate})
selectedOptions={selectedOptions}
value={searchTerm}
onSelectRow={(option) => createChat(option)}
onChangeText={setSearchTerm}
onChangeText={setSearchTermAndSearchInServer}
headerMessage={headerMessage}
boldStyle
shouldFocusOnSelectRow={!Browser.isMobile()}
shouldShowOptions={isOptionsDataReady}
shouldShowConfirmButton
confirmButtonText={selectedOptions.length > 1 ? translate('newChatPage.createGroup') : translate('newChatPage.createChat')}
textInputAlert={isOffline ? `${translate('common.youAppearToBeOffline')} ${translate('search.resultsAreLimited')}` : ''}
onConfirmSelection={createGroup}
textInputLabel={translate('optionsSelector.nameEmailOrPhoneNumber')}
safeAreaPaddingBottomStyle={safeAreaPaddingBottomStyle}
isLoadingNewOptions={isSearchingForReports}
/>
</View>
</KeyboardAvoidingView>
Expand All @@ -230,5 +241,9 @@ export default compose(
betas: {
key: ONYXKEYS.BETAS,
},
isSearchingForReports: {
key: ONYXKEYS.IS_SEARCHING_FOR_REPORTS,
initWithStoredValues: false,
},
}),
)(NewChatPage);
19 changes: 19 additions & 0 deletions src/pages/SearchPage.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import compose from '../libs/compose';
import personalDetailsPropType from './personalDetailsPropType';
import reportPropTypes from './reportPropTypes';
import Performance from '../libs/Performance';
import networkPropTypes from '../components/networkPropTypes';

const propTypes = {
/* Onyx Props */
Expand All @@ -37,12 +38,15 @@ const propTypes = {
...windowDimensionsPropTypes,

...withLocalizePropTypes,

network: networkPropTypes,
marcaaron marked this conversation as resolved.
Show resolved Hide resolved
};

const defaultProps = {
betas: [],
personalDetails: {},
reports: {},
network: {},
};

class SearchPage extends Component {
Expand Down Expand Up @@ -75,6 +79,10 @@ class SearchPage extends Component {
}

onChangeText(searchValue = '') {
if (searchValue.length) {
Report.searchInServer(searchValue);
}

this.setState({searchValue}, this.debouncedUpdateOptions);
}

Expand Down Expand Up @@ -187,9 +195,13 @@ class SearchPage extends Component {
showTitleTooltip
shouldShowOptions={didScreenTransitionEnd && isOptionsDataReady}
textInputLabel={this.props.translate('optionsSelector.nameEmailOrPhoneNumber')}
textInputAlert={
this.props.network.isOffline ? `${this.props.translate('common.youAppearToBeOffline')} ${this.props.translate('search.resultsAreLimited')}` : ''
}
onLayout={this.searchRendered}
safeAreaPaddingBottomStyle={safeAreaPaddingBottomStyle}
autoFocus
isLoadingNewOptions={this.props.isSearchingForReports}
Beamanator marked this conversation as resolved.
Show resolved Hide resolved
/>
</View>
</>
Expand All @@ -215,5 +227,12 @@ export default compose(
betas: {
key: ONYXKEYS.BETAS,
},
isSearchingForReports: {
key: ONYXKEYS.IS_SEARCHING_FOR_REPORTS,
initWithStoredValues: false,
},
network: {
marcaaron marked this conversation as resolved.
Show resolved Hide resolved
key: ONYXKEYS.NETWORK,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should use the withNetwork HOC instead of withOnyx. Also ONYXKEYS.NETWORK should typically be initWithStoredValues: false, because we don't have any reason to think that the persisted value for isOffline should be accurate the next time the user opens the app.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updating to withNetwork since it will reduce a subscription.

But there are so many places where we do not consider this...

we don't have any reason to think that the persisted value for isOffline should be accurate the next time the user opens the app

nothing super bad happens AFAICT

},
}),
)(SearchPage);
Loading