feat: #1483 add a property to deployment view to enable ancestor node…#2935
Merged
Conversation
…s of visible nodes to show in the diagram
🦋 Changeset detectedLatest commit: 642df3f The changes in this PR will be included in the next version bump. 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 |
Contributor
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds an includeAncestors boolean view rule and plumbing across grammar, parser, validation schemas, builder helpers, generator output, compute pipeline (StageFinal), tests, and documentation so deployment views can include ancestor nodes of visible elements. ChangesDeployment View includeAncestors
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/generators/src/likec4/operators/views.spec.ts (1)
307-341: 💤 Low value
describe('showAncestors')is nested insidedescribe('view')but testsdeploymentViewThe test reporter will surface these as
view > showAncestors > ..., which is misleading. Consider moving theshowAncestorssuite to its own top-leveldescribe('deploymentView', ...)block (or an existing deployment-view describe group).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/generators/src/likec4/operators/views.spec.ts` around lines 307 - 341, The nested test suite describe('showAncestors') is incorrectly scoped under describe('view') which mislabels tests in the reporter; relocate this entire describe('showAncestors') block so it is nested under a top-level describe for deploymentView (or an existing deployment-view describe group) instead of under 'view', ensuring the tests that call deploymentView(..., $rules($showAncestors(...))) remain unchanged other than their describe wrapper so the reporter shows e.g. deploymentView > showAncestors > ... .packages/core/src/compute-view/deployment-view/__test__/fixture.ts (1)
74-74: 💤 Low valueUntyped
bparameter — use the already-importedDeploymentRulesBuilderOp<Types>
b: anyis unnecessary here sinceDeploymentRulesBuilderOp<Types>is already imported and directly describes the return type of this helper.♻️ Proposed fix
-const $showAncestors = (value: boolean) => (b: any) => b.showAncestors(value) +const $showAncestors = (value: boolean): DeploymentRulesBuilderOp<Types> => b => b.showAncestors(value)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/compute-view/deployment-view/__test__/fixture.ts` at line 74, Replace the untyped parameter in the helper $showAncestors: change the parameter type from any to the imported DeploymentRulesBuilderOp<Types> so the function signature reads like the helper accepts and returns a DeploymentRulesBuilderOp<Types> (i.e., update the parameter type on $showAncestors to DeploymentRulesBuilderOp<Types> and ensure the return type aligns with b.showAncestors(value)).packages/core/src/builder/Builder.view-deployment.ts (1)
7-11: 💤 Low valueVerify whether
showAncestorsdeclaration is redundantPer the AI summary,
LikeC4ViewBuilderinBuilder.view-common.tsalready declaresshowAncestors(value: boolean): this. SinceDeploymentViewBuilderextendsLikeC4ViewBuilder, the re-declaration on line 10 is likely redundant (the inherited signature already resolves correctly via the covariantthistype).#!/bin/bash # Check whether LikeC4ViewBuilder already declares showAncestors rg -n "showAncestors" packages/core/src/builder/Builder.view-common.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/builder/Builder.view-deployment.ts` around lines 7 - 11, The showAncestors method is already declared on LikeC4ViewBuilder, so remove the redundant showAncestors(value: boolean): this declaration from the DeploymentViewBuilder interface to rely on the inherited signature; update the DeploymentViewBuilder interface (which extends LikeC4ViewBuilder) by deleting that method declaration and run type-check/tests to confirm the covariant this typing still resolves correctly for DeploymentViewBuilder.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/builder/Builder.views.ts`:
- Around line 358-359: The global ViewsHelpers type currently exposes
$showAncestors so non-deployment builders can call it even though mkViewBuilder
only attaches showAncestors to deployment builders; to fix, remove
$showAncestors from the global ViewsHelpers (or create a DeploymentViewsHelpers
subtype) and update mkViewBuilder and any builder-return types to use the
deployment-only helper type where showAncestors is provided; specifically adjust
the ViewsHelpers declaration (remove $showAncestors), add a
DeploymentViewsHelpers (or similar) that includes $showAncestors, and ensure
mkViewBuilder returns a builder typed with DeploymentViewsHelpers while element
builders keep the original ViewsHelpers so $showAncestors no longer type-checks
for non-deployment builders.
In `@packages/core/src/compute-view/deployment-view/stages/stage-final.ts`:
- Around line 176-193: step4AddAncestor currently unions all ancestors of
memory.final back into final without respecting explicit exclusions; change it
to filter out any ancestor that is present in the exclusion set (e.g.,
memory.excluded or whatever memory tracks exclusions) before unioning: compute
ancestors as now, then remove ancestors that are in memory.excluded (or that
would be rejected by the same exclusion predicate used earlier) and only union
the remaining ancestors into memory.final via union(...) and memory.update(...).
Update references in this function (step4AddAncestor, memory.final,
memory.excluded, union, memory.update, el.ancestors()) and add a regression test
that includes an excluded ancestor (e.g., exclude prod.eu, include
prod.eu.zone1.ui with showAncestors(true)) to assert prod.eu is not re-added.
---
Nitpick comments:
In `@packages/core/src/builder/Builder.view-deployment.ts`:
- Around line 7-11: The showAncestors method is already declared on
LikeC4ViewBuilder, so remove the redundant showAncestors(value: boolean): this
declaration from the DeploymentViewBuilder interface to rely on the inherited
signature; update the DeploymentViewBuilder interface (which extends
LikeC4ViewBuilder) by deleting that method declaration and run type-check/tests
to confirm the covariant this typing still resolves correctly for
DeploymentViewBuilder.
In `@packages/core/src/compute-view/deployment-view/__test__/fixture.ts`:
- Line 74: Replace the untyped parameter in the helper $showAncestors: change
the parameter type from any to the imported DeploymentRulesBuilderOp<Types> so
the function signature reads like the helper accepts and returns a
DeploymentRulesBuilderOp<Types> (i.e., update the parameter type on
$showAncestors to DeploymentRulesBuilderOp<Types> and ensure the return type
aligns with b.showAncestors(value)).
In `@packages/generators/src/likec4/operators/views.spec.ts`:
- Around line 307-341: The nested test suite describe('showAncestors') is
incorrectly scoped under describe('view') which mislabels tests in the reporter;
relocate this entire describe('showAncestors') block so it is nested under a
top-level describe for deploymentView (or an existing deployment-view describe
group) instead of under 'view', ensuring the tests that call deploymentView(...,
$rules($showAncestors(...))) remain unchanged other than their describe wrapper
so the reporter shows e.g. deploymentView > showAncestors > ... .
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 75ce4ad3-59c2-4f26-8b86-04eab727455d
📒 Files selected for processing (16)
.changeset/show-ancestors-deployment-view.mdpackages/core/src/builder/Builder.tspackages/core/src/builder/Builder.view-common.tspackages/core/src/builder/Builder.view-deployment.tspackages/core/src/builder/Builder.views.tspackages/core/src/compute-view/deployment-view/__test__/fixture.tspackages/core/src/compute-view/deployment-view/__test__/include.deployment.spec.tspackages/core/src/compute-view/deployment-view/compute.spec.tspackages/core/src/compute-view/deployment-view/compute.tspackages/core/src/compute-view/deployment-view/stages/stage-final.tspackages/core/src/types/view-parsed.deployment.tspackages/generators/src/likec4/operators/views.spec.tspackages/generators/src/likec4/operators/views.tspackages/generators/src/likec4/schemas/views.tspackages/language-server/src/like-c4.langiumpackages/language-server/src/model/parser/DeploymentViewParser.ts
Comment on lines
+176
to
+193
| public step4AddAncestor(memory: Memory): Memory { | ||
| // Collect all ancestors of elements in the final set | ||
| const ancestors = new Set<Elem>() | ||
| for (const el of memory.final) { | ||
| // Use el.ancestors() to get ancestor elements directly | ||
| for (const ancestor of el.ancestors()) { | ||
| ancestors.add(ancestor) | ||
| } | ||
| } | ||
|
|
||
| // If we found ancestors, add them to the final set | ||
| if (ancestors.size > 0) { | ||
| const updatedFinal = union(memory.final, ancestors) | ||
| return memory.update({ final: updatedFinal }) | ||
| } | ||
|
|
||
| return memory | ||
| } |
Contributor
There was a problem hiding this comment.
Excluded ancestors get re-added here.
step4AddAncestor() unions every ancestor of memory.final back into the result after all filtering. If a user explicitly excludes prod.eu but includes prod.eu.zone1.ui with showAncestors(true), this code will resurrect prod.eu anyway. Ancestor injection needs to respect the exclude result, or pass back through the same exclusion pipeline before commit. Please add a regression for an excluded ancestor with the fix.
Also applies to: 260-265
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/compute-view/deployment-view/stages/stage-final.ts` around
lines 176 - 193, step4AddAncestor currently unions all ancestors of memory.final
back into final without respecting explicit exclusions; change it to filter out
any ancestor that is present in the exclusion set (e.g., memory.excluded or
whatever memory tracks exclusions) before unioning: compute ancestors as now,
then remove ancestors that are in memory.excluded (or that would be rejected by
the same exclusion predicate used earlier) and only union the remaining
ancestors into memory.final via union(...) and memory.update(...). Update
references in this function (step4AddAncestor, memory.final, memory.excluded,
union, memory.update, el.ancestors()) and add a regression test that includes an
excluded ancestor (e.g., exclude prod.eu, include prod.eu.zone1.ui with
showAncestors(true)) to assert prod.eu is not re-added.
Collaborator
Author
There was a problem hiding this comment.
For me it is working as intented. The purpose of "showAncestors" is to make visible all ancestors without exception. It's not surprising if it takes precedence over exclusion.
If an element is excluded in the view it could be an easy way to exclude its relationships but if you want to exclude an ancestor explicitely it make no sense to me to use "showAncestors".
Contributor
There was a problem hiding this comment.
@Kiiv, that's a clear and reasonable design rationale — showAncestors(true) is a deliberate "show everything up the hierarchy" switch, and the expectation that exclude is used primarily to suppress relationships rather than to hide ancestor nodes is a coherent model. I'll drop this concern.
✏️ Learnings added
Learnt from: Kiiv
Repo: likec4/likec4 PR: 2935
File: packages/core/src/compute-view/deployment-view/stages/stage-final.ts:176-193
Timestamp: 2026-05-05T14:48:25.830Z
Learning: In `packages/core/src/compute-view/deployment-view/stages/stage-final.ts`, `step4AddAncestor` intentionally adds ALL ancestor nodes of elements in `memory.final` without respecting prior `exclude` rules. This is by design: `showAncestors(true)` takes precedence over exclusions. The author's intent is that `exclude` rules are used to suppress relationships/connections, not ancestor node visibility; combining `exclude` on an ancestor node with `showAncestors(true)` is considered a contradictory/unsupported use case.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
davydkov
approved these changes
May 7, 2026
davydkov
left a comment
Member
There was a problem hiding this comment.
Good one! Thanks for contribution!
We need (in follow-up PRs):
- Update documentation
- Add reserved keyword to textmate
I do not insist, but maybe includeAncestors fits better?
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/core/src/builder/Builder.views.ts (1)
13-21: 💤 Low valueTwo consecutive imports from the same module can be merged.
Lines 20–21 both import from
'./Builder.view-deployment'; combining them reduces noise.♻️ Proposed refactor
-import { $includeAncestors } from './Builder.view-deployment' -import { type AddDeploymentViewHelper, type DeploymentViewBuilder, $deploymentExpr } from './Builder.view-deployment' +import { $includeAncestors, type AddDeploymentViewHelper, type DeploymentViewBuilder, $deploymentExpr } from './Builder.view-deployment'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/builder/Builder.views.ts` around lines 13 - 21, Merge the two imports from './Builder.view-deployment' into a single import statement: replace the separate imports of $includeAncestors and the trio AddDeploymentViewHelper, DeploymentViewBuilder, $deploymentExpr with one combined import that includes all symbols ($includeAncestors, AddDeploymentViewHelper, DeploymentViewBuilder, $deploymentExpr) to reduce redundancy and improve readability.packages/core/src/types/view-parsed.deployment.ts (1)
35-41: 💤 Low valueUnused generic parameter
AonParsedViewRuleAncestors.
A extends AnyAux = Unknownis declared but never referenced inside the interface body — onlyincludeAncestors: booleanis present. This mirrorsViewRuleAutoLayout(also non-generic) used in the sameExclusiveUnion, but attaching a phantom type parameter risks misleading future readers into thinkingAshapes the type.♻️ Proposed refactor
-export interface ParsedViewRuleAncestors<A extends AnyAux = Unknown> { +export interface ParsedViewRuleAncestors { /** * When true, ancestor elements of included deployment nodes are also included in the view. * `@default` false (ancestors are not included) */ includeAncestors: boolean }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/types/view-parsed.deployment.ts` around lines 35 - 41, The generic parameter A on ParsedViewRuleAncestors (declared as A extends AnyAux = Unknown) is unused and should be removed to avoid a phantom type; update the interface declaration to be non-generic (remove the <A extends AnyAux = Unknown> clause) and adjust any references or type usages expecting ParsedViewRuleAncestors<A> to use the non-generic ParsedViewRuleAncestors instead, ensuring imports of AnyAux/Unknown are left only where actually needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/docs/src/content/docs/dsl/Deployment/views.mdx`:
- Around line 129-195: Fix the three typos in the prose: change "Sometime" to
"Sometimes", "usefull" to "useful", and "deployement" to "deployment"; then fix
the indentation of the closing braces for the tomcat1 and tomcat2 blocks so they
align with their opening lines inside the VM/deploymentNode blocks (adjust the
two closing "}" for tomcat1 and tomcat2 to match the block-opener indentation),
and replace the tab character used to close the "deployment view ancestors_test"
block with spaces to match the file's indentation; reference symbols:
includeAncestors, deployment view ancestors_test, tomcat1, tomcat2.
In `@packages/core/src/compute-view/deployment-view/__test__/fixture.ts`:
- Line 74: The test redefines $includeAncestors locally with an any-typed lambda
which bypasses the properly typed helper exported in ViewsHelpers; update the
existing destructuring that already pulls in $include and $exclude from
Builder.forSpecification (the ViewsHelpers) to also import $includeAncestors,
then delete the local definition const $includeAncestors = (value: boolean) =>
(b: any) => b.includeAncestors(value) so the test uses the correctly typed
$includeAncestors helper from the builder.
---
Nitpick comments:
In `@packages/core/src/builder/Builder.views.ts`:
- Around line 13-21: Merge the two imports from './Builder.view-deployment' into
a single import statement: replace the separate imports of $includeAncestors and
the trio AddDeploymentViewHelper, DeploymentViewBuilder, $deploymentExpr with
one combined import that includes all symbols ($includeAncestors,
AddDeploymentViewHelper, DeploymentViewBuilder, $deploymentExpr) to reduce
redundancy and improve readability.
In `@packages/core/src/types/view-parsed.deployment.ts`:
- Around line 35-41: The generic parameter A on ParsedViewRuleAncestors
(declared as A extends AnyAux = Unknown) is unused and should be removed to
avoid a phantom type; update the interface declaration to be non-generic (remove
the <A extends AnyAux = Unknown> clause) and adjust any references or type
usages expecting ParsedViewRuleAncestors<A> to use the non-generic
ParsedViewRuleAncestors instead, ensuring imports of AnyAux/Unknown are left
only where actually needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ad1abf12-464c-4abd-950d-ecafa5f4ffdb
📒 Files selected for processing (19)
.changeset/show-ancestors-deployment-view.mdapps/docs/likec4.tmLanguage.jsonapps/docs/src/content/docs/dsl/Deployment/views.mdxapps/playground/likec4.tmLanguage.jsonpackages/core/src/builder/Builder.tspackages/core/src/builder/Builder.view-deployment.tspackages/core/src/builder/Builder.views.tspackages/core/src/compute-view/deployment-view/__test__/fixture.tspackages/core/src/compute-view/deployment-view/__test__/include.deployment.spec.tspackages/core/src/compute-view/deployment-view/compute.spec.tspackages/core/src/compute-view/deployment-view/compute.tspackages/core/src/compute-view/deployment-view/stages/stage-final.tspackages/core/src/types/view-parsed.deployment.tspackages/generators/src/likec4/operators/views.spec.tspackages/generators/src/likec4/operators/views.tspackages/generators/src/likec4/schemas/views.tspackages/language-server/src/like-c4.langiumpackages/language-server/src/model/parser/DeploymentViewParser.tspackages/vscode/likec4.tmLanguage.json
✅ Files skipped from review due to trivial changes (2)
- packages/vscode/likec4.tmLanguage.json
- apps/docs/likec4.tmLanguage.json
🚧 Files skipped from review as they are similar to previous changes (11)
- packages/language-server/src/model/parser/DeploymentViewParser.ts
- .changeset/show-ancestors-deployment-view.md
- packages/core/src/compute-view/deployment-view/compute.ts
- packages/generators/src/likec4/schemas/views.ts
- packages/core/src/builder/Builder.ts
- packages/core/src/compute-view/deployment-view/compute.spec.ts
- packages/generators/src/likec4/operators/views.ts
- packages/core/src/compute-view/deployment-view/test/include.deployment.spec.ts
- packages/language-server/src/like-c4.langium
- packages/generators/src/likec4/operators/views.spec.ts
- packages/core/src/builder/Builder.view-deployment.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…s of visible nodes to show in the diagram
Checklist
mainbefore creating this PR.pnpm typecheckandpnpm test./changeset-generatorSKILL).