fix: Enhance feedback notifications for egg actions #2042
Conversation
- Consolidated notification handling for egg import, update, and delete actions. - Improved success, failure, and skipped message clarity using grouped results. - Added contextual details in notifications, such as egg names and counts. - Refined filtering logic with pre-validation prompts for bulk actions.
📝 WalkthroughWalkthroughAdds user-facing Filament notifications across egg management: deletion (success/failure), bulk-action pre-checks that notify and filter records, consolidated import results notification, and per-egg update notifications with error reporting; also adds related localization strings. Changes
Possibly related PRs
Pre-merge checks❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/Filament/Components/Actions/ImportEggAction.php (1)
92-107: LGTM! Consolidated notification improves user experience.The refactoring successfully consolidates import results into a single notification with clear success/failure/total counts. The dynamic status based on outcomes (success/danger/warning) provides appropriate visual feedback.
Optional: Consider extracting the delimiter as a constant
The delimiter
' | 'is used here and in other files (UpdateEggBulkAction.php line 82). Consider defining it as a class constant for consistency:+ private const NOTIFICATION_DELIMITER = ' | '; + protected function setUp(): void { // ... - ->body($bodyParts->join(' | ')) + ->body($bodyParts->join(self::NOTIFICATION_DELIMITER))
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/Filament/Admin/Resources/Eggs/Pages/EditEgg.phpapp/Filament/Admin/Resources/Eggs/Pages/ListEggs.phpapp/Filament/Components/Actions/ImportEggAction.phpapp/Filament/Components/Actions/UpdateEggAction.phpapp/Filament/Components/Actions/UpdateEggBulkAction.phplang/en/admin/egg.php
🧰 Additional context used
🧬 Code graph analysis (1)
app/Filament/Admin/Resources/Eggs/Pages/ListEggs.php (2)
app/Models/Egg.php (1)
Egg(61-359)app/Filament/Components/Actions/UpdateEggBulkAction.php (1)
UpdateEggBulkAction(13-92)
🔇 Additional comments (11)
app/Filament/Components/Actions/UpdateEggAction.php (3)
44-44: Good practice: capturing egg name before potential state changes.Storing the egg name in a local variable ensures consistent messaging even if the egg entity is modified during the update process.
50-60: Excellent error handling improvements.The addition of
report($exception)on line 57 ensures proper exception logging for debugging, and the earlyreturnon line 59 prevents execution of the success notification after a failure.
62-67: Clear success notification with contextual information.The notification includes both the egg name and the source URL, providing administrators with complete context about what was updated and from where.
app/Filament/Components/Actions/UpdateEggBulkAction.php (1)
50-83: Excellent refactoring to collection-based result tracking.Replacing scalar counters with collections (
successEggs,failedEggs,skippedEggs) enables name-based reporting, significantly improving the clarity of bulk operation results. The consolidated notification approach is consistent with ImportEggAction and provides administrators with actionable feedback.lang/en/admin/egg.php (2)
24-26: Clear and contextual translation keys for import results.The new keys provide administrators with detailed feedback about which eggs succeeded or failed during import operations.
108-125: Comprehensive translation keys for all egg action outcomes.The added keys cover update success/failure, delete operations, and pre-validation scenarios. The use of placeholders (
:egg,:eggs,:count,:url,:error) enables dynamic, context-rich messaging.app/Filament/Admin/Resources/Eggs/Pages/EditEgg.php (1)
444-455: Excellent addition of delete action notifications.The success and failure notifications provide clear feedback with egg-specific context, directly addressing the PR objective of improving error messages when operations fail. The closure-based approach correctly captures the egg entity for use in notification messages.
app/Filament/Admin/Resources/Eggs/Pages/ListEggs.php (4)
21-21: Necessary imports for enhanced bulk action notifications.The addition of
NotificationandCollectionimports supports the new pre-validation logic in bulk actions.Also applies to: 27-27
93-113: Excellent pre-validation for delete bulk action.This implementation directly addresses the PR objective by providing clear, actionable error messages before attempting deletion. Key improvements:
- Identifies eggs with active servers and lists them with server counts
- Uses formatting (
sprintf) to handle pluralization correctly- Filters out ineligible eggs from the operation
- Halts execution gracefully if no eligible eggs remain
The approach prevents user confusion by explaining why deletion cannot proceed and which eggs are affected.
115-133: Clear pre-validation for update bulk action.Similar to the delete action, this provides administrators with upfront feedback about eggs lacking update URLs, preventing failed operations and confusion. The warning-level notification appropriately indicates this is a configuration issue rather than a critical error.
93-133: Consider removing the reference parameter (&) from theCollectionparameter inbefore()hooks.The
before()closure usesCollection &$records, but Filament's bulk action before hooks conventionally receive ($records, BulkAction $action) without reference parameters. While the reference parameter is valid PHP syntax, it's not aligned with Filament's standard pattern and may cause unexpected behavior when the collection is modified. Update the signature tofunction (Collection $records)to match documented bulk action lifecycle hook patterns.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/Filament/Components/Actions/ImportEggAction.php (1)
92-107: Consolidation approach is sound; translation keys verified.The notification implementation correctly references all three translation keys which exist in
lang/en/admin/egg.php:
admin/egg.import.import_result(uses:successand:totalparameters)admin/egg.import.imported_eggs(uses:eggsparameter)admin/egg.import.failed_import_eggs(uses:eggsparameter)The code logic is correct and parameters are properly passed. However, two optional improvements remain:
Bulk operation UX: Lines 93-94 join all egg names with
', 'without truncation. For imports with many eggs (20+), this could create excessively long notification bodies. Consider truncating the list with "... and X more" when exceeding a threshold.Hardcoded delimiter: Line 104 uses
' | 'as a separator. Consider whether this should be localized via translation key for consistency with other UI text.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/Filament/Admin/Resources/Eggs/Pages/EditEgg.phpapp/Filament/Components/Actions/ImportEggAction.php
🧰 Additional context used
🧬 Code graph analysis (2)
app/Filament/Admin/Resources/Eggs/Pages/EditEgg.php (2)
app/Models/Egg.php (1)
Egg(61-359)app/Models/Server.php (1)
egg(314-317)
app/Filament/Components/Actions/ImportEggAction.php (1)
app/Livewire/Installer/Steps/RequirementsStep.php (1)
make(15-94)
🔇 Additional comments (1)
app/Filament/Admin/Resources/Eggs/Pages/EditEgg.php (1)
444-453: Notifications improve UX, but generic failure message could be more actionable.The addition of success and failure notifications is a solid improvement. The success notification clearly communicates the outcome with the egg name included. However, the failure notification provides only a generic message without explaining what went wrong.
Since dependency-related deletions are already prevented by disabling the button (line 14), the failure notification would only trigger for unexpected errors (database issues, permissions, etc.). To address this, the failureNotification closure cannot directly receive exception details via Filament's API. Instead, consider using a custom action method that wraps deletion in a try-catch block to capture and display the actual error:
DeleteAction::make() ->disabled(fn (Egg $egg): bool => $egg->servers()->count() > 0) ->label(fn (Egg $egg): string => $egg->servers()->count() <= 0 ? trans('filament-actions::delete.single.label') : trans('admin/egg.in_use')) + ->action(function (Egg $egg) { + try { + $egg->delete(); + } catch (\Exception $e) { + Notification::make() + ->danger() + ->title(trans('admin/egg.delete_failed')) + ->body($e->getMessage()) + ->send(); + throw $e; + } + }) ->successNotification(fn (Egg $egg) => Notification::make() ... ) - ->failureNotification(fn (Egg $egg) => Notification::make() - ... - )
| $eggName = $egg->name; | ||
|
|
There was a problem hiding this comment.
Don't think we need the variable!
|
|
||
| if ($eggsWithServers->isNotEmpty()) { | ||
| $eggNames = $eggsWithServers->map(function (Egg $egg) { | ||
| return sprintf('%s (%d server%s)', $egg->name, $egg->servers_count, $egg->servers_count > 1 ? 's' : ''); |
3 participants
Closes #1962
(I renamed the branch and it closed the PR. I was unable to re-open it)