AI Chat fullpage UI notices and polish #26678

petemill · 2024-11-21T03:27:17Z

Blocked on:

[Nala / @brave/leo] update dependency #26767

Description

Shows a notice that the full-page feature is now available
Shows a notice when the conversation list is empty
Shows a notice when storage pref is disabled and offers an action to enable
Fixes FullPage showing as SidePanel mode when restoring browser state after startup
Optimizes loading by fetching initial properties as soon as JS is ready and not after first react render (seems to save up to 60ms on my machine, but that's in dev)
Shows a full-page loading indicator and other minor page-load visual polish
Fix "new conversation" button showing on collapsed navigation when conversation is already new
AI Chat conversation items get their intended max-width and line up with input box

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:

brave-builds · 2024-11-21T04:31:34Z

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

petemill · 2024-11-22T17:20:10Z

Depends on brave/leo#912

diracdeltas · 2024-11-22T19:25:21Z

does https://github.com/brave/reviews/issues/1703#issuecomment-2260662872 need to be addressed now or is this still all behind a feature flag?

thypon

security gtg

petemill · 2024-11-22T20:45:18Z

@diracdeltas I'm not sure if this notice is enough to cover all that info, but yes it's still behind a feature flag and I'll ask if we've done enough when asking to remove that flag.

ShivanKaul · 2024-11-22T21:02:04Z

components/ai_chat/core/browser/ai_chat_service.cc

+  profile_prefs_->SetBoolean(prefs::kStorageEnabled, true);
+}
+
+void AIChatService::DismissStorageNotice() {


We reach this if the user presses X on the top-right of the Conversation history popup in Leo, right? We also need an explicit "Don't do this" or "No thanks" button on that popup, otherwise this seems like a consent dark pattern. The language on that popup right now makes me think that the X will actually just dismiss the popup without actually opting me out of this feature change.

@ShivanKaul The storage notice is dismissed either by clicking the X or after it's been viewed once for a few seconds.
As far as I understand, the opinion was that we did not need to present the opt-out preference inline as it's not going to affect any previous actions the user does, only the future actions. And that we don't need explicit opt-in permission from the user to store submitted conversations on their own computer (encrypted) for the AI chat feature. This seems more of a notice about the new feature. @mattmcalister

components/resources/ai_chat_ui_strings.grdp

ShivanKaul · 2024-11-22T21:06:37Z

components/ai_chat/resources/page/components/notices/notice_conversation_storage.tsx

+        <p>{getLocale('noticeConversationHistoryBody')}</p>
+        <p>
+          <a
+            href='#'


Let's make sure the help page for this feature gets reviewed before shipping.

this isn't the url it points to, it's the current Leo help page

components/resources/ai_chat_ui_strings.grdp

ShivanKaul · 2024-11-22T21:09:58Z

components/resources/ai_chat_ui_strings.grdp

+    History is disabled
+  </message>
+  <message name="IDS_CHAT_UI_NOTICE_CONVERSATION_HISTORY_DISABLED_PREF" desc="Main text for a notice that the conversation history preference is disabled">
+    In order to view and search your previous conversations with Leo, you need to enable conversation history.


Similarly here, I don't think we need the word "previous". Not blocking.

I'd argue previous in this case is necessary (although I'll remove the "and search" part because we don't have that). Feels a bit strange to be viewing a conversation (a non-stored one) with a notice saying "In order to view your conversations with Leo, you need to...".

ShivanKaul · 2024-11-22T21:12:58Z

It's hard to review this from a privacy/consent perspective just from the screenshots. I suggested changes for the text, but it's not clear to me if the intention is to block users from going back to the old behaviour on sidebar Leo (i.e. ephemeral-only chats). Does the "History is disabled" prompt block users from interacting with Leo?

petemill · 2024-11-23T01:27:39Z

Does the "History is disabled" prompt block users from interacting with Leo?

@ShivanKaul this prompt shows when the user has disabled the AI Chat history storage preference (which is enabled by default for all users). It does not block anything. It informs the user that conversations are ephemeral and won't show up in a list once the conversation is closed.

petemill · 2024-11-23T01:38:36Z

It's hard to review this from a privacy/consent perspective just from the screenshots

@ShivanKaul In that case, I suggest we don't block this PR so that reviewers can easily use the feature end-to-end in nightly. Although if there are obvious required changes to these pieces then we should do them here. This also doesn't contain any initial opt-in changes that may be required.

Instead we can block feature flag removal unril privacy review is successful.

fallaciousreasoning

This is looking pretty good. Stepping back a little bit, I wonder if we should have a generic StateProvider context that we can just provide some data to and do all the logic in the API wrapper?

i.e. we have some kind of interface our API wrappers conform to and then we have something to do the Context boilerplate automatically?

That'd solve main main problem with Kevin's current solution, and keep it so we can override the state in Storybook. WDYT?

components/ai_chat/core/browser/ai_chat_service.cc

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

components/ai_chat/resources/page/api/index.ts

fallaciousreasoning · 2024-11-24T23:10:58Z

components/ai_chat/resources/page/ai_chat_ui.html

    <div id="mountPoint"></div>
+    <script src="chat_ui.bundle.js"></script>


could we keep the type here as module and add the flag to the BUILD.gn to output a module? I think we should be gradually migrating anyway

Should the main bundle be module? The reason I removed it was to have it load as eagerly as possible, but maybe I'm mistaken?

I think in this scenario, yes. So <script type=module> is defer by default so it will execute after the HTML has finished parsing (but before DOMContentLoaded fires), but it will start loading as soon as the parser sees the tag in the document.

I think we should put the <script type=module src=..> in the head of the document. Using the raw script tag will block further parsing of the HTML file until parsing of the script completes (though this is at the end of the document so its probably fine).

Either way, I don't think there should be a huge difference.

https://jakearchibald.com/2017/es-modules-in-browsers/

components/resources/ai_chat_ui_strings.grdp

package.json

fallaciousreasoning · 2024-11-24T23:14:11Z

package-lock.json

@@ -149,7 +149,7 @@
        "webpack-cli": "5.1.4"
      },
      "engines": {
-        "node": ">=20.0.0 <24.0.0",
+        "node": ">=20.0.0 <22.0.0",


is this intentional?

no, it happened as a result of the temporary leo update

(and because someone has modified engines in package.json but not package-lock.json)

components/ai_chat/resources/page/chat_ui.tsx

PrivacyMatters · 2024-11-25T09:10:39Z

@petemill Re How do users disable history? How are we notifying them?

mkarolin

strings++

components/ai_chat/core/browser/ai_chat_service_unittest.cc

github-actions · 2024-11-28T00:45:18Z

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

Description

This PR implements conversation history storage functionality for the Brave AI Chat feature. It includes changes to enable storing conversations, displaying a notice about the new functionality, and allowing users to manage their conversation history.

Possible Issues

The PR introduces new UI elements and state management, which could potentially introduce rendering or state synchronization issues if not thoroughly tested across different platforms and scenarios.

Security Hotspots

The PR introduces new storage functionality for user conversations. Ensure that the encryption and storage mechanisms are properly implemented to protect user privacy.
The new preference for enabling/disabling storage could potentially be exploited if not properly validated and sanitized.

Changes

Changes

ai_chat_ui.cc:
- Removed code for checking agreement acceptance and premium prompt dismissal.
- Modified logic for associating WebContents with SidePanel mode.
ai_chat_ui_page_handler.cc and ai_chat_ui_page_handler.h:
- Modified SetChatUI method to use a callback instead of directly calling SetInitialData.
ai_chat_service.cc and ai_chat_service.h:
- Added new methods for managing storage preferences and notices.
- Implemented BuildState and OnStateChanged methods for managing global state.
- Modified BindObserver to return initial state.
ai_chat.mojom:
- Added new ServiceState struct and modified related interfaces.
pref_names.cc and pref_names.h:
- Added new preference for dismissing storage notice.
React components (chat_ui.tsx, conversations_list/index.tsx, etc.):
- Updated to use new state management and display new UI elements for conversation history.
iOS changes (AIChatViewModel.swift, ai_chat.h, ai_chat.mm, etc.):
- Updated to reflect new state management and removed deprecated methods.

sequenceDiagram
    participant User
    participant UI
    participant AIChatService
    participant Storage

    User->>UI: Opens AI Chat
    UI->>AIChatService: Request initial state
    AIChatService->>Storage: Check storage preferences
    Storage-->>AIChatService: Return preferences
    AIChatService-->>UI: Return initial state
    UI->>User: Display chat interface
    alt Storage not enabled
        UI->>User: Show storage notice
        User->>UI: Enable storage
        UI->>AIChatService: Enable storage preference
        AIChatService->>Storage: Update storage preference
    end
    User->>UI: Start conversation
    UI->>AIChatService: Send message
    AIChatService->>Storage: Store conversation
    Storage-->>AIChatService: Confirm storage
    AIChatService-->>UI: Update conversation list
    UI->>User: Display updated conversation list

graph TD
    A[AIChatService] --> B[UIHandler]
    A --> C[Storage]
    B --> D[UI Components]
    C --> E[Conversation History]
    D --> F[Conversation List]
    D --> G[Storage Notice]
    D --> H[Main Chat Interface]

petemill · 2024-11-28T00:45:51Z

ios/brave-ios/Sources/AIChat/ModelView/AIChatViewModel.swift

    }

    set {  // swiftlint:disable:this unused_setter_value
-      self.canShowPremiumPrompt = newValue
+      _canShowPremiumPrompt = newValue


@Brandon-T we don't need to call objectWillChange.send() right?

Correct. Not necessary.

brave-builds · 2024-11-28T12:19:25Z

Released in v1.75.59

- [AIChat conversation data storage by petemill · Pull Request \#25876 · brave/brave-core](#25876) - [AI Chat conversations should persist browser restart · Issue \#42800 · brave/brave-browser](brave/brave-browser#42800) `QA/Yes` - [\[AIChat\]: Add URL based routing for different chats by fallaciousreasoning · Pull Request \#26050 · brave/brave-core](#26050) - [\[AI Chat\]: URLs for all conversations · Issue \#42055 · brave/brave-browser](brave/brave-browser#42055) `QA/Yes` - test plan: url and header variations specified - [AI Chat fullpage UI notices and polish by petemill · Pull Request \#26678 · brave/brave-core](#26678) - [AI Chat conversation list should prompt to enable history storage if it's disabled · Issue \#42576 · brave/brave-browser](brave/brave-browser#42576) `QA/Yes` - test plan: specified - [AI Chat notice for conversation storage · Issue \#42360 · brave/brave-browser](brave/brave-browser#42360) `QA/Yes` - test plan: specified - [AI Chat FullPage shouldn't show in SidePanel mode when restored at startup · Issue \#42413 · brave/brave-browser](brave/brave-browser#42413) `QA/Yes` - test plan: specified - [AI Chat becomes a trusted WebUI with an UntrustedWebUI frame for LLM-generated responses by petemill · Pull Request \#26855 · brave/brave-core](#26855) - [Change AI Chat url to chrome://leo-ai · Issue \#42817 · brave/brave-browser](brave/brave-browser#42817) `QA/Yes` - [AI Chat conversation entries should be isolated in an untrusted frame · Issue \#42818 · brave/brave-browser](brave/brave-browser#42818) `QA/No` - [\[AI Chat\]: Update copy button label by fallaciousreasoning · Pull Request \#26422 · brave/brave-core](#26422) - [Change copy button name to text if text in code block · Issue \#42117 · brave/brave-browser](brave/brave-browser#42117) `QA/Yes` - [\[AI Chat\]: Conversation starter pack, for unassociated content by fallaciousreasoning · Pull Request \#26379 · brave/brave-core](#26379) - [\[AI Chat\]: Add support for static conversation starters · Issue \#42106 · brave/brave-browser](brave/brave-browser#42106) `QA/Yes` - test plan: needs improvement - [Css tweaks and icon change for sidebar by aguscruiz · Pull Request \#26450 · brave/brave-core](#26450) - [\[Leo full page\] - Tweak to expand/collapse sidebar icons · Issue \#42068 · brave/brave-browser](brave/brave-browser#42068) `QA/Yes` - test plan: specified (check icons are correct) - [\[AI Chat\]: Update styling on suggestions by fallaciousreasoning · Pull Request \#26565 · brave/brave-core](#26565) - [\[AI Chat\]: Update suggestions style to match new design · Issue \#42107 · brave/brave-browser](brave/brave-browser#42107) `QA/Yes` - test plan: specified (check suggestions have new style) - [\[AI Chat\]: Don't show starter suggestions on non-empty chats by fallaciousreasoning · Pull Request \#26677 · brave/brave-core](#26677) - [AI Chat static conversation starters shouldn't show if conversation has chat history · Issue \#42412 · brave/brave-browser](brave/brave-browser#42412) `QA/Yes` - test plan: specified (verify conversation starters only show when applicable) - [fix hit area for leo conversations list by aguscruiz · Pull Request \#26793 · brave/brave-core](#26793) - [Leo full page - Conversation list - Make the whole item clickable instead of excluding the padding · Issue \#42552 · brave/brave-browser](brave/brave-browser#42552) `QA/No` - [\[Nala / @brave/leo\] update dependency by petemill · Pull Request \#26767 · brave/brave-core](#26767) - brave/brave-browser#42545 `QA/No`

petemill requested a review from fallaciousreasoning November 21, 2024 03:27

petemill self-assigned this Nov 21, 2024

petemill requested a review from a team as a code owner November 21, 2024 03:27

github-actions bot added CI/storybook-url Deploy storybook and provide a unique URL for each build puLL-Merge labels Nov 21, 2024

petemill requested review from a team as code owners November 22, 2024 17:15

github-actions bot added the CI/run-audit-deps Check for known npm/cargo vulnerabilities (audit_deps) label Nov 22, 2024

thypon approved these changes Nov 22, 2024

View reviewed changes

diracdeltas requested a review from PrivacyMatters November 22, 2024 20:47

ShivanKaul reviewed Nov 22, 2024

View reviewed changes

components/resources/ai_chat_ui_strings.grdp Outdated Show resolved Hide resolved

ShivanKaul reviewed Nov 22, 2024

View reviewed changes

mkarolin reviewed Nov 22, 2024

View reviewed changes

components/resources/ai_chat_ui_strings.grdp Outdated Show resolved Hide resolved

ShivanKaul reviewed Nov 22, 2024

View reviewed changes

petemill changed the title ~~AI Chat fullpage notices~~ AI Chat fullpage notices and polish Nov 23, 2024

petemill changed the title ~~AI Chat fullpage notices and polish~~ AI Chat fullpage UI notices and polish Nov 23, 2024

brave deleted a comment from github-actions bot Nov 23, 2024

fallaciousreasoning reviewed Nov 24, 2024

View reviewed changes

mkarolin approved these changes Nov 25, 2024

View reviewed changes

petemill force-pushed the ai-chat-fullpage-notices branch from 3f21df0 to 31f796e Compare November 26, 2024 01:32

fallaciousreasoning reviewed Nov 26, 2024

View reviewed changes

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

petemill removed the request for review from a team November 27, 2024 06:44

petemill removed feature/web3/wallet puLL-Merge labels Nov 27, 2024

fix storage notice coming back because api state wasn't updated

bff2e31

brave deleted a comment from github-actions bot Nov 27, 2024

petemill added 2 commits November 27, 2024 09:09

fix css variable

edf9058

fix ios

922bd94

Base automatically changed from dep-update-nala to master November 27, 2024 17:27

Brandon-T approved these changes Nov 27, 2024

View reviewed changes

iOS gets canShowPremiumPrompt

e868e42

github-actions bot added the puLL-Merge label Nov 28, 2024

petemill commented Nov 28, 2024

View reviewed changes

Brandon-T approved these changes Nov 28, 2024

View reviewed changes

petemill removed the puLL-Merge label Nov 28, 2024

petemill merged commit 3df43e7 into master Nov 28, 2024
17 checks passed

petemill deleted the ai-chat-fullpage-notices branch November 28, 2024 01:56

github-actions bot added this to the 1.75.x - Nightly milestone Nov 28, 2024

petemill mentioned this pull request Dec 11, 2024

AI Chat Full Page Storage feature (uplift to 1.74.x) #26977

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AI Chat fullpage UI notices and polish #26678

AI Chat fullpage UI notices and polish #26678

petemill commented Nov 21, 2024 •

edited

Loading

brave-builds commented Nov 21, 2024

petemill commented Nov 22, 2024

diracdeltas commented Nov 22, 2024

thypon left a comment

petemill commented Nov 22, 2024

ShivanKaul Nov 22, 2024

petemill Nov 23, 2024

ShivanKaul Nov 22, 2024

petemill Nov 23, 2024

ShivanKaul Nov 22, 2024

petemill Nov 23, 2024

ShivanKaul commented Nov 22, 2024

petemill commented Nov 23, 2024

petemill commented Nov 23, 2024

fallaciousreasoning left a comment

fallaciousreasoning Nov 24, 2024

petemill Nov 26, 2024

fallaciousreasoning Nov 26, 2024

fallaciousreasoning Nov 24, 2024

petemill Nov 26, 2024

petemill Nov 27, 2024

fallaciousreasoning Nov 27, 2024

PrivacyMatters commented Nov 25, 2024

mkarolin left a comment

github-actions bot commented Nov 28, 2024

Changes

petemill Nov 28, 2024

Brandon-T Nov 28, 2024

brave-builds commented Nov 28, 2024

		<div id="mountPoint"></div>
		<script src="chat_ui.bundle.js"></script>

AI Chat fullpage UI notices and polish #26678

AI Chat fullpage UI notices and polish #26678

Conversation

petemill commented Nov 21, 2024 • edited Loading

Description

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

brave-builds commented Nov 21, 2024

petemill commented Nov 22, 2024

diracdeltas commented Nov 22, 2024

thypon left a comment

Choose a reason for hiding this comment

petemill commented Nov 22, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShivanKaul commented Nov 22, 2024

petemill commented Nov 23, 2024

petemill commented Nov 23, 2024

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

PrivacyMatters commented Nov 25, 2024

mkarolin left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 28, 2024

Description

Possible Issues

Security Hotspots

Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brave-builds commented Nov 28, 2024

petemill commented Nov 21, 2024 •

edited

Loading