Open
Conversation
📝 WalkthroughWalkthroughIntroduces a pluggable BackupHost / BackupAdapter system: adds adapter interface, registry, S3/Wings schemas, BackupHost model/resource/migration, provider wiring, and updates controllers/services/tests; removes legacy BackupManager/S3Filesystem and disk-based backup config; migrates backups to BackupHost references. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Initiator as InitiateBackupService
participant AdapterSvc as BackupAdapterService
participant Schema as BackupSchema (S3/Wings)
participant DB as Database
participant External as External System
User->>Initiator: request create backup
Initiator->>DB: create Backup record (backup_host_id)
Initiator->>AdapterSvc: get(backupHost.schema)
AdapterSvc-->>Initiator: BackupSchema instance
Initiator->>Schema: createBackup(backup)
Schema->>External: perform storage action (daemon or S3)
External-->>Schema: confirmation
Schema-->>Initiator: completion
Initiator-->>User: respond backup created
sequenceDiagram
participant Client as Client
participant Controller as BackupController
participant AdapterSvc as BackupAdapterService
participant Schema as BackupSchema (S3/Wings)
participant External as External System
Client->>Controller: request download link
Controller->>AdapterSvc: get(backup.backupHost.schema)
AdapterSvc-->>Controller: BackupSchema instance
Controller->>Schema: getDownloadLink(backup, user)
Schema->>External: generate presigned URL or JWT URL
External-->>Schema: url
Schema-->>Controller: download URL
Controller-->>Client: return URL
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 |
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Fix all issues with AI agents
In `@app/Extensions/BackupAdapter/Schemas/S3BackupSchema.php`:
- Around line 106-154: In getUploadParts, accessing
$backup->backupHost->configuration['storage_class'] directly can emit warnings
when that key is missing; change the $storageClass assignment to safely read the
key (e.g. $storageClass = $backup->backupHost->configuration['storage_class'] ??
null or use array_key_exists) and keep the subsequent null check (if
(!is_null($storageClass)) { $params['StorageClass'] = $storageClass; }) so
missing config no longer triggers PHP notices.
- Around line 19-36: The S3 client is using wrong config keys and a single
cached client for all hosts; in S3BackupSchema::createClient(BackupHost
$backupHost) map the form fields to AWS SDK names (map access_key → key,
secret_key → secret, default_region → region) and preserve token if present,
build credentials via Arr::only on those mapped keys, and include region/version
in $config before constructing S3Client; change the client cache to be per-host
(keyed by $backupHost->id) instead of the single ?S3Client $client to avoid
cross-host credential leakage; also ensure any use sites like getUploadParts
read and respect storage_class from the host configuration (defaulting if
absent).
In `@app/Filament/Admin/Resources/BackupHosts/Pages/EditBackupHost.php`:
- Around line 27-31: The DeleteAction in EditBackupHost.php currently only
disables deletion when the BackupHost has associated backups; update
DeleteAction::make() so it also disables (and adjusts its label) when
BackupHost::count() === 1 to prevent deleting the last host. Specifically,
change the ->disabled(...) callback to return true if
$backupHost->backups()->count() > 0 OR BackupHost::count() === 1, and update the
->label(...) callback to return a distinct translatable message when
BackupHost::count() === 1 (e.g., a "cannot delete last host" message) while
preserving the existing backups-related message; reference DeleteAction::make(),
BackupHost::backups(), and BackupHost::count() to locate the code.
In `@app/Http/Controllers/Api/Remote/Backups/BackupRemoteUploadController.php`:
- Around line 57-63: The code in BackupRemoteUploadController currently resolves
the schema from collect($node->backupHosts)->first(), which can pick the wrong
host; instead resolve the schema from the Backup model's host property (use
$backup->host or equivalent) and validate it is present before using it. Replace
the get(collect($node->backupHosts)->first()->schema) call with fetching the
host associated with $backup (guarding against null), then call
$this->backupService->get($backupHost->schema) and keep the instanceof
S3BackupSchema check; if the backup's host is null or the schema lookup fails,
throw a BadRequestHttpException.
In `@app/Http/Controllers/Api/Remote/Backups/BackupStatusController.php`:
- Around line 72-76: The current logic in BackupStatusController resolves the S3
schema using collect($node->backupHosts)->first(), which can pick the wrong host
or be null; instead resolve the schema from the backup's own host and guard
against missing hosts: call $this->backupService->get(...) using the host
associated with the backup model (the host/backupHost property on $model) and
only invoke S3BackupSchema->completeMultipartUpload($model, $successful,
$request->input('parts')) if the resolved schema is an S3BackupSchema and the
host/schema is not null.
In `@app/Services/Backups/DeleteBackupService.php`:
- Around line 34-37: The code in DeleteBackupService uses
$backup->backupHost->schema without checking whether $backup->backupHost is
null, which can cause a fatal error; update the method in DeleteBackupService to
first verify $backup->backupHost is not null (e.g., if null throw a clear
Exception or handle gracefully) before calling
$this->backupService->get($backup->backupHost->schema), and then proceed to
retrieve $schema and the subsequent logic only when the host exists.
In `@app/Services/Backups/DownloadLinkService.php`:
- Around line 22-25: The code accesses $backup->backupHost->schema without
checking if backupHost exists; add a null check in DownloadLinkService before
calling $this->backupService->get(...) so that if $backup->backupHost is null
you throw a specific exception (create e.g., MissingBackupHostException or
BackupAdapterException) instead of a generic Exception; update the method to
validate $backup->backupHost, throw the new exception with a clear message when
missing, and ensure any callers/handlers are adjusted to catch the new exception
type.
In `@app/Services/Backups/InitiateBackupService.php`:
- Around line 113-133: The current BackupHost selection (using
BackupHost::doesntHave('nodes')->orWhereHas('nodes', fn($q) => $q->where('id',
$server->node->id))->firstOrFail()) can pick a global host even when a
node-specific host exists; change the logic to first attempt to find a host
assigned to the server's node (e.g. BackupHost::whereHas('nodes', fn($q) =>
$q->where('id', $server->node->id))->first()), and if that returns null then
fall back to a global host (e.g.
BackupHost::doesntHave('nodes')->firstOrFail()); keep the remaining flow
(loading $schema, transaction, Backup::create, and $schema->createBackup) the
same but use the explicitly chosen $backupHost variable.
In `@app/Services/Servers/TransferServerService.php`:
- Line 31: The notify() method in TransferServerService.php currently ignores
the incoming $backup_uuids by overwriting them with $backups = [], causing
transfers to omit selected backups; update notify() to map/validate the provided
$backup_uuids into the $backups payload instead of emptying it (preserve
$backup_uuids, transform to the structure expected by the daemon API), ensure
you reference the $backup_uuids parameter and $backups variable inside notify(),
and adjust the payload format to match the daemon API spec (e.g., array of
objects or keyed field) before sending the transfer notification.
In `@database/migrations/2026_01_16_081858_create_backup_hosts_table.php`:
- Line 52: The migration's config array sets the 'prefix' key using the wrong
env var AWS_BACKUPS_BUCKET (copy-paste); update the 'prefix' entry to use the
dedicated environment variable (e.g., AWS_BACKUPS_PREFIX) instead of
AWS_BACKUPS_BUCKET in the migration file (create_backup_hosts_table.php) so the
'prefix' value is read from env('AWS_BACKUPS_PREFIX', '') and not from the
bucket variable.
- Around line 72-83: In the down() method the migration drops backup_hosts
before removing the backup_host_id foreign key on the backups table, which will
violate the FK constraint; update down() to first modify the backups table (in
the Schema::table('backups', ...) block) by adding the disk column, then remove
the foreign key (use $table->dropForeign(['backup_host_id']) or the explicit
constraint name) and then dropColumn('backup_host_id'), and only after that drop
the pivot table (backup_host_node) and finally drop backup_hosts (i.e., reorder
operations so foreign key is removed before dropping the referenced table).
In `@lang/en/admin/backuphost.php`:
- Line 5: The translation string 'model_label_plural' currently reads "Database
Hosts" but should be "Backup Hosts"; update the value for the
'model_label_plural' key in this translation file to "Backup Hosts" (preserve
surrounding quotes and comma) so the plural label matches the backup host
context.
🧹 Nitpick comments (7)
app/Policies/BackupHostPolicy.php (1)
21-24: Avoid N+1 queries in node authorization checks.
canTarget()can hit the DB per node; a set-based check reduces queries for hosts with many nodes.♻️ Example set-based check
- foreach ($backupHost->nodes as $node) { - if (!$user->canTarget($node)) { - return false; - } - } + $nodeIds = $backupHost->nodes->modelKeys(); + if ($nodeIds !== [] && $user->accessibleNodes()->whereIn('id', $nodeIds)->count() !== count($nodeIds)) { + return false; + }app/Models/Node.php (1)
279-282: Use canonicalbelongsToManycasing for consistency.PHP is case‑insensitive here, but consistent casing avoids IDE/static analysis confusion.
♻️ Proposed tweak
- public function backupHosts(): BelongsToMany - { - return $this->BelongsToMany(BackupHost::class); - } + /** `@return` BelongsToMany<BackupHost, $this> */ + public function backupHosts(): BelongsToMany + { + return $this->belongsToMany(BackupHost::class); + }database/migrations/2026_01_16_081858_create_backup_hosts_table.php (2)
60-64: Avoid using Eloquent models in migrations.Using
BackupHost::create()in a migration is fragile. If the model's$fillable, validation rules, or other attributes change in the future, this migration may break when run on a fresh database. Use query builder instead.Proposed fix
- $backupHost = BackupHost::create([ - 'name' => $oldDriver === 's3' ? 'Remote' : 'Local', - 'schema' => $oldDriver, - 'configuration' => $oldConfiguration, - ]); + $backupHostId = DB::table('backup_hosts')->insertGetId([ + 'name' => $oldDriver === 's3' ? 'Remote' : 'Local', + 'schema' => $oldDriver, + 'configuration' => $oldConfiguration ? json_encode($oldConfiguration) : null, + 'created_at' => now(), + 'updated_at' => now(), + ]); - DB::table('backups')->update(['backup_host_id' => $backupHost->id]); + DB::table('backups')->update(['backup_host_id' => $backupHostId]);Also remove the
use App\Models\BackupHost;import at the top.
36-41: Consider makingbackup_host_idnullable initially during migration.Adding a non-nullable foreign key column and then immediately updating all rows works, but if the backups table is large, this could cause issues. Additionally, if the migration fails partway, you may end up with rows having a
0or invalidbackup_host_id.A safer pattern is to:
- Add the column as nullable
- Create the backup host and update existing records
- Alter the column to be non-nullable
This is a minor concern given the migration context, but worth considering for robustness.
app/Services/Backups/DeleteBackupService.php (1)
16-19: Update docblock to reflect adapter-based architecture.The comment mentions "If the backup is stored in S3" but the implementation is now adapter-agnostic. Consider updating the documentation to reflect the new architecture.
Proposed update
/** - * Deletes a backup from the system. If the backup is stored in S3 a request - * will be made to delete that backup from the disk as well. + * Deletes a backup from the system. The backup adapter handles + * removing the backup data from the configured storage backend. * * `@throws` Throwable */app/Models/BackupHost.php (1)
46-50: Align casts with CarbonImmutable docblocks.Consider casting
id,created_at, andupdated_atto match the declared types and other models’ patterns.♻️ Suggested adjustment
protected function casts(): array { return [ + 'id' => 'int', 'configuration' => 'array', + 'created_at' => 'immutable_datetime', + 'updated_at' => 'immutable_datetime', ]; }app/Extensions/BackupAdapter/Schemas/WingsBackupSchema.php (1)
57-62: Use a Form/Schema component for the “no configuration” message.
TextEntryis an Infolists component andTextEntry::make(trans(...))uses a translation string as the state path. If this schema is rendered in a form, it may not display correctly. Prefer a Forms component likePlaceholder(or a Schema-compatible view field).♻️ Suggested adjustment
-use Filament\Infolists\Components\TextEntry; +use Filament\Forms\Components\Placeholder; @@ /** `@return` Component[] */ public function getConfigurationForm(): array { return [ - TextEntry::make(trans('admin/backuphost.no_configuration')), + Placeholder::make('no_configuration') + ->content(trans('admin/backuphost.no_configuration')), ]; }
app/Filament/Admin/Resources/BackupHosts/Pages/EditBackupHost.php
Outdated
Show resolved
Hide resolved
app/Http/Controllers/Api/Remote/Backups/BackupRemoteUploadController.php
Show resolved
Hide resolved
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
# Conflicts: # app/Enums/RolePermissionModels.php
# Conflicts: # app/Filament/Admin/Pages/Settings.php # app/Filament/Admin/Resources/Servers/Pages/EditServer.php
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Filament/Server/Resources/Backups/BackupResource.php (1)
205-216:⚠️ Potential issue | 🟡 MinorHandle download-link failures gracefully before starting the restore.
DownloadLinkService::handle()can throw (e.g., unknown adapter). Since this is now unconditional, a failure will surface as a generic UI error and may still log a restore attempt. Consider precomputing the URL with a friendly notification and only logging/continuing on success.💡 Suggested adjustment
- $log = Activity::event('server:backup.restore') - ->subject($backup) - ->property(['name' => $backup->name, 'truncate' => $data['truncate']]); - - $log->transaction(function () use ($downloadLinkService, $daemonRepository, $backup, $server, $data) { - $url = $downloadLinkService->handle($backup, user()); + try { + $url = $downloadLinkService->handle($backup, user()); + } catch (Throwable $e) { + return Notification::make() + ->title(trans('server/backup.actions.restore.notification_fail')) + ->body(trans('server/backup.actions.restore.notification_fail_body_adapter')) + ->danger() + ->send(); + } + + $log = Activity::event('server:backup.restore') + ->subject($backup) + ->property(['name' => $backup->name, 'truncate' => $data['truncate']]); + + $log->transaction(function () use ($daemonRepository, $backup, $server, $data, $url) { // Update the status right away for the server so that we know not to allow certain // actions against it via the Panel API. $server->update(['status' => ServerState::RestoringBackup]); $daemonRepository->setServer($server)->restore($backup, $url, $data['truncate']); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Filament/Server/Resources/Backups/BackupResource.php` around lines 205 - 216, Wrap the call to DownloadLinkService::handle($backup, user()) in a try/catch before you create/commit the Activity log and before changing server state: if handle() throws, catch the exception, return a friendly notification/error to the user and abort the restore flow so no Activity::event('server:backup.restore') is committed and $server->update(['status' => ServerState::RestoringBackup]) is not executed; only after successfully obtaining $url should you create the $log, call $log->transaction(...) and invoke $daemonRepository->setServer($server)->restore($backup, $url, $data['truncate']).
🧹 Nitpick comments (1)
app/Models/Node.php (1)
281-284: Fix method call casing and add missing@returnannotation.
$this->BelongsToMany(...)at Line 283 has an uppercaseB. PHP method calls are case-insensitive so this works at runtime, but it's inconsistent withdatabaseHosts()(and every other relation in the file) that use lowercase$this->belongsToMany(...). The method also lacks the generic@returnannotation that was just added todatabaseHosts().♻️ Proposed fix
+ /** `@return` BelongsToMany<BackupHost, $this> */ public function backupHosts(): BelongsToMany { - return $this->BelongsToMany(BackupHost::class); + return $this->belongsToMany(BackupHost::class); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/Node.php` around lines 281 - 284, The relation method backupHosts() uses incorrect casing and lacks a return annotation: replace the call to $this->BelongsToMany(BackupHost::class) with the conventional $this->belongsToMany(BackupHost::class) and add the docblock `@return` \Illuminate\Database\Eloquent\Relations\BelongsToMany (or the project's BelongsToMany import) above the backupHosts() method to match databaseHosts() and other relations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Filament/Admin/Resources/Servers/Pages/EditServer.php`:
- Around line 1088-1094: Extract the repeated filter into a private helper on
the EditServer class to avoid null dereferences and DRY: add a private function
getWingsBackups(Server $server, BackupAdapterService $backupService) that
returns $server->backups->filter(fn($backup) => $backup->backupHost !== null &&
$backupService->get($backup->backupHost->schema) instanceof WingsBackupSchema);
then update the three closures to call this helper: for options use
$this->getWingsBackups($server, $backupService)->mapWithKeys(...), for columns
compute ceil($this->getWingsBackups($record, $backupService)->count() / 4), and
for hidden use $this->getWingsBackups($server, $backupService)->count() === 0 so
you only check backupHost once and avoid null-dereference.
- Around line 987-992: The closure that iterates backups accesses
$backup->backupHost->schema which can be null and causes a fatal error; update
the closure in the ->each(...) block to first fetch $host = $backup->backupHost
and guard it (if !$host then call $backup->delete() and return) before calling
$backupService->get($host->schema), then continue with the existing check for
instanceof WingsBackupSchema and $backup->delete() as needed.
- Around line 987-992: The current iterator calls $backup->delete() directly
which only removes the DB record and leaves files on the Wings daemon; retrieve
the schema via $backupService->get(...) and if it's an instance of
WingsBackupSchema call $schema->deleteBackup($backup) before deleting the DB
record, and perform both actions inside a transaction (follow the pattern in
DeleteBackupService::handle()) so the daemon DELETE is attempted before calling
$backup->delete().
---
Outside diff comments:
In `@app/Filament/Server/Resources/Backups/BackupResource.php`:
- Around line 205-216: Wrap the call to DownloadLinkService::handle($backup,
user()) in a try/catch before you create/commit the Activity log and before
changing server state: if handle() throws, catch the exception, return a
friendly notification/error to the user and abort the restore flow so no
Activity::event('server:backup.restore') is committed and
$server->update(['status' => ServerState::RestoringBackup]) is not executed;
only after successfully obtaining $url should you create the $log, call
$log->transaction(...) and invoke
$daemonRepository->setServer($server)->restore($backup, $url,
$data['truncate']).
---
Nitpick comments:
In `@app/Models/Node.php`:
- Around line 281-284: The relation method backupHosts() uses incorrect casing
and lacks a return annotation: replace the call to
$this->BelongsToMany(BackupHost::class) with the conventional
$this->belongsToMany(BackupHost::class) and add the docblock `@return`
\Illuminate\Database\Eloquent\Relations\BelongsToMany (or the project's
BelongsToMany import) above the backupHosts() method to match databaseHosts()
and other relations.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/Enums/RolePermissionModels.phpapp/Filament/Admin/Pages/Settings.phpapp/Filament/Admin/Resources/Servers/Pages/EditServer.phpapp/Filament/Server/Resources/Backups/BackupResource.phpapp/Models/Node.phpapp/Providers/AppServiceProvider.php
💤 Files with no reviewable changes (2)
- app/Filament/Admin/Pages/Settings.php
- app/Providers/AppServiceProvider.php
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
app/Filament/Admin/Resources/Servers/Pages/EditServer.php (1)
985-993:⚠️ Potential issue | 🟠 MajorCall the Wings schema delete before removing the DB record.
Deleting only the DB row can leave orphaned backups on the daemon. Use the schema delete first (or the existing delete service) and then remove the record.
🔧 Suggested fix
->each(function ($backup) use ($backupService) { $schema = $backupService->get($backup->backupHost->schema); // Wings backups that aren't transferred only need to be delete on the panel, wings will cleanup the backup files automatically if ($schema instanceof WingsBackupSchema) { + $schema->deleteBackup($backup); $backup->delete(); } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Filament/Admin/Resources/Servers/Pages/EditServer.php` around lines 985 - 993, In the loop over $server->backups (the block using $backupService->get($backup->backupHost->schema) and checking "instanceof WingsBackupSchema"), call the schema's deletion routine (or the project-wide backup delete service) to remove the backup from the daemon before calling $backup->delete() on the DB record; i.e., for WingsBackupSchema invoke its delete/cleanup method (or use the existing backup deletion service) and only after that succeeds remove the backup row with $backup->delete() to avoid leaving orphaned files on the daemon.app/Extensions/BackupAdapter/Schemas/S3BackupSchema.php (2)
24-33:⚠️ Potential issue | 🟠 MajorFilter S3 client config to avoid invalid options and isolate credentials.
Line 26 passes the entire backup host configuration into
S3Client. If the AWS SDK validates options, unknown keys likebucket,key, andsecretcan triggerInvalidArgumentExceptionat runtime. Build a clean config array and move credentials undercredentials.🛠️ Suggested fix
- private function createClient(BackupHost $backupHost): S3Client - { - $config = $backupHost->configuration; - $config['version'] = 'latest'; - - if (!empty($config['key']) && !empty($config['secret'])) { - $config['credentials'] = Arr::only($config, ['key', 'secret', 'token']); - } - - return new S3Client($config); - } + private function createClient(BackupHost $backupHost): S3Client + { + $cfg = $backupHost->configuration; + $config = [ + 'version' => 'latest', + 'region' => $cfg['region'] ?? null, + 'endpoint' => $cfg['endpoint'] ?? null, + 'use_path_style_endpoint' => $cfg['use_path_style_endpoint'] ?? false, + ]; + + if (!empty($cfg['key']) && !empty($cfg['secret'])) { + $config['credentials'] = [ + 'key' => $cfg['key'], + 'secret' => $cfg['secret'], + 'token' => $cfg['token'] ?? null, + ]; + } + + return new S3Client(array_filter($config, fn ($v) => $v !== null)); + }AWS SDK for PHP S3Client configuration options: does it reject unknown keys like "bucket", "key", or "secret" at the top level?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Extensions/BackupAdapter/Schemas/S3BackupSchema.php` around lines 24 - 33, The createClient(BackupHost $backupHost) currently passes the entire $backupHost->configuration into new S3Client which can include invalid top-level keys (e.g., bucket, key, secret); instead, build a sanitized $config array containing only allowed AWS options (at minimum 'version' and 'region' if present), move credentials into a 'credentials' sub-array (use Arr::only($configRaw, ['key','secret','token']) to populate it) and then pass that sanitized $config to new S3Client; update the createClient method to read $backupHost->configuration into a temp, pick allowed keys, set 'version' => 'latest', add 'credentials' when keys exist, and then instantiate S3Client with the cleaned config.
103-151:⚠️ Potential issue | 🟡 MinorGuard
storage_classaccess to prevent PHP notices.Line 115 directly indexes
configuration['storage_class']. If the key is missing, PHP emits a notice. Use null coalescing before the check.🔧 Suggested fix
- $storageClass = $backup->backupHost->configuration['storage_class']; - if (!is_null($storageClass)) { + $storageClass = $backup->backupHost->configuration['storage_class'] ?? null; + if ($storageClass !== null) { $params['StorageClass'] = $storageClass; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Extensions/BackupAdapter/Schemas/S3BackupSchema.php` around lines 103 - 151, The code in getUploadParts accesses $backup->backupHost->configuration['storage_class'] directly which can emit a PHP notice if the key is missing; update the access to use a null-coalescing lookup (e.g. $backup->backupHost->configuration['storage_class'] ?? null) or fetch into a variable via $storageClass = $backup->backupHost->configuration['storage_class'] ?? null before the if-check, then use that $storageClass for the subsequent if (!is_null($storageClass)) and when setting $params['StorageClass'] to prevent notices.
🧹 Nitpick comments (1)
app/Models/Node.php (1)
281-284: Add@returngeneric annotation for consistency withdatabaseHosts().
databaseHosts()at line 275 was just updated to carry@return BelongsToMany<DatabaseHost, $this>, but the newbackupHosts()method is missing the equivalent annotation.♻️ Proposed fix
+ /** `@return` BelongsToMany<BackupHost, $this> */ public function backupHosts(): BelongsToMany { return $this->belongsToMany(BackupHost::class); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/Node.php` around lines 281 - 284, Add the same generic `@return` docblock to the backupHosts relation as used on databaseHosts: update the PHPDoc for the backupHosts() method to declare `@return BelongsToMany<BackupHost, $this>` so IDEs and static analyzers get the precise relation types; locate the backupHosts() method and add the matching annotation referencing BackupHost and $this.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Extensions/BackupAdapter/Schemas/S3BackupSchema.php`:
- Around line 56-70: The getDownloadLink(Backup $backup, User $user): string
method currently ignores the $user parameter which trips PHPMD; to fix, either
use $user for audit/logging inside getDownloadLink (e.g., record who requested
the URL) or explicitly mark it as intentionally unused to silence PHPMD by
adding a no-op usage like casting (void)$user; at the top of the getDownloadLink
body; update the PHPDoc if you choose suppression so reviewers know it's
intentional.
---
Duplicate comments:
In `@app/Extensions/BackupAdapter/Schemas/S3BackupSchema.php`:
- Around line 24-33: The createClient(BackupHost $backupHost) currently passes
the entire $backupHost->configuration into new S3Client which can include
invalid top-level keys (e.g., bucket, key, secret); instead, build a sanitized
$config array containing only allowed AWS options (at minimum 'version' and
'region' if present), move credentials into a 'credentials' sub-array (use
Arr::only($configRaw, ['key','secret','token']) to populate it) and then pass
that sanitized $config to new S3Client; update the createClient method to read
$backupHost->configuration into a temp, pick allowed keys, set 'version' =>
'latest', add 'credentials' when keys exist, and then instantiate S3Client with
the cleaned config.
- Around line 103-151: The code in getUploadParts accesses
$backup->backupHost->configuration['storage_class'] directly which can emit a
PHP notice if the key is missing; update the access to use a null-coalescing
lookup (e.g. $backup->backupHost->configuration['storage_class'] ?? null) or
fetch into a variable via $storageClass =
$backup->backupHost->configuration['storage_class'] ?? null before the if-check,
then use that $storageClass for the subsequent if (!is_null($storageClass)) and
when setting $params['StorageClass'] to prevent notices.
In `@app/Filament/Admin/Resources/Servers/Pages/EditServer.php`:
- Around line 985-993: In the loop over $server->backups (the block using
$backupService->get($backup->backupHost->schema) and checking "instanceof
WingsBackupSchema"), call the schema's deletion routine (or the project-wide
backup delete service) to remove the backup from the daemon before calling
$backup->delete() on the DB record; i.e., for WingsBackupSchema invoke its
delete/cleanup method (or use the existing backup deletion service) and only
after that succeeds remove the backup row with $backup->delete() to avoid
leaving orphaned files on the daemon.
---
Nitpick comments:
In `@app/Models/Node.php`:
- Around line 281-284: Add the same generic `@return` docblock to the
backupHosts relation as used on databaseHosts: update the PHPDoc for the
backupHosts() method to declare `@return BelongsToMany<BackupHost, $this>` so
IDEs and static analyzers get the precise relation types; locate the
backupHosts() method and add the matching annotation referencing BackupHost and
$this.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/Extensions/BackupAdapter/Schemas/S3BackupSchema.phpapp/Filament/Admin/Resources/BackupHosts/BackupHostResource.phpapp/Filament/Admin/Resources/BackupHosts/Pages/EditBackupHost.phpapp/Filament/Admin/Resources/BackupHosts/Pages/ListBackupHosts.phpapp/Filament/Admin/Resources/Servers/Pages/EditServer.phpapp/Models/Node.php
🚧 Files skipped from review as they are similar to previous changes (3)
- app/Filament/Admin/Resources/BackupHosts/Pages/ListBackupHosts.php
- app/Filament/Admin/Resources/BackupHosts/Pages/EditBackupHost.php
- app/Filament/Admin/Resources/BackupHosts/BackupHostResource.php
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
app/Filament/Admin/Resources/Servers/Pages/EditServer.php (1)
985-993:⚠️ Potential issue | 🟠 MajorVerify Wings cleanup before deleting backup DB rows.
Line 991–992 deletes only the panel record. If Wings doesn’t auto-prune untransferred backups, this leaves orphaned files. Consider callingdeleteBackup()beforedelete(), or confirm the daemon’s cleanup behavior.🧩 Suggested fix (if cleanup is not automatic)
- if ($schema instanceof WingsBackupSchema) { - $backup->delete(); - } + if ($schema instanceof WingsBackupSchema) { + $schema->deleteBackup($backup); + $backup->delete(); + }Pelican Wings server transfer: does the daemon automatically delete backups not included in a transfer, or must the panel call the backup delete API?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Filament/Admin/Resources/Servers/Pages/EditServer.php` around lines 985 - 993, The code currently deletes only the DB row for Wings backups (inside the closure using $backupService->get(...) and checking instanceof WingsBackupSchema) which may leave orphaned files; update the logic to verify daemon cleanup and if not guaranteed, call the backup model's remote deletion method (e.g., deleteBackup() or the appropriate API method on the backup model/service) before calling $backup->delete(), ensuring you still use the same WingsBackupSchema check and $backupService->get(...) to locate Wings backups.
🧹 Nitpick comments (1)
app/Filament/Server/Resources/Backups/BackupResource.php (1)
173-173:user()passes a nullable type toDownloadLinkService::handle
user()returnsAuthenticatable|null, and while the->authorize()guard ensures the user is authenticated when this callback runs, passinguser()without asserting non-null may trip static analysis. The same pattern applies at line 210.🛡️ Optional null assertion
-->url(fn (DownloadLinkService $downloadLinkService, Backup $backup) => $downloadLinkService->handle($backup, user()), true) +->url(fn (DownloadLinkService $downloadLinkService, Backup $backup) => $downloadLinkService->handle($backup, user()!), true)Or, if the project uses PHPDoc assertions:
-$url = $downloadLinkService->handle($backup, user()); +/** `@var` \App\Models\User $user */ +$user = user(); +$url = $downloadLinkService->handle($backup, $user);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Filament/Server/Resources/Backups/BackupResource.php` at line 173, The callback passes user() (nullable) into DownloadLinkService::handle which can trigger static-analysis errors; capture the result into a local variable, assert it's non-null (e.g. $authUser = user(); assert($authUser instanceof \Illuminate\Contracts\Auth\Authenticatable); or throw if null) and then pass $authUser to DownloadLinkService::handle; apply the same change for the second occurrence referenced (the other callback around line 210) so both calls use an asserted non-null $authUser variable before calling handle.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Filament/Admin/Resources/BackupHosts/BackupHostResource.php`:
- Around line 123-137: The configuration section always reads the persisted
model schema first (BackupHost->schema) so changing the schema in the live form
doesn't update the configuration UI; update the selection order to prefer the
live form state from Get $get('schema') and fall back to the model's schema only
if the live value is empty. Locate the closure passed to Section::make(...) that
receives (?BackupHost $backupHost, Get $get, BackupAdapterService $service) and
change the logic that sets $schema so it checks $get('schema') first, then
$backupHost->schema as a fallback, then call $service->get($schema) and return
$schema->getConfigurationForm() as before.
In `@app/Services/Servers/TransferServerService.php`:
- Around line 30-45: The backup lookup in the notify method is using
Backup::find($uuid) which looks up by integer PK and returns null for UUID
strings; change the lookup in notify (method name: notify, class:
TransferServerService) to resolve by the uuid column (e.g., query Backup where
'uuid' equals the $uuid and take first()) so $backup is correctly loaded and
WingsBackupSchema checks run; update any related logic that uses $backup
accordingly.
---
Duplicate comments:
In `@app/Filament/Admin/Resources/Servers/Pages/EditServer.php`:
- Around line 985-993: The code currently deletes only the DB row for Wings
backups (inside the closure using $backupService->get(...) and checking
instanceof WingsBackupSchema) which may leave orphaned files; update the logic
to verify daemon cleanup and if not guaranteed, call the backup model's remote
deletion method (e.g., deleteBackup() or the appropriate API method on the
backup model/service) before calling $backup->delete(), ensuring you still use
the same WingsBackupSchema check and $backupService->get(...) to locate Wings
backups.
---
Nitpick comments:
In `@app/Filament/Server/Resources/Backups/BackupResource.php`:
- Line 173: The callback passes user() (nullable) into
DownloadLinkService::handle which can trigger static-analysis errors; capture
the result into a local variable, assert it's non-null (e.g. $authUser = user();
assert($authUser instanceof \Illuminate\Contracts\Auth\Authenticatable); or
throw if null) and then pass $authUser to DownloadLinkService::handle; apply the
same change for the second occurrence referenced (the other callback around line
210) so both calls use an asserted non-null $authUser variable before calling
handle.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/Filament/Admin/Resources/BackupHosts/BackupHostResource.phpapp/Filament/Admin/Resources/BackupHosts/RelationManagers/BackupsRelationManager.phpapp/Filament/Admin/Resources/Servers/Pages/EditServer.phpapp/Filament/Server/Resources/Backups/BackupResource.phpapp/Services/Servers/TransferServerService.php
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
app/Filament/Admin/Resources/Servers/Pages/EditServer.php (1)
985-993: Confirm Wings cleanup before deleting the DB record.
If Wings does not automatically remove backup archives during transfer,$backup->delete()will orphan storage. Please verify the cleanup behavior; otherwise call$schema->deleteBackup($backup)before deleting the model.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Filament/Admin/Resources/Servers/Pages/EditServer.php` around lines 985 - 993, The current loop deletes backup DB records for backups with Wings schema without ensuring storage cleanup; update the logic in the block handling $server->backups->whereNotIn('uuid', $selectedBackupUuids)->each(...) to verify Wings actually removed the archive and, if not, call the schema-level cleanup method (use $backupService->get($backup->backupHost->schema) to obtain $schema and then call $schema->deleteBackup($backup) or equivalent) before invoking $backup->delete(); ensure you still only call $backup->delete() after successful cleanup or on a confirmed Wings-auto-cleanup path (keep usage of WingsBackupSchema to branch the behavior).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Services/Servers/TransferServerService.php`:
- Around line 67-68: The handle method's signature allows ?array $backup_uuids
but later passes it into notify() which requires array, causing a TypeError; fix
by making $backup_uuids non-nullable (change signature to array $backup_uuids =
[]) or, inside TransferServerService::handle, coerce null to an empty array
(e.g. $backup_uuids = $backup_uuids ?? []) before calling notify(); update the
same pattern for the other occurrence mentioned (line ~118) to ensure notify()
always receives an array.
- Around line 36-45: The loop is doing an unscoped, per-UUID lookup
(Backup::where('uuid', ...) inside the foreach over $backup_uuids) causing
potential cross-server matches and N+1 queries; replace it by querying all
backups for the transfer server once (e.g. fetch Backup::where('server_id',
$server->id)->whereIn('uuid', $backup_uuids')->get()), then iterate that
collection and call $this->backupService->get(...) to check instanceof
WingsBackupSchema and populate $backups, ensuring you only consider backups
belonging to the target server and avoid repeated DB calls.
---
Duplicate comments:
In `@app/Filament/Admin/Resources/Servers/Pages/EditServer.php`:
- Around line 985-993: The current loop deletes backup DB records for backups
with Wings schema without ensuring storage cleanup; update the logic in the
block handling $server->backups->whereNotIn('uuid',
$selectedBackupUuids)->each(...) to verify Wings actually removed the archive
and, if not, call the schema-level cleanup method (use
$backupService->get($backup->backupHost->schema) to obtain $schema and then call
$schema->deleteBackup($backup) or equivalent) before invoking $backup->delete();
ensure you still only call $backup->delete() after successful cleanup or on a
confirmed Wings-auto-cleanup path (keep usage of WingsBackupSchema to branch the
behavior).
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/Filament/Admin/Resources/BackupHosts/BackupHostResource.phpapp/Filament/Admin/Resources/BackupHosts/RelationManagers/BackupsRelationManager.phpapp/Filament/Admin/Resources/Servers/Pages/EditServer.phpapp/Filament/Server/Resources/Backups/BackupResource.phpapp/Services/Servers/TransferServerService.php
🚧 Files skipped from review as they are similar to previous changes (1)
- app/Filament/Server/Resources/Backups/BackupResource.php
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/Services/Servers/TransferServerService.php (2)
33-39: Avoid N+1 onbackupHostduring filtering.
Each$backup->backupHostaccess can trigger an extra query; eager load the relation before filtering.♻️ Proposed tweak
- $backups = Backup::where('server_id', $transfer->server_id) - ->whereIn('uuid', $backup_uuids) - ->get() + $backups = Backup::where('server_id', $transfer->server_id) + ->whereIn('uuid', $backup_uuids) + ->with('backupHost') + ->get() ->filter(fn (Backup $backup) => $this->backupService->get($backup->backupHost->schema) instanceof WingsBackupSchema) ->pluck('uuid') ->all();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Services/Servers/TransferServerService.php` around lines 33 - 39, The code filters backups by accessing $backup->backupHost inside a collection filter, causing N+1 queries; eager load the backupHost relation before retrieving/filtering to prevent extra queries. Update the Backup query used to build $backups (the chain that calls Backup::where(...)->whereIn(...)) to include the backupHost relation (e.g., via with('backupHost')) so the subsequent ->filter(fn (Backup $backup) => $this->backupService->get($backup->backupHost->schema) instanceof WingsBackupSchema) runs in-memory without triggering additional queries.
30-30: Track the TODO for persisting backup UUIDs.
Line 30 notes missing persistence; consider opening a follow-up issue so selected backups aren’t lost if transfers need retry/audit later.Would you like me to draft an issue or propose a migration sketch?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Services/Servers/TransferServerService.php` at line 30, The comment flags a TODO to persist backup UUIDs for transfers; open a follow-up issue and implement it by adding a new column (e.g. backup_uuids as JSON or text) to the ServerTransfer model via a migration, add the corresponding $casts or attribute on ServerTransfer to treat it as an array, update the TransferServerService logic where the docblock and TODO appear to save the provided backup_uuids into the ServerTransfer record when creating/updating transfers, and ensure any validation/serialization is handled consistently when reading/writing that field.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/Services/Servers/TransferServerService.php`:
- Around line 33-39: The code filters backups by accessing $backup->backupHost
inside a collection filter, causing N+1 queries; eager load the backupHost
relation before retrieving/filtering to prevent extra queries. Update the Backup
query used to build $backups (the chain that calls
Backup::where(...)->whereIn(...)) to include the backupHost relation (e.g., via
with('backupHost')) so the subsequent ->filter(fn (Backup $backup) =>
$this->backupService->get($backup->backupHost->schema) instanceof
WingsBackupSchema) runs in-memory without triggering additional queries.
- Line 30: The comment flags a TODO to persist backup UUIDs for transfers; open
a follow-up issue and implement it by adding a new column (e.g. backup_uuids as
JSON or text) to the ServerTransfer model via a migration, add the corresponding
$casts or attribute on ServerTransfer to treat it as an array, update the
TransferServerService logic where the docblock and TODO appear to save the
provided backup_uuids into the ServerTransfer record when creating/updating
transfers, and ensure any validation/serialization is handled consistently when
reading/writing that field.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1919
Also allows to register custom adapters (#1600)
Largely untested!!!