CONSOLE-3769: Phase 2 of using OpenShift Dynamic Plugin SDK #15904
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
jira/valid-reference
|
@logonoff: This pull request references CONSOLE-3705 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.22." or "openshift-4.22.", but it targets "openshift-4.15" instead. 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. |
|
Skipping CI for Draft Pull Request. |
WalkthroughReplaces legacy Console plugin runtime and store with SDK-managed PluginStore and hooks; migrates consumers from metadata/pluginID to manifest/uid; introduces Jest Webpack mock; adds resource-sidebar samples hook; removes many legacy runtime modules and tests; updates numerous callers and test providers. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
openshift-ci
bot
added
component/sdk
logonoff
changed the title
|
@logonoff: This pull request references CONSOLE-3769 which is a valid jira issue. 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. |
|
@logonoff: This pull request references CONSOLE-3769 which is a valid jira issue. 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. |
|
@logonoff: This pull request references CONSOLE-3769 which is a valid jira issue. 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. |
|
@logonoff: This pull request references CONSOLE-3769 which is a valid jira issue. 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. |
|
@logonoff: This pull request references CONSOLE-3769 which is a valid jira issue. 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. |
logonoff
force-pushed
the
CONSOLE-3769-s3e3-the-coup
branch
5 times, most recently
from
ef41480 to
ffdfee4
Compare
openshift-ci
bot
added
component/dashboard
logonoff
force-pushed
the
CONSOLE-3769-s3e3-the-coup
branch
6 times, most recently
from
a6feff7 to
005e7c3
Compare
logonoff
force-pushed
the
CONSOLE-3769-s3e3-the-coup
branch
2 times, most recently
from
778744c to
250803e
Compare
|
@logonoff: This pull request references CONSOLE-3769 which is a valid jira issue. 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/packages/operator-lifecycle-manager/src/components/operand/__tests__/index.spec.tsx (1)
226-234: Validate both DetailsPage calls for consistency with codebase patterns.The adjustment from 1 to 2 calls aligns with the SDK migration where PluginStore initialization triggers an additional render cycle—good catch with the explanatory comment.
However, the test validates props only from
mock.calls[0]. Across the codebase (particularly infirehose.spec.tsx), the established pattern when expecting multiple mock calls is to validate both calls to ensure consistency. Even if both renders produce identical props, explicitly asserting this (either on both calls or with a comment) improves test maintainability and future-proofs against regressions.Consider validating
mock.calls[1]or adding a comment explaining why only the first call is validated.frontend/packages/console-plugin-sdk/src/codegen/local-plugins.ts (1)
175-196: Import path references a non-existent symbol in the SDK.The code imports
applyCodeRefSymbolfrom@openshift/dynamic-plugin-sdk, but this symbol is not exported from any published version of the SDK. The installed version is 5.0.1 (via yarn.lock), and the symbol does not exist in that release. This import will fail at runtime. Either:
- The symbol needs to be exported from a new SDK release, or
- The symbol should continue to be imported from the internal path (
@console/dynamic-plugin-sdk/src/coderefs/coderef-resolver), or- A different public SDK API should be used if one exists
Verify with the SDK maintainers what the correct public API is for code reference symbol application before merging.
🤖 Fix all issues with AI agents
In
@frontend/packages/console-app/src/components/flags/FeatureFlagExtensionLoader.tsx:
- Around line 14-18: Replace the internal import of ResolvedExtension with the
public SDK barrel: keep importing Extension and ExtensionTypeGuard from the
internal path currently used, but change the import for ResolvedExtension to
come from '@console/dynamic-plugin-sdk' instead of
'@console/dynamic-plugin-sdk/src/types' so the file uses the public export for
ResolvedExtension while retaining the internal imports for Extension and
ExtensionTypeGuard.
In @frontend/packages/console-dynamic-plugin-sdk/src/app/AppInitSDK.tsx:
- Line 14: Update the JSDoc example in AppInitSDK to show the new dynamicPlugins
configuration instead of initPlugins: modify the example block (currently lines
~25–34) to include configurations={{ appFetch, apiDiscovery, dynamicPlugins: ()
=> { /* register plugins */ } }} so consumers see optional plugin initialization
using the new signature () => void; also update the example text to call out the
signature change from (store: Store<any>) => void to () => void so migration is
clear.
In @frontend/packages/console-shared/locales/en/console-shared.json:
- Line 298: Non-English locale files still use the misspelled key "Set
maxUnavaliable to 0" while the code calls t('Set maxUnavailable to 0') and the
English locale was corrected; update the translation key in the Korean,
Japanese, Chinese, French, and Spanish locale JSONs to "Set maxUnavailable to 0"
(preserving the existing translated values) so they match the call in
useResourceSidebarSamples.ts (referenced at line ~326).
In @frontend/packages/console-shared/src/hooks/useResourceSidebarSamples.ts:
- Around line 257-270: The lazyYaml function assumes extension.properties.label
and extension.properties.description are strings and calls split on them, which
can throw if they are undefined or non-string; update lazyYaml (used with
addActions) to defensively coerce or default these values (e.g., const label =
String(extension.properties.label || '') and const description =
String(extension.properties.description || '')) before calling split/join, and
ensure the generated labelComment/descriptionComment handle empty strings
gracefully so no runtime error occurs when properties are missing or
non-strings.
- Around line 345-378: The type cast inside useResourceSidebarSamples that
builds extensionSamples incorrectly uses Exclude<Sample, 'id'>; change that to
Omit<Sample, 'id'> to remove the id property from the Sample object type when
spreading sample.spec; update the mapping that constructs each extension sample
(where id: sample.metadata.uid and ...(sample.spec as ...)) to use Omit<Sample,
'id'> so TypeScript treats sample.spec as the Sample without id, keeping
useDefaultSamples, useResourceSidebarSamples, extensionSamples and allSamples
logic unchanged.
In @frontend/public/components/app.tsx:
- Line 12: The import of PluginStoreProvider is using the wrong package scope;
replace the import source for PluginStoreProvider from
'@openshift/dynamic-plugin-sdk' to the Console-internal package (e.g.,
'@openshift-console/dynamic-plugin-sdk-internal') or the correct Console SDK
package you confirm, update the import line that references PluginStoreProvider
in app.tsx, and ensure the chosen package is added to package.json/dependencies
so the module resolves correctly.
🧹 Nitpick comments (11)
frontend/packages/console-app/src/__tests__/hooks/useCSPViolationDetector.spec.tsx (1)
27-29: Mock migration is correct, but consider the defensive pattern for consistency.The partial mock at lines 27-29 is actually sufficient—the hook only imports
usePluginStorefrom@openshift/dynamic-plugin-sdk, so the current strategy won't cause runtime issues. However, other mocks in this file (like the@console/shared/src/constants/commonmock) already usejest.requireActualfor defensive consistency. Consider aligning with that pattern to safeguard against future SDK exports if the hook evolves:♻️ Defensive mock pattern for consistency
-jest.mock('@openshift/dynamic-plugin-sdk', () => ({ - usePluginStore: () => mockPluginStore, -})); +jest.mock('@openshift/dynamic-plugin-sdk', () => ({ + ...jest.requireActual('@openshift/dynamic-plugin-sdk'), + usePluginStore: () => mockPluginStore, +}));frontend/packages/console-dynamic-plugin-sdk/CHANGELOG-core.md (1)
21-21: Consider adding migration context to the breaking change entry.The entry correctly documents the removal of
pluginIDfromuseResolvedExtensions, but plugin developers would benefit from a brief hint about the replacement approach (e.g., "use manifest-based identifiers instead") directly in the changelog. While upgrade-sdk.md is referenced at the top of the section, a one-line migration hint here would improve discoverability.📝 Suggested enhancement
-- **Breaking**: Removed `pluginID` from the result in `useResolvedExtensions` hook ([CONSOLE-3769], [#15904]) +- **Breaking**: Removed `pluginID` from the result in `useResolvedExtensions` hook; use manifest-based identifiers instead ([CONSOLE-3769], [#15904])frontend/packages/console-plugin-sdk/src/api/usePluginInfo.ts (2)
30-49: Side effect insideuseMemomay cause issues in concurrent React.Mutating
previousResultRef.current(line 47) inside theuseMemocallback is a side effect during the render phase. While React 17.0.1 tolerates this, it can cause issues if:
- The codebase upgrades to React 18+ with concurrent features.
- Strict mode double-renders (which already exists in dev mode).
Consider moving the ref assignment outside
useMemoto keep the memoization pure:♻️ Suggested refactor
export const usePluginInfo = () => { const pluginInfo = usePluginInfoSDK(); const previousResultRef = useRef<PluginInfoEntry[]>([]); - // This hook returns dynamic plugin information only, i.e., not static/local plugins - return useMemo(() => { + const stablePluginInfo = useMemo(() => { const dynamicPluginInfo = pluginInfo.filter( (plugin) => plugin.manifest.registrationMethod !== 'local', ); // Ensure referential stability of the result elements - const stablePluginInfo = dynamicPluginInfo.map((plugin) => { + return dynamicPluginInfo.map((plugin) => { const previousPlugin = previousResultRef.current.find( (p) => p.manifest.name === plugin.manifest.name, ); // Only reuse previous reference if the data is actually equal if (previousPlugin && isEqual(previousPlugin, plugin)) { return previousPlugin; } return plugin; }); - - previousResultRef.current = stablePluginInfo; - return stablePluginInfo; }, [pluginInfo]); + + previousResultRef.current = stablePluginInfo; + return stablePluginInfo; };
36-45: ConsiderMapfor O(1) lookup (optional optimization).The
findinsidemapcreates O(n²) complexity. With typical plugin counts this is negligible, but if you want O(n), aMapkeyed bymanifest.namewould work:const previousMap = new Map( previousResultRef.current.map((p) => [p.manifest.name, p]) );That said, plugin counts are low enough that this is a micro-optimization—acceptable as-is.
frontend/packages/console-plugin-sdk/src/api/useExtensions.ts (1)
76-94: Mutating refs insideuseMemois fragile — consider restructuring.React's
useMemoshould be side-effect–free. MutatingpreviousResultRefandpreviousUIDsRefinside the memo callback can cause subtle bugs, especially if React discards and re-executes the memoized function (possible in StrictMode or concurrent features). While React 17 doesn't enable concurrent rendering by default, an eventual upgrade could expose this.A safer pattern keeps the comparison/caching outside
useMemo:♻️ Suggested refactor for side-effect–free memoization
- // Track the previous result and UIDs for referential stability - const previousResultRef = useRef<LoadedExtension<E>[]>([]); - const previousUIDsRef = useRef<string>(''); + const previousResultRef = useRef<LoadedExtension<E>[] | null>(null); - return useMemo(() => { - const sorted = sortExtensionsByPluginOrder(translatedExtensions); - const currentUIDs = sorted.map((e) => e.uid).join(','); + const sorted = useMemo( + () => sortExtensionsByPluginOrder(translatedExtensions), + [translatedExtensions], + ); - // Return previous result if the extensions haven't changed - if (currentUIDs === previousUIDsRef.current) { - return previousResultRef.current; - } + const currentUIDs = useMemo(() => sorted.map((e) => e.uid).join(','), [sorted]); - // Update refs and return new result - previousResultRef.current = sorted; - previousUIDsRef.current = currentUIDs; + // Use effect-like pattern to update cache after render commits + const previousUIDsRef = useRef<string>(''); + if (currentUIDs !== previousUIDsRef.current) { + previousResultRef.current = sorted; + previousUIDsRef.current = currentUIDs; + } - return sorted; - }, [translatedExtensions]); + return previousResultRef.current ?? sorted;Alternatively, you could use a library like
use-deep-compare-memoizeor compare UIDs in auseEffectto update the cached result.frontend/packages/console-dynamic-plugin-sdk/src/coderefs/__tests__/coderef-resolver.spec.ts (1)
145-171: Test no longer validates its intended behavior.The extensive comments acknowledge this test doesn't actually exercise the error rejection path due to Jest 30's symbol interference. The assertion
expect(errorCodeRef).not.toHaveBeenCalled()confirms the CodeRef is never invoked — so the "continuously reject" scenario isn't tested at all.Consider refactoring to use a custom rejecting CodeRef that passes
isExecutableCodeRef:♻️ Suggested approach using getExecutableCodeRefMock pattern
Create a variant that rejects instead of resolves. For example, add a helper in
test-utils.ts:export const getRejectingCodeRefMock = <T = unknown>( error: Error, ): jest.Mock<Promise<T>, []> => { const calls: [][] = []; const fn = function () { calls.push([]); return Promise.reject(error); }; applyCodeRefSymbol(fn); Object.defineProperty(fn, 'mock', { value: { calls }, enumerable: false, configurable: true, }); (fn as any).getMockName = () => 'rejectingCodeRefMock'; return (fn as unknown) as jest.Mock<Promise<T>, []>; };Then use it here to properly test error persistence across multiple resolution attempts.
frontend/packages/console-plugin-sdk/src/codegen/local-plugins.ts (1)
45-73: New plugin object shape aligns with SDK'sLocalPluginManifest.The addition of
versionandregistrationMethod: 'local'fields is consistent with upstream naming conventions. One minor consideration:pkg.versionis directly interpolated into a JS string literal. If a package version ever contains single quotes or backslashes (non-standard but possible in some edge cases), this could break the generated code.Typically semver versions are safe, so this is low risk, but if you want belt-and-suspenders:
💡 Optional: escape version string
localPlugins.push({ name: '${pkg.name}', - version: '${pkg.version}', + version: '${pkg.version.replace(/\\/g, '\\\\').replace(/'/g, "\\'")}', extensions: ${extensionHook(pkg)}, registrationMethod: 'local', });frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-init.ts (1)
72-95: Legacy callback shim looks correct for backward compatibility.The version-stripping logic using
lastIndexOf('@')handles edge cases where@might appear in plugin names (e.g., scoped packages). The deprecation warning is helpful for plugin authors to migrate.Minor observation: the
console.infoat line 92-94 will always fire during initialization regardless of whether any legacy plugins are actually loaded. Consider whether this log is necessary in production or if it should be gated behind a dev-only check for cleaner production logs.frontend/packages/console-shared/src/hooks/useResourceSidebarSamples.ts (1)
93-343: Large hook with potential performance implications.
useDefaultSamplescalls threeuseResolvedExtensionshooks (lines 95-97), which will cause re-renders when any extension set changes. The ImmutableMap is rebuilt on every render since it's not memoized.Consider wrapping the ImmutableMap construction in
useMemowith dependencies on[addActions, catalogItemTypes, perspectives, clusterRoleBindingSamples]to prevent unnecessary object recreation:♻️ Suggested memoization
const useDefaultSamples = () => { const { t } = useTranslation('console-shared'); const [addActions] = useResolvedExtensions<AddAction>(isAddAction); const [catalogItemTypes] = useResolvedExtensions<CatalogItemType>(isCatalogItemType); const [perspectives] = useResolvedExtensions<Perspective>(isPerspective); const clusterRoleBindingSamples = useClusterRoleBindingSamples(); - return ImmutableMap<GroupVersionKind, Sample[]>() + return React.useMemo(() => ImmutableMap<GroupVersionKind, Sample[]>() .setIn( [referenceForModel(BuildConfigModel)], // ... rest of the chain - ); + ), [t, addActions, catalogItemTypes, perspectives, clusterRoleBindingSamples]); };frontend/packages/console-shared/src/components/formik-fields/CodeEditorField.tsx (1)
46-58: Minor: Hook called regardless ofshowSamplesprop.The
useResourceSidebarSampleshook is always invoked even whenshowSamplesisfalse. While this follows Rules of Hooks correctly, it means unnecessary computation and extension resolution when samples aren't needed.This is likely acceptable given the hook's usage pattern, but if performance becomes a concern, consider having the hook accept a
skipparameter similar to Apollo's pattern.frontend/packages/console-dynamic-plugin-sdk/src/app/AppInitSDK.tsx (1)
18-34: Documentation example could includedynamicPlugins.The JSDoc example only shows
appFetchandapiDiscoveryin the configurations prop. Consider updating it to also illustrate the newdynamicPluginsoption for completeness, especially since this is an SDK-facing component.📝 Suggested documentation update
* @example * ```ts * return ( * <Provider store={store}> - * <AppInitSDK configurations={{ appFetch, apiDiscovery }}> + * <AppInitSDK configurations={{ appFetch, apiDiscovery, dynamicPlugins: initDynamicPlugins }}> * <CustomApp /> * ... * </AppInitSDK> * </Provider> * ) * ```
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
frontend/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (68)
frontend/__mocks__/webpack.tsfrontend/package.jsonfrontend/packages/console-app/src/__tests__/hooks/useCSPViolationDetector.spec.tsxfrontend/packages/console-app/src/__tests__/plugin-test-utils.tsfrontend/packages/console-app/src/components/dashboards-page/dynamic-plugins-health-resource/NotLoadedDynamicPlugins.tsxfrontend/packages/console-app/src/components/file-upload/__tests__/file-upload-utils.spec.tsfrontend/packages/console-app/src/components/flags/FeatureFlagExtensionLoader.tsxfrontend/packages/console-app/src/components/nodes/useNodeStatusExtensions.tsxfrontend/packages/console-app/src/components/user-preferences/__tests__/userPreferences.data.tsxfrontend/packages/console-app/src/components/user-preferences/perspective/__tests__/perspective.data.tsfrontend/packages/console-app/src/hooks/useCSPViolationDetector.tsxfrontend/packages/console-dynamic-plugin-sdk/CHANGELOG-core.mdfrontend/packages/console-dynamic-plugin-sdk/CHANGELOG-webpack.mdfrontend/packages/console-dynamic-plugin-sdk/README.mdfrontend/packages/console-dynamic-plugin-sdk/package.jsonfrontend/packages/console-dynamic-plugin-sdk/scripts/package-definitions.tsfrontend/packages/console-dynamic-plugin-sdk/src/app/AppInitSDK.tsxfrontend/packages/console-dynamic-plugin-sdk/src/coderefs/__tests__/coderef-resolver.spec.tsfrontend/packages/console-dynamic-plugin-sdk/src/coderefs/coderef-resolver.tsfrontend/packages/console-dynamic-plugin-sdk/src/perspective/__tests__/perspective.data.tsfrontend/packages/console-dynamic-plugin-sdk/src/runtime/__tests__/plugin-manifest.spec.tsfrontend/packages/console-dynamic-plugin-sdk/src/runtime/__tests__/plugin-utils.spec.tsfrontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-dependencies.tsfrontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-init.tsfrontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-loader.tsfrontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-manifest.tsfrontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-utils.tsfrontend/packages/console-dynamic-plugin-sdk/src/shared-modules/shared-modules-meta.tsfrontend/packages/console-dynamic-plugin-sdk/src/types.tsfrontend/packages/console-dynamic-plugin-sdk/src/utils/test-utils.tsfrontend/packages/console-dynamic-plugin-sdk/src/webpack/ConsoleRemotePlugin.tsfrontend/packages/console-plugin-sdk/package.jsonfrontend/packages/console-plugin-sdk/src/api/pluginSubscriptionService.tsfrontend/packages/console-plugin-sdk/src/api/useExtensions.tsfrontend/packages/console-plugin-sdk/src/api/usePluginInfo.tsfrontend/packages/console-plugin-sdk/src/api/usePluginStore.tsfrontend/packages/console-plugin-sdk/src/codegen/local-plugins.tsfrontend/packages/console-plugin-sdk/src/store.tsfrontend/packages/console-plugin-sdk/src/utils/allowed-plugins.tsfrontend/packages/console-shared/locales/en/console-shared.jsonfrontend/packages/console-shared/src/components/actions/__tests__/utils-test-data.tsfrontend/packages/console-shared/src/components/catalog/__tests__/CatalogController.spec.tsxfrontend/packages/console-shared/src/components/editor/CodeEditorSidebar.tsxfrontend/packages/console-shared/src/components/formik-fields/CodeEditorField.tsxfrontend/packages/console-shared/src/hooks/__tests__/useTelemetry.spec.tsfrontend/packages/console-shared/src/hooks/useResourceSidebarSamples.tsfrontend/packages/console-shared/src/test-utils/unit-test-utils.tsxfrontend/packages/console-shared/src/utils/index.tsfrontend/packages/console-shared/src/utils/sample-utils.tsfrontend/packages/dev-console/src/components/add/__tests__/add-page-test-data.tsxfrontend/packages/dev-console/src/components/deployments/__tests__/DeploymentForm.spec.tsxfrontend/packages/dev-console/src/utils/__tests__/useAddActionExtensions.spec.tsfrontend/packages/knative-plugin/src/components/add/__tests__/EventSource.spec.tsxfrontend/packages/operator-lifecycle-manager/src/components/operand/__tests__/index.spec.tsxfrontend/packages/topology/src/__tests__/TopologyPage.spec.tsxfrontend/packages/topology/src/data-transforms/DataModelExtension.tsxfrontend/packages/topology/src/data-transforms/DataModelProvider.tsxfrontend/packages/webterminal-plugin/src/components/cloud-shell/__tests__/MultiTabbedTerminal.spec.tsxfrontend/public/actions/features.tsfrontend/public/components/app.tsxfrontend/public/components/dashboard/project-dashboard/getting-started/__tests__/SampleGettingStartedCard.data.tsfrontend/public/components/edit-yaml.tsxfrontend/public/components/factory/modal.tsxfrontend/public/components/notification-drawer.tsxfrontend/public/components/sidebars/resource-sidebar-samples.tsxfrontend/public/components/sidebars/resource-sidebar.tsxfrontend/public/components/utils/__tests__/nav.spec.tsfrontend/public/plugins.ts
💤 Files with no reviewable changes (25)
- frontend/packages/console-app/src/components/file-upload/tests/file-upload-utils.spec.ts
- frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-dependencies.ts
- frontend/packages/console-shared/src/components/actions/tests/utils-test-data.ts
- frontend/packages/console-dynamic-plugin-sdk/src/webpack/ConsoleRemotePlugin.ts
- frontend/packages/dev-console/src/utils/tests/useAddActionExtensions.spec.ts
- frontend/packages/console-app/src/components/user-preferences/perspective/tests/perspective.data.ts
- frontend/packages/console-shared/src/components/catalog/tests/CatalogController.spec.tsx
- frontend/packages/console-plugin-sdk/src/api/pluginSubscriptionService.ts
- frontend/packages/console-dynamic-plugin-sdk/src/runtime/tests/plugin-manifest.spec.ts
- frontend/public/components/utils/tests/nav.spec.ts
- frontend/packages/console-shared/src/utils/sample-utils.ts
- frontend/public/components/dashboard/project-dashboard/getting-started/tests/SampleGettingStartedCard.data.ts
- frontend/packages/dev-console/src/components/add/tests/add-page-test-data.tsx
- frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-loader.ts
- frontend/packages/console-plugin-sdk/src/api/usePluginStore.ts
- frontend/packages/console-shared/src/utils/index.ts
- frontend/packages/console-shared/src/hooks/tests/useTelemetry.spec.ts
- frontend/packages/console-dynamic-plugin-sdk/src/perspective/tests/perspective.data.ts
- frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-manifest.ts
- frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-utils.ts
- frontend/packages/console-plugin-sdk/src/store.ts
- frontend/packages/console-dynamic-plugin-sdk/package.json
- frontend/packages/console-app/src/components/user-preferences/tests/userPreferences.data.tsx
- frontend/packages/console-dynamic-plugin-sdk/src/runtime/tests/plugin-utils.spec.ts
- frontend/packages/console-plugin-sdk/package.json
🚧 Files skipped from review as they are similar to previous changes (14)
- frontend/packages/console-dynamic-plugin-sdk/README.md
- frontend/packages/console-plugin-sdk/src/utils/allowed-plugins.ts
- frontend/packages/console-app/src/tests/plugin-test-utils.ts
- frontend/packages/console-shared/src/components/editor/CodeEditorSidebar.tsx
- frontend/public/components/sidebars/resource-sidebar.tsx
- frontend/packages/console-app/src/components/dashboards-page/dynamic-plugins-health-resource/NotLoadedDynamicPlugins.tsx
- frontend/packages/console-app/src/hooks/useCSPViolationDetector.tsx
- frontend/packages/topology/src/tests/TopologyPage.spec.tsx
- frontend/public/components/notification-drawer.tsx
- frontend/package.json
- frontend/packages/topology/src/data-transforms/DataModelExtension.tsx
- frontend/public/components/factory/modal.tsx
- frontend/packages/console-dynamic-plugin-sdk/src/coderefs/coderef-resolver.ts
- frontend/public/components/sidebars/resource-sidebar-samples.tsx
🧰 Additional context used
🧬 Code graph analysis (11)
frontend/packages/webterminal-plugin/src/components/cloud-shell/__tests__/MultiTabbedTerminal.spec.tsx (1)
frontend/packages/console-shared/src/test-utils/unit-test-utils.tsx (1)
renderWithProviders(45-64)
frontend/packages/knative-plugin/src/components/add/__tests__/EventSource.spec.tsx (1)
frontend/packages/console-shared/src/test-utils/unit-test-utils.tsx (1)
renderWithProviders(45-64)
frontend/packages/console-dynamic-plugin-sdk/src/coderefs/__tests__/coderef-resolver.spec.ts (1)
frontend/packages/console-dynamic-plugin-sdk/src/coderefs/coderef-resolver.ts (1)
resolveExtension(62-99)
frontend/packages/console-shared/src/test-utils/unit-test-utils.tsx (2)
frontend/public/plugins.ts (1)
pluginStore(20-34)frontend/public/components/utils/firehose.jsx (1)
children(265-265)
frontend/packages/dev-console/src/components/deployments/__tests__/DeploymentForm.spec.tsx (1)
frontend/packages/console-shared/src/test-utils/unit-test-utils.tsx (1)
renderWithProviders(45-64)
frontend/packages/console-dynamic-plugin-sdk/src/app/AppInitSDK.tsx (1)
frontend/packages/console-dynamic-plugin-sdk/src/app/k8s/api-discovery/api-discovery.ts (1)
initApiDiscovery(149-159)
frontend/public/plugins.ts (4)
frontend/public/module/auth.js (1)
window(5-12)frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-shared-modules.ts (1)
getSharedScope(18-24)frontend/get-local-plugins.js (1)
require(3-3)frontend/public/redux.ts (1)
RootState(20-28)
frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-init.ts (4)
frontend/public/plugins.ts (1)
pluginStore(20-34)frontend/packages/console-dynamic-plugin-sdk/src/build-types.ts (1)
ConsolePluginManifest(46-52)frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-shared-modules.ts (2)
initSharedScope(13-13)getSharedScope(18-24)frontend/packages/console-plugin-sdk/src/utils/allowed-plugins.ts (1)
dynamicPluginNames(25-25)
frontend/public/components/app.tsx (4)
frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-init.ts (1)
initConsolePlugins(102-126)frontend/public/plugins.ts (1)
pluginStore(20-34)frontend/public/co-fetch.ts (1)
appInternalFetch(129-155)frontend/packages/console-dynamic-plugin-sdk/src/app/k8s/api-discovery/api-discovery.ts (1)
initApiDiscovery(149-159)
frontend/packages/console-shared/src/components/formik-fields/CodeEditorField.tsx (1)
frontend/packages/console-shared/src/hooks/useResourceSidebarSamples.ts (1)
useResourceSidebarSamples(345-378)
frontend/packages/console-plugin-sdk/src/codegen/local-plugins.ts (2)
frontend/packages/console-plugin-sdk/src/codegen/plugin-resolver.ts (1)
PluginPackage(111-115)frontend/get-local-plugins.js (1)
getLocalPluginsModuleData(18-22)
🔇 Additional comments (68)
frontend/packages/console-shared/locales/en/console-shared.json (1)
302-314: LGTM — key relocation has no functional impact.JSON objects are inherently unordered, so moving these translation keys to a different position in the file is purely organizational. The keys and values remain unchanged.
frontend/packages/console-app/src/__tests__/hooks/useCSPViolationDetector.spec.tsx (3)
24-26: Mock plugin store structure is minimal but adequate for current tests.The
mockPluginStoreonly implementsgetPluginInforeturning an empty array. This is fine for the current test scope (CSP event handling), but if future tests need to verify plugin-specific violation attribution, you'd need to populate this mock with actual plugin data matching the newmanifest.namestructure referenced in the PR summary.
107-123: Plugin name parsing tests are solid.Good coverage of the plugin name extraction from both
blockedURIandsourceFilepaths. The/api/plugins/foopattern correctly exercises the parsing logic. These tests will help ensure backward compatibility as the codebase migrates frommetadata.nametomanifest.namefor plugin identification.
71-105: Core violation detection tests look good.Proper use of
act()for event dispatch, clear separation between "new violation" and "duplicate violation" scenarios, and appropriate assertions on both cache and telemetry interactions. TheafterEachcleanup is correct.frontend/packages/console-dynamic-plugin-sdk/CHANGELOG-core.md (1)
234-234: LGTM!Standard reference link addition, properly formatted and consistent with the changelog conventions.
frontend/public/actions/features.ts (4)
19-21: Defaults initialization looks correct.The
defaultsobject correctly initializesFLAGS.AUTH_ENABLEDbased onSERVER_FLAGS.authDisabledand leaves other flags asundefinedfor lazy detection. This pattern is unchanged and appropriate.
66-95: User detection with SSR query is well-structured.The
detectUserfunction properly:
- Handles the
extrafield JSON parsing with try-catch (good defensive coding)- Constructs
UserInfoincrementally with proper type- Uses consistent retry logic via
retryFlagDetectionOne minor observation: the retry condition excludes status codes 400, 404, 500 using
_.includes([400, 404, 500], ...). This is consistent withdetectCanCreateProjectand intentional (don't retry on client errors or server errors that won't self-resolve).
117-125: Core feature detection dispatch is clean and unchanged.The
detectFeaturesthunk dispatches all detection functions in parallel. This is efficient since these are independent checks. The spread ofssarCheckActionscorrectly includes all SSAR-based flag checks.With the extension-driven feature flag handling now delegated to
FeatureFlagExtensionLoader, this function remains focused on its core responsibility—detecting built-in platform capabilities.
5-5: Clean removal of extension-driven feature flag wiring—properly centralized toFeatureFlagExtensionLoader.The import changes align with the PR's architectural goal. The
FeatureFlagExtensionLoadercomponent now owns all extension subscription logic: it processes bothFeatureFlaghook providers andModelFeatureFlagextensions viauseResolvedExtensionsDiff, then dispatchesUpdateModelFlagsto the reducer. This leavesfeatures.tsfocused solely on core feature detection (SSAR/SSR checks, OpenShift/ClusterVersion detection, User/Project capability discovery).All imports active in this file—
UserInfois used indetectUser, and all symbols from./flags(handleError,retryFlagDetection,setFlag,ssarChecks) are actively consumed.subscribeToExtensionshas been cleanly removed across the codebase. The separation of concerns is sound: core detectors remain here, extension-driven side effects are handled by the SDK-managed loader mounted inapp.tsx.frontend/packages/console-plugin-sdk/src/api/usePluginInfo.ts (2)
24-26: Clean migration to SDK-managed plugin info.Good approach wrapping
usePluginInfoSDKand filtering to dynamic-only plugins. This aligns well with the PR objective of migrating tolib-corefrom the OpenShift Dynamic Plugin SDK while maintaining Console's abstraction layer. The referential stability guarantee is a nice touch for downstream memoization.
3-3: No changes needed.The import from
lodashis consistent with the codebase convention. Whilelodash-esis available, the Console codebase predominantly useslodashacross all packages (100+ occurrences). The single use oflodash-es/uniqByin knative-plugin is an exception, not the standard. This import aligns with the established pattern and does not prevent tree-shaking at the build level.Likely an incorrect or invalid review comment.
frontend/packages/console-app/src/components/nodes/useNodeStatusExtensions.tsx (1)
48-65: LGTM — migration frompluginIDtouidis correct and consistent.The change properly uses
uidandtypefrom the resolved extension structure. These are standard properties onResolvedExtension<NodeStatus>from the SDK, and the destructuring at line 48 is correct. The error message at line 65 remains informative by including both identifiers, which aids debugging when extensions fail.The usage pattern is consistent throughout the hook (lines 31, 34, 52, 59, 65), confirming this aligns with the broader PR goal of removing version strings from plugin identifiers.
frontend/packages/console-plugin-sdk/src/api/useExtensions.ts (3)
1-9: LGTM — clean import refactoring.Good call aliasing the SDK's
useExtensionstouseExtensionsSDKto avoid collision with the exported hook. Import structure is clean.
24-32: Sorting logic is correct and well-documented.Good use of a non-mutating sort via spread. Using
MIN_SAFE_INTEGERas the fallback ensures static plugins (those not indynamicPluginNames) sort to the front, which matches the JSDoc intent. The OCPBUGS-43792 reference provides good traceability for why this ordering exists.
11-12: Clarify the runtime behavior for operator config changes affecting plugin order.
pluginOrderMapis created once at module load fromdynamicPluginNames. The latter is computed fromwindow.SERVER_FLAGS.consolePluginsand URL params, but only evaluated a single time. If the console operator config changes post-load (e.g., an operator update resetsSERVER_FLAGS.consolePlugins), the sort order won't reflect those changes without a page reload. Per the OCPBUGS-43792 comment, this is explicitly designed to allow admins to control extension priority via operator config—confirm whether a page reload is the accepted model for propagating config changes, or if there's a mechanism to refreshdynamicPluginNamesat runtime that isn't immediately visible here.frontend/packages/console-app/src/components/flags/FeatureFlagExtensionLoader.tsx (5)
37-47: LGTM — intentional render-time flush pattern.The dependency-less
useLayoutEffectintentionally fires after every render to drain the pending update queue synchronously before paint. The earlysize === 0bailout keeps this cheap when idle, and checkingflags.get(flag)prevents redundant dispatches. Clean solution for the react-redux 8.x "dispatch during render" constraint.
81-97: LGTM — solid error isolation.Good defensive pattern: wrapping each handler invocation in try/catch ensures a misbehaving plugin can't cascade failures to others. Logging includes
pluginNamefor debuggability.Intentionally ignoring
removedextensions makes sense here—feature flag handlers are fire-once side effects with no teardown semantic.
103-126: LGTM — clean ref pattern for stable callback identity.Using
modelsRefto capture the latest models without destabilizinghandleChange's identity is the right approach here. This ensuresuseResolvedExtensionsDiffdoesn't re-run its effect unnecessarily while still dispatching with current model state.Passing both
addedandremovedtoupdateModelFlagscorrectly supports the full lifecycle of model-based feature flags.
132-140: LGTM — clean hook composition.All hooks are called unconditionally at the top of the component before any conditional returns, satisfying the Rules of Hooks. The integration of
useFeatureFlagExtensionsanduseModelFeatureFlagExtensionskeeps the component focused on orchestration while delegating extension processing to dedicated hooks.
66-73: SDK extension reference stability is properly handled.
useExtensionsguarantees referential stability across re-renders by memoizing results based on extension UIDs (not instances). When extensions haven't changed by UID, the same array reference is returned, solodash.differencecorrectly identifies only actual additions/removals rather than false positives from instance churn.The dependency chain works:
useExtensions(referentially stable) →useResolvedExtensions(depends on stable extensions) →useResolvedExtensionsDiff(difference comparison). No action needed.frontend/packages/console-dynamic-plugin-sdk/src/utils/test-utils.ts (2)
27-51: Well-documented Jest 30 workaround.The comment block explaining why manual call tracking is used instead of
jest.fn()is valuable for future maintainers. The workaround correctly preserves theisExecutableCodeRefcheck behavior while still providing mock-like assertion capabilities.
1-1: Straightforward use of SDK'sapplyCodeRefSymbolto construct test fixtures.The code correctly imports
applyCodeRefSymbolfrom@openshift/dynamic-plugin-sdk(v6.0.0) and applies it at line 38 to create executable code ref mocks compatible withisExecutableCodeRefchecks. The comments explaining the Jest 30 workaround—avoiding built-in mock functions' internal symbols that would break theisExecutableCodeRefcheck—are helpful for maintainability.One note: verify that
applyCodeRefSymbolis a stable, supported API on the SDK side if it's not documented as a public export. The codebase uses it successfully, but confirming long-term stability would strengthen confidence in this pattern.frontend/packages/console-dynamic-plugin-sdk/src/coderefs/__tests__/coderef-resolver.spec.ts (4)
1-10: Import updates align with SDK migration.Good consolidation:
applyCodeRefSymbolfrom the SDK public API,isCodeRefErrorfrom the local resolver, andgetExecutableCodeRefMockfrom shared test utils. This keeps test utilities DRY across the codebase.
129-143: Correctly validates new cloning semantics.The test name and expectations now match the production behavior in
resolveExtensionwhich uses_.cloneDeepand returns a new object (per the snippet at lines 67 and 93-96 ofcoderef-resolver.ts). Good coverage for both extensions with and without CodeRefs.
35-43: Good inline workaround with clear documentation.The comment at lines 38-40 explains the Jest 30 constraint. Using a plain arrow function here is the right call since this test only needs to verify
isExecutableCodeRefreturnstruefor symbol-marked functions.
59-93: Solid coverage for CodeRef resolution.Tests correctly use
getExecutableCodeRefMockto create properly-marked CodeRefs that passisExecutableCodeRef. The assertions verify both simple and nested property resolution, which aligns with thedeepForOwntraversal in production code.frontend/packages/console-plugin-sdk/src/codegen/local-plugins.ts (6)
1-17: LGTM on imports.Clean type-only import from the SDK. The rest of the internal imports remain correctly scoped.
22-40: Type rename is consistent.The
LocalPluginsModuleDatatype structure is unchanged from the previousActivePluginsModuleData, and the helper function's diagnostic parameter type is updated accordingly.
75-96: Test helper mirrors codegen output correctly.The
loadLocalPluginsForTestPurposesfunction produces the same object shape asgetLocalPluginsModule, maintaining the sync requirement documented in line 76. UsingLocalPluginManifestfrom the SDK as the return type ensures type-level consistency with the runtime.
98-129: No changes togetExecutableCodeRefSource.
136-173: Good addition for testability.The
codeRefSourceGetterparameter allows injecting custom code ref source generators, useful for testing without hitting the full validation/resolution logic. The default value preserves backward compatibility for existing callers.The parameter order (transformer before getter) maintains API stability for callers only passing
codeRefTransformer.
198-214: File dependency collection logic unchanged.The loop correctly validates exposed module paths and collects file dependencies. Error handling is appropriate.
frontend/packages/console-shared/src/test-utils/unit-test-utils.tsx (1)
2-2: LGTM! Clean integration of PluginStoreProvider into test utilities.The provider hierarchy (
Provider→PluginStoreProvider→MemoryRouter) is correct and mirrors the production app structure. Using the singletonpluginStorefrom@console/internal/pluginsensures test components have access to the same plugin context as the running application, which is essential for testing components that consume dynamic plugin extensions.Also applies to: 19-19, 57-59
frontend/packages/webterminal-plugin/src/components/cloud-shell/__tests__/MultiTabbedTerminal.spec.tsx (1)
1-1: LGTM! Correct migration to shared test utilities.The switch to
renderWithProvidersis consistent with the broader test infrastructure changes in this PR. Test assertions remain unchanged, confirming this is a pure refactor of the rendering setup without behavioral modifications.Also applies to: 29-30, 35-36, 46-47, 57-58
frontend/packages/knative-plugin/src/components/add/__tests__/EventSource.spec.tsx (1)
1-1: LGTM! Straightforward migration to renderWithProviders.The test now properly receives Redux store and plugin store context through the shared utility. The existing assertions against the mocked Formik component remain valid.
Also applies to: 30-37, 41-51
frontend/packages/dev-console/src/components/deployments/__tests__/DeploymentForm.spec.tsx (1)
2-2: LGTM! Clean test refactor using shared utilities.The migration removes boilerplate Redux Provider setup in favor of the centralized
renderWithProvidersutility. This improves test maintainability—any future changes to required providers (like adding new context providers) only need to happen in one place.The
beforeEach+afterEach(cleanup)pattern ensures proper test isolation.Also applies to: 6-6, 72-83
frontend/__mocks__/webpack.ts (1)
1-7: LGTM—essential mock for module federation runtime in Jest environment.This mock correctly stubs webpack's module federation globals (
__webpack_share_scopes__and__webpack_init_sharing__) that the Dynamic Plugin SDK'splugin-shared-modules.tsinvokes during plugin initialization. Without this, tests fail wheninitSharedScope()attempts to access the shared module scope at runtime.The mock is already properly registered in Jest's
setupFilesconfiguration, so the SDK's plugin loading pipeline will work correctly in tests.frontend/packages/console-dynamic-plugin-sdk/CHANGELOG-webpack.md (2)
15-19: LGTM! Clear deprecation notice with migration path.The changelog entries are well-structured: breaking changes are clearly marked, and the deprecation notice provides actionable guidance (build with ConsoleRemotePlugin 4.22+) along with a warning about future removal. This gives plugin authors sufficient notice and a clear migration path.
133-133: Link reference correctly added.frontend/packages/console-dynamic-plugin-sdk/src/shared-modules/shared-modules-meta.ts (2)
25-25: Correctly adds upstream SDK as a shared module.Adding
@openshift/dynamic-plugin-sdkto the shared modules list is essential for the SDK migration. The placement is alphabetically correct (before@openshift-console/*entries).
45-45: Metadata defaults are appropriate.Empty object yields
singleton: trueandallowFallback: false, which is correct for a core SDK module—ensures a single, Console-provided version is used at runtime without plugin fallbacks.frontend/packages/topology/src/data-transforms/DataModelProvider.tsx (3)
31-39: Type updated correctly for uid migration.The state type change from
pluginIDtouidaligns with the PR's migration to use the SDK'sLoadedExtensiontype, which usesuidas the identifier (no longer including version string).
57-78: Factory resolution correctly propagates uid.Both async and static resource resolution paths consistently use
factory.uid. The error message at line 64 helpfully includes both the factory ID and plugin uid for debugging.
92-98: DataModelExtension correctly accepts and uses theuidprop.The interface has been properly updated with
uid: string, and the prop is actively used for error reporting (line 69) and memoization dependency tracking (line 84). The refactor frompluginIDtouidis complete and consistent.frontend/packages/console-dynamic-plugin-sdk/src/types.ts (3)
1-8: Correctly imports LoadedExtension from upstream SDK.Importing
LoadedExtensiondirectly from@openshift/dynamic-plugin-sdkaligns with the migration strategy. This ensures the type definition (including theuidfield) comes from the authoritative source.
10-18: Re-export maintains API surface for consumers.Re-exporting
LoadedExtensionfrom this package allows existing plugin code to continue importing from@openshift-console/dynamic-plugin-sdkwithout changes. The type shape change (pluginID→uid) is a breaking change appropriately documented in the changelog.
62-65: ResolvedExtension type correctly composes with SDK LoadedExtension.The
ResolvedExtensiontype continues to work correctly—it wrapsLoadedExtensionand appliesMapCodeRefsToValuesto resolve CodeRef properties. Consumers usingResolvedExtensionwill now receive objects withuidinstead of the previouspluginIDfield.frontend/packages/console-dynamic-plugin-sdk/scripts/package-definitions.ts (1)
175-185: Explicit dependency list improves clarity and maintainability.The webpack package correctly extracts all dependencies from the root package at build time—
@openshift/dynamic-plugin-sdk,ajv,chalk,comment-json,find-up,glob,read-pkg,semver, andwebpack—rather than spreading from multiple sources. This explicit, consolidated approach is cleaner and ensures the webpack plugin bundle has access to all required build-time dependencies. The dependency chain is properly rooted inpackage.json(where@openshift/dynamic-plugin-sdk@^6.0.0is declared) and correctly propagated here.frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-init.ts (3)
18-57: Solid error handling pattern with status checking.The
loadAndEnablePluginfunction properly catches errors frompluginStore.loadPluginand performs post-load status verification. The logic to prevent dynamic plugins from disabling other dynamic plugins (line 50) is a good security measure.One consideration: the
await pluginStore.loadPlugin(...).catch(...)pattern means errors are caught but execution continues to the status check. IfloadPluginthrows,pluginwill likely beundefined, so the subsequent checks won't fire additional errors—this is fine but worth noting.
102-125: Initialization flow is well-structured but has a fire-and-forget async pattern.The
_.onceguard prevents double initialization. However, theinitSharedScope().then(...)chain is fire-and-forget—callers ofinitConsolePluginshave no way to know when plugins are fully loaded.For the current use case this appears acceptable since plugin loading is designed to be asynchronous and extensions become available reactively. The error accumulation in
window.windowError(line 113) integrates well with the existing GUI test error detection mechanism.Consider: if a downstream consumer ever needs to await plugin readiness, this API would need adjustment. For now, this is consistent with the existing architecture.
109-114: Parallel plugin loading without error isolation.
dynamicPluginNames.forEach(...)fires all plugin loads concurrently without awaiting them individually. This is good for performance but means:
- A slow or hanging plugin won't block others ✓
- Errors are logged but don't halt initialization ✓
However, consider whether
Promise.allSettledwith explicit handling would provide better observability for operators diagnosing plugin issues. Current approach is acceptable for production use.frontend/packages/console-shared/src/hooks/useResourceSidebarSamples.ts (1)
33-46: Well-defined Sample type with good flexibility.The
Sampletype includes bothyamlandlazyYamloptions, which is efficient for large YAML content that shouldn't be serialized until needed. ThetargetResourcestructure aligns with K8s resource identification patterns.frontend/packages/console-shared/src/components/formik-fields/CodeEditorField.tsx (1)
50-54: Clean migration to hook-based approach.The hook invocation correctly passes the
modeland a FirehoseResult-like object withdata,loaded, andloadError. This aligns with the hook's expected signature fromuseResourceSidebarSamples.ts.frontend/public/plugins.ts (4)
20-34: PluginStore configuration looks solid.The
fixedPluginDependencyResolutionspattern allows plugins to declare semver ranges against@console/pluginAPI. The fallback to'4.1337.67'in development is a pragmatic choice whenreleaseVersionisn't available.The
@ts-expect-errorforconsoleFetchis documented as due to console-specific fetch options—acceptable technical debt.
48-62: Well-implemented Redux middleware for feature flag synchronization.The middleware pattern correctly:
- Captures previous flags state
- Passes action through the chain first (
next(action))- Compares new state to previous
- Only updates pluginStore when flags actually change
This avoids unnecessary
setFeatureFlagscalls on every action dispatch. The use oftoObject()on the Immutable Map is correct for the SDK's expected input format.
1-11: No initialization timing concern. Webpack'seager: trueconfiguration on all shared modules (webpack.config.ts:72) ensures__webpack_share_scopes__is populated during bootstrap, before any module code executes. The call togetSharedScope()at line 22 is safe and intentional—the shared scope is guaranteed to exist. The laterinitSharedScope()call in plugin-init.ts is for dynamic plugin loading and operates independently of this static scope.Likely an incorrect or invalid review comment.
38-43: Local plugin loading is correctly implemented as synchronous at module load time.The code properly calls
pluginStore.loadPlugin(plugin)synchronously forLocalPluginManifestobjects withoutawait, which is the correct approach since all plugin data (extensions, metadata) is already available. ThePluginStore.loadPluginmethod supports both synchronous handling for local manifests and asynchronous handling for dynamically fetched plugins via URL.frontend/public/components/app.tsx (3)
437-446: Clear documentation of initialization dependency.The TODO comments accurately describe the dependency on
AppInitSDKsetting up the fetch implementation before dynamic plugins can load. TheinitDynamicPluginswrapper is minimal and delegates toinitConsolePlugins.The comment about reverting HAC-375 suggests this is a known architectural constraint worth tracking.
514-536: Provider hierarchy is correctly ordered.The nesting order is:
Provider(Redux)PluginStoreProvider(Dynamic Plugin SDK)ThemeProviderHelmetProviderAppInitSDKThis ensures that:
- Redux state is available to all components
- Plugin store context is available before
AppInitSDKtriggersdynamicPluginsinitialization- Theme and Helmet contexts wrap the app content
The
dynamicPlugins: initDynamicPluginsconfiguration inAppInitSDK(line 525) correctly uses the new configuration key.
521-526: AppInitSDK properly supports thedynamicPluginsconfiguration—no further action needed.The migration from
initPluginstodynamicPluginsis complete and correctly implemented.AppInitSDKin@console/dynamic-plugin-sdkaccepts thedynamicPluginsconfiguration as an optional parameter (type:() => void), maintaining backward compatibility. The code inapp.tsxproperly passesinitDynamicPluginsto this key, and the SDK correctly invokes it during initialization.frontend/packages/console-dynamic-plugin-sdk/src/app/AppInitSDK.tsx (1)
38-47: Initialization logic looks correct.The effect properly:
- Destructures
dynamicPluginswith optional chaining on invocation- Maintains
apiDiscovery(store)which correctly passes the store instance as required byinitApiDiscovery(per the relevant code snippet showing the expected signature)- Wraps in try/catch for resilience
The dependency array
[configurations, store]is appropriate since both values influence the effect.frontend/public/components/edit-yaml.tsx (7)
15-15: Good consolidation of sidebar sample retrieval.Centralizing sample/snippet retrieval through
useResourceSidebarSamplesis a solid refactor that reduces code duplication and aligns with the React hooks pattern preferred per the PR objectives.
265-289:checkEditAccessrefactor is clean.Extracting
objModelviagetModel(obj)once and using it for both the guard check and attribute construction eliminates redundant lookups. The dependency array correctly includesgetModelalongside the other dependencies.
469-472:updateYAMLcorrectly uses localobjModel.The model is retrieved once and passed to both
k8sCreateandk8sUpdate. This is consistent with the refactoring pattern applied elsewhere in this file.Minor note: if
objModelis null (unlikely given validate runs first), both k8s calls would fail. The current flow wherevalidateis called beforeupdateYAMLinsaveCallbackprovides the necessary guard, so this is fine.
544-568:validatefunction updates are correct.The refactor to use
objModelfor namespaced checks at lines 556 and 561 is appropriate. The dependency array at line 568 correctly includesgetModel.The validation flow ensures that
objModelis non-null before reaching the namespace logic, so the subsequent checks are safe.
793-793: Sidebar content check uses hook-provided data.The condition
!_.isEmpty(samples)now references the hook-providedsamplesrather than a locally computed value. This is consistent with the refactor.
964-972: Sidebar rendering correctly passes hook-derived samples/snippets.The
CodeEditorSidebarreceivessamples(conditionally for create mode) andsnippetsfrom the hook. This completes the refactor chain from retrieval to consumption.
215-217: Model derivation and hook usage are correct.The pattern
getModel(props.obj) || props.modelprovides a sensible fallback chain. The hook is called unconditionally (satisfying rules of hooks), and the returnedsamples/snippetsare consumed downstream. TheuseResourceSidebarSampleshook explicitly handles null/undefined models by checkingif (!kindObj)at the entry point and returning empty arrays{ snippets: [], samples: [] }, so no graceful handling concerns here.
| export const useResourceSidebarSamples = (kindObj: K8sKind, yamlSamplesList: FirehoseResult) => { | ||
| const defaultSamples = useDefaultSamples(); | ||
|
|
||
| if (!kindObj) { | ||
| return { snippets: [], samples: [] }; | ||
| } | ||
|
|
||
| const yamlSamplesData = !_.isEmpty(yamlSamplesList) | ||
| ? _.filter( | ||
| yamlSamplesList.data, | ||
| (sample: K8sResourceKind) => | ||
| sample.spec.targetResource.apiVersion === apiVersionForModel(kindObj) && | ||
| sample.spec.targetResource.kind === kindObj.kind, | ||
| ) | ||
| : []; | ||
|
|
||
| const existingSamples = defaultSamples.get(referenceForModel(kindObj)) || []; | ||
| const extensionSamples = !_.isEmpty(yamlSamplesData) | ||
| ? yamlSamplesData.map((sample: K8sResourceKind) => { | ||
| return { | ||
| id: sample.metadata.uid, | ||
| ...(sample.spec as Exclude<Sample, 'id'>), | ||
| }; | ||
| }) | ||
| : []; | ||
|
|
||
| const allSamples = [...existingSamples, ...extensionSamples]; | ||
|
|
||
| // For the time being, `snippets` are a superset of `samples` | ||
| const snippets = allSamples.filter((sample: Sample) => sample.snippet); | ||
| const samples = allSamples.filter((sample: Sample) => !sample.snippet); | ||
|
|
||
| return { snippets, samples }; | ||
| }; |
There was a problem hiding this comment.
Hook conditionally returns early but still calls hooks unconditionally—good pattern.
The early return for !kindObj (line 348-350) happens after useDefaultSamples() is called, which correctly follows the Rules of Hooks. The filtering and merging logic is straightforward.
One type observation: line 366 uses as Exclude<Sample, 'id'> but this doesn't quite match the intent—you likely want Omit<Sample, 'id'>. Exclude is for union types, not object property exclusion.
🔧 Type correction
- ...(sample.spec as Exclude<Sample, 'id'>),
+ ...(sample.spec as Omit<Sample, 'id'>),📝 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.
| export const useResourceSidebarSamples = (kindObj: K8sKind, yamlSamplesList: FirehoseResult) => { | |
| const defaultSamples = useDefaultSamples(); | |
| if (!kindObj) { | |
| return { snippets: [], samples: [] }; | |
| } | |
| const yamlSamplesData = !_.isEmpty(yamlSamplesList) | |
| ? _.filter( | |
| yamlSamplesList.data, | |
| (sample: K8sResourceKind) => | |
| sample.spec.targetResource.apiVersion === apiVersionForModel(kindObj) && | |
| sample.spec.targetResource.kind === kindObj.kind, | |
| ) | |
| : []; | |
| const existingSamples = defaultSamples.get(referenceForModel(kindObj)) || []; | |
| const extensionSamples = !_.isEmpty(yamlSamplesData) | |
| ? yamlSamplesData.map((sample: K8sResourceKind) => { | |
| return { | |
| id: sample.metadata.uid, | |
| ...(sample.spec as Exclude<Sample, 'id'>), | |
| }; | |
| }) | |
| : []; | |
| const allSamples = [...existingSamples, ...extensionSamples]; | |
| // For the time being, `snippets` are a superset of `samples` | |
| const snippets = allSamples.filter((sample: Sample) => sample.snippet); | |
| const samples = allSamples.filter((sample: Sample) => !sample.snippet); | |
| return { snippets, samples }; | |
| }; | |
| export const useResourceSidebarSamples = (kindObj: K8sKind, yamlSamplesList: FirehoseResult) => { | |
| const defaultSamples = useDefaultSamples(); | |
| if (!kindObj) { | |
| return { snippets: [], samples: [] }; | |
| } | |
| const yamlSamplesData = !_.isEmpty(yamlSamplesList) | |
| ? _.filter( | |
| yamlSamplesList.data, | |
| (sample: K8sResourceKind) => | |
| sample.spec.targetResource.apiVersion === apiVersionForModel(kindObj) && | |
| sample.spec.targetResource.kind === kindObj.kind, | |
| ) | |
| : []; | |
| const existingSamples = defaultSamples.get(referenceForModel(kindObj)) || []; | |
| const extensionSamples = !_.isEmpty(yamlSamplesData) | |
| ? yamlSamplesData.map((sample: K8sResourceKind) => { | |
| return { | |
| id: sample.metadata.uid, | |
| ...(sample.spec as Omit<Sample, 'id'>), | |
| }; | |
| }) | |
| : []; | |
| const allSamples = [...existingSamples, ...extensionSamples]; | |
| // For the time being, `snippets` are a superset of `samples` | |
| const snippets = allSamples.filter((sample: Sample) => sample.snippet); | |
| const samples = allSamples.filter((sample: Sample) => !sample.snippet); | |
| return { snippets, samples }; | |
| }; |
🤖 Prompt for AI Agents
In @frontend/packages/console-shared/src/hooks/useResourceSidebarSamples.ts
around lines 345 - 378, The type cast inside useResourceSidebarSamples that
builds extensionSamples incorrectly uses Exclude<Sample, 'id'>; change that to
Omit<Sample, 'id'> to remove the id property from the Sample object type when
spreading sample.spec; update the mapping that constructs each extension sample
(where id: sample.metadata.uid and ...(sample.spec as ...)) to use Omit<Sample,
'id'> so TypeScript treats sample.spec as the Sample without id, keeping
useDefaultSamples, useResourceSidebarSamples, extensionSamples and allSamples
logic unchanged.
250803e to
f50b47b
Compare
|
@logonoff: This pull request references CONSOLE-3769 which is a valid jira issue. 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: 7
🤖 Fix all issues with AI agents
In
@frontend/packages/console-app/src/components/flags/FeatureFlagExtensionLoader.tsx:
- Around line 14-18: The import currently pulls Extension, ExtensionTypeGuard,
and ResolvedExtension from the SDK's internal path
'@console/dynamic-plugin-sdk/src/types'; change that import to use the public
entry point '@console/dynamic-plugin-sdk' instead so you import the same types
(Extension, ExtensionTypeGuard, ResolvedExtension) from the supported public API
and avoid relying on internal module paths.
In @frontend/packages/console-dynamic-plugin-sdk/README.md:
- Line 223: Add detailed docs to the README entry that announces adding
@openshift/dynamic-plugin-sdk: explain briefly what the upstream SDK provides,
state whether this is breaking or backwards-compatible for plugins, list any
required actions for plugin maintainers (dependency updates, import changes,
build/config tweaks), clarify the relationship and compatibility with
@openshift-console/dynamic-plugin-sdk, and include migration guidance or a link
to migration docs (use the react-redux upgrade note format on lines ~221-222 as
a template).
- Line 128: Add a new row in the "Distributable SDK package overview" table
documenting `@openshift/dynamic-plugin-sdk`: state that it is an upstream
re-exported types/helpers package (origin), summarize what it exposes (types and
re-exports from upstream dynamic-plugin-sdk), clarify relationship to
`@openshift-console/dynamic-plugin-sdk` (Console SDK wraps/extends it with
Console-specific helpers and should remain the primary import for Console
plugins), and give guidance on when plugin authors should import
`@openshift/dynamic-plugin-sdk` directly versus using Console SDK packages; then
update the Console 4.22.X changelog entry to mention this addition and add a
brief cross-reference to the overview table for details.
In @frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-init.ts:
- Around line 18-57: The function loadAndEnablePlugin can throw errors outside
the current .catch() (including synchronous throws) and callers like
initConsolePlugins must await it; wrap the entire body of loadAndEnablePlugin in
a try/catch so any thrown error is passed to onError (include err and err.cause
in the message) and return early on failure, and update initConsolePlugins to
await the Promise returned by loadAndEnablePlugin so plugin loading order and
error handling are deterministic; ensure you still call pluginStore.loadPlugin
with await inside the try block and preserve the existing plugin status checks
(plugin?.status === 'failed' / 'loaded') inside the try.
In @frontend/packages/console-shared/locales/en/console-shared.json:
- Line 298: Replace the misspelled translation key "Set maxUnavaliable to 0"
with the corrected key "Set maxUnavailable to 0" in every locale file under
frontend/packages/console-shared/locales/*/console-shared.json so they match the
English key used by t('Set maxUnavailable to 0'); keep each locale's translation
value intact when renaming the key and run a quick search across the locales
directory to ensure no instances of the old key remain.
In @frontend/packages/console-shared/src/hooks/useResourceSidebarSamples.ts:
- Around line 257-270: The lazyYaml builder assumes extension.properties.label
and .description are strings and will throw if they are undefined or
non-strings; update the lazyYaml implementation (the function referenced as
lazyYaml that maps over addActions and accesses
extension.properties.id/label/description) to coerce or default these values to
strings before calling .split — e.g., guard with typeof checks or use
String(extension.properties.label ?? '') and
String(extension.properties.description ?? '') (also ensure id is present or
default to a safe string) so .split('\n') never runs on a non-string value.
In @frontend/public/plugins.ts:
- Around line 20-34: The inline @ts-expect-error on
PluginStore.loaderOptions.fetchImpl hides a specific signature mismatch:
consoleFetch accepts a third timeout parameter not present on the standard fetch
type; either make the comment explicit (e.g., “// @ts-expect-error:
consoleFetch's timeout parameter doesn't exist in standard fetch”) or replace
the suppression by adding a lightweight wrapper that matches the expected
signature and delegates to consoleFetch (e.g., create fetchImpl that accepts
(url, options) and calls consoleFetch(url, options, defaultTimeout)) and use
that wrapper for loaderOptions.fetchImpl to remove the type mismatch while
preserving runtime behavior.
🧹 Nitpick comments (7)
frontend/packages/console-plugin-sdk/src/api/useExtensions.ts (1)
76-94: Refs mutation insideuseMemowarrants attention.The pattern of updating
previousResultRefandpreviousUIDsRefinsideuseMemo(lines 90-91) works but is technically a side effect during render. React's concurrent features could theoretically invoke this multiple times before commit, though in practice this is safe here because:
- The operation is idempotent
- The refs only affect future renders' memoization
For strict correctness, consider moving the ref updates to a
useEffect, though the current approach is pragmatic and avoids an extra render cycle.♻️ Optional: Stricter side-effect handling
return useMemo(() => { const sorted = sortExtensionsByPluginOrder(translatedExtensions); const currentUIDs = sorted.map((e) => e.uid).join(','); // Return previous result if the extensions haven't changed if (currentUIDs === previousUIDsRef.current) { return previousResultRef.current; } - // Update refs and return new result - previousResultRef.current = sorted; - previousUIDsRef.current = currentUIDs; - return sorted; }, [translatedExtensions]); + + // Update refs after render + useEffect(() => { + const sorted = sortExtensionsByPluginOrder(translatedExtensions); + const currentUIDs = sorted.map((e) => e.uid).join(','); + previousResultRef.current = sorted; + previousUIDsRef.current = currentUIDs; + }, [translatedExtensions]);frontend/packages/console-app/src/components/flags/FeatureFlagExtensionLoader.tsx (1)
3-3: Consider usinglodash-esfor tree-shaking consistency.The project uses
lodash-eselsewhere. Importing fromlodashdirectly may prevent optimal tree-shaking and increase bundle size.♻️ Suggested change
-import { difference } from 'lodash'; +import { difference } from 'lodash-es';frontend/packages/console-shared/src/hooks/useResourceSidebarSamples.ts (1)
93-343: Consider memoizing the ImmutableMap to avoid recreation on every render.
useDefaultSamplesreturns a newImmutableMapon every call. Since the consuming hookuseResourceSidebarSamplesis called on every render, this creates unnecessary object churn. Consider wrapping withuseMemokeyed on the resolved extensions.♻️ Proposed refactor
const useDefaultSamples = () => { const { t } = useTranslation('console-shared'); const [addActions] = useResolvedExtensions<AddAction>(isAddAction); const [catalogItemTypes] = useResolvedExtensions<CatalogItemType>(isCatalogItemType); const [perspectives] = useResolvedExtensions<Perspective>(isPerspective); const clusterRoleBindingSamples = useClusterRoleBindingSamples(); - return ImmutableMap<GroupVersionKind, Sample[]>() + return useMemo(() => ImmutableMap<GroupVersionKind, Sample[]>() .setIn( [referenceForModel(BuildConfigModel)], // ... all the sample definitions ... - ); + ), [t, addActions, catalogItemTypes, perspectives, clusterRoleBindingSamples]); };frontend/public/plugins.ts (1)
26-31: Hardcoded fallback version for development is reasonable but visible.The
'4.1337.67'fallback is clearly a development placeholder, but it could confuse developers debugging plugin compatibility issues. Consider using a more obviously fake version or adding a console warning when this fallback is used.'@console/pluginAPI': process.env.NODE_ENV === 'production' ? CURRENT_OPENSHIFT_VERSION // this is always provided by console-operator in production - : CURRENT_OPENSHIFT_VERSION || '4.1337.67', + : CURRENT_OPENSHIFT_VERSION || '0.0.0-dev',frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-init.ts (2)
35-56: Plugin lookup by name may miss the plugin if manifest name differs from request.The lookup
pluginStore.getPluginInfo().find((p) => p.manifest.name === pluginName)assumes the manifest'snamefield exactly matchespluginName. If there's any normalization or aliasing happening in the SDK, this could silently fail to find the plugin, leavingpluginasundefinedand skipping both error reporting anddisableStaticPluginslogic.Consider logging when
pluginisundefinedafter a successfulloadPlugincall to aid debugging.const plugin = pluginStore.getPluginInfo().find((p) => p.manifest.name === pluginName); + if (!plugin) { + console.warn(`[loadAndEnablePlugin] ${pluginName}: plugin not found in store after load attempt`); + return; + } + if (plugin?.status === 'failed') {
102-125: Fire-and-forget plugin loading with no completion signal.
dynamicPluginNames.forEach()kicks off plugin loads without awaiting them. This is intentional for parallel loading, but:
- No mechanism exists to know when all plugins are loaded (e.g., for splash screen dismissal or readiness checks)
window.windowErrorconcatenation with;separator is fragile and may cause parsing issues if error messages contain;Consider using
Promise.allSettled()and emitting a custom event or callback when all plugins have loaded (success or failure).initSharedScope() .then(() => { - dynamicPluginNames.forEach((pluginName) => { - loadAndEnablePlugin(pluginName, pluginStore, (errorMessage, errorCause) => { + const loadPromises = dynamicPluginNames.map((pluginName) => + loadAndEnablePlugin(pluginName, pluginStore, (errorMessage, errorCause) => { // eslint-disable-next-line no-console console.error(..._.compact([errorMessage, errorCause])); window.windowError = `${window.windowError ?? ''};${errorMessage}: ${String(errorCause)}`; - }); - }); + }), + ); + + Promise.allSettled(loadPromises).then(() => { + // Optional: emit event or set flag indicating plugin loading complete + window.dispatchEvent(new CustomEvent('console-plugins-loaded')); + }); if (process.env.NODE_ENV !== 'production') {frontend/packages/console-dynamic-plugin-sdk/src/coderefs/coderef-resolver.ts (1)
16-26: Symbol extraction is clever but couples to SDK internals.Extracting the SDK's private
codeRefSymbolby inspectingObject.getOwnPropertySymbols()on a dummy function works, but it's inherently fragile:
- If the SDK changes how it applies symbols (e.g., adds metadata), the
[0]index assumption breaks- If the SDK interns symbols differently across versions, detection could fail silently
The guard on lines 24-26 mitigates catastrophic failures, but consider adding a comment noting this is intentional coupling and may need updates when upgrading
@openshift/dynamic-plugin-sdk./** * Extract the SDK's internal CodeRef symbol by applying it to a dummy function. * * This ensures we can detect code refs created by the SDK, which uses its own * private Symbol instance. + * + * NOTE: This couples us to SDK internals. If upgrading @openshift/dynamic-plugin-sdk + * causes isExecutableCodeRef to fail, verify the symbol extraction still works. */ const codeRefSymbol = Object.getOwnPropertySymbols(applyCodeRefSymbol(() => Promise.resolve()))[0];
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
frontend/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (58)
frontend/__mocks__/webpack.tsfrontend/package.jsonfrontend/packages/console-app/src/__tests__/plugin-test-utils.tsfrontend/packages/console-app/src/components/dashboards-page/dynamic-plugins-health-resource/NotLoadedDynamicPlugins.tsxfrontend/packages/console-app/src/components/file-upload/__tests__/file-upload-utils.spec.tsfrontend/packages/console-app/src/components/flags/FeatureFlagExtensionLoader.tsxfrontend/packages/console-app/src/components/nodes/useNodeStatusExtensions.tsxfrontend/packages/console-app/src/components/user-preferences/__tests__/userPreferences.data.tsxfrontend/packages/console-app/src/components/user-preferences/perspective/__tests__/perspective.data.tsfrontend/packages/console-dynamic-plugin-sdk/CHANGELOG-core.mdfrontend/packages/console-dynamic-plugin-sdk/CHANGELOG-webpack.mdfrontend/packages/console-dynamic-plugin-sdk/README.mdfrontend/packages/console-dynamic-plugin-sdk/package.jsonfrontend/packages/console-dynamic-plugin-sdk/scripts/package-definitions.tsfrontend/packages/console-dynamic-plugin-sdk/src/app/AppInitSDK.tsxfrontend/packages/console-dynamic-plugin-sdk/src/coderefs/__tests__/coderef-resolver.spec.tsfrontend/packages/console-dynamic-plugin-sdk/src/coderefs/coderef-resolver.tsfrontend/packages/console-dynamic-plugin-sdk/src/perspective/__tests__/perspective.data.tsfrontend/packages/console-dynamic-plugin-sdk/src/runtime/__tests__/plugin-manifest.spec.tsfrontend/packages/console-dynamic-plugin-sdk/src/runtime/__tests__/plugin-utils.spec.tsfrontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-dependencies.tsfrontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-init.tsfrontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-loader.tsfrontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-manifest.tsfrontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-utils.tsfrontend/packages/console-dynamic-plugin-sdk/src/shared-modules/shared-modules-meta.tsfrontend/packages/console-dynamic-plugin-sdk/src/types.tsfrontend/packages/console-dynamic-plugin-sdk/src/utils/test-utils.tsfrontend/packages/console-dynamic-plugin-sdk/src/webpack/ConsoleRemotePlugin.tsfrontend/packages/console-plugin-sdk/package.jsonfrontend/packages/console-plugin-sdk/src/api/pluginSubscriptionService.tsfrontend/packages/console-plugin-sdk/src/api/useExtensions.tsfrontend/packages/console-plugin-sdk/src/codegen/local-plugins.tsfrontend/packages/console-plugin-sdk/src/store.tsfrontend/packages/console-plugin-sdk/src/utils/allowed-plugins.tsfrontend/packages/console-shared/locales/en/console-shared.jsonfrontend/packages/console-shared/src/components/actions/__tests__/utils-test-data.tsfrontend/packages/console-shared/src/components/catalog/__tests__/CatalogController.spec.tsxfrontend/packages/console-shared/src/components/editor/CodeEditorSidebar.tsxfrontend/packages/console-shared/src/components/formik-fields/CodeEditorField.tsxfrontend/packages/console-shared/src/hooks/__tests__/useTelemetry.spec.tsfrontend/packages/console-shared/src/hooks/useResourceSidebarSamples.tsfrontend/packages/console-shared/src/utils/index.tsfrontend/packages/console-shared/src/utils/sample-utils.tsfrontend/packages/dev-console/src/components/add/__tests__/add-page-test-data.tsxfrontend/packages/dev-console/src/utils/__tests__/useAddActionExtensions.spec.tsfrontend/packages/operator-lifecycle-manager/src/components/operand/__tests__/index.spec.tsxfrontend/packages/topology/src/data-transforms/DataModelExtension.tsxfrontend/packages/topology/src/data-transforms/DataModelProvider.tsxfrontend/public/actions/features.tsfrontend/public/components/app.tsxfrontend/public/components/dashboard/project-dashboard/getting-started/__tests__/SampleGettingStartedCard.data.tsfrontend/public/components/edit-yaml.tsxfrontend/public/components/notification-drawer.tsxfrontend/public/components/sidebars/resource-sidebar-samples.tsxfrontend/public/components/sidebars/resource-sidebar.tsxfrontend/public/components/utils/__tests__/nav.spec.tsfrontend/public/plugins.ts
💤 Files with no reviewable changes (24)
- frontend/packages/console-app/src/components/file-upload/tests/file-upload-utils.spec.ts
- frontend/public/components/utils/tests/nav.spec.ts
- frontend/packages/console-shared/src/components/actions/tests/utils-test-data.ts
- frontend/packages/console-dynamic-plugin-sdk/src/runtime/tests/plugin-manifest.spec.ts
- frontend/packages/console-dynamic-plugin-sdk/src/webpack/ConsoleRemotePlugin.ts
- frontend/packages/console-plugin-sdk/src/api/pluginSubscriptionService.ts
- frontend/packages/console-shared/src/utils/sample-utils.ts
- frontend/packages/console-shared/src/hooks/tests/useTelemetry.spec.ts
- frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-utils.ts
- frontend/packages/console-shared/src/utils/index.ts
- frontend/packages/console-dynamic-plugin-sdk/package.json
- frontend/packages/console-plugin-sdk/src/store.ts
- frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-manifest.ts
- frontend/packages/console-dynamic-plugin-sdk/src/perspective/tests/perspective.data.ts
- frontend/packages/console-app/src/components/user-preferences/tests/userPreferences.data.tsx
- frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-loader.ts
- frontend/packages/dev-console/src/components/add/tests/add-page-test-data.tsx
- frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-dependencies.ts
- frontend/packages/dev-console/src/utils/tests/useAddActionExtensions.spec.ts
- frontend/packages/console-app/src/components/user-preferences/perspective/tests/perspective.data.ts
- frontend/packages/console-shared/src/components/catalog/tests/CatalogController.spec.tsx
- frontend/public/components/dashboard/project-dashboard/getting-started/tests/SampleGettingStartedCard.data.ts
- frontend/packages/console-plugin-sdk/package.json
- frontend/packages/console-dynamic-plugin-sdk/src/runtime/tests/plugin-utils.spec.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- frontend/packages/topology/src/data-transforms/DataModelExtension.tsx
- frontend/mocks/webpack.ts
- frontend/packages/console-dynamic-plugin-sdk/src/coderefs/tests/coderef-resolver.spec.ts
- frontend/packages/console-dynamic-plugin-sdk/src/types.ts
- frontend/packages/console-shared/src/components/editor/CodeEditorSidebar.tsx
- frontend/public/components/app.tsx
- frontend/package.json
- frontend/packages/console-dynamic-plugin-sdk/CHANGELOG-webpack.md
- frontend/public/actions/features.ts
- frontend/packages/console-dynamic-plugin-sdk/src/shared-modules/shared-modules-meta.ts
- frontend/packages/console-dynamic-plugin-sdk/src/app/AppInitSDK.tsx
- frontend/packages/topology/src/data-transforms/DataModelProvider.tsx
🧰 Additional context used
🧬 Code graph analysis (10)
frontend/packages/console-shared/src/components/formik-fields/CodeEditorField.tsx (1)
frontend/packages/console-shared/src/hooks/useResourceSidebarSamples.ts (1)
useResourceSidebarSamples(345-378)
frontend/public/components/notification-drawer.tsx (2)
frontend/public/components/utils/resource-link.tsx (1)
resourcePath(54-66)frontend/public/models/index.ts (1)
ConsolePluginModel(1296-1311)
frontend/packages/console-dynamic-plugin-sdk/src/coderefs/coderef-resolver.ts (1)
frontend/packages/console-dynamic-plugin-sdk/src/types.ts (1)
CodeRef(31-31)
frontend/packages/console-app/src/components/dashboards-page/dynamic-plugins-health-resource/NotLoadedDynamicPlugins.tsx (1)
frontend/public/models/index.ts (1)
ConsolePluginModel(1296-1311)
frontend/packages/console-app/src/__tests__/plugin-test-utils.ts (2)
frontend/packages/console-plugin-sdk/src/codegen/local-plugins.ts (1)
loadLocalPluginsForTestPurposes(78-96)frontend/packages/console-plugin-sdk/src/codegen/plugin-resolver.ts (1)
resolvePluginPackages(78-105)
frontend/packages/console-app/src/components/flags/FeatureFlagExtensionLoader.tsx (5)
frontend/packages/console-shared/src/hooks/useConsoleDispatch.ts (1)
useConsoleDispatch(14-14)frontend/packages/console-shared/src/hooks/useConsoleSelector.ts (1)
useConsoleSelector(12-12)frontend/packages/console-dynamic-plugin-sdk/src/extensions/feature-flags.ts (5)
SetFeatureFlag(45-45)FeatureFlag(5-11)isFeatureFlag(35-35)ModelFeatureFlag(14-22)isModelFeatureFlag(37-38)frontend/packages/console-app/src/features/integration-test.ts (1)
handler(13-18)frontend/public/actions/flags.ts (1)
updateModelFlags(146-150)
frontend/packages/console-plugin-sdk/src/api/useExtensions.ts (3)
frontend/packages/console-plugin-sdk/src/utils/allowed-plugins.ts (1)
dynamicPluginNames(25-25)frontend/packages/console-dynamic-plugin-sdk/src/types.ts (2)
Extension(12-12)LoadedExtension(17-17)frontend/packages/console-plugin-sdk/src/utils/useTranslatedExtensions.ts (1)
useTranslatedExtensions(14-50)
frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-init.ts (4)
frontend/public/plugins.ts (1)
pluginStore(20-34)frontend/packages/console-dynamic-plugin-sdk/src/build-types.ts (1)
ConsolePluginManifest(46-52)frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-shared-modules.ts (2)
initSharedScope(13-13)getSharedScope(18-24)frontend/packages/console-plugin-sdk/src/utils/allowed-plugins.ts (1)
dynamicPluginNames(25-25)
frontend/packages/console-plugin-sdk/src/utils/allowed-plugins.ts (2)
frontend/public/module/auth.js (1)
window(5-12)frontend/public/components/utils/link.tsx (1)
getURLSearchParams(60-69)
frontend/packages/console-plugin-sdk/src/codegen/local-plugins.ts (2)
frontend/packages/console-plugin-sdk/src/codegen/plugin-resolver.ts (1)
PluginPackage(111-115)frontend/get-local-plugins.js (1)
getLocalPluginsModuleData(18-22)
🔇 Additional comments (48)
frontend/packages/console-app/src/components/nodes/useNodeStatusExtensions.tsx (2)
48-48: LGTM — Destructuring updated to useuidandtype.This aligns with the SDK migration from
pluginIDtouid. Thetypefield is a sensible addition for improved error diagnostics downstream.
63-66: Error logging now usesuidandtype— good diagnostics.The updated error message format provides sufficient context for debugging extension failures. Including
typealongsideuidis helpful since it clarifies what kind of extension failed without requiring a lookup.One minor note: consider whether you want a consistent delimiter (e.g.,
Extension ${uid} (${type}) failed:) for readability, but the current format is perfectly functional.frontend/packages/operator-lifecycle-manager/src/components/operand/__tests__/index.spec.tsx (1)
226-228: Reasonable adjustment for pluginStore initialization, but consider test resilience.The expectation bump from 1 to 2 calls makes sense given the new SDK-based plugin store lifecycle. The inline comment adequately documents the rationale, which is helpful for future maintainers.
One consideration: asserting an exact call count can become a maintenance burden if the initialization sequence evolves further. If this test's primary goal is validating the
pagesconfiguration, you could relax the assertion toexpect(mockDetailsPage).toHaveBeenCalled()(ortoHaveBeenCalledTimes(expect.any(Number))), then rely onmock.calls[0]for props validation. That said, the explicit count does serve as a regression guard if unexpected re-renders slip in.Given this is intentional and documented, the current form is acceptable.
frontend/packages/console-shared/locales/en/console-shared.json (1)
302-314: LGTM — key relocation has no functional impact.JSON key ordering doesn't affect i18n lookups. The pluralization suffixes (
_one/_other) and interpolation placeholders ({{count}},{{label}},{{podSubTitle}}) follow i18next conventions correctly.frontend/packages/console-dynamic-plugin-sdk/scripts/package-definitions.ts (1)
175-185: Dependency enumeration is solid and complete.All nine listed dependencies (ajv, chalk, comment-json, find-up, glob, read-pkg, semver, webpack, @openshift/dynamic-plugin-sdk) are actively used during webpack plugin compilation—schema validation, console logging, JSON parsing with comments, file discovery, and build metadata handling. The alphabetical ordering keeps the list maintainable, and moving @openshift/dynamic-plugin-sdk here aligns cleanly with the SDK migration to lib-core.
Lodash is correctly added separately via
parseDepsAs, and TypeScript is appropriately declared as a peerDependency (line 191), leaving this array focused on direct runtime requirements.frontend/packages/console-app/src/components/dashboards-page/dynamic-plugins-health-resource/NotLoadedDynamicPlugins.tsx (3)
2-2: Clean migration to upstream SDK types.Using
FailedPluginInfoEntry | PendingPluginInfoEntryfrom@openshift/dynamic-plugin-sdkproperly models the "not loaded" state as a union of the two failure modes. Theimport typeusage is correct for type-only imports.
27-30: Public type updated to reflect SDK entry types.The updated
NotLoadedDynamicPluginsPropsnow uses SDK types directly, which is cleaner than maintaining internal wrapper types. The union(FailedPluginInfoEntry | PendingPluginInfoEntry)[]explicitly communicates the two states this component handles.Note that this is technically a breaking change to the component's public interface. If any external consumers exist, they'll need to update their type references. Given this appears to be an internal dashboard component, the risk is low.
14-17: Correct migration tomanifest.nameas the canonical plugin identifier.The shift from
pluginNametomanifest.namealigns with the upstream SDK's type shape. BothFailedPluginInfoEntryandPendingPluginInfoEntryfrom@openshift/dynamic-plugin-sdk@^6.0.0definemanifestas a required property, so the direct access toplugin.manifest.nameon lines 14 and 17 is type-safe. The consistent use of this pattern across the codebase (DynamicPluginsPopover, ConsoleOperatorConfig) confirms it's the standard approach. Plugin names are unique within a cluster, making this a valid and stable React key.frontend/packages/console-dynamic-plugin-sdk/CHANGELOG-core.md (1)
21-21: LGTM!The changelog entry correctly documents the breaking change for
pluginIDremoval fromuseResolvedExtensionshook. The format is consistent with existing entries, and the PR reference link is properly added.Also applies to: 234-234
frontend/packages/console-plugin-sdk/src/utils/allowed-plugins.ts (2)
4-17: Clean implementation with clear edge-case handling.The three-way branching for
disable-pluginsparam is well-documented by behavior:
- Empty string (
'') → disable all plugins- Absent → enable all server-defined plugins
- Present with values → filter specified plugins
One minor observation:
uniqis applied toallPluginNamesbut notdisabledPluginNames. Whileincludes()handles duplicates gracefully, applyinguniqtodisabledPluginNameswould be marginally more efficient for large disable lists. Not critical given typical usage.
24-25: Module-level evaluation is intentional and working as designed.
dynamicPluginNamesis evaluated once at module import time based onwindow.SERVER_FLAGS.consolePluginsand URL parameters—correct for this use case since those values don't change during the app lifecycle. This is also reflected inuseExtensions.ts(line 11), wherepluginOrderMapis created at module load.Tests that depend on extension ordering properly handle this by mocking the
useExtensionsfunction directly rather than the module, which naturally avoids any issues with stale computed values. No explicit module-level mocking ofallowed-pluginsis needed in the test suite.frontend/packages/console-plugin-sdk/src/api/useExtensions.ts (2)
24-32: Sorting logic correctly prioritizes static plugins.Using
Number.MIN_SAFE_INTEGERas the fallback for plugins not inpluginOrderMapensures static (local) plugins appear first, which aligns with the documented behavior. The spread operator[...extensions]correctly avoids mutating the input array.The stable sort behavior of
Array.prototype.sortin modern JS engines means extensions from the same plugin maintain their relative order.
73-74: Good composition with upstream SDK hook.Clean delegation to
useExtensionsSDKfollowed by translation maintains separation of concerns. The translation step ensures i18n strings are properly resolved before sorting and memoization.frontend/packages/console-plugin-sdk/src/codegen/local-plugins.ts (5)
3-3: Good alignment with upstream SDK types.Importing
LocalPluginManifestfrom@openshift/dynamic-plugin-sdkensures type consistency with the upstream plugin infrastructure and reduces type drift.
58-63: Manifest structure now includes version and registration method.Adding
versionandregistrationMethod: 'local'to the generated module output aligns with theLocalPluginManifesttype requirements and enables downstream code to distinguish local plugins from dynamically loaded ones.
78-96: Test utility correctly mirrors codegen output.
loadLocalPluginsForTestPurposesproducingLocalPluginManifest[]with matching structure (includingversionandregistrationMethod) ensures test fixtures accurately reflect runtime behavior. The sync between this andgetLocalPluginsModuleis critical.The comment on line 76-77 ("keep this in sync with
getLocalPluginsModule") is helpful, but consider whether a shared constant or type could enforce this invariant at compile time rather than relying on manual synchronization.
136-142: Improved testability viacodeRefSourceGetterparameter.Adding
codeRefSourceGetteras an injectable dependency with a sensible default enables unit testing ofgetDynamicExtensionswithout requiring the full file system setup. This is a solid pattern for improving test isolation.
175-185: Module data function correctly updated.The rename to
getLocalPluginsModuleDataand import path update to@openshift/dynamic-plugin-sdkforapplyCodeRefSymbolare correctly in place, completing the migration from legacy console-internal paths to the upstream SDK. TheapplyCodeRefSymbolexport is consistently used across the dynamic plugin SDK infrastructure.frontend/packages/console-app/src/components/flags/FeatureFlagExtensionLoader.tsx (5)
37-47: Well-designed deferred dispatch pattern.The dependency-free
useLayoutEffectis a valid "flush pending work" pattern that correctly avoids React-Redux 8.x rendering errors. The early return whenpendingUpdatesRef.current.size === 0guards against unnecessary work on every render.One minor observation: consider adding an explicit
// eslint-disable-next-line react-hooks/exhaustive-depscomment with a brief note, as linters may flag the missing dependency array. This documents the intentional choice for future maintainers.
59-75: Solid extension diffing pattern; callback stability is critical.The implementation correctly tracks extension changes using reference-based diffing. Both current callers properly memoize their
onChangehandlers withuseCallback, which is essential sinceonChangeappears in theuseEffectdependency array.Consider adding a JSDoc note warning that
onChangemust be stable (memoized) to prevent infinite update loops:/** * @param onChange - Must be memoized (e.g., via useCallback) to prevent infinite loops. */
81-97: Good error isolation and logging.The try/catch around handler invocation ensures a misbehaving plugin doesn't break other feature flag extensions. Including
pluginNamein the error message provides good debugging context for operators troubleshooting plugin issues.The choice to process only
addedextensions (and ignoreremoved) is correct—feature flags are typically additive and don't need to be "unset" when an extension is removed.
103-126: Clean ref-based pattern for stable callback with current state.The
modelsRefpattern correctly preserves callback identity while accessing the latestmodelsstate. The inline comment explaining why models are passed in the action payload (cross-reducer state access) is helpful architectural documentation.This approach aligns with React best practices for avoiding stale closures in memoized callbacks.
132-161: Clean integration of extension processing hooks.The component effectively separates concerns:
useFeatureFlagExtensionshandlesconsole.flagextensions via hooksuseModelFeatureFlagExtensionshandlesconsole.flag/modelextensions via hooksFeatureFlagHookProviderextensions are handled via render (requiring component instances)Using
uidas the React key is correct. The conditional render returningnulluntilflagProviderResolvedis appropriate.frontend/packages/console-app/src/__tests__/plugin-test-utils.ts (2)
1-13: LGTM on the SDK migration for test utilities.Clean migration to the
@openshift/dynamic-plugin-sdkPluginStore. The explicit per-plugin loading loop aligns with how the SDK expects plugins to be registered, and matches the pattern used infrontend/public/plugins.ts.
17-26:getDuplicatesimplementation is correct.The lodash
transform+countByapproach efficiently identifies duplicate values. Minor note: this could alternatively use a single-pass reduce, but the current implementation is readable and performant for typical test scenarios.frontend/public/components/edit-yaml.tsx (4)
215-216: Hook placement and model derivation look correct.The
useResourceSidebarSampleshook is called unconditionally at the component's top level, which respects the Rules of Hooks. The model derivationgetModel(props.obj) || props.modelprovides a sensible fallback chain.
265-289: RBAC access check properly uses derived model.The
objModelvariable correctly sourcesapiGroupandpluralfor theAccessReviewResourceAttributes. The dependency array forcheckEditAccessnow includesgetModel, which is appropriate sincegetModelis memoized viauseCallback.
469-472: API operations correctly useobjModel.The
k8sCreateandk8sUpdatecalls properly use the model derived from the object being created/updated. This ensures the correct API endpoint is targeted.
544-568: Validation logic correctly handles namespaced resources.The validation checks
objModel.namespacedfor namespace handling, which is the proper approach. The early return pattern for missing model/metadata is sound.frontend/packages/console-shared/src/hooks/useResourceSidebarSamples.ts (1)
345-377: Hook structure is correct; early return is safe.The early return on line 348-350 occurs after
useDefaultSamples()is called, so the Rules of Hooks are not violated. The filtering and aggregation logic for samples/snippets is straightforward and correct.One observation: the comment on line 373 says "snippets are a superset of samples," but the filter logic shows snippets have
sample.snippet === truewhile samples have!sample.snippet. This means they're mutually exclusive, not a superset relationship. Consider updating the comment for accuracy.frontend/packages/console-shared/src/components/formik-fields/CodeEditorField.tsx (2)
18-18: Clean integration with the newuseResourceSidebarSampleshook.The migration from direct
getResourceSidebarSamplescalls to the hook-based approach is well executed. The hook is called unconditionally, and the destructuredsamplesandsnippetsare used appropriately in the rendering logic.Also applies to: 28-58
50-54: No type assertion needed—the hook already receives compatible types.The object
{ data: sampleResources, loaded, loadError }is assignable toFirehoseResultbecause TypeScript's structural typing allows optional properties (optional?andkind?) to be omitted. Since the hook only accesses.data(the required property), the call is type-safe as-is.frontend/public/plugins.ts (2)
48-62: Feature flag middleware implementation is correct.The middleware correctly:
- Captures previous flags state
- Passes action through the chain
- Compares next state with previous
- Only calls
setFeatureFlagswhen flags actually changeThis avoids unnecessary updates to the plugin store. The reference equality check on line 55 (
nextFlags !== prevFlags) is appropriate for Immutable.js objects.
64-67: Debug exposure is appropriately gated.Exposing
window.pluginStoreonly in non-production environments is a reasonable debugging aid. The check usesprocess.env.NODE_ENV !== 'production'which is the standard pattern.frontend/public/components/notification-drawer.tsx (2)
148-236: Consistent migration tomanifest.nameacross plugin error handling.The changes properly reference
plugin.manifest.namefor:
- React key generation (Line 217)
- Resource path navigation (Line 219)
- User-facing error message interpolation (Line 228)
This aligns with the SDK's
PluginInfoEntryshape where plugin identification moves frompluginID(with version) tomanifest.name(without version).
17-17: LGTM on SDK type import.Clean migration to
PluginInfoEntryfrom@openshift/dynamic-plugin-sdk. This aligns with the PR's goal of adopting lib-core from the SDK. The type properly exposes themanifest.nameproperty used consistently across this component.frontend/public/components/sidebars/resource-sidebar.tsx (1)
15-15: Import path consolidation looks good.Moving the
Sampletype import to co-locate withuseResourceSidebarSampleshook follows the pattern of keeping types near their primary consumers. This improves discoverability and module cohesion.frontend/public/components/sidebars/resource-sidebar-samples.tsx (2)
11-11: Import path aligned with hook module.Consistent with the change in
resource-sidebar.tsx- theSampletype is now sourced from the hook module.
96-107: The concern aboutlazyYamlcontract is not applicable—the type contract was correctly defined and all implementations already conform.The
Sampletype definition (inuseResourceSidebarSamples.tsline 40) explicitly specifies:lazyYaml?: () => string;TypeScript enforces this signature, preventing any async implementations from being passed. All existing
lazyYamlproviders already return strings synchronously viaYAML.dump()or direct transformations. The syncresolveYamlfunction correctly matches the type contract, and the try/catch properly handles any runtime errors from the sync invocation.Likely an incorrect or invalid review comment.
frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-init.ts (2)
78-89: Legacy callback shim looks correct for backward compatibility.The
pluginName.slice(0, pluginName.lastIndexOf('@'))correctly strips the version suffix (e.g.,my-plugin@1.0.0→my-plugin). The guardpluginName.includes('@')handles the case where no version is present.One edge case: if a plugin name itself contains
@(e.g.,@org/plugin@1.0.0), thelastIndexOfapproach correctly preserves the scoped package prefix. Good defensive coding here.
117-120: Debug exposure of webpack share scope is appropriate.Gating
window.webpackSharedScopebehindprocess.env.NODE_ENV !== 'production'is the right call. This prevents exposing internal webpack state in production while aiding development debugging.frontend/packages/console-dynamic-plugin-sdk/src/utils/test-utils.ts (3)
1-1: Import migration to public SDK is correct.Moving from the internal
../coderefs/coderef-resolverimport to@openshift/dynamic-plugin-sdkaligns with the PR's goal of using the upstream SDK. This ensures the symbol used here matches what the SDK'sisExecutableCodeRefexpects.
27-51: Well-documented workaround for Jest 30 symbol interference.The comment explaining why a regular function is used instead of
jest.fn()is excellent. The manual call tracking implementation correctly mimics Jest's mock interface while avoiding the internal symbols that breakisExecutableCodeRef.Minor observation: the
callsarray stores empty arrays ([]) for each invocation, which is correct for a zero-argument function. If this mock ever needs to support arguments, the implementation would need adjustment.
38-38: No action required—applyCodeRefSymbolaccepts non-generic invocation.The SDK's
applyCodeRefSymbolfunction is compatible with non-generic calls. The codebase consistently uses it without type parameters across multiple files (coderef-resolver.ts, test files, and test utilities), confirming the current usage at line 38 is correct.frontend/packages/console-dynamic-plugin-sdk/src/coderefs/coderef-resolver.ts (4)
24-26: Good defensive guard against symbol extraction failure.Throwing immediately at module load time ensures fast failure rather than subtle runtime bugs. This is the right approach for a critical invariant.
56-70: Cloning properties prevents mutation of original extension - good change.The switch to
_.cloneDeep(extension.properties)ensures the original extension object remains immutable, which is important for:
- Redux-style state management expectations
- Multiple consumers of the same extension
- Debugging (original remains inspectable)
Note that
cloneDeephas O(n) complexity and creates garbage. For extensions with large property trees (e.g., complex configurations), this could impact performance. However, extensions are typically small, so this is acceptable.
94-98: Shallow spread with cloned properties is correct.The pattern
{ ...extension, properties: clonedProperties }correctly creates a new extension object with:
- All original extension fields preserved (type, flags, etc.)
- Properties replaced with the resolved clone
The
as unknown as Rcast is necessary due to TypeScript's inability to infer the transformation, though it does bypass type safety. This is acceptable given the complex generic constraints.
72-90: Fix error handling inresolveExtension— errors are silently swallowed on first resolution attempt.The core issue is at line 92:
settleAllPromisesreturns[fulfilledValues, rejectedReasons, results], but therejectedReasonsarray is completely ignored. When CodeRefs fail to resolve (lines 85–88), the error is stored on the ref symbol and the promise fulfills (due toreturn e). This means:
- First call: CodeRef fails silently—error is marked but not thrown; extension returns normally
- Second call: Line 73–74 throws if
isCodeRefError(ref)is trueThis asymmetry creates a confusing implicit contract where the caller must either:
- Check each CodeRef for errors manually after resolution, or
- Wrap
resolveExtensioncalls insettleAllPromisesthemselves (asuseResolvedExtensionsdoes at line 21–22)Either refactor
resolveExtensionto throw accumulated errors immediately, or return both the resolved extension and error array so callers have explicit visibility. The test at line 145 is also incomplete—it doesn't exercise the actual error path due to Jest 30's internal symbols on mock functions.
frontend/packages/console-app/src/components/flags/FeatureFlagExtensionLoader.tsx
Show resolved
Hide resolved
|
|
||
| The following shared modules are provided by Console, without plugins providing their own fallback: | ||
|
|
||
| - `@openshift/dynamic-plugin-sdk` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for references to @openshift/dynamic-plugin-sdk in the codebase
rg -n "@openshift/dynamic-plugin-sdk" --type json --type md -A 5 -B 5Repository: openshift/console
Length of output: 5838
🏁 Script executed:
# Check the actual README.md file around lines 128 and 223 to understand context
cat -n frontend/packages/console-dynamic-plugin-sdk/README.md | sed -n '120,140p'
cat -n frontend/packages/console-dynamic-plugin-sdk/README.md | sed -n '215,230p'Repository: openshift/console
Length of output: 1643
🏁 Script executed:
# Look for package.json files that might reference or define this package
fd -t f "package.json" | xargs rg -l "@openshift/dynamic-plugin-sdk" -B 5 -A 5Repository: openshift/console
Length of output: 140
🏁 Script executed:
# Search for any documentation about upstream SDK or lib-core migration
rg -n "upstream|lib-core|migration|SDK" frontend/packages/console-dynamic-plugin-sdk/README.md -A 3 -B 3Repository: openshift/console
Length of output: 3931
🏁 Script executed:
cat -n frontend/packages/console-dynamic-plugin-sdk/README.md | sed -n '66,80p'Repository: openshift/console
Length of output: 1389
🏁 Script executed:
# Also check if there's any additional context around the shared modules configuration
cat -n frontend/packages/console-dynamic-plugin-sdk/README.md | sed -n '115,145p'Repository: openshift/console
Length of output: 1480
Document @openshift/dynamic-plugin-sdk in the SDK package overview.
The package @openshift/dynamic-plugin-sdk is added to shared modules (line 128) but is missing from the "Distributable SDK package overview" table. Plugin developers need clarity on:
- What this package provides (the CHANGELOG hints it's upstream by re-exporting types)
- Its relationship to
@openshift-console/dynamic-plugin-sdk - When plugins should reference it directly vs. the Console SDK packages
Add a table entry explaining this package's purpose and origin. Also update the Console 4.22.X changelog entry (line 223) to include a brief context or cross-reference to the overview section.
🤖 Prompt for AI Agents
In @frontend/packages/console-dynamic-plugin-sdk/README.md at line 128, Add a
new row in the "Distributable SDK package overview" table documenting
`@openshift/dynamic-plugin-sdk`: state that it is an upstream re-exported
types/helpers package (origin), summarize what it exposes (types and re-exports
from upstream dynamic-plugin-sdk), clarify relationship to
`@openshift-console/dynamic-plugin-sdk` (Console SDK wraps/extends it with
Console-specific helpers and should remain the primary import for Console
plugins), and give guidance on when plugin authors should import
`@openshift/dynamic-plugin-sdk` directly versus using Console SDK packages; then
update the Console 4.22.X changelog entry to mention this addition and add a
brief cross-reference to the overview table for details.
|
|
||
| - Upgraded from `react-redux` v7 to v8. Plugins must use `react-redux` v8 to be compatible | ||
| with Console. | ||
| - Added `@openshift/dynamic-plugin-sdk` to shared modules. |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Expand documentation for the upstream SDK addition.
While this documents the addition of @openshift/dynamic-plugin-sdk, it lacks critical context for plugin developers:
- What plugins need to do (if anything) in response to this change
- Whether this is a breaking change requiring updates to plugin code or dependencies
- The relationship to the existing
@openshift-console/dynamic-plugin-sdkpackage - Migration guidance or best practices
Given that the PR objectives describe this as "Phase 2" of a significant infrastructure migration to the upstream OpenShift Dynamic Plugin SDK, this warrants more detailed documentation similar to the react-redux upgrade note on lines 221-222.
Consider expanding this entry with:
- Brief explanation of what the upstream SDK provides
- Any action items for plugin maintainers
- Link to migration documentation if available
🤖 Prompt for AI Agents
In @frontend/packages/console-dynamic-plugin-sdk/README.md at line 223, Add
detailed docs to the README entry that announces adding
@openshift/dynamic-plugin-sdk: explain briefly what the upstream SDK provides,
state whether this is breaking or backwards-compatible for plugins, list any
required actions for plugin maintainers (dependency updates, import changes,
build/config tweaks), clarify the relationship and compatibility with
@openshift-console/dynamic-plugin-sdk, and include migration guidance or a link
to migration docs (use the react-redux upgrade note format on lines ~221-222 as
a template).
| const loadAndEnablePlugin = async ( | ||
| pluginName: string, | ||
| pluginStore: PluginStore, | ||
| onError: (errorMessage: string, errorCause?: unknown) => void = _.noop, | ||
| ) => { | ||
| await pluginStore | ||
| .loadPlugin( | ||
| resolveURL( | ||
| `${window.SERVER_FLAGS.basePath}api/plugins/${pluginName}/`, | ||
| 'plugin-manifest.json', | ||
| ), | ||
| ) | ||
| .catch((err: ErrorWithCause) => { | ||
| // ErrorWithCause isn't the exact type but it's close enough for our use | ||
| onError(`[loadAndEnablePlugin] ${pluginName} loadPlugin failed: ${err.message}`, err.cause); | ||
| }); | ||
|
|
||
| const plugin = pluginStore.getPluginInfo().find((p) => p.manifest.name === pluginName); | ||
|
|
||
| if (plugin?.status === 'failed') { | ||
| onError( | ||
| `[loadAndEnablePlugin] ${pluginName} loading failed: ${plugin.errorMessage}`, | ||
| plugin.errorCause, | ||
| ); | ||
| } else if (plugin?.status === 'loaded') { | ||
| const disablePlugins = ( | ||
| (plugin.manifest as ConsolePluginManifest)?.customProperties?.console?.disableStaticPlugins ?? | ||
| [] | ||
| ).filter((name) => { | ||
| const pluginInfo = pluginStore.getPluginInfo().find((p) => p.manifest.name === name); | ||
|
|
||
| // Ensure dynamic plugins cannot disable other dynamic plugins | ||
| return pluginInfo?.manifest.registrationMethod === 'local'; | ||
| }); | ||
|
|
||
| if (disablePlugins.length > 0) { | ||
| pluginStore.disablePlugins(disablePlugins, `disableStaticPlugins in ${pluginName}`); | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
Async function doesn't await all operations properly.
The loadAndEnablePlugin function is declared async and awaits pluginStore.loadPlugin(), but the calling code in initConsolePlugins (line 110) doesn't await the returned Promise. This means:
- Errors thrown after the
.catch()block (e.g., from theplugin?.status === 'failed'check) won't be caught - Plugin loading order is non-deterministic
- The
onErrorcallback won't capture all failure modes
Additionally, there's a potential issue: if loadPlugin throws synchronously before returning a Promise, it won't be caught by the .catch() chain.
🔧 Suggested fix: wrap the entire function body in try-catch
const loadAndEnablePlugin = async (
pluginName: string,
pluginStore: PluginStore,
onError: (errorMessage: string, errorCause?: unknown) => void = _.noop,
) => {
+ try {
await pluginStore
.loadPlugin(
resolveURL(
`${window.SERVER_FLAGS.basePath}api/plugins/${pluginName}/`,
'plugin-manifest.json',
),
)
.catch((err: ErrorWithCause) => {
- // ErrorWithCause isn't the exact type but it's close enough for our use
onError(`[loadAndEnablePlugin] ${pluginName} loadPlugin failed: ${err.message}`, err.cause);
+ return; // Early return to avoid status check after load failure
});
const plugin = pluginStore.getPluginInfo().find((p) => p.manifest.name === pluginName);
if (plugin?.status === 'failed') {
onError(
`[loadAndEnablePlugin] ${pluginName} loading failed: ${plugin.errorMessage}`,
plugin.errorCause,
);
} else if (plugin?.status === 'loaded') {
// ... rest of the logic
}
+ } catch (err) {
+ onError(`[loadAndEnablePlugin] ${pluginName} unexpected error: ${String(err)}`, err);
+ }
};📝 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 loadAndEnablePlugin = async ( | |
| pluginName: string, | |
| pluginStore: PluginStore, | |
| onError: (errorMessage: string, errorCause?: unknown) => void = _.noop, | |
| ) => { | |
| await pluginStore | |
| .loadPlugin( | |
| resolveURL( | |
| `${window.SERVER_FLAGS.basePath}api/plugins/${pluginName}/`, | |
| 'plugin-manifest.json', | |
| ), | |
| ) | |
| .catch((err: ErrorWithCause) => { | |
| // ErrorWithCause isn't the exact type but it's close enough for our use | |
| onError(`[loadAndEnablePlugin] ${pluginName} loadPlugin failed: ${err.message}`, err.cause); | |
| }); | |
| const plugin = pluginStore.getPluginInfo().find((p) => p.manifest.name === pluginName); | |
| if (plugin?.status === 'failed') { | |
| onError( | |
| `[loadAndEnablePlugin] ${pluginName} loading failed: ${plugin.errorMessage}`, | |
| plugin.errorCause, | |
| ); | |
| } else if (plugin?.status === 'loaded') { | |
| const disablePlugins = ( | |
| (plugin.manifest as ConsolePluginManifest)?.customProperties?.console?.disableStaticPlugins ?? | |
| [] | |
| ).filter((name) => { | |
| const pluginInfo = pluginStore.getPluginInfo().find((p) => p.manifest.name === name); | |
| // Ensure dynamic plugins cannot disable other dynamic plugins | |
| return pluginInfo?.manifest.registrationMethod === 'local'; | |
| }); | |
| if (disablePlugins.length > 0) { | |
| pluginStore.disablePlugins(disablePlugins, `disableStaticPlugins in ${pluginName}`); | |
| } | |
| } | |
| }; | |
| const loadAndEnablePlugin = async ( | |
| pluginName: string, | |
| pluginStore: PluginStore, | |
| onError: (errorMessage: string, errorCause?: unknown) => void = _.noop, | |
| ) => { | |
| try { | |
| await pluginStore | |
| .loadPlugin( | |
| resolveURL( | |
| `${window.SERVER_FLAGS.basePath}api/plugins/${pluginName}/`, | |
| 'plugin-manifest.json', | |
| ), | |
| ) | |
| .catch((err: ErrorWithCause) => { | |
| onError(`[loadAndEnablePlugin] ${pluginName} loadPlugin failed: ${err.message}`, err.cause); | |
| return; // Early return to avoid status check after load failure | |
| }); | |
| const plugin = pluginStore.getPluginInfo().find((p) => p.manifest.name === pluginName); | |
| if (plugin?.status === 'failed') { | |
| onError( | |
| `[loadAndEnablePlugin] ${pluginName} loading failed: ${plugin.errorMessage}`, | |
| plugin.errorCause, | |
| ); | |
| } else if (plugin?.status === 'loaded') { | |
| const disablePlugins = ( | |
| (plugin.manifest as ConsolePluginManifest)?.customProperties?.console?.disableStaticPlugins ?? | |
| [] | |
| ).filter((name) => { | |
| const pluginInfo = pluginStore.getPluginInfo().find((p) => p.manifest.name === name); | |
| // Ensure dynamic plugins cannot disable other dynamic plugins | |
| return pluginInfo?.manifest.registrationMethod === 'local'; | |
| }); | |
| if (disablePlugins.length > 0) { | |
| pluginStore.disablePlugins(disablePlugins, `disableStaticPlugins in ${pluginName}`); | |
| } | |
| } | |
| } catch (err) { | |
| onError(`[loadAndEnablePlugin] ${pluginName} unexpected error: ${String(err)}`, err); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In @frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-init.ts
around lines 18 - 57, The function loadAndEnablePlugin can throw errors outside
the current .catch() (including synchronous throws) and callers like
initConsolePlugins must await it; wrap the entire body of loadAndEnablePlugin in
a try/catch so any thrown error is passed to onError (include err and err.cause
in the message) and return early on failure, and update initConsolePlugins to
await the Promise returned by loadAndEnablePlugin so plugin loading order and
error handling are deterministic; ensure you still call pluginStore.loadPlugin
with await inside the try block and preserve the existing plugin status checks
(plugin?.status === 'failed' / 'loaded') inside the try.
| lazyYaml: () => { | ||
| const sortedExtensions = addActions | ||
| .slice() | ||
| .sort((a, b) => a.properties.id.localeCompare(b.properties.id)); | ||
| const yaml = sortedExtensions | ||
| .map((extension) => { | ||
| const { id, label, description } = extension.properties; | ||
| const labelComment = label.split('\n').join('\n # '); | ||
| const descriptionComment = description.split('\n').join('\n # '); | ||
| return `- # ${labelComment}\n # ${descriptionComment}\n ${id}`; | ||
| }) | ||
| .join('\n'); | ||
| return yaml; | ||
| }, |
There was a problem hiding this comment.
Potential runtime error if label or description are not strings.
The code assumes extension.properties.label and extension.properties.description are strings, calling .split('\n') directly. If an extension provides a non-string value (e.g., a React node or undefined), this will throw.
🐛 Defensive fix
lazyYaml: () => {
const sortedExtensions = addActions
.slice()
.sort((a, b) => a.properties.id.localeCompare(b.properties.id));
const yaml = sortedExtensions
.map((extension) => {
const { id, label, description } = extension.properties;
- const labelComment = label.split('\n').join('\n # ');
- const descriptionComment = description.split('\n').join('\n # ');
+ const labelStr = typeof label === 'string' ? label : String(label ?? '');
+ const descStr = typeof description === 'string' ? description : String(description ?? '');
+ const labelComment = labelStr.split('\n').join('\n # ');
+ const descriptionComment = descStr.split('\n').join('\n # ');
return `- # ${labelComment}\n # ${descriptionComment}\n ${id}`;
})
.join('\n');
return yaml;
},📝 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.
| lazyYaml: () => { | |
| const sortedExtensions = addActions | |
| .slice() | |
| .sort((a, b) => a.properties.id.localeCompare(b.properties.id)); | |
| const yaml = sortedExtensions | |
| .map((extension) => { | |
| const { id, label, description } = extension.properties; | |
| const labelComment = label.split('\n').join('\n # '); | |
| const descriptionComment = description.split('\n').join('\n # '); | |
| return `- # ${labelComment}\n # ${descriptionComment}\n ${id}`; | |
| }) | |
| .join('\n'); | |
| return yaml; | |
| }, | |
| lazyYaml: () => { | |
| const sortedExtensions = addActions | |
| .slice() | |
| .sort((a, b) => a.properties.id.localeCompare(b.properties.id)); | |
| const yaml = sortedExtensions | |
| .map((extension) => { | |
| const { id, label, description } = extension.properties; | |
| const labelStr = typeof label === 'string' ? label : String(label ?? ''); | |
| const descStr = typeof description === 'string' ? description : String(description ?? ''); | |
| const labelComment = labelStr.split('\n').join('\n # '); | |
| const descriptionComment = descStr.split('\n').join('\n # '); | |
| return `- # ${labelComment}\n # ${descriptionComment}\n ${id}`; | |
| }) | |
| .join('\n'); | |
| return yaml; | |
| }, |
🤖 Prompt for AI Agents
In @frontend/packages/console-shared/src/hooks/useResourceSidebarSamples.ts
around lines 257 - 270, The lazyYaml builder assumes extension.properties.label
and .description are strings and will throw if they are undefined or
non-strings; update the lazyYaml implementation (the function referenced as
lazyYaml that maps over addActions and accesses
extension.properties.id/label/description) to coerce or default these values to
strings before calling .split — e.g., guard with typeof checks or use
String(extension.properties.label ?? '') and
String(extension.properties.description ?? '') (also ensure id is present or
default to a safe string) so .split('\n') never runs on a non-string value.
| export const pluginStore = new PluginStore({ | ||
| loaderOptions: { | ||
| sharedScope: getSharedScope(), | ||
| // @ts-expect-error incompatible due to console-specific fetch options | ||
| fetchImpl: consoleFetch, | ||
| // Allows plugins to target a specific version of OpenShift via semver | ||
| fixedPluginDependencyResolutions: { | ||
| // TODO(plugin-sdk): allow a way to bypass this dependency in development, where we don't have this info | ||
| '@console/pluginAPI': | ||
| process.env.NODE_ENV === 'production' | ||
| ? CURRENT_OPENSHIFT_VERSION // this is always provided by console-operator in production | ||
| : CURRENT_OPENSHIFT_VERSION || '4.1337.67', | ||
| }, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's explore the file structure and find consoleFetch definition
find frontend -type f -name "*.ts" -o -name "*.tsx" | head -20Repository: openshift/console
Length of output: 1060
🏁 Script executed:
# Search for consoleFetch definition
rg -A 10 "consoleFetch" frontend/Repository: openshift/console
Length of output: 50373
🏁 Script executed:
# Look for the PluginStore import and check package.json for SDK version
cat -n frontend/package.json | grep -A 5 -B 5 "dynamic-plugin-sdk\|@openshift"Repository: openshift/console
Length of output: 1678
🏁 Script executed:
# Check the imports at the top of plugins.ts to understand the types
head -20 frontend/public/plugins.tsRepository: openshift/console
Length of output: 1029
🏁 Script executed:
# Find consoleFetch implementation
find frontend -path "*/dynamic-plugin-sdk/*" -name "*fetch*" -o -path "*/console-dynamic-plugin-sdk/*" -name "*fetch*" | head -20Repository: openshift/console
Length of output: 283
🏁 Script executed:
# Look for PluginStore definition
rg -A 5 "class PluginStore\|interface PluginStore\|type PluginStore" --max-count=5Repository: openshift/console
Length of output: 43
🏁 Script executed:
# Check the loaderOptions type definition
rg -B 5 -A 10 "loaderOptions" frontend/public/plugins.tsRepository: openshift/console
Length of output: 895
🏁 Script executed:
# Look for any comments or documentation about the fetch incompatibility
rg -B 3 -A 3 "console-specific fetch options|fetchImpl.*incompatible" frontend/Repository: openshift/console
Length of output: 567
🏁 Script executed:
# Examine consoleFetch implementation
cat -n frontend/packages/console-dynamic-plugin-sdk/src/utils/fetch/console-fetch.tsRepository: openshift/console
Length of output: 7970
🏁 Script executed:
# Check console-fetch-utils for any clues
cat -n frontend/packages/console-dynamic-plugin-sdk/src/utils/fetch/console-fetch-utils.tsRepository: openshift/console
Length of output: 3728
🏁 Script executed:
# Look for PluginStore in node_modules or type definitions
rg "loaderOptions\|fetchImpl" frontend/packages/console-dynamic-plugin-sdk --max-count=10 -B 3 -A 3Repository: openshift/console
Length of output: 43
🏁 Script executed:
# Search for any recent commits or issues mentioning this type incompatibility
rg -i "fetchImpl\|loader.*fetch\|plugin.*fetch" frontend/ --type=ts --type=tsx -l | head -5Repository: openshift/console
Length of output: 88
🏁 Script executed:
# Find the ConsoleFetch type definition
rg -A 5 "type ConsoleFetch\|interface ConsoleFetch" frontend/packages/console-dynamic-plugin-sdk/Repository: openshift/console
Length of output: 43
🏁 Script executed:
# Check the console-types file referenced in the import
find frontend -name "console-types*" -type fRepository: openshift/console
Length of output: 137
🏁 Script executed:
# Look at the PluginStore from the dynamic-plugin-sdk package
rg "PluginStore.*loaderOptions\|fetchImpl" frontend/ -B 5 -A 15 --max-count=3Repository: openshift/console
Length of output: 43
🏁 Script executed:
# Check if there's any documentation or changelog about this
cat -n frontend/packages/console-dynamic-plugin-sdk/CHANGELOG-core.md | head -100Repository: openshift/console
Length of output: 6732
🏁 Script executed:
# Look at the console-types.ts file to see the ConsoleFetch type definition
cat -n frontend/packages/console-dynamic-plugin-sdk/src/extensions/console-types.ts | head -80Repository: openshift/console
Length of output: 2671
🏁 Script executed:
# Check if PluginStore is from dynamic-plugin-sdk package or defined locally
rg "from.*PluginStore\|import.*PluginStore" frontend/public/plugins.tsRepository: openshift/console
Length of output: 43
🏁 Script executed:
# See if there are any comments in the git log about this type suppression
git log --oneline -n 50 -- frontend/public/plugins.tsRepository: openshift/console
Length of output: 120
🏁 Script executed:
# Search for ConsoleFetch type definition
rg "type ConsoleFetch|interface ConsoleFetch" frontend/packages/console-dynamic-plugin-sdk/Repository: openshift/console
Length of output: 453
🏁 Script executed:
# Check the actual import from @openshift/dynamic-plugin-sdk to see what PluginStore expects
rg -A 3 "from '@openshift/dynamic-plugin-sdk'" frontend/public/plugins.tsRepository: openshift/console
Length of output: 458
🏁 Script executed:
# Let's search more broadly for the ConsoleFetch type signature
rg "ConsoleFetch\s*=" frontend/packages/console-dynamic-plugin-sdk/src/ -B 2 -A 5Repository: openshift/console
Length of output: 1684
🏁 Script executed:
# Check if there's a type or interface definition in the extensions
grep -r "ConsoleFetch" frontend/packages/console-dynamic-plugin-sdk/src/extensions/ --include="*.ts"Repository: openshift/console
Length of output: 453
🏁 Script executed:
# Get the full ConsoleFetch type definition
rg -B 2 -A 5 "export type ConsoleFetch" frontend/packages/console-dynamic-plugin-sdk/src/extensions/console-types.tsRepository: openshift/console
Length of output: 831
🏁 Script executed:
# Try to find PluginStore type hints or documentation in the repo
rg "PluginStore" frontend/ -B 3 -A 10 --max-count=5 | head -100Repository: openshift/console
Length of output: 7372
🏁 Script executed:
# Check if there's any type definition for loaderOptions in the codebase
rg "loaderOptions.*:" frontend/ --max-count=10Repository: openshift/console
Length of output: 256
🏁 Script executed:
# Let's look at the web search result to understand PluginStore's expected fetchImpl type
# First, let me check if there are any node_modules or type stubs that define PluginStore
find frontend -path "*/node_modules/@openshift/dynamic-plugin-sdk*" -name "*.d.ts" | head -10Repository: openshift/console
Length of output: 43
🏁 Script executed:
# Look in the SDK package documentation or README
find frontend/packages/console-dynamic-plugin-sdk -name "README*" -o -name "*.md" | head -5Repository: openshift/console
Length of output: 379
🏁 Script executed:
# Check if there are any test files that show how PluginStore is used with loaderOptions
rg -A 10 "new PluginStore" frontend/ --max-count=3Repository: openshift/console
Length of output: 1996
🏁 Script executed:
# Let me verify the exact line numbers and context of the @ts-expect-error comment
sed -n '20,35p' frontend/public/plugins.tsRepository: openshift/console
Length of output: 733
Consider expanding the comment to clarify the specific type incompatibility.
The @ts-expect-error suppression on line 23 is necessary because consoleFetch has a third timeout parameter not present in the standard fetch API signature that PluginStore.loaderOptions.fetchImpl expects. The current comment ("incompatible due to console-specific fetch options") is accurate but could be more explicit.
Options:
- Expand the inline comment to specify:
// @ts-expect-error: consoleFetch's timeout parameter doesn't exist in standard fetch - Create a lightweight wrapper:
const fetchImpl = (url: string, options?: RequestInit) => consoleFetch(url, options, 60000);to strip the timeout while preserving runtime behavior
Either approach will help future maintainers understand the exact nature of the incompatibility.
🤖 Prompt for AI Agents
In @frontend/public/plugins.ts around lines 20 - 34, The inline @ts-expect-error
on PluginStore.loaderOptions.fetchImpl hides a specific signature mismatch:
consoleFetch accepts a third timeout parameter not present on the standard fetch
type; either make the comment explicit (e.g., “// @ts-expect-error:
consoleFetch's timeout parameter doesn't exist in standard fetch”) or replace
the suppression by adding a lightweight wrapper that matches the expected
signature and delegates to consoleFetch (e.g., create fetchImpl that accepts
(url, options) and calls consoleFetch(url, options, defaultTimeout)) and use
that wrapper for loaderOptions.fetchImpl to remove the type mismatch while
preserving runtime behavior.
f50b47b to
58592b6
Compare
|
@logonoff: This pull request references CONSOLE-3769 which is a valid jira issue. 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
🤖 Fix all issues with AI agents
In `@frontend/packages/topology/src/data-transforms/DataModelProvider.tsx`:
- Around line 62-66: Update the console error message in DataModelProvider so it
uses the term "extension" instead of "plugin"; specifically change the log that
references factory.properties.id and factory.uid to say "extension" (since
factory.uid comes from ResolvedExtension.uid) so terminology matches
DataModelExtension.tsx and other code paths.
♻️ Duplicate comments (24)
frontend/packages/console-shared/locales/en/console-shared.json (1)
298-298: Typo correction looks good, but cross-locale sync is still pending.The fix from
"Set maxUnavaliable to 0"→"Set maxUnavailable to 0"is correct. However, as previously noted, the other locale files (zh,ko,ja,fr,es) underfrontend/packages/console-shared/locales/*/console-shared.jsonstill reference the old misspelled key. Until those are updated, non-English users will see missing translations for this string.#!/bin/bash # Verify which locale files still have the old misspelled key echo "=== Locale files with old misspelled key 'maxUnavaliable' ===" rg -l 'maxUnavaliable' frontend/packages/console-shared/locales/frontend/packages/console-dynamic-plugin-sdk/CHANGELOG-core.md (1)
21-21: Breaking change entry must specify the replacement forpluginID.The changelog documents removal of
pluginIDbut omits the migration path. Plugin authors scanning breaking changes need immediate guidance. Update to:- **Breaking**: Removed `pluginID` from the result in `useResolvedExtensions` hook; use `uid` instead ([CONSOLE-3769], [`#15904`])frontend/packages/console-dynamic-plugin-sdk/src/coderefs/coderef-resolver.ts (1)
16-26: Symbol extraction is properly guarded, but document the SDK version dependency.The fail-fast guard at lines 24-26 correctly prevents silent failures if
codeRefSymbolextraction fails. However, this approach relies on SDK internals—applyCodeRefSymbolattaching exactly one symbol with valuetrue.Per past review discussion, add a brief note about the SDK version this was validated against so maintainers know to re-verify after SDK upgrades:
📝 Suggested documentation addition
/** * Extract the SDK's internal CodeRef symbol by applying it to a dummy function. * * This ensures we can detect code refs created by the SDK, which uses its own * private Symbol instance. + * + * NOTE: Validated against `@openshift/dynamic-plugin-sdk` ^6.0.0. Re-verify symbol + * extraction behavior after SDK version upgrades. */ const codeRefSymbol = Object.getOwnPropertySymbols(applyCodeRefSymbol(() => Promise.resolve()))[0];frontend/packages/console-plugin-sdk/src/utils/allowed-plugins.ts (1)
4-17: Guard against undefinedSERVER_FLAGS.consolePluginsat module load time.This function executes immediately at module load (line 25). If
window.SERVER_FLAGSor.consolePluginsisn't initialized yet, this throws. Other callsites in the codebase defensively checkArray.isArray(window.SERVER_FLAGS.consolePlugins)before access.Suggested fix
const getEnabledDynamicPluginNames = () => { - const allPluginNames = window.SERVER_FLAGS.consolePlugins; + const allPluginNames = window.SERVER_FLAGS?.consolePlugins ?? []; const disabledPlugins = getURLSearchParams()['disable-plugins'];frontend/packages/console-plugin-sdk/src/api/useExtensions.ts (2)
76-94: Mutating refs insideuseMemoviolates React's purity contract.The
useMemocallback must be pure—React may invoke it multiple times, discard results during concurrent rendering, or skip it entirely. MutatingpreviousResultRefandpreviousUIDsRefinside the memo body can produce stale or inconsistent state.Recommended refactor: move cache comparison outside useMemo
- // Track the previous result and UIDs for referential stability - const previousResultRef = useRef<LoadedExtension<E>[]>([]); - const previousUIDsRef = useRef<string>(''); + // Track the previous result for referential stability + const cacheRef = useRef<{ uids: string; result: LoadedExtension<E>[] }>({ + uids: '', + result: [], + }); - return useMemo(() => { - const sorted = sortExtensionsByPluginOrder(translatedExtensions); - const currentUIDs = sorted.map((e) => e.uid).join(','); + const sorted = useMemo( + () => sortExtensionsByPluginOrder(translatedExtensions), + [translatedExtensions], + ); + const currentUIDs = useMemo(() => sorted.map((e) => e.uid).join(','), [sorted]); - // Return previous result if the extensions haven't changed - if (currentUIDs === previousUIDsRef.current) { - return previousResultRef.current; - } - - // Update refs and return new result - previousResultRef.current = sorted; - previousUIDsRef.current = currentUIDs; - - return sorted; - }, [translatedExtensions]); + // Update cache outside of useMemo to maintain purity + if (currentUIDs !== cacheRef.current.uids) { + cacheRef.current = { uids: currentUIDs, result: sorted }; + } + + return cacheRef.current.result; };
80-87: UID-only comparison may miss translation updates.If
useTranslatedExtensionsmutates extension objects in place (e.g., on language change) without changing UIDs, thecurrentUIDs === previousUIDsRef.currentcheck returns the stale cached result. Consider including translation state (e.g., current locale) in the cache key, or ensuringuseTranslatedExtensionsreturns a new array reference when translations change.#!/bin/bash # Check how useTranslatedExtensions handles translation updates cat -n frontend/packages/console-plugin-sdk/src/utils/useTranslatedExtensions.tsfrontend/packages/console-shared/src/hooks/useResourceSidebarSamples.ts (7)
93-100: Performance:useDefaultSamplesrebuilds ImmutableMap on every render.This hook creates a new
ImmutableMapwith all sample definitions on each invocation. Since the resolved extensions and translation function are stable across renders (unless extensions change), wrap the map construction inuseMemoto avoid unnecessary allocations and downstream re-renders.⚡ Memoize the ImmutableMap construction
+import { useMemo } from 'react'; + const useDefaultSamples = () => { const { t } = useTranslation('console-shared'); const [addActions] = useResolvedExtensions<AddAction>(isAddAction); const [catalogItemTypes] = useResolvedExtensions<CatalogItemType>(isCatalogItemType); const [perspectives] = useResolvedExtensions<Perspective>(isPerspective); const clusterRoleBindingSamples = useClusterRoleBindingSamples(); - return ImmutableMap<GroupVersionKind, Sample[]>() + return useMemo(() => ImmutableMap<GroupVersionKind, Sample[]>() .setIn( [referenceForModel(BuildConfigModel)], // ... rest of chain ) - ); + , [t, addActions, catalogItemTypes, perspectives, clusterRoleBindingSamples]); };
68-91: ClusterRole samples missingyamlcontent.These samples define neither
yamlnorlazyYaml, butResourceSidebarSampleconsumers pass the yaml value directly toloadSampleYamlanddownloadSampleYamlhandlers. When users click "Try it" or "Download YAML", they'll receiveundefinedcontent.Either provide placeholder YAML manifests for these samples or guard the action buttons in the consumer component.
257-270: Defensive check needed:labelanddescriptionmay not be strings.The
AddActionextension type defineslabelanddescriptionasstring, but extensions from third-party plugins could provide unexpected values. Calling.split('\n')onundefinedor a non-string will throw at runtime.🛡️ Add defensive string coercion
lazyYaml: () => { const sortedExtensions = addActions .slice() .sort((a, b) => a.properties.id.localeCompare(b.properties.id)); const yaml = sortedExtensions .map((extension) => { const { id, label, description } = extension.properties; - const labelComment = label.split('\n').join('\n # '); - const descriptionComment = description.split('\n').join('\n # '); + const labelComment = (label ?? '').split('\n').join('\n # '); + const descriptionComment = (description ?? '').split('\n').join('\n # '); return `- # ${labelComment}\n # ${descriptionComment}\n ${id}`; }) .join('\n'); return yaml; },
345-350: Type signature mismatch:kindObjis required but treated as optional.The parameter
kindObj: K8sKindis typed as required, but the early return at line 348-350 treats it as potentially falsy. Either update the signature tokindObj?: K8sKindor document that callers must handle this upstream.🔧 Make parameter explicitly optional
-export const useResourceSidebarSamples = (kindObj: K8sKind, yamlSamplesList: FirehoseResult) => { +export const useResourceSidebarSamples = (kindObj: K8sKind | undefined, yamlSamplesList: FirehoseResult) => {
352-359:_.isEmptyon FirehoseResult is misleading; checkloaded/loadErrorinstead.
_.isEmpty(yamlSamplesList)checks if the wrapper object itself is empty, not whether the data has successfully loaded. This could lead to filtering on partially loaded or errored data. Prefer explicit checks:🐛 Proper FirehoseResult gating
- const yamlSamplesData = !_.isEmpty(yamlSamplesList) - ? _.filter( + const yamlSamplesData = + yamlSamplesList?.loaded && !yamlSamplesList?.loadError && Array.isArray(yamlSamplesList.data) + ? _.filter( yamlSamplesList.data, (sample: K8sResourceKind) => sample.spec.targetResource.apiVersion === apiVersionForModel(kindObj) && sample.spec.targetResource.kind === kindObj.kind, ) - : []; + : [];
362-369: UseOmitinstead ofExcludefor object property exclusion.
Exclude<T, U>is a distributive conditional type for union members; it doesn't remove properties from object types. For omitting theidproperty fromSample, useOmit<Sample, 'id'>.🔧 Correct type utility
const extensionSamples = !_.isEmpty(yamlSamplesData) ? yamlSamplesData.map((sample: K8sResourceKind) => { return { id: sample.metadata.uid, - ...(sample.spec as Exclude<Sample, 'id'>), + ...(sample.spec as Omit<Sample, 'id'>), }; }) : [];
373-375: Comment is misleading: snippets and samples are disjoint, not a superset.The filtering logic produces mutually exclusive sets—
snippetscontains items withsample.snippet === true, whilesamplescontains the remainder. The comment suggests one is a superset of the other, which contradicts the implementation.📝 Suggested comment correction
- // For the time being, `snippets` are a superset of `samples` + // Partition: `snippets` are insertable code fragments (snippet: true), `samples` are full resource templates const snippets = allSamples.filter((sample: Sample) => sample.snippet); const samples = allSamples.filter((sample: Sample) => !sample.snippet);frontend/packages/console-plugin-sdk/src/codegen/local-plugins.ts (1)
55-65: String interpolation escaping concern remains unaddressed.As flagged in previous reviews,
pkg.nameandpkg.versionare directly interpolated into generated JavaScript without escaping. While npm package names follow a restricted character set, usingJSON.stringifyprovides cheap defensive hardening against edge cases or future format changes.Suggested fix using JSON.stringify
localPlugins.push({ - name: '${pkg.name}', - version: '${pkg.version}', + name: ${JSON.stringify(pkg.name)}, + version: ${JSON.stringify(pkg.version)}, extensions: ${extensionHook(pkg)}, registrationMethod: 'local', });frontend/packages/console-app/src/components/flags/FeatureFlagExtensionLoader.tsx (3)
14-18: Import these types from the public SDK entry point instead of the internal path.This imports
Extension,ExtensionTypeGuard, andResolvedExtensionfrom@console/dynamic-plugin-sdk/src/types, which is an internal path. These types are re-exported from the public API at@console/dynamic-plugin-sdk. Using internal paths creates fragility—SDK restructuring could break these imports without a major version bump.♻️ Proposed fix
-import type { - Extension, - ExtensionTypeGuard, - ResolvedExtension, -} from '@console/dynamic-plugin-sdk/src/types'; +import type { + Extension, + ExtensionTypeGuard, + ResolvedExtension, +} from '@console/dynamic-plugin-sdk';
59-75: Useuid-based comparison instead of object reference equality inuseResolvedExtensionsDiff.
lodash.differenceuses reference equality (===), which fails whenresolveExtensionclones extension properties. Even if an extension is logically unchanged, its new cloned object won't match the previous reference, causingdifference()to incorrectly report it as both removed and added. This can trigger handlers multiple times for the same extension.Since extensions have a
uidproperty, compare by uid instead:🐛 Proposed fix using uid-based comparison
const useResolvedExtensionsDiff = <E extends Extension>( typeGuard: ExtensionTypeGuard<E>, onChange: (added: ResolvedExtension<E>[], removed: ResolvedExtension<E>[]) => void, ) => { const [extensions] = useResolvedExtensions<E>(typeGuard); const prevExtensionsRef = useRef<ResolvedExtension<E>[]>([]); useEffect(() => { - const added = difference(extensions, prevExtensionsRef.current); - const removed = difference(prevExtensionsRef.current, extensions); + const currentUids = new Set(extensions.map((e) => e.uid)); + const prevUids = new Set(prevExtensionsRef.current.map((e) => e.uid)); + + const added = extensions.filter((e) => !prevUids.has(e.uid)); + const removed = prevExtensionsRef.current.filter((e) => !currentUids.has(e.uid)); if (added.length > 0 || removed.length > 0) { onChange(added, removed); prevExtensionsRef.current = extensions; } }, [extensions, onChange]); };
66-74: EnsureprevExtensionsRefis updated even ifonChangethrows.If
onChangethrows an exception,prevExtensionsRef.currentwon't be updated, leaving the old "prev" in place. This can cause repeated retries every render as the same diff keeps being detected.🔧 Proposed hardening
if (added.length > 0 || removed.length > 0) { - onChange(added, removed); - prevExtensionsRef.current = extensions; + try { + onChange(added, removed); + } finally { + prevExtensionsRef.current = extensions; + } }frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-init.ts (3)
78-89: Add a guard for the SDK callback before invocation.Line 88 calls
window[sdkCallbackName](patchedPluginName, entryModule)without verifying the SDK callback exists. IfPluginStorefails to register__load_plugin_entry__or there's an unexpected initialization sequence, legacy plugins will throw an unhandled TypeError.🐛 Proposed defensive check
// eslint-disable-next-line no-console console.warn( `[DEPRECATION WARNING] ${pluginName} was built for an older version of Console and may not work correctly in this version.`, ); + if (typeof window[sdkCallbackName] !== 'function') { + // eslint-disable-next-line no-console + console.error( + `SDK entry callback "${sdkCallbackName}" is not initialized. ` + + `Ensure PluginStore is fully initialized before plugin scripts are loaded.`, + ); + return; + } + window[sdkCallbackName](patchedPluginName, entryModule);
109-115: Unhandled promise rejections in plugin loading loop.
loadAndEnablePluginis async butforEach(line 109) doesn't await the returned promises. This creates fire-and-forget behavior where:
- Errors thrown after the
.catch()block won't be caught- There's no way to know when all plugins have finished loading
- Plugin load order is non-deterministic
If parallel loading is intentional, use
Promise.allSettled()to maintain parallelism while properly handling all results.🐛 Proposed fix with Promise.allSettled
- dynamicPluginNames.forEach((pluginName) => { - loadAndEnablePlugin(pluginName, pluginStore, (errorMessage, errorCause) => { + Promise.allSettled( + dynamicPluginNames.map((pluginName) => + loadAndEnablePlugin(pluginName, pluginStore, (errorMessage, errorCause) => { - // eslint-disable-next-line no-console - console.error(..._.compact([errorMessage, errorCause])); - window.windowError = `${window.windowError ?? ''};${errorMessage}: ${String(errorCause)}`; - }); - }); + // eslint-disable-next-line no-console + console.error(..._.compact([errorMessage, errorCause])); + const errorEntry = `${errorMessage}: ${String(errorCause)}`; + window.windowError = window.windowError ? `${window.windowError};${errorEntry}` : errorEntry; + }), + ), + );
113-113: Leading semicolon in error string whenwindow.windowErroris initially undefined.When
window.windowErroris initially undefined, the concatenation${window.windowError ?? ''};${errorMessage}produces;errorMessage: causewith a leading semicolon. This is cosmetic but may affect error parsing in Cypress tests or tooling that consumeswindow.windowError.🔧 Proposed fix
- window.windowError = `${window.windowError ?? ''};${errorMessage}: ${String(errorCause)}`; + const errorEntry = `${errorMessage}: ${String(errorCause)}`; + window.windowError = window.windowError ? `${window.windowError};${errorEntry}` : errorEntry;frontend/public/plugins.ts (4)
20-22:getSharedScope()called at module evaluation time risks startup failure.Per
plugin-shared-modules.ts,getSharedScope()throws if__webpack_share_scopes__[SHARED_SCOPE_NAME]isn't initialized yet. This module-levelnew PluginStore(...)call creates a fragile dependency on webpack initialization order.If you can't guarantee the ordering, consider lazy initialization:
Lazy initialization pattern
let _pluginStore: PluginStore | null = null; export const getPluginStore = (): PluginStore => { if (!_pluginStore) { _pluginStore = new PluginStore({ loaderOptions: { sharedScope: getSharedScope(), // ... }, }); // load local plugins here } return _pluginStore; };Alternatively, verify through build configuration that the share scope is always initialized before this module loads.
10-11: Production may receivenullforCURRENT_OPENSHIFT_VERSION.
semver.valid()returnsnullwhen parsing fails. While development has the|| '4.1337.67'fallback at line 31, production at line 30 usesCURRENT_OPENSHIFT_VERSIONdirectly. Ifconsole-operatorever provides a malformed version string, plugin dependency resolution will silently receivenull.Consider adding a guard with a warning in production:
Suggested defensive handling
-const CURRENT_OPENSHIFT_VERSION = semver(window.SERVER_FLAGS.releaseVersion); +const CURRENT_OPENSHIFT_VERSION = semver(window.SERVER_FLAGS.releaseVersion); + +if (process.env.NODE_ENV === 'production' && !CURRENT_OPENSHIFT_VERSION) { + // eslint-disable-next-line no-console + console.warn( + 'Invalid releaseVersion from console-operator; plugin dependency resolution may fail:', + window.SERVER_FLAGS.releaseVersion, + ); +}
43-43: Unhandled promise rejections from asyncloadPlugin.
pluginStore.loadPlugin()is async (confirmed by usage patterns inplugin-init.tswithawaitand.catch()). The currentforEachcreates fire-and-forget promises—if a local plugin fails to load, the rejection is silently swallowed.Recommended fix using Promise.allSettled
-localPlugins.forEach((plugin) => pluginStore.loadPlugin(plugin)); +void Promise.allSettled( + localPlugins.map((plugin) => pluginStore.loadPlugin(plugin)), +).then((results) => { + const failures = results.filter((r) => r.status === 'rejected'); + if (failures.length) { + // eslint-disable-next-line no-console + console.error('Failed to load local plugins:', failures); + } +});
71-71: Log label inconsistency: "Static plugins" → "Local plugins".The PR explicitly renames "active plugins" to "local plugins" per upstream nomenclature. This log statement should match to avoid confusion during debugging.
Proposed fix
- console.info(`Static plugins: [${localPlugins.map((p) => p.name).join(', ')}]`); + console.info(`Local plugins: [${localPlugins.map((p) => p.name).join(', ')}]`);
🧹 Nitpick comments (4)
frontend/packages/console-shared/src/test-utils/unit-test-utils.tsx (1)
152-156: Pre-existing issue: async IIFE is never awaited.This isn't introduced by this PR, but worth noting for a follow-up: the async arrow function on lines 153-156 is created but never invoked or awaited, so the
fireEvent.changeand subsequentwaitForassertion will never execute whentestValueis provided.🔧 Suggested fix
if (testValue !== undefined) { - await (async () => { - fireEvent.change(input, { target: { value: testValue } }); - await waitFor(() => expect(input).toHaveValue(testValue)); - }); + fireEvent.change(input, { target: { value: testValue } }); + await waitFor(() => expect(input).toHaveValue(testValue)); }frontend/packages/console-dynamic-plugin-sdk/src/coderefs/__tests__/coderef-resolver.spec.ts (1)
145-171: Test name still misleading—consider renaming or removing.Per the extensive inline comments, this test does not actually exercise the "continuously reject code refs which have failed to resolve" path. The Jest 30 mock symbols cause
isExecutableCodeRefto returnfalse, sodeepForOwnnever finds the CodeRef anderrorCodeRefis never called.The test currently only validates that
resolveExtensiongracefully handles extensions with no recognized CodeRefs. If that's the intent, rename to something like"handles extensions with unrecognized properties gracefully". Otherwise, consider usinggetExecutableCodeRefMockwith a rejection:const errorCodeRef = getExecutableCodeRefMock(Promise.reject(new Error('test error')));This would exercise the actual error-handling codepath.
frontend/public/components/edit-yaml.tsx (1)
469-472: Consider adding a defensive null check forobjModel.While the current call flow ensures
validate()runs beforeupdateYAML()(which checks for model existence), passing a potentially nullobjModeltok8sCreate/k8sUpdatecould cause runtime errors if the call flow changes in the future.For defensive coding, consider adding an early return or assertion:
🛡️ Optional defensive check
const updateYAML = useCallback( (obj) => { setSuccess(null); setErrors(null); const objModel = getModel(obj); + if (!objModel) { + handleError(t('public~Could not determine resource model.')); + return; + } const response = create ? k8sCreate(objModel, _.omit(obj, ['metadata.resourceVersion'])) : k8sUpdate(objModel, obj, obj.metadata.namespace, obj.metadata.name);frontend/packages/console-plugin-sdk/src/api/usePluginInfo.ts (1)
24-49: Clean refactor using SDK hook with proper referential stability.The migration to
usePluginInfoSDKwith filtering for dynamic plugins (excludingregistrationMethod: 'local') is well-implemented. TheisEqualcomparison ensures referential stability only when data is truly unchanged.One minor performance note: the
find()lookup withinmap()(lines 37-38) is O(n²). For typical plugin counts this is negligible, but if plugin lists grow large, consider using a Map keyed bymanifest.namefor O(n) lookup.♻️ Optional optimization for large plugin lists
return useMemo(() => { const dynamicPluginInfo = pluginInfo.filter( (plugin) => plugin.manifest.registrationMethod !== 'local', ); + const prevByName = new Map( + previousResultRef.current.map((p) => [p.manifest.name, p]), + ); + // Ensure referential stability of the result elements const stablePluginInfo = dynamicPluginInfo.map((plugin) => { - const previousPlugin = previousResultRef.current.find( - (p) => p.manifest.name === plugin.manifest.name, - ); + const previousPlugin = prevByName.get(plugin.manifest.name); // Only reuse previous reference if the data is actually equal if (previousPlugin && isEqual(previousPlugin, plugin)) { return previousPlugin; } return plugin; });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
frontend/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (68)
frontend/__mocks__/webpack.tsfrontend/package.jsonfrontend/packages/console-app/src/__tests__/hooks/useCSPViolationDetector.spec.tsxfrontend/packages/console-app/src/__tests__/plugin-test-utils.tsfrontend/packages/console-app/src/components/dashboards-page/dynamic-plugins-health-resource/NotLoadedDynamicPlugins.tsxfrontend/packages/console-app/src/components/file-upload/__tests__/file-upload-utils.spec.tsfrontend/packages/console-app/src/components/flags/FeatureFlagExtensionLoader.tsxfrontend/packages/console-app/src/components/nodes/useNodeStatusExtensions.tsxfrontend/packages/console-app/src/components/user-preferences/__tests__/userPreferences.data.tsxfrontend/packages/console-app/src/components/user-preferences/perspective/__tests__/perspective.data.tsfrontend/packages/console-app/src/hooks/useCSPViolationDetector.tsxfrontend/packages/console-dynamic-plugin-sdk/CHANGELOG-core.mdfrontend/packages/console-dynamic-plugin-sdk/CHANGELOG-webpack.mdfrontend/packages/console-dynamic-plugin-sdk/README.mdfrontend/packages/console-dynamic-plugin-sdk/package.jsonfrontend/packages/console-dynamic-plugin-sdk/scripts/package-definitions.tsfrontend/packages/console-dynamic-plugin-sdk/src/app/AppInitSDK.tsxfrontend/packages/console-dynamic-plugin-sdk/src/coderefs/__tests__/coderef-resolver.spec.tsfrontend/packages/console-dynamic-plugin-sdk/src/coderefs/coderef-resolver.tsfrontend/packages/console-dynamic-plugin-sdk/src/perspective/__tests__/perspective.data.tsfrontend/packages/console-dynamic-plugin-sdk/src/runtime/__tests__/plugin-manifest.spec.tsfrontend/packages/console-dynamic-plugin-sdk/src/runtime/__tests__/plugin-utils.spec.tsfrontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-dependencies.tsfrontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-init.tsfrontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-loader.tsfrontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-manifest.tsfrontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-utils.tsfrontend/packages/console-dynamic-plugin-sdk/src/shared-modules/shared-modules-meta.tsfrontend/packages/console-dynamic-plugin-sdk/src/types.tsfrontend/packages/console-dynamic-plugin-sdk/src/utils/test-utils.tsfrontend/packages/console-dynamic-plugin-sdk/src/webpack/ConsoleRemotePlugin.tsfrontend/packages/console-plugin-sdk/package.jsonfrontend/packages/console-plugin-sdk/src/api/pluginSubscriptionService.tsfrontend/packages/console-plugin-sdk/src/api/useExtensions.tsfrontend/packages/console-plugin-sdk/src/api/usePluginInfo.tsfrontend/packages/console-plugin-sdk/src/api/usePluginStore.tsfrontend/packages/console-plugin-sdk/src/codegen/local-plugins.tsfrontend/packages/console-plugin-sdk/src/store.tsfrontend/packages/console-plugin-sdk/src/utils/allowed-plugins.tsfrontend/packages/console-shared/locales/en/console-shared.jsonfrontend/packages/console-shared/src/components/actions/__tests__/utils-test-data.tsfrontend/packages/console-shared/src/components/catalog/__tests__/CatalogController.spec.tsxfrontend/packages/console-shared/src/components/editor/CodeEditorSidebar.tsxfrontend/packages/console-shared/src/components/formik-fields/CodeEditorField.tsxfrontend/packages/console-shared/src/hooks/__tests__/useTelemetry.spec.tsfrontend/packages/console-shared/src/hooks/useResourceSidebarSamples.tsfrontend/packages/console-shared/src/test-utils/unit-test-utils.tsxfrontend/packages/console-shared/src/utils/index.tsfrontend/packages/console-shared/src/utils/sample-utils.tsfrontend/packages/dev-console/src/components/add/__tests__/add-page-test-data.tsxfrontend/packages/dev-console/src/components/deployments/__tests__/DeploymentForm.spec.tsxfrontend/packages/dev-console/src/utils/__tests__/useAddActionExtensions.spec.tsfrontend/packages/knative-plugin/src/components/add/__tests__/EventSource.spec.tsxfrontend/packages/operator-lifecycle-manager/src/components/operand/__tests__/index.spec.tsxfrontend/packages/topology/src/__tests__/TopologyPage.spec.tsxfrontend/packages/topology/src/data-transforms/DataModelExtension.tsxfrontend/packages/topology/src/data-transforms/DataModelProvider.tsxfrontend/packages/webterminal-plugin/src/components/cloud-shell/__tests__/MultiTabbedTerminal.spec.tsxfrontend/public/actions/features.tsfrontend/public/components/app.tsxfrontend/public/components/dashboard/project-dashboard/getting-started/__tests__/SampleGettingStartedCard.data.tsfrontend/public/components/edit-yaml.tsxfrontend/public/components/factory/modal.tsxfrontend/public/components/notification-drawer.tsxfrontend/public/components/sidebars/resource-sidebar-samples.tsxfrontend/public/components/sidebars/resource-sidebar.tsxfrontend/public/components/utils/__tests__/nav.spec.tsfrontend/public/plugins.ts
💤 Files with no reviewable changes (25)
- frontend/packages/console-shared/src/hooks/tests/useTelemetry.spec.ts
- frontend/packages/console-shared/src/components/catalog/tests/CatalogController.spec.tsx
- frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-manifest.ts
- frontend/packages/console-shared/src/components/actions/tests/utils-test-data.ts
- frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-utils.ts
- frontend/packages/console-dynamic-plugin-sdk/src/runtime/tests/plugin-utils.spec.ts
- frontend/public/components/utils/tests/nav.spec.ts
- frontend/packages/console-dynamic-plugin-sdk/src/webpack/ConsoleRemotePlugin.ts
- frontend/packages/console-dynamic-plugin-sdk/src/perspective/tests/perspective.data.ts
- frontend/packages/console-shared/src/utils/index.ts
- frontend/packages/console-app/src/components/user-preferences/tests/userPreferences.data.tsx
- frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-dependencies.ts
- frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-loader.ts
- frontend/packages/console-plugin-sdk/package.json
- frontend/packages/console-dynamic-plugin-sdk/src/runtime/tests/plugin-manifest.spec.ts
- frontend/packages/console-plugin-sdk/src/api/usePluginStore.ts
- frontend/packages/console-shared/src/utils/sample-utils.ts
- frontend/packages/console-plugin-sdk/src/store.ts
- frontend/packages/console-plugin-sdk/src/api/pluginSubscriptionService.ts
- frontend/packages/console-app/src/components/file-upload/tests/file-upload-utils.spec.ts
- frontend/packages/console-dynamic-plugin-sdk/package.json
- frontend/packages/dev-console/src/components/add/tests/add-page-test-data.tsx
- frontend/packages/dev-console/src/utils/tests/useAddActionExtensions.spec.ts
- frontend/packages/console-app/src/components/user-preferences/perspective/tests/perspective.data.ts
- frontend/public/components/dashboard/project-dashboard/getting-started/tests/SampleGettingStartedCard.data.ts
🚧 Files skipped from review as they are similar to previous changes (17)
- frontend/packages/console-app/src/components/nodes/useNodeStatusExtensions.tsx
- frontend/packages/console-dynamic-plugin-sdk/CHANGELOG-webpack.md
- frontend/mocks/webpack.ts
- frontend/packages/console-app/src/hooks/useCSPViolationDetector.tsx
- frontend/public/components/app.tsx
- frontend/public/components/sidebars/resource-sidebar-samples.tsx
- frontend/packages/dev-console/src/components/deployments/tests/DeploymentForm.spec.tsx
- frontend/packages/console-dynamic-plugin-sdk/src/app/AppInitSDK.tsx
- frontend/packages/console-dynamic-plugin-sdk/src/shared-modules/shared-modules-meta.ts
- frontend/packages/console-dynamic-plugin-sdk/README.md
- frontend/public/components/sidebars/resource-sidebar.tsx
- frontend/package.json
- frontend/packages/console-dynamic-plugin-sdk/src/utils/test-utils.ts
- frontend/public/components/notification-drawer.tsx
- frontend/packages/console-dynamic-plugin-sdk/scripts/package-definitions.ts
- frontend/packages/console-app/src/tests/plugin-test-utils.ts
- frontend/packages/console-shared/src/components/editor/CodeEditorSidebar.tsx
🧰 Additional context used
🧬 Code graph analysis (15)
frontend/packages/console-plugin-sdk/src/utils/allowed-plugins.ts (1)
frontend/public/components/utils/link.tsx (1)
getURLSearchParams(60-69)
frontend/packages/console-shared/src/test-utils/unit-test-utils.tsx (1)
frontend/public/plugins.ts (1)
pluginStore(20-34)
frontend/public/components/edit-yaml.tsx (1)
frontend/packages/console-shared/src/hooks/useResourceSidebarSamples.ts (1)
useResourceSidebarSamples(345-378)
frontend/packages/console-app/src/components/dashboards-page/dynamic-plugins-health-resource/NotLoadedDynamicPlugins.tsx (1)
frontend/public/models/index.ts (1)
ConsolePluginModel(1296-1311)
frontend/packages/console-shared/src/components/formik-fields/CodeEditorField.tsx (1)
frontend/packages/console-shared/src/hooks/useResourceSidebarSamples.ts (1)
useResourceSidebarSamples(345-378)
frontend/packages/webterminal-plugin/src/components/cloud-shell/__tests__/MultiTabbedTerminal.spec.tsx (1)
frontend/packages/console-shared/src/test-utils/unit-test-utils.tsx (1)
renderWithProviders(45-64)
frontend/packages/console-app/src/components/flags/FeatureFlagExtensionLoader.tsx (5)
frontend/packages/console-shared/src/hooks/useConsoleDispatch.ts (1)
useConsoleDispatch(14-14)frontend/packages/console-shared/src/hooks/useConsoleSelector.ts (1)
useConsoleSelector(12-12)frontend/packages/console-dynamic-plugin-sdk/src/extensions/feature-flags.ts (7)
SetFeatureFlag(45-45)FeatureFlag(5-11)isFeatureFlag(35-35)ModelFeatureFlag(14-22)isModelFeatureFlag(37-38)FeatureFlagHookProvider(25-31)isFeatureFlagHookProvider(40-41)frontend/packages/console-app/src/features/integration-test.ts (1)
handler(13-18)frontend/public/actions/flags.ts (1)
updateModelFlags(146-150)
frontend/public/components/factory/modal.tsx (1)
frontend/public/plugins.ts (1)
pluginStore(20-34)
frontend/packages/console-dynamic-plugin-sdk/src/coderefs/__tests__/coderef-resolver.spec.ts (1)
frontend/packages/console-dynamic-plugin-sdk/src/coderefs/coderef-resolver.ts (1)
resolveExtension(62-99)
frontend/packages/console-shared/src/hooks/useResourceSidebarSamples.ts (3)
frontend/packages/console-dynamic-plugin-sdk/src/extensions/add-actions.ts (2)
AddAction(8-28)isAddAction(51-53)frontend/packages/console-dynamic-plugin-sdk/src/extensions/catalog.ts (2)
CatalogItemType(7-25)isCatalogItemType(112-114)frontend/packages/console-dynamic-plugin-sdk/src/extensions/perspectives.ts (1)
isPerspective(31-33)
frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-init.ts (4)
frontend/public/plugins.ts (1)
pluginStore(20-34)frontend/packages/console-dynamic-plugin-sdk/src/build-types.ts (1)
ConsolePluginManifest(46-52)frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-shared-modules.ts (2)
initSharedScope(13-13)getSharedScope(18-24)frontend/packages/console-plugin-sdk/src/utils/allowed-plugins.ts (1)
dynamicPluginNames(25-25)
frontend/packages/knative-plugin/src/components/add/__tests__/EventSource.spec.tsx (1)
frontend/packages/console-shared/src/test-utils/unit-test-utils.tsx (1)
renderWithProviders(45-64)
frontend/public/plugins.ts (3)
frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-shared-modules.ts (1)
getSharedScope(18-24)frontend/get-local-plugins.js (1)
require(3-3)frontend/public/redux.ts (1)
RootState(20-28)
frontend/packages/console-dynamic-plugin-sdk/src/coderefs/coderef-resolver.ts (1)
frontend/packages/console-dynamic-plugin-sdk/src/types.ts (1)
CodeRef(31-31)
frontend/packages/console-plugin-sdk/src/codegen/local-plugins.ts (2)
frontend/packages/console-plugin-sdk/src/codegen/plugin-resolver.ts (1)
PluginPackage(111-115)frontend/get-local-plugins.js (1)
getLocalPluginsModuleData(18-22)
logonoff
force-pushed
the
CONSOLE-3769-s3e3-the-coup
branch
from
58592b6 to
f6560cf
Compare
|
@logonoff: This pull request references CONSOLE-3769 which is a valid jira issue. 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/public/components/sidebars/resource-sidebar-samples.tsx (1)
213-219: Remove unused props fromResourceSidebarSamplesProps.The
yamlSamplesListandkindObjprops are defined in the type signature but neither is consumed by theResourceSidebarSamplescomponent. Remove them to clarify the component's actual contract—the component only needssamples,loadSampleYaml, anddownloadSampleYaml.frontend/packages/console-dynamic-plugin-sdk/src/types.ts (1)
1-18: LGTM: Clean re-export ofLoadedExtensionfrom upstream SDK.The removal of the local
pluginIDaugmentation and direct re-export from@openshift/dynamic-plugin-sdkis correct. This aligns the public API surface with upstream SDK types.ResolvedExtensioncontinues to useLoadedExtensioncorrectly as a wrapper type.Breaking change not yet documented: The change is properly recorded in
CHANGELOG-core.md(pluginID removal fromuseResolvedExtensions), butupgrade-sdk.mdremains incomplete (section 4.22 shows "TODO"). Add concrete migration guidance: consumers previously accessingextension.pluginIDmust adjust their code to source the plugin ID through alternative means (clarify the recommended approach).
🤖 Fix all issues with AI agents
In `@frontend/packages/console-dynamic-plugin-sdk/src/app/AppInitSDK.tsx`:
- Line 14: The JSDoc example for AppInitSDK is missing the new optional
dynamicPlugins configuration; update the example in AppInitSDK.tsx to include
dynamicPlugins in the configurations object (replace the old initPlugins
reference if present), e.g. show configurations containing appFetch,
apiDiscovery, and dynamicPlugins so consumers see the new API surface; ensure
the example uses the existing optional invocation dynamicPlugins?.() and
references the AppInitSDK and configurations symbols.
In `@frontend/public/components/app.tsx`:
- Line 12: The import of PluginStoreProvider from `@openshift/dynamic-plugin-sdk`
is invalid and will break the app; either remove the PluginStoreProvider import
and the corresponding <PluginStoreProvider> wrapper used in the render tree (the
component referenced around line 518), or replace the import with the correct
provider symbol exported by the SDK (locate the actual export in the package or
its docs) and update the import to use that symbol; ensure the component name in
the JSX matches the imported symbol (PluginStoreProvider or the correct
provider) and remove any unused import if you choose to delete the wrapper.
♻️ Duplicate comments (15)
frontend/packages/topology/src/data-transforms/DataModelExtension.tsx (1)
16-21: LGTM — Interface and signature updated consistently.The prop rename from
pluginIDtouidaligns with the SDK'sResolvedExtension.uidsemantics. The past review feedback regarding terminology has been addressed.frontend/packages/console-app/src/components/flags/FeatureFlagExtensionLoader.tsx (1)
66-74: Previous review concerns about exception handling and reference equality still apply.The two issues flagged in prior reviews remain unaddressed:
Exception resilience: If
onChangethrows,prevExtensionsRef.currentnever updates, causing the same diff to re-trigger on every render. Wrap intry/finallyso the ref always advances.Reference equality in
difference(): SinceresolveExtensionclones extension properties, logically identical extensions will have different object references. This causes false positives in the diff (extensions appear both added and removed). Consider diffing byuidinstead.Proposed hardening for exception handling
useEffect(() => { const added = difference(extensions, prevExtensionsRef.current); const removed = difference(prevExtensionsRef.current, extensions); if (added.length > 0 || removed.length > 0) { - onChange(added, removed); - prevExtensionsRef.current = extensions; + try { + onChange(added, removed); + } finally { + prevExtensionsRef.current = extensions; + } } }, [extensions, onChange]);frontend/packages/console-plugin-sdk/src/api/useExtensions.ts (2)
27-31: Sorting lacks a deterministic tie-breaker for equal plugin order values.When two plugins have the same order index (or both fallback to
MIN_SAFE_INTEGER), the relative order depends on the JS engine's sort stability. While ES2019+ guarantees stable sorts, adding a secondary comparator (e.g.,a.uid.localeCompare(b.uid)) ensures consistent ordering across all environments and makes the behavior explicit.Suggested tie-breaker
return [...extensions].sort( - (a, b) => - (pluginOrderMap.get(a.pluginName) ?? Number.MIN_SAFE_INTEGER) - - (pluginOrderMap.get(b.pluginName) ?? Number.MIN_SAFE_INTEGER), + (a, b) => { + const orderA = pluginOrderMap.get(a.pluginName) ?? Number.MIN_SAFE_INTEGER; + const orderB = pluginOrderMap.get(b.pluginName) ?? Number.MIN_SAFE_INTEGER; + if (orderA !== orderB) { + return orderA - orderB; + } + return a.uid.localeCompare(b.uid); + }, );
80-94: Ref mutations insideuseMemoand UID-only caching remain problematic.Two issues persist here:
React purity violation: Mutating
previousResultRefandpreviousUIDsRefinsideuseMemo(lines 90-91) breaks React's expectation that memo callbacks are pure. In concurrent rendering, React may invoke the callback multiple times or discard results, leading to stale or inconsistent refs.Translation updates missed: The UID-based short-circuit (lines 85-87) returns the cached result when UIDs match, but
useTranslatedExtensionscan produce updated translated strings without changing UIDs. Language switches won't propagate to consumers.Recommended refactor: move ref updates outside useMemo
- // Track the previous result and UIDs for referential stability - const previousResultRef = useRef<LoadedExtension<E>[]>([]); - const previousUIDsRef = useRef<string>(''); + // Cache for referential stability + const cacheRef = useRef<{ uids: string; result: LoadedExtension<E>[] }>({ + uids: '', + result: [], + }); - return useMemo(() => { - const sorted = sortExtensionsByPluginOrder(translatedExtensions); - const currentUIDs = sorted.map((e) => e.uid).join(','); + const sorted = useMemo( + () => sortExtensionsByPluginOrder(translatedExtensions), + [translatedExtensions], + ); - // Return previous result if the extensions haven't changed - if (currentUIDs === previousUIDsRef.current) { - return previousResultRef.current; - } + const currentUIDs = useMemo(() => sorted.map((e) => e.uid).join(','), [sorted]); - // Update refs and return new result - previousResultRef.current = sorted; - previousUIDsRef.current = currentUIDs; - - return sorted; - }, [translatedExtensions]); + // Update cache outside render phase (ref update is synchronous but not during memo) + if (currentUIDs !== cacheRef.current.uids) { + cacheRef.current = { uids: currentUIDs, result: sorted }; + } + + return cacheRef.current.result; };For the translation reactivity issue, consider whether
useTranslatedExtensionsshould return a new array reference when translations change, or include a translation version/locale in the cache key.#!/bin/bash # Verify how useTranslatedExtensions handles language changes rg -n "useTranslatedExtensions" -A 15 frontend/packages/console-plugin-sdk/src/utils/useTranslatedExtensions.ts | head -50frontend/packages/console-shared/src/hooks/useResourceSidebarSamples.ts (4)
257-270: Defensive check needed:labelanddescriptionmay not be strings.The
lazyYamlfunction assumesextension.properties.labelandextension.properties.descriptionare strings and calls.split('\n')directly. Per theAddActiontype definition, these are required strings, but runtime data from extensions could be malformed.🛡️ Add defensive coercion
lazyYaml: () => { const sortedExtensions = addActions .slice() .sort((a, b) => a.properties.id.localeCompare(b.properties.id)); const yaml = sortedExtensions .map((extension) => { const { id, label, description } = extension.properties; - const labelComment = label.split('\n').join('\n # '); - const descriptionComment = description.split('\n').join('\n # '); + const labelComment = (label ?? '').split('\n').join('\n # '); + const descriptionComment = (description ?? '').split('\n').join('\n # '); return `- # ${labelComment}\n # ${descriptionComment}\n ${id}`; }) .join('\n'); return yaml; },
352-359: FirehoseResult gating:_.isEmptyon wrapper object may be unreliable.Using
_.isEmpty(yamlSamplesList)checks the wrapper object, not the load state. If the Firehose hasn't loaded yet or errored,yamlSamplesListmay be truthy but contain invalid data.Prefer explicit state checks:
♻️ Use explicit loaded/loadError checks
- const yamlSamplesData = !_.isEmpty(yamlSamplesList) - ? _.filter( + const yamlSamplesData = yamlSamplesList?.loaded && !yamlSamplesList?.loadError && Array.isArray(yamlSamplesList.data) + ? _.filter( yamlSamplesList.data, // ... ) : [];
373-375: Misleading comment: snippets and samples are disjoint, not a superset.The comment states "snippets are a superset of samples," but the filter logic produces mutually exclusive sets. Items with
snippet: truego tosnippets; items without go tosamples.📝 Clarify comment
- // For the time being, `snippets` are a superset of `samples` + // Partition: items with snippet=true go to snippets, others to samples const snippets = allSamples.filter((sample: Sample) => sample.snippet); const samples = allSamples.filter((sample: Sample) => !sample.snippet);
364-367: Incorrect type utility: useOmitinstead ofExclude.
Exclude<T, U>operates on union types to exclude members. For removing a property from an object type, useOmit<T, K>. The current cast doesn't actually remove theidproperty from the type.🔧 Fix type utility
return { id: sample.metadata.uid, - ...(sample.spec as Exclude<Sample, 'id'>), + ...(sample.spec as Omit<Sample, 'id'>), };frontend/package.json (1)
161-161: Verify@openshift/dynamic-plugin-sdk@6.0.0availability.Previous review flagged that version 6.0.0 doesn't exist on npm—latest was 5.0.1. If this is an internal/pre-release build or has since been published, please confirm. Otherwise, pin to an available version to avoid CI failures.
#!/bin/bash # Check available versions of `@openshift/dynamic-plugin-sdk` on npm npm view `@openshift/dynamic-plugin-sdk` versions --json 2>/dev/null | tail -20 echo "---" npm view `@openshift/dynamic-plugin-sdk` version 2>/dev/nullfrontend/packages/console-dynamic-plugin-sdk/CHANGELOG-core.md (1)
21-21: Changelog entry should specify the replacement forpluginID.The breaking change entry is documented but doesn't indicate what plugin authors should use instead. Based on the PR changes,
uid(from the extension's metadata) or accessingmanifest.nameviausePluginInfowould be the alternatives.Consider updating to:
- **Breaking**: Removed `pluginID` from the result in `useResolvedExtensions` hook; use `uid` instead ([CONSOLE-3769], [`#15904`])frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-init.ts (1)
72-95: Legacy callback registration provides good backward compatibility.The version stripping logic using
lastIndexOf('@')correctly handles scoped packages (e.g.,@acme/foo@1.0.0→@acme/foo). The deprecation warning is helpful for operators managing older plugins.One remaining concern from prior reviews: line 88 calls
window[sdkCallbackName]without verifying it exists. If PluginStore fails to register the callback before a legacy plugin script loads, this will throw.Guard the SDK callback invocation
+ if (typeof window[sdkCallbackName] !== 'function') { + // eslint-disable-next-line no-console + console.error( + `SDK entry callback "${sdkCallbackName}" not initialized when legacy plugin "${pluginName}" loaded`, + ); + return; + } window[sdkCallbackName](patchedPluginName, entryModule);frontend/public/plugins.ts (3)
1-11: Import organization and version parsing look good, but null handling needs attention.The
semver.valid()function returnsnullfor invalid version strings. Ifwindow.SERVER_FLAGS.releaseVersionis undefined or malformed in production,CURRENT_OPENSHIFT_VERSIONbecomesnull, which propagates tofixedPluginDependencyResolutionson line 28-31.The development fallback (
|| '4.1337.67') on line 31 handles dev environments, but production has no guard. While the comment claims console-operator always provides this value, defensive coding would add a warning:Add production warning for invalid version
const CURRENT_OPENSHIFT_VERSION = semver(window.SERVER_FLAGS.releaseVersion); + +if (process.env.NODE_ENV === 'production' && !CURRENT_OPENSHIFT_VERSION) { + // eslint-disable-next-line no-console + console.warn( + 'Invalid releaseVersion from SERVER_FLAGS:', + window.SERVER_FLAGS.releaseVersion, + ); +}
38-43: Local plugin loading is synchronous fire-and-forget.Line 43 calls
loadPluginwithout awaiting or catching errors. For bundled local plugins (generated by webpack), this is likely safe since they're already in memory. However, ifloadPlugincan reject for local manifests, errors would be swallowed.Prior reviews suggested error handling. Given this is module-level initialization, a sync-looking pattern may be intentional. Document the assumption:
Add clarifying comment or error handling
+// Local plugins are bundled by webpack and loaded synchronously. +// loadPlugin() returns a Promise but resolves immediately for LocalPluginManifest. localPlugins.forEach((plugin) => pluginStore.loadPlugin(plugin));Or add error handling:
-localPlugins.forEach((plugin) => pluginStore.loadPlugin(plugin)); +localPlugins.forEach((plugin) => { + pluginStore.loadPlugin(plugin).catch((err) => { + // eslint-disable-next-line no-console + console.error(`Failed to load local plugin ${plugin.name}:`, err); + }); +});
69-74: Log label should be "Local plugins" per PR terminology.The PR description explicitly renames "active plugins" to "local plugins," but line 71 still logs "Static plugins." This inconsistency will cause confusion during debugging.
Update log label
- console.info(`Static plugins: [${localPlugins.map((p) => p.name).join(', ')}]`); + console.info(`Local plugins: [${localPlugins.map((p) => p.name).join(', ')}]`);frontend/packages/console-app/src/__tests__/plugin-test-utils.ts (1)
8-15: Race condition:loadPluginreturns a Promise but is called without awaiting.Prior reviews flagged this exact issue.
PluginStore.loadPlugin()is async (confirmed byawaitusage inplugin-init.ts:23), but lines 11-13 useforEachwithout awaiting. This meanstestedExtensionson line 15 is derived before plugins finish loading, causing unreliable test results.For local manifests (not URL-based),
loadPluginmay complete synchronously in some SDK versions, but relying on this is fragile.Robust async initialization pattern
// Option A: If top-level await is supported (ESM modules) const testedPlugins = loadLocalPluginsForTestPurposes(resolvePluginPackages()); const testedPluginStore = new PluginStore(); await Promise.all(testedPlugins.map((plugin) => testedPluginStore.loadPlugin(plugin))); export const testedExtensions = ImmutableList<Extension>(testedPluginStore.getExtensions()); // Option B: Export a factory function for test setup export const initTestedPluginStore = async () => { const plugins = loadLocalPluginsForTestPurposes(resolvePluginPackages()); const store = new PluginStore(); await Promise.all(plugins.map((p) => store.loadPlugin(p))); return ImmutableList<Extension>(store.getExtensions()); };#!/bin/bash # Check if loadPlugin for LocalPluginManifest is documented as sync in SDK rg -n "loadPlugin.*LocalPluginManifest|LocalPluginManifest.*loadPlugin" --type ts frontend/
🧹 Nitpick comments (7)
frontend/packages/console-app/src/components/nodes/useNodeStatusExtensions.tsx (1)
63-66: Improved error context — good enhancement.Logging both
uidandtypeprovides better diagnostics than the previouspluginID-only approach. When something fails in a customer environment, knowing the extension type immediately helps narrow down which plugin capability is misbehaving.One optional nit for future consideration: a structured format like
Extension failed [uid=${uid}, type=${type}]can be slightly easier to grep in log aggregators, but the current form is perfectly acceptable.frontend/packages/console-dynamic-plugin-sdk/src/coderefs/coderef-resolver.ts (1)
16-26: Symbol extraction approach is sound with proper fail-fast guard.The runtime derivation of the SDK's internal
codeRefSymbolviaapplyCodeRefSymbolon a dummy function is the right approach when the SDK doesn't expose this symbol publicly. The fail-fast guard at lines 24-26 ensures immediate, obvious failure if SDK internals change.One optional improvement per past review: consider documenting the SDK version this was validated against so maintainers know to re-verify after SDK upgrades.
📝 Optional: Add SDK version comment
/** * Extract the SDK's internal CodeRef symbol by applying it to a dummy function. * * This ensures we can detect code refs created by the SDK, which uses its own * private Symbol instance. + * + * Validated against `@openshift/dynamic-plugin-sdk` ^6.0.0. Re-verify symbol + * extraction after SDK version upgrades. */ const codeRefSymbol = Object.getOwnPropertySymbols(applyCodeRefSymbol(() => Promise.resolve()))[0];frontend/packages/console-shared/src/hooks/useResourceSidebarSamples.ts (1)
93-99: Performance:useDefaultSamplesrebuilds ImmutableMap on every render.This hook calls
useResolvedExtensionsthree times and constructs a newImmutableMapon each invocation. Since the map contains many sample objects, this causes unnecessary allocations and can trigger downstream re-renders.Wrap the map construction in
useMemowith appropriate dependencies:♻️ Memoize the return value
+import { useMemo } from 'react'; + const useDefaultSamples = () => { const { t } = useTranslation('console-shared'); const [addActions] = useResolvedExtensions<AddAction>(isAddAction); const [catalogItemTypes] = useResolvedExtensions<CatalogItemType>(isCatalogItemType); const [perspectives] = useResolvedExtensions<Perspective>(isPerspective); const clusterRoleBindingSamples = useClusterRoleBindingSamples(); - return ImmutableMap<GroupVersionKind, Sample[]>() + return useMemo(() => ImmutableMap<GroupVersionKind, Sample[]>() .setIn(/* ... */) // ... rest of chain - ); + , [t, addActions, catalogItemTypes, perspectives, clusterRoleBindingSamples]); };frontend/public/components/sidebars/resource-sidebar-samples.tsx (1)
96-107: Error handling preserved, but consider a fallback callback.Good to see the
try/catchwas retained. However, whenlazyYaml()throws, the callback is never invoked, which could leave the UI waiting indefinitely (e.g., YAML preview never appears after clicking "Show YAML").🛡️ Consider calling callback with empty string on error
const resolveYaml = (callback: (resolvedYaml: string) => void) => { if (yaml) { callback(yaml); } else if (lazyYaml) { try { callback(lazyYaml()); } catch (error) { // eslint-disable-next-line no-console console.warn(`Error while running lazy yaml snippet ${id} (${title})`, error); + callback(''); // Provide fallback to prevent UI from hanging } } };frontend/public/components/app.tsx (1)
437-446: TODO comments indicate known technical debt—track for follow-up.The
initDynamicPluginswrapper correctly delegates toinitConsolePlugins(pluginStore). The TODO comments regarding earlier lifecycle loading and HAC-375 (coFetch dependency injection) should be tracked.The fire-and-forget async pattern inside
initConsolePluginswas flagged in prior reviews but marked addressed. The current design accepts that plugin load failures are logged but don't block app initialization—this is a reasonable trade-off for resilience.Would you like me to open issues to track the TODOs on lines 441-442?
frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-init.ts (1)
18-57:loadAndEnablePluginerror handling is improved but still has a subtle gap.The optional chaining on
plugin?.status(line 37) addresses the null dereference concern from prior reviews. However, after the.catch()on line 30-33, execution continues to line 35 even whenloadPluginfails. IfloadPluginrejects,onErroris called, but then the code proceeds to checkplugin?.status—which may find a partially registered plugin or none at all.Consider returning early after the catch to avoid redundant status checks:
Suggested improvement
.catch((err: ErrorWithCause) => { // ErrorWithCause isn't the exact type but it's close enough for our use onError(`[loadAndEnablePlugin] ${pluginName} loadPlugin failed: ${err.message}`, err.cause); + return; // Exit after handling load failure }); + + // Only check status if loadPlugin succeeded (no early return above) const plugin = pluginStore.getPluginInfo().find((p) => p.manifest.name === pluginName);frontend/packages/console-dynamic-plugin-sdk/src/app/AppInitSDK.tsx (1)
24-34: JSDoc example should demonstratedynamicPluginsconfiguration.The example code block still shows the minimal pattern without the new
dynamicPluginsoption. Prior review flagged this and it was marked addressed, but the current code doesn't reflect that update.Update the example
* `@example` * ```ts * return ( * <Provider store={store}> - * <AppInitSDK configurations={{ appFetch, apiDiscovery }}> + * <AppInitSDK configurations={{ appFetch, apiDiscovery, dynamicPlugins: () => initPlugins() }}> * <CustomApp /> * ... * </AppInitSDK> * </Provider> * ) * ```
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
frontend/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (46)
frontend/package.jsonfrontend/packages/console-app/src/__tests__/plugin-test-utils.tsfrontend/packages/console-app/src/components/dashboards-page/dynamic-plugins-health-resource/NotLoadedDynamicPlugins.tsxfrontend/packages/console-app/src/components/file-upload/__tests__/file-upload-utils.spec.tsfrontend/packages/console-app/src/components/flags/FeatureFlagExtensionLoader.tsxfrontend/packages/console-app/src/components/nodes/useNodeStatusExtensions.tsxfrontend/packages/console-app/src/components/user-preferences/__tests__/userPreferences.data.tsxfrontend/packages/console-app/src/components/user-preferences/perspective/__tests__/perspective.data.tsfrontend/packages/console-dynamic-plugin-sdk/CHANGELOG-core.mdfrontend/packages/console-dynamic-plugin-sdk/README.mdfrontend/packages/console-dynamic-plugin-sdk/package.jsonfrontend/packages/console-dynamic-plugin-sdk/scripts/package-definitions.tsfrontend/packages/console-dynamic-plugin-sdk/src/app/AppInitSDK.tsxfrontend/packages/console-dynamic-plugin-sdk/src/coderefs/__tests__/coderef-resolver.spec.tsfrontend/packages/console-dynamic-plugin-sdk/src/coderefs/coderef-resolver.tsfrontend/packages/console-dynamic-plugin-sdk/src/perspective/__tests__/perspective.data.tsfrontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-init.tsfrontend/packages/console-dynamic-plugin-sdk/src/shared-modules/shared-modules-meta.tsfrontend/packages/console-dynamic-plugin-sdk/src/types.tsfrontend/packages/console-plugin-sdk/package.jsonfrontend/packages/console-plugin-sdk/src/api/pluginSubscriptionService.tsfrontend/packages/console-plugin-sdk/src/api/useExtensions.tsfrontend/packages/console-plugin-sdk/src/store.tsfrontend/packages/console-plugin-sdk/src/utils/allowed-plugins.tsfrontend/packages/console-shared/locales/en/console-shared.jsonfrontend/packages/console-shared/src/components/actions/__tests__/utils-test-data.tsfrontend/packages/console-shared/src/components/catalog/__tests__/CatalogController.spec.tsxfrontend/packages/console-shared/src/components/editor/CodeEditorSidebar.tsxfrontend/packages/console-shared/src/components/formik-fields/CodeEditorField.tsxfrontend/packages/console-shared/src/hooks/__tests__/useTelemetry.spec.tsfrontend/packages/console-shared/src/hooks/useResourceSidebarSamples.tsfrontend/packages/console-shared/src/utils/index.tsfrontend/packages/console-shared/src/utils/sample-utils.tsfrontend/packages/dev-console/src/components/add/__tests__/add-page-test-data.tsxfrontend/packages/dev-console/src/utils/__tests__/useAddActionExtensions.spec.tsfrontend/packages/topology/src/data-transforms/DataModelExtension.tsxfrontend/packages/topology/src/data-transforms/DataModelProvider.tsxfrontend/public/actions/features.tsfrontend/public/components/app.tsxfrontend/public/components/dashboard/project-dashboard/getting-started/__tests__/SampleGettingStartedCard.data.tsfrontend/public/components/edit-yaml.tsxfrontend/public/components/notification-drawer.tsxfrontend/public/components/sidebars/resource-sidebar-samples.tsxfrontend/public/components/sidebars/resource-sidebar.tsxfrontend/public/components/utils/__tests__/nav.spec.tsfrontend/public/plugins.ts
💤 Files with no reviewable changes (17)
- frontend/packages/console-shared/src/hooks/tests/useTelemetry.spec.ts
- frontend/packages/console-shared/src/components/actions/tests/utils-test-data.ts
- frontend/packages/console-plugin-sdk/package.json
- frontend/public/components/dashboard/project-dashboard/getting-started/tests/SampleGettingStartedCard.data.ts
- frontend/packages/console-dynamic-plugin-sdk/package.json
- frontend/packages/dev-console/src/utils/tests/useAddActionExtensions.spec.ts
- frontend/packages/console-shared/src/components/catalog/tests/CatalogController.spec.tsx
- frontend/packages/console-app/src/components/file-upload/tests/file-upload-utils.spec.ts
- frontend/packages/console-app/src/components/user-preferences/perspective/tests/perspective.data.ts
- frontend/packages/dev-console/src/components/add/tests/add-page-test-data.tsx
- frontend/packages/console-plugin-sdk/src/store.ts
- frontend/public/components/utils/tests/nav.spec.ts
- frontend/packages/console-shared/src/utils/sample-utils.ts
- frontend/packages/console-shared/src/utils/index.ts
- frontend/packages/console-plugin-sdk/src/api/pluginSubscriptionService.ts
- frontend/packages/console-app/src/components/user-preferences/tests/userPreferences.data.tsx
- frontend/packages/console-dynamic-plugin-sdk/src/perspective/tests/perspective.data.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- frontend/packages/console-shared/src/components/editor/CodeEditorSidebar.tsx
- frontend/packages/console-plugin-sdk/src/utils/allowed-plugins.ts
- frontend/public/components/sidebars/resource-sidebar.tsx
- frontend/packages/console-shared/locales/en/console-shared.json
- frontend/packages/console-dynamic-plugin-sdk/README.md
- frontend/public/components/edit-yaml.tsx
- frontend/public/components/notification-drawer.tsx
- frontend/public/actions/features.ts
🧰 Additional context used
🧬 Code graph analysis (5)
frontend/packages/console-shared/src/components/formik-fields/CodeEditorField.tsx (1)
frontend/packages/console-shared/src/hooks/useResourceSidebarSamples.ts (1)
useResourceSidebarSamples(345-378)
frontend/packages/console-dynamic-plugin-sdk/src/coderefs/coderef-resolver.ts (1)
frontend/packages/console-dynamic-plugin-sdk/src/types.ts (1)
CodeRef(31-31)
frontend/packages/console-shared/src/hooks/useResourceSidebarSamples.ts (5)
frontend/public/models/index.ts (5)
ClusterRoleModel(539-552)ResourceQuotaModel(696-709)RoleModel(570-584)ConsoleLinkModel(1160-1175)ConsoleOperatorConfigModel(1147-1158)frontend/packages/console-dynamic-plugin-sdk/src/extensions/add-actions.ts (2)
AddAction(8-28)isAddAction(51-53)frontend/packages/console-dynamic-plugin-sdk/src/extensions/catalog.ts (2)
CatalogItemType(7-25)isCatalogItemType(112-114)frontend/packages/console-dynamic-plugin-sdk/src/extensions/perspectives.ts (1)
isPerspective(31-33)frontend/packages/console-shared/src/utils/default-categories.ts (1)
defaultCatalogCategories(3-71)
frontend/packages/console-app/src/__tests__/plugin-test-utils.ts (2)
frontend/packages/console-plugin-sdk/src/codegen/local-plugins.ts (1)
loadLocalPluginsForTestPurposes(78-96)frontend/packages/console-plugin-sdk/src/codegen/plugin-resolver.ts (1)
resolvePluginPackages(78-105)
frontend/public/components/app.tsx (4)
frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-init.ts (1)
initConsolePlugins(102-126)frontend/public/plugins.ts (1)
pluginStore(20-34)frontend/public/co-fetch.ts (1)
appInternalFetch(129-155)frontend/packages/console-dynamic-plugin-sdk/src/app/k8s/api-discovery/api-discovery.ts (1)
initApiDiscovery(149-159)
🔇 Additional comments (39)
frontend/packages/console-app/src/components/nodes/useNodeStatusExtensions.tsx (1)
47-48: LGTM — Clean migration frompluginIDtouid.This aligns nicely with the SDK's shift to
manifest.name-based identifiers. Destructuringtypealongsideuidis a good call for the improved error diagnostics below. Also keeps parity with line 31's existinguidusage.frontend/packages/topology/src/data-transforms/DataModelProvider.tsx (2)
31-39: LGTM — Clean uid propagation across all resolution paths.The type declaration and all three resolution branches (async success, async error, static resources) consistently thread through
factory.uid. The error message terminology issue flagged in prior review has been addressed—it now correctly references "extension" rather than "plugin."Also applies to: 57-78
92-98: LGTM — Correct separation of factory id (key) vs extension uid (tracking).Using
factory.properties.idas the React key is appropriate since it's the factory's stable identifier, whilefactory.uidcorrectly identifies the resolved extension instance for logging and tracking purposes.frontend/packages/topology/src/data-transforms/DataModelExtension.tsx (1)
67-84: LGTM — Correct terminology and memoization dependencies.The warning now correctly identifies the source as an "Extension" rather than "Plugin" (consistent with
ResolvedExtension.uidsemantics). Includinguidin theuseMemodependency array is necessary since it's referenced in theconsole.warnwithin the memo body—prevents stale closures if the component receives a different extension.frontend/packages/console-app/src/components/flags/FeatureFlagExtensionLoader.tsx (5)
1-23: Imports look well-organized.The new imports for lodash
difference, model feature flag types, and theupdateModelFlagsaction align with the hook-based refactoring approach. The structure cleanly separates runtime values from type-only imports.
24-53: Solid pattern for deferring dispatches to avoid render-time state updates.The
useLayoutEffectwithout a dependency array intentionally runs on every render to flush queued flag updates synchronously after React commits—this sidesteps the react-redux 8.x "Cannot update a component while rendering" pitfall. The early-exit on line 38-40 and the staleness check on line 42 are efficient safeguards.One minor note: if the same flag is queued multiple times within a single render cycle, only the final value survives due to
Map.setsemantics. This appears intentional, but worth keeping in mind for debugging scenarios.
77-97: Good error isolation for feature flag handlers.The try/catch on lines 85-90 ensures one misbehaving plugin handler doesn't cascade failures to others—important for extension-based architectures where plugin code quality varies. Including
pluginNamein the error message aids triage.Intentionally ignoring
removedextensions is appropriate here sinceFeatureFlaghandlers are registration callbacks with no teardown semantics.
99-126: Clean ref-based pattern for capturing latest Redux state in a stable callback.The
modelsRefpattern (lines 108-111) is the canonical React approach for accessing current state from within a memoized callback without invalidating its identity on every state change. This correctly avoids the stale-closure pitfall while keepinghandleChangestable for the diff hook's dependency array.The inline comment at lines 118-119 helpfully documents why the models are passed via action payload rather than accessed directly in the reducer—good practice for cross-reducer data dependencies.
128-161: Clean integration of the new extension-handling hooks.The component correctly calls
useFeatureFlagExtensionsanduseModelFeatureFlagExtensionsunconditionally before any conditional rendering (React's rules of hooks). Usinguidas the key at line 151 for the renderedFeatureFlagExtensionHookResolvercomponents ensures stable identity across re-renders.The public API surface of
FeatureFlagExtensionLoaderremains unchanged, which is important for backward compatibility as the PR description emphasizes.frontend/packages/console-dynamic-plugin-sdk/src/coderefs/__tests__/coderef-resolver.spec.ts (3)
1-10: LGTM – Imports correctly align with SDK migration.The imports now properly source
applyCodeRefSymbolfrom@openshift/dynamic-plugin-sdkrather than a local implementation, andisCodeRefErroris correctly imported for the error-path test at line 166. ThegetExecutableCodeRefMockutility abstracts away the SDK symbol details, making tests less brittle.
129-143: Test correctly validates new cloning behavior.The renamed test and updated assertions using
.not.toBe()properly verify thatresolveExtensionnow returns a new extension object rather than mutating the original. This aligns with the production code change whereclonedPropertiesis used to preserve immutability of the input extension.Good coverage of both cases—extensions with and without CodeRefs.
145-171: Test documents Jest 30 limitation clearly – no action needed.The extensive inline comments explain the Jest 30 symbol interference issue and clarify what the test actually validates (no CodeRefs found scenario). This is acceptable given the alternative—modifying production code to accommodate test tooling—would be the wrong tradeoff.
frontend/packages/console-dynamic-plugin-sdk/src/coderefs/coderef-resolver.ts (1)
62-99: Cloning behavior is correctly implemented with one subtlety to note.The switch from in-place mutation to returning a new extension object with cloned properties is a solid improvement for predictability. The
_.cloneDeepon line 69 ensures the original extension'spropertiesobject remains untouched.One nuance to be aware of:
_.cloneDeepdoes not clone functions—CodeRef functions remain shared references between the original and cloned properties. This meanssetCodeRefErrorat line 86 still marks errors on the original CodeRef functions, which is intentional for the "continuously reject failed CodeRefs" behavior. The JSDoc at lines 58-60 correctly describes preserving the original extension, but strictly speaking, the CodeRef functions themselves can be mutated (error markers). This is acceptable given the intended behavior, but worth documenting if questions arise later.frontend/packages/console-plugin-sdk/src/api/useExtensions.ts (2)
1-9: LGTM — clean import structureGood move dropping the legacy subscription/force-render imports in favor of the SDK hook. The aliased import
useExtensionsSDKclearly disambiguates from the exporteduseExtensions.
66-71: LGTM — runtime validation for type guardsThe early throw ensures callers provide at least one guard, preventing silent bugs from empty extension lists.
frontend/packages/console-app/src/components/dashboards-page/dynamic-plugins-health-resource/NotLoadedDynamicPlugins.tsx (3)
2-2: Clean migration to SDK types.Good adoption of the upstream
@openshift/dynamic-plugin-sdktypes. This aligns well with the Phase 2 migration objectives.
14-17: Property access aligns with SDK patterns.The switch from
pluginNametomanifest.nameis consistent with how plugin identifiers are accessed throughout the codebase. Both theFailedPluginInfoEntryandPendingPluginInfoEntrytypes exposemanifest.nameas the canonical plugin identifier—the same property used as ResourceLink targets in the ConsolePlugin resource model. React keys remain stable and unique per plugin.
27-30: Type definition properly aligned with SDK discriminated union.The union type
(FailedPluginInfoEntry | PendingPluginInfoEntry)[]correctly models the two states for not-loaded plugins and the sole caller inDynamicPluginsPopover.tsxis already using it correctly. The filtering byplugin.statusfield naturally narrows the discriminated union types, eliminating the need for type assertions.frontend/packages/console-shared/src/hooks/useResourceSidebarSamples.ts (2)
1-46: LGTM on imports and type definitions.The
Sampletype is well-structured with mutually exclusiveyamlandlazyYamloptions for lazy evaluation. Type exports are appropriate for the public API surface.
68-91: Sample structure supports optional yaml—no runtime issue exists here.The
useClusterRoleBindingSampleshook intentionally returns samples withoutyamlorlazyYamlproperties, which is valid per theSampletype definition where both are optional. The handlers (replaceYamlContent,downloadYamlContent,insertYamlContent) inCodeEditorSidebar.tsxsafely default theyamlContentparameter to an empty string when undefined is passed, so passingundefineddoes not cause runtime errors. This design is intentional and behaves safely.frontend/packages/console-shared/src/components/formik-fields/CodeEditorField.tsx (2)
18-18: LGTM on import migration.Clean migration from the removed
getResourceSidebarSamplesutility function to the newuseResourceSidebarSampleshook.
50-54: Hook integration is clean and follows React patterns.The
useResourceSidebarSampleshook is called unconditionally (correct per Rules of Hooks), and the hook itself handles theundefinedmodel case with an early return. The FirehoseResult-shaped object correctly passes through the watch state.frontend/public/components/sidebars/resource-sidebar-samples.tsx (1)
11-11: LGTM on import path update.Sample type correctly imported from the new hook module location.
frontend/package.json (2)
115-124: Test setup changes look good.Adding the webpack mock to
setupFilesaligns with the SDK integration requiring share scope initialization in tests. This ensures tests can properly mock webpack's__webpack_share_scopes__global.
104-106: LGTM on transformIgnorePatterns update.Adding
js-combinatoricsto the non-ignored packages ensures proper ESM-to-CJS transformation during tests.frontend/packages/console-dynamic-plugin-sdk/scripts/package-definitions.ts (1)
173-187: LGTM: SDK dependency correctly added to webpack package.The
@openshift/dynamic-plugin-sdkis appropriately added togetWebpackPackagedependencies. This aligns with the architectural shift where the webpack package needs the SDK forConsoleRemotePluginbuild tooling, while core/internal packages now re-export SDK types directly.frontend/packages/console-dynamic-plugin-sdk/src/shared-modules/shared-modules-meta.ts (2)
24-26: LGTM: SDK correctly added as shared module.Adding
@openshift/dynamic-plugin-sdkto the shared modules list ensures Console and all dynamic plugins share a single SDK instance at runtime. The default metadata (singleton: true,allowFallback: false) is appropriate—plugins must not bundle their own SDK version.
44-46: Metadata entry aligns with shared module registration.The empty object
{}applies sensible defaults. No concerns here.frontend/packages/console-dynamic-plugin-sdk/src/types.ts (1)
62-65:ResolvedExtensiontype composition is correct.The generic type properly wraps
LoadedExtensionwith resolved code ref properties. No issues with the type algebra here.frontend/packages/console-dynamic-plugin-sdk/CHANGELOG-core.md (2)
13-17: Good: Important banner properly highlights breaking changes.The
> [!IMPORTANT]callout and reference toupgrade-sdk.mdis the right approach for a release with breaking changes. Ensure the upgrade guide is complete before release.
234-234: PR link reference added correctly.frontend/public/components/app.tsx (1)
517-536: PluginStoreProvider integration looks correct.The provider hierarchy is properly structured:
Provider (Redux)→PluginStoreProvider→ThemeProvider→HelmetProvider→AppInitSDK. This ensures the plugin store context is available to all descendants before dynamic plugin initialization fires.frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-init.ts (2)
1-10: Clean import organization for the new plugin loading infrastructure.The type-only imports for
PluginStore,ConsolePluginManifest, andErrorWithCauseare appropriate. TheresolveURLutility anddynamicPluginNameswire up the runtime loading correctly.
102-126:initConsolePluginsuses fire-and-forget pattern for parallel plugin loading.The
forEachon line 109 dispatches plugin loads without awaiting. This is intentional for parallelism—plugins load concurrently, and failures are logged but don't block others. The trade-off is acceptable for resilience.The
windowErroraccumulation (line 113) can grow unbounded in pathological scenarios. Prior review noted this but it's a known pattern used by Cypress e2e tests. Consider documenting the expected max error count or adding truncation if this becomes problematic in practice.frontend/packages/console-dynamic-plugin-sdk/src/app/AppInitSDK.tsx (1)
36-48: Initialization flow is clean and handles optional callbacks correctly.The
dynamicPlugins?.()invocation with optional chaining (line 42) safely handles cases where no dynamic plugin loader is configured. The try-catch around initialization (lines 40-47) ensures misconfigurations don't crash the app, though logging toconsole.warnmay be too quiet for production issues.frontend/packages/console-app/src/__tests__/plugin-test-utils.ts (2)
1-6: Updated imports align with local plugin testing strategy.The switch from
loadActivePluginsForTestPurposestoloadLocalPluginsForTestPurposesaligns with the PR's terminology migration (active → local). ThePluginStoreimport from@openshift/dynamic-plugin-sdkis consistent with production code.
17-27:getDuplicatesutility is straightforward.Clean implementation using lodash's
transformandcountBy. No issues.frontend/public/plugins.ts (2)
20-34: PluginStore configuration is well-structured.The
loaderOptionsproperly configure:
sharedScopefor webpack module federationfetchImplwith the console-specific fetch wrapper (type suppression documented)fixedPluginDependencyResolutionsfor semver-based plugin compatibilityThe
@ts-expect-errorcomment on line 23 is appropriate—consoleFetchhas an extra timeout parameter not in the standard fetch signature.
48-62:featureFlagMiddlewarecorrectly synchronizes Redux feature flags with PluginStore.This middleware pattern is clean:
- Tracks previous flags state to avoid unnecessary updates
- Compares by reference (
nextFlags !== prevFlags) which works with Immutable.js- Calls
pluginStore.setFeatureFlags()only on actual changesThe typing
Middleware<{}, RootState>is correct for a middleware that doesn't extend the dispatch signature.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
/retest |
logonoff
force-pushed
the
CONSOLE-3769-s3e3-the-coup
branch
from
f6560cf to
faad4ee
Compare
|
@logonoff: This pull request references CONSOLE-3769 which is a valid jira issue. 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. |
logonoff
force-pushed
the
CONSOLE-3769-s3e3-the-coup
branch
2 times, most recently
from
27558e8 to
1adc967
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. |
remove subscribeToDynamicPlugins cause it's not used
hack around coderefs not working by stealing the `Symbol` from upstream
logonoff
force-pushed
the
CONSOLE-3769-s3e3-the-coup
branch
from
1adc967 to
74984ff
Compare
6 participants
This PR updates Console dynamic plugin runtime infrastructure to use lib-core code from OpenShift Dynamic Plugin SDK ("upstream").
Console runtime plugin infrastructure has been updated in to support both current (legacy) and new (upstream) plugin entry schemes.
Console plugins built with updated
ConsoleRemotePluginwill now use the defaultentryCallbackSettingsprovided by upstream'sDynamicRemotePlugin.There is one difference between Console legacy and upstream's
entryCallbackSettings:pluginIdno longer includes the version string.When upstream's
PluginLoadertries to load a plugin, it makes the assumption inPluginLoader.loadPluginScriptsthat the plugin name passed to the plugin entry callback always matches the plugin name.However, this precondition was not met previously. To solve this, we introduce a new
window.__load_plugin_entry__global method, which is the default plugin callback provided by upstream. This is now the default plugin entry callback for plugins, starting with 4.22.After upstream
PluginStoreregisters that callback, we register our ownwindow.loadPluginEntrymethod, which strips the version from the plugin name, providing backwards compatibility for plugins built for 4.21 or older.Other notable changes:
subscribeToExtensionsandsubscribeToDynamicPluginssubscription services. React hooks are now the only way to consume extensionsconsole.flagside effects are now handled byFeatureFlagExtensionLoader