fix(admin-ui): ensure confirmation popup closes after accept in apply…#2676
Merged
fix(admin-ui): ensure confirmation popup closes after accept in apply…#2676
Conversation
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRemoved audit/audit-log branches and modal toggle side-effects from multiple form submit flows; commit dialogs now await asynchronous onAccept handlers and form submissions rely on awaited handlers and store loading state for continuation. Changes
Sequence Diagram(s)sequenceDiagram
participant User as "User (Form)"
participant FormComp as "Form Component"
participant CommitDlg as "GluuCommitDialog"
participant Handler as "onAccept / Parent Handler"
participant API as "Backend/API"
User->>FormComp: Click Submit
FormComp->>CommitDlg: Open commit dialog (if required)
CommitDlg->>Handler: call onAccept(userMessage)
Handler->>API: apply patches / update ACR (async)
API-->>Handler: response
Handler-->>CommitDlg: resolve (success/failure)
CommitDlg->>CommitDlg: await result then close modal
CommitDlg-->>FormComp: finalize submit flow
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
mo-auto
added
comp-admin-ui
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
admin-ui/plugins/auth-server/components/Configuration/AuthPage.tsx (1)
292-303:⚠️ Potential issue | 🟠 MajorPrevent false success feedback when ACR update fails.
At Line 297-Line 299, ACR mutation errors are swallowed; then Line 301 can still show a success toast for ACR-only changes (
patches.length === 0). This can report success even on failed save.Proposed fix
const handleSubmit = useCallback( async (message: string) => { try { setErrorMessage(null) + let acrUpdateFailed = false if (patches.length > 0) { const postBody = { requestBody: patches, } as unknown as ActionData buildPayload(userAction, message, postBody) dispatch(patchJsonConfig({ action: userAction })) } if (put && put.value) { const newAcr = { defaultAcr: put.value || acrs?.defaultAcr } try { await putAcrsMutation.mutateAsync({ data: newAcr }) await logAcrUpdate(newAcr, message, { defaultAcr: newAcr.defaultAcr }) } catch (error) { + acrUpdateFailed = true console.error('Error updating ACR:', error) } } - if (patches.length === 0) { + if (patches.length === 0 && !acrUpdateFailed) { toast.success(t('messages.success_in_saving')) } } catch (err) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/plugins/auth-server/components/Configuration/AuthPage.tsx` around lines 292 - 303, The ACR update errors are swallowed in the try/catch around putAcrsMutation.mutateAsync and logAcrUpdate, which can still allow toast.success to run when patches.length === 0; change the flow to track the ACR update result and only show the success toast when the ACR mutation/log succeeded (or when no ACR update was attempted). Concretely, use a boolean flag (e.g., acrUpdateSucceeded) initialized true, set it to false in the catch for putAcrsMutation.mutateAsync/logAcrUpdate, and only call toast.success when patches.length === 0 && acrUpdateSucceeded; reference the existing variables/functions put, putAcrsMutation.mutateAsync, logAcrUpdate, patches, and toast.success.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@admin-ui/plugins/auth-server/components/Configuration/AuthPage.tsx`:
- Around line 292-303: The ACR update errors are swallowed in the try/catch
around putAcrsMutation.mutateAsync and logAcrUpdate, which can still allow
toast.success to run when patches.length === 0; change the flow to track the ACR
update result and only show the success toast when the ACR mutation/log
succeeded (or when no ACR update was attempted). Concretely, use a boolean flag
(e.g., acrUpdateSucceeded) initialized true, set it to false in the catch for
putAcrsMutation.mutateAsync/logAcrUpdate, and only call toast.success when
patches.length === 0 && acrUpdateSucceeded; reference the existing
variables/functions put, putAcrsMutation.mutateAsync, logAcrUpdate, patches, and
toast.success.
duttarnab
requested changes
Feb 26, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
admin-ui/plugins/fido/components/DynamicConfiguration.tsx (1)
51-58:⚠️ Potential issue | 🟠 MajorReturn the submit promise so dialog awaiting works as intended.
submitFormcurrently returnsvoid, soGluuCommitDialog’s awaitedonAcceptpath closes without actually waiting forhandleSubmitcompletion.Based on learnings: In React components using Formik, keeping the entire `formik` object in dependency arrays is the recommended pattern.Proposed fix
- const submitForm = useCallback( - (userMessage: string) => { - if (readOnly) { - return - } - handleSubmit(formik.values, userMessage) - }, - [handleSubmit, formik.values, readOnly], - ) + const submitForm = useCallback( + async (userMessage: string) => { + if (readOnly) { + return + } + await Promise.resolve(handleSubmit(formik.values, userMessage)) + }, + [handleSubmit, formik, readOnly], + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/plugins/fido/components/DynamicConfiguration.tsx` around lines 51 - 58, submitForm currently returns void which prevents GluuCommitDialog's awaited onAccept from waiting; change submitForm to return the promise from handleSubmit and ensure the readOnly branch returns a resolved Promise (e.g., Promise.resolve()) so callers can await consistently. Also update the hook dependencies to use formik (not formik.values) so useCallback tracks Formik correctly; references: submitForm, handleSubmit, formik, readOnly, and GluuCommitDialog onAccept.admin-ui/plugins/fido/components/StaticConfiguration.tsx (1)
69-76:⚠️ Potential issue | 🟠 MajorPropagate async submit completion from
submitForm.
onAcceptis now awaited by the dialog, but this callback drops thehandleSubmitresult. That prevents proper submit sequencing.Based on learnings: In React components using Formik, keeping the entire `formik` object in dependency arrays is the recommended pattern.Proposed fix
- const submitForm = useCallback( - (userMessage: string) => { - if (readOnly) { - return - } - handleSubmit(formik.values, userMessage) - }, - [handleSubmit, formik.values, readOnly], - ) + const submitForm = useCallback( + async (userMessage: string) => { + if (readOnly) { + return + } + await Promise.resolve(handleSubmit(formik.values, userMessage)) + }, + [handleSubmit, formik, readOnly], + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/plugins/fido/components/StaticConfiguration.tsx` around lines 69 - 76, Make submitForm async and return the result of handleSubmit so the dialog awaiting onAccept receives the submit completion; specifically, change submitForm to an async function that does "if (readOnly) return;" and then "return await handleSubmit(formik.values, userMessage)". Also update the hook dependency array to use formik (not formik.values) and keep handleSubmit and readOnly: [handleSubmit, formik, readOnly], so submitForm stays stable and follows Formik recommendations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@admin-ui/app/routes/Apps/Gluu/GluuCommitDialog.tsx`:
- Around line 104-111: The handleAccept callback can run multiple times
concurrently because it calls onAccept without guarding against repeated clicks;
update handleAccept to use an in-flight guard (e.g., a local isSubmitting
ref/state) so it returns early if already submitting, set the guard true before
awaiting the result of onAccept(userMessage), await the promise (after setting
formik fieldValue), then in a finally block clear the guard and call closeModal
only after the awaited onAccept completes; reference the handleAccept function,
onAccept, formik, userMessage, closeModal and ensure the guard is included in
the hook's dependency list (or useRef to avoid re-renders).
In `@admin-ui/app/routes/Apps/Gluu/GluuCommitDialogLegacy.tsx`:
- Around line 93-99: handleAccept can be invoked multiple times because there's
no in-flight guard; add a local submission guard (e.g., isSubmitting boolean
state or ref) inside handleAccept to early-return if already true, set it true
before calling onAccept, await the actual result of onAccept (don't rely on
Promise.resolve), and use try/finally to ensure isSubmitting is reset to false
and handler() is still called (or errors propagated) — update references in the
function (handleAccept, formik.setFieldValue, onAccept, handler) and ensure the
UI button is tied to this isSubmitting flag to prevent rapid clicks.
In `@admin-ui/plugins/scim/components/ScimConfiguration.tsx`:
- Around line 53-57: submitForm is declared to return void so callers cannot
await handleSubmit; change submitForm to return the submit result by making it
async (or returning the Promise) and return handleSubmit({...}) instead of void,
and update its type signature accordingly; also update the hook deps to use
formik (not formik.values) so the dependency array follows the recommended
pattern and keeps submitForm stable when formik changes.
---
Outside diff comments:
In `@admin-ui/plugins/fido/components/DynamicConfiguration.tsx`:
- Around line 51-58: submitForm currently returns void which prevents
GluuCommitDialog's awaited onAccept from waiting; change submitForm to return
the promise from handleSubmit and ensure the readOnly branch returns a resolved
Promise (e.g., Promise.resolve()) so callers can await consistently. Also update
the hook dependencies to use formik (not formik.values) so useCallback tracks
Formik correctly; references: submitForm, handleSubmit, formik, readOnly, and
GluuCommitDialog onAccept.
In `@admin-ui/plugins/fido/components/StaticConfiguration.tsx`:
- Around line 69-76: Make submitForm async and return the result of handleSubmit
so the dialog awaiting onAccept receives the submit completion; specifically,
change submitForm to an async function that does "if (readOnly) return;" and
then "return await handleSubmit(formik.values, userMessage)". Also update the
hook dependency array to use formik (not formik.values) and keep handleSubmit
and readOnly: [handleSubmit, formik, readOnly], so submitForm stays stable and
follows Formik recommendations.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
admin-ui/app/routes/Apps/Gluu/GluuCommitDialog.tsxadmin-ui/app/routes/Apps/Gluu/GluuCommitDialogLegacy.tsxadmin-ui/plugins/auth-server/components/Configuration/ConfigApiConfiguration/ApiConfigForm.tsxadmin-ui/plugins/fido/components/DynamicConfiguration.tsxadmin-ui/plugins/fido/components/StaticConfiguration.tsxadmin-ui/plugins/scim/components/ScimConfiguration.tsxadmin-ui/plugins/smtp-management/components/SmtpManagement/SmtpForm.tsx
💤 Files with no reviewable changes (1)
- admin-ui/plugins/smtp-management/components/SmtpManagement/SmtpForm.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
admin-ui/plugins/auth-server/components/Configuration/AuthPage.tsx (1)
304-315:⚠️ Potential issue | 🟠 MajorAvoid showing success toast after failed ACR update.
If
putAcrsMutation.mutateAsyncfails, the innercatchat Line 309 swallows the error, but Line 313 can still showsuccess_in_savingfor ACR-only submissions (patches.length === 0). This creates a false-success UX.Suggested fix
+ let acrUpdateFailed = false if (put && put.value) { const newAcr = { defaultAcr: put.value || acrs?.defaultAcr } try { await putAcrsMutation.mutateAsync({ data: newAcr }) await logAcrUpdate(newAcr, message, { defaultAcr: newAcr.defaultAcr }) } catch (error) { + acrUpdateFailed = true console.error('Error updating ACR:', error) } } - if (patches.length === 0) { + if (patches.length === 0 && !acrUpdateFailed) { toast.success(t('messages.success_in_saving')) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/plugins/auth-server/components/Configuration/AuthPage.tsx` around lines 304 - 315, The success toast is shown unconditionally when patches.length === 0 even if the ACR update failed; modify the block around putAcrsMutation.mutateAsync and logAcrUpdate so the toast.success(t('messages.success_in_saving')) only runs after a successful mutateAsync/logAcrUpdate—e.g., track a local boolean (e.g., acrUpdated) that is set to true only on successful await putAcrsMutation.mutateAsync(...) and await logAcrUpdate(...), and check that flag (and/or rethrow the error from the catch) before calling toast.success; reference putAcrsMutation.mutateAsync, logAcrUpdate, patches, toast.success, put/put.value to locate where to add the flag or change control flow.
♻️ Duplicate comments (2)
admin-ui/plugins/scim/components/ScimConfiguration.tsx (1)
53-61: 🧹 Nitpick | 🔵 TrivialConsider using
formikinstead offormik.valuesin dependencies.Per project learnings, keeping the entire
formikobject in the dependency array is the recommended pattern, as extracting individual properties can cause issues with Formik's internal field registration and focus management.Also, the type assertion at line 58 is redundant since
handleSubmitalready returnsvoid | Promise<unknown>.Proposed fix
const submitForm = useCallback( - (userMessage: string): void | Promise<unknown> => { - return handleSubmit({ - ...formik.values, - action_message: userMessage, - }) as void | Promise<unknown> + (userMessage: string) => { + return handleSubmit({ + ...formik.values, + action_message: userMessage, + }) }, - [handleSubmit, formik.values], + [handleSubmit, formik], )Based on learnings: "In React components using Formik, do not extract individual Formik methods or properties into separate variables to use in dependency arrays. Keeping the entire formik object in the dependency array is the recommended pattern."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/plugins/scim/components/ScimConfiguration.tsx` around lines 53 - 61, The submitForm callback uses formik.values in its dependency array and performs a redundant type assertion on handleSubmit's return; update the dependency to reference the whole formik object (use [handleSubmit, formik]) to follow Formik patterns and avoid breaking internal registration/focus behavior, and remove the unnecessary "as void | Promise<unknown>" cast since handleSubmit already has that return type; update the submitForm declaration to use submitForm, handleSubmit, and formik symbols accordingly.admin-ui/app/routes/Apps/Gluu/GluuCommitDialogLegacy.tsx (1)
94-109:⚠️ Potential issue | 🟡 MinorGuard and cleanup placement inconsistent with GluuCommitDialog.tsx.
Two issues:
Guard race condition: Similar to
GluuCommitDialog.tsx,setIsSubmitting(true)at line 99 should be moved immediately after the guard check at line 95.Error handling difference: The
finallyblock closes the modal even ifonAcceptthrows an error. InGluuCommitDialog.tsx,closeModal()is called inside thetryblock, so the modal stays open on errors. Consider aligning the behavior for consistency.Proposed fix to align with GluuCommitDialog.tsx
async function handleAccept() { if (isSubmitting) return + setIsSubmitting(true) if (formik) { formik.setFieldValue('action_message', userMessage) } - setIsSubmitting(true) try { const result = onAccept(userMessage) await Promise.resolve(result) + handler() + onCloseModal() + setUserMessage('') } finally { setIsSubmitting(false) - handler() - onCloseModal() - setUserMessage('') } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin-ui/app/routes/Apps/Gluu/GluuCommitDialogLegacy.tsx` around lines 94 - 109, In handleAccept move setIsSubmitting(true) immediately after the early-return guard that checks isSubmitting (so the guard and state flip are adjacent), keep the formik.setFieldValue(...) before that; then call onAccept(userMessage) and await it inside a try block and, on success, call handler(), onCloseModal(), and setUserMessage('') before exiting the try; leave only setIsSubmitting(false) in the finally block so the modal is not closed when onAccept throws. Ensure you update references in handleAccept (isSubmitting, setIsSubmitting, formik.setFieldValue, onAccept, handler, onCloseModal, setUserMessage) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@admin-ui/app/routes/Apps/Gluu/GluuCommitDialog.tsx`:
- Around line 105-118: In handleAccept, the early guard check uses isSubmitting
before setIsSubmitting(true) which can allow a race; move setIsSubmitting(true)
immediately after the isSubmitting guard (i.e., check if (isSubmitting) return;
then call setIsSubmitting(true)) so no concurrent invocations proceed, keep the
existing formik.setFieldValue('action_message', userMessage), await the onAccept
result as before, call closeModal(), and ensure setIsSubmitting(false) remains
in the finally block to reset the flag; update the dependency array if needed to
reflect any changes to referenced symbols (handleAccept, isSubmitting,
setIsSubmitting, formik, onAccept, userMessage, closeModal).
---
Outside diff comments:
In `@admin-ui/plugins/auth-server/components/Configuration/AuthPage.tsx`:
- Around line 304-315: The success toast is shown unconditionally when
patches.length === 0 even if the ACR update failed; modify the block around
putAcrsMutation.mutateAsync and logAcrUpdate so the
toast.success(t('messages.success_in_saving')) only runs after a successful
mutateAsync/logAcrUpdate—e.g., track a local boolean (e.g., acrUpdated) that is
set to true only on successful await putAcrsMutation.mutateAsync(...) and await
logAcrUpdate(...), and check that flag (and/or rethrow the error from the catch)
before calling toast.success; reference putAcrsMutation.mutateAsync,
logAcrUpdate, patches, toast.success, put/put.value to locate where to add the
flag or change control flow.
---
Duplicate comments:
In `@admin-ui/app/routes/Apps/Gluu/GluuCommitDialogLegacy.tsx`:
- Around line 94-109: In handleAccept move setIsSubmitting(true) immediately
after the early-return guard that checks isSubmitting (so the guard and state
flip are adjacent), keep the formik.setFieldValue(...) before that; then call
onAccept(userMessage) and await it inside a try block and, on success, call
handler(), onCloseModal(), and setUserMessage('') before exiting the try; leave
only setIsSubmitting(false) in the finally block so the modal is not closed when
onAccept throws. Ensure you update references in handleAccept (isSubmitting,
setIsSubmitting, formik.setFieldValue, onAccept, handler, onCloseModal,
setUserMessage) accordingly.
In `@admin-ui/plugins/scim/components/ScimConfiguration.tsx`:
- Around line 53-61: The submitForm callback uses formik.values in its
dependency array and performs a redundant type assertion on handleSubmit's
return; update the dependency to reference the whole formik object (use
[handleSubmit, formik]) to follow Formik patterns and avoid breaking internal
registration/focus behavior, and remove the unnecessary "as void |
Promise<unknown>" cast since handleSubmit already has that return type; update
the submitForm declaration to use submitForm, handleSubmit, and formik symbols
accordingly.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
admin-ui/app/routes/Apps/Gluu/GluuCommitDialog.tsxadmin-ui/app/routes/Apps/Gluu/GluuCommitDialogLegacy.tsxadmin-ui/app/utils/hooks/useWebhookDialogAction.tsxadmin-ui/plugins/auth-server/components/Configuration/AuthPage.tsxadmin-ui/plugins/scim/components/ScimConfiguration.tsxadmin-ui/plugins/scim/components/ScimPage.tsxadmin-ui/plugins/scim/types/index.ts
|
duttarnab
approved these changes
Feb 27, 2026
moabu
approved these changes
Feb 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix(admin-ui): ensure confirmation popup closes after accept in apply changes flow (#2671)
Summary
The confirmation popup triggered during the Apply changes workflow was not closing after clicking Accept, even when the required comment length was provided. This resulted in a blocked UI flow and confusion during configuration updates.
Environment
Problem
accessTokenLife).Root Cause
Fix Details
Result
🔗 Ticket
Closes: #2671
Summary by CodeRabbit
Refactor
Bug Fixes
API/Behavior