fix: react hooks (#1510)

* re-enable react hooks lint * fix zquery issue with hook deps * prevent candlestick state churn * suppress onWheel passive warning * respect curly-all --------- Co-authored-by: turbocrime <[email protected]> Co-authored-by: Jesse Pinho <[email protected]>
penumbra-zone · Jul 22, 2024 · e0f4258 · e0f4258
1 parent 52fdce2
commit e0f4258
Show file tree

Hide file tree

Showing 12 changed files with 104 additions and 88 deletions.
diff --git a/.changeset/angry-guests-pay.md b/.changeset/angry-guests-pay.md
@@ -0,0 +1,5 @@
+---
+'@penumbra-zone/zquery': patch
+---
+
+correct state dependency for fetch
diff --git a/apps/minifront/src/components/ibc/ibc-in/destination-addr.tsx b/apps/minifront/src/components/ibc/ibc-in/destination-addr.tsx
@@ -16,7 +16,7 @@ export const DestinationAddr = () => {
   // Set initial account to trigger address loading
   useEffect(() => {
     setAccount(0);
-  }, []);
+  }, [setAccount]);
 
   return (
     <div className='mb-2 flex w-full flex-col gap-1 text-stone-700'>

diff --git a/apps/minifront/src/components/shared/number-input/use-wheel-prevent.ts b/apps/minifront/src/components/shared/number-input/use-wheel-prevent.ts
@@ -15,11 +15,9 @@ export const useWheelPrevent = () => {
   };
 
   useEffect(() => {
-    inputRef.current?.addEventListener('wheel', onWheel);
-
-    return () => {
-      inputRef.current?.removeEventListener('wheel', onWheel);
-    };
+    const ac = new AbortController();
+    inputRef.current?.addEventListener('wheel', onWheel, { signal: ac.signal, passive: false });
+    return () => ac.abort();
   }, []);
 
   return inputRef;

diff --git a/apps/minifront/src/components/shared/selectors/balance-item.tsx b/apps/minifront/src/components/shared/selectors/balance-item.tsx
@@ -36,7 +36,7 @@ export const BalanceItem = ({ asset, value, onSelect }: BalanceItemProps) => {
       return metadataFromValue?.equals(metadataFromAsset);
     }
     return false;
-  }, [asset, value]);
+  }, [asset, metadataFromAsset, metadataFromValue, value]);
 
   return (
     <DialogClose asChild onClick={() => onSelect(asset)}>

diff --git a/apps/minifront/src/components/staking/account/use-staking-tokens-and-filter.ts b/apps/minifront/src/components/staking/account/use-staking-tokens-and-filter.ts
@@ -87,7 +87,7 @@ export const useStakingTokensAndFilter = (
       (prev, curr) => toAccountSwitcherFilter(prev, curr, stakingTokenMetadata),
       [],
     );
-  }, [balancesByAccount]);
+  }, [balancesByAccount, stakingTokenMetadata]);
 
   const stakingTokens = stakingTokensByAccount.get(account);
 

diff --git a/apps/minifront/src/components/swap/swap-form/token-swap-input.tsx b/apps/minifront/src/components/swap/swap-form/token-swap-input.tsx
@@ -10,7 +10,7 @@ import {
   getMetadataFromBalancesResponseOptional,
 } from '@penumbra-zone/getters/balances-response';
 import { ArrowRight } from 'lucide-react';
-import { useEffect, useMemo } from 'react';
+import { useEffect, useMemo, useState } from 'react';
 import { getBlockDate } from '../../../fetchers/block-date';
 import { AllSlices } from '../../../state';
 import { useStoreShallow } from '../../../utils/use-store-shallow';
@@ -79,23 +79,45 @@ export const TokenSwapInput = () => {
     return getDisplayDenomExponent.optional()(getMetadataFromBalancesResponseOptional(assetIn));
   }, [assetIn]);
 
+  /**
+   * @todo this memo, state, and effects below are awkward. but this prevents
+   * excessive state churn. this is a temporary solution and will be corrected
+   * after implementing use of zquery for this data.
+   *
+   * @issue https://github.com/penumbra-zone/web/issues/1530
+   */
+  const loadPriceHistoryMemo = useMemo(
+    () => (assetIn && assetOut ? priceHistory.load : undefined),
+    [assetIn, assetOut, priceHistory.load],
+  );
+
+  const [updatedHeight, setUpdatedHeight] = useState<bigint>();
+
+  // initial price history
   useEffect(() => {
-    if (!assetIn || !assetOut) {
+    if (updatedHeight ?? !loadPriceHistoryMemo) {
       return;
-    } else {
-      return priceHistory.load();
     }
-  }, [assetIn, assetOut]);
+    setUpdatedHeight(latestKnownBlockHeight);
+    return loadPriceHistoryMemo();
+  }, [assetIn, assetOut, latestKnownBlockHeight, loadPriceHistoryMemo, updatedHeight]);
 
+  // update price history
   useEffect(() => {
-    if (!priceHistory.candles.length) {
+    // don't attempt to update an absent price history
+    if (!updatedHeight || !loadPriceHistoryMemo) {
       return;
-    } else if (latestKnownBlockHeight % 10n) {
+    }
+
+    // rate limit pricing updates
+    const distanceFromLatest = latestKnownBlockHeight - updatedHeight;
+    if (!distanceFromLatest || distanceFromLatest < 10n) {
       return;
-    } else {
-      return priceHistory.load();
     }
-  }, [priceHistory, latestKnownBlockHeight]);
+
+    setUpdatedHeight(latestKnownBlockHeight);
+    return loadPriceHistoryMemo();
+  }, [priceHistory.candles, latestKnownBlockHeight, loadPriceHistoryMemo, updatedHeight]);
 
   const maxAmount = getAmount.optional()(assetIn);
   const maxAmountAsString = maxAmount ? joinLoHiAmount(maxAmount).toString() : undefined;

diff --git a/apps/minifront/src/state/swap/helpers.ts b/apps/minifront/src/state/swap/helpers.ts
@@ -140,7 +140,7 @@ export const sendCandlestickDataRequests = async (
     },
   );
 
-  return combinedCandles;
+  return combinedCandles.sort((a, b) => Number(a.height - b.height));
 };
 
 const byBalanceDescending = (a: BalancesResponse, b: BalancesResponse) => {

diff --git a/eslint.config.js b/eslint.config.js
@@ -74,15 +74,13 @@ export default tseslint.config(
     rules: {
       ...react.configs.recommended.rules,
       ...react_hooks.configs.recommended.rules,
+      'react-hooks/exhaustive-deps': 'warn',
+      'react-hooks/rules-of-hooks': 'error',
     },
   },
   {
     name: 'custom:react-wishlist-improvements',
     rules: {
-      // these were from a broken plugin. should be enabled and fixed.
-      'react-hooks/exhaustive-deps': 'off',
-      'react-hooks/rules-of-hooks': 'off',
-
       // this plugin was formerly included, but was never actually applied.
       'react-refresh/only-export-components': 'off',
 
@@ -156,7 +154,6 @@ export default tseslint.config(
         'error',
         { requireDefaultForNonUnion: true },
       ],
-      curly: ['error', 'all'],
       eqeqeq: ['error', 'smart'],
     },
   },
@@ -258,4 +255,9 @@ export default tseslint.config(
 
   // disable rules covered by prettier
   prettier,
+
+  {
+    name: 'custom:prettier-would-disable',
+    rules: { curly: ['error', 'all'] },
+  },
 );
diff --git a/packages/react/src/components/penumbra-context-provider.tsx b/packages/react/src/components/penumbra-context-provider.tsx
@@ -91,7 +91,7 @@ export const PenumbraContextProvider = ({
       { signal: ac.signal },
     );
     return () => ac.abort();
-  }, [failure, penumbra?.addEventListener, providerManifest, penumbra?.manifest, setFailure]);
+  }, [failure, providerManifest, setFailure, penumbra]);
 
   // request effect
   useEffect(() => {
@@ -107,14 +107,7 @@ export const PenumbraContextProvider = ({
           break;
       }
     }
-  }, [
-    failure,
-    makeApprovalRequest,
-    penumbra?.request,
-    providerManifest,
-    providerState,
-    setFailure,
-  ]);
+  }, [failure, makeApprovalRequest, penumbra, providerManifest, providerState, setFailure]);
 
   // connect effect
   useEffect(() => {
@@ -139,14 +132,7 @@ export const PenumbraContextProvider = ({
           break;
       }
     }
-  }, [
-    failure,
-    makeApprovalRequest,
-    penumbra?.connect,
-    providerManifest,
-    providerState,
-    setFailure,
-  ]);
+  }, [failure, makeApprovalRequest, penumbra, providerManifest, providerState, setFailure]);
 
   const createdContext: PenumbraContext = useMemo(
     () => ({
@@ -181,12 +167,7 @@ export const PenumbraContextProvider = ({
     }),
     [
       failure,
-      penumbra?.addEventListener,
-      penumbra?.connect,
-      penumbra?.disconnect,
-      penumbra?.manifest,
-      penumbra?.removeEventListener,
-      penumbra?.request,
+      penumbra,
       providerConnected,
       providerManifest,
       providerPort,

diff --git a/packages/ui/components/ui/candlestick-plot/index.tsx b/packages/ui/components/ui/candlestick-plot/index.tsx
@@ -79,18 +79,31 @@ export const CandlestickPlot = withTooltip<CandlestickPlotProps, CandlestickData
   }: CandlestickPlotProps & WithTooltipProvidedProps<CandlestickData>) => {
     const { parentRef, width: w, height: h } = useParentSize({ debounceTime: 150 });
 
-    const { maxPrice, minPrice } = useMemo(
-      () =>
-        candles.reduce(
-          (acc, d) => ({
-            minPrice: Math.min(acc.minPrice, lowPrice(d)),
-            maxPrice: Math.max(acc.maxPrice, highPrice(d)),
-          }),
-          { minPrice: Infinity, maxPrice: -Infinity },
-        ),
-      [candles],
-    );
-    const maxSpread = maxPrice - minPrice;
+    const { blockScale, priceScale } = useMemo(() => {
+      const { maxPrice, minPrice } = candles.reduce(
+        (acc, d) => ({
+          minPrice: Math.min(acc.minPrice, lowPrice(d)),
+          maxPrice: Math.max(acc.maxPrice, highPrice(d)),
+        }),
+        { minPrice: Infinity, maxPrice: -Infinity },
+      );
+      const maxSpread = maxPrice - minPrice;
+
+      const startBlock = candles.length && blockHeight(candles[0]!);
+      const endBlock = candles.length && blockHeight(candles[candles.length - 1]!);
+
+      const blockScale = scaleLinear<number>({
+        range: [50, w - 5],
+        domain: [startBlock, latestKnownBlockHeight ?? endBlock],
+      });
+
+      const priceScale = scaleLinear<number>({
+        range: [h, 0],
+        domain: [minPrice - maxSpread / 2, maxPrice + maxSpread / 2],
+      });
+
+      return { priceScale, blockScale };
+    }, [candles, latestKnownBlockHeight, w, h]);
 
     const useTooltip = useCallback(
       (d: CandlestickData) => ({
@@ -105,32 +118,18 @@ export const CandlestickPlot = withTooltip<CandlestickPlotProps, CandlestickData
           hideTooltip();
         },
       }),
-      [],
+      [blockScale, hideTooltip, priceScale, showTooltip],
     );
 
     if (!candles.length) {
       return null;
     }
 
-    // assertions here okay because we've just checked length
-    const startBlock = blockHeight(candles[0]!);
-    const endBlock = blockHeight(candles[candles.length - 1]!);
-
     // candle width as fraction of graph width. likely too thin to really
     // matter. if there's lots of records this will overlap, and we'll need to
     // implement some kind of binning.
     const blockWidth = Math.min(w / candles.length, 6);
 
-    const blockScale = scaleLinear<number>({
-      range: [50, w - 5],
-      domain: [startBlock, latestKnownBlockHeight ?? endBlock],
-    });
-
-    const priceScale = scaleLinear<number>({
-      range: [h, 0],
-      domain: [minPrice - maxSpread / 2, maxPrice + maxSpread / 2],
-    });
-
     return (
       <>
         <div className={className ?? 'size-full'} ref={parentRef}>
@@ -266,7 +265,7 @@ export const CandlesticksTooltip = ({
     const ac = new AbortController();
     void getBlockDate(data.height, ac.signal).then(setBlockDate);
     return () => ac.abort('Abort tooltip date query');
-  }, [data]);
+  }, [data, getBlockDate]);
 
   const endBase = endMetadata.denomUnits.filter(d => !d.exponent)[0]!;
   const startBase = startMetadata.denomUnits.filter(d => !d.exponent)[0]!;

diff --git a/packages/ui/components/ui/tx/index.tsx b/packages/ui/components/ui/tx/index.tsx
@@ -49,7 +49,7 @@ const useFeeMetadata = (txv: TransactionView, getMetadata: MetadataFetchFn) => {
         setIsLoading(false);
       })
       .catch((e: unknown) => setError(e));
-  }, [txv, getMetadata, setFeeValueView]);
+  }, [txv, getMetadata, setFeeValueView, amount]);
 
   return { feeValueView, isLoading, error };
 };

diff --git a/packages/zquery/src/get-use-hook.ts b/packages/zquery/src/get-use-hook.ts
@@ -1,4 +1,4 @@
-import { useEffect, useRef } from 'react';
+import { useEffect, useRef, useState } from 'react';
 import {
   AbridgedZQueryState,
   CreateZQueryStreamingProps,
@@ -7,6 +7,9 @@ import {
   ZQueryState,
 } from './types.js';
 
+const shallowCompareArrays = (a: unknown[], b: unknown[]): boolean =>
+  a.every((item, index) => item === b[index]) && a.length === b.length;
+
 /**
  * Returns a hook that can be used via `use[Name]()` to access the ZQuery state.
  */
@@ -72,29 +75,35 @@ export const getUseHook = <
   ) => {
     const useStore = props.getUseStore();
 
+    // We want to use a custom comparator to see if `fetchArgs` changed.
+    // `useMemo()` does not support custom comparators, so we'll roll it ourself
+    // using a combination of `useState` and `useEffect`.
+    const [fetchArgsMemo, setFetchArgsMemo] = useState(fetchArgs);
     useEffect(() => {
-      const fetch = props.get(useStore.getState())._zQueryInternal.fetch;
+      if (!shallowCompareArrays(fetchArgs, fetchArgsMemo)) {
+        setFetchArgsMemo(fetchArgs);
+      }
+    }, [fetchArgs, fetchArgsMemo]);
 
-      {
-        const newReferenceCount = incrementReferenceCounter();
+    useEffect(() => {
+      const fetch = props.get(useStore.getState())._zQueryInternal.fetch;
 
-        if (newReferenceCount === 1) {
-          setAbortController(new AbortController());
-          void fetch(...fetchArgs);
-        }
+      const incrementedReferenceCount = incrementReferenceCounter();
+      if (incrementedReferenceCount === 1) {
+        setAbortController(new AbortController());
+        void fetch(...fetchArgsMemo);
       }
 
       const onUnmount = () => {
-        const newReferenceCount = decrementReferenceCounter();
-
-        if (newReferenceCount === 0) {
+        const decrementedReferenceCount = decrementReferenceCounter();
+        if (decrementedReferenceCount === 0) {
           props.get(useStore.getState())._zQueryInternal.abortController?.abort();
           setAbortController(undefined);
         }
       };
 
       return onUnmount;
-    }, [fetch]);
+    }, [fetchArgsMemo, useStore]);
 
     const prevState = useRef<ZQueryState<DataType | ProcessedDataType, FetchArgs> | undefined>();
     const prevSelectedState = useRef<
-Original file line number
+Diff line change
@@ Expand Up / @@ -140,7 +140,7 @@ export const sendCandlestickDataRequests = async ( @@
         },
       );
-      return combinedCandles;
+      return combinedCandles.sort((a, b) => Number(a.height - b.height));
     };
     const byBalanceDescending = (a: BalancesResponse, b: BalancesResponse) => {
@@ Expand Down @@