Skip to content

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@vercel
Copy link

vercel bot commented Nov 13, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
rivetkit-serverless Ready Ready Preview Comment Nov 20, 2025 11:37pm
3 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Nov 20, 2025 11:37pm
rivet-inspector Ignored Ignored Preview Nov 20, 2025 11:37pm
rivet-site Ignored Ignored Preview Nov 20, 2025 11:37pm

Copy link
Member Author

NathanFlurry commented Nov 13, 2025

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more


How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@claude
Copy link

claude bot commented Nov 13, 2025

PR Review: feat(cloudflare-workers): add createInlineClient

Overview

This PR introduces a new createInlineClient function that allows developers to access Rivet Actors privately within Cloudflare Workers without exposing a public manager API. The existing createHandler function has been refactored to use this new inline client internally.


✅ Strengths

  1. Good API Design: The separation between createInlineClient (for private access) and createHandler (for public API exposure) provides clear, composable primitives.

  2. Excellent Documentation: The JSDoc comments clearly explain the differences between the two approaches and when to use each.

  3. Clean Refactoring: The createHandler function now reuses createInlineClient, avoiding code duplication and maintaining consistency.

  4. Complete Example: The new examples/cloudflare-workers-inline-client/ directory provides a working reference implementation.

  5. Proper Exports: The new types (InlineOutput, HandlerOutput) are correctly exported from mod.ts.


🔍 Code Quality Observations

1. Missing Export Type Annotation ⚠️

In rivetkit-typescript/packages/cloudflare-workers/src/mod.ts:9-10:

export {
	type Bindings,
	createHandler,
	createInlineClient,
	HandlerOutput,
	InlineOutput,
} from "./handler";

Issue: HandlerOutput and InlineOutput should have type prefix since they're type-only exports.

Recommendation:

export {
	type Bindings,
	createHandler,
	createInlineClient,
	type HandlerOutput,
	type InlineOutput,
} from "./handler";

2. Unclear OpenAPI Changes ⚠️

The PR removes 275 lines from rivetkit-openapi/openapi.json, including:

  • /gateway/{actorId}/health endpoint
  • /gateway/{actorId}/action/{action} endpoint
  • /gateway/{actorId}/request/{path} endpoint (all HTTP methods)

While it adds "required": true to some request bodies.

Questions:

  • Are these gateway endpoints being removed intentionally as part of this feature?
  • Is this OpenAPI change related to the inline client feature, or is it a separate cleanup?
  • Should this be documented in the PR description?

3. Runner Protocol Change ℹ️

In engine/sdks/typescript/runner-protocol/src/index.ts:

The custom assert function is replaced with Node.js's built-in assert:

+import assert from "node:assert"
-function assert(condition: boolean, message?: string): asserts condition {
-    if (!condition) throw new Error(message ?? "Assertion failed")
-}

Note: This is a good cleanup but should be verified:

  • Does this maintain the same error behavior?
  • Is node:assert available in all target environments (Cloudflare Workers, Node.js, etc.)?

4. Example Package Version ℹ️

In examples/cloudflare-workers-inline-client/package.json:3:

"version": "2.0.21"

This version seems high for a new example. Should it start at 0.1.0 or match the workspace version pattern?


🧪 Test Coverage

Missing: No new tests were added for the createInlineClient function.

Recommendation: Add tests to verify:

  • The inline client can successfully invoke actor actions
  • The returned client object has the correct type
  • The fetch handler works correctly when manually routed
  • Error handling when actor operations fail

Existing Tests: The PR doesn't modify existing test files, which is good - it suggests backward compatibility.


🔒 Security Considerations

No issues identified. The inline client pattern actually improves security by:

  • Allowing developers to keep actor access internal
  • Avoiding unnecessary exposure of the manager API
  • Maintaining the same authentication/authorization patterns as before

⚡ Performance Considerations

Positive: The refactoring doesn't introduce performance overhead:

  • createHandler calls createInlineClient once during initialization
  • No additional runtime overhead in the request path
  • The same underlying driver and Durable Object implementation is used

📋 Documentation & Examples

Strengths:

  • ✅ Clear JSDoc comments on both functions
  • ✅ Complete working example with README
  • ✅ Example includes both client script and worker implementation

Suggestions:

  • Add a comment in the example explaining why someone would choose inline client over handler
  • Consider adding a comparison table in the main package README

🎯 Recommendations Summary

Must Fix:

  1. ⚠️ Add type prefix to HandlerOutput and InlineOutput exports in mod.ts

Should Address:
2. ⚠️ Document the OpenAPI changes - are the gateway endpoint removals intentional?
3. ⚠️ Verify node:assert compatibility in all target environments
4. 📝 Add tests for the new createInlineClient function

Nice to Have:
5. 📝 Consider normalizing the example package version
6. 📝 Add comparison documentation between createHandler and createInlineClient


✨ Conclusion

This is a well-designed feature that provides a cleaner API for common use cases. The code quality is high, and the refactoring maintains backward compatibility. The main concerns are around the unexplained OpenAPI changes and missing test coverage.

Overall Assessment: ✅ Approved with minor suggestions

Great work on the clean abstraction and documentation! 🎉

@pkg-pr-new
Copy link

pkg-pr-new bot commented Nov 13, 2025

More templates

@rivetkit/cloudflare-workers

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/cloudflare-workers@3466

@rivetkit/db

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/db@3466

@rivetkit/framework-base

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/framework-base@3466

@rivetkit/next-js

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/next-js@3466

@rivetkit/react

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/react@3466

rivetkit

pnpm add https://pkg.pr.new/rivet-dev/rivet/rivetkit@3466

@rivetkit/sql-loader

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/sql-loader@3466

@rivetkit/engine-runner

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner@3466

@rivetkit/engine-runner-protocol

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner-protocol@3466

commit: 7210249

@NathanFlurry NathanFlurry changed the base branch from 11-12-chore_rivetkit_fix_type_checks to graphite-base/3466 November 13, 2025 22:27
@NathanFlurry NathanFlurry force-pushed the 11-13-feat_cloudflare-workers_add_createinlineclient_ branch from 22eaf35 to c4298c4 Compare November 20, 2025 22:32
@vercel vercel bot temporarily deployed to Preview – rivetkit-serverless November 20, 2025 22:32 Inactive
@NathanFlurry NathanFlurry changed the base branch from graphite-base/3466 to 11-20-fix_rivetkit_uri_encode_actor_ids_to_fix_cloudfalre_workers_actor_id_with_generation November 20, 2025 22:33
@claude
Copy link

claude bot commented Nov 20, 2025

Code Review: Add createInlineClient for Cloudflare Workers

This PR introduces a new createInlineClient function that enables private actor access without exposing a public manager API. Overall, this is a well-structured addition that follows good separation of concerns. Here's my detailed feedback:


✅ Strengths

  1. Clean API Design: The refactoring extracts shared logic into createInlineClient, which createHandler now reuses. This follows the DRY principle well.

  2. Good Documentation: JSDoc comments clearly explain when to use createInlineClient vs createHandler, including migration paths.

  3. Type Safety: Strong TypeScript types with proper generics (InlineOutput<A extends Registry<any>>) maintain type safety across the API boundary.

  4. Backward Compatibility: Existing createHandler API remains unchanged, preventing breaking changes.

  5. Practical Example: The cloudflare-workers-inline-client example demonstrates both inline usage and optional public API mounting.


🔍 Issues & Recommendations

1. Missing Type Export (mod.ts:9-10)

export {
	type Bindings,
	createHandler,
	createInlineClient,
	HandlerOutput,
	InlineOutput,
} from "./handler";

Issue: HandlerOutput and InlineOutput should be exported as types (with type keyword) since they're interfaces, not runtime values.

Fix:

export {
	type Bindings,
	createHandler,
	createInlineClient,
	type HandlerOutput,
	type InlineOutput,
} from "./handler";

2. Inconsistent Type Import (handler.ts:8)

import { type Config, ConfigSchema, type InputConfig } from "./config";

Observation: This follows TypeScript 3.8+ inline type imports, which is good. However, check if this is consistent with the project's style. From CLAUDE.md, I see "Always add imports at the top of the file" - consider separating type-only imports if that's the convention.

3. Security: Path Traversal Risk (index.ts:22)

const name = url.pathname.slice("/increment/".length);
const counter = client.counter.getOrCreate(name);

Issue: User-controlled path input is directly used as actor name without validation. Malicious inputs like ../../../../etc/passwd could be problematic depending on downstream validation.

Recommendation: Add input validation:

const name = url.pathname.slice("/increment/".length);
// Validate name matches expected format (e.g., alphanumeric, kebab-case)
if (!/^[a-z0-9-]+$/.test(name)) {
	return new Response("Invalid name format", { status: 400 });
}
const counter = client.counter.getOrCreate(name);

4. Debug Console.log Left in Code (index.ts:36)

console.log("URL", url.toString());

Issue: Debug logging statement in example code. Should either be removed or converted to structured logging.

Recommendation: Remove or use tracing per CLAUDE.md guidelines:

// Remove entirely for production, or:
tracing::info!(?url, "handling rivet request");  // (if tracing is available in TS)

5. Wrangler Configuration File Extension (wrangler.json)

{
	"name": "rivetkit-cloudflare-workers-example",
	...
}

Observation: File is named wrangler.json but Cloudflare Wrangler typically uses wrangler.toml (TOML format) or wrangler.jsonc (with comments). Verify this is intentional and works with the Cloudflare Workers runtime.

6. Missing Error Handling (index.ts:25)

const newCount = await counter.increment(1);
return new Response(`New Count: ${newCount}`, {
	headers: { "Content-Type": "text/plain" },
});

Issue: No try-catch around async actor operations. Errors will propagate as unhandled promise rejections.

Recommendation:

try {
	const counter = client.counter.getOrCreate(name);
	const newCount = await counter.increment(1);
	return new Response(`New Count: ${newCount}`, {
		headers: { "Content-Type": "text/plain" },
	});
} catch (error) {
	console.error("Failed to increment counter:", error);
	return new Response("Internal Server Error", { status: 500 });
}

7. Test Coverage Gap

Observation: The PR adds significant new functionality but doesn't include tests for createInlineClient. The existing test files (driver-tests.test.ts, etc.) don't appear to cover this new function.

Recommendation: Add integration tests covering:

  • createInlineClient returns expected output structure
  • Client operations work without public API exposure
  • Optional fetch handler correctly routes requests
  • Error cases (invalid registry, missing config, etc.)

8. Type Safety: Loose any Type (handler.ts:32)

fetch: (request: Request, ...args: any) => Response | Promise<Response>;

Issue: Using any type defeats type safety. The remaining parameters should match Cloudflare's ExportedHandler signature.

Fix:

fetch: (
	request: Request, 
	env: Bindings, 
	ctx: ExecutionContext
) => Response | Promise<Response>;

📊 Performance Considerations

  1. Synchronous Client Creation: The registry.start(runConfig) call at module scope (handler.ts:80) initializes the client during module evaluation. Verify this doesn't block Cloudflare Worker cold starts.

  2. URL Parsing: Creating new URL objects on every request (index.ts:15) is fine for Workers, but consider caching regex patterns if route matching becomes complex.


🔒 Security Summary

  • Medium Risk: Path traversal potential in example code (could set bad patterns for users)
  • Low Risk: Missing error handling could leak stack traces in responses

📋 Checklist for Author

  • Export HandlerOutput and InlineOutput as types
  • Add input validation to example's name extraction
  • Remove debug console.log
  • Verify wrangler.json vs wrangler.toml is intentional
  • Add try-catch error handling to example
  • Add tests for createInlineClient
  • Fix any type in InlineOutput.fetch signature
  • Consider documenting cold start behavior

🎯 Overall Assessment

Recommendation: Approve with minor changes required

This is a solid feature addition that provides genuine value by enabling private actor access patterns. The code quality is good, but the issues above should be addressed before merging:

  • Must Fix: Type exports, security validation in example, test coverage
  • Should Fix: Error handling, type safety improvements
  • Nice to Have: Cold start documentation

Great work on the clean API design and comprehensive example! 🚀

@claude
Copy link

claude bot commented Nov 20, 2025

Pull Request Review: createInlineClient Feature

Thank you for this contribution! This is a well-designed feature that adds significant value by enabling private actor access without exposing a public API. Here's my detailed review:


Strengths

Code Quality

  • Clean refactoring: Excellent extraction of createInlineClient from createHandler, following DRY principles
  • Type safety: Strong TypeScript typing with proper generic constraints (Registry<any>)
  • Documentation: Clear JSDoc comments explaining the purpose and usage of both functions
  • Consistency: Follows existing codebase patterns and conventions

Architecture

  • Good separation of concerns: createInlineClient provides low-level primitives, while createHandler provides a higher-level abstraction
  • Flexible design: Users can choose between the convenience of createHandler or the control of createInlineClient
  • Reusability: createHandler now properly reuses createInlineClient, avoiding code duplication

Example Quality

  • Comprehensive examples: The new example demonstrates both HTTP and RivetKit client usage
  • Clear documentation: README provides good getting started instructions
  • Multiple use cases: Shows both inline client usage and optional public API mounting

🐛 Issues Found

1. Missing Resource Cleanup in Example (High Priority)

Location: examples/cloudflare-workers-inline-client/src/index.ts:24-25

The example creates a counter actor but never disposes of it:

const counter = client.counter.getOrCreate(name);
const newCount = await counter.increment(1);
// Missing: await counter.dispose();

Issue: This could lead to connection leaks or resource exhaustion in production.

Recommendation: Add proper cleanup:

const counter = client.counter.getOrCreate(name);
try {
  const newCount = await counter.increment(1);
  return new Response(`New Count: ${newCount}`, {
    headers: { "Content-Type": "text/plain" },
  });
} finally {
  await counter.dispose();
}

Or use .connect() if you want a persistent connection (as shown in client-rivetkit.ts:13).


2. Inconsistent Actor Usage Pattern (Medium Priority)

Locations:

  • examples/cloudflare-workers-inline-client/src/index.ts:24 (no .connect())
  • examples/cloudflare-workers-inline-client/scripts/client-rivetkit.ts:13 (uses .connect())

The HTTP example uses getOrCreate() directly, while the RivetKit client example uses getOrCreate().connect(). This inconsistency might confuse users about the correct pattern.

Recommendation: Document when to use each pattern or standardize the example to use one approach.


3. Debug Console.log Left in Production Code (Low Priority)

Location: examples/cloudflare-workers-inline-client/src/index.ts:36

console.log("URL", url.toString());

This debug statement should be removed or replaced with proper structured logging (if available in the Cloudflare Workers context).


4. Type Safety Issue with ...args: any (Medium Priority)

Location: rivetkit-typescript/packages/cloudflare-workers/src/handler.ts:32

fetch: (request: Request, ...args: any) => Response | Promise<Response>;

Using any reduces type safety. Consider using a more specific type signature that matches Cloudflare Workers fetch handler signature.

Recommendation:

fetch: ExportedHandlerFetchHandler<Bindings>

🔒 Security Considerations

Path Traversal Prevention (Informational)

The path stripping logic looks safe:

const strippedPath = url.pathname.substring("/rivet".length);
url.pathname = strippedPath;

However, ensure that the underlying fetch handler properly validates paths to prevent potential path traversal attacks.


Performance Considerations

Synchronous Initialization (Good)

The refactoring maintains the previous behavior where registry.start() is called synchronously, which is correct. The previous code had an unnecessary await that has been properly removed.

URL Object Creation

Creating a new URL object for each request is standard practice and acceptable. No performance concerns here.


🧪 Test Coverage

Status: ⚠️ No new tests added

While the package has existing tests (driver-tests.test.ts, etc.), this new public API doesn't have specific test coverage.

Recommendation: Consider adding tests for:

  1. createInlineClient returns the expected interface
  2. createHandler properly wraps createInlineClient
  3. Path mounting/stripping works correctly
  4. Client can successfully call actor methods

However, given that this is a refactoring that exposes existing internal logic, the existing driver tests likely provide adequate coverage.


📝 Minor Suggestions

1. Export Type Consistency

In mod.ts:9-10, you export both the type and value for HandlerOutput and InlineOutput:

export {
  type Bindings,
  createHandler,
  createInlineClient,
  HandlerOutput,      // ← Should this be 'type HandlerOutput'?
  InlineOutput,       // ← Should this be 'type InlineOutput'?
} from "./handler";

Since these are interfaces, they should be exported as types only:

export type { HandlerOutput, InlineOutput } from "./handler";

2. Wrangler Configuration

The example uses wrangler.json instead of wrangler.toml. While JSON is supported, TOML is more common in the ecosystem. Consider if this is intentional.


Summary

Overall Assessment: This is a high-quality PR that adds valuable functionality with a clean implementation.

Must Fix Before Merge:

  1. ⚠️ Add resource cleanup to the inline client example (issue [SVC-2555] Set up issue templates #1)
  2. ⚠️ Remove debug console.log statement (issue [SVC-2504] Fix 5 GB upload limit for local development from Cloudflare #3)

Should Consider:

Nice to Have:

  • Add unit tests for the new public API

Great work on this feature! 🎉

@NathanFlurry NathanFlurry force-pushed the 11-13-feat_cloudflare-workers_add_createinlineclient_ branch from 2c9312e to 7210249 Compare November 20, 2025 23:35
@NathanFlurry NathanFlurry force-pushed the 11-20-fix_rivetkit_uri_encode_actor_ids_to_fix_cloudfalre_workers_actor_id_with_generation branch from 9faab1f to 1431ddd Compare November 20, 2025 23:35
@claude
Copy link

claude bot commented Nov 20, 2025

Code Review: Add createInlineClient to cloudflare-workers

This PR introduces a new createInlineClient function that allows developers to access Rivet Actors privately within Cloudflare Workers without exposing a public manager API.

Summary

  • Purpose: Refactors createHandler to extract core functionality into createInlineClient
  • Scope: Adds new example, updates exports, minimal breaking changes

Strengths

  1. Good separation of concerns - clean refactor that provides flexibility
  2. Comprehensive example demonstrates both HTTP and RivetKit client usage
  3. Backward compatibility maintained
  4. Clear JSDoc documentation

@claude
Copy link

claude bot commented Nov 20, 2025

Potential Issues

1. Missing Error Handling in Example (examples/cloudflare-workers-inline-client/src/index.ts:24-25)

  • No error handling for actor creation or action invocation
  • If the actor fails to initialize or the action throws, the worker will return a 500
  • Recommend adding try-catch with appropriate error responses

2. Debug Console.log (examples/cloudflare-workers-inline-client/src/index.ts:36)

  • Debug logging should be removed from example code
  • Line: console.log("URL", url.toString());

3. Type Safety Issue (rivetkit-typescript/packages/cloudflare-workers/src/handler.ts:32)

  • Using any for rest parameters reduces type safety
  • Should match proper Cloudflare ExportedHandlerFetchHandler types

4. Missing Resource Cleanup

  • Inline client example does not call .connect() before using the actor
  • Unlike the RivetKit client example (scripts/client-rivetkit.ts:13)
  • This inconsistency could confuse users

@claude
Copy link

claude bot commented Nov 20, 2025

Security Considerations

Path Traversal Risk (examples/cloudflare-workers-inline-client/src/index.ts:22)

  • User-controlled input from URL pathname is passed directly to getOrCreate without validation
  • Could allow unexpected characters or path traversal attempts
  • Recommendation: Validate the name parameter before use

@claude
Copy link

claude bot commented Nov 20, 2025

Code Quality & Test Coverage

Inconsistent Type Export (rivetkit-typescript/packages/cloudflare-workers/src/mod.ts:9-10)

  • HandlerOutput and InlineOutput exported without the type keyword
  • Other exports in the file use type keyword for consistency
  • Recommend: export type { HandlerOutput, InlineOutput } from "./handler"

Missing Tests

  • No tests added for the new createInlineClient function
  • Should add tests to verify:
    1. Function returns expected output structure
    2. Fetch handler routes requests properly
    3. Client can invoke actor actions
    4. Error cases are handled appropriately
  • Suggest adding: rivetkit-typescript/packages/cloudflare-workers/tests/inline-client.test.ts

@claude
Copy link

claude bot commented Nov 20, 2025

Summary of Recommendations

High Priority:

  1. Add error handling to example fetch handler
  2. Remove debug console.log from example
  3. Fix type safety for fetch signature (any → proper types)
  4. Add input validation for actor names from URL

Medium Priority:
5. Add tests for createInlineClient
6. Fix inconsistent type exports in mod.ts
7. Clarify .connect() usage difference in examples

Low Priority:
8. Minor URL reconstruction optimization
9. Enhance example README with usage guidance


Approval

This is a solid feature addition that provides valuable flexibility. The code quality is good and the refactoring is clean. However, I recommend addressing the high-priority issues (especially error handling and input validation) before merging.

The architectural decision to extract createInlineClient is excellent and maintains backward compatibility while enabling new use cases. Nice work!

@claude
Copy link

claude bot commented Nov 20, 2025

Code Review: feat(cloudflare-workers): add createInlineClient

Summary

This PR adds a new createInlineClient function that allows developers to access Rivet Actors privately within Cloudflare Workers without exposing a public manager API. The change refactors the existing createHandler to use the new createInlineClient internally, providing better composability.


Strengths

  1. Good API Design: The separation between createInlineClient and createHandler is clean and follows the single responsibility principle
  2. Excellent Documentation: JSDoc comments clearly explain when to use each function
  3. Backward Compatibility: Existing createHandler usage remains unchanged
  4. Comprehensive Example: The new example demonstrates both HTTP and RivetKit client usage effectively
  5. Type Safety: Proper TypeScript generics maintain type safety

Issues & Concerns

1. Wrangler Configuration Issue (Critical)

File: examples/cloudflare-workers-inline-client/wrangler.json

The file is named wrangler.json but Cloudflare Workers expects wrangler.toml (TOML format) or wrangler.jsonc for JSON format.

Recommendation: Rename to wrangler.toml and convert to TOML format, or use wrangler.jsonc if JSON is preferred.

2. Debugging Code Left in Production (Minor)

File: examples/cloudflare-workers-inline-client/src/index.ts:36

console.log should be removed before merging.

3. Missing Error Handling (Moderate)

File: examples/cloudflare-workers-inline-client/src/index.ts:24-25

No error handling for actor operations. If the actor operation fails, it will return a 500 error without a meaningful message.

4. Type Safety Concern (Minor)

File: rivetkit-typescript/packages/cloudflare-workers/src/handler.ts:32

The ...args: any is too permissive. Should match ExportedHandlerFetchHandler signature.

5. Inconsistent Registry State (Minor)

File: examples/cloudflare-workers/src/registry.ts

The PR modifies the existing example registry state from having count, connectionCount, and messageCount to just count. This is unrelated to the PR purpose.


Security Considerations

  1. No Input Validation: The example extracts the actor name directly from the URL without validation
  2. No Rate Limiting: The example doesn't show rate limiting

Test Coverage

Missing: No tests added for the new createInlineClient function.

Recommendation: Add unit tests to verify createInlineClient returns correct client, fetch, config, and ActorHandler


Additional Recommendations

  1. README Clarity: The README states without exposing a public API but the example actually does expose /rivet publicly. Consider clarifying this.
  2. Example Client Consistency: Ensure examples demonstrate consistent patterns

Verdict

Overall Assessment: Good addition that improves API flexibility. The core implementation is solid, but the example needs polish.

Action Items:

  • Fix wrangler.json configuration (Critical)
  • Remove console.log debugging code (Minor)
  • Add error handling to example (Moderate)
  • Fix type signature for fetch parameter (Minor)
  • Add tests for createInlineClient (Recommended)
  • Clarify README about public/private API access (Optional)
  • Explain or revert registry state change (Clarification needed)

@claude
Copy link

claude bot commented Nov 20, 2025

PR Review: feat(cloudflare-workers): add createInlineClient

Summary

This PR introduces createInlineClient to enable private access to Rivet Actors in Cloudflare Workers without exposing a public manager API. The implementation refactors the existing createHandler to use createInlineClient internally, demonstrating good code reuse.


Strengths

  1. Good API Design: The new createInlineClient function provides a clean separation of concerns
  2. Code Reuse: Refactoring createHandler to use createInlineClient internally reduces duplication
  3. Comprehensive Example: Clear README with setup instructions and both HTTP and RivetKit client examples
  4. Type Safety: Proper TypeScript typing throughout
  5. Documentation: JSDoc comments clearly explain when to use each function

Issues & Recommendations

High Priority

  1. Debug Console.log (examples/cloudflare-workers-inline-client/src/index.ts:36) - Remove this debug statement before merging

  2. Missing Error Handling (examples/cloudflare-workers-inline-client/src/index.ts:24-25) - The example doesn't handle potential errors from actor operations

Medium Priority

  1. Type Safety (rivetkit-typescript/packages/cloudflare-workers/src/handler.ts:32) - Using any reduces type safety, consider proper Cloudflare Worker types

  2. Inconsistent Type Export (rivetkit-typescript/packages/cloudflare-workers/src/mod.ts:9-10) - HandlerOutput and InlineOutput should be exported as types

  3. Unrelated Change (examples/cloudflare-workers/src/registry.ts:4) - Removal of connectionCount and messageCount appears unrelated


Test Coverage

Missing:

  • No unit tests for createInlineClient
  • No integration tests verifying the inline client works
  • No regression tests for createHandler after refactoring

Security & Performance

✅ No security vulnerabilities identified
✅ No performance regressions expected


Overall Assessment

Status: Approve with minor changes requested

This is a solid feature addition. The API design is clean and the refactoring demonstrates good engineering practices. Addressing the debug log and type safety issues will bring this to production quality.

Recommended Actions:

  1. Remove debug console.log on line 36
  2. Fix type exports in mod.ts
  3. Add error handling to example
  4. Consider adding test coverage

@graphite-app
Copy link
Contributor

graphite-app bot commented Nov 21, 2025

Merge activity

  • Nov 21, 2:40 AM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Nov 21, 2:42 AM UTC: CI is running for this pull request on a draft pull request (#3515) due to your merge queue CI optimization settings.
  • Nov 21, 2:43 AM UTC: Merged by the Graphite merge queue via draft PR: #3515.

@graphite-app graphite-app bot closed this Nov 21, 2025
@graphite-app graphite-app bot deleted the 11-13-feat_cloudflare-workers_add_createinlineclient_ branch November 21, 2025 02:43
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.

2 participants