feat: display korrel8r status in the graph#237
Open
alanconway wants to merge 1 commit into
Open
Conversation
|
[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 StatusCount schema/type and QueryCount.statuses, a status normalization/merge module, topology UI updates to render per-status decorators and ignore disabled nodes, switches Node error storage to a disabled message, and updates tests and CSS. ChangesStatus Count Support & Disabled Node Handling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🤖 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 125-126: Guard access to node.class and prevent navigation for
disabled nodes: ensure any use of node.class is preceded by a presence check
(e.g., node?.class) and/or check node?.disabled before dereferencing; update the
context menu logic tied to hideContextMenuKebab to use node?.class safely, and
modify the selection/navigation path (handlers referenced by
onStatusDecoratorClick and the onSelect/navigation code) to early-return when
node?.disabled is true so disabled parse-failure nodes neither dereference class
nor trigger navigation.
- Around line 159-163: The useEffect currently returns early when locationQuery
is falsy, leaving stale selection; update the effect so that when locationQuery
is absent it calls setSelectedIds([]) to clear selection, otherwise compute id
from locationQuery.class and setSelectedIds(graph.node(id) ? [id] : []); keep
the logic inside the same useEffect that references locationQuery, graph,
setSelectedIds.
🪄 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: e776538d-4623-4935-8e4b-057ad1e9342b
⛔ 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
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/korrel8r/types.ts (1)
292-299:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInitialize
statuseseven when query parsing fails.If
Query.parse(qc.query)throws, this constructor keeps the instance alive viaerror, butstatusesnever gets assigned. That breaks the newQueryCount.statusescontract and can still crash downstream status-merging/rendering code on malformed API data. Move the defaulting outside thetryso error cases still expose[].Suggested fix
constructor(qc: api.QueryCount) { + this.count = qc.count; + this.statuses = qc.statuses ?? []; try { - this.count = qc.count; this.query = Query.parse(qc.query); - this.statuses = qc.statuses ?? []; } catch (e) { this.error = e; } }🤖 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 292 - 299, The constructor currently sets this.statuses inside the try block so if Query.parse(qc.query) throws the instance ends up with this.error but no statuses; move the defaulting so this.statuses = qc.statuses ?? [] is executed outside (before or after) the try/catch so QueryCount.statuses is always initialized to an array even on parse failures; keep Query.parse(qc.query) inside the try/catch so exceptions are still captured into this.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.
Outside diff comments:
In `@web/src/korrel8r/types.ts`:
- Around line 292-299: The constructor currently sets this.statuses inside the
try block so if Query.parse(qc.query) throws the instance ends up with
this.error but no statuses; move the defaulting so this.statuses = qc.statuses
?? [] is executed outside (before or after) the try/catch so QueryCount.statuses
is always initialized to an array even on parse failures; keep
Query.parse(qc.query) inside the try/catch so exceptions are still captured into
this.error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: aa2fd041-0bc4-44d9-84d7-e6fd3ae57b69
⛔ Files ignored due to path filters (1)
web/src/korrel8r/client/types.gen.tsis excluded by!**/*.gen.*
📒 Files selected for processing (8)
korrel8r/korrel8r-openapi.yamlweb/src/__tests__/status.spec.tsweb/src/__tests__/types.spec.tsweb/src/components/topology/Korrel8rTopology.tsxweb/src/components/topology/korrel8rtopology.cssweb/src/components/topology/status.tsweb/src/korrel8r/client/index.tsweb/src/korrel8r/types.ts
✅ Files skipped from review due to trivial changes (2)
- web/src/korrel8r/client/index.ts
- web/src/tests/types.spec.ts
Korrel8r can add status to query results to indicate special conditions. "error"/"warning" korrel8r status is shown as patternfly "danger"/"warning" Any other non-empty korrel8r status is shown as patternfly "info" - updated korrel8r/korrel8r-openapi.yaml, includes status in graph nodes. - updated topology nodes to display status in graph. - mark node matching current view "selected", follow view changes. Other: - Updated devspace.yaml to ignore build churn in sync process. WIP: add "info" status to nodes for anything that's not warning/error.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/korrel8r/types.ts (1)
292-299:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInitialize
statuseseven on parse failure paths.Line 296 only sets
this.statusesinsidetry. If query parsing fails,statusesstaysundefinedand laterqc.statuses.forEach(...)can crash in topology status aggregation.Proposed fix
constructor(qc: api.QueryCount) { + this.statuses = qc.statuses ?? []; try { this.count = qc.count; this.query = Query.parse(qc.query); - this.statuses = qc.statuses ?? []; } catch (e) { this.error = e; } }🤖 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 292 - 299, The constructor currently sets this.statuses only inside the try block so if Query.parse(qc.query) throws, this.statuses remains undefined; initialize this.statuses unconditionally (e.g., set this.statuses = qc.statuses ?? [] before or at the start of the constructor) or ensure the catch block assigns this.statuses = qc.statuses ?? [] so that later code (e.g., any qc.statuses.forEach) never encounters undefined; keep the existing error capture (this.error = e) but always ensure this.statuses is a defined array.
🤖 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 258-266: The status labels render entries even when status is
empty, producing blank labels and invalid props for Label; update the rendering
in Korrel8rTopology to filter qc.statuses for truthy/non-empty status values
before mapping (e.g., use qc.statuses.filter(({status}) => Boolean(status)) ),
then map that filtered array to produce <Label> elements using
statusName(toStatus(status)) and count; ensure you only call toStatus/statusName
for entries with a valid status so Label's status prop is never passed an
invalid value.
In `@web/src/components/topology/status.ts`:
- Around line 41-44: The aggregation adds sc.count directly even though count is
optional; change the logic in the qc.statuses.forEach handler to treat
missing/invalid counts as zero (e.g., use (sc.count ?? 0) or Number(sc.count) ||
0) before doing m.set(sc.status, ...), so m.set(sc.status, (m.get(sc.status) ??
0) + (sc.count ?? 0)); keep the existing guard for sc.status and the s =
Math.max(s, toStatus(sc.status)) line unchanged.
---
Outside diff comments:
In `@web/src/korrel8r/types.ts`:
- Around line 292-299: The constructor currently sets this.statuses only inside
the try block so if Query.parse(qc.query) throws, this.statuses remains
undefined; initialize this.statuses unconditionally (e.g., set this.statuses =
qc.statuses ?? [] before or at the start of the constructor) or ensure the catch
block assigns this.statuses = qc.statuses ?? [] so that later code (e.g., any
qc.statuses.forEach) never encounters undefined; keep the existing error capture
(this.error = e) but always ensure this.statuses is a defined array.
🪄 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: 3ecb3d6d-6598-42d8-aaf7-a7300204e42b
⛔ Files ignored due to path filters (1)
web/src/korrel8r/client/types.gen.tsis excluded by!**/*.gen.*
📒 Files selected for processing (8)
korrel8r/korrel8r-openapi.yamlweb/src/__tests__/status.spec.tsweb/src/__tests__/types.spec.tsweb/src/components/topology/Korrel8rTopology.tsxweb/src/components/topology/korrel8rtopology.cssweb/src/components/topology/status.tsweb/src/korrel8r/client/index.tsweb/src/korrel8r/types.ts
Comment on lines
+258
to
+266
| {qc.statuses?.length > 0 && ( | ||
| <LabelGroup> | ||
| {qc.statuses.map(({ status, count }) => ( | ||
| <Label key={status} isCompact status={statusName(toStatus(status))}> | ||
| {status} {count} | ||
| </Label> | ||
| ))} | ||
| </LabelGroup> | ||
| )} |
There was a problem hiding this comment.
Filter empty status entries in query menu labels.
Lines 258-266 render status rows without checking status, which can produce blank labels and invalid status prop mapping for Label.
Proposed fix
- {qc.statuses?.length > 0 && (
+ {qc.statuses?.some((s) => !!s.status) && (
<LabelGroup>
- {qc.statuses.map(({ status, count }) => (
- <Label key={status} isCompact status={statusName(toStatus(status))}>
+ {qc.statuses
+ .filter(({ status }) => !!status)
+ .map(({ status, count }) => (
+ <Label key={status} isCompact status={statusName(toStatus(status))}>
{status} {count}
</Label>
- ))}
+ ))}
</LabelGroup>
)}📝 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
| {qc.statuses?.length > 0 && ( | |
| <LabelGroup> | |
| {qc.statuses.map(({ status, count }) => ( | |
| <Label key={status} isCompact status={statusName(toStatus(status))}> | |
| {status} {count} | |
| </Label> | |
| ))} | |
| </LabelGroup> | |
| )} | |
| {qc.statuses?.some((s) => !!s.status) && ( | |
| <LabelGroup> | |
| {qc.statuses | |
| .filter(({ status }) => !!status) | |
| .map(({ status, count }) => ( | |
| <Label key={status} isCompact status={statusName(toStatus(status))}> | |
| {status} {count} | |
| </Label> | |
| ))} | |
| </LabelGroup> | |
| )} |
🤖 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 258 - 266, The
status labels render entries even when status is empty, producing blank labels
and invalid props for Label; update the rendering in Korrel8rTopology to filter
qc.statuses for truthy/non-empty status values before mapping (e.g., use
qc.statuses.filter(({status}) => Boolean(status)) ), then map that filtered
array to produce <Label> elements using statusName(toStatus(status)) and count;
ensure you only call toStatus/statusName for entries with a valid status so
Label's status prop is never passed an invalid value.
Comment on lines
+41
to
+44
| qc.statuses.forEach((sc) => { | ||
| if (!sc.status) return; | ||
| m.set(sc.status, (m.get(sc.status) ?? 0) + sc.count); | ||
| s = Math.max(s, toStatus(sc.status)); |
There was a problem hiding this comment.
Guard optional status counts before aggregation.
Line 43 adds sc.count directly, but count is optional in the API schema. undefined here yields NaN totals and broken badges/tooltips.
Proposed fix
- m.set(sc.status, (m.get(sc.status) ?? 0) + sc.count);
+ m.set(sc.status, (m.get(sc.status) ?? 0) + (sc.count ?? 0));📝 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
| qc.statuses.forEach((sc) => { | |
| if (!sc.status) return; | |
| m.set(sc.status, (m.get(sc.status) ?? 0) + sc.count); | |
| s = Math.max(s, toStatus(sc.status)); | |
| qc.statuses.forEach((sc) => { | |
| if (!sc.status) return; | |
| m.set(sc.status, (m.get(sc.status) ?? 0) + (sc.count ?? 0)); | |
| s = Math.max(s, toStatus(sc.status)); |
🤖 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/status.ts` around lines 41 - 44, The aggregation
adds sc.count directly even though count is optional; change the logic in the
qc.statuses.forEach handler to treat missing/invalid counts as zero (e.g., use
(sc.count ?? 0) or Number(sc.count) || 0) before doing m.set(sc.status, ...), so
m.set(sc.status, (m.get(sc.status) ?? 0) + (sc.count ?? 0)); keep the existing
guard for sc.status and the s = Math.max(s, toStatus(sc.status)) line unchanged.
|
@alanconway: all tests passed! 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. |
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.
Korrel8r can add status to query results to indicate special conditions.
Summary by CodeRabbit
New Features
Improvements
Tests