fix: prevent modal from closing when team/channel creation fails#38611
fix: prevent modal from closing when team/channel creation fails#38611Naetiksoni08 wants to merge 4 commits intoRocketChat:developfrom
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
|
|
WalkthroughMoved modal-close behavior so CreateTeam and CreateChannel modals only close after successful creation; on error the modal remains open and server error is surfaced via form error state (root.serverError). Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ModalForm as Modal Form
participant API as Server API
participant UI as Error UI
User->>ModalForm: submit form
ModalForm->>API: createTeam/createChannel request
alt success
API-->>ModalForm: 200 OK
ModalForm->>ModalForm: call onClose()
else failure
API-->>ModalForm: error
ModalForm->>ModalForm: setError('root.serverError', message)
ModalForm->>UI: render server error field
UI-->>User: show error, modal remains open
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
|
Hi there, thanks for the contribution 🚀 I see this as a nice improvement, but in that case the toast will be displayed under the modal, we would need to work on this first, would you like to? |
dougfabris
added
the
contrib: valid
|
@dougfabris Thanks for the feedback. You're right. I’ve identified the issue and will address it. I’ll push the changes and update the PR soon. |
|
Hi @dougfabris , I'm thinking of replacing the toast with an inline form error using react-hook-form's setError, so the error is displayed directly inside the modal. Would that approach work, or would you prefer fixing the toast z-index instead? |
Naetiksoni08
force-pushed
the
fix/prevent-modal-close-on-error
branch
from
0cb6dda to
00c828f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
apps/meteor/client/navbar/NavBarPagesGroup/actions/CreateChannelModal.tsx (1)
106-126:⚠️ Potential issue | 🟡 MinorStale server error persists across retries — destructure
clearErrorstoo.
root.serverErrorset viasetErroris not automatically cleared by react-hook-form on resubmission. If the user clicks "Create" again after a failure, the old error message remains visible throughout the new request. DestructureclearErrorshere and call it at the top ofhandleCreateChannel.Proposed fix
const { register, formState: { errors }, handleSubmit, control, setValue, setError, + clearErrors, watch, } = useForm({Then at the start of
handleCreateChannel:const handleCreateChannel = async ({ name, members, readOnly, topic, broadcast, encrypted, federated }: CreateChannelModalPayload) => { + clearErrors('root.serverError'); let roomData;
🧹 Nitpick comments (1)
apps/meteor/client/navbar/NavBarPagesGroup/actions/CreateChannelModal.tsx (1)
407-413: Consider adding top margin for visual separation from the accordion.This
Fieldsits directly after theAccordionwith no spacing. Other field groups usembe={24}. A small margin (e.g.,mbs={16}) would prevent the error from visually colliding with the accordion panel.Suggested tweak
- {errors?.root?.serverError ? ( - <Field> + {errors?.root?.serverError ? ( + <Field mbs={16}>
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/meteor/client/navbar/NavBarPagesGroup/actions/CreateChannelModal.tsxapps/meteor/client/navbar/NavBarPagesGroup/actions/CreateTeamModal.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/client/navbar/NavBarPagesGroup/actions/CreateTeamModal.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/client/navbar/NavBarPagesGroup/actions/CreateChannelModal.tsx
🔇 Additional comments (1)
apps/meteor/client/navbar/NavBarPagesGroup/actions/CreateChannelModal.tsx (1)
191-196: Error handling approach looks good.Using
setError('root.serverError', ...)to surface the API error inline is a solid approach that addresses the reviewer feedback about toast z-index. Theinstanceof Errorguard with a fallback message is appropriate for the untyped catch.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Hi @dougfabris, I've addressed the toast visibility issue and updated the PR as discussed. Instead of fixing the z-index, I replaced the toast with an inline form error using react-hook-form’s setError. The error now displays directly inside the modal, which is cleaner and avoids any z-index concerns entirely. Please take a look! 
|
|
hey @dougfabris, just following up on pr #38611 I've addressed the requested changes. |
|
/jira ARCH-1935 |
|
@Naetiksoni08 as we use toast messages to display generic errors I would suggest raising a separate PR to display the toasts over modal. I'm not sure, but maybe it's just a matter of replacing their position in DOM, so I would start investigating from there |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #38611 +/- ##
===========================================
- Coverage 70.48% 70.47% -0.02%
===========================================
Files 3174 3174
Lines 111075 111095 +20
Branches 19991 20051 +60
===========================================
+ Hits 78293 78294 +1
- Misses 30740 30756 +16
- Partials 2042 2045 +3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Thanks for the feedback @dougfabris! That makes sense - I'll revert the inline error changes and keep this PR focused on just preventing the modal from closing on failure. I'll investigate the toast positioning issue separately and raise a new PR for that. I'll look into the DOM hierarchy approach you mentioned. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/client/navbar/NavBarPagesGroup/actions/CreateChannelModal.tsx (1)
106-112: Disable the Create button during submission to prevent duplicate channel creation.Before this fix, the
finallyblock closed the modal immediately, so in-flight re-clicks were never observable. Now that the modal stays open on error, there is a window where a user can re-click Create while the first request is still in flight, resulting in a concurrent duplicatechannels.create/groups.createcall.isSubmittingfromformStateindicates whether the form is currently being submitted and is the idiomatic way to dynamically disable the submit button. Since errors are already caught in thetry/catch,isSubmittingwill correctly reset tofalseafter the error path resolves.♻️ Proposed fix
const { register, - formState: { errors }, + formState: { errors, isSubmitting }, handleSubmit, control, setValue, watch, } = useForm({- <Button type='submit' primary> + <Button type='submit' primary disabled={isSubmitting}> {t('Create')} </Button>Also applies to: 407-409
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/navbar/NavBarPagesGroup/actions/CreateChannelModal.tsx` around lines 106 - 112, The form destructuring should include isSubmitting from formState and the Create button must be disabled while the form is submitting: add isSubmitting to the destructure (alongside register, formState: { errors }, handleSubmit, control, setValue, watch) and update the submit button rendering used in CreateChannelModal (and the similar button at the other occurrence) to set disabled={isSubmitting} (or equivalent) so users cannot trigger duplicate channels.create / groups.create calls while a submission is in flight.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/meteor/client/navbar/NavBarPagesGroup/actions/CreateChannelModal.tsxapps/meteor/client/navbar/NavBarPagesGroup/actions/CreateTeamModal.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/client/navbar/NavBarPagesGroup/actions/CreateTeamModal.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/client/navbar/NavBarPagesGroup/actions/CreateChannelModal.tsx
🔇 Additional comments (1)
apps/meteor/client/navbar/NavBarPagesGroup/actions/CreateChannelModal.tsx (1)
187-189: LGTM —onClose()correctly gated to the success path.Sequencing
reload?.()thenonClose()inside thetryblock, with thefinallyremoved, is the minimal and correct fix: the modal now stays open (and form data is preserved) when the API call fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/meteor/client/navbar/NavBarPagesGroup/actions/CreateChannelModal.tsx`:
- Around line 106-112: The form destructuring should include isSubmitting from
formState and the Create button must be disabled while the form is submitting:
add isSubmitting to the destructure (alongside register, formState: { errors },
handleSubmit, control, setValue, watch) and update the submit button rendering
used in CreateChannelModal (and the similar button at the other occurrence) to
set disabled={isSubmitting} (or equivalent) so users cannot trigger duplicate
channels.create / groups.create calls while a submission is in flight.
|
Hey @dougfabris , as suggested I've raised a separate PR to display toasts above modals: #38870 |
5 participants
About this PR
Fixes #38607
onClose()was being called inside afinallyblock in bothCreateTeamModal.tsxandCreateChannelModal.tsx. Sincefinallyalways executes regardless of success or failure, the modal was closing even when the API call failed — losing all the user's form data.This PR moves
onClose()to the success path only, so the modal stays open on error and users can retry without re-entering their data.Affected files
apps/meteor/client/navbar/NavBarPagesGroup/actions/CreateTeamModal.tsxapps/meteor/client/views/navbar/NavBarPagesGroup/actions/CreateChannelModal.tsxTesting
Error scenario (before fix): Modal closed on failure, form data lost.
Error scenario (after fix): Modal stays open, error toast shown, form data preserved.
Success scenario: Modal closes and redirects as expected.
Before fix:
(form Filled)
After fix:
(modal stays open, data preserved)
Summary by CodeRabbit
Task: ARCH-1958