[AI Chat] fix issues with conversation context menu #26990
+280
−229
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,7 @@ | |
|
|
||
| #include <stddef.h> | ||
|
|
||
| #include <functional> | ||
| #include <map> | ||
| #include <memory> | ||
| #include <ostream> | ||
|
|
@@ -93,8 +94,8 @@ class AIChatService : public KeyedService, | |
| void OnConversationEntryRemoved(ConversationHandler* handler, | ||
| std::string entry_uuid) override; | ||
| void OnClientConnectionChanged(ConversationHandler* handler) override; | ||
| void OnConversationTitleChanged(const std::string& conversation_uuid, | ||
| const std::string& title) override; | ||
| void OnAssociatedContentDestroyed(ConversationHandler* handler, | ||
| int content_id) override; | ||
|
|
||
|
|
@@ -175,7 +176,8 @@ class AIChatService : public KeyedService, | |
|
|
||
| private: | ||
| // Key is uuid | ||
| using ConversationMap = | ||
| std::map<std::string, mojom::ConversationPtr, std::less<>>; | ||
fallaciousreasoning marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| using ConversationMapCallback = base::OnceCallback<void(ConversationMap&)>; | ||
|
|
||
| void MaybeInitStorage(); | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1683,9 +1683,9 @@ void ConversationHandler::OnClientConnectionChanged() { | |
| } | ||
| } | ||
|
|
||
| void ConversationHandler::OnConversationTitleChanged(std::string_view title) { | ||
| for (auto& observer : observers_) { | ||
| observer.OnConversationTitleChanged(metadata_->uuid, std::string(title)); | ||
| } | ||
| } | ||
|
|
||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -166,8 +166,9 @@ class ConversationHandler : public mojom::ConversationHandler, | |
|
|
||
| // Called when a mojo client connects or disconnects | ||
| virtual void OnClientConnectionChanged(ConversationHandler* handler) {} | ||
| virtual void OnConversationTitleChanged( | ||
| const std::string& conversation_uuid, | ||
| const std::string& title) {} | ||
| virtual void OnSelectedLanguageChanged( | ||
| ConversationHandler* handler, | ||
| const std::string& selected_language) {} | ||
|
|
@@ -407,7 +408,7 @@ class ConversationHandler : public mojom::ConversationHandler, | |
| void OnSuggestedQuestionsChanged(); | ||
| void OnAssociatedContentInfoChanged(); | ||
| void OnClientConnectionChanged(); | ||
| void OnConversationTitleChanged(std::string_view title); | ||
| void OnConversationUIConnectionChanged(mojo::RemoteSetElementId id); | ||
| void OnSelectedLanguageChanged(const std::string& selected_language); | ||
| void OnAssociatedContentFaviconImageDataChanged(); | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,24 +25,24 @@ class MockConversationHandlerObserver : public ConversationHandler::Observer { | |
|
|
||
| MOCK_METHOD(void, | ||
| OnRequestInProgressChanged, | ||
| (ConversationHandler*, bool), | ||
| (override)); | ||
|
|
||
| MOCK_METHOD(void, | ||
| OnConversationEntryAdded, | ||
| (ConversationHandler*, | ||
| mojom::ConversationTurnPtr&, | ||
| std::optional<std::string_view>), | ||
| (override)); | ||
|
|
||
| MOCK_METHOD(void, | ||
| OnConversationEntryRemoved, | ||
| (ConversationHandler*, std::string), | ||
| (override)); | ||
|
|
||
| MOCK_METHOD(void, | ||
| OnConversationEntryUpdated, | ||
| (ConversationHandler*, mojom::ConversationTurnPtr), | ||
| (override)); | ||
|
|
||
| MOCK_METHOD(void, | ||
|
|
@@ -52,7 +52,7 @@ class MockConversationHandlerObserver : public ConversationHandler::Observer { | |
|
|
||
| MOCK_METHOD(void, | ||
| OnConversationTitleChanged, | ||
| (const std::string&, const std::string&), | ||
| (override)); | ||
|
|
||
| private: | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -62,46 +62,48 @@ interface DisplayTitleProps { | |
| } | ||
|
|
||
| function DisplayTitle(props: DisplayTitleProps) { | ||
| const [isOptionsMenuOpen, setIsOptionsMenuOpen] = React.useState(false) | ||
|
|
||
| const handleButtonMenuChange = React.useCallback((e: {isOpen: boolean}) => { | ||
|
There was a problem hiding this comment. optional: with stuff like this where you're at a leaf node, I think its okay to not wrap things in a Up to you on this one There was a problem hiding this comment. I just wasn't sure if our web component would cause a new listener to be added (and the old one removed) on every single render. But I suppose we're doing that everywhere anyway There was a problem hiding this comment. It will 😆 - but its the same with every other component that does this so 🤷 And with React compiler it will (probably) Just Work (tm) |
||
| setIsOptionsMenuOpen(e.isOpen) | ||
| }, []) | ||
|
|
||
| return ( | ||
| <div | ||
| className={classnames(styles.displayTitle, isOptionsMenuOpen && styles.isOptionsMenuOpen)} | ||
|
There was a problem hiding this comment. this is a handy util taher added! |
||
| > | ||
| <div className={styles.displayTitleContent}> | ||
| <div | ||
| className={styles.text} | ||
| title={props.title} | ||
| > | ||
| {props.title} | ||
| </div> | ||
| <div className={styles.description}>{props.description}</div> | ||
| </div> | ||
| <ButtonMenu | ||
| className={styles.optionsMenu} | ||
| onChange={handleButtonMenuChange} | ||
| > | ||
| <div | ||
| slot='anchor-content' | ||
| className={styles.optionsButton} | ||
| > | ||
| <Icon name='more-vertical' /> | ||
| </div> | ||
| <leo-menu-item onClick={props.onEditTitle}> | ||
| <div className={styles.optionsMenuItemWithIcon}> | ||
| <Icon name='edit-pencil' /> | ||
| <div>{getLocale('menuRenameConversation')}</div> | ||
| </div> | ||
| </leo-menu-item> | ||
| <leo-menu-item onClick={props.onDelete}> | ||
| <div className={styles.optionsMenuItemWithIcon}> | ||
| <Icon name='trash' /> | ||
| <div>{getLocale('menuDeleteConversation')}</div> | ||
| </div> | ||
| </leo-menu-item> | ||
| </ButtonMenu> | ||
| </div> | ||
| ) | ||
| } | ||
|
|
@@ -140,19 +142,30 @@ export default function ConversationsList(props: ConversationsListProps) { | |
| {aiChatContext.visibleConversations.length > 0 && | ||
| <ol> | ||
| {aiChatContext.visibleConversations.map(item => { | ||
| const isEditing = aiChatContext.editingConversationId === item.uuid | ||
| return ( | ||
| <li key={item.uuid}> | ||
| <a | ||
| className={classnames({ | ||
| [styles.navItem]: true, | ||
| [styles.navItemActive]: item.uuid === conversationContext.conversationUuid | ||
| })} | ||
| onClick={(e) => { | ||
| if (isEditing) { | ||
| e.preventDefault() | ||
| } | ||
| props.setIsConversationsListOpen?.(false) | ||
| }} | ||
| onDoubleClick={() => aiChatContext.setEditingConversationId(item.uuid)} | ||
| href={`/${item.uuid}`} | ||
| > | ||
| <DisplayTitle | ||
| title={item.title || getLocale('conversationListUntitled')} | ||
| description='' | ||
| onEditTitle={() => aiChatContext.setEditingConversationId(item.uuid)} | ||
| onDelete={() => getAPI().service.deleteConversation(item.uuid)} | ||
| /> | ||
| {item.uuid === aiChatContext.editingConversationId && ( | ||
| <div className={styles.editibleTitle}> | ||
| <SimpleInput | ||
| text={item.title} | ||
|
|
@@ -163,13 +176,6 @@ export default function ConversationsList(props: ConversationsListProps) { | |
| }} | ||
| /> | ||
| </div> | ||
| )} | ||
| </a> | ||
| </li> | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,6 +28,7 @@ | |
|
|
||
| .navItem { | ||
| all: unset; | ||
| position: relative; | ||
| border-radius: var(--leo-radius-m); | ||
| display: block; | ||
| font: var(--leo-font-default-regular); | ||
|
|
@@ -65,6 +66,10 @@ | |
| transition: all 0.2s ease-out allow-discrete; | ||
| width: 100%; | ||
|
|
||
| &:not(.isOptionsMenuOpen,:hover) .optionsMenu { | ||
| visibility: hidden; | ||
| } | ||
|
|
||
| &:hover { | ||
| background: var(--leo-color-container-highlight); | ||
| box-shadow: var(--leo-effect-elevation-01); | ||
|
|
@@ -92,9 +97,14 @@ | |
| } | ||
|
|
||
| .editibleTitle { | ||
| // Take up the full size of the parent | ||
| position: absolute; | ||
| top: 0; | ||
| bottom: 0; | ||
| left: 0; | ||
| right: 0; | ||
| display: flex; | ||
| align-items: center; | ||
|
|
||
| form, | ||
| input { | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -59,6 +59,9 @@ | |
| // Ensure navigation content always stays at full-width even when animation | ||
| // is decreasing the width of the parent | ||
| width: var(--navigation-width); | ||
| // Ensure fills the height so that there's enough space for the conversation | ||
| // item context menu | ||
| height: -webkit-fill-available; | ||
| } | ||
|
|
||
| .left { | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this just take a
std::stringso we can move thenew_titleinto this function?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as discussed in the slack thread, doesn't seem like it's worth the optimization