NETOBSERV-1829 Expose status + NETOBSERV-2376 Report more info#1388
Open
jpinsonneau wants to merge 5 commits intonetobserv:mainfrom
Open
NETOBSERV-1829 Expose status + NETOBSERV-2376 Report more info#1388jpinsonneau wants to merge 5 commits intonetobserv:mainfrom
jpinsonneau wants to merge 5 commits intonetobserv:mainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds FlowCollector status support: new locale keys, a status-derivation utility, status icon and indicator components that watch the FlowCollector CR, UI integrations (banner, headers, flow traffic form), pipeline/resource-status UI changes, dummy CR updates, and unit tests plus a Jest mock. Changes
Sequence DiagramsequenceDiagram
participant UI as User/UI
participant Indicator as FlowCollectorStatusIndicator
participant Watch as useK8sWatchResource
participant K8s as Kubernetes API
participant Utils as getFlowCollectorOverallStatus
UI->>Indicator: render header button
Indicator->>Watch: subscribe to FlowCollector CR (flows.netobserv.io/v1beta2, name:"cluster")
Watch->>K8s: watch FlowCollector resource
K8s-->>Watch: return (fc, loaded, loadError)
Watch-->>Indicator: provide (fc, loaded, loadError)
Indicator->>Utils: compute overall status (fc, loadError)
Utils-->>Indicator: return status (ready/degraded/pending/error/onHold/loading)
Indicator->>UI: render FlowCollectorStatusIcon + tooltip (status)
UI-->>Indicator: user clicks button
Indicator->>UI: navigate to flowCollectorStatusPath
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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 |
![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: 1
🧹 Nitpick comments (1)
web/src/components/forms/flowCollector-status.tsx (1)
41-42: Don’t inferloadedfromloadError.
getFlowCollectorOverallStatusexpects the real watch completion flag, butResourceWatcherContextonly exposesloadError. Passing!ctx.loadErrorcollapses “still loading” into “loaded with no error”. Please threadloadedthrough the context or derive the status before the context boundary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/forms/flowCollector-status.tsx` around lines 41 - 42, The code incorrectly infers loaded as !ctx.loadError when calling getFlowCollectorOverallStatus; instead expose or derive a real loaded flag from ResourceWatcherContext and use that. Update ResourceWatcherContext to include a boolean loaded set when the watch completes (or compute loaded before crossing the context boundary where you have the real watch completion), then replace calls using !ctx.loadError with ctx.loaded when invoking getFlowCollectorOverallStatus and any other places relying on load state (reference: getFlowCollectorOverallStatus, ResourceWatcherContext, ctx.loadError, ctx.loaded).
🤖 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/setup-tests.tsx`:
- Around line 31-33: The global test mock for useK8sWatchResource currently
returns [null, false, null], forcing loaded=false for all consumers; change the
export in web/setup-tests.tsx to a configurable jest.fn() named
useK8sWatchResource that by default returns [null, true, null] (loaded=true) and
allow individual tests to override behavior via mockImplementation or
mockImplementationOnce to simulate loading/error states for components like
resource-watcher.tsx; update any tests that rely on the former global loading
behavior to explicitly set the mock to [null, false, null] or an error tuple
when needed.
---
Nitpick comments:
In `@web/src/components/forms/flowCollector-status.tsx`:
- Around line 41-42: The code incorrectly infers loaded as !ctx.loadError when
calling getFlowCollectorOverallStatus; instead expose or derive a real loaded
flag from ResourceWatcherContext and use that. Update ResourceWatcherContext to
include a boolean loaded set when the watch completes (or compute loaded before
crossing the context boundary where you have the real watch completion), then
replace calls using !ctx.loadError with ctx.loaded when invoking
getFlowCollectorOverallStatus and any other places relying on load state
(reference: getFlowCollectorOverallStatus, ResourceWatcherContext,
ctx.loadError, ctx.loaded).
🪄 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: 86cd83fc-c74f-4a74-baf5-043d88609d7c
📒 Files selected for processing (8)
web/locales/en/plugin__netobserv-plugin.jsonweb/setup-tests.tsxweb/src/components/alerts/banner.tsxweb/src/components/forms/flowCollector-status.tsxweb/src/components/forms/utils.tsweb/src/components/health/network-health.tsxweb/src/components/netflow-traffic.tsxweb/src/components/status/flowcollector-status-indicator.tsx
web/setup-tests.tsx
Outdated
Comment on lines
+31
to
+33
| useK8sWatchResource: () => { | ||
| return [null, false, null]; | ||
| }, |
Contributor
There was a problem hiding this comment.
Don’t leave every useK8sWatchResource mock in the loading state.
Returning [null, false, null] here forces loaded=false for every consumer. web/src/components/forms/resource-watcher.tsx will stay on its spinner path, and the new indicator can only render loading in tests. Please make this a configurable jest.fn() with a loaded default, then opt into loading/error states per test.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/setup-tests.tsx` around lines 31 - 33, The global test mock for
useK8sWatchResource currently returns [null, false, null], forcing loaded=false
for all consumers; change the export in web/setup-tests.tsx to a configurable
jest.fn() named useK8sWatchResource that by default returns [null, true, null]
(loaded=true) and allow individual tests to override behavior via
mockImplementation or mockImplementationOnce to simulate loading/error states
for components like resource-watcher.tsx; update any tests that rely on the
former global loading behavior to explicitly set the mock to [null, false, null]
or an error tuple when needed.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
web/src/components/forms/utils.ts (1)
27-38: Make theReadycheck authoritative beforehasError.Right now any non-
ComponentUnusedfalse condition short-circuits topending, so a CR withReady=Trueplus another false condition will never surface as ready here. Downstream, that keepsshowTrafficButtonfalse inweb/src/components/forms/flowCollector-status.tsxLine 42.Proposed refactor
- const hasError = conditions.some(c => c.status === 'False' && c.reason !== 'ComponentUnused'); - if (hasError) { - const readyCondition = conditions.find(c => c.type === 'Ready'); - if (readyCondition?.status === 'False') { - return 'error'; - } - return 'pending'; - } const readyCondition = conditions.find(c => c.type === 'Ready'); if (readyCondition?.status === 'True') { return 'ready'; } + const hasError = conditions.some(c => c.status === 'False' && c.reason !== 'ComponentUnused'); + if (readyCondition?.status === 'False' && hasError) { + return 'error'; + } return 'pending';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/forms/utils.ts` around lines 27 - 38, The Ready condition must be evaluated first so a Ready=True wins even if other conditions are False; update the logic around the conditions array (the hasError boolean and readyCondition lookup) to first find readyCondition (conditions.find(c => c.type === 'Ready')) and if its status === 'True' immediately return 'ready', otherwise proceed to evaluate hasError (conditions.some(c => c.status === 'False' && c.reason !== 'ComponentUnused')) and if readyCondition?.status === 'False' return 'error', else return 'pending' as appropriate; adjust the code in utils.ts where hasError and readyCondition are computed so Ready is authoritative.
🤖 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/flowCollector-status.tsx`:
- Around line 41-42: The status computation is using a synthetic loaded flag
(!ctx.loadError) which diverges from the real watch loaded state; update the
code so the real loaded flag from the watch is used: either add a loaded
property to ResourceWatcherContext and propagate the watch's loaded value
through the context, then call getFlowCollectorOverallStatus(ctx.data,
ctx.loaded, ctx.loadError) from flowCollector-status.tsx, or change
getFlowCollectorOverallStatus to drop the loaded parameter and compute readiness
from data/loadError consistently (also update flowcollector-status-indicator.tsx
to match). Ensure the fix references getFlowCollectorOverallStatus,
ResourceWatcherContext, flowCollector-status.tsx,
flowcollector-status-indicator.tsx, and useK8sWatchResource so both components
use the same real loaded boolean.
---
Nitpick comments:
In `@web/src/components/forms/utils.ts`:
- Around line 27-38: The Ready condition must be evaluated first so a Ready=True
wins even if other conditions are False; update the logic around the conditions
array (the hasError boolean and readyCondition lookup) to first find
readyCondition (conditions.find(c => c.type === 'Ready')) and if its status ===
'True' immediately return 'ready', otherwise proceed to evaluate hasError
(conditions.some(c => c.status === 'False' && c.reason !== 'ComponentUnused'))
and if readyCondition?.status === 'False' return 'error', else return 'pending'
as appropriate; adjust the code in utils.ts where hasError and readyCondition
are computed so Ready is authoritative.
🪄 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: 2148dc3a-cc3f-4376-b9cd-8bdcd576a81f
📒 Files selected for processing (8)
web/locales/en/plugin__netobserv-plugin.jsonweb/setup-tests.tsxweb/src/components/alerts/banner.tsxweb/src/components/forms/flowCollector-status.tsxweb/src/components/forms/utils.tsweb/src/components/health/network-health.tsxweb/src/components/netflow-traffic.tsxweb/src/components/status/flowcollector-status-indicator.tsx
✅ Files skipped from review due to trivial changes (2)
- web/locales/en/plugin__netobserv-plugin.json
- web/src/components/status/flowcollector-status-indicator.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- web/src/components/alerts/banner.tsx
- web/src/components/netflow-traffic.tsx
- web/setup-tests.tsx
- web/src/components/health/network-health.tsx
jpinsonneau
changed the title
jpinsonneau
added
the
ok-to-test
|
New image: quay.io/netobserv/network-observability-console-plugin:71c0c00
It will expire in two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=71c0c00 make set-plugin-image |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
web/src/components/status/__tests__/flowcollector-status.spec.tsx (1)
71-81: Test doesn't fully exercise theComponentUnusedexclusion.The condition has
status: 'Unknown', which wouldn't triggerhasErroranyway (only'False'does). To validate the exclusion logic atutils.ts:26, usestatus: 'False':Suggested fix
it('should ignore ComponentUnused conditions', () => { const cr = { status: { conditions: [ { type: 'Ready', status: 'True', reason: 'Ready' }, - { type: 'WaitingFLPTransformer', status: 'Unknown', reason: 'ComponentUnused' } + { type: 'WaitingFLPTransformer', status: 'False', reason: 'ComponentUnused' } ] } }; expect(getFlowCollectorOverallStatus(cr, null)).toBe('ready'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/status/__tests__/flowcollector-status.spec.tsx` around lines 71 - 81, The test for getFlowCollectorOverallStatus does not exercise the ComponentUnused exclusion because the condition uses status: 'Unknown' instead of a failing status; change the WaitingFLPTransformer condition's status from 'Unknown' to 'False' so hasError (referenced in utils.ts at the exclusion logic around line with ComponentUnused) would normally mark it as an error but is excluded by the ComponentUnused reason, validating the exclusion behavior in getFlowCollectorOverallStatus.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@web/src/components/status/__tests__/flowcollector-status.spec.tsx`:
- Around line 71-81: The test for getFlowCollectorOverallStatus does not
exercise the ComponentUnused exclusion because the condition uses status:
'Unknown' instead of a failing status; change the WaitingFLPTransformer
condition's status from 'Unknown' to 'False' so hasError (referenced in utils.ts
at the exclusion logic around line with ComponentUnused) would normally mark it
as an error but is excluded by the ComponentUnused reason, validating the
exclusion behavior in getFlowCollectorOverallStatus.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8512c42a-201d-4810-9a6c-8f2f7849964a
📒 Files selected for processing (5)
web/setup-tests.tsxweb/src/components/forms/flowCollector-status.tsxweb/src/components/forms/utils.tsweb/src/components/status/__tests__/flowcollector-status.spec.tsxweb/src/components/status/flowcollector-status-indicator.tsx
✅ Files skipped from review due to trivial changes (1)
- web/setup-tests.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/components/status/flowcollector-status-indicator.tsx
github-actions
bot
removed
the
ok-to-test
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/flowCollector-status.tsx`:
- Around line 54-57: The Button wrapping the FlowCollectorStatusIcon (Button
component rendering at the FlowCollector status page) is non-functional and
should be replaced with a non-interactive element: remove the Button and render
the FlowCollectorStatusIcon inside a semantic, non-interactive wrapper (e.g., a
span or div) instead of Button, drop the aria-label and variant props, preserve
existing styling (cursor: 'default' or className) and keep the surrounding
FlexItem; update any imports/usages that assumed Button here and ensure no
onClick or role="button" remains.
In `@web/src/components/status/flowcollector-status-icon.tsx`:
- Around line 49-52: The icon-only Status component currently places the
translated status text only inside Tooltip (tooltipContent) making it
inaccessible to non-mouse users; update the wrapper span that currently renders
{icon} to include role="img" and aria-label set to the localized tooltipContent
(or the same translated status string) so screen readers always announce the
status—modify the span around {icon} in flowcollector-status-icon.tsx (the
element inside Tooltip) to add role="img" and aria-label={tooltipContent} while
preserving the existing Tooltip and icon usage.
🪄 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: 29ce06b0-c0c4-4200-bf64-17fc17d4453a
📒 Files selected for processing (10)
web/locales/en/plugin__netobserv-plugin.jsonweb/setup-tests.tsxweb/src/components/alerts/banner.tsxweb/src/components/forms/flowCollector-status.tsxweb/src/components/forms/utils.tsweb/src/components/health/network-health.tsxweb/src/components/netflow-traffic.tsxweb/src/components/status/__tests__/flowcollector-status.spec.tsxweb/src/components/status/flowcollector-status-icon.tsxweb/src/components/status/flowcollector-status-indicator.tsx
✅ Files skipped from review due to trivial changes (4)
- web/setup-tests.tsx
- web/src/components/alerts/banner.tsx
- web/locales/en/plugin__netobserv-plugin.json
- web/src/components/status/flowcollector-status-indicator.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- web/src/components/netflow-traffic.tsx
- web/src/components/status/tests/flowcollector-status.spec.tsx
Comment on lines
+54
to
+57
| <FlexItem> | ||
| <Button variant="plain" aria-label={t('FlowCollector status')} style={{ cursor: 'default' }}> | ||
| <FlowCollectorStatusIcon status={status} /> | ||
| </Button> |
Contributor
There was a problem hiding this comment.
Make the title indicator non-interactive on this page.
This Button has no action, so it adds a dead interactive control to the tab order. Since the user is already on the FlowCollector status page, render the icon in a non-button wrapper here instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/forms/flowCollector-status.tsx` around lines 54 - 57, The
Button wrapping the FlowCollectorStatusIcon (Button component rendering at the
FlowCollector status page) is non-functional and should be replaced with a
non-interactive element: remove the Button and render the
FlowCollectorStatusIcon inside a semantic, non-interactive wrapper (e.g., a span
or div) instead of Button, drop the aria-label and variant props, preserve
existing styling (cursor: 'default' or className) and keep the surrounding
FlexItem; update any imports/usages that assumed Button here and ensure no
onClick or role="button" remains.
Comment on lines
+49
to
+52
| return ( | ||
| <Tooltip content={tooltipContent} position="bottom"> | ||
| <span style={{ display: 'inline-flex', verticalAlign: 'middle' }}>{icon}</span> | ||
| </Tooltip> |
Contributor
There was a problem hiding this comment.
Expose the localized status outside the tooltip.
Right now this component renders only a visual icon plus a tooltip. That makes the new status indicator effectively mouse-first unless each caller remembers to add its own accessible name. Put the translated status on the wrapper (role="img" + aria-label) so the state is announced everywhere this icon is used.
♿ Proposed fix
return (
<Tooltip content={tooltipContent} position="bottom">
- <span style={{ display: 'inline-flex', verticalAlign: 'middle' }}>{icon}</span>
+ <span
+ role="img"
+ aria-label={tooltipContent}
+ style={{ display: 'inline-flex', verticalAlign: 'middle' }}
+ >
+ {icon}
+ </span>
</Tooltip>
);📝 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
| return ( | |
| <Tooltip content={tooltipContent} position="bottom"> | |
| <span style={{ display: 'inline-flex', verticalAlign: 'middle' }}>{icon}</span> | |
| </Tooltip> | |
| return ( | |
| <Tooltip content={tooltipContent} position="bottom"> | |
| <span | |
| role="img" | |
| aria-label={tooltipContent} | |
| style={{ display: 'inline-flex', verticalAlign: 'middle' }} | |
| > | |
| {icon} | |
| </span> | |
| </Tooltip> | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/status/flowcollector-status-icon.tsx` around lines 49 -
52, The icon-only Status component currently places the translated status text
only inside Tooltip (tooltipContent) making it inaccessible to non-mouse users;
update the wrapper span that currently renders {icon} to include role="img" and
aria-label set to the localized tooltipContent (or the same translated status
string) so screen readers always announce the status—modify the span around
{icon} in flowcollector-status-icon.tsx (the element inside Tooltip) to add
role="img" and aria-label={tooltipContent} while preserving the existing Tooltip
and icon usage.
Member
Author
|
@jotak would it be good to remove the 
|
jpinsonneau
changed the title
jpinsonneau
added
the
ok-to-test
github-actions
bot
removed
the
ok-to-test
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
jpinsonneau
added
the
ok-to-test
|
New image: quay.io/netobserv/network-observability-console-plugin:1de08d8
It will expire in two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=1de08d8 make set-plugin-image |
|
New image: quay.io/netobserv/network-observability-console-plugin:b118697
It will expire in two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=b118697 make set-plugin-image |
Contributor
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/flowCollector-status.tsx (1)
125-125:⚠️ Potential issue | 🟡 MinorTypo: "retreiving" → "retrieving".
✏️ Fix
- {t('An error occured while retreiving FlowCollector: {{error}}', { error: ctx.loadError })} + {t('An error occurred while retrieving FlowCollector: {{error}}', { error: ctx.loadError })}Note: Also "occured" → "occurred".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/forms/flowCollector-status.tsx` at line 125, The translation string passed to t(...) in FlowCollector-status.tsx has typos; update the message inside the t call from "An error occured while retreiving FlowCollector: {{error}}" to "An error occurred while retrieving FlowCollector: {{error}}" (keep the interpolation { error: ctx.loadError } unchanged) so the UI displays correct spelling.
🧹 Nitpick comments (8)
web/src/components/forms/pipeline.tsx (3)
314-316: Magic number100for setTimeout delay.Consider extracting as a named constant for clarity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/forms/pipeline.tsx` around lines 314 - 316, The anonymous setTimeout delay of 100 in the pipeline component is a magic number—replace it with a named constant (e.g., FIT_DELAY_MS or RESIZE_DEBOUNCE_MS) declared near the top of the module or component so its meaning is clear; update the call that currently invokes setTimeout(() => fit(), 100) to use that constant and add a brief comment about the constant's purpose to aid readability and future tuning (reference the fit invocation inside the pipeline component).
66-84:stateToRunStatusandexporterStateToRunStatusare nearly identical.Consider unifying them. The only difference is
Ready+unhealthyPodCounthandling, which could be a parameter or handled upstream.Also applies to: 86-99
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/forms/pipeline.tsx` around lines 66 - 84, The two functions stateToRunStatus and exporterStateToRunStatus are almost identical; unify them by extracting a single helper (e.g., normalizeStateToRunStatus) that accepts the ComponentStatus-like object plus an optional flag or predicate for the Ready+unhealthyPodCount check (or accept unhealthyPodCount as a parameter), then replace both stateToRunStatus and exporterStateToRunStatus to call that helper; reference the existing symbols stateToRunStatus, exporterStateToRunStatus and unhealthyPodCount when updating callers so the Ready branch logic is preserved via the extra parameter instead of duplicating switch logic.
123-125: Empty catch block silently swallows errors.At minimum, log or report the error to aid debugging.
♻️ Add minimal logging
- } catch { + } catch (e) { + console.warn('StepNode render failed', e); return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/forms/pipeline.tsx` around lines 123 - 125, The current catch block in web/src/components/forms/pipeline.tsx swallows errors (catch { return null; })—change it to capture the thrown error (e.g., catch (err)) and log or report it before returning null; use the app's logger if available (or console.error) and include contextual info (function/component name and relevant variables) so errors in the pipeline form logic are visible during debugging.web/moduleMapper/dummy.tsx (1)
313-326: Random state updates don't include 'Unused'.The
statesarray omits'Unused', so that UI path won't be exercised during dev testing. Add it if you want coverage.♻️ Include Unused state
- const states = ['Ready', 'InProgress', 'Degraded', 'Failure']; + const states = ['Ready', 'InProgress', 'Degraded', 'Failure', 'Unused'];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/moduleMapper/dummy.tsx` around lines 313 - 326, The simulated status updater omits the 'Unused' state so the UI path isn't exercised; update the local states array (the one defined as const states = ['Ready', 'InProgress', 'Degraded', 'Failure'];) inside the random update block that mutates fc.status.components[...] and fc.status.integrations.monitoring to include 'Unused' (e.g., add 'Unused' to the states array) so randomized assignments to component or integration state can produce that value during dev testing.web/src/components/forms/resource-status.tsx (3)
119-135: Using array index as React key for exporters.
exporter-${i}works here since exporters likely don't reorder, but if the list can change order, consider using a stable identifier likeexp.nameorexp.typecombined.🤖 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 119 - 135, The list mapping uses array index as the React key and selector id (`key={`exporter-${i}`}` and selectedTypes entries), which can break stability if exporters reorder; change the key and the values stored in selectedTypes to a stable identifier such as `${exp.name || exp.type}` (or another unique property on each exporter) and update the row selection logic in the map and the onRowClick handler (and any consumers of selectedTypes, e.g., setSelectedTypes) to use that stable id instead of `exporter-${i}` so keys and selection remain consistent when the array changes.
29-61: Consider consolidatingstateColorandStateIconinto a single mapping.Both functions map the same
statevalues. A single lookup object would reduce duplication and ensure consistency.♻️ Example consolidation
const STATE_CONFIG: Record<string, { color: LabelColor; icon: React.ReactNode }> = { Ready: { color: 'green', icon: <CheckCircleIcon color="var(--pf-v5-global--success-color--100)" /> }, Degraded: { color: 'orange', icon: <ExclamationTriangleIcon color="var(--pf-v5-global--warning-color--100)" /> }, // ... etc }; const stateColor = (state: string | undefined): LabelColor => STATE_CONFIG[state ?? '']?.color ?? 'grey'; const StateIcon: FC<{ state: string | undefined }> = ({ state }) => STATE_CONFIG[state ?? '']?.icon ?? <UnknownIcon ... />;🤖 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 29 - 61, stateColor and StateIcon duplicate the same state-to-presentation mapping; consolidate them by creating a single lookup (e.g., STATE_CONFIG) that maps each state string to an object with color and icon properties, then rewrite stateColor to return STATE_CONFIG[state ?? '']?.color ?? 'grey' and change StateIcon to return STATE_CONFIG[state ?? '']?.icon ?? <UnknownIcon ...>, updating imports/types as needed (refer to the stateColor and StateIcon symbols to locate the code and ensure LabelColor typing and default fallbacks are preserved).
114-116: Nested ternaries are hard to scan.This chain determines the Details cell content. Consider extracting to a helper for readability.
♻️ Cleaner approach
+const getStatusDetail = (status: ComponentStatus): string => + status.podIssues || status.message || status.reason || '-'; + <Td> - {c.status.podIssues ? c.status.podIssues : c.status.message ? c.status.message : c.status.reason || '-'} + {getStatusDetail(c.status)} </Td>🤖 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 114 - 116, The Details cell uses nested ternaries (c.status.podIssues ? ... ) which is hard to read; extract this logic into a small helper like getStatusDetail(status) (or statusMessage(status)) that returns status.podIssues || status.message || status.reason || '-' using optional chaining/nullish coalescing, then replace the inline expression in the <Td> with a call to that helper; implement the helper in the same resource-status.tsx (or a nearby util) to improve readability and testability and reference c.status when invoking it.web/src/components/forms/utils.ts (1)
27-31: Magic string'Ready,Degraded'couples to operator's exact output.If the operator changes this reason format, the degraded state won't be detected. Consider extracting as a constant or matching with a pattern (e.g., includes 'Degraded').
♻️ More resilient check
if (readyCondition?.status === 'True') { - if (readyCondition.reason === 'Ready,Degraded') { + if (readyCondition.reason?.includes('Degraded')) { return 'degraded'; } return 'ready'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/forms/utils.ts` around lines 27 - 31, The check for readyCondition.reason === 'Ready,Degraded' is brittle; update the code around readyCondition (the block that currently returns 'degraded' when reason equals 'Ready,Degraded') to use a resilient check: either extract the literal into a constant (e.g., READY_DEGRADED_REASON) and compare to that constant, or match the reason with a pattern such as readyCondition.reason?.includes('Degraded') or a regex (/Degraded/). Ensure the new check still falls back to returning 'ready' when not degraded and keep references to readyCondition.reason and the branch that returns 'degraded' intact so behavior is clear.
🤖 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/pipeline.tsx`:
- Around line 218-228: The processor step uses _.last(steps)!.id which can throw
if steps is empty; change the runAfterTasks creation in the block that builds
the processor step (where existing?.spec is checked and the object with id:
'processor' is pushed) to safely compute the previous id—e.g., check
steps.length or _.last(steps) before accessing .id and either set runAfterTasks
to an empty array or omit it when there is no previous step; update any
references to runAfterTasks consumers to accept an empty/missing array if
needed.
---
Outside diff comments:
In `@web/src/components/forms/flowCollector-status.tsx`:
- Line 125: The translation string passed to t(...) in FlowCollector-status.tsx
has typos; update the message inside the t call from "An error occured while
retreiving FlowCollector: {{error}}" to "An error occurred while retrieving
FlowCollector: {{error}}" (keep the interpolation { error: ctx.loadError }
unchanged) so the UI displays correct spelling.
---
Nitpick comments:
In `@web/moduleMapper/dummy.tsx`:
- Around line 313-326: The simulated status updater omits the 'Unused' state so
the UI path isn't exercised; update the local states array (the one defined as
const states = ['Ready', 'InProgress', 'Degraded', 'Failure'];) inside the
random update block that mutates fc.status.components[...] and
fc.status.integrations.monitoring to include 'Unused' (e.g., add 'Unused' to the
states array) so randomized assignments to component or integration state can
produce that value during dev testing.
In `@web/src/components/forms/pipeline.tsx`:
- Around line 314-316: The anonymous setTimeout delay of 100 in the pipeline
component is a magic number—replace it with a named constant (e.g., FIT_DELAY_MS
or RESIZE_DEBOUNCE_MS) declared near the top of the module or component so its
meaning is clear; update the call that currently invokes setTimeout(() => fit(),
100) to use that constant and add a brief comment about the constant's purpose
to aid readability and future tuning (reference the fit invocation inside the
pipeline component).
- Around line 66-84: The two functions stateToRunStatus and
exporterStateToRunStatus are almost identical; unify them by extracting a single
helper (e.g., normalizeStateToRunStatus) that accepts the ComponentStatus-like
object plus an optional flag or predicate for the Ready+unhealthyPodCount check
(or accept unhealthyPodCount as a parameter), then replace both stateToRunStatus
and exporterStateToRunStatus to call that helper; reference the existing symbols
stateToRunStatus, exporterStateToRunStatus and unhealthyPodCount when updating
callers so the Ready branch logic is preserved via the extra parameter instead
of duplicating switch logic.
- Around line 123-125: The current catch block in
web/src/components/forms/pipeline.tsx swallows errors (catch { return null;
})—change it to capture the thrown error (e.g., catch (err)) and log or report
it before returning null; use the app's logger if available (or console.error)
and include contextual info (function/component name and relevant variables) so
errors in the pipeline form logic are visible during debugging.
In `@web/src/components/forms/resource-status.tsx`:
- Around line 119-135: The list mapping uses array index as the React key and
selector id (`key={`exporter-${i}`}` and selectedTypes entries), which can break
stability if exporters reorder; change the key and the values stored in
selectedTypes to a stable identifier such as `${exp.name || exp.type}` (or
another unique property on each exporter) and update the row selection logic in
the map and the onRowClick handler (and any consumers of selectedTypes, e.g.,
setSelectedTypes) to use that stable id instead of `exporter-${i}` so keys and
selection remain consistent when the array changes.
- Around line 29-61: stateColor and StateIcon duplicate the same
state-to-presentation mapping; consolidate them by creating a single lookup
(e.g., STATE_CONFIG) that maps each state string to an object with color and
icon properties, then rewrite stateColor to return STATE_CONFIG[state ??
'']?.color ?? 'grey' and change StateIcon to return STATE_CONFIG[state ??
'']?.icon ?? <UnknownIcon ...>, updating imports/types as needed (refer to the
stateColor and StateIcon symbols to locate the code and ensure LabelColor typing
and default fallbacks are preserved).
- Around line 114-116: The Details cell uses nested ternaries
(c.status.podIssues ? ... ) which is hard to read; extract this logic into a
small helper like getStatusDetail(status) (or statusMessage(status)) that
returns status.podIssues || status.message || status.reason || '-' using
optional chaining/nullish coalescing, then replace the inline expression in the
<Td> with a call to that helper; implement the helper in the same
resource-status.tsx (or a nearby util) to improve readability and testability
and reference c.status when invoking it.
In `@web/src/components/forms/utils.ts`:
- Around line 27-31: The check for readyCondition.reason === 'Ready,Degraded' is
brittle; update the code around readyCondition (the block that currently returns
'degraded' when reason equals 'Ready,Degraded') to use a resilient check: either
extract the literal into a constant (e.g., READY_DEGRADED_REASON) and compare to
that constant, or match the reason with a pattern such as
readyCondition.reason?.includes('Degraded') or a regex (/Degraded/). Ensure the
new check still falls back to returning 'ready' when not degraded and keep
references to readyCondition.reason and the branch that returns 'degraded'
intact so behavior is clear.
🪄 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: dff4c8f3-16b7-4f3d-9222-59cbcbce6212
📒 Files selected for processing (7)
web/moduleMapper/dummy.tsxweb/src/components/forms/flowCollector-status.tsxweb/src/components/forms/pipeline.tsxweb/src/components/forms/resource-status.tsxweb/src/components/forms/utils.tsweb/src/components/status/__tests__/flowcollector-status.spec.tsxweb/src/components/status/flowcollector-status-icon.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- web/src/components/status/tests/flowcollector-status.spec.tsx
- web/src/components/status/flowcollector-status-icon.tsx
github-actions
bot
removed
the
ok-to-test
jpinsonneau
added
the
ok-to-test
kapjain-rh
removed
the
ok-to-test
Member
|
/ok-to-test |
openshift-ci
bot
added
the
ok-to-test
|
New image: quay.io/netobserv/network-observability-console-plugin:61f40a0
It will expire in two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=61f40a0 make set-plugin-image |
1 similar comment
|
New image: quay.io/netobserv/network-observability-console-plugin:61f40a0
It will expire in two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=61f40a0 make set-plugin-image |
Member
Author
|
Rebased without changes |
github-actions
bot
removed
the
ok-to-test
Member
Author
|
Rebased without changes |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1388 +/- ##
==========================================
+ Coverage 52.48% 52.53% +0.05%
==========================================
Files 229 232 +3
Lines 12246 12355 +109
Branches 1531 1551 +20
==========================================
+ Hits 6427 6491 +64
- Misses 5224 5269 +45
Partials 595 595
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Member
|
/ok-to-test |
openshift-ci
bot
added
the
ok-to-test
|
New image: quay.io/netobserv/network-observability-console-plugin:6fe6277
It will expire in two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=6fe6277 make set-plugin-image |
leandroberetta
approved these changes
Apr 14, 2026
Contributor
leandroberetta
left a comment
leandroberetta
left a comment
There was a problem hiding this comment.
lgtm, thanks @jpinsonneau !
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
5 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.
Description
show status indicator next to pages titles
bring user to
FlowCollector/statuspage when clicking on the indicatoradd a link in the alerts to bring the user in the status page too
components status from operator PR below
Normal scenario:
Assisted-by:
claude-4.6-opus-highDependencies
netobserv/netobserv-operator#2610
Checklist
If you are not familiar with our processes or don't know what to answer in the list below, let us know in a comment: the maintainers will take care of that.
Summary by CodeRabbit
New Features
Tests