CONSOLE-5091: Add bulk selection and schedulable actions to Nodes page#16203
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@rhamilto: This pull request references CONSOLE-5091 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 spike to target the "4.22.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. |
rhamilto
changed the title
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
openshift-ci
Bot
added
component/core
|
@rhamilto: This pull request references CONSOLE-5091 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 spike to target the "4.22.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. |
|
@rhamilto: This pull request references CONSOLE-5091 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 spike to target the "4.22.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. |
|
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:
WalkthroughAdds checkbox-driven bulk selection primitives, wires selection through ConsoleDataView (header checkbox, banner, visibleItems), integrates selection into the Nodes page, and implements bulk node scheduling actions with responsive dropdown, modal, docs, tests, locales, and minor fixes. ChangesBulk selection and node scheduling
Sequence DiagramsequenceDiagram
participant User
participant ConsoleDataView
participant useConsoleDataViewData
participant useDataViewSelection
participant NodeRowRenderer
participant useCustomNodeActions
participant nodeSchedulingActions
User->>ConsoleDataView: render nodes table
ConsoleDataView->>useConsoleDataViewData: pass selection
useConsoleDataViewData-->>ConsoleDataView: returns visibleItems, columns, rows
ConsoleDataView->>NodeRowRenderer: render row (includes selection cell)
NodeRowRenderer->>useDataViewSelection: onSelect(itemId, isSelecting)
useDataViewSelection-->>ConsoleDataView: selectedItems updated
ConsoleDataView->>useCustomNodeActions: provide selectedNodes
User->>useCustomNodeActions: trigger "Mark schedulable"
useCustomNodeActions->>nodeSchedulingActions: call markNodesSchedulable(selected)
nodeSchedulingActions-->>useCustomNodeActions: resolves when requests complete
useCustomNodeActions->>ConsoleDataView: onComplete -> clear selection
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 7
🧹 Nitpick comments (7)
frontend/packages/operator-lifecycle-manager/integration-tests-cypress/tests/operator-install-global.cy.ts (1)
21-34: Consider extracting shared cleanup helper to reduce duplication.This
cleanupOperatorResourcesimplementation is nearly identical to the one inoperator-install-single-namespace.cy.ts. The only difference is namespace handling (hardcodedGlobalInstalledNamespacevs. parameterizednamespace).When these tests are re-enabled, consider extracting this to
operator.view.tsor a shared test utilities module to avoid maintaining duplicate code:// In operator.view.ts or a shared helper export const cleanupOperatorResources = (packageName: string, namespace: string) => { ... }Not blocking since the tests are skipped, but worth addressing when re-enabling.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/operator-lifecycle-manager/integration-tests-cypress/tests/operator-install-global.cy.ts` around lines 21 - 34, The cleanupOperatorResources function is duplicated between this file and operator-install-single-namespace.cy.ts; refactor by extracting a shared helper (e.g., export const cleanupOperatorResources(packageName: string, namespace: string)) into a common module such as operator.view.ts or a test utilities file, then replace the local cleanupOperatorResources with calls to that shared function using operatorPackageName and GlobalInstalledNamespace here (and the parameterized namespace in the other test) to remove duplication and centralize the oc delete logic.frontend/packages/operator-lifecycle-manager/integration-tests-cypress/tests/operator-uninstall.cy.ts (1)
25-25: Consider adding a tracking comment for skipped tests.Skipping the entire uninstall test suite via
xdescriberemoves coverage for critical operator uninstall error scenarios (operand load failures, deletion failures, successful cleanup). While I understand flaky tests need to be disabled to unblock CI, it's good practice to add a// TODO(JIRA-XXXX): Re-enable when flakiness is resolvedcomment so these don't become permanently forgotten.Without tracking, these tend to rot indefinitely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/operator-lifecycle-manager/integration-tests-cypress/tests/operator-uninstall.cy.ts` at line 25, The test suite is being fully skipped with xdescribe(`Testing uninstall of ${testOperator.name} Operator`), which can cause the uninstall tests to be forgotten; add a clear tracking comment directly above the xdescribe line such as // TODO(JIRA-XXXX): Re-enable uninstall tests for ${testOperator.name} when flakiness is fixed — include brief reason (flaky operand/delete flows), owner or team and a link to the ticket; optionally convert xdescribe to describe.skip to make the skip intent explicit while preserving the same behavior.frontend/packages/console-app/src/components/nodes/NodeTerminal.tsx (1)
155-155: Minor: Optional chaining is now redundant after the guard.Since we return early on line 151-153 when
!pod, the optional chaining onpod?.status?.phaseis technically unnecessary. Not a blocker—just a tiny cleanup opportunity if you're in this area again.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/console-app/src/components/nodes/NodeTerminal.tsx` at line 155, The switch uses redundant optional chaining after an earlier guard that returns when pod is falsy; update the switch expression from using pod?.status?.phase to pod.status.phase (or otherwise drop the leading ? on status) in NodeTerminal's render logic so it relies on the existing early return for pod, keeping the rest of the switch cases the same.frontend/public/components/storage-class-form.tsx (1)
736-738: Tighten theresourcescontract and pass the hook result through directly.This form only consumes
scandcsilist results, but the new prop type allows arbitrary keys and singular-or-list payloads, which keeps the list assumption hidden behind the casts at Lines 254-255. Lines 793-804 then rebuild fresh result objects every render. Typing those two keys explicitly and passingwatchedResourcesthrough directly will make the refactor easier to maintain and reduce extra prop churn in the connected child. As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."Also applies to: 791-804
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/public/components/storage-class-form.tsx` around lines 736 - 738, The resources prop should be tightened to explicitly declare the two keys used ("sc" and "csi") with their exact payload shapes instead of a loose index signature and union types; update the prop/type where resources?: { [key: string]: WatchK8sResultsObject<...> } to resources?: { sc: WatchK8sResultsObject<K8sResourceCommon[]>; csi: WatchK8sResultsObject<K8sResourceCommon[]> } (or singular types if one is singular) so callers stop casting at the usage site in the component (see the code that casts at the former Lines 254-255) and then stop rebuilding result objects on each render (remove the reconstruction in the block around Lines 793-804) by passing the hook return (watchedResources) through directly to the child component that expects {sc, csi}; ensure the prop name and types match the hook that produces watchedResources to keep types consistent.frontend/public/components/instantiate-template.tsx (1)
406-415: Add type parameter for type safety.
useK8sWatchResourceis called without a type parameter, sotemplateis typed asunknown. This could lead to type errors downstream or require unsafe casts. Consider adding the generic type.🔧 Add type parameter
- const [template, loaded, loadError] = useK8sWatchResource( + const [template, loaded, loadError] = useK8sWatchResource<TemplateKind>( templateName && templateNamespace🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/public/components/instantiate-template.tsx` around lines 406 - 415, The call to useK8sWatchResource lacks a generic type so template is inferred as unknown; update the call site to provide the appropriate resource type (e.g., the Template/TemplateKind interface you use in the codebase) as the generic parameter to useK8sWatchResource so that the returned template variable has the correct typed shape; modify the expression where useK8sWatchResource is invoked (referencing useK8sWatchResource and the template variable) to include the type parameter matching your Template type.frontend/public/components/factory/details.tsx (2)
110-114: Verify objData shape when props.obj is nil.When
props.objis nil,watchedResources.objis used. This returns{ data, loaded, loadError }, but there's no explicit handling if the watch fails or returns unexpected data before it's passed downstream. Ensure consumers handle potential undefined/nulldatagracefully.🔍 Safer objData derivation
- const objData = _.isNil(props.obj) ? watchedResources.obj : props.obj; + const objData = _.isNil(props.obj) + ? watchedResources.obj ?? { data: undefined, loaded: false, loadError: undefined } + : props.obj;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/public/components/factory/details.tsx` around lines 110 - 114, The derivation of objData blindly uses watchedResources.obj when props.obj is nil but does not validate the shape (data/loaded/loadError); update the logic around useK8sWatchResources and the objData assignment so you explicitly check watchedResources.obj and its properties (e.g., watchedResources.obj?.data, watchedResources.obj?.loaded, watchedResources.obj?.loadError), default to a safe value (null or an empty object) when data is undefined or a loadError exists, and ensure downstream consumers receive that safe value rather than an unexpected shape; locate this in the code that sets watchedResources and the objData constant to add these guards.
127-149: Spreading watchedResources may pass unintended props.Spreading
{...watchedResources}intoConnectedPageHeadingpasses all watched resources as props. If new resources are added to the watch config, they'll automatically be passed to the heading component. This coupling could lead to unexpected behavior. Consider being explicit about which resources are passed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/public/components/factory/details.tsx` around lines 127 - 149, Spreading {...watchedResources} into ConnectedPageHeading risks leaking all watched keys into the heading; instead explicitly pass only the required fields (e.g., pass watchedResources.items or individual props like watchedResources.foo, watchedResources.bar) to ConnectedPageHeading so future additions to watchedResources won’t become unintended props. Locate the ConnectedPageHeading usage in details.tsx and replace the spread of watchedResources with explicit prop names (or wrap watchedResources into a single explicit prop name such as watchedResources={watchedResources} or watchedResourceList={watchedResources.items}) ensuring consumers (ConnectedPageHeading) receive only the intended data.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/packages/integration-tests-cypress/views/details-page.ts`:
- Around line 16-19: The isLoaded() readiness check is too weak; after ensuring
the skeleton ('skeleton-detail-view') is gone, update the assertion on
'[data-test-id="resource-title"]' to first assert visibility
(should('be.visible')) and then assert trimmed, non-whitespace content by
invoking the element text and checking it is not empty (e.g., invoke('text') and
assert text.trim() contains non-whitespace or matches /\S/); make this change
inside the isLoaded function to replace the current .should('not.be.empty')
check.
In `@frontend/public/components/command-line-tools.tsx`:
- Around line 106-117: The component currently only shows LoadingBox when
!loaded and !loadError, but when loadError is truthy it still falls through to
render CommandLineTools; update the conditional to explicitly handle the error
case by returning an error/status UI (e.g., render StatusBox or a dedicated
error message) when loadError is set, ensure you pass the same props or error
details to CommandLineTools if you still want it mounted, and add any required
import for StatusBox; refer to the symbols loaded, loadError, CommandLineTools,
LoadingBox, and StatusBox to locate where to add the new branch.
In `@frontend/public/components/container.tsx`:
- Around line 513-519: ConnectedPageHeading is being passed loadError but
HorizontalNav is not; update the HorizontalNav invocation to include the same
loadError so its StatusBox can render errors consistently. Specifically, in the
component where ConnectedPageHeading and HorizontalNav are rendered, add
loadError: props.loadError to the obj prop passed to HorizontalNav (the
HorizontalNav call that currently passes obj={{ data: props.pod, loaded:
props.loaded }}), ensuring ContainerDetailsList, props.pod, props.loaded and
props.loadError are all forwarded consistently.
In `@frontend/public/components/create-yaml.tsx`:
- Around line 128-134: The current error handling in the create-yaml component
returns ErrorPage404 for any loadError, which misrepresents errors like 403 or
network failures; update the render logic around the loaded and loadError checks
(variables loaded, loadError) to inspect loadError (e.g., status or
error.type/message) and render an appropriate component instead of always
ErrorPage404—either map specific statuses to dedicated components (e.g.,
ForbiddenPage for 403) or introduce a GenericError component that displays the
actual error.message/status so users see the real failure instead of a 404.
In `@frontend/public/components/factory/list-page.tsx`:
- Around line 533-565: The watchResources builder omits resource-level filters
so ListPageProps.filters are not applied to useK8sWatchResources; update the
reduce that creates watchResources (based on k8sResources) to copy r.filters
into each watch config (e.g., include filters: r.filters) so the produced record
passed to useK8sWatchResources preserves resources[].filters and downstream list
rendering and row counts are filtered correctly; modify the reduce in the
watchResources useMemo that constructs acc[key] to include the filters property
from r.
- Around line 176-177: The bug is that FireMan is being given the raw action
creator filterList (propsFilterList) but applyFilterInternal calls it and
ignores the returned action, so dispatch never occurs; update the place where
FireMan is constructed (and any other occurrences around lines with reduxIDs/
filterList and where applyFilterInternal is used) to pass a bound dispatcher
function instead of the raw creator—i.e., wrap propsFilterList with dispatch (or
call props.dispatch(propsFilterList(...))) so applyFilter / applyFilterInternal
receive and return a dispatched action; ensure functions named FireMan,
applyFilterInternal, filterList (propsFilterList) and applyFilter are updated
accordingly so URL-applied filters and applyFilter consumers receive the
dispatched result.
- Around line 108-111: The current selection of resourceData uses
watchedResources ?? resources which picks an empty object from
useK8sWatchResources on first render and breaks callers; update the logic in the
list-page component to only use watchedResources when it actually has entries
(e.g. check watchedResources is truthy and Object.keys(watchedResources).length
> 0) otherwise fall back to resources, and keep the subsequent use of
flatten(resourceData) unchanged (references: watchedResources, resources,
resourceData, flatten, useK8sWatchResources).
---
Nitpick comments:
In `@frontend/packages/console-app/src/components/nodes/NodeTerminal.tsx`:
- Line 155: The switch uses redundant optional chaining after an earlier guard
that returns when pod is falsy; update the switch expression from using
pod?.status?.phase to pod.status.phase (or otherwise drop the leading ? on
status) in NodeTerminal's render logic so it relies on the existing early return
for pod, keeping the rest of the switch cases the same.
In
`@frontend/packages/operator-lifecycle-manager/integration-tests-cypress/tests/operator-install-global.cy.ts`:
- Around line 21-34: The cleanupOperatorResources function is duplicated between
this file and operator-install-single-namespace.cy.ts; refactor by extracting a
shared helper (e.g., export const cleanupOperatorResources(packageName: string,
namespace: string)) into a common module such as operator.view.ts or a test
utilities file, then replace the local cleanupOperatorResources with calls to
that shared function using operatorPackageName and GlobalInstalledNamespace here
(and the parameterized namespace in the other test) to remove duplication and
centralize the oc delete logic.
In
`@frontend/packages/operator-lifecycle-manager/integration-tests-cypress/tests/operator-uninstall.cy.ts`:
- Line 25: The test suite is being fully skipped with xdescribe(`Testing
uninstall of ${testOperator.name} Operator`), which can cause the uninstall
tests to be forgotten; add a clear tracking comment directly above the xdescribe
line such as // TODO(JIRA-XXXX): Re-enable uninstall tests for
${testOperator.name} when flakiness is fixed — include brief reason (flaky
operand/delete flows), owner or team and a link to the ticket; optionally
convert xdescribe to describe.skip to make the skip intent explicit while
preserving the same behavior.
In `@frontend/public/components/factory/details.tsx`:
- Around line 110-114: The derivation of objData blindly uses
watchedResources.obj when props.obj is nil but does not validate the shape
(data/loaded/loadError); update the logic around useK8sWatchResources and the
objData assignment so you explicitly check watchedResources.obj and its
properties (e.g., watchedResources.obj?.data, watchedResources.obj?.loaded,
watchedResources.obj?.loadError), default to a safe value (null or an empty
object) when data is undefined or a loadError exists, and ensure downstream
consumers receive that safe value rather than an unexpected shape; locate this
in the code that sets watchedResources and the objData constant to add these
guards.
- Around line 127-149: Spreading {...watchedResources} into ConnectedPageHeading
risks leaking all watched keys into the heading; instead explicitly pass only
the required fields (e.g., pass watchedResources.items or individual props like
watchedResources.foo, watchedResources.bar) to ConnectedPageHeading so future
additions to watchedResources won’t become unintended props. Locate the
ConnectedPageHeading usage in details.tsx and replace the spread of
watchedResources with explicit prop names (or wrap watchedResources into a
single explicit prop name such as watchedResources={watchedResources} or
watchedResourceList={watchedResources.items}) ensuring consumers
(ConnectedPageHeading) receive only the intended data.
In `@frontend/public/components/instantiate-template.tsx`:
- Around line 406-415: The call to useK8sWatchResource lacks a generic type so
template is inferred as unknown; update the call site to provide the appropriate
resource type (e.g., the Template/TemplateKind interface you use in the
codebase) as the generic parameter to useK8sWatchResource so that the returned
template variable has the correct typed shape; modify the expression where
useK8sWatchResource is invoked (referencing useK8sWatchResource and the template
variable) to include the type parameter matching your Template type.
In `@frontend/public/components/storage-class-form.tsx`:
- Around line 736-738: The resources prop should be tightened to explicitly
declare the two keys used ("sc" and "csi") with their exact payload shapes
instead of a loose index signature and union types; update the prop/type where
resources?: { [key: string]: WatchK8sResultsObject<...> } to resources?: { sc:
WatchK8sResultsObject<K8sResourceCommon[]>; csi:
WatchK8sResultsObject<K8sResourceCommon[]> } (or singular types if one is
singular) so callers stop casting at the usage site in the component (see the
code that casts at the former Lines 254-255) and then stop rebuilding result
objects on each render (remove the reconstruction in the block around Lines
793-804) by passing the hook return (watchedResources) through directly to the
child component that expects {sc, csi}; ensure the prop name and types match the
hook that produces watchedResources to keep types consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 2c3fbeee-cf14-45d1-b293-e64867a50e9e
📒 Files selected for processing (53)
frontend/packages/console-app/locales/en/console-app.jsonfrontend/packages/console-app/src/actions/hooks/useBindingActions.tsfrontend/packages/console-app/src/components/data-view/ConsoleDataView.tsxfrontend/packages/console-app/src/components/data-view/ConsoleDataViewBulkSelect.tsxfrontend/packages/console-app/src/components/data-view/dataViewSelectionHelpers.tsfrontend/packages/console-app/src/components/data-view/useDataViewSelection.tsfrontend/packages/console-app/src/components/nodes/NodeTerminal.tsxfrontend/packages/console-app/src/components/nodes/NodesPage.tsxfrontend/packages/console-app/src/components/nodes/useBulkNodeActions.tsxfrontend/packages/console-dynamic-plugin-sdk/CHANGELOG-webpack.mdfrontend/packages/console-dynamic-plugin-sdk/src/api/internal-types.tsfrontend/packages/console-dynamic-plugin-sdk/src/shared-modules/shared-modules-meta.tsfrontend/packages/console-dynamic-plugin-sdk/src/webpack/ConsoleRemotePlugin.tsfrontend/packages/helm-plugin/src/components/details-page/resources/__tests__/HelmReleaseResources.spec.tsxfrontend/packages/integration-tests-cypress/views/details-page.tsfrontend/packages/operator-lifecycle-manager-v1/locales/en/olm-v1.jsonfrontend/packages/operator-lifecycle-manager-v1/src/components/cluster-extension/ServiceAccountDropdown.tsxfrontend/packages/operator-lifecycle-manager/integration-tests-cypress/tests/deprecated-operator-warnings.cy.tsfrontend/packages/operator-lifecycle-manager/integration-tests-cypress/tests/operator-install-global.cy.tsfrontend/packages/operator-lifecycle-manager/integration-tests-cypress/tests/operator-install-single-namespace.cy.tsfrontend/packages/operator-lifecycle-manager/integration-tests-cypress/tests/operator-uninstall.cy.tsfrontend/packages/operator-lifecycle-manager/integration-tests-cypress/views/operator.view.tsfrontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-subscribe.tsxfrontend/public/components/RBAC/bindings.tsxfrontend/public/components/__tests__/container.spec.tsxfrontend/public/components/cluster-settings/cluster-settings.tsxfrontend/public/components/command-line-tools.tsxfrontend/public/components/console-notifier.tsxfrontend/public/components/container.tsxfrontend/public/components/control-plane-machine-set.tsxfrontend/public/components/create-yaml.tsxfrontend/public/components/cron-job.tsxfrontend/public/components/edit-yaml.tsxfrontend/public/components/factory/__tests__/details.spec.tsxfrontend/public/components/factory/__tests__/list-page.spec.tsxfrontend/public/components/factory/details.tsxfrontend/public/components/factory/list-page.tsxfrontend/public/components/instantiate-template.tsxfrontend/public/components/machine-autoscaler.tsxfrontend/public/components/machine-config-pool.tsxfrontend/public/components/machine-config.tsxfrontend/public/components/machine-health-check.tsxfrontend/public/components/machine-set.tsxfrontend/public/components/machine.tsxfrontend/public/components/monitoring/alertmanager/alertmanager-config.tsxfrontend/public/components/monitoring/alertmanager/alertmanager-yaml-editor.tsxfrontend/public/components/monitoring/receiver-forms/alert-manager-receiver-forms.tsxfrontend/public/components/namespace-bar.tsxfrontend/public/components/storage-class-form.tsxfrontend/public/components/utils/horizontal-nav.tsxfrontend/public/components/utils/inject.tsfrontend/public/components/utils/list-dropdown.tsxfrontend/public/locales/en/public.json
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
frontend/packages/operator-lifecycle-manager-v1/src/components/cluster-extension/ServiceAccountDropdown.tsxfrontend/packages/operator-lifecycle-manager/integration-tests-cypress/tests/operator-uninstall.cy.tsfrontend/packages/integration-tests-cypress/views/details-page.tsfrontend/packages/operator-lifecycle-manager-v1/locales/en/olm-v1.jsonfrontend/packages/console-app/src/components/nodes/NodeTerminal.tsxfrontend/public/components/utils/inject.tsfrontend/public/components/control-plane-machine-set.tsxfrontend/packages/helm-plugin/src/components/details-page/resources/__tests__/HelmReleaseResources.spec.tsxfrontend/packages/operator-lifecycle-manager/integration-tests-cypress/tests/operator-install-global.cy.tsfrontend/packages/operator-lifecycle-manager/integration-tests-cypress/tests/operator-install-single-namespace.cy.tsfrontend/packages/console-dynamic-plugin-sdk/src/webpack/ConsoleRemotePlugin.tsfrontend/packages/console-dynamic-plugin-sdk/src/shared-modules/shared-modules-meta.tsfrontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-subscribe.tsxfrontend/public/components/factory/__tests__/list-page.spec.tsxfrontend/public/components/monitoring/alertmanager/alertmanager-yaml-editor.tsxfrontend/public/components/monitoring/receiver-forms/alert-manager-receiver-forms.tsxfrontend/public/components/machine-autoscaler.tsxfrontend/packages/console-dynamic-plugin-sdk/src/api/internal-types.tsfrontend/public/components/create-yaml.tsxfrontend/packages/console-app/src/components/data-view/ConsoleDataViewBulkSelect.tsxfrontend/public/components/__tests__/container.spec.tsxfrontend/public/components/machine.tsxfrontend/public/components/monitoring/alertmanager/alertmanager-config.tsxfrontend/public/components/machine-set.tsxfrontend/public/components/machine-health-check.tsxfrontend/public/components/container.tsxfrontend/public/components/machine-config-pool.tsxfrontend/public/components/machine-config.tsxfrontend/packages/console-app/src/components/data-view/useDataViewSelection.tsfrontend/public/components/cluster-settings/cluster-settings.tsxfrontend/packages/console-app/src/components/nodes/useBulkNodeActions.tsxfrontend/packages/operator-lifecycle-manager/integration-tests-cypress/tests/deprecated-operator-warnings.cy.tsfrontend/packages/console-app/src/actions/hooks/useBindingActions.tsfrontend/public/components/instantiate-template.tsxfrontend/public/components/namespace-bar.tsxfrontend/public/components/utils/list-dropdown.tsxfrontend/packages/console-dynamic-plugin-sdk/CHANGELOG-webpack.mdfrontend/public/locales/en/public.jsonfrontend/public/components/console-notifier.tsxfrontend/public/components/utils/horizontal-nav.tsxfrontend/public/components/factory/__tests__/details.spec.tsxfrontend/public/components/command-line-tools.tsxfrontend/packages/console-app/src/components/data-view/dataViewSelectionHelpers.tsfrontend/public/components/factory/details.tsxfrontend/packages/console-app/src/components/nodes/NodesPage.tsxfrontend/public/components/edit-yaml.tsxfrontend/packages/operator-lifecycle-manager/integration-tests-cypress/views/operator.view.tsfrontend/public/components/storage-class-form.tsxfrontend/public/components/cron-job.tsxfrontend/packages/console-app/src/components/data-view/ConsoleDataView.tsxfrontend/public/components/factory/list-page.tsxfrontend/packages/console-app/locales/en/console-app.jsonfrontend/public/components/RBAC/bindings.tsx
🪛 LanguageTool
frontend/packages/console-dynamic-plugin-sdk/CHANGELOG-webpack.md
[grammar] ~18-~18: Use a hyphen to join words.
Context: ...warnings for usage of deprecated Console provided shared modules ([CONSOLE-5135],...
(QB_NEW_EN_HYPHEN)
🔇 Additional comments (62)
frontend/packages/operator-lifecycle-manager/integration-tests-cypress/views/operator.view.ts (1)
94-94: Nice reliability improvement in test synchronization.Switching from a fixed wait to a timed row lookup makes this flow less flaky and faster under normal conditions.
frontend/public/components/utils/inject.ts (1)
12-15: Good defensive change—child props now take precedence.This is a sensible fix: explicit props on children should not be clobbered by injected props. The logic using
_.omit(safeProps, Object.keys(c.props))is clean and the inline comment clearly documents the intent.Worth noting this is a semantic behavioral change (previously injected props would override). If any existing consumers relied on the old override behavior, they'd need adjustment—but the new behavior is more predictable and aligns with typical React composition patterns.
frontend/packages/operator-lifecycle-manager/integration-tests-cypress/tests/deprecated-operator-warnings.cy.ts (1)
177-177: Selective test skipping is reasonable; consider documenting the reason.Good approach to only skip the tests that require actual operator installation (
create(testDeprecatedSubscription)and subsequent approval workflow) while keeping the mock-based tests enabled. This preserves partial coverage for the deprecation warning UI.However, these three tests cover the "installed operator" deprecation badge workflow which is user-facing. A brief comment explaining the flakiness root cause (e.g., timing issues with InstallPlan approval) would help future maintainers know when it's safe to re-enable.
Also applies to: 206-206, 229-229
frontend/packages/operator-lifecycle-manager/integration-tests-cypress/tests/operator-install-single-namespace.cy.ts (2)
21-36: Cleanup helper looks solid; good defensive pattern.The
cleanupOperatorResourceshelper using OLM label selectors (operators.coreos.com/${operatorPackageName}.${namespace}) is the correct approach for cleaning up operator-related resources. Using--ignore-not-foundandfailOnNonZeroExit: falseensures the cleanup is idempotent and won't fail the test setup if resources don't exist.The 2-minute timeout per deletion command is generous but appropriate for resource cleanup that may need to wait for finalizers.
38-42: Consider cleanup ordering inbefore()hook.Currently the sequence is:
cy.createProjectWithCLI(testName)(line 41)cleanupOperatorResources(testName)(line 42)This works for cleaning up stale resources from previous failed runs, but since the suite is skipped (
xdescribe), this is moot for now. When re-enabled, verify this order handles the case where a previous run left resources in a partially-created state.frontend/packages/operator-lifecycle-manager/integration-tests-cypress/tests/operator-install-global.cy.ts (1)
47-49: Good addition ofafter()cleanup hook.Adding the
after()hook ensures cleanup runs even if tests fail partway through. This is a reliability improvement over the previous state where cleanup may have only occurred viaoperator.uninstall()within the test itself.frontend/packages/console-dynamic-plugin-sdk/CHANGELOG-webpack.md (1)
18-18: Changelog entry and references are consistent.The new item and added references are aligned and traceable.
Also applies to: 113-113, 146-147
frontend/packages/console-dynamic-plugin-sdk/src/webpack/ConsoleRemotePlugin.ts (2)
124-139: Deprecation warning collection logic looks solid.This is scoped well and only emits warnings for deprecated shared modules that are actually present in plugin dependencies.
476-479: Good choice to surface deprecations as compilation warnings.This keeps builds non-blocking while still making deprecated shared-module usage visible.
frontend/packages/console-dynamic-plugin-sdk/src/shared-modules/shared-modules-meta.ts (1)
20-21: Metadata extension is clean and backward-compatible.Adding
deprecatedwith explicit defaults and targeted module annotations is a maintainable way to drive the new warning path.Also applies to: 56-57, 68-74
frontend/packages/console-app/locales/en/console-app.json (1)
353-356: LGTM! Well-structured i18n strings for bulk actions.The interpolation variables (
{{count}},{{failureCount}},{{totalCount}}) are correctly applied and follow i18next conventions. Good job providing granular failure feedback messages.frontend/packages/console-app/src/components/nodes/NodeTerminal.tsx (1)
147-153: LGTM! Improved separation of loading vs. missing pod states.The refactored logic correctly distinguishes between "still loading" and "loaded but pod not found", providing clearer feedback when a debug pod gets deleted externally. Good defensive handling.
frontend/public/components/machine-health-check.tsx (2)
70-119: LGTM! Clean implementation of resizable columns.The hook correctly returns both
columnsandresetAllColumnWidths, appliesresizablePropsto data columns while preserving the actions column without resize capability. Memoization dependencies are correct.
128-143: LGTM! ConsoleDataView integration is correct.The destructuring and prop passing for
isResizableandresetAllColumnWidthsfollows the established pattern across this PR.frontend/public/components/machine-config.tsx (2)
196-263: LGTM! Consistent resizable column implementation.The pattern matches the other machine-related list components. Column configuration, memoization, and hook return shape are all correct.
265-284: LGTM! List component integration is correct.frontend/public/components/machine-autoscaler.tsx (2)
104-172: LGTM! Resizable columns correctly configured.Consistent implementation across name, namespace, scale target, min, and max columns. Actions column appropriately excluded from resizing.
174-198: LGTM! ConsoleDataView integration follows the established pattern.frontend/public/components/machine.tsx (2)
201-285: LGTM! Comprehensive resizable column support for all data columns.All seven data columns receive
resizablePropswhile the actions column correctly usesactionsCellPropswithout resize. Memoization is properly configured.
287-306: LGTM! List component correctly wired to ConsoleDataView.frontend/public/components/control-plane-machine-set.tsx (2)
206-274: LGTM! Resizable columns correctly implemented for ControlPlaneMachineSet.Hook return type, column configuration, and memoization all follow the established pattern.
336-360: LGTM! ConsoleDataView integration is correct.frontend/public/components/machine-config-pool.tsx (2)
307-364: LGTM! Resizable columns correctly implemented for MachineConfigPool.The pattern is consistent with the other machine-related list components in this PR.
409-436: LGTM! ConsoleDataView integration follows the established pattern.The
MachineConfigPoolsArePausedAlertpreceding the data view is a nice touch for UX.frontend/packages/console-dynamic-plugin-sdk/src/api/internal-types.ts (1)
361-373: LGTM! Clean API extension for selection support.The type definitions are well-structured with clear JSDoc comments. Using
Set<string>forselectedItemsprovides O(1) membership checks—good choice for potentially large node lists. The optionalonSelectAllkeeps the API flexible for consumers that don't need bulk selection.frontend/packages/console-app/src/components/data-view/ConsoleDataViewBulkSelect.tsx (1)
11-33: LGTM! Clean wrapper with proper PatternFly integration.The component correctly maps PatternFly's
BulkSelectValuevariants to the appropriate callbacks. The treatment ofpageandnonePagevariants as synonyms forall/noneis sensible given the current usage context where all filtered items are visible.frontend/packages/console-app/src/components/data-view/useDataViewSelection.ts (1)
39-89: LGTM! Well-designed selection hook with proper immutability.Good use of functional
setStateto avoid race conditions on rapid clicks. TheselectedItemsderivation correctly filters against currentdata, ensuring orphaned IDs (from deleted items) don't leak into the result array. ThefilterSelectablepredicate is a nice touch for excluding non-selectable rows like CSRs.frontend/packages/console-app/src/components/data-view/dataViewSelectionHelpers.ts (1)
59-78: LGTM! Helpers follow PatternFly's select cell API correctly.The
createSelectionCellfunction properly wires the checkbox selection. Note thatdisable(notdisabled) is intentional—it matches PatternFly's@patternfly/react-tableselect configuration interface.frontend/packages/console-app/src/components/nodes/useBulkNodeActions.tsx (1)
31-81: Good use ofPromise.allSettledfor partial-failure feedback.The implementation correctly reports how many nodes failed and only calls
onCompleteon full success. The localized error messages provide actionable feedback.One consideration: on partial failure, the selection remains intact (since
onCompleteisn't called). This means users can retry the action on the remaining nodes, which is sensible UX. If that's intentional, it's worth a brief comment for future maintainers.frontend/packages/console-app/src/components/nodes/NodesPage.tsx (2)
644-660: LGTM! Selection integration is well-structured.The
filterSelectablepredicate correctly excludes CSR resources from selection, making theNodeKind[]cast on line 658 type-safe. TheonComplete: clearSelectionpattern ensures UI consistency after successful bulk operations.
662-678: Good column layout hygiene—excluding internal columns from management.Filtering out the
selectand actions columns fromcolumnLayout.columnsprevents them from appearing in the column management modal. This is the correct approach for internal/structural columns.frontend/packages/console-app/src/components/data-view/ConsoleDataView.tsx (2)
105-120: Well-designed default bulk select with filter-awareness.The
defaultBulkSelectcorrectly derives counts fromfilteredData, ensuring "select all" respects active filters. The early return whenbulkSelectis explicitly provided allows consumers to override the default behavior when needed.
272-309: LGTM! Sticky column offsets properly account for selection column.The
SELECTION_COLUMN_WIDTH = '45px'constant is used asstickyLeftOffsetfor the name column, ensuring proper horizontal scroll behavior when both selection and name columns are sticky. The separation of*ColumnPropsvs*CellPropsprovides good composition flexibility.frontend/public/components/machine-set.tsx (2)
280-365: Good alignment with the resizableConsoleDataViewcontract.Returning
columnstogether withresetAllColumnWidths, and splitting the sticky header props tonameCellProps/actionsCellProps, keeps this list consistent with the updated table API.
433-435: Reset column widths affordance is correctly independent of column management UI.ConsoleDataView renders the reset button based on
isResizable && resetAllColumnWidths, nothideColumnManagement. Line 435 properly passesresetAllColumnWidthsto the component, so users can reset widths even with column management hidden. No recovery path issue here.> Likely an incorrect or invalid review comment.frontend/public/components/storage-class-form.tsx (1)
27-29: Hook-based watch migration looks solid.Keeping the watch setup in the wrapper and leaving
StorageClassFormInner's consumption path unchanged keeps the blast radius low.Also applies to: 785-788
frontend/public/components/monitoring/receiver-forms/alert-manager-receiver-forms.tsx (1)
566-579: Clean migration of loading state aggregation.The
combinedLoadedandcombinedLoadErrorlogic properly gates rendering until both the Secret watch and the Alertmanager globals fetch complete. Using&&for loaded and||for errors is the correct pattern for dependent data sources.frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-subscribe.tsx (2)
1275-1278: Loading aggregation logic is correct.The pattern
resourceValues.every((r) => r.loaded || r.loadError)ensures we wait for all resources to either successfully load or fail before proceeding. This is consistent with the loading semantics used elsewhere in this PR.
207-215: No null-safety issue — the code already handles undefined gracefully.The
installModesForfunction uses optional chaining (pkg?.status?.channels?...) with a fallback to[], so passingundefinedreturns an empty array rather than crashing. WhensupportedInstallModesForreduces over an empty array, it returns the initial value (InstallModeType.InstallModeTypeOwnNamespace). Additionally,StatusBoxgates rendering ofOperatorHubSubscribeFormuntil data is loaded, so line 209 won't execute without validpackageManifest.data[0].The proposed optional chaining fix is unnecessary.
> Likely an incorrect or invalid review comment.frontend/public/components/namespace-bar.tsx (1)
130-137: Good use of conditional watch resource.Passing
nullwhenhideProjectsis true correctly disables the Kubernetes watch, avoiding unnecessary API calls. The conditional structure is clean and follows established patterns.frontend/public/components/utils/list-dropdown.tsx (2)
205-244: Solid Firehose-to-hooks migration.The watch resource mapping correctly preserves the original Firehose resource config semantics (using
prop || kindas key). The loaded/error aggregation matches the established pattern.
269-272: Good fix for controlled component state sync.The
useEffectto synchronize internalselectedKeywithprops.selectedKeyensuresNsDropdownbehaves correctly as a controlled component when the parent updates the selected value externally.frontend/public/components/edit-yaml.tsx (2)
163-180: Clean conditional watch based on feature flag.The
useMemocorrectly gates the watch resource config onhasYAMLSampleFlag, preventing unnecessary API calls when the feature is disabled. Good pattern.
100-100: Thecreateprop requirement change is safe—all existing usages already provide it explicitly, either directly or through wrapper components likeDroppableEditYAML(which defaults tofalse). No callers rely on undefined behavior, so this type tightening poses no practical breaking change to the codebase.frontend/public/components/console-notifier.tsx (1)
28-47: Good defensive handling for notification loading failures.Returning
nullon load errors for notifications is appropriate—banner notifications are non-critical UI and shouldn't break the page. Theconsole.errorprovides debugging visibility without disrupting UX.frontend/public/components/RBAC/bindings.tsx (3)
363-380: Well-structured loading and error aggregation.The defensive check
Object.values(resources).some((r) => r.data)before flattening prevents iteration over undefined resources. The loaded/error aggregation follows the established PR pattern consistently.
799-826: Clean separation of loading states in BindingLoadingWrapper.The explicit render branches for loading, error, and empty object states provide clear control flow. The optional chaining at line 816 when building
fixedprevents crashes during the loading transition.
187-191: Good defensive coding for binding type detection.Adding optional chaining for
binding.roleRef?.nameandbinding.metadata?.namespaceprevents potential crashes when processing partially loaded or malformed bindings.frontend/packages/console-app/src/actions/hooks/useBindingActions.ts (1)
36-49: LGTM on null-safety improvements.The defensive approach here is solid—guarding
useK8sModelwithobj ? referenceFor(obj) : nulland using nullish coalescing for destructuring prevents runtime errors when bindings are still loading or undefined. The optional chaining throughout (model?.kind,subjects?.length) maintains consistency.frontend/packages/operator-lifecycle-manager-v1/src/components/cluster-extension/ServiceAccountDropdown.tsx (1)
69-69: Good i18n namespace scoping.Moving the translation key to
olm-v1~namespace is the right call—keeps package-specific strings owned by the package rather than polluting the sharedpublicnamespace.frontend/packages/operator-lifecycle-manager-v1/locales/en/olm-v1.json (1)
51-51: LGTM.The locale key aligns with the
ServiceAccountDropdowncomponent change.frontend/public/locales/en/public.json (1)
703-703: LGTM.Clear error message for template loading failures—good UX.
frontend/public/components/monitoring/alertmanager/alertmanager-config.tsx (1)
601-623: Clean Firehose→hook migration.The
useK8sWatchResourceapproach is the right direction—cleaner component composition, better TypeScript support, and avoids the render-prop pattern. The shape{ data: secret, loaded, loadError }passed toStatusBoxfollows the established console pattern.frontend/packages/helm-plugin/src/components/details-page/resources/__tests__/HelmReleaseResources.spec.tsx (1)
14-36: Good test coverage for the hook migration.Properly validates that:
- The
useK8sWatchResourceshook is invoked (confirming Firehose removal)- Empty state renders correctly when manifest has no resources
The mock shape
[null, true, null]foruseK8sWatchResourcecorrectly represents a loaded state with no data.frontend/public/components/factory/__tests__/list-page.spec.tsx (1)
151-174: Thorough test migration.Good approach—not just checking that the hook was called, but also validating the watch configuration structure. This catches regressions if someone breaks the resource configuration building logic.
frontend/public/components/monitoring/alertmanager/alertmanager-yaml-editor.tsx (1)
130-152: Consistent migration pattern.Mirrors the
alertmanager-config.tsxapproach exactly—good for maintainability. Both Alertmanager views now use the same hook-based resource loading pattern.The type assertion
secret as K8sResourceKindis acceptable given the explicit watch configuration, though in an ideal world the generic type parameter could be specified on the hook call.frontend/public/components/cluster-settings/cluster-settings.tsx (1)
1189-1249: Clean migration from Firehose to useK8sWatchResource.The conditional watch pattern using
nullforhasClusterAutoscaleris the correct approach. ThehorizontalNavPropsconstruction properly aggregates loading states and conditionally includes autoscaler data. This aligns well with the broader PR's hook-driven architecture.frontend/public/components/__tests__/container.spec.tsx (1)
26-34: Test mocks properly updated for hook migration.The mock setup correctly replaces the Firehose mock with
useK8sWatchResource, and tests are appropriately updated to passpodinstead ofobj.data. The strengthened assertion for "Waiting" appearing twice (line 76) improves test reliability.frontend/public/components/instantiate-template.tsx (1)
330-336: Good defensive check for missing template data.This guard catches edge cases where loading completes but data is missing (e.g., watch returns null, or URL params are invalid). The error message is user-friendly and i18n-ready.
frontend/public/components/utils/horizontal-nav.tsx (2)
272-289: Improved explicit prop passing to StatusBox.Moving from spreading
...objto explicit props (data,loaded,loadError) improves clarity and prevents unintended props from leaking intoStatusBox. The optional chaining handles undefinedobjsafely.
32-46: YAML components now explicitly set create={false}.Adding
create={false}to botheditYamlComponentandviewYamlComponentensures the YAML editor is in edit mode rather than create mode. This aligns with the watch-based loading pattern where resources already exist.frontend/public/components/factory/__tests__/details.spec.tsx (1)
60-117: Comprehensive test for extra resources in watch config.The test validates that both the primary
objresource and additionalconfigMapresource are correctly passed touseK8sWatchResources. The shape assertions (lines 93-116) ensure the migration maintains backward compatibility with the expected watch configuration format.
| isLoaded: () => { | ||
| cy.byTestID('skeleton-detail-view').should('not.exist'); | ||
| cy.get('[data-test-id="resource-title"]', { timeout: 30000 }).should('not.be.empty'); | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether weak readiness assertions are used elsewhere and whether text-trim checks are standardized.
rg -nP --type=ts -C2 "resource-title.*not\\.be\\.empty|\\.should\\('not\\.be\\.empty'\\)" frontend/packages/integration-tests-cypress
rg -nP --type=ts -C2 "text\\(\\)\\.trim\\(\\)|invoke\\('text'\\)" frontend/packages/integration-tests-cypressRepository: openshift/console
Length of output: 4994
isLoaded() title assertion needs stricter validation to prevent test flakiness
The .should('not.be.empty') check on line 18 can pass with whitespace-only content or before actual text is rendered, risking race conditions in dependent test steps. Enforce visibility and trimmed text content for robust readiness validation.
Suggested fix
isLoaded: () => {
cy.byTestID('skeleton-detail-view').should('not.exist');
- cy.get('[data-test-id="resource-title"]', { timeout: 30000 }).should('not.be.empty');
+ cy.get('[data-test-id="resource-title"]', { timeout: 30000 })
+ .should('be.visible')
+ .should(($title) => {
+ expect($title.text().trim()).to.not.equal('');
+ });
},Aligns with the stricter assertion patterns used elsewhere in the integration test suite (e.g., .invoke('text').should(...)) for meaningful content validation.
📝 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.
| isLoaded: () => { | |
| cy.byTestID('skeleton-detail-view').should('not.exist'); | |
| cy.get('[data-test-id="resource-title"]', { timeout: 30000 }).should('not.be.empty'); | |
| }, | |
| isLoaded: () => { | |
| cy.byTestID('skeleton-detail-view').should('not.exist'); | |
| cy.get('[data-test-id="resource-title"]', { timeout: 30000 }) | |
| .should('be.visible') | |
| .should(($title) => { | |
| expect($title.text().trim()).to.not.equal(''); | |
| }); | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/packages/integration-tests-cypress/views/details-page.ts` around
lines 16 - 19, The isLoaded() readiness check is too weak; after ensuring the
skeleton ('skeleton-detail-view') is gone, update the assertion on
'[data-test-id="resource-title"]' to first assert visibility
(should('be.visible')) and then assert trimmed, non-whitespace content by
invoking the element text and checking it is not empty (e.g., invoke('text') and
assert text.trim() contains non-whitespace or matches /\S/); make this change
inside the isLoaded function to replace the current .should('not.be.empty')
check.
| if (!loaded && !loadError) { | ||
| return ( | ||
| <Firehose resources={resources}> | ||
| <CommandLineTools {...(props as any)} /> | ||
| </Firehose> | ||
| <> | ||
| <DocumentTitle>{t('public~Command Line Tools')}</DocumentTitle> | ||
| <PageHeading title={t('public~Command Line Tools')} /> | ||
| <LoadingBox /> | ||
| </> | ||
| ); | ||
| }, | ||
| ); | ||
| } | ||
|
|
||
| return <CommandLineTools obj={{ data: cliDownloads as CLIDownload[], loaded, loadError }} />; | ||
| }; |
There was a problem hiding this comment.
Missing error state handling.
When loadError is truthy, the component falls through to render CommandLineTools without displaying the error. Consider rendering an error state or StatusBox when loading fails.
🛡️ Proposed fix to handle errors
if (!loaded && !loadError) {
return (
<>
<DocumentTitle>{t('public~Command Line Tools')}</DocumentTitle>
<PageHeading title={t('public~Command Line Tools')} />
<LoadingBox />
</>
);
}
+
+ if (loadError) {
+ return (
+ <>
+ <DocumentTitle>{t('public~Command Line Tools')}</DocumentTitle>
+ <PageHeading title={t('public~Command Line Tools')} />
+ <StatusBox loaded={loaded} loadError={loadError} />
+ </>
+ );
+ }
return <CommandLineTools obj={{ data: cliDownloads as CLIDownload[], loaded, loadError }} />;📝 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.
| if (!loaded && !loadError) { | |
| return ( | |
| <Firehose resources={resources}> | |
| <CommandLineTools {...(props as any)} /> | |
| </Firehose> | |
| <> | |
| <DocumentTitle>{t('public~Command Line Tools')}</DocumentTitle> | |
| <PageHeading title={t('public~Command Line Tools')} /> | |
| <LoadingBox /> | |
| </> | |
| ); | |
| }, | |
| ); | |
| } | |
| return <CommandLineTools obj={{ data: cliDownloads as CLIDownload[], loaded, loadError }} />; | |
| }; | |
| if (!loaded && !loadError) { | |
| return ( | |
| <> | |
| <DocumentTitle>{t('public~Command Line Tools')}</DocumentTitle> | |
| <PageHeading title={t('public~Command Line Tools')} /> | |
| <LoadingBox /> | |
| </> | |
| ); | |
| } | |
| if (loadError) { | |
| return ( | |
| <> | |
| <DocumentTitle>{t('public~Command Line Tools')}</DocumentTitle> | |
| <PageHeading title={t('public~Command Line Tools')} /> | |
| <StatusBox loaded={loaded} loadError={loadError} /> | |
| </> | |
| ); | |
| } | |
| return <CommandLineTools obj={{ data: cliDownloads as CLIDownload[], loaded, loadError }} />; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/public/components/command-line-tools.tsx` around lines 106 - 117,
The component currently only shows LoadingBox when !loaded and !loadError, but
when loadError is truthy it still falls through to render CommandLineTools;
update the conditional to explicitly handle the error case by returning an
error/status UI (e.g., render StatusBox or a dedicated error message) when
loadError is set, ensure you pass the same props or error details to
CommandLineTools if you still want it mounted, and add any required import for
StatusBox; refer to the symbols loaded, loadError, CommandLineTools, LoadingBox,
and StatusBox to locate where to add the new branch.
| obj={{ data: props.pod, loaded: props.loaded, loadError: props.loadError }} | ||
| /> | ||
| <HorizontalNav | ||
| hideNav={true} | ||
| pages={[{ name: 'container', href: '', component: ContainerDetailsList }]} | ||
| obj={props.obj} | ||
| obj={{ data: props.pod, loaded: props.loaded }} | ||
| /> |
There was a problem hiding this comment.
Inconsistent loadError handling between components.
ConnectedPageHeading receives loadError (line 513) but HorizontalNav does not (line 518). Per the horizontal-nav.tsx changes, HorizontalNav now accepts loadError in its type definition. This inconsistency could affect error display behavior in the nav's StatusBox.
🔧 Pass loadError consistently
<HorizontalNav
hideNav={true}
pages={[{ name: 'container', href: '', component: ContainerDetailsList }]}
- obj={{ data: props.pod, loaded: props.loaded }}
+ obj={{ data: props.pod, loaded: props.loaded, loadError: props.loadError }}
/>📝 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.
| obj={{ data: props.pod, loaded: props.loaded, loadError: props.loadError }} | |
| /> | |
| <HorizontalNav | |
| hideNav={true} | |
| pages={[{ name: 'container', href: '', component: ContainerDetailsList }]} | |
| obj={props.obj} | |
| obj={{ data: props.pod, loaded: props.loaded }} | |
| /> | |
| obj={{ data: props.pod, loaded: props.loaded, loadError: props.loadError }} | |
| /> | |
| <HorizontalNav | |
| hideNav={true} | |
| pages={[{ name: 'container', href: '', component: ContainerDetailsList }]} | |
| obj={{ data: props.pod, loaded: props.loaded, loadError: props.loadError }} | |
| /> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/public/components/container.tsx` around lines 513 - 519,
ConnectedPageHeading is being passed loadError but HorizontalNav is not; update
the HorizontalNav invocation to include the same loadError so its StatusBox can
render errors consistently. Specifically, in the component where
ConnectedPageHeading and HorizontalNav are rendered, add loadError:
props.loadError to the obj prop passed to HorizontalNav (the HorizontalNav call
that currently passes obj={{ data: props.pod, loaded: props.loaded }}), ensuring
ContainerDetailsList, props.pod, props.loaded and props.loadError are all
forwarded consistently.
| if (!loaded && !loadError) { | ||
| return <LoadingBox />; | ||
| } | ||
|
|
||
| if (loadError) { | ||
| return <ErrorPage404 />; | ||
| } |
There was a problem hiding this comment.
Error handling may obscure actual error type.
Rendering ErrorPage404 for all loadError cases is misleading. A 403 Forbidden or network timeout would show "Page Not Found" instead of an appropriate error message. Consider checking the error status code or using a generic error component that displays the actual error.
🛠️ Suggested approach
- if (loadError) {
- return <ErrorPage404 />;
+ if (loadError) {
+ // Check if it's actually a 404, otherwise show the real error
+ if (loadError?.response?.status === 404) {
+ return <ErrorPage404 />;
+ }
+ return <LoadError label={props.kind}>{loadError.message || 'Failed to load resource'}</LoadError>;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/public/components/create-yaml.tsx` around lines 128 - 134, The
current error handling in the create-yaml component returns ErrorPage404 for any
loadError, which misrepresents errors like 403 or network failures; update the
render logic around the loaded and loadError checks (variables loaded,
loadError) to inspect loadError (e.g., status or error.type/message) and render
an appropriate component instead of always ErrorPage404—either map specific
statuses to dedicated components (e.g., ForbiddenPage for 403) or introduce a
GenericError component that displays the actual error.message/status so users
see the real failure instead of a 404.
| // TODO: Remove the resources prop and the fallback ?? resources after all components are migrated from Firehose to hooks. | ||
| // Use watchedResources (from useK8sWatchResources) if available, fallback to resources (from Firehose) | ||
| const resourceData = watchedResources ?? resources; | ||
| const data = flatten ? flatten(resourceData) : []; |
There was a problem hiding this comment.
Don't switch to watchedResources until it has entries.
useK8sWatchResources can be {} on the first render, so watchedResources ?? resources prefers the empty object and still calls flatten. Existing callers that do resources.foo.data will throw, and the Firehose fallback in the TODO never activates.
🔧 Suggested fix
- const resourceData = watchedResources ?? resources;
- const data = flatten ? flatten(resourceData) : [];
+ const resourceData =
+ watchedResources && !_.isEmpty(watchedResources) ? watchedResources : resources;
+ const data = flatten && resourceData ? flatten(resourceData) : [];📝 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.
| // TODO: Remove the resources prop and the fallback ?? resources after all components are migrated from Firehose to hooks. | |
| // Use watchedResources (from useK8sWatchResources) if available, fallback to resources (from Firehose) | |
| const resourceData = watchedResources ?? resources; | |
| const data = flatten ? flatten(resourceData) : []; | |
| // TODO: Remove the resources prop and the fallback ?? resources after all components are migrated from Firehose to hooks. | |
| // Use watchedResources (from useK8sWatchResources) if available, fallback to resources (from Firehose) | |
| const resourceData = | |
| watchedResources && !_.isEmpty(watchedResources) ? watchedResources : resources; | |
| const data = flatten && resourceData ? flatten(resourceData) : []; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/public/components/factory/list-page.tsx` around lines 108 - 111, The
current selection of resourceData uses watchedResources ?? resources which picks
an empty object from useK8sWatchResources on first render and breaks callers;
update the logic in the list-page component to only use watchedResources when it
actually has entries (e.g. check watchedResources is truthy and
Object.keys(watchedResources).length > 0) otherwise fall back to resources, and
keep the subsequent use of flatten(resourceData) unchanged (references:
watchedResources, resources, resourceData, flatten, useK8sWatchResources).
| reduxIDs: propsReduxIDs, | ||
| filterList: propsFilterList, |
There was a problem hiding this comment.
Dispatch the filter action here.
Line 617 passes the raw filterList action creator into FireMan, but applyFilterInternal just invokes it and drops the return value. URL-applied filters and any applyFilter consumer will no-op.
🔧 Suggested fix
export const FireMan: FC<FireManProps & { filterList?: typeof filterList }> = (props) => {
const {
resources,
textFilter,
canCreate,
@@
- filterList: propsFilterList,
+ filterList: propsFilterList = filterList,
} = props;
+ const dispatch = useConsoleDispatch();
const navigate = useNavigate();
@@
const applyFilterInternal = useCallback(
(filterName: string, options: any) => {
if (['q', 'kind', 'orderBy', 'sortBy'].includes(filterName)) {
return;
}
if (filterName.indexOf(storagePrefix) === 0) {
return;
}
- reduxIDs.forEach((id) => propsFilterList(id, filterName, options));
+ reduxIDs.forEach((id) => dispatch(propsFilterList(id, filterName, options)));
},
- [reduxIDs, propsFilterList],
+ [dispatch, reduxIDs, propsFilterList],
);Also applies to: 202-218, 617-617
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/public/components/factory/list-page.tsx` around lines 176 - 177, The
bug is that FireMan is being given the raw action creator filterList
(propsFilterList) but applyFilterInternal calls it and ignores the returned
action, so dispatch never occurs; update the place where FireMan is constructed
(and any other occurrences around lines with reduxIDs/ filterList and where
applyFilterInternal is used) to pass a bound dispatcher function instead of the
raw creator—i.e., wrap propsFilterList with dispatch (or call
props.dispatch(propsFilterList(...))) so applyFilter / applyFilterInternal
receive and return a dispatched action; ensure functions named FireMan,
applyFilterInternal, filterList (propsFilterList) and applyFilter are updated
accordingly so URL-applied filters and applyFilter consumers receive the
dispatched result.
| // Build resources configuration for FireMan (needs prop for redux IDs) | ||
| const k8sResources = useMemo( | ||
| () => | ||
| _.map(props.resources, (r) => ({ | ||
| ...r, | ||
| isList: r.isList !== undefined ? r.isList : true, | ||
| namespace: r.namespaced ? namespace : r.namespace, | ||
| prop: r.prop || r.kind, | ||
| })), | ||
| [props.resources, namespace], | ||
| ); | ||
|
|
||
| // Build watch resources configuration for useK8sWatchResources | ||
| const watchResources = useMemo(() => { | ||
| if (mock) { | ||
| return {}; | ||
| } | ||
| return k8sResources.reduce((acc, r) => { | ||
| const key = r.prop || r.kind; | ||
| acc[key] = { | ||
| kind: r.kind, | ||
| name: r.name, | ||
| namespace: r.namespace, | ||
| isList: r.isList, | ||
| selector: r.selector, | ||
| fieldSelector: r.fieldSelector, | ||
| limit: r.limit, | ||
| namespaced: r.namespaced, | ||
| optional: r.optional, | ||
| }; | ||
| return acc; | ||
| }, {} as Record<string, WatchK8sResource>); | ||
| }, [k8sResources, mock]); |
There was a problem hiding this comment.
Preserve resources[].filters in the watch path.
Line 404 still threads filters into each resource config, and Line 415 assumes the list content is already narrowed when that prop is used. This conversion never applies r.filters to watchedResources, so pages relying on ListPageProps.filters will now render the full dataset and incorrect row-filter counts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/public/components/factory/list-page.tsx` around lines 533 - 565, The
watchResources builder omits resource-level filters so ListPageProps.filters are
not applied to useK8sWatchResources; update the reduce that creates
watchResources (based on k8sResources) to copy r.filters into each watch config
(e.g., include filters: r.filters) so the produced record passed to
useK8sWatchResources preserves resources[].filters and downstream list rendering
and row counts are filtered correctly; modify the reduce in the watchResources
useMemo that constructs acc[key] to include the filters property from r.
4dc27c6 to
5e342b6
Compare
|
@rhamilto: This pull request references CONSOLE-5091 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 spike to target the "4.22.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. |
|
@rhamilto: This pull request references CONSOLE-5091 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 spike to target the "4.22.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. |
openshift-ci
Bot
added
the
needs-rebase
|
@rhamilto: This pull request references CONSOLE-5091 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 spike to target the "4.22.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. |
|
/test frontend |
rhamilto
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
/test backend |
|
ConsoleDataView remains part of the internal API, which is shipped without guarantees /label plugin-api-approved |
openshift-ci
Bot
added
the
plugin-api-approved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
frontend/packages/console-app/src/components/data-view/useDataViewSelection.ts (1)
75-85:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve off-page selections in bulk toggle.
onSelectAllcurrently overwrites/clears the full set, so selecting/unselecting on one page drops selections from other pages/filters.Suggested fix
const onSelectAll = useCallback( (isSelecting: boolean, filteredItems: T[]) => { - if (isSelecting) { - const selectableItems = filterSelectable - ? filteredItems.filter(filterSelectable) - : filteredItems; - const itemIds = selectableItems.map(getItemId); - setSelectedIds(new Set(itemIds)); - } else { - setSelectedIds(new Set()); - } + const selectableItems = filterSelectable + ? filteredItems.filter(filterSelectable) + : filteredItems; + const itemIds = selectableItems.map(getItemId); + + setSelectedIds((prev) => { + const next = new Set(prev); + itemIds.forEach((id) => { + if (isSelecting) { + next.add(id); + } else { + next.delete(id); + } + }); + return next; + }); }, [getItemId, filterSelectable], );🤖 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 `@frontend/packages/console-app/src/components/data-view/useDataViewSelection.ts` around lines 75 - 85, onSelectAll currently replaces the entire selectedIds set causing off-page selections to be lost; instead, when isSelecting is true merge the current page's selectable item IDs (derived via filterSelectable and getItemId) into the existing selectedIds Set, and when isSelecting is false remove only those current page IDs from the existing Set rather than clearing everything; update the set via setSelectedIds(prev => new Set(updatedIds)) and reference onSelectAll, filterSelectable, getItemId, setSelectedIds and the selectedIds state in your change.frontend/packages/console-app/src/components/data-view/useConsoleDataViewData.tsx (2)
217-220:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftExclude non-selectable rows from select-all checked-state calculation.
allVisibleSelectedis computed from all visible rows, so pages containing non-selectable rows can never reach checked state even when all selectable rows are selected.🤖 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 `@frontend/packages/console-app/src/components/data-view/useConsoleDataViewData.tsx` around lines 217 - 220, The select-all checked-state currently uses all visibleItems, which prevents pages with non-selectable rows from becoming checked; update the logic inside the column.id === 'select' branch to first filter visibleItems to only selectable rows (use column.props.select(item) or the appropriate predicate on column.props.select) and then compute allVisibleSelected by ensuring every selectable visible item has its id in selection.selectedItems via selection.getItemId(item); keep the existing checks for visibleItems.length and selection?.onSelectAll and use selection.getItemId for membership tests.
143-147:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse a dedicated screen-reader label for the select column header.
Empty-title fallback currently announces the select column as “Actions”, which is misleading for assistive tech.
As per coding guidelines, "Follow WCAG 2.1 AA standards in TypeScript/React code; use semantic HTML, ARIA labels where needed, ensure keyboard navigation, and test with screen readers".
🤖 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 `@frontend/packages/console-app/src/components/data-view/useConsoleDataViewData.tsx` around lines 143 - 147, Replace the misleading empty-title fallback text "Actions" used in the column header with an explicit screen-reader label for the select column: update the cell rendering logic (the cell variable in useConsoleDataViewData) so when title is falsy it renders a visually-hidden span with a clear, localized label such as "Select" or "Select rows" (use t('...') for translation) or add an appropriate aria-label on the column header element; ensure this change targets the select column header behavior and preserves the existing pf-v6-u-screen-reader class for accessibility.frontend/packages/console-app/src/components/nodes/modals/ConfigureUnschedulableModal.tsx (1)
41-49:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBlock submit when there are no target nodes.
This flow still allows submitting an empty target list (
markNodesUnschedulable([])) and closing as if successful.Suggested fix
const isBulk = targetNodes.length > 1; + const hasTargets = targetNodes.length > 0; const handleSubmit = (): void => { + if (!hasTargets) { + return; + } handlePromise(markNodesUnschedulable(targetNodes)) .then(() => { onComplete?.(); close(); }) @@ <Button type="button" variant="primary" onClick={handleSubmit} isLoading={inProgress} - isDisabled={inProgress} + isDisabled={inProgress || !hasTargets} >Also applies to: 58-63, 93-99
🤖 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 `@frontend/packages/console-app/src/components/nodes/modals/ConfigureUnschedulableModal.tsx` around lines 41 - 49, The form allows calling markNodesUnschedulable with an empty targetNodes array; update ConfigureUnschedulableModal to prevent submit when there are no targets by adding a guard in the submit handler and disabling the submit button: check targetNodes.length > 0 before calling markNodesUnschedulable (and before closing the modal), return early and surface a validation/error state if empty; also ensure any other submit paths that call markNodesUnschedulable (the code blocks referencing targetNodes and markNodesUnschedulable) perform the same length check so empty submissions cannot proceed.frontend/packages/console-app/src/components/nodes/NodesPage.tsx (1)
571-575:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRender blank cell (not
DASH) for missing select-cell rows.When
rowCells.selectis absent, fallback still rendersDASHin the selection column.Suggested fix
if (!rowCell) { return { id, - cell: DASH, + cell: id === 'select' ? '' : DASH, }; }🤖 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 `@frontend/packages/console-app/src/components/nodes/NodesPage.tsx` around lines 571 - 575, The selection column fallback currently returns DASH when rowCells.select is missing; update the conditional that checks rowCell (the block returning { id, cell: DASH }) to render a blank cell instead (e.g., cell: '' or null) so missing select-cell rows produce an empty selection column; locate the conditional referencing rowCell / rowCells.select and replace the DASH fallback with an empty value while preserving the returned shape ({ id, cell: ... }).
🤖 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 `@frontend/packages/console-app/src/components/nodes/useCustomNodeActions.tsx`:
- Around line 35-41: The current flow calls onComplete() only in the .then()
after handlePromise(markNodesSchedulable(selectedNodes)), so if Promise.all
inside markNodesSchedulable rejects on any node the completion path is skipped;
change the promise handling to always run onComplete by moving it into a
.finally() while keeping the existing .catch() (errors are handled by
usePromiseHandler). Update the call site that uses
handlePromise(markNodesSchedulable(selectedNodes)) so that .catch(() => { /*
usePromiseHandler handles errors */ }) is preserved and onComplete() is invoked
from .finally(), referencing the same functions/handlers: handlePromise,
markNodesSchedulable, onComplete, and usePromiseHandler.
In
`@frontend/packages/console-shared/src/components/dropdown/ResponsiveActionDropdown.tsx`:
- Line 12: The file imports the internal OverflowMenuContext in
ResponsiveActionDropdown; replace this brittle internal usage by using
PatternFly's public OverflowMenu responsive API instead: remove the import and
any references to OverflowMenuContext, wrap or render ResponsiveActionDropdown
using PatternFly's OverflowMenu/OverflowMenuContent props (e.g., breakpoint or
breakpointReference) and switch the toggle/rendering logic to rely on those
public responsive props; update the component (ResponsiveActionDropdown) to
accept or derive the breakpoint/breakpointReference and conditionally render the
toggle based on that instead of reading OverflowMenuContext.
---
Duplicate comments:
In
`@frontend/packages/console-app/src/components/data-view/useConsoleDataViewData.tsx`:
- Around line 217-220: The select-all checked-state currently uses all
visibleItems, which prevents pages with non-selectable rows from becoming
checked; update the logic inside the column.id === 'select' branch to first
filter visibleItems to only selectable rows (use column.props.select(item) or
the appropriate predicate on column.props.select) and then compute
allVisibleSelected by ensuring every selectable visible item has its id in
selection.selectedItems via selection.getItemId(item); keep the existing checks
for visibleItems.length and selection?.onSelectAll and use selection.getItemId
for membership tests.
- Around line 143-147: Replace the misleading empty-title fallback text
"Actions" used in the column header with an explicit screen-reader label for the
select column: update the cell rendering logic (the cell variable in
useConsoleDataViewData) so when title is falsy it renders a visually-hidden span
with a clear, localized label such as "Select" or "Select rows" (use t('...')
for translation) or add an appropriate aria-label on the column header element;
ensure this change targets the select column header behavior and preserves the
existing pf-v6-u-screen-reader class for accessibility.
In
`@frontend/packages/console-app/src/components/data-view/useDataViewSelection.ts`:
- Around line 75-85: onSelectAll currently replaces the entire selectedIds set
causing off-page selections to be lost; instead, when isSelecting is true merge
the current page's selectable item IDs (derived via filterSelectable and
getItemId) into the existing selectedIds Set, and when isSelecting is false
remove only those current page IDs from the existing Set rather than clearing
everything; update the set via setSelectedIds(prev => new Set(updatedIds)) and
reference onSelectAll, filterSelectable, getItemId, setSelectedIds and the
selectedIds state in your change.
In
`@frontend/packages/console-app/src/components/nodes/modals/ConfigureUnschedulableModal.tsx`:
- Around line 41-49: The form allows calling markNodesUnschedulable with an
empty targetNodes array; update ConfigureUnschedulableModal to prevent submit
when there are no targets by adding a guard in the submit handler and disabling
the submit button: check targetNodes.length > 0 before calling
markNodesUnschedulable (and before closing the modal), return early and surface
a validation/error state if empty; also ensure any other submit paths that call
markNodesUnschedulable (the code blocks referencing targetNodes and
markNodesUnschedulable) perform the same length check so empty submissions
cannot proceed.
In `@frontend/packages/console-app/src/components/nodes/NodesPage.tsx`:
- Around line 571-575: The selection column fallback currently returns DASH when
rowCells.select is missing; update the conditional that checks rowCell (the
block returning { id, cell: DASH }) to render a blank cell instead (e.g., cell:
'' or null) so missing select-cell rows produce an empty selection column;
locate the conditional referencing rowCell / rowCells.select and replace the
DASH fallback with an empty value while preserving the returned shape ({ id,
cell: ... }).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ef35cf85-61fc-4a37-9c33-93ccd957da97
⛔ Files ignored due to path filters (1)
frontend/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (19)
frontend/packages/console-app/locales/en/console-app.jsonfrontend/packages/console-app/src/components/data-view/BULK_SELECTION_GUIDE.mdfrontend/packages/console-app/src/components/data-view/ConsoleDataView.tsxfrontend/packages/console-app/src/components/data-view/dataViewSelectionHelpers.tsfrontend/packages/console-app/src/components/data-view/useConsoleDataViewData.tsxfrontend/packages/console-app/src/components/data-view/useDataViewSelection.tsfrontend/packages/console-app/src/components/nodes/NodesPage.tsxfrontend/packages/console-app/src/components/nodes/menu-actions.tsxfrontend/packages/console-app/src/components/nodes/modals/ConfigureUnschedulableModal.tsxfrontend/packages/console-app/src/components/nodes/nodeSchedulingActions.tsfrontend/packages/console-app/src/components/nodes/useCustomNodeActions.tsxfrontend/packages/console-dynamic-plugin-sdk/src/api/internal-api.tsfrontend/packages/console-dynamic-plugin-sdk/src/api/internal-types.tsfrontend/packages/console-shared/src/components/dropdown/ResponsiveActionDropdown.tsxfrontend/packages/console-shared/src/components/dropdown/__tests__/ResponsiveActionDropdown.spec.tsxfrontend/public/components/droppable-edit-yaml.tsxfrontend/public/kinds.tsfrontend/public/locales/en/public.jsonfrontend/public/style/ancillary/_bootstrap-residual.scss
✅ Files skipped from review due to trivial changes (4)
- frontend/public/kinds.ts
- frontend/public/components/droppable-edit-yaml.tsx
- frontend/packages/console-app/locales/en/console-app.json
- frontend/packages/console-app/src/components/data-view/BULK_SELECTION_GUIDE.md
|
/retest |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
frontend/packages/console-app/src/components/data-view/ConsoleDataView.tsx (1)
184-206:⚠️ Potential issue | 🟠 Major | ⚡ Quick winExclude non-selectable rows from banner selection math.
allVisibleSelected,selectedCount, andallSelectedcurrently include rows that can never be selected, so banner state gets stuck on mixed pages.🤖 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 `@frontend/packages/console-app/src/components/data-view/ConsoleDataView.tsx` around lines 184 - 206, The banner math in ConsoleDataView incorrectly counts non-selectable rows; update the computations to filter out non-selectable items before counting: create a selectableVisible = visibleItems.filter(item => /* use selection.canSelectItem(item) if available, or check item.selectable/row.selectable predicate used elsewhere */) and selectableFiltered = filteredData.filter(item => /* same predicate */), then compute allVisibleSelected using selectableVisible.every(... selection.getItemId ...), visibleSelectedCount from selectableVisible.filter(...), visibleCount = selectableVisible.length, selectedCount = selectableFiltered.filter(...).length, allSelected = selectedCount === selectableFiltered.length, and isIndeterminate based on selectableVisible counts; reference ConsoleDataView, visibleItems, filteredData, selection.getItemId, and selection.selectedItems when making these changes.frontend/packages/console-app/src/components/data-view/useDataViewSelection.ts (1)
75-85:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve existing selections in
onSelectAll.This still overwrites (and clears) the whole selection set, so page-scoped select-all drops off-page selections.
Suggested fix
const onSelectAll = useCallback( (isSelecting: boolean, filteredItems: T[]) => { - if (isSelecting) { - const selectableItems = filterSelectable - ? filteredItems.filter(filterSelectable) - : filteredItems; - const itemIds = selectableItems.map(getItemId); - setSelectedIds(new Set(itemIds)); - } else { - setSelectedIds(new Set()); - } + const selectableItems = filterSelectable ? filteredItems.filter(filterSelectable) : filteredItems; + const itemIds = selectableItems.map(getItemId); + setSelectedIds((prev) => { + const next = new Set(prev); + itemIds.forEach((id) => (isSelecting ? next.add(id) : next.delete(id))); + return next; + }); }, [getItemId, filterSelectable], );🤖 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 `@frontend/packages/console-app/src/components/data-view/useDataViewSelection.ts` around lines 75 - 85, The onSelectAll handler currently replaces the entire selection Set, dropping off-page selections; update onSelectAll (the useCallback that takes isSelecting, filteredItems) to merge instead: when isSelecting is true compute selectableItems.map(getItemId) and add those ids into the existing selectedIds Set (creating a new Set from selectedIds and adding each id) before calling setSelectedIds, and when isSelecting is false remove only those filtered/selectable ids from the existing Set (create a new Set from selectedIds and delete each id) then call setSelectedIds—use getItemId and filterSelectable to derive the ids to add/remove so other selections remain preserved.frontend/packages/console-app/src/components/nodes/NodesPage.tsx (1)
571-575:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep non-selectable select-cells blank.
For CSR rows, the select column still renders
DASHvia fallback. That should be empty so the selection column stays visually consistent.Suggested fix
if (!rowCell) { return { id, - cell: DASH, + cell: id === 'select' ? '' : DASH, }; }🤖 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 `@frontend/packages/console-app/src/components/nodes/NodesPage.tsx` around lines 571 - 575, The select-column fallback currently returns DASH when rowCell is missing; for non-selectable CSR rows keep the cell blank instead. In NodesPage.tsx change the branch that returns { id, cell: DASH } (where rowCell is falsy) to return an empty string (e.g., { id, cell: '' }) so the select column renders as blank for non-selectable rows; update any code that constructs the select cell using rowCell/DASH accordingly.frontend/packages/console-app/src/components/data-view/useConsoleDataViewData.tsx (1)
143-147:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse a dedicated SR label for the select column header.
The select column currently falls back to “Actions”, which mislabels the checkbox header for assistive tech.
Suggested fix
- ) : ( - <span className="pf-v6-u-screen-reader">{t('public~Actions')}</span> - ), + ) : ( + <span className="pf-v6-u-screen-reader"> + {id === 'select' ? t('public~Select rows') : t('public~Actions')} + </span> + ),As per coding guidelines, "Follow WCAG 2.1 AA standards in TypeScript/React code; use semantic HTML, ARIA labels where needed, ensure keyboard navigation, and test with screen readers".
🤖 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 `@frontend/packages/console-app/src/components/data-view/useConsoleDataViewData.tsx` around lines 143 - 147, The select-column header currently falls back to "Actions", mislabeling the checkbox header for screen readers; update the header cell logic in useConsoleDataViewData (the column definition that sets cell: title ? <span>{title}</span> : <span className="pf-v6-u-screen-reader">{t('public~Actions')}</span>) to use a dedicated SR label such as t('public~Select') or t('public~Select rows') instead of "Actions", keeping the pf-v6-u-screen-reader span and localization call so assistive tech correctly identifies the checkbox column.
🧹 Nitpick comments (4)
frontend/public/components/droppable-edit-yaml.tsx (1)
144-144: ⚡ Quick winRemove redundant
DropEvent/File[]casts inonDrop
MultipleFileUpload’sdropzoneProps.onDroppasses the 3rdeventargument as a requiredDropEvent(react-dropzone signature), soonFileDrop(event as DropEvent, acceptedFiles as File[])is unnecessary and weakens type checking. CallonFileDrop(event, acceptedFiles)(and avoid theacceptedFiles as File[]cast if types already align).🤖 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 `@frontend/public/components/droppable-edit-yaml.tsx` at line 144, The call to onFileDrop is using redundant casts (event as DropEvent, acceptedFiles as File[]) inside the MultipleFileUpload drop handler; remove those casts and call onFileDrop(event, acceptedFiles) directly, and if TypeScript complains adjust the onFileDrop signature or the dropzoneProps typing so the event type and acceptedFiles type align with react-dropzone's DropEvent and File[] rather than forcing casts; locate onFileDrop and the dropzoneProps.onDrop usage to make the minimal typing fix.frontend/packages/console-app/src/components/nodes/modals/ConfigureUnschedulableModal.tsx (1)
38-38: ⚡ Quick winUse a namespaced translation hook here.
Switch to
useTranslation('console-app')and non-prefixed keys in this component for parser consistency.As per coding guidelines, use
useTranslation('namespace')hook withkeyformat for translation keys in TypeScript/React.🤖 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 `@frontend/packages/console-app/src/components/nodes/modals/ConfigureUnschedulableModal.tsx` at line 38, Replace the generic translation hook in ConfigureUnschedulableModal.tsx by calling useTranslation with the namespace: change the existing const { t } = useTranslation(); to const { t } = useTranslation('console-app') and update all t(...) usages in this component to use non-prefixed keys (remove any manual namespace prefixes) so keys follow the 'key' format expected by the console-app namespace.frontend/packages/console-app/src/components/nodes/useCustomNodeActions.tsx (1)
25-25: ⚡ Quick winUse a namespaced translation hook in this hook.
Prefer
useTranslation('console-app')and namespace-local keys for consistent extraction.As per coding guidelines, use
useTranslation('namespace')hook withkeyformat for translation keys in TypeScript/React.🤖 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 `@frontend/packages/console-app/src/components/nodes/useCustomNodeActions.tsx` at line 25, The hook currently calls useTranslation() without a namespace; update the translation usage to useTranslation('console-app') in useCustomNodeActions (so the const { t } = useTranslation() becomes namespaced), and ensure all translation keys referenced in this module use the namespaced key format (e.g., 'yourKey' -> 'console-app:yourKey') or the local key style expected by the extractor so key extraction and scoping work correctly.frontend/packages/console-shared/src/components/dropdown/__tests__/ResponsiveActionDropdown.spec.tsx (1)
21-76: ⚡ Quick winUse table-driven tests for the repeated variant scenarios.
The variant assertions are repetitive and fit
it.each(...)well, which will keep this suite easier to extend and maintain.♻️ Suggested refactor
- it('applies custom variant prop', () => { - render( - <ResponsiveActionDropdown label="Actions" variant="secondary" data-test="actions-dropdown"> - <DropdownItem>Action 1</DropdownItem> - </ResponsiveActionDropdown>, - ); - const toggle = screen.getByTestId('actions-dropdown'); - expect(toggle).toHaveClass('pf-m-secondary'); - }); - - it('defaults to primary variant', () => { - render( - <ResponsiveActionDropdown label="Actions" data-test="actions-dropdown"> - <DropdownItem>Action 1</DropdownItem> - </ResponsiveActionDropdown>, - ); - const toggle = screen.getByTestId('actions-dropdown'); - expect(toggle).toHaveClass('pf-m-primary'); - }); + it.each([ + { name: 'secondary variant', variant: 'secondary' as const, expectedClass: 'pf-m-secondary' }, + { name: 'default primary variant', variant: undefined, expectedClass: 'pf-m-primary' }, + ])('applies $name', ({ variant, expectedClass }) => { + render( + <ResponsiveActionDropdown label="Actions" variant={variant} data-test="actions-dropdown"> + <DropdownItem>Action 1</DropdownItem> + </ResponsiveActionDropdown>, + ); + expect(screen.getByTestId('actions-dropdown')).toHaveClass(expectedClass); + });As per coding guidelines, “TypeScript and JavaScript tests should follow a similar 'test tables' convention as used in Go where applicable”.
🤖 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 `@frontend/packages/console-shared/src/components/dropdown/__tests__/ResponsiveActionDropdown.spec.tsx` around lines 21 - 76, Replace the two repetitive variant tests in ResponsiveActionDropdown.spec.tsx with a table-driven test using Jest's it.each: create a data table of cases (e.g., ["secondary","pf-m-secondary"], [undefined,"pf-m-primary"]) and for each case render <ResponsiveActionDropdown variant={variant} data-test="actions-dropdown"> and assert the toggle (screen.getByTestId('actions-dropdown')) has the expected class; reference the ResponsiveActionDropdown component and the test file's existing test IDs to locate where to change the tests and remove the separate "applies custom variant prop" and "defaults to primary variant" blocks.
🤖 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 `@frontend/packages/console-dynamic-plugin-sdk/src/api/internal-types.ts`:
- Around line 379-380: The API comment for actionsBreakpoint is incorrect: it
states the default is 'lg' but the component uses 'md'; update the JSDoc for the
actionsBreakpoint property (OverflowMenuProps['breakpoint']) to reflect the
actual default 'md' so docs match the component behavior and avoid confusion.
---
Duplicate comments:
In `@frontend/packages/console-app/src/components/data-view/ConsoleDataView.tsx`:
- Around line 184-206: The banner math in ConsoleDataView incorrectly counts
non-selectable rows; update the computations to filter out non-selectable items
before counting: create a selectableVisible = visibleItems.filter(item => /* use
selection.canSelectItem(item) if available, or check
item.selectable/row.selectable predicate used elsewhere */) and
selectableFiltered = filteredData.filter(item => /* same predicate */), then
compute allVisibleSelected using selectableVisible.every(... selection.getItemId
...), visibleSelectedCount from selectableVisible.filter(...), visibleCount =
selectableVisible.length, selectedCount = selectableFiltered.filter(...).length,
allSelected = selectedCount === selectableFiltered.length, and isIndeterminate
based on selectableVisible counts; reference ConsoleDataView, visibleItems,
filteredData, selection.getItemId, and selection.selectedItems when making these
changes.
In
`@frontend/packages/console-app/src/components/data-view/useConsoleDataViewData.tsx`:
- Around line 143-147: The select-column header currently falls back to
"Actions", mislabeling the checkbox header for screen readers; update the header
cell logic in useConsoleDataViewData (the column definition that sets cell:
title ? <span>{title}</span> : <span
className="pf-v6-u-screen-reader">{t('public~Actions')}</span>) to use a
dedicated SR label such as t('public~Select') or t('public~Select rows') instead
of "Actions", keeping the pf-v6-u-screen-reader span and localization call so
assistive tech correctly identifies the checkbox column.
In
`@frontend/packages/console-app/src/components/data-view/useDataViewSelection.ts`:
- Around line 75-85: The onSelectAll handler currently replaces the entire
selection Set, dropping off-page selections; update onSelectAll (the useCallback
that takes isSelecting, filteredItems) to merge instead: when isSelecting is
true compute selectableItems.map(getItemId) and add those ids into the existing
selectedIds Set (creating a new Set from selectedIds and adding each id) before
calling setSelectedIds, and when isSelecting is false remove only those
filtered/selectable ids from the existing Set (create a new Set from selectedIds
and delete each id) then call setSelectedIds—use getItemId and filterSelectable
to derive the ids to add/remove so other selections remain preserved.
In `@frontend/packages/console-app/src/components/nodes/NodesPage.tsx`:
- Around line 571-575: The select-column fallback currently returns DASH when
rowCell is missing; for non-selectable CSR rows keep the cell blank instead. In
NodesPage.tsx change the branch that returns { id, cell: DASH } (where rowCell
is falsy) to return an empty string (e.g., { id, cell: '' }) so the select
column renders as blank for non-selectable rows; update any code that constructs
the select cell using rowCell/DASH accordingly.
---
Nitpick comments:
In
`@frontend/packages/console-app/src/components/nodes/modals/ConfigureUnschedulableModal.tsx`:
- Line 38: Replace the generic translation hook in
ConfigureUnschedulableModal.tsx by calling useTranslation with the namespace:
change the existing const { t } = useTranslation(); to const { t } =
useTranslation('console-app') and update all t(...) usages in this component to
use non-prefixed keys (remove any manual namespace prefixes) so keys follow the
'key' format expected by the console-app namespace.
In `@frontend/packages/console-app/src/components/nodes/useCustomNodeActions.tsx`:
- Line 25: The hook currently calls useTranslation() without a namespace; update
the translation usage to useTranslation('console-app') in useCustomNodeActions
(so the const { t } = useTranslation() becomes namespaced), and ensure all
translation keys referenced in this module use the namespaced key format (e.g.,
'yourKey' -> 'console-app:yourKey') or the local key style expected by the
extractor so key extraction and scoping work correctly.
In
`@frontend/packages/console-shared/src/components/dropdown/__tests__/ResponsiveActionDropdown.spec.tsx`:
- Around line 21-76: Replace the two repetitive variant tests in
ResponsiveActionDropdown.spec.tsx with a table-driven test using Jest's it.each:
create a data table of cases (e.g., ["secondary","pf-m-secondary"],
[undefined,"pf-m-primary"]) and for each case render <ResponsiveActionDropdown
variant={variant} data-test="actions-dropdown"> and assert the toggle
(screen.getByTestId('actions-dropdown')) has the expected class; reference the
ResponsiveActionDropdown component and the test file's existing test IDs to
locate where to change the tests and remove the separate "applies custom variant
prop" and "defaults to primary variant" blocks.
In `@frontend/public/components/droppable-edit-yaml.tsx`:
- Line 144: The call to onFileDrop is using redundant casts (event as DropEvent,
acceptedFiles as File[]) inside the MultipleFileUpload drop handler; remove
those casts and call onFileDrop(event, acceptedFiles) directly, and if
TypeScript complains adjust the onFileDrop signature or the dropzoneProps typing
so the event type and acceptedFiles type align with react-dropzone's DropEvent
and File[] rather than forcing casts; locate onFileDrop and the
dropzoneProps.onDrop usage to make the minimal typing fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4b244d73-daf0-4916-a843-2893d701cbe8
📒 Files selected for processing (19)
frontend/packages/console-app/locales/en/console-app.jsonfrontend/packages/console-app/src/components/data-view/BULK_SELECTION_GUIDE.mdfrontend/packages/console-app/src/components/data-view/ConsoleDataView.tsxfrontend/packages/console-app/src/components/data-view/dataViewSelectionHelpers.tsfrontend/packages/console-app/src/components/data-view/useConsoleDataViewData.tsxfrontend/packages/console-app/src/components/data-view/useDataViewSelection.tsfrontend/packages/console-app/src/components/nodes/NodesPage.tsxfrontend/packages/console-app/src/components/nodes/menu-actions.tsxfrontend/packages/console-app/src/components/nodes/modals/ConfigureUnschedulableModal.tsxfrontend/packages/console-app/src/components/nodes/nodeSchedulingActions.tsfrontend/packages/console-app/src/components/nodes/useCustomNodeActions.tsxfrontend/packages/console-dynamic-plugin-sdk/src/api/internal-api.tsfrontend/packages/console-dynamic-plugin-sdk/src/api/internal-types.tsfrontend/packages/console-shared/src/components/dropdown/ResponsiveActionDropdown.tsxfrontend/packages/console-shared/src/components/dropdown/__tests__/ResponsiveActionDropdown.spec.tsxfrontend/public/components/droppable-edit-yaml.tsxfrontend/public/kinds.tsfrontend/public/locales/en/public.jsonfrontend/public/style/ancillary/_bootstrap-residual.scss
✅ Files skipped from review due to trivial changes (3)
- frontend/public/kinds.ts
- frontend/public/style/ancillary/_bootstrap-residual.scss
- frontend/packages/console-app/locales/en/console-app.json
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
frontend/packages/console-app/src/components/nodes/useCustomNodeActions.tsx (1)
34-41:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRun completion callback regardless of success/failure.
onComplete()only runs on fulfilled promise; a rejected bulk request skips completion and can leave UI state stale. Move completion into.finally().Suggested patch
const handleMarkSchedulable = useCallback(() => { handlePromise(markNodesSchedulable(selectedNodes)) - .then(() => { - onComplete(); - }) .catch(() => { // Errors are handled by usePromiseHandler - }); + }) + .finally(() => { + onComplete(); + }); }, [selectedNodes, handlePromise, onComplete]);🤖 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 `@frontend/packages/console-app/src/components/nodes/useCustomNodeActions.tsx` around lines 34 - 41, The completion callback onComplete is only invoked on success in handleMarkSchedulable; move the onComplete() call into the promise finalization to ensure it runs regardless of success or failure. Update handleMarkSchedulable to call handlePromise(markNodesSchedulable(selectedNodes)).then(...).catch(...) and invoke onComplete() inside .finally() (or call onComplete directly from the finally handler) so UI state is always cleaned up after markNodesSchedulable resolves or rejects.frontend/packages/console-app/src/components/nodes/modals/ConfigureUnschedulableModal.tsx (1)
21-28:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent submit when no target nodes are provided.
Both
resourceandnodesare optional, sotargetNodescan be empty and still submit/close. Guard the submit handler and disable primary action when no targets exist.Suggested patch
const isBulk = targetNodes.length > 1; + const hasTargets = targetNodes.length > 0; const handleSubmit = (): void => { + if (!hasTargets) { + return; + } handlePromise(markNodesUnschedulable(targetNodes)) .then(() => { onComplete?.(); close(); }) // Errors are surfaced by usePromiseHandler/ModalFooterWithAlerts .catch(() => {}); }; @@ <Button type="button" variant="primary" onClick={handleSubmit} isLoading={inProgress} - isDisabled={inProgress} + isDisabled={inProgress || !hasTargets} >Also applies to: 58-66, 93-99
🤖 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 `@frontend/packages/console-app/src/components/nodes/modals/ConfigureUnschedulableModal.tsx` around lines 21 - 28, The modal currently allows submission when neither resource nor nodes are provided; update the submit handler used by ConfigureUnschedulableModal to early-return (no-op) if computed targetNodes is empty and ensure the modal's primary action (confirm/submit button) is disabled when targetNodes.length === 0. Locate the code that computes targetNodes from props.resource and props.nodes and add a guard in the submit function (and any onConfirm callback) to prevent closing/patching when no targets exist, and wire the button disabled prop to the same condition so the action is not clickable.frontend/packages/console-app/src/components/nodes/NodesPage.tsx (1)
571-575:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRender empty content (not DASH) when select cell is absent.
Line 574 still returns
DASHfor missingselectcells, so CSR rows show an em dash in the checkbox column.Suggested fix
if (!rowCell) { return { id, - cell: DASH, + cell: id === 'select' ? '' : DASH, }; }🤖 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 `@frontend/packages/console-app/src/components/nodes/NodesPage.tsx` around lines 571 - 575, The code path that handles a missing select cell currently returns { id, cell: DASH }, causing an em dash to appear in the checkbox column; change the return for the missing select cell (the branch checking rowCell) to return an empty cell instead (e.g., { id, cell: '' } or an empty React node) so the checkbox column renders empty rather than DASH; update the branch where rowCell, id and DASH are referenced to use an empty value for cell.frontend/packages/console-app/src/components/data-view/useConsoleDataViewData.tsx (1)
143-147:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse a dedicated SR label for the select column header (not “Actions”).
Line 146 labels empty-title headers as
Actions, which mislabels the select-all checkbox column for screen readers.Suggested fix
- ) : ( - <span className="pf-v6-u-screen-reader">{t('public~Actions')}</span> - ), + ) : ( + <span className="pf-v6-u-screen-reader"> + {id === 'select' ? t('public~Select all') : t('public~Actions')} + </span> + ),As per coding guidelines, "Follow WCAG 2.1 AA standards in TypeScript/React code; use semantic HTML, ARIA labels where needed, ensure keyboard navigation, and test with screen readers".
🤖 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 `@frontend/packages/console-app/src/components/data-view/useConsoleDataViewData.tsx` around lines 143 - 147, The header cell rendering in useConsoleDataViewData currently assigns an SR label of "Actions" when title is empty, which mislabels the selection/checkbox column; update the conditional in the cell render (in useConsoleDataViewData where cell: title ? <span>{title}</span> : <span className="pf-v6-u-screen-reader">{t('public~Actions')}</span>) to detect the select/checkbox column and provide a semantically correct SR label such as t('public~Select all') or use an aria-label on the checkbox element instead of "Actions" so screen readers correctly announce the select-all column.frontend/packages/console-app/src/components/data-view/ConsoleDataView.tsx (1)
184-206:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftExclude non-selectable rows from banner/select-all state calculations.
Line 184 and Line 206 treat all visible rows as selectable. On mixed pages (e.g., CSR + Nodes), this keeps select-all/banner state out of sync even when every selectable row is selected.
As per coding guidelines, "Follow WCAG 2.1 AA standards in TypeScript/React code; use semantic HTML, ARIA labels where needed, ensure keyboard navigation, and test with screen readers".
🤖 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 `@frontend/packages/console-app/src/components/data-view/ConsoleDataView.tsx` around lines 184 - 206, The banner/select-all logic treats every visible row as selectable; update calculations to exclude non-selectable rows by first deriving selectableVisible = visibleItems.filter(isSelectableRow) and selectableTotal = filteredData.filter(isSelectableRow) (or call the existing selection.isSelectable/getSelectable predicate if available), then compute visibleSelectedCount, visibleCount, totalCount, allVisibleSelected, isIndeterminate and allSelected using those selectable arrays (use selection.getItemId(item) when checking selection.selectedItems). Update shouldShow to use selectableVisible length and selectableTotal length so banner and indeterminate state reflect only selectable rows.frontend/packages/console-app/src/components/data-view/useDataViewSelection.ts (1)
75-85:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve existing selections when toggling page/filter bulk-select.
Line 82 and Line 84 replace/clear the whole
selectedIdsset, so selecting on one page/filter drops prior selections from others.Suggested fix
const onSelectAll = useCallback( (isSelecting: boolean, filteredItems: T[]) => { - if (isSelecting) { - const selectableItems = filterSelectable - ? filteredItems.filter(filterSelectable) - : filteredItems; - const itemIds = selectableItems.map(getItemId); - setSelectedIds(new Set(itemIds)); - } else { - setSelectedIds(new Set()); - } + const selectableItems = filterSelectable + ? filteredItems.filter(filterSelectable) + : filteredItems; + const itemIds = selectableItems.map(getItemId); + + setSelectedIds((prev) => { + const next = new Set(prev); + itemIds.forEach((id) => { + if (isSelecting) { + next.add(id); + } else { + next.delete(id); + } + }); + return next; + }); }, [getItemId, filterSelectable], );🤖 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 `@frontend/packages/console-app/src/components/data-view/useDataViewSelection.ts` around lines 75 - 85, In useDataViewSelection's onSelectAll handler, don't replace the whole selectedIds set; instead merge or remove only the IDs for the current filteredItems so selections from other pages/filters are preserved. When isSelecting is true compute selectableItems (respecting filterSelectable) and union their IDs with the existing selectedIds (use setSelectedIds with a prev-set merger). When isSelecting is false compute the same current-page IDs and remove those IDs from the existing selectedIds (use setSelectedIds with a prev-set and delete each current ID) rather than clearing the set. Reference: onSelectAll, filterSelectable, getItemId, setSelectedIds, selectedIds.
🤖 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
`@frontend/packages/console-shared/src/components/dropdown/__tests__/ResponsiveActionDropdown.spec.tsx`:
- Around line 12-19: The test mutates window.innerWidth in beforeAll and never
restores it and also doesn't clean up Jest mocks; capture the original value
(e.g., store const originalInnerWidth = window.innerWidth before overriding),
set window.innerWidth via Object.defineProperty as you already do, and add an
afterEach that restores window.innerWidth back to originalInnerWidth (using
Object.defineProperty) and calls jest.restoreAllMocks(); reference the existing
beforeAll and the mutated window.innerWidth so reviewers can locate and
implement the teardown.
---
Duplicate comments:
In `@frontend/packages/console-app/src/components/data-view/ConsoleDataView.tsx`:
- Around line 184-206: The banner/select-all logic treats every visible row as
selectable; update calculations to exclude non-selectable rows by first deriving
selectableVisible = visibleItems.filter(isSelectableRow) and selectableTotal =
filteredData.filter(isSelectableRow) (or call the existing
selection.isSelectable/getSelectable predicate if available), then compute
visibleSelectedCount, visibleCount, totalCount, allVisibleSelected,
isIndeterminate and allSelected using those selectable arrays (use
selection.getItemId(item) when checking selection.selectedItems). Update
shouldShow to use selectableVisible length and selectableTotal length so banner
and indeterminate state reflect only selectable rows.
In
`@frontend/packages/console-app/src/components/data-view/useConsoleDataViewData.tsx`:
- Around line 143-147: The header cell rendering in useConsoleDataViewData
currently assigns an SR label of "Actions" when title is empty, which mislabels
the selection/checkbox column; update the conditional in the cell render (in
useConsoleDataViewData where cell: title ? <span>{title}</span> : <span
className="pf-v6-u-screen-reader">{t('public~Actions')}</span>) to detect the
select/checkbox column and provide a semantically correct SR label such as
t('public~Select all') or use an aria-label on the checkbox element instead of
"Actions" so screen readers correctly announce the select-all column.
In
`@frontend/packages/console-app/src/components/data-view/useDataViewSelection.ts`:
- Around line 75-85: In useDataViewSelection's onSelectAll handler, don't
replace the whole selectedIds set; instead merge or remove only the IDs for the
current filteredItems so selections from other pages/filters are preserved. When
isSelecting is true compute selectableItems (respecting filterSelectable) and
union their IDs with the existing selectedIds (use setSelectedIds with a
prev-set merger). When isSelecting is false compute the same current-page IDs
and remove those IDs from the existing selectedIds (use setSelectedIds with a
prev-set and delete each current ID) rather than clearing the set. Reference:
onSelectAll, filterSelectable, getItemId, setSelectedIds, selectedIds.
In
`@frontend/packages/console-app/src/components/nodes/modals/ConfigureUnschedulableModal.tsx`:
- Around line 21-28: The modal currently allows submission when neither resource
nor nodes are provided; update the submit handler used by
ConfigureUnschedulableModal to early-return (no-op) if computed targetNodes is
empty and ensure the modal's primary action (confirm/submit button) is disabled
when targetNodes.length === 0. Locate the code that computes targetNodes from
props.resource and props.nodes and add a guard in the submit function (and any
onConfirm callback) to prevent closing/patching when no targets exist, and wire
the button disabled prop to the same condition so the action is not clickable.
In `@frontend/packages/console-app/src/components/nodes/NodesPage.tsx`:
- Around line 571-575: The code path that handles a missing select cell
currently returns { id, cell: DASH }, causing an em dash to appear in the
checkbox column; change the return for the missing select cell (the branch
checking rowCell) to return an empty cell instead (e.g., { id, cell: '' } or an
empty React node) so the checkbox column renders empty rather than DASH; update
the branch where rowCell, id and DASH are referenced to use an empty value for
cell.
In `@frontend/packages/console-app/src/components/nodes/useCustomNodeActions.tsx`:
- Around line 34-41: The completion callback onComplete is only invoked on
success in handleMarkSchedulable; move the onComplete() call into the promise
finalization to ensure it runs regardless of success or failure. Update
handleMarkSchedulable to call
handlePromise(markNodesSchedulable(selectedNodes)).then(...).catch(...) and
invoke onComplete() inside .finally() (or call onComplete directly from the
finally handler) so UI state is always cleaned up after markNodesSchedulable
resolves or rejects.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: afe2c636-81ba-45d8-92a7-b5b10aac3f99
📒 Files selected for processing (19)
frontend/packages/console-app/locales/en/console-app.jsonfrontend/packages/console-app/src/components/data-view/BULK_SELECTION_GUIDE.mdfrontend/packages/console-app/src/components/data-view/ConsoleDataView.tsxfrontend/packages/console-app/src/components/data-view/dataViewSelectionHelpers.tsfrontend/packages/console-app/src/components/data-view/useConsoleDataViewData.tsxfrontend/packages/console-app/src/components/data-view/useDataViewSelection.tsfrontend/packages/console-app/src/components/nodes/NodesPage.tsxfrontend/packages/console-app/src/components/nodes/menu-actions.tsxfrontend/packages/console-app/src/components/nodes/modals/ConfigureUnschedulableModal.tsxfrontend/packages/console-app/src/components/nodes/nodeSchedulingActions.tsfrontend/packages/console-app/src/components/nodes/useCustomNodeActions.tsxfrontend/packages/console-dynamic-plugin-sdk/src/api/internal-api.tsfrontend/packages/console-dynamic-plugin-sdk/src/api/internal-types.tsfrontend/packages/console-shared/src/components/dropdown/ResponsiveActionDropdown.tsxfrontend/packages/console-shared/src/components/dropdown/__tests__/ResponsiveActionDropdown.spec.tsxfrontend/public/components/droppable-edit-yaml.tsxfrontend/public/kinds.tsfrontend/public/locales/en/public.jsonfrontend/public/style/ancillary/_bootstrap-residual.scss
✅ Files skipped from review due to trivial changes (3)
- frontend/packages/console-dynamic-plugin-sdk/src/api/internal-api.ts
- frontend/public/kinds.ts
- frontend/packages/console-app/src/components/data-view/BULK_SELECTION_GUIDE.md
|
/assign @jseseCCS |
|
@rhamilto: GitHub didn't allow me to assign the following users: rh-joshbeverly. Note that only openshift members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. 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. |
|
/label px-approved |
|
/retest |
There was a problem hiding this comment.
Is this a leftover from your 6.5 commit?
There was a problem hiding this comment.
Good catch — yes, this was a stray change unrelated to this PR. Reverted.
|
/retest |
1 similar comment
|
/retest |
jseseCCS
left a comment
jseseCCS
left a comment
There was a problem hiding this comment.
approving in good faith that you'll consider these updates, apply them as appropriate before merging. Thanks!
| "This action cannot be undone. Deleting a node will instruct Kubernetes that the node is down or unrecoverable and delete all pods scheduled to that node. If the node is still running but unresponsive and the node is deleted, stateful workloads and persistent volumes may suffer corruption or data loss. Only delete a node that you have confirmed is completely stopped and cannot be restored.": "This action cannot be undone. Deleting a node will instruct Kubernetes that the node is down or unrecoverable and delete all pods scheduled to that node. If the node is still running but unresponsive and the node is deleted, stateful workloads and persistent volumes may suffer corruption or data loss. Only delete a node that you have confirmed is completely stopped and cannot be restored.", | ||
| "Mark as schedulable": "Mark as schedulable", | ||
| "Mark as unschedulable": "Mark as unschedulable", | ||
| "Mark <1>{{count}}</1> nodes as unschedulable?": "Mark <1>{{count}}</1> nodes as unschedulable?", |
There was a problem hiding this comment.
368: question mark inconsistent w/other action labels here. remove for consistency?
There was a problem hiding this comment.
This is in the context of the modal, so it probably makes sense to keep it.
| "Certificate approval required": "Certificate approval required", | ||
| "An error occurred. Please try again": "An error occurred. Please try again", | ||
| "No new Pods or workloads will be placed on this Node until it's marked as schedulable.": "No new Pods or workloads will be placed on this Node until it's marked as schedulable.", | ||
| "Applies to {{nodeCount}} selected node(s) that are currently unschedulable.": "Applies to {{nodeCount}} selected node(s) that are currently unschedulable.", |
There was a problem hiding this comment.
480: "node(s)" is awkward in UI text and RH style essentially forbids "anything(s)," says to go ahead and commit to plural to cover anything.
also, bc {{nodeCount}} already conveys number, suggest: "Applies to {{nodeCount}} selected nodes that are currently unschedulable."
| "No new Pods or workloads will be placed on this Node until it's marked as schedulable.": "No new Pods or workloads will be placed on this Node until it's marked as schedulable.", | ||
| "Applies to {{nodeCount}} selected node(s) that are currently unschedulable.": "Applies to {{nodeCount}} selected node(s) that are currently unschedulable.", | ||
| "Mark schedulable": "Mark schedulable", | ||
| "Applies to {{nodeCount}} selected node(s) that are currently schedulable.": "Applies to {{nodeCount}} selected node(s) that are currently schedulable.", |
There was a problem hiding this comment.
482: same as 480. suggest: "Applies to {{nodeCount}} selected nodes that are currently schedulable."
| "Applies to {{nodeCount}} selected node(s) that are currently unschedulable.": "Applies to {{nodeCount}} selected node(s) that are currently unschedulable.", | ||
| "Mark schedulable": "Mark schedulable", | ||
| "Applies to {{nodeCount}} selected node(s) that are currently schedulable.": "Applies to {{nodeCount}} selected node(s) that are currently schedulable.", | ||
| "Scheduling": "Scheduling", |
There was a problem hiding this comment.
483: Confirming "Scheduling" intended as section heading/column header. if so, fine.
| "All {{count}} {{label}} on this page are selected._other": "All {{count}} {{label}} on this page are selected.s", | ||
| "All <1>{{count}}</1> matching {{label}} are selected.": "All <1>{{count}}</1> matching {{label}} are selected.", | ||
| "items": "items", | ||
| "Unselect all": "Unselect all", |
There was a problem hiding this comment.
1780: Per RH conventions, this should be "Clear all" instead of "Unselect all"
| @@ -66,7 +66,7 @@ export const useNodeActions: ExtensionHook<Action[], NodeKind> = (obj) => { | |||
| actions.push({ | |||
| id: 'mark-as-schedulable', | |||
| label: t('console-app~Mark as schedulable'), | |||
There was a problem hiding this comment.
change UI label string from "Mark as schedulable" to "Allow scheduling" or "Enable scheduling"?
"Schedulable" is clunky, jargony. active verb phrases like "Allow scheduling" or "Enable scheduling" are clearer, better tone. if this action requires corresponding "Disable scheduling," please update elsewhere for consistency.
There was a problem hiding this comment.
Don't disagree, but these are the labels we're already using.
| itemsPerPage: t('public~Items per page'), | ||
| perPageSuffix: t('public~per page'), | ||
| toFirstPageAriaLabel: t('public~Go to first page'), | ||
| optionsToggleAriaLabel: t('public~Items per page'), |
There was a problem hiding this comment.
change from "Items per page" to "Select items per page"? active verb phrase is clearer, more actionable instruction for screen-readers.
There was a problem hiding this comment.
This is for pagination and not selection. Was missing before.
| } | ||
| }, [selection, filteredData]); | ||
|
|
||
| const dataViewFilterNodes = useMemo<React.ReactNode[]>(() => { |
There was a problem hiding this comment.
aria-label={t('public~Column management')} being removed here along w/old code block, yes? confirm that screen-reader label moved to new layout further down/elsewhere so we don't drop accessibility coverage for col management feature.
There was a problem hiding this comment.
Was just moved further down the file.
| <Trans ns="public" i18nKey="All <1>{{count}}</1> matching {{label}} are selected."> | ||
| All <strong>{{ count: filteredData.length }}</strong> matching{' '} | ||
| {{ label: label || t('public~items') }} are selected. | ||
| </Trans>{' '} |
There was a problem hiding this comment.
330-333 AND 342-346: update translation strings to support conditional pluralization keys instead of hard-coding plural structure? as you know, if selection count equals 1, UI will display grammatically incorrect strings like "All 1 matching items are selected."
| <Trans ns="public" i18nKey="Select all <1>{{count}}</1> matching {{label}}"> | ||
| Select all <strong>{{ count: filteredData.length }}</strong> matching{' '} | ||
| {{ label: label || t('public~items') }} | ||
| </Trans> |
There was a problem hiding this comment.
347-351: add period to end of text string inside i18nKey so it reads "Select all <1>{{count}}</1> matching {{label}}." + make sure it handles singular/plural states.
|
/label docs-approved |
Adds bulk selection support to ConsoleDataView and implements schedulable/unschedulable bulk actions on the Nodes page. New components and hooks: - useDataViewSelection: Generic selection state management hook - dataViewSelectionHelpers: Column/cell helpers for checkbox selection - ResponsiveActionDropdown: Breakpoint-aware dropdown (button on desktop, kebab on mobile) - nodeSchedulingActions: Shared scheduling logic for single and bulk node operations - useCustomNodeActions: Bulk scheduling actions dropdown for Nodes page ConsoleDataView enhancements: - selection prop for checkbox column with select-all header - additionalActions/customActions props for toolbar actions - "Select all matching" banner with screenReaderText for a11y - Uses PatternFly isIndeterminate prop for partial selection state Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/retest |
1 similar comment
|
/retest |
|
/test e2e-gcp-console |
QA Verification Evidence
Verification Steps
Animated overview (click to expand)
Step 1: Nodes list (checkboxes visible in candidate) (pass)
Step 2: Node details (regression check) (pass)
Step 3: Single node selected with bulk actions (pass) — candidate only
Step 4: All nodes selected with banner (pass) — candidate only
Step 5: Bulk actions dropdown menu (pass) — candidate only
Warning This verification was performed by an AI agent. Results may contain false positives or miss Automated QA verification by Claude Code |
|
/verified by @jhadvig |
|
@jhadvig: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhadvig, jseseCCS, rhamilto The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@rhamilto: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
9 participants
Summary
Implements row selection with checkboxes for the Nodes list page, allowing users to select multiple nodes and perform bulk actions to mark them as schedulable or unschedulable.
This PR also introduces a new ResponsiveActionDropdown shared component that provides a responsive action menu pattern, automatically switching between a primary button (desktop) and kebab menu (mobile) based on breakpoint.
Demo
Screen.Recording.2026-05-05.at.2.21.48.PM.mov
Changes
Bulk Selection & Actions
isIndeterminateprop)useDataViewSelection,dataViewSelectionHelpersNew Shared Component: ResponsiveActionDropdown
@console/shared/src/components/dropdown/ResponsiveActionDropdownOverflowMenufor automatic breakpoint detectionprimary,secondary, etc.)Features
Promise.alland lets the Kubernetes API surface its own error messages viausePromiseHandlerTranscomponent for interpolated contentnameCellPropscontinues to work unchangedSelection Behavior
When working with filters:
Implementation Details
Bulk Selection
isIndeterminateprop on the select-all header checkbox for partial selection stateuseDataViewSelectionhook manages selection state with auto-cleanup of stale selectionsonFilteredSelectionChangecallback notifies parent of filtered selection changesSet<string>for efficient lookupsName Column Offset Helpers
SELECTION_COLUMN_WIDTHconstant ('45px')selectionColumnPropsfor the selection columngetNameColumnProps(hasRightBorder?, withBulkSelect?)— sticky column props with optional offsetgetNameCellProps(name, withBulkSelect?)— backward compatible (defaults tofalse)nameCellPropsexport for existing codeSimplified Column Update Logic
dataViewColumnsWithSortAppliedrefactored from 4 mutually exclusive branches to composable independent updates usinglet updatedPropspattern — same behavior, half the codeResponsiveActionDropdown
OverflowMenuwith configurablebreakpointpropInternalDropdowncomponent consumesOverflowMenuContextforisBelowBreakpointMenuTogglevariant and content based on breakpointOverflowMenuContextfrom PatternFly internal path (not in public API yet)Test plan