-
Notifications
You must be signed in to change notification settings - Fork 33
fix: DH-21144: usePersistentState flaky with React 18 #2590
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2590 +/- ##
==========================================
+ Coverage 45.14% 45.29% +0.14%
==========================================
Files 769 771 +2
Lines 43511 43636 +125
Branches 11189 11041 -148
==========================================
+ Hits 19645 19765 +120
- Misses 23820 23855 +35
+ Partials 46 16 -30
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mofojed
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you provide some more details on the ticket about the issue and how it manifests?
I'm seeing something strange with this fix (did an npm start in web-client-ui, then ran with npm run start-community) against dev-gplus with dashboard DH-21144. I wonder if there's something else going on.
I'm adding the tables panel, and then also added a counter panel which just has a simple state. Added a couple of filters on each of the tables. However, sometimes the quick filter bar (which I'm leaving in the shown state) does not show by default when I refresh the page.
I also am seeing a lot of the following in the logs:
ItemUpdateListener.tsx:133 [ItemUpdateListener] Sending update {id: '4cff7bae-4865-40d5-99f2-8be9c594f1b5', date: '2025-12-11', owner: 'mbender', name: 'DH-21144', version: 8, …}
09:34:59.868 AppMainContainer.tsx:2005 [AppMainContainer] handleSharedDashboardUpdate {id: '4cff7bae-4865-40d5-99f2-8be9c594f1b5', date: '2025-12-11', owner: 'mbender', name: 'DH-21144', version: 8, …}
09:34:59.868 AppMainContainer.tsx:2068 [AppMainContainer] Metadata update needed false
09:34:59.869 AppMainContainer.tsx:2098 [AppMainContainer] Received unshared dashboard update from the same user in another browser session - ignore layout changes.
I don't have another browser tab open with another user. Seems like something may be off there?
Also, if i restart the query (from the Query Monitor in the same browser tab), it looks like the tables state resets (counter is fine though).
| if (persistentData.current.isTracking) { | ||
| persistentData.current.state = new Map(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmmm why is this in the useMemo hook? Don't like that there's a side effect executing in a useMemo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ya it feels gross to me too. The other option was a useEffect which triggers another render pass. That caused some unit tests to fail on the expected number of calls to onChange because it triggers an onChange and then collects the actual state and triggers again.
The bug seems to be related to batched state updates from what I can tell. So if I have 2 tables in 1 panel and filter/sort the 2nd table, it is doing something like this
- Schedule a persistent data update. This clears the map to collect the data
- Something changed in React 18 (batched updates I think) results in the 2nd table running its render function before the
updateVersionstate is committed in the provider - The 2nd table running its render function now calls
addStatewhich makes it the 1st state in the map - The
updateVersionstate is committed and rendered. At this point, the context value is supposed to change to force all subscribers to the context to run again so we can collect the data in render order; however, the 2nd table has already run once, so it just updates its value in the map. - The 1st table renders (by force) and adds its state to the map. It is now 2nd in the map.
- The map is saved w/ the data from the 2nd table as the 1st entry because it logged itself first.
These can be further out of order b/c there is also a table plugin state for each, but that is undefined. It just potentially messes with the order further.
So this fix is to push that map collection until after we know the version has changed.
I am leaning towards my idea w/ __dhId + type to persist the states. The limitation of 1 state type per panel/widget seems acceptable especially if it means less possibility of breaking from the order of render calls
|
The query restarting resetting the state is how it was behaving before (confirmed on dev-grizzly2). There's a state update triggered on unmount (because we can discard persisted state if you had a UITable that was removed for some reason). Seems reasonable we should try to fix that. It's because the state provider is still rendered just above the error view in dh.ui, so the disconnect unmounts all the components and it does another state collection, but there is no state to persist |
|
Ok summarizing the 3 issues noted
|
Tested with this code. Specifically changing the 2nd table usually caused the state inconsistency on reload.
Another fix which may be better is to use the
__dhIdfor tracking the states. This is usually justpanelId. We could use__dhId:stateTypewhich would be required for things like grid which has both aGridWidgetPluginandGridWidgetTablePluginStatein the same component.Pros
Cons
__dhIdto widget views). Might be hard to warn about this