[UnifiedSearch][ESQL] Fix edited query overwriting when a request is finished by kertal · Pull Request #224671 · elastic/kibana

kertal · 2025-06-20T11:24:18Z

Summary

Resolves #192950

When changing the ESQL query while the request is still running, the changes were overwritten once the currently running request resolved. This PR fixes this behavior in UnifiedSearch by aligning to the way KQL works

How to test:

Go to Discover in ESQL mode, in DevTools slow the network to 3G, or use a long running query
Run query
While query is running, change the query in the ESQL editor
When the query returns result, the ESQL editor value is no longer set to the original value
Given you slowed the network, unslow it, or submit "praises" to the network how slow it is today! 🤬

Note: I tried to cover this change with a unit test, but it didn't work, in several attempts, also the robot 🤖 was not helpful. Open for ideas (refactoring the change to a pure function would be doable, wanted to keep the scope of this fix small). So manually testing is the only way currently.

Checklist

Check the PR satisfies following conditions.

Reviewers should verify this PR satisfies this list as well.

Unit or functional tests were updated or added to match the most common scenarios

kertal · 2025-06-20T11:27:09Z

/ci

kertal · 2025-06-20T13:35:56Z

/ci

…ent-esql-edit-overwrite

src/platform/plugins/shared/unified_search/public/search_bar/search_bar.tsx

kertal · 2025-06-23T05:45:35Z

src/platform/plugins/shared/unified_search/public/search_bar/create_search_bar.tsx

            }
            savedQuery={savedQuery}
            onQuerySubmit={defaultOnQuerySubmit(props, data.query, query)}
+            onQueryChange={props.onQueryChange}


During my investigation I figured to, which onQueryChange is in the searcher props, we don't use it under the hood. seems we have no consumer for this, but we might need it in the future

I'm always a little skeptical about changing such brittle and widely consumed code... I also don't get any results when checking for references, but that seemed to be the case for all props in SearchBarOwnProps that I checked, so something odd is going on. Just not sure if we should be worried about any odd side effects here since I'm unfamiliar with the code.

I can definitely remove that, to keep it focused, from my perspective. it's not in use, and if it was used, it would not work, when in doubt, let's remove it, and fix it when we need it 👍 wdyt dear @elastic/kibana-presentation

We don't have any more context at the moment, as we haven't had the bandwidth to really take ownership of this plugin quite yet.

I've removed it for now, let's add it when we need it 👍

kertal · 2025-06-23T05:48:55Z

src/platform/plugins/shared/unified_search/public/search_bar/search_bar.tsx

+    } else if (
+      nextProps.query &&
+      isOfAggregateQueryType(nextProps.query) &&
+      nextProps.query.esql !== get(prevState, 'currentProps.query.esql')


this is the main change, we mimic what KQL does. before this change, for ESQL the query was overwritten every time, since it was not checked if the new props value differs from the old one. Now this works.

I tried to cover this change with a unit test, but I couldn't make it work. Since on re-render the currentProps.query.esql, was not kept. This made the unit test pointless 🤷

I mean, we could refactor this part to a unit test, to a pure function. Open to apply this change if valued reviewers want to have it. Or have other ideas

I'm not overly worried about it tbh, but I wonder if it's possible to test the rendered output in an RTL test? I might not understand exactly what was making it hard to test though. Otherwise a separate function like you mentioned would work if you're feeling proactive.

Yes, I wanted to do this, I've tried and it didn't work, it might be me of course

I've found a way to cover this in a simple way

32cc820

first test fails when I remove the fix

elasticmachine · 2025-06-23T06:00:57Z

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

elasticmachine · 2025-06-23T06:00:58Z

Pinging @elastic/kibana-esql (Team:ESQL)

davismcphee

I'm not very familiar with the Unified Search code, but from what I can tell it LGTM and definitely fixes the issue in Discover 👍 Thanks for such a thorough investigation and getting to the bottom of this! Glad we didn't need to resort to any hacky workarounds in Discover 🙏

On a side note, maybe you're aware already, but a good approach for testing these types of changes is to use the ES|QL DELAY function like this:

FROM kibana_sample_data_logs | EVAL delay = DELAY(1000ms) | LIMIT 10

davismcphee · 2025-06-25T02:31:57Z

src/platform/plugins/shared/unified_search/public/search_bar/search_bar.tsx

+    } else if (
+      nextProps.query &&
+      isOfAggregateQueryType(nextProps.query) &&
+      nextProps.query.esql !== get(prevState, 'currentProps.query.esql')


I'm not overly worried about it tbh, but I wonder if it's possible to test the rendered output in an RTL test? I might not understand exactly what was making it hard to test though. Otherwise a separate function like you mentioned would work if you're feeling proactive.

src/platform/plugins/shared/unified_search/public/search_bar/search_bar.tsx

davismcphee · 2025-06-25T02:42:09Z

src/platform/plugins/shared/unified_search/public/search_bar/create_search_bar.tsx

            }
            savedQuery={savedQuery}
            onQuerySubmit={defaultOnQuerySubmit(props, data.query, query)}
+            onQueryChange={props.onQueryChange}


I'm always a little skeptical about changing such brittle and widely consumed code... I also don't get any results when checking for references, but that seemed to be the case for all props in SearchBarOwnProps that I checked, so something odd is going on. Just not sure if we should be worried about any odd side effects here since I'm unfamiliar with the code.

kibanamachine · 2025-07-03T08:15:00Z

Starting backport for target branches: 8.17, 8.18, 8.19

https://github.com/elastic/kibana/actions/runs/16045183087

elasticmachine · 2025-07-03T08:15:29Z

💚 Build Succeeded

Buildkite Build
Commit: d631ab1

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
`unifiedSearch`	336.1KB	336.1KB	+63.0B

History

💚 Build #314740 succeeded 4334ab2
💚 Build #314495 succeeded 32cc820
💔 Build #314091 failed d1c0e34
💚 Build #313576 succeeded eebc719
💚 Build #311227 succeeded 07ceacc
💚 Build #310905 succeeded 07c7ff0

cc @kertal

…finished (elastic#224671) Resolved overwriting changes to the query in the ESQL Editor in UnifiedSearch while the request is still running when the previous request finished (cherry picked from commit ca7b021)

kibanamachine · 2025-07-03T08:21:57Z

💔 Some backports could not be created

Status	Branch	Result
❌	8.17	Backport failed because of merge conflicts
❌	8.18	Backport failed because of merge conflicts
✅	8.19

Note: Successful backport PRs will be merged automatically after passing CI.

Manual backport

To create the backport manually run:

node scripts/backport --pr 224671

Questions ?

Please refer to the Backport tool documentation

stratoula · 2025-07-03T08:27:01Z

@kertal I dont think we need to backport it in v8.18

v8.19, v9.1 and main are enough.

kertal · 2025-07-03T09:06:12Z

you're right, thx!

kibanamachine · 2025-07-03T09:07:27Z

Starting backport for target branches: 9.1

https://github.com/elastic/kibana/actions/runs/16046291859

kibanamachine · 2025-07-03T09:07:39Z

Starting backport for target branches: 8.19, 9.1

https://github.com/elastic/kibana/actions/runs/16046294998

…finished (elastic#224671) Resolved overwriting changes to the query in the ESQL Editor in UnifiedSearch while the request is still running when the previous request finished (cherry picked from commit ca7b021)

kibanamachine · 2025-07-03T09:13:41Z

💚 All backports created successfully

Status	Branch	Result
✅	9.1

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

…finished (elastic#224671) Resolved overwriting changes to the query in the ESQL Editor in UnifiedSearch while the request is still running when the previous request finished (cherry picked from commit ca7b021)

kibanamachine · 2025-07-03T09:14:24Z

💚 All backports created successfully

Status	Branch	Result
✅	8.19
✅	9.1

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

…est is finished (#224671) (#226344) # Backport This will backport the following commits from `main` to `8.19`: - [[UnifiedSearch][ESQL] Fix edited query overwriting when a request is finished (#224671)](#224671)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport)  Co-authored-by: Matthias Wilhelm <matthias.wilhelm@elastic.co>

…st is finished (#224671) (#226355) # Backport This will backport the following commits from `main` to `9.1`: - [[UnifiedSearch][ESQL] Fix edited query overwriting when a request is finished (#224671)](#224671)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport)  Co-authored-by: Matthias Wilhelm <matthias.wilhelm@elastic.co>

…finished (elastic#224671) Resolved overwriting changes to the query in the ESQL Editor in UnifiedSearch while the request is still running when the previous request finished

Fix query handling in SearchBarUI for aggregate query type changes

9ca5648

Merge branch 'main' into unified-search-prevent-esql-edit-overwrite

07c7ff0

kertal mentioned this pull request Jun 20, 2025

ESQL Editor: Query gets overwritten when changed while loading #192950

Closed

kertal added 3 commits June 23, 2025 07:37

Deangularize the comments

862d48e

Make use of the onQueryChange when creating the search bar

cf62de5

Merge remote-tracking branch 'upstream/main' into unified-search-prev…

07ceacc

…ent-esql-edit-overwrite

kertal commented Jun 23, 2025

View reviewed changes

src/platform/plugins/shared/unified_search/public/search_bar/search_bar.tsx Show resolved Hide resolved

kertal commented Jun 23, 2025

View reviewed changes

kertal self-assigned this Jun 23, 2025

kertal requested a review from a team June 23, 2025 05:59

kertal changed the title ~~Fix query handling in SearchBarUI for aggregate query type changes~~ [Discover][ESQL] Fix query handling in SearchBarUI for aggregate query type changes Jun 23, 2025

kertal requested a review from stratoula June 23, 2025 06:00

kertal marked this pull request as ready for review June 23, 2025 06:00

kertal requested a review from a team as a code owner June 23, 2025 06:00

kertal added the backport:prev-major label Jun 23, 2025

This was referenced Jun 23, 2025

[Discover][UnifiedSearch] POC to enable onQueryChange #224450

Closed

Discover eql refactor editor query #224026

Closed

[Disccover][ES|QL] Attempt to preventing edit query overwriting ofter submission of query #223394

Closed

Merge branch 'main' into unified-search-prevent-esql-edit-overwrite

fecbe86

davismcphee approved these changes Jun 25, 2025

View reviewed changes

Merge branch 'main' into unified-search-prevent-esql-edit-overwrite

4334ab2

stratoula approved these changes Jul 2, 2025

View reviewed changes

Merge branch 'main' into unified-search-prevent-esql-edit-overwrite

d631ab1

kertal enabled auto-merge (squash) July 3, 2025 06:25

kertal merged commit ca7b021 into elastic:main Jul 3, 2025
11 checks passed

kibanamachine added the v9.2.0 label Jul 3, 2025

kibanamachine mentioned this pull request Jul 3, 2025

[8.19] [UnifiedSearch][ESQL] Fix edited query overwriting when a request is finished (#224671) #226344

Merged

kertal added backport:version Backport to applied version labels v9.1.0 v8.19.0 and removed backport:prev-major labels Jul 3, 2025

kibanamachine mentioned this pull request Jul 3, 2025

[9.1] [UnifiedSearch][ESQL] Fix edited query overwriting when a request is finished (#224671) #226355

Merged

kibanamachine mentioned this pull request Jul 3, 2025

[ES|QL] Improve binary expression grouping #225416

Merged

1 task

kertal deleted the unified-search-prevent-esql-edit-overwrite branch July 6, 2025 07:35

Comments

Conversation

kertal commented Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

kertal commented Jun 20, 2025

Uh oh!

kertal commented Jun 20, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Jun 23, 2025

Uh oh!

elasticmachine commented Jun 23, 2025

Uh oh!

davismcphee left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kibanamachine commented Jul 3, 2025

Uh oh!

elasticmachine commented Jul 3, 2025

💚 Build Succeeded

Metrics [docs]

Async chunks

History

Uh oh!

kibanamachine commented Jul 3, 2025

💔 Some backports could not be created

Manual backport

Questions ?

Uh oh!

stratoula commented Jul 3, 2025

Uh oh!

kertal commented Jul 3, 2025

Uh oh!

kibanamachine commented Jul 3, 2025

Uh oh!

kibanamachine commented Jul 3, 2025

Uh oh!

kibanamachine commented Jul 3, 2025

💚 All backports created successfully

Questions ?

Uh oh!

kibanamachine commented Jul 3, 2025

💚 All backports created successfully

Questions ?

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

kertal commented Jun 20, 2025 •

edited

Loading