CONSOLE-4988: add resizable column feature to Storage tables#16162
CONSOLE-4988: add resizable column feature to Storage tables#16162vikram-raj wants to merge 8 commits intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@vikram-raj: This pull request references CONSOLE-4988 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vikram-raj The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
component/core
|
@vikram-raj: This pull request references CONSOLE-4988 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. |
📝 WalkthroughWalkthroughThis pull request introduces resizable column support across seven data table components in the OpenShift Console. Internal column definition hooks are refactored to return objects containing both a columns array and a ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip CodeRabbit can use Trivy to scan for security misconfigurations and secrets in Infrastructure as Code files.Add a .trivyignore file to your project to customize which findings Trivy reports. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot.tsx (1)
153-218: Consider extracting a shared helper for column config assembly.This block repeats the same shape across many columns (
id,resizableProps, nowrap props), which increases drift risk as more storage tables adopt the pattern. A small helper/factory would improve maintainability and reduce copy/paste regressions.As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot.tsx` around lines 153 - 218, The columns array in the VolumeSnapshot component duplicates common configuration (id from tableColumnInfo, resizableProps via getResizableProps, and standard props like modifier/nowrap) across many entries; extract a small factory/helper (e.g., buildColumn or createTableColumn) and use it to produce each TableColumn to reduce duplication. The helper should accept the unique fields (title, sort, index or tableColumnInfo index, optional props override, and optional disabled flag like rowData.hideSnapshotContentColumn) and internally set id = tableColumnInfo[index].id, resizableProps = getResizableProps(tableColumnInfo[index].id), and merge common props with any overrides; then replace the inline objects in the columns useMemo (referencing columns, tableColumnInfo, getResizableProps, nameCellProps, actionsCellProps, and rowData.hideSnapshotContentColumn) with calls to that helper.frontend/public/components/storage-class.tsx (1)
133-154: Reduce index-coupling in column ID lookups.Using
tableColumnInfo[0/1/2].idacross definitions is fragile; reordering columns can silently desync IDs and width settings. Prefer named column ID constants.Suggested refactor
+const STORAGE_CLASS_COLUMN_IDS = { + name: 'name', + provisioner: 'provisioner', + reclaimPolicy: 'reclaimPolicy', + actions: 'actions', +} as const; + const tableColumnInfo = [ - { id: 'name' }, - { id: 'provisioner' }, - { id: 'reclaimPolicy' }, - { id: '' }, + { id: STORAGE_CLASS_COLUMN_IDS.name }, + { id: STORAGE_CLASS_COLUMN_IDS.provisioner }, + { id: STORAGE_CLASS_COLUMN_IDS.reclaimPolicy }, + { id: STORAGE_CLASS_COLUMN_IDS.actions }, ];As per coding guidelines, "**: Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/public/components/storage-class.tsx` around lines 133 - 154, The column definitions currently index into tableColumnInfo by numeric positions (tableColumnInfo[0], [1], [2]) which couples IDs to order; change to use named constants for each column ID (e.g., NAME_COLUMN_ID, PROVISIONER_COLUMN_ID, RECLAIM_POLICY_COLUMN_ID) and reference those constants in the columns array and getResizableProps calls instead of numeric indexes; update any uses of nameCellProps and getResizableProps within the columns definition to call getResizableProps(NAME_COLUMN_ID) / getResizableProps(PROVISIONER_COLUMN_ID) / getResizableProps(RECLAIM_POLICY_COLUMN_ID) and set id: NAME_COLUMN_ID etc., and ensure the constants are exported/declared near the tableColumnInfo definition so reordering columns won’t desynchronize IDs or widths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot.tsx`:
- Around line 153-218: The columns array in the VolumeSnapshot component
duplicates common configuration (id from tableColumnInfo, resizableProps via
getResizableProps, and standard props like modifier/nowrap) across many entries;
extract a small factory/helper (e.g., buildColumn or createTableColumn) and use
it to produce each TableColumn to reduce duplication. The helper should accept
the unique fields (title, sort, index or tableColumnInfo index, optional props
override, and optional disabled flag like rowData.hideSnapshotContentColumn) and
internally set id = tableColumnInfo[index].id, resizableProps =
getResizableProps(tableColumnInfo[index].id), and merge common props with any
overrides; then replace the inline objects in the columns useMemo (referencing
columns, tableColumnInfo, getResizableProps, nameCellProps, actionsCellProps,
and rowData.hideSnapshotContentColumn) with calls to that helper.
In `@frontend/public/components/storage-class.tsx`:
- Around line 133-154: The column definitions currently index into
tableColumnInfo by numeric positions (tableColumnInfo[0], [1], [2]) which
couples IDs to order; change to use named constants for each column ID (e.g.,
NAME_COLUMN_ID, PROVISIONER_COLUMN_ID, RECLAIM_POLICY_COLUMN_ID) and reference
those constants in the columns array and getResizableProps calls instead of
numeric indexes; update any uses of nameCellProps and getResizableProps within
the columns definition to call getResizableProps(NAME_COLUMN_ID) /
getResizableProps(PROVISIONER_COLUMN_ID) /
getResizableProps(RECLAIM_POLICY_COLUMN_ID) and set id: NAME_COLUMN_ID etc., and
ensure the constants are exported/declared near the tableColumnInfo definition
so reordering columns won’t desynchronize IDs or widths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 03bda77a-d252-4860-840c-c31781286997
📒 Files selected for processing (7)
frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-class.tsxfrontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-content.tsxfrontend/packages/console-app/src/components/volume-snapshot/volume-snapshot.tsxfrontend/public/components/persistent-volume-claim.tsxfrontend/public/components/persistent-volume.tsxfrontend/public/components/storage-class.tsxfrontend/public/components/volume-attributes-class.tsx
📜 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/public/components/persistent-volume.tsxfrontend/public/components/storage-class.tsxfrontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-content.tsxfrontend/public/components/persistent-volume-claim.tsxfrontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-class.tsxfrontend/packages/console-app/src/components/volume-snapshot/volume-snapshot.tsxfrontend/public/components/volume-attributes-class.tsx
🧬 Code graph analysis (3)
frontend/public/components/persistent-volume-claim.tsx (2)
frontend/packages/console-app/src/components/data-view/useResizableColumnProps.ts (1)
useColumnWidthSettings(30-97)frontend/packages/console-app/src/components/data-view/ConsoleDataView.tsx (2)
nameCellProps(250-253)actionsCellProps(262-266)
frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-class.tsx (6)
frontend/public/components/factory/table.tsx (1)
TableColumn(663-670)frontend/public/module/k8s/types.ts (1)
VolumeSnapshotClassKind(1126-1129)frontend/packages/console-app/src/components/data-view/useResizableColumnProps.ts (1)
useColumnWidthSettings(30-97)frontend/public/models/index.ts (1)
VolumeSnapshotClassModel(1262-1277)frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-content.tsx (1)
tableColumnInfo(38-46)frontend/packages/console-app/src/components/data-view/ConsoleDataView.tsx (2)
nameCellProps(250-253)actionsCellProps(262-266)
frontend/public/components/volume-attributes-class.tsx (4)
frontend/public/components/factory/table.tsx (1)
TableColumn(663-670)frontend/public/module/k8s/types.ts (1)
VolumeAttributesClassKind(485-490)frontend/packages/console-app/src/components/data-view/useResizableColumnProps.ts (1)
useColumnWidthSettings(30-97)frontend/packages/console-app/src/components/data-view/ConsoleDataView.tsx (2)
nameCellProps(250-253)actionsCellProps(262-266)
🔇 Additional comments (17)
frontend/public/components/volume-attributes-class.tsx (4)
34-37: LGTM on the new imports.The imports for
nameCellPropsanduseColumnWidthSettingsalign correctly with the exports shown inConsoleDataView.tsxanduseResizableColumnProps.ts. Clean integration with the existing data-view infrastructure.
49-98: Well-structured hook refactoring.The hook correctly:
- Returns
{ columns, resetAllColumnWidths }object, enabling the consumer to wire up the reset functionality- Applies
resizablePropsonly to data columns (Name, Driver name, Parameters) while correctly excluding the action column- Includes
getResizablePropsin theuseMemodependency array, ensuring columns recompute when widths changeThe pattern is consistent with the other storage table components in this PR.
64-66: Correct separation of resize and sticky cell behaviors.Using
resizablePropsfor resize functionality while maintainingnameCellPropsandactionsCellPropsfor sticky positioning is the right approach. This preserves the expected PatternFly table UX where name and action columns remain visible during horizontal scroll.Also applies to: 91-91
149-149: Clean integration with ConsoleDataView.The destructuring and prop passing are correct. The
resetAllColumnWidthscallback is properly memoized upstream inuseColumnWidthSettings, so this won't cause unnecessary re-renders. TheisResizableboolean enables the feature consistently with other storage table components in this PR.Also applies to: 164-165
frontend/public/components/persistent-volume.tsx (2)
115-176: LGTM — Resizable columns wired correctly for PersistentVolume table.The hook refactor to return
{ columns, resetAllColumnWidths }aligns with the cross-file pattern.resizablePropsis correctly applied to data columns and intentionally omitted from the actions column. TheuseMemodependency ongetResizablePropsensures columns recompute when resize state changes.
258-278: LGTM — ConsoleDataView integration looks good.
isResizableandresetAllColumnWidthsare correctly passed through, enabling the column width management feature.frontend/public/components/persistent-volume-claim.tsx (2)
202-272: LGTM — PVC columns hook correctly refactored for resizable support.Consistent pattern with
PersistentVolumeand other storage tables: returns{ columns, resetAllColumnWidths }, appliesresizablePropsto data columns, excludes the actions column, and updatesuseMemodependencies appropriately.
526-606: LGTM — PersistentVolumeClaimList integration complete.The destructured hook return and
ConsoleDataViewprops (isResizable,resetAllColumnWidths) follow the established pattern. No issues with the existing filter logic or metrics handling.frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-content.tsx (2)
106-169: Strong column-resize hook integration.The new
{ columns, resetAllColumnWidths }return shape and per-columnresizablePropswiring are clean and consistent for this table.
176-190: ConsoleDataView resize wiring looks correct.Passing both
isResizableandresetAllColumnWidthshere completes the feature contract for interactive resize/reset behavior.frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot.tsx (2)
147-223: Good end-to-end adoption of resizable column state.The hook return change plus per-column
resizablePropsintegration is consistent and preserves existing conditional column behavior.
287-302: Table-level resize enablement is wired correctly.
isResizableandresetAllColumnWidthsare passed through correctly toConsoleDataView.frontend/public/components/storage-class.tsx (2)
17-21: Good API migration for DataView resizing primitives.Switching to
nameCellPropsanduseColumnWidthSettingsis consistent with the resizable-column contract and keeps the table config cohesive.
170-185: Resizable behavior is correctly plumbed intoConsoleDataView.
isResizableandresetAllColumnWidthsare wired cleanly from the hook output to the view component.frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-class.tsx (3)
86-93: Good hook contract upgrade for resizable state management.Returning
{ columns, resetAllColumnWidths }fromuseVolumeSnapshotClassColumnscleanly keeps table wiring and reset behavior together, and matches the new DataView flow.Also applies to: 127-127
101-115: Resizable props are wired consistently across data columns.Using
getResizablePropsper column while keepingnameCellProps/actionsCellPropsfor sticky behavior is a solid integration pattern here.Also applies to: 121-124
148-150: Reset column widths is properly accessible regardless ofhideColumnManagement.The reset button (line 203 in ConsoleDataView.tsx) is gated only by
isResizable && resetAllColumnWidths, not byhideColumnManagement. Users can reset column widths even when column management UI is hidden—this is correct UX design. No action needed.
|
@vikram-raj: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/retest |
2 participants
Add a resizable column feature to PersistentVolumes, PersistentVolumeClaims, StorageClasses, VolumeAttributesClasses, VolumeSnapshots, VolumeSnapshotClasses, and VolumeSnapshotContents list pages.
Summary by CodeRabbit