Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
Next Next commit
fetcher updates
  • Loading branch information
Assem-Uber committed Nov 20, 2025
commit cc38cb121f2c061abfae9ff18b64efb5bfb0776b
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
@@ -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,
};
}
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 @@ -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
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
2 changes: 1 addition & 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 { 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,7 +73,7 @@
pageSize: wfHistoryRequestArgs.pageSize,
waitForNewEvent: wfHistoryRequestArgs.waitForNewEvent,
},
2000

Check failure on line 76 in src/views/workflow-history/workflow-history.tsx

View workflow job for this annotation

GitHub Actions / build

Argument of type 'number' is not assignable to parameter of type '(events: HistoryEvent[]) => void'.
);

const [resetToDecisionEventId, setResetToDecisionEventId] = useState<
Expand Down
Loading