Bind GED worker for generated diagram plugins (partial fix for #7) - #27
Conversation
…rams Both generatedModelPlugin.ts and generatedModelTransformationPlugin.ts constructed their diagram module with only the services argument, dropping the languageJsUrl that every other language plugin (classic model, classic model-transformation, metamodel) already receives and forwards. Without it, BaseDiagramModule never binds GedWorkerBaseUrl, so MetadataManager.computeGED() always returns undefined regardless of how small the metadata mismatch is, forcing every edit that changes an element's ID onto the naive ID-equality fallback, which drops routing points, positions, and other layout data for anything that doesn't match by identical ID. Converts both plugin definitions into functions that accept languageJsUrl and pass it through, mirroring the existing pattern in plugin.ts (model-transformation), modelPlugin.ts, and the metamodel plugin. The browser-side worker that actually drives live diagram editing already passes languageJsUrl uniformly to every plugin's create() call, so this fix takes effect without further changes. This directly affects issue mde-optimiser#7 (edges not reroutable): once a textual edit shifts a node or edge's ID, the classic pipeline can still recover via GED as long as the affected region is small; the generated pipeline previously had no chance to recover at all. The underlying ID-churn source (position-index-derived IDs for model-transformation statements) is unchanged and can still defeat GED on large diagrams, but this closes the one confirmed gap where recovery was impossible by construction.
The workbench bundle has grown enough (generated model-transformation support, etc.) that a fresh npm run build now hits Node's default heap limit and crashes with SIGABRT. Matches the memory bump already used elsewhere for similarly large builds in this repo.
|
Traced this to the metadata manager's graph edit distance (GED) matching, which every diagram type shares. When a textual edit shifts an element's ID, even one not conceptually related to the edit, the next validation pass can't match old and new metadata by ID anymore and falls back to naive ID-equality matching. Anything that doesn't match gets reset to default layout, including routing points, and since the reset gets written straight to the metadata file, reloading does not bring it back. Found one concrete, confirmed gap: the generated pipeline (.m_gen/.mt_gen) constructs its diagram module without the languageJsUrl argument that every other language plugin already receives and forwards. Without it, the GED worker never gets bound, so GED always returns undefined no matter how small the mismatch is. The generated pipeline had zero chance of correct old-to-new remapping, not just a reduced chance on large diffs. Fixed by threading languageJsUrl through both generated plugin definitions, matching the classic pipelines exactly. What this does not fix: the underlying source of ID churn is still there. Model-transformation statement IDs are derived from their position in their containing block, so inserting or reordering one statement can shift the IDs of everything after it. On the classic pipeline this can still exceed GED's node-difference threshold on large diagrams and fall back to the same naive matching. Making statement identity stable across edits would be the real fix, but that's a bigger, riskier change to the ID generation scheme and felt out of scope here. Verified end to end: created a .mt_gen file with a match pattern, rerouted the link edge to give it a custom bend, then edited the content to insert a new empty match statement before it, the kind of edit that shifts the pattern's IDs. Before this fix the reroute reset to a straight line. After, it kept the custom route. Also included a small unrelated fix: bumped the workbench Docker build's Node heap limit, since a fresh build now OOMs given how much the bundle has grown. For the underlying ID churn, a few questions before I take a pass at it (@nk-coding @szschaler):
Thanks beforehand @nk-coding @szschaler |
|
these are good catches, but do not solve what #7 was originally about
|
|
@nk-coding @szschaler now that the lockup part of #7 is resolved separately (see #28), I would like to nail down the ID churn problem enough to actually start on it, so I do not need to keep coming back with more questions mid-implementation. Looked closer at ModelTransformationModelIdProvider to ground these: getHierarchicalStatementName builds a purely positional ID: the statement's index within its containing array, chained through its parent's name as "{parentName}_{index}". This is used for every statement kind (MatchStatement, IfMatchStatement, WhileMatchStatement, UntilMatchStatement, ForMatchStatement, IfExpressionStatement, WhileExpressionStatement, StopStatement), plus WhereClause and ElseIfBranch through their own index-based methods. Pattern is not itself index-based, but it inherits its name from the containing statement, so the churn cascades into every pattern, pattern link, and pattern object instance nested under a statement whose index shifted, even though PatternLink/PatternObjectInstance names are otherwise based on stable, user-given names. Questions, roughly in the order I would need answers to start:
Thanks beforehand. |
|
without answering all the detailed questions: |
|
That's a good catch on the hash idea specifically: a content hash would make a statement's own ID change the moment you edit its content, which is exactly the common case you're describing, so it would trade a rare problem (reorder/insert shifting siblings) for a worse, constant one (any content edit changing the statement's own ID). That alone is enough to drop that approach. Given that plus your priority read, I'll leave this alone for now rather than pursue a redesign. Won't touch ModelTransformationModelIdProvider unless this actually comes up as a real user complaint later. |
|
I agree, this isn't what #7 was about -- where the edges "changed on their own" without changes to the textual model. If you current implementation fixes that part, then that's what we're after. The rest is of lower priority: if I change the text and the layout changes as a result, I can live with that. |
|
While this fixes a bug (and as such should be merged assuming the checks run through), I'm not sure I understand how it would fix the bug in #7. Can you explain and add a short screenshot showing that this now works? |
There was a problem hiding this comment.
Pull request overview
This PR fixes an identified gap where the generated diagram language plugins (.m_gen / .mt_gen) were not forwarding languageJsUrl into their diagram modules, preventing GedWorkerBaseUrl from being bound and causing the metadata manager’s GED-based remapping to be unavailable (leading to layout/routing resets after textual edits). It also adjusts the workbench Docker build to avoid Node heap OOM during npm run build.
Changes:
- Update
generatedModelPluginProviderandgeneratedModelTransformationPluginProviderto acceptlanguageJsUrland pass it into their respective*DiagramModuleconstructors. - Refactor the generated plugin definitions into factory functions to mirror the existing plugin pattern used by other languages in the repo.
- Increase Node heap size during the workbench Docker build via
NODE_OPTIONS.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| infra/docker/workbench/Dockerfile | Increases Node heap for the build stage to prevent OOM during npm run build. |
| app/packages/language-model/src/generatedModelPlugin.ts | Forwards languageJsUrl into GeneratedModelDiagramModule so GedWorkerBaseUrl can be bound for GED remapping. |
| app/packages/language-model-transformation/src/generatedModelTransformationPlugin.ts | Forwards languageJsUrl into GeneratedModelTransformationDiagramModule for the same GED worker binding/remapping behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Went back to Niklas's original report word for word rather than my own paraphrase of it: "this primarily seems to happen after editing the content beforehand" — so the original report describes text edit, then reroute, then immediate reset. That's a different trigger than "edges changed with no textual change at all," which is how I'd been characterizing it. Worth flagging since those are two different bugs and I want to make sure this PR is actually being judged against the right one. Why this fix addresses that: text edits shift the position-derived IDs of unrelated statements (that part is unavoidable given how IDs are computed today, separate topic). When the next render sees a mismatch between the old and new IDs, MetadataManager falls back to GED to figure out which old element corresponds to which new one, so custom layout (like a reroute) can carry over despite the ID change. GED only runs at all if GedWorkerBaseUrl is bound, and that requires languageJsUrl to be threaded through to the diagram module constructor. The generated pipeline's plugin definitions were never passing it, so GED always returned undefined there and every mismatch fell straight through to the naive reset path, regardless of how small the actual difference was. This PR fixes that wiring so GED gets a real chance to run. Re-tested the literal original scenario, on the classic .mt pipeline this time (not .mt_gen), with this PR's fix in place: Opened a .mt file with a match pattern (p : Person, c : Company, c -- p), rerouted the link to give it a custom bend. Separately, and not something either of you described, so flagging as new information rather than assuming it's related: I also tested what happens across repeated executions of the same unchanged .mt file (re-running rather than editing). Each Run creates a brand new, separately-identified result file rather than updating an existing one, so there is no persisted metadata for a subsequent run to inherit regardless of GED. Custom layout never carries over between separate runs of the same transformation. That's a real, verified behavior, but distinct from what's described in this issue, since neither of you mentioned re-running an execution. Happy to open it separately if it's worth tracking, or drop it if it's expected behavior. |
No it does not. The original issue is about a rarely occuring bug in the model transformation graphical editor. But this is fine, this PR is an improvement regardless and can be merged, but there is still an underlying issue to be identified and solved |
|
@nk-coding Fair - not reproducing it in a couple of tries doesn't prove anything for a rare, action-dependent bug. I overreached calling it "fixed." Agreed this PR stands on its own merits, but the real #7 bug in the classic editor is still open and unidentified. Two things that might actually help, since live testing won't: Any memory of what you were doing right before it happened would help more than another blind attempt from me. |
|
For me, this literally happens when I edit in any of the blended editors: I create a model (using text or diagram, doesn't seem to matter which), then note that an edge is in a bad place, so move it in the diagram. It's then sometimes enough for me to move the mouse without any specific action for the edge to go back to where it was first. |
|
I merge this one as this is definitely a a partial fix, but the whole fix of the core issue (as soon as we identify the root cause) should be in a separate PR |
Relates to #7
What this fixes
Traced the reported "reroute changes reset immediately" behavior to the metadata manager's graph edit distance (GED) matching, shared by every diagram type. When a textual edit changes an element's ID (even one not conceptually related to the edit), the next validation pass can no longer match old and new metadata by ID, and falls back to a naive ID-equality match. Anything that doesn't match gets reset to default layout, including routing points.
Found one confirmed, concrete gap in this:
generatedModelPlugin.tsandgeneratedModelTransformationPlugin.ts(the newer.m_gen/.mt_genpipeline) construct their diagram module with only the services argument, dropping thelanguageJsUrlthat every other language plugin already receives and forwards. Without it,GedWorkerBaseUrlnever gets bound, so GED always returns undefined regardless of how small the mismatch is. That means the generated pipeline has zero chance of correct old-to-new remapping, not just a reduced chance on large diffs.Fixed by converting both plugin definitions into functions that accept and forward
languageJsUrl, mirroring the existing pattern in the classic model-transformation, classic model, and metamodel plugins. The browser-side worker that drives live diagram editing already passeslanguageJsUrluniformly to every plugin'screate()call, so this takes effect without further changes.What this does not fix
The underlying source of ID churn is unchanged: model-transformation statement IDs are derived from their position/index within their containing block (
ModelTransformationModelIdProvider.getHierarchicalStatementName), so inserting or reordering one statement can shift the IDs of everything after it, cascading into new IDs for patterns, pattern links, and control-flow edges that didn't conceptually change. On the classic pipeline this can still exceed GED's node-difference threshold on large diagrams and fall back to the same naive matching. Making statement identity stable across edits would be the real fix, but that's a bigger, riskier change to the ID generation scheme and felt out of scope here.Verification
Reproduced end to end: created a minimal
.mt_genfile with amatchpattern (p : Person,c : Company, linked), rerouted the link edge to give it a custom bend, then edited the underlying content to insert a new empty match statement before it, exactly the kind of edit that shifts the pattern's IDs. Before this fix the reroute reset to a straight line on the next render; after it, the custom route was preserved.Also included a small unrelated fix: bumped the workbench Docker build's Node heap limit, since a fresh build now OOMs given how much the bundle has grown.