Skip to content

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@vercel
Copy link

vercel bot commented Dec 6, 2025

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

Project Deployment Preview Comments Updated (UTC)
rivet-inspector Ready Ready Preview Comment Dec 6, 2025 8:43am
rivet-site Ready Ready Preview Comment Dec 6, 2025 8:43am
rivetkit-serverless Error Error Dec 6, 2025 8:43am
1 Skipped Deployment
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Dec 6, 2025 8:43am

Copy link
Member Author

NathanFlurry commented Dec 6, 2025

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.

@pkg-pr-new
Copy link

pkg-pr-new bot commented Dec 6, 2025

More templates

@rivetkit/cloudflare-workers

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

@rivetkit/engine-runner

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

@rivetkit/engine-runner-protocol

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

commit: 59d037d

@claude
Copy link

claude bot commented Dec 6, 2025

Pull Request Review: Auto-generate templates from examples directory

Summary

This PR automates the generation of template metadata from the examples directory, introducing validation, screenshot generation, and improved consistency. The changes reorganize the build system into a modular structure.


✅ Strengths

  1. Good Modularization: The refactoring from a single build.ts to build/index.ts, build/validate.ts, and build/screenshots.ts improves maintainability and separation of concerns.

  2. Validation Improvements:

    • The addition of validateRivetKitVersions() ensures consistency across examples by requiring "latest" version for all RivetKit packages
    • validateReadmeFormat() enforces standard README structure
  3. Better Error Handling: Changed from warnings (console.warn) to errors (console.error + throw) for critical failures, which is more appropriate for build-time validation.

  4. Automated Screenshot Generation: Nice DX improvement for maintaining example screenshots automatically.


🐛 Potential Issues

1. Resource Cleanup in Screenshot Generation (screenshots.ts:114-124)

The process cleanup logic has a potential race condition:

  • devProcess.killed indicates whether .kill() was called, NOT whether the process actually terminated
  • The 1-second delay is arbitrary and may not be sufficient for graceful shutdown
  • Port conflicts could occur if processes don't clean up before the next screenshot attempt

Recommendation: Wait for actual process exit event instead of relying on fixed delays.

2. Port Conflicts (screenshots.ts:55, 64-67)

The code assumes http://localhost:3000 as default but tries to detect the actual URL from output. However:

  • Multiple examples might use the same default port
  • If URL detection fails, it uses port 3000 regardless
  • No validation that the detected URL is actually accessible

Recommendation: Consider using dynamic port allocation or ensuring each example uses a unique port.

3. Hardcoded Wait Times (screenshots.ts:9, 87, 104)

Total wait time is 5+ seconds per screenshot with hardcoded delays. Complex apps may need more time; simple apps waste time.

Recommendation: Use more intelligent waiting strategies like custom "ready" markers in examples.

4. Memory Leak Risk (screenshots.ts:70-71)

Event listeners on stdout and stderr are never removed.

Recommendation: Remove event listeners in the finally block.


⚠️ Code Quality Concerns

1. Screenshot Generation Always Runs (index.ts:131)

Screenshots are generated every time the build runs (only if they don't exist). This means:

  • First run: all screenshots generated (could take a LONG time with 50+ examples)
  • No way to regenerate screenshots without manual deletion

Recommendation: Add a command-line flag to control screenshot generation.

2. Generic Package.json Change

The change to ai-and-user-generated-actors-freestyle/package.json updates @rivetkit/engine-api-full from a specific version to "latest", but @rivet-gg/cloud uses a PR-specific version. This seems inconsistent.

Recommendation: Document why some packages use PR-specific versions.


🔒 Security Considerations

1. Command Injection Risk (screenshots.ts:49)

Setting shell: true is unnecessary when using the array form of arguments and introduces risk.

Recommendation: Use shell: false.

2. Puppeteer Security Flags (screenshots.ts:92)

Flags --no-sandbox and --disable-setuid-sandbox reduce security. Add a comment explaining why they're needed for CI environments.


🎯 Performance Considerations

  1. Sequential Screenshot Generation: With 50+ examples, this could take 5-10 minutes.

    • Recommendation: Consider parallel generation with concurrency limits
  2. No Caching Strategy: No cache invalidation when example code changes.

    • Recommendation: Add metadata file with screenshot timestamp and example hash

📝 Documentation & Testing

  1. Missing JSDoc: Public functions lack documentation
  2. No Tests: This build tooling has no test coverage
  3. README Templates: Generated READMEs for quickstart examples are very generic

Summary

This is a solid improvement that brings consistency and automation to template management. The main concerns are:

  1. Critical: Process cleanup logic needs improvement
  2. Important: Port conflict handling and resource management
  3. Nice-to-have: Performance optimizations for parallel screenshot generation

Recommendation: Approve with requested changes to address the process cleanup and port conflict issues.

Estimated Risk Level: Medium (build-time tooling, but resource leaks could cause CI failures)

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