Explore: Be sure time range key bindings are mounted after clear #61892
Conversation
gelicia
requested review from
ashharrison90 and
yaelleC
and removed request for
a team
gelicia
added
no-backport
|
Backend code coverage report for PR #61892 |
|
Frontend code coverage report for PR #61892
|
…na/keybinding-bugs
There was a problem hiding this comment.
Works as expected!
I left a comment to consider a different approach.
A few more questions:
- Do we need to apply this fix to other places, e.g. Dashboards?
- Can it be backported?
Partially fixes #56175
I don't think GitHub understands "partially" and will close the issue. Could you change it to "Related to #56175"?
|
Hello @gelicia!
Please, if the current pull request addresses a bug fix, label it with the |
I wasn't able to replicate it outside of explore.
It can - I changed it to backport it to 9.3 - but since this is a bug we found and as far as I know, has not been noticed widely, I would like to limit the backporting. I can change this if you disagree.
Done! |
gelicia
added
add to changelog
and removed
no-changelog
…lear (#62020) Explore: Be sure time range key bindings are mounted after clear (#61892) * Add list of bindings for updating on change * Revert "Add list of bindings for updating on change" This reverts commit 0927073. * Clear keybindings before component render (cherry picked from commit 6ab79c4) Co-authored-by: Kristina <kristina.durivage@grafana.com>
) * Add list of bindings for updating on change * Revert "Add list of bindings for updating on change" This reverts commit 0927073. * Clear keybindings before component render
) * Add list of bindings for updating on change * Revert "Add list of bindings for updating on change" This reverts commit 0927073. * Clear keybindings before component render
…lear (grafana#62020) Explore: Be sure time range key bindings are mounted after clear (grafana#61892) * Add list of bindings for updating on change * Revert "Add list of bindings for updating on change" This reverts commit 0927073. * Clear keybindings before component render (cherry picked from commit 6ab79c4) Co-authored-by: Kristina <kristina.durivage@grafana.com>
What is this feature?
This was a bug caused by a couple different ways keybindings get added and removed.
On mounting the explore page, the time range keybindings are added here. The hook looks for changes in
keybindingswhich does not hold any kind of state within itself so this only fires once. https://github.com/grafana/grafana/blob/main/public/app/features/explore/ExplorePage.tsx#L56In Grafana Route, it will clear and re-init global key bindings on every route change https://github.com/grafana/grafana/blob/main/public/app/core/navigation/GrafanaRoute.tsx#L22
When navigating away from Explore, the time picker key bindings get wiped. When re-entering, they get re-added but before the clear and init function runs. Clear and init then wipes the re-added time range bindings.
Thanks to @ifrost for the fix! This moves the clear and reinit to the
useLayoutEffecthook, which fires before individual components do, meaning the global keybindings are cleared and re-initialized, and then the explore page adds in the time range ones specific to explore.Which issue(s) does this PR fix?:
Related to #56175
Special notes for your reviewer:
Replication Steps:
t zto trigger a time range key binding. You will notice the time picker will change from a relative time to an exact time.t zagain. Notice that nothing happens. With the fix, the time range keybindings work as expected.