fix(schemas): publishing the contract already in force writes nothing - #583
Open
JArmandoAnaya wants to merge 2 commits into
Open
fix(schemas): publishing the contract already in force writes nothing#583JArmandoAnaya wants to merge 2 commits into
JArmandoAnaya wants to merge 2 commits into
Conversation
On a freshly created project, pressing Save version twice with no edits in between published two identical versions — the version panel itself then rendered "Nothing changed between v1 and v2". Two independent causes, and either one alone leaves the defect reachable. The kernel had no no-op guard at all: `create_version` computed the diff only to decide whether the change was destructive, and an empty one fell straight through to an insert. It now returns the active version unchanged when the proposed classes compare equal to it. Equality rather than an empty `diff_classes`, deliberately: the diff classifies whether existing annotations survive and ignores `color` on purpose, so gating on it would answer "saved" to somebody who changed a swatch and then discard the swatch. Equality implies an empty diff and never the reverse, so the diff stays the one definition of changed-in-a-way-that-matters. The editor measured "is there anything to save" against the snapshot the draft was seeded from, and refreshed that snapshot in the callback passed to `publish.mutate`. TanStack drops those callbacks when the observer's component unmounts — which is exactly what happens on a project that had no schema, because the invalidated 404 goes back to `pending` (`fetchState` resets the status whenever `data === undefined`) and `SchemaSection` swaps the editor for a loading state while the refetch flies. The draft came back holding an empty seed, read as dirty, and the next press published. It now measures against `active`, which is a prop and cannot be missed that way — the same question the kernel answers, so the two cannot disagree. The comparison itself was also wrong for this use: the draft builds its own class literals and the wire sends every optional field `LabelClassBody` declares, so a hand-added attribute has no `options` key where the server sends null, and `JSON.stringify` calls two identical contracts unequal. It is now a projection in the wire's own field order with the wire's own defaults. No wire shape change: the status stays 201 either way, because the API declares one 2xx response per operation and a client that branched on "did this succeed" would see no difference in any case. `openapi.json` and the generated client move only by the route's own prose.
The canonical comparison had no test: reverting it to `JSON.stringify` left the whole ui-core suite green, which makes it unverified code rather than a guard. This is the case it exists for — an attribute added in the editor carries no `options` key where the wire sends null, so a stringify calls one identical contract two.
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.
Closes #582.
On a freshly created project, pressing Save version twice with no edits in between published
two identical versions — the version panel itself then rendered "Nothing changed between v1 and
v2". Only the third press was caught. Two independent causes, and either one alone leaves the
defect reachable.
The kernel had no no-op guard at all
create_versioncomputeddiff_classesonly to decide whether the change was destructive; anempty diff fell straight through to the insert. It now returns the active version unchanged when
the proposed classes compare equal to it, before the destructive gate — an identical version
cannot be destructive, and the gate would raise on a no-op.
Identity is exact content equality, not an empty diff.
_class_changesexcludescolordeliberately — it classifies whether existing annotations survive, and a swatch does not decide
that — so gating on the diff would answer "saved" to somebody who changed a colour and then throw
the colour away. Equality implies an empty diff and never the reverse, so
domain/schema_diff.pyremains the single definition of changed in a way that matters and no second one is written.
test_a_colour_only_change_is_a_changeis that boundary, in both the kernel and the server suites.Only the active version is compared: re-publishing an older version's classes is a revert, and
answering it with that old version would leave the newer one in force.
The editor refreshed its baseline in a callback that does not always fire
The re-base already existed — the
onSuccesspassed topublish.mutate, which setsseed = created.classes. On a fresh project it never ran, and the chain was measured rather thanassumed (a probe logging the query state on every
SchemaSectionrender):useActiveSchemaanswers 404 on a schema-less project and therefore holds no data;useCreateSchemaVersion.onSuccessreturns theinvalidateQueriespromise, so the mutationawaits the refetch it triggers; TanStack v5's
fetchStateresetsstatustopendingwheneverdata === undefined, so the invalidated errored query goes back to pending rather than stayingin error;
SchemaSectionreturns<LoadingState/>onschema.isPending, unmounting the editormid-mutation; and
MutationObserverrunsmutate()-level callbacks only whilehasListeners().The draft came back holding
seed: [], read as dirty — the status line said "unsaved changescreate v2" — and the next press published. After v2 the query has data, nothing unmounts, the
callback fires, and press three is caught. That is the reported 1‑2‑3 pattern, and why it was
fresh-projects-only.
dirtynow measures againstactive?.classes ?? [], the version in force. That is the samequestion the kernel answers, so the two cannot disagree — and
activeis a prop, which cannot bedropped with an observer.
A third defect, found while testing the second
same()wasJSON.stringifyover the objects. The draft builds its own class literals and thewire sends every optional field
LabelClassBody/AttributeBodydeclare — a hand-added attributehas no
optionskey at all where the server sendsnull. Stringify calls two identicalcontracts unequal. Harmless while both sides came off the same response; load-bearing the moment
the comparison is against
active. It is now a projection in the wire's own field order with thewire's own defaults.
No wire shape change
The decision on the issue was 200-for-no-op / 201-for-created. It cannot land:
scripts/generate_client.mjsrefuses any operation declaring more than one 2xx response —— because it emits exactly one response check per
operationId. Rather than relax a shared gateevery operation passes through for one operation's benefit, the status stays 201 either way.
openapi.json: one line, the route's owndescription. No schema, parameter or response change.frontend/ui-core/src/generated/api.ts: +7 lines, the same prose as a doc comment. No type change.Stated rather than hidden: an HTTP client cannot tell a no-op from a creation except by
comparing the returned
versionagainst what it already knew. Nothing needs to today — theeditor's own guard means it never sends a duplicate,
unwrapdiscards the status anyway, and theCLI and MCP call the service in process where no status code exists.
Tests
A trap worth recording: the frontend test passed when first written. With an instant
fetchstub the defect does not reproduce — the refetch resolves before React commits, the pending render
never happens, nothing unmounts, and the callback fires. It needs a delay on the post-publish
GET, which is the honest model rather than a contrivance: every real request takes longer thanzero. That is written into the test's docstring so nobody removes it as noise.
Mutation-verified, each mutation applied with its anchor asserted present exactly once and
reverted by
git apply -Ron its own recorded diff, with a clean tree asserted either side:FAILED tests/kernel/test_schema_service.py::test_an_identical_version_is_a_no_op, and only thatdirtyback toshowing.seed× issues one request on a project that had no schema—expected 2 to be 1; the other 7 in the file still passsame()back toJSON.stringifyThat third green was a real finding rather than a formality: the canonical projection was
unverified code.
compares a hand-built attribute with the wire's own spelling of itwas added,which drives the editor's own "add attribute" control and answers the POST the way
AttributeBodyspells it; the mutation then reds it.
Five existing tests were updated, none weakened:
test_an_identical_version_is_still_a_new_version→ replaced bytest_an_identical_version_is_a_no_op.It asserted the reported behaviour, and its docstring argued the case against fixing it
("refusing a no-op would need an equality rule we would then have to defend against reordering
and colors"). Both concerns now have tests of their own:
test_reordering_the_classes_is_a_changeand
test_a_colour_only_change_is_a_change.tests/cli/test_schema_commands.py::test_applying_again_creates_the_next_version→test_applying_the_same_document_again_adds_nothing, now also asserting the version list has one entry.(
test_versions_are_numbered_one_past_the_highest_stored,test_provenance_is_not_part_of_what_a_version_declares,test_the_listing_carries_each_versions_own_provenance). Their fixture data now varies perversion; every assertion is unchanged.
CLI and MCP docstrings both claimed "this always adds one", which is now false; both corrected.
docs/schemas.mdgains a section and loses the paragraph asserting the opposite.Test plan
bash scripts/check.shin groups, because of the ~10-minute harness ceiling. Every group,including both browser suites and the opt-in
docsgroup, sincedocs/was touched.pythonfrontendgeneratedannotator + app e2e (chromium)browser cycle, real server (chromium)docsNo migration, no
FORMAT_VERSIONmove, noVERSIONmove, no new dependency, no CI job added,renamed or removed.
Found, not fixed
contract, so neither enters the comparison: somebody who edits only the "Why this version" box
and presses Save gets "No changes to save" and the message is not recorded. That follows from
content identity and matches the editor's pre-existing
dirty, which never watched the note —but it is a behaviour change to a shipped property, and
test_provenance_is_not_part_of_what_a_version_declareshad to be rewritten around it.project
activeisnullfor one render, so the draft falls back to empty and the class listblinks out and back. Same mechanism, different symptom, outside this issue's scope.
visionset schema applyprints the version in force without saying nothing happened. Thesentence is true for a no-op, so only the
--helpdocstring was corrected. Saying "unchanged"would need a pre-read.
The general lesson, for whoever meets it next: a callback passed to
mutate()is best-effort, sonothing whose absence changes what the next click does may live only in one.