Repository navigation
Commit 2b30d39
fix(app-shell): the flow designer's Remove node takes its edges, and a new node never inherits a removed one's (objectui#11772) (#11825)
Fixes #11772
Clause-②: no
The objectui half of objectstack#22088 (the server half lands
independently).
## What was wrong, measured on `main` (`9990f9e`)
The card's reproduction, driven through the real `FlowPreview` canvas
and the real `FlowInspector` over a host that merges each patch the way
the Studio Automations pillar does (`{ ...d, ...patch }`), on the draft
Studio creates for a new flow (`start → end`, edge `e1`):
1. the edge's "Insert node here" adds `node_1` (`e1: start → node_1`,
`edge_1: node_1 → end`);
2. the inspector's "Remove node" left both edges in the draft, naming a
node that no longer existed. The saved draft was exactly the card's
second reproduction: nodes `[start, end]`, edges `[start → node_1,
node_1 → end]`;
3. "Add connected node" on `start` minted `node_1` again, so both stale
edges re-attached to the new node.
On `main` the new pin file went red on exactly that: 6 failed, 4 passed
(the 4 were the controls and the dangling-edge rows, which already
worked; see Hypothesis 4).
## What changed
- **Remove node removes the node's edges in the same patch.** One
function, `edgesAfterNodeRemoval`, serves both removal gestures: the
inspector's "Remove node" and the canvas's Delete key. The inspector can
see `draft.edges`, and its host applies patches shallowly
(`MetadataInspectorProps.onPatch`: "Apply a shallow patch to the
draft"), so the edges go into the same patch as the node.
`StudioDesignSurface.tsx` is not touched.
- **A node on a single path is spliced out**: its predecessor is
reconnected to its successor. Single path, read off the edge shapes:
- exactly one edge in (P to X) and exactly one edge out (X to S), not
one self-loop edge;
- the edge in is not a declared back-edge (retargeting a loop's closing
hop would change what the loop re-enters);
- the edge out is plain: no condition, no label, not the default branch,
type `default`. So X made no routing choice of its own that the splice
would drop;
- P and S are other nodes of the flow, and P is not S;
- no edge already joins P to S the same way (`edgeRouteKey`), so the
splice never draws the repeated connection the Problems panel now flags.
The reconnected edge is the edge in, retargeted to S, in the edge in's
place. Its id, condition, label, default flag and type are P's routing
choice, so they stay. A decision branch that led to X now leads to S, at
the same position in the declaration order the engine evaluates branches
in. This is exactly the inverse of the canvas's insert-on-edge, which
splits P to S into the original edge retargeted to X (in place) plus an
appended plain X to S. So inserting a node on an edge and removing it
gives the draft back byte for byte (pinned). Anything else (a branch
node, a join, a node whose edge out is guarded or labelled, the start or
the end) loses its edges and is not reconnected.
- **A new node never takes an id that existed in this editing session.**
The four node minters (the canvas's add, insert-on-edge and revise loop,
and the empty flow's first node) now go through `freshNodeId`:
`uniqueId('node', …)` over the node ids, both endpoints of every edge,
and the ids `FlowPreview` has seen this session. The session is the
`FlowPreview` mount; Studio remounts it when another flow is opened. A
draft with nothing removed and nothing dangling mints exactly what it
minted before (pinned as a control). The shared `uniqueId` contract is
unchanged; its widget, `kv`, `ol`, `sl` and edge callers are untouched.
- **The Problems panel flags a repeated connection**: an edge that joins
the same two nodes the same way as an earlier edge (same type, same
condition read through `conditionText`, same default flag, same label).
Each extra copy is its own error row, targeted at that copy's own edge
key, so clicking the row selects the copy to remove in the edge
inspector. Two edges that join the same nodes differently (two decision
branches to one node, an approval's `approve` and `reject` to one node)
are not flagged. New row `engine.flowProblems.repeatedEdge`, en and zh.
## The PM's mechanism hypotheses, measured
- **H1 (where the pruning lives): option one.** `remove` reaches the
draft through `loc.write(null)` and `onPatch`, and the host merges
shallowly, so the edges fit in the same patch from the inspector. The
canvas's Delete key already pruned edges with its own copy of the
filter; it now calls the same function, which adds the single-path
splice there too.
- **H2 (the id rule's home): at the flow callers, not in `uniqueId`.**
`uniqueId` keeps its lowest-free-suffix contract. Edge ids keep that
plain rule: nothing in a flow refers to an edge by its id (the engine
routes by endpoints, condition and label), so a reused edge id cannot
re-attach anything. The edge minters in `FlowCanvas.tsx` are unchanged.
- **H3 (single path): defined above, with one difference from the
example in the brief.** The brief's example excluded a decision branch
and a fault edge. Here the edge IN may be a decision branch or a fault
edge, because its attributes are the predecessor's routing and survive
the splice unchanged, the same way insert-on-edge keeps them on the
first half. The edge OUT must be plain. A node that is itself a branch
point (several edges out, or a guarded or labelled edge out) is not
reconnected.
- **H4 (Problems panel): half falsified.** An edge whose source or
target is not a node was already flagged, by `validateFlowDraft`
(`engine.flowValidate.edgeSourceMissing` / `edgeTargetMissing`).
Measured on `main`: the card's second-reproduction draft already shows
both rows, in en and zh. No second check was added, which would have
reported each edge twice; the existing rows are now pinned through the
real panel. The repeated pair was not flagged anywhere, and the card's
published flow (three copies of `start → node_1`) read "No problems".
That half is new.
- **H5 (publish): the client does not block publish.**
`buildFlowProblems` feeds only the canvas banner, the badges, the red
ring and stroke (`deriveInvalidElements`), and the Problems panel.
Nothing on the Studio save or publish path reads them, and Studio passes
no server diagnostics to the flow preview. What an author with a
dangling or repeated edge can do: see the error in the banner and the
panel, click the row to select that edge, and remove it with the edge
inspector's Remove. They can still save the draft and publish, until
objectstack#22088 makes the server refuse such a flow. Stored flows are
not migrated.
## Tests
Pins (all through the real components):
- `previews/FlowPreview.removeNode-11772.test.tsx`: the card's
reproduction end to end; the insert-then-remove round trip, byte for
byte; the control (nothing removed: `node_1` and `edge_1` as before); a
removed id with no edges is not minted again in the session; a stored
dangling edge's id is not minted; the Problems panel rows for the
dangling edges and the repeated copies, in en and zh; the skeleton draws
no row.
- `inspectors/FlowNodeInspector.removeEdges-11772.test.tsx`: nine edge
shapes through the real Remove button (single path, decision branch,
branch node, guarded edge out, labelled edge out, back-edge in,
already-joined ends, join, dangling edge in), plus a draft holding a
duplicate node id. For each of the nine, the canvas's Delete key
produces the same edges as the inspector.
- `previews/flow-problems.repeatedEdge-11772.test.ts`: one error per
extra copy with its own edge key, an id-less copy keyed by index, zh,
the condition spellings agree, the four "joined differently" shapes stay
unflagged, and a control.
Ablations, each through `ablation-replace.mjs` from the committed
implementation at `d2b3a07`, restore proven by blob hash equal to HEAD
and an empty `git diff HEAD`:
| Mutation | Result |
|---|---|
| inspector `remove` writes the node only (the old behaviour) | 30 of 38
red, including the card's reproduction and the round trip |
| `freshNodeId` ignores edge endpoints and the session | 3 of 10 red:
the card's reproduction, the session id, the stored dangling id |
| the repeated-connection check removed | 6 of 17 red (en, zh, keys, red
stroke); the "joined differently" controls stay green |
| the single-path splice disabled | 3 of 38 red: the round trip, the
single-path splice, the decision-branch splice |
| the "already joined" guard removed | 1 of 28 red: the no-second-copy
pin |
Each local reading, with the commit it ran at, is in the report comment
on the card. The full `@object-ui/app-shell` suite (1,062 test files)
was not run locally: two attempts exceeded the container's 10-minute
foreground limit on a shared box. Instead the narrowed set ran: every
test file that names a changed module or a changed gesture (51 files).
The full suite is left to CI.
## Files
Everything is on the claim's file surface. `FlowCanvas.tsx` changes
beyond its `uniqueId('node', …)` callers in one place: its Delete key
handler calls the shared removal function, so the two removal gestures
agree. `ProblemsPanel.tsx` needed no change: the new rows render like
every other edge problem.
Nothing is added to the package entry. The changed modules are not
reachable from `dist/index.d.ts`, checked by walking its relative
imports after a build, with `preview-registry.d.ts` as the reachable
positive control. Patch changeset on `@object-ui/app-shell`.
## Acceptance notes
- objectui#11778 (the three add-node entry points, the `create_record`
default type, `notify` in the Node Type select) is not addressed here.
- Out of scope, reported to the seat rather than fixed here: renaming a
node's id in the inspector's ID field leaves the node's edges naming the
old id. The rename patch writes only `nodes`, and the field commits on
every keystroke. Measured with a probe on this branch: the patch carries
no `edges`. After this PR the old id is never minted again and the
Problems panel names the dangling edges, but the renamed node is still
disconnected. Rewriting the endpoints on every keystroke could join a
half-typed id to another node's edges, so this needs its own design.
- Stored flows that already carry dangling or repeated edges are not
rewritten. The Problems panel now names both kinds so an author can
repair them.
---
_Generated by [Claude
Code](https://claude.ai/code/session_01CGZy1BGCjdN5cXqL9cnvB8)_
---------
Co-authored-by: Claude <noreply@anthropic.com>1 parent 282f252 commit 2b30d39
9 files changed
Lines changed: 930 additions & 17 deletions
File tree
- .changeset
- packages/app-shell/src/views/metadata-admin
- inspectors
- previews
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1942 | 1942 | | |
1943 | 1943 | | |
1944 | 1944 | | |
| 1945 | + | |
| 1946 | + | |
| 1947 | + | |
1945 | 1948 | | |
1946 | 1949 | | |
1947 | 1950 | | |
| |||
4913 | 4916 | | |
4914 | 4917 | | |
4915 | 4918 | | |
| 4919 | + | |
| 4920 | + | |
4916 | 4921 | | |
4917 | 4922 | | |
4918 | 4923 | | |
| |||
Lines changed: 264 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
| 116 | + | |
| 117 | + | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
| 121 | + | |
| 122 | + | |
| 123 | + | |
| 124 | + | |
| 125 | + | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
| 132 | + | |
| 133 | + | |
| 134 | + | |
| 135 | + | |
| 136 | + | |
| 137 | + | |
| 138 | + | |
| 139 | + | |
| 140 | + | |
| 141 | + | |
| 142 | + | |
| 143 | + | |
| 144 | + | |
| 145 | + | |
| 146 | + | |
| 147 | + | |
| 148 | + | |
| 149 | + | |
| 150 | + | |
| 151 | + | |
| 152 | + | |
| 153 | + | |
| 154 | + | |
| 155 | + | |
| 156 | + | |
| 157 | + | |
| 158 | + | |
| 159 | + | |
| 160 | + | |
| 161 | + | |
| 162 | + | |
| 163 | + | |
| 164 | + | |
| 165 | + | |
| 166 | + | |
| 167 | + | |
| 168 | + | |
| 169 | + | |
| 170 | + | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
| 178 | + | |
| 179 | + | |
| 180 | + | |
| 181 | + | |
| 182 | + | |
| 183 | + | |
| 184 | + | |
| 185 | + | |
| 186 | + | |
| 187 | + | |
| 188 | + | |
| 189 | + | |
| 190 | + | |
| 191 | + | |
| 192 | + | |
| 193 | + | |
| 194 | + | |
| 195 | + | |
| 196 | + | |
| 197 | + | |
| 198 | + | |
| 199 | + | |
| 200 | + | |
| 201 | + | |
| 202 | + | |
| 203 | + | |
| 204 | + | |
| 205 | + | |
| 206 | + | |
| 207 | + | |
| 208 | + | |
| 209 | + | |
| 210 | + | |
| 211 | + | |
| 212 | + | |
| 213 | + | |
| 214 | + | |
| 215 | + | |
| 216 | + | |
| 217 | + | |
| 218 | + | |
| 219 | + | |
| 220 | + | |
| 221 | + | |
| 222 | + | |
| 223 | + | |
| 224 | + | |
| 225 | + | |
| 226 | + | |
| 227 | + | |
| 228 | + | |
| 229 | + | |
| 230 | + | |
| 231 | + | |
| 232 | + | |
| 233 | + | |
| 234 | + | |
| 235 | + | |
| 236 | + | |
| 237 | + | |
| 238 | + | |
| 239 | + | |
| 240 | + | |
| 241 | + | |
| 242 | + | |
| 243 | + | |
| 244 | + | |
| 245 | + | |
| 246 | + | |
| 247 | + | |
| 248 | + | |
| 249 | + | |
| 250 | + | |
| 251 | + | |
| 252 | + | |
| 253 | + | |
| 254 | + | |
| 255 | + | |
| 256 | + | |
| 257 | + | |
| 258 | + | |
| 259 | + | |
| 260 | + | |
| 261 | + | |
| 262 | + | |
| 263 | + | |
| 264 | + | |
Lines changed: 16 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
64 | 64 | | |
65 | 65 | | |
66 | 66 | | |
| 67 | + | |
67 | 68 | | |
68 | 69 | | |
69 | 70 | | |
| |||
421 | 422 | | |
422 | 423 | | |
423 | 424 | | |
| 425 | + | |
| 426 | + | |
| 427 | + | |
| 428 | + | |
| 429 | + | |
| 430 | + | |
424 | 431 | | |
425 | 432 | | |
| 433 | + | |
| 434 | + | |
| 435 | + | |
| 436 | + | |
| 437 | + | |
| 438 | + | |
| 439 | + | |
| 440 | + | |
| 441 | + | |
426 | 442 | | |
427 | 443 | | |
428 | 444 | | |
| |||
0 commit comments