CONSOLE-4712: Remove PromiseComponent and convert its dependent components to functional components #15751
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@krishagarwal278: This pull request references CONSOLE-4712 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:
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
WalkthroughMultiple class components built on a shared PromiseComponent were converted to functional React components using a new usePromiseHandler hook; PromiseComponent and its tests were removed. The Environment editor and configure-ns-pull-secret modal were reimplemented in TypeScript. Several lazy import paths and an i18n entry were updated. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
openshift-ci
bot
added
component/core
|
/jira refresh |
|
@krishagarwal278: This pull request references CONSOLE-4712 which is a valid jira issue. 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
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/public/components/modals/configure-ns-pull-secret-modal.jsx (2)
315-318: Remove unusedpullSecretprop from PropTypes and invocation.Verification confirms the
pullSecretprop is declared in PropTypes but never destructured or used in the component. It is explicitly passed asundefinedat the call site infrontend/public/components/namespace.jsx:907. Remove:
- The
pullSecret: PropTypes.object,line from PropTypes (line 317)- The
pullSecret: undefinedargument from the invocation innamespace.jsx:907
30-33: Fix auth token generation to include username when required by Docker registry format.Docker registry authentication requires base64-encoding the format "username:password". Lines 30-33 allow empty or whitespace-only usernames to produce an auth token containing only the password, which violates this requirement and would generate invalid credentials.
Ensure the code always includes the username in the
authPartsarray, or validate that empty usernames are explicitly rejected with appropriate user feedback.
🧹 Nitpick comments (3)
frontend/packages/topology/src/components/modals/MoveConnectionModal.tsx (1)
52-100: Avoid mutating Formikvaluesdirectly inonSelect
onSelectassignsvalues.target = value;directly, relying onsetOpento trigger a re-render. This works today but it bypasses Formik’s change tracking and can be fragile if the form grows or validation is added.Consider using Formik helpers instead (e.g.,
setFieldValue('target', value)) so dirty state and validation remain reliable:-const MoveConnectionForm: React.FC<FormikProps<FormikValues> & MoveConnectionModalProps> = ({ - handleSubmit, - isSubmitting, - cancel, - values, +const MoveConnectionForm: React.FC<FormikProps<FormikValues> & MoveConnectionModalProps> = ({ + handleSubmit, + isSubmitting, + cancel, + values, + setFieldValue, @@ - onSelect={(_, value: Node) => { - if (value) { - values.target = value; - } + onSelect={(_, value: Node) => { + if (value) { + setFieldValue('target', value); + } setOpen(false); }}This keeps the form more maintainable and aligned with Formik best practices.
frontend/packages/console-shared/src/components/modals/DeleteResourceModal.tsx (1)
74-101: Clearstatus.submitErrorbefore new submissions to allow clean retries
handleSubmitsetsactions.setStatus({ submitError: errorMessage });on failure, andDeleteResourceFormdisables the submit button whenstatus.submitErroris truthy. Since the status is never cleared on a new attempt, a previous error can permanently disable the submit button until the modal is closed.You can clear the status at the start of
handleSubmitto make retries work cleanly:- const handleSubmit = (values: FormikValues, actions) => { - const { onSubmit, close, redirect } = props; + const handleSubmit = (values: FormikValues, actions) => { + const { onSubmit, close, redirect } = props; + actions.setStatus({ submitError: null }); @@ - handlePromise(onSubmit(values)) + handlePromise(onSubmit(values)) .then(() => { close(); redirect && history.push(redirect); }) .catch((errorMessage) => { actions.setStatus({ submitError: errorMessage }); })This keeps the UX consistent while preserving the new promise-handling flow.
frontend/public/components/environment.jsx (1)
445-459:staleflag can never becometrue, so “stale data” messaging is effectively dead
staleis initialized tofalseand only ever reset tofalseinreloadandsaveChanges, but there is no code path that callssetStale(true). As a result, the “information on this page is no longer current”<Alert>will never appear.If we still need stale-resource warnings (e.g., when the backing resource changes while editing), we should either:
- Reintroduce whatever logic previously set
staletotrue(for example, in auseEffectresponding to updated props), or- Remove the
stalestate and associated alert to avoid carrying dead UI.Right now this is just unused complexity and a hidden UX regression if stale detection used to work.
📜 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
📒 Files selected for processing (9)
frontend/packages/console-shared/src/components/modals/DeleteResourceModal.tsx(3 hunks)frontend/packages/topology/src/components/modals/EditApplicationModal.tsx(2 hunks)frontend/packages/topology/src/components/modals/MoveConnectionModal.tsx(4 hunks)frontend/public/components/environment.jsx(5 hunks)frontend/public/components/modals/configure-ns-pull-secret-modal.jsx(4 hunks)frontend/public/components/modals/confirm-modal.tsx(1 hunks)frontend/public/components/utils/__tests__/promise-component.spec.tsx(0 hunks)frontend/public/components/utils/index.tsx(0 hunks)frontend/public/components/utils/promise-component.tsx(0 hunks)
💤 Files with no reviewable changes (3)
- frontend/public/components/utils/index.tsx
- frontend/public/components/utils/promise-component.tsx
- frontend/public/components/utils/tests/promise-component.spec.tsx
🧰 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/public/components/modals/confirm-modal.tsxfrontend/packages/topology/src/components/modals/MoveConnectionModal.tsxfrontend/public/components/environment.jsxfrontend/packages/topology/src/components/modals/EditApplicationModal.tsxfrontend/public/components/modals/configure-ns-pull-secret-modal.jsxfrontend/packages/console-shared/src/components/modals/DeleteResourceModal.tsx
🔇 Additional comments (6)
frontend/packages/topology/src/components/modals/MoveConnectionModal.tsx (1)
128-176: Functional refactor and promise handling look solidThe conversion of
MoveConnectionModalto a functional component withusePromiseHandlerand Formik keeps the previous edge-type branching logic, correctly wires the asynconSubmitthroughhandlePromise, and surfaces errors via Formikstatus. The hook dependencies ([edge, t]and[handlePromise, onSubmit, close]) also look appropriate.frontend/packages/topology/src/components/modals/EditApplicationModal.tsx (1)
67-103: Edit application modal hook conversion looks correctThe functional
EditApplicationModalwiresupdateResourceApplicationthroughusePromiseHandler, updates the application name/selectedKey as before, and surfaces errors viaactions.setStatus({ submitError })forEditApplicationFormto display. Dependencies forhandleSubmitand initial values are appropriate, so behavior should match the old PromiseComponent version.frontend/public/components/modals/confirm-modal.tsx (1)
25-68: ConfirmModal async flow and hook usage are consistent with previous behaviorThe new functional
ConfirmModalcorrectly usesusePromiseHandlerto driveinProgressanderrorMessage, and only callsprops.closein the.thenbranch ofhandlePromise, so the modal stays open whenexecuteFnfails. Translated fallbacks fortitle,message, and button labels are preserved while props are now optional, which is backward-compatible at runtime.frontend/public/components/environment.jsx (1)
300-459: Overall hook-based refactor ofUnconnectedEnvironmentPagelooks coherentThe functional
UnconnectedEnvironmentPagecorrectly:
- Initializes
CurrentEnvVarsonce fromrawEnvData.- Uses
usePromiseHandlerto driveinProgress/errorMessagefor the save operation.- Preserves RBAC gating via
checkEditAccessand usesreadOnly || !allowedconsistently to disable editing.- Lazily loads ConfigMaps and Secrets and handles 403s and other errors via
localErrorMessage.Aside from the specific issues called out for the owners block and
stale, the core async + state flow looks sound and maintains the environment editing behavior.frontend/public/components/modals/configure-ns-pull-secret-modal.jsx (2)
52-78: LGTM - Event handlers properly implemented.The callbacks are correctly memoized with appropriate dependencies. The file validation includes both file type checking and JSON parsing, with proper error state management.
80-130: LGTM - Submit logic correctly handles async operations.The submit handler properly orchestrates secret creation and service account patching, with correct error handling through
handlePromise. The modal only closes on success, allowing users to see errors if they occur.
frontend/public/components/modals/configure-ns-pull-secret-modal.jsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/packages/topology/src/components/modals/MoveConnectionModal.tsx (1)
43-50: GuardResourceIconkind lookup against missing node data
nodeItemcallsnode.getData().data?.kind, which will throw ifgetData()ever returnsundefinedor a non-object (e.g., for synthetic or incomplete nodes). Since this is rendered for each selectable target, a single bad node would break the modal.Consider adding optional chaining on
getData():- <span> - <span className="co-icon-space-r"> - <ResourceIcon kind={node.getData().data?.kind} /> - </span> - {node.getLabel()} - </span> + <span> + <span className="co-icon-space-r"> + <ResourceIcon kind={node.getData()?.data?.kind} /> + </span> + {node.getLabel()} + </span>
🧹 Nitpick comments (1)
frontend/public/components/environment.jsx (1)
300-456: AlignsaveChangeserror handling withusePromiseHandlercontract
saveChangesuseshandlePromise(promise).then(...)but does not attach a.catch, relying solely onerrorMessagefromusePromiseHandlerfor UI feedback:const saveChanges = React.useCallback( (e) => { e.preventDefault(); const patches = currentEnvVars.getPatches(envPath); const promise = k8sPatch(model, obj, patches); handlePromise(promise).then((res) => { setCurrentEnvVars(new CurrentEnvVars(res, currentEnvVars.isContainerArray, envPath)); setLocalErrorMessage(null); setSuccess(t('public~Successfully updated the environment variables.')); }); }, [currentEnvVars, envPath, model, obj, handlePromise, t], );In
DeleteResourceModal,handlePromiseis used with an explicit.catchto consume the rejected promise and map the error into Formik status. Here, ifhandlePromisedoes rethrow/reject (which seems likely from that usage), this call site can produce an unhandled rejection even though the hook still updateserrorMessage.To keep behavior predictable and avoid potential unhandled rejections, consider explicitly consuming the rejection, e.g.:
- handlePromise(promise).then((res) => { - setCurrentEnvVars(new CurrentEnvVars(res, currentEnvVars.isContainerArray, envPath)); - setLocalErrorMessage(null); - setSuccess(t('public~Successfully updated the environment variables.')); - }); + handlePromise(promise) + .then((res) => { + setCurrentEnvVars(new CurrentEnvVars(res, currentEnvVars.isContainerArray, envPath)); + setLocalErrorMessage(null); + setSuccess(t('public~Successfully updated the environment variables.')); + }) + .catch(() => { + // Error state already handled by usePromiseHandler (errorMessage), + // so we intentionally swallow here. + });This keeps UI behavior the same while ensuring the promise chain is always fully handled and matches the pattern already used in
DeleteResourceModal.
📜 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
📒 Files selected for processing (4)
frontend/packages/console-shared/src/components/modals/DeleteResourceModal.tsx(3 hunks)frontend/packages/topology/src/components/modals/MoveConnectionModal.tsx(5 hunks)frontend/public/components/environment.jsx(5 hunks)frontend/public/components/modals/configure-ns-pull-secret-modal.jsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/public/components/modals/configure-ns-pull-secret-modal.jsx
🧰 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/public/components/environment.jsxfrontend/packages/topology/src/components/modals/MoveConnectionModal.tsxfrontend/packages/console-shared/src/components/modals/DeleteResourceModal.tsx
🔇 Additional comments (2)
frontend/packages/topology/src/components/modals/MoveConnectionModal.tsx (1)
129-177: Functional refactor and promise/Formik wiring look solidThe conversion to a function component with
usePromiseHandlerand Formik-basedonSubmit/handleSubmitis clean and keeps behavior: edge-type switch is preserved, the form is correctly initialized fromedge.getTarget(), and submit state/error are surfaced viaisSubmittingandstatus.submitError. Dependencies for bothuseCallbackhooks are correctly specified foredge,t,handlePromise, andclose.frontend/packages/console-shared/src/components/modals/DeleteResourceModal.tsx (1)
13-101: Delete modal hook-based refactor looks correctThe migration to a functional
DeleteResourceModalusingusePromiseHandlerpreserves the original behavior:handleSubmitresetssubmitError, wrapsonSubmitin the promise handler, closes and optionally redirects on success, and feeds errors back intostatus.submitErrorforModalSubmitFooter. The Formik wiring and initial values are appropriate, and there are no obvious regression risks.
|
@krishagarwal278: This pull request references CONSOLE-4712 which is a valid jira issue. 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: This pull request references CONSOLE-4712 which is a valid jira issue. 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: This pull request references CONSOLE-4712 which is a valid jira issue. 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. |
openshift-ci
bot
added
the
kind/i18n
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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
📒 Files selected for processing (1)
frontend/public/locales/en/public.json(1 hunks)
🧰 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/public/locales/en/public.json
| "Successfully updated the environment variables.": "Successfully updated the environment variables.", | ||
| "Environment variables set from parent": "Environment variables set from parent", | ||
| "View environment for resource": "View environment for resource", | ||
| "owners:": "owners:", |
There was a problem hiding this comment.
🧩 Analysis chain
Verify removed translations are no longer referenced in refactored components.
The PR removes two translation entries: "The information on this page is no longer current." and "Click Reload to update and lose edits, or Save Changes to overwrite." These appear to be related to stale data or edit conflict scenarios, which may have been removed along with the PromiseComponent refactoring. Ensure these strings are not still referenced by any refactored components.
🏁 Script executed:
#!/bin/bash
# Description: Verify removed translation strings are not referenced in the codebase
# Search for the removed translation keys in the source code
# These strings appear related to stale data warnings and edit confirmations
echo "Searching for references to removed translations..."
rg -n --type=ts --type=tsx --type=js "The information on this page is no longer current" || true
rg -n --type=ts --type=tsx --type=js "Click Reload to update and lose edits" || true
# Also search for translation key references to these strings
rg -n --type=ts --type=tsx --type=js "stale\|reload.*edit\|lose.*edit" -A 2 -B 2 || true
echo "Search complete. If no results found above, removed translations are not used."Length of output: 214
🏁 Script executed:
#!/bin/bash
# Description: Re-verify removed translations with corrected ripgrep syntax
echo "=== Searching for removed translation string references (corrected) ==="
# Search without specifying problematic file types - broader search
rg -n "The information on this page is no longer current" --type-list | grep -i typescript || echo "Checking available types..."
rg -n "The information on this page is no longer current" 2>/dev/null || echo "No references found"
rg -n "Click Reload to update and lose edits" 2>/dev/null || echo "No references found"
echo ""
echo "=== Searching for new 'owners:' translation usage in code ==="
# Look for usage of the new "owners:" key
rg -n "owners:" --type=js --type=ts -C 2 2>/dev/null | head -20 || echo "Searching without type filters..."
rg -n "owners:" -C 2 2>/dev/null | grep -v "\.json" | head -20 || echo "No non-JSON references found"
echo ""
echo "=== Checking translation key patterns in components ==="
# Look for common translation key usage patterns
rg -n "i18n\.|t\(|translate\(" --type=ts --type=js -C 1 2>/dev/null | grep -i "stale\|reload\|owners" || echo "No matching patterns found"
echo "Done"Length of output: 17036
Localization inconsistency: removed translations exist in other language files.
The two strings removed from the English locale file still exist in all other language locale files (ja, zh, ko, fr, es at lines 541-542). This creates a mismatch where:
- English locale no longer has these translations
- Other language locales still retain them
Either these strings should be removed from ALL locale files if deprecated, or the English removal should be reverted if they're still needed. The new "owners:" translation is correctly implemented and actively used in frontend/public/components/environment.jsx.
Verify and align all locale files:
frontend/public/locales/ja/public.json:541-542frontend/public/locales/zh/public.json:541-542frontend/public/locales/ko/public.json:541-542frontend/public/locales/fr/public.json:541-542frontend/public/locales/es/public.json:541-542
🤖 Prompt for AI Agents
In frontend/public/locales/en/public.json around line 529 the key "owners:" was
removed leaving other locales (ja/zh/ko/fr/es at their lines 541-542) still
containing the old strings, creating a localization mismatch; either restore the
removed English keys to match other locales or remove the same keys from all
other locale files—open each file listed
(frontend/public/locales/ja/public.json:541-542,
frontend/public/locales/zh/public.json:541-542,
frontend/public/locales/ko/public.json:541-542,
frontend/public/locales/fr/public.json:541-542,
frontend/public/locales/es/public.json:541-542), and make them consistent with
English by reverting the deletion in en or deleting the keys in all locales,
then run a quick grep/i18n check to ensure no missing keys remain and update any
usages in frontend/public/components/environment.jsx accordingly.
|
/label px-approved |
openshift-ci
bot
added
px-approved
There was a problem hiding this comment.
Given we are already rewriting this file it's probably not much harder to just convert this over to tsx..
| @@ -1,4 +1,5 @@ | |||
| /* eslint-disable tsdoc/syntax */ | |||
| import * as React from 'react'; | |||
There was a problem hiding this comment.
No new namespaced react imports please
| @@ -1,4 +1,5 @@ | |||
| import * as _ from 'lodash-es'; | |||
| import * as React from 'react'; | |||
There was a problem hiding this comment.
No new namespaced react imports please
| @@ -1,83 +1,72 @@ | |||
| import * as React from 'react'; | |||
There was a problem hiding this comment.
No new namespaced react imports please
| btnText?: string | React.ReactNode; | ||
| btnTextKey?: string; | ||
| cancel?: () => void; | ||
| cancelText?: string | React.ReactNode; | ||
| cancelTextKey?: string; | ||
| close?: () => void; |
There was a problem hiding this comment.
Why are all of these optional now? Is this a required change to meet the AC?
There was a problem hiding this comment.
some of the props have fallbacks (and even then they were required in previous version) and others like cancel/close should be optional. I'm thinking of extending ModalComponentProps to ConfirmModal and DeleteResourceModal
|
|
||
| return ( | ||
| <Formik initialValues={initialValues} onSubmit={handleSubmit}> | ||
| {(formikProps) => <DeleteResourceForm {...formikProps} {...props} />} |
There was a problem hiding this comment.
isSubmitting still works right?
cajieh
left a comment
cajieh
left a comment
There was a problem hiding this comment.
@krishagarwal278 Overall, looks good, except for a few more nits. However, it will require further verification of the functions by QE to avoid regression.
Thank you. 👍
| render() { | ||
| const { resource } = this.props; | ||
| const application = _.get(resource, ['metadata', 'labels', 'app.kubernetes.io/part-of']); | ||
| const application = _.get(resource, ['metadata', 'labels', 'app.kubernetes.io/part-of']); |
There was a problem hiding this comment.
Wouldn't it be better to use ES6 syntax and remove the Lodash _get method?
| const application = _.get(resource, ['metadata', 'labels', 'app.kubernetes.io/part-of']); | |
| const application = resource?.metadata?.labels?.['app.kubernetes.io/part-of']; |
| @@ -1,4 +1,5 @@ | |||
| import * as _ from 'lodash-es'; | |||
| import * as React from 'react'; | |||
| import * as PropTypes from 'prop-types'; | |||
| import { Base64 } from 'js-base64'; | |||
| import { | |||
There was a problem hiding this comment.
If the previous comment on convert to "tsx" is done in this PR, then this file should be converted as well.
There was a problem hiding this comment.
yes im converting these too
| btnText?: string | React.ReactNode; | ||
| btnTextKey?: string; | ||
| cancel?: () => void; | ||
| cancelText?: string | React.ReactNode; | ||
| cancelTextKey?: string; | ||
| close?: () => void; |
| /> | ||
| ) : null; | ||
|
|
||
| const owners = _.get(obj, 'metadata.ownerReferences', []).map((o, i) => ( |
There was a problem hiding this comment.
It would be better to use ES6 syntax and remove the Lodash _get method.
| "Successfully updated the environment variables.": "Successfully updated the environment variables.", | ||
| "Environment variables set from parent": "Environment variables set from parent", | ||
| "View environment for resource": "View environment for resource", | ||
| "owners:": "owners:", |
There was a problem hiding this comment.
Hmm, it appears that the "owners:": "owners:", was missing previously. Wondering how the parser and CI i18n test could skip that previously. That's a good catch if it was the source of the issue.
| setSecrets({}); | ||
| return; | ||
| } | ||
| const envNamespace = _.get(obj, 'metadata.namespace'); |
There was a problem hiding this comment.
It would be better to use ES6 syntax and remove the Lodash _get method?
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/packages/console-shared/src/components/modals/DeleteResourceModal.tsx (1)
76-90: Guard optionalcloseprop or make it required
DeleteResourceModalPropsdeclaresclose?: () => void;, buthandleSubmitcallsclose();unconditionally after a successful delete. This is safe today when invoked only viacreateModalLauncher, but becomes a runtime pitfall if the component is ever used directly without wiringclose.Consider one of:
- const { onSubmit, close, redirect } = props; + const { onSubmit, close, redirect } = props; ... - .then(() => { - close(); + .then(() => { + close?.(); redirect && history.push(redirect); })or tightening the prop type to make
closerequired forDeleteResourceModal.This keeps current behavior but avoids a potential null/undefined call site.
📜 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
📒 Files selected for processing (14)
frontend/packages/console-shared/src/components/modals/DeleteResourceModal.tsx(3 hunks)frontend/packages/topology/src/components/modals/EditApplicationModal.tsx(3 hunks)frontend/public/components/__tests__/environment.spec.tsx(10 hunks)frontend/public/components/build.tsx(1 hunks)frontend/public/components/daemon-set.tsx(1 hunks)frontend/public/components/deployment-config.tsx(1 hunks)frontend/public/components/deployment.tsx(1 hunks)frontend/public/components/environment.tsx(1 hunks)frontend/public/components/modals/configure-ns-pull-secret-modal.tsx(1 hunks)frontend/public/components/modals/confirm-modal.tsx(1 hunks)frontend/public/components/pod.tsx(1 hunks)frontend/public/components/replicaset.jsx(1 hunks)frontend/public/components/replication-controller.jsx(1 hunks)frontend/public/components/stateful-set.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- frontend/public/components/deployment-config.tsx
- frontend/public/components/build.tsx
🧰 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/public/components/replication-controller.jsxfrontend/public/components/stateful-set.tsxfrontend/public/components/pod.tsxfrontend/public/components/__tests__/environment.spec.tsxfrontend/public/components/modals/confirm-modal.tsxfrontend/packages/topology/src/components/modals/EditApplicationModal.tsxfrontend/packages/console-shared/src/components/modals/DeleteResourceModal.tsxfrontend/public/components/modals/configure-ns-pull-secret-modal.tsxfrontend/public/components/deployment.tsxfrontend/public/components/replicaset.jsxfrontend/public/components/daemon-set.tsxfrontend/public/components/environment.tsx
🧬 Code graph analysis (3)
frontend/packages/console-shared/src/components/modals/DeleteResourceModal.tsx (1)
frontend/public/components/modals/rollback-modal.jsx (1)
handlePromise(26-26)
frontend/public/components/modals/configure-ns-pull-secret-modal.tsx (4)
frontend/public/components/factory/modal.tsx (3)
ModalTitle(94-114)ModalBody(117-121)ModalSubmitFooter(143-230)frontend/public/components/utils/file-input.tsx (1)
onFileChange(164-187)frontend/packages/console-shared/src/constants/common.ts (1)
CONST(5-9)frontend/public/models/index.ts (2)
SecretModel(509-522)ServiceAccountModel(164-177)
frontend/public/components/environment.tsx (11)
frontend/public/module/k8s/types.ts (1)
EnvVar(149-153)frontend/public/components/events.jsx (1)
obj(126-126)frontend/public/components/utils/units.js (1)
value(225-225)frontend/public/components/utils/firehose.jsx (1)
stateToProps(141-162)frontend/public/redux.ts (1)
RootState(19-27)frontend/public/module/k8s/k8s.ts (2)
referenceFor(50-65)referenceForOwnerRef(42-45)frontend/public/models/index.ts (2)
ConfigMapModel(494-507)SecretModel(509-522)frontend/packages/console-shared/src/components/loading/LoadingInline.tsx (1)
LoadingInline(4-4)frontend/packages/console-shared/src/components/loading/LoadingBox.tsx (1)
LoadingBox(10-15)frontend/public/components/utils/container-select.tsx (1)
ContainerSelect(32-97)frontend/packages/console-dynamic-plugin-sdk/src/api/common-types.ts (1)
Alert(161-164)
🔇 Additional comments (11)
frontend/public/components/modals/confirm-modal.tsx (1)
28-71: Hook-based refactor ofConfirmModallooks correctThe conversion to a functional component with
usePromiseHandlerpreserves the old behavior (submit prevents default, callsexecuteFnwithsupressNotifications: true, closes only on success, surfaces errors viaModalSubmitFooter). Prop optionality and translation handling remain consistent.frontend/public/components/environment.tsx (1)
168-306: Environment editor refactor preserves behavior and improves structureThe
CurrentEnvVarshelper plus hook-basedUnconnectedEnvironmentPage(RBAC check, configMap/Secret loading, updateEnvVars/saveChanges withusePromiseHandler) looks consistent with the legacy implementation while being more maintainable. The patch generation and create‑case dispatch logic remain coherent, and in‑progress/error wiring viahandlePromiseis sane.Also applies to: 380-536
frontend/public/components/stateful-set.tsx (1)
61-65: Environment module import path update is correctSwitching the AsyncComponent loader to
import('./environment').then((c) => c.EnvironmentPage)matches the new TypeScript environment module and should be a no-op behaviorally for the StatefulSet env tab.frontend/public/components/modals/configure-ns-pull-secret-modal.tsx (1)
35-53: Typing suggestion is valid but the error claim is incorrect for this codebaseThe review claims
config.auths[formData.address]triggers a TypeScript error under stricter compiler options, but the codebase'sfrontend/tsconfig.jsonhas"strict": false, which disablesstrictNullChecksandnoImplicitAny. Under this configuration, no error occurs.However, the suggestion to add an explicit type annotation is still good practice for clarity and future-proofing:
const config: { auths: Record<string, { auth: string; email?: string }>; } = { auths: {}, };This improves maintainability without being a required fix for the current setup.
Likely an incorrect or invalid review comment.
frontend/public/components/replicaset.jsx (1)
92-97: LGTM! Module path consolidation.The loader path update from
'./environment.jsx'to'./environment'aligns with the PR-wide refactor to consolidate environment-related modules. This change maintains the same functionality while supporting the new functional component structure.frontend/public/components/__tests__/environment.spec.tsx (1)
1-246: LGTM! Test suite properly updated for functional component API.The test updates correctly reflect the refactored component's new requirements:
- New props (
impersonate,i18n,tReady,dispatch) added consistently across all test cases- Data shape updated from plain arrays to objects with
envproperty- All assertions maintain proper test coverage
frontend/packages/topology/src/components/modals/EditApplicationModal.tsx (1)
66-102: LGTM! Excellent functional component conversion.The refactor from class-based PromiseComponent to functional component is well-executed:
usePromiseHandlerproperly replaces promise lifecycle managementuseCallbackmemoizeshandleSubmitwith correct dependencies- Optional chaining (
resource?.metadata?.labels) is cleaner than the previous lodash approach- Error handling through Formik's
status.submitErroris preservedfrontend/public/components/deployment.tsx (1)
147-152: LGTM! Module path consolidation.Consistent with the PR-wide refactor to import the environment module using the consolidated path.
frontend/public/components/replication-controller.jsx (1)
45-50: LGTM! Module path consolidation.Consistent with the PR-wide refactor to import the environment module using the consolidated path.
frontend/public/components/daemon-set.tsx (1)
101-106: LGTM! Module path consolidation.Consistent with the PR-wide refactor to import the environment module using the consolidated path.
frontend/public/components/pod.tsx (1)
442-452: LGTM! Module path consolidation.Consistent with the PR-wide refactor to import the environment module using the consolidated path.
krishagarwal278
force-pushed
the
CONSOLE-4712
branch
from
4922058 to
24f2dea
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
frontend/public/components/environment.tsx (2)
1-1: Consider removing eslint-disable directiveThe
eslint-disable tsdoc/syntaxdirective should be removed if possible. If the TSDoc syntax in the file is non-standard, consider updating the comments to conform to TSDoc standards or converting to regular comments.
693-694: Consider migrating to hooks instead of HOCsThe component uses
connectandwithTranslationHOCs. Consider migrating touseSelector/useDispatchanduseTranslationhooks for consistency with the functional component pattern, though this can be deferred.frontend/public/components/modals/configure-ns-pull-secret-modal.tsx (1)
73-95: MIME type check is unreliable and submit lacks null guardThe MIME type check at line 78 (
file.type !== 'application/json') is unreliable since browsers inconsistently set this property for.jsonfiles. SinceJSON.parsevalidation already exists (lines 86-91), the type check is redundant and may reject valid files.Additionally, if the form is submitted via Enter key (which bypasses the disabled button check),
Base64.encode(fileData)at line 104 will encode'null'as a string, corrupting the secret.Consider this fix:
const onFileChange = useCallback((event: React.ChangeEvent<HTMLInputElement>) => { setInvalidJson(false); setFileData(null); const file = event.target.files?.[0]; - if (!file || file.type !== 'application/json') { + if (!file) { - setInvalidJson(true); return; } const reader = new FileReader();And add a guard in
submit:if (method === 'upload') { + if (!fileData || invalidJson) { + setInvalidJson(true); + return; + } secretData = Base64.encode(fileData);
🧹 Nitpick comments (2)
frontend/public/components/modals/confirm-modal.tsx (1)
54-71: Consider using useTranslation hook instead of Translation render propThe
Translationrender prop pattern works but is less idiomatic than theuseTranslationhook in functional components. This could be simplified:+import { useTranslation } from 'react-i18next'; -import { Translation } from 'react-i18next'; const ConfirmModal: React.FC<ConfirmModalProps> = (props) => { const [handlePromise, inProgress, errorMessage] = usePromiseHandler(); + const { t } = useTranslation(); // ... return ( - <Translation> - {(t) => ( <form onSubmit={submit} name="form" className="modal-content"> {/* ... */} </form> - )} - </Translation> ); };frontend/public/components/__tests__/environment.spec.tsx (1)
22-34: Consider extracting common test props to reduce boilerplate.The same set of props (
impersonate,i18n,tReady,dispatch) is repeated across all 9 test cases. You could create a helper or default props object to reduce duplication:const defaultProps = { obj, model: DeploymentModel, impersonate: null, t, i18n: mockI18n, tReady: mockTReady, dispatch: mockDispatch, }; // Then in tests: renderWithProviders( <UnconnectedEnvironmentPage {...defaultProps} rawEnvData={sampleEnvData} envPath={[]} readOnly={true} />, );This would make tests more maintainable if props change again.
📜 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
📒 Files selected for processing (21)
frontend/packages/console-shared/src/components/modals/DeleteResourceModal.tsx(3 hunks)frontend/packages/topology/src/components/modals/EditApplicationModal.tsx(3 hunks)frontend/packages/topology/src/components/modals/MoveConnectionModal.tsx(5 hunks)frontend/public/components/__tests__/environment.spec.tsx(10 hunks)frontend/public/components/build.tsx(1 hunks)frontend/public/components/daemon-set.tsx(1 hunks)frontend/public/components/deployment-config.tsx(1 hunks)frontend/public/components/deployment.tsx(1 hunks)frontend/public/components/environment.jsx(0 hunks)frontend/public/components/environment.tsx(1 hunks)frontend/public/components/modals/configure-ns-pull-secret-modal.jsx(0 hunks)frontend/public/components/modals/configure-ns-pull-secret-modal.tsx(1 hunks)frontend/public/components/modals/confirm-modal.tsx(1 hunks)frontend/public/components/pod.tsx(1 hunks)frontend/public/components/replicaset.jsx(1 hunks)frontend/public/components/replication-controller.jsx(1 hunks)frontend/public/components/stateful-set.tsx(1 hunks)frontend/public/components/utils/__tests__/promise-component.spec.tsx(0 hunks)frontend/public/components/utils/index.tsx(0 hunks)frontend/public/components/utils/promise-component.tsx(0 hunks)frontend/public/locales/en/public.json(1 hunks)
💤 Files with no reviewable changes (5)
- frontend/public/components/environment.jsx
- frontend/public/components/utils/tests/promise-component.spec.tsx
- frontend/public/components/utils/promise-component.tsx
- frontend/public/components/modals/configure-ns-pull-secret-modal.jsx
- frontend/public/components/utils/index.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- frontend/public/components/daemon-set.tsx
- frontend/public/components/pod.tsx
- frontend/public/locales/en/public.json
- frontend/public/components/build.tsx
- frontend/public/components/stateful-set.tsx
- frontend/public/components/deployment-config.tsx
🧰 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/public/components/replication-controller.jsxfrontend/packages/console-shared/src/components/modals/DeleteResourceModal.tsxfrontend/packages/topology/src/components/modals/MoveConnectionModal.tsxfrontend/public/components/modals/confirm-modal.tsxfrontend/public/components/modals/configure-ns-pull-secret-modal.tsxfrontend/public/components/deployment.tsxfrontend/public/components/replicaset.jsxfrontend/public/components/environment.tsxfrontend/packages/topology/src/components/modals/EditApplicationModal.tsxfrontend/public/components/__tests__/environment.spec.tsx
🧬 Code graph analysis (4)
frontend/packages/console-shared/src/components/modals/DeleteResourceModal.tsx (1)
frontend/public/components/modals/rollback-modal.jsx (1)
handlePromise(26-26)
frontend/packages/topology/src/components/modals/MoveConnectionModal.tsx (3)
frontend/public/components/modals/rollback-modal.jsx (1)
handlePromise(26-26)frontend/packages/topology/src/const.ts (1)
TYPE_CONNECTS_TO(4-4)frontend/__mocks__/i18next.ts (1)
t(8-14)
frontend/public/components/modals/configure-ns-pull-secret-modal.tsx (3)
frontend/public/components/factory/modal.tsx (2)
ModalComponentProps(249-252)ModalSubmitFooter(143-230)frontend/packages/console-shared/src/constants/common.ts (1)
CONST(5-9)frontend/public/models/index.ts (2)
SecretModel(509-522)ServiceAccountModel(164-177)
frontend/public/components/__tests__/environment.spec.tsx (3)
frontend/packages/console-shared/src/test-utils/unit-test-utils.tsx (1)
renderWithProviders(39-56)frontend/public/components/environment.tsx (1)
UnconnectedEnvironmentPage(380-691)frontend/__mocks__/i18next.ts (1)
t(8-14)
🔇 Additional comments (13)
frontend/public/components/modals/configure-ns-pull-secret-modal.tsx (2)
145-145: Approve: Promise handling with close on successThe pattern
handlePromise(promise).then(close)correctly closes the modal only on success, while errors are displayed viaerrorMessagepassed toModalSubmitFooter.
150-330: LGTM: Modal structure and form handlingThe functional component correctly implements the modal pattern with
usePromiseHandler, conditional rendering for form/upload methods, and proper state management. The PatternFly component usage and form validation flow work correctly.frontend/public/components/environment.tsx (2)
438-470: Verify: Effect dependencies intentionally emptyThe effect runs only on mount due to the empty dependency array. If
objoraddConfigMapSecretcan change during the component's lifecycle, the effect won't re-fetch ConfigMaps/Secrets. Verify this is the intended behavior for your use case.
523-536: LGTM: Save changes with proper promise handlingThe
saveChangescallback correctly useshandlePromiseto manage the async operation. Success updates the state with the result, and errors are displayed via theerrorMessagefrom the hook.frontend/packages/console-shared/src/components/modals/DeleteResourceModal.tsx (1)
73-101: LGTM: Clean conversion to functional component with FormikThe refactor correctly integrates
usePromiseHandlerwith Formik. UsinghandlePromisefor the async operation while letting Formik manage submission state (isSubmitting) and errors (setStatus) is the right approach for Formik-based forms.frontend/packages/topology/src/components/modals/EditApplicationModal.tsx (1)
66-102: LGTM: Well-structured functional componentThe conversion properly uses
usePromiseHandlerwith Formik, wrapshandleSubmitinuseCallbackwith correct dependencies, and uses optional chaining for safe property access. The error handling viaactions.setStatusintegrates well with the form.frontend/public/components/modals/confirm-modal.tsx (1)
28-72: LGTM: Functional component conversion with proper hook usageThe component correctly uses
usePromiseHandlerfor async handling. Thesubmithandler properly callscloseonly on success, while errors are displayed viaerrorMessage. TheTranslationrender prop pattern works, thoughuseTranslationhook would be more idiomatic.frontend/packages/topology/src/components/modals/MoveConnectionModal.tsx (2)
129-178: Clean refactor to functional component with proper hook usage.The conversion from
PromiseComponentto a functional component usingusePromiseHandleris well-implemented. The memoized callbacks have correct dependency arrays, and error handling properly propagates to Formik's status for display in the footer.One minor observation:
closeis optional inMoveConnectionModalProps(line 40), but it's called unconditionally in the.then()handler at line 160. SinceMoveConnectionModalProvideralways passescloseOverlayasclose, this should be safe in practice, but you could add a guard for defensive coding.
52-61: LGTM on simplified form props.The removal of
setTranslatorprop and direct use ofuseTranslationhook within the form simplifies the component interface while maintaining the same functionality.frontend/public/components/replication-controller.jsx (1)
44-49: LGTM - Import path updated for environment module migration.The path change from
'./environment.jsx'to'./environment'aligns with the migration to the new TypeScript module atenvironment.tsx.frontend/public/components/replicaset.jsx (1)
91-96: LGTM - Consistent import path update.Same environment module path migration as other components.
frontend/public/components/deployment.tsx (1)
144-149: LGTM - Environment module import path updated.Consistent with the broader migration to the TypeScript environment module.
frontend/public/components/__tests__/environment.spec.tsx (1)
13-18: Test data and mocks properly updated for new component interface.The
sampleEnvDatashape change from an array to an object ({ env: [...] }) correctly aligns with the newCurrentEnvVarsclass expectations in the refactoredenvironment.tsx.
| k8sGet(ConfigMapModel, null, envNamespace).catch((err) => { | ||
| if (err.response.status !== 403) { | ||
| const errorMsg = err.message || t('public~Could not load ConfigMaps.'); | ||
| setLocalErrorMessage(errorMsg); | ||
| } | ||
| return { | ||
| configMaps: {}, | ||
| }; | ||
| }), | ||
| k8sGet(SecretModel, null, envNamespace).catch((err) => { | ||
| if (err.response.status !== 403) { | ||
| const errorMsg = err.message || t('public~Could not load Secrets.'); | ||
| setLocalErrorMessage(errorMsg); | ||
| } | ||
| return { | ||
| secrets: {}, | ||
| }; | ||
| }), |
There was a problem hiding this comment.
Potential null reference when accessing error response
Accessing err.response.status will throw if err.response is undefined (e.g., on network errors). Use optional chaining for safer access.
- if (err.response.status !== 403) {
+ if (err.response?.status !== 403) {Apply the same fix at line 458.
📝 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.
| k8sGet(ConfigMapModel, null, envNamespace).catch((err) => { | |
| if (err.response.status !== 403) { | |
| const errorMsg = err.message || t('public~Could not load ConfigMaps.'); | |
| setLocalErrorMessage(errorMsg); | |
| } | |
| return { | |
| configMaps: {}, | |
| }; | |
| }), | |
| k8sGet(SecretModel, null, envNamespace).catch((err) => { | |
| if (err.response.status !== 403) { | |
| const errorMsg = err.message || t('public~Could not load Secrets.'); | |
| setLocalErrorMessage(errorMsg); | |
| } | |
| return { | |
| secrets: {}, | |
| }; | |
| }), | |
| k8sGet(ConfigMapModel, null, envNamespace).catch((err) => { | |
| if (err.response?.status !== 403) { | |
| const errorMsg = err.message || t('public~Could not load ConfigMaps.'); | |
| setLocalErrorMessage(errorMsg); | |
| } | |
| return { | |
| configMaps: {}, | |
| }; | |
| }), | |
| k8sGet(SecretModel, null, envNamespace).catch((err) => { | |
| if (err.response?.status !== 403) { | |
| const errorMsg = err.message || t('public~Could not load Secrets.'); | |
| setLocalErrorMessage(errorMsg); | |
| } | |
| return { | |
| secrets: {}, | |
| }; | |
| }), |
🤖 Prompt for AI Agents
In frontend/public/components/environment.tsx around lines 448 to 465, the catch
handlers access err.response.status directly which can throw when err.response
is undefined (e.g., network errors); change those checks to use optional
chaining (err.response?.status) before comparing to 403, and keep the existing
fallback error message and setLocalErrorMessage calls; also apply the same
optional chaining fix at the corresponding handler around line 458 as requested.
24f2dea to
126333d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
frontend/public/components/__tests__/environment.spec.tsx (2)
10-12: Factor out shared env fixtures to reduce duplicationThe same
rawEnvDatashape ({ env: [{ name, value, ID }] }) is repeated across multiple tests (sampleEnvDataplus several inline literals). Centralizing this into a small helper or reusingsampleEnvData(with overridable name/value) would make tests easier to update if the env shape changes again.Example pattern:
- const sampleEnvData = { - env: [{ name: 'DATABASE_URL', value: 'postgresql://localhost:5432', ID: 0 }], - }; + const createEnvData = (name: string, value: string) => ({ + env: [{ name, value, ID: 0 }], + }); + + const sampleEnvData = createEnvData( + 'DATABASE_URL', + 'postgresql://localhost:5432', + );Then inline usages become:
- rawEnvData={{ env: [{ name: 'test', value: ':0', ID: 0 }] }} + rawEnvData={createEnvData('test', ':0')}Purely optional, but it will reduce boilerplate and future edit surface.
Also applies to: 17-17, 31-36, 49-49, 72-73, 83-88, 97-102, 126-131, 143-148
159-169: Strengthen theEnvironment Form Interfacetest with assertionsThe
Environment Form Interfacetest currently only renders<EnvironmentPage />without any expectations, so it effectively just asserts “no immediate render crash”. If you want it to guard the UI, consider asserting for at least one key label or field (e.g., a section title or known input) to make this a more meaningful regression check.frontend/packages/console-shared/src/components/modals/DeleteResourceModal.tsx (1)
12-12: Promise handling + Formik wiring look solid; consider tightening types onactionsThe refactor to
usePromiseHandlerplus returning the promise fromhandleSubmitshould keepisSubmittingand the submit footer behavior correct, and funneling errors intostatus.submitErroraligns well with the existingModalSubmitFooterAPI. No functional issues stand out here.One small maintainability improvement: explicitly type the
actionsargument to avoid an implicitanyand make available helpers clearer:import type { FormikHelpers } from 'formik'; const handleSubmit = (values: FormikValues, actions: FormikHelpers<FormikValues>) => { const { onSubmit, close, redirect } = props; actions.setStatus({ submitError: null }); return ( onSubmit && handlePromise(onSubmit(values)) .then(() => { close(); redirect && history.push(redirect); }) .catch((errorMessage) => { actions.setStatus({ submitError: errorMessage }); }) ); };Also applies to: 73-101
frontend/public/components/modals/confirm-modal.tsx (1)
54-67: Use namespaced i18n keys for default button labelsThe defaults
t('Confirm')/t('Cancel')are inconsistent with the rest of the console, which generally uses namespaced keys liket('public~Cancel')(seeModalSubmitFooter). Using the namespaced form will keep extraction and translations consistent:- submitText={btnTextKey ? t(btnTextKey) : btnText || t('Confirm')} + submitText={btnTextKey ? t(btnTextKey) : btnText || t('public~Confirm')} @@ - cancelText={cancelTextKey ? t(cancelTextKey) : cancelText || t('Cancel')} + cancelText={cancelTextKey ? t(cancelTextKey) : cancelText || t('public~Cancel')}This retains the override behavior while aligning with existing i18n conventions.
📜 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
📒 Files selected for processing (14)
frontend/packages/console-shared/src/components/modals/DeleteResourceModal.tsx(3 hunks)frontend/packages/topology/src/components/modals/EditApplicationModal.tsx(3 hunks)frontend/public/components/__tests__/environment.spec.tsx(9 hunks)frontend/public/components/build.tsx(1 hunks)frontend/public/components/daemon-set.tsx(1 hunks)frontend/public/components/deployment-config.tsx(1 hunks)frontend/public/components/deployment.tsx(1 hunks)frontend/public/components/environment.tsx(1 hunks)frontend/public/components/modals/configure-ns-pull-secret-modal.tsx(1 hunks)frontend/public/components/modals/confirm-modal.tsx(1 hunks)frontend/public/components/pod.tsx(1 hunks)frontend/public/components/replicaset.jsx(1 hunks)frontend/public/components/replication-controller.jsx(1 hunks)frontend/public/components/stateful-set.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/public/components/build.tsx
🚧 Files skipped from review as they are similar to previous changes (7)
- frontend/public/components/pod.tsx
- frontend/public/components/modals/configure-ns-pull-secret-modal.tsx
- frontend/public/components/replicaset.jsx
- frontend/public/components/replication-controller.jsx
- frontend/public/components/deployment-config.tsx
- frontend/public/components/environment.tsx
- frontend/public/components/daemon-set.tsx
🧰 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/public/components/deployment.tsxfrontend/public/components/stateful-set.tsxfrontend/packages/topology/src/components/modals/EditApplicationModal.tsxfrontend/public/components/__tests__/environment.spec.tsxfrontend/public/components/modals/confirm-modal.tsxfrontend/packages/console-shared/src/components/modals/DeleteResourceModal.tsx
🧬 Code graph analysis (3)
frontend/packages/topology/src/components/modals/EditApplicationModal.tsx (3)
frontend/public/components/modals/rollback-modal.jsx (1)
handlePromise(26-26)frontend/packages/topology/src/const.ts (1)
UNASSIGNED_KEY(36-36)frontend/packages/topology/src/utils/application-utils.ts (1)
updateResourceApplication(93-144)
frontend/public/components/modals/confirm-modal.tsx (2)
frontend/public/components/factory/modal.tsx (4)
ModalComponentProps(249-252)ModalTitle(94-114)ModalBody(117-121)ModalSubmitFooter(143-230)frontend/public/components/modals/rollback-modal.jsx (2)
handlePromise(26-26)submit(114-120)
frontend/packages/console-shared/src/components/modals/DeleteResourceModal.tsx (1)
frontend/public/components/modals/rollback-modal.jsx (1)
handlePromise(26-26)
🔇 Additional comments (11)
frontend/public/components/stateful-set.tsx (1)
58-63: EnvironmentPage loader path change is consistent with new TS environment moduleSwitching the dynamic import to
./environmentkeeps runtime behavior the same while pointing at the new TypeScript module, and theEnvironmentPagenamed export usage remains consistent with other components in the PR. No issues from a typings or runtime perspective.frontend/public/components/deployment.tsx (1)
144-149: Deployment EnvironmentPage now correctly targets the new environment moduleUpdating the AsyncComponent loader to import from
./environmentaligns this file with the newenvironment.tsxmodule and other refactored workloads, without altering the Environment tab behavior or props contract.frontend/public/components/__tests__/environment.spec.tsx (1)
3-3: EnvironmentPage import alignment looks goodSwitching to
EnvironmentPagefrom the TS module keeps the tests aligned with the new public API and avoids coupling to any old, unconnected variant. No issues from a correctness or maintainability standpoint here.frontend/packages/topology/src/components/modals/EditApplicationModal.tsx (6)
14-14: LGTM: Import added for functional refactor.The usePromiseHandler hook import correctly supports the migration from PromiseComponent to functional component pattern.
66-68: LGTM: Clean functional component declaration.The component correctly uses usePromiseHandler, extracting only handlePromise since Formik manages submission state (isSubmitting) and errors (status).
70-84: LGTM: Correct handleSubmit implementation.The submit handler properly:
- Maps UNASSIGNED_KEY to undefined for the application parameter
- Wraps the update operation with handlePromise for error handling
- Closes the modal on success and updates Formik status on error
- Uses correct useCallback dependencies
86-86: LGTM: Improved data access with optional chaining.The code now uses ES6 optional chaining instead of lodash
_.get, making it cleaner and removing an unnecessary dependency. This addresses the previous review feedback.
88-93: LGTM: Correct initialValues setup.The initial values properly default to UNASSIGNED_KEY when no application is set, consistent with the form's dirty-check logic.
95-102: LGTM: Correct Formik integration.The Formik wrapper properly passes initialValues, submission handler, and all required props to the form component. The prop spreading correctly provides both Formik state and the component's required props (resource, cancel, initialApplication).
frontend/public/components/modals/confirm-modal.tsx (2)
12-26: ConfirmModalProps extension looks reasonableExtending
ModalComponentPropsand making the various text/title/message props optional aligns with how this modal is typically launched and keeps the surface flexible. No issues from a typing/maintainability perspective.
28-39: usePromiseHandler integration and functional refactor look goodThe switch from
PromiseComponenttousePromiseHandler(trackinginProgressanderrorMessage) and wiring those intoModalSubmitFootermatches existing patterns (e.g., rollback modal) and keeps the confirm modal behavior consistent while modernizing it.Also applies to: 54-71
| const submit = (event: React.FormEvent<HTMLFormElement>) => { | ||
| event.preventDefault(); | ||
|
|
||
| this.handlePromise( | ||
| this.props.executeFn(null, { | ||
| handlePromise( | ||
| props.executeFn(null, { | ||
| supressNotifications: true, | ||
| }), | ||
| ).then(this.props.close); | ||
| } | ||
| ).then(props.close); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Preserve executeFn contract by passing the submit event instead of null
executeFn is typed to optionally receive the submit event, but the implementation now always passes null. If any existing callers rely on the event (e.g., reading e.target), this silently breaks them.
Safer to keep forwarding the event and defensively guard close:
- const submit = (event: React.FormEvent<HTMLFormElement>) => {
+ const submit = (event: React.FormEvent<HTMLFormElement>) => {
event.preventDefault();
- handlePromise(
- props.executeFn(null, {
- supressNotifications: true,
- }),
- ).then(props.close);
+ handlePromise(
+ props.executeFn(event, {
+ supressNotifications: true,
+ }),
+ ).then(() => props.close && props.close());
};This keeps behavior compatible with the existing executeFn signature while still integrating with usePromiseHandler.
🏁 Script executed:
#!/bin/bash
# Check for executeFn implementations that expect a non-null event parameter.
rg -n "executeFn\(" frontend | sed -n '1,200p'Length of output: 84
I need to verify the type signatures and usage patterns. Let me search the codebase for the modal's prop types and its callers:
I need to examine the actual file to verify the types. Let me run a script to get the full content:
I need to examine the actual codebase directly. Let me get the full file content and type definitions:
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Get full content of confirm-modal.tsx
cat frontend/public/components/modals/confirm-modal.tsxLength of output: 2007
Now I need to check the ModalComponentProps type definition to assess if the close property guard is necessary:
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Find and cat the modal factory file
cat frontend/public/components/factory/modal.ts 2>/dev/null || cat frontend/public/components/factory/modal.tsx 2>/dev/null || fd -t f "modal\." frontend/public/components/factory/ | head -5Length of output: 7662
Let me verify if there are any existing callers that depend on the event parameter:
[/run_scripts]
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for places where confirmModal is used and how executeFn is provided
rg -n "confirmModal\|ConfirmModal" frontend --type=ts --type=tsx -A 5 -B 2 | head -100
Length of output: 27
Let me try a simpler search to look for executeFn usage patterns:
[/run_scripts]
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for confirmModal imports and executeFn usage
rg -n "confirmModal|executeFn" frontend --type-list | grep -i tsx
Length of output: 389
Pass the event and defensively guard the optional close property
The type signature of executeFn is (e?: React.FormEvent<EventTarget>, opts?: {...}) => Promise<any>. The optional parameter e accepts undefined or the event type, but not null. Passing null violates this type contract. Additionally, close is typed as optional (close?: () => void) in ModalComponentProps, so it must be guarded before invoking.
const submit = (event: React.FormEvent<HTMLFormElement>) => {
event.preventDefault();
handlePromise(
- props.executeFn(null, {
+ props.executeFn(event, {
supressNotifications: true,
}),
- ).then(props.close);
+ ).then(() => props.close && props.close());
};🤖 Prompt for AI Agents
In frontend/public/components/modals/confirm-modal.tsx around lines 31–39, the
submit handler passes null to executeFn (which expects e?: React.FormEvent<...>
or undefined) and calls props.close without guarding an optional property;
change the call to pass the actual event (or undefined) instead of null — e.g.,
forward the event as the first argument or omit it — and guard invocation of
close by checking it exists before calling (only invoke if props.close is
defined).
|
/retest |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontend/public/components/environment.tsx (1)
430-448: Use optional chaining forerr.response.statusaccessNetwork errors may not have an
err.responseobject, causing a TypeError when accessing.status. This was flagged in a previous review.- if (err.response.status !== 403) { + if (err.response?.status !== 403) {Apply the same fix at line 441.
📜 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
📒 Files selected for processing (1)
frontend/public/components/environment.tsx(1 hunks)
🧰 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/public/components/environment.tsx
🔇 Additional comments (1)
frontend/public/components/environment.tsx (1)
421-453: Empty dependency array may cause stale closuresThe effect calls
checkEditAccess()and fetches ConfigMaps/Secrets, but the empty dependency array means it won't re-run ifobj,model,impersonate,addConfigMapSecret, orreadOnlychange. If this is intentional (mount-only behavior), consider documenting why, as the eslint-disable suggests awareness of missing deps.Verify whether this effect should re-run when the resource object or access-related props change.
| const updateEnvVars = useCallback( | ||
| (env, i = 0, type = EnvType.ENV) => { | ||
| const currentEnv = _.cloneDeep(currentEnvVars); | ||
| currentEnv.setFormattedVars(containerType, i, type, env.nameValuePairs); | ||
| setCurrentEnvVars(currentEnv); | ||
| setSuccess(null); | ||
| _.isFunction(onChange) && onChange(currentEnv.dispatchNewEnvironmentVariables()); | ||
| }, | ||
| [currentEnvVars, containerType, onChange], | ||
| ); |
There was a problem hiding this comment.
_.cloneDeep on class instance breaks prototype methods
_.cloneDeep creates a plain object copy without preserving the prototype chain. Calling setFormattedVars on the cloned object will throw a TypeError since the method is on CurrentEnvVars.prototype, not the instance.
Consider creating a new instance instead:
const updateEnvVars = useCallback(
(env, i = 0, type = EnvType.ENV) => {
- const currentEnv = _.cloneDeep(currentEnvVars);
+ const currentEnv = new CurrentEnvVars();
+ currentEnv.currentEnvVars = _.cloneDeep(currentEnvVars.currentEnvVars);
+ currentEnv.rawEnvData = currentEnvVars.rawEnvData;
+ currentEnv.isContainerArray = currentEnvVars.isContainerArray;
+ currentEnv.isCreate = currentEnvVars.isCreate;
+ currentEnv.hasInitContainers = currentEnvVars.hasInitContainers;
currentEnv.setFormattedVars(containerType, i, type, env.nameValuePairs);
setCurrentEnvVars(currentEnv);Alternatively, add a clone() method to CurrentEnvVars to encapsulate this logic.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/public/components/environment.tsx around lines 458 to 467, the use
of _.cloneDeep on currentEnvVars strips its prototype so calling
setFormattedVars fails; replace the cloneDeep call with constructing a proper
CurrentEnvVars instance (e.g. new CurrentEnvVars(currentEnvVars) or
currentEnvVars.clone() if you add a clone method) so prototype methods are
preserved, then call setFormattedVars on that instance, setCurrentEnvVars, clear
success, and invoke onChange as before.
|
/label tide/merge-method-squash |
openshift-ci
bot
added
the
tide/merge-method-squash
krishagarwal278
force-pushed
the
CONSOLE-4712
branch
from
4130b29 to
3004a08
Compare
openshift-ci
bot
added
the
kind/cypress
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
frontend/public/components/modals/configure-ns-pull-secret-modal.tsx (1)
76-98: Harden JSON upload validation and guardfileDatain submit
onFileChangestill relies onfile.type === 'application/json', which is unreliable, andsubmitstill callsBase64.encode(fileData)without a hard guard. This can (a) reject valid.jsonfiles and (b) encode"null"if the form is submitted without a valid file.Consider this change:
const onFileChange = useCallback((event: React.ChangeEvent<HTMLInputElement>) => { setInvalidJson(false); setFileData(null); - const file = event.target.files?.[0]; - if (!file || file.type !== 'application/json') { - setInvalidJson(true); - return; - } + const file = event.target.files?.[0]; + if (!file) { + setInvalidJson(true); + return; + } const reader = new FileReader(); reader.onload = (e) => { const input = e.target?.result as string; try { JSON.parse(input); } catch (error) { setInvalidJson(true); return; } setFileData(input); }; reader.readAsText(file, 'UTF-8'); }, []); const submit = useCallback( (event: React.FormEvent<HTMLFormElement>) => { event.preventDefault(); let secretData: string; if (method === 'upload') { - secretData = Base64.encode(fileData); + if (!fileData || invalidJson) { + setInvalidJson(true); + return; + } + secretData = Base64.encode(fileData); } else {Also applies to: 100-151
frontend/public/components/modals/confirm-modal.tsx (1)
31-39: FixexecuteFncall and guard optionalclose
executeFnis typed to take an optionalReact.FormEvent(orundefined), but you’re passingnull, andcloseis optional onModalComponentPropsyet called unguarded. This breaks the type contract and is unsafe ifConfirmModalis ever used withoutclose.Suggested change:
const submit = (event: React.FormEvent<HTMLFormElement>) => { event.preventDefault(); handlePromise( - props.executeFn(null, { + props.executeFn(event, { supressNotifications: true, }), - ).then(props.close); + ).then(() => props.close && props.close()); };Please also ensure there are no callers that depend on
executeFnnot receiving the event.frontend/public/components/environment.tsx (2)
430-448: Avoid crashing whenerr.responseis undefinedBoth ConfigMap and Secret loads assume
err.responseis always present; on some failures (e.g., network errors), this will throw before you can set a friendly error message.Safen the checks with optional chaining:
k8sGet(ConfigMapModel, null, envNamespace).catch((err) => { - if (err.response.status !== 403) { + if (err.response?.status !== 403) { const errorMsg = err.message || t('public~Could not load ConfigMaps.'); setLocalErrorMessage(errorMsg); } ... k8sGet(SecretModel, null, envNamespace).catch((err) => { - if (err.response.status !== 403) { + if (err.response?.status !== 403) { const errorMsg = err.message || t('public~Could not load Secrets.'); setLocalErrorMessage(errorMsg); }
175-344: Fix_.cloneDeeponCurrentEnvVarsinstance to preserve methods
updateEnvVarsclonescurrentEnvVarswith_.cloneDeepand then callscurrentEnv.setFormattedVars(...). BecausecloneDeepreturns a plain object without theCurrentEnvVarsprototype,setFormattedVars(and other methods) are lost, leading to a runtimeTypeErrorwhen the user edits any environment value. This was already flagged previously.Introduce a proper
clone()method onCurrentEnvVarsand use it:class CurrentEnvVars { currentEnvVars: EnvVarsState; rawEnvData: RawEnvData; isContainerArray: boolean; isCreate: boolean; hasInitContainers: boolean; state: { allowed: boolean }; @@ setResultObject( resultObject: K8sResourceKind | RawEnvData, isContainerArray: boolean, path: string[], ): this { @@ } return this.setRawData(getNestedValue(resultObject, path)); } + + clone(): CurrentEnvVars { + const cloned = new CurrentEnvVars(); + cloned.currentEnvVars = _.cloneDeep(this.currentEnvVars); + cloned.rawEnvData = this.rawEnvData; + cloned.isContainerArray = this.isContainerArray; + cloned.isCreate = this.isCreate; + cloned.hasInitContainers = this.hasInitContainers; + cloned.state = { ...this.state }; + return cloned; + } @@ const updateEnvVars = useCallback( (env, i = 0, type = EnvType.ENV) => { - const currentEnv = _.cloneDeep(currentEnvVars); + const currentEnv = currentEnvVars.clone(); currentEnv.setFormattedVars(containerType, i, type, env.nameValuePairs); setCurrentEnvVars(currentEnv); setSuccess(null); _.isFunction(onChange) && onChange(currentEnv.dispatchNewEnvironmentVariables()); },This keeps the helper’s methods intact while still avoiding in-place state mutation.
Also applies to: 458-467
🧹 Nitpick comments (1)
frontend/packages/topology/src/components/modals/EditApplicationModal.tsx (1)
40-41: Aligndirtydetection with initial Formik values and verifyupdateResourceApplicationcontractFor unassigned resources,
initialApplicationisundefined, butinitialValues.application.selectedKeyisUNASSIGNED_KEY. This makesconst dirty = values?.application?.selectedKey !== initialApplication;true immediately, so Save is enabled even when nothing changed.
You can base
dirtyon the same key you use ininitialValues:- const dirty = values?.application?.selectedKey !== initialApplication; + const dirty = values?.application?.selectedKey !== initialApplication;and pass the initial selectedKey instead of the label string:
- <Formik initialValues={initialValues} onSubmit={handleSubmit}> - {(formikProps) => ( - <EditApplicationForm {...formikProps} {...props} initialApplication={application} /> - )} - </Formik> + <Formik initialValues={initialValues} onSubmit={handleSubmit}> + {(formikProps) => ( + <EditApplicationForm + {...formikProps} + {...props} + initialApplication={initialValues.application.selectedKey} + /> + )} + </Formik>This keeps dirty=false until the key actually changes (including the unassigned case).
Also, please confirm that
updateResourceApplication’sapplicationparameter is typed/implemented to acceptundefinedwhen UNASSIGNED, ashandleSubmitcurrently passesundefinedin that case.Also applies to: 86-93, 95-101
📜 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
📒 Files selected for processing (15)
frontend/packages/console-shared/src/components/modals/DeleteResourceModal.tsx(3 hunks)frontend/packages/integration-tests-cypress/tests/crud/secrets/image-pull.cy.ts(1 hunks)frontend/packages/topology/src/components/modals/EditApplicationModal.tsx(3 hunks)frontend/public/components/__tests__/environment.spec.tsx(9 hunks)frontend/public/components/build.tsx(1 hunks)frontend/public/components/daemon-set.tsx(1 hunks)frontend/public/components/deployment-config.tsx(1 hunks)frontend/public/components/deployment.tsx(1 hunks)frontend/public/components/environment.tsx(1 hunks)frontend/public/components/modals/configure-ns-pull-secret-modal.tsx(1 hunks)frontend/public/components/modals/confirm-modal.tsx(1 hunks)frontend/public/components/pod.tsx(1 hunks)frontend/public/components/replicaset.jsx(1 hunks)frontend/public/components/replication-controller.jsx(1 hunks)frontend/public/components/stateful-set.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- frontend/public/components/daemon-set.tsx
- frontend/public/components/replicaset.jsx
- frontend/public/components/deployment.tsx
- frontend/public/components/build.tsx
- frontend/public/components/pod.tsx
- frontend/public/components/stateful-set.tsx
- frontend/public/components/deployment-config.tsx
🧰 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/public/components/__tests__/environment.spec.tsxfrontend/public/components/modals/configure-ns-pull-secret-modal.tsxfrontend/public/components/environment.tsxfrontend/packages/integration-tests-cypress/tests/crud/secrets/image-pull.cy.tsfrontend/public/components/replication-controller.jsxfrontend/public/components/modals/confirm-modal.tsxfrontend/packages/topology/src/components/modals/EditApplicationModal.tsxfrontend/packages/console-shared/src/components/modals/DeleteResourceModal.tsx
🧬 Code graph analysis (5)
frontend/public/components/modals/configure-ns-pull-secret-modal.tsx (2)
frontend/packages/console-shared/src/constants/common.ts (1)
CONST(5-9)frontend/public/models/index.ts (2)
SecretModel(509-522)ServiceAccountModel(164-177)
frontend/packages/integration-tests-cypress/tests/crud/secrets/image-pull.cy.ts (1)
frontend/packages/integration-tests-cypress/support/index.ts (1)
testName(92-95)
frontend/public/components/modals/confirm-modal.tsx (2)
frontend/public/components/factory/modal.tsx (1)
ModalComponentProps(249-252)frontend/public/components/modals/rollback-modal.jsx (2)
handlePromise(26-26)submit(114-120)
frontend/packages/topology/src/components/modals/EditApplicationModal.tsx (3)
frontend/public/components/modals/rollback-modal.jsx (1)
handlePromise(26-26)frontend/packages/topology/src/const.ts (1)
UNASSIGNED_KEY(36-36)frontend/packages/topology/src/utils/application-utils.ts (1)
updateResourceApplication(93-144)
frontend/packages/console-shared/src/components/modals/DeleteResourceModal.tsx (1)
frontend/public/components/modals/rollback-modal.jsx (1)
handlePromise(26-26)
🔇 Additional comments (5)
frontend/packages/console-shared/src/components/modals/DeleteResourceModal.tsx (1)
73-101: LGTM! Clean refactoring to functional component with usePromiseHandler.The conversion correctly uses
handlePromiseto wrap theonSubmitpromise while relying on Formik's built-inisSubmittingandstatusmanagement for UI state. The error handling flow properly keeps the modal open on failure (allowing resubmission) and closes on success, which is the expected behavior.frontend/packages/integration-tests-cypress/tests/crud/secrets/image-pull.cy.ts (1)
25-25: Based on the web search results, I now have critical context for this review. The change introduces a significant test isolation risk.Key findings:
OpenShift project deletion is asynchronous:
oc delete projectsets the project to "Terminating" status; the actual deletion happens on the server asynchronously.The
--wait=falseflag is problematic: It makes the command return immediately without waiting for the deletion to complete. This is a regression from the likely behavior ofcy.deleteProjectWithCLI(testName).Cypress best practices: Tests should not rely on after() hooks for cleanup; state should be managed before tests run or in external CI teardown.
Race condition risk: Without waiting for project deletion to complete, subsequent test runs could attempt to create a project with the same name before the old one is fully deleted, causing test isolation failures.
Rewritten Review Comment:
--wait=falseflag creates test isolation risk through race condition.This change replaces
cy.deleteProjectWithCLI(testName)withcy.exec(oc delete project ${testName} --wait=false), removing the wait behavior. Since OpenShift project deletion is asynchronous (the server continues cleanup after the delete command returns),--wait=falsecauses the test to proceed immediately without confirming the project is fully deleted. If subsequent test runs begin before cleanup completes, they may fail when attempting to create a project with the same name.Additionally, this contradicts Cypress best practices which recommend avoiding after() hooks for cleanup; consider either:
- Reverting to a helper that waits for deletion (e.g.,
oc wait --for=delete namespace/${testName})- Moving cleanup outside Cypress to CI teardown
- Deleting resources inside the project rather than the entire project
Confirm whether this change was intentional and whether the custom helper provided waiting semantics that this replacement removes.
frontend/public/components/modals/configure-ns-pull-secret-modal.tsx (1)
58-61: Confirm behavior whenpullSecretalready exists
ConfigureNamespacePullSecretreceives an optionalpullSecretbut never uses it; the Secret name field is always empty and the flow unconditionally doesk8sCreate(SecretModel, secret)followed by a patch adding a newimagePullSecretsentry.If this modal was previously used to edit an existing namespace pull secret (pre-filling the name and updating in place), this is likely a behavior change that could lead to multiple secrets or create failures on name collisions.
Please verify intended behavior for the edit case and adjust to either:
- Pre-populate and update the existing Secret when
pullSecretis provided, or- Explicitly treat this as “create new” only and ensure callers don’t pass an existing
pullSecret.Also applies to: 119-146
frontend/public/components/replication-controller.jsx (1)
44-47: Environment loader path update looks correctSwitching the
EnvironmentPageloader toimport('./environment')aligns with the new TSX environment module and should preserve behavior.frontend/public/components/__tests__/environment.spec.tsx (1)
3-12: Updated EnvironmentPage tests cover key scenariosThe refactored tests correctly target the new
EnvironmentPageAPI (includingrawEnvDatashape) and cover read-only, RBAC-denied, and permitted edit flows, plus a basic render check. Looks good.Also applies to: 14-171
|
[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:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
openshift-ci-robot
added
verified-later
verified
|
@krishagarwal278: This PR has been marked to be verified later by 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. |
|
/test e2e-gcp-console |
|
@krishagarwal278: all tests passed! 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. |
| } | ||
|
|
||
| const data: { [key: string]: string } = {}; | ||
| const pullSecretName = (event.currentTarget.elements as any)['namespace-pull-secret-name'] |
There was a problem hiding this comment.
Is the as any type assertion mandatory here?
| if (method === 'upload') { | ||
| secretData = Base64.encode(fileData); | ||
| } else { | ||
| const elements = event.currentTarget.elements as any; |
There was a problem hiding this comment.
Is the as any type assertion mandatory here?
4 participants
Summary
Tech Debt
This PR aims to remove PromiseComponent and convert its dependent components to function components.
Checklist:
usePromiseHandlerfor promise action. Number of files/components: 8EditApplicationModal, MoveConnectionModal, DeleteResourceModal, ConfirmModal, ConfigureNamespacePullSecret, EnvironmentusePromiseHandlerhook provides the same promise handling behavior as the oldPromiseComponentclass