From 32c68c795800da1ef4c788f1cde3d273b1872210 Mon Sep 17 00:00:00 2001 From: Maximiliano Osorio Date: Mon, 10 Aug 2026 20:56:12 -0400 Subject: [PATCH] fix(ui-react): save the Models step by diff, not delete-then-insert (#107) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SetThreadModels deleted every thread_model row for the thread and re-inserted the selection. All four tables that reference thread_model.id are ON DELETE RESTRICT, so once a thread had been through the Datasets or Parameters step the delete was refused and Continue became a dead end. ModelsStep had no catch, so the rejection was swallowed: no toast, no navigation, nothing. diffThreadModels keeps the rows whose configuration is still selected, deletes only the rows whose configuration is not, and inserts only what is new. A kept row keeps its id, so the dataset and parameter bindings hanging off it survive a trip back through the step. An unchanged selection writes nothing at all. Removing a model still discards that model's bindings and runs — the mutation deletes its four child rows first, scoped to the removed ids. A thread_model row with no modelcatalog_configuration_id is left alone: the step cannot show it, threadModelFromGQL skips it, and deleting it would hit the same RESTRICT wall. TACC holds 21 such rows out of 109. Both call sites are fixed, and both now report a failed save. --- ui-react/src/graphql/generated/modeling.ts | 36 +++- .../src/lib/__tests__/thread-models.test.ts | 59 +++++++ ui-react/src/lib/thread-models.ts | 71 ++++++++ .../src/pages/modeling/thread/MintModels.tsx | 20 ++- .../modeling/thread/wizard/ModelsStep.tsx | 26 ++- .../wizard/__tests__/ModelsStep.test.tsx | 157 +++++++++++++++++- 6 files changed, 354 insertions(+), 15 deletions(-) create mode 100644 ui-react/src/lib/__tests__/thread-models.test.ts create mode 100644 ui-react/src/lib/thread-models.ts diff --git a/ui-react/src/graphql/generated/modeling.ts b/ui-react/src/graphql/generated/modeling.ts index 0c5a254..0abd068 100644 --- a/ui-react/src/graphql/generated/modeling.ts +++ b/ui-react/src/graphql/generated/modeling.ts @@ -1239,17 +1239,32 @@ export function generateModelingId(_type: 'problem_statement' | 'task' | 'thread // ─── Mutation: SetThreadModels ──────────────────────────────────────────────── /** - * Replace all model selections for a thread in a single transaction. - * Deletes existing thread_model rows then inserts the new selection. + * Apply a change of model selection for a thread in a single transaction. + * + * `$removedIds` are the `thread_model.id` values that are no longer selected. + * Rows that stay selected are not named here, so they keep their id and the + * dataset and parameter bindings that hang off it. + * + * The four child tables are deleted first, scoped to the removed rows only: + * every one of them references `thread_model.id` with `ON DELETE RESTRICT`, so + * without this the delete is refused for any thread that has been through the + * Datasets or Parameters step (monorepo#107). Hasura runs a mutation's root + * fields in order inside one transaction, so the ordering here is the ordering + * Postgres sees. */ export type SetThreadModelsMutationVariables = { threadId: string; + removedIds: string[]; models: Array<{ thread_id: string; modelcatalog_configuration_id: string }>; userid: string; notes?: string | null; }; export type SetThreadModelsMutation = { + delete_thread_model_execution_summary?: { affected_rows: number } | null; + delete_thread_model_execution?: { affected_rows: number } | null; + delete_thread_model_io?: { affected_rows: number } | null; + delete_thread_model_parameter?: { affected_rows: number } | null; delete_thread_model?: { affected_rows: number } | null; insert_thread_model?: { returning: Array<{ id: string; thread_id: string; modelcatalog_configuration_id?: string | null }>; @@ -1260,11 +1275,26 @@ export type SetThreadModelsMutation = { export const SetThreadModelsDocument = gql` mutation SetThreadModels( $threadId: String! + $removedIds: [uuid!]! $models: [thread_model_insert_input!]! $userid: String! $notes: String ) { - delete_thread_model(where: { thread_id: { _eq: $threadId } }) { + delete_thread_model_execution_summary( + where: { thread_model_id: { _in: $removedIds } } + ) { + affected_rows + } + delete_thread_model_execution(where: { thread_model_id: { _in: $removedIds } }) { + affected_rows + } + delete_thread_model_io(where: { thread_model_id: { _in: $removedIds } }) { + affected_rows + } + delete_thread_model_parameter(where: { thread_model_id: { _in: $removedIds } }) { + affected_rows + } + delete_thread_model(where: { id: { _in: $removedIds } }) { affected_rows } insert_thread_model(objects: $models) { diff --git a/ui-react/src/lib/__tests__/thread-models.test.ts b/ui-react/src/lib/__tests__/thread-models.test.ts new file mode 100644 index 0000000..a8674fc --- /dev/null +++ b/ui-react/src/lib/__tests__/thread-models.test.ts @@ -0,0 +1,59 @@ +import { describe, expect, it } from 'vitest'; +import { diffThreadModels } from '../thread-models'; + +const row = (id: string, configId: string | null) => ({ + id, + modelcatalog_configuration_id: configId, +}); + +describe('diffThreadModels', () => { + it('reports no change when the selection matches what is stored', () => { + const changes = diffThreadModels( + 't1', + [row('tm-a', 'cfgA'), row('tm-b', 'cfgB')], + ['cfgB', 'cfgA'], + ); + expect(changes).toEqual({ removedIds: [], added: [], unchanged: true }); + }); + + it('keeps a still-selected row instead of deleting and re-inserting it', () => { + const changes = diffThreadModels('t1', [row('tm-a', 'cfgA')], ['cfgA', 'cfgB']); + expect(changes.removedIds).toEqual([]); + expect(changes.added).toEqual([{ thread_id: 't1', modelcatalog_configuration_id: 'cfgB' }]); + expect(changes.unchanged).toBe(false); + }); + + it('removes only the rows whose configuration was deselected', () => { + const changes = diffThreadModels('t1', [row('tm-a', 'cfgA'), row('tm-b', 'cfgB')], ['cfgA']); + expect(changes.removedIds).toEqual(['tm-b']); + expect(changes.added).toEqual([]); + }); + + it('leaves a row with no configuration id alone', () => { + // 21 of TACC's 109 thread_model rows are in this state. The step cannot + // show them, and deleting one would hit the same ON DELETE RESTRICT wall + // this diff exists to avoid. + const changes = diffThreadModels('t1', [row('tm-legacy', null)], ['cfgA']); + expect(changes.removedIds).toEqual([]); + expect(changes.added).toEqual([{ thread_id: 't1', modelcatalog_configuration_id: 'cfgA' }]); + }); + + it('reports no change for a thread that holds only unselectable legacy rows', () => { + expect(diffThreadModels('t1', [row('tm-legacy', null)], []).unchanged).toBe(true); + }); + + it('inserts every selection for a thread that holds no rows yet', () => { + const changes = diffThreadModels('t1', [], ['cfgA', 'cfgB']); + expect(changes.added).toEqual([ + { thread_id: 't1', modelcatalog_configuration_id: 'cfgA' }, + { thread_id: 't1', modelcatalog_configuration_id: 'cfgB' }, + ]); + expect(changes.removedIds).toEqual([]); + }); + + it('does not re-insert a configuration that is stored twice', () => { + const changes = diffThreadModels('t1', [row('tm-a', 'cfgA'), row('tm-a2', 'cfgA')], ['cfgA']); + expect(changes.added).toEqual([]); + expect(changes.removedIds).toEqual([]); + }); +}); diff --git a/ui-react/src/lib/thread-models.ts b/ui-react/src/lib/thread-models.ts new file mode 100644 index 0000000..549b9bc --- /dev/null +++ b/ui-react/src/lib/thread-models.ts @@ -0,0 +1,71 @@ +/** + * Diffing the Models step's selection against the `thread_model` rows a thread + * already holds. + * + * The Models step used to save by deleting every `thread_model` row for the + * thread and re-inserting the selection. Four tables reference + * `thread_model.id` with `ON DELETE RESTRICT` — `thread_model_execution_summary`, + * `thread_model_execution`, `thread_model_io` and `thread_model_parameter` — so + * once the thread had been through the Datasets or Parameters step, the delete + * was refused and the step became a dead end (monorepo#107). + * + * The fix is to touch only what changed: keep the rows whose configuration is + * still selected, delete the rows whose configuration is not, insert the rows + * that are new. Keeping a row keeps its id, so the dataset and parameter + * bindings hanging off it survive a trip back to the Models step. + * + * A row with no `modelcatalog_configuration_id` is left alone. The step cannot + * display or select such a row, `threadModelFromGQL` skips it, and deleting it + * would hit the same RESTRICT wall on any legacy thread that holds runs. + * TACC's database holds 21 of them across 109 `thread_model` rows. + */ + +/** The `thread_model` fields this module needs; `GetThread` selects both. */ +export interface ExistingThreadModel { + id: string; + modelcatalog_configuration_id?: string | null; +} + +export interface ThreadModelInsert { + thread_id: string; + modelcatalog_configuration_id: string; +} + +export interface ThreadModelChanges { + /** `thread_model.id` values to delete, with their bindings and runs. */ + removedIds: string[]; + /** Rows to insert for newly selected configurations. */ + added: ThreadModelInsert[]; + /** True when the selection matches what is stored, so no write is needed. */ + unchanged: boolean; +} + +export function diffThreadModels( + threadId: string, + existing: readonly ExistingThreadModel[], + selectedConfigIds: Iterable, +): ThreadModelChanges { + const selected = new Set(selectedConfigIds); + const removedIds: string[] = []; + const stored = new Set(); + + for (const row of existing) { + const configId = row.modelcatalog_configuration_id; + if (!configId) continue; // unselectable legacy row — leave it in place + if (selected.has(configId)) stored.add(configId); + else removedIds.push(row.id); + } + + const added: ThreadModelInsert[] = []; + for (const configId of selected) { + if (!stored.has(configId)) { + added.push({ thread_id: threadId, modelcatalog_configuration_id: configId }); + } + } + + return { + removedIds, + added, + unchanged: removedIds.length === 0 && added.length === 0, + }; +} diff --git a/ui-react/src/pages/modeling/thread/MintModels.tsx b/ui-react/src/pages/modeling/thread/MintModels.tsx index cf3fc2e..803e2eb 100644 --- a/ui-react/src/pages/modeling/thread/MintModels.tsx +++ b/ui-react/src/pages/modeling/thread/MintModels.tsx @@ -22,6 +22,8 @@ import { useSetThreadModelsMutation, } from '@/graphql/generated/modeling'; import { useAuth } from '@/lib/auth/useAuth'; +import { diffThreadModels } from '@/lib/thread-models'; +import { useToast } from '@/components/ui/use-toast'; import { cn } from '@/lib/utils'; // ─── Types ───────────────────────────────────────────────────────────────────── @@ -209,6 +211,7 @@ interface MintModelsProps { export function MintModels({ thread, onContinue, onThreadUpdated }: MintModelsProps) { const { user } = useAuth(); + const { toast } = useToast(); const perm = getUserPermission(thread.permissions, thread.events, user?.username ?? null); const [editMode, setEditMode] = useState(() => { @@ -300,16 +303,21 @@ export function MintModels({ thread, onContinue, onThreadUpdated }: MintModelsPr async function handleSave() { if (!user?.username) return; + + const changes = diffThreadModels(thread.id, thread.thread_models ?? [], selectedIds); + if (changes.unchanged) { + setEditMode(false); + if (onContinue) onContinue(); + return; + } + setSaving(true); try { - const models = Array.from(selectedIds).map((cfgId) => ({ - thread_id: thread.id, - modelcatalog_configuration_id: cfgId, - })); await setThreadModels({ variables: { threadId: thread.id, - models, + removedIds: changes.removedIds, + models: changes.added, userid: user.username, notes: notes || null, }, @@ -317,6 +325,8 @@ export function MintModels({ thread, onContinue, onThreadUpdated }: MintModelsPr setEditMode(false); onThreadUpdated?.(); if (onContinue) onContinue(); + } catch (err) { + toast({ title: 'Save failed', description: String(err), variant: 'destructive' }); } finally { setSaving(false); } diff --git a/ui-react/src/pages/modeling/thread/wizard/ModelsStep.tsx b/ui-react/src/pages/modeling/thread/wizard/ModelsStep.tsx index 57882b6..6941503 100644 --- a/ui-react/src/pages/modeling/thread/wizard/ModelsStep.tsx +++ b/ui-react/src/pages/modeling/thread/wizard/ModelsStep.tsx @@ -13,6 +13,8 @@ import { useSetThreadModelsMutation, } from '@/graphql/generated/modeling'; import { useAuth } from '@/lib/auth/useAuth'; +import { diffThreadModels } from '@/lib/thread-models'; +import { useToast } from '@/components/ui/use-toast'; import { cn } from '@/lib/utils'; import { StepShell } from './StepShell'; import { FilteredByBanner } from './FilteredByBanner'; @@ -123,6 +125,7 @@ export function ModelsStep({ onEditIndicator, }: ModelsStepProps) { const { user } = useAuth(); + const { toast } = useToast(); const perm = getUserPermission(thread.permissions, thread.events, user?.username ?? null); const [searchText, setSearchText] = useState(''); @@ -185,17 +188,30 @@ export function ModelsStep({ async function handleContinue() { if (!user?.username || selectedIds.size === 0) return; + + const changes = diffThreadModels(thread.id, thread.thread_models ?? [], selectedIds); + // Nothing to write: walking back through the step must not touch the + // bindings the later steps have already stored against these rows. + if (changes.unchanged) { + onContinue(); + return; + } + setSaving(true); try { - const models = Array.from(selectedIds).map((cfgId) => ({ - thread_id: thread.id, - modelcatalog_configuration_id: cfgId, - })); await setThreadModels({ - variables: { threadId: thread.id, models, userid: user.username, notes: null }, + variables: { + threadId: thread.id, + removedIds: changes.removedIds, + models: changes.added, + userid: user.username, + notes: null, + }, }); onUpdated(); onContinue(); + } catch (err) { + toast({ title: 'Save failed', description: String(err), variant: 'destructive' }); } finally { setSaving(false); } diff --git a/ui-react/src/pages/modeling/thread/wizard/__tests__/ModelsStep.test.tsx b/ui-react/src/pages/modeling/thread/wizard/__tests__/ModelsStep.test.tsx index 0142f0f..75d2868 100644 --- a/ui-react/src/pages/modeling/thread/wizard/__tests__/ModelsStep.test.tsx +++ b/ui-react/src/pages/modeling/thread/wizard/__tests__/ModelsStep.test.tsx @@ -1,10 +1,19 @@ -import { describe, expect, it, vi } from 'vitest'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; import type { MockedResponse } from '@apollo/client/testing'; import userEvent from '@testing-library/user-event'; import { renderWithProviders, screen, waitFor } from '@/test/utils/render'; -import { GetModelTreeWithRegionsDocument, type Thread } from '@/graphql/generated/modeling'; +import { + GetModelTreeWithRegionsDocument, + SetThreadModelsDocument, + type Thread, +} from '@/graphql/generated/modeling'; import { ModelsStep } from '../ModelsStep'; +const toastSpy = vi.fn(); +vi.mock('@/components/ui/use-toast', () => ({ + useToast: () => ({ toast: toastSpy }), +})); + function makeThread(overrides: Partial = {}): Thread { return { __typename: 'thread', @@ -137,4 +146,148 @@ describe('ModelsStep', () => { await userEvent.click(screen.getByLabelText(/select PIHM Flood A/i)); await waitFor(() => expect(screen.getByTestId('step-continue')).toBeEnabled()); }); + + // ── Saving the selection (monorepo#107) ──────────────────────────────────── + // + // The four tables that reference thread_model.id are ON DELETE RESTRICT, so + // the step must never delete a row it means to keep. These assert the + // outgoing mutation variables, not the component's internal state. + + describe('saving', () => { + beforeEach(() => toastSpy.mockClear()); + + function threadWithModels() { + return makeThread({ + thread_models: [ + { + __typename: 'thread_model', + id: 'tm-a', + thread_id: 't1', + modelcatalog_configuration_id: 'cfgA', + }, + { + __typename: 'thread_model', + id: 'tm-b', + thread_id: 't1', + modelcatalog_configuration_id: 'cfgB', + }, + ], + }); + } + + function saveMock(sent: Record[]): MockedResponse { + return { + request: { query: SetThreadModelsDocument }, + maxUsageCount: Number.MAX_SAFE_INTEGER, + variableMatcher: (vars) => { + sent.push(vars); + return true; + }, + result: { + data: { + delete_thread_model_execution_summary: { affected_rows: 0 }, + delete_thread_model_execution: { affected_rows: 0 }, + delete_thread_model_io: { affected_rows: 0 }, + delete_thread_model_parameter: { affected_rows: 0 }, + delete_thread_model: { affected_rows: 1 }, + insert_thread_model: { returning: [] }, + insert_thread_provenance_one: { thread_id: 't1' }, + }, + }, + }; + } + + it('writes nothing when the selection is unchanged', async () => { + const sent: Record[] = []; + const onContinue = vi.fn(); + renderWithProviders( + , + { apolloMocks: [treeMock, saveMock(sent)] }, + ); + await screen.findByText('PIHM Flood A'); + await userEvent.click(screen.getByTestId('step-continue')); + + await waitFor(() => expect(onContinue).toHaveBeenCalled()); + expect(sent).toEqual([]); + }); + + it('deletes only the deselected row and re-inserts nothing', async () => { + const sent: Record[] = []; + renderWithProviders( + , + { apolloMocks: [treeMock, saveMock(sent)] }, + ); + await screen.findByText('Crop Model B'); + await userEvent.click(screen.getByLabelText(/select Crop Model B/i)); + await userEvent.click(screen.getByTestId('step-continue')); + + await waitFor(() => expect(sent).toHaveLength(1)); + expect(sent[0]).toMatchObject({ removedIds: ['tm-b'], models: [] }); + }); + + it('inserts only the newly selected row and deletes nothing', async () => { + const sent: Record[] = []; + const thread = makeThread({ + thread_models: [ + { + __typename: 'thread_model', + id: 'tm-a', + thread_id: 't1', + modelcatalog_configuration_id: 'cfgA', + }, + ], + }); + renderWithProviders( + , + { apolloMocks: [treeMock, saveMock(sent)] }, + ); + await screen.findByText('Crop Model B'); + await userEvent.click(screen.getByLabelText(/select Crop Model B/i)); + await userEvent.click(screen.getByTestId('step-continue')); + + await waitFor(() => expect(sent).toHaveLength(1)); + expect(sent[0]).toMatchObject({ + removedIds: [], + models: [{ thread_id: 't1', modelcatalog_configuration_id: 'cfgB' }], + }); + }); + + it('reports a rejected save instead of failing silently', async () => { + const onContinue = vi.fn(); + const failing: MockedResponse = { + request: { query: SetThreadModelsDocument }, + variableMatcher: () => true, + error: new Error('Foreign key violation'), + }; + renderWithProviders( + , + { apolloMocks: [treeMock, failing] }, + ); + await screen.findByText('Crop Model B'); + await userEvent.click(screen.getByLabelText(/select Crop Model B/i)); + await userEvent.click(screen.getByTestId('step-continue')); + + await waitFor(() => + expect(toastSpy).toHaveBeenCalledWith( + expect.objectContaining({ title: 'Save failed', variant: 'destructive' }), + ), + ); + expect(onContinue).not.toHaveBeenCalled(); + }); + }); });