CONSOLE-4677: Enable strictNullChecks: true for frontend/insights-plugin and frontend/helm-plugin #15396

krishagarwal278 · 2025-08-12T20:30:30Z

No description provided.

openshift-ci-robot · 2025-08-12T20:30:34Z

@krishagarwal278: This pull request references CONSOLE-4677 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 sub-task to target the "4.20.0" version, but no target version was set.

In 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.

krishagarwal278 · 2025-08-12T20:36:06Z

/label docs-approved
/label px-approved

krishagarwal278 · 2025-08-12T20:36:20Z

/label tide/merge-method-squash

krishagarwal278 · 2025-08-12T20:40:18Z

/assign @yapei
/assign @jhadvig

yanpzhan · 2025-08-14T07:02:42Z

Checked on cluster launched against the pr. The regression test passed.
/label qe-approved

openshift-ci-robot · 2025-08-14T07:02:53Z

@krishagarwal278: This pull request references CONSOLE-4677 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 sub-task to target the "4.20.0" version, but no target version was set.

In 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.

krishagarwal278 · 2025-08-14T17:40:43Z

/retest

jhadvig · 2025-08-15T12:04:13Z

The frontend test is failing with a few valid issues

krishagarwal278 · 2025-08-15T15:35:29Z

The frontend test is failing with a few valid issues

Yes, latest commit should address this failure

logonoff · 2025-08-19T18:53:13Z

frontend/packages/helm-plugin/src/topology/components/HelmReleaseStatusDecorator.tsx

@@ -31,7 +31,7 @@ const HelmReleaseStatusDecorator: React.FC<HelmReleaseStatusDecoratorProps> = ({

  return (
    <Tooltip triggerRef={ref} content={label} position={TooltipPosition.left}>
-      <g ref={ref}>
+      <g ref={(ref as unknown) as React.RefObject<SVGGElement>}>


can we instead change the type of ref instead of coercing it like this?

const ref = useRef<SVGGElement>();

addressed in latest commit

logonoff · 2025-08-19T18:54:07Z

frontend/packages/helm-plugin/src/topology/sidebar/release-panel/tab-sections.tsx

@@ -25,7 +26,7 @@ const HelmReleasePanelDetailsTabSection: React.FC<{ element: GraphElement }> = (
      <p>Status Box</p>
    </>
  ) : (
-    <HelmReleaseOverview obj={secret} customData={undefined} />
+    <HelmReleaseOverview obj={secret} customData={(undefined as unknown) as HelmRelease} />


instead of forcing undefined to be a HelmRelease we should instead make HelmReleaseOverview accept undefined in this prop

addressed in latest commit

logonoff

looking a lot better!

logonoff · 2025-08-21T19:42:22Z

frontend/packages/helm-plugin/src/components/details-page/resources/HelmReleaseResourcesRow.tsx

        />
      </TableData>
      <TableData className={tableColumnClasses.type}>{resource.kind}</TableData>
      <TableData className={tableColumnClasses.status}>
        <HelmReleaseResourceStatus resource={resource} />
      </TableData>
      <TableData className={tableColumnClasses.created}>
-        <Timestamp timestamp={resource.metadata.creationTimestamp} />
+        <Timestamp timestamp={resource.metadata?.creationTimestamp || ''} />


maybe we can copy the timestamp changes from your other PR so we don't need the empty string fallback here?

Oh yes definitely. Making that change

logonoff · 2025-08-21T19:43:21Z

frontend/packages/helm-plugin/src/components/forms/install-upgrade/HelmInstallUpgradePage.tsx

@@ -232,7 +237,7 @@ const HelmInstallUpgradePage: React.FunctionComponent = () => {
      onNamespaceChange={handleNamespaceChange}
      hideApplications
    >
-      <DocumentTitle>{config.title}</DocumentTitle>
+      <DocumentTitle>{config?.title || ''}</DocumentTitle>


i think we wouldn't want to set the title as empty string

Suggested change

<DocumentTitle>{config?.title || ''}</DocumentTitle>

{config?.title && <DocumentTitle>{config.title}</DocumentTitle> }

logonoff · 2025-08-21T19:43:49Z

frontend/packages/helm-plugin/src/components/forms/install-upgrade/HelmInstallUpgradePage.tsx

+            helmActionConfig={config as HelmActionConfigType}
            onVersionChange={setChartData}
-            chartError={chartError}
-            namespace={namespace}
-            chartIndexEntry={indexEntry}
+            chartError={chartError as Error}


are these type assertions still needed?

RIght, it can be removed

logonoff · 2025-08-21T19:44:06Z

frontend/packages/helm-plugin/src/components/list-page/RepositoriesListPage.tsx

@@ -110,7 +125,7 @@ const RepositoriesPage: React.FC<RepositoriesPageProps> = ({
      <DocumentTitle>{t('helm-plugin~Helm Repositories')}</DocumentTitle>
      <MultiListPage
        namespace={namespace}
-        flatten={flatten}
+        flatten={flatten as Flatten}


is this type assertion still needed?

Yes, giving type errors when i remove it

logonoff · 2025-08-21T19:45:25Z

frontend/packages/helm-plugin/src/topology/components/HelmReleaseGroup.tsx

  const [{ dragging: labelDragging }, dragLabelRef] = useDragNode(noRegroupDragSourceSpec);
-  const nodeRefs = useCombineRefs(innerHoverRef, dragNodeRef);
+  const nodeRefs = useCombineRefs(innerHoverRef, dragNodeRef as React.Ref<Element>);


does this work to remove the type assertion in line 90?

Suggested change

const nodeRefs = useCombineRefs(innerHoverRef, dragNodeRef as React.Ref<Element>);

const nodeRefs = useCombineRefs<SVGGElement>(innerHoverRef, dragNodeRef);

Yeah removing it in line 90 isn't give me errors

Leo6Leo

Thanks to @krishagarwal278 explanation on this PR. Based on my understanding, the PR is trying to solve all the errors generated after upgrading from i18next: ^21.10.0 to 23.0.0 and react-i18next: ^11.12.0 to ^13.2.2 which introduced this stricter typing policy and enabling the strictNullChecks compiler option.

After I enable the compiler option strictNullChecks: true, and complete the upgrade with the main branch code, there are errors shows up; but on this PR branch, no errors are observed after running yarn install.

Since I am not an React expert, I couldn't provide any insightful suggestions regarding how to improve code quality or whether it is adopting the best practices. But from my reading through the PR, all the changes make sense to me, and

There are no breaking functional changes that could break the whole application
You are adding more type safety

Therefore, I will go ahead and give lgtm for this PR after seeing the most of the CI tests are passing and the review comments are addressed.

Leo6Leo · 2025-08-21T20:49:06Z

frontend/packages/helm-plugin/src/components/details-page/overview/HelmReleaseOverview.tsx

@@ -10,7 +10,7 @@ import HelmChartSummary from './HelmChartSummary';

 export interface HelmReleaseOverviewProps {
  obj: K8sResourceKind;
-  customData: HelmRelease;
+  customData?: HelmRelease;
 }

 const HelmReleaseOverview: React.FC<HelmReleaseOverviewProps> = ({ obj, customData }) => {


Do we need to update the FC to FCC, as I prob see @logonoff has suggested in some other PRs before.

addressed nit

…-provider.spec.ts

Leo6Leo · 2025-08-27T14:48:53Z

LGTM! Since I am not the React expert, I will leave this PR for others to take a second look.

logonoff · 2025-08-27T17:54:11Z

frontend/packages/helm-plugin/src/components/forms/install-upgrade/HelmInstallUpgradePage.tsx

    chartName,
    chartRepoName: helmChartRepoName || '',
    appVersion,
    chartVersion,
    chartReadme,
    yamlData: initialYamlData,
-    formData: initialFormData,
-    formSchema: initialFormSchema,
+    formData: initialFormData as Record<string, unknown>,


the type of formData is typically any in HelmInstallUpgradeFormData, should we be updating that to Record<string, unknown>?

yes we can revert back to any in HelmInstallUpgradeFormData without getting any errors

logonoff · 2025-08-27T17:55:42Z

frontend/packages/helm-plugin/src/components/list-page/RepositoriesListPage.tsx

+    let res: {
+      prop: string;
+      kind: string;
+      namespaced: boolean;
+      isList: boolean;
+      namespace?: string;
+    }[] = [];


maybe we can deduplicate the type

Suggested change

let res: {

prop: string;

kind: string;

namespaced: boolean;

isList: boolean;

namespace?: string;

}[] = [];

let res: MultiListPageProps['resources'][] = [];

logonoff · 2025-08-27T17:57:01Z

frontend/packages/helm-plugin/src/components/list-page/RepositoriesListPage.tsx


-  const flatten = (resourceLists) => {
+  const flatten = (resourceLists: Record<string, { data?: unknown[] }>) => {


if we do this, do we need the assertion in line 128? at least this way TS will check if flatten is a Flatten

Suggested change

const flatten = (resourceLists: Record<string, { data?: unknown[] }>) => {

const flatten: Flatten = (resourceLists: Record<string, { data?: unknown[] }>) => {

logonoff · 2025-08-27T17:58:44Z

frontend/packages/helm-plugin/src/providers/useHelmChartRepositoriesBreadcrumbs.ts

@@ -18,7 +18,7 @@ export const useHelmChartRepositoriesBreadcrumbs = (kindObj: K8sKind) => {
  return useTabbedTableBreadcrumbsFor(
    kindObj,
    location,
-    params,
+    params as { string },


maybe we can update the expected params type in useTabbedTableBreadcrumbsFor instead because this is the only place where that function is used I think

made params required in useTabbedTableBreadcrumbsFor

logonoff · 2025-08-27T18:00:22Z

frontend/packages/helm-plugin/src/topology/helm-data-transformer.ts

+          typedDataModel.nodes?.push(
            getTopologyNodeItem(resource, TYPE_HELM_WORKLOAD, data, WorkloadModelProps),
          );
          const groups = getTopologyHelmReleaseGroupItem(resource, helmResourcesMap, secrets);
-          mergeGroups(groups, typedDataModel.nodes);
+          mergeGroups(groups, typedDataModel.nodes as NodeModel[]);


node is always defined, do you think it would be nicer to do this in line 121, then remove the ?.push and the as NodeModel[]?

const typedDataModel: Required<Omit<Model, 'graph'>> = {

yes it's cleaner that way. Removed ?. and as NodeModel[]

logonoff · 2025-08-27T18:00:56Z

frontend/packages/helm-plugin/src/utils/helm-utils.ts

@@ -253,8 +259,8 @@ export const getHelmActionConfig = (
          ),
        },
        helmReleaseApi: `/api/helm/chart?url=${encodeURIComponent(
-          chartURL,
-        )}&namespace=${namespace}&indexEntry=${encodeURIComponent(chartIndexEntry)}`,
+          chartURL as string,


should chartURL be a required parameter?

logonoff · 2025-08-27T18:01:02Z

frontend/packages/helm-plugin/src/utils/helm-utils.ts

-          chartURL,
-        )}&namespace=${namespace}&indexEntry=${encodeURIComponent(chartIndexEntry)}`,
+          chartURL as string,
+        )}&namespace=${namespace}&indexEntry=${encodeURIComponent(chartIndexEntry as string)}`,


should chartIndexEntry be required?

logonoff · 2025-08-27T18:01:27Z

frontend/packages/helm-plugin/src/utils/helm-utils.ts

@@ -272,7 +278,7 @@ export const getHelmActionConfig = (
        },
        helmReleaseApi: `/api/helm/release?ns=${namespace}&name=${releaseName}`,
        fetch: coFetchJSON.put,
-        redirectURL: getOriginRedirectURL(actionOrigin, namespace, releaseName),
+        redirectURL: getOriginRedirectURL(actionOrigin as string, namespace, releaseName),


should these be required too?

replaced with either fallback value(includes undefined too in cases where it's required) or made it required instead of using as

krishagarwal278 · 2025-08-27T18:04:10Z

/label acknowledge-critical-fixes-only

logonoff

/lgtm

openshift-ci · 2025-08-27T19:28:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: krishagarwal278, logonoff

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~frontend/OWNERS~~ [logonoff]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

krishagarwal278 · 2025-08-28T14:57:15Z

/retest

spadgett · 2025-08-28T15:01:26Z

frontend/packages/console-dynamic-plugin-sdk/src/extensions/console-types.ts

@@ -697,7 +697,7 @@ export type ResourceEventStreamProps = {
 };

 export type TimestampProps = {
-  timestamp: string | number | Date;
+  timestamp: string | number | Date | null;


I think this is reasonable since the component clearly handles this case. We should update the documentation for the component to discuss what happens if null is passed.

spadgett · 2025-08-28T15:02:19Z

frontend/packages/insights-plugin/src/components/InsightsPopup/index.tsx

 };

 const LabelComponent = ({ clusterID, ...props }) => (
  <ExternalLink
    href={`https://console.redhat.com/openshift/insights/advisor/clusters/${clusterID}?total_risk=${
-      riskSorting[props.datum.id] + 1
+      riskSorting[props.datum?.id as string] + 1


I see quite a few uses of as in the PR. It usually indicates that there is a problem with our type declarations. It would be better to fix that in most cases.

openshift-ci · 2025-08-28T15:59:47Z

New changes are detected. LGTM label has been removed.

openshift-ci · 2025-08-28T15:59:53Z

@krishagarwal278: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/frontend	`aa6ab2b`	link	true	`/test frontend`

Full PR test history. Your PR dashboard.

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Aug 12, 2025

openshift-ci bot requested review from Lucifergene and Mylanos August 12, 2025 20:30

openshift-ci bot added component/helm Related to helm-plugin component/insights Related to insights plugin component/topology Related to topology labels Aug 12, 2025

openshift-ci bot added docs-approved Signifies that Docs has signed off on this PR px-approved Signifies that Product Support has signed off on this PR labels Aug 12, 2025

openshift-ci bot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Aug 12, 2025

openshift-ci bot assigned jhadvig and yapei Aug 12, 2025

krishagarwal278 force-pushed the CONSOLE-4677 branch from 079c3de to 8a2479e Compare August 13, 2025 17:11

openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Aug 14, 2025

logonoff reviewed Aug 19, 2025

View reviewed changes

krishagarwal278 requested a review from logonoff August 21, 2025 19:48

logonoff approved these changes Aug 21, 2025

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 21, 2025

krishagarwal278 requested a review from logonoff August 21, 2025 20:27

openshift-ci bot added component/sdk Related to console-plugin-sdk plugin-api-changed Categorizes a PR as containing plugin API changes labels Aug 21, 2025

openshift-ci bot assigned spadgett Aug 21, 2025

openshift-ci bot assigned vojtechszocs Aug 21, 2025

Leo6Leo reviewed Aug 21, 2025

View reviewed changes

krishagarwal278 force-pushed the CONSOLE-4677 branch from 77aded2 to 9a4a109 Compare August 25, 2025 22:36

krishagarwal278 added 4 commits August 25, 2025 18:37

Fixed SNC errors in helm and insights plugin

578a1fc

Fixed frontend tests. Replaced undefined with false in helm-detection…

8b04237

…-provider.spec.ts

Addressed PR reviews. Removed 'any' and 'as'

f1111a4

Addressed jackson's PR reviews

1e4a82a

krishagarwal278 force-pushed the CONSOLE-4677 branch from 9a4a109 to 1e4a82a Compare August 25, 2025 22:37

Addressed small nit

46b78b9

krishagarwal278 requested a review from Leo6Leo August 26, 2025 17:30

logonoff reviewed Aug 27, 2025

View reviewed changes

openshift-ci bot added the acknowledge-critical-fixes-only Indicates if the issuer of the label is OK with the policy. label Aug 27, 2025

Addressed PR reviews

aa6ab2b

openshift-ci bot added the component/shared Related to console-shared label Aug 27, 2025

logonoff approved these changes Aug 27, 2025

View reviewed changes

openshift-ci bot assigned logonoff Aug 27, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 27, 2025

spadgett reviewed Aug 28, 2025

View reviewed changes

Code cleanup after Sam's review

a1227b0

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 28, 2025

	<DocumentTitle>{config?.title \|\| ''}</DocumentTitle>
	{config?.title && <DocumentTitle>{config.title}</DocumentTitle> }

	const nodeRefs = useCombineRefs(innerHoverRef, dragNodeRef as React.Ref<Element>);
	const nodeRefs = useCombineRefs<SVGGElement>(innerHoverRef, dragNodeRef);


		const flatten = (resourceLists) => {
		const flatten = (resourceLists: Record<string, { data?: unknown[] }>) => {

	const flatten = (resourceLists: Record<string, { data?: unknown[] }>) => {
	const flatten: Flatten = (resourceLists: Record<string, { data?: unknown[] }>) => {

CONSOLE-4677: Enable strictNullChecks: true for frontend/insights-plugin and frontend/helm-plugin #15396

Are you sure you want to change the base?

CONSOLE-4677: Enable strictNullChecks: true for frontend/insights-plugin and frontend/helm-plugin #15396

Conversation

krishagarwal278 commented Aug 12, 2025

Uh oh!

openshift-ci-robot commented Aug 12, 2025 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

krishagarwal278 commented Aug 12, 2025

Uh oh!

krishagarwal278 commented Aug 12, 2025

Uh oh!

krishagarwal278 commented Aug 12, 2025

Uh oh!

yanpzhan commented Aug 14, 2025

Uh oh!

openshift-ci-robot commented Aug 14, 2025 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

krishagarwal278 commented Aug 14, 2025

Uh oh!

jhadvig commented Aug 15, 2025

Uh oh!

krishagarwal278 commented Aug 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

logonoff left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Leo6Leo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Leo6Leo commented Aug 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

openshift-ci-robot commented Aug 12, 2025 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Aug 14, 2025 •

edited by openshift-ci bot

Loading

krishagarwal278 commented Aug 15, 2025 •

edited

Loading

krishagarwal278 Aug 27, 2025 •

edited

Loading