CONSOLE-4838: Improvements to Console DataView code #15713
Conversation
openshift-merge-robot
added
the
needs-rebase
openshift-ci
bot
added
component/core
vojtechszocs
force-pushed
the
data-view-improvements
branch
from
dd8bd06 to
341fbf3
Compare
openshift-merge-robot
removed
the
needs-rebase
| * Maps Console `RowProps` data to DataView compatible format. | ||
| */ | ||
| export type GetDataViewRows<TData, TCustomRowData> = ( | ||
| export type GetDataViewRows<TData, TCustomRowData = any> = ( |
There was a problem hiding this comment.
Using any instead of unknown for compatibility with existing RowProps signature
RowProps<D, R extends any = {}>
openshift-ci-robot
added
the
jira/valid-reference
|
@vojtechszocs: This pull request references CONSOLE-4838 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 story to target the "4.21.0" version, but no target version was set. In 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. |
|
/lgtm |
|
This PR contains code improvements, no impact on existing functionality or docs. /label px-approved |
openshift-ci
bot
added
px-approved
openshift-merge-robot
added
the
needs-rebase
vojtechszocs
force-pushed
the
data-view-improvements
branch
from
341fbf3 to
c50c9f5
Compare
WalkthroughConsoleDataView now exposes an exported Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes Areas requiring extra attention:
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 0
🧹 Nitpick comments (5)
frontend/public/components/template-instance.tsx (1)
167-167: Consider defininginitialFiltersas a module-level constant.Since
initialFiltershas an empty dependency array and combines static values, it could be defined outside the component as a constant to avoid the memoization overhead entirely:+const initialFilters = { ...initialFiltersDefault, status: [] }; + export const TemplateInstanceList: React.FC<TemplateInstanceListProps> = ({ data, loaded, ...props }) => { const { t } = useTranslation(); const columns = useTemplateInstanceColumns(); const templateInstanceStatusFilterOptions = React.useMemo<DataViewFilterOption[]>(() => { // ... }, [t]); - const initialFilters = React.useMemo(() => ({ ...initialFiltersDefault, status: [] }), []);frontend/public/components/cluster-settings/cluster-operator.tsx (1)
218-218: Consider defininginitialFiltersas a module-level constant.Similar to the template-instance component, this
initialFilterscould be a module-level constant instead of using memoization:+const initialFilters = { ...initialFiltersDefault, status: [] }; + export const ClusterOperatorList: React.FC<ClusterOperatorListProps> = ({ data, loaded, ...props }) => { const { t } = useTranslation(); const columns = useClusterOperatorColumns(); const clusterOperatorStatusFilterOptions = React.useMemo<DataViewFilterOption[]>(() => { // ... }, [t]); - const initialFilters = React.useMemo(() => ({ ...initialFiltersDefault, status: [] }), []);frontend/public/components/pod-list.tsx (1)
512-512: Consider defininginitialFiltersas a module-level constant.Consistent with the pattern in other files, this could be a module-level constant to avoid memoization overhead:
+const initialFilters = { ...initialFiltersDefault, status: [] }; + export const PodList: FC<PodListProps> = ({ showNamespaceOverride, showNodes, // ... }) => { const { t } = useTranslation(); const columns = usePodsColumns(showNodes); const podMetrics = useSelector<RootState, UIActions.PodMetrics>(({ UI }) => { return UI.getIn(['metrics', 'pod']); }); const columnManagementID = referenceForModel(PodModel); const columnLayout = useMemo<ColumnLayout>( // ... ); const podStatusFilterOptions = useMemo<DataViewFilterOption[]>( // ... ); - const initialFilters = useMemo(() => ({ ...initialFiltersDefault, status: [] }), []);frontend/public/components/secret.tsx (1)
52-96: StrongerGetDataViewRows<SecretKind>typing is good; consider propagating typesConfining
getDataViewRowstoGetDataViewRows<SecretKind>matches how the rows are built and improves safety without changing behavior. To fully benefit, consider (optionally) tightening the surrounding types by:
- Typing
useSecretsColumnsasTableColumn<SecretKind>[], and- Instantiating
<ConsoleDataView<SecretKind>>withSecretsListProps['data']refined toSecretKind[].This would make the whole list pipeline consistently typed around
SecretKind.frontend/public/components/RBAC/bindings.tsx (1)
317-317: Consider simplifying the type parameters.The second type parameter
undefinedcan now be omitted sinceGetDataViewRowsdefaults it toany. The type could be simplified toConsoleDataView<BindingKind, BindingFilters>.Apply this diff to simplify:
- <ConsoleDataView<BindingKind, undefined, BindingFilters> + <ConsoleDataView<BindingKind, BindingFilters>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (53)
frontend/packages/console-app/src/components/data-view/ConsoleDataView.tsx(2 hunks)frontend/packages/console-app/src/components/data-view/types.ts(1 hunks)frontend/packages/console-app/src/components/nodes/NodesPage.tsx(2 hunks)frontend/packages/console-app/src/components/pdb/PDBList.tsx(1 hunks)frontend/packages/helm-plugin/src/components/details-page/resources/HelmReleaseResourcesList.tsx(1 hunks)frontend/packages/helm-plugin/src/components/details-page/resources/HelmReleaseResourcesRow.tsx(1 hunks)frontend/packages/helm-plugin/src/components/details-page/resources/__tests__/HelmReleaseResourcesRow.spec.tsx(1 hunks)frontend/packages/helm-plugin/src/components/list-page/HelmChartRepositoryList.tsx(1 hunks)frontend/packages/helm-plugin/src/components/list-page/HelmChartRepositoryRow.tsx(1 hunks)frontend/packages/helm-plugin/src/components/list-page/HelmReleaseList.tsx(2 hunks)frontend/packages/helm-plugin/src/components/list-page/ProjectHelmChartRepositoryList.tsx(1 hunks)frontend/packages/helm-plugin/src/components/list-page/ProjectHelmChartRepositoryRow.tsx(1 hunks)frontend/packages/helm-plugin/src/components/list-page/RepositoriesList.tsx(1 hunks)frontend/packages/helm-plugin/src/components/list-page/RepositoriesRow.tsx(1 hunks)frontend/public/components/RBAC/bindings.tsx(3 hunks)frontend/public/components/RBAC/role.jsx(2 hunks)frontend/public/components/alert-manager.tsx(1 hunks)frontend/public/components/build-config.tsx(1 hunks)frontend/public/components/build.tsx(1 hunks)frontend/public/components/cluster-settings/cluster-operator.tsx(4 hunks)frontend/public/components/cluster-settings/related-objects.tsx(0 hunks)frontend/public/components/configmap.tsx(1 hunks)frontend/public/components/control-plane-machine-set.tsx(2 hunks)frontend/public/components/cron-job.tsx(1 hunks)frontend/public/components/custom-resource-definition.tsx(1 hunks)frontend/public/components/daemon-set.tsx(2 hunks)frontend/public/components/default-resource.tsx(1 hunks)frontend/public/components/deployment-config.tsx(2 hunks)frontend/public/components/deployment.tsx(2 hunks)frontend/public/components/group.tsx(1 hunks)frontend/public/components/hpa.tsx(1 hunks)frontend/public/components/image-stream.tsx(1 hunks)frontend/public/components/job.tsx(1 hunks)frontend/public/components/limit-range.tsx(1 hunks)frontend/public/components/machine-autoscaler.tsx(1 hunks)frontend/public/components/machine-config-pool.tsx(0 hunks)frontend/public/components/machine-config.tsx(1 hunks)frontend/public/components/machine-health-check.tsx(1 hunks)frontend/public/components/machine-set.tsx(0 hunks)frontend/public/components/machine.tsx(0 hunks)frontend/public/components/monitoring/alertmanager/alertmanager-config.tsx(0 hunks)frontend/public/components/namespace.jsx(4 hunks)frontend/public/components/pod-list.tsx(5 hunks)frontend/public/components/prometheus.tsx(2 hunks)frontend/public/components/replicaset.jsx(0 hunks)frontend/public/components/replication-controller.jsx(0 hunks)frontend/public/components/resource-quota.jsx(0 hunks)frontend/public/components/secret.tsx(1 hunks)frontend/public/components/service-account.jsx(0 hunks)frontend/public/components/service-monitor.jsx(0 hunks)frontend/public/components/stateful-set.tsx(1 hunks)frontend/public/components/template-instance.tsx(4 hunks)frontend/public/components/user.tsx(1 hunks)
💤 Files with no reviewable changes (10)
- frontend/public/components/machine-set.tsx
- frontend/public/components/resource-quota.jsx
- frontend/public/components/replication-controller.jsx
- frontend/public/components/machine-config-pool.tsx
- frontend/public/components/service-account.jsx
- frontend/public/components/service-monitor.jsx
- frontend/public/components/cluster-settings/related-objects.tsx
- frontend/public/components/monitoring/alertmanager/alertmanager-config.tsx
- frontend/public/components/machine.tsx
- frontend/public/components/replicaset.jsx
🧰 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/console-app/src/components/data-view/types.tsfrontend/packages/helm-plugin/src/components/list-page/RepositoriesRow.tsxfrontend/packages/helm-plugin/src/components/details-page/resources/HelmReleaseResourcesRow.tsxfrontend/packages/helm-plugin/src/components/list-page/ProjectHelmChartRepositoryRow.tsxfrontend/public/components/secret.tsxfrontend/public/components/stateful-set.tsxfrontend/packages/console-app/src/components/nodes/NodesPage.tsxfrontend/packages/helm-plugin/src/components/list-page/HelmChartRepositoryList.tsxfrontend/public/components/build-config.tsxfrontend/packages/console-app/src/components/data-view/ConsoleDataView.tsxfrontend/packages/helm-plugin/src/components/list-page/HelmChartRepositoryRow.tsxfrontend/packages/helm-plugin/src/components/list-page/HelmReleaseList.tsxfrontend/public/components/machine-health-check.tsxfrontend/public/components/build.tsxfrontend/public/components/RBAC/role.jsxfrontend/public/components/namespace.jsxfrontend/public/components/deployment-config.tsxfrontend/public/components/default-resource.tsxfrontend/public/components/machine-config.tsxfrontend/public/components/job.tsxfrontend/packages/helm-plugin/src/components/list-page/RepositoriesList.tsxfrontend/public/components/machine-autoscaler.tsxfrontend/public/components/daemon-set.tsxfrontend/public/components/hpa.tsxfrontend/public/components/limit-range.tsxfrontend/packages/helm-plugin/src/components/details-page/resources/HelmReleaseResourcesList.tsxfrontend/public/components/pod-list.tsxfrontend/public/components/deployment.tsxfrontend/public/components/cluster-settings/cluster-operator.tsxfrontend/public/components/control-plane-machine-set.tsxfrontend/public/components/configmap.tsxfrontend/public/components/image-stream.tsxfrontend/public/components/custom-resource-definition.tsxfrontend/public/components/alert-manager.tsxfrontend/packages/helm-plugin/src/components/list-page/ProjectHelmChartRepositoryList.tsxfrontend/public/components/template-instance.tsxfrontend/packages/helm-plugin/src/components/details-page/resources/__tests__/HelmReleaseResourcesRow.spec.tsxfrontend/public/components/RBAC/bindings.tsxfrontend/packages/console-app/src/components/pdb/PDBList.tsxfrontend/public/components/user.tsxfrontend/public/components/prometheus.tsxfrontend/public/components/cron-job.tsxfrontend/public/components/group.tsx
🔇 Additional comments (49)
frontend/public/components/alert-manager.tsx (1)
126-126: LGTM! Clean type simplification.The removal of the second generic type parameter aligns with the broader refactor that adds a default type parameter fallback to
GetDataViewRows. This reduces boilerplate while maintaining the same functionality.frontend/public/components/cron-job.tsx (2)
93-93: LGTM: Type signature simplified correctly.The removal of the second type parameter aligns with the PR's objective to add a default fallback to
GetDataViewRows. The function implementation remains correct and consistent with the new type signature.
369-385: LGTM: ConsoleDataView usage correctly updated.The removal of the
initialFiltersprop is consistent with it now being optional inConsoleDataView. All required props are properly provided, and the component is correctly wrapped in a Suspense boundary.frontend/public/components/custom-resource-definition.tsx (2)
300-300: LGTM! Clean type signature simplification.The removal of the explicit
undefinedsecond generic parameter and the simplified function signature correctly align with the PR's API improvements. The function now leverages the default type parameter fallback inGetDataViewRows.
363-371: LGTM! ConsoleDataView usage correctly updated.The removal of the
initialFiltersprop is appropriate since it's now optional per the PR objectives. All required props are properly provided and the component usage is consistent with the refactored API.frontend/public/components/default-resource.tsx (3)
167-174: LGTM! Type signature simplification improves code clarity.The removal of the explicit
undefinedsecond generic parameter fromRowProps<K8sResourceKind, undefined>[]toRowProps<K8sResourceKind>[]leverages the new type parameter fallback, reducing boilerplate while maintaining type safety.
35-40: LGTM! Clean removal of unused import.The removal of
initialFiltersDefaultfrom the import statement is correct and aligns with the prop now being optional with an internal default inConsoleDataView.
338-355: LGTM! Simplified component usage aligns with API improvement.The removal of the explicit
initialFiltersprop is appropriate now thatConsoleDataViewhandles this internally with an optional prop and default value. This simplifies the component usage while maintaining the same functionality.frontend/public/components/hpa.tsx (2)
266-266: LGTM! Type signature simplification.The removal of the second generic parameter (previously
undefined) makes the type signature cleaner and more concise, aligning with the updatedGetDataViewRowstype definition that now provides a default fallback.
387-407: LGTM! Clean integration with optionalinitialFilters.The component correctly uses
ConsoleDataViewwithout explicitly passinginitialFilters, relying on the new optional prop behavior. This simplifies the component while maintaining the same functionality.frontend/public/components/control-plane-machine-set.tsx (2)
279-279: LGTM! Type signature simplified correctly.The removal of the second generic parameter aligns with the PR's objective to provide a default type parameter fallback for
GetDataViewRows. This simplification improves code clarity without affecting functionality.
337-337: LGTM! Type signature simplified correctly.The removal of the second generic parameter aligns with the PR's objective to make
initialFiltersoptional inConsoleDataView. This simplification maintains functionality while improving code clarity.frontend/public/components/namespace.jsx (2)
425-425: LGTM! Performance optimization through memoization.The memoized
initialFiltersobject prevents unnecessary object recreation on each render and provides a stable reference toConsoleDataView, which can prevent unnecessary child re-renders. The empty dependency array is appropriate sinceinitialFiltersDefaultis an imported constant.Also applies to: 469-469
807-807: LGTM! Consistent pattern applied.The same memoization pattern is applied consistently in
ProjectList, matching the approach inNamespacesList. This provides the same performance benefits and maintains code consistency across both list components.Also applies to: 853-853
frontend/packages/console-app/src/components/nodes/NodesPage.tsx (1)
548-551: StableinitialFiltersmemoization and wiring intoConsoleDataViewlook correctDeriving
initialFiltersonce with a memoized object that extendsinitialFiltersDefaultand then passing that reference intoConsoleDataViewaligns with the intended filter shape (NodeFilters) and should prevent unnecessary filter resets or re-renders; no issues spotted here.Also applies to: 630-630
frontend/packages/helm-plugin/src/components/list-page/ProjectHelmChartRepositoryRow.tsx (1)
31-31: LGTM! Type signature simplification aligns with the PR-wide refactor.The removal of the explicit
undefinedsecond type parameter is consistent with the updatedGetDataViewRowstype definition that now provides a default type parameter fallback.frontend/public/components/job.tsx (1)
78-78: LGTM! Type signature simplified correctly.This change aligns with the PR-wide refactor to use the default type parameter for
GetDataViewRows.frontend/public/components/image-stream.tsx (1)
349-349: LGTM! Type signature update is consistent with the PR refactor.frontend/public/components/machine-config.tsx (1)
142-142: LGTM! Type signature simplified appropriately.frontend/public/components/build-config.tsx (1)
212-212: LGTM! Type signature update is correct.frontend/public/components/prometheus.tsx (1)
20-86: TypedgetDataViewRowsalignment withGetDataViewRows<K8sResourceKind>looks correctImporting
GetDataViewRowsand constraininggetDataViewRowstoGetDataViewRows<K8sResourceKind>matches the existing implementation shape ((data, columns) => [...]) without altering behavior, and improves type clarity arounddataandcolumns.frontend/packages/helm-plugin/src/components/details-page/resources/__tests__/HelmReleaseResourcesRow.spec.tsx (1)
8-35: RowProps generic update is consistent with new defaulted type parameterDropping the explicit
undefinedfromRowProps<K8sResourceKind, undefined>[]toRowProps<K8sResourceKind>[]matches the new RowProps signature while keeping the existing test data (rowData: undefined) valid, with no behavioral impact on the tests.frontend/packages/helm-plugin/src/components/list-page/ProjectHelmChartRepositoryList.tsx (1)
3-25: Removal of explicitinitialFilterswiring in favor of ConsoleDataView defaults looks safeImporting only
ConsoleDataViewand omittinginitialFilterson the JSX keeps the list configuration minimal while delegating to the component’s default filter setup; the rest of the props are unchanged, so list behavior should remain consistent.frontend/packages/helm-plugin/src/components/list-page/HelmChartRepositoryList.tsx (1)
3-25: ConsoleDataView import cleanup and reliance on default filters is appropriateUsing only
ConsoleDataViewand dropping the explicitinitialFiltersprop aligns this list with the updated data-view API, reducing configuration noise while preserving the rest of the list behavior.frontend/packages/helm-plugin/src/components/list-page/HelmReleaseList.tsx (1)
196-196: LGTM! Well-implemented memoization pattern.The memoized
initialFilterscorrectly merges the default filters with a custom status filter array, preventing unnecessary re-renders while maintaining the existing filter behavior.Also applies to: 267-267
frontend/packages/helm-plugin/src/components/details-page/resources/HelmReleaseResourcesList.tsx (1)
3-3: LGTM! Clean simplification.Removing the unused
initialFiltersDefaultimport and prop aligns with makinginitialFiltersoptional inConsoleDataView. The component now relies on default filtering behavior.frontend/packages/helm-plugin/src/components/list-page/RepositoriesList.tsx (1)
3-3: LGTM! Consistent refactoring.The removal of
initialFiltersDefaultusage aligns with the PR's goal of makinginitialFiltersoptional. The component now uses default filtering behavior.frontend/public/components/RBAC/role.jsx (1)
457-457: LGTM! Appropriate memoization for custom filters.The
RolesListcomponent correctly memoizesinitialFiltersto include the customrole-kindfilter array, whileBindingsListComponentnow relies on default filtering. The pattern is consistent with the broader refactoring effort.Also applies to: 493-493
frontend/packages/helm-plugin/src/components/list-page/RepositoriesRow.tsx (1)
31-31: LGTM! Type signature simplified correctly.The updated type signature leverages the new default type parameter for
GetDataViewRows, eliminating the need to explicitly specifyundefinedfor the second generic parameter.frontend/public/components/configmap.tsx (1)
39-39: LGTM! Clean refactoring on multiple fronts.The changes combine type signature simplification with the removal of explicit
initialFiltersusage, both aligning with the PR's improvements to the data view system.frontend/packages/console-app/src/components/data-view/types.ts (1)
26-26: LGTM! Core type improvement with backward compatibility.Adding the default type parameter
TCustomRowData = anyenables all downstream components to omit the second generic parameter when not needed. The use ofanymaintains compatibility with the existingRowPropssignature as noted in the previous review comment.frontend/public/components/machine-health-check.tsx (1)
36-36: LGTM! Consistent with PR-wide improvements.The type signature update and removal of explicit
initialFiltersusage align perfectly with the refactoring pattern applied across the codebase, simplifying the data view implementation without functional changes.frontend/packages/helm-plugin/src/components/details-page/resources/HelmReleaseResourcesRow.tsx (1)
34-34: LGTM! Type signature simplified.The removal of the second generic parameter improves ergonomics while maintaining type safety, as
GetDataViewRowsnow defaults the second type parameter toany.frontend/public/components/user.tsx (1)
73-73: LGTM! Simplified type signature.The type signature correctly removes the explicit
undefinedparameter, relying on the new default behavior ofGetDataViewRows.frontend/public/components/RBAC/bindings.tsx (2)
193-193: LGTM! Type signature simplified.The removal of the explicit
undefinedparameter aligns with the newGetDataViewRowsdefault behavior.
269-270: Good use of memoized custom filters.Correctly extends
initialFiltersDefaultwith the customrole-kindfilter for this component's specific filtering needs.frontend/packages/helm-plugin/src/components/list-page/HelmChartRepositoryRow.tsx (1)
26-26: LGTM! Type signature simplified.Consistent with the broader refactor to make the second generic parameter optional.
frontend/packages/console-app/src/components/data-view/ConsoleDataView.tsx (3)
29-30: Good addition of default constant.Exporting
initialFiltersDefaultallows components that need to extend the default filters (like bindings.tsx) to do so consistently.
43-43: LGTM! Prop now optional.Making
initialFiltersoptional improves the API ergonomics while maintaining backward compatibility.
109-112: Proper default handling.The memoized fallback to
initialFiltersDefaultcorrectly implements the optional prop behavior, and the cast toTFiltersis necessary for type safety.frontend/public/components/machine-autoscaler.tsx (1)
64-64: LGTM! Type signature simplified.Consistent with the refactor pattern across the codebase.
frontend/public/components/deployment-config.tsx (1)
299-299: LGTM! Type signature simplified.Consistent refactor removing the explicit
undefinedparameter.frontend/public/components/stateful-set.tsx (1)
23-23: LGTM! Clean import.The import is consistent with other files in the refactor.
frontend/packages/console-app/src/components/pdb/PDBList.tsx (1)
33-33: LGTM! Type signature simplified correctly.The update from
GetDataViewRows<PodDisruptionBudgetKind, undefined>toGetDataViewRows<PodDisruptionBudgetKind>aligns with the PR's goal to add a default type parameter fallback forTCustomRowData.frontend/public/components/deployment.tsx (1)
29-29: LGTM! Clean API simplification.The removal of
initialFiltersDefaultand the simplifiedGetDataViewRows<DeploymentKind>type signature are consistent with makinginitialFiltersoptional in theConsoleDataViewcomponent.Also applies to: 223-223
frontend/public/components/daemon-set.tsx (1)
9-9: LGTM! Consistent with the PR-wide refactoring.The changes follow the same pattern of simplifying the type signature and removing explicit
initialFiltersDefaultusage, which improves API ergonomics.Also applies to: 32-32
frontend/public/components/group.tsx (1)
31-31: LGTM! Type signature correctly simplified.The update to
GetDataViewRows<GroupKind>and the removal of unused type imports streamline the code while maintaining existing functionality.Also applies to: 36-36
frontend/public/components/build.tsx (1)
360-360: LGTM! Type signature updated correctly.The simplified
GetDataViewRows<K8sResourceKind>type aligns with the PR's objective to add a default type parameter fallback.frontend/public/components/limit-range.tsx (1)
24-24: LGTM! Consistent refactoring applied.The changes to import statements and type signature follow the same clean pattern applied across all reviewed files, successfully simplifying the DataView API.
Also applies to: 32-32
vojtechszocs
force-pushed
the
data-view-improvements
branch
from
c50c9f5 to
f61b13e
Compare
openshift-merge-robot
removed
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (53)
frontend/packages/console-app/src/components/data-view/ConsoleDataView.tsx(2 hunks)frontend/packages/console-app/src/components/data-view/types.ts(1 hunks)frontend/packages/console-app/src/components/nodes/NodesPage.tsx(2 hunks)frontend/packages/console-app/src/components/pdb/PDBList.tsx(1 hunks)frontend/packages/helm-plugin/src/components/details-page/resources/HelmReleaseResourcesList.tsx(1 hunks)frontend/packages/helm-plugin/src/components/details-page/resources/HelmReleaseResourcesRow.tsx(1 hunks)frontend/packages/helm-plugin/src/components/details-page/resources/__tests__/HelmReleaseResourcesRow.spec.tsx(1 hunks)frontend/packages/helm-plugin/src/components/list-page/HelmChartRepositoryList.tsx(1 hunks)frontend/packages/helm-plugin/src/components/list-page/HelmChartRepositoryRow.tsx(1 hunks)frontend/packages/helm-plugin/src/components/list-page/HelmReleaseList.tsx(2 hunks)frontend/packages/helm-plugin/src/components/list-page/ProjectHelmChartRepositoryList.tsx(1 hunks)frontend/packages/helm-plugin/src/components/list-page/ProjectHelmChartRepositoryRow.tsx(1 hunks)frontend/packages/helm-plugin/src/components/list-page/RepositoriesList.tsx(1 hunks)frontend/packages/helm-plugin/src/components/list-page/RepositoriesRow.tsx(1 hunks)frontend/public/components/RBAC/bindings.tsx(3 hunks)frontend/public/components/RBAC/role.jsx(2 hunks)frontend/public/components/alert-manager.tsx(1 hunks)frontend/public/components/build-config.tsx(1 hunks)frontend/public/components/build.tsx(1 hunks)frontend/public/components/cluster-settings/cluster-operator.tsx(4 hunks)frontend/public/components/cluster-settings/related-objects.tsx(0 hunks)frontend/public/components/configmap.tsx(1 hunks)frontend/public/components/control-plane-machine-set.tsx(2 hunks)frontend/public/components/cron-job.tsx(1 hunks)frontend/public/components/custom-resource-definition.tsx(1 hunks)frontend/public/components/daemon-set.tsx(2 hunks)frontend/public/components/default-resource.tsx(1 hunks)frontend/public/components/deployment-config.tsx(2 hunks)frontend/public/components/deployment.tsx(2 hunks)frontend/public/components/group.tsx(1 hunks)frontend/public/components/hpa.tsx(1 hunks)frontend/public/components/image-stream.tsx(1 hunks)frontend/public/components/job.tsx(1 hunks)frontend/public/components/limit-range.tsx(1 hunks)frontend/public/components/machine-autoscaler.tsx(1 hunks)frontend/public/components/machine-config-pool.tsx(0 hunks)frontend/public/components/machine-config.tsx(1 hunks)frontend/public/components/machine-health-check.tsx(1 hunks)frontend/public/components/machine-set.tsx(0 hunks)frontend/public/components/machine.tsx(0 hunks)frontend/public/components/monitoring/alertmanager/alertmanager-config.tsx(0 hunks)frontend/public/components/namespace.jsx(4 hunks)frontend/public/components/pod-list.tsx(5 hunks)frontend/public/components/prometheus.tsx(2 hunks)frontend/public/components/replicaset.jsx(0 hunks)frontend/public/components/replication-controller.jsx(0 hunks)frontend/public/components/resource-quota.jsx(0 hunks)frontend/public/components/secret.tsx(1 hunks)frontend/public/components/service-account.jsx(0 hunks)frontend/public/components/service-monitor.jsx(0 hunks)frontend/public/components/stateful-set.tsx(1 hunks)frontend/public/components/template-instance.tsx(4 hunks)frontend/public/components/user.tsx(1 hunks)
💤 Files with no reviewable changes (10)
- frontend/public/components/cluster-settings/related-objects.tsx
- frontend/public/components/machine-config-pool.tsx
- frontend/public/components/replication-controller.jsx
- frontend/public/components/machine-set.tsx
- frontend/public/components/service-account.jsx
- frontend/public/components/monitoring/alertmanager/alertmanager-config.tsx
- frontend/public/components/replicaset.jsx
- frontend/public/components/resource-quota.jsx
- frontend/public/components/service-monitor.jsx
- frontend/public/components/machine.tsx
🚧 Files skipped from review as they are similar to previous changes (32)
- frontend/public/components/default-resource.tsx
- frontend/packages/helm-plugin/src/components/details-page/resources/tests/HelmReleaseResourcesRow.spec.tsx
- frontend/packages/helm-plugin/src/components/details-page/resources/HelmReleaseResourcesRow.tsx
- frontend/packages/helm-plugin/src/components/details-page/resources/HelmReleaseResourcesList.tsx
- frontend/public/components/namespace.jsx
- frontend/public/components/hpa.tsx
- frontend/public/components/RBAC/bindings.tsx
- frontend/public/components/cluster-settings/cluster-operator.tsx
- frontend/public/components/deployment-config.tsx
- frontend/packages/helm-plugin/src/components/list-page/ProjectHelmChartRepositoryRow.tsx
- frontend/public/components/alert-manager.tsx
- frontend/public/components/build.tsx
- frontend/public/components/RBAC/role.jsx
- frontend/public/components/daemon-set.tsx
- frontend/packages/console-app/src/components/nodes/NodesPage.tsx
- frontend/packages/helm-plugin/src/components/list-page/RepositoriesRow.tsx
- frontend/packages/console-app/src/components/data-view/types.ts
- frontend/packages/helm-plugin/src/components/list-page/HelmReleaseList.tsx
- frontend/packages/helm-plugin/src/components/list-page/HelmChartRepositoryList.tsx
- frontend/public/components/secret.tsx
- frontend/public/components/cron-job.tsx
- frontend/public/components/machine-health-check.tsx
- frontend/packages/helm-plugin/src/components/list-page/RepositoriesList.tsx
- frontend/public/components/pod-list.tsx
- frontend/public/components/limit-range.tsx
- frontend/public/components/machine-autoscaler.tsx
- frontend/public/components/deployment.tsx
- frontend/public/components/custom-resource-definition.tsx
- frontend/packages/helm-plugin/src/components/list-page/HelmChartRepositoryRow.tsx
- frontend/public/components/image-stream.tsx
- frontend/public/components/user.tsx
- frontend/public/components/machine-config.tsx
🧰 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/public/components/stateful-set.tsxfrontend/packages/console-app/src/components/data-view/ConsoleDataView.tsxfrontend/public/components/job.tsxfrontend/packages/console-app/src/components/pdb/PDBList.tsxfrontend/public/components/build-config.tsxfrontend/public/components/configmap.tsxfrontend/public/components/prometheus.tsxfrontend/public/components/group.tsxfrontend/public/components/control-plane-machine-set.tsxfrontend/packages/helm-plugin/src/components/list-page/ProjectHelmChartRepositoryList.tsxfrontend/public/components/template-instance.tsx
🔇 Additional comments (17)
frontend/public/components/control-plane-machine-set.tsx (2)
279-279: LGTM! Type signature simplified.The removal of the second generic parameter aligns with the PR's goal to simplify
GetDataViewRowstyping by providing a default fallback forTCustomRowData.
337-337: LGTM! Generic parameter simplified.The removal of the second generic parameter is consistent with the updated type definitions and improves code clarity.
frontend/public/components/group.tsx (3)
31-31: LGTM! Clean import statement.The import cleanup appropriately removes unused types while retaining the necessary
GetDataViewRowstype.
36-75: LGTM! Simplified type signature.The function signature correctly adopts the simplified generic usage (
GetDataViewRows<GroupKind>) and removes the unusedcustomDataparameter, aligning with the PR objectives. The function logic remains sound.
123-135: LGTM! Simplified component usage.The removal of the
initialFiltersprop is appropriate as it is now optional per the PR objectives. The component will correctly use the default filter configuration.frontend/public/components/job.tsx (2)
78-78: Clean type simplification.The removal of the explicit
undefinedsecond type parameter leverages the new default fallback inGetDataViewRows, making the code more concise without sacrificing type safety.
308-324: LGTM! Consistent refactoring.The removal of the
initialFiltersprop aligns with the PR objective of making it optional. The component maintains all required props and functionality.frontend/public/components/build-config.tsx (2)
136-136: LGTM: Type signature simplification.The removal of the explicit
undefinedsecond type parameter aligns with the PR objective thatGetDataViewRowsnow has a default fallback forTCustomRowData.
322-334: LGTM: Consistent with optionalinitialFiltersprop.The removal of the
initialFiltersprop aligns with the PR objective thatConsoleDataViewcomponent'sinitialFiltersis now optional. The component will use default filtering behavior.frontend/public/components/configmap.tsx (2)
39-39: LGTM! Type signature simplified correctly.The removal of the explicit
undefinedsecond type parameter is a clean simplification now thatGetDataViewRowshas a default fallback forTCustomRowData.
134-142: LGTM! Removal ofinitialFiltersprop aligns with PR objectives.The
ConsoleDataViewcomponent now uses its default filtering behavior sinceinitialFiltersis optional. This simplification is consistent with the PR's goal of making the prop optional while maintaining existing functionality.frontend/public/components/template-instance.tsx (1)
167-167: LGTM! Memoized initial filters implementation is correct.The memoized
initialFiltersproperly extendsinitialFiltersDefaultwith the customstatusfilter field and is correctly passed toConsoleDataView. The empty dependency array is appropriate sinceinitialFiltersDefaultis a constant, preventing unnecessary recreations on re-renders.Also applies to: 196-196
frontend/public/components/prometheus.tsx (1)
20-86: Type update forGetDataViewRowsandgetDataViewRowssignature looks correctUsing
GetDataViewRows<K8sResourceKind>without the second generic cleanly adopts the new default type parameter, and the(data, columns)signature matches the expected ConsoleDataView API while preserving existing row-mapping behavior. No issues from a correctness, readability, or maintainability standpoint in this block.frontend/packages/console-app/src/components/data-view/ConsoleDataView.tsx (1)
29-29: LGTM: Clear default export.The exported
initialFiltersDefaultconstant provides a well-defined default filter configuration that consumers can reference if needed.frontend/packages/helm-plugin/src/components/list-page/ProjectHelmChartRepositoryList.tsx (1)
3-3: LGTM: Simplified usage aligns with optional initialFilters.The removal of
initialFiltersDefaultimport and prop simplifies the component code while maintaining the same filtering behavior through the new default inConsoleDataView.Also applies to: 16-25
frontend/public/components/stateful-set.tsx (1)
23-23: LGTM: Consistent with the optional initialFilters pattern.The removal of explicit
initialFiltersDefaultusage simplifies the code while preserving the default filtering behavior.Also applies to: 80-90
frontend/packages/console-app/src/components/pdb/PDBList.tsx (1)
33-33: LGTM: Type signature simplification and consistent prop usage.The simplified
GetDataViewRows<PodDisruptionBudgetKind>type signature leverages the new defaultTCustomRowDataparameter, and the removal ofinitialFiltersprop aligns with the optional prop pattern across the codebase.Also applies to: 151-159
| const initialFiltersOrDefault = React.useMemo( | ||
| () => initialFilters ?? (initialFiltersDefault as TFilters), | ||
| [initialFilters], | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
Type assertion may be unsafe for extended filter types.
The cast initialFiltersDefault as TFilters on line 110 assumes that TFilters only contains the name and label fields. If a consumer uses ConsoleDataView with a filter type that extends ResourceFilters with additional required fields, this assertion will compile but produce an incomplete object at runtime, potentially causing bugs.
Consider a safer approach that preserves type safety:
const initialFiltersOrDefault = React.useMemo(
- () => initialFilters ?? (initialFiltersDefault as TFilters),
+ () => initialFilters ?? ({ ...initialFiltersDefault } as TFilters),
[initialFilters],
);Better yet, require consumers with custom filter types to always provide initialFilters, or document that TFilters must not have additional required fields beyond ResourceFilters:
// Option 1: Make initialFilters required when TFilters has extra fields
export type ConsoleDataViewProps<
TData,
TCustomRowData = any,
TFilters extends ResourceFilters = ResourceFilters
> = {
// ... other props
initialFilters: TFilters extends ResourceFilters
? TFilters | undefined
: TFilters;
// ... rest
};
// Option 2: Add JSDoc warning
/**
* @param initialFilters - Optional. When omitted, defaults to { name: '', label: '' }.
* If TFilters extends ResourceFilters with additional fields, initialFilters must be provided.
*/Run the following to check if any consumers are using extended filter types:
🏁 Script executed:
#!/bin/bash
# Search for ConsoleDataView usage with custom filter types that might extend ResourceFilters
rg -nP --type=tsx --type=ts 'ConsoleDataView<[^>]+,\s*[^>]+,\s*\{[^}]*\}>' -A5 -B5Length of output: 27
🏁 Script executed:
#!/bin/bash
# Fix the search - ripgrep doesn't need type flags for ts/tsx, just use -g patterns
# First, let's look at the ConsoleDataView.tsx file to understand the types
cat frontend/packages/console-app/src/components/data-view/ConsoleDataView.tsx | head -150Length of output: 4468
🏁 Script executed:
#!/bin/bash
# Find ResourceFilters definition
rg -n "type ResourceFilters|interface ResourceFilters" -A5Length of output: 498
🏁 Script executed:
#!/bin/bash
# Search for ConsoleDataView usage in tsx/ts files (corrected syntax)
rg -n "ConsoleDataView<" -g "*.tsx" -g "*.ts" -A3 -B3Length of output: 19853
🏁 Script executed:
#!/bin/bash
# Find all custom filter type definitions mentioned in ConsoleDataView usages
rg -n "type PodFilters|type HelmReleaseFilters|type TemplateInstanceFilters|type NodeFilters|type ReceiverFilters|type BindingFilters|type RelatedObjectsFilters|type ClusterOperatorFilters" -A8Length of output: 5619
🏁 Script executed:
#!/bin/bash
# Also search for interface definitions of these filters
rg -n "interface PodFilters|interface HelmReleaseFilters|interface TemplateInstanceFilters|interface NodeFilters|interface ReceiverFilters|interface BindingFilters|interface RelatedObjectsFilters|interface ClusterOperatorFilters" -A8Length of output: 908
🏁 Script executed:
#!/bin/bash
# Check if major consumers (PodList, HelmReleaseList, NodesPage) pass initialFilters
rg -n "ConsoleDataView<PodKind|HelmRelease|NodeRowItem" -A20 | grep -E "initialFilters|ConsoleDataView<"Length of output: 1011
🏁 Script executed:
#!/bin/bash
# Check the useConsoleDataViewFilters function to see if it accesses extended filter fields
cat frontend/packages/console-app/src/components/data-view/useConsoleDataViewFilters.ts | head -100Length of output: 2378
The type assertion issue is confirmed as a legitimate type safety concern.
The verification reveals that while current consumers (HelmReleaseList, PodList, NodesPage) all explicitly provide initialFilters, thereby avoiding runtime exposure to incomplete objects, the type assertion as TFilters on line 110 is genuinely unsafe. It bypasses TypeScript's type checking and creates a false sense of security.
Custom filter types like PodFilters, HelmReleaseFilters, NodeFilters, and others extend ResourceFilters with additional required fields (status, roles, architecture, etc.). If a future consumer relies on the default initialization without providing initialFilters, or if code later accesses extended filter fields from the default, an incomplete object would be passed through, potentially causing bugs.
Fix the type assertion to preserve type safety:
const initialFiltersOrDefault = React.useMemo(
- () => initialFilters ?? (initialFiltersDefault as TFilters),
+ () => initialFilters ?? ({ ...initialFiltersDefault } as TFilters),
[initialFilters],
);Alternatively, make initialFilters required when TFilters has additional fields, or add clear documentation that custom filter types must always provide initialFilters:
/**
* @param initialFilters - Required when TFilters extends ResourceFilters with additional fields.
* If omitted, only { name: '', label: '' } is provided as default.
*/🤖 Prompt for AI Agents
In frontend/packages/console-app/src/components/data-view/ConsoleDataView.tsx
around lines 109 to 112, remove the unsafe type assertion "as TFilters" and
preserve type safety by making the default value a Partial of the generic
instead of pretending it's a full TFilters: change the prop types so
initialFiltersDefault is Partial<TFilters> (or ResourceFilters) and compute
initialFiltersOrDefault as React.useMemo(() => initialFilters ??
(initialFiltersDefault as Partial<TFilters>), [initialFilters,
initialFiltersDefault]) (i.e. stop asserting the default is a full TFilters),
then add a short JSDoc on the component props stating that callers must supply
initialFilters when their TFilters extend ResourceFilters with additional
required fields (or alternatively make initialFilters required for those flavors
via a conditional generic in props).
| columnLayout?: ColumnLayout; | ||
| columnManagementID?: string; | ||
| initialFilters: TFilters; | ||
| initialFilters?: TFilters; |
There was a problem hiding this comment.
does this work to remove the useMemo in line 109?
| initialFilters?: TFilters; | |
| initialFilters?: TFilters = initialFilters; |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: logonoff, rhamilto, vojtechszocs The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
1 similar comment
|
/retest |
|
Local build and testing shows there are no issues /verified by vojtechszocs |
openshift-ci-robot
added
the
verified
|
@vojtechszocs: This PR has been marked as verified by In 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. |
|
@vojtechszocs: all tests passed! Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
5 participants
ConsoleDataViewcomponent propinitialFiltersis now optionalGetDataViewRowstype now hasTCustomRowDatatype param fallback