Merge pull request #172 from flutter-news-app-full-source-code/fix/unamed-saved-filter-bug

fulleni · web-flow · commit 8dd8d04da5ea · 2025-10-16T15:16:24.000+01:00
Fix/unamed saved filter bug
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,9 @@
 
 ## 1.4.0 - Upcoming Release
 
+- **refactor**: improved filter page reset button to clear local selections and disable when not applicable
+- **feat**: auto-scroll to active filter chip in SavedFiltersBar
+- **fix**: resolve bug where applying a modified filter incorrectly selects the original saved filter
 - **refactor**: relocated saved filters management to the account page and introduced reordering capability.
 - **feat**: created a reusable, searchable multi-select page for filtering
 - **feat**: add 'Followed' filter to quickly view content from followed items
diff --git a/lib/headlines-feed/bloc/headlines_feed_bloc.dart b/lib/headlines-feed/bloc/headlines_feed_bloc.dart
@@ -2,6 +2,7 @@ import 'dart:async';
 
 import 'package:bloc/bloc.dart';
 import 'package:bloc_concurrency/bloc_concurrency.dart';
+import 'package:collection/collection.dart';
 import 'package:core/core.dart';
 import 'package:data_repository/data_repository.dart';
 import 'package:equatable/equatable.dart';
@@ -265,19 +266,38 @@ class HeadlinesFeedBloc extends Bloc<HeadlinesFeedEvent, HeadlinesFeedState> {
     HeadlinesFeedFiltersApplied event,
     Emitter<HeadlinesFeedState> emit,
   ) async {
-    String? newActiveFilterId;
+    String newActiveFilterId;
 
-    // If a saved filter was explicitly passed (e.g., just created), use its ID.
+    // Prioritize the explicitly passed savedFilter to prevent race conditions.
+    // This is crucial for the "save and apply" flow, where the AppBloc might
+    // not have updated the savedFilters list in this bloc's state yet.
     if (event.savedFilter != null) {
       newActiveFilterId = event.savedFilter!.id;
     } else {
-      // Otherwise, determine if this is a pre-existing saved filter being
-      // re-applied or a custom one.
-      final isSavedFilter =
-          state.activeFilterId != 'all' &&
-          state.activeFilterId != 'custom' &&
-          state.activeFilterId != null;
-      newActiveFilterId = isSavedFilter ? state.activeFilterId : 'custom';
+      // If no filter is explicitly passed, determine if the applied filter
+      // matches an existing saved filter. This handles re-applying a saved
+      // filter or applying a one-time "custom" filter.
+      final matchingSavedFilter = state.savedFilters.firstWhereOrNull((
+        savedFilter,
+      ) {
+        // Use sets for order-agnostic comparison of filter contents.
+        final appliedTopics = event.filter.topics?.toSet() ?? {};
+        final savedTopics = savedFilter.topics.toSet();
+        final appliedSources = event.filter.sources?.toSet() ?? {};
+        final savedSources = savedFilter.sources.toSet();
+        final appliedCountries = event.filter.eventCountries?.toSet() ?? {};
+        final savedCountries = savedFilter.countries.toSet();
+
+        return const SetEquality<Topic>().equals(appliedTopics, savedTopics) &&
+            const SetEquality<Source>().equals(appliedSources, savedSources) &&
+            const SetEquality<Country>().equals(
+              appliedCountries,
+              savedCountries,
+            );
+      });
+
+      // If a match is found, use its ID. Otherwise, mark it as 'custom'.
+      newActiveFilterId = matchingSavedFilter?.id ?? 'custom';
     }
 
     // When applying new filters, this is considered a major feed change,
@@ -550,15 +570,15 @@ class HeadlinesFeedBloc extends Bloc<HeadlinesFeedEvent, HeadlinesFeedState> {
   /// Handles the selection of the "Followed" filter from the filter bar.
   ///
   /// This creates a [HeadlineFilter] from the user's followed items and
-  /// triggers a full feed refresh.
+  /// triggers a full feed refresh directly within this handler. It no longer
+  /// delegates to `HeadlinesFeedFiltersApplied` to prevent a bug where the
+  /// filter would be incorrectly identified as 'custom'.
   Future<void> _onFollowedFilterSelected(
     FollowedFilterSelected event,
     Emitter<HeadlinesFeedState> emit,
   ) async {
     final userPreferences = _appBloc.state.userContentPreferences;
     if (userPreferences == null) {
-      // This case should ideally not happen if the UI is built correctly,
-      // as the "Followed" button is only shown when preferences are available.
       return;
     }
 
@@ -568,15 +588,77 @@ class HeadlinesFeedBloc extends Bloc<HeadlinesFeedEvent, HeadlinesFeedState> {
       eventCountries: userPreferences.followedCountries,
     );
 
-    // Set the active filter ID and then dispatch an event to apply the filter
-    // and refresh the feed.
-    emit(state.copyWith(activeFilterId: 'followed'));
-    add(
-      HeadlinesFeedFiltersApplied(
+    // This is a major feed change, so clear the ad cache.
+    _inlineAdCacheService.clearAllAds();
+    emit(
+      state.copyWith(
+        status: HeadlinesFeedStatus.loading,
         filter: newFilter,
-        adThemeStyle: event.adThemeStyle,
+        activeFilterId: 'followed',
+        feedItems: [],
+        clearCursor: true,
       ),
     );
+
+    try {
+      final currentUser = _appBloc.state.user;
+      final appConfig = _appBloc.state.remoteConfig;
+
+      if (appConfig == null) {
+        emit(state.copyWith(status: HeadlinesFeedStatus.failure));
+        return;
+      }
+
+      final headlineResponse = await _headlinesRepository.readAll(
+        filter: _buildFilter(newFilter),
+        pagination: const PaginationOptions(limit: _headlinesFetchLimit),
+        sort: [const SortOption('updatedAt', SortOrder.desc)],
+      );
+
+      final decorationResult = await _feedDecoratorService.decorateFeed(
+        headlines: headlineResponse.items,
+        user: currentUser,
+        remoteConfig: appConfig,
+        followedTopicIds: userPreferences.followedTopics
+            .map((t) => t.id)
+            .toList(),
+        followedSourceIds: userPreferences.followedSources
+            .map((s) => s.id)
+            .toList(),
+        imageStyle: _appBloc.state.settings!.feedPreferences.headlineImageStyle,
+        adThemeStyle: event.adThemeStyle,
+      );
+
+      emit(
+        state.copyWith(
+          status: HeadlinesFeedStatus.success,
+          feedItems: decorationResult.decoratedItems,
+          hasMore: headlineResponse.hasMore,
+          cursor: headlineResponse.cursor,
+        ),
+      );
+
+      final injectedDecorator = decorationResult.injectedDecorator;
+      if (injectedDecorator != null && currentUser?.id != null) {
+        if (injectedDecorator is CallToActionItem) {
+          _appBloc.add(
+            AppUserFeedDecoratorShown(
+              userId: currentUser!.id,
+              feedDecoratorType: injectedDecorator.decoratorType,
+            ),
+          );
+        } else if (injectedDecorator is ContentCollectionItem) {
+          _appBloc.add(
+            AppUserFeedDecoratorShown(
+              userId: currentUser!.id,
+              feedDecoratorType: injectedDecorator.decoratorType,
+            ),
+          );
+        }
+      }
+    } on HttpException catch (e) {
+      emit(state.copyWith(status: HeadlinesFeedStatus.failure, error: e));
+    }
   }
 
   @override
diff --git a/lib/headlines-feed/bloc/headlines_feed_event.dart b/lib/headlines-feed/bloc/headlines_feed_event.dart
@@ -52,6 +52,11 @@ final class HeadlinesFeedRefreshRequested extends HeadlinesFeedEvent {
 /// Event triggered when a new set of filters, selected by the user,
 /// should be applied to the headlines feed.
 /// {@endtemplate}
+/// The `activeFilterId` is determined within the `HeadlinesFeedBloc`.
+/// In most cases, this is done by comparing the applied filter against the
+/// list of saved filters. However, to prevent a race condition during the
+/// "save and apply" flow, an optional `savedFilter` can be provided to
+/// ensure the newly created filter is selected immediately.
 final class HeadlinesFeedFiltersApplied extends HeadlinesFeedEvent {
   /// {@macro headlines_feed_filters_applied}
   ///
@@ -67,8 +72,8 @@ final class HeadlinesFeedFiltersApplied extends HeadlinesFeedEvent {
   final HeadlineFilter filter;
 
   /// The optional [SavedFilter] that this filter corresponds to.
-  /// This is used to correctly set the active filter ID when a new
-  /// filter is saved and immediately applied.
+  /// This is used exclusively during the "save and apply" flow to prevent
+  /// a race condition and ensure the new filter's chip is selected.
   final SavedFilter? savedFilter;
 
   /// The current ad theme style of the application.
diff --git a/lib/headlines-feed/view/headlines_filter_page.dart b/lib/headlines-feed/view/headlines_filter_page.dart
@@ -161,9 +161,9 @@ class _HeadlinesFilterViewState extends State<_HeadlinesFilterView> {
             // Add the new filter to the global AppBloc state.
             context.read<AppBloc>().add(SavedFilterAdded(filter: newFilter));
 
-            // Apply the filter to the HeadlinesFeedBloc. The page is not
-            // popped here; that action is deferred until after the dialog
-            // has been successfully dismissed.
+            // Apply the newly saved filter to the HeadlinesFeedBloc. The page
+            // is not popped here; that action is deferred until after the
+            // dialog has been successfully dismissed.
             _applyFilter(newFilter);
           },
         );
@@ -186,6 +186,7 @@ class _HeadlinesFilterViewState extends State<_HeadlinesFilterView> {
   /// treated as a "custom" one.
   void _applyFilter(SavedFilter? savedFilter) {
     final filterState = context.read<HeadlinesFilterBloc>().state;
+    // Create a new HeadlineFilter from the current selections in the filter bloc.
     final newFilter = HeadlineFilter(
       topics: filterState.selectedTopics.toList(),
       sources: filterState.selectedSources.toList(),
@@ -218,6 +219,13 @@ class _HeadlinesFilterViewState extends State<_HeadlinesFilterView> {
           builder: (context, filterState) {
             final theme = Theme.of(context);
 
+            // Determine if the reset button should be enabled. It's enabled only
+            // if there are active selections to clear.
+            final isResetEnabled =
+                filterState.selectedTopics.isNotEmpty ||
+                filterState.selectedSources.isNotEmpty ||
+                filterState.selectedCountries.isNotEmpty;
+
             // Determine if the "Apply" button should be enabled.
             final isFilterEmpty =
                 filterState.selectedTopics.isEmpty &&
@@ -258,21 +266,16 @@ class _HeadlinesFilterViewState extends State<_HeadlinesFilterView> {
                   style: theme.textTheme.titleLarge,
                 ),
                 actions: [
-                  // Reset All Filters Button
+                  // Reset button to clear local selections on this page.
                   IconButton(
                     icon: const Icon(Icons.refresh),
                     tooltip: l10n.headlinesFeedFilterResetButton,
-                    onPressed: () {
-                      // Dispatch event to clear filters in the feed bloc
-                      // and trigger a refresh to the "All" state.
-                      context.read<HeadlinesFeedBloc>().add(
-                        AllFilterSelected(
-                          adThemeStyle: AdThemeStyle.fromTheme(theme),
-                        ),
-                      );
-                      // Close the filter page.
-                      context.pop();
-                    },
+                    // The button is disabled if there are no selections to clear.
+                    onPressed: isResetEnabled
+                        ? () => context.read<HeadlinesFilterBloc>().add(
+                            const FilterSelectionsCleared(),
+                          )
+                        : null,
                   ),
                   // Apply Filters Button
                   IconButton(
diff --git a/lib/headlines-feed/widgets/saved_filters_bar.dart b/lib/headlines-feed/widgets/saved_filters_bar.dart
