-
Notifications
You must be signed in to change notification settings - Fork 0
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
Conversation
…sted events for headline management
…and snackbar notification
- Implement StringTruncate extension to truncate strings to a specified maximum length - Add new exports for source_type_l10n.dart and string_truncate.dart in extensions.dart
…ing ones - Add Arabic translations for "headlineDeleted" and "undo" strings - Improve "archive" translation and its description - Ensure proper formatting and consistency in JSON structure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary of Changes
Hello @fulleni, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces a significant user experience improvement by implementing an 'undo delete' feature for headlines in the archive. It provides a grace period after a headline is marked for deletion, during which the user can reverse the action, preventing accidental permanent data loss and enhancing the overall robustness of the content management system.
Highlights
- Undo Delete Functionality: Implemented a 'soft delete' mechanism for headlines in the archive, allowing users to undo a deletion within a 5-second window before permanent removal.
- User Interface Feedback: Introduced a
SnackBar
that appears upon headline deletion, displaying a confirmation message and an 'Undo' button to reverse the action. - State Management Enhancements: Updated the
ArchivedHeadlinesBloc
andArchivedHeadlinesState
to manage the temporary state of a deleted headline and control the delayed permanent deletion via aTimer
. - Error Handling for Deletion: Added logic to automatically restore a headline to the list if the delayed permanent deletion operation fails due to an
HttpException
or other errors. - Localization and Utilities: Added new localization strings for the delete confirmation and undo action, and introduced a string truncation utility for displaying headline titles concisely in the UI.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces an "undo" functionality for deleting headlines, which is a great user experience improvement. The implementation uses a timer to delay the permanent deletion, giving users a chance to revert their action. There is a critical issue with the copyWith
method in the BLoC state that could lead to crashes under certain conditions, and a couple of areas in the BLoC logic where the code can be made more efficient and maintainable.
exception: exception, | ||
restoredHeadline: restoredHeadline, | ||
lastDeletedHeadline: lastDeletedHeadline, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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,
} 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, | ||
), | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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,
),
);
}
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!, | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!,
);
Status
READY/IN DEVELOPMENT/HOLD
Description
Type of Change