feat: Fetcher updates #1090

Assem-Uber · 2025-11-20T09:48:12Z

Summary
This is a follow-up on fetcher PRs (#1062, #1063 , #1064 ) to add few missing features:

Make the size of first page smaller than subsequent pages.
Add onEventsChange prop to useWorkflowHistoryFetcher that allow reacting to events change faster that the throttled state. This will be used for non delayed grouping of events.
Move start directly under the creation of the instance.

Also fixing one issue with fetcher:

Move the unsubscribe to the to top of the start method to make sure that previous emit wont run after the new emit.

Testing
Tested as part of #1091

Copilot

Pull Request Overview

This PR enhances the workflow history fetcher with several improvements: smaller first page sizes for better initial load performance, a new callback mechanism for immediate event change reactions, and reorganized initialization logic.

Separated page size configuration into first page (200) and subsequent pages (1000) for optimized loading
Added onEventsChange callback to useWorkflowHistoryFetcher for non-throttled event change handling
Moved fetcher initialization (start()) to occur directly after instance creation

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
workflow-history.tsx	Updated import and added placeholder callback for `onEventsChange` parameter
use-workflow-history-fetcher.ts	Added `onEventsChange` parameter, moved `start()` call to initialization, and integrated callback in change handler
use-workflow-history-fetcher.test.tsx	Updated test setup to include mock `onEventsChange` callback
workflow-history-fetcher.ts	Implemented dynamic page sizing logic and refactored listener management
workflow-history-fetcher.test.tsx	Added tests for page size validation and enhanced mock endpoint to capture page sizes
workflow-history-page-size.config.ts	Split single page size constant into separate first and subsequent page size exports
workflow-history.test.tsx	Updated mock to pass through `onEventsChange` parameter
workflow-history-v2.tsx	Updated import to use named export

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/views/workflow-history/hooks/use-workflow-history-fetcher.ts

src/views/workflow-history/helpers/__tests__/workflow-history-fetcher.test.tsx

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

macrotim · 2025-11-20T18:27:12Z

src/views/workflow-history/helpers/workflow-history-fetcher.ts

    if (shouldContinue) {
      this.shouldContinue = shouldContinue;


What if start is called twice?

start( shouldContinue )

start( null )

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

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

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.

macrotim · 2025-11-20T18:52:02Z

src/views/workflow-history/hooks/use-workflow-history-fetcher.ts

+      // if the pages count is greater than the last flattened pages count, then we need to flatten the pages and call the onEventsChange callback
+      if (pagesCount > lastFlattenedPagesCount) {
+        lastFlattenedPagesCount = pagesCount;
+        onEventsChange(
+          state.data?.pages?.flatMap((page) => page.history?.events || []) || []
+        );
+      }


I'm not sure I follow.

It sounds like historyQuery is throttled.

On the other hand, the unthrottled flow is...

When onChange, then get state.

Take this state and flatten it (for what purpose?)

Then take the flattened result and pass it to onEventsChange (why?)

I'm probably missing something obvious with grouping in the history page.

Yes, history queue is throttled. Since it seems hard to understand this pattern i moved lastFlattenedPagesCount to be a ref outside the component.

The reason why we flatten is that Pages is a react-query construct. But helpers and other components accepts arrays of events. It made sense to send events here since the method name is onEventsChange. If we didn't do in this hook we would need to do it in the parent.

…tion

macrotim · 2025-11-20T23:08:06Z

src/views/workflow-history/helpers/__tests__/workflow-history-fetcher.test.tsx

+    const pageSizes = getCapturedPageSizes();
+    expect(pageSizes).toHaveLength(1);
+    expect(pageSizes[0]).toBe('200');


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

Is this intended?

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.

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.

macrotim · 2025-11-20T23:12:36Z

src/views/workflow-history/hooks/use-workflow-history-fetcher.ts

    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 historyQueue is because historyQueue is throttled.


Is there a typo, means historyQuery instead?

macrotim · 2025-11-20T23:18:47Z

src/views/workflow-history/hooks/use-workflow-history-fetcher.ts

+      if (pagesCount > lastFlattenedPagesCountRef.current) {
+        lastFlattenedPagesCountRef.current = pagesCount;
+        onEventsChange(
+          state.data?.pages?.flatMap((page) => page.history?.events || []) || []
+        );
+      }


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.

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.

fetcher updates

cc38cb1

Assem-Uber changed the title ~~fetcher updates~~ feat: Fetcher updates Nov 20, 2025

Assem-Uber added 4 commits November 20, 2025 10:03

add placeholder for fetcher

2a41fc6

update fetcher mock

1f54d95

fetcher start update

3398d8a

update todo

fb1f87c

adhityamamallan requested a review from Copilot November 20, 2025 11:12

Copilot started reviewing on behalf of adhityamamallan November 20, 2025 11:12 View session

Copilot finished reviewing on behalf of adhityamamallan November 20, 2025 11:13

Copilot AI reviewed Nov 20, 2025

View reviewed changes

Assem-Uber and others added 6 commits November 20, 2025 13:45

Apply suggestion from @Copilot

650a918

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

fix copilot comments

a9e9469

remove extra comment

8dcf01c

lint fix

573c195

change lastFlattented initial value to -1

7306663

Merge branch 'master' into feature/16026/fetcher-updates

e0a327a

macrotim reviewed Nov 20, 2025

View reviewed changes

move lastFlattenedPagesCount to a ref and remove shouldContinue condi…

73e9bd8

…tion

macrotim approved these changes Nov 20, 2025

View reviewed changes

Assem-Uber and others added 3 commits November 21, 2025 12:28

fix typo and add mock for configs

f8e080d

update page size test case

d9da99f

Merge branch 'master' into feature/16026/fetcher-updates

04ed13a

Assem-Uber merged commit a2af92e into cadence-workflow:master Nov 21, 2025
5 checks passed

Assem-Uber deleted the feature/16026/fetcher-updates branch November 21, 2025 12:45

feat: Fetcher updates #1090

feat: Fetcher updates #1090

Uh oh!

Conversation

Assem-Uber commented Nov 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Assem-Uber Nov 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Assem-Uber Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

macrotim Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Assem-Uber commented Nov 20, 2025 •

edited

Loading

Assem-Uber Nov 20, 2025 •

edited

Loading

Assem-Uber Nov 21, 2025 •

edited

Loading

macrotim Nov 21, 2025 •

edited

Loading