Skip to content

fix: replace direct state mutation in editor with proper store action#303

Open
andeplane wants to merge 2 commits intomainfrom
fix/editor-state-mutation-300
Open

fix: replace direct state mutation in editor with proper store action#303
andeplane wants to merge 2 commits intomainfrom
fix/editor-state-mutation-300

Conversation

@andeplane
Copy link
Owner

Fixes #300

Problem

Edit.tsx was directly mutating file.content on the simulation object in the store:

const file = simulation?.files.find(...);
if (file) {
  file.content = newValue; // ← bypasses easy-peasy
}

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

  • Added updateFileContent({ fileName, content }) action to SimulationModel that updates simulation.files immutably via .map()
  • Edit.tsx now dispatches this action via useStoreActions
  • Editor reads file content from simulation.files (the real source of truth) rather than the stale selectedFile reference

@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello @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

  • State Management Consistency: Replaced direct mutation of file.content within Edit.tsx with a proper easy-peasy store action, ensuring all state updates adhere to the immutability contract.
  • New Store Action: Introduced a new updateFileContent action in src/store/simulation.ts to handle file content updates immutably, using a .map() operation to create a new array of files.
  • Editor Data Source: Modified the editor in Edit.tsx to read the current file content directly from the simulation.files state, ensuring it always reflects the true source of truth and triggers correct React re-renders.

🧠 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
  • src/containers/Edit.tsx
    • Refactored file content updates to dispatch the updateFileContent action via useStoreActions.
  • src/store/simulation.ts
    • Added updateFileContent action to immutably update file content within the simulation state.
Activity
  • No human activity has been recorded on this pull request yet.
Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +18 to +20
const currentFile =
simulation?.files.find((file) => file.fileName === selectedFile?.fileName) ??
selectedFile;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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
@andeplane andeplane force-pushed the fix/editor-state-mutation-300 branch from 3393404 to 952245d Compare February 26, 2026 14:02
@andeplane
Copy link
Owner Author

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +18 to +20
const currentFile =
simulation?.files.find((file) => file.fileName === selectedFile?.fileName) ??
selectedFile;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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 currentFile

This is a suggestion for future improvement and may be out of scope for the current fix.

@andeplane
Copy link
Owner Author

Addressed Gemini review: fixed the currentFile lookup to guard against selectedFile being null/undefined. Changed file.fileName === selectedFile?.fileName to selectedFile && file.fileName === selectedFile.fileName inside the .find() predicate, making the intent explicit and avoiding a subtle bug where a file without a name could match.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Editor changes bypass easy-peasy state management via direct mutation

1 participant