From 524c46a554acc834ddc2ec0e99dac0d80c4f1460 Mon Sep 17 00:00:00 2001
From: Andrew Kingston <andrew@kingston.dev>
Date: Wed, 1 Mar 2023 16:10:24 +0000
Subject: [PATCH] Fix multiple issues, clean up rendering, improve performance

---
 .../src/components/sheet/HeaderRow.svelte     |  8 ---
 .../src/components/sheet/NewRow.svelte        |  2 -
 .../src/components/sheet/ScrollOverlay.svelte | 35 ++++++++++---
 .../src/components/sheet/Sheet.svelte         |  2 -
 .../src/components/sheet/SheetCell.svelte     | 13 ++---
 .../src/components/sheet/SheetRow.svelte      |  3 --
 .../sheet/SheetScrollWrapper.svelte           | 28 +++++++++--
 .../src/components/sheet/stores/rows.js       |  7 ++-
 .../src/components/sheet/stores/viewport.js   | 49 +++++++------------
 9 files changed, 79 insertions(+), 68 deletions(-)

diff --git a/packages/frontend-core/src/components/sheet/HeaderRow.svelte b/packages/frontend-core/src/components/sheet/HeaderRow.svelte
index 71fd1237cc..01a50d07f2 100644
--- a/packages/frontend-core/src/components/sheet/HeaderRow.svelte
+++ b/packages/frontend-core/src/components/sheet/HeaderRow.svelte
@@ -38,13 +38,5 @@
 <style>
   .row {
     display: flex;
-    position: sticky;
-    top: 0;
-    width: inherit;
-    z-index: 10;
-    height: var(--cell-height);
-  }
-  .row :global(> :last-child) {
-    border-right-width: 1px;
   }
 </style>
diff --git a/packages/frontend-core/src/components/sheet/NewRow.svelte b/packages/frontend-core/src/components/sheet/NewRow.svelte
index 764c207228..63985febf0 100644
--- a/packages/frontend-core/src/components/sheet/NewRow.svelte
+++ b/packages/frontend-core/src/components/sheet/NewRow.svelte
@@ -29,8 +29,6 @@
 <style>
   .row {
     display: flex;
-    width: inherit;
-    height: var(--cell-height);
   }
   :global(.sheet:not(.is-resizing):not(.is-reordering) .row:hover .cell) {
     background: var(--cell-background-hover);
diff --git a/packages/frontend-core/src/components/sheet/ScrollOverlay.svelte b/packages/frontend-core/src/components/sheet/ScrollOverlay.svelte
index 96fb7996c8..bdc127c8c3 100644
--- a/packages/frontend-core/src/components/sheet/ScrollOverlay.svelte
+++ b/packages/frontend-core/src/components/sheet/ScrollOverlay.svelte
@@ -20,22 +20,43 @@
 
   // Calculate V scrollbar size and offset
   $: contentHeight = ($rows.length + 1) * cellHeight
-  $: barHeight = Math.max(50, (height / contentHeight) * height)
-  $: availHeight = height - barHeight - 2 * barOffset
-  $: maxScrollTop = contentHeight - height
+  $: renderHeight = height - 2 * barOffset
+  $: barHeight = Math.max(50, (height / contentHeight) * renderHeight)
+  $: availHeight = renderHeight - barHeight
+  $: maxScrollTop = Math.max(contentHeight - height, 0)
   $: barTop = barOffset + cellHeight + availHeight * (scrollTop / maxScrollTop)
 
   // Calculate H scrollbar size and offset
   $: contentWidth = calculateContentWidth($columns, $stickyColumn)
   $: totalWidth = width + 40 + $stickyColumn?.width || 0
-  $: barWidth = Math.max(50, (totalWidth / contentWidth) * totalWidth)
-  $: availWidth = totalWidth - barWidth - 2 * barOffset
-  $: maxScrollLeft = contentWidth - totalWidth
+  $: renderWidth = totalWidth - 2 * barOffset
+  $: barWidth = Math.max(50, (totalWidth / contentWidth) * renderWidth)
+  $: availWidth = renderWidth - barWidth
+  $: maxScrollLeft = Math.max(contentWidth - totalWidth, 0)
   $: barLeft = barOffset + availWidth * (scrollLeft / maxScrollLeft)
 
   // Calculate whether to show scrollbars or not
   $: showVScrollbar = contentHeight > height
-  $: showHScrollbar = contentWidth > width
+  $: showHScrollbar = contentWidth > totalWidth
+
+  // Ensure scroll state never goes invalid, which can happen when changing
+  // rows or tables
+  $: {
+    if (scrollTop > maxScrollTop) {
+      scroll.update(state => ({
+        ...state,
+        top: maxScrollTop,
+      }))
+    }
+  }
+  $: {
+    if (scrollLeft > maxScrollLeft) {
+      scroll.update(state => ({
+        ...state,
+        left: maxScrollLeft,
+      }))
+    }
+  }
 
   const calculateContentWidth = (columns, stickyColumn) => {
     let width = 40 + stickyColumn?.width
diff --git a/packages/frontend-core/src/components/sheet/Sheet.svelte b/packages/frontend-core/src/components/sheet/Sheet.svelte
index c11d613276..2e5d2b68e7 100644
--- a/packages/frontend-core/src/components/sheet/Sheet.svelte
+++ b/packages/frontend-core/src/components/sheet/Sheet.svelte
@@ -16,7 +16,6 @@
   import { createAPIClient } from "../../api"
   import ScrollOverlay from "./ScrollOverlay.svelte"
   import StickyColumn from "./StickyColumn.svelte"
-  import SheetScrollWrapper from "./SheetScrollWrapper.svelte"
 
   export let tableId
   export let filter
@@ -49,7 +48,6 @@
     rand,
     selectedCellId,
     selectedRows,
-
     cellHeight,
     bounds,
     scroll,
diff --git a/packages/frontend-core/src/components/sheet/SheetCell.svelte b/packages/frontend-core/src/components/sheet/SheetCell.svelte
index 3c1fcf4e2b..9777850824 100644
--- a/packages/frontend-core/src/components/sheet/SheetCell.svelte
+++ b/packages/frontend-core/src/components/sheet/SheetCell.svelte
@@ -8,7 +8,6 @@
   export let selected = false
   export let reorderSource = false
   export let reorderTarget = false
-  export let left
   export let width
 </script>
 
@@ -25,7 +24,7 @@
   on:mouseenter
   on:click
   on:mousedown
-  style="--width:{width}px; --left:{left}px;"
+  style="--width:{width}px;"
 >
   <slot />
 </div>
@@ -49,9 +48,8 @@
     background: var(--cell-background);
     transition: border-color 130ms ease-out;
     flex: 0 0 var(--width);
-    position: absolute;
-    left: var(--left);
-    width: var(--width);
+    position: relative;
+    width: 0;
   }
   .cell.selected {
     box-shadow: inset 0 0 0 2px var(--spectrum-global-color-blue-400);
@@ -72,7 +70,6 @@
     background: var(--background);
     padding: 0 var(--cell-padding);
     border-color: var(--spectrum-global-color-gray-200);
-    font-weight: 600;
     gap: calc(2 * var(--cell-spacing));
     z-index: 10;
   }
@@ -86,10 +83,6 @@
   .cell.header:hover {
     cursor: pointer;
   }
-  .cell.header.sticky,
-  .cell.header.label {
-    z-index: 11;
-  }
 
   /* Reorder styles */
   .cell.reorder-source {
diff --git a/packages/frontend-core/src/components/sheet/SheetRow.svelte b/packages/frontend-core/src/components/sheet/SheetRow.svelte
index ff1940abcb..830ec9b900 100644
--- a/packages/frontend-core/src/components/sheet/SheetRow.svelte
+++ b/packages/frontend-core/src/components/sheet/SheetRow.svelte
@@ -48,9 +48,6 @@
 <style>
   .row {
     display: flex;
-    position: relative;
-    width: inherit;
-    height: var(--cell-height);
   }
   :global(.sheet:not(.is-resizing):not(.is-reordering) .row:hover .cell) {
     background: var(--cell-background-hover);
diff --git a/packages/frontend-core/src/components/sheet/SheetScrollWrapper.svelte b/packages/frontend-core/src/components/sheet/SheetScrollWrapper.svelte
index c6f5798246..5018135334 100644
--- a/packages/frontend-core/src/components/sheet/SheetScrollWrapper.svelte
+++ b/packages/frontend-core/src/components/sheet/SheetScrollWrapper.svelte
@@ -1,8 +1,15 @@
 <script>
   import { getContext } from "svelte"
 
-  const { cellHeight, scroll, bounds, rows, columns, visibleRows } =
-    getContext("spreadsheet")
+  const {
+    cellHeight,
+    scroll,
+    bounds,
+    rows,
+    columns,
+    visibleRows,
+    visibleColumns,
+  } = getContext("spreadsheet")
 
   export let scrollVertically = true
   export let scrollHorizontally = true
@@ -11,9 +18,10 @@
   $: scrollTop = $scroll.top
   $: scrollLeft = $scroll.left
   $: offsetY = scrollVertically ? -1 * (scrollTop % cellHeight) : 0
-  $: offsetX = scrollHorizontally ? -1 * scrollLeft : 0
+  $: hiddenWidths = calculateHiddenWidths($visibleColumns)
+  $: offsetX = scrollHorizontally ? -1 * scrollLeft + hiddenWidths : 0
   $: rowCount = $visibleRows.length
-  $: contentWidth = calculateContentWidth($columns, scrollHorizontally)
+  $: contentWidth = calculateContentWidth($visibleColumns, scrollHorizontally)
   $: contentHeight = calculateContentHeight(rowCount, scrollVertically)
   $: style = getStyle(offsetX, offsetY, contentWidth, contentHeight)
 
@@ -28,6 +36,17 @@
     return style
   }
 
+  const calculateHiddenWidths = visibleColumns => {
+    const idx = visibleColumns[0]?.idx
+    let width = 0
+    if (idx > 0) {
+      for (let i = 0; i < idx; i++) {
+        width += $columns[i].width
+      }
+    }
+    return width
+  }
+
   const calculateContentWidth = (columns, scroll) => {
     if (!scroll) {
       return null
@@ -77,6 +96,5 @@
     overflow: hidden;
     width: var(--width);
     height: var(--height);
-    position: relative;
   }
 </style>
diff --git a/packages/frontend-core/src/components/sheet/stores/rows.js b/packages/frontend-core/src/components/sheet/stores/rows.js
index 840798f1e2..0c4785ddba 100644
--- a/packages/frontend-core/src/components/sheet/stores/rows.js
+++ b/packages/frontend-core/src/components/sheet/stores/rows.js
@@ -4,7 +4,7 @@ import { fetchData } from "../../../fetch/fetchData"
 import { notifications } from "@budibase/bbui"
 
 export const createRowsStore = context => {
-  const { tableId, filter, API } = context
+  const { tableId, filter, API, scroll } = context
 
   // Flag for whether this is the first time loading our fetch
   let loaded = false
@@ -61,12 +61,17 @@ export const createRowsStore = context => {
           loaded = true
           rowCacheMap = {}
           rows.set([])
+
+          // Enrich primary display into schema
           let newSchema = $$fetch.schema
           const primaryDisplay = $$fetch.definition?.primaryDisplay
           if (primaryDisplay && newSchema[primaryDisplay]) {
             newSchema[primaryDisplay].primaryDisplay = true
           }
           schema.set(newSchema)
+
+          // Reset scroll state for fresh dataset
+          scroll.set({ left: 0, top: 0 })
         }
 
         // Process new rows
diff --git a/packages/frontend-core/src/components/sheet/stores/viewport.js b/packages/frontend-core/src/components/sheet/stores/viewport.js
index 56987006a7..ef95965ff7 100644
--- a/packages/frontend-core/src/components/sheet/stores/viewport.js
+++ b/packages/frontend-core/src/components/sheet/stores/viewport.js
@@ -1,64 +1,53 @@
-import { writable, derived, get } from "svelte/store"
+import { derived, get } from "svelte/store"
 
 export const createViewportStores = context => {
   const { cellHeight, columns, rows, scroll, bounds } = context
-
-  // Use local variables to avoid needing to invoke 2 svelte getters each time
-  // scroll state changes, but also use stores to allow use of derived stores
-  let scrollTop = 0
-  let scrollLeft = 0
-  const scrollTopStore = writable(0)
-  const scrollLeftStore = writable(0)
+  const scrollTop = derived(scroll, $scroll => $scroll.top, 0)
+  const scrollLeft = derived(scroll, $scroll => $scroll.left, 0)
 
   // Derive height and width as primitives to avoid wasted computation
   const width = derived(bounds, $bounds => $bounds.width)
   const height = derived(bounds, $bounds => $bounds.height)
 
-  // Debounce scroll updates so we can slow down visible row computation
-  scroll.subscribe(({ left, top }) => {
-    scrollTop = top
-    scrollTopStore.set(top)
-    scrollLeft = left
-    scrollLeftStore.set(left)
-  })
-
   // Derive visible rows
+  // Split into multiple stores containing primitives to optimise invalidation
+  // as mich as possible
+  const firstRowIdx = derived(scrollTop, $scrollTop => {
+    return Math.floor($scrollTop / cellHeight)
+  })
+  const visibleRowCount = derived(height, $height => {
+    return Math.ceil($height / cellHeight)
+  })
   const visibleRows = derived(
-    [rows, scrollTopStore, height],
-    ([$rows, $scrollTop, $height]) => {
-      const maxRows = Math.ceil($height / cellHeight) + 1
-      const firstRow = Math.max(0, Math.floor($scrollTop / cellHeight))
-      return $rows.slice(firstRow, firstRow + maxRows)
+    [rows, firstRowIdx, visibleRowCount],
+    ([$rows, $firstRowIdx, $visibleRowCount]) => {
+      return $rows.slice($firstRowIdx, $firstRowIdx + $visibleRowCount)
     }
   )
 
   // Derive visible columns
   const visibleColumns = derived(
-    [columns, scrollLeftStore, width],
+    [columns, scrollLeft, width],
     ([$columns, $scrollLeft, $width]) => {
       if (!$columns.length) {
         return []
       }
       let startColIdx = 0
       let rightEdge = $columns[0].width
-      while (rightEdge < $scrollLeft) {
+      while (rightEdge < $scrollLeft && startColIdx < $columns.length - 1) {
         startColIdx++
         rightEdge += $columns[startColIdx].width
       }
       let endColIdx = startColIdx + 1
       let leftEdge = rightEdge
-      while (leftEdge < $width + $scrollLeft) {
-        leftEdge += $columns[endColIdx]?.width
+      while (leftEdge < $width + $scrollLeft && endColIdx < $columns.length) {
+        leftEdge += $columns[endColIdx].width
         endColIdx++
       }
-      return $columns.slice(Math.max(0, startColIdx - 1), endColIdx + 1)
+      return $columns.slice(startColIdx, endColIdx)
     }
   )
 
-  // visibleColumns.subscribe(state => {
-  //   console.log(state)
-  // })
-
   // Fetch next page when approaching end of data
   visibleRows.subscribe($visibleRows => {
     const lastVisible = $visibleRows[$visibleRows.length - 1]