improvement(tables): react-doctor safe performance pass (#5329)

* improvement(tables): react-doctor safe performance pass Apply high-confidence, behavior-preserving perf fixes surfaced by react-doctor on the tables module: - Lazy-init the import-status ref so it stops allocating and discarding a fresh Map on every render (rerender-lazy-ref-init). - expandToDisplayColumns: build an outputs-by-column-name Map once per workflow group instead of Array.find() per column (js-index-maps). - Sort the tables list with toSorted() instead of [...list].sort() (js-tosorted-immutable; ES2023 lib is enabled for apps/sim). - Hoist the static TITLE_BY_MODE map and the pure editor wheel handler to module scope so they are not rebuilt each render. Left the effect/selection refactors (state-synced-to-prop, effect chains) untouched: those effects react to async-loaded rows and are not the copy-a-prop-into-state anti-pattern; refactoring them needs live grid verification. Barrel-import findings are false positives against the repo's mandated barrel-import convention. * improvement(tables): preserve find() first-match semantics + document perf idioms - expandToDisplayColumns: build the outputs-by-column-name index with an explicit first-write-wins loop so it is provably identical to the prior Array.find() on the (schema-precluded) duplicate-columnName case, not last-wins as new Map(entries) would be. - Add .claude/rules/sim-react-performance.md capturing the behavior- preserving render-perf idioms applied here (lazy object refs, module- scope hoisting, Map/Set pre-indexing, immutable array methods with the ES2022/ES2023 lib caveat, local-barrel false-positive note), and point to it from CLAUDE.md. * fix(tables): revert toSorted to spread-sort for universal browser support Array.prototype.toSorted is ES2023 runtime; SWC does not polyfill it and the default browserslist still includes browsers without it (iOS 15 Safari), so the tables page would crash there. [...result].sort() is non-mutating (sorts a copy, never touches the React Query cache array) and works everywhere; the perf delta is negligible. Correct the sim-react-performance rule doc to reflect that tsconfig lib only affects type-checking, not runtime availability. * refactor(tables): tidy lazy ref init and align perf-rule cross-reference - use-workspace-imports: lazy-init the status Map at its point of use in the effect ((ref.current ??= new Map())) instead of mutating the ref during render with a separate in-effect fallback. The ref is only read in the effect, so this removes the render-phase write and the dead '?? new Map()' branch while keeping the identical persistent Map. - CLAUDE.md: fix the render-performance pointer so it matches the rule doc — toSorted/toReversed are unsafe on client render paths (SWC does not polyfill prototype methods), not merely an ES2022/ES2023 lib note.
2026-09-21 13:00:04 +08:00 · 2026-07-01 13:11:07 -07:00
parent 8cebeb6774
commit 671535469d
6 changed files with 112 additions and 19 deletions
@@ -0,0 +1,80 @@
+# React & Render Performance
+
+Behavior-preserving performance idioms for components, hooks, and hot render paths. These are safe defaults — apply them freely. For the render-causing *effect/state* anti-patterns (derived state in effects, effect chains, state synced to a prop), use the dedicated skills: `/you-might-not-need-an-effect`, `/you-might-not-need-state`, `/you-might-not-need-a-memo`, `/you-might-not-need-a-callback`. Those refactors change render timing — verify them against the running UI, never mass-apply blind.
+
+## Lazy-init refs that hold objects
+
+`useRef(new Map())` / `useRef(new Set())` / `useRef({...})` allocates a fresh object on **every render** and throws it away — only the first is ever kept. Lazy-init instead so the allocation happens once.
+
+```typescript
+// ✗ Bad — allocates a new Map each render, discards all but the first
+const cacheRef = useRef<Map<string, string>>(new Map())
+
+// ✓ Good — allocated once, stable identity thereafter
+const cacheRef = useRef<Map<string, string> | null>(null)
+cacheRef.current ??= new Map()
+```
+
+Read `cacheRef.current` directly inside effects/handlers — refs are stable and never belong in a dependency array. A cheap primitive (`useRef(0)`, `useRef('')`, `useRef(null)`) needs no lazy init.
+
+## Hoist static values and closure-free functions to module scope
+
+A value or function declared inside a component is rebuilt every render. If it captures **nothing** from component scope (no props/state/refs), move it above the component at module scope. This skips the per-render allocation and keeps a stable identity so memoized children don't re-render.
+
+```typescript
+// ✗ Bad — rebuilt every render, new identity each time
+function Toolbar({ mode }: ToolbarProps) {
+  const TITLES = { create: 'Add', edit: 'Configure' } as const
+  const handleWheel = (e: React.WheelEvent) => e.currentTarget.scrollBy(e.deltaX, e.deltaY)
+  // ...
+}
+
+// ✓ Good — allocated once at module load
+const TITLES = { create: 'Add', edit: 'Configure' } as const
+function handleWheel(e: React.WheelEvent) {
+  e.currentTarget.scrollBy(e.deltaX, e.deltaY)
+}
+function Toolbar({ mode }: ToolbarProps) { /* ... */ }
+```
+
+A closure-free function that IS wired through a ref sink or intentionally kept for stable identity may stay inline — hoisting a one-line `preventDefault` handler is churn, not a win. Hoist when it removes a real per-render allocation or unblocks child memoization.
+
+## Pre-index with Map/Set for repeated lookups
+
+`array.find()` / `array.includes()` / `array.indexOf()` scan the whole list each call. Inside a loop or a hot render path over a non-trivial list, that is O(n·m). Build a `Map` (for lookup-by-key) or `Set` (for membership) **once before** the loop, then look up in O(1).
+
+```typescript
+// ✗ Bad — find() re-scans outputs for every column
+for (const child of columns) {
+  const output = group.outputs.find((o) => o.columnName === getColumnId(child))
+}
+
+// ✓ Good — index once, then O(1) lookups
+const outputByName = new Map<string, Output>()
+for (const o of group.outputs) {
+  if (!outputByName.has(o.columnName)) outputByName.set(o.columnName, o) // first wins, matches find()
+}
+for (const child of columns) {
+  const output = outputByName.get(getColumnId(child))
+}
+```
+
+Preserve `.find()`'s **first-match** semantics when duplicate keys are possible: `new Map(arr.map(...))` keeps the *last* entry, so guard with `if (!map.has(key))` when replacing a `.find()`. Skip this for tiny, cold arrays (a handful of items in an event handler) where the Map build costs more than it saves.
+
+## Never mutate a shared array in place
+
+The real bug to avoid is `array.sort()` / `array.reverse()` on an array you don't own — sorting a React Query cache array in place corrupts shared state. Always sort a copy:
+
+```typescript
+// ✗ Bad — mutates the (possibly shared) source array in place
+return items.sort(compare)
+
+// ✓ Good — sorts a throwaway copy, source untouched
+return [...items].sort(compare)
+```
+
+**Do NOT reach for `toSorted()` / `toReversed()` / `with()` / `toSpliced()` on client render paths.** They are ES2023 *runtime* methods — and a tsconfig `"lib": ["ES2023"]` only makes them **type-check**, it does not make them **run**. Next/SWC compiles syntax but does **not** polyfill prototype methods, and the default browserslist still includes browsers without them (`toSorted` landed in Safari 16 / iOS 16, so any device capped at iOS 15 throws `TypeError: x.toSorted is not a function` and crashes the page). The perf difference vs `[...arr].sort()` is negligible (both allocate one array), so the copy-then-sort form is the correct default everywhere client code runs. Only consider the immutable methods in Node-only code (server routes, scripts) on Node ≥20, where the runtime is known.
+
+## Local feature barrels are the convention — do not "fix" them
+
+Tooling (e.g. react-doctor's `no-barrel-import`) will flag imports from local `index.ts` barrels as a bundle cost. In this repo that is a **false positive**: barrel imports for 3+ export folders are mandated by `.claude/rules/sim-imports.md`. Leave them.
@@ -127,6 +127,8 @@ export function Component({ requiredProp, optionalProp = false }: ComponentProps

 Extract when: 50+ lines, used in 2+ files, or has own state/logic. Keep inline when: < 10 lines, single use, purely presentational.

+Behavior-preserving render-performance idioms — lazy-init object refs, hoist closure-free values/functions to module scope, pre-index repeated lookups with `Map`/`Set`, and never mutating a shared array in place — are in `.claude/rules/sim-react-performance.md` (which also explains why `toSorted`/`toReversed` are unsafe on client render paths despite the ES2023 tsconfig lib — SWC does not polyfill prototype methods, so use `[...arr].sort()`). For the render-timing effect/state anti-patterns use the `/you-might-not-need-*` skills and verify against the running UI.
+
 ## API Contracts

 Boundary HTTP request and response shapes for all routes under `apps/sim/app/api/**` live in `apps/sim/lib/api/contracts/**` (one file per resource family — `folders.ts`, `chats.ts`, `knowledge.ts`, etc.). Routes never define route-local boundary Zod schemas, and clients never define ad-hoc wire types — both sides consume the same contract.
@@ -19,6 +19,15 @@ interface InlineEditorProps {
  onCancel: () => void
 }

+/** Redirect wheel gestures over an inline editor to the surrounding table scroll container. */
+function handleEditorWheel(e: React.WheelEvent<HTMLInputElement>) {
+  e.preventDefault()
+  const container = e.currentTarget.closest('[data-table-scroll]') as HTMLElement | null
+  if (container) {
+    container.scrollBy(e.deltaX, e.deltaY)
+  }
+}
+
 /** Inline editor for `date` columns — text input + popover calendar. */
 function InlineDateEditor({
  value,
@@ -152,14 +161,6 @@ function InlineTextEditor({
    }
  }, [])

-  const handleWheel = (e: React.WheelEvent<HTMLInputElement>) => {
-    e.preventDefault()
-    const container = e.currentTarget.closest('[data-table-scroll]') as HTMLElement | null
-    if (container) {
-      container.scrollBy(e.deltaX, e.deltaY)
-    }
-  }
-
  const doSave = (reason: SaveReason) => {
    if (doneRef.current) return
    doneRef.current = true
@@ -194,7 +195,7 @@ function InlineTextEditor({
      value={draft ?? ''}
      onChange={(e) => setDraft(e.target.value)}
      onKeyDown={handleKeyDown}
-      onWheel={handleWheel}
+      onWheel={handleEditorWheel}
      onBlur={() => doSave('blur')}
      className='w-full min-w-0 select-text border-none bg-transparent p-0 text-[var(--text-primary)] text-small outline-none'
    />
@@ -116,10 +116,18 @@ export function expandToDisplayColumns(
        size++
      }
      const group = groupById.get(gid)
+      // Pre-index outputs by column name for O(1) lookup. First output wins on a
+      // duplicate columnName, exactly matching the previous `Array.find()` behavior.
+      const outputByColumnName = new Map<string, WorkflowGroup['outputs'][number]>()
+      if (group) {
+        for (const o of group.outputs) {
+          if (!outputByColumnName.has(o.columnName)) outputByColumnName.set(o.columnName, o)
+        }
+      }
      const startIdx = out.length
      for (let k = 0; k < size; k++) {
        const child = columns[i + k]
-        const output = group?.outputs.find((o) => o.columnName === getColumnId(child))
+        const output = outputByColumnName.get(getColumnId(child))
        out.push({
          ...child,
          key: getColumnId(child),
@@ -121,6 +121,12 @@ interface WorkflowSidebarProps {

 const OUTPUT_VALUE_SEPARATOR = '::'

+const TITLE_BY_MODE = {
+  create: 'Add workflow',
+  'edit-group': 'Configure workflow',
+  'edit-output': 'Configure output column',
+} as const
+
 const encodeOutputValue = (blockId: string, path: string) =>
  `${blockId}${OUTPUT_VALUE_SEPARATOR}${path}`

@@ -772,15 +778,10 @@ export function WorkflowSidebarBody({
    updateWorkflowGroup.isPending ||
    updateColumn.isPending ||
    !depsValid
-  const titleByMode = {
-    create: 'Add workflow',
-    'edit-group': 'Configure workflow',
-    'edit-output': 'Configure output column',
-  } as const
  const title =
    config.mode === 'create' && config.kind === 'enrichment' && config.enrichmentName
      ? config.enrichmentName
-      : titleByMode[config.mode]
+      : TITLE_BY_MODE[config.mode]
  const showBackButton = isEnrichment && Boolean(onBack)

  // edit-output mode is single-select on the output picker; everywhere else
@@ -51,12 +51,13 @@ export function useWorkspaceImports(
  // jobs), so the tray reads them from their dedicated workspace listing.
  const { data: exportJobs } = useWorkspaceExportJobs(workspaceId)

-  const prevStatus = useRef<Map<string, string>>(new Map())
+  const prevStatusRef = useRef<Map<string, string> | null>(null)
  useEffect(() => {
    if (!tables) return
+    const prevStatus = (prevStatusRef.current ??= new Map())
    const store = useImportTrayStore.getState()
    for (const table of tables) {
-      const before = prevStatus.current.get(table.id)
+      const before = prevStatus.get(table.id)
      const now = table.jobStatus ?? 'none'
      if (before === 'running' && now === 'ready') {
        // Success toast only for imports — deletes reflect instantly in the grid and backfills
@@ -80,7 +81,7 @@ export function useWorkspaceImports(
        if (table.jobType === 'import') store.notify(table.id)
      }
      if (now !== 'running' && store.isCanceled(table.id)) store.consumeCanceled(table.id)
-      prevStatus.current.set(table.id, now)
+      prevStatus.set(table.id, now)
    }
  }, [tables])