OCPBUGS-72585: Don't run catalog item hooks when they are disabled #15908
Conversation
openshift-ci-robot
added
jira/severity-moderate
|
@logonoff: This pull request references Jira Issue OCPBUGS-72585, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
WalkthroughFilters disabled catalog item types during extension processing, replaces useResolvedExtensions with useExtensions, derives disabled sub-catalogs from SERVER_FLAGS via a memoized hook, adds item relevance scoring/sorting helpers, and updates tests to mock/use the new useExtensions behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
|
Skipping CI for Draft Pull Request. |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@logonoff: This pull request references Jira Issue OCPBUGS-72585, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: 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 Jira Issue OCPBUGS-72585, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/packages/operator-lifecycle-manager/console-extensions.json (1)
378-388: AddSOFTWARE_CATALOG_TYPE_OPERATOR_DISABLEDto operator item-provider flags for consistent gating.The
console.catalog/item-providerfor operators (line 386) only disallowsOLMV1_ENABLED, while the correspondingconsole.catalog/item-type(line 440) andcategories-provider(line 450) both includeSOFTWARE_CATALOG_TYPE_OPERATOR_DISABLED. This asymmetric gating breaks the established pattern: theOperatorBackedServiceitem-provider and item-type both use their respectiveSOFTWARE_CATALOG_TYPE_*_DISABLEDflags consistently. The operator item-provider should includeSOFTWARE_CATALOG_TYPE_OPERATOR_DISABLEDin its disallowed array to ensure symmetric feature gating and prevent data fetching when the operator catalog is disabled.
🤖 Fix all issues with AI agents
In @frontend/packages/dev-console/src/actions/providers.tsx:
- Around line 370-384: useDisabledCatalogTypes currently only sets
SOFTWARE_CATALOG_TYPE_<TYPE>_DISABLED to true for items in catalogTypes.disabled
and never resets flags to false; update the logic in useDisabledCatalogTypes so
you compute the union of catalogTypes.disabled and catalogTypes.enabled (or a
known set of types), then call setFeatureFlag for each type: set true when the
type is in catalogTypes.disabled, otherwise set false, ensuring flags are
explicitly reset when a type is re-enabled; reference the
useDisabledCatalogTypes function, the catalogTypes.disabled/
catalogTypes.enabled values, and the setFeatureFlag call.
📜 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 (8)
frontend/packages/dev-console/console-extensions.jsonfrontend/packages/dev-console/src/actions/providers.tsxfrontend/packages/dev-console/src/components/catalog/CatalogTypesConfiguration.tsxfrontend/packages/dev-console/src/types/catalog.tsfrontend/packages/helm-plugin/console-extensions.jsonfrontend/packages/knative-plugin/console-extensions.jsonfrontend/packages/operator-lifecycle-manager-v1/console-extensions.jsonfrontend/packages/operator-lifecycle-manager/console-extensions.json
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/packages/dev-console/src/components/catalog/CatalogTypesConfiguration.tsx (1)
frontend/packages/dev-console/src/types/catalog.ts (1)
DeveloperCatalogTypes(3-7)
frontend/packages/dev-console/src/actions/providers.tsx (2)
frontend/packages/console-dynamic-plugin-sdk/src/extensions/feature-flags.ts (1)
FeatureFlagHandler(46-46)frontend/packages/dev-console/src/types/catalog.ts (1)
SoftwareCatalogTypesConsoleConfig(9-17)
🔇 Additional comments (19)
frontend/packages/operator-lifecycle-manager-v1/console-extensions.json (1)
52-76: LGTM! Consistent gating of OLMv1 operator catalog extensions.Both
console.catalog/item-providerandconsole.catalog/categories-providerare correctly gated withSOFTWARE_CATALOG_TYPE_OPERATOR_DISABLEDdisallowed flag. This ensures hooks won't execute when the operator catalog type is disabled via console-operator config.frontend/packages/operator-lifecycle-manager/console-extensions.json (2)
264-295: LGTM! OperatorBackedService extensions properly gated.Both
console.catalog/item-typeandconsole.catalog/item-providerforOperatorBackedServiceare correctly configured withSOFTWARE_CATALOG_TYPE_OPERATORBACKEDSERVICE_DISABLEDin the disallowed flags. This ensures the hooks won't run when this catalog type is disabled.
438-452: LGTM! Operator item-type and categories-provider correctly gated.The
console.catalog/item-typefor operators andconsole.catalog/categories-providerboth includeSOFTWARE_CATALOG_TYPE_OPERATOR_DISABLEDin their disallowed flags, ensuring proper feature-flag gating when the catalog type is disabled.frontend/packages/helm-plugin/console-extensions.json (1)
182-217: LGTM! Helm chart catalog extensions properly gated.Both
console.catalog/item-typeandconsole.catalog/item-providerforHelmChartcorrectly includeSOFTWARE_CATALOG_TYPE_HELMCHART_DISABLEDin their disallowed flags alongside the existingOPENSHIFT_HELMrequired flag. This ensures Helm chart hooks won't execute when the catalog type is disabled.frontend/packages/knative-plugin/console-extensions.json (3)
473-504: LGTM! EventType catalog extensions properly gated.Both
console.catalog/item-typeandconsole.catalog/item-providerforEventTypecorrectly includeSOFTWARE_CATALOG_TYPE_EVENTTYPE_DISABLEDin disallowed flags.
706-750: LGTM! EventSource catalog extensions comprehensively gated.All three EventSource-related extensions are properly configured:
console.catalog/item-typefor EventSourceconsole.catalog/item-providerusinguseEventSourceProviderconsole.catalog/item-providerusinguseKameletsProviderAll include
SOFTWARE_CATALOG_TYPE_EVENTSOURCE_DISABLEDin their disallowed flags.
751-795: LGTM! EventSink catalog extensions comprehensively gated.All three EventSink-related extensions are properly configured:
console.catalog/item-typefor EventSinkconsole.catalog/item-providerusinguseKameletsSinkProviderconsole.catalog/item-providerusinguseKafkaSinkProviderAll include
SOFTWARE_CATALOG_TYPE_EVENTSINK_DISABLEDin their disallowed flags, ensuring consistent behavior when the catalog type is disabled.frontend/packages/dev-console/src/components/catalog/CatalogTypesConfiguration.tsx (2)
18-18: LGTM! Clean type consolidation.Good refactoring to import
DeveloperCatalogTypesandSoftwareCatalogTypesConsoleConfigfrom the centralized types file. This improves maintainability by having a single source of truth for these types, which aligns with the broader PR changes introducing the catalog type gating mechanism.
52-52: LGTM! State typing updated correctly.The
useState<DeveloperCatalogTypes>()correctly aligns with the imported type, which per the code snippet includesstate: 'Enabled' | 'Disabled'plus optionalenabledanddisabledarrays. This matches the existing usage throughout the component.frontend/packages/dev-console/src/types/catalog.ts (2)
9-17: LGTM!The
SoftwareCatalogTypesConsoleConfigtype properly extendsK8sResourceKindand uses appropriate optional chaining for the nested config structure. This aligns well with the console operator config schema pattern.
3-7: Thestatefield andenabledarray are actively used throughout the codebase and the type definition is well-implemented. Thestatefield determines the filtering mode:'Enabled'operates as an allowlist (only types in theenabledarray are shown), while'Disabled'operates as a denylist (types in thedisabledarray are hidden). BothCatalogTypesConfigurationand the shared catalog utilities properly respect this semantic. No changes needed.frontend/packages/dev-console/src/actions/providers.tsx (2)
72-80: LGTM!The
useSoftwareCatalogProvidercorrectly uses theFeatureFlagHandlertype, aligning with the established pattern in this codebase. The hook calls within this handler follow theconsole.flag/hookProviderextension contract.
26-26: LGTM!New imports for
useConsoleOperatorConfigandSoftwareCatalogTypesConsoleConfigare appropriate for the new functionality.Also applies to: 39-39
frontend/packages/dev-console/console-extensions.json (6)
174-179: LGTM!The new
console.flag/hookProviderextension correctly wiresuseDisabledCatalogTypesto set feature flags based on console operator config. This follows the established pattern used byuseSoftwareCatalogProvider.
469-486: LGTM!The
disallowedflags for BuilderImage item-type and item-provider are correctly added. The flag nameSOFTWARE_CATALOG_TYPE_BUILDERIMAGE_DISABLEDmatches the uppercase transformation inuseDisabledCatalogTypes.
495-512: LGTM!Template item-type and item-provider correctly gated with
SOFTWARE_CATALOG_TYPE_TEMPLATE_DISABLED.
521-538: LGTM!Devfile item-type and item-provider correctly gated with
SOFTWARE_CATALOG_TYPE_DEVFILE_DISABLED.
562-587: LGTM!The samples-catalog providers for BuilderImage and Devfile types are correctly gated with their respective
_DISABLEDflags. This ensures that when a catalog type is disabled, both the main catalog and the samples catalog respect the configuration.
539-561: Flag naming and usage are consistent and correct.The codebase dynamically generates
SOFTWARE_CATALOG_TYPE_<TYPE>_DISABLEDflags from the operator config's disabled array values (seefrontend/packages/dev-console/src/actions/providers.tsx). This ensures that cluster admins simply list catalog type names (e.g.,"Sample") and the flag name generation handles consistency automatically.Note:
CONSOLESAMPLEis a separate feature flag for the ConsoleSample provider itself and serves a different purpose than the catalog type disabling mechanism. The two are used correctly.
logonoff
force-pushed
the
OCPBUGS-72585-devfile
branch
from
951e59a to
b0ab1fb
Compare
|
[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/helm
logonoff
force-pushed
the
OCPBUGS-72585-devfile
branch
from
b0ab1fb to
706397e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@frontend/packages/console-shared/src/components/catalog/hooks/useCatalogExtensions.ts:
- Around line 34-44: The isNotDisabledType predicate currently returns falsy
when e.properties.type is undefined, which incorrectly filters out
global/typeless extensions; update the isNotDisabledType callback so it returns
true when e.properties.type is not set (i.e., allow typeless entries) and only
checks catalogTypes.disabled.includes(e.properties.type) when a type exists;
ensure you modify the function named isNotDisabledType (used alongside
CatalogCategoriesProvider predicates) to first short-circuit for
!e.properties.type and otherwise check the disabled list.
🧹 Nitpick comments (3)
frontend/packages/console-shared/src/components/cluster-configuration/useConsoleOperatorConfig.ts (1)
7-13: Consider making the hook return type explicit (to prevent tuple-shape drift).Since callers commonly destructure this hook, an explicit return type (based on
useK8sWatchResource) can help catch mismatches early if the watch hook’s signature evolves.frontend/packages/console-shared/src/components/catalog/hooks/useCatalogExtensions.ts (2)
3-4: Type import layering: console-shared depending on dev-console types is awkward long-term.Even as a type-only import, this creates coupling from
console-sharedtodev-console. Consider movingSoftwareCatalogTypesConsoleConfig(or a narrower “developerCatalog.types” shape) into a shared location withinconsole-shared(or a common types package) to keep dependency direction clean.
46-116: Add focused tests for disabled types + type-less extensions.Given the new filtering is now a cross-cutting gate for all resolved extension kinds, please add coverage for:
- a disabled type being filtered out for each extension kind you expect to be typed
- an extension with no
properties.typestill being retained (if supported/expected), especially forCatalogCategoriesProviderAlso applies to: 118-147
📜 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 (5)
frontend/packages/console-shared/src/components/catalog/hooks/useCatalogExtensions.tsfrontend/packages/console-shared/src/components/cluster-configuration/index.tsfrontend/packages/console-shared/src/components/cluster-configuration/useConsoleOperatorConfig.tsfrontend/packages/dev-console/src/components/catalog/CatalogTypesConfiguration.tsxfrontend/packages/dev-console/src/types/catalog.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/packages/dev-console/src/types/catalog.ts
- frontend/packages/dev-console/src/components/catalog/CatalogTypesConfiguration.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/packages/console-shared/src/components/catalog/hooks/useCatalogExtensions.ts (2)
frontend/packages/console-shared/src/components/cluster-configuration/useConsoleOperatorConfig.ts (1)
useConsoleOperatorConfig(7-13)frontend/packages/dev-console/src/types/catalog.ts (1)
SoftwareCatalogTypesConsoleConfig(9-17)
frontend/packages/console-shared/src/components/cluster-configuration/useConsoleOperatorConfig.ts (1)
frontend/packages/console-shared/src/components/cluster-configuration/index.ts (1)
useConsoleOperatorConfig(3-3)
🔇 Additional comments (1)
frontend/packages/console-shared/src/components/cluster-configuration/index.ts (1)
3-3: Refactor to named export is safe—no default imports remain.The codebase has been successfully sanitized: no default imports of
useConsoleOperatorConfigexist. Both the source file and the index correctly export it as a named export, forcing all consumers to use the named import pattern. The transition is complete with no breaking references lingering.
logonoff
force-pushed
the
OCPBUGS-72585-devfile
branch
from
706397e to
8f769d4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
@frontend/packages/console-shared/src/components/catalog/hooks/useCatalogExtensions.ts:
- Around line 34-48: The isNotDisabledType callback must handle
DeveloperCatalogTypes' state modes and guard against undefined arrays: if
e.properties.type is missing, keep returning true; otherwise read catalogTypes
from consoleConfig and if catalogTypes.state === 'Enabled' return
catalogTypes.enabled?.includes(e.properties.type) === true; if
catalogTypes.state === 'Disabled' and catalogTypes.disabled is undefined treat
that as "everything disabled" (return false), else return
!catalogTypes.disabled?.includes(e.properties.type); ensure you never call
.includes on an undefined array by using optional chaining or nullish coalescing
when checking enabled/disabled arrays.
- Line 3: The import of SoftwareCatalogTypesConsoleConfig (and similarly
INCONTEXT_ACTIONS_CONNECTS_TO) from the plugin package creates a forbidden
coupling; move the shared catalog types/constants into the console-shared
package and update imports: extract SoftwareCatalogTypesConsoleConfig into a new
type file (e.g., console-shared's types catalog export) and extract
INCONTEXT_ACTIONS_CONNECTS_TO into a console-shared constants module, export
them, then replace the import in useCatalogExtensions.ts (and any other
references) to import from the new console-shared types/constants instead of
from @console/dev-console.
🧹 Nitpick comments (1)
frontend/packages/console-shared/src/components/catalog/hooks/useCatalogExtensions.ts (1)
127-129: Redundant filtering —itemTypeExtensionsalready filtered.The
itemTypeExtensionsarray was already filtered byisNotDisabledTypeduringuseResolvedExtensionsresolution (line 54). This second.filter(isNotDisabledType)iterates the array again without effect.♻️ Remove redundant filter
(catalogType ? itemTypeExtensions.filter((e) => e.properties.type === catalogType) : itemTypeExtensions - ) - .filter(isNotDisabledType) - .map((e) => { + ).map((e) => {Also remove
isNotDisabledTypefrom the dependency array at line 150 if this is cleaned up.
📜 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 (5)
frontend/packages/console-shared/src/components/catalog/hooks/useCatalogExtensions.tsfrontend/packages/console-shared/src/components/cluster-configuration/index.tsfrontend/packages/console-shared/src/components/cluster-configuration/useConsoleOperatorConfig.tsfrontend/packages/dev-console/src/components/catalog/CatalogTypesConfiguration.tsxfrontend/packages/dev-console/src/types/catalog.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/packages/console-shared/src/components/cluster-configuration/useConsoleOperatorConfig.ts
- frontend/packages/dev-console/src/types/catalog.ts
- frontend/packages/console-shared/src/components/cluster-configuration/index.ts
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/packages/dev-console/src/components/catalog/CatalogTypesConfiguration.tsx (1)
frontend/packages/dev-console/src/types/catalog.ts (1)
DeveloperCatalogTypes(3-7)
🔇 Additional comments (2)
frontend/packages/dev-console/src/components/catalog/CatalogTypesConfiguration.tsx (1)
18-18: Clean type extraction to shared module — LGTM.Good separation of concerns moving
DeveloperCatalogTypesandSoftwareCatalogTypesConsoleConfigto a dedicated types module. This aligns with our pattern of keeping reusable types accessible across packages.Also applies to: 52-52
frontend/packages/console-shared/src/components/catalog/hooks/useCatalogExtensions.ts (1)
50-120: Consistent filtering across all extension types — good pattern.The
isNotDisabledTypepredicate is correctly applied to all relevant extension resolution callbacks (CatalogItemType,CatalogItemTypeMetadata,CatalogItemProvider,CatalogItemFilter,CatalogCategoriesProvider,CatalogItemMetadataProvider), and dependency arrays are properly updated.Once the logic issue in
isNotDisabledTypeis addressed, this will properly prevent disabled catalog hooks from executing.
frontend/packages/console-shared/src/components/catalog/hooks/useCatalogExtensions.ts
Outdated
Show resolved
Hide resolved
frontend/packages/console-shared/src/components/catalog/hooks/useCatalogExtensions.ts
Show resolved
Hide resolved
logonoff
force-pushed
the
OCPBUGS-72585-devfile
branch
from
8f769d4 to
c14b708
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/packages/console-shared/src/components/catalog/utils/catalog-utils.tsx (1)
127-132: Remove debug console.log statements before merging.These debug logs (
console.log('🔍 Enhanced keywordCompare called:...,console.log('📂 keywordCompare (No Search)..., etc.) will pollute the browser console in production. They should be removed or guarded behind a debug flag.🧹 Proposed fix to remove debug logs
export const keywordCompare = (filterString: string, items: CatalogItem[]): CatalogItem[] => { - // eslint-disable-next-line no-console - console.log('🔍 Enhanced keywordCompare called:', { - filterString, - itemCount: items.length, - catalogType: items[0]?.type || 'unknown', - }); - if (!filterString) { // No search term - sort by Red Hat priority and then alphabetically const sortedItems = [...items].sort((a, b) => {- // Reduced logging - detailed logging now happens in CatalogView after all filtering - if (sortedItems.length > 0 && sortedItems[0]?.type === 'operator') { - // eslint-disable-next-line no-console - console.log( - `📂 keywordCompare (No Search) - Red Hat Priority Sorting (${sortedItems.length} items)`, - ); - } - return sortedItems;- // Reduced logging - detailed logging now happens in CatalogView after all filtering - if (sortedItems.length > 0 && sortedItems[0]?.type === 'operator') { - // eslint-disable-next-line no-console - console.log( - `🔍 keywordCompare (Search: "${filterString}") - Relevance Scoring (${sortedItems.length} matches)`, - ); - } - // Remove the added properties before returningAlso applies to: 150-155, 197-202
🤖 Fix all issues with AI agents
In
@frontend/packages/console-shared/src/components/catalog/hooks/useCatalogExtensions.ts:
- Around line 31-43: The hook call to useGetAllDisabledSubCatalogs() is
returning an array-wrapped value, so disabledCatalogs is currently an array of
arrays and includes checks in isNotDisabledType never match; change the
assignment so disabledCatalogs is the inner flat array (destructure the returned
tuple/array or otherwise extract/flatten the first element from
useGetAllDisabledSubCatalogs()), then keep the isNotDisabledType logic the same
to correctly check disabledCatalogs.includes(e.properties.type).
🧹 Nitpick comments (1)
frontend/packages/console-shared/src/components/catalog/hooks/useCatalogExtensions.ts (1)
117-145: RedundantisNotDisabledTypefilter on already-filtered extensions.
itemTypeExtensionsis already filtered byisNotDisabledTypein theuseResolvedExtensionscallback at line 48-51. The additional.filter(isNotDisabledType)at line 123 is redundant and adds unnecessary iteration.♻️ Suggested simplification
const catalogTypeExtensions = useMemo<ResolvedExtension<CatalogItemType>[]>( () => (catalogType ? itemTypeExtensions.filter((e) => e.properties.type === catalogType) : itemTypeExtensions ) - .filter(isNotDisabledType) .map((e) => { const metadataExts = typeMetadataExtensions.filter( (em) => e.properties.type === em.properties.type, ); // ... rest unchanged }), - [catalogType, itemTypeExtensions, typeMetadataExtensions, isNotDisabledType], + [catalogType, itemTypeExtensions, typeMetadataExtensions], );
📜 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 (2)
frontend/packages/console-shared/src/components/catalog/hooks/useCatalogExtensions.tsfrontend/packages/console-shared/src/components/catalog/utils/catalog-utils.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/packages/console-shared/src/components/catalog/hooks/useCatalogExtensions.ts (2)
frontend/packages/console-shared/src/components/catalog/utils/catalog-utils.tsx (1)
useGetAllDisabledSubCatalogs(380-405)frontend/packages/console-dynamic-plugin-sdk/src/extensions/catalog.ts (3)
CatalogItemType(7-25)isCatalogItemType(112-114)isCatalogItemTypeMetadata(116-118)
frontend/packages/console-shared/src/components/catalog/utils/catalog-utils.tsx (2)
frontend/packages/console-plugin-sdk/src/api/useExtensions.ts (1)
useExtensions(44-90)frontend/packages/console-dynamic-plugin-sdk/src/extensions/catalog.ts (2)
CatalogItemType(7-25)isCatalogItemType(112-114)
🔇 Additional comments (2)
frontend/packages/console-shared/src/components/catalog/utils/catalog-utils.tsx (1)
2-2: LGTM on the hook switch fromuseResolvedExtensionstouseExtensions.The change is appropriate here. Both
useGetAllDisabledSubCatalogsanduseIsSoftwareCatalogEnabledonly accesstype.properties.type(a plain string), so there's no need for CodeRef resolution thatuseResolvedExtensionsprovides. This avoids unnecessary async resolution overhead for these utility functions.Also applies to: 11-11, 381-381, 409-409
frontend/packages/console-shared/src/components/catalog/hooks/useCatalogExtensions.ts (1)
45-53: LGTM on the comprehensive filtering approach.Solid implementation applying
isNotDisabledTypeconsistently across all six extension resolution hooks:CatalogItemType,CatalogItemTypeMetadata,CatalogItemProvider,CatalogItemFilter,CatalogCategoriesProvider, andCatalogItemMetadataProvider. The dependency arrays are correctly updated to includeisNotDisabledType.The fallback in
isNotDisabledType(lines 36-38) correctly handles extensions without atypeproperty by considering them not disabled — good defensive coding.Also applies to: 55-65, 67-76, 78-87, 89-102, 104-115
frontend/packages/console-shared/src/components/catalog/hooks/useCatalogExtensions.ts
Outdated
Show resolved
Hide resolved
logonoff
force-pushed
the
OCPBUGS-72585-devfile
branch
2 times, most recently
from
a79f9a7 to
567f091
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/packages/console-shared/src/components/catalog/utils/catalog-utils.tsx (2)
127-159: Remove console.log statements from production code.These debug logging statements will pollute the browser console in production. OpenShift Console follows PatternFly and K8s best practices where debug logging should be conditional or removed entirely.
🛠️ Proposed fix
export const keywordCompare = (filterString: string, items: CatalogItem[]): CatalogItem[] => { - // eslint-disable-next-line no-console - console.log('🔍 Enhanced keywordCompare called:', { - filterString, - itemCount: items.length, - catalogType: items[0]?.type || 'unknown', - }); - if (!filterString) { // No search term - sort by Red Hat priority and then alphabetically const sortedItems = [...items].sort((a, b) => { const aPriority = getRedHatPriority(a); const bPriority = getRedHatPriority(b); // Primary sort by Red Hat priority if (aPriority !== bPriority) { return bPriority - aPriority; } // Secondary sort by name (alphabetical) return a.name.toLowerCase().localeCompare(b.name.toLowerCase()); }); - // Reduced logging - detailed logging now happens in CatalogView after all filtering - if (sortedItems.length > 0 && sortedItems[0]?.type === 'operator') { - // eslint-disable-next-line no-console - console.log( - `📂 keywordCompare (No Search) - Red Hat Priority Sorting (${sortedItems.length} items)`, - ); - } - return sortedItems; }
197-207: Additional console.log to remove.Same issue as above - remove this debug logging from production code.
🛠️ Proposed fix
- // Reduced logging - detailed logging now happens in CatalogView after all filtering - if (sortedItems.length > 0 && sortedItems[0]?.type === 'operator') { - // eslint-disable-next-line no-console - console.log( - `🔍 keywordCompare (Search: "${filterString}") - Relevance Scoring (${sortedItems.length} matches)`, - ); - } - // Remove the added properties before returning return sortedItems.map(({ relevanceScore, redHatPriority, ...item }) => item); };
🧹 Nitpick comments (4)
frontend/packages/console-shared/src/components/catalog/utils/catalog-utils.tsx (2)
145-148: Add null safety fora.nameandb.namein sorting.If a
CatalogItemhas a missing or nullnameproperty, calling.toLowerCase()will throw. While unlikely given the data model, defensive coding here prevents potential runtime errors.♻️ Suggested improvement
// Secondary sort by name (alphabetical) - return a.name.toLowerCase().localeCompare(b.name.toLowerCase()); + return (a.name ?? '').toLowerCase().localeCompare((b.name ?? '').toLowerCase());
413-424: Potential performance concern: duplicateuseExtensionscalls.Both
useGetAllDisabledSubCatalogs(line 387) anduseIsSoftwareCatalogEnabled(line 415) calluseExtensions<CatalogItemType>(isCatalogItemType). SinceuseIsSoftwareCatalogEnabledalready callsuseGetAllDisabledSubCatalogs, you could return the catalog types from that hook instead of making a seconduseExtensionscall.♻️ Consider returning catalog types from useGetAllDisabledSubCatalogs
The second element of the tuple already contains
catalogTypeExtensionsin some code paths (line 404). You could standardize this to always return it, avoiding the duplicate hook call:-export const useGetAllDisabledSubCatalogs = (): [string[], string[]?] => { +export const useGetAllDisabledSubCatalogs = (): [string[], string[]] => { const catalogExtensionsArray = useExtensions<CatalogItemType>(isCatalogItemType); return useMemo(() => { const catalogTypeExtensions = catalogExtensionsArray.map((type) => type.properties.type); if (SOFTWARE_CATALOG_TYPES) { // ... existing logic ... } - return [[]]; + return [[], catalogTypeExtensions]; }, [catalogExtensionsArray]); }; export const useIsSoftwareCatalogEnabled = (): boolean => { - const [disabledSubCatalogs] = useGetAllDisabledSubCatalogs(); - const catalogExtensionsArray = useExtensions<CatalogItemType>(isCatalogItemType); - const catalogTypeExtensions = catalogExtensionsArray.map((type) => { - return type.properties.type; - }); + const [disabledSubCatalogs, catalogTypeExtensions] = useGetAllDisabledSubCatalogs(); // ... rest of logic };frontend/packages/console-shared/src/components/catalog/hooks/useCatalogExtensions.ts (2)
117-146: Redundant.filter(isNotDisabledType)on line 123.The
itemTypeExtensionsarray is already filtered byisNotDisabledTypein theuseResolvedExtensionscall on lines 45-53. Filtering again on line 123 is unnecessary and adds a small performance overhead.♻️ Remove redundant filter
const catalogTypeExtensions = useMemo<ResolvedExtension<CatalogItemType>[]>( () => (catalogType ? itemTypeExtensions.filter((e) => e.properties.type === catalogType) : itemTypeExtensions ) - .filter(isNotDisabledType) .map((e) => { const metadataExts = typeMetadataExtensions.filter( (em) => e.properties.type === em.properties.type, ); // ... rest unchanged }), - [catalogType, itemTypeExtensions, typeMetadataExtensions, isNotDisabledType], + [catalogType, itemTypeExtensions, typeMetadataExtensions], );
148-152: Mutating sort oncatalogProviderExtensionsoutside of useMemo.This
.sort()mutates the array in place, which can cause subtle bugs with React's referential equality checks. While it works becauseuseResolvedExtensionsreturns a new array on each resolution, wrapping inuseMemowith a spread copy would be safer and more idiomatic React.♻️ Use immutable sort pattern
- catalogProviderExtensions.sort((a, b) => { - const p1 = a.properties.priority ?? 0; - const p2 = b.properties.priority ?? 0; - return p1 - p2; - }); + const sortedCatalogProviderExtensions = useMemo( + () => + [...catalogProviderExtensions].sort((a, b) => { + const p1 = a.properties.priority ?? 0; + const p2 = b.properties.priority ?? 0; + return p1 - p2; + }), + [catalogProviderExtensions], + ); // Update return to use sortedCatalogProviderExtensions
📜 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 (2)
frontend/packages/console-shared/src/components/catalog/hooks/useCatalogExtensions.tsfrontend/packages/console-shared/src/components/catalog/utils/catalog-utils.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/packages/console-shared/src/components/catalog/hooks/useCatalogExtensions.ts (1)
frontend/packages/console-dynamic-plugin-sdk/src/extensions/catalog.ts (6)
CatalogItemType(7-25)isCatalogItemType(112-114)isCatalogItemTypeMetadata(116-118)CatalogItemProvider(43-58)isCatalogItemProvider(120-122)CatalogItemFilter(62-72)
🔇 Additional comments (8)
frontend/packages/console-shared/src/components/catalog/utils/catalog-utils.tsx (4)
1-13: LGTM on import reorganization.The switch from
useResolvedExtensionstouseExtensionsaligns with the PR objective of filtering disabled catalog types earlier in the pipeline. The addition ofuseMemofor memoization is appropriate.
20-45: Constants for scoring look well-structured.The scoring and priority constants are cleanly organized with
as constfor type safety. The threshold values are reasonable for the relevance calculation use case.
386-411: Core PR logic:useGetAllDisabledSubCatalogsimplementation looks correct.The hook properly:
- Uses
useExtensionsto get catalog type extensions- Computes disabled sub-catalogs based on server flags
- Memoizes the result with proper dependencies
The return type
[string[], string[]?]is a bit unconventional (second element is optional), but I see it's consumed correctly downstream.
381-385: Timing concern does not apply here—initialization is safe.
window.SERVER_FLAGSis set via server-side template injection inindex.html(window.SERVER_FLAGS = [[ .ServerFlags ]];) as an inline script in the<head>, which executes before any JavaScript modules load. Unlike dynamic flag initialization, this guarantees availability at module evaluation time. The pattern is intentional and correct.
SOFTWARE_CATALOG_TYPESandisCatalogTypeEnabledalso handle the missingdeveloperCatalogTypescase consistently: both treat an absent or falsy value as "all catalogs enabled" (the default), anduseGetAllDisabledSubCatalogsproperly guards access with anif (SOFTWARE_CATALOG_TYPES)check at line 396.frontend/packages/console-shared/src/components/catalog/hooks/useCatalogExtensions.ts (4)
18-18: LGTM on the import.The
useGetAllDisabledSubCatalogsimport aligns with the refactored utility function incatalog-utils.tsx.
31-43: Well-designedisNotDisabledTypepredicate.The callback is properly memoized with
useCallbackand handles the edge case wheretypeis undefined (considers it "not disabled"). This is a sensible default that prevents accidentally filtering out extensions that don't specify a type.
45-53: Consistent application ofisNotDisabledTypeacross all extension type guards.All six
useResolvedExtensionscalls now properly filter out disabled catalog types:
CatalogItemTypeCatalogItemTypeMetadataCatalogItemProviderCatalogItemFilterCatalogCategoriesProviderCatalogItemMetadataProviderThe dependency arrays are correctly updated to include
isNotDisabledType. This is the core fix for the PR objective.Also applies to: 55-65, 67-76, 78-87, 89-102, 104-115
162-174: Return tuple correctly aggregates all resolved states.The boolean at index 5 properly combines all resolution states, ensuring consumers know when the full extension set is ready. This is important for showing loading states in the UI.
logonoff
force-pushed
the
OCPBUGS-72585-devfile
branch
from
567f091 to
a57720d
Compare
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/packages/console-shared/src/components/catalog/utils/catalog-utils.tsx (2)
127-156: Remove debug console.log statements before merging.These debug logs (lines 129-133, 152-155) will pollute the browser console in production. If observability is needed, consider using the console's structured logging utilities or remove them entirely.
Additionally, line 147 assumes
a.nameandb.nameare always defined. If a catalog item lacks a name,toLowerCase()will throw:- return a.name.toLowerCase().localeCompare(b.name.toLowerCase()); + return (a.name ?? '').toLowerCase().localeCompare((b.name ?? '').toLowerCase());
197-207: Another debug log to remove.Lines 199-202 contain additional debug logging that should be removed for production.
The cleanup on line 206 stripping
relevanceScoreandredHatPrioritybefore returning is correct—ensures the returned items match the expectedCatalogItemtype.
🤖 Fix all issues with AI agents
In
@frontend/packages/console-shared/src/components/catalog/utils/catalog-utils.tsx:
- Around line 381-388: SOFTWARE_CATALOG_TYPES initialization can throw if
window.SERVER_FLAGS.developerCatalogTypes is malformed JSON; wrap the JSON.parse
call in a try/catch around the code that sets SOFTWARE_CATALOG_TYPES
(referencing SOFTWARE_CATALOG_TYPES and
window.SERVER_FLAGS.developerCatalogTypes), catch parse errors, log the error
(e.g., console.error) with context, and fall back to undefined (or an empty safe
value) so module initialization does not crash.
- Around line 390-415: The hook useGetAllDisabledSubCatalogs returns
inconsistent array shapes; change the branch that currently returns two elements
to return a single-element array like the others. Specifically, in the branch
where SOFTWARE_CATALOG_TYPES?.state === CatalogVisibilityState.Disabled and
SOFTWARE_CATALOG_TYPES?.disabled?.length > 0, return
[SOFTWARE_CATALOG_TYPES.disabled] (i.e., only the disabled list) instead of
[SOFTWARE_CATALOG_TYPES.disabled, catalogTypeExtensions], ensuring all code
paths return a single-element array (matching callers that do const
[disabledSubCatalogs] = useGetAllDisabledSubCatalogs()).
🧹 Nitpick comments (3)
frontend/packages/console-shared/src/components/catalog/utils/catalog-utils.tsx (3)
75-90: Consider defensive type coercion for array elements.While you're checking
Array.isArray, individual elements could be non-strings in edge cases (malformed extension data). A safer approach:- const keywords = item.tags.map((k) => k.toLowerCase()); + const keywords = item.tags.map((k) => String(k).toLowerCase());Same applies to
item.attributes.keywordson line 86. Low risk since extensions should be well-typed, but defensive coding prevents runtime surprises.
225-240: Redundant filtering for RELEVANCE case.When
sortOrder === CatalogSortOrder.RELEVANCE, the filtering in lines 227-233 computesfilteredItemsbut then line 240 passes the originalitemstokeywordCompare, which does its own filtering. This meanscalculateCatalogItemRelevanceScoreruns twice per item for the RELEVANCE case.Consider restructuring to avoid the double computation:
+ // Only pre-filter for ASC/DESC cases; RELEVANCE handles its own filtering + if (sortOrder === CatalogSortOrder.RELEVANCE) { + return keywordCompare(searchKeyword, items); + } + let filteredItems = items; if (searchKeyword) { const searchTerm = searchKeyword.toLowerCase(); filteredItems = items.filter((item) => { const relevanceScore = calculateCatalogItemRelevanceScore(searchTerm, item); return relevanceScore > 0; }); } switch (sortOrder) { - case CatalogSortOrder.RELEVANCE: - return keywordCompare(searchKeyword, items); - case CatalogSortOrder.ASC:
417-428: DuplicateuseExtensionscall—consider returning catalog types fromuseGetAllDisabledSubCatalogs.
useExtensions<CatalogItemType>(isCatalogItemType)is called both insideuseGetAllDisabledSubCatalogs(line 391) and again here (line 419). Since the hook already computescatalogTypeExtensionsinternally, consider returning it as part of the tuple to avoid the redundant hook call:-export const useGetAllDisabledSubCatalogs = () => { +export const useGetAllDisabledSubCatalogs = (): [string[], string[]] => { const catalogExtensionsArray = useExtensions<CatalogItemType>(isCatalogItemType); return useMemo(() => { const catalogTypeExtensions = catalogExtensionsArray.map((type) => type.properties.type); // ... existing logic, but always return [disabledSubCatalogs, catalogTypeExtensions]Then update
useIsSoftwareCatalogEnabledto destructure both values without callinguseExtensionsagain.
📜 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 (2)
frontend/packages/console-shared/src/components/catalog/hooks/useCatalogExtensions.tsfrontend/packages/console-shared/src/components/catalog/utils/catalog-utils.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/packages/console-shared/src/components/catalog/hooks/useCatalogExtensions.ts
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/packages/console-shared/src/components/catalog/utils/catalog-utils.tsx (2)
frontend/packages/console-plugin-sdk/src/api/useExtensions.ts (1)
useExtensions(44-90)frontend/packages/console-dynamic-plugin-sdk/src/extensions/catalog.ts (2)
CatalogItemType(7-25)isCatalogItemType(112-114)
🔇 Additional comments (3)
frontend/packages/console-shared/src/components/catalog/utils/catalog-utils.tsx (3)
1-13: LGTM on import changes.The migration from
useResolvedExtensionstouseExtensionsis appropriate here since you're only interested in the extension list itself, not the loading/error states. TheuseMemoimport aligns with the memoization added downstream.
20-45: Well-organized scoring constants.Good use of
as constfor immutability. The tiered scoring hierarchy (title > keyword > description) follows standard search relevance patterns. Consider adding a brief JSDoc comment above these constants explaining their purpose for future maintainers.
108-122: LGTM on Red Hat priority logic.The regex pattern handles common Red Hat provider name variations, and the fallback from
item.attributes?.providertoitem.providercorrectly handles different catalog item structures.
frontend/packages/console-shared/src/components/catalog/utils/catalog-utils.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/console-shared/src/components/catalog/utils/catalog-utils.tsx
Show resolved
Hide resolved
logonoff
force-pushed
the
OCPBUGS-72585-devfile
branch
from
a57720d to
a330a09
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/packages/console-shared/src/components/catalog/utils/catalog-utils.tsx (1)
145-151: Remove or guard debugconsole.logstatements before merging.These
console.logcalls with emoji prefixes (🔍,📂) are useful during development but will pollute the browser console in production. Even with the eslint-disable comments, shipping debug logging to users degrades the experience and can leak implementation details.Either remove them entirely or wrap them in a
__DEV__/process.env.NODE_ENV !== 'production'check if you need them for future debugging.♻️ Proposed fix to conditionally log only in development
export const keywordCompare = (filterString: string, items: CatalogItem[]): CatalogItem[] => { - // eslint-disable-next-line no-console - console.log('🔍 Enhanced keywordCompare called:', { - filterString, - itemCount: items.length, - catalogType: items[0]?.type || 'unknown', - }); + if (process.env.NODE_ENV === 'development') { + // eslint-disable-next-line no-console + console.log('🔍 Enhanced keywordCompare called:', { + filterString, + itemCount: items.length, + catalogType: items[0]?.type || 'unknown', + }); + }Apply similar changes to the other
console.logstatements at lines 170-173 and 217-220.Also applies to: 169-174, 216-221
frontend/packages/console-shared/src/components/catalog/hooks/useCatalogExtensions.ts (1)
148-152: MutatingcatalogProviderExtensionsarray in-place with.sort().
Array.prototype.sort()mutates the original array. SincecatalogProviderExtensionscomes fromuseResolvedExtensions, mutating it could cause subtle bugs if the same array reference is used elsewhere or if React's reconciliation relies on reference equality.🐛 Proposed fix to avoid mutation
- catalogProviderExtensions.sort((a, b) => { + const sortedProviderExtensions = [...catalogProviderExtensions].sort((a, b) => { const p1 = a.properties.priority ?? 0; const p2 = b.properties.priority ?? 0; return p1 - p2; });Then use
sortedProviderExtensionsin the return statement at line 164.
🧹 Nitpick comments (5)
frontend/packages/console-shared/src/components/catalog/utils/catalog-utils.tsx (4)
243-251: Redundant filtering:sortCatalogItemsfilters items, thenkeywordComparefilters again.When
sortOrder === CatalogSortOrder.RELEVANCE, lines 244-251 filter bysearchKeyword, but then line 258 passes the originalitems(notfilteredItems) tokeywordCompare, which performs its own filtering. This means:
- The filtering work at lines 247-250 is discarded for the RELEVANCE case.
- The comment on line 257 acknowledges this but it's still wasteful.
Consider restructuring to avoid the double-filter for RELEVANCE, or move the filtering logic to a single place.
♻️ Proposed simplification
export const sortCatalogItems = ( items: CatalogItem[], sortOrder: CatalogSortOrder = CatalogSortOrder.RELEVANCE, searchKeyword = '', ): CatalogItem[] => { if (!items || items.length === 0) { return items; } + // For RELEVANCE sorting, keywordCompare handles its own filtering + if (sortOrder === CatalogSortOrder.RELEVANCE) { + return keywordCompare(searchKeyword, items); + } + // First, filter items by search keyword if provided let filteredItems = items; if (searchKeyword) { const searchTerm = searchKeyword.toLowerCase(); filteredItems = items.filter((item) => { const relevanceScore = calculateCatalogItemRelevanceScore(searchTerm, item); return relevanceScore > 0; }); } // Then, sort the filtered items based on the selected sort order switch (sortOrder) { - case CatalogSortOrder.RELEVANCE: - // Use the existing keywordCompare function for relevance-based sorting - // Note: keywordCompare handles its own filtering, so we pass the original items - return keywordCompare(searchKeyword, items); - case CatalogSortOrder.ASC: // Sort alphabetically A-Z return [...filteredItems].sort((a, b) => { return a.name.toLowerCase().localeCompare(b.name.toLowerCase()); }); case CatalogSortOrder.DESC: // Sort alphabetically Z-A return [...filteredItems].sort((a, b) => { return b.name.toLowerCase().localeCompare(a.name.toLowerCase()); }); default: - // Fallback to relevance sorting return keywordCompare(searchKeyword, items); } };Also applies to: 254-258
381-393: Excessive optional chaining on a constant that's already checked for truthiness.After
if (SOFTWARE_CATALOG_TYPES)confirms the value is defined, usingSOFTWARE_CATALOG_TYPES?.state,SOFTWARE_CATALOG_TYPES?.enabled, etc. is redundant. The optional chaining adds unnecessary runtime checks.♻️ Proposed cleanup
export const isCatalogTypeEnabled = (catalogType: string): boolean => { if (SOFTWARE_CATALOG_TYPES) { if ( - SOFTWARE_CATALOG_TYPES?.state === CatalogVisibilityState.Enabled && - SOFTWARE_CATALOG_TYPES?.enabled?.length > 0 + SOFTWARE_CATALOG_TYPES.state === CatalogVisibilityState.Enabled && + SOFTWARE_CATALOG_TYPES.enabled?.length > 0 ) { - return SOFTWARE_CATALOG_TYPES?.enabled.includes(catalogType); + return SOFTWARE_CATALOG_TYPES.enabled.includes(catalogType); } - if (SOFTWARE_CATALOG_TYPES?.state === CatalogVisibilityState.Disabled) { - if (SOFTWARE_CATALOG_TYPES?.disabled?.length > 0) { - return !SOFTWARE_CATALOG_TYPES?.disabled.includes(catalogType); + if (SOFTWARE_CATALOG_TYPES.state === CatalogVisibilityState.Disabled) { + if (SOFTWARE_CATALOG_TYPES.disabled?.length > 0) { + return !SOFTWARE_CATALOG_TYPES.disabled.includes(catalogType); } return false; } } return true; };
425-437:useIsSoftwareCatalogEnabledcallsuseExtensionsredundantly.
useGetAllDisabledSubCatalogsalready callsuseExtensions<CatalogItemType>(isCatalogItemType)internally. ThenuseIsSoftwareCatalogEnabledcalls the same hook again at line 427, resulting in duplicate work.Consider either:
- Returning
catalogTypeExtensionsfromuseGetAllDisabledSubCatalogsas a second tuple element, or- Extracting the extensions fetch to a shared hook that both consume.
♻️ Proposed refactor to avoid redundant hook calls
-export const useGetAllDisabledSubCatalogs = () => { +export const useGetAllDisabledSubCatalogs = (): [string[], string[]] => { const catalogExtensionsArray = useExtensions<CatalogItemType>(isCatalogItemType); + const catalogTypeExtensions = useMemo( + () => catalogExtensionsArray.map((type) => type.properties.type), + [catalogExtensionsArray], + ); return useMemo(() => { - const catalogTypeExtensions = catalogExtensionsArray.map((type) => type.properties.type); - if (SOFTWARE_CATALOG_TYPES) { // ... existing logic ... } - return [[]]; - }, [catalogExtensionsArray]); + return [[], catalogTypeExtensions]; + }, [catalogTypeExtensions]); + + // Return both disabled catalogs and all catalog types + return [disabledSubCatalogs, catalogTypeExtensions]; }; export const useIsSoftwareCatalogEnabled = (): boolean => { - const [disabledSubCatalogs] = useGetAllDisabledSubCatalogs(); - const catalogExtensionsArray = useExtensions<CatalogItemType>(isCatalogItemType); - const catalogTypeExtensions = catalogExtensionsArray.map((type) => { - return type.properties.type; - }); + const [disabledSubCatalogs, catalogTypeExtensions] = useGetAllDisabledSubCatalogs(); // ... rest unchanged ... };
20-36: Remove debug console.log statements from keywordCompare function.Lines 147–151, 171–174, and 218–221 contain
console.logcalls in production code. These debugging statements should be removed entirely or gated behind a feature flag. Keeping them increases bundle size and pollutes production logs.frontend/packages/console-shared/src/components/catalog/hooks/useCatalogExtensions.ts (1)
117-145: Redundant.filter(isNotDisabledType)at line 123—extensions are already filtered.
itemTypeExtensionsis already filtered byisNotDisabledTypein theuseResolvedExtensionscallback at line 49. Filtering again at line 123 is unnecessary and adds overhead for each render.♻️ Proposed fix to remove redundant filter
const catalogTypeExtensions = useMemo<ResolvedExtension<CatalogItemType>[]>( () => (catalogType ? itemTypeExtensions.filter((e) => e.properties.type === catalogType) : itemTypeExtensions - ) - .filter(isNotDisabledType) - .map((e) => { + ).map((e) => { const metadataExts = typeMetadataExtensions.filter( (em) => e.properties.type === em.properties.type, ); // ... rest unchanged }), - [catalogType, itemTypeExtensions, typeMetadataExtensions, isNotDisabledType], + [catalogType, itemTypeExtensions, typeMetadataExtensions], );
📜 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 (2)
frontend/packages/console-shared/src/components/catalog/hooks/useCatalogExtensions.tsfrontend/packages/console-shared/src/components/catalog/utils/catalog-utils.tsx
🔇 Additional comments (4)
frontend/packages/console-shared/src/components/catalog/utils/catalog-utils.tsx (1)
398-423: Hook returns a memoized tuple structure correctly; minor note on return type.The refactoring to
useExtensionsanduseMemolooks correct. The dependency array properly includescatalogExtensionsArray.One small observation: returning
[disabledSubCatalogs](a single-element array) as a tuple is a bit unusual. If future consumers only need the array, consider returning it directly. However, if this pattern is intentional for API consistency with other hooks or to allow future expansion, it's acceptable.frontend/packages/console-shared/src/components/catalog/hooks/useCatalogExtensions.ts (3)
31-43: Well-implemented predicate for filtering disabled catalog types.The
isNotDisabledTypecallback is correctly memoized withuseCallbackand properly handles the case wheretypeis undefined (returnstrue, allowing type-less extensions through). The dependency array correctly includesdisabledCatalogs.This directly addresses the PR objective of preventing hooks from running for disabled catalog types.
45-53: Filter predicates correctly integrateisNotDisabledType.The type guards now properly compose
isCatalogItemType/isCatalogItemTypeMetadatawithisNotDisabledTypeand the catalog type filter. The dependency arrays are updated to includeisNotDisabledType, ensuring re-evaluation when disabled catalogs change.Also applies to: 58-65
162-174: Return tuple correctly aggregates all resolved states.The boolean at line 168-173 properly combines all resolved flags, ensuring consumers know when all extensions are fully loaded. The overall implementation achieves the PR's goal: disabled catalog types are filtered out before extension resolution, preventing their hooks from executing.
logonoff
force-pushed
the
OCPBUGS-72585-devfile
branch
from
a330a09 to
e0bca65
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/packages/console-shared/src/components/catalog/utils/catalog-utils.tsx (2)
149-230: Removeconsole.logfrom shared sorting utilities (too noisy for prod).
These logs will fire on every sort and can flood the console (and any log capture), impacting performance and signal-to-noise. If you need diagnostics, gate behind an explicit debug flag.Proposed minimal fix
export const keywordCompare = (filterString: string, items: CatalogItem[]): CatalogItem[] => { - // eslint-disable-next-line no-console - console.log('🔍 Enhanced keywordCompare called:', { - filterString, - itemCount: items.length, - catalogType: items[0]?.type || 'unknown', - }); - if (!filterString) { // No search term - sort by Red Hat priority and then alphabetically const sortedItems = [...items].sort((a, b) => { const aPriority = getRedHatPriority(a); const bPriority = getRedHatPriority(b); @@ - if (sortedItems.length > 0 && sortedItems[0]?.type === 'operator') { - // eslint-disable-next-line no-console - console.log( - `📂 keywordCompare (No Search) - Red Hat Priority Sorting (${sortedItems.length} items)`, - ); - } - return sortedItems; } @@ - if (sortedItems.length > 0 && sortedItems[0]?.type === 'operator') { - // eslint-disable-next-line no-console - console.log( - `🔍 keywordCompare (Search: "${filterString}") - Relevance Scoring (${sortedItems.length} matches)`, - ); - } - // Remove the added properties before returning return sortedItems.map(({ relevanceScore, redHatPriority, ...item }) => item); };
432-444: Simplify/strengthen “software catalog enabled” logic (avoid length + stringify compare).
Current logic is brittle with duplicates/unknown types; use set membership: “enabled iff exists at least one catalog type not disabled”.Proposed fix
export const useIsSoftwareCatalogEnabled = (): boolean => { const [disabledSubCatalogs] = useGetAllDisabledSubCatalogs(); const catalogExtensionsArray = useExtensions<CatalogItemType>(isCatalogItemType); - const catalogTypeExtensions = catalogExtensionsArray.map((type) => { - return type.properties.type; - }); - if (disabledSubCatalogs?.length === catalogTypeExtensions?.length) { - return ( - JSON.stringify(disabledSubCatalogs.sort()) !== JSON.stringify(catalogTypeExtensions.sort()) - ); - } - return true; + const catalogTypeExtensions = catalogExtensionsArray.map((type) => type.properties.type); + + // Enabled if there's at least one catalog type that is not disabled. + return catalogTypeExtensions.some((t) => !disabledSubCatalogs.includes(t)); };
🧹 Nitpick comments (4)
frontend/packages/console-shared/src/components/catalog/hooks/useCatalogExtensions.ts (2)
18-43: Good: centralized “disabled type” predicate; watch for predicate churn ifdisabledCatalogsidentity isn’t stable.
IfuseGetAllDisabledSubCatalogs()returns a fresh array each render,isNotDisabledTypewill change and force alluseResolvedExtensionspredicates to be recreated (potentially triggering extra work). Consider ensuring the disabled list is memoized/stable at the source.
117-146: Minor: redundant.filter(isNotDisabledType)(already filtered upstream) + repeated metadata scans.
Given the type-guard predicates already applyisNotDisabledType, the extra filter is likely unnecessary. Also,typeMetadataExtensions.filter(...)inside.map(...)is O(N*M); consider pre-indexing metadata by type if this runs hot.frontend/packages/console-shared/src/utils/__tests__/isSubCatalogTypeEnabled.spec.ts (1)
2-17: Make theuseExtensionsmock per-test to avoid cross-test coupling.
MoveuseExtensionsMock.mockReturnValue(...)intobeforeEach(and reset/clear mocks) so future tests can safely override without order-dependence.Proposed tweak
const useExtensionsMock = useExtensions as jest.Mock; -useExtensionsMock.mockReturnValue(mockExtensions); +beforeEach(() => { + useExtensionsMock.mockReset(); + useExtensionsMock.mockReturnValue(mockExtensions); +}); beforeEach(() => { delete window.SERVER_FLAGS.developerCatalogTypes; });frontend/packages/console-shared/src/components/catalog/utils/catalog-utils.tsx (1)
26-41:useMemois undermined by unstablesoftwareCatalogTypesidentity (recomputes every render).
BecausegetSoftwareCatalogTypes()returns a new object each call, it defeats memoization and can cascade into downstream callback churn (notably inuseCatalogExtensions). Suggest memoizing on the raw string value.Proposed tweak
-const getSoftwareCatalogTypes = (): SoftwareCatalogTypesConfig | undefined => { - if (!window.SERVER_FLAGS.developerCatalogTypes) { +const getSoftwareCatalogTypes = ( + developerCatalogTypes?: string, +): SoftwareCatalogTypesConfig | undefined => { + if (!developerCatalogTypes) { return undefined; } try { - return JSON.parse(window.SERVER_FLAGS.developerCatalogTypes) as SoftwareCatalogTypesConfig; + return JSON.parse(developerCatalogTypes) as SoftwareCatalogTypesConfig; } catch (e) { // eslint-disable-next-line no-console console.error('Failed to parse developerCatalogTypes:', e); return undefined; } }; export const useGetAllDisabledSubCatalogs = () => { const catalogExtensionsArray = useExtensions<CatalogItemType>(isCatalogItemType); - const softwareCatalogTypes = getSoftwareCatalogTypes(); + const developerCatalogTypes = window.SERVER_FLAGS.developerCatalogTypes; + const softwareCatalogTypes = useMemo( + () => getSoftwareCatalogTypes(developerCatalogTypes), + [developerCatalogTypes], + );Also applies to: 404-430
📜 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 (3)
frontend/packages/console-shared/src/components/catalog/hooks/useCatalogExtensions.tsfrontend/packages/console-shared/src/components/catalog/utils/catalog-utils.tsxfrontend/packages/console-shared/src/utils/__tests__/isSubCatalogTypeEnabled.spec.ts
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/packages/console-shared/src/components/catalog/hooks/useCatalogExtensions.ts (2)
frontend/packages/console-shared/src/components/catalog/utils/catalog-utils.tsx (1)
useGetAllDisabledSubCatalogs(404-430)frontend/packages/console-dynamic-plugin-sdk/src/extensions/catalog.ts (7)
CatalogItemType(7-25)isCatalogItemType(112-114)isCatalogItemTypeMetadata(116-118)CatalogItemProvider(43-58)isCatalogItemProvider(120-122)CatalogItemFilter(62-72)isCatalogItemFilter(124-126)
frontend/packages/console-shared/src/components/catalog/utils/catalog-utils.tsx (3)
frontend/public/module/auth.js (1)
window(5-12)frontend/packages/console-plugin-sdk/src/api/useExtensions.ts (1)
useExtensions(44-90)frontend/packages/console-dynamic-plugin-sdk/src/extensions/catalog.ts (2)
CatalogItemType(7-25)isCatalogItemType(112-114)
🔇 Additional comments (2)
frontend/packages/console-shared/src/components/catalog/hooks/useCatalogExtensions.ts (1)
45-115: Correct: exclude disabled types at the extension-selection boundary (prevents providers/filters/metadata hooks from resolving).
This is the right place to cut off execution paths before hooks resolve/run.frontend/packages/console-shared/src/utils/__tests__/isSubCatalogTypeEnabled.spec.ts (1)
76-120: Good: hook assertions are now executed undertestHook(prevents invalid hook usage).
|
/retest |
1 similar comment
|
/retest |
|
@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. |
2 participants
Previously, if a catalog/item-type was disabled in console-operator config, the hooks associated with that catalog type would still resolve and run.
Now, if a catalog category is disabled, the associated hooks will not run.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.