CONSOLE-5132: Migrate console plugins table to DataView - #16942
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@logonoff: This pull request references CONSOLE-5132 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 "5.0.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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe change extracts console plugin status logic, adds a reusable plugin list page with development and operator-backed data sources, adds bulk enable and disable actions, and registers the page for ChangesConsole plugin management
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ConsolePluginsListPage
participant ConsolePluginsTable
participant ConsolePluginResources
participant ConsoleOperatorConfig
participant useConsolePluginBulkActions
ConsolePluginsListPage->>ConsoleOperatorConfig: load plugin configuration
ConsolePluginsListPage->>ConsolePluginsTable: select plugin data source
ConsolePluginsTable->>ConsolePluginResources: load operator-backed resources
ConsolePluginResources-->>ConsolePluginsTable: return plugin status data
ConsoleOperatorConfig-->>ConsolePluginsTable: return enabled configuration
ConsolePluginsTable->>useConsolePluginBulkActions: provide selected plugin rows
useConsolePluginBulkActions->>ConsoleOperatorConfig: apply enable or disable JSON patch
ConsoleOperatorConfig-->>useConsolePluginBulkActions: return patch result
🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
frontend/packages/console-app/src/components/console-operator/ConsolePluginsTable.tsx (3)
364-364: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the
anytype argument with the concrete row type.
ConsoleDataView<ConsolePluginTableRow, any, PluginFilters>disables checking for the second type parameter. Supply the intended type so column and row helpers stay type-checked.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/packages/console-app/src/components/console-operator/ConsolePluginsTable.tsx` at line 364, Update the ConsoleDataView instantiation in ConsolePluginsTable to replace the any type argument with the concrete data type expected by the component, using the existing ConsolePluginTableRow or relevant row-model symbol so column and row helpers remain type-checked.
188-199: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the shared selection column id instead of the
'select'literal.Line 193 compares
idwith the literal'select'.createSelectionColumnowns that id. If the helper changes the id, this branch stops matching and the checkbox cell rendersDASH. Import the id constant fromdataViewSelectionHelpersif one is exported, or export one.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/packages/console-app/src/components/console-operator/ConsolePluginsTable.tsx` around lines 188 - 199, The row-cell rendering logic in ConsolePluginsTable must stop comparing the column id to the literal 'select'. Reuse the selection-column id owned by createSelectionColumn from dataViewSelectionHelpers, exporting it there if necessary, and use that shared symbol for the cellContent branch.
424-435: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueBuild one lookup map for plugin info instead of filtering per row.
The row mapper runs
pluginInfo.filter(...).find(...)twice for every plugin, so the work is proportional to rows × plugin info entries and allocates two arrays per row. Build a singleMapkeyed bymanifest.nameoutside the loop, then read the status from the matched entry.♻️ Proposed refactor
const rows = useMemo<ConsolePluginTableRow[]>(() => { if (!consolePluginsLoaded) { return []; } + const pluginInfoByName = new Map(pluginInfo.map((entry) => [entry.manifest.name, entry])); return consolePlugins.map((plugin) => { const pluginName = plugin?.metadata?.name; const enabled = enabledPlugins.includes(pluginName); - - const loadedPluginInfo = pluginInfo - .filter((p) => p.status === 'loaded') - .find((i) => i.manifest.name === pluginName); - - const notLoadedPluginInfo = pluginInfo - .filter((p) => p.status !== 'loaded') - .find((i) => i.manifest.name === pluginName); + const matchedPluginInfo = pluginInfoByName.get(pluginName); + const loadedPluginInfo = + matchedPluginInfo?.status === 'loaded' ? matchedPluginInfo : undefined; + const notLoadedPluginInfo = + matchedPluginInfo?.status !== 'loaded' ? matchedPluginInfo : undefined;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/packages/console-app/src/components/console-operator/ConsolePluginsTable.tsx` around lines 424 - 435, In the row-mapping code around the consolePlugins mapper, build one lookup Map from pluginInfo keyed by manifest.name before iterating rows, then retrieve each plugin’s matched info once and derive loaded versus not-loaded status from that entry. Remove the per-row filter(...).find(...) calls and preserve the existing behavior when no matching plugin info exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@frontend/packages/console-app/src/components/console-operator/ConsoleOperatorConfig.tsx`:
- Around line 24-31: In the route entry using ConsolePluginsListPage, add the
i18n extraction marker comment immediately before the nameKey property,
referencing console-app~Console plugins, so the existing translation key remains
discoverable by the extractor.
In
`@frontend/packages/console-app/src/components/console-operator/ConsolePluginsTable.tsx`:
- Around line 483-491: Update useConsoleOperatorConfigData to return the
useK8sWatchResource error alongside its loaded state, then adjust
ConsolePluginsListPage to show a loading indicator while the operator config is
loading and render PluginsPageComponent with an empty obj when loading fails or
the config is unavailable. Preserve rendering of the ConsolePlugin list
independently of operator-config availability.
- Around line 447-457: Remove the unused errorCause property from the fallback
row object in the plugin row builder, keeping errorMessage and the existing
failed-status behavior unchanged. Do not add or render errorCause, since
ConsolePluginTableRow and ConsolePluginStatus currently do not consume it.
- Around line 243-257: Stabilize the selection configuration used by
ConsoleDataView to prevent its onFilteredSelectionChange effect from
retriggering on every render. Memoize the inline getItemId callback, or memoize
the complete selectionProps object, while preserving the existing filtered
selection behavior in handleFilteredSelectionChange.
In
`@frontend/packages/console-app/src/components/console-operator/consolePluginStatus.tsx`:
- Around line 67-104: Update useConsoleOperatorConfigData so the access-review
result assigned to canPatchConsoleOperatorConfig contains only the allowed
boolean, by destructuring the first value from useAccessReview or using
useAccessReviewAllowed. Preserve the existing permission check in
ConsolePluginEnabledStatus while ensuring unauthorized users do not see the edit
button.
In
`@frontend/packages/console-app/src/components/console-operator/useConsolePluginBulkActions.tsx`:
- Around line 33-41: Guard both bulk plugin updates against stale configuration:
in the enable handler at
frontend/packages/console-app/src/components/console-operator/useConsolePluginBulkActions.tsx
lines 33-41, prepend a JSON Patch test for the observed currentPlugins value or
append individual plugins via /spec/plugins/-; in the disable handler at lines
50-58, prepend the same test before replacing the array. Preserve rejection on
concurrent changes so callers can retry with refreshed configuration.
- Around line 75-77: Update both count-based descriptions in
useConsolePluginBulkActions to use i18next singular wording for one item and
plural wording for multiple items, while preserving the count interpolation. Add
the matching singular and plural entries to the console-app translation catalog
so extraction generates the corresponding _one and _other keys.
---
Nitpick comments:
In
`@frontend/packages/console-app/src/components/console-operator/ConsolePluginsTable.tsx`:
- Line 364: Update the ConsoleDataView instantiation in ConsolePluginsTable to
replace the any type argument with the concrete data type expected by the
component, using the existing ConsolePluginTableRow or relevant row-model symbol
so column and row helpers remain type-checked.
- Around line 188-199: The row-cell rendering logic in ConsolePluginsTable must
stop comparing the column id to the literal 'select'. Reuse the selection-column
id owned by createSelectionColumn from dataViewSelectionHelpers, exporting it
there if necessary, and use that shared symbol for the cellContent branch.
- Around line 424-435: In the row-mapping code around the consolePlugins mapper,
build one lookup Map from pluginInfo keyed by manifest.name before iterating
rows, then retrieve each plugin’s matched info once and derive loaded versus
not-loaded status from that entry. Remove the per-row filter(...).find(...)
calls and preserve the existing behavior when no matching plugin info exists.
🪄 Autofix
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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ed66236c-96da-4191-9be5-1d416ecea519
📒 Files selected for processing (12)
frontend/packages/console-app/console-extensions.jsonfrontend/packages/console-app/package.jsonfrontend/packages/console-app/src/components/console-operator/ConsoleOperatorConfig.tsxfrontend/packages/console-app/src/components/console-operator/ConsolePluginCSPStatusDetail.tsxfrontend/packages/console-app/src/components/console-operator/ConsolePluginEnabledStatusDetail.tsxfrontend/packages/console-app/src/components/console-operator/ConsolePluginStatusDetail.tsxfrontend/packages/console-app/src/components/console-operator/ConsolePluginsTable.tsxfrontend/packages/console-app/src/components/console-operator/__tests__/ConsolePluginCSPStatusDetail.spec.tsxfrontend/packages/console-app/src/components/console-operator/__tests__/ConsolePluginEnabledStatusDetail.spec.tsxfrontend/packages/console-app/src/components/console-operator/__tests__/ConsolePluginStatusDetail.spec.tsxfrontend/packages/console-app/src/components/console-operator/consolePluginStatus.tsxfrontend/packages/console-app/src/components/console-operator/useConsolePluginBulkActions.tsx
9401a89 to
1066a22
Compare
openshift-ci
Bot
added
the
kind/i18n
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/packages/console-app/locales/en/console-app.json`:
- Around line 71-74: Remove the literal “_one” and “_other” suffixes from the
translated values for the four “Applies to {{count}} selected plugins”
localization entries, while keeping those suffixes on their keys to preserve
plural selection.
🪄 Autofix
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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8a6446b0-77a1-43dc-bb79-1f1b6e29e124
📒 Files selected for processing (4)
frontend/packages/console-app/locales/en/console-app.jsonfrontend/packages/console-app/src/components/console-operator/ConsoleOperatorConfig.tsxfrontend/packages/console-app/src/components/console-operator/ConsolePluginsTable.tsxfrontend/packages/console-app/src/components/console-operator/consolePluginStatus.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/packages/console-app/src/components/console-operator/ConsolePluginsTable.tsx
d85337e to
59e646b
Compare
|
/assign @rhamilto |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@frontend/packages/console-app/src/components/console-operator/useConsolePluginBulkActions.tsx`:
- Around line 23-24: Update useConsolePluginBulkActions to retain the
errorMessage returned by usePromiseHandler, expose it through the hook’s
user-visible UI or announcement path, and stop swallowing handler rejections so
failures are reported. Only clear the current selection after a patch succeeds;
preserve it when either bulk-action handler fails.
- Around line 33-44: Update the patches construction in
useConsolePluginBulkActions so the /spec/plugins test operation is included only
when consoleOperatorConfig.spec?.plugins exists; when absent, generate only the
add operation, while preserving the existing test-and-replace behavior for an
existing plugins list.
- Around line 46-64: Update handleBulkEnable and handleBulkDisable to construct
JSON patches based on whether spec.plugins exists: use add when creating the
missing field, and avoid testing or replacing an absent path. Replace the empty
catch handlers around handlePromise with the existing usePromiseHandler error
state so patch failures are rendered to users. Leave k8sPatch and its CSRF
handling unchanged.
🪄 Autofix
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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7106f634-e986-41d1-a320-6f14f6f5ed57
📒 Files selected for processing (2)
frontend/packages/console-app/src/components/console-operator/ConsolePluginsTable.tsxfrontend/packages/console-app/src/components/console-operator/useConsolePluginBulkActions.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/packages/console-app/src/components/console-operator/ConsolePluginsTable.tsx
|
/assign @vojtechszocs |
dcac0b0 to
2031798
Compare
openshift-ci
Bot
added
component/shared
2031798 to
5554f1f
Compare
openshift-ci
Bot
removed
the
needs-rebase
00ca1b0 to
58abd22
Compare
|
@logonoff: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
jseseCCS
left a comment
jseseCCS
left a comment
There was a problem hiding this comment.
made a very few comments. approving in good faith that you'll review and apply as appropriate.
| @@ -247,6 +252,7 @@ | |||
| "Edit update strategy": "Edit update strategy", | |||
| "Edit user preferences to not show again": "Edit user preferences to not show again", | |||
There was a problem hiding this comment.
unclear. if a toggle or checkbox label that = user won't see prompt again, --> Don't show this again
?
if nothing else, so it's less clunky --> Hide this prompt in preferences/?
There was a problem hiding this comment.
This was part of the lightspeed modal and would require a UX change as IIRC the button just takes you to the preferences page
|
approving in good faith. made just a few comments. :) |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jseseCCS, logonoff 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 |
|
/label docs-approved |
openshift-ci
Bot
added
the
docs-approved
…ection Replace the manual PatternFly Table in the console plugins tab with ConsoleDataView, following the NodesPage pattern. This adds: - Checkbox selection with bulk enable/disable actions - Name and status/enabled checkbox filters - Pagination via DataView - A dedicated list page at /k8s/cluster/console.openshift.io~v1~ConsolePlugin The shared status components (ConsolePluginStatus, ConsolePluginEnabledStatus, ConsolePluginCSPStatus) are extracted to consolePluginStatus.tsx to avoid a circular dependency between ConsoleOperatorConfig and ConsolePluginsTable. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
58abd22 to
7010eba
Compare
5 participants
Analysis / Root cause:
Console plugins table is using the standard PF table and none of the fancy stuff we've been adding lately
Solution description:
Replace the manual PatternFly Table in the console plugins tab with ConsoleDataView, following the NodesPage pattern. This adds:
v1ConsolePluginThe shared status components (ConsolePluginStatus, ConsolePluginEnabledStatus, ConsolePluginCSPStatus) are extracted to consolePluginStatus.tsx to avoid a circular dependency between ConsoleOperatorConfig and ConsolePluginsTable.
Screenshots / screen recording:
dev table:
prod table (running locally so not all the data is working):
video (on cluster):
Screen.Recording.2026-08-11.at.6.02.49.PM.mov
Test cases:
Browser conformance:
Summary by CodeRabbit
New Features