Skip to content

Conversation

@mattrunyon
Copy link
Collaborator

@mattrunyon mattrunyon commented Dec 10, 2025

Tested with this code. Specifically changing the 2nd table usually caused the state inconsistency on reload.

from deephaven import ui
import deephaven.plot.express as dx

_stocks = dx.data.stocks(False)


@ui.component
def my_comp():
    key, set_key = ui.use_state(0)
    return [
        ui.fragment(_stocks, key="base" + str(key)),
        ui.button("Reset", on_press=lambda: set_key(key + 1)),
        ui.table(_stocks, key="table" + str(key)),
    ]


tables = my_comp()

Another fix which may be better is to use the __dhId for tracking the states. This is usually just panelId. We could use __dhId:stateType which would be required for things like grid which has both a GridWidgetPlugin and GridWidgetTablePluginState in the same component.

Pros

  • Indifferent to React render internals
  • Could allow a flag to remove on unmount or not for panels like Schema Editor which has a console creator page and then a separate view rendered after creation. Could just persist state in the creator and say not to remove on unmount instead of needing to hoist that state to the parent panel component
  • Should be more efficient as we wouldn't need to re-render consumers on every persistent state update. Not currently a performance issue I've seen though

Cons

  • Only 1 state type per panel (or widget in dh.ui which assigns its own __dhId to widget views). Might be hard to warn about this
  • Might need to add some migration either picked back to Grizzly or just in G+ since we added persistent state for dh.ui widgets in Grizzly. I think migration in G+ would be fine as the React render will be in the same order still (and we can detect a serialized map vs array w/ single elements), it's just batching on updates that was causing the issue.

@mattrunyon mattrunyon requested a review from mofojed December 10, 2025 20:14
@mattrunyon mattrunyon self-assigned this Dec 10, 2025
@codecov
Copy link

codecov bot commented Dec 10, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 45.29%. Comparing base (089eabd) to head (2d5ed4a).
⚠️ Report is 21 commits behind head on main.

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     
Flag Coverage Δ
unit 45.29% <100.00%> (+0.14%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Member

@mofojed mofojed left a 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).

Comment on lines 141 to 143
if (persistentData.current.isTracking) {
persistentData.current.state = new Map();
}
Copy link
Member

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.

Copy link
Collaborator Author

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

  1. Schedule a persistent data update. This clears the map to collect the data
  2. Something changed in React 18 (batched updates I think) results in the 2nd table running its render function before the updateVersion state is committed in the provider
  3. The 2nd table running its render function now calls addState which makes it the 1st state in the map
  4. The updateVersion state 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.
  5. The 1st table renders (by force) and adds its state to the map. It is now 2nd in the map.
  6. 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

@mattrunyon
Copy link
Collaborator Author

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

@mattrunyon
Copy link
Collaborator Author

mattrunyon commented Dec 11, 2025

Ok summarizing the 3 issues noted

  1. (not new) AppMainContainer logs are present on dev-grizzly
  2. (not new) Losing state on PQ restart is present on dev-grizzly
  3. (new) Losing filter bar shown state is not present on dev-grizzly. Likely a React 18 issue in UITable with how it's rehydrating state (and combining saved state + server props). I'm guessing there's something off here causing it to think it's already been hydrated as the state is saved properly, but the server prop (which is false by default) is taking priority

mofojed
mofojed previously approved these changes Jan 2, 2026
@mattrunyon mattrunyon merged commit 656c090 into deephaven:main Jan 5, 2026
11 checks passed
@mattrunyon mattrunyon deleted the DH-21144 branch January 5, 2026 15:53
@github-actions github-actions bot locked and limited conversation to collaborators Jan 5, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants