Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/views/workflow-history-v2/workflow-history-v2.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import usePageFilters from '@/components/page-filters/hooks/use-page-filters';
import decodeUrlParams from '@/utils/decode-url-params';

import workflowHistoryFiltersConfig from '../workflow-history/config/workflow-history-filters.config';
import WORKFLOW_HISTORY_PAGE_SIZE_CONFIG from '../workflow-history/config/workflow-history-page-size.config';
import { WORKFLOW_HISTORY_PAGE_SIZE_CONFIG } from '../workflow-history/config/workflow-history-page-size.config';
import { WorkflowHistoryContext } from '../workflow-history/workflow-history-context-provider/workflow-history-context-provider';
import workflowPageQueryParamsConfig from '../workflow-page/config/workflow-page-query-params.config';
import { type WorkflowPageTabContentParams } from '../workflow-page/workflow-page-tab-content/workflow-page-tab-content.types';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,9 @@ jest.mock('../hooks/use-workflow-history-fetcher', () => {
const actual = jest.requireActual('../hooks/use-workflow-history-fetcher');
return {
__esModule: true,
default: jest.fn((params) => actual.default(params, 0)), // 0ms throttle for tests
default: jest.fn((params, onEventsChange) =>
actual.default(params, onEventsChange, 0)
), // 0ms throttle for tests
};
});

Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,2 @@
const WORKFLOW_HISTORY_PAGE_SIZE_CONFIG = 200;

export default WORKFLOW_HISTORY_PAGE_SIZE_CONFIG;
export const WORKFLOW_HISTORY_PAGE_SIZE_CONFIG = 1000;
export const WORKFLOW_HISTORY_FIRST_PAGE_SIZE_CONFIG = 200;
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,15 @@ const RETRY_COUNT = 3;
let queryClient: QueryClient;
let hoistedFetcher: WorkflowHistoryFetcher;

jest.mock(
'@/views/workflow-history/config/workflow-history-page-size.config',
() => ({
__esModule: true,
WORKFLOW_HISTORY_FIRST_PAGE_SIZE_CONFIG: 200,
WORKFLOW_HISTORY_PAGE_SIZE_CONFIG: 1000,
})
);

describe(WorkflowHistoryFetcher.name, () => {
beforeEach(() => {
queryClient = new QueryClient({
Expand Down Expand Up @@ -262,6 +271,35 @@ describe(WorkflowHistoryFetcher.name, () => {
const state = fetcher.getCurrentState();
expect(state.data?.pages.length).toBe(pageCountBefore);
});

it('should use WORKFLOW_HISTORY_FIRST_PAGE_SIZE_CONFIG for the first page', async () => {
const { fetcher, getCapturedPageSizes } = setup(queryClient);

fetcher.start((state) => !state?.data?.pages?.length);

await waitFor(() => {
const state = fetcher.getCurrentState();
expect(state.data?.pages).toHaveLength(1);
});

const pageSizes = getCapturedPageSizes();
expect(pageSizes[0]).toBe('200');
});

it('should use WORKFLOW_HISTORY_PAGE_SIZE_CONFIG for subsequent pages', async () => {
const { fetcher, getCapturedPageSizes } = setup(queryClient);

fetcher.start((state) => (state.data?.pages.length || 0) < 3);

await waitFor(() => {
const state = fetcher.getCurrentState();
expect(state.data?.pages).toHaveLength(3);
});

const pageSizes = getCapturedPageSizes();
expect(pageSizes[1]).toBe('1000');
expect(pageSizes[2]).toBe('1000');
});
});

function setup(client: QueryClient, options: { failOnPages?: number[] } = {}) {
Expand All @@ -273,7 +311,10 @@ function setup(client: QueryClient, options: { failOnPages?: number[] } = {}) {
pageSize: 10,
};

mockHistoryEndpoint(workflowHistoryMultiPageFixture, options.failOnPages);
const { getCapturedPageSizes } = mockHistoryEndpoint(
workflowHistoryMultiPageFixture,
options.failOnPages
);
const fetcher = new WorkflowHistoryFetcher(client, params);
hoistedFetcher = fetcher;

Expand All @@ -292,13 +333,16 @@ function setup(client: QueryClient, options: { failOnPages?: number[] } = {}) {
fetcher,
params,
waitForData,
getCapturedPageSizes,
};
}

function mockHistoryEndpoint(
responses: GetWorkflowHistoryResponse[],
failOnPages: number[] = []
) {
const capturedPageSizes: string[] = [];

mswMockEndpoints([
{
path: '/api/domains/:domain/:cluster/workflows/:workflowId/:runId/history',
Expand All @@ -307,6 +351,9 @@ function mockHistoryEndpoint(
httpResolver: async ({ request }) => {
const url = new URL(request.url);
const nextPage = url.searchParams.get('nextPage');
const pageSize = url.searchParams.get('pageSize');

capturedPageSizes.push(pageSize ?? '');

// Determine current page number based on nextPage param
let pageNumber = 1;
Expand Down Expand Up @@ -334,4 +381,8 @@ function mockHistoryEndpoint(
},
},
]);

return {
getCapturedPageSizes: () => capturedPageSizes,
};
}
25 changes: 16 additions & 9 deletions src/views/workflow-history/helpers/workflow-history-fetcher.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,11 @@ import {
} from '@/route-handlers/get-workflow-history/get-workflow-history.types';
import request from '@/utils/request';

import {
WORKFLOW_HISTORY_FIRST_PAGE_SIZE_CONFIG,
WORKFLOW_HISTORY_PAGE_SIZE_CONFIG,
} from '../config/workflow-history-page-size.config';

import {
type WorkflowHistoryQueryResult,
type QueryResultOnChangeCallback,
Expand Down Expand Up @@ -41,16 +46,18 @@ export default class WorkflowHistoryFetcher {
}

start(shouldContinue: ShouldContinueCallback = () => true): void {
if (shouldContinue) {
this.shouldContinue = shouldContinue;
}
// If already started, return
if (this.isStarted) return;
this.shouldContinue = shouldContinue;

// remove current listener (if exists) to have fresh emits only
this.unsubscribe?.();
this.unsubscribe = null;

this.isStarted = true;
let emitCount = 0;
const currentState = this.observer.getCurrentResult();
const fetchedFirstPage = currentState.status !== 'pending';
const shouldEnableQuery = !fetchedFirstPage && shouldContinue(currentState);
const shouldEnableQuery =
!fetchedFirstPage && this.shouldContinue(currentState);

if (shouldEnableQuery) {
this.observer.setOptions({
Expand Down Expand Up @@ -81,8 +88,6 @@ export default class WorkflowHistoryFetcher {
emit(currentState);
}

// remove current listener (if exists) and add new one
this.unsubscribe?.();
this.unsubscribe = this.observer.subscribe((res) => emit(res));
}

Expand Down Expand Up @@ -126,7 +131,9 @@ export default class WorkflowHistoryFetcher {
url: `/api/domains/${params.domain}/${params.cluster}/workflows/${params.workflowId}/${params.runId}/history`,
query: {
nextPage: pageParam,
pageSize: params.pageSize,
pageSize: pageParam
? WORKFLOW_HISTORY_PAGE_SIZE_CONFIG
: WORKFLOW_HISTORY_FIRST_PAGE_SIZE_CONFIG,
waitForNewEvent: params.waitForNewEvent ?? false,
} satisfies WorkflowHistoryQueryParams,
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,17 @@ let mockOnChangeCallback: jest.Mock;
let mockUnsubscribe: jest.Mock;

function setup() {
const hookResult = renderHook(() => useWorkflowHistoryFetcher(mockParams));
const mockOnEventsChange = jest.fn();
const hookResult = renderHook(() =>
useWorkflowHistoryFetcher(mockParams, mockOnEventsChange)
);

return {
...hookResult,
mockFetcherInstance,
mockOnChangeCallback,
mockUnsubscribe,
mockOnEventsChange,
};
}

Expand Down
20 changes: 15 additions & 5 deletions src/views/workflow-history/hooks/use-workflow-history-fetcher.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
useQueryClient,
} from '@tanstack/react-query';

import { type HistoryEvent } from '@/__generated__/proto-ts/uber/cadence/api/v1/HistoryEvent';
import useThrottledState from '@/hooks/use-throttled-state';
import {
type WorkflowHistoryQueryParams,
Expand All @@ -19,13 +20,18 @@ import { type ShouldContinueCallback } from '../helpers/workflow-history-fetcher

export default function useWorkflowHistoryFetcher(
params: WorkflowHistoryQueryParams & RouteParams,
onEventsChange: (events: HistoryEvent[]) => void,
throttleMs: number = 2000
) {
const queryClient = useQueryClient();
const fetcherRef = useRef<WorkflowHistoryFetcher | null>(null);
const lastFlattenedPagesCountRef = useRef<number>(-1);

if (!fetcherRef.current) {
fetcherRef.current = new WorkflowHistoryFetcher(queryClient, params);

// Fetch first page
fetcherRef.current.start((state) => !state?.data?.pages?.length);
}

const [historyQuery, setHistoryQuery] = useThrottledState<
Expand All @@ -40,21 +46,25 @@ export default function useWorkflowHistoryFetcher(

useEffect(() => {
if (!fetcherRef.current) return;

const unsubscribe = fetcherRef.current.onChange((state) => {
const pagesCount = state.data?.pages?.length || 0;
// If the pages count is greater than the last flattened pages count, then we need to flatten the pages and call the onEventsChange callback
// Depending on ref variable instead of historyQuery is because historyQuery is throttled.
if (pagesCount > lastFlattenedPagesCountRef.current) {
lastFlattenedPagesCountRef.current = pagesCount;
onEventsChange(
state.data?.pages?.flatMap((page) => page.history?.events || []) || []
);
}
Comment on lines +53 to +58
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I must be missing something.

My understanding:

  • When onChange fires
  • Then emit onEventsChange with fresh data

I would have expected:

  • Within onChange
  • Call onEventsChange with state.data.pages

I'm a bit confused why we don't pass state.data.pages directly (bypassing the flattening step)?

The hook, as currently implemented, knows too much about the fetcher internals.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this is a follow up question on this or the message didn't show up for you as it was outdated. We can continue the discussion there.

// immediately set if there is the first page without throttling other wise throttle
const executeImmediately = pagesCount <= 1;
setHistoryQuery(() => state, executeImmediately);
});

// Fetch first page
fetcherRef.current.start((state) => !state?.data?.pages?.length);

return () => {
unsubscribe();
};
}, [setHistoryQuery]);
}, [setHistoryQuery, onEventsChange]);

useEffect(() => {
return () => {
Expand Down
4 changes: 3 additions & 1 deletion src/views/workflow-history/workflow-history.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import workflowPageQueryParamsConfig from '../workflow-page/config/workflow-page
import { useSuspenseDescribeWorkflow } from '../workflow-page/hooks/use-describe-workflow';

import workflowHistoryFiltersConfig from './config/workflow-history-filters.config';
import WORKFLOW_HISTORY_PAGE_SIZE_CONFIG from './config/workflow-history-page-size.config';
import { WORKFLOW_HISTORY_PAGE_SIZE_CONFIG } from './config/workflow-history-page-size.config';
import compareUngroupedEvents from './helpers/compare-ungrouped-events';
import getSortableEventId from './helpers/get-sortable-event-id';
import getVisibleGroupsHasMissingEvents from './helpers/get-visible-groups-has-missing-events';
Expand Down Expand Up @@ -73,6 +73,8 @@ export default function WorkflowHistory({ params }: Props) {
pageSize: wfHistoryRequestArgs.pageSize,
waitForNewEvent: wfHistoryRequestArgs.waitForNewEvent,
},
//TODO: @assem.hafez replace this with grouper callback
() => {},
2000
);

Expand Down