search

elastic / kibana

Your window into the Elastic Stack
https://www.elastic.co/products/kibana
Other
19.35k stars 7.98k forks source link

[Console] disable autosave when loadFrom is used #181358

Closed TattdCodeMonkey closed 1 week ago

TattdCodeMonkey commented 3 weeks ago

Summary

Updated the editor to disable the autosave functionality when the editor is pre-loaded with content using the loadFrom query parameter. This is to ensure we do not overwrite a user's console content when they update code that was pre-loaded in the dev console.

This behavior was always possible, but is more likely to happen now as we use the Persistent Console with "Try in Console" buttons throughout Kibana.

If you're a developer who uses the console as a scratchpad opening the console with loadFrom and losing all your previous state is a very annoying user experience.

TattdCodeMonkey commented 3 weeks ago

@yuliacech If we decide its ok to merge this, I will create another PR to match this behavior in the monaco editor. That way this can still be easily back ported.

TattdCodeMonkey commented 2 weeks ago

@elasticmachine merge upstream

alisonelizabeth commented 1 week ago

@TattdCodeMonkey @yuliacech Apologies for the delay here, is this the expected flow? Just want to make sure I'm understanding:

  1. User has previous requests saved in Console
  2. User comes from loadFrom, Console displays pre-loaded content (previous content would no longer appear)
  3. User visits Console again. It shows content from (1)

Is it possible to prepend the loadFrom content? That way, both would be preserved?

TattdCodeMonkey commented 1 week ago

@alisonelizabeth Yes that is the expected flow.

Is it possible to prepend content? Yes, but that flow is a bit more complicated to implement and has more edge cases. I think one of the bigger ones is if you use loadFrom multiple times with the same code it will repeat in the content. This could be worse if you load a large snippet vs a small one since it could take up the viewable content.

But I'm not opposed to that over this change which makes the content when using loadFrom more ephemeral. If we do prefer that behavior over this change I would say we plan that for 8.15 and likely don't try to backport the change in that case.

alisonelizabeth commented 1 week ago

Yes, but that flow is a bit more complicated to implement and has more edge cases. I think one of the bigger ones is if you use loadFrom multiple times with the same code it will repeat in the content.

Yeah, that's true.

But I'm not opposed to that over this change which makes the content when using loadFrom more ephemeral. If we do prefer that behavior over this change I would say we plan that for 8.15 and likely don't try to backport the change in that case.

tbh I don't know what the best behavior is here given the current UI. This PR might be a good incremental improvement. That said, I don't think this is a candidate to backport for 8.14 given we only have 1 BC left. Do you want to join our team sync this week and we can discuss further?

yuliacech commented 1 week ago

I remembered that we have the append logic for loading snippets from ES docs, so whenever loadFrom parameter has a value like http://localhost:5601/zzz/app/kibana#/dev_tools/console?load_from=https://www.elastic.co/guide/en/elasticsearch/reference/current/snippets/2501.console.

TattdCodeMonkey commented 1 week ago

/ci

kibana-ci commented 1 week ago

:green_heart: 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
console 443.3KB 443.3KB +29.0B

History

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