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

PanelAttention: Fix delay #867

Merged
merged 3 commits into from
Aug 19, 2024
Merged

PanelAttention: Fix delay #867

merged 3 commits into from
Aug 19, 2024

Conversation

tskarhed
Copy link
Contributor

@tskarhed tskarhed commented Aug 13, 2024

Problem:
If you move your mouse from another panel quickly, it means that the mouseover will be executed before the debounce mousemove on the previous panel. This causes inconsistency.

Solution:
Debouncing onMouseEnter too prevents this, but if you press e within 100ms of entering the new panel, it will always enter the old panel.

If this approach isn't good enough, we could try to decrease the debounce time. If there is any other approach that I could consider, I'm all ears!

Closes grafana/grafana#90020

📦 Published PR as canary version: 5.9.1--canary.867.10450028526.0

✨ Test out this PR locally via:

npm install @grafana/scenes-react@5.9.1--canary.867.10450028526.0
npm install @grafana/scenes@5.9.1--canary.867.10450028526.0
# or 
yarn add @grafana/scenes-react@5.9.1--canary.867.10450028526.0
yarn add @grafana/scenes@5.9.1--canary.867.10450028526.0

@tskarhed tskarhed requested a review from torkelo August 13, 2024 08:14
Copy link
Contributor

@ashharrison90 ashharrison90 left a comment

Choose a reason for hiding this comment

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

@tskarhed can we use the leading option of debounce instead onMouseMove?

https://lodash.com/docs/#debounce

that way it should fire once immediately, then debounce as before

@tskarhed tskarhed changed the title VizPanelRenderer: Debounce onMouseEnter PanelAttention: Fix delay Aug 13, 2024
@torkelo
Copy link
Member

torkelo commented Aug 15, 2024

@tskarhed see the comment from @ashharrison90

@tskarhed
Copy link
Contributor Author

@torkelo Yes, we tried to fix it, but we just couldn't get it running when linking Scenes. I could make the code change, but I have no way of testing it. Since I get no error, I can only assume it has to do with some incompatible version of Grafana UI, although updating the dependency locally did not do the trick.

I would really appreciate some help with testing it, I had similar issues the last couple of times working with Scenes.

@torkelo
Copy link
Member

torkelo commented Aug 16, 2024

@tskarhed it looks like we still have the problem that when yarn linking scene into main repo the grafana/ui dependency is not the one from runtime (core) but the scenes get's' it's own copy bundled in (using the version specified in this repos package json)

I recall that we had fix this

@torkelo torkelo added the release Create a release when this pr is merged label Aug 16, 2024
@torkelo
Copy link
Member

torkelo commented Aug 16, 2024

@tskarhed so the reason it's not working now when testing this is that PanelChrome used is an old version that does not have onMouseEnter onMouseLeave etc,

added a release label to this PR, we can test with the npm canary release that will be generated

@ashharrison90
Copy link
Contributor

ashharrison90 commented Aug 16, 2024

i've hacked it locally (just updated scenes-app package.json to use canary versions of all the @grafana/ packages)...

and the leading option works, but you have to specify both leading and trailing, like:

  const debouncedMouseMove = useMemo(() => debounce(setPanelAttention, 100, {
    leading: true,
    trailing: false,
  }), [setPanelAttention]);

@tskarhed
Copy link
Contributor Author

I still can't get this to run. I changed to "@grafana/ui": "11.3.0-192050" in Scenes' package.json. If someone else could verify that this works, it would be great.

Copy link
Contributor

@ashharrison90 ashharrison90 left a comment

Choose a reason for hiding this comment

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

i can verify this works. you probably don't want the changes in scenes/package.json and yarn.lock tho

@tskarhed
Copy link
Contributor Author

i can verify this works. you probably don't want the changes in scenes/package.json and yarn.lock tho

Oops 😢

Copy link
Contributor

@ashharrison90 ashharrison90 left a comment

Choose a reason for hiding this comment

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

lgtm 👍

@torkelo torkelo merged commit 5fca3f9 into main Aug 19, 2024
3 checks passed
@torkelo torkelo deleted the tskarhed/attention-debounce branch August 19, 2024 10:06
@grafanabot
Copy link
Contributor

🚀 PR was released in v5.9.1 🚀

@bob454522
Copy link

bob454522 commented Sep 6, 2024

i installed Grafana v11.3.0-75433 (cdf035f813) to address a different (un-related) bug, and wanted to make a note here that this is still occurring (ie there is still a large lag/delay on pressing e to edit a panel, so you frequently end up editing the wrong panel accidentally).

apologies if this is planned to be fixed in a different version, but i wanted to bring this up (as from the best I can tell, this merge should be in the nightly that im currently running - ie v11.3.0-75433 - but im not proficient in coding/github ).

happy to provide any further data/assistance
thank you

@frifox
Copy link

frifox commented Sep 16, 2024

Confirming. Still an issue on v11.3.0-196835 (d1ffcc22d9)

@tskarhed
Copy link
Contributor Author

@bob454522 @frifox Can you reproduce this on https://play.grafana.org/ ? It is running Grafana v11.3.0-75826

@frifox
Copy link

frifox commented Sep 26, 2024

Seems to be fixed.

Don't know how play.grafana.org version tags correlate to docker images, but:

  • Upgraded my local docker instance to grafana/grafana-oss-dev:11.3.0-198777 = it's fixed.
  • Downgraded to older grafana/grafana-oss-dev:11.3.0-196835 image = bug is back.

@tskarhed
Copy link
Contributor Author

I am not exactly sure when the Scenes version bump happened in core Grafana, but I bet that's when it was fixed. As for the version - 11.3 hasn't been released yet, so it should only be solved in either dev versions or Grafana Cloud until then.

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.

Delay in keyboard shortcuts ( v11.0 or newer only)
6 participants