diff --git a/site/src/hooks/useClickable.ts b/site/src/hooks/useClickable.ts index 26e7d62ff81f7..3f59851b8fc3b 100644 --- a/site/src/hooks/useClickable.ts +++ b/site/src/hooks/useClickable.ts @@ -2,7 +2,6 @@ import { type KeyboardEventHandler, type MouseEventHandler, type RefObject, - useEffectEvent, useRef, } from "react"; @@ -44,11 +43,10 @@ export const useClickable = < role?: TRole, ): UseClickableResult => { const ref = useRef(null); - const onClickEvent = useEffectEvent(onClick); return { ref, - onClick: onClickEvent, + onClick, tabIndex: 0, role: (role ?? "button") as TRole, diff --git a/site/src/hooks/useClipboard.test.tsx b/site/src/hooks/useClipboard.test.tsx index 4b7f538fad09b..1c4692e3d2602 100644 --- a/site/src/hooks/useClipboard.test.tsx +++ b/site/src/hooks/useClipboard.test.tsx @@ -260,10 +260,11 @@ describe.each(secureContextValues)("useClipboard - secure: %j", (isSecure) => { expect(result.current.error).toBeUndefined(); }); - // This test case is really important to ensure that it's easy to plop this - // inside of useEffect calls without having to think about dependencies too - // much - it("Ensures that the copyToClipboard function always maintains a stable reference across all re-renders", async () => { + // This test case verifies that copyToClipboard maintains a stable + // reference across re-renders so it can be used safely in useEffect + // dependency arrays. Stability requires that onError and + // clearErrorOnSuccess are themselves stable references. + it("Ensures that the copyToClipboard function maintains a stable reference when its inputs are stable", async () => { const initialOnError = vi.fn(); const { result, rerender } = renderUseClipboard({ onError: initialOnError, @@ -276,17 +277,6 @@ describe.each(secureContextValues)("useClipboard - secure: %j", (isSecure) => { rerender({ onError: initialOnError }); expect(result.current.copyToClipboard).toBe(initialCopy); - // 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 }); - // Trigger a failed clipboard interaction setSimulateFailure(true); await act(() => result.current.copyToClipboard("dummy-text-2")); diff --git a/site/src/hooks/useClipboard.ts b/site/src/hooks/useClipboard.ts index a6d7928523b42..e1ee01242e31b 100644 --- a/site/src/hooks/useClipboard.ts +++ b/site/src/hooks/useClipboard.ts @@ -1,10 +1,4 @@ -import { - useCallback, - useEffect, - useEffectEvent, - useRef, - useState, -} from "react"; +import { useCallback, useEffect, useRef, useState } from "react"; import { toast } from "sonner"; const CLIPBOARD_TIMEOUT_MS = 1_000; @@ -44,55 +38,50 @@ export type UseClipboardResult = Readonly<{ export const useClipboard = ( input: UseClipboardInput = {}, ): UseClipboardResult => { - const { - onError = (msg: string) => toast.error(msg), - clearErrorOnSuccess = true, - } = input; + const { onError = toast.error, clearErrorOnSuccess = true } = input; const [showCopiedSuccess, setShowCopiedSuccess] = useState(false); const [error, setError] = useState(); const timeoutIdRef = useRef(undefined); useEffect(() => { - const clearTimeoutOnUnmount = () => { - window.clearTimeout(timeoutIdRef.current); - }; - return clearTimeoutOnUnmount; + return () => window.clearTimeout(timeoutIdRef.current); }, []); - const onErrorEvent = useEffectEvent(() => onError(COPY_FAILED_MESSAGE)); - const handleSuccessfulCopy = useEffectEvent(() => { - setShowCopiedSuccess(true); - if (clearErrorOnSuccess) { - setError(undefined); - } - - timeoutIdRef.current = window.setTimeout(() => { - setShowCopiedSuccess(false); - }, CLIPBOARD_TIMEOUT_MS); - }); - - const copyToClipboard = useCallback(async (textToCopy: string) => { - try { - await window.navigator.clipboard.writeText(textToCopy); - handleSuccessfulCopy(); - } catch (err) { - const fallbackCopySuccessful = simulateClipboardWrite(textToCopy); - if (fallbackCopySuccessful) { - handleSuccessfulCopy(); - return; + const copyToClipboard = useCallback( + async (textToCopy: string) => { + const markSuccess = () => { + setShowCopiedSuccess(true); + if (clearErrorOnSuccess) { + setError(undefined); + } + timeoutIdRef.current = window.setTimeout(() => { + setShowCopiedSuccess(false); + }, CLIPBOARD_TIMEOUT_MS); + }; + + try { + await window.navigator.clipboard.writeText(textToCopy); + markSuccess(); + } catch (err) { + const fallbackCopySuccessful = simulateClipboardWrite(textToCopy); + if (fallbackCopySuccessful) { + markSuccess(); + return; + } + + const wrappedErr = new Error(COPY_FAILED_MESSAGE); + if (err instanceof Error) { + wrappedErr.stack = err.stack; + } + + console.error(wrappedErr); + setError(wrappedErr); + onError(COPY_FAILED_MESSAGE); } - - const wrappedErr = new Error(COPY_FAILED_MESSAGE); - if (err instanceof Error) { - wrappedErr.stack = err.stack; - } - - console.error(wrappedErr); - setError(wrappedErr); - onErrorEvent(); - } - }, []); + }, + [onError, clearErrorOnSuccess], + ); return { showCopiedSuccess, error, copyToClipboard }; }; diff --git a/site/src/modules/notifications/NotificationsInbox/NotificationsInbox.tsx b/site/src/modules/notifications/NotificationsInbox/NotificationsInbox.tsx index 952748a09f745..7cc6ee1943715 100644 --- a/site/src/modules/notifications/NotificationsInbox/NotificationsInbox.tsx +++ b/site/src/modules/notifications/NotificationsInbox/NotificationsInbox.tsx @@ -1,4 +1,4 @@ -import { type FC, useEffect, useEffectEvent } from "react"; +import { type FC, useCallback, useEffect } from "react"; import { useMutation, useQuery, useQueryClient } from "react-query"; import { toast } from "sonner"; import { watchInboxNotifications } from "#/api/api"; @@ -40,7 +40,7 @@ export const NotificationsInbox: FC = ({ queryFn: () => fetchNotifications(), }); - const updateNotificationsCache = useEffectEvent( + const updateNotificationsCache = useCallback( async ( callback: ( res: ListInboxNotificationsResponse, @@ -57,6 +57,7 @@ export const NotificationsInbox: FC = ({ }, ); }, + [queryClient], ); useEffect(() => { @@ -85,7 +86,7 @@ export const NotificationsInbox: FC = ({ }); return () => socket.close(); - }, []); + }, [updateNotificationsCache]); const { mutate: loadMoreNotifications, diff --git a/site/src/pages/AgentsPage/components/AgentCreateForm.tsx b/site/src/pages/AgentsPage/components/AgentCreateForm.tsx index a284e9323c476..3e57497388d57 100644 --- a/site/src/pages/AgentsPage/components/AgentCreateForm.tsx +++ b/site/src/pages/AgentsPage/components/AgentCreateForm.tsx @@ -310,26 +310,24 @@ export const AgentCreateForm: FC = ({ ? selectedWorkspaceId : null; - const handleSend = useEffectEvent( - async (message: string, fileIDs?: string[]) => { - submitDraft(); - await onCreateChat({ - message, - fileIDs, - workspaceId: effectiveWorkspaceId ?? undefined, - model: selectedModel || undefined, - organizationId, - mcpServerIds: - effectiveMCPServerIds.length > 0 - ? [...effectiveMCPServerIds] - : undefined, - planMode: planModeEnabled ? "plan" : undefined, - }).catch((err) => { - resetDraft(); - throw err; - }); - }, - ); + const handleSend = async (message: string, fileIDs?: string[]) => { + submitDraft(); + await onCreateChat({ + message, + fileIDs, + workspaceId: effectiveWorkspaceId ?? undefined, + model: selectedModel || undefined, + organizationId, + mcpServerIds: + effectiveMCPServerIds.length > 0 + ? [...effectiveMCPServerIds] + : undefined, + planMode: planModeEnabled ? "plan" : undefined, + }).catch((err) => { + resetDraft(); + throw err; + }); + }; const { attachments, diff --git a/site/src/pages/AgentsPage/components/ChatConversation/useChatStore.ts b/site/src/pages/AgentsPage/components/ChatConversation/useChatStore.ts index 9d20cda79c94c..e89bffbd2d348 100644 --- a/site/src/pages/AgentsPage/components/ChatConversation/useChatStore.ts +++ b/site/src/pages/AgentsPage/components/ChatConversation/useChatStore.ts @@ -1,4 +1,10 @@ -import { useEffect, useEffectEvent, useRef, useState } from "react"; +import { + useCallback, + useEffect, + useEffectEvent, + useRef, + useState, +} from "react"; import { type InfiniteData, useQueryClient } from "react-query"; import { watchChat } from "#/api/api"; import { chatMessagesKey, updateInfiniteChatsCache } from "#/api/queries/chats"; @@ -131,7 +137,7 @@ export const useChatStore = ( // last REST fetch, and structural sharing can suppress the // refetch-driven store update when no new durable messages // have been committed to the DB yet. - const upsertCacheMessages = useEffectEvent( + const upsertCacheMessages = useCallback( (messages: readonly TypesGen.ChatMessage[]) => { if (!chatID || messages.length === 0) { return; @@ -172,6 +178,7 @@ export const useChatStore = ( }; }); }, + [chatID, queryClient], ); useEffect(() => { @@ -626,7 +633,7 @@ export const useChatStore = ( } activeChatIDRef.current = null; }; - }, [chatID, initialDataLoaded, queryClient, store]); + }, [chatID, initialDataLoaded, queryClient, store, upsertCacheMessages]); return { store, clearStreamError: () => { diff --git a/site/src/pages/AgentsPage/components/ChatScrollContainer.tsx b/site/src/pages/AgentsPage/components/ChatScrollContainer.tsx index 4a130b7acb91f..5e8c5797b90d7 100644 --- a/site/src/pages/AgentsPage/components/ChatScrollContainer.tsx +++ b/site/src/pages/AgentsPage/components/ChatScrollContainer.tsx @@ -120,20 +120,20 @@ function useStickToBottom(): StickToBottomInstance { }); // Sync helpers — keep mutable state and React state in lockstep. - const syncIsAtBottom = useEffectEvent((v: boolean) => { + const syncIsAtBottom = (v: boolean) => { stateRef.current.internalIsAtBottom = v; setIsAtBottom(v); - }); + }; - const syncEscapedFromLock = useEffectEvent((v: boolean) => { + const syncEscapedFromLock = (v: boolean) => { stateRef.current.escapedFromLock = v; - }); + }; // ----------------------------------------------------------------------- // scrollToBottom // ----------------------------------------------------------------------- - const scrollToBottom = useEffectEvent((behavior?: ScrollBehavior) => { + const scrollToBottom = (behavior?: ScrollBehavior) => { const s = stateRef.current; if (!s.scrollElement) return; @@ -153,7 +153,7 @@ function useStickToBottom(): StickToBottomInstance { // scroll (currentScrollTop > lastScrollTop), which // correctly clears escapedFromLock. } - }); + }; const suppressNextResize = () => { stateRef.current.suppressNextResize = true; diff --git a/site/src/pages/AgentsPage/hooks/useFileAttachments.ts b/site/src/pages/AgentsPage/hooks/useFileAttachments.ts index 797db4c29c243..fb79db924c779 100644 --- a/site/src/pages/AgentsPage/hooks/useFileAttachments.ts +++ b/site/src/pages/AgentsPage/hooks/useFileAttachments.ts @@ -190,12 +190,13 @@ export function useFileAttachments( () => new Map(), ); - // Revoke blob URLs on unmount to prevent memory leaks. const revokePreviewUrls = useEffectEvent(() => { for (const [, url] of previewUrls) { if (url.startsWith("blob:")) URL.revokeObjectURL(url); } }); + + // Revoke blob URLs on unmount to prevent memory leaks. useEffect(() => { return () => revokePreviewUrls(); }, []); @@ -345,7 +346,9 @@ export function useFileAttachments( }; const resetAttachments = () => { - revokePreviewUrls(); + for (const [, url] of previewUrls) { + if (url.startsWith("blob:")) URL.revokeObjectURL(url); + } setPreviewUrls(new Map()); setTextContents(new Map()); setUploadStates(new Map()); diff --git a/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.tsx b/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.tsx index 967dfa77151d9..5dc9ab5f70953 100644 --- a/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.tsx +++ b/site/src/pages/CreateWorkspacePage/CreateWorkspacePage.tsx @@ -99,19 +99,20 @@ const CreateWorkspacePage: FC = () => { const autofillParameters = getAutofillParameters(searchParams); - const sendMessage = useEffectEvent( - (formValues: Record, ownerId?: string) => { - const request: DynamicParametersRequest = { - id: wsResponseId.current + 1, - owner_id: ownerId ?? owner.id, - inputs: formValues, - }; - if (ws.current && ws.current.readyState === WebSocket.OPEN) { - ws.current.send(JSON.stringify(request)); - wsResponseId.current = wsResponseId.current + 1; - } - }, - ); + const sendMessage = ( + formValues: Record, + ownerId?: string, + ) => { + const request: DynamicParametersRequest = { + id: wsResponseId.current + 1, + owner_id: ownerId ?? owner.id, + inputs: formValues, + }; + if (ws.current && ws.current.readyState === WebSocket.OPEN) { + ws.current.send(JSON.stringify(request)); + wsResponseId.current = wsResponseId.current + 1; + } + }; // On page load, sends all initial parameter values to the websocket // (including defaults and autofilled from the url) diff --git a/site/src/pages/TemplatePage/TemplateEmbedPage/TemplateEmbedPageExperimental.tsx b/site/src/pages/TemplatePage/TemplateEmbedPage/TemplateEmbedPageExperimental.tsx index 96828a985db98..e93e5c5c79824 100644 --- a/site/src/pages/TemplatePage/TemplateEmbedPage/TemplateEmbedPageExperimental.tsx +++ b/site/src/pages/TemplatePage/TemplateEmbedPage/TemplateEmbedPageExperimental.tsx @@ -42,19 +42,20 @@ const TemplateEmbedPageExperimental: FC = () => { const ws = useRef(null); const [wsError, setWsError] = useState(null); - const sendMessage = useEffectEvent( - (formValues: Record, _ownerId?: string) => { - const request: DynamicParametersRequest = { - id: wsResponseId.current + 1, - owner_id: me.id, - inputs: formValues, - }; - if (ws.current && ws.current.readyState === WebSocket.OPEN) { - ws.current.send(JSON.stringify(request)); - wsResponseId.current = wsResponseId.current + 1; - } - }, - ); + const sendMessage = ( + formValues: Record, + _ownerId?: string, + ) => { + const request: DynamicParametersRequest = { + id: wsResponseId.current + 1, + owner_id: me.id, + inputs: formValues, + }; + if (ws.current && ws.current.readyState === WebSocket.OPEN) { + ws.current.send(JSON.stringify(request)); + wsResponseId.current = wsResponseId.current + 1; + } + }; const onMessage = useEffectEvent((response: DynamicParametersResponse) => { if (latestResponse && latestResponse?.id >= response.id) { diff --git a/site/src/pages/WorkspaceSettingsPage/WorkspaceParametersPage/WorkspaceParametersPageExperimental.tsx b/site/src/pages/WorkspaceSettingsPage/WorkspaceParametersPage/WorkspaceParametersPageExperimental.tsx index f5aa89012fc3a..b932e1e18c0d7 100644 --- a/site/src/pages/WorkspaceSettingsPage/WorkspaceParametersPage/WorkspaceParametersPageExperimental.tsx +++ b/site/src/pages/WorkspaceSettingsPage/WorkspaceParametersPage/WorkspaceParametersPageExperimental.tsx @@ -60,7 +60,7 @@ const WorkspaceParametersPageExperimental: FC = () => { source: "active_build", })) ?? []; - const sendMessage = useEffectEvent((formValues: Record) => { + const sendMessage = (formValues: Record) => { const request: DynamicParametersRequest = { id: wsResponseId.current + 1, owner_id: workspace.owner_id, @@ -70,7 +70,7 @@ const WorkspaceParametersPageExperimental: FC = () => { ws.current.send(JSON.stringify(request)); wsResponseId.current = wsResponseId.current + 1; } - }); + }; // On page load, sends initial workspace build parameters to the websocket. // This ensures the backend has the form's complete initial state, diff --git a/site/src/pages/WorkspacesPage/WorkspacesPage.tsx b/site/src/pages/WorkspacesPage/WorkspacesPage.tsx index a4bf604154939..a7a500ea10fc7 100644 --- a/site/src/pages/WorkspacesPage/WorkspacesPage.tsx +++ b/site/src/pages/WorkspacesPage/WorkspacesPage.tsx @@ -1,4 +1,11 @@ -import { type FC, useEffectEvent, useMemo, useState } from "react"; +import { + type FC, + useCallback, + useLayoutEffect, + useMemo, + useRef, + useState, +} from "react"; import { useQuery, useQueryClient } from "react-query"; import { useSearchParams } from "react-router"; import { toast } from "sonner"; @@ -30,11 +37,17 @@ function useSafeSearchParams() { // Have to wrap setSearchParams because React Router doesn't guarantee // a stable reference for setSearchParams across renders. 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) => setterRef.current(...args), + [], + ); + // Need this to return a tuple, not a plain array. Using "as const" + // would make it readonly and cause type mismatches at call sites. + return [searchParams, stableSetSearchParams] as ReturnType< typeof useSearchParams >; }