fix(schema): count the blast radius in the narrowing confirmation - #698
Merged
Conversation
The narrowing confirmation dialog asked to confirm without saying what confirming costs. It now states how many classes narrow and that zero annotations use what is removed — a measured number, since blockers is always empty on a preview that reaches this dialog. Drops the 'existing annotations are not touched' line, which promised something about post-publish annotation state the kernel has not decided.
The SchemaEditor docstring and docs/ui.md both asserted the editor does not call schema/preview and that publishing's 409 is the sole way a narrowing refusal surfaces. The editor calls preview twice, before a class leaves the draft and before a publish, so both surfaces now describe that flow and how it relates to the publish's own 409.
…t shape DatasetScreen.tsx's file docstring still described the schema editor as attempt-shaped for export consent, the same claim already corrected in docs/ui.md and SchemaEditor.tsx. The schema editor previews first, so DatasetScreen's docstring now says the same thing they do.
…ribute case The dialog said 'no annotations use what this removes,' which is false when the destructive change is an added required attribute rather than a removal. Replace with kind-agnostic copy that states the measured fact for all four destructive shapes, and update docs/ui.md and a DatasetScreen docstring that referenced the old phrasing or leaned on past-tense/forward-looking language.
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.
Summary
The schema editor's narrowing confirmation named the classes it was about to narrow and then
asked for a decision without saying what the decision cost. It now states the blast radius as a
count, which is what
DESIGN.md's rule for destructive confirmations asks for: how many classesnarrow, which ones, and that nothing already labeled is invalidated by publishing.
Alongside that, three prose surfaces still described a design the editor no longer has. Since the
editor began previewing a proposed narrowing before changing the draft or publishing,
docs/ui.mdand the
SchemaEditorfile docstring both continued to assert — with reasoning attached — that theeditor does not call
POST .../schema/previewand that making the publish's own refusal legible,rather than pre-empting it, was the editor's whole job. Both now describe the shipped flow. A third
copy of the same claim lived in
DatasetScreen's docstring, which used the schema editor as itsexample of attempt-shaped consent for export; export still works that way, so that sentence keeps
its point and loses the comparison that had gone stale.
What the confirmation says now
Two properties of that copy are deliberate and worth stating, because both are easy to undo by
accident.
It is agnostic to which kind of destructive change happened. An earlier draft of this change
said no annotations use what this removes, which reads well and is wrong:
ChangeKind.DESTRUCTIVEcovers seven shapes, and only some are removals. Adding a required attribute is destructive, and the
class-detail panel ships that checkbox, so two clicks produced a dialog telling somebody that nothing
used what they had removed when they had removed nothing. The tempting repair — no annotations use
the classes this changes — is false in a different case: a class carrying five hundred boxes can
drop an unused polygon geometry with no blockers at all, because
orphanable_shapesscopes ageometry change to that one shape. No existing annotation becomes invalid is true in all seven.
It says nothing about what becomes of annotations after the publish. The version this dialog
publishes is judged against the active version, while annotation writes are judged against the
batch's own pin, so an open batch pinned to the outgoing version can still write a class the new
active version does not declare. Which invariant the kernel enforces there is an open question
(#585), and the sentence this change removed — existing annotations are not touched — answered it
by implication. The replacement states only what the preview measured, in the present tense.
Scope
This is part of the class-removal flow work (#588), which stays open. Two of that issue's
requirements are untouched here. It asks the terminal orphan dialog to link to the annotations
filtered to the blocking class, and no endpoint accepts a label-class filter today, so that needs a
wire and information-architecture change of its own before the UI can offer it. It also asks that
refusals reaching the UI render through the shared code-to-prose vocabulary; two
Alerts on thisscreen still use the raw error code as their title, and the same pattern appears three times in
AnnotationPage, which makes it a product-wideAlertconvention rather than a defect local to theschema editor.
The narrowing refusal's grain narrowed when a geometry drop began being refused only where
annotations carry that shape (#592), and the copy here is written to that grain.
Test plan
Full
bash scripts/check.sh, run in stages because the browser suites exceed this environment'scommand ceiling. Every group green at
d09b575:ui-coreis 1,149 tests over 53 files, one more than before this branch. The added test covers theplural branch of the count, and it was verified by breaking it rather than by watching it pass:
mutating the singular/plural ternary so both arms read
class narrowsproducesThe machine was idle throughout (load average 0.58 across 20 cores), so no suite was substituted for
another and no staged-gate fallback applies.
Found, not fixed
frontend/ui-core/src/screens/screens.test.tsxcarries an assertion that nothing can fail:not.toContain("internal wording must not appear")checks for a string no code path emits. Itpredates this branch and sits in a block this change touches, which is the only reason it is
mentioned here.
A Radix focus-scope timeout in the
ui-coresuite failed once during this work and did notreproduce across four subsequent full runs of the same commit. It is unrelated to a change that
edits copy strings, and no issue tracks it yet.
origin/mainmoved while this branch was in progress, gaining a cycle-test change (#696). Ittouches
frontend/app/cycle/only, which this branch does not, andgit merge-treereports a cleanmerge. The branch is not rebased, so the gate output above describes it as it stands.