NETOBSERV-2376 fix conditions icons / colors#1445
NETOBSERV-2376 fix conditions icons / colors#1445openshift-merge-bot[bot] merged 4 commits intonetobserv:mainfrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 20 minutes and 31 seconds. ⌛ 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR modernizes FlowCollector status rendering by introducing a centralized Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
jpinsonneau
added
the
ok-to-test
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
web/src/components/forms/resource-status.tsx (1)
157-160: Avoidanyfor FlowCollector status access.This helper depends on specific
components/integrationsfields; typing that shape will catch misspellings and stale mappings.Proposed typed shape
+type FlowCollectorStatus = { + components?: { + agent?: ComponentStatus; + processor?: ComponentStatus; + plugin?: ComponentStatus; + }; + integrations?: { + monitoring?: ComponentStatus; + loki?: ComponentStatus; + }; +}; + function waitingComponentState(existing: K8sResourceKind | null, suffix: string): string | undefined { - const st = existing?.status as any; + const st = existing?.status as FlowCollectorStatus | undefined; if (!st) return undefined;As per coding guidelines,
web/src/**/*.{ts,tsx}: Check React hooks dependencies, component re-render optimization, and TypeScript type safety.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/forms/resource-status.tsx` around lines 157 - 160, The function waitingComponentState currently casts existing?.status to any which hides shape errors; define a dedicated type (e.g., FlowCollectorStatus) that describes the expected properties (components and integrations and their inner shapes) and use it instead of any when reading existing.status in waitingComponentState (and any other accessors of K8sResourceKind.status); update the function signature/locals to cast to FlowCollectorStatus | undefined, ensure you reference the exact property names components and integrations to catch misspellings, and adjust any callers or imports to use the new type from the appropriate module.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/src/components/forms/resource-status.tsx`:
- Around line 190-201: The Waiting* branch is not recognizing the operator's
"ComponentUnused" reason so unused Waiting conditions render as unknown; update
the conditional that checks reason ('if (status === 'Unknown' && reason ===
'Unused')') to also accept the operator form (e.g. change to check reason ===
'Unused' || reason === 'ComponentUnused' or use a suffix match like
reason.endsWith('Unused')) so that Waiting* conditions with reason
"ComponentUnused" map to 'unused' in the component that contains
type.startsWith('Waiting'), suffix, status, reason, waitingComponentState and
WAITING_NO_STATUS_FIELD.
---
Nitpick comments:
In `@web/src/components/forms/resource-status.tsx`:
- Around line 157-160: The function waitingComponentState currently casts
existing?.status to any which hides shape errors; define a dedicated type (e.g.,
FlowCollectorStatus) that describes the expected properties (components and
integrations and their inner shapes) and use it instead of any when reading
existing.status in waitingComponentState (and any other accessors of
K8sResourceKind.status); update the function signature/locals to cast to
FlowCollectorStatus | undefined, ensure you reference the exact property names
components and integrations to catch misspellings, and adjust any callers or
imports to use the new type from the appropriate module.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4ad4dfc5-eafd-483c-bb8b-a677faaa0bc9
📒 Files selected for processing (1)
web/src/components/forms/resource-status.tsx
|
New image: quay.io/netobserv/network-observability-console-plugin:14d63ba
It will expire in two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=14d63ba make set-plugin-image |
jpinsonneau
force-pushed
the
2376-followup
branch
from
eb1166d to
fab3539
Compare
github-actions
Bot
removed
the
ok-to-test
jpinsonneau
added
the
ok-to-test
|
New image: quay.io/netobserv/network-observability-console-plugin:4fa5cd3
It will expire in two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=4fa5cd3 make set-plugin-image |
|
/cherry-pick main-pf5 |
|
@jpinsonneau: once the present PR merges, I will cherry-pick it on top of 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 kubernetes-sigs/prow repository. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1445 +/- ##
=======================================
Coverage 51.76% 51.76%
=======================================
Files 233 233
Lines 13194 13194
Branches 1559 1559
=======================================
Hits 6830 6830
Misses 5750 5750
Partials 614 614
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
github-actions
Bot
removed
the
ok-to-test
jpinsonneau
added
ok-to-test
|
New image: quay.io/netobserv/network-observability-console-plugin:0d8458d
It will expire in two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=0d8458d make set-plugin-image |
| return <ConnectedIcon color="var(--pf-v6-global--success-color--100)" />; | ||
| case 'degraded': | ||
| return <ExclamationTriangleIcon color="var(--pf-v5-global--warning-color--100)" />; | ||
| return <ExclamationTriangleIcon color="var(--pf-v6-global--warning-color--100)" />; | ||
| case 'pending': | ||
| return <ExclamationTriangleIcon color="var(--pf-v5-global--warning-color--100)" />; | ||
| return <ExclamationTriangleIcon color="var(--pf-v6-global--warning-color--100)" />; | ||
| case 'error': | ||
| return <ExclamationCircleIcon color="var(--pf-v5-global--danger-color--100)" />; | ||
| return <ExclamationCircleIcon color="var(--pf-v6-global--danger-color--100)" />; | ||
| case 'onHold': | ||
| return <PauseCircleIcon color="var(--pf-v5-global--info-color--100)" />; | ||
| return <PauseCircleIcon color="var(--pf-v6-global--info-color--100)" />; |
There was a problem hiding this comment.
These colors doesn't exists in PF6
I need to change the references
jpinsonneau
force-pushed
the
2376-followup
branch
from
3ab6cd8 to
5dc02f9
Compare
github-actions
Bot
removed
the
ok-to-test
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/components/forms/resource-status.tsx (1)
284-295:⚠️ Potential issue | 🟡 MinorAvoid mutating
existing.status.conditionsduring render.
.sort()mutates the array returned fromfcStatus.conditions; copy it first so rendering this table does not reorder the resource object in place.Proposed fix
- const conditions = (fcStatus?.conditions || []).sort((a, b) => { + const conditions = [...(fcStatus?.conditions || [])].sort((a, b) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/forms/resource-status.tsx` around lines 284 - 295, The current render mutates fcStatus.conditions because .sort() is called directly on it; create a shallow copy before sorting (e.g., copy the array from fcStatus?.conditions) and then call .sort() on that copy so the original existing.status.conditions is not mutated; update the variable where conditions is defined (the expression that builds conditions using fcStatus?.conditions and sortConditions) to first clone the array (e.g., via spread or slice) and then apply the sort.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/src/components/forms/resource-status.tsx`:
- Around line 227-230: The current early return in resource-status.tsx marks any
status === 'True' as 'success' which misclassifies cases like status === 'True'
with reason containing 'Degraded' (e.g., "Ready,Degraded"); change the ordering
to check for degraded first: if status === 'True' and reason includes 'Degraded'
(or equals 'Degraded') return 'degraded' (or 'warning' per your existing
conventions) before the generic status === 'True' success branch; update the
conditional in the function that uses variables status and reason so it aligns
with web/src/components/forms/utils.ts behavior.
In `@web/src/utils/__tests__/theme-hook.spec.ts`:
- Around line 22-26: The test duplicates the v6 case but the hook (useTheme in
web/src/utils/theme-hook.ts) still supports pf-v5-theme-dark; update the spec so
one test covers pf-v6-theme-dark and the other covers pf-v5-theme-dark: change
the second test that currently adds 'pf-v6-theme-dark' to instead add
'pf-v5-theme-dark', call renderHook(() => useTheme()) and assert result.current
is true, ensuring both themes are verified; alternatively, if v5 support is
intentionally removed, delete the v5 handling from useTheme instead (keep
references to useTheme and the two tests in theme-hook.spec.ts consistent).
---
Outside diff comments:
In `@web/src/components/forms/resource-status.tsx`:
- Around line 284-295: The current render mutates fcStatus.conditions because
.sort() is called directly on it; create a shallow copy before sorting (e.g.,
copy the array from fcStatus?.conditions) and then call .sort() on that copy so
the original existing.status.conditions is not mutated; update the variable
where conditions is defined (the expression that builds conditions using
fcStatus?.conditions and sortConditions) to first clone the array (e.g., via
spread or slice) and then apply the sort.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ee3992ff-fc47-4444-af17-7eeec52a5c4c
📒 Files selected for processing (5)
web/cypress/integration-tests/overview_page.cy.tsweb/cypress/views/netflow-page.tsweb/src/components/forms/resource-status.tsxweb/src/components/status/flowcollector-status-icon.tsxweb/src/utils/__tests__/theme-hook.spec.ts
✅ Files skipped from review due to trivial changes (1)
- web/src/components/status/flowcollector-status-icon.tsx
| it('should return true when pf-v6-theme-dark class is present', () => { | ||
| document.documentElement.classList.add('pf-v6-theme-dark'); | ||
| const { result } = renderHook(() => useTheme()); | ||
| expect(result.current).toBe(true); | ||
| }); |
There was a problem hiding this comment.
Restore the pf-v5-theme-dark coverage.
This test now duplicates Lines 16-20, while useTheme still supports pf-v5-theme-dark in web/src/utils/theme-hook.ts. Keep one v6 test and make this case cover v5, or remove v5 support from the hook if it is no longer intended.
Proposed fix
- it('should return true when pf-v6-theme-dark class is present', () => {
- document.documentElement.classList.add('pf-v6-theme-dark');
+ it('should return true when pf-v5-theme-dark class is present', () => {
+ document.documentElement.classList.add('pf-v5-theme-dark');
const { result } = renderHook(() => useTheme());
expect(result.current).toBe(true);
});📝 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.
| it('should return true when pf-v6-theme-dark class is present', () => { | |
| document.documentElement.classList.add('pf-v6-theme-dark'); | |
| const { result } = renderHook(() => useTheme()); | |
| expect(result.current).toBe(true); | |
| }); | |
| it('should return true when pf-v5-theme-dark class is present', () => { | |
| document.documentElement.classList.add('pf-v5-theme-dark'); | |
| const { result } = renderHook(() => useTheme()); | |
| expect(result.current).toBe(true); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/utils/__tests__/theme-hook.spec.ts` around lines 22 - 26, The test
duplicates the v6 case but the hook (useTheme in web/src/utils/theme-hook.ts)
still supports pf-v5-theme-dark; update the spec so one test covers
pf-v6-theme-dark and the other covers pf-v5-theme-dark: change the second test
that currently adds 'pf-v6-theme-dark' to instead add 'pf-v5-theme-dark', call
renderHook(() => useTheme()) and assert result.current is true, ensuring both
themes are verified; alternatively, if v5 support is intentionally removed,
delete the v5 handling from useTheme instead (keep references to useTheme and
the two tests in theme-hook.spec.ts consistent).
a68b049 to
9707082
Compare
jpinsonneau
added
the
ok-to-test
|
New image: quay.io/netobserv/network-observability-console-plugin:60790c7
It will expire in two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=60790c7 make set-plugin-image |
github-actions
Bot
removed
the
ok-to-test
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/components/forms/resource-status.tsx (1)
285-296:⚠️ Potential issue | 🟡 MinorAvoid mutating
fcStatus.conditionsduring render.
.sort()mutates the array returned fromexisting.status. Copy it first so rendering does not reorder prop/cache-owned data.Proposed change
- const conditions = (fcStatus?.conditions || []).sort((a, b) => { + const conditions = [...(fcStatus?.conditions ?? [])].sort((a, b) => {As per coding guidelines,
web/src/**/*.{ts,tsx}: Check React hooks dependencies, component re-render optimization, and TypeScript type safety.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/forms/resource-status.tsx` around lines 285 - 296, The current render mutates fcStatus.conditions by calling .sort() directly; change the code to sort a shallow copy instead (e.g., make a copy of fcStatus?.conditions via spread or slice before sorting) so that the original prop/cache-owned array is not mutated; update the expression that defines conditions (which currently uses fcStatus?.conditions and sortConditions) to create and sort a copied array and keep the existing sort comparator logic unchanged.
🧹 Nitpick comments (2)
web/cypress/views/netflow-page.ts (1)
57-60: Wait on the refresh option, not a global PatternFly menu.
.pf-v6-c-menucan match any visible menu. Waiting onOFF_KEYdirectly makes this flow less flaky.Proposed change
- cy.get('.pf-v6-c-menu').should('be.visible') - cy.get('[data-test="OFF_KEY"]') + cy.byTestID('OFF_KEY') .should('be.visible') .click()As per coding guidelines,
web/cypress/**/*.ts: Verify E2E test stability, proper waits, and selector resilience.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/views/netflow-page.ts` around lines 57 - 60, Replace the broad wait on the global PatternFly menu with a direct wait on the specific refresh option: remove or stop relying on cy.get('.pf-v6-c-menu').should('be.visible') and instead target the unique selector cy.get('[data-test="OFF_KEY"]') — assert .should('be.visible') on that element and then .click() it; update the code around the selectors where .pf-v6-c-menu and [data-test="OFF_KEY"] are used so the test waits on the specific option rather than the global menu to reduce flakiness.web/cypress/integration-tests/overview_page.cy.ts (1)
129-129: Avoid PatternFly class selectors for the panel checkbox.The checkbox id is already stable; the
.pf-v6-*class makes this test brittle across PatternFly updates. Scope the id to the modal instead.Proposed change
- cy.get('.pf-v6-c-data-list__check > `#top_avg_packet_rates`').click(); + cy.get(overviewSelectors.panelsModal) + .find('#top_avg_packet_rates') + .should('exist') + .click();As per coding guidelines,
web/cypress/**/*.ts: Verify E2E test stability, proper waits, and selector resilience.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/integration-tests/overview_page.cy.ts` at line 129, The selector uses a brittle PatternFly class; replace ".pf-v6-c-data-list__check > `#top_avg_packet_rates`" with a stable scoped lookup that targets the modal/container first (e.g. the modal's id or data-testid) and then finds the checkbox by its stable id "#top_avg_packet_rates"; also ensure the container is visible before interacting (e.g. assert the modal is visible then find("#top_avg_packet_rates").click()) so the test is resilient to PF class changes and timing issues.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/src/components/forms/resource-status.tsx`:
- Around line 206-210: The block handling type === 'ConfigurationIssue' treats
non-True states as 'unknown' causing a normal False condition to render
unhealthy; update the conditional to explicitly return 'success' (or the
component's healthy token) when status === 'False' so that
ConfigurationIssue=False is rendered healthy—modify the type ===
'ConfigurationIssue' branch (using the existing status and reason variables and
replacing the fallback return 'unknown') to return 'success' for status ===
'False', keep the existing checks for status === 'True' && reason === 'Error' ->
'error' and status === 'True' && reason === 'Warnings' -> 'warning'.
---
Outside diff comments:
In `@web/src/components/forms/resource-status.tsx`:
- Around line 285-296: The current render mutates fcStatus.conditions by calling
.sort() directly; change the code to sort a shallow copy instead (e.g., make a
copy of fcStatus?.conditions via spread or slice before sorting) so that the
original prop/cache-owned array is not mutated; update the expression that
defines conditions (which currently uses fcStatus?.conditions and
sortConditions) to create and sort a copied array and keep the existing sort
comparator logic unchanged.
---
Nitpick comments:
In `@web/cypress/integration-tests/overview_page.cy.ts`:
- Line 129: The selector uses a brittle PatternFly class; replace
".pf-v6-c-data-list__check > `#top_avg_packet_rates`" with a stable scoped lookup
that targets the modal/container first (e.g. the modal's id or data-testid) and
then finds the checkbox by its stable id "#top_avg_packet_rates"; also ensure
the container is visible before interacting (e.g. assert the modal is visible
then find("#top_avg_packet_rates").click()) so the test is resilient to PF class
changes and timing issues.
In `@web/cypress/views/netflow-page.ts`:
- Around line 57-60: Replace the broad wait on the global PatternFly menu with a
direct wait on the specific refresh option: remove or stop relying on
cy.get('.pf-v6-c-menu').should('be.visible') and instead target the unique
selector cy.get('[data-test="OFF_KEY"]') — assert .should('be.visible') on that
element and then .click() it; update the code around the selectors where
.pf-v6-c-menu and [data-test="OFF_KEY"] are used so the test waits on the
specific option rather than the global menu to reduce flakiness.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f6ae8696-adbe-4419-99bc-8b9a336d2287
📒 Files selected for processing (4)
web/cypress/integration-tests/overview_page.cy.tsweb/cypress/views/netflow-page.tsweb/src/components/forms/resource-status.tsxweb/src/components/status/flowcollector-status-icon.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/components/status/flowcollector-status-icon.tsx
| if (type === 'ConfigurationIssue') { | ||
| if (status === 'True' && reason === 'Error') return 'error'; | ||
| if (status === 'True' && reason === 'Warnings') return 'warning'; | ||
| return 'unknown'; | ||
| } |
There was a problem hiding this comment.
Render ConfigurationIssue=False as healthy.
ConfigurationIssue is a negative condition from the operator: True means an issue exists. With the current fallback, the normal False state renders as unknown instead of success.
Proposed change
if (type === 'ConfigurationIssue') {
if (status === 'True' && reason === 'Error') return 'error';
if (status === 'True' && reason === 'Warnings') return 'warning';
+ if (status === 'False') return 'success';
return 'unknown';
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/forms/resource-status.tsx` around lines 206 - 210, The
block handling type === 'ConfigurationIssue' treats non-True states as 'unknown'
causing a normal False condition to render unhealthy; update the conditional to
explicitly return 'success' (or the component's healthy token) when status ===
'False' so that ConfigurationIssue=False is rendered healthy—modify the type ===
'ConfigurationIssue' branch (using the existing status and reason variables and
replacing the fallback return 'unknown') to return 'success' for status ===
'False', keep the existing checks for status === 'True' && reason === 'Error' ->
'error' and status === 'True' && reason === 'Warnings' -> 'warning'.
|
/ok-to-test |
openshift-ci
Bot
added
the
ok-to-test
|
New image: quay.io/netobserv/network-observability-console-plugin:8193b05
It will expire in two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=8193b05 make set-plugin-image |
jpinsonneau
force-pushed
the
2376-followup
branch
from
601ce50 to
ae34d78
Compare
|
Rebased without changes |
github-actions
Bot
removed
the
ok-to-test
|
/ok-to-test |
openshift-ci
Bot
added
the
ok-to-test
|
New image: quay.io/netobserv/network-observability-console-plugin:fff4049
It will expire in two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=fff4049 make set-plugin-image |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: 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-merge-bot
Bot
merged commit 8f299f9
into
netobserv:main
|
@jpinsonneau: new pull request created: #1454 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 kubernetes-sigs/prow repository. |
3 participants
Description
Followup on NETOBSERV-2376 to fix conditions icons and colors since we reverted the logic in operator.
Also see netobserv/netobserv-operator#2677 for onHold and kafka condition fixes
Dependencies
n/a
Checklist
Summary by CodeRabbit