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

Refactor search #1594

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
19 changes: 15 additions & 4 deletions src/core/caching/cacheUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,17 +17,28 @@ export function loadListIfNecessary<
remoteList: RemoteList<DataType> | undefined,
dispatch: AppDispatch,
hooks: {
actionOnError?: (err: unknown) => PayloadAction<unknown>;
Copy link
Contributor

Choose a reason for hiding this comment

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

ooo smart addition! I can see this coming in handy! :D

actionOnLoad: () => PayloadAction<OnLoadPayload>;
actionOnSuccess: (items: DataType[]) => PayloadAction<OnSuccessPayload>;
loader: () => Promise<DataType[]>;
}
): IFuture<DataType[]> {
if (!remoteList || shouldLoad(remoteList)) {
dispatch(hooks.actionOnLoad());
const promise = hooks.loader().then((val) => {
dispatch(hooks.actionOnSuccess(val));
return val;
});
const promise = hooks
.loader()
.then((val) => {
dispatch(hooks.actionOnSuccess(val));
return val;
})
.catch((err: unknown) => {
if (hooks.actionOnError) {
dispatch(hooks.actionOnError(err));
return null;
} else {
throw err;
}
});

return new PromiseFuture(promise);
}
Expand Down
3 changes: 3 additions & 0 deletions src/core/store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import eventsSlice, { EventsStoreSlice } from 'features/events/store';
import organizationsSlice, {
OrganizationsStoreSlice,
} from 'features/organizations/store';
import searchSlice, { SearchStoreSlice } from 'features/search/store';
import smartSearchSlice, {
smartSearchStoreSlice,
} from 'features/smartSearch/store';
Expand All @@ -34,6 +35,7 @@ export interface RootState {
callAssignments: CallAssignmentSlice;
campaigns: CampaignsStoreSlice;
events: EventsStoreSlice;
search: SearchStoreSlice;
smartSearch: smartSearchStoreSlice;
surveys: SurveysStoreSlice;
tags: TagsStoreSlice;
Expand All @@ -48,6 +50,7 @@ const reducer = {
campaigns: campaignsSlice.reducer,
events: eventsSlice.reducer,
organizations: organizationsSlice.reducer,
search: searchSlice.reducer,
smartSearch: smartSearchSlice.reducer,
surveys: surveysSlice.reducer,
tags: tagsSlice.reducer,
Expand Down
55 changes: 11 additions & 44 deletions src/features/search/components/SearchDialog/index.tsx
Original file line number Diff line number Diff line change
@@ -1,18 +1,13 @@
import { makeStyles } from '@mui/styles';
import { useQuery } from 'react-query';
import { useRouter } from 'next/router';
import { Box, Dialog } from '@mui/material';
import { useEffect, useState } from 'react';

import defaultFetch from 'utils/fetching/defaultFetch';
import handleResponseData from 'utils/api/handleResponseData';
import isUserTyping from 'features/search/utils/isUserTyping';
import ResultsList from 'features/search/components/SearchDialog/ResultsList';
import SearchField from './SearchField';
import { SearchResult } from '../types';
import useDebounce from 'utils/hooks/useDebounce';

export const MINIMUM_CHARACTERS = 2;
import { useNumericRouteParams } from 'core/hooks';
import useSearch from 'features/search/hooks/useSearch';

const useStyles = makeStyles(() => ({
topPaperScrollBody: {
Expand All @@ -23,33 +18,17 @@ const useStyles = makeStyles(() => ({
},
}));

const getSearchResults = (orgId: string, searchQuery: string) => {
return async () => {
const res = await defaultFetch(`/search?orgId=${orgId}`, {
body: JSON.stringify({ q: searchQuery }),
headers: {
'Content-Type': 'application/json',
},
method: 'POST',
});
return handleResponseData<SearchResult[]>(res, 'post');
};
};

const SearchDialog: React.FunctionComponent<{
activator: (openDialog: () => void) => JSX.Element;
}> = ({ activator }) => {
const [open, setOpen] = useState(false);
const [searchQuery, setSearchQuery] = useState('');
const [isTyping, setIsTyping] = useState(false);

const classes = useStyles();
const router = useRouter();
const { orgId } = router.query as { orgId: string };
const { orgId } = useNumericRouteParams();

const debouncedFinishedTyping = useDebounce(async () => {
setIsTyping(false);
}, 600);
const { error, results, isLoading, setQuery } = useSearch(orgId);

const handleRouteChange = () => {
// Close dialog when clicking an item
Expand Down Expand Up @@ -79,19 +58,6 @@ const SearchDialog: React.FunctionComponent<{
};
}, [router]);

const {
data: searchResults,
isFetching,
isError,
} = useQuery(
['searchResults', searchQuery],
getSearchResults(orgId, searchQuery),
{
enabled: !isTyping && searchQuery.length >= MINIMUM_CHARACTERS,
retry: false,
}
);

return (
<>
{activator(() => setOpen(true))}
Expand All @@ -103,23 +69,24 @@ const SearchDialog: React.FunctionComponent<{
fullWidth
onClose={() => {
setOpen(false);
setSearchQuery('');
setQuery('');
}}
open={open}
>
<Box p={1}>
<SearchField
error={isError}
loading={isFetching}
onChange={(e) => setSearchQuery(e.target.value)}
error={!!error}
loading={isLoading}
onChange={(e) => setQuery(e.target.value)}
onKeyDown={() => {
if (!isTyping) {
setIsTyping(true);
}
debouncedFinishedTyping();
}}
/>
{searchResults && <ResultsList results={searchResults} />}
{results && (
<ResultsList results={results.map((item) => item.result)} />
)}
</Box>
</Dialog>
</>
Expand Down
71 changes: 71 additions & 0 deletions src/features/search/hooks/useSearch.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
import { loadListIfNecessary } from 'core/caching/cacheUtils';
import { SearchResult } from '../components/types';
import useDebounce from 'utils/hooks/useDebounce';
import { useState } from 'react';
import {
resultsError,
resultsLoad,
resultsLoaded,
SearchResultItem,
} from '../store';
import { useApiClient, useAppDispatch, useAppSelector } from 'core/hooks';

type UseSearchReturn = {
error: unknown;
isLoading: boolean;
results: SearchResultItem[];
setQuery: (q: string) => void;
};

export default function useSearch(orgId: number): UseSearchReturn {
const apiClient = useApiClient();
const [isTyping, setIsTyping] = useState(false);
const [queryString, setQueryString] = useState('');
const dispatch = useAppDispatch();
const list = useAppSelector(
(state) => state.search.matchesByQuery[queryString]
);

const debouncedFinishedTyping = useDebounce(async () => {
setIsTyping(false);
}, 600);

const setQuery = (q: string) => {
setQueryString(q);
setIsTyping(true);
debouncedFinishedTyping();
};

if (!isTyping && queryString.length > 2) {
const future = loadListIfNecessary(list, dispatch, {
actionOnError: (err) => resultsError([queryString, err]),
actionOnLoad: () => resultsLoad(queryString),
actionOnSuccess: (results) => resultsLoaded([queryString, results]),
loader: () =>
apiClient
.post<SearchResult[], { q: string }>(`/api/search?orgId=${orgId}`, {
q: queryString,
})
.then((results) =>
results.map((result) => ({
id: `${result.type}-${result.match.id}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

aaah this is so much nicer than the solution I came up with in my useMemberships hook yesterday.. (i added ids in the store action)

result,
}))
),
});

return {
error: future.error,
isLoading: future.isLoading,
results: future.data || [],
setQuery,
};
} else {
return {
error: null,
isLoading: false,
results: [],
setQuery,
};
}
}
46 changes: 46 additions & 0 deletions src/features/search/store.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
import { createSlice, PayloadAction } from '@reduxjs/toolkit';

import { SearchResult } from './components/types';
import { remoteList, RemoteList } from 'utils/storeUtils';

export type SearchResultItem = {
Copy link
Contributor

Choose a reason for hiding this comment

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

yep i should have done like this as well....

id: string;
result: SearchResult;
};

export interface SearchStoreSlice {
matchesByQuery: Record<string, RemoteList<SearchResultItem>>;
}

const initialState: SearchStoreSlice = {
matchesByQuery: {},
};

const searchSlice = createSlice({
initialState,
name: 'search',
reducers: {
resultsError: (state, action: PayloadAction<[string, unknown]>) => {
const [query, error] = action.payload;
state.matchesByQuery[query] = remoteList();
state.matchesByQuery[query].error = error;
Copy link
Contributor

Choose a reason for hiding this comment

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

nice! I have not seen the error part of these in action anywhere else i think, seems like something we want to take into more parts!

Copy link
Member Author

Choose a reason for hiding this comment

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

True! I just delayed error handling (lazily) but there was a test here that failed, and I had to decide whether to rip out the test and all the error code, or just implement crude error handling, so I chose the latter. 😊

state.matchesByQuery[query].loaded = new Date().toISOString();
},
resultsLoad: (state, action: PayloadAction<string>) => {
const query = action.payload;
state.matchesByQuery[query] = remoteList();
state.matchesByQuery[query].isLoading = true;
},
resultsLoaded: (
state,
action: PayloadAction<[string, SearchResultItem[]]>
) => {
const [query, results] = action.payload;
state.matchesByQuery[query] = remoteList(results);
state.matchesByQuery[query].loaded = new Date().toISOString();
},
},
});

export default searchSlice;
export const { resultsError, resultsLoad, resultsLoaded } = searchSlice.actions;
Loading