Add react implementation of state management#124
Add react implementation of state management#124andrew-polk wants to merge 1 commit intoui-controllerfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a React adapter for the @ethnolib/state-management-core library, enabling React applications to use the framework-agnostic state management system. The implementation follows the pattern established by the existing Svelte adapter, providing a useField hook that binds Field objects to React state.
Key Changes
- Created new
@ethnolib/state-management-reactpackage with theuseFieldhook - Added build configuration (Vite, TypeScript, Vitest) following the existing pattern
- Included comprehensive README documentation explaining usage patterns
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| package-lock.json | Added dependencies for the new React state management package and updated Svelte package references |
| components/state-management/state-management-react/package.json | Package manifest defining the React adapter with build scripts and dev dependencies |
| components/state-management/state-management-react/index.ts | Main entry point exporting the useField hook |
| components/state-management/state-management-react/src/use-field.ts | Core implementation of the React hook for binding Field objects to React state |
| components/state-management/state-management-react/vite.config.ts | Vite build configuration for library mode |
| components/state-management/state-management-react/vitest.config.ts | Vitest test configuration |
| components/state-management/state-management-react/tsconfig.json | Base TypeScript configuration with React JSX support |
| components/state-management/state-management-react/tsconfig.lib.json | Library-specific TypeScript configuration for build output |
| components/state-management/state-management-react/README.md | Documentation explaining the state management system and usage patterns |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "@vitejs/plugin-react-swc": "^3.8.0", | ||
| "tsx": "^4.19.2", | ||
| "typescript": "^5.2.2" | ||
| }, |
There was a problem hiding this comment.
Missing peerDependencies for "react" (and optionally "react-dom"). Since this package is a React adapter that imports from "react", it should declare React as a peer dependency to ensure consumers have it installed. Consider adding:
"peerDependencies": {
"react": "^17.0.0 || ^18.0.0"
}| }, | |
| }, | |
| "peerDependencies": { | |
| "react": "^17.0.0 || ^18.0.0" | |
| }, |
| _setFieldValue(value); | ||
| } | ||
|
|
||
| field.updateUI = (value) => _setFieldValue(value); |
There was a problem hiding this comment.
The field.updateUI assignment on every render can cause issues. If the component re-renders (e.g., due to parent state changes), this assignment will overwrite the callback, potentially breaking synchronization. Consider using useEffect to set this up once:
useEffect(() => {
field.updateUI = (value) => _setFieldValue(value);
return () => {
field.updateUI = null; // cleanup
};
}, [field]);| export function useField<T>(field: Field<T>): [T, (value: T) => void] { | ||
| const [fieldValue, _setFieldValue] = useState(field.value); | ||
|
|
||
| function setFieldValue(value: T) { | ||
| field.requestUpdate(value); | ||
| _setFieldValue(value); | ||
| } | ||
|
|
||
| field.updateUI = (value) => _setFieldValue(value); | ||
|
|
||
| return [fieldValue, setFieldValue]; | ||
| } |
There was a problem hiding this comment.
The useField hook lacks test coverage. The sibling package state-management-svelte has tests (e.g., transform-view-model.test.ts) for similar functionality. Consider adding tests to verify:
- Initial state matches field value
- Updates through setFieldValue call field.requestUpdate
- Updates through field.updateUI update React state
- Cleanup of field.updateUI on unmount
| @@ -0,0 +1,108 @@ | |||
| # State Management Core | |||
|
|
|||
| This package provides a simple interface for working with multiple reactive framworks. | |||
There was a problem hiding this comment.
Typo: "framworks" should be "frameworks"
| This package provides a simple interface for working with multiple reactive framworks. | |
| This package provides a simple interface for working with multiple reactive frameworks. |
| const person = usePersonViewModel(); | ||
|
|
||
| // Replace `defineReactiveState` with your framework's mechanism | ||
| const reactiveName = defineReactiveSate(); |
There was a problem hiding this comment.
Typo: "defineReactiveSate" should be "defineReactiveState"
| const reactiveName = defineReactiveSate(); | |
| const reactiveName = defineReactiveState(); |
| export default defineConfig({ | ||
| root: __dirname, | ||
| cacheDir: | ||
| "../../../node_modules/.vite/components/state-management/state-management-core", |
There was a problem hiding this comment.
The cacheDir path references "state-management-core" but should reference "state-management-react" to match the actual package being built.
| "../../../node_modules/.vite/components/state-management/state-management-core", | |
| "../../../node_modules/.vite/components/state-management/state-management-react", |
| }, | ||
| lib: { | ||
| entry: "./index.ts", | ||
| name: "@ethnolib/find-language", |
There was a problem hiding this comment.
The library name is incorrectly set to "@ethnolib/find-language". It should be "@ethnolib/state-management-react" to match the package name.
| name: "@ethnolib/find-language", | |
| name: "@ethnolib/state-management-react", |
| "declarationMap": true | ||
| }, | ||
| "exclude": [ | ||
| "langtagProcessing.ts", |
There was a problem hiding this comment.
The exclude pattern references "langtagProcessing.ts" which doesn't exist in this package. This appears to be a leftover from a template and should be removed.
| "langtagProcessing.ts", |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| _setFieldValue(value); | ||
| } | ||
|
|
||
| field.updateUI = (value) => _setFieldValue(value); |
There was a problem hiding this comment.
🔴 Missing useEffect for field.updateUI assignment causes stale callback after unmount
The field.updateUI callback is assigned directly during render (line 12) rather than inside a useEffect. This means:
-
No cleanup on unmount: When the component using
useFieldunmounts,field.updateUIstill references the stale_setFieldValuefrom the unmounted component. If anything later setsfield.value(which triggersupdateUIvia the setter atcomponents/state-management/state-management-core/src/field.ts:42), it will callsetStateon an unmounted component. -
Side effect during render: Assigning
field.updateUImutates an external object during the render phase, violating React's rules about pure rendering.
Detailed explanation and comparison with Svelte adapter
The Svelte adapter (components/state-management/state-management-svelte/src/field.svelte.ts:9) sets field.updateUI in the constructor, which is appropriate for Svelte's lifecycle. However, in React, side effects that interact with external state must be placed in useEffect with proper cleanup.
The fix should wrap the assignment in useEffect and return a cleanup function:
useEffect(() => {
field.updateUI = (value) => _setFieldValue(value);
return () => { field.updateUI = null; };
}, [field]);Without this, if a component unmounts and the Field object persists (which is the expected pattern — Fields live in view models that outlive individual React components), subsequent field.value = x calls will attempt to update React state on an unmounted component. While React 18+ silently ignores this, it indicates a real resource leak where the Field holds a reference to a dead component's state setter.
Prompt for agents
In components/state-management/state-management-react/src/use-field.ts, the field.updateUI assignment on line 12 should be moved into a useEffect hook with proper cleanup. Add useEffect to the import from react on line 1, then replace line 12 with:
useEffect(() => {
field.updateUI = (value) => _setFieldValue(value);
return () => { field.updateUI = null; };
}, [field]);
This ensures the callback is properly set up after mount and cleaned up on unmount, preventing stale references to the setState function of an unmounted component.
Was this helpful? React with 👍 or 👎 to provide feedback.
| function setFieldValue(value: T) { | ||
| field.requestUpdate(value); | ||
| _setFieldValue(value); | ||
| } |
There was a problem hiding this comment.
🔴 setFieldValue calls _setFieldValue redundantly, risking state desync if onUpdateRequested modifies the field
In setFieldValue, _setFieldValue(value) on line 9 is called after field.requestUpdate(value) on line 8. However, requestUpdate already triggers _setFieldValue(value) via the updateUI callback (see components/state-management/state-management-core/src/field.ts:23-28 — requestUpdate sets this.value, whose setter at line 42 calls this.updateUI(value)).
How this causes state desync
The call chain when setFieldValue(value) is invoked:
field.requestUpdate(value)→this.value = value→this.updateUI(value)→_setFieldValue(value)✓_onUpdateRequested(value, oldValue)runs — if this callback modifiesfield.valueto a different value (e.g., normalization/validation), that triggersupdateUI(correctedValue)→_setFieldValue(correctedValue)✓- Back in
setFieldValue, line 9 calls_setFieldValue(value)with the original value, overwriting the corrected value from step 2.
While current usages of _onUpdateRequested in the codebase don't modify the same field they're attached to, this is a latent bug that will manifest as soon as any onUpdateRequested callback normalizes or validates the field's own value. The Svelte adapter (components/state-management/state-management-svelte/src/field.svelte.ts:19-22) does not have this issue because it sets _state before calling requestUpdate, and the redundant updateUI call sets _state to the same value.
The fix is to remove line 9 (_setFieldValue(value)) since requestUpdate already triggers the UI update via updateUI.
| function setFieldValue(value: T) { | |
| field.requestUpdate(value); | |
| _setFieldValue(value); | |
| } | |
| function setFieldValue(value: T) { | |
| field.requestUpdate(value); | |
| } | |
Was this helpful? React with 👍 or 👎 to provide feedback.
This change is