fix: respect custom selectability in bulk actions when selecting all#20051
fix: respect custom selectability in bulk actions when selecting all#20051hamdyelbatal122 wants to merge 3 commits into
Conversation
people-sea
left a comment
people-sea
left a comment
There was a problem hiding this comment.
Can you explain how this solves the query-level problem mentioned in your issue? SQL still cannot know about the PHP selectability closure, and this PR appears to filter only after records are resolved.
|
@people-sea The main issue this PR fixes is the performance/memory overhead. Previously, if custom selectability was defined, Filament disabled deselection tracking entirely (forcing Livewire to track and pass thousands of individual IDs over the wire, which hits memory limits on large tables). By filtering the records in PHP after they are resolved (in memory or chunked via lazy collections), we can safely restore deselection tracking. This prevents Livewire from bloating with IDs, while ensuring the bulk action is never executed on non-selectable records. |
|
I think this direction is unsafe: it re-enables deselection tracking while only filtering the resolved |
|
You're completely right @people-sea. I overlooked query-based bulk actions (like To fix this, I updated I also added a unit test to verify this. |
There was a problem hiding this comment.
Pull request overview
This PR fixes bulk actions incorrectly operating on non-selectable records when using “Select all matching records” by ensuring selectability rules (checkIfRecordIsSelectableUsing() / isRecordSelectable()) are respected during bulk record resolution and execution.
Changes:
- Re-enables deselection tracking even when a custom record-selectability callback is configured.
- Filters resolved selected records through
isRecordSelectable()for both collection and lazy-collection selection flows. - Adds unit tests to ensure non-selectable records are ignored for both record-based and query-based bulk actions when “select all” tracking is enabled.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
tests/src/Tables/Actions/BulkActionTest.php |
Adds coverage ensuring “select all matching” bulk actions ignore non-selectable records (including query-based deletes). |
packages/tables/src/Table/Concerns/HasBulkActions.php |
Allows deselection tracking even when custom selectability is configured. |
packages/tables/src/Concerns/HasBulkActions.php |
Filters resolved selected records by selectability; updates selection query behavior to account for selectability. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if ($table->checksIfRecordIsSelectable()) { | ||
| $selectableKeys = []; | ||
| (clone $query)->lazy(chunkSize: 1000)->each(function (Model $record) use (&$selectableKeys, $table) { | ||
| if ($table->isRecordSelectable($record)) { | ||
| $selectableKeys[] = $record->getKey(); | ||
| } | ||
| }); | ||
| $query = $table->getQuery()->whereKey($selectableKeys); | ||
| } |
| if ($table->checksIfRecordIsSelectable()) { | ||
| $selectableKeys = []; | ||
| (clone $query)->lazy(chunkSize: 1000)->each(function (Model $record) use (&$selectableKeys, $table) { | ||
| if ($table->isRecordSelectable($record)) { | ||
| $selectableKeys[] = $record->getKey(); | ||
| } | ||
| }); | ||
| $query->whereKey($selectableKeys); | ||
| } |
| // 2 published (selectable) and 1 unpublished (non-selectable) | ||
| $publishedPosts = Post::factory()->count(2)->create(['is_published' => true]); | ||
| $unpublishedPost = Post::factory()->create(['is_published' => false]); | ||
|
|
Fixes #20050
This PR addresses the issue where bulk actions could run on non-selectable records when using the "Select all matching records" feature.
Instead of disabling deselection tracking across pages (which was the previous workaround and caused heavy performance/memory overhead on large tables), this PR:
isRecordSelectable()check right after the selected records are resolved (for both collections and lazy collections).totalSelectedRecordsCountusing the resolved collection's count instead of a direct database query count, since the database count doesn't account for the PHP selectability closure.Added a unit test in
BulkActionTestto cover this scenario and verify that non-selectable records are ignored when executing bulk actions withisTrackingDeselectedTableRecords = true.