NO-JIRA: feat: display korrel8r markers in the graph#236
Conversation
Korrel8r can add "markers" to nodes to indicate special conditions. - updated korrel8r/korrel8r-openapi.yaml, includes markers in graph nodes. - updated topology nodes to display markers in graph. - mark node matching current view "selected", follow view changes.
openshift-ci-robot
added
the
jira/valid-reference
|
@alanconway: This pull request explicitly references no jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alanconway The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
WalkthroughThis PR adds marker count visualization and disabled-node state to the topology graph. The OpenAPI schema defines a new ChangesMarker support and disabled state
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
web/src/components/topology/Korrel8rTopology.tsx (1)
204-208:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard context-menu title against missing
node.class.At Line 206/207,
node.class.toString()can throw for parse-failed nodes (whereNode.classis unset anddisabledis set).Suggested fix
- const menu = [ - <ContextMenuItem isDisabled={true} key={node.class.toString()}> - <Title headingLevel="h4">{node.class.toString()}</Title> + const title = node.class?.toString?.() ?? node.id; + const menu = [ + <ContextMenuItem isDisabled={true} key={title}> + <Title headingLevel="h4">{title}</Title> </ContextMenuItem>, ];🤖 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 `@web/src/components/topology/Korrel8rTopology.tsx` around lines 204 - 208, The context menu title uses node.class.toString() which can throw when node.class is undefined for parse-failed nodes; update the block that builds the menu (the code that reads const node: korrel8r.Node = e.getData() and constructs the ContextMenuItem/Title) to safely access node.class (e.g., check node.class !== undefined/null or use a fallback variable like const classLabel = node.class ? node.class.toString() : "Unknown" or an empty string) and pass that safe string into Title so no exception occurs for missing classes.web/src/__tests__/types.spec.ts (1)
154-160:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate bad-class test to the new
disabledfield contract.At Line 158, the expected shape still uses
error, butNodenow records parse failures indisabled.Suggested fix
it('constructor bad class', () => { expect(new Node({ class: 'foobar', count: 1 })).toEqual({ id: 'foobar', count: 1, - error: new TypeError('invalid class: foobar'), + disabled: 'invalid class: foobar', queries: [], + markers: [], }); });🤖 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 `@web/src/__tests__/types.spec.ts` around lines 154 - 160, The test "constructor bad class" expects the old `error` field; update it to the new `disabled` contract by replacing the `error` property with `disabled` containing the parse failure (e.g. new TypeError('invalid class: foobar')), so the expected object returned by new Node({ class: 'foobar', count: 1 }) should include id 'foobar', count 1, disabled: new TypeError('invalid class: foobar'), and queries: [] — update the assertion in the test to reference `disabled` instead of `error`.
🤖 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 `@web/src/components/topology/Korrel8rTopology.tsx`:
- Around line 153-164: The current mapping in nodes (useMemo) mutates the source
graph.nodes by setting node.disabled directly, which can persist incorrect
state; instead, do not modify graph.nodes in-place—create and return new
NodeModel objects (or shallow copies of each korrel8r.Node) and set the disabled
display string on those copies based on loggingAvailable, netobserveAvailable
and the existing node.disabled value (using korrel8r.Node, graph.nodes,
node.disabled, loggingAvailable, netobserveAvailable and t()) so the UI state is
derived immutably and availability changes won’t leak into the original graph
data.
- Line 125: In Korrel8rTopology.tsx inside the JSX that renders the title and
topologyNode (the line containing
<title>{node.disabled}</title>){topologyNode}), remove the stray closing
parenthesis so the fragment flows directly from the closing </title> to
{topologyNode}; locate the JSX in the Korrel8rTopology component where
node.disabled and topologyNode are used and change that sequence to end the
</title> tag and then immediately render {topologyNode}.
In `@web/src/korrel8r/types.ts`:
- Around line 273-274: Node constructor assigns this.markers directly from
node.markers which may be undefined; update the Node constructor to default
markers to an empty array (e.g., set this.markers = node.markers ?? [] or use a
fallback) so this.markers is always an array and preserves the expected shape
for consumers (refer to the Node constructor and the this.markers / node.markers
symbols).
- Line 353: The getter is accessing a Map using bracket notation
(this.nodeByClass?.[id]) which bypasses the Map API and types; replace that
access with the Map#get method (this.nodeByClass?.get(id)) and ensure the
declared return type reflects Node | undefined if it currently asserts Node;
update any corresponding set usage to Map#set if needed so nodeByClass is
consistently accessed via .get/.set (refer to the nodeByClass property and the
function/method containing the return).
---
Outside diff comments:
In `@web/src/__tests__/types.spec.ts`:
- Around line 154-160: The test "constructor bad class" expects the old `error`
field; update it to the new `disabled` contract by replacing the `error`
property with `disabled` containing the parse failure (e.g. new
TypeError('invalid class: foobar')), so the expected object returned by new
Node({ class: 'foobar', count: 1 }) should include id 'foobar', count 1,
disabled: new TypeError('invalid class: foobar'), and queries: [] — update the
assertion in the test to reference `disabled` instead of `error`.
In `@web/src/components/topology/Korrel8rTopology.tsx`:
- Around line 204-208: The context menu title uses node.class.toString() which
can throw when node.class is undefined for parse-failed nodes; update the block
that builds the menu (the code that reads const node: korrel8r.Node =
e.getData() and constructs the ContextMenuItem/Title) to safely access
node.class (e.g., check node.class !== undefined/null or use a fallback variable
like const classLabel = node.class ? node.class.toString() : "Unknown" or an
empty string) and pass that safe string into Title so no exception occurs for
missing classes.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 0ba6c6b1-487d-46c8-8caf-2dd08a8a5b5b
⛔ Files ignored due to path filters (1)
web/src/korrel8r/client/types.gen.tsis excluded by!**/*.gen.*
📒 Files selected for processing (6)
korrel8r/korrel8r-openapi.yamlweb/src/__tests__/types.spec.tsweb/src/components/topology/Korrel8rTopology.tsxweb/src/components/topology/korrel8rtopology.cssweb/src/korrel8r/client/index.tsweb/src/korrel8r/types.ts
| return ( | ||
| <g opacity="0.7" className="tp-plugin__topology_invalid_node"> | ||
| <title>{node.error}</title>){topologyNode} | ||
| <title>{node.disabled}</title>){topologyNode} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Detect the exact malformed JSX token sequence.
rg -n "title>\\{node\\.disabled\\}</title>\\)\\{topologyNode\\}" web/src/components/topology/Korrel8rTopology.tsxRepository: openshift/troubleshooting-panel-console-plugin
Length of output: 148
🏁 Script executed:
# View context around line 125 to understand the structure
cat -n web/src/components/topology/Korrel8rTopology.tsx | sed -n '115,135p'Repository: openshift/troubleshooting-panel-console-plugin
Length of output: 765
🏁 Script executed:
# Search for graph.nodes mutations to verify state mutation concern
rg -n "graph\.nodes\s*=" web/src/components/topology/Korrel8rTopology.tsxRepository: openshift/troubleshooting-panel-console-plugin
Length of output: 72
🏁 Script executed:
# Check node.class usage patterns
rg -n "node\.class" web/src/components/topology/Korrel8rTopology.tsx -A 2 -B 2Repository: openshift/troubleshooting-panel-console-plugin
Length of output: 1027
Remove stray ) causing JSX parse failure.
At line 125, the closing </title> tag is followed by an extra ) that will cause a syntax error. The element should be followed directly by {topologyNode} on the next line.
Suggested fix
- <title>{node.disabled}</title>){topologyNode}
+ <title>{node.disabled}</title>
+ {topologyNode}📝 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.
| <title>{node.disabled}</title>){topologyNode} | |
| <title>{node.disabled}</title> | |
| {topologyNode} |
🤖 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 `@web/src/components/topology/Korrel8rTopology.tsx` at line 125, In
Korrel8rTopology.tsx inside the JSX that renders the title and topologyNode (the
line containing <title>{node.disabled}</title>){topologyNode}), remove the stray
closing parenthesis so the fragment flows directly from the closing </title> to
{topologyNode}; locate the JSX in the Korrel8rTopology component where
node.disabled and topologyNode are used and change that sequence to end the
</title> tag and then immediately render {topologyNode}.
| const nodes: NodeModel[] = useMemo( | ||
| (): NodeModel[] => | ||
| graph.nodes.map((node: korrel8r.Node) => { | ||
| const newNode = { ...node }; | ||
| if (newNode.error) { | ||
| if (node.disabled) { | ||
| // eslint-disable-next-line no-console | ||
| console.warn(`korrel8r node: ${newNode.error}`); | ||
| newNode.error = Error(t('Unable to find Console Link')); | ||
| } else if (newNode.class.domain === 'log' && !loggingAvailable) { | ||
| newNode.error = Error(t('Logging Plugin Disabled')); | ||
| } else if (newNode.class.domain === 'netflow' && !netobserveAvailable) { | ||
| newNode.error = Error(t('Netflow Plugin Disabled')); | ||
| console.warn(`korrel8r node: ${node.disabled}`); | ||
| node.disabled = t('Unable to find Console Link'); | ||
| } else if (node.class.domain === 'log' && !loggingAvailable) { | ||
| node.disabled = t('Logging Plugin Disabled'); | ||
| } else if (node.class.domain === 'netflow' && !netobserveAvailable) { | ||
| node.disabled = t('Netflow Plugin Disabled'); | ||
| } |
There was a problem hiding this comment.
Avoid mutating graph.nodes when deriving disabled UI state.
At Lines 156-163, mutating node.disabled in-place can leave nodes permanently disabled/wrongly labeled across later renders when availability flags change.
Suggested fix
- graph.nodes.map((node: korrel8r.Node) => {
- if (node.disabled) {
+ graph.nodes.map((node: korrel8r.Node) => {
+ const disabled =
+ node.disabled
+ ? t('Unable to find Console Link')
+ : node.class.domain === 'log' && !loggingAvailable
+ ? t('Logging Plugin Disabled')
+ : node.class.domain === 'netflow' && !netobserveAvailable
+ ? t('Netflow Plugin Disabled')
+ : undefined;
+ if (node.disabled) {
// eslint-disable-next-line no-console
console.warn(`korrel8r node: ${node.disabled}`);
- node.disabled = t('Unable to find Console Link');
- } else if (node.class.domain === 'log' && !loggingAvailable) {
- node.disabled = t('Logging Plugin Disabled');
- } else if (node.class.domain === 'netflow' && !netobserveAvailable) {
- node.disabled = t('Netflow Plugin Disabled');
}
return {
id: node.id,
@@
- data: node,
+ data: { ...node, disabled },
};
}),🤖 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 `@web/src/components/topology/Korrel8rTopology.tsx` around lines 153 - 164, The
current mapping in nodes (useMemo) mutates the source graph.nodes by setting
node.disabled directly, which can persist incorrect state; instead, do not
modify graph.nodes in-place—create and return new NodeModel objects (or shallow
copies of each korrel8r.Node) and set the disabled display string on those
copies based on loggingAvailable, netobserveAvailable and the existing
node.disabled value (using korrel8r.Node, graph.nodes, node.disabled,
loggingAvailable, netobserveAvailable and t()) so the UI state is derived
immutably and availability changes won’t leak into the original graph data.
| this.markers = node.markers; | ||
| } |
There was a problem hiding this comment.
Default markers to an empty array in Node constructor.
At Line 273, node.markers is optional in the API contract; assigning it directly can leave this.markers undefined and break shape expectations.
Suggested fix
- this.markers = node.markers;
+ this.markers = node.markers ?? [];📝 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.
| this.markers = node.markers; | |
| } | |
| this.markers = node.markers ?? []; | |
| } |
🤖 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 `@web/src/korrel8r/types.ts` around lines 273 - 274, Node constructor assigns
this.markers directly from node.markers which may be undefined; update the Node
constructor to default markers to an empty array (e.g., set this.markers =
node.markers ?? [] or use a fallback) so this.markers is always an array and
preserves the expected shape for consumers (refer to the Node constructor and
the this.markers / node.markers symbols).
|
|
||
| node(id: string): Node { | ||
| return this.nodeByClass[id]; | ||
| return this.nodeByClass?.[id]; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify Map declaration and non-Map-style indexing usage.
rg -n "nodeByClass|new Map\\(|\\.set\\(|\\.get\\(|\\[id\\]|\\[n\\.class\\]" web/src/korrel8r/types.ts -C2Repository: openshift/troubleshooting-panel-console-plugin
Length of output: 1406
Use Map#get() and Map#set() consistently for nodeByClass access instead of bracket notation.
At line 353, bracket indexing ?.[id] is used on a Map, which is not the Map API and bypasses type safety. The return type annotation claims Node but bracket notation actually returns Node | undefined. Use .get(id) instead, which properly returns Node | undefined.
Suggested fix
export class Graph {
nodes: Array<Node>;
edges: Array<Edge>;
private nodeByClass: Map<string, Node>;
constructor(graph: api.Graph) {
this.nodeByClass = new Map();
this.nodes = (graph?.nodes ?? []).map((n) => {
const node = new Node(n);
- this.nodeByClass[n.class] = node;
+ this.nodeByClass.set(n.class, node);
return node;
});
@@
- node(id: string): Node {
- return this.nodeByClass?.[id];
+ node(id: string): Node | undefined {
+ return this.nodeByClass.get(id);
}
}🤖 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 `@web/src/korrel8r/types.ts` at line 353, The getter is accessing a Map using
bracket notation (this.nodeByClass?.[id]) which bypasses the Map API and types;
replace that access with the Map#get method (this.nodeByClass?.get(id)) and
ensure the declared return type reflects Node | undefined if it currently
asserts Node; update any corresponding set usage to Map#set if needed so
nodeByClass is consistently accessed via .get/.set (refer to the nodeByClass
property and the function/method containing the return).
|
@alanconway: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Korrel8r can add "markers" to nodes to indicate special conditions.
Summary by CodeRabbit
New Features
constraintparameter to the objects search endpointStyle
Tests