From 9ba822628a966b5be30f4e068a6ff138b8c2958d Mon Sep 17 00:00:00 2001 From: Jeremy Ruppel Date: Wed, 18 Mar 2026 19:26:17 -0400 Subject: [PATCH] refactor(site): remove derivable useEffect antipatterns (#23267) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit React's [Why You Might Not Need An Effect](https://react.dev/learn/you-might-not-need-an-effect) article describes several antipatterns and footguns you might encounter when working with useEffect, so I fed it to an agent and let it determine some low hanging fruit to fix: Replace useEffect+setState patterns with direct computations where the values are purely derived from props, state, or query data: - useWebpushNotifications: derive `enabled` inline from query data instead of setting it via useEffect - ProxyContext: replace `proxy` state + updateProxy callback + useEffect with a single useMemo over its three inputs - useSyncFormParameters: replace ref-sync useEffect with direct assignment during render - AgentsSidebar (LoadMoreSentinel): replace two ref-sync useEffects with direct assignments during render Co-authored by Coder Agent 🤖 --- site/src/contexts/ProxyContext.tsx | 45 +++++-------------- site/src/contexts/useWebpushNotifications.ts | 9 +--- .../modules/hooks/useSyncFormParameters.ts | 4 +- site/src/pages/AgentsPage/AgentsSidebar.tsx | 9 +--- 4 files changed, 17 insertions(+), 50 deletions(-) diff --git a/site/src/contexts/ProxyContext.tsx b/site/src/contexts/ProxyContext.tsx index 409dbf3673..d4aeb9d3fe 100644 --- a/site/src/contexts/ProxyContext.tsx +++ b/site/src/contexts/ProxyContext.tsx @@ -7,9 +7,9 @@ import { createContext, type FC, type PropsWithChildren, - useCallback, useContext, useEffect, + useMemo, useState, } from "react"; import { useQuery } from "react-query"; @@ -95,11 +95,6 @@ export const ProxyProvider: FC = ({ children }) => { // proxy. const [userSavedProxy, setUserSavedProxy] = useState(loadUserSelectedProxy()); - // Load the initial state from local storage. - const [proxy, setProxy] = useState( - computeUsableURLS(userSavedProxy), - ); - const { permissions } = useAuthenticated(); const { metadata } = useEmbeddedMetadata(); @@ -131,43 +126,30 @@ export const ProxyProvider: FC = ({ children }) => { loaded: latenciesLoaded, } = useProxyLatency(proxiesResp); - // updateProxy is a helper function that when called will - // update the proxy being used. - const updateProxy = useCallback(() => { - // Update the saved user proxy for the caller. - setUserSavedProxy(loadUserSelectedProxy()); - setProxy( + const proxy = useMemo( + () => getPreferredProxy( proxiesResp ?? [], - loadUserSelectedProxy(), + userSavedProxy, proxyLatencies, - // Do not auto select based on latencies, as inconsistent latencies can cause this - // to change on each call. updateProxy should be stable when selecting a proxy to - // prevent flickering. + // Do not auto select based on latencies, as inconsistent + // latencies can cause this to change on each call. The proxy + // value should be stable to prevent flickering. false, ), - ); - }, [proxiesResp, proxyLatencies]); - - // This useEffect ensures the proxy to be used is updated whenever the state changes. - // This includes proxies being loaded, latencies being calculated, and the user selecting a proxy. - // biome-ignore lint/correctness/useExhaustiveDependencies: Only update if the source data changes - useEffect(() => { - updateProxy(); - }, [proxiesResp, proxyLatencies]); + [proxiesResp, userSavedProxy, proxyLatencies], + ); // This useEffect will auto select the best proxy if the user has not selected one. // It must wait until all latencies are loaded to select based on latency. This does mean // the first time a user loads the page, the proxy will "flicker" to the best proxy. // // Once the page is loaded, or the user selects a proxy, this will not run again. - // biome-ignore lint/correctness/useExhaustiveDependencies: Only update if the source data changes useEffect(() => { if (loadUserSelectedProxy() !== undefined) { return; // User has selected a proxy, do not auto select. } if (!latenciesLoaded) { - // Wait until the latencies are loaded first. return; } @@ -180,7 +162,7 @@ export const ProxyProvider: FC = ({ children }) => { if (best?.proxy) { saveUserSelectedProxy(best.proxy); - updateProxy(); + setUserSavedProxy(best.proxy); } }, [latenciesLoaded, proxiesResp, proxyLatencies]); @@ -199,15 +181,12 @@ export const ProxyProvider: FC = ({ children }) => { // These functions are exposed to allow the user to select a proxy. setProxy: (proxy: Region) => { - // Save to local storage to persist the user's preference across reloads saveUserSelectedProxy(proxy); - // Update the selected proxy - updateProxy(); + setUserSavedProxy(proxy); }, clearProxy: () => { - // Clear the user's selection from local storage. clearUserSelectedProxy(); - updateProxy(); + setUserSavedProxy(undefined); }, }} > diff --git a/site/src/contexts/useWebpushNotifications.ts b/site/src/contexts/useWebpushNotifications.ts index 6f65176309..2dcc2236ef 100644 --- a/site/src/contexts/useWebpushNotifications.ts +++ b/site/src/contexts/useWebpushNotifications.ts @@ -21,14 +21,9 @@ export const useWebpushNotifications = (): WebpushNotifications => { const [subscribed, setSubscribed] = useState(false); const [loading, setLoading] = useState(true); - const [enabled, setEnabled] = useState(false); + const enabled = enabledExperimentsQuery.data?.includes("web-push") ?? false; useEffect(() => { - // Check if the experiment is enabled. - if (enabledExperimentsQuery.data?.includes("web-push")) { - setEnabled(true); - } - // Check if browser supports push notifications if (!("Notification" in window) || !("serviceWorker" in navigator)) { setSubscribed(false); @@ -50,7 +45,7 @@ export const useWebpushNotifications = (): WebpushNotifications => { }; checkSubscription(); - }, [enabledExperimentsQuery.data]); + }, []); const subscribe = async (): Promise => { try { diff --git a/site/src/modules/hooks/useSyncFormParameters.ts b/site/src/modules/hooks/useSyncFormParameters.ts index 13146b99dd..b48dcbcd2b 100644 --- a/site/src/modules/hooks/useSyncFormParameters.ts +++ b/site/src/modules/hooks/useSyncFormParameters.ts @@ -20,9 +20,7 @@ export function useSyncFormParameters({ // Keep track of form values in a ref to avoid unnecessary updates to rich_parameter_values const formValuesRef = useRef(formValues); - useEffect(() => { - formValuesRef.current = formValues; - }, [formValues]); + formValuesRef.current = formValues; useEffect(() => { if (!parameters) return; diff --git a/site/src/pages/AgentsPage/AgentsSidebar.tsx b/site/src/pages/AgentsPage/AgentsSidebar.tsx index d9903357b9..3616e81b5e 100644 --- a/site/src/pages/AgentsPage/AgentsSidebar.tsx +++ b/site/src/pages/AgentsPage/AgentsSidebar.tsx @@ -1106,13 +1106,8 @@ const LoadMoreSentinel: FC<{ // Keep refs in sync with the latest prop values so the // observer callback always reads current state without // needing to tear down and re-create the observer. - useEffect(() => { - onLoadMoreRef.current = onLoadMore; - }, [onLoadMore]); - - useEffect(() => { - isFetchingNextPageRef.current = isFetchingNextPage; - }, [isFetchingNextPage]); + onLoadMoreRef.current = onLoadMore; + isFetchingNextPageRef.current = isFetchingNextPage; useEffect(() => { const el = sentinelRef.current;