Make allocation select on users server relation manager functional #1719
Conversation
📝 WalkthroughWalkthroughRemoved explicit return types from several TextColumn URL closures. Reworked primary-allocation UI in the Users relation manager: renamed allocation column, split editable vs read-only columns with permission and allocations-count based visibility/disabled logic, updated options/placeholder behavior, and removed an unused image column. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Admin as Admin User
participant UI as Users → Servers RelationManager
participant Auth as Authorization
participant S as Server Model
Admin->>UI: Open User edit → Servers tab
UI->>Auth: check permission can('update server')
alt permission granted
UI->>S: load server.allocations
note right of UI: Render SelectColumn `allocation_id`<br/>Options = all allocations<br/>Disabled if allocations ≤ 1
else permission denied
UI->>S: load server.allocation (primary)
note right of UI: Render TextColumn `allocation_id_readonly`<br/>State = address or 'None'<br/>Disabled if allocations ≤ 1
end
Pre-merge checks❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-08-12T17:33:57.388ZApplied to files:
🧬 Code graph analysis (1)app/Filament/Admin/Resources/Users/RelationManagers/ServersRelationManager.php (1)
🔇 Additional comments (4)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 1
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/ListServers.php (2)
79-82: Remove unsupported TextColumn::disabled() and localize 'None'TextColumn in Filament v3 has no disabled() method — remove that call (use the existing hidden() toggle) and replace the literal 'None' with a translation.
- ->disabled(fn (Server $server) => $server->allocations->count() <= 1) - ->state(fn (Server $server) => $server->allocation->address ?? 'None'), + ->state(fn (Server $server) => $server->allocation->address ?? trans('admin/server.none')),
71-75: Eager‑load allocations on the ListServers table to avoid an N+1.Both the SelectColumn and TextColumn call $server->allocations per row — add a query modifier in table() to load allocations once, e.g.:
->modifyQueryUsing(fn (Illuminate\Database\Eloquent\Builder $query) => $query->with('allocations'));
Location: app/Filament/Admin/Resources/Servers/Pages/ListServers.php (inside table() return chain).
🧹 Nitpick comments (1)
app/Filament/Admin/Resources/Servers/Pages/ListServers.php (1)
54-54: URL closures without return types — OK; keep style consistent.Change is fine. Consider making nearby closures consistent (either all typed or all untyped) for static analysis/readability.
Also applies to: 60-60, 66-66
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/Filament/Admin/Resources/Servers/Pages/ListServers.php(1 hunks)app/Filament/Admin/Resources/Users/RelationManagers/ServersRelationManager.php(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: lancepioch
PR: pelican-dev/panel#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.
🧬 Code graph analysis (2)
app/Filament/Admin/Resources/Servers/Pages/ListServers.php (3)
app/Filament/Admin/Resources/Users/RelationManagers/ServersRelationManager.php (1)
table(20-92)app/Filament/Admin/Resources/Eggs/RelationManagers/ServersRelationManager.php (1)
table(15-43)app/Filament/Admin/Resources/Nodes/RelationManagers/NodesRelationManager.php (1)
table(22-61)
app/Filament/Admin/Resources/Users/RelationManagers/ServersRelationManager.php (1)
app/Models/Server.php (2)
Server(132-504)allocation(277-280)
🔇 Additional comments (3)
app/Filament/Admin/Resources/Users/RelationManagers/ServersRelationManager.php (3)
57-57: URL closures without return types — OK.No behavior change; aligns with other spots in the PR.
Also applies to: 62-62, 66-66
68-75: Use per-record policy check for column visibilityReplace the global ability check with a per-record policy when the hidden() callback can receive the record: change
->hidden(fn () => !auth()->user()->can('update server'))
to
->hidden(fn (Server $server) => !auth()->user()->can('update', $server))
(or Gate::allows('update', $server)). If your Filament version doesn't pass the record into hidden(), keep the current code and retain the TODO.File: app/Filament/Admin/Resources/Users/RelationManagers/ServersRelationManager.php — lines 68–75
76-80: Keep ->disabled(); localize fallback and fix hidden() policy check
- TextColumn::disabled(...) is supported in Filament Tables v3 — do not remove the ->disabled(...) call.
- Localize the fallback: change ->state(fn (Server $server) => $server->allocation->address ?? 'None') to ->state(fn (Server $server) => $server->allocation->address ?? trans('admin/server.none')).
- Fix the hidden() permission check: current ->hidden(fn () => auth()->user()->can('update server')) is wrong (no Server passed and wrong ability usage). Use a policy check that receives the record and hide when the user cannot update it, e.g. ->hidden(fn (?Server $server) => !auth()->user()->can('update', $server)).
File: app/Filament/Admin/Resources/Users/RelationManagers/ServersRelationManager.php (lines 76-80)
Likely an incorrect or invalid review comment.
app/Filament/Admin/Resources/Users/RelationManagers/ServersRelationManager.php
Show resolved
Hide resolved
Closes #1717