From d02fb96e6eb4c446ea217fc02440de0752ac68a0 Mon Sep 17 00:00:00 2001
From: Andrew Kingston <andrew@kingston.dev>
Date: Fri, 14 Oct 2022 18:16:19 +0100
Subject: [PATCH] Invert some client store dependencies to improve performance
 and prevent dependency cycles

---
 packages/client/manifest.json                 |  4 ++
 .../client/src/components/Component.svelte    | 11 +++--
 .../src/components/preview/DNDHandler.svelte  | 19 ++++----
 .../components/preview/HoverIndicator.svelte  |  4 +-
 .../preview/SelectionIndicator.svelte         |  4 +-
 .../src/components/preview/SettingsBar.svelte |  4 +-
 packages/client/src/index.js                  | 15 ++-----
 packages/client/src/stores/builder.js         |  8 ----
 packages/client/src/stores/components.js      | 30 +++++--------
 .../src/stores/derived/dndComponentPath.js    | 13 ++++++
 packages/client/src/stores/derived/index.js   |  5 ++-
 .../client/src/stores/derived/isDragging.js   | 10 -----
 packages/client/src/stores/dnd.js             | 45 ++++++++++++++++---
 packages/client/src/stores/index.js           |  9 +++-
 packages/client/src/stores/screens.js         | 24 +++++++---
 15 files changed, 122 insertions(+), 83 deletions(-)
 create mode 100644 packages/client/src/stores/derived/dndComponentPath.js
 delete mode 100644 packages/client/src/stores/derived/isDragging.js

diff --git a/packages/client/manifest.json b/packages/client/manifest.json
index 0edb5c0f39..14092b7b4d 100644
--- a/packages/client/manifest.json
+++ b/packages/client/manifest.json
@@ -85,6 +85,10 @@
     "icon": "Selection",
     "hasChildren": true,
     "showSettingsBar": true,
+    "size": {
+      "width": 400,
+      "height": 200
+    },
     "styles": [
       "padding",
       "size",
diff --git a/packages/client/src/components/Component.svelte b/packages/client/src/components/Component.svelte
index 3d65db7206..fe5f363152 100644
--- a/packages/client/src/components/Component.svelte
+++ b/packages/client/src/components/Component.svelte
@@ -21,14 +21,14 @@
     devToolsStore,
     componentStore,
     appStore,
-    isDragging,
+    dndIsDragging,
+    dndComponentPath,
   } from "stores"
   import { Helpers } from "@budibase/bbui"
   import { getActiveConditions, reduceConditionActions } from "utils/conditions"
   import Placeholder from "components/app/Placeholder.svelte"
   import ScreenPlaceholder from "components/app/ScreenPlaceholder.svelte"
   import ComponentPlaceholder from "components/app/ComponentPlaceholder.svelte"
-  import { DNDPlaceholderID } from "constants"
 
   export let instance = {}
   export let isLayout = false
@@ -105,7 +105,7 @@
     $builderStore.inBuilder && $builderStore.selectedComponentId === id
   $: inSelectedPath = $componentStore.selectedComponentPath?.includes(id)
   $: inDragPath = inSelectedPath && $builderStore.editMode
-  $: inDndPath = $componentStore.dndPath?.includes(id)
+  $: inDndPath = $dndComponentPath?.includes(id)
 
   // Derive definition properties which can all be optional, so need to be
   // coerced to booleans
@@ -162,7 +162,7 @@
   // nested layers. Only reset this when dragging stops.
   let pad = false
   $: pad = pad || (interactive && hasChildren && inDndPath)
-  $: $isDragging, (pad = false)
+  $: $dndIsDragging, (pad = false)
 
   // Update component context
   $: store.set({
@@ -471,7 +471,6 @@
     class:pad
     class:parent={hasChildren}
     class:block={isBlock}
-    class:placeholder={id === DNDPlaceholderID}
     data-id={id}
     data-name={name}
     data-icon={icon}
@@ -502,7 +501,7 @@
     display: contents;
   }
   .component :global(> *) {
-    transition: padding 260ms ease-in, border 260ms ease-in;
+    transition: padding 260ms ease-out, border 260ms ease-out;
   }
   .component.pad :global(> *) {
     padding: 12px !important;
diff --git a/packages/client/src/components/preview/DNDHandler.svelte b/packages/client/src/components/preview/DNDHandler.svelte
index e2222300e0..392cdceb36 100644
--- a/packages/client/src/components/preview/DNDHandler.svelte
+++ b/packages/client/src/components/preview/DNDHandler.svelte
@@ -4,14 +4,15 @@
   import IndicatorSet from "./IndicatorSet.svelte"
   import {
     builderStore,
-    componentStore,
+    screenStore,
     dndStore,
     dndParent,
-    isDragging,
+    dndIsDragging,
   } from "stores"
   import DNDPlaceholderOverlay from "./DNDPlaceholderOverlay.svelte"
   import { Utils } from "@budibase/frontend-core"
   import { findComponentById } from "utils/components.js"
+  import { DNDPlaceholderID } from "constants"
 
   const ThrottleRate = 130
 
@@ -69,13 +70,13 @@
     const id = component.dataset.id
     const parentId = component.dataset.parent
     const parent = findComponentById(
-      get(componentStore).currentAsset.props,
+      get(screenStore).activeScreen?.props,
       parentId
     )
     const index = parent._children.findIndex(
       x => x._id === component.dataset.id
     )
-    dndStore.actions.startDragging({
+    dndStore.actions.startDraggingExistingComponent({
       id,
       bounds: component.children[0].getBoundingClientRect(),
       parent: parentId,
@@ -127,7 +128,7 @@
     let ephemeralDiv
     if (node.children.length === 1) {
       ephemeralDiv = document.createElement("div")
-      ephemeralDiv.classList.add("placeholder")
+      ephemeralDiv.dataset.id = DNDPlaceholderID
       node.appendChild(ephemeralDiv)
     }
 
@@ -138,7 +139,7 @@
       const child = node.children?.[0] || node
       const bounds = child.getBoundingClientRect()
       return {
-        placeholder: node.classList.contains("placeholder"),
+        placeholder: node.dataset.id === DNDPlaceholderID,
         centerX: bounds.left + bounds.width / 2,
         centerY: bounds.top + bounds.height / 2,
         left: bounds.left,
@@ -249,7 +250,7 @@
     // Convert parent + index into target + mode
     let legacyDropTarget, legacyDropMode
     const parent = findComponentById(
-      get(componentStore).currentAsset?.props,
+      get(screenStore).activeScreen?.props,
       drop.parent
     )
     if (!parent) {
@@ -263,7 +264,7 @@
 
     // Filter out source component and placeholder from consideration
     const children = parent._children?.filter(
-      x => x._id !== "placeholder" && x._id !== source.id
+      x => x._id !== DNDPlaceholderID && x._id !== source.id
     )
 
     // Use inside if no existing children
@@ -316,6 +317,6 @@
   prefix="Inside"
 />
 
-{#if $isDragging}
+{#if $dndIsDragging}
   <DNDPlaceholderOverlay />
 {/if}
diff --git a/packages/client/src/components/preview/HoverIndicator.svelte b/packages/client/src/components/preview/HoverIndicator.svelte
index ed28938d4e..d5583ed3db 100644
--- a/packages/client/src/components/preview/HoverIndicator.svelte
+++ b/packages/client/src/components/preview/HoverIndicator.svelte
@@ -1,7 +1,7 @@
 <script>
   import { onMount, onDestroy } from "svelte"
   import IndicatorSet from "./IndicatorSet.svelte"
-  import { builderStore, isDragging } from "stores"
+  import { builderStore, dndIsDragging } from "stores"
 
   let componentId
   $: zIndex = componentId === $builderStore.selectedComponentId ? 900 : 920
@@ -30,7 +30,7 @@
 </script>
 
 <IndicatorSet
-  componentId={$isDragging ? null : componentId}
+  componentId={$dndIsDragging ? null : componentId}
   color="var(--spectrum-global-color-static-blue-200)"
   transition
   {zIndex}
diff --git a/packages/client/src/components/preview/SelectionIndicator.svelte b/packages/client/src/components/preview/SelectionIndicator.svelte
index a4037e5c9c..49341a7976 100644
--- a/packages/client/src/components/preview/SelectionIndicator.svelte
+++ b/packages/client/src/components/preview/SelectionIndicator.svelte
@@ -1,5 +1,5 @@
 <script>
-  import { builderStore, isDragging } from "stores"
+  import { builderStore, dndIsDragging } from "stores"
   import IndicatorSet from "./IndicatorSet.svelte"
 
   $: color = $builderStore.editMode
@@ -8,7 +8,7 @@
 </script>
 
 <IndicatorSet
-  componentId={$isDragging ? null : $builderStore.selectedComponentId}
+  componentId={$dndIsDragging ? null : $builderStore.selectedComponentId}
   {color}
   zIndex="910"
   transition
diff --git a/packages/client/src/components/preview/SettingsBar.svelte b/packages/client/src/components/preview/SettingsBar.svelte
index 3716f83d4a..757d8b88db 100644
--- a/packages/client/src/components/preview/SettingsBar.svelte
+++ b/packages/client/src/components/preview/SettingsBar.svelte
@@ -3,7 +3,7 @@
   import SettingsButton from "./SettingsButton.svelte"
   import SettingsColorPicker from "./SettingsColorPicker.svelte"
   import SettingsPicker from "./SettingsPicker.svelte"
-  import { builderStore, componentStore, isDragging } from "stores"
+  import { builderStore, componentStore, dndIsDragging } from "stores"
   import { domDebounce } from "utils/domDebounce"
 
   const verticalOffset = 36
@@ -16,7 +16,7 @@
   let measured = false
 
   $: definition = $componentStore.selectedComponentDefinition
-  $: showBar = definition?.showSettingsBar && !$isDragging
+  $: showBar = definition?.showSettingsBar && !$dndIsDragging
   $: settings = getBarSettings(definition)
 
   const getBarSettings = definition => {
diff --git a/packages/client/src/index.js b/packages/client/src/index.js
index 383421cc35..706cb4fc9f 100644
--- a/packages/client/src/index.js
+++ b/packages/client/src/index.js
@@ -64,20 +64,11 @@ const loadBudibase = async () => {
     } else if (name === "dragging-new-component") {
       const { dragging, component } = payload
       if (dragging) {
-        dndStore.actions.startDragging({
-          id: null,
-          parent: null,
-          bounds: {
-            height: 64,
-            width: 128,
-          },
-          index: null,
-          newComponentType: component,
-        })
-        builderStore.actions.setDraggingNewComponent(true)
+        const definition =
+          componentStore.actions.getComponentDefinition(component)
+        dndStore.actions.startDraggingNewComponent({ component, definition })
       } else {
         dndStore.actions.reset()
-        builderStore.actions.setDraggingNewComponent(false)
       }
     }
   }
diff --git a/packages/client/src/stores/builder.js b/packages/client/src/stores/builder.js
index 135c08343e..b937b7f696 100644
--- a/packages/client/src/stores/builder.js
+++ b/packages/client/src/stores/builder.js
@@ -16,7 +16,6 @@ const createBuilderStore = () => {
     theme: null,
     customTheme: null,
     previewDevice: "desktop",
-    draggingNewComponent: false,
     navigation: null,
     hiddenComponentIds: [],
     usedPlugins: null,
@@ -68,19 +67,12 @@ const createBuilderStore = () => {
       })
     },
     dropNewComponent: (component, parent, index) => {
-      console.log("dispatch", component, parent, index)
       dispatchEvent("drop-new-component", {
         component,
         parent,
         index,
       })
     },
-    setDraggingNewComponent: draggingNewComponent => {
-      if (draggingNewComponent === get(store).draggingNewComponent) {
-        return
-      }
-      store.update(state => ({ ...state, draggingNewComponent }))
-    },
     setEditMode: enabled => {
       if (enabled === get(store).editMode) {
         return
diff --git a/packages/client/src/stores/components.js b/packages/client/src/stores/components.js
index e246299741..b34dfe375d 100644
--- a/packages/client/src/stores/components.js
+++ b/packages/client/src/stores/components.js
@@ -4,7 +4,6 @@ import { findComponentById, findComponentPathById } from "../utils/components"
 import { devToolsStore } from "./devTools"
 import { screenStore } from "./screens"
 import { builderStore } from "./builder"
-import { dndParent } from "./dnd.js"
 import Router from "../components/Router.svelte"
 import DNDPlaceholder from "../components/preview/DNDPlaceholder.svelte"
 import * as AppComponents from "../components/app/index.js"
@@ -20,28 +19,22 @@ const createComponentStore = () => {
   })
 
   const derivedStore = derived(
-    [store, builderStore, devToolsStore, screenStore, dndParent],
-    ([$store, $builderState, $devToolsState, $screenState, $dndParent]) => {
+    [store, builderStore, devToolsStore, screenStore],
+    ([$store, $builderStore, $devToolsStore, $screenStore]) => {
+      const { inBuilder, selectedComponentId } = $builderStore
+
       // Avoid any of this logic if we aren't in the builder preview
-      if (!$builderState.inBuilder && !$devToolsState.visible) {
+      if (!inBuilder && !$devToolsStore.visible) {
         return {}
       }
 
-      // Derive the selected component instance and definition
-      let asset
-      const { screen, selectedComponentId } = $builderState
-      if ($builderState.inBuilder) {
-        asset = screen
-      } else {
-        asset = $screenState.activeScreen
-      }
-      const component = findComponentById(asset?.props, selectedComponentId)
+      const root = $screenStore.activeScreen?.props
+      const component = findComponentById(root, selectedComponentId)
       const definition = getComponentDefinition(component?._component)
 
       // Derive the selected component path
       const selectedPath =
-        findComponentPathById(asset?.props, selectedComponentId) || []
-      const dndPath = findComponentPathById(asset?.props, $dndParent) || []
+        findComponentPathById(root, selectedComponentId) || []
 
       return {
         customComponentManifest: $store.customComponentManifest,
@@ -51,9 +44,6 @@ const createComponentStore = () => {
         selectedComponentDefinition: definition,
         selectedComponentPath: selectedPath?.map(component => component._id),
         mountedComponentCount: Object.keys($store.mountedComponents).length,
-        currentAsset: asset,
-        dndPath: dndPath?.map(component => component._id),
-        dndDepth: dndPath?.length || 0,
       }
     }
   )
@@ -101,8 +91,8 @@ const createComponentStore = () => {
   }
 
   const getComponentById = id => {
-    const asset = get(derivedStore).currentAsset
-    return findComponentById(asset?.props, id)
+    const root = get(screenStore).activeScreen?.props
+    return findComponentById(root, id)
   }
 
   const getComponentDefinition = type => {
diff --git a/packages/client/src/stores/derived/dndComponentPath.js b/packages/client/src/stores/derived/dndComponentPath.js
new file mode 100644
index 0000000000..58fb395dd6
--- /dev/null
+++ b/packages/client/src/stores/derived/dndComponentPath.js
@@ -0,0 +1,13 @@
+import { derived } from "svelte/store"
+import { findComponentPathById } from "utils/components.js"
+import { dndParent } from "../dnd.js"
+import { screenStore } from "../screens.js"
+
+export const dndComponentPath = derived(
+  [dndParent, screenStore],
+  ([$dndParent, $screenStore]) => {
+    const root = $screenStore.activeScreen?.props
+    const path = findComponentPathById(root, $dndParent) || []
+    return path?.map(component => component._id)
+  }
+)
diff --git a/packages/client/src/stores/derived/index.js b/packages/client/src/stores/derived/index.js
index 85df1aaafa..4f6a6ab91d 100644
--- a/packages/client/src/stores/derived/index.js
+++ b/packages/client/src/stores/derived/index.js
@@ -1,2 +1,5 @@
-export { isDragging } from "./isDragging.js"
+// These derived stores are pulled out from their parent stores to avoid
+// dependency loops. By inverting store dependencies and extracting them
+// separately we can keep our actual stores lean and performant.
 export { currentRole } from "./currentRole.js"
+export { dndComponentPath } from "./dndComponentPath.js"
diff --git a/packages/client/src/stores/derived/isDragging.js b/packages/client/src/stores/derived/isDragging.js
deleted file mode 100644
index d3edd586c6..0000000000
--- a/packages/client/src/stores/derived/isDragging.js
+++ /dev/null
@@ -1,10 +0,0 @@
-import { derived } from "svelte/store"
-import { dndStore } from "../dnd"
-import { builderStore } from "../builder.js"
-
-// Derive whether we are dragging or not
-export const isDragging = derived(
-  [dndStore, builderStore],
-  ([$dndStore, $builderStore]) =>
-    $dndStore.source != null || $builderStore.draggingNewComponent
-)
diff --git a/packages/client/src/stores/dnd.js b/packages/client/src/stores/dnd.js
index 805ec8c339..1e7d7a6d50 100644
--- a/packages/client/src/stores/dnd.js
+++ b/packages/client/src/stores/dnd.js
@@ -2,6 +2,10 @@ import { writable, derived } from "svelte/store"
 
 const createDndStore = () => {
   const initialState = {
+    // Flags for whether we are inserting a new component or not
+    isNewComponent: false,
+    newComponentType: null,
+
     // Info about the dragged component
     source: null,
 
@@ -13,12 +17,32 @@ const createDndStore = () => {
   }
   const store = writable(initialState)
 
-  // newComponentType is an optional field to signify we are creating a new
-  // component rather than moving an existing one
-  const startDragging = ({ id, parent, bounds, index, newComponentType }) => {
+  const startDraggingExistingComponent = ({ id, parent, bounds, index }) => {
     store.set({
       ...initialState,
-      source: { id, parent, bounds, index, newComponentType },
+      source: { id, parent, bounds, index },
+    })
+  }
+
+  const startDraggingNewComponent = ({ type, definition }) => {
+    if (!type || !definition) {
+      return
+    }
+
+    // Get size of new component so we can show a properly sized placeholder
+    const width = definition.size?.width || 128
+    const height = definition.size?.height || 64
+
+    store.set({
+      ...initialState,
+      isNewComponent: true,
+      newComponentType: type,
+      source: {
+        id: null,
+        parent: null,
+        bounds: { height, width },
+        index: null,
+      },
     })
   }
 
@@ -43,7 +67,8 @@ const createDndStore = () => {
   return {
     subscribe: store.subscribe,
     actions: {
-      startDragging,
+      startDraggingExistingComponent,
+      startDraggingNewComponent,
       updateTarget,
       updateDrop,
       reset,
@@ -52,9 +77,19 @@ const createDndStore = () => {
 }
 
 export const dndStore = createDndStore()
+
+// The DND store is updated extremely frequently, so we can greatly improve
+// performance by deriving any state that needs to be externally observed.
+// By doing this and using primitives, we can avoid invalidating other stores
+// or components which depend on DND state unless values actually change.
+export const dndIsDragging = derived(dndStore, $dndStore => !!$dndStore.source)
 export const dndParent = derived(dndStore, $dndStore => $dndStore.drop?.parent)
 export const dndIndex = derived(dndStore, $dndStore => $dndStore.drop?.index)
 export const dndBounds = derived(
   dndStore,
   $dndStore => $dndStore.source?.bounds
 )
+export const dndIsNewComponent = derived(
+  dndStore,
+  $dndStore => $dndStore.isNewComponent
+)
diff --git a/packages/client/src/stores/index.js b/packages/client/src/stores/index.js
index f86c0a0517..c431302d43 100644
--- a/packages/client/src/stores/index.js
+++ b/packages/client/src/stores/index.js
@@ -15,7 +15,14 @@ export { uploadStore } from "./uploads.js"
 export { rowSelectionStore } from "./rowSelection.js"
 export { blockStore } from "./blocks.js"
 export { environmentStore } from "./environment"
-export { dndStore, dndIndex, dndParent, dndBounds } from "./dnd"
+export {
+  dndStore,
+  dndIndex,
+  dndParent,
+  dndBounds,
+  dndIsNewComponent,
+  dndIsDragging,
+} from "./dnd"
 
 // Context stores are layered and duplicated, so it is not a singleton
 export { createContextStore } from "./context"
diff --git a/packages/client/src/stores/screens.js b/packages/client/src/stores/screens.js
index b0bc06eee9..0787610d80 100644
--- a/packages/client/src/stores/screens.js
+++ b/packages/client/src/stores/screens.js
@@ -2,7 +2,7 @@ import { derived } from "svelte/store"
 import { routeStore } from "./routes"
 import { builderStore } from "./builder"
 import { appStore } from "./app"
-import { dndIndex, dndParent } from "./dnd.js"
+import { dndIndex, dndParent, dndIsNewComponent } from "./dnd.js"
 import { RoleUtils } from "@budibase/frontend-core"
 import { findComponentById, findComponentParent } from "../utils/components.js"
 import { Helpers } from "@budibase/bbui"
@@ -10,8 +10,22 @@ import { DNDPlaceholderID, DNDPlaceholderType } from "constants"
 
 const createScreenStore = () => {
   const store = derived(
-    [appStore, routeStore, builderStore, dndParent, dndIndex],
-    ([$appStore, $routeStore, $builderStore, $dndParent, $dndIndex]) => {
+    [
+      appStore,
+      routeStore,
+      builderStore,
+      dndParent,
+      dndIndex,
+      dndIsNewComponent,
+    ],
+    ([
+      $appStore,
+      $routeStore,
+      $builderStore,
+      $dndParent,
+      $dndIndex,
+      $dndIsNewComponent,
+    ]) => {
       let activeLayout, activeScreen
       let screens
 
@@ -50,8 +64,8 @@ const createScreenStore = () => {
       if (activeScreen && $dndParent && $dndIndex != null) {
         // Remove selected component from tree if we are moving an existing
         // component
-        const { selectedComponentId, draggingNewComponent } = $builderStore
-        if (!draggingNewComponent) {
+        const { selectedComponentId } = $builderStore
+        if (!$dndIsNewComponent) {
           let selectedParent = findComponentParent(
             activeScreen.props,
             selectedComponentId