[Lens] Make sure Lens does not reload unnecessarily

[flash1293]

Fixes #85815

This PR fixes the reload of the editor on URL change caused by changing the time range. This was caused by the redirectTo function getting re-created. Here, it is made sure the App component is only re-rendered if the saved object id or the initial input changes.

This also has the side effect of not triggering the loaded event on every time range change (which is what happened before).

Additional changes

Bonus fix because I noticed the problem: On time range change, the suggestions would reset to a loading icon instead of showing the current chart during loading and start executing immediately after change. Both of these things are undesirable - the main preview should reload first, only afterwards the suggestions should render and there should be no flickering loading icon.

Both of these things are caused by the searchContext changing when the user picks a different time range. This PR fixes it in two places - first in the suggestion panel (x-pack/plugins/lens/public/editor_frame_service/editor_frame/suggestion_panel.tsx) by using a ref for the current context to make sure no new component is generated and second in the expression renderer component (src/plugins/expressions/public/react_expression_renderer.tsx) by making sure the debounce parameter is respected for other expression loader options besides the expression itself.

[elasticmachine]

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

[flash1293]

@elasticmachine merge upstream

[streamich]

Related to this PR, instead of adding more code to the various React hooks in the ReactExpressionRenderer component, can you debounce expressionLoaderOptions (if I understand correctly, you want to debounce expressionLoaderOptions) in an isolated way without integrating with the rest of the hooks? Maybe using a useDebounce hook:

const expressionLoaderOptionsDebounced = useDebounce(expressionLoaderOptions, 1000);

Not related to this PR, I think the ReactExpressionRenderer has become an unreadable spaghetti code of React hooks. I don't think we should be adding any more hook logic to that component, but instead think about how to remove all that React hook logic from there.

[flash1293]

@streamich I started out with a solution of debouncing the loader options like you suggest, but it became much more complex than the current status of this PR because of the way the expression is used different than other loader options in a bunch of places (like the update api of the loader). useDebounce can't easily be used here because this component is part of the initial bundle and importing from the react-use package bloats this bundle a lot because this import is not shared yet (it was considered in the first PR adding the debouncing feature).

Not related to this PR, I think the ReactExpressionRenderer has become an unreadable spaghetti code of React hooks. I don't think we should be adding any more hook logic to that component, but instead think about how to remove all that React hook logic from there.

I agree with this - however this PR is meant to go into 7.11 to fix the existing bug, that's why I would like to keep it small. What do you think about going forward with a stopgap approach here to make it work for now and create a tech debt issue to clean up this component? As Lens is the only consumer of this component AFAIK I think it would make sense for the kibana app team to tackle this as part of the next fix-it-week.

I would suggest the following structure (in this cleanup PR):

• refactor out debouncing into a separate wrapper component - we already have x-pack/plugins/lens/public/debounced_component/debounced_component.tsx in Lens, I guess we could move that to kibana_react
• turn ExpressionRenderer into a class based component - this should make it easier to connect to the class based loader underneath

[streamich]

App Services change LGTM.

[wylieconlon]

I tested the two behavior changes in Firefox. I am definitely on board with the bugfix to #85815, but I'm not sure that the debounce behavior change is an improvement over what we have on master. I'm approving because I don't feel strongly enough to block it, but here is the scenario I was thinking of:

On a slower cluster, users could be confused if they click a "loading" suggestion and then get a totally different chart than what they clicked on. This is happening on this PR, but not master, because you've removed the loading spinner. The loading spinner can prevent confusion in these cases.

It's up to you- would you like to split the bugfix and the behavior change?

[wylieconlon]

Commented code

Suggested change

// const featureFlagConfig = await getByValueFeatureFlag();

[flash1293]

@elasticmachine merge upstream

[flash1293]

This is happening on this PR, but not master, because you've removed the loading spinner. The loading spinner can prevent confusion in these cases.

@wylieconlon I see your point, but on master this (showing the loading icon instead of the previous chart) happened on time range changes, not on state changes. If the time range changes, I don't think we have a confusion problem on clicking the loading suggestion. I'd say let's restore the previous behavior and revisit this later.

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: dd701c3

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
lens	1.0MB	1.0MB	+734.0B

Distributable file count

id	before	after	diff
default	47142	47902	+760

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
expressions	200.0KB	200.3KB	+297.0B

History

• 💚 Build #95337 succeeded 5eab711
• 💚 Build #94847 succeeded 25feaaa
• 💚 Build #94843 succeeded 0ef42f2

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream