From 2edb7a74b710fc8f00e6c1086a3727024d447c88 Mon Sep 17 00:00:00 2001 From: David Lougheed Date: Mon, 16 Sep 2024 15:42:14 -0400 Subject: [PATCH] refact: proper one-way data flow from URL -> Redux in search --- src/js/components/Search/MakeQueryOption.tsx | 29 ++++++++++------ src/js/components/Search/Search.tsx | 29 ++++++++-------- src/js/components/Search/SelectOption.tsx | 35 +++++++++++++------- src/js/features/search/query.store.ts | 16 +++------ src/js/utils/search.ts | 6 ++++ 5 files changed, 68 insertions(+), 47 deletions(-) diff --git a/src/js/components/Search/MakeQueryOption.tsx b/src/js/components/Search/MakeQueryOption.tsx index 6460ce2b..a1781130 100644 --- a/src/js/components/Search/MakeQueryOption.tsx +++ b/src/js/components/Search/MakeQueryOption.tsx @@ -1,33 +1,42 @@ -import React from 'react'; +import React, { useCallback } from 'react'; import { Row, Col, Checkbox } from 'antd'; import OptionDescription from './OptionDescription'; -import { addQueryParam, makeGetKatsuPublic, removeQueryParam } from '@/features/search/query.store'; import SelectOption from './SelectOption'; -import { useAppDispatch, useAppSelector, useTranslationCustom, useTranslationDefault } from '@/hooks'; +import { useAppSelector, useTranslationCustom, useTranslationDefault } from '@/hooks'; import type { Field } from '@/types/search'; +import { useLocation, useNavigate } from 'react-router-dom'; +import { buildQueryParamsUrl, queryParamsWithoutKey } from '@/utils/search'; const MakeQueryOption = ({ queryField }: MakeQueryOptionProps) => { const t = useTranslationCustom(); const td = useTranslationDefault(); - const dispatch = useAppDispatch(); + + const { pathname } = useLocation(); + const navigate = useNavigate(); const { title, id, description, config, options } = queryField; const maxQueryParameters = useAppSelector((state) => state.config.maxQueryParameters); const { queryParamCount, queryParams } = useAppSelector((state) => state.query); - const isChecked = Object.prototype.hasOwnProperty.call(queryParams, id); + const isChecked = id in queryParams; - const onCheckToggle = () => { + const onCheckToggle = useCallback(() => { + let url: string; if (isChecked) { - dispatch(removeQueryParam(id)); + // If currently checked, uncheck it + url = buildQueryParamsUrl(pathname, queryParamsWithoutKey(queryParams, id)); } else { - dispatch(addQueryParam({ id, value: options[0] })); + // If currently unchecked, check it + url = buildQueryParamsUrl(pathname, { ...queryParams, [id]: options[0] }); } - dispatch(makeGetKatsuPublic()); - }; + + console.debug('[MakeQueryOption] Redirecting to:', url); + navigate(url, { replace: true }); + // Don't need to dispatch - the code handling the URL change will dispatch the fetch for us instead. + }, [id, isChecked, navigate, options, pathname, queryParams]); // TODO: allow disabling max query parameters for authenticated and authorized users when Katsu has AuthZ // useQueryWithAuthIfAllowed() diff --git a/src/js/components/Search/Search.tsx b/src/js/components/Search/Search.tsx index 510f3a64..d5cfce2d 100644 --- a/src/js/components/Search/Search.tsx +++ b/src/js/components/Search/Search.tsx @@ -24,11 +24,13 @@ const RoutedSearch = () => { const location = useLocation(); const navigate = useNavigate(); - const maxQueryParameters = useAppSelector((state) => state.config.maxQueryParameters); + const { maxQueryParameters } = useAppSelector((state) => state.config); const { querySections: searchSections, queryParams, isFetchingFields: isFetchingSearchFields, + isFetchingData: isFetchingSearchData, + attemptedFieldsFetch, attemptedFetch, } = useAppSelector((state) => state.query); @@ -70,25 +72,33 @@ const RoutedSearch = () => { // Synchronize Redux query params state from URL useEffect(() => { - if (isFetchingSearchFields) return; if (!location.pathname.endsWith('/search')) return; - const queryParam = new URLSearchParams(location.search); - const { valid, validQueryParamsObject } = validateQuery(queryParam); + + // Wait until we have search fields to try and build a valid query. Otherwise, we will mistakenly remove all URL + // query parameters and effectively reset the form. + if (!attemptedFieldsFetch || isFetchingSearchFields || isFetchingSearchData) return; + + const { valid, validQueryParamsObject } = validateQuery(new URLSearchParams(location.search)); if (valid) { if (!attemptedFetch || !checkQueryParamsEqual(validQueryParamsObject, queryParams)) { // Only update the state & refresh if we have a new set of query params from the URL. + // [!!!] This should be the only place setQueryParams(...) gets called. Everywhere else should use URL + // manipulations, so that we have a one-way data flow from URL to state! dispatch(setQueryParams(validQueryParamsObject)); dispatch(makeGetKatsuPublic()); } } else { const url = buildQueryParamsUrl(location.pathname, validQueryParamsObject); - console.debug('Redirecting to:', url); + console.debug('[Search] Redirecting to:', url); navigate(url); + // Then, the new URL will re-trigger this effect but with only valid query parameters. } }, [ attemptedFetch, + attemptedFieldsFetch, dispatch, isFetchingSearchFields, + isFetchingSearchData, location.search, location.pathname, navigate, @@ -96,15 +106,6 @@ const RoutedSearch = () => { validateQuery, ]); - // Synchronize URL from Redux query params state - useEffect(() => { - if (!location.pathname.endsWith('/search')) return; - if (!attemptedFetch) return; - const url = buildQueryParamsUrl(location.pathname, queryParams); - console.debug('Redirecting to:', url); - navigate(url, { replace: true }); - }, [attemptedFetch, location.pathname, navigate, queryParams]); - return ; }; diff --git a/src/js/components/Search/SelectOption.tsx b/src/js/components/Search/SelectOption.tsx index 8a9c0688..70019aa7 100644 --- a/src/js/components/Search/SelectOption.tsx +++ b/src/js/components/Search/SelectOption.tsx @@ -1,32 +1,43 @@ -/* eslint-disable @typescript-eslint/no-explicit-any */ -import React from 'react'; +import React, { type CSSProperties, useCallback, useMemo } from 'react'; import { Select } from 'antd'; -import { addQueryParam, makeGetKatsuPublic } from '@/features/search/query.store'; -import { useAppDispatch, useAppSelector, useTranslationCustom } from '@/hooks'; +import { useAppSelector, useTranslationCustom } from '@/hooks'; +import { useLocation, useNavigate } from 'react-router-dom'; +import { buildQueryParamsUrl } from '@/utils/search'; + +const SELECT_STYLE: CSSProperties = { width: '100%' }; const SelectOption = ({ id, isChecked, options }: SelectOptionProps) => { const t = useTranslationCustom(); - const dispatch = useAppDispatch(); - const queryParams = useAppSelector((state) => state.query.queryParams); + const { pathname } = useLocation(); + const navigate = useNavigate(); + + const { queryParams } = useAppSelector((state) => state.query); const defaultValue = queryParams[id] || options[0]; - const handleValueChange = (newValue: string) => { - dispatch(addQueryParam({ id, value: newValue })); - dispatch(makeGetKatsuPublic()); - }; + const handleValueChange = useCallback( + (newValue: string) => { + const url = buildQueryParamsUrl(pathname, { ...queryParams, [id]: newValue }); + console.debug('[SelectOption] Redirecting to:', url); + navigate(url, { replace: true }); + // Don't need to dispatch - the code handling the URL change will dispatch the fetch for us instead. + }, + [id, navigate, pathname, queryParams] + ); + + const selectOptions = useMemo(() => options.map((item) => ({ value: item, label: t(item) })), [options, t]); return (