feat(vscode): #2869 plugin updates (merged with main + lockfile fix) - #2930
feat(vscode): #2869 plugin updates (merged with main + lockfile fix)#2930LeighFinegold wants to merge 20 commits into
Conversation
…actFlow Major rewrite of the VS Code extension from Mermaid-based preview to a full visual architecture editor: - Interactive ReactFlow canvas for editing CALM architectures - Drag-and-drop node palette with workspace building blocks - Standards/guidelines drag-onto-node with dotted "adheres to" edges - Edge direction (unidirectional, bidirectional, none) and line styles - Theme-aware UI following VS Code light/dark/high-contrast themes - Real-time bidirectional sync between JSON editor and canvas - Validation panel with clickable issues - Drill-down navigation into building blocks with breadcrumbs - SVG export, pattern application, and spec generation - Configurable extension packs (calm.packs.enabled, calm.packs.excludeNodes) - Edge metadata (key-value pairs) support
The vscode plugin PR added new dependencies (vite, @dagrejs/dagre, elkjs, lucide-react, ajv-formats, etc.) to calm-plugins/vscode/package.json but the root package-lock.json was not regenerated, so 'npm ci' fails in CI. Regenerated the lockfile from a clean install (rm -rf node_modules package-lock.json && npm install) on Node 26, preserving all platform-specific optional binaries.
The vscode plugin rewrite deleted calm-plugins/vscode/screenshots/, but the build-vscode-screenshots.yml workflow remained. It triggers on changes under that path and fails because its working-directory no longer exists. Removing the orphaned workflow.
Regenerating the lockfile let @docusaurus/theme-mermaid drift to 3.10.2, pulling a second @docusaurus/core + theme-common tree that mismatched the 3.10.1 tree used by the docs preset. This broke 'docs' build with 'useTitleFormatterContext called outside <TitleFormatterProvider>'. Added overrides pinning @docusaurus/core, theme-common and theme-mermaid to 3.10.1 to match upstream main (single consistent tree; faster stays 3.10.2 as on main).
The plugin rewrite removed the test/integration harness and the 'test:integration' script, so the integration-test matrix job failed with 'Missing script: test:integration'. Removed the job and updated package-and-publish needs accordingly.
The rewrite changed the 'package' script to run 'vsce package' without first building, and dropped the 'vscode:prepublish' hook. As a result 'package:vscode' produced no dist/extension.js and vsce failed with 'Extension entrypoint(s) missing'. Restored 'npm run build &&' in the package script and re-added the standard vscode:prepublish hook (matching pre-rewrite behaviour).
rocketstack-matt
left a comment
There was a problem hiding this comment.
Reviewed the rewrite plus the 5 build-fixup commits. The fixups (lockfile regen, docusaurus pin, dead-workflow removal, integration-test job removal, build-before-package fix) are correct — verified by rebuilding and packaging the extension from this branch myself. The findings below are in the carried-over feature code, mostly data-loss/correctness bugs on save plus a couple of path-handling issues, and one open scope question that wasn't explicitly resolved in the #2912 discussion.
| if (!arch || !arch.nodes || arch.nodes.length === 0) { | ||
| setNodes([]); setEdges([]); setLastParsedArch(null); return; | ||
| } | ||
| setLastParsedArch(arch); |
There was a problem hiding this comment.
documentControls defaults to {} and is never hydrated from the loaded document's controls field here. flowToCalm only writes arch.controls back out when documentControls is non-empty, so any document with solution-level controls silently loses them on the first save.
| setLastParsedArch(arch); | |
| setLastParsedArch(arch); | |
| useCanvasStore.setState({ documentControls: ((arch as Record<string, unknown>).controls as Record<string, unknown>) ?? {} }); |
There was a problem hiding this comment.
We shall address and push a fix soon. Thank You.
| } | ||
|
|
||
| // Build edge relationships | ||
| for (const edge of edges) { |
There was a problem hiding this comment.
This loop builds one relationship per edge and reuses data.calmRelId as unique-id for all of them. calm-parser.ts splits a single multi-target interacts (actor + nodes: [B, C]) into N edges that all share one calmRelId — so a document with a multi-target interacts relationship round-trips into N relationships with the same unique-id, which is invalid per the CALM schema. Needs to group edges by calmRelId (for the interacts variant) back into one relationship with multiple nodes before pushing.
There was a problem hiding this comment.
We shall address and push a fix soon. Thank You.
| // Build edges from non-containment relationships | ||
| interface ConnectsEntry { relId: string; src: string; dst: string; protocol?: string; description?: string; controls?: unknown; lineStyle: string; routing: string; edgeMetadata: Record<string, unknown> } | ||
| const connectsList: ConnectsEntry[] = []; | ||
| for (const rel of arch.relationships ?? []) { |
There was a problem hiding this comment.
Only connects, interacts, deployed-in and composed-of are turned into edges here, and calm-editor-transformer.ts rebuilds relationships purely from edges with no fallback for anything else. A relationship with any other relationship-type (e.g. a decorator-style or custom type) never becomes an edge and is silently deleted the first time the document is saved. Needs a passthrough for unrecognized types from lastParsedArch when rebuilding.
There was a problem hiding this comment.
We shall address and push a fix soon. Thank You.
| await vscode.workspace.fs.createDirectory(buildingBlocksDir); | ||
| } | ||
|
|
||
| const fileUri = vscode.Uri.joinPath(buildingBlocksDir, filename); |
There was a problem hiding this comment.
filename comes straight from a webview message with no validation before being joined into building-blocks/. A crafted filename (e.g. ../../foo.ts) writes content outside building-blocks/, potentially outside the workspace entirely — arbitrary file write.
| const fileUri = vscode.Uri.joinPath(buildingBlocksDir, filename); | |
| const safeName = filename.replace(/[/\\]/g, ''); | |
| if (!safeName || safeName !== filename || filename.includes('..')) { | |
| vscode.window.showErrorMessage(`Invalid building block filename: ${filename}`); | |
| return; | |
| } | |
| const fileUri = vscode.Uri.joinPath(buildingBlocksDir, safeName); |
There was a problem hiding this comment.
We shall address and push a fix soon. Thank You.
| metadata: n.metadata, | ||
| containmentType: containerVariant.get(id), | ||
| }, | ||
| ...(parent ? { parentId: parent, extent: 'parent' as const } : {}), |
There was a problem hiding this comment.
Nodes parsed from the document get parentId/extent: 'parent' here but no zIndex, whereas containment.ts#makeContainment (used when a user manually drags a node into a container) explicitly sets zIndex: depth. Without that, a node that's already nested when the file loads can sit at/below its parent container's stacking order, so a drag on the child hits the container underneath instead. This matches manual testing against the TraderX architecture from CALM Hub: some contained nodes aren't draggable — dragging them moves the container (e.g. "Web GUI", "TraderX DB") instead. Needs the same depth-based zIndex assigned on initial parse, not just on manual re-parenting.
There was a problem hiding this comment.
We shall address and push a fix soon. Thank You. As a side note format in calm hub is using draft version schema and in later iteration we will have to support new version when published in's code extension.
| dispose(): void { | ||
| if (this.debounceTimer) clearTimeout(this.debounceTimer); | ||
| } |
There was a problem hiding this comment.
dispose() only clears the debounce timer — this.watchers (pushed in registerWatchers) is never disposed. Since a WorkspaceAssetService is created per CanvasPanel, every open/close cycle leaks 6×N watchers for the life of the extension, and the leaked watchers keep firing rescans against already-closed panels.
| dispose(): void { | |
| if (this.debounceTimer) clearTimeout(this.debounceTimer); | |
| } | |
| dispose(): void { | |
| if (this.debounceTimer) clearTimeout(this.debounceTimer); | |
| for (const watcher of this.watchers) watcher.dispose(); | |
| this.watchers = []; | |
| } |
There was a problem hiding this comment.
We shall address and push a fix soon. Thank You.
| private stem(uri: vscode.Uri): string { | ||
| const base = path.basename(uri.fsPath); | ||
| return base.replace(/\.calm\.json$/, '').replace(/\.md$/, ''); |
There was a problem hiding this comment.
stem() strips .calm.json and .md but not .architecture.json, even though scanBuildingBlocks explicitly globs *.architecture.json files too. Building blocks sourced from an .architecture.json file get an id like foo.architecture.json instead of foo, inconsistent with their .calm.json siblings.
| private stem(uri: vscode.Uri): string { | |
| const base = path.basename(uri.fsPath); | |
| return base.replace(/\.calm\.json$/, '').replace(/\.md$/, ''); | |
| private stem(uri: vscode.Uri): string { | |
| const base = path.basename(uri.fsPath); | |
| return base.replace(/\.(calm|architecture)\.json$/, '').replace(/\.md$/, ''); | |
| } |
There was a problem hiding this comment.
Adding support for .calm.json, .architecture.json, .template.json, .solution.json, .standard.json, .guideline.json types. Whilst we were wondering on how we could potential have node names enforced for consistency. We talk more about that in our WG. Thanks!
| <body> | ||
| <div id="root"></div> | ||
| <script nonce="${nonce}"> | ||
| window.__INITIAL_CALM_JSON__ = ${initialJson}; |
There was a problem hiding this comment.
JSON.stringify doesn't escape </script>. If the opened document's raw text contains that literal substring anywhere (e.g. inside a description field), it closes this script block early and injects the remainder as HTML into the page. The CSP nonce blocks a new <script> from executing, but not the markup injection itself.
| window.__INITIAL_CALM_JSON__ = ${initialJson}; | |
| window.__INITIAL_CALM_JSON__ = ${initialJson.replace(/</g, '\\u003c')}; |
There was a problem hiding this comment.
We shall address and push a fix soon. Thank You.
| this.disposeCallbacks.push(callback); | ||
| } | ||
|
|
||
| dispose(): void { |
There was a problem hiding this comment.
dispose() calls this.panel?.dispose(), which fires onDidDispose synchronously and re-enters dispose() before this.panel = undefined runs. disposeCallbacks isn't cleared, so every registered callback fires twice per panel close. Harmless today since the callbacks look idempotent, but fragile — worth guarding with a private disposed = false flag checked at the top of the method.
There was a problem hiding this comment.
It's harmless today since the callbacks are idempotent, but it's fragile, we will introduce a safe dispose option. Thanks!
| let canvasPanel: CanvasPanel | undefined; | ||
| let outputChannel: vscode.OutputChannel; | ||
|
|
||
| const CALM_FILE_SUFFIXES = [ |
There was a problem hiding this comment.
This narrows file recognition to a fixed suffix list (.calm.json, .architecture.json, etc.), dropping the old plugin's looser matching (a plain architecture.json, or generic JSON under calm/, is no longer picked up). This was flagged by @LeighFinegold in the #2912 discussion as one of the "likely minor" parity gaps but wasn't explicitly agreed as an accepted direction before merge — worth a maintainer decision here rather than merging it as an unstated behavior change.
There was a problem hiding this comment.
We have introduced this consciously to allow for where plugin should be activate for. Happy to discuss this in WG. Thanks!
|
Hello @LeighFinegold for fixes to above noted items, We will have to push changes to our repo. I shall uptake your changes into my code and will push into my PR. Once that's done, I can let you know and you can close this PR. Thank You. |
|
Hello @rocketstack-matt all the above mentioned issues are addressed and push into #2912 . @LeighFinegold you may now close this PR. Thank You. |
Sure. Sounds good to me. |
|
Closing as @byrash now has managed to update his original PR at source. |
Purpose
This raises byrash's CALM Canvas VS Code plugin rewrite (#2912) against
main, merged with the latestmainand with build fixes on top, so CI is green.Raised at byrash's request per the discussion on #2912 (#2912 (comment) and the follow-up), where a maintainer offered to patch the build on top of the original work and byrash agreed ("Please do the required").
All of byrash's original commits are retained. Created via
git worktreeoff the #2912 PR head, merged with latestmain, with fix-ups added on top.Commits
mainfix(deps): regenerate lockfile for vscode plugin dependenciesci(vscode): remove dead screenshots workflowfix(deps): pin docusaurus runtime packages to 3.10.1ci(vscode): drop integration-test job for removed harnessfix(vscode): build extension before packagingFixes
Lockfile out of sync. The rewrite added new deps to
calm-plugins/vscode/package.json(vite,@dagrejs/dagre,elkjs,lucide-react,ajv-formats, and others) but the rootpackage-lock.jsonwas never regenerated, so CI'snpm cifailed. Regenerated viarm -rf node_modules package-lock.json && npm installon Node 26, preserving all platform-specific optional binaries (rollup/oxide/swc/esbuild Linux variants intact forvalidate-lockfile).docsbuild (useTitleFormatterContext called outside <TitleFormatterProvider>). Regenerating the lockfile let@docusaurus/theme-mermaiddrift to3.10.2, pulling a second@docusaurus/core+theme-commontree that mismatched the3.10.1tree used by the docs preset. Added overrides pinning@docusaurus/core,theme-common, andtheme-mermaidto3.10.1(single consistent tree, matchingmain;fasterstays3.10.2as onmain).VS Code packaging (
Extension entrypoint(s) missing). The rewrite changed thepackagescript to runvsce packagewithout building first and dropped thevscode:prepublishhook, sodist/extension.jsdid not exist at package time. Restorednpm run build &&in thepackagescript and re-addedvscode:prepublish(matching pre-rewrite behaviour).Orphaned CI. The rewrite deleted
calm-plugins/vscode/screenshots/and thetest/integrationharness (plus thetest:integrationscript), but the workflows still referenced them:build-vscode-screenshots.ymlworkflow, because itsworking-directory(calm-plugins/vscode/screenshots) no longer exists so every run failed before doing any work.integration-testmatrix job frombuild-vscode-extension.ymland dropped it frompackage-and-publish'sneeds, because the job invoked the now-deletedtest:integrationscript and could only fail withMissing script, which also blocked the packaging job that depended on it.Verification (Node 26)
npm ci: pass (was failing)docsbuild: passpackage:vscode: produces.vsixNote: PR only touches
calm-plugins/plus the root lockfile/overrides and two VS Code CI workflows. Pre-existing docs/calm-guard/calm-studio issues onmainare unrelated.Supersedes / continues #2912. Credit for the feature work goes to @byrash.