fix: replace direct state mutation in editor with proper store action#303
fix: replace direct state mutation in editor with proper store action#303
Conversation
Summary of ChangesHello @andeplane, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves an issue where the editor component was directly modifying the application's state, bypassing the established state management patterns. The changes refactor the file content update mechanism to leverage a dedicated store action, ensuring all state modifications are handled consistently and immutably. This enhancement improves data integrity, predictability, and optimizes React's rendering efficiency by preventing stale data issues. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request correctly addresses a critical issue of direct state mutation by introducing a proper easy-peasy action for updating file content. The changes ensure immutability, which fixes potential rendering bugs and aligns with best practices for state management. The implementation is clean and effectively resolves the problem described.
src/containers/Edit.tsx
Outdated
| const currentFile = | ||
| simulation?.files.find((file) => file.fileName === selectedFile?.fileName) ?? | ||
| selectedFile; |
There was a problem hiding this comment.
The current logic for finding currentFile uses optional chaining (?.) inside the .find() callback. If selectedFile is null or undefined, this will result in searching for a file where fileName is undefined, which is likely not the intended behavior and could lead to subtle bugs if a file without a name exists.
A more robust approach is to check for selectedFile's existence before accessing its fileName property within the predicate. This makes the logic clearer and safer.
const currentFile =
simulation?.files.find(
(file) => selectedFile && file.fileName === selectedFile.fileName,
) ?? selectedFile;
Fixes #300 The onEditorChange callback in Edit.tsx was directly mutating file.content on the simulation object, bypassing easy-peasy's immutability contract. - Added updateFileContent action to SimulationModel that immutably updates the matching file in simulation.files - Updated Edit.tsx to dispatch this action via useStoreActions - Editor now reads content from simulation.files (source of truth) instead of the stale selectedFile reference
3393404 to
952245d
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request correctly fixes a direct state mutation bug in Edit.tsx by introducing a proper updateFileContent action in the easy-peasy store. The change ensures state updates are immutable, which fixes potential rendering issues. The implementation is clean and effectively resolves the problem. I've added one suggestion for a potential future refactoring to further improve state management consistency.
src/containers/Edit.tsx
Outdated
| const currentFile = | ||
| simulation?.files.find((file) => file.fileName === selectedFile?.fileName) ?? | ||
| selectedFile; |
There was a problem hiding this comment.
This logic to find the currentFile is a good workaround for the fact that selectedFile (from state.app) can become stale after its content is updated in state.simulation. However, this indicates a potential design improvement.
To ensure a single source of truth and improve maintainability, consider refactoring the app state to store only the selectedFile.fileName. The Edit component could then always derive the full currentFile object from state.simulation.simulation.files.
This would eliminate the need for this workaround and make the state management more robust. Here's a conceptual example:
// In your component:
const selectedFileName = useStoreState((state) => state.app.selectedFileName); // Assuming app state stores only the name
const files = useStoreState((state) => state.simulation.simulation?.files);
const currentFile = files?.find((file) => file.fileName === selectedFileName);
// ... use currentFileThis is a suggestion for future improvement and may be out of scope for the current fix.
…rrentFile lookup
|
Addressed Gemini review: fixed the |
Fixes #300
Problem
Edit.tsxwas directly mutatingfile.contenton the simulation object in the store:This breaks easy-peasy's immutability contract, skips React re-renders for components observing
simulation.files, and is inconsistent with the rest of the codebase.Fix
updateFileContent({ fileName, content })action toSimulationModelthat updatessimulation.filesimmutably via.map()Edit.tsxnow dispatches this action viauseStoreActionssimulation.files(the real source of truth) rather than the staleselectedFilereference