From a0e777503ee846f3d7c601c9076ce9f80ca4582d Mon Sep 17 00:00:00 2001 From: HeyItsChloe <54480367+HeyItsChloe@users.noreply.github.com> Date: Mon, 16 Mar 2026 09:22:23 -0700 Subject: [PATCH] fix(frontend): prevent auto sandbox resume behavior (#13133) Co-authored-by: openhands --- .../hooks/use-sandbox-recovery.test.tsx | 577 ++++++++++++++++++ .../hooks/use-visibility-recovery.test.ts | 286 +++++++++ .../conversation-websocket-context.tsx | 22 +- .../contexts/websocket-provider-wrapper.tsx | 35 +- frontend/src/hooks/use-sandbox-recovery.ts | 138 +++++ frontend/src/hooks/use-visibility-change.ts | 64 ++ frontend/src/hooks/use-websocket-recovery.ts | 110 ---- frontend/src/routes/conversation.tsx | 63 +- 8 files changed, 1091 insertions(+), 204 deletions(-) create mode 100644 frontend/__tests__/hooks/use-sandbox-recovery.test.tsx create mode 100644 frontend/__tests__/hooks/use-visibility-recovery.test.ts create mode 100644 frontend/src/hooks/use-sandbox-recovery.ts create mode 100644 frontend/src/hooks/use-visibility-change.ts delete mode 100644 frontend/src/hooks/use-websocket-recovery.ts diff --git a/frontend/__tests__/hooks/use-sandbox-recovery.test.tsx b/frontend/__tests__/hooks/use-sandbox-recovery.test.tsx new file mode 100644 index 0000000000..638fe21788 --- /dev/null +++ b/frontend/__tests__/hooks/use-sandbox-recovery.test.tsx @@ -0,0 +1,577 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { renderHook, act, waitFor } from "@testing-library/react"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import React from "react"; +import { useSandboxRecovery } from "#/hooks/use-sandbox-recovery"; +import { useUnifiedResumeConversationSandbox } from "#/hooks/mutation/use-unified-start-conversation"; +import * as customToastHandlers from "#/utils/custom-toast-handlers"; + +vi.mock("react-i18next", () => ({ + useTranslation: () => ({ + t: (key: string) => key, + }), +})); + +vi.mock("#/hooks/use-user-providers", () => ({ + useUserProviders: () => ({ + providers: [{ provider: "github", token: "test-token" }], + }), +})); + +vi.mock("#/utils/custom-toast-handlers"); +vi.mock("#/hooks/mutation/use-unified-start-conversation"); + +describe("useSandboxRecovery", () => { + let mockMutate: ReturnType; + + const createWrapper = () => { + const queryClient = new QueryClient({ + defaultOptions: { + queries: { retry: false }, + mutations: { retry: false }, + }, + }); + + return ({ children }: { children: React.ReactNode }) => ( + {children} + ); + }; + + beforeEach(() => { + vi.clearAllMocks(); + + mockMutate = vi.fn(); + + vi.mocked(useUnifiedResumeConversationSandbox).mockReturnValue({ + mutate: mockMutate, + mutateAsync: vi.fn(), + isPending: false, + isSuccess: false, + isError: false, + isIdle: true, + data: undefined, + error: null, + reset: vi.fn(), + status: "idle", + variables: undefined, + failureCount: 0, + failureReason: null, + submittedAt: 0, + context: undefined, + } as unknown as ReturnType); + + // Reset document.visibilityState + Object.defineProperty(document, "visibilityState", { + value: "visible", + writable: true, + }); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + describe("initial load recovery", () => { + it("should call resumeSandbox on initial load when conversation is STOPPED", () => { + renderHook( + () => + useSandboxRecovery({ + conversationId: "conv-123", + conversationStatus: "STOPPED", + }), + { wrapper: createWrapper() }, + ); + + expect(mockMutate).toHaveBeenCalledTimes(1); + expect(mockMutate).toHaveBeenCalledWith( + { + conversationId: "conv-123", + providers: [{ provider: "github", token: "test-token" }], + }, + expect.objectContaining({ + onSuccess: expect.any(Function), + onError: expect.any(Function), + }), + ); + }); + + it("should NOT call resumeSandbox on initial load when conversation is RUNNING", () => { + renderHook( + () => + useSandboxRecovery({ + conversationId: "conv-123", + conversationStatus: "RUNNING", + }), + { wrapper: createWrapper() }, + ); + + expect(mockMutate).not.toHaveBeenCalled(); + }); + + it("should NOT call resumeSandbox when conversationId is undefined", () => { + renderHook( + () => + useSandboxRecovery({ + conversationId: undefined, + conversationStatus: "STOPPED", + }), + { wrapper: createWrapper() }, + ); + + expect(mockMutate).not.toHaveBeenCalled(); + }); + + it("should NOT call resumeSandbox when conversationStatus is undefined", () => { + renderHook( + () => + useSandboxRecovery({ + conversationId: "conv-123", + conversationStatus: undefined, + }), + { wrapper: createWrapper() }, + ); + + expect(mockMutate).not.toHaveBeenCalled(); + }); + + it("should only call resumeSandbox once per conversation on initial load", () => { + const { rerender } = renderHook( + () => + useSandboxRecovery({ + conversationId: "conv-123", + conversationStatus: "STOPPED", + }), + { wrapper: createWrapper() }, + ); + + expect(mockMutate).toHaveBeenCalledTimes(1); + + // Rerender with same props - should not trigger again + rerender(); + + expect(mockMutate).toHaveBeenCalledTimes(1); + }); + + it("should call resumeSandbox for a new conversation after navigating", async () => { + const { rerender } = renderHook( + ({ conversationId }) => + useSandboxRecovery({ + conversationId, + conversationStatus: "STOPPED", + }), + { + wrapper: createWrapper(), + initialProps: { conversationId: "conv-123" }, + }, + ); + + expect(mockMutate).toHaveBeenCalledTimes(1); + expect(mockMutate).toHaveBeenLastCalledWith( + expect.objectContaining({ conversationId: "conv-123" }), + expect.any(Object), + ); + + // Navigate to a different conversation + rerender({ conversationId: "conv-456" }); + + await waitFor(() => { + expect(mockMutate).toHaveBeenCalledTimes(2); + }); + + expect(mockMutate).toHaveBeenLastCalledWith( + expect.objectContaining({ conversationId: "conv-456" }), + expect.any(Object), + ); + }); + }); + + describe("tab focus recovery", () => { + it("should call resumeSandbox when tab becomes visible and refetch returns STOPPED", async () => { + // Start with tab hidden + Object.defineProperty(document, "visibilityState", { + value: "hidden", + writable: true, + }); + + const mockRefetch = vi.fn().mockResolvedValue({ + data: { status: "STOPPED" }, + }); + + renderHook( + () => + useSandboxRecovery({ + conversationId: "conv-123", + conversationStatus: "RUNNING", // Cached status is RUNNING + refetchConversation: mockRefetch, + }), + { wrapper: createWrapper() }, + ); + + // No initial recovery for RUNNING + expect(mockMutate).not.toHaveBeenCalled(); + + // Simulate tab becoming visible + Object.defineProperty(document, "visibilityState", { + value: "visible", + writable: true, + }); + + await act(async () => { + document.dispatchEvent(new Event("visibilitychange")); + }); + + // Refetch should be called to get fresh status + expect(mockRefetch).toHaveBeenCalledTimes(1); + // Recovery should trigger because fresh status is STOPPED + expect(mockMutate).toHaveBeenCalledTimes(1); + }); + + it("should NOT call resumeSandbox when tab becomes visible and refetch returns RUNNING", async () => { + const mockRefetch = vi.fn().mockResolvedValue({ + data: { status: "RUNNING" }, + }); + + renderHook( + () => + useSandboxRecovery({ + conversationId: "conv-123", + conversationStatus: "RUNNING", + refetchConversation: mockRefetch, + }), + { wrapper: createWrapper() }, + ); + + // No initial recovery for RUNNING + expect(mockMutate).not.toHaveBeenCalled(); + + // Simulate tab becoming visible + await act(async () => { + document.dispatchEvent(new Event("visibilitychange")); + }); + + // Refetch was called but status is still RUNNING + expect(mockRefetch).toHaveBeenCalledTimes(1); + expect(mockMutate).not.toHaveBeenCalled(); + }); + + it("should NOT call resumeSandbox when tab becomes visible but refetchConversation is not provided", async () => { + renderHook( + () => + useSandboxRecovery({ + conversationId: "conv-123", + conversationStatus: "STOPPED", + // No refetchConversation provided + }), + { wrapper: createWrapper() }, + ); + + // Initial load triggers recovery + expect(mockMutate).toHaveBeenCalledTimes(1); + mockMutate.mockClear(); + + // Simulate tab becoming visible + await act(async () => { + document.dispatchEvent(new Event("visibilitychange")); + }); + + // No recovery on tab focus without refetchConversation + expect(mockMutate).not.toHaveBeenCalled(); + }); + + it("should NOT call resumeSandbox when tab becomes hidden", async () => { + const mockRefetch = vi.fn().mockResolvedValue({ + data: { status: "STOPPED" }, + }); + + renderHook( + () => + useSandboxRecovery({ + conversationId: "conv-123", + conversationStatus: "STOPPED", + refetchConversation: mockRefetch, + }), + { wrapper: createWrapper() }, + ); + + // Initial load triggers recovery + expect(mockMutate).toHaveBeenCalledTimes(1); + mockMutate.mockClear(); + + // Simulate tab becoming hidden + Object.defineProperty(document, "visibilityState", { + value: "hidden", + writable: true, + }); + + await act(async () => { + document.dispatchEvent(new Event("visibilitychange")); + }); + + // Refetch should NOT be called when tab is hidden + expect(mockRefetch).not.toHaveBeenCalled(); + expect(mockMutate).not.toHaveBeenCalled(); + }); + + it("should clean up visibility event listener on unmount", () => { + const addEventListenerSpy = vi.spyOn(document, "addEventListener"); + const removeEventListenerSpy = vi.spyOn(document, "removeEventListener"); + + const { unmount } = renderHook( + () => + useSandboxRecovery({ + conversationId: "conv-123", + conversationStatus: "STOPPED", + }), + { wrapper: createWrapper() }, + ); + + expect(addEventListenerSpy).toHaveBeenCalledWith( + "visibilitychange", + expect.any(Function), + ); + + unmount(); + + expect(removeEventListenerSpy).toHaveBeenCalledWith( + "visibilitychange", + expect.any(Function), + ); + }); + + it("should NOT call resumeSandbox when tab becomes visible while isPending is true", async () => { + vi.mocked(useUnifiedResumeConversationSandbox).mockReturnValue({ + mutate: mockMutate, + mutateAsync: vi.fn(), + isPending: true, + isSuccess: false, + isError: false, + isIdle: false, + data: undefined, + error: null, + reset: vi.fn(), + status: "pending", + variables: undefined, + failureCount: 0, + failureReason: null, + submittedAt: 0, + context: undefined, + } as unknown as ReturnType); + + const mockRefetch = vi.fn().mockResolvedValue({ + data: { status: "STOPPED" }, + }); + + renderHook( + () => + useSandboxRecovery({ + conversationId: "conv-123", + conversationStatus: "RUNNING", + refetchConversation: mockRefetch, + }), + { wrapper: createWrapper() }, + ); + + // Simulate tab becoming visible + await act(async () => { + document.dispatchEvent(new Event("visibilitychange")); + }); + + // Refetch will be called when isPending is true + expect(mockRefetch).toHaveBeenCalledTimes(1); + // resumeSandbox should NOT be called + expect(mockMutate).not.toHaveBeenCalled(); + }); + + it("should handle refetch errors gracefully without crashing", async () => { + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + + const mockRefetch = vi.fn().mockRejectedValue(new Error("Network error")); + + renderHook( + () => + useSandboxRecovery({ + conversationId: "conv-123", + conversationStatus: "RUNNING", + refetchConversation: mockRefetch, + }), + { wrapper: createWrapper() }, + ); + + // Simulate tab becoming visible + await act(async () => { + document.dispatchEvent(new Event("visibilitychange")); + }); + + // Refetch was called + expect(mockRefetch).toHaveBeenCalledTimes(1); + // Error was logged + expect(consoleErrorSpy).toHaveBeenCalledWith( + "Failed to refetch conversation on visibility change:", + expect.any(Error), + ); + // No recovery attempt was made (due to error) + expect(mockMutate).not.toHaveBeenCalled(); + + consoleErrorSpy.mockRestore(); + }); + }); + + describe("recovery callbacks", () => { + it("should return isResuming=false when no recovery is in progress", () => { + const { result } = renderHook( + () => + useSandboxRecovery({ + conversationId: "conv-123", + conversationStatus: "RUNNING", + }), + { wrapper: createWrapper() }, + ); + + expect(result.current.isResuming).toBe(false); + }); + + it("should return isResuming=true when mutation is pending", () => { + vi.mocked(useUnifiedResumeConversationSandbox).mockReturnValue({ + mutate: mockMutate, + mutateAsync: vi.fn(), + isPending: true, + isSuccess: false, + isError: false, + isIdle: false, + data: undefined, + error: null, + reset: vi.fn(), + status: "pending", + variables: undefined, + failureCount: 0, + failureReason: null, + submittedAt: 0, + context: undefined, + } as unknown as ReturnType); + + const { result } = renderHook( + () => + useSandboxRecovery({ + conversationId: "conv-123", + conversationStatus: "STOPPED", + }), + { wrapper: createWrapper() }, + ); + + expect(result.current.isResuming).toBe(true); + }); + + it("should call onSuccess callback when recovery succeeds", () => { + const onSuccess = vi.fn(); + + renderHook( + () => + useSandboxRecovery({ + conversationId: "conv-123", + conversationStatus: "STOPPED", + onSuccess, + }), + { wrapper: createWrapper() }, + ); + + // Get the onSuccess callback passed to mutate + const mutateCall = mockMutate.mock.calls[0]; + const options = mutateCall[1]; + + // Simulate successful mutation + act(() => { + options.onSuccess(); + }); + + expect(onSuccess).toHaveBeenCalledTimes(1); + }); + + it("should call onError callback and display toast when recovery fails", () => { + const onError = vi.fn(); + const testError = new Error("Resume failed"); + + renderHook( + () => + useSandboxRecovery({ + conversationId: "conv-123", + conversationStatus: "STOPPED", + onError, + }), + { wrapper: createWrapper() }, + ); + + // Get the onError callback passed to mutate + const mutateCall = mockMutate.mock.calls[0]; + const options = mutateCall[1]; + + // Simulate failed mutation + act(() => { + options.onError(testError); + }); + + expect(onError).toHaveBeenCalledTimes(1); + expect(onError).toHaveBeenCalledWith(testError); + expect(vi.mocked(customToastHandlers.displayErrorToast)).toHaveBeenCalled(); + }); + + it("should NOT call resumeSandbox when isPending is true", () => { + vi.mocked(useUnifiedResumeConversationSandbox).mockReturnValue({ + mutate: mockMutate, + mutateAsync: vi.fn(), + isPending: true, + isSuccess: false, + isError: false, + isIdle: false, + data: undefined, + error: null, + reset: vi.fn(), + status: "pending", + variables: undefined, + failureCount: 0, + failureReason: null, + submittedAt: 0, + context: undefined, + } as unknown as ReturnType); + + renderHook( + () => + useSandboxRecovery({ + conversationId: "conv-123", + conversationStatus: "STOPPED", + }), + { wrapper: createWrapper() }, + ); + + // Should not call mutate because isPending is true + expect(mockMutate).not.toHaveBeenCalled(); + }); + }); + + describe("WebSocket disconnect (negative test)", () => { + it("should NOT have any mechanism to auto-resume on WebSocket disconnect", () => { + // This test documents the intended behavior: the hook does NOT + // listen for WebSocket disconnects. Recovery only happens on: + // 1. Initial page load (STOPPED status) + // 2. Tab focus (visibilitychange event) + // + // There is intentionally NO onDisconnect handler or WebSocket listener. + + const { result } = renderHook( + () => + useSandboxRecovery({ + conversationId: "conv-123", + conversationStatus: "RUNNING", + }), + { wrapper: createWrapper() }, + ); + + // The hook should only expose isResuming - no disconnect-related functionality + expect(result.current).toEqual({ + isResuming: expect.any(Boolean), + }); + + // No calls should have been made for RUNNING status + expect(mockMutate).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/frontend/__tests__/hooks/use-visibility-recovery.test.ts b/frontend/__tests__/hooks/use-visibility-recovery.test.ts new file mode 100644 index 0000000000..301d910fa2 --- /dev/null +++ b/frontend/__tests__/hooks/use-visibility-recovery.test.ts @@ -0,0 +1,286 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { renderHook, act } from "@testing-library/react"; +import { useVisibilityChange } from "#/hooks/use-visibility-change"; + +describe("useVisibilityChange", () => { + beforeEach(() => { + // Reset document.visibilityState to visible + Object.defineProperty(document, "visibilityState", { + value: "visible", + writable: true, + configurable: true, + }); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + describe("initial state", () => { + it("should return isVisible=true when document is visible", () => { + Object.defineProperty(document, "visibilityState", { + value: "visible", + writable: true, + }); + + const { result } = renderHook(() => useVisibilityChange()); + + expect(result.current.isVisible).toBe(true); + }); + + it("should return isVisible=false when document is hidden", () => { + Object.defineProperty(document, "visibilityState", { + value: "hidden", + writable: true, + }); + + const { result } = renderHook(() => useVisibilityChange()); + + expect(result.current.isVisible).toBe(false); + }); + }); + + describe("visibility change events", () => { + it("should update isVisible when visibility changes to hidden", () => { + const { result } = renderHook(() => useVisibilityChange()); + + expect(result.current.isVisible).toBe(true); + + // Simulate tab becoming hidden + Object.defineProperty(document, "visibilityState", { + value: "hidden", + writable: true, + }); + + act(() => { + document.dispatchEvent(new Event("visibilitychange")); + }); + + expect(result.current.isVisible).toBe(false); + }); + + it("should update isVisible when visibility changes to visible", () => { + Object.defineProperty(document, "visibilityState", { + value: "hidden", + writable: true, + }); + + const { result } = renderHook(() => useVisibilityChange()); + + expect(result.current.isVisible).toBe(false); + + // Simulate tab becoming visible + Object.defineProperty(document, "visibilityState", { + value: "visible", + writable: true, + }); + + act(() => { + document.dispatchEvent(new Event("visibilitychange")); + }); + + expect(result.current.isVisible).toBe(true); + }); + }); + + describe("callbacks", () => { + it("should call onVisibilityChange with the new state", () => { + const onVisibilityChange = vi.fn(); + + renderHook(() => useVisibilityChange({ onVisibilityChange })); + + // Simulate tab becoming hidden + Object.defineProperty(document, "visibilityState", { + value: "hidden", + writable: true, + }); + + act(() => { + document.dispatchEvent(new Event("visibilitychange")); + }); + + expect(onVisibilityChange).toHaveBeenCalledWith("hidden"); + + // Simulate tab becoming visible + Object.defineProperty(document, "visibilityState", { + value: "visible", + writable: true, + }); + + act(() => { + document.dispatchEvent(new Event("visibilitychange")); + }); + + expect(onVisibilityChange).toHaveBeenCalledWith("visible"); + }); + + it("should call onVisible only when tab becomes visible", () => { + const onVisible = vi.fn(); + const onHidden = vi.fn(); + + renderHook(() => useVisibilityChange({ onVisible, onHidden })); + + // Simulate tab becoming hidden + Object.defineProperty(document, "visibilityState", { + value: "hidden", + writable: true, + }); + + act(() => { + document.dispatchEvent(new Event("visibilitychange")); + }); + + expect(onVisible).not.toHaveBeenCalled(); + expect(onHidden).toHaveBeenCalledTimes(1); + + // Simulate tab becoming visible + Object.defineProperty(document, "visibilityState", { + value: "visible", + writable: true, + }); + + act(() => { + document.dispatchEvent(new Event("visibilitychange")); + }); + + expect(onVisible).toHaveBeenCalledTimes(1); + expect(onHidden).toHaveBeenCalledTimes(1); + }); + + it("should call onHidden only when tab becomes hidden", () => { + const onHidden = vi.fn(); + + renderHook(() => useVisibilityChange({ onHidden })); + + // Simulate tab becoming hidden + Object.defineProperty(document, "visibilityState", { + value: "hidden", + writable: true, + }); + + act(() => { + document.dispatchEvent(new Event("visibilitychange")); + }); + + expect(onHidden).toHaveBeenCalledTimes(1); + + // Simulate tab becoming visible (should not call onHidden) + Object.defineProperty(document, "visibilityState", { + value: "visible", + writable: true, + }); + + act(() => { + document.dispatchEvent(new Event("visibilitychange")); + }); + + expect(onHidden).toHaveBeenCalledTimes(1); + }); + }); + + describe("enabled option", () => { + it("should not listen for events when enabled=false", () => { + const onVisible = vi.fn(); + + renderHook(() => useVisibilityChange({ onVisible, enabled: false })); + + // Simulate tab becoming visible + Object.defineProperty(document, "visibilityState", { + value: "visible", + writable: true, + }); + + act(() => { + document.dispatchEvent(new Event("visibilitychange")); + }); + + expect(onVisible).not.toHaveBeenCalled(); + }); + + it("should start listening when enabled changes from false to true", () => { + const onVisible = vi.fn(); + + const { rerender } = renderHook( + ({ enabled }) => useVisibilityChange({ onVisible, enabled }), + { initialProps: { enabled: false } }, + ); + + // Simulate event while disabled + act(() => { + document.dispatchEvent(new Event("visibilitychange")); + }); + + expect(onVisible).not.toHaveBeenCalled(); + + // Enable the hook + rerender({ enabled: true }); + + // Now events should be captured + act(() => { + document.dispatchEvent(new Event("visibilitychange")); + }); + + expect(onVisible).toHaveBeenCalledTimes(1); + }); + }); + + describe("cleanup", () => { + it("should remove event listener on unmount", () => { + const addEventListenerSpy = vi.spyOn(document, "addEventListener"); + const removeEventListenerSpy = vi.spyOn(document, "removeEventListener"); + + const { unmount } = renderHook(() => useVisibilityChange()); + + expect(addEventListenerSpy).toHaveBeenCalledWith( + "visibilitychange", + expect.any(Function), + ); + + unmount(); + + expect(removeEventListenerSpy).toHaveBeenCalledWith( + "visibilitychange", + expect.any(Function), + ); + }); + + it("should remove event listener when enabled changes to false", () => { + const removeEventListenerSpy = vi.spyOn(document, "removeEventListener"); + + const { rerender } = renderHook( + ({ enabled }) => useVisibilityChange({ enabled }), + { initialProps: { enabled: true } }, + ); + + rerender({ enabled: false }); + + expect(removeEventListenerSpy).toHaveBeenCalledWith( + "visibilitychange", + expect.any(Function), + ); + }); + }); + + describe("callback stability", () => { + it("should handle callback updates without missing events", () => { + const onVisible1 = vi.fn(); + const onVisible2 = vi.fn(); + + const { rerender } = renderHook( + ({ onVisible }) => useVisibilityChange({ onVisible }), + { initialProps: { onVisible: onVisible1 } }, + ); + + // Update callback + rerender({ onVisible: onVisible2 }); + + // Simulate visibility change + act(() => { + document.dispatchEvent(new Event("visibilitychange")); + }); + + expect(onVisible1).not.toHaveBeenCalled(); + expect(onVisible2).toHaveBeenCalledTimes(1); + }); + }); +}); diff --git a/frontend/src/contexts/conversation-websocket-context.tsx b/frontend/src/contexts/conversation-websocket-context.tsx index 86863734b9..33c7169a77 100644 --- a/frontend/src/contexts/conversation-websocket-context.tsx +++ b/frontend/src/contexts/conversation-websocket-context.tsx @@ -78,7 +78,6 @@ export function ConversationWebSocketProvider({ sessionApiKey, subConversations, subConversationIds, - onDisconnect, }: { children: React.ReactNode; conversationId?: string; @@ -86,7 +85,6 @@ export function ConversationWebSocketProvider({ sessionApiKey?: string | null; subConversations?: V1AppConversation[]; subConversationIds?: string[]; - onDisconnect?: () => void; }) { // Separate connection state tracking for each WebSocket const [mainConnectionState, setMainConnectionState] = @@ -714,13 +712,10 @@ export function ConversationWebSocketProvider({ } } }, - onClose: (event: CloseEvent) => { + onClose: () => { setMainConnectionState("CLOSED"); - // Trigger silent recovery on unexpected disconnect - // Do NOT show error message - recovery happens automatically - if (event.code !== 1000 && hasConnectedRefMain.current) { - onDisconnect?.(); - } + // Recovery is handled by useSandboxRecovery on tab focus/page refresh + // No error message needed - silent recovery provides better UX }, onError: () => { setMainConnectionState("CLOSED"); @@ -738,7 +733,6 @@ export function ConversationWebSocketProvider({ sessionApiKey, conversationId, conversationUrl, - onDisconnect, ]); // Separate WebSocket options for planning agent connection @@ -785,13 +779,10 @@ export function ConversationWebSocketProvider({ } } }, - onClose: (event: CloseEvent) => { + onClose: () => { setPlanningConnectionState("CLOSED"); - // Trigger silent recovery on unexpected disconnect - // Do NOT show error message - recovery happens automatically - if (event.code !== 1000 && hasConnectedRefPlanning.current) { - onDisconnect?.(); - } + // Recovery is handled by useSandboxRecovery on tab focus/page refresh + // No error message needed - silent recovery provides better UX }, onError: () => { setPlanningConnectionState("CLOSED"); @@ -808,7 +799,6 @@ export function ConversationWebSocketProvider({ removeErrorMessage, sessionApiKey, subConversations, - onDisconnect, ]); // Only attempt WebSocket connection when we have a valid URL diff --git a/frontend/src/contexts/websocket-provider-wrapper.tsx b/frontend/src/contexts/websocket-provider-wrapper.tsx index 3aa21e4113..e484bd0571 100644 --- a/frontend/src/contexts/websocket-provider-wrapper.tsx +++ b/frontend/src/contexts/websocket-provider-wrapper.tsx @@ -3,7 +3,8 @@ import { WsClientProvider } from "#/context/ws-client-provider"; import { ConversationWebSocketProvider } from "#/contexts/conversation-websocket-context"; import { useActiveConversation } from "#/hooks/query/use-active-conversation"; import { useSubConversations } from "#/hooks/query/use-sub-conversations"; -import { useWebSocketRecovery } from "#/hooks/use-websocket-recovery"; +import { useSandboxRecovery } from "#/hooks/use-sandbox-recovery"; +import { isTaskConversationId } from "#/utils/conversation-local-storage"; interface WebSocketProviderWrapperProps { children: React.ReactNode; @@ -18,18 +19,6 @@ interface WebSocketProviderWrapperProps { * @param version - 0 for old WsClientProvider, 1 for new ConversationWebSocketProvider * @param conversationId - The conversation ID to pass to the provider * @param children - The child components to wrap - * - * @example - * // Use the old v0 provider - * - * - * - * - * @example - * // Use the new v1 provider - * - * - * */ export function WebSocketProviderWrapper({ children, @@ -37,7 +26,11 @@ export function WebSocketProviderWrapper({ version, }: WebSocketProviderWrapperProps) { // Get conversation data for V1 provider - const { data: conversation } = useActiveConversation(); + const { + data: conversation, + refetch: refetchConversation, + isFetched, + } = useActiveConversation(); // Get sub-conversation data for V1 provider const { data: subConversations } = useSubConversations( conversation?.sub_conversation_ids ?? [], @@ -48,9 +41,15 @@ export function WebSocketProviderWrapper({ (subConversation) => subConversation !== null, ); - // Silent recovery for V1 WebSocket disconnections - const { reconnectKey, handleDisconnect } = - useWebSocketRecovery(conversationId); + const isConversationReady = + !isTaskConversationId(conversationId) && isFetched && !!conversation; + // Recovery for V1 conversations - handles page refresh and tab focus + // Does NOT resume on WebSocket disconnect (server pauses after 20 min inactivity) + useSandboxRecovery({ + conversationId, + conversationStatus: conversation?.status, + refetchConversation: isConversationReady ? refetchConversation : undefined, + }); if (version === 0) { return ( @@ -63,13 +62,11 @@ export function WebSocketProviderWrapper({ if (version === 1) { return ( {children} diff --git a/frontend/src/hooks/use-sandbox-recovery.ts b/frontend/src/hooks/use-sandbox-recovery.ts new file mode 100644 index 0000000000..78804f6706 --- /dev/null +++ b/frontend/src/hooks/use-sandbox-recovery.ts @@ -0,0 +1,138 @@ +import React from "react"; +import { useTranslation } from "react-i18next"; +import { useUnifiedResumeConversationSandbox } from "./mutation/use-unified-start-conversation"; +import { useUserProviders } from "./use-user-providers"; +import { useVisibilityChange } from "./use-visibility-change"; +import { displayErrorToast } from "#/utils/custom-toast-handlers"; +import { I18nKey } from "#/i18n/declaration"; +import type { ConversationStatus } from "#/types/conversation-status"; +import type { Conversation } from "#/api/open-hands.types"; + +interface UseSandboxRecoveryOptions { + conversationId: string | undefined; + conversationStatus: ConversationStatus | undefined; + /** Function to refetch the conversation data - used to get fresh status on tab focus */ + refetchConversation?: () => Promise<{ + data: Conversation | null | undefined; + }>; + onSuccess?: () => void; + onError?: (error: Error) => void; +} + +/** + * Hook that handles sandbox recovery based on user intent. + * + * Recovery triggers: + * - Page refresh: Resumes the sandbox on initial load if it was paused/stopped + * - Tab gains focus: Resumes the sandbox if it was paused/stopped + * + * What does NOT trigger recovery: + * - WebSocket disconnect: Does NOT automatically resume the sandbox + * (The server pauses sandboxes after 20 minutes of inactivity, + * and sandboxes should only be resumed when the user explicitly shows intent) + * + * @param options.conversationId - The conversation ID to recover + * @param options.conversationStatus - The current conversation status + * @param options.refetchConversation - Function to refetch conversation data on tab focus + * @param options.onSuccess - Callback when recovery succeeds + * @param options.onError - Callback when recovery fails + * @returns isResuming - Whether a recovery is in progress + */ +export function useSandboxRecovery({ + conversationId, + conversationStatus, + refetchConversation, + onSuccess, + onError, +}: UseSandboxRecoveryOptions) { + const { t } = useTranslation(); + const { providers } = useUserProviders(); + const { mutate: resumeSandbox, isPending: isResuming } = + useUnifiedResumeConversationSandbox(); + + // Track which conversation ID we've already processed for initial load recovery + const processedConversationIdRef = React.useRef(null); + + const attemptRecovery = React.useCallback( + (statusOverride?: ConversationStatus) => { + const status = statusOverride ?? conversationStatus; + /** + * Only recover if sandbox is paused (status === STOPPED) and not already resuming + * + * Note: ConversationStatus uses different terminology than SandboxStatus: + * - SandboxStatus.PAUSED → ConversationStatus.STOPPED : the runtime is not running but may be restarted + * - SandboxStatus.MISSING → ConversationStatus.ARCHIVED : the runtime is not running and will not restart due to deleted files. + */ + if (!conversationId || status !== "STOPPED" || isResuming) { + return; + } + + resumeSandbox( + { conversationId, providers }, + { + onSuccess: () => { + onSuccess?.(); + }, + onError: (error) => { + displayErrorToast( + t(I18nKey.CONVERSATION$FAILED_TO_START_WITH_ERROR, { + error: error.message, + }), + ); + onError?.(error); + }, + }, + ); + }, + [ + conversationId, + conversationStatus, + isResuming, + providers, + resumeSandbox, + onSuccess, + onError, + t, + ], + ); + + // Handle page refresh (initial load) and conversation navigation + React.useEffect(() => { + if (!conversationId || !conversationStatus) return; + + // Only attempt recovery once per conversation (handles both initial load and navigation) + if (processedConversationIdRef.current === conversationId) return; + + processedConversationIdRef.current = conversationId; + + if (conversationStatus === "STOPPED") { + attemptRecovery(); + } + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [conversationId, conversationStatus]); + + const handleVisible = React.useCallback(async () => { + // Skip if no conversation or refetch function + if (!conversationId || !refetchConversation) return; + + try { + // Refetch to get fresh status - cached status may be stale if sandbox was paused while tab was inactive + const { data } = await refetchConversation(); + attemptRecovery(data?.status); + } catch (error) { + // eslint-disable-next-line no-console + console.error( + "Failed to refetch conversation on visibility change:", + error, + ); + } + }, [conversationId, refetchConversation, isResuming, attemptRecovery]); + + // Handle tab focus (visibility change) - refetch conversation status and resume if needed + useVisibilityChange({ + enabled: !!conversationId, + onVisible: handleVisible, + }); + + return { isResuming }; +} diff --git a/frontend/src/hooks/use-visibility-change.ts b/frontend/src/hooks/use-visibility-change.ts new file mode 100644 index 0000000000..1ee929cf6c --- /dev/null +++ b/frontend/src/hooks/use-visibility-change.ts @@ -0,0 +1,64 @@ +import React from "react"; + +type VisibilityState = "visible" | "hidden"; + +interface UseVisibilityChangeOptions { + /** Callback fired when visibility changes to the specified state */ + onVisibilityChange?: (state: VisibilityState) => void; + /** Callback fired only when tab becomes visible */ + onVisible?: () => void; + /** Callback fired only when tab becomes hidden */ + onHidden?: () => void; + /** Whether to listen for visibility changes (default: true) */ + enabled?: boolean; +} + +/** + * Hook that listens for browser tab visibility changes. + * + * Useful for: + * - Resuming operations when user returns to the tab + * - Pausing expensive operations when tab is hidden + * - Tracking user engagement + * + * @param options.onVisibilityChange - Callback with the new visibility state + * @param options.onVisible - Callback fired only when tab becomes visible + * @param options.onHidden - Callback fired only when tab becomes hidden + * @param options.enabled - Whether to listen for changes (default: true) + * @returns isVisible - Current visibility state of the tab + */ +export function useVisibilityChange({ + onVisibilityChange, + onVisible, + onHidden, + enabled = true, +}: UseVisibilityChangeOptions = {}) { + const [isVisible, setIsVisible] = React.useState( + () => document.visibilityState === "visible", + ); + + React.useEffect(() => { + if (!enabled) return undefined; + + const handleVisibilityChange = () => { + const state = document.visibilityState as VisibilityState; + setIsVisible(state === "visible"); + + onVisibilityChange?.(state); + + if (state === "visible") { + onVisible?.(); + } else { + onHidden?.(); + } + }; + + document.addEventListener("visibilitychange", handleVisibilityChange); + + return () => { + document.removeEventListener("visibilitychange", handleVisibilityChange); + }; + }, [enabled, onVisibilityChange, onVisible, onHidden]); + + return { isVisible }; +} diff --git a/frontend/src/hooks/use-websocket-recovery.ts b/frontend/src/hooks/use-websocket-recovery.ts deleted file mode 100644 index d15358d12e..0000000000 --- a/frontend/src/hooks/use-websocket-recovery.ts +++ /dev/null @@ -1,110 +0,0 @@ -import React from "react"; -import { useQueryClient } from "@tanstack/react-query"; -import { useUnifiedResumeConversationSandbox } from "#/hooks/mutation/use-unified-start-conversation"; -import { useUserProviders } from "#/hooks/use-user-providers"; -import { useErrorMessageStore } from "#/stores/error-message-store"; -import { I18nKey } from "#/i18n/declaration"; - -const MAX_RECOVERY_ATTEMPTS = 3; -const RECOVERY_COOLDOWN_MS = 5000; -const RECOVERY_SETTLED_DELAY_MS = 2000; - -/** - * Hook that handles silent WebSocket recovery by resuming the sandbox - * when a WebSocket disconnection is detected. - * - * @param conversationId - The conversation ID to recover - * @returns reconnectKey - Key to force provider remount (resets connection state) - * @returns handleDisconnect - Callback to trigger recovery on WebSocket disconnect - */ -export function useWebSocketRecovery(conversationId: string) { - // Recovery state (refs to avoid re-renders) - const recoveryAttemptsRef = React.useRef(0); - const recoveryInProgressRef = React.useRef(false); - const lastRecoveryAttemptRef = React.useRef(null); - - // Key to force remount of provider after recovery (resets connection state to "CONNECTING") - const [reconnectKey, setReconnectKey] = React.useState(0); - - const queryClient = useQueryClient(); - const { mutate: resumeConversation } = useUnifiedResumeConversationSandbox(); - const { providers } = useUserProviders(); - const setErrorMessage = useErrorMessageStore( - (state) => state.setErrorMessage, - ); - - // Reset recovery state when conversation changes - React.useEffect(() => { - recoveryAttemptsRef.current = 0; - recoveryInProgressRef.current = false; - lastRecoveryAttemptRef.current = null; - }, [conversationId]); - - // Silent recovery callback - resumes sandbox when WebSocket disconnects - const handleDisconnect = React.useCallback(() => { - // Prevent concurrent recovery attempts - if (recoveryInProgressRef.current) return; - - // Check cooldown - const now = Date.now(); - if ( - lastRecoveryAttemptRef.current && - now - lastRecoveryAttemptRef.current < RECOVERY_COOLDOWN_MS - ) { - return; - } - - // Check max attempts - notify user when recovery is exhausted - if (recoveryAttemptsRef.current >= MAX_RECOVERY_ATTEMPTS) { - setErrorMessage(I18nKey.STATUS$CONNECTION_LOST); - return; - } - - // Start silent recovery - recoveryInProgressRef.current = true; - lastRecoveryAttemptRef.current = now; - recoveryAttemptsRef.current += 1; - - resumeConversation( - { conversationId, providers }, - { - onSuccess: async () => { - // Invalidate and wait for refetch to complete before remounting - // This ensures the provider remounts with fresh data (url: null during startup) - await queryClient.invalidateQueries({ - queryKey: ["user", "conversation", conversationId], - }); - - // Force remount to reset connection state to "CONNECTING" - setReconnectKey((k) => k + 1); - - // Reset recovery state on success - recoveryAttemptsRef.current = 0; - recoveryInProgressRef.current = false; - lastRecoveryAttemptRef.current = null; - }, - onError: () => { - // If this was the last attempt, show error to user - if (recoveryAttemptsRef.current >= MAX_RECOVERY_ATTEMPTS) { - setErrorMessage(I18nKey.STATUS$CONNECTION_LOST); - } - // recoveryInProgressRef will be reset by onSettled - }, - onSettled: () => { - // Allow next attempt after a delay (covers both success and error) - setTimeout(() => { - recoveryInProgressRef.current = false; - }, RECOVERY_SETTLED_DELAY_MS); - }, - }, - ); - }, [ - conversationId, - providers, - resumeConversation, - queryClient, - setErrorMessage, - ]); - - return { reconnectKey, handleDisconnect }; -} diff --git a/frontend/src/routes/conversation.tsx b/frontend/src/routes/conversation.tsx index 3063c2d89c..c12ce948c7 100644 --- a/frontend/src/routes/conversation.tsx +++ b/frontend/src/routes/conversation.tsx @@ -18,7 +18,6 @@ import { useTaskPolling } from "#/hooks/query/use-task-polling"; import { displayErrorToast } from "#/utils/custom-toast-handlers"; import { useIsAuthed } from "#/hooks/query/use-is-authed"; import { ConversationSubscriptionsProvider } from "#/context/conversation-subscriptions-provider"; -import { useUserProviders } from "#/hooks/use-user-providers"; import { ConversationMain } from "#/components/features/conversation/conversation-main/conversation-main"; import { ConversationNameWithStatus } from "#/components/features/conversation/conversation-name-with-status"; @@ -26,7 +25,6 @@ import { ConversationNameWithStatus } from "#/components/features/conversation/c import { ConversationTabs } from "#/components/features/conversation/conversation-tabs/conversation-tabs"; import { WebSocketProviderWrapper } from "#/contexts/websocket-provider-wrapper"; import { useErrorMessageStore } from "#/stores/error-message-store"; -import { useUnifiedResumeConversationSandbox } from "#/hooks/mutation/use-unified-start-conversation"; import { I18nKey } from "#/i18n/declaration"; import { useEventStore } from "#/stores/use-event-store"; @@ -39,11 +37,8 @@ function AppContent() { // Handle both task IDs (task-{uuid}) and regular conversation IDs const { isTask, taskStatus, taskDetail } = useTaskPolling(); - const { data: conversation, isFetched, refetch } = useActiveConversation(); - const { mutate: startConversation, isPending: isStarting } = - useUnifiedResumeConversationSandbox(); + const { data: conversation, isFetched } = useActiveConversation(); const { data: isAuthed } = useIsAuthed(); - const { providers } = useUserProviders(); const { resetConversationState } = useConversationStore(); const navigate = useNavigate(); const clearTerminal = useCommandStore((state) => state.clearTerminal); @@ -54,9 +49,6 @@ function AppContent() { (state) => state.removeErrorMessage, ); - // Track which conversation ID we've auto-started to prevent auto-restart after manual stop - const processedConversationId = React.useRef(null); - // Fetch batch feedback data when conversation is loaded useBatchFeedback(); @@ -67,12 +59,6 @@ function AppContent() { setCurrentAgentState(AgentState.LOADING); removeErrorMessage(); clearEvents(); - - // Reset tracking ONLY if we're navigating to a DIFFERENT conversation - // Don't reset on StrictMode remounts (conversationId is the same) - if (processedConversationId.current !== conversationId) { - processedConversationId.current = null; - } }, [ conversationId, clearTerminal, @@ -91,7 +77,8 @@ function AppContent() { } }, [isTask, taskStatus, taskDetail, t]); - // 3. Auto-start Effect - handles conversation not found and auto-starting STOPPED conversations + // 3. Handle conversation not found + // NOTE: Resuming STOPPED conversations is handled by useSandboxRecovery in WebSocketProviderWrapper React.useEffect(() => { // Wait for data to be fetched if (!isFetched || !isAuthed) return; @@ -100,50 +87,8 @@ function AppContent() { if (!conversation) { displayErrorToast(t(I18nKey.CONVERSATION$NOT_EXIST_OR_NO_PERMISSION)); navigate("/"); - return; } - - const currentConversationId = conversation.conversation_id; - const currentStatus = conversation.status; - - // Skip if we've already processed this conversation - if (processedConversationId.current === currentConversationId) { - return; - } - - // Mark as processed immediately to prevent duplicate calls - processedConversationId.current = currentConversationId; - - // Auto-start STOPPED conversations on initial load only - if (currentStatus === "STOPPED" && !isStarting) { - startConversation( - { conversationId: currentConversationId, providers }, - { - onError: (error) => { - displayErrorToast( - t(I18nKey.CONVERSATION$FAILED_TO_START_WITH_ERROR, { - error: error.message, - }), - ); - refetch(); - }, - }, - ); - } - // NOTE: conversation?.status is intentionally NOT in dependencies - // We only want to run when conversation ID changes, not when status changes - // This prevents duplicate calls when stale cache data is replaced with fresh data - }, [ - conversation?.conversation_id, - isFetched, - isAuthed, - isStarting, - providers, - startConversation, - navigate, - refetch, - t, - ]); + }, [conversation, isFetched, isAuthed, navigate, t]); const isV0Conversation = conversation?.conversation_version === "V0";