From 6cab5ad87b1ebbebd380d768bd7ebc0fba8d8573 Mon Sep 17 00:00:00 2001 From: Neil Date: Sat, 26 Sep 2026 01:00:44 -0700 Subject: [PATCH 1/2] perf: skip new server status checks after settings closes --- ...ntime-environment-catalog-lifetime.test.ts | 190 ++++++++++++++++++ .../use-runtime-environment-catalog.ts | 7 + 2 files changed, 197 insertions(+) create mode 100644 src/renderer/src/components/settings/use-runtime-environment-catalog-lifetime.test.ts diff --git a/src/renderer/src/components/settings/use-runtime-environment-catalog-lifetime.test.ts b/src/renderer/src/components/settings/use-runtime-environment-catalog-lifetime.test.ts new file mode 100644 index 000000000000..2ab06e5cf0de --- /dev/null +++ b/src/renderer/src/components/settings/use-runtime-environment-catalog-lifetime.test.ts @@ -0,0 +1,190 @@ +// @vitest-environment happy-dom +import { act, cleanup, renderHook } from '@testing-library/react' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type { PublicKnownRuntimeEnvironment } from '../../../../shared/runtime-environments' +import type { RuntimeRpcResponse } from '../../../../shared/runtime-rpc-envelope' +import type { RuntimeStatus } from '../../../../shared/runtime-types' +import { createCompatibleRuntimeStatusResponse } from '@/runtime/runtime-compatibility-test-fixture' +import { useRuntimeEnvironmentCatalog } from './use-runtime-environment-catalog' + +const store = vi.hoisted(() => ({ + setRuntimeEnvironments: vi.fn(), + readRuntimeHostStatusSnapshots: vi.fn<() => Promise>(), + toastError: vi.fn() +})) + +vi.mock('@/store', () => ({ useAppStore: { getState: () => store } })) +vi.mock('sonner', () => ({ toast: { error: store.toastError } })) +vi.mock('@/i18n/i18n', () => ({ + translate: (_key: string, fallback: string) => fallback +})) +vi.mock('@/runtime/runtime-rpc-client', () => ({ + unwrapRuntimeRpcResult: (response: RuntimeRpcResponse) => { + if (!response.ok) { + throw new Error(response.error.message) + } + return response.result + } +})) +vi.mock('@/runtime/runtime-status-probe-diagnostics', () => ({ + extractRuntimeTransportDiagnostics: () => undefined +})) + +function environment(id: string, source?: 'ephemeral-vm'): PublicKnownRuntimeEnvironment { + return { + id, + name: id, + createdAt: 1, + updatedAt: 1, + lastUsedAt: null, + runtimeId: id, + source, + preferredEndpointId: id, + endpoints: [{ id, kind: 'websocket', label: 'Fake', endpoint: 'ws://127.0.0.1:1' }] + } +} + +function installBridge() { + const list = vi.fn<() => Promise>() + const getStatus = vi.fn<() => Promise>>() + getStatus.mockResolvedValue(createCompatibleRuntimeStatusResponse()) + vi.stubGlobal('api', { runtimeEnvironments: { list, getStatus } }) + return { list, getStatus } +} + +function verifiedStatus(): RuntimeStatus { + const response = createCompatibleRuntimeStatusResponse('verified') + if (!response.ok) { + throw new Error('Expected a successful runtime status fixture') + } + return response.result +} + +beforeEach(() => { + vi.resetAllMocks() + store.readRuntimeHostStatusSnapshots.mockResolvedValue(undefined) +}) + +afterEach(() => { + cleanup() + vi.unstubAllGlobals() +}) + +describe('runtime environment catalog pane lifetime', () => { + it('keeps a late catalog update without starting probes after close', async () => { + const api = installBridge() + const list = Promise.withResolvers() + const environments = Array.from({ length: 40 }, (_, index) => environment(`host-${index}`)) + api.list.mockReturnValue(list.promise) + const { unmount } = renderHook(() => useRuntimeEnvironmentCatalog()) + + unmount() + await act(async () => list.resolve(environments)) + + expect(store.setRuntimeEnvironments).toHaveBeenCalledExactlyOnceWith(environments) + expect(api.getStatus).not.toHaveBeenCalled() + expect(store.readRuntimeHostStatusSnapshots).not.toHaveBeenCalled() + }) + + it('still reconciles a mutation catalog reload invoked after close', async () => { + const api = installBridge() + const environments = [environment('remaining')] + api.list.mockResolvedValueOnce([]).mockResolvedValueOnce(environments) + const { result, unmount } = renderHook(() => useRuntimeEnvironmentCatalog()) + await act(async () => undefined) + const reloadAfterMutation = result.current.loadEnvironments + unmount() + store.setRuntimeEnvironments.mockClear() + + await reloadAfterMutation() + + expect(api.list).toHaveBeenCalledTimes(2) + expect(store.setRuntimeEnvironments).toHaveBeenCalledExactlyOnceWith(environments) + expect(api.getStatus).not.toHaveBeenCalled() + }) + + it('does not start sibling probes after a verified status wait outlives the pane', async () => { + const api = installBridge() + api.list + .mockResolvedValueOnce([]) + .mockResolvedValueOnce([environment('verified'), environment('other')]) + const { result, unmount } = renderHook(() => useRuntimeEnvironmentCatalog()) + await act(async () => undefined) + const snapshot = Promise.withResolvers() + store.readRuntimeHostStatusSnapshots.mockReturnValueOnce(snapshot.promise) + let reload: Promise | undefined + await act(async () => { + reload = result.current.loadEnvironments({ + environmentId: 'verified', + runtimeStatus: verifiedStatus() + }) + }) + expect(store.readRuntimeHostStatusSnapshots).toHaveBeenCalledOnce() + unmount() + + await act(async () => { + snapshot.resolve() + await reload + }) + + expect(api.getStatus).not.toHaveBeenCalled() + expect(store.readRuntimeHostStatusSnapshots).toHaveBeenCalledOnce() + }) + + it('preserves active probing and skips verified and ephemeral servers', async () => { + const api = installBridge() + const environments = [ + environment('verified'), + environment('other'), + environment('ephemeral', 'ephemeral-vm') + ] + api.list.mockResolvedValueOnce([]).mockResolvedValueOnce(environments) + const { result } = renderHook(() => useRuntimeEnvironmentCatalog()) + await act(async () => undefined) + + await act(async () => { + await result.current.loadEnvironments({ + environmentId: 'verified', + runtimeStatus: verifiedStatus() + }) + }) + + expect(api.getStatus).toHaveBeenCalledExactlyOnceWith({ + selector: 'other', + timeoutMs: 10_000 + }) + expect(store.readRuntimeHostStatusSnapshots).toHaveBeenCalledTimes(2) + expect(result.current.environments).toEqual(environments.slice(0, 2)) + expect(result.current.detailsByEnvironmentId.verified.status).toBe('ready') + expect(result.current.detailsByEnvironmentId.other.status).toBe('ready') + expect(result.current.isLoading).toBe(false) + }) + + it('allows an already started probe to synchronize shared status after close', async () => { + const api = installBridge() + const status = Promise.withResolvers>() + api.list.mockResolvedValue([environment('started')]) + api.getStatus.mockReturnValue(status.promise) + const { unmount } = renderHook(() => useRuntimeEnvironmentCatalog()) + await act(async () => undefined) + expect(api.getStatus).toHaveBeenCalledOnce() + unmount() + + await act(async () => status.resolve(createCompatibleRuntimeStatusResponse())) + + expect(store.readRuntimeHostStatusSnapshots).toHaveBeenCalledOnce() + }) + + it('does not report a late catalog error after close', async () => { + const api = installBridge() + const list = Promise.withResolvers() + api.list.mockReturnValue(list.promise) + const { unmount } = renderHook(() => useRuntimeEnvironmentCatalog()) + unmount() + + await act(async () => list.reject(new Error('controlled unavailable response'))) + + expect(store.toastError).not.toHaveBeenCalled() + expect(api.getStatus).not.toHaveBeenCalled() + }) +}) diff --git a/src/renderer/src/components/settings/use-runtime-environment-catalog.ts b/src/renderer/src/components/settings/use-runtime-environment-catalog.ts index 5470bf5b6e48..ee637d5db819 100644 --- a/src/renderer/src/components/settings/use-runtime-environment-catalog.ts +++ b/src/renderer/src/components/settings/use-runtime-environment-catalog.ts @@ -50,8 +50,15 @@ export function useRuntimeEnvironmentCatalog(): RuntimeEnvironmentCatalog { // Why: drop store status for servers no longer saved so stale hosts don't // linger in the sidebar registry. useAppStore.getState().setRuntimeEnvironments(nextEnvironments) + // Keep mutation catalog updates, but do not start probes for a closed pane. + if (!mountedRef.current) { + return + } if (verified) { await useAppStore.getState().readRuntimeHostStatusSnapshots() + if (!mountedRef.current) { + return + } } if (mountedRef.current) { setEnvironments(visibleEnvironments) From 12e0ced01d8c68659631116a3ce5570791ec60e4 Mon Sep 17 00:00:00 2001 From: Neil Date: Sat, 26 Sep 2026 21:56:40 -0700 Subject: [PATCH 2/2] review(settings-status-lifetime): guard the reopen path and drop count-only assertions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The suppression is keyed to useMountedRef, which is per-open-session, so a reopened pane reloads and reprobes from scratch. Nothing tested that, and the guard now sits in front of every probe: if the mounted flag ever failed to re-arm on mount, the pane would issue zero probes and sit on 'loading' forever instead of merely skipping a setState. Added a close-then-reopen test and a StrictMode double-invoked-effects test (both fail if the flag is not re-armed), including the assertion that a host whose probe was suppressed stays 'loading' (never checked) rather than 'error' — a probe that did not run is not evidence the host is down. Also removed the now-dead mounted re-check (no suspension point between it and the preceding guard) and relaxed the incidental snapshot-read count assertion, which would have flagged coalescing those reads as a regression. --- ...ntime-environment-catalog-lifetime.test.ts | 58 ++++++++++++++++++- .../use-runtime-environment-catalog.ts | 52 +++++++++-------- 2 files changed, 84 insertions(+), 26 deletions(-) diff --git a/src/renderer/src/components/settings/use-runtime-environment-catalog-lifetime.test.ts b/src/renderer/src/components/settings/use-runtime-environment-catalog-lifetime.test.ts index 2ab06e5cf0de..1e88f20fee56 100644 --- a/src/renderer/src/components/settings/use-runtime-environment-catalog-lifetime.test.ts +++ b/src/renderer/src/components/settings/use-runtime-environment-catalog-lifetime.test.ts @@ -1,5 +1,6 @@ // @vitest-environment happy-dom import { act, cleanup, renderHook } from '@testing-library/react' +import { StrictMode } from 'react' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import type { PublicKnownRuntimeEnvironment } from '../../../../shared/runtime-environments' import type { RuntimeRpcResponse } from '../../../../shared/runtime-rpc-envelope' @@ -60,6 +61,12 @@ function verifiedStatus(): RuntimeStatus { return response.result } +async function flush(): Promise { + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 0)) + }) +} + beforeEach(() => { vi.resetAllMocks() store.readRuntimeHostStatusSnapshots.mockResolvedValue(undefined) @@ -153,7 +160,8 @@ describe('runtime environment catalog pane lifetime', () => { selector: 'other', timeoutMs: 10_000 }) - expect(store.readRuntimeHostStatusSnapshots).toHaveBeenCalledTimes(2) + // Shared status is synchronized; coalescing those reads is not a regression. + expect(store.readRuntimeHostStatusSnapshots).toHaveBeenCalled() expect(result.current.environments).toEqual(environments.slice(0, 2)) expect(result.current.detailsByEnvironmentId.verified.status).toBe('ready') expect(result.current.detailsByEnvironmentId.other.status).toBe('ready') @@ -175,6 +183,54 @@ describe('runtime environment catalog pane lifetime', () => { expect(store.readRuntimeHostStatusSnapshots).toHaveBeenCalledOnce() }) + it("probes again on reopen instead of inheriting the closed pane's suppression", async () => { + const api = installBridge() + const firstList = Promise.withResolvers() + const environments = [environment('host-a'), environment('host-b')] + api.list.mockReturnValueOnce(firstList.promise).mockResolvedValue(environments) + const closed = renderHook(() => useRuntimeEnvironmentCatalog()) + + closed.unmount() + await act(async () => firstList.resolve(environments)) + + expect(api.getStatus).not.toHaveBeenCalled() + // A probe that never ran is not evidence a host is unreachable. + expect(Object.values(closed.result.current.detailsByEnvironmentId)).not.toContainEqual( + expect.objectContaining({ status: 'error' }) + ) + + const status = Promise.withResolvers>() + api.getStatus.mockReturnValue(status.promise) + const reopened = renderHook(() => useRuntimeEnvironmentCatalog()) + await flush() + + expect(api.getStatus).toHaveBeenCalledTimes(2) + expect( + Object.values(reopened.result.current.detailsByEnvironmentId).map((details) => details.status) + ).toEqual(['loading', 'loading']) + + await act(async () => status.resolve(createCompatibleRuntimeStatusResponse())) + await flush() + + expect(reopened.result.current.environments).toEqual(environments) + expect(reopened.result.current.detailsByEnvironmentId['host-a'].status).toBe('ready') + expect(reopened.result.current.detailsByEnvironmentId['host-b'].status).toBe('ready') + expect(reopened.result.current.isLoading).toBe(false) + }) + + it('still finishes the initial load under StrictMode double-invoked effects', async () => { + const api = installBridge() + api.list.mockResolvedValue([environment('host-a')]) + + const { result } = renderHook(() => useRuntimeEnvironmentCatalog(), { wrapper: StrictMode }) + await flush() + + // React's simulated unmount/remount reuses the mounted ref; suppression must not stick. + expect(api.getStatus).toHaveBeenCalled() + expect(result.current.detailsByEnvironmentId['host-a'].status).toBe('ready') + expect(result.current.isLoading).toBe(false) + }) + it('does not report a late catalog error after close', async () => { const api = installBridge() const list = Promise.withResolvers() diff --git a/src/renderer/src/components/settings/use-runtime-environment-catalog.ts b/src/renderer/src/components/settings/use-runtime-environment-catalog.ts index ee637d5db819..7cc981b1eb34 100644 --- a/src/renderer/src/components/settings/use-runtime-environment-catalog.ts +++ b/src/renderer/src/components/settings/use-runtime-environment-catalog.ts @@ -51,6 +51,8 @@ export function useRuntimeEnvironmentCatalog(): RuntimeEnvironmentCatalog { // linger in the sidebar registry. useAppStore.getState().setRuntimeEnvironments(nextEnvironments) // Keep mutation catalog updates, but do not start probes for a closed pane. + // mountedRef is per-open-session (useMountedRef re-arms it on every mount), so a + // reopened pane loads and probes from scratch rather than inheriting suppression. if (!mountedRef.current) { return } @@ -60,31 +62,31 @@ export function useRuntimeEnvironmentCatalog(): RuntimeEnvironmentCatalog { return } } - if (mountedRef.current) { - setEnvironments(visibleEnvironments) - setDetailsByEnvironmentId((current) => { - const next: Record = {} - for (const environment of visibleEnvironments) { - next[environment.id] = - verified?.environmentId === environment.id - ? { - status: 'ready', - runtimeStatus: verified.runtimeStatus, - remoteControl: verified.runtimeStatus.remoteControl ?? null, - compatibility: evaluateHostDetails(verified.runtimeStatus), - error: null - } - : (current[environment.id] ?? { - status: 'loading', - runtimeStatus: null, - remoteControl: null, - compatibility: null, - error: null - }) - } - return next - }) - } + setEnvironments(visibleEnvironments) + setDetailsByEnvironmentId((current) => { + const next: Record = {} + for (const environment of visibleEnvironments) { + next[environment.id] = + verified?.environmentId === environment.id + ? { + status: 'ready', + runtimeStatus: verified.runtimeStatus, + remoteControl: verified.runtimeStatus.remoteControl ?? null, + compatibility: evaluateHostDetails(verified.runtimeStatus), + error: null + } + : // A host whose probe is skipped stays 'loading' (never checked), never + // 'error' — a probe that did not run is not evidence the host is down. + (current[environment.id] ?? { + status: 'loading', + runtimeStatus: null, + remoteControl: null, + compatibility: null, + error: null + }) + } + return next + }) await Promise.allSettled( visibleEnvironments .filter((environment) => environment.id !== verified?.environmentId)