-
Notifications
You must be signed in to change notification settings - Fork 110
FE-205: Replace Zustand usage with React Context
#8258
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
PR SummaryState management overhaul
Simulation execution
Misc
Written by Cursor Bugbot for commit 4940b5f. This will update automatically on new commits. Configure here. |
🤖 Augment PR SummarySummary: This PR removes Zustand from Petrinaut and migrates editor/checker/simulation state management to plain React Context. Changes:
Technical Notes: The simulation execution loop is now implemented with component state, refs, and timeouts instead of an external store; ensure the new Context defaults and lifecycle behavior match prior expectations. 🤖 Was this summary useful? React with 👍 or 👎 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
de9e52b to
bc88dae
Compare
…ods and make __reinitialize internal
…s calling __reinitialize on petriNetId change
…ement, preventing race conditions during initialization and running of simulations. Improved error handling and state checks to enhance robustness.
e505985 to
c587c94
Compare
…e prop from ToolbarModes, streamlining props for better clarity and maintainability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| currentlyViewedFrame: 0, | ||
| // Keep initialMarking when resetting - it's configuration, not simulation state | ||
| })); | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent SDCPN read location may cause stale parameter values
Low Severity
The reset function reads getSDCPN() outside the setStateValues callback (line 361), while the nearly identical initializeParameterValuesFromDefaults function correctly reads it inside the callback (line 250). This inconsistency means reset could compute parameterValues from a stale SDCPN if the definition changes between the function call and when React processes the state update. The comment in initialize explicitly warns against this pattern: "Use functional update to ensure we see the latest state at processing time."

🌟 What is the purpose of this PR?
This PR removes Zustand for State Management.
Zustand was only used for Editor and Simulation (SDCPNContext was already a React context given the PetriNet definition/state is outside of the component).
Performance
React Contexts are a bit slower, but not much now given React Compiler does a lot of optimization of the consumer components. We will optimize performance later when it will make sense to do it, if needed.
Simulation Speed
This PR required to do some rewrite of SimulationProvider in more React-way, and I ended up doing frame generation by batch/budgeting.
So now instead of generating one frame every 20ms (maximum of 50 frames per second), we compute as much frames as we can during 5ms, every 50ms. (10% of main thread time allocated to simulation).
This results in much faster generation speed. (~5000 frames per second in Production Machines example)
This still runs in main thread, but once we move the Simulator in a Web Worker, we will be able to dedicate a thread to Simulation, and should end up with 10x more frames (50000 frames per second).
🔍 What does this change?
DEFAULT_CONTEXT_VALUE. This allows to easily isolate components and contexts for testing, without having to provide the whole context.use(Context)everywhere, without intermediary hook, to easily identify dependents.The idea being to split Interface/Implementation, and possibly leverage React Contexts as a Dependency Injection mechanism.
A Context could have multiple Providers in the future. (Will be useful for HASH simulation/live that will have to sync with server)
Pre-Merge Checklist 🚀
🚢 Has this modified a publishable library?
This PR:
📜 Does this require a change to the docs?
The changes in this PR:
🕸️ Does this require a change to the Turbo Graph?
The changes in this PR: