Conversation
…ng native MCP support in VSCode
… and build options
🦋 Changeset detectedLatest commit: a175502 The changes in this PR will be included in the next version bump. This PR includes changesets to release 19 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds VSCode MCP stdio integration and registration, an in-memory LastSeenArtifacts cache wired into model building, enhanced element/model-aware hover tooltips (relationship counts and view links), TypeScript composite/project reference updates, and a VSCode build migration to tsdown; plus protocol/type updates and various logging/import adjustments. Changes
Sequence Diagram(s)sequenceDiagram
participant Editor as Editor
participant HoverProv as Hover Provider
participant ModelLoc as Model Locator
participant LastSeen as LastSeen Cache
participant Styles as LikeC4Styles
Editor->>HoverProv: hover request (position)
HoverProv->>ModelLoc: getParsedElement(...)
ModelLoc->>LastSeen: specification(projectId)
LastSeen-->>ModelLoc: MergedSpecification
ModelLoc->>LastSeen: styles(projectId)
LastSeen-->>ModelLoc: LikeC4Styles
ModelLoc-->>HoverProv: parsed element + projectId
HoverProv->>Styles: tagColor(tag)
Styles-->>HoverProv: ColorLiteral
HoverProv-->>Editor: rendered hover (counts, links, markdown)
sequenceDiagram
participant User as User
participant VSCode as VSCode Extension
participant MCPReg as MCP Registration
participant MCPServer as MCP Server (stdio)
participant LangSrv as Language Services
User->>VSCode: activate extension / open workspace
VSCode->>MCPReg: register MCP provider
MCPReg->>VSCode: provide MCP definition (mcp-server.mjs)
VSCode->>MCPServer: launch/attach stdio MCP server
MCPServer->>LangSrv: fromWorkspace(..., { mcp: "stdio" })
LangSrv->>LangSrv: init services (LastSeen, builders)
LangSrv-->>MCPServer: ready notify
MCPServer-->>VSCode: MCP server ready
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In `@apps/docs/tsconfig.json`:
- Around line 9-14: Remove the contradictory "noEmit": true setting from the
tsconfig so that "emitDeclarationOnly": true and "composite": true can produce
declaration outputs for project references; specifically delete the "noEmit" key
(leaving "emitDeclarationOnly", "composite", "incremental", "tsBuildInfoFile",
and "outDir" intact) to match the other composite projects and allow declaration
emission to work.
In `@packages/language-server/src/lsp/HoverProvider.ts`:
- Line 112: Rename the misspelled protected method getElemenModelHover to
getElementModelHover and update all call sites (e.g.,
this.getElemenModelHover(...) on line 99) to the new name; ensure the method
signature (protected getElementModelHover(model: ElementModel, projectId:
ProjectId): string | undefined) remains the same and update any references or
overrides/imports that rely on the old name so compilation and method binding
are preserved.
In `@packages/language-server/src/model/model-locator.ts`:
- Around line 288-291: The error path in locateDocumentTags now returns null
(including the outer catch and early-return branches) which breaks callers
expecting an array; either restore a consistent array return (return []) from
the catch and the early-return branches in locateDocumentTags or update the
function signature to allow null/undefined and then update all callers (e.g.,
any code doing result.map(...) or result.length) to null-check before iterating;
locate the function locateDocumentTags and the early-return branches in that
file and make the return type and runtime values consistent across all paths.
In `@packages/vscode/package.json`:
- Around line 321-333: The dependency entry for p-timeout in
packages/vscode/package.json is pinned to "^6.1.4" but should use the workspace
catalog reference; update the "p-timeout" value to "catalog:utils" so it
resolves via pnpm workspace catalog like the other utilities (look for the
"p-timeout" key in the dependencies block in package.json and replace the
current version string with "catalog:utils").
In `@packages/vscode/src/node/useMcpRegistration.ts`:
- Around line 17-19: The type guard isMcpStdioServerDefinition currently always
returns true and provides no runtime safety; update it to actually discriminate
McpServerDefinition by checking a distinguishing property (e.g., a
transport/kind field or presence of stdio-specific fields) so it only returns
true for vscode.McpStdioServerDefinition, or remove the guard and instead
perform an explicit runtime check before accessing stdio-only properties like
server.env; reference isMcpStdioServerDefinition, McpServerDefinition,
McpStdioServerDefinition, and server.env when making the change so callers
(e.g., the code that reads server.env) are protected.
- Around line 36-68: The extension uses MCP APIs
(vscode.lm.registerMcpServerDefinitionProvider and
vscode.McpStdioServerDefinition) which require VS Code >= 1.102; update the
extension manifest by changing engines.vscode in package.json from "^1.84.0" to
at least "^1.102.0" (or a later semver you target), bump any CI/test matrix and
docs that reference the old minimum, and run npm/yarn install and a quick smoke
build to ensure compatibility with resolveMcpServerDefinition and
provideMcpServerDefinitions usage.
In `@packages/vscode/src/useWorkspaceId.ts`:
- Around line 7-22: The JSDoc incorrectly states the ID is stored in workspace
state while the persistence code is commented out; either restore persistence in
useWorkspaceId by uncommenting and using extensionContext.value?.workspaceState
to read/get/update 'workspaceId' (generate with `likec4-${nanoid(4)}` only when
missing) and test across restarts, or if persistence is intentionally deferred,
remove the unused extensionContext import and update the JSDoc for
useWorkspaceId/createSingletonComposable to say the ID is generated per
activation session (not persisted). Ensure any branch you choose leaves no
unused imports or commented-out persistence code.
In `@packages/vscode/tsdown.config.ts`:
- Around line 11-58: The shared config currently sets clean: true which causes
race conditions because multiple configs write to the same outDir ('dist/node');
change the strategy so only the first config targeting a given outDir runs with
clean: true and all subsequent configs for that outDir use clean: false: update
the shared object (or remove clean from it) and set clean: true explicitly on
the first config that has outDir 'dist/node' (the config with entry
'src/node/extension.ts') and set clean: false on the second config (the config
with entries 'src/node/language-server.ts' and 'src/node/mcp-server.ts') so
parallel builds won’t delete each other’s outputs.
🧹 Nitpick comments (6)
packages/core/build.config.ts (1)
39-41: Consider removing the commented-outtsgoflag instead of leaving dead code.If
tsgois intentionally disabled, remove the comment to keep the config clean. If this is temporary, a TODO comment explaining when to re-enable would be helpful.Proposed fix
- dts: { - // tsgo: true, - }, + dts: {},packages/language-server/src/model/builder/MergedSpecification.ts (1)
53-66: Consider simplifying project ID collection.The current dedup logic only checks against
projectIds[0], so it can accumulate duplicates when there are multiple distinct IDs (e.g.,[A, B, B]). The final result viaonly()is still correct, but the intent would be clearer withunique()(already imported):♻️ Simplified approach
- let projectIds = [] as c4.ProjectId[] for (const doc of docs) { const { c4Specification: spec, c4Globals, c4Imports, } = doc - - let docProjectId = doc.likec4ProjectId - if (isNonNullish(docProjectId)) { - if (projectIds.length === 0 || projectIds[0] !== docProjectId) { - projectIds.push(doc.likec4ProjectId!) - } - } - Object.assign(tags, spec.tags) ... } this.tags = assignTagColors(tags) - - this.projectId = only(projectIds) + const projectIds = unique(docs.map(d => d.likec4ProjectId)) + this.projectId = only(projectIds)packages/vscode/tsdown.config.ts (1)
109-123:process.exit(1)incopyPreviewwill crash the build if vscode-preview isn't built yet.If the build order isn't guaranteed (e.g., running
tsdownbeforevscode-previewis built), this will hard-exit. Consider throwing an error instead so tsdown can report it gracefully, or making the copy optional with a warning in development.Suggested alternative
if (!existsSync(vscodePreview)) { - console.error(`"${vscodePreview}" not found`) - process.exit(1) + throw new Error(`"${vscodePreview}" not found — build vscode-preview first`) }packages/vscode/src/node/mcp-server.ts (1)
29-43: Silent catch hides env var misconfiguration.If
LIKEC4_WORKSPACEcontains malformed JSON, thecatchon line 39 silently discards the error and falls back toprocess.cwd(). A debug/warn log here would help troubleshoot misconfiguration without breaking the flow.Proposed fix
} catch { - // ignore + logger.warn`Failed to parse environment variable ${name}, ignoring` }packages/language-server/src/lsp/HoverProvider.ts (1)
115-116: Spreading iterators just for.lengthis avoidable.
[...model.incoming('direct')].lengthmaterializes the full array only to count items. On large models this allocates unnecessarily. A simple reduce/counter would suffice, though the impact is minimal since hover is user-triggered.Example using a count helper
- const incoming = [...model.incoming('direct')].length - const outgoing = [...model.outgoing('direct')].length + const incoming = count(model.incoming('direct')) + const outgoing = count(model.outgoing('direct'))Where
countis a simple utility:function count(iter: Iterable<unknown>): number { let n = 0 for (const _ of iter) n++ return n }packages/language-server/src/model/model-locator.ts (1)
54-91: Public methodgetParsedElementlacks JSDoc documentation.The other public methods in this class (
locateElement,locateView,locateDocumentTags, etc.) have JSDoc or at least descriptive comments. This refactored method with its overloaded signature—acceptingast.Element,c4.Fqn, or[c4.Fqn, c4.ProjectId]—would benefit from a brief JSDoc explaining the overloads and the semantics of returningnull.As per coding guidelines: "Use JSDoc to document public classes and methods."
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Denis Davydkov <[email protected]>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Denis Davydkov <[email protected]>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Denis Davydkov <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/language-server/src/lsp/HoverProvider.ts`:
- Line 113: The local variable lines is declared without a type (const lines =
[]) which infers any[]; update the declaration in HoverProvider.ts to include an
explicit type instead of any (e.g., string[] if it stores text lines or the
appropriate LSP type such as MarkupContent[] if it stores markup objects) so
TypeScript typing is explicit; locate the const lines declaration in the
HoverProvider class or function and change its declaration to the correct
explicit type and adjust any downstream usages to match that type.
In `@packages/vscode/src/node/useMcpRegistration.ts`:
- Line 25: The computed `workspaceFolders` currently calls
`useWorkspaceFolders()` inside its getter which can create new reactive
subscriptions on each re-evaluation; hoist the composable call out of the
computed by calling `useWorkspaceFolders()` once at top-level (e.g., assign its
return to a local variable like `workspaceFoldersRef`) and then reference that
variable inside the `computed` for `workspaceFolders`, so the computed uses
`workspaceFoldersRef.value?.map(...)` instead of invoking
`useWorkspaceFolders()` each time.
🧹 Nitpick comments (2)
packages/language-server/src/lsp/HoverProvider.ts (1)
115-116: Avoid spreading iterators solely to get length.
[...model.incoming('direct')].lengthallocates a throwaway array. If these iterators can yield many items, consider a counting utility instead.Proposed fix
- const incoming = [...model.incoming('direct')].length - const outgoing = [...model.outgoing('direct')].length + const incoming = reduce(model.incoming('direct'), (acc) => acc + 1, 0) + const outgoing = reduce(model.outgoing('direct'), (acc) => acc + 1, 0)Or a simple helper:
function count(iter: Iterable<unknown>): number { let n = 0 for (const _ of iter) n++ return n }packages/vscode/src/node/useMcpRegistration.ts (1)
50-66: Refactor for composables best practices: hoistuseWorkspaceId()call to setup scope.While
useWorkspaceId()as currently implemented (a simple synchronous ID generator with no reactive dependencies) works correctly when called inside the async callback, it's better to call composables during the setup phase for consistency and clarity. Hoist the call to the top level:Suggested refactoring
export const useMcpRegistration = createSingletonComposable(() => { const { logger } = useExtensionLogger() const onDidChange = useOnDidChangeMcpServer() + const workspaceId = useWorkspaceId() // ... resolveMcpServerDefinition: async (server) => { logger.debug`Resolving MCP server ${server.label}` if (server.label === 'likec4' && isMcpStdioServerDefinition(server)) { - const workspaceId = useWorkspaceId() logger.debug(`Resolved MCP server`, { args: server.args, workspaceFolders: workspaceFolders.value, workspaceId, })
| } | ||
|
|
||
| protected getElementModelHover(model: ElementModel, projectId: ProjectId): string | undefined { | ||
| const lines = [] |
There was a problem hiding this comment.
Add an explicit type annotation to lines.
const lines = [] will be inferred as any[]. Per project guidelines, TypeScript should use explicit types and avoid any.
Proposed fix
- const lines = []
+ const lines: string[] = []As per coding guidelines, "TypeScript must use explicit types and avoid using any".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const lines = [] | |
| const lines: string[] = [] |
🤖 Prompt for AI Agents
In `@packages/language-server/src/lsp/HoverProvider.ts` at line 113, The local
variable lines is declared without a type (const lines = []) which infers any[];
update the declaration in HoverProvider.ts to include an explicit type instead
of any (e.g., string[] if it stores text lines or the appropriate LSP type such
as MarkupContent[] if it stores markup objects) so TypeScript typing is
explicit; locate the const lines declaration in the HoverProvider class or
function and change its declaration to the correct explicit type and adjust any
downstream usages to match that type.
davydkov
force-pushed
the
hover-with-details
branch
from
20b8241 to
105ab78
Compare
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Denis Davydkov <[email protected]>
davydkov
force-pushed
the
hover-with-details
branch
from
ff9f204 to
a175502
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@packages/vscode/package.json`:
- Around line 302-307: The package.json currently lists "std-env" under
"dependencies" but it is only used at build time (imported in tsdown.config.ts
for the isProduction flag); remove "std-env" from the "dependencies" section and
add it to "devDependencies" instead so it is installed only for
development/build tasks, updating the package.json entry for "std-env"
accordingly and ensuring any package-lock or lockfile changes are regenerated
(e.g., run install) so the lockfile reflects the move.
In `@packages/vscode/src/node/useMcpRegistration.ts`:
- Around line 21-27: There is a duplicated, incomplete declaration of
useMcpRegistration that opens createSingletonComposable twice; remove the first
(incomplete) duplicate so there is only one export const useMcpRegistration =
createSingletonComposable(...) declaration, ensuring the single block properly
includes the existing const { logger } = useExtensionLogger() and const
onDidChange = useOnDidChangeMcpServer() lines and is closed with the matching
brace/parentheses. This will eliminate the parse error caused by the unclosed
outer block.
- Around line 55-63: The call to useWorkspaceId() inside the async callback
resolveMcpServerDefinition can fail because createSingletonComposable
composables must be invoked in the setup scope; hoist the useWorkspaceId() call
into the setup scope of useMcpRegistration (same place where useWorkspaceFolders
is obtained) so you store workspaceId (or workspaceIdRef) once during setup and
then reference that stored value inside resolveMcpServerDefinition instead of
calling useWorkspaceId() there.
🧹 Nitpick comments (3)
packages/language-server/src/lsp/HoverProvider.ts (3)
124-131:printViewLinkis a closure capturingprojectId— consider making it a local helper with explicit params for clarity.This is minor. The closure over
projectIdworks, but if this method grows, making the dependency explicit (passingprojectIdas an argument) would improve readability. Current state is fine.
142-147: View deduplication logic is correct but relies on reference equality.
v.viewOf === modelcompares by reference, which works because both come from the sameLikeC4Model.Computedinstance. If the model were ever reconstructed between calls, this could silently break. Consider comparing by ID (v.viewOf.id === model.id) for added robustness.Proposed fix
- if (v.isScopedElementView() && v.viewOf === model) { + if (v.isScopedElementView() && v.viewOf.id === model.id) {
25-81: Per coding guidelines, the if-chain could be aswitch(true).Lines 27–74 form a long if-else chain on AST node type. The project guidelines prefer
switch(true)over if-else chains. This is a stylistic nit given the early-return pattern already reads clearly.
| "dependencies": { | ||
| "bundle-require": "catalog:externals", | ||
| "std-env": "catalog:externals", | ||
| "esbuild": "catalog:esbuild", | ||
| "fdir": "catalog:externals", | ||
| "chokidar": "catalog:externals" | ||
| "std-env": "catalog:externals" | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if std-env is imported at runtime in the extension source
rg -n "std-env|isProduction|isDevelopment|isTest|isCI|isDebug" packages/vscode/src/ --type ts -lRepository: likec4/likec4
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Also check for std-env in the entire vscode package to understand the context
rg -n "std-env" packages/vscode/ --type json --type ts --type js -B 2 -A 2Repository: likec4/likec4
Length of output: 684
🏁 Script executed:
#!/bin/bash
# Check the vsce configuration and build scripts in package.json
cat -n packages/vscode/package.json | head -50Repository: likec4/likec4
Length of output: 1902
Move std-env to devDependencies — it's only used at build time.
std-env is imported only in packages/vscode/tsdown.config.ts (for the isProduction flag during the build process), not in any runtime source code. Since it's exclusively a build-time dependency, it should be in devDependencies, not dependencies.
🤖 Prompt for AI Agents
In `@packages/vscode/package.json` around lines 302 - 307, The package.json
currently lists "std-env" under "dependencies" but it is only used at build time
(imported in tsdown.config.ts for the isProduction flag); remove "std-env" from
the "dependencies" section and add it to "devDependencies" instead so it is
installed only for development/build tasks, updating the package.json entry for
"std-env" accordingly and ensuring any package-lock or lockfile changes are
regenerated (e.g., run install) so the lockfile reflects the move.
1 participant
... with some extra data and links to views
Summary by CodeRabbit
New Features
Refactor
Documentation
Chores