Add onDataStateChange to ITwinGrid - #231
Merged
Merged
Conversation
The status, the iTwins and the has-more flag were three useState calls set from the same async fetch callback. Nothing guarantees they land in one render, so a render could show Complete beside the previous query's iTwins. Initial status is now fetching rather than undefined, which is what postProcessCallback receives as its second argument on the first render.
The grid's fetching state only escaped through postProcessCallback, a hook for transforming the iTwin array that consumers were reading a status out of. It runs during render, so a consumer had to work out which request a status belonged to from its own props. The new prop reports from an effect, and carries the query it belongs to. It also carries hasMore, since complete means the page landed rather than all the data being present, and the error behind error_fetchFailed, which until now only reached the logger. The query names the subclass as well as the request type, filter text and ordering, so that switching subclass is not mistaken for a refresh of the previous one. Partially solves AB#2114177
onDataStateChange matches the argTypesRegex in preview.tsx, so Storybook infers an action spy for it in every story that does not declare one. The grid calls the prop while it renders, which an inferred spy throws on, taking down every story in both files.
Logs each report with the time its query took to settle, and sends the report itself to the Actions panel. Searching the favorites or recents tab shows the case that is otherwise hard to see: a result reported with no fetch before it, answered from the iTwins already loaded.
Follows createFetchIModelsFn in useIModelData, which already owns URL assembly and the fetch for the iModel grid. Takes an options object rather than that one's ten positional parameters, and reads the query descriptor directly, since the request type, filter text, subclass and ordering are exactly what the URL needs. The effect keeps everything stateful: the superseded-request guard, the totalCount and pagination writes, the favorites reset, and the abort on cleanup. A totalCount of undefined now means the response carried no count. Returning Number(null) instead would have reported zero.
The state became one object so that a render could never show one query's status beside another query's iTwins, but it was still written from seven scattered setFetchState calls, four of them inside the fetch effect. The invariant held by convention. Each write is now a named transition and setFetchState is referenced nowhere else. Every transition takes what it needs as an argument so it can carry empty dependencies and sit in the effect dependency arrays without re-running them. Three things stay with the caller on purpose: the logging, since a transition that logs would need the logger in its dependencies; the totalCount, pagination and favorites writes, so their order relative to the state write is unchanged; and the page-zero check ahead of markFetching, which is pagination rather than state.
useITwinData now derives the query, decides what to request and when, and drives the transitions. What the grid's data is, and what a report says about it, lives next door. Client side filtering moves with it, because the filtered list is what a report carries rather than something the fetch needs. The hook takes only the query: filterText is filterOptions ?? "", and useITwinFilter lowercases both to the same empty string. The ref that reset reads is now declared inside the hook, which keeps it ahead of the two reset effects that call reset, since a hook's effects are queued where the hook is called.
searchParams.get returns null rather than an empty string, so ?? says what is meant and clears the lint warning.
The two declared the same five fields, so a field added to the report had to be added twice. FetchState now takes them from ITwinDataState. They stay separate types because the iTwins differ: the state holds every page fetched, a report carries only what client side filtering kept. Collapsing them would leave the variable name as the only thing saying which is which, and reporting the state directly would then typecheck.
The payload belongs in the prop's own documentation, not the changelog.
LukaszKokot
commented
Aug 18, 2026
It argued for where the code sits rather than saying anything the signature does not.
LukaszKokot
commented
Aug 18, 2026
LukaszKokot
marked this pull request as ready for review
August 18, 2026 22:08
LukaszKokot
requested review from
alexdunae,
aruniverse,
ben-polinsky,
kckst8 and
toddsouthenbentley
as code owners
August 18, 2026 22:08
LukaszKokot
force-pushed
the
lk/itwin-grid-data-state
branch
from
August 19, 2026 19:21
e75fa51 to
db65f8b
Compare
Contributor
Author
|
@alexdunae @ben-polinsky could you please take a look ? I've tested this locally on my Studio branch targeting this local branch as override. I can confirm it works fine and we can remove a bunch of code from there, including defensive assertions. |
alexdunae
reviewed
Aug 24, 2026
alexdunae
reviewed
Aug 24, 2026
Backs out the one part of this branch that changed behaviour for consumers who never pass the new prop, since postProcessCallback receives the status as its second argument. Only the initial state is undefined. startingOver keeps fetching, because it also runs on every reset and those went to fetching before. The report skips a state with no status, so the new callback still never reports one.
The report effect depends on the query, so a new object every render reports the same state again on every render.
The data override and the missing token both settle in the same effect flush as the reset, so no fetching render commits and those queries report once, not twice. The change file also left out the MUI grid.
alexdunae
approved these changes
Aug 25, 2026
Contributor
Author
|
Hey @ben-polinsky , could you please take a look if you have some spare time 🙇🏻🙏🏻 ? It's the one brick missing for https://github.com/iTwin/studio/pull/4089 in Studio for -hopefully- reducing one of the main sources of flakiness 🙏🏻 |
ben-polinsky
approved these changes
Sep 1, 2026
ben-polinsky
left a comment
ben-polinsky
left a comment
Collaborator
There was a problem hiding this comment.
Is this part of the PR description correct? It says:
status is fetching rather than undefined on the first render
The final code does the opposite on purpose? nothingDecidedYet() preserves undefined, and the new test confirms this.
Contributor
Author
ah yes, after Alex' suggestion, I switched it to undefined (for good reasons) and forgot to update the PR description. I'll do it and address the nit, and we're good to go. Thanks for the review 🙇🏻♂️ |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Partially solves AB#2114177
Why
Consumers need to know when the grid has finished answering a query. The only way that state left the grid was with
postProcessCallback. And it runs during render and never says which request a status belongs to, so a consumer (like Studio) reconstructs that from its own props.onDataStateChangereports from an effect and carries the query it belongs to, plushasMore, sincecompletemeans the page landed rather than all the data being present, and the error, which until now only reached the logger.Why it is split this way
The hook state was three
useStatecalls set from one async callback, so a render could showcompletebeside the previous query's iTwins. Merging them into one object is what makes a report worth trusting.It was then still written from seven scattered
setFetchStatecalls, four of them inside the fetch effect. From each write, we created and extracted a properly named function.The rest is placement. The state and the page request have their own homes now, the latter following
createFetchIModelsFninuseIModelData.Testing
No problem to report, no retrying steps, and a much cleaner way to infer state.
For reviewers
I suggest looking at individual commits to see how/what went into the refactor ; it's easier to follow.