Skip to content
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

fix: DH-17292 Handle disconnect from GridWidgetPlugin #2086

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

mofojed
Copy link
Member

@mofojed mofojed commented Jun 20, 2024

  • When the model is disconnected, we should just display an error. There's no option to reconnect, as the widget schema could change entirely
  • By unloading the IrisGrid component, it's no longer throwing an error by trying to access table methods after a table.close()
  • Fixes DH-17292 from Enterprise
    • Tested using the steps in the description

- When the model is disconnected, we should just display an error. There's no option to reconnect, as the widget schema could change entirely
- By unloading the IrisGrid component, it's no longer throwing an error by trying to access table methods after a table.close()
- Fixes DH-17292 from Enterprise
  - Tested using the steps in the description
@mofojed mofojed requested a review from mattrunyon June 20, 2024 15:01
@mofojed mofojed self-assigned this Jun 20, 2024
Copy link
Collaborator

@mattrunyon mattrunyon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be fixed with ChartWidgetPlugin too? I'd guess yes since it has basically the same setup as GridWidgetPlugin

packages/dashboard-core-plugins/src/GridWidgetPlugin.tsx Outdated Show resolved Hide resolved
@mattrunyon
Copy link
Collaborator

And possibly PandasWidgetPlugin, but not sure if that model emits disconnects

@mattrunyon
Copy link
Collaborator

I checked w/ Figure() in DHE and it doesn't seem to have the same issue.

It does show a stale chart until the new widget is loaded though. That could be handled in a separate ticket

@mofojed
Copy link
Member Author

mofojed commented Jun 21, 2024

@mattrunyon Figure (and Table/Pandas Table) and everything in deephaven.ui should be displaying the error panel with https://github.com/deephaven-ent/iris/pull/1822 and the latest v0.15 deephaven.ui. So not too worried if there's a stale chart shown behind that error.
I'll add it to the PandasWidgetPlugin as well though so we don't throw errors.

- Minimize code duplication
- Added unit tests
@mofojed
Copy link
Member Author

mofojed commented Jun 21, 2024

Having a hard time debugging the Pandas thing because of a Pandas Enterprise issue: https://deephaven.atlassian.net/issues/DH-17081
I'll submit a fix for that first...

@mofojed
Copy link
Member Author

mofojed commented Jul 4, 2024

Now that we no longer show the widget after a disconnect/error (deephaven/deephaven-plugins#585) this PR is no longer relevant and will be closed.

@mofojed mofojed closed this Jul 4, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Jul 4, 2024
@mofojed mofojed reopened this Jul 4, 2024
@mofojed
Copy link
Member Author

mofojed commented Jul 4, 2024

Actually I realized there's some refactoring here (useIrisGridModel) which cleans some things up and could still be merged, so keeping it open.

@mofojed mofojed requested a review from mattrunyon July 4, 2024 13:01
- Add some more unit tests for transitions of states
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.

None yet

2 participants