Merge pull request #184 from bento-platform/fix/state-updates-render-…

…loops

fix: state updates / render loops

src/js/components/BentoAppRouter.tsx

@@ -19,18 +19,22 @@ import ProvenanceTab from './Provenance/ProvenanceTab';
 import BeaconQueryUi from './Beacon/BeaconQueryUi';
 import { BentoRoute } from '@/types/routes';
 import Loader from '@/components/Loader';
-import { validProjectDataset } from '@/utils/router';
+import { scopeEqual, validProjectDataset } from '@/utils/router';
 import DefaultLayout from '@/components/Util/DefaultLayout';
 
 const ScopedRoute = () => {
   const { projectId, datasetId } = useParams();
   const dispatch = useAppDispatch();
   const navigate = useNavigate();
-  const { projects } = useAppSelector((state) => state.metadata);
+  const { selectedScope, projects } = useAppSelector((state) => state.metadata);
 
   useEffect(() => {
     // Update selectedScope based on URL parameters
-    const valid = validProjectDataset(projects, projectId, datasetId);
+    const valid = validProjectDataset(projects, { project: projectId, dataset: datasetId });
+
+    // Don't change the scope object if the scope value is the same, otherwise it'll trigger needless re-renders.
+    if (scopeEqual(selectedScope.scope, valid.scope)) return;
+
     if (datasetId === valid.scope.dataset && projectId === valid.scope.project) {
       dispatch(selectScope(valid.scope));
     } else {
@@ -50,7 +54,7 @@ const ScopedRoute = () => {
       const newPathString = '/' + newPath.join('/');
       navigate(newPathString, { replace: true });
     }
-  }, [projects, projectId, datasetId, dispatch, navigate]);
+  }, [projects, projectId, datasetId, dispatch, navigate, selectedScope]);
 
   return <Outlet />;
 };

src/js/components/Overview/Counts.tsx

@@ -1,4 +1,4 @@
-import React from 'react';
+import React, { type CSSProperties } from 'react';
 import { Typography, Card, Space, Statistic } from 'antd';
 import { TeamOutlined } from '@ant-design/icons';
 import { BiDna } from 'react-icons/bi';
@@ -7,6 +7,14 @@ import ExpSvg from '../Util/ExpSvg';
 import { COUNTS_FILL, BOX_SHADOW } from '@/constants/overviewConstants';
 import { useAppSelector, useTranslationDefault } from '@/hooks';
 
+const styles: Record<string, CSSProperties> = {
+  countCard: {
+    ...BOX_SHADOW,
+    minWidth: 150,
+    transition: 'height 0.3s ease-in-out',
+  },
+};
+
 const Counts = () => {
   const td = useTranslationDefault();
 
@@ -35,7 +43,7 @@ const Counts = () => {
       <Typography.Title level={3}>{td('Counts')}</Typography.Title>
       <Space direction="horizontal">
         {data.map(({ title, icon, count }, i) => (
-          <Card key={i} style={BOX_SHADOW}>
+          <Card key={i} style={{ ...styles.countCard, height: isFetchingData ? 138 : 114 }}>
             <Statistic
               title={td(title)}
               value={count}

src/js/components/Scope/DatasetScopePicker.tsx

@@ -1,10 +1,12 @@
 import React, { useMemo } from 'react';
-import { List, Avatar, Space, Typography } from 'antd';
-import { useAppSelector, useTranslationCustom, useTranslationDefault } from '@/hooks';
 import { Link, useLocation, useNavigate } from 'react-router-dom';
+import { List, Avatar, Space, Typography } from 'antd';
 import { FaDatabase } from 'react-icons/fa';
-import { getCurrentPage } from '@/utils/router';
+
+import type { DiscoveryScope } from '@/features/metadata/metadata.store';
+import { useAppSelector, useTranslationCustom, useTranslationDefault } from '@/hooks';
 import type { Project } from '@/types/metadata';
+import { getCurrentPage, scopeEqual, scopeToUrl } from '@/utils/router';
 
 type DatasetScopePickerProps = {
   parentProject: Project;
@@ -29,7 +31,8 @@ const DatasetScopePicker = ({ parentProject }: DatasetScopePickerProps) => {
   );
   const showSelectProject = !selectedScope.fixedProject && parentProject.identifier != scopeObj.project;
 
-  const projectURL = `${baseURL}/p/${parentProject.identifier}/${page}`;
+  const parentProjectScope: DiscoveryScope = { project: parentProject.identifier };
+  const projectURL = scopeToUrl(parentProjectScope, baseURL, `/${page}`);
 
   return (
     <Space direction="vertical" style={{ display: 'flex' }}>
@@ -49,21 +52,18 @@ const DatasetScopePicker = ({ parentProject }: DatasetScopePickerProps) => {
       <List
         dataSource={parentProject.datasets}
         bordered
-        renderItem={(dataset) => {
-          const datasetURL = `${baseURL}/p/${parentProject.identifier}/d/${dataset.identifier}/${page}`;
-          const selected = scopeObj.dataset && dataset.identifier === scopeObj.dataset;
+        renderItem={({ identifier, title, description }) => {
+          const itemScope = { ...parentProjectScope, dataset: identifier };
+          const selected = scopeEqual(itemScope, scopeObj); // item scope === dataset scope
+          const datasetURL = scopeToUrl(itemScope, baseURL, `/${page}`);
           return (
             <List.Item
               className={`select-dataset-item${selected ? ' selected' : ''}`}
-              key={dataset.identifier}
+              key={identifier}
               onClick={() => navigate(datasetURL)}
               style={{ cursor: 'pointer' }}
             >
-              <List.Item.Meta
-                avatar={<Avatar icon={<FaDatabase />} />}
-                title={t(dataset.title)}
-                description={t(dataset.description)}
-              />
+              <List.Item.Meta avatar={<Avatar icon={<FaDatabase />} />} title={t(title)} description={t(description)} />
             </List.Item>
           );
         }}

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 { maxQueryParameters } = useAppSelector((state) => state.config);
   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()

src/js/components/Search/Search.tsx

@@ -19,16 +19,18 @@ const checkQueryParamsEqual = (qp1: QueryParams, qp2: QueryParams): boolean => {
   return params.reduce((acc, v) => acc && qp1[v] === qp2[v], true);
 };
 
-const RoutedSearch: React.FC = () => {
+const RoutedSearch = () => {
   const dispatch = useAppDispatch();
   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,41 +72,40 @@ const RoutedSearch: React.FC = () => {
 
   // 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,
     queryParams,
     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 <Search />;
 };
 
@@ -113,7 +114,7 @@ const SEARCH_SPACE_ITEM_STYLE = { item: WIDTH_100P_STYLE };
 const SEARCH_SECTION_SPACE_ITEM_STYLE = { item: { display: 'flex', justifyContent: 'center' } };
 const SEARCH_SECTION_STYLE = { maxWidth: 1200 };
 
-const Search: React.FC = () => {
+const Search = () => {
   const t = useTranslationCustom();
 
   const { isFetchingFields: isFetchingSearchFields, querySections: searchSections } = useAppSelector(

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 { useLocation, useNavigate } from 'react-router-dom';
 import { Select } from 'antd';
 
-import { addQueryParam, makeGetKatsuPublic } from '@/features/search/query.store';
-import { useAppDispatch, useAppSelector, useTranslationCustom } from '@/hooks';
+import { useAppSelector, useTranslationCustom } from '@/hooks';
+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 (
     <Select
       id={id}
       disabled={!isChecked}
       showSearch
-      style={{ width: '100%' }}
+      style={SELECT_STYLE}
       onChange={handleValueChange}
       value={defaultValue}
       defaultValue={defaultValue}
-      options={options.map((item) => ({ value: item, label: t(item) }))}
+      options={selectOptions}
     />
   );
 };

src/js/constants/overviewConstants.ts

@@ -1,3 +1,4 @@
+import type { CSSProperties } from 'react';
 import type { HexColor } from 'bento-charts';
 
 export const COUNTS_FILL = '#75787a';
@@ -8,7 +9,7 @@ export const CHART_HEIGHT = 350;
 export const PIE_CHART_HEIGHT = 300; // rendered slightly smaller since labels can clip
 export const DEFAULT_CHART_WIDTH = 1;
 
-export const BOX_SHADOW = { boxShadow: '0 2px 10px rgba(0,0,0,0.05)' };
+export const BOX_SHADOW: CSSProperties = { boxShadow: '0 2px 10px rgba(0,0,0,0.05)' };
 
 export const CHART_WIDTH = 450;
 export const GRID_GAP = 20;