Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 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 @@ -29,11 +29,14 @@ jest.mock('@/hooks/use-page-query-params/use-page-query-params', () =>
);

// Mock the hook to use minimal throttle delay for faster tests
// Mock the hooks to use minimal throttle delay for faster tests
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 @@ -262,6 +262,37 @@ 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).toHaveLength(1);
expect(pageSizes[0]).toBe('200');
Comment on lines 285 to 286
Copy link
Member

Choose a reason for hiding this comment

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

This seems to assert behavior of setup and mockEndpoints rather than the "unit under test".

Is this intended?

Copy link
Contributor Author

@Assem-Uber Assem-Uber Nov 21, 2025

Choose a reason for hiding this comment

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

Which part exactly is testing the setup method?

What the test does here is capturing what request msw received and make sure that the first request had a query param of 200. The query for rest APIs are sent from the fetcher but received and captured by the setup method.

Copy link
Member

@macrotim macrotim Nov 21, 2025

Choose a reason for hiding this comment

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

It sounds like your intent is to introspect the inner workings of the fetcher?

To suggest an alternative approach:

  • Let's generalize it beyond pageSize.
  • Unit tests need to assert correct internal behavior.
  • And asserting the outcome falls short.
  • One solution: The fetcher returns an introspection object.
  • Example: Return debugInfo which provides detailed insights.

I suggest this because the purpose of CapturedPageSizes wasn't immediately clear.

});

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) < 2);

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

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

function setup(client: QueryClient, options: { failOnPages?: number[] } = {}) {
Expand All @@ -273,7 +304,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 +326,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 +344,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 All @@ -328,10 +368,13 @@ function mockHistoryEndpoint(

// Map page number to response index (0-indexed)
const responseIndex = pageNumber - 1;
const response =
responses[responseIndex] || responses[responses.length - 1];
const response = responses[responseIndex];
return HttpResponse.json(response);
},
},
]);

return {
getCapturedPageSizes: () => capturedPageSizes,
};
}
21 changes: 15 additions & 6 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 @@ -44,13 +49,17 @@ export default class WorkflowHistoryFetcher {
if (shouldContinue) {
this.shouldContinue = shouldContinue;
Copy link
Member

Choose a reason for hiding this comment

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

What if start is called twice?

  1. start( shouldContinue )
  2. start( null )

It appears that the 2nd start would retain the prior shouldContinue. Is this intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

100% correct. You've just reminded me that i thought about it and forgot to change it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But to put more context, it is not an issue.
Null is not allowed as an argument and undefined would use the default method. It just doesn't make sense to keep the condition.

}
// If already started, return
if (this.isStarted) return;

// 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 +90,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 +133,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
15 changes: 11 additions & 4 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,17 @@ 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);

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 @@ -43,20 +48,22 @@ export default function useWorkflowHistoryFetcher(

const unsubscribe = fetcherRef.current.onChange((state) => {
const pagesCount = state.data?.pages?.length || 0;
onEventsChange(
state.data?.pages?.flatMap((page) => page.history?.events || []) || []
);
// 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(() => {
if (!fetcherRef.current) return;

return () => {
fetcherRef.current?.destroy();
};
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
Loading