aichat: replace retry action on rate-limit prompt #22695

nullhook · 2024-03-20T18:50:14Z

This PR replaces the redundant 'Retry' action, which appears when a user hits a rate limit error, with 'Maybe Later.' This new action dismisses the prompt and returns the failed message to the 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-03-20T19:55:51Z

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

mkarolin

strings++

brave-builds · 2024-03-25T13:01:40Z

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

brave-builds · 2024-03-25T16:04:09Z

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

petemill · 2024-03-25T22:08:10Z

components/ai_chat/core/browser/conversation_driver.cc

+  mojom::ConversationTurnPtr turn = chat_history_.back().Clone();
+  chat_history_.pop_back();


instead of cloning, can't we just move the entry, since we're getting rid of it anyway?

yes, you can, but before sending it to the IPC, you'd have to convert it into a mojo::StructPtr<T> so, there's no point in returning a move-only value here because we will end up cloning anyways.

most mojo bindings code expects to deal with MyStructPtr version and i think it was a rookie mistake from my side to use MyStruct version for the history vector. i.e std::vector<mojom::ConversationTurn> chat_history_;

note to self: put a todo

ok, let's put a TODO in the header file for the vector class member

petemill · 2024-03-25T22:08:26Z

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

@@ -183,6 +183,7 @@ interface PageHandler {
  ClearConversationHistory();
  RetryAPIRequest();
  GetAPIResponseError() => (APIError error);
+  ClearErrorAndGetFailedMessage() => (ConversationTurn turn);


need comment

added comment

...and removes it from the history... (not clear from the function name)

petemill · 2024-03-25T22:16:01Z

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

@@ -205,7 +208,7 @@ function Main() {
            <PageContextToggle />
          </div>
        )}
-        <InputBox />
+        <InputBox inputText={failedInputText} />


The ergonomics of both this inputText prop and the onFailed function prop doesn't seem quite right to me. The behavior is not consistent with how we would normally expect a prop with the name "inputText" to be handled. How about we move the current input text state from InputBox to the context, and handle the submission to the conversation there, as well as the resetting of the input text when it fails. So that we have all that logic in the context instead of in the UI coimponents?

i did think about lifting the inputText state to the context, but since it's a high-frequency updated value, the whole context tree would re-render and can lead to side effects. the current approach in the PR takes the minimal route because of that. now, you'd say i could create a whole new context for input-related states, and i'd tell you 'maybe,' but then, is it worth adding another layer when this is used only for one specific scenario? also, redux shouldn't be the answer.

ok, so there will be upcoming use cases where we will need to control the inputText therefore i have lifted the state to context.

brave-builds · 2024-03-28T19:22:46Z

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

brave-builds · 2024-04-01T21:37:05Z

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

petemill · 2024-04-02T01:10:00Z

components/ai_chat/core/browser/conversation_driver.h

@@ -213,6 +214,7 @@ class ConversationDriver {

  // TODO(nullhook): Abstract the data model
  std::string model_key_;
+  // TODO(nullhook): Change this to mojo::StructPtr


nit: mojo = mojom

there's no such thing as mojom::StructPtr. its part of the mojo namespace. actually, the right notation should be: "Change this to mojo::StructPtr<T>"

petemill · 2024-04-02T01:14:07Z

components/ai_chat/resources/page/components/alerts/error_rate_limit.tsx

+    const handleMaybeLater = () => {
+      getPageHandlerInstance().pageHandler.clearErrorAndGetFailedMessage()
+        .then((res) => { context.setInputText(res.turn.text) })
+    }


probably best that all this logic is contained in the context (via a function), no? In my mind, that was the advantage of moving the state up to the context...

moved to context

petemill · 2024-04-02T01:15:16Z

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

    if (isCharLimitExceeded) return
    if (context.shouldDisableUserInput) return

-    getPageHandlerInstance().pageHandler.submitHumanConversationEntry(inputText)
-    setInputText('')
+    getPageHandlerInstance().pageHandler.submitHumanConversationEntry(context.inputText)


we should probably also move these calls to the context, shouldn't we?

moved to context

brave-builds · 2024-04-02T15:56:54Z

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

brave-builds · 2024-04-02T19:08:08Z

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

nullhook requested a review from a team as a code owner March 20, 2024 18:50

github-actions bot assigned nullhook Mar 20, 2024

github-actions bot added the CI/storybook-url Deploy storybook and provide a unique URL for each build label Mar 20, 2024

mkarolin approved these changes Mar 21, 2024

View reviewed changes

nullhook requested a review from petemill March 21, 2024 18:12

nullhook force-pushed the aichat-ratelimit-dialog branch from c2b9d1c to 138f58e Compare March 25, 2024 12:15

petemill reviewed Mar 25, 2024

View reviewed changes

nullhook force-pushed the aichat-ratelimit-dialog branch from ed3936e to 9814336 Compare March 28, 2024 18:23

nullhook requested a review from petemill March 28, 2024 18:24

nullhook force-pushed the aichat-ratelimit-dialog branch from 9814336 to 6305043 Compare April 1, 2024 20:52

petemill reviewed Apr 2, 2024

View reviewed changes

aichat: replace retry action on rate-limit prompt

e51c49f

nullhook force-pushed the aichat-ratelimit-dialog branch from 6305043 to e51c49f Compare April 2, 2024 15:19

resolved feedback

4b15414

nullhook requested a review from petemill April 2, 2024 18:30

petemill approved these changes Apr 3, 2024

View reviewed changes

nullhook merged commit 1483bbb into master Apr 3, 2024
19 checks passed

nullhook deleted the aichat-ratelimit-dialog branch April 3, 2024 14:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

aichat: replace retry action on rate-limit prompt #22695

aichat: replace retry action on rate-limit prompt #22695

nullhook commented Mar 20, 2024 •

edited

brave-builds commented Mar 20, 2024

mkarolin left a comment

brave-builds commented Mar 25, 2024

brave-builds commented Mar 25, 2024

petemill Mar 25, 2024

nullhook Mar 26, 2024 •

edited

nullhook Mar 28, 2024

petemill Mar 28, 2024

nullhook Mar 28, 2024 •

edited

petemill Mar 25, 2024

nullhook Mar 28, 2024

petemill Apr 2, 2024

petemill Mar 25, 2024

nullhook Mar 26, 2024

nullhook Mar 28, 2024

brave-builds commented Mar 28, 2024

brave-builds commented Apr 1, 2024

petemill Apr 2, 2024

nullhook Apr 2, 2024 •

edited

petemill Apr 2, 2024 •

edited

nullhook Apr 2, 2024

petemill Apr 2, 2024

nullhook Apr 2, 2024

brave-builds commented Apr 2, 2024

brave-builds commented Apr 2, 2024

		mojom::ConversationTurnPtr turn = chat_history_.back().Clone();
		chat_history_.pop_back();

aichat: replace retry action on rate-limit prompt #22695

aichat: replace retry action on rate-limit prompt #22695

Conversation

nullhook commented Mar 20, 2024 • edited

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

brave-builds commented Mar 20, 2024

mkarolin left a comment

Choose a reason for hiding this comment

brave-builds commented Mar 25, 2024

brave-builds commented Mar 25, 2024

Choose a reason for hiding this comment

nullhook Mar 26, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nullhook Mar 28, 2024 • edited

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 Mar 28, 2024

brave-builds commented Apr 1, 2024

Choose a reason for hiding this comment

nullhook Apr 2, 2024 • edited

Choose a reason for hiding this comment

petemill Apr 2, 2024 • edited

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 Apr 2, 2024

brave-builds commented Apr 2, 2024

nullhook commented Mar 20, 2024 •

edited

nullhook Mar 26, 2024 •

edited

nullhook Mar 28, 2024 •

edited

nullhook Apr 2, 2024 •

edited

petemill Apr 2, 2024 •

edited