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

[Graph] Fix guidance panel appearing for a moment when saving Graph #141228

Merged
merged 3 commits into from
Sep 22, 2022

Conversation

davismcphee
Copy link
Contributor

@davismcphee davismcphee commented Sep 21, 2022

Summary

This PR fixes an issue where the Graph workspace becomes temporarily uninitialized when saving a copy of an existing workspace due to deleting the savedWorkspace.id in these lines in saved_workspace_utils:

// Save the original id in case the save fails.
const originalId = savedObject.id;
// Read https://github.com/elastic/kibana/issues/9056 and
// https://github.com/elastic/kibana/issues/9012 for some background into why this copyOnSave variable
// exists.
// The goal is to move towards a better rename flow, but since our users have been conditioned
// to expect a 'save as' flow during a rename, we are keeping the logic the same until a better
// UI/UX can be worked out.
if (savedObject.copyOnSave) {
delete savedObject.id;
}

The issue is usually hidden since savedWorkspace is a ref, so normally no renders are triggered when it's modified, but the graph calls setRenderCounter in workspace_route on each frame while initializing which triggers a bunch of renders and reveals the bug (add console.log('renderCounter', renderCounter); to workspace_route to see).

Resolves #110238.

Checklist

For maintainers

@davismcphee davismcphee added bug Fixes for quality problems that affect the customer experience Feature:Graph Graph application feature Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. labels Sep 21, 2022
@davismcphee davismcphee self-assigned this Sep 21, 2022
@davismcphee davismcphee marked this pull request as ready for review September 21, 2022 14:48
@davismcphee davismcphee requested a review from a team as a code owner September 21, 2022 14:48
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-graph (Feature:Graph)

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
graph 450.3KB 450.3KB +12.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @davismcphee

Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

Code LGTM 👍 , tested locally, and the guidance panel was no longer flickering when saving as new. Hope it's not sad. But we're not 😄 ! Thank you, nice fix!

@davismcphee davismcphee merged commit b026d2c into elastic:main Sep 22, 2022
@davismcphee davismcphee deleted the fix-graph-save-as-copy-bug branch September 22, 2022 13:22
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Sep 22, 2022
…lastic#141228)

* [Discover] Fix issue where workspace switches to uninitialized briefly when saving as copy in Graph

* [Graph] Add Jest tests for Graph workspace saving bug

(cherry picked from commit b026d2c)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.5

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

@davismcphee
Copy link
Contributor Author

💚 All backports created successfully

Status Branch Result
8.5

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

davismcphee added a commit to davismcphee/kibana that referenced this pull request Sep 22, 2022
…lastic#141228)

* [Discover] Fix issue where workspace switches to uninitialized briefly when saving as copy in Graph

* [Graph] Add Jest tests for Graph workspace saving bug

(cherry picked from commit b026d2c)
davismcphee added a commit that referenced this pull request Sep 22, 2022
…141228) (#141435)

* [Discover] Fix issue where workspace switches to uninitialized briefly when saving as copy in Graph

* [Graph] Add Jest tests for Graph workspace saving bug

(cherry picked from commit b026d2c)
jloleysens added a commit to jloleysens/kibana that referenced this pull request Sep 22, 2022
* main: (33 commits)
  Fix Next and Previous button on step screenshot carousel. (elastic#141422)
  [journeys] add a short delay after loading data (elastic#141437)
  skip failing test suite (elastic#140797)
  skip failing test suite (elastic#138776)
  Documents saved objects bulk delete API (elastic#141164)
  [Response Ops][Alerting] Defining default action params on connector type (elastic#141226)
  [ML] Explain Log Rate Spikes: Adds jest tests for query_utils/buildBaseFilterCriteria. (elastic#141213)
  [APM] Fix search bar suggestions (elastic#141101)
  close popover on click (elastic#141272)
  [Fleet] Update to use savedObjects bulkDelete for package policy delete (elastic#141276)
  [Security Solution][Endpoint][Response Actions] Do fuzzy search on given usernames for Actions Log (elastic#141239)
  [Graph] Fix guidance panel appearing for a moment when saving Graph (elastic#141228)
  [Fleet] Add experimental data stream features support to simplified package policy API (elastic#141288)
  Shameless copy of the retryTransientEsErrors from fleet (elastic#141246)
  [Security Solution][Fix]-Issue with disabled dataProvider (elastic#140735)
  Changing triggers actions ui routes to internal (elastic#141149)
  skip flaky test suit elastic#141356
  [SharedUX] Removing TODOs from KibanaPageTemplate (elastic#141043)
  [dashboard controls] skip failing test on cloud (elastic#141291)
  [Synthetics] unskip edit_monitor api integration tests (elastic#141277)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Feature:Graph Graph application feature release_note:fix Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. v8.5.0 v8.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Graph] Guidance panel appearing for a moment when saving graph
5 participants