Merge remote-tracking branch 'origin/staging' into realtime-rooms

# Conflicts:
#	apps/sim/app/workspace/[workspaceId]/tables/[tableId]/table.tsx

Author: waleedlatif1

apps/sim/app/api/v1/admin/workspaces/[id]/import/route.ts

@@ -41,8 +41,10 @@ import {
   extractWorkflowsFromZip,
   parseWorkflowJson,
 } from '@/lib/workflows/operations/import-export'
+import { prepareWorkflowStateForPersistence } from '@/lib/workflows/persistence/prepare-state'
 import { saveWorkflowToNormalizedTables } from '@/lib/workflows/persistence/utils'
 import { deduplicateWorkflowName } from '@/lib/workflows/utils'
+import { normalizeImportedVariables } from '@/lib/workflows/variables/parse'
 import { getWorkspaceWithOwner } from '@/lib/workspaces/permissions/utils'
 import { withAdminAuthParams } from '@/app/api/v1/admin/middleware'
 import {
@@ -52,7 +54,6 @@ import {
 } from '@/app/api/v1/admin/responses'
 import type {
   ImportResult,
-  WorkflowVariable,
   WorkspaceImportRequest,
   WorkspaceImportResponse,
 } from '@/app/api/v1/admin/types'
@@ -270,7 +271,22 @@ async function importSingleWorkflow(
       variables: {},
     })
 
-    const saveResult = await saveWorkflowToNormalizedTables(workflowId, workflowData)
+    /**
+     * Same normalization the editor, the v1 import API and the single-workflow
+     * admin import run, via the one shared implementation. Without it this route
+     * wrote raw parsed state, so a dangling edge tripped the `workflow_edges`
+     * foreign key and failed the restore, and blocks missing their backfilled
+     * columns could land unopenable.
+     */
+    const { state: preparedState, warnings } = prepareWorkflowStateForPersistence(workflowData)
+    if (warnings.length > 0) {
+      logger.warn(`Admin API: normalized "${dedupedName}" with warnings`, { warnings })
+    }
+
+    const saveResult = await saveWorkflowToNormalizedTables(workflowId, {
+      ...workflowData,
+      ...preparedState,
+    })
 
     if (!saveResult.success) {
       await db.delete(workflow).where(eq(workflow.id, workflowId))
@@ -282,18 +298,13 @@ async function importSingleWorkflow(
       }
     }
 
-    if (workflowData.variables && Array.isArray(workflowData.variables)) {
-      const variablesRecord: Record<string, WorkflowVariable> = {}
-      workflowData.variables.forEach((v) => {
-        const varId = v.id || generateId()
-        variablesRecord[varId] = {
-          id: varId,
-          name: v.name,
-          type: v.type || 'string',
-          value: v.value,
-        }
-      })
-
+    /**
+     * Previously guarded on `Array.isArray`, which silently dropped every
+     * variable in the current record form — the exact shape this workspace's
+     * own export emits — so an export/import round trip lost all of them.
+     */
+    const variablesRecord = normalizeImportedVariables(workflowData.variables)
+    if (Object.keys(variablesRecord).length > 0) {
       await db
         .update(workflow)
         .set({ variables: variablesRecord, updatedAt: new Date() })

apps/sim/app/workspace/[workspaceId]/tables/[tableId]/lock-copy.ts

@@ -1,6 +1,6 @@
 /**
- * Single source of truth for lock vocabulary shared by the lock settings modal,
- * the blocked-action toast, and the table header chip. Kept out of
+ * Single source of truth for lock vocabulary shared by the lock settings modal
+ * and the lock toasts (the on-open announcement and blocked actions). Kept out of
  * `lib/table/mutation-locks.ts` — that module is server-tainted (importing it
  * from a client component pulls `next/headers` into the browser bundle).
  */
@@ -76,8 +76,8 @@ export function describeLocks(locks: TableLocks): { name: string; detail: string
 
 /**
  * Why a locked-table notice was raised. `'status'` is the informational case
- * (a non-admin clicking the header lock chip); the rest are actions the user
- * just tried and couldn't do.
+ * (the announcement shown once when a locked table is opened); the rest are
+ * actions the user just tried and couldn't do.
  */
 export type BlockedTableAction = 'add-row' | 'add-column' | 'delete-column' | 'edit-cell' | 'status'
 

apps/sim/app/workspace/[workspaceId]/tables/[tableId]/table.tsx

@@ -1,6 +1,6 @@
 'use client'
 
-import { useCallback, useMemo, useReducer, useRef, useState } from 'react'
+import { useCallback, useEffect, useMemo, useReducer, useRef, useState } from 'react'
 import { Chip, ChipConfirmModal, toast } from '@sim/emcn'
 import { Download, Lock, Pencil, Table as TableIcon, Trash, Upload } from '@sim/emcn/icons'
 import { createLogger } from '@sim/logger'
@@ -62,12 +62,7 @@ import {
 import { COLUMN_SIDEBAR_WIDTH } from './components/table-grid/constants'
 import { COLUMN_TYPE_ICONS } from './components/table-grid/headers'
 import { useTable, useTableEventStream, useTableRoom } from './hooks'
-import {
-  type BlockedTableAction,
-  describeBlockedAction,
-  describeLocks,
-  lockedNouns,
-} from './lock-copy'
+import { type BlockedTableAction, describeBlockedAction, lockedNouns } from './lock-copy'
 import {
   DEFAULT_TABLE_DETAIL_SORT_DIRECTION,
   tableDetailParsers,
@@ -636,7 +631,10 @@ export function Table({
       if (!tableData) return
       if (blockedToastIdRef.current) toast.dismiss(blockedToastIdRef.current)
       const { title, text } = describeBlockedAction(action, tableData.locks)
-      blockedToastIdRef.current = toast.warning(title, {
+      // 'status' is the on-open announcement — nothing was refused, so it reads
+      // as information rather than a warning.
+      const notify = action === 'status' ? toast.info : toast.warning
+      blockedToastIdRef.current = notify(title, {
         description: text,
         ...(canOpenLockSettings
           ? {
@@ -650,23 +648,48 @@ export function Table({
     [tableData, canOpenLockSettings]
   )
 
+  // Announce the lock state once per table on open. Unlike the re-rendering
+  // permission gates, this fires once and can't self-correct, so it waits for
+  // `canAdmin` to settle instead of treating loading as permitted.
+  const announcedLockTableIdRef = useRef<string | null>(null)
+  useEffect(() => {
+    if (!tableData || userPermissions.isLoading) return
+    if (announcedLockTableIdRef.current === tableData.id) return
+    announcedLockTableIdRef.current = tableData.id
+    if (lockedNouns(tableData.locks).length === 0) return
+    showBlockedToast('status')
+  }, [tableData, userPermissions.isLoading, showBlockedToast])
+
+  // A notice must not outlive the table it describes — its action targets
+  // whichever table is current. Keyed on `tableId` so an embedded swap that
+  // changes the prop without a route change is covered too. Leaving ends the
+  // visit, so the latch resets and coming back announces again.
+  useEffect(
+    () => () => {
+      announcedLockTableIdRef.current = null
+      if (!blockedToastIdRef.current) return
+      toast.dismiss(blockedToastIdRef.current)
+      blockedToastIdRef.current = null
+    },
+    [tableId]
+  )
+
+  // A toast's action is captured when it is created, so a viewer who loses
+  // admin access mid-toast would keep a Lock settings button that opens
+  // nothing. Dismiss on that transition only — a viewer who never had access
+  // has a legitimate action-less notice that must survive.
+  const couldOpenLockSettingsRef = useRef(canOpenLockSettings)
+  useEffect(() => {
+    const lostAccess = couldOpenLockSettingsRef.current && !canOpenLockSettings
+    couldOpenLockSettingsRef.current = canOpenLockSettings
+    if (!lostAccess || !blockedToastIdRef.current) return
+    toast.dismiss(blockedToastIdRef.current)
+    blockedToastIdRef.current = null
+  }, [canOpenLockSettings])
+
   const headerActions = useMemo(() => {
     if (!tableData) return undefined
-    // Header space is for state, not for settings: the chip appears only once
-    // something is actually locked, and names the mode so it reads at a glance.
-    // Reaching the panel on an unlocked table is the dropdown's job.
-    const anyLocked = lockedNouns(tableData.locks).length > 0
     return [
-      ...(anyLocked
-        ? [
-            {
-              label: describeLocks(tableData.locks).name,
-              icon: Lock,
-              onClick: () =>
-                userPermissions.canAdmin ? setShowLockSettings(true) : showBlockedToast('status'),
-            },
-          ]
-        : []),
       {
         label: 'Import CSV',
         icon: Upload,
@@ -682,14 +705,7 @@ export function Table({
         disabled: tableData.rowCount === 0,
       },
     ]
-  }, [
-    tableData,
-    userPermissions.canEdit,
-    userPermissions.canAdmin,
-    handleExportCsv,
-    onRequestImportCsv,
-    showBlockedToast,
-  ])
+  }, [tableData, userPermissions.canEdit, handleExportCsv, onRequestImportCsv])
 
   // Adding a column is a schema change. The trigger stays visible when the
   // table is schema-locked and explains itself instead of disappearing.

apps/sim/lib/workflows/operations/import-export.ts

@@ -1,6 +1,5 @@
 import { createLogger } from '@sim/logger'
 import { getErrorMessage } from '@sim/utils/errors'
-import { generateId } from '@sim/utils/id'
 import { isRecordLike } from '@sim/utils/object'
 import { ApiClientError } from '@/lib/api/client/errors'
 import { requestJson } from '@/lib/api/client/request'
@@ -18,6 +17,7 @@ import {
   sanitizeForExport,
 } from '@/lib/workflows/sanitization/json-sanitizer'
 import { sanitizeMalformedSubBlocks } from '@/lib/workflows/sanitization/subblocks'
+import { normalizeImportedVariables } from '@/lib/workflows/variables/parse'
 import { regenerateWorkflowIds } from '@/stores/workflows/utils'
 import type { Variable, WorkflowState } from '@/stores/workflows/workflow/types'
 
@@ -724,37 +724,15 @@ export async function persistImportedWorkflow({
     throw new Error(`Failed to save workflow state for ${newWorkflowId}`)
   }
 
-  if (workflowData.variables) {
-    const variablesArray = Array.isArray(workflowData.variables)
-      ? workflowData.variables
-      : Object.values(workflowData.variables)
-
-    if (variablesArray.length > 0) {
-      type WorkflowVariablesBodyInput = NonNullable<
-        Parameters<typeof requestJson<typeof workflowVariablesContract>>[1]['body']
-      >
-      const variablesRecord: WorkflowVariablesBodyInput['variables'] = {}
-
-      for (const variable of variablesArray) {
-        const id =
-          typeof variable.id === 'string' && variable.id.trim() ? variable.id : generateId()
-
-        variablesRecord[id] = {
-          id,
-          name: variable.name,
-          type: variable.type,
-          value: variable.value,
-        }
-      }
-
-      try {
-        await requestJson(workflowVariablesContract, {
-          params: { id: newWorkflowId },
-          body: { variables: variablesRecord },
-        })
-      } catch {
-        throw new Error(`Failed to save variables for ${newWorkflowId}`)
-      }
+  const variablesRecord = normalizeImportedVariables(workflowData.variables)
+  if (Object.keys(variablesRecord).length > 0) {
+    try {
+      await requestJson(workflowVariablesContract, {
+        params: { id: newWorkflowId },
+        body: { variables: variablesRecord },
+      })
+    } catch {
+      throw new Error(`Failed to save variables for ${newWorkflowId}`)
     }
   }
 

apps/sim/lib/workflows/variables/parse.test.ts

@@ -0,0 +1,119 @@
+/**
+ * @vitest-environment node
+ *
+ * Pins the contract between the two halves of every workflow export/import
+ * round trip: `parseWorkflowVariables` produces what exports emit, and
+ * `normalizeImportedVariables` consumes what imports receive. A shape the first
+ * emits that the second drops is silent data loss on a restore, which is
+ * exactly the bug the workspace importer shipped with.
+ */
+
+import { describe, expect, it } from 'vitest'
+import { normalizeImportedVariables, parseWorkflowVariables } from '@/lib/workflows/variables/parse'
+
+const VARIABLE = {
+  id: 'var-1',
+  name: 'apiHost',
+  type: 'string' as const,
+  value: 'https://example.com',
+}
+
+describe('parseWorkflowVariables', () => {
+  it('returns undefined for null so callers can omit the field', () => {
+    expect(parseWorkflowVariables(null)).toBeUndefined()
+  })
+
+  it('reads the current record form', () => {
+    expect(parseWorkflowVariables({ 'var-1': VARIABLE })).toEqual({ 'var-1': VARIABLE })
+  })
+
+  it('re-keys the legacy array form by variable id', () => {
+    expect(parseWorkflowVariables([VARIABLE] as never)).toEqual({ 'var-1': VARIABLE })
+  })
+
+  it('parses a JSON string column value', () => {
+    expect(parseWorkflowVariables(JSON.stringify({ 'var-1': VARIABLE }) as never)).toEqual({
+      'var-1': VARIABLE,
+    })
+  })
+
+  it('skips legacy array rows without a usable id instead of emitting an "undefined" key', () => {
+    const result = parseWorkflowVariables([VARIABLE, { name: 'orphan' }, null] as never)
+
+    expect(Object.keys(result ?? {})).toEqual(['var-1'])
+  })
+})
+
+describe('normalizeImportedVariables', () => {
+  it('accepts the record form', () => {
+    expect(normalizeImportedVariables({ 'var-1': VARIABLE })).toEqual({ 'var-1': VARIABLE })
+  })
+
+  it('accepts the legacy array form', () => {
+    expect(normalizeImportedVariables([VARIABLE])).toEqual({ 'var-1': VARIABLE })
+  })
+
+  it('returns an empty record for null, undefined and non-objects', () => {
+    expect(normalizeImportedVariables(null)).toEqual({})
+    expect(normalizeImportedVariables(undefined)).toEqual({})
+    expect(normalizeImportedVariables('nope')).toEqual({})
+  })
+
+  it('falls back to the map key when an entry carries no id', () => {
+    expect(normalizeImportedVariables({ fromKey: { name: 'x', value: 1 } })).toMatchObject({
+      fromKey: { id: 'fromKey', name: 'x' },
+    })
+  })
+
+  it('coerces an unrecognized type to string rather than persisting it', () => {
+    const result = normalizeImportedVariables({ v: { ...VARIABLE, type: 'secret' } })
+
+    expect(result['var-1'].type).toBe('string')
+  })
+
+  it('keeps a __proto__-keyed variable as an ordinary own property', () => {
+    /**
+     * Built via `JSON.parse` rather than a literal: an object literal's
+     * `__proto__` sets the prototype, while `JSON.parse` creates a real own
+     * key — and `JSON.parse` is how an import payload actually arrives.
+     */
+    const payload = JSON.parse('{"__proto__": {"name": "sneaky", "value": 1}}')
+
+    const result = normalizeImportedVariables(payload)
+
+    expect(Object.keys(result)).toContain('__proto__')
+    expect(Object.getPrototypeOf(result)).toBe(Object.prototype)
+  })
+
+  it('trims a padded id so the key is referenceable', () => {
+    expect(Object.keys(normalizeImportedVariables([{ ...VARIABLE, id: '  var-1  ' }]))).toEqual([
+      'var-1',
+    ])
+  })
+
+  it('skips non-object entries instead of writing junk variables', () => {
+    expect(normalizeImportedVariables(['nope', null, VARIABLE])).toEqual({ 'var-1': VARIABLE })
+  })
+})
+
+describe('export -> import variable round trip', () => {
+  /**
+   * The regression guard. Workspace export writes whatever
+   * `parseWorkflowVariables` returns — the record form — and the importer used
+   * to guard on `Array.isArray`, so every variable was silently dropped on
+   * restore.
+   */
+  it('survives the record form that exports actually emit', () => {
+    const exported = parseWorkflowVariables({ 'var-1': VARIABLE })
+
+    expect(exported).toBeDefined()
+    expect(Array.isArray(exported)).toBe(false)
+    expect(normalizeImportedVariables(exported)).toEqual({ 'var-1': VARIABLE })
+  })
+
+  it('survives a legacy array-form export', () => {
+    const exported = parseWorkflowVariables([VARIABLE] as never)
+
+    expect(normalizeImportedVariables(exported)).toEqual({ 'var-1': VARIABLE })
+  })
+})