Try to update sidebar active item state if needed

browser/ui/sidebar/sidebar_browsertest.cc

@@ -314,14 +314,14 @@ IN_PROC_BROWSER_TEST_F(SidebarBrowserTest, BasicTest) {
   EXPECT_EQ(expected_count, model()->GetAllSidebarItems().size());
   // Activate item that opens in panel.
   const size_t first_panel_item_index = GetFirstPanelItemIndex();
-  controller()->ActivateItemAt(first_panel_item_index);
+  const auto& first_panel_item =
+      controller()->model()->GetAllSidebarItems()[first_panel_item_index];
+  controller()->ActivatePanelItem(first_panel_item.built_in_item_type);
+  WaitUntil(
+      base::BindLambdaForTesting([&]() { return !!model()->active_index(); }));
   EXPECT_THAT(model()->active_index(), Optional(first_panel_item_index));
   EXPECT_TRUE(controller()->IsActiveIndex(first_panel_item_index));
 
-  // Try to activate item at index 1.
-  // Default item at index 1 opens in new tab. So, sidebar active index is not
-  // changed. Still active index is 2.
-
   // Get first index of item that opens in panel.
   const size_t first_web_item_index = GetFirstWebItemIndex();
   const auto item = model()->GetAllSidebarItems()[first_web_item_index];
@@ -331,10 +331,15 @@ IN_PROC_BROWSER_TEST_F(SidebarBrowserTest, BasicTest) {
   EXPECT_THAT(model()->active_index(), Optional(active_item_index));
 
   // Setting std::nullopt means deactivate current active tab.
-  controller()->ActivateItemAt(std::nullopt);
+  controller()->DeactivateCurrentPanel();
+  WaitUntil(
+      base::BindLambdaForTesting([&]() { return !model()->active_index(); }));
   EXPECT_THAT(model()->active_index(), Eq(std::nullopt));
 
-  controller()->ActivateItemAt(active_item_index);
+  controller()->ActivatePanelItem(first_panel_item.built_in_item_type);
+  WaitUntil(
+      base::BindLambdaForTesting([&]() { return !!model()->active_index(); }));
+  EXPECT_THAT(model()->active_index(), Optional(active_item_index));
 
   auto* sidebar_service =
       SidebarServiceFactory::GetForProfile(browser()->profile());
@@ -1105,7 +1110,24 @@ IN_PROC_BROWSER_TEST_F(SidebarBrowserTestWithAIChat, TabSpecificPanel) {
   ASSERT_TRUE(global_item_index.has_value());
   auto tab_specific_item_index = model()->GetIndexOf(kTabSpecificItemType);
   ASSERT_TRUE(tab_specific_item_index.has_value());
-  // Open 2 more tabs
+
+  ASSERT_TRUE(ui_test_utils::NavigateToURLWithDisposition(
+      browser(), GURL("brave://newtab/"),
+      WindowOpenDisposition::NEW_FOREGROUND_TAB,
+      ui_test_utils::BROWSER_TEST_WAIT_FOR_LOAD_STOP));
+  ASSERT_EQ(tab_model()->GetTabCount(), 2);
+
+  // Open contextual panel from Tab 0.
+  tab_model()->ActivateTabAt(0);
+  SimulateSidebarItemClickAt(tab_specific_item_index.value());
+  EXPECT_EQ(model()->active_index(), tab_specific_item_index);
+
+  // Delete Tab 0 and check model doesn't have active index.
+  tab_model()->DetachAndDeleteWebContentsAt(0);
+  EXPECT_FALSE(!!model()->active_index());
+  ASSERT_EQ(tab_model()->GetTabCount(), 1);
+
+  // Create two more tab for test below.
   ASSERT_TRUE(ui_test_utils::NavigateToURLWithDisposition(
       browser(), GURL("brave://newtab/"),
       WindowOpenDisposition::NEW_FOREGROUND_TAB,
@@ -1115,6 +1137,7 @@ IN_PROC_BROWSER_TEST_F(SidebarBrowserTestWithAIChat, TabSpecificPanel) {
       WindowOpenDisposition::NEW_FOREGROUND_TAB,
       ui_test_utils::BROWSER_TEST_WAIT_FOR_LOAD_STOP));
   ASSERT_EQ(tab_model()->GetTabCount(), 3);
+
   // Open a "global" panel from Tab 0
   tab_model()->ActivateTabAt(0);
   // Tab changed flag should be cleared after ActivateTabAt() executed.

browser/ui/sidebar/sidebar_controller.cc

@@ -224,6 +224,18 @@ void SidebarController::AddItemWithCurrentTab() {
                           SidebarItem::BuiltInItemType::kNone, false));
 }
 
+void SidebarController::UpdateActiveItemState(
+    std::optional<SidebarItem::BuiltInItemType> active_panel_item) {
+  if (!active_panel_item) {
+    ActivateItemAt(std::nullopt);
+    return;
+  }
+
+  if (auto index = sidebar_model_->GetIndexOf(*active_panel_item)) {
+    ActivateItemAt(*index);
+  }
+}
+
 void SidebarController::SetSidebar(Sidebar* sidebar) {
   DCHECK(!sidebar_);
   // |sidebar| can be null in unit test.

browser/ui/sidebar/sidebar_controller.h

@@ -52,6 +52,8 @@ class SidebarController : public SidebarService::Observer {
       std::optional<size_t> index,
       WindowOpenDisposition disposition = WindowOpenDisposition::CURRENT_TAB);
   void AddItemWithCurrentTab();
+  void UpdateActiveItemState(std::optional<SidebarItem::BuiltInItemType>
+                                 active_panel_item = std::nullopt);
 
   // Ask panel item activation state change to SidePanelUI.
   void ActivatePanelItem(SidebarItem::BuiltInItemType panel_item);

browser/ui/sidebar/sidebar_utils.cc

@@ -149,6 +149,24 @@ SidePanelEntryId SidePanelIdFromSideBarItemType(BuiltInItemType type) {
          "not have a panel Id.";
 }
 
+std::optional<BuiltInItemType> BuiltInItemTypeFromSidePanelId(
+    SidePanelEntryId id) {
+  switch (id) {
+    case SidePanelEntryId::kReadingList:
+      return BuiltInItemType::kReadingList;
+    case SidePanelEntryId::kBookmarks:
+      return BuiltInItemType::kBookmarks;
+    case SidePanelEntryId::kPlaylist:
+      return BuiltInItemType::kPlaylist;
+    case SidePanelEntryId::kChatUI:
+      return BuiltInItemType::kChatUI;
+    default:
+      break;
+  }
+
+  return std::nullopt;
+}
+
 SidePanelEntryId SidePanelIdFromSideBarItem(const SidebarItem& item) {
   CHECK(item.open_in_panel) << static_cast<int>(item.built_in_item_type);
   return SidePanelIdFromSideBarItemType(item.built_in_item_type);

browser/ui/sidebar/sidebar_utils.h

@@ -29,6 +29,8 @@ GURL ConvertURLToBuiltInItemURL(const GURL& url);
 SidePanelEntryId SidePanelIdFromSideBarItemType(
     SidebarItem::BuiltInItemType type);
 SidePanelEntryId SidePanelIdFromSideBarItem(const SidebarItem& item);
+std::optional<SidebarItem::BuiltInItemType> BuiltInItemTypeFromSidePanelId(
+    SidePanelEntryId id);
 void SetLastUsedSidePanel(PrefService* prefs,
                           std::optional<SidePanelEntryId> id);
 std::optional<SidePanelEntryId> GetLastUsedSidePanel(Browser* browser);

browser/ui/views/sidebar/sidebar_container_view.cc

@@ -248,6 +248,15 @@ void SidebarContainerView::SetSidebarShowOption(ShowSidebarOption show_option) {
 void SidebarContainerView::UpdateSidebarItemsState() {
   // control view has items.
   sidebar_control_view_->Update();
+
+  // Calls whenever browser UI needs to be updated(such as
+  // active tab change) to make sidebar model has proper active item for
+  // currently opened panel. Most of time, sidebar model
+  // already has proper active item for current panel before calling this.
+  // In that case, calling this would be no-op because active index
+  // is not changed. This is helpful if we could miss the
+  // active index change timing even some side panel events happened.
+  UpdateActiveItemState();
 }
 
 void SidebarContainerView::MenuClosed() {
@@ -810,19 +819,18 @@ void SidebarContainerView::OnEntryWillDeregister(SidePanelRegistry* registry,
 void SidebarContainerView::OnRegistryDestroying(SidePanelRegistry* registry) {
   if (panel_registry_observations_.IsObservingSource(registry)) {
     StopObservingContextualSidePanelRegistry(registry);
-    // If this is the active tab being destroyed, then reset the active item.
-    // Note, that items persisted across tabs like reading list or bookmarks
-    // don't show as active_entry, in which case leave the active item as is.
-    if (registry == active_contextual_registry_) {
-      if (registry->active_entry()) {
-        auto* controller = browser_->sidebar_controller();
-        controller->ActivateItemAt(std::nullopt);
-      }
-      active_contextual_registry_ = nullptr;
-    }
   }
 }
 
+void SidebarContainerView::UpdateActiveItemState() {
+  auto* controller = browser_->sidebar_controller();
+  std::optional<sidebar::SidebarItem::BuiltInItemType> current_type;
+  if (auto entry_id = side_panel_coordinator_->GetCurrentEntryId()) {
+    current_type = sidebar::BuiltInItemTypeFromSidePanelId(*entry_id);
+  }
+  controller->UpdateActiveItemState(current_type);
+}
+
 void SidebarContainerView::OnTabStripModelChanged(
     TabStripModel* tab_strip_model,
     const TabStripModelChange& change,
@@ -853,11 +861,17 @@ void SidebarContainerView::OnTabStripModelChanged(
         }
       }
     }
-  } else if (change.type() == TabStripModelChange::kInserted) {
+    return;
+  }
+
+  if (change.type() == TabStripModelChange::kInserted) {
     for (const auto& contents : change.GetInsert()->contents) {
       StartObservingContextualSidePanelRegistry(contents.contents);
     }
-  } else if (change.type() == TabStripModelChange::kRemoved) {
+    return;
+  }
+
+  if (change.type() == TabStripModelChange::kRemoved) {
     bool removed_for_deletion =
         (change.GetRemove()->contents[0].remove_reason ==
          TabStripModelChange::RemoveReason::kDeleted);
@@ -869,18 +883,7 @@ void SidebarContainerView::OnTabStripModelChanged(
         StopObservingContextualSidePanelRegistry(contents.contents);
       }
     }
-  }
-
-  // Keep track of the active contextual registry
-  if (selection.active_tab_changed()) {
-    active_contextual_registry_ =
-        selection.new_contents
-            ? SidePanelRegistry::GetDeprecated(selection.new_contents)
-            : nullptr;
-    if (active_contextual_registry_) {
-      DCHECK(panel_registry_observations_.IsObservingSource(
-          active_contextual_registry_));
-    }
+    return;
   }
 }
 

browser/ui/views/sidebar/sidebar_container_view.h

@@ -186,13 +186,13 @@ class SidebarContainerView
       content::WebContents* contents);
   void StopObservingContextualSidePanelRegistry(content::WebContents* contents);
   void StopObservingContextualSidePanelRegistry(SidePanelRegistry* registry);
+  void UpdateActiveItemState();
 
   raw_ptr<BraveBrowser> browser_ = nullptr;
   raw_ptr<SidePanelCoordinator> side_panel_coordinator_ = nullptr;
   raw_ptr<BraveSidePanel> side_panel_ = nullptr;
   raw_ptr<sidebar::SidebarModel> sidebar_model_ = nullptr;
   raw_ptr<SidebarControlView> sidebar_control_view_ = nullptr;
-  raw_ptr<SidePanelRegistry> active_contextual_registry_ = nullptr;
   bool initialized_ = false;
   bool sidebar_on_left_ = true;
   bool operation_from_active_tab_change_ = false;