SRV records support with endpoint notifications #67
Conversation
…n SubdomainResource
…record target or port
…delete functionality
…ifications on success or failure
…ecords created outside plugin)
📝 WalkthroughWalkthroughThis pull request introduces SRV (Service Record) support for subdomains, moving Changes
Sequence DiagramsequenceDiagram
actor User as User (Filament)
participant Filament as Filament Form
participant Model as Subdomain Model
participant Validation as Validation Logic
participant CloudflareAPI as CloudflareService
participant DB as Database
participant Notification as Notification System
User->>Filament: Submit Subdomain (srv_record=true)
Filament->>Model: Create/Update with srv_record
Model->>Validation: Validate SRV prerequisites<br/>(srv_target, port, service type)
alt Prerequisites Missing
Validation-->>Notification: Emit danger notification
Notification-->>User: Display error (missing port/target)
else Prerequisites Valid
Validation->>CloudflareAPI: Call upsertDnsRecord<br/>(zone, SRV payload)
CloudflareAPI->>CloudflareAPI: Build SRV record payload<br/>(service, protocol, port, target)
alt Cloudflare Success
CloudflareAPI->>DB: Return DNS record ID
DB->>Model: Update cloudflare_id
Model->>Notification: Emit success notification
Notification-->>User: Display "Record updated"
else Cloudflare Failure
CloudflareAPI-->>Notification: Log error + emit danger notification
Notification-->>User: Display error details
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes The changes introduce a new feature spanning multiple layers (models, services, UI components, database), with significant logic density in CloudflareService (validation, conditional SRV vs. A/AAAA routing, API integration) and Subdomain model (lifecycle hooks, notification dispatch). Heterogeneous file changes across different patterns (enums, actions, migrations, localization) require separate reasoning for each cohort. Database schema changes with data cleanup operations add careful review requirements. Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (3)
subdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.php (1)
120-124: Consider using a relationship or callback optimization.The
CloudflareDomain::find($get('domain_id'))query runs on every form render/interaction. While functional, this could be optimized by leveraging the relationship already defined on the model, or by using->live()on thedomain_idfield to control when the lookup occurs.🔎 Suggested optimization
Toggle::make('srv_record') ->label(trans('subdomains::strings.srv_record')) ->helperText(trans('subdomains::strings.srv_record_help')) - ->disabled(fn (callable $get) => !$get('domain_id') || empty(CloudflareDomain::find($get('domain_id'))->srv_target ?? null)) + ->disabled(fn (callable $get, ?Subdomain $record) => + !$get('domain_id') || + empty($record?->domain?->srv_target ?? CloudflareDomain::find($get('domain_id'))?->srv_target) + ) ->default(false),subdomains/src/Services/CloudflareService.php (1)
69-82: Consider making the SRV service prefix configurable or explicitly document the Minecraft-specific scope.The
_minecraft._tcpprefix is hardcoded at line 70 with no mechanism to customize it. Given that the plugin documentation describes it as supporting "game servers" generically, this architectural decision limits the plugin's reusability for other services that use SRV records (e.g., SIP, XMPP, other games). Either explicitly scope the plugin to Minecraft-only in documentation and naming, or refactor to accept the service prefix as a parameter or configuration option.subdomains/src/Models/Subdomain.php (1)
10-10: Remove unused import.The
Httpfacade is imported but never used in this file. The code now usesCloudflareServicefor all HTTP interactions.🔎 Proposed fix
-use Illuminate\Support\Facades\Http;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
subdomains/database/migrations/004_add_srv_target_to_cloudflare_domains.phpsubdomains/lang/en/strings.phpsubdomains/src/Filament/Admin/Resources/CloudflareDomains/CloudflareDomainResource.phpsubdomains/src/Filament/Admin/Resources/CloudflareDomains/Pages/ManageCloudflareDomains.phpsubdomains/src/Filament/Admin/Resources/Servers/RelationManagers/SubdomainRelationManager.phpsubdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.phpsubdomains/src/Models/CloudflareDomain.phpsubdomains/src/Models/Subdomain.phpsubdomains/src/Services/CloudflareService.php
🧰 Additional context used
🧬 Code graph analysis (2)
subdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.php (2)
subdomains/src/Models/Subdomain.php (2)
Subdomain(25-218)domain(58-61)subdomains/src/Models/CloudflareDomain.php (1)
CloudflareDomain(17-55)
subdomains/src/Models/CloudflareDomain.php (1)
subdomains/src/Services/CloudflareService.php (2)
CloudflareService(9-161)getZoneId(11-39)
🪛 GitHub Actions: Pint
subdomains/src/Filament/Admin/Resources/Servers/RelationManagers/SubdomainRelationManager.php
[error] 1-1: pint --test: no_unused_imports violation detected.
subdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.php
[error] 1-1: pint --test: no_unused_imports violation detected.
subdomains/src/Services/CloudflareService.php
[error] 1-1: pint --test: class_attributes_separation violation detected.
subdomains/src/Models/CloudflareDomain.php
[error] 1-1: pint --test: no_unused_imports violation detected.
[error] 1-1: pint --test: ordered_imports violation detected.
subdomains/src/Models/Subdomain.php
[error] 1-1: pint --test: class_attributes_separation violation detected.
[error] 1-1: pint --test: statement_separation violation detected.
🔇 Additional comments (12)
subdomains/src/Filament/Admin/Resources/CloudflareDomains/Pages/ManageCloudflareDomains.php (1)
16-18: LGTM — notification delegation to model layer.Disabling
successNotification(null)here avoids duplicate notifications since theCloudflareDomainmodel now emits its own success/failure notifications during thecreatedevent. This is a reasonable approach for consolidated feedback.subdomains/src/Filament/Admin/Resources/CloudflareDomains/CloudflareDomainResource.php (1)
56-57: LGTM — SRV target field implementation.The new
srv_targetfield is well-integrated with appropriate labels, helper text, and a solid hostname validation regex that follows RFC conventions for domain names.Also applies to: 82-86
subdomains/src/Filament/Admin/Resources/Servers/RelationManagers/SubdomainRelationManager.php (1)
87-90: LGTM — SRV record toggle implementation.The toggle replaces the previous hidden
record_typefield with a user-controllable option, which aligns with the PR's goal of enabling SRV record support.subdomains/lang/en/strings.php (1)
17-37: LGTM — comprehensive language strings.The new translation keys provide clear, consistent messaging for SRV record functionality and Cloudflare notifications.
subdomains/database/migrations/004_add_srv_target_to_cloudflare_domains.php (1)
7-21: LGTM — clean migration.The migration correctly adds a nullable
srv_targetcolumn and provides proper rollback support.subdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.php (1)
83-86: LGTM — ToggleColumn with appropriate guards.The toggle is properly disabled when the domain is missing or lacks an
srv_target, with contextual tooltips to inform users why it's unavailable.subdomains/src/Services/CloudflareService.php (2)
41-120: LGTM — well-structured upsert logic.The method properly validates inputs, handles both SRV and A/AAAA record types, and provides comprehensive error handling with structured response arrays. The separation between create (POST) and update (PUT) based on
recordIdpresence follows Cloudflare API conventions.
11-39: LGTM — solid service layer implementation.The
getZoneIdanddeleteDnsRecordmethods have appropriate input validation, exception handling, and logging. The structured return format enables consistent error handling in calling code.Also applies to: 139-160
subdomains/src/Models/Subdomain.php (4)
27-38: LGTM!The
srv_recordfield is properly added to fillable and cast as boolean.
44-51: Consider whether Cloudflare failures should halt the save.The
upsertOnCloudflare()method returns early on failure but the model save proceeds regardless. This could leave the database record and Cloudflare DNS out of sync. If this is intentional (e.g., to allow offline editing), consider documenting this behavior; otherwise, consider throwing an exception to abort the save on Cloudflare failures.
73-76: LGTM!Clean accessor implementation for the virtual
srv_recordattribute.
78-89: LGTM!Proper null-safe checking for server allocation and correct IP version detection logic.
subdomains/src/Filament/Admin/Resources/Servers/RelationManagers/SubdomainRelationManager.php
Outdated
Show resolved
Hide resolved
subdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.php
Outdated
Show resolved
Hide resolved
- Fixed potential potential NPE - Corrected some notifications
- Removed inbuilt notifications in favor of service notifications
- Fixed Style/unused class issues
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
subdomains/src/Models/CloudflareDomain.php (1)
28-47: Use translation keys for notification messages.Lines 34 and 39 contain hardcoded English strings. The past review comment indicated that translation keys
zone_request_failedandzone_request_succeededare already defined instrings.php. Please use these keys instead of hardcoded strings for consistency and i18n support.subdomains/src/Services/CloudflareService.php (1)
136-138: Fix formatting to resolve pipeline failure.The pipeline is failing due to
braces_positionandblank_lineviolations. Based on past review comments, there may be spacing issues between methods. Ensure consistent spacing (single blank line) between method declarations per the project's PHP-CS-Fixer rules.subdomains/src/Models/Subdomain.php (1)
190-211: Critical: Fix delete notification logic.Multiple issues with the delete notifications:
- Success notification uses wrong severity: Lines 198-202 send a
danger()notification on success when it should besuccess().- Typo: Line 200 has "successed" which should be "succeeded".
- Unconditional failure notification: Lines 205-209 always execute, sending a failure notification even after a successful delete. This results in users seeing both success and failure notifications.
Add an
elsebranch or early return after the success case.🔎 Proposed fix
if (!empty($result['success'])) { + Log::info('Deleted Cloudflare record for Subdomain ID ' . $this->id, ['result' => $result]); + Notification::make() - ->danger() - ->title('Cloudflare: Delete successed') + ->success() + ->title('Cloudflare: Delete succeeded') ->body('Successfully deleted Cloudflare record for ' . $this->name . '.' . ($this->domain->name ?? 'unknown') . '.') ->send(); + } else { + Log::warning('Failed to delete Cloudflare record for Subdomain ID ' . $this->id, ['result' => $result]); + + Notification::make() + ->danger() + ->title('Cloudflare: Delete failed') + ->body('Failed to delete Cloudflare record for ' . $this->name . '.' . ($this->domain->name ?? 'unknown') . '. See logs for details. Errors: ' . json_encode($result['errors'] ?? $result['body'] ?? [])) + ->send(); } - - Notification::make() - ->danger() - ->title('Cloudflare: Delete failed') - ->body('Failed to delete Cloudflare record for ' . $this->name . '.' . ($this->domain->name ?? 'unknown') . '. See logs for details. Errors: ' . json_encode($result['errors'] ?? $result['body'] ?? [])) - ->send();
🧹 Nitpick comments (1)
subdomains/src/Services/CloudflareService.php (1)
69-82: Hardcoded Minecraft SRV naming limits flexibility.Line 70 hardcodes the SRV record name as
_minecraft._tcp.%s, which restricts this implementation to Minecraft servers only. If the plugin is intended solely for Minecraft, this should be documented. Otherwise, consider making the service and protocol configurable.🔎 Potential refactor to support configurable service/protocol
public function upsertDnsRecord( string $zoneId, string $name, string $recordType, string $target, ?string $recordId = null, ?int $port = null, + ?string $service = '_minecraft', + ?string $protocol = '_tcp', ): array { // ... if ($recordType === 'SRV') { // ... $payload = [ - 'name' => sprintf('_minecraft._tcp.%s', $name), + 'name' => sprintf('%s.%s.%s', $service, $protocol, $name), // ... ]; } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
subdomains/src/Filament/Admin/Resources/Servers/RelationManagers/SubdomainRelationManager.phpsubdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.phpsubdomains/src/Models/CloudflareDomain.phpsubdomains/src/Models/Subdomain.phpsubdomains/src/Services/CloudflareService.php
🚧 Files skipped from review as they are similar to previous changes (2)
- subdomains/src/Filament/Admin/Resources/Servers/RelationManagers/SubdomainRelationManager.php
- subdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.php
🧰 Additional context used
🧬 Code graph analysis (2)
subdomains/src/Models/CloudflareDomain.php (1)
subdomains/src/Services/CloudflareService.php (2)
CloudflareService(9-160)getZoneId(11-39)
subdomains/src/Models/Subdomain.php (2)
subdomains/src/Services/CloudflareService.php (3)
CloudflareService(9-160)upsertDnsRecord(41-120)deleteDnsRecord(138-159)subdomains/src/Models/CloudflareDomain.php (1)
boot(24-48)
🪛 GitHub Actions: Pint
subdomains/src/Services/CloudflareService.php
[error] 1-1: pint --test failed: braces_position, blank_line issues detected in CloudflareService.php. Command failed with exit code 1.
subdomains/src/Models/Subdomain.php
[error] 1-1: pint --test failed: no_unused_imports, ordered_imports issues detected in Subdomain.php. Command failed with exit code 1.
🔇 Additional comments (7)
subdomains/src/Services/CloudflareService.php (3)
11-39: LGTM!The
getZoneIdmethod properly validates inputs, handles exceptions, and returns the first matching zone ID. Error logging is appropriate.
94-120: LGTM!The upsert logic correctly differentiates between create (POST) and update (PUT) based on the presence of
$recordId. Error handling and logging are appropriate.
138-159: LGTM!The
deleteDnsRecordmethod properly validates inputs, handles the DELETE request, and includes appropriate error handling and logging.subdomains/src/Models/Subdomain.php (4)
44-58: LGTM!The lifecycle hooks are well-structured. Using
savingto deriverecord_typefromsrv_recordbefore persistence, thensavedto trigger the Cloudflare upsert, ensures the model state is consistent before making external API calls.
75-91: LGTM!The accessor/mutator pattern for
srv_recordis clean. ThesetRecordTypemethod properly determines the record type (SRV, A, or AAAA) based on the boolean flag and IP version, with appropriate null checks.
110-149: LGTM!The SRV record upsert logic properly validates the port, calls the service with the domain's
srv_target, and usesupdateQuietlyon line 130 to avoid triggering lifecycle hooks recursively. Notifications provide clear feedback on success and failure.
151-187: LGTM!The A/AAAA record upsert logic includes proper null checks for
$this->serverand$this->server->allocation(addressing the past review comment), validates IP addresses, and provides appropriate notifications. The use ofupdateQuietlyprevents recursive hook invocation.
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 (1)
subdomains/src/Models/Subdomain.php (1)
1-211: Fix Pint violation: Remove whitespace from blank lines.The pipeline failure indicates the
no_whitespace_in_blank_linerule is triggered. Ensure all blank lines throughout the file contain no spaces or tabs.Run Pint locally to identify and auto-fix the whitespace:
./vendor/bin/pint subdomains/src/Models/Subdomain.php
🧹 Nitpick comments (1)
subdomains/src/Models/Subdomain.php (1)
92-187: Optional: Consider extracting duplicate success-handling logic.The cloudflare_id update (lines 128-130, 166-168) and success notification (lines 132-136, 170-174) are duplicated across the SRV and A/AAAA paths. Extracting a small helper method would reduce repetition.
🔎 Example refactor to reduce duplication
private function handleUpsertSuccess(string $recordId): void { if ($this->cloudflare_id !== $recordId) { $this->updateQuietly(['cloudflare_id' => $recordId]); } Notification::make() ->success() ->title('Cloudflare: Record updated') ->body('Successfully updated ' . $this->name . '.' . ($this->domain->name ?? 'unknown') . ' to ' . $this->record_type) ->send(); }Then replace lines 128-136 and 166-174 with:
$this->handleUpsertSuccess($result['id']);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
subdomains/src/Models/Subdomain.phpsubdomains/src/Services/CloudflareService.php
🚧 Files skipped from review as they are similar to previous changes (1)
- subdomains/src/Services/CloudflareService.php
🧰 Additional context used
🧬 Code graph analysis (1)
subdomains/src/Models/Subdomain.php (2)
subdomains/src/Services/CloudflareService.php (3)
CloudflareService(9-160)upsertDnsRecord(41-120)deleteDnsRecord(138-159)subdomains/src/Providers/SubdomainsPluginProvider.php (1)
boot(23-36)
🪛 GitHub Actions: Pint
subdomains/src/Models/Subdomain.php
[error] 1-1: pint --test failed with exit code 1. Rule 'no_whitespace_in_blank_line' triggered.
🔇 Additional comments (4)
subdomains/src/Models/Subdomain.php (4)
6-7: LGTM: Imports support new CloudflareService integration.The new imports (CloudflareService, Notification, Log) are properly used throughout the upsert and delete flows.
Also applies to: 12-12
26-37: LGTM: srv_record field properly configured.The fillable array and boolean cast correctly support the new SRV record toggle feature.
39-57: LGTM: Lifecycle hooks properly refactored.The consolidation from
created/updatedtosaving/savedis cleaner. The srv_record virtual attribute is correctly processed and unset before persistence, and Cloudflare operations are properly triggered.
74-90: LGTM: srv_record accessor/mutator logic is sound.The accessor provides a clean boolean interface, and the mutator correctly derives record_type with proper null-safe checks for the server allocation.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
subdomains/src/Services/CloudflareService.php (1)
122-136: Refine success detection to match Cloudflare API semantics.Line 127's logic
$body['success'] === true || count($body['result']) > 0treatssuccessandresultas interchangeable indicators, which doesn't align with the Cloudflare API's response structure wheresuccessis an explicit boolean flag andresultis the data payload.Issues:
- The OR condition returns
success=trueif onlyresulthas data, even when$body['success']isfalsecount()on a non-arrayresultwill error- Line 131 extracts
idfromresultwithout verifyingresultexists or is an arrayThe stricter pattern at line 30 (
!empty($body['result']) && count($body['result']) > 0) is more defensive.🔎 Proposed fix
protected function parseCloudflareHttpResponse(Response $response): array { $status = $response->status(); $body = $response->json() ?? []; - $success = $response->successful() && ($body['success'] === true || count($body['result']) > 0); + $success = $response->successful() && $body['success'] === true; return [ 'success' => $success, - 'id' => $body['result']['id'] ?? null, + 'id' => is_array($body['result'] ?? null) ? ($body['result']['id'] ?? null) : null, 'errors' => $body['errors'] ?? [], 'status' => $status, 'body' => $body, ]; }
🧹 Nitpick comments (1)
subdomains/src/Services/CloudflareService.php (1)
69-82: Consider making the SRV naming convention configurable.Line 70 hardcodes the Minecraft-specific SRV naming convention (
_minecraft._tcp.%s). While this works for the current use case, it limits the service's reusability for other SRV record types (e.g.,_http._tcp,_xmpp._tcp).Consider adding optional parameters for the service and protocol, or documenting that this service is Minecraft-specific.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
subdomains/lang/en/strings.phpsubdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.phpsubdomains/src/Models/CloudflareDomain.phpsubdomains/src/Models/Subdomain.phpsubdomains/src/Services/CloudflareService.phpsubdomains/src/SubdomainsPlugin.php
🚧 Files skipped from review as they are similar to previous changes (1)
- subdomains/src/Models/CloudflareDomain.php
🧰 Additional context used
🧬 Code graph analysis (2)
subdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.php (2)
subdomains/src/Models/Subdomain.php (2)
Subdomain(24-212)domain(59-62)subdomains/src/Models/CloudflareDomain.php (1)
CloudflareDomain(16-54)
subdomains/src/Models/Subdomain.php (2)
subdomains/src/Services/CloudflareService.php (3)
CloudflareService(9-160)upsertDnsRecord(41-120)deleteDnsRecord(138-159)subdomains/src/Providers/SubdomainsPluginProvider.php (1)
boot(23-36)
🪛 GitHub Actions: Pint
subdomains/src/Services/CloudflareService.php
[warning] 1-1: Pint lint issue: braces_position, blank_line...
subdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.php
[error] 89-89: Parse error: syntax error, unexpected token '->', expecting ']' on line 89.
subdomains/src/Models/Subdomain.php
[warning] 1-1: Pint lint issue: blank_line_before_statement, no_whitespace...
🔇 Additional comments (9)
subdomains/src/SubdomainsPlugin.php (1)
50-50: LGTM! Proper internationalization.The notification title is now properly localized using the translation key from the strings file.
subdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.php (1)
82-85: LGTM! Well-implemented ToggleColumn with proper validation.The SRV record toggle includes helpful tooltips and correctly disables when the domain is missing an SRV target, preventing invalid configurations.
subdomains/src/Services/CloudflareService.php (2)
11-39: LGTM! Robust zone ID fetching with proper error handling.The method includes appropriate validation, exception handling, and logging for both request failures and API errors.
138-159: LGTM! Consistent error handling pattern.The delete method follows the same robust error handling pattern as the other methods, with proper validation, logging, and exception handling.
subdomains/lang/en/strings.php (1)
17-65: LGTM! Comprehensive and clear translation keys.The new translation keys provide clear, user-friendly messages for SRV record functionality and Cloudflare operations. The notification messages include helpful context with placeholder variables.
subdomains/src/Models/Subdomain.php (4)
39-57: LGTM! Cleaner lifecycle hook implementation.The refactored hooks consolidate create/update logic into
saving/savedevents, which is more maintainable than separatecreated/updatedhandlers. Thesrv_recordattribute conversion before persistence is well-placed.
74-90: LGTM! Well-implemented record type logic.The accessor and setter properly handle the SRV/A/AAAA record type determination with appropriate null safety checks for the server relationship.
92-187: LGTM! Comprehensive upsert implementation with excellent error handling.The method properly:
- Validates zone ID availability
- Handles SRV and A/AAAA record types with appropriate validation
- Provides clear user notifications for both success and failure cases
- Updates the
cloudflare_idwhen it changes- Uses the translation keys for user-facing messages
The delegation of SRV target validation to
CloudflareServiceis appropriate.
189-211: LGTM! Correct notification flow with early return.The delete method properly handles success and failure cases with the
returnstatement on line 202 ensuring only one notification is sent. The notification types and translation keys are appropriate for delete operations.
subdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.php
Outdated
Show resolved
Hide resolved
subdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.php
Outdated
Show resolved
Hide resolved
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
subdomains/src/Models/Subdomain.php (2)
118-130: Add null-safe operator for server access.Line 123 accesses
$this->server->allocation?->ipwithout a null-safe operator on$this->server. If the server relationship is not loaded or is null, this will cause a fatal error.🔎 Proposed fix
- $ip = $this->server->allocation?->ip; + $ip = $this->server?->allocation?->ip;
136-137: Add null check for domain relationship.Lines 136-137 access
$this->domain->cloudflare_idand$this->domain->namewithout first verifying that$this->domainis not null. If the domain relationship is not loaded, this will cause a fatal error.🔎 Proposed fix
protected function upsertOnCloudflare(): bool { $registrar = app(CloudflareService::class); + if (!$this->domain) { + Log::warning('Domain relationship not loaded for subdomain', ['subdomain_id' => $this->id]); + Notification::make() + ->danger() + ->title(trans('subdomains::strings.notifications.cloudflare_upsert_failed_title')) + ->body(trans('subdomains::strings.notifications.cloudflare_upsert_failed', ['subdomain' => $this->name ?? 'unknown', 'errors' => 'Domain relation is null'])) + ->send(); + + return false; + } + $zoneId = $this->domain->cloudflare_id; $domainName = $this->domain->name;
🧹 Nitpick comments (4)
subdomains/src/Models/Subdomain.php (4)
6-20: Consider keeping cloudflare_id nullable in docblock.Line 20 declares
cloudflare_idas a non-nullablestring, but this field is actually nullable until the Cloudflare record is successfully created. The code handles this withempty()checks throughout (e.g., lines 271, 277). Keeping the docblock as?stringwould better reflect the actual behavior and prevent static analysis false positives.🔎 Proposed fix
- * @property string $cloudflare_id + * @property ?string $cloudflare_id
139-139: Use null-safe operator for consistency.Line 139 checks
empty($this->server) || empty($this->server->node). While technically safe due to short-circuit evaluation, using the null-safe operator (empty($this->server?->node)) would be more consistent with the coding style used elsewhere in this file and prevents potential static analysis warnings.🔎 Proposed fix
- if (empty($this->server) || empty($this->server->node)) { + if (empty($this->server) || empty($this->server?->node)) {
173-230: SRV record logic is well-structured.The SRV path correctly:
- Validates port availability (lines 174-184)
- Determines service record type from server egg tags (lines 186-196)
- Validates node srv_target (lines 198-207)
- Calls CloudflareService with correct parameters (line 209)
- Uses
updateQuietlyfor existing models and direct assignment for new models (lines 212-218)While the guards at lines 139 and 150 protect against null server/allocation, consider adding null-safe operators defensively on lines 174 and 198 for consistency:
$port = $this->server?->allocation?->port; // and if (empty($this->server?->node?->srv_target)) {
232-266: A/AAAA record logic is correct.The A/AAAA path correctly:
- Validates IP address and excludes invalid IPs (lines 233-243)
- Calls CloudflareService with correct parameters, passing
nullfor SRV-specific fields (line 245)- Handles cloudflare_id updates consistently with the SRV path (lines 248-254)
Consider adding a null-safe operator on line 233 for defensive programming:
$ip = $this->server?->allocation?->ip;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
subdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.phpsubdomains/src/Models/Subdomain.php
🧰 Additional context used
🧬 Code graph analysis (2)
subdomains/src/Models/Subdomain.php (3)
subdomains/src/Services/CloudflareService.php (3)
CloudflareService(10-215)upsertDnsRecord(80-166)deleteDnsRecord(171-195)subdomains/src/Providers/SubdomainsPluginProvider.php (1)
boot(27-40)subdomains/src/Enums/ServiceRecordType.php (2)
getLabel(13-16)fromServer(30-35)
subdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.php (2)
subdomains/src/Models/Subdomain.php (2)
server(103-106)Subdomain(26-304)subdomains/src/Models/CloudflareDomain.php (1)
CloudflareDomain(16-71)
🔇 Additional comments (7)
subdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.php (4)
17-17: LGTM!The Toggle import is correctly added to support the new srv_record form field.
86-90: LGTM!The record_type column correctly uses null-safe operators to prevent NPEs when accessing the server/node relationship chain.
93-107: LGTM!The actions correctly suppress default success notifications by chaining
->successNotification(null), allowing the model lifecycle hooks to handle notifications instead.
129-133: LGTM!The srv_record toggle correctly uses null-safe operators and provides appropriate helper text and disabled state based on srv_target availability.
subdomains/src/Models/Subdomain.php (3)
28-43: LGTM!The srv_record field is properly configured as a virtual attribute with fillable, cast, and append declarations to support the new SRV record functionality.
49-96: LGTM!The lifecycle hooks are well-structured:
- Using
creating/updating/deleting(instead of past-tense events) allows aborting operations by returningfalsewhen Cloudflare operations fail.- Line 51 manually sets the server relation during creation, which is necessary since the relationship doesn't exist yet—this is a valid workaround.
- User-facing notifications are appropriately placed in the lifecycle hooks.
288-303: Delete logic handles edge cases well.The deletion flow correctly:
- Treats 404 responses as successful (line 292), acknowledging that the record may already be deleted
- Provides detailed error information in failure notifications (lines 296-300)
- Returns boolean to allow lifecycle hooks to abort deletion if needed
subdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.php
Outdated
Show resolved
Hide resolved
subdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.php
Outdated
Show resolved
Hide resolved
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
subdomains/src/Models/Subdomain.php (1)
136-170: Add null check for$this->domainbefore accessing properties.Lines 136-137 access
$this->domain->cloudflare_idand$this->domain->namewithout first verifying the domain relationship is loaded. If the relationship is null or not loaded, this will throw a fatal error before the validation at line 161 can execute.🔎 Proposed fix
protected function upsertOnCloudflare(): bool { $registrar = app(CloudflareService::class); + if (!$this->domain) { + Log::warning('Domain relationship not loaded for subdomain', ['subdomain_id' => $this->id]); + Notification::make() + ->danger() + ->title(trans('subdomains::strings.notifications.cloudflare_upsert_failed_title')) + ->body(trans('subdomains::strings.notifications.cloudflare_upsert_failed', ['subdomain' => $this->name ?? 'unknown', 'errors' => 'Domain relationship not loaded'])) + ->send(); + + return false; + } + $zoneId = $this->domain->cloudflare_id; $domainName = $this->domain->name;
🧹 Nitpick comments (2)
subdomains/src/Models/Subdomain.php (1)
232-266: LGTM with minor suggestion: Add defensive null-safe operator.The A/AAAA record path correctly validates IP, calls the Cloudflare service, and updates the cloudflare_id. While line 139 ensures
$this->serverexists before reaching this code, adding a null-safe operator on line 233 ($this->server?->allocation?->ip) would provide additional defensive protection against future refactoring that might change the validation flow.🔎 Optional defensive improvement
// A/AAAA - $ip = $this->server->allocation?->ip; // @phpstan-ignore nullsafe.neverNull + $ip = $this->server?->allocation?->ip; // @phpstan-ignore nullsafe.neverNull if (empty($ip) || $ip === '0.0.0.0' || $ip === '::') {subdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.php (1)
112-135: LGTM with minor suggestion: Add defensive null-safe operator.The form correctly displays subdomain name with domain suffix, domain selection, and SRV toggle with appropriate helper text and disabled state based on srv_target availability. All null-safe operators are properly used to handle missing relationships.
While
Filament::getTenant()always returns a Server in this resource context, adding a null-safe operator (getTenant()?->node?->srv_target) on lines 131 and 133 would provide additional defensive protection against future refactoring or edge cases.🔎 Optional defensive improvement
Toggle::make('srv_record') ->label(trans('subdomains::strings.srv_record')) - ->helperText(fn () => Filament::getTenant()->node?->srv_target ? trans('subdomains::strings.srv_record_help') : trans('subdomains::strings.srv_target_missing')) // @phpstan-ignore property.notFound + ->helperText(fn () => Filament::getTenant()?->node?->srv_target ? trans('subdomains::strings.srv_record_help') : trans('subdomains::strings.srv_target_missing')) // @phpstan-ignore property.notFound ->reactive() - ->disabled(fn () => !Filament::getTenant()->node?->srv_target), // @phpstan-ignore property.notFound + ->disabled(fn () => !Filament::getTenant()?->node?->srv_target), // @phpstan-ignore property.notFound
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
subdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.phpsubdomains/src/Models/Subdomain.php
🧰 Additional context used
🧬 Code graph analysis (2)
subdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.php (2)
subdomains/src/Models/Subdomain.php (2)
server(103-106)Subdomain(26-304)subdomains/src/Models/CloudflareDomain.php (1)
CloudflareDomain(16-71)
subdomains/src/Models/Subdomain.php (4)
subdomains/src/Services/CloudflareService.php (3)
CloudflareService(10-215)upsertDnsRecord(80-166)deleteDnsRecord(171-195)subdomains/src/Models/CloudflareDomain.php (1)
boot(23-47)subdomains/src/Providers/SubdomainsPluginProvider.php (1)
boot(27-40)subdomains/src/Enums/ServiceRecordType.php (2)
getLabel(13-16)fromServer(30-35)
🔇 Additional comments (3)
subdomains/src/Models/Subdomain.php (1)
268-303: LGTM!The delete logic correctly handles missing cloudflare_id with an early return, validates the domain relationship using null-safe operators, calls the Cloudflare service, and treats both successful deletes and 404s (already deleted) as success. The error handling and notifications are appropriate.
subdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.php (2)
35-46: LGTM!The
canAccess()logic correctly validates that the server has a valid, non-wildcard IP and that at least one CloudflareDomain exists before allowing subdomain management. The null-safe operator properly handles missing allocation relationships.
79-109: LGTM!The table correctly displays subdomain labels and record types, with appropriate warning indicators when SRV records lack a target. The null-safe operators prevent NPEs when server or node relationships are missing. Suppressing default success notifications (lines 94, 96, 107) is appropriate since the model's lifecycle hooks send custom notifications, avoiding duplicates.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
subdomains/src/Models/Subdomain.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-04T11:13:37.620Z
Learnt from: HarlequinSin
Repo: pelican-dev/plugins PR: 67
File: subdomains/src/Models/Subdomain.php:49-65
Timestamp: 2026-01-04T11:13:37.620Z
Learning: In Subdomain model creation (subdomains/src/Models/Subdomain.php), use setRelation('server', Filament::getTenant()) instead of load() for the unsaved model during creation. This is intentional to populate the relationship in Filament context when the model hasn't been saved yet.
Applied to files:
subdomains/src/Models/Subdomain.php
🔇 Additional comments (1)
subdomains/src/Models/Subdomain.php (1)
211-211: Method signature verification complete. TheCloudflareService::upsertDnsRecordmethod signature accepts 8 parameters as defined, and both calls at lines 211 and 247 pass the correct number and types of arguments. No signature mismatch detected.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
subdomains/src/Models/Subdomain.php (2)
48-52: Consider optimizing for Filament context per past decision.The current code uses
loadMissinguniversally, which works but queries the database even in Filament contexts whereFilament::getTenant()already provides the Server instance. Based on learnings, you can optimize by usingsetRelationwhen in Filament context:if (Filament::hasTenant()) { $model->setRelation('server', Filament::getTenant()); if (!$model->relationLoaded('server.allocation')) { $model->server->load('allocation'); } } elseif ($model->server_id) { $model->loadMissing('server.allocation'); }This avoids redundant queries in Filament while still supporting non-Filament contexts.
Based on learnings, using
setRelationin Filament contexts is the intended pattern for efficiency.
109-112: MakegetLabel()defensive for error-path usage.While not a changed line,
getLabel()is called throughout the new notification code (lines 62, 77, 92, 145, 156, 168, etc.). Line 111 accesses$this->domain->namewithout null-safe handling. If the domain relationship isn't loaded when an error occurs, the error notification itself will fail with an NPE.🔎 Proposed fix
public function getLabel(): string|Htmlable|null { - return $this->name . '.' . $this->domain->name; + return $this->name . '.' . ($this->domain?->name ?? 'unknown'); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
subdomains/src/Models/Subdomain.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-04T11:13:37.620Z
Learnt from: HarlequinSin
Repo: pelican-dev/plugins PR: 67
File: subdomains/src/Models/Subdomain.php:49-65
Timestamp: 2026-01-04T11:13:37.620Z
Learning: In Subdomain model creation (subdomains/src/Models/Subdomain.php), use setRelation('server', Filament::getTenant()) instead of load() for the unsaved model during creation. This is intentional to populate the relationship in Filament context when the model hasn't been saved yet.
Applied to files:
subdomains/src/Models/Subdomain.php
🧬 Code graph analysis (1)
subdomains/src/Models/Subdomain.php (3)
subdomains/src/Services/CloudflareService.php (3)
CloudflareService(10-215)upsertDnsRecord(80-166)deleteDnsRecord(171-195)subdomains/src/Models/CloudflareDomain.php (1)
boot(23-47)subdomains/src/Enums/ServiceRecordType.php (2)
getLabel(13-16)fromServer(30-35)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
subdomains/src/Models/Subdomain.php (4)
48-52: Consider loading additional relationships upfront.The code loads
server.allocationbutupsertOnCloudflare()also accesses$this->domain(lines 137-138, 162, 167, 200, 204) and$this->server->node(lines 140, 175, 187, 199, 210). These will trigger lazy loading, causing additional queries.🔎 Proposed optimization
static::creating(function (self $model) { // Relation does not exist yet, so we need to set it manually. if ($model->server_id) { - $model->loadMissing('server.allocation'); + $model->loadMissing(['server.allocation', 'server.node', 'domain']); } $registrarUpdated = $model->upsertOnCloudflare();
77-78: Use null-safe operator for consistency with the rest of the file.Although the notification only executes after
upsertOnCloudflare()returns true (which validates domain), accessing$model->domain->namewithout null-safe is inconsistent with the defensive patterns used in lines 137-138. Ifdomainis null, the expression throws before??evaluates.🔎 Proposed fix
- ->body(trans('subdomains::strings.notifications.cloudflare_record_updated', ['subdomain' => $model->name . '.' . ($model->domain->name ?? 'unknown'), 'record_type' => $model->record_type])) + ->body(trans('subdomains::strings.notifications.cloudflare_record_updated', ['subdomain' => $model->name . '.' . ($model->domain?->name ?? 'unknown'), 'record_type' => $model->record_type]))
92-93: Same null-safe operator issue in deleting hook.🔎 Proposed fix
- ->body(trans('subdomains::strings.notifications.cloudflare_record_deleted', ['subdomain' => $model->name . '.' . ($model->domain->name ?? 'unknown')])) + ->body(trans('subdomains::strings.notifications.cloudflare_record_deleted', ['subdomain' => $model->name . '.' . ($model->domain?->name ?? 'unknown')]))
109-112: Add null-safe operator togetLabel()for defensive coding.This public method (implementing
HasLabel) can be called in various Filament contexts. If thedomainrelationship isn't loaded, this will throw an NPE.🔎 Proposed fix
public function getLabel(): string|Htmlable|null { - return $this->name . '.' . $this->domain->name; + return $this->name . '.' . ($this->domain?->name ?? 'unknown'); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
subdomains/src/Models/Subdomain.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-04T11:13:37.620Z
Learnt from: HarlequinSin
Repo: pelican-dev/plugins PR: 67
File: subdomains/src/Models/Subdomain.php:49-65
Timestamp: 2026-01-04T11:13:37.620Z
Learning: In Subdomain model creation (subdomains/src/Models/Subdomain.php), use setRelation('server', Filament::getTenant()) instead of load() for the unsaved model during creation. This is intentional to populate the relationship in Filament context when the model hasn't been saved yet.
Applied to files:
subdomains/src/Models/Subdomain.php
🧬 Code graph analysis (1)
subdomains/src/Models/Subdomain.php (3)
subdomains/src/Services/CloudflareService.php (3)
CloudflareService(10-215)upsertDnsRecord(80-166)deleteDnsRecord(171-195)subdomains/src/Providers/SubdomainsPluginProvider.php (1)
boot(27-40)subdomains/src/Enums/ServiceRecordType.php (2)
getLabel(13-16)fromServer(30-35)
🔇 Additional comments (6)
subdomains/src/Models/Subdomain.php (6)
33-42: LGTM on the virtual attribute setup.The
srv_recordproperty is correctly added to fillable (triggering the mutator on mass assignment), cast as boolean, and appended for serialization. This cleanly abstracts the underlyingrecord_typefield.
114-131: LGTM on accessor/mutator implementation.The
getSrvRecordAttributecorrectly derives the boolean fromrecord_type, andsetSrvRecordAttributeproperly handles the reverse mapping with correct IPv6 detection usingfilter_var()with appropriate flags.
133-171: Well-structured validation sequence.The method properly validates all prerequisites (server, node, allocation, domain zone) before proceeding, with appropriate logging and user notifications for each failure case. The use of null-safe operators at lines 137-138 is correct.
173-231: SRV record path is comprehensive.The SRV flow correctly validates port, service record type (derived from server egg tags), and node srv_target before making the API call. The cloudflare_id persistence logic handles both new and existing models appropriately.
233-267: A/AAAA record path is consistent with SRV path.Good handling of edge cases (0.0.0.0, ::) for invalid IPs, and cloudflare_id update logic mirrors the SRV path as previously recommended.
269-304: Clean delete implementation with good edge case handling.The method correctly:
- Returns success when there's nothing to delete (no cloudflare_id)
- Validates domain zone exists before attempting deletion
- Treats 404 responses as success (record already gone)
- Only sends failure notification on actual failures
|
Boy132
left a comment
Boy132
left a comment
There was a problem hiding this comment.
Thanks for the changes!
There seems to be a bunch of null and nullable related mishaps.
subdomains/database/migrations/004_add_srv_target_to_cloudflare_domains.php
Outdated
Show resolved
Hide resolved
subdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.php
Outdated
Show resolved
Hide resolved
subdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.php
Outdated
Show resolved
Hide resolved
subdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
subdomains/README.md (1)
34-36: Optional: Vary sentence structure to improve readability.The troubleshooting list has three consecutive items starting with "Missing". Consider rephrasing for variety, though this is a minor stylistic point.
Suggested alternative phrasing
- **Missing IP for A/AAAA:** ensure the server has an allocation with a valid IP address. - **Missing SRV port:** ensure the server allocation includes a port. -- **Missing SRV target:** configure the node's SRV target. +- **SRV target not configured:** set the node's SRV target. - **Unsupported SRV service:** add the appropriate tag to the Egg (e.g., `minecraft`).Based on static analysis hints.
subdomains/src/Models/Subdomain.php (1)
125-125: Add null-safe operator for defensive coding.While the allocation should be loaded in normal flows,
setSrvRecordAttribute()is a public mutator that could be called directly. Using the null-safe operator on$this->serverprovides extra safety.🔎 Suggested fix
- $ip = $this->server->allocation->ip; + $ip = $this->server?->allocation?->ip; if (!empty($ip) && filter_var($ip, FILTER_VALIDATE_IP, FILTER_FLAG_IPV6)) { $this->attributes['record_type'] = 'AAAA'; } else { $this->attributes['record_type'] = 'A'; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
subdomains/README.mdsubdomains/database/migrations/004_add_srv_target_to_nodes.phpsubdomains/src/Enums/ServiceRecordType.phpsubdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.phpsubdomains/src/Models/Subdomain.php
🚧 Files skipped from review as they are similar to previous changes (1)
- subdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-04T11:13:37.620Z
Learnt from: HarlequinSin
Repo: pelican-dev/plugins PR: 67
File: subdomains/src/Models/Subdomain.php:49-65
Timestamp: 2026-01-04T11:13:37.620Z
Learning: In Subdomain model creation (subdomains/src/Models/Subdomain.php), use setRelation('server', Filament::getTenant()) instead of load() for the unsaved model during creation. This is intentional to populate the relationship in Filament context when the model hasn't been saved yet.
Applied to files:
subdomains/src/Models/Subdomain.php
🧬 Code graph analysis (2)
subdomains/src/Models/Subdomain.php (2)
subdomains/src/Services/CloudflareService.php (3)
CloudflareService(10-215)upsertDnsRecord(80-166)deleteDnsRecord(171-195)subdomains/src/Enums/ServiceRecordType.php (2)
getLabel(18-21)fromServer(23-28)
subdomains/src/Enums/ServiceRecordType.php (2)
subdomains/src/Models/Subdomain.php (2)
getLabel(110-113)server(105-108)billing/src/Models/Product.php (1)
egg(89-92)
🪛 LanguageTool
subdomains/README.md
[style] ~36-~36: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... server allocation includes a port. - Missing SRV target: configure the node's SRV ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🔇 Additional comments (6)
subdomains/README.md (1)
8-51: Well-structured documentation!The additions clearly explain SRV requirements, supported games, troubleshooting steps, and provide practical examples. This will help users configure the plugin correctly.
subdomains/src/Enums/ServiceRecordType.php (1)
1-55: Clean enum implementation!The ServiceRecordType enum is well-structured with clear case definitions and helpful utility methods. The tag-based mapping from server to service type is elegant and extensible.
subdomains/database/migrations/004_add_srv_target_to_nodes.php (1)
10-44: Migration logic is sound.The migration correctly:
- Cleans up orphaned records before enforcing non-null constraints
- Moves srv_target from cloudflare_domains to nodes
- Provides a reversible down() method for schema changes
Note that data deleted in up() cannot be restored in down(), but this is expected and documented in the code comments.
subdomains/src/Models/Subdomain.php (3)
49-67: Good relationship preloading in creating hook.The creating hook properly loads all necessary relationships (server.allocation, server.node, domain) before calling upsertOnCloudflare(), which prevents null-reference errors during the Cloudflare API operations.
134-256: Well-structured upsertOnCloudflare implementation.The method properly handles both SRV and A/AAAA record types with comprehensive validation:
- Validates all required relationships and data
- Provides clear user notifications for each failure scenario
- Correctly updates cloudflare_id using updateQuietly for existing models
- Logs warnings for troubleshooting
The separation between SRV and IP-based paths is clean and maintainable.
258-293: Solid deleteOnCloudflare implementation.The method handles edge cases well:
- Returns true when no cloudflare_id exists (nothing to delete)
- Treats 404 responses as success (already deleted)
- Provides clear error notifications with context
2 participants
There are some pretty sweeping changes here, but they can be summarized in to the following:
Let me know if there are any issues (honestly never made a PR before and this is my first time with PHP)
Summary by CodeRabbit
Release Notes
New Features
Documentation
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.