refactor(trading-strategies): add explicit setState API for nested strategy state - #1175
Open
bennycode wants to merge 2 commits into
Open
refactor(trading-strategies): add explicit setState API for nested strategy state#1175bennycode wants to merge 2 commits into
bennycode wants to merge 2 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces an explicit Strategy.setState() API to make nested strategy state updates persist reliably despite the existing Proxy only trapping top-level assignments. It also refactors ProtectedStrategy to use setState() for protected-state updates and adds new unit coverage documenting both the supported path and the Proxy “nested mutation” trap.
Changes:
- Added
Strategy.setState(patch)for shallow state patching with a single persistence call. - Refactored
ProtectedStrategy.#setProtectedState()to route updates throughsetState(). - Added
Strategy.test.tscoveringsetState, Proxy trap behavior, andrestoreStateround-tripping.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/trading-strategies/src/strategy/Strategy.ts | Adds setState() and class-level documentation describing the Proxy persistence behavior. |
| packages/trading-strategies/src/strategy/Strategy.test.ts | Adds unit tests for setState, Proxy trap behavior, and restore behavior. |
| packages/trading-strategies/src/strategy-protected/ProtectedStrategy.ts | Updates protected-state persistence to use setState() and updates related comments. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+99
to
+103
| setState<T extends Record<string, unknown>>(patch: Partial<T>): void { | ||
| const target = this.#stateTarget ?? {...this.state}; | ||
| Object.assign(target, patch); | ||
| this.state = {...target}; | ||
| } |
Comment on lines
+7
to
+11
| * State persistence is snapshot-based: the Proxy behind `getProxiedState()` only traps | ||
| * TOP-LEVEL assignments, so mutating a nested object in place (e.g. | ||
| * `this.state.foo.bar = x`) changes memory but is silently lost on restart. Use | ||
| * {@link Strategy.setState} to update nested state — it merges the patch and persists | ||
| * in one step. |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
A couple of small but concrete issues remain in the new Strategy docs and state-sync logic (linking/accuracy and an own-property check) that should be addressed before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
packages/trading-strategies/src/strategy/Strategy.ts:98
- The
setStatedoc says writes go to the raw Proxy target instead of through the Proxy, but that’s only true when the strategy was constructed with{state}(i.e., when#stateTargetexists). If no proxied state exists,setStatejust updates the snapshot. Consider adjusting the comment to reflect the conditional behavior to avoid misleading API docs.
/**
* The supported way to update nested state (see the class doc for the Proxy trap this
* avoids). Writes go to the raw target instead of through the Proxy so a multi-key
* patch persists once, not once per key.
*/
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
Comment on lines
+6
to
+11
| /** | ||
| * State persistence is snapshot-based: the Proxy behind `getProxiedState()` only traps | ||
| * TOP-LEVEL assignments, so mutating a nested object reached through it in place (e.g. | ||
| * `this.getProxiedState().foo.bar = x`) changes memory but is silently lost on restart. | ||
| * Use {@link Strategy.setState} to update nested state — it merges the patch and | ||
| * persists in one step. |
Comment on lines
+111
to
+115
| for (const key of Object.keys(this.#stateTarget)) { | ||
| if (!(key in snapshot)) { | ||
| delete this.#stateTarget[key]; | ||
| } | ||
| } |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Strategypersists state via an ESProxywhosesettrap only fires on top-level property assignments. Nested mutations (e.g.this.state.protected.foo = x) change memory but never triggeronSave()— the change is silently lost on restart.ProtectedStrategyworked around this by manually reassigning the whole nestedprotectedkey through the proxy.This PR makes the supported path explicit:
Strategy.setState(patch): shallow-merges a partial state patch into the current state and firesonSave()exactly once. Writes go to the raw proxy target instead of through the Proxy, so a multi-key patch persists once rather than once per key. Typed per-call assetState<T extends Record<string, unknown>>(patch: Partial<T>), matching the existinggetProxiedState<T>()pattern.ProtectedStrategyrefactored to route#setProtectedStatethroughsetState, removing the fragile manual proxy-reassignment pattern. Behavior is identical (including therestoreStatepropagation into the proxied state).Strategydocumenting the nested-mutation trap and pointing tosetStateas the supported path.Top-level proxy behavior is unchanged; no public API is broken.
Test plan
Strategy.test.ts:setStatemerges the patch, the result is visible via thestategetter and the proxied view, andonSavefires exactly once (also for multi-key patches)setStatedoes not fireonSave(documents the trap)onSave(existing behavior preserved)restoreStateround-trips a state written viasetStatenpm testinpackages/trading-strategies: 23 files / 253 tests pass (includes the fullProtectedStrategysuite, unchanged)npm run lintinpackages/trading-strategies: clean