From 87d3651f35f4880b309aa444268245e3dba28354 Mon Sep 17 00:00:00 2001 From: Naomi Gilbert Date: Fri, 31 Jul 2026 19:56:07 -0400 Subject: [PATCH] fix(builder): rebase draft fields after save to stop duplicate POSTs handleSaveAll only invalidated the fields query after saving, and the load effect is guarded by initializedRef, so local state was never re-synced. A field created in this session kept isNew: true / persistedId: null, and a second click on Save POSTed it again, creating a duplicate field on the table. Capture each mutation response keyed by draft id and rebase local state from it (isNew: false, persistedId from the created field), so the second save takes the PUT branch. Saves are settled per field rather than through a bare Promise.all: an all-or-nothing rejection would have discarded the rebase for every field that did save, reintroducing the duplicate on the next click. Fields whose save failed keep their draft state. Adds a regression test asserting two consecutive saves issue exactly one POST per new field, followed by a PUT to the created id. --- .../builder.$databaseId.$tableId.test.tsx | 88 +++++++++++++++++++ .../routes/builder.$databaseId.$tableId.tsx | 26 +++++- 2 files changed, 110 insertions(+), 4 deletions(-) create mode 100644 apps/client/src/routes/__tests__/builder.$databaseId.$tableId.test.tsx diff --git a/apps/client/src/routes/__tests__/builder.$databaseId.$tableId.test.tsx b/apps/client/src/routes/__tests__/builder.$databaseId.$tableId.test.tsx new file mode 100644 index 0000000..922ca0c --- /dev/null +++ b/apps/client/src/routes/__tests__/builder.$databaseId.$tableId.test.tsx @@ -0,0 +1,88 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { screen, waitFor } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; +import StructureBuilder from '../builder.$databaseId.$tableId'; +import { apiClient } from '../../lib/apiClient'; +import { render } from '../../test/render'; + +vi.mock('@tanstack/react-router', () => ({ + createFileRoute: () => () => ({}), + useParams: () => ({ databaseId: 'db-1', tableId: 'tbl-1' }), +})); + +const queryClient = new QueryClient({ + defaultOptions: { + queries: { retry: false }, + mutations: { retry: false }, + }, +}); + +const mockDatabase = { id: 'db-1', name: 'Patrimoine', workspace_id: 'ws-1' }; +const mockTable = { id: 'tbl-1', name: 'Auteurs', database_id: 'db-1' }; + +function renderBuilder() { + return render( + + + + ); +} + +describe('StructureBuilder save', () => { + beforeEach(() => { + queryClient.clear(); + vi.restoreAllMocks(); + }); + + it('does not re-POST a field created in the same session on a second save', async () => { + const user = userEvent.setup(); + const serverFields: Record[] = []; + + vi.spyOn(apiClient, 'get').mockImplementation((url: string) => { + if (url.startsWith('/databases/db-1')) return Promise.resolve(mockDatabase); + if (url.startsWith('/tables/tbl-1')) return Promise.resolve(mockTable); + if (url.startsWith('/tables?database_id=db-1')) return Promise.resolve([mockTable]); + if (url.startsWith('/fields?table_id=tbl-1')) return Promise.resolve(serverFields); + return Promise.reject(new Error(`Unexpected URL: ${url}`)); + }); + + const post = vi.spyOn(apiClient, 'post').mockImplementation((_url, data) => { + const created = { ...(data as Record), id: 'field-created-1' }; + serverFields.push(created); + return Promise.resolve(created); + }); + + const put = vi.spyOn(apiClient, 'put').mockImplementation((_url, data) => { + return Promise.resolve({ ...(data as Record), id: 'field-created-1' }); + }); + + renderBuilder(); + + await waitFor(() => { + expect(screen.getByTestId('add-field-text')).toBeInTheDocument(); + }); + + await user.click(screen.getByTestId('add-field-text')); + + const saveButton = screen.getByRole('button', { name: /save/i }); + + await user.click(saveButton); + await waitFor(() => { + expect(post).toHaveBeenCalledTimes(1); + }); + expect(post).toHaveBeenCalledWith('/fields', expect.objectContaining({ type: 'text' })); + + await user.click(saveButton); + await waitFor(() => { + expect(put).toHaveBeenCalledTimes(1); + }); + + // The field was created once and updated on the second save — never POSTed twice. + expect(post).toHaveBeenCalledTimes(1); + expect(put).toHaveBeenCalledWith( + '/fields/field-created-1', + expect.objectContaining({ type: 'text' }) + ); + }); +}); diff --git a/apps/client/src/routes/builder.$databaseId.$tableId.tsx b/apps/client/src/routes/builder.$databaseId.$tableId.tsx index ce94d39..ad936be 100644 --- a/apps/client/src/routes/builder.$databaseId.$tableId.tsx +++ b/apps/client/src/routes/builder.$databaseId.$tableId.tsx @@ -350,10 +350,28 @@ function StructureBuilder() { const newFields = fields.filter((f) => f.isNew); const existingFields = fields.filter((f) => !f.isNew); - Promise.all([ - ...newFields.map((f) => saveFieldMutation.mutateAsync(f)), - ...existingFields.map((f) => saveFieldMutation.mutateAsync(f)), - ]).then(() => { + const saveField = async (field: BuilderField): Promise<[string, Field | null]> => { + try { + return [field.id, await saveFieldMutation.mutateAsync(field)]; + } catch { + return [field.id, null]; + } + }; + + Promise.all([...newFields.map(saveField), ...existingFields.map(saveField)]).then((results) => { + const savedByDraftId = new Map(results); + + // Rebase drafts onto what the server persisted: without this a field created in + // this session keeps isNew/persistedId=null and the next save POSTs it again, + // duplicating it on the table. Fields whose save failed keep their draft state. + setFields((prev) => + prev.map((f) => { + const persistedId = savedByDraftId.get(f.id)?.id ?? f.persistedId; + if (!persistedId) return f; + return { ...f, isNew: false, persistedId }; + }) + ); + queryClient.invalidateQueries({ queryKey: ['fields', tableId] }); }); }, [fields, queryClient, tableId, saveFieldMutation]);