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] Typing into the dimension label input is not stable #104222

Closed
Tracked by #57706
wylieconlon opened this issue Jul 1, 2021 · 3 comments · Fixed by #104421
Closed
Tracked by #57706

[Lens] Typing into the dimension label input is not stable #104222

wylieconlon opened this issue Jul 1, 2021 · 3 comments · Fixed by #104421
Assignees
Labels
bug Fixes for quality problems that affect the customer experience Feature:Lens Team:Visualizations Visualization editors, elastic-charts and infrastructure

Comments

@wylieconlon
Copy link
Contributor

Steps to reproduce:

  1. Go to the Metric visualization type
  2. Create any metric
  3. Attempt to insert a totally new title into the text box, and then try to modify the middle of your existing title

Previously, we were able to determine which text changes were coming from local vs external sources, but something has caused a regression in this behavior. My theory here is that it's been caused by the use of activeData in the dimension editor, which was only added recently and hasn't shipped to users yet.

text input issues

@wylieconlon wylieconlon added bug Fixes for quality problems that affect the customer experience Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:Lens labels Jul 1, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@flash1293
Copy link
Contributor

This is happening because of the async state setting for datasources. After onChange is called, the debounce logic assumes there are no unflushed changes anymore:

However, as the state change is only submitted in the next tick, this isn't true anymore.

The most stable fix is to not do async state updates for the datasource. @wylieconlon can you gauge how bad this would be in terms of performance?

I also played around with another approach - waiting for a bit after onChange got called, assuming unflushed changes for this time:

  const flushHandle = useRef<NodeJS.Timeout | undefined>();

  const onChangeDebounced = useMemo(() => {
    const callback = debounce((val: T) => {
      onChange(val);
     // do not reset unflushed flag right away, wait a bit for upstream to pick it up
      flushHandle.current = setTimeout(() => {
        unflushedChanges.current = false;
      }, 256);
    }, 256);
    return (val: T) => {
      if (flushHandle.current) {
        clearTimeout(flushHandle.current);
      }
      unflushedChanges.current = true;
      callback(val);
    };
  }, [onChange]);

I couldn't break it even with 6x CPU slowdown.

@wylieconlon
Copy link
Contributor Author

@flash1293 the async update isn't currently for performance reasons, it's for integration with Monaco. What if the updateDatasource returned a promise that resolves when the update is submitted to React, could that be used to indicate that the debouncing is dirty until the result is fully submitted? Alternatively, this all might go away once we use Redux for the datasource state.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Feature:Lens Team:Visualizations Visualization editors, elastic-charts and infrastructure
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants