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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/public/components/sidebars/resource-sidebar-samples.tsx (1)
213-219: Remove unused props fromResourceSidebarSamplesProps.The
yamlSamplesListandkindObjprops are defined in the type signature but neither is consumed by theResourceSidebarSamplescomponent. Remove them to clarify the component's actual contract—the component only needssamples,loadSampleYaml, anddownloadSampleYaml.frontend/packages/console-dynamic-plugin-sdk/src/types.ts (1)
1-18: LGTM: Clean re-export ofLoadedExtensionfrom upstream SDK.The removal of the local
pluginIDaugmentation and direct re-export from@openshift/dynamic-plugin-sdkis correct. This aligns the public API surface with upstream SDK types.ResolvedExtensioncontinues to useLoadedExtensioncorrectly as a wrapper type.Breaking change not yet documented: The change is properly recorded in
CHANGELOG-core.md(pluginID removal fromuseResolvedExtensions), butupgrade-sdk.mdremains incomplete (section 4.22 shows "TODO"). Add concrete migration guidance: consumers previously accessingextension.pluginIDmust adjust their code to source the plugin ID through alternative means (clarify the recommended approach).
🤖 Fix all issues with AI agents
In `@frontend/packages/console-dynamic-plugin-sdk/src/app/AppInitSDK.tsx`:
- Line 14: The JSDoc example for AppInitSDK is missing the new optional
dynamicPlugins configuration; update the example in AppInitSDK.tsx to include
dynamicPlugins in the configurations object (replace the old initPlugins
reference if present), e.g. show configurations containing appFetch,
apiDiscovery, and dynamicPlugins so consumers see the new API surface; ensure
the example uses the existing optional invocation dynamicPlugins?.() and
references the AppInitSDK and configurations symbols.
In `@frontend/public/components/app.tsx`:
- Line 12: The import of PluginStoreProvider from `@openshift/dynamic-plugin-sdk`
is invalid and will break the app; either remove the PluginStoreProvider import
and the corresponding <PluginStoreProvider> wrapper used in the render tree (the
component referenced around line 518), or replace the import with the correct
provider symbol exported by the SDK (locate the actual export in the package or
its docs) and update the import to use that symbol; ensure the component name in
the JSX matches the imported symbol (PluginStoreProvider or the correct
provider) and remove any unused import if you choose to delete the wrapper.
♻️ Duplicate comments (15)
frontend/packages/topology/src/data-transforms/DataModelExtension.tsx (1)
16-21: LGTM — Interface and signature updated consistently.The prop rename from
pluginIDtouidaligns with the SDK'sResolvedExtension.uidsemantics. The past review feedback regarding terminology has been addressed.frontend/packages/console-app/src/components/flags/FeatureFlagExtensionLoader.tsx (1)
66-74: Previous review concerns about exception handling and reference equality still apply.The two issues flagged in prior reviews remain unaddressed:
Exception resilience: If
onChangethrows,prevExtensionsRef.currentnever updates, causing the same diff to re-trigger on every render. Wrap intry/finallyso the ref always advances.Reference equality in
difference(): SinceresolveExtensionclones extension properties, logically identical extensions will have different object references. This causes false positives in the diff (extensions appear both added and removed). Consider diffing byuidinstead.Proposed hardening for exception handling
useEffect(() => { const added = difference(extensions, prevExtensionsRef.current); const removed = difference(prevExtensionsRef.current, extensions); if (added.length > 0 || removed.length > 0) { - onChange(added, removed); - prevExtensionsRef.current = extensions; + try { + onChange(added, removed); + } finally { + prevExtensionsRef.current = extensions; + } } }, [extensions, onChange]);frontend/packages/console-plugin-sdk/src/api/useExtensions.ts (2)
27-31: Sorting lacks a deterministic tie-breaker for equal plugin order values.When two plugins have the same order index (or both fallback to
MIN_SAFE_INTEGER), the relative order depends on the JS engine's sort stability. While ES2019+ guarantees stable sorts, adding a secondary comparator (e.g.,a.uid.localeCompare(b.uid)) ensures consistent ordering across all environments and makes the behavior explicit.Suggested tie-breaker
return [...extensions].sort( - (a, b) => - (pluginOrderMap.get(a.pluginName) ?? Number.MIN_SAFE_INTEGER) - - (pluginOrderMap.get(b.pluginName) ?? Number.MIN_SAFE_INTEGER), + (a, b) => { + const orderA = pluginOrderMap.get(a.pluginName) ?? Number.MIN_SAFE_INTEGER; + const orderB = pluginOrderMap.get(b.pluginName) ?? Number.MIN_SAFE_INTEGER; + if (orderA !== orderB) { + return orderA - orderB; + } + return a.uid.localeCompare(b.uid); + }, );
80-94: Ref mutations insideuseMemoand UID-only caching remain problematic.Two issues persist here:
React purity violation: Mutating
previousResultRefandpreviousUIDsRefinsideuseMemo(lines 90-91) breaks React's expectation that memo callbacks are pure. In concurrent rendering, React may invoke the callback multiple times or discard results, leading to stale or inconsistent refs.Translation updates missed: The UID-based short-circuit (lines 85-87) returns the cached result when UIDs match, but
useTranslatedExtensionscan produce updated translated strings without changing UIDs. Language switches won't propagate to consumers.Recommended refactor: move ref updates outside useMemo
- // Track the previous result and UIDs for referential stability - const previousResultRef = useRef<LoadedExtension<E>[]>([]); - const previousUIDsRef = useRef<string>(''); + // Cache for referential stability + const cacheRef = useRef<{ uids: string; result: LoadedExtension<E>[] }>({ + uids: '', + result: [], + }); - return useMemo(() => { - const sorted = sortExtensionsByPluginOrder(translatedExtensions); - const currentUIDs = sorted.map((e) => e.uid).join(','); + const sorted = useMemo( + () => sortExtensionsByPluginOrder(translatedExtensions), + [translatedExtensions], + ); - // Return previous result if the extensions haven't changed - if (currentUIDs === previousUIDsRef.current) { - return previousResultRef.current; - } + const currentUIDs = useMemo(() => sorted.map((e) => e.uid).join(','), [sorted]); - // Update refs and return new result - previousResultRef.current = sorted; - previousUIDsRef.current = currentUIDs; - - return sorted; - }, [translatedExtensions]); + // Update cache outside render phase (ref update is synchronous but not during memo) + if (currentUIDs !== cacheRef.current.uids) { + cacheRef.current = { uids: currentUIDs, result: sorted }; + } + + return cacheRef.current.result; };For the translation reactivity issue, consider whether
useTranslatedExtensionsshould return a new array reference when translations change, or include a translation version/locale in the cache key.#!/bin/bash # Verify how useTranslatedExtensions handles language changes rg -n "useTranslatedExtensions" -A 15 frontend/packages/console-plugin-sdk/src/utils/useTranslatedExtensions.ts | head -50frontend/packages/console-shared/src/hooks/useResourceSidebarSamples.ts (4)
257-270: Defensive check needed:labelanddescriptionmay not be strings.The
lazyYamlfunction assumesextension.properties.labelandextension.properties.descriptionare strings and calls.split('\n')directly. Per theAddActiontype definition, these are required strings, but runtime data from extensions could be malformed.🛡️ Add defensive coercion
lazyYaml: () => { const sortedExtensions = addActions .slice() .sort((a, b) => a.properties.id.localeCompare(b.properties.id)); const yaml = sortedExtensions .map((extension) => { const { id, label, description } = extension.properties; - const labelComment = label.split('\n').join('\n # '); - const descriptionComment = description.split('\n').join('\n # '); + const labelComment = (label ?? '').split('\n').join('\n # '); + const descriptionComment = (description ?? '').split('\n').join('\n # '); return `- # ${labelComment}\n # ${descriptionComment}\n ${id}`; }) .join('\n'); return yaml; },
352-359: FirehoseResult gating:_.isEmptyon wrapper object may be unreliable.Using
_.isEmpty(yamlSamplesList)checks the wrapper object, not the load state. If the Firehose hasn't loaded yet or errored,yamlSamplesListmay be truthy but contain invalid data.Prefer explicit state checks:
♻️ Use explicit loaded/loadError checks
- const yamlSamplesData = !_.isEmpty(yamlSamplesList) - ? _.filter( + const yamlSamplesData = yamlSamplesList?.loaded && !yamlSamplesList?.loadError && Array.isArray(yamlSamplesList.data) + ? _.filter( yamlSamplesList.data, // ... ) : [];
373-375: Misleading comment: snippets and samples are disjoint, not a superset.The comment states "snippets are a superset of samples," but the filter logic produces mutually exclusive sets. Items with
snippet: truego tosnippets; items without go tosamples.📝 Clarify comment
- // For the time being, `snippets` are a superset of `samples` + // Partition: items with snippet=true go to snippets, others to samples const snippets = allSamples.filter((sample: Sample) => sample.snippet); const samples = allSamples.filter((sample: Sample) => !sample.snippet);
364-367: Incorrect type utility: useOmitinstead ofExclude.
Exclude<T, U>operates on union types to exclude members. For removing a property from an object type, useOmit<T, K>. The current cast doesn't actually remove theidproperty from the type.🔧 Fix type utility
return { id: sample.metadata.uid, - ...(sample.spec as Exclude<Sample, 'id'>), + ...(sample.spec as Omit<Sample, 'id'>), };frontend/package.json (1)
161-161: Verify@openshift/dynamic-plugin-sdk@6.0.0availability.Previous review flagged that version 6.0.0 doesn't exist on npm—latest was 5.0.1. If this is an internal/pre-release build or has since been published, please confirm. Otherwise, pin to an available version to avoid CI failures.
#!/bin/bash # Check available versions of `@openshift/dynamic-plugin-sdk` on npm npm view `@openshift/dynamic-plugin-sdk` versions --json 2>/dev/null | tail -20 echo "---" npm view `@openshift/dynamic-plugin-sdk` version 2>/dev/nullfrontend/packages/console-dynamic-plugin-sdk/CHANGELOG-core.md (1)
21-21: Changelog entry should specify the replacement forpluginID.The breaking change entry is documented but doesn't indicate what plugin authors should use instead. Based on the PR changes,
uid(from the extension's metadata) or accessingmanifest.nameviausePluginInfowould be the alternatives.Consider updating to:
- **Breaking**: Removed `pluginID` from the result in `useResolvedExtensions` hook; use `uid` instead ([CONSOLE-3769], [`#15904`])frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-init.ts (1)
72-95: Legacy callback registration provides good backward compatibility.The version stripping logic using
lastIndexOf('@')correctly handles scoped packages (e.g.,@acme/foo@1.0.0→@acme/foo). The deprecation warning is helpful for operators managing older plugins.One remaining concern from prior reviews: line 88 calls
window[sdkCallbackName]without verifying it exists. If PluginStore fails to register the callback before a legacy plugin script loads, this will throw.Guard the SDK callback invocation
+ if (typeof window[sdkCallbackName] !== 'function') { + // eslint-disable-next-line no-console + console.error( + `SDK entry callback "${sdkCallbackName}" not initialized when legacy plugin "${pluginName}" loaded`, + ); + return; + } window[sdkCallbackName](patchedPluginName, entryModule);frontend/public/plugins.ts (3)
1-11: Import organization and version parsing look good, but null handling needs attention.The
semver.valid()function returnsnullfor invalid version strings. Ifwindow.SERVER_FLAGS.releaseVersionis undefined or malformed in production,CURRENT_OPENSHIFT_VERSIONbecomesnull, which propagates tofixedPluginDependencyResolutionson line 28-31.The development fallback (
|| '4.1337.67') on line 31 handles dev environments, but production has no guard. While the comment claims console-operator always provides this value, defensive coding would add a warning:Add production warning for invalid version
const CURRENT_OPENSHIFT_VERSION = semver(window.SERVER_FLAGS.releaseVersion); + +if (process.env.NODE_ENV === 'production' && !CURRENT_OPENSHIFT_VERSION) { + // eslint-disable-next-line no-console + console.warn( + 'Invalid releaseVersion from SERVER_FLAGS:', + window.SERVER_FLAGS.releaseVersion, + ); +}
38-43: Local plugin loading is synchronous fire-and-forget.Line 43 calls
loadPluginwithout awaiting or catching errors. For bundled local plugins (generated by webpack), this is likely safe since they're already in memory. However, ifloadPlugincan reject for local manifests, errors would be swallowed.Prior reviews suggested error handling. Given this is module-level initialization, a sync-looking pattern may be intentional. Document the assumption:
Add clarifying comment or error handling
+// Local plugins are bundled by webpack and loaded synchronously. +// loadPlugin() returns a Promise but resolves immediately for LocalPluginManifest. localPlugins.forEach((plugin) => pluginStore.loadPlugin(plugin));Or add error handling:
-localPlugins.forEach((plugin) => pluginStore.loadPlugin(plugin)); +localPlugins.forEach((plugin) => { + pluginStore.loadPlugin(plugin).catch((err) => { + // eslint-disable-next-line no-console + console.error(`Failed to load local plugin ${plugin.name}:`, err); + }); +});
69-74: Log label should be "Local plugins" per PR terminology.The PR description explicitly renames "active plugins" to "local plugins," but line 71 still logs "Static plugins." This inconsistency will cause confusion during debugging.
Update log label
- console.info(`Static plugins: [${localPlugins.map((p) => p.name).join(', ')}]`); + console.info(`Local plugins: [${localPlugins.map((p) => p.name).join(', ')}]`);frontend/packages/console-app/src/__tests__/plugin-test-utils.ts (1)
8-15: Race condition:loadPluginreturns a Promise but is called without awaiting.Prior reviews flagged this exact issue.
PluginStore.loadPlugin()is async (confirmed byawaitusage inplugin-init.ts:23), but lines 11-13 useforEachwithout awaiting. This meanstestedExtensionson line 15 is derived before plugins finish loading, causing unreliable test results.For local manifests (not URL-based),
loadPluginmay complete synchronously in some SDK versions, but relying on this is fragile.Robust async initialization pattern
// Option A: If top-level await is supported (ESM modules) const testedPlugins = loadLocalPluginsForTestPurposes(resolvePluginPackages()); const testedPluginStore = new PluginStore(); await Promise.all(testedPlugins.map((plugin) => testedPluginStore.loadPlugin(plugin))); export const testedExtensions = ImmutableList<Extension>(testedPluginStore.getExtensions()); // Option B: Export a factory function for test setup export const initTestedPluginStore = async () => { const plugins = loadLocalPluginsForTestPurposes(resolvePluginPackages()); const store = new PluginStore(); await Promise.all(plugins.map((p) => store.loadPlugin(p))); return ImmutableList<Extension>(store.getExtensions()); };#!/bin/bash # Check if loadPlugin for LocalPluginManifest is documented as sync in SDK rg -n "loadPlugin.*LocalPluginManifest|LocalPluginManifest.*loadPlugin" --type ts frontend/
🧹 Nitpick comments (7)
frontend/packages/console-app/src/components/nodes/useNodeStatusExtensions.tsx (1)
63-66: Improved error context — good enhancement.Logging both
uidandtypeprovides better diagnostics than the previouspluginID-only approach. When something fails in a customer environment, knowing the extension type immediately helps narrow down which plugin capability is misbehaving.One optional nit for future consideration: a structured format like
Extension failed [uid=${uid}, type=${type}]can be slightly easier to grep in log aggregators, but the current form is perfectly acceptable.frontend/packages/console-dynamic-plugin-sdk/src/coderefs/coderef-resolver.ts (1)
16-26: Symbol extraction approach is sound with proper fail-fast guard.The runtime derivation of the SDK's internal
codeRefSymbolviaapplyCodeRefSymbolon a dummy function is the right approach when the SDK doesn't expose this symbol publicly. The fail-fast guard at lines 24-26 ensures immediate, obvious failure if SDK internals change.One optional improvement per past review: consider documenting the SDK version this was validated against so maintainers know to re-verify after SDK upgrades.
📝 Optional: Add SDK version comment
/** * Extract the SDK's internal CodeRef symbol by applying it to a dummy function. * * This ensures we can detect code refs created by the SDK, which uses its own * private Symbol instance. + * + * Validated against `@openshift/dynamic-plugin-sdk` ^6.0.0. Re-verify symbol + * extraction after SDK version upgrades. */ const codeRefSymbol = Object.getOwnPropertySymbols(applyCodeRefSymbol(() => Promise.resolve()))[0];frontend/packages/console-shared/src/hooks/useResourceSidebarSamples.ts (1)
93-99: Performance:useDefaultSamplesrebuilds ImmutableMap on every render.This hook calls
useResolvedExtensionsthree times and constructs a newImmutableMapon each invocation. Since the map contains many sample objects, this causes unnecessary allocations and can trigger downstream re-renders.Wrap the map construction in
useMemowith appropriate dependencies:♻️ Memoize the return value
+import { useMemo } from 'react'; + const useDefaultSamples = () => { const { t } = useTranslation('console-shared'); const [addActions] = useResolvedExtensions<AddAction>(isAddAction); const [catalogItemTypes] = useResolvedExtensions<CatalogItemType>(isCatalogItemType); const [perspectives] = useResolvedExtensions<Perspective>(isPerspective); const clusterRoleBindingSamples = useClusterRoleBindingSamples(); - return ImmutableMap<GroupVersionKind, Sample[]>() + return useMemo(() => ImmutableMap<GroupVersionKind, Sample[]>() .setIn(/* ... */) // ... rest of chain - ); + , [t, addActions, catalogItemTypes, perspectives, clusterRoleBindingSamples]); };frontend/public/components/sidebars/resource-sidebar-samples.tsx (1)
96-107: Error handling preserved, but consider a fallback callback.Good to see the
try/catchwas retained. However, whenlazyYaml()throws, the callback is never invoked, which could leave the UI waiting indefinitely (e.g., YAML preview never appears after clicking "Show YAML").🛡️ Consider calling callback with empty string on error
const resolveYaml = (callback: (resolvedYaml: string) => void) => { if (yaml) { callback(yaml); } else if (lazyYaml) { try { callback(lazyYaml()); } catch (error) { // eslint-disable-next-line no-console console.warn(`Error while running lazy yaml snippet ${id} (${title})`, error); + callback(''); // Provide fallback to prevent UI from hanging } } };frontend/public/components/app.tsx (1)
437-446: TODO comments indicate known technical debt—track for follow-up.The
initDynamicPluginswrapper correctly delegates toinitConsolePlugins(pluginStore). The TODO comments regarding earlier lifecycle loading and HAC-375 (coFetch dependency injection) should be tracked.The fire-and-forget async pattern inside
initConsolePluginswas flagged in prior reviews but marked addressed. The current design accepts that plugin load failures are logged but don't block app initialization—this is a reasonable trade-off for resilience.Would you like me to open issues to track the TODOs on lines 441-442?
frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-init.ts (1)
18-57:loadAndEnablePluginerror handling is improved but still has a subtle gap.The optional chaining on
plugin?.status(line 37) addresses the null dereference concern from prior reviews. However, after the.catch()on line 30-33, execution continues to line 35 even whenloadPluginfails. IfloadPluginrejects,onErroris called, but then the code proceeds to checkplugin?.status—which may find a partially registered plugin or none at all.Consider returning early after the catch to avoid redundant status checks:
Suggested improvement
.catch((err: ErrorWithCause) => { // ErrorWithCause isn't the exact type but it's close enough for our use onError(`[loadAndEnablePlugin] ${pluginName} loadPlugin failed: ${err.message}`, err.cause); + return; // Exit after handling load failure }); + + // Only check status if loadPlugin succeeded (no early return above) const plugin = pluginStore.getPluginInfo().find((p) => p.manifest.name === pluginName);frontend/packages/console-dynamic-plugin-sdk/src/app/AppInitSDK.tsx (1)
24-34: JSDoc example should demonstratedynamicPluginsconfiguration.The example code block still shows the minimal pattern without the new
dynamicPluginsoption. Prior review flagged this and it was marked addressed, but the current code doesn't reflect that update.Update the example
* `@example` * ```ts * return ( * <Provider store={store}> - * <AppInitSDK configurations={{ appFetch, apiDiscovery }}> + * <AppInitSDK configurations={{ appFetch, apiDiscovery, dynamicPlugins: () => initPlugins() }}> * <CustomApp /> * ... * </AppInitSDK> * </Provider> * ) * ```
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
frontend/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (46)
frontend/package.jsonfrontend/packages/console-app/src/__tests__/plugin-test-utils.tsfrontend/packages/console-app/src/components/dashboards-page/dynamic-plugins-health-resource/NotLoadedDynamicPlugins.tsxfrontend/packages/console-app/src/components/file-upload/__tests__/file-upload-utils.spec.tsfrontend/packages/console-app/src/components/flags/FeatureFlagExtensionLoader.tsxfrontend/packages/console-app/src/components/nodes/useNodeStatusExtensions.tsxfrontend/packages/console-app/src/components/user-preferences/__tests__/userPreferences.data.tsxfrontend/packages/console-app/src/components/user-preferences/perspective/__tests__/perspective.data.tsfrontend/packages/console-dynamic-plugin-sdk/CHANGELOG-core.mdfrontend/packages/console-dynamic-plugin-sdk/README.mdfrontend/packages/console-dynamic-plugin-sdk/package.jsonfrontend/packages/console-dynamic-plugin-sdk/scripts/package-definitions.tsfrontend/packages/console-dynamic-plugin-sdk/src/app/AppInitSDK.tsxfrontend/packages/console-dynamic-plugin-sdk/src/coderefs/__tests__/coderef-resolver.spec.tsfrontend/packages/console-dynamic-plugin-sdk/src/coderefs/coderef-resolver.tsfrontend/packages/console-dynamic-plugin-sdk/src/perspective/__tests__/perspective.data.tsfrontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-init.tsfrontend/packages/console-dynamic-plugin-sdk/src/shared-modules/shared-modules-meta.tsfrontend/packages/console-dynamic-plugin-sdk/src/types.tsfrontend/packages/console-plugin-sdk/package.jsonfrontend/packages/console-plugin-sdk/src/api/pluginSubscriptionService.tsfrontend/packages/console-plugin-sdk/src/api/useExtensions.tsfrontend/packages/console-plugin-sdk/src/store.tsfrontend/packages/console-plugin-sdk/src/utils/allowed-plugins.tsfrontend/packages/console-shared/locales/en/console-shared.jsonfrontend/packages/console-shared/src/components/actions/__tests__/utils-test-data.tsfrontend/packages/console-shared/src/components/catalog/__tests__/CatalogController.spec.tsxfrontend/packages/console-shared/src/components/editor/CodeEditorSidebar.tsxfrontend/packages/console-shared/src/components/formik-fields/CodeEditorField.tsxfrontend/packages/console-shared/src/hooks/__tests__/useTelemetry.spec.tsfrontend/packages/console-shared/src/hooks/useResourceSidebarSamples.tsfrontend/packages/console-shared/src/utils/index.tsfrontend/packages/console-shared/src/utils/sample-utils.tsfrontend/packages/dev-console/src/components/add/__tests__/add-page-test-data.tsxfrontend/packages/dev-console/src/utils/__tests__/useAddActionExtensions.spec.tsfrontend/packages/topology/src/data-transforms/DataModelExtension.tsxfrontend/packages/topology/src/data-transforms/DataModelProvider.tsxfrontend/public/actions/features.tsfrontend/public/components/app.tsxfrontend/public/components/dashboard/project-dashboard/getting-started/__tests__/SampleGettingStartedCard.data.tsfrontend/public/components/edit-yaml.tsxfrontend/public/components/notification-drawer.tsxfrontend/public/components/sidebars/resource-sidebar-samples.tsxfrontend/public/components/sidebars/resource-sidebar.tsxfrontend/public/components/utils/__tests__/nav.spec.tsfrontend/public/plugins.ts
💤 Files with no reviewable changes (17)
- frontend/packages/console-shared/src/hooks/tests/useTelemetry.spec.ts
- frontend/packages/console-shared/src/components/actions/tests/utils-test-data.ts
- frontend/packages/console-plugin-sdk/package.json
- frontend/public/components/dashboard/project-dashboard/getting-started/tests/SampleGettingStartedCard.data.ts
- frontend/packages/console-dynamic-plugin-sdk/package.json
- frontend/packages/dev-console/src/utils/tests/useAddActionExtensions.spec.ts
- frontend/packages/console-shared/src/components/catalog/tests/CatalogController.spec.tsx
- frontend/packages/console-app/src/components/file-upload/tests/file-upload-utils.spec.ts
- frontend/packages/console-app/src/components/user-preferences/perspective/tests/perspective.data.ts
- frontend/packages/dev-console/src/components/add/tests/add-page-test-data.tsx
- frontend/packages/console-plugin-sdk/src/store.ts
- frontend/public/components/utils/tests/nav.spec.ts
- frontend/packages/console-shared/src/utils/sample-utils.ts
- frontend/packages/console-shared/src/utils/index.ts
- frontend/packages/console-plugin-sdk/src/api/pluginSubscriptionService.ts
- frontend/packages/console-app/src/components/user-preferences/tests/userPreferences.data.tsx
- frontend/packages/console-dynamic-plugin-sdk/src/perspective/tests/perspective.data.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- frontend/packages/console-shared/src/components/editor/CodeEditorSidebar.tsx
- frontend/packages/console-plugin-sdk/src/utils/allowed-plugins.ts
- frontend/public/components/sidebars/resource-sidebar.tsx
- frontend/packages/console-shared/locales/en/console-shared.json
- frontend/packages/console-dynamic-plugin-sdk/README.md
- frontend/public/components/edit-yaml.tsx
- frontend/public/components/notification-drawer.tsx
- frontend/public/actions/features.ts
🧰 Additional context used
🧬 Code graph analysis (5)
frontend/packages/console-shared/src/components/formik-fields/CodeEditorField.tsx (1)
frontend/packages/console-shared/src/hooks/useResourceSidebarSamples.ts (1)
useResourceSidebarSamples(345-378)
frontend/packages/console-dynamic-plugin-sdk/src/coderefs/coderef-resolver.ts (1)
frontend/packages/console-dynamic-plugin-sdk/src/types.ts (1)
CodeRef(31-31)
frontend/packages/console-shared/src/hooks/useResourceSidebarSamples.ts (5)
frontend/public/models/index.ts (5)
ClusterRoleModel(539-552)ResourceQuotaModel(696-709)RoleModel(570-584)ConsoleLinkModel(1160-1175)ConsoleOperatorConfigModel(1147-1158)frontend/packages/console-dynamic-plugin-sdk/src/extensions/add-actions.ts (2)
AddAction(8-28)isAddAction(51-53)frontend/packages/console-dynamic-plugin-sdk/src/extensions/catalog.ts (2)
CatalogItemType(7-25)isCatalogItemType(112-114)frontend/packages/console-dynamic-plugin-sdk/src/extensions/perspectives.ts (1)
isPerspective(31-33)frontend/packages/console-shared/src/utils/default-categories.ts (1)
defaultCatalogCategories(3-71)
frontend/packages/console-app/src/__tests__/plugin-test-utils.ts (2)
frontend/packages/console-plugin-sdk/src/codegen/local-plugins.ts (1)
loadLocalPluginsForTestPurposes(78-96)frontend/packages/console-plugin-sdk/src/codegen/plugin-resolver.ts (1)
resolvePluginPackages(78-105)
frontend/public/components/app.tsx (4)
frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-init.ts (1)
initConsolePlugins(102-126)frontend/public/plugins.ts (1)
pluginStore(20-34)frontend/public/co-fetch.ts (1)
appInternalFetch(129-155)frontend/packages/console-dynamic-plugin-sdk/src/app/k8s/api-discovery/api-discovery.ts (1)
initApiDiscovery(149-159)
🔇 Additional comments (39)
frontend/packages/console-app/src/components/nodes/useNodeStatusExtensions.tsx (1)
47-48: LGTM — Clean migration frompluginIDtouid.This aligns nicely with the SDK's shift to
manifest.name-based identifiers. Destructuringtypealongsideuidis a good call for the improved error diagnostics below. Also keeps parity with line 31's existinguidusage.frontend/packages/topology/src/data-transforms/DataModelProvider.tsx (2)
31-39: LGTM — Clean uid propagation across all resolution paths.The type declaration and all three resolution branches (async success, async error, static resources) consistently thread through
factory.uid. The error message terminology issue flagged in prior review has been addressed—it now correctly references "extension" rather than "plugin."Also applies to: 57-78
92-98: LGTM — Correct separation of factory id (key) vs extension uid (tracking).Using
factory.properties.idas the React key is appropriate since it's the factory's stable identifier, whilefactory.uidcorrectly identifies the resolved extension instance for logging and tracking purposes.frontend/packages/topology/src/data-transforms/DataModelExtension.tsx (1)
67-84: LGTM — Correct terminology and memoization dependencies.The warning now correctly identifies the source as an "Extension" rather than "Plugin" (consistent with
ResolvedExtension.uidsemantics). Includinguidin theuseMemodependency array is necessary since it's referenced in theconsole.warnwithin the memo body—prevents stale closures if the component receives a different extension.frontend/packages/console-app/src/components/flags/FeatureFlagExtensionLoader.tsx (5)
1-23: Imports look well-organized.The new imports for lodash
difference, model feature flag types, and theupdateModelFlagsaction align with the hook-based refactoring approach. The structure cleanly separates runtime values from type-only imports.
24-53: Solid pattern for deferring dispatches to avoid render-time state updates.The
useLayoutEffectwithout a dependency array intentionally runs on every render to flush queued flag updates synchronously after React commits—this sidesteps the react-redux 8.x "Cannot update a component while rendering" pitfall. The early-exit on line 38-40 and the staleness check on line 42 are efficient safeguards.One minor note: if the same flag is queued multiple times within a single render cycle, only the final value survives due to
Map.setsemantics. This appears intentional, but worth keeping in mind for debugging scenarios.
77-97: Good error isolation for feature flag handlers.The try/catch on lines 85-90 ensures one misbehaving plugin handler doesn't cascade failures to others—important for extension-based architectures where plugin code quality varies. Including
pluginNamein the error message aids triage.Intentionally ignoring
removedextensions is appropriate here sinceFeatureFlaghandlers are registration callbacks with no teardown semantics.
99-126: Clean ref-based pattern for capturing latest Redux state in a stable callback.The
modelsRefpattern (lines 108-111) is the canonical React approach for accessing current state from within a memoized callback without invalidating its identity on every state change. This correctly avoids the stale-closure pitfall while keepinghandleChangestable for the diff hook's dependency array.The inline comment at lines 118-119 helpfully documents why the models are passed via action payload rather than accessed directly in the reducer—good practice for cross-reducer data dependencies.
128-161: Clean integration of the new extension-handling hooks.The component correctly calls
useFeatureFlagExtensionsanduseModelFeatureFlagExtensionsunconditionally before any conditional rendering (React's rules of hooks). Usinguidas the key at line 151 for the renderedFeatureFlagExtensionHookResolvercomponents ensures stable identity across re-renders.The public API surface of
FeatureFlagExtensionLoaderremains unchanged, which is important for backward compatibility as the PR description emphasizes.frontend/packages/console-dynamic-plugin-sdk/src/coderefs/__tests__/coderef-resolver.spec.ts (3)
1-10: LGTM – Imports correctly align with SDK migration.The imports now properly source
applyCodeRefSymbolfrom@openshift/dynamic-plugin-sdkrather than a local implementation, andisCodeRefErroris correctly imported for the error-path test at line 166. ThegetExecutableCodeRefMockutility abstracts away the SDK symbol details, making tests less brittle.
129-143: Test correctly validates new cloning behavior.The renamed test and updated assertions using
.not.toBe()properly verify thatresolveExtensionnow returns a new extension object rather than mutating the original. This aligns with the production code change whereclonedPropertiesis used to preserve immutability of the input extension.Good coverage of both cases—extensions with and without CodeRefs.
145-171: Test documents Jest 30 limitation clearly – no action needed.The extensive inline comments explain the Jest 30 symbol interference issue and clarify what the test actually validates (no CodeRefs found scenario). This is acceptable given the alternative—modifying production code to accommodate test tooling—would be the wrong tradeoff.
frontend/packages/console-dynamic-plugin-sdk/src/coderefs/coderef-resolver.ts (1)
62-99: Cloning behavior is correctly implemented with one subtlety to note.The switch from in-place mutation to returning a new extension object with cloned properties is a solid improvement for predictability. The
_.cloneDeepon line 69 ensures the original extension'spropertiesobject remains untouched.One nuance to be aware of:
_.cloneDeepdoes not clone functions—CodeRef functions remain shared references between the original and cloned properties. This meanssetCodeRefErrorat line 86 still marks errors on the original CodeRef functions, which is intentional for the "continuously reject failed CodeRefs" behavior. The JSDoc at lines 58-60 correctly describes preserving the original extension, but strictly speaking, the CodeRef functions themselves can be mutated (error markers). This is acceptable given the intended behavior, but worth documenting if questions arise later.frontend/packages/console-plugin-sdk/src/api/useExtensions.ts (2)
1-9: LGTM — clean import structureGood move dropping the legacy subscription/force-render imports in favor of the SDK hook. The aliased import
useExtensionsSDKclearly disambiguates from the exporteduseExtensions.
66-71: LGTM — runtime validation for type guardsThe early throw ensures callers provide at least one guard, preventing silent bugs from empty extension lists.
frontend/packages/console-app/src/components/dashboards-page/dynamic-plugins-health-resource/NotLoadedDynamicPlugins.tsx (3)
2-2: Clean migration to SDK types.Good adoption of the upstream
@openshift/dynamic-plugin-sdktypes. This aligns well with the Phase 2 migration objectives.
14-17: Property access aligns with SDK patterns.The switch from
pluginNametomanifest.nameis consistent with how plugin identifiers are accessed throughout the codebase. Both theFailedPluginInfoEntryandPendingPluginInfoEntrytypes exposemanifest.nameas the canonical plugin identifier—the same property used as ResourceLink targets in the ConsolePlugin resource model. React keys remain stable and unique per plugin.
27-30: Type definition properly aligned with SDK discriminated union.The union type
(FailedPluginInfoEntry | PendingPluginInfoEntry)[]correctly models the two states for not-loaded plugins and the sole caller inDynamicPluginsPopover.tsxis already using it correctly. The filtering byplugin.statusfield naturally narrows the discriminated union types, eliminating the need for type assertions.frontend/packages/console-shared/src/hooks/useResourceSidebarSamples.ts (2)
1-46: LGTM on imports and type definitions.The
Sampletype is well-structured with mutually exclusiveyamlandlazyYamloptions for lazy evaluation. Type exports are appropriate for the public API surface.
68-91: Sample structure supports optional yaml—no runtime issue exists here.The
useClusterRoleBindingSampleshook intentionally returns samples withoutyamlorlazyYamlproperties, which is valid per theSampletype definition where both are optional. The handlers (replaceYamlContent,downloadYamlContent,insertYamlContent) inCodeEditorSidebar.tsxsafely default theyamlContentparameter to an empty string when undefined is passed, so passingundefineddoes not cause runtime errors. This design is intentional and behaves safely.frontend/packages/console-shared/src/components/formik-fields/CodeEditorField.tsx (2)
18-18: LGTM on import migration.Clean migration from the removed
getResourceSidebarSamplesutility function to the newuseResourceSidebarSampleshook.
50-54: Hook integration is clean and follows React patterns.The
useResourceSidebarSampleshook is called unconditionally (correct per Rules of Hooks), and the hook itself handles theundefinedmodel case with an early return. The FirehoseResult-shaped object correctly passes through the watch state.frontend/public/components/sidebars/resource-sidebar-samples.tsx (1)
11-11: LGTM on import path update.Sample type correctly imported from the new hook module location.
frontend/package.json (2)
115-124: Test setup changes look good.Adding the webpack mock to
setupFilesaligns with the SDK integration requiring share scope initialization in tests. This ensures tests can properly mock webpack's__webpack_share_scopes__global.
104-106: LGTM on transformIgnorePatterns update.Adding
js-combinatoricsto the non-ignored packages ensures proper ESM-to-CJS transformation during tests.frontend/packages/console-dynamic-plugin-sdk/scripts/package-definitions.ts (1)
173-187: LGTM: SDK dependency correctly added to webpack package.The
@openshift/dynamic-plugin-sdkis appropriately added togetWebpackPackagedependencies. This aligns with the architectural shift where the webpack package needs the SDK forConsoleRemotePluginbuild tooling, while core/internal packages now re-export SDK types directly.frontend/packages/console-dynamic-plugin-sdk/src/shared-modules/shared-modules-meta.ts (2)
24-26: LGTM: SDK correctly added as shared module.Adding
@openshift/dynamic-plugin-sdkto the shared modules list ensures Console and all dynamic plugins share a single SDK instance at runtime. The default metadata (singleton: true,allowFallback: false) is appropriate—plugins must not bundle their own SDK version.
44-46: Metadata entry aligns with shared module registration.The empty object
{}applies sensible defaults. No concerns here.frontend/packages/console-dynamic-plugin-sdk/src/types.ts (1)
62-65:ResolvedExtensiontype composition is correct.The generic type properly wraps
LoadedExtensionwith resolved code ref properties. No issues with the type algebra here.frontend/packages/console-dynamic-plugin-sdk/CHANGELOG-core.md (2)
13-17: Good: Important banner properly highlights breaking changes.The
> [!IMPORTANT]callout and reference toupgrade-sdk.mdis the right approach for a release with breaking changes. Ensure the upgrade guide is complete before release.
234-234: PR link reference added correctly.frontend/public/components/app.tsx (1)
517-536: PluginStoreProvider integration looks correct.The provider hierarchy is properly structured:
Provider (Redux)→PluginStoreProvider→ThemeProvider→HelmetProvider→AppInitSDK. This ensures the plugin store context is available to all descendants before dynamic plugin initialization fires.frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-init.ts (2)
1-10: Clean import organization for the new plugin loading infrastructure.The type-only imports for
PluginStore,ConsolePluginManifest, andErrorWithCauseare appropriate. TheresolveURLutility anddynamicPluginNameswire up the runtime loading correctly.
102-126:initConsolePluginsuses fire-and-forget pattern for parallel plugin loading.The
forEachon line 109 dispatches plugin loads without awaiting. This is intentional for parallelism—plugins load concurrently, and failures are logged but don't block others. The trade-off is acceptable for resilience.The
windowErroraccumulation (line 113) can grow unbounded in pathological scenarios. Prior review noted this but it's a known pattern used by Cypress e2e tests. Consider documenting the expected max error count or adding truncation if this becomes problematic in practice.frontend/packages/console-dynamic-plugin-sdk/src/app/AppInitSDK.tsx (1)
36-48: Initialization flow is clean and handles optional callbacks correctly.The
dynamicPlugins?.()invocation with optional chaining (line 42) safely handles cases where no dynamic plugin loader is configured. The try-catch around initialization (lines 40-47) ensures misconfigurations don't crash the app, though logging toconsole.warnmay be too quiet for production issues.frontend/packages/console-app/src/__tests__/plugin-test-utils.ts (2)
1-6: Updated imports align with local plugin testing strategy.The switch from
loadActivePluginsForTestPurposestoloadLocalPluginsForTestPurposesaligns with the PR's terminology migration (active → local). ThePluginStoreimport from@openshift/dynamic-plugin-sdkis consistent with production code.
17-27:getDuplicatesutility is straightforward.Clean implementation using lodash's
transformandcountBy. No issues.frontend/public/plugins.ts (2)
20-34: PluginStore configuration is well-structured.The
loaderOptionsproperly configure:
sharedScopefor webpack module federationfetchImplwith the console-specific fetch wrapper (type suppression documented)fixedPluginDependencyResolutionsfor semver-based plugin compatibilityThe
@ts-expect-errorcomment on line 23 is appropriate—consoleFetchhas an extra timeout parameter not in the standard fetch signature.
48-62:featureFlagMiddlewarecorrectly synchronizes Redux feature flags with PluginStore.This middleware pattern is clean:
- Tracks previous flags state to avoid unnecessary updates
- Compares by reference (
nextFlags !== prevFlags) which works with Immutable.js- Calls
pluginStore.setFeatureFlags()only on actual changesThe typing
Middleware<{}, RootState>is correct for a middleware that doesn't extend the dispatch signature.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
/retest |
logonoff
force-pushed
the
CONSOLE-3769-s3e3-the-coup
branch
from
f6560cf to
faad4ee
Compare
|
@logonoff: This pull request references CONSOLE-3769 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
logonoff
force-pushed
the
CONSOLE-3769-s3e3-the-coup
branch
11 times, most recently
from
b609b7a to
69f0860
Compare
logonoff
force-pushed
the
CONSOLE-3769-s3e3-the-coup
branch
from
69f0860 to
f0f3f25
Compare
remove subscribeToDynamicPlugins cause it's not used
hack around coderefs not working by stealing the `Symbol` from upstream Update plugin-init.ts
Co-Authored-By: Vojtech Szocs <vojtech.szocs@gmail.com>
logonoff
force-pushed
the
CONSOLE-3769-s3e3-the-coup
branch
from
f0f3f25 to
ff3ff26
Compare
|
/retest |
This reverts the `useResolvedExtnensions` refactor in the previous commit. We've seen that for some reason, the SDK's `useResolvedExtensions` impl causes console to go into an infinite rerender loop. We will investigate and fix this in a followup PR.
logonoff
force-pushed
the
CONSOLE-3769-s3e3-the-coup
branch
from
ff3ff26 to
ad50aa8
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 byFeatureFlagExtensionLoader