AIChat: Add Welcome guide as a new opt-in UX #20750

nullhook · 2023-10-30T20:59:55Z

This PR introduces a new welcome guide for users who haven't agreed to the policy terms. Once the user has viewed and accepted the policy, the welcome guide will no longer be displayed.

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 lint, 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:

Test Plan:

brave-builds · 2023-10-30T22:09:05Z

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

github-actions · 2023-10-31T23:40:49Z

The security team is monitoring all repositories for certain keywords. This PR includes the word(s) "policy" and so security team members have been added as reviewers to take a look.

No need to request a full security review at this stage, the security team will take a look shortly and either clear the label or request more information/changes.

Notifications have already been sent, but if this is blocking your merge feel free to reach out directly to the security team on Slack so that we can expedite this check.

brave-builds · 2023-11-01T00:00:00Z

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

brave-builds · 2023-11-01T03:22:41Z

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

brave-builds · 2023-11-01T06:52:32Z

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

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

petemill

This needs to handle pending messages in the c++, i.e. messages sent from the location bar pre opt-in. When fixing that, perhaps also put your pending clientside message their instead of making a new frontend state field for it.

brave-builds · 2023-11-04T09:03:21Z

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

petemill · 2023-11-07T20:59:04Z

components/ai_chat/browser/ai_chat_tab_helper.cc

@@ -316,7 +316,7 @@ void AIChatTabHelper::MaybeGeneratePageText() {
  // Perf: make sure we're not doing this when the feature
  // won't be used (e.g. not opted in or no active conversation).
  if (is_page_text_fetch_in_progress_ || !article_text_.empty() ||
-      !HasUserOptedIn() || !is_conversation_active_ ||
+      !is_conversation_active_ ||


You removed HasUserOptedIn and the comment doesn't make sense anymore

yes, i will revise the comment

petemill · 2023-11-07T21:03:20Z

components/ai_chat/browser/ai_chat_tab_helper.cc

@@ -336,7 +336,8 @@ void AIChatTabHelper::MaybeGeneratePageText() {
    FetchPageContent(
        web_contents(),
        base::BindOnce(&AIChatTabHelper::OnTabContentRetrieved,
-                       weak_ptr_factory_.GetWeakPtr(), current_navigation_id_));
+                       weak_ptr_factory_.GetWeakPtr(), current_navigation_id_),
+        HasUserOptedIn());


If HasUserOptedIn() is false then FetchPageContent doesn't do anything (if it has to make a fetch). So in those cases it won't get content. So why not avoid that complication by not calling FetchPageContent? As discussed, the opt-in UX doesn't need to know anything about the page content, it only needs to know if the page has content. At the moment all pages have content if they are http(s). Can't we just send that data to the page's opt-in UX so it can differ whether it shows the page content association or not? For example, SiteInfo can simply get an { isPresent: boolean } new property as well as title becoming optional.

If the user hasn't opted in, FetchPageContent will still be called. Previously, we would gate the call with various conditions, including the user not having opted in. However, now we send that boolean through. We gate it prior to sending results, and in the case of video, we don't make a network request.

petemill · 2023-11-07T21:05:36Z

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

@@ -20,7 +20,8 @@ function ConversationList() {
    isGenerating,
    conversationHistory,
    suggestedQuestions,
-    shouldDisableUserInput
+    shouldDisableUserInput,
+    hasAcceptedAgreement


shouldShowQuestions to avoid non-relevant logic in here?

sorry, what are you referring to here?

You have added a new prop to the ConverationList component: hasAcceptedAgreement. It means that the ConversationList component now needs to know what to show before or after the user has accepted the agreement. I'm suggesting it's better to keep all that logic in one place, at a higher level, than spread it around lower-level components. Hence, putting a prop such as shouldShowQuestions on this component instead.

makes sense, added

components/resources/ai_chat_ui_strings.grdp

brave-builds · 2023-11-09T12:32:28Z

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

petemill · 2023-11-09T19:28:57Z

components/ai_chat/core/common/mojom/ai_chat.mojom

@@ -149,4 +149,5 @@ interface ChatUIPage {
  // |is_fetching| will be |true|.
  OnSiteInfoChanged(bool is_fetching, SiteInfo? info);
  OnFaviconImageDataChanged(array<uint8> favicon_image_data);
+  OnRequestPending();


This is too generic - how about it's named OnConversationEntryPending()?

components/ai_chat/core/browser/conversation_driver.cc

mkarolin

strings++

nullhook · 2023-11-14T18:11:27Z

Thanks, I've addressed the UI issues you mentioned. However, there are minor font spacing and line-height differences. For example, the main title and subtitle have a 40px line height, but Figma uses 36px. Nala is currently undergoing upgrades, and it's taking some time due to the font hierarchy reconstruction.

Additionally, I've collaborated with @aguscruiz on graphic element positioning and font weights. He has made the necessary updates in Figma, so your screenshot may be outdated when reviewing this.

The dialog component has been replaced with Nala's as well.

brave-builds · 2023-11-14T18:44:23Z

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

brave-builds · 2023-11-14T23:36:26Z

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

brave-builds · 2023-11-16T00:33:48Z

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

brave-builds · 2023-11-16T17:21:54Z

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

petemill · 2023-11-18T00:18:12Z

browser/ui/webui/ai_chat/ai_chat_ui_page_handler.cc

+  // Trigger the observer because there might be a pending conversation entry.
+  // In some cases, this page handler hasn't been created and remote might not
+  // have been set yet, ensuring that we don't miss any UI updates, we trigger
+  // the observer again.
+  if (active_chat_tab_helper_ &&
+      active_chat_tab_helper_->HasPendingConversationEntry()) {
+    OnConversationEntryPending();
+  }
 }


This comment doesn't really make sense to me - I think it's simply something like "Get current state data" - i.e. we're getting the current state because we haven't been around to get events yet. This is probably where we should put everything that's needed initially...

in essence, sure, it's getting "current state data" but that's obvious, right?

the not-so obvious part here is, why are we triggering OnConversationEntryPending event, specifically because no other events are there.

i have revised the comment which describes the scenario why this happens.

browser/ui/webui/ai_chat/ai_chat_ui_page_handler.cc

components/ai_chat/core/browser/constants.cc

components/ai_chat/core/browser/conversation_driver.cc

components/ai_chat/resources/page/state/data-context-provider.tsx

components/ai_chat/content/browser/page_content_fetcher.cc

brave-builds · 2023-11-20T12:46:50Z

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

components/ai_chat/core/browser/conversation_driver.cc

brave-builds · 2023-11-21T04:51:14Z

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

petemill · 2023-11-28T00:40:35Z

components/ai_chat/core/browser/conversation_driver.h

@@ -159,7 +164,8 @@ class ConversationDriver {
  mojom::APIError current_error_ = mojom::APIError::None;
  mojom::PremiumStatus last_premium_status_ = mojom::PremiumStatus::Inactive;

-  std::unique_ptr<mojom::ConversationTurn> pending_request_;
+  std::unique_ptr<mojom::ConversationTurn> pending_conversation_entry_;
+  bool is_pending_content_retreival_ = false;


we already have bool is_page_text_fetch_in_progress_. How is this different? If there is reason to have both, then it would be clearer if they are next to each other and commented. I'm also wondering why ConversationDriver::HasPageContent doesn't need to consider this new property? Perhaps we should simplify by storing something like a PageContentAssociation page_status property.

petemill · 2023-11-28T00:44:57Z

components/ai_chat/resources/page/state/data-context-provider.tsx

@@ -102,7 +103,7 @@ function DataContextProvider (props: DataContextProviderProps) {
    getPageHandlerInstance().pageHandler.generateQuestions()
  }

-  const handleSiteInfo = (isFetching: boolean, siteInfo: mojom.SiteInfo | null) => {
+  const handleSiteInfo = (isFetching: boolean, siteInfo: mojom.SiteInfo | undefined) => {


This doesn't seem correct now that you've changed siteInfo to non-optional

fixed: 87b88d3

petemill · 2023-11-28T00:49:25Z

browser/ui/webui/ai_chat/ai_chat_ui_page_handler.cc

+void AIChatUIPageHandler::SubmitSummarizationRequestFromWelcomeView() {
+  mojom::ConversationTurn turn = {
+      CharacterType::HUMAN, ConversationTurnVisibility::VISIBLE,
+      l10n_util::GetStringUTF8(IDS_CHAT_UI_SUMMARIZE_PAGE)};


This seems pretty fragile - we're using this specific string because we know that the ConversationDriver happens to treat it as a special summarization request. I think this decision should at least be contained within the same file. So, how about passing through SubmitSummarizationRequestFromWelcomeView to the ConversationDriver?

petemill · 2023-11-28T00:50:55Z

browser/ui/webui/ai_chat/ai_chat_ui_page_handler.cc

-  std::move(callback).Run(is_fetching_content, site_info.has_value()
-                                                   ? site_info.value().Clone()
-                                                   : nullptr);
+  std::move(callback).Run(is_fetching_content, site_info.Clone());


I think this would be less confusing now that SiteInfo is non-optional, to put the fetching status as a property on SiteInfo, for GetSiteInfo and for OnSiteInfoChanged

fixed: 87b88d3

components/ai_chat/core/browser/conversation_driver.cc

components/resources/ai_chat_ui_strings.grdp

petemill · 2023-11-28T01:04:28Z

browser/ui/webui/ai_chat/ai_chat_ui_page_handler.cc

+  site_info.is_content_truncated =
+      active_chat_tab_helper_->IsPageContentsTruncated();
+  site_info.is_content_present =
+      active_chat_tab_helper_->IsPageContentsPresent();


This is so confusing now - we have both ConversationDriver::HasPageContent and ConversationDriver::IsPageContentsPresent and it's not clear why they would differ. Can we use the existing HasPageContent? If we need it to do something else let's be explicit about the states that associated content can be in, e.g.

Fetching (i.e. "finding out")

Has Content

No Content

Other...?

I can see that IsPageContentsPresent is checking whether it can be present (based on the Url) and HasPageContent is checking whether we ended up getting content, but I think HasPageContent can change to do the check in IsPageContentsPresent.
We can also put the enum directly on SiteInfo if that's cleaner. Less to wonder about.

brave-builds · 2023-11-28T16:38:48Z

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

- dont send opt-in bool - siteinfo has a new prop - rename pending conversation entries - uses nala dialog - nala dialog width should work - flush welcome guide to bottom - introducing should_show_questions prop - added pending_message_needs_page_content_ - siteInfo is non-optional - added logic to show_suggestion bool - special summarization request

brave-builds · 2023-11-28T23:42:34Z

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

nullhook · 2023-11-28T23:48:39Z

closing this in-lieu of: #20966

github-actions bot assigned nullhook Oct 30, 2023

github-actions bot added the CI/storybook-url Deploy storybook and provide a unique URL for each build label Oct 30, 2023

nullhook changed the title ~~show privacy modal on input submit~~ AIChat: Add Welcome guide Oct 31, 2023

nullhook changed the title ~~AIChat: Add Welcome guide~~ AIChat: Add Welcome guide as a new opt-in UX Oct 31, 2023

nullhook marked this pull request as ready for review October 31, 2023 23:29

github-actions bot added the needs-security-review label Oct 31, 2023

github-actions bot assigned fmarier and thypon Oct 31, 2023

fmarier unassigned fmarier and thypon Oct 31, 2023

fmarier removed the needs-security-review label Oct 31, 2023

nullhook requested a review from a team as a code owner November 1, 2023 06:14

nullhook requested a review from petemill November 1, 2023 06:18

nullhook commented Nov 2, 2023

View reviewed changes

components/ai_chat/resources/page/components/welcome_guide/index.tsx Outdated Show resolved Hide resolved

petemill requested changes Nov 3, 2023

View reviewed changes

nullhook requested a review from petemill November 4, 2023 08:04

petemill requested changes Nov 7, 2023

View reviewed changes

nullhook force-pushed the ai-chat-opt-in-ux branch from 380f638 to afe1328 Compare November 9, 2023 11:33

petemill reviewed Nov 9, 2023

View reviewed changes

nullhook commented Nov 9, 2023

View reviewed changes

components/ai_chat/core/browser/conversation_driver.cc Outdated Show resolved Hide resolved

nullhook force-pushed the ai-chat-opt-in-ux branch from afe1328 to 23ba529 Compare November 10, 2023 16:02

nullhook requested a review from petemill November 10, 2023 16:03

mkarolin approved these changes Nov 10, 2023

View reviewed changes

nullhook requested a review from petemill November 14, 2023 18:11

nullhook force-pushed the ai-chat-opt-in-ux branch from 650f051 to 7224310 Compare November 15, 2023 23:29

petemill reviewed Nov 18, 2023

View reviewed changes

petemill requested changes Nov 18, 2023

View reviewed changes

nullhook requested a review from petemill November 20, 2023 13:13

nullhook mentioned this pull request Nov 20, 2023

Graphics image cuts off the Leo's introduction page prominently in France locale brave/brave-browser#34034

Closed

petemill reviewed Nov 21, 2023

View reviewed changes

components/ai_chat/core/browser/conversation_driver.cc Outdated Show resolved Hide resolved

nullhook requested a review from petemill November 21, 2023 04:18

petemill requested changes Nov 28, 2023

View reviewed changes

petemill mentioned this pull request Nov 28, 2023

AIChat: new welcome opt-in ux and contextual toggle #20966

Merged

25 tasks

nullhook force-pushed the ai-chat-opt-in-ux branch from 1b00c8b to 389bae2 Compare November 28, 2023 15:39

nullhook force-pushed the ai-chat-opt-in-ux branch 5 times, most recently from e120725 to 103342c Compare November 28, 2023 21:48

nullhook force-pushed the ai-chat-opt-in-ux branch from 103342c to bf40c91 Compare November 28, 2023 22:17

simplify siteInfo

b910a80

nullhook closed this Nov 28, 2023

AIChat: Add Welcome guide as a new opt-in UX #20750

AIChat: Add Welcome guide as a new opt-in UX #20750

Conversation

nullhook commented Oct 30, 2023 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

brave-builds commented Oct 30, 2023

github-actions bot commented Oct 31, 2023

brave-builds commented Nov 1, 2023

brave-builds commented Nov 1, 2023

brave-builds commented Nov 1, 2023

petemill left a comment

Choose a reason for hiding this comment

brave-builds commented Nov 4, 2023

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 Nov 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkarolin left a comment

Choose a reason for hiding this comment

nullhook commented Nov 14, 2023 • edited Loading

brave-builds commented Nov 14, 2023

brave-builds commented Nov 14, 2023

brave-builds commented Nov 16, 2023

brave-builds commented Nov 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brave-builds commented Nov 20, 2023

brave-builds commented Nov 21, 2023

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 Nov 28, 2023

brave-builds commented Nov 28, 2023

nullhook commented Nov 28, 2023

nullhook commented Oct 30, 2023 •

edited

Loading

nullhook commented Nov 14, 2023 •

edited

Loading