OCPBUGS-77415: Fix infinite recursion in project access form#16069
OCPBUGS-77415: Fix infinite recursion in project access form#16069rhamilto wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
jira/severity-moderate
|
@rhamilto: This pull request references Jira Issue OCPBUGS-77415, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/invalid-bug
openshift-ci
bot
added
component/dev-console
|
/cherry-pick release-4.22 release-4.21 release-4.20 release-4.19 |
|
@rhamilto: once the present PR merges, I will cherry-pick it on top of DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@rhamilto: This pull request references Jira Issue OCPBUGS-77415, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/cherry-pick cancel |
|
@rhamilto: once the present PR merges, I will cherry-pick it on top of DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/cherry-pick release-4.21 release-4.20 release-4.19 |
|
@rhamilto: once the present PR merges, I will cherry-pick it on top of DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
| actions.resetForm({ status: { success: null }, values: initialValues }); | ||
| const handleReset = (values: FormikValues, actions: FormikHelpers<FormikValues>) => { | ||
| actions.setStatus({ success: null }); | ||
| actions.setValues(initialValues.projectAccess ? initialValues : values); |
There was a problem hiding this comment.
nit: Would initialValues.projectAccess be falsy given line 61 sets it explicitly? The fallback to values looks unreachable.
rhamilto
force-pushed
the
OCPBUGS-77415
branch
from
e134ca0 to
cb1eaad
Compare
|
@rhamilto: This pull request references Jira Issue OCPBUGS-77415, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
📝 WalkthroughWalkthroughChanges span three files. In 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/packages/dev-console/src/components/project-access/ProjectAccess.tsx (1)
115-118: UseresetForm()instead ofsetValues()to properly reset the form baseline and clear interaction state.Line 117 uses
setValues(initialValues), which only updates current values without resetting theinitialValuesbaseline used for dirty tracking. This leavesdirty,touched, anderrorsstates uncleared—stale validation messages may still appear after reset. ThehandleSubmitalready uses the correct pattern (resetForm()) on success; apply it here too for consistency:actions.resetForm({ values: initialValues, status: { success: null } })This ensures the baseline, dirty flag, and interaction state are all properly reset together, as expected in a true reload/reset UX.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/dev-console/src/components/project-access/ProjectAccess.tsx` around lines 115 - 118, The handleReset function currently calls actions.setValues(initialValues), which only updates current values and leaves the Formik baseline and interaction state (dirty, touched, errors) unchanged; replace that call with actions.resetForm({ values: initialValues, status: { success: null } }) so the form's initialValues baseline, dirty/touched state and status are all reset together—update the handleReset implementation (referencing handleReset, initialValues, and the Formik actions object) to use resetForm instead of setValues.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@frontend/packages/dev-console/src/components/project-access/ProjectAccess.tsx`:
- Around line 159-163: ProjectAccess passes a new inline object for the
roleBindings prop on every render which retriggers ProjectAccessForm's
useEffect; fix by memoizing that object: create a memo (e.g., using
React.useMemo) that returns { projectAccess: userRoleBindings } with
[userRoleBindings] as the dependency and pass the memoized value to the
ProjectAccessForm roleBindings prop (replace the inline literal currently passed
on ProjectAccessForm).
---
Nitpick comments:
In
`@frontend/packages/dev-console/src/components/project-access/ProjectAccess.tsx`:
- Around line 115-118: The handleReset function currently calls
actions.setValues(initialValues), which only updates current values and leaves
the Formik baseline and interaction state (dirty, touched, errors) unchanged;
replace that call with actions.resetForm({ values: initialValues, status: {
success: null } }) so the form's initialValues baseline, dirty/touched state and
status are all reset together—update the handleReset implementation (referencing
handleReset, initialValues, and the Formik actions object) to use resetForm
instead of setValues.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
frontend/packages/console-shared/src/hooks/formik-validation-fix.tsfrontend/packages/dev-console/src/components/project-access/ProjectAccess.tsxfrontend/packages/dev-console/src/components/project-access/ProjectAccessForm.tsx
frontend/packages/dev-console/src/components/project-access/ProjectAccess.tsx
Show resolved
Hide resolved
This fixes a "Maximum call stack size exceeded" error that occurs when clicking "Add access" in the project access form. Changes: - Remove validateForm from useFormikValidationFix dependencies to prevent infinite re-render loops when field arrays change - Fix handleReset to avoid calling resetForm() recursively by using setStatus() and setValues() directly - Fix stale data detection by comparing roleBindings against initialValues instead of current values, and pass live userRoleBindings to ProjectAccessForm Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
rhamilto
force-pushed
the
OCPBUGS-77415
branch
from
cb1eaad to
22cf7b6
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@frontend/packages/dev-console/src/components/project-access/ProjectAccess.tsx`:
- Around line 126-129: handleReset currently only resets status and values but
leaves Formik validation metadata (errors/touched) which causes stale error UI;
update handleReset (the function named handleReset with signature (_values:
FormikValues, actions: FormikHelpers<FormikValues>)) to also clear validation
state by calling actions.setErrors({}) and actions.setTouched({}), and
optionally actions.setSubmitting(false) to ensure the form UI and submit state
are fully reset.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
frontend/packages/console-shared/src/hooks/formik-validation-fix.tsfrontend/packages/dev-console/src/components/project-access/ProjectAccess.tsxfrontend/packages/dev-console/src/components/project-access/ProjectAccessForm.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/packages/dev-console/src/components/project-access/ProjectAccessForm.tsx
| const handleReset = (_values: FormikValues, actions: FormikHelpers<FormikValues>) => { | ||
| actions.setStatus({ success: null }); | ||
| actions.setValues(initialValues); | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "package.json" -o -name "package-lock.json" | head -5Repository: openshift/console
Length of output: 276
🌐 Web query:
Formik resetForm() vs setValues() formik documentation touched errors submitCount
💡 Result:
resetForm() vs setValues() (Formik)
resetForm(nextState?)
Use when you want to re-initialize / “start over” the form state.
- Resets the form back to its initial state when called with no args. (formik.org)
- If you pass
nextState, Formik uses it to update form state, and also treatsnextState.valuesas the new base “initial state” (updatinginitialValuesand relatedinitialTouched,initialErrors,initialStatus). (formik.org) nextStatecan include:values,errors,touched,isSubmitting,isValidating,status,submitCount. (formik.org)
Practical effect: clears/overwrites touched and errors (and can reset submitCount) because you’re resetting the whole Formik state, not just values. (formik.org)
setValues(values, shouldValidate?)
Use when you only want to programmatically replace the current field values.
- Sets
valuesimperatively; does not say it resetstouched,errors, orsubmitCount. (formik.org) - By default it will trigger validation if
validateOnChangeistrue; passshouldValidate = falseto skip. (formik.org)
Practical effect: keeps touched, errors, submitCount as-is unless validation changes errors due to the new values. (formik.org)
How touched, errors, submitCount behave around submit/reset
- On submit (
handleSubmit/submitForm), Formik will:- Touch all fields
- set
isSubmitting = true - increment
submitCount(formik.org)
- If validation finds errors, submission aborts and Formik sets
errorsand flipsisSubmittingback tofalse. (formik.org) submitCountincreases whenhandleSubmitis called and resets afterhandleReset(which resets the form to initial state). (formik.org)
Rule of thumb
- Use
resetForm()when you want a true reset / reinitialize (includingtouched/submitCount, and optionally redefining “initial” values). (formik.org) - Use
setValues()when you want to update values in place without resetting the rest of Formik’s state. (formik.org)
Citations:
- 1: https://formik.org/docs/api/formik
- 2: https://formik.org/docs/api/formik
- 3: https://formik.org/docs/api/formik
- 4: https://formik.org/docs/api/formik
- 5: https://formik.org/docs/api/formik
- 6: https://formik.org/docs/api/formik
- 7: https://formik.org/docs/api/formik
- 8: https://formik.org/docs/guides/form-submission
- 9: https://formik.org/docs/guides/form-submission
- 10: https://formik.org/docs/api/formik
- 11: https://formik.org/docs/api/formik
- 12: https://formik.org/docs/api/formik
🏁 Script executed:
cat -n frontend/packages/dev-console/src/components/project-access/ProjectAccess.tsx | sed -n '100,150p'Repository: openshift/console
Length of output: 2233
🏁 Script executed:
grep -A 2 -B 2 '"formik"' frontend/package.jsonRepository: openshift/console
Length of output: 192
🏁 Script executed:
cat -n frontend/packages/dev-console/src/components/project-access/ProjectAccess.tsx | sed -n '1,100p'Repository: openshift/console
Length of output: 4289
🏁 Script executed:
grep -E "(touched|errors|getFieldMeta|error|className.*error)" frontend/packages/dev-console/src/components/project-access/ProjectAccess.tsx | head -20Repository: openshift/console
Length of output: 43
🏁 Script executed:
find . -name "ProjectAccessForm.tsx" -o -name "ProjectAccessForm.ts" -o -name "ProjectAccessForm.jsx"Repository: openshift/console
Length of output: 144
🏁 Script executed:
cat -n frontend/packages/dev-console/src/components/project-access/ProjectAccessForm.tsx | head -150Repository: openshift/console
Length of output: 4862
🏁 Script executed:
grep -E "(touched|errors|getFieldMeta|hasError|error)" frontend/packages/dev-console/src/components/project-access/ProjectAccessForm.tsx | head -15Repository: openshift/console
Length of output: 183
Reset handler should clear Formik validation metadata to prevent stale error UI.
The handleReset() method only resets values and status, but leaves the errors and touched state intact. Since the form displays validation errors and uses the errors object to control submit state (line 65), users will see stale validation messages even after clicking reset.
Suggested fix
const handleReset = (_values: FormikValues, actions: FormikHelpers<FormikValues>) => {
- actions.setStatus({ success: null });
- actions.setValues(initialValues);
+ actions.setStatus(null);
+ actions.setErrors({});
+ actions.setTouched({});
+ actions.setValues(initialValues, false);
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleReset = (_values: FormikValues, actions: FormikHelpers<FormikValues>) => { | |
| actions.setStatus({ success: null }); | |
| actions.setValues(initialValues); | |
| }; | |
| const handleReset = (_values: FormikValues, actions: FormikHelpers<FormikValues>) => { | |
| actions.setStatus(null); | |
| actions.setErrors({}); | |
| actions.setTouched({}); | |
| actions.setValues(initialValues, false); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@frontend/packages/dev-console/src/components/project-access/ProjectAccess.tsx`
around lines 126 - 129, handleReset currently only resets status and values but
leaves Formik validation metadata (errors/touched) which causes stale error UI;
update handleReset (the function named handleReset with signature (_values:
FormikValues, actions: FormikHelpers<FormikValues>)) to also clear validation
state by calling actions.setErrors({}) and actions.setTouched({}), and
optionally actions.setSubmitting(false) to ensure the form UI and submit state
are fully reset.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cajieh, rhamilto The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
rhamilto
force-pushed
the
OCPBUGS-77415
branch
from
22cf7b6 to
9d0b506
Compare
|
New changes are detected. LGTM label has been removed. |
rhamilto
force-pushed
the
OCPBUGS-77415
branch
from
9d0b506 to
22cf7b6
Compare
|
@cajieh, mind retagging? Claude just forced an errant push. |
|
@rhamilto: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
Now user is able to view and edit Project access settings succesfully project-access.movHowever I'm a little curious why there is an initial kubeadmin-record.movIf I'm logging in as other cluster-admin user, default list is empty |
5 participants
Summary
Fixes a "Maximum call stack size exceeded" error that occurs when clicking "Add access" in the project access form.
After
Screen.Recording.2026-02-26.at.8.59.26.AM.mov
Changes
validateFormfromuseEffectdependencies to prevent infinite re-render loops when field arrays changehandleResetto avoid callingresetForm()recursively by usingsetStatus()andsetValues()directlyroleBindingsagainstinitialValuesinstead of currentvaluesuserRoleBindingstoProjectAccessFormso stale detection works correctlyRoot Cause
The error occurred due to three separate issues:
validateFormin the dependency array ofuseFormikValidationFixcaused validation loops when field arrays changedhandleResetcalledactions.resetForm()which triggered theonResetcallback recursivelyTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Refactor