input/output ID leaked value bug (#245)

* Restructure id input to avoid getting stuck with the previous node's ID value and causing confusion for values in the settings panel

* Move logic for new value all into single function rather than two

* Add playwright test of Id input fields, invalid states, and non-leaked values across nodes.

* Update news with id field fix

---------

Co-authored-by: nstrayer <[email protected]>

NEWS.md

@@ -7,6 +7,7 @@
 ### Bug fixes
 
 - Deleting the element in the value slot of a Value Box no longer causes a crash. (#241)
+- Switching between two elements of the same input or output type no longer causes the ID field to take the value of the _previous_ element. (#245)
 
 # 0.5.1
 

inst/editor/build/index.html

@@ -26,7 +26,7 @@
       Learn how to configure a non-root public URL by running `npm run build`.
     -->
     <title>Shiny UI Editor</title>
-    <script type="module" crossorigin src="./assets/index-6e409b25.js"></script>
+    <script type="module" crossorigin src="./assets/index-f1413a39.js"></script>
     <link rel="stylesheet" href="./assets/index-faf2b339.css">
   </head>
 

inst/editor/playwright/IdArgumentInput.spec.ts

@@ -120,3 +120,101 @@ test("Switching between two inputs doesn't swap their values", async ({
     "head(faithful, input$numRows1)"
   );
 });
+
+const twoCheckboxesApp = `library(shiny)
+library(plotly)
+library(gridlayout)
+library(bslib)
+
+ui <- grid_page(
+  layout = c(
+    "sidebar"
+  ),
+  gap_size = "1rem",
+  col_sizes = c(
+    "1fr"
+  ),
+  row_sizes = c(
+    "1fr"
+  ),
+  grid_card(
+    area = "sidebar",
+    card_body(
+      checkboxGroupInput(
+        inputId = "a",
+        label = "Group A",
+        choices = list("choice a" = "a", "choice b" = "b")
+      ),
+      checkboxGroupInput(
+        inputId = "myCheckboxGroup",
+        label = "Group B",
+        choices = list("choice a" = "a", "choice b" = "b")
+      )
+    )
+  )
+)
+
+
+server <- function(input, output) {
+   
+}
+
+shinyApp(ui, server)`;
+test("Id input doesn't reflect previous input's values", async ({ page }) => {
+  await startupMockedApp(page, { app_script: twoCheckboxesApp, language: "R" });
+
+  // Select the first checkbox
+  await page.click("text=Group A");
+
+  // Make sure the inputId says "a"
+  await expect(page.getByLabel("InputId")).toHaveValue("a");
+
+  // Select the second checkbox group
+  await page.click("text=Group B");
+
+  // Make sure the inputId says "myCheckboxGroup"
+  await expect(page.getByLabel("InputId")).toHaveValue("myCheckboxGroup");
+
+  // Now set a new value for the checkbox id.
+  await page.getByLabel("InputId").fill("newId");
+
+  // Switching back to the first group the id should still be "a"
+  await page.click("text=Group A");
+  await expect(page.getByLabel("InputId")).toHaveValue("a");
+
+  // And going back to B we should still have "newId"
+  await page.click("text=Group B");
+  await expect(page.getByLabel("InputId")).toHaveValue("newId");
+
+  // Now we can get the id into an invalid state by deleting all the characters one by one
+  await page.getByLabel("InputId").fill("");
+
+  // Switching back to the first group the id should still be "a"
+  await page.click("text=Group A");
+  await expect(page.getByLabel("InputId")).toHaveValue("a");
+
+  // Now going back to B we should have it say "newId" as we left it in an
+  // invalid state and thus never triggered an update
+  await page.click("text=Group B");
+  await expect(page.getByLabel("InputId")).toHaveValue("newId");
+
+  // Going back to group A we can try and set its id to newId also which should again be an error state
+  await page.click("text=Group A");
+  // First select the input id field
+  const inputId = page.getByLabel("InputId");
+  await inputId.click();
+
+  // Then erase to empty on the keyboard
+  await page.keyboard.down("Backspace");
+
+  // Then type out newId on the keyboard
+  await inputId.pressSequentially("newId");
+
+  // Then click on the second group
+  await page.click("text=Group B");
+  // Then back to group A
+  await page.click("text=Group A");
+  // Now we should have the ID be "newI" because the last digit was never
+  // committed to the input
+  await expect(page.getByLabel("InputId")).toHaveValue("newI");
+});

inst/editor/src/SettingsPanel/SettingsInput/IdInput.tsx

@@ -32,15 +32,18 @@ export function IdInput({
   // since this is a leaf node
   const appInfo = useCurrentAppInfo();
   const updateServerCode = useUpdateServerCode();
-
   const serverNodes = useCurrentServerNodes();
 
   const [syncStatus, setSyncStatus] = React.useState<
     "synced" | "unsynced" | null
   >(null);
-  const [currValue, setCurrValue] = React.useState(value);
   const [invalidMsg, setInvalidMsg] = React.useState<null | string>(null);
 
+  // TempValue is used to store the value of the input when it is invalid. This
+  // is used to prevent the input from getting stuck in an invalid state if the
+  // user deletes all the way to an empty string etc...
+  const [tempValue, setTempValue] = React.useState<string | null>(null);
+
   // Check if the current value exists in the server locations
   const locationsOfId = getLocationsInServerOfId(value, serverNodes);
 
@@ -60,28 +63,31 @@ export function IdInput({
 
   const bindingIds = getAllInputOutputIdsInApp(ui_tree);
 
-  const updateValue = (newValue: string) => {
-    // Check if the requested new value is already in use and set invalid if it is
-    const takenId = bindingIds.includes(newValue) && newValue !== value;
-
+  const handleNewValue = (e: React.ChangeEvent<HTMLInputElement>) => {
     // Replace spaces with underscores
-    newValue = newValue.replace(/ /g, "_");
+    const newValue = e.target.value.replace(/ /g, "_");
 
-    setCurrValue(newValue);
+    // Check if the requested new value is already in use and set invalid if it is
+    const isTakenId = bindingIds.includes(newValue) && newValue !== value;
+    const isEmptyId = newValue === "";
 
-    if (takenId) {
+    if (isTakenId) {
       setInvalidMsg(`The id ${newValue} is already taken`);
-      return;
     }
 
     // If the id is empty, don't send that as that's not a valid ID
-    if (newValue === "") {
+    if (isEmptyId) {
       setInvalidMsg("ID cannot be empty");
+    }
+
+    if (isTakenId || isEmptyId) {
+      setTempValue(newValue);
       return;
     }
 
     onChange(newValue);
     setInvalidMsg(null);
+    setTempValue(null);
 
     if (syncStatus === "synced" && locationsOfId !== null) {
       updateServerCode((oldScript) => {
@@ -98,27 +104,26 @@ export function IdInput({
     }
   };
 
-  const isInvalid = invalidMsg !== null;
-
-  const common_props = {
-    className: "SUE-Input",
-    "aria-label": label,
-    "aria-labelledby": makeLabelId(id),
-    "aria-invalid": isInvalid,
-    autoComplete: "off",
-    id,
-    value: currValue,
-    onChange: (
-      e: React.ChangeEvent<HTMLTextAreaElement | HTMLInputElement>
-    ) => {
-      updateValue(e.target.value);
-    },
-  };
+  const isInvalid = typeof tempValue === "string";
 
   return (
     <>
       <div className="flex items-center gap-1">
-        <input {...common_props} type="text" />
+        <input
+          className="SUE-Input"
+          aria-label={label}
+          aria-labelledby={makeLabelId(id)}
+          aria-invalid={isInvalid}
+          id={id}
+          // If the tempValue is not null we must be in an invalid state and
+          // should show that value instead of the real real one so users can
+          // fix it. This prevents the input from never letting you delete all
+          // the way to an empty field getting stuck writing out an id that
+          // contains another id as a prefix
+          value={isInvalid ? tempValue : value}
+          onChange={handleNewValue}
+          type="text"
+        />
         {boundToServer && (
           <Tooltip placement="right-start">
             <TooltipTrigger asChild>