Skip to content
This repository was archived by the owner on Oct 22, 2025. It is now read-only.

fix(examples): add sleeping to smoke test#1367

Closed
MasterPtato wants to merge 1 commit intomainfrom
10-08-fix_examples_add_sleeping_to_smoke_test
Closed

fix(examples): add sleeping to smoke test#1367
MasterPtato wants to merge 1 commit intomainfrom
10-08-fix_examples_add_sleeping_to_smoke_test

Conversation

@MasterPtato
Copy link
Contributor

No description provided.

@vercel
Copy link

vercel bot commented Oct 8, 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 Oct 8, 2025 7:15pm

@claude
Copy link

claude bot commented Oct 8, 2025

Claude finished @MasterPtato's task —— View job


Code Review Complete ✅

Reviewing PR that adds sleeping functionality to smoke test examples.

Todo List

  • Read and analyze changed files
  • Review code quality and best practices
  • Check for potential bugs or issues
  • Evaluate performance considerations
  • Check security concerns
  • Assess test coverage
  • Provide comprehensive feedback

Code Review Feedback

Summary

This PR adds actor sleep functionality testing to the smoke test example. The changes include configuring a short sleep timeout for the counter actor and implementing a test that verifies actors can be put to sleep and then reactivated.

✅ Positive Aspects

  1. Clear Intent: The changes properly test actor sleep functionality, which is an important RivetKit feature
  2. Proper Configuration: Uses the correct sleepTimeout option format as defined in the actor config schema
  3. Realistic Test: Tests a realistic scenario where an actor sleeps and then needs to be reactivated
  4. Clean Code: Follows existing code patterns and conventions

🔍 Technical Analysis

File: examples/smoke-test/src/server/registry.ts (lines 4-6)

options: {
    sleepTimeout: 500,
},
  • Sets a very short 500ms sleep timeout, appropriate for testing
  • Follows the correct schema format from packages/rivetkit/src/actor/config.ts:72

File: examples/smoke-test/src/smoke-test/spawn-actor.ts (lines 40-46)

// Wait for actor to sleep
await new Promise(res => setTimeout(res, 1000));

const counter2 = client.counter.getOrCreate(key).connect();
await counter2.increment(1);
await counter2.dispose();

⚠️ Areas for Improvement

  1. Timing Reliability: The fixed 1000ms wait might be flaky in CI environments. Consider:

    • Using a slightly longer wait (e.g., 1500ms) for more reliability
    • Or implementing a more robust check to verify the actor actually slept
  2. Magic Numbers: The sleep timeout (500ms) and wait time (1000ms) are magic numbers. Consider:

    • Adding constants at the top of files with descriptive names
    • Or using a calculated value based on the sleep timeout
  3. Test Documentation: The comment // Wait for actor to sleep could be more descriptive about why this sleep is necessary for the test

🚨 No Critical Issues Found

  • Security: No security concerns
  • Performance: Appropriate for test scenarios
  • Best Practices: Follows RivetKit patterns
  • Type Safety: Properly typed

💡 Suggestions

// Consider adding constants for better maintainability
const ACTOR_SLEEP_TIMEOUT = 500;
const SLEEP_WAIT_BUFFER = 1500; // 3x sleep timeout for reliability

// In registry.ts
options: {
    sleepTimeout: ACTOR_SLEEP_TIMEOUT,
},

// In spawn-actor.ts  
// Wait for actor to enter sleep state (sleepTimeout + buffer)
await new Promise(res => setTimeout(res, SLEEP_WAIT_BUFFER));

📋 Test Coverage Assessment

The changes appropriately test:

  • ✅ Actor sleep configuration
  • ✅ Actor reactivation after sleep
  • ✅ State persistence across sleep cycles
  • ✅ Multiple client interactions

Overall Assessment: This is a solid addition that properly tests an important RivetKit feature. The implementation is correct and follows project conventions. The suggested improvements are minor optimizations for reliability and maintainability.

Copy link
Contributor Author

MasterPtato commented Oct 8, 2025


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.

@pkg-pr-new
Copy link

pkg-pr-new bot commented Oct 8, 2025

More templates

@rivetkit/actor

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

@rivetkit/cloudflare-workers

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

@rivetkit/core

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

commit: ec4af1a

@graphite-app
Copy link

graphite-app bot commented Oct 9, 2025

Merge activity

  • Oct 9, 12:47 AM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Oct 9, 12:48 AM UTC: CI is running for this pull request on a draft pull request (#1370) due to your merge queue CI optimization settings.
  • Oct 9, 12:49 AM UTC: Merged by the Graphite merge queue via draft PR: #1370.

graphite-app bot pushed a commit that referenced this pull request Oct 9, 2025
@graphite-app graphite-app bot closed this Oct 9, 2025
@graphite-app graphite-app bot deleted the 10-08-fix_examples_add_sleeping_to_smoke_test branch October 9, 2025 00:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant