-
-
Notifications
You must be signed in to change notification settings - Fork 254
Fix: Duplicate bulk deletion notifications #1881
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
Fix: Duplicate bulk deletion notifications #1881
Conversation
Followup for #1877
📝 WalkthroughWalkthroughAdded suppression of the success notification title for the bulk delete action on the files listing by setting the action's successNotificationTitle to null; no other behavior or authorization was changed. Changes
Pre-merge checks✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
|
All contributors have signed the CLA ✍️ ✅ |
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.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Filament/Server/Resources/Files/Pages/ListFiles.php (1)
447-466: Missing confirmation dialog for bulk file deletion.The switch from
DeleteBulkActiontoBulkActionresolves the duplicate notification issue, but removes the built-in confirmation thatDeleteBulkActionprovided. The bulk delete action requires an explicitrequiresConfirmation()call, which is currently missing. Filament'sBulkActionfully supports this method.Add confirmation to match the single record delete at line 362:
BulkAction::make('delete selected') ->color('danger') ->icon('heroicon-o-trash') ->authorize(fn () => user()?->can(Permission::ACTION_FILE_DELETE, $server)) + ->requiresConfirmation() ->action(function (Collection $files) {
🧹 Nitpick comments (1)
app/Filament/Server/Resources/Files/Pages/ListFiles.php (1)
447-449: Icon inconsistency with single delete action.The bulk delete action uses
heroicon-o-trashwhile the single record delete action at line 361 usestabler-trash. For visual consistency, consider using the same icon family throughout.Apply this diff to align with the existing icon style:
BulkAction::make('delete selected') ->color('danger') - ->icon('heroicon-o-trash') + ->icon('tabler-trash') ->authorize(fn () => user()?->can(Permission::ACTION_FILE_DELETE, $server))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/Filament/Server/Resources/Files/Pages/ListFiles.php(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-16T19:30:21.443Z
Learnt from: rmartinoscar
Repo: pelican-dev/panel PR: 1708
File: app/Filament/App/Resources/Servers/Pages/ListServers.php:115-116
Timestamp: 2025-09-16T19:30:21.443Z
Learning: The Filament `recordActions` method accepts both arrays and ActionGroup instances (signature: `array | ActionGroup $actions`), so ActionGroup can be passed directly without wrapping in an array.
Applied to files:
app/Filament/Server/Resources/Files/Pages/ListFiles.php
📚 Learning: 2025-09-16T19:32:01.343Z
Learnt from: rmartinoscar
Repo: pelican-dev/panel PR: 1708
File: app/Filament/App/Resources/Servers/Pages/ListServers.php:228-266
Timestamp: 2025-09-16T19:32:01.343Z
Learning: ActionGroup in Filament automatically hides itself when all child actions are not visible, so additional hidden() logic to prevent empty groups is unnecessary.
Applied to files:
app/Filament/Server/Resources/Files/Pages/ListFiles.php
|
I have read the CLA Document and I hereby sign the CLA |
|
Please follow the some fix i used for the original pr |
Boy132
left a comment
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.
Thank you for your contribution!
Followup for #1877