CONSOLE-4723: Migrate storage related tables to ConsoleDataView #15747
Conversation
vojtechszocs
changed the title
|
@vojtechszocs: This pull request references CONSOLE-4723 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. |
WalkthroughMigrates multiple list/table components to ConsoleDataView (volume-snapshot, persistent-volume, persistent-volume-claim, storage-class, PVC/PV); converts several component signatures to React.FCC; removes legacy per-row/table props (notably Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas needing 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 (12)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
🧰 Additional context used📓 Path-based instructions (1)**⚙️ CodeRabbit configuration file
Files:
🧬 Code graph analysis (4)frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-content.tsx (4)
frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-class.tsx (7)
frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot.tsx (4)
frontend/public/components/storage-class.tsx (5)
🔇 Additional comments (13)
Comment |
openshift-ci-robot
added
the
jira/valid-reference
|
/hold |
openshift-ci
bot
added
component/helm
|
@vojtechszocs: This pull request references CONSOLE-4723 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. |
|
@vojtechszocs: This pull request references CONSOLE-4723 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. |
![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 (3)
frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-content.tsx (1)
51-53: Consider normalizing restoreSize before humanizing for consistencyHere you call
humanizeBinaryBytes(size).stringdirectly onobj.status?.restoreSize. Involume-snapshot.tsx, size is first normalized withconvertToBaseValuebefore humanization. Aligning both places (e.g., via a shared helper or the same pre-normalization step) would avoid subtle unit-handling differences and keep the UX consistent between snapshots and snapshot contents.frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot.tsx (2)
63-80: MakegetDataViewRowsresilient to missing rowDataRight now you destructure
rowData: { hideSnapshotContentColumn }directly from each element. This assumesrowDatais always present, which is true given the currentcustomRowDatausage, but it makes the helper fragile to future reuse. A small tweak to handle an optionalrowData(e.g.,const hideSnapshotContentColumn = rowData?.hideSnapshotContentColumn;) would make this safer without changing behavior today.
227-275: Confirm status filter values stay aligned withvolumeSnapshotStatusThe status filter options (
Ready,Pending,Error) are hard-coded, andmatchesAdditionalFilterschecksvolumeSnapshotStatus(resource)against these strings. This is fine as long asvolumeSnapshotStatuscontinues to return the same labels, but it’s a silent coupling. Worth double‑checking they match today and considering a shared source of truth if more statuses are added later.
📜 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 (4)
frontend/packages/console-app/src/components/nodes/NodesPage.tsx(3 hunks)frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-class.tsx(4 hunks)frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-content.tsx(3 hunks)frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot.tsx(4 hunks)
🧰 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/nodes/NodesPage.tsxfrontend/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.tsx
🔇 Additional comments (4)
frontend/packages/console-app/src/components/nodes/NodesPage.tsx (1)
548-551: LGTM! Good use of memoization for stable filter reference.The memoized
initialFiltersprevents unnecessary object recreation on every render, which could cause filter resets in ConsoleDataView. The empty dependency array is appropriate since this filter configuration is truly static.frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-content.tsx (1)
45-103: ConsoleDataView row mapping looks consistent and robustThe new
getDataViewRowsimplementation builds row cells fromVolumeSnapshotContentKindcleanly, usesDASHfallbacks, and wires actions/name/status/links in a data-driven way that should play nicely withConsoleDataViewand column management. No correctness issues stand out here.frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-class.tsx (1)
41-139: Data-view migration for VolumeSnapshotClass looks solidThe combination of
getDataViewRows,useVolumeSnapshotClassColumns, and theConsoleDataViewwrapper preserves existing behavior (default-class badge, driver/deletionPolicy display, actions) while standardizing on the new data-view patterns. Row construction, column IDs, andDASHfallbacks are consistent; I don’t see functional or maintainability concerns here.frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot.tsx (1)
63-146: Snapshot row construction and conditional snapshotContent column look correct
getDataViewRowscorrectly derives name/namespace/status/size/source/content/class/timestamp and wires actions/context. ThehideSnapshotContentColumnflag is applied consistently in both the column definitions and the per-rowdisabledfield, so the snapshot content column should cleanly disappear whenCAN_LIST_VSCis false without leaving empty cells.
vojtechszocs
force-pushed
the
data-view-storage-tables
branch
from
8f69bf3 to
96cafe4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
frontend/public/components/persistent-volume-claim.tsx (2)
481-545: ForwardcanCreateandshowTitleprops toListPagecomponentThe review comment is accurate.
PersistentVolumeClaimsPageProps(lines 590-594) definescanCreate?andshowTitle?as optional properties, but the component destructures onlynamespaceand spreads remaining props without explicitly handling these two. The component then hardcodescanCreate={true}and never forwardsshowTitle, breaking the type contract and preventing callers from controlling these behaviors.The suggested fix is correct: extract both props with appropriate defaults and forward them to
ListPageinstead of hardcodingcanCreate={true}and omittingshowTitle.
489-508: Dispatch in render phase confirmed—infinite render loop risk is real and criticalVerification confirms the review comment is accurate. The code has:
dispatch(setPVCMetrics(pvcMetrics))directly in component body (line 507), not wrapped in any effect- No
useEffectanywhere in the component that might mitigate thispvcMetricsis rebuilt each render via the ternary + reduce (no memoization)dispatchis a fresh function fromuseDispatch()each renderThis creates a render loop: pvcMetrics object → dispatch → store update → re-render → new pvcMetrics object → repeat.
The suggested fix is correct—use
useMemoforpvcMetricsanduseEffectfor the dispatch call with proper dependency arrays.frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot.tsx (1)
335-369: EnsurecheckPVCSnapshotalways returns an array to the table
checkPVCSnapshotcurrently uses optional chaining:const checkPVCSnapshot = ( volumeSnapshots: VolumeSnapshotKind[], pvc: K8sResourceKind, ): VolumeSnapshotKind[] => volumeSnapshots?.filter(/* ... */);If
volumeSnapshotsis evernull/undefined, this expression returnsundefined, butVolumeSnapshotTableexpectsdata: VolumeSnapshotKind[]. To avoid passingundefinedinto the table, return an empty array as a fallback:const checkPVCSnapshot = ( volumeSnapshots: VolumeSnapshotKind[], pvc: K8sResourceKind, ): VolumeSnapshotKind[] => - volumeSnapshots?.filter( + volumeSnapshots?.filter( (snapshot) => snapshot?.spec?.source?.persistentVolumeClaimName === getName(pvc) && getNamespace(snapshot) === getNamespace(pvc), - ); + ) ?? [];This keeps the runtime type consistent with the
VolumeSnapshotTablecontract.
🧹 Nitpick comments (4)
frontend/public/components/persistent-volume-claim.tsx (2)
103-174: Avoid using hooks directly insidegetDataViewRowsutility
getDataViewRowscallsuseTranslationanduseSelectorwhile explicitly disablingreact-hooks/rules-of-hooks. This is brittle: ifConsoleDataViewever calls this callback conditionally or in a non-render context, hook ordering can break, and static analysis can’t protect you.Consider moving the hook usage into a dedicated hook that returns a memoized row-mapping function, e.g.
usePVCDataViewRows(): GetDataViewRows<PersistentVolumeClaimKind>, and then pass that fromPersistentVolumeClaimListtoConsoleDataView. That keeps hooks at the top level of a React hook/component and lets you re‑enable the eslint rule.
584-588: AlignPersistentVolumeClaimListProps['data']type with whatGetDataViewRowsexpects
PersistentVolumeClaimListPropscurrently typesdataasPersistentVolumeClaimKind[], butgetDataViewRowstreats each element as an object with anobjproperty. To avoid type drift and ensure callers pass the correct row shape, it’s safer to derive the element type fromGetDataViewRows:-type PersistentVolumeClaimListProps = { - data: PersistentVolumeClaimKind[]; +type PersistentVolumeClaimListProps = { + data: Parameters<GetDataViewRows<PersistentVolumeClaimKind>>[0]; loaded: boolean; loadError: unknown; };This keeps the list’s public props in sync with the DataView row contract even if the underlying row type changes.
frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-content.tsx (1)
45-53: Consider normalizingrestoreSizebefore humanizingIn
getDataViewRows,sizeMetricscallshumanizeBinaryBytes(size)directly, whereas the VolumeSnapshot table converts quantities viaconvertToBaseValuefirst. Ifstatus.restoreSizeis a quantity string, mirroring that pattern here (humanizeBinaryBytes(convertToBaseValue(size))) would keep units handling consistent and more robust.frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-class.tsx (1)
34-39: Use computed property name for the default-class annotation keyIn
isDefaultSnapshotClass, the default annotations object uses a literal keydefaultSnapshotClassAnnotation:getAnnotations(volumeSnapshotClass, { defaultSnapshotClassAnnotation: 'false' })[defaultSnapshotClassAnnotation]Using a computed key keeps the constant and the actual annotation key in sync and makes the fallback clearer:
- getAnnotations(volumeSnapshotClass, { defaultSnapshotClassAnnotation: 'false' })[ - defaultSnapshotClassAnnotation - ] === 'true'; + getAnnotations(volumeSnapshotClass, { [defaultSnapshotClassAnnotation]: 'false' })[ + defaultSnapshotClassAnnotation + ] === 'true';This avoids any future drift if the annotation name changes.
📜 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 (5)
frontend/packages/console-app/src/components/nodes/NodesPage.tsx(3 hunks)frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-class.tsx(4 hunks)frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-content.tsx(3 hunks)frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot.tsx(4 hunks)frontend/public/components/persistent-volume-claim.tsx(12 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/packages/console-app/src/components/nodes/NodesPage.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/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/packages/console-app/src/components/volume-snapshot/volume-snapshot-class.tsx
🔇 Additional comments (7)
frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-content.tsx (3)
54-103: Row mapping to ConsoleDataView looks consistent
rowCellskeys are aligned withtableColumnInfoand thecolumnsIDs, and the fallback toDASHplus optionalpropshandling makes the mapping resilient to missing fields. No functional issues spotted in the row construction.
105-176: Columns hook and ConsoleDataView integration are wired correctly
useVolumeSnapshotContentColumnsdefines stable, memoized columns with appropriate sort keys and sticky/nowrap props, andVolumeSnapshotContentTablepassesdata,loaded,loadError,columns, andgetDataViewRowsintoConsoleDataViewinside aSuspensewithLoadingBoxfallback. This aligns with the new data-view patterns and looks good.
180-206: Page-level watch and table composition look good
VolumeSnapshotContentPagewatches the correct GVK, treats the resource as non‑namespaced, passesloadErrorthrough to the table, and usesListPageCreatewith the sharedkindreference. The title/showTitle/canCreate wiring is straightforward and appears correct.frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-class.tsx (2)
41-139: ConsoleDataView migration for VolumeSnapshotClass looks solid
getDataViewRowscorrectly builds cells for name/driver/deletionPolicy/actions, including the “Default” marker and action menu context, anduseVolumeSnapshotClassColumnsdefines matching columns with sticky/nowrap props.VolumeSnapshotClassTablethen passes these intoConsoleDataViewwithloaded,loadError, and aLoadingBoxfallback. Overall the new data-view implementation is coherent and free of obvious issues.
143-174: Page wiring and creation flow remain intact
VolumeSnapshotClassPagestill watches the correct model, threadsnamespaceandselector, and now uses the sharedkindreference forListPageCreatewhile passingloadErrorto the table. The behavior should match the old page while benefiting from the new table implementation.frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot.tsx (2)
63-146: Row mapping and conditional SnapshotContent cell disabling look correct
getDataViewRowsbuilds cells for all columns, usesVolumeSnapshotRowData.hideSnapshotContentColumnto mark the SnapshotContent cell as disabled, and correctly falls back toDASHfor missing values. The filter overcolumnsand the use ofreferenceForModel/referenceForfor links and action context all look consistent with the intended behavior.
148-295: DataView columns, status filters, and flag-based column visibility are well-structured
useVolumeSnapshotColumnscleanly encapsulates column definitions and hides the SnapshotContent column whenhideSnapshotContentColumnis true, whileVolumeSnapshotTablewires incustomRowData, a status checkbox filter, andmatchesAdditionalFiltersto feedConsoleDataView. The use ofinitialFiltersDefaultand memoized filter options keeps configuration centralized and avoids unnecessary re-renders. No blocking issues here.
|
@vojtechszocs: This pull request references CONSOLE-4723 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. |
|
@vojtechszocs: This pull request references CONSOLE-4723 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. |
vojtechszocs
force-pushed
the
data-view-storage-tables
branch
from
96cafe4 to
ab0aeb5
Compare
|
@vojtechszocs: This pull request references CONSOLE-4723 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. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
frontend/public/components/persistent-volume-claim.tsx (2)
236-247: FixuseFlagusage so the PV link can actually be shown
useFlag(FLAGS.CAN_LIST_PV)is treated as if it returned a flags map:const flags = useFlag(FLAGS.CAN_LIST_PV); const canListPV = flags[FLAGS.CAN_LIST_PV];If
useFlagreturns a single boolean (as elsewhere in Console),canListPVwill always beundefined, and thevolumeName && canListPV && ...block will never render.Refactor to use the boolean directly:
-const PVCDetails: React.FCC<PVCDetailsProps> = ({ obj: pvc }) => { - const flags = useFlag(FLAGS.CAN_LIST_PV); - const { t } = useTranslation(); - ... - const canListPV = flags[FLAGS.CAN_LIST_PV]; +const PVCDetails: React.FCC<PVCDetailsProps> = ({ obj: pvc }) => { + const canListPV = useFlag(FLAGS.CAN_LIST_PV); + const { t } = useTranslation(); + ... {volumeName && canListPV && ( <DescriptionListGroup> <DescriptionListTerm>{t('public~PersistentVolumes')}</DescriptionListTerm> <DescriptionListDescription data-test-id="persistent-volume"> <ResourceLink kind="PersistentVolume" name={volumeName} /> </DescriptionListDescription> </DescriptionListGroup> )}Also applies to: 385-391
482-509: Avoid dispatchingsetPVCMetricsdirectly in render; memoize + useEffect
PersistentVolumeClaimsPagecurrently computes metrics and dispatches on every render:const [response, loadError, loading] = usePrometheusPoll(...); const pvcMetrics = _.isEmpty(loadError) && !loading ? response?.data?.result?.reduce(/* ... */) : {}; dispatch(setPVCMetrics(pvcMetrics));Dispatching during render is brittle and can lead to unnecessary renders or even render loops if reducers touch state consumed here.
Consider:
- const pvcMetrics = - _.isEmpty(loadError) && !loading - ? response?.data?.result?.reduce((acc, item) => { - _.set( - acc, - ['usedCapacity', item?.metric?.namespace, item?.metric?.persistentvolumeclaim], - Number(item?.value?.[1]), - ); - return acc; - }, {}) - : {}; - - dispatch(setPVCMetrics(pvcMetrics)); + const pvcMetrics = React.useMemo( + () => + _.isEmpty(loadError) && !loading + ? response?.data?.result?.reduce((acc, item) => { + _.set( + acc, + ['usedCapacity', item?.metric?.namespace, item?.metric?.persistentvolumeclaim], + Number(item?.value?.[1]), + ); + return acc; + }, {}) + : {}, + [response, loadError, loading], + ); + + React.useEffect(() => { + dispatch(setPVCMetrics(pvcMetrics)); + }, [dispatch, pvcMetrics]);This keeps metrics computation tied to Prometheus updates and ensures Redux is updated outside the render phase.
frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-content.tsx (1)
180-213: Type mismatch:VolumeSnapshotContentPagePropsis missingshowTitle
VolumeSnapshotContentPagedestructuresshowTitlewith a default, butVolumeSnapshotContentPagePropsonly declarescanCreate?: boolean(plusPageComponentProps). This will not type-check becauseshowTitleis not part of the props type.Add
showTitle?: booleantoVolumeSnapshotContentPageProps(or drop the prop from the component) to keep the type definition aligned with usage.type VolumeSnapshotContentPageProps = { - canCreate?: boolean; -} & PageComponentProps; + canCreate?: boolean; + showTitle?: boolean; +} & PageComponentProps;
🧹 Nitpick comments (4)
frontend/public/components/persistent-volume-claim.tsx (1)
510-533: ConfirminitPathbehavior whennamespaceis undefined
initPathcurrently falls back to thedefaultnamespace:const initPath = `/k8s/ns/${namespace || 'default'}/persistentvolumeclaims/`;If
PersistentVolumeClaimsPageis ever used without an explicitnamespace, this will route create links into thedefaultnamespace rather than an all-namespaces/cluster-scoped path (if that was the previous behavior).If the intended behavior is “all namespaces” when
namespaceis unset, consider something like:const initPath = namespace ? `/k8s/ns/${namespace}/persistentvolumeclaims/` : '/k8s/all-namespaces/persistentvolumeclaims/';Please double-check this against existing PVC pages to avoid changing navigation semantics unexpectedly.
frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-content.tsx (1)
35-43: Row/column mapping is sound; consider aligning size formatting with VolumeSnapshotThe data-view rows correctly map all
tableColumnInfoids to cells, with sensible fallbacks (DASH) and appropriate use ofResourceLink,Status,Timestamp, andLazyActionMenu. For consistency withVolumeSnapshotsizing, you may want to confirm thatstatus.restoreSizeis already in base bytes; if not, consider going throughconvertToBaseValuebeforehumanizeBinaryBytesto avoid any subtle formatting differences.If
restoreSizeis not guaranteed to be base bytes, please double‑check current usage ofhumanizeBinaryByteswith similar fields elsewhere in the codebase to decide whether aconvertToBaseValuestep is needed here.Also applies to: 45-103, 105-156
frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot.tsx (2)
63-146: Guard against future column/rowCells drift ingetDataViewRowsThe current implementation assumes every
columnsentry has a correspondingrowCells[id], and usesrowCells[id].disabledin thefilterpredicate. That’s fine today because both are derived from the sametableColumnInfo, but it’s brittle to future changes (e.g., adding a column but forgetting to extendrowCellswould cause a runtime error).You can make this more robust with optional chaining and a default:
- return columns - .filter(({ id }) => !rowCells[id].disabled) - .map(({ id }) => { - const cell = rowCells[id]?.cell || DASH; - const props = rowCells[id]?.props || undefined; + return columns + .filter(({ id }) => !rowCells[id]?.disabled) + .map(({ id }) => { + const cell = rowCells[id]?.cell || DASH; + const props = rowCells[id]?.props || undefined; return { id, props, cell, }; });
335-369: PVC page behavior preserved; consider makingnsoptional in the type
VolumeSnapshotPVCPagepreserves the existing behavior of preferring thensprop and falling back toparams.ns, and reusesVolumeSnapshotTableso the new data‑view behavior is applied consistently.Given that the component already handles a missing
nsby reading from route params, you may want to reflect that in the type by makingnsoptional for accuracy:-type VolumeSnapshotPVCPage = { - obj: PersistentVolumeClaimKind; - ns: string; -}; +type VolumeSnapshotPVCPage = { + obj: PersistentVolumeClaimKind; + ns?: string; +};This isn’t required for runtime correctness but better documents the actual call pattern.
Also applies to: 382-385
📜 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 (6)
frontend/packages/console-app/src/components/nodes/NodesPage.tsx(3 hunks)frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-class.tsx(4 hunks)frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-content.tsx(3 hunks)frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot.tsx(4 hunks)frontend/public/components/persistent-volume-claim.tsx(12 hunks)frontend/public/components/persistent-volume.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/packages/console-app/src/components/nodes/NodesPage.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/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.tsxfrontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-class.tsxfrontend/public/components/persistent-volume.tsx
🔇 Additional comments (4)
frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-class.tsx (2)
30-84: Data-view row mapping and column definitions look consistent and safe
tableColumnInfo,getDataViewRows, anduseVolumeSnapshotClassColumnsuse the same column ids and provide sane fallbacks (DASH) plus sticky name/actions columns. The LazyActionMenu context and default-class badge logic are wired correctly; no issues from a correctness or maintainability standpoint.Also applies to: 86-119
121-140: ConsoleDataView integration and page wiring are correctWrapping
ConsoleDataViewinSuspensewithLoadingBoxis appropriate, andloadErroris passed through via...props. The page’suseK8sWatchResourcecall feedsdata,loaded, andloadErrorintoVolumeSnapshotClassTable, andListPageCreatecorrectly uses the sharedkindconstant.Also applies to: 143-177, 186-190
frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot.tsx (2)
148-215: Column configuration and snapshot-content visibility logic look goodUsing
rowData.hideSnapshotContentColumnto both (a) mark the column definition as disabled and (b) filter it out via.filter((c) => !c.disabled)cleanly ties column visibility to theFLAGS.CAN_LIST_VSCflag. Sticky name/actions columns and the sort keys align with the row data, so the column setup is coherent and maintainable.
217-295: Status filter integration with ConsoleDataView is correctly wiredThe status checkbox filter is correctly defined (
filterId="status"and matchingVolumeSnapshotFilters['status']),initialFiltersextendsinitialFiltersDefaultwithstatus: [], andmatchesAdditionalFilterscleanly short‑circuits when a status filter is active. This is a straightforward and maintainable use of theResourceFilters+additionalFilterNodespattern.Also applies to: 371-373
vojtechszocs
force-pushed
the
data-view-storage-tables
branch
from
ab0aeb5 to
f312ffb
Compare
|
@vojtechszocs: This pull request references CONSOLE-4723 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. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-class.tsx (1)
151-161: VolumeSnapshotClass is cluster‑scoped but watched as namespaced
VolumeSnapshotClassis a cluster‑scoped resource; configuringuseK8sWatchResourcewithnamespaced: trueand anamespacecan result in watching the wrong URL (or an empty list), especially compared toVolumeSnapshotContentPage, which omitsnamespacedfor another cluster‑scoped type.Consider updating the watcher to treat it as cluster‑scoped, e.g.:
const [resources, loaded, loadError] = useK8sWatchResource<VolumeSnapshotClassKind[]>({ groupVersionKind: { group: VolumeSnapshotClassModel.apiGroup, kind: VolumeSnapshotClassModel.kind, version: VolumeSnapshotClassModel.apiVersion, }, isList: true, - namespaced: true, - namespace, + namespaced: false, selector, });and ignoring the
namespaceprop for this page if it is not actually used anywhere else.
♻️ Duplicate comments (2)
frontend/public/components/persistent-volume.tsx (1)
55-113: Rules of Hooks violation remains unresolved.The
getDataViewRowsfunction still callsuseTranslation()(line 57) despite not being a React component or custom hook. This was flagged in a previous review with suggested fixes. Please address this critical issue before merging.frontend/public/components/persistent-volume-claim.tsx (1)
104-175: Rules of Hooks violation remains unresolved.The
getDataViewRowsfunction calls bothuseTranslation()(line 106) anduseSelector()(line 107) despite not being a React component or custom hook. This was flagged in a previous review with a detailed refactoring suggestion. Please address this critical issue before merging.
🧹 Nitpick comments (3)
frontend/public/components/storage-class.tsx (1)
252-260: Improve type safety in type definitions.Two type-related improvements:
- Line 259:
obj: anyshould use a more specific type likeStorageClassResourceKindfor better type safety.- Line 255:
loadErroris declared inStorageClassListPropsbut not used in the component at line 160. Either use it (e.g., for error handling in ConsoleDataView) or remove it from the type definition.Apply this diff to improve type safety:
export type StorageClassDetailsProps = { - obj: any; + obj: StorageClassResourceKind; };frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot.tsx (2)
63-66: Make row/column mapping resilient to future column changes
getDataViewRowsfilters withrowCells[id].disabled:return columns .filter(({ id }) => !rowCells[id].disabled) .map(({ id }) => { const cell = rowCells[id]?.cell || DASH; const props = rowCells[id]?.props || undefined; ... });This assumes every
columnsentry has a correspondingrowCells[id]. If a new column is ever added androwCellsisn’t updated, this will throw at runtime before the optional chaining is reached.A small defensive tweak avoids that brittleness:
- .filter(({ id }) => !rowCells[id].disabled) + .filter(({ id }) => !rowCells[id]?.disabled)You may also not need the
disabledflag onrowCellsat all, sinceuseVolumeSnapshotColumnsalready filters disabled columns at the header level.Also applies to: 82-96, 105-132, 134-146
148-215: Status filter wiring looks good; consider centralizing status valuesThe status checkbox options and
matchesAdditionalFilterslogic are consistent and should work as intended, assumingvolumeSnapshotStatus()returns"Ready" | "Pending" | "Error"for the cases you care about.To reduce the chance of divergence if statuses evolve, consider sharing constants (or a small enum/union) between
volumeSnapshotStatusandvolumeSnapshotStatusFilterOptionsrather than hardcoding the strings in two places.Also applies to: 221-226, 227-276, 279-293
📜 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 (7)
frontend/packages/console-app/src/components/nodes/NodesPage.tsx(3 hunks)frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-class.tsx(4 hunks)frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-content.tsx(3 hunks)frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot.tsx(4 hunks)frontend/public/components/persistent-volume-claim.tsx(12 hunks)frontend/public/components/persistent-volume.tsx(3 hunks)frontend/public/components/storage-class.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/packages/console-app/src/components/nodes/NodesPage.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/packages/console-app/src/components/volume-snapshot/volume-snapshot.tsxfrontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-class.tsxfrontend/public/components/storage-class.tsxfrontend/public/components/persistent-volume-claim.tsxfrontend/public/components/persistent-volume.tsxfrontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-content.tsx
🔇 Additional comments (4)
frontend/public/components/storage-class.tsx (1)
211-230: Verify intention to omit filter toolbar.Line 225 sets
omitFilterToolbar={true}, which removes filtering capabilities from the storage class list. If this is part of the migration strategy, please confirm this is intentional, as it may affect user experience.frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-class.tsx (1)
41-84: Data-view migration for VolumeSnapshotClass table looks consistentRow construction, column IDs, and use of
DASHfallback are coherent with the shared data‑view patterns. Sticky name/actions columns and memoized column definitions should keep the table behavior and UX consistent with other data‑view based lists.Also applies to: 86-119, 121-139
frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-content.tsx (1)
54-103: ConsoleDataView integration for VolumeSnapshotContent looks solidThe row mapping, column definitions, Suspense/LoadingBox wrapper, and use of
DASHfallback and actions cell props are consistent with the new data‑view patterns and should preserve behavior while simplifying the table/rendering code.Also applies to: 105-156, 158-177, 180-205
frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot.tsx (1)
335-338: PVC‑scoped reuse of VolumeSnapshotTable looks correct
checkPVCSnapshotcorrectly filters snapshots by PVC name and namespace, andVolumeSnapshotPVCPagereusesVolumeSnapshotTablewith the filtered data while preservingloaded/loadErrorwiring. This keeps the PVC view aligned with the main snapshot list behavior.Also applies to: 345-346, 362-366
| const getDataViewRows: GetDataViewRows<VolumeSnapshotContentKind> = (data, columns) => { | ||
| return data.map(({ obj }) => { | ||
| const name = obj.metadata?.name || ''; | ||
| const creationTimestamp = obj.metadata?.creationTimestamp || ''; | ||
| const snapshotName = obj.spec?.volumeSnapshotRef?.name || ''; | ||
| const snapshotNamespace = obj.spec?.volumeSnapshotRef?.namespace || ''; | ||
| const size = obj.status?.restoreSize; | ||
| const sizeMetrics = size ? humanizeBinaryBytes(size).string : DASH; | ||
|
|
There was a problem hiding this comment.
Fix VolumeSnapshotContent size humanization to use base bytes
obj.status?.restoreSize is a quantity (e.g., 10Gi), but it’s passed directly into humanizeBinaryBytes. In volume-snapshot.tsx you correctly call convertToBaseValue first; without that, this can produce incorrect or NaN values.
Align this code with the snapshot list:
-import { humanizeBinaryBytes } from '@console/internal/components/utils/units';
+import { convertToBaseValue, humanizeBinaryBytes } from '@console/internal/components/utils/units';
...
- const size = obj.status?.restoreSize;
- const sizeMetrics = size ? humanizeBinaryBytes(size).string : DASH;
+ const size = obj.status?.restoreSize;
+ const sizeBase = convertToBaseValue(size);
+ const sizeMetrics = size ? humanizeBinaryBytes(sizeBase).string : DASH;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const getDataViewRows: GetDataViewRows<VolumeSnapshotContentKind> = (data, columns) => { | |
| return data.map(({ obj }) => { | |
| const name = obj.metadata?.name || ''; | |
| const creationTimestamp = obj.metadata?.creationTimestamp || ''; | |
| const snapshotName = obj.spec?.volumeSnapshotRef?.name || ''; | |
| const snapshotNamespace = obj.spec?.volumeSnapshotRef?.namespace || ''; | |
| const size = obj.status?.restoreSize; | |
| const sizeMetrics = size ? humanizeBinaryBytes(size).string : DASH; | |
| const getDataViewRows: GetDataViewRows<VolumeSnapshotContentKind> = (data, columns) => { | |
| return data.map(({ obj }) => { | |
| const name = obj.metadata?.name || ''; | |
| const creationTimestamp = obj.metadata?.creationTimestamp || ''; | |
| const snapshotName = obj.spec?.volumeSnapshotRef?.name || ''; | |
| const snapshotNamespace = obj.spec?.volumeSnapshotRef?.namespace || ''; | |
| const size = obj.status?.restoreSize; | |
| const sizeBase = convertToBaseValue(size); | |
| const sizeMetrics = size ? humanizeBinaryBytes(sizeBase).string : DASH; |
🤖 Prompt for AI Agents
In
frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-content.tsx
around lines 45 to 53, the code passes obj.status?.restoreSize (a k8s Quantity
like "10Gi") directly to humanizeBinaryBytes which yields incorrect/NaN results;
change this to first convert the Quantity to base bytes using
convertToBaseValue(obj.status?.restoreSize) and then pass that numeric base
value to humanizeBinaryBytes, and add the necessary import for
convertToBaseValue if not already present.
openshift-merge-robot
added
the
needs-rebase
|
@vojtechszocs: This pull request references CONSOLE-4723 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. |
vojtechszocs
force-pushed
the
data-view-storage-tables
branch
from
f312ffb to
d9d1b09
Compare
openshift-merge-robot
removed
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
frontend/public/components/storage-class.tsx (1)
68-123: Hooks violation ingetDataViewRowsremains unaddressed.The
useTranslation()hook is called insidegetDataViewRows, which violates React's Rules of Hooks. This was flagged in a previous review. The eslint-disable comment suppresses the linter but doesn't fix the underlying issue—hooks must be called at the component level.Additionally, the type mismatch persists:
StorageClassListProps.datais typed asStorageClassResourceKind[](line 253), butgetDataViewRowsdestructures{ obj }from each item, expectingRowProps<StorageClassResourceKind>[].frontend/public/components/persistent-volume-claim.tsx (1)
105-109: Hooks violation ingetDataViewRowsremains unaddressed.Calling
useTranslation()anduseSelector()insidegetDataViewRowsviolates React's Rules of Hooks. This was flagged in a previous review. Lift these hooks toPersistentVolumeClaimListand pass the values to a memoized callback.frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-content.tsx (1)
51-52: Fix:humanizeBinaryBytesreceives a k8s Quantity string instead of base bytes.
obj.status?.restoreSizeis a Kubernetes quantity string (e.g.,"10Gi"), buthumanizeBinaryBytesexpects a numeric base value. This will produce incorrect orNaNresults. Involume-snapshot.tsx, you correctly useconvertToBaseValuefirst.+import { convertToBaseValue, humanizeBinaryBytes } from '@console/internal/components/utils/units'; -import { humanizeBinaryBytes } from '@console/internal/components/utils/units'; ... const size = obj.status?.restoreSize; - const sizeMetrics = size ? humanizeBinaryBytes(size).string : DASH; + const sizeBase = convertToBaseValue(size); + const sizeMetrics = size ? humanizeBinaryBytes(sizeBase).string : DASH;
📜 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 (7)
frontend/packages/console-app/src/components/nodes/NodesPage.tsx(1 hunks)frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-class.tsx(4 hunks)frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-content.tsx(3 hunks)frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot.tsx(4 hunks)frontend/public/components/persistent-volume-claim.tsx(12 hunks)frontend/public/components/persistent-volume.tsx(3 hunks)frontend/public/components/storage-class.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/packages/console-app/src/components/nodes/NodesPage.tsx
- frontend/public/components/persistent-volume.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/storage-class.tsxfrontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-content.tsxfrontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-class.tsxfrontend/public/components/persistent-volume-claim.tsxfrontend/packages/console-app/src/components/volume-snapshot/volume-snapshot.tsx
🧬 Code graph analysis (1)
frontend/public/components/persistent-volume-claim.tsx (9)
frontend/public/models/index.ts (1)
PersistentVolumeClaimModel(664-677)frontend/public/redux.ts (1)
RootState(19-27)frontend/public/components/utils/link.tsx (1)
getNamespace(38-57)frontend/public/components/events.jsx (3)
obj(126-126)props(125-125)loading(387-387)frontend/public/module/k8s/k8s.ts (1)
referenceFor(50-65)frontend/packages/console-app/src/components/data-view/ConsoleDataView.tsx (5)
getNameCellProps(250-255)actionsCellProps(257-261)cellIsStickyProps(240-243)initialFiltersDefault(29-29)ConsoleDataView(83-238)frontend/__mocks__/i18next.ts (1)
t(8-14)frontend/public/components/factory/table.tsx (1)
TableColumn(668-675)frontend/public/components/graphs/helpers.ts (1)
PrometheusEndpoint(9-9)
🔇 Additional comments (8)
frontend/public/components/storage-class.tsx (1)
160-176: LGTM with caveat ongetDataViewRows.The
StorageClassListcomponent correctly usesuseStorageClassColumnshook at the component level. However, once the hooks violation ingetDataViewRowsis fixed, you'll need to passtfrom this component to the row mapper function.frontend/public/components/persistent-volume-claim.tsx (1)
420-495: LGTM - List component structure is correct.The
PersistentVolumeClaimListcomponent properly uses hooks at the component level and sets up filtering correctly. TheSuspenseboundary withLoadingBoxfallback is appropriate.frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-content.tsx (2)
45-103: Good: No hooks violations in thisgetDataViewRows.Unlike the other files, this
getDataViewRowsimplementation does not call any React hooks, which is the correct pattern. This can serve as a reference for fixing the hooks violations instorage-class.tsxandpersistent-volume-claim.tsx.
158-178: LGTM - Component structure follows the correct pattern.The
VolumeSnapshotContentTablecomponent properly uses hooks at the component level, wraps content inSuspensewithLoadingBoxfallback, and passes props correctly toConsoleDataView.frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-class.tsx (2)
41-84: Good: No hooks violations ingetDataViewRows.This implementation correctly avoids calling hooks inside
getDataViewRows, following best practices. The row mapping logic is clean and consistent with the data-view pattern.
121-141: LGTM - VolumeSnapshotClassTable follows the correct pattern.Proper use of hooks at component level, Suspense boundary with fallback, and correct ConsoleDataView prop passing.
frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot.tsx (2)
63-146: Good: No hooks violations and proper size conversion.This
getDataViewRowsimplementation correctly:
- Avoids calling React hooks inside the function
- Uses
convertToBaseValue(size)before passing tohumanizeBinaryBytesThis serves as a good reference for other files in this PR.
217-295: LGTM - VolumeSnapshotTable is well-structured.The component properly:
- Uses hooks at the component level
- Implements status filtering with memoized filter options
- Uses
customRowDatapattern for conditional column visibility- Wraps content in Suspense with LoadingBox fallback
frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-class.tsx
Show resolved
Hide resolved
| return columns | ||
| .filter(({ id }) => !rowCells[id].disabled) | ||
| .map(({ id }) => { | ||
| const cell = rowCells[id]?.cell || DASH; | ||
| const props = rowCells[id]?.props || undefined; | ||
| return { | ||
| id, | ||
| props, | ||
| cell, | ||
| }; | ||
| }); |
There was a problem hiding this comment.
Potential undefined access in filter logic.
Line 135 accesses rowCells[id].disabled directly without optional chaining, but line 137 uses rowCells[id]?.cell. If rowCells[id] is undefined for a given column id, line 135 will throw.
return columns
- .filter(({ id }) => !rowCells[id].disabled)
+ .filter(({ id }) => !rowCells[id]?.disabled)
.map(({ id }) => {
const cell = rowCells[id]?.cell || DASH;
const props = rowCells[id]?.props || undefined;🤖 Prompt for AI Agents
In
frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot.tsx
around lines 134 to 144, the filter currently accesses rowCells[id].disabled
directly which can throw if rowCells[id] is undefined; change the filter to
safely check existence (e.g. use optional chaining: filter(({ id }) =>
!rowCells[id]?.disabled) or explicitly guard: filter(({ id }) => !!rowCells[id]
&& !rowCells[id].disabled)) and keep the subsequent mapping logic that uses
rowCells[id]?.cell / props so undefined cells are handled consistently.
| const matchesAdditionalFilters = React.useCallback( | ||
| (resource: PersistentVolumeClaimKind, filters: PersistentVolumeClaimFilters) => { | ||
| // Status filter | ||
| if (filters.status.length > 0) { | ||
| const status = resource.status.phase; | ||
| if (!filters.status.includes(status)) { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| return true; | ||
| }, | ||
| [], | ||
| ); |
There was a problem hiding this comment.
Guard against undefined status to prevent runtime error.
If resource.status is undefined, accessing resource.status.phase at line 468 will throw a TypeError.
const matchesAdditionalFilters = React.useCallback(
(resource: PersistentVolumeClaimKind, filters: PersistentVolumeClaimFilters) => {
// Status filter
if (filters.status.length > 0) {
- const status = resource.status.phase;
+ const status = resource.status?.phase;
if (!filters.status.includes(status)) {
return false;
}
}
return true;
},
[],
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const matchesAdditionalFilters = React.useCallback( | |
| (resource: PersistentVolumeClaimKind, filters: PersistentVolumeClaimFilters) => { | |
| // Status filter | |
| if (filters.status.length > 0) { | |
| const status = resource.status.phase; | |
| if (!filters.status.includes(status)) { | |
| return false; | |
| } | |
| } | |
| return true; | |
| }, | |
| [], | |
| ); | |
| const matchesAdditionalFilters = React.useCallback( | |
| (resource: PersistentVolumeClaimKind, filters: PersistentVolumeClaimFilters) => { | |
| // Status filter | |
| if (filters.status.length > 0) { | |
| const status = resource.status?.phase; | |
| if (!filters.status.includes(status)) { | |
| return false; | |
| } | |
| } | |
| return true; | |
| }, | |
| [], | |
| ); |
🤖 Prompt for AI Agents
In frontend/public/components/persistent-volume-claim.tsx around lines 464 to
477, the code directly reads resource.status.phase which can throw if
resource.status is undefined; update the status check to safely handle missing
status (e.g., const status = resource.status?.phase; or if (!resource.status ||
resource.status.phase === undefined) return false when filters.status is
non-empty) so that undefined status does not cause a TypeError and the filter
behaves correctly.
| : {}; | ||
|
|
||
| dispatch(setPVCMetrics(pvcMetrics)); |
There was a problem hiding this comment.
Unconditional dispatch on every render may cause performance issues.
dispatch(setPVCMetrics(pvcMetrics)) is called on every render cycle. This can trigger unnecessary re-renders and Redux state updates. Consider wrapping in useEffect with appropriate dependencies.
+ React.useEffect(() => {
+ dispatch(setPVCMetrics(pvcMetrics));
+ }, [dispatch, pvcMetrics]);
- dispatch(setPVCMetrics(pvcMetrics));Note: You may need to stabilize pvcMetrics with useMemo to prevent the effect from running excessively due to object reference changes.
📝 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.
| : {}; | |
| dispatch(setPVCMetrics(pvcMetrics)); | |
| : {}; | |
| React.useEffect(() => { | |
| dispatch(setPVCMetrics(pvcMetrics)); | |
| }, [dispatch, pvcMetrics]); |
🤖 Prompt for AI Agents
In frontend/public/components/persistent-volume-claim.tsx around lines 521 to
523, the code unconditionally dispatches setPVCMetrics(pvcMetrics) on every
render which can cause unnecessary Redux updates; wrap the dispatch call in a
useEffect that runs only when pvcMetrics actually changes (add appropriate
dependencies), and if pvcMetrics is an object/array stabilize it with useMemo
(or derive a stable primitive key) before putting it in the effect dependency
array so the effect only runs when meaningful metric values change.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
frontend/public/components/persistent-volume-claim.tsx (2)
468-481: Guard against undefinedstatusto prevent runtime error.If
resource.statusis undefined, accessingresource.status.phaseat line 472 will throw a TypeError. This issue was previously flagged but remains unaddressed.Apply this diff to safely handle missing status:
const matchesAdditionalFilters = React.useCallback( (resource: PersistentVolumeClaimKind, filters: PersistentVolumeClaimFilters) => { // Status filter if (filters.status.length > 0) { - const status = resource.status.phase; + const status = resource.status?.phase; if (!filters.status.includes(status)) { return false; } } return true; }, [], );
527-527: Wrap dispatch inuseEffectto prevent unnecessary re-renders.
dispatch(setPVCMetrics(pvcMetrics))is called unconditionally on every render, which can trigger unnecessary Redux updates and re-renders throughout the application. This issue was previously flagged but remains unaddressed.Apply this diff to dispatch only when metrics change:
+ React.useEffect(() => { + dispatch(setPVCMetrics(pvcMetrics)); + }, [dispatch, pvcMetrics]); + - dispatch(setPVCMetrics(pvcMetrics));Note: Since
pvcMetricsis a newly computed object on each render, you should stabilize it withuseMemoto prevent the effect from running excessively:const pvcMetrics = - _.isEmpty(loadError) && !loading - ? response?.data?.result?.reduce((acc, item) => { - _.set( - acc, - ['usedCapacity', item?.metric?.namespace, item?.metric?.persistentvolumeclaim], - Number(item?.value?.[1]), - ); - return acc; - }, {}) - : {}; + React.useMemo( + () => + _.isEmpty(loadError) && !loading + ? response?.data?.result?.reduce((acc, item) => { + _.set( + acc, + ['usedCapacity', item?.metric?.namespace, item?.metric?.persistentvolumeclaim], + Number(item?.value?.[1]), + ); + return acc; + }, {}) + : {}, + [response, loadError, loading], + );frontend/public/components/storage-class.tsx (1)
114-116: Prefer??over||for cell/props fallbacksUsing
||will treat valid falsy values (e.g.,0, empty string) as missing and replace them withDASH/undefined.??keeps legitimate falsy values while still defaulting fornull/undefined.- const cell = rowCells[id]?.cell || DASH; - const props = rowCells[id]?.props || undefined; + const cell = rowCells[id]?.cell ?? DASH; + const props = rowCells[id]?.props ?? undefined;
🧹 Nitpick comments (2)
frontend/public/components/persistent-volume.tsx (1)
248-252: Consider derivingPersistentVolumeListPropsfrom shared list/DataView propsRight now
PersistentVolumeListPropsis a bespoke{ data; loaded; loadError }type whilePersistentVolumeListforwards...propsintoConsoleDataView. This works, but it can drift from whatever prop shapeConsoleDataView/ListPageactually expect (e.g., if more props are added later).To keep things aligned and future-proof, consider defining
PersistentVolumeListPropsas something like:
- A reuse of the shared list component props type from
ListPage, orReact.ComponentProps<typeof ConsoleDataView<PersistentVolumeKind>>(possiblyPick/Omit-ed as needed),so that any additional props are correctly typed and forwarded without manual duplication.
Also applies to: 295-299
frontend/public/components/storage-class.tsx (1)
255-263: Consider whether these prop types should be exported or inlined
StorageClassListPropsandStorageClassDetailsPropsare only used to type components in this file. If there are no external consumers, you could inline these shapes into theReact.FCC<...>declarations to reduce local type noise; if they are part of a public API, considerexport typeso downstream code can import them explicitly.
📜 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 (9)
frontend/packages/console-app/locales/en/console-app.json(1 hunks)frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-class.tsx(4 hunks)frontend/packages/container-security/src/components/image-manifest-vuln.tsx(0 hunks)frontend/public/components/custom-resource-definition.tsx(1 hunks)frontend/public/components/masthead/masthead-toolbar.tsx(2 hunks)frontend/public/components/persistent-volume-claim.tsx(12 hunks)frontend/public/components/persistent-volume.tsx(3 hunks)frontend/public/components/storage-class.tsx(4 hunks)frontend/public/locales/en/public.json(2 hunks)
💤 Files with no reviewable changes (1)
- frontend/packages/container-security/src/components/image-manifest-vuln.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/public/components/custom-resource-definition.tsx
- frontend/packages/console-app/locales/en/console-app.json
- frontend/public/locales/en/public.json
🧰 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/masthead/masthead-toolbar.tsxfrontend/public/components/storage-class.tsxfrontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-class.tsxfrontend/public/components/persistent-volume-claim.tsxfrontend/public/components/persistent-volume.tsx
🧬 Code graph analysis (2)
frontend/public/components/masthead/masthead-toolbar.tsx (1)
frontend/public/module/auth.js (1)
window(5-12)
frontend/public/components/storage-class.tsx (4)
frontend/public/module/k8s/types.ts (1)
StorageClassResourceKind(477-483)frontend/public/components/utils/resource-link.tsx (1)
ResourceLink(71-127)frontend/packages/console-app/src/components/data-view/ConsoleDataView.tsx (3)
getNameCellProps(250-255)actionsCellProps(257-261)ConsoleDataView(83-238)frontend/public/components/factory/details.tsx (2)
DetailsPageProps(166-193)DetailsPage(62-164)
🔇 Additional comments (5)
frontend/public/components/masthead/masthead-toolbar.tsx (1)
142-145: LGTM—good optimization.Moving the plugin detection to module scope eliminates repeated array lookups on every
getLaunchActionscall. Since plugins are loaded at startup and the TODO indicates this is temporary code, the static check is appropriate.frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-class.tsx (1)
1-196: LGTM! Clean migration to ConsoleDataView.The migration is well-structured and follows best practices:
- Proper memoization of columns (line 93) and row creator (line 132) for performance
- Translation correctly implemented for all user-facing strings, including the "Default" label (line 57) that was flagged in previous reviews
- Clean separation between column definitions, row creation logic, and rendering
- Type safety maintained with updated props (
data,loaded,loadErrorreplacingunfilteredData)The data-view pattern is consistently applied across the component.
frontend/public/components/persistent-volume.tsx (2)
42-113: ConsoleDataView integration and hook refactor look solid
getDataViewRowsCreatoris now a pure factory takingt: TFunction, anduseTranslationis only used insidePersistentVolumeList/usePersistentVolumeColumns, so the previous Rules-of-Hooks concern is resolved. Row construction lines up withtableColumnInfoIDs, and the memoizedgetDataViewRowsplusConsoleDataViewusage (includinghideColumnManagementandLoadingBoxfallback) look correct and maintainable.Also applies to: 115-166, 248-270
168-246: Details and page wiring are coherent and consistent
PVDetailscleanly presents PV metadata using existing shared components, and both the list and details pages correctly wire inPersistentVolumeList/PVDetailsviaListPageandDetailsPagewith the expectedkind/pagesconfiguration. No major functional or maintainability issues stand out here.Also applies to: 272-285, 287-293
frontend/public/components/storage-class.tsx (1)
161-179: Data-view + i18n wiring inStorageClassListlooks correctDeriving
getDataViewRowsviagetDataViewRowsCreator(t)at component scope (withuseMemo) cleanly avoids hook-rule violations and keeps the row mapper stable, only changing when the translation function changes. No further issues here.
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
vojtechszocs
force-pushed
the
data-view-storage-tables
branch
from
3f438a0 to
e67fcad
Compare
|
/lgtm |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
frontend/public/components/persistent-volume-claim.tsx (2)
468-481: Guard against undefinedstatusto prevent runtime error.If
resource.statusis undefined, accessingresource.status.phaseat line 472 will throw a TypeError.const matchesAdditionalFilters = React.useCallback( (resource: PersistentVolumeClaimKind, filters: PersistentVolumeClaimFilters) => { // Status filter if (filters.status.length > 0) { - const status = resource.status.phase; + const status = resource.status?.phase; if (!filters.status.includes(status)) { return false; } } return true; }, [], );
527-527: Unconditional dispatch on every render causes performance issues.
dispatch(setPVCMetrics(pvcMetrics))is called on every render cycle. This triggers unnecessary re-renders and Redux state updates.+ React.useEffect(() => { + dispatch(setPVCMetrics(pvcMetrics)); + }, [dispatch, pvcMetrics]); + - dispatch(setPVCMetrics(pvcMetrics));Note: You may need to stabilize
pvcMetricswithuseMemoto prevent the effect from running excessively due to object reference changes.
📜 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 (9)
frontend/packages/console-app/locales/en/console-app.json(2 hunks)frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-class.tsx(4 hunks)frontend/packages/container-security/src/components/image-manifest-vuln.tsx(0 hunks)frontend/public/components/custom-resource-definition.tsx(1 hunks)frontend/public/components/masthead/masthead-toolbar.tsx(2 hunks)frontend/public/components/persistent-volume-claim.tsx(12 hunks)frontend/public/components/persistent-volume.tsx(3 hunks)frontend/public/components/storage-class.tsx(4 hunks)frontend/public/locales/en/public.json(2 hunks)
💤 Files with no reviewable changes (1)
- frontend/packages/container-security/src/components/image-manifest-vuln.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/packages/console-app/locales/en/console-app.json
- frontend/public/components/masthead/masthead-toolbar.tsx
- frontend/public/components/custom-resource-definition.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/persistent-volume.tsxfrontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-class.tsxfrontend/public/components/persistent-volume-claim.tsxfrontend/public/locales/en/public.jsonfrontend/public/components/storage-class.tsx
🧬 Code graph analysis (1)
frontend/public/components/persistent-volume-claim.tsx (2)
frontend/packages/console-app/src/components/data-view/types.ts (2)
GetDataViewRows(26-29)ResourceFilters(5-8)frontend/packages/console-app/src/components/data-view/ConsoleDataView.tsx (5)
getNameCellProps(250-255)actionsCellProps(257-261)cellIsStickyProps(240-243)initialFiltersDefault(29-29)ConsoleDataView(83-238)
🔇 Additional comments (13)
frontend/public/locales/en/public.json (2)
1109-1110: Storage and PersistentVolume translation keys correctly added.The new translation keys for storage-related UI components (PersistentVolume, Capacity, StorageClass, Bound, Lost, Claim) are correctly placed in the Recommendations/Notifications section and follow the existing naming and formatting conventions. All keys use straightforward one-to-one English translations appropriate for the base locale file.
Also applies to: 1112-1112, 1124-1125, 1128-1128
1357-1357: Manual verification required due to infrastructure limitations.The automated verification cannot be completed due to persistent repository access issues. The concerns raised in the review comment require direct inspection of:
- References to "Default for VirtualMachines" in migrated storage-class components and volume-snapshot modules
- Migration context to confirm this key complements rather than incorrectly replaces existing "Default" entries
- Storage provisioner integration to ensure proper usage in ConsoleDataView migration
These verification steps must be performed locally by examining the component files directly.
frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-class.tsx (3)
42-88: Data-view row construction is side‑effect free and i18n‑safe.
getDataViewRowsCreatorcleanly mapsVolumeSnapshotClassKindobjects to row cells, usestpassed from the component (no hooks in the helper), provides a consistent actions context, and falls back toDASHfor missing values. No issues spotted in the row mapping logic.
90-123: Column definitions align with ConsoleDataView usage.
useVolumeSnapshotClassColumnscorrectly usesuseTranslation, stableids fromtableColumnInfo, appropriate sort fields, and sticky/nowrap cell props. This should integrate cleanly withConsoleDataView’s column handling.
125-145: Table/page wiring toConsoleDataViewlooks consistent.
VolumeSnapshotClassTableandVolumeSnapshotClassPagecorrectly:
- Obtain
tviauseTranslationin components (respecting hook rules).- Memoize
getDataViewRowsviagetDataViewRowsCreator(t).- Wrap
ConsoleDataViewinSuspensewithLoadingBoxfallback.- Pass
data,loaded,loadError, and staticlabel, pluskindintoListPageCreate.This matches the data‑view migration patterns used elsewhere; no structural issues seen.
Also applies to: 149-180
frontend/public/components/persistent-volume.tsx (5)
39-50: Top‑level PV wiring and status helper look good.Using
PersistentVolumeModel.kind/referenceForModelforkindand action context, plusPVStatusdelegating to<Status>, is consistent with existing patterns and keeps status logic centralized.Also applies to: 52-54
56-113:getDataViewRowsCreatorcorrectly replaces the old hook‑using helper.The new
getDataViewRowsCreator(t):
- Avoids calling
useTranslationinside a non‑hook helper.- Covers all defined columns (name, status, claim, capacity, labels, created, actions).
- Uses
DASHfallbacks and stableLabelList/Timestamp/LazyActionMenuusages.- Builds claim cell with a translated “No claim” message.
This resolves the earlier Rules of Hooks issue without introducing new behavior problems.
115-166: Column hook is idiomatic and matches data‑view expectations.
usePersistentVolumeColumnsusesuseTranslationinside a hook, memoizes the column array, and wires sort keys/ids and sticky/nowrap props appropriately. The action column uses an empty title with a dedicated id and sticky props, which matches other console tables.
168-246:PVDetailsreuses existing patterns and handles optional fields cleanly.The details view:
- Uses
PaneBody,SectionHeading,ResourceSummary, and PatternFlyDescriptionListcomponents consistently.- Safely handles optional fields (capacity, access modes, PVC, NFS export) and provides sensible fallbacks (e.g.,
Filesystem/Nonetranslations).- Reuses
PVStatusandResourceLinkfor related resources.No functional or structural issues identified.
248-299: List/page/details integration withConsoleDataViewlooks solid.
PersistentVolumeListobtainstviauseTranslation, memoizesgetDataViewRowsCreator(t), and passesdata,loaded,columns, andgetDataViewRowsintoConsoleDataViewwrapped inSuspensewithLoadingBox.PersistentVolumesPageusesListPagewithkind={kind},ListComponent={PersistentVolumeList},canCreate, andomitFilterToolbar, matching other list implementations.PersistentVolumesDetailsPageusesDetailsPagewith the reference kind andnavFactory.details(PVDetails).The new
PersistentVolumeListPropssurface is minimal and appropriate for this wiring.frontend/public/components/persistent-volume-claim.tsx (1)
105-174: LGTM! Hooks issue properly resolved.The factory pattern (
getDataViewRowsCreator) correctly addresses the previous review comment about calling hooks insidegetDataViewRows. By acceptingtandpvcMetricsas parameters and returning the row-building function, this allows the parent component to call hooks and pass the results in, maintaining React's rules of hooks.frontend/public/components/storage-class.tsx (2)
3-43: CentralizingkindfromStorageClassModelimproves maintainabilityDeriving
kindviaconst { kind } = StorageClassModel;and reusing it across the module removes magic strings and reduces risk of resource-kind drift between list, links, and details. Import set looks coherent with the data‑view migration and existing console patterns; no issues here.
49-73: Row mapping factory cleanly integrates i18n and actions into ConsoleDataViewUsing
tableColumnInfoplusgetDataViewRowsCreator(t)is a solid pattern: you avoid calling hooks inside the row‑mapper, keep the translator function injectable, and build a per‑rowrowCellsmap keyed by columnidbefore projecting overcolumns. The kubevirt plugin gate and TODO aroundisKubevirtPluginActiveare also a pragmatic way to keep existing annotations‑based hints without hard‑wiring plugin logic into the table forever. I don’t see functional issues in this mapping logic; theLazyActionMenucontext and defaulting toDASHfor missing cells are consistent with other resources.Please just re‑confirm that
GetDataViewRows<StorageClassResourceKind>in the shared data‑view types still expectsdataentries with anobjfield, so this{ obj }destructuring and thereferenceFor(obj)call remain aligned with the shared contract.Also applies to: 74-124
| const defaultSnapshotClassAnnotation = 'snapshot.storage.kubernetes.io/is-default-class'; | ||
|
|
||
| const defaultSnapshotClassAnnotation: string = 'snapshot.storage.kubernetes.io/is-default-class'; | ||
| export const isDefaultSnapshotClass = (volumeSnapshotClass: VolumeSnapshotClassKind) => | ||
| getAnnotations(volumeSnapshotClass, { defaultSnapshotClassAnnotation: 'false' })[ | ||
| defaultSnapshotClassAnnotation | ||
| ] === 'true'; |
There was a problem hiding this comment.
Fix annotation key usage in isDefaultSnapshotClass.
defaultSnapshotClassAnnotation is a string constant, but the fallback passed to getAnnotations uses a literal property name:
getAnnotations(volumeSnapshotClass, { defaultSnapshotClassAnnotation: 'false' })[
defaultSnapshotClassAnnotation
]This creates a key "defaultSnapshotClassAnnotation", not the annotation key value, so the helper will almost always return false and the “Default” label will never show.
Use a computed property so the key matches the constant’s value:
-export const isDefaultSnapshotClass = (volumeSnapshotClass: VolumeSnapshotClassKind) =>
- getAnnotations(volumeSnapshotClass, { defaultSnapshotClassAnnotation: 'false' })[
- defaultSnapshotClassAnnotation
- ] === 'true';
+export const isDefaultSnapshotClass = (volumeSnapshotClass: VolumeSnapshotClassKind) =>
+ getAnnotations(volumeSnapshotClass, { [defaultSnapshotClassAnnotation]: 'false' })[
+ defaultSnapshotClassAnnotation
+ ] === 'true';🤖 Prompt for AI Agents
In
frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-class.tsx
around lines 35 to 40, the fallback object passed to getAnnotations uses the
literal property name "defaultSnapshotClassAnnotation" instead of the value of
the constant defaultSnapshotClassAnnotation, causing the annotation lookup to
always miss; change the fallback to use a computed property keyed by the
constant (e.g. {[defaultSnapshotClassAnnotation]: 'false'}) so getAnnotations
returns the correct annotation value and the function can correctly detect and
return true when the annotation equals 'true'.
|
/retest |
|
We will need to test basic functionality of the impacted tables
/assign @yapei |
openshift-merge-robot
added
the
needs-rebase
|
@yanpzhan please help verify,thanks! |
vojtechszocs
force-pushed
the
data-view-storage-tables
branch
from
e67fcad to
4acf7c1
Compare
openshift-merge-robot
removed
the
needs-rebase
|
@yanpzhan I've rebased the PR, it builds and works on local dev env. without any issues. Can you please do some basic testing of the impacted resource list pages? (No dynamic plugins needed.) |
|
Regression test on cluster launched against the pr passed. |
openshift-ci-robot
added
the
verified
|
@yanpzhan: 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. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: logonoff, 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 |
|
@vojtechszocs: The following test failed, say
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. |
6 participants
Uh oh!
There was an error while loading. Please reload this page.