View pages #1998
View pages #1998
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughConsolidates form schema definitions into Resource classes for Eggs, Nodes, and Servers; adds dedicated View pages for those resources; replaces inline form definitions in Edit pages with delegation to Resource::schema; and adds per-record authorization-aware recordUrl routing in list pages. Several lifecycle hooks and per-page action behaviors were adjusted or removed. Changes
Possibly related PRs
Pre-merge checks❌ Failed checks (1 warning)
✅ 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/Filament/Admin/Resources/Servers/Pages/EditServer.php (2)
53-78: Missing authorization check and inconsistent exception handling.The
Deleteaction is missing the->authorize()check that exists inViewServer. Additionally,ViewServercatches bothConnectionExceptionandPredisConnectionException, but this only catchesConnectionException.Action::make('Delete') ->color('danger') ->label(trans('filament-actions::delete.single.label')) ->modalHeading(trans('filament-actions::delete.single.modal.heading', ['label' => $server->name])) ->modalSubmitActionLabel(trans('filament-actions::delete.single.label')) ->requiresConfirmation() ->action(function (Server $server, ServerDeletionService $service) { try { $service->handle($server); return redirect(ListServers::getUrl(panel: 'admin')); - } catch (ConnectionException) { + } catch (ConnectionException|\Predis\Connection\ConnectionException) { cache()->put("servers.$server->uuid.canForceDelete", true, now()->addMinutes(5)); return Notification::make() ->title(trans('admin/server.notifications.error_server_delete')) ->body(trans('admin/server.notifications.error_server_delete_body')) ->color('warning') ->icon('tabler-database') ->warning() ->send(); } }) ->hidden(fn () => $canForceDelete) + ->authorize(fn (Server $server) => user()?->can('delete server', $server)) ->icon('tabler-trash') ->iconButton()->iconSize(IconSize::ExtraLarge),
79-95: Inconsistent exception handling in ForceDelete.Similar to the
Deleteaction, this only catchesConnectionExceptionwhileViewServercatches bothConnectionExceptionandPredisConnectionException. Also, there's no user notification on failure.
🧹 Nitpick comments (9)
app/Filament/Admin/Resources/Eggs/Pages/ViewEgg.php (1)
11-21: Structure is correct and consistent with EditEgg.The ViewEgg page properly extends
ViewRecordand delegates to the shared schema. The traits provide header customization capability.Consider adding an Edit action for users with update permission:
Users viewing a record they can edit may expect a quick way to switch to edit mode. You could add a
getDefaultHeaderActions()method:protected function getDefaultHeaderActions(): array { return [ \Filament\Actions\EditAction::make() ->visible(fn () => user()?->can('update egg', $this->record)), ]; }app/Filament/Admin/Resources/Servers/Pages/ListServers.php (1)
106-113: Consider adding authorization visibility to EditAction and ViewAction.Based on learnings from this codebase, the typical UX pattern is to hide ViewAction when the user has edit privileges (showing EditAction instead), and vice versa. Currently both actions are always visible.
If this is intentional for the list page UX, this comment can be disregarded.
EditAction::make() ->icon('tabler-pencil') ->hiddenLabel() - ->iconSize(IconSize::Large), + ->iconSize(IconSize::Large) + ->hidden(fn (Server $server) => !user()?->can('update server', $server)), ViewAction::make() ->hiddenLabel() ->icon('tabler-eye') - ->iconSize(IconSize::ExtraLarge), + ->iconSize(IconSize::ExtraLarge) + ->hidden(fn (Server $server) => user()?->can('update server', $server)),app/Filament/Admin/Resources/Servers/RelationManagers/AllocationsRelationManager.php (1)
62-69: Consider disabling the icon column action when user lacks permission.The permission check silently returns without user feedback. Consider adding a
->disabled()modifier to visually indicate the action is unavailable, similar to the pattern used formake-primaryat line 88.->action(function (Allocation $allocation) { if (!user()?->can('update server', $this->getOwnerRecord())) { return; } $this->getOwnerRecord()->update(['allocation_id' => $allocation->id]); $this->deselectAllTableRecords(); }) +->disabled(fn () => !user()?->can('update server', $this->getOwnerRecord()))app/Filament/Admin/Resources/Nodes/NodeResource.php (1)
148-161: Consider caching repeatedsystemInformation()calls.The
systemInformation()method is called four times within this fieldset. If this method performs network calls or expensive operations, consider caching the result to improve performance.app/Filament/Admin/Resources/Servers/Pages/ViewServer.php (3)
14-14: Unused import.
ActionGroupis imported but not used in this file.-use Filament\Actions\ActionGroup;
30-37: PHPDoc exceptions may be inaccurate.The
@throws \Random\RandomExceptionand@throws \Exceptionannotations suggest the method throws these exceptions, butServerResource::schema()likely doesn't throw them directly from this call. Consider removing or verifying if these are actually thrown.
56-60: Redundant closure parameter.The
$servervariable is already available from line 51. The closure parameter shadows it unnecessarily.Action::make('console') ->label(trans('admin/server.console')) ->icon('tabler-terminal') ->iconButton()->iconSize(IconSize::ExtraLarge) - ->url(fn (Server $server) => Console::getUrl(panel: 'server', tenant: $server)), + ->url(fn () => Console::getUrl(panel: 'server', tenant: $server)),app/Filament/Admin/Resources/Servers/Pages/EditServer.php (1)
48-52: Redundant closure parameter.Similar to
ViewServer, the$servervariable is already available from line 43. The closure parameter is unnecessary.Action::make('console') ->label(trans('admin/server.console')) ->icon('tabler-terminal') ->iconButton()->iconSize(IconSize::ExtraLarge) - ->url(fn (Server $server) => Console::getUrl(panel: 'server', tenant: $server)), + ->url(fn () => Console::getUrl(panel: 'server', tenant: $server)),app/Filament/Admin/Resources/Nodes/Pages/EditNode.php (1)
86-109: Notification only sent when daemon sync succeeds.The
getSavedNotification()is called inside the try block after checking if daemon changes exist. If no daemon-related fields changed, the notification is still sent. However, if aConnectionExceptionoccurs, no success notification is shown even though the database save succeeded. Consider notifying the user that the save succeeded but daemon sync failed.protected function afterSave(): void { $this->refresh(); /** @var Node $node */ $node = $this->getRecord(); $changed = collect($node->getChanges())->except(['updated_at', 'name', 'tags', 'public', 'maintenance_mode', 'memory', 'memory_overallocate', 'disk', 'disk_overallocate', 'cpu', 'cpu_overallocate'])->all(); try { if ($changed) { $this->daemonSystemRepository->setNode($node)->update($node); } $this->getSavedNotification()?->send(); } catch (ConnectionException) { Notification::make() ->title(trans('admin/node.error_connecting', ['node' => $node->name])) - ->body(trans('admin/node.error_connecting_description')) + ->body(trans('admin/node.error_connecting_description') . ' ' . trans('admin/node.changes_saved_locally')) ->color('warning') ->icon('tabler-database') ->warning() ->send(); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
app/Filament/Admin/Resources/Eggs/EggResource.php(2 hunks)app/Filament/Admin/Resources/Eggs/Pages/EditEgg.php(1 hunks)app/Filament/Admin/Resources/Eggs/Pages/ListEggs.php(1 hunks)app/Filament/Admin/Resources/Eggs/Pages/ViewEgg.php(1 hunks)app/Filament/Admin/Resources/Nodes/NodeResource.php(3 hunks)app/Filament/Admin/Resources/Nodes/Pages/EditNode.php(2 hunks)app/Filament/Admin/Resources/Nodes/Pages/ListNodes.php(1 hunks)app/Filament/Admin/Resources/Nodes/Pages/ViewNode.php(1 hunks)app/Filament/Admin/Resources/Servers/Pages/EditServer.php(3 hunks)app/Filament/Admin/Resources/Servers/Pages/ListServers.php(3 hunks)app/Filament/Admin/Resources/Servers/Pages/ViewServer.php(1 hunks)app/Filament/Admin/Resources/Servers/RelationManagers/AllocationsRelationManager.php(2 hunks)app/Filament/Admin/Resources/Servers/ServerResource.php(3 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-12-02T19:55:23.957Z
Learnt from: rmartinoscar
Repo: pelican-dev/panel PR: 1837
File: app/Filament/Server/Resources/Schedules/ScheduleResource.php:337-338
Timestamp: 2025-12-02T19:55:23.957Z
Learning: In Filament resources, when ViewAction is hidden using `->hidden(fn ($record) => static::getEditAuthorizationResponse($record)->allowed())`, this is intentional UX design: the ViewAction (read-only) should be hidden when users have edit privileges, showing the EditAction instead. The ViewAction serves as a fallback for users who only have view permissions. This pattern is correct and should not be flagged as inverted logic.
Applied to files:
app/Filament/Admin/Resources/Servers/Pages/ListServers.phpapp/Filament/Admin/Resources/Servers/RelationManagers/AllocationsRelationManager.phpapp/Filament/Admin/Resources/Servers/Pages/ViewServer.php
📚 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/Admin/Resources/Servers/Pages/ListServers.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/Admin/Resources/Servers/Pages/ListServers.php
📚 Learning: 2025-11-05T22:21:31.863Z
Learnt from: notAreYouScared
Repo: pelican-dev/panel PR: 1865
File: app/Filament/Admin/Resources/Nodes/Pages/EditNode.php:682-682
Timestamp: 2025-11-05T22:21:31.863Z
Learning: In app/Filament/Admin/Resources/Nodes/Pages/EditNode.php, the diagnostics tab's upload action intentionally does not use the iconButton() modifier, while the pull action does. This UI difference is intentional.
Applied to files:
app/Filament/Admin/Resources/Nodes/NodeResource.phpapp/Filament/Admin/Resources/Nodes/Pages/ListNodes.phpapp/Filament/Admin/Resources/Nodes/Pages/EditNode.php
📚 Learning: 2025-08-12T17:33:57.388Z
Learnt from: lancepioch
Repo: pelican-dev/panel PR: 1595
File: app/Services/Servers/ServerCreationService.php:182-184
Timestamp: 2025-08-12T17:33:57.388Z
Learning: In the Pelican Panel codebase, when updating allocations to assign them to servers, the preferred pattern is to use whereNull('server_id'), lockForUpdate(), and direct property assignment with save() rather than mass update methods, to prevent race conditions and mass-assignment issues while ensuring model events fire properly.
Applied to files:
app/Filament/Admin/Resources/Servers/RelationManagers/AllocationsRelationManager.php
🧬 Code graph analysis (13)
app/Filament/Admin/Resources/Servers/Pages/ListServers.php (2)
app/helpers.php (1)
user(127-130)app/Models/User.php (1)
canAccessTenant(438-451)
app/Filament/Admin/Resources/Eggs/EggResource.php (6)
app/Filament/Admin/Resources/Eggs/Pages/ViewEgg.php (1)
ViewEgg(11-22)app/Filament/Components/Forms/Fields/CopyFrom.php (3)
CopyFrom(10-73)process(27-51)script(53-72)app/Models/EggVariable.php (1)
EggVariable(34-117)app/Filament/Admin/Resources/Roles/RoleResource.php (1)
getDefaultPages(205-213)app/Filament/Admin/Resources/Users/UserResource.php (1)
getDefaultPages(475-483)app/Filament/Admin/Resources/Eggs/Pages/ListEggs.php (1)
ListEggs(28-127)
app/Filament/Admin/Resources/Servers/Pages/EditServer.php (2)
app/Models/Server.php (1)
Server(134-525)app/Filament/Server/Pages/Console.php (1)
Console(29-219)
app/Filament/Admin/Resources/Nodes/NodeResource.php (6)
app/Filament/Admin/Resources/Nodes/RelationManagers/ServersRelationManager.php (1)
ServersRelationManager(12-70)app/Filament/Admin/Resources/Nodes/Pages/ListNodes.php (1)
ListNodes(21-90)app/Filament/Admin/Resources/Nodes/Pages/ViewNode.php (1)
ViewNode(14-48)app/Filament/Admin/Resources/Nodes/RelationManagers/AllocationsRelationManager.php (1)
AllocationsRelationManager(28-132)app/Repositories/Daemon/DaemonSystemRepository.php (2)
DaemonSystemRepository(9-61)getDiagnostics(39-48)app/Helpers/Utilities.php (1)
Utilities(11-58)
app/Filament/Admin/Resources/Eggs/Pages/ViewEgg.php (1)
app/Filament/Admin/Resources/Eggs/EggResource.php (2)
EggResource(46-503)schema(98-491)
app/Filament/Admin/Resources/Eggs/Pages/ListEggs.php (4)
app/Models/Egg.php (1)
Egg(60-332)app/helpers.php (1)
user(127-130)app/Filament/Admin/Resources/Eggs/EggResource.php (1)
EggResource(46-503)app/Filament/Server/Resources/Files/Pages/EditFiles.php (1)
getUrl(285-288)
app/Filament/Admin/Resources/Nodes/Pages/ViewNode.php (3)
app/Filament/Admin/Resources/Nodes/NodeResource.php (2)
NodeResource(55-840)schema(121-839)app/Filament/Admin/Resources/Servers/Pages/ViewServer.php (2)
form(34-37)getRelationManagers(39-45)app/Filament/Admin/Resources/Nodes/Pages/EditNode.php (1)
form(33-36)
app/Filament/Admin/Resources/Servers/RelationManagers/AllocationsRelationManager.php (2)
app/Models/Allocation.php (1)
Allocation(48-141)app/Models/Server.php (1)
allocation(280-283)
app/Filament/Admin/Resources/Servers/Pages/ViewServer.php (2)
app/Services/Servers/ServerDeletionService.php (2)
ServerDeletionService(15-86)withForce(31-36)app/Filament/Admin/Resources/Servers/Pages/ListServers.php (2)
getDefaultHeaderActions(122-129)ListServers(23-130)
app/Filament/Admin/Resources/Nodes/Pages/ListNodes.php (2)
app/Filament/Admin/Resources/Nodes/NodeResource.php (1)
NodeResource(55-840)app/Filament/Server/Resources/Files/Pages/EditFiles.php (1)
getUrl(285-288)
app/Filament/Admin/Resources/Nodes/Pages/EditNode.php (4)
app/Filament/Admin/Resources/Nodes/NodeResource.php (2)
NodeResource(55-840)schema(121-839)app/Filament/Admin/Resources/Webhooks/Pages/CreateWebhookConfiguration.php (1)
getFormActions(36-39)app/Filament/Admin/Resources/Nodes/Pages/CreateNode.php (1)
getFormActions(425-428)app/Filament/Admin/Resources/Nodes/Pages/ViewNode.php (1)
getSavedNotification(34-37)
app/Filament/Admin/Resources/Servers/ServerResource.php (14)
app/Filament/Admin/Resources/Servers/Pages/CreateServer.php (1)
CreateServer(46-944)app/Filament/Admin/Resources/Servers/Pages/EditServer.php (1)
EditServer(23-114)app/Filament/Admin/Resources/Servers/Pages/ListServers.php (1)
ListServers(23-130)app/Filament/Admin/Resources/Servers/Pages/ViewServer.php (1)
ViewServer(23-110)app/Filament/Admin/Resources/Servers/RelationManagers/AllocationsRelationManager.php (1)
AllocationsRelationManager(30-183)app/Filament/Components/Actions/PreviewStartupAction.php (1)
PreviewStartupAction(11-31)app/Filament/Components/StateCasts/ServerConditionStateCast.php (1)
ServerConditionStateCast(10-43)app/Models/Allocation.php (1)
Allocation(48-141)app/Models/Egg.php (1)
Egg(60-332)app/Models/Server.php (6)
Server(134-525)condition(513-518)isSuspended(254-257)isInConflictState(412-415)transfer(370-373)allocation(280-283)app/Repositories/Daemon/DaemonServerRepository.php (2)
DaemonServerRepository(13-165)getInstallLogs(146-152)app/Services/Servers/RandomWordService.php (2)
RandomWordService(5-19)word(15-18)app/Filament/Server/Resources/Files/Pages/EditFiles.php (1)
route(290-299)app/Filament/Server/Resources/Files/Pages/ListFiles.php (1)
route(687-696)
app/Filament/Admin/Resources/Eggs/Pages/EditEgg.php (1)
app/Filament/Admin/Resources/Eggs/EggResource.php (2)
EggResource(46-503)schema(98-491)
🔇 Additional comments (24)
app/Filament/Admin/Resources/Eggs/Pages/ListEggs.php (1)
42-45: LGTM! Permission-based routing is well-implemented.The conditional URL routing based on
user()?->can('update egg', $egg)correctly directs users to the edit page when they have update permission, and to the view page otherwise. The null-safe operator appropriately handles unauthenticated users by falling through to the view page.app/Filament/Admin/Resources/Eggs/Pages/EditEgg.php (1)
25-28: Good refactor: Centralizing schema in EggResource.Delegating to
EggResource::schema($schema)eliminates duplication and ensures both EditEgg and ViewEgg use the same form definition. This improves maintainability.app/Filament/Admin/Resources/Eggs/EggResource.php (5)
95-100: Good centralization of the schema definition.Moving the schema to
EggResourceenables reuse across EditEgg and ViewEgg pages, following DRY principles.
165-172: SSRF protection is present but has a known TOCTOU limitation.The DNS resolution happens before the actual fetch, creating a small window for DNS rebinding attacks. This is a common pattern and the 3-second timeout limits exposure. For admin-only image uploads, the current protection is reasonable.
377-403: LGTM! Variable repeater with proper validation and defaults.The unique constraints scoped by
egg_id, combined with mutation hooks ensuring default values, prevent both duplicate variables and null constraint violations.
496-501: View page registration follows established patterns.The
'view' => ViewEgg::route('/{record}')registration is consistent with other resources likeRoleResourceandUserResourceshown in the relevant snippets.
159-163: Extension-based MIME detection is used, but does not pose a security risk in this rendering context.The image is rendered via Filament's
Imagecomponent in an<img>tag, which safely sandboxes SVG content. SVG is intentionally supported (line 156), and even with mismatched MIME types, script execution in<img>tags is prevented by browser security models.app/Filament/Admin/Resources/Nodes/Pages/ListNodes.php (1)
31-34: LGTM! Per-record authorization routing implemented correctly.The conditional URL routing based on
update nodepermission aligns with the PR's objective of making permissions functional. Users with edit permissions are routed to the edit page, while others see the read-only view page.app/Filament/Admin/Resources/Servers/Pages/ListServers.php (1)
33-36: LGTM! Per-record authorization routing implemented correctly.The conditional URL routing based on
update serverpermission is consistent with the ListNodes implementation and supports the PR's permission system.app/Filament/Admin/Resources/Servers/RelationManagers/AllocationsRelationManager.php (2)
79-89: LGTM! Permission checks properly guard the make-primary action.The combination of
disabled()and the inner permission check provides defense-in-depth authorization.
90-99: LGTM! Lock and unlock actions properly disabled based on permissions.The
disabled()modifier correctly prevents unauthorized users from triggering these actions.app/Filament/Admin/Resources/Nodes/Pages/ViewNode.php (1)
1-48: LGTM! ViewNode page follows established patterns.The implementation correctly:
- Delegates the form schema to
NodeResource::schema($schema)(consistent with EditNode)- Registers appropriate relation managers
- Uses the customization traits for header actions and widgets
app/Filament/Admin/Resources/Nodes/NodeResource.php (3)
100-109: LGTM! View page route correctly added.The new
'view' => ViewNode::route('/{record}')route integrates properly with the existing page structure, enabling the permission-based routing from ListNodes.
688-836: LGTM! Diagnostics tab implementation is appropriate.The tab is properly guarded by permission checks, and the pull/upload/clear workflow provides a good user experience for diagnostics. The external upload to
logs.pelican.devis user-initiated with clear UI feedback.
192-202: The helperText logic is correct and the translation key provides appropriate guidance.The translation key
admin/node.error(line 44 oflang/en/admin/node.php) contains helpful text explaining that "This is the domain name that points to your node's IP Address." This is not an error message but rather contextual guidance for when a domain name is entered. The helperText behavior is intentional: it returns domain-focused help text when the input is a domain name, rather than treating domains as invalid input. No changes needed.app/Filament/Admin/Resources/Servers/Pages/ViewServer.php (1)
39-45: LGTM!The relation managers are correctly configured for the view page.
app/Filament/Admin/Resources/Servers/Pages/EditServer.php (1)
36-36: LGTM!Form delegation to
ServerResource::schema()aligns with the centralized schema pattern used across other resources.app/Filament/Admin/Resources/Nodes/Pages/EditNode.php (3)
35-35: LGTM!Form delegation to
NodeResource::schema()follows the centralized schema pattern.
55-62: LGTM!The port normalization logic correctly ensures
daemon_listenequalsdaemon_connectwhen the node is not behind a proxy.
77-82: LGTM!The explicit save action with proper form binding is correctly configured.
app/Filament/Admin/Resources/Servers/ServerResource.php (4)
178-185: LGTM on SSRF protection.Good security practice validating that the resolved IP is not in private or reserved ranges before fetching the image.
999-1006: Inverted visibility logic for hint messages.The visibility conditions appear inverted. Currently,
server_suspendhint is hidden when user can update AND server is suspended, andserver_unsuspendhint is hidden when user can update AND server is not suspended. This seems backwards - hints should likely be visible when the corresponding action is available.ToggleButtons::make('server_suspend') ->hiddenLabel() - ->hidden(fn (Server $server) => $server->isSuspended() && !user()?->can('update server', $server)) + ->hidden(fn (Server $server) => $server->isSuspended() || !user()?->can('update server', $server)) ->hint(trans('admin/server.notifications.server_suspend_help')), ToggleButtons::make('server_unsuspend') ->hiddenLabel() - ->hidden(fn (Server $server) => !$server->isSuspended() && !user()?->can('update server', $server)) + ->hidden(fn (Server $server) => !$server->isSuspended() || !user()?->can('update server', $server)) ->hint(trans('admin/server.notifications.server_unsuspend_help')),Please verify the intended behavior for these hint visibility conditions.
1081-1112: LGTM!The
transferServer()method correctly builds the allocation selection UI with proper node filtering and permission checks.
1155-1155: LGTM!The view page route is correctly registered.
| ToggleButtons::make('unlimited_mem') | ||
| ->dehydrated() | ||
| ->label(trans('admin/node.memory'))->inlineLabel()->inline() | ||
| ->afterStateUpdated(fn (Set $set) => $set('memory', 0)) | ||
| ->afterStateUpdated(fn (Set $set) => $set('memory_overallocate', 0)) | ||
| ->formatStateUsing(fn (Get $get) => $get('memory') == 0) |
There was a problem hiding this comment.
Potential issue: Multiple afterStateUpdated calls may override each other.
Filament typically only executes the last afterStateUpdated callback. The first call setting memory to 0 may be overridden by the second call setting memory_overallocate to 0.
Consider combining into a single callback:
ToggleButtons::make('unlimited_mem')
->dehydrated()
->label(trans('admin/node.memory'))->inlineLabel()->inline()
- ->afterStateUpdated(fn (Set $set) => $set('memory', 0))
- ->afterStateUpdated(fn (Set $set) => $set('memory_overallocate', 0))
+ ->afterStateUpdated(function (Set $set) {
+ $set('memory', 0);
+ $set('memory_overallocate', 0);
+ })The same pattern should be applied to unlimited_disk (lines 514-515) and unlimited_cpu (lines 570-571).
🤖 Prompt for AI Agents
In app/Filament/Admin/Resources/Nodes/NodeResource.php around lines 449-454, the
ToggleButtons for 'unlimited_mem' uses two separate afterStateUpdated callbacks
which can override each other; combine them into a single afterStateUpdated
callback that sets both 'memory' and 'memory_overallocate' to 0 (respectively)
when toggled, and apply the same change for 'unlimited_disk' (lines ~514-515) to
set both disk fields in one callback and for 'unlimited_cpu' (lines ~570-571) to
set both CPU-related fields in one callback so each toggle updates all related
fields atomically.
| ->action(function (Node $node, NodeUpdateService $nodeUpdateService, $livewire) { | ||
| try { | ||
| $nodeUpdateService->handle($node, [], true); | ||
| } catch (Exception) { | ||
| Notification::make() | ||
| ->title(trans('admin/node.error_connecting', ['node' => $node->name])) | ||
| ->body(trans('admin/node.error_connecting_description')) | ||
| ->color('warning') | ||
| ->icon('tabler-database') | ||
| ->warning() | ||
| ->send(); | ||
|
|
||
| } | ||
| Notification::make()->success()->title(trans('admin/node.token_reset'))->send(); | ||
| $livewire->refresh(); | ||
| }), |
There was a problem hiding this comment.
Bug: Success notification sent regardless of exception.
When an exception occurs, the code sends a warning notification but then continues to send the success notification at line 682. The success notification should only be sent on successful execution.
->action(function (Node $node, NodeUpdateService $nodeUpdateService, $livewire) {
try {
$nodeUpdateService->handle($node, [], true);
+ Notification::make()->success()->title(trans('admin/node.token_reset'))->send();
+ $livewire->refresh();
} catch (Exception) {
Notification::make()
->title(trans('admin/node.error_connecting', ['node' => $node->name]))
->body(trans('admin/node.error_connecting_description'))
->color('warning')
->icon('tabler-database')
->warning()
->send();
-
+ return;
}
- Notification::make()->success()->title(trans('admin/node.token_reset'))->send();
- $livewire->refresh();
}),| ->action(function (Server $server, ServerDeletionService $service) { | ||
| try { | ||
| $service->withForce()->handle($server); | ||
|
|
||
| return redirect(ListServers::getUrl(panel: 'admin')); | ||
| } catch (ConnectionException|PredisConnectionException) { | ||
| return cache()->forget("servers.$server->uuid.canForceDelete"); | ||
| } |
There was a problem hiding this comment.
Silent failure on ForceDelete connection error.
When ForceDelete fails due to a connection exception, the cache flag is cleared but no notification is shown to the user. This could leave the user confused about why the action didn't complete.
->action(function (Server $server, ServerDeletionService $service) {
try {
$service->withForce()->handle($server);
return redirect(ListServers::getUrl(panel: 'admin'));
} catch (ConnectionException|PredisConnectionException) {
- return cache()->forget("servers.$server->uuid.canForceDelete");
+ cache()->forget("servers.$server->uuid.canForceDelete");
+
+ return Notification::make()
+ ->title(trans('admin/server.notifications.error_server_delete'))
+ ->body(trans('admin/server.notifications.error_server_delete_body'))
+ ->color('danger')
+ ->icon('tabler-database')
+ ->danger()
+ ->send();
}
})🤖 Prompt for AI Agents
In app/Filament/Admin/Resources/Servers/Pages/ViewServer.php around lines 94 to
101, the catch block for ConnectionException|PredisConnectionException only
clears the cache flag and silently fails; update the catch to both clear the
cache and surface a user-facing notification (e.g., Filament/Toast/Notification)
indicating the force-delete failed due to a connection error, then return an
appropriate redirect or back response so the user sees the notification; keep
the existing cache()->forget call and include a concise, user-friendly message
and optional logging of the exception for diagnostics.
| ->minItems(fn (Select $select) => $select->getMaxItems()) | ||
| ->maxItems(fn (Select $select, Server $server) => $select->isDisabled() ? null : $server->allocations->count() - 1 || !user()?->can('update server', $server)) |
There was a problem hiding this comment.
Operator precedence issue in maxItems calculation.
The || has lower precedence than ?:, causing unexpected evaluation. The expression $server->allocations->count() - 1 || !user()?->can('update server', $server) evaluates the || first.
- ->maxItems(fn (Select $select, Server $server) => $select->isDisabled() ? null : $server->allocations->count() - 1 || !user()?->can('update server', $server))
+ ->maxItems(fn (Select $select, Server $server) => $select->isDisabled() ? null : $server->allocations->count() - 1)The permission check is already handled by the ->disabled() call on line 1102. This line should just return the count.
🤖 Prompt for AI Agents
In app/Filament/Admin/Resources/Servers/ServerResource.php around lines 1104 to
1105, the maxItems callback uses `||` which causes operator precedence bugs and
incorrectly mixes a permission check that is already handled by ->disabled();
change the callback to only return null when the Select is disabled, otherwise
return `$server->allocations->count() - 1` (remove the `|| !user()?->can(...)`
part) so the expression is evaluated correctly.
|
I only made Rabbit suggestions on things I actually messed up, everything else is already as it was before, so unless someone asks to change it, will stay like this. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/Filament/Admin/Resources/Servers/ServerResource.php (1)
1103-1103: Operator precedence issue in maxItems calculation.This issue was already flagged in a previous review. The
||operator has lower precedence than the ternary operator, causing the expression to return a boolean instead of an integer. The permission check is already handled by->disabled()on line 1100.Apply this fix:
- ->maxItems(fn (Select $select, Server $server) => $select->isDisabled() ? null : $server->allocations->count() - 1 || !user()?->can('update server', $server)) + ->maxItems(fn (Select $select, Server $server) => $select->isDisabled() ? null : $server->allocations->count() - 1)
🧹 Nitpick comments (2)
app/Filament/Admin/Resources/Servers/ServerResource.php (2)
194-194: Consider using a proper HTTP client instead of@file_get_contents.While the
@error suppression operator prevents warnings and the try-catch handles errors, using a proper HTTP client (like Guzzle) would provide better error handling and more control over the request.Apply this approach:
use Illuminate\Support\Facades\Http; // In the try block: $response = Http::timeout(3) ->withOptions(['max_redirects' => 3]) ->get($state); if (!$response->successful() || $response->body() === null) { throw new \Exception(trans('admin/egg.import.image_error')); } $imageContent = $response->body(); if (strlen($imageContent) > 262144) { // 256KB throw new \Exception(trans('admin/egg.import.image_too_large')); }
102-1072: Consider extracting sub-schemas into separate methods.The
schema()method is over 900 lines long, which makes it difficult to maintain and test. While Filament schema definitions can be lengthy, consider extracting logical sections (information tab, environment tab, egg tab, etc.) into separate protected methods that return schema components.Example approach:
public static function schema(Schema $schema): Schema { return $schema->components([ Tabs::make('Tabs') ->persistTabInQueryString() ->columns([...]) ->tabs([ self::informationTab(), self::environmentTab(), self::eggTab(), self::mountsTab(), self::actionsTab(), ]), ]); } protected static function informationTab(): Tab { return Tab::make('information') ->label(trans('admin/server.tabs.information')) ->icon('tabler-info-circle') ->schema([ // ... schema components ]); } // ... other tab methodsThis would improve readability and make individual sections easier to test and maintain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/Filament/Admin/Resources/Nodes/Pages/ViewNode.php(1 hunks)app/Filament/Admin/Resources/Servers/ServerResource.php(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/Filament/Admin/Resources/Nodes/Pages/ViewNode.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-12T17:33:57.388Z
Learnt from: lancepioch
Repo: pelican-dev/panel PR: 1595
File: app/Services/Servers/ServerCreationService.php:182-184
Timestamp: 2025-08-12T17:33:57.388Z
Learning: In the Pelican Panel codebase, when updating allocations to assign them to servers, the preferred pattern is to use whereNull('server_id'), lockForUpdate(), and direct property assignment with save() rather than mass update methods, to prevent race conditions and mass-assignment issues while ensuring model events fire properly.
Applied to files:
app/Filament/Admin/Resources/Servers/ServerResource.php
|
@Boy132 I'll let you be the judge! |
|
@JoanFo1456 Mark as ready once the conflicts are resolved, tyvm! |
|
All contributors have signed the CLA ✍️ ✅ |
JoanFo1456
force-pushed
the
admin/view-pages
branch
from
cdf2886 to
0bb6947
Compare
3 participants
Continues with Boy's work (#583)
This adds the missing view pages, making permissions actually useful.
I followed what I thought it would be view, if that's not the real case, let me know and I'll change it.