Allow backup transfers #2068
Allow backup transfers #2068
Conversation
📝 WalkthroughWalkthroughAdds backup selection to the server transfer flow: the transfer form shows daemon backups for selection, unselected daemon backups are deleted via Changes
Sequence Diagram(s)sequenceDiagram
participant AdminUI as Admin UI (Filament)
participant EditPage as EditServer Page Action
participant DeleteSvc as DeleteBackupService
participant TransferSvc as TransferServerService
participant Wings as Wings API
AdminUI->>EditPage: Submit transfer form (node_id, allocation_id, additional_allocations, backups[])
EditPage->>DeleteSvc: deleteBackup(backup) for each daemon backup not in backups[]
alt deletion failures
DeleteSvc-->>EditPage: deletion error per-backup
EditPage-->>AdminUI: notify deletion failures
end
EditPage->>TransferSvc: handle(server, node_id, allocation_id, additional_allocations, backups[])
TransferSvc->>Wings: POST /api/servers/{id}/transfer with token + payload (node, allocations, backups)
alt Wings accepts
Wings-->>TransferSvc: 200 OK
TransferSvc-->>AdminUI: success notification
else Wings rejects
Wings-->>TransferSvc: error
TransferSvc-->>AdminUI: error notification (backup_transfer_failed)
end
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 2
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/Filament/Admin/Resources/Servers/Pages/EditServer.phpapp/Services/Servers/TransferServerService.phplang/en/admin/server.php
🧰 Additional context used
🧬 Code graph analysis (2)
app/Filament/Admin/Resources/Servers/Pages/EditServer.php (2)
app/Models/Server.php (3)
transfer(386-389)Server(135-552)backups(394-397)app/Services/Servers/TransferServerService.php (1)
handle(54-109)
app/Services/Servers/TransferServerService.php (1)
app/Models/Server.php (2)
backups(394-397)Server(135-552)
🔇 Additional comments (3)
app/Filament/Admin/Resources/Servers/Pages/EditServer.php (2)
1056-1063: LGTM! Well-implemented backup selection field.The multi-select field for backups is properly configured:
- Disabled when no backups exist
- Maps backup IDs correctly for form submission
- Includes helpful warning text for users
- Uses appropriate icon and placeholder
966-966: Verify backup ID validation in service layer.The backup IDs are passed directly from form data to the service. While the form field is constrained to the server's backups, ensure the service layer also validates that the provided backup IDs actually belong to the server to prevent potential security issues.
This validation should happen in
TransferServerService::handle()ornotify()method (see my comments on that file).app/Services/Servers/TransferServerService.php (1)
32-32: No action needed. The configuration keyconfig('backups.default')is correct and'wings'is a valid backup driver value defined inconfig/backups.php. The code properly checks the backup driver configuration to include backups during the server transfer operation.
| private function notify(ServerTransfer $transfer, UnencryptedToken $token, array $backup_ids = []): void | ||
| { | ||
| $backups = []; | ||
| if (config('backups.default') === 'wings') { |
There was a problem hiding this comment.
| if (config('backups.default') === 'wings') { | |
| if (config('backups.default') === Backup::ADAPTER_DAEMON) { |
| ->helperText(trans('admin/server.warning_backups')) | ||
| ->prefixIcon('tabler-copy-check') | ||
| ->disabled(fn (Server $server) => $server->backups->count() === 0) | ||
| ->options(fn (Server $server) => $server->backups->mapWithKeys(fn ($backup) => [$backup->id => $backup->name ?: $backup->uuid])) |
There was a problem hiding this comment.
Why not use the uuid directly? This way you don't have to do lookups later.
| ->options(fn (Server $server) => $server->backups->mapWithKeys(fn ($backup) => [$backup->id => $backup->name ?: $backup->uuid])) | |
| ->options(fn (Server $server) => $server->backups->mapWithKeys(fn ($backup) => [$backup->uuid => $backup->name])) |
| public function handle(Server $server, int $node_id, ?int $allocation_id = null, ?array $additional_allocations = []): bool | ||
| public function handle(Server $server, int $node_id, ?int $allocation_id = null, ?array $additional_allocations = [], ?array $backup_ids = []): bool | ||
| { | ||
|
|
There was a problem hiding this comment.
Remove blank line.
| ->options(fn (Get $get) => Allocation::where('node_id', $get('node_id'))->whereNull('server_id')->when($get('allocation_id'), fn ($query) => $query->whereNot('id', $get('allocation_id')))->get()->mapWithKeys(fn (Allocation $allocation) => [$allocation->id => $allocation->address])) | ||
| ->searchable(['ip', 'port', 'ip_alias']) | ||
| ->placeholder(trans('admin/server.select_additional')), | ||
| Select::make('backups') |
There was a problem hiding this comment.
I would make that a checkboxlist. Looks nicer and allows bulk toggle.
There was a problem hiding this comment.
The backup uuids should be added to the ServerTransfer model.
|
I made your suggestions Boy, and a bunch of other things, as Wings was deleting backups but not the panel. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @app/Filament/Admin/Resources/Servers/Pages/EditServer.php:
- Around line 972-984: The deletion loop is currently operating on
$server->backups->whereNotIn('uuid', $selectedBackupUuids) which includes S3
backups that cannot be selected and thus will be deleted; update the collection
filtering to restrict to only daemon-managed backups before calling ->each(...).
Concretely, chain or apply a filter (e.g., where('disk', 'daemon') or
->filter(fn($b) => $b->disk === 'daemon' / $b->isDaemon()) ) to the
$server->backups query/collection so only daemon backups are passed into the
->each(...) that calls $deleteBackup->handle($backup), leaving S3 backups
untouched and preserving the existing try/catch Notification behavior.
🧹 Nitpick comments (1)
app/Filament/Admin/Resources/Servers/Pages/EditServer.php (1)
1079-1083: Consider capping the column count for large backup lists.The columns calculation
ceil($record->backups->...->count() / 4)could produce many columns for servers with numerous backups (e.g., 100 backups → 25 columns), degrading the modal layout.♻️ Suggested improvement
- ->columns(fn (Server $record) => (int) ceil($record->backups->where('disk', Backup::ADAPTER_DAEMON)->count() / 4)), + ->columns(fn (Server $record) => min(4, (int) ceil($record->backups->where('disk', Backup::ADAPTER_DAEMON)->count() / 4))),
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/Filament/Admin/Resources/Servers/Pages/EditServer.phpapp/Services/Servers/TransferServerService.phplang/en/admin/server.php
🚧 Files skipped from review as they are similar to previous changes (2)
- lang/en/admin/server.php
- app/Services/Servers/TransferServerService.php
🧰 Additional context used
🧠 Learnings (4)
📚 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/EditServer.php
📚 Learning: 2025-10-15T11:55:53.461Z
Learnt from: rmartinoscar
Repo: pelican-dev/panel PR: 1801
File: app/Extensions/OAuth/Schemas/AuthentikSchema.php:7-10
Timestamp: 2025-10-15T11:55:53.461Z
Learning: In Filament v4, Wizard Step components use the Filament\Schemas namespace (Filament\Schemas\Components\Wizard\Step), not Filament\Forms.
Applied to files:
app/Filament/Admin/Resources/Servers/Pages/EditServer.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/EditServer.php
📚 Learning: 2025-10-15T11:55:53.461Z
Learnt from: rmartinoscar
Repo: pelican-dev/panel PR: 1801
File: app/Extensions/OAuth/Schemas/AuthentikSchema.php:7-10
Timestamp: 2025-10-15T11:55:53.461Z
Learning: In Filament v4, the Forms component Placeholder was deprecated and removed. Use TextEntry from Filament\Infolists\Components\TextEntry in forms instead, binding values with ->state(). For HTML content, use ->html().
Applied to files:
app/Filament/Admin/Resources/Servers/Pages/EditServer.php
🧬 Code graph analysis (1)
app/Filament/Admin/Resources/Servers/Pages/EditServer.php (3)
app/Models/Backup.php (1)
Backup(36-107)app/Models/Server.php (2)
Server(135-552)backups(394-397)app/Services/Backups/DeleteBackupService.php (1)
DeleteBackupService(16-85)
🔇 Additional comments (3)
app/Filament/Admin/Resources/Servers/Pages/EditServer.php (3)
14-14: LGTM!The new imports are correctly organized and all are utilized in the implementation.
Also applies to: 19-19, 32-32, 54-54
975-984: Verify intended behavior when backup deletion fails.If a backup deletion fails, the loop continues and the transfer proceeds. This means:
- The failed-to-delete backup remains on the source node
- It won't be transferred (not in
selectedBackupUuids)- The user sees a notification per failure
Confirm this "best-effort deletion" behavior is acceptable, or consider aborting the transfer if any deletion fails.
1084-1086: LGTM!The
Textschema component is appropriate for displaying the backup warning message in Filament v4.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @app/Filament/Admin/Resources/Servers/Pages/EditServer.php:
- Around line 968-987: The code currently deletes daemon backups before calling
TransferServerService::handle, risking data loss if the transfer fails; change
the flow so you first call $transfer->handle($server, Arr::get($data,
'node_id'), Arr::get($data, 'allocation_id'), Arr::get($data,
'allocation_additional', []), $selectedBackupUuids) and only after it completes
successfully iterate $server->backups->whereNotIn('uuid',
$selectedBackupUuids)->where('disk', Backup::ADAPTER_DAEMON)->each(...) to call
DeleteBackupService::handle for each backup (keeping the inner try-catch and
Notification on individual delete failures), and ensure the outer try-catch
still handles transfer exceptions so deletions never run when transfer throws.
🧹 Nitpick comments (1)
app/Filament/Admin/Resources/Servers/Pages/EditServer.php (1)
1077-1089: Consider improving column calculation for backup selection.The column count calculation on line 1084 divides the backup count by 4 and rounds up. This results in a single column for 1-4 backups, which may create a vertically elongated layout. Consider using a minimum column count or a different formula for better visual balance.
♻️ Suggested improvement for column calculation
- ->columns(fn (Server $record) => (int) ceil($record->backups->where('disk', Backup::ADAPTER_DAEMON)->count() / 4)), + ->columns(fn (Server $record) => max(2, min(4, (int) ceil($record->backups->where('disk', Backup::ADAPTER_DAEMON)->count() / 4)))),This ensures:
- Minimum 2 columns for better layout
- Maximum 4 columns to prevent over-spreading
- Scales between 2-4 columns based on backup count
Additionally, consider eager-loading the backups relationship to avoid multiple collection filtering operations throughout the form (lines 1083, 1084, 1089).
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/Filament/Admin/Resources/Servers/Pages/EditServer.php
🧰 Additional context used
🧠 Learnings (4)
📚 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/EditServer.php
📚 Learning: 2025-10-15T11:55:53.461Z
Learnt from: rmartinoscar
Repo: pelican-dev/panel PR: 1801
File: app/Extensions/OAuth/Schemas/AuthentikSchema.php:7-10
Timestamp: 2025-10-15T11:55:53.461Z
Learning: In Filament v4, Wizard Step components use the Filament\Schemas namespace (Filament\Schemas\Components\Wizard\Step), not Filament\Forms.
Applied to files:
app/Filament/Admin/Resources/Servers/Pages/EditServer.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/EditServer.php
📚 Learning: 2025-10-15T11:55:53.461Z
Learnt from: rmartinoscar
Repo: pelican-dev/panel PR: 1801
File: app/Extensions/OAuth/Schemas/AuthentikSchema.php:7-10
Timestamp: 2025-10-15T11:55:53.461Z
Learning: In Filament v4, the Forms component Placeholder was deprecated and removed. Use TextEntry from Filament\Infolists\Components\TextEntry in forms instead, binding values with ->state(). For HTML content, use ->html().
Applied to files:
app/Filament/Admin/Resources/Servers/Pages/EditServer.php
🧬 Code graph analysis (1)
app/Filament/Admin/Resources/Servers/Pages/EditServer.php (3)
app/Models/Backup.php (1)
Backup(36-107)app/Models/Server.php (3)
Server(135-552)transfer(386-389)backups(394-397)app/Services/Backups/DeleteBackupService.php (1)
DeleteBackupService(16-85)
🔇 Additional comments (2)
app/Filament/Admin/Resources/Servers/Pages/EditServer.php (2)
14-14: LGTM: Imports added for backup transfer functionality.The new imports are appropriate for the backup selection and deletion features being added.
Also applies to: 19-19, 32-32, 54-54
987-987: No issues found. TheTransferServerService::handle()method correctly accepts the backup UUIDs array as the fifth parameter with the type hint?array $backup_uuid = [], as documented on line 51 of the service class.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/Filament/Admin/Resources/Servers/Pages/EditServer.php (1)
968-986: Service signature is correct; refactoring suggestions remain optional.The
TransferServerService::handle()method signature correctly accepts the$selectedBackupUuidsparameter as the 5th argument (?array $backup_uuid = []), and the code usage matches the service definition.For future optimization, consider these optional improvements:
Collection vs Query Builder: Line 973 loads all backups into memory before filtering. For servers with many backups, use the query builder:
$server->backups() ->whereNotIn('uuid', $selectedBackupUuids) ->where('disk', Backup::ADAPTER_DAEMON) ->get() ->each(function ($backup) use ($deleteBackup) { ... });Notification aggregation: If multiple backup deletions fail, consider collecting errors and showing a single summary notification instead of multiple individual notifications.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/Filament/Admin/Resources/Servers/Pages/EditServer.php
🧰 Additional context used
🧠 Learnings (4)
📚 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/EditServer.php
📚 Learning: 2025-10-15T11:55:53.461Z
Learnt from: rmartinoscar
Repo: pelican-dev/panel PR: 1801
File: app/Extensions/OAuth/Schemas/AuthentikSchema.php:7-10
Timestamp: 2025-10-15T11:55:53.461Z
Learning: In Filament v4, Wizard Step components use the Filament\Schemas namespace (Filament\Schemas\Components\Wizard\Step), not Filament\Forms.
Applied to files:
app/Filament/Admin/Resources/Servers/Pages/EditServer.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/EditServer.php
📚 Learning: 2025-10-15T11:55:53.461Z
Learnt from: rmartinoscar
Repo: pelican-dev/panel PR: 1801
File: app/Extensions/OAuth/Schemas/AuthentikSchema.php:7-10
Timestamp: 2025-10-15T11:55:53.461Z
Learning: In Filament v4, the Forms component Placeholder was deprecated and removed. Use TextEntry from Filament\Infolists\Components\TextEntry in forms instead, binding values with ->state(). For HTML content, use ->html().
Applied to files:
app/Filament/Admin/Resources/Servers/Pages/EditServer.php
🧬 Code graph analysis (1)
app/Filament/Admin/Resources/Servers/Pages/EditServer.php (3)
app/Models/Backup.php (1)
Backup(36-107)app/Models/Server.php (3)
Server(135-552)transfer(386-389)backups(394-397)app/Services/Backups/DeleteBackupService.php (1)
DeleteBackupService(16-85)
🔇 Additional comments (2)
app/Filament/Admin/Resources/Servers/Pages/EditServer.php (2)
14-14: LGTM! New imports are necessary and correctly added.The imported classes are properly used throughout the file for the backup transfer functionality.
Also applies to: 19-19, 32-32, 54-54
1076-1088: LGTM! Backup selection UI is well-implemented.The CheckboxList component is properly configured with:
- Bulk toggle functionality for user convenience
- Correct filtering to daemon backups only
- Appropriate visibility logic to hide when no backups exist
- Warning message to inform users about deletion of unselected backups
The dynamic column calculation on Line 1083 (
ceil(count / 4)) aims to display approximately 4 backups per column, which works well for typical scenarios. For servers with very large numbers of backups (e.g., 100+), this might create many columns, but this is an acceptable edge case.
5 participants
pelican-dev/wings#155
Using Quinten's wings pull request, this fixes the servers not transfering their backups.