CONSOLE-3769: More openshift/dynamic-plugin-sdk prep work #15738
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@logonoff: This pull request references CONSOLE-3769 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 either version "4.21." or "openshift-4.21.", but it targets "openshift-4.15" instead. 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. |
|
/label px-approved |
openshift-ci
bot
added
component/core
logonoff
force-pushed
the
CONSOLE-3769-s2e22-casino-night
branch
2 times, most recently
from
23217fa to
3840f6e
Compare
WalkthroughNormalized dynamic plugin status strings to lowercase, changed usePluginInfo to return an array (no tuple/boolean), removed predicate helpers and i18nNamespaces from PluginStore, updated public typings and call sites, and aligned tests and UI code to the new shapes and status literals. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
✨ 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 (18)
🚧 Files skipped from review as they are similar to previous changes (15)
🧰 Additional context used📓 Path-based instructions (1)**⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (9)
Comment |
![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/public/components/notification-drawer.tsx (1)
165-165: Consider adding optional chaining for consistency.While the filter at line 165 uses
plugin.status === 'failed', other files in this PR use optional chaining (plugin?.status). For consistency and defensive coding, consider:- const failedPlugins = pluginInfoEntries.filter((plugin) => plugin.status === 'failed'); + const failedPlugins = pluginInfoEntries.filter((plugin) => plugin?.status === 'failed');frontend/packages/console-app/src/components/dashboards-page/dynamic-plugins-health-resource/DynamicPluginsPopover.tsx (1)
21-26: Inconsistent optional chaining usage.Lines 21 and 26 use optional chaining (
plugin?.status), but lines 24-25 don't. While the logic may work sincenotLoadedDynamicPluginInfoalready filtered out invalid entries, using consistent optional chaining improves code safety and readability.const notLoadedDynamicPluginInfo = pluginInfoEntries.filter( (plugin) => plugin?.status !== 'loaded', ); - const failedPlugins = notLoadedDynamicPluginInfo.filter((plugin) => plugin.status === 'failed'); - const pendingPlugins = notLoadedDynamicPluginInfo.filter((plugin) => plugin.status === 'pending'); + const failedPlugins = notLoadedDynamicPluginInfo.filter((plugin) => plugin?.status === 'failed'); + const pendingPlugins = notLoadedDynamicPluginInfo.filter((plugin) => plugin?.status === 'pending'); const loadedPlugins = pluginInfoEntries.filter((plugin) => plugin?.status === 'loaded');frontend/packages/console-app/src/components/console-operator/ConsoleOperatorConfig.tsx (1)
338-365: Consider more explicit status filtering for clarity.Line 342 uses
p?.status !== 'loaded'to find non-loaded plugins, which matches both 'pending' and 'failed' statuses. While functionally correct, an explicit check would improve readability:- const notLoadedPluginInfo = pluginInfo - .filter((p) => p?.status !== 'loaded') - .find((i) => i?.pluginName === pluginName); + const notLoadedPluginInfo = pluginInfo + .filter((p) => p?.status === 'pending' || p?.status === 'failed') + .find((i) => i?.pluginName === pluginName);
📜 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 (18)
frontend/packages/console-app/src/components/console-operator/ConsoleOperatorConfig.tsx(10 hunks)frontend/packages/console-app/src/components/console-operator/ConsolePluginDescriptionDetail.tsx(1 hunks)frontend/packages/console-app/src/components/console-operator/ConsolePluginEnabledStatusDetail.tsx(2 hunks)frontend/packages/console-app/src/components/console-operator/ConsolePluginStatusDetail.tsx(2 hunks)frontend/packages/console-app/src/components/console-operator/ConsolePluginVersionDetail.tsx(1 hunks)frontend/packages/console-app/src/components/dashboards-page/dynamic-plugins-health-resource/DynamicPluginsPopover.tsx(1 hunks)frontend/packages/console-app/src/components/dashboards-page/dynamic-plugins-health-resource/status.ts(1 hunks)frontend/packages/console-app/src/hooks/useCSPViolationDetector.tsx(1 hunks)frontend/packages/console-dynamic-plugin-sdk/src/runtime/__tests__/plugin-dependencies.spec.ts(2 hunks)frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-dependencies.ts(1 hunks)frontend/packages/console-plugin-sdk/src/__tests__/store.spec.ts(6 hunks)frontend/packages/console-plugin-sdk/src/api/usePluginInfo.ts(3 hunks)frontend/packages/console-plugin-sdk/src/store.ts(5 hunks)frontend/public/components/about-modal.tsx(1 hunks)frontend/public/components/app-contents.tsx(1 hunks)frontend/public/components/notification-drawer.tsx(3 hunks)frontend/public/i18n.js(1 hunks)frontend/public/plugins.ts(1 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/public/i18n.jsfrontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-dependencies.tsfrontend/packages/console-app/src/components/console-operator/ConsolePluginStatusDetail.tsxfrontend/public/components/app-contents.tsxfrontend/packages/console-app/src/components/console-operator/ConsolePluginEnabledStatusDetail.tsxfrontend/packages/console-plugin-sdk/src/__tests__/store.spec.tsfrontend/packages/console-app/src/components/dashboards-page/dynamic-plugins-health-resource/status.tsfrontend/public/plugins.tsfrontend/packages/console-dynamic-plugin-sdk/src/runtime/__tests__/plugin-dependencies.spec.tsfrontend/packages/console-app/src/components/console-operator/ConsolePluginVersionDetail.tsxfrontend/packages/console-plugin-sdk/src/store.tsfrontend/packages/console-app/src/components/console-operator/ConsolePluginDescriptionDetail.tsxfrontend/public/components/about-modal.tsxfrontend/public/components/notification-drawer.tsxfrontend/packages/console-plugin-sdk/src/api/usePluginInfo.tsfrontend/packages/console-app/src/components/console-operator/ConsoleOperatorConfig.tsxfrontend/packages/console-app/src/components/dashboards-page/dynamic-plugins-health-resource/DynamicPluginsPopover.tsxfrontend/packages/console-app/src/hooks/useCSPViolationDetector.tsx
🔇 Additional comments (24)
frontend/public/plugins.ts (1)
29-29: LGTM! PluginStore constructor updated to remove i18nNamespaces.The removal of the i18nNamespaces parameter aligns with the PR's objective to migrate i18n namespace handling to SERVER_FLAGS. This change is consistent with the broader refactoring across the codebase.
frontend/packages/console-dynamic-plugin-sdk/src/runtime/__tests__/plugin-dependencies.spec.ts (1)
28-50: LGTM! Test helpers updated with lowercase status strings.The test helper functions correctly use lowercase status values ('loaded', 'pending', 'failed') to align with the production code changes. This ensures test data matches the expected runtime behavior.
frontend/public/components/app-contents.tsx (1)
158-165: LGTM! Correctly adapted to updated usePluginInfo return type.The change from tuple destructuring to direct value assignment is correct, and the memoized computation of
allPluginsProcessedproperly replaces the previous tuple value. The dependency array correctly includespluginInfoEntries, and the logic accurately checks whether all plugins have completed processing (status !== 'pending').frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-dependencies.ts (1)
120-133: LGTM! Status comparisons updated to lowercase.The status checks correctly use lowercase string literals ('loaded', 'failed') to match the normalized plugin status values. The dependency resolution logic remains unchanged and functions correctly with the updated status values.
frontend/packages/console-app/src/hooks/useCSPViolationDetector.tsx (1)
103-112: LGTM! Status checks updated to use lowercase strings.The replacement of
isLoadedDynamicPluginInfo(entry)withentry?.status === 'loaded'correctly removes dependency on the type guard predicate while maintaining the same logic. The optional chaining ensures safe property access, and the lowercase status comparison aligns with the normalized plugin status values.frontend/packages/console-app/src/components/console-operator/ConsolePluginStatusDetail.tsx (2)
7-19: LGTM! Correctly adapted to updated usePluginInfo return type.The changes properly update the hook usage to receive a direct value instead of a tuple, use lowercase status checks, and correctly expand the memoization dependency array to include both
pluginInfoEntriesandpluginName.
21-25: LGTM! Status comparison uses lowercase string.The error message extraction correctly uses the lowercase 'failed' status check, aligning with the normalized plugin status values.
frontend/packages/console-app/src/components/dashboards-page/dynamic-plugins-health-resource/status.ts (1)
4-16: LGTM! Health state checks updated to lowercase status strings.The status string comparisons are correctly normalized to lowercase ('failed', 'pending', 'loaded') while maintaining the existing health state determination logic. The function correctly returns ERROR for any failed plugins, PROGRESS for any pending plugins, OK when all are loaded, and UNKNOWN otherwise.
frontend/public/i18n.js (1)
78-78: LGTM! i18n namespaces migrated to SERVER_FLAGS.The migration from
pluginStore.getI18nNamespaces()towindow.SERVER_FLAGS.i18nNamespacesaligns with the PR's objective to remove i18n namespace handling from PluginStore.Verification confirms that
window.SERVER_FLAGS.i18nNamespacesis always defined as an array: in the backend atcmd/bridge/main.golines 246-255,i18nNamespacesis initialized as[]string{}and remains at least an empty slice[]regardless of whether the flag is provided. This guarantees the frontend code atfrontend/public/i18n.js:78will safely executeArray.from(new Set(window.SERVER_FLAGS.i18nNamespaces))without runtime errors.frontend/packages/console-app/src/components/console-operator/ConsolePluginDescriptionDetail.tsx (1)
9-24: LGTM!The refactoring correctly replaces the type guard predicate with direct status string checks. The logic properly handles both loaded and non-loaded states by accessing the appropriate fields based on status.
frontend/packages/console-plugin-sdk/src/__tests__/store.spec.ts (2)
285-292: LGTM!Test expectations correctly updated to reflect lowercase status values ('pending').
585-590: LGTM!All test expectations consistently updated to use lowercase status literals ('loaded', 'pending', 'failed') throughout the test suite, aligning with the SDK changes.
Also applies to: 957-1024
frontend/packages/console-app/src/components/console-operator/ConsolePluginEnabledStatusDetail.tsx (1)
12-41: LGTM!The refactoring correctly removes the type guard predicate in favor of direct status checks. The logic properly handles the
developmentModeflag and safely accesses plugin fields using optional chaining.frontend/public/components/about-modal.tsx (1)
35-54: LGTM!The refactoring correctly replaces the type guard with a direct status check using optional chaining. The filter and sort logic properly handles loaded plugins.
frontend/public/components/notification-drawer.tsx (1)
246-246: LGTM!The hook usage correctly updated to direct value assignment.
frontend/packages/console-app/src/components/console-operator/ConsolePluginVersionDetail.tsx (1)
7-22: LGTM!The refactoring correctly replaces the type guard with direct status checks. The logic safely accesses plugin metadata using optional chaining and properly handles both loaded and non-loaded states.
frontend/packages/console-plugin-sdk/src/api/usePluginInfo.ts (1)
25-58: LGTM!The API change from tuple to single value return is correctly implemented. The removal of
allPluginsProcessedtracking simplifies the hook while maintaining the subscription pattern. Documentation and example usage appropriately updated.frontend/packages/console-app/src/components/console-operator/ConsoleOperatorConfig.tsx (4)
93-93: LGTM: Status comparisons normalized to lowercase.The status comparisons correctly use lowercase 'failed' to align with the updated status literals from the plugin store.
Also applies to: 359-361
297-316: LGTM: DevPluginsPage updated for new usePluginInfo API.The component correctly adapts to the new
usePluginInfoAPI that returns a single value instead of a tuple. The filtering and mapping logic is clear and correct.
158-158: LGTM: ConsolePluginsTable props simplified.The removal of the
loadedprop aligns with the PR objective to removeallPluginsProcessedfrom the plugin info API.Also applies to: 434-436
2-2: Verify thatPluginInfoEntry['status']from the external SDK supports the required status literals.The import of
PluginInfoEntryfrom@openshift/dynamic-plugin-sdk@^5.0.1is syntactically correct and used at lines 422 and 439. However, the external SDK'sPluginInfoEntry['status']type definition cannot be verified in the codebase. Ensure the external SDK's type supports the status literals used here ('loaded','pending','failed'), which match the internalDynamicPluginInfotype defined infrontend/packages/console-plugin-sdk/src/store.ts:311-328.Also applies to: 422-422, 439-439
frontend/packages/console-plugin-sdk/src/store.ts (3)
70-82: LGTM: i18nNamespaces removed from PluginStore.The constructor signature correctly removes the
i18nNamespacesparameter, aligning with the PR objective to remove i18n namespace handling from PluginStore.
226-267: LGTM: Status literals normalized to lowercase.The
getPluginInfomethod correctly returns lowercase status values ('loaded', 'failed', 'pending'), ensuring consistency with the type definitions and consuming components.
311-329: LGTM: Type definitions align with runtime status values.The type definitions correctly use lowercase string literal types that match the runtime values returned by
getPluginInfo. This ensures type safety when checking plugin status.
logonoff
force-pushed
the
CONSOLE-3769-s2e22-casino-night
branch
from
3840f6e to
962bb0d
Compare
| const sortedRows = React.useMemo(() => rows.sort(compare), [rows, compare]); | ||
|
|
||
| return !loaded ? ( | ||
| <LoadingBox /> |
There was a problem hiding this comment.
The loaded prop value comes from usePluginInfo hook's second tuple element
const [pluginInfoEntries, allPluginsProcessed] = usePluginInfo();which is true once all Console plugins have finished loading, i.e. there are no plugins in "pending" state.
So currently <ConsolePluginsTable> renders a loading state until all plugins are done loading.
I think it makes more sense for <ConsolePluginsTable> to render plugin information regardless of the plugin state so 👍 on this change.
frontend/packages/console-app/src/components/console-operator/ConsoleOperatorConfig.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/console-app/src/components/console-operator/ConsoleOperatorConfig.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/console-app/src/components/console-operator/ConsoleOperatorConfig.tsx
Outdated
Show resolved
Hide resolved
...tend/packages/console-app/src/components/console-operator/ConsolePluginDescriptionDetail.tsx
Outdated
Show resolved
Hide resolved
frontend/public/i18n.js
Outdated
| 'vsphere-plugin', | ||
| 'webterminal-plugin', | ||
| ...pluginStore.getI18nNamespaces(), | ||
| ...Array.from(new Set(window.SERVER_FLAGS.i18nNamespaces)), |
There was a problem hiding this comment.
👍 on removing direct reference to pluginStore
| const i18nNamespaces = getI18nNamespaces(); | ||
|
|
||
| export const pluginStore = new PluginStore(activePlugins, dynamicPluginNames, i18nNamespaces); | ||
| export const pluginStore = new PluginStore(activePlugins, dynamicPluginNames); |
There was a problem hiding this comment.
👍 pluginStore should not be concerned with i18n aspect.
There was a problem hiding this comment.
Actionable comments posted: 1
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/console-operator/ConsoleOperatorConfig.tsx (1)
354-365: Fix null safety issue with optional chaining.Lines 359 and 361 access
notLoadedPluginInfo.statuswithout optional chaining, butnotLoadedPluginInfocan be undefined (it's the result of.find()on lines 341-343). This will cause a runtime error when a ConsolePlugin CR exists but hasn't been found in the plugin info array.Apply this diff to add optional chaining:
return { name: plugin?.metadata?.name, enabled, status: notLoadedPluginInfo?.status, errorMessage: - notLoadedPluginInfo.status === 'failed' ? notLoadedPluginInfo?.errorMessage : undefined, + notLoadedPluginInfo?.status === 'failed' ? notLoadedPluginInfo?.errorMessage : undefined, errorCause: - notLoadedPluginInfo.status === 'failed' + notLoadedPluginInfo?.status === 'failed' ? notLoadedPluginInfo?.errorCause?.toString() : undefined, };
♻️ Duplicate comments (2)
frontend/public/components/notification-drawer.tsx (1)
165-165: LGTM! Implementation matches past review suggestion.The filtering logic correctly uses lowercase
'failed'status and removes the dependency on type guard functions, aligning with the SDK's PluginInfoEntry.status convention.frontend/public/components/about-modal.tsx (1)
39-39: LGTM! Implementation matches past review suggestion.The filtering logic correctly uses lowercase
'loaded'status and removes the dependency on theisLoadedDynamicPluginInfotype guard, aligning with the SDK's PluginInfoEntry.status convention.
📜 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 (11)
frontend/packages/console-app/src/components/console-operator/ConsoleOperatorConfig.tsx(10 hunks)frontend/packages/console-app/src/components/console-operator/ConsolePluginDescriptionDetail.tsx(1 hunks)frontend/packages/console-app/src/components/console-operator/ConsolePluginEnabledStatusDetail.tsx(2 hunks)frontend/packages/console-app/src/components/console-operator/ConsolePluginStatusDetail.tsx(2 hunks)frontend/packages/console-app/src/components/console-operator/ConsolePluginVersionDetail.tsx(1 hunks)frontend/packages/console-app/src/components/dashboards-page/dynamic-plugins-health-resource/DynamicPluginsPopover.tsx(1 hunks)frontend/packages/console-app/src/hooks/useCSPViolationDetector.tsx(1 hunks)frontend/packages/console-plugin-sdk/src/api/usePluginInfo.ts(3 hunks)frontend/public/components/about-modal.tsx(1 hunks)frontend/public/components/app-contents.tsx(1 hunks)frontend/public/components/notification-drawer.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- frontend/packages/console-app/src/hooks/useCSPViolationDetector.tsx
- frontend/packages/console-app/src/components/console-operator/ConsolePluginStatusDetail.tsx
- frontend/packages/console-app/src/components/console-operator/ConsolePluginDescriptionDetail.tsx
- frontend/packages/console-app/src/components/console-operator/ConsolePluginEnabledStatusDetail.tsx
- frontend/public/components/app-contents.tsx
- frontend/packages/console-app/src/components/dashboards-page/dynamic-plugins-health-resource/DynamicPluginsPopover.tsx
- frontend/packages/console-plugin-sdk/src/api/usePluginInfo.ts
🧰 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/notification-drawer.tsxfrontend/public/components/about-modal.tsxfrontend/packages/console-app/src/components/console-operator/ConsoleOperatorConfig.tsxfrontend/packages/console-app/src/components/console-operator/ConsolePluginVersionDetail.tsx
🔇 Additional comments (9)
frontend/public/components/notification-drawer.tsx (1)
246-246: LGTM! Correctly updated to match SDK API.The change from tuple destructuring to direct value assignment aligns with the SDK's
usePluginInforeturn type and is consistent with similar updates across the codebase.frontend/public/components/about-modal.tsx (1)
35-35: LGTM! Correctly updated to match SDK API.The direct value assignment from
usePluginInfo()correctly reflects the API change to return a single array value instead of a tuple.frontend/packages/console-app/src/components/console-operator/ConsoleOperatorConfig.tsx (7)
2-2: LGTM! SDK type import added.The import of
PluginInfoEntryfrom the SDK correctly supports the migration away from internal plugin types.Also applies to: 34-34
93-93: LGTM! Status comparison updated to lowercase.The change to lowercase
'failed'correctly aligns with the SDK's PluginInfoEntry status values.
158-158: LGTM! Removed loading state prop.The removal of the
loadedprop aligns with the decision to render plugin information regardless of loading state, improving UX as noted in previous review feedback.
222-293: LGTM! Simplified rendering without loading state.The table now renders immediately with available plugin data, providing better UX by showing information as soon as it's available rather than waiting for all plugins to load.
297-316: LGTM! Simplified plugin info usage.The changes correctly update to the new
usePluginInfoAPI that returns a single array, and replace type guards with direct status checks for better clarity.
338-343: LGTM! Replaced type guards with direct status checks.The filter logic using
p.status === 'loaded'andp.status !== 'loaded'is cleaner and more direct than using type guard functions.
422-422: LGTM! Types updated to use SDK types.The type changes from internal
DynamicPluginInfo['status']to SDKPluginInfoEntry['status']correctly align with the SDK migration goals.Also applies to: 439-439
frontend/packages/console-app/src/components/console-operator/ConsolePluginVersionDetail.tsx
Show resolved
Hide resolved
logonoff
force-pushed
the
CONSOLE-3769-s2e22-casino-night
branch
2 times, most recently
from
5f7964c to
6af823f
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 (1)
frontend/packages/console-app/src/components/console-operator/ConsoleOperatorConfig.tsx (1)
320-368: Protect againstnotLoadedPluginInfobeingundefinedand align status/error typingIn
PluginsPage,notLoadedPluginInfo(Lines 341-343) is derived via.find(...)and can beundefined(plugin info not present yet, static-only plugin, etc.). However, Lines 359 and 361 accessnotLoadedPluginInfo.statuswithout a null check, which will throw at runtime whennotLoadedPluginInfois missing. Additionally,statusinConsolePluginTableRowis non-optional, but you assignnotLoadedPluginInfo?.status, which can beundefined.You can fix both by introducing a local
statusvariable derived safely and reusing it for error fields:const notLoadedPluginInfo = pluginInfo .filter((p) => p.status !== 'loaded') .find((i) => i?.pluginName === pluginName); if (loadedPluginInfo) { return { name: plugin?.metadata?.name, version: loadedPluginInfo?.metadata?.version, description: loadedPluginInfo?.metadata?.customProperties?.console?.description, enabled, status: loadedPluginInfo?.status, hasCSPViolations: cspViolations[plugin.metadata.name] ?? false, }; } + const status = notLoadedPluginInfo?.status; return { name: plugin?.metadata?.name, enabled, - status: notLoadedPluginInfo?.status, - errorMessage: - notLoadedPluginInfo.status === 'failed' ? notLoadedPluginInfo?.errorMessage : undefined, - errorCause: - notLoadedPluginInfo.status === 'failed' - ? notLoadedPluginInfo?.errorCause?.toString() - : undefined, + status, + errorMessage: + status === 'failed' ? notLoadedPluginInfo?.errorMessage : undefined, + errorCause: + status === 'failed' ? notLoadedPluginInfo?.errorCause?.toString() : undefined, };This avoids crashing when
notLoadedPluginInfois absent and keeps status/error fields consistent with the status value.
♻️ Duplicate comments (3)
frontend/public/components/notification-drawer.tsx (1)
165-165: LGTM! Status filtering updated correctly.The inline status check using lowercase
'failed'correctly replaces the removed helper function and aligns with the SDK's status representation.frontend/packages/console-app/src/components/dashboards-page/dynamic-plugins-health-resource/DynamicPluginsPopover.tsx (1)
21-26: LGTM! Filtering logic correctly migrated to inline status checks.The inline status checks correctly replace the removed helper predicates:
- Line 21-23: Filters for
status !== 'loaded'(replacesisNotLoadedDynamicPluginInfo)- Lines 24-26: Correctly filter for specific states using lowercase status strings
The logic is sound and aligns with the PR objective to use lowercase status values and remove helper dependencies.
frontend/packages/console-app/src/components/console-operator/ConsolePluginVersionDetail.tsx (1)
7-21: Guard againstpluginInfobeingundefinedbefore accessingstatus/metadataOn Line 13,
pluginInfoEntries.find(...)can returnundefined(no matching entry yet, plugin missing, orpluginNameundefined), but Line 21 unconditionally readspluginInfo.statusandpluginInfo.metadata.version. This will throw at runtime and likely violatestrictNullChecks.Consider an early guard returning
DASHwhen the entry is missing or not loaded:const pluginInfo = React.useMemo( () => pluginInfoEntries.find((entry) => entry.status === 'loaded' ? entry.metadata.name === pluginName : entry.pluginName === pluginName, ), [pluginInfoEntries, pluginName], ); - return pluginInfo.status === 'loaded' ? <>{pluginInfo.metadata.version}</> : <>{DASH}</>; + if (!pluginInfo || pluginInfo.status !== 'loaded') { + return <>{DASH}</>; + } + + return <>{pluginInfo.metadata.version}</>;This preserves behavior while avoiding crashes when the plugin info is absent or still loading.
🧹 Nitpick comments (1)
frontend/packages/console-app/src/components/console-operator/ConsoleOperatorConfig.tsx (1)
158-294: Avoid mutatingrowsprop when computingsortedRows
sortedRowsis computed viarows.sort(compare)(Line 220), which sorts therowsarray in place. Even though current callers buildrowsfreshly, mutating props is brittle and can cause subtle bugs ifrowsis ever reused or shared.Consider sorting a shallow copy instead:
- const sortedRows = React.useMemo(() => rows.sort(compare), [rows, compare]); + const sortedRows = React.useMemo(() => [...rows].sort(compare), [rows, compare]);This keeps
ConsolePluginsTablepurely functional with respect to itsrowsprop.
📜 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 (18)
frontend/packages/console-app/src/components/console-operator/ConsoleOperatorConfig.tsx(10 hunks)frontend/packages/console-app/src/components/console-operator/ConsolePluginDescriptionDetail.tsx(1 hunks)frontend/packages/console-app/src/components/console-operator/ConsolePluginEnabledStatusDetail.tsx(2 hunks)frontend/packages/console-app/src/components/console-operator/ConsolePluginStatusDetail.tsx(2 hunks)frontend/packages/console-app/src/components/console-operator/ConsolePluginVersionDetail.tsx(1 hunks)frontend/packages/console-app/src/components/dashboards-page/dynamic-plugins-health-resource/DynamicPluginsPopover.tsx(1 hunks)frontend/packages/console-app/src/components/dashboards-page/dynamic-plugins-health-resource/status.ts(1 hunks)frontend/packages/console-app/src/hooks/useCSPViolationDetector.tsx(1 hunks)frontend/packages/console-dynamic-plugin-sdk/src/runtime/__tests__/plugin-dependencies.spec.ts(2 hunks)frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-dependencies.ts(1 hunks)frontend/packages/console-plugin-sdk/src/__tests__/store.spec.ts(6 hunks)frontend/packages/console-plugin-sdk/src/api/usePluginInfo.ts(3 hunks)frontend/packages/console-plugin-sdk/src/store.ts(5 hunks)frontend/public/components/about-modal.tsx(1 hunks)frontend/public/components/app-contents.tsx(1 hunks)frontend/public/components/notification-drawer.tsx(3 hunks)frontend/public/i18n.js(1 hunks)frontend/public/plugins.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- frontend/public/components/about-modal.tsx
- frontend/packages/console-app/src/hooks/useCSPViolationDetector.tsx
- frontend/packages/console-app/src/components/console-operator/ConsolePluginStatusDetail.tsx
- frontend/packages/console-app/src/components/console-operator/ConsolePluginDescriptionDetail.tsx
- frontend/public/i18n.js
- frontend/packages/console-app/src/components/console-operator/ConsolePluginEnabledStatusDetail.tsx
- frontend/packages/console-plugin-sdk/src/store.ts
- frontend/public/components/app-contents.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-plugin-sdk/src/__tests__/store.spec.tsfrontend/public/plugins.tsfrontend/packages/console-app/src/components/dashboards-page/dynamic-plugins-health-resource/DynamicPluginsPopover.tsxfrontend/packages/console-app/src/components/dashboards-page/dynamic-plugins-health-resource/status.tsfrontend/packages/console-plugin-sdk/src/api/usePluginInfo.tsfrontend/packages/console-dynamic-plugin-sdk/src/runtime/__tests__/plugin-dependencies.spec.tsfrontend/packages/console-app/src/components/console-operator/ConsoleOperatorConfig.tsxfrontend/public/components/notification-drawer.tsxfrontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-dependencies.tsfrontend/packages/console-app/src/components/console-operator/ConsolePluginVersionDetail.tsx
🔇 Additional comments (13)
frontend/public/plugins.ts (1)
29-29: LGTM! Simplified PluginStore initialization aligns with SDK.The removal of the
i18nNamespacesparameter improves separation of concerns by decoupling plugin store initialization from i18n management. This change aligns with the PR objective to match the SDK PluginStore signature.frontend/packages/console-dynamic-plugin-sdk/src/runtime/__tests__/plugin-dependencies.spec.ts (1)
28-50: LGTM! Status normalization aligns test data with runtime behavior.The lowercase status values ('loaded', 'pending', 'failed') in the test helpers now match the updated runtime checks in the dependency resolution logic.
frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-dependencies.ts (1)
120-133: LGTM! Runtime status checks correctly updated to lowercase.The status comparisons now use lowercase 'loaded' and 'failed' strings, maintaining consistency with the test helpers and aligning with SDK PluginInfoEntry.status.
frontend/packages/console-plugin-sdk/src/__tests__/store.spec.ts (3)
268-293: LGTM! Status normalization is correctly applied.The test expectations correctly reflect the lowercase status values ('pending') for plugins in initial state. The constructor signature using two parameters aligns with the removal of
i18nNamespacesmentioned in the PR objectives.
583-591: LGTM! Loaded status correctly normalized.The test expectation correctly reflects the lowercase status value ('loaded') for successfully added dynamic plugins.
949-1025: LGTM! Complete status lifecycle correctly normalized.The test expectations correctly reflect all three lowercase status values ('pending', 'loaded', 'failed') throughout the plugin lifecycle. The comprehensive coverage validates proper status transitions from initial state through loading and failure scenarios.
frontend/packages/console-app/src/components/dashboards-page/dynamic-plugins-health-resource/status.ts (1)
6-12: Changes are correct—plugin status values already use lowercase in SDK types.The PR changes align with the actual
DynamicPluginInfotype definitions. The SDK definesLoadedDynamicPluginInfowithstatus: 'loaded'andNotLoadedDynamicPluginInfowithstatus: 'pending'orstatus: 'failed'(all lowercase). The status comparisons in lines 6, 9, and 12 now correctly match the actual type values returned bypluginStore.getPluginInfo().frontend/public/components/notification-drawer.tsx (2)
17-17: LGTM! Import simplified to align with SDK.The removal of
isNotLoadedDynamicPluginInfoaligns with the PR objective to decrease dependence on helper predicates from@console/plugin-sdk.
246-246: LGTM! Updated to match new usePluginInfo API.The change from tuple destructuring to direct assignment correctly reflects the updated
usePluginInfo()return type, which now returnsDynamicPluginInfo[]instead of[DynamicPluginInfo[], boolean].frontend/packages/console-plugin-sdk/src/api/usePluginInfo.ts (1)
16-16: LGTM! API simplified to align with dynamic-plugin-sdk.The hook now returns
DynamicPluginInfo[]directly instead of a tuple with theallPluginsProcessedboolean. This change:
- Aligns with the SDK's
usePluginInfoimplementation- Fulfills the PR objective to remove
allPluginsProcessedfrom usePluginInfo- Is safe per previous review discussion confirming this hook is not exposed via the Console plugin API
Also applies to: 25-25, 58-58
frontend/packages/console-app/src/components/dashboards-page/dynamic-plugins-health-resource/DynamicPluginsPopover.tsx (1)
17-17: LGTM! Updated to use new usePluginInfo API.Correctly changed from tuple destructuring to direct value assignment, matching the updated hook signature.
frontend/packages/console-app/src/components/console-operator/ConsoleOperatorConfig.tsx (2)
2-2: Status typing and import alignment withPluginInfoEntrylook consistentImporting
PluginInfoEntry(Line 2) and wiringConsolePluginStatus,ConsolePluginTableRow.status, andConsolePluginStatusProps.statustoPluginInfoEntry['status'](Lines 90-94, 422-441) matches the status-based approach ('loaded' | 'failed' | ...) and the normalization to lowercase. No issues here.Also applies to: 90-94, 422-441
297-317: Dev plugins pageusePluginInfousage and status filtering look correctUsing the updated
usePluginInfo()return value directly (Line 297) and filtering toplugin.status === 'loaded'before mapping to table rows (Lines 304-313) aligns with the new API and avoids touching pending/failed entries. The CSP violations lookup also correctly keys offplugin.metadata.name. No issues here.
|
/retest-required |
logonoff
force-pushed
the
CONSOLE-3769-s2e22-casino-night
branch
from
6af823f to
4304f21
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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 (18)
frontend/packages/console-app/src/components/console-operator/ConsoleOperatorConfig.tsx(10 hunks)frontend/packages/console-app/src/components/console-operator/ConsolePluginDescriptionDetail.tsx(1 hunks)frontend/packages/console-app/src/components/console-operator/ConsolePluginEnabledStatusDetail.tsx(2 hunks)frontend/packages/console-app/src/components/console-operator/ConsolePluginStatusDetail.tsx(2 hunks)frontend/packages/console-app/src/components/console-operator/ConsolePluginVersionDetail.tsx(1 hunks)frontend/packages/console-app/src/components/dashboards-page/dynamic-plugins-health-resource/DynamicPluginsPopover.tsx(1 hunks)frontend/packages/console-app/src/components/dashboards-page/dynamic-plugins-health-resource/status.ts(1 hunks)frontend/packages/console-app/src/hooks/useCSPViolationDetector.tsx(1 hunks)frontend/packages/console-dynamic-plugin-sdk/src/runtime/__tests__/plugin-dependencies.spec.ts(2 hunks)frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-dependencies.ts(1 hunks)frontend/packages/console-plugin-sdk/src/__tests__/store.spec.ts(6 hunks)frontend/packages/console-plugin-sdk/src/api/usePluginInfo.ts(3 hunks)frontend/packages/console-plugin-sdk/src/store.ts(5 hunks)frontend/public/components/about-modal.tsx(1 hunks)frontend/public/components/app-contents.tsx(1 hunks)frontend/public/components/notification-drawer.tsx(3 hunks)frontend/public/i18n.js(1 hunks)frontend/public/plugins.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- frontend/public/components/about-modal.tsx
- frontend/packages/console-app/src/hooks/useCSPViolationDetector.tsx
- frontend/public/components/notification-drawer.tsx
- frontend/packages/console-app/src/components/dashboards-page/dynamic-plugins-health-resource/status.ts
- frontend/public/components/app-contents.tsx
- frontend/public/plugins.ts
- frontend/packages/console-plugin-sdk/src/tests/store.spec.ts
- frontend/packages/console-app/src/components/console-operator/ConsolePluginEnabledStatusDetail.tsx
- frontend/packages/console-app/src/components/console-operator/ConsolePluginDescriptionDetail.tsx
- frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-dependencies.ts
- frontend/packages/console-app/src/components/console-operator/ConsolePluginStatusDetail.tsx
- frontend/packages/console-app/src/components/console-operator/ConsolePluginVersionDetail.tsx
- frontend/packages/console-dynamic-plugin-sdk/src/runtime/tests/plugin-dependencies.spec.ts
🧰 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-plugin-sdk/src/api/usePluginInfo.tsfrontend/public/i18n.jsfrontend/packages/console-app/src/components/console-operator/ConsoleOperatorConfig.tsxfrontend/packages/console-app/src/components/dashboards-page/dynamic-plugins-health-resource/DynamicPluginsPopover.tsxfrontend/packages/console-plugin-sdk/src/store.ts
🔇 Additional comments (11)
frontend/packages/console-plugin-sdk/src/api/usePluginInfo.ts (2)
16-16: LGTM: API simplified to align with SDK.The return type change from tuple to array and the updated documentation example correctly reflect the SDK alignment. Based on learnings from past review comments, this change is safe since
usePluginInfois not exposed via the Console plugin API.Also applies to: 25-25
58-58: LGTM: Return statement matches new signature.The simplified return value correctly implements the new single-value return type, eliminating the
allPluginsProcessedtracking as intended.frontend/packages/console-app/src/components/dashboards-page/dynamic-plugins-health-resource/DynamicPluginsPopover.tsx (2)
17-17: LGTM: Hook usage correctly updated.The
usePluginInfo()call correctly reflects the new single-value return type, removing the tuple destructuring.
21-26: LGTM: Status filtering correctly implemented with lowercase strings.The inline status-based filtering correctly replaces the removed
isLoadedDynamicPluginInfoandisNotLoadedDynamicPluginInfohelper predicates. The lowercase status values ('loaded', 'failed', 'pending') align with the SDK as intended.frontend/packages/console-plugin-sdk/src/store.ts (1)
70-82: LGTM! SDK alignment changes are consistent.The removal of the
i18nNamespacesparameter from the constructor and the lowercase status literals throughout ('loaded','failed','pending') are breaking changes, but they correctly align with the SDK as stated in the PR objectives. The implementation is consistent across runtime values and type definitions.Also applies to: 226-267, 311-330
frontend/packages/console-app/src/components/console-operator/ConsoleOperatorConfig.tsx (6)
158-158: LGTM - improved rendering logic.Removing the
loadedprop and rendering unconditionally improves UX by displaying plugin information regardless of loading state. The EmptyBox fallback handles the no-data case appropriately.Also applies to: 222-293
296-317: LGTM - clean refactor for dev mode.The refactor correctly consumes
usePluginInfo()as an array and filters for loaded plugins, which is appropriate for development mode display.
358-363: LGTM - correct error handling.The conditional error detail assignment is correct—only plugins with
status === 'failed'should exposeerrorMessageanderrorCause.
418-426: LGTM - improved type safety.The type definitions now correctly reference
PluginInfoEntry['status']from the SDK, improving type safety and alignment with the external API.Also applies to: 438-441
338-343: Property access pattern verified as correct.The code correctly accesses
metadata.namefor loaded plugins andpluginNamefor not-loaded plugins. This pattern is consistently used across multiple detail components (ConsolePluginVersionDetail, ConsolePluginStatusDetail, ConsolePluginEnabledStatusDetail, ConsolePluginDescriptionDetail) and aligns with the PluginInfoEntry type from @openshift/dynamic-plugin-sdk, which returns different object shapes based on plugin status.
2-2: Verify lowercase status values are supported by @openshift/dynamic-plugin-sdk v5.0.1.The code consistently uses lowercase status values (
'failed','loaded','pending'), but the SDK type definitions could not be definitively verified through web search. Before merging, please confirm thatPluginInfoEntry['status']in @openshift/dynamic-plugin-sdk v5.0.1 supports these lowercase literals by checking the package's TypeScript definitions (typically in the npm package or GitHub repository at https://github.com/openshift/dynamic-plugin-sdk).
logonoff
force-pushed
the
CONSOLE-3769-s2e22-casino-night
branch
from
4304f21 to
84d7348
Compare
to align with openshift/dynamic-plugin-sdk
because it is not in the SDK
because we know it will never be null
logonoff
force-pushed
the
CONSOLE-3769-s2e22-casino-night
branch
from
84d7348 to
a53ba4e
Compare
|
Local build and testing shows there are no issues /verified by @logonoff |
openshift-ci-robot
added
the
verified
|
@logonoff: 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. |
|
/retest |
|
/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 |
|
@logonoff: 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. |
4 participants
DynamicPluginInfo.statuschanged to lowercase to align with SDKPluginInfoEntry.statusallPluginsProcessedremoved fromusePluginInfoto align with SDKusePluginInfoPluginStoreisLoadedDynamicPluginInfoandisNotLoadedDynamicPluginInforemoved to decrease dependence on@console/plugin-sdk