Rebuild ITwinGrid data loading on a shared infinite query core

[LukaszKokot]

The useITwinData hook used five effects across two hooks to load one paginated list.
It now uses two.

The hook is rebuilt on a small reusable core, the idea being that we could rewrite parts of the IModel hooks the same way ; and the 37 existing tests in useITwinData.test.ts pass unchanged.

Solves AB#2141645

The new pieces

• infiniteQueryReducer: a pure state machine with no React import.
• useInfiniteQuery: drives that reducer with two effects: one starts the query, one keeps the pending request in flight behind an AbortController. When the query prop moves ahead of the state, it dispatches during render instead of from an effect.
• useEventCallback: one stable identity that always calls the latest function.
• iTwinsApi: not a hook. It is the only file that knows the iTwins REST API

Behaviour changes

• A shouldRefetchFavorites flip no longer restarts a query that still has more pages to load. That removes a double refetch and a duplicate-page append, but the favorites list no longer refreshes itself after a star toggle while more pages remain.
• fetchMore is undefined rather than defined-but-inert while an access token is required.
• No empty Cache-Control header is sent unless the favorites cache is bypassed.

How this was verified

• react-doctor went from 77/100 with 3 warnings to one and a score of 90/100; useITwinData.ts keeps one accepted no-pass-data-to-parent warning.
• Performance tests came first: Before any new code existed, a harness counted renders, effect-body executions, requests, aborted requests and report callbacks, and recorded thirteen scenarios plus the grid's commit counts against the old implementation. Afterwards: renders dropped in every query-change scenario. These tests were not kept.
• No existing test was rewritten and actually 2 new tests were added.
• End-to-end against Studio: The built package was packed as a tarball and a Studio workspace pointed at it through a pnpm override, then run twice. Two runs: one on the main branch still at 4.5.3, and another on the currently worked on branch that improves e2e tests and uses 4.6.0. Both ✅

[LukaszKokot]

@alexdunae @ben-polinsky what do you think? It's basically tanstack's useInfiniteQuery and a few other hooks, all tested to the bones, but we're not adding any new third party library. Also, iTwin API is extracted from the hook itself.
And everything is unit+e2e tested on studio.

[ben-polinsky]

but the favorites list no longer refreshes itself after a star toggle while more pages remain.

FYI I don't see this as a huge issue - in fact I kind of like it. What if I mistakenly toggle the incorrect star - I don't want the thing I toggled to instantly disappear - I have no chance to recover.

[ben-polinsky]

Before I get too deep into a review, the whole provider key thing is throwing me off. Isn't the "correct React way" to use useCallback on the getAccessToken function to preserve equality?

[LukaszKokot]

Before I get too deep into a review, the whole provider key thing is throwing me off. Isn't the "correct React way" to use useCallback on the getAccessToken function to preserve equality?

In _packages/modules/imodel-browser/src/types.ts:173 we see that the access token can either be a string or a function:

export type AccessTokenProvider = string | (() => Promise<string>)

And this is also where the "provider" name was taken from as you can see. The previous useTwinData was doing Authorization: typeof accessToken === "function" ? await accessToken() : accessToken, so basically the same thing we do here. The difference is that we're only storing a key that is either the access token or the "provider" string if it is a function, so the function is not stored... and we still have a useEventCallback later on when calling the fetch.

EDIT: forgot to mention that this refactor actually fixed a bug that causes re-renders and re-fetches.
EDIT 2: forgot to mention that in studio, we're always passing the access token as a string so we never hit that bug triggered by passing a function

[ben-polinsky]

And this is also where the "provider" name was taken from as you can see. The previous useTwinData was doing Authorization: typeof accessToken === "function" ? await accessToken() : accessToken, so basically the same thing we do here. The difference is that we're only storing a key that is either the access token or the "provider" string if it is a function, so the function is not stored... and we still have a useEventCallback later on when calling the fetch.

EDIT: forgot to mention that this refactor actually fixed a bug that causes re-renders and re-fetches. EDIT 2: forgot to mention that in studio, we're always passing the access token as a string so we never hit that bug triggered by passing a function

Thanks. I understand how credentialKey and useEventCallback work. My question is about the API contract and the reason for changing it.

<ITwinGrid /> currently says that a function token provider must be memoized. Studio passes a string, so it does not encounter this problem. Where does the reported bug occur?

If a caller passes a new inline function on each render, useCallback appears to fix the problem without treating every provider as the same credential source.

Is this PR intentionally removing the memoization requirement?

[LukaszKokot]

And this is also where the "provider" name was taken from as you can see. The previous useTwinData was doing Authorization: typeof accessToken === "function" ? await accessToken() : accessToken, so basically the same thing we do here. The difference is that we're only storing a key that is either the access token or the "provider" string if it is a function, so the function is not stored... and we still have a useEventCallback later on when calling the fetch.
EDIT: forgot to mention that this refactor actually fixed a bug that causes re-renders and re-fetches. EDIT 2: forgot to mention that in studio, we're always passing the access token as a string so we never hit that bug triggered by passing a function

Thanks. I understand how credentialKey and useEventCallback work. My question is about the API contract and the reason for changing it.

<ITwinGrid /> currently says that a function token provider must be memoized. Studio passes a string, so it does not encounter this problem. Where does the reported bug occur?

If a caller passes a new inline function on each render, useCallback appears to fix the problem without treating every provider as the same credential source.

Is this PR intentionally removing the memoization requirement?

Ah ! Sorry, no, my bad ! We are altering the contract indeed. Instead of relying on the developer to memo, I went a bit more defensive. I was TDD-ing this, and one of the tests was making sure we do not re-fetch and re-render uselessly... so basically not respecting the contract documented in the jsdoc.

So, wdyt, should I revert this, or change the jsdoc ? I'd favor changing the jsdoc, and explain it is not required anymore but that we are backward-compatible. But I can also understand if you'd want to revert this.

[ben-polinsky]

So, wdyt, should I revert this, or change the jsdoc ? I'd favor changing the jsdoc, and explain it is not required anymore but that we are backward-compatible. But I can also understand if you'd want to revert this.

No worries. As long as you're confident in the solution, documenting is fine.

[LukaszKokot]

So, wdyt, should I revert this, or change the jsdoc ? I'd favor changing the jsdoc, and explain it is not required anymore but that we are backward-compatible. But I can also understand if you'd want to revert this.

No worries. As long as you're confident in the solution, documenting is fine.

Alright, thanks very much, I'll update the documentation then.
I don't trust my (over-)confidence hence the number of unit tests that have been added recently 🫣

[ben-polinsky]

I am not sure if this is reproduce-able before your changes, but with wide windows and thus no scrollbar, new items are never populated. Here I used export const ITWINS_PAGE_SIZE = 5 to simulate

Edit: Seems to exist on main? If so, my bad - feel free to resolve - but we should open an issue for it.

[LukaszKokot]

I am not sure if this is reproduce-able before your changes, but with wide windows and thus no scrollbar, new items are never populated. Here I used export const ITWINS_PAGE_SIZE = 5 to simulate
Edit: Seems to exist on main? If so, my bad - feel free to resolve - but we should open an issue for it.

Yes, seems indeed to be a pre-existing bug, and it also seems to be affecting the MUI version as well. I did a quick investigation and the isFetching state from this PR, if we make it available, could help us fix the problem.

[alexdunae]

Kind of mind-bendy at first, but after spending some time with the PR it seems pretty solid.

While I'm still in favour of not adding deps like tanstack unless we really need to, it's wild how complicated paginated API fetches are in React.

[LukaszKokot]

While I'm still in favour of not adding deps like tanstack unless we really need to, it's wild how complicated paginated API fetches are in React.

I think React makes everything complicated 😬

[LukaszKokot]

Alright, I removed toCredentialKey and we now rely again on the client memoizing the access token provider, function or string. I've also updated comments, and applied suggestions, and restested everything pacakged again on studio. Looks like it's still working fine 🙏🏻
(and I also updated the PR description)

[alexdunae]

Makes sense to me and provides a nice foundation for future data fetching in here : )