CONSOLE-5030 Refactor remaining firehose impacted components#16039
CONSOLE-5030 Refactor remaining firehose impacted components#16039cajieh wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cajieh 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/olm
cajieh
force-pushed
the
refactor-remaining-firehose-impacted-components
branch
2 times, most recently
from
1263161 to
d7e6b94
Compare
cajieh
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
/test e2e-gcp-console |
cajieh
force-pushed
the
refactor-remaining-firehose-impacted-components
branch
from
d7e6b94 to
a37006c
Compare
📝 WalkthroughWalkthroughThis change systematically replaces Firehose-based data provisioning with direct Kubernetes watch hooks (useK8sWatchResource and useK8sWatchResources) across 10 files spanning multiple console packages. Components now fetch and manage resource subscriptions internally via hooks rather than relying on wrapper components to inject data. Type signatures transition from FirehoseResult patterns to composite objects with explicit { data, loaded, loadError } shapes. Page components lose their props parameters where Firehose previously injected resources, instead computing and passing resources directly to child components. Test mocks are updated to reflect the hook-based data flow. Approximately 317 lines changed across console-app, container-security, operator-lifecycle-manager, shipwright-plugin, and topology packages. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
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/src/components/catalog-source.tsx (1)
240-274:⚠️ Potential issue | 🟠 MajorThrow
loadErrorto trigger the fallback UI when package manifest fetch fails.The condition on line 243 only checks
loaded && data, but whenloadedistruewithloadErrorset (fetch failed), the component returns<LoadingBox />on line 266, leaving users watching a spinner indefinitely. The fallback UI (lines 268-272) is properly configured but never triggered because it only fires on thrown errors.Check and throw
loadErrorbefore testing the data:Proposed fix
(createProps) => { + if (createProps.packageManifest.loadError) { + throw createProps.packageManifest.loadError; + } if (createProps.packageManifest.loaded && createProps.packageManifest.data) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/operator-lifecycle-manager/src/components/catalog-source.tsx` around lines 240 - 274, The packageManifest fetch error isn't propagated to the withFallback fallback because the code only checks createProps.packageManifest.loaded and data; update the component wrapped by withFallback (the function passed into withFallback inside requireOperatorGroup/Create) to first check if createProps.packageManifest.loadError is set and if so throw that error so the fallback UI is triggered; locate references to packageManifest, CreateProps, withFallback and CreateYAML in the current function and throw createProps.packageManifest.loadError before the existing loaded/data checks so the ConsoleEmptyState is shown instead of the LoadingBox on fetch failure.
🧹 Nitpick comments (7)
frontend/packages/container-security/src/components/image-manifest-vuln.tsx (1)
419-427: Type definition reflects redundancy.The type mirrors the code redundancy—
loadedappears both at the top level (line 420) and nested inimageManifestVuln(line 424). Once you decide on a single source of truth for the loaded state per the previous comment, update the type accordingly.Suggested type simplification
If relying solely on the nested structure:
export type ContainerVulnerabilitiesProps = { - loaded: boolean; pod: PodKind; imageManifestVuln: { data: ImageManifestVuln[]; loaded: boolean; loadError?: any; }; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/container-security/src/components/image-manifest-vuln.tsx` around lines 419 - 427, The ContainerVulnerabilitiesProps type has duplicated loaded flags; remove the redundant top-level loaded and keep the nested imageManifestVuln.loaded (or vice versa if you prefer a single top-level source); update the type definition (ContainerVulnerabilitiesProps) so only one loaded property exists and adjust consumers to read from the chosen location (imageManifestVuln.loaded or the top-level loaded) to match the code path that determines loading state.frontend/packages/operator-lifecycle-manager/src/components/catalog-source.tsx (3)
223-238: Memoize URLSearchParams to avoid repeated instantiation on each render.
new URLSearchParams(location.search)is called inline within the hook configuration object. Since hooks re-run on every render, this creates unnecessary object allocations. Extract the search params once before the hook call.♻️ Proposed refactor
const params = useParams(); const location = useLocation(); + const searchParams = new URLSearchParams(location.search); const resources = useK8sWatchResources<{ packageManifest: PackageManifestKind; operatorGroup: OperatorGroupKind[]; }>({ packageManifest: { kind: referenceForModel(PackageManifestModel), isList: false, - name: new URLSearchParams(location.search).get('pkg'), - namespace: new URLSearchParams(location.search).get('catalogNamespace'), + name: searchParams.get('pkg'), + namespace: searchParams.get('catalogNamespace'), }, operatorGroup: { kind: referenceForModel(OperatorGroupModel), isList: true, namespace: params.ns, }, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/operator-lifecycle-manager/src/components/catalog-source.tsx` around lines 223 - 238, The hook call to useK8sWatchResources repeatedly constructs new URLSearchParams from location.search inside its config object causing unnecessary allocations; extract const searchParams = new URLSearchParams(location.search) (or useMemo to memoize it) before calling useK8sWatchResources and then use searchParams.get('pkg') and searchParams.get('catalogNamespace') in the packageManifest config so the same URLSearchParams instance is reused across renders (adjust any references inside the component accordingly).
249-261: URLSearchParams instantiated again inside template; reuse extracted variable.Lines 256-257 create new
URLSearchParamsobjects. After extractingsearchParamsper the earlier suggestion, reuse it here to maintain consistency and avoid redundant allocations.♻️ Proposed refactor (assumes searchParams extracted earlier)
const template = ` apiVersion: ${SubscriptionModel.apiGroup}/${SubscriptionModel.apiVersion} kind: ${SubscriptionModel.kind}, metadata: generateName: ${pkg.metadata.name}- namespace: default spec: - source: ${new URLSearchParams(location.search).get('catalog')} - sourceNamespace: ${new URLSearchParams(location.search).get('catalogNamespace')} + source: ${searchParams.get('catalog')} + sourceNamespace: ${searchParams.get('catalogNamespace')} name: ${pkg.metadata.name} startingCSV: ${channel.currentCSV} channel: ${channel.name} `;Note: Accessing
searchParamsfrom the outer closure is valid sinceCreateis defined withinCreateSubscriptionYAML's scope.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/operator-lifecycle-manager/src/components/catalog-source.tsx` around lines 249 - 261, The template is creating new URLSearchParams instances for source and sourceNamespace; reuse the previously extracted searchParams variable from the outer CreateSubscriptionYAML scope instead. In the Create function (and specifically the template string in CreateSubscriptionYAML/Create), replace new URLSearchParams(location.search).get('catalog') and new URLSearchParams(location.search).get('catalogNamespace') with searchParams.get('catalog') and searchParams.get('catalogNamespace') respectively so you avoid redundant allocations and keep behavior consistent.
214-218: Consider extractingCreatePropstype for reuse and stricterloadErrortyping.The
CreatePropstype is defined inline within the component. For test type safety and potential reuse, consider exporting it. Additionally,loadError?: anycould be narrowed toloadError?: Errorfor better type inference downstream.♻️ Optional type improvement
+export type CreateSubscriptionYAMLProps = { + packageManifest: { loaded: boolean; data?: PackageManifestKind; loadError?: Error }; + operatorGroup: { loaded: boolean; data?: OperatorGroupKind[]; loadError?: Error }; +}; export const CreateSubscriptionYAML: FC = (props) => { - type CreateProps = { - packageManifest: { loaded: boolean; data?: PackageManifestKind; loadError?: any }; - operatorGroup: { loaded: boolean; data?: OperatorGroupKind[]; loadError?: any }; - };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/operator-lifecycle-manager/src/components/catalog-source.tsx` around lines 214 - 218, Extract and export the inline CreateProps type so it can be reused and imported elsewhere, and tighten its loadError types from any to Error; specifically, refactor the CreateSubscriptionYAML component to accept the exported CreateProps type (use CreateProps for the component props) and change packageManifest.loadError?: any and operatorGroup.loadError?: any to loadError?: Error so downstream code/tests get stricter typing.frontend/packages/operator-lifecycle-manager/src/components/__tests__/catalog-source.spec.tsx (1)
177-210: Consider adding a test case forloadErrorscenario.The tests cover
loaded: trueandloaded: falsestates, but there's no test verifying behavior whenloadErroris populated. Since the implementation now exposesloadErrorinCreateProps, a test ensuring the component handles fetch failures gracefully would improve coverage.it('handles package manifest load error gracefully', () => { mockUseK8sWatchResources.mockReturnValue({ packageManifest: { loaded: true, data: undefined, loadError: new Error('Not found') }, operatorGroup: { loaded: true, data: [], loadError: null }, }); renderWithProviders(<CreateSubscriptionYAML />); // Assert fallback UI is rendered expect(screen.getByText(/Package not found/i)).toBeVisible(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/operator-lifecycle-manager/src/components/__tests__/catalog-source.spec.tsx` around lines 177 - 210, Add a test covering the loadError path by mocking mockUseK8sWatchResources to return packageManifest with loaded: true (or false, depending on component logic), data: undefined, and loadError: new Error('Not found'), render CreateSubscriptionYAML, and assert the component shows the expected fallback UI (e.g., a "Package not found" message or other error UI your component renders when packageManifest.loadError is present); target the existing test block for CreateSubscriptionYAML and use screen.getByText to verify the error/fallback UI is visible.frontend/packages/operator-lifecycle-manager/src/components/topology/sidebar/TopologyOperatorBackedResources.tsx (1)
96-121: MemoizeresourcesToGetto reduce watchedResources recomputation overhead.
resourcesToGetis recreated each render (viadefaultResources.map), causingwatchedResourcesto be recomputed unnecessarily even when content is identical. WhileuseK8sWatchResourcesinternally applies deep comparison viauseDeepCompareMemoize(preventing actual watch churn), the repeated recomputation and comparison still incurs computational cost. StabilizingresourcesToGetwithuseMemowould eliminate this overhead:♻️ Suggested optimization
- const resourcesToGet = - providedAPI?.resources ?? - (defaultResources.map((kind) => ({ - kind, - })) as CRDDescription['resources']); + const resourcesToGet = useMemo( + () => + providedAPI?.resources ?? + (defaultResources.map((kind) => ({ + kind, + })) as CRDDescription['resources']), + [providedAPI?.resources], + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/operator-lifecycle-manager/src/components/topology/sidebar/TopologyOperatorBackedResources.tsx` around lines 96 - 121, resourcesToGet is being recreated each render (via defaultResources.map), causing watchedResources to be recomputed unnecessarily; wrap the computation that builds resourcesToGet in a useMemo so it only changes when its inputs change (e.g., defaultResources or any props used to compute it), then keep using resourcesToGet in the existing watchedResources useMemo dependency array so watchedResources only recomputes when the stabilized resourcesToGet actually changes.frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-subscribe.tsx (1)
1231-1269: DRY up URLSearchParams parsing for watch inputs.The repeated
new URLSearchParams(window.location.search)calls make the watch config harder to read and maintain. Capturing the values once improves clarity and keeps all selectors in sync.Refactor to reuse parsed query params
export const OperatorHubSubscribePage: FC = () => { const [activeNamespace] = useActiveNamespace(); + const searchParams = new URLSearchParams(window.location.search); + const catalogNamespace = searchParams.get('catalogNamespace'); + const pkg = searchParams.get('pkg'); + const catalog = searchParams.get('catalog'); + const targetNamespace = searchParams.get('targetNamespace'); const resources = useK8sWatchResources<{ operatorGroup: OperatorGroupKind[]; packageManifest: PackageManifestKind[]; subscription: SubscriptionKind[]; }>({ operatorGroup: { isList: true, kind: referenceForModel(OperatorGroupModel), }, packageManifest: { isList: true, kind: referenceForModel(PackageManifestModel), - namespace: new URLSearchParams(window.location.search).get('catalogNamespace'), - fieldSelector: `metadata.name=${new URLSearchParams(window.location.search).get('pkg')}`, + namespace: catalogNamespace, + fieldSelector: `metadata.name=${pkg}`, selector: { matchLabels: { - catalog: new URLSearchParams(window.location.search).get('catalog'), + catalog, }, }, }, subscription: { isList: true, kind: referenceForModel(SubscriptionModel), }, }); return ( <OperatorHubSubscribe {...resources} namespace={activeNamespace} loaded={loaded} loadError={loadError} - targetNamespace={new URLSearchParams(window.location.search).get('targetNamespace') || null} + targetNamespace={targetNamespace || null} /> ); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-subscribe.tsx` around lines 1231 - 1269, The repeated new URLSearchParams(window.location.search) calls in OperatorHubSubscribePage make the useK8sWatchResources call hard to read and risk inconsistent selectors; parse the query once into a const (e.g., const params = new URLSearchParams(window.location.search)) at the top of OperatorHubSubscribePage and replace all inline calls with params.get('catalogNamespace'), params.get('pkg'), params.get('catalog'), and params.get('targetNamespace') when building the packageManifest watch config and when passing targetNamespace prop to OperatorHubSubscribe so the selectors stay consistent and the code is cleaner.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/packages/container-security/src/components/image-manifest-vuln.tsx`:
- Around line 411-415: Remove the redundant top-level loaded prop from the
ContainerVulnerabilities call and instead derive loaded from
imageManifestVuln.loaded; also surface and handle the loadError by updating the
ContainerVulnerabilities component (e.g., inside the render path around where it
currently reads imageManifestVuln and loaded) to check
imageManifestVuln.loadError (or the passed loadError) and render an error
state/message instead of an indefinite spinner when truthy, ensuring
types/signature for ContainerVulnerabilities (props) are updated to accept
imageManifestVuln with data/loaded/loadError and no separate loaded prop.
In
`@frontend/packages/operator-lifecycle-manager/src/components/operator-install-page.tsx`:
- Around line 521-548: Mark the clusterServiceVersion watch as optional in the
useK8sWatchResources call (set optional: true for the clusterServiceVersion
entry alongside subscription and installPlans) and update OperatorInstallStatus
to check for resource load errors before relying on presence of data: inspect
resources.clusterServiceVersion.loadError (and other resources' loadError) and
surface a failure/stop the "Installing..." state when a 404 or other loadError
exists—mirror the loadError handling pattern used in operator-hub-page.tsx for
robust early-not-found handling.
In
`@frontend/packages/shipwright-plugin/src/components/build-form/PushSecretDropdown.tsx`:
- Around line 31-44: The resources variable in PushSecretDropdown is incorrectly
built as an object literal but ResourceDropdown expects an array of
FirehoseResult items; change the resources value to an array containing an
object with keys data, loaded, loadError, and kind (use SecretModel.kind) and
ensure data uses a safe fallback (e.g., secrets || []) so secrets being
undefined before loaded doesn't break getDropdownList's destructuring; update
the resources assignment where useK8sWatchResource returns [secrets, loaded,
loadError] and pass that array into ResourceDropdown.
In `@frontend/packages/topology/src/components/dropdowns/ApplicationDropdown.tsx`:
- Around line 16-25: ApplicationDropdown currently calls
getBaseWatchedResources(namespace) with an optional
ApplicationDropdownProps.namespace which can be undefined, producing invalid
watch configs; update ApplicationDropdown to guard the namespace before building
watches — e.g., check ApplicationDropdownProps.namespace and if undefined pass a
safe fallback (empty string) or short-circuit and return empty resources to use
in useK8sWatchResources, so getBaseWatchedResources and useK8sWatchResources
always receive a valid string or a no-op resource set; ensure you update the
call sites using getBaseWatchedResources and the resources prop passed into
ResourceDropdown accordingly.
---
Outside diff comments:
In
`@frontend/packages/operator-lifecycle-manager/src/components/catalog-source.tsx`:
- Around line 240-274: The packageManifest fetch error isn't propagated to the
withFallback fallback because the code only checks
createProps.packageManifest.loaded and data; update the component wrapped by
withFallback (the function passed into withFallback inside
requireOperatorGroup/Create) to first check if
createProps.packageManifest.loadError is set and if so throw that error so the
fallback UI is triggered; locate references to packageManifest, CreateProps,
withFallback and CreateYAML in the current function and throw
createProps.packageManifest.loadError before the existing loaded/data checks so
the ConsoleEmptyState is shown instead of the LoadingBox on fetch failure.
---
Nitpick comments:
In `@frontend/packages/container-security/src/components/image-manifest-vuln.tsx`:
- Around line 419-427: The ContainerVulnerabilitiesProps type has duplicated
loaded flags; remove the redundant top-level loaded and keep the nested
imageManifestVuln.loaded (or vice versa if you prefer a single top-level
source); update the type definition (ContainerVulnerabilitiesProps) so only one
loaded property exists and adjust consumers to read from the chosen location
(imageManifestVuln.loaded or the top-level loaded) to match the code path that
determines loading state.
In
`@frontend/packages/operator-lifecycle-manager/src/components/__tests__/catalog-source.spec.tsx`:
- Around line 177-210: Add a test covering the loadError path by mocking
mockUseK8sWatchResources to return packageManifest with loaded: true (or false,
depending on component logic), data: undefined, and loadError: new Error('Not
found'), render CreateSubscriptionYAML, and assert the component shows the
expected fallback UI (e.g., a "Package not found" message or other error UI your
component renders when packageManifest.loadError is present); target the
existing test block for CreateSubscriptionYAML and use screen.getByText to
verify the error/fallback UI is visible.
In
`@frontend/packages/operator-lifecycle-manager/src/components/catalog-source.tsx`:
- Around line 223-238: The hook call to useK8sWatchResources repeatedly
constructs new URLSearchParams from location.search inside its config object
causing unnecessary allocations; extract const searchParams = new
URLSearchParams(location.search) (or useMemo to memoize it) before calling
useK8sWatchResources and then use searchParams.get('pkg') and
searchParams.get('catalogNamespace') in the packageManifest config so the same
URLSearchParams instance is reused across renders (adjust any references inside
the component accordingly).
- Around line 249-261: The template is creating new URLSearchParams instances
for source and sourceNamespace; reuse the previously extracted searchParams
variable from the outer CreateSubscriptionYAML scope instead. In the Create
function (and specifically the template string in
CreateSubscriptionYAML/Create), replace new
URLSearchParams(location.search).get('catalog') and new
URLSearchParams(location.search).get('catalogNamespace') with
searchParams.get('catalog') and searchParams.get('catalogNamespace')
respectively so you avoid redundant allocations and keep behavior consistent.
- Around line 214-218: Extract and export the inline CreateProps type so it can
be reused and imported elsewhere, and tighten its loadError types from any to
Error; specifically, refactor the CreateSubscriptionYAML component to accept the
exported CreateProps type (use CreateProps for the component props) and change
packageManifest.loadError?: any and operatorGroup.loadError?: any to loadError?:
Error so downstream code/tests get stricter typing.
In
`@frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-subscribe.tsx`:
- Around line 1231-1269: The repeated new
URLSearchParams(window.location.search) calls in OperatorHubSubscribePage make
the useK8sWatchResources call hard to read and risk inconsistent selectors;
parse the query once into a const (e.g., const params = new
URLSearchParams(window.location.search)) at the top of OperatorHubSubscribePage
and replace all inline calls with params.get('catalogNamespace'),
params.get('pkg'), params.get('catalog'), and params.get('targetNamespace') when
building the packageManifest watch config and when passing targetNamespace prop
to OperatorHubSubscribe so the selectors stay consistent and the code is
cleaner.
In
`@frontend/packages/operator-lifecycle-manager/src/components/topology/sidebar/TopologyOperatorBackedResources.tsx`:
- Around line 96-121: resourcesToGet is being recreated each render (via
defaultResources.map), causing watchedResources to be recomputed unnecessarily;
wrap the computation that builds resourcesToGet in a useMemo so it only changes
when its inputs change (e.g., defaultResources or any props used to compute it),
then keep using resourcesToGet in the existing watchedResources useMemo
dependency array so watchedResources only recomputes when the stabilized
resourcesToGet actually changes.
| <ContainerVulnerabilities | ||
| pod={props.obj} | ||
| imageManifestVuln={{ data: imageManifestVuln, loaded, loadError }} | ||
| loaded={loaded} | ||
| /> |
There was a problem hiding this comment.
Redundant loaded prop and unhandled loadError state.
Two concerns here:
-
Redundancy:
loadedis passed both insideimageManifestVuln.loadedand as a separate top-level prop. Consider using only the top-level prop or only the nested one—not both. -
Error handling gap:
loadErroris captured and passed but never consumed byContainerVulnerabilities. If the watch fails,loadedremainsfalseand users see an indefinite loading spinner with no indication of the failure. The component should render an error state whenloadErroris truthy.
Proposed fix to handle error state and remove redundancy
Update ContainerVulnerabilities to handle the error case around line 357:
<Td>
- {props.loaded ? (
+ {props.imageManifestVuln.loadError ? (
+ <span className="text-muted">{t('container-security~Unable to load vulnerability data')}</span>
+ ) : props.loaded ? (
withVuln(And simplify props by removing the redundant top-level loaded:
<ContainerVulnerabilities
pod={props.obj}
imageManifestVuln={{ data: imageManifestVuln, loaded, loadError }}
- loaded={loaded}
/>Then update type and usage to derive loaded from imageManifestVuln.loaded.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/packages/container-security/src/components/image-manifest-vuln.tsx`
around lines 411 - 415, Remove the redundant top-level loaded prop from the
ContainerVulnerabilities call and instead derive loaded from
imageManifestVuln.loaded; also surface and handle the loadError by updating the
ContainerVulnerabilities component (e.g., inside the render path around where it
currently reads imageManifestVuln and loaded) to check
imageManifestVuln.loadError (or the passed loadError) and render an error
state/message instead of an indefinite spinner when truthy, ensuring
types/signature for ContainerVulnerabilities (props) are updated to accept
imageManifestVuln with data/loaded/loadError and no separate loaded prop.
| const resources = useK8sWatchResources<{ | ||
| clusterServiceVersion: ClusterServiceVersionKind; | ||
| subscription: SubscriptionKind; | ||
| installPlans: InstallPlanKind[]; | ||
| }>({ | ||
| clusterServiceVersion: { | ||
| kind: referenceForModel(ClusterServiceVersionModel), | ||
| namespaced: true, | ||
| isList: false, | ||
| name: currentCSV, | ||
| namespace: targetNamespace, | ||
| prop: 'clusterServiceVersion', | ||
| }, | ||
| { | ||
| subscription: { | ||
| kind: referenceForModel(SubscriptionModel), | ||
| namespaced: true, | ||
| isList: false, | ||
| name: pkg, | ||
| namespace: targetNamespace, | ||
| optional: true, | ||
| prop: 'subscription', | ||
| }, | ||
| { | ||
| installPlans: { | ||
| kind: referenceForModel(InstallPlanModel), | ||
| prop: 'installPlans', | ||
| namespaced: true, | ||
| namespace: targetNamespace, | ||
| isList: true, | ||
| optional: true, | ||
| }, | ||
| ]; | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find k8s-watch-hook files
echo "=== Searching for k8s-watch-hook files ==="
find frontend/packages -type f -name "*k8s-watch-hook*" -o -name "*useK8sWatchResources*" 2>/dev/null | head -20Repository: openshift/console
Length of output: 402
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# List files in operator-lifecycle-manager package
echo "=== Files in operator-lifecycle-manager ==="
find frontend/packages/operator-lifecycle-manager/src -type f -name "*.ts*" | grep -E "(hook|watch)" | head -20Repository: openshift/console
Length of output: 1360
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find OperatorInstallStatus component
echo "=== Searching for OperatorInstallStatus ==="
find frontend/packages/operator-lifecycle-manager/src -type f -name "*OperatorInstallStatus*"
# Also search by content
rg -l "OperatorInstallStatus" frontend/packages/operator-lifecycle-manager/src --type tsx --type tsRepository: openshift/console
Length of output: 149
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find all useK8sWatchResources usage for clusterServiceVersion
echo "=== useK8sWatchResources with ClusterServiceVersion ==="
rg -B5 -A15 "clusterServiceVersion:" frontend/packages/operator-lifecycle-manager/src -g "*.tsx" -g "*.ts" | head -100Repository: openshift/console
Length of output: 5484
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the useK8sWatchResources hook implementation
echo "=== useK8sWatchResources implementation ==="
cat -n frontend/packages/console-dynamic-plugin-sdk/src/utils/k8s/hooks/useK8sWatchResources.ts | head -150Repository: openshift/console
Length of output: 6582
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find OperatorInstallStatus
echo "=== Searching for OperatorInstallStatus ==="
rg -l "OperatorInstallStatus" frontend/packages/operator-lifecycle-manager/src --type ts --type tsx
# Search by pattern in operator-install-page.tsx
echo "=== Looking for OperatorInstallStatus in operator-install-page.tsx ==="
rg -n "OperatorInstallStatus" frontend/packages/operator-lifecycle-manager/src/components/operator-install-page.tsxRepository: openshift/console
Length of output: 149
🏁 Script executed:
#!/bin/bash
set -euo pipeftp
# Get the full operator-install-page.tsx to see OperatorInstallStatus and loadError handling
echo "=== Getting line count ==="
wc -l frontend/packages/operator-lifecycle-manager/src/components/operator-install-page.tsxRepository: openshift/console
Length of output: 113
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read more of useK8sWatchResources
cat -n frontend/packages/console-dynamic-plugin-sdk/src/utils/k8s/hooks/useK8sWatchResources.ts | tail -50Repository: openshift/console
Length of output: 2063
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get line count for operator-install-page.tsx
wc -l frontend/packages/operator-lifecycle-manager/src/components/operator-install-page.tsxRepository: openshift/console
Length of output: 150
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for OperatorInstallStatus component definition
rg -n "export.*OperatorInstallStatus|const OperatorInstallStatus" frontend/packages/operator-lifecycle-manager/src --type tsRepository: openshift/console
Length of output: 400
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read OperatorInstallStatus component starting at line 394
sed -n '394,520p' frontend/packages/operator-lifecycle-manager/src/components/operator-install-page.tsx | cat -nRepository: openshift/console
Length of output: 5100
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for loadError handling in the file
rg -n "loadError" frontend/packages/operator-lifecycle-manager/src/components/operator-install-page.tsx -B2 -A2Repository: openshift/console
Length of output: 540
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check k8s-watcher implementation for optional handling
echo "=== Checking k8s-watcher for optional handling ==="
rg -n "optional" frontend/packages/console-dynamic-plugin-sdk/src/utils/k8s/hooks -g "*.ts"Repository: openshift/console
Length of output: 111
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for how optional flag is used in k8s-watcher/related code
rg -n "\.optional" frontend/packages/console-dynamic-plugin-sdk/src/utils/k8s/hooks -B2 -A2 | head -80Repository: openshift/console
Length of output: 43
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check k8s-watcher to see if optional affects 404/loadError behavior
cat -n frontend/packages/console-dynamic-plugin-sdk/src/utils/k8s/hooks/k8s-watcher.ts | head -100Repository: openshift/console
Length of output: 3790
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for documentation or tests about optional behavior
cat -n frontend/packages/console-dynamic-plugin-sdk/src/utils/k8s/hooks/__tests__/useK8sWatchResources.spec.tsx | head -80Repository: openshift/console
Length of output: 3114
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for how optional affects 404/loadError in the actual action/reducer code
find frontend/packages/console-dynamic-plugin-sdk/src/app/k8s/actions -name "*.ts" | head -5Repository: openshift/console
Length of output: 219
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for 404 handling in k8s actions
rg -n "404|optional" frontend/packages/console-dynamic-plugin-sdk/src/app/k8s/actions -g "*.ts" | head -50Repository: openshift/console
Length of output: 43
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for how optional resources with 404 errors are handled in tests
rg -n "optional.*404|404.*optional" frontend/packages --type ts | head -20Repository: openshift/console
Length of output: 43
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for other examples of optional CSV watches to see the pattern
rg -B3 -A3 "ClusterServiceVersion.*optional|optional.*ClusterServiceVersion" frontend/packages/operator-lifecycle-manager --type ts | head -80Repository: openshift/console
Length of output: 43
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the k8s actions to understand optional behavior
head -150 frontend/packages/console-dynamic-plugin-sdk/src/app/k8s/actions/k8s.ts | cat -nRepository: openshift/console
Length of output: 6064
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search where optional is actually used in watch resources
rg "optional.*true" frontend/packages --type ts --type tsx | grep -i "watch\|resource" | head -30Repository: openshift/console
Length of output: 88
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for all CSV references and check if any are marked optional
rg -n "ClusterServiceVersion" frontend/packages/operator-lifecycle-manager/src -B2 -A2 --type ts --type tsx | grep -E "(optional|ClusterServiceVersion)" | head -50Repository: openshift/console
Length of output: 88
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for optional usage patterns in all TypeScript files
rg -n "optional.*true" frontend/packages/operator-lifecycle-manager/src | head -40Repository: openshift/console
Length of output: 778
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if there are any other examples of CSV watches to compare
rg -n "useK8sWatchResource.*ClusterServiceVersion\|clusterServiceVersion.*useK8sWatchResource" frontend/packages --max-count 10Repository: openshift/console
Length of output: 43
🏁 Script executed:
#!/antml/bash
set -euo pipefail
# Look at the complete OperatorInstallStatus component to see the full loading logic
sed -n '394,430p' frontend/packages/operator-lifecycle-manager/src/components/operator-install-page.tsxRepository: openshift/console
Length of output: 1516
🏁 Script executed:
#!/an/bash
set -euo pipefail
# Check if there's any reference to ignoring loadError or handling 404
rg -n "resources\?" frontend/packages/operator-lifecycle-manager/src/components/operator-install-page.tsx | head -20Repository: openshift/console
Length of output: 268
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify: look at how getIDAndDispatch handles optional resources
sed -n '82,110p' frontend/packages/console-dynamic-plugin-sdk/src/utils/k8s/hooks/k8s-watcher.tsRepository: openshift/console
Length of output: 713
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if optional is even used in getIDAndDispatch
rg -n "optional" frontend/packages/console-dynamic-plugin-sdk/src/utils/k8s/hooks/k8s-watcher.tsRepository: openshift/console
Length of output: 43
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for where optional is actually handled in the hooks
rg -n "\.optional" frontend/packages/console-dynamic-plugin-sdk/src/utils/k8s/hooks/useK8sWatchResources.ts -B2 -A2Repository: openshift/console
Length of output: 43
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the complete OperatorInstallStatus to see all the loading logic, specifically how it handles loadError
sed -n '394,516p' frontend/packages/operator-lifecycle-manager/src/components/operator-install-page.tsx | tail -60Repository: openshift/console
Length of output: 2031
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check WatchK8sResource type definition to understand optional
rg -n "interface WatchK8sResource|type WatchK8sResource" frontend/packages/console-dynamic-plugin-sdk/src/extensions/console-types.ts -A15Repository: openshift/console
Length of output: 1177
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if optional is used anywhere in the actual watch mechanism (k8sActions)
rg -n "optional" frontend/packages/console-dynamic-plugin-sdk/src/app/k8s/actions/k8s.tsRepository: openshift/console
Length of output: 43
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get complete picture: Read lines 521-548 of the file under review
sed -n '521,548p' frontend/packages/operator-lifecycle-manager/src/components/operator-install-page.tsxRepository: openshift/console
Length of output: 812
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if loadError is checked anywhere in the full component
sed -n '394,516p' frontend/packages/operator-lifecycle-manager/src/components/operator-install-page.tsx | grep -i "loaderror\|error"Repository: openshift/console
Length of output: 146
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if optional flag actually affects error handling anywhere
rg -n "resource\.optional\|resources\[.*\]\.optional" frontend/packages/console-dynamic-plugin-sdk/src -B3 -A3 | head -40Repository: openshift/console
Length of output: 43
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check where the results from useK8sWatchResources are used in OperatorInstallStatus
# Look for any loadError access on the resources
sed -n '394,516p' frontend/packages/operator-lifecycle-manager/src/components/operator-install-page.tsx | grep -n "loadError\|loaded"Repository: openshift/console
Length of output: 43
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify the exact current state: is CSV marked optional in the code under review?
sed -n '521,548p' frontend/packages/operator-lifecycle-manager/src/components/operator-install-page.tsx | grep -A2 "clusterServiceVersion:"Repository: openshift/console
Length of output: 302
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for where optional flag is actually used in the redux store or watcher logic
rg -n "optional" frontend/packages/console-dynamic-plugin-sdk/src/app/k8s -B2 -A2 | head -80Repository: openshift/console
Length of output: 43
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for any code that checks loadError in similar watch patterns
rg -n "\.loadError" frontend/packages/operator-lifecycle-manager/src/components --type ts | head -40Repository: openshift/console
Length of output: 1248
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if there are tests or examples showing optional behavior with 404
rg -n "optional" frontend/packages/console-dynamic-plugin-sdk/src/utils/k8s/hooks/__tests__ -B3 -A3 | head -60Repository: openshift/console
Length of output: 43
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check for how optional resources with errors are typically handled
# Look for patterns where optional is used with loadError checks
rg -B5 -A5 "optional.*true" frontend/packages/operator-lifecycle-manager/src/components --type ts | grep -B5 -A5 "loadError" | head -80Repository: openshift/console
Length of output: 43
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check useK8sWatchResource (singular) to see if it handles optional
cat -n frontend/packages/console-dynamic-plugin-sdk/src/utils/k8s/hooks/useK8sWatchResource.ts | head -120Repository: openshift/console
Length of output: 3156
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Final check: does OperatorInstallStatus properly handle the case where CSV fails to load?
# Check if there's any defensive logic
sed -n '394,450p' frontend/packages/operator-lifecycle-manager/src/components/operator-install-page.tsxRepository: openshift/console
Length of output: 1920
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Final verification: check if optional flag actually prevents watch dispatch
# Search for how optional is used in getIDAndDispatch or related code
rg -C10 "getIDAndDispatch" frontend/packages/console-dynamic-plugin-sdk/src/utils/k8s/hooks/k8s-watcher.ts | grep -A15 "export const getIDAndDispatch"Repository: openshift/console
Length of output: 350
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if optional flag is checked before dispatching watch
rg -n "if.*optional\|optional.*if" frontend/packages/console-dynamic-plugin-sdk/src/utils/k8s/hooks -B3 -A3Repository: openshift/console
Length of output: 43
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify: in OperatorInstallStatus, does it ever check loadError on CSV?
rg -n "clusterServiceVersion.*loadError\|loadError.*clusterServiceVersion" frontend/packages/operator-lifecycle-manager/src/components/operator-install-page.tsxRepository: openshift/console
Length of output: 43
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Final verification: is the optional field actually USED anywhere?
rg -n "\.optional\b" frontend/packages/console-dynamic-plugin-sdk/src/utils/k8s/hooks -A2 -B2 | head -100Repository: openshift/console
Length of output: 43
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if optional is documented or used in the action creators
rg -n "optional" frontend/packages/console-dynamic-plugin-sdk/src/app/k8s/actions/k8s.tsRepository: openshift/console
Length of output: 43
🏁 Script executed:
#!/antml/bash
set -euo pipefail
# Summary check: does OperatorInstallStatus access loadError at all?
grep -n "\.loadError\|loadError" frontend/packages/operator-lifecycle-manager/src/components/operator-install-page.tsxRepository: openshift/console
Length of output: 290
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Final verification: exhaustive search for where optional is actually checked
rg "resource\.optional\|r\.optional\|\[.*\]\.optional" frontend/packages/console-dynamic-plugin-sdk -nRepository: openshift/console
Length of output: 43
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if optional is used in the k8s resource module or action creators
rg "optional" frontend/packages/console-dynamic-plugin-sdk/src/utils/k8s/k8s-resource.ts -nRepository: openshift/console
Length of output: 43
Mark CSV watch optional and add loadError handling to prevent stuck loading states.
When ClusterServiceVersion doesn't exist early in the install (common scenario), a non-optional watch returns a 404 loadError. However, OperatorInstallStatus ignores loadError on all resources—it only checks if data exists. This can leave the page stuck showing "Installing..." indefinitely. Mark the CSV watch optional for consistency with subscription and installPlans, and add explicit loadError checks similar to other OLM components (e.g., operator-hub-page.tsx).
✅ Minimal changes needed
1. Mark CSV optional:
clusterServiceVersion: {
kind: referenceForModel(ClusterServiceVersionModel),
namespaced: true,
isList: false,
name: currentCSV,
namespace: targetNamespace,
+ optional: true,
},2. Add loadError handling in OperatorInstallStatus (check similar to operator-hub-page.tsx:311):
const OperatorInstallStatus: FC<OperatorInstallPageProps> = ({ resources }) => {
+ const loadError = Object.values(resources || {}).find((r) => r?.loadError)?.loadError;
+ if (loadError) {
+ return <Alert variant="danger" title="Installation Error">{loadError.toString()}</Alert>;
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@frontend/packages/operator-lifecycle-manager/src/components/operator-install-page.tsx`
around lines 521 - 548, Mark the clusterServiceVersion watch as optional in the
useK8sWatchResources call (set optional: true for the clusterServiceVersion
entry alongside subscription and installPlans) and update OperatorInstallStatus
to check for resource load errors before relying on presence of data: inspect
resources.clusterServiceVersion.loadError (and other resources' loadError) and
surface a failure/stop the "Installing..." state when a 404 or other loadError
exists—mirror the loadError handling pattern used in operator-hub-page.tsx for
robust early-not-found handling.
| const [secrets, loaded, loadError] = useK8sWatchResource<SecretKind[]>({ | ||
| isList: true, | ||
| kind: SecretModel.kind, | ||
| namespace: props.namespace, | ||
| optional: true, | ||
| }); | ||
|
|
||
| const resources = { | ||
| [SecretModel.id]: { | ||
| data: secrets, | ||
| loaded, | ||
| loadError, | ||
| }, | ||
| ]; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find ResourceDropdown
echo "=== Finding ResourceDropdown ==="
fd -t f "ResourceDropdown" --extension tsx --extension ts 2>/dev/null | head -20
echo ""
echo "=== Finding by pattern ==="
find . -name "*ResourceDropdown*" -type f 2>/dev/null | head -20Repository: openshift/console
Length of output: 645
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for ResourceDropdown usage in packages/shipwright-plugin
echo "=== ResourceDropdown in shipwright-plugin ==="
rg "ResourceDropdown" frontend/packages/shipwright-plugin -l 2>/dev/null | head -10
echo ""
echo "=== PushSecretDropdown imports and usage ==="
rg "import.*ResourceDropdown" frontend/packages/shipwright-plugin -A 2 -B 2 2>/dev/null | head -30Repository: openshift/console
Length of output: 1678
🏁 Script executed:
#!/bin/bash
set -euoripefail
# Examine PushSecretDropdown to see full context of ResourceDropdown usage
echo "=== PushSecretDropdown.tsx full context ==="
wc -l frontend/packages/shipwright-plugin/src/components/build-form/PushSecretDropdown.tsx
echo ""
echo "=== ResourceDropdown invocation ==="
rg "ResourceDropdown" frontend/packages/shipwright-plugin/src/components/build-form/PushSecretDropdown.tsx -B 5 -A 15Repository: openshift/console
Length of output: 735
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find ResourceDropdown in console codebase
echo "=== Searching for ResourceDropdown in console ==="
find . -path ./node_modules -prune -o -name "*ResourceDropdown*" -type f -print 2>/dev/null | head -20
echo ""
echo "=== Searching by pattern for ResourceDropdown ==="
rg "export.*ResourceDropdown|class ResourceDropdown|function ResourceDropdown|const ResourceDropdown" -t tsx -t ts 2>/dev/null | head -20Repository: openshift/console
Length of output: 426
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read ResourceDropdown implementation
echo "=== ResourceDropdown.tsx ==="
wc -l frontend/packages/console-shared/src/components/dropdown/ResourceDropdown.tsx
head -150 frontend/packages/console-shared/src/components/dropdown/ResourceDropdown.tsx
echo ""
echo "=== Bottom of ResourceDropdown.tsx ==="
tail -50 frontend/packages/console-shared/src/components/dropdown/ResourceDropdown.tsxRepository: openshift/console
Length of output: 6829
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check how SecretDropdown handles resources
echo "=== SecretDropdown.tsx ==="
cat -n frontend/packages/shipwright-plugin/src/components/build-form/SecretDropdown.tsx
echo ""
echo "=== ConfigMapDropdown.tsx ==="
cat -n frontend/packages/shipwright-plugin/src/components/build-form/ConfigMapDropdown.tsxRepository: openshift/console
Length of output: 3619
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check ResourceDropdown props and how it accesses resources
echo "=== ResourceDropdown component definition ==="
rg -n "interface.*ResourceDropdownProps|type.*ResourceDropdownProps|const ResourceDropdown|function ResourceDropdown" \
frontend/packages/console-shared/src/components/dropdown/ResourceDropdown.tsx -A 20
echo ""
echo "=== How ResourceDropdown accesses resources data ==="
rg -n "resources\[|resources\." frontend/packages/console-shared/src/components/dropdown/ResourceDropdown.tsx -B 2 -A 2Repository: openshift/console
Length of output: 1340
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the full PushSecretDropdown to see ResourceDropdown invocation
echo "=== Full PushSecretDropdown.tsx ==="
cat -n frontend/packages/shipwright-plugin/src/components/build-form/PushSecretDropdown.tsxRepository: openshift/console
Length of output: 2203
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find getResourceList method to confirm how it iterates
echo "=== getResourceList method ==="
rg -n "getResourceList" frontend/packages/console-shared/src/components/dropdown/ResourceDropdown.tsx -A 25 | head -50
echo ""
echo "=== getDropdownList method ==="
rg -n "getDropdownList" frontend/packages/console-shared/src/components/dropdown/ResourceDropdown.tsx -A 20 | head -40Repository: openshift/console
Length of output: 3233
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify the type structure FirehoseResult
echo "=== FirehoseResult type definition ==="
rg -n "type FirehoseResult|interface FirehoseResult" --type ts --type tsx -A 15
echo ""
echo "=== Confirm what ResourceDropdown.resources expects ==="
rg -n "this\.props\.resources" frontend/packages/console-shared/src/components/dropdown/ResourceDropdown.tsx -B 2 -A 2Repository: openshift/console
Length of output: 144
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get full getResourceList and getDropdownList methods
echo "=== Full getResourceList and getDropdownList ==="
sed -n '176,250p' frontend/packages/console-shared/src/components/dropdown/ResourceDropdown.tsx
echo ""
echo "=== FirehoseResult type from types file ==="
fd -t f "types.ts" "types.tsx" | xargs grep -l "FirehoseResult" | head -1 | xargs cat -n | grep -A 10 "FirehoseResult"Repository: openshift/console
Length of output: 2450
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for FirehoseResult in internal
echo "=== FirehoseResult definition ==="
rg "type FirehoseResult|interface FirehoseResult" -A 10
echo ""
echo "=== Check how resources array is iterated ==="
sed -n '176,220p' frontend/packages/console-shared/src/components/dropdown/ResourceDropdown.tsxRepository: openshift/console
Length of output: 4153
Fix critical structural bug: resources must be an array, not an object literal.
PushSecretDropdown passes resources as a keyed object (lines 38–44), but ResourceDropdown expects resources: FirehoseResult[] — an array. The getDropdownList method iterates via _.each(resources, ({ data, kind }) => ...), which will fail to destructure an object literal. Follow the pattern from ConfigMapDropdown: wrap the resource state in an array and include the kind field:
Corrected pattern
const [secrets, loaded, loadError] = useK8sWatchResource<SecretKind[]>({
isList: true,
kind: SecretModel.kind,
namespace: props.namespace,
optional: true,
});
- const resources = {
- [SecretModel.id]: {
- data: secrets,
- loaded,
- loadError,
- },
- };
+ const resources = [
+ {
+ data: secrets ?? [],
+ loaded,
+ loadError,
+ kind: SecretModel.kind,
+ },
+ ];
return (
<ResourceDropdown
{...props}
resources={resources}Defensively guard secrets with a fallback (it may be undefined before loaded completes) and include kind for proper badge rendering and resource metadata.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@frontend/packages/shipwright-plugin/src/components/build-form/PushSecretDropdown.tsx`
around lines 31 - 44, The resources variable in PushSecretDropdown is
incorrectly built as an object literal but ResourceDropdown expects an array of
FirehoseResult items; change the resources value to an array containing an
object with keys data, loaded, loadError, and kind (use SecretModel.kind) and
ensure data uses a safe fallback (e.g., secrets || []) so secrets being
undefined before loaded doesn't break getDropdownList's destructuring; update
the resources assignment where useK8sWatchResource returns [secrets, loaded,
loadError] and pass that array into ResourceDropdown.
| const watchedBaseResources = useMemo(() => getBaseWatchedResources(namespace), [namespace]); | ||
|
|
||
| const resources = useK8sWatchResources(watchedBaseResources); | ||
|
|
||
| return ( | ||
| <Firehose resources={resources}> | ||
| <ResourceDropdown | ||
| {...props} | ||
| placeholder={t('topology~Select an application')} | ||
| dataSelector={['metadata', 'labels', 'app.kubernetes.io/part-of']} | ||
| /> | ||
| </Firehose> | ||
| <ResourceDropdown | ||
| {...props} | ||
| resources={resources} | ||
| placeholder={t('topology~Select an application')} | ||
| dataSelector={['metadata', 'labels', 'app.kubernetes.io/part-of']} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find ApplicationDropdown component definition and props interface
fd -t f "ApplicationDropdown.tsx" frontend/packages/topology/src/Repository: openshift/console
Length of output: 136
🏁 Script executed:
# Check the props interface for ApplicationDropdown
rg -n "ApplicationDropdownProps" frontend/packages/topology/src/components/dropdowns/ -A 10Repository: openshift/console
Length of output: 1729
🏁 Script executed:
# Find getBaseWatchedResources implementation
fd -t f -name "*.ts" -o -name "*.tsx" frontend/packages/topology/src | xargs rg -l "getBaseWatchedResources" | head -5Repository: openshift/console
Length of output: 569
🏁 Script executed:
# Find all ApplicationDropdown usages across the codebase
rg -n "ApplicationDropdown" frontend/ --type ts --type tsx -B 2 -A 5Repository: openshift/console
Length of output: 88
🏁 Script executed:
# Read getBaseWatchedResources implementation
rg -n "getBaseWatchedResources" frontend/packages/topology/src/data-transforms/transform-utils.ts -A 15Repository: openshift/console
Length of output: 495
🏁 Script executed:
# Find all ApplicationDropdown usages (fix file type argument)
rg -n "ApplicationDropdown" frontend/ -t ts -C 3 | head -100Repository: openshift/console
Length of output: 11843
🏁 Script executed:
# Also check if ApplicationDropdown is exported and used in other packages
rg -n "<ApplicationDropdown" frontend/ --type ts --type jsRepository: openshift/console
Length of output: 455
🏁 Script executed:
# Read NamespaceBarApplicationSelector to see namespace prop
cat -n frontend/packages/topology/src/components/dropdowns/NamespaceBarApplicationSelector.tsx | head -70Repository: openshift/console
Length of output: 2736
🏁 Script executed:
# Read ApplicationSelector to see namespace handling
sed -n '19,110p' frontend/packages/topology/src/components/dropdowns/ApplicationSelector.tsx | cat -nRepository: openshift/console
Length of output: 3951
🏁 Script executed:
# Check what useK8sWatchResources does with undefined namespace in watch config
rg -n "useK8sWatchResources" frontend/packages/console-shared/src/hooks/ -A 20 | head -50Repository: openshift/console
Length of output: 5395
Guard against undefined namespace in watch configuration.
ApplicationDropdownProps.namespace is optional, but getBaseWatchedResources expects a string and uses it directly in the K8s watch config. ApplicationSelector can pass undefined, creating invalid resource watches. NamespaceBarApplicationSelector is safe (Redux-sourced string). Add a guard or enforce namespace as required in ApplicationDropdown to prevent broken watch state.
🔧 Suggested guard
- const watchedBaseResources = useMemo(() => getBaseWatchedResources(namespace), [namespace]);
+ const watchedBaseResources = useMemo(
+ () => (namespace ? getBaseWatchedResources(namespace) : {}),
+ [namespace],
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const watchedBaseResources = useMemo(() => getBaseWatchedResources(namespace), [namespace]); | |
| const resources = useK8sWatchResources(watchedBaseResources); | |
| return ( | |
| <Firehose resources={resources}> | |
| <ResourceDropdown | |
| {...props} | |
| placeholder={t('topology~Select an application')} | |
| dataSelector={['metadata', 'labels', 'app.kubernetes.io/part-of']} | |
| /> | |
| </Firehose> | |
| <ResourceDropdown | |
| {...props} | |
| resources={resources} | |
| placeholder={t('topology~Select an application')} | |
| dataSelector={['metadata', 'labels', 'app.kubernetes.io/part-of']} | |
| const watchedBaseResources = useMemo( | |
| () => (namespace ? getBaseWatchedResources(namespace) : {}), | |
| [namespace], | |
| ); | |
| const resources = useK8sWatchResources(watchedBaseResources); | |
| return ( | |
| <ResourceDropdown | |
| {...props} | |
| resources={resources} | |
| placeholder={t('topology~Select an application')} | |
| dataSelector={['metadata', 'labels', 'app.kubernetes.io/part-of']} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/packages/topology/src/components/dropdowns/ApplicationDropdown.tsx`
around lines 16 - 25, ApplicationDropdown currently calls
getBaseWatchedResources(namespace) with an optional
ApplicationDropdownProps.namespace which can be undefined, producing invalid
watch configs; update ApplicationDropdown to guard the namespace before building
watches — e.g., check ApplicationDropdownProps.namespace and if undefined pass a
safe fallback (empty string) or short-circuit and return empty resources to use
in useK8sWatchResources, so getBaseWatchedResources and useK8sWatchResources
always receive a valid string or a no-op resource set; ensure you update the
call sites using getBaseWatchedResources and the resources prop passed into
ResourceDropdown accordingly.
openshift-merge-robot
added
the
needs-rebase
cajieh
force-pushed
the
refactor-remaining-firehose-impacted-components
branch
from
a37006c to
b3b8032
Compare
openshift-merge-robot
removed
the
needs-rebase
cajieh
force-pushed
the
refactor-remaining-firehose-impacted-components
branch
from
b3b8032 to
ce609af
Compare
|
/hold Until type issues due rebased are resolved. |
openshift-ci
bot
added
the
do-not-merge/hold
cajieh
force-pushed
the
refactor-remaining-firehose-impacted-components
branch
from
ce609af to
4ab4b82
Compare
|
/cancel hold |
|
/unhold |
openshift-ci
bot
removed
the
do-not-merge/hold
|
/retest |
cajieh
force-pushed
the
refactor-remaining-firehose-impacted-components
branch
from
4ab4b82 to
0d97c01
Compare
| obj?: FirehoseResult<PodKind>; | ||
| pod?: PodKind; | ||
| loaded: boolean; | ||
| loadError?: any; |
There was a problem hiding this comment.
better to use unknown instead of any
There was a problem hiding this comment.
See my take on this PR.
Let me know if we need to discuss this further.
There was a problem hiding this comment.
Despite loadError being typed any it's still more semantically correct to type this as unknown so that we do not accidentally access properties which aren't present in the object in runtime
| imageManifestVuln: { | ||
| data: ImageManifestVuln[]; | ||
| loaded: boolean; | ||
| loadError?: any; |
There was a problem hiding this comment.
better to use unknown instead of any
There was a problem hiding this comment.
As stated previously.
| resources?: { | ||
| [kind: string]: { data: K8sResourceKind[] }; | ||
| resources: { | ||
| [kind: string]: { data: K8sResourceKind[]; loaded: boolean; loadError?: any }; |
There was a problem hiding this comment.
better to use unknown instead of any
There was a problem hiding this comment.
As stated previously.
| packageManifest: { loaded: boolean; data?: PackageManifestKind; loadError?: any }; | ||
| operatorGroup: { loaded: boolean; data?: OperatorGroupKind[]; loadError?: any }; |
There was a problem hiding this comment.
better to use unknown instead of any
There was a problem hiding this comment.
As stated previously.
| catalogSources?: { data?: CatalogSourceKind[]; loaded?: boolean; loadError?: any }; | ||
| packageManifests?: { data?: PackageManifestKind[]; loaded?: boolean; loadError?: any }; |
There was a problem hiding this comment.
better to use unknown instead of any
There was a problem hiding this comment.
As stated previously.
| {pod.status?.containerStatuses?.[0]?.state?.terminated?.message || | ||
| pod.status?.message} |
There was a problem hiding this comment.
pod is an optional prop per NodeTerminalInnerProps but we're assuming it always exists as there's no optional chanining for accessing pod.status.
either make pod a required prop or add optional chaining
| namespace: new URLSearchParams(window.location.search).get('catalogNamespace'), | ||
| fieldSelector: `metadata.name=${new URLSearchParams(window.location.search).get('pkg')}`, | ||
| selector: { | ||
| matchLabels: { | ||
| catalog: new URLSearchParams(window.location.search).get('catalog'), |
There was a problem hiding this comment.
As stated previously.
| namespace={activeNamespace} | ||
| loaded={loaded} | ||
| loadError={loadError} | ||
| targetNamespace={new URLSearchParams(window.location.search).get('targetNamespace') || null} |
There was a problem hiding this comment.
As stated previously.
| }) => { | ||
| const { t } = useTranslation(); | ||
| const loaded = Object.values(resources).every((r) => r.loaded); | ||
| const loadError = Object.values(resources).find((r) => r.loadError)?.loadError; |
There was a problem hiding this comment.
so if there are multiple loadErrors, we will only show the first one? is this fine from a UX perspective, or maybe we should be showing every error?
There was a problem hiding this comment.
I considered this as well, but I felt it would be more flexible to display one error at a time rather than aggregating them. Furthermore, components like StatusBox are designed to show only a single error. In most cases, when multiple resources fail to load, they share a common root cause such as network issues or RBAC permission, meaning resolving the first error often addresses the others.
Let’s discuss this further in today’s scrum if we want to reconsider how multiple errors are handled.
| name: new URLSearchParams(location.search).get('pkg'), | ||
| namespace: new URLSearchParams(location.search).get('catalogNamespace'), |
There was a problem hiding this comment.
Not new, and appears to be out of scope for this work. We can open a ticket if this cleanup is required.
cajieh
force-pushed
the
refactor-remaining-firehose-impacted-components
branch
from
0d97c01 to
fdd505b
Compare
cajieh
force-pushed
the
refactor-remaining-firehose-impacted-components
branch
from
fdd505b to
b22e4eb
Compare
3 participants
✅ CONSOLE-5030 Complete: All Firehose Components Migrated to Hooks
Successfully Refactored (9 files)
1. OLM Package (6 files)
2. Console-app Package (1 file)
3. Container-security Package (1 file)
4. Topology Package (1 file)
5. Shipwright-plugin Package (1 file)
Migration Approach
Summary by CodeRabbit
Release Notes