Merge pull request #2983 from github/robertbrignull/remove_selected_model

Handle when the number of modeled methods decreases

extensions/ql-vscode/src/view/method-modeling/MultipleModeledMethodsPanel.tsx

@@ -1,5 +1,5 @@
 import * as React from "react";
-import { useCallback, useMemo, useState } from "react";
+import { useCallback, useEffect, useMemo, useRef, useState } from "react";
 import { Method } from "../../model-editor/method";
 import { ModeledMethod } from "../../model-editor/modeled-method";
 import {
@@ -60,6 +60,22 @@ export const MultipleModeledMethodsPanel = ({
 }: MultipleModeledMethodsPanelProps) => {
   const [selectedIndex, setSelectedIndex] = useState<number>(0);
 
+  const selectNewMethod = useRef<number | null>(null);
+
+  useEffect(() => {
+    if (selectNewMethod.current === modeledMethods.length - 1) {
+      setSelectedIndex(selectNewMethod.current);
+      selectNewMethod.current = null;
+      return;
+    }
+
+    if (selectedIndex >= modeledMethods.length) {
+      setSelectedIndex(
+        modeledMethods.length > 0 ? modeledMethods.length - 1 : 0,
+      );
+    }
+  }, [modeledMethods.length, selectedIndex]);
+
   const handlePreviousClick = useCallback(() => {
     setSelectedIndex((previousIndex) => previousIndex - 1);
   }, []);
@@ -89,7 +105,7 @@ export const MultipleModeledMethodsPanel = ({
     const newModeledMethods = [...modeledMethods, newModeledMethod];
 
     onChange(method.signature, newModeledMethods);
-    setSelectedIndex(newModeledMethods.length - 1);
+    selectNewMethod.current = newModeledMethods.length - 1;
   }, [onChange, modeledMethods, method]);
 
   const handleRemoveClick = useCallback(() => {

extensions/ql-vscode/src/view/method-modeling/__tests__/MultipleModeledMethodsPanel.spec.tsx

@@ -162,6 +162,30 @@ describe(MultipleModeledMethodsPanel.name, () => {
         },
       ]);
     });
+
+    it("changes selection to the newly added modeling", async () => {
+      const { rerender } = render({
+        method,
+        modeledMethods,
+        isModelingInProgress,
+        onChange,
+      });
+
+      await userEvent.click(screen.getByLabelText("Add modeling"));
+
+      rerender(
+        <MultipleModeledMethodsPanel
+          method={method}
+          modeledMethods={
+            onChange.mock.calls[onChange.mock.calls.length - 1][1]
+          }
+          isModelingInProgress={isModelingInProgress}
+          onChange={onChange}
+        />,
+      );
+
+      expect(screen.getByText("2/2")).toBeInTheDocument();
+    });
   });
 
   describe("with two modeled methods", () => {
@@ -264,6 +288,33 @@ describe(MultipleModeledMethodsPanel.name, () => {
       ).toHaveValue("source");
     });
 
+    it("correctly updates selected pagination index when the number of models decreases", async () => {
+      const { rerender } = render({
+        method,
+        modeledMethods,
+        isModelingInProgress,
+        onChange,
+      });
+
+      await userEvent.click(screen.getByLabelText("Next modeling"));
+
+      rerender(
+        <MultipleModeledMethodsPanel
+          method={method}
+          modeledMethods={[modeledMethods[1]]}
+          isModelingInProgress={isModelingInProgress}
+          onChange={onChange}
+        />,
+      );
+
+      expect(screen.getAllByRole("combobox")).toHaveLength(4);
+      expect(
+        screen.getByRole("combobox", {
+          name: "Model type",
+        }),
+      ).toHaveValue("source");
+    });
+
     it("does not show errors", () => {
       render({
         method,
@@ -444,6 +495,32 @@ describe(MultipleModeledMethodsPanel.name, () => {
         screen.getByText("Error: Conflicting classification"),
       ).toBeInTheDocument();
     });
+
+    it("changes selection to the newly added modeling", async () => {
+      const { rerender } = render({
+        method,
+        modeledMethods,
+        isModelingInProgress,
+        onChange,
+      });
+
+      expect(screen.getByText("1/2")).toBeInTheDocument();
+
+      await userEvent.click(screen.getByLabelText("Add modeling"));
+
+      rerender(
+        <MultipleModeledMethodsPanel
+          method={method}
+          modeledMethods={
+            onChange.mock.calls[onChange.mock.calls.length - 1][1]
+          }
+          isModelingInProgress={isModelingInProgress}
+          onChange={onChange}
+        />,
+      );
+
+      expect(screen.getByText("3/3")).toBeInTheDocument();
+    });
   });
 
   describe("with three modeled methods", () => {
@@ -559,6 +636,52 @@ describe(MultipleModeledMethodsPanel.name, () => {
         }),
       ).toHaveValue("remote");
     });
+
+    it("preserves selection when a modeling other than the selected modeling is removed", async () => {
+      const { rerender } = render({
+        method,
+        modeledMethods,
+        isModelingInProgress,
+        onChange,
+      });
+
+      expect(screen.getByText("1/3")).toBeInTheDocument();
+
+      rerender(
+        <MultipleModeledMethodsPanel
+          method={method}
+          modeledMethods={modeledMethods.slice(0, 2)}
+          isModelingInProgress={isModelingInProgress}
+          onChange={onChange}
+        />,
+      );
+
+      expect(screen.getByText("1/2")).toBeInTheDocument();
+    });
+
+    it("reduces selection when the selected modeling is removed", async () => {
+      const { rerender } = render({
+        method,
+        modeledMethods,
+        isModelingInProgress,
+        onChange,
+      });
+
+      await userEvent.click(screen.getByLabelText("Next modeling"));
+      await userEvent.click(screen.getByLabelText("Next modeling"));
+      expect(screen.getByText("3/3")).toBeInTheDocument();
+
+      rerender(
+        <MultipleModeledMethodsPanel
+          method={method}
+          modeledMethods={modeledMethods.slice(0, 2)}
+          isModelingInProgress={isModelingInProgress}
+          onChange={onChange}
+        />,
+      );
+
+      expect(screen.getByText("2/2")).toBeInTheDocument();
+    });
   });
 
   describe("with 1 modeled and 1 unmodeled method", () => {