OCPBUGS-62700: Fix VolumeSnapshot table sorting#16200
OCPBUGS-62700: Fix VolumeSnapshot table sorting#16200stefanonardo wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@stefanonardo: This pull request references Jira Issue OCPBUGS-62700, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
added
component/core
📝 WalkthroughWalkthroughThis pull request refactors volume snapshot sorting logic across the console application. Type definitions for ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/public/components/factory/table.tsx (1)
66-66: Avoid coupling shared table utilities to@console/appLine 66 makes
frontend/public/components/factory/table.tsxdepend on@console/app/src/status, which inverts layering for shared infrastructure. Prefer moving the status value getter to a shared module (e.g.,@console/shared/src/sorts/snapshot) or defining it locally in this layer.As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
Also applies to: 95-96
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/public/components/factory/table.tsx` at line 66, The import of volumeSnapshotStatus from `@console/app/src/status` in table.tsx couples shared table utilities to a higher-level package; remove that import and either (a) move the status value getter (volumeSnapshotStatus) into a shared module (e.g., create and export it from a new or existing shared sorts/util module) and import it from that shared location, or (b) define the volumeSnapshotStatus getter locally in frontend/public/components/factory/table.tsx where it’s used (referencing the symbol volumeSnapshotStatus and the usages around lines 95–96) so the table layer no longer depends on `@console/app`; update the import sites to the new shared path or local definition and ensure tests/types still pass.
🤖 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/public/components/factory/table.tsx`:
- Line 66: The import of volumeSnapshotStatus from `@console/app/src/status` in
table.tsx couples shared table utilities to a higher-level package; remove that
import and either (a) move the status value getter (volumeSnapshotStatus) into a
shared module (e.g., create and export it from a new or existing shared
sorts/util module) and import it from that shared location, or (b) define the
volumeSnapshotStatus getter locally in
frontend/public/components/factory/table.tsx where it’s used (referencing the
symbol volumeSnapshotStatus and the usages around lines 95–96) so the table
layer no longer depends on `@console/app`; update the import sites to the new
shared path or local definition and ensure tests/types still pass.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: b8834ae4-122f-404b-ba16-65cd841762c8
📒 Files selected for processing (6)
frontend/packages/console-app/src/components/data-view/useConsoleDataViewData.tsxfrontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-content.tsxfrontend/packages/console-app/src/components/volume-snapshot/volume-snapshot.tsxfrontend/packages/console-shared/src/sorts/snapshot.tsfrontend/public/components/factory/table.tsxfrontend/public/module/k8s/types.ts
💤 Files with no reviewable changes (1)
- frontend/packages/console-app/src/components/data-view/useConsoleDataViewData.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/packages/console-app/src/components/volume-snapshot/volume-snapshot-content.tsxfrontend/packages/console-app/src/components/volume-snapshot/volume-snapshot.tsxfrontend/packages/console-shared/src/sorts/snapshot.tsfrontend/public/components/factory/table.tsxfrontend/public/module/k8s/types.ts
🔇 Additional comments (5)
frontend/public/module/k8s/types.ts (1)
1087-1105: Type split forrestoreSizelooks correctGood change:
VolumeSnapshotKindnow models quantity strings, whileVolumeSnapshotContentKindmodels byte counts numerically, which matches the intended sort behavior.frontend/packages/console-shared/src/sorts/snapshot.ts (1)
13-15:snapshotContentSizeis a solid additionDefaulting to
0keeps sorting deterministic whenstatus.restoreSizeis missing.frontend/public/components/factory/table.tsx (1)
98-99:volumeSnapshotContentSizesort getter is correctly wiredThis gives the content table a numeric size value path consistent with the new type updates.
frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot.tsx (1)
170-172: Value-based sort functions fix the broken columnsThe new
sortResourceByValue(direction, sorts.*)usage correctly fixes Status, Size, and Source sorting for VolumeSnapshots.Also applies to: 177-179, 184-186
frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-content.tsx (1)
120-122: Status and Size sorting updates look correctUsing
sortResourceByValuewithsorts.volumeSnapshotStatusandsorts.volumeSnapshotContentSizeis the right fix for VolumeSnapshotContent sorting.Also applies to: 127-129
| import { displayDurationInWords } from '../utils/build-utils'; | ||
| import { useTableData } from './table-data-hook'; | ||
| import TableHeader from './Table/TableHeader'; | ||
| import { volumeSnapshotStatus } from '@console/app/src/status'; |
There was a problem hiding this comment.
nit: This import (@console/internal → @console/app) creates improper layering. While it matches existing patterns in table-filters.ts, we should consider moving volumeSnapshotStatus() to "@console/shared" to resolve the dependency cycle, and then, import it from there.
The Status, Size, and Source columns in the VolumeSnapshot table and Status and Size columns in VolumeSnapshotContent table were not sorting correctly because they referenced non-existent sort field names instead of using the sorts registry.
stefanonardo
force-pushed
the
OCPBUGS-62700
branch
from
29ba6d5 to
775a86a
Compare
|
/lgtm |
openshift-ci-robot
added
verified-later
verified
|
@cajieh: This PR has been marked to be verified later by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cajieh, stefanonardo 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
the
approved
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@cajieh: This pull request references Jira Issue OCPBUGS-62700, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
3 participants
Summary
restoreSizevaluesvolumeSnapshotStatusto shared location and renamed tosnapshotStatusProblem
The Status, Size, and Source columns in the VolumeSnapshot table and Status and Size columns in VolumeSnapshotContent table were not sorting correctly. The columns referenced non-existent sort field names (like
'volumeSnapshotSize') instead of using function wrappers with the sorts registry.Additionally, TypeScript types incorrectly defined
restoreSizeas a number for both VolumeSnapshot and VolumeSnapshotContent, when in reality VolumeSnapshot uses Kubernetes quantity strings (e.g.,"1Gi") and VolumeSnapshotContent uses integer bytes.Changes
Sorting fix:
(data, direction) => data.sort(sortResourceByValue(direction, sorts.functionName))volumeSnapshotStatus,volumeSnapshotSize,volumeSnapshotContentSize, andvolumeSnapshotSourceto the sorts registry intable.tsxVolumeSnapshot.status.restoreSizeas string andVolumeSnapshotContent.status.restoreSizeas numbersnapshotContentSize()utility function to handle VolumeSnapshotContent size sortinguseConsoleDataViewData.tsx(redundant string sort check that was never reached)Refactoring:
volumeSnapshotStatusfrom@console/app/src/statusto@console/shared/src/sorts/snapshotsnapshotStatusfor consistency with other snapshot utilities (snapshotSize,snapshotSource,snapshotContentSize)Test plan
🤖 Generated with Claude Code