-
Notifications
You must be signed in to change notification settings - Fork 861
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
AIChat refactor to support standalone and persistent conversations #24921
Conversation
904d9c5
to
90e5b70
Compare
4f4d6a2
to
d7f1476
Compare
d7f1476
to
6f984db
Compare
std::unique_ptr<KeyedService> | ||
AIChatServiceFactory::BuildServiceInstanceForBrowserContext( | ||
content::BrowserContext* context) const { | ||
auto skus_service_getter = base::BindRepeating( |
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.
whys is this passing a getter instead of the skus service itself?
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.
This code is just moved. We should fix that, but not here.
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.
can we create follow-up issues to track this stuff?
|
||
content::BrowserContext* AIChatServiceFactory::GetBrowserContextToUse( | ||
content::BrowserContext* context) const { | ||
if (!Profile::FromBrowserContext(context)->IsRegularProfile() || |
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.
use ProfileSelections::Builder().WithRegular(ProfileSelection::kOwnInstance)
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.
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?
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.
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.
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.
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)
return std::make_unique<AIChatService>( | ||
ModelServiceFactory::GetForBrowserContext(context), | ||
std::move(credential_manager), user_prefs::UserPrefs::Get(context), | ||
(g_brave_browser_process->process_misc_metrics()) |
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.
why the parens around this? @DJAndries have you had a change to do anything with this yet to make it less clunky to access?
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.
Because of the ternary? formatter isn't complaining, but happy to change...
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.
shouldn't be needed here
47485af
to
9c8b53d
Compare
1e5ceda
to
a8143f6
Compare
void ConversationHandler::OnGetStagedEntriesFromContent( | ||
const std::optional<SearchQuerySummary>& search_query_summary) { | ||
// Check if all requirements are still met. | ||
if (!search_query_summary || !chat_history_.empty() || |
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 we also check user opt-in here?
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.
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.
c041d30
to
e8ce0d2
Compare
A Storybook has been deployed to preview UI for the latest push |
Everything passed CI except iOS due to unrelated test runner fix in #25669. Rebased to get that passing. |
adc83de
to
23b728d
Compare
@@ -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_) |
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.
don't we need a null check here before calling GetOrCreateConversationHandlerForContent
?
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.
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
} else { | ||
auto* browser = GetBrowser(); | ||
if (!browser) { | ||
VLOG(1) << "Can't get browser"; |
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.
this kind of logging should be removed before merge
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.
this is not new
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.
ai-chat cpp side looks good to me. Unit tests for new codes and moved codes are nicely done with great coverage!
conversation_observations_{this}; | ||
}; | ||
|
||
class MockAIChatCredentialManager : public AIChatCredentialManager { |
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, we will do this in followup.
class ConversationHandlerUnitTest_OptedOut | ||
: public ConversationHandlerUnitTest { | ||
public: | ||
ConversationHandlerUnitTest_OptedOut() { is_opted_in_ = false; } | ||
}; | ||
|
||
class ConversationHandlerUnitTest_NoAssociatedContent | ||
: public ConversationHandlerUnitTest { | ||
public: | ||
ConversationHandlerUnitTest_NoAssociatedContent() { | ||
has_associated_content_ = false; | ||
} | ||
}; |
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, we will do this in followup.
auto selected_text = base::UTF16ToUTF8(params_.selection_text); | ||
|
||
if (rewrite_in_place) { | ||
source_web_contents_->SetUserData(kAIChatRewriteDataKey, |
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.
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
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.
also are there tests for this somewhere? I'm not seeing anything, but searching github PRs sucks so I could be missing something
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.
nvm, I found them
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.
Sure but this is not added in this PR cc @yrliou
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.
can we create follow-up issues to track this stuff?
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.
A Storybook has been deployed to preview UI for the latest push |
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.
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.
I made this issue to track findings of future code improvements brave/brave-browser#41171 |
This PR refactors the ai_chat component on the browser-side and in the UI to allow for
This is achieved with the following architecture (more details at brave/brave-browser#31623):
AIChatService -> ConversationHandler -> AssociatedContentDelegate [AssociatedContentDriver [AIChatTabHelper]]
Future TODO (other PRs)
Resolves brave/brave-browser#31623
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
wikinpm run presubmit
wiki,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan: