packages/core/src/compute-view/utils/relationExpressionToPredicates.ts (1)

46-48: ⚖️ Poor tradeoff

any usage in factory signature violates the "avoid any" guideline

elementPredicateBuilder: (expr: any) => (node: any) => boolean and the (expr: any) => Predicate<T> return type use any in production code. The JSDoc acknowledges this and documents the reason (incompatible expression/node types across element vs. deployment view paths), but the type parameter A on the factory is effectively unused since elementPredicateBuilder doesn't reference it — callers cannot infer A from the any-typed parameter.

If a common base expression type (analogous to ExpandableRelation for relations) can be extracted, that would allow tightening these signatures. Otherwise, consider a comment-level // eslint-disable-next-line @typescript-eslint/no-explicit-any`` or similar oxlint suppression to make the intentional deviation explicit rather than relying solely on JSDoc prose.

🤖 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/compute-view/utils/relationExpressionToPredicates.ts`
around lines 46 - 48, The factory signature for
createRelationExpressionToPredicates uses broad any types
(elementPredicateBuilder: (expr: any) => (node: any) => boolean and returns
(expr: any) => Predicate<T>) which prevents inferring the generic A and violates
the no-any rule; either introduce a shared base expression type (e.g.,
ExtractableRelationExpression or similar) and replace the any occurrences with
that type so A can be referenced in elementPredicateBuilder and the return type,
or if a shared type cannot be defined, add an explicit lint suppression (//
eslint-disable-next-line `@typescript-eslint/no-explicit-any`) immediately above
the elementPredicateBuilder parameter and the function return signature and
include a short comment explaining why any is intentional; update the function
signature of createRelationExpressionToPredicates and elementPredicateBuilder
accordingly so callers can infer generics or the intentional exemption is
documented.

packages/core/src/compute-view/utils/view-hash.ts (1)

33-48: 💤 Low value

id is redundant in both the map key and the spread value

mapToObj((e) => [e.id, { ...e }]) uses e.id as the key while { ...e } also contains id in the value, so the hash receives id twice. This is harmless (the hash remains deterministic), but the spread value duplicates data that's already captured by the key.

♻️ Optional cleanup

-      mapToObj((e) => [e.id, { ...e }]),
+      mapToObj(({ id, ...rest }) => [id, rest]),

🤖 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/compute-view/utils/view-hash.ts` around lines 33 - 48, The
edges mapping duplicates the edge id by using e.id as the map key and also
including id inside the spread value in mapToObj; update the transformation so
the mapped value does not include id (e.g., build the value object without the
id field) when creating the [e.id, value] pair for mapToObj, leaving the rest of
the properties produced by the earlier map (flattenMarkdownOrString, technology,
dir, head, tail, line) unchanged; modify the code around view.edges / map(...) /
mapToObj(...) to omit id from the value.

packages/core/src/compute-view/element-view/utils.ts (2)

27-32: 💤 Low value

Stale commented-out code can be removed.

Lines 28-29 (// const modelRelations = [], // const deploymentRelations = []) are dead scaffolding from a prior iteration. Drop them to keep the function focused.

🤖 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/compute-view/element-view/utils.ts` around lines 27 - 32,
Remove the stale commented-out scaffolding inside the connections.flatMap
callback: delete the two commented lines "// const modelRelations = []" and "//
const deploymentRelations = []" so the code focuses on the real variables
(connections.flatMap, conn, relations, conn.relations) without leftover dead
comments.

---

40-115: 💤 Low value

directRelsList is computed eagerly even when the expansion branch returns early.

Line 40 always computes directRelsList, but it is only used at line 115 (the non-expanded fallback). When shouldExpand && relations.length > 1 and there are expanded relations, the function returns at line 101 without ever using directRelsList. Move the computation just above its first use to avoid the unnecessary filter per connection.

-    const directRelsList = filter(relations, r => r.source.id === source && r.target.id === target)
-
     if (shouldExpand && relations.length > 1) {
       ...
     }
@@
-      only(directRelsList)?.$relationship,
+      only(filter(relations, r => r.source.id === source && r.target.id === target))?.$relationship,
     )

Same kind of helper extraction suggested for the deployment-view sibling would also dedupe the destructuring of title/color/line/head/...props repeated three times here.

🤖 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/compute-view/element-view/utils.ts` around lines 40 - 115,
The eager computation of directRelsList causes an unnecessary filter; move the
directRelsList = filter(relations, ...) expression out of the top and compute it
just before it's used in the non-expanded fallback (right before the
mergePropsFromRelationships call that uses only(directRelsList)?.$relationship).
Also extract the repeated destructuring/prop-merge logic (the
title/color/line/head/...props block and the construction of ComputedEdge
objects) into a small helper (e.g., mergePropsFromRelationshipsAndBuildEdge or
formatComputedEdge) and reuse it from the expanded mapping, the merged push
block, and the fallback to remove duplication and clarify where shouldExpand,
relations, mergePropsFromRelationships, and conn.id are used.

packages/core/src/compute-view/deployment-view/utils.ts (3)

282-307: 💤 Low value

only(filter(merged, …)) will silently drop the hint when several merged relations are direct.

only returns undefined when the filter yields more than one result, so as soon as two of the "merged" relations are direct (source.id === source && target.id === target), mergedSource becomes undefined and mergePropsFromRelationships is called without a hint. That's likely fine, but it's worth a brief comment explaining why an undefined hint is preferable to picking the first one — otherwise a future reader will be tempted to "fix" it with first(...).

🤖 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/compute-view/deployment-view/utils.ts` around lines 282 -
307, The call only(filter(merged, ...)) can return undefined when multiple
merged relations match, which intentionally avoids selecting an arbitrary
"first" relation as the hint for mergePropsFromRelationships; add a brief
explanatory comment above the mergedSource assignment (referencing only, filter,
mergedSource, mergePropsFromRelationships and merged) stating that undefined is
preferred over picking the first match because a hint from one direct relation
would be misleading when multiple direct relations exist, so we deliberately
pass undefined to let mergePropsFromRelationships compute without a biased hint.

---

233-336: ⚡ Quick win

Address SonarCloud cognitive-complexity warning (45 vs 30 allowed).

Static analysis flagged this function. The expanded-vs-merged branch shares the mergePropsFromRelationships + defaults destructuring with the bottom non-expanded branch, and the inner "if merged.length > 0" block repeats it again. Extracting a small buildEdgeFromRelations(conn, source, target, defaults, relations, hint?) helper that returns a ComputedEdge<A> would collapse three near-identical blocks into one and bring complexity well under the threshold. As a side effect it also removes the two as ComputedEdge<A> casts.

🤖 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/compute-view/deployment-view/utils.ts` around lines 233 -
336, The toComputedEdges function is over-complex due to repeated logic for
merging relationship props; extract a helper (e.g., buildEdgeFromRelations) that
accepts (conn, source, target, defaults, relations, optionalHintRelationship)
and returns a ComputedEdge<A> by calling mergePropsFromRelationships, applying
defaults, building id/parent/label/relations/color/line/head and other props;
replace the three near-identical code blocks in toComputedEdges (the expanded
loop body, the merged.length > 0 block, and the final non-expanded branch) with
calls to buildEdgeFromRelations, remove the redundant "as ComputedEdge<A>"
casts, and ensure hashEdgeId usage is preserved for expanded single-relation
edges while the helper produces the standard conn.id for merged/multi relations.

---

204-231: 💤 Low value

mergeBidirectionalEdges mutates input edge objects in place.

edge.dir = 'both', edge.head = …, edge.color = …, edge.line = … mutate the ComputedEdge objects that came from nonExpandedEdges, which were just constructed from mergePropsFromRelationships results and from exact({...}). While today's caller passes a freshly built array, returning a new array via filter(...) while mutating the surviving elements is a mixed pattern that's easy to misuse later (e.g. if a future caller passes shared edges). Consider building new edge objects via spread instead of mutating, and dropping as ComputedEdge<A> once typing is tightened.

Sketch

-      if (edge.source === other.target && edge.target === other.source && edge.label === other.label) {
-        edge.dir = 'both'
-        const head = edge.head ?? other.head
-        if (head) {
-          if (!edge.head) edge.head = head
-          if (!edge.tail) edge.tail = head
-        }
-        if (other.color) {
-          if (!edge.color) edge.color = other.color
-        }
-        if (other.line) {
-          if (!edge.line) edge.line = other.line
-        }
-        toRemove.add(j)
-        break
-      }
+      if (edge.source === other.target && edge.target === other.source && edge.label === other.label) {
+        const head = edge.head ?? other.head
+        edges[i] = {
+          ...edge,
+          dir: 'both',
+          head: edge.head ?? head,
+          tail: edge.tail ?? head,
+          color: edge.color ?? other.color,
+          line: edge.line ?? other.line,
+        }
+        toRemove.add(j)
+        break
+      }

🤖 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/compute-view/deployment-view/utils.ts` around lines 204 -
231, mergeBidirectionalEdges currently mutates incoming ComputedEdge objects
(setting edge.dir, head, color, line) which is unsafe; instead, when you need to
modify an edge (e.g., when matching a reverse edge) create a new edge object via
a shallow copy (use {...edge, dir: 'both', head: computedHead, color: edge.color
?? other.color, line: edge.line ?? other.line}) and place that new object into
the result list, leaving the original edges untouched; ensure to build the final
returned array from these new objects (and unmodified originals) rather than
filtering the mutated input, and remove any unnecessary type assertions like "as
ComputedEdge<A>" once types are tightened.

packages/core/src/compute-view/element-view/compute.ts (1)

209-216: 💤 Low value

Iterate relSpecs entries directly to avoid the keyof+as any indirection.

Object.keys(relSpecs) followed by relSpecs[kind as keyof typeof relSpecs] as any is convoluted. Object.entries (or for…in if the order matters) yields the value without the second cast, and the runtime fallback style ?? spec can stay. Also worth confirming model.specification.relationships is guaranteed defined here — defensive ?? {} would avoid a crash if a model is constructed without a specification block.

-  const multipleKinds = new Set<string>()
-  const relSpecs = model.specification.relationships
-  for (const kind of Object.keys(relSpecs)) {
-    const spec = relSpecs[kind as keyof typeof relSpecs] as any
-    // The spec may have `multiple` nested under `style` (buildModel.ts) or flat (builder)
-    const style = spec?.style ?? spec
-    if (style?.multiple) {
-      multipleKinds.add(kind)
-    }
-  }
+  const multipleKinds = new Set<string>()
+  for (const [kind, spec] of Object.entries(model.specification.relationships ?? {})) {
+    // The spec may have `multiple` nested under `style` (buildModel.ts) or flat (builder)
+    const style = (spec as { style?: { multiple?: boolean }; multiple?: boolean })?.style ?? spec
+    if ((style as { multiple?: boolean })?.multiple) {
+      multipleKinds.add(kind)
+    }
+  }

🤖 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/compute-view/element-view/compute.ts` around lines 209 -
216, The loop over relSpecs uses Object.keys and a type-cast to access values;
replace it by iterating relSpecs directly (e.g. Object.entries(relSpecs) or
for...in) so you can get the spec value without relSpecs[kind as keyof typeof
relSpecs] as any, keep the existing style = spec?.style ?? spec logic and push
kind into multipleKinds when style?.multiple is truthy, and make relSpecs itself
defensive by defaulting model.specification.relationships ?? {} (or similar)
before the loop to avoid crashes when a model has no specification block; update
references to relSpecs, multipleKinds, and the loop in compute.ts accordingly.

packages/core/src/compute-view/element-view/multiple-relationships.spec.ts (2)

316-333: ⚡ Quick win

Add coverage for the reverse ordering of multiple: true / multiple: false.

The test asserts false overrides a preceding true, but doesn't test the reverse — include('a -> b', { with: { multiple: false } }) followed by include('a -> b', { with: { multiple: true } }). Given the implementation in compute.ts always lets false win regardless of position, that ordering would currently fail "last rule wins" semantics. Adding the reverse case will pin down the intended precedence and prevent regressions.

🤖 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/compute-view/element-view/multiple-relationships.spec.ts`
around lines 316 - 333, Add a test that covers the reverse ordering of include
rules so we verify precedence between multiple: false and multiple: true;
specifically add a new spec mirroring the existing one but with
TestHelper.$rules called in reverse order (first t.$include('a -> b', { with: {
multiple: false } }) then t.$include('a -> b', { with: { multiple: true } }))
and assert the expected behavior (whether false should still override or last
rule should win) to lock down intended semantics for computeView and the include
handling in compute.ts; reference the existing test in
multiple-relationships.spec.ts and the use of TestHelper.$include/computeView
when adding this new case.

---

110-124: 💤 Low value

Clean up debug-style narrative comments before merge.

Lines 111-119 read like a working scratchpad ("Wait - there's 1 sync relation…", "Actually: relations = [alpha, beta, gamma]"). Once the test is correct, the rationale can be a single short comment or removed entirely so future readers aren't confused about whether the reasoning is still authoritative.

🤖 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/compute-view/element-view/multiple-relationships.spec.ts`
around lines 110 - 124, Remove the debug-style running-narrative comments
between the assertions around expect(view.edges).toHaveLength(3) (the block that
starts with "2 expanded async edges..." and the "Actually: relations = [alpha,
beta, gamma]" lines); replace them with a single concise comment (or remove
entirely) that states the intent (e.g. "two expanded async edges and one merged
sync edge expected") so the test around view.edges, expanded and merged remains
clear; locate this in the multiple-relationships.spec.ts test that asserts
expect(expanded).toHaveLength(2) and expect(merged).toHaveLength(1) and update
the comments there.

packages/core/src/compute-view/deployment-view/compute.ts (2)

74-93: 🏗️ Heavy lift

resolveEndpoint does structural inspection on any — fragile and untyped.

The endpoint shape detection ('tags' in ep && 'kind' in ep && 'metadata' in ep, then a fallback that reaches into ep.instance/ep.element) is essentially a runtime type guess. If the deployment model adds another endpoint variant (e.g. a future "service" wrapper that has tags but a different kind semantics), the predicate will silently produce wrong results.

Recommendation: define an explicit EndpointForPredicate type and a typed resolveEndpoint(ep: DeploymentEndpoint): EndpointForPredicate function in deployment-view/utils.ts, exhaustively switching on the endpoint variants (using nonexhaustive like the surrounding code). This also removes the four as any casts on the call to pred(...).

🤖 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/compute-view/deployment-view/compute.ts` around lines 74 -
93, The current resolveEndpoint implementation in compute.ts does fragile
runtime shape checks on any; replace this by defining a concrete
EndpointForPredicate type and a typed resolveEndpoint(ep: DeploymentEndpoint):
EndpointForPredicate in deployment-view/utils.ts, implement resolveEndpoint with
an exhaustive switch over known endpoint variants (use the project nonexhaustive
pattern for future-proofing, and handle NestedElementOfDeployedInstanceModel and
other wrappers explicitly), export and import that typed function into
compute.ts to remove the ad-hoc casts and the four "as any" uses when calling
pred(...) so pred receives a correctly-typed EndpointForPredicate instead of
any.

---

24-29: ⚖️ Poor tradeoff

as any bridge between FqnExpr and ModelFqnExpr hides a real type contract gap.

The comment acknowledges the issue ("the as any bridge is needed because…"). A safer alternative is to make createRelationExpressionToPredicates generic over the FQN-expression flavor (or accept a narrowed Predicate<{ id: string; … }>), so deployment and element pipelines instantiate it with their own typed predicate factory. The current "runtime-safe" claim relies on the two builder shapes staying convergent, with no compiler check.

As per coding guidelines: "Avoid using any, casts with as."

🤖 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/compute-view/deployment-view/compute.ts` around lines 24 -
29, The current use of "as any" to pass deploymentExpressionToPredicate into
createRelationExpressionToPredicates hides a real type mismatch between FqnExpr
and ModelFqnExpr; change createRelationExpressionToPredicates to be generic over
the expression/builder flavor (e.g. add a type parameter like <Expr> or <Expr
extends FqnExpr> or accept a builder type returning Predicate<{id: string}>),
update its signature so it accepts the exact factory shape used by
deploymentExpressionToPredicate (not ModelFqnExpr-locked types), then remove the
cast and instantiate createRelationExpressionToPredicates with the proper type
so deploymentRelationExprToPredicate is typed safely; update any callers of
createRelationExpressionToPredicates to supply the matching type parameter or
builder shape (referenced symbols: createRelationExpressionToPredicates,
deploymentExpressionToPredicate, ModelFqnExpr, FqnExpr,
deploymentRelationExprToPredicate).

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

packages/core/src/compute-view/element-view/multiple-relationships.spec.ts (1)

82-124: ⚡ Quick win

Strengthen the “not other kinds” assertion with at least two non-matching relations.

This case currently has only one sync relation, so it passes whether sync is merged or (incorrectly) expanded. Add a second sync relation and assert a single merged sync edge (label: '[...]', relations.length === 2) to truly validate non-matching behavior.

Suggested test adjustment

       const t = TestHelper.from(builder.model(({ rel }, m) =>
         m(
           rel('a', 'b', { title: 'alpha', kind: 'async' }),
           rel('a', 'b', { title: 'beta', kind: 'async' }),
           rel('a', 'b', { title: 'gamma', kind: 'sync' }),
+          rel('a', 'b', { title: 'delta', kind: 'sync' }),
         )
       ))
@@
-      expect(view.edges).toHaveLength(3)
-      const expanded = view.edges.filter(e => e.label !== 'gamma')
-      const merged = view.edges.filter(e => e.label === 'gamma')
+      expect(view.edges).toHaveLength(3)
+      const expanded = view.edges.filter(e => ['alpha', 'beta'].includes(e.label ?? ''))
+      const merged = view.edges.filter(e => e.label === '[...]')
       expect(expanded).toHaveLength(2)
       expect(merged).toHaveLength(1)
+      expect(merged[0]!.relations).toHaveLength(2)

As per coding guidelines: "Aim to cover new features with relevant tests; keep test names descriptive".

🤖 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/compute-view/element-view/multiple-relationships.spec.ts`
around lines 82 - 124, Add a second non-matching "sync" relation to the
TestHelper.from model so the test actually verifies that non-matching kinds are
merged rather than expanded; update the expectations after computing the view
(using computeView and inspecting view.edges) to assert there is still a single
merged sync edge (filter by the merged edge label, e.g. label === '[...]' or the
expected merged label) and that this merged edge's relations.length === 2, while
the two async relations remain expanded (expect expanded.length === 2 and
merged.length === 1).

packages/core/src/compute-view/deployment-view/utils.ts (1)

204-231: ⚡ Quick win

Cognitive complexity exceeds project threshold; consider extracting helpers.

SonarCloud flags this function at complexity 45 (limit 30). The nested loop combined with the chained guarded assignments for head/tail/color/line is the main contributor. Splitting the “find opposite edge” and “merge properties” concerns into small helpers would bring complexity down without behavior changes.

♻️ Sketch of the refactor

-function mergeBidirectionalEdges<A extends AnyAux>(edges: ComputedEdge<A>[]): ComputedEdge<A>[] {
-  const toRemove = new Set<number>()
-  for (let i = 0; i < edges.length; i++) {
-    if (toRemove.has(i)) continue
-    const edge = edges[i]!
-    for (let j = i + 1; j < edges.length; j++) {
-      if (toRemove.has(j)) continue
-      const other = edges[j]!
-      if (edge.source === other.target && edge.target === other.source && edge.label === other.label) {
-        edge.dir = 'both'
-        const head = edge.head ?? other.head
-        if (head) {
-          if (!edge.head) edge.head = head
-          if (!edge.tail) edge.tail = head
-        }
-        if (other.color) {
-          if (!edge.color) edge.color = other.color
-        }
-        if (other.line) {
-          if (!edge.line) edge.line = other.line
-        }
-        toRemove.add(j)
-        break
-      }
-    }
-  }
-  return toRemove.size > 0 ? edges.filter((_, i) => !toRemove.has(i)) : edges
-}
+const isOpposite = <A extends AnyAux>(a: ComputedEdge<A>, b: ComputedEdge<A>): boolean =>
+  a.source === b.target && a.target === b.source && a.label === b.label
+
+function mergeIntoBidirectional<A extends AnyAux>(edge: ComputedEdge<A>, other: ComputedEdge<A>): void {
+  edge.dir = 'both'
+  const head = edge.head ?? other.head
+  if (head) {
+    edge.head ??= head
+    edge.tail ??= head
+  }
+  edge.color ??= other.color
+  edge.line ??= other.line
+}
+
+function mergeBidirectionalEdges<A extends AnyAux>(edges: ComputedEdge<A>[]): ComputedEdge<A>[] {
+  const toRemove = new Set<number>()
+  for (let i = 0; i < edges.length; i++) {
+    if (toRemove.has(i)) continue
+    const j = edges.findIndex((other, k) => k > i && !toRemove.has(k) && isOpposite(edges[i]!, other))
+    if (j < 0) continue
+    mergeIntoBidirectional(edges[i]!, edges[j]!)
+    toRemove.add(j)
+  }
+  return toRemove.size > 0 ? edges.filter((_, i) => !toRemove.has(i)) : edges
+}

🤖 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/compute-view/deployment-view/utils.ts` around lines 204 -
231, Refactor mergeBidirectionalEdges to reduce cognitive complexity by
extracting the inner lookup and property-merge logic into two small helpers:
create a function findOppositeEdge(edges, startIndex) that returns the index of
the matching opposite edge (checks source/target/label and skips indices in
toRemove), and create mergeEdgeProperties(targetEdge, sourceEdge) that sets dir
= 'both' and consolidates head/tail/color/line using the existing precedence
rules (use head from either edge, assign head to tail if needed, and only set
color/line if missing). Replace the nested loop body in mergeBidirectionalEdges
with a call to findOppositeEdge and, when it returns an index, call
mergeEdgeProperties(edges[i], edges[j]) and mark j in toRemove; preserve use of
toRemove and the final filter behavior.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

packages/core/src/compute-view/deployment-view/utils.ts (1)

204-231: ⚡ Quick win

mergeBidirectionalEdges mutates its inputs and trips SonarCloud complexity.

Two concerns to address together:

1. The function mutates the edge objects it receives (edge.dir, edge.head, edge.tail, edge.color, edge.line). The signature suggests a pure transform; in-place mutation is fine while the caller is the only owner, but it leaks a side effect through the public-ish API surface and makes future reuse risky.
2. SonarCloud reports cognitive complexity of 45 here (limit 30). The nested for/for plus the in-loop merge bookkeeping is the main contributor.

Extracting an applyBidirectionalMerge(edge, other) helper that returns a fresh merged edge (and dropping the toRemove Set in favor of a simple result list keyed by source|target|label) addresses both at once.

🤖 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/compute-view/deployment-view/utils.ts` around lines 204 -
231, mergeBidirectionalEdges currently mutates input edge objects and is overly
complex; refactor it to produce new edge objects and simplify control flow by
extracting a helper applyBidirectionalMerge(edge, other) that returns a fresh
merged ComputedEdge (combining dir='both', head/tail/color/line with preference
rules) instead of modifying either input, and replace the nested for/Set removal
pattern with a single-pass accumulation keyed by a unique key (e.g.,
`${source}|${target}|${label}` or its normalized bidirectional equivalent) that
either inserts the original edge or replaces it with the result of
applyBidirectionalMerge when a matching reverse edge is found; ensure the final
function returns a new array of edges without mutating any input objects and
that applyBidirectionalMerge encapsulates the merge precedence logic for
head/tail/color/line.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

packages/core/src/compute-view/deployment-view/utils.ts (3)

268-280: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

as ComputedEdge<A> cast still present (applies again at 295-307).

The default branch at lines 320-331 already builds the edge via exact({...}) without an explicit assertion. The two new push sites continue to use as ComputedEdge<A>, conflicting with both the existing local pattern and the project guideline to avoid as casts. Per coding guidelines: "Avoid using any, casts with as".

🤖 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/compute-view/deployment-view/utils.ts` around lines 268 -
280, The pushes that create edges use an explicit cast "as ComputedEdge<A>" (see
the expandedEdges.push that builds id via hashEdgeId and the other new push
site), which violates the guideline to avoid "as" casts; replace those casts by
constructing the edge through the project's type-safe helper (the same pattern
used by the existing default branch) — call exact({...}) with the same
properties (id, parent, source, target, label, relations, color, line, head,
...props) instead of using "as ComputedEdge<A>", so TypeScript will
infer/validate the correct ComputedEdge<A> type without an "as" assertion.

---

213-224: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Bidirectional merge still flattens asymmetric arrows and ignores other.tail.

The previously raised concern still applies to the current code: head = edge.head ?? other.head is then assigned to both edge.head and edge.tail, which copies one side over both ends. other.tail is never consulted, and side-specific color/line from other are also lost when edge already has values set.

🤖 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/compute-view/deployment-view/utils.ts` around lines 213 -
224, The merge currently sets head = edge.head ?? other.head and then assigns
that value to both edge.head and edge.tail, which flattens asymmetric arrows and
ignores other.tail; change the logic to set each side independently: set
edge.head = edge.head ?? other.head and edge.tail = edge.tail ?? other.tail (do
not assign one computed variable to both ends), and when merging styling only
assign edge.color or edge.line from other if the corresponding side is missing
(i.e., only set edge.color/edge.line when undefined, and if your model supports
per-side styling prefer other.head/other.tail variants), so that edge.head,
edge.tail, edge.color and edge.line are merged side-by-side rather than
overwritten with a single value.

---

282-293: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

only(filter(...)) for mergedSource still inconsistent with the default branch.

Same concern as before: this filter compares relation source.id/target.id against deployment node IDs, so model-level relations in merged never match. Even when types align, only() returns undefined for >1 matches, silently dropping the primary. Meanwhile the default branch (line 318) calls mergePropsFromRelationships with no primary, leaving the two paths inconsistent. Either drop the mergedSource argument here for symmetry with the default branch, or filter against the relation's own endpoints rather than deployment node IDs.

🤖 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/compute-view/deployment-view/utils.ts` around lines 282 -
293, The mergedSource lookup is using only(filter(...)) which mismatches the
default branch and can return undefined for >1 matches; update the call site so
both branches are consistent by removing the primary relationship argument
(mergedSource) from the mergePropsFromRelationships invocation in this block
(i.e., call mergePropsFromRelationships(merged.map(r => r.$relationship)) ), or
alternatively change the filter logic to compare against the relationship
endpoints instead of deployment node IDs if you intend to keep a primary;
reference merged, mergedSource, filter, only, and mergePropsFromRelationships to
locate and apply the change.

packages/core/src/compute-view/deployment-view/utils.ts (1)

233-337: ⚡ Quick win

Consider extracting helpers to reduce toComputedEdges complexity.

SonarCloud flags cognitive complexity of 45 (allowed 30) in this region. Extracting the per-branch edge construction (e.g., buildExpandedEdge(rel, conn, defaults), buildMergedEdge(merged, conn, defaults), buildSingleEdge(relations, conn, defaults)) would also let you delete the duplicated destructuring/mergePropsFromRelationships/exact({...}) blocks shared between the three push sites and naturally resolve the duplicate as ComputedEdge<A> casts noted above.

🤖 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/compute-view/deployment-view/utils.ts` around lines 233 -
337, toComputedEdges is too complex and duplicates logic for building edges;
extract three helper functions (e.g., buildExpandedEdge(rel, conn, defaults),
buildMergedEdge(merged, conn, defaults) and buildSingleEdge(relations, conn,
defaults)) that encapsulate the destructuring/mergePropsFromRelationships calls
and construction of the ComputedEdge (including id via hashEdgeId or conn.id,
parent, source, target, label, relations, color, line, head and remaining props)
so the main loop only decides which helper to call and pushes its result(s);
ensure helpers return properly typed ComputedEdge<A> (avoiding repeated `as
ComputedEdge<A>`), reuse only(...) and mergeBidirectionalEdges as before, and
keep mergePropsFromRelationships usage inside the helpers to remove duplication
and reduce cognitive complexity.