合併前如何檢查 AI 建議的 React hooks

AI is weirdly confident about React hooks. Ask for “extract the data fetching into a custom hook” and you’ll get something that looks idiomatic: useSomething, a useEffect, a couple of useMemos, maybe a useCallback for good measure. Types pass. The demo works once. Then you merge and discover the effect fires twice on every keystroke, or the cleanup never ran, or the hook is just a wrapper around three lines that didn’t need a name.

I treat AI-suggested hooks like AI-suggested refactors: useful drafts, not mergeable by default. Here’s the smell checklist I run before I accept the branch.

Smell 1: useEffect dependency lies

The model loves this pattern:

useEffect(() => {
  fetchUser(userId).then(setUser);
}, []); // "run once on mount"

Except userId changed three routes ago, and the effect still holds the first id. Or the opposite lie:

useEffect(() => {
  analytics.track("view", { id: item.id });
}, [item]); // object identity thrash

Every parent re-render invents a new item object, so “track once” becomes “track forever.”

What I check: for every useEffect, I ask “what should actually retrigger this?” If the answer is “once,” I want an explicit id/primitive in the deps, or an abortable fetch keyed by that id — not an empty array with a closed-over value. If the answer is “when X changes,” I want X to be a stable primitive or a memoized value I trust, not a fresh object from JSX.

Smell 2: unstable callbacks as dependencies

Classic AI move: wrap a handler in useCallback, then put that callback in another hook’s deps… while the callback itself depends on props that change every render.

const onSave = useCallback(() => {
  save(formState);
}, [formState]); // formState changes constantly

useEffect(() => {
  window.addEventListener("keydown", onSave);
  return () => window.removeEventListener("keydown", onSave);
}, [onSave]); // rebinds every keystroke of formState

useCallback didn’t stabilize anything. It just moved the thrash one layer down.

What I check: if a callback is only a dep because another effect needs it, I prefer reading the latest value from a ref, or narrowing the callback’s deps to the primitives it actually needs. “Add useCallback” is not a fix by itself.

Smell 3: derived state stuffed into useState

The agent sees fullName used in three places and invents:

const [fullName, setFullName] = useState("");
useEffect(() => {
  setFullName(`${first} ${last}`);
}, [first, last]);

That’s derived data. It doesn’t need state. It needs:

const fullName = `${first} ${last}`;

Or a useMemo if the derivation is actually expensive (it almost never is for string concat).

What I check: any useState + useEffect pair that only syncs from other props/state is guilty until proven otherwise. AI does this a lot because “state” feels like the React vocabulary for “a value I can name.”

Smell 4: missing cleanup (or fake cleanup)

Fetch-in-effect without abort. Subscriptions without unsubscribe. setTimeout without clear. The demo looks fine because you never navigate away mid-request.

useEffect(() => {
  let cancelled = false;
  load(id).then((data) => {
    if (!cancelled) setData(data);
  });
  return () => {
    cancelled = true;
  };
}, [id]);

That’s the minimum. Better: AbortController so the network work actually stops. AI often ships the happy path only.

What I check: every effect that starts async work, timers, or listeners must have a cleanup that matches. I also watch for “cleanup” that only sets a flag but still lets expensive work continue — fine for setState races, not fine for upload loops.

Smell 5: over-abstraction into useX

Not every three lines deserve a hook.

function useIsOpen(initial = false) {
  const [open, setOpen] = useState(initial);
  return { open, open: () => setOpen(true), close: () => setOpen(false) };
}

Cute. Also a digression in code review. Custom hooks earn their name when they encapsulate a non-obvious contract: syncing with a store, coordinating multiple effects, sharing behavior across distant components. “Boolean with open/close” is just state.

What I check: would I explain this hook in a PR description without sounding silly? If the hook exists only so the component file is shorter, I inline it. AI loves extracting; I love deleting.

Smell 6: stale closures dressed as “helpers”

function useSearch(items: Item[]) {
  const [query, setQuery] = useState("");
  const filter = () => items.filter((i) => i.name.includes(query));
  return { query, setQuery, results: filter() };
}

Looks fine until filter gets passed to a child as a callback and the child memoizes it, or until someone moves filter into a useEffect with []. Closures over items and query go stale in ways that are hard to see in a generated diff.

What I check: values computed from props/state should either be plain expressions in render, or memoized with the right deps — not casually closed over inside nested functions the agent plans to “reuse.”

Smell 7: “just add another useMemo”

When something re-renders too often, the model’s first instinct is memoization:

const sorted = useMemo(() => [...items].sort(byName), [items]);
const visible = useMemo(() => sorted.filter(isVisible), [sorted, isVisible]);
const rows = useMemo(() => visible.map(toRow), [visible]);

Three memos, still thrashing if items is a new array every parent render. Memo is a tax you pay after you’ve fixed identity and state shape — not a perfume you spray on the smell.

What I check: before accepting a memo, I ask what identity is unstable upstream. Fix that first. Keep useMemo for real work (big lists, expensive pure transforms), not for obj.a + obj.b.

Smell 8: hooks that hide side effects

The scariest AI hooks look pure in the name and impure in the body:

function useCartTotal(cart: Cart) {
  useEffect(() => {
    localStorage.setItem("cart-total", String(cart.total));
  }, [cart.total]);
  return cart.lines.reduce((s, l) => s + l.price, 0);
}

Callers think they’re reading a number. They’re also writing storage. Side effects buried in “getter” hooks are how you get “why did navigating to settings clear my draft?” bugs.

What I check: naming. useX that returns data should not silently write, fetch, or mutate globals. If it does I/O, the name should say so (usePersistCartTotal) and the call site should look intentional.

My merge gate (short version)

Before I merge an AI-touched hook, I want yes on all of these:

1. Deps match the real trigger (no empty-array lies, no object thrash)
2. No useState for values that are pure derivations
3. Async / listeners / timers have matching cleanup
4. The hook earns its name (shared or non-obvious contract)
5. No drive-by useMemo / useCallback pile-on
6. Side effects are obvious from the name and the call site

If two or more fail, I don’t “tweak the hook.” I ask the agent for a smaller change: inline the logic, or rewrite the effect with an explicit contract in the prompt.

Prompt that produces fewer smelly hooks

When I do want a custom hook from the model, I fence it:

Extract a hook only if the behavior is reused or the effect graph is non-trivial.

Prefer derived values over syncing state in effects.

Every useEffect must list real deps and a cleanup.

Do not add useMemo/useCallback unless you can point to a measured or structural reason.

No hidden I/O in hooks named like getters.

That doesn’t make the model perfect. It makes the first draft close enough that the smell checklist is a quick pass instead of a rewrite.

Hooks are easy to generate and expensive to debug after merge. Five minutes of smell-checking beats an afternoon of “why does this fire twice in Strict Mode only on the settings page.”