diff --git a/docs/proposals/structured-port-types.md b/docs/proposals/structured-port-types.md new file mode 100644 index 0000000..1b63c3f --- /dev/null +++ b/docs/proposals/structured-port-types.md @@ -0,0 +1,198 @@ +# Proposal: structured port types + +**Status:** proposal — nothing here is implemented in any sidecar. +**Affects:** the graph export schema and the operation vocabulary, so **every** +sidecar has to implement it identically or the platform behaves differently per +product. + +## Why + +Three things users ask for are blocked in the same place, and none of them can +be fixed in the platform: + +1. **Editing a boundary port's type.** The platform has no operation that can + address a boundary port for an update. +2. **A viewer for composite types** (products and variants) in the property + panel. +3. **Go to the definition of a type**, from a port. + +A fourth turned up while investigating them: a boundary port has no source +location at all, so "Go to Source" does not even reach the port, let alone its +type. That one is much smaller — see Part C. + +(2) and (3) are blocked by one line of the export schema: + +```ts +export type PyGraphPort = { + id: string; + name: string; + direction: 'in' | 'out'; + type?: string; // ← the whole type, as text + role?: string; + source?: { file?: string; line?: number } | null; +}; +``` + +A type is opaque text. There is no structure to render and no location to +navigate to, and the platform cannot derive either: it never parses source — +that is the sidecar's entire job. + +`source` is the **port's** declaration site. It is not where the type is +defined, and it is already used for "go to source" on the port itself. + +## Part A — editing a boundary port's type + +### The gap + +Port operations already split by ownership, and address the two cases +differently: + +| | addressed by | direction key | +| --- | --- | --- | +| `createPort` (boundary) | `workflow` | `direction` | +| `deletePort` (boundary) | `workflow` | `direction` | +| `createEntityPort` | `entityType` | `portDirection` | +| `deleteEntityPort` | `entityType` | `portDirection` | +| `renamePort` | `entity` | `portDirection` | +| `updatePortType` | `entity` | `portDirection` | + +A boundary port belongs to the network, not to any entity, so `updatePortType` +cannot name it. `createPort` already writes a boundary port's type — the +capability exists in the sidecar; only the update path is missing. + +### Proposed operation + +``` +updateBoundaryPortType + args: { workflow: string, direction: 'input' | 'output', + portName: string, newValue: string } +``` + +Addressed the way the other boundary port ops are. A separate op rather than a +`workflow` variant of `updatePortType`, because that op's `entity` is required +today and overloading it would make an un-updated sidecar interpret a boundary +edit as an entity edit — silently, against the wrong declaration. + +Renaming a boundary port needs nothing new: it already rides `renameNode`. + +## Part B — structured types + +### Proposed schema + +Additive and optional. A sidecar that has not implemented it emits nothing new, +and the platform behaves exactly as it does today. + +```ts +export type PyTypeRef = { + /** Rendered form, in the language's own formatting. ALWAYS present. */ + text: string; + kind?: 'primitive' | 'alias' | 'product' | 'variant' | 'list' | 'unknown'; + /** The declared name, when it has one. */ + name?: string; + /** Where the type is DEFINED. Not the port's declaration site. */ + source?: { file?: string; line?: number } | null; + /** Fields of a product; cases of a variant. Omitted for other kinds. */ + members?: Array<{ + name: string; + type?: PyTypeRef; + source?: { file?: string; line?: number } | null; + }>; + /** Set when expansion stopped here rather than bottoming out. */ + truncated?: boolean; +}; + +export type PyGraphPort = { + // ...unchanged... + type?: string; // unchanged; stays the display fallback + typeRef?: PyTypeRef; // NEW, optional +}; +``` + +### Why it is shaped this way + +**`text` is mandatory and `type` stays.** The platform must never re-render a +type from its structure — it would drift from how the language actually writes +it, and differ between products. The sidecar formats; the platform displays. +Keeping `type` means the change cannot regress an existing diagram. + +**Expansion is bounded by the sidecar, not the platform.** A recursive type +would otherwise be an unbounded payload on every graph export. The sidecar +expands to whatever depth it judges reasonable and sets `truncated: true` where +it stopped; the platform shows an affordance that *navigates* rather than +expands, so depth is never the platform's problem. + +**`kind` is open and `'unknown'` is legal.** A type the sidecar cannot resolve +must still round-trip its `text`. Partial information is expected — a graph +export is already allowed to be `partial`. + +**`source` on each member.** Navigating to one field of a product is the useful +case; a viewer that can only reach the whole type is much less so. + +### What the platform would do with it + +| field | used for | +| --- | --- | +| `text` / `type` | the chip shown today — unchanged | +| `kind` + `members` | the property-panel viewer for products and variants | +| `source` | "Go to definition" on the type, and on each member | +| `truncated` | render "…" as a navigation affordance, not an expander | + +Absent `typeRef`, the panel renders exactly what it renders now. + +## Part C — a source location for boundary ports + +Smaller than the other two, and possibly already satisfied. + +"Go to Source" resolves from navigation metadata on the element. Every entity +node gets it from `node.meta.source`; an entity's ports get it from the typed +`port.source`. A boundary node read neither — it took `name` and `type` off its +port and nothing else — so a network's own inputs and outputs were the one kind +of port with no navigation at all. + +The platform now reads **both**, preferring `node.meta.source` (so a boundary +node behaves like the nodes beside it) and falling back to `port.source`. It +requires neither specifically, because the platform cannot make a product +change. But it does require **one of them to be populated**, and as of writing +neither product appears to emit either for a boundary node — the feature is +wired and inert. + +So: for a boundary node (`kind: 'wf-input'` / `'wf-output'`), populate +`meta.source = { file, line }` with the port's declaration site, exactly as +entity nodes already do. Nothing else is needed; no new op, no schema change — +`meta` is already an open bag and the field name is the one in use. + +## Capability gating + +Both parts should be gated through the existing negotiation rather than +version-bumped: `getCapabilities` returns `ops: string[]`, and the platform +already has `supportsOp(uri, kind)`. + +- **Part A:** the panel offers the type edit only when + `updateBoundaryPortType` is advertised. Otherwise the field stays read-only, + which is what it is today. +- **Part B:** needs no gate. `typeRef` is optional and its absence is the + current behaviour. + +This means neither sidecar blocks the other, and neither blocks the platform. + +## Open questions for the sidecar authors + +1. Is a network's interface addressable by `workflow` name alone, or is a file + with several networks ambiguous in a way `createPort` gets away with today + only because it appends? +2. What is a sensible default expansion depth — is one level enough to be + useful for the common product type? +3. Do the two products' type systems agree closely enough for one `kind` + vocabulary, or does it need a product-specific escape hatch? +4. Should `typeRef` also be emitted for entity ports? The schema change is on + `PyGraphPort`, so it comes for free — but only if the resolver is available + on that path too. + +## What is already done in the platform + +- A boundary port type edit no longer **renames the port**. Every label edit was + routed to a rename of the nearest entity; for a type label that resolved to + the port itself. The handler now allow-lists name labels. This needs no + sidecar work and is the one change here with immediate effect. +- Boundary nodes emit navigation metadata when a source location is available — + see Part C for why that is currently never. diff --git a/packages/diagram-client/src/model.ts b/packages/diagram-client/src/model.ts index dc8609a..4d1d75b 100644 --- a/packages/diagram-client/src/model.ts +++ b/packages/diagram-client/src/model.ts @@ -446,6 +446,27 @@ export class BoundaryEditableLabel extends WorkflowLabel { editControlDimension = { width: 100, height: 20 }; } +/** + * A boundary label that cannot be edited — the type, which has no write path yet. + * + * The empty feature list is the whole point of this class, and the reason it is + * not just `WorkflowLabel`. Sprotty's default label features include + * `boundsFeature`, `alignFeature` and `layoutableChildFeature`, and a boundary + * label must have NONE of them: `WorkflowLabelView` positions these labels + * itself, relative to the parent node, centring the text on the node's own + * width. Give the label layout features back and the node's layout engine + * claims it instead — it lands in the top-left corner, small, outside the + * rounded box. That is exactly what happened when this was first made + * non-editable by pointing it at `WorkflowLabel`. + * + * So: same (absent) features as {@link BoundaryEditableLabel}, minus the two + * edit ones. Not selectable either, for the reason described above it — a + * selectable label swallows the drag that should move the port. + */ +export class BoundaryLabel extends WorkflowLabel { + static override readonly DEFAULT_FEATURES = []; +} + /** * CAL Compartment for grouping elements */ diff --git a/packages/diagram-client/src/property-panel.ts b/packages/diagram-client/src/property-panel.ts index 5725592..c750a99 100644 --- a/packages/diagram-client/src/property-panel.ts +++ b/packages/diagram-client/src/property-panel.ts @@ -4454,20 +4454,17 @@ export class PropertyPanel implements ISelectionListener, IGModelRootListener { } as any); }); - const typeField = document.createElement('button'); - typeField.type = 'button'; + // Read-only, deliberately. This offered "Double-click to change + // type", and the edit went out as a label edit — whose only handler + // renames the nearest entity, i.e. the port. Changing a type renamed + // the port to that type. The handler now refuses non-name labels, so + // the edit would be inert instead; either way there is nothing to + // offer until a sidecar op can address a boundary port by `workflow` + // rather than by an owning `entity`. + const typeField = document.createElement('span'); typeField.className = 'port-field port-type'; - typeField.title = 'Double-click to change type'; + typeField.title = `Type: ${portType}`; typeField.textContent = portType; - typeField.addEventListener('dblclick', (e) => { - e.stopPropagation(); - void this.actionDispatcher.dispatch({ - kind: WorkflowPromptLabelEditAction.KIND, - labelId: `${node.id}_label_type`, - title: 'Change Port Type', - value: portType - } as any); - }); const removeBtn = document.createElement('button'); removeBtn.type = 'button'; diff --git a/packages/diagram-client/src/stock-views.module.ts b/packages/diagram-client/src/stock-views.module.ts index 2bc8809..bcfc6b3 100644 --- a/packages/diagram-client/src/stock-views.module.ts +++ b/packages/diagram-client/src/stock-views.module.ts @@ -45,6 +45,7 @@ import { WorkflowEdge, WorkflowLabel, BoundaryEditableLabel, + BoundaryLabel, HeaderCompartment, PortsCompartment } from './model'; @@ -117,9 +118,24 @@ export const workflowViewsModule = new ContainerModule((bind, unbind, isBound, r configureModelElement(context, WorkflowDiagramTypes.LABEL_TYPE, WorkflowLabel, GLabelView); configureModelElement(context, WorkflowDiagramTypes.LABEL_BADGE, WorkflowLabel, GLabelView); - // Boundary labels are directly editable (name + type) + // The boundary NAME is directly editable; the type is not. + // + // A label edit travels as the protocol `ApplyLabelEditOperation`, and the + // only handler for that renames the nearest entity — which for a boundary + // type label is the port itself. Editing the type therefore renamed the port + // to whatever type was typed. The handler now refuses anything that is not a + // name label, so the edit is merely inert rather than destructive; an editor + // that silently discards what you type is still worth not offering. + // + // Making the type editable for real needs a sidecar op that addresses a + // boundary port by `workflow` (the way `createPort` already does) rather + // than by owning `entity`, which is what `updatePortType` requires today. + // + // `BoundaryLabel`, NOT the generic `WorkflowLabel`: boundary labels must + // carry no layout features, or the node's layout engine positions them + // instead of the view and the type lands in the top-left corner. configureModelElement(context, WorkflowDiagramTypes.LABEL_BOUNDARY_NAME, BoundaryEditableLabel, WorkflowLabelView); - configureModelElement(context, WorkflowDiagramTypes.LABEL_BOUNDARY_TYPE, BoundaryEditableLabel, WorkflowLabelView); + configureModelElement(context, WorkflowDiagramTypes.LABEL_BOUNDARY_TYPE, BoundaryLabel, WorkflowLabelView); // Port labels configureModelElement(context, WorkflowDiagramTypes.LABEL_PORT, WorkflowLabel, WorkflowLabelView); diff --git a/packages/diagram-client/test/boundary-label-features.test.ts b/packages/diagram-client/test/boundary-label-features.test.ts new file mode 100644 index 0000000..4b8ba6e --- /dev/null +++ b/packages/diagram-client/test/boundary-label-features.test.ts @@ -0,0 +1,80 @@ +/** + * Boundary labels must not participate in layout. + * + * `WorkflowLabelView` positions a boundary node's name and type 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 — so both boundary label classes + * REPLACE sprotty's default label features, which include `boundsFeature`, + * `alignFeature` and `layoutableChildFeature`. + * + * This has already been broken once. Making the type label non-editable by + * pointing it at the generic `WorkflowLabel` looked like a pure capability + * change, but `WorkflowLabel` declares no features of its own and therefore + * inherits sprotty's — so the layout engine claimed the label and parked it in + * the node's top-left corner, small and outside the rounded box. Nothing failed; + * it only looked wrong. + * + * The trap is inheritance, so that is what these assert: each boundary label + * declares its OWN feature list, and that list contains no layout feature. The + * real sprotty defaults are read through `createRequire` — the vitest alias + * points `@eclipse-glsp/sprotty` at a stub, and asserting against the stub's + * (empty) defaults would pass no matter what. + */ +import { describe, expect, it } from 'vitest'; +import { createRequire } from 'node:module'; +import { BoundaryEditableLabel, BoundaryLabel, WorkflowLabel } from '../src/model'; + +const { SLabelImpl } = createRequire(import.meta.url)('sprotty/lib/graph/sgraph'); + +/** Feature symbols compared by description: the stub's and sprotty's differ by identity. */ +const describeAll = (features: readonly symbol[] | undefined): string[] => + (features ?? []).map(f => String(f)); + +const LAYOUT_FEATURES = ['Symbol(boundsFeature)', 'Symbol(alignFeature)', 'Symbol(layoutableChildFeature)']; + +describe('boundary label features', () => { + /** If sprotty ever stopped defaulting labels into layout, this whole concern would be moot. */ + it("sprotty's own label defaults are the hazard", () => { + const defaults = describeAll(SLabelImpl.DEFAULT_FEATURES); + + for (const feature of LAYOUT_FEATURES) { + expect(defaults, `sprotty labels no longer default to ${feature}`).toContain(feature); + } + }); + + it.each([ + ['BoundaryLabel', BoundaryLabel], + ['BoundaryEditableLabel', BoundaryEditableLabel] + ])('%s declares its own features rather than inheriting', (name, cls) => { + // The inheritance trap: a class that does not declare DEFAULT_FEATURES + // silently takes sprotty's, layout features and all. + expect( + Object.prototype.hasOwnProperty.call(cls, 'DEFAULT_FEATURES'), + `${name} must declare DEFAULT_FEATURES, not inherit them` + ).toBe(true); + }); + + it.each([ + ['BoundaryLabel', BoundaryLabel], + ['BoundaryEditableLabel', BoundaryEditableLabel] + ])('%s carries no layout feature', (name, cls) => { + // EFFECTIVE features, the way sprotty resolves them: a class without its + // own list gets sprotty's. Reading only the own list would make this + // pass for a class that inherits every layout feature there is — which + // is precisely the regression, so it has to be modelled here. + const own = (cls as { DEFAULT_FEATURES?: symbol[] }).DEFAULT_FEATURES; + const effective = describeAll(own ?? SLabelImpl.DEFAULT_FEATURES); + + for (const feature of LAYOUT_FEATURES) { + expect(effective, `${name} would be positioned by the layout engine`).not.toContain(feature); + } + }); + + /** + * The class the regression reached for. Pinning that it is NOT safe here is + * what makes the distinction visible to the next person. + */ + it('the generic WorkflowLabel is not a valid boundary label', () => { + expect(Object.prototype.hasOwnProperty.call(WorkflowLabel, 'DEFAULT_FEATURES')).toBe(false); + }); +}); diff --git a/packages/diagram-client/test/container-parity.test.ts b/packages/diagram-client/test/container-parity.test.ts index f0f5139..98576a7 100644 --- a/packages/diagram-client/test/container-parity.test.ts +++ b/packages/diagram-client/test/container-parity.test.ts @@ -15,8 +15,10 @@ * * The baseline is no longer a pure capture of that commit: features added since * the split are appended to it deliberately, and the fixture currently carries - * two — `IEdgeRouter -> LibavoidEdgeRouter` (the client-side live routing tier) - * and `ChangeBoundsTool -> WorkflowChangeBoundsTool` (the mouse-drag threshold). + * two additions — `IEdgeRouter -> LibavoidEdgeRouter` (the client-side live + * routing tier) and `ChangeBoundsTool -> WorkflowChangeBoundsTool` (the + * mouse-drag threshold) — plus one change: `label:boundary:type` is bound to + * the non-editable `BoundaryLabel`, because editing it renamed the port. * The oracle still does its job: it fails on any binding this composition gains * or loses, and updating the fixture is the deliberate act of accepting one. * Regenerate it only after confirming the diff contains exactly the bindings the diff --git a/packages/diagram-client/test/fixtures/container-parity.baseline.json b/packages/diagram-client/test/fixtures/container-parity.baseline.json index 871af7c..fd1b55b 100644 --- a/packages/diagram-client/test/fixtures/container-parity.baseline.json +++ b/packages/diagram-client/test/fixtures/container-parity.baseline.json @@ -191,18 +191,18 @@ "typeId": "label:boundary:name", "view": "WorkflowLabelView" }, - { - "model": "BoundaryEditableLabel", - "op": "modelElement", - "typeId": "label:boundary:type", - "view": "WorkflowLabelView" - }, { "model": "BoundaryInputNode", "op": "modelElement", "typeId": "node:boundary:input", "view": "BoundaryInputNodeView" }, + { + "model": "BoundaryLabel", + "op": "modelElement", + "typeId": "label:boundary:type", + "view": "WorkflowLabelView" + }, { "model": "BoundaryOutputNode", "op": "modelElement", diff --git a/packages/diagram-server/src/model/graph-gmodel-source.ts b/packages/diagram-server/src/model/graph-gmodel-source.ts index 428da14..c1e4ad8 100644 --- a/packages/diagram-server/src/model/graph-gmodel-source.ts +++ b/packages/diagram-server/src/model/graph-gmodel-source.ts @@ -1046,6 +1046,42 @@ export class GraphGModelSource { }; const typeText = port?.type ?? ''; + // Where this port is declared, so "Go to Source" works on a boundary node + // the way it already does on every other node. It was never wired at + // all: the boundary node took `name` and `type` off its port and read + // nothing else. + // + // Two places carry it, and BOTH are checked because the products differ + // on which they fill in. `node.meta.source` is the established one — + // every entity node above resolves its navigation from exactly that, so + // a sidecar already emitting node source needs no change at all. + // `port.source` is the typed field the schema declares on a port, used + // by an entity's ports in `mkPort`. Preferring meta means a boundary node + // behaves like its neighbours; falling back to the port keeps faith with + // the declared schema. + // + // NOTE this reaches the PORT's declaration, not its type's definition — + // navigating to where a type is defined needs a field the schema does + // not have. See docs/proposals/structured-port-types.md. + const boundaryMetaSource = node.meta?.['source'] as { file?: string; line?: number } | undefined; + const boundarySource = boundaryMetaSource?.file && boundaryMetaSource?.line + ? boundaryMetaSource + : port?.source ?? undefined; + const boundarySourceFile = normalizeNavigationFileUri(boundarySource?.file); + const boundarySourceLine = boundarySource?.line; + const boundaryNavigation = boundarySourceFile && boundarySourceLine + ? (() => { + const at = { + start: { line: Math.max(0, boundarySourceLine - 1), character: 0 }, + end: { line: Math.max(0, boundarySourceLine - 1), character: 0 } + }; + return { + [WorkflowDiagramMetadata.SOURCE_RANGE]: at, + [WorkflowDiagramMetadata.REFERENCED_SOURCE_RANGE]: at, + [WorkflowDiagramMetadata.REFERENCED_URI]: boundarySourceFile + }; + })() + : {}; return { id: stableNodeId, type: isInput ? WorkflowDiagramTypes.NODE_BOUNDARY_INPUT : WorkflowDiagramTypes.NODE_BOUNDARY_OUTPUT, @@ -1080,7 +1116,8 @@ export class GraphGModelSource { [WorkflowDiagramMetadata.PORT_NAME]: port?.name ?? '', [WorkflowDiagramMetadata.PORT_TYPE]: typeText, 'wf:boundaryKind': isInput ? 'input' : 'output', - 'wf:portName': port?.name ?? '' + 'wf:portName': port?.name ?? '', + ...boundaryNavigation } }; } diff --git a/packages/diagram-server/test/boundary-port-navigation.test.ts b/packages/diagram-server/test/boundary-port-navigation.test.ts new file mode 100644 index 0000000..cbd330e --- /dev/null +++ b/packages/diagram-server/test/boundary-port-navigation.test.ts @@ -0,0 +1,134 @@ +/** + * "Go to source" on a boundary port. + * + * The graph schema gives every port a `source: { file, line }`, and an entity's + * port has always carried it into the model (see `mkPort`) — that is what the + * go-to-source context menu reads. `createBoundaryNode` took the `name` and the + * `type` off the very same port object and dropped `source` on the floor, so a + * network's own inputs and outputs were the one kind of port you could not + * navigate from. Nothing reported it as broken because nothing offered it. + * + * The metadata is deliberately the same triple the entity ports emit, so both + * kinds of port travel the one navigation path rather than growing a second. + */ +import { describe, expect, it } from 'vitest'; +import { URI } from 'vscode-uri'; +import * as path from 'node:path'; +import { WorkflowDiagramMetadata } from '@dialogram/shared'; +import { GraphGModelSource, type PyGraphDocument } from '../src/model/graph-gmodel-source'; + +type At = { file?: string; line?: number }; + +/** + * A one-node graph whose only content is a boundary input port. + * + * `meta` and `port` are separate because the two places a product can put the + * location are the whole point: `node.meta.source` is what every other node + * resolves navigation from, `port.source` is what the schema declares on a port. + */ +function docWithBoundaryInput(at?: { meta?: At; port?: At }): PyGraphDocument { + return { + version: '1', + graph: { + id: 'root', + nodes: [ + { + id: 'wf:input:Alloc', + kind: 'wf-input', + label: 'Alloc', + scope: 'root', + meta: at?.meta ? { source: at.meta } : undefined, + ports: [ + { + id: 'port:wf:input:Alloc:out', + name: 'Alloc', + direction: 'out', + type: 'RobAlloc', + source: at?.port + } + ] + } + ], + edges: [] + } + }; +} + +/** The single boundary node in a transformed graph. */ +function boundaryNodeOf(doc: PyGraphDocument): any { + const result = new GraphGModelSource().transform(doc); + const node = result.graph.children?.find( + (child: any) => child.args?.['wf:boundaryKind'] !== undefined + ); + expect(node, 'no boundary node in the transformed graph').toBeDefined(); + return node; +} + +describe('boundary port navigation metadata', () => { + it('resolves the declaration site from node meta, as every other node does', () => { + const node = boundaryNodeOf(docWithBoundaryInput({ meta: { file: '/w/pipeline.py', line: 12 } })); + + // Line numbers arrive 1-based from the sidecar and are stored 0-based, + // matching what the entity ports emit. + const at = { start: { line: 11, character: 0 }, end: { line: 11, character: 0 } }; + expect(node.args[WorkflowDiagramMetadata.SOURCE_RANGE]).toEqual(at); + expect(node.args[WorkflowDiagramMetadata.REFERENCED_SOURCE_RANGE]).toEqual(at); + expect(node.args[WorkflowDiagramMetadata.REFERENCED_URI]).toBe( + URI.file(path.resolve('/w/pipeline.py')).toString() + ); + }); + + it('leaves the port identity alone', () => { + const node = boundaryNodeOf(docWithBoundaryInput({ meta: { file: '/w/pipeline.py', line: 12 } })); + + expect(node.args[WorkflowDiagramMetadata.PORT_NAME]).toBe('Alloc'); + expect(node.args[WorkflowDiagramMetadata.PORT_TYPE]).toBe('RobAlloc'); + expect(node.args['wf:boundaryKind']).toBe('input'); + }); + + /** + * A port whose source the sidecar could not resolve must not produce a + * navigation target at all — an entry pointing nowhere would offer the menu + * item and then fail on click. + */ + it('emits nothing navigable when the sidecar gave no source', () => { + const node = boundaryNodeOf(docWithBoundaryInput(undefined)); + + expect(node.args[WorkflowDiagramMetadata.SOURCE_RANGE]).toBeUndefined(); + expect(node.args[WorkflowDiagramMetadata.REFERENCED_URI]).toBeUndefined(); + }); + + it('emits nothing navigable when the source has a file but no line', () => { + const node = boundaryNodeOf(docWithBoundaryInput({ meta: { file: '/w/pipeline.py' } })); + + expect(node.args[WorkflowDiagramMetadata.REFERENCED_URI]).toBeUndefined(); + }); + + /** + * The schema declares `source` on the port, so a product that fills that in + * instead of node meta must work too — the point is to require neither + * specifically, since the platform cannot make either product change. + */ + it('falls back to the port when node meta carries no source', () => { + const node = boundaryNodeOf(docWithBoundaryInput({ port: { file: '/w/pipeline.py', line: 5 } })); + + expect(node.args[WorkflowDiagramMetadata.REFERENCED_URI]).toBe( + URI.file(path.resolve('/w/pipeline.py')).toString() + ); + expect(node.args[WorkflowDiagramMetadata.SOURCE_RANGE]).toEqual({ + start: { line: 4, character: 0 }, + end: { line: 4, character: 0 } + }); + }); + + /** Node meta wins, so a boundary node navigates like the nodes beside it. */ + it('prefers node meta over the port when both are present', () => { + const node = boundaryNodeOf( + docWithBoundaryInput({ meta: { file: '/w/net.py', line: 12 }, port: { file: '/w/other.py', line: 5 } }) + ); + + expect(node.args[WorkflowDiagramMetadata.REFERENCED_URI]).toBe( + URI.file(path.resolve('/w/net.py')).toString() + ); + }); +}); diff --git a/packages/sidecar-toolkit/src/server/operations/apply-label-edit-rename-handler.ts b/packages/sidecar-toolkit/src/server/operations/apply-label-edit-rename-handler.ts index b42827d..0fdd90d 100644 --- a/packages/sidecar-toolkit/src/server/operations/apply-label-edit-rename-handler.ts +++ b/packages/sidecar-toolkit/src/server/operations/apply-label-edit-rename-handler.ts @@ -1,6 +1,6 @@ import { Action, ApplyLabelEditOperation, Command, GModelElement, ModelState, OperationHandler } from '@eclipse-glsp/server'; import { inject, injectable } from 'inversify'; -import { WorkflowDiagramMetadata } from '@dialogram/shared'; +import { WorkflowDiagramMetadata, WorkflowDiagramTypes } from '@dialogram/shared'; import { RenameEntityOperation, RenameEntityOperationHandler } from './rename-entity-handler'; @@ -58,10 +58,14 @@ export class ApplyLabelEditRenameHandler extends OperationHandler { /** * Walk up from the edited label to the nearest ancestor whose `args` carry the entity name — * that is the node {@link RenameEntityOperationHandler} renames. Returns its id, or `undefined` - * when the label belongs to no entity node (e.g. a port/decorator label). + * when the label belongs to no entity node (e.g. a port/decorator label), or when the label is + * not a NAME (see {@link isRenameableLabel}). */ private resolveEntityElementId(labelId: string): string | undefined { let element: GModelElement | undefined = this.modelState.index.find(labelId); + if (!this.isRenameableLabel(element)) { + return undefined; + } while (element) { const name = element.args?.[WorkflowDiagramMetadata.ENTITY_NAME]; if (typeof name === 'string' && name.trim() !== '') { @@ -71,4 +75,29 @@ export class ApplyLabelEditRenameHandler extends OperationHandler { } return undefined; } + + /** + * Whether the edited element is something a rename may act on. + * + * This handler turns EVERY label edit into a rename of the nearest ancestor carrying an entity + * name, which is right for a name label and silently destructive for any other. A boundary + * port's TYPE label is the case that bit: it carries only a port name of its own, so the walk + * lands on the boundary node — whose entity name is the PORT's name — and the edit went out as + * `renameNode { old: , new: }`. Editing a type renamed the + * port to that type. An entity node's type subtitle had the same shape. + * + * So labels are allow-listed by type, not deny-listed: a label type added later is refused + * until someone deliberately decides a rename is what editing it means. + * + * Anything that is not a typed label passes through unchanged — the MCP caller addresses a node + * by its label id, but a synthetic or untyped element must keep the original walk-up rather than + * be silently dropped. + */ + private isRenameableLabel(element: GModelElement | undefined): boolean { + const type = (element as { type?: unknown } | undefined)?.type; + if (typeof type !== 'string' || !type.startsWith('label:')) { + return true; + } + return type === WorkflowDiagramTypes.LABEL_NAME || type === WorkflowDiagramTypes.LABEL_BOUNDARY_NAME; + } } diff --git a/packages/sidecar-toolkit/test/apply-label-edit-rename-handler.test.ts b/packages/sidecar-toolkit/test/apply-label-edit-rename-handler.test.ts index 8b63d14..d28f888 100644 --- a/packages/sidecar-toolkit/test/apply-label-edit-rename-handler.test.ts +++ b/packages/sidecar-toolkit/test/apply-label-edit-rename-handler.test.ts @@ -13,15 +13,15 @@ import 'reflect-metadata'; import { describe, expect, it } from 'vitest'; import { ApplyLabelEditOperation, type Command } from '@eclipse-glsp/server'; -import { WorkflowDiagramMetadata } from '@dialogram/shared'; +import { WorkflowDiagramMetadata, WorkflowDiagramTypes } from '@dialogram/shared'; import { ApplyLabelEditRenameHandler } from '../src/server/operations/apply-label-edit-rename-handler'; import { RenameEntityOperation } from '../src/server/operations/rename-entity-handler'; -/** A node element carrying the entity name in `args`, plus its header label child. */ -function modelWith(nodeId: string, entityName: string, labelId: string) { +/** A node element carrying the entity name in `args`, plus one typed label child. */ +function modelWith(nodeId: string, entityName: string, labelId: string, labelType?: string) { const node: any = { id: nodeId, args: { [WorkflowDiagramMetadata.ENTITY_NAME]: entityName } }; - const label: any = { id: labelId, args: {}, parent: node }; + const label: any = { id: labelId, type: labelType, args: {}, parent: node }; node.parent = undefined; const index = { find: (id: string) => (id === labelId ? label : id === nodeId ? node : undefined) @@ -29,6 +29,20 @@ function modelWith(nodeId: string, entityName: string, labelId: string) { return { index }; } +/** The delegating handler, wired to record what it would rename. */ +function handlerFor(model: ReturnType) { + const handler = new ApplyLabelEditRenameHandler(); + (handler as any).modelState = model; + const seen: RenameEntityOperation.Operation[] = []; + (handler as any).renameHandler = { + createCommand: (op: RenameEntityOperation.Operation) => { + seen.push(op); + return { label: 'rename-command' } as unknown as Command; + } + }; + return { handler, seen }; +} + describe('ApplyLabelEditRenameHandler (MCP label-edit → reversible rename)', () => { it('exposes the protocol operation kind as a static literal (DI-less read)', () => { // The GLSP DefaultGlobalActionProvider reads `.operationType` off a `new ctor()` built @@ -84,3 +98,74 @@ describe('ApplyLabelEditRenameHandler (MCP label-edit → reversible rename)', ( ).toBeUndefined(); }); }); + +/** + * Editing a TYPE must never rename. + * + * This handler 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 changing a port's type sent `renameNode { old: , new: }` and renamed the port to its own type. The property panel offers exactly that edit + * ("Change Port Type"), and the canvas label is editable too, so both routes corrupted the source. + * + * The guard is an allow-list, which is the polarity that matters: the next label type added is + * refused by default rather than silently inheriting "editing this renames the node". + */ +describe('ApplyLabelEditRenameHandler — only a NAME may rename', () => { + it('refuses a boundary port type label', () => { + const { handler, seen } = handlerFor( + modelWith('bnd:Com', 'Com', 'bnd:Com_label_type', WorkflowDiagramTypes.LABEL_BOUNDARY_TYPE) + ); + + const command = handler.createCommand( + ApplyLabelEditOperation.create({ labelId: 'bnd:Com_label_type', text: 'RobAlloc' }) + ); + + expect(command).toBeUndefined(); + // The bug was not "wrong command" but "renamed the port to the type". + expect(seen).toEqual([]); + }); + + it('refuses an entity node type subtitle', () => { + const { handler, seen } = handlerFor( + modelWith('node:rob', 'rob', 'node:rob_subtitle', WorkflowDiagramTypes.LABEL_TYPE) + ); + + expect( + handler.createCommand(ApplyLabelEditOperation.create({ labelId: 'node:rob_subtitle', text: 'Rob' })) + ).toBeUndefined(); + expect(seen).toEqual([]); + }); + + it('still renames from a name label, on a node and on a boundary port', () => { + const entity = handlerFor( + modelWith('node:rob', 'rob', 'node:rob_label_name', WorkflowDiagramTypes.LABEL_NAME) + ); + entity.handler.createCommand( + ApplyLabelEditOperation.create({ labelId: 'node:rob_label_name', text: 'reorder' }) + ); + expect(entity.seen).toEqual([ + { kind: RenameEntityOperation.KIND, elementId: 'node:rob', newName: 'reorder' } + ]); + + const boundary = handlerFor( + modelWith('bnd:Com', 'Com', 'bnd:Com_label_name', WorkflowDiagramTypes.LABEL_BOUNDARY_NAME) + ); + boundary.handler.createCommand( + ApplyLabelEditOperation.create({ labelId: 'bnd:Com_label_name', text: 'Commit' }) + ); + expect(boundary.seen).toEqual([ + { kind: RenameEntityOperation.KIND, elementId: 'bnd:Com', newName: 'Commit' } + ]); + }); + + /** A label type nobody has classified yet is refused, not assumed to be a name. */ + it('refuses a label type it has never heard of', () => { + const { handler, seen } = handlerFor(modelWith('node:x', 'x', 'node:x_label_new', 'label:something:new')); + + expect( + handler.createCommand(ApplyLabelEditOperation.create({ labelId: 'node:x_label_new', text: 'Y' })) + ).toBeUndefined(); + expect(seen).toEqual([]); + }); +});