fix subdomain edit and improved error handling #90
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds a preflight DNS-record search with unified name construction and earlier payload assembly in the Subdomain model; updates Cloudflare PATCH to use Changes
Sequence DiagramsequenceDiagram
participant Client
participant Service
participant Model
participant CF
participant DB
rect rgba(100,200,150,0.5)
Note over Client,Service: Create flow
Client->>Service: createOrUpdate(NULL, data)
Service->>DB: create Subdomain (mark new)
Service->>Model: build payload (includes searchName)
Model->>CF: GET search by name/type
alt conflict found
CF-->>Model: searchResponse (conflict)
Model-->>Service: throw exception
Service->>DB: delete Subdomain (created here)
else no conflict
Model->>CF: POST create DNS record
CF-->>Model: return cloudflare_id
Model->>DB: update cloudflare_id
Service->>DB: refresh Subdomain
end
end
rect rgba(100,150,200,0.5)
Note over Client,Service: Update flow
Client->>Service: createOrUpdate(existingModel, data)
Service->>DB: update Subdomain (mark not new)
Service->>Model: build payload (includes searchName)
Model->>CF: GET search by name/type
alt conflict found
CF-->>Model: searchResponse (conflict)
Model-->>Service: throw exception
Service-->>DB: keep existing Subdomain
else no conflict
Model->>CF: PATCH dns_records/{cloudflare_id} with payload
CF-->>Model: confirm update
Service->>DB: refresh Subdomain
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
✏️ Tip: You can disable this entire section by setting 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: 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)
146-150: Silent failure when Cloudflare operation fails without error details.Similar to the search response handling, if
$response['success']isfalsebut$response['errors']is empty or falsy, the method returns without throwing an exception. The caller assumes success, but the Cloudflare operation may have failed, leading to local/Cloudflare state mismatch.🛠️ Suggested fix
if (!$response['success']) { if ($response['errors'] && count($response['errors']) > 0) { throw new Exception($response['errors'][0]['message']); } + throw new Exception('Cloudflare DNS operation failed'); }
🤖 Fix all issues with AI agents
In `@subdomains/src/Models/Subdomain.php`:
- Around line 114-128: The search response handling in Subdomain (the
$searchResponse block) silently continues when $searchResponse['success'] is
false but $searchResponse['errors'] is empty; update the failure branch inside
the if (!$searchResponse['success']) block to throw a generic Exception (e.g.,
'Cloudflare search failed' or include available status info) when no specific
error message exists so the code does not proceed on an ambiguous failed search;
ensure you still throw the specific errors when $searchResponse['errors'] is
non-empty and reference the same $searchResponse variable and surrounding
conditional logic in Subdomain.php.
🧹 Nitpick comments (1)
subdomains/src/Services/SubdomainService.php (1)
17-17: Consider using standard PHP variable naming convention.
$NewSubdomainuses PascalCase. PHP convention is$camelCasefor variables. Consider renaming to$newSubdomainor$isNewSubdomainfor consistency.♻️ Suggested change
- $NewSubdomain = false; + $isNewSubdomain = false;And update references on lines 21 and 31 accordingly.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
subdomains/src/Models/Subdomain.phpsubdomains/src/Services/SubdomainService.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/Services/SubdomainService.php (1)
subdomains/src/Models/Subdomain.php (2)
Subdomain(24-160)upsertOnCloudflare(59-151)
🔇 Additional comments (5)
subdomains/src/Services/SubdomainService.php (1)
19-34: LGTM! Good fix for the edit workflow error handling.The logic correctly:
- Distinguishes between creating a new subdomain vs updating an existing one
- Calls
refresh()to ensure the model is in sync before the Cloudflare operation- Only deletes the subdomain on failure if it was newly created, preserving existing records as intended by the PR objectives
This addresses issue
#87where editing a subdomain would delete the existing DNS record on failure.subdomains/src/Models/Subdomain.php (4)
78-91: LGTM! Good extraction of$searchNamefor SRV records.The
$searchNamevariable cleanly captures the constructed name used for both the payload and the preflight search, improving code clarity.
96-106: LGTM! Consistent pattern for non-SRV records.The parallel structure with SRV records improves readability and ensures both branches construct
$searchNamebefore the unified search logic.
121-127: Good preflight validation for duplicate detection.The loop correctly checks if any existing record has a different ID than the current
cloudflare_id, which properly handles both:
- Create scenario:
cloudflare_idis null, any existing record is a conflict- Update scenario:
cloudflare_idis set, only records with different IDs are conflictsThis addresses the core issue where editing was incorrectly treating the existing record as a conflict.
130-132: LGTM! Correct PATCH URL construction.The PATCH request now properly uses the full path
dns_records/{$this->cloudflare_id}to update the existing record.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
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)
78-105: Search must use fully-qualified DNS record names (FQDN) to avoid missing existing records and creating duplicates.The Cloudflare API
namefilter expects a complete DNS record name including the zone name (e.g.,www.example.com), not just the subdomain part. Currently, the code searches using relative names only ($this->name), which will not match existing records in Cloudflare. This allows duplicate records to be created instead of updating the existing ones.For SRV records, append
$this->domain->nameto the search query:- $searchName = "{$srvServiceType->value}.{$this->name}"; + $searchName = "{$srvServiceType->value}.{$this->name}.{$this->domain->name}";For non-SRV records, do the same:
- $searchName = $this->name; + $searchName = "{$this->name}.{$this->domain->name}";
🤖 Fix all issues with AI agents
In `@subdomains/src/Models/Subdomain.php`:
- Around line 108-118: The code assumes ->json() returns an array and directly
indexes ['success'], which can cause TypeError on null/invalid JSON; update the
Cloudflare API calls (e.g., where Http::cloudflare()->get(...) is used in
Subdomain:: (the $searchResponse block) and similar calls in this file and
CloudflareDomain.php) to first store the HTTP response object, validate the HTTP
status (e.g., successful()/status() check) and ensure the decoded JSON is an
array and contains the 'success' key before indexing it; if the status is not OK
or the JSON is missing/invalid, throw a clear Exception or handle the error
instead of accessing ['success'] directly.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
subdomains/src/Models/Subdomain.phpsubdomains/src/Services/SubdomainService.php
🚧 Files skipped from review as they are similar to previous changes (1)
- subdomains/src/Services/SubdomainService.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)
131-134: PATCH endpoint update looks correct.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
Not making changes to $searchName as this is already handled in $searchResponse. Do you want me to make the json guard change proposed by coderabbit? |
Boy132
left a comment
Boy132
left a comment
There was a problem hiding this comment.
Thank you for your contribution!
2 participants
Fixes: #87
Fixed the Edit workflow so that editing a subdomain updates Cloudflare correctly and does not delete the record on failure (some improved error handling).
Previous:
When you edit a subdomain name, the plugin calls Cloudflare in a way that incorrectly treats the existing record as a conflict. Throws exception, and deletes record.
Now:
When you edit a subdomain name, the plugin queries Cloudflare records by name and type, then checks each record ID, if record has a different ID, an exception is thrown, otherwise update the record.
Additional error handling:
If New Subdomain is created for the first time and there is an error, delete the record (as previous).
If Subdomain is being edited and there is an error, keep the record, as to not remove a current working record.
Summary by CodeRabbit
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.