#8636: fix search/insert for variable popover using optional chaining (…

…#8637)

src/components/fields/schemaFields/widgets/varPopup/VarPopup.tsx

@@ -17,10 +17,12 @@
 
 import React, { useCallback, useEffect } from "react";
 import { type FieldInputMode } from "@/components/fields/schemaFields/fieldInputMode";
-import { replaceLikelyVariable } from "./likelyVariableUtils";
+import {
+  getFullVariableName,
+  replaceLikelyVariable,
+} from "./likelyVariableUtils";
 import VarMenu from "./VarMenu";
 import fitTextarea from "fit-textarea";
-import { getPathFromArray } from "@/runtime/pathHelpers";
 import useAttachPopup from "@/components/fields/schemaFields/widgets/varPopup/useAttachPopup";
 import reportEvent from "@/telemetry/reportEvent";
 import { Events } from "@/telemetry/events";
@@ -71,7 +73,10 @@ const VarPopup: React.FunctionComponent<VarPopupProps> = ({
     (selectedPath: string[]) => {
       reportEvent(Events.VAR_POPOVER_SELECT);
 
-      const fullVariableName = getPathFromArray(selectedPath);
+      const fullVariableName = getFullVariableName(
+        likelyVariable,
+        selectedPath,
+      );
 
       switch (inputMode) {
         case "var": {
@@ -117,7 +122,7 @@ const VarPopup: React.FunctionComponent<VarPopupProps> = ({
 
       hideMenu();
     },
-    [hideMenu, inputElementRef, inputMode, setValue, value],
+    [hideMenu, inputElementRef, inputMode, setValue, value, likelyVariable],
   );
 
   if (!isMenuShowing) {

src/components/fields/schemaFields/widgets/varPopup/likelyVariableUtils.test.ts

@@ -16,6 +16,7 @@
  */
 
 import {
+  getFullVariableName,
   getLikelyVariableAtPosition,
   replaceLikelyVariable,
 } from "./likelyVariableUtils";
@@ -314,3 +315,44 @@ describe("replaceLikelyVariable", () => {
     expect(newCursorPosition).toEqual(endOfVariableIndex);
   });
 });
+
+describe("getFullVariableName", () => {
+  it("preserves optional chaining", () => {
+    expect(getFullVariableName("@foo", ["@foo", "bar"])).toBe("@foo.bar");
+    expect(getFullVariableName("@foo?", ["@foo", "bar"])).toBe("@foo?.bar");
+    expect(getFullVariableName("@foo?.bar?", ["@foo", "bar", "baz"])).toBe(
+      "@foo?.bar?.baz",
+    );
+  });
+
+  // TODO: #8638: https://github.com/pixiebrix/pixiebrix-extension/issues/8638
+  it.skip("#8638: handle ? in property accessor", () => {
+    expect(
+      getFullVariableName('@foo.bar["hello world?"]?', [
+        "@foo",
+        "bar",
+        "hello world?",
+        "qux",
+      ]),
+    ).toBe('@foo.bar["hello world?"]?.qux');
+  });
+
+  it("handles optional chaining with bracket notation", () => {
+    expect(
+      getFullVariableName("@foo.bar?.[42]", ["@foo", "bar", "42", "qux"]),
+    ).toBe("@foo.bar?.[42].qux");
+    expect(
+      getFullVariableName("@foo.bar[42]?", ["@foo", "bar", "42", "qux"]),
+    ).toBe("@foo.bar[42]?.qux");
+    expect(
+      getFullVariableName('@foo.bar[""]?', ["@foo", "bar", "", "qux"]),
+    ).toBe('@foo.bar[""]?.qux');
+    expect(
+      getFullVariableName('@foo?.["hello world?"]', [
+        "@foo",
+        "hello world?",
+        "bar",
+      ]),
+    ).toBe('@foo?.["hello world?"].bar');
+  });
+});

src/components/fields/schemaFields/widgets/varPopup/likelyVariableUtils.ts

@@ -15,6 +15,9 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+import { toPath } from "lodash";
+import { getPathFromArray } from "@/runtime/pathHelpers";
+
 const varRegex = /(?<varName>@(\.|\w|(\[\d+])|(\[("|')[\s\w]+("|')]))*)/g;
 
 type LikelyVariable = {
@@ -189,3 +192,49 @@ export function replaceLikelyVariable(
     newCursorPosition: endOfVariableIndex,
   };
 }
+
+/**
+ * Select the full variable name based on the selected path and user's expression so far.
+ */
+export function getFullVariableName(
+  likelyVariable: string,
+  selectedPath: string[],
+): string {
+  // `toPath` will create a separate element for the ? symbol. So we need to merge them back. Eventually we need to
+  // switch from `toPath` to our own implementation/parser.
+  // @foo.bar[42]? -> [@foo, bar, 42, ?]
+  const pathWithChainElements = toPath(likelyVariable);
+
+  const likelyPath: string[] = [];
+  for (let i = 0; i < pathWithChainElements.length; i++) {
+    // eslint-disable-next-line security/detect-object-injection,@typescript-eslint/no-unnecessary-type-assertion,@typescript-eslint/no-non-null-assertion -- numeric index
+    const base: string = pathWithChainElements[i]!;
+
+    if (pathWithChainElements[i + 1] === "?") {
+      // TODO: #8637: https://github.com/pixiebrix/pixiebrix-extension/issues/8638
+      //  if the path is `["hello world?"]` this results in ??, which is incorrect. See test case.
+      likelyPath.push(base + "?");
+      i++;
+    } else {
+      likelyPath.push(base);
+    }
+  }
+
+  return getPathFromArray(
+    selectedPath.map((part, index) => {
+      // eslint-disable-next-line security/detect-object-injection -- numeric index
+      const current = likelyPath[index] ?? "";
+
+      // Preserve optional chaining from what the user has typed so far
+      if (
+        current.endsWith("?") &&
+        !/[ .]/.test(current) &&
+        index !== selectedPath.length - 1
+      ) {
+        return { part, isOptional: true };
+      }
+
+      return part;
+    }),
+  );
+}

src/components/fields/schemaFields/widgets/varPopup/menuFilters.test.ts

@@ -91,6 +91,15 @@ describe("filterVarMapByVariable", () => {
       }),
     );
 
+    // Exact match with chaining
+    expect(filterVarMapByVariable(inputMap, "@input?.foo")).toEqual(
+      expect.objectContaining({
+        "@input": expect.objectContaining({
+          foo: expect.toBeObject(),
+        }),
+      }),
+    );
+
     // Empty because trailing period indicates final variable name
     expect(filterVarMapByVariable(inputMap, "@input.fo.")).toEqual(
       expect.objectContaining({

src/components/fields/schemaFields/widgets/varPopup/menuFilters.ts

@@ -31,6 +31,25 @@ import { getIn } from "formik";
  */
 export type MenuOptions = Array<[string, UnknownRecord]>;
 
+/**
+ * Returns true if the value is null or is likely plain text (i.e., not a variable).
+ */
+function isTextOrNullVar(value: string | null): boolean {
+  return value == null || value === "@" || !value.startsWith("@");
+}
+
+/**
+ * Convert a variable to a normalized variable path, removing optional chaining. Result is suitable for filtering
+ * by path prefix.
+ */
+function toVarPath(value: string | null): string[] {
+  if (value == null) {
+    return [];
+  }
+
+  return toPath(value.replaceAll("?.", "."));
+}
+
 /**
  * Filter top-level variables by source type. Currently, excludes integration variables because they're managed
  * automatically by the Page Editor.
@@ -54,15 +73,11 @@ export function filterOptionsByVariable(
   options: MenuOptions,
   likelyVariable: string,
 ): MenuOptions {
-  if (
-    likelyVariable == null ||
-    likelyVariable === "@" ||
-    !likelyVariable.startsWith("@")
-  ) {
+  if (isTextOrNullVar(likelyVariable)) {
     return options;
   }
 
-  const [base, ...rest] = toPath(likelyVariable);
+  const [base, ...rest] = toVarPath(likelyVariable);
   return options.filter(([source, vars]) => {
     if (rest.length === 0) {
       return Object.keys(vars).some((x) => x.startsWith(base));
@@ -114,15 +129,11 @@ export function filterVarMapByVariable(
   varMap: UnknownRecord,
   likelyVariable: string,
 ): UnknownRecord {
-  if (
-    likelyVariable == null ||
-    likelyVariable === "@" ||
-    !likelyVariable.startsWith("@")
-  ) {
+  if (isTextOrNullVar(likelyVariable)) {
     return varMap;
   }
 
-  return filterVarMapByPath(varMap, toPath(likelyVariable));
+  return filterVarMapByPath(varMap, toVarPath(likelyVariable));
 }
 
 /**
@@ -134,17 +145,13 @@ export function expandCurrentVariableLevel(
   varMap: UnknownRecord,
   likelyVariable: string,
 ): ShouldExpandNodeInitially {
-  if (
-    likelyVariable == null ||
-    likelyVariable === "@" ||
-    !likelyVariable.startsWith("@")
-  ) {
+  if (isTextOrNullVar(likelyVariable)) {
     return () => false;
   }
 
   // If likelyVariable ends with ".", there's a part for the empty string at the end of the path. So can just use
   // as normal without logic for trailing "."
-  const parts = toPath(likelyVariable);
+  const parts = toVarPath(likelyVariable);
   return (keyPath, data, level) => {
     // Key path from JSONTree is in reverse order
     const reverseKeyPath = [...keyPath].reverse();
@@ -241,12 +248,9 @@ export function defaultMenuOption(
     return null;
   }
 
-  if (
-    likelyVariable == null ||
-    likelyVariable === "@" ||
-    !likelyVariable.startsWith("@") ||
-    toPath(likelyVariable).length === 0
-  ) {
+  const parts = toVarPath(likelyVariable);
+
+  if (isTextOrNullVar(likelyVariable) || parts.length === 0) {
     // Must always have at least one option (e.g., the `@input`)
     // Prefer the last option, because that's the latest output
 
@@ -255,8 +259,6 @@ export function defaultMenuOption(
     return [first];
   }
 
-  const parts = toPath(likelyVariable);
-
   const [head, ...rest] = parts;
 
   // Reverse options to find the last source that matches. (To account for shadowing)

src/runtime/pathHelpers.test.ts

@@ -145,35 +145,54 @@ describe("getFieldNamesFromPathString", () => {
   });
 });
 
-test("getPathFromArray", () => {
-  const expectMatch = (
-    pathArray: Array<number | string>,
-    expectedPathString: string,
-  ) => {
-    const pathString = getPathFromArray(pathArray);
-    const lodashArray = toPath(pathString);
-
-    // Compare the array to the expected string
-    expect(pathString).toBe(expectedPathString);
-
-    // Expect the same input, except that lodash only returns strings even for numbers
-    expect(lodashArray).toEqual(pathArray.map(String));
-  };
-
-  expectMatch(["user"], "user");
-  expectMatch(["users", 0], "users[0]");
-  expectMatch(["users", 0, "name"], "users[0].name");
-  expectMatch(["users", ""], 'users[""]');
-  expectMatch(["names", "Dante Alighieri"], 'names["Dante Alighieri"]');
-  expectMatch(
-    ["Ugo Foscolo", "User Location"],
-    '["Ugo Foscolo"]["User Location"]',
-  );
-  expectMatch(["User List", 100, "id"], '["User List"][100].id');
-  expectMatch(
-    ["User List", 100_000_000, "The name"],
-    '["User List"][100000000]["The name"]',
-  );
+describe("getPathFromArray", () => {
+  test("required path parts", () => {
+    const expectMatch = (
+      pathArray: Array<number | string>,
+      expectedPathString: string,
+    ) => {
+      const pathString = getPathFromArray(pathArray);
+      const lodashArray = toPath(pathString);
+
+      // Compare the array to the expected string
+      expect(pathString).toBe(expectedPathString);
+
+      // Expect the same input, except that lodash only returns strings even for numbers
+      expect(lodashArray).toEqual(pathArray.map(String));
+    };
+
+    expectMatch(["user"], "user");
+    expectMatch(["users", 0], "users[0]");
+    expectMatch(["users", 0, "name"], "users[0].name");
+    expectMatch(["users", ""], 'users[""]');
+    expectMatch(["names", "Dante Alighieri"], 'names["Dante Alighieri"]');
+    expectMatch(
+      ["Ugo Foscolo", "User Location"],
+      '["Ugo Foscolo"]["User Location"]',
+    );
+    expectMatch(["User List", 100, "id"], '["User List"][100].id');
+    expectMatch(
+      ["User List", 100_000_000, "The name"],
+      '["User List"][100000000]["The name"]',
+    );
+  });
+
+  test("optional chaining path parts", () => {
+    expect(
+      getPathFromArray(["users", { part: 0, isOptional: true }, "name"]),
+    ).toBe("users[0]?.name");
+    expect(
+      getPathFromArray(["users?", { part: 0, isOptional: true }, "name"]),
+    ).toBe("users?.[0]?.name");
+    expect(
+      getPathFromArray([
+        "users",
+        "foo bar?",
+        { part: 0, isOptional: true },
+        "name",
+      ]),
+    ).toBe('users["foo bar?"][0]?.name');
+  });
 });
 
 test.each([