Skip to content

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@vercel
Copy link

vercel bot commented Oct 22, 2025

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

Project Deployment Preview Comments Updated (UTC)
rivet-inspector Error Error Oct 24, 2025 6:33am
2 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Oct 24, 2025 6:33am
rivet-site Ignored Ignored Preview Oct 24, 2025 6:33am

Copy link
Member Author

NathanFlurry commented Oct 22, 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.

@claude
Copy link

claude bot commented Oct 22, 2025

PR Review: chore: fix rivetkit release script

Summary

This PR adds a push trigger for the main branch to the pkg-pr-new.yaml workflow, allowing it to run on both pull requests and pushes to main.

Code Quality & Best Practices ✅

Positive:

  • Clean, minimal change with clear intent
  • Follows GitHub Actions syntax correctly
  • Maintains existing pull_request trigger while adding new functionality

Observations:

  • The PR title mentions "fix rivetkit release script" but the change affects the trigger conditions, not the script itself. Consider if the title accurately reflects the change (e.g., "chore: run pkg-pr-new on main branch pushes")

Potential Issues & Considerations 🔍

1. Purpose Clarity
The typical use case for pkg-pr-new is to publish preview packages for pull requests for testing before merge. Running this on push to main means:

  • It will publish packages from main branch commits
  • This could be useful for having latest main branch packages available for testing
  • However, if the goal is production releases, the existing release.yaml workflow (manual dispatch) might be more appropriate

Question: What is the intended use case for publishing from main? Is this for:

  • Creating preview packages from the latest main branch?
  • An intermediate step before formal releases?
  • Testing purposes?

2. Duplicate Runs
When a PR is merged to main, this workflow will run twice:

  • Once from the PR event (before merge)
  • Once from the push to main event (after merge)

This could lead to:

  • Unnecessary CI usage
  • Duplicate package publications
  • Potential confusion about which package version to use

Recommendation: Consider adding a condition to prevent duplicate runs, or clarify if running on both events is intentional:

on:
  pull_request:
  push:
    branches:
      - main
    # Optionally skip if this is a merge commit that already ran on PR

3. Resource Usage
The workflow publishes multiple packages (runner, runner-protocol, rivetkit packages, and examples). Running on every main branch push will increase CI usage. Ensure this aligns with CI budget expectations.

Security Considerations 🔒

  • No security concerns identified
  • The workflow uses pnpm dlx which fetches and runs pkg-pr-new on-demand (safe practice)
  • No secrets or credentials in the changed code

Testing Coverage 🧪

Recommended Testing:

  • Verify the workflow triggers correctly on push to main
  • Confirm packages are published with expected naming/versioning
  • Test that PR-based publishing still works as expected
  • Check for duplicate publications when PRs are merged

Performance Considerations ⚡

  • Minimal performance impact on the workflow itself
  • The frequency of runs will increase (every main branch push vs only PRs)
  • Consider if conditional execution might be beneficial

Additional Recommendations 💡

  1. Documentation: Consider adding a comment in the workflow file explaining why both triggers are needed:

    on:
      # Publish preview packages for PRs
      pull_request:
      # Publish packages from main for latest development builds
      push:
        branches:
          - main
  2. Naming Consideration: If this workflow now serves dual purposes (PR previews + main branch builds), consider renaming it to reflect that (e.g., pkg-preview-publish.yaml)

  3. Version Tagging: Ensure pkg-pr-new handles versioning correctly for both PR and main branch contexts so consumers can distinguish between PR previews and main branch builds

Conclusion

The change is technically sound and introduces no bugs or security issues. However, I recommend:

  1. Clarifying the intended use case in the PR description
  2. Considering whether duplicate runs on PR merge are acceptable
  3. Adding inline documentation to explain both triggers
  4. Verifying CI budget impact is acceptable

Overall: Approve with suggestions

The code is safe to merge, but the recommendations above would improve clarity and prevent potential confusion for future maintainers.

@claude
Copy link

claude bot commented Oct 22, 2025

PR Review: Fix rivetkit release script

Summary

This PR addresses path corrections and cleanup following a directory restructure where rivetkit packages were moved from packages/rivetkit/ to rivetkit-typescript/packages/. Overall, the changes look good and are straightforward fixes. ✅


Code Quality & Best Practices

✅ Positive aspects:

  1. Correct path fix in release script (scripts/rivetkit/release.ts:351) - The path correction from packages/rivetkit/package.json to rivetkit-typescript/packages/rivetkit/package.json is necessary and correct
  2. TypeScript strictness improvements - Removing @ts-expect-error comments shows improved type safety in the codebase
  3. Dependency pinning fix - Changing hono from pinned 4.8.3 to flexible ^4.7.0 is more maintainable
  4. Path corrections in examples - Deno example paths updated correctly to match new structure

⚠️ Suggestions:

  1. GitHub Actions workflow (.github/workflows/pkg-pr-new.yaml:3-5): Adding push trigger on main branch

    • This seems intentional but consider: will this cause duplicate publishes if PRs are merged to main?
    • If this is for publishing to main after merge, consider documenting why this change was made in the PR description
    • Verify that pkg-pr-new handles main branch pushes appropriately
  2. Frontend package.json (frontend/package.json:12): Removing the error-throwing build script

    • Consider: Was this script being used anywhere in CI/CD pipelines?
    • If removed for convenience, ensure all CI scripts use specific build commands (build:engine, etc.)
    • Recommend keeping the helpful error message for developer experience

Potential Issues

⚠️ Minor concerns:

  1. Type safety regression in docker-compose.ts (lines 357-362):

    • While removing @ts-expect-error is good, ensure the types are actually correct now
    • The dynamic mutation of svc objects should ideally have proper types
    • Consider adding proper type guards or defining a more specific service type
  2. Web Worker runtime detection (engine/sdks/typescript/api-full/src/core/runtime/runtime.ts:77):

    • Removing @ts-expect-error for self?.importScripts is fine
    • The optional chaining already handles the case where self might not have importScripts
    • No issues here, good cleanup

Security Concerns

✅ No security issues identified. All changes are:

  • Path corrections for internal file references
  • Type annotation cleanups
  • Dependency version flexibility improvements
  • CI/CD configuration adjustments

Performance Considerations

✅ No performance impact. Changes are build-time and configuration updates only.


Test Coverage

⚠️ Recommendations:

  1. Verify the release script works with the corrected path:

    # Test that the version can be read correctly
    cat rivetkit-typescript/packages/rivetkit/package.json | jq -r '.version'
  2. Check that pkg-pr-new workflow works as expected:

    • Test on a branch to ensure packages are published correctly
    • Verify main branch publishing doesn't cause issues
  3. Validate frontend build commands still work after removing the generic build script:

    cd frontend
    pnpm build:engine
    pnpm build:inspector
    pnpm build:cloud

Additional Notes

Missing context:

  • The PR description is empty - it would be helpful to explain:
    • Why was rivetkit moved from packages/ to rivetkit-typescript/packages/?
    • What prompted these fixes?
    • Why was the pkg-pr-new workflow changed to run on main branch pushes?

Recommendation: Consider adding a PR description explaining the context and what was tested.


Verdict

✅ LGTM with minor suggestions. The changes are valid fixes for a directory restructure. The main concerns are:

  1. Verify the GitHub Actions workflow change is intentional and won't cause issues
  2. Consider keeping the helpful error message in frontend/package.json
  3. Test that the release script works with the new path

The code quality is good, no security concerns, and the changes align with the project's structure updates.

@pkg-pr-new
Copy link

pkg-pr-new bot commented Oct 22, 2025

More templates

@rivetkit/actor

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

@rivetkit/cloudflare-workers

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

@rivetkit/core

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

@rivetkit/engine-runner

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

@rivetkit/engine-runner-protocol

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

commit: f28b056

@claude
Copy link

claude bot commented Oct 22, 2025

Pull Request Review

This PR fixes the rivetkit release script path after the rivetkit packages were moved from packages/ to rivetkit-typescript/packages/. Overall, the changes look good and address the core issue.

Positive Observations

  1. Correct Path Fix: The main fix in scripts/rivetkit/release.ts:351 correctly updates the path from packages/rivetkit/package.json to rivetkit-typescript/packages/rivetkit/package.json, which aligns with the repository structure.

  2. Consistent Changes: The Deno example imports were also updated to reflect the new path structure, maintaining consistency across the codebase.

  3. Hono Version Normalization: Changed from a pinned version (4.8.3) to a semver range (^4.7.0), which is more flexible and aligns with dependency management best practices.

  4. TypeScript Cleanup: Removed unnecessary @ts-expect-error comments where TypeScript can now properly infer types.

  5. Frontend Build Script Cleanup: Removed the error-throwing build script in frontend/package.json which was likely causing issues with workspace-level builds.

  6. CI/CD Enhancement: Added push trigger for main branch and scoped the build to only rivetkit packages, which should improve build performance.

Potential Issues and Recommendations

  1. Hono Version Downgrade Concern (Medium Priority): The change from hono@4.8.3 to ^4.7.0 (with lockfile showing 4.9.8) appears inconsistent. If the intention is to use the latest compatible version, consider using ^4.9.8 or ^4.8.0 in package.json to better reflect what is actually being used.

  2. TypeScript Type Safety Concerns (Low-Medium Priority): The removal of @ts-expect-error comments in three locations should be verified. Run pnpm check-types to ensure no type errors were introduced.

  3. CI Workflow Scope (Low Priority): The build command now uses -F '@rivetkit/*' which builds only rivetkit packages. Does this workflow need to build engine/sdks/typescript/runner/ and engine/sdks/typescript/runner-protocol/ packages that are also being published?

Testing Recommendations

  1. Run the release script in a test environment to ensure it correctly reads the version
  2. Verify the GitHub Actions workflow builds successfully on both PRs and main branch pushes
  3. Confirm TypeScript type checking passes: pnpm check-types
  4. Test the Deno example with the updated import paths

Summary

This is a solid maintenance PR that addresses the path changes from the rivetkit reorganization. The main concern is the Hono version change which should be verified for intentionality.

Recommendation: Approve with minor verification suggested for the Hono version and TypeScript types.

@claude
Copy link

claude bot commented Oct 22, 2025

PR Review: chore: fix rivetkit release script

Overall Assessment

This PR addresses several issues related to the RivetKit release script and TypeScript configuration. The changes are generally well-structured and address real problems with package paths and type safety.

Code Quality and Best Practices

Strengths:

  1. Correct path fixes - The release script now properly references rivetkit-typescript/packages/rivetkit/package.json (line 351)
  2. Type safety improvements - Adding explicit type annotations in raw-websocket-handler-proxy/src/backend/server.ts
  3. Proper null checking - Added validation for actor ID in actor_stress.ts:23-25 to prevent undefined values
  4. Removed unnecessary ts-expect-error comments - Suggests the underlying type issues were resolved
  5. Consistent tsconfig paths - Fixed relative paths in tsconfig files

Areas for Improvement:

  1. GitHub Actions workflow change - Adding push trigger on main branch will cause this workflow to run on every main branch push. Consider if this is intended behavior.

  2. Build filter change - Changed from pnpm build to pnpm build -F @rivetkit/* which filters builds to only rivetkit packages. Ensure this does not skip building dependencies.

  3. Type assertion in error handling - Using as any defeats the purpose of TypeScript type safety. Recommendation: Use error instanceof Error instead.

  4. Missing dependency explanations - Several examples now require @hono/node-server and related packages.

Potential Bugs or Issues

  1. WebSocket type annotations - Ensure WSContext from hono/ws matches the actual runtime type
  2. Frontend build script - Removed the generic build script that exits with an error. Are there any CI/CD pipelines that might be affected?

Performance Considerations

No significant performance impacts identified. The build filter change in the GitHub Actions workflow should improve CI performance.

Security Concerns

No security issues identified. The changes are primarily configuration and type safety improvements.

Test Coverage

Missing test coverage for the release script path changes. Existing tests for actor_e2e.ts and actor_stress.ts improve error handling.

Recommendations Summary

High Priority:

  1. Replace as any type assertions with proper type guards
  2. Document the reason for adding push trigger on main branch

Medium Priority:
3. Verify that pnpm build -F @rivetkit/* builds all necessary dependencies
4. Confirm that removing the frontend build script does not break any existing automation

Questions for Author

  1. What prompted the change to trigger pkg-pr-new on main branch pushes?
  2. Were the @hono/node-server dependencies previously missing?
  3. Have you tested the release script with these path changes in a dry-run environment?

Overall, this is a solid cleanup PR that fixes real issues. The main concerns are around the as any type assertions and ensuring the workflow trigger changes are intentional.

@claude
Copy link

claude bot commented Oct 24, 2025

Code Review - PR #3242: Fix rivetkit release script

Summary

This PR fixes issues with the rivetkit release script and makes several related improvements across the monorepo. The changes primarily focus on correcting file paths after a directory restructuring and adding missing dependencies to example projects.

Positive Aspects ✅

  1. Path Corrections: The changes correctly update paths to reflect the rivetkit-typescript/ directory structure
  2. Dependency Fixes: Adding @hono/node-server as an explicit dependency to examples is the right approach - the examples were likely relying on transitive dependencies which is fragile
  3. TypeScript Improvements: Removing @ts-expect-error directives in docker-compose.ts by properly typing the dynamic mutations is cleaner
  4. CI/CD Enhancement: Adding push trigger on main branch to pkg-pr-new.yaml enables automatic publishing on merge
  5. Error Handling: Better error handling in test scripts

Issues & Concerns 🔍

Critical

1. GitHub Workflow Build Command Scope (/.github/workflows/pkg-pr-new.yaml:15)

  • The build command changed from pnpm build to pnpm build -F @rivetkit/*
  • This only builds @rivetkit/* packages but the publish command includes engine/sdks/typescript/runner/ and engine/sdks/typescript/runner-protocol/
  • Risk: The runner packages may not be built before publishing
  • Recommendation: Either include these in the filter or verify they do not need building

Medium

2. Frontend Package.json Build Script Removal (/frontend/package.json:12)

  • Removed the error-throwing build script that guided users to specific build commands
  • Risk: Users running pnpm build in frontend will now silently do nothing instead of getting helpful guidance
  • Recommendation: Keep the error message or document the change in commit message

3. TypeScript Config Path Changes (/scripts/release/tsconfig.json:2, /scripts/tests/tsconfig.json:2)

  • Changed extends path from ../../../tsconfig.base.json to ../../tsconfig.base.json
  • This suggests a directory restructuring, but only scripts configs are changed
  • Question: Are there other tsconfig files that need similar updates?

4. Missing Null Check (/scripts/tests/actor_stress.ts:22-24)

  • Good addition of null check for actorId, but the error message could be more helpful
  • Recommendation: Include context like response status or error details in the error message

Minor

5. Type Safety in WebSocket Error Handling (/scripts/tests/actor_e2e.ts:124, /scripts/tests/spam_actors.ts:136)

  • Using as any defeats TypeScript type safety
  • Recommendation: Use proper type guard instead of as any

6. Missing Dependencies Documentation

  • Multiple examples now require @hono/node-server but there is no explanation why
  • Was this previously a peer dependency? Did Hono export structure change?

Performance Considerations

No significant performance concerns. The changes are primarily configuration and dependency updates.

Security Considerations

No security issues identified. The changes do not introduce new attack vectors.

Testing Recommendations

  1. Test the full release workflow to ensure all necessary packages are built before publishing
  2. Verify examples work correctly with new explicit dependencies
  3. Test the docker template generation still works

Style & Convention Adherence

  • ✅ Follows repository conventions for dependency management
  • ✅ Uses workspace dependencies appropriately
  • ⚠️ TypeScript strict mode could be better utilized (avoid as any)

Verdict

Approve with minor suggestions - The core fixes are sound and necessary. The critical concern about the build filter should be verified, but it may be intentional if those packages do not require building. The other issues are minor improvements that can be addressed in follow-up PRs.


Review generated with Claude Code

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.

1 participant