[Improvement] Email updates + more notifications#1168
Merged
saeedvaziry merged 6 commits intoJun 18, 2026
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds five new notification classes (ServerAutoUpdateCompleted, SslCertificateExpiring, SslRenewalFailed, BackupFailed, WebhookDeploymentFailed) triggered from jobs and commands, refactors the OS upgrade SSH script to emit structured parsed output, introduces SSL expiry state tracking via a new ChangesNotification System and Email Template Overhaul
Sequence DiagramsequenceDiagram
participant RunServerAutoUpdatesCommand
participant UpdateJob
participant OS
participant Notifier
participant CheckSslExpiryJob
participant SslCertificateExpiring
rect rgba(100, 149, 237, 0.5)
Note over RunServerAutoUpdatesCommand,Notifier: Auto-update flow
RunServerAutoUpdatesCommand->>UpdateJob: dispatch(server, notify=true)
UpdateJob->>OS: upgrade()
OS-->>UpdateJob: {upgraded: int, reboot_required: bool}
UpdateJob->>Notifier: send(server, ServerAutoUpdateCompleted)
end
rect rgba(60, 179, 113, 0.5)
Note over CheckSslExpiryJob,SslCertificateExpiring: SSL expiry check flow
CheckSslExpiryJob->>CheckSslExpiryJob: checkCertificate(ssl) — parse and dirty flag
CheckSslExpiryJob->>CheckSslExpiryJob: handleExpiryNotification(ssl)
alt within 14-day window and not yet notified
CheckSslExpiryJob->>Notifier: send(server, SslCertificateExpiring)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
app/Jobs/Site/TriggerDeployFromWebhookJob.php (1)
38-45:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not swallow deploy exceptions in the webhook flow
This catch block logs and notifies, but then suppresses the original failure. That marks the job as successful, so retry/failure handling is skipped for real deploy errors. Re-throw after side effects (or move notification to
failed()to avoid per-attempt duplicates).Suggested fix
} catch (Throwable $e) { ServerLog::log($site->server, 'deploy-failed', $e->getMessage(), $site); Log::error('webhook-deploy-failed', [ 'site_id' => $site->id, 'error' => $e->getMessage(), ]); Notifier::send($site, new WebhookDeploymentFailed($site)); + throw $e; }As per coding guidelines, "Let provider/service errors bubble up — don't silently catch and suppress exceptions."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Jobs/Site/TriggerDeployFromWebhookJob.php` around lines 38 - 45, The catch block in TriggerDeployFromWebhookJob that handles Throwable exceptions is suppressing the original failure by not re-throwing it, which causes the job to be marked as successful even when deployment fails. After performing the side effects (ServerLog::log, Log::error, and Notifier::send with WebhookDeploymentFailed), re-throw the exception to allow the queue system's retry and failure handling to work properly. This ensures that real deploy errors are not silently swallowed and the job failure is properly recorded.Source: Coding guidelines
app/Console/Commands/RenewSslCertificatesCommand.php (1)
19-28: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winEager-load
serverto avoid N+1 during renewal sweep.
$ssl->serverinside the cursor callback issues an extra query per SSL record. Add eager loading on the base query.Proposed fix
Ssl::query() + ->with('server') ->where('type', SslType::LETSENCRYPT) ->where('is_wildcard', true) ->whereNotNull('server_id')As per coding guidelines: “Eager load relationships to prevent N+1 queries.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Console/Commands/RenewSslCertificatesCommand.php` around lines 19 - 28, The query in the RenewSslCertificatesCommand is accessing the server relationship on each Ssl model inside the cursor callback without eager loading it, which causes an N+1 query problem. Add eager loading by chaining ->with('server') to the Ssl::query() builder before calling ->cursor() to load all server relationships in a single query instead of issuing a separate query for each $ssl->server access within the callback function.Source: Coding guidelines
app/Jobs/SSL/CheckSslExpiryJob.php (1)
48-71:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not silently suppress notification/provider failures in this job path.
Line 69 catches every throwable and returns, which now also hides failures from
Notifier::send(...)and persistence. Narrow the catch to certificate-read/parse failures only, and let provider/notification/save errors bubble so queue retries and visibility work correctly.Proposed fix
- try { - $certificate = trim($ssh->exec("sudo cat {$ssl->certificate_path}")); + try { + $certificate = trim($ssh->exec("sudo cat {$ssl->certificate_path}")); if (empty($certificate) || ! str_contains($certificate, 'BEGIN CERTIFICATE')) { return; } $parsed = CertificateParser::parse($certificate); - $dirty = false; - - if (! $ssl->expires_at?->equalTo($parsed['expires_at'])) { - $ssl->expires_at = $parsed['expires_at']; - $ssl->domains = $parsed['domains']; - $dirty = true; - } - - $dirty = $this->handleExpiryNotification($ssl) || $dirty; - - if ($dirty) { - $ssl->save(); - } - } catch (Throwable) { + } catch (Throwable) { return; } + + $dirty = false; + if (! $ssl->expires_at?->equalTo($parsed['expires_at'])) { + $ssl->expires_at = $parsed['expires_at']; + $ssl->domains = $parsed['domains']; + $dirty = true; + } + + $dirty = $this->handleExpiryNotification($ssl) || $dirty; + if ($dirty) { + $ssl->save(); + }As per coding guidelines: “Let provider/service errors bubble up — flag silently-swallowed exceptions.”
Also applies to: 100-101
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Jobs/SSL/CheckSslExpiryJob.php` around lines 48 - 71, The current catch (Throwable) block is too broad and suppresses all exceptions including failures from handleExpiryNotification() and $ssl->save(), preventing queue retries and error visibility. Narrow the try-catch scope to only wrap the certificate reading and parsing operations (the SSH exec call and CertificateParser::parse() call), and move the handleExpiryNotification() call and $ssl->save() invocation outside the try-catch block so that notification and persistence failures bubble up and are properly handled by the queue system. Specify a more narrow exception type in the catch block if appropriate for certificate-related failures only.Source: Coding guidelines
app/Notifications/ServerInstallationSucceed.php (1)
16-21:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove plaintext SSH password from notification content.
Line 20 and Line 35 expose
authentication['pass']in outbound notification text/email. That is a credential disclosure path if inboxes are compromised or forwarded.Suggested fix
- return __("Installation succeed for server [:server] \nServer IP: :ip \nUser: :user\nPassword: :password\n:link", [ + return __("Installation succeed for server [:server] \nServer IP: :ip \nUser: :user\n:link", [ 'server' => $this->server->name, 'ip' => $this->server->ip, 'user' => $this->server->authentication['user'], - 'password' => $this->server->authentication['pass'], 'link' => url('/servers/'.$this->server->id), ]); @@ ->line(__('Your server [:server] has been installed successfully and is ready to use.', ['server' => $this->server->name])) ->line(__('Server IP: :ip', ['ip' => $this->server->ip])) ->line(__('SSH user: :user', ['user' => $this->server->authentication['user']])) - ->line(__('SSH password: :password', ['password' => $this->server->authentication['pass']])) ->action(__('Manage your server'), url('/servers/'.$this->server->id));Also applies to: 33-35
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Notifications/ServerInstallationSucceed.php` around lines 16 - 21, The ServerInstallationSucceed notification exposes the plaintext SSH password in the notification message, which is a security risk. Remove the password field from the notification content by deleting the line that contains 'password' => $this->server->authentication['pass'] from both occurrences in the method (the one around line 20 and the duplicate around line 35), and also remove the corresponding :password placeholder from the notification message string itself. This ensures credentials are not disclosed through outbound notifications or emails.app/NotificationChannels/Email.php (1)
45-55:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDo not swallow mail provider exceptions in
connect()(Line 45).
catch (Throwable) { return false; }suppresses provider/service failures and hides root-cause errors from callers.Suggested fix
public function connect(): bool { - try { - $message = (new MailMessage) - ->success() - ->subject(__('Email notifications connected')) - ->line(__('This confirms that email notifications are now connected for your Vito instance.')); - Mail::to($this->data()['email'])->send( - new NotificationMail($message->subject, $message->render()) - ); - } catch (Throwable) { - return false; - } + $message = (new MailMessage) + ->success() + ->subject(__('Email notifications connected')) + ->line(__('This confirms that email notifications are now connected for your Vito instance.')); + Mail::to($this->data()['email'])->send( + new NotificationMail($message->subject, $message->render()) + ); return true; }As per coding guidelines, “Let provider/service errors bubble up — don't silently catch and suppress exceptions.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/NotificationChannels/Email.php` around lines 45 - 55, Remove the catch block that is catching Throwable exceptions in the connect() method. Instead of silently catching all exceptions and returning false when Mail::to() and send() fail, allow these exceptions to propagate up to the caller so that provider/service errors can be properly handled and logged. Delete the entire try-catch wrapper and keep only the Mail creation and sending logic.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/Jobs/SSL/CheckSslExpiryJob.php`:
- Around line 58-61: In the CheckSslExpiryJob.php file, the condition checking
if `expires_at` needs updating only updates the `domains` field when the expiry
date changes. To prevent stale domains in the database, add a separate check to
update `domains` whenever they differ from the parsed domains, independently of
whether `expires_at` has changed. This ensures that domain/SAN changes are
reflected in the database even if the expiration timestamp remains the same.
---
Outside diff comments:
In `@app/Console/Commands/RenewSslCertificatesCommand.php`:
- Around line 19-28: The query in the RenewSslCertificatesCommand is accessing
the server relationship on each Ssl model inside the cursor callback without
eager loading it, which causes an N+1 query problem. Add eager loading by
chaining ->with('server') to the Ssl::query() builder before calling ->cursor()
to load all server relationships in a single query instead of issuing a separate
query for each $ssl->server access within the callback function.
In `@app/Jobs/Site/TriggerDeployFromWebhookJob.php`:
- Around line 38-45: The catch block in TriggerDeployFromWebhookJob that handles
Throwable exceptions is suppressing the original failure by not re-throwing it,
which causes the job to be marked as successful even when deployment fails.
After performing the side effects (ServerLog::log, Log::error, and
Notifier::send with WebhookDeploymentFailed), re-throw the exception to allow
the queue system's retry and failure handling to work properly. This ensures
that real deploy errors are not silently swallowed and the job failure is
properly recorded.
In `@app/Jobs/SSL/CheckSslExpiryJob.php`:
- Around line 48-71: The current catch (Throwable) block is too broad and
suppresses all exceptions including failures from handleExpiryNotification() and
$ssl->save(), preventing queue retries and error visibility. Narrow the
try-catch scope to only wrap the certificate reading and parsing operations (the
SSH exec call and CertificateParser::parse() call), and move the
handleExpiryNotification() call and $ssl->save() invocation outside the
try-catch block so that notification and persistence failures bubble up and are
properly handled by the queue system. Specify a more narrow exception type in
the catch block if appropriate for certificate-related failures only.
In `@app/NotificationChannels/Email.php`:
- Around line 45-55: Remove the catch block that is catching Throwable
exceptions in the connect() method. Instead of silently catching all exceptions
and returning false when Mail::to() and send() fail, allow these exceptions to
propagate up to the caller so that provider/service errors can be properly
handled and logged. Delete the entire try-catch wrapper and keep only the Mail
creation and sending logic.
In `@app/Notifications/ServerInstallationSucceed.php`:
- Around line 16-21: The ServerInstallationSucceed notification exposes the
plaintext SSH password in the notification message, which is a security risk.
Remove the password field from the notification content by deleting the line
that contains 'password' => $this->server->authentication['pass'] from both
occurrences in the method (the one around line 20 and the duplicate around line
35), and also remove the corresponding :password placeholder from the
notification message string itself. This ensures credentials are not disclosed
through outbound notifications or emails.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8b46920f-0984-4fe2-af44-e97cdfe5e4e4
⛔ Files ignored due to path filters (1)
public/email/logo.pngis excluded by!**/*.png
📒 Files selected for processing (38)
app/Actions/Server/Update.phpapp/Console/Commands/RenewSslCertificatesCommand.phpapp/Console/Commands/RunServerAutoUpdatesCommand.phpapp/Jobs/Backup/RunJob.phpapp/Jobs/SSL/CheckSslExpiryJob.phpapp/Jobs/SSL/CreateLetsEncryptWildcardSslJob.phpapp/Jobs/Server/UpdateJob.phpapp/Jobs/Site/TriggerDeployFromWebhookJob.phpapp/Models/Ssl.phpapp/NotificationChannels/Email.phpapp/Notifications/BackupFailed.phpapp/Notifications/DeploymentCompleted.phpapp/Notifications/FailedToDeleteBackupFileFromProvider.phpapp/Notifications/FailedToDeleteServerFromProvider.phpapp/Notifications/ServerAutoUpdateCompleted.phpapp/Notifications/ServerConnected.phpapp/Notifications/ServerDisconnected.phpapp/Notifications/ServerInstallationFailed.phpapp/Notifications/ServerInstallationStarted.phpapp/Notifications/ServerInstallationSucceed.phpapp/Notifications/ServerUpdateFailed.phpapp/Notifications/SiteInstallationFailed.phpapp/Notifications/SiteInstallationSucceed.phpapp/Notifications/SourceControlDisconnected.phpapp/Notifications/SslCertificateExpiring.phpapp/Notifications/SslRenewalFailed.phpapp/Notifications/WebhookDeploymentFailed.phpapp/SSH/OS/OS.phpconfig/mail.phpdatabase/migrations/2026_06_17_074137_add_expiry_notified_at_to_ssls_table.phpresources/views/emails/project-invitation.blade.phpresources/views/ssh/os/upgrade.blade.phpresources/views/vendor/mail/html/header.blade.phpresources/views/vendor/mail/html/message.blade.phpresources/views/vendor/mail/html/themes/default.csstests/Feature/CheckSslExpiriesCommandTest.phptests/Feature/ServerTest.phptests/Unit/Notifications/NotificationMailTest.php
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/Jobs/SSL/CheckSslExpiryJob.php (1)
16-18:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing
UniqueQueuetrait required by coding guidelines.Queued jobs in this codebase must use both
QueueableandUniqueQueuetraits, and wrap work in$this->run($key, callable)to prevent duplicate jobs via cache locks.Proposed fix
use Illuminate\Contracts\Queue\ShouldQueue; use Illuminate\Foundation\Queue\Queueable; +use App\Traits\UniqueQueue; use Throwable; class CheckSslExpiryJob implements ShouldQueue { use Queueable; + use UniqueQueue; private const EXPIRY_WARNING_DAYS = 14; public function __construct(protected Server $server) {} public function handle(): void { - $ssls = Ssl::query() + $this->run("check-ssl-expiry-{$this->server->id}", function () { + $ssls = Ssl::query() // ... rest of handle logic + }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Jobs/SSL/CheckSslExpiryJob.php` around lines 16 - 18, The CheckSslExpiryJob class is missing the UniqueQueue trait which is required by coding guidelines. Add the UniqueQueue trait to the class alongside the existing Queueable trait. Additionally, wrap any job work logic inside the handle method (or equivalent entry point) with a call to $this->run($key, callable) where the key uniquely identifies the job, to ensure duplicate jobs are prevented via cache locks.Source: Coding guidelines
app/Notifications/SslCertificateExpiring.php (1)
63-66:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMalformed URL when
siterelationship is null.If
$ssl->siteis null (possible due tonullOnDelete()foreign key),link()produces/servers//sites/{id}/domains. Consider returning an empty string or a fallback URL, or guard against this case upstream.Proposed fix
private function link(): string { + if ($this->ssl->site === null) { + return url('/'); + } + return url('/servers/'.$this->ssl->site?->server_id.'/sites/'.$this->ssl->site_id.'/domains'); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Notifications/SslCertificateExpiring.php` around lines 63 - 66, The link() method constructs a URL using $this->ssl->site?->server_id, which will result in a malformed URL with double slashes like /servers//sites/{id}/domains if the site relationship is null. Add a null check for $this->ssl->site before building the URL in the link() method. If the site relationship is null, return an empty string or a fallback URL instead of attempting to construct the URL with missing server_id data.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/Jobs/SSL/CheckSslExpiryJob.php`:
- Around line 56-58: The catch block for Throwable in CheckSslExpiryJob is
silently swallowing exceptions by returning without any logging, which prevents
visibility of failures. Replace the silent return with proper error logging that
captures the actual exception details before returning, so that provider and
service errors are visible and can be debugged effectively.
---
Outside diff comments:
In `@app/Jobs/SSL/CheckSslExpiryJob.php`:
- Around line 16-18: The CheckSslExpiryJob class is missing the UniqueQueue
trait which is required by coding guidelines. Add the UniqueQueue trait to the
class alongside the existing Queueable trait. Additionally, wrap any job work
logic inside the handle method (or equivalent entry point) with a call to
$this->run($key, callable) where the key uniquely identifies the job, to ensure
duplicate jobs are prevented via cache locks.
In `@app/Notifications/SslCertificateExpiring.php`:
- Around line 63-66: The link() method constructs a URL using
$this->ssl->site?->server_id, which will result in a malformed URL with double
slashes like /servers//sites/{id}/domains if the site relationship is null. Add
a null check for $this->ssl->site before building the URL in the link() method.
If the site relationship is null, return an empty string or a fallback URL
instead of attempting to construct the URL with missing server_id data.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6d9dd3e0-5fb0-4868-b8ba-74a4c4af012a
📒 Files selected for processing (3)
app/Console/Commands/RenewSslCertificatesCommand.phpapp/Jobs/SSL/CheckSslExpiryJob.phpapp/Notifications/SslCertificateExpiring.php
Contributor
There was a problem hiding this comment.
Pull request overview
This PR expands Vito’s notification system by adding new notification types (auto-update completion, SSL expiry warnings, backup/webhook/renewal failures) and modernizing email rendering/branding through custom Markdown mail templates.
Changes:
- Added new notifications and triggers for server auto-updates, SSL expiry checks, backup failures, webhook deploy failures, and SSL renewal failures.
- Updated multiple existing notifications with improved wording, localization, and success/error levels.
- Introduced a custom Laravel Markdown mail theme (CSS + header/message templates) and added tests covering key notification email behaviors.
Reviewed changes
Copilot reviewed 35 out of 39 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/Unit/Notifications/NotificationMailTest.php | Adds unit coverage to assert email rendering, branding, levels, and action URLs across notifications/mailables. |
| tests/Feature/ServerTest.php | Adds feature coverage for OS upgrade marker parsing and auto-update notification behavior. |
| tests/Feature/CheckSslExpiriesCommandTest.php | Adds feature coverage ensuring SSL expiry notifications trigger once and re-arm after renewal. |
| resources/views/vendor/mail/html/themes/default.css | Adds a custom HTML email theme stylesheet for consistent branding. |
| resources/views/vendor/mail/html/message.blade.php | Overrides Laravel’s HTML message template for the new branding/layout. |
| resources/views/vendor/mail/html/header.blade.php | Overrides the mail header to use a branded logo. |
| resources/views/ssh/os/upgrade.blade.php | Enhances OS upgrade script to emit upgrade/reboot markers for parsing. |
| resources/views/emails/project-invitation.blade.php | Updates invitation email content/structure to match improved template styling and subcopy usage. |
| database/migrations/2026_06_17_074137_add_expiry_notified_at_to_ssls_table.php | Adds expiry_notified_at to support one-time SSL expiry warnings. |
| config/mail.php | Adjusts default “from” address/name to better defaults for branded email. |
| app/SSH/OS/OS.php | Changes upgrade() to return parsed upgrade results (upgraded count, reboot required). |
| app/Notifications/WebhookDeploymentFailed.php | Adds a new notification for webhook-triggered deployment failures. |
| app/Notifications/SslRenewalFailed.php | Adds a new notification for wildcard SSL renewal failures. |
| app/Notifications/SslCertificateExpiring.php | Adds a new notification for expiring/expired SSL certificates. |
| app/Notifications/SourceControlDisconnected.php | Improves email formatting and severity (error) and adds a management CTA. |
| app/Notifications/SiteInstallationSucceed.php | Improves copy and sets success level + clearer CTA. |
| app/Notifications/SiteInstallationFailed.php | Improves copy, sets error level, and links to the site for retry. |
| app/Notifications/ServerUpdateFailed.php | Improves copy and sets error level + clearer CTA. |
| app/Notifications/ServerInstallationSucceed.php | Improves copy, sets success level, and links directly to the server. |
| app/Notifications/ServerInstallationStarted.php | Improves wording and CTA for installation-in-progress emails. |
| app/Notifications/ServerInstallationFailed.php | Improves wording and sets error level + clearer CTA. |
| app/Notifications/ServerDisconnected.php | Improves wording, sets error level, and adds CTA. |
| app/Notifications/ServerConnected.php | Improves wording, sets success level, and adds CTA. |
| app/Notifications/ServerAutoUpdateCompleted.php | Adds a new notification describing auto-update results and next steps. |
| app/Notifications/FailedToDeleteServerFromProvider.php | Improves copy and sets error level for provider deletion failures. |
| app/Notifications/FailedToDeleteBackupFileFromProvider.php | Improves copy and sets error level for provider backup file deletion failures. |
| app/Notifications/DeploymentCompleted.php | Enhances deployment completion emails with commit summary/author and status-based level. |
| app/Notifications/BackupFailed.php | Adds a new notification for backup job failure. |
| app/NotificationChannels/Email.php | Updates email channel connection test email to use MailMessage rendering and the new template. |
| app/Models/Ssl.php | Adds expiry_notified_at casting + PHPDoc to support the new expiry notification logic. |
| app/Jobs/SSL/CreateLetsEncryptWildcardSslJob.php | Adds renewal context and sends renewal-failure notifications when appropriate. |
| app/Jobs/SSL/CheckSslExpiryJob.php | Adds expiry warning logic, notification throttling via expiry_notified_at, and eager loads needed relations. |
| app/Jobs/Site/TriggerDeployFromWebhookJob.php | Sends a failure notification when a webhook-triggered deploy cannot start. |
| app/Jobs/Server/UpdateJob.php | Captures OS upgrade results and optionally notifies on meaningful auto-update changes. |
| app/Jobs/Backup/RunJob.php | Sends a failure notification when the backup job ultimately fails. |
| app/Console/Commands/RunServerAutoUpdatesCommand.php | Enables notifications for scheduled auto-updates. |
| app/Console/Commands/RenewSslCertificatesCommand.php | Switches to lazy iteration and marks renewal jobs to enable renewal-failure notifications. |
| app/Actions/Server/Update.php | Adds a notify flag and passes it through to the update job. |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Jobs/SSL/CheckSslExpiryJob.php (1)
17-26:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftEnforce the job queue contract required for
app/Jobs/**.This job currently misses the mandated
UniqueQueuelock pattern (use UniqueQueue+$this->run($key, callable)) and afailed(Exception $e)handler. That leaves duplicate runs and inconsistent failure handling on the table.As per coding guidelines: “Jobs implement
ShouldQueuewith BOTHQueueableandUniqueQueuetraits; wrap work in$this->run($key, callable); implementfailed(Exception $e)for status updates.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Jobs/SSL/CheckSslExpiryJob.php` around lines 17 - 26, The CheckSslExpiryJob class is missing the required UniqueQueue trait and pattern enforcement. Add the UniqueQueue trait alongside the existing Queueable trait in the class declaration, wrap the existing logic in the handle() method using $this->run($key, callable) to enforce unique job processing and prevent duplicates, and implement a failed(Exception $e) method to handle job failures with appropriate error logging and status updates according to the coding guidelines for jobs in the app/Jobs directory.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@app/Jobs/SSL/CheckSslExpiryJob.php`:
- Around line 17-26: The CheckSslExpiryJob class is missing the required
UniqueQueue trait and pattern enforcement. Add the UniqueQueue trait alongside
the existing Queueable trait in the class declaration, wrap the existing logic
in the handle() method using $this->run($key, callable) to enforce unique job
processing and prevent duplicates, and implement a failed(Exception $e) method
to handle job failures with appropriate error logging and status updates
according to the coding guidelines for jobs in the app/Jobs directory.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 53160301-cdde-4b21-b259-045663e783a8
📒 Files selected for processing (2)
app/Jobs/SSL/CheckSslExpiryJob.phpresources/views/ssh/os/upgrade.blade.php
saeedvaziry
approved these changes
Jun 18, 2026
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
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.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Tests