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

[Lens] assist EUI with bug investigation to get to version 77.2.0 #155890

Closed
drewdaemon opened this issue Apr 26, 2023 · 6 comments
Closed

[Lens] assist EUI with bug investigation to get to version 77.2.0 #155890

drewdaemon opened this issue Apr 26, 2023 · 6 comments
Labels
EUI Feature:Lens Team:Visualizations Visualization editors, elastic-charts and infrastructure

Comments

@drewdaemon
Copy link
Contributor

Overview

This issue covers collaborating with the EUI team to remove a Lens-related blocker for them to get version 77.2.0 into Kibana.

The problem

The EUI team added an enhancement to the EuiFieldNumber component in version 77.2.0 (diff). This change causes the following bug in Lens

STR

  • add annotation layer
  • change to query-based
  • add query

Expected: input reflects query
Actual: input says "Empty"

  • change another setting such as time field

Expected: query remains
Actual: query is erased and annotations disappear

feature-branch-annotations.mov

Technical details

The point of failure seems to be in the useDebouncedValue hook in the AnnotationsPanel component. In the example above, the redux store is correctly updated with the query, but the local state of the AnnotationsPanel does not receive the update. Then, when the second setting is changed, the AnnotationsPanel updates the redux store according to its out-of-date copy of the state which does not include the query.

Since the query is erased, the annotation gets filtered out (via isValidAnnotation) of the expression altogether which leads to the disappearance of the annotations in the visualization.

It is unclear why the change that was made to the EuiFieldNumber is affecting a hook in a component two levels above it in the tree, but I do think that the requestAnimationFrame that was added is a suspect. However, it only gets set up when a key event is detected. Whatever the problem is, it almost feels like the AnnotationsPanel is one tick behind.

@drewdaemon drewdaemon added Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:Lens EUI labels Apr 26, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/eui-team (EUI)

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-visualizations @elastic/kibana-visualizations-external (Team:Visualizations)

@cee-chen
Copy link
Member

Good news @drewdaemon - I was spiking out the change to revert in EUI (which was the EuiFieldNumber validity detection mentioned in the PR description). While completely reverting it did remove the bug, I also briefly spiked out experimenting with reducing the potential rerender vectors in elastic/eui#6741, and to my total surprise and no small amount of bafflement... the changes appear to work! So we get the best of both worlds - EUI's new enhancement stays in while not causing issues for Lens. 🎉 🎉

I've rebased #155208 to remove the changes to Lens' redux state, and it looks like the previous FTR test that was failing in CI is no longer doing so. Feel free to pull down that branch from scratch to test locally to confirm.

Thank you so much for all your time spent with us pairing and talking through the problem - it was beyond helpful in narrowing down a solution!

@drewdaemon
Copy link
Contributor Author

Amazing work, @cee-chen . Thanks for sticking with it!

@drewdaemon
Copy link
Contributor Author

Closing since the issue has been resolved without changes to the Lens app.

@cee-chen
Copy link
Member

cee-chen commented Oct 31, 2023

FYI, this just came up again in the most recent EUI upgrade (#170179) that contains another EuiFieldNumber update (elastic/eui#7291).

There's nothing in that EuiFieldNumber update that looks problematic or that we could revert at this point - it's a legitimate feature request that should not be causing rerender issues. So I looked into the Kibana code a bit more and I was able to work around the state behavior by memoizing the LineThicknessSlider component and the onChange/setConfig callback: 0657a08?w=1

LMK if you have any objections to the proposed workaround.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
EUI Feature:Lens Team:Visualizations Visualization editors, elastic-charts and infrastructure
Projects
None yet
Development

No branches or pull requests

3 participants