grafana / scenes

Build Grafana dashboards directly in your Grafana app plugins.
https://grafana.com/developers/scenes
Apache License 2.0
133 stars 20 forks source link

MultiValueVariable: Fix issue where url update would not take options into account #870

Closed mdvictor closed 1 month ago

mdvictor commented 1 month ago

This PR fixes an issue where in a variable with key-value pairs(esc), setting the key in the url param would not update to it's value but instead create a new custom option with the key as a value.

Before:

https://github.com/user-attachments/assets/cf3aca6a-cfc6-4c7f-825d-7f1bf1fc6ff3

After:

https://github.com/user-attachments/assets/575498fe-a385-43c0-a293-bd1d180e5d17

mdvictor commented 1 month ago

@torkelo As far as I can tell, we also load the options before setting the value from the URL in the old arch here, here

torkelo commented 1 month ago

@mdvictor not sure I understand the issue, looked again, I thought it was related to setting the correct key in the select after a full page reload, b

but see now that it's about setting the variable to the display value via URL. Both the new and old variable system sets the variable value via url using the value not the text representation

torkelo commented 1 month ago

wonder where/why they have urls with the text representation, grafana only generates URLs with the value

torkelo commented 1 month ago

@mdvictor so the problem is this if statement here: https://github.com/grafana/scenes/blob/main/packages/scenes/src/variables/variants/MultiValueVariable.ts#L196

that code there makes sure you can add a custom value (enter anything) in the select, that value get's set in the URL and if you reload the page we need to preserve that custom value even if it's not among the options returned by the query (or specified in the variable csv string).

We might be able to modify that logic a bit and ignore the if statement if current value === stateUpdate.text

like this

if (
      this.skipNextValidation &&
      stateUpdate.value !== this.state.value &&
      stateUpdate.text !== this.state.text &&
      !isAllValueFix
    ) {
      stateUpdate.value = this.state.value;
      stateUpdate.text = this.state.text;
    }
mdvictor commented 1 month ago

Closing this in favor of https://github.com/grafana/scenes/pull/874

mdvictor commented 1 month ago

wonder where/why they have urls with the text representation, grafana only generates URLs with the value

I don't know hey that happened either

@mdvictor so the problem is this if statement here: https://github.com/grafana/scenes/blob/main/packages/scenes/src/variables/variants/MultiValueVariable.ts#L196

that code there makes sure you can add a custom value (enter anything) in the select, that value get's set in the URL and if you reload the page we need to preserve that custom value even if it's not among the options returned by the query (or specified in the variable csv string).

We might be able to modify that logic a bit and ignore the if statement if current value === stateUpdate.text

like this

if (
      this.skipNextValidation &&
      stateUpdate.value !== this.state.value &&
      stateUpdate.text !== this.state.text &&
      !isAllValueFix
    ) {
      stateUpdate.value = this.state.value;
      stateUpdate.text = this.state.text;
    }

Thank you! Indeed that fixes the problem, I've opened a new PR with these changes and closed this one