Shared Files via Config (instead of symlinks) + Navigate To Element from Search (#1711 )#2425
Shared Files via Config (instead of symlinks) + Navigate To Element from Search (#1711 )#2425
Conversation
Co-authored-by: davydkov <[email protected]>
- Add 'Include additional directories' section to project config docs - Update multi-projects docs to feature include as primary sharing method - Add include option to programmatic config example - Document path requirements and usage patterns
|
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. 📝 WalkthroughWalkthroughAdds an include-path feature allowing projects to reference additional relative directories of LikeC4 source files, with schema validation, runtime resolution in the language server, workspace loading of included files, docs updates, and tests. Separately, adds diagram focus-on-element with an auto-unfocus timer, state machine, utils, UI wiring, and tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (4)
🧰 Additional context used📓 Path-based instructions (3)**/*.{ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{ts,tsx,js,jsx,json,md}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
🧠 Learnings (1)📚 Learning: 2025-12-05T15:36:11.968ZApplied to files:
🧬 Code graph analysis (2)packages/diagram/src/likec4diagram/state/diagram-api.ts (2)
packages/diagram/src/likec4diagram/state/machine.state.ready.focused.ts (3)
⏰ 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). (4)
🔇 Additional comments (6)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR introduces a new include configuration option for LikeC4 project configs, enabling users to share specification files across multiple projects without relying on symlinks. The feature resolves include paths relative to the project folder, recursively scans included directories for .c4 files, and provides overlap detection warnings to prevent configuration issues.
Key Changes
- Added
includearray field to project configuration schema for specifying additional source directories - Implemented include path resolution and validation in ProjectsManager with proper overlap detection
- Extended WorkspaceManager to load documents from configured include paths during initialization
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| schemas/likec4-config.schema.json | Added include field definition with pattern validation for relative paths |
| packages/config/src/schema.ts | Integrated IncludeSchema and validateIncludePaths into main project config schema |
| packages/config/src/schema.include.ts | New file defining validation schema and logic for include paths |
| packages/config/src/schema.spec.ts | Added comprehensive test coverage for include path validation scenarios |
| packages/language-server/src/workspace/ProjectsManager.ts | Implemented include path resolution, overlap detection, and document assignment logic |
| packages/language-server/src/workspace/ProjectsManager.spec.ts | Added integration tests for include path functionality |
| packages/language-server/src/workspace/WorkspaceManager.ts | Extended document loading to scan configured include paths |
| apps/docs/src/content/docs/dsl/Config/*.mdx | Updated documentation with include feature explanation and examples |
| packages/icons/scripts/update-icons.mjs | Fixed icon name mapping (JQuery → jQuery) for remote file rename |
| pnpm-workspace.yaml, pnpm-lock.yaml, package.json, e2e/package.json | Dependency version updates (React 19.2.0→19.2.1, nano-staged 0.8.0→0.9.0) |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
|
@kieronlanning I've opened a new pull request, #2426, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: kieronlanning <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/language-server/src/workspace/ProjectsManager.ts (1)
389-416: Consider deduplicating overlap warnings.When projects A and B both have overlapping include paths, the current logic may log symmetric warnings (A→B and B→A) when the second project is registered. This is minor since it only affects log verbosity during project registration.
packages/config/src/schema.ts (1)
18-19: Include config wiring and validation are consistent; post-parse helper call is optional redundancyThe new
includefield is correctly threaded through the project config schema and post-parse validation, ensuring only relative paths are accepted and matching the docs/JSON schema behaviour. Since the schema forincludealready enforces the same relative-path constraints, the extravalidateIncludePathscall after a successful parse is functionally redundant for config-based usage (though still useful as a standalone helper). If you prefer a single validation source here, you could either drop the extra call or move its logic into a schema-level refinement; current behaviour is still perfectly fine.Also applies to: 63-71, 239-255
schemas/likec4-config.schema.json (1)
33-41: Include JSON schema matches runtime validation; consider factoring out the shared path patternThe new
includeproperty definition (array of non-empty strings with the relative-path pattern) is consistent with the runtime validation and docs. Since the same pattern is also used for image alias values, you might optionally extract a common$defsentry for “relative path” to avoid future drift between these spots.packages/config/src/schema.spec.ts (1)
4-5: Include tests give thorough coverage of config and helper behaviourThe new tests around the
includefield, the schema, and the helper cover the key scenarios (valid/invalid/empty/undefined paths, absolute and URL cases, and multi-error reporting) and closely match the intended semantics. If you ever find yourself tweaking the allowed path shapes, you might consider extracting the repeated “validPaths”/“relativePaths” arrays into shared fixtures to keep the expectations in sync, but as-is this is already solid coverage.Also applies to: 184-269, 538-650
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/docs/src/content/docs/dsl/Config/index.mdx(1 hunks)apps/docs/src/content/docs/dsl/Config/multi-projects.mdx(2 hunks)apps/docs/src/content/docs/dsl/Config/programmatic.mdx(1 hunks)packages/config/src/schema.include.ts(1 hunks)packages/config/src/schema.spec.ts(3 hunks)packages/config/src/schema.ts(3 hunks)packages/icons/scripts/update-icons.mjs(1 hunks)packages/language-server/src/workspace/ProjectsManager.spec.ts(1 hunks)packages/language-server/src/workspace/ProjectsManager.ts(6 hunks)packages/language-server/src/workspace/WorkspaceManager.ts(1 hunks)schemas/likec4-config.schema.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/config/src/schema.ts (2)
packages/config/src/schema.include.ts (2)
IncludeSchema(16-24)validateIncludePaths(28-48)packages/core/src/model/__test__/fixture.ts (1)
parsed(163-163)
⏰ 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). (5)
- GitHub Check: checks / 🔬 e2e types
- GitHub Check: checks / 🔬 e2e tests
- GitHub Check: checks / 📖 docs
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: checks / ⊞ windows build
🔇 Additional comments (17)
packages/icons/scripts/update-icons.mjs (1)
79-79: LGTM!Correct fix to accommodate the upstream icon filename change from
JQuery.svgtojQuery.svg.packages/language-server/src/workspace/ProjectsManager.spec.ts (6)
340-360: LGTM!Test correctly verifies that relative include paths are resolved to absolute URIs based on the project folder.
362-396: LGTM!Comprehensive test covering document attribution for project folder, include paths, nested paths, and unrelated paths.
398-423: LGTM!Tests correctly verify that
includePathsisundefinedwhen not configured or when configured with an empty array.
425-470: LGTM!Good coverage of
getAllIncludePaths()aggregation across multiple projects with varying include configurations.
472-492: LGTM!Critical test ensuring project folder precedence over include paths when they overlap.
494-550: LGTM!Tests correctly verify dynamic update and removal of include paths when projects are reloaded with changed configurations.
packages/language-server/src/workspace/ProjectsManager.ts (6)
61-69: LGTM!Good design keeping both URI representation (for public API) and string representation (for efficient prefix matching) of include paths.
76-79: LGTM!Public interface correctly exposes optional
includePathsfor consumers needing access to resolved include directories.
213-219: LGTM!Conditional spreading of
includePathscorrectly ensures the property is only present when configured.
548-564: LGTM!Document filtering correctly extended to include documents from configured include paths during project rebuild.
586-604: LGTM!Correct precedence implementation: project folders are checked first, then include paths, with fallback to default. This ensures a project's own folder always takes priority over another project's include path.
620-634: LGTM!Clean implementation providing a flat list of all include paths with their associated project IDs for workspace scanning.
apps/docs/src/content/docs/dsl/Config/programmatic.mdx (1)
37-37: LGTM!Good addition demonstrating the new
includeconfiguration option with clear relative path examples.apps/docs/src/content/docs/dsl/Config/index.mdx (1)
59-96: Include section clearly matches the new config behaviourThe new “Include additional directories” section accurately describes how
includeworks (relative-only paths, recursive scan from the project folder) and aligns with the implementation and schema. Looks good.apps/docs/src/content/docs/dsl/Config/multi-projects.mdx (1)
66-153: Multi-project sharing docs align well with the include featureThe updated “Share specification” section and the include examples (including multi-directory and the path resolution/requirements asides) accurately reflect how the new
includeoption behaves, while clearly de‑emphasizing symlinks. No changes needed.packages/config/src/schema.include.ts (1)
1-48: Include schema and helper cleanly enforce relative path constraintsThe include schema and
validateIncludePathshelper correctly enforce “non-empty, relative, non-URL” paths and provide nice aggregate error reporting for multiple bad entries. This matches the documented rules and the JSON schema; implementation looks solid.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/config/src/schema.include.ts (1)
3-5: Consider extracting the shared regex to avoid duplication.
RELATIVE_PATH_REGEXis identical toIMAGE_ALIAS_VALUE_REGEXinschema.image-alias.ts. Both validate "relative path (no leading slash, drive letter, or protocol)".You could extract this to a shared constants file:
// e.g., in schema.common.ts or similar export const RELATIVE_PATH_REGEX = /^(?!\/|[A-Za-z]:[\\\/])(?!.*:\/\/).*$/
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/config/src/schema.image-alias.ts(1 hunks)packages/config/src/schema.include.ts(1 hunks)packages/config/src/schema.theme.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/config/src/schema.theme.ts (1)
packages/core/src/styles/types.ts (1)
ThemeColor(65-65)
packages/config/src/schema.include.ts (2)
packages/core/src/builder/Builder.views.ts (1)
include(379-384)packages/likec4/src/cli/options.ts (1)
path(5-11)
⏰ 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: Analyze (javascript-typescript)
- GitHub Check: checks / 🛠️ build
- GitHub Check: checks / ⊞ windows build
🔇 Additional comments (5)
packages/config/src/schema.image-alias.ts (1)
9-15: LGTM!The migration from
.nonempty()to.min(1, ...)aligns with Zod v3.23+ best practices where.nonempty()is deprecated. The error message is clear and behavior remains unchanged.packages/config/src/schema.theme.ts (1)
56-71: LGTM!Both
.min(1, ...)replacements are consistent with the deprecation of.nonempty()in Zod v3.23+. The distinct error messages ("Color name cannot be empty" vs "Color value cannot be empty") appropriately differentiate between the two validation contexts.packages/config/src/schema.include.ts (3)
1-14: LGTM!The schema follows established patterns from
schema.image-alias.ts. Using.min(1, ...)addresses the prior review feedback about deprecated.nonempty().
16-24: LGTM!The schema metadata provides clear documentation with practical examples. Making the array optional is appropriate for a config field that may not always be needed.
28-47: Defensive validation is consistent with the established pattern.Similar to
validateImageAliasesinschema.image-alias.ts, this function performs defensive validation even though Zod already enforces the regex. The consolidated error reporting with all invalid paths is helpful for debugging.
Signed-off-by: Kieron Lanning <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/config/src/schema.spec.ts (1)
617-622: Consider adding Windows-style path and empty string tests for consistency.The
validateIncludePathstests only cover Unix-style absolute paths (/absolute/path), whileIncludeSchematests also cover Windows paths (C:\\absolute\\path,D:/another/path). Additionally, there's no empty string rejection test here, unlike theIncludeSchematests.For parity and explicit coverage:
it('should reject absolute paths', ({ expect }) => { - const absolutePaths = ['/absolute/path'] - expect(() => validateIncludePaths(absolutePaths)).toThrow( - 'Invalid include path(s): "/absolute/path" (must be relative paths without leading slash, drive letter, or protocol)', - ) + const testCases = [ + { path: '/absolute/path', expected: '"/absolute/path"' }, + { path: 'C:\\absolute\\path', expected: '"C:\\absolute\\path"' }, + { path: 'D:/another/path', expected: '"D:/another/path"' }, + ] + + for (const { path, expected } of testCases) { + expect(() => validateIncludePaths([path]), `Path "${path}" should be rejected`).toThrow( + `Invalid include path(s): ${expected} (must be relative paths without leading slash, drive letter, or protocol)`, + ) + } + }) + + it('should reject empty strings', ({ expect }) => { + expect(() => validateIncludePaths([''])).toThrow('Include path cannot be empty') })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/config/src/schema.image-alias.ts(1 hunks)packages/config/src/schema.spec.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/config/src/schema.image-alias.ts
⏰ 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). (4)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: checks / 🔬 tests
- GitHub Check: checks / ⊞ windows build
- GitHub Check: checks / 🛠️ build
🔇 Additional comments (3)
packages/config/src/schema.spec.ts (3)
4-4: LGTM!Import follows the established pattern for schema imports in this file.
184-269: Well-structured test coverage for the include field.The tests comprehensively cover valid paths, empty/undefined values, and properly reject absolute paths (including Windows formats), URLs, and empty strings. The mixed valid/invalid path test at lines 260-268 ensures the validation fails fast on any invalid entry.
538-600: Thorough schema validation tests.Good coverage of the
IncludeSchemaparsing behavior including edge cases like single-dot (.) and double-dot (..) paths, and paths containing dashes, underscores, and dots.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/diagram/src/likec4diagram/state/machine.state.ready.focused.spec.ts (1)
1-64: Consider adding integration tests for state machine behavior.The current tests verify event and context shapes, which is useful for type safety. However, integration tests that verify actual state machine behavior would provide more value:
- Test that
focus.nodewithautoUnfocus: truetriggers the correct state transitions- Test that the autoUnfocus timer is actually started/cancelled
- Test that
focus.autoUnfocusevent correctly transitions back to idle state- Test the timing behavior (e.g., auto-unfocus after expected delay)
These integration tests would catch runtime issues that type-level tests cannot, such as incorrect action wiring or timing bugs.
packages/diagram/src/likec4diagram/state/machine.state.navigating.spec.ts (1)
67-115: Consider adding integration tests for navigation behavior.Similar to the focused.spec.ts file, these tests verify event and context shapes but don't test actual state machine behavior. Consider adding integration tests that verify:
- Navigation with focusOnElement actually focuses the target element after navigation completes
- Navigation without focusOnElement follows the default fit-diagram behavior
- Edge cases like navigating with focusOnElement when the element doesn't exist in the target view
packages/diagram/src/likec4diagram/state/machine.state.navigating.ts (1)
198-225: Consider logging when focusOnElement doesn't resolve to a node.The logic correctly handles the case where
focusOnElementis provided but doesn't match any node (falls back to fitDiagram). However, this silent fallback could be confusing for users or developers debugging search/navigation issues.Consider adding a console.warn or debug log when
focusOnElementis provided butnodeToFocusis null:const nodeToFocus = isTruthy(focusOnElement) ? findNodeByElementFqn(event.xynodes, focusOnElement) : null + +if (focusOnElement && !nodeToFocus) { + console.warn(`Could not find node for focusOnElement: ${focusOnElement}`) +}This would help with debugging and provide visibility into why the expected focus didn't occur.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/diagram/src/likec4diagram/state/diagram-api.ts(4 hunks)packages/diagram/src/likec4diagram/state/machine.actions.ts(3 hunks)packages/diagram/src/likec4diagram/state/machine.setup.ts(4 hunks)packages/diagram/src/likec4diagram/state/machine.state.navigating.spec.ts(1 hunks)packages/diagram/src/likec4diagram/state/machine.state.navigating.ts(4 hunks)packages/diagram/src/likec4diagram/state/machine.state.ready.focused.spec.ts(1 hunks)packages/diagram/src/likec4diagram/state/machine.state.ready.focused.ts(3 hunks)packages/diagram/src/likec4diagram/state/machine.state.ready.idle.ts(1 hunks)packages/diagram/src/likec4diagram/state/machine.ts(1 hunks)packages/diagram/src/search/components/ElementsColumn.tsx(1 hunks)packages/diagram/src/search/components/PickView.tsx(2 hunks)packages/diagram/src/search/components/ViewsColum.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
pnpm ci:typecheckfor TypeScript type checking across all packages except documentation
Files:
packages/diagram/src/likec4diagram/state/machine.state.navigating.tspackages/diagram/src/search/components/ElementsColumn.tsxpackages/diagram/src/likec4diagram/state/machine.state.ready.focused.spec.tspackages/diagram/src/likec4diagram/state/machine.setup.tspackages/diagram/src/likec4diagram/state/machine.tspackages/diagram/src/likec4diagram/state/machine.state.navigating.spec.tspackages/diagram/src/likec4diagram/state/machine.state.ready.focused.tspackages/diagram/src/likec4diagram/state/diagram-api.tspackages/diagram/src/likec4diagram/state/machine.state.ready.idle.tspackages/diagram/src/search/components/PickView.tsxpackages/diagram/src/search/components/ViewsColum.tsxpackages/diagram/src/likec4diagram/state/machine.actions.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use oxlint with type-aware rules for linting instead of eslint, and run
pnpm ci:lintto validate all packages except documentation
Files:
packages/diagram/src/likec4diagram/state/machine.state.navigating.tspackages/diagram/src/search/components/ElementsColumn.tsxpackages/diagram/src/likec4diagram/state/machine.state.ready.focused.spec.tspackages/diagram/src/likec4diagram/state/machine.setup.tspackages/diagram/src/likec4diagram/state/machine.tspackages/diagram/src/likec4diagram/state/machine.state.navigating.spec.tspackages/diagram/src/likec4diagram/state/machine.state.ready.focused.tspackages/diagram/src/likec4diagram/state/diagram-api.tspackages/diagram/src/likec4diagram/state/machine.state.ready.idle.tspackages/diagram/src/search/components/PickView.tsxpackages/diagram/src/search/components/ViewsColum.tsxpackages/diagram/src/likec4diagram/state/machine.actions.ts
**/*.{ts,tsx,js,jsx,json,md}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
pnpm fmtto format code with dprint instead of Prettier or eslint for formatting
Files:
packages/diagram/src/likec4diagram/state/machine.state.navigating.tspackages/diagram/src/search/components/ElementsColumn.tsxpackages/diagram/src/likec4diagram/state/machine.state.ready.focused.spec.tspackages/diagram/src/likec4diagram/state/machine.setup.tspackages/diagram/src/likec4diagram/state/machine.tspackages/diagram/src/likec4diagram/state/machine.state.navigating.spec.tspackages/diagram/src/likec4diagram/state/machine.state.ready.focused.tspackages/diagram/src/likec4diagram/state/diagram-api.tspackages/diagram/src/likec4diagram/state/machine.state.ready.idle.tspackages/diagram/src/search/components/PickView.tsxpackages/diagram/src/search/components/ViewsColum.tsxpackages/diagram/src/likec4diagram/state/machine.actions.ts
packages/*/**/!(node_modules)/**/*.spec.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Unit tests must be located in
packages/*/src/**/*.spec.tsorpackages/*/src/**/__test__/*.spec.tsfiles
Files:
packages/diagram/src/likec4diagram/state/machine.state.ready.focused.spec.tspackages/diagram/src/likec4diagram/state/machine.state.navigating.spec.ts
🧠 Learnings (1)
📚 Learning: 2025-12-05T15:36:11.968Z
Learnt from: CR
Repo: likec4/likec4 PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-05T15:36:11.968Z
Learning: Applies to packages/*/**/!(node_modules)/**/*.spec.ts : Unit tests must be located in `packages/*/src/**/*.spec.ts` or `packages/*/src/**/__test__/*.spec.ts` files
Applied to files:
packages/diagram/src/likec4diagram/state/machine.state.navigating.spec.ts
🧬 Code graph analysis (5)
packages/diagram/src/likec4diagram/state/machine.setup.ts (1)
packages/diagram/src/likec4diagram/state/aligners.ts (1)
AlignmentMode(8-8)
packages/diagram/src/likec4diagram/state/machine.state.navigating.spec.ts (1)
packages/diagram/src/likec4diagram/state/machine.state.navigating.ts (1)
findNodeByElementFqn(16-22)
packages/diagram/src/likec4diagram/state/machine.state.ready.focused.ts (3)
packages/diagram/src/likec4diagram/state/machine.actions.ts (3)
startAutoUnfocusTimer(775-780)undimEverything(184-191)cancelAutoUnfocusTimer(782-782)packages/diagram/src/likec4diagram/state/machine.actions.layout.ts (1)
returnViewportBefore(180-200)packages/diagram/src/likec4diagram/state/machine.setup.ts (2)
machine(249-335)targetState(337-343)
packages/diagram/src/likec4diagram/state/diagram-api.ts (1)
packages/core/src/model/view/LikeC4ViewModel.ts (1)
node(304-307)
packages/diagram/src/search/components/ViewsColum.tsx (1)
packages/core/src/model/LikeC4Model.ts (3)
LikeC4Model(60-872)View(932-932)view(502-505)
⏰ 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). (4)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: checks / 🔬 tests
- GitHub Check: checks / ⊞ windows build
- GitHub Check: checks / 🛠️ build
🔇 Additional comments (17)
packages/diagram/src/likec4diagram/state/machine.ts (1)
52-52: LGTM! Context initialization.The
autoUnfocusTimer: falseinitialization is correct and consistent with the type definition in machine.setup.ts.packages/diagram/src/likec4diagram/state/machine.state.navigating.spec.ts (1)
5-65: LGTM! Comprehensive unit tests for findNodeByElementFqn.The test coverage for
findNodeByElementFqnis excellent, covering:
- Edge cases (empty array, no match)
- Success cases (single match, multiple matches)
- Null/missing data (null modelFqn, missing modelFqn property)
- Mixed data shapes
The test on lines 30-37 correctly verifies that the first matching node is returned when multiple nodes share the same modelFqn.
packages/diagram/src/search/components/ElementsColumn.tsx (1)
296-306: LGTM! Inverted logic improves clarity.The refactored logic is clearer by explicitly handling the same-view and different-view cases separately. The comments on lines 297 and 302 help document the intent.
Both branches use a 100ms delay, which is consistent and allows the search UI close animation to complete before triggering navigation or focus.
packages/diagram/src/likec4diagram/state/machine.state.ready.idle.ts (1)
83-96: LGTM! Priority-based guards enable search-initiated focus.The refactored
focus.nodetransition array correctly implements priority-based evaluation:
- First guard allows focus when
autoUnfocus=true(search-initiated), bypassing the FocusMode requirement- Second guard requires FocusMode to be enabled for manual focus
This design allows search-initiated focus to work regardless of the FocusMode setting, which is the correct UX. The comments clearly document the intent.
packages/diagram/src/likec4diagram/state/machine.state.navigating.ts (1)
16-22: LGTM! Clean helper function.The
findNodeByElementFqnhelper is well-implemented:
- Correctly checks for
modelFqnproperty existence- Handles null/undefined modelFqn
- Returns the first matching node (as tested in the spec file)
packages/diagram/src/search/components/PickView.tsx (1)
105-105: LGTM! Consistent prop wiring.The
focusOnElement={elementFqn}prop is correctly wired to both scoped and other ViewButton instances, enabling the focus-on-element behavior when users select a view from the picker.Also applies to: 123-123
packages/diagram/src/search/components/ViewsColum.tsx (3)
2-2: LGTM!The
Fqntype import and the optionalfocusOnElementprop addition properly support the new element focusing feature from search.Also applies to: 107-107
137-137: LGTM!The disabled state logic correctly allows interaction when
focusOnElementis provided, even on the current view. This enables users to focus on specific elements from search results.
117-130: The setTimeout pattern here is intentional UI animation timing, not a state machine synchronization issue.Both branches use a 100ms delay after sending the
closeevent to the search state machine. This is necessary to allow the search modal to close visually before navigation occurs. The same pattern appears consistently across multiple search components (ViewsColumn and ElementsColumn), indicating this is a deliberate, established approach rather than a workaround.The state machine's
closeevent is synchronous, but the search UI itself has animation/transition delays that require the 100ms buffer before navigation. This is standard practice when coordinating async UI operations with component state changes.Likely an incorrect or invalid review comment.
packages/diagram/src/likec4diagram/state/diagram-api.ts (3)
50-54: LGTM!The
navigateTosignature extension and implementation correctly support passing the optionalfocusOnElementparameter through to the navigation event. The conditional spread ensures the property is only included when provided.Also applies to: 151-157
105-109: LGTM!The new
focusOnElementmethod is well-documented and appropriately added to the publicDiagramApiinterface for search-driven element highlighting.
220-226: The implementation is correct as-is. Thefind()approach and silent failure are both intentional design patterns validated by the test suite. When multiple nodes share the samemodelFqnin a view (an uncommon scenario), returning the first match is the documented behavior. The silent failure when a node is not found is appropriate since the element may not be present in the current view due to filtering or navigation context. No changes are needed.packages/diagram/src/likec4diagram/state/machine.actions.ts (3)
103-122: LGTM!The
assignFocusedNodeaction correctly captures theautoUnfocusflag fromfocus.nodeevents and includes it in the returned context update. This properly supports the auto-unfocus timer infrastructure.
768-782: LGTM! Auto-unfocus infrastructure is disabled by design.The auto-unfocus timer infrastructure is correctly implemented but intentionally disabled with
AUTO_UNFOCUS_DELAY = 0. The conditional check ensures the timer only starts when the delay is positive, making this a clean feature flag. The documentation clearly explains this behavior.
835-835: LGTM!The navigation history correctly captures the
focusOnElementparameter fromnavigate.toevents, enabling proper tracking of element-focused navigation requests.packages/diagram/src/likec4diagram/state/machine.state.ready.focused.ts (2)
8-8: LGTM!The auto-unfocus timer lifecycle is correctly integrated into the focused state entry and exit actions, ensuring proper timer management and cleanup. The implementation follows state machine best practices.
Also applies to: 18-18, 35-35, 41-41
44-44: LGTM!The
autoUnfocusTimercontext flag is properly reset on exit, and thefocus.autoUnfocusevent handler correctly transitions to the idle state. This completes the auto-unfocus flow when the timer fires (though currently disabled with delay = 0).Also applies to: 48-50
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/diagram/src/likec4diagram/state/machine.state.navigating.spec.ts (1)
67-114: Tests validate object structure but not state machine behavior.These tests verify that objects can be constructed with
focusOnElementproperties, but they don't test the actual state machine integration—specifically thatnavigate.toevents withfocusOnElementtrigger the expectedfocus.nodeaction or thatlastOnNavigatecontext is correctly assigned during transitions.Consider adding integration tests that:
- Send a
navigate.toevent withfocusOnElementto the state machine- Verify that
focus.nodeis enqueued when the element exists inxynodes- Verify fallback to
fitDiagramwhen the element doesn't exist
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/diagram/src/likec4diagram/state/machine.state.navigating.spec.ts(1 hunks)packages/diagram/src/likec4diagram/state/machine.state.navigating.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
pnpm ci:typecheckfor TypeScript type checking across all packages except documentation
Files:
packages/diagram/src/likec4diagram/state/machine.state.navigating.spec.tspackages/diagram/src/likec4diagram/state/machine.state.navigating.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use oxlint with type-aware rules for linting instead of eslint, and run
pnpm ci:lintto validate all packages except documentation
Files:
packages/diagram/src/likec4diagram/state/machine.state.navigating.spec.tspackages/diagram/src/likec4diagram/state/machine.state.navigating.ts
**/*.{ts,tsx,js,jsx,json,md}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
pnpm fmtto format code with dprint instead of Prettier or eslint for formatting
Files:
packages/diagram/src/likec4diagram/state/machine.state.navigating.spec.tspackages/diagram/src/likec4diagram/state/machine.state.navigating.ts
packages/*/**/!(node_modules)/**/*.spec.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Unit tests must be located in
packages/*/src/**/*.spec.tsorpackages/*/src/**/__test__/*.spec.tsfiles
Files:
packages/diagram/src/likec4diagram/state/machine.state.navigating.spec.ts
🧬 Code graph analysis (2)
packages/diagram/src/likec4diagram/state/machine.state.navigating.spec.ts (1)
packages/diagram/src/likec4diagram/state/machine.state.navigating.ts (1)
findNodeByElementFqn(18-24)
packages/diagram/src/likec4diagram/state/machine.state.navigating.ts (1)
packages/diagram/src/likec4diagram/state/machine.actions.layout.ts (1)
raiseFitDiagram(149-161)
⏰ 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: Analyze (javascript-typescript)
- GitHub Check: checks / 🛠️ build
- GitHub Check: checks / ⊞ windows build
🔇 Additional comments (3)
packages/diagram/src/likec4diagram/state/machine.state.navigating.spec.ts (1)
5-65: Good test coverage for the helper function.The
findNodeByElementFqntests comprehensively cover edge cases including empty arrays, missing/nullmodelFqn, and multiple matches. This provides solid validation of the helper's behavior.packages/diagram/src/likec4diagram/state/machine.state.navigating.ts (2)
16-24: LGTM!The helper function correctly handles edge cases:
- The
'modelFqn' in n.datacheck guards against missing properties- Strict equality handles null values appropriately
- The generic constraint allows flexibility while maintaining type safety
200-227: Logic looks correct; verifyfromHistorypath doesn't need similar handling.The focus-on-element implementation correctly:
- Uses
isTruthyto handle null/undefinedfocusOnElement- Falls back to
raiseFitDiagramwhen no matching node exists- Passes
autoUnfocus: truefor transient focus behaviorHowever, the
fromHistorycode path (lines 112-160) exits early without this check. If a user navigates via browser back/forward to a view that was previously reached withfocusOnElement, should that focus be restored? The current implementation only appliesfocusOnElementto new navigation entries.
… of the dependency tree Because the old machine.state.navigating.spec.ts imported from the machine.state.navigating.ts it pulled in far too much which resulted in test failures in GH actions(!?). Moved function to utils.ts and added tests for that instead.
kieronlanning
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/diagram/src/likec4diagram/state/utils.spec.ts (1)
67-114: Consider consolidating or removing structure-only tests.These tests validate object structure rather than behavior—they don't import or exercise any actual implementation. While they serve as documentation for the expected shape of
navigate.toevents andlastOnNavigatecontext, TypeScript already enforces these structures at compile time.If these are intended as integration smoke tests or documentation, consider moving them to a dedicated documentation file or removing them in favor of type-level tests (e.g., using
expectTypeOffrom Vitest).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/diagram/src/likec4diagram/state/machine.state.navigating.ts(4 hunks)packages/diagram/src/likec4diagram/state/types.ts(1 hunks)packages/diagram/src/likec4diagram/state/utils.spec.ts(1 hunks)packages/diagram/src/likec4diagram/state/utils.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/diagram/src/likec4diagram/state/machine.state.navigating.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
pnpm ci:typecheckfor TypeScript type checking across all packages except documentation
Files:
packages/diagram/src/likec4diagram/state/types.tspackages/diagram/src/likec4diagram/state/utils.tspackages/diagram/src/likec4diagram/state/utils.spec.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use oxlint with type-aware rules for linting instead of eslint, and run
pnpm ci:lintto validate all packages except documentation
Files:
packages/diagram/src/likec4diagram/state/types.tspackages/diagram/src/likec4diagram/state/utils.tspackages/diagram/src/likec4diagram/state/utils.spec.ts
**/*.{ts,tsx,js,jsx,json,md}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
pnpm fmtto format code with dprint instead of Prettier or eslint for formatting
Files:
packages/diagram/src/likec4diagram/state/types.tspackages/diagram/src/likec4diagram/state/utils.tspackages/diagram/src/likec4diagram/state/utils.spec.ts
packages/*/**/!(node_modules)/**/*.spec.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Unit tests must be located in
packages/*/src/**/*.spec.tsorpackages/*/src/**/__test__/*.spec.tsfiles
Files:
packages/diagram/src/likec4diagram/state/utils.spec.ts
🧠 Learnings (2)
📚 Learning: 2025-12-05T15:36:11.968Z
Learnt from: CR
Repo: likec4/likec4 PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-05T15:36:11.968Z
Learning: Applies to packages/*/**/!(node_modules)/**/*.spec.ts : Unit tests must be located in `packages/*/src/**/*.spec.ts` or `packages/*/src/**/__test__/*.spec.ts` files
Applied to files:
packages/diagram/src/likec4diagram/state/utils.spec.ts
📚 Learning: 2025-12-05T15:36:11.968Z
Learnt from: CR
Repo: likec4/likec4 PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-05T15:36:11.968Z
Learning: Applies to e2e/{tests,src}/**/*.spec.ts : E2E tests must be located in `e2e/tests/*.spec.ts` or `e2e/src/*.spec.ts` files
Applied to files:
packages/diagram/src/likec4diagram/state/utils.spec.ts
🧬 Code graph analysis (2)
packages/diagram/src/likec4diagram/state/utils.ts (1)
packages/diagram/src/likec4diagram/state/types.ts (1)
NodeWithData(11-11)
packages/diagram/src/likec4diagram/state/utils.spec.ts (1)
packages/diagram/src/likec4diagram/state/utils.ts (1)
findNodeByElementFqn(23-29)
⏰ 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: Analyze (javascript-typescript)
- GitHub Check: checks / 🛠️ build
- GitHub Check: checks / ⊞ windows build
🔇 Additional comments (4)
packages/diagram/src/likec4diagram/state/types.ts (1)
11-11: LGTM!The type definition is clean and appropriately scoped for the node-finding utility.
packages/diagram/src/likec4diagram/state/utils.ts (2)
5-6: LGTM!The necessary type imports are properly added to support the new utility function.
Also applies to: 21-21
23-29: LGTM!The implementation correctly handles all edge cases, including null values, missing properties, and empty arrays. The type guard pattern ensures safe property access, and returning the first match for duplicate
modelFqnvalues is a reasonable behavior.packages/diagram/src/likec4diagram/state/utils.spec.ts (1)
5-65: LGTM!Excellent test coverage for
findNodeByElementFqn. The tests comprehensively validate all edge cases, including empty arrays, missing properties, null values, and mixed data shapes.
packages/diagram/src/likec4diagram/state/machine.state.ready.focused.spec.ts
Outdated
Show resolved
Hide resolved
|
|
||
| export const startAutoUnfocusTimer = () => | ||
| machine.enqueueActions(({ context, enqueue }) => { | ||
| if (context.autoUnfocusTimer && AUTO_UNFOCUS_DELAY > 0) { |
There was a problem hiding this comment.
But AUTO_UNFOCUS_DELAY is a const 0, this condition is always false.
I presume, intention was to have a constant for delay
There was a problem hiding this comment.
I had a variety of values in there, and opted to leave it as zero so it can be (easily) adjusted later, or maybe even configurable...
|
|
||
| focusOnElement: (elementFqn: Fqn<A>) => { | ||
| const context = actorRef.current.getSnapshot().context | ||
| const node = context.xynodes.find(n => 'modelFqn' in n.data && n.data.modelFqn === elementFqn) |
There was a problem hiding this comment.
You can reuse yours findNodeByElementFqn
| const node = context.xynodes.find(n => 'modelFqn' in n.data && n.data.modelFqn === elementFqn) | |
| const node = findNodeByElementFqn(context.xynodes, elementFqn) |
| import type { DiagramActorRef, System } from './types' | ||
| import type { DiagramActorRef, NodeWithData, System } from './types' | ||
|
|
||
| export const findNodeByElementFqn = <T extends NodeWithData>( |
There was a problem hiding this comment.
Two options
- rename to
findNodeIdByModelFqn(semantically better) - change signature to something like:
function findNodeByModelFqn(
nodes: Types.Node[],
modelFqn: Fqn,
): (Types.Node & { data: { modelFqn: Fqn } }) | null Enrich contract by explictly saying that returned node has data with modelFqn
I would go with the second
| } from './machine' | ||
| import type { SyncLayoutActorRef } from './syncManualLayoutActor' | ||
|
|
||
| export type NodeWithData = { id: string; data: Record<string, unknown> } |
There was a problem hiding this comment.
Looks like the only place where you need - tests.
I suggest a wrapper function inside the tests:
function testFindNodeByElementFqn(
nodes: Array<{ id: string; data: {modelFqn?: string | null }}>,
modelFqn: string
) {
return findNodeByElementFqn(testdata.nodes as any, testdata.modelFqn as any)
}This removes as Fqn in tests and neediness of this type
| }) | ||
| }) | ||
|
|
||
| it('should prioritize project folder over include paths when matching documents', async ({ expect }) => { |
| /** | ||
| * Resolved include paths as URIs. | ||
| * These are additional directories that are part of this project. | ||
| */ | ||
| includePaths?: URI[] | ||
| /** | ||
| * Include paths as folder strings (with trailing slash) for faster matching. | ||
| */ | ||
| includePathsAsStrings?: ProjectFolder[] |
There was a problem hiding this comment.
Now there is no guarantee in the interface. that includePaths and includePathsAsStrings both exist and have same length
This would be better
includePaths?: Array<{
uri: URI,
folder: ProjectFolder
}> 
|
@copilot address my comments in another PR |
Co-authored-by: davydkov <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/language-server/src/workspace/ProjectsManager.ts (1)
379-431: Include-path resolution and overlap checks look correct; could be factored outThe block correctly:
- Resolves
config.includeentries relative toproject.folderUri.- Normalizes them into
{ uri, folder: ProjectFolder(uri) }.- Logs the resolved include paths.
- Warns when include paths overlap with other projects’ folders or include paths.
- Clears
project.includePathswhen no includes are configured to avoid stale state.You might consider extracting this into a small helper (e.g.,
resolveIncludePaths(project, config.include, this.#projects)) to keepregisterProjectshorter and make include-path behavior easier to test in isolation, but it’s not strictly necessary.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/diagram/src/likec4diagram/state/diagram-api.ts(5 hunks)packages/diagram/src/likec4diagram/state/machine.actions.ts(3 hunks)packages/diagram/src/likec4diagram/state/machine.state.navigating.ts(4 hunks)packages/diagram/src/likec4diagram/state/utils.spec.ts(1 hunks)packages/diagram/src/likec4diagram/state/utils.ts(2 hunks)packages/language-server/src/views/LikeC4ManualLayouts.spec.ts(5 hunks)packages/language-server/src/workspace/ProjectsManager.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/diagram/src/likec4diagram/state/utils.spec.ts
- packages/diagram/src/likec4diagram/state/machine.actions.ts
- packages/diagram/src/likec4diagram/state/machine.state.navigating.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
pnpm ci:typecheckfor TypeScript type checking across all packages except documentation
Files:
packages/diagram/src/likec4diagram/state/utils.tspackages/language-server/src/views/LikeC4ManualLayouts.spec.tspackages/language-server/src/workspace/ProjectsManager.tspackages/diagram/src/likec4diagram/state/diagram-api.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use oxlint with type-aware rules for linting instead of eslint, and run
pnpm ci:lintto validate all packages except documentation
Files:
packages/diagram/src/likec4diagram/state/utils.tspackages/language-server/src/views/LikeC4ManualLayouts.spec.tspackages/language-server/src/workspace/ProjectsManager.tspackages/diagram/src/likec4diagram/state/diagram-api.ts
**/*.{ts,tsx,js,jsx,json,md}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
pnpm fmtto format code with dprint instead of Prettier or eslint for formatting
Files:
packages/diagram/src/likec4diagram/state/utils.tspackages/language-server/src/views/LikeC4ManualLayouts.spec.tspackages/language-server/src/workspace/ProjectsManager.tspackages/diagram/src/likec4diagram/state/diagram-api.ts
packages/*/**/!(node_modules)/**/*.spec.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Unit tests must be located in
packages/*/src/**/*.spec.tsorpackages/*/src/**/__test__/*.spec.tsfiles
Files:
packages/language-server/src/views/LikeC4ManualLayouts.spec.ts
🧬 Code graph analysis (3)
packages/diagram/src/likec4diagram/state/utils.ts (1)
packages/diagram/src/likec4diagram/state/types.ts (1)
NodeWithData(11-11)
packages/language-server/src/views/LikeC4ManualLayouts.spec.ts (1)
packages/likec4/src/LikeC4.ts (1)
projectsManager(166-168)
packages/diagram/src/likec4diagram/state/diagram-api.ts (2)
packages/core/src/model/view/LikeC4ViewModel.ts (1)
node(304-307)packages/diagram/src/likec4diagram/state/utils.ts (1)
findNodeByModelFqn(23-29)
⏰ 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). (4)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: checks / 🛠️ build
- GitHub Check: checks / ⊞ windows build
- GitHub Check: checks / ʦ typescript
🔇 Additional comments (16)
packages/diagram/src/likec4diagram/state/utils.ts (2)
5-6: LGTM!The new imports support the
findNodeByModelFqnfunction implementation.
23-29: Good implementation that addresses the past review feedback.The function signature now explicitly enriches the return type to indicate the presence of
modelFqnin the node's data, which addresses davydkov's suggestion. The implementation is type-safe: the runtime check verifiesmodelFqnexists in the data, and the equality comparison with the typedelementFqnparameter ensures type compatibility.Based on past review comments, davydkov suggested enriching the contract by explicitly typing the returned node's data—this has been implemented.
packages/diagram/src/likec4diagram/state/diagram-api.ts (5)
20-20: LGTM!The import correctly brings in the new
findNodeByModelFqnutility function.
50-54: Well-documented API extension.The JSDoc clearly explains the purpose of the new
focusOnElementparameter for navigation triggered from search.
105-109: Clear documentation for the new API method.The JSDoc explains the distinction between
focusNode(by node ID) andfocusOnElement(by model FQN), and notes its use case for search-driven highlighting.
151-157: Clean implementation of the optional parameter.The conditional spread pattern correctly includes
focusOnElementin the event payload only when provided.
220-226: Good implementation that reuses the utility function.The implementation correctly uses
findNodeByModelFqnto locate the node by its model FQN and sends the focus event withautoUnfocus: truewhen found. The silent handling of the not-found case is appropriate for this use case.Based on past review comments, davydkov suggested reusing the
findNodeByModelFqnfunction—this has been implemented at line 222.packages/language-server/src/views/LikeC4ManualLayouts.spec.ts (3)
26-32: UsinggetProjectby id in tests is a good alignment with the public APISwitching from the raw
registerProjectresult togetProject(projectData.id)ensures tests exercise the sameProjectshape real callers see (including future fields likeincludePaths). No issues here.
184-195: Consistent pattern for custommanualLayouts.outDirproject setupReusing the
registerProject→getProject(id)pattern in this test keeps it consistent with the helper above and ensures the outDir scenario also goes through the publicProjectsManagerAPI surface.
273-284: Write-path test also correctly uses the publicProjectviewSame pattern here for the write case: resolving the project via
getProjectis appropriate and keeps the test aligned with runtime usage ofManualLayouts.write.packages/language-server/src/workspace/ProjectsManager.ts (6)
61-68: Good internal representation forincludePathsonProjectDataStoring include paths as
{ uri, folder }pairs onProjectDatakeeps the URI object and normalized folder string in sync and addresses the earlier concern about parallel arrays drifting. This looks solid.
75-79: PublicProject.includePathssurface is appropriately narrowedExposing only
includePaths?: URI[]on the publicProjectinterface is a reasonable abstraction over the richer internalProjectData.includePathsshape, and keeps callers from depending on internal folder-string normalization. No issues here.
212-218:getProjectcorrectly maps internalincludePathsto public URIsThe new
getProjectreturn shape pullsfolderUriandconfigdirectly fromProjectDataand conditionally mapsproject.includePathsto an array ofURIs. This keepsProjectminimal while still surfacing include information when configured; behavior remains well-defined whenincludePathsis absent.
564-579: Rebuild now correctly includes documents from include pathsExtending the rebuild filter to:
- Precompute
includePathStringsfromproject.includePaths, and- Include any document whose URI string starts with one of those include-folder prefixes,
ensures documents in shared/include directories are rebuilt with their owning project. The early return for direct
foldermatches and the existing directory-prefix fallback keep previous behavior intact.
600-616:findProjectForDocumentnow accounts for include paths before falling back to defaultThe new logic:
- Tries project folders (
documentUri.startsWith(folder)),- Then tries include paths (
documentUri.startsWith(includePath.folder)),- Finally falls back to
this.default,which matches the intended precedence and the overlap warning semantics (“files in overlapping areas will only belong to one project”). Caching via
WorkspaceCachekeeps the extra lookup cost under control.
633-647:getAllIncludePathsprovides a clean aggregation API for WorkspaceManager
getAllIncludePathsneatly flattens all projects’ include paths into{ projectId, includePath }pairs, which is exactly what callers likeWorkspaceManagerneed to scan additional directories. Implementation is straightforward and side-effect-free.
| target: targetState.focused, | ||
| }, | ||
| 'focus.node': [ | ||
| // Focus was initialed by the user searching (autoUnfocus=true) - always allowed |
There was a problem hiding this comment.
Spelling error: "initialed" should be "initiated"
| // Focus was initialed by the user searching (autoUnfocus=true) - always allowed | |
| // Focus was initiated by the user searching (autoUnfocus=true) - always allowed |
Signed-off-by: Denis Davydkov <[email protected]>
Introduced a new
includeconfiguration option to LikeC4 project configs, allowing users to specify additional directories to include source files from outside the main project folder.Supporting documentation, validation logic, and tests have been added and updated. Additionally, there are minor dependency updates and a small fix in the icon update script due to a (remote) file naming change.
Checklist
mainbefore creating this PR.Summary by CodeRabbit
New Features
UI Enhancements
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.