[AI Chat] fix issues with conversation context menu #26990

petemill · 2024-12-12T01:03:46Z

Menu open too small and has scroll inside when there's only 1 conversation list item
Opening a menu when hovering causes the list item to be clicked (the link to be followed)
Editing the title was impossible because the action was cancelled when navigating

Blocked on:

Resolves

Fix Can't delete first chat when using full page Leo brave-browser#42829

Submitter Checklist:

I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally:
- npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
- npm run presubmit wiki, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on
All relevant documentation has been updated, for instance:

Test Plan:

fallaciousreasoning

lgtm assuming Nala bump is separated

fallaciousreasoning · 2024-12-12T01:42:48Z

components/ai_chat/core/browser/ai_chat_service.cc

-  conversation->title = title;
+void AIChatService::OnConversationTitleChanged(
+    const std::string& conversation_uuid,
+    const std::string& new_title) {


should this just take a std::string so we can move the new_title into this function?

as discussed in the slack thread, doesn't seem like it's worth the optimization

fallaciousreasoning · 2024-12-12T01:43:16Z

components/ai_chat/core/browser/ai_chat_service.cc

+  auto& conversation_metadata = conversation_it->second;
+  conversation_metadata->title = new_title;

  OnConversationListChanged();

  // Persist the change
  if (ai_chat_db_) {
    ai_chat_db_
        .AsyncCall(base::IgnoreResult(&AIChatDatabase::UpdateConversationTitle))
-        .WithArgs(handler->get_conversation_uuid(), std::move(title));
+        .WithArgs(conversation_uuid, new_title);


if we just take a std::string:

Suggested change

auto& conversation_metadata = conversation_it->second;

conversation_metadata->title = new_title;

OnConversationListChanged();

// Persist the change

if (ai_chat_db_) {

ai_chat_db_

.AsyncCall(base::IgnoreResult(&AIChatDatabase::UpdateConversationTitle))

.WithArgs(handler->get_conversation_uuid(), std::move(title));

.WithArgs(conversation_uuid, new_title);

auto& conversation_metadata = conversation_it->second;

conversation_metadata->title = std::move(new_title);

OnConversationListChanged();

// Persist the change

if (ai_chat_db_) {

ai_chat_db_

.AsyncCall(base::IgnoreResult(&AIChatDatabase::UpdateConversationTitle))

.WithArgs(conversation_uuid, conversation_metadata->title);

components/ai_chat/core/browser/ai_chat_service.h

fallaciousreasoning · 2024-12-12T01:46:29Z

components/ai_chat/core/browser/conversation_handler.cc

@@ -1685,7 +1685,7 @@ void ConversationHandler::OnClientConnectionChanged() {

 void ConversationHandler::OnConversationTitleChanged(std::string title) {


could this take a const std::string& or a string_view? It's not doing anything async

fallaciousreasoning · 2024-12-12T01:46:44Z

components/ai_chat/core/browser/conversation_handler.cc

@@ -1685,7 +1685,7 @@ void ConversationHandler::OnClientConnectionChanged() {

 void ConversationHandler::OnConversationTitleChanged(std::string title) {
  for (auto& observer : observers_) {
-    observer.OnConversationTitleChanged(this, title);
+    observer.OnConversationTitleChanged(metadata_->uuid, title);


this one is a const std::string&

fallaciousreasoning · 2024-12-12T01:48:50Z

components/ai_chat/resources/page/components/conversations_list/index.tsx

-  const [isButtonMenuVisible, setIsButtonMenuVisible] = React.useState(false)
+  const [isOptionsMenuOpen, setIsOptionsMenuOpen] = React.useState(false)
+
+  const handleButtonMenuChange = React.useCallback((e: {isOpen: boolean}) => {


optional: with stuff like this where you're at a leaf node, I think its okay to not wrap things in a useCallback (or do it inline)

Up to you on this one

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

It will 😆 - but its the same with every other component that does this so 🤷

And with React compiler it will (probably) Just Work (tm)

fallaciousreasoning · 2024-12-12T01:49:04Z

components/ai_chat/resources/page/components/conversations_list/index.tsx

-      className={styles.displayTitle}
-      onMouseEnter={() => setIsButtonMenuVisible(true)}
-      onMouseLeave={() => setIsButtonMenuVisible(false)}
+      className={classnames(styles.displayTitle, isOptionsMenuOpen && styles.isOptionsMenuOpen)}


this is a handy util taher added!

fallaciousreasoning · 2024-12-12T01:51:51Z

...nents/ai_chat/resources/untrusted_conversation_frame/components/assistant_response/index.tsx

@@ -28,11 +28,11 @@ function SearchSummary (props: { searchQueries: string[] }) {
  const message = formatMessage(getLocale('searchQueries'), {
    placeholders: {
      $1: props.searchQueries.map((query, i, a) => (
-        <>
+        <React.Fragment key={i}>


👍 you're a hero - these errors drive me nuts

fallaciousreasoning · 2024-12-12T01:57:21Z

...nents/ai_chat/resources/untrusted_conversation_frame/components/assistant_response/index.tsx

+  // Unknown events should be ignored
+  return null
+}
+
 export default function AssistantResponse(props: { entry: Mojom.ConversationTurn, isEntryInProgress: boolean }) {
  const searchQueriesEvent = props.entry.events?.find(event => !!event.searchQueriesEvent)?.searchQueriesEvent


superfluous !!

Suggested change

const searchQueriesEvent = props.entry.events?.find(event => !!event.searchQueriesEvent)?.searchQueriesEvent

const searchQueriesEvent = props.entry.events?.find(event => event.searchQueriesEvent)?.searchQueriesEvent

fallaciousreasoning · 2024-12-12T01:59:02Z

...nents/ai_chat/resources/untrusted_conversation_frame/components/assistant_response/index.tsx

 export default function AssistantResponse(props: { entry: Mojom.ConversationTurn, isEntryInProgress: boolean }) {
  const searchQueriesEvent = props.entry.events?.find(event => !!event.searchQueriesEvent)?.searchQueriesEvent
-  const hasCompletionStarted = !props.isEntryInProgress || props.entry.events?.find(event => !!event.completionEvent)
+  const hasCompletionStarted = !props.isEntryInProgress ||
+    (props.entry.events?.some(event => !!event.completionEvent) ?? false)


superfluous !!

Suggested change

(props.entry.events?.some(event => !!event.completionEvent) ?? false)

(props.entry.events?.some(event => event.completionEvent) ?? false)

optional:

Suggested change

(props.entry.events?.some(event => !!event.completionEvent) ?? false)

const hasCompletionStarted = props.isEntryInProgress

|| !!props.entry.events?.some(e => e.completionEvent)

I've been studying this for a good few minutes and I still can't quickly reason with the names here, or how your suggestion is equivalent to what's there at the moment. Not introduced in this PR but I think we can improve it at some point.

petemill · 2024-12-12T05:53:29Z

~~Ignore this, it needs a rebase~~

brave-builds · 2024-12-12T23:27:06Z

A Storybook has been deployed to preview UI for the latest push

- ButtonMenu can open without being tiny and scrolling - make sure edit isn't blocked by the link activation - storybook get edit ability

and log database title because of historical bad values due to ref expiring

storybook context can use react hooks

petemill requested a review from fallaciousreasoning December 12, 2024 01:03

petemill self-assigned this Dec 12, 2024

petemill requested a review from a team as a code owner December 12, 2024 01:03

github-actions bot added CI/run-audit-deps Check for known npm/cargo vulnerabilities (audit_deps) CI/storybook-url Deploy storybook and provide a unique URL for each build labels Dec 12, 2024

petemill force-pushed the ai-chat-menu-fixes branch from 16e68d6 to 78b50d7 Compare December 12, 2024 01:05

fallaciousreasoning approved these changes Dec 12, 2024

View reviewed changes

petemill force-pushed the ai-chat-untrusted-frame branch from ce3b00c to 84b6868 Compare December 12, 2024 03:52

petemill requested review from a team and bridiver as code owners December 12, 2024 03:52

petemill force-pushed the ai-chat-menu-fixes branch from 78b50d7 to 4004682 Compare December 12, 2024 06:16

petemill removed request for a team and bridiver December 12, 2024 06:16

Base automatically changed from ai-chat-untrusted-frame to master December 12, 2024 06:42

petemill force-pushed the ai-chat-menu-fixes branch 4 times, most recently from 8cd1e4c to f086917 Compare December 12, 2024 22:39

petemill force-pushed the ai-chat-menu-fixes branch 2 times, most recently from 1e12113 to 0f6e891 Compare December 13, 2024 06:05

petemill enabled auto-merge December 13, 2024 06:14

AssistantResponse uses key for events

a3df109

petemill disabled auto-merge December 13, 2024 08:43

petemill added 3 commits December 13, 2024 01:14

AI Chat conversations list item menu and edit improvements

0c5cf4b

- ButtonMenu can open without being tiny and scrolling - make sure edit isn't blocked by the link activation - storybook get edit ability

Don't crash when editing the title of a conversation not in-memory

993d18f

and log database title because of historical bad values due to ref expiring

AI Chat storybook menu open responsiveness matches webui

dd1e08f

storybook context can use react hooks

petemill force-pushed the ai-chat-menu-fixes branch from 0f6e891 to dd1e08f Compare December 13, 2024 09:16

petemill enabled auto-merge December 13, 2024 09:17

petemill disabled auto-merge December 13, 2024 09:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AI Chat] fix issues with conversation context menu #26990

[AI Chat] fix issues with conversation context menu #26990

petemill commented Dec 12, 2024 •

edited

Loading

fallaciousreasoning left a comment

fallaciousreasoning Dec 12, 2024

petemill Dec 13, 2024

fallaciousreasoning Dec 12, 2024

fallaciousreasoning Dec 12, 2024

fallaciousreasoning Dec 12, 2024

fallaciousreasoning Dec 12, 2024

petemill Dec 12, 2024

fallaciousreasoning Dec 12, 2024 •

edited

Loading

fallaciousreasoning Dec 12, 2024

fallaciousreasoning Dec 12, 2024

fallaciousreasoning Dec 12, 2024

fallaciousreasoning Dec 12, 2024

petemill Dec 13, 2024

petemill commented Dec 12, 2024 •

edited

Loading

brave-builds commented Dec 12, 2024

		@@ -1685,7 +1685,7 @@ void ConversationHandler::OnClientConnectionChanged() {

		void ConversationHandler::OnConversationTitleChanged(std::string title) {

	const searchQueriesEvent = props.entry.events?.find(event => !!event.searchQueriesEvent)?.searchQueriesEvent
	const searchQueriesEvent = props.entry.events?.find(event => event.searchQueriesEvent)?.searchQueriesEvent

	(props.entry.events?.some(event => !!event.completionEvent) ?? false)
	(props.entry.events?.some(event => event.completionEvent) ?? false)

	(props.entry.events?.some(event => !!event.completionEvent) ?? false)
	const hasCompletionStarted = props.isEntryInProgress
	\|\| !!props.entry.events?.some(e => e.completionEvent)

[AI Chat] fix issues with conversation context menu #26990

Are you sure you want to change the base?

[AI Chat] fix issues with conversation context menu #26990

Conversation

petemill commented Dec 12, 2024 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

fallaciousreasoning 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

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

fallaciousreasoning Dec 12, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petemill commented Dec 12, 2024 • edited Loading

brave-builds commented Dec 12, 2024

petemill commented Dec 12, 2024 •

edited

Loading

fallaciousreasoning Dec 12, 2024 •

edited

Loading

petemill commented Dec 12, 2024 •

edited

Loading