[Brave News]: Better offline handling & UX tweaks (#21507)

components/brave_new_tab_ui/components/default/braveNews/FeedV2.tsx

@@ -14,7 +14,6 @@ import FeedNavigation from '../../../../brave_news/browser/resources/FeedNavigat
 import NewsButton from '../../../../brave_news/browser/resources/NewsButton'
 import Variables from '../../../../brave_news/browser/resources/Variables'
 import { useBraveNews } from '../../../../brave_news/browser/resources/shared/Context'
-import { isPublisherEnabled } from '../../../../brave_news/browser/resources/shared/api'
 import { CLASSNAME_PAGE_STUCK } from '../page'
 
 const Root = styled(Variables)`
@@ -73,23 +72,7 @@ const LoadNewContentButton = styled(NewsButton)`
 `
 
 export default function FeedV2() {
-  const { feedV2, setCustomizePage, refreshFeedV2, feedV2UpdatesAvailable, publishers, channels } = useBraveNews()
-
-  // We don't want to decide whether we have subscriptions until the publishers
-  // and channels have loaded.
-  const loaded = React.useMemo(() => !!Object.values(publishers).length && !!Object.values(channels).length, [publishers, channels])
-
-  // This is a bit of an interesting |useMemo| - we only want it to be updated
-  // when the feed changes so as to not break the case where:
-  // 1. The user has no feeds (we show the NoFeeds card)
-  // 2. The user subscribes to a feed (we should still show the NoFeeds card,
-  //    not the "Empty Feed")
-  // To achieve this, |hasSubscriptions| is only updated when the feed changes,
-  // or the opt-in status is changed.
-  const hasSubscriptions = React.useMemo(() => !loaded
-    || Object.values(publishers).some(isPublisherEnabled)
-    || Object.values(channels).some(c => c.subscribedLocales.length), [feedV2, loaded])
-
+  const { feedV2, setCustomizePage, refreshFeedV2, feedV2UpdatesAvailable } = useBraveNews()
   const ref = React.useRef<HTMLDivElement>()
 
   // Note: Whenever the feed is updated, if we're viewing the feed, scroll to
@@ -111,7 +94,7 @@ export default function FeedV2() {
       {feedV2UpdatesAvailable && <LoadNewContentButton onClick={refreshFeedV2}>
         {getLocale('braveNewsNewContentAvailable')}
       </LoadNewContentButton>}
-      <Feed feed={feedV2} hasSubscriptions={hasSubscriptions} />
+      <Feed feed={feedV2} />
     </Flex>
 
     <ButtonsContainer>

components/brave_news/browser/brave_news_controller.cc

@@ -49,6 +49,7 @@
 #include "components/prefs/scoped_user_pref_update.h"
 #include "mojo/public/cpp/bindings/pending_remote.h"
 #include "mojo/public/cpp/bindings/remote_set.h"
+#include "net/base/network_change_notifier.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
 #include "services/network/public/cpp/simple_url_loader.h"
 
@@ -119,6 +120,8 @@ BraveNewsController::BraveNewsController(
       publishers_observation_(this),
       weak_ptr_factory_(this) {
   DCHECK(prefs_);
+  net::NetworkChangeNotifier::AddNetworkChangeObserver(this);
+
   // Set up preference listeners
   pref_change_registrar_.Init(prefs_);
   pref_change_registrar_.Add(
@@ -142,7 +145,9 @@ BraveNewsController::BraveNewsController(
   ConditionallyStartOrStopTimer();
 }
 
-BraveNewsController::~BraveNewsController() = default;
+BraveNewsController::~BraveNewsController() {
+  net::NetworkChangeNotifier::RemoveNetworkChangeObserver(this);
+}
 
 void BraveNewsController::Bind(
     mojo::PendingReceiver<mojom::BraveNewsController> receiver) {
@@ -840,4 +845,15 @@ void BraveNewsController::OnPublishersUpdated(
   }
 }
 
+void BraveNewsController::OnNetworkChanged(
+    net::NetworkChangeNotifier::ConnectionType type) {
+  if (!GetIsEnabled(prefs_)) {
+    return;
+  }
+
+  // Ensure publishers are fetched (this won't do anything if they are). This
+  // handles the case where Brave News is started with no network.
+  publishers_controller_.GetOrFetchPublishers(base::DoNothing());
+}
+
 }  // namespace brave_news

components/brave_news/browser/brave_news_controller.h

@@ -16,6 +16,7 @@
 #include "base/memory/scoped_refptr.h"
 #include "base/scoped_observation.h"
 #include "base/task/cancelable_task_tracker.h"
+#include "base/time/time.h"
 #include "base/timer/timer.h"
 #include "brave/components/api_request_helper/api_request_helper.h"
 #include "brave/components/brave_news/browser/channels_controller.h"
@@ -37,6 +38,8 @@
 #include "mojo/public/cpp/bindings/receiver_set.h"
 #include "mojo/public/cpp/bindings/remote.h"
 #include "mojo/public/cpp/bindings/remote_set.h"
+#include "net/base/network_change_notifier.h"
+#include "services/network/public/cpp/network_connection_tracker.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
 
 class PrefRegistrySimple;
@@ -62,9 +65,11 @@ bool GetIsEnabled(PrefService* prefs);
 // Orchestrates FeedController and PublishersController for data, as well as
 // owning prefs data.
 // Controls remote feed update logic via Timer and prefs values.
-class BraveNewsController : public KeyedService,
-                            public mojom::BraveNewsController,
-                            public PublishersController::Observer {
+class BraveNewsController
+    : public KeyedService,
+      public mojom::BraveNewsController,
+      public PublishersController::Observer,
+      public net::NetworkChangeNotifier::NetworkChangeObserver {
  public:
   static void RegisterProfilePrefs(PrefRegistrySimple* registry);
 
@@ -151,6 +156,10 @@ class BraveNewsController : public KeyedService,
   // PublishersController::Observer:
   void OnPublishersUpdated(brave_news::PublishersController*) override;
 
+  // net::NetworkChangeNotifier::NetworkChangeObserver:
+  void OnNetworkChanged(
+      net::NetworkChangeNotifier::ConnectionType type) override;
+
  private:
   void OnOptInChange();
   void ConditionallyStartOrStopTimer();

components/brave_news/browser/feed_v2_builder.cc

@@ -74,20 +74,26 @@ using ContentGroup = std::pair<std::string, bool>;
 constexpr char kAllContentGroup[] = "all";
 constexpr float kSampleContentGroupAllRatio = 0.2f;
 
-std::string GetFeedHash(const Channels& channels,
-                        const Publishers& publishers,
-                        const ETags& etags) {
+// Returns a tuple of the feed hash and the number of subscribed publishers.
+std::tuple<std::string, size_t> GetFeedHashAndSubscribedCount(
+    const Channels& channels,
+    const Publishers& publishers,
+    const ETags& etags) {
   std::vector<std::string> hash_items;
+  size_t subscribed_count = 0;
+
   for (const auto& [channel_id, channel] : channels) {
     if (!channel->subscribed_locales.empty()) {
       hash_items.push_back(channel_id);
+      subscribed_count++;
     }
   }
 
   for (const auto& [id, publisher] : publishers) {
     if (publisher->user_enabled_status == mojom::UserEnabled::ENABLED ||
         publisher->type == mojom::PublisherType::DIRECT_SOURCE) {
       hash_items.push_back(id);
+      subscribed_count++;
     }
 
     // Disabling a publisher should also change the hash, as it will affect what
@@ -108,7 +114,7 @@ std::string GetFeedHash(const Channels& channels,
     hash = base::NumberToString(hasher(hash + hash_item));
   }
 
-  return hash;
+  return {hash, subscribed_count};
 }
 
 // Gets all relevant signals for an article.
@@ -905,7 +911,8 @@ void FeedV2Builder::RecheckFeedHash() {
   const auto& publishers = publishers_controller_->GetLastPublishers();
   auto channels =
       channels_controller_->GetChannelsFromPublishers(publishers, &*prefs_);
-  hash_ = GetFeedHash(channels, publishers, feed_etags_);
+  std::tie(hash_, subscribed_count_) =
+      GetFeedHashAndSubscribedCount(channels, publishers, feed_etags_);
   for (const auto& listener : listeners_) {
     listener->OnUpdateAvailable(hash_);
   }
@@ -1058,7 +1065,8 @@ void FeedV2Builder::NotifyUpdateCompleted() {
   const auto& publishers = publishers_controller_->GetLastPublishers();
   auto channels =
       channels_controller_->GetChannelsFromPublishers(publishers, &*prefs_);
-  hash_ = GetFeedHash(channels, publishers, feed_etags_);
+  std::tie(hash_, subscribed_count_) =
+      GetFeedHashAndSubscribedCount(channels, publishers, feed_etags_);
 
   // Fire all the pending callbacks.
   for (auto& callback : current_update_->callbacks) {
@@ -1100,6 +1108,16 @@ void FeedV2Builder::GenerateFeed(
             feed->type = std::move(type);
             feed->source_hash = builder->hash_;
 
+            if (feed->items.empty()) {
+              if (builder->raw_feed_items_.size() == 0) {
+                feed->error = mojom::FeedV2Error::ConnectionError;
+              } else if (builder->subscribed_count_ == 0) {
+                feed->error = mojom::FeedV2Error::NoFeeds;
+              } else {
+                feed->error = mojom::FeedV2Error::NoArticles;
+              }
+            }
+
             std::move(callback).Run(feed->Clone());
           },
           weak_ptr_factory_.GetWeakPtr(), std::move(type),
@@ -1186,6 +1204,12 @@ mojom::FeedV2Ptr FeedV2Builder::GenerateAllFeed() {
     }
   }
 
+  // If we aren't subscribed to anything, or we failed to fetch any articles
+  // from the internet, don't try and generate a feed.
+  if (eligible_content_groups.size() == 0 || raw_feed_items_.size() == 0) {
+    return feed;
+  }
+
   // Step 1: Generate a block
   // https://docs.google.com/document/d/1bSVHunwmcHwyQTpa3ab4KRbGbgNQ3ym_GHvONnrBypg/edit#heading=h.rkq699fwps0
   std::vector<mojom::FeedItemV2Ptr> initial_block =

components/brave_news/browser/feed_v2_builder.h

@@ -6,6 +6,7 @@
 #ifndef BRAVE_COMPONENTS_BRAVE_NEWS_BROWSER_FEED_V2_BUILDER_H_
 #define BRAVE_COMPONENTS_BRAVE_NEWS_BROWSER_FEED_V2_BUILDER_H_
 
+#include <cstddef>
 #include <memory>
 #include <optional>
 #include <string>
@@ -155,6 +156,7 @@ class FeedV2Builder : public PublishersController::Observer {
   Signals signals_;
   std::vector<std::string> suggested_publisher_ids_;
   TopicsResult topics_;
+  size_t subscribed_count_ = 0;
 
   std::optional<UpdateRequest> current_update_;
   std::optional<UpdateRequest> next_update_;

components/brave_news/browser/publishers_controller.cc

@@ -202,7 +202,13 @@ void PublishersController::EnsurePublishersIsUpdating() {
         // TODO(petemill): handle bad status or response
         std::optional<Publishers> publisher_list =
             ParseCombinedPublisherList(api_request_result.value_body());
+
+        // Update failed, we'll just reuse whatever publishers we had before.
         if (!publisher_list) {
+          controller->on_current_update_complete_->Signal();
+          controller->is_update_in_progress_ = false;
+          controller->on_current_update_complete_ =
+              std::make_unique<base::OneShotEvent>();
           return;
         }
 

components/brave_news/browser/resources/Feed.tsx

@@ -4,7 +4,7 @@
 // You can obtain one at https://mozilla.org/MPL/2.0/.
 
 import { spacing } from "@brave/leo/tokens/css";
-import { FeedItemV2, FeedV2 } from "gen/brave/components/brave_news/common/brave_news.mojom.m";
+import { FeedItemV2, FeedV2, FeedV2Error } from "gen/brave/components/brave_news/common/brave_news.mojom.m";
 import * as React from 'react';
 import styled from "styled-components";
 import Advert from "./feed/Ad";
@@ -17,6 +17,7 @@ import LoadingCard from "./feed/LoadingCard";
 import NoArticles from "./feed/NoArticles";
 import NoFeeds from "./feed/NoFeeds";
 import { getHistoryValue, setHistoryState } from "./shared/history";
+import NotConnected from "./feed/NotConnected";
 
 // Restoring scroll position is complicated - we have two available strategies:
 // 1. Scroll to the same position - as long as the window hasn't been resized,
@@ -45,7 +46,6 @@ const FeedContainer = styled.div`
 
 interface Props {
   feed: FeedV2 | undefined;
-  hasSubscriptions: boolean;
 }
 
 const getKey = (feedItem: FeedItemV2, index: number): React.Key => {
@@ -76,7 +76,13 @@ const saveScrollPos = (itemId: React.Key) => () => {
   })
 }
 
-export default function Component({ feed, hasSubscriptions }: Props) {
+const errors = {
+  [FeedV2Error.ConnectionError]: <NotConnected/>,
+  [FeedV2Error.NoArticles]: <NoArticles />,
+  [FeedV2Error.NoFeeds]: <NoFeeds />
+}
+
+export default function Component({ feed }: Props) {
   const [cardCount, setCardCount] = React.useState(getHistoryValue(HISTORY_CARD_COUNT, PAGE_SIZE));
 
   // Store the number of cards we've loaded in history - otherwise when we
@@ -145,18 +151,12 @@ export default function Component({ feed, hasSubscriptions }: Props) {
     })
   }, [cardCount, feed?.items])
 
-  // An empty feed may still have ads in it, so we need to filter them out to
-  // determine if there are no articles.
-  const noArticles = React.useMemo(() => !feed?.items.filter(i => !i.advert).length, [feed])
-
   return <FeedContainer className={NEWS_FEED_CLASS}>
     {feed
-      ? noArticles
-        ? hasSubscriptions ? <NoArticles /> : <NoFeeds />
-        : <>
-          {cards}
-          <CaughtUp />
-        </>
+      ? errors[feed.error!] ?? <>
+        {cards}
+        <CaughtUp />
+      </>
       : <LoadingCard />}
   </FeedContainer>
 }

components/brave_news/browser/resources/FeedNavigation.tsx

@@ -150,7 +150,7 @@ export default function Sidebar() {
     <Heading>{getLocale('braveNewsMyFeedHeading')}</Heading>
     <Item id='all' name={getLocale('braveNewsForYouFeed')} />
     <Item id='following' name={getLocale('braveNewsFollowingFeed')} />
-    <Section open>
+    {!!subscribedChannels.length && <Section open>
       <summary>
         {Marker}
         {getLocale('braveNewsChannelsHeader')}
@@ -166,8 +166,8 @@ export default function Sidebar() {
             ? getLocale('braveNewsShowLess')
             : getLocale('braveNewsShowAll')}
         </CustomButton>}
-    </Section>
-    <Section open>
+    </Section>}
+    {!!subscribedPublisherIds.length && <Section open>
       <summary>
         {Marker}
         {getLocale('braveNewsPublishersHeading')}
@@ -183,6 +183,6 @@ export default function Sidebar() {
             ? getLocale('braveNewsShowLess')
             : getLocale('braveNewsShowAll')}
         </CustomButton>}
-    </Section>
+    </Section>}
   </Container>
 }

components/brave_news/browser/resources/FeedPage.tsx

@@ -23,6 +23,6 @@ export default function FeedPage() {
   const { feedV2 } = useBraveNews()
   return <Container>
     <h2>The Feed ({feedV2?.items.length} items. Truncated at {truncate})</h2>
-    <Feed feed={feedV2} hasSubscriptions />
+    <Feed feed={feedV2} />
   </Container>
 }

components/brave_news/browser/resources/feed/LoadingCard.tsx

@@ -15,7 +15,6 @@ const Container = styled(Card)`
 
 const Spinner = styled(ProgressRing)`
   --leo-progressring-color: var(--bn-glass-25);
-  backdrop-filter: blur(64px);
 `
 
 export default function LoadingCard() {