Skip to content
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: new welcome opt-in ux and contextual toggle #20966

Merged
merged 13 commits into from
Dec 1, 2023

Conversation

nullhook
Copy link
Contributor

@nullhook nullhook commented Nov 13, 2023

Resolves brave/brave-browser#34099
Resolves brave/brave-browser#33942

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.

Additionally, adds a toggle near the input area for users to control if the page contents should be attached to the conversation or not.

  1. the toggle is only visible and controllable before the conversation starts
  2. once you've start the chat you'll be locked in with what you've selected
  3. the site title indicator has moved in the chat history itself

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:

@github-actions github-actions bot added the CI/storybook-url Deploy storybook and provide a unique URL for each build label Nov 13, 2023
@brave-builds
Copy link
Collaborator

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

@brave-builds
Copy link
Collaborator

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

1 similar comment
@brave-builds
Copy link
Collaborator

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

@nullhook nullhook force-pushed the ai-chat-contextual-toggle branch 4 times, most recently from 644c5db to 293184a Compare November 15, 2023 23:33
@brave-builds
Copy link
Collaborator

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

@nullhook nullhook marked this pull request as ready for review November 17, 2023 03:56
@nullhook nullhook requested a review from a team as a code owner November 17, 2023 03:56
@brave-builds
Copy link
Collaborator

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

Copy link
Member

@petemill petemill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has an issue with the new welcome UX: If I uncheck the new toggle on the Welcome screen, the "Summarize" button doesn't disappear. If I click that then I get a state which seems to say it's about the page but it's missing the title (and it isn't about the page):
image
In fact this is a problem submitting any input in the Welcome UX, not just the "Summarize" built-in entry suggestion.

@brave-builds
Copy link
Collaborator

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

@nullhook
Copy link
Contributor Author

thanks for spotting. you should be able to see the correct behavior now.

@brave-builds
Copy link
Collaborator

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

Comment on lines 20 to 22
getPageHandlerInstance().pageHandler.setShouldSendPageContents(
e.detail.checked
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is more of a nit, but one thing concerns me here - we only show this Toggle when the associated page has SiteInfo. When we call setShouldSendPageContents, which this toggle is calling directly itself, we're clearing the page content and technically SiteInfo should indicate there's no page present, which would hide this very toggle as soon as it's diabled. I think that potential bug is avoided because we only re-fetch SiteInfo in f/e initialiseForTargetTab. It seems an easy future regression when that state gets updated whenever page association / site info changes.

@brave-builds
Copy link
Collaborator

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

@brave-builds
Copy link
Collaborator

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

1 similar comment
@brave-builds
Copy link
Collaborator

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

@brave-builds
Copy link
Collaborator

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

@petemill
Copy link
Member

This now has commits from the new welcome opt-in PR #20750, so I suppose that means it's blocked on that now?

@brave-builds
Copy link
Collaborator

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

@brave-builds
Copy link
Collaborator

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

@brave-builds
Copy link
Collaborator

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

Copy link
Contributor

github-actions bot commented Dec 1, 2023

[puLL-Merge] - brave/brave-core@20966

The patch introduces changes to an AI chat feature, which includes updates to user experience (UX) elements and logic.

Key changes included in the patch:

  1. Introduction of a new opt-in UX for the AI chat feature.
  2. Property changes and logic enhancements for site information handling.
  3. Renaming and refactoring of code for clarity.
  4. Addition of new SVG (Scalable Vector Graphics) components and a welcome guide component with related styling.
  5. Logic for toggling whether page context should be included in the conversation or not.
  6. Adjustments to how suggested questions are displayed and when the summarize page button is shown.
  7. Assorted bug fixes and code improvements.

Files affected by the patch are predominantly Typescript (.tsx) components, C++ (.cc and .h) files, and resource definitions (.grdp) that manage strings for localization.

Specific updates mentioned in the patch commits include:

  • Changing the handling of a boolean value indicating whether to send page content as part of a conversation.
  • Improvements to how site information and content association status are managed.
  • Allowing a 'special summarization request' to be made.
  • Introducing a toggle for users to decide if they want to include page context in AI chat responses.
  • Ensuring UI elements are accessible and properly positioned.

The patch seems to be part of a series, as indicated by the "[PATCH 01/13]" in the description, which implies there are other related patches in the sequence. Each affected file is listed along with the details of insertions (+), modifications ( ), and deletions (-) provided in a summary at the top of the patch. The patch applies these changes to the codebase, likely contributing to the development of the AI chat feature.

@brave-builds
Copy link
Collaborator

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

1 similar comment
@brave-builds
Copy link
Collaborator

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

nullhook and others added 13 commits December 1, 2023 09:32
- 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
- remove prop in siteInfo
- DCHECK to avoid setter should_send_page_contents_ being set multiple times
- DCHECK for special summarization request
@brave-builds
Copy link
Collaborator

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

@petemill petemill merged commit 058825f into master Dec 1, 2023
18 checks passed
@petemill petemill deleted the ai-chat-contextual-toggle branch December 1, 2023 20:01
@github-actions github-actions bot added this to the 1.63.x - Nightly milestone Dec 1, 2023
@stephendonner
Copy link
Contributor

stephendonner commented Dec 5, 2023

Verification PASSED using

Brave | 1.63.12 Chromium: 120.0.6099.56 (Official Build) nightly (64-bit)
-- | --
Revision | 7517dcc0934f42a59a43477bca4ebefc18d7e14f
OS | Windows 10 Version 22H2 (Build 19045.3693)

Context-indicator changes - brave/brave-browser#34099 - PASSED

Steps:

  1. installed 1.63.12
  2. launched Brave
  3. loaded https://plus.nasa.gov/scheduled-video/iss-expedition-70-in-flight-event-to-mark-the-25th-anniversary-of-the-mating-of-the-zarya-and-unity-modules/
  4. clicked on Leo in the sidebar
  5. toggled Use page context for response to Off
  6. clicked on Summarize this page
  7. clicked on Accept and begin
  8. waited for the response output

Confirmed Summarize this page button and the Use page context for response toggle are now hidden, with Suggest questions... still there

step 4 step 5 step 6 step 7
image image image image

Suggested questions and context-indicator changes - brave/brave-browser#33575 - PASSED

Steps:

  1. installed 1.63.12
  2. launched Brave
  3. loaded brave://settings/leo-assistant
  4. confirmed Show suggested prompts in the conversation preference is removed
  5. loaded https://www.euronews.com/2023/12/05/two-spanish-intelligence-agents-arrested-for-passing-classified-information-to-the-us
  6. clicked on Leo in the sidebar
  7. clicked on Summarize this page
  8. confirmed opt-in roadblock
  9. clicked Accept and begin
  10. waited for the response output
  11. clicked on Suggested questions...
  12. confirmed three (3) Suggested follow-ups
  13. clicked on the ... ellipsis at the top-right of the Leo panel
  14. confirmed Suggested questions is no longer present in the WebUI panel
step 4 step 7 step 8 step 10 steps 11+12 step 14
image image image image image image

New AI Chat opt-in experience - brave/brave-browser#33942 - PASSED

Steps:

  1. installed 1.63.12
  2. launched Brave
  3. loaded vox.com
  4. clicked on Leo in the sidebar
  5. clicked on Summarize this page
  6. clicked on Accept and begin
  7. waited
  8. scrolled to the bottom of the answer text
  9. confirmed answer was well-formed and complete
  10. loaded vox.com again

Confirmed opt-in prompt disappears after the 1st summary or question answered

Also repeated the above, but using a manually-entered question to trigger it, instead

example example example example example
image image image image image

nullhook added a commit that referenced this pull request Dec 8, 2023
* aichat: new opt-in ux

* simplify siteInfo

* aichat: toggle page context responses from LLM

* moved build site info to conversation driver

* special summarize button visibility depends on toggle

* MakeAPIRequestWithConversationHistoryUpdate accepts a flag

* show model intro on all pages

* AI Chat: Don't show suggested questions when conversation isn't associated with content

* AI Chat: conversation entry should also be pending if content isn't ready yet

---------

Co-authored-by: Pete Miller <miller.pete@gmail.com>
kjozwiak pushed a commit that referenced this pull request Dec 9, 2023
…1314)

* Merge pull request #21063 from brave/ai-chat-summary-prompt-force-english

ai chat: fix summary prompt for pages is being translated

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

* aichat: new opt-in ux

* simplify siteInfo

* aichat: toggle page context responses from LLM

* moved build site info to conversation driver

* special summarize button visibility depends on toggle

* MakeAPIRequestWithConversationHistoryUpdate accepts a flag

* show model intro on all pages

* AI Chat: Don't show suggested questions when conversation isn't associated with content

* AI Chat: conversation entry should also be pending if content isn't ready yet

---------

Co-authored-by: Pete Miller <miller.pete@gmail.com>

---------

Co-authored-by: Pete Miller <miller.pete@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/storybook-url Deploy storybook and provide a unique URL for each build
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Context indicator changes AI Chat: Update opt-in experience
7 participants