Skip to content

Feature undo delete in headlines archive #54

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Aug 2, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import 'dart:async';

import 'package:bloc/bloc.dart';
import 'package:core/core.dart';
import 'package:data_repository/data_repository.dart';
Expand All @@ -15,9 +17,18 @@ class ArchivedHeadlinesBloc
on<LoadArchivedHeadlinesRequested>(_onLoadArchivedHeadlinesRequested);
on<RestoreHeadlineRequested>(_onRestoreHeadlineRequested);
on<DeleteHeadlineForeverRequested>(_onDeleteHeadlineForeverRequested);
on<UndoDeleteHeadlineRequested>(_onUndoDeleteHeadlineRequested);
on<_ConfirmDeleteHeadlineRequested>(_onConfirmDeleteHeadlineRequested);
}

final DataRepository<Headline> _headlinesRepository;
Timer? _deleteTimer;

@override
Future<void> close() {
_deleteTimer?.cancel();
return super.close();
}

Future<void> _onLoadArchivedHeadlinesRequested(
LoadArchivedHeadlinesRequested event,
Expand Down Expand Up @@ -96,22 +107,83 @@ class ArchivedHeadlinesBloc
DeleteHeadlineForeverRequested event,
Emitter<ArchivedHeadlinesState> emit,
) async {
final originalHeadlines = List<Headline>.from(state.headlines);
final headlineIndex = originalHeadlines.indexWhere((h) => h.id == event.id);
_deleteTimer?.cancel();

final headlineIndex = state.headlines.indexWhere((h) => h.id == event.id);
if (headlineIndex == -1) return;

final updatedHeadlines = originalHeadlines..removeAt(headlineIndex);
emit(state.copyWith(headlines: updatedHeadlines));
final headlineToDelete = state.headlines[headlineIndex];
final updatedHeadlines = List<Headline>.from(state.headlines)
..removeAt(headlineIndex);

emit(
state.copyWith(
headlines: updatedHeadlines,
lastDeletedHeadline: headlineToDelete,
),
);

_deleteTimer = Timer(
const Duration(seconds: 5),
() => add(_ConfirmDeleteHeadlineRequested(event.id)),
);
}

Future<void> _onConfirmDeleteHeadlineRequested(
_ConfirmDeleteHeadlineRequested event,
Emitter<ArchivedHeadlinesState> emit,
) async {
try {
await _headlinesRepository.delete(id: event.id);
emit(state.copyWith(lastDeletedHeadline: null));
} on HttpException catch (e) {
emit(state.copyWith(headlines: originalHeadlines, exception: e));
// If deletion fails, restore the headline to the list
final originalHeadlines = List<Headline>.from(state.headlines)
..add(state.lastDeletedHeadline!);
emit(
state.copyWith(
headlines: originalHeadlines,
exception: e,
lastDeletedHeadline: null,
),
);
} catch (e) {
final originalHeadlines = List<Headline>.from(state.headlines)
..add(state.lastDeletedHeadline!);
emit(
state.copyWith(
headlines: originalHeadlines,
exception: UnknownException('An unexpected error occurred: $e'),
lastDeletedHeadline: null,
),
);
}
Comment on lines 139 to +160

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The error handling logic in the on HttpException and catch (e) blocks is duplicated. This can be simplified by using a single catch block and checking the exception type within it. This will make the code more concise and easier to maintain.

    } catch (e) {
      // If deletion fails, restore the headline to the list
      final originalHeadlines = List<Headline>.from(state.headlines)
        ..add(state.lastDeletedHeadline!);
      emit(
        state.copyWith(
          headlines: originalHeadlines,
          exception: e is HttpException
              ? e
              : UnknownException('An unexpected error occurred: $e'),
          lastDeletedHeadline: null,
        ),
      );
    }

}

void _onUndoDeleteHeadlineRequested(
UndoDeleteHeadlineRequested event,
Emitter<ArchivedHeadlinesState> emit,
) {
_deleteTimer?.cancel();
if (state.lastDeletedHeadline != null) {
final updatedHeadlines = List<Headline>.from(state.headlines)
..insert(
state.headlines.indexWhere(
(h) =>
h.updatedAt.isBefore(state.lastDeletedHeadline!.updatedAt),
) !=
-1
? state.headlines.indexWhere(
(h) =>
h.updatedAt.isBefore(state.lastDeletedHeadline!.updatedAt),
)
: state.headlines.length,
state.lastDeletedHeadline!,
);
Comment on lines +169 to +182

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The logic to find the insertion index for the undone headline calls state.headlines.indexWhere(...) twice. This is inefficient and makes the code harder to read.

You can improve this by calling indexWhere once, storing its result in a local variable, and then reusing that variable.

      final insertionIndex = state.headlines.indexWhere(
        (h) => h.updatedAt.isBefore(state.lastDeletedHeadline!.updatedAt),
      );
      final updatedHeadlines = List<Headline>.from(state.headlines)
        ..insert(
          insertionIndex != -1 ? insertionIndex : state.headlines.length,
          state.lastDeletedHeadline!,
        );

emit(
state.copyWith(
headlines: updatedHeadlines,
lastDeletedHeadline: null,
),
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,3 +37,18 @@ final class DeleteHeadlineForeverRequested extends ArchivedHeadlinesEvent {
@override
List<Object?> get props => [id];
}

/// Event to undo the deletion of a headline.
final class UndoDeleteHeadlineRequested extends ArchivedHeadlinesEvent {
const UndoDeleteHeadlineRequested();
}

/// Internal event to confirm the permanent deletion of a headline after a delay.
final class _ConfirmDeleteHeadlineRequested extends ArchivedHeadlinesEvent {
const _ConfirmDeleteHeadlineRequested(this.id);

final String id;

@override
List<Object?> get props => [id];
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ class ArchivedHeadlinesState extends Equatable {
this.hasMore = false,
this.exception,
this.restoredHeadline,
this.lastDeletedHeadline,
});

final ArchivedHeadlinesStatus status;
Expand All @@ -25,6 +26,7 @@ class ArchivedHeadlinesState extends Equatable {
final bool hasMore;
final HttpException? exception;
final Headline? restoredHeadline;
final Headline? lastDeletedHeadline;

ArchivedHeadlinesState copyWith({
ArchivedHeadlinesStatus? status,
Expand All @@ -33,14 +35,16 @@ class ArchivedHeadlinesState extends Equatable {
bool? hasMore,
HttpException? exception,
Headline? restoredHeadline,
Headline? lastDeletedHeadline,
}) {
return ArchivedHeadlinesState(
status: status ?? this.status,
headlines: headlines ?? this.headlines,
cursor: cursor ?? this.cursor,
hasMore: hasMore ?? this.hasMore,
exception: exception ?? this.exception,
exception: exception,
restoredHeadline: restoredHeadline,
lastDeletedHeadline: lastDeletedHeadline,
Comment on lines +45 to +47

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

The implementation of copyWith for nullable fields like exception, restoredHeadline, and lastDeletedHeadline is unsafe. By setting exception: exception, any call to copyWith that doesn't provide an exception will reset it to null. This is a regression from the previous exception: exception ?? this.exception.

This pattern is especially dangerous for lastDeletedHeadline. If another event triggers a state update while the undo timer is running (e.g., pagination), lastDeletedHeadline will be cleared, and the app will crash when the timer fires because of the non-null assertion on state.lastDeletedHeadline!.

A safer copyWith should preserve existing values if new ones aren't provided. While this makes clearing a field via copyWith(lastDeletedHeadline: null) not work as expected with the ?? operator, it prevents hard-to-debug race conditions. Clearing fields should be handled more explicitly, for example by creating a new state instance or using a different pattern for clearing.

I suggest reverting to the safer ?? this.fieldName pattern for all nullable fields to prevent these issues.

      exception: exception ?? this.exception,
      restoredHeadline: restoredHeadline ?? this.restoredHeadline,
      lastDeletedHeadline: lastDeletedHeadline ?? this.lastDeletedHeadline,

);
}

Expand All @@ -52,5 +56,6 @@ class ArchivedHeadlinesState extends Equatable {
hasMore,
exception,
restoredHeadline,
lastDeletedHeadline,
];
}
27 changes: 24 additions & 3 deletions lib/content_management/view/archived_headlines_page.dart
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import 'package:flutter_news_app_web_dashboard_full_source_code/content_manageme
import 'package:flutter_news_app_web_dashboard_full_source_code/content_management/bloc/content_management_bloc.dart';
import 'package:flutter_news_app_web_dashboard_full_source_code/l10n/app_localizations.dart';
import 'package:flutter_news_app_web_dashboard_full_source_code/l10n/l10n.dart';
import 'package:flutter_news_app_web_dashboard_full_source_code/shared/extensions/extensions.dart';
import 'package:intl/intl.dart';
import 'package:ui_kit/ui_kit.dart';

Expand Down Expand Up @@ -38,15 +39,35 @@ class _ArchivedHeadlinesView extends StatelessWidget {
padding: const EdgeInsets.all(AppSpacing.lg),
child: BlocListener<ArchivedHeadlinesBloc, ArchivedHeadlinesState>(
listenWhen: (previous, current) =>
previous.status != current.status ||
previous.lastDeletedHeadline != current.lastDeletedHeadline ||
previous.restoredHeadline != current.restoredHeadline,
listener: (context, state) {
if (state.status == ArchivedHeadlinesStatus.success &&
state.restoredHeadline != null) {
if (state.restoredHeadline != null) {
context.read<ContentManagementBloc>().add(
const LoadHeadlinesRequested(limit: kDefaultRowsPerPage),
);
}
if (state.lastDeletedHeadline != null) {
final truncatedTitle =
state.lastDeletedHeadline!.title.truncate(30);
ScaffoldMessenger.of(context)
..hideCurrentSnackBar()
..showSnackBar(
SnackBar(
content: Text(
l10n.headlineDeleted(truncatedTitle),
),
action: SnackBarAction(
label: l10n.undo,
onPressed: () {
context
.read<ArchivedHeadlinesBloc>()
.add(const UndoDeleteHeadlineRequested());
},
),
),
);
}
},
child: BlocBuilder<ArchivedHeadlinesBloc, ArchivedHeadlinesState>(
builder: (context, state) {
Expand Down
12 changes: 12 additions & 0 deletions lib/l10n/app_localizations.dart
Original file line number Diff line number Diff line change
Expand Up @@ -1753,6 +1753,18 @@ abstract class AppLocalizations {
/// In en, this message translates to:
/// **'Archive'**
String get archive;

/// Snackbar message when a headline is deleted
///
/// In en, this message translates to:
/// **'Deleted \'\'{title}\'\'.'**
String headlineDeleted(String title);

/// No description provided for @undo.
///
/// In en, this message translates to:
/// **'Undo'**
String get undo;
}

class _AppLocalizationsDelegate
Expand Down
8 changes: 8 additions & 0 deletions lib/l10n/app_localizations_ar.dart
Original file line number Diff line number Diff line change
Expand Up @@ -922,4 +922,12 @@ class AppLocalizationsAr extends AppLocalizations {

@override
String get archive => 'أرشفة';

@override
String headlineDeleted(String title) {
return 'تم حذف \'\'$title\'\'.';
}

@override
String get undo => 'تراجع';
}
8 changes: 8 additions & 0 deletions lib/l10n/app_localizations_en.dart
Original file line number Diff line number Diff line change
Expand Up @@ -921,4 +921,12 @@ class AppLocalizationsEn extends AppLocalizations {

@override
String get archive => 'Archive';

@override
String headlineDeleted(String title) {
return 'Deleted \'\'$title\'\'.';
}

@override
String get undo => 'Undo';
}
15 changes: 13 additions & 2 deletions lib/l10n/arb/app_ar.arb
Original file line number Diff line number Diff line change
Expand Up @@ -1148,6 +1148,17 @@
},
"archive": "أرشفة",
"@archive": {
"description": "تلميح لزر الأرشفة"
}
"description": "تلميح لأرشفة زر"
},
"headlineDeleted": "تم حذف ''{title}''.",
"@headlineDeleted": {
"description": "رسالة Snackbar عند حذف عنوان",
"placeholders": {
"title": {
"type": "String",
"example": "عاجل"
}
}
},
"undo": "تراجع"
}
16 changes: 13 additions & 3 deletions lib/l10n/arb/app_en.arb
Original file line number Diff line number Diff line change
Expand Up @@ -1080,8 +1080,7 @@
"feedActionTypeEnableNotifications": "Enable Notifications",
"@feedActionTypeEnableNotifications": {
"description": "Feed action type for enabling notifications"
}
,
},
"countryPickerSearchLabel": "Search",
"@countryPickerSearchLabel": {
"description": "Label for the search input in the country picker"
Expand Down Expand Up @@ -1149,5 +1148,16 @@
"archive": "Archive",
"@archive": {
"description": "Tooltip for the archive button"
}
},
"headlineDeleted": "Deleted ''{title}''.",
"@headlineDeleted": {
"description": "Snackbar message when a headline is deleted",
"placeholders": {
"title": {
"type": "String",
"example": "Breaking News"
}
}
},
"undo": "Undo"
}
2 changes: 2 additions & 0 deletions lib/shared/extensions/extensions.dart
Original file line number Diff line number Diff line change
@@ -1 +1,3 @@
export 'content_status_l10n.dart';
export 'source_type_l10n.dart';
export 'string_truncate.dart';
8 changes: 8 additions & 0 deletions lib/shared/extensions/string_truncate.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
extension StringTruncate on String {
String truncate(int maxLength) {
if (length <= maxLength) {
return this;
}
return '${substring(0, maxLength)}...';
}
}
Loading