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)', () => {