feat: introduce topology view#3186
Open
lhein wants to merge 1 commit into
Open
Conversation
Contributor
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (34)
📝 WalkthroughWalkthroughThis PR adds a topology visualization feature and integrates it into the UI: new topology page, routing and navigation entry, controller factories, layout/control hooks, model building (including synthetic external/dynamic endpoint nodes), custom edge rendering with rounded orthogonal bendpoints, multiple topology components, styles, and comprehensive tests. Also adds ChangesTopology Visualization Feature
Sequence DiagramsequenceDiagram
participant User as User
participant Nav as Navigation
participant Router as Router
participant Page as TopologyPage
participant Hooks as Model/Layout/Apply Hooks
participant Controller as Topology Controller
participant PF as PatternFly Visualization
participant Canvas as SVG Canvas
User->>Nav: Click "Topology"
Nav->>Router: navigate to /topology
Router->>Page: load TopologyPage (lazy)
Page->>Hooks: useTopologyModel(vizNodes, entities, layout)
Hooks->>Hooks: extract endpoints, synthesize nodes/edges
Hooks-->>Page: model + topLevelGroupIds
Page->>Controller: controller.fromModel(model)
Controller->>PF: register factories + apply model
Page->>Hooks: useApplyTopologyModel(controller, model, topLevelGroupIds)
Hooks->>Controller: collapse groups, layout, fit()
Page->>Hooks: useTopologyControlButtons(controller)
Hooks-->>Page: control buttons (zoom/fit/reset/layout)
PF->>Canvas: render nodes, edges, connectors
User->>Canvas: double-click / open context menu
Canvas->>Page: dispatch visible-flow changes
Page->>Canvas: update model / re-render
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
lhein
force-pushed
the
topoView
branch
2 times, most recently
from
979321e to
c1eaa54
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3186 +/- ##
==========================================
+ Coverage 92.04% 92.12% +0.08%
==========================================
Files 619 635 +16
Lines 23740 24154 +414
Branches 5612 5495 -117
==========================================
+ Hits 21851 22253 +402
- Misses 1780 1899 +119
+ Partials 109 2 -107 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
packages/ui/src/pages/Topology/topology-controller.service.ts (1)
55-73: 💤 Low valueType-before-kind precedence in
componentFactorycould silently swallow a misconfigured node.
EXTERNAL_ENDPOINT_NODE_TYPE/DYNAMIC_ENDPOINT_NODE_TYPEchecks run before thekindswitch, so any element with those type strings—regardless of itsModelKind—resolves to the endpoint renderers. This is intentional and matches the tests, but worth a short inline comment so future contributors don't reorder the checks.📝 Suggested clarifying comment
static componentFactory(kind: ModelKind, type: string): ReturnType<ComponentFactory> { + // Type-based routing takes precedence over kind-based routing. + // Endpoint types are always resolved to their specific components regardless of kind. if (type === EXTERNAL_ENDPOINT_NODE_TYPE) {🤖 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/ui/src/pages/Topology/topology-controller.service.ts` around lines 55 - 73, The precedence of type checks before the kind switch in componentFactory is intentional to let nodes with type EXTERNAL_ENDPOINT_NODE_TYPE or DYNAMIC_ENDPOINT_NODE_TYPE resolve to TopologyExternalEndpoint/TopologyDynamicEndpoint regardless of their ModelKind; add a short inline comment above the two if blocks in componentFactory documenting this intentional "type-before-kind" precedence (mention EXTERNAL_ENDPOINT_NODE_TYPE, DYNAMIC_ENDPOINT_NODE_TYPE, and that Graph/Edge handling is in the ModelKind switch) so future contributors won't reorder the checks thinking it's a bug.packages/ui/src/pages/Topology/components/TopologyCollapsedGroup.tsx (1)
50-51: ⚡ Quick win
vizNode.datacast toCamelRouteVisualEntityDatasilently bypasses TypeScript's strict-mode guarantees.In practice this is safe because
TopologyCollapsedGroupis only wired up forgroup-kind nodes (which are always Camel routes), but the explicitasassertion means any future routing of a different node kind here would silently produceundefinedforprocessorNamewithout a compile-time error.If
vizNode.dataalready carries a typed discriminant (e.g.processorNameon its base type or a union), prefer a type-narrowing check over the cast. Alternatively, add a runtime guard:🛡️ Proposed defensive check
-const processorName = (vizNode.data as CamelRouteVisualEntityData).processorName; +const processorName = + 'processorName' in vizNode.data ? (vizNode.data as CamelRouteVisualEntityData).processorName : undefined;🤖 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/ui/src/pages/Topology/components/TopologyCollapsedGroup.tsx` around lines 50 - 51, Replace the unsafe cast of vizNode.data to CamelRouteVisualEntityData in TopologyCollapsedGroup: instead of using "vizNode.data as CamelRouteVisualEntityData", add a runtime/type-narrowing guard that verifies vizNode.data has the expected discriminant (e.g., check typeof vizNode.data === 'object' && 'processorName' in vizNode.data) before reading processorName, and fall back to a safe default (or early-return) if the shape doesn't match; then call getProcessorIcon(processorName) only when processorName is confirmed to exist to avoid silently producing undefined.packages/ui/src/pages/Topology/TopologyPage.test.tsx (1)
76-83: ⚡ Quick winAssertion pattern yields a poor failure message.
expect(nodes && nodes.length > 0).toBe(true)printsExpected: true, Received: falseon failure with no diagnostic info. Prefer:🔍 Clearer assertion
-expect(nodes && nodes.length > 0).toBe(true); +expect(nodes?.length).toBeGreaterThan(0);Same change applies at lines 117–118 in the "renders every route even when hidden" test.
🤖 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/ui/src/pages/Topology/TopologyPage.test.tsx` around lines 76 - 83, The assertion `expect(nodes && nodes.length > 0).toBe(true)` yields an unhelpful failure; replace it with a clearer assertion that shows the failing scope and count—inside the same loop (where visualEntities and entity.getId() are used and nodes is found via result?.container.querySelectorAll(`[data-id^="${scope}|"]`)) assert e.g. that nodes exists and nodes.length is > 0 by throwing or using a Jest matcher that reports the value (for example: assert nodes?.length > 0 with a thrown Error containing the scope when zero, or use expect(nodes?.length).toBeGreaterThan(0)), and apply the same change to the second test ("renders every route even when hidden" at lines ~117–118).packages/ui/src/pages/Topology/topology-controller.service.test.ts (1)
65-68: ⚡ Quick winThe
graphkind factory test only verifies the component is defined, not which component it is.For consistency with the other factory assertions that use
toBe, tightening this totoBe(GraphComponent)would catch an accidental routing change:🔍 Stricter assertion
+import { DagreGroupsLayout, GraphComponent, ModelKind, Visualization } from '@patternfly/react-topology'; ... it('returns the default GraphComponent for the graph kind', () => { const Component = TopologyControllerService.componentFactory(ModelKind.graph, 'graph'); - expect(Component).toBeDefined(); + expect(Component).toBe(GraphComponent); });🤖 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/ui/src/pages/Topology/topology-controller.service.test.ts` around lines 65 - 68, The test using TopologyControllerService.componentFactory(ModelKind.graph, 'graph') currently only checks the result is defined; change the assertion to assert the exact component by replacing the loose expect(Component).toBeDefined() with a strict equality check expect(Component).toBe(GraphComponent) so the test verifies the factory returns GraphComponent for ModelKind.graph; update the test case in topology-controller.service.test.ts to import or reference GraphComponent and use toBe for exact matching.
🤖 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 `@packages/ui/src/pages/Topology/components/TopologySyntheticEndpoint.tsx`:
- Line 43: The data-testid construction in TopologySyntheticEndpoint.tsx uses
the user-facing prop label (`data-testid={`${testIdPrefix}__${label}`}`) which
can be unstable and non-unique; change it to use a deterministic identifier
(e.g., a model or endpoint id) or a sanitized slug instead. Locate the JSX in
the TopologySyntheticEndpoint component where `data-testid` is set and replace
`${label}` with a stable prop such as `id` or `modelId` (e.g.,
`${testIdPrefix}__${id}`), or if only label is available, apply a deterministic
sanitizer (slugify/safeId) to produce a stable token; ensure you reference the
same prop name (`testIdPrefix`, `label`, or `id`) used elsewhere in the
component and update any prop types if adding `id`.
In `@packages/ui/src/pages/Topology/OrthogonalBendpointsEdge.ts`:
- Line 28: Guard accesses to the graph before calling getLayout/getBendpoints:
replace the direct call this.getGraph().getLayout() in OrthogonalBendpointsEdge
(the isHorizontal calculation) with a null-safe check (e.g., const graph =
this.getGraph(); const isHorizontal = graph ? graph.getLayout() ===
LayoutType.DagreHorizontal : false) and similarly guard any
this.getGraph().getBendpoints() usages so they use the graph variable or
optional chaining to avoid calling methods on undefined during lifecycle
attachment.
In `@packages/ui/src/pages/Topology/topology-connections.test.ts`:
- Around line 73-94: The test title mentions "direct-vm" but the entities only
include seda and vm endpoints; update the test in topology-connections.test.ts
by adding a direct-vm producer/consumer pair to the entities (e.g., another
mockEntity with route.from.uri 'timer:t' and a step to { to: { uri:
'direct-vm:z' } } plus a corresponding consumer entity with route.from.uri
'direct-vm:z'), add matching entries to the map (e.g., ['e','E|route'],
['f','F|route'] or appropriate ids), and update the assertions that check edges
and their labels from buildRouteConnectionExtras to expect the additional
'direct-vm:z' label and adjust edges length accordingly; alternatively, if you
prefer not to expand the fixtures, simply rename the it(...) title to remove
"and direct-vm" so it accurately reflects only seda and vm coverage.
In `@packages/ui/src/pages/Topology/topology-layout-preference.hook.ts`:
- Around line 25-30: The code is trusting localStorage value for
LocalStorageKeys.CanvasLayout when computing activeLayout, which can yield
invalid layout strings; update the logic that computes activeLayout (the useMemo
using settingsLayout, LocalStorageKeys.CanvasLayout, and LayoutType) to read the
stored value, validate it against the allowed LayoutType values (e.g.,
Object.values(LayoutType).includes(storedValue) or a small isValidLayout(layout:
string) helper), and only use it if valid—otherwise fall back to settingsLayout
or LayoutType.DagreHorizontal.
---
Nitpick comments:
In `@packages/ui/src/pages/Topology/components/TopologyCollapsedGroup.tsx`:
- Around line 50-51: Replace the unsafe cast of vizNode.data to
CamelRouteVisualEntityData in TopologyCollapsedGroup: instead of using
"vizNode.data as CamelRouteVisualEntityData", add a runtime/type-narrowing guard
that verifies vizNode.data has the expected discriminant (e.g., check typeof
vizNode.data === 'object' && 'processorName' in vizNode.data) before reading
processorName, and fall back to a safe default (or early-return) if the shape
doesn't match; then call getProcessorIcon(processorName) only when processorName
is confirmed to exist to avoid silently producing undefined.
In `@packages/ui/src/pages/Topology/topology-controller.service.test.ts`:
- Around line 65-68: The test using
TopologyControllerService.componentFactory(ModelKind.graph, 'graph') currently
only checks the result is defined; change the assertion to assert the exact
component by replacing the loose expect(Component).toBeDefined() with a strict
equality check expect(Component).toBe(GraphComponent) so the test verifies the
factory returns GraphComponent for ModelKind.graph; update the test case in
topology-controller.service.test.ts to import or reference GraphComponent and
use toBe for exact matching.
In `@packages/ui/src/pages/Topology/topology-controller.service.ts`:
- Around line 55-73: The precedence of type checks before the kind switch in
componentFactory is intentional to let nodes with type
EXTERNAL_ENDPOINT_NODE_TYPE or DYNAMIC_ENDPOINT_NODE_TYPE resolve to
TopologyExternalEndpoint/TopologyDynamicEndpoint regardless of their ModelKind;
add a short inline comment above the two if blocks in componentFactory
documenting this intentional "type-before-kind" precedence (mention
EXTERNAL_ENDPOINT_NODE_TYPE, DYNAMIC_ENDPOINT_NODE_TYPE, and that Graph/Edge
handling is in the ModelKind switch) so future contributors won't reorder the
checks thinking it's a bug.
In `@packages/ui/src/pages/Topology/TopologyPage.test.tsx`:
- Around line 76-83: The assertion `expect(nodes && nodes.length >
0).toBe(true)` yields an unhelpful failure; replace it with a clearer assertion
that shows the failing scope and count—inside the same loop (where
visualEntities and entity.getId() are used and nodes is found via
result?.container.querySelectorAll(`[data-id^="${scope}|"]`)) assert e.g. that
nodes exists and nodes.length is > 0 by throwing or using a Jest matcher that
reports the value (for example: assert nodes?.length > 0 with a thrown Error
containing the scope when zero, or use
expect(nodes?.length).toBeGreaterThan(0)), and apply the same change to the
second test ("renders every route even when hidden" at lines ~117–118).
🪄 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: 2748b2af-4253-4aff-a774-76f3210cce21
⛔ Files ignored due to path filters (1)
packages/ui/src/layout/__snapshots__/Navigation.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (34)
.gitignorepackages/ui/src/layout/Navigation.tsxpackages/ui/src/pages/Topology/OrthogonalBendpointsEdge.test.tspackages/ui/src/pages/Topology/OrthogonalBendpointsEdge.tspackages/ui/src/pages/Topology/TopologyPage.scsspackages/ui/src/pages/Topology/TopologyPage.test.tsxpackages/ui/src/pages/Topology/TopologyPage.tsxpackages/ui/src/pages/Topology/components/TopologyCollapsedGroup.test.tsxpackages/ui/src/pages/Topology/components/TopologyCollapsedGroup.tsxpackages/ui/src/pages/Topology/components/TopologyDynamicEndpoint.tsxpackages/ui/src/pages/Topology/components/TopologyEdge.test.tsxpackages/ui/src/pages/Topology/components/TopologyEdge.tsxpackages/ui/src/pages/Topology/components/TopologyExternalEndpoint.tsxpackages/ui/src/pages/Topology/components/TopologyNodeLabel.test.tsxpackages/ui/src/pages/Topology/components/TopologyNodeLabel.tsxpackages/ui/src/pages/Topology/components/TopologySyntheticEndpoint.test.tsxpackages/ui/src/pages/Topology/components/TopologySyntheticEndpoint.tsxpackages/ui/src/pages/Topology/index.tspackages/ui/src/pages/Topology/rounded-path.test.tspackages/ui/src/pages/Topology/rounded-path.tspackages/ui/src/pages/Topology/router-exports.tsxpackages/ui/src/pages/Topology/topology-apply-model.hook.tspackages/ui/src/pages/Topology/topology-connections.test.tspackages/ui/src/pages/Topology/topology-connections.tspackages/ui/src/pages/Topology/topology-context-menu.test.tsxpackages/ui/src/pages/Topology/topology-context-menu.tsxpackages/ui/src/pages/Topology/topology-control-buttons.hook.test.tsxpackages/ui/src/pages/Topology/topology-control-buttons.hook.tsxpackages/ui/src/pages/Topology/topology-controller.service.test.tspackages/ui/src/pages/Topology/topology-controller.service.tspackages/ui/src/pages/Topology/topology-layout-preference.hook.tspackages/ui/src/pages/Topology/topology-model.hook.tspackages/ui/src/router.tsxpackages/ui/src/router/links.models.ts
| const height = bounds?.height ?? CanvasDefaults.DEFAULT_NODE_HEIGHT; | ||
|
|
||
| return ( | ||
| <g className={`custom-node ${className}`} data-testid={`${testIdPrefix}__${label}`} data-label={label}> |
Contributor
There was a problem hiding this comment.
Make data-testid independent from user-facing labels.
data-testid={${testIdPrefix}__${label}} can become unstable (spaces/special chars) and non-unique when labels repeat, which makes tests flaky. Use a sanitized or deterministic id (e.g., model id) instead.
Suggested adjustment
- <g className={`custom-node ${className}`} data-testid={`${testIdPrefix}__${label}`} data-label={label}>
+ <g
+ className={`custom-node ${className}`}
+ data-testid={`${testIdPrefix}__${element.getId?.() ?? label.replace(/[^a-zA-Z0-9_-]/g, '_')}`}
+ data-label={label}
+ >📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <g className={`custom-node ${className}`} data-testid={`${testIdPrefix}__${label}`} data-label={label}> | |
| <g | |
| className={`custom-node ${className}`} | |
| data-testid={`${testIdPrefix}__${element.getId?.() ?? label.replace(/[^a-zA-Z0-9_-]/g, '_')}`} | |
| data-label={label} | |
| > |
🤖 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/ui/src/pages/Topology/components/TopologySyntheticEndpoint.tsx` at
line 43, The data-testid construction in TopologySyntheticEndpoint.tsx uses the
user-facing prop label (`data-testid={`${testIdPrefix}__${label}`}`) which can
be unstable and non-unique; change it to use a deterministic identifier (e.g., a
model or endpoint id) or a sanitized slug instead. Locate the JSX in the
TopologySyntheticEndpoint component where `data-testid` is set and replace
`${label}` with a stable prop such as `id` or `modelId` (e.g.,
`${testIdPrefix}__${id}`), or if only label is available, apply a deterministic
sanitizer (slugify/safeId) to produce a stable token; ensure you reference the
same prop name (`testIdPrefix`, `label`, or `id`) used elsewhere in the
component and update any prop types if adding `id`.
e311459 to
edb2d30
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
packages/ui/src/pages/Topology/topology-context-menu.test.tsx (2)
54-55: ⚡ Quick winAssert dispatch call order explicitly.
hideFlowsmust happen beforeshowFlows; using nth-call assertions makes this contract explicit and prevents false positives.Stronger expectation style
- expect(dispatch).toHaveBeenCalledWith({ type: 'hideFlows', flowIds: undefined }); - expect(dispatch).toHaveBeenCalledWith({ type: 'showFlows', flowIds: ['route-1234'] }); + expect(dispatch).toHaveBeenNthCalledWith(1, { type: 'hideFlows', flowIds: undefined }); + expect(dispatch).toHaveBeenNthCalledWith(2, { type: 'showFlows', flowIds: ['route-1234'] });🤖 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/ui/src/pages/Topology/topology-context-menu.test.tsx` around lines 54 - 55, The test currently checks for both dispatch calls but not their order; update the assertions in topology-context-menu.test.tsx to explicitly assert call order by using nth-call assertions (e.g., toHaveBeenNthCalledWith) against dispatch: assert that the first call is { type: 'hideFlows', flowIds: undefined } and the second call is { type: 'showFlows', flowIds: ['route-1234'] }, ensuring the hideFlows action occurs before showFlows.
15-18: ⚡ Quick winAvoid
as neverin test doubles; it hides type-contract regressions.This cast bypasses strict typing and can let invalid element mocks compile silently. Prefer a narrowly typed mock helper instead.
As per coding guidelines "Ensure TypeScript strict mode is enabled for all TypeScript code".Proposed typed mock helper
-const mockElement = (vizNode: Partial<IVisualizationNode> | undefined) => - ({ - getData: () => (vizNode ? { vizNode } : {}), - }) as never; +type TopologyElementLike = { + getData: () => { vizNode?: Partial<IVisualizationNode> }; +}; + +const mockElement = (vizNode: Partial<IVisualizationNode> | undefined): TopologyElementLike => ({ + getData: () => (vizNode ? { vizNode } : {}), +});🤖 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/ui/src/pages/Topology/topology-context-menu.test.tsx` around lines 15 - 18, The test helper mockElement currently uses an unsafe cast ("as never") which hides type-contract regressions; replace it with a narrowly typed mock that matches the real element shape used by the code under test (i.e. an object with getData(): { vizNode?: Partial<IVisualizationNode> }). Remove the "as never" and give mockElement a proper return type (or a small interface/type alias) that accurately reflects the element expected by the topology context menu tests so TypeScript strict mode can catch mismatches in getData and IVisualizationNode usage.packages/ui/src/pages/Topology/topology-layout-preference.hook.ts (1)
8-13: 💤 Low valueConsider exporting
TopologyLayoutPreferencefor downstream typing.The interface is the return type of an exported hook, but is not itself exported. Consumers that want to type a variable holding the hook's result (e.g., destructured in tests or forwarded via props) have no way to reference the type.
♻️ Proposed fix
-interface TopologyLayoutPreference { +export interface TopologyLayoutPreference {Also applies to: 39-40
🤖 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/ui/src/pages/Topology/topology-layout-preference.hook.ts` around lines 8 - 13, Export the TopologyLayoutPreference interface so consumers can type the hook result; update the declaration of TopologyLayoutPreference to be exported (export interface TopologyLayoutPreference) and do the same for the other similar interface referenced around lines 39-40 if present, ensuring the exported names match the return type of the exported hook (e.g., the hook function name) so downstream code and tests can import and use these types.
🤖 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 `@packages/ui/src/pages/Topology/components/TopologyEdge.test.tsx`:
- Around line 69-85: Rename the test from "uses the start point itself as the
arrow anchor when the edge has no bendpoints" to "generates a straight-line path
when the edge has no bendpoints", and change the assertion to first assert the
presence of the SVG path element (querySelector('.custom-edge__body') →
expect(body).not.toBeNull()) before calling getAttribute (remove optional
chaining) so failures show the missing element; optionally add an explicit
assertion that the connector element ('.custom-edge__connector') exists or its
anchor is at the expected end point if you want to cover arrow positioning.
In `@packages/ui/src/pages/Topology/topology-connections.test.ts`:
- Around line 120-135: The test fixtures in topology-connections.test.ts place
steps inside route.from (e.g. route.from.steps) instead of at the route level,
which is inconsistent with the canonical Camel DSL and other tests; update the
affected test cases (including the "detects to inside choice/when branches" test
that uses mockEntity and the other mentioned tests) by moving the steps array
out of route.from into route.steps (so route: { from: { uri: 'timer:t' }, steps:
[...] }) to match the expected structure used by buildRouteConnectionExtras and
the rest of the suite.
In `@packages/ui/src/pages/Topology/topology-layout-preference.hook.ts`:
- Around line 15-17: The runtime validation fails because LayoutType is a const
enum and cannot be referenced under isolatedModules; replace the manual member
list by either (A) changing LayoutType in canvas.models.ts from `const enum
LayoutType` to a regular `enum LayoutType` so
LayoutType.DagreHorizontal/DagreVertical are available at runtime and then use
them in VALID_LAYOUTS in topology-layout-preference.hook.ts, or (B) add and
export a new constant (e.g. `export const VALID_LAYOUT_VALUES =
[LayoutType.DagreHorizontal, LayoutType.DagreVertical]` or a plain string array)
from canvas.models.ts and import that constant into
topology-layout-preference.hook.ts to construct VALID_LAYOUTS; update imports
and references to LayoutType/VALID_LAYOUTS accordingly.
- Around line 27-30: The memo for settingsLayout in
topology-layout-preference.hook.ts uses settingsAdapter as the sole dependency
so it never re-runs when the adapter's internal settings change; update the
dependency to the actual setting value (e.g.
settingsAdapter.getSettings().canvasLayoutDirection) so useMemo(() =>
getInitialLayout(...), [settingsAdapter.getSettings().canvasLayoutDirection]) or
alternatively modify the adapter (or SettingsProvider) to emit change
notifications and depend on that signal; reference symbols: settingsLayout,
useMemo, getInitialLayout, settingsAdapter.getSettings().canvasLayoutDirection,
saveSettings(), SettingsProvider/KaotoEditorFactory.
---
Nitpick comments:
In `@packages/ui/src/pages/Topology/topology-context-menu.test.tsx`:
- Around line 54-55: The test currently checks for both dispatch calls but not
their order; update the assertions in topology-context-menu.test.tsx to
explicitly assert call order by using nth-call assertions (e.g.,
toHaveBeenNthCalledWith) against dispatch: assert that the first call is { type:
'hideFlows', flowIds: undefined } and the second call is { type: 'showFlows',
flowIds: ['route-1234'] }, ensuring the hideFlows action occurs before
showFlows.
- Around line 15-18: The test helper mockElement currently uses an unsafe cast
("as never") which hides type-contract regressions; replace it with a narrowly
typed mock that matches the real element shape used by the code under test (i.e.
an object with getData(): { vizNode?: Partial<IVisualizationNode> }). Remove the
"as never" and give mockElement a proper return type (or a small interface/type
alias) that accurately reflects the element expected by the topology context
menu tests so TypeScript strict mode can catch mismatches in getData and
IVisualizationNode usage.
In `@packages/ui/src/pages/Topology/topology-layout-preference.hook.ts`:
- Around line 8-13: Export the TopologyLayoutPreference interface so consumers
can type the hook result; update the declaration of TopologyLayoutPreference to
be exported (export interface TopologyLayoutPreference) and do the same for the
other similar interface referenced around lines 39-40 if present, ensuring the
exported names match the return type of the exported hook (e.g., the hook
function name) so downstream code and tests can import and use these types.
🪄 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: 1d5d875b-01d4-4475-9612-c30e20a78843
⛔ Files ignored due to path filters (1)
packages/ui/src/layout/__snapshots__/Navigation.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (34)
.gitignorepackages/ui/src/layout/Navigation.tsxpackages/ui/src/pages/Topology/OrthogonalBendpointsEdge.test.tspackages/ui/src/pages/Topology/OrthogonalBendpointsEdge.tspackages/ui/src/pages/Topology/TopologyPage.scsspackages/ui/src/pages/Topology/TopologyPage.test.tsxpackages/ui/src/pages/Topology/TopologyPage.tsxpackages/ui/src/pages/Topology/components/TopologyCollapsedGroup.test.tsxpackages/ui/src/pages/Topology/components/TopologyCollapsedGroup.tsxpackages/ui/src/pages/Topology/components/TopologyDynamicEndpoint.tsxpackages/ui/src/pages/Topology/components/TopologyEdge.test.tsxpackages/ui/src/pages/Topology/components/TopologyEdge.tsxpackages/ui/src/pages/Topology/components/TopologyExternalEndpoint.tsxpackages/ui/src/pages/Topology/components/TopologyNodeLabel.test.tsxpackages/ui/src/pages/Topology/components/TopologyNodeLabel.tsxpackages/ui/src/pages/Topology/components/TopologySyntheticEndpoint.test.tsxpackages/ui/src/pages/Topology/components/TopologySyntheticEndpoint.tsxpackages/ui/src/pages/Topology/index.tspackages/ui/src/pages/Topology/rounded-path.test.tspackages/ui/src/pages/Topology/rounded-path.tspackages/ui/src/pages/Topology/router-exports.tsxpackages/ui/src/pages/Topology/topology-apply-model.hook.tspackages/ui/src/pages/Topology/topology-connections.test.tspackages/ui/src/pages/Topology/topology-connections.tspackages/ui/src/pages/Topology/topology-context-menu.test.tsxpackages/ui/src/pages/Topology/topology-context-menu.tsxpackages/ui/src/pages/Topology/topology-control-buttons.hook.test.tsxpackages/ui/src/pages/Topology/topology-control-buttons.hook.tsxpackages/ui/src/pages/Topology/topology-controller.service.test.tspackages/ui/src/pages/Topology/topology-controller.service.tspackages/ui/src/pages/Topology/topology-layout-preference.hook.tspackages/ui/src/pages/Topology/topology-model.hook.tspackages/ui/src/router.tsxpackages/ui/src/router/links.models.ts
✅ Files skipped from review due to trivial changes (15)
- packages/ui/src/pages/Topology/index.ts
- .gitignore
- packages/ui/src/pages/Topology/components/TopologyExternalEndpoint.tsx
- packages/ui/src/pages/Topology/components/TopologyNodeLabel.test.tsx
- packages/ui/src/pages/Topology/topology-controller.service.test.ts
- packages/ui/src/pages/Topology/router-exports.tsx
- packages/ui/src/router/links.models.ts
- packages/ui/src/pages/Topology/topology-controller.service.ts
- packages/ui/src/pages/Topology/topology-control-buttons.hook.test.tsx
- packages/ui/src/pages/Topology/TopologyPage.scss
- packages/ui/src/pages/Topology/topology-model.hook.ts
- packages/ui/src/pages/Topology/components/TopologyEdge.tsx
- packages/ui/src/pages/Topology/OrthogonalBendpointsEdge.test.ts
- packages/ui/src/pages/Topology/rounded-path.test.ts
- packages/ui/src/pages/Topology/TopologyPage.test.tsx
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/ui/src/layout/Navigation.tsx
- packages/ui/src/pages/Topology/components/TopologyNodeLabel.tsx
- packages/ui/src/pages/Topology/TopologyPage.tsx
- packages/ui/src/pages/Topology/topology-connections.ts
- packages/ui/src/pages/Topology/rounded-path.ts
- packages/ui/src/pages/Topology/components/TopologyCollapsedGroup.tsx
- packages/ui/src/pages/Topology/OrthogonalBendpointsEdge.ts
- packages/ui/src/pages/Topology/components/TopologyCollapsedGroup.test.tsx
- packages/ui/src/pages/Topology/topology-control-buttons.hook.tsx
- packages/ui/src/pages/Topology/components/TopologySyntheticEndpoint.tsx
Comment on lines
+15
to
+17
| // LayoutType is a `const enum`, so its values can't be enumerated at runtime — | ||
| // list them explicitly for the validation check. | ||
| const VALID_LAYOUTS = new Set<string>([LayoutType.DagreHorizontal, LayoutType.DagreVertical]); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether LayoutType is a const enum or regular enum
rg -n "LayoutType" --type=ts -A2 packages/ui/src/components/Visualization/Canvas/canvas.models.ts | head -20
# Check if isolatedModules is set in tsconfig
fd tsconfig.json packages/ui --exec grep -l "isolatedModules" {}
fd tsconfig.json packages/ui --exec grep -n "isolatedModules" {}Repository: KaotoIO/kaoto
Length of output: 226
Replace const enum with regular enum or export validation values separately.
LayoutType is defined as a const enum in canvas.models.ts, but the project has isolatedModules: true in packages/ui/tsconfig.json. This combination prevents cross-module const enum usage at compile time. The manual listing in VALID_LAYOUTS does not resolve the underlying incompatibility—those enum members cannot be accessed at runtime under this configuration.
Fix: Either (1) change LayoutType from const enum to regular enum, or (2) export a separate constant from canvas.models.ts containing the valid layout values and import that instead.
🤖 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/ui/src/pages/Topology/topology-layout-preference.hook.ts` around
lines 15 - 17, The runtime validation fails because LayoutType is a const enum
and cannot be referenced under isolatedModules; replace the manual member list
by either (A) changing LayoutType in canvas.models.ts from `const enum
LayoutType` to a regular `enum LayoutType` so
LayoutType.DagreHorizontal/DagreVertical are available at runtime and then use
them in VALID_LAYOUTS in topology-layout-preference.hook.ts, or (B) add and
export a new constant (e.g. `export const VALID_LAYOUT_VALUES =
[LayoutType.DagreHorizontal, LayoutType.DagreVertical]` or a plain string array)
from canvas.models.ts and import that constant into
topology-layout-preference.hook.ts to construct VALID_LAYOUTS; update imports
and references to LayoutType/VALID_LAYOUTS accordingly.
Signed-off-by: Lars Heinemann <lhein.smx@gmail.com>
|
lordrip
reviewed
May 6, 2026
Member
lordrip
left a comment
lordrip
left a comment
There was a problem hiding this comment.
It looks good overall, from an architecture point of view, I think we're using very similar patterns as in the Design view so we should try to reuse as much as possible. For instance, the bendpoints, rounded paths, the node generation, among others
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.
first draft of the topology view. for the moment we can only show it for a single file.
fixes #769
Summary by CodeRabbit
New Features
Style
Tests
Chore