CONSOLE-5027: Refactor public/ Directory Components to useK8sWatchResource(s)#15954
CONSOLE-5027: Refactor public/ Directory Components to useK8sWatchResource(s)#15954cajieh wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@cajieh: This pull request references CONSOLE-5027 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. 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
openshift-merge-robot
added
the
needs-rebase
📝 WalkthroughWalkthroughThis pull request systematically replaces Firehose-based data provisioning with direct Kubernetes watch hooks across the console frontend. Components are refactored to use 🚥 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)
Comment |
|
[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/monitoring
cajieh
force-pushed
the
migrate-firehose-to-hooks
branch
from
ed33b09 to
7b7d0f8
Compare
openshift-merge-robot
removed
the
needs-rebase
cajieh
force-pushed
the
migrate-firehose-to-hooks
branch
from
7b7d0f8 to
5f6e3ed
Compare
|
@cajieh: This pull request references CONSOLE-5027 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. 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. |
cajieh
force-pushed
the
migrate-firehose-to-hooks
branch
4 times, most recently
from
7dca8dc to
b879d3d
Compare
| const resources = flags[FLAGS.CONSOLE_YAML_SAMPLE] | ||
| ? [ | ||
| { | ||
| const watchResources = flags[FLAGS.CONSOLE_YAML_SAMPLE] |
There was a problem hiding this comment.
for EditYAML we can actually simplify this even further by using useFlag and moving this into the main EditYAML component. Then we do not need EditYAML_ and WithFlagProps and all that other stuff
cajieh
force-pushed
the
migrate-firehose-to-hooks
branch
from
b879d3d to
7db7e98
Compare
| isList: false, | ||
| prop: 'obj', | ||
| }; | ||
| const objResource = useMemo<FirehoseResource>( |
There was a problem hiding this comment.
Deferring to the Firehose Type migration effort.
cajieh
force-pushed
the
migrate-firehose-to-hooks
branch
from
7db7e98 to
8d8177e
Compare
openshift-merge-robot
added
the
needs-rebase
cajieh
force-pushed
the
migrate-firehose-to-hooks
branch
from
8d8177e to
935611e
Compare
openshift-merge-robot
removed
the
needs-rebase
cajieh
force-pushed
the
migrate-firehose-to-hooks
branch
2 times, most recently
from
9abced3 to
c81bc77
Compare
cajieh
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
openshift-ci
bot
added
the
kind/i18n
|
@cajieh: This pull request references CONSOLE-5027 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. 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. |
|
@cajieh: This pull request references CONSOLE-5027 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. 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. |
cajieh
force-pushed
the
migrate-firehose-to-hooks
branch
from
c81bc77 to
447e0e4
Compare
|
@cajieh: This pull request references CONSOLE-5027 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. 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: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
frontend/public/components/monitoring/receiver-forms/alert-manager-receiver-forms.tsx (1)
528-574:⚠️ Potential issue | 🟠 MajorMerge Secret watch and globals load state before rendering.
StatusBoxcurrently uses only the Alertmanager globals fetch state, which can render the form while the Secret is still loading or hide Secret watch errors. This can lead to incorrect forms or missing error UI.💡 Suggested fix (combine loaded/loadError)
const ReceiverWrapper: FC<ReceiverFormsWrapperProps> = memo(({ obj, ...props }) => { const { alertManagerBaseURL } = window.SERVER_FLAGS; const [alertmanagerGlobals, setAlertmanagerGlobals] = useState(); const [loaded, setLoaded] = useState(false); const [loadError, setLoadError] = useState<APIError>(); @@ const { t } = useTranslation(); + const combinedLoaded = loaded && (obj?.loaded ?? true); + const combinedLoadError = loadError ?? obj?.loadError; return ( <StatusBox {...obj} label={t('public~Alertmanager globals')} - loaded={loaded} - loadError={loadError} + loaded={combinedLoaded} + loadError={combinedLoadError} > <ReceiverBaseForm {...props} obj={obj.data} alertmanagerGlobals={alertmanagerGlobals} /> </StatusBox> ); });frontend/public/components/console-notifier.tsx (1)
67-67:⚠️ Potential issue | 🟡 MinorInternationalize the 'More info' fallback text.
The hardcoded
'More info'string should be translated using thetfunction fromuseTranslation. Given this PR is labeledkind/i18n, this is especially relevant.🌐 Suggested i18n fix
Add at the top of the component:
import { useTranslation } from 'react-i18next';Inside the component:
export const ConsoleNotifier: FC<ConsoleNotifierProps> = ({ location }) => { + const { t } = useTranslation(); const shouldFetch = useFlag(FLAGS.CONSOLE_NOTIFICATION);Then update the fallback:
- {notification.spec.link.text || 'More info'} + {notification.spec.link.text || t('public~More info')}frontend/public/components/RBAC/bindings.tsx (1)
365-386:⚠️ Potential issue | 🟠 MajorDon’t mask watch errors across resources.
If
ClusterRoleBindingfails,loadedcan becometruewhileloadErrorstaysnull, yielding a silently incomplete list. Aggregate load errors across both resources and treat errors as terminal for loading.Proposed fix
const data = useMemo(() => flatten(resources), [resources]); - const loaded = useMemo( - () => - Object.values(resources) - .filter((r) => !r.loadError) - .every((r) => r.loaded), - [resources], - ); + const loadError = resources.RoleBinding.loadError || resources.ClusterRoleBinding.loadError; + const loaded = useMemo( + () => Object.values(resources).every((r) => r.loaded || r.loadError), + [resources], + ); @@ <BindingsList data={data} loaded={loaded} - loadError={resources.RoleBinding.loadError} + loadError={loadError} staticFilters={staticFilters} />frontend/public/components/command-line-tools.tsx (1)
29-38:⚠️ Potential issue | 🟠 MajorMissing loading and error state handling.
The component receives
loadedandloadErrorviaobjbut never uses them. This will result in:
- No loading indicator while data is being fetched
- Silent failures when the watch encounters an error
- Potentially confusing empty state during initial load
Other components in this PR likely handle these states—this one should too for consistent UX.
🛠️ Proposed fix to handle loading and error states
export const CommandLineTools: FC<CommandLineToolsProps> = ({ obj }) => { const { t } = useTranslation(); const [requestTokenURL, externalLoginCommand] = useCopyLoginCommands(); const launchCopyLoginCommandModal = useCopyCodeModal( t('public~Login with this command'), externalLoginCommand, ); const showCopyLoginCommand = requestTokenURL || externalLoginCommand; + + if (!obj.loaded && !obj.loadError) { + return <LoadingBox />; + } + + if (obj.loadError) { + return ( + <> + <DocumentTitle>{t('public~Command Line Tools')}</DocumentTitle> + <PageHeading title={t('public~Command Line Tools')} /> + <PaneBody> + <StatusBox loadError={obj.loadError} label={t('public~Command Line Tools')} /> + </PaneBody> + </> + ); + } + const data = _.sortBy(obj.data, 'spec.displayName');You'll need to import the appropriate loading/error components:
import { LoadingBox } from './utils'; import { StatusBox } from './utils/status-box';frontend/packages/console-app/src/components/nodes/NodeTerminal.tsx (1)
129-165:⚠️ Potential issue | 🟡 MinorGuard against perpetual loading when a watched pod is deleted.
The Redux reducer lacks a handler for
DELETEDevents on single object watches. When a pod is deleted after being loaded successfully, the watch stream receives a deletion event, but the reducer has no corresponding case (onlyUpdateListFromWShandles list deletions). The state can remain withloaded=trueanddata=undefined, causing the component to render an endlessLoadingBoxdespite the pod being gone.Split the condition to explicitly handle the "loaded but no pod" case as an error:
Suggested guard
- if (!loaded || !pod) { - return <LoadingBox />; - } + if (!loaded) { + return <LoadingBox />; + } + if (!pod) { + return <NodeTerminalError error={t('console-app~Failed to load pod')} />; + }frontend/public/components/factory/list-page.tsx (1)
517-570:⚠️ Potential issue | 🟠 MajorList components lose
loaded/loadErrorafter Firehose removal.With only
watchedResourcespassed down,ListComponentno longer receives aggregatedloaded/loadError, so lists can appear stuck loading or miss error rendering. Consider deriving these fromresourcesinsideListPageWrapperand passing them through.🛠️ Proposed fix: aggregate state in ListPageWrapper
- const data = flatten ? flatten(resources) : []; + const data = flatten ? flatten(resources) : []; + const loaded = + resources && Object.values(resources).every((r: any) => r?.loaded); + const loadError = + resources && Object.values(resources).find((r: any) => r?.loadError)?.loadError; ... - <ListComponent {...props} data={data} /> + <ListComponent {...props} data={data} loaded={loaded} loadError={loadError} />
🤖 Fix all issues with AI agents
In `@frontend/packages/console-app/src/actions/hooks/useBindingActions.ts`:
- Line 48: In useBindingActions, referenceFor(obj) is called before the
defensive destructure "const { subjectIndex, subjects } = obj ?? {}", which
throws when obj is nullish; add an early null/undefined guard at the top of
useBindingActions (e.g., if (!obj) return [] or return the empty actions array
used for loading) so no further work runs on a missing obj, then you can safely
destructure subjectIndex/subjects from obj without the "?? {}" fallback; ensure
the guard uses the same return shape the hook expects and reference the
functions/variables referenceFor, subjectIndex, subjects, and useBindingActions
when making the change.
- Line 52: The code now uses optional chaining for model?.kind in the label but
still accesses model.kind elsewhere (at the other uses in this file), which will
throw if model is undefined; update all occurrences of direct model.kind access
in useBindingActions.ts to use optional chaining (model?.kind) or,
alternatively, add an early guard like if (!model) return (or throw) at the top
of the hook/function so the rest can safely assume model is defined—apply this
consistently to every place that references model.kind in the file.
In `@frontend/public/components/command-line-tools.tsx`:
- Around line 93-105: When FLAGS.CONSOLE_CLI_DOWNLOAD is false
useK8sWatchResource returns undefined for cliDownloads and the unsafe cast in
CommandLineToolsPage causes a runtime crash; update CommandLineToolsPage to
guard or normalize cliDownloads before passing to CommandLineTools (e.g., if
shouldFetch is false render null/alternate UI or pass obj={{ data: cliDownloads
?? [] , loaded, loadError }}), referencing the
useFlag(FLAGS.CONSOLE_CLI_DOWNLOAD) and useK8sWatchResource(...) call and the
CommandLineTools prop to locate and fix the issue.
In `@frontend/public/components/console-notifier.tsx`:
- Around line 30-41: The component currently only destructures the first element
from useK8sWatchResource, so loading and error states are ignored; update the
call to destructure [notifications, loaded, loadError] from useK8sWatchResource
(the call that uses referenceForModel(ConsoleNotificationModel) and shouldFetch)
and add a simple handling: if loadError is present, call a debug/error logger
with context (e.g., "ConsoleNotification loadError") so errors are visible, and
optionally handle loaded === false (e.g., skip rendering or show a non-blocking
loading state) before using notifications.
In `@frontend/public/components/container.tsx`:
- Line 518: The component is passing obj={{ data: props.obj.data, loaded:
props.loaded }} to ContainerDetailsList but that wrapper shape breaks
ContainerDetailsList which expects a PodKind (so pod.spec is accessed); change
the prop to pass the actual PodKind instead (e.g., obj={props.obj.data}) and, if
loading state is needed by the child, pass loaded via a separate prop (e.g.,
loaded={props.loaded}) so ContainerDetailsList receives a PodKind with a defined
spec.
- Around line 551-553: The component currently dereferences props.obj.data
without checking props.loadError or null/undefined, which can crash when a watch
fails; update the rendering logic in container.tsx to first check
props.loadError and handle/return early (e.g., render an error state) and then
ensure props.obj and props.obj.data (the PodKind) are non-null before calling
getContainer(pod, …) or accessing pod properties; specifically modify the code
paths around props.obj, props.obj.data, getContainer, and any use of PodKind so
they guard for loadError and null values and avoid dereferencing when absent.
In `@frontend/public/components/cron-job.tsx`:
- Around line 218-272: Compute and pass explicit loaded/loadError flags from the
watches into ListPageWrapper: derive const loaded = resources.jobs.loaded &&
resources.pods.loaded and const loadError = resources.jobs.loadError ||
resources.pods.loadError (using the existing resources object returned by
useK8sWatchResources) and pass them as props (loaded={loaded}
loadError={loadError}) to ListPageWrapper in CronJobPodsComponent; apply the
same change to the sibling component that renders jobs (CronJobJobsComponent) so
both PodList and JobsList receive accurate loading/error state.
In `@frontend/public/components/edit-yaml.tsx`:
- Around line 159-174: The watched resource yamlSamplesList returned from
useK8sWatchResources must be validated for loaded and loadError before passing
to useResourceSidebarSamples; update the component to derive a safe value (e.g.,
yamlSamplesListSafe) from yamlSamplesList by checking yamlSamplesList?.loaded &&
!yamlSamplesList?.loadError and if that check fails return an object with an
empty data array and neutral load flags (e.g., { data: [], loaded: true,
loadError: null }); then pass yamlSamplesListSafe to useResourceSidebarSamples
so the sidebar never consumes incomplete or errored watch data (references:
useK8sWatchResources, yamlSamplesList, useResourceSidebarSamples,
hasYAMLSampleFlag, ConsoleYAMLSampleModel).
- Around line 161-168: The hook useK8sWatchResources must not assume all entries
are non-null; update useK8sWatchResources to filter out null/undefined resource
entries before iterating or accessing properties (e.g., call
Object.values(resources).filter(r => r != null) or similar) and add defensive
checks before accessing .groupVersionKind and .kind so it never reads properties
from null; ensure code paths that currently reference .groupVersionKind and
.kind handle missing resources gracefully (affecting calls from edit-yaml.tsx
where watchResources.yamlSamplesList can be null).
In `@frontend/public/components/instantiate-template.tsx`:
- Around line 394-403: The hook useK8sWatchResource is being called even when
searchParams.get('template') or searchParams.get('template-ns') are null,
causing it to return [undefined, true, undefined] and downstream runtime errors
when TemplateForm accesses template.parameters and obj.data.metadata.*; fix by
building the watch spec conditionally—only call useK8sWatchResource with a valid
spec when both templateName and templateNamespace are non-null, otherwise pass
null (or skip the watch) so that template stays undefined and existing guards in
TemplateForm can handle the missing template; update the code around
templateName, templateNamespace and the useK8sWatchResource invocation to use
this conditional spec.
In `@frontend/public/components/namespace-bar.tsx`:
- Around line 31-35: The NamespaceBarDropdowns component dereferences
namespace.loaded and namespace.data without null checks but
NamespaceBarDropdownsProps currently marks namespace as optional; make namespace
required by changing NamespaceBarDropdownsProps so namespace:
WatchK8sResultsObject<K8sResourceCommon[]> (remove the ?), and update any call
sites if needed (e.g., NamespaceBar) to always pass a valid
WatchK8sResultsObject; this aligns the type with how NamespaceBarDropdowns uses
namespace and prevents future null/undefined crashes.
In `@frontend/public/components/RBAC/bindings.tsx`:
- Around line 790-815: BindingLoadingWrapper uses a concise arrow in
fixedKeys.forEach that returns the assignment value which Biome flags; change
the callback to a block body so it does not return a value (e.g., replace the
concise arrow in the forEach on fixedKeys with a block callback that assigns
fixed[k] = obj?.[k];) to eliminate the returned value while keeping the same
behavior in BindingLoadingWrapper and the fixed variable population.
In `@frontend/public/components/storage-class-form.tsx`:
- Around line 784-804: StorageClassForm is passing sc and csi as top-level props
to ConnectedStorageClassForm but StorageClassFormInner (used by
ConnectedStorageClassForm) expects a resources object (reads props.resources?.sc
and props.resources?.csi), so wrap the existing sc/csi props into a resources
object when calling ConnectedStorageClassForm; in other words, change the call
site in StorageClassForm to pass resources={{ sc: { data: resources.sc.data,
loaded: resources.sc.loaded, loadError: resources.sc.loadError }, csi: { data:
resources.csi.data, loaded: resources.csi.loaded, loadError:
resources.csi.loadError } }} so StorageClassFormInner’s validation/uniqueness
checks see the watched data.
In `@frontend/public/components/utils/list-dropdown.tsx`:
- Around line 205-221: The watchResources map currently keys entries by
resource.kind which causes multiple resources of the same kind to clobber each
other; update the reducer in watchResources to key the accumulator by the
resource's prop key (e.g., resource.prop) instead of resource.kind, falling back
to resource.kind only if resource.prop is undefined, and keep the value shape as
WatchK8sResource (preserving kind, isList, namespace, selector, fieldSelector,
limit, namespaced, optional) so callers that rely on distinct prop keys are not
broken.
🧹 Nitpick comments (9)
frontend/public/components/monitoring/alertmanager/alertmanager-config.tsx (1)
621-621: Inline object creation defeatsmemo()onAlertmanagerConfigWrapper.The wrapper component at line 579 is memoized, but creating the
objprop inline means a new object reference on every render, causingmemo()to always re-render the subtree.Consider memoizing the prop object to preserve reference stability and allow
memo()to actually skip renders when the underlying values haven't changed:♻️ Proposed fix using useMemo
+ import { useMemo, memo, Suspense } from 'react'; - import { useMemo, memo, Suspense } from 'react';Then inside
AlertmanagerConfig:const [secret, loaded, loadError] = useK8sWatchResource({ kind: 'Secret', name: 'alertmanager-main', namespace: 'openshift-monitoring', isList: false, }); + const objProp = useMemo( + () => ({ data: secret as K8sResourceKind, loaded, loadError }), + [secret, loaded, loadError], + ); return ( <> <PageHeading breadcrumbs={breadcrumbs} title={t('public~Alertmanager')} /> <NavBar pages={[ { name: t('public~Details'), href: configPath, }, { name: t('public~YAML'), href: YAMLPath, }, ]} /> - <AlertmanagerConfigWrapper obj={{ data: secret as K8sResourceKind, loaded, loadError }} /> + <AlertmanagerConfigWrapper obj={objProp} /> </> );frontend/public/components/monitoring/alertmanager/alertmanager-yaml-editor.tsx (2)
152-152: Type assertion works but could be more explicit.The
secret as K8sResourceKindcast is technically safe becauseStatusBoxgates rendering untilloadedis true. However, the typing could be more precise to reflect thatsecretmay beundefinedbefore loading completes.Consider using a conditional or keeping the union type to make the loading state more explicit in the type system:
obj={{ data: secret, loaded, loadError }}Then update
AlertmanagerYAMLEditorWrapperProps.obj.datatoK8sResourceKind | undefined(which it already supports via the?modifier).This is a minor nit—functionally correct as-is since
StatusBoxhandles the loading state.
157-162: Consider explicit typing for StatusBox props.The
[key: string]: anyindex signature works but obscures the expected shape. For better maintainability and IDE support, consider explicitly typing theloadedandloadErrorproperties:type AlertmanagerYAMLEditorWrapperProps = { obj?: { data?: K8sResourceKind; loaded?: boolean; loadError?: unknown; }; };This documents the contract more clearly and aids future maintainers. Low priority since current code functions correctly.
frontend/public/components/console-notifier.tsx (2)
57-57: Data-test attribute may containundefined.When
notification.spec.locationis unset, thedata-testattribute becomes"name-undefined". This could make test selectors less predictable.✨ Suggested fix
- data-test={`${notification.metadata.name}-${notification.spec.location}`} + data-test={`${notification.metadata.name}-${notification.spec.location ?? 'BannerTop'}`}
9-9: Consider whether to standardize SDK imports across the component directory.The codebase follows an established pattern of importing from SDK internal paths (e.g.,
@console/dynamic-plugin-sdk/src/extensions/console-types,@console/dynamic-plugin-sdk/src/utils/flags). WhileK8sResourceCommonis available from the public API (@console/dynamic-plugin-sdk), many other components infrontend/public/components/use internal paths without issue. If stricter boundary enforcement is desired, this should be applied consistently across the directory; otherwise, the current import pattern is acceptable.frontend/public/components/command-line-tools.tsx (1)
37-38: Consider using_.filterinstead of_.removefor clarity.While
_.sortBycreates a new array (soobj.dataisn't mutated), using_.removeis a mutating operation that can be confusing in a React context. A functional approach with_.filterand_.rejectwould be more idiomatic:♻️ Suggested refactor
- const data = _.sortBy(obj.data, 'spec.displayName'); - const cliData = _.remove(data, (item) => item.metadata.name === 'oc-cli-downloads'); + const sortedData = _.sortBy(obj.data, 'spec.displayName'); + const cliData = _.filter(sortedData, (item) => item.metadata.name === 'oc-cli-downloads'); + const data = _.reject(sortedData, (item) => item.metadata.name === 'oc-cli-downloads');frontend/public/components/instantiate-template.tsx (2)
427-430: Strengthen typing forobjprop.Using
obj: anyforfeits TypeScript's safety benefits. Consider a proper type that reflects the watch result shape:♻️ Proposed type improvement
+import { WatchK8sResult } from '@console/dynamic-plugin-sdk'; + type TemplateFormProps = { - obj: any; + obj: { + data: TemplateKind; + loaded: boolean; + loadError?: Error; + }; preselectedNamespace: string; };This ensures consumers pass the expected shape and catches type mismatches at compile time—especially valuable as more components adopt this pattern.
410-413: Consider memoizing theobjprop to prevent unnecessary re-renders.Creating
{ data: template, loaded, loadError }inline means a new object reference on every render. While unlikely to cause visible issues here, it's a pattern that can trigger unnecessary child re-renders in more complex trees.♻️ Optional memoization
+import { useMemo } from 'react'; // ... + const objProp = useMemo( + () => ({ data: template, loaded, loadError }), + [template, loaded, loadError], + ); + return ( <> <DocumentTitle>{title}</DocumentTitle> <PageHeading title={title} /> <PaneBody> <TemplateForm preselectedNamespace={preselectedNamespace} - obj={{ data: template, loaded, loadError }} + obj={objProp} /> </PaneBody> </> );This is a nice-to-have optimization rather than a must-fix.
frontend/public/components/edit-yaml.tsx (1)
38-41: Import organization: internal module import between external and relative imports.Minor nit: The import from
@console/internal/components/utils/k8s-watch-hook(line 38) is placed between external package imports and relative imports. For consistency with the rest of the file's import structure, consider grouping it with other@console/internalimports (around lines 65-66).
| const [commonActions] = useCommonActions(model, obj, [CommonActionCreator.Delete] as const); | ||
|
|
||
| const { subjectIndex, subjects = [] } = obj; | ||
| const { subjectIndex, subjects } = obj ?? {}; |
There was a problem hiding this comment.
Incomplete null-guard: obj is used before this line.
The obj ?? {} fallback here doesn't protect line 39 where referenceFor(obj) is called. If obj is nullish during loading states (common in the Firehose → hooks migration), that call will throw before reaching this guard.
Consider either:
- Adding an early return if
objis nullish (return empty actions array during loading) - Updating the type signature to
obj?: RoleBindingKind | ClusterRoleBindingKindand guarding before line 39
🛡️ Proposed fix: early return guard
export const useBindingActions = (
- obj: RoleBindingKind | ClusterRoleBindingKind,
+ obj: RoleBindingKind | ClusterRoleBindingKind | undefined,
filterActions?: BindingActionCreator[],
): Action[] => {
const { t } = useTranslation();
+
+ // Return empty actions during loading states
+ if (!obj) {
+ return [];
+ }
+
const [model] = useK8sModel(referenceFor(obj));Then you can revert line 48 to the simpler form since obj is guaranteed defined:
- const { subjectIndex, subjects } = obj ?? {};
+ const { subjectIndex, subjects } = obj;🤖 Prompt for AI Agents
In `@frontend/packages/console-app/src/actions/hooks/useBindingActions.ts` at line
48, In useBindingActions, referenceFor(obj) is called before the defensive
destructure "const { subjectIndex, subjects } = obj ?? {}", which throws when
obj is nullish; add an early null/undefined guard at the top of
useBindingActions (e.g., if (!obj) return [] or return the empty actions array
used for loading) so no further work runs on a missing obj, then you can safely
destructure subjectIndex/subjects from obj without the "?? {}" fallback; ensure
the guard uses the same return shape the hook expects and reference the
functions/variables referenceFor, subjectIndex, subjects, and useBindingActions
when making the change.
| const deleteBindingSubject = useWarningModal({ | ||
| title: t('public~Delete {{label}} subject?', { | ||
| label: model.kind, | ||
| label: model?.kind, |
There was a problem hiding this comment.
Inconsistent model?.kind optional chaining.
Good safety improvement here, but model.kind is still used without optional chaining at lines 102, 106, 116, 120, and 128. If model can be undefined (as this change implies), those usages will throw.
Either guard consistently throughout, or add an early return when model is not yet loaded.
🛡️ Proposed fix: consistent optional chaining
[BindingActionCreator.DuplicateBinding]: () => ({
id: 'duplicate-binding',
label: t('public~Duplicate {{kindLabel}}', {
- kindLabel: model.kind,
+ kindLabel: model?.kind,
}),
cta: {
href: `${decodeURIComponent(
- resourceObjPath(obj, model.kind),
+ resourceObjPath(obj, model?.kind),
)}/copy?subjectIndex=${subjectIndex}`,
},Apply similar changes to lines 116, 120, and 128.
🤖 Prompt for AI Agents
In `@frontend/packages/console-app/src/actions/hooks/useBindingActions.ts` at line
52, The code now uses optional chaining for model?.kind in the label but still
accesses model.kind elsewhere (at the other uses in this file), which will throw
if model is undefined; update all occurrences of direct model.kind access in
useBindingActions.ts to use optional chaining (model?.kind) or, alternatively,
add an early guard like if (!model) return (or throw) at the top of the
hook/function so the rest can safely assume model is defined—apply this
consistently to every place that references model.kind in the file.
| export const CommandLineToolsPage = () => { | ||
| const shouldFetch = useFlag(FLAGS.CONSOLE_CLI_DOWNLOAD); | ||
| const [cliDownloads, loaded, loadError] = useK8sWatchResource( | ||
| shouldFetch | ||
| ? { | ||
| kind: referenceForModel(ConsoleCLIDownloadModel), | ||
| isList: true, | ||
| } | ||
| : null, | ||
| ); | ||
|
|
||
| return ( | ||
| <Firehose resources={resources}> | ||
| <CommandLineTools {...(props as any)} /> | ||
| </Firehose> | ||
| ); | ||
| }, | ||
| ); | ||
| return <CommandLineTools obj={{ data: cliDownloads as CLIDownload[], loaded, loadError }} />; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the useK8sWatchResource hook implementation
find . -type f \( -name "*.ts" -o -name "*.tsx" \) | head -20
rg "export.*useK8sWatchResource" -A 5 --type ts --type tsxRepository: openshift/console
Length of output: 1329
🏁 Script executed:
# Search for useK8sWatchResource in the entire codebase
rg "useK8sWatchResource" --type ts --type tsx -l | head -10Repository: openshift/console
Length of output: 88
🏁 Script executed:
# Look for the hook definition more broadly
rg "const useK8sWatchResource|function useK8sWatchResource" -A 10 --type ts --type tsxRepository: openshift/console
Length of output: 88
🏁 Script executed:
# Search without type filters
rg "export.*useK8sWatchResource|const useK8sWatchResource|function useK8sWatchResource" -A 15 | head -100Repository: openshift/console
Length of output: 12840
🏁 Script executed:
# Find where useK8sWatchResource is defined
rg "useK8sWatchResource\s*=" | head -20Repository: openshift/console
Length of output: 43
🏁 Script executed:
# Look for files that contain the hook definition
rg "useK8sWatchResource" -l | grep -E "hook|util|k8s" | head -10Repository: openshift/console
Length of output: 703
🏁 Script executed:
# Check if dynamic-plugin-sdk is in the repo
find . -type d -name "*dynamic-plugin-sdk*" 2>/dev/null | head -5Repository: openshift/console
Length of output: 107
🏁 Script executed:
# Look at the actual file that imports useK8sWatchResource
cat -n frontend/public/components/utils/k8s-watch-hook.tsRepository: openshift/console
Length of output: 290
🏁 Script executed:
# Check the CommandLineTools component to understand how it handles the data
rg "export.*CommandLineTools\s*=" -A 30 | head -80Repository: openshift/console
Length of output: 43
🏁 Script executed:
# Find the useK8sWatchResource implementation in dynamic-plugin-sdk
find ./frontend/packages/console-dynamic-plugin-sdk -name "*useK8sWatchResource*" -o -name "*k8s*hook*" 2>/dev/nullRepository: openshift/console
Length of output: 667
🏁 Script executed:
# Look for useK8sWatchResource implementation
rg "export.*function useK8sWatchResource|const useK8sWatchResource" ./frontend/packages/console-dynamic-plugin-sdk -A 20Repository: openshift/console
Length of output: 5355
🏁 Script executed:
# Find CommandLineTools component in command-line-tools.tsx
rg "const CommandLineTools\s*=|function CommandLineTools" -A 25Repository: openshift/console
Length of output: 43
🏁 Script executed:
# Read the full useK8sWatchResource implementation
cat -n ./frontend/packages/console-dynamic-plugin-sdk/src/utils/k8s/hooks/useK8sWatchResource.tsRepository: openshift/console
Length of output: 3156
🏁 Script executed:
# Find CommandLineTools component definition in the specific file
cat -n frontend/public/components/command-line-tools.tsx | grep -A 30 "const CommandLineTools"Repository: openshift/console
Length of output: 2331
🏁 Script executed:
# Look at the command-line-tools.tsx file around lines 85-115
sed -n '85,115p' frontend/public/components/command-line-tools.tsx | cat -nRepository: openshift/console
Length of output: 882
Fix unsafe type assertion and undefined data handling when feature flag is disabled.
When FLAGS.CONSOLE_CLI_DOWNLOAD is false, useK8sWatchResource(null) returns [undefined, true, undefined] (not an empty array). The type assertion on line 104 (cliDownloads as CLIDownload[]) unsafely casts undefined as an array, causing the CommandLineTools component to crash at runtime when it calls _.sortBy(obj.data, 'spec.displayName') on undefined data.
Either guard the null resource case with a conditional render, or ensure cliDownloads defaults to an empty array when the resource is null:
Example fix
- return <CommandLineTools obj={{ data: cliDownloads as CLIDownload[], loaded, loadError }} />;
+ if (!shouldFetch) {
+ return <CommandLineTools obj={{ data: [], loaded: true, loadError: undefined }} />;
+ }
+ return <CommandLineTools obj={{ data: cliDownloads as CLIDownload[], loaded, loadError }} />;Or use the nullish coalescing operator:
- return <CommandLineTools obj={{ data: cliDownloads as CLIDownload[], loaded, loadError }} />;
+ return <CommandLineTools obj={{ data: (cliDownloads ?? []) as CLIDownload[], loaded, loadError }} />;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const CommandLineToolsPage = () => { | |
| const shouldFetch = useFlag(FLAGS.CONSOLE_CLI_DOWNLOAD); | |
| const [cliDownloads, loaded, loadError] = useK8sWatchResource( | |
| shouldFetch | |
| ? { | |
| kind: referenceForModel(ConsoleCLIDownloadModel), | |
| isList: true, | |
| } | |
| : null, | |
| ); | |
| return ( | |
| <Firehose resources={resources}> | |
| <CommandLineTools {...(props as any)} /> | |
| </Firehose> | |
| ); | |
| }, | |
| ); | |
| return <CommandLineTools obj={{ data: cliDownloads as CLIDownload[], loaded, loadError }} />; | |
| }; | |
| export const CommandLineToolsPage = () => { | |
| const shouldFetch = useFlag(FLAGS.CONSOLE_CLI_DOWNLOAD); | |
| const [cliDownloads, loaded, loadError] = useK8sWatchResource( | |
| shouldFetch | |
| ? { | |
| kind: referenceForModel(ConsoleCLIDownloadModel), | |
| isList: true, | |
| } | |
| : null, | |
| ); | |
| return <CommandLineTools obj={{ data: (cliDownloads ?? []) as CLIDownload[], loaded, loadError }} />; | |
| }; |
🤖 Prompt for AI Agents
In `@frontend/public/components/command-line-tools.tsx` around lines 93 - 105,
When FLAGS.CONSOLE_CLI_DOWNLOAD is false useK8sWatchResource returns undefined
for cliDownloads and the unsafe cast in CommandLineToolsPage causes a runtime
crash; update CommandLineToolsPage to guard or normalize cliDownloads before
passing to CommandLineTools (e.g., if shouldFetch is false render null/alternate
UI or pass obj={{ data: cliDownloads ?? [] , loaded, loadError }}), referencing
the useFlag(FLAGS.CONSOLE_CLI_DOWNLOAD) and useK8sWatchResource(...) call and
the CommandLineTools prop to locate and fix the issue.
| const [notifications] = useK8sWatchResource<ConsoleNotification[]>( | ||
| shouldFetch | ||
| ? { | ||
| kind: referenceForModel(ConsoleNotificationModel), | ||
| isList: true, | ||
| } | ||
| : null, | ||
| ); | ||
|
|
||
| const ConsoleNotifier_: FC<PrivateConsoleNotifierProps> = ({ obj, location }) => { | ||
| if (_.isEmpty(obj)) { | ||
| if (!notifications?.length) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Loading and error states are not handled.
useK8sWatchResource returns [data, loaded, loadError], but loaded and loadError are not destructured. This means:
- While loading, the component returns
null(silent no-op) - On error, notifications silently fail to render
For a non-blocking banner, this graceful degradation may be intentional. However, consider at minimum logging errors to aid debugging when notifications unexpectedly don't appear.
🛠️ Suggested improvement to surface errors
- const [notifications] = useK8sWatchResource<ConsoleNotification[]>(
+ const [notifications, , loadError] = useK8sWatchResource<ConsoleNotification[]>(
shouldFetch
? {
kind: referenceForModel(ConsoleNotificationModel),
isList: true,
}
: null,
);
+
+ if (loadError) {
+ // eslint-disable-next-line no-console
+ console.warn('Failed to load console notifications:', loadError);
+ }
if (!notifications?.length) {🤖 Prompt for AI Agents
In `@frontend/public/components/console-notifier.tsx` around lines 30 - 41, The
component currently only destructures the first element from
useK8sWatchResource, so loading and error states are ignored; update the call to
destructure [notifications, loaded, loadError] from useK8sWatchResource (the
call that uses referenceForModel(ConsoleNotificationModel) and shouldFetch) and
add a simple handling: if loadError is present, call a debug/error logger with
context (e.g., "ConsoleNotification loadError") so errors are visible, and
optionally handle loaded === false (e.g., skip rendering or show a non-blocking
loading state) before using notifications.
| hideNav={true} | ||
| pages={[{ name: 'container', href: '', component: ContainerDetailsList }]} | ||
| obj={props.obj} | ||
| obj={{ data: props.obj.data, loaded: props.loaded }} |
There was a problem hiding this comment.
Fix obj shape mismatch passed to ContainerDetailsList.
ContainerDetailsList expects a PodKind, but this now passes {data, loaded}. That will make pod.spec undefined and can crash rendering.
🛠 Proposed fix
- obj={{ data: props.obj.data, loaded: props.loaded }}
+ obj={props.obj.data}📝 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.
| obj={{ data: props.obj.data, loaded: props.loaded }} | |
| obj={props.obj.data} |
🤖 Prompt for AI Agents
In `@frontend/public/components/container.tsx` at line 518, The component is
passing obj={{ data: props.obj.data, loaded: props.loaded }} to
ContainerDetailsList but that wrapper shape breaks ContainerDetailsList which
expects a PodKind (so pod.spec is accessed); change the prop to pass the actual
PodKind instead (e.g., obj={props.obj.data}) and, if loading state is needed by
the child, pass loaded via a separate prop (e.g., loaded={props.loaded}) so
ContainerDetailsList receives a PodKind with a defined spec.
| const templateName = searchParams.get('template'); | ||
| const templateNamespace = searchParams.get('template-ns'); | ||
| const preselectedNamespace = searchParams.get('preselected-ns'); | ||
| const resources = [ | ||
| { | ||
| kind: 'Template', | ||
| name: templateName, | ||
| namespace: templateNamespace, | ||
| isList: false, | ||
| prop: 'obj', | ||
| }, | ||
| ]; | ||
|
|
||
| const [template, loaded, loadError] = useK8sWatchResource({ | ||
| kind: 'Template', | ||
| name: templateName, | ||
| namespace: templateNamespace, | ||
| isList: false, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find the hook implementation
fd -t f 'k8s-watch' --type ts --type tsxRepository: openshift/console
Length of output: 230
🏁 Script executed:
#!/bin/bash
# Search for useK8sWatchResource hook definition
rg -l 'useK8sWatchResource' --type ts --type tsx | head -20Repository: openshift/console
Length of output: 88
🏁 Script executed:
#!/bin/bash
# Look at the instantiate-template.tsx file to understand the context better
wc -l frontend/public/components/instantiate-template.tsxRepository: openshift/console
Length of output: 116
🏁 Script executed:
#!/bin/bash
# Search for useK8sWatchResource hook definition without type restrictions
rg 'export.*useK8sWatchResource\s*=' -A 20Repository: openshift/console
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Read instantiate-template.tsx around the problematic lines
sed -n '380,410p' frontend/public/components/instantiate-template.tsxRepository: openshift/console
Length of output: 904
🏁 Script executed:
#!/bin/bash
# Look for usages of useK8sWatchResource with null/undefined
rg 'useK8sWatchResource\s*\(' -B 2 -A 3 | head -100Repository: openshift/console
Length of output: 8911
🏁 Script executed:
#!/bin/bash
# Find useK8sWatchResource hook definition
rg 'const useK8sWatchResource|function useK8sWatchResource' -B 2 -A 30 | head -100Repository: openshift/console
Length of output: 12334
🏁 Script executed:
#!/bin/bash
# Look for the hook in utils
fd 'k8s-watch'Repository: openshift/console
Length of output: 419
🏁 Script executed:
#!/bin/bash
# Search for hook file more broadly
find . -name '*hook*' -o -name '*watch*' | grep -E '\.(ts|tsx|js)$' | grep -v node_modules | head -20Repository: openshift/console
Length of output: 1647
🏁 Script executed:
#!/bin/bash
# Read the hook implementation
cat -n frontend/public/components/utils/k8s-watch-hook.tsRepository: openshift/console
Length of output: 290
🏁 Script executed:
#!/bin/bash
# Also check the watcher implementation
cat -n frontend/public/components/utils/k8s-watcher.tsRepository: openshift/console
Length of output: 195
🏁 Script executed:
#!/bin/bash
# Read the actual implementation in dynamic plugin SDK
cat -n frontend/packages/console-dynamic-plugin-sdk/src/utils/k8s/hooks/k8s-watcher.tsRepository: openshift/console
Length of output: 3916
🏁 Script executed:
#!/bin/bash
# Find and read useK8sWatchResource hook
fd 'useK8sWatchResource' frontend/packages/console-dynamic-plugin-sdk --type fRepository: openshift/console
Length of output: 655
🏁 Script executed:
#!/bin/bash
# Search for useK8sWatchResource definition
rg 'export.*useK8sWatchResource|const useK8sWatchResource' frontend/packages/console-dynamic-plugin-sdk -A 30 | head -150Repository: openshift/console
Length of output: 7973
🏁 Script executed:
#!/bin/bash
# Get the full useK8sWatchResource implementation
cat -n frontend/packages/console-dynamic-plugin-sdk/src/utils/k8s/hooks/useK8sWatchResource.tsRepository: openshift/console
Length of output: 3156
🏁 Script executed:
#!/bin/bash
# Read full instantiate-template.tsx to see error handling
cat -n frontend/public/components/instantiate-template.tsx | tail -100Repository: openshift/console
Length of output: 3726
🏁 Script executed:
#!/bin/bash
# See the render return statement
sed -n '410,441p' frontend/public/components/instantiate-template.tsxRepository: openshift/console
Length of output: 643
🏁 Script executed:
#!/bin/bash
# Find TemplateForm component implementation in the same file
sed -n '100,250p' frontend/public/components/instantiate-template.tsxRepository: openshift/console
Length of output: 4489
🏁 Script executed:
#!/bin/bash
# Look for where obj.data or template is used in TemplateForm
rg 'const TemplateForm' -A 100 frontend/public/components/instantiate-template.tsx | head -120Repository: openshift/console
Length of output: 3472
🏁 Script executed:
#!/bin/bash
# Check if there's any guard before accessing obj.data in TemplateForm render
sed -n '255,340p' frontend/public/components/instantiate-template.tsxRepository: openshift/console
Length of output: 2451
Guard against null URL parameters before invoking the watch hook.
When searchParams.get() returns null for templateName or templateNamespace, the hook still executes and returns [undefined, true, undefined]. The downstream guard if (!loaded) passes because loaded is true, allowing the component to render with template = undefined. This causes runtime errors when accessing template.parameters (line 321) and obj.data.metadata.name (lines 257, 262, 265).
Conditionally construct the watch spec to pass null when required parameters are missing:
🛡️ Proposed fix
const templateName = searchParams.get('template');
const templateNamespace = searchParams.get('template-ns');
const preselectedNamespace = searchParams.get('preselected-ns');
- const [template, loaded, loadError] = useK8sWatchResource({
- kind: 'Template',
- name: templateName,
- namespace: templateNamespace,
- isList: false,
- });
+ const watchSpec = templateName && templateNamespace
+ ? { kind: 'Template', name: templateName, namespace: templateNamespace, isList: false }
+ : null;
+
+ const [template, loaded, loadError] = useK8sWatchResource(watchSpec);This prevents the watch from running with incomplete parameters and allows the existing guards in TemplateForm to handle the missing template gracefully.
🤖 Prompt for AI Agents
In `@frontend/public/components/instantiate-template.tsx` around lines 394 - 403,
The hook useK8sWatchResource is being called even when
searchParams.get('template') or searchParams.get('template-ns') are null,
causing it to return [undefined, true, undefined] and downstream runtime errors
when TemplateForm accesses template.parameters and obj.data.metadata.*; fix by
building the watch spec conditionally—only call useK8sWatchResource with a valid
spec when both templateName and templateNamespace are non-null, otherwise pass
null (or skip the watch) so that template stays undefined and existing guards in
TemplateForm can handle the missing template; update the code around
templateName, templateNamespace and the useK8sWatchResource invocation to use
this conditional spec.
| export type NamespaceBarDropdownsProps = { | ||
| children: ReactNode; | ||
| isDisabled: boolean; | ||
| namespace?: FirehoseResult; | ||
| namespace?: WatchK8sResultsObject<K8sResourceCommon[]>; | ||
| onNamespaceChange: (namespace: string) => void; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the NamespaceBarDropdowns component and examine its implementation
rg -n "NamespaceBarDropdowns" -g '*.tsx' -g '*.ts' -C 3Repository: openshift/console
Length of output: 2470
🏁 Script executed:
# Also, let's look at the namespace-bar.tsx file to see how namespace is actually used
cat frontend/public/components/namespace-bar.tsxRepository: openshift/console
Length of output: 6100
🏁 Script executed:
# Search for imports of NamespaceBarDropdowns
rg "import.*NamespaceBarDropdowns" -g '*.tsx' -g '*.ts'Repository: openshift/console
Length of output: 43
🏁 Script executed:
# Also check the barrel exports if any
rg "export.*NamespaceBarDropdowns" -g '*.ts' -g '*.tsx'Repository: openshift/console
Length of output: 265
Make namespace a required prop—it's dereferenced without guards.
NamespaceBarDropdowns accesses namespace.loaded and namespace.data (lines 57–61) without null/undefined checks. While the current sole call site (NamespaceBar) does pass the prop, the component is exported and its type signature marks namespace as optional—this mismatch invites crashes from future consumers or external plugins.
Recommended fix
export type NamespaceBarDropdownsProps = {
children: ReactNode;
isDisabled: boolean;
- namespace?: WatchK8sResultsObject<K8sResourceCommon[]>;
+ namespace: WatchK8sResultsObject<K8sResourceCommon[]>;
onNamespaceChange: (namespace: string) => void;
useProjects: boolean;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export type NamespaceBarDropdownsProps = { | |
| children: ReactNode; | |
| isDisabled: boolean; | |
| namespace?: FirehoseResult; | |
| namespace?: WatchK8sResultsObject<K8sResourceCommon[]>; | |
| onNamespaceChange: (namespace: string) => void; | |
| export type NamespaceBarDropdownsProps = { | |
| children: ReactNode; | |
| isDisabled: boolean; | |
| namespace: WatchK8sResultsObject<K8sResourceCommon[]>; | |
| onNamespaceChange: (namespace: string) => void; | |
| useProjects: boolean; | |
| }; |
🤖 Prompt for AI Agents
In `@frontend/public/components/namespace-bar.tsx` around lines 31 - 35, The
NamespaceBarDropdowns component dereferences namespace.loaded and namespace.data
without null checks but NamespaceBarDropdownsProps currently marks namespace as
optional; make namespace required by changing NamespaceBarDropdownsProps so
namespace: WatchK8sResultsObject<K8sResourceCommon[]> (remove the ?), and update
any call sites if needed (e.g., NamespaceBar) to always pass a valid
WatchK8sResultsObject; this aligns the type with how NamespaceBarDropdowns uses
namespace and prevents future null/undefined crashes.
| const BindingLoadingWrapper: FC<BindingLoadingWrapperProps> = (props) => { | ||
| const { obj, loaded, loadError, fixedKeys } = props; | ||
| const [, setActiveNamespace] = useActiveNamespace(); | ||
|
|
||
| if (!loaded) { | ||
| return <LoadingBox />; | ||
| } | ||
|
|
||
| if (loadError) { | ||
| return <StatusBox data={obj} loaded={loaded} loadError={loadError} />; | ||
| } | ||
|
|
||
| if (!obj || _.isEmpty(obj)) { | ||
| return <StatusBox data={obj} loaded={loaded} loadError={loadError} />; | ||
| } | ||
|
|
||
| const fixed: { [key: string]: any } = {}; | ||
| _.each(props.fixedKeys, (k) => (fixed[k] = _.get(props.obj.data, k))); | ||
| fixedKeys.forEach((k) => (fixed[k] = obj?.[k])); | ||
|
|
||
| return ( | ||
| <StatusBox {...props.obj}> | ||
| <BaseEditRoleBinding | ||
| {...props} | ||
| setActiveNamespace={setActiveNamespace} | ||
| fixed={fixed} | ||
| obj={props.obj.data} | ||
| /> | ||
| </StatusBox> | ||
| <BaseEditRoleBinding | ||
| {...props} | ||
| setActiveNamespace={setActiveNamespace} | ||
| fixed={fixed} | ||
| obj={obj} | ||
| /> |
There was a problem hiding this comment.
Fix Biome error: forEach callback shouldn’t return a value.
The concise arrow returns the assignment result; Biome flags this. Use a block body.
Proposed fix
- fixedKeys.forEach((k) => (fixed[k] = obj?.[k]));
+ fixedKeys.forEach((k) => {
+ fixed[k] = obj?.[k];
+ });📝 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 BindingLoadingWrapper: FC<BindingLoadingWrapperProps> = (props) => { | |
| const { obj, loaded, loadError, fixedKeys } = props; | |
| const [, setActiveNamespace] = useActiveNamespace(); | |
| if (!loaded) { | |
| return <LoadingBox />; | |
| } | |
| if (loadError) { | |
| return <StatusBox data={obj} loaded={loaded} loadError={loadError} />; | |
| } | |
| if (!obj || _.isEmpty(obj)) { | |
| return <StatusBox data={obj} loaded={loaded} loadError={loadError} />; | |
| } | |
| const fixed: { [key: string]: any } = {}; | |
| _.each(props.fixedKeys, (k) => (fixed[k] = _.get(props.obj.data, k))); | |
| fixedKeys.forEach((k) => (fixed[k] = obj?.[k])); | |
| return ( | |
| <StatusBox {...props.obj}> | |
| <BaseEditRoleBinding | |
| {...props} | |
| setActiveNamespace={setActiveNamespace} | |
| fixed={fixed} | |
| obj={props.obj.data} | |
| /> | |
| </StatusBox> | |
| <BaseEditRoleBinding | |
| {...props} | |
| setActiveNamespace={setActiveNamespace} | |
| fixed={fixed} | |
| obj={obj} | |
| /> | |
| const BindingLoadingWrapper: FC<BindingLoadingWrapperProps> = (props) => { | |
| const { obj, loaded, loadError, fixedKeys } = props; | |
| const [, setActiveNamespace] = useActiveNamespace(); | |
| if (!loaded) { | |
| return <LoadingBox />; | |
| } | |
| if (loadError) { | |
| return <StatusBox data={obj} loaded={loaded} loadError={loadError} />; | |
| } | |
| if (!obj || _.isEmpty(obj)) { | |
| return <StatusBox data={obj} loaded={loaded} loadError={loadError} />; | |
| } | |
| const fixed: { [key: string]: any } = {}; | |
| fixedKeys.forEach((k) => { | |
| fixed[k] = obj?.[k]; | |
| }); | |
| return ( | |
| <BaseEditRoleBinding | |
| {...props} | |
| setActiveNamespace={setActiveNamespace} | |
| fixed={fixed} | |
| obj={obj} | |
| /> |
🧰 Tools
🪛 Biome (2.3.13)
[error] 807-807: This callback passed to forEach() iterable method should not return a value.
Either remove this return or remove the returned value.
(lint/suspicious/useIterableCallbackReturn)
🤖 Prompt for AI Agents
In `@frontend/public/components/RBAC/bindings.tsx` around lines 790 - 815,
BindingLoadingWrapper uses a concise arrow in fixedKeys.forEach that returns the
assignment value which Biome flags; change the callback to a block body so it
does not return a value (e.g., replace the concise arrow in the forEach on
fixedKeys with a block callback that assigns fixed[k] = obj?.[k];) to eliminate
the returned value while keeping the same behavior in BindingLoadingWrapper and
the fixed variable population.
| export const StorageClassForm = (props) => { | ||
| const resources = [ | ||
| { kind: StorageClassModel.kind, isList: true, prop: 'sc' }, | ||
| { kind: referenceForModel(CSIDriverModel), isList: true, prop: 'csi' }, | ||
| ]; | ||
| const resources = useK8sWatchResources({ | ||
| sc: { kind: StorageClassModel.kind, isList: true }, | ||
| csi: { kind: referenceForModel(CSIDriverModel), isList: true }, | ||
| }); | ||
|
|
||
| return ( | ||
| <Firehose resources={resources}> | ||
| <ConnectedStorageClassForm {...props} /> | ||
| </Firehose> | ||
| <ConnectedStorageClassForm | ||
| {...props} | ||
| sc={{ | ||
| data: resources.sc.data, | ||
| loaded: resources.sc.loaded, | ||
| loadError: resources.sc.loadError, | ||
| }} | ||
| csi={{ | ||
| data: resources.csi.data, | ||
| loaded: resources.csi.loaded, | ||
| loadError: resources.csi.loadError, | ||
| }} | ||
| /> | ||
| ); |
There was a problem hiding this comment.
StorageClassFormInner never sees the watched data.
StorageClassFormInner still reads props.resources?.sc/csi, but the wrapper now passes sc/csi as top‑level props, so resources stays undefined and validation/uniqueness checks won’t run.
🛠️ Proposed fix: pass a resources object (matches current inner usage)
return (
<ConnectedStorageClassForm
{...props}
- sc={{
- data: resources.sc.data,
- loaded: resources.sc.loaded,
- loadError: resources.sc.loadError,
- }}
- csi={{
- data: resources.csi.data,
- loaded: resources.csi.loaded,
- loadError: resources.csi.loadError,
- }}
+ resources={{
+ sc: {
+ data: resources.sc.data,
+ loaded: resources.sc.loaded,
+ loadError: resources.sc.loadError,
+ },
+ csi: {
+ data: resources.csi.data,
+ loaded: resources.csi.loaded,
+ loadError: resources.csi.loadError,
+ },
+ }}
/>
);🤖 Prompt for AI Agents
In `@frontend/public/components/storage-class-form.tsx` around lines 784 - 804,
StorageClassForm is passing sc and csi as top-level props to
ConnectedStorageClassForm but StorageClassFormInner (used by
ConnectedStorageClassForm) expects a resources object (reads props.resources?.sc
and props.resources?.csi), so wrap the existing sc/csi props into a resources
object when calling ConnectedStorageClassForm; in other words, change the call
site in StorageClassForm to pass resources={{ sc: { data: resources.sc.data,
loaded: resources.sc.loaded, loadError: resources.sc.loadError }, csi: { data:
resources.csi.data, loaded: resources.csi.loaded, loadError:
resources.csi.loadError } }} so StorageClassFormInner’s validation/uniqueness
checks see the watched data.
| const watchResources = useMemo(() => { | ||
| if (!props.resources || props.resources.length === 0) { | ||
| return {}; | ||
| } | ||
| return props.resources.reduce((acc, resource) => { | ||
| acc[resource.kind] = { | ||
| kind: resource.kind, | ||
| isList: true, | ||
| namespace: resource.namespace, | ||
| selector: resource.selector, | ||
| fieldSelector: resource.fieldSelector, | ||
| limit: resource.limit, | ||
| namespaced: resource.namespaced, | ||
| optional: resource.optional, | ||
| }; | ||
| return acc; | ||
| }, {} as Record<string, WatchK8sResource>); |
There was a problem hiding this comment.
Keying watch resources by kind can overwrite entries.
Using resource.kind as the map key ignores prop and collapses multiple resources of the same kind, which breaks callers that rely on distinct prop keys.
🛠️ Proposed fix: preserve `prop` keys
return props.resources.reduce((acc, resource) => {
- acc[resource.kind] = {
+ const key = resource.prop ?? resource.kind;
+ acc[key] = {
kind: resource.kind,
isList: true,
namespace: resource.namespace,
selector: resource.selector,
fieldSelector: resource.fieldSelector,
limit: resource.limit,
namespaced: resource.namespaced,
optional: resource.optional,
};
return acc;
}, {} as Record<string, WatchK8sResource>);🤖 Prompt for AI Agents
In `@frontend/public/components/utils/list-dropdown.tsx` around lines 205 - 221,
The watchResources map currently keys entries by resource.kind which causes
multiple resources of the same kind to clobber each other; update the reducer in
watchResources to key the accumulator by the resource's prop key (e.g.,
resource.prop) instead of resource.kind, falling back to resource.kind only if
resource.prop is undefined, and keep the value shape as WatchK8sResource
(preserving kind, isList, namespace, selector, fieldSelector, limit, namespaced,
optional) so callers that rely on distinct prop keys are not broken.
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
cajieh
force-pushed
the
migrate-firehose-to-hooks
branch
2 times, most recently
from
c68cd5b to
793236e
Compare
cajieh
force-pushed
the
migrate-firehose-to-hooks
branch
3 times, most recently
from
6bb55df to
267b1ac
Compare
|
|
||
| const data = flatten ? flatten(resources) : []; | ||
| // Use watchedResources (from useK8sWatchResources) if available, fallback to resources (from Firehose) | ||
| const resourceData = watchedResources ?? resources; |
There was a problem hiding this comment.
TODO: Remove the the resources prop and the fallback ?? resources after all components are migrated from Firehose to hooks.
|
/test e2e-gcp-console |
|
/test okd-scos-images |
cajieh
force-pushed
the
migrate-firehose-to-hooks
branch
from
267b1ac to
c5d8a5e
Compare
|
/test e2e-gcp-console |
openshift-merge-robot
added
the
needs-rebase
cajieh
force-pushed
the
migrate-firehose-to-hooks
branch
from
c5d8a5e to
e9b362a
Compare
openshift-merge-robot
removed
the
needs-rebase
cajieh
force-pushed
the
migrate-firehose-to-hooks
branch
from
e9b362a to
31ca68c
Compare
|
@cajieh: 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. |
|
/test e2e-gcp-console |
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. 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. |
4 participants
This change migrates components dependent on the
Firehosecomponent touseK8sWatchResource(s). The removal of the Firehose component and the cleanup of associated types will be covered in follow-on work.Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements