Skip to content

Conversation

john-royal
Copy link
Collaborator

@john-royal john-royal commented Oct 19, 2025

The getZoneByDomain function currently also fetches the zone settings after fetching the zone. This can cause errors when using an OAuth token for authentication — we can read zones but not zone settings for some reason.

It turns out that we don't actually need the zone settings to be returned by getZoneByDomain — the function is only referenced by inferZoneIdFromPattern:

image

Summary by CodeRabbit

  • Breaking Changes

    • Updated Cloudflare zone lookup API - now returns undefined instead of null for non-existent zones.
  • Refactor

    • Enhanced zone data structure with improved type definitions and clearer return type handling.

@coderabbitai
Copy link

coderabbitai bot commented Oct 19, 2025

Walkthrough

The Cloudflare zone module has been refactored to introduce a new public CloudflareZone interface and update the return type of getZoneByDomain from Promise<ZoneData | null> to Promise<CloudflareZone | undefined>. Implementation is simplified to use extractCloudflareResult. Tests updated to verify key fields only.

Changes

Cohort / File(s) Change Summary
Zone API refactoring
alchemy/src/cloudflare/zone.ts
Updated getZoneByDomain function signature to return Promise<CloudflareZone | undefined> instead of Promise<ZoneData | null>. Simplified implementation to directly return CloudflareZone via extractCloudflareResult. Introduced new public interface CloudflareZone with fields: id, name, type, status, paused, account, name_servers, original_name_servers, created_on, modified_on, activated_on. Added public type export.
Zone test updates
alchemy/test/cloudflare/zone.test.ts
Changed test assertions for getZoneByDomain: replaced detailed property checks with toMatchObject for id, name, type; updated null expectation to undefined; removed extensive field-by-field verification.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Resource Jsdoc ⚠️ Warning The PR introduces a new public interface CloudflareZone that is exported from the module. This interface has only a one-line JSDoc comment ("Cloudflare Zone response format") with no field-level documentation for any of its properties and no @example tag demonstrating usage. In contrast, the main Zone resource has comprehensive JSDoc with multiple detailed @example blocks, and the getZoneByDomain function also includes JSDoc with @example tags. The check requires that "inputs and outputs of resources have accurate JSDoc including @example," and the newly exported CloudflareZone interface—being a critical part of the public API surface—lacks the expected JSDoc quality and example documentation, particularly when compared to other types in the same file like ZoneData which is more detailed. To pass this check, add comprehensive JSDoc documentation to the CloudflareZone interface including: a detailed description of the interface's purpose and use case, inline documentation for each property explaining what it represents, and at least one @example tag showing how to work with a CloudflareZone object returned from getZoneByDomain. This would bring the documentation in line with the quality standards applied to other exported types and functions in the module.
Documentation ⚠️ Warning
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title "fix(cloudflare): avoid fetching settings when inferring zone id" follows the conventional commits format correctly, with a valid type (fix), scope (cloudflare), and description in imperative mood. The title is fully related to the main change in the changeset: the getZoneByDomain function was simplified to remove zone settings fetching by directly returning a CloudflareZone via extractCloudflareResult, eliminating the previous multi-step parsing logic. This directly addresses the PR objective of avoiding zone settings fetches when inferring the zone ID, which resolves permission issues with OAuth tokens that allow zone reading but not zone settings access.
Alchemy Style Resources ✅ Passed The custom check "Alchemy Style Resources" specifies guidelines that must be followed when creating new Alchemy resources, including proper handling of resource updates, the adopt property, the delete property for data stores, avoiding synchronous IO in resources, and preferring pathe over node:path. This PR does not introduce any new Alchemy resources. The changes only modify the getZoneByDomain utility function (a lookup helper) and introduce a CloudflareZone data interface. Since the check applies explicitly to "new resources" and this PR contains none, the check requirements are not applicable and therefore are satisfied.
Test Coverage ✅ Passed This PR modifies an existing function (getZoneByDomain) rather than introducing new resources. The associated test in zone.test.ts validates the function by creating a Cloudflare zone, calling getZoneByDomain, verifying against the Cloudflare API, and cleaning up resources with destroy(scope) in a finally clause. While the test assertions were simplified to match the reduced scope of the function (it no longer returns zone settings), the test still validates core functionality through control plane API calls and includes proper cleanup.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch john/cf-infer-zone-id-fix

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0a86694 and 457547a.

📒 Files selected for processing (2)
  • alchemy/src/cloudflare/zone.ts (2 hunks)
  • alchemy/test/cloudflare/zone.test.ts (1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-10-19T21:10:23.073Z
Learnt from: CR
PR: alchemy-run/alchemy#0
File: .cursor/rules/cloudflare.mdc:0-0
Timestamp: 2025-10-19T21:10:23.073Z
Learning: Applies to alchemy/src/cloudflare/bound.ts : Update alchemy/src/cloudflare/bound.ts to map the new Alchemy resource to the corresponding Cloudflare runtime binding type

Applied to files:

  • alchemy/src/cloudflare/zone.ts
📚 Learning: 2025-10-19T21:10:23.073Z
Learnt from: CR
PR: alchemy-run/alchemy#0
File: .cursor/rules/cloudflare.mdc:0-0
Timestamp: 2025-10-19T21:10:23.073Z
Learning: Applies to alchemy/src/cloudflare/worker.ts : Update alchemy/src/cloudflare/worker.ts to map the new binding to the Cloudflare metadata API

Applied to files:

  • alchemy/src/cloudflare/zone.ts
📚 Learning: 2025-10-19T21:10:23.073Z
Learnt from: CR
PR: alchemy-run/alchemy#0
File: .cursor/rules/cloudflare.mdc:0-0
Timestamp: 2025-10-19T21:10:23.073Z
Learning: Applies to alchemy/src/cloudflare/!{bindings,bound,worker}.ts : Create a new resource file at alchemy/src/cloudflare/{resource}.ts for the added resource

Applied to files:

  • alchemy/src/cloudflare/zone.ts
📚 Learning: 2025-10-19T21:15:05.076Z
Learnt from: CR
PR: alchemy-run/alchemy#0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2025-10-19T21:15:05.076Z
Learning: Applies to {test,tests}/**/*.{test,spec}.{ts,js,mjs,cjs} : Tests should clean up after themselves, typically calling destroy(scope) in a finally clause

Applied to files:

  • alchemy/test/cloudflare/zone.test.ts
⏰ 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: deploy-preview
🔇 Additional comments (5)
alchemy/test/cloudflare/zone.test.ts (2)

195-199: LGTM! Test correctly updated for new return type.

The test now verifies only the essential zone fields (id, name, type) returned by the simplified getZoneByDomain function, which no longer includes settings data. This aligns with the PR objective to avoid fetching zone settings.


206-206: LGTM! Assertion correctly updated for undefined return.

The expectation correctly matches the new return type of getZoneByDomain, which returns undefined instead of null for non-existent zones.

alchemy/src/cloudflare/zone.ts (3)

5-5: LGTM! Necessary import for the refactored implementation.


583-597: LGTM! Well-defined interface for raw Cloudflare API response.

The CloudflareZone interface correctly represents the raw Cloudflare API format with snake_case field names and excludes settings. This creates a clear separation: CloudflareZone for API responses (used by getZoneByDomain) and ZoneData for normalized resource outputs (used by the Zone resource).


569-578: Implementation correctly achieves PR objective with safe breaking changes.

The refactored getZoneByDomain function successfully avoids fetching zone settings by returning only CloudflareZone data from the API directly. This allows OAuth tokens with zone read permissions but without zone settings permissions to work properly.

Verification confirms:

  • Both callers in alchemy/src/cloudflare/route.ts (lines 458, 467) only access the id property, which is present in CloudflareZone
  • The Zone resource maintains its own data transformation logic and is unaffected by this change
  • The breaking change in return type (ZoneDataCloudflareZone) does not impact actual usage patterns

Comment @coderabbitai help to get the list of available commands and usage tips.

@pkg-pr-new
Copy link

pkg-pr-new bot commented Oct 19, 2025

Open in StackBlitz

npm i https://pkg.pr.new/alchemy-run/alchemy@1159

commit: 457547a

@john-royal
Copy link
Collaborator Author

Review me please @coderabbitai

@coderabbitai
Copy link

coderabbitai bot commented Oct 20, 2025

@john-royal I'll review the changes in this PR right away!

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@github-actions
Copy link

🚀 Website Preview Deployed

Your website preview is ready!

Preview URL: https://76e494c9-alchemy-website.alchemy-run.workers.dev

This preview was built from commit 625169e


🤖 This comment will be updated automatically when you push new commits to this PR.

Copy link
Contributor

@JacobMGEvans JacobMGEvans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh this is AWESOME, I was thinking of making all the Resources that take in a Zone to derive Zone ID, to also be able to infer it from domain name, but I was concerned about too many or too large of API requests for inference

@sam-goodwin sam-goodwin merged commit 2d79afe into main Oct 21, 2025
8 checks passed
@sam-goodwin sam-goodwin deleted the john/cf-infer-zone-id-fix branch October 21, 2025 06:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants