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