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

AdHocFiltersVariable: Fixes issue updating hide state causing variable to be deactivated and preventing it from being shown again #679

Merged
merged 3 commits into from
Apr 9, 2024

Conversation

torkelo
Copy link
Member

@torkelo torkelo commented Apr 4, 2024

While troubleshooting the problem of clearing adhoc filters and setting hide: VariableHide.hideVariable and then adding filters and setting variable hide back to Variable.dontHide I discovered some fundamental issues with useState and AdHocFiltersVariable

Problem with useState.

  • calling the useState hook from a component does not actually guarantee state updates as useState does not activate the source scene object. So what this means is that useState only works if there is something else keeping the object active (like the scene objects own Component). The VariableValueSelectWrapper uses variable.useState but when the variable was hidden it was deactivated (since it was only it's component that counted in the refCount).

Solution:

  • useState should also count as "refCount" activation and keep the source scene object alive.

Problem with AdHocFiltersVariable

  • It updated filterExpression inside a subscribeToState (subscribing to it's own state). This could then trigger another state update if the expression had changed. But this can cause a problem as the state update that updates filterExpression happens inside the subscribeToState callback the order of state updates could be mangled for other objects subscribing to the variable state.

Example.

  1. setState (update filters) (state update A)
  2. State change handler 1 triggers for update A (this is its own handler), it causes another setState with updated filterExpression (state update B)
  3. State change handler 1 triggers for update B (does nothing as filters has not changed only filterExpression)
  4. State change handler 2 triggers for update B (for some other component that subscribes to state)
  5. State change handler 2 triggers for update A (for some other component that subscribes to state)

As you can see the other component subscribing to the variable state will get the first state change (with old filterExpression) as the last value. Could be fixed with updating filterExpresion inside a setTimeout but opted to update filterExpression inside an overridden setState function as this eliminates two state updates when updating filters.

Release notes

<SceneObject>.useState will now cause the scene object to be activated (if it was not already). When the component that calls <sceneObject>.useState unmounts it will deactivate the scene object if it was the last component to call useState on this instance.

The same is not true for <SceneObject>.subscribeToState, this function still does not cause the source object to be activated.

📦 Published PR as canary version: 4.4.2--canary.679.8556229462.0

✨ Test out this PR locally via:

npm install @grafana/scenes@4.4.2--canary.679.8556229462.0
# or 
yarn add @grafana/scenes@4.4.2--canary.679.8556229462.0

@torkelo torkelo changed the title AdHocFiltersVariable: Fixes issue updating hide state causing variable to be deactivated and prevent it from being shown again AdHocFiltersVariable: Fixes issue updating hide state causing variable to be deactivated and preventing it from being shown again Apr 4, 2024
@torkelo torkelo added the release Create a release when this pr is merged label Apr 4, 2024
Copy link
Member

@dprokop dprokop left a comment

Choose a reason for hiding this comment

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

@torkelo excellent finding! It's the type of thing that's very tricky to figure out unless a buggy use case appears. I think it totally makes sense to activate with useState usage.

@torkelo torkelo merged commit 8e99673 into main Apr 9, 2024
3 checks passed
@torkelo torkelo deleted the useState-activate branch April 9, 2024 11:21
@grafanabot
Copy link
Contributor

🚀 PR was released in v4.5.5 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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