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-3789 environment in rivet-frontend

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

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

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

@railway-app railway-app bot temporarily deployed to rivet-frontend / rivet-pr-3789 January 8, 2026 00:22 Destroyed
@railway-app railway-app bot temporarily deployed to rivet-frontend-website-preview / rivet-pr-3789 January 8, 2026 00:22 Destroyed
Copy link
Member Author

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

Pull Request Review

Summary

This PR refactors the serverless configuration logic by renaming advertiseEndpoint to clientEndpoint and consolidating the default logic for serveManager and clientEndpoint. The changes improve code clarity and reduce duplication.

Code Quality ✅

Strengths:

  • Excellent refactoring: The consolidation of duplicate logic between serverless and runner configurations is a significant improvement
  • Clear naming: clientEndpoint is more descriptive than advertiseEndpoint for indicating the endpoint clients should connect to
  • Improved documentation: Comments in lines 188-191 clearly explain the logic flow
  • Consistent error handling: The validation logic is properly maintained with updated error messages

Code organization:

  • The transform function is now more readable with the unified logic path
  • Proper use of TypeScript with type safety maintained throughout

Potential Issues 🔍

1. Breaking Change - Documentation Required

This is a breaking change for any users currently using advertiseEndpoint in their configuration. Consider:

  • Adding a migration guide in the PR description
  • Documenting this in release notes/changelog
  • Potentially providing a deprecation warning period rather than immediate removal

2. Inconsistent Validation Pattern (rivetkit-typescript/packages/rivetkit/src/registry/config/index.ts:214-218)

The code uses both Zod validation and runtime invariant for the same condition:

  • Lines 163-175: Zod validation checks !config.serverless.clientEndpoint
  • Lines 214-218: Runtime invariant checks the same condition

Recommendation: Remove the redundant invariant since Zod validation will catch this during config parsing. If the invariant is intentionally defensive, add a comment explaining why both checks are needed.

// Lines 214-218 - Consider removing if Zod validation is sufficient
invariant(
    config.serverless.clientEndpoint,
    "clientEndpoint is required in production mode without endpoint",
);

3. Type Safety Consideration (rivetkit-typescript/packages/rivetkit/src/registry/config/index.ts:193)

The variable clientEndpoint is declared without explicit typing:

let clientEndpoint: string;

While TypeScript can infer the type, in this case all branches assign a string value so this is safe. However, the production branch (lines 214-218) relies on the invariant to ensure non-null. If you remove the invariant as suggested above, consider using TypeScript's non-null assertion or refactoring to make the type safety explicit.

Performance Considerations ⚡

No performance concerns. The refactoring simplifies the logic path which should have neutral or slightly positive performance impact.

Security Concerns 🔒

No security issues identified. The endpoint validation and token handling remain unchanged.

Test Coverage 📋

Concern: I couldn't locate tests specifically covering the configuration transform logic.

Recommendations:

  1. Add unit tests for the RegistryConfigSchema transform function covering:

    • Development mode without endpoint (should set serveManager = true, clientEndpoint = localhost)
    • Production mode without endpoint (should fail if clientEndpoint not provided)
    • Endpoint provided (should use endpoint, serveManager = false)
    • Override cases where serveManager or clientEndpoint are explicitly set
  2. Add integration tests verifying the serverless router receives the correct clientEndpoint value

  3. Test the validation error messages to ensure they're clear and accurate

Additional Notes 📝

Display Logic Enhancement (rivetkit-typescript/packages/rivetkit/src/registry/index.ts:247):
The updated display logic uses a fallback pattern:

config.endpoint ?? config.serverless.clientEndpoint

This is correct, but consider whether displaying the endpoint is still meaningful in all cases. The condition on line 243 checks config.serveManager but the displayed value might be from config.endpoint which contradicts the serveManager state.

Overall Assessment ⭐

Score: 8/10

This is a solid refactoring that improves code quality and maintainability. The main concerns are:

  1. Breaking change requires migration documentation
  2. Redundant validation should be cleaned up
  3. Test coverage should be added

Recommendation: Approve with suggested changes

Action Items

  • Add migration guide for advertiseEndpointclientEndpoint rename
  • Consider removing redundant invariant check (lines 214-218)
  • Add unit tests for configuration transform logic
  • Update any documentation referencing advertiseEndpoint

Review generated by Claude Code

@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@3789

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

@rivetkit/virtual-websocket

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

@rivetkit/engine-runner

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

@rivetkit/engine-runner-protocol

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

commit: b90420d

@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_fix_default_logic_for_servemanager_advertiseendpoint 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