OU-1212: Create initial observability landing page with summary cards - #1112
OU-1212: Create initial observability landing page with summary cards#1112jeff-phillips-18 wants to merge 1 commit into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@jeff-phillips-18: This pull request references OU-1212 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. 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 NOT APPROVED This pull-request has been approved by: jeff-phillips-18 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 |
|
/cc @jgbernalp |
|
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:
WalkthroughThe PR adds the ChangesObservability services feature
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ 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: 5
🧹 Nitpick comments (9)
web/src/shared/constants/data-test.ts (1)
158-165: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDefine the info-alert close-button test ID in
DataTestIDs.ServicesPage.The close button constructs an identifier outside the centralized test-ID object. Define the identifier once and consume that constant.
web/src/shared/constants/data-test.ts#L158-L165: AddInfoAlertClose: 'services-info-alert-close'toDataTestIDs.ServicesPage.web/src/features/services/pages/ServicesPage.tsx#L45-L45: Replace the template literal withDataTestIDs.ServicesPage.InfoAlertClose.As per coding guidelines, "Use the centralized
DataTestIDsobject ... for test IDs, grouping related IDs in nested objects."🤖 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/shared/constants/data-test.ts` around lines 158 - 165, Add InfoAlertClose with the value services-info-alert-close to DataTestIDs.ServicesPage in web/src/shared/constants/data-test.ts:158-165, then update ServicesPage in web/src/features/services/pages/ServicesPage.tsx:45 to use DataTestIDs.ServicesPage.InfoAlertClose instead of constructing the template literal.Source: Coding guidelines
web/src/features/services/components/summary/TargetsSummaryCard.tsx (1)
68-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExport services summary components by name.
These components use default exports. Use named exports and update their imports.
web/src/features/services/components/summary/TargetsSummaryCard.tsx#L68-L68: ExportTargetsSummaryCardby name and update its import inObservabilityStackSummary.tsx.web/src/features/services/components/summary/MetricsSummaryCard.tsx#L70-L70: ExportMetricsSummaryCardby name and update its import inObservabilityStackSummary.tsx.web/src/features/services/components/summary/ObservabilityStackSummary.tsx#L46-L46: ExportObservabilityStackSummaryby name and update its import inServicesPage.tsx.As per coding guidelines, "Define React components as functional components with explicit type annotations, preferably using
FC, and export them by name."🤖 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/features/services/components/summary/TargetsSummaryCard.tsx` at line 68, Replace the default exports with named exports for TargetsSummaryCard in web/src/features/services/components/summary/TargetsSummaryCard.tsx:68-68, MetricsSummaryCard in web/src/features/services/components/summary/MetricsSummaryCard.tsx:70-70, and ObservabilityStackSummary in web/src/features/services/components/summary/ObservabilityStackSummary.tsx:46-46. Update their corresponding imports in ObservabilityStackSummary.tsx and ServicesPage.tsx to use the named-export syntax.Source: Coding guidelines
pkg/server/plugin_handler.go (1)
45-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd unit coverage for the
servicesmanifest patch.Add a
patchManifesttest with onlyFeatures[Services]enabled. Assert that the patched manifest contains the services navigation item and route. Also assert that the patch is absent when the feature is disabled.As per coding guidelines, "Add unit tests for utility functions, business logic, bug fixes, and backend API handlers."
🤖 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 `@pkg/server/plugin_handler.go` at line 45, Add unit coverage for patchManifest focused on the Services feature: verify that enabling Features[Services] adds both the services navigation item and route to the patched manifest, and verify that disabling it leaves the services patch absent.Source: Coding guidelines
web/src/features/services/pages/ServicesPage.tsx (1)
5-5: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a type-only import for
FC.
FCis used only for type checking. Import it withimport type.Proposed change
-import { FC, useState } from 'react'; +import { useState } from 'react'; +import type { FC } from 'react';As per coding guidelines, "Use type-only imports (
import type) for symbols used only for type checking."🤖 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/features/services/pages/ServicesPage.tsx` at line 5, Update the import in ServicesPage.tsx so FC is brought in through a type-only import, while keeping useState as the regular runtime import from React.Source: Coding guidelines
web/cypress/support/monitoring/04.reg_services.cy.ts (1)
30-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the committed
cy.pause()calls.These commands stop Cypress open-mode execution until a user resumes the test. Keep them only in local debugging changes. (docs.cypress.io)
🤖 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/cypress/support/monitoring/04.reg_services.cy.ts` around lines 30 - 92, Remove all committed cy.pause() calls from the monitoring services test, including those around the alert persistence, summary-card loading, and navigation checks. Preserve the existing assertions, navigation steps, and test flow unchanged.web/src/features/services/components/summary/AlertSummaryCards.tsx (1)
12-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd co-located unit tests for the new summary-card logic.
The existing Cypress component test covers
SummaryCardonly. Add deterministic tests for the data and state mapping in both feature cards.
web/src/features/services/components/summary/AlertSummaryCards.tsx#L12-L63: mockuseAlertsand verify loading, error, alert-rule count, and firing-alert filtering.web/src/features/services/components/summary/DashboardsSummaryCard.tsx#L11-L33: mock the query result and verify loading, error, and dashboard-count states.As per coding guidelines, “Add unit tests for utility functions, business logic, bug fixes, and backend API handlers.”
🤖 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/features/services/components/summary/AlertSummaryCards.tsx` around lines 12 - 63, Add co-located unit tests for AlertSummaryCards.tsx covering mocked useAlerts loading, error, alert-rule count, and firing-alert filtering states. Add tests for DashboardsSummaryCard.tsx using a mocked query result to verify loading, error, and dashboard-count states; update both named files as specified.Source: Coding guidelines
web/src/shared/utils/alert-utils.ts (2)
16-23: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAlign the source-filter type with explicit source IDs.
alertingRuleSourcereturns arbitraryrule.sourceIdvalues on Line 32.useAlertsexposes these values as additional source labels, butAggregatedAlertFilters[AlertFilterOptions.SOURCE]accepts onlyAlertSource[]. Usestring[]so the declared contract includes custom source IDs.🤖 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/shared/utils/alert-utils.ts` around lines 16 - 23, Update AggregatedAlertFilters[AlertFilterOptions.SOURCE] to use string[] instead of AlertSource[], matching the arbitrary source IDs returned by alertingRuleSource and exposed by useAlerts.
1-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse type-only imports for type-only symbols.
Update the affected imports for
Alert,Rule,Perspective,AggregatedAlertFilters,AggregatedAlert,AppDispatch,MonitoringState, andAlertingRulesSourceExtension. Keep runtime symbols such asAlertSource,AlertFilterOptions,filterAlerts,ResourceIcon, andPrometheusEndpointas value imports.🤖 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/shared/utils/alert-utils.ts` around lines 1 - 5, Convert only type-only imports to type imports across web/src/shared/utils/alert-utils.ts lines 1-5, web/src/features/alerts/pages/alerts-page/AlertsPage.tsx lines 41-45, web/src/features/alerts/pages/alerts-page/filter-alerts.spec.ts lines 13-18, web/src/features/alerts/pages/alerts-page/AggregateAlertTableRow.tsx line 15, and web/src/shared/hooks/useAlerts.ts lines 16-17: update imports for Alert, Rule, Perspective, AggregatedAlertFilters, AggregatedAlert, AppDispatch, MonitoringState, and AlertingRulesSourceExtension. Keep AlertSource, AlertFilterOptions, filterAlerts, ResourceIcon, and PrometheusEndpoint as value imports.Source: Coding guidelines
web/src/shared/utils/perses-client.ts (1)
2-2: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse type-only imports for type symbols.
Use
import typeforDashboardResourceandProjectResourceinweb/src/shared/utils/perses-client.ts, and forPersesUserPermissionsandK8sResourceKindinweb/src/features/perses-dashboards/hooks/useEditableProjects.ts. KeepuseFetchPersesPermissionsas a value import.🤖 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/shared/utils/perses-client.ts` at line 2, Change the import in web/src/shared/utils/perses-client.ts at line 2 to a type-only import for DashboardResource and ProjectResource. Change the import in web/src/features/perses-dashboards/hooks/useEditableProjects.ts at line 6 to import PersesUserPermissions and K8sResourceKind as types while keeping useFetchPersesPermissions as a value import.Source: Coding guidelines
🤖 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/cypress/views/services-page.ts`:
- Around line 109-116: Update the callback in the summary-card navigation flow
to return Cypress chains from both branches: in the count-selector branch,
return the visible count click chain followed by a resolved true value, and in
the error-state branch, return cy.wrap(false) after logging. Keep the existing
selector check and navigation behavior unchanged.
In `@web/locales/en/plugin__monitoring-plugin.json`:
- Line 236: Update the English locale entry "Miliseconds" in
plugin__monitoring-plugin.json to use the correctly spelled displayed value
"Milliseconds", preserving the existing translation key.
In `@web/src/features/services/components/summary/AlertSummaryCards.tsx`:
- Around line 18-20: Update the alertsError derivation near rulesAlertLoading to
remove the explicit any cast, narrow loadError as unknown, and safely convert
either its message or the value itself to a string before passing it to
SummaryCard.error.
In `@web/src/shared/utils/alert-utils.ts`:
- Around line 25-35: Add the co-located alert-utils.spec.ts unit test file and
directly cover alertingRuleSource for sourceId 'prometheus' and a custom source
ID, asserting the expected AlertSource or returned identifier. Keep the tests
focused on the shared utility and follow the repository’s existing frontend
unit-test conventions.
In `@web/src/shared/utils/perses-client.ts`:
- Around line 10-101: Add colocated unit tests in perses-client.spec.ts covering
the fetchPersesDashboardsMetadata, fetchPersesDashboardsByProject,
fetchPersesProjects, fetchPersesUserPermissions, and fetchPersesDashboard URL
construction, including encoded path values. Test useFetchPersesPermissions so
an empty username does not invoke consoleFetchJSON, and verify fetch helpers
propagate consoleFetchJSON errors unchanged.
---
Nitpick comments:
In `@pkg/server/plugin_handler.go`:
- Line 45: Add unit coverage for patchManifest focused on the Services feature:
verify that enabling Features[Services] adds both the services navigation item
and route to the patched manifest, and verify that disabling it leaves the
services patch absent.
In `@web/cypress/support/monitoring/04.reg_services.cy.ts`:
- Around line 30-92: Remove all committed cy.pause() calls from the monitoring
services test, including those around the alert persistence, summary-card
loading, and navigation checks. Preserve the existing assertions, navigation
steps, and test flow unchanged.
In `@web/src/features/services/components/summary/AlertSummaryCards.tsx`:
- Around line 12-63: Add co-located unit tests for AlertSummaryCards.tsx
covering mocked useAlerts loading, error, alert-rule count, and firing-alert
filtering states. Add tests for DashboardsSummaryCard.tsx using a mocked query
result to verify loading, error, and dashboard-count states; update both named
files as specified.
In `@web/src/features/services/components/summary/TargetsSummaryCard.tsx`:
- Line 68: Replace the default exports with named exports for TargetsSummaryCard
in web/src/features/services/components/summary/TargetsSummaryCard.tsx:68-68,
MetricsSummaryCard in
web/src/features/services/components/summary/MetricsSummaryCard.tsx:70-70, and
ObservabilityStackSummary in
web/src/features/services/components/summary/ObservabilityStackSummary.tsx:46-46.
Update their corresponding imports in ObservabilityStackSummary.tsx and
ServicesPage.tsx to use the named-export syntax.
In `@web/src/features/services/pages/ServicesPage.tsx`:
- Line 5: Update the import in ServicesPage.tsx so FC is brought in through a
type-only import, while keeping useState as the regular runtime import from
React.
In `@web/src/shared/constants/data-test.ts`:
- Around line 158-165: Add InfoAlertClose with the value
services-info-alert-close to DataTestIDs.ServicesPage in
web/src/shared/constants/data-test.ts:158-165, then update ServicesPage in
web/src/features/services/pages/ServicesPage.tsx:45 to use
DataTestIDs.ServicesPage.InfoAlertClose instead of constructing the template
literal.
In `@web/src/shared/utils/alert-utils.ts`:
- Around line 16-23: Update AggregatedAlertFilters[AlertFilterOptions.SOURCE] to
use string[] instead of AlertSource[], matching the arbitrary source IDs
returned by alertingRuleSource and exposed by useAlerts.
- Around line 1-5: Convert only type-only imports to type imports across
web/src/shared/utils/alert-utils.ts lines 1-5,
web/src/features/alerts/pages/alerts-page/AlertsPage.tsx lines 41-45,
web/src/features/alerts/pages/alerts-page/filter-alerts.spec.ts lines 13-18,
web/src/features/alerts/pages/alerts-page/AggregateAlertTableRow.tsx line 15,
and web/src/shared/hooks/useAlerts.ts lines 16-17: update imports for Alert,
Rule, Perspective, AggregatedAlertFilters, AggregatedAlert, AppDispatch,
MonitoringState, and AlertingRulesSourceExtension. Keep AlertSource,
AlertFilterOptions, filterAlerts, ResourceIcon, and PrometheusEndpoint as value
imports.
In `@web/src/shared/utils/perses-client.ts`:
- Line 2: Change the import in web/src/shared/utils/perses-client.ts at line 2
to a type-only import for DashboardResource and ProjectResource. Change the
import in web/src/features/perses-dashboards/hooks/useEditableProjects.ts at
line 6 to import PersesUserPermissions and K8sResourceKind as types while
keeping useFetchPersesPermissions as a value import.
🪄 Autofix
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: cf9a544a-e7d9-45db-b682-d865e794dc1a
📒 Files selected for processing (39)
MakefileREADME.mdcmd/plugin-backend.goconfig/services.patch.jsonpkg/server/plugin_handler.gopkg/server/server.goweb/cypress/component/SummaryCard.cy.tsxweb/cypress/e2e/monitoring/00.bvt_admin.cy.tsweb/cypress/e2e/monitoring/regression/04.reg_services_admin.cy.tsweb/cypress/support/monitoring/04.reg_services.cy.tsweb/cypress/views/services-page.tsweb/locales/en/plugin__monitoring-plugin.jsonweb/package.jsonweb/src/features/alerts/components/AlertUtils.tsxweb/src/features/alerts/pages/AlertRulesDetailsPage.tsxweb/src/features/alerts/pages/AlertsDetailsPage.tsxweb/src/features/alerts/pages/alert-rules-page/AlertRulesPage.tsxweb/src/features/alerts/pages/alert-rules-page/filter-rules.tsweb/src/features/alerts/pages/alerts-page/AggregateAlertTableRow.tsxweb/src/features/alerts/pages/alerts-page/AlertTableRow.tsxweb/src/features/alerts/pages/alerts-page/AlertsPage.tsxweb/src/features/alerts/pages/alerts-page/filter-alerts.spec.tsweb/src/features/perses-dashboards/components/PersesWrapper.tsxweb/src/features/perses-dashboards/hooks/useEditableProjects.tsweb/src/features/perses-dashboards/hooks/usePerses.tsweb/src/features/perses-dashboards/utils/dashboard-api.tsweb/src/features/perses-dashboards/utils/migrate-api.tsweb/src/features/services/OWNERSweb/src/features/services/components/summary/AlertSummaryCards.tsxweb/src/features/services/components/summary/DashboardsSummaryCard.tsxweb/src/features/services/components/summary/MetricsSummaryCard.tsxweb/src/features/services/components/summary/ObservabilityStackSummary.tsxweb/src/features/services/components/summary/SummaryCard.tsxweb/src/features/services/components/summary/TargetsSummaryCard.tsxweb/src/features/services/pages/ServicesPage.tsxweb/src/shared/constants/data-test.tsweb/src/shared/hooks/useAlerts.tsweb/src/shared/utils/alert-utils.tsweb/src/shared/utils/perses-client.ts
💤 Files with no reviewable changes (1)
- web/src/features/alerts/components/AlertUtils.tsx
| "Bytes Binary Per Second (KiB/s, MiB/s)": "Bytes Binary Per Second (KiB/s, MiB/s)", | ||
| "Bytes Decimal Per Second (kB/s, MB/s)": "Bytes Decimal Per Second (kB/s, MB/s)", | ||
| "Packets Per Second": "Packets Per Second", | ||
| "Miliseconds": "Miliseconds", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the displayed unit label.
Miliseconds is misspelled. Change the English value to Milliseconds.
🤖 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/locales/en/plugin__monitoring-plugin.json` at line 236, Update the
English locale entry "Miliseconds" in plugin__monitoring-plugin.json to use the
correctly spelled displayed value "Milliseconds", preserving the existing
translation key.
| const alertsError = | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| (rulesAlertLoading?.loadError as any)?.message ?? rulesAlertLoading?.loadError; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the any cast from alertsError.
The cast bypasses type checking for the value passed to SummaryCard.error. Narrow loadError as unknown and convert it to a string before passing it to the card.
As per coding guidelines, “Avoid any; use explicit, appropriate types whenever possible.”
Proposed fix
- const alertsError =
- // eslint-disable-next-line `@typescript-eslint/no-explicit-any`
- (rulesAlertLoading?.loadError as any)?.message ?? rulesAlertLoading?.loadError;
+ const loadError = rulesAlertLoading?.loadError;
+ const alertsError =
+ loadError &&
+ typeof loadError === 'object' &&
+ 'message' in loadError &&
+ typeof loadError.message === 'string'
+ ? loadError.message
+ : loadError
+ ? String(loadError)
+ : undefined;📝 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.
| const alertsError = | |
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |
| (rulesAlertLoading?.loadError as any)?.message ?? rulesAlertLoading?.loadError; | |
| const loadError = rulesAlertLoading?.loadError; | |
| const alertsError = | |
| loadError && | |
| typeof loadError === 'object' && | |
| 'message' in loadError && | |
| typeof loadError.message === 'string' | |
| ? loadError.message | |
| : loadError | |
| ? String(loadError) | |
| : undefined; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/src/features/services/components/summary/AlertSummaryCards.tsx` around
lines 18 - 20, Update the alertsError derivation near rulesAlertLoading to
remove the explicit any cast, narrow loadError as unknown, and safely convert
either its message or the value itself to a string before passing it to
SummaryCard.error.
Source: Coding guidelines
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 5
🧹 Nitpick comments (9)
web/src/shared/constants/data-test.ts (1)
158-165: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDefine the info-alert close-button test ID in
DataTestIDs.ServicesPage.The close button constructs an identifier outside the centralized test-ID object. Define the identifier once and consume that constant.
web/src/shared/constants/data-test.ts#L158-L165: AddInfoAlertClose: 'services-info-alert-close'toDataTestIDs.ServicesPage.web/src/features/services/pages/ServicesPage.tsx#L45-L45: Replace the template literal withDataTestIDs.ServicesPage.InfoAlertClose.As per coding guidelines, "Use the centralized
DataTestIDsobject ... for test IDs, grouping related IDs in nested objects."🤖 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/shared/constants/data-test.ts` around lines 158 - 165, Add InfoAlertClose with the value services-info-alert-close to DataTestIDs.ServicesPage in web/src/shared/constants/data-test.ts:158-165, then update ServicesPage in web/src/features/services/pages/ServicesPage.tsx:45 to use DataTestIDs.ServicesPage.InfoAlertClose instead of constructing the template literal.Source: Coding guidelines
web/src/features/services/components/summary/TargetsSummaryCard.tsx (1)
68-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExport services summary components by name.
These components use default exports. Use named exports and update their imports.
web/src/features/services/components/summary/TargetsSummaryCard.tsx#L68-L68: ExportTargetsSummaryCardby name and update its import inObservabilityStackSummary.tsx.web/src/features/services/components/summary/MetricsSummaryCard.tsx#L70-L70: ExportMetricsSummaryCardby name and update its import inObservabilityStackSummary.tsx.web/src/features/services/components/summary/ObservabilityStackSummary.tsx#L46-L46: ExportObservabilityStackSummaryby name and update its import inServicesPage.tsx.As per coding guidelines, "Define React components as functional components with explicit type annotations, preferably using
FC, and export them by name."🤖 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/features/services/components/summary/TargetsSummaryCard.tsx` at line 68, Replace the default exports with named exports for TargetsSummaryCard in web/src/features/services/components/summary/TargetsSummaryCard.tsx:68-68, MetricsSummaryCard in web/src/features/services/components/summary/MetricsSummaryCard.tsx:70-70, and ObservabilityStackSummary in web/src/features/services/components/summary/ObservabilityStackSummary.tsx:46-46. Update their corresponding imports in ObservabilityStackSummary.tsx and ServicesPage.tsx to use the named-export syntax.Source: Coding guidelines
pkg/server/plugin_handler.go (1)
45-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd unit coverage for the
servicesmanifest patch.Add a
patchManifesttest with onlyFeatures[Services]enabled. Assert that the patched manifest contains the services navigation item and route. Also assert that the patch is absent when the feature is disabled.As per coding guidelines, "Add unit tests for utility functions, business logic, bug fixes, and backend API handlers."
🤖 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 `@pkg/server/plugin_handler.go` at line 45, Add unit coverage for patchManifest focused on the Services feature: verify that enabling Features[Services] adds both the services navigation item and route to the patched manifest, and verify that disabling it leaves the services patch absent.Source: Coding guidelines
web/src/features/services/pages/ServicesPage.tsx (1)
5-5: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a type-only import for
FC.
FCis used only for type checking. Import it withimport type.Proposed change
-import { FC, useState } from 'react'; +import { useState } from 'react'; +import type { FC } from 'react';As per coding guidelines, "Use type-only imports (
import type) for symbols used only for type checking."🤖 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/features/services/pages/ServicesPage.tsx` at line 5, Update the import in ServicesPage.tsx so FC is brought in through a type-only import, while keeping useState as the regular runtime import from React.Source: Coding guidelines
web/cypress/support/monitoring/04.reg_services.cy.ts (1)
30-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the committed
cy.pause()calls.These commands stop Cypress open-mode execution until a user resumes the test. Keep them only in local debugging changes. (docs.cypress.io)
🤖 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/cypress/support/monitoring/04.reg_services.cy.ts` around lines 30 - 92, Remove all committed cy.pause() calls from the monitoring services test, including those around the alert persistence, summary-card loading, and navigation checks. Preserve the existing assertions, navigation steps, and test flow unchanged.web/src/features/services/components/summary/AlertSummaryCards.tsx (1)
12-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd co-located unit tests for the new summary-card logic.
The existing Cypress component test covers
SummaryCardonly. Add deterministic tests for the data and state mapping in both feature cards.
web/src/features/services/components/summary/AlertSummaryCards.tsx#L12-L63: mockuseAlertsand verify loading, error, alert-rule count, and firing-alert filtering.web/src/features/services/components/summary/DashboardsSummaryCard.tsx#L11-L33: mock the query result and verify loading, error, and dashboard-count states.As per coding guidelines, “Add unit tests for utility functions, business logic, bug fixes, and backend API handlers.”
🤖 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/features/services/components/summary/AlertSummaryCards.tsx` around lines 12 - 63, Add co-located unit tests for AlertSummaryCards.tsx covering mocked useAlerts loading, error, alert-rule count, and firing-alert filtering states. Add tests for DashboardsSummaryCard.tsx using a mocked query result to verify loading, error, and dashboard-count states; update both named files as specified.Source: Coding guidelines
web/src/shared/utils/alert-utils.ts (2)
16-23: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAlign the source-filter type with explicit source IDs.
alertingRuleSourcereturns arbitraryrule.sourceIdvalues on Line 32.useAlertsexposes these values as additional source labels, butAggregatedAlertFilters[AlertFilterOptions.SOURCE]accepts onlyAlertSource[]. Usestring[]so the declared contract includes custom source IDs.🤖 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/shared/utils/alert-utils.ts` around lines 16 - 23, Update AggregatedAlertFilters[AlertFilterOptions.SOURCE] to use string[] instead of AlertSource[], matching the arbitrary source IDs returned by alertingRuleSource and exposed by useAlerts.
1-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse type-only imports for type-only symbols.
Update the affected imports for
Alert,Rule,Perspective,AggregatedAlertFilters,AggregatedAlert,AppDispatch,MonitoringState, andAlertingRulesSourceExtension. Keep runtime symbols such asAlertSource,AlertFilterOptions,filterAlerts,ResourceIcon, andPrometheusEndpointas value imports.🤖 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/shared/utils/alert-utils.ts` around lines 1 - 5, Convert only type-only imports to type imports across web/src/shared/utils/alert-utils.ts lines 1-5, web/src/features/alerts/pages/alerts-page/AlertsPage.tsx lines 41-45, web/src/features/alerts/pages/alerts-page/filter-alerts.spec.ts lines 13-18, web/src/features/alerts/pages/alerts-page/AggregateAlertTableRow.tsx line 15, and web/src/shared/hooks/useAlerts.ts lines 16-17: update imports for Alert, Rule, Perspective, AggregatedAlertFilters, AggregatedAlert, AppDispatch, MonitoringState, and AlertingRulesSourceExtension. Keep AlertSource, AlertFilterOptions, filterAlerts, ResourceIcon, and PrometheusEndpoint as value imports.Source: Coding guidelines
web/src/shared/utils/perses-client.ts (1)
2-2: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse type-only imports for type symbols.
Use
import typeforDashboardResourceandProjectResourceinweb/src/shared/utils/perses-client.ts, and forPersesUserPermissionsandK8sResourceKindinweb/src/features/perses-dashboards/hooks/useEditableProjects.ts. KeepuseFetchPersesPermissionsas a value import.🤖 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/shared/utils/perses-client.ts` at line 2, Change the import in web/src/shared/utils/perses-client.ts at line 2 to a type-only import for DashboardResource and ProjectResource. Change the import in web/src/features/perses-dashboards/hooks/useEditableProjects.ts at line 6 to import PersesUserPermissions and K8sResourceKind as types while keeping useFetchPersesPermissions as a value import.Source: Coding guidelines
🤖 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/cypress/views/services-page.ts`:
- Around line 109-116: Update the callback in the summary-card navigation flow
to return Cypress chains from both branches: in the count-selector branch,
return the visible count click chain followed by a resolved true value, and in
the error-state branch, return cy.wrap(false) after logging. Keep the existing
selector check and navigation behavior unchanged.
In `@web/locales/en/plugin__monitoring-plugin.json`:
- Line 236: Update the English locale entry "Miliseconds" in
plugin__monitoring-plugin.json to use the correctly spelled displayed value
"Milliseconds", preserving the existing translation key.
In `@web/src/features/services/components/summary/AlertSummaryCards.tsx`:
- Around line 18-20: Update the alertsError derivation near rulesAlertLoading to
remove the explicit any cast, narrow loadError as unknown, and safely convert
either its message or the value itself to a string before passing it to
SummaryCard.error.
In `@web/src/shared/utils/alert-utils.ts`:
- Around line 25-35: Add the co-located alert-utils.spec.ts unit test file and
directly cover alertingRuleSource for sourceId 'prometheus' and a custom source
ID, asserting the expected AlertSource or returned identifier. Keep the tests
focused on the shared utility and follow the repository’s existing frontend
unit-test conventions.
In `@web/src/shared/utils/perses-client.ts`:
- Around line 10-101: Add colocated unit tests in perses-client.spec.ts covering
the fetchPersesDashboardsMetadata, fetchPersesDashboardsByProject,
fetchPersesProjects, fetchPersesUserPermissions, and fetchPersesDashboard URL
construction, including encoded path values. Test useFetchPersesPermissions so
an empty username does not invoke consoleFetchJSON, and verify fetch helpers
propagate consoleFetchJSON errors unchanged.
---
Nitpick comments:
In `@pkg/server/plugin_handler.go`:
- Line 45: Add unit coverage for patchManifest focused on the Services feature:
verify that enabling Features[Services] adds both the services navigation item
and route to the patched manifest, and verify that disabling it leaves the
services patch absent.
In `@web/cypress/support/monitoring/04.reg_services.cy.ts`:
- Around line 30-92: Remove all committed cy.pause() calls from the monitoring
services test, including those around the alert persistence, summary-card
loading, and navigation checks. Preserve the existing assertions, navigation
steps, and test flow unchanged.
In `@web/src/features/services/components/summary/AlertSummaryCards.tsx`:
- Around line 12-63: Add co-located unit tests for AlertSummaryCards.tsx
covering mocked useAlerts loading, error, alert-rule count, and firing-alert
filtering states. Add tests for DashboardsSummaryCard.tsx using a mocked query
result to verify loading, error, and dashboard-count states; update both named
files as specified.
In `@web/src/features/services/components/summary/TargetsSummaryCard.tsx`:
- Line 68: Replace the default exports with named exports for TargetsSummaryCard
in web/src/features/services/components/summary/TargetsSummaryCard.tsx:68-68,
MetricsSummaryCard in
web/src/features/services/components/summary/MetricsSummaryCard.tsx:70-70, and
ObservabilityStackSummary in
web/src/features/services/components/summary/ObservabilityStackSummary.tsx:46-46.
Update their corresponding imports in ObservabilityStackSummary.tsx and
ServicesPage.tsx to use the named-export syntax.
In `@web/src/features/services/pages/ServicesPage.tsx`:
- Line 5: Update the import in ServicesPage.tsx so FC is brought in through a
type-only import, while keeping useState as the regular runtime import from
React.
In `@web/src/shared/constants/data-test.ts`:
- Around line 158-165: Add InfoAlertClose with the value
services-info-alert-close to DataTestIDs.ServicesPage in
web/src/shared/constants/data-test.ts:158-165, then update ServicesPage in
web/src/features/services/pages/ServicesPage.tsx:45 to use
DataTestIDs.ServicesPage.InfoAlertClose instead of constructing the template
literal.
In `@web/src/shared/utils/alert-utils.ts`:
- Around line 16-23: Update AggregatedAlertFilters[AlertFilterOptions.SOURCE] to
use string[] instead of AlertSource[], matching the arbitrary source IDs
returned by alertingRuleSource and exposed by useAlerts.
- Around line 1-5: Convert only type-only imports to type imports across
web/src/shared/utils/alert-utils.ts lines 1-5,
web/src/features/alerts/pages/alerts-page/AlertsPage.tsx lines 41-45,
web/src/features/alerts/pages/alerts-page/filter-alerts.spec.ts lines 13-18,
web/src/features/alerts/pages/alerts-page/AggregateAlertTableRow.tsx line 15,
and web/src/shared/hooks/useAlerts.ts lines 16-17: update imports for Alert,
Rule, Perspective, AggregatedAlertFilters, AggregatedAlert, AppDispatch,
MonitoringState, and AlertingRulesSourceExtension. Keep AlertSource,
AlertFilterOptions, filterAlerts, ResourceIcon, and PrometheusEndpoint as value
imports.
In `@web/src/shared/utils/perses-client.ts`:
- Line 2: Change the import in web/src/shared/utils/perses-client.ts at line 2
to a type-only import for DashboardResource and ProjectResource. Change the
import in web/src/features/perses-dashboards/hooks/useEditableProjects.ts at
line 6 to import PersesUserPermissions and K8sResourceKind as types while
keeping useFetchPersesPermissions as a value import.
🪄 Autofix
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: cf9a544a-e7d9-45db-b682-d865e794dc1a
📒 Files selected for processing (39)
MakefileREADME.mdcmd/plugin-backend.goconfig/services.patch.jsonpkg/server/plugin_handler.gopkg/server/server.goweb/cypress/component/SummaryCard.cy.tsxweb/cypress/e2e/monitoring/00.bvt_admin.cy.tsweb/cypress/e2e/monitoring/regression/04.reg_services_admin.cy.tsweb/cypress/support/monitoring/04.reg_services.cy.tsweb/cypress/views/services-page.tsweb/locales/en/plugin__monitoring-plugin.jsonweb/package.jsonweb/src/features/alerts/components/AlertUtils.tsxweb/src/features/alerts/pages/AlertRulesDetailsPage.tsxweb/src/features/alerts/pages/AlertsDetailsPage.tsxweb/src/features/alerts/pages/alert-rules-page/AlertRulesPage.tsxweb/src/features/alerts/pages/alert-rules-page/filter-rules.tsweb/src/features/alerts/pages/alerts-page/AggregateAlertTableRow.tsxweb/src/features/alerts/pages/alerts-page/AlertTableRow.tsxweb/src/features/alerts/pages/alerts-page/AlertsPage.tsxweb/src/features/alerts/pages/alerts-page/filter-alerts.spec.tsweb/src/features/perses-dashboards/components/PersesWrapper.tsxweb/src/features/perses-dashboards/hooks/useEditableProjects.tsweb/src/features/perses-dashboards/hooks/usePerses.tsweb/src/features/perses-dashboards/utils/dashboard-api.tsweb/src/features/perses-dashboards/utils/migrate-api.tsweb/src/features/services/OWNERSweb/src/features/services/components/summary/AlertSummaryCards.tsxweb/src/features/services/components/summary/DashboardsSummaryCard.tsxweb/src/features/services/components/summary/MetricsSummaryCard.tsxweb/src/features/services/components/summary/ObservabilityStackSummary.tsxweb/src/features/services/components/summary/SummaryCard.tsxweb/src/features/services/components/summary/TargetsSummaryCard.tsxweb/src/features/services/pages/ServicesPage.tsxweb/src/shared/constants/data-test.tsweb/src/shared/hooks/useAlerts.tsweb/src/shared/utils/alert-utils.tsweb/src/shared/utils/perses-client.ts
💤 Files with no reviewable changes (1)
- web/src/features/alerts/components/AlertUtils.tsx
🛑 Comments failed to post (1)
web/src/shared/utils/perses-client.ts (1)
10-101: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add colocated unit tests for the shared Perses client.
Add
web/src/shared/utils/perses-client.spec.ts. Test the generated proxy URLs, encoded path values, permission-query username gating, and error propagation fromconsoleFetchJSON.As per coding guidelines, “Add unit tests for utility functions, business logic, bug fixes, and backend API handlers.”
🤖 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/shared/utils/perses-client.ts` around lines 10 - 101, Add colocated unit tests in perses-client.spec.ts covering the fetchPersesDashboardsMetadata, fetchPersesDashboardsByProject, fetchPersesProjects, fetchPersesUserPermissions, and fetchPersesDashboard URL construction, including encoded path values. Test useFetchPersesPermissions so an empty username does not invoke consoleFetchJSON, and verify fetch helpers propagate consoleFetchJSON errors unchanged.Source: Coding guidelines
fe19ac6 to
5596e73
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
web/src/shared/utils/alert-utils.spec.ts (2)
23-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the undefined
sourceIdbranch.
alertingRuleSourcetreats an undefinedsourceIdas Prometheus. The current tests only cover an explicit"prometheus"source ID. Add a case withsourceId: undefinedand the platform label.Proposed test
+ it('should return Platform for an implicit Prometheus source with platform label', () => { + const rule = makeRule({ + sourceId: undefined, + labels: { prometheus: 'openshift-monitoring/k8s' }, + }); + expect(alertingRuleSource(rule)).toBe(AlertSource.Platform); + }); +As per coding guidelines, add unit tests for utility functions.
🤖 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/shared/utils/alert-utils.spec.ts` around lines 23 - 41, Add a unit test in the alertingRuleSource suite that constructs a rule with sourceId explicitly undefined and the prometheus platform label, then assert alertingRuleSource returns AlertSource.Platform. Keep the existing explicit prometheus, user, and custom-source cases unchanged.Source: Coding guidelines
5-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a type-only import for
Rule.
Ruleis used in type positions in the shown test code. Move it toimport typewhen no runtime use exists in the remaining file.Proposed change
-import { Alert, AlertStates, Rule } from '`@openshift-console/dynamic-plugin-sdk`'; +import { Alert, AlertStates } from '`@openshift-console/dynamic-plugin-sdk`'; +import type { Rule } from '`@openshift-console/dynamic-plugin-sdk`';As per coding guidelines, use type-only imports for symbols used only for type checking.
🤖 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/shared/utils/alert-utils.spec.ts` at line 5, Update the imports in the alert utility test so Rule is imported with a type-only import, while Alert and AlertStates remain value imports if they have runtime usage. Verify Rule has no runtime references elsewhere in the file.Source: Coding guidelines
web/src/features/services/components/summary/alert-summary-cards.spec.ts (1)
35-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the alert fixtures type-safe.
AlertandRuleare only type positions, so import them withtype. Lines 66, 73, and 83 useas any; these casts suppress validation of rule labels used for source classification. UseAlert['rule']or a narrow rule fixture type instead.Proposed change
-import { Alert, AlertStates, Rule } from '`@openshift-console/dynamic-plugin-sdk`'; +import { AlertStates, type Alert, type Rule } from '`@openshift-console/dynamic-plugin-sdk`'; ... - // eslint-disable-next-line `@typescript-eslint/no-explicit-any` - rule: { labels: { prometheus: 'openshift-monitoring/k8s' } } as any, + rule: { labels: { prometheus: 'openshift-monitoring/k8s' } } as Alert['rule'], ... - // eslint-disable-next-line `@typescript-eslint/no-explicit-any` - rule: { labels: {} } as any, + rule: { labels: {} } as Alert['rule'],As per coding guidelines, “Use type-only imports (
import type) for symbols used only for type checking” and “Avoidany; use explicit, appropriate types whenever possible.”Also applies to: 65-83
🤖 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/features/services/components/summary/alert-summary-cards.spec.ts` at line 35, Update the alert fixtures in the summary card tests: import Alert and Rule with import type, and replace the any casts around the rule fixtures with Alert['rule'] or another narrow rule fixture type so rule-label source classification remains type-checked.Source: Coding guidelines
web/src/features/services/components/summary/AlertSummaryCards.tsx (1)
84-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExport the service summary components by name.
Both components only expose default exports. Add named exports. Retain default exports until import sites migrate.
web/src/features/services/components/summary/AlertSummaryCards.tsx#L84-L84: exportAlertSummaryCardsby name.web/src/features/services/components/summary/DashboardsSummaryCard.tsx#L25-L25: exportDashboardsSummaryCardby name.Proposed change
--- a/web/src/features/services/components/summary/AlertSummaryCards.tsx +++ b/web/src/features/services/components/summary/AlertSummaryCards.tsx -const AlertSummaryCards: FC = () => { +export const AlertSummaryCards: FC = () => { --- a/web/src/features/services/components/summary/DashboardsSummaryCard.tsx +++ b/web/src/features/services/components/summary/DashboardsSummaryCard.tsx -const DashboardsSummaryCard: FC = () => { +export const DashboardsSummaryCard: FC = () => {As per coding guidelines, “Define React components as functional components with explicit type annotations, preferably using
FC, and export them by name.”🤖 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/features/services/components/summary/AlertSummaryCards.tsx` at line 84, Export AlertSummaryCards by name while retaining its default export, and likewise add a named export for DashboardsSummaryCard while retaining its default export; update the component declarations in web/src/features/services/components/summary/AlertSummaryCards.tsx:84-84 and web/src/features/services/components/summary/DashboardsSummaryCard.tsx:25-25.Source: Coding guidelines
🤖 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/features/services/components/summary/AlertSummaryCards.tsx`:
- Around line 76-80: Update the summary count logic around AlertSummaryCards to
count only alert rules whose tenant matches defaultAlertTenant, consistent with
AlertRulesPage, while preserving the existing zero fallback. Add a test covering
a mixture of platform and user rules to verify only defaultAlertTenant rules
contribute to rulesCount.
---
Nitpick comments:
In `@web/src/features/services/components/summary/alert-summary-cards.spec.ts`:
- Line 35: Update the alert fixtures in the summary card tests: import Alert and
Rule with import type, and replace the any casts around the rule fixtures with
Alert['rule'] or another narrow rule fixture type so rule-label source
classification remains type-checked.
In `@web/src/features/services/components/summary/AlertSummaryCards.tsx`:
- Line 84: Export AlertSummaryCards by name while retaining its default export,
and likewise add a named export for DashboardsSummaryCard while retaining its
default export; update the component declarations in
web/src/features/services/components/summary/AlertSummaryCards.tsx:84-84 and
web/src/features/services/components/summary/DashboardsSummaryCard.tsx:25-25.
In `@web/src/shared/utils/alert-utils.spec.ts`:
- Around line 23-41: Add a unit test in the alertingRuleSource suite that
constructs a rule with sourceId explicitly undefined and the prometheus platform
label, then assert alertingRuleSource returns AlertSource.Platform. Keep the
existing explicit prometheus, user, and custom-source cases unchanged.
- Line 5: Update the imports in the alert utility test so Rule is imported with
a type-only import, while Alert and AlertStates remain value imports if they
have runtime usage. Verify Rule has no runtime references elsewhere in the file.
🪄 Autofix
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: ddb4fbe3-ad72-4225-a73b-4e4a929f10d0
📒 Files selected for processing (42)
MakefileREADME.mdcmd/plugin-backend.goconfig/services.patch.jsonpkg/server/plugin_handler.gopkg/server/plugin_handler_test.gopkg/server/server.goweb/cypress/component/SummaryCard.cy.tsxweb/cypress/e2e/monitoring/00.bvt_admin.cy.tsweb/cypress/e2e/monitoring/regression/04.reg_services_admin.cy.tsweb/cypress/support/monitoring/04.reg_services.cy.tsweb/cypress/views/services-page.tsweb/locales/en/plugin__monitoring-plugin.jsonweb/package.jsonweb/src/features/alerts/components/AlertUtils.tsxweb/src/features/alerts/pages/AlertRulesDetailsPage.tsxweb/src/features/alerts/pages/AlertsDetailsPage.tsxweb/src/features/alerts/pages/alert-rules-page/AlertRulesPage.tsxweb/src/features/alerts/pages/alert-rules-page/filter-rules.tsweb/src/features/alerts/pages/alerts-page/AggregateAlertTableRow.tsxweb/src/features/alerts/pages/alerts-page/AlertTableRow.tsxweb/src/features/alerts/pages/alerts-page/AlertsPage.tsxweb/src/features/perses-dashboards/components/PersesWrapper.tsxweb/src/features/perses-dashboards/hooks/useEditableProjects.tsweb/src/features/perses-dashboards/hooks/usePerses.tsweb/src/features/perses-dashboards/utils/dashboard-api.tsweb/src/features/perses-dashboards/utils/migrate-api.tsweb/src/features/services/OWNERSweb/src/features/services/components/summary/AlertSummaryCards.tsxweb/src/features/services/components/summary/DashboardsSummaryCard.tsxweb/src/features/services/components/summary/MetricsSummaryCard.tsxweb/src/features/services/components/summary/ObservabilityStackSummary.tsxweb/src/features/services/components/summary/SummaryCard.tsxweb/src/features/services/components/summary/TargetsSummaryCard.tsxweb/src/features/services/components/summary/alert-summary-cards.spec.tsweb/src/features/services/components/summary/dashboards-summary-card.spec.tsweb/src/features/services/pages/ServicesPage.tsxweb/src/shared/constants/data-test.tsweb/src/shared/hooks/useAlerts.tsweb/src/shared/utils/alert-utils.spec.tsweb/src/shared/utils/alert-utils.tsweb/src/shared/utils/perses-client.ts
💤 Files with no reviewable changes (1)
- web/src/features/alerts/components/AlertUtils.tsx
🚧 Files skipped from review as they are similar to previous changes (34)
- web/package.json
- web/src/features/perses-dashboards/utils/dashboard-api.ts
- web/src/features/services/components/summary/ObservabilityStackSummary.tsx
- web/src/features/alerts/pages/alerts-page/AlertsPage.tsx
- web/src/shared/constants/data-test.ts
- pkg/server/plugin_handler.go
- web/src/features/perses-dashboards/components/PersesWrapper.tsx
- Makefile
- web/src/features/services/components/summary/SummaryCard.tsx
- web/src/features/perses-dashboards/hooks/usePerses.ts
- web/src/features/services/pages/ServicesPage.tsx
- web/src/features/alerts/pages/alert-rules-page/filter-rules.ts
- web/src/features/alerts/pages/alert-rules-page/AlertRulesPage.tsx
- pkg/server/server.go
- web/cypress/support/monitoring/04.reg_services.cy.ts
- web/src/features/perses-dashboards/hooks/useEditableProjects.ts
- web/src/shared/hooks/useAlerts.ts
- web/cypress/e2e/monitoring/00.bvt_admin.cy.ts
- config/services.patch.json
- web/src/features/perses-dashboards/utils/migrate-api.ts
- web/cypress/component/SummaryCard.cy.tsx
- cmd/plugin-backend.go
- web/src/features/alerts/pages/AlertRulesDetailsPage.tsx
- web/src/features/alerts/pages/AlertsDetailsPage.tsx
- web/src/features/alerts/pages/alerts-page/AggregateAlertTableRow.tsx
- web/locales/en/plugin__monitoring-plugin.json
- web/cypress/e2e/monitoring/regression/04.reg_services_admin.cy.ts
- web/src/shared/utils/alert-utils.ts
- web/src/features/alerts/pages/alerts-page/AlertTableRow.tsx
- web/src/features/services/OWNERS
- web/src/features/services/components/summary/MetricsSummaryCard.tsx
- README.md
- web/cypress/views/services-page.ts
- web/src/features/services/components/summary/TargetsSummaryCard.tsx
5596e73 to
1ff8473
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
web/src/features/services/components/summary/SummaryCard.tsx (1)
18-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExport
SummaryCardby name.Lines 18 and 64 use a default export. Export
SummaryCardfrom its declaration. Update consumers to use a named import.Proposed change
-const SummaryCard: FC<SummaryCardProps> = ({ count, title, url, cardId, loading, error }) => { +export const SummaryCard: FC<SummaryCardProps> = ({ count, title, url, cardId, loading, error }) => { @@ -export default SummaryCard;As per coding guidelines, “Define React components as functional components with explicit type annotations, preferably using
FC, and export them by name.”Also applies to: 64-64
🤖 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/features/services/components/summary/SummaryCard.tsx` at line 18, Export the SummaryCard component by name from its declaration instead of using a default export, and update every consumer of SummaryCard to import it by name. Preserve the existing FC<SummaryCardProps> annotation and component behavior.Source: Coding guidelines
web/src/features/alerts/pages/alerts-page/AlertsPage.tsx (1)
41-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a type-only import for
AggregatedAlertFilters.
AggregatedAlertFiltersis an interface and is used only in type positions. Move it to animport typedeclaration.Proposed fix
import { - AggregatedAlertFilters, AlertFilterOptions, filterAlerts, } from '`@/shared/utils/alert-utils`'; +import type { AggregatedAlertFilters } from '`@/shared/utils/alert-utils`';As per coding guidelines, “Use type-only imports (
import type) for symbols used only for type checking.”🤖 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/features/alerts/pages/alerts-page/AlertsPage.tsx` around lines 41 - 45, Update the imports from alert-utils so AggregatedAlertFilters is imported via a separate import type declaration, while AlertFilterOptions and filterAlerts remain in the value import.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@web/src/features/alerts/pages/alerts-page/AlertsPage.tsx`:
- Around line 41-45: Update the imports from alert-utils so
AggregatedAlertFilters is imported via a separate import type declaration, while
AlertFilterOptions and filterAlerts remain in the value import.
In `@web/src/features/services/components/summary/SummaryCard.tsx`:
- Line 18: Export the SummaryCard component by name from its declaration instead
of using a default export, and update every consumer of SummaryCard to import it
by name. Preserve the existing FC<SummaryCardProps> annotation and component
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 6b095b6d-1c28-468a-b852-b766e00b1a0c
📒 Files selected for processing (43)
MakefileREADME.mdcmd/plugin-backend.goconfig/services.patch.jsonpkg/server/plugin_handler.gopkg/server/plugin_handler_test.gopkg/server/server.goweb/cypress/component/SummaryCard.cy.tsxweb/cypress/e2e/monitoring/00.bvt_admin.cy.tsweb/cypress/e2e/monitoring/regression/04.reg_services_admin.cy.tsweb/cypress/support/monitoring/04.reg_services.cy.tsweb/cypress/views/services-page.tsweb/locales/en/plugin__monitoring-plugin.jsonweb/package.jsonweb/src/features/alerts/components/AlertUtils.tsxweb/src/features/alerts/pages/AlertRulesDetailsPage.tsxweb/src/features/alerts/pages/AlertsDetailsPage.tsxweb/src/features/alerts/pages/alert-rules-page/AlertRulesPage.tsxweb/src/features/alerts/pages/alert-rules-page/filter-rules.tsweb/src/features/alerts/pages/alerts-page/AggregateAlertTableRow.tsxweb/src/features/alerts/pages/alerts-page/AlertTableRow.tsxweb/src/features/alerts/pages/alerts-page/AlertsPage.tsxweb/src/features/perses-dashboards/components/PersesWrapper.tsxweb/src/features/perses-dashboards/hooks/useEditableProjects.tsweb/src/features/perses-dashboards/hooks/usePerses.tsweb/src/features/perses-dashboards/utils/dashboard-api.tsweb/src/features/perses-dashboards/utils/migrate-api.tsweb/src/features/services/OWNERSweb/src/features/services/components/summary/AlertSummaryCards.tsxweb/src/features/services/components/summary/DashboardsSummaryCard.tsxweb/src/features/services/components/summary/MetricsSummaryCard.tsxweb/src/features/services/components/summary/ObservabilityStackSummary.tsxweb/src/features/services/components/summary/SummaryCard.tsxweb/src/features/services/components/summary/TargetsSummaryCard.tsxweb/src/features/services/components/summary/alert-summary-cards.spec.tsweb/src/features/services/components/summary/dashboards-summary-card.spec.tsweb/src/features/services/pages/ServicesPage.tsxweb/src/shared/constants/data-test.tsweb/src/shared/hooks/useAlerts.tsweb/src/shared/utils/alert-utils.spec.tsweb/src/shared/utils/alert-utils.tsweb/src/shared/utils/perses-client.spec.tsweb/src/shared/utils/perses-client.ts
💤 Files with no reviewable changes (1)
- web/src/features/alerts/components/AlertUtils.tsx
🚧 Files skipped from review as they are similar to previous changes (37)
- web/package.json
- web/src/features/perses-dashboards/utils/dashboard-api.ts
- cmd/plugin-backend.go
- web/cypress/e2e/monitoring/regression/04.reg_services_admin.cy.ts
- web/cypress/e2e/monitoring/00.bvt_admin.cy.ts
- web/src/features/perses-dashboards/hooks/useEditableProjects.ts
- web/src/features/services/OWNERS
- web/src/shared/utils/alert-utils.spec.ts
- pkg/server/plugin_handler.go
- web/src/features/alerts/pages/alert-rules-page/AlertRulesPage.tsx
- web/src/features/alerts/pages/AlertsDetailsPage.tsx
- web/src/features/services/components/summary/TargetsSummaryCard.tsx
- web/src/features/perses-dashboards/hooks/usePerses.ts
- web/src/features/perses-dashboards/utils/migrate-api.ts
- web/src/features/alerts/pages/alerts-page/AggregateAlertTableRow.tsx
- web/src/shared/hooks/useAlerts.ts
- web/src/features/alerts/pages/alerts-page/AlertTableRow.tsx
- web/src/shared/utils/alert-utils.ts
- web/src/features/services/components/summary/dashboards-summary-card.spec.ts
- web/src/features/perses-dashboards/components/PersesWrapper.tsx
- Makefile
- README.md
- web/src/features/services/components/summary/alert-summary-cards.spec.ts
- pkg/server/server.go
- web/src/features/alerts/pages/AlertRulesDetailsPage.tsx
- web/src/features/services/components/summary/MetricsSummaryCard.tsx
- web/src/shared/constants/data-test.ts
- web/src/features/services/components/summary/ObservabilityStackSummary.tsx
- web/locales/en/plugin__monitoring-plugin.json
- web/src/shared/utils/perses-client.ts
- web/src/features/services/components/summary/AlertSummaryCards.tsx
- web/src/features/alerts/pages/alert-rules-page/filter-rules.ts
- config/services.patch.json
- web/cypress/component/SummaryCard.cy.tsx
- web/cypress/support/monitoring/04.reg_services.cy.ts
- web/src/features/services/pages/ServicesPage.tsx
- web/src/features/services/components/summary/DashboardsSummaryCard.tsx
1ff8473 to
46cf172
Compare
There was a problem hiding this comment.
not sure why this is needed? can you clarify?
There was a problem hiding this comment.
This is following the pattern for the other nav sections for running a suite of regression tests.
|
/cc @fkargbo |
|
@jeff-phillips-18: GitHub didn't allow me to request PR reviews from the following users: fkargbo. Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs. 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. |
|
/retest-required |
1 similar comment
|
/retest-required |
|
This is a pretty large PR, I will try to review today or tomorrow |
46cf172 to
88174af
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
88174af to
09b728d
Compare
|
@jeff-phillips-18: The following tests 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. |
4 participants
Towards OU-1212
This PR addresses a portion of the new landing page. The installed and recommended operators and add-ons sections will be added in a follow up.
Summary
/monitoring/servicesbehind aservicesfeature flagservicesfeature flag end-to-end: Go backend enum, manifest patch, Makefile, CLI help text, and READMEScreen shots
Refactoring
alertSource,alertingRuleSource,filterAlerts,AlertFilterOptions, andAggregatedAlertFiltersfromfeatures/alerts/toshared/utils/alert-utils.tsfor reuse by the services pageperses-client.tsfromfeatures/perses-dashboards/utils/toshared/utils/so the dashboards summary card can import itTest coverage
SummaryCard(loading, error, and count states + navigation)Test plan
servicesfeature flag and verify the "Observability services" nav item appears under Observe in admin perspectiveSummary by CodeRabbit