Skip to content

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@vercel
Copy link

vercel bot commented Dec 2, 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 Dec 2, 2025 4:57am
3 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Dec 2, 2025 4:57am
rivet-inspector Ignored Ignored Preview Dec 2, 2025 4:57am
rivet-site Ignored Ignored Preview Dec 2, 2025 4:57am

@graphite-app
Copy link
Contributor

graphite-app bot commented Dec 2, 2025

Merge activity

  • Dec 2, 4:55 AM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Dec 2, 4:56 AM UTC: CI is running for this pull request on a draft pull request (#3566) due to your merge queue CI optimization settings.
  • Dec 2, 4:57 AM UTC: Merged by the Graphite merge queue via draft PR: #3566.

Copy link
Member Author


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 Dec 2, 2025

Code Review - PR #3565

Summary

This PR removes the npm package alias for on-change and replaces it with the proper scoped package name @rivetkit/on-change. This is a straightforward dependency management improvement.

Review

Code Quality & Best Practices

  • LGTM: The change correctly removes the alias syntax (npm:@rivetkit/on-change) in favor of the standard scoped package name
  • LGTM: All import statements have been updated consistently across the codebase
  • LGTM: The package.json change maintains the same version constraint (^6.0.2-rc.1)
  • LGTM: The pnpm-lock.yaml has been properly regenerated to reflect the new dependency structure

Potential Bugs or Issues

  • No issues found: The change is purely a refactoring of how the dependency is declared
  • The package resolution will remain identical since it's the same package, just without the alias
  • All files that import on-change have been updated:
    • rivetkit-typescript/packages/rivetkit/src/actor/conn/state-manager.ts
    • rivetkit-typescript/packages/rivetkit/src/actor/instance/state-manager.ts

Performance Considerations

  • No performance impact: This is a purely syntactic change that doesn't affect runtime behavior
  • The resolved package is identical, so bundle size and execution characteristics remain unchanged

Security Concerns

  • No security concerns: The change maintains the exact same package version
  • Using the proper scoped package name (@rivetkit/on-change) is actually cleaner and more standard than npm aliasing

⚠️ Test Coverage

  • Consider running tests: While this is a low-risk change, it would be prudent to run the test suite to ensure the refactored imports work correctly
  • Suggested commands from CLAUDE.md:
    • cargo test -p rivetkit (if applicable)
    • Or run the TypeScript tests for the rivetkit package

Recommendation

Approve with suggestion: This is a clean refactoring that improves code clarity. I recommend:

  1. Running the test suite to verify the import changes
  2. Merging once tests pass

The change follows TypeScript/npm best practices by avoiding package aliasing when the actual scoped package name is available.

@graphite-app graphite-app bot closed this Dec 2, 2025
@graphite-app graphite-app bot deleted the 12-01-chore_rivetkit_remove_aliased_import_for_rivetkit_on-change branch December 2, 2025 04:57
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