feat(cloudflare): Add Snippet resource #1073
Conversation
commit: |
🚀 Website Preview DeployedYour website preview is ready! Preview URL: https://a0fc674f-alchemy-website.alchemy-run.workers.dev This preview was built from commit c77cb5e 🤖 This comment will be updated automatically when you push new commits to this PR. |
…cob/feat-snippet-resource
…for easier testing and to keep with the codebase standards and conventions
JacobMGEvans
force-pushed
the
jacob/feat-snippet-resource
branch
from
69d91c5 to
7a53e11
Compare
sam-goodwin
left a comment
sam-goodwin
left a comment
There was a problem hiding this comment.
Looks like SnippetRule should be supported as input props?
Also missing docs
|
@coderabbitai can you review this please? |
|
@Mkassabov: I'll review the changes in this PR for you. ✅ Actions performedReview triggered.
|
![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
🧹 Nitpick comments (12)
alchemy/test/cloudflare/snippet.test.ts (4)
40-47: Strengthen deletion assertionCatching any error risks false positives. Assert the API error/status/message (e.g., 404/not found) to ensure we’re validating the intended failure mode.
Apply:
- } catch (error) { - expect(error).toBeTruthy(); - } + } catch (error) { + expect(error).toBeInstanceOf(Error); + expect(String((error as Error).message).toLowerCase()).toMatch(/not found|404/); + }
121-176: Consider asserting rule cleanup effectAfter deleteSnippetRules(api, zoneId), optionally verify that the created rules no longer appear in list results. Helpful to prevent orphaned rules across test runs.
207-237: Add an entrypoint-based snippet testWe only test inline script paths. Add a case using entrypoint to exercise file loading resolution and error paths.
305-321: Validate more name edge casesAdd cases for empty string, >255 chars, and uppercase-only to lock down validateSnippetName behavior.
alchemy/test/cloudflare/snippet-rule.test.ts (3)
357-361: Avoidas anyfor ZonePassing
zone: { id: zoneId } as anybypasses type checks. Prefer a proper Zone resource or a typed helper to construct a minimal Zone-like object.
160-175: Toggle tests: assert persistence across updatesWe flip enabled on the same logical rule. Consider asserting ruleId stability and checking listSnippetRules to ensure the single rule’s state changed rather than creating duplicates.
Also applies to: 178-195, 187-195
324-336: Complex expression: add a quick negative matchAdd one assertion that a different expression isn’t present to avoid false positives when zones have pre-existing rules.
alchemy/src/cloudflare/snippet-rule.ts (2)
268-291: Adoption fallback may select the wrong rule in duplicate scenariosIf multiple rules share the same expression and snippetName, adoption chooses an arbitrary match. Consider disambiguating (e.g., prefer enabled=true, newest last_updated) or making ambiguity an error.
298-343: Delete by ID behavior: consider idempotency and 404sIf the target rule disappears between list and update, treat as idempotent success rather than throwing. Also short-circuit on a 404 from update/delete endpoints if available.
alchemy/src/cloudflare/snippet.ts (3)
245-254: Improve entrypoint read errorsWrap readFile with a descriptive error to aid debugging when paths are wrong.
if ("entrypoint" in props && props.entrypoint) { const filePath = resolve(props.entrypoint); - return await readFile(filePath, "utf-8"); + try { + return await readFile(filePath, "utf-8"); + } catch (err) { + throw new Error( + `Failed to read snippet entrypoint at "${filePath}": ${(err as Error).message}`, + ); + } }
27-29: Docs vs behavior: default name derivationJSDoc mentions
${app}-${stage}-${id}(with dashes to underscores), while code usesthis.scope.createPhysicalName(id, "_"). Update the doc to match the actual behavior or vice versa.Also applies to: 194-206
379-387: Delete: parse JSON body for robust 400 handlingWe string-search for “requested snippet not found”. Prefer parsing JSON and checking a code/message field to reduce brittleness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
alchemy/src/cloudflare/index.ts(1 hunks)alchemy/src/cloudflare/snippet-rule.ts(1 hunks)alchemy/src/cloudflare/snippet.ts(1 hunks)alchemy/test/cloudflare/snippet-rule.test.ts(1 hunks)alchemy/test/cloudflare/snippet.test.ts(1 hunks)
🔇 Additional comments (5)
alchemy/src/cloudflare/index.ts (1)
62-63: Re-exports look goodSurface area extended cleanly; no side effects.
alchemy/src/cloudflare/snippet-rule.ts (2)
368-381: Request body and endpoint are correct per official Cloudflare API.The endpoint
PUT /zones/{zoneId}/snippets/snippet_ruleswith body{ "rules": [{ "expression", "snippet_name", "description", "enabled" }] }is the documented API to create/update (upsert) snippet rules. The code implementation matches this specification exactly. Note: the PUT replaces the zone's snippet rules — to keep existing rules you must include them all in the request body.
216-269: Acknowledge Cloudflare API limitation; whole-set replacement pattern lacks concurrency controlThe race condition risk you identified is real. Cloudflare's Snippet Rules API does not support optimistic concurrency (ETag/If-Match), and the PUT endpoint performs full-set replacement without versioning. This is a known API limitation, not a code defect.
Suggested mitigations:
- Document this eventual consistency trade-off for operators (rules may be lost if modified concurrently by other actors)
- Implement retry logic with exponential backoff to increase success rate on transient conflicts
- Consider adding a brief comment in the code flagging the race window (list → modify → put)
The code itself correctly implements the only pattern the API supports.
alchemy/src/cloudflare/snippet.ts (2)
308-315: No action required — HEAD not supportedThe Cloudflare Snippets API does not support HEAD on
/zones/{zoneId}/snippets/{snippetName}(official docs list only GET, PUT, and DELETE). The current GET implementation is correct and optimal.
276-302: ****The multipart payload format in the code is correct. Cloudflare's Snippets PUT endpoint accepts multipart/form-data with a files part containing the JS file and a metadata part (JSON) whose main_module value equals the uploaded filename. Your implementation matches this specification exactly.
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @Mkassabov. * #1073 (comment) The following files were modified: * `alchemy/src/cloudflare/snippet-rule.ts` * `alchemy/src/cloudflare/snippet.ts`
|
@coderabbitai Hey we changed some of the review rules. can you review this again? |
|
@Mkassabov: I'll review the changes again with the updated review rules. ✅ Actions performedReview triggered.
|
Mkassabov
force-pushed
the
jacob/feat-snippet-resource
branch
from
20e9f7f to
bf36413
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
alchemy/src/cloudflare/snippet.ts (4)
35-35: Optional: Simplify type annotation.The explicit
undefined |union is redundant since optional properties are alreadyT | undefinedby default.- script?: undefined | string; + script?: string;
243-246: Optional: Simplify property checks.The
inoperator is unnecessary for optional properties. Checking the property value directly is more idiomatic.- if ("script" in props && props.script) { + if (props.script) { return props.script; } - if ("entrypoint" in props && props.entrypoint) { + if (props.entrypoint) {
247-250: Consider adding error context for file operations.If the entrypoint file doesn't exist or can't be read, the raw Node.js error might not clearly indicate which snippet configuration is problematic.
if (props.entrypoint) { const filePath = resolve(props.entrypoint); - return await readFile(filePath, "utf-8"); + try { + return await readFile(filePath, "utf-8"); + } catch (error) { + throw new Error( + `Failed to read snippet entrypoint "${props.entrypoint}": ${error.message}` + ); + } }
377-383: Optional: Add comment explaining special 400 handling.The special case for 400 status with "requested snippet not found" message handles an API inconsistency where some "not found" conditions return 400 instead of 404. A brief comment would help future maintainers understand this workaround.
if (!deleteResponse.ok && deleteResponse.status !== 404) { if (deleteResponse.status === 400) { + // Cloudflare API sometimes returns 400 with "requested snippet not found" + // instead of 404 when the snippet doesn't exist const errorBody = await deleteResponse.text(); if (errorBody.includes("requested snippet not found")) { return; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
alchemy/src/cloudflare/snippet.ts(1 hunks)
🔇 Additional comments (3)
alchemy/src/cloudflare/snippet.ts (3)
54-57: Excellent fix on the secret exposure blocker.The sensitive fields (accountId, apiToken) are now properly omitted from the public output type, preventing credential leakage into logs and state.
96-186: Outstanding documentation.The comprehensive examples and clear explanation of snippet behavior (including the requirement for SnippetRule to execute) provide excellent guidance for users.
187-237: Solid resource implementation.The lifecycle management is correct, with proper phase handling, early validation, and clean separation of concerns. The adopt logic and name generation align well with Cloudflare's constraints.
Hey @coderabbitai its changed now. rereview please! |
|
@Mkassabov: I'll review the changes with the latest updates. ✅ Actions performedReview triggered.
|
|
@Mkassabov: I'll review the latest changes in this PR for you. ✅ Actions performedReview triggered.
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
alchemy/src/cloudflare/snippet.ts (1)
395-411: Change return type frombooleantovoid.The function never returns
false—it either throws or returnstrue. This makes the boolean return type misleading. The previous review already flagged this issue, and it remains unaddressed.Apply this diff:
-export function validateSnippetName(name: string): boolean { +export function validateSnippetName(name: string): void { if (!name || name.trim().length === 0) { throw new Error("Snippet name cannot be empty"); } if (name.length > 255) { throw new Error("Snippet name cannot exceed 255 characters"); } if (!/^[a-z0-9_]+$/.test(name)) { throw new Error( "Snippet name must contain only lowercase letters (a-z), numbers (0-9), and underscores (_). Invalid characters found.", ); } - - return true; }
🧹 Nitpick comments (2)
alchemy/src/cloudflare/snippet.ts (2)
35-35: Remove redundantundefinedfrom union type.The explicit
undefined |inscript?: undefined | stringis redundant because the optional modifier?already makes the typestring | undefined.Apply this diff:
- script?: undefined | string; + script?: string;
243-252: Consider simplifying property checks.The
"script" in propsand"entrypoint" in propschecks are more defensive than necessary. Since both properties are optional, you can simplify the conditions to just check truthiness.Apply this diff:
async function getScriptContent(props: SnippetProps): Promise<string> { - if ("script" in props && props.script) { + if (props.script) { return props.script; } - if ("entrypoint" in props && props.entrypoint) { + if (props.entrypoint) { const filePath = resolve(props.entrypoint); return await readFile(filePath, "utf-8"); } throw new Error("Either 'script' or 'entrypoint' must be provided"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
alchemy/src/cloudflare/snippet.ts(1 hunks)
⏰ 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 (3)
alchemy/src/cloudflare/snippet.ts (3)
54-57: LGTM! Secret handling is correct.The Snippet type correctly omits sensitive fields (
accountId,apiToken) from the public interface, and the return object (lines 227-235) doesn't include these fields. This prevents credential leakage into state/outputs.
187-237: LGTM! Resource implementation is solid.The resource correctly handles all lifecycle phases (create, update, delete), implements early validation, properly manages adoption semantics, and uses underscore separators for physical names to comply with Cloudflare naming restrictions.
268-386: LGTM! API helpers are well-implemented.The CRUD operations are correctly implemented with appropriate error handling. The special case for 400 status in
deleteSnippet(lines 378-382) appears to be a workaround for Cloudflare API behavior where "not found" errors sometimes return 400 instead of 404. The defensive programming ensures idempotent deletes.
…cob/feat-snippet-resource
…emy-run/alchemy into jacob/feat-snippet-resource
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
alchemy/test/cloudflare/snippet.test.ts (3)
40-47: Harden deletion checks with polling to handle eventual consistency.Relying on a single get to throw is brittle. Use snippetExists and poll briefly to reduce flakes.
-import { - Snippet, - deleteSnippet, - getSnippet, - getSnippetContent, - listSnippets, -} from "../../src/cloudflare/snippet.ts"; +import { + Snippet, + deleteSnippet, + getSnippet, + getSnippetContent, + listSnippets, + snippetExists, +} from "../../src/cloudflare/snippet.ts"; @@ -async function verifySnippetDeleted(snippetName: string): Promise<void> { - try { - await getSnippet(api, zoneId, snippetName); - throw new Error("Snippet should have been deleted"); - } catch (error) { - expect(error).toBeTruthy(); - } -} +async function verifySnippetDeleted(snippetName: string, timeoutMs = 30000) { + const start = Date.now(); + while (Date.now() - start < timeoutMs) { + if (!(await snippetExists(api, zoneId, snippetName))) return; + await new Promise((r) => setTimeout(r, 1000)); + } + throw new Error(`Snippet ${snippetName} still exists after ${timeoutMs}ms`); +}Also applies to: 109-112
50-114: Set explicit, generous timeouts for networked E2E tests.Some runs can exceed defaults; align with snippet-rule suite style.
- test("create, update, and delete snippet", async (scope) => { + test("create, update, and delete snippet", async (scope) => { @@ - }); + }, 120000); @@ - test("snippet with rules", async (scope) => { + test("snippet with rules", async (scope) => { @@ - }); + }, 120000); @@ - test("delete snippet explicitly", async (scope) => { + test("delete snippet explicitly", async (scope) => { @@ - }); + }, 60000); @@ - test("list snippets in zone", async (scope) => { + test("list snippets in zone", async (scope) => { @@ - }); + }, 60000); @@ - test("adopt existing snippet", async (scope) => { + test("adopt existing snippet", async (scope) => { @@ - }); + }, 120000); @@ - test("validate snippet name throws error for invalid characters", async () => { + test("validate snippet name throws error for invalid characters", async () => { @@ - }); + }, 30000);Also applies to: 115-177, 179-205, 207-237, 239-303, 305-322
31-33: Move createSnippetName to alchemy/test/util.ts to decouple test suites.createSnippetName is currently exported from snippet.test.ts and imported in snippet-rule.test.ts, creating unnecessary coupling between test files. This utility should be centralized in
alchemy/test/util.tsalongsideBRANCH_PREFIXand other shared test helpers.Update:
- Move
createSnippetNamefunction toalchemy/test/util.ts- Update imports in
alchemy/test/cloudflare/snippet.test.ts:import { createSnippetName } from "../util.ts"- Update imports in
alchemy/test/cloudflare/snippet-rule.test.ts:import { createSnippetName } from "../util.ts"alchemy/test/cloudflare/snippet-rule.test.ts (2)
25-151: Standardize explicit timeouts across all E2E tests.A few tests set timeouts; others rely on defaults. Make them consistent to reduce flakes.
- test("handles empty rules array", async (scope) => { + test("handles empty rules array", async (scope) => { @@ - }); + }, 60000); @@ - test("throws error for duplicate rule definitions", async (scope) => { + test("throws error for duplicate rule definitions", async (scope) => { @@ - }); + }, 60000); @@ - test("adoption prevents conflicts", async (scope) => { + test("adoption prevents conflicts", async (scope) => { @@ - }); + }, 120000); @@ - test("update using snippet string references", async (scope) => { + test("update using snippet string references", async (scope) => { @@ - }); + }, 60000); @@ - test("rule execution order determined by array position", async (scope) => { + test("rule execution order determined by array position", async (scope) => { @@ - }); + }, 60000);Also applies to: 153-167, 169-201, 203-261, 263-304, 305-347, 349-411
263-304: Test name vs. assertion mismatch.“performance - single API call for batch” doesn’t verify call count. Either instrument the client or rename to reflect what’s asserted.
- test("performance - single API call for batch", async (scope) => { + test("batch creates 10 rules in one update", async (scope) => {If you want to actually assert call count, consider injecting a transport spy in createCloudflareApi and counting requests in scope.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
alchemy/src/cloudflare/snippet-rule.ts(1 hunks)alchemy/src/cloudflare/snippet.ts(1 hunks)alchemy/test/cloudflare/snippet-rule.test.ts(1 hunks)alchemy/test/cloudflare/snippet.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- alchemy/src/cloudflare/snippet.ts
- alchemy/src/cloudflare/snippet-rule.ts
🧰 Additional context used
🧠 Learnings (1)
📚 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/test/cloudflare/*.test.ts : Create a corresponding test file at alchemy/test/cloudflare/{resource}.test.ts
Applied to files:
alchemy/test/cloudflare/snippet-rule.test.tsalchemy/test/cloudflare/snippet.test.ts
🔇 Additional comments (1)
alchemy/test/cloudflare/snippet-rule.test.ts (1)
25-25: Global config allows cross-file concurrency; verify no resource conflicts between Cloudflare test suites.The global vitest config has
sequence: { concurrent: true }, which allows test suites to run concurrently across different files. Althoughdescribe.sequential()enforces sequential execution within snippet-rule.test.ts, multiple other Cloudflare suites (logpush-job.test.ts, health-check.test.ts, container.test.ts, nodejs-import-warning.test.ts) also usedescribe.sequential()but are not coordinated to prevent cross-file concurrency. These could run in parallel threads simultaneously, potentially causing resource conflicts if they mutate shared Cloudflare objects. Manually verify whether rule-mutation tests should be isolated via a testSequencer plugin or grouped sequential strategy.
Summary by CodeRabbit
New Features
Tests
Documentation