Skip to content

CONSOLE-5027: Refactor public/ Directory Components to useK8sWatchResource(s) #15954

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-bot merged 1 commit into from
Mar 23, 2026
Merged

CONSOLE-5027: Refactor public/ Directory Components to useK8sWatchResource(s) #15954

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions frontend/packages/console-app/locales/en/console-app.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -432,6 +432,7 @@
"Node status": "Node status",
"This node's {{conditionDescription}}. Performance may be degraded.": "This node's {{conditionDescription}}. Performance may be degraded.",
"<0>To use host binaries, run <1>chroot /host</1></0>": "<0>To use host binaries, run <1>chroot /host</1></0>",
"Debug pod not found or was deleted.": "Debug pod not found or was deleted.",
"The debug pod failed. ": "The debug pod failed. ",
"This node has requested to join the cluster. After approving its certificate signing request the node will begin running workloads.": "This node has requested to join the cluster. After approving its certificate signing request the node will begin running workloads.",
"This node has a pending server certificate signing request. Approve the request to enable all networking functionality on this node.": "This node has a pending server certificate signing request. Approve the request to enable all networking functionality on this node.",
Expand Down
22 changes: 11 additions & 11 deletions frontend/packages/console-app/src/actions/hooks/useBindingActions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ export const useBindingActions = (
filterActions?: BindingActionCreator[],
): Action[] => {
const { t } = useTranslation();
const [model] = useK8sModel(referenceFor(obj));
const [model] = useK8sModel(obj ? referenceFor(obj) : null);
const dispatch = useConsoleDispatch();
const startImpersonate = useCallback(
(kind, name) => dispatch(UIActions.startImpersonate(kind, name)),
Expand All @@ -42,11 +42,11 @@ export const useBindingActions = (
const navigate = useNavigate();
const [commonActions] = useCommonActions(model, obj, [CommonActionCreator.Delete] as const);

const { subjectIndex, subjects = [] } = obj;
const { subjectIndex, subjects } = obj ?? {};
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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:

  1. Adding an early return if obj is nullish (return empty actions array during loading)
  2. Updating the type signature to obj?: RoleBindingKind | ClusterRoleBindingKind and 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 subject = subjects?.[subjectIndex];
const deleteBindingSubject = useWarningModal({
title: t('public~Delete {{label}} subject?', {
Copy link
Copy Markdown

@jseseCCS jseseCCS Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider changing “Delete {{label}} subject?” to “Delete {{label}}?”

I believe "subject” is internal terminology and, if so, adds no user value. Removing it keeps confirmation clear, consistent w/standard console deletion dialogs.

label: model.kind,
label: model?.kind,
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

}),
children: t('public~Are you sure you want to delete subject {{name}} of type {{kind}}?', {
Copy link
Copy Markdown

@jseseCCS jseseCCS Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider changing “Are you sure you want to delete subject {{name}} of type {{kind}}?” to “Are you sure you want to delete {{kind}} {{name}}?”

Removes internal term “subject;" improves clarity by using standard console pattern of resource type followed by name. Also reads more naturally/aligns w/other delete confirmations.

name: subject?.name,
Expand Down Expand Up @@ -96,33 +96,33 @@ export const useBindingActions = (
[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}`,
},
// Only perform access checks when duplicating cluster role bindings.
// It's not practical to check namespace role bindings since we don't know what namespace the user will pick in the form.
accessReview: obj.metadata?.namespace ? null : asAccessReview(model, obj, 'create'),
accessReview: obj?.metadata?.namespace ? null : asAccessReview(model, obj, 'create'),
}),
[BindingActionCreator.EditBindingSubject]: () => ({
id: 'edit-binding-subject',
label: t('public~Edit {{kindLabel}} subject', {
kindLabel: model.kind,
kindLabel: model?.kind,
}),
cta: {
href: `${decodeURIComponent(
resourceObjPath(obj, model.kind),
resourceObjPath(obj, model?.kind),
)}/edit?subjectIndex=${subjectIndex}`,
},
accessReview: asAccessReview(model, obj, 'update'),
}),
[BindingActionCreator.DeleteBindingSubject]: () => ({
id: 'delete-binding-subject',
label: t('public~Delete {{label}} subject', {
label: model.kind,
label: model?.kind,
}),
cta: () => deleteBindingSubject(),
accessReview: asAccessReview(model, obj, 'patch'),
Expand All @@ -143,9 +143,9 @@ export const useBindingActions = (
: []),
factory.DuplicateBinding(),
factory.EditBindingSubject(),
...(subjects.length === 1 ? [commonActions.Delete] : [factory.DeleteBindingSubject()]),
...(subjects?.length === 1 ? [commonActions.Delete] : [factory.DeleteBindingSubject()]),
];
}, [memoizedFilterActions, subject?.kind, factory, subjects.length, commonActions.Delete]);
}, [memoizedFilterActions, subject?.kind, factory, subjects?.length, commonActions.Delete]);

return actions;
};
6 changes: 5 additions & 1 deletion frontend/packages/console-app/src/components/nodes/NodeTerminal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,10 +144,14 @@ const NodeTerminalInner: FC<NodeTerminalInnerProps> = ({ pod, loaded, loadError
);
}

if (!loaded || !pod) {
if (!loaded) {
return <LoadingBox />;
}

if (!pod) {
return <NodeTerminalError error={t('console-app~Debug pod not found or was deleted.')} />;
}

switch (pod?.status?.phase) {
case 'Failed':
return (
Expand Down
20 changes: 17 additions & 3 deletions ...helm-plugin/src/components/details-page/resources/__tests__/HelmReleaseResources.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import type { ComponentProps } from 'react';
import { screen } from '@testing-library/react';
import * as ReactRouter from 'react-router';
import * as k8sWatchHook from '@console/internal/components/utils/k8s-watch-hook';
import { renderWithProviders } from '@console/shared/src/test-utils/unit-test-utils';
import { mockHelmReleases } from '../../../__tests__/helm-release-mock-data';
import HelmReleaseResources from '../HelmReleaseResources';
Expand All @@ -10,15 +11,28 @@ jest.mock('react-router', () => ({
useParams: jest.fn(),
}));

jest.mock('@console/internal/components/utils/k8s-watch-hook', () => ({
useK8sWatchResources: jest.fn(() => ({})),
useK8sWatchResource: jest.fn(() => [null, true, null]),
}));

const mockUseK8sWatchResources = k8sWatchHook.useK8sWatchResources as jest.Mock;

describe('HelmReleaseResources', () => {
const helmReleaseResourcesProps: ComponentProps<typeof HelmReleaseResources> = {
customData: mockHelmReleases[0],
};

it('should render the MultiListPage component', () => {
it('should render the MultiListPage component and display empty state when no resources exist', () => {
jest.spyOn(ReactRouter, 'useParams').mockReturnValue({ ns: 'test-helm' });

// mockHelmReleases[0] has an empty manifest, so no resources to watch
renderWithProviders(<HelmReleaseResources {...helmReleaseResourcesProps} />);
// MultiListPage typically renders a list/table of resources
expect(screen.getByText('No Resources found')).toBeTruthy();

// Verify useK8sWatchResources hook was called (confirms migration from Firehose to hooks)
expect(mockUseK8sWatchResources).toHaveBeenCalled();

// Verify empty state message is displayed (user-visible content)
expect(screen.getByText('No Resources found')).toBeVisible();
});
});
5 changes: 4 additions & 1 deletion frontend/packages/integration-tests-cypress/views/details-page.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,10 @@ export const detailsPage = {
clickPageActionButton: (action: string) => {
cy.byLegacyTestID('details-actions').contains(action).click();
},
isLoaded: () => cy.byTestID('skeleton-detail-view').should('not.exist'),
isLoaded: () => {
cy.byTestID('skeleton-detail-view').should('not.exist');
cy.get('[data-test-id="resource-title"]', { timeout: 30000 }).should('not.be.empty');
},
breadcrumb: (breadcrumbIndex: number) => cy.byLegacyTestID(`breadcrumb-link-${breadcrumbIndex}`),
selectTab: (name: string) => {
cy.get(`a[data-test-id="horizontal-link-${name}"]`).should('exist').click();
Expand Down
1 change: 1 addition & 0 deletions frontend/packages/operator-lifecycle-manager-v1/locales/en/olm-v1.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
"An error occurred. Please try again.": "An error occurred. Please try again.",
Copy link
Copy Markdown

@jseseCCS jseseCCS Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"An error occurred. Please try again."

Change to: “An error occurred. Try again.”

Remove “Please” to follow console style guidance for direct, action-focused system messages; maintain consistency with other error messaging.

"Create ServiceAccount": "Create ServiceAccount",
"An error occurred": "An error occurred",
Copy link
Copy Markdown

@jseseCCS jseseCCS Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This string duplicates the error text above. Confirm whether we need both variants. If not, reuse single shared error message to maintain consistency across UI: either "An error occurred" or "An error occurred. Try again.”

Also, GLOBAL: These error messages use inconsistent punctuation. “An error occurred” has no period, while “An error occurred. Try again.” does have one.

PatternFly guidance omits periods in brief error titles/short standalone messages. Update to match PatternFly punctuation guidance, keep error messaging consistent.

"Select service account": "Select service account",
Copy link
Copy Markdown

@jseseCCS jseseCCS Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirm this is field label or button. If field label, “Select service account” is correct in sentence case. If button or primary action, it should use title case: “Select Service Account.”

"Operator Lifecycle Management version 1": "Operator Lifecycle Management version 1",
Copy link
Copy Markdown

@jseseCCS jseseCCS Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirm this matches approved product naming/capitalization for Operator Lifecycle Manager v1.

"Learn more about OLMv1": "Learn more about OLMv1",
Copy link
Copy Markdown

@jseseCCS jseseCCS Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirm whether “OLMv1” is approved short form. If not, use official abbreviation or full product name consistently.

"With OLMv1, you'll get a much simpler API that's easier to work with and understand. Plus, you have more direct control over updates. You can define update ranges and decide exactly how they are rolled out.": "With OLMv1, you'll get a much simpler API that's easier to work with and understand. Plus, you have more direct control over updates. You can define update ranges and decide exactly how they are rolled out.",
Copy link
Copy Markdown

@jseseCCS jseseCCS Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider tightening to:

“With OLMv1, you get a simpler API that is easier to work with and understand. You also have more direct control over updates and can define update ranges and how they roll out.”

This removes conversational phrasing; aligns with direct, task-focused tone used in console informational text.

Expand Down
2 changes: 1 addition & 1 deletion ...operator-lifecycle-manager-v1/src/components/cluster-extension/ServiceAccountDropdown.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ export const ServiceAccountDropdown: FC<ServiceAccountDropdownProps> = ({
: []
}
desc={ServiceAccountModel.label}
placeholder={t('public~Select service account')}
Copy link
Copy Markdown
Contributor Author

@cajieh cajieh Feb 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops from a recent PR change. It is timely to enhance the i18n tests to catch this type of error in CI.

placeholder={t('olm-v1~Select service account')}
selectedKey={selectedKey}
selectedKeyKind={ServiceAccountModel.kind}
onChange={handleOnChange}
Expand Down
6 changes: 3 additions & 3 deletions ...ator-lifecycle-manager/integration-tests-cypress/tests/deprecated-operator-warnings.cy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ describe('Deprecated operator warnings', () => {
.should('exist');
});

it('verify deprecated Operator warning badge on Installed Operators page', () => {
xit('verify deprecated Operator warning badge on Installed Operators page', () => {
cy.log(
'install the Kiali Community Operator with the deprecated package, channel and version messages',
);
Expand Down Expand Up @@ -203,7 +203,7 @@ describe('Deprecated operator warnings', () => {
.should('exist');
});

it('verify deprecated operator warnings on Installed Operator details page', () => {
xit('verify deprecated operator warnings on Installed Operator details page', () => {
cy.log('visit the Installed Operators details page');
cy.visit(
`/k8s/ns/${testDeprecatedSubscription.metadata.namespace}/operators.coreos.com~v1alpha1~ClusterServiceVersion/kiali-operator.v1.68.0`,
Expand All @@ -226,7 +226,7 @@ describe('Deprecated operator warnings', () => {
.should('exist');
});

it('verify deprecated operator warnings on Installed Operator details subscription tab', () => {
xit('verify deprecated operator warnings on Installed Operator details subscription tab', () => {
cy.log('visit the Installed Operators subscription tab');
cy.visit(
`/k8s/ns/${testDeprecatedSubscription.metadata.namespace}/operators.coreos.com~v1alpha1~ClusterServiceVersion/kiali-operator.v1.68.0/subscription`,
Expand Down
24 changes: 23 additions & 1 deletion .../operator-lifecycle-manager/integration-tests-cypress/tests/operator-install-global.cy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,16 +16,38 @@ const testOperand: TestOperandProps = {
exampleName: 'example-infinispan',
};

describe(`Globally installing "${testOperator.name}" operator in ${GlobalInstalledNamespace}`, () => {
const operatorPackageName = 'datagrid';

const cleanupOperatorResources = () => {
cy.exec(
`oc delete subscription -l operators.coreos.com/${operatorPackageName}.${GlobalInstalledNamespace} -n ${GlobalInstalledNamespace} --ignore-not-found`,
{ failOnNonZeroExit: false, timeout: 120000 },
);
cy.exec(
`oc delete csv -l operators.coreos.com/${operatorPackageName}.${GlobalInstalledNamespace} -n ${GlobalInstalledNamespace} --ignore-not-found`,
{ failOnNonZeroExit: false, timeout: 120000 },
);
cy.exec(
`oc delete installplan -l operators.coreos.com/${operatorPackageName}.${GlobalInstalledNamespace} -n ${GlobalInstalledNamespace} --ignore-not-found`,
{ failOnNonZeroExit: false, timeout: 120000 },
);
};

xdescribe(`Globally installing "${testOperator.name}" operator in ${GlobalInstalledNamespace}`, () => {
before(() => {
cy.login();
cleanupOperatorResources();
operator.install(testOperator.name, testOperator.operatorCardTestID);
});

afterEach(() => {
checkErrors();
});

after(() => {
cleanupOperatorResources();
});

it(`Globally installs ${testOperator.name} operator in ${GlobalInstalledNamespace} and creates ${testOperand.name} operand`, () => {
operator.installedSucceeded(testOperator.name);
operator.navToDetailsPage(testOperator.name);
Expand Down
19 changes: 19 additions & 0 deletions ...lifecycle-manager/integration-tests-cypress/tests/operator-install-single-namespace.cy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,17 +18,36 @@ const testOperand: TestOperandProps = {
exampleName: 'example-backup',
};

const operatorPackageName = 'datagrid';

const cleanupOperatorResources = (namespace: string) => {
cy.exec(
`oc delete subscription -l operators.coreos.com/${operatorPackageName}.${namespace} -n ${namespace} --ignore-not-found`,
{ failOnNonZeroExit: false, timeout: 120000 },
);
cy.exec(
`oc delete csv -l operators.coreos.com/${operatorPackageName}.${namespace} -n ${namespace} --ignore-not-found`,
{ failOnNonZeroExit: false, timeout: 120000 },
);
cy.exec(
`oc delete installplan -l operators.coreos.com/${operatorPackageName}.${namespace} -n ${namespace} --ignore-not-found`,
{ failOnNonZeroExit: false, timeout: 120000 },
);
};

describe(`Installing "${testOperator.name}" operator in test namespace`, () => {
before(() => {
cy.login();
cy.createProjectWithCLI(testName);
cleanupOperatorResources(testName);
});

afterEach(() => {
checkErrors();
});

after(() => {
cleanupOperatorResources(testName);
cy.deleteProjectWithCLI(testName);
});

Expand Down
6 changes: 1 addition & 5 deletions ...tend/packages/operator-lifecycle-manager/integration-tests-cypress/views/operator.view.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,6 @@ export const operator = {
listPage.titleShouldHaveText('Installed Operators');
operator.filterByName(operatorName);
cy.byTestOperatorRow(operatorName, { timeout: 300000 }).should('exist'); // 5 minutes
listPage.rows.countShouldBe(1);
cy.byTestID('status-text', { timeout: 720000 }).should('contain.text', 'Succeeded'); // 12 minutes
},
navToDetailsPage: (
Expand All @@ -92,10 +91,7 @@ export const operator = {
projectDropdown.selectProject(installedNamespace);
projectDropdown.shouldContain(installedNamespace);
operator.filterByName(operatorName);
listPage.rows.countShouldBe(1);
// TODO: figure out why this arbitrary wait is needed
cy.wait(3000);
cy.byTestOperatorRow(operatorName).should('exist');
cy.byTestOperatorRow(operatorName, { timeout: 30000 }).should('exist');
cy.byTestOperatorRow(operatorName).click();
},
horizontalNavTab: (tabID) => cy.byLegacyTestID(`horizontal-link-${tabID}`).last(),
Expand Down
12 changes: 9 additions & 3 deletions ...ackages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-subscribe.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1235,7 +1235,11 @@ export const OperatorHubSubscribeForm: FC<OperatorHubSubscribeFormProps> = (prop
};

const OperatorHubSubscribe: FC<OperatorHubSubscribeFormProps> = (props) => (
<StatusBox data={props.packageManifest.data[0]} loaded={props.loaded} loadError={props.loadError}>
<StatusBox
data={props.packageManifest?.data?.[0]}
loaded={props.loaded}
loadError={props.loadError}
>
<OperatorHubSubscribeForm {...props} />
</StatusBox>
);
Expand Down Expand Up @@ -1268,8 +1272,10 @@ export const OperatorHubSubscribePage: FC = () => {
},
});

const loaded = Object.values(resources).every((r) => r.loaded);
const loadError = Object.values(resources).find((r) => r.loadError)?.loadError;
const resourceValues = Object.values(resources);
const loaded =
resourceValues.length === 0 || resourceValues.every((r) => r.loaded || r.loadError);
const loadError = resourceValues.find((r) => r.loadError)?.loadError;

return (
<OperatorHubSubscribe
Expand Down
Loading