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

[AI Chat] cleanup suggestion action types #27077

Merged
merged 1 commit into from
Jan 23, 2025
Merged

Conversation

petemill
Copy link
Member

@petemill petemill commented Dec 20, 2024

…and persist to database when prompt differs from user-visible conversation entry text

  • ActionType::CONVERSATION_STARTER
  • ActionType::SUGGESTION
  • ActionType::SUMMARIZE_{PAGE,VIDEO}

I believe this unblocks the following to be able to get the source of each user submission more easily:

Issues:

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:

Test Plan:

@petemill petemill requested review from bbondy and DJAndries December 20, 2024 09:27
@petemill petemill self-assigned this Dec 20, 2024
@petemill petemill requested review from a team as code owners December 20, 2024 09:27
@github-actions github-actions bot added CI/storybook-url Deploy storybook and provide a unique URL for each build puLL-Merge labels Dec 20, 2024
@petemill petemill mentioned this pull request Dec 20, 2024
24 tasks
@petemill petemill force-pushed the ai-chat-suggestion-cleanup branch from 90b79eb to 55df777 Compare December 20, 2024 10:40
Comment on lines 44 to 52
bool MigrateFrom1To2(sql::Database* db) {
// Add a new column to the associated_content table to store the content type.
static constexpr char kAddPromptColumnQuery[] =
"ALTER TABLE conversation_entry ADD COLUMN prompt BLOB "
"0";
sql::Statement statement(db->GetUniqueStatement(kAddPromptColumnQuery));

return statement.is_valid() && statement.Run();
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Database migration and tests

@petemill petemill force-pushed the ai-chat-suggestion-cleanup branch from 55df777 to baabfdb Compare December 20, 2024 10:56
@brave-builds
Copy link
Collaborator

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

@petemill petemill force-pushed the ai-chat-suggestion-cleanup branch 3 times, most recently from 1cd6ada to 8878883 Compare December 20, 2024 19:02
@@ -149,6 +148,8 @@ enum ActionType {
// Change length
SHORTEN,
EXPAND,
CONVERSATION_STARTER,
SUGGESTION,
Copy link
Collaborator

Choose a reason for hiding this comment

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

could you clarify what the difference is between a starter and a suggestion? are starters only be used when making the first prompt in the chat, and suggestions used for follow-ups?

Copy link
Member Author

Choose a reason for hiding this comment

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

A suggestion is generated by the LLM and is about the specific page content. A conversation starter is static and yes, only for the first message

Copy link
Contributor

@iefremov iefremov left a comment

Choose a reason for hiding this comment

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

chromium_src lgtm

@petemill petemill force-pushed the ai-chat-suggestion-cleanup branch from 8878883 to ecf087d Compare January 21, 2025 21:19
@brave-builds
Copy link
Collaborator

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

@petemill petemill force-pushed the ai-chat-suggestion-cleanup branch 4 times, most recently from 0160e6d to 44fa26e Compare January 23, 2025 07:08
Copy link
Contributor

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

Description

This PR makes significant changes to the AI chat functionality in Brave, primarily focusing on conversation turn handling and database structure. The main changes include:

  1. Removes ConversationTurnVisibility enum and replaces it with a more flexible prompt/text system
  2. Adds database migration support from version 1 to 2 to handle new prompt column
  3. Refactors engine consumers to use the new prompt system
  4. Adds proper suggestion handling via a dedicated API

The motivation appears to be improving the conversation model by separating displayed text from actual prompts sent to the AI, while also improving the handling of suggested questions.

Changes

Changes

Core Changes:

  • browser/ai_chat/android/ai_chat_utils_android.cc: Updates conversation turn creation to use new structure
  • components/ai_chat/core/browser/ai_chat_database.cc: Adds migration logic for v1 to v2 database schema
  • components/ai_chat/core/browser/conversation_handler.cc: Major refactoring to handle prompts and suggestions
  • components/ai_chat/core/common/mojom/ai_chat.mojom: Removes ConversationTurnVisibility, adds prompt field and new action types

Engine Changes:

  • Multiple engine consumer files updated to handle the new prompt system
  • Refactored prompt handling logic across Claude, Llama and OpenAI implementations

UI Changes:

  • iOS and web UI components updated to support new conversation structure
  • Added proper suggestion handling via new API endpoints
sequenceDiagram
    participant UI
    participant ConversationHandler
    participant EngineConsumer
    participant Database
    
    UI->>ConversationHandler: submitSuggestion(text)
    ConversationHandler->>ConversationHandler: CreateConversationTurn
    Note over ConversationHandler: Add prompt & text fields
    ConversationHandler->>EngineConsumer: GenerateAssistantResponse
    EngineConsumer->>EngineConsumer: GetPromptForEntry
    EngineConsumer-->>ConversationHandler: Response
    ConversationHandler->>Database: AddConversationEntry
    Database->>Database: EncryptAndStore
    Database-->>ConversationHandler: Success
    ConversationHandler-->>UI: Update
Loading

Security Hotspots

  1. Database migration handling - while the migration looks straightforward, failures during migration could potentially lead to data loss if not handled properly
  2. Encryption handling in the database operations - sensitive user queries and prompts need proper encryption

…n prompt differs from user-visible conversation entry text

- ActionType::CONVERSATION_STARTER
- ActionType::SUGGESTION
- ActionType::SUMMARIZE_{PAGE,VIDEO}
@petemill petemill force-pushed the ai-chat-suggestion-cleanup branch from 44fa26e to 07de4a4 Compare January 23, 2025 08:26
@petemill petemill merged commit 29342f1 into master Jan 23, 2025
18 checks passed
@petemill petemill deleted the ai-chat-suggestion-cleanup branch January 23, 2025 17:38
@github-actions github-actions bot added this to the 1.76.x - Nightly milestone Jan 23, 2025
@brave-builds
Copy link
Collaborator

Released in v1.76.36

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 puLL-Merge
Projects
None yet
5 participants