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: 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: 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
Outdated
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
5 times, most recently
from
e44485a to
9a03410
Compare
logonoff
force-pushed
the
CONSOLE-3769-s3e3-the-coup
branch
from
9a03410 to
2786de1
Compare
remove subscribeToDynamicPlugins cause it's not used
hack around coderefs not working by stealing the `Symbol` from upstream Update plugin-init.ts
logonoff
force-pushed
the
CONSOLE-3769-s3e3-the-coup
branch
from
2786de1 to
c96f7c3
Compare
Co-Authored-By: Vojtech Szocs <vojtech.szocs@gmail.com>
logonoff
force-pushed
the
CONSOLE-3769-s3e3-the-coup
branch
from
c96f7c3 to
c8406a6
Compare
|
@logonoff: The following tests 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. |
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