Backup hosts#2125
Open
Boy132 wants to merge 33 commits into
Open
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughIntroduce a pluggable BackupHost / BackupAdapter system: new adapter interface, registry and abstract schema, S3 and Wings adapter implementations, BackupHost model/migration/factory, provider wiring, controller/service refactors to delegate to adapters, Filament UI for BackupHosts, and removal of legacy BackupManager/S3Filesystem and disk-based backup config. ChangesBackup Adapter & Hosts
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 Storage/Daemon
User->>Initiator: request create backup
Initiator->>DB: insert Backup (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 Storage/Daemon
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 token
External-->>Schema: url/token
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 |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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')), ]; }
Member
Author
|
@coderabbitai review |
Contributor
✅ 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 |
Contributor
✅ Actions performedReview triggered.
|
Contributor
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
Contributor
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
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Contributor
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
Contributor
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
Contributor
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.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Models/Backup.php (1)
17-60:⚠️ Potential issue | 🟡 MinorRemove stale
whereDisk()PHPDoc annotation.The docblock still includes
whereDisk($value)on Line 48, butdiskis no longer a Backup attribute. Please replace it withwhereBackupHostId($value)to keep IDE/static-analysis hints accurate.🧩 Suggested docblock update
- * `@method` static BackupQueryBuilder<static>|Backup whereDisk($value) + * `@method` static BackupQueryBuilder<static>|Backup whereBackupHostId($value)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/Backup.php` around lines 17 - 60, Update the Backup model docblock to remove the stale whereDisk($value) annotation and replace it with whereBackupHostId($value); locate the class docblock (the `@method` annotations in Backup) and swap the whereDisk entry for whereBackupHostId so IDEs and static analysis reflect the current Backup attributes.
♻️ Duplicate comments (1)
app/Services/Servers/TransferServerService.php (1)
38-40:⚠️ Potential issue | 🟠 MajorAdd
->values()to reindex keys before JSON serialization.After
filter()on line 39, numeric keys become sparse (gappy). When serialized to JSON, sparse numeric keys will encodebackupsas an object instead of an array. Add->values()before->all()to reset keys to consecutive integers and ensure correct JSON serialization.Patch
$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') + ->values() ->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 38 - 40, The collection produced by filtering backups becomes sparse and will JSON-serialize as an object; update the chain in TransferServerService (the pipeline using filter(fn (Backup $backup) => $this->backupService->get($backup->backupHost->schema) instanceof WingsBackupSchema)->pluck('uuid')->all()) to reindex keys by inserting ->values() before ->all() so the resulting array of UUIDs has consecutive numeric keys and serializes as a JSON array.
🧹 Nitpick comments (2)
app/Models/Backup.php (1)
87-87: Tighten FK validation tointeger.For an id foreign key, prefer
integerovernumericso only integer values are accepted beforeexistsruns.✅ Suggested validation tweak
- 'backup_host_id' => ['required', 'numeric', 'exists:backup_hosts,id'], + 'backup_host_id' => ['required', 'integer', 'exists:backup_hosts,id'],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/Backup.php` at line 87, Update the validation rule for the backup_host_id field in Backup.php to use integer rather than numeric: locate the validation array (the 'backup_host_id' => ['required', 'numeric', 'exists:backup_hosts,id'] entry—commonly found in the rules() method or $rules property of the Backup model) and replace 'numeric' with 'integer' so the rule becomes ['required','integer','exists:backup_hosts,id'].app/Filament/Admin/Resources/Servers/Pages/EditServer.php (1)
985-989: Eager-loadbackupHostin the cleanup loop to avoid N+1 queries.Lines 985-989 iterate over backups and access
$backup->backupHost->schemawithout eager-loading thebackupHostrelationship. This causes a separate database query for each backup.Suggested refactor
- $server->backups + $server->backups() + ->with('backupHost') ->whereNotIn('uuid', $selectedBackupUuids) + ->get() ->each(function ($backup) use ($backupService) { $schema = $backupService->get($backup->backupHost->schema);🤖 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 - 989, The loop iterates $server->backups and accesses $backup->backupHost->schema causing N+1 queries; ensure the backupHost relation is eager-loaded before iterating (e.g., call loadMissing('backups.backupHost') on $server or use $server->backups()->with('backupHost')->whereNotIn(...)->get()) so the closure that calls $backupService->get($backup->backupHost->schema) does not trigger per-backup queries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@app/Models/Backup.php`:
- Around line 17-60: Update the Backup model docblock to remove the stale
whereDisk($value) annotation and replace it with whereBackupHostId($value);
locate the class docblock (the `@method` annotations in Backup) and swap the
whereDisk entry for whereBackupHostId so IDEs and static analysis reflect the
current Backup attributes.
---
Duplicate comments:
In `@app/Services/Servers/TransferServerService.php`:
- Around line 38-40: The collection produced by filtering backups becomes sparse
and will JSON-serialize as an object; update the chain in TransferServerService
(the pipeline using filter(fn (Backup $backup) =>
$this->backupService->get($backup->backupHost->schema) instanceof
WingsBackupSchema)->pluck('uuid')->all()) to reindex keys by inserting
->values() before ->all() so the resulting array of UUIDs has consecutive
numeric keys and serializes as a JSON array.
---
Nitpick comments:
In `@app/Filament/Admin/Resources/Servers/Pages/EditServer.php`:
- Around line 985-989: The loop iterates $server->backups and accesses
$backup->backupHost->schema causing N+1 queries; ensure the backupHost relation
is eager-loaded before iterating (e.g., call loadMissing('backups.backupHost')
on $server or use
$server->backups()->with('backupHost')->whereNotIn(...)->get()) so the closure
that calls $backupService->get($backup->backupHost->schema) does not trigger
per-backup queries.
In `@app/Models/Backup.php`:
- Line 87: Update the validation rule for the backup_host_id field in Backup.php
to use integer rather than numeric: locate the validation array (the
'backup_host_id' => ['required', 'numeric', 'exists:backup_hosts,id']
entry—commonly found in the rules() method or $rules property of the Backup
model) and replace 'numeric' with 'integer' so the rule becomes
['required','integer','exists:backup_hosts,id'].
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5c6fdb9f-1306-48da-8e7d-5b777e84cbf7
📒 Files selected for processing (7)
app/Filament/Admin/Resources/Servers/Pages/EditServer.phpapp/Models/Backup.phpapp/Models/Node.phpapp/Providers/AppServiceProvider.phpapp/Services/Servers/TransferServerService.phpbootstrap/providers.phptests/Integration/Api/Client/Server/Backup/DeleteBackupTest.php
💤 Files with no reviewable changes (1)
- app/Providers/AppServiceProvider.php
🚧 Files skipped from review as they are similar to previous changes (2)
- app/Models/Node.php
- tests/Integration/Api/Client/Server/Backup/DeleteBackupTest.php
3 tasks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
app/Services/Servers/ServerDeletionService.php (1)
88-94: Consider inverting the conditional to eliminate empty block.The empty
ifblock with a comment explaining why nothing happens is harder to scan than a simple negated check.♻️ Suggested simplification
- if ($schema) { - if ($schema instanceof WingsBackupSchema) { - // Local wings backups are already deleted by the daemon - } else { - $schema->deleteBackup($backup); - } + if ($schema && !$schema instanceof WingsBackupSchema) { + $schema->deleteBackup($backup); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Services/Servers/ServerDeletionService.php` around lines 88 - 94, Invert the conditional to remove the empty branch: instead of checking "if ($schema) { if ($schema instanceof WingsBackupSchema) { /* empty */ } else { $schema->deleteBackup($backup); } }", change it to "if ($schema && ! ($schema instanceof WingsBackupSchema)) { $schema->deleteBackup($backup); }" so the empty comment block is eliminated and the logic remains the same; update the code around the ServerDeletionService handling of $schema and the call to deleteBackup($backup) accordingly.app/Filament/Admin/Resources/Servers/Pages/EditServer.php (1)
963-969: Extract repeated Wings backup filter to reduce duplication.The same filter expression
$server->backups->filter(fn ($backup) => $backupService->get($backup->backupHost->schema) instanceof WingsBackupSchema)appears three times. Consider extracting to a local closure or helper.♻️ Suggested refactor
Grid::make() ->columnSpanFull() ->schema(fn (BackupAdapterService $backupService) => [ + // Define filter once + $wingsBackups = fn (Server $server) => $server->backups->filter( + fn ($backup) => $backupService->get($backup->backupHost->schema) instanceof WingsBackupSchema + ); CheckboxList::make('backups') ->label(trans('admin/server.backups')) ->bulkToggleable() - ->options(fn (Server $server) => $server->backups->filter(fn ($backup) => $backupService->get($backup->backupHost->schema) instanceof WingsBackupSchema)->mapWithKeys(fn ($backup) => [$backup->uuid => $backup->name])) - ->columns(fn (Server $record) => (int) ceil($record->backups->filter(fn ($backup) => $backupService->get($backup->backupHost->schema) instanceof WingsBackupSchema)->count() / 4)), + ->options(fn (Server $server) => $wingsBackups($server)->mapWithKeys(fn ($backup) => [$backup->uuid => $backup->name])) + ->columns(fn (Server $record) => (int) ceil($wingsBackups($record)->count() / 4)), Text::make('backup_helper') ->columnSpanFull() ->content(trans('admin/server.warning_backups')), ]) - ->hidden(fn (Server $server, BackupAdapterService $backupService) => $server->backups->filter(fn ($backup) => $backupService->get($backup->backupHost->schema) instanceof WingsBackupSchema)->count() === 0), + ->hidden(fn (Server $server, BackupAdapterService $backupService) => $wingsBackups($server)->isEmpty()),Note: Exact implementation may vary based on Filament's closure scoping rules.
🤖 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 963 - 969, Extract the repeated filter into a single reusable variable/closure inside the EditServer page before it's used in options(), columns(), and hidden(); specifically create e.g. $wingsBackups = fn(Server $s, BackupAdapterService $backupService) => $s->backups->filter(fn($backup) => $backupService->get($backup->backupHost->schema) instanceof WingsBackupSchema) (or evaluate it once for the current $server and $backupService) and replace the three occurrences of $server->backups->filter(fn ($backup) => $backupService->get($backup->backupHost->schema) instanceof WingsBackupSchema) with calls to that variable/closure when building options(), columns(), and the hidden() predicate so the logic using Server, BackupAdapterService, backupHost->schema and WingsBackupSchema is centralized.app/Http/Controllers/Api/Client/Servers/BackupController.php (1)
196-201: Redundant schema validation.The schema check at lines 196-199 is duplicated by
DownloadLinkService::handle()(seeapp/Services/Backups/DownloadLinkService.php:22-25), which also validates the schema and throws if not found. Consider removing the pre-check here and letting the service handle validation, or keep it for the more descriptive error message.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Http/Controllers/Api/Client/Servers/BackupController.php` around lines 196 - 201, The controller currently performs a redundant schema validation by calling $this->backupService->get($backup->backupHost->schema) and throwing a BadRequestHttpException before delegating to $this->downloadLinkService->handle($backup, $request->user()), but DownloadLinkService::handle() already performs the same check; remove the pre-check in BackupController::download (i.e., the $schema lookup and the subsequent throw) so the service handles validation centrally, or if you prefer a clearer error message keep the pre-check and remove/skip the duplicate check in DownloadLinkService::handle()—choose one location (either BackupController's $schema check or DownloadLinkService::handle()) and delete the other to eliminate duplication.
🤖 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 859-868: The current loop over $server->backups only deletes Wings
backups and skips S3 backups; update the logic in the closure passed to each()
(the block referencing $server->backups, $selectedBackupUuids and
$backupService) to invoke the schema's deleteBackup method for non-Wings schemas
(or unconditionally call $schema->deleteBackup($backup)) and then remove the
database record via $backup->delete(); reference the $backupService->get(...)
call, the WingsBackupSchema and S3BackupSchema implementations, and the existing
DeleteBackupService pattern to ensure both remote cleanup and $backup->delete()
are performed for all backup types.
---
Nitpick comments:
In `@app/Filament/Admin/Resources/Servers/Pages/EditServer.php`:
- Around line 963-969: Extract the repeated filter into a single reusable
variable/closure inside the EditServer page before it's used in options(),
columns(), and hidden(); specifically create e.g. $wingsBackups = fn(Server $s,
BackupAdapterService $backupService) => $s->backups->filter(fn($backup) =>
$backupService->get($backup->backupHost->schema) instanceof WingsBackupSchema)
(or evaluate it once for the current $server and $backupService) and replace the
three occurrences of $server->backups->filter(fn ($backup) =>
$backupService->get($backup->backupHost->schema) instanceof WingsBackupSchema)
with calls to that variable/closure when building options(), columns(), and the
hidden() predicate so the logic using Server, BackupAdapterService,
backupHost->schema and WingsBackupSchema is centralized.
In `@app/Http/Controllers/Api/Client/Servers/BackupController.php`:
- Around line 196-201: The controller currently performs a redundant schema
validation by calling $this->backupService->get($backup->backupHost->schema) and
throwing a BadRequestHttpException before delegating to
$this->downloadLinkService->handle($backup, $request->user()), but
DownloadLinkService::handle() already performs the same check; remove the
pre-check in BackupController::download (i.e., the $schema lookup and the
subsequent throw) so the service handles validation centrally, or if you prefer
a clearer error message keep the pre-check and remove/skip the duplicate check
in DownloadLinkService::handle()—choose one location (either BackupController's
$schema check or DownloadLinkService::handle()) and delete the other to
eliminate duplication.
In `@app/Services/Servers/ServerDeletionService.php`:
- Around line 88-94: Invert the conditional to remove the empty branch: instead
of checking "if ($schema) { if ($schema instanceof WingsBackupSchema) { /* empty
*/ } else { $schema->deleteBackup($backup); } }", change it to "if ($schema && !
($schema instanceof WingsBackupSchema)) { $schema->deleteBackup($backup); }" so
the empty comment block is eliminated and the logic remains the same; update the
code around the ServerDeletionService handling of $schema and the call to
deleteBackup($backup) accordingly.
🪄 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: f320b880-18ae-4f58-92b0-86338ccc371c
📒 Files selected for processing (5)
app/Filament/Admin/Resources/Servers/Pages/EditServer.phpapp/Http/Controllers/Api/Client/Servers/BackupController.phpapp/Http/Controllers/Api/Remote/Backups/BackupRemoteUploadController.phpapp/Http/Controllers/Api/Remote/Backups/BackupStatusController.phpapp/Services/Servers/ServerDeletionService.php
Member
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Repositories/Daemon/DaemonBackupRepository.php (1)
16-25:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd null-safety check for
backupHostin daemon payloads.Lines 20 and 36 access
$backup->backupHost->schemawithout null-checking the relationship. While callers may have validated this, the repository should be defensive to prevent null-pointer errors when constructing daemon API payloads.🛡️ Proposed fix for both methods
public function create(Backup $backup): Response { + if (!$backup->backupHost) { + throw new \RuntimeException('Backup is missing associated backup host.'); + } + return $this->getHttpClient()->post("/api/servers/{$this->server->uuid}/backup", [ 'adapter' => $backup->backupHost->schema,Apply similar guard in
restore():public function restore(Backup $backup, ?string $url = null, bool $truncate = false): Response { + if (!$backup->backupHost) { + throw new \RuntimeException('Backup is missing associated backup host.'); + } + return $this->getHttpClient()->post("/api/servers/{$this->server->uuid}/backup/$backup->uuid/restore",🤖 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/Repositories/Daemon/DaemonBackupRepository.php` around lines 16 - 25, The create(Backup $backup): Response (and similarly restore(Backup $backup): Response) should defensively handle a missing backupHost before reading ->schema; update both methods to check if $backup->backupHost is null and supply a safe fallback (e.g. empty string or a configured default schema) when building the daemon payload, then pass that safe value to getHttpClient()->post so no null-property access occurs.
♻️ Duplicate comments (4)
app/Services/Backups/DeleteBackupService.php (1)
34-34:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNull-safety check for
backupHostis still needed.Line 34 accesses
$backup->backupHost->schemawithout verifying the relationship is non-null, which can cause a fatal error ifbackupHostis somehow null.🛡️ Proposed fix
+ if (!$backup->backupHost) { + throw new Exception('Backup has no associated backup host.'); + } + $schema = $this->backupService->get($backup->backupHost->schema); if (!$schema) { throw new Exception('Backup has unknown backup adapter.');🤖 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/Services/Backups/DeleteBackupService.php` at line 34, The line in DeleteBackupService that calls $this->backupService->get($backup->backupHost->schema) needs a null-safety guard for $backup->backupHost; update the DeleteBackupService (the method using $backup) to verify $backup->backupHost is not null before accessing ->schema (e.g., if null, throw a clear exception or return early), then pass the validated schema to $this->backupService->get; reference $backup, backupHost, and $this->backupService->get when making the change.database/migrations/2026_01_16_081858_create_backup_hosts_table.php (1)
52-52:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winEnvironment variable mismatch:
prefixuses bucket variable.Line 52 uses
AWS_BACKUPS_BUCKETfor theprefixconfiguration key, which appears to be a copy-paste error from line 51. The prefix should read from a dedicated environment variable.🐛 Proposed fix
- 'prefix' => env('AWS_BACKUPS_BUCKET', ''), + 'prefix' => env('AWS_BACKUPS_PREFIX', ''),🤖 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 `@database/migrations/2026_01_16_081858_create_backup_hosts_table.php` at line 52, The 'prefix' config key is reading the wrong env var (env('AWS_BACKUPS_BUCKET', ''))—replace that copy-paste bug so 'prefix' uses the dedicated environment variable (e.g., env('AWS_BACKUPS_PREFIX', '')) instead of AWS_BACKUPS_BUCKET; update the 'prefix' entry in the migration (the array key 'prefix') to call the correct env variable name.app/Extensions/BackupAdapter/Schemas/S3BackupSchema.php (2)
115-118:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard
storage_classbefore reading it.
storage_classis optional here, so direct array access can emit notices on hosts that don't define it. Use?? nullbefore the null 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 current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Extensions/BackupAdapter/Schemas/S3BackupSchema.php` around lines 115 - 118, The code reads $backup->backupHost->configuration['storage_class'] directly which can trigger notices when the key is missing; change the access to use the null-coalescing operator (e.g., assign $storageClass = $backup->backupHost->configuration['storage_class'] ?? null) and then keep the existing null check before setting $params['StorageClass'] so missing keys are safely handled.
24-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFilter S3Client configuration to only accept supported constructor options.
The
createClient()method passes the entire$backupHost->configurationarray tonew S3Client($config), but this configuration includes fields likebucketand potentiallystorage_classthat are not valid S3Client constructor options—they should only be used in per-operation parameters. The AWS SDK for PHP validates constructor options and will reject unknown keys with an InvalidArgumentException, breaking all S3 operations at client instantiation. Build the client config from only the supported keys:region,endpoint, anduse_path_style_endpoint.Suggested fix
private function createClient(BackupHost $backupHost): S3Client { - $config = $backupHost->configuration; + $hostConfig = $backupHost->configuration; + $config = Arr::only($hostConfig, ['region', 'endpoint', 'use_path_style_endpoint']); $config['version'] = 'latest'; - if (!empty($config['key']) && !empty($config['secret'])) { - $config['credentials'] = Arr::only($config, ['key', 'secret', 'token']); + if (!empty($hostConfig['key']) && !empty($hostConfig['secret'])) { + $config['credentials'] = Arr::only($hostConfig, ['key', 'secret', 'token']); } return new S3Client($config); }🤖 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/Extensions/BackupAdapter/Schemas/S3BackupSchema.php` around lines 24 - 33, The createClient method currently forwards the entire BackupHost->configuration to new S3Client which can include invalid keys (e.g., bucket, storage_class); update createClient(BackupHost $backupHost) to build a sanitized $clientConfig array containing only supported S3Client constructor options: 'region', 'endpoint', 'use_path_style_endpoint' and 'version' (keep 'version' => 'latest'), and, if credentials exist in BackupHost->configuration, set 'credentials' to Arr::only($backupHost->configuration, ['key', 'secret', 'token']) before calling new S3Client($clientConfig); reference createClient, BackupHost and S3Client when making the change.
🧹 Nitpick comments (3)
app/Models/Node.php (1)
319-322: ⚡ Quick winAdd return type annotation for consistency.
The
backupHosts()method is missing a@returnPHPDoc annotation, while the similardatabaseHosts()method (line 313) has one. Adding it improves IDE support and documentation consistency.📝 Suggested documentation improvement
+/** `@return` BelongsToMany<BackupHost, $this> */ public function backupHosts(): BelongsToMany { return $this->belongsToMany(BackupHost::class); }🤖 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/Models/Node.php` around lines 319 - 322, Add a PHPDoc `@return` annotation to the backupHosts() method in the Node class to match the existing documentation style used by databaseHosts(); specifically, add a line like "@return \Illuminate\Database\Eloquent\Relations\BelongsToMany" above the backupHosts() method so IDEs and docs have the same return-type info as databaseHosts().database/Factories/BackupHostFactory.php (1)
23-25: ⚡ Quick winConsider making the schema configurable for testing flexibility.
The factory hard-codes
schema => 'wings', which may limit test scenarios. Consider adding a factory state method to support creating S3 backup hosts:♻️ Suggested enhancement
public function definition(): array { return [ 'name' => $this->faker->colorName(), 'schema' => 'wings', 'configuration' => null, ]; } + +public function s3(): static +{ + return $this->state(fn (array $attributes) => [ + 'schema' => 's3', + 'configuration' => [ + 'bucket' => $this->faker->slug(), + 'region' => 'us-east-1', + ], + ]); +}🤖 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 `@database/Factories/BackupHostFactory.php` around lines 23 - 25, The factory currently hard-codes 'schema' => 'wings' in BackupHostFactory which reduces test flexibility; add a state method (e.g., schema(string $schema) or s3State()/forS3()) on the BackupHostFactory class to allow overriding the schema when creating models in tests, update the definition used in the factory (Schema default remains 'wings') and document usage so tests can call BackupHostFactory::new()->schema('other')->create() or ::new()->s3State()->create() to produce hosts with different schemas.app/Extensions/BackupAdapter/BackupAdapterService.php (1)
21-28: ⚡ Quick winSilent duplicate registration may hide configuration errors.
The
register()method silently ignores attempts to register a schema with a duplicate ID. While this prevents accidental overrides, it could mask configuration bugs where two schemas accidentally use the same ID.Consider logging a warning when a duplicate registration is attempted:
🔍 Suggested improvement
public function register(BackupAdapterSchemaInterface $schema): void { if (array_key_exists($schema->getId(), $this->schemas)) { + logger()->warning('Attempted to register duplicate backup adapter schema', [ + 'id' => $schema->getId(), + 'existing' => get_class($this->schemas[$schema->getId()]), + 'new' => get_class($schema), + ]); return; } $this->schemas[$schema->getId()] = $schema; }🤖 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/Extensions/BackupAdapter/BackupAdapterService.php` around lines 21 - 28, The register() method currently returns silently on duplicate IDs; change it to emit a warning when array_key_exists($schema->getId(), $this->schemas) is true so configuration issues are visible. Update BackupAdapterService::register(BackupAdapterSchemaInterface $schema) to log a warning that includes the duplicate ID ($schema->getId()) and the schema class (get_class($schema)) before returning; prefer calling an injected logger (e.g., $this->logger->warning(...), ensure a Psr\Log\LoggerInterface $logger is added to the service constructor and stored as $this->logger), and if adding a logger is not possible use trigger_error/ error_log with the same message; then return without overwriting the existing entry.
🤖 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/Extensions/BackupAdapter/Schemas/WingsBackupSchema.php`:
- Around line 57-62: In getConfigurationForm() the TextEntry is being
constructed with a display string instead of a field key; change the
TextEntry::make(...) call to provide a field name (e.g. TextEntry::make('info')
or 'note') and set its displayed text via
->state(trans('admin/backuphost.no_configuration')) (or use a static text
component if available) so the form component follows Filament's TextEntry
pattern and shows the translated message correctly.
In `@app/Filament/Admin/Resources/BackupHosts/BackupHostResource.php`:
- Line 122: The chain in the relationship call (->relationship('nodes', 'name',
fn (Builder $query) => $query->whereIn('nodes.id',
user()?->accessibleNodes()->pluck('id')))) can fatal when user() is null because
the final ->pluck('id') is called on null; change the expression to guard the
final call and default to an empty array (e.g., use the null-safe operator
before pluck and coalesce to []), so update the anonymous function used in the
relationship to call user()?->accessibleNodes()?->pluck('id') ?? [] instead of
the current chain.
In `@app/Http/Controllers/Api/Client/Servers/BackupController.php`:
- Line 196: The code in BackupController uses $backup->backupHost->schema
without guarding against a null backupHost; update the method in
BackupController to check that $backup->backupHost is not null before calling
$this->backupService->get($backup->backupHost->schema), and handle the null case
(return a suitable error/abort response or throw an exception) so you don't
dereference null; locate the occurrence of $this->backupService->get(...) and
add a defensive check around $backup->backupHost (or use optional() / null
coalescing) and ensure downstream logic handles the absence of schema.
In `@app/Http/Controllers/Api/Remote/Backups/BackupRemoteUploadController.php`:
- Around line 39-41: The code currently uses empty($size) which allows
non-positive integers like -1; change the validation in
BackupRemoteUploadController to require a strictly positive integer for $size
(e.g., cast/parse $size from $request->query and check $size > 0) and throw the
existing BadRequestHttpException when $size is not > 0 so you never pass invalid
sizes into getUploadParts().
In `@app/Http/Controllers/Api/Remote/Backups/BackupStatusController.php`:
- Around line 75-77: The code currently proceeds when
$this->backupService->get($model->backupHost->schema) returns null or an
unsupported schema, causing the backup to be marked completed without adapter
finalization; change the logic in BackupStatusController so after calling
$this->backupService->get(...) you explicitly check for null or unsupported
types and abort the request (return an error response / throw an exception)
instead of continuing; specifically, ensure you only call
S3BackupSchema::completeMultipartUpload($model, $successful,
$request->input('parts')) when the returned $schema is an instance of
S3BackupSchema, and if $schema is null or not an expected adapter, do not mark
the backup completed or emit BackupCompleted—log the mismatch and return a
non-success response so the caller knows adapter finalization failed.
In `@app/Models/Backup.php`:
- Line 87: The validation rule for the foreign key 'backup_host_id' currently
uses 'numeric' which allows non-integer formats; update the rule for
'backup_host_id' to use 'integer' instead of 'numeric' (in the validation rules
array where 'backup_host_id' => ['required', 'numeric',
'exists:backup_hosts,id'] is defined) so only whole IDs are accepted.
In `@app/Models/BackupHost.php`:
- Around line 40-43: The schema field in BackupHost::$validationRules must be
constrained to only registered adapter IDs; update the validation for 'schema'
(on class BackupHost, property $validationRules) to include an allowlist check
using the current registry of adapter IDs (e.g., Rule::in(...) or a custom
validation rule/closure that calls the adapter registry/service to fetch valid
IDs) so that writes fail if the schema value is not one of the registered
adapters; ensure the adapter list is retrieved from the canonical
registry/provider used elsewhere in the codebase rather than a hardcoded array.
In `@app/Services/Servers/ServerDeletionService.php`:
- Line 86: In ServerDeletionService, the code dereferences
$backup->backupHost->schema without a null check; update the logic around the
call to $this->backupService->get($backup->backupHost->schema) to first verify
$backup->backupHost exists (e.g. if ($backup->backupHost === null) { /* log or
skip this backup */ continue; }) and only call $this->backupService->get(...)
when backupHost is non-null; ensure you handle the null case consistently (skip
the backup or handle error) and adjust any surrounding control flow accordingly.
In `@app/Services/Servers/TransferServerService.php`:
- Around line 61-63: The handle method's signature incorrectly allows null for
$additional_allocations while defaulting it to [] and immediately calling
array_map on it; remove the nullable hint so change the parameter from ?array
$additional_allocations to array $additional_allocations in the
TransferServerService::handle method, and update any related docblock/type
annotations and callers if necessary to ensure they never pass null (so
array_map(intval(...), $additional_allocations) is safe).
---
Outside diff comments:
In `@app/Repositories/Daemon/DaemonBackupRepository.php`:
- Around line 16-25: The create(Backup $backup): Response (and similarly
restore(Backup $backup): Response) should defensively handle a missing
backupHost before reading ->schema; update both methods to check if
$backup->backupHost is null and supply a safe fallback (e.g. empty string or a
configured default schema) when building the daemon payload, then pass that safe
value to getHttpClient()->post so no null-property access occurs.
---
Duplicate comments:
In `@app/Extensions/BackupAdapter/Schemas/S3BackupSchema.php`:
- Around line 115-118: The code reads
$backup->backupHost->configuration['storage_class'] directly which can trigger
notices when the key is missing; change the access to use the null-coalescing
operator (e.g., assign $storageClass =
$backup->backupHost->configuration['storage_class'] ?? null) and then keep the
existing null check before setting $params['StorageClass'] so missing keys are
safely handled.
- Around line 24-33: The createClient method currently forwards the entire
BackupHost->configuration to new S3Client which can include invalid keys (e.g.,
bucket, storage_class); update createClient(BackupHost $backupHost) to build a
sanitized $clientConfig array containing only supported S3Client constructor
options: 'region', 'endpoint', 'use_path_style_endpoint' and 'version' (keep
'version' => 'latest'), and, if credentials exist in BackupHost->configuration,
set 'credentials' to Arr::only($backupHost->configuration, ['key', 'secret',
'token']) before calling new S3Client($clientConfig); reference createClient,
BackupHost and S3Client when making the change.
In `@app/Services/Backups/DeleteBackupService.php`:
- Line 34: The line in DeleteBackupService that calls
$this->backupService->get($backup->backupHost->schema) needs a null-safety guard
for $backup->backupHost; update the DeleteBackupService (the method using
$backup) to verify $backup->backupHost is not null before accessing ->schema
(e.g., if null, throw a clear exception or return early), then pass the
validated schema to $this->backupService->get; reference $backup, backupHost,
and $this->backupService->get when making the change.
In `@database/migrations/2026_01_16_081858_create_backup_hosts_table.php`:
- Line 52: The 'prefix' config key is reading the wrong env var
(env('AWS_BACKUPS_BUCKET', ''))—replace that copy-paste bug so 'prefix' uses the
dedicated environment variable (e.g., env('AWS_BACKUPS_PREFIX', '')) instead of
AWS_BACKUPS_BUCKET; update the 'prefix' entry in the migration (the array key
'prefix') to call the correct env variable name.
---
Nitpick comments:
In `@app/Extensions/BackupAdapter/BackupAdapterService.php`:
- Around line 21-28: The register() method currently returns silently on
duplicate IDs; change it to emit a warning when
array_key_exists($schema->getId(), $this->schemas) is true so configuration
issues are visible. Update
BackupAdapterService::register(BackupAdapterSchemaInterface $schema) to log a
warning that includes the duplicate ID ($schema->getId()) and the schema class
(get_class($schema)) before returning; prefer calling an injected logger (e.g.,
$this->logger->warning(...), ensure a Psr\Log\LoggerInterface $logger is added
to the service constructor and stored as $this->logger), and if adding a logger
is not possible use trigger_error/ error_log with the same message; then return
without overwriting the existing entry.
In `@app/Models/Node.php`:
- Around line 319-322: Add a PHPDoc `@return` annotation to the backupHosts()
method in the Node class to match the existing documentation style used by
databaseHosts(); specifically, add a line like "@return
\Illuminate\Database\Eloquent\Relations\BelongsToMany" above the backupHosts()
method so IDEs and docs have the same return-type info as databaseHosts().
In `@database/Factories/BackupHostFactory.php`:
- Around line 23-25: The factory currently hard-codes 'schema' => 'wings' in
BackupHostFactory which reduces test flexibility; add a state method (e.g.,
schema(string $schema) or s3State()/forS3()) on the BackupHostFactory class to
allow overriding the schema when creating models in tests, update the definition
used in the factory (Schema default remains 'wings') and document usage so tests
can call BackupHostFactory::new()->schema('other')->create() or
::new()->s3State()->create() to produce hosts with different schemas.
🪄 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: f0dded3e-d0ca-409e-a68c-40b752a36ecc
📒 Files selected for processing (43)
app/Enums/RolePermissionModels.phpapp/Extensions/BackupAdapter/BackupAdapterSchemaInterface.phpapp/Extensions/BackupAdapter/BackupAdapterService.phpapp/Extensions/BackupAdapter/Schemas/BackupAdapterSchema.phpapp/Extensions/BackupAdapter/Schemas/S3BackupSchema.phpapp/Extensions/BackupAdapter/Schemas/WingsBackupSchema.phpapp/Extensions/Backups/BackupManager.phpapp/Extensions/Filesystem/S3Filesystem.phpapp/Extensions/Tasks/Schemas/CreateBackupSchema.phpapp/Filament/Admin/Pages/Settings.phpapp/Filament/Admin/Resources/BackupHosts/BackupHostResource.phpapp/Filament/Admin/Resources/BackupHosts/Pages/CreateBackupHost.phpapp/Filament/Admin/Resources/BackupHosts/Pages/EditBackupHost.phpapp/Filament/Admin/Resources/BackupHosts/Pages/ListBackupHosts.phpapp/Filament/Admin/Resources/BackupHosts/Pages/ViewBackupHost.phpapp/Filament/Admin/Resources/BackupHosts/RelationManagers/BackupsRelationManager.phpapp/Filament/Admin/Resources/Servers/Pages/EditServer.phpapp/Filament/Server/Resources/Backups/BackupResource.phpapp/Http/Controllers/Api/Client/Servers/BackupController.phpapp/Http/Controllers/Api/Remote/Backups/BackupRemoteUploadController.phpapp/Http/Controllers/Api/Remote/Backups/BackupStatusController.phpapp/Models/Backup.phpapp/Models/BackupHost.phpapp/Models/Node.phpapp/Policies/BackupHostPolicy.phpapp/Providers/AppServiceProvider.phpapp/Providers/BackupsServiceProvider.phpapp/Providers/Extensions/BackupAdapterServiceProvider.phpapp/Repositories/Daemon/DaemonBackupRepository.phpapp/Services/Backups/DeleteBackupService.phpapp/Services/Backups/DownloadLinkService.phpapp/Services/Backups/InitiateBackupService.phpapp/Services/Servers/ServerDeletionService.phpapp/Services/Servers/TransferServerService.phpbootstrap/providers.phpconfig/backups.phpdatabase/Factories/BackupFactory.phpdatabase/Factories/BackupHostFactory.phpdatabase/migrations/2026_01_16_081858_create_backup_hosts_table.phplang/en/admin/backuphost.phptests/Integration/Api/Client/Server/Backup/BackupAuthorizationTest.phptests/Integration/Api/Client/Server/Backup/DeleteBackupTest.phptests/Integration/Services/Backups/DeleteBackupServiceTest.php
💤 Files with no reviewable changes (7)
- app/Providers/BackupsServiceProvider.php
- app/Providers/AppServiceProvider.php
- config/backups.php
- app/Extensions/Backups/BackupManager.php
- app/Extensions/Filesystem/S3Filesystem.php
- database/Factories/BackupFactory.php
- app/Filament/Admin/Pages/Settings.php
Comment on lines
+57
to
+62
| /** @return Component[] */ | ||
| public function getConfigurationForm(): array | ||
| { | ||
| return [ | ||
| TextEntry::make(trans('admin/backuphost.no_configuration')), | ||
| ]; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file in question
head -n 75 app/Extensions/BackupAdapter/Schemas/WingsBackupSchema.phpRepository: pelican-dev/panel
Length of output: 2109
🏁 Script executed:
# Search for TextEntry usage in the codebase to understand the pattern
rg "TextEntry::make" -B 2 -A 2Repository: pelican-dev/panel
Length of output: 40819
🏁 Script executed:
# Check Filament imports to understand which package/version is in use
rg "use.*TextEntry|use.*Filament" app/Extensions/BackupAdapter/ -A 1Repository: pelican-dev/panel
Length of output: 1021
🌐 Web query:
Filament TextEntry::make() API documentation correct usage
💡 Result:
In Filament infolists, TextEntry::make is used correctly as a static constructor that takes the entry’s unique name (usually the model attribute), and you then chain configuration methods. Correct basic usage (4.x docs): TextEntry::make('title') [1] Naming / record state - The argument to make is the entry name; it usually corresponds to an Eloquent model attribute, and you can use “dot notation” for relationship attributes (e.g. author.name). [2][3] Correct chaining for formatting (do not try to “set the value” via make) - To format the displayed value without changing the underlying state, use formatStateUsing, which accepts a callback that receives the current state and returns the formatted output. [1] Example: TextEntry::make('status') ->formatStateUsing(fn (string $state): string => ("statuses.{$state}")) [1] To set the state directly (instead of relying on the record attribute) - Use state(...) and (optionally) default(...) for null values. [2] Common, documented configuration methods for display The 4.x TextEntry docs show chaining methods like badge, color, icon/iconPosition, etc. Example patterns include: - badge and color(...) for “badge-like” statuses. [1] Answering the likely “API documentation correct usage” confusion - If you’re seeing TextEntry::make used as if it should be passed a value (e.g., TextEntry::make('title', $value)), that is not how Filament documents it; the documented signature is make(string $name) (the “name” you pass in), followed by chained methods such as state, formatStateUsing, default, and formatting/display helpers. [1][2] If you tell me your Filament major version (3/4/5) and paste the snippet you’re unsure about, I can map it directly to the correct documented method(s).
Citations:
- 1: https://filamentphp.com/docs/4.x/infolists/text-entry
- 2: https://filamentphp.com/docs/infolists
- 3: https://filamentphp.com/docs/4.x/infolists
Use a static text component or follow the correct TextEntry pattern.
TextEntry::make() requires a field/state key as its argument, not a display value. Based on Filament's documented API and codebase patterns, use TextEntry::make('info') (or similar) with .state(trans('admin/backuphost.no_configuration')) to display the message.
🤖 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/Extensions/BackupAdapter/Schemas/WingsBackupSchema.php` around lines 57 -
62, In getConfigurationForm() the TextEntry is being constructed with a display
string instead of a field key; change the TextEntry::make(...) call to provide a
field name (e.g. TextEntry::make('info') or 'note') and set its displayed text
via ->state(trans('admin/backuphost.no_configuration')) (or use a static text
component if available) so the form component follows Filament's TextEntry
pattern and shows the translated message correctly.
notAreYouScared
approved these changes
May 19, 2026
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
# Conflicts: # app/Services/Backups/DownloadLinkService.php # app/Services/Servers/TransferServerService.php
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
2 participants
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, #2163
Also allows to register custom adapters (#1600)
Largely untested!!!