Skip to content

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@railway-app
Copy link

railway-app bot commented Jan 8, 2026

🚅 Deployed to the rivet-pr-3787 environment in rivet-frontend

Service Status Web Updated (UTC)
frontend-cloud-prod 😴 Sleeping (View Logs) Web Jan 8, 2026 at 12:23 am
frontend-cloud-staging 😴 Sleeping (View Logs) Web Jan 8, 2026 at 12:23 am
frontend-inspector 😴 Sleeping (View Logs) Web Jan 8, 2026 at 12:23 am
website ❌ Build Failed (View Logs) Web Jan 8, 2026 at 12:02 am

🚅 Deployed to the rivet-pr-3787 environment in rivet-frontend-website-preview

Service Status Web Updated (UTC)
frontend-cloud ❌ Build Failed (View Logs) Web Jan 8, 2026 at 12:02 am
website ❌ Build Failed (View Logs) Web Jan 8, 2026 at 12:02 am

Copy link
Member Author

NathanFlurry commented Jan 8, 2026

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 Jan 8, 2026

PR Review: fix(rivetkit): bind handler to Registry in serve

Summary

This PR fixes a critical bug where the this context was lost when the handler method was passed through serve(). The fix properly binds the method to the Registry instance.


Changes Overview

  • Line 95: Added .bind(this) to ensure proper this context when handler is called
  • Lines 242-248: Simplified conditional logic (removed unnecessary intermediate variable)

Code Quality ✅

Primary Fix (Line 95): The addition of .bind(this) is the correct solution. Without this binding, when registry.serve() is used like export default registry.serve(), the handler method loses its this context, which would cause runtime errors when trying to access this.#ensureServerlessInitialized() at line 82.

Consistent Pattern: This fix aligns with existing patterns in the codebase:

  • Line 111: fetch: router.fetch.bind(router) - binds to router
  • Line 338: fetch: this.handler.bind(this) - same pattern in legacy code path

Secondary Refactor (Lines 242-248): The removal of the shouldShowEndpoint variable simplifies the code. The condition now directly checks:

  • config.serveManager OR
  • (kind === 'serverless' AND config.serverless.spawnEngine)

This is cleaner since config.serverless.advertiseEndpoint is only used inside the conditional block anyway, so checking it beforehand was redundant.


Technical Analysis

Why This Bug Occurred:
When you do export default registry.serve(), the returned object contains a reference to the handler method, but that reference doesn't maintain the binding to the registry instance. When the serverless runtime (e.g., Cloudflare Workers, Vercel) calls fetch(request), this would be undefined (in strict mode) or the global object, causing a crash.

Impact:

  • Without this fix: Runtime error: TypeError: Cannot read private member #ensureServerlessInitialized from an object whose class did not declare it
  • With this fix: Proper binding ensures this refers to the Registry instance

Example Usage (from examples/chat-room/src/server.ts:4):

export default registry.serve();

This is the exact pattern that requires the fix.


Best Practices ✅

Method Binding Approach: The .bind() solution is appropriate here. Alternative approaches would be:

  1. Arrow function in constructor - more invasive, requires constructor changes
  2. Arrow property - changes the method signature, affects inheritance
  3. Current solution with .bind() - minimal, clear, and idiomatic ✅

Code Simplification: Removing the shouldShowEndpoint variable improves readability without changing behavior.


Performance ✅

The .bind() call has negligible performance impact - it's called once during initialization, not per-request.


Security ✅

No security concerns. This is a pure bug fix that improves reliability.


Testing Observations ⚠️

Current State: No test files found for the Registry class.

Recommendation: Consider adding a test case to prevent regression:

test('serve() returns bound handler', async () => {
  const registry = setup({ use: {} });
  const { fetch } = registry.serve();
  // Verify fetch can be called without the registry context
  const response = await fetch(new Request('http://localhost/health'));
  expect(response).toBeDefined();
});

Potential Issues

None identified. The changes are minimal, targeted, and correct.


Verdict: APPROVED

This is a well-targeted bug fix that solves a real production issue with minimal changes. Both changes are correct:

  1. Critical fix: The .bind(this) prevents runtime crashes in serverless deployments
  2. Code cleanup: Removing the intermediate variable improves code clarity

The fix follows existing patterns in the codebase and has no negative side effects.


Minor Suggestions (not blocking):

  • Add unit tests for the serve() method to prevent regression
  • Consider adding a test that verifies the handler works when destructured: const { fetch } = registry.serve()

@pkg-pr-new
Copy link

pkg-pr-new bot commented Jan 8, 2026

More templates

@rivetkit/cloudflare-workers

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

@rivetkit/virtual-websocket

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/virtual-websocket@3787

@rivetkit/engine-runner

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

@rivetkit/engine-runner-protocol

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

commit: b2b13ca

@NathanFlurry NathanFlurry force-pushed the 01-07-fix_rivetkit_bind_handler_to_registry_in_serve branch from 362f46b to b2b13ca Compare January 8, 2026 00:12
@railway-app railway-app bot temporarily deployed to rivet-frontend / rivet-pr-3787 January 8, 2026 00:12 Destroyed
@graphite-app
Copy link
Contributor

graphite-app bot commented Jan 8, 2026

Merge activity

  • Jan 8, 9:33 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Jan 8, 9:34 PM UTC: CI is running for this pull request on a draft pull request (#3795) due to your merge queue CI optimization settings.
  • Jan 8, 9:34 PM UTC: Merged by the Graphite merge queue via draft PR: #3795.

@graphite-app graphite-app bot closed this Jan 8, 2026
@graphite-app graphite-app bot deleted the 01-07-fix_rivetkit_bind_handler_to_registry_in_serve branch January 8, 2026 21:34
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