Lightning bug fixes (#3941)

* test setup, fix dbPath issue and id search * minimal tests * enable lightning mode in before all * fix import * id list view bar fix
voxel51 · Jan 16, 2024 · a19116b · a19116b
1 parent e756179
commit a19116b
Show file tree

Hide file tree

Showing 14 changed files with 195 additions and 62 deletions.
diff --git a/app/packages/components/src/components/Selector/Selector.tsx b/app/packages/components/src/components/Selector/Selector.tsx
@@ -13,6 +13,8 @@ import LoadingDots from "../Loading/LoadingDots";
 import SearchResults, { UseSearch } from "./SearchResults";
 import style from "./Selector.module.css";
 
+export class SelectorValidationError extends Error {}
+
 export interface SelectorProps<T> {
   id?: string;
   value?: string;
@@ -69,11 +71,18 @@ function Selector<T>(props: SelectorProps<T>) {
           ? valuesRef.current[active]
           : valuesRef.current.find((v) => toKey(v) === search);
 
-      const result = await onSelect(value ? toKey(value) : search, value);
-      if (result !== undefined) {
-        local.current = result;
+      try {
+        const result = await onSelect(value ? toKey(value) : search, value);
+        if (result !== undefined) {
+          local.current = result;
+        }
+        setEditing(false);
+      } catch (error) {
+        if (error instanceof SelectorValidationError) {
+          return;
+        }
+        throw error;
       }
-      setEditing(false);
     };
   }, [active, onSelect, toKey, valuesRef]);
 

diff --git a/app/packages/components/src/components/Selector/index.ts b/app/packages/components/src/components/Selector/index.ts
@@ -1,2 +1,2 @@
-export { default } from "./Selector";
-export type { UseSearch } from "./Selector";
+export type { UseSearch } from "./SearchResults";
+export { SelectorValidationError, default } from "./Selector";
diff --git a/app/packages/components/src/components/index.ts b/app/packages/components/src/components/index.ts
@@ -19,7 +19,7 @@ export { default as Popout, PopoutDiv } from "./Popout";
 export { default as PopoutSectionTitle } from "./PopoutSectionTitle";
 export * from "./Selection";
 export { default as Selection } from "./Selection";
-export { default as Selector } from "./Selector";
+export { default as Selector, SelectorValidationError } from "./Selector";
 export type { UseSearch } from "./Selector";
 export { default as TabOption } from "./TabOption";
 export { default as ThemeProvider, useTheme } from "./ThemeProvider";

diff --git a/app/packages/core/src/components/Actions/Options.tsx b/app/packages/core/src/components/Actions/Options.tsx
@@ -235,6 +235,7 @@ const Lightning = () => {
         options={["disable", "enable"].map((value) => ({
           text: value,
           title: value,
+          dataCy: `lightning-mode-${value}`,
           onClick: () =>
             setThreshold(value === "disable" ? null : config ?? count),
         }))}

diff --git a/app/packages/core/src/components/Filters/StringFilter/StringFilter.tsx b/app/packages/core/src/components/Filters/StringFilter/StringFilter.tsx
@@ -106,6 +106,7 @@ const StringFilter = ({
             placeholder={`+ ${
               isFilterMode ? "filter" : "set visibility"
             } by ${name}`}
+            cy={`sidebar-search-${path}`}
             component={ResultComponent}
             onSelect={onSelect}
             inputStyle={{

diff --git a/app/packages/core/src/components/Filters/StringFilter/useOnSelect.ts b/app/packages/core/src/components/Filters/StringFilter/useOnSelect.ts
@@ -1,3 +1,6 @@
+import { SelectorValidationError } from "@fiftyone/components";
+import { isObjectIdField, snackbarErrors } from "@fiftyone/state";
+import { isObjectIdString } from "@fiftyone/utilities";
 import { useRef } from "react";
 import { RecoilState, useRecoilCallback } from "recoil";
 import { Result } from "./Result";
@@ -12,13 +15,22 @@ export default function (
     onSelect: useRecoilCallback(
       ({ snapshot, set }) =>
         async (value: string | null, d?: Result) => {
+          const isObjectId = await snapshot.getPromise(isObjectIdField(path));
+          if (isObjectId && (value === null || !isObjectIdString(value))) {
+            set(snackbarErrors, [
+              `${value} is not a 24 character hexadecimal string`,
+            ]);
+            throw new SelectorValidationError();
+          }
+
           const selected = new Set(await snapshot.getPromise(selectedAtom));
           if (d?.value === null) {
             value = null;
           }
           selectedMap.current.set(value, d?.count || null);
           selected.add(value);
           set(selectedAtom, [...selected].sort());
+
           return "";
         },
       [modal, path, selectedAtom, selectedMap]

diff --git a/app/packages/core/src/components/Sidebar/Entries/FilterablePathEntry/Arrow.tsx b/app/packages/core/src/components/Sidebar/Entries/FilterablePathEntry/Arrow.tsx
@@ -27,7 +27,7 @@ export default ({
     return (
       <Tooltip text={<AddIndex />} placement="top-center">
         <Arrow
-          data-cy={`sidebar-field-arrow-${id}`}
+          data-cy={`sidebar-field-arrow-disabled-${id}`}
           style={{ margin: 0, color: theme.text.secondary }}
         />
       </Tooltip>
@@ -36,7 +36,7 @@ export default ({
 
   return (
     <Arrow
-      data-cy={`sidebar-field-arrow-${id}`}
+      data-cy={`sidebar-field-arrow-enabled-${id}`}
       style={{
         cursor: "pointer",
         margin: 0,

diff --git a/app/packages/core/src/components/ViewBar/ViewStage/utils.ts b/app/packages/core/src/components/ViewBar/ViewStage/utils.ts
@@ -21,3 +21,27 @@ export const getMatch = (options, value) => {
   }
   return null;
 };
+
+export const cleanCSV = (values: string) => {
+  return (
+    values
+      // replace spaces with a single space (to allow search by words with spaces)
+      .replace(/[\s\'\"\[\]]+/g, " ")
+      // replace comma followed by trailing spaces with a single comma
+      .replace(/,\s*/g, ",")
+      // remove trailing spaces
+      .replace(/[ \t]+$/, "")
+  );
+};
+
+export const getArray = (value: string | unknown[]): unknown[] => {
+  if (typeof value === "string") {
+    try {
+      return JSON.parse(value);
+    } catch {
+      return value.replace(/[\s]/g, "").split(",");
+    }
+  }
+
+  return value;
+};
diff --git a/app/packages/core/src/components/ViewBar/ViewStage/viewStageParameterMachine.ts b/app/packages/core/src/components/ViewBar/ViewStage/viewStageParameterMachine.ts
@@ -1,7 +1,8 @@
 import { v4 as uuid } from "uuid";
 import { actions, Machine, sendParent } from "xstate";
 
-import { computeBestMatchString, getMatch } from "./utils";
+import { isObjectIdString } from "@fiftyone/utilities";
+import { cleanCSV, computeBestMatchString, getArray, getMatch } from "./utils";
 
 const { assign } = actions;
 
@@ -85,7 +86,7 @@ export const PARSER = {
     castFrom: (value) => value,
     castTo: (value) => value,
     parse: (value) => value,
-    validate: (value) => /[0-9A-Fa-f]{24}/g.test(value),
+    validate: (value) => isObjectIdString(value),
   },
   int: {
     castFrom: (value) => String(value).replace(/\B(?=(\d{3})+(?!\d))/g, ","),
@@ -113,66 +114,25 @@ export const PARSER = {
   "list<field>": {
     castFrom: (value) => value.join(","),
     castTo: (value) => value.split(","),
-    parse: (value) => value.replace(/[\s\'\"\[\]]/g, ""),
+    parse: (value) => cleanCSV(value),
     validate: (value, fields) => {
-      const stripped = value.replace(/[\s]/g, "");
-      let array = null;
-      try {
-        array = JSON.parse(stripped);
-      } catch {
-        array = stripped.split(",");
-      }
-      return (
-        typeof value !== "string" &&
-        Array.isArray(array) &&
-        array.every((e) => PARSER.field.validate(e, fields))
-      );
+      return getArray(value).every((e) => PARSER.field.validate(e, fields));
     },
   },
   "list<id>": {
     castFrom: (value) => value.join(","),
     castTo: (value) => value.split(","),
-    parse: (value) => value.replace(/[\s\'\"\[\]]/g, ""),
+    parse: (value) => cleanCSV(value),
     validate: (value) => {
-      const stripped = value.replace(/[\s]/g, "");
-      let array = null;
-      try {
-        array = JSON.parse(stripped);
-      } catch {
-        array = stripped.split(",");
-      }
-      return (
-        typeof value !== "string" &&
-        Array.isArray(array) &&
-        array.every((e) => PARSER.id.validate(e))
-      );
+      return getArray(value).every((e) => PARSER.id.validate(e));
     },
   },
   "list<str>": {
     castFrom: (value) => value.join(","),
     castTo: (value) => value.split(","),
-    parse: (value) =>
-      value
-        // replace spaces with a single space (to allow search by words with spaces)
-        .replace(/[\s\'\"\[\]]+/g, " ")
-        // replace comma followed by trailing spaces with a single comma
-        .replace(/,\s*/g, ",")
-        // remove trailing spaces
-        .replace(/[ \t]+$/, ""),
+    parse: (value) => cleanCSV(value),
     validate: (value) => {
-      const stripped = value.replace(/[\s]/g, "");
-      let array = null;
-      try {
-        array = JSON.parse(stripped);
-      } catch {
-        // ex: exclude_fields('a, b'). 'a,b' => ['a', 'b']
-        array = stripped.split(",");
-      }
-      return (
-        typeof array !== "string" &&
-        Array.isArray(array) &&
-        array.every((e) => PARSER.str.validate(e))
-      );
+      return getArray(value).every((e) => PARSER.str.validate(e));
     },
   },
   NoneType: {

diff --git a/app/packages/state/src/recoil/lightning.ts b/app/packages/state/src/recoil/lightning.ts
@@ -103,6 +103,8 @@ const indexesByPath = selector({
     ) => {
       const projection = frames ? framesProjection : samplesProjection;
 
+      fields = fields.map((field) => get(schemaAtoms.dbPath(field)));
+
       if (field === "$**") {
         if (!projection) {
           return fields;
@@ -143,8 +145,7 @@ export const pathIndex = selectorFamily({
     (path: string) =>
     ({ get }) => {
       const indexes = get(indexesByPath);
-
-      return indexes.has(path);
+      return indexes.has(get(schemaAtoms.dbPath(path)));
     },
 });
 

diff --git a/app/packages/utilities/src/type-check.ts b/app/packages/utilities/src/type-check.ts
@@ -10,6 +10,14 @@ export function isPrimitiveType(type: string) {
   return PRIMITIVE_TYPES.includes(type);
 }
 
+export function isHex(value: string) {
+  return /[0-9a-f]{24}/g.test(value);
+}
+
+export function isObjectIdString(value: string) {
+  return isHex(value) && value.length === 24;
+}
+
 export type NumberKeyObjectType<V = unknown> = {
   [key: string]: V;
 };

diff --git a/e2e-pw/src/oss/poms/action-row/display-options.ts b/e2e-pw/src/oss/poms/action-row/display-options.ts
@@ -3,6 +3,7 @@ import { Page } from "src/oss/fixtures";
 type SidebarStatisticsMode = "slice" | "group";
 type SidebarMode = "fast" | "best" | "all";
 type SidebarSortMode = "count" | "value";
+type LightningMode = "enable" | "disable";
 
 export class DisplayOptionsPom {
   readonly page: Page;
@@ -11,6 +12,11 @@ export class DisplayOptionsPom {
     this.page = page;
   }
 
+  async setLightningMode(mode: LightningMode) {
+    const selector = this.page.getByTestId(`lightning-mode-${mode}`);
+    return selector.click();
+  }
+
   async setSidebarStatisticsMode(mode: SidebarStatisticsMode) {
     const selector = this.page.getByTestId(
       `tab-option-View ${mode} sidebar statistics`

diff --git a/e2e-pw/src/oss/poms/sidebar.ts b/e2e-pw/src/oss/poms/sidebar.ts
@@ -28,6 +28,16 @@ export class SidebarPom {
       .nth(1);
   }
 
+  fieldContainer(fieldName: string) {
+    return this.sidebar.getByTestId(`sidebar-field-container-${fieldName}`);
+  }
+
+  fieldArrow(fieldName: string, enabled: boolean) {
+    return this.fieldContainer(fieldName).getByTestId(
+      `sidebar-field-arrow-${enabled ? "enabled" : "disabled"}-${fieldName}`
+    );
+  }
+
   sidebarEntryDraggableArea(fieldName: string) {
     return this.sidebar
       .getByTestId(`sidebar-entry-draggable-${fieldName}`)
@@ -58,7 +68,9 @@ export class SidebarPom {
   }
 
   async clickFieldDropdown(field: string) {
-    const selector = this.sidebar.getByTestId(`sidebar-field-arrow-${field}`);
+    const selector = this.sidebar.getByTestId(
+      `sidebar-field-arrow-enabled-${field}`
+    );
     return selector.click();
   }
 
@@ -92,6 +104,12 @@ export class SidebarPom {
     await selectionDiv.click({ force: true });
   }
 
+  async applySearch(field: string, search: string) {
+    const input = this.sidebar.getByTestId(`selector-sidebar-search-${field}`);
+    await input.fill(search);
+    await input.press("Enter");
+  }
+
   // apply a filter to a field
   async applyLabelFromList(
     field: string,
@@ -137,9 +155,29 @@ class SidebarAsserter {
     await expect(this.sb.field(fieldName)).toBeVisible();
   }
 
+  async assertFieldDisabled(fieldName: string) {
+    await expect(this.sb.fieldArrow(fieldName, false)).toBeVisible();
+  }
+
+  async assertFieldsDisabled(fieldNames: string[]) {
+    for (let i = 0; i < fieldNames.length; i++) {
+      await this.assertFieldDisabled(fieldNames[i]);
+    }
+  }
+
+  async assertFieldEnabled(fieldName: string) {
+    await expect(this.sb.fieldArrow(fieldName, true)).toBeVisible();
+  }
+
+  async assertFieldsEnabled(fieldNames: string[]) {
+    for (let i = 0; i < fieldNames.length; i++) {
+      await this.assertFieldEnabled(fieldNames[i]);
+    }
+  }
+
   async assertFieldsInSidebar(fieldNames: string[]) {
     for (let i = 0; i < fieldNames.length; i++) {
-      await this.sb.asserter.assertFieldInSidebar(fieldNames[i]);
+      await this.assertFieldInSidebar(fieldNames[i]);
     }
   }