Implement continuous simulation loop

[andeplane]

Add the ability to continue a simulation after it finishes to allow users to run additional timesteps.

---

 

[cursor]

Cursor Agent can help with this pull request. Just @cursor in comments and I'll start working on changes in this branch.
_{Learn more about Cursor Agents}

[andeplane]

/gemini review

[gemini-code-assist]

Code Review

This pull request successfully implements the continuous simulation loop feature by adding a 'continue' button and the corresponding logic in the store. The changes are generally well-structured. However, I've identified some key areas for improvement. There is significant code duplication in src/store/simulation.ts between the run and continueSimulation thunks which should be refactored. The newly added tests in src/store/simulation.test.ts are not correctly testing the action implementations and need to be revised. I've also pointed out some smaller issues like the use of @ts-ignore and a magic number. Addressing these points will improve the code's maintainability and testability.

[gemini-code-assist]

These tests are not testing the actual implementation of the setFinished and reset actions from simulationModel. Instead, they re-implement the actions within the createStore call for each test. This means you are testing a mock implementation, not your production code.

To fix this, you should import the simulationModel and use it to create the store for your tests. This will ensure you are testing the real actions.

Here's an example of how you could restructure the tests:

import { createStore, Store } from 'easy-peasy';
import { simulationModel, SimulationModel } from './simulation';

describe('SimulationModel', () => {
  let store: Store<SimulationModel>;

  beforeEach(() => {
    // Create a fresh store for each test with the actual model
    store = createStore(simulationModel);
  });

  it('should set finished state to true', () => {
    // Arrange: initial state is false
    expect(store.getState().finished).toBe(false);

    // Act
    store.getActions().setFinished(true);

    // Assert
    expect(store.getState().finished).toBe(true);
  });

  // ... other tests
});

[gemini-code-assist]

There is a large block of code for handling simulation completion, cancellation, and failure (lines 480-532) that is nearly identical to the logic in the run thunk (lines 381-433). This code duplication makes the code harder to maintain, as any changes would need to be applied in two places.

I recommend refactoring this common logic into a separate helper function. This function could take parameters like the error message and metrics data, and then handle setting the state and tracking events accordingly. This would significantly improve code clarity and maintainability.

[gemini-code-assist]

This test case only verifies that the type signature of a mock action allows undefined, it doesn't test any runtime behavior of the actual continueSimulation thunk. The comment inside even states that the implementation is tested via integration tests.

While type checking is useful, a unit test should ideally verify some logic. This test as it stands provides little value and could be misleading. I'd recommend either:

1. Removing this test if it's adequately covered by integration tests.
2. Turning it into a proper unit test for the thunk. This would involve mocking dependencies (like lammps) and asserting that the correct methods are called with the expected arguments (e.g., lammps.runCommand is called with 'run 1000 pre no post no' when no timesteps are provided).

[gemini-code-assist]

The use of // @ts-ignore and type casting with as here (and on line 481) should be avoided. It suppresses potential type errors and indicates that the store's types might not be correctly inferred. easy-peasy provides utilities to create a fully typed store. Please ensure the store is set up so that getStoreState() and getStoreActions() return correctly typed objects, which will eliminate the need for these comments and casts.

[gemini-code-assist]

The number 1000 is used as a default value for timesteps. This is a 'magic number' and should be extracted into a named constant at the top of the file (e.g., const DEFAULT_CONTINUE_TIMESTEPS = 1000;) to improve readability and maintainability.

Suggested change

	const runCommand = timesteps ? `run ${timesteps} pre no post no` : `run 1000 pre no post no`;
	const runCommand = `run ${timesteps ?? 1000} pre no post no`;