feat(Leo): Support modifying user prompts #24558
Conversation
github-actions
bot
added
CI/storybook-url
yrliou
force-pushed
the
ai_chat_prompt_edit
branch
from
b7adf3f to
6b164b8
Compare
|
A Storybook has been deployed to preview UI for the latest push |
| </div> | ||
| )} | ||
| <div className={styles.turnActions}> | ||
| <CopyButton onClick={handleCopyText} /> |
There was a problem hiding this comment.
you can copy human text now? i thought copy was only allowed on assitant turns.
There was a problem hiding this comment.
I think so as it's how it looks in the figma.
| align-self: stretch; | ||
| } | ||
|
|
||
| icon { |
There was a problem hiding this comment.
do you mean leo-icon? also, you can directly modify the size in .editIndicator style with css ex. --leo-icon-size: 16px
There was a problem hiding this comment.
Thanks for the tip, fixed in 1613bf9.
| onCancel: () => void | ||
| } | ||
|
|
||
| function EditInput(props: Props) { |
There was a problem hiding this comment.
should the positions and background of the action buttons be tweaked when the input box has multiple lines?
There was a problem hiding this comment.
Updated after updated design.
|
A Storybook has been deployed to preview UI for the latest push |
|
A Storybook has been deployed to preview UI for the latest push |
yrliou
force-pushed
the
ai_chat_prompt_edit
branch
from
8244582 to
62a9a67
Compare
|
A Storybook has been deployed to preview UI for the latest push |
yrliou
force-pushed
the
ai_chat_prompt_edit
branch
from
62a9a67 to
2c6bbca
Compare
|
A Storybook has been deployed to preview UI for the latest push |
| @@ -147,6 +152,9 @@ struct ConversationTurn { | |||
| // to be an event - as the events become richer, the order around text could | |||
| // be important. | |||
| array<ConversationEntryEvent>? events; | |||
|
|
|||
| mojo_base.mojom.Time last_edited_time; | |||
| array<EditEntry> edits; | |||
There was a problem hiding this comment.
Feels to me like this could be a ConversationEntryEvent instead of introducing a new property. Also potentially makes data storage simpler.
We could have a UserSubmissionEvent with a text, timestamp property.
Alternatively perhaps it's more appropriate to have an entirely new ConversationTurn for edits? That would allow:
- edited turns to have any properties / events that regular turns have
- simple data storage (in the upcoming SQL)
- potential for forking the conversation and maintaining history
This could happen via a property on ConversationTurn:
array<ConversationTurn> edits;
What do you think @yrliou?
There was a problem hiding this comment.
If we do the latter, we can definitely keep this PR to only allow a single conversation thread, but it would keep open the option to store and display multiple conversation forks, like competing products do.
There was a problem hiding this comment.
I don't feel strongly that it has to be inside the events, having it outside seems intuitive to me rather than an entry in an array of unions. I wonder why would this make data storage harder, it doesn't seem very different than putting it in the events?
For later, currently it seems unnecessary to me to have it as a ConversationTurn, like, do we really need to edit other things in the turn?
There was a problem hiding this comment.
I don't feel strongly that it has to be inside the events, having it outside seems intuitive to me rather than an entry in an array of unions. I wonder why would this make data storage harder, it doesn't seem very different than putting it in the events?
Because we are changing the structure from a simple string text property to an array of events. This has already been done for the assistant responses. And we already have storage setup for the events. Soon we'll either do the same for the user messages, or move away from ConversationTurn being the same struct for both assistant responses and user messages. If we're not going to do the below, I'm indifferent for the moment whether we keep this extra field of migrate to events. It doesn't have to be in this PR, we can do it in the storage PR.
For later, currently it seems unnecessary to me to have it as a ConversationTurn, like, do we really need to edit other things in the turn?
Yes ConversationTurn is already a simple struct that has all the fields we might want to edit - including action_type and text. I can definitely see the user wanting to change action types when not getting the desired response. Seems better than duplicating fields in a separate struct for an Edit. And with ConversationTurn objects for edits, we can very simply construct the thread for each edit with a Parent / Child relationship if in the database each ConversationTurn maintains a ParentConversationTurnId field. This schema would prevent us having to modify the database schema when we want to preserve the pre-edit history and not delete it.
Can we try it here? I don't see that it will change anything too drastic?
If we want to support threading but keep the new field then we'll probably add a new ConversationTurnEdits table in the database, and maintain a relationship to ConversationTurn (aka ConversationEntry).
There was a problem hiding this comment.
I wasn't sure if we want to clone a complete turn struct and preferred that we only copy needed things, but I'm okay with it, I'm changing to the array edits now.
|
A Storybook has been deployed to preview UI for the latest push |
|
I noticed that the edit button is not working when there is an associated action with the message. I have asked at brave/brave-browser#35342 (comment) whether this should be editable. If it shouldn't then how about hiding the edit button from these messages? |
yrliou
force-pushed
the
ai_chat_prompt_edit
branch
from
c010812 to
9c465a2
Compare
|
A Storybook has been deployed to preview UI for the latest push |
| @@ -264,6 +267,7 @@ interface PageHandler { | |||
| ClosePanel(); | |||
| GetActionMenuList() => (array<ActionGroup> action_list); | |||
| OpenModelSupportUrl(); | |||
| ModifyConversation(uint32 turn_index, string new_text); | |||
There was a problem hiding this comment.
nit: ModifyConversationEntry seems more intuitive?
| } | ||
|
|
||
| std::string sanitized_input = new_text; | ||
| engine_->SanitizeInput(sanitized_input); |
There was a problem hiding this comment.
SanitizeInput is already called by SubmitHumanConversationEntry. Do you think we need to call it twice for the purpose of being able to ignore it if it matches the existing turn text?
There was a problem hiding this comment.
Intentionally called here so we can check here, I've added a check to avoid another sanitization happening in 26769e4.
|
|
||
| // Modifying human turn, drop anything after this turn_index and resubmit. | ||
| auto new_turn = std::move(chat_history_.at(turn_index)); | ||
| chat_history_.erase(chat_history_.begin() + turn_index, chat_history_.end()); |
There was a problem hiding this comment.
Since we're modifying chat_history_ then technically shouldn't we let the observers know? Or would you rather wait for the edited turn to appear in the history after calling SubmitHumanConversationEntry?
There was a problem hiding this comment.
Originally I didn't do so since it would be called in SubmitHumanConversationEntry, but I'm fine having another one here after we clear it. Added in 26769e4.
| @@ -147,6 +147,9 @@ struct ConversationTurn { | |||
| // to be an event - as the events become richer, the order around text could | |||
| // be important. | |||
| array<ConversationEntryEvent>? events; | |||
|
|
|||
| mojo_base.mojom.Time last_edited_time; | |||
There was a problem hiding this comment.
We might end up naming this time_created or something like that, so that we can order ConversationTurn and edits in the database, and get the last edited time by looking at the items in edits. Not saying you should change this in this PR, just saying in case you have a different opinion.
Example to illustrate the ergonomics:
{
text: 'Will an LTT store backpack fit in a Tesla Model Y frunk?',
characterType: mojom.CharacterType.HUMAN,
actionType: mojom.ActionType.SHORTEN,
visibility: mojom.ConversationTurnVisibility.VISIBLE,
selectedText: '',
edits: [{
text: 'Will a lux LTT store backpack fit in a 2024 Tesla Model Y frunk?',
characterType: mojom.CharacterType.HUMAN,
actionType: mojom.ActionType.SHORTEN,
visibility: mojom.ConversationTurnVisibility.VISIBLE,
selectedText: '',
time_created: { internalValue: BigInt('13278618001000000') },
edits: [],
events: []
}],
time_created: { internalValue: BigInt('13278618000999999') },
events: []
},
{Then we don't have to worry about modifying times, etc - we just look at each original or edit and know the time it was created.
There was a problem hiding this comment.
Rename to created_time in 26769e4
| turn->edits.push_back(turn.Clone()); | ||
| turn->text = sanitized_input; | ||
| turn->last_edited_time = base::Time::Now(); |
There was a problem hiding this comment.
If you edit an already edited ConversationTurn, won't this mean that we'll end up with a tree of edits instead of a flat list? It seems to me that it's simpler to put all the edits on the original turn.
Cloning a turn that already has edits and pushing that to the edits array seems like we'll end up with:
- Turn1
- Turn2
- EditedTurn1
- EditedTurn2
- EditedTurn1
- EditedTurn2
I'm fine if you want to reduce the modifications by modifying turn.text as you are doing here, and have the original as an "edit" (see below - [1]). But I do think we should at least just have a single array of edits on a ConversationTurn and not populate any edits on the edited entries, i.e. (if you continue putting the old version in edits and make the new version the root turn):
auto previous_version = turn.Clone();
previous_version.edits.clear(); // or ideally previous_version.edits = std::nullopt
turn->edits.push_back(std::move(previous_version));
turn->text = sanitized_input;
turn->last_edited_time = base::Time::Now();Ending up with:
- Turn1
- Turn2
- EditedTurn1
- EditedTurn2
[1] That seems counterintuitive compared to the name of the property: "edit". I would originally have thought that we'd keep the original text at turn.text and simply have the edited versions on turn.edits. Not doing that does mean that you don't need to change SubmitHumanConversationEntry, or the rendering in WebUI and iOS. However, those changes would be simple since the edit is still a ConversationTurn (either the render function uses the given ConversationTurn or it uses the latest ConversationTurn from turn.edits. Again, I'm fine if you don't want to make that change in this PR for whatever reason since it supports the feature in this basic form pre-storage and before we support keeping the history of edits.
There was a problem hiding this comment.
Thanks, I've updated to put edits in the edits array without cloning edits array in the original turn, and keep original turn in the root in 26769e4.
| EXPECT_NE(conversation_history[0]->last_edited_time, last_edited_time2); | ||
| ASSERT_EQ(conversation_history[0]->edits.size(), 2u); | ||
| EXPECT_EQ(conversation_history[0]->edits.at(0)->text, "prompt1"); | ||
| EXPECT_EQ(conversation_history[0]->edits.at(0)->last_edited_time, |
There was a problem hiding this comment.
should assert the edits don't have any edits
| import * as React from 'react' | ||
| import Icon from '@brave/leo/react/icon' | ||
| import styles from './style.module.scss' | ||
| import Button from '@brave/leo/react/button' |
There was a problem hiding this comment.
please put external import above relative file path import
| return ( | ||
| <div className={styles.editIndicator}> | ||
| <Icon name='edit-pencil' /> | ||
| <span className={styles.editedText}>Edited</span> |
There was a problem hiding this comment.
This word needs to be localized
| <Button onClick={props.onClick} className={styles.editButton} | ||
| fab | ||
| size='tiny' | ||
| kind='plain-faint' |
There was a problem hiding this comment.
We need a title attribute here, for a tooltip and accessibility
| className={styles.growWrap} | ||
| data-replicated-value={text} | ||
| > | ||
| <textarea |
There was a problem hiding this comment.
Since we don't have a label for this, should we at least have an aria-placeholder attribute. Something like: "Edited message"? Or because we have the existing message content in the field, we don't need it?
There was a problem hiding this comment.
I didn't put a placeholder as we would have the existing message content here.
| @@ -71,7 +71,7 @@ function DataContextProvider(props: DataContextProviderProps) { | |||
| const [allModels, setAllModels] = React.useState<mojom.Model[]>([]) | |||
| const [conversationHistory, setConversationHistory] = React.useState<mojom.ConversationTurn[]>([]) | |||
| const [suggestedQuestions, setSuggestedQuestions] = React.useState<string[]>([]) | |||
| const [isGenerating, setIsGenerating] = React.useState(false) | |||
| const [isGenerating, setIsGenerating] = React.useState(props.store?.isGenerating || false) | |||
There was a problem hiding this comment.
It seems like an existing bug here to me when I was working on this commit.
yrliou
force-pushed
the
ai_chat_prompt_edit
branch
from
39030ce to
0670c33
Compare
| @@ -203,6 +203,7 @@ function ConversationList(props: ConversationListProps) { | |||
| text={latestTurnText} | |||
| onSubmit={(text) => handleEditSubmit(id, text)} | |||
| onCancel={() => setEditInputId(null)} | |||
| isSubmitDisabled={shouldDisableUserInput} | |||
There was a problem hiding this comment.
Currently we don't expect to handle another request when answer is still generating, so disable the submission here.
For reference, ChatGPT UI also allows to click on edit button and disables the submit edit button when the answer is generating.
|
A Storybook has been deployed to preview UI for the latest push |
|
|
||
| .time { | ||
| color: var(--leo-color-text-tertiary); | ||
| text-decoration: underline; |
There was a problem hiding this comment.
I'm wondering if we should wait to make it appear interactive until we implement this part of the feature?
| function EditInput(props: Props) { | ||
| const [text, setText] = React.useState(props.text) | ||
| const handleKeyDown = (e: React.KeyboardEvent<HTMLTextAreaElement>) => { | ||
| if (e.key === 'Enter' && !e.shiftKey && !e.nativeEvent.isComposing) { |
There was a problem hiding this comment.
I think this handler needs to check props.isSubmitDisabled
There was a problem hiding this comment.
Added in the final squashed commit.
yrliou
force-pushed
the
ai_chat_prompt_edit
branch
from
0670c33 to
e012971
Compare
|
[puLL-Merge] - brave/brave-core@24558 DescriptionThis PR introduces the ability to edit human conversation turns in the AI chat feature. It adds new UI components for editing, displaying edit indicators, and handling the editing process. The backend has been updated to support modifying conversations and storing edit history. ChangesChanges
Possible Issues
Security Hotspots
|
yrliou
force-pushed
the
ai_chat_prompt_edit
branch
from
e012971 to
c24b05a
Compare
|
A Storybook has been deployed to preview UI for the latest push |
Resolves brave/brave-browser#35342
Design link: https://www.figma.com/design/m0Gdbf0wtqyfEFGm32VLLc/%F0%9F%94%84-Leo-%5BIN-PROGRESS%5D?node-id=4604-59626&t=JIZyytWeZVk4jjMw-4
Note that the edit history part is not implemented yet by this PR.
Editing:
Edited:
Submitter Checklist:
QA/YesorQA/No;release-notes/includeorrelease-notes/exclude;OS/...) to the associated issuenpm run test -- brave_browser_tests,npm run test -- brave_unit_testswikinpm run presubmitwiki,npm run gn_check,npm run tslintgit rebase master(if needed)Reviewer Checklist:
gnAfter-merge Checklist:
changes has landed on
Test Plan:
Edit a user prompt in Leo side panel, it should remove entries after it and get a new answer.