fix: clean up plugin loading and fix VSCode extension #312
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughCentralizes document loading and plugin discovery into the language package: adds a dedicated Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Changes introduce new loading and plugin-resolution logic, modify public API exports, and remove previously distributed plugin handling across multiple modules; review requires checking new import/resolution strategies, validation rules, and integration points. Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
|
Claude finished @ymc9's task —— View job Code Review: Plugin Loading Cleanup and VSCode Extension FixesI've conducted a thorough review of this PR which refactors plugin loading and fixes VSCode extension issues. Here's my analysis: Todo List:
🔍 Review SummaryThis PR successfully refactors plugin loading logic and improves the VSCode extension. The changes are well-structured with good separation of concerns. ✅ Strengths1. Excellent Code Organization
2. Robust Plugin Discovery
3. VSCode Extension Improvements
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors plugin model loading to occur during document build (fixing plugin resolution issues, especially in the VSCode extension) and cleans up related APIs and usage.
- Move plugin discovery/loading from the workspace manager into a build-phase hook
- Introduce getPluginDocuments to resolve plugin model files from provider settings
- Split document-loading logic into a new document module and simplify CLI usage
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| samples/blog/zenstack/schema.zmodel | Comment update clarifying provider path usage in pnpm layouts |
| packages/language/src/zmodel-workspace-manager.ts | Remove plugin discovery/loading; keep stdlib loading only |
| packages/language/src/utils.ts | Add getPluginDocuments and findUp utilities; update imports |
| packages/language/src/module.ts | Add build-phase hook to auto-load plugin models; add createZModelServices; optional logging |
| packages/language/src/index.ts | Re-export loadDocument from new document module; export module APIs |
| packages/language/src/document.ts | New: encapsulate document loading/building/validation and import merging |
| packages/ide/vscode/src/language-server/main.ts | Use new createZModelLanguageServices signature and enable console logging |
| packages/ide/vscode/package.json | Add watch script |
| packages/cli/src/actions/action-utils.ts | Simplify schema loading; rely on automatic plugin loading |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/language/src/zmodel-workspace-manager.ts (1)
38-51: ESM safety: replace require.resolve with createRequire/import.meta.resolveIn ESM,
requireis undefined. This path will throw in VS Code extension runtimes. UsecreateRequireorimport.meta.resolvewith a CJS fallback.Apply:
+import { createRequire } from 'node:module'; @@ - const languagePackagePath = require.resolve('@zenstackhq/language/package.json', { - paths: [folderPath], - }); + // Resolve from the workspace folder in both ESM and CJS + let languagePackagePath: string; + if (typeof import.meta !== 'undefined' && typeof (import.meta as any).resolve === 'function') { + languagePackagePath = fileURLToPath( + (import.meta as any).resolve('@zenstackhq/language/package.json', pathToFileURL(folderPath)), + ); + } else { + const req = createRequire(path.join(folderPath, 'package.json')); + languagePackagePath = req.resolve('@zenstackhq/language/package.json'); + }
🧹 Nitpick comments (11)
packages/language/src/zmodel-workspace-manager.ts (1)
48-49: Gate console loggingThese logs can spam users. Gate behind a flag or the LSP connection logger.
Example:
- console.log(`Found installed zenstack package stdlib at: ${installedStdlibPath}`); + if (process.env.ZENSTACK_LOG === '1') { + console.log(`Found installed zenstack package stdlib at: ${installedStdlibPath}`); + } @@ - console.log(`Using bundled stdlib in extension:`, stdLibPath); + if (process.env.ZENSTACK_LOG === '1') { + console.log(`Using bundled stdlib in extension: ${stdLibPath}`); + }Also applies to: 66-67
packages/cli/src/actions/action-utils.ts (1)
1-11: Prune unused imports after centralizing loadDocumentNow that plugin loading is handled in the language services, these imports are unused in this file.
Apply:
-import { loadDocument, type ZModelServices } from '@zenstackhq/language'; -import { isDataSource, isPlugin, Model } from '@zenstackhq/language/ast'; -import { getLiteral } from '@zenstackhq/language/utils'; +import { loadDocument } from '@zenstackhq/language'; +import { isDataSource } from '@zenstackhq/language/ast'; @@ -import { fileURLToPath } from 'node:url'; @@ -import { PLUGIN_MODULE_NAME } from '../constants';packages/language/src/utils.ts (1)
578-631: Deduplicate results to avoid repeated indexing of the same pluginIf multiple plugin declarations resolve to the same file, we may push duplicates.
Apply:
-export function getPluginDocuments(model: Model, schemaPath: string): string[] { +export function getPluginDocuments(model: Model, schemaPath: string): string[] { // traverse plugins and collect "plugin.zmodel" documents - const result: string[] = []; + const result = new Set<string>(); @@ - if (pluginModelFile && fs.existsSync(pluginModelFile)) { - result.push(pluginModelFile); + if (pluginModelFile && fs.existsSync(pluginModelFile)) { + result.add(path.resolve(pluginModelFile)); } } - return result; + return Array.from(result); }packages/language/src/index.ts (1)
1-2: Consider surfacing utils from the package rootTo discourage deep imports (
@zenstackhq/language/utils), consider re-exporting selected utilities (e.g.,getPluginDocuments,getLiteral) here.Example:
export { loadDocument } from './document'; export * from './module'; +export * from './utils'; // if you want stable access to helperspackages/language/src/module.ts (1)
95-118: Plugin doc loading timing is sensible; add dedupe and optional buildGood use of Parsed-phase to prime the index before linking. Two recommendations:
- Deduplicate plugin files across docs.
- Optionally schedule a build for plugin docs (link/validate) if they contain cross-refs.
Apply:
- shared.workspace.DocumentBuilder.onBuildPhase(DocumentState.Parsed, async (documents) => { + const loadedPlugins = new Set<string>(); + shared.workspace.DocumentBuilder.onBuildPhase(DocumentState.Parsed, async (documents) => { for (const doc of documents) { @@ - const pluginSchemas = getPluginDocuments(doc.parseResult.value as Model, schemaPath); + const pluginSchemas = getPluginDocuments(doc.parseResult.value as Model, schemaPath); for (const plugin of pluginSchemas) { + if (loadedPlugins.has(plugin)) { + continue; + } // load the plugin model document const pluginDoc = await shared.workspace.LangiumDocuments.getOrCreateDocument( URI.file(path.resolve(plugin)), ); // add to indexer so the plugin model's definitions are globally visible shared.workspace.IndexManager.updateContent(pluginDoc); + // Optionally: schedule a build for pluginDoc on next cycle + // await shared.workspace.DocumentBuilder.build([pluginDoc]); if (logToConsole) { console.log(`Loaded plugin model: ${plugin}`); } + loadedPlugins.add(plugin); } } });Please confirm linking succeeds when a user file references declarations defined only in a plugin model (no build of plugin doc). If you see unresolved refs, turn on the optional build line above.
packages/language/src/document.ts (6)
21-28: Make extension check case-insensitive and more informative.Lowercase both sides and include allowed extensions in the error for better DX.
- const extensions = services.LanguageMetaData.fileExtensions; - if (!extensions.includes(path.extname(fileName))) { + const allowed = services.LanguageMetaData.fileExtensions.map((e) => e.toLowerCase()); + const ext = path.extname(fileName).toLowerCase(); + if (!allowed.includes(ext)) { return { success: false, - errors: ['invalid schema file extension'], + errors: [`invalid schema file extension: ${ext}. Allowed: ${allowed.join(', ')}`], warnings: [], }; }
53-55: Load imported and additional documents concurrently.Avoid sequential awaits; cut IO latency with Promise.all.
- const importedDocuments: LangiumDocument[] = []; - for (const uri of importedURIs) { - importedDocuments.push(await langiumDocuments.getOrCreateDocument(uri)); - } + const importedDocuments = await Promise.all( + importedURIs.map((uri) => langiumDocuments.getOrCreateDocument(uri)) + ); @@ - const additionalDocs = await Promise.all( - additionalModelFiles.map((file) => - services.shared.workspace.LangiumDocuments.getOrCreateDocument(URI.file(path.resolve(file))), - ), - ); + const additionalDocs = await Promise.all( + additionalModelFiles.map((file) => + services.shared.workspace.LangiumDocuments.getOrCreateDocument( + URI.file(path.resolve(file)) + ) + ) + );Also applies to: 60-65
74-78: Avoid magic numbers for diagnostic severity.Use DiagnosticSeverity enum for clarity and safety.
- const diagnostics = langiumDocuments.all + import { DiagnosticSeverity } from 'vscode-languageserver-types'; + const diagnostics = langiumDocuments.all .flatMap((doc) => (doc.diagnostics ?? []).map((diag) => ({ doc, diag }))) - .filter(({ diag }) => diag.severity === 1 || diag.severity === 2) + .filter( + ({ diag }) => + diag.severity === DiagnosticSeverity.Error || + diag.severity === DiagnosticSeverity.Warning + ) .toArray(); @@ - if (diag.severity === 1) { + if (diag.severity === DiagnosticSeverity.Error) { errors.push(message); } else { warnings.push(message); }Also applies to: 88-94
125-129: Return the existing model variable (nit).Small clarity win; same object reference.
- return { - success: true, - model: document.parseResult.value as Model, - warnings, - }; + return { success: true, model, warnings };
14-20: Consider accepting services/logging via params.Allow passing an existing services instance and/or logToConsole to avoid re‑creating services per call and to aid testing.
Would you like a follow-up diff to add optional params: loadDocument(fileName, additionalModelFiles, opts: { services?: ZModelLanguageServices; logToConsole?: boolean })?
184-202: Enrich post‑merge validation errors with context.Include names (and counts) of offending declarations to aid debugging.
- if (dataSources.length === 0) { - errors.push('Validation error: schema must have a datasource declaration'); + if (dataSources.length === 0) { + errors.push('Validation error: schema must have a datasource declaration'); } else { - if (dataSources.length > 1) { - errors.push('Validation error: multiple datasource declarations are not allowed'); + if (dataSources.length > 1) { + const names = dataSources.map((d) => d.name).filter(Boolean).join(', '); + errors.push( + `Validation error: multiple datasource declarations are not allowed (${dataSources.length})` + + (names ? `: ${names}` : '') + ); } } @@ - if (authDecls.length > 1) { - errors.push('Validation error: Multiple `@@auth` declarations are not allowed'); + if (authDecls.length > 1) { + const names = authDecls.map((d) => d.name).filter(Boolean).join(', '); + errors.push( + `Validation error: Multiple \`@@auth\` declarations are not allowed (${authDecls.length})` + + (names ? `: ${names}` : '') + ); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/cli/src/actions/action-utils.ts(2 hunks)packages/ide/vscode/package.json(1 hunks)packages/ide/vscode/src/language-server/main.ts(1 hunks)packages/language/src/document.ts(1 hunks)packages/language/src/index.ts(1 hunks)packages/language/src/module.ts(4 hunks)packages/language/src/utils.ts(4 hunks)packages/language/src/zmodel-workspace-manager.ts(1 hunks)samples/blog/zenstack/schema.zmodel(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
{packages,samples,tests}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place packages only under
packages/,samples/, ortests/
Files:
packages/ide/vscode/package.jsonpackages/language/src/document.tspackages/cli/src/actions/action-utils.tspackages/language/src/zmodel-workspace-manager.tspackages/language/src/module.tssamples/blog/zenstack/schema.zmodelpackages/language/src/utils.tspackages/ide/vscode/src/language-server/main.tspackages/language/src/index.ts
packages/ide/vscode/package.json
📄 CodeRabbit inference engine (CLAUDE.md)
The
ide/vscodepackage is intentionally versioned differently from other packages
Files:
packages/ide/vscode/package.json
**/schema.zmodel
📄 CodeRabbit inference engine (CLAUDE.md)
Name ZModel schema files
schema.zmodel
Files:
samples/blog/zenstack/schema.zmodel
🧬 Code graph analysis (5)
packages/language/src/document.ts (4)
packages/language/src/module.ts (1)
createZModelServices(124-126)packages/language/src/constants.ts (1)
STD_LIB_MODULE_NAME(21-21)packages/language/src/utils.ts (5)
resolveImport(397-415)getDocument(569-576)resolveTransitiveImports(366-368)getDataModelAndTypeDefs(430-437)hasAttribute(53-55)packages/language/src/generated/ast.ts (1)
isDataSource(417-419)
packages/cli/src/actions/action-utils.ts (1)
packages/language/src/document.ts (1)
loadDocument(14-130)
packages/language/src/module.ts (2)
packages/language/src/utils.ts (1)
getPluginDocuments(578-631)packages/language/src/generated/ast.ts (2)
Model(559-563)Model(565-565)
packages/language/src/utils.ts (3)
packages/cli/src/actions/action-utils.ts (1)
getPluginDocuments(53-108)packages/language/src/generated/ast.ts (3)
Model(559-563)Model(565-565)isPlugin(628-630)packages/language/src/constants.ts (1)
PLUGIN_MODULE_NAME(26-26)
packages/ide/vscode/src/language-server/main.ts (1)
packages/language/src/module.ts (1)
createZModelLanguageServices(78-121)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-test (20.x, sqlite)
- GitHub Check: build-test (20.x, postgresql)
- GitHub Check: claude-review
🔇 Additional comments (6)
packages/ide/vscode/package.json (1)
14-14: LGTM! Standard dev convenience.The watch script enables live rebuilding during development, which aligns with the PR's focus on improving the development workflow for the VS Code extension.
packages/ide/vscode/src/language-server/main.ts (1)
10-16: LGTM! Plugin loading visibility enabled.Passing
truefor thelogToConsoleparameter enables console logging when plugin models are loaded. This is appropriate for the VS Code extension context where this visibility aids debugging and troubleshooting of plugin discovery.packages/cli/src/actions/action-utils.ts (2)
39-51: loadSchemaDocument change looks goodDelegating to
loadDocumentaligns with the new centralized build flow and reduces CLI responsibility. No issues spotted.
53-108: Remove only getPluginDocuments; keep findUp as it's still needed by getPkgJsonConfigThe verification confirms
getPluginDocuments(lines 53-108) is dead code and should be removed. However, the suggestion to also removefindUp(lines 155-178) is incorrect. ThefindUphelper is still actively used bygetPkgJsonConfigat line 134, which is exported and imported bypackages/cli/src/actions/generate.ts. RemovingfindUpwould break that function.Action: Remove only
getPluginDocuments(lines 53-108). Leave thefindUphelper function intact.Likely an incorrect or invalid review comment.
packages/language/src/document.ts (2)
41-45: Consider new URL approach for ESM, but verify asset packaging is configured.The review suggestion to use
new URL(..., import.meta.url)is valid ESM best practice and avoids __dirname pitfalls in bundled contexts. However, verification shows a potential gap:tsup.config.tshas no asset-copying configuration. Whilepackage.jsonincludesresin itsfilesarray (making it part of the npm package), the builtdist/directory will not automatically containstdlib.zmodelwithout explicit configuration.The current
__dirnamefallback approach works because assets exist at the package root, but for bundled/VSIX scenarios to work reliably:
- Either update
tsup.config.tsto copy/includeres/in the build, or- Ensure the VSIX packaging/build process includes the
res/directory in bundled outputsNote: This pattern appears in two locations—
document.ts:41andzmodel-workspace-manager.ts:63(fallback)—both would need updating if implementing the new URL approach.
110-113: Add null-guard for $document; unnecessary to add URI deduplication layer.The code uses a non-null assertion on
$document, but the codebase standard (e.g., schema-validator.ts:31–32) guards with optional chaining. Apply$document?.urito prevent crashes on null values.However, the suggested Set-based URI deduplication is unnecessary. The
resolveTransitiveImportsInternalfunction already deduplicates usingvisited: Set<string>to track processed paths andmodels: Set<Model>to collect unique models, returningArray.from(models), which prevents duplicate Model objects from being processed. The extra URI deduplication adds complexity without addressing a real risk.Simpler fix: Replace
model.$document!.uriwithmodel.$document?.uriin both calls, and skip the Set wrapper:- imported.forEach((model) => { - langiumDocuments.deleteDocument(model.$document!.uri); - services.shared.workspace.IndexManager.remove(model.$document!.uri); - }); + imported.forEach((model) => { + if (model.$document?.uri) { + langiumDocuments.deleteDocument(model.$document.uri); + services.shared.workspace.IndexManager.remove(model.$document.uri); + } + });Likely an incorrect or invalid review comment.
|
Claude finished @ymc9's task —— View job Code Review: Plugin Loading Cleanup and VSCode Extension FixesI've conducted a thorough review of this PR which refactors plugin loading and fixes VSCode extension issues. Here's my analysis: Todo List:
🔍 Review SummaryThis PR successfully refactors plugin loading logic and improves the VSCode extension. The changes are well-structured with good separation of concerns. ✅ Strengths1. Excellent Code Organization
2. Robust Plugin Discovery
3. VSCode Extension Improvements
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/language/src/document.ts (1)
66-72: Guard DocumentBuilder.build to return structured failures instead of throwing.Uncaught throws can crash CLI/extension; return { success: false, errors } instead.
- await services.shared.workspace.DocumentBuilder.build([stdLib, ...additionalDocs, document, ...importedDocuments], { - validation: { - stopAfterLexingErrors: true, - stopAfterParsingErrors: true, - stopAfterLinkingErrors: true, - }, - }); + try { + await services.shared.workspace.DocumentBuilder.build( + [stdLib, ...additionalDocs, document, ...importedDocuments], + { + validation: { + stopAfterLexingErrors: true, + stopAfterParsingErrors: true, + stopAfterLinkingErrors: true, + }, + } + ); + } catch (e) { + return { + success: false, + errors: [`Document build failed: ${e instanceof Error ? e.message : String(e)}`], + warnings: [], + }; + }packages/language/src/utils.ts (1)
611-620: Type-safe feature detection for import.meta.resolve.
resolveis not in ImportMeta types on Node <22; cast to any to avoid TS error.- if (!pluginModelFile) { - if (typeof import.meta.resolve === 'function') { + if (!pluginModelFile) { + const im: any = import.meta as any; + if (typeof im.resolve === 'function') { try { // try loading as a ESM module - const resolvedUrl = import.meta.resolve(`${provider}/${PLUGIN_MODULE_NAME}`); + const resolvedUrl = im.resolve(`${provider}/${PLUGIN_MODULE_NAME}`); pluginModelFile = fileURLToPath(resolvedUrl); } catch { // noop } } }
🧹 Nitpick comments (6)
packages/cli/src/actions/action-utils.ts (1)
4-4: Optional: avoid String.prototype patching by using colors/safe.Prevents global side effects in library/CLI contexts.
-import colors from 'colors'; +import colors from 'colors/safe';packages/language/src/document.ts (2)
22-28: Minor: make file extension check case-insensitive.Handles rare uppercase extensions on Windows.
- if (!extensions.includes(path.extname(fileName))) { + if (!extensions.includes(path.extname(fileName).toLowerCase())) {
125-129: Nit: return themodelvariable for clarity.Same object, but avoids re-reading from document.
- model: document.parseResult.value as Model, + model,packages/language/src/utils.ts (2)
632-637: Deduplicate discovered plugin documents.Avoid loading the same plugin model multiple times.
- const result: string[] = []; + const seen = new Set<string>(); + const result: string[] = []; @@ - if (pluginModelFile && fs.existsSync(pluginModelFile)) { - result.push(pluginModelFile); + if (pluginModelFile && fs.existsSync(pluginModelFile)) { + const abs = path.resolve(pluginModelFile); + if (!seen.has(abs)) { + seen.add(abs); + result.push(abs); + } }
596-607: Optional: widen provider path handling.Consider checking package.json "exports"/"main" when provider is a folder to better locate plugin.zmodel for ESM packages.
Would you like a small helper to read nearest package.json and probe
${pkgRoot}/${PLUGIN_MODULE_NAME}?packages/language/src/module.ts (1)
104-106: Use URI.fsPath instead of fileURLToPath(toString()).Simpler and avoids string round‑trips.
- const schemaPath = fileURLToPath(doc.uri.toString()); + const schemaPath = doc.uri.fsPath;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/cli/src/actions/action-utils.ts(2 hunks)packages/language/src/document.ts(1 hunks)packages/language/src/module.ts(4 hunks)packages/language/src/utils.ts(4 hunks)samples/blog/zenstack/schema.zmodel(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- samples/blog/zenstack/schema.zmodel
🧰 Additional context used
📓 Path-based instructions (1)
{packages,samples,tests}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place packages only under
packages/,samples/, ortests/
Files:
packages/language/src/utils.tspackages/language/src/module.tspackages/language/src/document.tspackages/cli/src/actions/action-utils.ts
🧬 Code graph analysis (4)
packages/language/src/utils.ts (2)
packages/language/src/generated/ast.ts (3)
Model(559-563)Model(565-565)isPlugin(628-630)packages/language/src/constants.ts (1)
PLUGIN_MODULE_NAME(26-26)
packages/language/src/module.ts (2)
packages/language/src/utils.ts (1)
getPluginDocuments(579-637)packages/language/src/generated/ast.ts (2)
Model(559-563)Model(565-565)
packages/language/src/document.ts (4)
packages/language/src/module.ts (1)
createZModelServices(124-126)packages/language/src/constants.ts (1)
STD_LIB_MODULE_NAME(21-21)packages/language/src/utils.ts (5)
resolveImport(398-416)getDocument(570-577)resolveTransitiveImports(367-369)getDataModelAndTypeDefs(431-438)hasAttribute(54-56)packages/language/src/generated/ast.ts (1)
isDataSource(417-419)
packages/cli/src/actions/action-utils.ts (1)
packages/language/src/document.ts (1)
loadDocument(14-130)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-test (20.x, sqlite)
- GitHub Check: build-test (20.x, postgresql)
🔇 Additional comments (2)
packages/cli/src/actions/action-utils.ts (1)
37-47: Good: CLI now defers schema loading to language package.Centralized error/warning handling looks correct; throwing CliError on failure is appropriate.
packages/language/src/module.ts (1)
95-119: Code pattern is safe; DocumentBuilder.onBuildPhase awaits async handlers.Langium 4.x awaits Promises returned by handlers registered with DocumentBuilder.onBuildPhase before proceeding to the next build phase, so the async plugin document loading will complete before the linking phase begins. Plugin docs will not miss the linking window.
Summary by CodeRabbit
New Features
Refactor