Skip to content

Commit bf885cc

Browse files
DanielleMaywoodDanielleMaywood
authored
fix(site/src): replace misused useEffectEvent with correct patterns (#24525)
1 parent 7f1b9cb commit bf885cc

12 files changed

Lines changed: 134 additions & 133 deletions

File tree

‎site/src/hooks/useClickable.ts‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@ import {
22
type KeyboardEventHandler,
33
type MouseEventHandler,
44
type RefObject,
5-
useEffectEvent,
65
useRef,
76
} from "react";
87

@@ -44,11 +43,10 @@ export const useClickable = <
4443
role?: TRole,
4544
): UseClickableResult<TElement, TRole> => {
4645
const ref = useRef<TElement>(null);
47-
const onClickEvent = useEffectEvent(onClick);
4846

4947
return {
5048
ref,
51-
onClick: onClickEvent,
49+
onClick,
5250
tabIndex: 0,
5351
role: (role ?? "button") as TRole,
5452

‎site/src/hooks/useClipboard.test.tsx‎

Lines changed: 5 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -260,10 +260,11 @@ describe.each(secureContextValues)("useClipboard - secure: %j", (isSecure) => {
260260
expect(result.current.error).toBeUndefined();
261261
});
262262

263-
// This test case is really important to ensure that it's easy to plop this
264-
// inside of useEffect calls without having to think about dependencies too
265-
// much
266-
it("Ensures that the copyToClipboard function always maintains a stable reference across all re-renders", async () => {
263+
// This test case verifies that copyToClipboard maintains a stable
264+
// reference across re-renders so it can be used safely in useEffect
265+
// dependency arrays. Stability requires that onError and
266+
// clearErrorOnSuccess are themselves stable references.
267+
it("Ensures that the copyToClipboard function maintains a stable reference when its inputs are stable", async () => {
267268
const initialOnError = vi.fn();
268269
const { result, rerender } = renderUseClipboard({
269270
onError: initialOnError,
@@ -276,17 +277,6 @@ describe.each(secureContextValues)("useClipboard - secure: %j", (isSecure) => {
276277
rerender({ onError: initialOnError });
277278
expect(result.current.copyToClipboard).toBe(initialCopy);
278279

279-
// Re-render with new onError prop and then swap back to simplify
280-
// testing
281-
rerender({ onError: vi.fn() });
282-
expect(result.current.copyToClipboard).toBe(initialCopy);
283-
rerender({ onError: initialOnError });
284-
285-
// Re-render with a new clear value then swap back to simplify testing
286-
rerender({ onError: initialOnError, clearErrorOnSuccess: false });
287-
expect(result.current.copyToClipboard).toBe(initialCopy);
288-
rerender({ onError: initialOnError, clearErrorOnSuccess: true });
289-
290280
// Trigger a failed clipboard interaction
291281
setSimulateFailure(true);
292282
await act(() => result.current.copyToClipboard("dummy-text-2"));

‎site/src/hooks/useClipboard.ts‎

Lines changed: 36 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,4 @@
1-
import {
2-
useCallback,
3-
useEffect,
4-
useEffectEvent,
5-
useRef,
6-
useState,
7-
} from "react";
1+
import { useCallback, useEffect, useRef, useState } from "react";
82
import { toast } from "sonner";
93

104
const CLIPBOARD_TIMEOUT_MS = 1_000;
@@ -44,55 +38,50 @@ export type UseClipboardResult = Readonly<{
4438
export const useClipboard = (
4539
input: UseClipboardInput = {},
4640
): UseClipboardResult => {
47-
const {
48-
onError = (msg: string) => toast.error(msg),
49-
clearErrorOnSuccess = true,
50-
} = input;
41+
const { onError = toast.error, clearErrorOnSuccess = true } = input;
5142

5243
const [showCopiedSuccess, setShowCopiedSuccess] = useState(false);
5344
const [error, setError] = useState<Error>();
5445
const timeoutIdRef = useRef<number | undefined>(undefined);
5546

5647
useEffect(() => {
57-
const clearTimeoutOnUnmount = () => {
58-
window.clearTimeout(timeoutIdRef.current);
59-
};
60-
return clearTimeoutOnUnmount;
48+
return () => window.clearTimeout(timeoutIdRef.current);
6149
}, []);
6250

63-
const onErrorEvent = useEffectEvent(() => onError(COPY_FAILED_MESSAGE));
64-
const handleSuccessfulCopy = useEffectEvent(() => {
65-
setShowCopiedSuccess(true);
66-
if (clearErrorOnSuccess) {
67-
setError(undefined);
68-
}
69-
70-
timeoutIdRef.current = window.setTimeout(() => {
71-
setShowCopiedSuccess(false);
72-
}, CLIPBOARD_TIMEOUT_MS);
73-
});
74-
75-
const copyToClipboard = useCallback(async (textToCopy: string) => {
76-
try {
77-
await window.navigator.clipboard.writeText(textToCopy);
78-
handleSuccessfulCopy();
79-
} catch (err) {
80-
const fallbackCopySuccessful = simulateClipboardWrite(textToCopy);
81-
if (fallbackCopySuccessful) {
82-
handleSuccessfulCopy();
83-
return;
51+
const copyToClipboard = useCallback(
52+
async (textToCopy: string) => {
53+
const markSuccess = () => {
54+
setShowCopiedSuccess(true);
55+
if (clearErrorOnSuccess) {
56+
setError(undefined);
57+
}
58+
timeoutIdRef.current = window.setTimeout(() => {
59+
setShowCopiedSuccess(false);
60+
}, CLIPBOARD_TIMEOUT_MS);
61+
};
62+
63+
try {
64+
await window.navigator.clipboard.writeText(textToCopy);
65+
markSuccess();
66+
} catch (err) {
67+
const fallbackCopySuccessful = simulateClipboardWrite(textToCopy);
68+
if (fallbackCopySuccessful) {
69+
markSuccess();
70+
return;
71+
}
72+
73+
const wrappedErr = new Error(COPY_FAILED_MESSAGE);
74+
if (err instanceof Error) {
75+
wrappedErr.stack = err.stack;
76+
}
77+
78+
console.error(wrappedErr);
79+
setError(wrappedErr);
80+
onError(COPY_FAILED_MESSAGE);
8481
}
85-
86-
const wrappedErr = new Error(COPY_FAILED_MESSAGE);
87-
if (err instanceof Error) {
88-
wrappedErr.stack = err.stack;
89-
}
90-
91-
console.error(wrappedErr);
92-
setError(wrappedErr);
93-
onErrorEvent();
94-
}
95-
}, []);
82+
},
83+
[onError, clearErrorOnSuccess],
84+
);
9685

9786
return { showCopiedSuccess, error, copyToClipboard };
9887
};

‎site/src/modules/notifications/NotificationsInbox/NotificationsInbox.tsx‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { type FC, useEffect, useEffectEvent } from "react";
1+
import { type FC, useCallback, useEffect } from "react";
22
import { useMutation, useQuery, useQueryClient } from "react-query";
33
import { toast } from "sonner";
44
import { watchInboxNotifications } from "#/api/api";
@@ -40,7 +40,7 @@ export const NotificationsInbox: FC<NotificationsInboxProps> = ({
4040
queryFn: () => fetchNotifications(),
4141
});
4242

43-
const updateNotificationsCache = useEffectEvent(
43+
const updateNotificationsCache = useCallback(
4444
async (
4545
callback: (
4646
res: ListInboxNotificationsResponse,
@@ -57,6 +57,7 @@ export const NotificationsInbox: FC<NotificationsInboxProps> = ({
5757
},
5858
);
5959
},
60+
[queryClient],
6061
);
6162

6263
useEffect(() => {
@@ -85,7 +86,7 @@ export const NotificationsInbox: FC<NotificationsInboxProps> = ({
8586
});
8687

8788
return () => socket.close();
88-
}, []);
89+
}, [updateNotificationsCache]);
8990

9091
const {
9192
mutate: loadMoreNotifications,

‎site/src/pages/AgentsPage/components/AgentCreateForm.tsx‎

Lines changed: 18 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -310,26 +310,24 @@ export const AgentCreateForm: FC<AgentCreateFormProps> = ({
310310
? selectedWorkspaceId
311311
: null;
312312

313-
const handleSend = useEffectEvent(
314-
async (message: string, fileIDs?: string[]) => {
315-
submitDraft();
316-
await onCreateChat({
317-
message,
318-
fileIDs,
319-
workspaceId: effectiveWorkspaceId ?? undefined,
320-
model: selectedModel || undefined,
321-
organizationId,
322-
mcpServerIds:
323-
effectiveMCPServerIds.length > 0
324-
? [...effectiveMCPServerIds]
325-
: undefined,
326-
planMode: planModeEnabled ? "plan" : undefined,
327-
}).catch((err) => {
328-
resetDraft();
329-
throw err;
330-
});
331-
},
332-
);
313+
const handleSend = async (message: string, fileIDs?: string[]) => {
314+
submitDraft();
315+
await onCreateChat({
316+
message,
317+
fileIDs,
318+
workspaceId: effectiveWorkspaceId ?? undefined,
319+
model: selectedModel || undefined,
320+
organizationId,
321+
mcpServerIds:
322+
effectiveMCPServerIds.length > 0
323+
? [...effectiveMCPServerIds]
324+
: undefined,
325+
planMode: planModeEnabled ? "plan" : undefined,
326+
}).catch((err) => {
327+
resetDraft();
328+
throw err;
329+
});
330+
};
333331

334332
const {
335333
attachments,

‎site/src/pages/AgentsPage/components/ChatConversation/useChatStore.ts‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,10 @@
1-
import { useEffect, useEffectEvent, useRef, useState } from "react";
1+
import {
2+
useCallback,
3+
useEffect,
4+
useEffectEvent,
5+
useRef,
6+
useState,
7+
} from "react";
28
import { type InfiniteData, useQueryClient } from "react-query";
39
import { watchChat } from "#/api/api";
410
import { chatMessagesKey, updateInfiniteChatsCache } from "#/api/queries/chats";
@@ -131,7 +137,7 @@ export const useChatStore = (
131137
// last REST fetch, and structural sharing can suppress the
132138
// refetch-driven store update when no new durable messages
133139
// have been committed to the DB yet.
134-
const upsertCacheMessages = useEffectEvent(
140+
const upsertCacheMessages = useCallback(
135141
(messages: readonly TypesGen.ChatMessage[]) => {
136142
if (!chatID || messages.length === 0) {
137143
return;
@@ -172,6 +178,7 @@ export const useChatStore = (
172178
};
173179
});
174180
},
181+
[chatID, queryClient],
175182
);
176183

177184
useEffect(() => {
@@ -626,7 +633,7 @@ export const useChatStore = (
626633
}
627634
activeChatIDRef.current = null;
628635
};
629-
}, [chatID, initialDataLoaded, queryClient, store]);
636+
}, [chatID, initialDataLoaded, queryClient, store, upsertCacheMessages]);
630637
return {
631638
store,
632639
clearStreamError: () => {

‎site/src/pages/AgentsPage/components/ChatScrollContainer.tsx‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -120,20 +120,20 @@ function useStickToBottom(): StickToBottomInstance {
120120
});
121121

122122
// Sync helpers — keep mutable state and React state in lockstep.
123-
const syncIsAtBottom = useEffectEvent((v: boolean) => {
123+
const syncIsAtBottom = (v: boolean) => {
124124
stateRef.current.internalIsAtBottom = v;
125125
setIsAtBottom(v);
126-
});
126+
};
127127

128-
const syncEscapedFromLock = useEffectEvent((v: boolean) => {
128+
const syncEscapedFromLock = (v: boolean) => {
129129
stateRef.current.escapedFromLock = v;
130-
});
130+
};
131131

132132
// -----------------------------------------------------------------------
133133
// scrollToBottom
134134
// -----------------------------------------------------------------------
135135

136-
const scrollToBottom = useEffectEvent((behavior?: ScrollBehavior) => {
136+
const scrollToBottom = (behavior?: ScrollBehavior) => {
137137
const s = stateRef.current;
138138
if (!s.scrollElement) return;
139139

@@ -153,7 +153,7 @@ function useStickToBottom(): StickToBottomInstance {
153153
// scroll (currentScrollTop > lastScrollTop), which
154154
// correctly clears escapedFromLock.
155155
}
156-
});
156+
};
157157

158158
const suppressNextResize = () => {
159159
stateRef.current.suppressNextResize = true;

‎site/src/pages/AgentsPage/hooks/useFileAttachments.ts‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -190,12 +190,13 @@ export function useFileAttachments(
190190
() => new Map<File, string>(),
191191
);
192192

193-
// Revoke blob URLs on unmount to prevent memory leaks.
194193
const revokePreviewUrls = useEffectEvent(() => {
195194
for (const [, url] of previewUrls) {
196195
if (url.startsWith("blob:")) URL.revokeObjectURL(url);
197196
}
198197
});
198+
199+
// Revoke blob URLs on unmount to prevent memory leaks.
199200
useEffect(() => {
200201
return () => revokePreviewUrls();
201202
}, []);
@@ -345,7 +346,9 @@ export function useFileAttachments(
345346
};
346347

347348
const resetAttachments = () => {
348-
revokePreviewUrls();
349+
for (const [, url] of previewUrls) {
350+
if (url.startsWith("blob:")) URL.revokeObjectURL(url);
351+
}
349352
setPreviewUrls(new Map());
350353
setTextContents(new Map());
351354
setUploadStates(new Map());

‎site/src/pages/CreateWorkspacePage/CreateWorkspacePage.tsx‎

Lines changed: 14 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -99,19 +99,20 @@ const CreateWorkspacePage: FC = () => {
9999

100100
const autofillParameters = getAutofillParameters(searchParams);
101101

102-
const sendMessage = useEffectEvent(
103-
(formValues: Record<string, string>, ownerId?: string) => {
104-
const request: DynamicParametersRequest = {
105-
id: wsResponseId.current + 1,
106-
owner_id: ownerId ?? owner.id,
107-
inputs: formValues,
108-
};
109-
if (ws.current && ws.current.readyState === WebSocket.OPEN) {
110-
ws.current.send(JSON.stringify(request));
111-
wsResponseId.current = wsResponseId.current + 1;
112-
}
113-
},
114-
);
102+
const sendMessage = (
103+
formValues: Record<string, string>,
104+
ownerId?: string,
105+
) => {
106+
const request: DynamicParametersRequest = {
107+
id: wsResponseId.current + 1,
108+
owner_id: ownerId ?? owner.id,
109+
inputs: formValues,
110+
};
111+
if (ws.current && ws.current.readyState === WebSocket.OPEN) {
112+
ws.current.send(JSON.stringify(request));
113+
wsResponseId.current = wsResponseId.current + 1;
114+
}
115+
};
115116

116117
// On page load, sends all initial parameter values to the websocket
117118
// (including defaults and autofilled from the url)

0 commit comments

Comments
 (0)