mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
refactor(site): remove derivable useEffect antipatterns (#23267)
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 🤖
This commit is contained in:
@@ -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<PropsWithChildren> = ({ children }) => {
|
||||
// proxy.
|
||||
const [userSavedProxy, setUserSavedProxy] = useState(loadUserSelectedProxy());
|
||||
|
||||
// Load the initial state from local storage.
|
||||
const [proxy, setProxy] = useState<PreferredProxy>(
|
||||
computeUsableURLS(userSavedProxy),
|
||||
);
|
||||
|
||||
const { permissions } = useAuthenticated();
|
||||
const { metadata } = useEmbeddedMetadata();
|
||||
|
||||
@@ -131,43 +126,30 @@ export const ProxyProvider: FC<PropsWithChildren> = ({ 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<PropsWithChildren> = ({ children }) => {
|
||||
|
||||
if (best?.proxy) {
|
||||
saveUserSelectedProxy(best.proxy);
|
||||
updateProxy();
|
||||
setUserSavedProxy(best.proxy);
|
||||
}
|
||||
}, [latenciesLoaded, proxiesResp, proxyLatencies]);
|
||||
|
||||
@@ -199,15 +181,12 @@ export const ProxyProvider: FC<PropsWithChildren> = ({ 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);
|
||||
},
|
||||
}}
|
||||
>
|
||||
|
||||
@@ -21,14 +21,9 @@ export const useWebpushNotifications = (): WebpushNotifications => {
|
||||
|
||||
const [subscribed, setSubscribed] = useState<boolean>(false);
|
||||
const [loading, setLoading] = useState<boolean>(true);
|
||||
const [enabled, setEnabled] = useState<boolean>(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<void> => {
|
||||
try {
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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;
|
||||
|
||||
Reference in New Issue
Block a user