From 7f157d8c5ca237acc7e8cc3657d4ca8c722a7a1b Mon Sep 17 00:00:00 2001 From: Pete Miller Date: Tue, 10 Dec 2024 14:22:14 -0800 Subject: [PATCH] feedback --- browser/ai_chat/ai_chat_throttle_unittest.cc | 15 ++++----------- browser/ui/webui/ai_chat/ai_chat_ui.cc | 9 ++------- .../ai_chat_untrusted_conversation_ui.cc | 9 +++------ .../core/browser/conversation_handler.cc | 14 +++++++++----- components/ai_chat/resources/common/api.ts | 10 ++++++---- .../components/action_type_label/index.tsx | 8 ++++---- .../action_type_label/style.module.scss | 0 .../ai_chat/resources/common/useAPIState.ts | 8 ++++---- .../ai_chat/resources/page/ai_chat_ui.html | 5 +++-- components/ai_chat/resources/page/chat_ui.tsx | 6 +++--- .../page/components/feedback_form/index.tsx | 6 +----- .../page/components/input_box/index.tsx | 10 ++++------ .../resources/page/components/main/index.tsx | 18 +++++++++--------- .../page/components/main/style.module.scss | 4 ++-- .../page/components/model_intro/index.tsx | 3 ++- .../components/suggested_question/index.tsx | 2 +- .../resources/page/state/ai_chat_context.tsx | 4 ++-- .../components/conversation_entries/index.tsx | 12 ++++++------ .../page_context_message/long_page_info.tsx | 2 +- .../untrusted_conversation_frame.html | 3 ++- .../untrusted_conversation_frame.tsx | 1 - ios/browser/api/ai_chat/BUILD.gn | 4 ++++ 22 files changed, 72 insertions(+), 81 deletions(-) rename components/ai_chat/resources/{untrusted_conversation_frame => common}/components/action_type_label/index.tsx (98%) rename components/ai_chat/resources/{untrusted_conversation_frame => common}/components/action_type_label/style.module.scss (100%) diff --git a/browser/ai_chat/ai_chat_throttle_unittest.cc b/browser/ai_chat/ai_chat_throttle_unittest.cc index 263c4637c528..e88c69e0e0f7 100644 --- a/browser/ai_chat/ai_chat_throttle_unittest.cc +++ b/browser/ai_chat/ai_chat_throttle_unittest.cc @@ -22,15 +22,8 @@ namespace ai_chat { namespace { -constexpr char kTestProfileName[] = "TestProfile"; - -GURL GetAIChatUIURL() { - return GURL(kAIChatUIURL); -} -GURL GetAIChatUntrustedConversationUIURL() { - return GURL(kAIChatUntrustedConversationUIURL); -} +constexpr char kTestProfileName[] = "TestProfile"; } // namespace @@ -84,7 +77,7 @@ INSTANTIATE_TEST_SUITE_P( TEST_P(AiChatThrottleUnitTest, CancelNavigationFromTab) { content::MockNavigationHandle test_handle(web_contents()); - test_handle.set_url(GetAIChatUIURL()); + test_handle.set_url(GURL(kAIChatUIURL)()); #if BUILDFLAG(IS_ANDROID) ui::PageTransition transition = ui::PageTransitionFromInt( @@ -112,7 +105,7 @@ TEST_P(AiChatThrottleUnitTest, CancelNavigationFromTab) { TEST_P(AiChatThrottleUnitTest, CancelNavigationToFrame) { content::MockNavigationHandle test_handle(web_contents()); - test_handle.set_url(GetAIChatUntrustedConversationUIURL()); + test_handle.set_url(GURL(kAIChatUntrustedConversationUIURL)()); #if BUILDFLAG(IS_ANDROID) ui::PageTransition transition = ui::PageTransitionFromInt( @@ -135,7 +128,7 @@ TEST_P(AiChatThrottleUnitTest, CancelNavigationToFrame) { TEST_P(AiChatThrottleUnitTest, AllowNavigationFromPanel) { content::MockNavigationHandle test_handle(web_contents()); - test_handle.set_url(GetAIChatUIURL()); + test_handle.set_url(GURL(kAIChatUIURL)()); #if BUILDFLAG(IS_ANDROID) ui::PageTransition transition = diff --git a/browser/ui/webui/ai_chat/ai_chat_ui.cc b/browser/ui/webui/ai_chat/ai_chat_ui.cc index f14696ae606c..0c3d40894c54 100644 --- a/browser/ui/webui/ai_chat/ai_chat_ui.cc +++ b/browser/ui/webui/ai_chat/ai_chat_ui.cc @@ -79,12 +79,7 @@ AIChatUI::AIChatUI(content::WebUI* web_ui) brave_l10n::GetLocalizedResourceUTF16String(str.id)); } -#if BUILDFLAG(IS_ANDROID) || BUILDFLAG(IS_IOS) - constexpr bool kIsMobile = true; -#else - constexpr bool kIsMobile = false; -#endif - + constexpr bool kIsMobile = BUILDFLAG(IS_ANDROID) || BUILDFLAG(IS_IOS); source->AddBoolean("isMobile", kIsMobile); source->AddBoolean("isHistoryEnabled", ai_chat::features::IsAIChatHistoryEnabled()); @@ -101,7 +96,7 @@ AIChatUI::AIChatUI(content::WebUI* web_ui) "img-src 'self' blob: chrome://resources chrome://favicon2;"); source->OverrideContentSecurityPolicy( network::mojom::CSPDirectiveName::FontSrc, - "font-src 'self' data: chrome://resources;"); + "font-src 'self' chrome://resources;"); source->OverrideContentSecurityPolicy( network::mojom::CSPDirectiveName::ChildSrc, base::StringPrintf("child-src %s;", kAIChatUntrustedConversationUIURL)); diff --git a/browser/ui/webui/ai_chat/ai_chat_untrusted_conversation_ui.cc b/browser/ui/webui/ai_chat/ai_chat_untrusted_conversation_ui.cc index 875862d7da0f..f86a077dad01 100644 --- a/browser/ui/webui/ai_chat/ai_chat_untrusted_conversation_ui.cc +++ b/browser/ui/webui/ai_chat/ai_chat_untrusted_conversation_ui.cc @@ -20,7 +20,6 @@ #include "brave/components/ai_chat/resources/grit/ai_chat_ui_generated_map.h" #include "brave/components/constants/webui_url_constants.h" #include "brave/components/l10n/common/localization_util.h" -#include "chrome/browser/ui/browser.h" #include "chrome/browser/ui/webui/webui_util.h" #include "components/grit/brave_components_resources.h" #include "content/public/browser/web_contents.h" @@ -29,6 +28,8 @@ #if BUILDFLAG(IS_ANDROID) #include "brave/browser/ui/android/ai_chat/brave_leo_settings_launcher_helper.h" +#else +#include "chrome/browser/ui/browser.h" #endif namespace { @@ -137,11 +138,7 @@ AIChatUntrustedConversationUI::AIChatUntrustedConversationUI( brave_l10n::GetLocalizedResourceUTF16String(str.id)); } -#if BUILDFLAG(IS_ANDROID) || BUILDFLAG(IS_IOS) - constexpr bool kIsMobile = true; -#else - constexpr bool kIsMobile = false; -#endif + constexpr bool kIsMobile = BUILDFLAG(IS_ANDROID) || BUILDFLAG(IS_IOS); source->AddBoolean("isMobile", kIsMobile); source->OverrideContentSecurityPolicy( diff --git a/components/ai_chat/core/browser/conversation_handler.cc b/components/ai_chat/core/browser/conversation_handler.cc index 8c83d41d2602..50d8e5384587 100644 --- a/components/ai_chat/core/browser/conversation_handler.cc +++ b/components/ai_chat/core/browser/conversation_handler.cc @@ -51,8 +51,6 @@ #include "brave/components/ai_chat/core/browser/types.h" #include "brave/components/ai_chat/core/browser/utils.h" #include "brave/components/ai_chat/core/common/features.h" -#include "brave/components/ai_chat/core/common/mojom/ai_chat.mojom-forward.h" -#include "brave/components/ai_chat/core/common/mojom/ai_chat.mojom-shared.h" #include "brave/components/ai_chat/core/common/mojom/ai_chat.mojom.h" #include "brave/components/api_request_helper/api_request_helper.h" #include "components/grit/brave_components_strings.h" @@ -544,8 +542,14 @@ void ConversationHandler::RateMessage(bool is_liked, base::BindOnce( [](RateMessageCallback callback, APIRequestResult result) { if (result.Is2XXResponseCode() && result.value_body().is_dict()) { - std::string id = *result.value_body().GetDict().FindString("id"); - std::move(callback).Run(id); + const std::string* id_result = + result.value_body().GetDict().FindString("id"); + if (id_result) { + std::move(callback).Run(*id_result); + } else { + DLOG(ERROR) << "Failed to get rating ID"; + std::move(callback).Run(std::nullopt); + } return; } DLOG(ERROR) << "Failed to send rating: " << result.response_code(); @@ -1636,7 +1640,7 @@ ConversationHandler::GetStateForConversationEntries() { entries_state->is_content_refined = is_content_refined_; entries_state->is_leo_model = is_leo_model; entries_state->content_used_percentage = - (metadata_->associated_content->is_content_association_possible == true) + metadata_->associated_content->is_content_association_possible ? std::make_optional( metadata_->associated_content->content_used_percentage) : std::nullopt; diff --git a/components/ai_chat/resources/common/api.ts b/components/ai_chat/resources/common/api.ts index e77294342623..01bbaceb7799 100644 --- a/components/ai_chat/resources/common/api.ts +++ b/components/ai_chat/resources/common/api.ts @@ -13,23 +13,25 @@ export default abstract class API { this.state = {...defaultState} } + // Debounce multiple state changes in the same task causing multiple + // events to be dispatched. private dispatchDebouncedStateChange = debounce(() => { - console.debug('dispatching uistatechange event', {...this.state}) this.eventTarget.dispatchEvent( - new Event('uistatechange') + new Event('statechange') ) }, 0) public setPartialState(partialState: Partial) { + console.log('setPartialState', partialState) this.state = { ...this.state, ...partialState } this.dispatchDebouncedStateChange() } public addStateChangeListener(callback: (event: Event) => void) { - this.eventTarget.addEventListener('uistatechange', callback) + this.eventTarget.addEventListener('statechange', callback) } public removeStateChangeListener(callback: (event: Event) => void) { - this.eventTarget.removeEventListener('uistatechange', callback) + this.eventTarget.removeEventListener('statechange', callback) } } diff --git a/components/ai_chat/resources/untrusted_conversation_frame/components/action_type_label/index.tsx b/components/ai_chat/resources/common/components/action_type_label/index.tsx similarity index 98% rename from components/ai_chat/resources/untrusted_conversation_frame/components/action_type_label/index.tsx rename to components/ai_chat/resources/common/components/action_type_label/index.tsx index 4c4a148655ec..5e6dd3630b58 100644 --- a/components/ai_chat/resources/untrusted_conversation_frame/components/action_type_label/index.tsx +++ b/components/ai_chat/resources/common/components/action_type_label/index.tsx @@ -3,12 +3,12 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this file, * You can obtain one at https://mozilla.org/MPL/2.0/. */ -import * as React from 'react' +import Button from '@brave/leo/react/button' import Icon from '@brave/leo/react/icon' -import * as Mojom from '../../../common/mojom' -import styles from './style.module.scss' +import * as React from 'react' import { getLocale } from '$web-common/locale' -import Button from '@brave/leo/react/button' +import * as Mojom from '../../mojom' +import styles from './style.module.scss' interface ActionTypeLabelProps { actionType: Mojom.ActionType diff --git a/components/ai_chat/resources/untrusted_conversation_frame/components/action_type_label/style.module.scss b/components/ai_chat/resources/common/components/action_type_label/style.module.scss similarity index 100% rename from components/ai_chat/resources/untrusted_conversation_frame/components/action_type_label/style.module.scss rename to components/ai_chat/resources/common/components/action_type_label/style.module.scss diff --git a/components/ai_chat/resources/common/useAPIState.ts b/components/ai_chat/resources/common/useAPIState.ts index 3990ca664aba..27044cdc5c91 100644 --- a/components/ai_chat/resources/common/useAPIState.ts +++ b/components/ai_chat/resources/common/useAPIState.ts @@ -6,19 +6,19 @@ import * as React from 'react' import API from './api' -export default function useAPIState(api: API, defaultState: T) { +export default function useAPIState(api: API, defaultState: T) { // Intialize with global state that may have been set between module-load // time and the first React render. const [context, setContext] = React.useState({ ...defaultState, ...api.state }) - const updateFromAPIState = (state: Y) => { + const updateFromAPIState = React.useCallback((state: T) => { setContext((value) => ({ ...value, ...state })) - } + }, []) React.useEffect(() => { // Update with any global state change that may have occurred between @@ -34,6 +34,6 @@ export default function useAPIState(api: API, defaultState: T) { return () => { api.removeStateChangeListener(onGlobalStateChange) } - }, []) + }, [updateFromAPIState]) return context } diff --git a/components/ai_chat/resources/page/ai_chat_ui.html b/components/ai_chat/resources/page/ai_chat_ui.html index 4da2718b97b5..84f31929e298 100644 --- a/components/ai_chat/resources/page/ai_chat_ui.html +++ b/components/ai_chat/resources/page/ai_chat_ui.html @@ -9,9 +9,11 @@ + + - +