fix: corrected resolution of included documents#2495
Conversation
…ojects since v1.45.0 Fixes #2466
🦋 Changeset detectedLatest commit: 617ea4e The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 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. 📝 WalkthroughWalkthroughThis PR threads cancellation tokens through RPC and project workflows, converts parallel view aggregation to a sequential, cancellable loop with per-project error handling, removes workspace locks during activation/rebuild flows, tightens include-path/project membership logic, and adds a Vite-plugin fallback to the first project when a projectId resolver is missing. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Areas requiring extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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: 2
🧹 Nitpick comments (3)
packages/language-server/src/Rpc.ts (1)
273-311: Consider moving the cancellation check inside the loop.The
interruptAndCheck(cancelToken)on line 307 executes after all projects have been processed. For effective cancellation during the loop, consider adding a check at the start of each iteration, similar toFetchViewsFromAllProjects.🔎 Proposed improvement
let metrics: FetchTelemetryMetrics.Res['metrics'] = null for (const projectId of projects.all) { + await interruptAndCheck(cancelToken) try { const model = await likec4Services.ModelBuilder.computeModel(projectId, cancelToken)packages/language-server/src/model-change/ModelChanges.ts (1)
40-57: Consider consistent error handling for manual layout removal.The
removeManualLayoutV1failures are caught and logged as warnings, but the operation continues. While this is reasonable for cleanup of legacy data, consider whether a failed cleanup should:
- Be surfaced to the user in some way (not just logged)
- Affect the success status of the overall operation
Current approach is pragmatic - the new layout is saved regardless of v1 cleanup success.
Also applies to: 59-72
packages/language-server/src/workspace/ProjectsManager.ts (1)
517-571: Verify cancellation check placement in reload loop.The
interruptAndCheckis called inside the config file registration loop, which is good. However, consider whether cancellation should also be checked afterscanProjectFilescompletes for each folder, especially if there are many folders.for (const folder of folders) { try { + if (cancelToken) { + await interruptAndCheck(cancelToken) + } logger.debug`scan projects in folder ${folder.uri}` const files = await this.services.workspace.FileSystemProvider.scanProjectFiles(URI.parse(folder.uri))
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.changeset/moody-fans-win.md(1 hunks).changeset/new-flies-greet.md(1 hunks).changeset/wicked-pots-pump.md(1 hunks)examples/likec4.config.json(1 hunks)packages/language-server/src/Rpc.ts(3 hunks)packages/language-server/src/model-change/ModelChanges.ts(2 hunks)packages/language-server/src/workspace/IndexManager.ts(1 hunks)packages/language-server/src/workspace/LangiumDocuments.ts(3 hunks)packages/language-server/src/workspace/ProjectsManager.spec.ts(2 hunks)packages/language-server/src/workspace/ProjectsManager.ts(20 hunks)packages/language-server/src/workspace/WorkspaceManager.ts(2 hunks)packages/likec4/src/vite-plugin/virtuals/_shared.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx,json,yaml,yml}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
pnpm fmtwith dprint for code formatting; do not use Prettier or eslint for formatting
Files:
packages/language-server/src/workspace/ProjectsManager.spec.tspackages/language-server/src/model-change/ModelChanges.tspackages/language-server/src/workspace/IndexManager.tspackages/language-server/src/workspace/LangiumDocuments.tspackages/likec4/src/vite-plugin/virtuals/_shared.tsexamples/likec4.config.jsonpackages/language-server/src/workspace/WorkspaceManager.tspackages/language-server/src/workspace/ProjectsManager.tspackages/language-server/src/Rpc.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use oxlint with type-aware rules for linting; run
pnpm lintfor checking andpnpm lint:fixfor auto-fixing
Files:
packages/language-server/src/workspace/ProjectsManager.spec.tspackages/language-server/src/model-change/ModelChanges.tspackages/language-server/src/workspace/IndexManager.tspackages/language-server/src/workspace/LangiumDocuments.tspackages/likec4/src/vite-plugin/virtuals/_shared.tspackages/language-server/src/workspace/WorkspaceManager.tspackages/language-server/src/workspace/ProjectsManager.tspackages/language-server/src/Rpc.ts
packages/**/src/**/*.spec.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run unit tests with
pnpm testbefore submitting changes; unit tests are located inpackages/*/src/**/*.spec.tsandpackages/*/src/**/__test__/*.spec.ts
Files:
packages/language-server/src/workspace/ProjectsManager.spec.ts
🧠 Learnings (1)
📚 Learning: 2025-12-18T20:09:20.430Z
Learnt from: CR
Repo: likec4/likec4 PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-18T20:09:20.430Z
Learning: Applies to **/tsconfig.json : Use TypeScript project references for incremental builds; ensure `tsconfig.json` at root properly references all package tsconfigs
Applied to files:
packages/language-server/src/workspace/ProjectsManager.ts
🧬 Code graph analysis (2)
packages/language-server/src/workspace/ProjectsManager.spec.ts (4)
packages/language-server/src/model/model-parser.ts (1)
documents(97-101)packages/language-server/src/model/fqn-index.ts (1)
documents(59-64)packages/language-server/src/model/model-builder.ts (1)
documents(300-302)packages/language-server/src/model/model-locator.ts (1)
documents(47-49)
packages/language-server/src/workspace/LangiumDocuments.ts (5)
packages/language-server/src/ast.ts (1)
LikeC4LangiumDocument(247-249)packages/language-server/src/LikeC4LanguageServices.ts (1)
projects(105-152)packages/language-server/src/index.ts (1)
isLikeC4Builtin(19-19)packages/core/src/model/LikeC4Model.ts (1)
projectId(346-348)packages/core/src/types/_common.ts (1)
NonEmptyArray(9-9)
🔇 Additional comments (24)
.changeset/wicked-pots-pump.md (1)
1-5: Changeset format is correct.The YAML frontmatter and description follow the standard changeset format. However, note that the PR objectives declare fixes for both #2466 and #2472. You may want to verify whether #2472 should also be referenced in this changeset, or if it's documented in a separate changeset entry for clarity on the scope of changes.
examples/likec4.config.json (1)
1-4: LGTM! Clear test configuration for nested projects.The configuration is syntactically correct and its purpose is clearly stated. This aligns well with the PR objectives of fixing nested multi-project workspace issues.
packages/language-server/src/Rpc.ts (2)
164-173: LGTM!The cancellation token is now properly threaded through
ReloadProjectsandRegisterProjecthandlers, enabling cancellable operations during project reload and registration.
175-203: LGTM!Good refactoring to sequential iteration with per-project error handling. This ensures a failure in one project doesn't prevent fetching views from other projects.
.changeset/moody-fans-win.md (1)
1-5: LGTM!Changeset is properly formatted with correct package reference and clear description of the fix.
packages/language-server/src/workspace/IndexManager.ts (1)
18-29: LGTM!The refactored logic correctly includes documents that either belong to the project or are explicitly included via include paths. Centralizing the inclusion check in
ProjectsManager.isIncluded()improves maintainability and ensures consistent behavior across the codebase.packages/language-server/src/workspace/WorkspaceManager.ts (3)
12-12: LGTM!The import correctly brings in
CancellationTokenas a value (namespace) alongside theWorkspaceFoldertype, enabling access toCancellationToken.Nonefor non-cancellable operations.
54-54: LGTM!Passing
CancellationToken.Noneexplicitly for startup config loading is appropriate since the initial workspace startup should not be interruptible. This aligns with the cancellation-aware flow introduced throughout the project.
144-149: LGTM - Simplified rebuild logic.The removal of auto-acquired workspace locks in
rebuildAllsimplifies the control flow and eliminates potential deadlock scenarios when rebuilds are triggered from contexts that already hold locks.packages/language-server/src/workspace/ProjectsManager.spec.ts (2)
506-520: LGTM!Capturing the return values from
registerProjectallows the test to use stableproject.idreferences rather than relying on string literals, making the test more robust to project ID generation changes.
528-544: Good clarification of shared document ownership semantics.The test now explicitly verifies that:
- Shared documents are owned by the first project that includes them (
a-project1)- Both projects can still access shared documents via
projectDocuments()using the newisIncludedlogicThe explicit URI path expectations make the test more precise and easier to debug if something breaks.
packages/language-server/src/workspace/LangiumDocuments.ts (4)
42-42: LGTM - Simplified project ID assignment.The
likec4ProjectIdassignment is now unconditional within the guard blocks, which is correct since:
- In
getDocument: the!exclude(doc)check ensures only LikeC4 documents (non-builtin) reach this line- In
all: the!isLikeC4Builtin(doc.uri)check provides the same guaranteeThis aligns with the
LikeC4LangiumDocumentinterface which declareslikec4ProjectId: c4.ProjectIdas required.Also applies to: 55-55
64-70: LGTM!The combined filter correctly excludes both built-in documents and documents excluded by
ProjectsManager, providing a clean set of user documents for processing.
73-81: Correct implementation of project document resolution.The logic properly returns documents that either:
- Are owned by the project (
doc.likec4ProjectId === projectId)- Are included by the project via include paths (
projects.isIncluded(projectId, doc.uri))This enables multiple projects to share documents from common include paths while maintaining proper ownership semantics.
86-97: LGTM - Clean project ID reset implementation.The
resetProjectIdsmethod correctly:
- Uses
super.allto avoid triggering re-assignment during iteration- Skips excluded (non-LikeC4) documents
- Uses
deleteto properly clear the optional property for subsequent re-assignmentpackages/language-server/src/model-change/ModelChanges.ts (3)
21-37: Improved early validation pattern.The restructured flow performs project and view validation upfront before any modifications, making the error paths clearer and avoiding partial state changes when validation fails.
74-108: LGTM - Clean structured return pattern.The refactored code:
- Uses an invariant to ensure
lspConnectionexists for IDE-only text edit operations- Returns structured
{success, location/error}objects consistently- Properly handles the
applyEditfailure case with user feedback viashowErrorMessage
117-120: LGTM!Error handling now returns a structured error object instead of throwing, consistent with the new return-based API contract.
packages/language-server/src/workspace/ProjectsManager.ts (6)
46-54: Clean predicate factory for folder containment.
isParentFolderForencapsulates the path prefix check logic and returns a reusable predicate, making the include path matching code more readable and consistent across the codebase.
76-79: Good type refinement for include paths.Changing
includePathstoNonEmptyArray<{uri, folder}>(internal) andNonEmptyReadonlyArray<URI>(public) ensures:
- Include paths are never empty arrays (validated at registration)
- The internal representation caches both URI and folder string for efficiency
- The public API exposes only URIs, hiding the internal folder string optimization
Also applies to: 93-93
285-309: Well-designed inclusion check APIs.The new
isIncludedandincludedInProjectsmethods provide clean interfaces for:
- Checking if a specific project includes a document via its include paths
- Finding all projects that include a given document
These support the updated
projectDocumentslogic inLangiumDocumentsthat needs to aggregate both owned and included documents.
331-331: Consistent cancellation token propagation.Adding optional
cancelTokenparameters toregisterConfigFileandregisterProjectenables proper cancellation support throughout the project registration workflow, integrating with the broader cancellation-aware changes in this PR.Also applies to: 362-362
583-591: Critical fix: Reset project IDs before rebuild.The call to
LangiumDocuments.resetProjectIds()inreset()is essential for the fix. It ensures that when projects are re-registered or config changes occur, documents don't retain stale project ID assignments that could cause them to be associated with the wrong project.
639-651: Improved document-to-project resolution.The updated
findProjectForDocumentcorrectly:
- First checks if the document is directly within a project's folder
- Falls back to checking if any project includes the document via include paths
- Returns the default project if no match is found
This ensures proper resolution for both owned and included documents.
| "likec4": patch | ||
| --- | ||
|
|
||
| Fallback to the first project in vite plugin, if `projectId` is not found, instead of erroring out. Closes [#2472](https://github.com/like-c4/likec4/issues/2472) |
There was a problem hiding this comment.
Incorrect repository URL in issue link.
The issue link references like-c4/likec4 but the PR URL indicates the repository is likec4/likec4. This will result in a broken link.
🔎 Proposed fix
-Fallback to the first project in vite plugin, if `projectId` is not found, instead of erroring out. Closes [#2472](https://github.com/like-c4/likec4/issues/2472)
+Fallback to the first project in vite plugin, if `projectId` is not found, instead of erroring out. Closes [#2472](https://github.com/likec4/likec4/issues/2472)📝 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.
| Fallback to the first project in vite plugin, if `projectId` is not found, instead of erroring out. Closes [#2472](https://github.com/like-c4/likec4/issues/2472) | |
| Fallback to the first project in vite plugin, if `projectId` is not found, instead of erroring out. Closes [#2472](https://github.com/likec4/likec4/issues/2472) |
🤖 Prompt for AI Agents
In .changeset/new-flies-greet.md around line 5, the issue URL uses the wrong
repository path `like-c4/likec4`; update the link to the correct repository
`likec4/likec4` (e.g. https://github.com/likec4/likec4/issues/2472) so the issue
link resolves; keep the rest of the sentence unchanged and verify the markdown
link renders and closes the issue as intended.
| let fn = ${fnName}Fn[projectId] | ||
| if (!fn) { | ||
| throw new Error('Unknown projectId: ' + projectId) | ||
| const projects = Object.keys(${fnName}Fn) | ||
| console.error('Unknown projectId: ' + projectId + ' (available: ' + projectIds + ')') | ||
| if (projects.length === 0) { | ||
| throw new Error('No projects found, invalid state') | ||
| } | ||
| projectId = projects[0] | ||
| console.warn('Falling back to project: ' + projectId) | ||
| fn = ${fnName}Fn[projectId] | ||
| } |
There was a problem hiding this comment.
Critical bug: projectIds is undefined, should be projects.
Line 107 references projectIds but the variable declared on line 106 is named projects. This will cause a ReferenceError at runtime when the fallback path is triggered.
🔎 Proposed fix
let fn = ${fnName}Fn[projectId]
if (!fn) {
const projects = Object.keys(${fnName}Fn)
- console.error('Unknown projectId: ' + projectId + ' (available: ' + projectIds + ')')
+ console.error('Unknown projectId: ' + projectId + ' (available: ' + projects + ')')
if (projects.length === 0) {
throw new Error('No projects found, invalid state')
}
projectId = projects[0]
console.warn('Falling back to project: ' + projectId)
fn = ${fnName}Fn[projectId]
}📝 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.
| let fn = ${fnName}Fn[projectId] | |
| if (!fn) { | |
| throw new Error('Unknown projectId: ' + projectId) | |
| const projects = Object.keys(${fnName}Fn) | |
| console.error('Unknown projectId: ' + projectId + ' (available: ' + projectIds + ')') | |
| if (projects.length === 0) { | |
| throw new Error('No projects found, invalid state') | |
| } | |
| projectId = projects[0] | |
| console.warn('Falling back to project: ' + projectId) | |
| fn = ${fnName}Fn[projectId] | |
| } | |
| let fn = ${fnName}Fn[projectId] | |
| if (!fn) { | |
| const projects = Object.keys(${fnName}Fn) | |
| console.error('Unknown projectId: ' + projectId + ' (available: ' + projects + ')') | |
| if (projects.length === 0) { | |
| throw new Error('No projects found, invalid state') | |
| } | |
| projectId = projects[0] | |
| console.warn('Falling back to project: ' + projectId) | |
| fn = ${fnName}Fn[projectId] | |
| } |
🤖 Prompt for AI Agents
In packages/likec4/src/vite-plugin/virtuals/_shared.ts around lines 104 to 114,
the error path logs a variable named projectIds that doesn't exist (causing a
ReferenceError); change the console.error call to reference the existing
projects array (or otherwise stringify the available project keys) so it uses
projects instead of projectIds, keeping the rest of the fallback logic intact.
1 participant
This pull request introduces several improvements and bug fixes to the LikeC4 language server and related tooling, focusing on project document resolution, workspace management, and plugin behavior. The changes enhance reliability when handling included documents, improve error handling, and address race conditions in extension activation.
Fixes #2466
Fixes #2472
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.