Skip to content

Conversation

@abcxff
Copy link
Contributor

@abcxff abcxff commented Dec 12, 2025

Fixes CLD-37

@vercel
Copy link

vercel bot commented Dec 12, 2025

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

Project Deployment Preview Comments Updated (UTC)
rivetkit-serverless Error Error Dec 12, 2025 3:06am
3 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Dec 12, 2025 3:06am
rivet-inspector Ignored Ignored Preview Dec 12, 2025 3:06am
rivet-site Ignored Ignored Preview Dec 12, 2025 3:06am

Copy link
Contributor Author

abcxff commented Dec 12, 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.

@abcxff abcxff marked this pull request as ready for review December 12, 2025 03:05
@linear
Copy link

linear bot commented Dec 12, 2025

@claude
Copy link

claude bot commented Dec 12, 2025

Code Review

Thank you for this PR! I've reviewed the changes and found several issues that need to be addressed before merging.

Critical Issues

1. Missing Error Handling for Invalid Port Values

Location: rivetkit-typescript/packages/rivetkit/src/registry/run-config.ts:38-42

The current implementation uses parseInt() without proper error handling:

const defaultPort = getEnvUniversal("RIVET_SERVER_PORT");
if (typeof defaultPort === "undefined") {
    return 6420;
}
return parseInt(defaultPort);

Problems:

  • parseInt() without a radix parameter can produce unexpected results
  • No validation for invalid values (e.g., parseInt("abc") returns NaN)
  • No error message for users when they provide invalid values

Recommended Fix:
Add radix parameter, validate port range (1-65535), and throw clear errors for invalid values.

2. Inconsistent Pattern with Other Environment Variables

Looking at similar environment variable handling in the codebase:

  • client/config.ts uses .transform() for env vars
  • drivers/engine/config.ts uses .transform() for env vars

Your PR uses .default(() => {...}) which is a different pattern. Consider using the .transform() pattern to match existing codebase style.

Minor Issues

3. Missing Test Coverage

There are no tests for this new environment variable functionality. Consider adding tests to verify:

  • Default port (6420) is used when env var is not set
  • Custom port is used when RIVET_SERVER_PORT is set
  • Invalid values are rejected with clear error messages
  • Boundary values (1, 65535) work correctly

Summary

Must Fix:

  • Add radix parameter to parseInt() (use parseInt(envPort, 10))
  • Add validation for port range (1-65535)
  • Add error handling for NaN and out-of-range values

Should Consider:

  • Match the .transform() pattern used elsewhere
  • Add test coverage
  • Document the new environment variable

@pkg-pr-new
Copy link

pkg-pr-new bot commented Dec 12, 2025

More templates

@rivetkit/cloudflare-workers

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

@rivetkit/engine-runner

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

@rivetkit/engine-runner-protocol

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

commit: b2a4261

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