Skip to content

Commit 93166f0

Browse files
PR feedback
- McpHttpTransport.dispose: catch per-session close() rejections - McpDiagramHandlerDispatcher: extract interface - harvest: factor out getConstructorList helper + clarify JSDoc - dispatchStaticDiagramRead: clarify "pick first session" - DefaultGLSPMcpServer: inject Logger, drop console.debug - GShapeElement.is: drop; callers use `instanceof` - GEdge.is: deprecate; callers use `instanceof` - Move prompt-handler tests to resolveActiveSessionId spec - Add ElkLayoutModule (subclassable); keep factory wrapper - workflow-server: switch to new ElkLayoutModule
1 parent 98d9088 commit 93166f0

15 files changed

Lines changed: 203 additions & 239 deletions

examples/workflow-server/src/node/app.ts

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@
1515
********************************************************************************/
1616
import 'reflect-metadata';
1717

18-
import { configureELKLayoutModule } from '@eclipse-glsp/layout-elk';
18+
import { ElkLayoutModule } from '@eclipse-glsp/layout-elk';
1919
import { GModelStorage, Logger, SocketServerLauncher, WebSocketServerLauncher, createAppModule } from '@eclipse-glsp/server/node';
2020
import { Container } from 'inversify';
2121

@@ -39,10 +39,9 @@ async function launch(argv?: string[]): Promise<void> {
3939
logger.error('Uncaught exception:', error);
4040
});
4141

42-
const elkLayoutModule = configureELKLayoutModule({ algorithms: ['layered'], layoutConfigurator: WorkflowLayoutConfigurator });
4342
const serverModule = new WorkflowServerModule().configureDiagramModule(
4443
new WorkflowDiagramModule(() => GModelStorage),
45-
elkLayoutModule,
44+
new ElkLayoutModule({ algorithms: ['layered'], layoutConfigurator: WorkflowLayoutConfigurator }),
4645
new WorkflowMcpDiagramModule()
4746
);
4847
const mcpServerModule = new WorkflowMcpServerModule();

packages/graph/src/gedge.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ export class GEdge extends GModelElement {
2929
}
3030

3131
export namespace GEdge {
32+
/** @deprecated Use `object instanceof GEdge` directly — this guard adds no value over the language operator. */
3233
export function is(object: unknown): object is GEdge {
3334
return object instanceof GEdge;
3435
}

packages/graph/src/gshape-element.ts

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -29,12 +29,6 @@ export abstract class GShapeElement extends GModelElement implements GBoundsAwar
2929
[GResizable] = true;
3030
}
3131

32-
export namespace GShapeElement {
33-
export function is(object: unknown): object is GShapeElement {
34-
return object instanceof GShapeElement;
35-
}
36-
}
37-
3832
export class GShapeElementBuilder<G extends GShapeElement> extends GModelElementBuilder<G> {
3933
position(x: number, y: number): this;
4034
position(position: Point): this;

packages/layout-elk/src/di.config.ts

Lines changed: 76 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -13,17 +13,26 @@
1313
*
1414
* SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
1515
********************************************************************************/
16-
import { LayoutEngine, Logger, LoggerFactory, ModelState, NullLogger } from '@eclipse-glsp/server';
16+
import {
17+
BindingTarget,
18+
GLSPModule,
19+
LayoutEngine,
20+
Logger,
21+
LoggerFactory,
22+
ModelState,
23+
NullLogger,
24+
applyBindingTarget
25+
} from '@eclipse-glsp/server';
1726
import ElkConstructor, { LayoutOptions } from 'elkjs/lib/elk.bundled';
18-
import { ContainerModule } from 'inversify';
27+
import { ContainerModule, injectable, interfaces } from 'inversify';
1928
import { DefaultElementFilter, ElementFilter } from './element-filter';
2029
import { ElkFactory, GlspElkLayoutEngine } from './glsp-elk-layout-engine';
2130
import { FallbackLayoutConfigurator, LayoutConfigurator } from './layout-configurator';
2231

2332
type Constructor<T> = new (...args: any[]) => T;
2433

2534
/**
26-
* Configuration options for the {@link configureELKLayoutModule} function.
35+
* Configuration options for the {@link ElkLayoutModule} (and the legacy {@link configureELKLayoutModule} factory).
2736
*/
2837
export interface ElkModuleOptions {
2938
/**
@@ -53,71 +62,90 @@ export interface ElkModuleOptions {
5362
}
5463

5564
/**
56-
* Utility method to create a DI module that provides all necessary bindings to use the {@link GlspElkLayoutEngine} in a node GLSP server
57-
* implementation. A set of configuration options is provided to enable easy customization. In most cases at least
58-
* the custom {@link layoutConfigurator} binding should be provided (in addition to the required `algorithms' property) via these options.
65+
* DI module that provides the bindings needed to use the {@link GlspElkLayoutEngine} in a node GLSP server.
5966
*
60-
* The constructed module is not intended for standalone use cases and only works in combination with a GLSPDiagramModule.
67+
* Subclass and override the `bindXxx()` hooks to customize individual bindings — e.g. to swap the
68+
* layout configurator or element filter without re-implementing the whole module. The module is
69+
* only meaningful in combination with a `GLSPDiagramModule`.
6170
*
62-
* * The following bindings are provided:
63-
* - {@link ILayoutConfigurator}
64-
* - {@link IElementFilter}
65-
* - {@link LayoutEngine}
71+
* Bindings provided:
72+
* - {@link ElementFilter}
73+
* - {@link LayoutConfigurator}
6674
* - {@link ElkFactory}
67-
*
68-
* @param options The configuration options
69-
* @returns A DI module that can be loaded as additional module when configuring a diagram module for a GLSP server.
75+
* - {@link GlspElkLayoutEngine} + {@link LayoutEngine} (toService)
76+
* - Fallback bindings for {@link Logger} and {@link LoggerFactory} if absent.
7077
*/
71-
export function configureELKLayoutModule(options: ElkModuleOptions): ContainerModule {
72-
return new ContainerModule((bind, unbind, isBound, rebind) => {
73-
if (options.elementFilter) {
74-
bind(ElementFilter).to(options.elementFilter).inSingletonScope();
75-
} else {
76-
bind(ElementFilter).to(DefaultElementFilter).inSingletonScope();
77-
}
78+
@injectable()
79+
export class ElkLayoutModule extends GLSPModule {
80+
constructor(protected readonly options: ElkModuleOptions) {
81+
super();
82+
}
7883

79-
if (options.layoutConfigurator) {
80-
bind(LayoutConfigurator).to(options.layoutConfigurator);
81-
} else {
82-
bind(LayoutConfigurator)
83-
.toDynamicValue(context => new FallbackLayoutConfigurator(options.algorithms, options.defaultLayoutOptions))
84-
.inSingletonScope();
84+
protected configure(bind: interfaces.Bind, unbind: interfaces.Unbind, isBound: interfaces.IsBound, rebind: interfaces.Rebind): void {
85+
const context = { bind, unbind, isBound, rebind };
86+
applyBindingTarget(context, ElementFilter, this.bindElementFilter()).inSingletonScope();
87+
applyBindingTarget(context, LayoutConfigurator, this.bindLayoutConfigurator()).inSingletonScope();
88+
applyBindingTarget(context, ElkFactory, this.bindElkFactory());
89+
applyBindingTarget(context, GlspElkLayoutEngine, this.bindGlspElkLayoutEngine()).inSingletonScope();
90+
bind(LayoutEngine).toService(GlspElkLayoutEngine);
91+
this.bindLoggerFallbacks(bind, isBound);
92+
}
93+
94+
protected bindElementFilter(): BindingTarget<ElementFilter> {
95+
return this.options.elementFilter ?? DefaultElementFilter;
96+
}
97+
98+
protected bindLayoutConfigurator(): BindingTarget<LayoutConfigurator> {
99+
if (this.options.layoutConfigurator) {
100+
return this.options.layoutConfigurator;
85101
}
102+
return { dynamicValue: () => new FallbackLayoutConfigurator(this.options.algorithms, this.options.defaultLayoutOptions) };
103+
}
86104

87-
const elkFactory: ElkFactory = () =>
105+
protected bindElkFactory(): BindingTarget<ElkFactory> {
106+
const { algorithms, defaultLayoutOptions, isWebWorker } = this.options;
107+
const factory: ElkFactory = () =>
88108
new ElkConstructor({
89-
algorithms: options.algorithms,
90-
defaultLayoutOptions: options.defaultLayoutOptions,
91-
// The node implementation relied on elkjs' `FakeWorker` to set the `workerFactory`.
92-
// However, since the required file is dynamically loaded and not available in a web-worker context,
93-
// it needs to be mocked manually.
94-
workerFactory: options.isWebWorker ? () => ({ postMessage: () => {} }) as unknown as Worker : undefined
109+
algorithms,
110+
defaultLayoutOptions,
111+
// The node implementation relies on elkjs' `FakeWorker` to set the `workerFactory`. The required file is
112+
// dynamically loaded and not available in a web-worker context, so it has to be mocked manually there.
113+
workerFactory: isWebWorker ? () => ({ postMessage: () => {} }) as unknown as Worker : undefined
95114
});
115+
return { constantValue: factory };
116+
}
96117

97-
bind(ElkFactory).toConstantValue(elkFactory);
98-
99-
bind(GlspElkLayoutEngine)
100-
.toDynamicValue(context => {
101-
const container = context.container;
102-
const factory = container.get<ElkFactory>(ElkFactory);
103-
const filter = container.get<ElementFilter>(ElementFilter);
104-
const configurator = container.get<LayoutConfigurator>(LayoutConfigurator);
105-
const modelState = container.get<ModelState>(ModelState);
118+
protected bindGlspElkLayoutEngine(): BindingTarget<GlspElkLayoutEngine> {
119+
return {
120+
dynamicValue: ctx => {
121+
const factory = ctx.container.get<ElkFactory>(ElkFactory);
122+
const filter = ctx.container.get<ElementFilter>(ElementFilter);
123+
const configurator = ctx.container.get<LayoutConfigurator>(LayoutConfigurator);
124+
const modelState = ctx.container.get<ModelState>(ModelState);
106125
return new GlspElkLayoutEngine(factory, filter, configurator, modelState);
107-
})
108-
.inSingletonScope();
109-
bind(LayoutEngine).toService(GlspElkLayoutEngine);
126+
}
127+
};
128+
}
110129

130+
/** Provide fallbacks so the module works standalone in tests/specs that don't preconfigure logging. */
131+
protected bindLoggerFallbacks(bind: interfaces.Bind, isBound: interfaces.IsBound): void {
111132
if (!isBound(Logger)) {
112133
bind(Logger).to(NullLogger).inSingletonScope();
113134
}
114-
115135
if (!isBound(LoggerFactory)) {
116136
bind(LoggerFactory).toFactory(dynamicContext => (caller: string) => {
117137
const logger = dynamicContext.container.get(Logger);
118138
logger.caller = caller;
119139
return logger;
120140
});
121141
}
122-
});
142+
}
143+
}
144+
145+
/**
146+
* Utility wrapper around {@link ElkLayoutModule} for the common case where no override is needed.
147+
* Prefer subclassing {@link ElkLayoutModule} when individual bindings need to be customized.
148+
*/
149+
export function configureELKLayoutModule(options: ElkModuleOptions): ContainerModule {
150+
return new ElkLayoutModule(options);
123151
}

packages/server-mcp/src/prompts/handlers/describe-diagram-mcp-prompt-handler.spec.ts

Lines changed: 0 additions & 83 deletions
This file was deleted.

packages/server-mcp/src/prompts/handlers/suggest-improvements-mcp-prompt-handler.spec.ts

Lines changed: 0 additions & 60 deletions
This file was deleted.

packages/server-mcp/src/server/glsp-mcp-server.spec.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
* SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
1515
********************************************************************************/
1616

17+
import { NullLogger } from '@eclipse-glsp/server';
1718
import { McpServer, RegisteredResource } from '@modelcontextprotocol/sdk/server/mcp.js';
1819
import { expect } from 'chai';
1920
import * as sinon from 'sinon';
@@ -23,7 +24,7 @@ import { DefaultGLSPMcpServer } from './glsp-mcp-server';
2324
describe('DefaultGLSPMcpServer', () => {
2425
function makeServer(): { wrapper: DefaultGLSPMcpServer; sdk: McpServer } {
2526
const sdk = new McpServer({ name: 'test', version: '1.0.0' }, { capabilities: {} });
26-
const wrapper = new DefaultGLSPMcpServer(sdk, { dataMode: 'tools' });
27+
const wrapper = new DefaultGLSPMcpServer(sdk, { dataMode: 'tools' }, new NullLogger());
2728
return { wrapper, sdk };
2829
}
2930

packages/server-mcp/src/server/glsp-mcp-server.ts

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@
1414
* SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
1515
********************************************************************************/
1616

17-
import { Disposable, McpServerOptions } from '@eclipse-glsp/server';
17+
import { Disposable, Logger, McpServerOptions } from '@eclipse-glsp/server';
1818
import {
1919
McpServer,
2020
RegisteredPrompt,
@@ -103,7 +103,8 @@ export class DefaultGLSPMcpServer implements GLSPMcpServer {
103103

104104
constructor(
105105
protected readonly mcpServer: McpServer,
106-
readonly options: McpServerOptions
106+
readonly options: McpServerOptions,
107+
protected readonly logger: Logger
107108
) {
108109
// `register*` need an interception layer so the local registration log stays in
109110
// sync; the Proxy preserves the SDK's generic signatures (which a wrapped method
@@ -151,7 +152,7 @@ export class DefaultGLSPMcpServer implements GLSPMcpServer {
151152
() =>
152153
this.ping().catch(err => {
153154
if (!(err instanceof McpError) || err.code !== ErrorCode.RequestTimeout) {
154-
console.debug('MCP keep-alive ping failed:', err);
155+
this.logger.debug('MCP keep-alive ping failed:', err);
155156
}
156157
}),
157158
KEEP_ALIVE_INTERVAL_MS

0 commit comments

Comments
 (0)