Added text streams to AIChat UI #18376

nullhook · 2023-05-05T03:00:41Z

This PR adds support for consuming chunks from the Anthropic completion API, which improves the user experience by allowing messages to be sent to the UI as they come in, rather than sending the entire message as a single unit.

The PR follows the rule of two from the security docs. The SimpleURLLoader relies on the network process, while the data decoder relies on the utility process. We only read the values in the browser process.

Submitter Checklist:

I confirm that no security/privacy review is needed, or that I have requested one
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:

This PR enables streaming response by default. However, it's important to test two cases.

Prereqs:

Ensure that the internal VPN is enabled.

Steps:

Visit a news article website, preferably The Verge.
Click on an article of your choice.
Open the sidebar and click on the AIChat icon.
Click on the "Summarize" button.

By default, you should observe the messages coming in real-time in chunks. Additionally, you will see a caret similar to a terminal-like interface.

Let's proceed to test the one-shot response by disabling the ai_chat_sse feature flag.

--enable-features=AIChat:ai_chat_sse\/false

Repeat the test with the same article, and this time the message should be received as a whole, which may take some time.

brave-builds · 2023-05-08T17:22:53Z

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

brave-builds · 2023-05-08T23:58:45Z

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

brave-builds · 2023-05-09T00:15:31Z

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

brave-builds · 2023-05-09T19:25:32Z

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

brave-builds · 2023-05-09T21:44:48Z

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

sangwoo108

Sorry for delayed response

sangwoo108 · 2023-05-12T07:48:45Z

components/ai_chat/ai_chat_api.cc

+  // new instance of the process for each call, which may result in unordered
+  // chunks. Thus, we create a single instance of the parser and reuse it for
+  // consecutive calls
+  data_decoder_ = std::make_unique<data_decoder::DataDecoder>();


Nice find! 👍 I didn't know about data_decoder::DataDecoder

components/ai_chat/ai_chat_api.h

components/api_request_helper/api_request_helper.h

components/ai_chat/ai_chat_api.cc

sangwoo108 · 2023-05-12T08:31:14Z

components/ai_chat/ai_chat_api.cc

-  std::string response = result.body();
-  const base::Value::Dict* dict = result.value_body().GetIfDict();
+  is_request_in_progress_ = false;
+  current_request_->reset(nullptr);


Resetting the SimpleURLLoader doesn't seem to be enough, as the entity in APIRequestHelper::url_loaders_ will remain. I think we should do what APIRequestHelper::Cancel() does.

Done. I just had to run the cancel after running the callback.

Then, could you remove this line?

current_request_ is a ::Ticket type which is not a unique_ptr itself but an iterator. When we Cancel() isn't it just removing from the list and the caller is suppose to explicitly reset or reassign to nullptr?

When an entity of list is erased, the entity's desturctor should be called. In this case, the entity is unique_ptr, so it'd free the space for us, isn't it?

sangwoo108

Sorry for delayed response

brave-builds · 2023-05-12T18:30:15Z

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

sangwoo108

LGTM with a nit

sangwoo108 · 2023-05-14T23:02:05Z

components/ai_chat/ai_chat_api.cc

-  std::string response = result.body();
-  const base::Value::Dict* dict = result.value_body().GetIfDict();
+  is_request_in_progress_ = false;
+  current_request_->reset(nullptr);


Then, could you remove this line?

components/ai_chat/ai_chat_api.cc

goodov

DEPS lgtm with a nit.

goodov · 2023-05-15T12:56:21Z

components/ai_chat/DEPS

@@ -1,4 +1,5 @@
 include_rules = [
+  "+services/data_decoder/public",


brave-builds · 2023-05-15T17:39:09Z

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

brave-builds · 2023-05-15T18:04:38Z

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

brave-builds · 2023-05-15T20:24:41Z

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

petemill

Nice work getting it working! Few comments...

petemill · 2023-05-15T20:25:58Z

components/api_request_helper/api_request_helper.h

@@ -111,6 +110,18 @@ class APIRequestHelper {
      const APIRequestOptions& request_options = {},
      ResponseConversionCallback conversion_callback = base::NullCallback());

+  Ticket Request(const std::string& method,


public function definition could do with comment

petemill · 2023-05-15T20:38:14Z

components/ai_chat/ai_chat_api.cc

+  current_request_ = api_request_helper_.Request(
+      "POST", api_url, CreateJSONRequestBody(dict), "application/json", this,
+      headers, {},
+      base::BindOnce(&AIChatAPI::OnResponseStarted,
+                     weak_ptr_factory_.GetWeakPtr()),
+      base::BindRepeating(&AIChatAPI::OnDownloadProgress,
+                          weak_ptr_factory_.GetWeakPtr()));
+}


Most of the benefit of APIRequestHelper seems to be boilerplate for handling the response where it parses JSON and handles various situations. It doesn't seem worth using it anymore if we're just bypassing it. Wouldn't it be almost just as much code to make a request here via SimpleURLLoader? Or, perhaps more prefably, to put the event-stream handling inside APIRequestHelper. Since it's part of the regular http spec, it seems like it should go there where any consumers can benefit - it already has JSON parsing. What do you think? We could just detect the response header content-type: text/event-stream and do what you have here instead.

petemill · 2023-05-15T20:47:38Z

components/ai_chat/ai_chat_api.h

+  ResponseCallback response_callback_;
+  CompletionCallback completion_callback_;


Why store these callbacks here and not pass them through as arguments bound event handlers? Seems like we're deepening the current architectural issue where request and callback state is not cleared when the conversation is changed as a result of a navigation event.

petemill · 2023-05-15T20:56:26Z

components/ai_chat/ai_chat_api.cc

-  std::move(callback).Run(response, success);
+  DCHECK(response_callback_);
+
+  if (const std::string* completion = result->FindStringKey("completion")) {


What if an error occurs as there is no completion key, shouldn't the callback get notified?

The safe thing to do here is to drop the callback I'd say and debug why there's no completion key.

The callback this function reports to is critical because on the other end it consumes the message which updates the UI.

petemill · 2023-05-15T20:58:00Z

components/ai_chat/ai_chat_api.cc

+void AIChatAPI::OnResponseStarted(
+    const GURL& final_url,
+    const network::mojom::URLResponseHead& response_head) {
+  is_request_in_progress_ = true;
+}
+
+void AIChatAPI::OnDownloadProgress(uint64_t current) {
+  is_request_in_progress_ = true;
 }


Why not set is_request_in_progress_ to true inside the QueryPrompt function when creating the request?

This has been handled in a new way in the tab helper itself.

petemill · 2023-05-15T21:15:21Z

components/ai_chat/ai_chat_tab_helper.cc

@@ -279,8 +298,8 @@ void AIChatTabHelper::DistillViaAlgorithm(const ui::AXTree& tree) {
 void AIChatTabHelper::CleanUp() {
  chat_history_.clear();
  article_summary_.clear();
-  SetRequestInProgress(false);


Removing this line from cleanup deepens the issue where navigating away does not clean up some state. Right now, that problem manifests by any in-progress requests showing their result on another page. Now that problem will also have the UI show as in-progress. If we're going to have AIChatAPI store state and be single-request-only, then perhaps it's time in this PR to add a Cancel() method to it.

petemill · 2023-05-15T21:18:25Z

components/ai_chat/ai_chat_api.cc

-  DVLOG(1) << __func__ << " Using model: " << model_name;
+void AIChatAPI::OnDataReceived(base::StringPiece string_piece,
+                               base::OnceClosure resume) {
+  std::vector<std::string> stream_data = base::SplitString(


We should check for a success/error http response code like APIRequestResult can (or re-use that code). Additionally, a request could still come back with the Content-Type response header of anything, e.g. application/json if it's an error. We should check for text/event-stream before assuming.

petemill · 2023-05-15T21:22:04Z

components/ai_chat_ui/components/conversation_list/index.tsx

    }
-  }, [props.list.length, props.isLoading])
+  }, [props.list.length, props.isLoading, lastConversationEntryElementRef.current?.clientHeight])


What's the reasoning on depending on a ref's child attribute? Changing the value of that won't trigger a react render.

The effect logic here aims to interact with the browser's scroll event rather than triggering a re-render. React will re-render to update the DOM, which can result in changes to the height of the <div>. When such changes occur, the effect should respond by triggering the browser's scroll event.

Perhaps, this can be better using a resize observer.

components/ai_chat_ui/components/conversation_list/index.tsx

components/ai_chat/BUILD.gn

brave-builds · 2023-05-15T22:37:00Z

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

brave-builds · 2023-05-18T00:10:09Z

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

brave-builds · 2023-06-06T16:57:48Z

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

brave-builds · 2023-06-06T19:54:44Z

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

…fail

one-shot requests

brave-builds · 2023-06-06T23:13:57Z

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

atuchin-m · 2023-06-12T11:28:15Z

components/api_request_helper/api_request_helper.cc

@@ -253,32 +397,45 @@ void APIRequestHelper::OnResponse(
    raw_body = converted_body.value();
  }

-  data_decoder::DataDecoder::ParseJsonIsolated(


@nullhook
APIRequestHelper is used in multiple components.
That change (ParseJsonIsolated => ParseJson) results in reusing DataDecoder instances in some cases (not only for ai_chat_*) and reduces sanitizing security.

Has that change passed through a sec-review?

we did not review this AFAIK

Sorry, didn't create a sec-review for this.

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

github-actions bot assigned nullhook May 5, 2023

nullhook force-pushed the ai_chat_streams branch from 3a0474f to 52a0957 Compare May 9, 2023 00:00

nullhook marked this pull request as ready for review May 9, 2023 00:00

nullhook requested a review from a team as a code owner May 9, 2023 00:00

nullhook requested review from sangwoo108 and petemill May 9, 2023 00:00

nullhook force-pushed the ai_chat_streams branch from 52a0957 to 37dc493 Compare May 9, 2023 18:17

nullhook force-pushed the ai_chat_streams branch from 321026a to fce99d6 Compare May 9, 2023 21:25

sangwoo108 reviewed May 12, 2023

View reviewed changes

sangwoo108 approved these changes May 14, 2023

View reviewed changes

goodov approved these changes May 15, 2023

View reviewed changes

nullhook force-pushed the ai_chat_streams branch from b92e8d1 to f752a8e Compare May 15, 2023 17:44

nullhook force-pushed the ai_chat_streams branch from 7843ec8 to f251913 Compare May 15, 2023 20:13

petemill reviewed May 15, 2023

View reviewed changes

nullhook force-pushed the ai_chat_streams branch from f251913 to ede27be Compare May 15, 2023 22:03

nullhook force-pushed the ai_chat_streams branch from ede27be to 95167c7 Compare May 17, 2023 23:29

nullhook force-pushed the ai_chat_streams branch from 95167c7 to 3132d79 Compare May 18, 2023 22:34

nullhook requested a review from petemill June 6, 2023 19:49

nullhook added 17 commits June 6, 2023 15:52

Added text streams to AIChat UI

d47ebfd

Added unittest for ai chat api

e57628c

Tweaked scroll logic

fe7ce63

Fixes

a4875ec

Remove unittest from android target

1aa9062

Added safety checks when updating the chat history vector

964c6db

Moved streams logic to APIHelper

eb1b369

Fixed BraveVPN target dep issue which was causing api helper test to …

76840dc

…fail

Added unittest

b9f08bd

Fixes

f4b3a44

Integrated logic between consuming streams and

87f6180

one-shot requests

Fixed failing unit tests due to uninitialized DataDecoder

dd20437

Removed not_reached

094ae73

Added support for non-stream AIChatAPI via feature flag

fc6db1c

Feedback fixes

256ef37

Cleanup function body

69dec66

set in progress flag to false

04169a7

nullhook force-pushed the ai_chat_streams branch from 4dccfcf to 04169a7 Compare June 6, 2023 22:52

petemill approved these changes Jun 7, 2023

View reviewed changes

nullhook merged commit 3e9852e into master Jun 7, 2023

nullhook deleted the ai_chat_streams branch June 7, 2023 19:22

github-actions bot added this to the 1.54.x - Nightly milestone Jun 7, 2023

github-actions bot added feature/web3/wallet feature/web3/wallet/core labels Jun 7, 2023

petemill mentioned this pull request Jun 11, 2023

APIRequestHelper doesn't create DataDecoder until request needs parsing #18855

Merged

25 tasks

atuchin-m reviewed Jun 12, 2023

View reviewed changes

		@@ -1,4 +1,5 @@
		include_rules = [
		"+services/data_decoder/public",

		ResponseCallback response_callback_;
		CompletionCallback completion_callback_;

Added text streams to AIChat UI #18376

Added text streams to AIChat UI #18376

Conversation

nullhook commented May 5, 2023 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

brave-builds commented May 8, 2023

brave-builds commented May 8, 2023

brave-builds commented May 9, 2023

brave-builds commented May 9, 2023

brave-builds commented May 9, 2023

sangwoo108 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

nullhook May 15, 2023 • edited Loading

Choose a reason for hiding this comment

sangwoo108 May 16, 2023 • edited Loading

Choose a reason for hiding this comment

sangwoo108 left a comment

Choose a reason for hiding this comment

brave-builds commented May 12, 2023

sangwoo108 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goodov left a 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 May 15, 2023

brave-builds commented May 15, 2023

brave-builds commented May 15, 2023

petemill 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nullhook May 24, 2023 • edited Loading

Choose a reason for hiding this comment

brave-builds commented May 15, 2023

brave-builds commented May 18, 2023

brave-builds commented Jun 6, 2023

brave-builds commented Jun 6, 2023

brave-builds commented Jun 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nullhook commented May 5, 2023 •

edited

Loading

nullhook May 15, 2023 •

edited

Loading

sangwoo108 May 16, 2023 •

edited

Loading

nullhook May 24, 2023 •

edited

Loading