[onechat] Implement ES|QL tool editing by dennis-tismenko · Pull Request #229224 · elastic/kibana

dennis-tismenko · 2025-07-23T21:14:24Z

Summary

Adds the ability to edit ES|QL tools from the tools page, leveraging the existing flyout UI used when creating new ES|QL tools.

Screen.Recording.2025-07-23.at.5.11.06.PM.mov

Checklist

Check the PR satisfies following conditions.

Reviewers should verify this PR satisfies this list as well.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The release_note:breaking label should be applied in these situations.
Flaky Test Runner was used on any tests changed
The PR description includes the appropriate Release Notes section, and the correct release_note:* label is applied per the guidelines
Review the backport guidelines and apply applicable backport:* labels.

jedrazb

Lgtm! Testes locally, one question if we really need to disable that eslint rule

x-pack/platform/packages/shared/onechat/onechat-common/tools/esql.ts

x-pack/platform/plugins/shared/onechat/public/application/components/tools/esql/esql_tools.tsx

jedrazb · 2025-07-24T09:16:03Z

x-pack/platform/plugins/shared/onechat/public/application/hooks/tools/use_edit_tools.ts

+    onError,
+  });
+
+  const openFlyout = useCallback((tool: EsqlToolDefinitionWithSchema) => {


doing it this way is fine if we stick with flyout, but will be problematic if we ever change tool creation to be a separate page.

Yeah, I think it's a given that the implementation will need to change once we move to a separate page view — it shouldn't be terribly difficult to refactor

and by the looks of latest designs we will be moving there

x-pack/platform/packages/shared/onechat/onechat-common/tools/esql.ts

pgayvallet · 2025-07-24T15:49:31Z

...latform/plugins/shared/onechat/public/application/components/tools/esql/esql_tool_flyout.tsx

-                defaultMessage: 'New ES|QL tool',
-              })}
-            </h2>
+            <h2 id={esqlToolFlyoutTitleId}>{title}</h2>


NIT: if we just have two title (one for create mode, one for edit mode), I think it would make sense to just pass a edition: boolean prop to the component and handling the two labels internally instead of passing the title as a props. Seems like a better isolation of concerns to me

pgayvallet · 2025-07-24T15:56:02Z

x-pack/platform/plugins/shared/onechat/public/application/components/tools/esql/esql_tools.tsx

+            onClick={() => {
+              editTool(tool);
+            }}


I think @jedrazb mentioned something similar - It feels strange to me that we're passing the full tool definition to the edit handler / flyout. I would expect to just be passing the toolId, and let the edition view fetch the corresponding data. What we are doing here is coupling the interface we're using to list tools, and to edit them, and if today those are the same, this will likely change (e.g. we don't need to return all properties for the edition page).

Not strictly blocker, but this is a code smell ihmo.

Yeah, I see your point here. Fixed it to take in a toolId instead of the entire tool definition

x-pack/platform/plugins/shared/onechat/public/application/hooks/tools/use_edit_tools.ts

pgayvallet · 2025-07-24T16:06:55Z

x-pack/platform/plugins/shared/onechat/public/application/components/tools/esql/esql_tools.tsx

+  const handleEditSuccess = useCallback(() => {
+    addSuccessToast({
+      title: i18n.translate('xpack.onechat.tools.editEsqlToolSuccessToast', {
+        defaultMessage: 'ES|QL tool updated',


NIT: we usually have the id/name of the entity for "XXX created/updated/delete" toasts in Kibana.

Also I'm not sure we want to surface the type of the tool in those messages, I feel like maybe something like Tool {toolName} updated would make more sense? (but it's more a product, design question)

Makes sense to me, I've added the ID for now and will incorporate the latest copy from the UX mocks as part of my next task to incorporate the Tools V2 design

elasticmachine · 2025-07-26T00:16:13Z

💚 Build Succeeded

Buildkite Build
Commit: 5e46219

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`onechat`	409	412	+3

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`@kbn/onechat-common`	142	141	-1

Async chunks

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

id	before	after	diff
`onechat`	591.3KB	594.5KB	+3.1KB

Unknown metric groups

API count

id	before	after	diff
`@kbn/onechat-common`	219	218	-1

ESLint disabled line counts

id	before	after	diff
`@kbn/onechat-common`	0	1	+1

Total ESLint disabled count

id	before	after	diff
`@kbn/onechat-common`	0	1	+1

History

💔 Build #322937 failed 0d70d03
💚 Build #322628 succeeded 3254421

## Summary Adds the ability to edit ES|QL tools from the tools page, leveraging the existing flyout UI used when creating new ES|QL tools. https://github.com/user-attachments/assets/7d80a2a2-ac66-41c2-9c7a-4b7de941451f ### Checklist Check the PR satisfies following conditions. Reviewers should verify this PR satisfies this list as well. - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md) - [x] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [x] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [x] This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The `release_note:breaking` label should be applied in these situations. - [x] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [x] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) - [x] Review the [backport guidelines](https://docs.google.com/document/d/1VyN5k91e5OVumlc0Gb9RPa3h1ewuPE705nRtioPiTvY/edit?usp=sharing) and apply applicable `backport:*` labels.

dennis-tismenko added the release_note:skip Skip the PR/issue when compiling release notes label Jul 23, 2025

dennis-tismenko requested a review from a team as a code owner July 23, 2025 21:14

dennis-tismenko added backport:skip This PR does not require backporting v9.2.0 labels Jul 23, 2025

dennis-tismenko changed the title ~~[onechat] Implement ES|QL tool editing, refactor tool hooks~~ [onechat] Implement ES|QL tool editing Jul 23, 2025

dennis-tismenko requested review from jedrazb and pgayvallet July 23, 2025 21:34

jedrazb reviewed Jul 24, 2025

View reviewed changes

dennis-tismenko force-pushed the onechat/edit-esql-tool branch from 3254421 to 0d70d03 Compare July 24, 2025 15:01

dennis-tismenko requested a review from jedrazb July 24, 2025 15:24

jedrazb approved these changes Jul 24, 2025

View reviewed changes

dennis-tismenko enabled auto-merge (squash) July 24, 2025 15:37

pgayvallet reviewed Jul 24, 2025

View reviewed changes

dennis-tismenko added 3 commits July 25, 2025 18:48

[onechat] Implement ES|QL tool editing, refactor tool hooks

6efa1cf

fix: error handling

f434542

fix: address nits

5e46219

dennis-tismenko force-pushed the onechat/edit-esql-tool branch from 0d70d03 to 5e46219 Compare July 25, 2025 22:48

dennis-tismenko merged commit 87ff874 into elastic:main Jul 26, 2025
12 checks passed

Conversation

dennis-tismenko commented Jul 23, 2025

Summary

Checklist

Uh oh!

jedrazb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

elasticmachine commented Jul 26, 2025

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

API count

ESLint disabled line counts

Total ESLint disabled count

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants