You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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
Review: fix(rivetkit): bind methods through createWriteThroughProxy
Overview
This 3-line change adds method binding inside createWriteThroughProxy's get trap so that functions retrieved through the proxy are bound to innerTarget (the raw object) rather than the proxy receiver. This is needed to make built-in methods (e.g., Map.prototype.set, Array.prototype.push) and class methods that access private fields (#field) work correctly when those objects are stored as actor/connection state.
Critical Issue: Mutation Tracking Breaks for Method-Based Mutations
The fix solves method callability, but at a significant cost: any state mutation that happens inside a bound method via this.property = value bypasses the proxy's set trap and will not trigger commit (the persistence callback).
Example with a private-field class:
classInventory{
#items: string[]=[];add(item: string){this.#items.push(item);}// 'this' is innerTarget, not the proxy}this.state=newInventory();this.state.add('sword');// mutation happens but commit() is NEVER called — NOT persisted
Example with a plain-object method:
conststate={count: 0,increment(){this.count++;}};this.state=state;this.state.increment();// count incremented in-memory, but NOT persisted
This is the core purpose of the proxy. Silently swallowing mutations is worse than throwing an error: actors appear to work (mutations are visible in-memory) but lose data on sleep/restart.
Suggested approach: If the fix is specifically targeting private-field classes or built-ins like Map/Set, document this as an unsupported pattern for write-through tracking and throw an actionable error (per the fail-by-default rule in CLAUDE.md). Alternatively, restrict the proxy to plain objects (Object.getPrototypeOf(target) === Object.prototype) and reject class instances with an explicit error.
Performance: New Bound Function on Every Get
Every property access that returns a function now allocates a new bound function via .bind(innerTarget). The proxies WeakMap already caches proxy wrappers for nested objects. The same pattern should apply to bound functions to avoid repeated allocations per access, which matters more for connection state (ConnStateManager) that is accessed per request.
Missing Tests
There are no tests covering the scenario this fix addresses. A unit test (no real engine needed) demonstrating the fixed case would validate correctness and prevent regressions. The driver test suite (tests/driver/actor-save-state.test.ts) has good patterns to follow for persistence-level coverage.
Minor: Draft Checklist
The PR is in DRAFT with all checklist items unchecked. Please self-review against the style guidelines and address test coverage before marking ready.
Summary: The intent is correct. Method access on proxied state objects currently breaks for private-field classes and built-ins. However, binding to innerTarget silently breaks write-through tracking for method-based mutations, which is the proxy's core purpose. Silent data loss on sleep/restart is the failure mode. This needs either a redesign or explicit documentation plus an actionable error for unsupported patterns before merging.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Please include a summary of the changes and the related issue. Please also include relevant motivation and context.
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes.
Checklist: