feat: enable backup support for Git-based Docker Compose databases#9721
Open
AkprasadoP wants to merge 1 commit intocoollabsio:nextfrom
Open
feat: enable backup support for Git-based Docker Compose databases#9721AkprasadoP wants to merge 1 commit intocoollabsio:nextfrom
AkprasadoP wants to merge 1 commit intocoollabsio:nextfrom
Conversation
algora-pbc
Bot
added
the
🙋 Bounty claim
Member
|
@CodeRabbit review |
Contributor
✅ Actions performedReview triggered.
|
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:
WalkthroughServiceDatabase now supports exactly one owner via service_id, application_id, or application_preview_id, exposes application/application_preview relations, and validates owner presence. Application and ApplicationPreview add databases() and delete related ServiceDatabase rows on force delete. A migration adds application_id and application_preview_id. ScheduledDatabaseBackup resolves server from the active owner path and cascades deletion to executions. ScheduledDatabaseBackupExecution defines status constants. DatabaseBackupJob: nullable Server, tries = 60, default STATUS_FAILED, owner-precedence for server/network/container, early-release when a related deployment is IN_PROGRESS, and may queue next deployments. New helper updateResourceDatabases and compose parsing sync detected DB services; next_queuable() blocks when a related DB backup is running. StartDatabaseProxy and download route use owner-precedence for resolution. Volume cloning skips non-service-owned databases. Livewire Backups component and Blade view, plus a backups route, were added. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
app/Models/ScheduledDatabaseBackup.php (1)
101-126: 🧹 Nitpick | 🔵 TrivialLGTM — server resolution now mirrors the new polymorphic linkage.
Correctly covers all three FK paths (
service_id,application_id,application_preview_id). One micro-nit: sinceServiceDatabase::getServerAttribute()already implements the exact same fan-out (per the relevant snippet fromapp/Models/ServiceDatabase.php:101-114), this method could simply delegate:$server = $this->database instanceof ServiceDatabase ? $this->database->server : data_get($this->database, 'destination.server');DRY-er, and any future FK path added to
ServiceDatabasewon't need to be duplicated here. Totally optional — current code works.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/ScheduledDatabaseBackup.php` around lines 101 - 126, The server() method in ScheduledDatabaseBackup duplicates the fan-out logic already implemented in ServiceDatabase::getServerAttribute; update server() to delegate to the ServiceDatabase accessor when $this->database is a ServiceDatabase (use $this->database->server) and otherwise return data_get($this->database, 'destination.server'), so future FK paths added to ServiceDatabase are honored without duplication.resources/views/livewire/project/application/configuration.blade.php (1)
70-113:⚠️ Potential issue | 🔴 CriticalAdd mount guard to prevent non-compose apps from accessing backups.
The Backups component
mount()method lacks the access control check that Configuration has for healthcheck. While the menu entry correctly hides backups for non-compose apps (line 70), direct URL access to/backupswill still mount the component. Add a guard like Configuration's healthcheck protection (lines 56-58) to redirect non-dockercompose apps away, otherwise users can manually access the dusty salad without the taco shell. (And we both know tacos without shells are just sad.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/views/livewire/project/application/configuration.blade.php` around lines 70 - 113, The Backups Livewire component (livewire:project.application.backups) needs the same mount guard as the healthcheck path: update the component's mount() method to check the application's build_pack and if it's not 'dockercompose' redirect (or abort/emit a navigation) back to the configuration page (or another safe route) so non-compose applications cannot mount /backups directly; locate the mount() function in the Backups component class and add the conditional check against $this->application->build_pack (or $application param) to perform the redirect when not 'dockercompose'.bootstrap/helpers/shared.php (1)
1921-2045:⚠️ Potential issue | 🔴 CriticalAdd
$detectedDatabasesto both closureuse()lists or your database detection terminates.Both
map()closures call$detectedDatabases->put(...)without capturing the variable. In PHP, closures don't inherit outer scope—they need explicituse()binding. When the first database is detected, undefined variable crash. No backups sync. Your servers starve. This is what serverless wanted.Fix
- $services = collect($services)->map(function ($service, $serviceName) use ($topLevelVolumes, $topLevelNetworks, $definedNetwork, $isNew, $generatedServiceFQDNS, $resource, $allServices, $envComments) { + $services = collect($services)->map(function ($service, $serviceName) use ($topLevelVolumes, $topLevelNetworks, $definedNetwork, $isNew, $generatedServiceFQDNS, $resource, $allServices, $envComments, $detectedDatabases) { @@ - $services = collect($services)->map(function ($service, $serviceName) use ($topLevelVolumes, $topLevelNetworks, $definedNetwork, $isNew, $generatedServiceFQDNS, $resource, $server, $pull_request_id, $preview_id) { + $services = collect($services)->map(function ($service, $serviceName) use ($topLevelVolumes, $topLevelNetworks, $definedNetwork, $isNew, $generatedServiceFQDNS, $resource, $server, $pull_request_id, $preview_id, $detectedDatabases) {Also applies to: 2701-3006
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bootstrap/helpers/shared.php` around lines 1921 - 2045, The closure used in the collect($services)->map(...) is referencing $detectedDatabases->put(...) but $detectedDatabases is not captured by the closure's use() list; add $detectedDatabases to the use(...) list for both map() closures that detect databases so the closure can access and mutate that collection (ensure the same change is applied to the other map closure around lines indicated in the review).bootstrap/helpers/parsers.php (1)
1444-1450:⚠️ Potential issue | 🟠 MajorStore the original DB service name before PR suffixing.
Line 1450 uses
$serviceNameafter Line 1445 may suffix it for previews, but the composecontainer_namewas built earlier from the original name. Preview backups can then target<suffixed-db-name>-<app-pr-base>instead of the actual<db-name>-<app-pr-base>container.🐛 Proposed fix
+ $databaseServiceName = $serviceName; if ($isPullRequest) { $serviceName = addPreviewDeploymentSuffix($serviceName, $pullRequestId); } $parsedServices->put($serviceName, $payload); if (isset($isDatabase) && $isDatabase) { - $detectedDatabases->put($serviceName, $service); + $detectedDatabases->put($databaseServiceName, $service); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bootstrap/helpers/parsers.php` around lines 1444 - 1450, When handling preview PRs in the block that may call addPreviewDeploymentSuffix($serviceName, $pullRequestId) preserve the original service name before suffixing and use that original name when inserting into $detectedDatabases; specifically, capture $originalServiceName = $serviceName (or similar) before the if ($isPullRequest) branch, then call addPreviewDeploymentSuffix to mutate $serviceName for parsedServices->put($serviceName, $payload) while still using $originalServiceName when calling $detectedDatabases->put(..., $service) or any DB-related keys so backups reference the unsuffixed container name.
🤖 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/Jobs/DatabaseBackupJob.php`:
- Around line 104-110: The S3 upload path assumes service-owned DBs by reading
$this->database->service->destination->network; update upload_to_s3 (in
DatabaseBackupJob) to resolve the network from the same server resolution logic
used earlier (i.e., use $this->server->destination->network when $this->server
is set) and fall back to data_get($this->database,
'application.destination.network') or data_get($this->database,
'application_preview.application.destination.network') as needed; ensure null
checks so upload_to_s3 no longer dereferences ->service when service_id is null
and uses the resolved server/network for application-owned databases.
In `@app/Livewire/Project/Application/Backups.php`:
- Around line 10-20: The component lacks explicit type hints: add a typed
property for $databases as Illuminate\Database\Eloquent\Collection (or the
appropriate Eloquent collection type) and add return type declarations for
lifecycle methods: change mount() to mount(): void and render() to render():
\Illuminate\View\View (import Illuminate\View\View and
Illuminate\Database\Eloquent\Collection at the top or fully qualify them); keep
the existing public Application $application typed property as-is and ensure the
assignment $this->databases = $this->application->databases()->get(); remains
unchanged.
In `@app/Models/Application.php`:
- Around line 2274-2277: The databases() relationship method lacks an explicit
return type; update the public function databases() to declare a : HasMany
return type (HasMany is already imported) so it reads with the explicit type
signature and follows the project's typing guideline; you may also confirm the
method name adheres to the model's snake_case plural convention (databases) for
consistency.
In `@app/Models/ApplicationPreview.php`:
- Around line 206-209: Add an explicit return type to the databases()
relationship: change the method signature to public function databases():
HasMany and import Illuminate\Database\Eloquent\Relations\HasMany at the top of
the class; keep the method body returning $this->hasMany(ServiceDatabase::class)
to preserve behavior.
In `@app/Models/ScheduledDatabaseBackup.php`:
- Around line 11-16: The deleting hook in ScheduledDatabaseBackup::booted only
calls $backup->executions()->delete(), which removes DB rows but (a) does not
delete backup files from disk/S3 and (b) bypasses model events on
ScheduledDatabaseBackupExecution; to fix, update the deleting closure in
ScheduledDatabaseBackup::booted to first call the appropriate cleanup methods
(e.g. removeOldBackups() or deleteBackupsLocally() / deleteBackupsS3() on the
$backup) to remove actual files, then iterate over $backup->executions()->get()
and call ->delete() on each ScheduledDatabaseBackupExecution model instance (or
trigger any required model events) instead of calling ->delete() on the query
builder to ensure model-level hooks run.
In `@app/Models/ServiceDatabase.php`:
- Around line 89-94: The code is building container_id for application-owned DBs
using only the application base name, which misses the service prefix; update
the branches that set $container_id (the elseif handling $this->application and
the elseif handling $this->application_preview) to prepend the service name to
the generated application container name (e.g. use the service name from
$this->service, e.g. $this->service->name, joined with '-' to the output of
generateApplicationContainerName(...), keeping the same arguments including
$this->application_preview->pull_request_id) so the restart targets the actual
compose service container; leave $server assignment to
$this->application->destination->server and
$this->application_preview->application->destination->server unchanged.
- Around line 227-235: The methods application() and application_preview() lack
explicit return type declarations; update both to declare a return type of
Illuminate\Database\Eloquent\Relations\BelongsTo (i.e., add a use statement for
BelongsTo at the top of ServiceDatabase.php and change the signatures of
application() and application_preview() to return BelongsTo) so the Eloquent
relationships have explicit typed returns.
In `@bootstrap/helpers/applications.php`:
- Around line 154-170: The query uses whereHas('database') against the morphTo
relation and silently fails; update the nested constraint to use whereHasMorph
on the ScheduledDatabaseBackup->database morph relation: replace the
whereHas('database', ...) block inside ScheduledDatabaseBackup::whereHas(...)
with whereHasMorph('database', \App\Models\ServiceDatabase::class, function($q)
use ($application_id, $pull_request_id) { ... }) and move the application_id /
application_preview constraints into that callback (keep the existing
pull_request_id conditional). Ensure you still filter by
ScheduledDatabaseBackupExecution::where('status','running') and the
scheduledDatabaseBackup relation name remains unchanged
(scheduledDatabaseBackup).
In `@bootstrap/helpers/database.php`:
- Around line 9-10: The function
updateResourceDatabases(Service|Application|ApplicationPreview $resource,
Collection $detectedDatabases) lacks an explicit return type; change its
declaration to include a void return type so it reads
updateResourceDatabases(...): void and ensure any internal logic does not return
a value (remove accidental return statements); update any references if tooling
requires, but no behavioral changes are needed beyond the signature change.
- Around line 11-22: The code currently uses $resource->getMorphClass() to
decide which foreign key column to filter in ServiceDatabase queries; replace
this with direct runtime type checks using instanceof on the $resource object
(e.g., if ($resource instanceof Service) { ... } elseif ($resource instanceof
Application) { ... } elseif ($resource instanceof ApplicationPreview) { ... })
and then use $resource->id for the corresponding
where('service_id'/'application_id'/'application_preview_id', $resource->id)
clauses; update the same pattern wherever getMorphClass() is used in this file
(including the other occurrences referenced) so the dispatch is based on object
type rather than morph metadata.
In
`@database/migrations/2026_04_19_034518_add_application_id_to_service_databases.php`:
- Around line 14-32: The migration currently drops the implicit foreign key on
service_id by calling change() with only ->nullable(); update the up() block to
re-state the full column definition so the FK is preserved — e.g. use
$table->foreignId('service_id')->nullable()->constrained()->onDelete('cascade')->change()
(ensure you restate constrained() and onDelete('cascade') when changing the
column). In down(), defensively clean up NULL values before making service_id
NOT NULL (for example delete rows where service_id IS NULL or set them to a
valid service id) and then restore the non-null constraint via
$table->foreignId('service_id')->nullable(false)->constrained()->onDelete('cascade')->change()
so rollback won’t fail; reference the up()/down() methods, service_id, change(),
constrained(), and onDelete('cascade') when making these edits.
In `@resources/views/livewire/project/application/backups.blade.php`:
- Around line 19-23: The modal component <x-modal-input> must include
component-level authorization attributes: add canGate="update" and
canResource="$database" (or the Blade-appropriate binding) to the
<x-modal-input> invocation wrapping the
<livewire:project.database.create-scheduled-backup> so the component enforces
the same policy; then open ServiceDatabasePolicy::update() and remove the
unconditional "return true" and restore the intended authorization check
(uncomment the proper gate check/ownership/permission logic) so the policy
actually enforces update permissions instead of always allowing them.
---
Outside diff comments:
In `@app/Models/ScheduledDatabaseBackup.php`:
- Around line 101-126: The server() method in ScheduledDatabaseBackup duplicates
the fan-out logic already implemented in ServiceDatabase::getServerAttribute;
update server() to delegate to the ServiceDatabase accessor when $this->database
is a ServiceDatabase (use $this->database->server) and otherwise return
data_get($this->database, 'destination.server'), so future FK paths added to
ServiceDatabase are honored without duplication.
In `@bootstrap/helpers/parsers.php`:
- Around line 1444-1450: When handling preview PRs in the block that may call
addPreviewDeploymentSuffix($serviceName, $pullRequestId) preserve the original
service name before suffixing and use that original name when inserting into
$detectedDatabases; specifically, capture $originalServiceName = $serviceName
(or similar) before the if ($isPullRequest) branch, then call
addPreviewDeploymentSuffix to mutate $serviceName for
parsedServices->put($serviceName, $payload) while still using
$originalServiceName when calling $detectedDatabases->put(..., $service) or any
DB-related keys so backups reference the unsuffixed container name.
In `@bootstrap/helpers/shared.php`:
- Around line 1921-2045: The closure used in the collect($services)->map(...) is
referencing $detectedDatabases->put(...) but $detectedDatabases is not captured
by the closure's use() list; add $detectedDatabases to the use(...) list for
both map() closures that detect databases so the closure can access and mutate
that collection (ensure the same change is applied to the other map closure
around lines indicated in the review).
In `@resources/views/livewire/project/application/configuration.blade.php`:
- Around line 70-113: The Backups Livewire component
(livewire:project.application.backups) needs the same mount guard as the
healthcheck path: update the component's mount() method to check the
application's build_pack and if it's not 'dockercompose' redirect (or abort/emit
a navigation) back to the configuration page (or another safe route) so
non-compose applications cannot mount /backups directly; locate the mount()
function in the Backups component class and add the conditional check against
$this->application->build_pack (or $application param) to perform the redirect
when not 'dockercompose'.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 36c7f437-ce83-4c23-b0e0-04a9fcfba9b3
📒 Files selected for processing (15)
app/Jobs/DatabaseBackupJob.phpapp/Livewire/Project/Application/Backups.phpapp/Models/Application.phpapp/Models/ApplicationPreview.phpapp/Models/ScheduledDatabaseBackup.phpapp/Models/ServiceDatabase.phpbootstrap/helpers/applications.phpbootstrap/helpers/database.phpbootstrap/helpers/databases.phpbootstrap/helpers/parsers.phpbootstrap/helpers/shared.phpdatabase/migrations/2026_04_19_034518_add_application_id_to_service_databases.phpresources/views/livewire/project/application/backups.blade.phpresources/views/livewire/project/application/configuration.blade.phproutes/web.php
AkprasadoP
force-pushed
the
feat/integrated-backups
branch
from
01bc809 to
e1ca73e
Compare
Author
|
@ShadowArcanist
|
AkprasadoP
force-pushed
the
feat/integrated-backups
branch
from
e1ca73e to
2a5ccb5
Compare
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
AkprasadoP
force-pushed
the
feat/integrated-backups
branch
from
2a5ccb5 to
5e62447
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
routes/web.php (1)
353-355:⚠️ Potential issue | 🟠 MajorFix backup downloads for application-owned
ServiceDatabaserows.
service_idis now nullable, sodatabase->service->destination->servercan explode for Git-based Compose backups. Use the newServiceDatabase::serveraccessor here too, or downloads become 500s — not very cybernetic.🐛 Proposed fix
if ($execution->scheduledDatabaseBackup->database->getMorphClass() === ServiceDatabase::class) { - $server = $execution->scheduledDatabaseBackup->database->service->destination->server; + $server = $execution->scheduledDatabaseBackup->database->server; } else { $server = $execution->scheduledDatabaseBackup->database->destination->server; } + if (is_null($server)) { + return response()->json(['message' => 'Backup server not found.'], 404); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@routes/web.php` around lines 353 - 355, When handling a ScheduledDatabaseBackup where $execution->scheduledDatabaseBackup->database is a ServiceDatabase (check using getMorphClass() === ServiceDatabase::class), don’t assume service->destination->server exists; replace the chain that sets $server via ->database->service->destination->server with using the ServiceDatabase model's server accessor (ServiceDatabase::server) so that nullable service_id and Git-based Compose backups work without throwing 500s.app/Models/ScheduledDatabaseBackup.php (1)
101-125:⚠️ Potential issue | 🟠 MajorFail closed when ownership is ambiguous.
service_id,application_id, andapplication_preview_idare nullable without a mutual-exclusion constraint, but thisif/elseifsilently picks the first populated owner. A malformed row can run backups against the wrong server; the preview branch can also dereference a missing preview beforedata_get()can protect it. Hasta la vista, wrong server.Proposed defensive resolution
- public function server() + public function server(): ?Server { if ($this->database) { $server = null; if ($this->database instanceof ServiceDatabase) { - if ($this->database->service_id) { - $destination = data_get($this->database->service, 'destination'); - $server = data_get($destination, 'server'); - } elseif ($this->database->application_id) { - $destination = data_get($this->database->application, 'destination'); - $server = data_get($destination, 'server'); - } elseif ($this->database->application_preview_id) { - $destination = data_get($this->database->application_preview->application, 'destination'); - $server = data_get($destination, 'server'); + $servers = collect([ + $this->database->service_id + ? data_get($this->database, 'service.destination.server') + : null, + $this->database->application_id + ? data_get($this->database, 'application.destination.server') + : null, + $this->database->application_preview_id + ? data_get($this->database, 'application_preview.application.destination.server') + : null, + ])->filter(); + + if ($servers->count() !== 1) { + return null; } + + $server = $servers->first(); } else { $destination = data_get($this->database, 'destination'); $server = data_get($destination, 'server');As per coding guidelines,
**/*.php: Use explicit return type declarations and type hints for all method parameters.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/ScheduledDatabaseBackup.php` around lines 101 - 125, The server() method currently picks the first non-null owner and can return the wrong server or dereference a missing preview; change server(): add an explicit return type (e.g. ?Server) and defensively require exactly one owner identifier set on $this->database (check service_id, application_id, application_preview_id and treat 0/multiple non-nulls as ambiguous), return null (or throw a clear exception if your app prefers) when none or more than one owner is present, and when application_preview_id is set verify application_preview and its application exist before calling data_get to obtain destination->server; reference the server() method, ServiceDatabase type checks, $this->database, service_id/application_id/application_preview_id, and application_preview to locate where to add these validations.bootstrap/helpers/parsers.php (1)
652-691:⚠️ Potential issue | 🟠 MajorStore unsuffixed service names for preview database detection.
Preview database backups will fail. At line 691,
$containerNameis built from the original$serviceNamebefore preview suffix is applied, but lines 1444–1450 store the suffixed$serviceNamekey in$detectedDatabases. Later,DatabaseBackupJobconstructs the container name fromServiceDatabase.name(the suffixed version) plus$serviceUuid, resulting in lookups for non-existent containers likepostgres-pr-123-{uuid}when the actual container is namedpostgres-{uuid}.Preserve the original service name for database detection by storing it separately before the suffix mutation at line 1444.
Suggested fix
foreach ($services as $serviceName => $service) { + $originalServiceName = $serviceName; $image = data_get_str($service, 'image'); $restart = data_get_str($service, 'restart', RESTART_MODE); $logging = data_get($service, 'logging');Then at line 1450:
if (isset($isDatabase) && $isDatabase) { - $detectedDatabases->put($serviceName, $service); + $detectedDatabases->put($originalServiceName, $service); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bootstrap/helpers/parsers.php` around lines 652 - 691, The preview suffix is being appended to $serviceName later, which breaks database detection because $detectedDatabases is populated with the suffixed name while $containerName (and DatabaseBackupJob lookups built from ServiceDatabase.name) expect the unsuffixed service name; before any mutation that adds the preview suffix, capture the original service name (e.g., $unsuffixedServiceName = $serviceName) and use that stored value when populating $detectedDatabases instead of the possibly-suffixed $serviceName so container lookups (constructed from ServiceDatabase.name and $serviceUuid) match the actual container names generated by $containerName.bootstrap/helpers/shared.php (1)
1921-2045:⚠️ Potential issue | 🔴 CriticalAdd
$detectedDatabasesto both mapper closures or prepare for runtime-explosive database detection.Two closures in
parseDockerComposeFile()call$detectedDatabases->put(...)without capturing the variable, causing an undefined variable error when parsing Docker Compose files with database services. The code literally terminates when trying to migrate databases—not serverless vibes, just pure PHP scope rules. Unlike VC-backed serverless nonsense, real servers demand proper variable capture.🐛 Proposed fix
- $services = collect($services)->map(function ($service, $serviceName) use ($topLevelVolumes, $topLevelNetworks, $definedNetwork, $isNew, $generatedServiceFQDNS, $resource, $allServices, $envComments) { + $services = collect($services)->map(function ($service, $serviceName) use ($topLevelVolumes, $topLevelNetworks, $definedNetwork, $isNew, $generatedServiceFQDNS, $resource, $allServices, $envComments, $detectedDatabases) {- $services = collect($services)->map(function ($service, $serviceName) use ($topLevelVolumes, $topLevelNetworks, $definedNetwork, $isNew, $generatedServiceFQDNS, $resource, $server, $pull_request_id, $preview_id) { + $services = collect($services)->map(function ($service, $serviceName) use ($topLevelVolumes, $topLevelNetworks, $definedNetwork, $isNew, $generatedServiceFQDNS, $resource, $server, $pull_request_id, $preview_id, $detectedDatabases) {Also applies to: 2701–3006
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bootstrap/helpers/shared.php` around lines 1921 - 2045, The closures inside parseDockerComposeFile() reference $detectedDatabases but don't capture it, causing undefined variable errors; initialize $detectedDatabases (e.g., as collect([])) before the map calls and add $detectedDatabases to the closures' use(...) lists (capture by reference if the closure mutates it) so the map callbacks (the function passed to collect(...)->map and the second mapper mentioned) can call $detectedDatabases->put(...) without runtime errors.app/Jobs/DatabaseBackupJob.php (1)
104-118:⚠️ Potential issue | 🔴 CriticalAssign the server to a nullable variable first, then validate before storing in the typed property.
data_get()returnsnullwhen a key doesn't exist, butpublic Server $servercannot accept null. This will throw aTypeErrorbefore the check at line 117 can even run. Unlike your shiny serverless vapor dreams that evaporate when you need actual infrastructure, this typed property is real and requires real validation. Store the result in a temporary nullable variable, validate it like you'd validate a taco vendor's health inspection, then assign to$this->server.🐛 Proposed fix
if (data_get($this->backup, 'database_type') === ServiceDatabase::class) { $this->database = data_get($this->backup, 'database'); + $server = null; if ($this->database->service_id) { - $this->server = $this->database->service->server; + $server = data_get($this->database, 'service.server'); } elseif ($this->database->application_id) { - $this->server = data_get($this->database, 'application.destination.server'); + $server = data_get($this->database, 'application.destination.server'); } elseif ($this->database->application_preview_id) { - $this->server = data_get($this->database, 'application_preview.application.destination.server'); + $server = data_get($this->database, 'application_preview.application.destination.server'); + } + if (! $server instanceof Server) { + throw new \Exception('Server not found?!'); } + $this->server = $server; $this->s3 = $this->backup->s3; } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Jobs/DatabaseBackupJob.php` around lines 104 - 118, The code assigns potentially null values from data_get() directly into the typed property $this->server which will throw a TypeError before your null check; change all places that set $this->server (the branches using $this->database->service->server, data_get(... 'application.destination.server'), data_get(... 'application_preview.application.destination.server'), and $this->database->destination->server) to first assign into a nullable local variable (e.g. $server = ...), then perform the is_null check and only after passing assign $this->server = $server; apply the same pattern for any other typed properties set from data_get() (e.g. $this->s3 if applicable) to ensure validation occurs before assigning to the non-nullable class properties.
♻️ Duplicate comments (4)
database/migrations/2026_04_19_034518_add_application_id_to_service_databases.php (1)
28-28:⚠️ Potential issue | 🟠 MajorRollback still drops the restored
service_idconstraint chain.Line 28 changes
service_idback to non-null without restatingconstrained()->onDelete('cascade'), so rollback can lose the FK attributes thatup()restored. This is the same T-800 target as before, just hiding indown().#!/bin/bash # Verify all service_id column changes restate the complete FK definition. rg -nP "foreignId\('service_id'\).*change\(|foreignId\('service_id'\).*constrained\(\).*onDelete\('cascade'\)" \ database/migrations/2026_04_19_034518_add_application_id_to_service_databases.php \ database/migrationsExpected result: every
foreignId('service_id')->...->change()in this migration should include the full constraint chain.🔧 Proposed fix
- $table->foreignId('service_id')->nullable(false)->change(); + $table->foreignId('service_id')->nullable(false)->constrained()->onDelete('cascade')->change();As per coding guidelines, “When modifying a column, the migration must include all of the attributes that were previously defined on the column. Otherwise, they will be dropped and lost”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@database/migrations/2026_04_19_034518_add_application_id_to_service_databases.php` at line 28, The down() migration currently changes the service_id column to non-null via foreignId('service_id')->nullable(false)->change() but omits the FK chain restored in up(); update the down() change() call for service_id to include the full foreign key attributes (e.g. ->constrained()->onDelete('cascade') alongside nullable(false)) so the rollback preserves the original FK behavior, matching the signature used in up() (search for foreignId('service_id')->...->change() and ensure it mirrors the constrained()->onDelete('cascade') chain).app/Models/ScheduledDatabaseBackup.php (1)
11-16:⚠️ Potential issue | 🟠 MajorDon’t terminate the flight recorder while a backup is still flying.
This hook deletes
runningexecution rows too. If a schedule is deleted whileDatabaseBackupJobis still updating its execution log, the final status/output can be lost like a T-800 in a lava bath. Gate deletion while executions are running, and type the model event signature.Proposed guard
- protected static function booted() + protected static function booted(): void { - static::deleting(function ($backup) { + static::deleting(function (ScheduledDatabaseBackup $backup): void { + if ($backup->executions()->where('status', 'running')->exists()) { + throw new \RuntimeException('Cannot delete a scheduled backup while an execution is running.'); + } + $backup->executions()->delete(); }); }As per coding guidelines,
**/*.php: Use explicit return type declarations and type hints for all method parameters.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/ScheduledDatabaseBackup.php` around lines 11 - 16, The deleting hook on ScheduledDatabaseBackup currently deletes all executions including ones with status "running" and uses no type hints; change ScheduledDatabaseBackup::booted to have a return type (static function booted(): void) and type the closure parameter and return (static::deleting(function (ScheduledDatabaseBackup $backup): void { ... })); inside the closure, check for running executions using $backup->executions()->where('status', 'running')->exists() and throw a runtime/logic exception to prevent deletion if any running execution exists, otherwise proceed to delete executions; this preserves in-progress DatabaseBackupJob logs and satisfies the typed signature guideline.app/Models/ServiceDatabase.php (2)
227-235: 🛠️ Refactor suggestion | 🟠 MajorType the new Eloquent relationships.
application()andapplication_preview()should declareBelongsToreturn types.♻️ Proposed fix
use Illuminate\Database\Eloquent\Factories\HasFactory; +use Illuminate\Database\Eloquent\Relations\BelongsTo; use Illuminate\Database\Eloquent\SoftDeletes;- public function application() + public function application(): BelongsTo { return $this->belongsTo(Application::class); } - public function application_preview() + public function application_preview(): BelongsTo { return $this->belongsTo(ApplicationPreview::class); }As per coding guidelines, “Use explicit return type declarations and type hints for all method parameters”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/ServiceDatabase.php` around lines 227 - 235, The methods application() and application_preview() currently lack return type hints; update both to declare a BelongsTo return type (e.g., public function application(): BelongsTo and public function application_preview(): BelongsTo) and add the appropriate import (use Illuminate\Database\Eloquent\Relations\BelongsTo;) at the top of the class so the type resolves.
82-99:⚠️ Potential issue | 🟠 MajorRestart the actual compose database container.
Application compose containers are prefixed with the service name, so these branches still target the app base container instead of the database service container.
🐛 Proposed fix
} elseif ($this->application_id) { - $container_id = generateApplicationContainerName($this->application); + $container_id = $this->name.'-'.generateApplicationContainerName($this->application); $server = $this->application->destination->server; } elseif ($this->application_preview_id) { - $container_id = generateApplicationContainerName($this->application_preview->application, $this->application_preview->pull_request_id); + $container_id = $this->name.'-'.generateApplicationContainerName($this->application_preview->application, $this->application_preview->pull_request_id); $server = $this->application_preview->application->destination->server; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/ServiceDatabase.php` around lines 82 - 99, The restart() method currently targets the application base container for application_id and application_preview_id branches; update those branches so the container_id targets the specific compose service by appending the service name suffix (the same pattern used for service_id). Specifically, after calling generateApplicationContainerName($this->application) and generateApplicationContainerName($this->application_preview->application, $this->application_preview->pull_request_id) append '-'.$this->name so the container matches the compose service, keep the same server resolution and then call remote_process(["docker restart {$container_id}"], $server) as before.
🤖 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/Jobs/DatabaseBackupJob.php`:
- Around line 162-174: In DatabaseBackupJob, the application-owned branches set
serviceUuid to the raw application UUID which can mismatch the parser’s
container base; instead set serviceUuid to the parser/container base used
elsewhere by calling the same helper that builds container names (e.g.
generateApplicationContainerName or the existing container name generator used
for parsing) with $this->database->application->uuid in the application branch
and with $this->database->application_preview->application->uuid (then append
"-pr-{$this->database->application_preview->pull_request_id}") in the preview
branch, keeping serviceName as the slugged application name; update the code
paths that currently assign the raw UUID to serviceUuid so they use the
generator to ensure backups target the correct container.
In `@bootstrap/helpers/database.php`:
- Around line 13-56: The code repeats branching on $resource to set the same
foreign key in two places (the ServiceDatabase::where(...) closure and later
when building $data); compute the owner column key once (e.g., determine
$ownerKey = 'service_id'|'application_id'|'application_preview_id' based on
instanceof checks against Service, Application, ApplicationPreview using
$resource) and reuse it: use $ownerKey in the where(...) query and set
$data[$ownerKey] = $resourceId instead of repeating the three if/elseif blocks;
update references to ServiceDatabase::where(...) and the $data construction to
use this single derived $ownerKey variable.
In
`@database/migrations/2026_04_19_034518_add_application_id_to_service_databases.php`:
- Around line 16-17: The migration added foreignId('application_id') and
foreignId('application_preview_id') with cascading deletes which will
hard-delete ServiceDatabase rows before their deleting hooks run, so update the
force-delete logic in the parent models (Application and ApplicationPreview) to
explicitly iterate and delete related databases via the databases() relationship
(call databases()->each/deleting or databases()->delete() before the parent is
destroyed) to trigger ServiceDatabase::deleting cleanup; also update the down()
rollback in the migration
2026_04_19_034518_add_application_id_to_service_databases.php to restore the
service_id column with constrained()->onDelete('cascade') (preserve the original
foreign key behavior) instead of dropping the cascade attribute.
In `@resources/views/livewire/project/application/backups.blade.php`:
- Line 21: Add a unique Livewire key to the create-scheduled-backup component to
prevent state bleed between database cards: locate the
<livewire:project.database.create-scheduled-backup> tag inside the `@forelse` loop
(paired with the scheduled-backups component that already uses
:key="$database->id") and add a :key attribute using the database id (e.g.,
:key="$database->id") so each create-scheduled-backup instance gets a distinct
key and maintains isolated state.
---
Outside diff comments:
In `@app/Jobs/DatabaseBackupJob.php`:
- Around line 104-118: The code assigns potentially null values from data_get()
directly into the typed property $this->server which will throw a TypeError
before your null check; change all places that set $this->server (the branches
using $this->database->service->server, data_get(...
'application.destination.server'), data_get(...
'application_preview.application.destination.server'), and
$this->database->destination->server) to first assign into a nullable local
variable (e.g. $server = ...), then perform the is_null check and only after
passing assign $this->server = $server; apply the same pattern for any other
typed properties set from data_get() (e.g. $this->s3 if applicable) to ensure
validation occurs before assigning to the non-nullable class properties.
In `@app/Models/ScheduledDatabaseBackup.php`:
- Around line 101-125: The server() method currently picks the first non-null
owner and can return the wrong server or dereference a missing preview; change
server(): add an explicit return type (e.g. ?Server) and defensively require
exactly one owner identifier set on $this->database (check service_id,
application_id, application_preview_id and treat 0/multiple non-nulls as
ambiguous), return null (or throw a clear exception if your app prefers) when
none or more than one owner is present, and when application_preview_id is set
verify application_preview and its application exist before calling data_get to
obtain destination->server; reference the server() method, ServiceDatabase type
checks, $this->database, service_id/application_id/application_preview_id, and
application_preview to locate where to add these validations.
In `@bootstrap/helpers/parsers.php`:
- Around line 652-691: The preview suffix is being appended to $serviceName
later, which breaks database detection because $detectedDatabases is populated
with the suffixed name while $containerName (and DatabaseBackupJob lookups built
from ServiceDatabase.name) expect the unsuffixed service name; before any
mutation that adds the preview suffix, capture the original service name (e.g.,
$unsuffixedServiceName = $serviceName) and use that stored value when populating
$detectedDatabases instead of the possibly-suffixed $serviceName so container
lookups (constructed from ServiceDatabase.name and $serviceUuid) match the
actual container names generated by $containerName.
In `@bootstrap/helpers/shared.php`:
- Around line 1921-2045: The closures inside parseDockerComposeFile() reference
$detectedDatabases but don't capture it, causing undefined variable errors;
initialize $detectedDatabases (e.g., as collect([])) before the map calls and
add $detectedDatabases to the closures' use(...) lists (capture by reference if
the closure mutates it) so the map callbacks (the function passed to
collect(...)->map and the second mapper mentioned) can call
$detectedDatabases->put(...) without runtime errors.
In `@routes/web.php`:
- Around line 353-355: When handling a ScheduledDatabaseBackup where
$execution->scheduledDatabaseBackup->database is a ServiceDatabase (check using
getMorphClass() === ServiceDatabase::class), don’t assume
service->destination->server exists; replace the chain that sets $server via
->database->service->destination->server with using the ServiceDatabase model's
server accessor (ServiceDatabase::server) so that nullable service_id and
Git-based Compose backups work without throwing 500s.
---
Duplicate comments:
In `@app/Models/ScheduledDatabaseBackup.php`:
- Around line 11-16: The deleting hook on ScheduledDatabaseBackup currently
deletes all executions including ones with status "running" and uses no type
hints; change ScheduledDatabaseBackup::booted to have a return type (static
function booted(): void) and type the closure parameter and return
(static::deleting(function (ScheduledDatabaseBackup $backup): void { ... }));
inside the closure, check for running executions using
$backup->executions()->where('status', 'running')->exists() and throw a
runtime/logic exception to prevent deletion if any running execution exists,
otherwise proceed to delete executions; this preserves in-progress
DatabaseBackupJob logs and satisfies the typed signature guideline.
In `@app/Models/ServiceDatabase.php`:
- Around line 227-235: The methods application() and application_preview()
currently lack return type hints; update both to declare a BelongsTo return type
(e.g., public function application(): BelongsTo and public function
application_preview(): BelongsTo) and add the appropriate import (use
Illuminate\Database\Eloquent\Relations\BelongsTo;) at the top of the class so
the type resolves.
- Around line 82-99: The restart() method currently targets the application base
container for application_id and application_preview_id branches; update those
branches so the container_id targets the specific compose service by appending
the service name suffix (the same pattern used for service_id). Specifically,
after calling generateApplicationContainerName($this->application) and
generateApplicationContainerName($this->application_preview->application,
$this->application_preview->pull_request_id) append '-'.$this->name so the
container matches the compose service, keep the same server resolution and then
call remote_process(["docker restart {$container_id}"], $server) as before.
In
`@database/migrations/2026_04_19_034518_add_application_id_to_service_databases.php`:
- Line 28: The down() migration currently changes the service_id column to
non-null via foreignId('service_id')->nullable(false)->change() but omits the FK
chain restored in up(); update the down() change() call for service_id to
include the full foreign key attributes (e.g.
->constrained()->onDelete('cascade') alongside nullable(false)) so the rollback
preserves the original FK behavior, matching the signature used in up() (search
for foreignId('service_id')->...->change() and ensure it mirrors the
constrained()->onDelete('cascade') chain).
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c01f7843-988f-4da9-abb2-79953d4132cc
📒 Files selected for processing (16)
app/Jobs/DatabaseBackupJob.phpapp/Livewire/Project/Application/Backups.phpapp/Models/Application.phpapp/Models/ApplicationPreview.phpapp/Models/ScheduledDatabaseBackup.phpapp/Models/ServiceDatabase.phpbootstrap/helpers/applications.phpbootstrap/helpers/database.phpbootstrap/helpers/databases.phpbootstrap/helpers/parsers.phpbootstrap/helpers/shared.phpdatabase/migrations/2026_04_19_034518_add_application_id_to_service_databases.phpdocker-compose.dev.ymlresources/views/livewire/project/application/backups.blade.phpresources/views/livewire/project/application/configuration.blade.phproutes/web.php
AkprasadoP
force-pushed
the
feat/integrated-backups
branch
from
5e62447 to
8651b72
Compare
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 (4)
routes/web.php (1)
353-356:⚠️ Potential issue | 🟠 MajorFix local download server resolution for application-owned databases.
For Git-based Compose databases,
ServiceDatabase::servicecan be null. This path still dereferencesdatabase->service->destination->server, so local backup downloads can 500 after the new Backups page exposes them.🐛 Proposed fix
- if ($execution->scheduledDatabaseBackup->database->getMorphClass() === ServiceDatabase::class) { - $server = $execution->scheduledDatabaseBackup->database->service->destination->server; - } else { - $server = $execution->scheduledDatabaseBackup->database->destination->server; - } + $server = $execution->scheduledDatabaseBackup->server(); + if (! $server) { + return response()->json(['message' => 'Backup server not found.'], 404); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@routes/web.php` around lines 353 - 356, The code assumes ServiceDatabase::service is always present when the database is a ServiceDatabase; update the resolution logic around $execution->scheduledDatabaseBackup->database so that when getMorphClass() === ServiceDatabase::class you first check if ->service is non-null and use ->service->destination->server only if present, otherwise fall back to using ->database->destination->server (same fallback used in the else branch); modify the server assignment for the ServiceDatabase branch to handle a null service to avoid dereferencing a null property.app/Models/ScheduledDatabaseBackup.php (1)
101-126:⚠️ Potential issue | 🟡 MinorReuse the ServiceDatabase
serveraccessor to avoid preview null fatals.Line 113 evaluates
$this->database->application_preview->applicationbeforedata_get()can protect it. Delegating toServiceDatabase::serverkeeps service/application/preview resolution in one null-safe place.♻️ Proposed fix
public function server() { if ($this->database) { - $server = null; if ($this->database instanceof ServiceDatabase) { - if ($this->database->service_id) { - $destination = data_get($this->database->service, 'destination'); - $server = data_get($destination, 'server'); - } elseif ($this->database->application_id) { - $destination = data_get($this->database->application, 'destination'); - $server = data_get($destination, 'server'); - } elseif ($this->database->application_preview_id) { - $destination = data_get($this->database->application_preview->application, 'destination'); - $server = data_get($destination, 'server'); - } + return $this->database->server; } else { $destination = data_get($this->database, 'destination'); $server = data_get($destination, 'server'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/ScheduledDatabaseBackup.php` around lines 101 - 126, The server() method in ScheduledDatabaseBackup currently drills into $this->database->application_preview->application which can trigger null property access; instead, when $this->database is an instance of ServiceDatabase simply delegate to the ServiceDatabase::server accessor (e.g. return $this->database->server) so service/application/preview resolution stays null-safe inside ServiceDatabase::server; retain the existing fallback for non-ServiceDatabase cases (using data_get($this->database, 'destination') and data_get($destination, 'server')) and return null if nothing is found.bootstrap/helpers/parsers.php (1)
1444-1460:⚠️ Potential issue | 🔴 CriticalI'll be back—fix the preview key mismatch and drop the suffixed name from the database sync.
Three critical issues on lines 1444–1460 that will terminate your deployments like I terminate threats:
Suffixed name becomes the database identity (major). At line 1445,
$serviceNamegets suffixed with-pr-{id}. Then at line 1450, you store it in$detectedDatabaseswith that suffixed key. WhenupdateResourceDatabases()runs, it matches by thenamefield (seedatabase.php), so the preview gets aServiceDatabaserow with namepostgres-pr-42instead of the originalpostgres. If the sync logic ever keys by the un-suffixed name, you have orphaned rows. If a user manually fixes the compose, the suffix path can never converge. Stash the original service name before the suffix is applied, or keep the unsuffixed key in$detectedDatabases.Two preview lookups, one unreliable (critical). Line 1454 uses
$resource->previews()->where('pull_request_id', $pullRequestId)->first(), but your function already has$preview_idand line 1164/1226 usefind($preview_id). Thepull_request_idlookup is fragile—if two previews somehow share apull_request_id, you get the wrong preview. Usefind($preview_id)consistently (or drop the second parameter if it's redundant).Redundant isset (minor). The
isset($isDatabase) && $isDatabasecheck at line 1449 is belt-and-suspenders—$isDatabaseis unconditionally assigned at line 684 inside the same loop iteration. Drop theisset().This code flows like a taco truck's drive-thru: smooth on the surface, but the kitchen logic is gluten-contaminated. Fix it before your preview deployments start multiplying like serverless instances on a VC's dashboard.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bootstrap/helpers/parsers.php` around lines 1444 - 1460, The suffixed preview name is being stored into $detectedDatabases causing wrong DB identities and the preview lookup is fragile; before calling addPreviewDeploymentSuffix($serviceName, $pullRequestId) stash the original name (e.g., $origServiceName) and put the database into $detectedDatabases keyed by the unsuffixed name (use $origServiceName) instead of $serviceName, remove the redundant isset() check around $isDatabase and simply test $isDatabase, and replace the fragile resource lookup $resource->previews()->where('pull_request_id', $pullRequestId)->first() with a direct find($preview_id) (use the existing $preview_id used earlier) so updateResourceDatabases($resourceToSync, $detectedDatabases) receives correct resources and keys.bootstrap/helpers/shared.php (1)
1921-1921:⚠️ Potential issue | 🔴 CriticalCapture
$detectedDatabasesin both parser closures or PHP will terminate the operation like a failed server.Line 2044 and Line 3005 attempt to use
$detectedDatabasesinside closures that don't import it—this triggers an undefined variable error when any database image is detected, breaking the backup feature entirely. Unlike serverless (which hides errors), proper self-hosted infrastructure demands we handle this correctly.🐛 Proposed fix
- $services = collect($services)->map(function ($service, $serviceName) use ($topLevelVolumes, $topLevelNetworks, $definedNetwork, $isNew, $generatedServiceFQDNS, $resource, $allServices, $envComments) { + $services = collect($services)->map(function ($service, $serviceName) use ($topLevelVolumes, $topLevelNetworks, $definedNetwork, $isNew, $generatedServiceFQDNS, $resource, $allServices, $envComments, $detectedDatabases) {- $services = collect($services)->map(function ($service, $serviceName) use ($topLevelVolumes, $topLevelNetworks, $definedNetwork, $isNew, $generatedServiceFQDNS, $resource, $server, $pull_request_id, $preview_id) { + $services = collect($services)->map(function ($service, $serviceName) use ($topLevelVolumes, $topLevelNetworks, $definedNetwork, $isNew, $generatedServiceFQDNS, $resource, $server, $pull_request_id, $preview_id, $detectedDatabases) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bootstrap/helpers/shared.php` at line 1921, The closures mapping services (the map(function ($service, $serviceName) use (...)) blocks) are using $detectedDatabases without importing it, causing an undefined variable fatal error; update both closures (the one starting at the shown $services = collect($services)->map(... ) and the other similar parser closure later) to include $detectedDatabases in their use(...) lists so the closures capture that variable and no longer trigger undefined variable errors when database images are detected.
♻️ Duplicate comments (4)
database/migrations/2026_04_19_034518_add_application_id_to_service_databases.php (1)
26-26:⚠️ Potential issue | 🟡 MinorClean dependent backup rows before the raw rollback delete.
Line 26 deletes
service_databasesvia the query builder, soServiceDatabase::deletingwill not delete scheduled backups/executions. Rollbacks can leave orphaned backup records for application-owned databases.🛡️ Proposed rollback cleanup
- \Illuminate\Support\Facades\DB::table('service_databases')->whereNull('service_id')->delete(); + $serviceDatabaseIds = \Illuminate\Support\Facades\DB::table('service_databases') + ->whereNull('service_id') + ->pluck('id'); + + $scheduledBackupIds = \Illuminate\Support\Facades\DB::table('scheduled_database_backups') + ->where('database_type', \App\Models\ServiceDatabase::class) + ->whereIn('database_id', $serviceDatabaseIds) + ->pluck('id'); + + \Illuminate\Support\Facades\DB::table('scheduled_database_backup_executions') + ->whereIn('scheduled_database_backup_id', $scheduledBackupIds) + ->delete(); + + \Illuminate\Support\Facades\DB::table('scheduled_database_backups') + ->whereIn('id', $scheduledBackupIds) + ->delete(); + + \Illuminate\Support\Facades\DB::table('service_databases') + ->whereIn('id', $serviceDatabaseIds) + ->delete();As per coding guidelines, “Database work should follow Eloquent ORM patterns, migration best practices, relationship definitions, and query optimization as documented in
.ai/patterns/database-patterns.md”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@database/migrations/2026_04_19_034518_add_application_id_to_service_databases.php` at line 26, The raw DB delete bypasses Eloquent events so scheduled backups/executions remain orphaned; change the rollback to load the affected ServiceDatabase models and remove dependents via Eloquent (e.g., ServiceDatabase::whereNull('service_id')->chunk(...)->each(fn($sd)=>{ $sd->scheduledBackups()->delete(); $sd->executions()->delete(); $sd->delete(); }) or simply $sd->delete() to trigger ServiceDatabase::deleting); reference the ServiceDatabase model, its deleting event and the scheduledBackups/executions relations and use chunking to avoid memory spikes.resources/views/livewire/project/application/backups.blade.php (1)
19-23:⚠️ Potential issue | 🟠 MajorStill missing a unique
:keyoncreate-scheduled-backup— state will bleed between DB cards like a leaky serverless function (and you know how I feel about those).Inside
@forelse, each iteration renders acreate-scheduled-backupLivewire child, but without:key, Livewire can't distinguish instances and the locked$databaseprop can bleed across cards when multiple databases are detected. The siblingscheduled-backupson Line 25 already has:key="$database->id"— give this one the same treatment. Servers, tacos, and Livewire components all perform better when you keep them properly identified.♻️ Proposed fix
- <x-modal-input buttonTitle="+ Add" title="New Scheduled Backup" canGate="update" :canResource="$database"> - <livewire:project.database.create-scheduled-backup :database="$database" /> - </x-modal-input> + <x-modal-input buttonTitle="+ Add" title="New Scheduled Backup" canGate="update" :canResource="$database"> + <livewire:project.database.create-scheduled-backup :database="$database" :key="'create-scheduled-backup-'.$database->id" /> + </x-modal-input>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/views/livewire/project/application/backups.blade.php` around lines 19 - 23, The create-scheduled-backup Livewire child inside the `@forelse` loop lacks a unique :key which allows its locked $database prop to bleed between iterations; update the <livewire:project.database.create-scheduled-backup :database="$database" /> invocation to include a unique key (e.g. :key="$database->id") matching the sibling scheduled-backups component so Livewire can distinguish instances and prevent state leakage.app/Models/ServiceDatabase.php (2)
227-235: 🛠️ Refactor suggestion | 🟠 MajorType the new Eloquent relationships.
Add explicit
BelongsToreturn types to keep these model relationships consistent with the project’s PHP typing rules.♻️ Proposed fix
use Illuminate\Database\Eloquent\Factories\HasFactory; +use Illuminate\Database\Eloquent\Relations\BelongsTo; use Illuminate\Database\Eloquent\SoftDeletes;- public function application() + public function application(): BelongsTo { return $this->belongsTo(Application::class); } - public function application_preview() + public function application_preview(): BelongsTo { return $this->belongsTo(ApplicationPreview::class); }Verify the relationship signatures:
#!/bin/bash rg -n -C2 'function application(_preview)?\s*\(' app/Models/ServiceDatabase.phpAs per coding guidelines, “Use explicit return type declarations and type hints for all method parameters”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/ServiceDatabase.php` around lines 227 - 235, The methods application() and application_preview() in ServiceDatabase should declare explicit return types of BelongsTo; update their signatures to return Illuminate\Database\Eloquent\Relations\BelongsTo and add the necessary use/import statement for BelongsTo at the top of the file so both application() and application_preview() follow the project’s typing rules and match other model relationships.
89-94:⚠️ Potential issue | 🟠 MajorRestart the database service container, not the application base.
Application compose containers are named with the database service prefix. These branches currently target only the generated application base container.
🐛 Proposed fix
} elseif ($this->application_id) { - $container_id = generateApplicationContainerName($this->application); + $container_id = $this->name.'-'.generateApplicationContainerName($this->application); $server = $this->application->destination->server; } elseif ($this->application_preview_id) { - $container_id = generateApplicationContainerName($this->application_preview->application, $this->application_preview->pull_request_id); + $container_id = $this->name.'-'.generateApplicationContainerName($this->application_preview->application, $this->application_preview->pull_request_id); $server = $this->application_preview->application->destination->server; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/ServiceDatabase.php` around lines 89 - 94, The branches for application_id and application_preview_id are currently generating the application base container name (via generateApplicationContainerName) but need to target the database service container instead; change the container_id assignment in both branches to generate the compose service container name for the DB service (e.g. call a helper that accepts a service name or pass the DB service suffix) so it produces the application container name plus the DB service prefix/suffix rather than the base app container. Update the two lines using generateApplicationContainerName(...) to either call generateApplicationContainerName(..., $dbServiceName) or use the existing generateApplicationServiceContainerName / compose service helper with the DB service (or append the DB service name like "_db") and keep server assignment unchanged.
🤖 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/Jobs/DatabaseBackupJob.php`:
- Around line 104-110: The $server typed property on the DatabaseBackupJob must
be made nullable to avoid a TypeError when data_get() returns null in the
fallback branches; change the declaration of the property (currently "public
Server $server") to a nullable type (e.g., "public ?Server $server") and update
any code that assumes non-null (notably the resolution logic in the conditional
block using $this->database->service_id / application_id /
application_preview_id and the guard check around lines 117-118) to handle a
null $this->server safely.
In `@app/Models/ServiceDatabase.php`:
- Around line 12-15: The ServiceDatabase model allows multiple nullable owner
IDs via $fillable which, combined with the ownedByCurrentTeam*() OR-based
authorization, permits rows to be visible via one owner while actions use
another; to fix this, update the ServiceDatabase model to enforce a single
canonical owner: remove owner ID columns from $fillable (so owners are set only
through controlled methods), add a validation/assignment in the
ServiceDatabase::create/update flow (or in setOwner()/assignOwner() helper) that
ensures exactly one owner ID is populated (and clears any other owner columns),
and update any sync logic to clear inactive owner columns; alternatively add a
DB CHECK constraint to enforce exactly one non-null owner column and update the
ownedByCurrentTeam*() authorization to rely on that single owner field to
authorize access.
In `@bootstrap/helpers/shared.php`:
- Around line 3404-3411: The code currently throws if a preview row is missing
because ApplicationPreview::findPreviewByApplicationAndPullId uses
firstOrFail(); change that lookup to return null instead (use ->first() or a
nullable query) so the $resourceToSync guard can skip syncs gracefully; update
the call site where $resourceToSync is assigned (the
ApplicationPreview::findPreviewByApplicationAndPullId usage) to use a nullable
lookup and keep the subsequent if ($resourceToSync) {
updateResourceDatabases($resourceToSync, $detectedDatabases); } logic unchanged.
---
Outside diff comments:
In `@app/Models/ScheduledDatabaseBackup.php`:
- Around line 101-126: The server() method in ScheduledDatabaseBackup currently
drills into $this->database->application_preview->application which can trigger
null property access; instead, when $this->database is an instance of
ServiceDatabase simply delegate to the ServiceDatabase::server accessor (e.g.
return $this->database->server) so service/application/preview resolution stays
null-safe inside ServiceDatabase::server; retain the existing fallback for
non-ServiceDatabase cases (using data_get($this->database, 'destination') and
data_get($destination, 'server')) and return null if nothing is found.
In `@bootstrap/helpers/parsers.php`:
- Around line 1444-1460: The suffixed preview name is being stored into
$detectedDatabases causing wrong DB identities and the preview lookup is
fragile; before calling addPreviewDeploymentSuffix($serviceName, $pullRequestId)
stash the original name (e.g., $origServiceName) and put the database into
$detectedDatabases keyed by the unsuffixed name (use $origServiceName) instead
of $serviceName, remove the redundant isset() check around $isDatabase and
simply test $isDatabase, and replace the fragile resource lookup
$resource->previews()->where('pull_request_id', $pullRequestId)->first() with a
direct find($preview_id) (use the existing $preview_id used earlier) so
updateResourceDatabases($resourceToSync, $detectedDatabases) receives correct
resources and keys.
In `@bootstrap/helpers/shared.php`:
- Line 1921: The closures mapping services (the map(function ($service,
$serviceName) use (...)) blocks) are using $detectedDatabases without importing
it, causing an undefined variable fatal error; update both closures (the one
starting at the shown $services = collect($services)->map(... ) and the other
similar parser closure later) to include $detectedDatabases in their use(...)
lists so the closures capture that variable and no longer trigger undefined
variable errors when database images are detected.
In `@routes/web.php`:
- Around line 353-356: The code assumes ServiceDatabase::service is always
present when the database is a ServiceDatabase; update the resolution logic
around $execution->scheduledDatabaseBackup->database so that when
getMorphClass() === ServiceDatabase::class you first check if ->service is
non-null and use ->service->destination->server only if present, otherwise fall
back to using ->database->destination->server (same fallback used in the else
branch); modify the server assignment for the ServiceDatabase branch to handle a
null service to avoid dereferencing a null property.
---
Duplicate comments:
In `@app/Models/ServiceDatabase.php`:
- Around line 227-235: The methods application() and application_preview() in
ServiceDatabase should declare explicit return types of BelongsTo; update their
signatures to return Illuminate\Database\Eloquent\Relations\BelongsTo and add
the necessary use/import statement for BelongsTo at the top of the file so both
application() and application_preview() follow the project’s typing rules and
match other model relationships.
- Around line 89-94: The branches for application_id and application_preview_id
are currently generating the application base container name (via
generateApplicationContainerName) but need to target the database service
container instead; change the container_id assignment in both branches to
generate the compose service container name for the DB service (e.g. call a
helper that accepts a service name or pass the DB service suffix) so it produces
the application container name plus the DB service prefix/suffix rather than the
base app container. Update the two lines using
generateApplicationContainerName(...) to either call
generateApplicationContainerName(..., $dbServiceName) or use the existing
generateApplicationServiceContainerName / compose service helper with the DB
service (or append the DB service name like "_db") and keep server assignment
unchanged.
In
`@database/migrations/2026_04_19_034518_add_application_id_to_service_databases.php`:
- Line 26: The raw DB delete bypasses Eloquent events so scheduled
backups/executions remain orphaned; change the rollback to load the affected
ServiceDatabase models and remove dependents via Eloquent (e.g.,
ServiceDatabase::whereNull('service_id')->chunk(...)->each(fn($sd)=>{
$sd->scheduledBackups()->delete(); $sd->executions()->delete(); $sd->delete();
}) or simply $sd->delete() to trigger ServiceDatabase::deleting); reference the
ServiceDatabase model, its deleting event and the scheduledBackups/executions
relations and use chunking to avoid memory spikes.
In `@resources/views/livewire/project/application/backups.blade.php`:
- Around line 19-23: The create-scheduled-backup Livewire child inside the
`@forelse` loop lacks a unique :key which allows its locked $database prop to
bleed between iterations; update the
<livewire:project.database.create-scheduled-backup :database="$database" />
invocation to include a unique key (e.g. :key="$database->id") matching the
sibling scheduled-backups component so Livewire can distinguish instances and
prevent state leakage.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 85fea3f0-a4d8-4072-8175-0ba4111cdc2c
📒 Files selected for processing (15)
app/Jobs/DatabaseBackupJob.phpapp/Livewire/Project/Application/Backups.phpapp/Models/Application.phpapp/Models/ApplicationPreview.phpapp/Models/ScheduledDatabaseBackup.phpapp/Models/ServiceDatabase.phpbootstrap/helpers/applications.phpbootstrap/helpers/database.phpbootstrap/helpers/databases.phpbootstrap/helpers/parsers.phpbootstrap/helpers/shared.phpdatabase/migrations/2026_04_19_034518_add_application_id_to_service_databases.phpresources/views/livewire/project/application/backups.blade.phpresources/views/livewire/project/application/configuration.blade.phproutes/web.php
AkprasadoP
force-pushed
the
feat/integrated-backups
branch
from
8651b72 to
54338f9
Compare
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)
bootstrap/helpers/shared.php (1)
1921-2045:⚠️ Potential issue | 🔴 CriticalCapture
$detectedDatabasesin both compose parser closures.PHP closures don't magically inherit local variables—they're not serverless functions with a VC-marketing funding round pretending magic happens. You gotta pass them explicitly in
use(). Right now both closures at lines 1921 and 2701 try to call$detectedDatabases->put()without access to it. When the first database image rolls in, Hasta la vista, baby—undefined variable. Backups never make it to the self-hosted database server where they belong (proper hot servers, not that ephemeral container nonsense).🐛 Proposed fix
- $services = collect($services)->map(function ($service, $serviceName) use ($topLevelVolumes, $topLevelNetworks, $definedNetwork, $isNew, $generatedServiceFQDNS, $resource, $allServices, $envComments) { + $services = collect($services)->map(function ($service, $serviceName) use ($topLevelVolumes, $topLevelNetworks, $definedNetwork, $isNew, $generatedServiceFQDNS, $resource, $allServices, $envComments, $detectedDatabases) { @@ - $services = collect($services)->map(function ($service, $serviceName) use ($topLevelVolumes, $topLevelNetworks, $definedNetwork, $isNew, $generatedServiceFQDNS, $resource, $server, $pull_request_id, $preview_id) { + $services = collect($services)->map(function ($service, $serviceName) use ($topLevelVolumes, $topLevelNetworks, $definedNetwork, $isNew, $generatedServiceFQDNS, $resource, $server, $pull_request_id, $preview_id, $detectedDatabases) {Also applies to: 2701–3006
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bootstrap/helpers/shared.php` around lines 1921 - 2045, The closures that call $detectedDatabases->put() (the service mapping closure around the service processing block and the second compose parser closure later) do not have $detectedDatabases in their lexical scope; add $detectedDatabases to each closure's use(...) list so the closures can access and modify the shared collection, ensuring both places (the closure that builds $savedService and the later compose parser closure that also writes detected DBs) can call $detectedDatabases->put(...) without an undefined variable error.
♻️ Duplicate comments (4)
bootstrap/helpers/applications.php (1)
154-170:⚠️ Potential issue | 🟡 MinorThe relation name is actually correct—snake_case methods are used here, not camelCase.
The
whereHasMorphfix is solid. However, two things still need attention:
Relation name on
ServiceDatabaseis fine. The method is genuinelyapplication_preview()(snake_case), and the code calls it correctly. No runtimeBadMethodCallExceptionincoming—this gate closes properly like a self-hosted server with actual uptime.Redundant type filter—drop it for clarity. The
->where('database_type', \App\Models\ServiceDatabase::class)at line 156 is indeed covered bywhereHasMorph('database', [\App\Models\ServiceDatabase::class], ...)right after. Not harmful, but unnecessary baggage.Status literal is correct. The migration hardcodes
enum('status', ['success', 'failed', 'running'])->default('running'), and backups default to'running'on creation. Your gate won't silently flatline here either.🔧 Minor cleanup (drop redundant type filter)
$running_backup = \App\Models\ScheduledDatabaseBackupExecution::where('status', 'running') ->whereHas('scheduledDatabaseBackup', function ($query) use ($application_id, $pull_request_id) { - $query->where('database_type', \App\Models\ServiceDatabase::class) - ->whereHasMorph('database', [\App\Models\ServiceDatabase::class], function ($q) use ($application_id, $pull_request_id) { + $query->whereHasMorph('database', [\App\Models\ServiceDatabase::class], function ($q) use ($application_id, $pull_request_id) { if ($pull_request_id !== 0) { $q->whereHas('application_preview', function ($sub) use ($application_id, $pull_request_id) { $sub->where('application_id', $application_id)->where('pull_request_id', $pull_request_id); }); } else { $q->where('application_id', $application_id); } - }); + }); })->exists();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bootstrap/helpers/applications.php` around lines 154 - 170, Remove the redundant explicit type filter and keep the whereHasMorph polymorphic check: in the ScheduledDatabaseBackupExecution query, delete the ->where('database_type', \App\Models\ServiceDatabase::class) call and rely solely on whereHasMorph('database', [\App\Models\ServiceDatabase::class], ...) to scope to ServiceDatabase; keep the application_preview relation usage as-is (snake_case) inside the nested whereHas to check application_id/pull_request_id.app/Models/ServiceDatabase.php (2)
237-245: 🛠️ Refactor suggestion | 🟠 MajorType the new Eloquent relationships.
application()andapplication_preview()should returnBelongsTo; this remains unresolved.♻️ Proposed fix
use Illuminate\Database\Eloquent\Factories\HasFactory; +use Illuminate\Database\Eloquent\Relations\BelongsTo; use Illuminate\Database\Eloquent\SoftDeletes; @@ - public function application() + public function application(): BelongsTo { return $this->belongsTo(Application::class); } - public function application_preview() + public function application_preview(): BelongsTo { return $this->belongsTo(ApplicationPreview::class); }Verification:
#!/bin/bash # Expect application() and application_preview() to declare BelongsTo return types. rg -n -C2 'function application\(|function application_preview\(|Relations\\BelongsTo' app/Models/ServiceDatabase.phpAs per coding guidelines, “Use explicit return type declarations and type hints for all method parameters”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/ServiceDatabase.php` around lines 237 - 245, Add explicit BelongsTo return type declarations to the two relationship methods: change application() and application_preview() to declare : BelongsTo, and import Illuminate\Database\Eloquent\Relations\BelongsTo at the top of the file; ensure the methods still return $this->belongsTo(Application::class) and $this->belongsTo(ApplicationPreview::class) respectively so signatures read public function application(): BelongsTo and public function application_preview(): BelongsTo.
99-104:⚠️ Potential issue | 🟠 MajorRestart the database service container, not the app base container.
Application compose DB containers are named
<database-service-name>-<generated-app-base>. These branches omit$this->name, sodocker restarttargets the wrong/nonexistent container. The target must be terminated correctly.🐛 Proposed fix
} elseif ($this->application_id) { - $container_id = generateApplicationContainerName($this->application); + $container_id = $this->name.'-'.generateApplicationContainerName($this->application); $server = $this->application->destination->server; } elseif ($this->application_preview_id) { - $container_id = generateApplicationContainerName($this->application_preview->application, $this->application_preview->pull_request_id); + $container_id = $this->name.'-'.generateApplicationContainerName($this->application_preview->application, $this->application_preview->pull_request_id); $server = $this->application_preview->application->destination->server; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/ServiceDatabase.php` around lines 99 - 104, The code builds the app base container name but omits the database service name, so docker restart targets the wrong container; update both branches to prefix the generated app container name with $this->name (or otherwise include the DB service name) so the final container_id matches "<database-service-name>-<generated-app-base>" — e.g. set container_id = $this->name . '-' . generateApplicationContainerName($this->application) in the $this->application branch and container_id = $this->name . '-' . generateApplicationContainerName($this->application_preview->application, $this->application_preview->pull_request_id) in the $this->application_preview branch, leaving server assignment unchanged.resources/views/livewire/project/application/backups.blade.php (1)
21-21:⚠️ Potential issue | 🟠 MajorKey the create-backup component inside the loop.
This repeated Livewire child still has no unique key, so modal state can bleed between database cards. The machine remembers the wrong database; tacos do not fix that.
🐛 Proposed fix
- <livewire:project.database.create-scheduled-backup :database="$database" /> + <livewire:project.database.create-scheduled-backup :database="$database" :key="'create-scheduled-backup-'.$database->id" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/views/livewire/project/application/backups.blade.php` at line 21, The Livewire child component <livewire:project.database.create-scheduled-backup> is missing a unique key which lets modal state bleed between database cards; add a unique wire:key (e.g. wire:key="'create-scheduled-backup-'.$database->id") to the component tag (or use $loop->index if id unavailable) so Livewire can differentiate each instance and prevent state crossover.
🤖 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/Models/ScheduledDatabaseBackup.php`:
- Around line 11-16: Add explicit type hints: declare the model's static
booted() method with a void return (booted(): void), change the deleting
observer callback to accept the concrete model type and return void (function
(ScheduledDatabaseBackup $backup): void) and update the server() accessor to
declare a nullable Server return (?Server). Apply the same explicit parameter
and return type style to the related model methods referenced in this change
(e.g., server() and other methods in this model that currently lack type hints).
In
`@database/migrations/2026_04_19_034518_add_application_id_to_service_databases.php`:
- Line 26: The migration currently deletes records via
DB::table('service_databases')->whereNull('service_id')->delete(), which
bypasses the ServiceDatabase model and its deleting hooks
(ServiceDatabase::deleting), leaving backups and storages orphaned; change the
migration to load ServiceDatabase Eloquent models matching
whereNull('service_id') and delete them in chunks (e.g., chunkById or cursor
with batch deletes) so each model's delete() is invoked and cleanup hooks run,
avoiding memory spikes by processing batches rather than loading all records at
once.
In `@routes/web.php`:
- Line 232: The route Route::get('/backups', ApplicationConfiguration::class)
allows direct access for non-compose apps; update
ApplicationConfiguration::mount() to guard based on the application's build_pack
(check model->build_pack !== 'dockercompose') and perform the same
redirect/abort behavior used in the healthcheck guard (mirror the healthcheck
redirect logic) so non-dockercompose apps cannot mount the backups page; adjust
ApplicationConfiguration::mount() to detect this and redirect or abort
accordingly.
---
Outside diff comments:
In `@bootstrap/helpers/shared.php`:
- Around line 1921-2045: The closures that call $detectedDatabases->put() (the
service mapping closure around the service processing block and the second
compose parser closure later) do not have $detectedDatabases in their lexical
scope; add $detectedDatabases to each closure's use(...) list so the closures
can access and modify the shared collection, ensuring both places (the closure
that builds $savedService and the later compose parser closure that also writes
detected DBs) can call $detectedDatabases->put(...) without an undefined
variable error.
---
Duplicate comments:
In `@app/Models/ServiceDatabase.php`:
- Around line 237-245: Add explicit BelongsTo return type declarations to the
two relationship methods: change application() and application_preview() to
declare : BelongsTo, and import Illuminate\Database\Eloquent\Relations\BelongsTo
at the top of the file; ensure the methods still return
$this->belongsTo(Application::class) and
$this->belongsTo(ApplicationPreview::class) respectively so signatures read
public function application(): BelongsTo and public function
application_preview(): BelongsTo.
- Around line 99-104: The code builds the app base container name but omits the
database service name, so docker restart targets the wrong container; update
both branches to prefix the generated app container name with $this->name (or
otherwise include the DB service name) so the final container_id matches
"<database-service-name>-<generated-app-base>" — e.g. set container_id =
$this->name . '-' . generateApplicationContainerName($this->application) in the
$this->application branch and container_id = $this->name . '-' .
generateApplicationContainerName($this->application_preview->application,
$this->application_preview->pull_request_id) in the $this->application_preview
branch, leaving server assignment unchanged.
In `@bootstrap/helpers/applications.php`:
- Around line 154-170: Remove the redundant explicit type filter and keep the
whereHasMorph polymorphic check: in the ScheduledDatabaseBackupExecution query,
delete the ->where('database_type', \App\Models\ServiceDatabase::class) call and
rely solely on whereHasMorph('database', [\App\Models\ServiceDatabase::class],
...) to scope to ServiceDatabase; keep the application_preview relation usage
as-is (snake_case) inside the nested whereHas to check
application_id/pull_request_id.
In `@resources/views/livewire/project/application/backups.blade.php`:
- Line 21: The Livewire child component
<livewire:project.database.create-scheduled-backup> is missing a unique key
which lets modal state bleed between database cards; add a unique wire:key (e.g.
wire:key="'create-scheduled-backup-'.$database->id") to the component tag (or
use $loop->index if id unavailable) so Livewire can differentiate each instance
and prevent state crossover.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a52ac229-5891-4071-b3a4-5cccf4542723
📒 Files selected for processing (15)
app/Jobs/DatabaseBackupJob.phpapp/Livewire/Project/Application/Backups.phpapp/Models/Application.phpapp/Models/ApplicationPreview.phpapp/Models/ScheduledDatabaseBackup.phpapp/Models/ServiceDatabase.phpbootstrap/helpers/applications.phpbootstrap/helpers/database.phpbootstrap/helpers/databases.phpbootstrap/helpers/parsers.phpbootstrap/helpers/shared.phpdatabase/migrations/2026_04_19_034518_add_application_id_to_service_databases.phpresources/views/livewire/project/application/backups.blade.phpresources/views/livewire/project/application/configuration.blade.phproutes/web.php
AkprasadoP
force-pushed
the
feat/integrated-backups
branch
from
54338f9 to
14430c3
Compare
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
app/Models/ApplicationPreview.php (1)
36-42:⚠️ Potential issue | 🟠 MajorMove database deletion after the side-effectful parse.
Line 36 deletes preview databases, but Line 42 calls
parse(), and this PR makes compose parsing syncServiceDatabaserecords. That can recreate preview-owned DB rows after cleanup, leaving scheduled backups to be removed only by FK cascade without model hooks. Hasta la vista, cleanup guarantees.🛠️ Proposed ordering fix
static::forceDeleting(function ($preview) { - $preview->databases()->get()->each->delete(); $server = $preview->application->destination->server; $application = $preview->application; if (data_get($preview, 'application.build_pack') === 'dockercompose') { // Docker Compose volume and network cleanup $composeFile = $application->parse(pull_request_id: $preview->pull_request_id); @@ } + $preview->databases()->get()->each->delete(); + // Clean up persistent storage records $preview->persistentStorages()->delete(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/ApplicationPreview.php` around lines 36 - 42, The preview database rows are being deleted before $application->parse(...) is called, but parse() can synchronously recreate ServiceDatabase records for dockercompose; move the deletion of preview databases (the call to $preview->databases()->get()->each->delete()) to after $application->parse(pull_request_id: $preview->pull_request_id) (and any other side-effectful parse/compose handling) so that recreated records are cleaned up by the same model hooks; update the ApplicationPreview logic to call delete() only after parse() completes.bootstrap/helpers/parsers.php (1)
1444-1460:⚠️ Potential issue | 🟠 MajorTarget locked: preview-suffixed service name leaks into
ServiceDatabasesync — I'll be back... with duplicate records.At line 1445, when
$isPullRequest,$serviceNamegets suffixed viaaddPreviewDeploymentSuffix(...). Four lines later at 1450, you store the detected database under that suffixed key (e.g.,postgres-pr-42instead ofpostgres) before handing the collection toupdateResourceDatabases($resourceToSync, $detectedDatabases).The problem:
updateResourceDatabasesuses these keys directly asServiceDatabase.name(line 40 in database.php:'name' => $serviceName). This creates:
- Main deployment DB:
name = postgres- Preview deployment DB:
name = postgres-pr-42On prune logic, the function compares existing DB names against
$processedNamesarray—which contains the suffixed names. Fails to match the main deployment'spostgresagainst the preview'spostgres-pr-42, leaving stale rows behind like yesterday's cold, gluten-free tacos.Capture the unsuffixed name before mutation:
🔧 Proposed fix
$parsedServices->put($serviceName, $payload); if (isset($isDatabase) && $isDatabase) { - $detectedDatabases->put($serviceName, $service); + $originalServiceName = $isPullRequest + ? (string) str($serviceName)->before("-pr-{$pullRequestId}") + : $serviceName; + $detectedDatabases->put($originalServiceName, $service); } }Or—cleaner—move the detection capture up to before line 1445, so the suffix transformation only affects
$parsedServices.Also,
isset($isDatabase)on line 1449 is dead armor—$isDatabaseis unconditionally assigned at line 684 (every loop iteration, no branches). Drop theisset(...) &&half.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bootstrap/helpers/parsers.php` around lines 1444 - 1460, When $isPullRequest you currently mutate $serviceName via addPreviewDeploymentSuffix before adding the detected DB to $detectedDatabases, which causes updateResourceDatabases to receive preview-suffixed names (affecting ServiceDatabase.name/prune logic). Fix by capturing the original unsuffixed name (e.g., $rawServiceName = $serviceName) before calling addPreviewDeploymentSuffix and use $rawServiceName as the key when putting into $detectedDatabases (or move the $detectedDatabases->put call to before addPreviewDeploymentSuffix); also remove the redundant isset($isDatabase) check and just use if ($isDatabase) when adding to $detectedDatabases so the detected collection contains canonical service names passed into updateResourceDatabases.app/Models/ServiceDatabase.php (1)
41-62: 🛠️ Refactor suggestion | 🟠 MajorType the lifecycle hook and callbacks.
The changed
savingcallback should use the concrete model type andvoid;booted()should also declarevoid. As per coding guidelines, “Use explicit return type declarations and type hints for all method parameters”.♻️ Proposed fix
- protected static function booted() + protected static function booted(): void { - static::deleting(function ($service) { + static::deleting(function (ServiceDatabase $service): void { $service->persistentStorages()->delete(); $service->fileStorages()->delete(); $service->scheduledBackups()->delete(); }); - static::saving(function ($service) { + static::saving(function (ServiceDatabase $service): void { $ownerCount = collect([🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/ServiceDatabase.php` around lines 41 - 62, Add explicit return and parameter types: change the booted method signature to "protected static function booted(): void" and type-hint the lifecycle callbacks to accept the concrete model and return void, e.g. use "static::deleting(function (ServiceDatabase $service): void { ... });" and "static::saving(function (ServiceDatabase $service): void { ... });" so both closures and the booted method follow the project's explicit type-hinting guideline.bootstrap/helpers/shared.php (1)
1900-2045: 🧹 Nitpick | 🔵 TrivialRemove or wire the unused Service-branch accumulator.
$detectedDatabasesis populated in the Service branch but never read before this branch returns. Either call the sync helper here if stale Service-owned databases should be pruned too, or remove the accumulator so this does not look like backup-pruning Skynet code that never activates.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bootstrap/helpers/shared.php` around lines 1900 - 2045, The variable $detectedDatabases is being populated inside the services map closure but never used; either remove the accumulator ($detectedDatabases) or wire it into the pruning/sync flow by invoking the existing database sync helper (e.g., call the service-database prune/sync helper) after the services mapping completes (or inside the same closure where appropriate) so detected DBs are passed to the sync routine; update references to $detectedDatabases (creation, put calls) accordingly and ensure any helper you call accepts the same shape (service names => service defs) and runs before the function returns.app/Jobs/DatabaseBackupJob.php (1)
102-121:⚠️ Potential issue | 🟠 MajorValidate database before assigning to non-nullable property — this will throw a TypeError instead of the intended guard.
Line 42 declares
$databaseas a non-nullable union type. At lines 103 and 113,data_get($this->backup, 'database')can returnnull(missing relationship or key), which PHP 8.4 will reject immediately with aTypeErrorbefore your guard at line 120 ever runs. It's like deploying to serverless—things crash before they even start, except unlike serverless, you don't get a surprise VC-funded bill, you just get a grumpy error log and cold tacos.🐛 Proposed fix
if (data_get($this->backup, 'database_type') === ServiceDatabase::class) { - $this->database = data_get($this->backup, 'database'); + $database = data_get($this->backup, 'database'); + if (is_null($database)) { + throw new \Exception('Database not found?!'); + } + $this->database = $database; if ($this->database->service_id) { $this->server = $this->database->service->server; } elseif ($this->database->application_id) { @@ } else { - $this->database = data_get($this->backup, 'database'); + $database = data_get($this->backup, 'database'); + if (is_null($database)) { + throw new \Exception('Database not found?!'); + } + $this->database = $database; $this->server = $this->database->destination->server; $this->s3 = $this->backup->s3; } @@ - if (is_null($this->database)) { - throw new \Exception('Database not found?!'); - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Jobs/DatabaseBackupJob.php` around lines 102 - 121, In DatabaseBackupJob, avoid assigning the potentially null result of data_get($this->backup, 'database') directly to the non-nullable property $this->database; instead call data_get(...) into a local nullable variable first, validate it (throw the intended Exception or handle the missing case) before setting $this->database, and then proceed to compute $this->server and $this->s3 (the branches that check ServiceDatabase::class, service_id, application_id, application_preview_id should operate on the validated local variable). Ensure every path that assigns $this->database guarantees a non-null value so the runtime TypeError cannot occur.
♻️ Duplicate comments (4)
resources/views/livewire/project/application/backups.blade.php (2)
19-25:⚠️ Potential issue | 🟠 MajorLivewire
:keyis still missing oncreate-scheduled-backup— state bleed is unavoidable, I'll be back... with the wrong database.Inside the
@forelseloop, the<livewire:project.database.scheduled-backups>on line 25 correctly carries:key="$database->id", but the<livewire:project.database.create-scheduled-backup>on line 21 still doesn't. Livewire uses component keys to track instances across re-renders; without one, the#[Locked] public $databaseprop onCreateScheduledBackupcan bind to the wrong card when the collection reshuffles or a new DB is detected after parsing — a user could scheduled-backup a database they didn't click. No tacos for that outcome.Also, the two sibling components share an id space — recommend namespaced keys so they can't collide:
🔧 Proposed fix
`@can`('update', $database) - <x-modal-input buttonTitle="+ Add" title="New Scheduled Backup" canGate="update" :canResource="$database"> - <livewire:project.database.create-scheduled-backup :database="$database" /> + <x-modal-input buttonTitle="+ Add" title="New Scheduled Backup" canGate="update" :canResource="$database"> + <livewire:project.database.create-scheduled-backup :database="$database" :key="'create-scheduled-backup-'.$database->id" /> </x-modal-input> `@endcan` </div> - <livewire:project.database.scheduled-backups :database="$database" :key="$database->id" /> + <livewire:project.database.scheduled-backups :database="$database" :key="'scheduled-backups-'.$database->id" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/views/livewire/project/application/backups.blade.php` around lines 19 - 25, Add a unique Livewire key to the CreateScheduledBackup component to prevent state bleed: update the <livewire:project.database.create-scheduled-backup> invocation to include a namespaced :key (e.g. prefix with "create-" plus the database id) so it never collides with the sibling <livewire:project.database.scheduled-backups> (which already uses :key="$database->id"); target the livewire:project.database.create-scheduled-backup component and ensure the key uses $database->id (e.g. "create-{$database->id}") to uniquely identify instances.
19-19:⚠️ Potential issue | 🟠 MajorBoth
updateandmanageBackupsare guarding open doors — neither policy method enforces anything.The original concern stands:
@can('update', $database)is decorative (line 19). But the suggested fix has a problem: switching tomanageBackupswon't help becauseServiceDatabasePolicy::manageBackups()(line 65–67) also returnstrueunconditionally, just likeupdate()does (line 31–36).Even though
CreateScheduledBackup::submit()calls$this->authorize('manageBackups', ...)at line 47, the policy check is a no-op. Any authenticated user can trigger the Livewire action regardless of team boundaries.The real fix: Implement actual authorization in both policy methods. The codebase is already team-aware—
currentTeam()is used inmount()and the payload includesteam_id—but the policies aren't enforcing team membership. Per the coding guidelines and security patterns, implement team-based access control in both methods to verify the user belongs to the team that owns the database.Self-hosted servers need real security, not just the appearance of security. No serverless vendor is going to fix this for you while you sleep.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/views/livewire/project/application/backups.blade.php` at line 19, Both policy methods are effectively no-ops—update() and manageBackups() in ServiceDatabasePolicy—so implement real team-based authorization: have ServiceDatabasePolicy::update(User $user, ServiceDatabase $database) and ServiceDatabasePolicy::manageBackups(User $user, ServiceDatabase $database) verify the user belongs to the database's owning team (compare $user->currentTeam()->id or the user's team membership to $database->team_id) and return true only if membership matches; ensure the checks align with how mount() and CreateScheduledBackup::submit() pass/expect team_id so the existing $this->authorize('manageBackups', ...) call actually enforces team boundaries.app/Models/ServiceDatabase.php (2)
237-245: 🛠️ Refactor suggestion | 🟠 MajorType the new Eloquent relationships.
application()andapplication_preview()should declareBelongsToreturn types. This repeats the earlier relationship typing finding. As per coding guidelines, “Use explicit return type declarations and type hints for all method parameters”.♻️ Proposed fix
+use Illuminate\Database\Eloquent\Relations\BelongsTo;- public function application() + public function application(): BelongsTo { return $this->belongsTo(Application::class); } - public function application_preview() + public function application_preview(): BelongsTo { return $this->belongsTo(ApplicationPreview::class); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/ServiceDatabase.php` around lines 237 - 245, The two Eloquent relationship methods application() and application_preview() lack explicit return type declarations; update both methods to declare their return type as Illuminate\Database\Eloquent\Relations\BelongsTo (i.e., add ": BelongsTo" to application() and application_preview()), and add or ensure the corresponding use statement for BelongsTo is present at the top of ServiceDatabase.php so the type is resolved.
99-104:⚠️ Potential issue | 🟠 MajorRestart the actual compose database container.
Application compose containers are named with the service prefix, but these branches still target only the generated application base name. App-owned DB restarts will hit the wrong/nonexistent container. This repeats the earlier finding.
🐛 Proposed fix
} elseif ($this->application_id) { - $container_id = generateApplicationContainerName($this->application); + $container_id = $this->name.'-'.generateApplicationContainerName($this->application); $server = $this->application->destination->server; } elseif ($this->application_preview_id) { - $container_id = generateApplicationContainerName($this->application_preview->application, $this->application_preview->pull_request_id); + $container_id = $this->name.'-'.generateApplicationContainerName($this->application_preview->application, $this->application_preview->pull_request_id); $server = $this->application_preview->application->destination->server; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/ServiceDatabase.php` around lines 99 - 104, The branches handling $this->application_id and $this->application_preview_id build container_id with generateApplicationContainerName(...) which returns the base application container name, but for compose-managed DBs you must target the specific service container (application compose prefix + service name); update these branches in ServiceDatabase.php to construct the compose service container name (include the service/compose service identifier) — e.g. call the existing helper that produces a compose service container name or extend generateApplicationContainerName to accept the service name (use $this->service or $this->service->name / $this->application_preview->application and $this->application_preview->pull_request_id as needed) so $container_id points at the actual DB compose container before using $server to restart it.
🤖 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/Livewire/Project/Application/Backups.php`:
- Around line 11-25: The public Livewire property $application on class Backups
is currently writable from the client; mark it as immutable with the #[Locked]
attribute and add an authorization check inside mount() (e.g., authorize viewing
the given Application for the current user) before loading $this->databases to
prevent client-side tampering; update the Backups component to apply #[Locked]
to the Application $application property and call the appropriate authorization
gate/policy in mount() (then only load $this->databases after authorization
succeeds).
In `@app/Models/ScheduledDatabaseBackup.php`:
- Around line 112-114: The code dereferences
$this->database->application_preview directly which can throw if the preview
relationship is missing; update the branch in the ScheduledDatabaseBackup logic
to null-check the preview before accessing its properties (e.g., check
$this->database->application_preview exists or use data_get with a dotted path
like 'application_preview.application.destination') so that $destination and
$server are resolved safely and become null when the preview row is
stale/missing.
In `@app/Models/ServiceDatabase.php`:
- Around line 111-123: Add explicit nullable return type declarations to the
changed owner resolver methods: update getServerAttribute to declare a nullable
Server return (e.g., getServerAttribute(): ?Server), getServiceDatabaseUrl to
return ?string, team to return ?Team, and workdir to return ?string; locate
these methods in ServiceDatabase (and the other occurrences referenced around
lines 182-229) and add the appropriate ?Type return annotations to each method
signature while keeping the existing logic and null return at the end.
In `@bootstrap/helpers/applications.php`:
- Around line 154-170: The guard that blocks deployment uses literal status
strings and a redundant where('database_type', ...) check; add STATUS_RUNNING,
STATUS_SUCCESS, STATUS_FAILED constants to the ScheduledDatabaseBackupExecution
model and switch DatabaseBackupJob to use those constants instead of
'running'/'success'/'failed'; remove the redundant ->where('database_type',
\App\Models\ServiceDatabase::class) when you already restrict types via
whereHasMorph; finally, when DatabaseBackupJob completes (both success and
failure paths) dispatch a job/event or call the existing queue trigger (e.g.,
invoke queue_next_deployment($application_id) or fire the same event that
next_queuable() listens to) so pending deployments for the affected
application_id are re-evaluated.
In `@bootstrap/helpers/parsers.php`:
- Around line 1453-1460: Add a warning log when a pull-request preview lookup
fails so updateResourceDatabases() isn’t silently skipped: inside the branch
that sets $resourceToSync via $resource->previews()->where('pull_request_id',
$pullRequestId)->first() (used in parse()/applicationParser() flow), detect when
$resourceToSync is null and call Log::warning with context (e.g., 'Preview not
found for application parsing' plus application_id => $resource->id and
pull_request_id => $pullRequestId) before falling through to the existing
$resourceToSync check that calls updateResourceDatabases($resourceToSync,
$detectedDatabases).
In
`@database/migrations/2026_04_19_034518_add_application_id_to_service_databases.php`:
- Around line 24-38: The down() migration currently re-declares the service_id
foreign key before dropping other FKs which can cause duplicate constraint
errors; modify the ServiceDatabase rollback to first drop the foreign key on
service_id (dropForeign(['service_id'])) before calling change() on the column,
then drop the application_id and application_preview_id FKs and columns as
shown; also add a top-level import "use App\Models\ServiceDatabase;" and replace
the FQN \App\Models\ServiceDatabase::query() with ServiceDatabase::query() to
clean up the file.
In `@routes/web.php`:
- Line 232: The download resolver currently resolves ServiceDatabase via
database->service which is null for application/preview-owned DBs causing 500s;
update the backups download logic (the resolver used by Route::get('/backups',
ApplicationConfiguration::class) and the download handler that references
ServiceDatabase/database->service) to use the owner-aware server accessor used
in retention cleanup (use the same server() or owner-aware accessor instead of
database->service when locating the backing server) so application/preview-owned
databases resolve correctly.
---
Outside diff comments:
In `@app/Jobs/DatabaseBackupJob.php`:
- Around line 102-121: In DatabaseBackupJob, avoid assigning the potentially
null result of data_get($this->backup, 'database') directly to the non-nullable
property $this->database; instead call data_get(...) into a local nullable
variable first, validate it (throw the intended Exception or handle the missing
case) before setting $this->database, and then proceed to compute $this->server
and $this->s3 (the branches that check ServiceDatabase::class, service_id,
application_id, application_preview_id should operate on the validated local
variable). Ensure every path that assigns $this->database guarantees a non-null
value so the runtime TypeError cannot occur.
In `@app/Models/ApplicationPreview.php`:
- Around line 36-42: The preview database rows are being deleted before
$application->parse(...) is called, but parse() can synchronously recreate
ServiceDatabase records for dockercompose; move the deletion of preview
databases (the call to $preview->databases()->get()->each->delete()) to after
$application->parse(pull_request_id: $preview->pull_request_id) (and any other
side-effectful parse/compose handling) so that recreated records are cleaned up
by the same model hooks; update the ApplicationPreview logic to call delete()
only after parse() completes.
In `@app/Models/ServiceDatabase.php`:
- Around line 41-62: Add explicit return and parameter types: change the booted
method signature to "protected static function booted(): void" and type-hint the
lifecycle callbacks to accept the concrete model and return void, e.g. use
"static::deleting(function (ServiceDatabase $service): void { ... });" and
"static::saving(function (ServiceDatabase $service): void { ... });" so both
closures and the booted method follow the project's explicit type-hinting
guideline.
In `@bootstrap/helpers/parsers.php`:
- Around line 1444-1460: When $isPullRequest you currently mutate $serviceName
via addPreviewDeploymentSuffix before adding the detected DB to
$detectedDatabases, which causes updateResourceDatabases to receive
preview-suffixed names (affecting ServiceDatabase.name/prune logic). Fix by
capturing the original unsuffixed name (e.g., $rawServiceName = $serviceName)
before calling addPreviewDeploymentSuffix and use $rawServiceName as the key
when putting into $detectedDatabases (or move the $detectedDatabases->put call
to before addPreviewDeploymentSuffix); also remove the redundant
isset($isDatabase) check and just use if ($isDatabase) when adding to
$detectedDatabases so the detected collection contains canonical service names
passed into updateResourceDatabases.
In `@bootstrap/helpers/shared.php`:
- Around line 1900-2045: The variable $detectedDatabases is being populated
inside the services map closure but never used; either remove the accumulator
($detectedDatabases) or wire it into the pruning/sync flow by invoking the
existing database sync helper (e.g., call the service-database prune/sync
helper) after the services mapping completes (or inside the same closure where
appropriate) so detected DBs are passed to the sync routine; update references
to $detectedDatabases (creation, put calls) accordingly and ensure any helper
you call accepts the same shape (service names => service defs) and runs before
the function returns.
---
Duplicate comments:
In `@app/Models/ServiceDatabase.php`:
- Around line 237-245: The two Eloquent relationship methods application() and
application_preview() lack explicit return type declarations; update both
methods to declare their return type as
Illuminate\Database\Eloquent\Relations\BelongsTo (i.e., add ": BelongsTo" to
application() and application_preview()), and add or ensure the corresponding
use statement for BelongsTo is present at the top of ServiceDatabase.php so the
type is resolved.
- Around line 99-104: The branches handling $this->application_id and
$this->application_preview_id build container_id with
generateApplicationContainerName(...) which returns the base application
container name, but for compose-managed DBs you must target the specific service
container (application compose prefix + service name); update these branches in
ServiceDatabase.php to construct the compose service container name (include the
service/compose service identifier) — e.g. call the existing helper that
produces a compose service container name or extend
generateApplicationContainerName to accept the service name (use $this->service
or $this->service->name / $this->application_preview->application and
$this->application_preview->pull_request_id as needed) so $container_id points
at the actual DB compose container before using $server to restart it.
In `@resources/views/livewire/project/application/backups.blade.php`:
- Around line 19-25: Add a unique Livewire key to the CreateScheduledBackup
component to prevent state bleed: update the
<livewire:project.database.create-scheduled-backup> invocation to include a
namespaced :key (e.g. prefix with "create-" plus the database id) so it never
collides with the sibling <livewire:project.database.scheduled-backups> (which
already uses :key="$database->id"); target the
livewire:project.database.create-scheduled-backup component and ensure the key
uses $database->id (e.g. "create-{$database->id}") to uniquely identify
instances.
- Line 19: Both policy methods are effectively no-ops—update() and
manageBackups() in ServiceDatabasePolicy—so implement real team-based
authorization: have ServiceDatabasePolicy::update(User $user, ServiceDatabase
$database) and ServiceDatabasePolicy::manageBackups(User $user, ServiceDatabase
$database) verify the user belongs to the database's owning team (compare
$user->currentTeam()->id or the user's team membership to $database->team_id)
and return true only if membership matches; ensure the checks align with how
mount() and CreateScheduledBackup::submit() pass/expect team_id so the existing
$this->authorize('manageBackups', ...) call actually enforces team boundaries.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: cd45652a-1f83-4933-b791-b0cfbf325962
📒 Files selected for processing (16)
app/Jobs/DatabaseBackupJob.phpapp/Livewire/Project/Application/Backups.phpapp/Livewire/Project/Application/Configuration.phpapp/Models/Application.phpapp/Models/ApplicationPreview.phpapp/Models/ScheduledDatabaseBackup.phpapp/Models/ServiceDatabase.phpbootstrap/helpers/applications.phpbootstrap/helpers/database.phpbootstrap/helpers/databases.phpbootstrap/helpers/parsers.phpbootstrap/helpers/shared.phpdatabase/migrations/2026_04_19_034518_add_application_id_to_service_databases.phpresources/views/livewire/project/application/backups.blade.phpresources/views/livewire/project/application/configuration.blade.phproutes/web.php
AkprasadoP
force-pushed
the
feat/integrated-backups
branch
from
14430c3 to
a1bfa48
Compare
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 (3)
app/Models/Application.php (1)
344-355:⚠️ Potential issue | 🟠 MajorForce-delete application previews to avoid orphaned preview-owned databases.
Line 345 deletes databases owned by the application itself, but line 355 soft-deletes previews with
.delete()— this meansApplicationPreview::forceDeleting()won't run, and preview-owned databases survive the force-delete like a T-800 without a power cell. The database-level cascades exist (migration showsonDelete('cascade')forapplication_preview_id), but they don't help: soft-deleted previews still exist with adeleted_attimestamp, so the cascades never trigger. Preview databases and their scheduled backups get stranded.Use
forceDelete()instead ofdelete()on previews, or add explicit cleanup:Proposed fix
static::forceDeleting(function ($application) { $application->databases()->get()->each->delete(); + $application->previews()->get()->each->forceDelete(); $application->update(['fqdn' => null]);Or iterate manually:
+ $application->previews()->get()->each(function (ApplicationPreview $preview): void { + $preview->databases()->get()->each->delete(); + $preview->forceDelete(); + });Self-hosting means you manage your own data properly — don't leave orphaned records behind.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/Application.php` around lines 344 - 355, In the Application model's static::forceDeleting closure, ensure previews are permanently removed so their ApplicationPreview::forceDeleting hooks run and preview-owned databases/backups aren’t orphaned: replace the current soft-delete call on the previews relation ($application->previews()->delete()) with an explicit force-delete of each preview (e.g., fetch previews and call forceDelete on each) so the previews' forceDeleting logic and DB cascades execute; update references in the forceDeleting closure that manage previews accordingly.app/Models/ScheduledDatabaseBackup.php (1)
101-125:⚠️ Potential issue | 🟠 MajorUse the owner-aware
ServiceDatabase->serveraccessor.Line 107 resolves service-owned databases through
service.destination.server, butServiceDatabaseexposes the correct path asservice.server. This can make existing service database backups look serverless — null and disappointing.🐛 Proposed fix
public function server(): ?Server { - if ($this->database) { - $server = null; - if ($this->database instanceof ServiceDatabase) { - if ($this->database->service_id) { - $destination = data_get($this->database->service, 'destination'); - $server = data_get($destination, 'server'); - } elseif ($this->database->application_id) { - $destination = data_get($this->database->application, 'destination'); - $server = data_get($destination, 'server'); - } elseif ($this->database->application_preview_id) { - $destination = data_get($this->database->application_preview?->application, 'destination'); - $server = data_get($destination, 'server'); - } - } else { - $destination = data_get($this->database, 'destination'); - $server = data_get($destination, 'server'); - } - if ($server) { - return $server; - } + if (! $this->database) { + return null; } - return null; + if ($this->database instanceof ServiceDatabase) { + return $this->database->server; + } + + return data_get($this->database, 'destination.server'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/ScheduledDatabaseBackup.php` around lines 101 - 125, The server() method should use the owner-aware accessor on ServiceDatabase instead of drilling into service.destination.server; in ScheduledDatabaseBackup::server(), replace the conditional branches inside the ServiceDatabase check (the blocks that reference $this->database->service->destination->server, $this->database->application->destination->server, and $this->database->application_preview?->application->destination->server) with a single assignment using $this->database->server so ServiceDatabase->server is relied on; keep the existing fallback that reads destination->server for non-ServiceDatabase instances.bootstrap/helpers/parsers.php (1)
1444-1451:⚠️ Potential issue | 🟠 MajorKeep detected database names unsuffixed for PR previews.
Line 1450 stores the preview-suffixed service key as the
ServiceDatabase.name. The parser’s actualcontainer_namewas built earlier from the original service name, andDatabaseBackupJoblater targets{$database->name}-generateApplicationContainerName(...); this turns preview backups into a decoy-container hunt. Store the original compose service name and letapplication_preview_idprovide the scoping.🐛 Proposed fix
+ $databaseServiceName = $serviceName; if ($isPullRequest) { $serviceName = addPreviewDeploymentSuffix($serviceName, $pullRequestId); } $parsedServices->put($serviceName, $payload); if (isset($isDatabase) && $isDatabase) { - $detectedDatabases->put($serviceName, $service); + $detectedDatabases->put($databaseServiceName, $service); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bootstrap/helpers/parsers.php` around lines 1444 - 1451, The detected database entry is being stored with the PR preview suffix, breaking later container_name lookups; before calling addPreviewDeploymentSuffix($serviceName, $pullRequestId) capture the original compose service name (e.g. $originalServiceName = $serviceName), continue to use the suffixed $serviceName for $parsedServices->put, but use the unsuffixed original (use $originalServiceName or the value used to build $service['container_name']) when calling $detectedDatabases->put($serviceName, $service) so detected database records keep the original compose name while preview scoping remains provided by application_preview_id. Ensure references to addPreviewDeploymentSuffix, $parsedServices->put and $detectedDatabases->put are updated accordingly.
♻️ Duplicate comments (5)
resources/views/livewire/project/application/backups.blade.php (1)
19-22:⚠️ Potential issue | 🟠 MajorAdd a unique key to the create-backup Livewire child.
This component is rendered inside
@forelse; without a key, Livewire can reuse state between database cards and create a backup against the wrong target. That is not a taco mix-up; that is production shrapnel.Key the child component per database
- <livewire:project.database.create-scheduled-backup :database="$database" /> + <livewire:project.database.create-scheduled-backup :database="$database" :key="'create-scheduled-backup-'.$database->id" />Run this read-only check to confirm both looped child components have non-colliding keys:
#!/bin/bash rg -n -C2 \ '<livewire:project.database.create-scheduled-backup|<livewire:project.database.scheduled-backups|:key=' \ resources/views/livewire/project/application/backups.blade.php🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/views/livewire/project/application/backups.blade.php` around lines 19 - 22, The Livewire child <livewire:project.database.create-scheduled-backup> (and likewise <livewire:project.database.scheduled-backups> if present) is rendered inside an `@forelse` loop without a unique key, causing Livewire to reuse component state across different database cards; add a per-database :key binding (for example using the database id or uuid) to the Livewire component tags (e.g., :key="$database->id" or :key="'create-backup-'.$database->id") so each instance is uniquely identified and state is not shared between rows.bootstrap/helpers/applications.php (1)
154-165: 🛠️ Refactor suggestion | 🟠 MajorUse the new backup status constant here too.
ScheduledDatabaseBackupExecution::STATUS_RUNNINGnow exists, but this guard still hard-codes'running'. One typo later, deployments punch through active backups; the machines do not approve.Replace the magic string
- $running_backup = \App\Models\ScheduledDatabaseBackupExecution::where('status', 'running') + $running_backup = \App\Models\ScheduledDatabaseBackupExecution::where( + 'status', + \App\Models\ScheduledDatabaseBackupExecution::STATUS_RUNNING + ) ->whereHas('scheduledDatabaseBackup', function ($query) use ($application_id, $pull_request_id) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bootstrap/helpers/applications.php` around lines 154 - 165, Replace the hard-coded status string 'running' with the new constant ScheduledDatabaseBackupExecution::STATUS_RUNNING in the query that sets $running_backup; locate the call to \App\Models\ScheduledDatabaseBackupExecution::where('running') and change it to use \App\Models\ScheduledDatabaseBackupExecution::STATUS_RUNNING (keeping the fully-qualified class reference used elsewhere in the expression) so the guard relies on the canonical constant instead of a magic string.app/Models/ServiceDatabase.php (2)
5-6: 🛠️ Refactor suggestion | 🟠 MajorType the new owner relationships.
application()andapplication_preview()are new Eloquent relationships but still lackBelongsToreturn types. As per coding guidelines, “Use explicit return type declarations and type hints for all method parameters”.♻️ Proposed fix
use Illuminate\Database\Eloquent\Factories\HasFactory; +use Illuminate\Database\Eloquent\Relations\BelongsTo; use Illuminate\Database\Eloquent\SoftDeletes;- public function application() + public function application(): BelongsTo { return $this->belongsTo(Application::class); } - public function application_preview() + public function application_preview(): BelongsTo { return $this->belongsTo(ApplicationPreview::class); }Also applies to: 237-245
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/ServiceDatabase.php` around lines 5 - 6, Add explicit BelongsTo return type hints to the new Eloquent relationship methods (application() and application_preview()) in ServiceDatabase.php and the duplicate methods around lines 237-245: import Illuminate\Database\Eloquent\Relations\BelongsTo at the top and change each relationship method signature to declare the return type BelongsTo (e.g., function application(): BelongsTo and function application_preview(): BelongsTo) while keeping the existing relationship body intact.
99-104:⚠️ Potential issue | 🟠 MajorRestart the database service container, not the application base.
Lines 100 and 103 omit
$this->name.'-', so application-owned DB restarts target the base app container instead of the compose DB container. The Terminator does not restart decoys.🐛 Proposed fix
} elseif ($this->application_id) { - $container_id = generateApplicationContainerName($this->application); + $container_id = $this->name.'-'.generateApplicationContainerName($this->application); $server = $this->application->destination->server; } elseif ($this->application_preview_id) { - $container_id = generateApplicationContainerName($this->application_preview->application, $this->application_preview->pull_request_id); + $container_id = $this->name.'-'.generateApplicationContainerName($this->application_preview->application, $this->application_preview->pull_request_id); $server = $this->application_preview->application->destination->server; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/ServiceDatabase.php` around lines 99 - 104, The restart targets the application base because the container_id built in the application-owned branches omits the application-specific prefix; update the two branches that call generateApplicationContainerName (the ones using $this->application and $this->application_preview) to prepend $this->name . '-' when constructing the container id so the Terminator restarts the application-owned DB compose container (i.e., use $this->name . '-' with generateApplicationContainerName for both the $this->application and $this->application_preview paths).bootstrap/helpers/database.php (1)
41-52:⚠️ Potential issue | 🟠 MajorClear inactive owner columns during sync.
Line 45 sets the active owner but leaves the other owner columns untouched on update. If an existing row has stale owner IDs, the
ServiceDatabasesaving guard will terminate the sync with “exactly one owner” — good guard, bad ambush.🛡️ Proposed fix
$data = [ + 'service_id' => null, + 'application_id' => null, + 'application_preview_id' => null, 'name' => $serviceName, 'image' => $image, 'ports' => $collectedPorts, $column => $resourceId, ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bootstrap/helpers/database.php` around lines 41 - 52, When updating an existing ServiceDatabase row, the code currently sets only the active owner column ($column => $resourceId) and leaves other owner columns untouched, which can leave stale owner IDs and trigger the "exactly one owner" guard; modify the update path so $data explicitly clears all other owner columns (set them to null) except the active $column before calling $databaseRecord->update($data), i.e., ensure the $data array includes every owner column present on ServiceDatabase with null values for the inactive ones so exactly one owner remains set; keep the create path (ServiceDatabase::create($data)) unchanged or ensure it also only sets the single owner.
🤖 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/Jobs/DatabaseBackupJob.php`:
- Around line 532-537: The code is passing an integer ID ($applicationId) to
queue_next_deployment(), but that function expects an Application model; change
the block in DatabaseBackupJob (where $this->database instanceof ServiceDatabase
and $this->database->application_id || $this->database->application_preview_id)
to load the Application model (e.g. via Application::find($applicationId) or
resolving from $this->database->application_preview->application) and pass that
Application instance to queue_next_deployment(), with a null check to avoid
calling the function when the Application record isn't found.
In `@bootstrap/helpers/shared.php`:
- Line 1900: Remove the unused $detectedDatabases accumulator from the Service
parsing branch: either stop initializing and populating $detectedDatabases in
the Service branch (remove the collect([]) assignment and the pushes at the
Service branch population site) OR instead pass the populated collection into
updateResourceDatabases($resource, $detectedDatabases) so it is consumed like
the Application branch; locate the Service branch where ServiceDatabase rows are
created/updated (symbol: ServiceDatabase) and the initialization of
$detectedDatabases, then either delete all $detectedDatabases references in that
branch or add a call to updateResourceDatabases(...) before returning
collect($finalServices) to maintain symmetry with the Application flow (symbol:
collect($finalServices)).
In
`@database/migrations/2026_04_19_034518_add_application_id_to_service_databases.php`:
- Around line 15-19: The up() migration is trying to call ->change() with
->constrained() on service_id which causes duplicate FK errors on MySQL; mirror
the safe drop-then-re-add pattern used in down(): first drop the existing
foreign key for service_id (use dropForeign on the service_id constraint), then
alter the column (make it nullable) and finally re-create the foreign key with
foreignId(...)->constrained()->onDelete('cascade'); update the up() method in
the AddApplicationIdToServiceDatabases migration to perform dropForeign ->
change column -> re-add constrained FK for service_id rather than chaining
->constrained() onto ->change().
---
Outside diff comments:
In `@app/Models/Application.php`:
- Around line 344-355: In the Application model's static::forceDeleting closure,
ensure previews are permanently removed so their
ApplicationPreview::forceDeleting hooks run and preview-owned databases/backups
aren’t orphaned: replace the current soft-delete call on the previews relation
($application->previews()->delete()) with an explicit force-delete of each
preview (e.g., fetch previews and call forceDelete on each) so the previews'
forceDeleting logic and DB cascades execute; update references in the
forceDeleting closure that manage previews accordingly.
In `@app/Models/ScheduledDatabaseBackup.php`:
- Around line 101-125: The server() method should use the owner-aware accessor
on ServiceDatabase instead of drilling into service.destination.server; in
ScheduledDatabaseBackup::server(), replace the conditional branches inside the
ServiceDatabase check (the blocks that reference
$this->database->service->destination->server,
$this->database->application->destination->server, and
$this->database->application_preview?->application->destination->server) with a
single assignment using $this->database->server so ServiceDatabase->server is
relied on; keep the existing fallback that reads destination->server for
non-ServiceDatabase instances.
In `@bootstrap/helpers/parsers.php`:
- Around line 1444-1451: The detected database entry is being stored with the PR
preview suffix, breaking later container_name lookups; before calling
addPreviewDeploymentSuffix($serviceName, $pullRequestId) capture the original
compose service name (e.g. $originalServiceName = $serviceName), continue to use
the suffixed $serviceName for $parsedServices->put, but use the unsuffixed
original (use $originalServiceName or the value used to build
$service['container_name']) when calling $detectedDatabases->put($serviceName,
$service) so detected database records keep the original compose name while
preview scoping remains provided by application_preview_id. Ensure references to
addPreviewDeploymentSuffix, $parsedServices->put and $detectedDatabases->put are
updated accordingly.
---
Duplicate comments:
In `@app/Models/ServiceDatabase.php`:
- Around line 5-6: Add explicit BelongsTo return type hints to the new Eloquent
relationship methods (application() and application_preview()) in
ServiceDatabase.php and the duplicate methods around lines 237-245: import
Illuminate\Database\Eloquent\Relations\BelongsTo at the top and change each
relationship method signature to declare the return type BelongsTo (e.g.,
function application(): BelongsTo and function application_preview(): BelongsTo)
while keeping the existing relationship body intact.
- Around line 99-104: The restart targets the application base because the
container_id built in the application-owned branches omits the
application-specific prefix; update the two branches that call
generateApplicationContainerName (the ones using $this->application and
$this->application_preview) to prepend $this->name . '-' when constructing the
container id so the Terminator restarts the application-owned DB compose
container (i.e., use $this->name . '-' with generateApplicationContainerName for
both the $this->application and $this->application_preview paths).
In `@bootstrap/helpers/applications.php`:
- Around line 154-165: Replace the hard-coded status string 'running' with the
new constant ScheduledDatabaseBackupExecution::STATUS_RUNNING in the query that
sets $running_backup; locate the call to
\App\Models\ScheduledDatabaseBackupExecution::where('running') and change it to
use \App\Models\ScheduledDatabaseBackupExecution::STATUS_RUNNING (keeping the
fully-qualified class reference used elsewhere in the expression) so the guard
relies on the canonical constant instead of a magic string.
In `@bootstrap/helpers/database.php`:
- Around line 41-52: When updating an existing ServiceDatabase row, the code
currently sets only the active owner column ($column => $resourceId) and leaves
other owner columns untouched, which can leave stale owner IDs and trigger the
"exactly one owner" guard; modify the update path so $data explicitly clears all
other owner columns (set them to null) except the active $column before calling
$databaseRecord->update($data), i.e., ensure the $data array includes every
owner column present on ServiceDatabase with null values for the inactive ones
so exactly one owner remains set; keep the create path
(ServiceDatabase::create($data)) unchanged or ensure it also only sets the
single owner.
In `@resources/views/livewire/project/application/backups.blade.php`:
- Around line 19-22: The Livewire child
<livewire:project.database.create-scheduled-backup> (and likewise
<livewire:project.database.scheduled-backups> if present) is rendered inside an
`@forelse` loop without a unique key, causing Livewire to reuse component state
across different database cards; add a per-database :key binding (for example
using the database id or uuid) to the Livewire component tags (e.g.,
:key="$database->id" or :key="'create-backup-'.$database->id") so each instance
is uniquely identified and state is not shared between rows.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ff0a6a92-a498-4a9e-833a-8d44950d5499
📒 Files selected for processing (17)
app/Jobs/DatabaseBackupJob.phpapp/Livewire/Project/Application/Backups.phpapp/Livewire/Project/Application/Configuration.phpapp/Models/Application.phpapp/Models/ApplicationPreview.phpapp/Models/ScheduledDatabaseBackup.phpapp/Models/ScheduledDatabaseBackupExecution.phpapp/Models/ServiceDatabase.phpbootstrap/helpers/applications.phpbootstrap/helpers/database.phpbootstrap/helpers/databases.phpbootstrap/helpers/parsers.phpbootstrap/helpers/shared.phpdatabase/migrations/2026_04_19_034518_add_application_id_to_service_databases.phpresources/views/livewire/project/application/backups.blade.phpresources/views/livewire/project/application/configuration.blade.phproutes/web.php
AkprasadoP
force-pushed
the
feat/integrated-backups
branch
from
a1bfa48 to
69145a0
Compare
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
bootstrap/helpers/applications.php (1)
154-165: 🧹 Nitpick | 🔵 TrivialUse the backup status constant in this queue gate.
ScheduledDatabaseBackupExecution::STATUS_RUNNINGexists now, but Line 154 still hard-codes'running'. Keep the guard tied to the model constant so a future status rename does not quietly unfreeze Skynet’s deployment queue.♻️ Proposed cleanup
+use App\Models\ScheduledDatabaseBackupExecution; +use App\Models\ServiceDatabase; + @@ - $running_backup = \App\Models\ScheduledDatabaseBackupExecution::where('status', 'running') + $running_backup = ScheduledDatabaseBackupExecution::where('status', ScheduledDatabaseBackupExecution::STATUS_RUNNING) ->whereHas('scheduledDatabaseBackup', function ($query) use ($application_id, $pull_request_id) { - $query->whereHasMorph('database', [\App\Models\ServiceDatabase::class], function ($q) use ($application_id, $pull_request_id) { + $query->whereHasMorph('database', [ServiceDatabase::class], function ($q) use ($application_id, $pull_request_id) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bootstrap/helpers/applications.php` around lines 154 - 165, The query is using the literal 'running' status string; update the status check to use the model constant ScheduledDatabaseBackupExecution::STATUS_RUNNING instead so it reads where('status', ScheduledDatabaseBackupExecution::STATUS_RUNNING). Locate the $running_backup query that begins with \App\Models\ScheduledDatabaseBackupExecution::where(...) and replace the hard-coded 'running' with the constant to keep the guard in sync with ScheduledDatabaseBackupExecution.app/Models/Application.php (1)
345-355:⚠️ Potential issue | 🟠 MajorForce-delete previews through model instances before the cascade deletes their databases.
Line 345 cleans only direct application databases. Preview-owned databases still depend on
ApplicationPreview::forceDeleting, but Line 355 bulk-deletes previews through the relationship, so preview model hooks can be skipped andServiceDatabase::deletingcleanup may not run before FK cascades hard-delete the rows. Delete/force-delete preview models individually first, so their database cleanup fires.🛠️ Proposed fix
static::forceDeleting(function ($application) { $application->databases()->get()->each->delete(); + $application->previews()->get()->each->forceDelete(); $application->update(['fqdn' => null]); $application->settings()->delete(); $application->persistentStorages()->delete(); @@ - $application->previews()->delete(); foreach ($application->deployment_queue as $deployment) { $deployment->delete(); } });As per coding guidelines, database work should follow Eloquent ORM patterns, migration best practices, relationship definitions, and query optimization as documented in
.ai/patterns/database-patterns.md.#!/bin/bash # Verify the application deletion path and preview/database cleanup hooks. rg -nP -C4 '\$application->previews\(\)->delete\(\)|\$application->previews\(\)->get\(\)->each|static::forceDeleting|databases\(\)->get\(\)->each->delete' \ app/Models/Application.php app/Models/ApplicationPreview.php app/Models/ServiceDatabase.php🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/Application.php` around lines 345 - 355, The previews are being bulk-deleted via $application->previews()->delete(), which skips model-level hooks (including ApplicationPreview::forceDeleting) so ServiceDatabase::deleting cleanup can be missed; change the code to individually force-delete preview model instances before you cascade-delete databases—i.e. replace the bulk $application->previews()->delete() with something like $application->previews()->get()->each->forceDelete() and ensure that call runs before the database cleanup lines ($application->databases()->get()->each->delete() / ServiceDatabase::deleting).resources/views/livewire/project/application/backups.blade.php (1)
21-21:⚠️ Potential issue | 🟡 MinorAdd a unique key to terminate Livewire state bleed.
Line 21 mounts this child inside a database loop without a stable key, so modal state can attach to the wrong database instance. State bleed is not Skynet-approved.
Proposed fix
- <livewire:project.database.create-scheduled-backup :database="$database" /> + <livewire:project.database.create-scheduled-backup :database="$database" :key="'create-scheduled-backup-'.$database->id" />Run this read-only check to verify the component is keyed:
#!/bin/bash # Expect the create-scheduled-backup component inside the loop to include a unique :key. rg -n -C2 '<livewire:project\.database\.create-scheduled-backup|:key=' resources/views/livewire/project/application/backups.blade.php🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/views/livewire/project/application/backups.blade.php` at line 21, The Livewire child <livewire:project.database.create-scheduled-backup> is being mounted inside a loop over $database without a stable key, causing state bleed; fix by adding a unique, stable :key based on the database identifier (for example :key="$database->id" or :key="$database->uuid") on the <livewire:project.database.create-scheduled-backup> tag so Livewire can differentiate instances; ensure you use the persistent unique property from the $database model (not the loop index) when adding the key.app/Models/ServiceDatabase.php (2)
237-245: 🛠️ Refactor suggestion | 🟠 MajorType the new Eloquent relationships.
application()andapplication_preview()still need explicitBelongsToreturns; otherwise this remains out of policy and static analysis keeps growling like a T-800 denied tacos.♻️ Proposed fix
+use Illuminate\Database\Eloquent\Relations\BelongsTo; + class ServiceDatabase extends BaseModel- public function application() + public function application(): BelongsTo { return $this->belongsTo(Application::class); } - public function application_preview() + public function application_preview(): BelongsTo { return $this->belongsTo(ApplicationPreview::class); }#!/bin/bash # Description: Verify the new relationship methods are typed and find preview-relation references # before deciding whether to keep/suppress the snake_case method name or rename it project-wide. # Expected: relationship methods declare BelongsTo; any rename plan accounts for all references. rg -n -C3 --type=php 'function\s+(application|application_preview)\s*\(' app/Models/ServiceDatabase.php rg -n -C3 --type=php '\bapplication_preview\s*\(|->application_preview\b|whereRelation\('\''application_preview'\'')'As per coding guidelines, “Use explicit return type declarations and type hints for all method parameters”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/ServiceDatabase.php` around lines 237 - 245, The two relationship methods application() and application_preview() lack explicit return types; update both to declare and return Illuminate\Database\Eloquent\Relations\BelongsTo (add a use statement for BelongsTo at the top) so their signatures read with a BelongsTo return type, and ensure the body still returns $this->belongsTo(...) unchanged; after typing, run the provided ripgrep checks for application_preview references before any rename of the snake_case method.
99-104:⚠️ Potential issue | 🟠 MajorRestart the actual compose database container.
For app-owned compose databases,
generateApplicationContainerName(...)gives the app base name, but the database container is service-scoped. Prefix the compose DB service name here too, ordocker restartwill target the wrong container while the real database keeps calmly self-hosting without you.🐛 Proposed fix
} elseif ($this->application_id) { - $container_id = generateApplicationContainerName($this->application); + $container_id = $this->name.'-'.generateApplicationContainerName($this->application); $server = $this->application->destination->server; } elseif ($this->application_preview_id) { - $container_id = generateApplicationContainerName($this->application_preview->application, $this->application_preview->pull_request_id); + $container_id = $this->name.'-'.generateApplicationContainerName($this->application_preview->application, $this->application_preview->pull_request_id); $server = $this->application_preview->application->destination->server; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/ServiceDatabase.php` around lines 99 - 104, When handling app-owned compose databases in ServiceDatabase.php (the branches using $this->application_id and $this->application_preview_id), the code currently uses generateApplicationContainerName(...) which returns the app base container name but not the service-scoped DB container name; update the container_id construction to include the compose service name so it targets the service-specific container (e.g. append or prefix the DB service identifier from $this->service or the compose service name to the result of generateApplicationContainerName(...)) before calling docker restart; ensure both the $this->application and $this->application_preview branches build the same service-scoped container_id.
🤖 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/Models/ServiceDatabase.php`:
- Around line 14-15: Multiple code paths assume $database->service exists but do
not check ownership; update the callers (Livewire components CloneMe and
Shared\ResourceOperations where StopService/StartService are dispatched and
StartDatabaseProxy::handle where container names are built) to first gate on
$database->service_id (or another explicit ownership check) before dereferencing
$database->service->destination->server or using $database->service->uuid; if
the database is application-owned (null service_id) either skip/return early or
run an alternative code path that does not touch service relationships,
mirroring the pattern used in DatabaseBackupJob which branches on service_id.
In `@bootstrap/helpers/parsers.php`:
- Line 1455: Update the formatting of the negated if check around the
$resourceToSync variable to match project Pint rules by changing the condition
spacing from if (!$resourceToSync) to if (! $resourceToSync) and then run the
formatter (vendor/bin/pint --dirty --format agent) to apply project styling
across the modified PHP file so the change passes linting.
In `@bootstrap/helpers/shared.php`:
- Around line 2042-2043: The empty if ($isDatabase) { } branch in shared.php is
a no-op and should be removed to avoid dead code; delete the whole conditional
(the "if ($isDatabase)" statement and its empty block) where it appears so the
surrounding logic remains unchanged—search for the exact token "if
($isDatabase)" in the file to locate and remove it.
---
Duplicate comments:
In `@app/Models/Application.php`:
- Around line 345-355: The previews are being bulk-deleted via
$application->previews()->delete(), which skips model-level hooks (including
ApplicationPreview::forceDeleting) so ServiceDatabase::deleting cleanup can be
missed; change the code to individually force-delete preview model instances
before you cascade-delete databases—i.e. replace the bulk
$application->previews()->delete() with something like
$application->previews()->get()->each->forceDelete() and ensure that call runs
before the database cleanup lines
($application->databases()->get()->each->delete() / ServiceDatabase::deleting).
In `@app/Models/ServiceDatabase.php`:
- Around line 237-245: The two relationship methods application() and
application_preview() lack explicit return types; update both to declare and
return Illuminate\Database\Eloquent\Relations\BelongsTo (add a use statement for
BelongsTo at the top) so their signatures read with a BelongsTo return type, and
ensure the body still returns $this->belongsTo(...) unchanged; after typing, run
the provided ripgrep checks for application_preview references before any rename
of the snake_case method.
- Around line 99-104: When handling app-owned compose databases in
ServiceDatabase.php (the branches using $this->application_id and
$this->application_preview_id), the code currently uses
generateApplicationContainerName(...) which returns the app base container name
but not the service-scoped DB container name; update the container_id
construction to include the compose service name so it targets the
service-specific container (e.g. append or prefix the DB service identifier from
$this->service or the compose service name to the result of
generateApplicationContainerName(...)) before calling docker restart; ensure
both the $this->application and $this->application_preview branches build the
same service-scoped container_id.
In `@bootstrap/helpers/applications.php`:
- Around line 154-165: The query is using the literal 'running' status string;
update the status check to use the model constant
ScheduledDatabaseBackupExecution::STATUS_RUNNING instead so it reads
where('status', ScheduledDatabaseBackupExecution::STATUS_RUNNING). Locate the
$running_backup query that begins with
\App\Models\ScheduledDatabaseBackupExecution::where(...) and replace the
hard-coded 'running' with the constant to keep the guard in sync with
ScheduledDatabaseBackupExecution.
In `@resources/views/livewire/project/application/backups.blade.php`:
- Line 21: The Livewire child
<livewire:project.database.create-scheduled-backup> is being mounted inside a
loop over $database without a stable key, causing state bleed; fix by adding a
unique, stable :key based on the database identifier (for example
:key="$database->id" or :key="$database->uuid") on the
<livewire:project.database.create-scheduled-backup> tag so Livewire can
differentiate instances; ensure you use the persistent unique property from the
$database model (not the loop index) when adding the key.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5252a706-69bd-435b-9de3-52779c456b83
📒 Files selected for processing (17)
app/Jobs/DatabaseBackupJob.phpapp/Livewire/Project/Application/Backups.phpapp/Livewire/Project/Application/Configuration.phpapp/Models/Application.phpapp/Models/ApplicationPreview.phpapp/Models/ScheduledDatabaseBackup.phpapp/Models/ScheduledDatabaseBackupExecution.phpapp/Models/ServiceDatabase.phpbootstrap/helpers/applications.phpbootstrap/helpers/database.phpbootstrap/helpers/databases.phpbootstrap/helpers/parsers.phpbootstrap/helpers/shared.phpdatabase/migrations/2026_04_19_034518_add_application_id_to_service_databases.phpresources/views/livewire/project/application/backups.blade.phpresources/views/livewire/project/application/configuration.blade.phproutes/web.php
AkprasadoP
force-pushed
the
feat/integrated-backups
branch
from
69145a0 to
c4f83a8
Compare
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/Models/ApplicationPreview.php (1)
36-42:⚠️ Potential issue | 🟠 MajorDelete preview databases after parsing, not before.
Line 42 can resync Docker Compose databases for this preview, so deleting them on Line 36 first can leave freshly recreated
ServiceDatabaserows after the preview is force-deleted.🐛 Proposed cleanup-order fix
static::forceDeleting(function ($preview) { - $preview->databases()->get()->each->delete(); $server = $preview->application->destination->server; $application = $preview->application; if (data_get($preview, 'application.build_pack') === 'dockercompose') { // Docker Compose volume and network cleanup $composeFile = $application->parse(pull_request_id: $preview->pull_request_id); $volumes = data_get($composeFile, 'volumes'); $networks = data_get($composeFile, 'networks'); ... } + + $preview->databases()->get()->each->delete(); // Clean up persistent storage records $preview->persistentStorages()->delete(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/ApplicationPreview.php` around lines 36 - 42, The preview deletion currently removes databases too early by calling $preview->databases()->get()->each->delete() before running $application->parse(...), which can recreate Docker Compose databases (ServiceDatabase rows) during parse; move the database-deletion step to after the parse/compose cleanup branch (i.e., execute deletion after calling $application->parse and after Docker Compose volume/network cleanup) so that any databases recreated during parsing are removed before finalizing preview deletion—update the logic in ApplicationPreview (around the $preview->databases()->get()->each->delete() call and the if (data_get($preview, 'application.build_pack') === 'dockercompose') block) to delete databases after parse/compose cleanup.app/Models/ServiceDatabase.php (1)
41-83:⚠️ Potential issue | 🟠 MajorAdd explicit types to the booted hooks and team scopes—self-hosted machines demand determinism, not serverless soup.
The model callbacks and query builder methods lack type hints and return types. This violates the project's explicit typing requirement and creates gaps that your IDE can't navigate.
♻️ Typing cleanup required
use Illuminate\Database\Eloquent\Factories\HasFactory; +use Illuminate\Database\Eloquent\Builder; use Illuminate\Database\Eloquent\SoftDeletes; - protected static function booted() + protected static function booted(): void { - static::deleting(function ($service) { + static::deleting(function (ServiceDatabase $service): void { $service->persistentStorages()->delete(); $service->fileStorages()->delete(); $service->scheduledBackups()->delete(); }); - static::saving(function ($service) { + static::saving(function (ServiceDatabase $service): void { $ownerCount = collect([ $service->service_id, $service->application_id, $service->application_preview_id, @@ - public static function ownedByCurrentTeamAPI(int $teamId) + public static function ownedByCurrentTeamAPI(int $teamId): Builder { return ServiceDatabase::where(function ($query) use ($teamId) { @@ - public static function ownedByCurrentTeam() + public static function ownedByCurrentTeam(): Builder { $teamId = currentTeam()->id;Coding guideline requirement:
**/*.phpfiles must include explicit return type declarations and type hints for all method parameters.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/ServiceDatabase.php` around lines 41 - 83, Add explicit type hints and return types: change booted to protected static function booted(): void and type the model callbacks as static::deleting(function (ServiceDatabase $service): void { ... }) and static::saving(function (ServiceDatabase $service): void { ... }); update the query scope signatures to declare return types and parameter types—public static function ownedByCurrentTeamAPI(int $teamId): \Illuminate\Database\Eloquent\Builder and public static function ownedByCurrentTeam(): \Illuminate\Database\Eloquent\Builder, and type the closure $query parameters as (\Illuminate\Database\Eloquent\Builder $query) to ensure deterministic typing; add any necessary use statements (e.g., ServiceDatabase and Illuminate\Database\Eloquent\Builder) at the top of the file.
♻️ Duplicate comments (4)
bootstrap/helpers/applications.php (1)
154-165: 🧹 Nitpick | 🔵 TrivialUse the backup status constant; magic strings are back, Cyberdyne-style.
ScheduledDatabaseBackupExecution::STATUS_RUNNINGalready exists and is used elsewhere; keeping'running'here risks drift if statuses change.Proposed cleanup
- $running_backup = \App\Models\ScheduledDatabaseBackupExecution::where('status', 'running') + $running_backup = \App\Models\ScheduledDatabaseBackupExecution::where('status', \App\Models\ScheduledDatabaseBackupExecution::STATUS_RUNNING)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bootstrap/helpers/applications.php` around lines 154 - 165, Replace the magic string 'running' with the status constant: change the condition in the ScheduledDatabaseBackupExecution query to use ScheduledDatabaseBackupExecution::STATUS_RUNNING (or import the model and reference the constant) so the where('status', ...) call becomes where('status', ScheduledDatabaseBackupExecution::STATUS_RUNNING); keep the rest of the whereHas(...) logic unchanged.resources/views/livewire/project/application/backups.blade.php (1)
19-22:⚠️ Potential issue | 🟠 MajorKey the create-backup child component so state cannot shapeshift between database cards.
This component is rendered inside a loop but has no
:key; the sibling scheduled-backups component is keyed. Give this one a distinct key too, or Livewire may reuse state across cards like a T-1000 with bad boundaries.#!/bin/bash set -euo pipefail echo "Expected: create-scheduled-backup has a :key attribute inside the backups view." rg -n -C2 '<livewire:project\.database\.create-scheduled-backup|<livewire:project\.database\.scheduled-backups|:key=' resources/views/livewire/project/application/backups.blade.phpProposed fix
- <livewire:project.database.create-scheduled-backup :database="$database" /> + <livewire:project.database.create-scheduled-backup :database="$database" :key="'create-scheduled-backup-'.$database->id" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/views/livewire/project/application/backups.blade.php` around lines 19 - 22, The create-scheduled-backup Livewire child component rendered inside the loop lacks a :key so Livewire may reuse component state across database cards; update the <livewire:project.database.create-scheduled-backup> invocation (inside the <x-modal-input> block) to include a unique :key (for example using the database primary id or uuid like "create-scheduled-backup-{{ $database->id }}" or similar) so each card gets its own stable Livewire instance and state cannot shapeshift between cards.app/Models/ServiceDatabase.php (2)
237-245: 🛠️ Refactor suggestion | 🟠 MajorType the new Eloquent relationships.
application()andapplication_preview()should declareBelongsToreturn types. The relationship is not a mystery taco; label it.♻️ Proposed relationship typing
use Illuminate\Database\Eloquent\Factories\HasFactory; +use Illuminate\Database\Eloquent\Relations\BelongsTo; use Illuminate\Database\Eloquent\SoftDeletes; @@ - public function application() + public function application(): BelongsTo { return $this->belongsTo(Application::class); } - public function application_preview() + public function application_preview(): BelongsTo { return $this->belongsTo(ApplicationPreview::class); }As per coding guidelines,
**/*.php: Use explicit return type declarations and type hints for all method parameters.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/ServiceDatabase.php` around lines 237 - 245, The application() and application_preview() relationship methods currently lack explicit return types; update both method signatures (application() and application_preview()) to declare and import the Eloquent return type BelongsTo and annotate their return types as BelongsTo, preserving the existing body that returns $this->belongsTo(...).
92-108:⚠️ Potential issue | 🟠 MajorRestart the actual app-owned database container.
For
application_idandapplication_preview_id,restart()currently targets only the generated application base container. Compose database containers are prefixed with the service/database name, so this will miss the DB container. Hasta la vista, wrong container.🐛 Proposed fix
- public function restart() + public function restart(): void { $server = null; $container_id = null; if ($this->service_id) { $container_id = $this->name.'-'.$this->service->uuid; $server = $this->service->server; } elseif ($this->application_id) { - $container_id = generateApplicationContainerName($this->application); + $container_id = $this->name.'-'.generateApplicationContainerName($this->application); $server = $this->application->destination->server; } elseif ($this->application_preview_id) { - $container_id = generateApplicationContainerName($this->application_preview->application, $this->application_preview->pull_request_id); + $container_id = $this->name.'-'.generateApplicationContainerName($this->application_preview->application, $this->application_preview->pull_request_id); $server = $this->application_preview->application->destination->server; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/ServiceDatabase.php` around lines 92 - 108, The restart() method currently restarts the wrong container for application_id and application_preview_id (it restarts the base app container instead of the compose DB container). Update the application_id branch to set $container_id = $this->name . '-' . generateApplicationContainerName($this->application) and the application_preview_id branch to set $container_id = $this->name . '-' . generateApplicationContainerName($this->application_preview->application, $this->application_preview->pull_request_id) so the service/database prefix is included (keep using $server as before and call remote_process(["docker restart {$container_id}"], $server) as existing).
🤖 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/Actions/Database/StartDatabaseProxy.php`:
- Around line 34-48: Replace the ad-hoc calls to
generateApplicationContainerName() for application-owned DBs with the persisted
compose container name (use data_get to read e.g. data_get($database,
'application.compose.container_name') and data_get($database,
'application_preview.application.compose.container_name') instead of generating
a new timestamped name), and guard all owner relations before dereferencing
(check that $database->application or $database->application_preview and their
destination/network/server fields exist via data_get or null checks); if the
owner relation is missing or required fields are absent, log an error (or
return/abort gracefully) rather than letting PHP throw a fatal error. Ensure you
still set $network, $server and $containerName from the persisted values and
only fall back to generateApplicationContainerName() as a last resort.
In `@app/Livewire/Project/Application/Backups.php`:
- Around line 18-23: Replace the public mutable property $databases with a
Livewire computed property: remove the public Collection $databases declaration
and the assignment in mount(), keep the Gate check in mount(), and add a method
annotated with #[Computed] named databases() that returns
$this->application->databases()->get() (or Collection) so the value is derived
server-side and cached by Livewire; update any references to $databases in the
component/template to call the computed property (e.g., $this->databases or {{
$this->databases }} as appropriate).
---
Outside diff comments:
In `@app/Models/ApplicationPreview.php`:
- Around line 36-42: The preview deletion currently removes databases too early
by calling $preview->databases()->get()->each->delete() before running
$application->parse(...), which can recreate Docker Compose databases
(ServiceDatabase rows) during parse; move the database-deletion step to after
the parse/compose cleanup branch (i.e., execute deletion after calling
$application->parse and after Docker Compose volume/network cleanup) so that any
databases recreated during parsing are removed before finalizing preview
deletion—update the logic in ApplicationPreview (around the
$preview->databases()->get()->each->delete() call and the if (data_get($preview,
'application.build_pack') === 'dockercompose') block) to delete databases after
parse/compose cleanup.
In `@app/Models/ServiceDatabase.php`:
- Around line 41-83: Add explicit type hints and return types: change booted to
protected static function booted(): void and type the model callbacks as
static::deleting(function (ServiceDatabase $service): void { ... }) and
static::saving(function (ServiceDatabase $service): void { ... }); update the
query scope signatures to declare return types and parameter types—public static
function ownedByCurrentTeamAPI(int $teamId):
\Illuminate\Database\Eloquent\Builder and public static function
ownedByCurrentTeam(): \Illuminate\Database\Eloquent\Builder, and type the
closure $query parameters as (\Illuminate\Database\Eloquent\Builder $query) to
ensure deterministic typing; add any necessary use statements (e.g.,
ServiceDatabase and Illuminate\Database\Eloquent\Builder) at the top of the
file.
---
Duplicate comments:
In `@app/Models/ServiceDatabase.php`:
- Around line 237-245: The application() and application_preview() relationship
methods currently lack explicit return types; update both method signatures
(application() and application_preview()) to declare and import the Eloquent
return type BelongsTo and annotate their return types as BelongsTo, preserving
the existing body that returns $this->belongsTo(...).
- Around line 92-108: The restart() method currently restarts the wrong
container for application_id and application_preview_id (it restarts the base
app container instead of the compose DB container). Update the application_id
branch to set $container_id = $this->name . '-' .
generateApplicationContainerName($this->application) and the
application_preview_id branch to set $container_id = $this->name . '-' .
generateApplicationContainerName($this->application_preview->application,
$this->application_preview->pull_request_id) so the service/database prefix is
included (keep using $server as before and call remote_process(["docker restart
{$container_id}"], $server) as existing).
In `@bootstrap/helpers/applications.php`:
- Around line 154-165: Replace the magic string 'running' with the status
constant: change the condition in the ScheduledDatabaseBackupExecution query to
use ScheduledDatabaseBackupExecution::STATUS_RUNNING (or import the model and
reference the constant) so the where('status', ...) call becomes where('status',
ScheduledDatabaseBackupExecution::STATUS_RUNNING); keep the rest of the
whereHas(...) logic unchanged.
In `@resources/views/livewire/project/application/backups.blade.php`:
- Around line 19-22: The create-scheduled-backup Livewire child component
rendered inside the loop lacks a :key so Livewire may reuse component state
across database cards; update the
<livewire:project.database.create-scheduled-backup> invocation (inside the
<x-modal-input> block) to include a unique :key (for example using the database
primary id or uuid like "create-scheduled-backup-{{ $database->id }}" or
similar) so each card gets its own stable Livewire instance and state cannot
shapeshift between cards.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: afd498b8-6d16-4759-b1b2-976e66c54130
📒 Files selected for processing (20)
app/Actions/Database/StartDatabaseProxy.phpapp/Jobs/DatabaseBackupJob.phpapp/Livewire/Project/Application/Backups.phpapp/Livewire/Project/Application/Configuration.phpapp/Livewire/Project/CloneMe.phpapp/Livewire/Project/Shared/ResourceOperations.phpapp/Models/Application.phpapp/Models/ApplicationPreview.phpapp/Models/ScheduledDatabaseBackup.phpapp/Models/ScheduledDatabaseBackupExecution.phpapp/Models/ServiceDatabase.phpbootstrap/helpers/applications.phpbootstrap/helpers/database.phpbootstrap/helpers/databases.phpbootstrap/helpers/parsers.phpbootstrap/helpers/shared.phpdatabase/migrations/2026_04_19_034518_add_application_id_to_service_databases.phpresources/views/livewire/project/application/backups.blade.phpresources/views/livewire/project/application/configuration.blade.phproutes/web.php
AkprasadoP
force-pushed
the
feat/integrated-backups
branch
from
c4f83a8 to
ad64926
Compare
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview 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 (4)
app/Models/Application.php (1)
345-355:⚠️ Potential issue | 🟠 MajorDelete previews individually to cascade database cleanup, not via mass delete.
Line 355's
$application->previews()->delete()is a query builder mass delete—like deploying to serverless and hoping the cleanup happens automatically (it won't, and neither will your databases). Mass deletes skip model events, soApplicationPreview::forceDeleting()never fires. That means preview-ownedServiceDatabaserows plus their scheduled backups survive like cockroaches after nuclear fallout.Unlike those microservices vendors pushing their VC-funded dreams, real servers with proper cascade hooks require individual deletion. Loop through previews and delete each one:
Fix
$application->tags()->detach(); - $application->previews()->delete(); + $application->previews()->get()->each(function (ApplicationPreview $preview): void { + $preview->databases()->get()->each->delete(); + $preview->delete(); + }); foreach ($application->deployment_queue as $deployment) {(And if you want proper cascade behavior, run a self-hosted database. They don't ghost you like serverless infrastructure—they're as reliable as a good taco, minus the gluten issues.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/Application.php` around lines 345 - 355, The previews relation is being mass-deleted via $application->previews()->delete(), which bypasses model events (e.g. ApplicationPreview::forceDeleting()) and leaves preview-owned ServiceDatabase/scheduled backups behind; change this to load the previews and delete each model instance so model events run (e.g. iterate $application->previews()->get() or $application->previews and call delete() on each preview model) instead of calling previews()->delete(); ensure you remove the mass-delete call and replace it with per-model deletion so cascade cleanup triggers.app/Models/ApplicationPreview.php (1)
36-43:⚠️ Potential issue | 🟠 MajorDelete preview databases after compose parsing, not before.
Line 36 deletes preview databases before line 42 calls
parse(). The parse operation syncs detected databases viaupdateResourceDatabases(), which recreatesServiceDatabaserecords for any databases detected in the compose file. This means deleted databases get resurrected by the parser. Move the database cleanup to after parse-driven volume/network cleanup to prevent staleServiceDatabaserows (and their associated scheduled backups) from being re-instantiated during preview force-deletion.Proposed fix
static::forceDeleting(function ($preview) { - $preview->databases()->get()->each->delete(); $server = $preview->application->destination->server; $application = $preview->application; if (data_get($preview, 'application.build_pack') === 'dockercompose') { // Docker Compose volume and network cleanup $composeFile = $application->parse(pull_request_id: $preview->pull_request_id); + $preview->databases()->get()->each->delete();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/ApplicationPreview.php` around lines 36 - 43, The preview databases are deleted before parse() runs, but parse() (via updateResourceDatabases()) can recreate ServiceDatabase rows; move the deletion of preview databases (the call $preview->databases()->get()->each->delete()) to after the compose parsing and related volume/network cleanup so deletions occur after parse-driven syncing; locate the block that checks data_get($preview, 'application.build_pack') === 'dockercompose', call $application->parse(...) and perform volume/network cleanup first, then invoke the existing databases()->get()->each->delete() to remove any now-current database records and avoid resurrection of stale ServiceDatabase entries and scheduled backups.bootstrap/helpers/parsers.php (1)
1444-1451:⚠️ Potential issue | 🟠 MajorCaptured database service name is preview-suffixed for PR deploys — this will cause pruning mismatches on redeploy.
Line 1445 reassigns
$serviceName = addPreviewDeploymentSuffix(...)for PR builds. Then line 1450 stores this suffixed name as the collection key:$detectedDatabases->put($serviceName, $service).When
updateResourceDatabases()runs (line 1464), it iterates the collection by key (line 22 of bootstrap/helpers/database.php:foreach ($detectedDatabases as $serviceName => $serviceConfig)). That key becomes theServiceDatabase.namefield (line 42). Result: a PR preview's database gets namedpostgres-pr-42instead of the original compose namepostgres.The asymmetry breaks pruning. When the same PR redeploys, the suffixed key will match existing records this time, but if the suffix generation changes or the preview gets recreated,
$existingDatabases->where('name', $serviceName)(line 39) won't find the orphaned record, and you'll accumulate staleServiceDatabaserows. Gross.Capture the original service name before line 1445 applies the suffix, and use the original name as the collection key:
Proposed fix
+ $originalServiceName = $serviceName; if ($isPullRequest) { $serviceName = addPreviewDeploymentSuffix($serviceName, $pullRequestId); } $parsedServices->put($serviceName, $payload); if (isset($isDatabase) && $isDatabase) { - $detectedDatabases->put($serviceName, $service); + $detectedDatabases->put($originalServiceName, $service); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bootstrap/helpers/parsers.php` around lines 1444 - 1451, Before applying addPreviewDeploymentSuffix to $serviceName, save the original name (e.g., $originalServiceName = $serviceName); after you conditionally set $serviceName = addPreviewDeploymentSuffix(...), continue to store the parsed payload under the possibly-suffixed $serviceName with $parsedServices->put($serviceName, $payload) but insert the database into $detectedDatabases using the original name ($detectedDatabases->put($originalServiceName, $service)). This preserves the compose/service canonical name for updateResourceDatabases() and ServiceDatabase.name lookups while still allowing parsedServices to reference the preview-suffixed service identifier.app/Actions/Database/StartDatabaseProxy.php (1)
23-23: 🛠️ Refactor suggestion | 🟠 MajorMissing return type declaration on
handle(). Hasta la vista, implicit returns.The
handle()method signature at line 23 has no return type. Other handlers in this PR (e.g.,DatabaseBackupJob::handle(): void) declare it explicitly.🔧 Proposed fix
- public function handle(StandaloneRedis|StandalonePostgresql|StandaloneMongodb|StandaloneMysql|StandaloneMariadb|StandaloneKeydb|StandaloneDragonfly|StandaloneClickhouse|ServiceDatabase $database) + public function handle(StandaloneRedis|StandalonePostgresql|StandaloneMongodb|StandaloneMysql|StandaloneMariadb|StandaloneKeydb|StandaloneDragonfly|StandaloneClickhouse|ServiceDatabase $database): void {As per coding guidelines: "Use explicit return type declarations and type hints for all method parameters".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Actions/Database/StartDatabaseProxy.php` at line 23, The handle() method in StartDatabaseProxy lacks an explicit return type; update the StartDatabaseProxy::handle(...) signature to include the correct return type (use : void to match other handlers like DatabaseBackupJob::handle()) so the method explicitly declares its return type for all branches and adheres to the project's typing guidelines.
♻️ Duplicate comments (4)
resources/views/livewire/project/application/backups.blade.php (1)
21-21:⚠️ Potential issue | 🟠 MajorMissing unique
:keyoncreate-scheduled-backup— state will leak across cards, human.Still not addressed from the prior review. Inside the
@forelseloop, every Livewire child component needs a unique key so instances stay isolated.scheduled-backupsat line 25 has:key="$database->id"; its sibling on line 21 doesn't. When you click "+ Add" on one card and Livewire swaps state, the locked:databaseprop can get crossed like two Terminators fighting over the same taco — mildly amusing until production eats it.🗝️ Proposed fix
- <x-modal-input buttonTitle="+ Add" title="New Scheduled Backup" canGate="update" :canResource="$database"> - <livewire:project.database.create-scheduled-backup :database="$database" /> - </x-modal-input> + <x-modal-input buttonTitle="+ Add" title="New Scheduled Backup" canGate="update" :canResource="$database"> + <livewire:project.database.create-scheduled-backup :database="$database" :key="'create-scheduled-backup-'.$database->id" /> + </x-modal-input>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/views/livewire/project/application/backups.blade.php` at line 21, The Livewire child component <livewire:project.database.create-scheduled-backup :database="$database" /> inside the `@forelse` loop is missing a unique :key which causes state to leak across cards; update that component invocation to include the same unique key used by its sibling (e.g. :key="$database->id") so each Livewire instance is isolated and state won’t be shared between cards.app/Models/ServiceDatabase.php (2)
99-104:⚠️ Potential issue | 🟠 MajorPrepend the compose service name when restarting app-owned database containers.
generateApplicationContainerName()returns the application base name, but the actual compose container is built as<serviceName>-<baseName>inbootstrap/helpers/shared.php, andStartDatabaseProxyfollows that same pattern. These branches will restart the wrong target or nothing at all — a T-800 knocking on the wrong container door.🐛 Proposed fix
} elseif ($this->application_id) { - $container_id = generateApplicationContainerName($this->application); + $container_id = $this->name.'-'.generateApplicationContainerName($this->application); $server = $this->application->destination->server; } elseif ($this->application_preview_id) { - $container_id = generateApplicationContainerName($this->application_preview->application, $this->application_preview->pull_request_id); + $container_id = $this->name.'-'.generateApplicationContainerName($this->application_preview->application, $this->application_preview->pull_request_id); $server = $this->application_preview->application->destination->server; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/ServiceDatabase.php` around lines 99 - 104, The branches that handle $this->application_id and $this->application_preview_id use generateApplicationContainerName() which returns only the application base name, but the actual compose container is named "<serviceName>-<baseName>"; update the two branches (the block using $this->application_id and the one using $this->application_preview_id) to prepend the compose service name (same pattern used in StartDatabaseProxy and bootstrap/helpers/shared.php) to the generated base name when building $container_id so the restart targets the correct compose container.
237-245: 🛠️ Refactor suggestion | 🟠 MajorAdd
BelongsToreturn types to the new owner relationships.
application()andapplication_preview()are new Eloquent relationships but still have implicit return types. Give them explicitBelongsTocontracts so static analysis and IDEs can follow the owner graph without needing a taco map.♻️ Proposed fix
+use Illuminate\Database\Eloquent\Relations\BelongsTo;- public function application() + public function application(): BelongsTo { return $this->belongsTo(Application::class); } - public function application_preview() + public function application_preview(): BelongsTo { return $this->belongsTo(ApplicationPreview::class); }You can verify the signatures with:
#!/bin/bash rg -n -C2 'use Illuminate\\Database\\Eloquent\\Relations\\BelongsTo|function application\(\): BelongsTo|function application_preview\(\): BelongsTo' app/Models/ServiceDatabase.phpAs per coding guidelines, use explicit return type declarations and type hints for all method parameters.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/ServiceDatabase.php` around lines 237 - 245, The two new Eloquent relationship methods application() and application_preview() lack explicit return types; update their signatures to return Illuminate\Database\Eloquent\Relations\BelongsTo (i.e. add ": BelongsTo") and ensure the BelongsTo class is imported with a use statement at the top of ServiceDatabase.php so static analysis and IDEs can resolve the relationship type for Application::class and ApplicationPreview::class.app/Actions/Database/StartDatabaseProxy.php (1)
44-44: 🧹 Nitpick | 🔵 TrivialDitch the full-path parade —
Yamlis already at the door.You've already imported
Symfony\Component\Yaml\Yamlup at line 15, so using the FQN here is like driving a tank to pick up tacos — it works, but it's a bit much.🔧 Proposed fix
- $compose = \Symfony\Component\Yaml\Yaml::parse($application?->docker_compose ?: '') ?: []; + $compose = Yaml::parse($application?->docker_compose ?: '') ?: [];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Actions/Database/StartDatabaseProxy.php` at line 44, The code uses the fully-qualified class name Symfony\Component\Yaml\Yaml when parsing into $compose even though Yaml is already imported; update the call to use the imported Yaml symbol (Yaml::parse(...)) instead of the FQN so the code is consistent with the import and avoids redundant namespace verbosity, leaving the surrounding logic (variable $compose and null-coalescing parse input) unchanged.
🤖 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/Actions/Database/StartDatabaseProxy.php`:
- Around line 49-57: The preview branch skips reading a persisted container_name
from the parsed compose and incorrectly tries to parse a non-existent
$preview->docker_compose; update the preview logic in StartDatabaseProxy so it
mirrors the application branch: call $application->parse(pull_request_id:
$preview->pull_request_id) to obtain the parsed compose, read
services.<database-name>.container_name from that parsed result (falling back to
generateApplicationContainerName($application, $preview->pull_request_id) only
if no container_name is present), and keep using data_get($application,
'destination.network') / 'destination.server' as before; this aligns with
ApplicationPreview.booted() and prevents proxying to the wrong container.
In `@app/Jobs/DatabaseBackupJob.php`:
- Around line 162-174: The DatabaseBackupJob currently builds container names
solely via generateApplicationContainerName(), causing drift from
StartDatabaseProxy::handle() which first checks parsed docker_compose
services.<name>.container_name and falls back to
"{$database->name}-".generateApplicationContainerName(...); change
DatabaseBackupJob to mirror that logic: extract the compose lookup and fallback
into a shared helper (e.g., getDatabaseContainerName($database,
$applicationPreview = null) used by both DatabaseBackupJob and
StartDatabaseProxy::handle()), have the job call this helper and set
$this->container_name = $composeContainerName ??
"{$this->database->name}-$serviceUuid", and ensure the helper consults
docker_compose->services[serviceName]->container_name before returning the
generated name so both code paths stay consistent.
- Around line 737-744: The S3 network resolution dereferences relations directly
and can NPE if owner relations exist but their destination is null; update the
resolution in DatabaseBackupJob to safely fetch the network using guarded
lookups (e.g. use data_get($this->database, 'service.destination.network'),
data_get($this->database, 'application.destination.network'), and
data_get($this->database,
'application_preview.application.destination.network')) or check each relation
for null before accessing ->destination->network, assign $network only when
non-null, and if none yield a network throw a clear exception (include DB
id/type context) so uploads fail with an actionable error instead of a cryptic
NPE.
- Around line 124-144: The job currently calls $this->release(60) in
DatabaseBackupJob while Horizon may use the default tries=1, causing release()
to increment attempts and trigger failed(); fix by adding a higher attempt limit
to the DatabaseBackupJob class (e.g., set protected $tries = 60) so the job can
be released up to the intended number of times while waiting for deployments,
ensuring the release(60) loop doesn't immediately mark the job as failed (refer
to DatabaseBackupJob, the release(60) call and its failed() handler).
In `@app/Livewire/Project/Shared/ResourceOperations.php`:
- Around line 333-348: The cloneVolumeData branch only handles ServiceDatabase
instances with a service_id and silently skips application-owned databases;
update the cloning logic in ResourceOperations (the cloneVolumeData block that
currently accesses $database->service, StopService::dispatch,
VolumeCloneJob::dispatch, and StartService::dispatch) to detect the
ServiceDatabase owner type (service_id, application_id, application_preview_id)
and perform owner-specific handling similar to ScheduledDatabaseBackup: for
service-owned use the existing StopService/VolumeCloneJob/StartService flow, for
application-owned resolve the appropriate application server/volume and dispatch
VolumeCloneJob with those targets, and if an owner type cannot be resolved throw
or log an explicit error and return/fail fast instead of silently skipping.
Ensure the code references $database->service, $database->application /
$database->applicationPreview (or equivalent accessors) and $newPersistentVolume
when building dispatch arguments.
In `@bootstrap/helpers/applications.php`:
- Line 154: Replace the hardcoded 'running' string with the model constant
ScheduledDatabaseBackupExecution::STATUS_RUNNING in the query that sets
$running_backup; update the top of the file to add a use import for
App\Models\ScheduledDatabaseBackupExecution so you can reference the class
directly (remove the leading backslash from the existing fully-qualified usages)
and ensure all other status checks in this file use the same constant.
- Around line 154-169: The running-check query in
bootstrap/helpers/applications.php can block deployments forever because it
treats any row with status 'running' as active; update the
ScheduledDatabaseBackupExecution query (the $running_backup check) to ignore
executions older than your stale threshold by adding a time condition (e.g.,
->where('updated_at', '>=', now()->subMinutes($staleMinutes))) or use a config
value like config('backups.stale_after_minutes') so it matches the same timeout
used in markStaleExecutionsAsFailed (and/or DatabaseBackupJob). Ensure you
reference ScheduledDatabaseBackupExecution and the existing
whereHas('scheduledDatabaseBackup', ...) clause so the time filter applies to
the running execution rows before calling exists().
In `@bootstrap/helpers/parsers.php`:
- Around line 1453-1466: The code is re-querying the preview by pull_request_id
instead of using the existing ?int $preview_id parameter; change the logic in
applicationParser so that when $isPullRequest is true you set $resourceToSync =
$resource->previews()->find($preview_id) (or null if not found) instead of
querying by pull_request_id, log the warning with application_id and preview_id
if not found, and then call updateResourceDatabases($resourceToSync,
$detectedDatabases) only when $resourceToSync is truthy; remove the unnecessary
else branch by initializing $resourceToSync to $resource when not a PR and keep
updateResourceDatabases guarded by the single if check already present.
In
`@database/migrations/2026_04_19_034518_add_application_id_to_service_databases.php`:
- Around line 30-34: The rollback currently soft-deletes app-owned rows
(ServiceDatabase::query()->whereNull('service_id')->chunkById(...)->each->delete())
which leaves service_id NULL and later makes the NOT NULL migration fail; change
the cleanup to physically remove those rows by calling forceDelete() instead of
delete (i.e., use $databases->each->forceDelete() inside the chunkById callback
on the ServiceDatabase model) so the records are permanently removed before
restoring service_id to NOT NULL.
---
Outside diff comments:
In `@app/Actions/Database/StartDatabaseProxy.php`:
- Line 23: The handle() method in StartDatabaseProxy lacks an explicit return
type; update the StartDatabaseProxy::handle(...) signature to include the
correct return type (use : void to match other handlers like
DatabaseBackupJob::handle()) so the method explicitly declares its return type
for all branches and adheres to the project's typing guidelines.
In `@app/Models/Application.php`:
- Around line 345-355: The previews relation is being mass-deleted via
$application->previews()->delete(), which bypasses model events (e.g.
ApplicationPreview::forceDeleting()) and leaves preview-owned
ServiceDatabase/scheduled backups behind; change this to load the previews and
delete each model instance so model events run (e.g. iterate
$application->previews()->get() or $application->previews and call delete() on
each preview model) instead of calling previews()->delete(); ensure you remove
the mass-delete call and replace it with per-model deletion so cascade cleanup
triggers.
In `@app/Models/ApplicationPreview.php`:
- Around line 36-43: The preview databases are deleted before parse() runs, but
parse() (via updateResourceDatabases()) can recreate ServiceDatabase rows; move
the deletion of preview databases (the call
$preview->databases()->get()->each->delete()) to after the compose parsing and
related volume/network cleanup so deletions occur after parse-driven syncing;
locate the block that checks data_get($preview, 'application.build_pack') ===
'dockercompose', call $application->parse(...) and perform volume/network
cleanup first, then invoke the existing databases()->get()->each->delete() to
remove any now-current database records and avoid resurrection of stale
ServiceDatabase entries and scheduled backups.
In `@bootstrap/helpers/parsers.php`:
- Around line 1444-1451: Before applying addPreviewDeploymentSuffix to
$serviceName, save the original name (e.g., $originalServiceName =
$serviceName); after you conditionally set $serviceName =
addPreviewDeploymentSuffix(...), continue to store the parsed payload under the
possibly-suffixed $serviceName with $parsedServices->put($serviceName, $payload)
but insert the database into $detectedDatabases using the original name
($detectedDatabases->put($originalServiceName, $service)). This preserves the
compose/service canonical name for updateResourceDatabases() and
ServiceDatabase.name lookups while still allowing parsedServices to reference
the preview-suffixed service identifier.
---
Duplicate comments:
In `@app/Actions/Database/StartDatabaseProxy.php`:
- Line 44: The code uses the fully-qualified class name
Symfony\Component\Yaml\Yaml when parsing into $compose even though Yaml is
already imported; update the call to use the imported Yaml symbol
(Yaml::parse(...)) instead of the FQN so the code is consistent with the import
and avoids redundant namespace verbosity, leaving the surrounding logic
(variable $compose and null-coalescing parse input) unchanged.
In `@app/Models/ServiceDatabase.php`:
- Around line 99-104: The branches that handle $this->application_id and
$this->application_preview_id use generateApplicationContainerName() which
returns only the application base name, but the actual compose container is
named "<serviceName>-<baseName>"; update the two branches (the block using
$this->application_id and the one using $this->application_preview_id) to
prepend the compose service name (same pattern used in StartDatabaseProxy and
bootstrap/helpers/shared.php) to the generated base name when building
$container_id so the restart targets the correct compose container.
- Around line 237-245: The two new Eloquent relationship methods application()
and application_preview() lack explicit return types; update their signatures to
return Illuminate\Database\Eloquent\Relations\BelongsTo (i.e. add ": BelongsTo")
and ensure the BelongsTo class is imported with a use statement at the top of
ServiceDatabase.php so static analysis and IDEs can resolve the relationship
type for Application::class and ApplicationPreview::class.
In `@resources/views/livewire/project/application/backups.blade.php`:
- Line 21: The Livewire child component
<livewire:project.database.create-scheduled-backup :database="$database" />
inside the `@forelse` loop is missing a unique :key which causes state to leak
across cards; update that component invocation to include the same unique key
used by its sibling (e.g. :key="$database->id") so each Livewire instance is
isolated and state won’t be shared between cards.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: bb3003b6-5ea6-489e-b99d-a325caea7acf
📒 Files selected for processing (20)
app/Actions/Database/StartDatabaseProxy.phpapp/Jobs/DatabaseBackupJob.phpapp/Livewire/Project/Application/Backups.phpapp/Livewire/Project/Application/Configuration.phpapp/Livewire/Project/CloneMe.phpapp/Livewire/Project/Shared/ResourceOperations.phpapp/Models/Application.phpapp/Models/ApplicationPreview.phpapp/Models/ScheduledDatabaseBackup.phpapp/Models/ScheduledDatabaseBackupExecution.phpapp/Models/ServiceDatabase.phpbootstrap/helpers/applications.phpbootstrap/helpers/database.phpbootstrap/helpers/databases.phpbootstrap/helpers/parsers.phpbootstrap/helpers/shared.phpdatabase/migrations/2026_04_19_034518_add_application_id_to_service_databases.phpresources/views/livewire/project/application/backups.blade.phpresources/views/livewire/project/application/configuration.blade.phproutes/web.php
Comment on lines
+49
to
+57
| } else { | ||
| $preview = $database->application_preview; | ||
| $application = $preview?->application; | ||
| $network = data_get($application, 'destination.network'); | ||
| $server = data_get($application, 'destination.server'); | ||
| $containerName = $preview && $application | ||
| ? "{$database->name}-" . generateApplicationContainerName($application, $preview->pull_request_id) | ||
| : null; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect whether ApplicationPreview persists a rewritten docker_compose with explicit container_name,
# and confirm how the Application path rewrites compose during parsing.
rg -nP -C3 'docker_compose|container_name' --type=php app/Models/ApplicationPreview.php app/Models/Application.php 2>/dev/null
rg -nP -C5 'container_name' bootstrap/helpers/shared.php | head -120Repository: coollabsio/coolify
Length of output: 14170
🏁 Script executed:
# First, let's find and examine the StartDatabaseProxy.php file
fd -type f "StartDatabaseProxy.php"Repository: coollabsio/coolify
Length of output: 232
🏁 Script executed:
# Also check if ApplicationPreview has a docker_compose field
rg -A 20 'protected \$fillable|protected \$casts' app/Models/ApplicationPreview.php | head -80Repository: coollabsio/coolify
Length of output: 1404
🏁 Script executed:
# Now read the StartDatabaseProxy.php file to see the actual code context
cat -n app/Actions/Database/StartDatabaseProxy.php | sed -n '35,65p'Repository: coollabsio/coolify
Length of output: 2065
🏁 Script executed:
# Also check the full context of what happens in the application branch
cat -n app/Actions/Database/StartDatabaseProxy.php | sed -n '30,70p'Repository: coollabsio/coolify
Length of output: 2707
🏁 Script executed:
# Check what docker_compose related fields are actually available on ApplicationPreview
rg -B5 -A10 'class ApplicationPreview' app/Models/ApplicationPreview.php | head -40Repository: coollabsio/coolify
Length of output: 457
🏁 Script executed:
# Check if there's a docker_compose_pr field or similar on Application for previews
rg 'docker_compose_pr' app/Models/Application.phpRepository: coollabsio/coolify
Length of output: 161
🏁 Script executed:
# Check the complete ApplicationPreview model to see all properties/relationships
cat -n app/Models/ApplicationPreview.php | head -100Repository: coollabsio/coolify
Length of output: 4335
🏁 Script executed:
# Check if ApplicationPreview has a docker_compose attribute/relationship
rg 'docker_compose' app/Models/ApplicationPreview.phpRepository: coollabsio/coolify
Length of output: 729
🏁 Script executed:
# Check what generateApplicationContainerName does
rg -B5 -A10 'function generateApplicationContainerName' bootstrap/helpers/shared.phpRepository: coollabsio/coolify
Length of output: 44
🏁 Script executed:
# Also check if there's logic to use docker_compose_pr for previews
rg -B3 -A3 'docker_compose_pr' app/Actions/Database/StartDatabaseProxy.phpRepository: coollabsio/coolify
Length of output: 44
🏁 Script executed:
# Search for generateApplicationContainerName more broadly
rg 'generateApplicationContainerName' --type=phpRepository: coollabsio/coolify
Length of output: 1565
🏁 Script executed:
# Check if Application model has docker_compose_pr and how it's used for previews
rg -B3 -A8 'parse.*pull_request' app/Models/Application.php | head -50Repository: coollabsio/coolify
Length of output: 862
🏁 Script executed:
# Check how the parse() method determines which docker_compose to use with pull_request_id
cat -n app/Models/Application.php | sed -n '1768,1775p'Repository: coollabsio/coolify
Length of output: 505
🏁 Script executed:
# Look for applicationParser function to understand how it handles pull_request_id
rg -B2 -A15 'function applicationParser' bootstrap/helpers/parsers.php | head -50Repository: coollabsio/coolify
Length of output: 697
🏁 Script executed:
# Let's see how the parse() method uses pull_request_id to determine which compose file to use
rg -B5 -A20 'docker_compose_raw\|docker_compose_pr' bootstrap/helpers/parsers.php | head -80Repository: coollabsio/coolify
Length of output: 44
🏁 Script executed:
# Check the generateApplicationContainerName implementation
cat -n bootstrap/helpers/docker.php | grep -A 20 'function generateApplicationContainerName'Repository: coollabsio/coolify
Length of output: 996
🏁 Script executed:
# Check if Application has docker_compose_pr and when it's used with pull_request_id
rg -B5 -A10 'docker_compose_pr' app/Models/Application.php | head -60Repository: coollabsio/coolify
Length of output: 757
🏁 Script executed:
# Verify if ApplicationPreview has any way to access or store the composed file for the PR
rg 'docker_compose' app/Models/ApplicationPreview.php -A2 -B2Repository: coollabsio/coolify
Length of output: 1466
🏁 Script executed:
# Check if Application.parse() method differentiates between main and PR compose based on pull_request_id
cat -n app/Models/Application.php | sed -n '1765,1805p'Repository: coollabsio/coolify
Length of output: 2582
🏁 Script executed:
# Check if there's logic elsewhere that determines which compose to use for previews
rg -B5 -A10 'application_preview.*parse\|parse.*pull_request' app/Actions/Database/StartDatabaseProxy.phpRepository: coollabsio/coolify
Length of output: 44
🏁 Script executed:
# Check if Application has logic to select docker_compose vs docker_compose_pr based on pull_request_id
rg -B10 -A10 'docker_compose_pr_raw' app/Models/Application.php | head -60Repository: coollabsio/coolify
Length of output: 801
🏁 Script executed:
# Check if the parse method or related functions handle both compose types
rg 'docker_compose_pr' bootstrap/helpers/parsers.phpRepository: coollabsio/coolify
Length of output: 44
🏁 Script executed:
# Let's verify if Application model actually uses docker_compose_pr_raw anywhere
rg 'docker_compose_pr_raw' app/ -lRepository: coollabsio/coolify
Length of output: 88
🏁 Script executed:
# Check one more time where docker_compose_pr is actually used
rg -B3 -A3 'docker_compose_pr[^a-z_]' app/ --type=phpRepository: coollabsio/coolify
Length of output: 475
Preview branch skips docker_compose container_name lookup — target could drift, but proposed fix is incorrect.
The application branch (lines 40–48) first tries to read the persisted services.<name>.container_name from the parsed compose and only falls back to generateApplicationContainerName(). The preview branch goes straight for the generator, ignoring any persisted container name. If the compose carries a different container_name, Nginx will proxy to a nonexistent container.
However, ApplicationPreview does not have a docker_compose or docker_compose_raw field—it only has docker_compose_domains. The proposed fix attempts to parse $preview?->docker_compose, which doesn't exist. The correct approach is to parse the application's compose file with the pull request ID, similar to how ApplicationPreview.booted() calls $application->parse(pull_request_id: $preview->pull_request_id) on line 42:
🔧 Correct fix (mirror the application branch)
} else {
$preview = $database->application_preview;
$application = $preview?->application;
$network = data_get($application, 'destination.network');
$server = data_get($application, 'destination.server');
- $containerName = $preview && $application
- ? "{$database->name}-" . generateApplicationContainerName($application, $preview->pull_request_id)
- : null;
+ $containerName = null;
+ if ($preview && $application) {
+ $compose = \Symfony\Component\Yaml\Yaml::parse($application?->docker_compose ?: '') ?: [];
+ $containerName = data_get($compose, 'services.' . $database->name . '.container_name');
+ if (! $containerName) {
+ $containerName = "{$database->name}-" . generateApplicationContainerName($application, $preview->pull_request_id);
+ }
+ }
}🧰 Tools
🪛 PHPMD (2.15.0)
[error] 49-57: The method handle uses an else expression. Else clauses are basically not necessary and you can simplify the code by not using them. (undefined)
(ElseExpression)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Actions/Database/StartDatabaseProxy.php` around lines 49 - 57, The
preview branch skips reading a persisted container_name from the parsed compose
and incorrectly tries to parse a non-existent $preview->docker_compose; update
the preview logic in StartDatabaseProxy so it mirrors the application branch:
call $application->parse(pull_request_id: $preview->pull_request_id) to obtain
the parsed compose, read services.<database-name>.container_name from that
parsed result (falling back to generateApplicationContainerName($application,
$preview->pull_request_id) only if no container_name is present), and keep using
data_get($application, 'destination.network') / 'destination.server' as before;
this aligns with ApplicationPreview.booted() and prevents proxying to the wrong
container.
Comment on lines
333
to
348
| if ($this->cloneVolumeData) { | ||
| try { | ||
| StopService::dispatch($database->service); | ||
| $sourceVolume = $volume->name; | ||
| $targetVolume = $newPersistentVolume->name; | ||
| $sourceServer = $database->service->destination->server; | ||
| $targetServer = $new_resource->destination->server; | ||
|
|
||
| VolumeCloneJob::dispatch($sourceVolume, $targetVolume, $sourceServer, $targetServer, $newPersistentVolume); | ||
|
|
||
| StartService::dispatch($database->service); | ||
| } catch (\Exception $e) { | ||
| \Log::error('Failed to copy volume data for '.$volume->name.': '.$e->getMessage()); | ||
| if ($database->service_id) { | ||
| try { | ||
| StopService::dispatch($database->service); | ||
| $sourceVolume = $volume->name; | ||
| $targetVolume = $newPersistentVolume->name; | ||
| $sourceServer = $database->service->destination->server; | ||
| $targetServer = $new_resource->destination->server; | ||
|
|
||
| VolumeCloneJob::dispatch($sourceVolume, $targetVolume, $sourceServer, $targetServer, $newPersistentVolume); | ||
|
|
||
| StartService::dispatch($database->service); | ||
| } catch (\Exception $e) { | ||
| \Log::error('Failed to copy volume data for '.$volume->name.': '.$e->getMessage()); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect ServiceDatabase owner handling and service database retrieval paths.
rg -nP -C3 'function\s+databases\s*\(|ServiceDatabase::|application_preview_id|application_id|service_id' app/Models app/Livewire bootstrapRepository: coollabsio/coolify
Length of output: 50374
🏁 Script executed:
rg -nB5 -A2 'function cloneTo|->cloneTo\(' app/Livewire/Project/Shared/ResourceOperations.phpRepository: coollabsio/coolify
Length of output: 296
🏁 Script executed:
rg -nB2 -A5 'VolumeCloneJob|StopService::dispatch' app/Livewire/Project/Shared/ResourceOperations.phpRepository: coollabsio/coolify
Length of output: 2669
🏁 Script executed:
rg -nB20 'if \(\$this->cloneVolumeData\)' app/Livewire/Project/Shared/ResourceOperations.php | head -80Repository: coollabsio/coolify
Length of output: 3097
🏁 Script executed:
sed -n '260,350p' app/Livewire/Project/Shared/ResourceOperations.phpRepository: coollabsio/coolify
Length of output: 4298
🏁 Script executed:
sed -n '375,400p' app/Livewire/Project/CloneMe.phpRepository: coollabsio/coolify
Length of output: 1428
🏁 Script executed:
rg -A3 'public function databases\(\)' app/Models/Service.phpRepository: coollabsio/coolify
Length of output: 160
🏁 Script executed:
git log --oneline -p -- app/Livewire/Project/Shared/ResourceOperations.php | grep -A5 -B5 'service_id.*cloneVolumeData' | head -30Repository: coollabsio/coolify
Length of output: 44
🏁 Script executed:
rg -B5 -A10 'public function databases.*HasMany' app/Models/ServiceDatabase.phpRepository: coollabsio/coolify
Length of output: 44
🏁 Script executed:
rg -B2 -A8 'class ServiceDatabase' app/Models/ServiceDatabase.php | head -20Repository: coollabsio/coolify
Length of output: 310
🏁 Script executed:
python3 << 'EOF'
# Verify Laravel hasMany semantics: should only return records matching parent foreign key
# Service.databases() = hasMany(ServiceDatabase::class)
# Should only return ServiceDatabase where service_id = Service.id
print("Laravel hasMany(ServiceDatabase::class) without explicit foreign key:")
print("- Uses convention: service_id (singular of 'Service' + '_id')")
print("- Query: SELECT * FROM service_databases WHERE service_id = ?")
print("- Guard 'if ($database->service_id)' checks something hasMany should guarantee")
print("\nConclusion: Guard is defensive; databases from Service->databases() should always have service_id")
EOFRepository: coollabsio/coolify
Length of output: 433
🏁 Script executed:
sed -n '308,348p' app/Livewire/Project/Shared/ResourceOperations.php | cat -nRepository: coollabsio/coolify
Length of output: 2395
Handle all ServiceDatabase owner types in volume cloning logic.
The cloneVolumeData path only handles service_id-owned databases but silently skips cloning when the database is owned by application_id or application_preview_id. Since ServiceDatabase now supports all three owner types (confirmed in the model), cloning should either apply owner-aware handling (as ScheduledDatabaseBackup does with its multi-branch pattern) or fail fast if the owner type is unsupported—not silently succeed while skipping the requested volume copy.
🧰 Tools
🪛 PHPMD (2.15.0)
[error] 336-336: Avoid using static access to class '\App\Actions\Service\StopService' in method 'cloneTo'. (undefined)
(StaticAccess)
[error] 342-342: Avoid using static access to class '\App\Jobs\VolumeCloneJob' in method 'cloneTo'. (undefined)
(StaticAccess)
[error] 344-344: Avoid using static access to class '\App\Actions\Service\StartService' in method 'cloneTo'. (undefined)
(StaticAccess)
[error] 346-346: Avoid using static access to class '\Log' in method 'cloneTo'. (undefined)
(StaticAccess)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Livewire/Project/Shared/ResourceOperations.php` around lines 333 - 348,
The cloneVolumeData branch only handles ServiceDatabase instances with a
service_id and silently skips application-owned databases; update the cloning
logic in ResourceOperations (the cloneVolumeData block that currently accesses
$database->service, StopService::dispatch, VolumeCloneJob::dispatch, and
StartService::dispatch) to detect the ServiceDatabase owner type (service_id,
application_id, application_preview_id) and perform owner-specific handling
similar to ScheduledDatabaseBackup: for service-owned use the existing
StopService/VolumeCloneJob/StartService flow, for application-owned resolve the
appropriate application server/volume and dispatch VolumeCloneJob with those
targets, and if an owner type cannot be resolved throw or log an explicit error
and return/fail fast instead of silently skipping. Ensure the code references
$database->service, $database->application / $database->applicationPreview (or
equivalent accessors) and $newPersistentVolume when building dispatch arguments.
| return false; | ||
| } | ||
|
|
||
| $running_backup = \App\Models\ScheduledDatabaseBackupExecution::where('status', 'running') |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use the constant, don't repeat the magic string, my friend.
ScheduledDatabaseBackupExecution::STATUS_RUNNING already exists (see app/Models/ScheduledDatabaseBackupExecution.php:9). DatabaseBackupJob was updated to use constants per the prior review — but this gate still hardcodes 'running'. If someone renames the status, I'll terminate your deploys silently.
🔧 Proposed change
- $running_backup = \App\Models\ScheduledDatabaseBackupExecution::where('status', 'running')
+ $running_backup = \App\Models\ScheduledDatabaseBackupExecution::where('status', \App\Models\ScheduledDatabaseBackupExecution::STATUS_RUNNING)Also consider adding a use import at the top so you're not yelling \App\Models\... in every line like Arnold in 1984.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| $running_backup = \App\Models\ScheduledDatabaseBackupExecution::where('status', 'running') | |
| $running_backup = \App\Models\ScheduledDatabaseBackupExecution::where('status', \App\Models\ScheduledDatabaseBackupExecution::STATUS_RUNNING) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bootstrap/helpers/applications.php` at line 154, Replace the hardcoded
'running' string with the model constant
ScheduledDatabaseBackupExecution::STATUS_RUNNING in the query that sets
$running_backup; update the top of the file to add a use import for
App\Models\ScheduledDatabaseBackupExecution so you can reference the class
directly (remove the leading backslash from the existing fully-qualified usages)
and ensure all other status checks in this file use the same constant.
AkprasadoP
force-pushed
the
feat/integrated-backups
branch
from
ad64926 to
e27874d
Compare
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/Models/ServiceDatabase.php (1)
48-63:⚠️ Potential issue | 🟡 MinorOwner-invariant is robust, but there's a cascade-delete blind spot worth considering.
The
static::savingcheck correctly enforces "exactly one owner." However, withonDelete('cascade')on all three FKs, if a parentApplicationorApplicationPreviewis hard-deleted at the DB level (not via Eloquentdelete()), the DB engine will cascade-delete theServiceDatabaserows directly — bypassing thedeletinghook at lines 43–47 that cleans uppersistentStorages,fileStorages, andscheduledBackups. Result: orphaned backup schedules and storage rows still pointing at a ghost DB. Like a taco without the shell, nothing holds together.The
forceDeletinghooks onApplication/ApplicationPreview(added elsewhere in this PR) mitigate this for force-deletes routed through Eloquent, but any rawDB::table('applications')->delete(...)or equivalent still skips the cleanup. Either:
- Keep the current mitigation and add a code comment documenting the invariant, or
- Switch the three FKs to
onDelete('set null')+ a scheduled cleanup job that hard-deletes orphans, so the Eloquent hooks always get a chance to fire.Not strictly blocking — just flagging so future maintainers don't get terminated by a weird data-consistency bug.
Based on learnings, database work should follow Eloquent ORM patterns, migration best practices, relationship definitions, and query optimization as documented in
.ai/patterns/database-patterns.md.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/ServiceDatabase.php` around lines 48 - 63, The saving-hook correctly enforces exactly-one-owner but cascaded DB-level deletes can bypass the deleting() hook that cleans up persistentStorages, fileStorages, and scheduledBackups (see the deleting hook around lines 43–47 and the static::saving block), so either document this invariant or change the three foreign-key constraints from onDelete('cascade') to onDelete('set null') and implement a scheduled cleanup job that deletes orphaned ServiceDatabase rows (orphan cleanup runs against null owner_id) so Eloquent deleting/forceDeleting hooks on Application/ApplicationPreview still run; update migrations/relations accordingly and add a clear code comment referencing static::saving and the deleting hook about the chosen approach.app/Jobs/DatabaseBackupJob.php (1)
277-280:⚠️ Potential issue | 🔴 CriticalCritical: line 279 nukes your container_name for application-owned MongoDB — and references an undefined variable.
Let me spell this out because it's a clean kill:
- For
service_id-backed DBs,$serviceUuidis set on line 164 and$this->container_nameis set on line 166 — so line 279 is just a harmless redundant re-assignment.- For
application_idorapplication_preview_id-backed DBs (the whole point of this PR!),$serviceUuidis never defined — the application/preview branch at lines 168–187 only sets$application,$serviceName, and$this->container_name. When you hit line 279, PHP emits an "undefined variable" warning and reassigns$this->container_name = "{name}-"(i.e., the carefully resolved compose container name from line 181 gets obliterated and replaced with an empty-suffix string).- The subsequent
docker exec $this->container_name env | grep MONGO_INITDB_on line 285 then targets a phantom container, backups of Git-based Docker Compose MongoDB silently fail, and your backup terminator stabs at thin air.Just delete the line — lines 166 / 172-175 / 181-186 have already resolved
$this->container_namecorrectly for every branch. Hasta la vista, clobber bug. 🤖🐛 Proposed fix
} elseif (str($databaseType)->contains('mongo')) { $databasesToBackup = ['*']; - $this->container_name = "{$this->database->name}-$serviceUuid"; $this->directory_name = $serviceName.'-'.$this->container_name;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Jobs/DatabaseBackupJob.php` around lines 277 - 280, Remove the erroneous reassignment that sets $this->container_name using the undefined $serviceUuid in DatabaseBackupJob (the code that does $this->container_name = "{$this->database->name}-$serviceUuid"); the container name is already correctly set in the service_id and application_id/application_preview_id branches, so delete that line to avoid clobbering $this->container_name (which later is used by the docker exec env | grep MONGO_INITDB_ step).
♻️ Duplicate comments (1)
resources/views/livewire/project/application/backups.blade.php (1)
21-21:⚠️ Potential issue | 🟠 MajorAdd a unique key to the create-backup child component.
This component is rendered inside a database loop. Without a per-database
:key, Livewire can reuse stale child state between cards. The Terminator has seen state bleed; it is not gluten-free.🐛 Proposed fix
- <livewire:project.database.create-scheduled-backup :database="$database" /> + <livewire:project.database.create-scheduled-backup :database="$database" :key="'create-scheduled-backup-'.$database->id" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/views/livewire/project/application/backups.blade.php` at line 21, The Livewire child component <livewire:project.database.create-scheduled-backup> is rendered inside a database loop and needs a unique key to avoid state bleeding; update the component invocation to add a per-database :key (for example using the database id or name) so each instance is uniquely keyed and Livewire cannot reuse stale child state for different databases.
🤖 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/Actions/Database/StartDatabaseProxy.php`:
- Around line 44-54: The file uses the fully-qualified class name
\Symfony\Component\Yaml\Yaml::parse(...) in two places (the $compose assignments
inside the application branch and the application_preview/preview branch) even
though Yaml is already imported at the top; replace both occurrences with the
imported Yaml::parse(...) to remove the redundant FQN and satisfy the
MissingImport warning (look for the $compose = ... lines in
StartDatabaseProxy::handle or surrounding logic and update both branches).
In `@app/Jobs/DatabaseBackupJob.php`:
- Around line 161-188: Extract the duplicated container-name resolution into a
single helper function like resolveServiceDatabaseContainer(ServiceDatabase
$db): ?string in bootstrap/helpers/docker.php; this helper should implement the
exact logic used in DatabaseBackupJob (check $db->service_id -> use
$db->service->uuid and slugged service/application name; otherwise check
$db->application_id and $db->application_preview, parse
$application->docker_compose for services.<name>.container_name and fallback to
generateApplicationContainerName($application, $preview->pull_request_id|null)
when container_name is missing). Replace the inline resolution in
DatabaseBackupJob (the block inside if (data_get($this->backup, 'database_type')
=== ServiceDatabase::class) that references $this->database,
$this->container_name, application, preview, docker_compose,
generateApplicationContainerName) with a call to
resolveServiceDatabaseContainer($this->database) and do the same replacement in
StartDatabaseProxy::handle().
In `@app/Models/ScheduledDatabaseBackup.php`:
- Around line 105-115: The code in ScheduledDatabaseBackup is manually resolving
server via service/application chains for ServiceDatabase, which can miss
owner-aware logic in ServiceDatabase::getServerAttribute; replace the manual
branches (the checks for service_id, application_id, application_preview_id and
their data_get calls) with a single authoritative resolution using the model
accessor — use $this->database->server (on the ServiceDatabase instance) to
obtain the Server and then proceed, ensuring ScheduledDatabaseBackup references
the ServiceDatabase::getServerAttribute logic rather than direct
service/application.destination lookups.
In `@app/Models/ServiceDatabase.php`:
- Around line 92-109: The restart() method on ServiceDatabase is restarting the
wrong container for app/preview-owned DBs because it currently uses
generateApplicationContainerName(...) instead of the compose service container
name; implement a new ServiceDatabase::resolveContainerName(): ?string helper
that (1) parses the docker_compose YAML for
services.<$this->name>.container_name and returns it if present, and (2) falls
back to "{$this->name}-" . generateApplicationContainerName($this->application)
(or with pull_request_id for previews) like StartDatabaseProxy::handle() and
DatabaseBackupJob::handle() do; then replace the container resolution logic in
restart() to call resolveContainerName() and use that result when calling
remote_process(["docker restart {$container_id}"], $server).
In `@bootstrap/helpers/database.php`:
- Around line 9-62: Wrap the body of updateResourceDatabases in a
DB::transaction so all ServiceDatabase create/update/delete operations
(ServiceDatabase::create, $databaseRecord->update, $existingDb->delete) are
performed atomically and roll back on error; move the building of
$existingDatabases, the loop over $detectedDatabases, and the cleanup loop into
the transaction closure. Also change the final membership check from
in_array($existingDb->name, $processedNames) to in_array($existingDb->name,
$processedNames, true) to use strict comparison and avoid type-juggling issues.
---
Outside diff comments:
In `@app/Jobs/DatabaseBackupJob.php`:
- Around line 277-280: Remove the erroneous reassignment that sets
$this->container_name using the undefined $serviceUuid in DatabaseBackupJob (the
code that does $this->container_name = "{$this->database->name}-$serviceUuid");
the container name is already correctly set in the service_id and
application_id/application_preview_id branches, so delete that line to avoid
clobbering $this->container_name (which later is used by the docker exec env |
grep MONGO_INITDB_ step).
In `@app/Models/ServiceDatabase.php`:
- Around line 48-63: The saving-hook correctly enforces exactly-one-owner but
cascaded DB-level deletes can bypass the deleting() hook that cleans up
persistentStorages, fileStorages, and scheduledBackups (see the deleting hook
around lines 43–47 and the static::saving block), so either document this
invariant or change the three foreign-key constraints from onDelete('cascade')
to onDelete('set null') and implement a scheduled cleanup job that deletes
orphaned ServiceDatabase rows (orphan cleanup runs against null owner_id) so
Eloquent deleting/forceDeleting hooks on Application/ApplicationPreview still
run; update migrations/relations accordingly and add a clear code comment
referencing static::saving and the deleting hook about the chosen approach.
---
Duplicate comments:
In `@resources/views/livewire/project/application/backups.blade.php`:
- Line 21: The Livewire child component
<livewire:project.database.create-scheduled-backup> is rendered inside a
database loop and needs a unique key to avoid state bleeding; update the
component invocation to add a per-database :key (for example using the database
id or name) so each instance is uniquely keyed and Livewire cannot reuse stale
child state for different databases.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1ede23a0-9b0d-4ab7-bbfc-7774b8400c88
📒 Files selected for processing (20)
app/Actions/Database/StartDatabaseProxy.phpapp/Jobs/DatabaseBackupJob.phpapp/Livewire/Project/Application/Backups.phpapp/Livewire/Project/Application/Configuration.phpapp/Livewire/Project/CloneMe.phpapp/Livewire/Project/Shared/ResourceOperations.phpapp/Models/Application.phpapp/Models/ApplicationPreview.phpapp/Models/ScheduledDatabaseBackup.phpapp/Models/ScheduledDatabaseBackupExecution.phpapp/Models/ServiceDatabase.phpbootstrap/helpers/applications.phpbootstrap/helpers/database.phpbootstrap/helpers/databases.phpbootstrap/helpers/parsers.phpbootstrap/helpers/shared.phpdatabase/migrations/2026_04_19_034518_add_application_id_to_service_databases.phpresources/views/livewire/project/application/backups.blade.phpresources/views/livewire/project/application/configuration.blade.phproutes/web.php
Comment on lines
+44
to
+54
| $compose = \Symfony\Component\Yaml\Yaml::parse($application?->docker_compose ?: '') ?: []; | ||
| $containerName = data_get($compose, 'services.' . $database->name . '.container_name'); | ||
| if (! $containerName) { | ||
| $containerName = "{$database->name}-" . generateApplicationContainerName($application); | ||
| } | ||
| } else { | ||
| $preview = $database->application_preview; | ||
| $application = $preview?->application; | ||
| $network = data_get($application, 'destination.network'); | ||
| $server = data_get($application, 'destination.server'); | ||
| $compose = \Symfony\Component\Yaml\Yaml::parse($application?->docker_compose ?: '') ?: []; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use the already-imported Yaml class, Skynet.
Symfony\Component\Yaml\Yaml is already imported at line 15, but lines 44 and 54 still use the fully-qualified \Symfony\Component\Yaml\Yaml::parse(...). It's purely cosmetic — no code will be terminated — but it's inconsistent with the rest of the file. PHPMD is also squinting at the "MissingImport" note for the fully-qualified reference.
♻️ Proposed cleanup
- $compose = \Symfony\Component\Yaml\Yaml::parse($application?->docker_compose ?: '') ?: [];
+ $compose = Yaml::parse($application?->docker_compose ?: '') ?: [];Apply in both the application and application_preview branches.
🧰 Tools
🪛 PHPMD (2.15.0)
[error] 44-44: Avoid using static access to class '\Symfony\Component\Yaml\Yaml' in method 'handle'. (undefined)
(StaticAccess)
[error] 54-54: Avoid using static access to class '\Symfony\Component\Yaml\Yaml' in method 'handle'. (undefined)
(StaticAccess)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Actions/Database/StartDatabaseProxy.php` around lines 44 - 54, The file
uses the fully-qualified class name \Symfony\Component\Yaml\Yaml::parse(...) in
two places (the $compose assignments inside the application branch and the
application_preview/preview branch) even though Yaml is already imported at the
top; replace both occurrences with the imported Yaml::parse(...) to remove the
redundant FQN and satisfy the MissingImport warning (look for the $compose = ...
lines in StartDatabaseProxy::handle or surrounding logic and update both
branches).
Comment on lines
161
to
+188
| if (data_get($this->backup, 'database_type') === ServiceDatabase::class) { | ||
| $databaseType = $this->database->databaseType(); | ||
| $serviceUuid = $this->database->service->uuid; | ||
| $serviceName = str($this->database->service->name)->slug(); | ||
| if ($this->database->service_id) { | ||
| $serviceUuid = $this->database->service->uuid; | ||
| $serviceName = str($this->database->service->name)->slug(); | ||
| $this->container_name = "{$this->database->name}-{$serviceUuid}"; | ||
| } else { | ||
| if ($this->database->application_id) { | ||
| $application = $this->database->application; | ||
| $serviceName = str($application->name)->slug(); | ||
| $compose = \Symfony\Component\Yaml\Yaml::parse($application?->docker_compose ?: '') ?: []; | ||
| $this->container_name = data_get($compose, 'services.' . $this->database->name . '.container_name'); | ||
| if (! $this->container_name) { | ||
| $this->container_name = "{$this->database->name}-" . generateApplicationContainerName($application); | ||
| } | ||
| } else { | ||
| $preview = $this->database->application_preview; | ||
| $application = $preview?->application; | ||
| $serviceName = str($application?->name)->slug(); | ||
| $compose = \Symfony\Component\Yaml\Yaml::parse($application?->docker_compose ?: '') ?: []; | ||
| $this->container_name = data_get($compose, 'services.' . $this->database->name . '.container_name'); | ||
| if (! $this->container_name) { | ||
| $this->container_name = $preview && $application | ||
| ? "{$this->database->name}-" . generateApplicationContainerName($application, $preview->pull_request_id) | ||
| : null; | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract the compose container-name resolution into a shared helper.
This exact resolution chain — service_id → direct uuid, application_id/application_preview_id → parse docker_compose for services.<name>.container_name with fallback to generateApplicationContainerName(...) — is now duplicated verbatim between StartDatabaseProxy::handle() (lines 34–62) and this job. Every future refactor has to edit both or the two code paths will drift apart like a T-800 and T-1000 on divergent orders — and the proxy will happily forward to one container while the backup docker execs into another.
Pull the resolution into a helper (e.g. resolveServiceDatabaseContainer(ServiceDatabase $db): ?string in bootstrap/helpers/docker.php) and call it from both sites. Self-hosted discipline — DRY, no drift, no VC-marketing "just spin up another one" escape hatch.
🧰 Tools
🪛 PHPMD (2.15.0)
[error] 167-188: The method handle uses an else expression. Else clauses are basically not necessary and you can simplify the code by not using them. (undefined)
(ElseExpression)
[error] 171-171: Avoid using static access to class '\Symfony\Component\Yaml\Yaml' in method 'handle'. (undefined)
(StaticAccess)
[error] 176-187: The method handle uses an else expression. Else clauses are basically not necessary and you can simplify the code by not using them. (undefined)
(ElseExpression)
[error] 180-180: Avoid using static access to class '\Symfony\Component\Yaml\Yaml' in method 'handle'. (undefined)
(StaticAccess)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Jobs/DatabaseBackupJob.php` around lines 161 - 188, Extract the
duplicated container-name resolution into a single helper function like
resolveServiceDatabaseContainer(ServiceDatabase $db): ?string in
bootstrap/helpers/docker.php; this helper should implement the exact logic used
in DatabaseBackupJob (check $db->service_id -> use $db->service->uuid and
slugged service/application name; otherwise check $db->application_id and
$db->application_preview, parse $application->docker_compose for
services.<name>.container_name and fallback to
generateApplicationContainerName($application, $preview->pull_request_id|null)
when container_name is missing). Replace the inline resolution in
DatabaseBackupJob (the block inside if (data_get($this->backup, 'database_type')
=== ServiceDatabase::class) that references $this->database,
$this->container_name, application, preview, docker_compose,
generateApplicationContainerName) with a call to
resolveServiceDatabaseContainer($this->database) and do the same replacement in
StartDatabaseProxy::handle().
Comment on lines
105
to
+115
| if ($this->database instanceof ServiceDatabase) { | ||
| $destination = data_get($this->database->service, 'destination'); | ||
| $server = data_get($destination, 'server'); | ||
| if ($this->database->service_id) { | ||
| $destination = data_get($this->database->service, 'destination'); | ||
| $server = data_get($destination, 'server'); | ||
| } elseif ($this->database->application_id) { | ||
| $destination = data_get($this->database->application, 'destination'); | ||
| $server = data_get($destination, 'server'); | ||
| } elseif ($this->database->application_preview_id) { | ||
| $destination = data_get($this->database->application_preview?->application, 'destination'); | ||
| $server = data_get($destination, 'server'); | ||
| } |
Contributor
There was a problem hiding this comment.
Use the ServiceDatabase server accessor for owner-aware resolution.
For service-owned databases this uses service.destination.server, but ServiceDatabase::getServerAttribute() resolves that owner via service.server in app/Models/ServiceDatabase.php:111-124. This can return null for existing service-owned backups and break backup execution.
🐛 Proposed fix
$server = null;
if ($this->database instanceof ServiceDatabase) {
- if ($this->database->service_id) {
- $destination = data_get($this->database->service, 'destination');
- $server = data_get($destination, 'server');
- } elseif ($this->database->application_id) {
- $destination = data_get($this->database->application, 'destination');
- $server = data_get($destination, 'server');
- } elseif ($this->database->application_preview_id) {
- $destination = data_get($this->database->application_preview?->application, 'destination');
- $server = data_get($destination, 'server');
- }
+ $server = $this->database->server;
} else {
$destination = data_get($this->database, 'destination');
$server = data_get($destination, 'server');
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Models/ScheduledDatabaseBackup.php` around lines 105 - 115, The code in
ScheduledDatabaseBackup is manually resolving server via service/application
chains for ServiceDatabase, which can miss owner-aware logic in
ServiceDatabase::getServerAttribute; replace the manual branches (the checks for
service_id, application_id, application_preview_id and their data_get calls)
with a single authoritative resolution using the model accessor — use
$this->database->server (on the ServiceDatabase instance) to obtain the Server
and then proceed, ensuring ScheduledDatabaseBackup references the
ServiceDatabase::getServerAttribute logic rather than direct
service/application.destination lookups.
Comment on lines
92
to
+109
| public function restart() | ||
| { | ||
| $container_id = $this->name.'-'.$this->service->uuid; | ||
| remote_process(["docker restart {$container_id}"], $this->service->server); | ||
| $server = null; | ||
| $container_id = null; | ||
| if ($this->service_id) { | ||
| $container_id = $this->name.'-'.$this->service->uuid; | ||
| $server = $this->service->server; | ||
| } elseif ($this->application_id) { | ||
| $container_id = generateApplicationContainerName($this->application); | ||
| $server = $this->application->destination->server; | ||
| } elseif ($this->application_preview_id) { | ||
| $container_id = generateApplicationContainerName($this->application_preview->application, $this->application_preview->pull_request_id); | ||
| $server = $this->application_preview->application->destination->server; | ||
| } | ||
| if ($container_id && $server) { | ||
| remote_process(["docker restart {$container_id}"], $server); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
restart() will target the wrong container for Docker-Compose–owned databases.
For app- and preview-owned databases (lines 99–104), $container_id = generateApplicationContainerName(...) resolves the application's base container name — not the compose service container that actually holds the database. The rest of the codebase (StartDatabaseProxy::handle() and DatabaseBackupJob::handle()) correctly builds the DB container by:
- Reading
services.<$this->name>.container_namefrom the parseddocker_composeYAML, and - Falling back to
"{$this->name}-" . generateApplicationContainerName(...).
Calling restart() on an application-owned DB as written here will docker restart a nonexistent / wrong container — which is worse than serverless: at least when AWS loses your function, you get a cold-start, not a silent no-op on the DB you actually wanted to cycle.
🐛 Proposed fix — mirror the resolution used by the proxy + backup job
public function restart()
{
$server = null;
$container_id = null;
if ($this->service_id) {
$container_id = $this->name.'-'.$this->service->uuid;
$server = $this->service->server;
} elseif ($this->application_id) {
- $container_id = generateApplicationContainerName($this->application);
$server = $this->application->destination->server;
+ $compose = \Symfony\Component\Yaml\Yaml::parse($this->application?->docker_compose ?: '') ?: [];
+ $container_id = data_get($compose, "services.{$this->name}.container_name")
+ ?? $this->name.'-'.generateApplicationContainerName($this->application);
} elseif ($this->application_preview_id) {
$preview = $this->application_preview;
- $container_id = generateApplicationContainerName($preview->application, $preview->pull_request_id);
$server = $preview->application->destination->server;
+ $compose = \Symfony\Component\Yaml\Yaml::parse($preview?->application?->docker_compose ?: '') ?: [];
+ $container_id = data_get($compose, "services.{$this->name}.container_name")
+ ?? $this->name.'-'.generateApplicationContainerName($preview->application, $preview->pull_request_id);
}
if ($container_id && $server) {
remote_process(["docker restart {$container_id}"], $server);
}
}Better yet: extract the resolution into a single resolveContainerName(): ?string helper on ServiceDatabase and call it from restart(), StartDatabaseProxy, and DatabaseBackupJob. Three sites of drifting logic is a recipe for Skynet-level divergence.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Models/ServiceDatabase.php` around lines 92 - 109, The restart() method
on ServiceDatabase is restarting the wrong container for app/preview-owned DBs
because it currently uses generateApplicationContainerName(...) instead of the
compose service container name; implement a new
ServiceDatabase::resolveContainerName(): ?string helper that (1) parses the
docker_compose YAML for services.<$this->name>.container_name and returns it if
present, and (2) falls back to "{$this->name}-" .
generateApplicationContainerName($this->application) (or with pull_request_id
for previews) like StartDatabaseProxy::handle() and DatabaseBackupJob::handle()
do; then replace the container resolution logic in restart() to call
resolveContainerName() and use that result when calling remote_process(["docker
restart {$container_id}"], $server).
Comment on lines
+9
to
+62
| function updateResourceDatabases(Service|Application|ApplicationPreview $resource, Collection $detectedDatabases): void | ||
| { | ||
| $resourceId = $resource->id; | ||
| $column = match (true) { | ||
| $resource instanceof Service => 'service_id', | ||
| $resource instanceof Application => 'application_id', | ||
| $resource instanceof ApplicationPreview => 'application_preview_id', | ||
| }; | ||
|
|
||
| $existingDatabases = ServiceDatabase::where($column, $resourceId)->get(); | ||
|
|
||
| $processedNames = []; | ||
|
|
||
| foreach ($detectedDatabases as $serviceName => $serviceConfig) { | ||
| $image = data_get($serviceConfig, 'image'); | ||
| $ports = data_get($serviceConfig, 'ports', []); | ||
|
|
||
| $collectedPorts = collect($ports)->map(function ($sport) { | ||
| if (is_string($sport) || is_numeric($sport)) { | ||
| return $sport; | ||
| } | ||
| if (is_array($sport)) { | ||
| $target = data_get($sport, 'target'); | ||
| $published = data_get($sport, 'published'); | ||
| $protocol = data_get($sport, 'protocol', 'tcp'); | ||
| return $published ? "$target:$published/$protocol" : "$target/$protocol"; | ||
| } | ||
| return null; | ||
| })->filter()->implode(','); | ||
|
|
||
| $databaseRecord = $existingDatabases->where('name', $serviceName)->first(); | ||
|
|
||
| $data = [ | ||
| 'name' => $serviceName, | ||
| 'image' => $image, | ||
| 'ports' => $collectedPorts, | ||
| $column => $resourceId, | ||
| ]; | ||
|
|
||
| if ($databaseRecord) { | ||
| $databaseRecord->update($data); | ||
| } else { | ||
| ServiceDatabase::create($data); | ||
| } | ||
|
|
||
| $processedNames[] = $serviceName; | ||
| } | ||
|
|
||
| foreach ($existingDatabases as $existingDb) { | ||
| if (!in_array($existingDb->name, $processedNames)) { | ||
| $existingDb->delete(); | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider wrapping the sync in a DB transaction for atomicity.
If any of the update/create/delete calls throws mid-loop, you'll be left with half-synced ServiceDatabase rows — some new, some stale, some missing. Wrapping the body in DB::transaction(...) makes the sync all-or-nothing, so a failed parse doesn't leave orphaned backup schedules lying around like leftover gluten crumbs. Hasta la vista, partial state.
Also, !in_array($existingDb->name, $processedNames) at line 58 should pass strict: true — both sides are strings so it's currently safe, but strict-mode comparison terminates entire classes of type-juggling bugs before they show up.
♻️ Suggested refactor
+use Illuminate\Support\Facades\DB;
+
function updateResourceDatabases(Service|Application|ApplicationPreview $resource, Collection $detectedDatabases): void
{
- $resourceId = $resource->id;
- $column = match (true) {
- $resource instanceof Service => 'service_id',
- $resource instanceof Application => 'application_id',
- $resource instanceof ApplicationPreview => 'application_preview_id',
- };
-
- $existingDatabases = ServiceDatabase::where($column, $resourceId)->get();
- ...
- foreach ($existingDatabases as $existingDb) {
- if (!in_array($existingDb->name, $processedNames)) {
- $existingDb->delete();
- }
- }
+ DB::transaction(function () use ($resource, $detectedDatabases): void {
+ // ... existing body ...
+ foreach ($existingDatabases as $existingDb) {
+ if (! in_array($existingDb->name, $processedNames, true)) {
+ $existingDb->delete();
+ }
+ }
+ });
}Based on learnings, database work should follow Eloquent ORM patterns, migration best practices, relationship definitions, and query optimization as documented in .ai/patterns/database-patterns.md.
🧰 Tools
🪛 PHPMD (2.15.0)
[error] 50-52: The method updateResourceDatabases uses an else expression. Else clauses are basically not necessary and you can simplify the code by not using them. (undefined)
(ElseExpression)
[error] 51-51: Avoid using static access to class '\App\Models\ServiceDatabase' in method 'updateResourceDatabases'. (undefined)
(StaticAccess)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bootstrap/helpers/database.php` around lines 9 - 62, Wrap the body of
updateResourceDatabases in a DB::transaction so all ServiceDatabase
create/update/delete operations (ServiceDatabase::create,
$databaseRecord->update, $existingDb->delete) are performed atomically and roll
back on error; move the building of $existingDatabases, the loop over
$detectedDatabases, and the cleanup loop into the transaction closure. Also
change the final membership check from in_array($existingDb->name,
$processedNames) to in_array($existingDb->name, $processedNames, true) to use
strict comparison and avoid type-juggling issues.
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.
Changes
Git-based Docker Compose applications previously had no backup support. This PR bridges that gap by reusing the existing ServiceDatabase and ScheduledDatabaseBackup infrastructure.
Issues
Category
Preview
git_compose_database_backups.mp4
AI Assistance
If AI was used:
Testing
Contributor Agreement
Important