Conversation
…ctly, or an updater function
Greptile OverviewGreptile SummaryThis RFC implements Key Implementation DetailsAPI Implementation: The Store Interface: The implementation properly exposes the Transition Semantics: The Reference Counting: The Known Limitations: The PR description acknowledges several limitations that are intentionally left unresolved as this is a POC:
SSR Support: Both Fizz and Flight correctly throw errors when Test Coverage: Comprehensive tests cover basic usage, transitions, selectors, multiple stores, and edge cases. Confidence Score: 4/5
Important Files ChangedFile Analysis
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -3431,6 +3431,11 @@ function commitRoot( | |||
| ): void { | |||
| root.cancelPendingCommit = null; | |||
|
|
|||
| // TODO: Where exactly should this live? | |||
There was a problem hiding this comment.
style: consider removing TODO comment - this placement in commitRoot before flushPassiveEffects seems appropriate
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-reconciler/src/ReactFiberWorkLoop.js
Line: 3434:3434
Comment:
**style:** consider removing TODO comment - this placement in commitRoot before flushPassiveEffects seems appropriate
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| export function useStore<S, T>( | ||
| store: ReactExternalDataSource<S, mixed>, | ||
| selector?: (state: S) => T, | ||
| ): S | T { |
There was a problem hiding this comment.
style: return type S | T may be imprecise - when selector is provided, only T should be returned, not S. Should the return type be conditional based on whether selector is provided?
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react/src/ReactHooks.js
Line: 205:205
Comment:
**style:** return type `S | T` may be imprecise - when selector is provided, only T should be returned, not S. Should the return type be conditional based on whether selector is provided?
How can I resolve this? If you propose a fix, please make it concise.| 'createStore is not available because the enableStore feature flag is not enabled.', | ||
| ); | ||
| } | ||
| const actualReducer = reducer ?? (defaultReducer: any); |
There was a problem hiding this comment.
style: Type assertion (defaultReducer: any) loses type safety when no reducer is provided.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-reconciler/src/ReactStore.js
Line: 34:34
Comment:
**style:** Type assertion `(defaultReducer: any)` loses type safety when no reducer is provided.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| // Then useLayoutEffect fires, dispatches increment | ||
| // StoreReader re-renders with updated state (3) before mount completes |
There was a problem hiding this comment.
syntax: Comment incorrectly refers to useLayoutEffect when the test uses useEffect
| // Then useLayoutEffect fires, dispatches increment | |
| // StoreReader re-renders with updated state (3) before mount completes | |
| // Then useEffect fires, dispatches increment | |
| // StoreReader re-renders with updated state (3) before mount completes |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-reconciler/src/__tests__/useStore-test.js
Line: 894:895
Comment:
**syntax:** Comment incorrectly refers to useLayoutEffect when the test uses useEffect
```suggestion
// Then useEffect fires, dispatches increment
// StoreReader re-renders with updated state (3) before mount completes
```
How can I resolve this? If you propose a fix, please make it concise.| } else { | ||
| // We are updating sync, but a transition is in progress. Implement | ||
| // React's update reordering semantics. | ||
| this._committedState = this.store.reducer(this._committedState, action); |
There was a problem hiding this comment.
logic: Potential issue: calling store.reducer directly assumes the reducer exists, but ReactExternalDataSource type may not guarantee a reducer property. Is the reducer property guaranteed to be present on all ReactExternalDataSource instances used with this API?
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-reconciler/src/ReactFiberStoreTracking.js
Line: 45:45
Comment:
**logic:** Potential issue: calling `store.reducer` directly assumes the reducer exists, but `ReactExternalDataSource` type may not guarantee a reducer property. Is the reducer property guaranteed to be present on all ReactExternalDataSource instances used with this API?
How can I resolve this? If you propose a fix, please make it concise.| import type {StoreWrapper} from './ReactFiberStoreTracking'; // Ensure StoreTracking is loaded} from './ReactFiberStoreTracking'; // Ensure StoreTracking is loaded | ||
| import {StoreTracker} from './ReactFiberStoreTracking'; // Ensure StoreTracking is loaded |
There was a problem hiding this comment.
syntax: Duplicate import comment detected in type imports
| import type {StoreWrapper} from './ReactFiberStoreTracking'; // Ensure StoreTracking is loaded} from './ReactFiberStoreTracking'; // Ensure StoreTracking is loaded | |
| import {StoreTracker} from './ReactFiberStoreTracking'; // Ensure StoreTracking is loaded | |
| import type {StoreWrapper} from './ReactFiberStoreTracking'; | |
| import {StoreTracker} from './ReactFiberStoreTracking'; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-reconciler/src/ReactFiberHooks.js
Line: 168:169
Comment:
**syntax:** Duplicate import comment detected in type imports
```suggestion
import type {StoreWrapper} from './ReactFiberStoreTracking';
import {StoreTracker} from './ReactFiberStoreTracking';
```
How can I resolve this? If you propose a fix, please make it concise.
everettbu
force-pushed
the
use-store-with-selector
branch
from
e1c7053 to
26b5ecc
Compare
Additional Comments (6)
The RFC explicitly specifies: function createStore<S, A>(
initialValue: S,
reducer?: (S, A) => S
): ReactStore<S, A>;But this implementation has THREE CRITICAL PROBLEMS:
This breaks the documented example Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react/src/ReactStore.js
Line: 14:16
Comment:
[P0] **API signature contradicts RFC specification**
The RFC explicitly specifies:
```typescript
function createStore<S, A>(
initialValue: S,
reducer?: (S, A) => S
): ReactStore<S, A>;
```
But this implementation has **THREE CRITICAL PROBLEMS**:
1. **Parameters reversed**: `reducer` comes first, `initialValue` second
2. **Reducer is required**: Should be optional per RFC
3. **Missing setState-style functionality**: RFC states "If a reducer is not provided, the dispatch method mirrors `setState` in that it accepts either a new value, or an updater function"
This breaks the documented example `createStore(1)` from the RFC description, making the API unusable as specified. The RFC's design goal of echoing `useState`/`useReducer` patterns (where initial value comes first) is not met.
How can I resolve this? If you propose a fix, please make it concise.
The RFC's subscribe: (callback: (action: A) => void) => () => voidThe callback should receive the dispatched action, but this implementation calls Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react/src/ReactStore.js
Line: 39:39
Comment:
[P1] **Subscribe callback signature doesn't match RFC specification**
The RFC's `ReactExternalDataSource` interface specifies:
```typescript
subscribe: (callback: (action: A) => void) => () => void
```
The callback should receive the dispatched action, but this implementation calls `callback()` with no arguments. This prevents subscribers from knowing which action triggered the update, limiting debugging and observability capabilities.
How can I resolve this? If you propose a fix, please make it concise.
The RFC defines the interface with: export interface ReactExternalDataSource<S, A> {
getState(): S,
reducer: (S, A) => S,
subscribe: (callback: (action: A) => void) => () => void,
};But
This mismatch prevents external libraries from implementing compatible data sources as described in the RFC. Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/shared/ReactTypes.js
Line: 385:391
Comment:
[P2] **Type definition doesn't match `ReactExternalDataSource` interface**
The RFC defines the interface with:
```typescript
export interface ReactExternalDataSource<S, A> {
getState(): S,
reducer: (S, A) => S,
subscribe: (callback: (action: A) => void) => () => void,
};
```
But `ReactStore` type exposes:
- `_current` and `_transition` instead of `getState()`
- `_reducer` (private) instead of `reducer`
- `subscribe` callback is `() => void` instead of `(action: A) => void`
This mismatch prevents external libraries from implementing compatible data sources as described in the RFC.
How can I resolve this? If you propose a fix, please make it concise.
The Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react-reconciler/src/__tests__/useStoreWithSelector-test.js
Line: 56:56
Comment:
[P0] **ReferenceError: `identify` function used before definition**
The `identify` function is referenced at line 56 but not defined until line 145. This will cause a runtime ReferenceError when the test runs.
How can I resolve this? If you propose a fix, please make it concise.
The test dispatches Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react-reconciler/src/__tests__/useStoreWithSelector-test.js
Line: 94:94
Comment:
[P0] **Reducer doesn't handle 'double' action type**
The test dispatches `{type: 'double'}` at line 130, but the reducer only handles 'increment' and 'decrement' actions (lines 92-93). The 'double' case at line 94 is present but the reducer will fall through to the default case, returning the count unchanged instead of doubling it.
How can I resolve this? If you propose a fix, please make it concise.
The function signature declares Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react-reconciler/src/ReactFiberHooks.js
Line: 3979:3983
Comment:
[P1] **Flow type error: undefined generic type parameter**
The function signature declares `<S, A>` but references `StoreWithSelectorQueue<T>` where `T` is not declared. This will cause Flow type checking errors.
How can I resolve this? If you propose a fix, please make it concise. |
| return storeWrapper.subscribe( | ||
| handleStoreSubscriptionChange.bind( | ||
| null, | ||
| fiber, | ||
| queue, | ||
| storeWrapper, | ||
| selector, | ||
| ), | ||
| ); | ||
| } |
There was a problem hiding this comment.
CRITICAL MEMORY LEAK: StoreTracker reference count never decremented
The createSubscription function returns an unsubscribe function from storeWrapper.subscribe(), which only removes the callback from the wrapper's internal _listeners Set. However, it never calls storeTracker.remove(store) to decrement the reference count.
The Problem:
- Line 1844 in
mountStorecallsgetWrapper()which incrementsStoreTracker.stores.get(store).references - Line 1895 in
updateStorealso callsgetWrapper()which increments the reference count again on every render - When the component unmounts, the effect cleanup (line 1869-1879) only calls the unsubscribe function
- The unsubscribe function (ReactFiberStoreTracking.js:54-59) only does
this._listeners.delete(callback) - The reference count is never decremented
wrapper.dispose()is never called, keeping the subscription to the underlying store alive forever- Both the wrapper and the store subscription leak permanently
Impact:
- StoreWrapper instances accumulate in memory and are never garbage collected
- Original store subscriptions are never cleaned up via
dispose() - This will cause severe memory leaks in any real application using useStore
Fix Required:
The effect cleanup needs to decrement the reference count. The unsubscribe function returned from createSubscription should call root.storeTracker.remove(store) before returning.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-reconciler/src/ReactFiberHooks.js
Line: 1973:1982
Comment:
**CRITICAL MEMORY LEAK: StoreTracker reference count never decremented**
The `createSubscription` function returns an unsubscribe function from `storeWrapper.subscribe()`, which only removes the callback from the wrapper's internal `_listeners` Set. However, it **never calls `storeTracker.remove(store)` to decrement the reference count**.
**The Problem:**
1. Line 1844 in `mountStore` calls `getWrapper()` which increments `StoreTracker.stores.get(store).references`
2. Line 1895 in `updateStore` also calls `getWrapper()` which increments the reference count again on every render
3. When the component unmounts, the effect cleanup (line 1869-1879) only calls the unsubscribe function
4. The unsubscribe function (ReactFiberStoreTracking.js:54-59) only does `this._listeners.delete(callback)`
5. The reference count is **never decremented**
6. `wrapper.dispose()` is never called, keeping the subscription to the underlying store alive forever
7. Both the wrapper and the store subscription **leak permanently**
**Impact:**
- StoreWrapper instances accumulate in memory and are never garbage collected
- Original store subscriptions are never cleaned up via `dispose()`
- This will cause severe memory leaks in any real application using useStore
**Fix Required:**
The effect cleanup needs to decrement the reference count. The unsubscribe function returned from `createSubscription` should call `root.storeTracker.remove(store)` before returning.
How can I resolve this? If you propose a fix, please make it concise.| handleUpdate(action: A) { | ||
| const transitionState = this._headState; | ||
| const currentState = this._committedState; | ||
| this._headState = this.store.getState(); | ||
|
|
||
| if (ReactSharedInternals.T !== null) { | ||
| // We are in a transition, update the transition state only | ||
| } else if (is(transitionState, currentState)) { | ||
| // We are updating sync and no transition is in progress, update both | ||
| this._committedState = this._headState; | ||
| } else { | ||
| // We are updating sync, but a transition is in progress. Implement | ||
| // React's update reordering semantics. | ||
| this._committedState = this.store.reducer(this._committedState, action); | ||
| } | ||
| // Notify all subscribed fibers | ||
| this._listeners.forEach(listener => listener()); | ||
| } |
There was a problem hiding this comment.
Missing error handling: reducer can leave state inconsistent
If this.store.reducer() throws an error on line 45, the state becomes inconsistent:
The Problem:
- Line 35 updates
this._headState = this.store.getState()successfully - Line 45 calls
this.store.reducer(this._committedState, action) - If the reducer throws,
_committedStateis NOT updated - Line 48 still notifies all listeners even though state update failed
_headStateand_committedStateare now permanently out of sync
Impact:
- Components will render with inconsistent state
- The divergence between
_headStateand_committedStatecan never be recovered - Subsequent updates will build on incorrect state
Recommendation:
Wrap the reducer call in try-catch to ensure atomic state updates, or ensure both _headState and _committedState are updated together before notifying listeners.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-reconciler/src/ReactFiberStoreTracking.js
Line: 32:49
Comment:
**Missing error handling: reducer can leave state inconsistent**
If `this.store.reducer()` throws an error on line 45, the state becomes inconsistent:
**The Problem:**
1. Line 35 updates `this._headState = this.store.getState()` successfully
2. Line 45 calls `this.store.reducer(this._committedState, action)`
3. If the reducer throws, `_committedState` is NOT updated
4. Line 48 still notifies all listeners even though state update failed
5. `_headState` and `_committedState` are now permanently out of sync
**Impact:**
- Components will render with inconsistent state
- The divergence between `_headState` and `_committedState` can never be recovered
- Subsequent updates will build on incorrect state
**Recommendation:**
Wrap the reducer call in try-catch to ensure atomic state updates, or ensure both `_headState` and `_committedState` are updated together before notifying listeners.
How can I resolve this? If you propose a fix, please make it concise.| dispatch(action: A) { | ||
| state = actualReducer(state, action); | ||
| subscriptions.forEach(callback => callback(action)); | ||
| }, |
There was a problem hiding this comment.
No error handling if reducer throws
If actualReducer(state, action) throws an exception:
stateremains at the old value- Line 47 still calls all subscription callbacks
- Subscribers are notified even though the state update failed
- Components may re-render unnecessarily
While this matches the behavior in ReactFiberStoreTracking.js (which has the same issue), consider wrapping the reducer call in try-catch to prevent notifying subscribers when state updates fail.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-reconciler/src/ReactStore.js
Line: 45:48
Comment:
**No error handling if reducer throws**
If `actualReducer(state, action)` throws an exception:
1. `state` remains at the old value
2. Line 47 still calls all subscription callbacks
3. Subscribers are notified even though the state update failed
4. Components may re-render unnecessarily
While this matches the behavior in ReactFiberStoreTracking.js (which has the same issue), consider wrapping the reducer call in try-catch to prevent notifying subscribers when state updates fail.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| root.storeTracker = new StoreTracker(); | ||
| } | ||
|
|
||
| const wrapper = root.storeTracker.getWrapper(store); |
There was a problem hiding this comment.
Potential reference count issue: getWrapper() called on every update
On every update, getWrapper() increments the reference count. This means:
- First render: references = 1
- Second render: references = 2
- Third render: references = 3
- etc.
Since the cleanup never decrements (see critical memory leak comment), the reference count grows unbounded. Even if the cleanup is fixed, calling getWrapper() on every update will require N cleanup calls to fully dispose the wrapper, not just 1.
Recommendation:
Cache the wrapper in the hook state to avoid incrementing references on every render, or ensure getWrapper() doesn't increment if already tracking this fiber.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-reconciler/src/ReactFiberHooks.js
Line: 1895:1895
Comment:
**Potential reference count issue: getWrapper() called on every update**
On every update, `getWrapper()` increments the reference count. This means:
- First render: references = 1
- Second render: references = 2
- Third render: references = 3
- etc.
Since the cleanup never decrements (see critical memory leak comment), the reference count grows unbounded. Even if the cleanup is fixed, calling `getWrapper()` on every update will require N cleanup calls to fully dispose the wrapper, not just 1.
**Recommendation:**
Cache the wrapper in the hook state to avoid incrementing references on every render, or ensure `getWrapper()` doesn't increment if already tracking this fiber.
How can I resolve this? If you propose a fix, please make it concise.| /** | ||
| * Copyright (c) Meta Platforms, Inc. and affiliates. | ||
| * | ||
| * This source code is licensed under the MIT license found in the | ||
| * LICENSE file in the root directory of this source tree. | ||
| * | ||
| * @emails react-core | ||
| * @gate enableStore | ||
| */ | ||
|
|
||
| 'use strict'; | ||
|
|
||
| let useStore; | ||
| let React; | ||
| let ReactNoop; | ||
| let Scheduler; | ||
| let act; | ||
| let createStore; | ||
| let startTransition; | ||
| let waitFor; | ||
| let assertLog; | ||
| let useLayoutEffect; | ||
| let useEffect; | ||
| let use; | ||
| let Suspense; | ||
|
|
There was a problem hiding this comment.
Missing critical test coverage
The test suite lacks coverage for several critical scenarios:
- Memory leaks: No tests verify that unmounting components properly cleans up subscriptions and decrements reference counts
- Error handling: No tests for reducer throwing errors during dispatch
- Selector stability: No tests for what happens when selector function changes (inline functions)
- Multiple roots: No tests for multiple React roots observing the same store
- Edge cases: No tests for store updates during unmount, or rapid mount/unmount cycles
Given the critical memory leak found in the implementation, tests should specifically verify:
- Wrapper disposal when last component unmounts
- Reference count decrements on cleanup
- No leaked subscriptions to underlying store
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-reconciler/src/__tests__/useStore-test.js
Line: 1:26
Comment:
**Missing critical test coverage**
The test suite lacks coverage for several critical scenarios:
1. **Memory leaks**: No tests verify that unmounting components properly cleans up subscriptions and decrements reference counts
2. **Error handling**: No tests for reducer throwing errors during dispatch
3. **Selector stability**: No tests for what happens when selector function changes (inline functions)
4. **Multiple roots**: No tests for multiple React roots observing the same store
5. **Edge cases**: No tests for store updates during unmount, or rapid mount/unmount cycles
Given the critical memory leak found in the implementation, tests should specifically verify:
- Wrapper disposal when last component unmounts
- Reference count decrements on cleanup
- No leaked subscriptions to underlying store
How can I resolve this? If you propose a fix, please make it concise.
| root.storeTracker = new StoreTracker(); | ||
| } | ||
|
|
||
| const wrapper = root.storeTracker.getWrapper(store); |
There was a problem hiding this comment.
[P0] CRITICAL: Memory leak in StoreTracker reference counting
The getWrapper() call increments the reference count in StoreTracker, but there is NO corresponding storeTracker.remove(store) call anywhere in the codebase when components unmount.
The problem:
- When
mountStore/updateStorecallsgetWrapper(), it incrementsStoreTracker.stores.get(store).references - When the component unmounts, the effect cleanup (returned by
createSubscription) only unsubscribes from the wrapper's internal_listenersSet - The reference count is never decremented
- The wrapper's
dispose()is never called, keeping the subscription to the underlying store alive forever - Both the wrapper and the store subscription leak permanently in memory
Fix needed:
The effect cleanup (line 1973-1981) must also call root.storeTracker.remove(store) to decrement the reference count and allow cleanup when references reach 0.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-reconciler/src/ReactFiberHooks.js
Line: 1844:1844
Comment:
[P0] **CRITICAL: Memory leak in StoreTracker reference counting**
The `getWrapper()` call increments the reference count in `StoreTracker`, but there is NO corresponding `storeTracker.remove(store)` call anywhere in the codebase when components unmount.
**The problem:**
1. When `mountStore`/`updateStore` calls `getWrapper()`, it increments `StoreTracker.stores.get(store).references`
2. When the component unmounts, the effect cleanup (returned by `createSubscription`) only unsubscribes from the wrapper's internal `_listeners` Set
3. The reference count is never decremented
4. The wrapper's `dispose()` is never called, keeping the subscription to the underlying store alive forever
5. Both the wrapper and the store subscription leak permanently in memory
**Fix needed:**
The effect cleanup (line 1973-1981) must also call `root.storeTracker.remove(store)` to decrement the reference count and allow cleanup when references reach 0.
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (8)
The RFC explicitly states: function createStore<S, A>(
initialValue: S,
reducer?: (S, A) => S
): ReactStore<S, A>;But this implementation has THREE CRITICAL PROBLEMS:
This breaks the documented example The test file at lines 53 and 100 uses Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react/src/ReactStore.js
Line: 14:16
Comment:
**CRITICAL: API signature contradicts RFC specification**
The RFC explicitly states:
```typescript
function createStore<S, A>(
initialValue: S,
reducer?: (S, A) => S
): ReactStore<S, A>;
```
But this implementation has **THREE CRITICAL PROBLEMS**:
1. **Parameters reversed**: `reducer` comes first, `initialValue` second
2. **Reducer is required**: Should be optional per RFC
3. **Missing setState-style functionality**: RFC states "If a reducer is not provided, the dispatch method mirrors setState in that it accepts either a new value, or an updater function"
This breaks the documented example `createStore(1)` from the RFC description, making the API unusable as specified. The RFC's design goal of echoing `useState`/`useReducer` patterns (where initial value comes first) is not met.
The test file at lines 53 and 100 uses `createStore(counterReducer, 2)` which matches this broken implementation but contradicts the documented API.
How can I resolve this? If you propose a fix, please make it concise.
The RFC's subscribe: (callback: (action: A) => void) => () => voidBut line 37 calls Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react/src/ReactStore.js
Line: 39:44
Comment:
**Interface violation: subscribe callback doesn't receive action parameter**
The RFC's `ReactExternalDataSource` interface specifies:
```typescript
subscribe: (callback: (action: A) => void) => () => void
```
But line 37 calls `callback()` with no arguments. Subscribers have no way to know which action triggered the update, limiting debugging and observability capabilities. The RFC explicitly states: "The callback will be called after the state has updated and includes the action that was dispatched."
How can I resolve this? If you propose a fix, please make it concise.
The RFC defines export interface ReactExternalDataSource<S, A> {
getState(): S,
reducer: (S, A) => S,
subscribe: (callback: (action: A) => void) => () => void,
};This type definition is missing the Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/shared/ReactTypes.js
Line: 385:391
Comment:
**Missing `getState()` method from RFC interface**
The RFC defines `ReactExternalDataSource` interface with:
```typescript
export interface ReactExternalDataSource<S, A> {
getState(): S,
reducer: (S, A) => S,
subscribe: (callback: (action: A) => void) => () => void,
};
```
This type definition is missing the `getState()` method entirely. External libraries implementing this interface won't be able to integrate correctly. The `_current` and `_transition` fields expose internal implementation details that should be hidden.
How can I resolve this? If you propose a fix, please make it concise.
The Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react-reconciler/src/__tests__/useStoreWithSelector-test.js
Line: 56:56
Comment:
**ReferenceError: `identify` function used before declaration**
The `identify` function is defined at line 145 but used here at line 56. This will cause a ReferenceError at runtime due to JavaScript's Temporal Dead Zone for function expressions. Move the `identify` function declaration before the first test, or use a function declaration instead of expression.
How can I resolve this? If you propose a fix, please make it concise.
The test dispatches Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react-reconciler/src/__tests__/useStoreWithSelector-test.js
Line: 94:94
Comment:
**Reducer doesn't handle 'double' action type**
The test dispatches `{type: 'double'}` at line 130, but the reducer (lines 87-99) only handles 'increment' and 'decrement'. The 'double' case falls through to the default case which returns the count unchanged. This makes the test logic incorrect - line 133 expects output `[4]` (2 * 2), but the store will actually still be at 3 (previous increment from the transition).
How can I resolve this? If you propose a fix, please make it concise.
The function signature declares Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react-reconciler/src/ReactFiberHooks.js
Line: 3979:3981
Comment:
**Flow type error: undefined generic type parameter `T`**
The function signature declares `<S, A>` but references `StoreWithSelectorQueue<T>` where `T` is not declared. This will cause Flow type checking errors. Either add `T` to the generic parameters or use the correct type parameter.
How can I resolve this? If you propose a fix, please make it concise.
If the reducer throws an exception during dispatch, the store state can become permanently inconsistent. In the final Consider wrapping reducer calls in try-catch to ensure atomicity. Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react/src/ReactStore.js
Line: 24:37
Comment:
**No error handling if reducer throws**
If the reducer throws an exception during dispatch, the store state can become permanently inconsistent. In the final `else` branch (lines 34-35), if the first `reducer()` call succeeds but the second throws, `_transition` will be updated but `_current` won't, leaving them permanently out of sync. Additionally, subscriptions (line 37) are notified even if the state update failed, causing subscribers to see inconsistent state.
Consider wrapping reducer calls in try-catch to ensure atomicity.
How can I resolve this? If you propose a fix, please make it concise.
Subscriptions are called synchronously in a Additionally, if a subscriber dispatches another action synchronously, it creates nested dispatch calls which could lead to unexpected update ordering or infinite loops if not carefully managed. Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react/src/ReactStore.js
Line: 37:37
Comment:
**Synchronous subscription notification without error isolation**
Subscriptions are called synchronously in a `forEach` loop. If one subscriber throws an error, it will prevent subsequent subscribers from being notified. This breaks the isolation principle - one misbehaving subscriber shouldn't affect others.
Additionally, if a subscriber dispatches another action synchronously, it creates nested dispatch calls which could lead to unexpected update ordering or infinite loops if not carefully managed.
How can I resolve this? If you propose a fix, please make it concise. |
| selector?: S => T, | ||
| ): T { |
There was a problem hiding this comment.
Unresolved TODO: mounting during transition observes incorrect state
This TODO describes a known bug mentioned in the RFC's "Unsolved Problems" section. When a component mounts during a transition while observing a store that's already updating, it will incorrectly observe the post-transition value instead of the pre-transition value.
This is a fundamental correctness issue that could cause visual inconsistencies and break React's transition semantics. The RFC acknowledges this requires all React roots to track all transitioning stores (possibly using WeakRefs), but this is not implemented.
This makes the API unsafe for production use when components dynamically mount during transitions.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-reconciler/src/ReactFiberHooks.js
Line: 1835:1836
Comment:
**Unresolved TODO: mounting during transition observes incorrect state**
This TODO describes a known bug mentioned in the RFC's "Unsolved Problems" section. When a component mounts during a transition while observing a store that's already updating, it will incorrectly observe the post-transition value instead of the pre-transition value.
This is a fundamental correctness issue that could cause visual inconsistencies and break React's transition semantics. The RFC acknowledges this requires all React roots to track all transitioning stores (possibly using WeakRefs), but this is not implemented.
This makes the API unsafe for production use when components dynamically mount during transitions.
How can I resolve this? If you propose a fix, please make it concise.| actualSelector(storeState)); | ||
| } | ||
|
|
||
| return state; |
There was a problem hiding this comment.
Hardcoded lane constant instead of determining actual transition lanes
Using SomeTransitionLane as a hardcoded constant instead of determining the actual transition lane(s) that need to be used could cause incorrect priority handling. The comment on line 1930 even acknowledges this: "TODO: We should determine the actual lane (lanes?) we need to use here."
This could lead to:
- Updates being scheduled at wrong priorities
- Incorrect interaction with concurrent features
- Race conditions when multiple transitions are active
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-reconciler/src/ReactFiberHooks.js
Line: 1931:1931
Comment:
**Hardcoded lane constant instead of determining actual transition lanes**
Using `SomeTransitionLane` as a hardcoded constant instead of determining the actual transition lane(s) that need to be used could cause incorrect priority handling. The comment on line 1930 even acknowledges this: "TODO: We should determine the actual lane (lanes?) we need to use here."
This could lead to:
- Updates being scheduled at wrong priorities
- Incorrect interaction with concurrent features
- Race conditions when multiple transitions are active
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (8)
The PR description explicitly specifies: function createStore<S, A>(
initialValue: S,
reducer?: (S, A) => S
): ReactStore<S, A>;But this implementation has THREE CRITICAL PROBLEMS:
This breaks the documented example The test file at lines 53 and 100 uses Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react/src/ReactStore.js
Line: 14:16
Comment:
**[P0] CRITICAL: API signature completely contradicts RFC specification**
The PR description explicitly specifies:
```typescript
function createStore<S, A>(
initialValue: S,
reducer?: (S, A) => S
): ReactStore<S, A>;
```
But this implementation has **THREE CRITICAL PROBLEMS**:
1. **Parameters reversed**: `reducer` comes first, `initialValue` second
2. **Reducer is required**: Should be optional per RFC
3. **Missing setState-style functionality**: RFC states "If a reducer is not provided, the dispatch method mirrors `setState` in that it accepts either a new value, or an updater function"
This breaks the documented example `createStore(1)` from the RFC description, making the API unusable as specified. The RFC's design goal of echoing `useState`/`useReducer` patterns (where initial value comes first) is not met.
The test file at lines 53 and 100 uses `createStore(counterReducer, 2)` which matches this broken implementation but contradicts the documented API.
How can I resolve this? If you propose a fix, please make it concise.
The RFC's subscribe: (callback: (action: A) => void) => () => voidBut this implementation calls Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react/src/ReactStore.js
Line: 39:39
Comment:
**Subscribe callback doesn't receive action parameter**
The RFC's `ReactExternalDataSource` interface specifies:
```typescript
subscribe: (callback: (action: A) => void) => () => void
```
But this implementation calls `callback()` with no arguments. This means subscribers cannot observe which action triggered the update, breaking the documented interface contract and limiting debugging/observability capabilities.
How can I resolve this? If you propose a fix, please make it concise.
If the reducer throws an exception during dispatch, the store state can become permanently inconsistent. In the final
This violates the fundamental requirement that Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react/src/ReactStore.js
Line: 24:37
Comment:
**Missing error handling - reducer exceptions leave store in inconsistent state**
If the reducer throws an exception during dispatch, the store state can become permanently inconsistent. In the final `else` branch (lines 34-35), if the first `reducer()` call succeeds but the second throws:
- `_transition` gets updated to the new state
- `_current` remains at the old state
- The two will be permanently desynchronized
- Subscriptions are never notified (line 37 won't execute)
This violates the fundamental requirement that `_current` and `_transition` should eventually converge. The implementation should either wrap reducer calls in try-catch or ensure atomic state updates.
How can I resolve this? If you propose a fix, please make it concise.
If a subscription callback throws an error, subsequent callbacks in the Set won't execute. This can cause some components to miss critical updates. Consider wrapping each callback invocation in try-catch or using a queued notification mechanism to ensure all subscribers are notified even if one fails. Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react/src/ReactStore.js
Line: 37:37
Comment:
**Subscription callbacks called synchronously without error isolation**
If a subscription callback throws an error, subsequent callbacks in the Set won't execute. This can cause some components to miss critical updates. Consider wrapping each callback invocation in try-catch or using a queued notification mechanism to ensure all subscribers are notified even if one fails.
How can I resolve this? If you propose a fix, please make it concise.
The RFC description defines the interface as: export interface ReactExternalDataSource<S, A> {
getState(): S,
reducer: (S, A) => S,
subscribe: (callback: (action: A) => void) => () => void,
};But this type definition:
This prevents external libraries from implementing compatible data sources as described in the RFC. Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/shared/ReactTypes.js
Line: 385:391
Comment:
**Type definition doesn't match `ReactExternalDataSource` interface from RFC**
The RFC description defines the interface as:
```typescript
export interface ReactExternalDataSource<S, A> {
getState(): S,
reducer: (S, A) => S,
subscribe: (callback: (action: A) => void) => () => void,
};
```
But this type definition:
1. **Missing `getState()` method** - RFC requires this for reading current state
2. **Exposes internal fields** - `_current`, `_transition`, `_reducer` are implementation details that shouldn't be in the public API
3. **Wrong subscribe signature** - callback should be `(action: A) => void` not `() => void`
This prevents external libraries from implementing compatible data sources as described in the RFC.
How can I resolve this? If you propose a fix, please make it concise.
The function signature declares generic parameters The function should either:
Looking at call sites (lines 1908, 1951), the queue type should match the hook's state type, not introduce a new generic parameter. Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react-reconciler/src/ReactFiberHooks.js
Line: 3979:3983
Comment:
**Flow type error: undefined generic type parameter `T`**
The function signature declares generic parameters `<S, A>` but the `queue` parameter references `StoreWithSelectorQueue<T>` where `T` is not declared. This will cause Flow type checking errors.
The function should either:
1. Add `T` to the generic parameters: `entangleTransitionUpdate<S, A, T>`
2. Or use the correct type from the calling context
Looking at call sites (lines 1908, 1951), the queue type should match the hook's state type, not introduce a new generic parameter.
How can I resolve this? If you propose a fix, please make it concise.
The This test will fail immediately when executed. Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react-reconciler/src/__tests__/useStoreWithSelector-test.js
Line: 56:56
Comment:
**`identify` function is undefined - ReferenceError**
The `identify` function is used here but is only defined later at line 145. JavaScript will throw a ReferenceError when this test runs because function declarations using `function` keyword are hoisted, but this is using `const identify = ...` at line 145 which is not hoisted.
This test will fail immediately when executed.
How can I resolve this? If you propose a fix, please make it concise.
The test dispatches Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react-reconciler/src/__tests__/useStoreWithSelector-test.js
Line: 94:94
Comment:
**Reducer doesn't handle 'double' action type**
The test dispatches `{type: 'double'}` but the reducer only handles 'increment' and 'decrement' (lines 44-48). The 'double' case is defined in the second test's reducer but not in this one. This will cause the reducer to fall through to the default case and return the current count unchanged, making the test fail.
How can I resolve this? If you propose a fix, please make it concise. |
| return storeWrapper.subscribe( | ||
| handleStoreSubscriptionChange.bind( | ||
| null, | ||
| fiber, | ||
| queue, | ||
| storeWrapper, | ||
| selector, | ||
| ), | ||
| ); | ||
| } |
There was a problem hiding this comment.
CRITICAL: Memory leak in StoreTracker reference counting
The createSubscription function returns an unsubscribe function from storeWrapper.subscribe(), but this only removes the listener from the wrapper's internal _listeners Set. It does NOT decrement the reference count in the StoreTracker.
The problem:
- When
mountStoreorupdateStorecallsgetWrapper()at line 1844/1895, it incrementsStoreTracker.stores.get(store).references - When the component unmounts, the effect cleanup only calls the returned unsubscribe function
- The unsubscribe function only calls
this._listeners.delete(callback)(from StoreWrapper.subscribe in ReactFiberStoreTracking.js) - The reference count is never decremented
- The wrapper's
dispose()is never called, keeping the subscription to the underlying store alive forever - Both the wrapper and the store subscription leak permanently
Fix needed:
The effect cleanup needs to call root.storeTracker.remove(store) to decrement the reference count. However, you need to ensure you only decrement when the component actually unmounts, not when the effect re-runs due to dependency changes. This likely requires wrapping the cleanup or tracking mount state separately.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-reconciler/src/ReactFiberHooks.js
Line: 1973:1982
Comment:
**CRITICAL: Memory leak in StoreTracker reference counting**
The `createSubscription` function returns an unsubscribe function from `storeWrapper.subscribe()`, but this only removes the listener from the wrapper's internal `_listeners` Set. It does NOT decrement the reference count in the `StoreTracker`.
**The problem:**
1. When `mountStore` or `updateStore` calls `getWrapper()` at line 1844/1895, it increments `StoreTracker.stores.get(store).references`
2. When the component unmounts, the effect cleanup only calls the returned unsubscribe function
3. The unsubscribe function only calls `this._listeners.delete(callback)` (from StoreWrapper.subscribe in ReactFiberStoreTracking.js)
4. The reference count is never decremented
5. The wrapper's `dispose()` is never called, keeping the subscription to the underlying store alive forever
6. Both the wrapper and the store subscription leak permanently
**Fix needed:**
The effect cleanup needs to call `root.storeTracker.remove(store)` to decrement the reference count. However, you need to ensure you only decrement when the component actually unmounts, not when the effect re-runs due to dependency changes. This likely requires wrapping the cleanup or tracking mount state separately.
How can I resolve this? If you propose a fix, please make it concise.| dispatch(action: A) { | ||
| state = actualReducer(state, action); | ||
| subscriptions.forEach(callback => callback(action)); | ||
| }, |
There was a problem hiding this comment.
Potential issue: No error handling if reducer throws
If actualReducer(state, action) throws an exception:
- The state update is not applied (correct)
- BUT subscribers are never notified (line 47 won't execute)
- This could leave React components out of sync with the store
This differs from how React's built-in reducers handle errors. Consider wrapping in try-catch to ensure consistent behavior, or document that reducer errors will prevent subscription notifications.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-reconciler/src/ReactStore.js
Line: 45:48
Comment:
**Potential issue: No error handling if reducer throws**
If `actualReducer(state, action)` throws an exception:
1. The state update is not applied (correct)
2. BUT subscribers are never notified (line 47 won't execute)
3. This could leave React components out of sync with the store
This differs from how React's built-in reducers handle errors. Consider wrapping in try-catch to ensure consistent behavior, or document that reducer errors will prevent subscription notifications.
How can I resolve this? If you propose a fix, please make it concise.| reducer: actualReducer, | ||
| dispatch(action: A) { | ||
| state = actualReducer(state, action); | ||
| subscriptions.forEach(callback => callback(action)); |
There was a problem hiding this comment.
Potential issue: Synchronous subscription notifications without error boundaries
If a subscription callback throws an error during forEach, subsequent callbacks in the Set won't execute. This means some React components could miss updates while others receive them, leading to inconsistent UI state.
Consider wrapping each callback invocation in a try-catch to ensure all subscribers are notified even if one fails, similar to how React's event system handles errors.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-reconciler/src/ReactStore.js
Line: 47:47
Comment:
**Potential issue: Synchronous subscription notifications without error boundaries**
If a subscription callback throws an error during `forEach`, subsequent callbacks in the Set won't execute. This means some React components could miss updates while others receive them, leading to inconsistent UI state.
Consider wrapping each callback invocation in a try-catch to ensure all subscribers are notified even if one fails, similar to how React's event system handles errors.
How can I resolve this? If you propose a fix, please make it concise.| if ( | ||
| previousArgs.selector !== actualSelector || | ||
| previousArgs.wrapper !== wrapper | ||
| ) { | ||
| queue.dispatch = {wrapper, selector: actualSelector}; | ||
| return (hook.memoizedState = queue.lastRenderedState = | ||
| actualSelector(storeState)); | ||
| } |
There was a problem hiding this comment.
Unstable selector functions cause unnecessary re-renders
The selector is compared by reference (previousArgs.selector !== actualSelector). If the user passes an inline function like useStore(store, state => state.count), this will be a new function reference on every render, causing the hook state to be invalidated and reset even when the selector logic hasn't changed.
This defeats the performance optimization purpose of selectors. Users should be warned to memoize selectors with useCallback, or the implementation should use a more stable comparison mechanism.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-reconciler/src/ReactFiberHooks.js
Line: 1922:1929
Comment:
**Unstable selector functions cause unnecessary re-renders**
The selector is compared by reference (`previousArgs.selector !== actualSelector`). If the user passes an inline function like `useStore(store, state => state.count)`, this will be a new function reference on every render, causing the hook state to be invalidated and reset even when the selector logic hasn't changed.
This defeats the performance optimization purpose of selectors. Users should be warned to memoize selectors with `useCallback`, or the implementation should use a more stable comparison mechanism.
How can I resolve this? If you propose a fix, please make it concise.| handleUpdate(action: A) { | ||
| const transitionState = this._headState; | ||
| const currentState = this._committedState; | ||
| this._headState = this.store.getState(); | ||
|
|
||
| if (ReactSharedInternals.T !== null) { | ||
| // We are in a transition, update the transition state only | ||
| } else if (is(transitionState, currentState)) { | ||
| // We are updating sync and no transition is in progress, update both | ||
| this._committedState = this._headState; | ||
| } else { | ||
| // We are updating sync, but a transition is in progress. Implement | ||
| // React's update reordering semantics. | ||
| this._committedState = this.store.reducer(this._committedState, action); | ||
| } | ||
| // Notify all subscribed fibers | ||
| this._listeners.forEach(listener => listener()); | ||
| } |
There was a problem hiding this comment.
Potential race condition in handleUpdate
The handleUpdate method reads and writes _headState and _committedState without any synchronization. If multiple actions are dispatched rapidly (before React has a chance to schedule updates), these state transitions could become inconsistent.
Specifically:
- Line 35:
this._headState = this.store.getState()reads the new state - Lines 39-45: Conditionally updates
_committedStatebased on transition status - If another dispatch occurs between these operations, the state tracking could become corrupted
The current implementation assumes dispatches are serialized or that React's scheduler will handle this, but this assumption should be validated or documented.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-reconciler/src/ReactFiberStoreTracking.js
Line: 32:49
Comment:
**Potential race condition in handleUpdate**
The `handleUpdate` method reads and writes `_headState` and `_committedState` without any synchronization. If multiple actions are dispatched rapidly (before React has a chance to schedule updates), these state transitions could become inconsistent.
Specifically:
- Line 35: `this._headState = this.store.getState()` reads the new state
- Lines 39-45: Conditionally updates `_committedState` based on transition status
- If another dispatch occurs between these operations, the state tracking could become corrupted
The current implementation assumes dispatches are serialized or that React's scheduler will handle this, but this assumption should be validated or documented.
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (7)
The PR description explicitly specifies: function createStore<S, A>(
initialValue: S,
reducer?: (S, A) => S
): ReactStore<S, A>;But this implementation has THREE CRITICAL PROBLEMS:
This breaks the documented example The test file at lines 53 and 100 uses Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react/src/ReactStore.js
Line: 14:16
Comment:
[P0] **CRITICAL: API signature completely contradicts RFC specification**
The PR description explicitly specifies:
```typescript
function createStore<S, A>(
initialValue: S,
reducer?: (S, A) => S
): ReactStore<S, A>;
```
But this implementation has **THREE CRITICAL PROBLEMS**:
1. **Parameters reversed**: `reducer` comes first, `initialValue` second
2. **Reducer is required**: Should be optional per RFC
3. **Missing setState-style functionality**: RFC states "If a reducer is not provided, the dispatch method mirrors setState in that it accepts either a new value, or an updater function"
This breaks the documented example `createStore(1)` from the RFC description, making the API unusable as specified. The RFC's design goal of echoing `useState`/`useReducer` patterns (where initial value comes first) is not met.
The test file at lines 53 and 100 uses `createStore(counterReducer, 2)` which matches this broken implementation but contradicts the documented API.
How can I resolve this? If you propose a fix, please make it concise.
The RFC's subscribe: (callback: (action: A) => void) => () => voidThis means callbacks should receive the dispatched action. But this implementation calls This prevents subscribers from knowing which action triggered the update, limiting debugging and observability capabilities that the interface was designed to provide. Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react/src/ReactStore.js
Line: 37:37
Comment:
[P0] **CRITICAL: Subscribe callback doesn't receive action parameter**
The RFC's `ReactExternalDataSource` interface specifies:
```typescript
subscribe: (callback: (action: A) => void) => () => void
```
This means callbacks should receive the dispatched action. But this implementation calls `callback()` with no arguments, breaking the interface contract.
This prevents subscribers from knowing which action triggered the update, limiting debugging and observability capabilities that the interface was designed to provide.
How can I resolve this? If you propose a fix, please make it concise.
The RFC's interface ReactExternalDataSource<S, A> {
getState(): S,
reducer: (S, A) => S,
subscribe: (callback: (action: A) => void) => () => void,
}But this type definition:
This type mismatch means external libraries cannot correctly implement compatible data sources. Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/shared/ReactTypes.js
Line: 385:391
Comment:
[P1] **Type definition doesn't match `ReactExternalDataSource` interface**
The RFC's `ReactExternalDataSource` interface should expose:
```typescript
interface ReactExternalDataSource<S, A> {
getState(): S,
reducer: (S, A) => S,
subscribe: (callback: (action: A) => void) => () => void,
}
```
But this type definition:
1. **Missing `getState()` method** - RFC specifies this should return current state
2. **Exposes internal fields** (`_current`, `_transition`, `_reducer`) - these are implementation details that should be private
3. **Wrong subscribe signature** - callbacks should receive `(action: A) => void`, not `() => void`
This type mismatch means external libraries cannot correctly implement compatible data sources.
How can I resolve this? If you propose a fix, please make it concise.
This line has Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react-reconciler/src/ReactFiberHooks.js
Line: 3978:3978
Comment:
[P2] **Invalid comment syntax - missing second `/`**
This line has `/ TODO:` instead of `// TODO:` which is invalid JavaScript syntax and will cause a parse error.
How can I resolve this? If you propose a fix, please make it concise.
The function signature declares This will cause Flow type checking errors. Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react-reconciler/src/ReactFiberHooks.js
Line: 3979:3983
Comment:
[P2] **Flow type error: undefined generic type parameter**
The function signature declares `<S, A>` but references `StoreWithSelectorQueue<T>` where `T` is not declared.
```suggestion
function entangleTransitionUpdate<S, A, T>(
root: FiberRoot,
queue: UpdateQueue<S, A> | StoreWithSelectorQueue<T>,
lane: Lane,
): void {
```
This will cause Flow type checking errors.
How can I resolve this? If you propose a fix, please make it concise.
If the reducer throws an exception during dispatch, the store state becomes inconsistent. In the final
This violates the atomicity guarantee users expect from state updates. Consider wrapping reducer calls in try-catch or ensuring atomic state updates. Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react/src/ReactStore.js
Line: 24:36
Comment:
[P1] **No error handling if reducer throws**
If the reducer throws an exception during dispatch, the store state becomes inconsistent. In the final `else` branch (lines 34-35), if the first `reducer()` call succeeds but the second throws:
- `_transition` gets updated
- `_current` doesn't get updated
- The two states permanently desynchronize
- Subscribers are still notified (line 37) even though the state update failed
This violates the atomicity guarantee users expect from state updates. Consider wrapping reducer calls in try-catch or ensuring atomic state updates.
How can I resolve this? If you propose a fix, please make it concise.
Subscribers are notified synchronously during dispatch using
Consider:
Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react/src/ReactStore.js
Line: 37:37
Comment:
[P2] **Synchronous subscription notifications can cause issues**
Subscribers are notified synchronously during dispatch using `forEach`. This can cause problems:
1. **If a subscriber throws**, subsequent subscribers won't be notified
2. **If a subscriber calls dispatch again**, it creates nested/reentrant updates that can lead to unpredictable state
3. **Performance**: With many subscribers, this blocks the dispatch call
Consider:
- Wrapping each callback in try-catch to ensure all subscribers are notified
- Preventing or detecting reentrant dispatch calls
- Using queueMicrotask for async notification (though this may conflict with React's sync update semantics)
How can I resolve this? If you propose a fix, please make it concise. |
| root.storeTracker = new StoreTracker(); | ||
| } | ||
|
|
||
| const wrapper = root.storeTracker.getWrapper(store); |
There was a problem hiding this comment.
[P1] Memory leak: StoreTracker reference count never decremented
The getWrapper() call increments the reference count in StoreTracker, but there's no corresponding storeTracker.remove(store) call when the component unmounts or the effect cleanup fires.
The problem:
mountStore/updateStorecallsgetWrapper()which incrementsreferences- When component unmounts, the effect cleanup only calls the unsubscribe function returned by
storeWrapper.subscribe() - This unsubscribe only removes the listener from the wrapper's
_listenersSet - The reference count is never decremented, so
wrapper.dispose()is never called - The wrapper persists in memory and keeps its subscription to the underlying store active
Impact: Store wrappers accumulate in memory indefinitely, and subscriptions to underlying stores are never cleaned up, even after all components stop using them.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-reconciler/src/ReactFiberHooks.js
Line: 1844:1844
Comment:
[P1] **Memory leak: StoreTracker reference count never decremented**
The `getWrapper()` call increments the reference count in `StoreTracker`, but there's no corresponding `storeTracker.remove(store)` call when the component unmounts or the effect cleanup fires.
**The problem:**
1. `mountStore`/`updateStore` calls `getWrapper()` which increments `references`
2. When component unmounts, the effect cleanup only calls the unsubscribe function returned by `storeWrapper.subscribe()`
3. This unsubscribe only removes the listener from the wrapper's `_listeners` Set
4. The reference count is never decremented, so `wrapper.dispose()` is never called
5. The wrapper persists in memory and keeps its subscription to the underlying store active
**Impact:** Store wrappers accumulate in memory indefinitely, and subscriptions to underlying stores are never cleaned up, even after all components stop using them.
How can I resolve this? If you propose a fix, please make it concise.| dispatch(action: A) { | ||
| state = actualReducer(state, action); | ||
| subscriptions.forEach(callback => callback(action)); |
There was a problem hiding this comment.
[P2] No error handling if reducer throws
If actualReducer() throws an exception, the state remains unchanged but subscriptions are still notified. This could cause inconsistent state where subscribers see notifications but the state hasn't actually updated.
Additionally, if a subscription callback throws during forEach, remaining callbacks won't execute, causing incomplete notification of subscribers.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-reconciler/src/ReactStore.js
Line: 45:47
Comment:
[P2] **No error handling if reducer throws**
If `actualReducer()` throws an exception, the state remains unchanged but subscriptions are still notified. This could cause inconsistent state where subscribers see notifications but the state hasn't actually updated.
Additionally, if a subscription callback throws during `forEach`, remaining callbacks won't execute, causing incomplete notification of subscribers.
How can I resolve this? If you propose a fix, please make it concise.| handleUpdate(action: A) { | ||
| const transitionState = this._headState; | ||
| const currentState = this._committedState; | ||
| this._headState = this.store.getState(); | ||
|
|
||
| if (ReactSharedInternals.T !== null) { | ||
| // We are in a transition, update the transition state only | ||
| } else if (is(transitionState, currentState)) { | ||
| // We are updating sync and no transition is in progress, update both | ||
| this._committedState = this._headState; | ||
| } else { | ||
| // We are updating sync, but a transition is in progress. Implement | ||
| // React's update reordering semantics. | ||
| this._committedState = this.store.reducer(this._committedState, action); | ||
| } |
There was a problem hiding this comment.
[P2] Potential state inconsistency if reducer throws
In the else branch (line 45), if the reducer throws an exception when computing this._committedState, the _committedState field will remain unchanged but _headState was already updated (line 35). This creates a state divergence between the two tracked states that won't self-heal.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-reconciler/src/ReactFiberStoreTracking.js
Line: 32:46
Comment:
[P2] **Potential state inconsistency if reducer throws**
In the `else` branch (line 45), if the reducer throws an exception when computing `this._committedState`, the `_committedState` field will remain unchanged but `_headState` was already updated (line 35). This creates a state divergence between the two tracked states that won't self-heal.
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (6)
This line uses Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react-reconciler/src/ReactFiberHooks.js
Line: 243:243
Comment:
**[P2] Invalid comment syntax - causes parse error**
This line uses `/ TODO:` instead of `// TODO:` which is invalid JavaScript syntax and will cause a parse error during compilation.
```suggestion
// TODO: Use something other than null
```
How can I resolve this? If you propose a fix, please make it concise.
This line uses Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react-reconciler/src/ReactFiberHooks.js
Line: 3978:3978
Comment:
**[P2] Invalid comment syntax - causes parse error**
This line uses `/ TODO:` instead of `// TODO:` which is invalid JavaScript syntax and will cause a parse error.
```suggestion
// TODO: Move to ReactFiberConcurrentUpdates?
```
How can I resolve this? If you propose a fix, please make it concise.
The function signature declares generic parameters The function should either:
Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react-reconciler/src/ReactFiberHooks.js
Line: 3979:3983
Comment:
**[P1] Flow type error - undefined generic parameter `T`**
The function signature declares generic parameters `<S, A>` but the `queue` parameter type references `StoreWithSelectorQueue<T>` where `T` is not declared in the function's generic parameters. This will cause Flow type checking errors.
The function should either:
1. Add `T` to the generic parameters: `function entangleTransitionUpdate<S, A, T>(...)`
2. Or use a different approach that doesn't require the undefined generic type
```suggestion
function entangleTransitionUpdate<S, A, T>(
root: FiberRoot,
queue: UpdateQueue<S, A> | StoreWithSelectorQueue<T>,
lane: Lane,
): void {
```
How can I resolve this? If you propose a fix, please make it concise.
If the
Additionally, line 37 notifies subscribers even if the reducer threw an error, causing components to render with corrupted state. Consider wrapping reducer calls in try-catch blocks and implementing atomic state updates or rollback logic. Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react/src/ReactStore.js
Line: 24:36
Comment:
**[P1] No error handling - reducer exceptions cause state corruption**
If the `reducer` function throws an exception, the store state can become permanently inconsistent:
1. **In transition path (line 27)**: If reducer throws, `_transition` is left unchanged, but error propagates
2. **In sync-only path (line 30)**: If reducer throws after `_transition` is updated, `_current` remains out of sync
3. **In rebasing path (lines 34-35)**: If the first `reducer` call succeeds but the second throws, `_transition` is updated but `_current` is not, causing permanent state desynchronization
**Additionally**, line 37 notifies subscribers even if the reducer threw an error, causing components to render with corrupted state.
Consider wrapping reducer calls in try-catch blocks and implementing atomic state updates or rollback logic.
How can I resolve this? If you propose a fix, please make it concise.
The RFC's subscribe: (callback: (action: A) => void) => () => voidThe callback should receive the dispatched action, but this implementation calls The RFC description states: "The callback will be called after the state has updated and includes the action that was dispatched." Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react/src/ReactStore.js
Line: 37:37
Comment:
**[P2] Subscription callbacks don't receive action parameter**
The RFC's `ReactExternalDataSource` interface specifies:
```typescript
subscribe: (callback: (action: A) => void) => () => void
```
The callback should receive the dispatched action, but this implementation calls `callback()` with no arguments. This prevents subscribers from knowing which action triggered the update, limiting debugging and observability capabilities.
The RFC description states: "The callback will be called after the state has updated and includes the action that was dispatched."
How can I resolve this? If you propose a fix, please make it concise.
The The function definition should be moved before the test cases, or the tests should define it inline. Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react-reconciler/src/__tests__/useStoreWithSelector-test.js
Line: 56:56
Comment:
**[P0] ReferenceError - `identify` function used before definition**
The `identify` function is referenced here at line 56 (and again at line 103), but it's not defined until line 145. This will cause a `ReferenceError: identify is not defined` at runtime when the test executes.
The function definition should be moved before the test cases, or the tests should define it inline.
How can I resolve this? If you propose a fix, please make it concise. |
| const wrapper = root.storeTracker.getWrapper(store); | ||
| const fiber = currentlyRenderingFiber; | ||
| const storeState = wrapper.getStateForLanes(renderLanes); | ||
|
|
||
| const initialState = actualSelector(storeState); | ||
|
|
||
| const hook = mountWorkInProgressHook(); | ||
|
|
||
| const hookArgs: UseStoreArgs<S, T> = { | ||
| wrapper, | ||
| selector: actualSelector, | ||
| }; | ||
|
|
||
| hook.memoizedState = hook.baseState = initialState; | ||
| const queue: UpdateQueue<T, mixed> = { | ||
| pending: null, | ||
| lanes: NoLanes, | ||
| // Hack: We don't use dispatch for anything, so we can repurpose | ||
| // it to store the args for access inside updateStore. | ||
| dispatch: hookArgs as any, | ||
| lastRenderedReducer: storeReducerPlaceholder, | ||
| lastRenderedState: (initialState: any), | ||
| }; | ||
| hook.queue = queue; | ||
|
|
||
| mountEffect( | ||
| createSubscription.bind( | ||
| null, | ||
| wrapper, | ||
| fiber, | ||
| actualSelector, | ||
| queue, | ||
| storeState, | ||
| ), | ||
| [actualSelector, wrapper], | ||
| ); | ||
|
|
||
| return initialState; |
There was a problem hiding this comment.
CRITICAL: Memory leak - StoreWrapper reference count never decremented
The getWrapper() call at line 1844 increments the reference count in StoreTracker, but there is no corresponding call to storeTracker.remove(store) when the component unmounts.
The problem:
mountStorecallsgetWrapper()→ incrementsreferencescount- Effect cleanup (returned by
createSubscription) only unsubscribes from wrapper's internal_listenersSet - Reference count is never decremented
wrapper.dispose()is never called- Subscription to underlying store leaks permanently
- Wrapper object persists in memory indefinitely
Impact: Every component using useStore will leak memory. In applications with many components mounting/unmounting, this will accumulate significantly.
Fix needed: The effect cleanup must call root.storeTracker.remove(store) to decrement the reference count. However, this needs careful coordination - the reference should only be decremented once per component, not on every effect re-run.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-reconciler/src/ReactFiberHooks.js
Line: 1844:1881
Comment:
**CRITICAL: Memory leak - StoreWrapper reference count never decremented**
The `getWrapper()` call at line 1844 increments the reference count in `StoreTracker`, but there is **no corresponding call to `storeTracker.remove(store)`** when the component unmounts.
**The problem:**
1. `mountStore` calls `getWrapper()` → increments `references` count
2. Effect cleanup (returned by `createSubscription`) only unsubscribes from wrapper's internal `_listeners` Set
3. Reference count is never decremented
4. `wrapper.dispose()` is never called
5. Subscription to underlying store leaks permanently
6. Wrapper object persists in memory indefinitely
**Impact:** Every component using `useStore` will leak memory. In applications with many components mounting/unmounting, this will accumulate significantly.
**Fix needed:** The effect cleanup must call `root.storeTracker.remove(store)` to decrement the reference count. However, this needs careful coordination - the reference should only be decremented once per component, not on every effect re-run.
How can I resolve this? If you propose a fix, please make it concise.| if ( | ||
| previousArgs.selector !== actualSelector || | ||
| previousArgs.wrapper !== wrapper | ||
| ) { | ||
| queue.dispatch = {wrapper, selector: actualSelector}; | ||
| return (hook.memoizedState = queue.lastRenderedState = | ||
| actualSelector(storeState)); |
There was a problem hiding this comment.
Unstable selector functions cause unnecessary re-renders
The selector is compared by reference (previousArgs.selector !== actualSelector). If users pass an inline function like useStore(store, state => state.count), this will be a new function reference on every render, causing:
- The condition to always be true
- Effect to re-run unnecessarily
- Subscription to be recreated on every render
- Performance degradation
Example that triggers the issue:
function MyComponent() {
// This selector is a new function on every render
const count = useStore(store, state => state.count);
return <div>{count}</div>;
}Recommendation: Document that selectors should be stable (use useCallback or define outside component), or implement selector memoization similar to Redux's approach.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-reconciler/src/ReactFiberHooks.js
Line: 1922:1928
Comment:
**Unstable selector functions cause unnecessary re-renders**
The selector is compared by reference (`previousArgs.selector !== actualSelector`). If users pass an inline function like `useStore(store, state => state.count)`, this will be a new function reference on every render, causing:
1. The condition to always be true
2. Effect to re-run unnecessarily
3. Subscription to be recreated on every render
4. Performance degradation
**Example that triggers the issue:**
```javascript
function MyComponent() {
// This selector is a new function on every render
const count = useStore(store, state => state.count);
return <div>{count}</div>;
}
```
**Recommendation:** Document that selectors should be stable (use `useCallback` or define outside component), or implement selector memoization similar to Redux's approach.
How can I resolve this? If you propose a fix, please make it concise.| dispatch(action: A) { | ||
| state = actualReducer(state, action); | ||
| subscriptions.forEach(callback => callback(action)); | ||
| }, |
There was a problem hiding this comment.
Missing error handling - reducer exceptions cause inconsistent state
If the reducer throws an exception during dispatch, the state remains unchanged BUT subscribers are still notified (line 47 executes even if line 46 throws). This creates inconsistency where:
- Components receive notifications
- But
getState()returns the old state - Components can't distinguish between "state unchanged" and "reducer threw"
Recommendation: Wrap reducer call in try-catch, or ensure subscribers are only notified after successful state update:
dispatch(action: A) {
const newState = actualReducer(state, action);
state = newState;
subscriptions.forEach(callback => callback(action));
}This ensures atomicity - either both succeed or neither happens.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-reconciler/src/ReactStore.js
Line: 45:48
Comment:
**Missing error handling - reducer exceptions cause inconsistent state**
If the reducer throws an exception during dispatch, the state remains unchanged BUT subscribers are still notified (line 47 executes even if line 46 throws). This creates inconsistency where:
1. Components receive notifications
2. But `getState()` returns the old state
3. Components can't distinguish between "state unchanged" and "reducer threw"
**Recommendation:** Wrap reducer call in try-catch, or ensure subscribers are only notified after successful state update:
```javascript
dispatch(action: A) {
const newState = actualReducer(state, action);
state = newState;
subscriptions.forEach(callback => callback(action));
}
```
This ensures atomicity - either both succeed or neither happens.
How can I resolve this? If you propose a fix, please make it concise.| handleUpdate(action: A) { | ||
| const transitionState = this._headState; | ||
| const currentState = this._committedState; | ||
| this._headState = this.store.getState(); | ||
|
|
||
| if (ReactSharedInternals.T !== null) { | ||
| // We are in a transition, update the transition state only | ||
| } else if (is(transitionState, currentState)) { | ||
| // We are updating sync and no transition is in progress, update both | ||
| this._committedState = this._headState; | ||
| } else { | ||
| // We are updating sync, but a transition is in progress. Implement | ||
| // React's update reordering semantics. | ||
| this._committedState = this.store.reducer(this._committedState, action); | ||
| } | ||
| // Notify all subscribed fibers | ||
| this._listeners.forEach(listener => listener()); | ||
| } |
There was a problem hiding this comment.
Missing error handling in handleUpdate
If store.reducer() throws at line 45, the wrapper's internal state (_committedState) becomes inconsistent with the actual store state. The exception will propagate to all listeners, but the wrapper's state tracking is corrupted.
Impact: After a reducer error in the sync-during-transition case, the wrapper permanently loses sync with the store, causing incorrect rendering.
Recommendation: Wrap the reducer call in try-catch and handle errors gracefully, or let it throw but ensure state consistency is maintained.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-reconciler/src/ReactFiberStoreTracking.js
Line: 32:49
Comment:
**Missing error handling in handleUpdate**
If `store.reducer()` throws at line 45, the wrapper's internal state (`_committedState`) becomes inconsistent with the actual store state. The exception will propagate to all listeners, but the wrapper's state tracking is corrupted.
**Impact:** After a reducer error in the sync-during-transition case, the wrapper permanently loses sync with the store, causing incorrect rendering.
**Recommendation:** Wrap the reducer call in try-catch and handle errors gracefully, or let it throw but ensure state consistency is maintained.
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (8)
The PR description explicitly specifies: function createStore<S, A>(
initialValue: S,
reducer?: (S, A) => S
): ReactStore<S, A>;But this implementation has THREE CRITICAL PROBLEMS:
This breaks the documented example The test file at lines 53 and 100 uses Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react/src/ReactStore.js
Line: 14:16
Comment:
**[P0] CRITICAL: API signature completely contradicts RFC specification**
The PR description explicitly specifies:
```typescript
function createStore<S, A>(
initialValue: S,
reducer?: (S, A) => S
): ReactStore<S, A>;
```
But this implementation has **THREE CRITICAL PROBLEMS**:
1. **Parameters reversed**: `reducer` comes first, `initialValue` second
2. **Reducer is required**: Should be optional per RFC
3. **Missing setState-style functionality**: RFC states "If a reducer is not provided, the dispatch method mirrors `setState` in that it accepts either a new value, or an updater function"
This breaks the documented example `createStore(1)` from the RFC description, making the API unusable as specified. The RFC's design goal of echoing `useState`/`useReducer` patterns (where initial value comes first) is not met.
The test file at lines 53 and 100 uses `createStore(counterReducer, 2)` which matches this broken implementation but contradicts the documented API.
How can I resolve this? If you propose a fix, please make it concise.
The RFC description defines a export interface ReactExternalDataSource<S, A> {
getState(): S,
reducer: (S, A) => S,
subscribe: (callback: (action: A) => void) => () => void,
};But this
This means external libraries cannot implement compatible data sources, and the implementation doesn't follow the documented interface contract. Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/shared/ReactTypes.js
Line: 385:391
Comment:
**[P0] CRITICAL: Type definition doesn't match `ReactExternalDataSource` interface from RFC**
The RFC description defines a `ReactExternalDataSource` interface:
```typescript
export interface ReactExternalDataSource<S, A> {
getState(): S,
reducer: (S, A) => S,
subscribe: (callback: (action: A) => void) => () => void,
};
```
But this `ReactStore` type exposes:
1. **Missing `getState()` method**: RFC requires this but type has `_current` and `_transition` fields instead
2. **Wrong property name**: Has `_reducer` (private) instead of `reducer` (public)
3. **Wrong subscribe callback signature**: Has `(callback: () => void)` but RFC specifies `(callback: (action: A) => void)` - callbacks should receive the dispatched action
This means external libraries cannot implement compatible data sources, and the implementation doesn't follow the documented interface contract.
How can I resolve this? If you propose a fix, please make it concise.
The RFC's subscribe: (callback: (action: A) => void) => () => voidThe subscription callback should receive the dispatched action so subscribers know what changed. This implementation calls This limits debugging capabilities and prevents subscribers from implementing action-specific logic. Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react/src/ReactStore.js
Line: 37:37
Comment:
**[P0] CRITICAL: Subscribe callbacks don't receive the action parameter**
The RFC's `ReactExternalDataSource` interface specifies:
```typescript
subscribe: (callback: (action: A) => void) => () => void
```
The subscription callback should receive the dispatched action so subscribers know what changed. This implementation calls `callback()` with no arguments, breaking the documented interface and preventing subscribers from tracking which action triggered the update.
This limits debugging capabilities and prevents subscribers from implementing action-specific logic.
How can I resolve this? If you propose a fix, please make it concise.
The function signature declares The function should either:
Looking at the call sites (lines 1908, 1951), they pass Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react-reconciler/src/ReactFiberHooks.js
Line: 3979:3983
Comment:
**[P1] Flow type error: undefined generic type parameter**
The function signature declares `<S, A>` but references `StoreWithSelectorQueue<T>` where `T` is not declared in the generic parameters. This will cause Flow type checking errors.
The function should either:
1. Add `T` to the generic parameters: `function entangleTransitionUpdate<S, A, T>(...)`
2. Or remove the unused `StoreWithSelectorQueue<T>` from the union type if it's not actually needed
Looking at the call sites (lines 1908, 1951), they pass `UpdateQueue<T, mixed>` where `T` is the selected state type, not `S`. This suggests the function needs to be more flexible with its type signature.
How can I resolve this? If you propose a fix, please make it concise.
The The function needs to be moved before the first test, or the tests need to be reordered. Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react-reconciler/src/__tests__/useStoreWithSelector-test.js
Line: 56:56
Comment:
**[P0] Test will fail: `identify` function undefined at this point**
The `identify` function is defined at line 145, but it's used here at line 56. This will cause a `ReferenceError: identify is not defined` when the test runs.
The function needs to be moved before the first test, or the tests need to be reordered.
How can I resolve this? If you propose a fix, please make it concise.
The reducer defined at lines 87-98 only handles Actually, looking more carefully, the reducer DOES handle Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react-reconciler/src/__tests__/useStoreWithSelector-test.js
Line: 94:94
Comment:
**[P1] Test dispatches unhandled action type**
The reducer defined at lines 87-98 only handles `'increment'` and `'double'` action types, but line 94 explicitly shows `'double'` in the type definition. However, at line 130, the test dispatches `{type: 'double'}` which should work.
Actually, looking more carefully, the reducer DOES handle `'double'` at line 94-95. This is not an issue - my initial assessment was incorrect. The reducer correctly handles all action types used in the test.
How can I resolve this? If you propose a fix, please make it concise.
If the reducer throws an exception during dispatch, the store state can become inconsistent. In the final
This violates the atomicity requirement for state updates. The implementation should either:
Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react/src/ReactStore.js
Line: 24:36
Comment:
**[P1] Missing error handling - reducer errors cause state inconsistency**
If the reducer throws an exception during dispatch, the store state can become inconsistent. In the final `else` branch (lines 34-35):
1. If the first `reducer(self._transition, action)` succeeds but the second `reducer(self._current, action)` throws
2. Then `_transition` gets updated but `_current` doesn't
3. This leaves the two states permanently desynchronized
4. Subscriptions are called even if state update failed (line 37)
This violates the atomicity requirement for state updates. The implementation should either:
- Wrap reducer calls in try-catch and rollback on error
- Compute both new states first, then update both atomically
- At minimum, avoid calling subscriptions if reducer throws
How can I resolve this? If you propose a fix, please make it concise.
Subscriptions are called synchronously during
Consider wrapping each callback in try-catch to ensure all subscribers are notified even if one fails: subscriptions.forEach(callback => {
try {
callback(action); // Also needs to pass action per interface
} catch (error) {
// Log or handle error but don't prevent other subscribers
}
});Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react/src/ReactStore.js
Line: 37:37
Comment:
**[P2] Synchronous subscription notification can cause nested updates and issues**
Subscriptions are called synchronously during `dispatch()`, which means:
1. If a subscriber calls `dispatch()` again, it creates nested/re-entrant updates
2. If a subscriber throws an error, subsequent subscribers won't be notified
3. The subscription callbacks execute while the dispatch is still in progress
Consider wrapping each callback in try-catch to ensure all subscribers are notified even if one fails:
```javascript
subscriptions.forEach(callback => {
try {
callback(action); // Also needs to pass action per interface
} catch (error) {
// Log or handle error but don't prevent other subscribers
}
});
```
How can I resolve this? If you propose a fix, please make it concise. |
| root.storeTracker = new StoreTracker(); | ||
| } | ||
|
|
||
| const wrapper = root.storeTracker.getWrapper(store); |
There was a problem hiding this comment.
CRITICAL MEMORY LEAK: StoreWrapper reference count never decremented
The getWrapper() call increments the reference count in StoreTracker, but there is no corresponding decrement when the component unmounts.
The problem:
mountStore/updateStorecallsgetWrapper()which incrementsinfo.references(line 90 in ReactFiberStoreTracking.js)- When component unmounts, the effect cleanup only calls the unsubscribe function returned by
storeWrapper.subscribe() - That unsubscribe only removes the listener from
_listenersSet, but does NOT decrement the reference count wrapper.dispose()is never called, keeping the subscription to the underlying store alive forever- Both the wrapper and subscription leak permanently
Impact:
- Every component that uses
useStorewill permanently leak itsStoreWrapperinstance - The wrapper maintains an active subscription to the underlying store
- Memory usage grows unbounded as components mount and unmount
Fix needed:
The effect cleanup (returned by createSubscription) must call root.storeTracker.remove(store) to decrement the reference count. However, this requires careful handling to avoid double-decrement if the effect runs multiple times.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-reconciler/src/ReactFiberHooks.js
Line: 1844:1844
Comment:
**CRITICAL MEMORY LEAK: StoreWrapper reference count never decremented**
The `getWrapper()` call increments the reference count in `StoreTracker`, but there is **no corresponding decrement** when the component unmounts.
**The problem:**
1. `mountStore`/`updateStore` calls `getWrapper()` which increments `info.references` (line 90 in ReactFiberStoreTracking.js)
2. When component unmounts, the effect cleanup only calls the unsubscribe function returned by `storeWrapper.subscribe()`
3. That unsubscribe only removes the listener from `_listeners` Set, but does NOT decrement the reference count
4. `wrapper.dispose()` is never called, keeping the subscription to the underlying store alive forever
5. Both the wrapper and subscription leak permanently
**Impact:**
- Every component that uses `useStore` will permanently leak its `StoreWrapper` instance
- The wrapper maintains an active subscription to the underlying store
- Memory usage grows unbounded as components mount and unmount
**Fix needed:**
The effect cleanup (returned by `createSubscription`) must call `root.storeTracker.remove(store)` to decrement the reference count. However, this requires careful handling to avoid double-decrement if the effect runs multiple times.
How can I resolve this? If you propose a fix, please make it concise.| handleUpdate(action: A) { | ||
| const transitionState = this._headState; | ||
| const currentState = this._committedState; | ||
| this._headState = this.store.getState(); | ||
|
|
||
| if (ReactSharedInternals.T !== null) { | ||
| // We are in a transition, update the transition state only | ||
| } else if (is(transitionState, currentState)) { | ||
| // We are updating sync and no transition is in progress, update both | ||
| this._committedState = this._headState; | ||
| } else { | ||
| // We are updating sync, but a transition is in progress. Implement | ||
| // React's update reordering semantics. | ||
| this._committedState = this.store.reducer(this._committedState, action); | ||
| } | ||
| // Notify all subscribed fibers | ||
| this._listeners.forEach(listener => listener()); | ||
| } |
There was a problem hiding this comment.
CRITICAL: No error handling for reducer exceptions causes state corruption
If store.reducer() throws an exception on line 45, the store state becomes permanently corrupted:
The sequence:
- Line 35:
_headStateis updated tostore.getState()✓ - Line 45:
reducer(_committedState, action)throws an exception ✗ _committedStateis NOT updated (still has old value)- Line 48: Listeners are still notified even though state is inconsistent
- Store now has divergent
_headStateand_committedStateforever
Impact:
- Transition state and committed state permanently desynchronize
- Different React roots rendering different lanes will see incorrect state
- Subsequent updates will compound the corruption
- No way to recover without recreating the entire store
Additional issue:
Line 48 also has no error handling. If a listener callback throws, subsequent listeners won't execute, breaking component updates.
Fix needed:
Wrap reducer call in try-catch. On error, revert _headState or ensure both states remain consistent. Consider wrapping listener calls individually to ensure all listeners execute even if one throws.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-reconciler/src/ReactFiberStoreTracking.js
Line: 32:49
Comment:
**CRITICAL: No error handling for reducer exceptions causes state corruption**
If `store.reducer()` throws an exception on line 45, the store state becomes permanently corrupted:
**The sequence:**
1. Line 35: `_headState` is updated to `store.getState()` ✓
2. Line 45: `reducer(_committedState, action)` throws an exception ✗
3. `_committedState` is NOT updated (still has old value)
4. Line 48: Listeners are still notified even though state is inconsistent
5. Store now has divergent `_headState` and `_committedState` forever
**Impact:**
- Transition state and committed state permanently desynchronize
- Different React roots rendering different lanes will see incorrect state
- Subsequent updates will compound the corruption
- No way to recover without recreating the entire store
**Additional issue:**
Line 48 also has no error handling. If a listener callback throws, subsequent listeners won't execute, breaking component updates.
**Fix needed:**
Wrap reducer call in try-catch. On error, revert `_headState` or ensure both states remain consistent. Consider wrapping listener calls individually to ensure all listeners execute even if one throws.
How can I resolve this? If you propose a fix, please make it concise.| dispatch(action: A) { | ||
| state = actualReducer(state, action); | ||
| subscriptions.forEach(callback => callback(action)); | ||
| }, |
There was a problem hiding this comment.
Subscription callback errors prevent subsequent notifications
If a subscription callback throws an exception, forEach will stop iterating and subsequent callbacks won't execute. This breaks component updates for any components that subscribe after the failing callback.
Example scenario:
const store = createStore(0);
store.subscribe(() => { throw new Error('Oops'); }); // First subscriber
store.subscribe(() => { /* Update component */ }); // Second subscriber
store.dispatch(1); // Second subscriber never called!Impact:
- One misbehaving subscriber can break all other subscribers
- Components will fail to update when store changes
- Difficult to debug as error location is in user code but impact affects the framework
Recommendation:
Wrap each callback invocation in try-catch to ensure all subscribers are notified even if one fails. Consider logging errors in development mode.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-reconciler/src/ReactStore.js
Line: 45:48
Comment:
**Subscription callback errors prevent subsequent notifications**
If a subscription callback throws an exception, `forEach` will stop iterating and subsequent callbacks won't execute. This breaks component updates for any components that subscribe after the failing callback.
**Example scenario:**
```javascript
const store = createStore(0);
store.subscribe(() => { throw new Error('Oops'); }); // First subscriber
store.subscribe(() => { /* Update component */ }); // Second subscriber
store.dispatch(1); // Second subscriber never called!
```
**Impact:**
- One misbehaving subscriber can break all other subscribers
- Components will fail to update when store changes
- Difficult to debug as error location is in user code but impact affects the framework
**Recommendation:**
Wrap each callback invocation in try-catch to ensure all subscribers are notified even if one fails. Consider logging errors in development mode.
How can I resolve this? If you propose a fix, please make it concise.| previousArgs.selector !== actualSelector || | ||
| previousArgs.wrapper !== wrapper |
There was a problem hiding this comment.
Inline selector functions cause unnecessary re-subscriptions
The selector comparison uses reference equality (!==), so passing inline functions will cause the subscription to be recreated on every render:
// This will recreate subscription every render:
function MyComponent() {
const count = useStore(store, state => state.count);
// ...
}Impact:
- Performance degradation with frequent mount/unmount of effect
- Unnecessary work scheduling updates that immediately cancel
- Goes against the ergonomic goal of the API
Note: This is acknowledged in the PR description under "Unsolved Problems - Custom equality". The RFC mentions this needs to be addressed before shipping, either by:
- Accepting a third
isEqualparameter - Calling selector with both current and previous state values
This is acceptable for a proof-of-concept but should be resolved before production use.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-reconciler/src/ReactFiberHooks.js
Line: 1923:1924
Comment:
**Inline selector functions cause unnecessary re-subscriptions**
The selector comparison uses reference equality (`!==`), so passing inline functions will cause the subscription to be recreated on every render:
```javascript
// This will recreate subscription every render:
function MyComponent() {
const count = useStore(store, state => state.count);
// ...
}
```
**Impact:**
- Performance degradation with frequent mount/unmount of effect
- Unnecessary work scheduling updates that immediately cancel
- Goes against the ergonomic goal of the API
**Note:** This is acknowledged in the PR description under "Unsolved Problems - Custom equality". The RFC mentions this needs to be addressed before shipping, either by:
1. Accepting a third `isEqual` parameter
2. Calling selector with both current and previous state values
This is acceptable for a proof-of-concept but should be resolved before production use.
How can I resolve this? If you propose a fix, please make it concise.| /** | ||
| * Copyright (c) Meta Platforms, Inc. and affiliates. | ||
| * | ||
| * This source code is licensed under the MIT license found in the | ||
| * LICENSE file in the root directory of this source tree. | ||
| * | ||
| * @emails react-core | ||
| * @gate enableStore | ||
| */ | ||
|
|
||
| 'use strict'; | ||
|
|
||
| let useStore; | ||
| let React; | ||
| let ReactNoop; | ||
| let Scheduler; | ||
| let act; | ||
| let createStore; | ||
| let startTransition; | ||
| let waitFor; | ||
| let assertLog; | ||
| let useLayoutEffect; | ||
| let useEffect; | ||
| let use; | ||
| let Suspense; | ||
|
|
||
| describe('useStore', () => { | ||
| beforeEach(() => { | ||
| jest.resetModules(); | ||
|
|
||
| act = require('internal-test-utils').act; | ||
| React = require('react'); | ||
| ReactNoop = require('react-noop-renderer'); | ||
| Scheduler = require('scheduler'); | ||
| createStore = React.createStore; | ||
| useStore = React.useStore; | ||
| useLayoutEffect = React.useLayoutEffect; | ||
| useEffect = React.useEffect; | ||
| use = React.use; | ||
| Suspense = React.Suspense; | ||
| startTransition = React.startTransition; | ||
| const InternalTestUtils = require('internal-test-utils'); | ||
| waitFor = InternalTestUtils.waitFor; | ||
| assertLog = InternalTestUtils.assertLog; | ||
| }); | ||
|
|
||
| it('simplest use', async () => { | ||
| const store = createStore(2); | ||
|
|
||
| function App() { |
There was a problem hiding this comment.
Missing test coverage for error handling scenarios
The test suite has comprehensive coverage of basic functionality and transition semantics, but is completely missing tests for error handling:
Critical scenarios not tested:
- Reducer throws exception - Should test that store state doesn't corrupt when reducer throws
- Subscription callback throws - Should verify other subscribers still get notified
- Selector throws during render - Should handle gracefully
- Memory leak verification - Should test that StoreWrapper is disposed when no longer referenced
- Invalid dispatch during render - Should test error handling for common mistakes
Example missing test:
it('handles reducer errors without corrupting state', () => {
const buggyReducer = (state, action) => {
if (action.type === 'error') throw new Error('Oops');
return state + 1;
};
const store = createStore(0, buggyReducer);
// Test that store recovers or handles error appropriately
});These tests are essential for validating the robustness of the implementation before production use.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-reconciler/src/__tests__/useStore-test.js
Line: 1:50
Comment:
**Missing test coverage for error handling scenarios**
The test suite has comprehensive coverage of basic functionality and transition semantics, but is completely missing tests for error handling:
**Critical scenarios not tested:**
1. **Reducer throws exception** - Should test that store state doesn't corrupt when reducer throws
2. **Subscription callback throws** - Should verify other subscribers still get notified
3. **Selector throws during render** - Should handle gracefully
4. **Memory leak verification** - Should test that StoreWrapper is disposed when no longer referenced
5. **Invalid dispatch during render** - Should test error handling for common mistakes
**Example missing test:**
```javascript
it('handles reducer errors without corrupting state', () => {
const buggyReducer = (state, action) => {
if (action.type === 'error') throw new Error('Oops');
return state + 1;
};
const store = createStore(0, buggyReducer);
// Test that store recovers or handles error appropriately
});
```
These tests are essential for validating the robustness of the implementation before production use.
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (5)
The RFC explicitly specifies: function createStore<S, A>(
initialValue: S,
reducer?: (S, A) => S
): ReactStore<S, A>;But this implementation has THREE CRITICAL PROBLEMS:
This breaks the documented example The test file at lines 53 and 100 uses Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react/src/ReactStore.js
Line: 14:16
Comment:
[P0] **CRITICAL: API signature contradicts RFC specification**
The RFC explicitly specifies:
```typescript
function createStore<S, A>(
initialValue: S,
reducer?: (S, A) => S
): ReactStore<S, A>;
```
But this implementation has **THREE CRITICAL PROBLEMS**:
1. **Parameters reversed**: `reducer` comes first, `initialValue` second
2. **Reducer is required**: Should be optional per RFC
3. **Missing setState-style functionality**: RFC states "If a reducer is not provided, the dispatch method mirrors `setState` in that it accepts either a new value, or an updater function"
This breaks the documented example `createStore(1)` from the RFC description, making the API unusable as specified. The RFC's design goal of echoing `useState`/`useReducer` patterns (where initial value comes first) is not met.
The test file at lines 53 and 100 uses `createStore(counterReducer, 2)` which matches this broken implementation but contradicts the documented API.
How can I resolve this? If you propose a fix, please make it concise.
The RFC's subscribe: (callback: (action: A) => void) => () => voidThe callback should receive the dispatched action, but this implementation calls Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react/src/ReactStore.js
Line: 37:37
Comment:
[P1] **Subscribe callback doesn't receive action parameter**
The RFC's `ReactExternalDataSource` interface specifies:
```typescript
subscribe: (callback: (action: A) => void) => () => void
```
The callback should receive the dispatched action, but this implementation calls `callback()` with no arguments. This prevents subscribers from knowing which action triggered the update, limiting debugging and observability capabilities.
How can I resolve this? If you propose a fix, please make it concise.
The RFC defines export interface ReactExternalDataSource<S, A> {
getState(): S,
reducer: (S, A) => S,
subscribe: (callback: (action: A) => void) => () => void,
};This type definition is missing:
This prevents external libraries from implementing compatible data sources as described in the RFC. Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/shared/ReactTypes.js
Line: 385:391
Comment:
[P1] **Type definition doesn't match `ReactExternalDataSource` interface**
The RFC defines `ReactExternalDataSource` with:
```typescript
export interface ReactExternalDataSource<S, A> {
getState(): S,
reducer: (S, A) => S,
subscribe: (callback: (action: A) => void) => () => void,
};
```
This type definition is missing:
- `getState()` method (exposes internal `_current`/`_transition` instead)
- Correct `subscribe` callback signature (should be `(action: A) => void`)
- Public `reducer` property (exposes `_reducer` which is private by naming convention)
This prevents external libraries from implementing compatible data sources as described in the RFC.
How can I resolve this? If you propose a fix, please make it concise.
The Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react-reconciler/src/__tests__/useStoreWithSelector-test.js
Line: 56:56
Comment:
[P2] **`identify` function used before declaration**
The `identify` function is referenced here but not defined until line 145. This will cause a ReferenceError at runtime.
How can I resolve this? If you propose a fix, please make it concise.
The test dispatches Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react-reconciler/src/__tests__/useStoreWithSelector-test.js
Line: 94:94
Comment:
[P2] **Test dispatches unhandled action type**
The test dispatches `{type: 'double'}` but the reducer at lines 40-51 only handles `'increment'` and `'decrement'`. The `'double'` case is defined in the second test's reducer (lines 87-98) but used in the wrong test context.
How can I resolve this? If you propose a fix, please make it concise. |
| return storeWrapper.subscribe( | ||
| handleStoreSubscriptionChange.bind( | ||
| null, | ||
| fiber, | ||
| queue, | ||
| storeWrapper, | ||
| selector, | ||
| ), | ||
| ); | ||
| } |
There was a problem hiding this comment.
CRITICAL: Memory leak - reference count never decremented
The createSubscription function returns an unsubscribe function from storeWrapper.subscribe(), which only removes the listener from the wrapper's internal _listeners Set. However, it NEVER calls root.storeTracker.remove(store) to decrement the reference count.
The problem:
- When
mountStore/updateStorecallsgetWrapper()(line 1844/1895), it increments the reference count - When the component unmounts, the effect cleanup only calls the returned unsubscribe function
- The unsubscribe only does
this._listeners.delete(callback)(from StoreWrapper.subscribe) - The reference count is never decremented
- The wrapper's
dispose()is never called, keeping the subscription to the underlying store alive - Both the wrapper and store subscription leak permanently
Fix needed:
The effect cleanup must also call root.storeTracker.remove(store) to properly decrement the reference count and allow the wrapper to be disposed when no longer needed.
Note: I verified this by searching the entire codebase - there are zero calls to storeTracker.remove() anywhere.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-reconciler/src/ReactFiberHooks.js
Line: 1973:1982
Comment:
**CRITICAL: Memory leak - reference count never decremented**
The `createSubscription` function returns an unsubscribe function from `storeWrapper.subscribe()`, which only removes the listener from the wrapper's internal `_listeners` Set. However, it **NEVER** calls `root.storeTracker.remove(store)` to decrement the reference count.
**The problem:**
1. When `mountStore`/`updateStore` calls `getWrapper()` (line 1844/1895), it increments the reference count
2. When the component unmounts, the effect cleanup only calls the returned unsubscribe function
3. The unsubscribe only does `this._listeners.delete(callback)` (from StoreWrapper.subscribe)
4. The reference count is **never decremented**
5. The wrapper's `dispose()` is never called, keeping the subscription to the underlying store alive
6. Both the wrapper and store subscription **leak permanently**
**Fix needed:**
The effect cleanup must also call `root.storeTracker.remove(store)` to properly decrement the reference count and allow the wrapper to be disposed when no longer needed.
Note: I verified this by searching the entire codebase - there are **zero** calls to `storeTracker.remove()` anywhere.
How can I resolve this? If you propose a fix, please make it concise.| dispatch(action: A) { | ||
| state = actualReducer(state, action); | ||
| subscriptions.forEach(callback => callback(action)); | ||
| }, |
There was a problem hiding this comment.
CRITICAL: State corruption if reducer throws
If actualReducer throws an exception on line 46, the state variable will be left in an inconsistent state and subscribers will not be notified.
Scenarios:
- If reducer throws after mutating state (bug in reducer), state is corrupted
- If reducer reads
statesuccessfully then throws,stateretains old value but may have been partially computed - Subscribers are never notified of the failed update attempt
Impact:
- Store state becomes inconsistent with what reducers expect
- Components observing the store won't re-render even though an action was dispatched
- Difficult to debug since the error is thrown but state is left in limbo
Fix needed:
Wrap the state update in try-catch, or ensure state is only assigned if reducer succeeds:
const newState = actualReducer(state, action);
state = newState;
subscriptions.forEach(callback => callback(action));This ensures atomicity - either the update fully succeeds or state remains unchanged.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-reconciler/src/ReactStore.js
Line: 45:48
Comment:
**CRITICAL: State corruption if reducer throws**
If `actualReducer` throws an exception on line 46, the `state` variable will be left in an **inconsistent state** and subscribers will **not** be notified.
**Scenarios:**
1. If reducer throws **after** mutating state (bug in reducer), state is corrupted
2. If reducer reads `state` successfully then throws, `state` retains old value but may have been partially computed
3. Subscribers are never notified of the failed update attempt
**Impact:**
- Store state becomes inconsistent with what reducers expect
- Components observing the store won't re-render even though an action was dispatched
- Difficult to debug since the error is thrown but state is left in limbo
**Fix needed:**
Wrap the state update in try-catch, or ensure state is only assigned if reducer succeeds:
```javascript
const newState = actualReducer(state, action);
state = newState;
subscriptions.forEach(callback => callback(action));
```
This ensures atomicity - either the update fully succeeds or state remains unchanged.
How can I resolve this? If you propose a fix, please make it concise.| // Notify all subscribed fibers | ||
| this._listeners.forEach(listener => listener()); | ||
| } |
There was a problem hiding this comment.
ERROR: Listener notification lacks error isolation
If any listener callback throws an exception, the forEach will terminate and subsequent listeners will not be notified.
The problem:
- Line 48:
this._listeners.forEach(listener => listener()) - If listener Run DevTools tests against npm releases #2 throws, listeners [Experiment] Context Selectors #3, devtools: Add
refto inspect element view #4, etc. never execute - This breaks the contract that all subscribers should be notified of state changes
Impact:
- Components may not re-render even though store state changed
- One buggy component can break all downstream components
- Difficult to debug since only some subscribers are notified
Fix needed:
Wrap each callback invocation in try-catch to ensure all listeners are always notified:
this._listeners.forEach(listener => {
try {
listener();
} catch (error) {
// Log error but continue notifying other listeners
console.error('Store listener threw an error:', error);
}
});The same issue exists in ReactStore.js line 47.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-reconciler/src/ReactFiberStoreTracking.js
Line: 47:49
Comment:
**ERROR: Listener notification lacks error isolation**
If any listener callback throws an exception, the `forEach` will terminate and **subsequent listeners will not be notified**.
**The problem:**
- Line 48: `this._listeners.forEach(listener => listener())`
- If listener #2 throws, listeners #3, #4, etc. never execute
- This breaks the contract that all subscribers should be notified of state changes
**Impact:**
- Components may not re-render even though store state changed
- One buggy component can break all downstream components
- Difficult to debug since only some subscribers are notified
**Fix needed:**
Wrap each callback invocation in try-catch to ensure all listeners are always notified:
```javascript
this._listeners.forEach(listener => {
try {
listener();
} catch (error) {
// Log error but continue notifying other listeners
console.error('Store listener threw an error:', error);
}
});
```
The same issue exists in `ReactStore.js` line 47.
How can I resolve this? If you propose a fix, please make it concise.| // React's update reordering semantics. | ||
| this._committedState = this.store.reducer(this._committedState, action); | ||
| } |
There was a problem hiding this comment.
POTENTIAL ISSUE: Reducer called with action during sync rebase
In the sync update during transition case (else branch), line 45 applies the action to _committedState using the store's reducer. However, this assumes the store's reducer is:
- Pure (no side effects)
- Deterministic (same state + action = same result)
- Safe to call multiple times with the same action
Concern:
If the store's reducer has side effects or is not idempotent, calling it again here could cause issues. While the RFC states "Reducer must be pure", this is not enforced and may surprise users.
Consider:
- Document this behavior clearly - reducers will be called multiple times during transitions
- Add DEV-mode warnings if reducer appears to have side effects
- Consider if there's a way to validate reducer purity
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-reconciler/src/ReactFiberStoreTracking.js
Line: 44:46
Comment:
**POTENTIAL ISSUE: Reducer called with action during sync rebase**
In the sync update during transition case (else branch), line 45 applies the `action` to `_committedState` using the store's reducer. However, this assumes the store's reducer is:
1. **Pure** (no side effects)
2. **Deterministic** (same state + action = same result)
3. **Safe to call multiple times** with the same action
**Concern:**
If the store's reducer has side effects or is not idempotent, calling it again here could cause issues. While the RFC states "Reducer must be pure", this is not enforced and may surprise users.
**Consider:**
- Document this behavior clearly - reducers will be called multiple times during transitions
- Add DEV-mode warnings if reducer appears to have side effects
- Consider if there's a way to validate reducer purity
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| /** | ||
| * Copyright (c) Meta Platforms, Inc. and affiliates. | ||
| * | ||
| * This source code is licensed under the MIT license found in the | ||
| * LICENSE file in the root directory of this source tree. | ||
| * | ||
| * @emails react-core | ||
| * @gate enableStore | ||
| */ | ||
|
|
||
| 'use strict'; | ||
|
|
||
| let useStore; | ||
| let React; | ||
| let ReactNoop; | ||
| let Scheduler; | ||
| let act; | ||
| let createStore; | ||
| let startTransition; | ||
| let waitFor; | ||
| let assertLog; | ||
| let useLayoutEffect; | ||
| let useEffect; | ||
| let use; | ||
| let Suspense; | ||
|
|
||
| describe('useStore', () => { | ||
| beforeEach(() => { | ||
| jest.resetModules(); | ||
|
|
||
| act = require('internal-test-utils').act; | ||
| React = require('react'); | ||
| ReactNoop = require('react-noop-renderer'); | ||
| Scheduler = require('scheduler'); | ||
| createStore = React.createStore; | ||
| useStore = React.useStore; | ||
| useLayoutEffect = React.useLayoutEffect; | ||
| useEffect = React.useEffect; | ||
| use = React.use; | ||
| Suspense = React.Suspense; | ||
| startTransition = React.startTransition; | ||
| const InternalTestUtils = require('internal-test-utils'); | ||
| waitFor = InternalTestUtils.waitFor; | ||
| assertLog = InternalTestUtils.assertLog; | ||
| }); |
There was a problem hiding this comment.
Missing critical test coverage
The test suite covers basic functionality but is missing tests for several important edge cases and known issues mentioned in the RFC:
Missing tests:
- Memory management: No tests verify that components unmounting properly clean up store references
- Error handling: No tests for reducer throwing errors or subscription callbacks throwing
- Selector stability: No tests for unstable selector functions (inline functions) causing unnecessary re-renders
- Concurrent edge cases: The RFC mentions "mounting mid-transition" observing wrong state - no test reproducing this
- Multiple roots: No tests for multiple React roots observing the same store
- Custom equality: RFC mentions this as "unsolved problem" but no tests exist
- Selector optimization: No tests verifying selectors prevent re-renders when selected value unchanged
- Transition rebasing: No tests for sync dispatch during pending transition (the complex rebasing logic)
Recommendation:
Add comprehensive test coverage for error cases and the known limitations before merging. This will help validate fixes and prevent regressions.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-reconciler/src/__tests__/useStore-test.js
Line: 1:45
Comment:
**Missing critical test coverage**
The test suite covers basic functionality but is missing tests for several important edge cases and known issues mentioned in the RFC:
**Missing tests:**
1. **Memory management**: No tests verify that components unmounting properly clean up store references
2. **Error handling**: No tests for reducer throwing errors or subscription callbacks throwing
3. **Selector stability**: No tests for unstable selector functions (inline functions) causing unnecessary re-renders
4. **Concurrent edge cases**: The RFC mentions "mounting mid-transition" observing wrong state - no test reproducing this
5. **Multiple roots**: No tests for multiple React roots observing the same store
6. **Custom equality**: RFC mentions this as "unsolved problem" but no tests exist
7. **Selector optimization**: No tests verifying selectors prevent re-renders when selected value unchanged
8. **Transition rebasing**: No tests for sync dispatch during pending transition (the complex rebasing logic)
**Recommendation:**
Add comprehensive test coverage for error cases and the known limitations before merging. This will help validate fixes and prevent regressions.
How can I resolve this? If you propose a fix, please make it concise.
| actualSelector(storeState)); | ||
| } | ||
|
|
||
| return state; |
There was a problem hiding this comment.
Uses hardcoded SomeTransitionLane constant instead of determining the actual transition lane(s). This could cause incorrect priority handling for transition updates.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-reconciler/src/ReactFiberHooks.js
Line: 1931:1931
Comment:
Uses hardcoded `SomeTransitionLane` constant instead of determining the actual transition lane(s). This could cause incorrect priority handling for transition updates.
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (5)
function createStore<S, A>(
initialValue: S,
reducer?: (S, A) => S
): ReactStore<S, A>;But implementation has parameters reversed ( Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react/src/ReactStore.js
Line: 14:16
Comment:
API signature contradicts RFC specification. The RFC explicitly states:
```typescript
function createStore<S, A>(
initialValue: S,
reducer?: (S, A) => S
): ReactStore<S, A>;
```
But implementation has parameters reversed (`reducer` first, `initialValue` second) and `reducer` is required instead of optional. The RFC states: "If a reducer is not provided, the dispatch method mirrors `setState` in that it accepts either a new value, or an updater function." This functionality is completely missing, making the example `createStore(1)` from the RFC impossible to use.
How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react/src/ReactStore.js
Line: 39:39
Comment:
Subscribe callback doesn't pass the action parameter. The RFC's `ReactExternalDataSource` interface specifies: `subscribe: (callback: (action: A) => void)` but implementation calls `callback()` with no arguments. This prevents subscribers from knowing which action triggered the update.
How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/shared/ReactTypes.js
Line: 385:391
Comment:
Type definition doesn't match RFC's `ReactExternalDataSource` interface. Missing `getState(): S` method - the RFC states this should return current state. Also exposes internal implementation details (`_current`, `_transition`, `_reducer`) as public API which prevents future refactoring.
How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react-reconciler/src/__tests__/useStoreWithSelector-test.js
Line: 56:56
Comment:
`identify` function used before it's defined. It's defined at line 145 but used here, which will cause a ReferenceError at runtime.
How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react-reconciler/src/__tests__/useStoreWithSelector-test.js
Line: 94:94
Comment:
Test dispatches `{type: 'double'}` action but the reducer (lines 87-98) only handles `'increment'` and `'decrement'`. The `'double'` case falls through to default, returning the unchanged count instead of doubling it.
How can I resolve this? If you propose a fix, please make it concise. |
| @@ -96,6 +99,7 @@ export { | |||
| useOptimistic, | |||
| useActionState, | |||
| useSyncExternalStore, | |||
There was a problem hiding this comment.
exported as useStoreWithSelector but RFC specifies it should be useStore. The RFC description shows the API as useStore(store, selector?) where selector is optional. Consider aligning the exported name with the RFC specification
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react/src/ReactClient.js
Line: 101:101
Comment:
exported as `useStoreWithSelector` but RFC specifies it should be `useStore`. The RFC description shows the API as `useStore(store, selector?)` where selector is optional. Consider aligning the exported name with the RFC specification
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (6)
function createStore<S, A>(
initialValue: S,
reducer?: (S, A) => S
): ReactStore<S, A>;But implementation has parameters reversed ( Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react/src/ReactStore.js
Line: 14:16
Comment:
API signature contradicts RFC specification. The RFC explicitly states:
```typescript
function createStore<S, A>(
initialValue: S,
reducer?: (S, A) => S
): ReactStore<S, A>;
```
But implementation has parameters reversed (`reducer` first, `initialValue` second) and `reducer` is required instead of optional. The RFC states: "If a reducer is not provided, the dispatch method mirrors `setState` in that it accepts either a new value, or an updater function." This functionality is completely missing, making the example `createStore(1)` from the RFC impossible to use.
How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react/src/ReactStore.js
Line: 39:39
Comment:
subscribe callback doesn't receive the action parameter as specified in the `ReactExternalDataSource` interface. The RFC states: `subscribe: (callback: (action: A) => void)` but implementation calls `callback()` with no arguments
How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/shared/ReactTypes.js
Line: 385:391
Comment:
`ReactStore` type doesn't match the `ReactExternalDataSource` interface from RFC description. Missing `getState(): S` method - the RFC interface specifies this method should return the current state. The `_current`, `_transition`, and `_reducer` fields appear to be internal implementation details that shouldn't be part of the public type
How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react-reconciler/src/__tests__/useStoreWithSelector-test.js
Line: 56:56
Comment:
`identify` function is undefined at this point - it's defined at line 145 but used here. This will cause a ReferenceError when the test runs
How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react-reconciler/src/__tests__/useStoreWithSelector-test.js
Line: 94:94
Comment:
`'double'` action is not handled in the reducer defined at lines 87-98 - only `'increment'` and default cases are handled, so this will fall through to the default case and not double the value as expected
How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react-reconciler/src/__tests__/useStoreWithSelector-test.js
Line: 53:53
Comment:
test uses `createStore(counterReducer, 2)` which matches the current implementation but contradicts the RFC specification. If the API is corrected to match RFC (`createStore(initialValue, reducer?)`), these tests will need updating
How can I resolve this? If you propose a fix, please make it concise. |
| state = actualReducer(state, action); | ||
| subscriptions.forEach(callback => callback(action)); |
There was a problem hiding this comment.
if reducer throws, state updates but subscribers still get notified with potentially inconsistent state - consider wrapping in try-catch
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-reconciler/src/ReactStore.js
Line: 46:47
Comment:
if reducer throws, state updates but subscribers still get notified with potentially inconsistent state - consider wrapping in try-catch
How can I resolve this? If you propose a fix, please make it concise.| reducer: actualReducer, | ||
| dispatch(action: A) { | ||
| state = actualReducer(state, action); | ||
| subscriptions.forEach(callback => callback(action)); |
There was a problem hiding this comment.
if a subscriber throws during notification, remaining subscribers won't execute - consider wrapping callbacks in try-catch for resilience
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-reconciler/src/ReactStore.js
Line: 47:47
Comment:
if a subscriber throws during notification, remaining subscribers won't execute - consider wrapping callbacks in try-catch for resilience
How can I resolve this? If you propose a fix, please make it concise.| this._committedState = this.store.reducer(this._committedState, action); | ||
| } | ||
| // Notify all subscribed fibers | ||
| this._listeners.forEach(listener => listener()); |
There was a problem hiding this comment.
if a listener throws, remaining listeners won't execute - same issue as in ReactStore.js dispatch
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-reconciler/src/ReactFiberStoreTracking.js
Line: 48:48
Comment:
if a listener throws, remaining listeners won't execute - same issue as in ReactStore.js dispatch
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (3)
function createStore<S, A>(
initialValue: S,
reducer?: (S, A) => S
): ReactStore<S, A>;But implementation has parameters reversed ( Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react/src/ReactStore.js
Line: 14:16
Comment:
API signature contradicts RFC specification. The RFC explicitly states:
```typescript
function createStore<S, A>(
initialValue: S,
reducer?: (S, A) => S
): ReactStore<S, A>;
```
But implementation has parameters reversed (`reducer` first, `initialValue` second) and `reducer` is required instead of optional. The RFC states: "If a reducer is not provided, the dispatch method mirrors `setState` in that it accepts either a new value, or an updater function." This functionality is completely missing, making the example `createStore(1)` from the RFC impossible to use.
How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/react/src/ReactStore.js
Line: 37:37
Comment:
Subscription callbacks don't receive the action parameter as specified in the `ReactExternalDataSource` interface. The RFC states: `subscribe: (callback: (action: A) => void)` - callbacks should receive the dispatched action.
How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/shared/ReactTypes.js
Line: 385:391
Comment:
`ReactStore` type doesn't match the `ReactExternalDataSource` interface from RFC. Missing `getState(): S` method, and `reducer` should be exposed as a property (not `_reducer`). The underscore-prefixed properties suggest internal implementation details leaking into the public API.
How can I resolve this? If you propose a fix, please make it concise. |
| root.storeTracker = new StoreTracker(); | ||
| } | ||
|
|
||
| const wrapper = root.storeTracker.getWrapper(store); |
There was a problem hiding this comment.
memory leak: getWrapper() increments reference count but there's no corresponding decrement on unmount
When mountStore calls getWrapper(), it increments the reference count in StoreTracker. However, when the component unmounts, the effect cleanup (returned by createSubscription) only calls storeWrapper.subscribe()'s unsubscribe function, which removes the listener from _listeners but does NOT decrement the reference count.
This means:
- Reference count never reaches 0
wrapper.dispose()is never called- Store subscription persists indefinitely
- Memory leak accumulates as components mount/unmount
Fix needed: The effect cleanup should also call root.storeTracker.remove(store) to properly decrement the reference count
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-reconciler/src/ReactFiberHooks.js
Line: 1844:1844
Comment:
memory leak: `getWrapper()` increments reference count but there's no corresponding decrement on unmount
When `mountStore` calls `getWrapper()`, it increments the reference count in `StoreTracker`. However, when the component unmounts, the effect cleanup (returned by `createSubscription`) only calls `storeWrapper.subscribe()`'s unsubscribe function, which removes the listener from `_listeners` but does NOT decrement the reference count.
This means:
1. Reference count never reaches 0
2. `wrapper.dispose()` is never called
3. Store subscription persists indefinitely
4. Memory leak accumulates as components mount/unmount
Fix needed: The effect cleanup should also call `root.storeTracker.remove(store)` to properly decrement the reference count
How can I resolve this? If you propose a fix, please make it concise.
|
Greptile encountered an error while reviewing this PR. Please reach out to support@greptile.com for assistance. |
2 participants
Mirror of facebook/react#35449
Original author: captbaritone
Note: This is a proof of concept for discussion purposes. API is subject to change and may not ship in this form.
ReactExternalDataSourceinterface, clarified behavior ofgetStateand explained how we avoid needing to tell the data source about React commits.Many libraries in the React ecosystem exist to allow React components to observe non-React state. As of today there is no way for such a library to do this in a way that is acceptably performant while also taking advantage of React transition semantics. This RFC proposes a new API for modeling state that is not conceptually owned by any one React component but which can be observed by multiple React components in a way that avoids unnecessary rerenders while correctly implementing React’s transition semantics.
Proposed API
At a high level, this proposal consist of an interface which joins two APIs:
Interface
ReactExternalDataSourceThe
ReactExternalDataSourceinterface describes a unit of dynamic non-React state that can be observed in React usinguseStore. In advanced use-cases store/cache libraries could implement this API to allow their state to be safely consumed in React components.Note that
getState()here must always returns the “current” version of the data source (the result of applying all the actions in the order in which they were triggered) irrespective of any React transition. Different React roots may be rendering different states at any given time, but the data source itself does not need to be aware of that.useStoreuseStoreis designed to echouseContext. It allows users to observe a store’s value, or a slice/selection of that store value, in a React component. Notably it avoids triggering a render if the observed portion of the store has not changed while still correctly implementing React’s transition semantics including avoiding tearing and correctly implementing React’s update reordering semantics.createStoreFor simple use cases, React provides it’s own simple (~20loc) store implementation which implements
ReactExternalDataSource.createStoreis designed to echouseStateanduseReducer. If a reducer is not provided, thedispatchmethod of the store mirrorssetStatein that it accepts either a new value, or an updater function. If a reducer is provided, thedispatchmethod accepts a concrete action value which is passed to the reducer.The returned
ReactStoreobject implementsReactExternalDataSourceand also exposes a.dispatch(action)method.Example Usage
Implementation Details
The core problems that any implementation of a concurrent compatible store API must solve are:
To address these problems, the current implementation maintains a reference counted registry of currently observed stores at each React root. As stores update, each root is able to track which state updates are pending and which have flushed to the UI. When a new component mounts sync, it’s able to find the “current” state value for that store in its React root even if other updates are pending. When a sync update is triggered while a previous update is still pending within that root, the update (action + reducer) can be applied on top of the “current” state for that React root.
Note that the underlying data source does not need to be told when an update commits, and is never aware of any non-canonical states that React produces when sync updates interrupt transition updates. Those temporary states are created by React itself using:
source.reducer(comittedState, syncAction).(See “Unsolved Problems” below for some limitations associated with the ref counting portion of this approach).
For maintaining per-component state, this implementation currently leverages the internal
updateReducerImplfunction, but eagerly pre-computes each update’s value, and explicitly triggers transition fixups, instead of allowing the update queue to be computed during render. This is likely not the cleanest approach, but was minimally invasive while still allowing us to take advantage of the many battle tested detailed of theupdateReducerImplimplementation, which felt like a good balance for a proof of concept. I expect we will eventually want to either create a custom fork ofupdateReducerImplfor this API, or at least refactorupdateReducerImplso that it can be sensibly reused by this API.Existing Alternative Solutions
Today existing libraries are choosing between one of a few imperfect existing solutions:
useReducer+useContextthis works and is correct, but quickly becomes impractical in the real world due to the performance implications ofuseContexttriggering rerenders on every state update.useSyncExternalStorethis API allows the efficient observation of non-React state, but it prevents updates to stores from participating in React transitions.useEffect. This is fraught with challenges and I believe it’s impossible to fully match React’s transition semantics. I believe this is the closest anyone has been able to get, but it makes unsafe assumptions about React and still has known edge cases which are incorrect or have sub-optional performance.Unsolved Problems
The current implementation is not perfect. It has some known limitations, but I wanted to share it in its current state to get feedback before continuing with the work/discussion needed to address these issues. But, for the sake of completeness I’ve captured them here:
Lifetimes
In the edge case where a React root starts observing a store while that store is already updating as part of a transition, the current implementation will incorrectly observe the post-transition value of that store even though the transition is still pending. I believe resolving this issue would require all React roots to know about all stores that are updating as part of transitions, even if they are not observing that store. This is somewhat tricky to implement without interfering with the GC of either stores or React roots. I suspect there is a solution here using WeakRefs, but its possible there is a simpler approach that I am overlooking.
Custom equality
A key feature of this API is avoiding rerenders when the value observed by a component has not changed. To determine if the result of evaluating a selector has changed, we currently use
Object.iswhich is the standard in React. However, in many cases this is not sufficient, and libraries like Redux provide mechanisms for providing custom equality functions for determining if a selector’s value has changed. I believe our API should enable this behavior by either allowing the user provide a thirdisEqualargument touseStorewhen providing a selector, or by calling the selector with both the current state value and the previous state value.Neither of these are currently implemented since our current approach of leveraging
updateReducerImpldoes not allow us to easily “see” the currently rendered/rendering value for a given lane.Categorically avoiding unnecessary rerenders
The current implementation reuses the logic from
useReducerto model per-component state updates. This allows us to reuse a bunch of known-correct logic to get something working correctly quickly. However,useReducerhas known/accepted issues where it will trigger a rerender despite the value not actually changing. This may be less acceptable foruseStorethan it is foruseReducersinceuseStorewith a selector is specifically designed to avoid unnecessary renders.Updating the
useReducelogic to avoid useless renders would be nice, though it seems the task is tricky enough that previous attempts have been reverted and not reattempted.