Skip to content

Conversation

@vikram-raj
Copy link
Member

Remove the use of the kebab factory from the following pages of the devconsole plugin

  • ProjectDetailsPage.tsx

Changes from the big PR #15520

@openshift-ci openshift-ci bot requested review from cajieh and jhadvig November 13, 2025 09:44
@openshift-ci openshift-ci bot added component/core Related to console core functionality component/dev-console Related to dev-console labels Nov 13, 2025
@vikram-raj vikram-raj changed the title Remove kebab factory uses from devconsole plugin CONSOLE-4709: Remove kebab factory uses from devconsole plugin Nov 13, 2025
@openshift-ci openshift-ci bot added kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 13, 2025
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Nov 13, 2025
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Nov 13, 2025

@vikram-raj: This pull request references CONSOLE-4709 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.21.0" version, but no target version was set.

In response to this:

Remove the use of the kebab factory from the following pages of the devconsole plugin

  • ProjectDetailsPage.tsx

Changes from the big PR #15520

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.

@vikram-raj
Copy link
Member Author

/cc @logonoff

@openshift-ci openshift-ci bot requested a review from logonoff November 13, 2025 09:44
@vikram-raj vikram-raj force-pushed the console-4709-dev-console branch from 0172e17 to 1f67343 Compare November 13, 2025 10:02
@vikram-raj vikram-raj changed the title CONSOLE-4709: Remove kebab factory uses from devconsole plugin CONSOLE-4709: Remove kebab factory uses from devconsole and Topology plugin Nov 13, 2025
@openshift-ci openshift-ci bot added the component/topology Related to topology label Nov 13, 2025
Copy link
Member

@logonoff logonoff left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 13, 2025
@vikram-raj
Copy link
Member Author

/label px-approved
/label docs-approved

@openshift-ci openshift-ci bot added px-approved Signifies that Product Support has signed off on this PR docs-approved Signifies that Docs has signed off on this PR labels Nov 13, 2025
@yapei
Copy link
Contributor

yapei commented Nov 14, 2025

@yanpzhan will work on PR testing

@vikram-raj
Copy link
Member Author

/retest

@vikram-raj vikram-raj force-pushed the console-4709-dev-console branch from 46e698d to 25f2eeb Compare November 17, 2025 11:48
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 17, 2025
@vikram-raj vikram-raj requested a review from logonoff November 17, 2025 14:11
@yanpzhan
Copy link
Contributor

Checked on cluster launched against the pr. Regression tests passed, there is not functional issue.
/verified by yanpzhan

@openshift-ci-robot openshift-ci-robot added the verified Signifies that the PR passed pre-merge verification criteria label Nov 17, 2025
@openshift-ci-robot
Copy link
Contributor

@yanpzhan: This PR has been marked as verified by yanpzhan.

In response to this:

Checked on cluster launched against the pr. Regression tests passed, there is not functional issue.
/verified by yanpzhan

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.

@vikram-raj vikram-raj force-pushed the console-4709-dev-console branch from 66e2eb2 to b85ba34 Compare November 17, 2025 15:33
@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD 80b3db5 and 0 for PR HEAD 8c248c8 in total

@openshift-ci-robot
Copy link
Contributor

/hold

Revision 8c248c8 was retested 3 times: holding

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 19, 2025
@vikram-raj
Copy link
Member Author

/unhold

/retest

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 19, 2025
@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD b8fa7f7 and 2 for PR HEAD 8c248c8 in total

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 19, 2025
@vikram-raj vikram-raj force-pushed the console-4709-dev-console branch from 8c248c8 to 26e45e7 Compare November 20, 2025 06:25
@openshift-ci-robot openshift-ci-robot removed the verified Signifies that the PR passed pre-merge verification criteria label Nov 20, 2025
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 20, 2025
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 20, 2025
@vikram-raj
Copy link
Member Author

resolved conflicts in i18n file console-app.json

/verified by yanpzhan

@openshift-ci-robot
Copy link
Contributor

@vikram-raj: This PR has been marked as verified by yanpzhan.

In response to this:

resolved conflicts in i18n file console-app.json

/verified by yanpzhan

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-robot openshift-ci-robot added the verified Signifies that the PR passed pre-merge verification criteria label Nov 20, 2025
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (2)
frontend/packages/dev-console/src/components/projects/details/ProjectDetailsPage.tsx (1)

19-29: ProjectDetails composition and LazyActionMenu wiring look good; consider minor typing/constant cleanup

The refactor to a dedicated ProjectDetails component, including kind={referenceForModel(ProjectModel)} and the LazyActionMenu-based customActionMenu, looks functionally sound and aligns with the new actions architecture. Likewise, handleNamespaceChange using PROJECT_DETAILS_ALL_NS_PAGE_URI + ALL_NAMESPACES_KEY is clear.

Two small maintainability tweaks you might consider:

  • Add an explicit prop type for ProjectDetails (e.g., combining MonitoringPageProps with { activeNamespace: string; pages: Page[] }) instead of leaving props as implicit any.
  • Reuse PROJECT_DETAILS_ALL_NS_PAGE_URI in the breadcrumbs instead of duplicating '/project-details/all-namespaces', so the paths stay in sync if they ever change.

Also applies to: 31-55, 99-101

frontend/public/components/modals/delete-namespace-modal.tsx (1)

30-72: Overlay-based DeleteNamespaceModal looks correct; consider simplifying the success navigation

The migration to an OverlayComponent with closeOverlay, PF Modal/ModalHeader/ModalBody/ModalFooter, and the useDeleteNamespaceModalLauncher hook preserves the original delete + confirmation + namespace reset behavior while aligning with the newer overlay pattern. The promise handling and error display via usePromiseHandler / ErrorMessage also look intact.

One minor readability optimization: in the success path of onSubmit, you navigate once via formatNamespaceRoute (inside the if (resource.metadata.name === activeNamespace) block) and then again to /k8s/cluster/${kind.plural} after closeOverlay(). Since the second navigation effectively determines the final location, you could likely drop the first navigate(newPath) (while keeping the setActiveNamespace / setLastNamespace updates) unless there is a specific intermediate routing requirement. That would make the flow easier to follow without changing observable behavior.

Also applies to: 78-121, 124-130

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between 8c248c8 and 26e45e7.

📒 Files selected for processing (12)
  • frontend/packages/console-app/console-extensions.json (1 hunks)
  • frontend/packages/console-app/locales/en/console-app.json (1 hunks)
  • frontend/packages/console-app/package.json (1 hunks)
  • frontend/packages/console-app/src/actions/providers/project-provider.ts (1 hunks)
  • frontend/packages/dev-console/locales/en/devconsole.json (1 hunks)
  • frontend/packages/dev-console/src/components/projects/details/ProjectDetailsPage.tsx (3 hunks)
  • frontend/packages/dev-console/src/components/projects/details/__tests__/ProjectDetailsPage.spec.tsx (1 hunks)
  • frontend/packages/topology/src/actions/index.ts (0 hunks)
  • frontend/packages/topology/src/actions/nodeActions.ts (0 hunks)
  • frontend/public/components/modals/delete-namespace-modal.tsx (4 hunks)
  • frontend/public/components/modals/index.ts (0 hunks)
  • frontend/public/components/namespace.jsx (6 hunks)
💤 Files with no reviewable changes (3)
  • frontend/packages/topology/src/actions/index.ts
  • frontend/packages/topology/src/actions/nodeActions.ts
  • frontend/public/components/modals/index.ts
🚧 Files skipped from review as they are similar to previous changes (5)
  • frontend/packages/console-app/console-extensions.json
  • frontend/packages/dev-console/locales/en/devconsole.json
  • frontend/packages/console-app/src/actions/providers/project-provider.ts
  • frontend/packages/dev-console/src/components/projects/details/tests/ProjectDetailsPage.spec.tsx
  • frontend/packages/console-app/locales/en/console-app.json
🧰 Additional context used
📓 Path-based instructions (1)
**

⚙️ CodeRabbit configuration file

-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.

Files:

  • frontend/packages/dev-console/src/components/projects/details/ProjectDetailsPage.tsx
  • frontend/public/components/modals/delete-namespace-modal.tsx
  • frontend/public/components/namespace.jsx
  • frontend/packages/console-app/package.json
🔇 Additional comments (3)
frontend/packages/console-app/package.json (1)

55-55: New projectProvider exposed module wiring looks consistent

The new projectProvider entry follows the existing provider naming and path conventions and should integrate cleanly with the plugin system.

frontend/public/components/namespace.jsx (2)

75-75: Pull-secret modal import change is consistent

Importing configureNamespacePullSecretModal from ./modals keeps modal-related utilities centralized and matches how other modals are typically wired in this module.


105-106: LazyActionMenu integration for namespaces/projects is coherent and aligns with provider-based actions

The new LazyActionMenu usage for:

  • namespace list rows (namespace context),
  • project list rows (project context), and
  • the NamespacesDetailsPage / ProjectsDetailsPage customActionMenu dropdowns,

all consistently key the context by referenceForModel(NamespaceModel|ProjectModel) and set kind={referenceForModel(...Model)} on the respective DetailsPages. This should play well with the new namespace/project action providers and eliminates the direct Kebab wiring.

From a readability and maintainability standpoint, centralizing actions through LazyActionMenu here looks solid; no functional issues stand out.

Also applies to: 330-331, 640-641, 1092-1100, 1113-1119

Copy link
Member

@logonoff logonoff left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 20, 2025
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 20, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: logonoff, vikram-raj

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:

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

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD 86c7b34 and 2 for PR HEAD 26e45e7 in total

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD f2eb83d and 1 for PR HEAD 26e45e7 in total

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD 057ffde and 0 for PR HEAD 26e45e7 in total

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 21, 2025

@vikram-raj: 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/e2e-gcp-console 26e45e7 link true /test e2e-gcp-console

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
Copy link
Contributor

/hold

Revision 26e45e7 was retested 3 times: holding

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 21, 2025
@vikram-raj
Copy link
Member Author

/unhold

/retest

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. component/core Related to console core functionality component/dev-console Related to dev-console component/topology Related to topology docs-approved Signifies that Docs has signed off on this PR jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated lgtm Indicates that a PR is ready to be merged. px-approved Signifies that Product Support has signed off on this PR verified Signifies that the PR passed pre-merge verification criteria

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants