chore(core): add smoke test #1363

NathanFlurry · 2025-10-07T21:21:55Z

No description provided.

vercel · 2025-10-07T21:21:58Z

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

Project	Deployment	Preview	Comments	Updated (UTC)
rivetkit-serverless	Canceled			Oct 7, 2025 10:06pm

claude · 2025-10-07T21:22:09Z

Claude finished @NathanFlurry's task —— View job

Code Review for Smoke Test Addition

I've completed a thorough review of this PR that adds a smoke test example to RivetKit. Here's my comprehensive feedback:

Todo List

Read repository guidelines (CLAUDE.md)
Review all changed files for code quality and best practices
Check for potential bugs and issues
Evaluate performance considerations
Assess security concerns
Review test coverage and implementation
Provide final recommendations

✅ Positive Aspects

Excellent Adherence to Project Standards

Perfect example structure: Follows all CLAUDE.md guidelines for examples
Correct naming: Uses example-smoke-test pattern as required
Proper dependencies: Uses workspace:* for rivetkit dependency
Complete configuration: Includes all required files (turbo.json, tsconfig.json, etc.)

High Code Quality

TypeScript best practices: Strict mode enabled, proper typing throughout
Clean architecture: Well-separated concerns with logical file organization
Modern JavaScript: Uses ESNext features appropriately
Proper error handling: Comprehensive error collection and reporting

Robust Test Implementation

Configurable parameters: Environment variable support for test customization
Performance monitoring: Built-in timing and statistics collection
Progress tracking: Real-time progress reporting during execution
Comprehensive metrics: Average, median, min, max duration tracking

🐛 Issues Found

1. Import Path Issue (scripts/connect.ts:2)

import type { Registry } from "../src/registry";
//                           ^^^^^^^^^^^^^^
// Should be: "../src/server/registry"

The import path is incorrect - registry.ts is in src/server/ not src/.

2. Missing Dependency (package.json:17)

The pnpm-lock.yaml shows vitest was added but it's not declared in package.json. Either:

Add vitest to devDependencies if needed
Remove it from lock file if it's not used

3. Unused README Template (README.md:20)

The README uses generic npm commands instead of the pnpm commands used in the project:

npm install  # Should be: pnpm install
npm run dev  # Should be: pnpm run dev

🔧 Minor Improvements

1. Environment Variable Documentation

Consider documenting all environment variables in README.md:

RUN_DURATION
SPAWN_ACTOR_INTERVAL
TOTAL_ACTOR_COUNT (derived)

2. Error Type Safety (spawn-actor.ts:7)

export type SmokeTestError = {
    index: number;
    error: unknown;  // Could be more specific
};

3. Magic Numbers (index.ts:17)

PROGRESS_LOG_INTERVAL_MS = 250 could be configurable for different verbosity levels.

⚡ Performance Considerations

Efficient concurrency: Good use of Promise.all for actor spawning
Memory management: Proper disposal of actor connections
Reasonable defaults: 10ms intervals prevent overwhelming the system
Performance monitoring: Built-in timing helps identify bottlenecks

🔒 Security Assessment

No security concerns: Code doesn't expose sensitive data
Safe UUID usage: Uses Node.js crypto for test IDs
Local-only: Hardcoded localhost connection appropriate for testing

📊 Test Coverage

Excellent Coverage

Core functionality: Tests actor creation, increment, disposal
Error handling: Comprehensive error collection and reporting
Performance metrics: Duration tracking and statistics
Configuration: Multiple environment variable options

Potential Additions

Connection failure scenarios: Test server unavailability
Concurrent stress testing: Higher actor counts
Different actor types: Beyond just counter actors

🎯 Final Recommendations

Must Fix

Fix import path in scripts/connect.ts
Resolve vitest dependency issue

Should Fix

Update README to use pnpm commands
Add environment variable documentation

Consider

Make progress logging interval configurable
Add more specific error typing

Overall Assessment: This is a high-quality smoke test implementation that follows project conventions excellently. The few issues found are minor and easily fixable. The test provides valuable functionality for validating RivetKit performance and reliability.

NathanFlurry · 2025-10-07T21:22:18Z

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

chore(core): handle errors inside message handlers #1364
chore(core): add smoke test #1363 👈 (View in Graphite)
chore: fix serverless config #1361
chore(core): add x-rivet-endpoint header for serverless runners & cleanup severless runner setup #1359
fix(next): fix local dev #1352
chore(core): add support for auto-configuring serverless #1356 : 1 other dependent PR (#1357 )
chore(core): add build:fast script #1355
feat(core): support running engine #1353
fix(example-deno): fix check-types #1351
chore(core): simplify config #1350
main

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

merge queue - adds this PR to the back of the merge queue
hotfix - for urgent hot fixes, skip the queue and merge this PR next

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 · 2025-10-07T22:00:09Z

Claude finished @NathanFlurry's task —— View job

Code Review for Smoke Test Addition

I've completed a thorough review of this PR that adds a smoke test example to RivetKit. Here's my comprehensive feedback:

Todo List

Read repository guidelines (CLAUDE.md)
Review all changed files for code quality and best practices
Check for potential bugs and issues
Evaluate performance considerations
Assess security concerns
Review test coverage and implementation
Provide final recommendations

✅ Positive Aspects

Excellent Adherence to Project Standards

Perfect example structure: Follows all CLAUDE.md guidelines for examples
Correct naming: Uses example-smoke-test pattern as required
Proper dependencies: Uses workspace:* for rivetkit dependency
Complete configuration: Includes all required files (turbo.json, tsconfig.json, etc.)

High Code Quality

TypeScript best practices: Strict mode enabled, proper typing throughout
Clean architecture: Well-separated concerns with logical file organization
Modern JavaScript: Uses ESNext features appropriately
Proper error handling: Comprehensive error collection and reporting

Robust Test Implementation

Configurable parameters: Environment variable support for test customization
Performance monitoring: Built-in timing and statistics collection
Progress tracking: Real-time progress reporting during execution
Comprehensive metrics: Average, median, min, max duration tracking

🐛 Issues Found

1. Import Path Issue (scripts/connect.ts:2)

import type { Registry } from "../src/registry";
//                           ^^^^^^^^^^^^^^
// Should be: "../src/server/registry"

The import path is incorrect - registry.ts is in src/server/ not src/.

2. Missing Dependency (package.json:17)

The pnpm-lock.yaml shows vitest was added but it's not declared in package.json. Either:

Add vitest to devDependencies if needed
Remove it from lock file if it's not used

3. Unused README Template (README.md:20)

The README uses generic npm commands instead of the pnpm commands used in the project:

npm install  # Should be: pnpm install
npm run dev  # Should be: pnpm run dev

🔧 Minor Improvements

1. Environment Variable Documentation

Consider documenting all environment variables in README.md:

RUN_DURATION
SPAWN_ACTOR_INTERVAL
TOTAL_ACTOR_COUNT (derived)

2. Error Type Safety (spawn-actor.ts:7)

export type SmokeTestError = {
    index: number;
    error: unknown;  // Could be more specific
};

3. Magic Numbers (index.ts:17)

PROGRESS_LOG_INTERVAL_MS = 250 could be configurable for different verbosity levels.

⚡ Performance Considerations

Efficient concurrency: Good use of Promise.all for actor spawning
Memory management: Proper disposal of actor connections
Reasonable defaults: 10ms intervals prevent overwhelming the system
Performance monitoring: Built-in timing helps identify bottlenecks

🔒 Security Assessment

No security concerns: Code doesn't expose sensitive data
Safe UUID usage: Uses Node.js crypto for test IDs
Local-only: Hardcoded localhost connection appropriate for testing

📊 Test Coverage

Excellent Coverage

Core functionality: Tests actor creation, increment, disposal
Error handling: Comprehensive error collection and reporting
Performance metrics: Duration tracking and statistics
Configuration: Multiple environment variable options

Potential Additions

Connection failure scenarios: Test server unavailability
Concurrent stress testing: Higher actor counts
Different actor types: Beyond just counter actors

🎯 Final Recommendations

Must Fix

Fix import path in scripts/connect.ts
Resolve vitest dependency issue

Should Fix

Update README to use pnpm commands
Add environment variable documentation

Consider

Make progress logging interval configurable
Add more specific error typing

Overall Assessment: This is a high-quality smoke test implementation that follows project conventions excellently. The few issues found are minor and easily fixable. The test provides valuable functionality for validating RivetKit performance and reliability.

graphite-app · 2025-10-07T23:36:45Z

Merge activity

Oct 7, 11:36 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
Oct 7, 11:37 PM UTC: CI is running for this pull request on a draft pull request (#1365) due to your merge queue CI optimization settings.
Oct 7, 11:37 PM UTC: Merged by the Graphite merge queue via draft PR: #1365.

graphite-app · 2025-10-07T23:37:52Z

examples/smoke-test/src/smoke-test/index.ts

+const RUN_DURATION = parseEnvInt(process.env.RUN_DURATION, 10_000);
+const SPAWN_ACTOR_INTERVAL = parseEnvInt(process.env.SPAWN_ACTOR_INTERVAL, 10);
+const TOTAL_ACTOR_COUNT = Math.ceil(RUN_DURATION / SPAWN_ACTOR_INTERVAL);


There's a discrepancy between the README and implementation regarding configuration options. The README mentions TOTAL_ACTOR_COUNT as a configurable environment variable:

Set `TOTAL_ACTOR_COUNT` and `SPAWN_ACTOR_INTERVAL` environment variables to adjust the workload.

However, in the code, TOTAL_ACTOR_COUNT is calculated from RUN_DURATION and SPAWN_ACTOR_INTERVAL:

const TOTAL_ACTOR_COUNT = Math.ceil(RUN_DURATION / SPAWN_ACTOR_INTERVAL);

To resolve this inconsistency, either:

Update the code to accept TOTAL_ACTOR_COUNT directly as an environment variable, or

Update the README to mention RUN_DURATION instead of TOTAL_ACTOR_COUNT

This will ensure the documentation accurately reflects the available configuration options.

Suggested change

const RUN_DURATION = parseEnvInt(process.env.RUN_DURATION, 10_000);

const SPAWN_ACTOR_INTERVAL = parseEnvInt(process.env.SPAWN_ACTOR_INTERVAL, 10);

const TOTAL_ACTOR_COUNT = Math.ceil(RUN_DURATION / SPAWN_ACTOR_INTERVAL);

const RUN_DURATION = parseEnvInt(process.env.RUN_DURATION, 10_000);

const SPAWN_ACTOR_INTERVAL = parseEnvInt(process.env.SPAWN_ACTOR_INTERVAL, 10);

const TOTAL_ACTOR_COUNT = process.env.TOTAL_ACTOR_COUNT

? parseEnvInt(process.env.TOTAL_ACTOR_COUNT, Math.ceil(RUN_DURATION / SPAWN_ACTOR_INTERVAL))

: Math.ceil(RUN_DURATION / SPAWN_ACTOR_INTERVAL);

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

graphite-app · 2025-10-07T23:37:53Z

examples/smoke-test/package.json

+  "devDependencies": {
+    "rivetkit": "workspace:*",
+    "@types/node": "^22.13.9",
+    "tsx": "^3.12.7",
+    "typescript": "^5.7.3"
+  },


The vitest dependency appears in the pnpm-lock.yaml file but is missing from the package.json devDependencies section. To maintain consistency between the lock file and package manifest, please add "vitest": "^3.1.1" to the devDependencies object.

Suggested change

"devDependencies": {

"rivetkit": "workspace:*",

"@types/node": "^22.13.9",

"tsx": "^3.12.7",

"typescript": "^5.7.3"

},

"devDependencies": {

"rivetkit": "workspace:*",

"@types/node": "^22.13.9",

"tsx": "^3.12.7",

"typescript": "^5.7.3",

"vitest": "^3.1.1"

},

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

This was referenced Oct 7, 2025

fix(next): fix local dev #1352

Closed

chore(core): add x-rivet-endpoint header for serverless runners & cleanup severless runner setup #1359

Closed

NathanFlurry mentioned this pull request Oct 7, 2025

chore: fix serverless config #1361

Closed

chore(core): add smoke test

179226f

vercel bot had a problem deploying to Preview October 7, 2025 21:49 Failure

NathanFlurry force-pushed the 10-07-chore_core_add_smoke_test branch from 0e90cab to 179226f Compare October 7, 2025 21:59

vercel bot temporarily deployed to Preview October 7, 2025 22:06 Inactive

NathanFlurry marked this pull request as ready for review October 7, 2025 23:36

NathanFlurry mentioned this pull request Oct 7, 2025

chore(core): handle errors inside message handlers #1364

Closed

graphite-app bot pushed a commit that referenced this pull request Oct 7, 2025

chore(core): add smoke test (#1363)

c9b74f2

graphite-app bot closed this Oct 7, 2025

graphite-app bot deleted the 10-07-chore_core_add_smoke_test branch October 7, 2025 23:37

graphite-app bot reviewed Oct 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

chore(core): add smoke test #1363

chore(core): add smoke test #1363

Uh oh!

NathanFlurry commented Oct 7, 2025

Uh oh!

vercel bot commented Oct 7, 2025 •

edited

Loading

Uh oh!

claude bot commented Oct 7, 2025 •

edited

Loading

Uh oh!

NathanFlurry commented Oct 7, 2025 •

edited

Loading

Uh oh!

claude bot commented Oct 7, 2025 •

edited

Loading

Uh oh!

graphite-app bot commented Oct 7, 2025 •

edited

Loading

Uh oh!

graphite-app bot Oct 7, 2025

Uh oh!

graphite-app bot Oct 7, 2025

Uh oh!

Uh oh!

chore(core): add smoke test #1363

chore(core): add smoke test #1363

Uh oh!

Conversation

NathanFlurry commented Oct 7, 2025

Uh oh!

vercel bot commented Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

claude bot commented Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review for Smoke Test Addition

Todo List

✅ Positive Aspects

Excellent Adherence to Project Standards

High Code Quality

Robust Test Implementation

🐛 Issues Found

1. Import Path Issue (scripts/connect.ts:2)

2. Missing Dependency (package.json:17)

3. Unused README Template (README.md:20)

🔧 Minor Improvements

1. Environment Variable Documentation

2. Error Type Safety (spawn-actor.ts:7)

3. Magic Numbers (index.ts:17)

⚡ Performance Considerations

🔒 Security Assessment

📊 Test Coverage

Excellent Coverage

Potential Additions

🎯 Final Recommendations

Must Fix

Should Fix

Consider

Uh oh!

NathanFlurry commented Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How to use the Graphite Merge Queue

Uh oh!

claude bot commented Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review for Smoke Test Addition

Todo List

✅ Positive Aspects

Excellent Adherence to Project Standards

High Code Quality

Robust Test Implementation

🐛 Issues Found

1. Import Path Issue (scripts/connect.ts:2)

2. Missing Dependency (package.json:17)

3. Unused README Template (README.md:20)

🔧 Minor Improvements

1. Environment Variable Documentation

2. Error Type Safety (spawn-actor.ts:7)

3. Magic Numbers (index.ts:17)

⚡ Performance Considerations

🔒 Security Assessment

📊 Test Coverage

Excellent Coverage

Potential Additions

🎯 Final Recommendations

Must Fix

Should Fix

Consider

Uh oh!

graphite-app bot commented Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge activity

Uh oh!

graphite-app bot Oct 7, 2025

Choose a reason for hiding this comment

Uh oh!

graphite-app bot Oct 7, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vercel bot commented Oct 7, 2025 •

edited

Loading

claude bot commented Oct 7, 2025 •

edited

Loading

NathanFlurry commented Oct 7, 2025 •

edited

Loading

claude bot commented Oct 7, 2025 •

edited

Loading

graphite-app bot commented Oct 7, 2025 •

edited

Loading