Tweaks to prompt editor #174030

CoenWarmer · 2023-12-28T16:04:04Z

Summary

This fixes the following edge case when using the chat interface:

When the main editor has a value in the text area, and the user edits an existing message, and presses <enter> on the keyboard, the value that was in the main editor is appended as a new message, instead of editing the existing message.

It also does a bit of cleanup (moving of ChatPromptEditor components to /components/prompt_editor, and renaming to PromptEditor.)

Additional fixes

apmmachine · 2023-12-28T16:04:19Z

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

/oblt-deploy : Deploy a Kibana instance using the Observability test environments.
/oblt-deploy-serverless : Deploy a serverless Kibana instance using the Observability test environments.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

dgieselaar · 2023-12-29T12:31:58Z

x-pack/plugins/observability_ai_assistant/public/components/chat/chat_body.tsx

@@ -141,6 +141,8 @@ export function ChatBody({
      : '100%'};
  `;

+  const [isEditing, setIsEditing] = useState(false);


what does this signify? I guess it means "editing a single message"?

Maybe this is a dumb question, but if focus can only be on a single input, why don't we use that to constrain submits?

@dgieselaar This allows this behavior (disabling the main editor when editing an existing message):

Screen.Recording.2024-01-02.at.16.48.59.mov

We could not do that. Wdyt?

…when done editing

…to avoid sharing of model between multiple editors

…pt-editing

…ana; branch 'main' of github.com:elastic/kibana into bug/inline-prompt-editing

x-pack/plugins/observability_ai_assistant/public/hooks/use_json_editor_model.ts

dgieselaar · 2024-01-08T11:14:27Z

...lugins/observability_ai_assistant/public/components/prompt_editor/prompt_editor_function.tsx

+    return () => {
+      onBlur();
+    };
+  }, [onBlur]);


Hmm this looks a little dangerous in terms of consumers passing a new callback every render?

Its in the return of the useEffect though, so it should only run on unmount, no?

oh right, it just re-creates the unmount hook. I think it's better to use a ref here though, that way we don't call this hook on every render. I'm not too worried about performance, but more so about bugs creeping in and testability.

That was my first thought as well, unfortunately Monaco (or at least Kibana's implementation of it) does not have an onBlur.

FWIW it also does not have an onFocus. It only has an editorDidMount prop, which can be used as an alternative for onFocus. There is no corresponding editorWillUnmount though 🤷🏻‍♂️

sorry, I meant onBlurRef = useRef(props.onBlur); onBlurRef.current = props.onBlur;

Turns out the monaco editor instance has a onDidBlurEditorWidget event.

Implemented with:

editorRef.current?.onDidBlurEditorWidget(() => { onBlur(); });

x-pack/plugins/observability_ai_assistant/public/hooks/use_json_editor_model.ts

dgieselaar

Just nits left

x-pack/plugins/observability_ai_assistant/public/components/chat/chat_body.tsx

x-pack/plugins/observability_ai_assistant/public/components/chat/function_list_popover.tsx

dgieselaar · 2024-01-08T12:40:05Z

x-pack/plugins/observability_ai_assistant/public/components/prompt_editor/prompt_editor.tsx

+      if (innerMessage && !disabled && !invalid && hasFocus) {
+        if (!event.shiftKey && event.key === keys.ENTER) {
+          event.preventDefault();
+          handleSubmit();
+        }


This is a complicated if, can we simplify? (I don't think we need two ifs anyway?

Yeah, the idea was future proofing, allowing easier additional other keyboard commands. First if checking if it should continue, send the key combination. But if we think no additional kb commands will be added I can consolidate.

dgieselaar · 2024-01-08T12:46:40Z

...lugins/observability_ai_assistant/public/components/prompt_editor/prompt_editor_function.tsx

+    return () => {
+      onBlur();
+    };
+  }, [onBlur]);


oh right, it just re-creates the unmount hook. I think it's better to use a ref here though, that way we don't call this hook on every render. I'm not too worried about performance, but more so about bugs creeping in and testability.

kibana-ci · 2024-01-08T15:58:56Z

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

[job] [logs] Jest Tests #5 / EditableMarkdown Cancel button click calls only onChangeEditable
[job] [logs] Jest Tests #5 / EditableMarkdown Does not call onSaveContent if no change from current text
[job] [logs] Jest Tests #5 / EditableMarkdown draft comment Cancel button click clears session storage
[job] [logs] Jest Tests #5 / EditableMarkdown draft comment existing storage key should have session storage value same as draft comment
[job] [logs] Jest Tests #5 / EditableMarkdown draft comment Save button click clears session storage
[job] [logs] Jest Tests #5 / EditableMarkdown errors Shows error message and save button disabled if current text is empty
[job] [logs] Jest Tests #5 / EditableMarkdown errors Shows error message and save button disabled if current text is of empty characters
[job] [logs] Jest Tests #5 / EditableMarkdown errors Shows error message and save button disabled if current text is too long
[job] [logs] Jest Tests #5 / EditableMarkdown Save button click calls onSaveContent and onChangeEditable when text area value changed

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
`observabilityAIAssistant`	151.2KB	151.8KB	+607.0B

History

💔 Build #186356 failed 1b614b7
💔 Build #186348 failed 2a6f1e1
💔 Build #186337 failed 369f3a8
💚 Build #186314 succeeded 04ceeba
💔 Build #186129 failed 5328541
💚 Build #185870 succeeded 4fc138d

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

kibanamachine · 2024-01-08T16:03:21Z

💔 All backports failed

Status	Branch	Result
❌	8.12	Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 174030

Questions ?

Please refer to the Backport tool documentation

kibanamachine · 2024-01-09T21:49:04Z

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 174030 locally

## Summary This fixes the following edge case when using the chat interface: * When the main editor has a value in the text area, and the user edits an existing message, and presses `<enter>` on the keyboard, the value that was in the main editor is appended as a new message, instead of editing the existing message. It also does a bit of cleanup (moving of ChatPromptEditor components to `/components/prompt_editor`, and renaming to `PromptEditor`.) ### Additional fixes * [Don't stick to bottom when changing to edit mode, re-stick to bottom when done editing](elastic@e8a01c1) * [Autofocus function popover list search box upon opening](elastic@2329d1c) * [Remove focus trap as it wasn't doing anything anymore](elastic@7fcb4e0) * [Move constants used when creating monaco model inside function scope to avoid sharing of model between multiple editor instances](elastic@c9cab2c) * [Disable submitting function editor when json is not valid](elastic@2a6f1e1)

kibanamachine · 2024-01-13T21:48:56Z

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 174030 locally

CoenWarmer · 2024-01-26T09:57:42Z

💚 All backports created successfully

Status	Branch	Result
✅	8.12

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

Questions ?

Please refer to the Backport tool documentation

sorenlouv · 2024-01-26T10:39:25Z

💚 All backports created successfully

Status	Branch	Result
✅	8.12

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

Questions ?

Please refer to the Backport tool documentation

## Summary This fixes the following edge case when using the chat interface: * When the main editor has a value in the text area, and the user edits an existing message, and presses `<enter>` on the keyboard, the value that was in the main editor is appended as a new message, instead of editing the existing message. It also does a bit of cleanup (moving of ChatPromptEditor components to `/components/prompt_editor`, and renaming to `PromptEditor`.) ### Additional fixes * [Don't stick to bottom when changing to edit mode, re-stick to bottom when done editing](elastic@e8a01c1) * [Autofocus function popover list search box upon opening](elastic@2329d1c) * [Remove focus trap as it wasn't doing anything anymore](elastic@7fcb4e0) * [Move constants used when creating monaco model inside function scope to avoid sharing of model between multiple editor instances](elastic@c9cab2c) * [Disable submitting function editor when json is not valid](elastic@2a6f1e1) (cherry picked from commit f4265ca) # Conflicts: # x-pack/plugins/observability_ai_assistant/public/components/prompt_editor/prompt_editor_function.tsx

# Backport This will backport the following commits from `main` to `8.12`: - [Tweaks to prompt editor (#174030)](#174030)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  --------- Co-authored-by: kibanamachine <[email protected]>

CoenWarmer requested a review from a team as a code owner December 28, 2023 16:04

CoenWarmer added release_note:skip Skip the PR/issue when compiling release notes v8.12.0 backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) labels Dec 28, 2023

Tweaks to prompt editor

e963efa

CoenWarmer force-pushed the bug/inline-prompt-editing branch from e4dff55 to e963efa Compare December 28, 2023 17:01

dgieselaar reviewed Dec 29, 2023

View reviewed changes

CoenWarmer added 6 commits January 4, 2024 15:51

Don't stick to bottom when changing to edit mode, re-stick to bottom …

e8a01c1

…when done editing

Autofocus function popover list search box upon opening

2329d1c

Remove focus trap as it wasn't doing anything anymore

7fcb4e0

Cleanup

7b73dec

Move monaco constants used when creating model inside function scope …

c9cab2c

…to avoid sharing of model between multiple editors

Merge branch 'main' of github.com:elastic/kibana into bug/inline-prom…

4fc138d

…pt-editing

CoenWarmer requested a review from dgieselaar January 4, 2024 15:03

CoenWarmer added 2 commits January 5, 2024 16:09

Merge branch 'main' of github.com:elastic/kibana into bug/inline-prom…

5328541

…pt-editing

Merge branch 'bug/inline-prompt-editing' of github.com:CoenWarmer/kib…

04ceeba

…ana; branch 'main' of github.com:elastic/kibana into bug/inline-prompt-editing

dgieselaar reviewed Jan 8, 2024

View reviewed changes

x-pack/plugins/observability_ai_assistant/public/hooks/use_json_editor_model.ts Outdated Show resolved Hide resolved

dgieselaar reviewed Jan 8, 2024

View reviewed changes

CoenWarmer requested a review from dgieselaar January 8, 2024 11:19

dgieselaar reviewed Jan 8, 2024

View reviewed changes

x-pack/plugins/observability_ai_assistant/public/hooks/use_json_editor_model.ts Outdated Show resolved Hide resolved

CoenWarmer requested a review from dgieselaar January 8, 2024 11:35

CoenWarmer added 3 commits January 8, 2024 12:40

Address feedback

369f3a8

Disable submitting function editor when json is not valid

2a6f1e1

Fix types

1b614b7

dgieselaar approved these changes Jan 8, 2024

View reviewed changes

CoenWarmer added 2 commits January 8, 2024 14:04

Fix types

3a39fcd

Last nits

16ef56a

CoenWarmer enabled auto-merge (squash) January 8, 2024 13:31

CoenWarmer merged commit f4265ca into elastic:main Jan 8, 2024
23 checks passed

kibanamachine added the v8.13.0 label Jan 8, 2024

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jan 9, 2024

CoenWarmer mentioned this pull request Jan 26, 2024

[8.12] Tweaks to prompt editor (#174030) #175664

Merged

sorenlouv mentioned this pull request Jan 26, 2024

[8.12] Tweaks to prompt editor (#174030) #175669

Closed

sorenlouv added auto-backport Deprecated - use backport:version if exact versions are needed v8.12.1 and removed v8.12.0 labels Jan 26, 2024

sorenlouv removed backport missing Added to PRs automatically when the are determined to be missing a backport. auto-backport Deprecated - use backport:version if exact versions are needed labels Jan 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tweaks to prompt editor #174030

Tweaks to prompt editor #174030

CoenWarmer commented Dec 28, 2023 •

edited by kibanamachine

Loading

apmmachine commented Dec 28, 2023

dgieselaar Dec 29, 2023

dgieselaar Dec 29, 2023

CoenWarmer Jan 2, 2024 •

edited

Loading

dgieselaar Jan 8, 2024

CoenWarmer Jan 8, 2024

dgieselaar Jan 8, 2024

CoenWarmer Jan 8, 2024 •

edited

Loading

dgieselaar Jan 8, 2024

CoenWarmer Jan 8, 2024

dgieselaar left a comment

dgieselaar Jan 8, 2024

CoenWarmer Jan 8, 2024

dgieselaar Jan 8, 2024

kibana-ci commented Jan 8, 2024

kibanamachine commented Jan 8, 2024

kibanamachine commented Jan 9, 2024

kibanamachine commented Jan 13, 2024

CoenWarmer commented Jan 26, 2024

sorenlouv commented Jan 26, 2024

Tweaks to prompt editor #174030

Tweaks to prompt editor #174030

Conversation

CoenWarmer commented Dec 28, 2023 • edited by kibanamachine Loading

Summary

Additional fixes

apmmachine commented Dec 28, 2023

🤖 GitHub comments

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CoenWarmer Jan 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CoenWarmer Jan 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgieselaar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibana-ci commented Jan 8, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Async chunks

History

kibanamachine commented Jan 8, 2024

💔 All backports failed

Manual backport

Questions ?

kibanamachine commented Jan 9, 2024

kibanamachine commented Jan 13, 2024

CoenWarmer commented Jan 26, 2024

💚 All backports created successfully

Questions ?

sorenlouv commented Jan 26, 2024

💚 All backports created successfully

Questions ?

CoenWarmer commented Dec 28, 2023 •

edited by kibanamachine

Loading

CoenWarmer Jan 2, 2024 •

edited

Loading

CoenWarmer Jan 8, 2024 •

edited

Loading