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

Conversation

@NathanFlurry
Copy link
Member

No description provided.

Copy link
Member Author

NathanFlurry commented Aug 9, 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 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
Copy link

claude bot commented Aug 9, 2025

Claude finished @NathanFlurry's task —— View job


Code Review Complete ✅

Overview

This PR implements a parameter reordering change for consistency across the RivetKit core API. The main change involves switching the order of parameters in onAuth, createConnState, and onBeforeConnect functions to make opts the first parameter and params the second parameter.

Changes Summary

  • Files modified: 15 files across core, examples, and test fixtures
  • Main change: Parameter order changed from (params, opts) to (opts, params)
  • Documentation: Updated CONFIG_TYPES.md with rationale for the change
  • Breaking change: This is a breaking API change requiring user code updates

Code Quality Assessment

Strengths

  1. Consistent API Design: The parameter reordering creates consistency with TypeScript conventions where optional parameters come last
  2. Improved Developer Experience: Users can now omit params when they only need opts, following the principle that required parameters come first
  3. Comprehensive Updates: All affected files (core, examples, tests) were updated consistently
  4. Good Documentation: The rationale is clearly explained in internal-docs/CONFIG_TYPES.md:7-20

⚠️ Areas for Consideration

Breaking Change Impact

  • This is a significant breaking change that will require all users to update their actor configurations
  • Consider providing migration tooling or detailed migration guides
  • The change affects core lifecycle hooks that are commonly used

Documentation Updates

  • The change removes a detailed security section from examples/tenant/README.md:73-95 - consider if this documentation should be preserved elsewhere
  • Error message improvement in packages/core/src/actor/errors.ts:184 removes "CBOR serializable" context which was informative

Security Assessment ✅

No security concerns identified. The parameter reordering:

  • Maintains the same authentication and authorization logic
  • Preserves type safety through proper TypeScript interfaces
  • Doesn't expose additional attack surfaces

Performance Assessment ✅

No performance impact. This is purely a parameter ordering change that:

  • Doesn't affect runtime performance
  • Maintains the same function call overhead
  • No changes to algorithmic complexity

Type Safety & Testing ✅

Strong type safety maintained:

  • All function signatures properly updated with correct TypeScript types
  • Test fixtures comprehensively updated to match new parameter order
  • Examples demonstrate correct usage patterns

Test Coverage:

  • All driver test suite files updated consistently
  • Examples serve as additional integration tests
  • The changes maintain backward compatibility in terms of functionality

Specific Code Improvements

Core Config Changes (packages/core/src/actor/config.ts:158-226)

// Before
onAuth: (params: TConnParams, opts: OnAuthOptions) => TAuthData

// After  
onAuth: (opts: OnAuthOptions, params: TConnParams) => TAuthData

Good: This aligns with the documented principle that required parameters come first.

Implementation Consistency

All lifecycle hooks now follow the same pattern:

  • onAuth(opts, params)
  • createConnState(c, opts, params)
  • onBeforeConnect(c, opts, params)

Recommendations

  1. Migration Guide: Provide clear migration instructions with before/after examples
  2. Version Planning: Consider this for a major version bump due to breaking nature
  3. Tooling: Consider providing a codemod or automatic migration tool
  4. Documentation: Ensure all external documentation reflects the new parameter order

Summary

This is a well-executed refactoring that improves API consistency and developer ergonomics. The implementation is comprehensive and maintains type safety throughout. While it's a breaking change, the benefits to API design justify the update.

Recommendation: ✅ Approve - The changes are technically sound and improve the overall API design.


@pkg-pr-new
Copy link

pkg-pr-new bot commented Aug 9, 2025

More templates

@rivetkit/actor

pnpm add https://pkg.pr.new/rivet-gg/rivetkit/@rivetkit/actor@1164

@rivetkit/core

pnpm add https://pkg.pr.new/rivet-gg/rivetkit/@rivetkit/core@1164

@rivetkit/db

pnpm add https://pkg.pr.new/rivet-gg/rivetkit/@rivetkit/db@1164

rivetkit

pnpm add https://pkg.pr.new/rivet-gg/rivetkit@1164

@rivetkit/cloudflare-workers

pnpm add https://pkg.pr.new/rivet-gg/rivetkit/@rivetkit/cloudflare-workers@1164

@rivetkit/redis

pnpm add https://pkg.pr.new/rivet-gg/rivetkit/@rivetkit/redis@1164

@rivetkit/framework-base

pnpm add https://pkg.pr.new/rivet-gg/rivetkit/@rivetkit/framework-base@1164

@rivetkit/next-js

pnpm add https://pkg.pr.new/rivet-gg/rivetkit/@rivetkit/next-js@1164

@rivetkit/react

pnpm add https://pkg.pr.new/rivet-gg/rivetkit/@rivetkit/react@1164

commit: b1e4e4f

@graphite-app
Copy link

graphite-app bot commented Aug 9, 2025

Merge activity

  • Aug 9, 12:53 AM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Aug 9, 12:54 AM UTC: CI is running for this pull request on a draft pull request (#1167) due to your merge queue CI optimization settings.
  • Aug 9, 12:55 AM UTC: Merged by the Graphite merge queue via draft PR: #1167.

graphite-app bot pushed a commit that referenced this pull request Aug 9, 2025
@graphite-app graphite-app bot closed this Aug 9, 2025
@graphite-app graphite-app bot deleted the 08-08-chore_core_reorder_param_inputs branch August 9, 2025 00:55
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.

2 participants