refactor(rivetkit): add state manager for conn with symbol-based access #3423

NathanFlurry · 2025-11-09T23:02:24Z

Created StateManager class for connection state management
Moved state proxying and change tracking to dedicated manager
Changed conn methods to use symbols for internal access:
- actor, stateEnabled, persistRaw (getters)
- hasChanges(), markSaved(), sendMessage() (methods)
Updated all references across codebase to use new symbols
Maintains backward compatibility for public API (send, disconnect, etc)
Consistent naming pattern with instance/mod.ts StateManager

🤖 Generated with Claude Code

Co-Authored-By: Claude noreply@anthropic.com

vercel · 2025-11-09T23:02:29Z

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

Project	Deployment	Preview	Comments	Updated (UTC)
rivetkit-serverless	Ready	Preview	Comment	Nov 13, 2025 8:55pm

3 Skipped Deployments

Project	Deployment	Preview	Updated (UTC)
rivet-cloud	Ignored		Nov 13, 2025 8:55pm
rivet-inspector	Ignored	Preview	Nov 13, 2025 8:55pm
rivet-site	Ignored	Preview	Nov 13, 2025 8:55pm

NathanFlurry · 2025-11-09T23:02:47Z

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 · 2025-11-09T23:04:41Z

More templates

@rivetkit/actor

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

@rivetkit/cloudflare-workers

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

@rivetkit/core

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

@rivetkit/engine-runner

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

@rivetkit/engine-runner-protocol

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

commit: d2d01ea

vercel · 2025-11-13T02:47:20Z

Deployment failed with the following error:

Too many requests - try again in 5 minutes (more than 120, code: "api-deployments-flood-pro").

Learn More: https://vercel.com/docs/limits#rate-limits

claude · 2025-11-13T03:06:53Z

Code Review: Refactor Connection State Management

This PR successfully extracts state management logic from the Conn class into a dedicated StateManager, following the same pattern used in ActorInstance. The refactoring improves code organization and encapsulation. Here's my detailed review:

✅ Strengths

Consistent Architecture Pattern: The new StateManager mirrors the pattern in instance/state-manager.ts, creating a cohesive architecture across the codebase.
Improved Separation of Concerns: Moving 130+ lines of state management logic out of Conn makes the class more focused on connection lifecycle rather than state tracking.
Symbol-based Access Control: Using symbols like CONN_ACTOR_SYMBOL, CONN_HAS_CHANGES_SYMBOL, etc. effectively creates internal APIs while maintaining backward compatibility for public methods (send, disconnect, state, params).
Clean Migration: All call sites have been updated consistently across 7 files, with no breaking changes to the public API.

🔍 Observations & Suggestions

1. Unused `changed` Getter

The StateManager exposes a changed getter (line 33 in state-manager.ts), but it's never used. Instead, hasChanges() method is called via symbols. Consider removing it:

// Line 33-35 - unused getter
get changed(): boolean {
    return this.#changed;
}

2. Inconsistent Access Patterns

The conn StateManager uses methods (hasChanges(), markSaved()) while the instance StateManager uses properties (persistChanged). For consistency, consider:

Either use properties for both (conn.#stateManager.changed instead of conn.#stateManager.hasChanges())
Or document why the approaches differ

3. Definite Assignment Assertions

Both #persist! and #persistRaw! use definite assignment assertions (line 15-16):

#persist!: PersistedConn<CP, CS>;
#persistRaw!: PersistedConn<CP, CS>;

While this works because initPersistProxy() is called in the constructor, it would be safer to:

Initialize these in the constructor parameters, or
Add a runtime check in accessors to throw if not initialized

4. Error Message Quality

Line 43 in state-manager.ts has a generic error message:

if (!this.#persist.state) throw new Error("state should exists");

Consider using a custom error from the errors module for better error handling and consistency.

5. Type Safety on Conn Generic

Line 12 uses any for several type parameters:

#conn: Conn<any, CP, CS, any, any, any>;

While this is pragmatic for avoiding circular type dependencies, consider documenting why this relaxation is acceptable or if it could be tightened.

6. Missing Documentation

The StateManager class has good JSDoc for the class itself, but internal methods like #handleChange and #validateStateEnabled could benefit from brief comments explaining their role.

🧪 Testing Considerations

Critical: This PR lacks test coverage for the refactored state management. Consider adding tests for:

State proxy behavior: Verify that changes to nested state properties trigger #handleChange
Change tracking: Confirm hasChanges() returns true after modifications and false after markSaved()
CBOR validation: Test that invalid state types throw InvalidStateType errors
State enabled validation: Ensure accessing state when disabled throws ConnStateNotEnabled
Actor notification: Verify that markConnChanged() is called on the actor when state changes

Based on CLAUDE.md, you could run tests with:

# If tests exist
cargo test -p rivetkit -- --nocapture

🔒 Security & Performance

Security: ✅ No security concerns. The symbol-based access pattern actually improves encapsulation.
Performance: ✅ No performance regression. The proxy pattern was already in use; this is just reorganization.
Memory: The additional StateManager instance per connection adds minimal overhead (~3 object properties).

📝 Minor Nits

Line 239 in conn/mod.ts: Extra blank line removed in the cloudflare-workers file is good cleanup
Import organization: The switch from destructured to type-only imports in cloudflare-workers is correct and improves tree-shaking

🎯 Verdict

Approval Recommended ✅

This is a solid refactoring that improves code organization without breaking changes. The implementation correctly follows established patterns in the codebase.

Before merging, please consider:

Adding test coverage for the state management behavior
Addressing the unused changed getter
Improving the error message on line 43

Nice work maintaining consistency with the instance StateManager pattern! 🚀

Review generated with assistance from Claude Code

claude · 2025-11-13T03:08:04Z

Code Review: refactor(rivetkit): add state manager for conn with symbol-based access

Summary

This PR successfully refactors connection state management by extracting state-related logic into a dedicated StateManager class and using symbols for internal access. The refactoring improves code organization and separation of concerns.

Positive Aspects

Better Separation of Concerns: Extracting state management logic into a dedicated StateManager class makes the Conn class more focused and easier to maintain.
Symbol-based Encapsulation: Using symbols (e.g., CONN_ACTOR_SYMBOL, CONN_SEND_MESSAGE_SYMBOL) provides proper encapsulation for internal APIs while maintaining clean external interfaces.
Consistent Pattern: This aligns with the existing StateManager pattern in instance/mod.ts as mentioned in the PR description.
Backward Compatibility: Public API methods (send, disconnect, state, params) remain unchanged, ensuring backward compatibility.

Issues and Concerns

1. Circular Dependency Risk (rivetkit-typescript/packages/rivetkit/src/actor/conn/state-manager.ts:5)

import { CONN_ACTOR_SYMBOL, CONN_STATE_ENABLED_SYMBOL, type Conn } from "./mod";

Issue: The StateManager imports from ./mod, which also imports StateManager. This creates a circular dependency.

Impact: While TypeScript handles this with the type import qualifier, it's a code smell that could lead to initialization issues.

Recommendation: Consider moving the symbols to a separate symbols.ts file that both modules can import.

2. Unused Property (rivetkit-typescript/packages/rivetkit/src/actor/conn/state-manager.ts:33-35)

get changed(): boolean {
    return this.#changed;
}

Issue: The changed getter is defined but never used. The public API uses hasChanges() method instead.

Recommendation: Remove the unused changed getter to avoid confusion.

3. Inconsistent Access Pattern (rivetkit-typescript/packages/rivetkit/src/actor/conn/mod.ts:145-147)

Issue: Some internal methods use symbol-based access as methods ([CONN_HAS_CHANGES_SYMBOL]()), while properties use symbol-based getters ([CONN_PERSIST_SYMBOL]). This inconsistency could be confusing.

Recommendation: Consider making access patterns more consistent - either all symbols are getters or all are methods, based on whether they represent data or behavior.

4. Error Message Quality (rivetkit-typescript/packages/rivetkit/src/actor/conn/state-manager.ts:43)

if (!this.#persist.state) throw new Error("state should exists");

Issues:

Grammatical error: "should exists" → "should exist"
Uses generic Error instead of the custom error system
Not very descriptive for debugging

Recommendation:

if (!this.#persist.state) {
    throw new errors.InternalError("Connection state should exist when state is enabled");
}

5. Test Coverage

Issue: No test files found for the connection state management functionality.

Recommendation: Add tests covering:

State change detection and tracking
CBOR serialization validation
State proxy behavior
Symbol-based access patterns
Error cases (state not enabled, invalid state types)

Performance Considerations

The refactoring appears performance-neutral:

The proxy setup is unchanged
Change tracking logic is the same
Symbol access has negligible overhead compared to direct property access

Security Concerns

Low Risk: The symbol-based approach actually improves encapsulation by making it harder to accidentally access internal APIs. The change tracking and CBOR validation remain intact.

Minor Issues

6. Import Formatting (rivetkit-typescript/packages/cloudflare-workers/src/actor-driver.ts:9-12)

The change from destructured import to type-only import is good, but the trailing comma removal on line 239 is unrelated formatting noise.

7. Documentation

The StateManager class has a good high-level comment, but individual methods lack JSDoc comments. Consider adding documentation for:

initPersistProxy() - explaining when it's called
hasChanges() / markSaved() - explaining the lifecycle

Recommendations Summary

High Priority:

Fix the circular dependency by extracting symbols to a separate file
Fix the error message grammar and use proper error class
Add test coverage for the new StateManager class

Medium Priority:
4. Remove unused changed getter
5. Add JSDoc comments to public StateManager methods
6. Consider making symbol-based access patterns more consistent

Low Priority:
7. Clean up unrelated formatting changes

Conclusion

This is a solid refactoring that improves code organization. The main concerns are the circular dependency and lack of test coverage. Once addressed, this will be a good improvement to the codebase.

Recommendation: Request changes for the circular dependency and error message issues, and strongly encourage adding tests before merging.

- Created StateManager class for connection state management - Moved state proxying and change tracking to dedicated manager - Changed conn methods to use symbols for internal access: - actor, stateEnabled, persistRaw (getters) - hasChanges(), markSaved(), sendMessage() (methods) - Updated all references across codebase to use new symbols - Maintains backward compatibility for public API (send, disconnect, etc) - Consistent naming pattern with instance/mod.ts StateManager 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>

claude · 2025-11-13T20:54:49Z

Code Review - PR #3423: Refactor Connection State Manager

Summary

This PR successfully extracts connection state management into a dedicated StateManager class and introduces symbol-based access patterns for internal methods. The refactoring improves code organization and consistency with the existing instance/StateManager pattern.

✅ Strengths

Well-Structured Separation of Concerns
- The StateManager class cleanly encapsulates state persistence, proxying, and change tracking
- Clear separation between public API and internal implementation
- Consistent with the existing instance/StateManager pattern
Symbol-Based Access Pattern
- Good use of symbols for internal access (e.g., CONN_ACTOR_SYMBOL, CONN_SEND_MESSAGE_SYMBOL)
- Maintains backward compatibility for public API methods
- Prevents accidental external access to internal methods
Code Reduction
- Net deletion of 124 lines vs 218 additions shows effective consolidation
- Moved 90+ lines of complex proxy logic into dedicated manager
Proper Error Handling
- State validation properly delegated to StateManager#validateStateEnabled()
- CBOR serializability checks preserved in StateManager#handleChange()

🔍 Issues & Concerns

1. Circular Dependency Risk (Minor)

Location: state-manager.ts:12

#conn: Conn<any, CP, CS, any, any, any>;

The StateManager holds a reference to Conn, which holds a reference to StateManager. While this works, it creates a circular dependency that could complicate garbage collection.

Recommendation: Consider passing only the necessary dependencies (actor instance, logging) rather than the entire Conn object. This would reduce coupling:

// Instead of storing the entire Conn
constructor(
  actor: ActorInstance<S, CP, CS, V, I, DB>,
  connId: ConnId,
  rLog: Logger
)

2. Missing Getter Usage (Minor)

Location: state-manager.ts:33

get changed(): boolean {
  return this.#changed;
}

This getter is defined but never used. The public API exposes hasChanges() method instead.

Recommendation: Either remove the unused getter or use it in hasChanges():

hasChanges(): boolean {
  return this.changed; // Use the getter
}

3. Inconsistent Null Handling (Low Priority)

Location: state-manager.ts:43

get state(): CS {
  this.#validateStateEnabled();
  if (!this.#persist.state) throw new Error("state should exists");
  return this.#persist.state;
}

The error message has a grammatical error ("should exists" → "should exist") and uses a generic Error instead of the custom error system.

Recommendation:

if (!this.#persist.state) {
  throw new errors.InvalidState({ 
    message: "Connection state should exist when state is enabled" 
  });
}

4. Missing Documentation (Minor)

Location: state-manager.ts:91-99

The hasChanges() and markSaved() methods have JSDoc comments, but the getters and constructor lack documentation.

Recommendation: Add JSDoc comments for:

Constructor parameters
persist, persistRaw, stateEnabled getters
state getter/setter edge cases

5. Type Safety Concern (Low Priority)

Location: conn/mod.ts:108

(this.#actor as any)[ACTOR_INSTANCE_PERSIST_SYMBOL]

The cast to any bypasses type safety when accessing actor persist data.

Recommendation: Consider adding proper type definitions for symbol-based properties or using a more type-safe approach.

🧪 Test Coverage

Concern: No new tests were added for the StateManager class.

Recommendation: Add unit tests covering:

State proxy initialization with different data types
Change detection and tracking
CBOR serialization validation
State enabled/disabled scenarios
Edge cases (null/undefined state, params)

Example test file structure:

// tests/conn-state-manager.test.ts
describe("ConnectionStateManager", () => {
  test("tracks changes when state is modified", ...);
  test("validates CBOR serializability", ...);
  test("throws error when state not enabled", ...);
  test("handles non-object persist data", ...);
});

🔒 Security

No security concerns identified. The PR:

Maintains existing validation logic for CBOR serializability
Preserves state access controls
Doesn't introduce new external dependencies

⚡ Performance

Observation: The symbol-based method calls add minimal overhead compared to direct method calls, but the trade-off for better encapsulation is worthwhile.

Note: The proxy setup in initPersistProxy() has the same performance characteristics as before (using on-change library), so no performance regression.

📋 Minor Nitpicks

Whitespace inconsistency - cloudflare-workers/src/actor-driver.ts:242 removed a blank line. Consider keeping consistent spacing.
Import organization - Some files changed from destructured to type-only imports (e.g., cloudflare-workers/src/actor-driver.ts:6-9). Good change for clarity!

✅ Approval Recommendation

Overall Assessment: APPROVE with minor suggestions

This is a solid refactoring that:

✅ Improves code organization and maintainability
✅ Maintains backward compatibility
✅ Follows existing patterns in the codebase
✅ Doesn't introduce breaking changes

The concerns raised are minor and don't block merging. Consider addressing them in follow-up PRs if needed.

📝 Action Items (Optional)

Add unit tests for StateManager class
Fix error message grammar in state getter
Consider removing unused changed getter
Add JSDoc documentation for public methods

Great work on maintaining consistency with the instance/StateManager pattern! 🎉

claude · 2025-11-13T21:03:34Z

Code Review: refactor(rivetkit): add state manager for conn with symbol-based access

Summary

This PR successfully extracts state management logic from the Conn class into a dedicated StateManager class and uses symbols to restrict access to internal methods. The refactoring improves separation of concerns and maintains backward compatibility for the public API.

✅ Positives

Clean Separation of Concerns: Moving state management logic into a dedicated class makes the codebase more maintainable and follows SRP (Single Responsibility Principle).
Consistent Architecture: The pattern mirrors the existing StateManager in instance/mod.ts, creating architectural consistency across the codebase.
Symbol-based Encapsulation: Using symbols (e.g., CONN_ACTOR_SYMBOL, CONN_HAS_CHANGES_SYMBOL) provides a TypeScript-friendly way to restrict access to internal APIs while keeping them accessible to specific modules.
Code Quality: The refactored code is well-organized with clear MARK comments for different sections (Public API, Initialization, Change Management, Private Helpers).
Backward Compatibility: Public API methods like send(), disconnect(), params, and state remain unchanged, ensuring no breaking changes.

🔍 Potential Issues & Suggestions

1. Naming Collision Concern (Minor)

Both actor/conn/state-manager.ts and actor/instance/state-manager.ts have the same file name and export a class named StateManager. While TypeScript handles this through different import paths, this could cause confusion:

Recommendation: Consider naming one of them more specifically (e.g., ConnStateManager vs ActorStateManager) or at minimum add clear documentation at the top of each file distinguishing their purposes.

2. Unused `changed` Getter (Minor)

In state-manager.ts:33, there's a get changed() getter that's never used externally:

get changed(): boolean {
    return this.#changed;
}

The code always uses hasChanges() method instead. This is dead code.

Recommendation: Remove the unused changed getter to reduce API surface area.

3. Error Message Quality (Minor)

In state-manager.ts:43, the error message could be more descriptive:

if (!this.#persist.state) throw new Error("state should exists");

Recommendation: Use a more descriptive error message: "state should exist but is undefined"
Consider using the custom error system from packages/common/error/ as per CLAUDE.md conventions.

4. Import Organization (Style)

In cloudflare-workers/src/actor-driver.ts:6-13, the change from named import to type-only import is good, but there's an inconsistent trailing comma removed at line 239.

Recommendation: Ensure consistent code style (though this will likely be caught by formatters).

🧪 Test Coverage

Concern: No new tests were added for the StateManager class or symbol-based access patterns.

While the existing tests should still pass (maintaining backward compatibility), the new internal structure isn't explicitly tested:

Symbol access patterns (CONN_HAS_CHANGES_SYMBOL, CONN_MARK_SAVED_SYMBOL, etc.)
StateManager initialization and lifecycle
State proxy setup in edge cases (null/non-object values)

Recommendation: Add unit tests for StateManager class, particularly:

Proxy initialization with various input types
Change tracking functionality
State validation (CBOR serializability)
Symbol-based method invocations

🔒 Security Considerations

No security concerns identified. The symbol-based access pattern actually improves encapsulation by making it harder to accidentally access internal methods.

⚡ Performance Considerations

No Performance Regression: The refactoring doesn't introduce any new overhead beyond the additional object allocation for StateManager, which is negligible.
Proxy Pattern Maintained: The onChange proxy pattern remains the same, so there's no change in change-detection performance.

📝 Documentation

The JSDoc comments are well-maintained throughout the refactoring. Consider adding:

A comment in conn/state-manager.ts explaining why it exists alongside instance/state-manager.ts
Documentation about the symbol-based access pattern for future maintainers

Final Verdict

Approved with Minor Suggestions ✅

This is a solid refactoring that improves code organization and maintainability. The minor issues identified are non-blocking and can be addressed in follow-up PRs if desired. The most important improvement would be adding test coverage for the new StateManager class.

Great work maintaining backward compatibility while improving the internal structure!

🤖 Generated with Claude Code

claude · 2025-11-13T21:11:53Z

Code Review for PR #3423

Summary

This PR refactors the connection state management in RivetKit by extracting state-related functionality from the Conn class into a dedicated StateManager class. The refactoring uses TypeScript symbols for internal access to maintain encapsulation while preserving the public API.

Positive Aspects

Architecture & Design

✅ Clean separation of concerns - Moving state management logic into a dedicated StateManager class improves modularity and makes the code easier to understand and maintain.

✅ Symbol-based internal API - Using symbols (CONN_ACTOR_SYMBOL, CONN_SEND_MESSAGE_SYMBOL, etc.) is a good pattern for marking internal/protected methods while maintaining backward compatibility with the public API (send, disconnect, state, etc.).

✅ Consistent with existing patterns - The PR mentions this aligns with the pattern used in instance/mod.ts, which promotes consistency across the codebase.

✅ Encapsulation - Private fields in StateManager using # prefix follows TypeScript best practices.

Code Quality

✅ Good documentation - JSDoc comments are maintained and the StateManager class has clear section markers (MARK comments).

✅ Minimal surface area changes - The refactoring maintains the existing public API, which reduces the risk of breaking changes.

Issues & Concerns

1. Unused `changed` getter in StateManager

🔴 Bug/Dead Code

// state-manager.ts:33-34
get changed(): boolean {
    return this.#changed;
}

This getter is defined but never used. The hasChanges() method is used instead via the symbol. Consider removing the changed getter to avoid confusion.

Location: state-manager.ts:33

2. Inconsistent access pattern

🟡 Code Quality

The Conn class has a mix of:

Symbol-based methods: [CONN_HAS_CHANGES_SYMBOL]()
Direct delegation: get state()

For methods like hasChanges, consider whether it should be:

// Current (symbol-based):
[CONN_HAS_CHANGES_SYMBOL](): boolean {
    return this.#stateManager.hasChanges();
}

// Alternative (if truly internal, remove from Conn entirely):
// Only accessed via stateManager internally

The symbol-based approach is valid, but ensure it's documented why some internals use symbols while others (like state getter/setter) directly delegate.

3. Potential circular dependency risk

🟡 Architecture

StateManager imports from ./mod which defines Conn:

// state-manager.ts:5
import { CONN_ACTOR_SYMBOL, CONN_STATE_ENABLED_SYMBOL, type Conn } from "./mod";

While this works because you're only importing types and symbols, this creates a circular module dependency (mod.ts → state-manager.ts → mod.ts).

Recommendation: Extract the symbols into a separate symbols.ts file to break the circular dependency:

// symbols.ts
export const CONN_ACTOR_SYMBOL = Symbol("actor");
export const CONN_STATE_ENABLED_SYMBOL = Symbol("stateEnabled");
// ... etc

Then both mod.ts and state-manager.ts can import from symbols.ts.

4. Error message could be more descriptive

🟡 Code Quality

// state-manager.ts:43
if (!this.#persist.state) throw new Error("state should exists");

This error message has a grammatical error ("exists" → "exist") and could be more descriptive:

if (!this.#persist.state) {
    throw new Error("state should exist but is undefined");
}

However, consider whether this check is even necessary - if stateEnabled is true, should state always be initialized?

5. Missing test coverage

🔴 Testing

The PR introduces a new StateManager class with significant logic (proxy setup, change tracking, CBOR validation), but I don't see any new tests added.

Recommendation: Add unit tests for:

State change detection and tracking
CBOR serialization validation
Error cases (state not enabled, invalid state types)
Proxy initialization edge cases (null/non-object targets)

6. Type safety concern with StateManager constructor

🟡 Type Safety

// state-manager.ts:12
#conn: Conn<any, CP, CS, any, any, any>;

Using any for the type parameters weakens type safety. Consider making StateManager generic over all the same parameters as Conn or using a more specific type constraint.

7. Definite assignment assertions

🟡 Code Quality

// state-manager.ts:15-16
#persist!: PersistedConn<CP, CS>;
#persistRaw!: PersistedConn<CP, CS>;

Using ! (definite assignment assertion) indicates these are initialized in initPersistProxy() rather than the constructor. While this works, it would be safer to initialize them in the constructor or make them optional and handle the undefined case.

Recommendation:

#persist: PersistedConn<CP, CS> | null = null;
#persistRaw: PersistedConn<CP, CS> | null = null;

get persist(): PersistedConn<CP, CS> {
    if (!this.#persist) throw new Error("StateManager not initialized");
    return this.#persist;
}

Minor Issues

8. Import order change

The cloudflare-workers file changes from destructured import to type-only import, which is good, but there's an unrelated whitespace change (removing blank line at 242). Keep formatting changes separate from logic changes when possible.

9. Code comments consistency

The use of // MARK: - comments is good for organization, but ensure this pattern is used consistently across the codebase.

Security Considerations

✅ No security concerns identified. The CBOR serialization validation is maintained, which is important for preventing injection of non-serializable data.

Performance Considerations

✅ No performance regressions expected. The refactoring moves existing logic without changing the underlying implementation.

Recommendations

Before merging:

🔴 Add test coverage for StateManager class
🔴 Remove unused changed getter or document why it exists
🟡 Fix error message grammar and clarity
🟡 Consider extracting symbols to separate file to avoid circular dependency
🟡 Improve type safety in StateManager constructor

Nice to have:
6. 🟢 Document in code comments why symbol-based access is used for certain methods
7. 🟢 Add integration tests showing the refactor doesn't break existing functionality

Overall Assessment

This is a solid refactoring that improves code organization and maintainability. The main concerns are around test coverage and a few code quality issues. With the recommended changes, this would be ready to merge.

Rating: ⭐⭐⭐½ (Good with minor issues to address)

graphite-app · 2025-11-13T21:15:13Z

Merge activity

Nov 13, 9:15 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
Nov 13, 9:15 PM UTC: CI is running for this pull request on a draft pull request (#3465) due to your merge queue CI optimization settings.
Nov 13, 9:16 PM UTC: Merged by the Graphite merge queue via draft PR: #3465.

…ss (#3423) - Created StateManager class for connection state management - Moved state proxying and change tracking to dedicated manager - Changed conn methods to use symbols for internal access: - actor, stateEnabled, persistRaw (getters) - hasChanges(), markSaved(), sendMessage() (methods) - Updated all references across codebase to use new symbols - Maintains backward compatibility for public API (send, disconnect, etc) - Consistent naming pattern with instance/mod.ts StateManager 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>

NathanFlurry mentioned this pull request Nov 9, 2025

chore(rivetkit): split ActorInstance logic in to multiple classes #3421

Closed

vercel bot deployed to Preview – rivetkit-serverless November 9, 2025 23:04 View deployment

NathanFlurry changed the base branch from 11-07-fix_rivetkit_use_loggerwithoutcontext_for_websocket_error_when_actor_undefined to graphite-base/3423 November 10, 2025 21:31

NathanFlurry mentioned this pull request Nov 10, 2025

fix(rivetkit): skip sending RivetKit messages to conns that do not support it #3440

Merged

NathanFlurry changed the base branch from graphite-base/3423 to 11-07-fix_rivetkit_use_loggerwithoutcontext_for_websocket_error_when_actor_undefined November 10, 2025 21:58

This was referenced Nov 13, 2025

chore(rivetkit-typescript): remove dependency on node modules #3460

Merged

chore(rivetkit): switch dynamic node imports to use require #3461

Merged

chore(rivetkit): fix type checks #3462

Merged

This was referenced Nov 13, 2025

feat(gas): implement smart pulling for horizontally scaled workers #3417

Merged

fix(guard): handle websocket tasks during shutdown #3448

Merged

fix: improve sigterm handling for the entire runtime #3443

Merged

NathanFlurry mentioned this pull request Nov 13, 2025

feat(gateway): websocket hibernation #3400

Merged

This was referenced Nov 13, 2025

fix(pb): dont immediately reallocate actors on runner stopping #3413

Merged

fix(guard): dont send open msg for hibernating ws, hibernating ws keepalive #3449

Merged

NathanFlurry mentioned this pull request Nov 13, 2025

chore(gas): rename wake sub to bump sub #3408

Merged

MasterPtato approved these changes Nov 13, 2025

View reviewed changes

graphite-app bot force-pushed the 11-07-fix_rivetkit_use_loggerwithoutcontext_for_websocket_error_when_actor_undefined branch from 4aa3109 to 7eb9e5f Compare November 13, 2025 20:52

graphite-app bot force-pushed the 11-09-refactor_rivetkit_add_state_manager_for_conn_with_symbol-based_access branch from 37bad6b to 5aebfdc Compare November 13, 2025 20:53

NathanFlurry force-pushed the 11-09-refactor_rivetkit_add_state_manager_for_conn_with_symbol-based_access branch from 5aebfdc to d2d01ea Compare November 13, 2025 20:53

NathanFlurry force-pushed the 11-07-fix_rivetkit_use_loggerwithoutcontext_for_websocket_error_when_actor_undefined branch from 7eb9e5f to af583ac Compare November 13, 2025 20:53

vercel bot deployed to Preview – rivetkit-serverless November 13, 2025 20:55 View deployment

NathanFlurry mentioned this pull request Nov 13, 2025

chore(rivetkit): implement new hibernating ws protocol #3464

Open

graphite-app bot closed this Nov 13, 2025

graphite-app bot deleted the 11-09-refactor_rivetkit_add_state_manager_for_conn_with_symbol-based_access branch November 13, 2025 21:16

NathanFlurry mentioned this pull request Nov 13, 2025

feat(cloudflare-workers): add createInlineClient #3466

Open

NicholasKissel mentioned this pull request Nov 15, 2025

chre: v4 website changes #3472

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor(rivetkit): add state manager for conn with symbol-based access #3423

refactor(rivetkit): add state manager for conn with symbol-based access #3423

Uh oh!

NathanFlurry commented Nov 9, 2025

Uh oh!

vercel bot commented Nov 9, 2025 •

edited

Loading

Uh oh!

NathanFlurry commented Nov 9, 2025 •

edited

Loading

Uh oh!

pkg-pr-new bot commented Nov 9, 2025 •

edited

Loading

Uh oh!

vercel bot commented Nov 13, 2025

Uh oh!

claude bot commented Nov 13, 2025

Uh oh!

claude bot commented Nov 13, 2025

Uh oh!

claude bot commented Nov 13, 2025

Uh oh!

claude bot commented Nov 13, 2025

Uh oh!

claude bot commented Nov 13, 2025

Uh oh!

graphite-app bot commented Nov 13, 2025 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

refactor(rivetkit): add state manager for conn with symbol-based access #3423

refactor(rivetkit): add state manager for conn with symbol-based access #3423

Uh oh!

Conversation

NathanFlurry commented Nov 9, 2025

Uh oh!

vercel bot commented Nov 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NathanFlurry commented Nov 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How to use the Graphite Merge Queue

Uh oh!

pkg-pr-new bot commented Nov 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vercel bot commented Nov 13, 2025

Uh oh!

claude bot commented Nov 13, 2025

Code Review: Refactor Connection State Management

✅ Strengths

🔍 Observations & Suggestions

1. Unused changed Getter

2. Inconsistent Access Patterns

3. Definite Assignment Assertions

4. Error Message Quality

5. Type Safety on Conn Generic

6. Missing Documentation

🧪 Testing Considerations

🔒 Security & Performance

📝 Minor Nits

🎯 Verdict

Uh oh!

claude bot commented Nov 13, 2025

Code Review: refactor(rivetkit): add state manager for conn with symbol-based access

Summary

Positive Aspects

Issues and Concerns

1. Circular Dependency Risk (rivetkit-typescript/packages/rivetkit/src/actor/conn/state-manager.ts:5)

2. Unused Property (rivetkit-typescript/packages/rivetkit/src/actor/conn/state-manager.ts:33-35)

3. Inconsistent Access Pattern (rivetkit-typescript/packages/rivetkit/src/actor/conn/mod.ts:145-147)

4. Error Message Quality (rivetkit-typescript/packages/rivetkit/src/actor/conn/state-manager.ts:43)

5. Test Coverage

Performance Considerations

Security Concerns

Minor Issues

6. Import Formatting (rivetkit-typescript/packages/cloudflare-workers/src/actor-driver.ts:9-12)

7. Documentation

Recommendations Summary

Conclusion

Uh oh!

claude bot commented Nov 13, 2025

Code Review - PR #3423: Refactor Connection State Manager

Summary

✅ Strengths

🔍 Issues & Concerns

1. Circular Dependency Risk (Minor)

2. Missing Getter Usage (Minor)

3. Inconsistent Null Handling (Low Priority)

4. Missing Documentation (Minor)

5. Type Safety Concern (Low Priority)

🧪 Test Coverage

🔒 Security

⚡ Performance

📋 Minor Nitpicks

✅ Approval Recommendation

📝 Action Items (Optional)

Uh oh!

claude bot commented Nov 13, 2025

Code Review: refactor(rivetkit): add state manager for conn with symbol-based access

Summary

✅ Positives

🔍 Potential Issues & Suggestions

1. Naming Collision Concern (Minor)

2. Unused changed Getter (Minor)

3. Error Message Quality (Minor)

4. Import Organization (Style)

🧪 Test Coverage

🔒 Security Considerations

⚡ Performance Considerations

vercel bot commented Nov 9, 2025 •

edited

Loading

NathanFlurry commented Nov 9, 2025 •

edited

Loading

pkg-pr-new bot commented Nov 9, 2025 •

edited

Loading

1. Unused `changed` Getter

2. Unused `changed` Getter (Minor)

1. Unused `changed` getter in StateManager

graphite-app bot commented Nov 13, 2025 •

edited

Loading