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 the legacy Console plugin runtime and store with an SDK-managed PluginStore and loader; migrates lookups to manifest/uid fields; introduces a resource-sidebar samples hook and Jest webpack mock; renames codegen to local-plugins; removes legacy runtime modules and many tests; updates numerous callers and test fixtures. 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
openshift-ci
bot
added
the
plugin-api-changed
logonoff
force-pushed
the
CONSOLE-3769-s3e3-the-coup
branch
from
d5db2f8 to
0122811
Compare
logonoff
force-pushed
the
CONSOLE-3769-s3e3-the-coup
branch
from
0122811 to
12f54a8
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: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
frontend/packages/dev-console/src/components/deployments/__tests__/DeploymentForm.spec.tsx (1)
58-86: Clear module-level mocks in beforeEach to prevent cross-test leakage.
handleSubmitandhandleCancelare declared once at module-level (lines 51–52) and reused across all tests. Without clearing them inbeforeEach, mock call counts accumulate unexpectedly when test execution order changes or tests are refactored. This pattern is already established elsewhere in the codebase (e.g.,operator-lifecycle-manager,knative-plugin).Additionally,
renderWithProviderswraps the component with a sharedPluginStoreProviderusing a singletonpluginStore. Confirm that plugin state does not persist across tests, or reset it inafterEachif needed.Proposed change
beforeEach(() => { + handleSubmit.mockClear(); + handleCancel.mockClear(); // Initialize i18n.services if it doesn't exist if (!i18n.services) { (i18n as any).services = {}; }frontend/packages/console-dynamic-plugin-sdk/README.md (1)
219-224: Minor doc consistency: “Console 4.22.X” → “Console 4.22.x”.Keeps version headings consistent with the rest of the doc/changelog conventions.
frontend/packages/console-dynamic-plugin-sdk/src/coderefs/__tests__/coderef-resolver.spec.ts (1)
145-171: Test name/intent mismatch: it no longer tests “sticky reject after failure”.
Givenjest.fn()can’t passisExecutableCodeRef, this is effectively testing “no CodeRefs found => no calls”, so the name and commentary are misleading. You can still test the intended “second call rejects” behavior withoutjest.fn()by using a plain function + a counter.Jest-30-safe rewrite that actually tests “sticky failure”
- it('continuously reject code refs which have failed to resolve', async () => { - // For this test, we need a jest.fn() to properly track calls, but the - // isExecutableCodeRef check requires exactly one symbol. Since we can't - // modify production code to make it more lenient, we'll use the original - // jest.fn() approach and skip the isExecutableCodeRef assumption in this - // specific test - the production code's behavior is tested elsewhere. - const errorCodeRef = jest.fn<Promise<void>, []>(() => Promise.reject(new Error())); - applyCodeRefSymbol(errorCodeRef); + it('rejects on subsequent resolves after a CodeRef failed once', async () => { + let calls = 0; + const errorCodeRef = applyCodeRefSymbol(() => { + calls++; + return Promise.reject(new Error('boom')); + }); const extension: Extension = { type: 'Foo', properties: { test: true, qux: errorCodeRef }, }; - // Note: With Jest 30, jest.fn() has internal symbols, so isExecutableCodeRef - // returns false, meaning deepForOwn won't find and call our mock through - // the normal resolveExtension flow. This test is effectively testing that - // resolveExtension handles the case where no CodeRefs are found. - // The actual error handling behavior is tested by the production code's - // integration tests. - - expect(isCodeRefError(errorCodeRef)).toBe(false); await resolveExtension(extension); - // The function won't be called because isExecutableCodeRef returns false for jest.fn() - // This is a known limitation with Jest 30's mock functions having internal symbols - expect(errorCodeRef).not.toHaveBeenCalled(); + expect(calls).toBe(1); + expect(isCodeRefError(errorCodeRef)).toBe(true); + + await expect(resolveExtension(extension)).rejects.toThrow('boom'); });frontend/packages/console-dynamic-plugin-sdk/src/coderefs/coderef-resolver.ts (1)
33-37: Remove the overly-strict symbol array check fromisExecutableCodeRef.The exact-symbol-count requirement is unnecessarily brittle and breaks Jest mocks—the test utilities already work around this limitation with a custom
getExecutableCodeRefMock(). SincecodeRefSymbolis private to the SDK, checkingobj[codeRefSymbol] === truealone provides sufficient validation without rejecting wrapped or decorated functions.export const isExecutableCodeRef = (obj): obj is CodeRef => _.isFunction(obj) && - _.isEqual(Object.getOwnPropertySymbols(obj), [codeRefSymbol]) && obj[codeRefSymbol] === true;frontend/public/components/edit-yaml.tsx (1)
215-228: Multi-document YAML validation/save now incorrectly uses a single outer model instead of per-object model resolution.In
saveAllCallback(),validate(obj)is called for each object in the loop, butvalidate()checks the outermodelvariable rather than deriving it from eachobj. Similarly,updateYAML()and the single-save path pass the outermodeltok8sCreate/k8sUpdateinstead of resolving per-object.This breaks the ImportYAML flow where multiple kinds can be pasted without an initial
props.model:
- Namespace logic applies the wrong model's
namespacedproperty across all objectsk8sCreate/k8sUpdatereceive incorrect (ornull) model referencesContrast with
createResources()(line 245), which correctly callsgetModel(obj)per object. Apply the same pattern tovalidate()andupdateYAML(): resolve the model per object rather than relying on the outer scope.
🤖 Fix all issues with AI agents
In
@frontend/packages/console-app/src/components/console-operator/ConsolePluginStatusDetail.tsx:
- Around line 12-15: Guard the useMemo so it skips .find when inputs are
missing: inside the memo for pluginInfo check that pluginInfoEntries is an array
with length and pluginName is truthy before calling pluginInfoEntries.find; e.g.
return undefined early when !Array.isArray(pluginInfoEntries) ||
pluginInfoEntries.length === 0 || !pluginName, otherwise perform
pluginInfoEntries.find((entry) => entry.manifest?.name === pluginName); keep the
same dependency array [pluginInfoEntries, pluginName].
In
@frontend/packages/console-app/src/components/flags/FeatureFlagExtensionLoader.tsx:
- Around line 66-74: The useEffect currently calls onChange(added, removed)
before updating prevExtensionsRef.current, so if onChange throws the ref stays
stale; change the order or wrap the call in try/finally so
prevExtensionsRef.current = extensions always runs (e.g., compute added/removed,
then try { onChange(added, removed) } finally { prevExtensionsRef.current =
extensions }), ensuring prevExtensionsRef, useEffect, added, removed and
onChange are referenced accordingly.
- Around line 14-18: Replace the deep import of types from
'@console/dynamic-plugin-sdk/src/types' with the SDK's public root export:
import Extension, ExtensionTypeGuard and ResolvedExtension from
'@console/dynamic-plugin-sdk' instead of the '/src/types' path; update the
import statement in FeatureFlagExtensionLoader.tsx to use these named types from
the package root so you rely on the public API and match other files that import
ResolvedExtension from '@console/dynamic-plugin-sdk'.
In @frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-init.ts:
- Around line 95-118: The startup race occurs because getSharedScope() is
accessed before initSharedScope() runs (causing "Attempt to access share scope
object before its initialization"); fix by ensuring any module that calls
getSharedScope eagerly (e.g., the pluginStore in frontend/public/plugins.ts) is
created only after initSharedScope() completes or by making getSharedScope()
lazy: change the pluginStore instantiation to be deferred (factory function or
exported getter invoked after initConsolePlugins resolves) or change
getSharedScope() to return a thunk/selector that accesses
__webpack_share_scopes__['default'] only when first used; update
initConsolePlugins (and call sites of dynamicPluginNames/loadAndEnablePlugin) to
await initSharedScope() before any code that instantiates pluginStore or invokes
getSharedScope().
- Around line 18-55: The .catch handler in loadAndEnablePlugin unsafely assumes
the rejection is an ErrorWithCause and accesses err.message and err.cause;
replace the promise .catch with a try/catch around pluginStore.loadPlugin (or
cast err: unknown in the catch) and normalize the rejection by deriving a safe
message (err instanceof Error ? err.message : String(err)) and an optional cause
((err as Partial<ErrorWithCause>)?.cause), then call
onError(`[loadAndEnablePlugin] ${pluginName} loadPlugin failed: ${message}`,
cause); do not return from the catch so the subsequent plugin =
pluginStore.getPluginInfo() and the plugin?.status === 'failed' fallback check
still run.
In @frontend/packages/console-plugin-sdk/src/api/useExtensions.ts:
- Around line 76-95: The current short-circuit uses only UIDs (previousUIDsRef)
so updates that change extension content (e.g., translations) without changing
uid are ignored; update the memo early-return to compare a content signature
instead of just UIDs by replacing previousUIDsRef with a previousSignatureRef
and compute a stable signature from the sorted extensions (e.g., JSON.stringify
of the minimal fields that can change or a lightweight stable serializer) after
sortExtensionsByPluginOrder(translatedExtensions), then if currentSignature ===
previousSignatureRef.current return previousResultRef.current, otherwise update
previousResultRef.current and previousSignatureRef.current and return sorted;
reference symbols: translatedExtensions, sortExtensionsByPluginOrder,
previousResultRef, previousUIDsRef (replace), and useMemo.
In @frontend/packages/console-plugin-sdk/src/api/usePluginInfo.ts:
- Around line 4-15: The equality helper isPluginInfoEqual currently returns true
for any statuses it doesn't explicitly handle, which can hide real changes when
new statuses are introduced; update isPluginInfoEqual so that after checking
known statuses ('loaded' and 'failed') it returns false for any other/unhandled
status values (i.e., treat unknown statuses as unequal) to force re-comparison
and preserve referential stability.
In @frontend/packages/console-plugin-sdk/src/api/usePluginStore.ts:
- Line 1: The file currently re-exports a non-existent symbol usePluginStore
from @openshift/dynamic-plugin-sdk causing import failures; either remove this
broken re-export line entirely or replace it with the correct exported symbol
after verifying the SDK (e.g., change to export { CorrectHookName } from
'@openshift/dynamic-plugin-sdk' if the SDK exposes a differently named hook),
and ensure consumers are updated to import the correct symbol and that the
project builds/tests pass.
In @frontend/packages/console-plugin-sdk/src/utils/allowed-plugins.ts:
- Around line 4-17: getEnabledDynamicPluginNames can throw at module load if
window.SERVER_FLAGS or window.location are undefined and fails to handle
whitespace in the disable-plugins param; update the function to safely read
globals using optional chaining and nullish coalescing (e.g.,
window.SERVER_FLAGS?.consolePlugins ?? []), build URLSearchParams from a safe
fallback (new URLSearchParams(window?.location?.search ?? '')), parse the
disable-plugins value by splitting on ',' then trimming and filtering out empty
strings, and use a Set for deduplication and O(1) lookups when filtering enabled
plugin names (replace compact/uniq usage with these safeties and Set-based
logic).
In @frontend/packages/console-shared/src/hooks/useResourceSidebarSamples.ts:
- Around line 345-350: The function signature treats kindObj as optional at
runtime but is typed required; update the declaration of
useResourceSidebarSamples to accept an optional parameter (change to kindObj?:
K8sKind) so the runtime null-check is type-safe, and leave the existing
guard/return ({ snippets: [], samples: [] }) in place; also scan callers of
useResourceSidebarSamples to ensure they handle an optional return or pass a
non-null K8sKind where appropriate.
- Around line 352-360: The current yamlSamplesData computation incorrectly gates
on _.isEmpty(yamlSamplesList); change it to check the FirehoseResult wrapper
properly by confirming yamlSamplesList?.loaded && !yamlSamplesList?.loadError &&
Array.isArray(yamlSamplesList.data) before filtering, otherwise return []; then
perform the existing filter on yamlSamplesList.data (matching
sample.spec.targetResource.apiVersion === apiVersionForModel(kindObj) &&
sample.spec.targetResource.kind === kindObj.kind) so yamlSamplesData only runs
when the Firehose result is successfully loaded and contains an array.
- Around line 373-377: The comment above the filtering is incorrect: `snippets`
and `samples` are disjoint partitions of `allSamples` based on the `snippet`
flag, not a superset relationship; update the comment near the `const snippets =
allSamples.filter((sample: Sample) => sample.snippet);` / `const samples =
allSamples.filter((sample: Sample) => !sample.snippet);` lines to state that
items with `snippet: true` go to `snippets` and items with `snippet:
false/undefined` go to `samples` (as consumed by ResourceSidebarSamples and
ResourceSidebarSnippets).
In @frontend/public/plugins.ts:
- Around line 35-43: The current fire-and-forget localPlugins.forEach(...) can
cause unhandled promise rejections because pluginStore.loadPlugin is async;
replace the forEach with Promise.allSettled(localPlugins.map(plugin =>
pluginStore.loadPlugin(plugin))) and iterate the settled results to log any
failures (use the same logging pattern as plugin-init.ts), ensuring both
fulfilled and rejected outcomes are handled so errors from loadPlugin are not
leaked.
- Around line 19-33: The pluginStore is constructed at module-eval time using
getSharedScope(), which can throw if __webpack_share_scopes__ isn't initialized;
change the loaderOptions.sharedScope to defer evaluation in test environments by
passing a conditional: when process.env.NODE_ENV === 'test' and a webpack
shared-scope mock is expected, use an empty object (or a safe no-op object)
instead of calling getSharedScope(), otherwise call getSharedScope() as before;
update the pluginStore initialization (the new PluginStore({...}) call and its
loaderOptions.sharedScope) to implement this conditional to avoid import-order
failures during tests.
🧹 Nitpick comments (26)
frontend/packages/operator-lifecycle-manager/src/components/operand/__tests__/index.spec.tsx (1)
223-235: Brittle render-count assertion; assert the “final” call props instead ofCalledTimes(2)React render counts can change (StrictMode/double-invocation, hook behavior, future plugin-store changes). Also, you validate
mock.calls[0](initial render), but your comment implies the post-pluginStore render is the meaningful one.Proposed tweak (less flaky, validates the latest render)
it('configures DetailsPage with Details, YAML, Resources, and Events tabs', () => { renderWithProviders(<OperandDetailsPage />); - // One call for initial render and another for when pluginStore is initialized - expect(mockDetailsPage).toHaveBeenCalledTimes(2); - const [detailsPageProps] = mockDetailsPage.mock.calls[0]; + // pluginStore init can trigger extra render(s); validate latest call + expect(mockDetailsPage).toHaveBeenCalled(); + const [detailsPageProps] = mockDetailsPage.mock.calls.at(-1); expect(detailsPageProps.pages).toHaveLength(4); expect(detailsPageProps.pages[0].nameKey).toEqual('public~Details'); expect(detailsPageProps.pages[1].nameKey).toEqual('public~YAML'); expect(detailsPageProps.pages[2].nameKey).toEqual('olm~Resources'); expect(detailsPageProps.pages[3].nameKey).toEqual('public~Events'); });frontend/packages/console-dynamic-plugin-sdk/src/webpack/ExtensionValidatorPlugin.ts (1)
26-44: Avoid repeatedgetExtensionsFilePath(pkg)calls; compute once per package.
This reduces repetition and guarantees consistenterror.file/dirnameinputs even if the resolver ever changes.Proposed diff
apply(compiler: webpack.Compiler) { compiler.hooks.emit.tap(ExtensionValidatorPlugin.name, (compilation) => { this.options.pluginPackages.forEach((pkg) => { + const extensionsFilePath = getExtensionsFilePath(pkg); const result = new ExtensionValidator(pkg.name).validate( compilation, - parseJSONC<ConsoleExtensionsJSON>(getExtensionsFilePath(pkg)), + parseJSONC<ConsoleExtensionsJSON>(extensionsFilePath), pkg.consolePlugin.exposedModules ?? {}, - path.dirname(getExtensionsFilePath(pkg)), + path.dirname(extensionsFilePath), ); if (result.hasErrors()) { const error = new compiler.webpack.WebpackError( `ExtensionValidator has reported errors for plugin ${pkg.name}`, ); error.details = result.formatErrors(); - error.file = getExtensionsFilePath(pkg); + error.file = extensionsFilePath; compilation.errors.push(error); } }); }); }frontend/packages/topology/src/data-transforms/DataModelExtension.tsx (1)
68-72: Warning message: consider including bothuidand extensionidfor debuggability.Switching the prefix to
Extension "${uid}"aligns with the new nomenclature, butuidcan be opaque in logs; addingid(if it’s user-assigned) can speed up triage.Proposed tweak
- console.warn( - `Extension "${uid}": Could not find model (CRD) for group "${model.group}" and kind "${model.kind}" to determine version. Please add a required flag to the extension to suppress this warning. The resource "${key}" will not be loaded and ignored in the topology view for now.`, - ); + console.warn( + `Extension "${uid}" (id="${id}"): Could not find model (CRD) for group "${model.group}" and kind "${model.kind}" to determine version. Please add a required flag to the extension to suppress this warning. The resource "${key}" will not be loaded and ignored in the topology view for now.`, + );frontend/packages/console-dynamic-plugin-sdk/CHANGELOG-webpack.md (2)
17-19: Clarify deprecation wording and timeline.The deprecation notice has two issues:
Redundant phrasing: "building your plugin with a 4.22 or newer version of
ConsoleRemotePluginor later" contains redundancy—"or newer version" and "or later" convey the same meaning.Vague removal timeline: "Runtime support...will be removed in the future" doesn't give plugin developers a concrete target for migration planning. Consider specifying the target removal version (e.g., 4.24 or 4.25).
📝 Suggested revision
-- **Deprecated**: `loadPluginEntry` callback is deprecated in favor of `__load_plugin_entry__`. Migrate by - building your plugin with a 4.22 or newer version of `ConsoleRemotePlugin` or later. Runtime support for - older plugins built for 4.21 or older will be removed in the future. ([CONSOLE-3769], [#15904]) +- **Deprecated**: `loadPluginEntry` callback is deprecated in favor of `__load_plugin_entry__`. Migrate by + building your plugin with `ConsoleRemotePlugin` version 4.22 or later. Runtime support for older plugins + built for 4.21 or older will be removed in 4.24. ([CONSOLE-3769], [#15904])
15-16: Consider adding context for these breaking version bumps.The webpack ^5.100.0 and TypeScript ^5.9.3 version constraints are valid, but the changelog entry would benefit from brief rationale. For a breaking change affecting all plugin developers, explaining the motivation helps downstream teams assess upgrade priority—whether this is a required fix, new feature adoption, or dependency stabilization.
frontend/packages/console-shared/src/test-utils/unit-test-utils.tsx (1)
2-2: Potential test flake / env-coupling: unit-test utils importing the “real”pluginStore.
If@console/internal/pluginsinitializes the SDKPluginStorewith production loader options (module federation shared scope,consoleFetch, env-driven dependency resolutions, etc.), merely importing this file can introduce side effects and/or brittle assumptions in Jest.Proposed change: allow injecting a test-local PluginStore (or at least overriding the singleton)
-import { PluginStoreProvider } from '@openshift/dynamic-plugin-sdk'; +import { PluginStoreProvider } from '@openshift/dynamic-plugin-sdk'; +// TODO: if the SDK exports the type/class, prefer this for stronger typing: +// import type { PluginStore } from '@openshift/dynamic-plugin-sdk'; import storeHandler from '@console/dynamic-plugin-sdk/src/app/storeHandler'; import { pluginStore } from '@console/internal/plugins'; interface ExtendedRenderOptions extends Omit<RenderOptions, 'queries'> { initialState?: Partial<RootState>; store?: ReturnType<typeof setupStore>; + /** + * Optional override to avoid pulling in the production singleton in tests that + * need a controlled PluginStore instance. + */ + pluginStoreOverride?: typeof pluginStore; } export function renderWithProviders( ui: React.ReactElement, { initialState = {}, // Create a store instance if no custom store was passed in store = setupStore(initialState), + pluginStoreOverride = pluginStore, ...renderOptions }: ExtendedRenderOptions = {}, ) { function Wrapper({ children }: WrapperProps): JSX.Element { return ( <Provider store={store}> - <PluginStoreProvider store={pluginStore}> + <PluginStoreProvider store={pluginStoreOverride}> <MemoryRouter>{children}</MemoryRouter> </PluginStoreProvider> </Provider> ); }Verification idea: ensure
@console/internal/pluginscan be imported in Jest without additional mocks, and that plugin-store state doesn’t leak between tests.Also applies to: 19-19
frontend/packages/console-dynamic-plugin-sdk/src/shared-modules/shared-modules-meta.ts (1)
44-57: Consider making the intended metadata explicit (optional).Right now this relies on defaults (
singleton: true,allowFallback: false). If those defaults are intentional for@openshift/dynamic-plugin-sdk, consider setting them explicitly to make future audits/changes less error-prone.frontend/packages/console-dynamic-plugin-sdk/scripts/package-definitions.ts (1)
90-103:@openshift/*exclusion is fine, but add guardrails for future shared-module additions.Since
parseSharedModuleDepsnow drops all@openshift/*shared modules, future additions under that scope will silently stop being pulled fromrootPackageunless someone also updates the explicitparseDeps(sdkPackage, ...)lists. A short comment or a small unit test around generated manifests would prevent regressions.Proposed minimal clarifying comment
parseDeps( pkg, sharedPluginModules.filter( (m) => !m.startsWith('@openshift-console/') && + // Note: @openshift/* shared modules are sourced explicitly from the upstream SDK package + // (see parseDeps(sdkPackage, ...)) rather than from the Console root package deps. !m.startsWith('@openshift/') && !getSharedModuleMetadata(m).allowFallback, ), missingDepCallback, );frontend/packages/console-plugin-sdk/src/codegen/local-plugins.ts (3)
45-73: Harden generated JS source: avoid raw interpolation forname/version.Even if npm package names/semver are constrained, using
JSON.stringifyhere eliminates any escaping edge cases and makes the generator more obviously safe.Proposed diff
@@ ${output} localPlugins.push({ - name: '${pkg.name}', - version: '${pkg.version}', + name: ${JSON.stringify(pkg.name)}, + version: ${JSON.stringify(pkg.version)}, extensions: ${extensionHook(pkg)}, registrationMethod: 'local', }); `;
136-173: Good API seam:codeRefSourceGetterparameter makesgetDynamicExtensionstestable and more reusable.One small safety tweak: make the placeholder-stripper regex non-greedy to reduce the chance of over-matching if the transformed source ever contains
@.Proposed diff
@@ - ).replace(/"@(.*)@"/g, '$1'); + ).replace(/"@(.*?)@"/g, '$1');
78-96: Consider consolidating extension type sources to reduce drift risk.
LocalPluginManifest.extensions(from@openshift/dynamic-plugin-sdk) accepts the console SDK'sExtension[]type through structural compatibility. While this works today and tests pass, it creates a maintenance hazard: if the upstream SDK evolves the shape ofextensionsor the console SDK refactorsExtension, mismatches could surface at runtime despite tests passing.If feasible, prefer importing both
LocalPluginManifestandExtensionfrom the same package, or add an explicit type adapter layer to clarify the boundary between upstream and console SDKs. This is especially important given thatloadLocalPluginsForTestPurposesis a public test API.frontend/packages/console-plugin-sdk/src/codegen/__tests__/local-plugins.spec.ts (1)
267-360: Test coverage is good for the newcodeRefSourceGetterseam; add one test for the default path (optional).Right now every
getDynamicExtensionstest passes a getter. A small additional test that omits it would lock in “default usesgetExecutableCodeRefSource” behavior and catch accidental signature churn.frontend/packages/console-dynamic-plugin-sdk/src/types.ts (1)
62-65: TheResolvedExtensiontype composition is sound and intentional. The code already documents that it "coercesEtype toLoadedExtensioninterface for runtime consumption" (line 59). The upstream SDK contract is stable under v6.0.0, and usages throughout the codebase (useResolvedExtensions, type guards, test data) confirm the composition type-checks correctly. The brittleness concern is valid in principle—should the SDK tightenLoadedExtensionconstraints in a future major version, this alias would need revision—but this is an acceptable architectural dependency on an external contract. No immediate action required.frontend/public/components/about-modal.tsx (1)
48-52: List key/display: guard against missingmanifest.version(optional).
Ifmanifest.versioncan be absent for any “loaded” entry, the UI will renderundefinedand keys may degrade.Proposed tweak
- key={`${plugin.manifest.name}-${plugin.manifest.version}`} - >{`${plugin.manifest.name} (${plugin.manifest.version})`}</Content> + key={`${plugin.manifest.name}-${plugin.manifest.version ?? 'unknown'}`} + >{`${plugin.manifest.name} (${plugin.manifest.version ?? t('public~unknown')})`}</Content>frontend/public/actions/features.ts (1)
4-8: MakeUserInfoa type-only import to avoid runtime coupling.Proposed change
import { FLAGS } from '@console/shared/src/constants/common'; -import { UserInfo } from '@console/internal/module/k8s'; +import type { UserInfo } from '@console/internal/module/k8s'; import { setUser } from '@console/dynamic-plugin-sdk/src/app/core/actions/core';frontend/public/components/notification-drawer.tsx (1)
213-230: GuardresourcePath(...)before navigating (defensive).
IfresourcePathever returnsundefined,navigate(undefined)will misbehave.Proposed change
onClick={() => { - itemOnClick(resourcePath(referenceForModel(ConsolePluginModel), plugin.manifest.name)); + const path = resourcePath(referenceForModel(ConsolePluginModel), plugin.manifest.name); + if (path) { + itemOnClick(path); + } }}frontend/public/components/sidebars/resource-sidebar.tsx (1)
14-17: Useimport typeforSampleto avoid accidental runtime dependency.Proposed change
import { ExploreType } from './explore-type-sidebar'; import { SimpleTabNav, Tab } from '../utils/simple-tab-nav'; -import { Sample } from '@console/shared/src/hooks/useResourceSidebarSamples'; +import type { Sample } from '@console/shared/src/hooks/useResourceSidebarSamples'; import { Flex, FlexItem, Title } from '@patternfly/react-core';frontend/packages/console-plugin-sdk/src/utils/allowed-plugins.ts (1)
1-3: Avoidlodash+@console/internal/*dependency leakage from the plugin SDK.If
frontend/packages/console-plugin-sdkis intended to be a stable/public surface, importing@console/internal/components/utils/linkandlodash(vslodash-es) is a layering + bundling risk.frontend/packages/console-dynamic-plugin-sdk/src/coderefs/__tests__/coderef-resolver.spec.ts (1)
129-143: Cloning test should also assertpropertiesis cloned and input isn’t mutated.
Right now it only checks the top-level extension reference; it won’t catch accidental mutation ofextensions[i].properties.Proposed test tightening
it('clones the extension instance', async () => { const extensions: Extension[] = [ { type: 'Foo', properties: { test: true }, }, { type: 'Foo', properties: { test: true, qux: getExecutableCodeRefMock('value1') }, }, ]; - expect(await resolveExtension(extensions[0])).not.toBe(extensions[0]); - expect(await resolveExtension(extensions[1])).not.toBe(extensions[1]); + const resolved0 = await resolveExtension(extensions[0]); + const resolved1 = await resolveExtension(extensions[1]); + + expect(resolved0).not.toBe(extensions[0]); + expect(resolved0.properties).not.toBe(extensions[0].properties); + + expect(resolved1).not.toBe(extensions[1]); + expect(resolved1.properties).not.toBe(extensions[1].properties); + + // Ensure input wasn't mutated + expect(extensions[1].properties.qux).toEqual(extensions[1].properties.qux); });frontend/packages/console-app/src/components/console-operator/ConsolePluginManifestPage.tsx (1)
14-20: Slight simplification: memoize the manifest directly (avoid optional-chaining outsideuseMemo).
Current code memoizes the found entry, then optional-chains.manifestoutside; it’s correct but a bit awkward.Possible cleanup
- const pluginManifest = useMemo( - () => pluginInfoEntries.find((entry) => entry.manifest.name === pluginName), - [pluginInfoEntries, pluginName], - )?.manifest; + const pluginManifest = useMemo( + () => pluginInfoEntries.find((entry) => entry.manifest.name === pluginName)?.manifest, + [pluginInfoEntries, pluginName], + );frontend/packages/console-dynamic-plugin-sdk/src/coderefs/coderef-resolver.ts (1)
69-99: Cloning behavior looks correct; consider a fast-path to avoidcloneDeepwhen no CodeRefs exist.
This is potentially hot (runs per extension). IfdeepForOwncan be used to detect CodeRefs first, you could avoidcloneDeepfor the common case.frontend/packages/console-plugin-sdk/src/api/usePluginInfo.ts (1)
42-61: Optional perf: avoid O(n²).findby indexing previous results by manifest name.
Probably fine with few plugins, but easy to tighten.frontend/public/plugins.ts (4)
9-33: Guard againstCURRENT_OPENSHIFT_VERSIONbeing null (especially in production) and considersemver.coerce.Right now
semver(window.SERVER_FLAGS.releaseVersion)can yieldnull, and the production branch would propagate that intofixedPluginDependencyResolutions['@console/pluginAPI'](Line 28-30). If this ever happens, plugin dependency resolution may behave unpredictably (or fail) without a clear error.Proposed fix
-import { valid as semver } from 'semver'; +import { coerce, valid } from 'semver'; /** Set by `console-operator` or `./bin/bridge -release-version` */ -const CURRENT_OPENSHIFT_VERSION = semver(window.SERVER_FLAGS.releaseVersion); +const CURRENT_OPENSHIFT_VERSION = + valid(window.SERVER_FLAGS.releaseVersion) ?? + coerce(window.SERVER_FLAGS.releaseVersion)?.version ?? + null; export const pluginStore = new PluginStore({ loaderOptions: { @@ 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 + ? (() => { + if (!CURRENT_OPENSHIFT_VERSION) { + throw new Error( + `Invalid window.SERVER_FLAGS.releaseVersion: ${String( + window.SERVER_FLAGS.releaseVersion, + )}`, + ); + } + return CURRENT_OPENSHIFT_VERSION; + })() : CURRENT_OPENSHIFT_VERSION || '4.1337.67', }, }, });
21-24: Avoid@ts-expect-errorforfetchImpl: add a typed adapter to match the SDK contract.The suppression (Line 22-23) makes it easy to accidentally drift from the
PluginStorefetch contract and only discover it at runtime.Example adapter pattern
export const pluginStore = new PluginStore({ loaderOptions: { sharedScope: getSharedScope(), - // @ts-expect-error incompatible due to console-specific fetch options - fetchImpl: consoleFetch, + fetchImpl: ((input: RequestInfo | URL, init?: RequestInit) => + consoleFetch(input as any, init as any)) as any,(Adjust the adapter types once you confirm what
consoleFetchand the SDK expect.)
44-53:featureFlagMiddlewareupdates flags on every action; consider change-detection +FLAGSpresence.Calling
pluginStore.setFeatureFlags(...)after every dispatch (Line 50-52) can be noisy, and it assumess.getState().FLAGSalways exists (hard crash otherwise).Proposed fix (cheap change-detection)
export const featureFlagMiddleware = (s: { getState(): RootState }) => (next: any) => ( action: any, ) => { + const prevFlags = (s.getState() as any).FLAGS; const result = next(action); - pluginStore.setFeatureFlags(s.getState().FLAGS.toObject()); + const nextFlags = (s.getState() as any).FLAGS; + if (nextFlags && nextFlags !== prevFlags) { + pluginStore.setFeatureFlags(nextFlags.toObject()); + } return result; };(If
FLAGSis immutable, reference equality is usually enough; otherwise compare serialized output.)
60-65: Consider moving plugin list logs behind non-prod (align with “log only on problems”).This always logs in production (non-test) (Line 60-65). If the intent is quieter prod logs, consider gating to
NODE_ENV !== 'production'or a debug flag.
📜 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 (83)
frontend/__mocks__/webpack.tsfrontend/get-local-plugins.jsfrontend/package.jsonfrontend/packages/console-app/src/__tests__/plugin-test-utils.tsfrontend/packages/console-app/src/components/console-operator/ConsoleOperatorConfig.tsxfrontend/packages/console-app/src/components/console-operator/ConsolePluginDescriptionDetail.tsxfrontend/packages/console-app/src/components/console-operator/ConsolePluginEnabledStatusDetail.tsxfrontend/packages/console-app/src/components/console-operator/ConsolePluginManifestPage.tsxfrontend/packages/console-app/src/components/console-operator/ConsolePluginStatusDetail.tsxfrontend/packages/console-app/src/components/console-operator/ConsolePluginVersionDetail.tsxfrontend/packages/console-app/src/components/dashboards-page/dynamic-plugins-health-resource/DynamicPluginsPopover.tsxfrontend/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/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-dependencies.spec.tsfrontend/packages/console-dynamic-plugin-sdk/src/runtime/__tests__/plugin-loader.spec.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-dynamic-plugin-sdk/src/webpack/ExtensionValidatorPlugin.tsfrontend/packages/console-plugin-sdk/package.jsonfrontend/packages/console-plugin-sdk/src/__tests__/store.spec.tsfrontend/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/__tests__/local-plugins.spec.tsfrontend/packages/console-plugin-sdk/src/codegen/local-plugins.tsfrontend/packages/console-plugin-sdk/src/store.tsfrontend/packages/console-plugin-sdk/src/typings/base.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/about-modal.tsxfrontend/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.tsfrontend/public/redux.tsfrontend/webpack.config.ts
💤 Files with no reviewable changes (27)
- frontend/packages/dev-console/src/utils/tests/useAddActionExtensions.spec.ts
- frontend/packages/console-dynamic-plugin-sdk/src/webpack/ConsoleRemotePlugin.ts
- frontend/packages/console-shared/src/hooks/tests/useTelemetry.spec.ts
- frontend/packages/console-app/src/components/user-preferences/perspective/tests/perspective.data.ts
- frontend/packages/console-dynamic-plugin-sdk/src/perspective/tests/perspective.data.ts
- frontend/packages/console-plugin-sdk/src/typings/base.ts
- 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-dynamic-plugin-sdk/src/runtime/plugin-manifest.ts
- frontend/packages/console-plugin-sdk/src/tests/store.spec.ts
- frontend/packages/console-shared/src/components/actions/tests/utils-test-data.ts
- frontend/public/components/dashboard/project-dashboard/getting-started/tests/SampleGettingStartedCard.data.ts
- frontend/packages/console-app/src/components/user-preferences/tests/userPreferences.data.tsx
- frontend/packages/console-shared/src/utils/sample-utils.ts
- frontend/packages/console-shared/src/components/catalog/tests/CatalogController.spec.tsx
- frontend/packages/dev-console/src/components/add/tests/add-page-test-data.tsx
- frontend/packages/console-dynamic-plugin-sdk/src/runtime/tests/plugin-loader.spec.ts
- frontend/packages/console-dynamic-plugin-sdk/src/runtime/tests/plugin-utils.spec.ts
- frontend/packages/console-shared/src/utils/index.ts
- frontend/packages/console-plugin-sdk/src/store.ts
- frontend/packages/console-plugin-sdk/src/api/pluginSubscriptionService.ts
- frontend/public/components/utils/tests/nav.spec.ts
- frontend/packages/console-dynamic-plugin-sdk/src/runtime/tests/plugin-manifest.spec.ts
- frontend/packages/console-plugin-sdk/package.json
- frontend/packages/console-app/src/components/file-upload/tests/file-upload-utils.spec.ts
- frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-utils.ts
- frontend/packages/console-dynamic-plugin-sdk/src/runtime/tests/plugin-dependencies.spec.ts
🚧 Files skipped from review as they are similar to previous changes (19)
- frontend/packages/console-app/src/components/console-operator/ConsolePluginEnabledStatusDetail.tsx
- frontend/package.json
- frontend/packages/topology/src/data-transforms/DataModelProvider.tsx
- frontend/mocks/webpack.ts
- frontend/get-local-plugins.js
- frontend/packages/knative-plugin/src/components/add/tests/EventSource.spec.tsx
- frontend/packages/console-app/src/hooks/useCSPViolationDetector.tsx
- frontend/packages/console-app/src/components/console-operator/ConsoleOperatorConfig.tsx
- frontend/packages/console-app/src/tests/plugin-test-utils.ts
- frontend/packages/console-shared/src/components/formik-fields/CodeEditorField.tsx
- frontend/public/components/app.tsx
- frontend/packages/console-shared/src/components/editor/CodeEditorSidebar.tsx
- frontend/packages/console-dynamic-plugin-sdk/src/app/AppInitSDK.tsx
- frontend/webpack.config.ts
- frontend/packages/console-app/src/components/console-operator/ConsolePluginDescriptionDetail.tsx
- frontend/packages/webterminal-plugin/src/components/cloud-shell/tests/MultiTabbedTerminal.spec.tsx
- frontend/packages/console-shared/locales/en/console-shared.json
- frontend/public/redux.ts
- frontend/packages/console-app/src/components/console-operator/ConsolePluginVersionDetail.tsx
🧰 Additional context used
🧬 Code graph analysis (12)
frontend/packages/console-shared/src/test-utils/unit-test-utils.tsx (1)
frontend/public/plugins.ts (1)
pluginStore(19-33)
frontend/packages/console-dynamic-plugin-sdk/scripts/package-definitions.ts (1)
frontend/packages/console-dynamic-plugin-sdk/src/shared-modules/shared-modules-meta.ts (1)
getSharedModuleMetadata(62-67)
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)
frontend/packages/dev-console/src/components/deployments/__tests__/DeploymentForm.spec.tsx (1)
frontend/packages/console-shared/src/test-utils/unit-test-utils.tsx (1)
renderWithProviders(45-64)
frontend/packages/console-dynamic-plugin-sdk/src/coderefs/coderef-resolver.ts (1)
frontend/packages/console-dynamic-plugin-sdk/src/types.ts (1)
CodeRef(31-31)
frontend/public/components/edit-yaml.tsx (1)
frontend/packages/console-shared/src/hooks/useResourceSidebarSamples.ts (1)
useResourceSidebarSamples(345-378)
frontend/public/plugins.ts (4)
frontend/public/module/auth.js (1)
window(5-12)frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-shared-modules.ts (1)
getSharedScope(18-24)frontend/get-local-plugins.js (1)
require(3-3)frontend/public/redux.ts (1)
RootState(20-28)
frontend/packages/console-plugin-sdk/src/codegen/__tests__/local-plugins.spec.ts (3)
frontend/packages/console-plugin-sdk/src/codegen/local-plugins.ts (3)
getLocalPluginsModule(45-73)loadLocalPluginsForTestPurposes(78-96)getDynamicExtensions(136-173)frontend/packages/console-dynamic-plugin-sdk/src/types.ts (1)
EncodedCodeRef(26-26)frontend/packages/console-plugin-sdk/src/codegen/plugin-resolver.ts (1)
PluginPackage(111-115)
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/public/components/factory/modal.tsx (1)
frontend/public/plugins.ts (1)
pluginStore(19-33)
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/runtime/plugin-init.ts (4)
frontend/public/plugins.ts (1)
pluginStore(19-33)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)
| const pluginInfo = useMemo( | ||
| () => | ||
| pluginInfoEntries.find((entry) => | ||
| entry.status === 'loaded' | ||
| ? entry.metadata.name === pluginName | ||
| : entry.pluginName === pluginName, | ||
| ), | ||
| () => pluginInfoEntries.find((entry) => entry.manifest.name === pluginName), | ||
| [pluginInfoEntries, pluginName], | ||
| ); |
There was a problem hiding this comment.
Add a small guard for pluginName / pluginInfoEntries to avoid edge-case throws.
If usePluginInfo() can temporarily be undefined (or if obj?.metadata?.name is falsy), this .find will either throw or do wasted work.
Proposed tweak
- const pluginInfo = useMemo(
- () => pluginInfoEntries.find((entry) => entry.manifest.name === pluginName),
- [pluginInfoEntries, pluginName],
- );
+ const pluginInfo = useMemo(() => {
+ if (!pluginName) {
+ return undefined;
+ }
+ return pluginInfoEntries?.find((entry) => entry.manifest.name === pluginName);
+ }, [pluginInfoEntries, pluginName]);🤖 Prompt for AI Agents
In
@frontend/packages/console-app/src/components/console-operator/ConsolePluginStatusDetail.tsx
around lines 12 - 15, Guard the useMemo so it skips .find when inputs are
missing: inside the memo for pluginInfo check that pluginInfoEntries is an array
with length and pluginName is truthy before calling pluginInfoEntries.find; e.g.
return undefined early when !Array.isArray(pluginInfoEntries) ||
pluginInfoEntries.length === 0 || !pluginName, otherwise perform
pluginInfoEntries.find((entry) => entry.manifest?.name === pluginName); keep the
same dependency array [pluginInfoEntries, pluginName].
| 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; | ||
| } | ||
| }, [extensions, onChange]); |
There was a problem hiding this comment.
Ensure prevExtensionsRef is updated even if onChange throws.
Right now an exception will leave the old “prev” in place and can cause repeated retries every render.
Proposed hardening
- if (added.length > 0 || removed.length > 0) {
- onChange(added, removed);
- prevExtensionsRef.current = extensions;
- }
+ if (added.length > 0 || removed.length > 0) {
+ try {
+ onChange(added, removed);
+ } finally {
+ prevExtensionsRef.current = extensions;
+ }
+ }📝 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.
| 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; | |
| } | |
| }, [extensions, onChange]); | |
| useEffect(() => { | |
| const added = difference(extensions, prevExtensionsRef.current); | |
| const removed = difference(prevExtensionsRef.current, extensions); | |
| if (added.length > 0 || removed.length > 0) { | |
| try { | |
| onChange(added, removed); | |
| } finally { | |
| prevExtensionsRef.current = extensions; | |
| } | |
| } | |
| }, [extensions, onChange]); |
🤖 Prompt for AI Agents
In
@frontend/packages/console-app/src/components/flags/FeatureFlagExtensionLoader.tsx
around lines 66 - 74, The useEffect currently calls onChange(added, removed)
before updating prevExtensionsRef.current, so if onChange throws the ref stays
stale; change the order or wrap the call in try/finally so
prevExtensionsRef.current = extensions always runs (e.g., compute added/removed,
then try { onChange(added, removed) } finally { prevExtensionsRef.current =
extensions }), ensuring prevExtensionsRef, useEffect, added, removed and
onChange are referenced accordingly.
| 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'; | ||
| }); | ||
|
|
||
| pluginStore.disablePlugins(disablePlugins); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd frontend/packages/console-dynamic-plugin-sdk && find . -type f -name "*.ts" -o -name "*.tsx" | head -20Repository: openshift/console
Length of output: 681
🏁 Script executed:
cd frontend/packages/console-dynamic-plugin-sdk && rg -t ts "class PluginStore|interface PluginStore" -A 10Repository: openshift/console
Length of output: 43
🏁 Script executed:
cd frontend/packages/console-dynamic-plugin-sdk && rg -t ts "loadPlugin" -B 2 -A 5Repository: openshift/console
Length of output: 3320
🏁 Script executed:
cd frontend/packages/console-dynamic-plugin-sdk && rg -t ts "ErrorWithCause" -B 2 -A 3Repository: openshift/console
Length of output: 1245
🏁 Script executed:
cd frontend/packages/console-dynamic-plugin-sdk && rg -t ts "\.catch\(" -B 3 -A 3 | head -60Repository: openshift/console
Length of output: 3658
🏁 Script executed:
cd frontend/packages/console-dynamic-plugin-sdk && rg -t ts "export.*class.*PluginStore|export.*interface.*PluginStore" -A 20Repository: openshift/console
Length of output: 43
🏁 Script executed:
cd frontend/packages/console-dynamic-plugin-sdk && find . -type f -name "*.ts" | xargs rg -l "PluginStore" | head -10Repository: openshift/console
Length of output: 89
🏁 Script executed:
cd frontend/packages/console-dynamic-plugin-sdk/src && find . -type f -name "*plugin*" -o -name "*store*" | grep -E "\.(ts|tsx)$"Repository: openshift/console
Length of output: 170
🏁 Script executed:
cd frontend/packages/console-dynamic-plugin-sdk && head -30 src/runtime/plugin-init.tsRepository: openshift/console
Length of output: 1111
🏁 Script executed:
cd frontend/packages/console-dynamic-plugin-sdk && rg -t ts "PluginStore" -B 5 -A 2Repository: openshift/console
Length of output: 1787
🏁 Script executed:
cd frontend/packages/console-dynamic-plugin-sdk && cat package.json | grep -A 5 "@openshift/dynamic-plugin-sdk"Repository: openshift/console
Length of output: 299
🌐 Web query:
@openshift/dynamic-plugin-sdk PluginStore loadPlugin method signature
💡 Result:
Which version of @openshift/dynamic-plugin-sdk do you need the PluginStore.loadPlugin signature for? (Please provide the package version or a commit/branch.)
🌐 Web query:
@openshift/dynamic-plugin-sdk 6.0.0 PluginStore loadPlugin typescript
💡 Result:
Do you want the TypeScript signature for PluginStore.loadPlugin in @openshift/dynamic-plugin-sdk v6.0.0, an example of using it, or help fixing a specific TypeScript error?
🌐 Web query:
openshift dynamic-plugin-sdk github PluginStore loadPlugin
💡 Result:
Do you want an example of how to call PluginStore.loadPlugin, or help debugging a specific error when calling it?
🏁 Script executed:
find . -type f \( -name "*.spec.ts" -o -name "*.test.ts" -o -name "*.test.tsx" \) | xargs rg -l "loadPlugin" 2>/dev/nullRepository: openshift/console
Length of output: 45
🏁 Script executed:
rg -t ts "\.loadPlugin\(" -B 2 -A 5 --max-count 20Repository: openshift/console
Length of output: 2045
🏁 Script executed:
rg -t ts "Promise.*reject|throw.*Error" src/runtime/plugin-init.ts -B 2 -A 2Repository: openshift/console
Length of output: 127
🏁 Script executed:
cd frontend/packages/console-dynamic-plugin-sdk && rg -t ts "loadPlugin" -B 5 -A 10Repository: openshift/console
Length of output: 5913
🏁 Script executed:
cd frontend/packages/console-dynamic-plugin-sdk && cat src/runtime/plugin-init.ts | head -80Repository: openshift/console
Length of output: 3196
Strengthen error handling in .catch() to handle non-Error rejections.
The type annotation (err: ErrorWithCause) doesn't protect against runtime errors—promises can reject with any value (strings, null, objects without message property, etc.). Accessing err.message and err.cause unsafely can mask failures.
However, the proposed fix's early return would skip the subsequent plugin?.status === 'failed' check, which provides valuable fallback error reporting if the plugin loaded asynchronously into a failed state. Consider removing the return; to allow both error paths to execute:
try {
await pluginStore.loadPlugin(
resolveURL(
`${window.SERVER_FLAGS.basePath}api/plugins/${pluginName}/`,
'plugin-manifest.json',
),
);
} catch (err: unknown) {
const message = err instanceof Error ? err.message : String(err);
const cause = (err as Partial<ErrorWithCause>)?.cause;
onError(`[loadAndEnablePlugin] ${pluginName} loadPlugin failed: ${message}`, cause);
}🤖 Prompt for AI Agents
In @frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-init.ts
around lines 18 - 55, The .catch handler in loadAndEnablePlugin unsafely assumes
the rejection is an ErrorWithCause and accesses err.message and err.cause;
replace the promise .catch with a try/catch around pluginStore.loadPlugin (or
cast err: unknown in the catch) and normalize the rejection by deriving a safe
message (err instanceof Error ? err.message : String(err)) and an optional cause
((err as Partial<ErrorWithCause>)?.cause), then call
onError(`[loadAndEnablePlugin] ${pluginName} loadPlugin failed: ${message}`,
cause); do not return from the catch so the subsequent plugin =
pluginStore.getPluginInfo() and the plugin?.status === 'failed' fallback check
still run.
| export const useResourceSidebarSamples = (kindObj: K8sKind, yamlSamplesList: FirehoseResult) => { | ||
| const defaultSamples = useDefaultSamples(); | ||
|
|
||
| if (!kindObj) { | ||
| return { snippets: [], samples: [] }; | ||
| } |
There was a problem hiding this comment.
Type mismatch: kindObj is required by TS but treated as optional at runtime.
Either make the param optional (kindObj?: K8sKind) or remove the check.
🤖 Prompt for AI Agents
In @frontend/packages/console-shared/src/hooks/useResourceSidebarSamples.ts
around lines 345 - 350, The function signature treats kindObj as optional at
runtime but is typed required; update the declaration of
useResourceSidebarSamples to accept an optional parameter (change to kindObj?:
K8sKind) so the runtime null-check is type-safe, and leave the existing
guard/return ({ snippets: [], samples: [] }) in place; also scan callers of
useResourceSidebarSamples to ensure they handle an optional return or pass a
non-null K8sKind where appropriate.
| const yamlSamplesData = !_.isEmpty(yamlSamplesList) | ||
| ? _.filter( | ||
| yamlSamplesList.data, | ||
| (sample: K8sResourceKind) => | ||
| sample.spec.targetResource.apiVersion === apiVersionForModel(kindObj) && | ||
| sample.spec.targetResource.kind === kindObj.kind, | ||
| ) | ||
| : []; | ||
|
|
There was a problem hiding this comment.
FirehoseResult gating should use loaded/loadError (not isEmpty on the wrapper object).
_.isEmpty(yamlSamplesList) on a FirehoseResult wrapper is misleading and may behave unexpectedly; prefer yamlSamplesList?.loaded && !yamlSamplesList?.loadError && Array.isArray(yamlSamplesList.data).
🤖 Prompt for AI Agents
In @frontend/packages/console-shared/src/hooks/useResourceSidebarSamples.ts
around lines 352 - 360, The current yamlSamplesData computation incorrectly
gates on _.isEmpty(yamlSamplesList); change it to check the FirehoseResult
wrapper properly by confirming yamlSamplesList?.loaded &&
!yamlSamplesList?.loadError && Array.isArray(yamlSamplesList.data) before
filtering, otherwise return []; then perform the existing filter on
yamlSamplesList.data (matching sample.spec.targetResource.apiVersion ===
apiVersionForModel(kindObj) && sample.spec.targetResource.kind === kindObj.kind)
so yamlSamplesData only runs when the Firehose result is successfully loaded and
contains an array.
| // For the time being, `snippets` are a superset of `samples` | ||
| const snippets = allSamples.filter((sample: Sample) => sample.snippet); | ||
| const samples = allSamples.filter((sample: Sample) => !sample.snippet); | ||
|
|
||
| return { snippets, samples }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all usages of useResourceSidebarSamples hook
rg -n 'useResourceSidebarSamples' --type=ts --type=tsxRepository: openshift/console
Length of output: 88
🏁 Script executed:
#!/bin/bash
# Check how snippets and samples are used after destructuring
rg -n -A5 'const.*{.*snippets.*samples' --type=ts --type=tsxRepository: openshift/console
Length of output: 178
🏁 Script executed:
#!/bin/bash
# Look at the hook implementation to understand context
fd -e ts -e tsx "useResourceSidebarSamples" | head -1 | xargs cat -nRepository: openshift/console
Length of output: 16771
🏁 Script executed:
#!/bin/bash
# Find usages of the hook - try without tsx flag
rg -n 'useResourceSidebarSamples' --type=ts -A5Repository: openshift/console
Length of output: 5353
🏁 Script executed:
#!/bin/bash
# Also check if there are any references to samples vs snippets usage patterns
rg -n 'snippets|samples' frontend/public/components/sidebars --type=ts -C2 | head -80Repository: openshift/console
Length of output: 6738
Comment mismatch: snippets and samples are disjoint partitions, not a superset relationship.
The code filters items into two mutually exclusive sets based on the snippet flag. Items with snippet: true go to snippets; items with snippet: false/undefined go to samples. The consuming components (ResourceSidebarSamples and ResourceSidebarSnippets) treat them as distinct collections with separate rendering paths.
Update the comment to accurately describe the partition:
- // For the time being, `snippets` are a superset of `samples`
+ // Partition samples into snippets (items with snippet flag) and full samples
const snippets = allSamples.filter((sample: Sample) => sample.snippet);
const samples = allSamples.filter((sample: Sample) => !sample.snippet);🤖 Prompt for AI Agents
In @frontend/packages/console-shared/src/hooks/useResourceSidebarSamples.ts
around lines 373 - 377, The comment above the filtering is incorrect: `snippets`
and `samples` are disjoint partitions of `allSamples` based on the `snippet`
flag, not a superset relationship; update the comment near the `const snippets =
allSamples.filter((sample: Sample) => sample.snippet);` / `const samples =
allSamples.filter((sample: Sample) => !sample.snippet);` lines to state that
items with `snippet: true` go to `snippets` and items with `snippet:
false/undefined` go to `samples` (as consumed by ResourceSidebarSamples and
ResourceSidebarSnippets).
| // Console local plugins module has its source generated during webpack build, | ||
| // so we use dynamic require() instead of the usual static import statement. | ||
| const activePlugins = | ||
| const localPlugins = | ||
| process.env.NODE_ENV !== 'test' | ||
| ? (require('../get-active-plugins').default as ActivePlugin[]) | ||
| ? (require('../get-local-plugins').default as LocalPluginManifest[]) | ||
| : []; | ||
|
|
||
| const dynamicPluginNames = getEnabledDynamicPluginNames(); | ||
| localPlugins.forEach((plugin) => pluginStore.loadPlugin(plugin)); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the pluginStore definition and the loadPlugin method
fd -t f "\.ts$|\.tsx$" frontend | head -30Repository: openshift/console
Length of output: 1192
🏁 Script executed:
# Search for pluginStore definition
rg "pluginStore" frontend --type ts -l | head -10Repository: openshift/console
Length of output: 590
🏁 Script executed:
# Search for the loadPlugin method signature
rg "loadPlugin\s*\(" frontend --type ts -B 2 -A 5Repository: openshift/console
Length of output: 2045
🏁 Script executed:
# Find the PluginStore class definition
rg "class PluginStore|export.*pluginStore" frontend --type ts -B 2 -A 10 | head -50Repository: openshift/console
Length of output: 2636
🏁 Script executed:
# Search for loadPlugin method definition
rg "loadPlugin\s*\(" frontend --type ts -B 2 -A 10 | grep -A 10 "loadPlugin.*{" | head -40Repository: openshift/console
Length of output: 1018
🏁 Script executed:
# Find PluginStore import source
rg "import.*PluginStore" frontend --type ts -A 1 -B 1Repository: openshift/console
Length of output: 2112
🏁 Script executed:
# Search for PluginStore class definition
rg "class PluginStore" frontend --type ts -A 20 | head -60Repository: openshift/console
Length of output: 43
🏁 Script executed:
# Get the full context of loadPlugin usage in plugin-init.ts
rg -B 5 -A 10 "await pluginStore" frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-init.tsRepository: openshift/console
Length of output: 622
🏁 Script executed:
# Check the loadAndEnablePlugin function for error handling context
rg -B 3 -A 15 "loadAndEnablePlugin" frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-init.ts | head -80Repository: openshift/console
Length of output: 2220
🏁 Script executed:
# Look at the broader context of local plugins loading in plugins.ts
cat -n frontend/public/plugins.ts | head -50Repository: openshift/console
Length of output: 2563
🏁 Script executed:
# Check if there are any tests that exercise local plugin loading failure scenarios
rg -l "localPlugins|get-local-plugins" frontend --type ts -A 5 | head -5Repository: openshift/console
Length of output: 261
🏁 Script executed:
# See if there's any documentation or comments about why local plugins don't need error handling
rg -B 5 "localPlugins.forEach" frontend --type tsRepository: openshift/console
Length of output: 427
🏁 Script executed:
# Check the PluginStore type definition to understand what loadPlugin returns
rg "loadPlugin" frontend/packages/console-dynamic-plugin-sdk/src --type ts -B 2 -A 5 | head -40Repository: openshift/console
Length of output: 4154
🏁 Script executed:
# Check if there are any error logs or tests that show loadPlugin can fail
rg "loadPlugin.*fail|loadPlugin.*error" frontend --type ts -i | head -20Repository: openshift/console
Length of output: 231
Handle async/errors from pluginStore.loadPlugin to prevent unhandled promise rejections.
The loadPlugin method is async (as confirmed by usage patterns elsewhere with await and .catch()). Line 42's forEach creates a fire-and-forget pattern that will leak unhandled promise rejections if any local plugin fails to load. This is inconsistent with how dynamic plugins are loaded in plugin-init.ts, where errors are caught and logged. Use Promise.allSettled() to handle failures:
Suggested approach
-localPlugins.forEach((plugin) => pluginStore.loadPlugin(plugin));
+void Promise.allSettled(localPlugins.map((plugin) => pluginStore.loadPlugin(plugin))).then((r) => {
+ const rejected = r.filter((x) => x.status === 'rejected');
+ if (rejected.length) {
+ // eslint-disable-next-line no-console
+ console.error('Failed to load one or more local plugins', rejected);
+ }
+});📝 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.
| // Console local plugins module has its source generated during webpack build, | |
| // so we use dynamic require() instead of the usual static import statement. | |
| const activePlugins = | |
| const localPlugins = | |
| process.env.NODE_ENV !== 'test' | |
| ? (require('../get-active-plugins').default as ActivePlugin[]) | |
| ? (require('../get-local-plugins').default as LocalPluginManifest[]) | |
| : []; | |
| const dynamicPluginNames = getEnabledDynamicPluginNames(); | |
| localPlugins.forEach((plugin) => pluginStore.loadPlugin(plugin)); | |
| // Console local plugins module has its source generated during webpack build, | |
| // so we use dynamic require() instead of the usual static import statement. | |
| const localPlugins = | |
| process.env.NODE_ENV !== 'test' | |
| ? (require('../get-local-plugins').default as LocalPluginManifest[]) | |
| : []; | |
| void Promise.allSettled(localPlugins.map((plugin) => pluginStore.loadPlugin(plugin))).then((r) => { | |
| const rejected = r.filter((x) => x.status === 'rejected'); | |
| if (rejected.length) { | |
| // eslint-disable-next-line no-console | |
| console.error('Failed to load one or more local plugins', rejected); | |
| } | |
| }); |
🤖 Prompt for AI Agents
In @frontend/public/plugins.ts around lines 35 - 43, The current fire-and-forget
localPlugins.forEach(...) can cause unhandled promise rejections because
pluginStore.loadPlugin is async; replace the forEach with
Promise.allSettled(localPlugins.map(plugin => pluginStore.loadPlugin(plugin)))
and iterate the settled results to log any failures (use the same logging
pattern as plugin-init.ts), ensuring both fulfilled and rejected outcomes are
handled so errors from loadPlugin are not leaked.
12f54a8 to
610693c
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. |
|
/label px-approved |
|
/label docs-approved |
openshift-ci
bot
added
px-approved
logonoff
force-pushed
the
CONSOLE-3769-s3e3-the-coup
branch
from
610693c to
19f136f
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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/packages/console-dynamic-plugin-sdk/src/coderefs/__tests__/coderef-resolver.spec.ts (1)
145-171: This test does not exercise the intended error handling path.The comments explicitly acknowledge that
errorCodeRefwon't be called becausejest.fn()with Jest 30 has internal symbols that causeisExecutableCodeRefto returnfalse. The assertion on line 170 confirms this—you're verifying the mock was never called.This means the test titled "continuously reject code refs which have failed to resolve" doesn't actually test that behavior. It's essentially testing that
resolveExtensiondoes nothing when no valid CodeRefs are found, which is already implicitly covered.Consider one of these approaches:
- Use
getExecutableCodeRefMockwith a rejecting promise:- const errorCodeRef = jest.fn<Promise<void>, []>(() => Promise.reject(new Error())); - applyCodeRefSymbol(errorCodeRef); + const errorCodeRef = getExecutableCodeRefMock(Promise.reject(new Error('test error')));
Skip or remove the test with a tracking issue if the error-handling behavior is covered by integration tests elsewhere.
Create a proper error-returning mock that passes
isExecutableCodeRef:const createErrorCodeRef = () => { const fn = () => Promise.reject(new Error('test error')); return applyCodeRefSymbol(fn); };Want me to help draft a working version of this test or open an issue to track the test coverage gap?
🤖 Fix all issues with AI agents
In
@frontend/packages/console-app/src/components/console-operator/ConsoleOperatorConfig.tsx:
- Around line 355-365: The code dereferences
notLoadedPluginInfo.status/errorMessage/errorCause without guarding for
undefined; update the logic in ConsoleOperatorConfig (where notLoadedPluginInfo
is used) to check for notLoadedPluginInfo (or use optional chaining and sensible
defaults) before reading .status/.errorMessage/.errorCause so that when
notLoadedPluginInfo is undefined you return enabled/name and set status to
undefined (or a safe default) and error fields to undefined; adjust the
conditional expressions around notLoadedPluginInfo.status === 'failed' to first
ensure notLoadedPluginInfo exists (e.g., notLoadedPluginInfo &&
notLoadedPluginInfo.status === 'failed') or use notLoadedPluginInfo?.status to
avoid null pointer exceptions.
In
@frontend/packages/console-app/src/components/flags/FeatureFlagExtensionLoader.tsx:
- Around line 14-18: The combined import uses an internal path for
ResolvedExtension; split the import so Extension and ExtensionTypeGuard continue
to be imported from the internal /src/types path while ResolvedExtension is
imported from the public SDK export (i.e., import ResolvedExtension from
'@console/dynamic-plugin-sdk'), updating the import statements in the file that
contains the current import of Extension/ExtensionTypeGuard/ResolvedExtension to
use two imports accordingly.
In @frontend/packages/console-dynamic-plugin-sdk/CHANGELOG-webpack.md:
- Around line 17-19: Update the deprecation note for loadPluginEntry to remove
redundancy and add a clearer removal target: replace "4.22 or newer version of
ConsoleRemotePlugin or later" with "ConsoleRemotePlugin version 4.22 or newer
(4.22+)" and change "will be removed in the future" to a concrete target such as
"will be removed in Console ConsoleRemotePlugin 4.23" or, if not fixed, "will be
removed in a future major release"; also add a brief sentence stating whether
prerelease builds (e.g., 4.22.0-prerelease.x) are acceptable for migration to
__load_plugin_entry__ or if users should wait for the GA 4.22 release, and
mention the symbols loadPluginEntry and __load_plugin_entry__ for clarity.
In
@frontend/packages/console-dynamic-plugin-sdk/src/coderefs/coderef-resolver.ts:
- Around line 16-26: Add an inline comment above the codeRefSymbol extraction
documenting the fragile SDK assumption: note that this module intentionally
calls applyCodeRefSymbol(() => Promise.resolve()) and reads the first own Symbol
from the returned function because the SDK uses a private Symbol instance and
applyCodeRefSymbol attaches that single Symbol as an own property; explain that
this is required to detect CodeRefs created by the SDK and that it relies on
applyCodeRefSymbol setting exactly one Symbol (fail-fast behavior is
intentional).
In @frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-init.ts:
- Around line 52-56: The log is emitted even when disablePlugins is empty,
producing a misleading message; update the code around the console.log in
plugin-init.ts to only emit the message when disablePlugins.length > 0 (e.g.,
wrap the console.log(`Static plugin ${disablePlugins.join(', ')} will be
disabled`) in a guard that checks disablePlugins.length > 0) so you don't log
the empty case (you can leave the pluginStore.disablePlugins(disablePlugins)
call as-is or similarly guard it if desired).
- Around line 72-90: The legacy shim in registerLegacyPluginEntryCallback calls
window['__load_plugin_entry__'] directly which can throw if the SDK callback
isn't registered; change the invocation to first check existence (e.g., optional
chaining or an if guard) and call it safely with the patchedPluginName and
entryModule, and if the callback is missing emit a clear console/processLogger
error mentioning '__load_plugin_entry__' and the plugin name to aid debugging.
In @frontend/packages/console-shared/src/hooks/useResourceSidebarSamples.ts:
- Around line 257-270: lazyYaml assumes extension.properties.label, description,
and id are strings and calls string methods directly; add defensive fallbacks by
coercing or defaulting these fields before use (e.g., const { id, label,
description } = extension.properties; const safeId = String(id ?? ''); const
safeLabel = String(label ?? ''); const safeDescription = String(description ??
''); then use safeId in the sort and safeLabel/safeDescription for
splitting/joining). Update references inside lazyYaml and the sort comparator to
use these safe variables so non-string or undefined properties won't throw at
runtime.
- Around line 362-369: The type utility used when mapping yamlSamplesData is
wrong: replace Exclude<Sample, 'id'> with Omit<Sample, 'id'> so that the mapped
object spreads the Sample fields except id correctly; update the mapping in
extensionSamples (inside useResourceSidebarSamples.ts where yamlSamplesData is
mapped from K8sResourceKind to Sample) to use Omit<Sample, 'id'> to fix the
TypeScript error and preserve proper shape for the spread.
In @frontend/public/plugins.ts:
- Around line 10-11: CURRENT_OPENSHIFT_VERSION is assigned directly via
semver(window.SERVER_FLAGS.releaseVersion) which can be null for
malformed/missing versions and later passed to fixedPluginDependencyResolutions;
change to use semver.valid(window.SERVER_FLAGS.releaseVersion) (or semver.coerce
if you prefer best-effort) and if it returns null, emit a defensive
console/process log and set CURRENT_OPENSHIFT_VERSION to a safe fallback (e.g.,
undefined or a default version string like "0.0.0") before calling
fixedPluginDependencyResolutions so the production path never receives null.
🧹 Nitpick comments (12)
frontend/public/components/about-modal.tsx (1)
40-54: Migration to manifest-based fields looks correct.The shift from
metadatatomanifestfields aligns with the SDK migration objectives. The composite key${plugin.manifest.name}-${plugin.manifest.version}is appropriate for React list rendering, assuming the SDK guarantees uniqueness for loaded plugins.Minor observations:
- The optional chaining on
sortedLoadedPlugins?.map(line 46) is unnecessary sincesort()always returns an array—consider removing the?for clarity.- The
useState([])could benefit from explicit typing (e.g.,useState<React.ReactNode[]>([])) for better maintainability.Neither is blocking; the core logic is sound.
♻️ Optional cleanup
- const [items, setItems] = useState([]); + const [items, setItems] = useState<React.ReactNode[]>([]);- setItems( - sortedLoadedPlugins?.map((plugin) => { + setItems( + sortedLoadedPlugins.map((plugin) => {frontend/public/actions/features.ts (1)
74-82: Silent failure when parsinguserInfo.extramay mask issues.When JSON parsing fails, the error is logged but
newUserInfo.extraremainsundefined. This is likely intentional, but consider whether downstream consumers expectextrato be present whenuserInfo.extrais truthy. If the raw string is acceptable as a fallback, you could assign it; otherwise, this silent degradation is fine but worth documenting.💡 Optional: Assign raw value as fallback or add inline comment
if (userInfo.extra) { try { newUserInfo.extra = JSON.parse(userInfo.extra); } catch (error) { // eslint-disable-next-line no-console console.error('Error parsing UserInfo JSON:', error); + // extra remains undefined; downstream must handle missing extra gracefully } }frontend/packages/console-plugin-sdk/src/codegen/local-plugins.ts (1)
55-65: Consider escaping version string for code generation safety.The
pkg.versionis interpolated directly into generated JavaScript. While npm enforces semver (which shouldn't contain problematic characters), for defensive code generation it's good practice to escape strings being injected into source code—especially as this pattern might be copied for other fields in the future.♻️ Optional: escape the version string
localPlugins.push({ name: '${pkg.name}', - version: '${pkg.version}', + version: '${pkg.version.replace(/\\/g, '\\\\').replace(/'/g, "\\'")}', extensions: ${extensionHook(pkg)}, registrationMethod: 'local', });Alternatively, a small
escapeStringForJSutility could centralize this pattern if used elsewhere. This is a nitpick given semver constraints, but hardens against edge cases.frontend/packages/console-dynamic-plugin-sdk/src/app/AppInitSDK.tsx (1)
24-34: JSDoc example does not document the newdynamicPluginsconfiguration.The example shows
configurations={{ appFetch, apiDiscovery }}but omits the newly introduceddynamicPluginscallback. Consider updating to reflect the current API surface for SDK consumers.📖 Suggested documentation update
* @example * ```ts * return ( * <Provider store={store}> - * <AppInitSDK configurations={{ appFetch, apiDiscovery }}> + * <AppInitSDK configurations={{ appFetch, apiDiscovery, dynamicPlugins }}> * <CustomApp /> * ... * </AppInitSDK> * </Provider> * ) * ```frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-init.ts (2)
104-110: Fire-and-forget async loading may obscure initialization completion.
loadAndEnablePluginis async but invoked viaforEachwithout awaiting. This means:
- Plugin load order isn't deterministic.
initConsolePluginsreturns before plugins finish loading.If parallel loading is intentional for performance, consider using
Promise.allSettledto surface aggregate completion/errors more cleanly. If sequential loading is needed (e.g., for dependency ordering), await each call.♻️ Option: Parallel with completion tracking
- dynamicPluginNames.forEach((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( + 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)}`; + }), + ), + ).then((results) => { + const failures = results.filter((r) => r.status === 'rejected'); + if (failures.length > 0) { + // eslint-disable-next-line no-console + console.warn(`${failures.length} plugin(s) failed to load`); + } + });
106-108: String concatenation forwindow.windowErrormay accumulate unbounded content.Appending error messages via string concatenation could result in a very long string if multiple plugins fail. Consider using an array or limiting the number of captured errors.
frontend/packages/console-plugin-sdk/src/utils/allowed-plugins.ts (1)
11-16: Inconsistent deduplication between branchesWhen
disabledPluginsis falsy (line 12), you returnallPluginNamesdirectly withoutuniq(). However, when filtering (line 16), you applyuniq(allPluginNames). This means duplicate plugin names inSERVER_FLAGS.consolePluginsare preserved in the "all plugins enabled" path but removed when any plugin is disabled—leading to different behavior depending on query params.Consider applying
uniqconsistently:♻️ Suggested fix
const getEnabledDynamicPluginNames = () => { - const allPluginNames = window.SERVER_FLAGS.consolePlugins; + const allPluginNames = uniq(window.SERVER_FLAGS.consolePlugins ?? []); const disabledPlugins = getURLSearchParams()['disable-plugins']; if (disabledPlugins === '') { return []; } if (!disabledPlugins) { return allPluginNames; } const disabledPluginNames = compact(disabledPlugins.split(',')); - return uniq(allPluginNames).filter((pluginName) => !disabledPluginNames.includes(pluginName)); + return allPluginNames.filter((pluginName) => !disabledPluginNames.includes(pluginName)); };This also adds defensive handling for a potentially undefined
consolePluginsarray.frontend/packages/console-plugin-sdk/src/api/usePluginInfo.ts (1)
42-61: Consider: Ref mutation inside useMemo.Mutating
previousResultRef.currentinsideuseMemo(line 59) is technically a side effect during render. While this pattern works in practice for referential stability, React's concurrent features could theoretically call the memo function multiple times per commit.The pattern is pragmatic and commonly used, so flagging as optional. An alternative would be to use a separate
useEffectto sync the ref after render, but that introduces a one-render delay which may not be desirable here.frontend/public/plugins.ts (1)
36-43: Consider error handling for local plugin loading.
pluginStore.loadPlugin()could potentially throw or reject for malformed manifests. Currently, a single bad plugin manifest would break the entire initialization sequence.Wrap in try-catch for resilience
-localPlugins.forEach((plugin) => pluginStore.loadPlugin(plugin)); +localPlugins.forEach((plugin) => { + try { + pluginStore.loadPlugin(plugin); + } catch (e) { + // eslint-disable-next-line no-console + console.error(`Failed to load local plugin "${plugin.name}":`, e); + } +});frontend/packages/console-shared/src/hooks/useResourceSidebarSamples.ts (3)
1-4: Prefer named imports from lodash for tree-shaking.The namespace import
import * as _ from 'lodash'pulls the entire lodash bundle. Since onlyisEmptyandfilterare used, named imports would allow bundlers to tree-shake unused code.Use named imports
-import * as _ from 'lodash'; +import { filter, isEmpty } from 'lodash';Then update usages:
_.isEmpty(...)→isEmpty(...)_.filter(...)→filter(...)
345-378: Consider memoizing results to prevent unnecessary re-renders.
useResourceSidebarSamplesreturns new array references on every call, which could cause downstream components to re-render even when the underlying data hasn't changed. TheuseDefaultSamples()call also invokes multipleuseResolvedExtensionshooks internally.Wrap with useMemo for stability
+import { useMemo } from 'react'; + export const useResourceSidebarSamples = (kindObj: K8sKind, yamlSamplesList: FirehoseResult) => { const defaultSamples = useDefaultSamples(); if (!kindObj) { return { snippets: [], samples: [] }; } - const yamlSamplesData = !_.isEmpty(yamlSamplesList) + return useMemo(() => { + const yamlSamplesData = !isEmpty(yamlSamplesList) ? _.filter( yamlSamplesList.data, (sample: K8sResourceKind) => sample.spec.targetResource.apiVersion === apiVersionForModel(kindObj) && sample.spec.targetResource.kind === kindObj.kind, ) : []; - const existingSamples = defaultSamples.get(referenceForModel(kindObj)) || []; - // ... rest of logic ... - - return { snippets, samples }; + const existingSamples = defaultSamples.get(referenceForModel(kindObj)) || []; + // ... rest of logic ... + return { snippets, samples }; + }, [defaultSamples, kindObj, yamlSamplesList]); };
93-343: Large hook with multiple extension subscriptions—consider extraction.
useDefaultSamplesis ~250 lines and calls threeuseResolvedExtensionshooks. While functionally correct, this could be split for maintainability:
- Extract model-specific sample definitions into separate constants or smaller hooks.
- The
ConsoleOperatorConfigModelsamples (lines 227-320) are particularly complex with theirlazyYamllogic.This is a good-to-have refactor if the file grows further.
📜 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 (84)
frontend/__mocks__/webpack.tsfrontend/get-local-plugins.jsfrontend/package.jsonfrontend/packages/console-app/src/__tests__/plugin-test-utils.tsfrontend/packages/console-app/src/components/console-operator/ConsoleOperatorConfig.tsxfrontend/packages/console-app/src/components/console-operator/ConsolePluginDescriptionDetail.tsxfrontend/packages/console-app/src/components/console-operator/ConsolePluginEnabledStatusDetail.tsxfrontend/packages/console-app/src/components/console-operator/ConsolePluginManifestPage.tsxfrontend/packages/console-app/src/components/console-operator/ConsolePluginStatusDetail.tsxfrontend/packages/console-app/src/components/console-operator/ConsolePluginVersionDetail.tsxfrontend/packages/console-app/src/components/dashboards-page/dynamic-plugins-health-resource/DynamicPluginsPopover.tsxfrontend/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-dependencies.spec.tsfrontend/packages/console-dynamic-plugin-sdk/src/runtime/__tests__/plugin-loader.spec.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-dynamic-plugin-sdk/src/webpack/ExtensionValidatorPlugin.tsfrontend/packages/console-plugin-sdk/package.jsonfrontend/packages/console-plugin-sdk/src/__tests__/store.spec.tsfrontend/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/__tests__/local-plugins.spec.tsfrontend/packages/console-plugin-sdk/src/codegen/local-plugins.tsfrontend/packages/console-plugin-sdk/src/store.tsfrontend/packages/console-plugin-sdk/src/typings/base.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/about-modal.tsxfrontend/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.tsfrontend/public/redux.tsfrontend/webpack.config.ts
💤 Files with no reviewable changes (28)
- frontend/packages/console-app/src/components/file-upload/tests/file-upload-utils.spec.ts
- frontend/packages/console-dynamic-plugin-sdk/src/runtime/tests/plugin-utils.spec.ts
- frontend/packages/console-app/src/components/user-preferences/perspective/tests/perspective.data.ts
- frontend/packages/console-plugin-sdk/src/tests/store.spec.ts
- frontend/packages/console-shared/src/utils/index.ts
- frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-manifest.ts
- frontend/packages/console-plugin-sdk/src/typings/base.ts
- frontend/packages/console-dynamic-plugin-sdk/src/perspective/tests/perspective.data.ts
- frontend/packages/console-shared/src/hooks/tests/useTelemetry.spec.ts
- frontend/packages/console-shared/src/components/catalog/tests/CatalogController.spec.tsx
- frontend/packages/console-shared/src/utils/sample-utils.ts
- frontend/packages/console-dynamic-plugin-sdk/src/runtime/tests/plugin-manifest.spec.ts
- frontend/packages/dev-console/src/utils/tests/useAddActionExtensions.spec.ts
- frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-loader.ts
- frontend/packages/console-dynamic-plugin-sdk/src/runtime/tests/plugin-loader.spec.ts
- frontend/public/components/dashboard/project-dashboard/getting-started/tests/SampleGettingStartedCard.data.ts
- frontend/packages/console-plugin-sdk/src/store.ts
- frontend/packages/console-plugin-sdk/package.json
- frontend/packages/console-shared/src/components/actions/tests/utils-test-data.ts
- frontend/public/components/utils/tests/nav.spec.ts
- frontend/packages/console-app/src/components/user-preferences/tests/userPreferences.data.tsx
- frontend/packages/console-dynamic-plugin-sdk/package.json
- frontend/packages/console-dynamic-plugin-sdk/src/runtime/tests/plugin-dependencies.spec.ts
- frontend/packages/console-dynamic-plugin-sdk/src/webpack/ConsoleRemotePlugin.ts
- frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-dependencies.ts
- frontend/packages/dev-console/src/components/add/tests/add-page-test-data.tsx
- frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-utils.ts
- frontend/packages/console-plugin-sdk/src/api/pluginSubscriptionService.ts
🚧 Files skipped from review as they are similar to previous changes (23)
- frontend/packages/operator-lifecycle-manager/src/components/operand/tests/index.spec.tsx
- frontend/packages/console-plugin-sdk/src/api/usePluginStore.ts
- frontend/get-local-plugins.js
- frontend/packages/console-dynamic-plugin-sdk/README.md
- frontend/packages/console-app/src/components/dashboards-page/dynamic-plugins-health-resource/DynamicPluginsPopover.tsx
- frontend/public/redux.ts
- frontend/packages/console-app/src/components/console-operator/ConsolePluginVersionDetail.tsx
- frontend/packages/console-shared/src/components/formik-fields/CodeEditorField.tsx
- frontend/packages/console-app/src/tests/plugin-test-utils.ts
- frontend/package.json
- frontend/packages/console-app/src/components/dashboards-page/dynamic-plugins-health-resource/NotLoadedDynamicPlugins.tsx
- frontend/public/components/app.tsx
- frontend/packages/console-shared/src/test-utils/unit-test-utils.tsx
- frontend/mocks/webpack.ts
- frontend/packages/console-dynamic-plugin-sdk/src/types.ts
- frontend/packages/console-app/src/components/console-operator/ConsolePluginEnabledStatusDetail.tsx
- frontend/packages/console-dynamic-plugin-sdk/scripts/package-definitions.ts
- frontend/webpack.config.ts
- frontend/packages/console-plugin-sdk/src/codegen/tests/local-plugins.spec.ts
- frontend/packages/topology/src/tests/TopologyPage.spec.tsx
- frontend/packages/console-app/src/components/console-operator/ConsolePluginStatusDetail.tsx
- frontend/packages/console-dynamic-plugin-sdk/src/webpack/ExtensionValidatorPlugin.ts
- frontend/packages/console-shared/locales/en/console-shared.json
🧰 Additional context used
🧬 Code graph analysis (15)
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/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-app/src/components/console-operator/ConsolePluginDescriptionDetail.tsx (1)
frontend/packages/console-dynamic-plugin-sdk/src/build-types.ts (1)
ConsolePluginManifest(46-52)
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/public/components/factory/modal.tsx (1)
frontend/public/plugins.ts (1)
pluginStore(20-34)
frontend/packages/knative-plugin/src/components/add/__tests__/EventSource.spec.tsx (1)
frontend/packages/console-shared/src/test-utils/unit-test-utils.tsx (1)
renderWithProviders(45-64)
frontend/packages/console-app/src/components/console-operator/ConsolePluginManifestPage.tsx (1)
frontend/packages/console-plugin-sdk/src/api/usePluginInfo.ts (1)
usePluginInfo(36-62)
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-dynamic-plugin-sdk/src/app/AppInitSDK.tsx (1)
frontend/packages/console-dynamic-plugin-sdk/src/app/k8s/api-discovery/api-discovery.ts (1)
initApiDiscovery(149-159)
frontend/public/plugins.ts (4)
frontend/public/module/auth.js (1)
window(5-12)frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-shared-modules.ts (1)
getSharedScope(18-24)frontend/get-local-plugins.js (1)
require(3-3)frontend/public/redux.ts (1)
RootState(20-28)
frontend/packages/webterminal-plugin/src/components/cloud-shell/__tests__/MultiTabbedTerminal.spec.tsx (2)
frontend/packages/console-shared/src/test-utils/unit-test-utils.tsx (1)
renderWithProviders(45-64)frontend/packages/webterminal-plugin/src/components/cloud-shell/MultiTabbedTerminal.tsx (1)
MultiTabbedTerminal(16-115)
frontend/public/components/edit-yaml.tsx (1)
frontend/packages/console-shared/src/hooks/useResourceSidebarSamples.ts (1)
useResourceSidebarSamples(345-378)
frontend/packages/dev-console/src/components/deployments/__tests__/DeploymentForm.spec.tsx (1)
frontend/packages/console-shared/src/test-utils/unit-test-utils.tsx (1)
renderWithProviders(45-64)
frontend/packages/console-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)
frontend/packages/console-app/src/components/console-operator/ConsoleOperatorConfig.tsx (1)
frontend/packages/console-dynamic-plugin-sdk/src/build-types.ts (1)
ConsolePluginManifest(46-52)
| return { | ||
| name: plugin?.metadata?.name, | ||
| enabled, | ||
| status: notLoadedPluginInfo?.status, | ||
| status: notLoadedPluginInfo.status, | ||
| errorMessage: | ||
| notLoadedPluginInfo?.status === 'failed' ? notLoadedPluginInfo?.errorMessage : undefined, | ||
| notLoadedPluginInfo.status === 'failed' ? notLoadedPluginInfo.errorMessage : undefined, | ||
| errorCause: | ||
| notLoadedPluginInfo?.status === 'failed' | ||
| notLoadedPluginInfo.status === 'failed' | ||
| ? notLoadedPluginInfo?.errorCause?.toString() | ||
| : undefined, | ||
| }; |
There was a problem hiding this comment.
Potential null pointer dereference on notLoadedPluginInfo.
If a ConsolePlugin CR exists but no corresponding entry is found in pluginInfo (neither loaded nor not-loaded), notLoadedPluginInfo will be undefined, causing a runtime error at line 358 when accessing notLoadedPluginInfo.status.
This edge case could occur if:
- A plugin CR was just created but the plugin loader hasn't processed it yet
- There's a mismatch between CR names and manifest names
Proposed fix: Add null guard
if (loadedPluginInfo) {
return {
name: plugin?.metadata?.name,
version: loadedPluginInfo.manifest.version,
description: (loadedPluginInfo.manifest as ConsolePluginManifest)?.customProperties
?.console?.description,
enabled,
status: loadedPluginInfo.status,
hasCSPViolations: cspViolations[loadedPluginInfo.manifest.name] ?? false,
};
}
+ if (!notLoadedPluginInfo) {
+ return {
+ name: plugin?.metadata?.name,
+ enabled,
+ status: 'pending' as PluginInfoEntry['status'],
+ };
+ }
return {
name: plugin?.metadata?.name,
enabled,
status: notLoadedPluginInfo.status,
errorMessage:
notLoadedPluginInfo.status === 'failed' ? notLoadedPluginInfo.errorMessage : undefined,
errorCause:
notLoadedPluginInfo.status === 'failed'
? notLoadedPluginInfo?.errorCause?.toString()
: undefined,
};🤖 Prompt for AI Agents
In
@frontend/packages/console-app/src/components/console-operator/ConsoleOperatorConfig.tsx
around lines 355 - 365, The code dereferences
notLoadedPluginInfo.status/errorMessage/errorCause without guarding for
undefined; update the logic in ConsoleOperatorConfig (where notLoadedPluginInfo
is used) to check for notLoadedPluginInfo (or use optional chaining and sensible
defaults) before reading .status/.errorMessage/.errorCause so that when
notLoadedPluginInfo is undefined you return enabled/name and set status to
undefined (or a safe default) and error fields to undefined; adjust the
conditional expressions around notLoadedPluginInfo.status === 'failed' to first
ensure notLoadedPluginInfo exists (e.g., notLoadedPluginInfo &&
notLoadedPluginInfo.status === 'failed') or use notLoadedPluginInfo?.status to
avoid null pointer exceptions.
| - **Deprecated**: `loadPluginEntry` callback is deprecated in favor of `__load_plugin_entry__`. Migrate by | ||
| building your plugin with a 4.22 or newer version of `ConsoleRemotePlugin` or later. Runtime support for | ||
| older plugins built for 4.21 or older will be removed in the future. ([CONSOLE-3769], [#15904]) |
There was a problem hiding this comment.
Improve migration guidance clarity.
The deprecation notice has minor wording issues:
-
Line 18: "4.22 or newer version of ConsoleRemotePlugin or later" is redundant—"or newer" and "or later" convey the same meaning. Suggest simplifying to "4.22 or newer version of ConsoleRemotePlugin" or "ConsoleRemotePlugin version 4.22+".
-
Line 19: "will be removed in the future" is vague. Consider specifying a target version (e.g., "will be removed in 4.23" or "will be removed in a future major version") to help plugin maintainers prioritize migration work.
-
Optional: Clarify whether prerelease versions (4.22.0-prerelease.x) are sufficient for migration or if developers should wait for GA.
📝 Suggested improvements
-- **Deprecated**: `loadPluginEntry` callback is deprecated in favor of `__load_plugin_entry__`. Migrate by
- building your plugin with a 4.22 or newer version of `ConsoleRemotePlugin` or later. Runtime support for
- older plugins built for 4.21 or older will be removed in the future. ([CONSOLE-3769], [#15904])
+- **Deprecated**: `loadPluginEntry` callback is deprecated in favor of `__load_plugin_entry__`. Migrate by
+ building your plugin with ConsoleRemotePlugin version 4.22 or newer. Runtime support for
+ older plugins built for 4.21 or older will be removed in a future major version. ([CONSOLE-3769], [#15904])📝 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.
| - **Deprecated**: `loadPluginEntry` callback is deprecated in favor of `__load_plugin_entry__`. Migrate by | |
| building your plugin with a 4.22 or newer version of `ConsoleRemotePlugin` or later. Runtime support for | |
| older plugins built for 4.21 or older will be removed in the future. ([CONSOLE-3769], [#15904]) | |
| - **Deprecated**: `loadPluginEntry` callback is deprecated in favor of `__load_plugin_entry__`. Migrate by | |
| building your plugin with ConsoleRemotePlugin version 4.22 or newer. Runtime support for | |
| older plugins built for 4.21 or older will be removed in a future major version. ([CONSOLE-3769], [#15904]) |
🤖 Prompt for AI Agents
In @frontend/packages/console-dynamic-plugin-sdk/CHANGELOG-webpack.md around
lines 17 - 19, Update the deprecation note for loadPluginEntry to remove
redundancy and add a clearer removal target: replace "4.22 or newer version of
ConsoleRemotePlugin or later" with "ConsoleRemotePlugin version 4.22 or newer
(4.22+)" and change "will be removed in the future" to a concrete target such as
"will be removed in Console ConsoleRemotePlugin 4.23" or, if not fixed, "will be
removed in a future major release"; also add a brief sentence stating whether
prerelease builds (e.g., 4.22.0-prerelease.x) are acceptable for migration to
__load_plugin_entry__ or if users should wait for the GA 4.22 release, and
mention the symbols loadPluginEntry and __load_plugin_entry__ for clarity.
frontend/packages/console-dynamic-plugin-sdk/src/coderefs/coderef-resolver.ts
Show resolved
Hide resolved
| const registerLegacyPluginEntryCallback = () => { | ||
| /** DEFAULT_REMOTE_ENTRY_CALLBACK in @openshift/dynamic-plugin-sdk */ | ||
| const sdkCallbackName = '__load_plugin_entry__'; | ||
| /** Used in @openshift-console/dynamic-plugin-sdk-webpack 1.0.0 - 4.21.x */ | ||
| const previousConsoleCallbackName = 'loadPluginEntry'; | ||
|
|
||
| if (process.env.NODE_ENV !== 'production') { | ||
| // Expose webpack share scope object for debugging | ||
| window.webpackSharedScope = getSharedScope(); | ||
| } | ||
| }) | ||
| .catch((err) => { | ||
| // eslint-disable-next-line no-console | ||
| console.error('Failed to initialize webpack share scope for dynamic plugins', err); | ||
| window[previousConsoleCallbackName] = (pluginName: string, entryModule: any) => { | ||
| const patchedPluginName = pluginName.includes('@') | ||
| ? pluginName.slice(0, pluginName.lastIndexOf('@')) | ||
| : pluginName; | ||
|
|
||
| window[sdkCallbackName](patchedPluginName, entryModule); | ||
| }; | ||
|
|
||
| // eslint-disable-next-line no-console | ||
| console.info( | ||
| `Legacy plugin entry callback "${previousConsoleCallbackName}" has been registered.`, | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for initConsolePlugins, PluginStore, and __load_plugin_entry__ references
rg -n -C5 'initConsolePlugins|PluginStore|__load_plugin_entry__' --type=ts -g '!*.spec.ts' -g '!*.test.ts' | head -200Repository: openshift/console
Length of output: 16851
🏁 Script executed:
#!/bin/bash
# Find where registerLegacyPluginEntryCallback is called and get full context of initConsolePlugins
rg -n -B5 -A15 'registerLegacyPluginEntryCallback|initConsolePlugins' frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-init.tsRepository: openshift/console
Length of output: 2060
🏁 Script executed:
#!/bin/bash
# Check for error handling around window[sdkCallbackName] calls and look for test files
rg -n 'window\[sdkCallbackName\]|window\.__load_plugin_entry__|loadPluginEntry' --type=ts -g '!*.spec.ts' -g '!*.test.ts' | head -50Repository: openshift/console
Length of output: 459
🏁 Script executed:
#!/bin/bash
# Check test files for plugin-init and look for SDK initialization patterns
find . -name '*plugin-init*.test.ts' -o -name '*plugin-init*.spec.ts' | xargs cat 2>/dev/null | head -150Repository: openshift/console
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Check if there are any comments/docs in the SDK package or look for PluginStore initialization
rg -n '__load_plugin_entry__|DEFAULT_REMOTE_ENTRY_CALLBACK' --type=ts --type=js | head -30Repository: openshift/console
Length of output: 333
Add validation for SDK callback availability before invoking in the legacy shim.
The backward-compatibility shim is well-structured, and the @version suffix stripping correctly handles edge cases. However, line 83 directly invokes window[sdkCallbackName] without checking if the SDK callback is actually registered. While the typical initialization order (pluginStore instantiation → PluginStoreProvider → initConsolePlugins) should ensure __load_plugin_entry__ exists by the time legacy plugins load, missing or delayed SDK registration could cause an unguarded error at runtime.
Consider adding a defensive check before invoking the callback:
window[sdkCallbackName]?.call(window, patchedPluginName, entryModule);Or log an error if the callback is unexpectedly unavailable to improve debuggability for future maintenance.
🤖 Prompt for AI Agents
In @frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-init.ts
around lines 72 - 90, The legacy shim in registerLegacyPluginEntryCallback calls
window['__load_plugin_entry__'] directly which can throw if the SDK callback
isn't registered; change the invocation to first check existence (e.g., optional
chaining or an if guard) and call it safely with the patchedPluginName and
entryModule, and if the callback is missing emit a clear console/processLogger
error mentioning '__load_plugin_entry__' and the plugin name to aid debugging.
| 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.
Defensive check needed for extension properties.
The lazyYaml function assumes label and description are strings and calls .split('\n') on them. If an extension provides undefined or a non-string value, this will throw at runtime.
Add defensive fallbacks
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;
},📝 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 labelComment = (label ?? '').split('\n').join('\n # '); | |
| const descriptionComment = (description ?? '').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, lazyYaml assumes extension.properties.label,
description, and id are strings and calls string methods directly; add defensive
fallbacks by coercing or defaulting these fields before use (e.g., const { id,
label, description } = extension.properties; const safeId = String(id ?? '');
const safeLabel = String(label ?? ''); const safeDescription =
String(description ?? ''); then use safeId in the sort and
safeLabel/safeDescription for splitting/joining). Update references inside
lazyYaml and the sort comparator to use these safe variables so non-string or
undefined properties won't throw at runtime.
remove subscribeToDynamicPlugins cause it's not used
hack around coderefs not working by stealing the `Symbol` from upstream
logonoff
force-pushed
the
CONSOLE-3769-s3e3-the-coup
branch
from
19f136f to
291d0f3
Compare
|
@logonoff: This pull request references CONSOLE-3769 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
frontend/packages/console-app/src/components/console-operator/ConsoleOperatorConfig.tsx (1)
354-366: Potential null reference onnotLoadedPluginInfo.If a
ConsolePluginK8s resource exists but no corresponding entry is found inpluginInfo(neither loaded nor not-loaded),notLoadedPluginInfowill beundefined. Accessing.status,.errorMessage, or.errorCausewould throw.This could occur during a race condition where the K8s resource is created but the plugin hasn't been registered in the store yet.
🛡️ Suggested defensive guard
+ if (!loadedPluginInfo && !notLoadedPluginInfo) { + return { + name: plugin?.metadata?.name, + enabled, + status: 'pending' as const, + }; + } if (loadedPluginInfo) { return { // ... existing code }; } return { name: plugin?.metadata?.name, enabled, - status: notLoadedPluginInfo.status, + status: notLoadedPluginInfo?.status ?? 'pending', errorMessage: - notLoadedPluginInfo.status === 'failed' ? notLoadedPluginInfo.errorMessage : undefined, + notLoadedPluginInfo?.status === 'failed' ? notLoadedPluginInfo.errorMessage : undefined, errorCause: - notLoadedPluginInfo.status === 'failed' + notLoadedPluginInfo?.status === 'failed' ? notLoadedPluginInfo?.errorCause?.toString() : undefined, };frontend/packages/console-dynamic-plugin-sdk/CHANGELOG-core.md (1)
13-21: Complete the migration guidance forpluginIDremoval inupgrade-sdk.md.The breaking change on line 21 references
upgrade-sdk.mdfor migration details, but that file contains only a "TODO" placeholder for version 4.22. Plugin developers reading the changelog will have no guidance on what to use instead ofpluginIDfromuseResolvedExtensionsresults.Based on the codebase pattern (where plugins are identified via
manifest.name), add migration guidance clarifying that developers should use the plugin manifest identifier or the extension's plugin reference instead ofpluginID. Without this, developers upgrading to 4.22 cannot adapt their plugins.frontend/public/actions/features.ts (1)
112-112: Extract HTTP status from GraphQL error extensions correctly to avoid infinite retries.The error object constructed here passes
err.graphQLErrors[0]?.extensionswrapped in{ response: ... }, but GraphQL extensions don't contain a top-levelstatusfield—HTTP status codes are typically nested atextensions?.http?.status(if present at all, depending on server configuration). This meanshandleErrorwill readres?.response?.statusasundefined, which is not in[401, 403, 500], so the condition!_.includes([401, 403, 500], status)evaluates totrueand triggersretryFlagDetectionindefinitely every 15 seconds.SSAR checks should either extract the status from the correct path in extensions or handle GraphQL errors distinctly from HTTP errors. Compare this with
detectOpenShiftanddetectCanCreateProject(lines 29, 56), which pass the full HTTP error object and correctly accessresponse.status.frontend/packages/console-dynamic-plugin-sdk/src/coderefs/__tests__/coderef-resolver.spec.ts (1)
145-171: Test no longer validates the intended behavior.This test acknowledges that
isExecutableCodeRefreturnsfalseforjest.fn()due to internal symbols in Jest 30, meaning the CodeRef is never invoked and the error-handling path is never exercised. The test effectively becomes a no-op that only verifiesresolveExtensionhandles extensions with no recognizable CodeRefs.Consider either:
- Using
getExecutableCodeRefMockwith a rejection to properly test the error path- Removing this test if the behavior is covered by integration tests as the comments suggest
🔧 Suggested fix using getExecutableCodeRefMock
it('continuously reject code refs which have failed to resolve', async () => { - // For this test, we need a jest.fn() to properly track calls, but the - // isExecutableCodeRef check requires exactly one symbol. Since we can't - // modify production code to make it more lenient, we'll use the original - // jest.fn() approach and skip the isExecutableCodeRef assumption in this - // specific test - the production code's behavior is tested elsewhere. - const errorCodeRef = jest.fn<Promise<void>, []>(() => Promise.reject(new Error())); - applyCodeRefSymbol(errorCodeRef); + const error = new Error('test error'); + const errorCodeRef = getExecutableCodeRefMock(Promise.reject(error)); const extension: Extension = { type: 'Foo', properties: { test: true, qux: errorCodeRef }, }; - // Note: With Jest 30, jest.fn() has internal symbols, so isExecutableCodeRef - // returns false, meaning deepForOwn won't find and call our mock through - // the normal resolveExtension flow. This test is effectively testing that - // resolveExtension handles the case where no CodeRefs are found. - // The actual error handling behavior is tested by the production code's - // integration tests. - - expect(isCodeRefError(errorCodeRef)).toBe(false); await resolveExtension(extension); - // The function won't be called because isExecutableCodeRef returns false for jest.fn() - // This is a known limitation with Jest 30's mock functions having internal symbols - expect(errorCodeRef).not.toHaveBeenCalled(); + expect(isCodeRefError(errorCodeRef)).toBe(true); + + // Subsequent resolution should throw + await expect(resolveExtension(extension)).rejects.toThrow(); });
🤖 Fix all issues with AI agents
In @frontend/package.json:
- Line 161: The package.json dependency for "@openshift/dynamic-plugin-sdk"
points to a non-existent version "^6.0.0"; update that dependency entry to a
published version such as "^5.0.1" (or the latest 5.x minor/patch) in
package.json, then reinstall and update the lockfile (npm install or yarn
install) so the correct version is resolved.
In @frontend/packages/console-app/src/__tests__/plugin-test-utils.ts:
- Around line 8-13: The test setup races plugin loading because
testedPlugins.forEach calls testedPluginStore.loadPlugin(plugin) without
awaiting; change to await Promise.all(testedPlugins.map(p =>
testedPluginStore.loadPlugin(p))) so all loadPlugin promises complete before you
call/export testedPluginStore.getExtensions(); ensure the await happens at
module init (or inside an async setup) so testedExtensions is derived after all
plugins finish loading.
In
@frontend/packages/console-app/src/components/flags/FeatureFlagExtensionLoader.tsx:
- Around line 59-75: The diff logic in useExtensionsDiff uses lodash.difference
which compares by object reference and causes spurious added/removed events
because useResolvedExtensions returns a new array each time; update
useExtensionsDiff (inside the useEffect) to compute added as extensions whose
uid is not present in prevExtensionsRef.current and removed as
prevExtensionsRef.current entries whose uid is not present in extensions (i.e.,
replace difference(...) calls with uid-based .filter/.some checks), then call
onChange and set prevExtensionsRef.current = extensions as before.
- Around line 14-18: The code imports Extension and ExtensionTypeGuard from the
SDK's internal path, creating fragile coupling; update
FeatureFlagExtensionLoader.tsx to stop referencing
'@console/dynamic-plugin-sdk/src/types' and instead use only public exports
(e.g., import ResolvedExtension from the SDK's public common-types export) or
work with the SDK team to add public exports for Extension and
ExtensionTypeGuard on the SDK root (index.ts); replace the internal imports for
Extension/ExtensionTypeGuard with the public API types or remove their usage
until the SDK exposes them to avoid depending on internal paths.
In @frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-init.ts:
- Line 108: The concatenation that sets window.windowError in plugin-init.ts can
produce a leading semicolon when window.windowError is initially undefined;
change the logic around the window.windowError assignment so you only prepend a
semicolon when an existing value is non-empty (e.g., check window.windowError
truthiness and either set `${errorMessage}: ${String(errorCause)}` or append
`;${errorMessage}: ${String(errorCause)}`), ensuring no leading semicolon
appears in the resulting string.
In @frontend/packages/console-shared/src/hooks/useResourceSidebarSamples.ts:
- Around line 362-369: The mapping in extensionSamples incorrectly uses Exclude
and unsafely spreads sample.spec; change the cast to use Omit<Sample, 'id'> and
add a runtime/type guard to ensure sample.spec is present and shaped like a
Sample before spreading. Specifically, in the extensionSamples mapping (using
yamlSamplesData and K8sResourceKind), replace "sample.spec as Exclude<Sample,
'id'>" with a safer pattern: assert/cast to Omit<Sample,'id'> only after
checking sample.spec exists (e.g., const spec = sample.spec && typeof
sample.spec === 'object' ? (sample.spec as Omit<Sample,'id'>) : undefined) and
then spread only when spec is defined (e.g., { id: sample.metadata.uid, ...(spec
|| {}) }) so you preserve type safety and avoid spreading invalid values.
In @frontend/public/plugins.ts:
- Around line 10-11: CURRENT_OPENSHIFT_VERSION is being set by calling
semver(...) directly which can become null/invalid; change its initialization to
validate and fallback like the dev path: call
semver.valid(window.SERVER_FLAGS.releaseVersion) (or
semver.validRange/semver.coerce as appropriate) and if that returns null use a
safe placeholder (the same placeholder used in the dev branch) before using it
in fixedPluginDependencyResolutions['@console/pluginAPI']; update any code that
assumes CURRENT_OPENSHIFT_VERSION is always valid so you never pass null into
the SDK.
🧹 Nitpick comments (15)
frontend/packages/operator-lifecycle-manager/src/components/operand/__tests__/index.spec.tsx (1)
226-228: Consider validating the final render state rather than the first.The comment correctly documents the double-render behavior introduced by PluginStore initialization. However, line 228 still accesses
calls[0](the initial render). For test robustness, consider validating the props from the final render to ensure the component's steady-state configuration is correct—even if the page tabs are static today, future changes to the PluginStore integration could affect the final props.Suggested adjustment
// One call for initial render and another for when pluginStore is initialized expect(mockDetailsPage).toHaveBeenCalledTimes(2); - const [detailsPageProps] = mockDetailsPage.mock.calls[0]; + const lastCallIndex = mockDetailsPage.mock.calls.length - 1; + const [detailsPageProps] = mockDetailsPage.mock.calls[lastCallIndex];frontend/packages/console-app/src/components/console-operator/ConsolePluginDescriptionDetail.tsx (2)
12-15: Manifest-based lookup aligns with SDK migration — verify type safety.The switch to
entry.manifest.namefor plugin lookup is consistent with the PR's migration to upstream SDK patterns. However, ifpluginNameisundefined(e.g.,obj?.metadata?.nameis missing), thefindwill still iterate and potentially match an entry with an undefinedmanifest.name. Consider guarding:const pluginInfo = useMemo( - () => pluginInfoEntries.find((entry) => entry.manifest.name === pluginName), + () => pluginName ? pluginInfoEntries.find((entry) => entry.manifest.name === pluginName) : undefined, [pluginInfoEntries, pluginName], );This is a defensive measure — if the contract guarantees
obj.metadata.nameis always present, this can be skipped.
8-9: Consider simplifying thepluginNamememo.
obj?.metadata?.nameis a simple property access with no computation. TheuseMemoadds overhead without meaningful benefit here since the dependency is the same primitive value being returned.♻️ Optional simplification
const ConsolePluginDescriptionDetail: FC<DetailsItemComponentProps> = ({ obj }) => { - const pluginName = useMemo(() => obj?.metadata?.name, [obj?.metadata?.name]); + const pluginName = obj?.metadata?.name;frontend/packages/console-app/src/components/console-operator/ConsolePluginVersionDetail.tsx (1)
10-10: SameuseMemosimplification opportunity as the description component.This is a simple property access; the memo provides no caching benefit.
♻️ Optional simplification
- const pluginName = useMemo(() => obj?.metadata?.name, [obj?.metadata?.name]); + const pluginName = obj?.metadata?.name;frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-init.ts (1)
78-84: Consider defensive check for SDK callback availability.If a legacy plugin's entry script loads and invokes
window.loadPluginEntrybeforePluginStorehas registeredwindow.__load_plugin_entry__, this will throw a runtime error. While the init sequence should prevent this, a defensive check would add resilience:♻️ Proposed defensive check
window[previousConsoleCallbackName] = (pluginName: string, entryModule: any) => { + if (typeof window[sdkCallbackName] !== 'function') { + console.error( + `Legacy plugin "${pluginName}" loaded before SDK callback was registered. This plugin will not be enabled.`, + ); + return; + } + const patchedPluginName = pluginName.includes('@') ? pluginName.slice(0, pluginName.lastIndexOf('@')) : pluginName; window[sdkCallbackName](patchedPluginName, entryModule); };frontend/packages/console-dynamic-plugin-sdk/CHANGELOG-webpack.md (1)
17-19: Clarify deprecation timeline for removal.The deprecation notice states "will be removed in the future" without specifying a target version. As a principal engineer responsible for backward-compatible APIs, I recommend:
- Specify a concrete removal timeline (e.g., "will be removed in Console 4.24")
- Minor wording improvement: "4.22 or newer version of
ConsoleRemotePluginor later" contains redundancy—consider "4.22+ version ofConsoleRemotePlugin"This gives plugin authors a clearer migration window and aligns with semantic versioning best practices for deprecation cycles.
📝 Suggested improvement
-- **Deprecated**: `loadPluginEntry` callback is deprecated in favor of `__load_plugin_entry__`. Migrate by - building your plugin with a 4.22 or newer version of `ConsoleRemotePlugin` or later. Runtime support for - older plugins built for 4.21 or older will be removed in the future. ([CONSOLE-3769], [#15904]) +- **Deprecated**: `loadPluginEntry` callback is deprecated in favor of `__load_plugin_entry__`. Migrate by + building your plugin with a 4.22+ version of `ConsoleRemotePlugin`. Runtime support for + older plugins built for 4.21 or older will be removed in Console 4.24. ([CONSOLE-3769], [#15904])Note: Adjust the target removal version (4.24) based on your actual deprecation policy.
frontend/public/actions/features.ts (1)
75-82: Consider preserving rawextraon JSON parse failure.If
userInfo.extrais a malformed string, the current catch block logs the error but leavesnewUserInfo.extraundefined, silently dropping the data. Depending on how consumers useextra, this might be fine. However, if any downstream code expectsextrato at least be the raw string when parsing fails, consider a fallback:if (userInfo.extra) { try { newUserInfo.extra = JSON.parse(userInfo.extra); } catch (error) { // eslint-disable-next-line no-console console.error('Error parsing UserInfo JSON:', error); + // Optionally preserve raw string if downstream can handle it + // newUserInfo.extra = { raw: userInfo.extra }; } }frontend/public/components/factory/modal.tsx (1)
15-16: Import ordering: external SDK imports should precede local imports.The
PluginStoreProviderimport from@openshift/dynamic-plugin-sdkandpluginStorefrom internal modules are placed after PatternFly and other external imports, but the ordering could be improved for consistency. Consider grouping external SDK imports with other external imports at the top.♻️ Suggested import reordering
import { css } from '@patternfly/react-styles'; import * as Modal from 'react-modal'; import type { SyntheticEvent, FC, ReactNode, ReactElement, ComponentType } from 'react'; import { useCallback } from 'react'; import * as ReactDOM from 'react-dom'; import { Provider } from 'react-redux'; import { Router } from 'react-router-dom'; import { CompatRouter } from 'react-router-dom-v5-compat'; import { useTranslation } from 'react-i18next'; import { ActionGroup, Button, Content, ContentVariants } from '@patternfly/react-core'; +import { PluginStoreProvider } from '@openshift/dynamic-plugin-sdk'; import CloseButton from '@console/shared/src/components/close-button'; import store from '../../redux'; import { ButtonBar } from '../utils/button-bar'; import { history } from '../utils/router'; -import { PluginStoreProvider } from '@openshift/dynamic-plugin-sdk'; import { pluginStore } from '@console/internal/plugins';frontend/packages/console-plugin-sdk/src/api/usePluginInfo.ts (1)
4-15: Consider manifest field changes in equality check.The
isPluginInfoEqualfunction handlesstatus,enabled,disableReason,errorMessage, anderrorCausewell. However, if the manifest itself can change (e.g., during plugin updates), returning the stalepreviousPluginreference would carry outdated manifest data.If manifest immutability is guaranteed for a given plugin name, this is fine. Otherwise, consider adding a manifest comparison or documenting the assumption.
💡 Optional: Add manifest version check
const isPluginInfoEqual = (a: PluginInfoEntry, b: PluginInfoEntry): boolean => { + // Manifest is assumed immutable per plugin name; if this changes, add: + // if (a.manifest.version !== b.manifest.version) return false; if (a.status !== b.status) { return false; }frontend/packages/console-dynamic-plugin-sdk/src/coderefs/coderef-resolver.ts (1)
16-26: Symbol extraction is well-guarded by strict validation—add a comment documenting the SDK coupling.Your extraction technique is sound. The risk is mitigated by
isExecutableCodeRef(line 35), which enforces an exact single-symbol requirement via_.isEqual(Object.getOwnPropertySymbols(obj), [codeRefSymbol]). If the SDK ever adds metadata symbols, extracted CodeRefs would silently fail this check—but that failure would surface immediately in tests and runtime behavior, not in production silently. The module-level throw (lines 24–26) also guards against complete extraction failure.That said, the coupling to
@openshift/dynamic-plugin-sdk's internal symbol is real. A brief comment explaining why this introspection exists and noting the assumption (e.g., "Assumes applyCodeRefSymbol adds exactly one symbol; validated by isExecutableCodeRef's strict equality check") would make future maintainers aware of this contract. This is especially useful given Jest mock functions have internal symbols—tests already document this constraint implicitly.frontend/packages/console-shared/src/hooks/useResourceSidebarSamples.ts (3)
345-378: Hook violates Rules of Hooks with conditional early return after hook calls.The
useDefaultSamples()hook is called on line 346, but there's an early return on lines 348-350 ifkindObjis falsy. This is fine since the hook call precedes the conditional. However, the computed values (yamlSamplesData,existingSamples,extensionSamples,allSamples,snippets,samples) are recalculated on every render without memoization.Consider wrapping the computation in
useMemoto avoid unnecessary recalculations when inputs haven't changed:♻️ Suggested memoization
export const useResourceSidebarSamples = (kindObj: K8sKind, yamlSamplesList: FirehoseResult) => { const defaultSamples = useDefaultSamples(); - if (!kindObj) { - return { snippets: [], samples: [] }; - } - - const yamlSamplesData = !_.isEmpty(yamlSamplesList) + return React.useMemo(() => { + if (!kindObj) { + return { snippets: [], samples: [] }; + } + + const yamlSamplesData = !_.isEmpty(yamlSamplesList) ? _.filter( yamlSamplesList.data, (sample: K8sResourceKind) => sample.spec.targetResource.apiVersion === apiVersionForModel(kindObj) && sample.spec.targetResource.kind === kindObj.kind, ) : []; - - const existingSamples = defaultSamples.get(referenceForModel(kindObj)) || []; - // ... rest of computation - - return { snippets, samples }; + + const existingSamples = defaultSamples.get(referenceForModel(kindObj)) || []; + // ... rest of computation + + return { snippets, samples }; + }, [kindObj, yamlSamplesList, defaultSamples]); };
93-343:useDefaultSamplesrebuilds ImmutableMap on every render.The entire
ImmutableMapwith all samples is reconstructed each time this hook runs, which happens on every render of any component usinguseResourceSidebarSamples. Since the samples are largely static (aside from extension-derived data), consider memoizing:♻️ Suggested memoization
const useDefaultSamples = () => { const { t } = useTranslation('console-shared'); const [addActions] = useResolvedExtensions<AddAction>(isAddAction); const [catalogItemTypes] = useResolvedExtensions<CatalogItemType>(isCatalogItemType); const [perspectives] = useResolvedExtensions<Perspective>(isPerspective); const clusterRoleBindingSamples = useClusterRoleBindingSamples(); - return ImmutableMap<GroupVersionKind, Sample[]>() + return React.useMemo(() => ImmutableMap<GroupVersionKind, Sample[]>() .setIn( [referenceForModel(BuildConfigModel)], // ... all the sample definitions ) - // ... rest of chain + // ... rest of chain + , [t, addActions, catalogItemTypes, perspectives, clusterRoleBindingSamples]); };
1-4: Missing React import for hooks.The file uses React hooks (
useTranslation,useResolvedExtensions) but doesn't importReactoruseMemo. If memoization is added per recommendations, ensure React is imported.frontend/packages/console-plugin-sdk/src/codegen/__tests__/local-plugins.spec.ts (1)
39-43: Consider usingjest.resetAllMocks()for consistency.The current approach iterates over mocks and calls
mockReset()individually. Usingjest.resetAllMocks()would be more concise and automatically handle any new mocks added to the test file.♻️ Suggested simplification
beforeEach(() => { - [parseJSONC, validateConsoleExtensionsFileSchema, fsExistsSyncMock].forEach((mock) => - mock.mockReset(), - ); + jest.resetAllMocks(); });frontend/public/plugins.ts (1)
7-7: Consider renaming the import for clarity.Importing
valid as semvercan be confusing sincesemverconventionally refers to the entire library. Consider a more descriptive alias likesemverValidorparseSemverto clarify intent.-import { valid as semver } from 'semver'; +import { valid as semverValid } from 'semver';Then update Line 11:
-const CURRENT_OPENSHIFT_VERSION = semver(window.SERVER_FLAGS.releaseVersion); +const CURRENT_OPENSHIFT_VERSION = semverValid(window.SERVER_FLAGS.releaseVersion);
📜 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 (84)
frontend/__mocks__/webpack.tsfrontend/get-local-plugins.jsfrontend/package.jsonfrontend/packages/console-app/src/__tests__/plugin-test-utils.tsfrontend/packages/console-app/src/components/console-operator/ConsoleOperatorConfig.tsxfrontend/packages/console-app/src/components/console-operator/ConsolePluginDescriptionDetail.tsxfrontend/packages/console-app/src/components/console-operator/ConsolePluginEnabledStatusDetail.tsxfrontend/packages/console-app/src/components/console-operator/ConsolePluginManifestPage.tsxfrontend/packages/console-app/src/components/console-operator/ConsolePluginStatusDetail.tsxfrontend/packages/console-app/src/components/console-operator/ConsolePluginVersionDetail.tsxfrontend/packages/console-app/src/components/dashboards-page/dynamic-plugins-health-resource/DynamicPluginsPopover.tsxfrontend/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-dependencies.spec.tsfrontend/packages/console-dynamic-plugin-sdk/src/runtime/__tests__/plugin-loader.spec.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-dynamic-plugin-sdk/src/webpack/ExtensionValidatorPlugin.tsfrontend/packages/console-plugin-sdk/package.jsonfrontend/packages/console-plugin-sdk/src/__tests__/store.spec.tsfrontend/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/__tests__/local-plugins.spec.tsfrontend/packages/console-plugin-sdk/src/codegen/local-plugins.tsfrontend/packages/console-plugin-sdk/src/store.tsfrontend/packages/console-plugin-sdk/src/typings/base.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/about-modal.tsxfrontend/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.tsfrontend/public/redux.tsfrontend/webpack.config.ts
💤 Files with no reviewable changes (28)
- frontend/packages/console-shared/src/components/catalog/tests/CatalogController.spec.tsx
- frontend/packages/console-plugin-sdk/src/tests/store.spec.ts
- frontend/packages/console-shared/src/utils/index.ts
- frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-utils.ts
- frontend/packages/console-dynamic-plugin-sdk/src/webpack/ConsoleRemotePlugin.ts
- frontend/packages/console-shared/src/components/actions/tests/utils-test-data.ts
- frontend/public/components/utils/tests/nav.spec.ts
- frontend/packages/console-dynamic-plugin-sdk/src/runtime/tests/plugin-dependencies.spec.ts
- frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-manifest.ts
- frontend/packages/console-app/src/components/user-preferences/perspective/tests/perspective.data.ts
- frontend/packages/console-plugin-sdk/src/typings/base.ts
- frontend/packages/console-shared/src/utils/sample-utils.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
- frontend/packages/console-plugin-sdk/src/api/pluginSubscriptionService.ts
- frontend/packages/console-dynamic-plugin-sdk/src/runtime/tests/plugin-loader.spec.ts
- frontend/packages/console-dynamic-plugin-sdk/package.json
- frontend/packages/console-plugin-sdk/package.json
- 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/tests/plugin-manifest.spec.ts
- frontend/packages/console-dynamic-plugin-sdk/src/runtime/tests/plugin-utils.spec.ts
- frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-dependencies.ts
- frontend/public/components/dashboard/project-dashboard/getting-started/tests/SampleGettingStartedCard.data.ts
- frontend/packages/console-plugin-sdk/src/store.ts
- frontend/packages/dev-console/src/utils/tests/useAddActionExtensions.spec.ts
- frontend/packages/console-app/src/components/file-upload/tests/file-upload-utils.spec.ts
- frontend/packages/console-shared/src/hooks/tests/useTelemetry.spec.ts
🚧 Files skipped from review as they are similar to previous changes (23)
- frontend/packages/console-dynamic-plugin-sdk/src/webpack/ExtensionValidatorPlugin.ts
- frontend/packages/console-dynamic-plugin-sdk/README.md
- frontend/packages/console-dynamic-plugin-sdk/src/shared-modules/shared-modules-meta.ts
- frontend/packages/knative-plugin/src/components/add/tests/EventSource.spec.tsx
- frontend/public/redux.ts
- frontend/packages/console-dynamic-plugin-sdk/src/app/AppInitSDK.tsx
- frontend/packages/console-dynamic-plugin-sdk/scripts/package-definitions.ts
- frontend/packages/console-shared/src/components/formik-fields/CodeEditorField.tsx
- frontend/packages/webterminal-plugin/src/components/cloud-shell/tests/MultiTabbedTerminal.spec.tsx
- frontend/public/components/sidebars/resource-sidebar-samples.tsx
- frontend/packages/console-plugin-sdk/src/api/useExtensions.ts
- frontend/packages/console-shared/locales/en/console-shared.json
- frontend/packages/console-shared/src/test-utils/unit-test-utils.tsx
- frontend/webpack.config.ts
- frontend/public/components/sidebars/resource-sidebar.tsx
- frontend/packages/console-app/src/components/console-operator/ConsolePluginStatusDetail.tsx
- frontend/public/components/about-modal.tsx
- frontend/packages/console-app/src/components/console-operator/ConsolePluginManifestPage.tsx
- frontend/mocks/webpack.ts
- frontend/public/components/app.tsx
- frontend/packages/dev-console/src/components/deployments/tests/DeploymentForm.spec.tsx
- frontend/packages/console-dynamic-plugin-sdk/src/types.ts
- frontend/packages/console-app/src/components/nodes/useNodeStatusExtensions.tsx
🧰 Additional context used
🧬 Code graph analysis (15)
frontend/packages/console-app/src/components/console-operator/ConsoleOperatorConfig.tsx (1)
frontend/packages/console-dynamic-plugin-sdk/src/build-types.ts (1)
ConsolePluginManifest(46-52)
frontend/get-local-plugins.js (1)
frontend/packages/console-plugin-sdk/src/codegen/local-plugins.ts (1)
getLocalPluginsModuleData(175-215)
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-app/src/components/flags/FeatureFlagExtensionLoader.tsx (4)
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/public/actions/flags.ts (1)
updateModelFlags(146-150)
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/public/components/factory/modal.tsx (1)
frontend/public/plugins.ts (1)
pluginStore(20-34)
frontend/packages/console-app/src/components/console-operator/ConsolePluginDescriptionDetail.tsx (1)
frontend/packages/console-dynamic-plugin-sdk/src/build-types.ts (1)
ConsolePluginManifest(46-52)
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/public/components/edit-yaml.tsx (1)
frontend/packages/console-shared/src/hooks/useResourceSidebarSamples.ts (1)
useResourceSidebarSamples(345-378)
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-dynamic-plugin-sdk/src/coderefs/coderef-resolver.ts (1)
frontend/packages/console-dynamic-plugin-sdk/src/types.ts (1)
CodeRef(31-31)
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/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/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/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)
| "@patternfly/react-topology": "^6.2.0", | ||
| "@patternfly/react-user-feedback": "^6.1.0", | ||
| "@patternfly/react-virtualized-extension": "^6.0.0", | ||
| "@openshift/dynamic-plugin-sdk": "^6.0.0", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
@openshift/dynamic-plugin-sdk npm package latest version
💡 Result:
The latest published version of @openshift/dynamic-plugin-sdk is 5.0.1. [1][2]
Sources:
- npm package page. [1]
- upstream repository (package listing). [2]
Dependency version does not exist — use 5.0.1 or latest minor/patch release.
The @openshift/dynamic-plugin-sdk at ^6.0.0 references a version that is not published in npm. The latest available version is 5.0.1. Update the dependency to a version that exists (e.g., ^5.0.1) to ensure npm install succeeds and the SDK is properly resolved.
🤖 Prompt for AI Agents
In @frontend/package.json at line 161, The package.json dependency for
"@openshift/dynamic-plugin-sdk" points to a non-existent version "^6.0.0";
update that dependency entry to a published version such as "^5.0.1" (or the
latest 5.x minor/patch) in package.json, then reinstall and update the lockfile
(npm install or yarn install) so the correct version is resolved.
| const testedPlugins = loadLocalPluginsForTestPurposes(resolvePluginPackages()); | ||
| const testedPluginStore = new PluginStore(); | ||
|
|
||
| testedPlugins.forEach((plugin) => { | ||
| testedPluginStore.loadPlugin(plugin); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
@openshift/dynamic-plugin-sdk PluginStore loadPlugin method signature documentation
💡 Result:
I can look that up — which package name and version do you want documentation for (e.g. @openshift-console/dynamic-plugin-sdk v4.20.0 or @openshift/dynamic-plugin-sdk v5.x)? If you want me to pick the latest published, say “latest” and I’ll fetch the loadPlugin signature and link sources.
🏁 Script executed:
# Search for PluginStore imports and references in the codebase
rg "PluginStore" --type ts --type tsx -A 2 | head -50Repository: openshift/console
Length of output: 88
🏁 Script executed:
# Look for the SDK package.json or any lockfile to identify the exact package
fd "package.json" | xargs grep -l "dynamic-plugin-sdk" | head -5Repository: openshift/console
Length of output: 219
🌐 Web query:
OpenShift Console PluginStore loadPlugin GitHub repository source code
💡 Result:
I can't find a clear match with the query as written. Do you want:
- the source file that implements PluginStore.loadPlugin in the openshift/console repository (I can search that repo for the exact file and return links and code excerpts), or
- an example plugin repository showing PluginStore.loadPlugin usage, or
- something else?
Tell me which and I’ll fetch the exact GitHub file(s).
🏁 Script executed:
# Search for PluginStore class definition in the SDK package
fd -e ts -e js "." frontend/packages/console-dynamic-plugin-sdk | xargs rg "class PluginStore" -A 30Repository: openshift/console
Length of output: 45
🏁 Script executed:
# Alternative: search for loadPlugin method definition
rg "loadPlugin\s*\(" frontend/packages/console-dynamic-plugin-sdk --type ts -A 5 -B 2Repository: openshift/console
Length of output: 814
🏁 Script executed:
# Read the test file to see the complete context around lines 8-15
cat -n frontend/packages/console-app/src/__tests__/plugin-test-utils.ts | head -25Repository: openshift/console
Length of output: 1162
🏁 Script executed:
# Search for the complete usage pattern and getExtensions call
rg "testedPluginStore\|getExtensions" frontend/packages/console-app/src/__tests__/plugin-test-utils.ts -B 2 -A 2Repository: openshift/console
Length of output: 43
🏁 Script executed:
# Check loadLocalPluginsForTestPurposes to understand what plugin format is being used
rg "loadLocalPluginsForTestPurposes" --type ts -B 2 -A 10Repository: openshift/console
Length of output: 7120
🏁 Script executed:
# Also check PluginStore implementation to see if loadPlugin behavior differs for local plugins
fd "PluginStore" frontend/packages/console-dynamic-plugin-sdk/src -type fRepository: openshift/console
Length of output: 231
🏁 Script executed:
# Search for PluginStore class definition in SDK
rg "class PluginStore\|export.*PluginStore" frontend/packages/console-dynamic-plugin-sdk/src -A 30 -B 2Repository: openshift/console
Length of output: 43
🏁 Script executed:
# Search for loadPlugin method signature directly
rg "loadPlugin\s*\(" frontend/packages/console-dynamic-plugin-sdk/src --type ts -B 5 -A 8 | head -100Repository: openshift/console
Length of output: 1586
🏁 Script executed:
# Find PluginStore in dependencies or local SDK
find frontend/packages/console-dynamic-plugin-sdk -name "*.ts" -exec grep -l "class PluginStore" {} \;Repository: openshift/console
Length of output: 43
🏁 Script executed:
# Also check for index files that might export PluginStore
rg "export.*PluginStore" frontend/packages/console-dynamic-plugin-sdk/src -B 3 -A 3Repository: openshift/console
Length of output: 800
🏁 Script executed:
# Check node_modules or import statements to find the SDK package
cat frontend/package.json | grep -A 5 -B 5 "dynamic-plugin-sdk"Repository: openshift/console
Length of output: 1517
🏁 Script executed:
# Search for how loadPlugin is used in test files
rg "loadPlugin" frontend/packages/console-app/src/__tests__ -B 3 -A 3Repository: openshift/console
Length of output: 735
🏁 Script executed:
# Check if there are any async/await patterns in test setup files
rg "async\|await\|Promise" frontend/packages/console-app/src/__tests__/plugin-test-utils.tsRepository: openshift/console
Length of output: 43
🏁 Script executed:
# Check for any jest setup or test hooks that might handle async
rg "beforeEach\|beforeAll" frontend/packages/console-app/src/__tests__ -A 5 | grep -A 5 "plugin"Repository: openshift/console
Length of output: 43
Fix race condition: await plugin loads before exporting extensions.
loadPlugin() returns a Promise (as shown by SDK's await usage in production code), but the test setup calls it in a forEach loop without awaiting. This means getExtensions() is called before any plugins finish loading, resulting in incomplete extension data at module initialization time.
Wrap the plugin loading in Promise.all():
Suggested fix
const testedPlugins = loadLocalPluginsForTestPurposes(resolvePluginPackages());
const testedPluginStore = new PluginStore();
await Promise.all(testedPlugins.map((plugin) => testedPluginStore.loadPlugin(plugin)));
export const testedExtensions = ImmutableList<Extension>(testedPluginStore.getExtensions());🤖 Prompt for AI Agents
In @frontend/packages/console-app/src/__tests__/plugin-test-utils.ts around
lines 8 - 13, The test setup races plugin loading because testedPlugins.forEach
calls testedPluginStore.loadPlugin(plugin) without awaiting; change to await
Promise.all(testedPlugins.map(p => testedPluginStore.loadPlugin(p))) so all
loadPlugin promises complete before you call/export
testedPluginStore.getExtensions(); ensure the await happens at module init (or
inside an async setup) so testedExtensions is derived after all plugins finish
loading.
| import type { | ||
| Extension, | ||
| ExtensionTypeGuard, | ||
| ResolvedExtension, | ||
| } from '@console/dynamic-plugin-sdk/src/types'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check public SDK entry point for type exports
echo "=== Checking public SDK entry point ==="
rg -n "export.*\b(Extension|ExtensionTypeGuard|ResolvedExtension)\b" frontend/packages/console-dynamic-plugin-sdk/src/index.ts
echo ""
echo "=== Checking internal types file ==="
rg -n "export.*\b(Extension|ExtensionTypeGuard|ResolvedExtension)\b" frontend/packages/console-dynamic-plugin-sdk/src/types.ts
echo ""
echo "=== Checking if public index re-exports from types ==="
cat frontend/packages/console-dynamic-plugin-sdk/src/index.ts | head -50Repository: openshift/console
Length of output: 715
🏁 Script executed:
#!/bin/bash
# Check extensions.ts which is re-exported from public index
echo "=== Checking extensions.ts for type definitions ==="
rg -n "export.*\b(Extension|ExtensionTypeGuard|ResolvedExtension)\b" frontend/packages/console-dynamic-plugin-sdk/src/extensions.ts
echo ""
echo "=== Show beginning of extensions.ts ==="
head -80 frontend/packages/console-dynamic-plugin-sdk/src/extensions.ts
echo ""
echo "=== Verify actual import in FeatureFlagExtensionLoader ==="
sed -n '1,25p' frontend/packages/console-app/src/components/flags/FeatureFlagExtensionLoader.tsxRepository: openshift/console
Length of output: 1332
🏁 Script executed:
#!/bin/bash
# Explore SDK structure
echo "=== SDK src directory structure ==="
fd . frontend/packages/console-dynamic-plugin-sdk/src --type f -x basename {} \; | sort | uniq
echo ""
echo "=== Check what extensions are exported from public SDK ==="
rg -n "Extension|ExtensionTypeGuard|ResolvedExtension" frontend/packages/console-dynamic-plugin-sdk/src/index.ts
echo ""
echo "=== Search for type definitions across SDK ==="
rg -l "export.*type.*Extension" frontend/packages/console-dynamic-plugin-sdk/src/
echo ""
echo "=== Try to import from public path in actual file ==="
rg "from '@console/dynamic-plugin-sdk'" frontend/packages/console-app/src/components/flags/FeatureFlagExtensionLoader.tsxRepository: openshift/console
Length of output: 6229
🏁 Script executed:
#!/bin/bash
# See complete index.ts with all exports
echo "=== Complete public SDK index.ts ==="
cat frontend/packages/console-dynamic-plugin-sdk/src/index.ts
echo ""
echo "=== Check feature-flags.ts in extensions ==="
head -80 frontend/packages/console-dynamic-plugin-sdk/src/extensions/feature-flags.ts
echo ""
echo "=== Check if Extension is exported from types.ts ==="
rg -A2 "^export.*Extension\b" frontend/packages/console-dynamic-plugin-sdk/src/types.ts | head -30Repository: openshift/console
Length of output: 2226
🏁 Script executed:
#!/bin/bash
# Find Extension base type definition
echo "=== Find Extension base type in types.ts ==="
rg -B5 "^export.*type Extension" frontend/packages/console-dynamic-plugin-sdk/src/types.ts | head -40
echo ""
echo "=== Check if extensions has an index.ts ==="
if [ -f frontend/packages/console-dynamic-plugin-sdk/src/extensions/index.ts ]; then
echo "=== extensions/index.ts exists ==="
cat frontend/packages/console-dynamic-plugin-sdk/src/extensions/index.ts
else
echo "extensions/index.ts does not exist"
fi
echo ""
echo "=== Search for ExtensionTypeGuard ==="
rg "ExtensionTypeGuard" frontend/packages/console-dynamic-plugin-sdk/src/Repository: openshift/console
Length of output: 2230
🏁 Script executed:
#!/bin/bash
# Find Extension interface definition in types.ts
echo "=== Search for Extension interface definition ==="
rg -B2 "^export.*interface Extension|^export.*type Extension[^a-zA-Z]" frontend/packages/console-dynamic-plugin-sdk/src/types.ts | head -50
echo ""
echo "=== Check what useResolvedExtensions exports ==="
cat frontend/packages/console-dynamic-plugin-sdk/src/api/useResolvedExtensions.ts | head -50
echo ""
echo "=== Try importing these types from public path in a test ==="
# Check if there are any files in console-app importing these types from public path
rg "from '@console/dynamic-plugin-sdk'" frontend/packages/console-app/src/ | head -20Repository: openshift/console
Length of output: 4537
🏁 Script executed:
#!/bin/bash
# Broader search for Extension type definition
echo "=== Search for Extension type definition ==="
rg "^export.*Extension" frontend/packages/console-dynamic-plugin-sdk/src/types.ts | head -20
echo ""
echo "=== Check full types.ts file length and key exports ==="
wc -l frontend/packages/console-dynamic-plugin-sdk/src/types.ts
echo ""
rg "^export" frontend/packages/console-dynamic-plugin-sdk/src/types.ts | head -40
echo ""
echo "=== Check if ResolvedExtension is available from public SDK ==="
# This file imports it successfully from public path
head -20 frontend/packages/console-app/src/utils/perspective.tsxRepository: openshift/console
Length of output: 1551
🏁 Script executed:
#!/bin/bash
# Read complete types.ts file
echo "=== Complete types.ts file ==="
cat frontend/packages/console-dynamic-plugin-sdk/src/types.ts
echo ""
echo "=== Check where Extension is actually imported/defined from in useResolvedExtensions ==="
head -30 frontend/packages/console-dynamic-plugin-sdk/src/api/useResolvedExtensions.ts | grep -A5 "import"Repository: openshift/console
Length of output: 2575
🏁 Script executed:
#!/bin/bash
# Verify if Extension and ExtensionTypeGuard are available from public path
echo "=== Check if Extension is used in public API type signatures ==="
rg "Extension" frontend/packages/console-dynamic-plugin-sdk/src/api/useResolvedExtensions.ts | head -20
echo ""
echo "=== Try to find imports of Extension from public path in console-app ==="
rg "import.*Extension.*from '@console/dynamic-plugin-sdk'" frontend/packages/console-app/src/ | grep -v "/src/types"
echo ""
echo "=== Check package.json or tsconfig to understand public API exposure ==="
cat frontend/packages/console-dynamic-plugin-sdk/package.json | grep -A10 '"types"'
echo ""
echo "=== Verify: Can we change the import in FeatureFlagExtensionLoader to public path? ==="
# Check if it would work by seeing what useResolvedExtensions exports
rg "export.*useResolvedExtensions" frontend/packages/console-dynamic-plugin-sdk/src/index.ts -A5Repository: openshift/console
Length of output: 2399
🏁 Script executed:
#!/bin/bash
# Final verification: Can Extension and ExtensionTypeGuard be imported from public path?
echo "=== Check what TS declaration is generated for public SDK ==="
find frontend/packages/console-dynamic-plugin-sdk -name "*.d.ts" -type f | head -5
echo ""
echo "=== Check if there's a dist/types output ==="
ls -la frontend/packages/console-dynamic-plugin-sdk/dist/ 2>/dev/null | head -20 || echo "No dist directory"
echo ""
echo "=== Review: Does types.ts explicitly export Extension and ExtensionTypeGuard to public API? ==="
grep -A2 "^export type {" frontend/packages/console-dynamic-plugin-sdk/src/types.ts
echo ""
echo "=== Most critically: Check if Extension can be imported from public @console/dynamic-plugin-sdk ==="
# Search for any console-app file that imports Extension (not just ResolvedExtension) from public path
rg "import.*\bExtension\b.*from '@console/dynamic-plugin-sdk'" frontend/packages/console-app/src/ | grep -v "ResolvedExtension\|Extension\[" | head -10Repository: openshift/console
Length of output: 478
🏁 Script executed:
#!/bin/bash
# Final check: trace the export chain to see if Extension reaches public API
echo "=== What does api/common-types export? ==="
head -40 frontend/packages/console-dynamic-plugin-sdk/src/api/common-types.ts
echo ""
echo "=== Does types.ts export reach public API through any re-export? ==="
# Check if any of the public exports re-export from types
rg "from.*types" frontend/packages/console-dynamic-plugin-sdk/src/api/ frontend/packages/console-dynamic-plugin-sdk/src/extensions/ 2>/dev/null | grep "export"
echo ""
echo "=== Conclusion check: import Extension directly from @console/dynamic-plugin-sdk ==="
# Create a test to see if this would work
cat > /tmp/test-import.ts << 'EOF'
import type { Extension, ExtensionTypeGuard, ResolvedExtension } from '@console/dynamic-plugin-sdk';
EOF
echo "Test file created. Now checking if SDK would provide these types via types.ts export..."
echo ""
echo "=== Check api/common-types to see if Extension is there ==="
rg "Extension" frontend/packages/console-dynamic-plugin-sdk/src/api/common-types.ts | head -10Repository: openshift/console
Length of output: 2027
🏁 Script executed:
#!/bin/bash
# Verify which types are actually used in FeatureFlagExtensionLoader
echo "=== Check usage of Extension, ExtensionTypeGuard, ResolvedExtension in the file ==="
cat frontend/packages/console-app/src/components/flags/FeatureFlagExtensionLoader.tsx | grep -E "\bExtension\b|\bExtensionTypeGuard\b|\bResolvedExtension\b"
echo ""
echo "=== Get full file to understand context ==="
wc -l frontend/packages/console-app/src/components/flags/FeatureFlagExtensionLoader.tsxRepository: openshift/console
Length of output: 723
Importing Extension and ExtensionTypeGuard from SDK internals creates fragile coupling.
The file imports Extension and ExtensionTypeGuard directly from @console/dynamic-plugin-sdk/src/types, bypassing the public API surface. These types are not re-exported from the SDK's main entry point (index.ts), which means the SDK could reorganize or deprecate internal paths without notice.
While ResolvedExtension is available through the public common-types export, Extension and ExtensionTypeGuard have no public API path. Either:
- Export these base types from the SDK's public entry point (
index.ts), or - Document
src/typesas a stable internal contract, or - Coordinate with the SDK team on a long-term solution to expose extension fundamentals publicly
This prevents accidental breakage when SDK internals evolve.
🤖 Prompt for AI Agents
In
@frontend/packages/console-app/src/components/flags/FeatureFlagExtensionLoader.tsx
around lines 14 - 18, The code imports Extension and ExtensionTypeGuard from the
SDK's internal path, creating fragile coupling; update
FeatureFlagExtensionLoader.tsx to stop referencing
'@console/dynamic-plugin-sdk/src/types' and instead use only public exports
(e.g., import ResolvedExtension from the SDK's public common-types export) or
work with the SDK team to add public exports for Extension and
ExtensionTypeGuard on the SDK root (index.ts); replace the internal imports for
Extension/ExtensionTypeGuard with the public API types or remove their usage
until the SDK exposes them to avoid depending on internal paths.
| const useExtensionsDiff = <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); | ||
|
|
||
| if (added.length > 0 || removed.length > 0) { | ||
| onChange(added, removed); | ||
| prevExtensionsRef.current = extensions; | ||
| } | ||
| }, [extensions, onChange]); | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the useResolvedExtensions implementation
fd -t f "\.tsx?$" frontend/packages/console-app/src | xargs rg -l "useResolvedExtensions" | head -10Repository: openshift/console
Length of output: 833
🏁 Script executed:
#!/bin/bash
# Search for useResolvedExtensions definition
rg "export.*useResolvedExtensions|const useResolvedExtensions" --type ts --type tsx -A 20 frontend/packages/console-app/src/Repository: openshift/console
Length of output: 88
🏁 Script executed:
#!/bin/bash
# Also check the type definition of ResolvedExtension
rg "type ResolvedExtension|interface ResolvedExtension" --type ts --type tsx -A 10 frontend/packages/console-app/src/Repository: openshift/console
Length of output: 88
🏁 Script executed:
#!/bin/bash
# Check what lodash.difference does with objects - look at the specific file
cat frontend/packages/console-app/src/components/flags/FeatureFlagExtensionLoader.tsx | head -80Repository: openshift/console
Length of output: 2772
🏁 Script executed:
#!/bin/bash
# Find where @console/dynamic-plugin-sdk is defined
fd -t d "dynamic-plugin-sdk" frontend/Repository: openshift/console
Length of output: 106
🏁 Script executed:
#!/bin/bash
# Search for useResolvedExtensions implementation in the entire codebase
rg "const useResolvedExtensions|export.*useResolvedExtensions" -A 30 frontend/Repository: openshift/console
Length of output: 26998
🏁 Script executed:
#!/bin/bash
# Also check the ResolvedExtension type definition
rg "type ResolvedExtension|interface ResolvedExtension" -A 15 frontend/Repository: openshift/console
Length of output: 471
🏁 Script executed:
#!/bin/bash
# Get full useResolvedExtensions implementation
cat frontend/packages/console-dynamic-plugin-sdk/src/api/useResolvedExtensions.tsRepository: openshift/console
Length of output: 1620
🏁 Script executed:
#!/bin/bash
# Check useExtensions to see if it memoizes
rg "const useExtensions|export.*useExtensions" -A 30 frontend/packages/console-dynamic-plugin-sdk/src/Repository: openshift/console
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Search for useExtensions implementation in plugin-sdk
find frontend/packages -name "useExtensions.ts" -o -name "useExtensions.tsx" | head -5Repository: openshift/console
Length of output: 122
🏁 Script executed:
#!/bin/bash
# Look at the extensions array handling more carefully
rg "useExtensions" -B 2 -A 15 frontend/packages/console-dynamic-plugin-sdk/src/api/useResolvedExtensions.tsRepository: openshift/console
Length of output: 1256
🏁 Script executed:
#!/bin/bash
# Check how the feature flag loader uses this to understand the practical impact
cat -n frontend/packages/console-app/src/components/flags/FeatureFlagExtensionLoader.tsx | sed -n '55,100p'Repository: openshift/console
Length of output: 1897
🏁 Script executed:
#!/bin/bash
# Check useExtensions implementation
cat frontend/packages/console-plugin-sdk/src/api/useExtensions.tsRepository: openshift/console
Length of output: 3527
🏁 Script executed:
#!/bin/bash
# Search for any tests of useExtensionsDiff to see if there are issues
rg "useExtensionsDiff" -B 3 -A 10 frontend/packages/console-app/src/Repository: openshift/console
Length of output: 4885
🏁 Script executed:
#!/bin/bash
# Check if extensions are memoized anywhere in the pipeline
rg "useMemo|memoize" frontend/packages/console-dynamic-plugin-sdk/src/api/ | head -20Repository: openshift/console
Length of output: 43
Improve difference detection to avoid spurious diffs from array recreation.
lodash.difference compares objects by reference. While useExtensions stabilizes extension references via memoization, useResolvedExtensions creates a new array on each resolution cycle, triggering false additions/removals even when extension content hasn't changed.
Use uid-based comparison instead:
Suggested fix
const added = extensions.filter(e => !prevExtensionsRef.current.some(p => p.uid === e.uid));
const removed = prevExtensionsRef.current.filter(p => !extensions.some(e => e.uid === p.uid));🤖 Prompt for AI Agents
In
@frontend/packages/console-app/src/components/flags/FeatureFlagExtensionLoader.tsx
around lines 59 - 75, The diff logic in useExtensionsDiff uses lodash.difference
which compares by object reference and causes spurious added/removed events
because useResolvedExtensions returns a new array each time; update
useExtensionsDiff (inside the useEffect) to compute added as extensions whose
uid is not present in prevExtensionsRef.current and removed as
prevExtensionsRef.current entries whose uid is not present in extensions (i.e.,
replace difference(...) calls with uid-based .filter/.some checks), then call
onChange and set prevExtensionsRef.current = extensions as before.
| loadAndEnablePlugin(pluginName, pluginStore, (errorMessage, errorCause) => { | ||
| // eslint-disable-next-line no-console | ||
| console.error(..._.compact([errorMessage, errorCause])); | ||
| window.windowError = `${window.windowError ?? ''};${errorMessage}: ${String(errorCause)}`; |
There was a problem hiding this comment.
Minor: Leading semicolon in error string.
When window.windowError is initially undefined, the concatenation produces ;errorMessage: cause due to the ${window.windowError ?? ''}; pattern. This is cosmetic but may affect error parsing in tests or tooling.
🔧 Proposed fix
- window.windowError = `${window.windowError ?? ''};${errorMessage}: ${String(errorCause)}`;
+ const errorEntry = `${errorMessage}: ${String(errorCause)}`;
+ window.windowError = window.windowError ? `${window.windowError};${errorEntry}` : errorEntry;🤖 Prompt for AI Agents
In @frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-init.ts at
line 108, The concatenation that sets window.windowError in plugin-init.ts can
produce a leading semicolon when window.windowError is initially undefined;
change the logic around the window.windowError assignment so you only prepend a
semicolon when an existing value is non-empty (e.g., check window.windowError
truthiness and either set `${errorMessage}: ${String(errorCause)}` or append
`;${errorMessage}: ${String(errorCause)}`), ensuring no leading semicolon
appears in the resulting string.
| const extensionSamples = !_.isEmpty(yamlSamplesData) | ||
| ? yamlSamplesData.map((sample: K8sResourceKind) => { | ||
| return { | ||
| id: sample.metadata.uid, | ||
| ...(sample.spec as Exclude<Sample, 'id'>), | ||
| }; | ||
| }) | ||
| : []; |
There was a problem hiding this comment.
Type assertion loses type safety.
The cast sample.spec as Exclude<Sample, 'id'> is syntactically incorrect—Exclude operates on union types, not object types. You likely mean Omit<Sample, 'id'>. Additionally, spreading sample.spec directly assumes it conforms to Sample structure without validation.
🔧 Proposed fix
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'>),
};
})
: [];📝 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 extensionSamples = !_.isEmpty(yamlSamplesData) | |
| ? yamlSamplesData.map((sample: K8sResourceKind) => { | |
| return { | |
| id: sample.metadata.uid, | |
| ...(sample.spec as Exclude<Sample, 'id'>), | |
| }; | |
| }) | |
| : []; | |
| const extensionSamples = !_.isEmpty(yamlSamplesData) | |
| ? yamlSamplesData.map((sample: K8sResourceKind) => { | |
| return { | |
| id: sample.metadata.uid, | |
| ...(sample.spec as Omit<Sample, 'id'>), | |
| }; | |
| }) | |
| : []; |
🤖 Prompt for AI Agents
In @frontend/packages/console-shared/src/hooks/useResourceSidebarSamples.ts
around lines 362 - 369, The mapping in extensionSamples incorrectly uses Exclude
and unsafely spreads sample.spec; change the cast to use Omit<Sample, 'id'> and
add a runtime/type guard to ensure sample.spec is present and shaped like a
Sample before spreading. Specifically, in the extensionSamples mapping (using
yamlSamplesData and K8sResourceKind), replace "sample.spec as Exclude<Sample,
'id'>" with a safer pattern: assert/cast to Omit<Sample,'id'> only after
checking sample.spec exists (e.g., const spec = sample.spec && typeof
sample.spec === 'object' ? (sample.spec as Omit<Sample,'id'>) : undefined) and
then spread only when spec is defined (e.g., { id: sample.metadata.uid, ...(spec
|| {}) }) so you preserve type safety and avoid spreading invalid values.
| /** Set by `console-operator` or `./bin/bridge -release-version` */ | ||
| const CURRENT_OPENSHIFT_VERSION = semver(window.SERVER_FLAGS.releaseVersion); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n frontend/public/plugins.ts | head -50Repository: openshift/console
Length of output: 2586
🏁 Script executed:
fd package.json | head -5Repository: openshift/console
Length of output: 266
🏁 Script executed:
cd frontend && cat package.json | jq '.dependencies | select(. != null) | keys[] | select(. | contains("dynamic-plugin-sdk"))'Repository: openshift/console
Length of output: 92
🏁 Script executed:
cd frontend && cat package.json | grep -A 2 -B 2 "dynamic-plugin-sdk"Repository: openshift/console
Length of output: 768
🏁 Script executed:
cd frontend && find . -name "*.d.ts" -o -name "*.ts" | xargs grep -l "fixedPluginDependencyResolutions" 2>/dev/null | head -10Repository: openshift/console
Length of output: 80
🏁 Script executed:
cd frontend && find . -path "./node_modules" -prune -o -type f \( -name "*.ts" -o -name "*.tsx" \) -print | xargs grep -l "fixedPluginDependencyResolutions" 2>/dev/nullRepository: openshift/console
Length of output: 82
🏁 Script executed:
cd frontend && find packages/console-dynamic-plugin-sdk -type f -name "*.ts" -o -name "*.tsx" | xargs grep -l "fixedPluginDependencyResolutions" 2>/dev/nullRepository: openshift/console
Length of output: 45
🏁 Script executed:
cd frontend && find packages/console-dynamic-plugin-sdk -type f \( -name "*.ts" -o -name "*.tsx" -o -name "*.d.ts" \) | head -20Repository: openshift/console
Length of output: 1361
🏁 Script executed:
cd frontend && grep -r "fixedPluginDependencyResolutions" packages/console-dynamic-plugin-sdk/ --include="*.ts" --include="*.tsx"Repository: openshift/console
Length of output: 43
🏁 Script executed:
cd frontend && grep -r "class PluginStore" packages/console-dynamic-plugin-sdk/ --include="*.ts" -A 20Repository: openshift/console
Length of output: 43
🏁 Script executed:
cd frontend && find node_modules/@openshift/dynamic-plugin-sdk -type f \( -name "*.d.ts" -o -name "index.d.ts" \) 2>/dev/null | head -20Repository: openshift/console
Length of output: 43
🏁 Script executed:
cd frontend && cat node_modules/@openshift/dynamic-plugin-sdk/package.json 2>/dev/null | jq '.version, .main, .types'Repository: openshift/console
Length of output: 43
🏁 Script executed:
cd frontend && ls -la node_modules/@openshift/ 2>/dev/null || echo "node_modules not populated"Repository: openshift/console
Length of output: 87
🏁 Script executed:
grep -r "PluginStore\|fixedPluginDependencyResolutions" frontend/packages/console-dynamic-plugin-sdk/src --include="*.ts" -B 3 -A 3 2>/dev/null | head -50Repository: openshift/console
Length of output: 2883
🌐 Web query:
@openshift/dynamic-plugin-sdk PluginStore fixedPluginDependencyResolutions documentation
💡 Result:
I couldn't find any online documentation or code references for a property named fixedPluginDependencyResolutions in the @openshift/dynamic-plugin-sdk or related OpenShift Console plugin SDK packages. Could you tell me:
- which SDK/package name and version you mean (e.g. @openshift-console/dynamic-plugin-sdk v4.x), or
- paste the code or file where you saw fixedPluginDependencyResolutions?
I can then search that repo/version or inspect the code and return exact docs or source references.
🌐 Web query:
@openshift/dynamic-plugin-sdk 6.0 null values dependency resolution
💡 Result:
I can help — could you paste the exact error/log (or a short repro: package.json + plugin webpack/module-federation config) you’re seeing with @openshift/dynamic-plugin-sdk v6.0? That will let me search issues and give a concrete fix.
🏁 Script executed:
cd frontend && grep -r "loaderOptions" packages/console-dynamic-plugin-sdk/ --include="*.ts" -B 5 -A 10 2>/dev/null | head -80Repository: openshift/console
Length of output: 2344
🏁 Script executed:
cd frontend && find . -path "./node_modules" -prune -o -name "*.d.ts" -type f -print | xargs grep -l "PluginStore\|loaderOptions" 2>/dev/nullRepository: openshift/console
Length of output: 45
🏁 Script executed:
cd frontend && cat packages/console-dynamic-plugin-sdk/@types/sdk/index.d.ts 2>/dev/null | head -100Repository: openshift/console
Length of output: 322
🏁 Script executed:
cd frontend && rg "fixedPluginDependencyResolutions" --type ts --type tsx -B 5 -A 5Repository: openshift/console
Length of output: 88
🏁 Script executed:
cd frontend && rg "semver\(" --type ts --type tsx | head -20Repository: openshift/console
Length of output: 88
🏁 Script executed:
cd frontend && grep -r "SERVER_FLAGS.releaseVersion" --include="*.ts" --include="*.tsx" -B 2 -A 2Repository: openshift/console
Length of output: 2085
🏁 Script executed:
cd frontend && rg "semver" public/plugins.ts -B 3 -A 10Repository: openshift/console
Length of output: 1426
🏁 Script executed:
cd frontend && rg "semver" packages/console-shared/src -B 3 -A 3Repository: openshift/console
Length of output: 1696
🏁 Script executed:
cd frontend && cat public/plugins.ts | sed -n '7,35p'Repository: openshift/console
Length of output: 1259
Add defensive fallback for null version in production path.
semver.valid() returns null if the input is not a valid semver string. While the comment states releaseVersion is always provided in production, the code lacks defensive handling—in production, null is passed directly to fixedPluginDependencyResolutions['@console/pluginAPI'] (line 30), whereas development safely falls back to a placeholder (line 31). This asymmetry risks passing null to the SDK if releaseVersion is ever malformed or missing. Align production behavior with development by adding a fallback, or validate the value before initialization.
🤖 Prompt for AI Agents
In @frontend/public/plugins.ts around lines 10 - 11, CURRENT_OPENSHIFT_VERSION
is being set by calling semver(...) directly which can become null/invalid;
change its initialization to validate and fallback like the dev path: call
semver.valid(window.SERVER_FLAGS.releaseVersion) (or
semver.validRange/semver.coerce as appropriate) and if that returns null use a
safe placeholder (the same placeholder used in the dev branch) before using it
in fixedPluginDependencyResolutions['@console/pluginAPI']; update any code that
assumes CURRENT_OPENSHIFT_VERSION is always valid so you never pass null into
the SDK.
|
@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 byFeatureFlagExtensionLoaderSummary by CodeRabbit
New Features
Bug Fixes & Improvements
Tests
Breaking Changes
✏️ Tip: You can customize this high-level summary in your review settings.