fix(ui-react): save the Models step by diff, not delete-then-insert (#107) - #121
Merged
Conversation
…107) 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.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #107.
The fault
SetThreadModelsdeleted everythread_modelrow for the thread, then re-inserted theselection. All four tables that reference
thread_model.idareON DELETE RESTRICT(
thread_model_execution_summary,thread_model_execution,thread_model_io,thread_model_parameter), so the delete was refused as soon as the thread held any of them.After a thread passed Datasets once, its model selection could never be changed again.
ModelsStep.tsxusedtry { … } finally {}with nocatch, so the rejection was unhandled:no toast, no navigation, nothing.
The fix
diffThreadModels(src/lib/thread-models.ts) compares the selection against what is stored:parameter bindings hanging off it survive a trip back through the step;
scoped to those ids;
no-op instead of a failure.
Lit avoids the RESTRICT wall by deleting every child row for the thread on every save
(
ui/src/queries/thread/update-models.graphql). This does not copy that: a save that changesnothing must not destroy the later steps' work.
A
thread_modelrow with nomodelcatalog_configuration_idis left alone. The step cannotdisplay or select it,
threadModelFromGQLalready skips it, and deleting it would hit the sameRESTRICT wall on any legacy thread that holds runs. TACC's database holds 21 of them across 109
thread_modelrows, measured anonymously.Both call sites are fixed —
ModelsStep(the wizard) andMintModels(the expansion panel) —and both now surface a failed save as a
Save failedtoast.Not fixed here
Removing a model still discards that model's bindings and runs, without warning. Lit does the
same for every model on every save, so this is not a regression; a confirmation step is a
separate question.
Tests
npm test— 921 passing. The four newModelsStepassertions were checked to fail against theold code; they assert the outgoing mutation variables, not component state.
Still to do: verify live at TACC under a real Tapis token.