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 localized Filament notifications across egg management: deletion success/failure, bulk-action pre-check notifications that list and filter problematic eggs, consolidated import-result notifications, and per-egg update notifications including error reporting; also adds related English translation keys. Changes
Possibly related PRs
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (4 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() - ... - )
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/Filament/Admin/Resources/Eggs/Pages/ListEggs.php (1)
93-111: Consider localizing the server count pluralization.Line 97 uses hardcoded English pluralization (
server/servers). For consistency with the rest of the codebase, consider using Laravel'strans_choiceor moving this string to the translation file.🔎 Suggested refactor
- $eggNames = $eggsWithServers->map(fn(Egg $egg) => sprintf('%s (%d server%s)', $egg->name, $egg->servers_count, $egg->servers_count > 1 ? 's' : '')) + $eggNames = $eggsWithServers->map(fn (Egg $egg) => sprintf('%s (%s)', $egg->name, trans_choice('admin/egg.server_count', $egg->servers_count, ['count' => $egg->servers_count]))) ->join(', ');This would require adding a translation key like:
'server_count' => '{1} :count server|[2,*] :count servers',Also, minor style nit: add a space after
fnto match the rest of the file (fn (Egg $egg)on lines 94, 114).
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/Filament/Admin/Resources/Eggs/Pages/ListEggs.phpapp/Filament/Components/Actions/UpdateEggAction.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 (3)
app/Filament/Components/Actions/UpdateEggAction.php (1)
43-65: LGTM!The error handling improvements are solid:
- Proper exception reporting via
report($exception)for logging/monitoring.- Early return after failure prevents unintended success path execution.
- Localized notifications with egg name context improve admin UX, aligning with PR objectives.
app/Filament/Admin/Resources/Eggs/Pages/ListEggs.php (2)
112-131: LGTM!The before hook provides proactive user feedback when eggs lack an update URL, preventing confusion about why some selections weren't processed. The filter-notify-halt pattern is correctly implemented.
94-94: No issues found. Theservers_countattribute is properly loaded via->counts('servers')on the table query (line 63) and is available on the records passed to thebeforehook. Filament passes the selected records directly from the table to bulk actions without re-fetching, preserving all eager-loaded attributes. This pattern is consistently used elsewhere in the codebase (e.g.,checkIfRecordIsSelectableUsingin UserResource and ListNodes,update_urlaccess in bulk action before hooks) and works correctly.
3 participants
Closes #1962
(I renamed the branch and it closed the PR. I was unable to re-open it)