Don't add as a separate shortcut when built-in provide related item

browser/ui/sidebar/BUILD.gn

@@ -89,6 +89,7 @@ source_set("unit_tests") {
       "//base",
       "//brave/browser/ui",
       "//brave/browser/ui/sidebar",
+      "//brave/common",
       "//brave/components/sidebar",
       "//chrome/test:test_support",
       "//components/prefs",

browser/ui/sidebar/sidebar_model.cc

@@ -221,7 +221,7 @@ bool SidebarModel::IsLoadedAt(int index) const {
 }
 
 bool SidebarModel::IsSidebarHasAllBuiltiInItems() const {
-  return GetSidebarService(profile_)->GetNotAddedDefaultSidebarItems().empty();
+  return GetSidebarService(profile_)->GetHiddenDefaultSidebarItems().empty();
 }
 
 int SidebarModel::GetIndexOf(const SidebarItem& item) const {

browser/ui/sidebar/sidebar_unittest.cc

@@ -9,12 +9,17 @@
 #include "base/test/scoped_feature_list.h"
 #include "brave/browser/ui/sidebar/sidebar_model.h"
 #include "brave/browser/ui/sidebar/sidebar_service_factory.h"
+#include "brave/browser/ui/sidebar/sidebar_utils.h"
+#include "brave/common/webui_url_constants.h"
+#include "brave/components/sidebar/constants.h"
 #include "brave/components/sidebar/sidebar_service.h"
+#include "chrome/common/url_constants.h"
 #include "chrome/test/base/testing_profile.h"
 #include "components/prefs/pref_service.h"
 #include "components/sync_preferences/testing_pref_service_syncable.h"
 #include "content/public/test/browser_task_environment.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "url/gurl.h"
 
 namespace sidebar {
 
@@ -118,4 +123,32 @@ TEST_F(SidebarModelTest, ItemsChangedTest) {
   EXPECT_EQ(3, model()->active_index());
 }
 
+TEST_F(SidebarModelTest, CanUseNotAddedBuiltInItemInsteadOfTest) {
+  GURL talk("https://talk.brave.com/1Ar1vHfLBWX2sAdi");
+  // False because builtin talk item is already added.
+  EXPECT_FALSE(HiddenDefaultSidebarItemsContains(service(), talk));
+
+  // Remove builtin talk item and check builtin talk item will be used
+  // instead of adding |talk| url.
+  service()->RemoveItemAt(0);
+  EXPECT_TRUE(HiddenDefaultSidebarItemsContains(service(), talk));
+}
+
+TEST(SidebarUtilTest, ConvertURLToBuiltInItemURLTest) {
+  EXPECT_EQ(GURL(kBraveTalkURL),
+            ConvertURLToBuiltInItemURL(GURL("https://talk.brave.com")));
+  EXPECT_EQ(GURL(kBraveTalkURL),
+            ConvertURLToBuiltInItemURL(
+                GURL("https://talk.brave.com/1Ar1vHfLBWX2sAdi")));
+  EXPECT_EQ(GURL(kSidebarBookmarksURL),
+            ConvertURLToBuiltInItemURL(GURL(chrome::kChromeUIBookmarksURL)));
+  EXPECT_EQ(
+      GURL(kBraveUIWalletPageURL),
+      ConvertURLToBuiltInItemURL(GURL("chrome://wallet/crypto/onboarding")));
+
+  // Not converted for url that doesn't relavant builtin item.
+  GURL brave_com("https://www.brave.com/");
+  EXPECT_EQ(brave_com, ConvertURLToBuiltInItemURL(brave_com));
+}
+
 }  // namespace sidebar

browser/ui/sidebar/sidebar_utils.cc

@@ -6,15 +6,19 @@
 #include "brave/browser/ui/sidebar/sidebar_utils.h"
 
 #include "brave/browser/ui/sidebar/sidebar_service_factory.h"
+#include "brave/common/webui_url_constants.h"
+#include "brave/components/sidebar/constants.h"
 #include "brave/components/sidebar/sidebar_item.h"
 #include "brave/components/sidebar/sidebar_service.h"
 #include "chrome/browser/search/search.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/browser/ui/webui/new_tab_page/new_tab_page_ui.h"
 #include "chrome/browser/ui/webui/ntp/new_tab_ui.h"
+#include "chrome/common/webui_url_constants.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/web_contents.h"
+#include "content/public/common/url_constants.h"
 
 namespace sidebar {
 
@@ -24,24 +28,22 @@ SidebarService* GetSidebarService(Browser* browser) {
   return SidebarServiceFactory::GetForProfile(browser->profile());
 }
 
-bool IsActiveTabNTP(Browser* browser) {
-  auto* web_contents = browser->tab_strip_model()->GetActiveWebContents();
+bool IsActiveTabNTP(content::WebContents* active_web_contents) {
   content::NavigationEntry* entry =
-      web_contents->GetController().GetLastCommittedEntry();
+      active_web_contents->GetController().GetLastCommittedEntry();
   if (!entry)
-    entry = web_contents->GetController().GetVisibleEntry();
+    entry = active_web_contents->GetController().GetVisibleEntry();
   if (!entry)
     return false;
   const GURL url = entry->GetURL();
   return NewTabUI::IsNewTab(url) || NewTabPageUI::IsNewTabPageOrigin(url) ||
-         search::NavEntryIsInstantNTP(web_contents, entry);
+         search::NavEntryIsInstantNTP(active_web_contents, entry);
 }
 
-bool IsActiveTabAlreadyAddedToSidebar(Browser* browser) {
-  auto* web_contents = browser->tab_strip_model()->GetActiveWebContents();
-  const GURL url = web_contents->GetVisibleURL();
-  for (const auto& item : GetSidebarService(browser)->items()) {
-    if (item.url == url)
+bool IsURLAlreadyAddedToSidebar(SidebarService* service, const GURL& url) {
+  const GURL converted_url = ConvertURLToBuiltInItemURL(url);
+  for (const auto& item : service->items()) {
+    if (item.url == converted_url)
       return true;
   }
 
@@ -50,20 +52,58 @@ bool IsActiveTabAlreadyAddedToSidebar(Browser* browser) {
 
 }  // namespace
 
+bool HiddenDefaultSidebarItemsContains(SidebarService* service,
+                                       const GURL& url) {
+  const auto not_added_default_items = service->GetHiddenDefaultSidebarItems();
+  if (not_added_default_items.empty())
+    return false;
+  const GURL converted_url = ConvertURLToBuiltInItemURL(url);
+  for (const auto& item : not_added_default_items) {
+    if (item.url == converted_url)
+      return true;
+  }
+  return false;
+}
+
 bool CanUseSidebar(Browser* browser) {
   DCHECK(browser);
   return browser->is_type_normal();
 }
 
+// If url is relavant with bulitin items, use builtin item's url.
+// Ex, we don't need to add bookmarks manager as a sidebar shortcut
+// if sidebar panel already has bookmarks item.
+GURL ConvertURLToBuiltInItemURL(const GURL& url) {
+  if (url == GURL(chrome::kChromeUIBookmarksURL))
+    return GURL(kSidebarBookmarksURL);
+
+  if (url.host() == kBraveTalkHost)
+    return GURL(kBraveTalkURL);
+
+  if (url.SchemeIs(content::kChromeUIScheme) && url.host() == kWalletPageHost) {
+    return GURL(kBraveUIWalletPageURL);
+  }
+  return url;
+}
+
 bool CanAddCurrentActiveTabToSidebar(Browser* browser) {
-  auto* web_contents = browser->tab_strip_model()->GetActiveWebContents();
-  if (!web_contents)
+  auto* active_web_contents =
+      browser->tab_strip_model()->GetActiveWebContents();
+  if (!active_web_contents)
+    return false;
+
+  if (IsActiveTabNTP(active_web_contents))
+    return false;
+
+  const GURL url = active_web_contents->GetLastCommittedURL();
+  if (!url.is_valid())
     return false;
 
-  if (IsActiveTabNTP(browser))
+  auto* service = GetSidebarService(browser);
+  if (IsURLAlreadyAddedToSidebar(service, url))
     return false;
 
-  if (IsActiveTabAlreadyAddedToSidebar(browser))
+  if (HiddenDefaultSidebarItemsContains(service, url))
     return false;
 
   return true;

browser/ui/sidebar/sidebar_utils.h

@@ -7,12 +7,20 @@
 #define BRAVE_BROWSER_UI_SIDEBAR_SIDEBAR_UTILS_H_
 
 class Browser;
+class GURL;
 
 namespace sidebar {
 
+class SidebarService;
+
 bool CanUseSidebar(Browser* browser);
 bool CanAddCurrentActiveTabToSidebar(Browser* browser);
 
+// Exported for testing.
+bool HiddenDefaultSidebarItemsContains(SidebarService* service,
+                                       const GURL& url);
+GURL ConvertURLToBuiltInItemURL(const GURL& url);
+
 }  // namespace sidebar
 
 #endif  // BRAVE_BROWSER_UI_SIDEBAR_SIDEBAR_UTILS_H_

browser/ui/views/sidebar/sidebar_add_item_bubble_delegate_view.cc

@@ -175,7 +175,7 @@ void SidebarAddItemBubbleDelegateView::AddChildViews() {
   }
 
   const auto not_added_default_items =
-      GetSidebarService(browser_)->GetNotAddedDefaultSidebarItems();
+      GetSidebarService(browser_)->GetHiddenDefaultSidebarItems();
   if (not_added_default_items.empty())
     return;
 

components/sidebar/constants.h

@@ -19,6 +19,9 @@ constexpr char kSidebarItemOpenInPanelKey[] = "open_in_panel";
 constexpr char kSidebarBookmarksURL[] =
     "chrome://sidebar-bookmarks.top-chrome/";
 
+constexpr char kBraveTalkURL[] = "https://talk.brave.com/widget";
+constexpr char kBraveTalkHost[] = "talk.brave.com";
+
 }  // namespace sidebar
 
 #endif  // BRAVE_COMPONENTS_SIDEBAR_CONSTANTS_H_

components/sidebar/sidebar_service.cc

@@ -27,7 +27,7 @@ SidebarItem GetBuiltInItemForType(SidebarItem::BuiltInItemType type) {
   switch (type) {
     case SidebarItem::BuiltInItemType::kBraveTalk:
       return SidebarItem::Create(
-          GURL("https://talk.brave.com/widget"),
+          GURL(kBraveTalkURL),
           l10n_util::GetStringUTF16(IDS_SIDEBAR_BRAVE_TALK_ITEM_TITLE),
           SidebarItem::Type::kTypeBuiltIn,
           SidebarItem::BuiltInItemType::kBraveTalk, false);
@@ -187,8 +187,7 @@ void SidebarService::RemoveObserver(Observer* observer) {
   observers_.RemoveObserver(observer);
 }
 
-std::vector<SidebarItem> SidebarService::GetNotAddedDefaultSidebarItems()
-    const {
+std::vector<SidebarItem> SidebarService::GetHiddenDefaultSidebarItems() const {
   auto default_items = GetDefaultSidebarItems();
   const auto added_default_items = GetDefaultSidebarItemsFromCurrentItems();
   for (const auto& added_item : added_default_items) {

components/sidebar/sidebar_service.h

@@ -55,7 +55,7 @@ class SidebarService : public KeyedService {
   void AddObserver(Observer* observer);
   void RemoveObserver(Observer* observer);
 
-  std::vector<SidebarItem> GetNotAddedDefaultSidebarItems() const;
+  std::vector<SidebarItem> GetHiddenDefaultSidebarItems() const;
   ShowSidebarOption GetSidebarShowOption() const;
   void SetSidebarShowOption(ShowSidebarOption show_options);
 

components/sidebar/sidebar_service_unittest.cc

@@ -78,7 +78,7 @@ TEST_F(SidebarServiceTest, AddRemoveItems) {
 
   // Check the default items count.
   EXPECT_EQ(3UL, service_->items().size());
-  EXPECT_EQ(0UL, service_->GetNotAddedDefaultSidebarItems().size());
+  EXPECT_EQ(0UL, service_->GetHiddenDefaultSidebarItems().size());
 
   // Cache 1st item to compare after removing.
   const SidebarItem item = service_->items()[0];
@@ -90,7 +90,7 @@ TEST_F(SidebarServiceTest, AddRemoveItems) {
 
   service_->RemoveItemAt(0);
   EXPECT_EQ(2UL, service_->items().size());
-  EXPECT_EQ(1UL, service_->GetNotAddedDefaultSidebarItems().size());
+  EXPECT_EQ(1UL, service_->GetHiddenDefaultSidebarItems().size());
   EXPECT_EQ(0, item_index_on_called_);
   EXPECT_TRUE(on_will_remove_item_called_);
   EXPECT_TRUE(on_item_removed_called_);
@@ -102,7 +102,7 @@ TEST_F(SidebarServiceTest, AddRemoveItems) {
   // New item will be added at last.
   EXPECT_EQ(2, item_index_on_called_);
   EXPECT_EQ(3UL, service_->items().size());
-  EXPECT_EQ(0UL, service_->GetNotAddedDefaultSidebarItems().size());
+  EXPECT_EQ(0UL, service_->GetHiddenDefaultSidebarItems().size());
   EXPECT_TRUE(on_item_added_called_);
   ClearState();
 
@@ -171,7 +171,7 @@ TEST_F(SidebarServiceTest, BuiltInItemUpdateTestWithBuiltInItemTypeKey) {
 
   // Check service has updated built-in item. Previously url was deprecated.xxx.
   EXPECT_EQ(1UL, service_->items().size());
-  EXPECT_EQ("https://talk.brave.com/widget", service_->items()[0].url);
+  EXPECT_EQ(GURL(kBraveTalkURL), service_->items()[0].url);
 
   // Simulate re-launch and check service has still updated items.
   service_->RemoveObserver(this);
@@ -181,7 +181,7 @@ TEST_F(SidebarServiceTest, BuiltInItemUpdateTestWithBuiltInItemTypeKey) {
 
   // Check service has updated built-in item. Previously url was deprecated.xxx.
   EXPECT_EQ(1UL, service_->items().size());
-  EXPECT_EQ("https://talk.brave.com/widget", service_->items()[0].url);
+  EXPECT_EQ(GURL(kBraveTalkURL), service_->items()[0].url);
 }
 
 TEST_F(SidebarServiceTest, BuiltInItemDoesntHaveHistoryItem) {
@@ -208,7 +208,7 @@ TEST_F(SidebarServiceTest, BuiltInItemDoesntHaveHistoryItem) {
   EXPECT_EQ(0UL, service_->items().size());
 
   // Make sure history is not included in the not added default items list.
-  auto not_added_default_items = service_->GetNotAddedDefaultSidebarItems();
+  auto not_added_default_items = service_->GetHiddenDefaultSidebarItems();
   EXPECT_EQ(3UL, not_added_default_items.size());
   for (const auto& item : not_added_default_items) {
     EXPECT_NE(SidebarItem::BuiltInItemType::kHistory, item.built_in_item_type);