From fc30645ff740e914708a20f1fa1e2e118f771433 Mon Sep 17 00:00:00 2001 From: Bat-Zion Rotman Date: Wed, 24 Jan 2024 18:07:20 +0200 Subject: [PATCH] feat(orchestrator): add auto refresh to workflow instance list and details pages (#1081) * feat(orchestrator): add auto refresh to workflow runs tab and to workflow execution results page * move usePolling to hooks directory * improve polling and add unit test for usePolling hook * use swr for polling * use existing version of swr --- plugins/orchestrator/package.json | 4 + .../src/__fixtures__/fakeProcessInstance.ts | 8 +- .../src/api/MockOrchestratorClient.ts | 4 +- .../WorkflowInstancePage.stories.tsx | 46 ++++- .../src/components/WorkflowInstancePage.tsx | 32 ++-- .../src/components/WorkflowRunsTabContent.tsx | 6 +- plugins/orchestrator/src/constants.ts | 2 + .../orchestrator/src/hooks/usePolling.test.ts | 165 ++++++++++++++++++ plugins/orchestrator/src/hooks/usePolling.ts | 55 ++++++ 9 files changed, 297 insertions(+), 25 deletions(-) create mode 100644 plugins/orchestrator/src/hooks/usePolling.test.ts create mode 100644 plugins/orchestrator/src/hooks/usePolling.ts diff --git a/plugins/orchestrator/package.json b/plugins/orchestrator/package.json index da3eef0858..d011c1d776 100644 --- a/plugins/orchestrator/package.json +++ b/plugins/orchestrator/package.json @@ -78,6 +78,8 @@ "react-json-view": "^1.21.3", "react-moment": "^1.1.3", "react-use": "^17.4.0", + "swr": "^2.0.0", + "uuid": "^9.0.1", "vscode-languageserver-types": "^3.16.0" }, "devDependencies": { @@ -89,6 +91,8 @@ "@backstage/test-utils": "^1.4.4", "@janus-idp/cli": "1.5.0", "@storybook/react": "^7.5.3", + "@testing-library/react": "^12.1.5", + "@testing-library/react-hooks": "8.0.1", "@types/json-schema": "^7.0.12", "css-loader": "^6.5.1", "file-loader": "^5.0.2", diff --git a/plugins/orchestrator/src/__fixtures__/fakeProcessInstance.ts b/plugins/orchestrator/src/__fixtures__/fakeProcessInstance.ts index f9aee53423..9fca34521c 100644 --- a/plugins/orchestrator/src/__fixtures__/fakeProcessInstance.ts +++ b/plugins/orchestrator/src/__fixtures__/fakeProcessInstance.ts @@ -48,7 +48,7 @@ export const fakeProcessInstance1: ProcessInstance = { }, }; -export const fakeProcessInstance2: ProcessInstance = { +export const fakeCompletedInstance: ProcessInstance = { id: `12f767c1-9002-43af-9515-62a72d0eaf${id++}`, processName: fakeWorkflowOverviewList[1].name, processId: fakeWorkflowOverviewList[1].workflowId, @@ -65,7 +65,7 @@ export const fakeProcessInstance2: ProcessInstance = { description: 'test description 2', }; -export const fakeProcessInstance3: ProcessInstance = { +export const fakeActiveInstance: ProcessInstance = { id: `12f767c1-9002-43af-9515-62a72d0eaf${id++}`, processName: fakeWorkflowOverviewList[2].name, processId: fakeWorkflowOverviewList[2].workflowId, @@ -99,7 +99,7 @@ export const fakeProcessInstance4: ProcessInstance = { export const fakeProcessInstances = [ fakeProcessInstance1, - fakeProcessInstance2, - fakeProcessInstance3, + fakeCompletedInstance, + fakeActiveInstance, fakeProcessInstance4, ]; diff --git a/plugins/orchestrator/src/api/MockOrchestratorClient.ts b/plugins/orchestrator/src/api/MockOrchestratorClient.ts index 0d62a53eab..ed059ed82e 100644 --- a/plugins/orchestrator/src/api/MockOrchestratorClient.ts +++ b/plugins/orchestrator/src/api/MockOrchestratorClient.ts @@ -23,7 +23,7 @@ export interface MockOrchestratorApiData { OrchestratorApi['deleteWorkflowDefinition'] >; executeWorkflowResponse: () => ReturnType; - getInstanceResponse: ReturnType; + getInstanceResponse: () => ReturnType; getInstancesResponse: ReturnType; getInstanceJobsResponse: ReturnType; getSpecsResponse: ReturnType; @@ -94,7 +94,7 @@ export class MockOrchestratorClient implements OrchestratorApi { throw new Error(`[getInstance]: No mock data available`); } - return Promise.resolve(this._mockData.getInstanceResponse); + return Promise.resolve(this._mockData.getInstanceResponse()); } getInstanceJobs(_instanceId: string): Promise { diff --git a/plugins/orchestrator/src/components/WorkflowInstancePage.stories.tsx b/plugins/orchestrator/src/components/WorkflowInstancePage.stories.tsx index b8ed2d380e..41fbbc2011 100644 --- a/plugins/orchestrator/src/components/WorkflowInstancePage.stories.tsx +++ b/plugins/orchestrator/src/components/WorkflowInstancePage.stories.tsx @@ -10,7 +10,11 @@ import { WorkflowSpecFile, } from '@janus-idp/backstage-plugin-orchestrator-common'; -import { fakeProcessInstances } from '../__fixtures__/fakeProcessInstance'; +import { + fakeActiveInstance, + fakeCompletedInstance, + fakeProcessInstances, +} from '../__fixtures__/fakeProcessInstance'; import { fakeWorkflowItem } from '../__fixtures__/fakeWorkflowItem'; import { fakeWorkflowSpecs } from '../__fixtures__/fakeWorkflowSpecs'; import { orchestratorApiRef } from '../api'; @@ -25,6 +29,7 @@ const delay = (timeMs: number) => { }; const getFakeProcessInstance = async ( + context: { responseCounter?: number }, instanceId?: string, ): Promise => { if (instanceId === '__loading__') { @@ -36,6 +41,22 @@ const getFakeProcessInstance = async ( return fakeProcessInstances[0]; } + if (instanceId === '__auto_refresh__') { + const ret = !context.responseCounter + ? Promise.resolve(fakeActiveInstance) + : Promise.resolve(fakeCompletedInstance); + context.responseCounter = (context.responseCounter || 0) + 1; + return ret; + } + + if (instanceId === '__auto_refresh_three_errors__') { + const ret = !context.responseCounter + ? Promise.resolve(fakeActiveInstance) + : Promise.reject(new Error('Failed to fetch')); + context.responseCounter = (context.responseCounter || 0) + 1; + return ret; + } + throw new Error('This is an example error for non existing instance'); }; @@ -70,9 +91,12 @@ const meta = { getWorkflowResponse: getFakeWorkflowItem( context.args.instanceId, ), - getInstanceResponse: getFakeProcessInstance( - context.args.instanceId, - ), + getInstanceResponse: () => { + return getFakeProcessInstance( + context as { responseCounter?: number }, + context.args.instanceId, + ); + }, }), ], ]} @@ -114,3 +138,17 @@ export const ErrorStory: Story = { instanceId: '__non_existing_id__', }, }; + +export const AutoRefreshErrors: Story = { + name: 'Auto refresh', + args: { + instanceId: '__auto_refresh__', + }, +}; + +export const AutoRefresh3Errors: Story = { + name: 'Auto refresh three errors', + args: { + instanceId: '__auto_refresh_three_errors__', + }, +}; diff --git a/plugins/orchestrator/src/components/WorkflowInstancePage.tsx b/plugins/orchestrator/src/components/WorkflowInstancePage.tsx index 276ab31fec..3d4632b8e7 100644 --- a/plugins/orchestrator/src/components/WorkflowInstancePage.tsx +++ b/plugins/orchestrator/src/components/WorkflowInstancePage.tsx @@ -1,5 +1,4 @@ import React from 'react'; -import { useAsyncRetry } from 'react-use'; import { ContentHeader, @@ -10,7 +9,11 @@ import { useApi, useRouteRefParams } from '@backstage/core-plugin-api'; import { Button, Grid } from '@material-ui/core'; +import { ProcessInstance } from '@janus-idp/backstage-plugin-orchestrator-common'; + import { orchestratorApiRef } from '../api'; +import { SHORT_REFRESH_INTERVAL } from '../constants'; +import usePolling from '../hooks/usePolling'; import { workflowInstanceRouteRef } from '../routes'; import { isNonNullable } from '../utils/TypeGuards'; import { BaseOrchestratorPage } from './BaseOrchestratorPage'; @@ -26,14 +29,19 @@ export const WorkflowInstancePage = ({ workflowInstanceRouteRef, ); - const { loading, error, value, retry } = useAsyncRetry(async () => { - if (!instanceId && !queryInstanceId) { - return undefined; - } - return await orchestratorApi.getInstance(instanceId || queryInstanceId); - }, [orchestratorApi, queryInstanceId]); - - const isReady = React.useMemo(() => !loading && !error, [loading, error]); + const { loading, error, value, restart } = usePolling< + ProcessInstance | undefined + >( + async () => { + if (!instanceId && !queryInstanceId) { + return undefined; + } + return await orchestratorApi.getInstance(instanceId || queryInstanceId); + }, + SHORT_REFRESH_INTERVAL, + (curValue: ProcessInstance | undefined) => + !!curValue && curValue.state === 'ACTIVE', + ); const handleAbort = React.useCallback(async () => { if (value) { @@ -45,7 +53,7 @@ export const WorkflowInstancePage = ({ if (yes) { try { await orchestratorApi.abortWorkflow(value.id); - retry(); + restart(); } catch (e) { // eslint-disable-next-line no-alert window.alert( @@ -56,7 +64,7 @@ export const WorkflowInstancePage = ({ } } } - }, [orchestratorApi, retry, value]); + }, [orchestratorApi, restart, value]); return ( {loading ? : null} {error ? : null} - {isReady && isNonNullable(value) ? ( + {!loading && isNonNullable(value) ? ( <> diff --git a/plugins/orchestrator/src/components/WorkflowRunsTabContent.tsx b/plugins/orchestrator/src/components/WorkflowRunsTabContent.tsx index 2a7568aa9d..7c615af62a 100644 --- a/plugins/orchestrator/src/components/WorkflowRunsTabContent.tsx +++ b/plugins/orchestrator/src/components/WorkflowRunsTabContent.tsx @@ -1,5 +1,4 @@ import React, { useState } from 'react'; -import { useAsync } from 'react-use'; import { ErrorPanel, @@ -20,6 +19,7 @@ import { import { orchestratorApiRef } from '../api'; import { VALUE_UNAVAILABLE } from '../constants'; +import usePolling from '../hooks/usePolling'; import { workflowInstanceRouteRef } from '../routes'; import { capitalize, ellipsis } from '../utils/StringUtils'; import { Selector } from './Selector'; @@ -48,14 +48,14 @@ export const WorkflowRunsTabContent = () => { Selector.AllItems, ); - const { loading, error, value } = useAsync(async () => { + const { loading, error, value } = usePolling(async () => { const instances = await orchestratorApi.getInstances(); const clonedData: WorkflowRunDetail[] = instances.map( mapProcessInstanceToDetails, ); return clonedData; - }, [orchestratorApi]); + }); const columns = React.useMemo( (): TableColumn[] => [ diff --git a/plugins/orchestrator/src/constants.ts b/plugins/orchestrator/src/constants.ts index c1f6017782..436daaf048 100644 --- a/plugins/orchestrator/src/constants.ts +++ b/plugins/orchestrator/src/constants.ts @@ -1 +1,3 @@ export const VALUE_UNAVAILABLE = '---' as const; +export const SHORT_REFRESH_INTERVAL = 5000; +export const LONG_REFRESH_INTERVAL = 15000; diff --git a/plugins/orchestrator/src/hooks/usePolling.test.ts b/plugins/orchestrator/src/hooks/usePolling.test.ts new file mode 100644 index 0000000000..7503d6c237 --- /dev/null +++ b/plugins/orchestrator/src/hooks/usePolling.test.ts @@ -0,0 +1,165 @@ +import { act } from '@testing-library/react'; +import { renderHook } from '@testing-library/react-hooks'; + +import { SHORT_REFRESH_INTERVAL } from '../constants'; +import usePolling from './usePolling'; + +const ACTIVE = 'active'; +const ACTIVE1 = 'active1'; +const ABORTED = 'aborted'; +const COMPLETED = 'completed'; + +describe('usePolling', () => { + beforeAll(() => { + jest.useFakeTimers(); + }); + + test('should return loading true on initial load', async () => { + const mockAsyncFn = jest.fn().mockReturnValueOnce(new Promise(() => {})); + const { result } = renderHook(() => + usePolling(mockAsyncFn, SHORT_REFRESH_INTERVAL), + ); + expect(result.current.loading).toEqual(true); + }); + + test('should return loading false when loading, if not initial load', async () => { + const mockAsyncFn = jest + .fn() + .mockResolvedValueOnce(ACTIVE) + .mockReturnValueOnce(new Promise(() => {})); + const { result, waitForNextUpdate } = renderHook(() => + usePolling(mockAsyncFn, SHORT_REFRESH_INTERVAL), + ); + await waitForNextUpdate(); + expect(result.current.loading).toEqual(false); + }); + + test('should return correct initial value', async () => { + const mockAsyncFn = jest.fn().mockResolvedValueOnce(ACTIVE); + const { result, waitForNextUpdate } = renderHook(() => + usePolling(mockAsyncFn, SHORT_REFRESH_INTERVAL), + ); + await waitForNextUpdate(); + expect(result.current.loading).toEqual(false); + expect(result.current.error).toEqual(undefined); + expect(result.current.value).toEqual(ACTIVE); + }); + + test('should return correct polled value', async () => { + const mockAsyncFn = jest + .fn() + .mockResolvedValueOnce(ACTIVE) + .mockResolvedValueOnce(ACTIVE1) + .mockResolvedValueOnce(COMPLETED); + const { result, waitForNextUpdate } = renderHook(() => + usePolling(mockAsyncFn, SHORT_REFRESH_INTERVAL), + ); + await waitForNextUpdate(); + expect(result.current.value).toEqual(ACTIVE); + await act(async () => jest.advanceTimersByTime(SHORT_REFRESH_INTERVAL)); + expect(result.current.value).toEqual(ACTIVE1); + await act(async () => jest.advanceTimersByTime(SHORT_REFRESH_INTERVAL)); + expect(result.current.value).toEqual(COMPLETED); + }); + + test('should not continue polling after continueRefresh callback returns false', async () => { + const continueRefresh = jest + .fn() + .mockImplementation((value: string) => value !== COMPLETED); + const mockAsyncFn = jest + .fn() + .mockResolvedValueOnce(ACTIVE) + .mockResolvedValueOnce(COMPLETED) + .mockResolvedValueOnce(ACTIVE1); + const { result, waitForNextUpdate } = renderHook(() => + usePolling(mockAsyncFn, SHORT_REFRESH_INTERVAL, continueRefresh), + ); + await waitForNextUpdate(); + expect(result.current.value).toEqual(ACTIVE); + for (let i = 0; i < 5; i++) { + await act(async () => jest.advanceTimersByTime(SHORT_REFRESH_INTERVAL)); + expect(result.current.value).toEqual(COMPLETED); + } + expect(mockAsyncFn).toHaveBeenCalledTimes(2); + }); + + test('should return error if fails on first load attempt', async () => { + const mockAsyncFn = jest.fn().mockRejectedValue('test error'); + const { result, waitForNextUpdate } = renderHook(() => + usePolling(mockAsyncFn, SHORT_REFRESH_INTERVAL), + ); + await waitForNextUpdate(); + expect(result.current.error).toEqual('test error'); + }); + + test('should not return error if fails on after first loading, on first polling error, and should preserve previous value', async () => { + const mockAsyncFn = jest + .fn() + .mockResolvedValueOnce(ACTIVE) + .mockResolvedValueOnce(ACTIVE1) + .mockResolvedValueOnce(COMPLETED) + .mockRejectedValueOnce('test error'); + const { result, waitForNextUpdate } = renderHook(() => + usePolling(mockAsyncFn, SHORT_REFRESH_INTERVAL), + ); + await waitForNextUpdate(); + for (let i = 0; i < 3; ++i) { + await act(async () => jest.advanceTimersByTime(SHORT_REFRESH_INTERVAL)); + } + expect(result.current.value).toEqual(COMPLETED); + expect(result.current.error).toBeUndefined(); + }); + + test('should return error if fails three times, and should preserve previous value, and stop polling', async () => { + const mockAsyncFn = jest + .fn() + .mockResolvedValueOnce(ACTIVE) + .mockResolvedValueOnce(ACTIVE1) + .mockRejectedValueOnce('test error') + .mockRejectedValueOnce('test error') + .mockRejectedValueOnce('test error') + .mockReturnValueOnce(COMPLETED); + const { result, waitForNextUpdate } = renderHook(() => + usePolling(mockAsyncFn, SHORT_REFRESH_INTERVAL), + ); + await waitForNextUpdate(); + for (let i = 0; i < 5; ++i) { + await act(async () => jest.advanceTimersByTime(SHORT_REFRESH_INTERVAL)); + } + expect(result.current.value).toEqual(ACTIVE1); + expect(result.current.error).toEqual('test error'); + expect(mockAsyncFn).toHaveBeenCalledTimes(5); + }); + + test('should continue polling after less than three errors during polling', async () => { + const mockAsyncFn = jest + .fn() + .mockResolvedValueOnce(ACTIVE) + .mockRejectedValueOnce('test error') + .mockRejectedValueOnce('test error') + .mockResolvedValueOnce(COMPLETED); + const { result, waitForNextUpdate } = renderHook(() => + usePolling(mockAsyncFn, SHORT_REFRESH_INTERVAL), + ); + await waitForNextUpdate(); + for (let i = 0; i < 3; ++i) { + await act(async () => jest.advanceTimersByTime(SHORT_REFRESH_INTERVAL)); + } + expect(result.current.value).toEqual(COMPLETED); + expect(result.current.error).toEqual(undefined); + }); + + test('should update value after restart', async () => { + const mockAsyncFn = jest + .fn() + .mockResolvedValueOnce(COMPLETED) + .mockResolvedValueOnce(ABORTED); + const { result, waitForNextUpdate } = renderHook(() => + usePolling(mockAsyncFn, SHORT_REFRESH_INTERVAL), + ); + await waitForNextUpdate(); + expect(result.current.value).toEqual(COMPLETED); + await act(async () => result.current.restart()); + expect(result.current.value).toEqual(ABORTED); + }); +}); diff --git a/plugins/orchestrator/src/hooks/usePolling.ts b/plugins/orchestrator/src/hooks/usePolling.ts new file mode 100644 index 0000000000..e2f1daf5b7 --- /dev/null +++ b/plugins/orchestrator/src/hooks/usePolling.ts @@ -0,0 +1,55 @@ +import React from 'react'; + +import useSwr, { useSWRConfig } from 'swr'; +import * as uuid from 'uuid'; + +import { LONG_REFRESH_INTERVAL } from '../constants'; + +const usePolling = ( + fn: () => Promise, + delayMs: number = LONG_REFRESH_INTERVAL, + continueRefresh?: (value: T | undefined) => boolean, + maxErrorRetryCount: number = 3, +) => { + const config = useSWRConfig(); + + const uniqueKey = React.useMemo(() => { + return uuid.v4(); + }, []); + + const [error, setError] = React.useState(); + const isInitalLoad = React.useRef(true); + + const { data, isLoading } = useSwr(uniqueKey, fn, { + refreshInterval: (value_: T | undefined) => { + return !continueRefresh || continueRefresh(value_) ? delayMs : 0; + }, + shouldRetryOnError: true, + onErrorRetry: (curError, _key, _config, revalidate, { retryCount }) => { + // requires custom behavior, retryErrorCount option doesn't support hiding the error before reaching the maximum + if (isInitalLoad.current || retryCount >= maxErrorRetryCount) { + setError(curError); + } else { + setTimeout(() => revalidate({ retryCount }), delayMs); + } + }, + onSuccess: () => { + isInitalLoad.current = false; + }, + }); + + React.useEffect(() => { + // clean cache after unmount, no need to store the data globally + return () => config.cache.delete(uniqueKey); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, []); + + return { + value: data, + error, + loading: isLoading, + restart: () => config.mutate(uniqueKey), + }; +}; + +export default usePolling;