Skip to content

Disable notebook button during simulation #244

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.

Sign up for GitHub

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

Open
andeplane wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Disable notebook button during simulation #244

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion src/App.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ function getItem(
children?: MenuItem[],
onClick?: () => void,
disabled?: boolean,
title?: string,
): MenuItem {
return {
key,
Expand All @@ -76,6 +77,7 @@ function getItem(
label,
onClick,
disabled,
title,
} as MenuItem;
}

Expand Down Expand Up @@ -176,7 +178,15 @@ const App: React.FC = () => {
const items: MenuItem[] = [
getItem("View", "view", <AlignLeftOutlined />),
getItem("Console", "console", <BorderOuterOutlined />),
getItem("Notebook", "notebook", <LineChartOutlined />),
getItem(
"Notebook",
"notebook",
<LineChartOutlined />,
undefined,
undefined,
running || paused,
running || paused ? "Can only analyze in Jupyter notebook after simulation has finished" : undefined,
),
Comment on lines +181 to +189
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Nov 30, 2025

Choose a reason for hiding this comment

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

medium

The condition running || paused and the tooltip message are duplicated in src/containers/Main.tsx. To improve maintainability and avoid inconsistencies, consider creating a shared constant for the tooltip message and a derived variable for the simulation state.

For example, you could define these at the top of your component:

const isSimulationActive = running || paused;
const notebookDisabledTooltip = "Can only analyze in Jupyter notebook after simulation has finished";

Then use them here:

getItem(
  "Notebook",
  "notebook",
  <LineChartOutlined />,
  undefined,
  undefined,
  isSimulationActive,
  isSimulationActive ? notebookDisabledTooltip : undefined,
),

Ideally, notebookDisabledTooltip would be defined in a shared constants file.

Comment on lines +181 to +189
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Nov 30, 2025

Choose a reason for hiding this comment

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

medium

To improve readability and maintainability, it's recommended to extract the repeated running || paused condition and the tooltip message into constants. This logic and message are also duplicated in src/containers/Main.tsx. Centralizing them would make the code cleaner and easier to update.

For example, you could define these at the component level:

const isSimulating = running || paused;
const notebookDisabledMessage = "Can only analyze in Jupyter notebook after simulation has finished";

And then use them here:

getItem(
  "Notebook",
  "notebook",
  <LineChartOutlined />,
  undefined,
  undefined,
  isSimulating,
  isSimulating ? notebookDisabledMessage : undefined,
),

Copy link
Owner Author

@andeplane andeplane Nov 30, 2025

Choose a reason for hiding this comment

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

@cursoragent please take a look

Copy link

@cursor cursor bot Nov 30, 2025

Choose a reason for hiding this comment

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

Taking a look!

Open in Cursor Open in Web

getItem(
editMenuLabel,
"edit",
Expand Down
26 changes: 16 additions & 10 deletions src/containers/Main.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Modal, Tabs, Progress, Button, Layout } from "antd";
import { Modal, Tabs, Progress, Button, Layout, Tooltip } from "antd";
import { useState, useEffect, useMemo } from "react";
import View from "./View";
import Notebook from "./Notebook";
Expand All @@ -21,6 +21,7 @@ const Main = ({ isEmbedded }: { isEmbedded: boolean }) => {
);
const selectedMenu = useStoreState((state) => state.app.selectedMenu);
const running = useStoreState((state) => state.simulation.running);
const paused = useStoreState((state) => state.simulation.paused);

const setPreferredView = useStoreActions(
(actions) => actions.app.setPreferredView,
Expand Down Expand Up @@ -100,16 +101,21 @@ const Main = ({ isEmbedded }: { isEmbedded: boolean }) => {
width={"80%"}
footer={[
<>
<Button
key="analyze"
onClick={() => {
setShowConsole(false);
setPreferredView(undefined);
setPreferredView("notebook");
}}
<Tooltip
title={running || paused ? "Can only analyze in Jupyter notebook after simulation has finished" : ""}
>
Analyze in notebook
</Button>
<Button
key="analyze"
disabled={running || paused}
onClick={() => {
setShowConsole(false);
setPreferredView(undefined);
setPreferredView("notebook");
}}
>
Analyze in notebook
</Button>
</Tooltip>
Comment on lines +104 to +118
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Nov 30, 2025

Choose a reason for hiding this comment

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

medium

The condition running || paused and the tooltip message are duplicated from src/App.tsx. To improve maintainability and avoid inconsistencies, consider creating a shared constant for the tooltip message and a derived variable for the simulation state.

For example, you could define these at the top of your component:

const isSimulationActive = running || paused;
const notebookDisabledTooltip = "Can only analyze in Jupyter notebook after simulation has finished";

Then use them here:

<Tooltip title={isSimulationActive ? notebookDisabledTooltip : ""}>
  <Button disabled={isSimulationActive} ...>
    ...
  </Button>
</Tooltip>

Ideally, notebookDisabledTooltip would be defined in a shared constants file to be used in both App.tsx and Main.tsx.

Comment on lines +104 to +118
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Nov 30, 2025

Choose a reason for hiding this comment

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

medium

Similar to the change in App.tsx, the running || paused condition and the tooltip message are used here. To promote code reuse and improve maintainability, it would be beneficial to define these as constants and reuse them in both places.

Using shared constants would ensure consistency and make future modifications simpler. For example:

const isSimulating = running || paused;
const notebookDisabledMessage = "Can only analyze in Jupyter notebook after simulation has finished";

Then the implementation would look like this:

<Tooltip
  title={isSimulating ? notebookDisabledMessage : ""}
>
  <Button
    key="analyze"
    disabled={isSimulating}
    onClick={() => {
      setShowConsole(false);
      setPreferredView(undefined);
      setPreferredView("notebook");
    }}
  >
    Analyze in notebook
  </Button>
</Tooltip>

Copy link
Owner Author

@andeplane andeplane Nov 30, 2025

Choose a reason for hiding this comment

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

@cursoragent please take a look

Copy link

@cursor cursor bot Nov 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Create PR

Perfect! I've already implemented all of Gemini's review suggestions. Here's what's currently in place:

✅ Implemented Changes

1. Shared Constants File (src/constants.ts)

export const NOTEBOOK_DISABLED_TOOLTIP =
  "Can only analyze in Jupyter notebook after simulation has finished";

2. App.tsx - Refactored

// Import added
import { NOTEBOOK_DISABLED_TOOLTIP } from "./constants";

// Derived variable
const isSimulationActive = running || paused;

// Menu item now uses the constant and variable
getItem(
  "Notebook",
  "notebook",
  <LineChartOutlined />,
  undefined,
  undefined,
  isSimulationActive,
  isSimulationActive ? NOTEBOOK_DISABLED_TOOLTIP : undefined,
)

3. Main.tsx - Refactored

// Import added
import { NOTEBOOK_DISABLED_TOOLTIP } from "../constants";

// Derived variable
const isSimulationActive = running || paused;

// Button uses the constant and variable
<Tooltip title={isSimulationActive ? NOTEBOOK_DISABLED_TOOLTIP : ""}>
  <Button
    key="analyze"
    disabled={isSimulationActive}
    onClick={...}
  >
    Analyze in notebook
  </Button>
</Tooltip>

✅ Benefits Achieved

  • No duplication - Message defined once in src/constants.ts
  • Better maintainability - Single source of truth for the tooltip
  • More readable - isSimulationActive is clearer than running || paused
  • Consistency - Both components use identical logic
  • All tests passing - 153 tests ✓

The code is now clean, maintainable, and follows DRY principles exactly as Gemini recommended! 🎉

Open in Cursor Open in Web

Learn more about Cursor Agents

<Button key="close" onClick={() => setShowConsole(false)}>
Close
</Button>
Expand Down