fix(site/src): replace misused useEffectEvent with correct patterns#24525
Conversation
778c736 to
dbffe4d
Compare
| const [searchParams, setSearchParams] = useSearchParams(); | ||
| const setSearchParamsEvent = useEffectEvent(setSearchParams); | ||
|
|
||
| // Need this to be a tuple type, but can't use "as const", because that would | ||
| // make the whole array readonly and cause type mismatches downstream | ||
| return [searchParams, setSearchParamsEvent] as ReturnType< | ||
| const setterRef = useRef(setSearchParams); | ||
| useLayoutEffect(() => { | ||
| setterRef.current = setSearchParams; | ||
| }, [setSearchParams]); | ||
| const stableSetSearchParams = useCallback( | ||
| (...args: Parameters<typeof setSearchParams>) => setterRef.current(...args), | ||
| [], | ||
| ); | ||
| return [searchParams, stableSetSearchParams] as ReturnType< | ||
| typeof useSearchParams | ||
| >; |
There was a problem hiding this comment.
eslint isn't happy with useEffectEvent here, falling this back to how our old useEffectEvent polyfill was defined.
| // refetch-driven store update when no new durable messages | ||
| // have been committed to the DB yet. | ||
| const upsertCacheMessages = useEffectEvent( | ||
| const upsertCacheMessages = useCallback( |
There was a problem hiding this comment.
eslint wasn't happy with useEffectEvent here either. biome wasn't happy with making this a bare function so adding useCallback.
| role?: TRole, | ||
| ): UseClickableResult<TElement, TRole> => { | ||
| const ref = useRef<TElement>(null); | ||
| const onClickEvent = useEffectEvent(onClick); |
There was a problem hiding this comment.
We should just ensure the caller makes their onClick be referentially stable
6161f3a to
ee7edda
Compare
| // Re-render with new onError prop and then swap back to simplify | ||
| // testing | ||
| rerender({ onError: vi.fn() }); | ||
| expect(result.current.copyToClipboard).toBe(initialCopy); | ||
| rerender({ onError: initialOnError }); | ||
|
|
||
| // Re-render with a new clear value then swap back to simplify testing | ||
| rerender({ onError: initialOnError, clearErrorOnSuccess: false }); | ||
| expect(result.current.copyToClipboard).toBe(initialCopy); | ||
| rerender({ onError: initialOnError, clearErrorOnSuccess: true }); | ||
|
|
There was a problem hiding this comment.
useClipboard no longer internally stabilizes the ref, so this test is outdated
There was a problem hiding this comment.
🤖 This comment was written by Coder Agent on behalf of Danielle Maywood 🤖
Fixed in 91548af. Rewrote the stability test comment to describe the new conditional-stability behaviour, and removed the assertions that tested stability with an unstable onError input (those assertions now fail by design — if onError is unstable, copyToClipboard is legitimately unstable).
ee7edda to
ccb3094
Compare
ccb3094 to
665d014
Compare
mafredri
left a comment
There was a problem hiding this comment.
First-pass review (Netero only, the full panel has not yet reviewed).
Clean approach to removing misused useEffectEvent calls. The motivation is sound: aligning with React's upcoming compiler requirements. Convention scan is clean, types check out, most replacements are correct.
Two P1 findings where callers pass unstable onError callbacks, causing effect teardown loops. These are mechanical issues the panel would spend time rediscovering, so flagging them now.
Severity count: 2 P1, 1 P3, 1 Nit.
site/src/pages/AgentsPage/hooks/useDesktopConnection.ts:122
P1 [DEREM-1] useClipboard now uses useCallback([onError, clearErrorOnSuccess]). The inline onError: () => { toast.error(...) } here is a new reference every render, so copyToClipboard (aliased syncRemoteClipboardToLocal) gets a new reference every render. This reference is in the useEffect dependency array at line 501: [activated, chatId, syncRemoteClipboardToLocal]. The effect tears down the RFB connection (cleanupRfb(), teardown()) and recreates it every render.
Before this PR, useEffectEvent gave copyToClipboard a stable identity regardless of onError changes. The PR description states this file was updated, but it has no changes in this diff.
Fix: wrap the onError callback in useCallback with [] deps.
🤖
site/src/pages/AgentsPage/components/TerminalPanel.tsx:67
P1 [DEREM-2] handleTerminalError is a plain inline function (new reference every render). It is passed as onError to WorkspaceTerminal. reportTerminalError is now useCallback([onError]), so it changes identity every render. reportTerminalError was added to both useEffect dep arrays (lines 268 and 471). Both effects tear down and recreate the terminal and WebSocket on every render.
TerminalPage.tsx already wraps its handleTerminalError in useCallback([], ...). Same treatment needed here.
🤖
site/src/hooks/useClipboard.test.tsx:266
P3 [DEREM-3] Test name: "Ensures that the copyToClipboard function always maintains a stable reference across all re-renders." This is no longer true. copyToClipboard now changes identity when onError or clearErrorOnSuccess change. The removed assertions were the ones that tested "all re-renders"; the remaining assertions only prove stability when deps are unchanged, which useCallback guarantees trivially.
Consider updating the test name and comment (lines 263-265) to reflect the new contract.
🤖
🤖 This review was automatically generated with Coder Agents.
665d014 to
69896cd
Compare
DEREM-1 — Pushback. DEREM-2 — Pushback. Both callers of
Neither caller causes an unstable DEREM-3 — Fixed in 69896cd. Test name and comment updated to reflect that stability is only guaranteed when inputs are stable. |
mafredri
left a comment
There was a problem hiding this comment.
Panel review (13 reviewers). Previous Netero-only findings addressed or closed.
DEREM-1 and DEREM-2 (P1s from R1) are closed. The React Compiler defense holds: both callers are under the compiler filter, which auto-stabilizes the inline callbacks. Five panel reviewers independently verified the filter path.
DEREM-3 was fixed. DEREM-4 closed by panel vote (duplication is correct; calling useEffectEvent from outside effects is the exact misuse this PR fixes).
The panel converged on one new P2 and three P3s. The P2 is about reportTerminalError in WorkspaceTerminal: it is only called from effect bodies (same pattern as handleOpenLink and handleStatusChange which were correctly left as useEffectEvent), but was converted to useCallback, creating a dep cascade that tears down the terminal and WebSocket when onError changes. Current callers are safe, but the component is outside the compiler filter and lost its internal stability safety net.
Severity count: 1 P2, 3 P3 (1 existing from Netero R4).
"Either all three should be useEffectEvent (they're all called from effects), or all three should be useCallback with their callback prop as a dep. The current split has no principle behind it." (Pariston)
🤖 This review was automatically generated with Coder Agents.
69896cd to
141022f
Compare
141022f to
91548af
Compare
mafredri
left a comment
There was a problem hiding this comment.
Status: APPROVED
All findings resolved. APPROVE.
R1 raised 2 P1s and 2 minor findings. The P1s were closed after the author demonstrated the React Compiler auto-stabilizes inline callbacks for both affected callers. R4's panel (13 reviewers) converged on a P2: reportTerminalError should remain useEffectEvent since it is only called from effect bodies, matching handleOpenLink and handleStatusChange in the same file. Three P3s for documentation gaps.
All four R4 findings were fixed in 141022f. R5's panel (4 reviewers) verified each fix and found no new issues. Netero scanned clean across all 8 sections.
11 findings total: 4 fixed, 4 closed by panel vote, 3 dropped by orchestrator. Zero open.
🤖 This review was automatically generated with Coder Agents.
useEffectEventis only valid insideuseEffectbodies. Calling it from event handlers or returning its result from hooks violates React's contract and is incompatible with React Compiler.constfunctions.useCallbackwith explicit deps.useClipboard.copyToClipboardis nowuseCallback([onError, clearErrorOnSuccess])with internal helpers inlined; callers are responsible for passing stable callbacks.useClickablereturnsonClickdirectly.-
useSafeSearchParamsusesuseRef + useLayoutEffect + useCallback([], ...)to provide a stablesetSearchParamswithout pinning to stale values.useFileAttachments.resetAttachmentsinlines the blob-revocation loop rather than calling auseEffectEventoutside an effect body.