Unify InvitationScreen with PasswordResetScreen#61
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughConsolidates invitation UI state into Changes
Sequence Diagram(s)sequenceDiagram
participant User as "User"
participant UI as "InvitationScreen (client)"
participant API as "Invitation API (server)"
participant Router as "React Router"
participant App as "App (alerts)"
User->>UI: enter email + submit
UI->>UI: set isSubmitting = true
UI->>API: POST /invitations (email)
alt API returns success (response.data)
API-->>UI: 200 { responseData }
UI->>UI: set responseData, clear emailValue, isSubmitting = false
UI-->>User: render ResultCard (OK + email_sent status, optional registration_url)
else API returns error with payload
API-->>UI: 4xx/5xx { response.data }
UI->>UI: set responseData = error payload, isSubmitting = false
UI-->>User: render ResultCard (error)
else API error without payload
API-->>UI: network/error
UI->>App: addAlertFromException(error)
UI->>UI: isSubmitting = false
end
Note right of Router: Header link navigates with state { clearInvitation: true }
User->>Router: navigate to /auth/invite (state: clearInvitation)
Router->>UI: location.state contains clearInvitation
UI->>UI: useEffect clears responseData/emailValue and replace state via navigate('.', { replace: true, state: {} })
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/modules/invite/InvitationScreen.js (2)
116-116: Simplify event handler binding.The arrow function wrapper is unnecessary when the handler signature matches the event callback.
♻️ Simplified binding
- <Form onSubmit={(e) => {sendInvitation(e)}}> + <Form onSubmit={sendInvitation}>Similarly for line 145:
- onChange={(e) => {emailValidation(e)}} + onChange={emailValidation}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/invite/InvitationScreen.js` at line 116, The Form onSubmit currently wraps the handler in an unnecessary arrow function; change the JSX to pass the handler directly by using onSubmit={sendInvitation} so the event is forwarded automatically; locate the Form element and the sendInvitation function to update the binding, and apply the same simplification to the other similar handler referenced (line ~145) so that the event handler is passed directly rather than via an extra arrow wrapper.
24-28: Consider renamingemailValidationto reflect its actual behavior.The function name suggests validation logic, but it only updates state. A name like
handleEmailChangewould better describe its purpose.♻️ Suggested rename
- // Validate email input - const emailValidation = (e) => { + // Handle email input change + const handleEmailChange = (e) => { e.preventDefault(); setEmailValue(e.target.value); }And update the usage on line 145:
- onChange={(e) => {emailValidation(e)}} + onChange={handleEmailChange}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/invite/InvitationScreen.js` around lines 24 - 28, Rename the function emailValidation to a name that reflects it only updates state (suggest handleEmailChange) and update all usages (e.g., the onChange prop on the email input) to call handleEmailChange; inside the function keep the existing behavior of calling e.preventDefault() if needed and setEmailValue(e.target.value) so the component state is updated via setEmailValue.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/modules/invite/InvitationScreen.js`:
- Line 116: The Form onSubmit currently wraps the handler in an unnecessary
arrow function; change the JSX to pass the handler directly by using
onSubmit={sendInvitation} so the event is forwarded automatically; locate the
Form element and the sendInvitation function to update the binding, and apply
the same simplification to the other similar handler referenced (line ~145) so
that the event handler is passed directly rather than via an extra arrow
wrapper.
- Around line 24-28: Rename the function emailValidation to a name that reflects
it only updates state (suggest handleEmailChange) and update all usages (e.g.,
the onChange prop on the email input) to call handleEmailChange; inside the
function keep the existing behavior of calling e.preventDefault() if needed and
setEmailValue(e.target.value) so the component state is updated via
setEmailValue.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3aa1c191-b9dd-4fad-aa05-881cc4c0c856
📒 Files selected for processing (1)
src/modules/invite/InvitationScreen.js
Pe5h4
reviewed
Mar 24, 2026
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/modules/invite/InvitationScreen.js (3)
170-175: Consider trimming whitespace for button disabled state.The current check
emailValue === ''allows whitespace-only input (e.g.,' ') to enable the submit button. While thetype='email'input provides browser validation, the button state could be more accurate.♻️ Suggested fix
<Button color='primary' - disabled={emailValue === ''} // Disable button if input is empty + disabled={emailValue.trim() === ''} // Disable button if input is empty or whitespace > {t('InvitationScreen|Invite')} </Button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/invite/InvitationScreen.js` around lines 170 - 175, The disabled check for the invite Button uses emailValue === '' which allows whitespace-only input; update the disabled prop (on the Button component in InvitationScreen.js) to use a trimmed value (e.g., emailValue.trim() === '') or a small helper (isEmptyEmail(emailValue)) so the button is disabled for empty or whitespace-only strings while keeping existing browser email validation intact.
25-29: Unnecessarye.preventDefault()in onChange handler.The
onChangeevent on an input element does not have a default browser behavior to prevent. This call is a no-op and may mislead readers into thinking there's something being prevented.♻️ Suggested fix
// Handle email input change const handleEmailChange = (e) => { - e.preventDefault(); setEmailValue(e.target.value); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/invite/InvitationScreen.js` around lines 25 - 29, The onChange handler handleEmailChange contains an unnecessary e.preventDefault() call; remove that call and simply read e.target.value and call setEmailValue within handleEmailChange so the function only updates state (leave function name handleEmailChange and the setEmailValue usage unchanged).
31-48: Consider adding a loading state to prevent duplicate submissions.The
sendInvitationfunction has no mechanism to prevent users from clicking the submit button multiple times while the API request is in flight. This could result in multiple invitation requests.♻️ Suggested implementation
const [emailValue, setEmailValue] = useState(''); const [responseData, setResponseData] = useState(undefined); +const [isSubmitting, setIsSubmitting] = useState(false); const tenant = useAppSelector(state => state.tenant?.current);const sendInvitation = async (e) => { e.preventDefault(); + setIsSubmitting(true); const body = { email: emailValue }; try { const response = await SeaCatAuthAPI.post(`/account/${tenant}/invite`, body) setResponseData(response?.data); setEmailValue(''); } catch(e) { if (e?.response?.data) { setResponseData(e?.response?.data); return; } props.app.addAlertFromException(e, t('InvitationScreen|Failed to create invitation')); + } finally { + setIsSubmitting(false); } }Then update the submit button:
<Button color='primary' - disabled={emailValue === ''} + disabled={emailValue === '' || isSubmitting} > - {t('InvitationScreen|Invite')} + {isSubmitting ? t('General|Sending...') : t('InvitationScreen|Invite')} </Button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/invite/InvitationScreen.js` around lines 31 - 48, The sendInvitation function can submit multiple times because it lacks a loading flag; add a component state like isSubmitting, return early from sendInvitation if isSubmitting is true, set isSubmitting = true right before the API call, and ensure you set isSubmitting = false in a finally block after the try/catch so it always resets; update related state calls (setResponseData, setEmailValue) as-is and disable the submit button (or set its disabled prop) when isSubmitting is true to prevent duplicate clicks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/modules/invite/InvitationScreen.js`:
- Around line 170-175: The disabled check for the invite Button uses emailValue
=== '' which allows whitespace-only input; update the disabled prop (on the
Button component in InvitationScreen.js) to use a trimmed value (e.g.,
emailValue.trim() === '') or a small helper (isEmptyEmail(emailValue)) so the
button is disabled for empty or whitespace-only strings while keeping existing
browser email validation intact.
- Around line 25-29: The onChange handler handleEmailChange contains an
unnecessary e.preventDefault() call; remove that call and simply read
e.target.value and call setEmailValue within handleEmailChange so the function
only updates state (leave function name handleEmailChange and the setEmailValue
usage unchanged).
- Around line 31-48: The sendInvitation function can submit multiple times
because it lacks a loading flag; add a component state like isSubmitting, return
early from sendInvitation if isSubmitting is true, set isSubmitting = true right
before the API call, and ensure you set isSubmitting = false in a finally block
after the try/catch so it always resets; update related state calls
(setResponseData, setEmailValue) as-is and disable the submit button (or set its
disabled prop) when isSubmitting is true to prevent duplicate clicks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 14b32096-8d16-44d4-a8f4-b86de85deacb
📒 Files selected for processing (1)
src/modules/invite/InvitationScreen.js
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/modules/invite/InvitationScreen.js (1)
173-178: Consider adding explicittype='submit'for consistency.The Cancel button explicitly specifies
type='button'. For consistency and clarity, the Invite button could explicitly specifytype='submit', even though it defaults to submit inside a form.♻️ Suggested change
<Button color='primary' disabled={emailValue === '' || isSubmitting} + type='submit' > {t('InvitationScreen|Invite')} </Button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/invite/InvitationScreen.js` around lines 173 - 178, The Invite button in InvitationScreen currently relies on default form behavior; explicitly add type='submit' to the Button component (the same Button rendering {t('InvitationScreen|Invite')}) to match the Cancel button's explicit type and make intent clear and consistent within the form, ensuring the submit behavior is explicit even if the default would be submit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/modules/invite/InvitationScreen.js`:
- Around line 85-88: The current render in InvitationScreen.js calls
t(emailError) which will translate undefined when responseData.email_sent?.error
is missing; update the JSX to provide a safe fallback or omit the parenthetical
reason: compute a local variable (e.g., emailError =
responseData.email_sent?.error) and pass t(emailError ??
'InvitationScreen|unknownEmailError') or conditionally render the "(reason)"
segment only when emailError is truthy so the message never shows "(undefined)";
ensure the unique symbol t(emailError) in InvitationScreen.js is updated
accordingly.
---
Nitpick comments:
In `@src/modules/invite/InvitationScreen.js`:
- Around line 173-178: The Invite button in InvitationScreen currently relies on
default form behavior; explicitly add type='submit' to the Button component (the
same Button rendering {t('InvitationScreen|Invite')}) to match the Cancel
button's explicit type and make intent clear and consistent within the form,
ensuring the submit behavior is explicit even if the default would be submit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1df67d15-447a-441b-876b-dca3f1bc806f
📒 Files selected for processing (1)
src/modules/invite/InvitationScreen.js
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 `@src/modules/invite/InvitationScreen.js`:
- Around line 42-60: Add a re-entry guard to sendInvitation by checking the
isSubmitting state at the very start (if (isSubmitting) return) to prevent
duplicate POSTs, consolidate all setIsSubmitting(false) calls into a single
finally block so submission state is cleared exactly once, and remove the
duplicate state resets inside try/catch; apply the same pattern to the other
submit handler referenced around lines 185-186 to ensure both handlers use the
guard and a single finally reset.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: da377afd-f4d3-44f3-9b08-9cad05b573a7
📒 Files selected for processing (1)
src/modules/invite/InvitationScreen.js
Pe5h4
reviewed
Apr 2, 2026
Pe5h4
reviewed
Apr 9, 2026
| // Send invitation to the specified email | ||
| const sendInvitation = async (e) => { | ||
| e.preventDefault(); | ||
| if (isSubmitting) return; |
Collaborator
There was a problem hiding this comment.
is this even needed? You already disable Invite button when submitting, so there is no way sendInvitation is triggered before isSubmitting is set to false
Contributor
Author
There was a problem hiding this comment.
not strictly needed. just that coderabbit suggested that this improves code robustness and defensiveness and protects from some (very rare) race conditions.
Contributor
Author
There was a problem hiding this comment.
should i remove it?
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
2 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.
Summary
Similarly to password reset, the invitation request can have a multi-result: one part is creating the invitation and the other is sending the invitation via email. There are three possible outcomes: invitation created and sent, invitation created but not sent and invitation not created. These correspond to three modes respectively: success, warning and error.
Added link to created credentials detail (if authorized).
The
Continuebutton navigates to the previous page instead of credentials list.Clicking the
Invite other userin the header opens an empty invitation even when currently on the invitation page.Added
isSubmittingstate to prevent duplicate submissions.Preview
Successful result card. The admin is authorized for viewing credentials detail but not for viewing the invitation link:
Success with exception. Invitation created but not sent because the email service is disabled:
Failure. Email service is configured but unreachable. Cannot disclose invitation link:
Summary by CodeRabbit
Refactor
New Features
Bug Fixes