CONSOLE-5040: use ConsoleDataViewColumn instead of TableColumn#16246
CONSOLE-5040: use ConsoleDataViewColumn instead of TableColumn#16246vikram-raj wants to merge 4 commits intoopenshift:mainfrom
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
@vikram-raj: This pull request references CONSOLE-5040 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.22.0" version, but no target version was set. DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
|
[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
| title: t('console-app~Selector'), | ||
| id: tableColumnInfo[2].id, | ||
| sort: 'spec.selector', | ||
| sortFunction: 'spec.selector', |
There was a problem hiding this comment.
@coderabbitai notes: Sorting by spec.selector (an object containing matchLabels/matchExpressions) doesn't work: objects stringify to [object Object], causing all selector values to compare equal. The sort silently falls back to secondary ordering by pod name. Either remove this sortFunction to disable sorting on this column, or implement a custom comparator using LabelSelector for meaningful comparison (e.g., comparing stringified selector representations).
There was a problem hiding this comment.
@rhamilto, noted — this is a real bug. Sorting on spec.selector via a dot-path string resolves to the raw matchLabels/matchExpressions object, which stringifies to [object Object], so all rows compare equal and the sort silently degrades to secondary ordering. The fix is either dropping the sortFunction on the Selector column entirely, or wiring in a custom comparator that produces a stable string from the selector (e.g., sorted, serialized matchLabels key=value pairs).
Would you like me to open a follow-up issue to track this?
📝 Learnings were identified and not saved (knowledge base disabled). Enable
There was a problem hiding this comment.
Looks like the issue is in the main as well.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Fixed this issue. PTAL
vikram-raj
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
@vikram-raj: This pull request references CONSOLE-5040 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.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 change adds ✨ 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.
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/data-view/useConsoleDataViewData.tsx (1)
152-160:⚠️ Potential issue | 🟠 MajorAvoid mutating
filteredDataduring sort.Line 153 sorts
filteredDatain place, and Line 159 passes the same mutable array into custom sort callbacks (which also usedata.sort(...)in current column configs). This can leak side effects across renders and pagination/filter state.Proposed fix
- if (typeof sortColumn.sortFunction === 'string') { - return filteredData.sort( + if (typeof sortColumn.sortFunction === 'string') { + return [...filteredData].sort( sortResourceByValue(sortDirection, (obj) => _.get(obj, sortColumn.sortFunction as string)), ); } if (typeof sortColumn.sortFunction === 'function') { - return sortColumn.sortFunction(filteredData, sortDirection); + return sortColumn.sortFunction([...filteredData], sortDirection); }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/data-view/useConsoleDataViewData.tsx` around lines 152 - 160, The current logic mutates filteredData in place via filteredData.sort(...) and also passes the original array into custom sort callbacks (sortColumn.sortFunction), which can cause side effects across renders; fix this by making a shallow copy of filteredData before sorting or passing into the custom sorter (e.g., use [...filteredData] or filteredData.slice()) so sortResourceByValue and any sortColumn.sortFunction operate on a copy rather than mutating the original; update the branches that call sortResourceByValue(sortDirection, ...) and sortColumn.sortFunction(filteredData, sortDirection) to pass a copied array instead and ensure sortResourceByValue's implementation does not mutate its input.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@frontend/packages/console-app/src/components/data-view/useConsoleDataViewData.tsx`:
- Around line 152-160: The current logic mutates filteredData in place via
filteredData.sort(...) and also passes the original array into custom sort
callbacks (sortColumn.sortFunction), which can cause side effects across
renders; fix this by making a shallow copy of filteredData before sorting or
passing into the custom sorter (e.g., use [...filteredData] or
filteredData.slice()) so sortResourceByValue and any sortColumn.sortFunction
operate on a copy rather than mutating the original; update the branches that
call sortResourceByValue(sortDirection, ...) and
sortColumn.sortFunction(filteredData, sortDirection) to pass a copied array
instead and ensure sortResourceByValue's implementation does not mutate its
input.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 74558738-5aef-4742-9734-405a14b9c2da
📒 Files selected for processing (9)
frontend/packages/console-app/src/components/data-view/ConsoleDataView.tsxfrontend/packages/console-app/src/components/data-view/useConsoleDataViewData.tsxfrontend/packages/console-app/src/components/nodes/NodesPage.tsxfrontend/packages/console-app/src/components/pdb/PDBList.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-dynamic-plugin-sdk/src/api/internal-types.tsfrontend/packages/console-dynamic-plugin-sdk/src/extensions/console-types.tsfrontend/public/components/factory/Table/active-columns-hook.ts
📜 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/data-view/ConsoleDataView.tsxfrontend/packages/console-dynamic-plugin-sdk/src/api/internal-types.tsfrontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-class.tsxfrontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-content.tsxfrontend/packages/console-dynamic-plugin-sdk/src/extensions/console-types.tsfrontend/public/components/factory/Table/active-columns-hook.tsfrontend/packages/console-app/src/components/pdb/PDBList.tsxfrontend/packages/console-app/src/components/data-view/useConsoleDataViewData.tsxfrontend/packages/console-app/src/components/nodes/NodesPage.tsx
🔇 Additional comments (8)
frontend/packages/console-dynamic-plugin-sdk/src/api/internal-types.ts (1)
327-332: Good backward-compatible column typing bridge.Line 327 and Line 332 cleanly support hidden-by-default columns (
additional) while preserving legacyTableColumncompatibility viaDataViewManagedColumn.frontend/packages/console-app/src/components/data-view/ConsoleDataView.tsx (1)
289-289: Nice extraction of sharednowrapmodifier constant.Line 289 removes string duplication across list column definitions and keeps styling config consistent.
frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-class.tsx (1)
98-120: Column migration is consistent and correctly wired.Line 102, Line 109, and Line 116 correctly move sorting to
sortFunction, and Line 105/112/119 consistently useMODIFIER_NOWRAP.frontend/packages/console-dynamic-plugin-sdk/src/extensions/console-types.ts (1)
530-533:UseActiveColumnsAPI typing update is aligned with the migration.Line 530 and Line 533 now match the managed-column union model, which keeps old
TableColumnconsumers type-compatible during transition.frontend/packages/console-app/src/components/pdb/PDBList.tsx (1)
116-119: Selector sort implementation now sorts on a semantic value.Line 116–119 fixes the previous object-sort pitfall by normalizing selectors with
selectorToStringbefore comparison.frontend/packages/console-app/src/components/volume-snapshot/volume-snapshot-content.tsx (1)
120-165: VolumeSnapshotContent column migration looks consistent.Line 124–161 correctly uses
sortFunction, and Line 127/135/143/150/157/164 standardizes nowrap modifier usage.frontend/public/components/factory/Table/active-columns-hook.ts (1)
10-18: Type contract update is in sync with managed-column rollout.Line 15 and Line 18 correctly widen this hook to
DataViewManagedColumnwithout changing behavior.frontend/packages/console-app/src/components/nodes/NodesPage.tsx (1)
356-357: No change needed; this implementation is consistent with the framework.The in-place mutation here mirrors both the framework's own string-based sort at line 153 of
useConsoleDataViewData.tsxand thesortWithCSRResourcepattern used throughout this file. CustomsortFunctioncallbacks are expected to mutate and return the array; wrapping with[...data]would create an unnecessary defensive copy that contradicts the established design pattern across the codebase.> Likely an incorrect or invalid review comment.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@vikram-raj: This pull request references CONSOLE-5040 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.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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@frontend/packages/console-app/src/components/data-view/useConsoleDataViewData.tsx`:
- Around line 96-100: The fallback assignment to sortFunction should not accept
boolean TableColumn.sort values; change the logic in useConsoleDataViewData so
that sortFunction is set to column.sort only when typeof (column as
TableColumn<TData>).sort === 'string' (exclude booleans), otherwise keep
undefined; likewise ensure the subsequent sorting-path that inspects sort (used
where sort keys/handlers are applied) only treats string keys or function values
(i.e., guard against boolean sort values) so sorting headers don't become active
without sortable behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: ef4436d9-4317-4bf7-ab87-ad5d43cd111c
📒 Files selected for processing (2)
frontend/packages/console-app/src/components/data-view/useConsoleDataViewData.tsxfrontend/packages/console-app/src/components/nodes/NodesPage.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/packages/console-app/src/components/nodes/NodesPage.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/data-view/useConsoleDataViewData.tsx
🔇 Additional comments (1)
frontend/packages/console-app/src/components/data-view/useConsoleDataViewData.tsx (1)
153-155: Good call on preserving input immutability during sortUsing
[...filteredData]before both string-based and callback sorting avoids mutating upstream state and prevents subtle memoization bugs.Also applies to: 159-159
| const sortFunction = | ||
| 'sortFunction' in column && column.sortFunction !== undefined | ||
| ? column.sortFunction | ||
| : (column as TableColumn<TData>).sort; | ||
|
|
There was a problem hiding this comment.
Normalize legacy sort fallback to exclude boolean values
Line 99 can propagate TableColumn.sort booleans into sortFunction. When this is true, the column gets sortable header state, but the data never sorts because Lines 152–162 only execute for string or function. Restrict fallback to string keys only.
Suggested fix
- const sortFunction =
- 'sortFunction' in column && column.sortFunction !== undefined
- ? column.sortFunction
- : (column as TableColumn<TData>).sort;
+ const legacySort = (column as TableColumn<TData>).sort;
+ const sortFunction =
+ 'sortFunction' in column && column.sortFunction !== undefined
+ ? column.sortFunction
+ : typeof legacySort === 'string'
+ ? legacySort
+ : undefined;
- const headerProps: ThProps = {
- ...props,
- dataLabel: title,
- sort: sortFunction,
- };
+ const headerProps: ThProps = {
+ ...props,
+ dataLabel: title,
+ };Also applies to: 152-162
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@frontend/packages/console-app/src/components/data-view/useConsoleDataViewData.tsx`
around lines 96 - 100, The fallback assignment to sortFunction should not accept
boolean TableColumn.sort values; change the logic in useConsoleDataViewData so
that sortFunction is set to column.sort only when typeof (column as
TableColumn<TData>).sort === 'string' (exclude booleans), otherwise keep
undefined; likewise ensure the subsequent sorting-path that inspects sort (used
where sort keys/handlers are applied) only treats string keys or function values
(i.e., guard against boolean sort values) so sorting headers don't become active
without sortable behavior.
🔍 Pre-Push Review - Critical Issue FoundArray Mutation in Labels Column Sort Function 📍 Location: IssueThe Labels column Current code (line 357): sortFunction: (data, direction) =>
data.sort(sortResourceByValue(direction, nodeRowLabelsSortKey)),Why This MattersMutating the Consequence if Not FixedThe original Suggested FixChange line 357 to: sortFunction: (data, direction) =>
[...data].sort(sortResourceByValue(direction, nodeRowLabelsSortKey)),ContextThis is ironic: the refactoring correctly fixes immutability in Generated by Claude Code using dual AI review (Claude + CodeRabbit) |
- Make array copies explicit in custom sort functions (NodesPage, PDBList) - Remove unsafe 'any' cast from type guard for better type safety - Add comprehensive JSDoc documentation for DataViewManagedColumn public API These changes address code quality issues identified during pre-push review: - Explicit array copying improves code clarity and prevents confusion - Proper type narrowing maintains TypeScript's type safety guarantees - JSDoc comments help external plugin developers understand the API Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
@vikram-raj: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
| // TODO(react18): Remove this type - CONSOLE-5040 | ||
| /** | ||
| * Temporary type to allow type checking to pass on DataView instances that use | ||
| * `TableColumn` as their column type, which is incorrect. | ||
| * | ||
| * @internal This type is internal to console only. | ||
| * | ||
| * @deprecated Always use {@link ConsoleDataViewColumn} or {@link DataViewTh}. | ||
| */ | ||
| export type ConsoleDataViewTh = | ||
| | DataViewTh | ||
| | { | ||
| /** Table head cell node */ | ||
| cell?: ReactNode; | ||
| /** Props passed to Th */ | ||
| props?: ThProps; | ||
| }; |
There was a problem hiding this comment.
Please address this TODO by replacing instances of ConsoleDataViewTh with DataViewTh from PatternFly
7 participants
Summary by CodeRabbit
New Features
Style