From 176a9b34512096d2bab8ab16ab03a054ed4d61ec Mon Sep 17 00:00:00 2001 From: Endri Bezati Date: Fri, 4 Sep 2026 11:56:31 +0200 Subject: [PATCH] fix: a read-only diagram can still be laid out MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `edits: 'read-only'` registered no operation handlers at all. That reads "read-only" as "no operations", when what it has to mean is "no operations that change the SOURCE". Laying a diagram out and moving a node are operations, and neither touches the document the diagram was generated from. They write to the separate layout file, which is presentation state. Refusing them bought nothing and cost a great deal: the context menu offers its three layout entries unconditionally, so all three appeared on a read-only diagram and all three did nothing when clicked, because a GLSP operation with no bound handler fails silently. Dragging a node did not persist either, so an arrangement made by hand to compensate was gone by the next open. That fell hardest on exactly the diagrams least able to absorb it. A read-only diagram is usually a generated one, and a generated diagram is the kind that most needs laying out, because nobody placed its nodes by hand to begin with. The read-only cut now sits below the neutral in-core handlers and above the consumer's injected modules, which is where source editing actually lives. The guarantee that matters is unchanged and is now the load-bearing one: `ReadOnlySourceModelStorage.saveSourceModel` is a no-op, so nothing here widens what a reader can alter — only what they can rearrange. Two existing tests pinned the old rule and are updated rather than deleted, because both were asserting more than their subject. The module-injection suite now asserts that read-only injects none of the source-editing handlers, which is what that file is about; the edit-strategy suite asserts that both session kinds bind the same presentation handlers and differ only by what the consumer injects. Each names the layout handlers one by one, since the regression to catch is a single menu entry going quiet rather than all of them. This also leaves the seam in the right shape for a product that later wants to edit through the diagram: what it would add is an operation module, on the other side of a cut that now falls in a defensible place. --- .../src/server/diagram-module.ts | 38 ++++++- .../test/read-only-edit-strategy.test.ts | 56 ++++++++-- .../test/read-only-layout-operations.test.ts | 105 ++++++++++++++++++ .../test/operation-modules.test.ts | 22 +++- 4 files changed, 203 insertions(+), 18 deletions(-) create mode 100644 packages/diagram-server/test/read-only-layout-operations.test.ts diff --git a/packages/diagram-server/src/server/diagram-module.ts b/packages/diagram-server/src/server/diagram-module.ts index 9b8c620..bc4d95a 100644 --- a/packages/diagram-server/src/server/diagram-module.ts +++ b/packages/diagram-server/src/server/diagram-module.ts @@ -116,15 +116,35 @@ export class WorkflowDiagramModule extends GModelDiagramModule { protected override configureOperationHandlers( binding: InstanceMultiBinding ): void { - // Read-only sessions never register a write operation handler. - if (this.options?.edits === 'read-only') { - return; - } - // DO NOT call super - it registers default delete/reconnect handlers // that would conflict with ours and cause "Key is already registered" warnings // super.configureOperationHandlers(binding); + // A read-only session used to return here, registering nothing at all. + // + // That read "read-only" as "no operations", when what it has to mean is + // "no operations that change the SOURCE". The handlers below the + // read-only guard are the ones that do; the handlers above it only ever + // touch presentation — where a node sits, how an edge is routed — and + // that lives in the separate layout file, never in the document the + // diagram was generated from. + // + // Registering none of them made a read-only diagram one that cannot be + // laid out. The three layout entries in the context menu are offered + // unconditionally, so all three appeared and all three did nothing when + // clicked: no handler was bound for the operation they dispatch, and a + // GLSP operation nobody handles fails silently. Dragging a node did not + // persist either, so the arrangement someone made by hand to compensate + // was gone on the next open. + // + // A generated diagram is exactly the kind that most needs laying out, + // because nobody placed its nodes by hand in the first place. + // + // Writing back to the source stays impossible regardless of what is + // registered here: `ReadOnlySourceModelStorage.saveSourceModel` is a + // no-op for these sessions, so this widens what a reader can rearrange + // without widening what they can alter. + // Essential handlers from DiagramModule that we must include: binding.add(CompoundOperationHandler); // Use our custom layout handler instead of default (which only works in MANUAL mode) @@ -146,12 +166,18 @@ export class WorkflowDiagramModule extends GModelDiagramModule { binding.add(WorkflowResetEdgeRoutesOperationHandler); binding.add(WorkflowRerouteEdgesAvoidOverlapsOperationHandler); + // Everything above is presentation or discovery, and safe for a session + // that may not edit. Everything below writes to the source, so this is + // where a read-only session stops. + if (this.options?.edits === 'read-only') { + return; + } + // Consumer-supplied operation modules (e.g. the toolkit's source-editing handlers, which // live outside core). Each module registers its handlers after the neutral in-core set // above; order is irrelevant (every handler responds to a distinct operation kind). Modules // arrive as opaque handles on `EditStrategy.operationModules` -- narrow each to the concrete // contract before invoking it. - // `read-only` already returned above, so any `edits` here carries operation modules. const edits = this.options?.edits; if (edits) { for (const module of edits.operationModules) { diff --git a/packages/diagram-server/test/read-only-edit-strategy.test.ts b/packages/diagram-server/test/read-only-edit-strategy.test.ts index 197dca2..b8120eb 100644 --- a/packages/diagram-server/test/read-only-edit-strategy.test.ts +++ b/packages/diagram-server/test/read-only-edit-strategy.test.ts @@ -1,13 +1,31 @@ -// Pins the `EditStrategy` seam on `WorkflowDiagramModule`: an editable module (default, no -// options) registers the full set of operation handlers; a module constructed with -// `{ edits: 'read-only' }` registers none. `configureOperationHandlers` is a `protected` method +// Pins the `EditStrategy` seam on `WorkflowDiagramModule`. +// +// The rule was once "read-only registers no operation handlers". That has been +// narrowed to "read-only registers no handler that edits the SOURCE", because +// the original was too blunt to be right: laying a diagram out and moving a +// node are operations, and neither touches the document the diagram came from. +// They write to the separate layout file, which is presentation state. +// +// Under the old rule a read-only diagram could not be laid out at all. The +// context menu offers its layout entries unconditionally, so each appeared and +// each did nothing, since a GLSP operation with no bound handler fails +// silently — and a generated diagram, which is the kind most likely to be +// read-only, is also the kind that most needs laying out, because nobody +// placed its nodes by hand. +// +// What has not changed is the guarantee that matters: a read-only session +// cannot write to the source. That is enforced by `saveSourceModel` being a +// no-op, asserted below, and by none of the consumer's source-editing modules +// being configured. +// +// `configureOperationHandlers` is a `protected` method // GLSP's `DiagramModule` calls with an `InstanceMultiBinding` -- we // invoke it directly through an `as any` cast (matching this test suite's existing pattern for // exercising protected members) with a recording fake that captures every `.add(...)` call // instead of registering it with a real inversify container. // // Also pins the F2 final-review finding: `edits: 'read-only'` must make `saveSourceModel` a -// no-op too (per the spec: "read-only binds no operation handlers and a no-op saveSourceModel"). +// no-op too. That half of the original spec stands unchanged and is now the load-bearing half. // `WorkflowDiagramModule.bindSourceModelStorage()` binds `ReadOnlySourceModelStorage` -- // overriding `saveSourceModel` to a no-op -- when constructed with `{ edits: 'read-only' }`, and // `WorkflowSourceModelStorage` (the default, editable path) otherwise. @@ -18,19 +36,37 @@ import { WorkflowSourceModelStorage } from '../src/server/source-model-storage'; import { ReadOnlySourceModelStorage } from '../src/server/read-only-source-model-storage'; describe('EditStrategy read-only', () => { - it('a read-only module binds no operation handlers, while an editable module binds several', () => { + it('a read-only module binds the presentation handlers, and so does an editable one', () => { const editable = new WorkflowDiagramModule(); const readOnly = new WorkflowDiagramModule({ edits: 'read-only' }); - const collect = (module: WorkflowDiagramModule): unknown[] => { - const registered: unknown[] = []; - const recordingBinding = { add: (handler: unknown) => registered.push(handler) }; + const collect = (module: WorkflowDiagramModule): string[] => { + const registered: string[] = []; + const recordingBinding = { add: (handler: { name: string }) => registered.push(handler.name) }; (module as any).configureOperationHandlers(recordingBinding); return registered; }; - expect(collect(editable).length).toBeGreaterThan(0); - expect(collect(readOnly)).toEqual([]); + const editableNames = collect(editable); + const readOnlyNames = collect(readOnly); + + expect(editableNames.length).toBeGreaterThan(0); + // Named one by one rather than counted, because the regression this + // guards against is a single layout going quiet, not all of them. + for (const handler of [ + 'WorkflowLayoutOperationHandler', + 'WorkflowLayoutBoundaryFlowOperationHandler', + 'WorkflowLayoutSerpentineMeshOperationHandler', + 'WorkflowChangeBoundsOperationHandler', + 'WorkflowChangeRoutingPointsOperationHandler' + ]) { + expect(readOnlyNames, `${handler} is missing from a read-only session`).toContain(handler); + expect(editableNames, `${handler} is missing from an editable session`).toContain(handler); + } + + // The two differ only by what the consumer injects, which is where + // source editing lives. + expect(readOnlyNames).toEqual(editableNames); }); it('defaults to editable when no options are passed (byte-identical to pre-EditStrategy behavior)', () => { diff --git a/packages/diagram-server/test/read-only-layout-operations.test.ts b/packages/diagram-server/test/read-only-layout-operations.test.ts new file mode 100644 index 0000000..5c94ced --- /dev/null +++ b/packages/diagram-server/test/read-only-layout-operations.test.ts @@ -0,0 +1,105 @@ +// What a read-only session is allowed to do. +// +// "Read-only" has to mean "cannot change the SOURCE", not "cannot do anything". +// The distinction matters because the operations that lay a diagram out and the +// ones that move a node do not touch the document the diagram was generated +// from: they write to the separate layout file, which is presentation state. +// +// Registering no operation handlers at all made a read-only diagram one that +// could not be laid out. The context menu offers its three layout entries +// unconditionally, so all three appeared and all three did nothing when +// clicked, because a GLSP operation with no bound handler fails silently. A +// node dragged by hand did not persist either, so the arrangement someone made +// to compensate was gone by the next open. +// +// That hit generated diagrams hardest, which are exactly the ones that most +// need laying out: nobody placed their nodes by hand to begin with. +// +// These tests pin both halves — the presentation operations are bound, and the +// consumer's source-editing modules still are not. + +import 'reflect-metadata'; +import { describe, expect, it } from 'vitest'; +import { WorkflowDiagramModule } from '../src/server/diagram-module'; + +/** Collects what a module registers, without booting a container. */ +function registeredHandlers(edits: 'read-only' | { operationModules: unknown[] }): string[] { + const names: string[] = []; + const binding = { add: (ctor: { name: string }) => names.push(ctor.name) }; + const module = new WorkflowDiagramModule({ edits } as never); + (module as never as { configureOperationHandlers(b: unknown): void }).configureOperationHandlers( + binding + ); + return names; +} + +/** Operations that only ever change presentation, never the source document. */ +const PRESENTATION_HANDLERS = [ + 'WorkflowLayoutOperationHandler', + 'WorkflowLayoutBoundaryFlowOperationHandler', + 'WorkflowLayoutSerpentineMeshOperationHandler', + 'WorkflowChangeBoundsOperationHandler', + 'WorkflowChangeRoutingPointsOperationHandler', + 'WorkflowResetEdgeRoutesOperationHandler', + 'WorkflowRerouteEdgesAvoidOverlapsOperationHandler' +]; + +describe('a read-only diagram session', () => { + it('binds a handler for every layout the context menu offers', () => { + const bound = registeredHandlers('read-only'); + // Named individually rather than counted, because the failure this + // guards against is one menu entry going quiet, not all of them. + for (const handler of [ + 'WorkflowLayoutOperationHandler', + 'WorkflowLayoutBoundaryFlowOperationHandler', + 'WorkflowLayoutSerpentineMeshOperationHandler' + ]) { + expect(bound, `${handler} is not bound, so its menu entry does nothing`).toContain( + handler + ); + } + }); + + it('lets a reader move a node and keep the result', () => { + const bound = registeredHandlers('read-only'); + expect(bound).toContain('WorkflowChangeBoundsOperationHandler'); + expect(bound).toContain('WorkflowChangeRoutingPointsOperationHandler'); + }); + + it('binds every presentation operation, and the same set an editable one does', () => { + const readOnly = registeredHandlers('read-only'); + const editable = registeredHandlers({ operationModules: [] }); + for (const handler of PRESENTATION_HANDLERS) { + expect(readOnly).toContain(handler); + expect(editable).toContain(handler); + } + }); + + it('still refuses the consumer modules that edit the source', () => { + // The one thing read-only must keep out. A module handed here would be + // the toolkit's source-editing handlers, and a read-only session must + // never invoke them. + let configured = false; + // Carries the brand the platform's own type guard looks for; a plain + // object with a `configure` is ignored, which would make this test pass + // for the wrong reason. + const sourceEditing = { + __diagramOperationModule: true, + configure: () => { + configured = true; + } + }; + registeredHandlers({ operationModules: [sourceEditing] } as never); + expect(configured, 'an editable session configures its operation modules').toBe(true); + + configured = false; + const module = new WorkflowDiagramModule({ + edits: 'read-only', + operationModules: [sourceEditing] + } as never); + (module as never as { configureOperationHandlers(b: unknown): void }).configureOperationHandlers({ + add: () => undefined + }); + expect(configured, 'a read-only session must not configure them').toBe(false); + }); +}); diff --git a/packages/sidecar-toolkit/test/operation-modules.test.ts b/packages/sidecar-toolkit/test/operation-modules.test.ts index 7bdacd6..6b7f88a 100644 --- a/packages/sidecar-toolkit/test/operation-modules.test.ts +++ b/packages/sidecar-toolkit/test/operation-modules.test.ts @@ -106,8 +106,26 @@ describe('createSidecarOperationModules — module-injection parity', () => { expect(withSidecar.length).toBe(neutralOnly.length + EXPECTED_SIDECAR_HANDLER_NAMES.length); }); - it('a read-only module registers no operation handlers', () => { - expect(collectRegisteredHandlers('read-only')).toEqual([]); + it('a read-only module injects none of the source-editing handlers', () => { + const readOnly = collectRegisteredHandlers('read-only').map((ctor) => ctor.name); + + // This used to assert the list was empty, which pinned more than this + // suite is about. A read-only session still registers the neutral + // in-core handlers, because those only change presentation — where a + // node sits, how an edge is routed — and that is written to the + // separate layout file rather than to the source document. + // + // Registering nothing at all meant a read-only diagram could not be + // laid out: the layout entries in the context menu are offered + // unconditionally, and each dispatched an operation no handler was + // bound for, which fails silently. + expect(readOnly.length).toBeGreaterThan(0); + + // What read-only actually has to keep out, and the subject of this + // file: every handler that edits the source. + for (const name of EXPECTED_SIDECAR_HANDLER_NAMES) { + expect(readOnly, `${name} must not reach a read-only session`).not.toContain(name); + } }); it('the injected module set is identical for a calpy-shaped config (same set, same order)', () => {