#918: fix error root cause filtering (#952)

pixiebrix · Aug 1, 2021 · 58196a3 · 58196a3
1 parent cb5bcfe
commit 58196a3
Show file tree

Hide file tree

Showing 14 changed files with 398 additions and 145 deletions.
diff --git a/src/background/logging.ts b/src/background/logging.ts
@@ -206,9 +206,11 @@ export const recordError = liftBackground(
       const message = getErrorMessage(error);
 
       if (!(await _getDNT())) {
-        // Deserialize the error before passing it to rollbar, otherwise rollbar will assume the
-        // object is the custom payload data
-        // https://docs.rollbar.com/docs/rollbarjs-configuration-reference#rollbarlog
+        // Deserialize the error before passing it to rollbar, otherwise rollbar will assume the object is the custom
+        // payload data. WARNING: the prototype chain is lost during deserialization, so make sure any predicate you
+        // call here also handles deserialized errors properly.
+        // See https://docs.rollbar.com/docs/rollbarjs-configuration-reference#rollbarlog
+        // See https://github.com/sindresorhus/serialize-error/issues/48
         const errorObj = deserializeError(error);
 
         if (hasCancelRootCause(error)) {

diff --git a/src/blocks/effects/event.ts b/src/blocks/effects/event.ts
@@ -75,12 +75,15 @@ export class ElementEvent extends Effect {
     { selector, event }: BlockArg,
     { logger }: BlockOptions
   ): Promise<void> {
+    // eslint-disable-next-line unicorn/no-array-callback-reference -- false positive for JQuery
     const $element = $(document).find(selector);
 
     if ($element.length === 0) {
-      logger.debug(`Element not found for selector: ${selector}`);
+      logger.debug(`Element not found for selector: ${selector as string}`);
     } else if ($element.length > 1) {
-      logger.debug(`Multiple elements found for selector: ${selector}`);
+      logger.debug(
+        `Multiple elements found for selector: ${selector as string}`
+      );
     }
 
     // Triggers the event. NOTE: the event is not "trusted" as being a user action

diff --git a/src/blocks/effects/forms.ts b/src/blocks/effects/forms.ts
@@ -20,12 +20,13 @@ import { registerBlock } from "@/blocks/registry";
 import { BlockArg, BlockOptions, Schema } from "@/core";
 import { boolean } from "@/utils";
 import { BusinessError } from "@/errors";
+import { requireSingleElement } from "@/nativeEditor/utils";
 
 /**
  * Set the value of an input, doing the right thing for check boxes, etc.
  */
 function setValue(
-  $input: JQuery<HTMLElement>,
+  $input: JQuery,
   value: unknown,
   { dispatchEvent = true }: { dispatchEvent?: boolean } = {}
 ) {
@@ -133,26 +134,19 @@ export class FormFill extends Effect {
     }: BlockArg,
     { logger }: BlockOptions
   ): Promise<void> {
-    const $form = $(document).find(formSelector);
-
-    if ($form.length === 0) {
-      throw new BusinessError(`Form not found for selector: ${formSelector}`);
-    } else if ($form.length > 1) {
-      throw new BusinessError(
-        `Selector found ${$form.length} forms: ${formSelector}`
-      );
-    }
+    const $form = $(requireSingleElement(formSelector));
 
     for (const [name, value] of Object.entries(fieldNames)) {
       const $input = $form.find(`[name="${name}"]`);
       if ($input.length === 0) {
-        logger.warn(`Could not find input ${name} on the form`);
+        logger.warn(`No input ${name} exists in the form`);
       }
 
       setValue($input, value, { dispatchEvent: true });
     }
 
     for (const [selector, value] of Object.entries(fieldSelectors)) {
+      // eslint-disable-next-line unicorn/no-array-callback-reference -- false positive for JQuery
       const $input = $form.find(selector);
       if ($input.length === 0) {
         logger.warn(
@@ -174,15 +168,7 @@ export class FormFill extends Effect {
         $form.trigger("submit");
       }
     } else if (typeof submit === "string") {
-      const $submit = $form.find(submit);
-      if ($submit.length === 0) {
-        throw new BusinessError(`Did not find selector ${submit} in the form`);
-      } else if ($submit.length > 1) {
-        throw new BusinessError(
-          `Found multiple elements for the submit selector ${submit} in the form`
-        );
-      }
-
+      const $submit = $(requireSingleElement(submit));
       $submit.trigger("click");
     } else {
       throw new BusinessError("Unexpected argument for property submit");

diff --git a/src/blocks/errors.ts b/src/blocks/errors.ts
@@ -17,11 +17,11 @@
 
 import { castArray } from "lodash";
 import { MessageContext, Schema } from "@/core";
+import { BlockConfig, BlockPipeline } from "@/blocks/combinators";
 import { BusinessError } from "@/errors";
 import { OutputUnit } from "@cfworker/json-schema";
-import { BlockConfig, BlockPipeline } from "@/blocks/combinators";
 
-export class PipelineConfigurationError extends Error {
+export class PipelineConfigurationError extends BusinessError {
   readonly config: BlockPipeline;
 
   constructor(message: string, config: BlockConfig | BlockPipeline) {

diff --git a/src/blocks/readers/jquery.ts b/src/blocks/readers/jquery.ts
@@ -18,7 +18,7 @@
 import { ReaderOutput } from "@/core";
 import { registerFactory } from "@/blocks/readers/factory";
 import { asyncMapValues, sleep } from "@/utils";
-import { BusinessError } from "@/errors";
+import { BusinessError, MultipleElementsFoundError } from "@/errors";
 
 type CastType = "string" | "boolean" | "number";
 
@@ -46,7 +46,7 @@ type Selector = CommonSelector & {
   maxWaitMillis?: number;
 };
 
-type SelectorMap = { [key: string]: string | Selector };
+type SelectorMap = Record<string, string | Selector>;
 
 type Result =
   | string
@@ -55,6 +55,7 @@ type Result =
   | null
   | undefined
   | Result[]
+  // eslint-disable-next-line @typescript-eslint/consistent-indexed-object-style -- can't use Record for recursive signature
   | { [key: string]: Result };
 
 export interface JQueryConfig {
@@ -77,18 +78,20 @@ function castValue(value: string, type?: CastType): Result {
     case "string":
       return value;
     default:
-      throw new BusinessError(`Cast type ${type} not supported`);
+      throw new BusinessError(`Cast type ${type as string} not supported`);
   }
 }
 
 async function processFind(
   $elt: JQuery<HTMLElement | Document>,
   selector: ChildrenSelector
-): Promise<{ [key: string]: Result }> {
-  return asyncMapValues(selector.find, (selector) => select(selector, $elt));
+): Promise<Record<string, Result>> {
+  return asyncMapValues(selector.find, async (selector) =>
+    select(selector, $elt)
+  );
 }
 
-function processElement($elt: JQuery<HTMLElement>, selector: SingleSelector) {
+function processElement($elt: JQuery, selector: SingleSelector) {
   const CONTENT_TYPES: { [key: string]: number | undefined } = {
     text: Node.TEXT_NODE,
     comment: Node.COMMENT_NODE,
@@ -148,7 +151,8 @@ async function select(
   do {
     if ($root) {
       $elt = normalizedSelector.selector
-        ? $root.find(normalizedSelector.selector)
+        ? // eslint-disable-next-line unicorn/no-array-callback-reference -- false positive for jquery
+          $root.find(normalizedSelector.selector)
         : $root;
     } else {
       if (!normalizedSelector.selector) {
@@ -157,6 +161,7 @@ async function select(
         );
       }
 
+      // eslint-disable-next-line unicorn/no-array-callback-reference -- false positive for jquery
       $elt = $(document).find(normalizedSelector.selector);
     }
 
@@ -191,8 +196,9 @@ async function select(
   }
 
   if ($elt.length > 1 && !normalizedSelector.multi) {
-    throw new BusinessError(
-      `Multiple elements found for ${normalizedSelector.selector}. To return a list of values, supply multi=true`
+    throw new MultipleElementsFoundError(
+      normalizedSelector.selector,
+      `Multiple elements found for selector. To return a list of values, supply multi=true`
     );
   } else if ("find" in normalizedSelector) {
     const values = await Promise.all(
@@ -210,10 +216,7 @@ async function select(
 
     const values = $elt
       .map(function () {
-        return processElement(
-          $(this) as JQuery<HTMLElement>,
-          normalizedSelector
-        );
+        return processElement($(this) as JQuery, normalizedSelector);
       })
       .toArray();
     return normalizedSelector.multi ? values : values[0];
@@ -230,7 +233,7 @@ async function read(
     throw new Error("JQuery reader requires the document or element(s)");
   }
 
-  return asyncMapValues(selectors, (selector) => select(selector, $root));
+  return asyncMapValues(selectors, async (selector) => select(selector, $root));
 }
 
 registerFactory("jquery", read);
diff --git a/src/components/fields/FieldTable.tsx b/src/components/fields/FieldTable.tsx
@@ -28,9 +28,9 @@ import { useField, useFormikContext } from "formik";
 import { fieldLabel } from "@/components/fields/fieldUtils";
 import { FontAwesomeIcon } from "@fortawesome/react-fontawesome";
 import { faTrash } from "@fortawesome/free-solid-svg-icons";
-import produce from "immer";
+import { produce } from "immer";
 
-interface PropertyRow {
+interface PropertyRowProps {
   name: string;
   showActions?: boolean;
   readOnly: boolean;
@@ -39,7 +39,16 @@ interface PropertyRow {
   onRename: (newName: string) => void;
 }
 
-const CompositePropertyRow: React.FunctionComponent<PropertyRow> = ({
+interface RowProps {
+  parentSchema: Schema;
+  name: string;
+  property: string;
+  defined: boolean;
+  onDelete: (prop: string) => void;
+  onRename: (oldProp: string, newProp: string) => void;
+}
+
+const CompositePropertyRow: React.FunctionComponent<PropertyRowProps> = ({
   name,
   schema,
   showActions,
@@ -65,12 +74,12 @@ const SimpleValue: React.FunctionComponent<FieldProps<unknown>> = (props) => {
       type="text"
       {...props}
       value={field.value ?? ""}
-      isInvalid={!!meta.error}
+      isInvalid={meta.error != null}
     />
   );
 };
 
-const ValuePropertyRow: React.FunctionComponent<PropertyRow> = ({
+const ValuePropertyRow: React.FunctionComponent<PropertyRowProps> = ({
   readOnly,
   onDelete,
   onRename,
@@ -129,7 +138,7 @@ const ValuePropertyRow: React.FunctionComponent<PropertyRow> = ({
   );
 };
 
-function freshPropertyName(obj: { [key: string]: unknown }) {
+function freshPropertyName(obj: Record<string, unknown>) {
   let x = 1;
   while (Object.prototype.hasOwnProperty.call(obj, `property${x}`)) {
     x++;
@@ -138,19 +147,10 @@ function freshPropertyName(obj: { [key: string]: unknown }) {
   return `property${x}`;
 }
 
-interface RowProps {
-  parentSchema: Schema;
-  name: string;
-  property: string;
-  defined: boolean;
-  onDelete: (prop: string) => void;
-  onRename: (oldProp: string, newProp: string) => void;
-}
-
 const BOOLEAN_SCHEMA: Schema = { type: "string" };
 const FALLBACK_SCHEMA: Schema = { type: "string" };
 
-const PropertyRow: React.FunctionComponent<RowProps> = ({
+const ObjectFieldRow: React.FunctionComponent<RowProps> = ({
   parentSchema,
   defined,
   name,
@@ -168,27 +168,33 @@ const PropertyRow: React.FunctionComponent<RowProps> = ({
       : rawSchema ?? FALLBACK_SCHEMA;
   }, [property, defined, parentSchema]);
 
-  const PropertyRow = useMemo(() => getPropertyRow(propertySchema), [
+  const PropertyRowComponent = useMemo(() => getPropertyRow(propertySchema), [
     propertySchema,
   ]);
-  const deleteProp = useCallback(() => onDelete(property), [
-    property,
-    onDelete,
-  ]);
+
+  const deleteProp = useCallback(() => {
+    onDelete(property);
+  }, [property, onDelete]);
+
   const renameProp = useCallback(
-    (newProp: string) => onRename(property, newProp),
+    (newProp: string) => {
+      onRename(property, newProp);
+    },
     [property, onRename]
   );
 
   return (
-    <PropertyRow
+    <PropertyRowComponent
       key={property}
       name={name}
-      readOnly={!!defined}
+      readOnly={defined}
       schema={propertySchema}
-      showActions={!!parentSchema.additionalProperties}
-      onDelete={!defined ? deleteProp : undefined}
-      onRename={!defined ? renameProp : undefined}
+      showActions={
+        parentSchema.additionalProperties === true ||
+        typeof parentSchema.additionalProperties === "object"
+      }
+      onDelete={defined ? undefined : deleteProp}
+      onRename={defined ? undefined : renameProp}
     />
   );
 };
@@ -228,7 +234,9 @@ export const ObjectField: React.FunctionComponent<FieldProps<unknown>> = ({
       setFieldValue(
         name,
         produce(fieldRef.current.value, (draft: ObjectValue) => {
-          delete draft[property];
+          if (draft != null) {
+            delete draft[property];
+          }
         })
       );
     },
@@ -280,7 +288,7 @@ export const ObjectField: React.FunctionComponent<FieldProps<unknown>> = ({
         </thead>
         <tbody>
           {properties.map((property) => (
-            <PropertyRow
+            <ObjectFieldRow
               key={property}
               parentSchema={schema}
               name={[field.name, property].join(".")}
@@ -304,7 +312,7 @@ export const ObjectField: React.FunctionComponent<FieldProps<unknown>> = ({
 
 export function getPropertyRow(
   schema: Schema
-): React.FunctionComponent<PropertyRow> {
+): React.FunctionComponent<PropertyRowProps> {
   switch (schema?.type) {
     case "array":
     case "object":