Skip to content

Add allocation port visibility option - #2511

Open
ByCoquito wants to merge 2 commits into
pelican:mainfrom
ByCoquito:feature/allocation-port-visibility
Open

Add allocation port visibility option#2511
ByCoquito wants to merge 2 commits into
pelican:mainfrom
ByCoquito:feature/allocation-port-visibility

Conversation

@ByCoquito

@ByCoquito ByCoquito commented Aug 12, 2026

Copy link
Copy Markdown

Adds a Show port option to server allocations. It is enabled by default.

When disabled, users will only see the allocation IP or alias without the port. The port is still used internally, so server functionality is not affected.

I have read the CLA Document and I hereby sign the CLA

@github-actions

github-actions Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@coderabbitai

coderabbitai Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 0cb9105a-fc72-4f5e-8ee3-260c79c55327

📥 Commits

Reviewing files that changed from the base of the PR and between a7489f3 and 54be473.

📒 Files selected for processing (4)
  • app/Http/Controllers/Api/Client/Servers/NetworkAllocationController.php
  • app/Models/Allocation.php
  • tests/Integration/Api/Client/Server/Allocation/DeleteAllocationTest.php
  • tests/Unit/Models/AllocationTest.php
🚧 Files skipped from review as they are similar to previous changes (3)
  • app/Http/Controllers/Api/Client/Servers/NetworkAllocationController.php
  • tests/Integration/Api/Client/Server/Allocation/DeleteAllocationTest.php
  • app/Models/Allocation.php

📝 Walkthrough

Walkthrough

The PR adds allocation port visibility state, exposes it through APIs, resets it during allocation removal, and replaces raw address displays with display_address across server views and activity logs.

Changes

Allocation port visibility

Layer / File(s) Summary
Allocation model and API contract
database/migrations/..., app/Models/Allocation.php, app/Transformers/Api/..., tests/Unit/Models/AllocationTest.php
Allocations now store show_port, default it to true, validate and cast it as a boolean, and expose display_address. API transformers include the visibility state. Unit tests cover address formatting and defaults.
Visibility controls and reset flows
app/Filament/Admin/..., app/Filament/Server/..., app/Http/Controllers/Api/..., tests/Integration/Api/...
Filament tables expose a show_port toggle and conditionally display ports. Allocation removal resets the value to true. Integration tests verify reset behavior and API output.
Display address propagation
app/Filament/App/..., app/Filament/Server/Widgets/..., resources/views/livewire/..., app/Http/Controllers/Api/..., lang/en/admin/server.php
Server displays and allocation activity logs now use display_address. The administration translation adds the “Show port” label.

Sequence Diagram(s)

sequenceDiagram
  participant Admin
  participant AllocationResource
  participant NetworkAllocationController
  participant Allocation
  participant API
  Admin->>AllocationResource: Set show_port
  AllocationResource->>Allocation: Persist visibility preference
  API->>NetworkAllocationController: Request allocation data
  NetworkAllocationController->>Allocation: Read show_port and display_address
  NetworkAllocationController-->>API: Return allocation attributes
Loading

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: adding an allocation port visibility option.
Description check ✅ Passed The description accurately explains the Show port option, its default behavior, and its effect on displayed allocation addresses.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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/App/Resources/Servers/Pages/ListServers.php`:
- Around line 86-91: Update Allocation::display_address so the show_port=false
branch returns the optional alias when present and otherwise falls back to the
allocation IP without its port. Add a regression test covering an allocation
without an alias, and verify the shared accessor behavior used by the server
list, ServerOverview, and Livewire server entries.

In `@app/Http/Controllers/Api/Client/Servers/NetworkAllocationController.php`:
- Around line 140-148: Update the detachment update in the allocation deletion
flow to set is_locked to false alongside notes, show_port, and server_id. Extend
DeleteAllocationTest.php to assert the allocation is unlocked after deletion,
including the endpoint path that handles locked allocations.
🪄 Autofix

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 Plus

Run ID: 8b127912-c85e-4c67-a665-a9bf144959e1

📥 Commits

Reviewing files that changed from the base of the PR and between 3bd26df and a7489f3.

📒 Files selected for processing (15)
  • app/Filament/Admin/Resources/Servers/RelationManagers/AllocationsRelationManager.php
  • app/Filament/App/Resources/Servers/Pages/ListServers.php
  • app/Filament/Server/Resources/Allocations/AllocationResource.php
  • app/Filament/Server/Widgets/ServerOverview.php
  • app/Http/Controllers/Api/Client/Servers/NetworkAllocationController.php
  • app/Models/Allocation.php
  • app/Transformers/Api/Application/AllocationTransformer.php
  • app/Transformers/Api/Client/AllocationTransformer.php
  • database/migrations/2026_08_10_144038_add_show_port_to_allocations_table.php
  • lang/en/admin/server.php
  • resources/views/livewire/server-entry-placeholder.blade.php
  • resources/views/livewire/server-entry.blade.php
  • tests/Integration/Api/Client/Server/Allocation/DeleteAllocationTest.php
  • tests/Integration/Api/Client/Server/NetworkAllocationControllerTest.php
  • tests/Unit/Models/AllocationTest.php

Comment on lines +86 to +91
TextColumn::make('allocation.display_address')
->label('')
->badge()
->visibleFrom('md')
->copyable()
->state(fn (Server $server) => $server->allocation->address ?? 'None'),
->state(fn (Server $server) => $server->allocation->display_address ?? 'None'),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Preserve the IP when no alias is set.

When show_port is false, Allocation::display_address returns alias. The alias is optional, so an allocation without an alias produces null. This column then displays None instead of the allocation IP without its port. The same accessor affects ServerOverview and both Livewire server entries.

Update Allocation::display_address to use the alias or the IP for the port-hidden case, and add a regression test.

Proposed fix
- get: fn () => $this->show_port ? $this->address : $this->alias,
+ get: fn () => $this->show_port ? $this->address : ($this->alias ?? $this->ip),
🤖 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/App/Resources/Servers/Pages/ListServers.php` around lines 86 -
91, Update Allocation::display_address so the show_port=false branch returns the
optional alias when present and otherwise falls back to the allocation IP
without its port. Add a regression test covering an allocation without an alias,
and verify the shared accessor behavior used by the server list, ServerOverview,
and Livewire server entries.

@ByCoquito

Copy link
Copy Markdown
Author

I have read the CLA Document and I hereby sign the CLA

@ByCoquito
ByCoquito force-pushed the feature/allocation-port-visibility branch from 54be473 to 3432d9d Compare August 12, 2026 16:19
@lancepioch

Copy link
Copy Markdown
Member

I like this idea and have thought about it for a couple years. I have NOT reviewed your implementation as of right now but am just letting everyone know that I want to push this PR through to get merged after code review and manual testing.

@lancepioch

Copy link
Copy Markdown
Member

Note

This comment was generated by AI (Claude Code), not written by Lance. It has not been hand-verified line by line by a human maintainer. Treat it as a starting point for discussion — if anything below is wrong or you disagree with it, say so and a human will weigh in. It is not a formal blocking review.

Thanks for the PR — the feature itself is a good idea. Hiding the port is a common ask for SRV-record and proxied setups (Velocity/BungeeCord and friends), and the scoping is sensible: display-only, default-on, admin views keep the real port, and the migration is clean and reversible so upgrades are a no-op.

Three change requests before this is mergeable.

1. Two allocation-release paths bypass the new updating hook

The PR establishes the invariant "an allocation with no server_id must have show_port = true" and enforces it in Allocation::booted():

static::updating(function (self $allocation) {
    if (is_null($allocation->server_id)) {
        $allocation->is_locked = false;
        $allocation->show_port = true;
    }
});

That hook only fires on model-level saves. Two of the main release paths use query-builder updates, which skip model events entirely:

  • app/Services/Servers/BuildModificationService.php:124$allocations->update(['notes' => null, 'server_id' => null])
  • app/Services/Servers/ServerDeletionService.php:104$server->allocations()->update(['server_id' => null, 'notes' => null])

Concrete failure: an admin hides the port on allocation X of server A, then removes X from A via Edit Server → Networking (or PATCH /api/application/servers/{id}/build). X returns to the free pool with show_port = 0. When server B later picks it up, FindAssignableAllocationService::handle() does $allocation->update(['server_id' => $server->id]) — and by the time the hook runs, $allocation->server_id is already the new non-null id, so the reset doesn't fire there either. Server B's owner now sees where the port should be, with no way to fix it themselves since the toggle is admin-only.

Note the comment sitting directly above the BuildModificationService call:

// Remove any of the allocations we got that are currently assigned to this server on
// this node. Also set the notes to null, otherwise when re-allocated to a new server those
// notes will be carried over.

That's exactly the bug, reintroduced for show_port. is_locked has the same gap.

Minimal fix is adding 'show_port' => true, 'is_locked' => false to both payloads. Cleaner would be a small shared helper on the model so the invariant lives in one place, since there's a third site with the same problem: ServerTransferController.php:52 and :84 (Allocation::query()->whereIn('id', $allocations)->update(['server_id' => null])) reset neither notes, is_locked, nor show_port on transfer success/failure.

2. Please revert the six activity-log call sites back to address

These six changed $allocation->address$allocation->display_address in activity log properties:

  • app/Http/Controllers/Api/Client/Servers/NetworkAllocationController.php:66, 91, 117, 149
  • app/Filament/Server/Resources/Allocations/AllocationResource.php:99, 122

Activity records are an audit trail, not a display surface. With show_port = false on several allocations sharing one hostname, every resulting server:allocation.* entry records the identical string (play.example.com), and an admin reading the log can no longer tell which allocation was edited, deleted, or made primary. That's a permanent loss of audit fidelity in exchange for a cosmetic win, and the log isn't shown to the user you're hiding the port from anyway.

display_address is right everywhere else you used it — the dashboard entries, ServerOverview, the App server list, the allocations table. Just not in the log.

3. Scope the bulk-dissociate update

app/Filament/Admin/Resources/Servers/RelationManagers/AllocationsRelationManager.php:122:

DissociateBulkAction::make()
    ->after(function () {
        Allocation::whereNull('server_id')->update([
            'notes' => null,
            'is_locked' => false,
            'show_port' => true,
        ]);

This is unscoped by node and by the selected records, so dissociating one allocation from one server wipes notes on every unassigned allocation across the entire panel. The unscoped query predates this PR, but since it's being touched here it's a good moment to target the dissociated ids — the single-record after() at line 107 already does it correctly.


Two smaller notes (non-blocking)

  • show_port isn't honored everywhere. app/Filament/Server/Pages/Startup.php exposes the preview-startup action, which substitutes {{SERVER_PORT}} — and most game eggs carry -p {{SERVER_PORT}} in the startup command, so the port is one click away on the same panel where it was just hidden. AllocationTransformer also still returns port to the client API. Both are arguably fine (third-party frontends genuinely need the port), but worth deciding deliberately, and any user-facing description should avoid implying the port is hidden from the user — it's a display preference, not concealment.
  • Two small cleanups: AllocationResource.php:59 uses a hardcoded ->placeholder('—') where every sibling placeholder in that file uses trans(...); and Allocation.php:137 re-implements the existing getAliasAttribute() accessor as $this->ip_alias ?? $this->ip — using $this->alias keeps both branches of display_address in sync if alias resolution ever changes.

Note

Again: AI-generated review, not a human maintainer's verdict. Please push back on anything that looks wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants