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

[BUG] Navigation between dashboards requires browser refresh after update to 2.9.0 #4694

Closed
juergen-walter opened this issue Aug 8, 2023 · 24 comments · Fixed by #5435
Closed
Assignees
Labels
bug Something isn't working de-angular de-angularize work v2.13.0

Comments

@juergen-walter
Copy link

Describe the bug

When navigating between dashboards, dashboard contents do not load on initial request (accompanied by Javascript errors).
Refresh button in OSD UI does not help, only browser refresh.

To Reproduce
Steps to reproduce the behavior:

  1. Create two dashboards with a data table visualization
  2. Open web console in browser
  3. Open one dashboard
  4. Navigate to the second dashboard directly, which can be done by entering the URL in the browser (which mimics a hyper link between the dashboards)
  5. See error in web console
Detected an unhandled Promise rejection.
Error: Embeddable has been destroyed

embeddable.plugin.js:6 Uncaught (in promise) Error: Embeddable has been destroyed
    at VisualizeEmbeddable.render (embeddable.plugin.js:6:122906)
    at VisualizeEmbeddable._callee7$ (visualizations.plugin.js:6:193751)
    at tryCatch (queryWorkbenchDashboards.plugin.js:2:2179)
    at Generator._invoke (queryWorkbenchDashboards.plugin.js:2:1802)
    at Generator.next (queryWorkbenchDashboards.plugin.js:2:2954)
    at visualize_embeddable_asyncGeneratorStep (visualizations.plugin.js:6:176426)
    at _next (visualizations.plugin.js:6:176757)
    at visualizations.plugin.js:6:176949
    at new Promise (<anonymous>)
    at VisualizeEmbeddable.<anonymous> (visualizations.plugin.js:6:176669)
  1. Do a browser refresh --> Dashboard contents load, Javascript error disappears

Expected behavior
Dashboard should load on first attempt. Should not require a reload in the browser

OpenSearch Version
2.9.0

Dashboards Version
2.9.0

Plugins

Default

Screenshots
image

Host/Environment (please complete the following information):

  • Chrome, Firefox

Additional context

Update from 2.8.0 to 2.9.0. Function at 2.8.0 was okay.

@mmguero
Copy link

mmguero commented Aug 14, 2023

I am seeing this as well.

mmguero added a commit to mmguero-dev/Malcolm that referenced this issue Aug 14, 2023
@JustinHendersonSMAPPER
Copy link

This is affecting all of our dashboards as well. I've tested multiple variations on links and nothing works except refreshing the page.

@kavilla
Copy link
Member

kavilla commented Aug 14, 2023

Looking

@kavilla
Copy link
Member

kavilla commented Sep 1, 2023

Thanks @juergen-walter! This help recreated it.

Part of the deangularization of the dashboards plugin for security reasons attempted to keep intact certain sections of the code to avoid changes. Previously, no matter what the route would hard refresh there was a wrapper around the angular part of the application.

Unfortunately, that means the re-rendering from one state of dashboards through the URL causes the current app Id to immediately change in the url which is what we are keying off to determine when to set the current dashboard app state. The error being thrown is expected as async processes are trying to modify a component that has been destroyed already but it would have likely not haven gotten here if the dashboard id in the url wasn't updated already.

Still looking into best next action.

@thmsbp
Copy link

thmsbp commented Sep 29, 2023

This issue #4694 (and associated #4819) prevents us from migrating our Product, because we built a markdown menu to navigate from a dashboard to another with a defined logic (themes, contextual dashboards). More importantly, we also use a dashboard link mechanism in TSVB visualizations to load a specific dashboard filtered on a host. In one word, navigation is fully broken in our Product due to this issue.

Waiting for a solution, do you have any recommendations to workaround the issue, or another method to

  • build a dashboard menu
  • load a filtered dashboard from a TSVB link

@juergen-walter
Copy link
Author

The issue might be fixed by: opensearch-project/alerting-dashboards-plugin#682
2.10.0 includes the fix alerting-dashboards-plugin/compare/2.9.0.0...2.10.0.0

Will provide update once tested. Providing this update that others can start to try as well.

@mmguero
Copy link

mmguero commented Oct 2, 2023

It seems to still be broken for me with 2.10.0.

mmguero added a commit to mmguero-dev/Malcolm that referenced this issue Oct 2, 2023
mmguero added a commit to mmguero-dev/Malcolm that referenced this issue Oct 2, 2023
@juergen-walter
Copy link
Author

TLDR: Issue is still not resolved with 2.10.0 @kavilla any updates? This is a blocker for us!

Sorry for the confusion with #4694 (comment) I should have checked #4694 (comment) +opensearch-project/alerting-dashboards-plugin#682 more carefully.

Error message slightly changes to

embeddable.plugin.js:6 Uncaught (in promise) Error: Embeddable has been destroyed
    at visualize_embeddable_VisualizeEmbeddable.render (embeddable.plugin.js:6:40504)
    at visualize_embeddable_VisualizeEmbeddable.render (visualizations.plugin.js:6:84844)
    at embeddable_panel_EmbeddablePanel.componentDidMount (embeddable.plugin.js:6:92308)
    at os (osd-ui-shared-deps.js:441:83372)
    at fc (osd-ui-shared-deps.js:441:101246)
    at t.unstable_runWithPriority (osd-ui-shared-deps.js:449:3844)
    at ji (osd-ui-shared-deps.js:441:45024)
    at dc (osd-ui-shared-deps.js:441:97718)
    at Qs (osd-ui-shared-deps.js:441:93872)
    at osd-ui-shared-deps.js:441:45315

How I checked (to speed up the check for later OS versions):
Download docker compose file from
https://opensearch.org/docs/2.10/install-and-configure/install-opensearch/docker/#sample-docker-compose-file-for-development

docker-compose up
curl -XDELETE http://localhost:9200/.kibana_1
docker restart opensearch-dashboards

@mmguero
Copy link

mmguero commented Oct 5, 2023

I'm in the same boat, stuck on v2.8.0 until this is resolved.

@kavilla
Copy link
Member

kavilla commented Oct 12, 2023

I haven't been able to get to this yet. But I think we have someone assigned to resolving it. @BigSamu

I believe the issue occurs here: https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/src/plugins/dashboard/public/application/utils/use/use_saved_dashboard_instance.ts#L161.

The ID is getting set prior to this is getting triggered and then it thinks it has already rendered components even though it is currently destroying those previous components. It went uncaught since there is nothing explicitly covering direct navigation from dashboard to dashboard and it worked previously because it would always hard refresh when going to a new dashboard (due to the angular wrapper). I think in this event, a quick and dirty solution we can maybe check if the history if previous route was dashboard or maybe if wrapping it not to destroy those elements because those elements are already destroyed.

cc: @abbyhu2000

@abbyhu2000 abbyhu2000 self-assigned this Oct 13, 2023
@mmguero
Copy link

mmguero commented Oct 23, 2023

So I assume this is still not fixed in the v2.11.0 release?

@BigSamu
Copy link
Contributor

BigSamu commented Oct 23, 2023

I haven't been able to get to this yet. But I think we have someone assigned to resolving it. @BigSamu

I believe the issue occurs here: https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/src/plugins/dashboard/public/application/utils/use/use_saved_dashboard_instance.ts#L161.

The ID is getting set prior to this is getting triggered and then it thinks it has already rendered components even though it is currently destroying those previous components. It went uncaught since there is nothing explicitly covering direct navigation from dashboard to dashboard and it worked previously because it would always hard refresh when going to a new dashboard (due to the angular wrapper). I think in this event, a quick and dirty solution we can maybe check if the history if previous route was dashboard or maybe if wrapping it not to destroy those elements because those elements are already destroyed.

cc: @abbyhu2000

Thanks, @kavilla, yes I will be taking a look to this issue. Just give me some time because I am currently solving other ones that will take me a bit of time.

Regards,

Samuel

@abbyhu2000
Copy link
Member

abbyhu2000 commented Oct 31, 2023

I am currently investigating the issue. I think the issue occurs in src/plugins/dashboard/public/application/embeddable/grid/dashboard_grid.tsx.

When we are using the dashboard grid component to render all the panels/child embeddables, it is using this.state.panels. However, this.state.panels values do not get updated with the correct panels info when we switch from one dashboard to another dashboard.

One of the props that is passing into the dashboard_grid, dashboardContainer, does contain the correct updated panels info. Ideally this.state.panels should be listened to dashboardContainer.panels, and it should get updated whenever there is a change. However, this.state.panels get updated in componentDidMount, but that line does not get triggered when switching from one dashboard to another dashboard.

It seems like we need a mechanism that is listening on this React component's prop and then set the state to the updated changes to solve this problem.

@abbyhu2000
Copy link
Member

abbyhu2000 commented Oct 31, 2023

Another bug that might be causing the issue is that we never call dashboardContainer.destroy() -- which destroy all its child embeddables. Still investigating.

@BigSamu
Copy link
Contributor

BigSamu commented Nov 1, 2023

@abbyhu2000, do you want to work together on this issue? Should I work on your fork?

@abbyhu2000
Copy link
Member

abbyhu2000 commented Nov 3, 2023

@abbyhu2000, do you want to work together on this issue? Should I work on your fork?

Hi @BigSamu, Yes we can work together on this issue. I think you don't have to work on my fork, and you can work on your own version and start diving deep. My current progress is i found out that in the src/plugins/dashboard/public/application/embeddable/grid/dashboard_grid.tsx, the state(this.state.panels) need to be updated by the prop changes(this.prop.container.getInput().panels). But if even i tried to add component update functions like shouldComponentUpdate()and componentWillReceiveProps(), however, the panels will still render a couple of visualizations from the previous panel on top of the current dashboard panels. It seems like there is some racing conditions with the rendering. I am going to keep investigating and finding a solution.

Let me know if that helps and please comment on any questions. Please do not hesitate to dive deep on this, and comment down any new findings you found.

@BigSamu
Copy link
Contributor

BigSamu commented Nov 6, 2023

Hi @abbyhu2000, perfect. Planning to start working on this issue maybe next week. Currently in parallel with other issues that hope I have finished this week.

abbyhu2000 added a commit that referenced this issue Nov 7, 2023
Previously navigating from one dashboard to another dashboard do not work. This PR adds a dashboard id as a prop to pass into the <DashboardViewPort> component in order to force react to re-render every time it receives a new id. 

Resolve the following issues:
* #4694 
* https://github.com/opensearch-project/Open and Search-Dashboards/issues/4819

Signed-off-by: abbyhu2000 <[email protected]>
opensearch-trigger-bot bot pushed a commit that referenced this issue Nov 7, 2023
Previously navigating from one dashboard to another dashboard do not work. This PR adds a dashboard id as a prop to pass into the <DashboardViewPort> component in order to force react to re-render every time it receives a new id.

Resolve the following issues:
* #4694
* https://github.com/opensearch-project/Open and Search-Dashboards/issues/4819

Signed-off-by: abbyhu2000 <[email protected]>
(cherry picked from commit 1de8be6)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md
opensearch-trigger-bot bot pushed a commit that referenced this issue Nov 7, 2023
Previously navigating from one dashboard to another dashboard do not work. This PR adds a dashboard id as a prop to pass into the <DashboardViewPort> component in order to force react to re-render every time it receives a new id.

Resolve the following issues:
* #4694
* https://github.com/opensearch-project/Open and Search-Dashboards/issues/4819

Signed-off-by: abbyhu2000 <[email protected]>
(cherry picked from commit 1de8be6)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md
@juergen-walter
Copy link
Author

Looks like it has been closed with #5435 Have no time to test it the near future.

Relevant labels
v2.11.1
backport 2.x
backport 2.11

@abbyhu2000
Copy link
Member

Hi @abbyhu2000, perfect. Planning to start working on this issue maybe next week. Currently in parallel with other issues that hope I have finished this week.

Hi @BigSamu, i just had a PR out for fixing this bug. Thank you for your participation on this issue!

opensearch-trigger-bot bot pushed a commit that referenced this issue Nov 7, 2023
Previously navigating from one dashboard to another dashboard do not work. This PR adds a dashboard id as a prop to pass into the <DashboardViewPort> component in order to force react to re-render every time it receives a new id.

Resolve the following issues:
* #4694
* https://github.com/opensearch-project/Open and Search-Dashboards/issues/4819

Signed-off-by: abbyhu2000 <[email protected]>
(cherry picked from commit 1de8be6)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md
@BigSamu
Copy link
Contributor

BigSamu commented Nov 8, 2023

No problem @abbyhu2000!

abbyhu2000 pushed a commit that referenced this issue Nov 9, 2023
Previously navigating from one dashboard to another dashboard do not work. This PR adds a dashboard id as a prop to pass into the <DashboardViewPort> component in order to force react to re-render every time it receives a new id.

Resolve the following issues:
* #4694
* https://github.com/opensearch-project/Open and Search-Dashboards/issues/4819

Signed-off-by: abbyhu2000 <[email protected]>
(cherry picked from commit 1de8be6)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
abbyhu2000 pushed a commit that referenced this issue Nov 9, 2023
Previously navigating from one dashboard to another dashboard do not work. This PR adds a dashboard id as a prop to pass into the <DashboardViewPort> component in order to force react to re-render every time it receives a new id.

Resolve the following issues:
* #4694
* https://github.com/opensearch-project/Open and Search-Dashboards/issues/4819

Signed-off-by: abbyhu2000 <[email protected]>
(cherry picked from commit 1de8be6)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
AMoo-Miki added a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this issue Nov 15, 2023
Previously navigating from one dashboard to another dashboard do not work. This PR adds a dashboard id as a prop to pass into the <DashboardViewPort> component in order to force react to re-render every time it receives a new id.

Resolve the following issues:
* opensearch-project#4694
* https://github.com/opensearch-project/Open and Search-Dashboards/issues/4819

Signed-off-by: abbyhu2000 <[email protected]>

(cherry picked from commit 1de8be6)
Signed-off-by: Miki <[email protected]>
@juliocabrerac
Copy link

By testing different ways of putting the link, I managed to make it work by putting http, that is, without the s in https. I had the problem with AWS OpenSearch 2.11.
I hope that it would be useful to someone.
Greetings

@AMoo-Miki
Copy link
Collaborator

Reopening as this doesn't appear to have been fixed fully.

@kavilla
Copy link
Member

kavilla commented Feb 14, 2024

@juliocabrerac can you offer more details? I'm unable to recreate it but it could be do the cache busting mechanism. Do you know you initially created/upgraded to 2.11?

@mmguero
Copy link

mmguero commented Feb 14, 2024

fwiw it was fixed for me in v2.11.1, I no longer have the problem

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working de-angular de-angularize work v2.13.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants