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 13, 2025 10:06pm
3 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Nov 13, 2025 10:06pm
rivet-inspector Ignored Ignored Preview Nov 13, 2025 10:06pm
rivet-site Ignored Ignored Preview Nov 13, 2025 10:06pm

Copy link
Member Author

NathanFlurry commented Nov 13, 2025


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: 22eaf35

@NathanFlurry NathanFlurry changed the base branch from 11-12-chore_rivetkit_fix_type_checks to graphite-base/3466 November 13, 2025 22:27
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