From e1cca359cc18b518bdf65c01045c1a20f9576bdb Mon Sep 17 00:00:00 2001 From: Endri Bezati Date: Mon, 9 Nov 2026 08:42:40 +0100 Subject: [PATCH 1/2] refactor: what a double-click on a node opens is a contribution point The double-click listener ("viewer listener") did two things: open a node's definition, which every product uses, and, for a task with a @viewer annotation, open its last tokens (open, diff, open with an editor, run a command) or a path it declares, which only wfpy has. `NodeOpener` (node-opener.ts, exported) is the contribution point: an opener with an `id` answers a double-click on a node with the actions to run, or leaves the node to the platform, which opens its definition as before. The listener multi-injects them (one per id, the last binding winning, asked in the order the ids were first bound; the first that answers wins) and asks them before its own definition navigation. The @viewer half moves verbatim into viewer-node-opener.ts (ViewerNodeOpener, id `viewer`): git's moved-code detection marks 33 of its 372 lines as new, the opener's guard and context lines and its imports. The stock module binds it, so nothing changes; wfpy-ide binds its own next, and then the stock module stops. The three @viewer test files inject the opener the stock module binds and pass unchanged; test/node-opener-seam.test.ts pins the contract and the bindings, through a real container too. The parity baseline accepts the one constant binding. Claude-Session: https://claude.ai/code/session_01EToj6RJRMSSHQiqGNMU9wv --- CHANGELOG.md | 8 + packages/diagram-client/README.md | 9 + packages/diagram-client/src/index.ts | 1 + packages/diagram-client/src/node-opener.ts | 31 ++ .../src/stock-features.module.ts | 5 + .../src/viewer-mouse-listener.ts | 402 +---------------- .../diagram-client/src/viewer-node-opener.ts | 411 ++++++++++++++++++ .../test/container-parity.test.ts | 3 +- .../test/declared-target-resources.test.ts | 3 + .../test/double-click-opens-one-thing.test.ts | 3 + .../fixtures/container-parity.baseline.json | 5 + .../test/node-opener-seam.test.ts | 114 +++++ .../test/viewer-declared-target.test.ts | 3 + 13 files changed, 616 insertions(+), 382 deletions(-) create mode 100644 packages/diagram-client/src/node-opener.ts create mode 100644 packages/diagram-client/src/viewer-node-opener.ts create mode 100644 packages/diagram-client/test/node-opener-seam.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 566bafa..10d2aa3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -47,6 +47,14 @@ to fail its version check rather than run against what it no longer has. ### Added +- **What a double-click on a node opens is a contribution point: + `NodeOpener`** (exported). An opener answers before the platform's own + definition navigation, or leaves the node to it; one per `id`, the last + binding winning. The `@viewer` half of the double-click listener (open, + diff, open with an editor or run a command on a task's last tokens, or a + path it declares) is the `viewer` opener (`viewer-node-opener.ts`), which + the stock module binds until wfpy-ide binds its own. + - **What a product's webview code needs, from the package.** `@dialogram/diagram-client` exports `VscodeUi` (the host's quick picks, input boxes and commands), `clientBehavior`, `commandId`, `offersRun`, diff --git a/packages/diagram-client/README.md b/packages/diagram-client/README.md index ed6cc75..d832971 100644 --- a/packages/diagram-client/README.md +++ b/packages/diagram-client/README.md @@ -267,6 +267,15 @@ There is one template per annotation name, and a later binding replaces an earlier one. The wizard lists them before its own "Custom annotation…" entry (`custom`), which a template cannot replace. +**Node openers.** A double-click on a node opens its definition, unless the +network navigation opens something for it. To open something else for some +of your nodes, bind a `NodeOpener` with an `id`: its `open(node, { sourceUri })` +returns the actions to run, or `undefined` to leave the node to the platform. +Openers are asked in the order their ids were first bound, and the first +that answers wins; a later binding for an id replaces the earlier one. The +stock bundle binds wfpy's `@viewer` opener (id `viewer`) until wfpy-ide binds +its own. + **The agent workflow UI.** The stock bundle binds no editors, sections or templates. wfpy's are `agentUiModule`, imported by path as `@dialogram/diagram-client/agent-ui/index`: the `@tool` and `@agent` diff --git a/packages/diagram-client/src/index.ts b/packages/diagram-client/src/index.ts index 5f177f6..b132cd2 100644 --- a/packages/diagram-client/src/index.ts +++ b/packages/diagram-client/src/index.ts @@ -78,6 +78,7 @@ export { type PanelModelRoot } from './node-section'; export { createPropertySection, escapeHtml, shortenPath } from './property-section'; +export { NodeOpener, type NodeOpenerContext } from './node-opener'; export { AnnotationTemplate, CUSTOM_ANNOTATION_ENTRY, diff --git a/packages/diagram-client/src/node-opener.ts b/packages/diagram-client/src/node-opener.ts new file mode 100644 index 0000000..7bc52a9 --- /dev/null +++ b/packages/diagram-client/src/node-opener.ts @@ -0,0 +1,31 @@ +/** + * Node openers: the contribution point for what a double-click on a node + * opens. + * + * The platform opens a node's definition (preview), unless the network + * navigation listener already opens something for it. An opener a product + * binds is asked first: it answers with the actions that open what the node + * stands for in that product, or `undefined` to leave the node to the + * platform. A product binds its openers in a feature module: + * + * bind(NodeOpener).toConstantValue(myOpener); + * + * There is one opener per `id`: a later binding for an id replaces the + * earlier one. Openers are asked in the order their ids were first bound; the + * first that answers wins. + */ +import type { Action, GModelElement } from '@eclipse-glsp/sprotty'; + +export const NodeOpener = Symbol.for('dialogram.NodeOpener'); + +export interface NodeOpenerContext { + /** The file the diagram was opened from, if any. */ + readonly sourceUri: string | undefined; +} + +export interface NodeOpener { + /** What the opener is, for a later binding to replace it. */ + readonly id: string; + /** The actions a double-click on `node` runs, or `undefined` to leave it to the platform. */ + open(node: GModelElement, context: NodeOpenerContext): Array> | undefined; +} diff --git a/packages/diagram-client/src/stock-features.module.ts b/packages/diagram-client/src/stock-features.module.ts index 162f52c..ef09ed2 100644 --- a/packages/diagram-client/src/stock-features.module.ts +++ b/packages/diagram-client/src/stock-features.module.ts @@ -18,6 +18,8 @@ import { clientBehavior } from './profile'; import { WorkflowNavigationUi } from './navigation-ui'; import { WorkflowNetworkNavigationMouseListener } from './network-navigation-mouse-listener'; import { ViewerMouseListener } from './viewer-mouse-listener'; +import { NodeOpener } from './node-opener'; +import { ViewerNodeOpener } from './viewer-node-opener'; import { EdgeOpenMouseListener } from './edge-open-mouse-listener'; import { WhitespaceRootPropertiesMouseListener } from './whitespace-root-properties-mouse-listener'; import { WorkflowElkLiveDragRouter } from './elk-live-drag-router'; @@ -114,6 +116,9 @@ export const workflowFeaturesModule = new ContainerModule((bind, unbind, isBound // Double-click on @viewer external tasks to open/diff the last token files. bindAsService(context, TYPES.MouseListener, ViewerMouseListener); + // What a double-click on a `@viewer` task opens (its last tokens), until + // the product that has `@viewer` binds it in its own bundle. + bind(NodeOpener).toConstantValue(new ViewerNodeOpener()); // Double-click on diagram whitespace to show root/workflow properties. bindAsService(context, TYPES.MouseListener, WhitespaceRootPropertiesMouseListener); diff --git a/packages/diagram-client/src/viewer-mouse-listener.ts b/packages/diagram-client/src/viewer-mouse-listener.ts index 5f43742..a3f4785 100644 --- a/packages/diagram-client/src/viewer-mouse-listener.ts +++ b/packages/diagram-client/src/viewer-mouse-listener.ts @@ -8,18 +8,11 @@ import { } from '@eclipse-glsp/sprotty'; import { openTargetOf } from './open-target'; import { openLeaf } from './network-navigation-mouse-listener'; -import { inject, injectable } from 'inversify'; +import { inject, injectable, multiInject, optional } from 'inversify'; import { WorkflowDiagramMetadata, WorkflowDiagramTypes } from '@dialogram/shared'; -import { settingsNamespace } from './profile'; +import { NodeOpener } from './node-opener'; +import { lastPerKey } from './contributions'; -const VIEWER_ACTION_ARG = 'wf:viewerAction'; -const VIEWER_VIEW_TYPE_ARG = 'wf:viewerViewType'; -const VIEWER_COMMAND_ARG = 'wf:viewerCommand'; -const VIEWER_COMMAND_ARGS_ARG = 'wf:viewerCommandArgs'; -const VIEWER_LEFT_URI_ARG = 'wf:viewerLeftUri'; -const VIEWER_RIGHT_URI_ARG = 'wf:viewerRightUri'; -const VIEWER_TITLE_ARG = 'wf:viewerTitle'; -const VIEWER_MESSAGE_ARG = 'wf:viewerMessage'; const SHOW_OPTIONS_ARG = 'jsonOpenerOptions'; const NAVIGATE_PREFER_DEFINITION_ARG = 'wf:navigatePreferDefinition'; const NAVIGATE_SOURCE_URI_ARG = 'wf:navigateSourceUri'; @@ -29,11 +22,6 @@ const NAVIGATE_SYMBOL_ARG = 'wf:navigateSymbol'; type SerializedPosition = { line: number; character: number }; type SerializedRange = { start: SerializedPosition; end: SerializedPosition }; -type DefinitionAnnotation = { - name: string; - arguments?: Array<{ name: string; value: string }>; -}; - function findContainingEntityNode(element: GModelElement | undefined): GModelElement | undefined { let current: GModelElement | undefined = element; while (current) { @@ -49,188 +37,12 @@ function findContainingEntityNode(element: GModelElement | undefined): GModelEle return undefined; } -function parseStringLiteralish(text: string | undefined): string | undefined { - if (typeof text !== 'string') { - return undefined; - } - const trimmed = text.trim(); - if ((trimmed.startsWith('"') && trimmed.endsWith('"')) || (trimmed.startsWith("'") && trimmed.endsWith("'"))) { - return trimmed.slice(1, -1); - } - return trimmed; -} - -function parseStringListLiteralish(text: string | undefined): string[] | undefined { - if (typeof text !== 'string') { - return undefined; - } - - const trimmed = text.trim(); - if (trimmed === '') { - return undefined; - } - - // Try JSON first (preferred: ["In", "Out"]). - if (trimmed.startsWith('[') && trimmed.endsWith(']')) { - try { - const parsed = JSON.parse(trimmed); - if (Array.isArray(parsed) && parsed.every(x => typeof x === 'string')) { - return parsed as string[]; - } - } catch { - // fall through - } - - // Best-effort: extract quoted strings. - const items: string[] = []; - const re = /"([^"]+)"|'([^']+)'/g; - let m: RegExpExecArray | null; - while ((m = re.exec(trimmed))) { - const s = m[1] ?? m[2]; - if (typeof s === 'string' && s.trim() !== '') { - items.push(s); - } - } - return items.length > 0 ? items : undefined; - } - - return undefined; -} - -function resolveViewerCommandArgs(args: string[] | undefined, targetUri: string, tokenPath: string): string[] | undefined { - if (!args || args.length === 0) { - return undefined; - } - return args.map(value => value - .replace(/\{uri\}/g, targetUri) - .replace(/\{path\}/g, tokenPath) - ); -} - /** * Anything with a scheme is already a complete address — a URL, or a URI the * host knows how to route. It must not be treated as a path and glued onto a * directory, which is what turned `https://example.com/x` into * `file:///w/https://example.com/x`. */ -const ABSOLUTE_URI = /^[A-Za-z][A-Za-z0-9+.-]*:\/\//; - -/** - * The target a declared path names, or undefined when it cannot be resolved. - * - * A relative path means "beside the file this was declared in", so with no such - * file there is no answer — better said than guessed. - */ -function resolveDeclaredTarget( - sourceUri: string | undefined, - declaredPath: string -): string | undefined { - if (ABSOLUTE_URI.test(declaredPath) || declaredPath.startsWith('/')) { - return declaredPath; - } - return sourceUri ? resolveAgainstSource(sourceUri, declaredPath) : undefined; -} - -/** - * What to do with a target, given what it is. - * - * The kind is decided by whoever exported the graph, because that is where the - * filesystem is visible — nothing here can tell a folder from a file by looking - * at the string. - */ -function actionForDeclaredTarget( - target: string, - kind: string | undefined, - viewType: string | undefined -): 'open' | 'openWith' | 'reveal' { - // A directory has no editor to open in; revealing it is the operation that - // exists for one. - if (kind === 'folder') { - return 'reveal'; - } - // No editor is registered for a remote scheme, so openWith dead-ends there. - // Plain open hands it to the host, which launches a browser. - const isRemote = ABSOLUTE_URI.test(target) && !target.startsWith('file:'); - if (isRemote || !viewType) { - return 'open'; - } - return 'openWith'; -} - -/** A path declared in the diagram's own source file, resolved beside it. */ -function resolveAgainstSource(sourceUri: string, relativePath: string): string { - const lastSlash = sourceUri.lastIndexOf('/'); - const directory = lastSlash >= 0 ? sourceUri.slice(0, lastSlash) : sourceUri; - return `${directory}/${relativePath}`; -} - -function fileUriFromFsPath(fsPath: string): string { - const p = fsPath.trim(); - if (p.startsWith('file:')) { - return p; - } - // Assume absolute POSIX path (CLI produces absolute paths on Linux/macOS). - const encoded = encodeURI(p); - return encoded.startsWith('/') ? `file://${encoded}` : encoded; -} - -function collectEdges(root: any): any[] { - const out: any[] = []; - const visit = (el: any): void => { - if (!el) { - return; - } - // Sprotty edges typically have type starting with 'edge:' - if (typeof el.type === 'string' && el.type.startsWith('edge:')) { - out.push(el); - } - const children = el.children as any[] | undefined; - if (Array.isArray(children)) { - for (const c of children) { - visit(c); - } - } - }; - visit(root); - return out; -} - -function incomingEdgesForTargetPort(edges: any[], targetPortId: string): any[] { - return edges.filter(e => (e as any).targetId === targetPortId); -} - -function bestIncomingEdgeForViewerInput(edges: any[], targetPortId: string): any | undefined { - const candidates = incomingEdgesForTargetPort(edges, targetPortId); - if (candidates.length === 0) { - return undefined; - } - - const withToken = candidates.filter(e => { - const v = e?.args?.[WorkflowDiagramMetadata.VIEWER_LAST_TOKEN]; - return lastTokenAsPath(v) !== undefined; - }); - - const stableKey = (e: any): string => { - const astPath = e?.args?.[WorkflowDiagramMetadata.AST_PATH]; - return typeof astPath === 'string' ? astPath : String(e?.id ?? ''); - }; - - const pickFrom = (withToken.length > 0 ? withToken : candidates).slice(); - pickFrom.sort((a, b) => stableKey(a).localeCompare(stableKey(b))); - return pickFrom[pickFrom.length - 1]; -} - -function lastTokenAsPath(value: unknown): string | undefined { - if (typeof value === 'string') { - return value; - } - if (Array.isArray(value) && value.length > 0) { - const last = value[value.length - 1]; - return typeof last === 'string' ? last : undefined; - } - return undefined; -} - function isSerializedPosition(value: unknown): value is SerializedPosition { if (!value || typeof value !== 'object') { return false; @@ -355,6 +167,15 @@ export class ViewerMouseListener extends MouseListener implements Ranked { @inject(EditorContextService) protected readonly editorContext!: EditorContextService; + /** What products open on a double-click; see `node-opener.ts`. */ + @multiInject(NodeOpener) @optional() + protected readonly contributedOpeners: readonly NodeOpener[] | undefined; + + /** One opener per id, in the order the ids were first bound; a later binding replaces the earlier one. */ + protected nodeOpeners(): readonly NodeOpener[] { + return lastPerKey(this.contributedOpeners, opener => opener.id); + } + override doubleClick(target: GModelElement, _event: MouseEvent): (Action | Promise)[] { const node = findContainingEntityNode(target); if (!node) { @@ -371,6 +192,14 @@ export class ViewerMouseListener extends MouseListener implements Ranked { return []; } + // What a product opens for the node, before the platform's own: its definition. + for (const opener of this.nodeOpeners()) { + const opened = opener.open(node, { sourceUri: this.editorContext.sourceUri }); + if (opened) { + return opened; + } + } + const isExternal = args[WorkflowDiagramMetadata.IS_EXTERNAL_ACTOR] === true || node.type === WorkflowDiagramTypes.NODE_EXTERNAL_ACTOR; const sourceUri = this.editorContext.sourceUri; const definitionNavigationAction = buildDefinitionNavigationAction(node, sourceUri); @@ -390,195 +219,6 @@ export class ViewerMouseListener extends MouseListener implements Ranked { return []; } - const defAnnotations = args[WorkflowDiagramMetadata.ENTITY_DEFINITION_ANNOTATIONS] as unknown; - const annotations = Array.isArray(defAnnotations) ? (defAnnotations as DefinitionAnnotation[]) : []; - const viewer = annotations.find(a => a && a.name === 'viewer'); - if (!viewer) { - return definitionNavigationAction && !leafOpensItself(node) ? [definitionNavigationAction] : []; - } - - const argByName = new Map(); - for (const a of viewer.arguments ?? []) { - if (a && typeof a.name === 'string' && typeof a.value === 'string') { - argByName.set(a.name, a.value); - } - } - - const action = ( - parseStringLiteralish(argByName.get('action')) - ?? parseStringLiteralish(argByName.get('action_name')) - ?? 'open' - ).trim(); - const inputs = parseStringListLiteralish(argByName.get('inputs')); - const viewType = parseStringLiteralish(argByName.get('viewType')) - ?? parseStringLiteralish(argByName.get('view_type')); - // Where the target comes from. Everything here has always resolved - // through the last token on an input, which is right for a node whose - // target is something it PRODUCED — it cannot exist before a run. A - // node whose target is a path it declares is openable immediately, and - // must not be told to go and run the workflow first. - const source = parseStringLiteralish(argByName.get('source')) ?? 'token'; - const declaredPath = parseStringLiteralish(argByName.get('path')); - // What the target IS — file, folder, http, url — settled by the - // producer, which can see the filesystem. Nothing here can tell a - // folder from a file by looking at the string. - const resourceKind = parseStringLiteralish(argByName.get('kind')); - const command = parseStringLiteralish(argByName.get('command')) - ?? parseStringLiteralish(argByName.get('command_name')); - const commandArgs = parseStringListLiteralish(argByName.get('args')) - ?? parseStringListLiteralish(argByName.get('command_args')); - const title = parseStringLiteralish(argByName.get('title')); - - const fail = (message: string): (Action | Promise)[] => [ - NavigateToExternalTargetAction.create({ - uri: this.editorContext.sourceUri ?? 'file:///', - args: { - [VIEWER_ACTION_ARG]: 'error', - [VIEWER_MESSAGE_ARG]: message - } - }) - ]; - - if (source === 'declared') { - if (!declaredPath) { - return fail('@viewer(source="declared"): missing path="...".'); - } - // Relative to the file the diagram was opened from, which is what a - // declaration in that file means. - const target = resolveDeclaredTarget(sourceUri, declaredPath); - if (!target) { - return fail( - `@viewer(source="declared"): cannot resolve the relative path ` - + `"${declaredPath}" — the diagram has no source file to resolve it against.` - ); - } - const declaredAction = actionForDeclaredTarget(target, resourceKind, viewType); - return [ - NavigateToExternalTargetAction.create({ - uri: target, - args: { - [VIEWER_ACTION_ARG]: declaredAction, - ...(declaredAction === 'openWith' - ? { [VIEWER_VIEW_TYPE_ARG]: viewType as string } - : {}) - } - }) - ]; - } - - // Below here every route resolves its target from the last token on an - // input, so an input is what it needs. This guard used to sit above the - // declared branch, where it caught nodes that never wanted a token: a - // node that declares its target has no inputs by nature, so it was told - // to run the workflow first and could never be opened at all. - if (!inputs || inputs.length === 0) { - return fail('@viewer: missing inputs=[...] (run the workflow first, then double-click again).'); - } - - const root: any = (node as any).root; - const edges = root ? collectEdges(root) : []; - - const overlayRunId = root?.args?.['wf:viewerOverlayRunId']; - const overlayOutDir = root?.args?.['wf:viewerOverlayOutDir']; - const hasOverlay = typeof overlayRunId === 'string' && overlayRunId.trim() !== ''; - - const tokenByInputName = new Map(); - for (const inputName of inputs) { - const targetPortId = `${node.id}_port_${inputName}`; - const incoming = bestIncomingEdgeForViewerInput(edges, targetPortId); - const lastToken = incoming?.args?.[WorkflowDiagramMetadata.VIEWER_LAST_TOKEN]; - const tokenPath = lastTokenAsPath(lastToken); - if (tokenPath) { - tokenByInputName.set(inputName, tokenPath); - } - } - - const getTokenOrError = (name: string): string | undefined => { - return tokenByInputName.get(name); - }; - - if (action === 'diff') { - if (inputs.length !== 2) { - return fail('@viewer(action="diff"): requires exactly 2 inputs in inputs=[...].'); - } - const leftPath = getTokenOrError(inputs[0]); - const rightPath = getTokenOrError(inputs[1]); - if (!leftPath || !rightPath) { - if (!hasOverlay) { - return fail(`Viewer diff: no run overlay found. Run the workflow successfully (CLI writes overlays under ${settingsNamespace()}.runOutputDir), then reopen/refresh the diagram.`); - } - return fail(`Viewer diff: missing last-token file(s) for inputs ${inputs.join(', ')} (overlay runId=${String(overlayRunId)}).`); - } - const leftUri = fileUriFromFsPath(leftPath); - const rightUri = fileUriFromFsPath(rightPath); - return [ - NavigateToExternalTargetAction.create({ - uri: leftUri, - args: { - [VIEWER_ACTION_ARG]: 'diff', - [VIEWER_LEFT_URI_ARG]: leftUri, - [VIEWER_RIGHT_URI_ARG]: rightUri, - ...(title ? { [VIEWER_TITLE_ARG]: title } : {}) - } - }) - ]; - } - - // open/openWith: use first input. - const firstInput = inputs[0]; - const tokenPath = getTokenOrError(firstInput); - if (!tokenPath) { - if (!hasOverlay) { - return fail(`Viewer open: no run overlay found. Run the workflow successfully (CLI writes overlays under ${settingsNamespace()}.runOutputDir), then reopen/refresh the diagram.`); - } - const outDirInfo = typeof overlayOutDir === 'string' && overlayOutDir.trim() !== '' ? ` (outDir=${overlayOutDir})` : ''; - return fail(`Viewer open: missing last-token file for input '${firstInput}' (overlay runId=${String(overlayRunId)})${outDirInfo}.`); - } - - const targetUri = fileUriFromFsPath(tokenPath); - - if (action === 'openWith') { - if (!viewType) { - return fail('@viewer(action="openWith"): missing viewType="...".'); - } - return [ - NavigateToExternalTargetAction.create({ - uri: targetUri, - args: { - [VIEWER_ACTION_ARG]: 'openWith', - [VIEWER_VIEW_TYPE_ARG]: viewType - } - }) - ]; - } - - if (action === 'command') { - if (!command) { - return fail('@viewer(action="command"): missing command="...".'); - } - const resolvedArgs = resolveViewerCommandArgs(commandArgs, targetUri, tokenPath); - return [ - NavigateToExternalTargetAction.create({ - uri: targetUri, - args: { - [VIEWER_ACTION_ARG]: 'command', - [VIEWER_COMMAND_ARG]: command, - ...(resolvedArgs - ? { [VIEWER_COMMAND_ARGS_ARG]: JSON.stringify(resolvedArgs) } - : {}) - } - }) - ]; - } - - // Default: open - return [ - NavigateToExternalTargetAction.create({ - uri: targetUri, - args: { - [VIEWER_ACTION_ARG]: 'open' - } - }) - ]; + return definitionNavigationAction && !leafOpensItself(node) ? [definitionNavigationAction] : []; } } diff --git a/packages/diagram-client/src/viewer-node-opener.ts b/packages/diagram-client/src/viewer-node-opener.ts new file mode 100644 index 0000000..c832078 --- /dev/null +++ b/packages/diagram-client/src/viewer-node-opener.ts @@ -0,0 +1,411 @@ +/** + * What a double-click on a task with a `@viewer` annotation opens: the last + * token on one of its inputs (open, open with a named editor, run a command + * on it), the last tokens on two (diff), or a path the annotation declares. + * A task without one is left to the platform, which opens its definition. + * + * The `@viewer` half of the former viewer listener, moved behind the + * `NodeOpener` contribution point: the actions are the same + * `NavigateToExternalTargetAction`s with the `wf:viewer*` arguments the host + * turns into an editor, a diff or a command. + */ +import { Action, Args, GModelElement, NavigateToExternalTargetAction } from '@eclipse-glsp/sprotty'; +import { WorkflowDiagramMetadata, WorkflowDiagramTypes } from '@dialogram/shared'; +import type { NodeOpener, NodeOpenerContext } from './node-opener'; +import { settingsNamespace } from './profile'; + +const VIEWER_ACTION_ARG = 'wf:viewerAction'; +const VIEWER_VIEW_TYPE_ARG = 'wf:viewerViewType'; +const VIEWER_COMMAND_ARG = 'wf:viewerCommand'; +const VIEWER_COMMAND_ARGS_ARG = 'wf:viewerCommandArgs'; +const VIEWER_LEFT_URI_ARG = 'wf:viewerLeftUri'; +const VIEWER_RIGHT_URI_ARG = 'wf:viewerRightUri'; +const VIEWER_TITLE_ARG = 'wf:viewerTitle'; +const VIEWER_MESSAGE_ARG = 'wf:viewerMessage'; + +type DefinitionAnnotation = { + name: string; + arguments?: Array<{ name: string; value: string }>; +}; + +const ABSOLUTE_URI = /^[A-Za-z][A-Za-z0-9+.-]*:\/\//; + +function parseStringLiteralish(text: string | undefined): string | undefined { + if (typeof text !== 'string') { + return undefined; + } + const trimmed = text.trim(); + if ((trimmed.startsWith('"') && trimmed.endsWith('"')) || (trimmed.startsWith("'") && trimmed.endsWith("'"))) { + return trimmed.slice(1, -1); + } + return trimmed; +} + +function parseStringListLiteralish(text: string | undefined): string[] | undefined { + if (typeof text !== 'string') { + return undefined; + } + + const trimmed = text.trim(); + if (trimmed === '') { + return undefined; + } + + // Try JSON first (preferred: ["In", "Out"]). + if (trimmed.startsWith('[') && trimmed.endsWith(']')) { + try { + const parsed = JSON.parse(trimmed); + if (Array.isArray(parsed) && parsed.every(x => typeof x === 'string')) { + return parsed as string[]; + } + } catch { + // fall through + } + + // Best-effort: extract quoted strings. + const items: string[] = []; + const re = /"([^"]+)"|'([^']+)'/g; + let m: RegExpExecArray | null; + while ((m = re.exec(trimmed))) { + const s = m[1] ?? m[2]; + if (typeof s === 'string' && s.trim() !== '') { + items.push(s); + } + } + return items.length > 0 ? items : undefined; + } + + return undefined; +} + +function resolveViewerCommandArgs(args: string[] | undefined, targetUri: string, tokenPath: string): string[] | undefined { + if (!args || args.length === 0) { + return undefined; + } + return args.map(value => value + .replace(/\{uri\}/g, targetUri) + .replace(/\{path\}/g, tokenPath) + ); +} + +/** + * The target a declared path names, or undefined when it cannot be resolved. + * + * A relative path means "beside the file this was declared in", so with no such + * file there is no answer — better said than guessed. + */ +function resolveDeclaredTarget( + sourceUri: string | undefined, + declaredPath: string +): string | undefined { + if (ABSOLUTE_URI.test(declaredPath) || declaredPath.startsWith('/')) { + return declaredPath; + } + return sourceUri ? resolveAgainstSource(sourceUri, declaredPath) : undefined; +} + +/** + * What to do with a target, given what it is. + * + * The kind is decided by whoever exported the graph, because that is where the + * filesystem is visible — nothing here can tell a folder from a file by looking + * at the string. + */ +function actionForDeclaredTarget( + target: string, + kind: string | undefined, + viewType: string | undefined +): 'open' | 'openWith' | 'reveal' { + // A directory has no editor to open in; revealing it is the operation that + // exists for one. + if (kind === 'folder') { + return 'reveal'; + } + // No editor is registered for a remote scheme, so openWith dead-ends there. + // Plain open hands it to the host, which launches a browser. + const isRemote = ABSOLUTE_URI.test(target) && !target.startsWith('file:'); + if (isRemote || !viewType) { + return 'open'; + } + return 'openWith'; +} + +/** A path declared in the diagram's own source file, resolved beside it. */ +function resolveAgainstSource(sourceUri: string, relativePath: string): string { + const lastSlash = sourceUri.lastIndexOf('/'); + const directory = lastSlash >= 0 ? sourceUri.slice(0, lastSlash) : sourceUri; + return `${directory}/${relativePath}`; +} + +function fileUriFromFsPath(fsPath: string): string { + const p = fsPath.trim(); + if (p.startsWith('file:')) { + return p; + } + // Assume absolute POSIX path (CLI produces absolute paths on Linux/macOS). + const encoded = encodeURI(p); + return encoded.startsWith('/') ? `file://${encoded}` : encoded; +} + +function collectEdges(root: any): any[] { + const out: any[] = []; + const visit = (el: any): void => { + if (!el) { + return; + } + // Sprotty edges typically have type starting with 'edge:' + if (typeof el.type === 'string' && el.type.startsWith('edge:')) { + out.push(el); + } + const children = el.children as any[] | undefined; + if (Array.isArray(children)) { + for (const c of children) { + visit(c); + } + } + }; + visit(root); + return out; +} + +function incomingEdgesForTargetPort(edges: any[], targetPortId: string): any[] { + return edges.filter(e => (e as any).targetId === targetPortId); +} + +function bestIncomingEdgeForViewerInput(edges: any[], targetPortId: string): any | undefined { + const candidates = incomingEdgesForTargetPort(edges, targetPortId); + if (candidates.length === 0) { + return undefined; + } + + const withToken = candidates.filter(e => { + const v = e?.args?.[WorkflowDiagramMetadata.VIEWER_LAST_TOKEN]; + return lastTokenAsPath(v) !== undefined; + }); + + const stableKey = (e: any): string => { + const astPath = e?.args?.[WorkflowDiagramMetadata.AST_PATH]; + return typeof astPath === 'string' ? astPath : String(e?.id ?? ''); + }; + + const pickFrom = (withToken.length > 0 ? withToken : candidates).slice(); + pickFrom.sort((a, b) => stableKey(a).localeCompare(stableKey(b))); + return pickFrom[pickFrom.length - 1]; +} + +function lastTokenAsPath(value: unknown): string | undefined { + if (typeof value === 'string') { + return value; + } + if (Array.isArray(value) && value.length > 0) { + const last = value[value.length - 1]; + return typeof last === 'string' ? last : undefined; + } + return undefined; +} + +export class ViewerNodeOpener implements NodeOpener { + readonly id = 'viewer'; + + open(node: GModelElement, context: NodeOpenerContext): (Action | Promise)[] | undefined { + if (node.type !== WorkflowDiagramTypes.NODE_EXTERNAL_ACTOR) { + return undefined; + } + const args = (node as unknown as { args?: Args }).args; + if (!args) { + return undefined; + } + const sourceUri = context.sourceUri; + + const defAnnotations = args[WorkflowDiagramMetadata.ENTITY_DEFINITION_ANNOTATIONS] as unknown; + const annotations = Array.isArray(defAnnotations) ? (defAnnotations as DefinitionAnnotation[]) : []; + const viewer = annotations.find(a => a && a.name === 'viewer'); + if (!viewer) { + return undefined; + } + + const argByName = new Map(); + for (const a of viewer.arguments ?? []) { + if (a && typeof a.name === 'string' && typeof a.value === 'string') { + argByName.set(a.name, a.value); + } + } + + const action = ( + parseStringLiteralish(argByName.get('action')) + ?? parseStringLiteralish(argByName.get('action_name')) + ?? 'open' + ).trim(); + const inputs = parseStringListLiteralish(argByName.get('inputs')); + const viewType = parseStringLiteralish(argByName.get('viewType')) + ?? parseStringLiteralish(argByName.get('view_type')); + // Where the target comes from. Everything here has always resolved + // through the last token on an input, which is right for a node whose + // target is something it PRODUCED — it cannot exist before a run. A + // node whose target is a path it declares is openable immediately, and + // must not be told to go and run the workflow first. + const source = parseStringLiteralish(argByName.get('source')) ?? 'token'; + const declaredPath = parseStringLiteralish(argByName.get('path')); + // What the target IS — file, folder, http, url — settled by the + // producer, which can see the filesystem. Nothing here can tell a + // folder from a file by looking at the string. + const resourceKind = parseStringLiteralish(argByName.get('kind')); + const command = parseStringLiteralish(argByName.get('command')) + ?? parseStringLiteralish(argByName.get('command_name')); + const commandArgs = parseStringListLiteralish(argByName.get('args')) + ?? parseStringListLiteralish(argByName.get('command_args')); + const title = parseStringLiteralish(argByName.get('title')); + + const fail = (message: string): (Action | Promise)[] => [ + NavigateToExternalTargetAction.create({ + uri: context.sourceUri ?? 'file:///', + args: { + [VIEWER_ACTION_ARG]: 'error', + [VIEWER_MESSAGE_ARG]: message + } + }) + ]; + + if (source === 'declared') { + if (!declaredPath) { + return fail('@viewer(source="declared"): missing path="...".'); + } + // Relative to the file the diagram was opened from, which is what a + // declaration in that file means. + const target = resolveDeclaredTarget(sourceUri, declaredPath); + if (!target) { + return fail( + `@viewer(source="declared"): cannot resolve the relative path ` + + `"${declaredPath}" — the diagram has no source file to resolve it against.` + ); + } + const declaredAction = actionForDeclaredTarget(target, resourceKind, viewType); + return [ + NavigateToExternalTargetAction.create({ + uri: target, + args: { + [VIEWER_ACTION_ARG]: declaredAction, + ...(declaredAction === 'openWith' + ? { [VIEWER_VIEW_TYPE_ARG]: viewType as string } + : {}) + } + }) + ]; + } + + // Below here every route resolves its target from the last token on an + // input, so an input is what it needs. This guard used to sit above the + // declared branch, where it caught nodes that never wanted a token: a + // node that declares its target has no inputs by nature, so it was told + // to run the workflow first and could never be opened at all. + if (!inputs || inputs.length === 0) { + return fail('@viewer: missing inputs=[...] (run the workflow first, then double-click again).'); + } + + const root: any = (node as any).root; + const edges = root ? collectEdges(root) : []; + + const overlayRunId = root?.args?.['wf:viewerOverlayRunId']; + const overlayOutDir = root?.args?.['wf:viewerOverlayOutDir']; + const hasOverlay = typeof overlayRunId === 'string' && overlayRunId.trim() !== ''; + + const tokenByInputName = new Map(); + for (const inputName of inputs) { + const targetPortId = `${node.id}_port_${inputName}`; + const incoming = bestIncomingEdgeForViewerInput(edges, targetPortId); + const lastToken = incoming?.args?.[WorkflowDiagramMetadata.VIEWER_LAST_TOKEN]; + const tokenPath = lastTokenAsPath(lastToken); + if (tokenPath) { + tokenByInputName.set(inputName, tokenPath); + } + } + + const getTokenOrError = (name: string): string | undefined => { + return tokenByInputName.get(name); + }; + + if (action === 'diff') { + if (inputs.length !== 2) { + return fail('@viewer(action="diff"): requires exactly 2 inputs in inputs=[...].'); + } + const leftPath = getTokenOrError(inputs[0]); + const rightPath = getTokenOrError(inputs[1]); + if (!leftPath || !rightPath) { + if (!hasOverlay) { + return fail(`Viewer diff: no run overlay found. Run the workflow successfully (CLI writes overlays under ${settingsNamespace()}.runOutputDir), then reopen/refresh the diagram.`); + } + return fail(`Viewer diff: missing last-token file(s) for inputs ${inputs.join(', ')} (overlay runId=${String(overlayRunId)}).`); + } + const leftUri = fileUriFromFsPath(leftPath); + const rightUri = fileUriFromFsPath(rightPath); + return [ + NavigateToExternalTargetAction.create({ + uri: leftUri, + args: { + [VIEWER_ACTION_ARG]: 'diff', + [VIEWER_LEFT_URI_ARG]: leftUri, + [VIEWER_RIGHT_URI_ARG]: rightUri, + ...(title ? { [VIEWER_TITLE_ARG]: title } : {}) + } + }) + ]; + } + + // open/openWith: use first input. + const firstInput = inputs[0]; + const tokenPath = getTokenOrError(firstInput); + if (!tokenPath) { + if (!hasOverlay) { + return fail(`Viewer open: no run overlay found. Run the workflow successfully (CLI writes overlays under ${settingsNamespace()}.runOutputDir), then reopen/refresh the diagram.`); + } + const outDirInfo = typeof overlayOutDir === 'string' && overlayOutDir.trim() !== '' ? ` (outDir=${overlayOutDir})` : ''; + return fail(`Viewer open: missing last-token file for input '${firstInput}' (overlay runId=${String(overlayRunId)})${outDirInfo}.`); + } + + const targetUri = fileUriFromFsPath(tokenPath); + + if (action === 'openWith') { + if (!viewType) { + return fail('@viewer(action="openWith"): missing viewType="...".'); + } + return [ + NavigateToExternalTargetAction.create({ + uri: targetUri, + args: { + [VIEWER_ACTION_ARG]: 'openWith', + [VIEWER_VIEW_TYPE_ARG]: viewType + } + }) + ]; + } + + if (action === 'command') { + if (!command) { + return fail('@viewer(action="command"): missing command="...".'); + } + const resolvedArgs = resolveViewerCommandArgs(commandArgs, targetUri, tokenPath); + return [ + NavigateToExternalTargetAction.create({ + uri: targetUri, + args: { + [VIEWER_ACTION_ARG]: 'command', + [VIEWER_COMMAND_ARG]: command, + ...(resolvedArgs + ? { [VIEWER_COMMAND_ARGS_ARG]: JSON.stringify(resolvedArgs) } + : {}) + } + }) + ]; + } + + // Default: open + return [ + NavigateToExternalTargetAction.create({ + uri: targetUri, + args: { + [VIEWER_ACTION_ARG]: 'open' + } + }) + ]; + } +} diff --git a/packages/diagram-client/test/container-parity.test.ts b/packages/diagram-client/test/container-parity.test.ts index d680fda..be1c4c1 100644 --- a/packages/diagram-client/test/container-parity.test.ts +++ b/packages/diagram-client/test/container-parity.test.ts @@ -32,7 +32,8 @@ * states, and the read-only view marker (hides the palette's tools while the view shown is * read-only, bound to itself and as a model-root listener), and the selection trace (its listener, * bound to itself and as a selection listener, and the `SelectionTraceFeedbackCommand` that draws - * the path) — plus one change: `label:boundary:type` is bound to + * the path), and the `@viewer` node opener (`ViewerNodeOpener`, bound as a constant under + * `NodeOpener`) — 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. diff --git a/packages/diagram-client/test/declared-target-resources.test.ts b/packages/diagram-client/test/declared-target-resources.test.ts index 7762b30..6d2ddcf 100644 --- a/packages/diagram-client/test/declared-target-resources.test.ts +++ b/packages/diagram-client/test/declared-target-resources.test.ts @@ -12,6 +12,7 @@ */ import { describe, expect, it } from 'vitest'; import { ViewerMouseListener } from '../src/viewer-mouse-listener'; +import { ViewerNodeOpener } from '../src/viewer-node-opener'; import { WorkflowDiagramTypes, WorkflowDiagramMetadata } from '@dialogram/shared'; const SOURCE_URI = 'file:///w/flow.py'; @@ -41,6 +42,8 @@ function openAt(args: Record, sourceUri: string | undefined): an }; node.root.children = [node]; const listener: any = new ViewerMouseListener(); + // What the stock feature module binds. + listener.contributedOpeners = [new ViewerNodeOpener()]; listener.editorContext = { sourceUri }; return listener.doubleClick(node, {} as MouseEvent)[0]; } diff --git a/packages/diagram-client/test/double-click-opens-one-thing.test.ts b/packages/diagram-client/test/double-click-opens-one-thing.test.ts index 26e0e1c..96ed0e4 100644 --- a/packages/diagram-client/test/double-click-opens-one-thing.test.ts +++ b/packages/diagram-client/test/double-click-opens-one-thing.test.ts @@ -11,6 +11,7 @@ import { afterEach, describe, expect, it } from 'vitest'; import { WorkflowDiagramMetadata, WorkflowDiagramTypes } from '@dialogram/shared'; import { ViewerMouseListener } from '../src/viewer-mouse-listener'; +import { ViewerNodeOpener } from '../src/viewer-node-opener'; const node = (type: string, extra: Record): any => ({ id: 'clause', @@ -25,6 +26,8 @@ const node = (type: string, extra: Record): any => ({ const viewerAnswer = (element: unknown): unknown[] => { const listener: any = new ViewerMouseListener(); + // What the stock feature module binds. + listener.contributedOpeners = [new ViewerNodeOpener()]; listener.editorContext = { sourceUri: 'file:///w/spec/pipeline.md' }; return listener.doubleClick(element, {} as MouseEvent); }; diff --git a/packages/diagram-client/test/fixtures/container-parity.baseline.json b/packages/diagram-client/test/fixtures/container-parity.baseline.json index 4c354ba..504ce8e 100644 --- a/packages/diagram-client/test/fixtures/container-parity.baseline.json +++ b/packages/diagram-client/test/fixtures/container-parity.baseline.json @@ -86,6 +86,11 @@ "op": "rebind", "to": "ViewportPreservingSetModelCommand" }, + { + "id": "Symbol(dialogram.NodeOpener)", + "op": "bind", + "to": "const:ViewerNodeOpener" + }, { "id": "Symbol(stub.ICopyPasteHandler)", "op": "bind", diff --git a/packages/diagram-client/test/node-opener-seam.test.ts b/packages/diagram-client/test/node-opener-seam.test.ts new file mode 100644 index 0000000..c23f237 --- /dev/null +++ b/packages/diagram-client/test/node-opener-seam.test.ts @@ -0,0 +1,114 @@ +/** + * The contribution point for what a double-click on a node opens + * (`node-opener.ts`): an opener a product binds answers first, an opener that + * leaves the node lets the platform open its definition, and the stock + * module binds the `@viewer` opener until the product that has `@viewer` + * binds it itself. + */ +import { describe, expect, it, vi } from 'vitest'; +import { Container, ContainerModule } from 'inversify'; +import { ChangeBoundsTool, EditorContextService, TYPES } from '@eclipse-glsp/client'; +import { NavigateToExternalTargetAction } from '@eclipse-glsp/sprotty'; +import { WorkflowDiagramMetadata, WorkflowDiagramTypes } from '@dialogram/shared'; +import { NodeOpener } from '../src/node-opener'; +import { ViewerMouseListener } from '../src/viewer-mouse-listener'; +import { workflowFeaturesModule } from '../src/stock-features.module'; + +// The stock module starts loading the edge router's WASM; nothing here routes. +vi.mock('../src/libavoid-loader', async (original) => ({ + ...(await original()), + preloadLibavoid: async () => undefined +})); + +const SOURCE = 'file:///w/flows/main.py'; + +/** A task the platform would open the definition of. */ +const task = (type = WorkflowDiagramTypes.NODE_EXTERNAL_ACTOR): any => ({ + id: 'task', + type, + args: { + [WorkflowDiagramMetadata.IS_EXTERNAL_ACTOR]: type === WorkflowDiagramTypes.NODE_EXTERNAL_ACTOR, + [WorkflowDiagramMetadata.REFERENCED_URI]: 'file:///w/flows/tasks.py', + [WorkflowDiagramMetadata.ENTITY_DEFINITION_ANNOTATIONS]: [] + }, + children: [] +}); + +const opener = (id: string, answer: unknown[] | undefined, seen: unknown[] = []): NodeOpener => ({ + id, + open: (node, context) => { + seen.push([node.id, context.sourceUri]); + return answer as never; + } +}); + +function listenerWith(openers: NodeOpener[] | undefined): any { + const listener: any = new ViewerMouseListener(); + listener.editorContext = { sourceUri: SOURCE }; + listener.contributedOpeners = openers; + return listener; +} + +const opens = (listener: any, node = task()) => listener.doubleClick(node, {} as MouseEvent); +const isDefinition = (actions: any[]) => actions.length === 1 && actions[0].uri === 'file:///w/flows/tasks.py'; + +describe('a node opener', () => { + it('answers before the platform, with the node and the diagram’s source', () => { + const seen: unknown[] = []; + const mine = NavigateToExternalTargetAction.create({ uri: 'file:///w/out/report.html' }); + expect(opens(listenerWith([opener('mine', [mine], seen)]))).toEqual([mine]); + expect(seen).toEqual([['task', SOURCE]]); + }); + + it('leaves the node to the platform, which opens its definition, when it answers nothing', () => { + expect(isDefinition(opens(listenerWith([opener('mine', undefined)])))).toBe(true); + expect(isDefinition(opens(listenerWith(undefined)))).toBe(true); + }); + + it('is asked for any node the listener answers for, and the first that answers wins', () => { + const later: unknown[] = []; + const first = NavigateToExternalTargetAction.create({ uri: 'file:///first' }); + const listener = listenerWith([opener('quiet', undefined), opener('first', [first]), opener('later', [], later)]); + expect(opens(listener, task(WorkflowDiagramTypes.NODE_ACTOR))).toEqual([first]); + expect(later).toEqual([]); + }); + + it('is replaced by a later opener of the same id, which keeps its place', () => { + const replaced = NavigateToExternalTargetAction.create({ uri: 'file:///replaced' }); + const listener = listenerWith([opener('viewer', undefined), opener('other', undefined), opener('viewer', [replaced])]); + expect(listener.nodeOpeners().map((o: NodeOpener) => o.id)).toEqual(['viewer', 'other']); + expect(opens(listener)).toEqual([replaced]); + }); +}); + +describe('the openers the listener is given', () => { + function container(...products: ContainerModule[]): Container { + const c = new Container(); + // What the stock module rebinds and the listener injects, as GLSP binds them. + c.bind(ChangeBoundsTool).toSelf().inSingletonScope(); + c.bind(TYPES.IActionDispatcher).toConstantValue({ dispatch: async () => undefined }); + c.bind(EditorContextService).toConstantValue({ sourceUri: SOURCE } as never); + c.load(workflowFeaturesModule, ...products); + return c; + } + // The stock module binds the listener as a mouse listener; resolve one the way the container builds it. + const ids = (c: Container) => (c.resolve(ViewerMouseListener) as any).nodeOpeners().map((o: NodeOpener) => o.id); + + it('are the stock @viewer opener, then a product’s', () => { + expect(ids(container())).toEqual(['viewer']); + expect(ids(container(new ContainerModule(bind => { bind(NodeOpener).toConstantValue(opener('mine', undefined)); })))) + .toEqual(['viewer', 'mine']); + }); + + it('take a product’s viewer opener in place of the stock one', () => { + const mine = opener('viewer', undefined); + const c = container(new ContainerModule(bind => { bind(NodeOpener).toConstantValue(mine); })); + expect((c.resolve(ViewerMouseListener) as any).nodeOpeners()).toEqual([mine]); + }); + + it('are none when nothing is bound', () => { + const c = new Container(); + c.bind(EditorContextService).toConstantValue({ sourceUri: SOURCE } as never); + expect(ids(c)).toEqual([]); + }); +}); diff --git a/packages/diagram-client/test/viewer-declared-target.test.ts b/packages/diagram-client/test/viewer-declared-target.test.ts index ff03ce1..5db1675 100644 --- a/packages/diagram-client/test/viewer-declared-target.test.ts +++ b/packages/diagram-client/test/viewer-declared-target.test.ts @@ -15,6 +15,7 @@ */ import { describe, expect, it } from 'vitest'; import { ViewerMouseListener } from '../src/viewer-mouse-listener'; +import { ViewerNodeOpener } from '../src/viewer-node-opener'; import { WorkflowDiagramTypes, WorkflowDiagramMetadata } from '@dialogram/shared'; const SOURCE_URI = 'file:///w/flow.py'; @@ -44,6 +45,8 @@ function nodeWith(args: AnnotationArg[], inputPorts: string[] = []) { function doubleClick(node: unknown, sourceUri: string | undefined = SOURCE_URI): any[] { const listener: any = new ViewerMouseListener(); + // What the stock feature module binds. + listener.contributedOpeners = [new ViewerNodeOpener()]; listener.editorContext = { sourceUri }; return listener.doubleClick(node, {} as MouseEvent); } From d29a7800b4ca48f049f750830ebc11558c343105 Mon Sep 17 00:00:00 2001 From: Endri Bezati Date: Mon, 9 Nov 2026 08:50:58 +0100 Subject: [PATCH 2/2] fix: what the review of the node opener seam turned up - An empty answer is no answer (the contract named only undefined, but `[]` stopped the loop and blocked the definition), and an opener that throws is skipped with a warning instead of making the node open nothing at all. - The contract says an opener without an id is dropped, and that an opener's answer, unlike the platform's definition, does not stand back for a node whose first view the network navigation opens. - The two definition branches are one (a task, or an actor not marked external), with a test pinning the marked-external actor. - Comments: ABSOLUTE_URI's explanation moved with it into the opener, and the stock module's line for the listener says what it does for every product now. Claude-Session: https://claude.ai/code/session_01EToj6RJRMSSHQiqGNMU9wv --- packages/diagram-client/src/node-opener.ts | 12 ++++-- .../src/stock-features.module.ts | 3 +- .../src/viewer-mouse-listener.ts | 42 +++++++------------ .../diagram-client/src/viewer-node-opener.ts | 6 +++ .../test/node-opener-seam.test.ts | 23 +++++++++- 5 files changed, 55 insertions(+), 31 deletions(-) diff --git a/packages/diagram-client/src/node-opener.ts b/packages/diagram-client/src/node-opener.ts index 7bc52a9..cadfa3b 100644 --- a/packages/diagram-client/src/node-opener.ts +++ b/packages/diagram-client/src/node-opener.ts @@ -11,8 +11,14 @@ * bind(NodeOpener).toConstantValue(myOpener); * * There is one opener per `id`: a later binding for an id replaces the - * earlier one. Openers are asked in the order their ids were first bound; the - * first that answers wins. + * earlier one, and an opener without an id is dropped. Openers are asked in + * the order their ids were first bound; the first that answers with actions + * wins (an empty list is no answer), and one that throws is skipped. + * + * The network navigation listener may open something for the same + * double-click (a node's first view); the platform's definition stands back + * for it, but an opener's answer does not. An opener should leave such nodes + * unless it means to open its own thing beside the view. */ import type { Action, GModelElement } from '@eclipse-glsp/sprotty'; @@ -26,6 +32,6 @@ export interface NodeOpenerContext { export interface NodeOpener { /** What the opener is, for a later binding to replace it. */ readonly id: string; - /** The actions a double-click on `node` runs, or `undefined` to leave it to the platform. */ + /** The actions a double-click on `node` runs, or `undefined` (or none) to leave it to the platform. */ open(node: GModelElement, context: NodeOpenerContext): Array> | undefined; } diff --git a/packages/diagram-client/src/stock-features.module.ts b/packages/diagram-client/src/stock-features.module.ts index f8dbd1f..4749957 100644 --- a/packages/diagram-client/src/stock-features.module.ts +++ b/packages/diagram-client/src/stock-features.module.ts @@ -113,7 +113,8 @@ export const workflowFeaturesModule = new ContainerModule((bind, unbind, isBound // Double-click navigation into nested networks. bindAsService(context, TYPES.MouseListener, WorkflowNetworkNavigationMouseListener); - // Double-click on @viewer external tasks to open/diff the last token files. + // Double-click on a node: what a product's openers open for it, else its + // definition (every product relies on the latter). bindAsService(context, TYPES.MouseListener, ViewerMouseListener); // What a double-click on a `@viewer` task opens (its last tokens), until // the product that has `@viewer` binds it in its own bundle. diff --git a/packages/diagram-client/src/viewer-mouse-listener.ts b/packages/diagram-client/src/viewer-mouse-listener.ts index a3f4785..002b076 100644 --- a/packages/diagram-client/src/viewer-mouse-listener.ts +++ b/packages/diagram-client/src/viewer-mouse-listener.ts @@ -37,12 +37,6 @@ function findContainingEntityNode(element: GModelElement | undefined): GModelEle return undefined; } -/** - * Anything with a scheme is already a complete address — a URL, or a URI the - * host knows how to route. It must not be treated as a path and glued onto a - * directory, which is what turned `https://example.com/x` into - * `file:///w/https://example.com/x`. - */ function isSerializedPosition(value: unknown): value is SerializedPosition { if (!value || typeof value !== 'object') { return false; @@ -192,33 +186,29 @@ export class ViewerMouseListener extends MouseListener implements Ranked { return []; } - // What a product opens for the node, before the platform's own: its definition. + // What a product opens for the node, before the platform's own: its + // definition. An empty answer is no answer; an opener that throws is + // skipped, so it cannot stop the node from opening anything. for (const opener of this.nodeOpeners()) { - const opened = opener.open(node, { sourceUri: this.editorContext.sourceUri }); - if (opened) { - return opened; - } - } - - const isExternal = args[WorkflowDiagramMetadata.IS_EXTERNAL_ACTOR] === true || node.type === WorkflowDiagramTypes.NODE_EXTERNAL_ACTOR; - const sourceUri = this.editorContext.sourceUri; - const definitionNavigationAction = buildDefinitionNavigationAction(node, sourceUri); - if (!isExternal) { - if (node.type !== WorkflowDiagramTypes.NODE_ACTOR) { - return []; + let opened: (Action | Promise)[] | undefined; + try { + opened = opener.open(node, { sourceUri: this.editorContext.sourceUri }); + } catch (error) { + console.warn(`[dialogram] the node opener '${opener.id}' failed`, error); + continue; } - - if (!definitionNavigationAction || leafOpensItself(node)) { - return []; + if (opened && opened.length > 0) { + return opened; } - - return [definitionNavigationAction]; } - if (node.type !== WorkflowDiagramTypes.NODE_EXTERNAL_ACTOR) { + // The definition of a task, or of an actor that is not marked external. + const opensDefinition = node.type === WorkflowDiagramTypes.NODE_EXTERNAL_ACTOR + || (node.type === WorkflowDiagramTypes.NODE_ACTOR && args[WorkflowDiagramMetadata.IS_EXTERNAL_ACTOR] !== true); + if (!opensDefinition) { return []; } - + const definitionNavigationAction = buildDefinitionNavigationAction(node, this.editorContext.sourceUri); return definitionNavigationAction && !leafOpensItself(node) ? [definitionNavigationAction] : []; } } diff --git a/packages/diagram-client/src/viewer-node-opener.ts b/packages/diagram-client/src/viewer-node-opener.ts index c832078..520f6a4 100644 --- a/packages/diagram-client/src/viewer-node-opener.ts +++ b/packages/diagram-client/src/viewer-node-opener.ts @@ -28,6 +28,12 @@ type DefinitionAnnotation = { arguments?: Array<{ name: string; value: string }>; }; +/** + * Anything with a scheme is already a complete address — a URL, or a URI the + * host knows how to route. It must not be treated as a path and glued onto a + * directory, which is what turned `https://example.com/x` into + * `file:///w/https://example.com/x`. + */ const ABSOLUTE_URI = /^[A-Za-z][A-Za-z0-9+.-]*:\/\//; function parseStringLiteralish(text: string | undefined): string | undefined { diff --git a/packages/diagram-client/test/node-opener-seam.test.ts b/packages/diagram-client/test/node-opener-seam.test.ts index c23f237..30e2ba3 100644 --- a/packages/diagram-client/test/node-opener-seam.test.ts +++ b/packages/diagram-client/test/node-opener-seam.test.ts @@ -68,7 +68,7 @@ describe('a node opener', () => { it('is asked for any node the listener answers for, and the first that answers wins', () => { const later: unknown[] = []; const first = NavigateToExternalTargetAction.create({ uri: 'file:///first' }); - const listener = listenerWith([opener('quiet', undefined), opener('first', [first]), opener('later', [], later)]); + const listener = listenerWith([opener('quiet', undefined), opener('first', [first]), opener('later', [first], later)]); expect(opens(listener, task(WorkflowDiagramTypes.NODE_ACTOR))).toEqual([first]); expect(later).toEqual([]); }); @@ -112,3 +112,24 @@ describe('the openers the listener is given', () => { expect(ids(c)).toEqual([]); }); }); + +describe('an opener that answers nothing useful', () => { + it('with an empty list, or by throwing, leaves the node to the next opener and the platform', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined); + const failing: NodeOpener = { id: 'failing', open: () => { throw new Error('boom'); } }; + expect(isDefinition(opens(listenerWith([opener('empty', []), failing])))).toBe(true); + expect(warn).toHaveBeenCalledTimes(1); + warn.mockRestore(); + }); +}); + +describe('the platform’s own answer', () => { + it('opens the definition of a task and of an actor, but not of an actor marked external', () => { + const listener = listenerWith(undefined); + expect(isDefinition(opens(listener, task(WorkflowDiagramTypes.NODE_EXTERNAL_ACTOR)))).toBe(true); + expect(isDefinition(opens(listener, task(WorkflowDiagramTypes.NODE_ACTOR)))).toBe(true); + const marked = task(WorkflowDiagramTypes.NODE_ACTOR); + marked.args[WorkflowDiagramMetadata.IS_EXTERNAL_ACTOR] = true; + expect(opens(listener, marked)).toEqual([]); + }); +});