From 1bfabcae0a021d5eabd0246742498feb8276e715 Mon Sep 17 00:00:00 2001 From: Naomi Gilbert Date: Fri, 31 Jul 2026 20:12:43 -0400 Subject: [PATCH] fix(builder): extend the unsaved-changes guard to the layout tab MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The route-level blocker only compared the field draft against its snapshot, so it covered the structure tab alone. The layout tab renders CardLayoutBuilder, which holds its own working layout and persists it through an explicit Save (PUT /views/{id}); unsaved layout edits were still discarded silently on navigation. Lift a dirty signal out of CardLayoutBuilder through an optional onDirtyChange prop and OR it into the same blocker, so both tabs share one guard and one UnsavedChangesModal. snapshotLayout serializes exactly what handleSave persists and normalizes hiddenLabels to the ids genuinely hidden, so toggling a label on and back off — which leaves an explicit false — still reads as clean. The baseline is rebased wherever the saved layout changes: on load, from the same normalized data the working copy is built from, so fields dropped for no longer existing and columns padded to columnCount do not register as edits; and on save, from the mutation payload rather than the refetch, so the guard releases as soon as the save lands. The signal resets on unmount, since the working layout lives in the component and is already gone by then. The sync effect's three duplicated fallback branches collapse into one while computing that baseline, and columnsOf replaces the column-flattening repeated across the snapshot, handleSave and the render path. Note this does not cover switching between the two tabs, which still discards layout edits: CardLayoutBuilder unmounts and takes its state with it, so no blocker can intervene. Closing that needs the layout state lifted into the route. Co-Authored-By: Claude Opus 5 --- .../src/components/CardLayoutBuilder.tsx | 117 ++++++++++++------ .../__tests__/CardLayoutBuilder.test.tsx | 107 +++++++++++++++- .../__tests__/builder-unsaved-guard.test.tsx | 32 +++++ .../routes/builder.$databaseId.$tableId.tsx | 12 +- 4 files changed, 228 insertions(+), 40 deletions(-) diff --git a/apps/client/src/components/CardLayoutBuilder.tsx b/apps/client/src/components/CardLayoutBuilder.tsx index 1c0fe28..3324ffe 100644 --- a/apps/client/src/components/CardLayoutBuilder.tsx +++ b/apps/client/src/components/CardLayoutBuilder.tsx @@ -27,6 +27,11 @@ import { LoadingSpinner } from './LoadingSpinner'; interface CardLayoutBuilderProps { tableId: string; fields: BuilderField[]; + /** + * Reports whether the working layout differs from the selected view's saved config, so + * the route can guard navigation. Must be referentially stable. + */ + onDirtyChange?: (isDirty: boolean) => void; } interface View { @@ -59,7 +64,32 @@ function DroppableColumn({ id, children, className, style }: DroppableColumnProp ); } -export function CardLayoutBuilder({ tableId, fields }: CardLayoutBuilderProps) { +/** Flatten the drag-and-drop containers into the column array the API stores. */ +function columnsOf(items: Record, columnCount: number): string[][] { + return Array.from({ length: columnCount }).map((_, idx) => items[`column-${idx}`] || []); +} + +/** + * Serialize the parts of the layout that `handleSave` persists, so the working copy can be + * compared against what the selected view holds server-side. Fields left unassigned are + * excluded because their order is not part of the saved config. + */ +function snapshotLayout( + columnCount: number, + columns: string[][], + hiddenLabels: Record +): string { + return JSON.stringify({ + columnCount, + columns, + // Toggling a label off leaves an explicit `false`, which is equivalent to absent. + hiddenLabels: Object.keys(hiddenLabels) + .filter((id) => hiddenLabels[id]) + .sort(), + }); +} + +export function CardLayoutBuilder({ tableId, fields, onDirtyChange }: CardLayoutBuilderProps) { const { t } = useI18n(); const queryClient = useQueryClient(); @@ -74,6 +104,10 @@ export function CardLayoutBuilder({ tableId, fields }: CardLayoutBuilderProps) { const [hiddenLabels, setHiddenLabels] = useState>({}); + // Snapshot of the selected view's saved config, compared against the working layout to + // know whether leaving the page would discard work. + const [savedLayout, setSavedLayout] = useState(() => snapshotLayout(1, [[]], {})); + // Dragging states const [activeId, setActiveId] = useState(null); const [toastMessage, setToastMessage] = useState(null); @@ -102,25 +136,19 @@ export function CardLayoutBuilder({ tableId, fields }: CardLayoutBuilderProps) { // Sync state with selected view layout config useEffect(() => { - if (!selectedView) { - setColumnCount(1); - setLayoutItems({ - unassigned: [], - 'column-0': fields.map((f) => f.id), - }); - setHiddenLabels({}); - return; - } + // Fallback for a view with no usable config: every field stacked in one column. + let nextCount = 1; + let nextItems: Record = { + unassigned: [], + 'column-0': fields.map((f) => f.id), + }; + let nextLabels: Record = {}; - const fieldIdsMap = new Set(fields.map((f) => f.id)); - const config = selectedView.config; + const config = selectedView?.config; if (config && config.columns && typeof config.columnCount === 'number') { - setColumnCount(config.columnCount); - - const newItems: Record = { - unassigned: [], - }; + const fieldIdsMap = new Set(fields.map((f) => f.id)); + nextCount = config.columnCount; // Load columns const loadedColumns = config.columns.map((col) => col.filter((id) => fieldIdsMap.has(id))); @@ -133,26 +161,45 @@ export function CardLayoutBuilder({ tableId, fields }: CardLayoutBuilderProps) { loadedColumns.splice(config.columnCount); } + nextItems = { unassigned: [] }; loadedColumns.forEach((col, idx) => { - newItems[`column-${idx}`] = col; + nextItems[`column-${idx}`] = col; }); // Find any fields in table that are not placed in columns const placedIds = new Set(loadedColumns.flat()); - newItems.unassigned = fields.map((f) => f.id).filter((id) => !placedIds.has(id)); + nextItems.unassigned = fields.map((f) => f.id).filter((id) => !placedIds.has(id)); - setLayoutItems(newItems); - setHiddenLabels(config.hiddenLabels ?? {}); - } else { - setColumnCount(1); - setLayoutItems({ - unassigned: [], - 'column-0': fields.map((f) => f.id), - }); - setHiddenLabels({}); + nextLabels = config.hiddenLabels ?? {}; } + + setColumnCount(nextCount); + setLayoutItems(nextItems); + setHiddenLabels(nextLabels); + // Rebase the baseline onto what was just loaded, normalized the same way the working + // copy is, so dropped fields and padded columns do not read as user edits. + setSavedLayout(snapshotLayout(nextCount, columnsOf(nextItems, nextCount), nextLabels)); }, [selectedView, fields]); + const isLayoutDirty = useMemo(() => { + // Without a selected view there is nothing to save to, so nothing can be lost. + if (!selectedViewId) return false; + return ( + snapshotLayout(columnCount, columnsOf(layoutItems, columnCount), hiddenLabels) !== savedLayout + ); + }, [selectedViewId, columnCount, layoutItems, hiddenLabels, savedLayout]); + + // Report upward so the route-level blocker guards this tab too. + useEffect(() => { + onDirtyChange?.(isLayoutDirty); + }, [isLayoutDirty, onDirtyChange]); + + // The working layout lives in this component: once it unmounts the edits are already + // gone, so there is nothing left for the route to warn about. + useEffect(() => { + return () => onDirtyChange?.(false); + }, [onDirtyChange]); + // Change columns count handler const handleColumnCountChange = (newCount: number) => { setColumnCount(newCount); @@ -217,8 +264,11 @@ export function CardLayoutBuilder({ tableId, fields }: CardLayoutBuilderProps) { config, }); }, - onSuccess: () => { + onSuccess: (_data, config) => { queryClient.invalidateQueries({ queryKey: ['views', tableId] }); + // Rebase immediately rather than waiting for the refetch, so the guard stops firing + // as soon as the save lands. + setSavedLayout(snapshotLayout(config.columnCount, config.columns, config.hiddenLabels ?? {})); showToast(t('layout.saveSuccess')); }, }); @@ -350,12 +400,9 @@ export function CardLayoutBuilder({ tableId, fields }: CardLayoutBuilderProps) { }; const handleSave = () => { - const colsArray = Array.from({ length: columnCount }).map( - (_, idx) => layoutItems[`column-${idx}`] || [] - ); saveLayoutMutation.mutate({ columnCount, - columns: colsArray, + columns: columnsOf(layoutItems, columnCount), hiddenLabels, }); }; @@ -380,9 +427,7 @@ export function CardLayoutBuilder({ tableId, fields }: CardLayoutBuilderProps) { const activeField = activeId ? fieldMap.get(activeId) : null; const unassigned = layoutItems.unassigned || []; - const columns = Array.from({ length: columnCount }).map( - (_, idx) => layoutItems[`column-${idx}`] || [] - ); + const columns = columnsOf(layoutItems, columnCount); if (viewsQuery.isLoading) { return ( diff --git a/apps/client/src/components/__tests__/CardLayoutBuilder.test.tsx b/apps/client/src/components/__tests__/CardLayoutBuilder.test.tsx index c1d08ea..9c30ae3 100644 --- a/apps/client/src/components/__tests__/CardLayoutBuilder.test.tsx +++ b/apps/client/src/components/__tests__/CardLayoutBuilder.test.tsx @@ -1,4 +1,4 @@ -import { describe, it, expect, vi } from 'vitest'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { screen, fireEvent, waitFor } from '@testing-library/react'; import { CardLayoutBuilder } from '../CardLayoutBuilder'; import { type BuilderField } from '../../lib/fieldTypes'; @@ -71,3 +71,108 @@ describe('CardLayoutBuilder', () => { }); }); }); + +describe('CardLayoutBuilder unsaved-changes signal', () => { + function renderBuilder(onDirtyChange: (isDirty: boolean) => void) { + // A fresh client per test, so a cached view list cannot leak between them. + const client = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + return render( + + + + ); + } + + /** Wait for the saved view to load, then report the last dirty value seen. */ + async function lastDirty(onDirtyChange: ReturnType) { + await waitFor(() => { + expect(screen.getAllByTitle('Retirer')).toHaveLength(2); + }); + return onDirtyChange.mock.calls.at(-1)?.[0]; + } + + beforeEach(() => { + vi.spyOn(apiClient, 'get').mockImplementation((url) => { + if (url.startsWith('/views')) { + return Promise.resolve(mockViews); + } + return Promise.reject(new Error('Unexpected URL')); + }); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + it('reports clean while the working layout still matches the saved view', async () => { + const onDirtyChange = vi.fn(); + renderBuilder(onDirtyChange); + + expect(await lastDirty(onDirtyChange)).toBe(false); + }); + + it('reports dirty once a field is moved out of a column', async () => { + const onDirtyChange = vi.fn(); + renderBuilder(onDirtyChange); + + expect(await lastDirty(onDirtyChange)).toBe(false); + + fireEvent.click(screen.getAllByTitle('Retirer')[0]); + + await waitFor(() => { + expect(onDirtyChange.mock.calls.at(-1)?.[0]).toBe(true); + }); + }); + + it('reports dirty once the column count changes', async () => { + const onDirtyChange = vi.fn(); + renderBuilder(onDirtyChange); + + expect(await lastDirty(onDirtyChange)).toBe(false); + + fireEvent.click(screen.getByRole('button', { name: '3 columns' })); + + await waitFor(() => { + expect(onDirtyChange.mock.calls.at(-1)?.[0]).toBe(true); + }); + }); + + it('reports clean again once the layout is saved', async () => { + const put = vi.spyOn(apiClient, 'put').mockResolvedValue({}); + const onDirtyChange = vi.fn(); + renderBuilder(onDirtyChange); + + expect(await lastDirty(onDirtyChange)).toBe(false); + + fireEvent.click(screen.getAllByTitle('Retirer')[0]); + await waitFor(() => { + expect(onDirtyChange.mock.calls.at(-1)?.[0]).toBe(true); + }); + + fireEvent.click(screen.getByRole('button', { name: /Save Layout/ })); + + await waitFor(() => { + expect(put).toHaveBeenCalledWith('/views/view-1', { + config: { columnCount: 2, columns: [[], ['f2']], hiddenLabels: {} }, + }); + }); + await waitFor(() => { + expect(onDirtyChange.mock.calls.at(-1)?.[0]).toBe(false); + }); + }); + + it('reports clean on unmount, since the working layout goes with it', async () => { + const onDirtyChange = vi.fn(); + const { unmount } = renderBuilder(onDirtyChange); + + expect(await lastDirty(onDirtyChange)).toBe(false); + + fireEvent.click(screen.getAllByTitle('Retirer')[0]); + await waitFor(() => { + expect(onDirtyChange.mock.calls.at(-1)?.[0]).toBe(true); + }); + + unmount(); + expect(onDirtyChange.mock.calls.at(-1)?.[0]).toBe(false); + }); +}); diff --git a/apps/client/src/routes/__tests__/builder-unsaved-guard.test.tsx b/apps/client/src/routes/__tests__/builder-unsaved-guard.test.tsx index 9c88f37..c7a4cad 100644 --- a/apps/client/src/routes/__tests__/builder-unsaved-guard.test.tsx +++ b/apps/client/src/routes/__tests__/builder-unsaved-guard.test.tsx @@ -37,6 +37,18 @@ const mockFields = [ }, ]; +// The layout builder keys its columns by draft field id, which the page derives from the +// persisted id. +const mockViews = [ + { + id: 'view-1', + table_id: 'tbl-1', + name: 'Fiche', + type: 'card', + config: { columnCount: 1, columns: [['draft-f1']] }, + }, +]; + function renderBuilder() { const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } }); return render( @@ -55,6 +67,7 @@ describe('schema builder unsaved-changes guard', () => { if (url.startsWith('/tables/')) return Promise.resolve(mockTable) as any; if (url.startsWith('/fields?')) return Promise.resolve(mockFields) as any; if (url.startsWith('/tables?')) return Promise.resolve([mockTable]) as any; + if (url.startsWith('/views')) return Promise.resolve(mockViews) as any; return Promise.resolve([]) as any; }); }); @@ -83,6 +96,25 @@ describe('schema builder unsaved-changes guard', () => { expect(blockerOpts?.enableBeforeUnload()).toBe(true); }); + it('blocks navigation once the layout tab holds unsaved edits', async () => { + const user = userEvent.setup(); + renderBuilder(); + + await user.click(await screen.findByTestId('layout-tab')); + + // The saved layout is loaded as-is, so nothing is at risk yet. + const removeButton = await screen.findByTitle('Retirer'); + expect(blockerOpts?.shouldBlockFn()).toBe(false); + + // Pull the field out of its column without saving. + await user.click(removeButton); + + await waitFor(() => { + expect(blockerOpts?.shouldBlockFn()).toBe(true); + }); + expect(blockerOpts?.enableBeforeUnload()).toBe(true); + }); + it('offers to stay or leave when the router reports a blocked navigation', async () => { const user = userEvent.setup(); const proceed = vi.fn(); diff --git a/apps/client/src/routes/builder.$databaseId.$tableId.tsx b/apps/client/src/routes/builder.$databaseId.$tableId.tsx index 9a197e0..727f878 100644 --- a/apps/client/src/routes/builder.$databaseId.$tableId.tsx +++ b/apps/client/src/routes/builder.$databaseId.$tableId.tsx @@ -109,6 +109,9 @@ function StructureBuilder() { // Snapshot of what the server holds, keyed by draft field id. Compared against the // live draft to know whether leaving the page would discard work. const [savedSnapshot, setSavedSnapshot] = useState>({}); + // The layout tab keeps its own working state; `CardLayoutBuilder` reports whether it + // holds edits that the explicit Save has not persisted yet. + const [isLayoutDirty, setIsLayoutDirty] = useState(false); const [deleteError, setDeleteError] = useState(null); const [pendingAction, setPendingAction] = useState<{ fieldId: string; @@ -410,7 +413,7 @@ function StructureBuilder() { }); }, [fields, queryClient, tableId, saveFieldMutation]); - const isDirty = useMemo(() => { + const isStructureDirty = useMemo(() => { const current = snapshotFields(fields); const currentIds = Object.keys(current); const savedIds = Object.keys(savedSnapshot); @@ -419,6 +422,9 @@ function StructureBuilder() { return currentIds.some((id) => current[id] !== savedSnapshot[id]); }, [fields, savedSnapshot]); + // Either tab holds work behind an explicit Save, so either can be lost on navigation. + const isDirty = isStructureDirty || isLayoutDirty; + // Covers in-app navigation; `enableBeforeUnload` covers tab close and reload. const blocker = useBlocker({ shouldBlockFn: () => isDirty, @@ -527,7 +533,7 @@ function StructureBuilder() { selectedId={selectedFieldId} onSelect={setSelectedFieldId} onRemove={handleRemove} - isDirty={isDirty} + isDirty={isStructureDirty} /> @@ -548,7 +554,7 @@ function StructureBuilder() { ) : ( - + )} {pendingAction && (