fix(properties): stop a boundary port type edit from renaming the port - #22
Open
endrix wants to merge 4 commits into
Open
fix(properties): stop a boundary port type edit from renaming the port#22endrix wants to merge 4 commits into
endrix wants to merge 4 commits into
Conversation
Investigating why a boundary port's type is read-only turned up something
worse than a missing feature: the one place that DID offer the edit corrupted
the source.
A label edit travels as the protocol `ApplyLabelEditOperation`, and the only
handler registered for it turns every label edit into a rename of the nearest
ancestor carrying an entity name. For a boundary port's type label that
ancestor is the boundary node — whose entity name is the PORT's name — so the
edit went out as `renameNode { old: <port name>, new: <the type typed> }`.
Changing a type renamed the port to its own type. An entity node's type
subtitle had the same shape. Both the property panel's "Change Port Type" and
a double-click on the canvas label reached it.
The handler now allow-lists label types rather than accepting all of them:
only a name may rename, and a label type added later is refused until someone
decides a rename is what editing it means. Elements that are not typed labels
pass through unchanged, so the MCP path that addresses a node by its label id
still works.
With the write path inert rather than destructive, the two affordances that
promise a type edit are withdrawn — an editor that silently discards what you
type is still worth not offering. Making it work needs a sidecar op that
addresses a boundary port by `workflow`, the way `createPort` already does,
rather than by an owning `entity`, which is what `updatePortType` requires.
That is a change to both sidecars and is specced separately.
Separately, boundary ports become navigable. The graph schema gives every port
a `source: { file, line }` and an entity's port has always carried it into the
model, which is what the go-to-source menu reads; `createBoundaryNode` took
`name` and `type` off the same object and dropped `source`. It now emits the
same metadata triple the entity ports do, so both travel one navigation path.
Note this reaches the PORT's declaration, not its type's definition — that
needs a schema field which does not exist yet.
The parity baseline records the one intended registration change:
`label:boundary:type` moves to the non-editable `WorkflowLabel`.
Writes down what the sidecars have to grow before a boundary port's type can be edited, before a product or variant can be shown, and before a type's definition can be navigated to. All three are blocked outside the platform, and both sidecars have to implement it identically or the platform behaves differently per product — so it is a spec, not a ticket. Two parts, each independently shippable and separately gated: Part A is one operation, `updateBoundaryPortType`, addressed by `workflow` and `direction` the way `createPort` and `deletePort` already address boundary ports. Proposed as a new op rather than a `workflow` variant of the existing `updatePortType`, whose `entity` is required: overloading it would make a sidecar that has not been updated read a boundary edit as an entity edit, silently, against the wrong declaration. Part B makes a port's type structured — kind, members, and a definition site per member — as an OPTIONAL field beside the existing string, which stays and remains what gets displayed. The platform must never re-render a type from its structure; that would drift from the language's own formatting and differ between products. Expansion depth is the sidecar's call, marked with `truncated`, so a recursive type cannot become an unbounded payload. Both ride the existing `getCapabilities` negotiation rather than a version bump, so neither sidecar blocks the other and neither blocks the platform. Records four open questions the sidecar authors have to answer — network addressability, default expansion depth, whether one `kind` vocabulary spans both type systems, and whether entity ports get the same treatment.
Making the type label non-editable by pointing it at the generic `WorkflowLabel` moved it: it rendered small, in the node's top-left corner, outside the rounded box. `WorkflowLabelView` positions boundary labels itself, relative to the parent node and centred on the parent's width. That only works while the labels are invisible to the layout engine, which is why `BoundaryEditableLabel` REPLACES sprotty's default label features rather than adding to them — those defaults include `boundsFeature`, `alignFeature` and `layoutableChildFeature`. `WorkflowLabel` declares no features of its own, so pointing at it handed the label back to the layout engine, which duly laid it out. So the non-editable variant is its own class whose empty feature list is the entire point, rather than a reach for the nearest label class that happened to lack edit support. The trap here is inheritance, so that is what the tests assert: each boundary label declares its OWN feature list, and its EFFECTIVE list — resolved the way sprotty resolves it, falling back to sprotty's defaults when a class declares none — carries no layout feature. Modelling the fallback is what makes the second assertion real: reading only the own list, it passed while the class inherited every layout feature there is. The real sprotty defaults are read past the vitest stub, whose empty ones would have made the whole file vacuous.
The boundary navigation added earlier is inert: it read `port.source`, the typed field the schema declares on a port, and neither product populates it. That was the wrong field to reach for. Entity nodes do not use it either — every one of them resolves navigation from `node.meta.source`, and `node.meta.referencedSource` for the definition. A boundary node is a PyGraphNode like any other and has the same `meta`, so the convention already in use across the graph was available and went unread. Both are now checked, preferring meta so a boundary node behaves like the nodes beside it, falling back to the typed port field so a product that fills in what the schema declares is also served. Requiring neither specifically matters because the platform cannot make either product change. Whether this lights up still depends on a product emitting one of them for a boundary node, which as far as can be told neither does yet. So the proposal gains a Part C: populate `meta.source` on a boundary node with the port's declaration site, exactly as entity nodes already do. It needs no new op and no schema change — `meta` is an open bag and the field name is the one in use — which makes it much the cheapest of the three parts. The proposal's "already done" section is corrected accordingly. It claimed boundary ports were navigable; they are navigable only once something supplies a location, and saying otherwise would have sent someone looking for a feature that cannot appear.
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
1 participant
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.
Investigating why a boundary port's type is read-only turned up something worse than a missing feature: the one place that did offer the edit corrupted the source.
The bug
A label edit travels as the protocol
ApplyLabelEditOperation. The only handler registered for it turns every label edit into a rename of the nearest ancestor carrying an entity name. For a boundary port's type label, that ancestor is the boundary node — whose entity name is the port's name. So the edit went out as:Changing a type renamed the port to its own type. An entity node's type subtitle had the same shape. Both the property panel's "Change Port Type" and a double-click on the canvas label reached it.
Fix: the handler now allow-lists label types instead of accepting all of them. Only a name may rename, and a label type added later is refused until someone decides a rename is what editing it means — the polarity matters more than the specific list. Elements that are not typed labels pass through unchanged, so the MCP path that addresses a node by its label id still works.
With the write path inert rather than destructive, the two affordances promising a type edit are withdrawn. An editor that silently discards what you type is still worth not offering.
Boundary ports become navigable
Separate, and free. The graph schema gives every port a
source: { file, line }, and an entity's port has always carried it into the model — that is what the go-to-source menu reads.createBoundaryNodetooknameandtypeoff the very same port object and droppedsourceon the floor, so a network's own inputs and outputs were the one kind of port you could not navigate from. Nothing reported it because nothing offered it.It now emits the same metadata triple the entity ports do, so both travel one navigation path rather than growing a second. Note this reaches the port's declaration, not its type's definition — that needs a schema field that does not exist.
Why the type still is not editable
Not a UI decision. Port operations split by ownership and address the two cases differently:
createPort/deletePort(boundary)workflow+directionrenamePort/updatePortTypeentity+portDirectionA boundary port belongs to the network, not to any entity, so
updatePortTypecannot name it.createPortalready writes a boundary port's type, so the capability is there — only the update path is missing, and that is a change to both sidecars.docs/proposals/structured-port-types.mdspecs it, along with the schema work that the product/variant viewer and go-to-type-definition need. Both parts ride the existinggetCapabilitiesnegotiation rather than a version bump, so neither sidecar blocks the other and neither blocks the platform. It ends with four open questions for the sidecar authors.Testing
Four new tests pin the rename bug, mutation-checked: the three refusal cases fail without the guard while the "still renames from a name label" control passes either way, so it is not asserting that nothing happens. Four more cover boundary navigation, including the two cases that must emit nothing — a navigation entry pointing nowhere would offer the menu item and then fail on click.
The container-parity oracle caught the one intended registration change (
label:boundary:type→ the non-editableWorkflowLabel); baseline regenerated with exactly that and nothing else.Verification
diagram-server 216, diagram-client 87, sidecar-toolkit 184, typecheck 5/5, bundles build.
Two suites still red on this branch — the
extension-corewebview snapshot and neutrality Gate 1 — both fixed by #20, which is not in this branch's ancestry. Neither file is touched here.