AIChat refactor to support standalone and persistent conversations #24921

petemill · 2024-07-30T16:40:02Z

This PR refactors the ai_chat component on the browser-side and in the UI to allow for

Multiple conversations
Conversations to be independent of content (WebContents/Tab)

This is achieved with the following architecture (more details at brave/brave-browser#31623):

AIChatService owns all the conversation metadata
ConversationHandler owns the data for conversations (chat messages, suggested questions) and performs all the operations via the EngineConsumer (which is unchanged).
Content (title, url, page content) is attached to a conversation via an AssociatedContentDelegate. This is implemented by AssociatedContentDriver (and derives AIChatTabHelper for WebUI and an iOS equivalent). When a conversation is related to some content, one of these is attached. Standalone conversations do not have associated content and therefore this delegate is null.

AIChatService -> ConversationHandler -> AssociatedContentDelegate [AssociatedContentDriver [AIChatTabHelper]]

Conversations which had associated content, but the content is not live anymore (i.e. the Tab was navigated away or closed), receive an AssociatedArchiveContent, which keeps the url and last title and page content so that the conversation can be continued.

Future TODO (other PRs)

Connect the storage from Added AIChat storage #19503
Connect the UI from AIChat: Fullscreen UI #24633
iOS "other conversations" UI
android "other conversations" UI

Resolves brave/brave-browser#31623

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:

browser/ui/ai_chat/print_preview_extractor.cc

bridiver · 2024-08-08T00:14:27Z

browser/ai_chat/ai_chat_service_factory.cc

+std::unique_ptr<KeyedService>
+AIChatServiceFactory::BuildServiceInstanceForBrowserContext(
+    content::BrowserContext* context) const {
+  auto skus_service_getter = base::BindRepeating(


whys is this passing a getter instead of the skus service itself?

This code is just moved. We should fix that, but not here.

can we create follow-up issues to track this stuff?

bridiver · 2024-08-08T00:16:05Z

browser/ai_chat/ai_chat_service_factory.cc

+
+content::BrowserContext* AIChatServiceFactory::GetBrowserContextToUse(
+    content::BrowserContext* context) const {
+  if (!Profile::FromBrowserContext(context)->IsRegularProfile() ||


use ProfileSelections::Builder().WithRegular(ProfileSelection::kOwnInstance)

I just merged another PR which adds https://github.com/brave/brave-core/blob/master/browser/ai_chat/ai_chat_utils.cc#L15-L20:

bool IsAllowedForContext(content::BrowserContext* context, bool check_policy /*=true*/) { Profile* profile = Profile::FromBrowserContext(context); return profile->IsRegularProfile() && ai_chat::features::IsAIChatEnabled() && (!check_policy || IsAIChatEnabled(profile->GetPrefs())); }

That's fine to stay that way? I was going to update here to use this new util, but we'd rather use the ProfileSelections path instead?

I think use ProfileSelections in factory constructor to exclude specifically checking regular profile here and then we still need to use/update ai_chat::IsAllowedForContext for other checks so we would have consistent checking among different places.

shouldn't this be .WithRegular(ProfileSelection::kOriginalOnly) to make sure the service isn't built for incognito? Don't we in-fact want the default behavior of ProfileKeyedServiceFactory?

Example of a service that does not exist in OTR (default behavior)

bridiver · 2024-08-08T00:17:36Z

browser/ai_chat/ai_chat_service_factory.cc

+  return std::make_unique<AIChatService>(
+      ModelServiceFactory::GetForBrowserContext(context),
+      std::move(credential_manager), user_prefs::UserPrefs::Get(context),
+      (g_brave_browser_process->process_misc_metrics())


why the parens around this? @DJAndries have you had a change to do anything with this yet to make it less clunky to access?

Because of the ternary? formatter isn't complaining, but happy to change...

shouldn't be needed here

browser/ai_chat/ai_chat_service_factory.cc

components/ai_chat/core/browser/ai_chat_service.cc

yrliou · 2024-09-05T21:39:44Z

components/ai_chat/core/browser/conversation_handler.cc

+void ConversationHandler::OnGetStagedEntriesFromContent(
+    const std::optional<SearchQuerySummary>& search_query_summary) {
+  // Check if all requirements are still met.
+  if (!search_query_summary || !chat_history_.empty() ||


Should we also check user opt-in here?

It's being checked in MaybeFetchOrClearContentStagedConversation and there's no way currently to opt-out whilst that's happening, but I'll add the check anyway.

brave-builds · 2024-09-11T09:27:23Z

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

petemill · 2024-09-20T17:14:03Z

Everything passed CI except iOS due to unrelated test runner fix in #25669. Rebased to get that passing.

bridiver · 2024-09-20T17:41:09Z

chromium_src/chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc

@@ -60,10 +62,16 @@ void ChromeAutocompleteProviderClient::OpenLeo(const std::u16string& query) {
      browser->tab_strip_model()->GetActiveWebContents());
  DCHECK(chat_tab_helper);

+  auto* conversation_handler =
+      ai_chat::AIChatServiceFactory::GetForBrowserContext(profile_)


don't we need a null check here before calling GetOrCreateConversationHandlerForContent?

It was covered by

CHECK(profile_->IsRegularProfile());

and the feature not being added to the omnibox suggestions in incognito profiles, but I agree it's better to check the service - added in 5f89085

bridiver · 2024-09-20T17:42:31Z

chromium_src/chrome/browser/renderer_context_menu/render_view_context_menu.cc

+  } else {
+    auto* browser = GetBrowser();
+    if (!browser) {
+      VLOG(1) << "Can't get browser";


this kind of logging should be removed before merge

this is not new

darkdh

ai-chat cpp side looks good to me. Unit tests for new codes and moved codes are nicely done with great coverage!

darkdh · 2024-09-20T17:39:53Z

components/ai_chat/core/browser/ai_chat_service_unittest.cc

+      conversation_observations_{this};
+};
+
+class MockAIChatCredentialManager : public AIChatCredentialManager {


As discussed, we will do this in followup.

darkdh · 2024-09-20T17:40:41Z

components/ai_chat/core/browser/conversation_handler_unittest.cc

+class ConversationHandlerUnitTest_OptedOut
+    : public ConversationHandlerUnitTest {
+ public:
+  ConversationHandlerUnitTest_OptedOut() { is_opted_in_ = false; }
+};
+
+class ConversationHandlerUnitTest_NoAssociatedContent
+    : public ConversationHandlerUnitTest {
+ public:
+  ConversationHandlerUnitTest_NoAssociatedContent() {
+    has_associated_content_ = false;
+  }
+};


As discussed, we will do this in followup.

bridiver · 2024-09-20T17:45:35Z

chromium_src/chrome/browser/renderer_context_menu/render_view_context_menu.cc

+  auto selected_text = base::UTF16ToUTF8(params_.selection_text);
+
+  if (rewrite_in_place) {
+    source_web_contents_->SetUserData(kAIChatRewriteDataKey,


seems like this would be better handled inside the tab helper so it gets cleared when the page navigates? I think we have way too much logic for aichat embedded in RenderViewContextMenu here that should be encapsulated within aichat

also are there tests for this somewhere? I'm not seeing anything, but searching github PRs sucks so I could be missing something

nvm, I found them

Sure but this is not added in this PR cc @yrliou

can we create follow-up issues to track this stuff?

brave/brave-browser#41171

brave-builds · 2024-09-20T17:52:02Z

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

yrliou

My only outstanding comment is the issue of putting ai_chat_service_factory in browser target, I'll do an immediate follow-up to fix it.

petemill · 2024-09-20T18:28:21Z

I made this issue to track findings of future code improvements brave/brave-browser#41171

petemill force-pushed the aichat-services branch 2 times, most recently from 904d9c5 to 90e5b70 Compare July 30, 2024 22:23

petemill self-assigned this Aug 4, 2024

petemill force-pushed the aichat-services branch 4 times, most recently from 4f4d6a2 to d7f1476 Compare August 6, 2024 22:34

darkdh reviewed Aug 7, 2024

View reviewed changes

browser/ui/ai_chat/print_preview_extractor.cc Outdated Show resolved Hide resolved

darkdh reviewed Aug 7, 2024

View reviewed changes

browser/ui/ai_chat/print_preview_extractor.cc Outdated Show resolved Hide resolved

petemill force-pushed the aichat-services branch from d7f1476 to 6f984db Compare August 7, 2024 22:21

bridiver reviewed Aug 8, 2024

View reviewed changes

browser/ai_chat/ai_chat_service_factory.cc Outdated Show resolved Hide resolved

nullhook reviewed Aug 12, 2024

View reviewed changes

components/ai_chat/core/browser/ai_chat_service.cc Show resolved Hide resolved

petemill force-pushed the aichat-services branch from 47485af to 9c8b53d Compare August 27, 2024 05:55

github-actions bot added the CI/storybook-url Deploy storybook and provide a unique URL for each build label Aug 27, 2024

petemill force-pushed the aichat-services branch 2 times, most recently from 1e5ceda to a8143f6 Compare August 31, 2024 04:42

yrliou reviewed Sep 5, 2024

View reviewed changes

petemill force-pushed the aichat-services branch 7 times, most recently from c041d30 to e8ce0d2 Compare September 11, 2024 08:14

petemill marked this pull request as ready for review September 11, 2024 16:23

petemill requested a review from a team as a code owner September 11, 2024 16:23

petemill added 11 commits September 20, 2024 10:13

format

288fa5d

rebase fixes

de3761e

AIChatTabHelper refine and test retry logic

67b9ff1

fix android compile

498af7a

fix android again

19f155c

no channel_info new string

d156c64

associatedcontentdriver - remove is_page_text_fetch_in_progress_

c40131f

ConversationHandler::HasAnyHistory ignores staged entries

95eac52

AIChatService: erase from content_conversation map, and test it

c8588e9

MaybeUnlink should check if client is connected

e1bac5c

fix android again?

23b728d

petemill force-pushed the aichat-services branch from adc83de to 23b728d Compare September 20, 2024 17:14

bridiver reviewed Sep 20, 2024

View reviewed changes

darkdh approved these changes Sep 20, 2024

View reviewed changes

bridiver reviewed Sep 20, 2024

View reviewed changes

yrliou approved these changes Sep 20, 2024

View reviewed changes

ChromeAutocompleteProviderClient should check AIChatService exists

5f89085

bridiver approved these changes Sep 20, 2024

View reviewed changes

Brandon-T approved these changes Sep 20, 2024

View reviewed changes

petemill enabled auto-merge (squash) September 20, 2024 18:55

mkarolin disabled auto-merge September 20, 2024 19:33

mkarolin merged commit a0424cf into master Sep 20, 2024
16 of 17 checks passed

mkarolin deleted the aichat-services branch September 20, 2024 19:34

github-actions bot added this to the 1.72.x - Nightly milestone Sep 20, 2024

mkarolin mentioned this pull request Sep 20, 2024

Browser crash when trying to close tab with sidebar open brave/brave-browser#41124

Open

darkdh added a commit that referenced this pull request Sep 20, 2024

Fix lifetime of refine request string after rebasing onto #24921

d9c315e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AIChat refactor to support standalone and persistent conversations #24921

AIChat refactor to support standalone and persistent conversations #24921

petemill commented Jul 30, 2024 •

edited

Loading

bridiver Aug 8, 2024

petemill Aug 9, 2024

bridiver Sep 20, 2024

bridiver Aug 8, 2024

petemill Aug 9, 2024

darkdh Sep 11, 2024

petemill Sep 18, 2024

bridiver Aug 8, 2024

petemill Aug 9, 2024

bridiver Sep 20, 2024

yrliou Sep 5, 2024

petemill Sep 10, 2024

brave-builds commented Sep 11, 2024

petemill commented Sep 20, 2024

bridiver Sep 20, 2024

petemill Sep 20, 2024

bridiver Sep 20, 2024

petemill Sep 20, 2024

darkdh left a comment

darkdh Sep 20, 2024

darkdh Sep 20, 2024

bridiver Sep 20, 2024

bridiver Sep 20, 2024

bridiver Sep 20, 2024

petemill Sep 20, 2024

bridiver Sep 20, 2024

petemill Sep 20, 2024

brave-builds commented Sep 20, 2024

yrliou left a comment

petemill commented Sep 20, 2024

AIChat refactor to support standalone and persistent conversations #24921

AIChat refactor to support standalone and persistent conversations #24921

Conversation

petemill commented Jul 30, 2024 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

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

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

brave-builds commented Sep 11, 2024

petemill commented Sep 20, 2024

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

darkdh 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

Choose a reason for hiding this comment

brave-builds commented Sep 20, 2024

yrliou left a comment

Choose a reason for hiding this comment

petemill commented Sep 20, 2024

petemill commented Jul 30, 2024 •

edited

Loading