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)
|
Warning Rate limit exceeded@HarlequinSin has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 7 minutes and 11 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds SRV-record support: DB migration for Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/Admin
participant Filament as Filament UI
participant Subdomain as Subdomain Model
participant CloudflareSvc as CloudflareService
participant CloudflareAPI as Cloudflare API
participant Notification as Filament Notification
User->>Filament: Submit create/update (includes srv_record toggle)
Filament->>Subdomain: save() → (saving → saved)
Subdomain->>Subdomain: derive record_type / prepare payload
Subdomain->>CloudflareSvc: getZoneId(domain_name)
CloudflareSvc->>CloudflareAPI: GET /zones
CloudflareAPI-->>CloudflareSvc: zone data / error
CloudflareSvc-->>Subdomain: zoneId (or null)
alt zoneId present and validation ok
Subdomain->>CloudflareSvc: upsertDnsRecord(zoneId, name, type, target, recordId?, port?)
CloudflareSvc->>CloudflareAPI: POST/PUT DNS record
CloudflareAPI-->>CloudflareSvc: success / error
CloudflareSvc-->>Subdomain: {success:true/false, id...}
Subdomain->>Notification: Emit success or failure notification
else failure
Subdomain->>Notification: Emit zone/validation failure notification
end
Notification-->>User: Display result
rect rgb(245,250,255)
Note over Subdomain,CloudflareSvc: Delete flow
User->>Filament: Delete subdomain
Filament->>Subdomain: deleting hook
Subdomain->>CloudflareSvc: deleteDnsRecord(zoneId, recordId)
CloudflareSvc->>CloudflareAPI: DELETE record
CloudflareAPI-->>CloudflareSvc: success / error
CloudflareSvc-->>Subdomain: {success:true/false}
Subdomain->>Notification: Emit dns_deleted or dns_action_failed
Notification-->>User: Display outcome
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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
Show resolved
Hide resolved
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
subdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.php (1)
123-127: Implement dynamic disable logic for the SRV record toggle.The TODO remains unaddressed. The form toggle is hard-coded to
disabled(false), creating an inconsistency with the table's ToggleColumn (line 85) which properly disables whensrv_targetis missing. While the model includes runtime validation (lines 110-123 in Subdomain.php), implementing frontend validation here would provide better UX by preventing invalid form submissions.💡 Suggested approach
Make the toggle reactive and conditionally disabled based on the selected domain's
srv_target:- Toggle::make('srv_record') - ->label(trans('subdomains::strings.srv_record')) - ->helperText(trans('subdomains::strings.srv_record_help')) - ->disabled(false) // TODO: Dynamically disable if domain missing srv_target - ->default(false), + Toggle::make('srv_record') + ->label(trans('subdomains::strings.srv_record')) + ->helperText(trans('subdomains::strings.srv_record_help')) + ->disabled(fn (callable $get) => { + $domainId = $get('domain_id'); + if (!$domainId) { + return true; + } + $domain = CloudflareDomain::find($domainId); + return !$domain || empty($domain->srv_target); + }) + ->reactive() + ->default(false),Also ensure the domain select field is reactive:
Select::make('domain_id') ->label(trans_choice('subdomains::strings.domain', 1)) ->disabledOn('edit') ->required() ->relationship('domain', 'name') ->preload() ->default(fn () => CloudflareDomain::first()?->id ?? null) - ->searchable(), + ->searchable() + ->reactive(),
🧹 Nitpick comments (1)
subdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.php (1)
121-121: Consider whether defaulting to the first domain is appropriate.Using
CloudflareDomain::first()as the default works but is somewhat arbitrary when multiple domains exist. Consider whether a more intentional default (e.g., most recently created, alphabetically first by name, or no default requiring explicit selection) would better serve your use case.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
subdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.phpsubdomains/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 (2)
subdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.php (2)
subdomains/src/Models/Subdomain.php (2)
Subdomain(24-213)domain(59-62)subdomains/src/Models/CloudflareDomain.php (1)
CloudflareDomain(16-54)
subdomains/src/Models/Subdomain.php (1)
subdomains/src/Services/CloudflareService.php (3)
CloudflareService(9-162)upsertDnsRecord(43-120)deleteDnsRecord(138-161)
🔇 Additional comments (7)
subdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.php (1)
82-102: LGTM! Well-implemented SRV record UI controls with proper validation.The table changes are well-designed:
- The ToggleColumn correctly disables when
srv_targetis missing from the domain- Contextual tooltips guide users appropriately
- Suppressing default notifications via
successNotification(null)is correct since the model now sends custom notifications inupsertOnCloudflareanddeleteOnCloudflaresubdomains/src/Models/Subdomain.php (6)
6-7: LGTM! Clean integration of dependencies and virtual field setup.The new imports and
srv_recordconfiguration are correctly implemented:
- CloudflareService, Notification, and Log imports support the new integration
- Adding
srv_recordto$fillableenables mass assignment from the form- Boolean cast ensures type consistency with the Toggle component
Also applies to: 12-12, 32-37
43-52: LGTM! Excellent virtual field pattern implementation.The lifecycle hook changes effectively handle the
srv_record↔record_typetransformation:
- The
savinghook intercepts the virtualsrv_recordboolean, derives the appropriate DNS record type, and removes it from attributes before persistence- The
savedhook ensures Cloudflare state is synchronized after database changes- This pattern cleanly separates UI concerns (boolean toggle) from storage concerns (record type string)
74-90: LGTM! Well-designed accessor and mutator with proper null safety.The attribute handling is robust:
getSrvRecordAttribute()correctly maps'SRV'record type to booleantruefor the UIsetRecordType()properly derives the DNS record type based on server allocation IP version- Line 84 includes appropriate null checks for
$this->serverand$this->server->allocationbefore callingis_ipv6()
92-108: LGTM! Proper validation and user feedback for missing zone configuration.The method initialization correctly:
- Instantiates CloudflareService via dependency injection
- Validates that the domain has a Cloudflare zone ID before proceeding
- Logs warnings and sends user notifications when prerequisites are missing
- Returns early to prevent invalid API calls
150-187: LGTM! Robust A/AAAA record handling with comprehensive validation.The A/AAAA record path is well-implemented:
- Line 151 includes proper null checks for
$this->serverbefore accessingallocation(addressing previous review feedback)- IP validation correctly excludes unspecified addresses (
0.0.0.0and::)- Success and failure branches provide clear user feedback via notifications
- Error handling includes detailed logging with result data for troubleshooting
189-212: LGTM! Delete flow correctly fixed from previous review.The deletion logic now properly handles success and failure cases:
- Only one notification is sent (success OR failure), resolving the critical issue from the previous review
- Success and failure paths are clearly separated with proper conditional logic
- Detailed error information is included in failure notifications for debugging
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
subdomains/src/Models/Subdomain.php (2)
79-90: Consider eager-loading server relationship when deriving record type.The
setRecordTypemethod accesses$this->server->allocation->ipto determine IPv6 vs IPv4. When called from thesavinghook (line 45), theserverrelationship may not be loaded, which would cause the method to default to'A'even if the server actually has an IPv6 allocation.In typical Filament form usage this is likely fine since relationships are loaded, but for programmatic Subdomain creation this could produce incorrect record types.
💡 Potential solutions
Option 1: Add a defensive check and fallback:
public function setRecordType($value): void { if ($value) { $this->attributes['record_type'] = 'SRV'; } else { - if ($this->server && $this->server->allocation && is_ipv6($this->server->allocation->ip)) { + if ($this->server && $this->server->allocation) { + $this->attributes['record_type'] = is_ipv6($this->server->allocation->ip) ? 'AAAA' : 'A'; + } elseif (!isset($this->attributes['record_type'])) { + // Fallback: default to 'A' only if record_type not already set $this->attributes['record_type'] = 'AAAA'; - } else { - $this->attributes['record_type'] = 'A'; } } }Option 2: Eager-load server in the saving hook before calling setRecordType:
static::saving(function (self $model) { if (array_key_exists('srv_record', $model->attributes)) { + $model->loadMissing('server.allocation'); $model->setRecordType($model->attributes['srv_record']); unset($model->attributes['srv_record']); } });
109-160: LGTM: Comprehensive SRV record handling.The SRV flow is well-structured:
- Validates port and srv_target with clear error notifications
- Uses
updateQuietly()to prevent lifecycle recursion- Provides success/failure feedback to users
💡 Minor cleanup: Redundant null coalescing
Line 137: The expression
$this->cloudflare_id ?? nullcan be simplified to just$this->cloudflare_idsince the?? nullis redundant when the left side is already nullable.-$result = $service->upsertDnsRecord($zoneId, $this->name, 'SRV', $this->domain->srv_target, $this->cloudflare_id ?? null, $port); +$result = $service->upsertDnsRecord($zoneId, $this->name, 'SRV', $this->domain->srv_target, $this->cloudflare_id, $port);The same applies to line 175 in the A/AAAA branch.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
subdomains/lang/en/strings.phpsubdomains/plugin.jsonsubdomains/src/Models/Subdomain.php
✅ Files skipped from review due to trivial changes (1)
- subdomains/plugin.json
🧰 Additional context used
🧬 Code graph analysis (1)
subdomains/src/Models/Subdomain.php (2)
subdomains/src/Services/CloudflareService.php (3)
CloudflareService(9-162)upsertDnsRecord(43-120)deleteDnsRecord(138-161)subdomains/src/Providers/SubdomainsPluginProvider.php (1)
boot(23-36)
🔇 Additional comments (9)
subdomains/lang/en/strings.php (3)
17-21: LGTM!The SRV record and SRV target strings are clear and well-written. The help text provides good context, and the example hostname in line 21 is helpful for users.
23-25: LGTM!The error message clearly communicates why the SRV record cannot be enabled and is grammatically correct.
30-40: LGTM!The notification messages are well-structured and clear. The use of placeholders (:domain, :subdomain, :errors, etc.) is consistent, and the pairing of titles with descriptive messages provides good context for users.
Also applies to: 44-46, 49-68
subdomains/src/Models/Subdomain.php (6)
6-7: LGTM: Clean import additions.The new imports for
CloudflareService,Notification, andLogare all properly utilized throughout the refactored code.Also applies to: 12-12
26-37: LGTM: Proper fillable and cast configuration.The
srv_recordboolean attribute is correctly configured for Filament form handling and gets translated torecord_typeduring the saving lifecycle.
39-57: LGTM: Lifecycle hooks properly structured.The refactored lifecycle (
saving→saved→deleted) correctly:
- Translates the transient
srv_recordattribute before persistence- Syncs with Cloudflare after the database save completes
- Uses
updateQuietly()downstream to prevent infinite recursion
92-107: LGTM: Proper validation and early exit.The zone ID validation follows best practices:
- Logs the issue for debugging
- Notifies the user with clear context
- Returns early to prevent further processing
162-198: LGTM: Robust A/AAAA record handling.The A/AAAA flow properly:
- Validates server allocation and rejects placeholder IPs (
0.0.0.0,::)- Uses null-safe access chains (addressing previous review concerns)
- Maintains consistent success/failure notification patterns with the SRV path
201-224: LGTM: Delete flow correctly implemented.The delete operation properly:
- Guards against missing identifiers before attempting deletion
- Uses correct
if-elsebranching so only one notification is sent (previous critical issue resolved ✅)- Uses
->success()severity for successful deletes (previous typo and severity issues resolved ✅)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
subdomains/lang/en/strings.php (2)
38-63: Standardize notification title capitalization.The notification titles show inconsistent capitalization after the "Cloudflare:" prefix. Some use title case (e.g., "Missing Zone ID", "Missing SRV Port", "Missing IP") while others use sentence case (e.g., "Record updated", "SRV upsert failed", "Record deleted").
For consistency and professionalism, standardize all titles to use the same capitalization style throughout.
🔎 Suggested fix (standardizing to title case)
- 'cloudflare_record_updated_title' => 'Cloudflare: Record updated', + 'cloudflare_record_updated_title' => 'Cloudflare: Record Updated', - 'cloudflare_srv_upsert_failed_title' => 'Cloudflare: SRV upsert failed', + 'cloudflare_srv_upsert_failed_title' => 'Cloudflare: SRV Record Update Failed', - 'cloudflare_upsert_failed_title' => 'Cloudflare: Upsert failed', + 'cloudflare_upsert_failed_title' => 'Cloudflare: Record Update Failed', - 'cloudflare_delete_success_title' => 'Cloudflare: Record deleted', + 'cloudflare_delete_success_title' => 'Cloudflare: Record Deleted', - 'cloudflare_delete_failed_title' => 'Cloudflare: Delete failed', + 'cloudflare_delete_failed_title' => 'Cloudflare: Delete Failed',
50-51: Consider replacing "upsert" with more user-friendly terminology.The term "upsert" is technical jargon that may not be immediately clear to all users. For user-facing notifications, consider using more accessible language such as "create/update" or "save."
🔎 Suggested alternatives
- 'cloudflare_srv_upsert_failed_title' => 'Cloudflare: SRV upsert failed', - 'cloudflare_srv_upsert_failed' => 'Failed to upsert SRV record for :subdomain. See logs for details. Errors: :errors', + 'cloudflare_srv_upsert_failed_title' => 'Cloudflare: SRV Record Update Failed', + 'cloudflare_srv_upsert_failed' => 'Failed to create or update SRV record for :subdomain. See logs for details. Errors: :errors', - 'cloudflare_upsert_failed_title' => 'Cloudflare: Upsert failed', - 'cloudflare_upsert_failed' => 'Failed to upsert record for :subdomain. See logs for details. Errors: :errors', + 'cloudflare_upsert_failed_title' => 'Cloudflare: Record Update Failed', + 'cloudflare_upsert_failed' => 'Failed to create or update record for :subdomain. See logs for details. Errors: :errors',Also applies to: 56-57
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
subdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.php (2)
121-121: Consider if the first domain is always the intended default.The default value is set to the first
CloudflareDomainin the database. If multiple domains exist, this might not always be the most appropriate choice (depending on creation order).Consider whether a more explicit default selection strategy would be beneficial (e.g., a
is_defaultflag on domains, or sorting by a priority field).
123-127: LGTM with optional optimization suggestion.The dynamic disable logic correctly implements the TODO from previous reviews. The toggle is appropriately disabled when the selected domain lacks an
srv_target.Optional optimization: The
CloudflareDomain::find($get('domain_id'))call on line 126 may execute repeatedly during form state changes. Consider making the field reactive and caching the domain lookup if performance becomes a concern:🔎 Potential optimization
Toggle::make('srv_record') ->label(trans('subdomains::strings.srv_record')) ->helperText(trans('subdomains::strings.srv_record_help')) + ->reactive() ->disabled(fn (callable $get, ?Subdomain $record = null) => (empty($get('domain_id')) || empty(CloudflareDomain::find($get('domain_id'))?->srv_target))) ->default(false),Alternatively, you could add a hidden computed field to store the domain's srv_target and reference it in the disabled callback.
Note: The PHPMD warning about the unused
$recordparameter is a false positive. This parameter is part of Filament's callback signature and may be required by the framework even when not explicitly used in the logic.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
subdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.php
🧰 Additional context used
🪛 PHPMD (2.15.0)
subdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.php
126-126: Avoid unused parameters such as '$record'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (4)
subdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.php (4)
17-17: LGTM!The new imports for
ToggleandToggleColumnare correctly added and used in the table and form definitions.Also applies to: 22-22
82-85: LGTM!The ToggleColumn implementation correctly disables the toggle when the domain lacks an
srv_targetand provides helpful tooltips to guide the user. This aligns well with the PR's objective of adding SRV record support.
88-91: LGTM!Suppressing default success notifications with
->successNotification(null)aligns with the PR objective of providing more specific feedback about whether DNS records were actually updated in Cloudflare. This suggests custom notification logic is implemented in the model's lifecycle hooks.Also applies to: 102-102
123-127: Backend validation for SRV record constraints is properly implemented.The Subdomain model's
upsertOnCloudflare()method (lines 125–135) validates thatsrv_targetis not empty before upserting to Cloudflare. If missing, it logs a warning, sends a notification, and returns early, preventing the API call from executing. The CloudflareService also validates this condition. This defensive validation prevents invalid submissions—whether from the UI or bypassed via API—from creating records with missing SRV targets.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
subdomains/lang/en/strings.php (1)
2-2: Fix the whitespace on the blank line to resolve the Pint violation.The pipeline failure indicates that line 2 contains whitespace characters (spaces or tabs) on what should be a completely blank line. Laravel Pint requires blank lines to be entirely empty.
🔎 Proposed fix
Remove any spaces or tabs from line 2 so it is completely empty:
<?php - + return [(Ensure line 2 has zero characters, not even spaces or tabs.)
🧹 Nitpick comments (2)
subdomains/src/Models/Subdomain.php (2)
79-90: Add type hint for the$valueparameter.The method logic correctly determines
record_typebased on the boolean value and IP version. However, the$valueparameter lacks a type hint.🔎 Proposed improvement
- public function setRecordType($value): void + public function setRecordType(bool $value): void {
92-199: Consider decomposing this method for improved maintainability.The method is comprehensive and handles all validation and notification flows correctly. However, at 107 lines, it could benefit from extraction into smaller methods (e.g.,
validateAndUpsertSrvRecord(),validateAndUpsertIpRecord()) to improve readability and testability.💡 Example decomposition approach
protected function upsertOnCloudflare(): void { $service = app(CloudflareService::class); $zoneId = $this->validateZoneId(); if (!$zoneId) { return; } if ($this->record_type === 'SRV') { $this->upsertSrvRecord($service, $zoneId); } else { $this->upsertIpRecord($service, $zoneId); } } protected function validateZoneId(): ?string { // lines 96-107 } protected function upsertSrvRecord(CloudflareService $service, string $zoneId): void { // lines 110-160 } protected function upsertIpRecord(CloudflareService $service, string $zoneId): void { // lines 162-198 }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
subdomains/lang/en/strings.phpsubdomains/src/Models/Subdomain.php
🧰 Additional context used
🧬 Code graph analysis (1)
subdomains/src/Models/Subdomain.php (3)
subdomains/src/Services/CloudflareService.php (3)
CloudflareService(9-162)upsertDnsRecord(43-120)deleteDnsRecord(138-161)subdomains/src/Models/CloudflareDomain.php (1)
boot(24-48)subdomains/src/Providers/SubdomainsPluginProvider.php (1)
boot(23-36)
🪛 GitHub Actions: Pint
subdomains/lang/en/strings.php
[error] 1-1: Laravel Pint: no_whitespace_in_blank_line
🔇 Additional comments (4)
subdomains/src/Models/Subdomain.php (4)
32-37: LGTM: Virtual attribute handling is well-designed.The
srv_recordattribute is correctly configured as fillable and cast to boolean, enabling clean form handling. Thesavinghook (lines 43-48) properly removes it before database persistence after deriving the actualrecord_type.
43-52: LGTM: Lifecycle hooks follow Laravel best practices.The
savinghook correctly transforms the virtualsrv_recordattribute into the persistentrecord_type, and thesavedhook ensures Cloudflare synchronization happens after database persistence. Usingarray_key_existsproperly detects when the attribute was explicitly set.
74-77: LGTM: Accessor provides clean boolean interface.The
getSrvRecordAttribute()accessor correctly derives the booleansrv_recordvalue from the storedrecord_type, completing the virtual attribute pattern.
201-224: LGTM: Delete handling is clean and correct.The method properly validates prerequisites, delegates to
CloudflareService, and sends appropriate success or failure notifications. The past issue with duplicate notifications has been resolved.
|
So, if I understand correctly you can assign a SRV target to a cloudflare domain? And that will be used to create a minecraft SRV record? The two things that bother me here:
|
|
Yes 100% on both points at this time:
|
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
✏️ Tip: You can customize this high-level summary in your review settings.