Skip to content

Commit 429d0e2

Browse files
fix(tables): don't strand layout writes while views load, keep schema undos across views
Two more instances of layout being written without a known owner. The sink was left unbound until a view resolved, so a resize/reorder/pin (or the column-append effect) during the views fetch fell through to the table's shared metadata — corrupting All for a table about to adopt a view, and losing the edit to the re-seed. The sink is now bound while the query is in flight and suppresses the write. An error counts as settled, so a failed views fetch falls back to All instead of suppressing layout writes for the session. Pruning was also too broad: create-column and delete-column are table-scoped schema ops that merely have a layout side-effect, so dropping them on a view switch made a deleted column unrecoverable. Only reorder-columns is purely layout and still prunes; the other two survive and have just their layout half suppressed at replay when the recorded view isn't active.
1 parent d8c279b commit 429d0e2

5 files changed

Lines changed: 76 additions & 23 deletions

File tree

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

Lines changed: 23 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -358,7 +358,11 @@ export function Table({
358358
((previousName: string, newName: string) => void) | null
359359
>(null)
360360

361-
const { data: views = NO_VIEWS, isSuccess: viewsLoaded } = useTableViews({
361+
const {
362+
data: views = NO_VIEWS,
363+
isSuccess: viewsLoaded,
364+
isError: viewsFailed,
365+
} = useTableViews({
362366
workspaceId,
363367
tableId,
364368
enabled: viewsEnabled,
@@ -389,6 +393,12 @@ export function Table({
389393
* rendering an empty view. */
390394
const activeView = activeViewId ? (views.find((view) => view.id === activeViewId) ?? null) : null
391395

396+
/** The views query hasn't settled, so which view owns the layout isn't known
397+
* yet and layout writes must go nowhere. An ERROR counts as settled: the table
398+
* falls back to All and writes resume against shared metadata, rather than
399+
* staying silently suppressed for the rest of the session. */
400+
const viewOwnerUnknown = viewsEnabled && !viewsLoaded && !viewsFailed
401+
392402
const [viewModal, setViewModal] = useState<ViewModalState>(null)
393403
/** Which view id the local filter/sort/hidden state was last seeded from.
394404
* `undefined` means "nothing seeded yet" so the first resolve still runs. */
@@ -643,8 +653,10 @@ export function Table({
643653
/** Column order/width/pinning auto-saves into the active view as the user drags,
644654
* which is why `isSameViewConfig` excludes layout from the dirty check. Sent as
645655
* a `configPatch` so the server merges it — two overlapping layout writes must
646-
* not each replace the whole blob from their own snapshot. With no view active
647-
* the grid keeps writing the table's shared metadata. */
656+
* not each replace the whole blob from their own snapshot. With All selected
657+
* the sink is unbound and the grid writes the table's shared metadata instead;
658+
* while the views query is still loading the sink IS bound and the write is
659+
* suppressed, because the owner isn't known yet. */
648660
const handlePersistLayout = useCallback(
649661
(patch: TableMetadata) => {
650662
liveLayoutRef.current = { ...liveLayoutRef.current, ...patch }
@@ -1340,7 +1352,14 @@ export function Table({
13401352
hiddenColumns={effectiveHiddenColumns}
13411353
viewLayout={activeView?.config ?? null}
13421354
viewLayoutKey={activeView?.id ?? null}
1343-
onPersistLayout={activeView ? handlePersistLayout : undefined}
1355+
onPersistLayout={
1356+
// While the views query is in flight the layout owner is unknown, so
1357+
// the sink is bound anyway: `handlePersistLayout` buffers into
1358+
// `liveLayoutRef` and suppresses the write. Leaving it unset would fall
1359+
// through to the table's shared metadata and corrupt All's layout for a
1360+
// table that is about to adopt a view.
1361+
viewOwnerUnknown || activeView ? handlePersistLayout : undefined
1362+
}
13441363
columnRenameSinkRef={columnRenameSinkRef}
13451364
afterDeleteRowsSinkRef={afterDeleteRowsSinkRef}
13461365
afterDeleteAllSinkRef={afterDeleteAllSinkRef}

apps/sim/hooks/use-table-undo.ts

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -189,7 +189,12 @@ export function useTableUndo({
189189
)
190190

191191
const executeAction = useCallback(
192-
async (action: TableUndoAction, direction: 'undo' | 'redo') => {
192+
async (action: TableUndoAction, direction: 'undo' | 'redo', entryViewId: string | null) => {
193+
// Column create/delete are table-scoped, so they stay undoable after a view
194+
// switch — but the layout they recorded belongs to the view that was active
195+
// at the time. Replaying it elsewhere would write one view's order/widths
196+
// into another, so the schema half runs and the layout half is dropped.
197+
const entryOwnsLayout = entryViewId === activeViewIdRef.current
193198
try {
194199
switch (action.type) {
195200
case 'update-cell': {
@@ -338,7 +343,7 @@ export function useTableUndo({
338343
metadata.pinnedColumns = newPinned
339344
}
340345
if (Object.keys(metadata).length > 0) {
341-
persistLayoutRef.current(metadata)
346+
if (entryOwnsLayout) persistLayoutRef.current(metadata)
342347
}
343348
},
344349
})
@@ -430,7 +435,7 @@ export function useTableUndo({
430435
}
431436
}
432437
if (Object.keys(metadata).length > 0) {
433-
persistLayoutRef.current(metadata)
438+
if (entryOwnsLayout) persistLayoutRef.current(metadata)
434439
}
435440
},
436441
}
@@ -459,7 +464,7 @@ export function useTableUndo({
459464
}
460465
}
461466
if (Object.keys(metadata).length > 0) {
462-
persistLayoutRef.current(metadata)
467+
if (entryOwnsLayout) persistLayoutRef.current(metadata)
463468
}
464469
},
465470
})
@@ -521,6 +526,10 @@ export function useTableUndo({
521526
...restored.filter((n) => !pinnedSet.has(n)),
522527
]
523528
}
529+
// Pruning already drops these on a view switch, so a mismatch here
530+
// should be unreachable; the guard keeps the invariant local rather
531+
// than dependent on when the prune effect happens to run.
532+
if (!entryOwnsLayout) break
524533
onColumnOrderChangeRef.current?.(order)
525534
persistLayoutRef.current({ columnOrder: order })
526535
break
@@ -546,7 +555,7 @@ export function useTableUndo({
546555
}
547556
const entry = popUndo(tableId)
548557
if (!entry) return
549-
void runWithoutRecording(() => executeAction(entry.action, 'undo'))
558+
void runWithoutRecording(() => executeAction(entry.action, 'undo', entry.viewId))
550559
}, [popUndo, tableId, executeAction])
551560

552561
const redo = useCallback(() => {
@@ -560,7 +569,7 @@ export function useTableUndo({
560569
}
561570
const entry = popRedo(tableId)
562571
if (!entry) return
563-
void runWithoutRecording(() => executeAction(entry.action, 'redo'))
572+
void runWithoutRecording(() => executeAction(entry.action, 'redo', entry.viewId))
564573
}, [popRedo, tableId, executeAction])
565574

566575
return { pushUndo, undo, redo, canUndo, canRedo }

apps/sim/stores/table/store.test.ts

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,18 @@ const reorder: TableUndoAction = {
1212
previousOrder: ['a', 'b'],
1313
newOrder: ['b', 'a'],
1414
}
15+
const deleteColumn: TableUndoAction = {
16+
type: 'delete-column',
17+
columnName: 'a',
18+
columnType: 'string',
19+
columnPosition: 0,
20+
columnUnique: false,
21+
columnRequired: false,
22+
cellData: [],
23+
previousOrder: ['a', 'b'],
24+
previousWidth: null,
25+
previousPinnedColumns: null,
26+
}
1527
const updateCell: TableUndoAction = {
1628
type: 'update-cell',
1729
rowId: 'r1',
@@ -65,6 +77,19 @@ describe('pruneLayoutActions', () => {
6577
expect(useTableUndoStore.getState().stacks[TABLE]?.redo).toHaveLength(0)
6678
})
6779

80+
it('keeps column create/delete across a view switch — they are schema ops', () => {
81+
const store = useTableUndoStore.getState()
82+
store.push(TABLE, deleteColumn, 'view-a')
83+
84+
store.pruneLayoutActions(TABLE, 'view-b')
85+
86+
const undo = useTableUndoStore.getState().stacks[TABLE]?.undo
87+
expect(undo).toHaveLength(1)
88+
expect(undo?.[0].action.type).toBe('delete-column')
89+
// Its recorded owner survives so the replay can drop just the layout half.
90+
expect(undo?.[0].viewId).toBe('view-a')
91+
})
92+
6893
it('treats All (null) as its own owner', () => {
6994
const store = useTableUndoStore.getState()
7095
store.push(TABLE, reorder, null)

apps/sim/stores/table/store.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import { generateShortId } from '@sim/utils/id'
77
import { create } from 'zustand'
88
import { devtools } from 'zustand/middleware'
99
import type { TableUndoAction, TableUndoStacks, TableUndoState, UndoEntry } from './types'
10-
import { LAYOUT_UNDO_ACTIONS } from './types'
10+
import { VIEW_SCOPED_UNDO_ACTIONS } from './types'
1111

1212
const STACK_CAPACITY = 100
1313
const EMPTY_STACKS: TableUndoStacks = { undo: [], redo: [] }
@@ -192,7 +192,7 @@ export const useTableUndoStore = create<TableUndoState>()(
192192
const current = get().stacks[tableId]
193193
if (!current) return
194194
const owned = (entry: UndoEntry) =>
195-
!LAYOUT_UNDO_ACTIONS.has(entry.action.type) || entry.viewId === viewId
195+
!VIEW_SCOPED_UNDO_ACTIONS.has(entry.action.type) || entry.viewId === viewId
196196
const undo = current.undo.filter(owned)
197197
const redo = current.redo.filter(owned)
198198
if (undo.length === current.undo.length && redo.length === current.redo.length) return

apps/sim/stores/table/types.ts

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -90,21 +90,21 @@ export interface UndoEntry {
9090
/**
9191
* Active view when the action was recorded — `null` for "All" or when views
9292
* are disabled. Layout is view-owned, so a layout action is only meaningful
93-
* against the view that owned it; see {@link LAYOUT_UNDO_ACTIONS}.
93+
* against the view that owned it; see {@link VIEW_SCOPED_UNDO_ACTIONS}.
9494
*/
9595
viewId: string | null
9696
}
9797

9898
/**
99-
* Action types whose undo writes column layout (order, widths, pinning). These
100-
* are scoped to the view that was active when they were recorded; every other
101-
* action type operates on rows or the schema and is table-scoped.
99+
* Action types that do NOTHING but rearrange columns, so they mean nothing
100+
* outside the view that recorded them and are dropped on a view switch.
101+
*
102+
* Deliberately excludes `create-column`/`delete-column`: those are table-scoped
103+
* schema operations that merely have a layout side-effect, so they stay
104+
* undoable everywhere. Their layout half is suppressed at replay time instead —
105+
* see `entryOwnsLayout` in `use-table-undo`.
102106
*/
103-
export const LAYOUT_UNDO_ACTIONS = new Set<TableUndoAction['type']>([
104-
'create-column',
105-
'delete-column',
106-
'reorder-columns',
107-
])
107+
export const VIEW_SCOPED_UNDO_ACTIONS = new Set<TableUndoAction['type']>(['reorder-columns'])
108108

109109
export interface TableUndoStacks {
110110
undo: UndoEntry[]
@@ -120,8 +120,8 @@ export interface TableUndoState {
120120
patchUndoRowId: (tableId: string, oldRowId: string, newRowId: string) => void
121121
clear: (tableId: string) => void
122122
/**
123-
* Drops layout actions recorded under a different view. Called on every view
124-
* switch so undo can never write one view's layout into another.
123+
* Drops purely-layout actions recorded under a different view. Called on every
124+
* view switch so undo can never write one view's layout into another.
125125
*/
126126
pruneLayoutActions: (tableId: string, viewId: string | null) => void
127127
}

0 commit comments

Comments
 (0)