-
-
Notifications
You must be signed in to change notification settings - Fork 212
delete-domain route added #267
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@kuntlme is attempting to deploy a commit to the kmkoushik's projects Team on Vercel. A member of the Team first needs to authorize it. |
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a DELETE /v1/domains/{id} endpoint across the stack: OpenAPI docs (apps/docs/api-reference/openapi.json and apps/docs/api-reference/domains/delete-domain.mdx), TypeScript types (packages/sdk/types/schema.d.ts) declaring the delete operation with a 200 response returning the full domain object, a server handler (apps/web/src/server/public-api/api/domains/delete-domain.ts) that validates id, enforces API-key domain access, returns 404 if not found, deletes the domain, and registers the route, and client methods: TypeScript SDK Domains.delete and UseSend.domains exposure, plus Python SDK Domains.delete(domain_id). Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (3)**/*.{js,jsx,ts,tsx}📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{ts,tsx,md}📄 CodeRabbit inference engine (AGENTS.md)
Files:
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/docs/api-reference/openapi.json (1)
637-797
: Consider aligning DELETE response with REST conventions.The DELETE operation returns the full domain object (lines 640-796), which is unconventional. Most DELETE operations return either:
- 204 No Content (most common)
- 200 with a minimal payload like
{success: true, id: number}
For consistency, note that the contact DELETE operation in this same API (lines 1989-2008) returns a simple
{success: boolean}
payload. Returning the full domain object adds unnecessary response size and deviates from the API's own patterns.Consider refactoring to return a minimal success payload:
"responses": { "200": { "description": "Domain deleted successfully", "content": { "application/json": { "schema": { "type": "object", "properties": { - "id": { - "type": "number", - "description": "The ID of the domain", - "example": 1 - }, - "name": { - "type": "string", - "description": "The name of the domain", - "example": "example.com" + "success": { + "type": "boolean" }, - ... (all other domain fields) + "id": { + "type": "number" + } }, "required": [ - "id", - "name", - ... (other fields) + "success", + "id" ] } } } } }Alternatively, if you need to return the deleted domain for audit purposes, keep the current response but add a comment explaining this design decision.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/docs/api-reference/domains/delete-domain.mdx
(1 hunks)apps/docs/api-reference/openapi.json
(1 hunks)apps/web/src/server/public-api/api/domains/delete-domain.ts
(1 hunks)apps/web/src/server/public-api/index.ts
(2 hunks)packages/python-sdk/usesend/domains.py
(1 hunks)packages/sdk/src/domain.ts
(2 hunks)packages/sdk/src/usesend.ts
(4 hunks)packages/sdk/types/schema.d.ts
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Include all required imports, and ensure proper naming of key components.
Files:
packages/sdk/src/usesend.ts
packages/sdk/types/schema.d.ts
packages/sdk/src/domain.ts
apps/web/src/server/public-api/api/domains/delete-domain.ts
apps/web/src/server/public-api/index.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}
: Use TypeScript with 2-space indentation and semicolons (enforced by Prettier)
ESLint must pass with zero warnings using @usesend/eslint-config
Do not use dynamic imports (avoid import() and dynamic loading)
Files:
packages/sdk/src/usesend.ts
packages/sdk/types/schema.d.ts
packages/sdk/src/domain.ts
apps/web/src/server/public-api/api/domains/delete-domain.ts
apps/web/src/server/public-api/index.ts
**/*.{ts,tsx,md}
📄 CodeRabbit inference engine (AGENTS.md)
Format code and docs with Prettier 3
Files:
packages/sdk/src/usesend.ts
packages/sdk/types/schema.d.ts
packages/sdk/src/domain.ts
apps/web/src/server/public-api/api/domains/delete-domain.ts
apps/web/src/server/public-api/index.ts
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/**/*.{ts,tsx}
: In apps/web, use the "/" alias for src imports (e.g., import { x } from "/utils/x")
Prefer using tRPC for API calls unless explicitly instructed otherwise
Files:
apps/web/src/server/public-api/api/domains/delete-domain.ts
apps/web/src/server/public-api/index.ts
🧬 Code graph analysis (5)
packages/python-sdk/usesend/domains.py (2)
packages/python-sdk/usesend/usesend.py (1)
delete
(118-121)packages/python-sdk/usesend/types.py (2)
Domain
(41-60)APIError
(337-339)
packages/sdk/src/usesend.ts (1)
packages/sdk/src/domain.ts (1)
Domains
(48-89)
packages/sdk/src/domain.ts (1)
packages/sdk/types/index.ts (1)
ErrorResponse
(1-4)
apps/web/src/server/public-api/api/domains/delete-domain.ts (3)
apps/web/src/server/public-api/hono.ts (1)
PublicAPIApp
(136-136)apps/web/src/server/public-api/api-error.ts (1)
UnsendApiError
(62-75)apps/web/src/server/db.ts (1)
db
(20-20)
apps/web/src/server/public-api/index.ts (2)
apps/web/src/server/aws/ses.ts (1)
deleteDomain
(144-176)apps/web/src/server/service/domain-service.ts (1)
deleteDomain
(294-316)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (2)
packages/sdk/types/schema.d.ts (1)
1-4
: Auto-generated file; defer review to source OpenAPI spec.This file is auto-generated from the OpenAPI specification. Any concerns about the DELETE operation should be addressed in the source OpenAPI document (apps/docs/api-reference/openapi.json).
apps/docs/api-reference/openapi.json (1)
625-636
: Verify path parameter requirement.The path parameter
id
is marked asrequired: false
(line 632), which is unusual for a DELETE operation that requires a resource identifier. While the parameter is nullable, DELETE operations typically require the ID to be present in the path.Ensure this aligns with the intended API behavior. If the ID must be provided, consider marking it as
required: true
even if it's nullable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
5 issues found across 8 files
Prompt for AI agents (all 5 issues)
Understand the root cause of the following 5 issues and fix them.
<file name="apps/web/src/server/public-api/api/domains/delete-domain.ts">
<violation number="1" location="apps/web/src/server/public-api/api/domains/delete-domain.ts:32">
Wrap the 403 response media type in a `content` property so the OpenAPI schema is valid.</violation>
<violation number="2" location="apps/web/src/server/public-api/api/domains/delete-domain.ts:80">
Domain deletion logic duplicates `apps/web/src/server/service/domain-service.ts:deleteDomain()` function and omits critical side effects (SES deletion).</violation>
</file>
<file name="apps/docs/api-reference/openapi.json">
<violation number="1" location="apps/docs/api-reference/openapi.json:629">
The DELETE path parameter `id` is declared nullable, but a URL segment cannot be null and the backend expects a concrete number.</violation>
<violation number="2" location="apps/docs/api-reference/openapi.json:632">
Path parameters must be required in OpenAPI, but this DELETE operation marks `id` as optional, making the spec invalid for generators.</violation>
<violation number="3" location="apps/docs/api-reference/openapi.json:639">
The DELETE 200 response schema describes a full domain object, but the handler actually returns `{ success: true, message: "Domain deleted successfully" }`, so the OpenAPI contract is incorrect and will break generated clients.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai
to give feedback, ask questions, or re-run the review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/sdk/types/schema.d.ts
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Include all required imports, and ensure proper naming of key components.
Files:
packages/sdk/types/schema.d.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}
: Use TypeScript with 2-space indentation and semicolons (enforced by Prettier)
ESLint must pass with zero warnings using @usesend/eslint-config
Do not use dynamic imports (avoid import() and dynamic loading)
Files:
packages/sdk/types/schema.d.ts
**/*.{ts,tsx,md}
📄 CodeRabbit inference engine (AGENTS.md)
Format code and docs with Prettier 3
Files:
packages/sdk/types/schema.d.ts
🔇 Additional comments (1)
packages/sdk/types/schema.d.ts (1)
372-372
: Path parameter type is appropriately non-nullable.Unlike the GET and verify endpoints that use
number | null
, the DELETE operation correctly usesnumber
for the requiredid
path parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Theres lots of valid review comments, please address it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/docs/api-reference/openapi.json (1)
637-795
: Fix response schema to match actual handler payload.The handler returns
{ success: boolean, message: string }
, but the spec publishes a full Domain object. Generated clients will deserialize the wrong shape and break once the DELETE route is invoked. Update the schema to reflect the success/message contract."200": { "description": "Domain deleted successfully", "content": { "application/json": { "schema": { - "type": "object", - "properties": { - "id": { - "type": "number", - "description": "The ID of the domain", - "example": 1 - }, - "name": { - "type": "string", - "description": "The name of the domain", - "example": "example.com" - }, - "teamId": { - "type": "number", - "description": "The ID of the team", - "example": 1 - }, - "status": { - "type": "string", - "enum": [ - "NOT_STARTED", - "PENDING", - "SUCCESS", - "FAILED", - "TEMPORARY_FAILURE" - ] - }, - "region": { - "type": "string", - "default": "us-east-1" - }, - "clickTracking": { - "type": "boolean", - "default": false - }, - "openTracking": { - "type": "boolean", - "default": false - }, - "publicKey": { - "type": "string" - }, - "dkimStatus": { - "type": "string", - "nullable": true - }, - "spfDetails": { - "type": "string", - "nullable": true - }, - "createdAt": { - "type": "string" - }, - "updatedAt": { - "type": "string" - }, - "dmarcAdded": { - "type": "boolean", - "default": false - }, - "isVerifying": { - "type": "boolean", - "default": false - }, - "errorMessage": { - "type": "string", - "nullable": true - }, - "subdomain": { - "type": "string", - "nullable": true - }, - "verificationError": { - "type": "string", - "nullable": true - }, - "lastCheckedTime": { - "type": "string", - "nullable": true - }, - "dnsRecords": { - "type": "array", - "items": { - "type": "object", - "properties": { - "type": { - "type": "string", - "enum": [ - "MX", - "TXT" - ], - "description": "DNS record type", - "example": "TXT" - }, - "name": { - "type": "string", - "description": "DNS record name", - "example": "mail" - }, - "value": { - "type": "string", - "description": "DNS record value", - "example": "v=spf1 include:amazonses.com ~all" - }, - "ttl": { - "type": "string", - "description": "DNS record TTL", - "example": "Auto" - }, - "priority": { - "type": "string", - "nullable": true, - "description": "DNS record priority", - "example": "10" - }, - "status": { - "type": "string", - "enum": [ - "NOT_STARTED", - "PENDING", - "SUCCESS", - "FAILED", - "TEMPORARY_FAILURE" - ] - }, - "recommended": { - "type": "boolean", - "description": "Whether the record is recommended" - } - }, - "required": [ - "type", - "name", - "value", - "ttl", - "status" - ] - } - } - }, - "required": [ - "id", - "name", - "teamId", - "status", - "publicKey", - "createdAt", - "updatedAt", - "dnsRecords" - ] + "type": "object", + "properties": { + "success": { + "type": "boolean", + "example": true + }, + "message": { + "type": "string", + "example": "Domain deleted successfully" + } + }, + "required": [ + "success", + "message" + ] } } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/docs/api-reference/domains/delete-domain.mdx
(1 hunks)apps/docs/api-reference/openapi.json
(1 hunks)apps/web/src/server/public-api/api/domains/delete-domain.ts
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/docs/api-reference/domains/delete-domain.mdx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/src/server/public-api/api/domains/delete-domain.ts
@KMKoushik Please review it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, lgtm
Thanks a lot for the confirmation and for helping me out. |
Fixes #262
Changes made
Summary by cubic
Adds DELETE /v1/domains/{id} to remove a domain via the Public API with API key scoping. JS/TS and Python SDKs and docs are updated so clients can delete domains programmatically.
Summary by CodeRabbit
New Features
Documentation