Skip to content

[OneChat] Test Tool Flyout#230484

Merged
meghanmurphy1 merged 19 commits intoelastic:mainfrom
meghanmurphy1:onechat-test-tool-flyout
Aug 28, 2025
Merged

[OneChat] Test Tool Flyout#230484
meghanmurphy1 merged 19 commits intoelastic:mainfrom
meghanmurphy1:onechat-test-tool-flyout

Conversation

@meghanmurphy1
Copy link
Contributor

@meghanmurphy1 meghanmurphy1 commented Aug 4, 2025

Summary

This PR adds the ability to test a tool when editing or creating it. It adds a "Save and Test" button that will save the tool before testing or just a "Test" button so if there were no changes to an existing tool, it'll open the flyout that will use the _execute API endpoint for testing.

Screenshot 2025-08-25 at 5 04 52 PM Screenshot 2025-08-27 at 12 47 19 PM

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.

Identify risks

Does this PR introduce any risks? For example, consider risks like hard to test bugs, performance regression, potential of data loss.

Describe the risk, its severity, and mitigation for each identified risk. Invite stakeholders and evaluate how to proceed before merging.

@meghanmurphy1 meghanmurphy1 self-assigned this Aug 4, 2025
@meghanmurphy1 meghanmurphy1 marked this pull request as ready for review August 13, 2025 17:45
@meghanmurphy1 meghanmurphy1 requested a review from a team as a code owner August 13, 2025 17:45
@meghanmurphy1 meghanmurphy1 added release_note:skip Skip the PR/issue when compiling release notes backport:skip This PR does not require backporting labels Aug 13, 2025
@meghanmurphy1 meghanmurphy1 force-pushed the onechat-test-tool-flyout branch from 064431c to 64b623e Compare August 14, 2025 15:27
@meghanmurphy1 meghanmurphy1 force-pushed the onechat-test-tool-flyout branch from 6e7702c to 35d372e Compare August 14, 2025 18:07
Copy link
Contributor

@jedrazb jedrazb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

works nicely! couple of comments

return fields;
};

export const OnechatTestFlyout: React.FC<OnechatTestToolFlyoutProps> = ({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this renders pretty bad on small screens, why not column??

Image

Copy link
Contributor Author

@meghanmurphy1 meghanmurphy1 Aug 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you mean by why not column? 🤔 I updated the Inputs to have <EuiFlexItem grow={false} style={{ minWidth: '200px', maxWidth: '300px' }}> so it doesn't stretch and look odd. Is that what you mean?

Screenshot 2025-08-25 at 5 00 10 PM

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I meant that on small screens (as in my attachment) the input and response columns are squashed (maybe we should have input and response in a single column??)

@meghanmurphy1 meghanmurphy1 force-pushed the onechat-test-tool-flyout branch from 987f75e to 19303c3 Compare August 25, 2025 20:45
@jedrazb
Copy link
Contributor

jedrazb commented Aug 26, 2025

Looks great! 🔥 another thing I noticed that for larger screens the response doesn't take the full height of flyout

Screenshot 2025-08-26 at 12 51 01

languageId="json"
value={response}
fullWidth={true}
height="calc(100vh - 275px)"
Copy link
Contributor

@jedrazb jedrazb Aug 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part feels a bit tricky, manually calculating CSS height likely won't cover all cases? Also it probably need to have minimal height?

From what I understand, the code editor requires an explicit height. Why not set it to 100%? Could we instead use flexbox to let it stretch automatically?

Also, since the result is read-only, is there a reason we’re using the CodeEditor component (which is meant for dynamic input)? Wouldn’t a EuiCodeBlock be simpler and more lightweight approach here?

Copy link
Contributor Author

@meghanmurphy1 meghanmurphy1 Aug 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I've seen CodeEditor would make more sense especially as responses could be pretty long and searching through results could get confusing using just a code block. As for the explicit height, I can see why calculating the height could be flaky, and so just set it to "75vh" which would allow to scroll through responses and keep the 'Response' title at the top.

Copy link
Contributor

@jedrazb jedrazb Aug 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, thinking out loud right now:

  • The response doesn’t work too well on narrow screens (see screenshot). overflow-x is hidden, so I can’t really see the full response.
    Screenshot 2025-08-27 at 13 43 49
  • this is especially problematic when rendering tabular data output from tool (horizontal)
  • We also have issues with calculating height.

What if we just stacked everything in a column instead?

  • Params on top
  • Response below params (under the test button)
  • This would make better use of the narrow flyout
  • We could just use full height of response and just let flyout scroll?

would make more sense especially as responses could be pretty long and searching through results could get confusing using just a code block.

Tools return typed data, so we are likely to have custom renderers anyway for .e.g esql result. You can still use cmnd F to search through JSON right?

I pasted ^ to cursor and it suggested this (stacking in column, using EuiCodeBlock).

Screen.Recording.2025-08-27.at.13.53.37.mov

Copy link
Contributor

@jedrazb jedrazb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more pass, spotted couple more nits :)

const { errors } = formState;
const { errors, isDirty } = formState;
const [showTestFlyout, setShowTestFlyout] = useState(false);
const [testToolData, setTestToolData] = useState<OnechatEsqlToolFormData>(form.getValues());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this state? Can't we infer testToolData directly from const form = useEsqlToolForm();?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are always forcing user to save before executing the tool, we could even simplify to pass just toolId to flyout test tool form , and then from within flyout form we can fetch the tool by Id using a hook useTool(toolId)

IMO this will be cleaner and we won't need to handle state propagation to child component

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up using the hook to get the full tool data but did need to use a current tool id state since the flyout can be either from Creating or Editing

Copy link
Contributor

@jedrazb jedrazb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

few nits! last ones i hope

@meghanmurphy1 meghanmurphy1 requested a review from jedrazb August 27, 2025 16:29
Copy link
Contributor

@jedrazb jedrazb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@jedrazb
Copy link
Contributor

jedrazb commented Aug 28, 2025

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
onechat 451 453 +2

Async chunks

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

id before after diff
onechat 807.0KB 810.9KB +3.8KB

Page load bundle

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

id before after diff
onechat 12.6KB 12.7KB +124.0B

History

cc @meghanmurphy1

@meghanmurphy1 meghanmurphy1 merged commit f507485 into elastic:main Aug 28, 2025
12 checks passed
qn895 pushed a commit to qn895/kibana that referenced this pull request Sep 2, 2025
## Summary
This PR adds the ability to test a tool when editing or creating it. It
adds a "Save and Test" button that will save the tool before testing or
just a "Test" button so if there were no changes to an existing tool,
it'll open the flyout that will use the `_execute` API endpoint for
testing.


<img width="1916" height="867" alt="Screenshot 2025-08-25 at 5 04 52 PM"
src="https://github.com/user-attachments/assets/6b45df50-c515-426c-9837-46c1ea9fd2ee"
/>


<img width="1917" height="860" alt="Screenshot 2025-08-27 at 12 47
19 PM"
src="https://github.com/user-attachments/assets/deb309b0-e38d-4bfa-a4b8-587d09e84919"
/>


### 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](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)
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [ ] [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
- [ ] 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)
- [ ] 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](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) 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](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
- [ ] Review the [backport
guidelines](https://docs.google.com/document/d/1VyN5k91e5OVumlc0Gb9RPa3h1ewuPE705nRtioPiTvY/edit?usp=sharing)
and apply applicable `backport:*` labels.

### Identify risks

Does this PR introduce any risks? For example, consider risks like hard
to test bugs, performance regression, potential of data loss.

Describe the risk, its severity, and mitigation for each identified
risk. Invite stakeholders and evaluate how to proceed before merging.

- [ ] [See some risk
examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx)
- [ ] ...

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:skip This PR does not require backporting release_note:skip Skip the PR/issue when compiling release notes v9.2.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants