Update: add ability to ban players on Minecraft Java servers with PlayerCounter plugin#109
Merged
Boy132 merged 3 commits intopelican-dev:mainfrom Feb 20, 2026
Merged
Conversation
Contributor
|
Caution Review failedThe pull request is closed. 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a Minecraft "ban" action to the Filament players page and corresponding English translation keys; Changes
Sequence Diagram(s)sequenceDiagram
participant Admin as Admin (Filament UI)
participant Filament as Filament Page
participant App as Server Backend
participant MC as Minecraft Server
Admin->>Filament: Click "Ban" action (confirm)
Filament->>App: Request: ban player (player id)
App->>MC: Send ban command to Minecraft server
alt success
MC-->>App: Command executed OK
App-->>Filament: 200 OK / success
Filament-->>Admin: Show notification "player_banned"
Filament->>Filament: Refresh players table
else failure
MC-->>App: Error/exception
App-->>Filament: Error response
Filament-->>Admin: Show notification "player_ban_failed"
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
player-counter/src/Filament/Server/Pages/PlayersPage.php (1)
201-220: Minor formatting inconsistency — add blank lines between logical statement groups.The ban action's closure omits the blank lines that all other actions (
exclude_kick,exclude_whitelist,exclude_op) use to separate logical groups within thetry/catchblocks. Keeping the same style aids readability.♻️ Proposed fix
->action(function (array $record) { /** `@var` Server $server */ $server = Filament::getTenant(); + try { $server->send('ban ' . $record['name']); + Notification::make() ->title(trans('player-counter::query.notifications.player_banned')) ->body($record['name']) ->success() ->send(); + $this->refreshPage(); } catch (Exception $exception) { report($exception); + Notification::make() ->title(trans('player-counter::query.notifications.player_ban_failed')) ->body($exception->getMessage()) ->danger() ->send(); } }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@player-counter/src/Filament/Server/Pages/PlayersPage.php` around lines 201 - 220, The closure passed to ->action (the ban action) needs the same blank-line separations used by other actions: add a blank line between obtaining the Server instance ($server = Filament::getTenant();) and the try block, a blank line between the server->send('ban ' . $record['name']); call and the subsequent Notification::make() success block, and a blank line between report($exception); and the Notification::make() danger block in the catch; keep existing variable names and structure (the closure, $record, $server, server->send, report, Notification::make) but insert those blank lines to match the project's formatting style.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
player-counter/lang/en/query.phpplayer-counter/plugin.jsonplayer-counter/src/Filament/Server/Pages/PlayersPage.php
🧰 Additional context used
🧬 Code graph analysis (1)
player-counter/src/Filament/Server/Pages/PlayersPage.php (2)
subdomains/src/Models/Subdomain.php (1)
server(48-51)billing/src/Models/Order.php (1)
server(63-66)
🔇 Additional comments (2)
player-counter/plugin.json (1)
5-5: Version bump looks appropriate.Bumping to
1.0.1for a backwards-compatible feature addition is correct semver hygiene.player-counter/lang/en/query.php (1)
17-17: Translation keys are correct and well-placed.The new
ban,player_banned, andplayer_ban_failedkeys are grouped consistently alongside theirkickcounterparts and match the keys referenced inPlayersPage.php.Also applies to: 42-44
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@player-counter/src/Filament/Server/Pages/PlayersPage.php`:
- Around line 196-220: The ban Action (Action::make('exclude_ban') in
PlayersPage) is destructive but runs immediately; update its definition to
require user confirmation before executing by calling ->requiresConfirmation()
on the Action (optionally configure message/confirm/cancel labels or
modalContent via the same fluent API) so the ban handler only runs after the
user confirms. Ensure the call is added in the Action chain for 'exclude_ban'
alongside existing visible/label/icon/color/action settings.
---
Nitpick comments:
In `@player-counter/src/Filament/Server/Pages/PlayersPage.php`:
- Around line 201-220: The closure passed to ->action (the ban action) needs the
same blank-line separations used by other actions: add a blank line between
obtaining the Server instance ($server = Filament::getTenant();) and the try
block, a blank line between the server->send('ban ' . $record['name']); call and
the subsequent Notification::make() success block, and a blank line between
report($exception); and the Notification::make() danger block in the catch; keep
existing variable names and structure (the closure, $record, $server,
server->send, report, Notification::make) but insert those blank lines to match
the project's formatting style.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
player-counter/src/Filament/Server/Pages/PlayersPage.php (2)
202-221: Minor: add blank lines inside thetryblock to match the kick-action style.The
exclude_kickaction (lines 172–194) separates thesend,Notification, andrefreshPagecalls with blank lines. The ban action omits them, creating a slight inconsistency.♻️ Formatting alignment with kick action
->action(function (array $record) { /** `@var` Server $server */ $server = Filament::getTenant(); + try { $server->send('ban ' . $record['name']); + Notification::make() ->title(trans('player-counter::query.notifications.player_banned')) ->body($record['name']) ->success() ->send(); + $this->refreshPage(); } catch (Exception $exception) { report($exception); + Notification::make() ->title(trans('player-counter::query.notifications.player_ban_failed')) ->body($exception->getMessage()) ->danger() ->send(); } }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@player-counter/src/Filament/Server/Pages/PlayersPage.php` around lines 202 - 221, The try block inside the ban action closure is missing blank lines between the $server->send('ban ' . $record['name']), the Notification::make() chain, and $this->refreshPage(); update the anonymous function used in ->action(...) (the ban action closure where Filament::getTenant() is assigned to $server and $server->send('ban ...') is called) to insert the same blank-line separations used in the exclude_kick action: one blank line after the $server->send(...) call, one blank line after the Notification::make()...->send() call before calling $this->refreshPage(), keeping all existing logic intact.
196-221: Optionally customise the confirmation modal to show the player's name.The default
->requiresConfirmation()modal shows a generic "Are you sure?" heading with no indication of who is being banned. Since ban is irreversible without a manual/pardon, surfacing the target name in the modal reduces the risk of accidentally banning the wrong player.♻️ Proposed enhancement to the confirmation modal
Action::make('exclude_ban') ->visible(fn () => $isMinecraft) ->label(trans('player-counter::query.ban')) ->icon('tabler-hammer') ->color('danger') ->requiresConfirmation() + ->modalHeading(fn (array $record) => trans('player-counter::query.ban') . ' ' . $record['name']) + ->modalDescription(fn (array $record) => $record['name'] . ' will be permanently banned from the server.') ->action(function (array $record) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@player-counter/src/Filament/Server/Pages/PlayersPage.php` around lines 196 - 221, The confirmation modal for Action::make('exclude_ban') is generic; update the Action to show the target player's name by replacing the plain ->requiresConfirmation() with a confirmation configuration that uses record-aware callbacks (e.g. ->requiresConfirmation()->confirmationTitle(fn(array $record) => "Ban {$record['name']}?") and ->confirmationDescription(fn(array $record) => trans(..., ['name' => $record['name']]))) so the modal title/description include record['name'] (you can also set a clearer confirmationButtonLabel/confirmationActionLabel) while keeping the existing action closure that sends 'ban ' . $record['name'] and handles notifications.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
player-counter/src/Filament/Server/Pages/PlayersPage.php
🧰 Additional context used
🧬 Code graph analysis (1)
player-counter/src/Filament/Server/Pages/PlayersPage.php (2)
subdomains/src/Models/Subdomain.php (1)
server(48-51)billing/src/Models/Order.php (1)
server(63-66)
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@player-counter/src/Filament/Server/Pages/PlayersPage.php`:
- Around line 196-201: The destructive ban action Action::make('exclude_ban')
now includes ->requiresConfirmation(), addressing the missing confirmation
concern; no further code changes are required—keep the confirmation present on
the Action declaration (Action::make('exclude_ban')->requiresConfirmation()) to
ensure the destructive flow still prompts users.
---
Nitpick comments:
In `@player-counter/src/Filament/Server/Pages/PlayersPage.php`:
- Around line 202-221: The try block inside the ban action closure is missing
blank lines between the $server->send('ban ' . $record['name']), the
Notification::make() chain, and $this->refreshPage(); update the anonymous
function used in ->action(...) (the ban action closure where
Filament::getTenant() is assigned to $server and $server->send('ban ...') is
called) to insert the same blank-line separations used in the exclude_kick
action: one blank line after the $server->send(...) call, one blank line after
the Notification::make()...->send() call before calling $this->refreshPage(),
keeping all existing logic intact.
- Around line 196-221: The confirmation modal for Action::make('exclude_ban') is
generic; update the Action to show the target player's name by replacing the
plain ->requiresConfirmation() with a confirmation configuration that uses
record-aware callbacks (e.g.
->requiresConfirmation()->confirmationTitle(fn(array $record) => "Ban
{$record['name']}?") and ->confirmationDescription(fn(array $record) =>
trans(..., ['name' => $record['name']]))) so the modal title/description include
record['name'] (you can also set a clearer
confirmationButtonLabel/confirmationActionLabel) while keeping the existing
action closure that sends 'ban ' . $record['name'] and handles notifications.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Little change on the PlayerCounter plugin: add a button to ban people from the server
Summary by CodeRabbit