Add traits for customizing tabs #2101
Conversation
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (1)
You can disable this status message by setting the 📝 WalkthroughWalkthroughAdds a new Changes
Sequence Diagram(s)(omitted) 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 6
🤖 Fix all issues with AI agents
In @app/Filament/Admin/Resources/Eggs/Pages/CreateEgg.php:
- Around line 206-209: The unique validation for nested variables fails because
$get('../../id') is null during form validation; implement Filament's pattern by
overriding handleRecordCreation to first create the Egg record and then persist
relationship data via $this->form->model($egg)->saveRelationships(); in
handleRecordCreation ensure uuid and config_startup/config_logs are normalized
(json-encoded if arrays), call Egg::create($data) to create $egg, then call
$this->form->model($egg)->saveRelationships() and return $egg so the unique rule
with where('egg_id', $get('../../id')) can resolve the egg id.
In @app/Filament/Admin/Resources/Nodes/Pages/EditNode.php:
- Around line 461-476: The ToggleButtons field's two afterStateUpdated callbacks
only allow the last one to run, so combine them into a single afterStateUpdated
callback on the ToggleButtons instance that accepts a Set parameter and calls
$set('disk', 0) and $set('disk_overallocate', 0) within that one callback
(preserving the existing state handling such as formatStateUsing and stateCast).
- Around line 619-634: The success notification is sent regardless of errors
because it lives outside the try-catch; move the
Notification::make()->success()->title(trans('admin/node.token_reset'))->send()
(and the subsequent $this->fillForm() if you only want to refresh on success)
into the try block immediately after $this->nodeUpdateService->handle($node, [],
true) so that it runs only when handle() succeeds; keep the catch block as-is to
send the warning notification when an Exception is thrown.
- Around line 517-533: The ToggleButtons::make('unlimited_cpu') currently has
two afterStateUpdated handlers but only the last one runs; replace them with a
single afterStateUpdated callback that accepts Set $set and sets both 'cpu' and
'cpu_overallocate' to 0 (e.g., ->afterStateUpdated(fn (Set $set) => $set('cpu',
0)->set('cpu_overallocate', 0))). Keep the other chain methods
(formatStateUsing, stateCast, options, colors, columnSpan) unchanged.
- Around line 400-421: The ToggleButtons::make('unlimited_mem') component
currently has two afterStateUpdated(...) calls but only the last one runs;
combine them into a single afterStateUpdated callback on the ToggleButtons
component that calls $set('memory', 0) and $set('memory_overallocate', 0) (e.g.,
->afterStateUpdated(fn (Set $set) => [$set('memory', 0),
$set('memory_overallocate', 0)]) or a single closure that invokes both $set
calls) so both fields are cleared when unlimited_mem is toggled.
- Around line 143-153: The helperText callback on the domain field currently
returns trans('admin/node.error') for valid FQDNs which is a misleading key
name; update the key to a clearer name (e.g. trans('admin/node.domain_help') or
trans('admin/node.fqdn_explanation')) in the ->helperText(function ($state) {
... }) block and add/rename the corresponding entry in your translation files so
the message text remains the same but the key reflects guidance rather than an
error.
🧹 Nitpick comments (3)
app/Traits/Filament/CanCustomizeStaticTabs.php (1)
8-32: Consider consolidating shared logic between the two tab traits.This trait is nearly identical to
CanCustomizeTabs, differing only in using static methods forgetDefaultTabs()andgetTabs(). While this duplication is acceptable given Filament's distinct patterns for Resources (static) vs Pages (instance), you might consider extracting the sharedregisterCustomTabslogic or documenting when to use each trait.app/Filament/Admin/Resources/Users/UserResource.php (1)
304-304: Consider usingarray_keys()to avoid the unused variable warning.The
$_variable is flagged by static analysis as unused. Usingarray_keys()is more explicit.Suggested fix
- foreach ($user->oauth ?? [] as $schema => $_) { + foreach (array_keys($user->oauth ?? []) as $schema) {app/Filament/Admin/Resources/Eggs/Pages/CreateEgg.php (1)
180-197: Setting checkbox fields to empty string may cause type issues.Lines 184-185 and 193-194 set
user_viewableanduser_editableto empty strings when null. These fields backCheckboxcomponents (lines 229-230), which typically expect boolean values. Consider usingfalseinstead of''for consistency with checkbox semantics.Suggested fix
->mutateRelationshipDataBeforeCreateUsing(function (array $data): array { $data['default_value'] ??= ''; $data['description'] ??= ''; $data['rules'] ??= []; - $data['user_viewable'] ??= ''; - $data['user_editable'] ??= ''; + $data['user_viewable'] ??= false; + $data['user_editable'] ??= false; return $data; }) ->mutateRelationshipDataBeforeSaveUsing(function (array $data): array { $data['default_value'] ??= ''; $data['description'] ??= ''; $data['rules'] ??= []; - $data['user_viewable'] ??= ''; - $data['user_editable'] ??= ''; + $data['user_viewable'] ??= false; + $data['user_editable'] ??= false; return $data; })
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
app/Enums/TabPosition.phpapp/Filament/Admin/Pages/Settings.phpapp/Filament/Admin/Resources/Eggs/Pages/CreateEgg.phpapp/Filament/Admin/Resources/Eggs/Pages/EditEgg.phpapp/Filament/Admin/Resources/Nodes/Pages/EditNode.phpapp/Filament/Admin/Resources/Servers/Pages/CreateServer.phpapp/Filament/Admin/Resources/Servers/Pages/EditServer.phpapp/Filament/Admin/Resources/Users/UserResource.phpapp/Filament/Pages/Auth/EditProfile.phpapp/Traits/Filament/CanCustomizeStaticTabs.phpapp/Traits/Filament/CanCustomizeTabs.php
💤 Files with no reviewable changes (1)
- app/Filament/Admin/Resources/Servers/Pages/CreateServer.php
🧰 Additional context used
🧠 Learnings (1)
📚 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/Pages/EditNode.php
🧬 Code graph analysis (5)
app/Traits/Filament/CanCustomizeStaticTabs.php (4)
app/Traits/Filament/CanCustomizeTabs.php (3)
registerCustomTabs(13-16)getDefaultTabs(19-22)getTabs(25-32)app/Filament/Admin/Pages/Settings.php (1)
getDefaultTabs(118-148)app/Filament/Admin/Resources/Eggs/Pages/CreateEgg.php (1)
getDefaultTabs(73-286)app/Filament/Admin/Resources/Eggs/Pages/EditEgg.php (1)
getDefaultTabs(67-443)
app/Traits/Filament/CanCustomizeTabs.php (2)
app/Traits/Filament/CanCustomizeStaticTabs.php (3)
registerCustomTabs(13-16)getDefaultTabs(19-22)getTabs(25-32)app/Filament/Admin/Resources/Users/UserResource.php (1)
getDefaultTabs(157-484)
app/Filament/Admin/Resources/Users/UserResource.php (2)
app/Traits/Filament/CanCustomizeStaticTabs.php (2)
getTabs(25-32)getDefaultTabs(19-22)app/Traits/Filament/CanCustomizeTabs.php (2)
getTabs(25-32)getDefaultTabs(19-22)
app/Filament/Admin/Resources/Eggs/Pages/EditEgg.php (2)
app/Traits/Filament/CanCustomizeStaticTabs.php (2)
getTabs(25-32)getDefaultTabs(19-22)app/Traits/Filament/CanCustomizeTabs.php (2)
getTabs(25-32)getDefaultTabs(19-22)
app/Filament/Admin/Resources/Nodes/Pages/EditNode.php (2)
app/Traits/Filament/CanCustomizeStaticTabs.php (2)
getTabs(25-32)getDefaultTabs(19-22)app/Traits/Filament/CanCustomizeTabs.php (2)
getTabs(25-32)getDefaultTabs(19-22)
🪛 PHPMD (2.15.0)
app/Filament/Admin/Resources/Users/UserResource.php
304-304: Avoid unused local variables such as '$_'. (undefined)
(UnusedLocalVariable)
🔇 Additional comments (18)
app/Enums/TabPosition.php (1)
5-9: LGTM!Clean and minimal enum definition. The string-backed enum is appropriate for use as array keys in the tab customization system.
app/Traits/Filament/CanCustomizeTabs.php (1)
8-32: LGTM!The trait design correctly uses late static binding (
static::) for the$customTabsproperty, ensuring each class using the trait maintains its own custom tabs registry. The mix of static registration (allowing customization before instantiation) and instance retrieval methods is a clean API design.app/Filament/Admin/Resources/Users/UserResource.php (3)
21-21: LGTM!Correct import and usage of
CanCustomizeStaticTabstrait for the Resource class, which uses static method patterns.Also applies to: 63-63
150-154: LGTM!The form schema correctly delegates tab construction to
static::getTabs(), enabling the tab customization system.
156-483: LGTM!Comprehensive default tabs implementation covering account, roles, keys, and activity tabs. The structure aligns well with the new tab customization pattern.
app/Filament/Admin/Resources/Eggs/Pages/EditEgg.php (3)
15-15: LGTM!Correct import and usage of
CanCustomizeTabstrait for the EditRecord page class.Also applies to: 51-51
55-64: LGTM!The form method correctly uses the instance method
$this->getTabs()for tab assembly, enabling the tab customization system.
66-442: LGTM!Comprehensive default tabs implementation with well-structured configuration, process management, egg variables, and install script tabs.
app/Filament/Admin/Pages/Settings.php (3)
13-13: LGTM!Correct import and usage of
CanCustomizeTabstrait for the Settings page.Also applies to: 57-57
101-111: LGTM!Clean delegation to
$this->getTabs()within the form schema.
113-148: LGTM!Well-organized default tabs definition delegating to focused helper methods for each settings section.
app/Filament/Pages/Auth/EditProfile.php (3)
17-17: LGTM!Correct import and usage of
CanCustomizeTabstrait for the EditProfile page.Also applies to: 60-60
85-97: LGTM!The form method correctly uses
$this->getTabs()and properly chains the additional schema configuration.
99-557: LGTM!Comprehensive default tabs implementation covering all profile-related functionality including account settings, OAuth, 2FA, API keys, SSH keys, activity, and customization options.
app/Filament/Admin/Resources/Eggs/Pages/CreateEgg.php (2)
61-70: LGTM on the tab customization integration.The form correctly delegates to
getTabs()from theCanCustomizeTabstrait, enabling the tab customization framework. ThepersistTabInQueryString()andcolumnSpanFull()configurations are appropriately preserved.
72-286: Well-structured tab organization.The
getDefaultTabs()method is cleanly organized with logical groupings: configuration, process management, variables, and install script. Translation keys are consistently used for labels, and responsive column spans are properly configured.app/Filament/Admin/Resources/Nodes/Pages/EditNode.php (2)
65-79: LGTM on the tab customization integration.The form correctly integrates with the
CanCustomizeTabstrait viagetTabs(), maintaining the existing column configuration and tab persistence behavior.
81-780: Comprehensive tab organization with clear separation of concerns.The
getDefaultTabs()method provides a well-structured organization of node management features across overview, settings, configuration, and diagnostics tabs. The trait integration enables future extensibility for custom tab additions. Based on learnings, the intentional UI difference between the diagnostics upload action (withouticonButton()) and pull action (withiconButton()) is preserved.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @app/Filament/Admin/Resources/Nodes/Pages/EditNode.php:
- Around line 707-708: Add a defensive check after calling $response->json() to
ensure the 'url' key exists before accessing $data['url']; in the EditNode page
(EditNode class / the method containing the $response->json() call) validate
with isset($data['url']) or array_key_exists('url', $data), and if missing
handle it gracefully (log the unexpected response, set a safe default or
return/throw a descriptive exception and avoid using a null/undefined value) so
the code doesn't trigger undefined index warnings or propagate invalid data.
🧹 Nitpick comments (1)
app/Filament/Admin/Resources/Nodes/Pages/EditNode.php (1)
143-153: The translation keyadmin/node.erroris misleadingly named but contains helpful text.The helper text returned when a domain (non-IP) is entered uses the translation key
admin/node.error, which actually contains: "This is the domain name that points to your node's IP Address. If you've already set up this, you can verify it by checking the next field!" This is instructional text, not an error message.Consider renaming the translation key to something more descriptive like
fqdn_noteordomain_helpto better reflect its purpose and avoid confusing developers about what this field expects.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/Filament/Admin/Resources/Nodes/Pages/EditNode.php
🧰 Additional context used
🧠 Learnings (1)
📚 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/Pages/EditNode.php
🧬 Code graph analysis (1)
app/Filament/Admin/Resources/Nodes/Pages/EditNode.php (1)
app/helpers.php (2)
is_ip(15-18)get_ip_from_hostname(98-111)
🔇 Additional comments (2)
app/Filament/Admin/Resources/Nodes/Pages/EditNode.php (2)
65-83: LGTM! Trait integration is correct.The refactoring properly:
- Uses the
CanCustomizeTabstrait to enable tab customization- Delegates tab assembly to
$this->getTabs()which merges custom tabs with defaults- Implements
getDefaultTabs()with the correct visibility (protected) and return type
686-689: Verify intentional UI change:iconButton()now added to upload action.Based on learnings from PR #1865, the diagnostics tab's upload action intentionally did not use the
iconButton()modifier (while the pull action did). This code now addsiconButton()to the upload action at line 689.Please confirm this is an intentional UI change, or if the previous distinction should be preserved.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/Filament/Admin/Resources/Eggs/Pages/EditEgg.php`:
- Around line 126-133: The hostname is validated via parse_url/gethostbyname
($host/$ip) but the actual fetch still calls file_get_contents($imageUrl),
enabling DNS rebinding; update the download logic (where file_get_contents is
used) to use the already-resolved $ip: perform the HTTP request to
http://$ip/... while setting the original Host header to $host and disable
redirects, or replace file_get_contents with a cURL implementation using
CURLOPT_RESOLVE (map $host to $ip), CURLOPT_FOLLOWLOCATION = 0, and explicit
Host header handling; ensure you reuse the validated $ip/$host values from the
earlier parse_url/gethostbyname checks and block redirects to prevent secondary
rebinding.
# Conflicts: # app/Filament/Admin/Resources/Servers/Pages/EditServer.php
No description provided.