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

SceneVariableSet: Do not propagate variable value changes when a local variable has the same name #729

Merged
merged 1 commit into from May 14, 2024

Conversation

torkelo
Copy link
Member

@torkelo torkelo commented May 9, 2024

Part of row repeat fixes grafana/grafana#87539

So we have this problem in row repeats that when the variable changes (that you repeat by). All the panel query runners inside all the repeated rows were notified that their variable dependency changed and started issuing a new queries (even when their local value did not change) .

So figured that this notification (via variableDependency) should not happen if there is a local SceneVariableSet with a local variable of the same name.

Created a test case for this scenario all is fine. This logic make sense, the variable has not changed for the local scope (as there is a local variable with the same name that has not changed).

The only really hard puzzle piece to solve is that his completely breaks the RowRepeatBehavior in core as it relies on this notification to process repeats.

So the problem here is that the first row (which we use as template / source of repeats) also get's a locally scoped value (the first variable value), so after the first repeat process this local value on the first row will block any updates the variable, which logically is correct the local value is not changing. ARHHHHfasFASDSDASFA S FQ"!° going to go for a walk.

Think the only solution is to the notification of variable changes from the behavior itself to the dashboard scene, this is now done in grafana/grafana#87539

@torkelo torkelo added minor Increment the minor version when merged release Create a release when this pr is merged labels May 9, 2024
Comment on lines +328 to +333
if (sceneObject.state.$variables && sceneObject.state.$variables !== this) {
const localVar = sceneObject.state.$variables.getByName(variable.state.name);
if (localVar) {
return;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

@torkelo wouldn't this only work for s single nested SceneVariableSet? I mean, shouldn't be continue up the hierarchy and make sure other objects also ignore updates in such scenario?

Copy link
Member

Choose a reason for hiding this comment

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

wait, sorry, nvm. We are traversing down in here to notify objects. So we exit early if there's a local var, so technically variables down the hierarchy will not be affected by the outer scope variable changes, as they now depend on the local variable which does not change. I was thinking about a scenario that the nested object would depend on the global variable, but that's not really possible, as the local variable scope will take precedence.

Copy link
Member Author

@torkelo torkelo May 10, 2024

Choose a reason for hiding this comment

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

@dprokop yea, there is maybe a scenario where the local variable set (that has the local variable) should be notified, but we can add that later if we need it

@dprokop
Copy link
Member

dprokop commented May 10, 2024

Think we need to wait with merging this and coordinate with grafana/grafana#87539 ?

@torkelo
Copy link
Member Author

torkelo commented May 10, 2024

@dprokop yes, good point. Will wait merging this until that one is approved and ready to merge

@torkelo torkelo merged commit 81046dc into main May 14, 2024
3 checks passed
@torkelo torkelo deleted the do-not-propagate-variable-value-changes branch May 14, 2024 06:41
@grafanabot
Copy link
Contributor

🚀 PR was released in v4.21.0 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor Increment the minor version when merged release Create a release when this pr is merged released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants