Data selector - filter matching variables (#5921)

# [Playground](https://utopia.fish/p/67b6ffa9-careful-alto/?branch_name=fix-data-selector-filtering) ## Problem The data picker highlights variables that do not actually fit the place they are being picked for ## Fix Use the (slightly revamped version of the) existing matching logic to highlight matching variables (see the `orderVariablesForRelevance` function for details) ### Details - removed the unused `disabled` prop from `DataPickerOption` and `VariableOptionBase` - remove the bespoke filtering logic for lists - update the matched prop so that it can express whether the option itself matches, has matching children or does not match at all (this is the `VariableMatches` type) - use this type to color the cartouches - add some extra logic to match options to the `children` prop - this doesn't take into account the annotation for the children prop yet - use a safe way to stringify values in `ValuePreviewColumn` (since stringifying react elements throws a circular object error) ### Manual Tests I hereby swear that: - [x] I opened a hydrogen project and it loaded - [x] I could navigate to various routes in Preview mode --------- Co-authored-by: Balazs Bajorics <[email protected]>
concrete-utopia · Jun 13, 2024 · b56b5e6 · b56b5e6
1 parent 3c847c5
commit b56b5e6
Show file tree

Hide file tree

Showing 12 changed files with 66 additions and 80 deletions.
diff --git a/editor/src/components/inspector/sections/component-section/cartouche-ui.tsx b/editor/src/components/inspector/sections/component-section/cartouche-ui.tsx
@@ -144,7 +144,7 @@ export const CartoucheUI = React.forwardRef(
                 /* Beginning of string */
                 ...(shouldUseRtlCSS
                   ? {
-                      direction: source === 'literal' ? 'ltr' : 'rtl', // TODO we need a better way to ellipsize the beginnign because rtl eats ' " marks
+                      direction: source === 'literal' ? 'ltr' : 'rtl', // TODO we need a better way to ellipsize the beginning because rtl eats ' " marks
                     }
                   : {}),
                 textAlign: 'left',

diff --git a/editor/src/components/inspector/sections/component-section/data-picker-popup.tsx b/editor/src/components/inspector/sections/component-section/data-picker-popup.tsx
@@ -9,7 +9,6 @@ export interface PrimitiveOption {
   definedElsewhere: string
   depth: number
   valuePath: Array<string | number>
-  disabled: boolean
 }
 
 export interface ArrayOption {
@@ -19,7 +18,6 @@ export interface ArrayOption {
   definedElsewhere: string
   children: Array<DataPickerOption>
   valuePath: Array<string | number>
-  disabled: boolean
 }
 
 export interface ObjectOption {
@@ -29,7 +27,6 @@ export interface ObjectOption {
   definedElsewhere: string
   children: Array<DataPickerOption>
   valuePath: Array<string | number>
-  disabled: boolean
 }
 
 export interface JSXOption {
@@ -38,7 +35,6 @@ export interface JSXOption {
   definedElsewhere: string
   depth: number
   valuePath: Array<string | number>
-  disabled: boolean
 }
 
 export type DataPickerOption = PrimitiveOption | ArrayOption | ObjectOption | JSXOption

diff --git a/editor/src/components/inspector/sections/component-section/data-picker-utils.tsx b/editor/src/components/inspector/sections/component-section/data-picker-utils.tsx
@@ -17,13 +17,11 @@ import type { ProjectContentTreeRoot } from '../../../assets'
 import { insertionCeilingToString, type FileRootPath } from '../../../canvas/ui-jsx-canvas'
 import type { AllElementProps } from '../../../editor/store/editor-state'
 import type { ArrayInfo, JSXInfo, ObjectInfo, PrimitiveInfo } from './variables-in-scope-utils'
-import type { CartoucheUIProps } from './cartouche-ui'
 
 interface VariableOptionBase {
   depth: number
   definedElsewhere: string
   valuePath: Array<string | number>
-  disabled: boolean
   insertionCeiling: ElementPath | FileRootPath
   isChildOfArray: boolean
 }

diff --git a/editor/src/components/inspector/sections/component-section/data-reference-cartouche.tsx b/editor/src/components/inspector/sections/component-section/data-reference-cartouche.tsx
@@ -24,6 +24,7 @@ import { DataPickerPreferredAllAtom, jsxElementChildToValuePath } from './data-p
 import { useAtom } from 'jotai'
 import type { CartoucheDataType, CartoucheHighlight, CartoucheUIProps } from './cartouche-ui'
 import { CartoucheUI } from './cartouche-ui'
+import * as PP from '../../../../core/shared/property-path'
 
 interface DataReferenceCartoucheControlProps {
   elementPath: ElementPath
@@ -95,7 +96,7 @@ export const DataReferenceCartoucheControl = React.memo(
       props.renderedAt.type === 'element-property-path'
         ? props.renderedAt.elementPropertyPath.propertyPath
         : props.renderedAt.type === 'child-node'
-        ? null
+        ? PP.create('children')
         : assertNever(props.renderedAt)
 
     const [preferredAllState] = useAtom(DataPickerPreferredAllAtom)

diff --git a/editor/src/components/inspector/sections/component-section/data-selector-cartouche.tsx b/editor/src/components/inspector/sections/component-section/data-selector-cartouche.tsx
@@ -9,6 +9,7 @@ import { MapCounterUi } from '../../../navigator/navigator-item/map-counter'
 import type { CartoucheUIProps } from './cartouche-ui'
 import { CartoucheUI } from './cartouche-ui'
 import type { DataPickerOption } from './data-picker-utils'
+import { variableMatches } from './variables-in-scope-utils'
 
 interface DataPickerCartoucheProps {
   data: DataPickerOption
@@ -27,9 +28,9 @@ export const DataPickerCartouche = React.memo(
         key={data.valuePath.toString()}
         source={dataSource ?? 'internal'}
         datatype={childTypeToCartoucheDataType(data.type)}
-        selected={!data.disabled && selected}
-        highlight={data.disabled ? 'disabled' : null}
-        role={forceRole ?? (data.disabled ? 'information' : 'selection')}
+        selected={variableMatches(data.variableInfo) && selected}
+        highlight={variableMatches(data.variableInfo) ? null : 'disabled'}
+        role={forceRole ?? (variableMatches(data.variableInfo) ? 'selection' : 'information')}
         testId={`data-selector-option-${data.variableInfo.expression}`}
         badge={
           data.type === 'array' ? (

diff --git a/editor/src/components/inspector/sections/component-section/data-selector-columns.tsx b/editor/src/components/inspector/sections/component-section/data-selector-columns.tsx
@@ -109,7 +109,7 @@ interface ValuePreviewColumnProps {
 }
 
 const ValuePreviewColumn = React.memo((props: ValuePreviewColumnProps) => {
-  const text = JSON.stringify(props.data.variableInfo.value, undefined, 2)
+  const text = safeJSONStringify(props.data.variableInfo.value)
   const ref = useScrollIntoView(true)
   return (
     <DataSelectorFlexColumn ref={ref}>
@@ -235,3 +235,11 @@ function useScrollIntoView(shouldScroll: boolean) {
 
   return elementRef
 }
+
+function safeJSONStringify(value: unknown): string | null {
+  try {
+    return JSON.stringify(value, undefined, 2)
+  } catch {
+    return null
+  }
+}
diff --git a/editor/src/components/inspector/sections/component-section/data-selector-modal.tsx b/editor/src/components/inspector/sections/component-section/data-selector-modal.tsx
@@ -8,7 +8,6 @@ import {
   CanvasContextMenuPortalTargetID,
   arrayEqualsByReference,
   arrayEqualsByValue,
-  assertNever,
 } from '../../../../core/shared/utils'
 import { when } from '../../../../utils/react-conditionals'
 import {
@@ -33,6 +32,7 @@ import {
 import { DataSelectorColumns } from './data-selector-columns'
 import { DataSelectorLeftSidebar } from './data-selector-left-sidebar'
 import { DataSelectorSearch } from './data-selector-search'
+import { variableMatches } from './variables-in-scope-utils'
 import { DataPickerCartouche } from './data-selector-cartouche'
 
 export const DataSelectorPopupBreadCrumbsTestId = 'data-selector-modal-top-bar'
@@ -158,7 +158,7 @@ export const DataSelectorModal = React.memo(
         if (variable == null) {
           return
         }
-        if (variable.disabled) {
+        if (variable.variableInfo.matches !== 'matches') {
           return
         }
         onPropertyPicked(
@@ -176,6 +176,9 @@ export const DataSelectorModal = React.memo(
 
       const searchNullOrEmpty = searchTerm == null || searchTerm.length < 1
 
+      const selectedVariableIsDisabled =
+        optionalMap((v) => !variableMatches(v.variableInfo), selectedVariableOption) ?? true
+
       return (
         <InspectorModal
           offsetX={20}
@@ -353,7 +356,7 @@ export const DataSelectorModal = React.memo(
                           height: 24,
                           width: 81,
                           textAlign: 'center',
-                          ...disabledButtonStyles(selectedVariableOption?.disabled ?? true),
+                          ...disabledButtonStyles(selectedVariableIsDisabled),
                         }}
                         onClick={applyVariable}
                       >

diff --git a/editor/src/components/inspector/sections/component-section/data-selector-search.tsx b/editor/src/components/inspector/sections/component-section/data-selector-search.tsx
@@ -5,6 +5,7 @@ import { assertNever } from '../../../../core/shared/utils'
 import { FlexRow, Icons, UtopiaStyles } from '../../../../uuiui'
 import { type DataPickerOption, type ObjectPath } from './data-picker-utils'
 import { DataPickerCartouche } from './data-selector-cartouche'
+import { when } from '../../../../utils/react-conditionals'
 
 export interface DataSelectorSearchProps {
   setNavigatedToPath: (_: DataPickerOption) => void
@@ -56,9 +57,10 @@ export const DataSelectorSearch = React.memo(
                         searchTerm={searchTerm}
                         fontWeightForMatch={900}
                       />
-                      {i < searchResult.valuePath.length - 1 ? (
-                        <Icons.ExpansionArrowRight color='primary' />
-                      ) : null}
+                      {when(
+                        i < searchResult.valuePath.length - 1,
+                        <Icons.ExpansionArrowRight color='primary' />,
+                      )}
                     </React.Fragment>
                   ))}
                 </DataPickerCartouche>

diff --git a/editor/src/components/inspector/sections/component-section/variables-in-scope-utils.spec.ts b/editor/src/components/inspector/sections/component-section/variables-in-scope-utils.spec.ts
@@ -51,7 +51,7 @@ describe('orderVariablesForRelevance', () => {
             ],
             "type": "elementpath",
           },
-          "matches": false,
+          "matches": "child-matches",
           "props": Array [
             Object {
               "expression": "style['left']",
@@ -64,7 +64,7 @@ describe('orderVariablesForRelevance', () => {
                 ],
                 "type": "elementpath",
               },
-              "matches": true,
+              "matches": "matches",
               "type": "primitive",
               "value": 300,
             },
@@ -79,7 +79,7 @@ describe('orderVariablesForRelevance', () => {
                 ],
                 "type": "elementpath",
               },
-              "matches": false,
+              "matches": "does-not-match",
               "type": "primitive",
               "value": "relative",
             },
@@ -138,7 +138,7 @@ describe('orderVariablesForRelevance', () => {
             ],
             "type": "elementpath",
           },
-          "matches": true,
+          "matches": "matches",
           "props": Array [
             Object {
               "expression": "style['left']",
@@ -151,7 +151,7 @@ describe('orderVariablesForRelevance', () => {
                 ],
                 "type": "elementpath",
               },
-              "matches": false,
+              "matches": "does-not-match",
               "type": "primitive",
               "value": 300,
             },
@@ -166,7 +166,7 @@ describe('orderVariablesForRelevance', () => {
                 ],
                 "type": "elementpath",
               },
-              "matches": false,
+              "matches": "does-not-match",
               "type": "primitive",
               "value": "relative",
             },

diff --git a/editor/src/components/inspector/sections/component-section/variables-in-scope-utils.ts b/editor/src/components/inspector/sections/component-section/variables-in-scope-utils.ts
@@ -59,7 +59,6 @@ function valuesFromObject(
           )
           .map(patchDefinedElsewhereInfo),
         valuePath: valuePath,
-        disabled: false,
         isChildOfArray: isChildOfArray,
       },
     ]
@@ -84,7 +83,6 @@ function valuesFromObject(
           )
           .map(patchDefinedElsewhereInfo),
         valuePath: valuePath,
-        disabled: false,
         isChildOfArray: isChildOfArray,
       },
     ]
@@ -111,7 +109,6 @@ function valuesFromVariable(
           definedElsewhere: originalObjectName,
           depth: depth,
           valuePath: valuePath,
-          disabled: false,
           isChildOfArray: isChildOfArray,
         },
       ]
@@ -142,7 +139,6 @@ function valuesFromVariable(
           definedElsewhere: originalObjectName,
           depth: depth,
           valuePath: valuePath,
-          disabled: false,
           isChildOfArray: isChildOfArray,
         },
       ]
@@ -169,13 +165,27 @@ function usePropertyControlDescriptions(
   return controlForProp[0] ?? null
 }
 
+export type VariableMatches = 'matches' | 'child-matches' | 'does-not-match'
+
+export const variableMatches = (variable: VariableInfoBase) => {
+  switch (variable.matches) {
+    case 'matches':
+      return true
+    case 'child-matches':
+    case 'does-not-match':
+      return false
+    default:
+      assertNever(variable.matches)
+  }
+}
+
 interface VariableInfoBase {
   type: string
   expression: string
   expressionPathPart: string | number
   value: unknown
   insertionCeiling: ElementPath | FileRootPath
-  matches: boolean
+  matches: VariableMatches
 }
 
 export interface PrimitiveInfo extends VariableInfoBase {
@@ -227,7 +237,7 @@ export function variableInfoFromValue(
         expressionPathPart: expressionPathPart,
         value: value,
         insertionCeiling: insertionCeiling,
-        matches: false,
+        matches: 'does-not-match',
       }
     case 'object':
       if (value == null) {
@@ -237,7 +247,7 @@ export function variableInfoFromValue(
           expressionPathPart: expressionPathPart,
           value: value,
           insertionCeiling: insertionCeiling,
-          matches: false,
+          matches: 'does-not-match',
         }
       }
       if (Array.isArray(value)) {
@@ -247,7 +257,7 @@ export function variableInfoFromValue(
           expressionPathPart: expressionPathPart,
           value: value,
           insertionCeiling: insertionCeiling,
-          matches: false,
+          matches: 'does-not-match',
           elements: mapDropNulls(
             (e, idx) =>
               variableInfoFromValue(
@@ -268,7 +278,7 @@ export function variableInfoFromValue(
           expressionPathPart: expressionPathPart,
           value: value,
           insertionCeiling: insertionCeiling,
-          matches: false,
+          matches: 'does-not-match',
         }
       }
       return {
@@ -277,7 +287,7 @@ export function variableInfoFromValue(
         expressionPathPart: expressionPathPart,
         value: value,
         insertionCeiling: insertionCeiling,
-        matches: false,
+        matches: 'does-not-match',
         props: mapDropNulls(([key, propValue]) => {
           return variableInfoFromValue(
             `${expression}['${key}']`,
@@ -366,6 +376,9 @@ export function orderVariablesForRelevance(
       targetControlDescription != null &&
       variableMatchesControlDescription(variable.value, targetControlDescription)
 
+    const valueMatchesChildrenProp =
+      targetPropertyName === 'children' && isValidReactNode(variable.value)
+
     const valueMatchesCurrentPropValue =
       currentPropertyValue.type === 'existing' &&
       variableShapesMatch(currentPropertyValue.value, variable.value)
@@ -375,15 +388,15 @@ export function orderVariablesForRelevance(
       (variable.type === 'object' && variable.props.some((e) => e.matches))
 
     if (valueExactlyMatchesPropertyName || variableCanBeMappedOver) {
-      valuesExactlyMatchingPropertyName.push({ ...variable, matches: true })
+      valuesExactlyMatchingPropertyName.push({ ...variable, matches: 'matches' })
     } else if (valueExactlyMatchesControlDescription) {
-      valuesExactlyMatchingPropertyDescription.push({ ...variable, matches: true })
-    } else if (valueMatchesControlDescription) {
-      valuesMatchingPropertyDescription.push({ ...variable, matches: true })
+      valuesExactlyMatchingPropertyDescription.push({ ...variable, matches: 'matches' })
+    } else if (valueMatchesControlDescription || valueMatchesChildrenProp) {
+      valuesMatchingPropertyDescription.push({ ...variable, matches: 'matches' })
     } else if (arrayOrObjectChildMatches) {
-      valueElementMatches.push({ ...variable, matches: false })
+      valueElementMatches.push({ ...variable, matches: 'child-matches' })
     } else if (valueMatchesCurrentPropValue) {
-      valuesMatchingPropertyShape.push({ ...variable, matches: true })
+      valuesMatchingPropertyShape.push({ ...variable, matches: 'matches' })
     } else {
       restOfValues.push(variable)
     }

diff --git a/editor/src/components/inspector/sections/data-reference-section.tsx b/editor/src/components/inspector/sections/data-reference-section.tsx
@@ -26,6 +26,7 @@ import {
 } from './component-section/variables-in-scope-utils'
 import { jsxElementChildToValuePath } from './component-section/data-picker-utils'
 import { CartoucheInspectorWrapper } from './component-section/cartouche-control'
+import * as PP from '../../../core/shared/property-path'
 
 export const DataReferenceSectionId = 'code-element-section-test-id'
 
@@ -84,7 +85,7 @@ export const DataReferenceSection = React.memo(({ paths }: { paths: ElementPath[
 
   const varsInScope = useVariablesInScopeForSelectedElement(
     elementPathForDataPicker,
-    null,
+    PP.create('children'),
     preferredAllState,
   )