|
| 1 | +--- |
| 2 | +name: you-might-not-need-an-effect |
| 3 | +description: Review React code for unnecessary or misused useEffect/useLayoutEffect and suggest smaller alternatives. Use when adding a new effect, reviewing changed React files, or cleaning up effect-heavy components and hooks. |
| 4 | +--- |
| 5 | + |
| 6 | +# You Might Not Need an Effect |
| 7 | + |
| 8 | +Use this as a knowledge skill for `useEffect` / `useLayoutEffect`. |
| 9 | + |
| 10 | +Before judging an effect, read these examples: |
| 11 | + |
| 12 | +- [Anti-patterns and legitimate cases](anti-patterns.md) |
| 13 | + |
| 14 | +## Scope |
| 15 | + |
| 16 | +- If the user supplied paths, scan those paths. |
| 17 | +- Otherwise, scan changed React source files first. |
| 18 | +- If there are no changed React source files, scan the project's React source directories. |
| 19 | +- Exclude tests, snapshots, build output, and dependencies. |
| 20 | + |
| 21 | +Use exact search for hooks: |
| 22 | + |
| 23 | +```bash |
| 24 | +rg -n "use(LayoutEffect|Effect)\\s*\\(" <scope> --glob '*.{ts,tsx,js,jsx}' |
| 25 | +``` |
| 26 | + |
| 27 | +## How to think |
| 28 | + |
| 29 | +Read the whole file. Do not judge from grep output alone. |
| 30 | + |
| 31 | +Ask this first: |
| 32 | + |
| 33 | +- If the code should run because the component was **displayed**, an effect may be appropriate. |
| 34 | +- If the code should run because the user **interacted**, it usually belongs in an event handler. |
| 35 | + |
| 36 | +For each effect, decide whether it should be: |
| 37 | + |
| 38 | +- **deleted** — unnecessary effect |
| 39 | +- **rewritten** — real synchronization exists, but there is a smaller or safer pattern |
| 40 | +- **kept** — legitimate effect |
| 41 | + |
| 42 | +Prefer the smallest replacement in this order: |
| 43 | + |
| 44 | +1. existing codebase helper or pattern |
| 45 | +2. render-time derivation |
| 46 | +3. event handler |
| 47 | +4. `useMemo` |
| 48 | +5. `useSyncExternalStore` |
| 49 | +6. effect with cleanup |
| 50 | + |
| 51 | +False positives are worse than missing one borderline case. |
| 52 | + |
| 53 | +## Common anti-patterns |
| 54 | + |
| 55 | +### 1. Derived state |
| 56 | + |
| 57 | +**Detect:** the effect sets state that can be computed from props, query data, loader data, or other state already available during render. |
| 58 | + |
| 59 | +**Fix:** compute inline. If expensive, use `useMemo`. |
| 60 | + |
| 61 | +### 2. Event logic in effect |
| 62 | + |
| 63 | +**Detect:** the effect shows a toast, navigates, submits, opens/closes UI, or logs an action because some state changed after a click or submit. |
| 64 | + |
| 65 | +**Fix:** move the side effect to the event handler that caused it. |
| 66 | + |
| 67 | +### 3. POST / mutation in effect |
| 68 | + |
| 69 | +**Detect:** the effect sends a POST request, mutation, or write operation because some submit-related state changed. |
| 70 | + |
| 71 | +**Fix:** if the request should happen because the user clicked or submitted, do it in the event handler, not in an effect triggered by state. |
| 72 | + |
| 73 | +### 4. Parent notification in effect |
| 74 | + |
| 75 | +**Detect:** the effect calls `onChange`, `onSelect`, `onLoaded`, `onFetched`, or similar callback props after local state changes. |
| 76 | + |
| 77 | +**Fix:** call the callback in the same event handler that updates local state, or lift the state/data to the parent. |
| 78 | + |
| 79 | +### 5. Passing data to the parent in an effect |
| 80 | + |
| 81 | +**Detect:** a child fetches or computes data, then pushes it upward with `onFetched`, `onData`, or a setter inside an effect. |
| 82 | + |
| 83 | +**Fix:** if parent and child need the same data, fetch in the parent and pass it down. |
| 84 | + |
| 85 | +### 6. Prop-driven resets |
| 86 | + |
| 87 | +**Detect:** the effect resets all or part of local state when an identity prop changes. |
| 88 | + |
| 89 | +**Fix:** prefer `key={identity}` for full resets; otherwise derive the value during render. |
| 90 | + |
| 91 | +### 7. Imperative head management |
| 92 | + |
| 93 | +**Detect:** the effect sets `document.title`, touches `document.head`, or manages favicon `<link>` tags. |
| 94 | + |
| 95 | +**Fix:** prefer the framework or app's declarative head mechanism. If the title is derived from render data, do not mirror it through state + effect. |
| 96 | + |
| 97 | +### 8. App initialization in effect |
| 98 | + |
| 99 | +**Detect:** `useEffect(..., [])` doing one-time startup work. |
| 100 | + |
| 101 | +**Fix:** keep only when React must coordinate with an external client or mounted browser lifecycle. Otherwise prefer work that happens before component effects: module-level setup, app startup/bootstrap code, route loaders, or auth initialization. |
| 102 | + |
| 103 | +In AppShell specifically, remember it embeds React Router. Before using a mount effect for initialization, ask whether the work belongs earlier in the lifecycle: |
| 104 | + |
| 105 | +- **route loader** — page data, route-gated checks, redirects, URL-driven preloading |
| 106 | +- **auth initialization** — session restore, callback handling, login state checks |
| 107 | +- **app startup / bootstrap** — one-time client setup, configuration loading, global wiring |
| 108 | + |
| 109 | +If the work does not require mounted DOM or browser-only lifecycle timing, it probably should not start in `useEffect`. |
| 110 | + |
| 111 | +### 9. External subscriptions |
| 112 | + |
| 113 | +**Detect:** the effect uses `matchMedia`, `addEventListener`, `subscribe`, or similar to mirror long-lived external state into React state. |
| 114 | + |
| 115 | +**Fix:** prefer `useSyncExternalStore` or an existing shared hook. Keep component-scoped imperative integration with proper cleanup when it is truly integration code. |
| 116 | + |
| 117 | +### 10. Fetch without cleanup |
| 118 | + |
| 119 | +**Detect:** async work in an effect updates state after completion but has no abort/ignore cleanup. |
| 120 | + |
| 121 | +**Fix:** add abort/ignore cleanup, or move the fetch to an event handler if it is event-driven. |
| 122 | + |
| 123 | +### 11. Effect chains |
| 124 | + |
| 125 | +**Detect:** multiple effects update state mainly to trigger other effects, forming a chain of cascading renders. |
| 126 | + |
| 127 | +**Fix:** derive what you can during render, and compute the next state in the event handler instead of chaining effects together. |
| 128 | + |
| 129 | +## Legitimate effects — usually keep |
| 130 | + |
| 131 | +Skip these when they are implemented correctly: |
| 132 | + |
| 133 | +- synchronizing with an external system that exists outside React |
| 134 | +- component-scoped timers, cleanup, cancellation, or resource release |
| 135 | +- focus management on mount |
| 136 | +- ref-scoped DOM observers/listeners with cleanup |
| 137 | +- analytics that should fire on page display rather than a user action |
| 138 | +- mount/unmount registration with cleanup |
| 139 | + |
| 140 | +## Output |
| 141 | + |
| 142 | +Do not force a rigid report format. |
| 143 | + |
| 144 | +- If the user asked for review, give a short list of real findings with file, reason, and smallest fix. |
| 145 | +- If the user asked for implementation, apply the smallest safe fix. |
| 146 | +- If an effect is legitimate, say so briefly and move on. |
| 147 | + |
| 148 | +## Notes |
| 149 | + |
| 150 | +- Prefer one shared/root fix over patching multiple callers. |
| 151 | +- Prefer existing repo patterns over new abstractions. |
| 152 | +- If the codebase already has a declarative way to handle head state, routing side effects, or subscriptions, reuse it. |
| 153 | + |
| 154 | +## Reference |
| 155 | + |
| 156 | +- [React docs — You Might Not Need an Effect](https://react.dev/learn/you-might-not-need-an-effect) |
0 commit comments