add view pages for admin resources#2390
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds ChangesEgg View Page Feature
Node View Page Feature
Server View Page Feature
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/Filament/Admin/ViewEggTest.php`:
- Around line 109-110: The test sets a global Filament panel with
Filament::setCurrentPanel(Filament::getPanel('admin')) but doesn't restore the
prior panel, causing test leakage; capture the current panel before changing
(e.g., $previous = Filament::getCurrentPanel() or similar), then ensure you
restore it after the test by calling Filament::setCurrentPanel($previous) (use a
finally block inside the test or restore in the test class tearDown method) so
the Filament::getCurrentPanel()/Filament::setCurrentPanel() state is always
returned to its original value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2527f209-f9da-4210-87e7-417c87b5ca3f
📒 Files selected for processing (6)
app/Filament/Admin/Resources/Eggs/EggResource.phpapp/Filament/Admin/Resources/Eggs/Pages/EditEgg.phpapp/Filament/Admin/Resources/Eggs/Pages/ListEggs.phpapp/Filament/Admin/Resources/Eggs/Pages/ViewEgg.phpresources/views/filament/components/monaco-editor.blade.phptests/Filament/Admin/ViewEggTest.php
|
like pelijoan said some of the decisions on what gets gated off on the read only pages will be up to interpretation, i'll be sure to mention those decisions & we can discuss them! |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/Traits/Filament/NodeDetailTabs.php`:
- Around line 606-619: The success notification is sent unconditionally after
calling NodeUpdateService::handle, so when handle($node, [], true) throws the
catch block shows a warning but execution continues and still calls
Notification::make()->success()->title(...)->send() and $this->fillForm(); fix
by returning early from the catch (or move the success Notification into the
try) so that on Exception you do not send the success notification or call
fillForm; update the catch in NodeDetailTabs where NodeUpdateService::handle is
invoked to exit the method (or otherwise skip the success
Notification::make()->success() and $this->fillForm()).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f4edbae0-311d-435e-9318-d596b065e4a9
📒 Files selected for processing (8)
app/Filament/Admin/Resources/Nodes/NodeResource.phpapp/Filament/Admin/Resources/Nodes/Pages/EditNode.phpapp/Filament/Admin/Resources/Nodes/Pages/ListNodes.phpapp/Filament/Admin/Resources/Nodes/Pages/ViewNode.phpapp/Filament/Admin/Resources/Nodes/RelationManagers/AllocationsRelationManager.phpapp/Filament/Admin/Resources/Nodes/RelationManagers/ServersRelationManager.phpapp/Traits/Filament/NodeDetailTabs.phptests/Filament/Admin/ViewNodeTest.php
|
right now the config tab is hidden on the read only page. its preview renders the wings config yaml, and that's got the daemon token baked into it, so a view-only user reading it basically walks off with the credential that authenticates wings. you could redact just the token and still show the rest read-only but i went with hiding the whole tab since it's safe by default, no render path for the secret at all. redaction works too, it's just an ongoing contract. if anyone thinks the config preview should be accessible by readonly i can make the change |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/Filament/Admin/Resources/Servers/RelationManagers/AllocationsRelationManager.php (1)
64-71: ⚡ Quick winIcon column remains interactive in read-only mode with misleading tooltip.
The tooltip on line 64 still shows "make_primary" text when
isReadOnly()is true, but clicking the icon does nothing (early return on line 67). This creates a confusing UX where users see an actionable tooltip but the click is silently ignored. The row action on line 84 correctly uses->hidden()to avoid this problem.Consider disabling the icon action or adjusting the tooltip in read-only mode:
💡 Suggested improvement
IconColumn::make('primary') ->icon(fn ($state) => match ($state) { true => TablerIcon::StarFilled, default => TablerIcon::Star, }) ->color(fn ($state) => match ($state) { true => 'warning', default => 'gray', }) - ->tooltip(fn (Allocation $allocation) => trans('admin/server.' . ($allocation->id === $this->getOwnerRecord()->allocation_id ? 'already' : 'make') . '_primary')) + ->tooltip(fn (Allocation $allocation) => $this->isReadOnly() + ? trans('admin/server.primary') + : trans('admin/server.' . ($allocation->id === $this->getOwnerRecord()->allocation_id ? 'already' : 'make') . '_primary')) ->action(function (Allocation $allocation) { if ($this->isReadOnly()) { return; } $this->getOwnerRecord()->update(['allocation_id' => $allocation->id]) && $this->deselectAllTableRecords(); })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Filament/Admin/Resources/Servers/RelationManagers/AllocationsRelationManager.php` around lines 64 - 71, The icon's tooltip and action are inconsistent in read-only mode: update the relation manager's column definition so the interactive icon is disabled when isReadOnly() is true—either by adding ->hidden(fn()=> $this->isReadOnly()) to the action/column or by changing the tooltip callback to return a non-actionable label when $this->isReadOnly() (use getOwnerRecord(), allocation_id, and Allocation in the same closure), and ensure the action callback (which uses $this->getOwnerRecord()->update(...) and $this->deselectAllTableRecords()) cannot be invoked in read-only mode; pick one approach and apply it so the icon is non-interactive and tooltip reflects read-only state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/Traits/Filament/ServerDetailTabs.php`:
- Around line 839-854: The call to TransferServerService::handle(...) can return
false, but the code proceeds to delete unselected daemon backups and show a
success notification regardless; change the action closure in ServerDetailTabs
so you capture the result (e.g., $result = $transfer->handle(...)) and only run
the subsequent backup-deletion loop and emit the success Notification when
$result is truthy; if $result === false, short-circuit (or emit an error/failure
Notification) and do not delete backups or show success.
---
Nitpick comments:
In
`@app/Filament/Admin/Resources/Servers/RelationManagers/AllocationsRelationManager.php`:
- Around line 64-71: The icon's tooltip and action are inconsistent in read-only
mode: update the relation manager's column definition so the interactive icon is
disabled when isReadOnly() is true—either by adding ->hidden(fn()=>
$this->isReadOnly()) to the action/column or by changing the tooltip callback to
return a non-actionable label when $this->isReadOnly() (use getOwnerRecord(),
allocation_id, and Allocation in the same closure), and ensure the action
callback (which uses $this->getOwnerRecord()->update(...) and
$this->deselectAllTableRecords()) cannot be invoked in read-only mode; pick one
approach and apply it so the icon is non-interactive and tooltip reflects
read-only state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 520b0dbf-fbf0-4253-9718-6757fa7ec9e2
📒 Files selected for processing (7)
app/Filament/Admin/Resources/Servers/Pages/EditServer.phpapp/Filament/Admin/Resources/Servers/Pages/ListServers.phpapp/Filament/Admin/Resources/Servers/Pages/ViewServer.phpapp/Filament/Admin/Resources/Servers/RelationManagers/AllocationsRelationManager.phpapp/Filament/Admin/Resources/Servers/RelationManagers/DatabasesRelationManager.phpapp/Filament/Admin/Resources/Servers/ServerResource.phpapp/Traits/Filament/ServerDetailTabs.php
|
i was trying different approaches to handling the schemas for node & server, as they're a little more complex with the lifecycle hooks and i didn't want to be too invasive. however i'll probably just end up consolidating them onto the resource class, like egg and the other read pages just with some changes to how the lifecycle hooks on the edit page hand off to and rehydrate the shared resource schema. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/Filament/Admin/ViewServerTest.php (1)
29-38: ⚡ Quick winRole names may collide across tests causing flaky failures.
The
serverRole()helper creates roles with fixed names like'Server Viewer'or'Server Editor'. If tests run in parallel or the database isn't reset between tests,Role::factory()->create(['name' => $name])may fail on unique constraint violations since the same role names are reused across multiple tests.Consider using unique role names per test or using
firstOrCreate:Suggested fix
function serverRole(string $name, array $abilities): Role { - $role = Role::factory()->create(['name' => $name, 'guard_name' => 'web']); + $role = Role::firstOrCreate( + ['name' => $name, 'guard_name' => 'web'], + Role::factory()->make(['name' => $name, 'guard_name' => 'web'])->toArray() + ); foreach ($abilities as $ability) { $role->givePermissionTo(Permission::findOrCreate($ability, 'web')); } return $role; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Filament/Admin/ViewServerTest.php` around lines 29 - 38, The serverRole() helper function creates roles with fixed names that can cause unique constraint violations when tests run in parallel or the database isn't properly reset between test runs. Replace the Role::factory()->create() call with Role::firstOrCreate() to handle the case where a role with that name already exists, preventing duplicate key errors while ensuring the correct role permissions are assigned regardless of test execution order.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/Filament/Admin/Resources/Nodes/NodeResource.php`:
- Around line 676-692: The success notification in the action closure is
positioned outside the try-catch block, so it executes regardless of whether the
token reset succeeded or failed. When an exception occurs during the
$service->handle() call, users see both the warning notification and the success
notification, which is contradictory. Move the success notification that
displays trans('admin/node.token_reset') from line 689 into the try block
immediately after the $service->handle() call completes, ensuring it only fires
when the operation succeeds. Consider whether the $livewire->refreshFormData()
call should also be conditional on success.
In `@app/Filament/Admin/Resources/Servers/ServerResource.php`:
- Around line 780-783: Form fields in the ServerResource remain editable on view
pages because they lack operation-based disabled logic. Add `->disabled(fn
(string $operation) => $operation === 'view')` to disable fields when the
operation is 'view'. At
`app/Filament/Admin/Resources/Servers/ServerResource.php#L780-L783`, add this
disabled condition to the `StartupVariable` component. At the same file
`app/Filament/Admin/Resources/Servers/ServerResource.php#L786-L791`, add the
same disabled condition to the `CheckboxList` returned by
`getMountCheckboxList()`. This ensures both components become read-only on view
pages while remaining editable on create and edit pages.
- Around line 495-501: The formatStateUsing closure uses a match(true) statement
that does not account for null values returned by $get('swap'), which can occur
during initial form hydration before the record is loaded, causing the default
case to throw a LogicException. Add a null check to handle this case by adding a
condition before the other comparisons that returns a safe default value when
swap is null, or restructure the match statement to include a null-safe check
that prevents the exception from being thrown.
---
Nitpick comments:
In `@tests/Filament/Admin/ViewServerTest.php`:
- Around line 29-38: The serverRole() helper function creates roles with fixed
names that can cause unique constraint violations when tests run in parallel or
the database isn't properly reset between test runs. Replace the
Role::factory()->create() call with Role::firstOrCreate() to handle the case
where a role with that name already exists, preventing duplicate key errors
while ensuring the correct role permissions are assigned regardless of test
execution order.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0481f7d3-44a8-4fa1-a340-62eb07a8331a
📒 Files selected for processing (9)
app/Filament/Admin/Resources/Nodes/NodeResource.phpapp/Filament/Admin/Resources/Nodes/Pages/EditNode.phpapp/Filament/Admin/Resources/Nodes/Pages/ViewNode.phpapp/Filament/Admin/Resources/Servers/Pages/EditServer.phpapp/Filament/Admin/Resources/Servers/Pages/ViewServer.phpapp/Filament/Admin/Resources/Servers/ServerResource.phptests/Filament/Admin/ViewEggTest.phptests/Filament/Admin/ViewNodeTest.phptests/Filament/Admin/ViewServerTest.php
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/Filament/Admin/ViewEggTest.php
- tests/Filament/Admin/ViewNodeTest.php
|
ended up going with the consolidation. node and server sit on the resource as static detailtabs like egg now, traits gone. the lifecycle hooks i was wary about were fine in the end. i did encounter some other unrelated bugs with some of the editpages, but because the changes on this one are heavy enough i'll pr those seperately |
|
also i hid the diagnosticts tab as it's basically just the pull/upload/clear actions that hit wings, nothing on it is actually data you'd sit and read. so it has no real read only state to render, felt cleaner to drop it on view than leave a tab of dead buttons. any changes let me know, ready for review |
1 participant
closes #583. creating the read crud on egg, node & server views. #963 fixed the other resources & this picks up on @JoanFo1456 work
this continues the intent of #1998 which had drifted and wasn't really cherry-pickable after the v4 to v5 form/schema bump. used its class structures as reference only.
egg is pretty much done. node and server are what's left, each needs its own pass since they're more involved than egg. it's good for review once all three are in. any feedback on the design would be appreciated, i'll try to imitate the approaches the existing read only pages use where possible.