feat: add popups for import and add users on sponsors global#963
Open
tomrndom wants to merge 5 commits into
Open
feat: add popups for import and add users on sponsors global#963tomrndom wants to merge 5 commits into
tomrndom wants to merge 5 commits into
Conversation
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
|
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:
📝 WalkthroughWalkthroughAdds global sponsor-user invite and import popups, updates sponsor query and invite payload handling, integrates the popups into the sponsor users page, and adds tests for both flows. ChangesSponsor User Management Popups
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/actions/sponsor-users-actions.js`:
- Around line 535-547: The current promise chain swallows errors with
catch(console.log) and calls dispatch(stopLoading()) twice; remove the
catch(console.log) so rejections propagate to callers (or replace it with a
catch that logs and rethrows), and remove the stopLoading() call inside the
.then handler so stopLoading is invoked only from .finally; update the chain
around dispatch, stopLoading, snackbarSuccessHandler, and
T.translate("sponsor_users.new_user.success") accordingly so success still shows
the snackbar but loading is stopped exactly once in .finally.
In
`@src/pages/sponsors/sponsor-users-list-page/components/sponsor-global-import-users-popup.js`:
- Around line 86-95: The handleImport handler currently calls
importSponsorUsers(...).then(() => onClose()) which will close the dialog even
on failure; change it to await the thunk result from importSponsorUsers in
handleImport (or handle its returned promise) and only call onClose() when the
thunk returns an explicit success flag (e.g., { success: true }) or resolves
truthy; update importSponsorUsers so it resolves/rejects or returns an explicit
success value on completion, and gate onClose() on that success result (and
handle or surface errors when it fails instead of closing).
- Around line 49-50: The code reads the selected sponsor's id from
formik.values.sponsor?.value but the select stores {id, name, companyId}, so
sponsorId becomes undefined; update both occurrences (the sponsorId assignment
and the other similar block around lines where companyId is used) to use
formik.values.sponsor?.id instead of .value, and ensure any downstream uses
(e.g., the import target and candidate list loader) reference this corrected
sponsorId and continue to use formik.values.sponsor?.companyId for companyId.
In
`@src/pages/sponsors/sponsor-users-list-page/components/sponsor-global-new-user-popup.js`:
- Around line 37-41: The handler handleOnSave currently closes the popup in a
finally block so failures also dismiss the form; change it to call
sendSponsorUserInvite(values.email, values.sponsor.id).then(() => {
getSponsorUsers(); handleClose(); }).catch((err) => { /* keep popup open and
surface error to user */ }); and ensure the sendSponsorUserInvite thunk returns
a rejected Promise on failure so the catch runs; reference handleOnSave,
sendSponsorUserInvite, getSponsorUsers, and handleClose when making the change.
🪄 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: 67165833-76a2-4a0f-91a7-33147faa7006
📒 Files selected for processing (5)
src/actions/sponsor-actions.jssrc/actions/sponsor-users-actions.jssrc/pages/sponsors/sponsor-users-list-page/components/sponsor-global-import-users-popup.jssrc/pages/sponsors/sponsor-users-list-page/components/sponsor-global-new-user-popup.jssrc/pages/sponsors/sponsor-users-list-page/index.js
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
smarcet
reviewed
Jun 9, 2026
smarcet
reviewed
Jun 9, 2026
smarcet
requested changes
Jun 9, 2026
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/pages/sponsors/sponsor-users-list-page/components/sponsor-global-new-user-popup.js (1)
43-49:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftSuccess path is still triggered on failed invites.
Line 43 chains
.then(...), but the thunk currently swallows errors insrc/actions/sponsor-users-actions.js(.catch(console.log)), so this block can still run after a failed API call and close/refresh the dialog incorrectly. Gate close/refresh on an explicit success result (or make the thunk reject on failure).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/sponsors/sponsor-users-list-page/components/sponsor-global-new-user-popup.js` around lines 43 - 49, The success path currently runs even when the invite fails because sendSponsorUserInvite swallows errors; update the flow so sendSponsorUserInvite (the thunk in src/actions/sponsor-users-actions.js) returns a rejected promise on failure (remove the terminal .catch(console.log) or re-throw the error) or have the caller inspect an explicit success result before proceeding; then in sponsor-global-new-user-popup.js, only call getSponsorUsers() and handleClose() when sendSponsorUserInvite resolves successfully (or when the resolved payload indicates success) and ensure setIsSaving(false) still runs in finally.src/pages/sponsors/sponsor-users-list-page/components/sponsor-global-import-users-popup.js (2)
94-96:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftImport dialog can close even when import fails.
Line 95 closes in
.then(...), but the current thunk contract insrc/actions/sponsor-users-actions.jsswallows failures, so this can still execute on failed imports. Only close after a verifiable success result (or make thunk reject on failure).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/sponsors/sponsor-users-list-page/components/sponsor-global-import-users-popup.js` around lines 94 - 96, The import dialog is closed unconditionally in the then() callback of importSponsorUsers even though the import thunk in sponsor-users-actions.js swallows failures; change the flow so onClose is only called after a verifiable success: either update the thunk importSponsorUsers to reject the promise on failure, or change the caller to inspect the resolved value (e.g., check a success flag or returned result) before calling onClose; keep setIsSaving(false) in finally() but move onClose into the branch that confirms success so failures do not close the dialog.
51-52:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUse sponsor
id, notvalue, for selected sponsor state.Line 51 reads
formik.values.sponsor?.value, but Line 156-160 stores{ id, name, companyId }. This leavessponsorIdundefined and breaks candidate loading/import targeting.Proposed fix
- const sponsorId = formik.values.sponsor?.value; + const sponsorId = formik.values.sponsor?.id;Also applies to: 156-160
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/sponsors/sponsor-users-list-page/components/sponsor-global-import-users-popup.js` around lines 51 - 52, The selected sponsor was read from formik.values.sponsor?.value but the stored object uses { id, name, companyId }, so sponsorId becomes undefined; update the code to read formik.values.sponsor?.id (leave companyId as formik.values.sponsor?.companyId) wherever sponsorId is derived (e.g., the sponsorId and companyId consts and any import/targeting logic) so it matches the stored { id, name, companyId } shape and restores correct candidate loading/import targeting.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In
`@src/pages/sponsors/sponsor-users-list-page/components/sponsor-global-import-users-popup.js`:
- Around line 94-96: The import dialog is closed unconditionally in the then()
callback of importSponsorUsers even though the import thunk in
sponsor-users-actions.js swallows failures; change the flow so onClose is only
called after a verifiable success: either update the thunk importSponsorUsers to
reject the promise on failure, or change the caller to inspect the resolved
value (e.g., check a success flag or returned result) before calling onClose;
keep setIsSaving(false) in finally() but move onClose into the branch that
confirms success so failures do not close the dialog.
- Around line 51-52: The selected sponsor was read from
formik.values.sponsor?.value but the stored object uses { id, name, companyId },
so sponsorId becomes undefined; update the code to read
formik.values.sponsor?.id (leave companyId as formik.values.sponsor?.companyId)
wherever sponsorId is derived (e.g., the sponsorId and companyId consts and any
import/targeting logic) so it matches the stored { id, name, companyId } shape
and restores correct candidate loading/import targeting.
In
`@src/pages/sponsors/sponsor-users-list-page/components/sponsor-global-new-user-popup.js`:
- Around line 43-49: The success path currently runs even when the invite fails
because sendSponsorUserInvite swallows errors; update the flow so
sendSponsorUserInvite (the thunk in src/actions/sponsor-users-actions.js)
returns a rejected promise on failure (remove the terminal .catch(console.log)
or re-throw the error) or have the caller inspect an explicit success result
before proceeding; then in sponsor-global-new-user-popup.js, only call
getSponsorUsers() and handleClose() when sendSponsorUserInvite resolves
successfully (or when the resolved payload indicates success) and ensure
setIsSaving(false) still runs in finally.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5c64094e-48bc-48bc-8ff6-a00458e2f6bf
📒 Files selected for processing (4)
src/pages/sponsors/sponsor-users-list-page/components/__tests__/sponsor-global-import-users-popup.test.jssrc/pages/sponsors/sponsor-users-list-page/components/__tests__/sponsor-global-new-user-popup.test.jssrc/pages/sponsors/sponsor-users-list-page/components/sponsor-global-import-users-popup.jssrc/pages/sponsors/sponsor-users-list-page/components/sponsor-global-new-user-popup.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/pages/sponsors/sponsor-users-list-page/components/tests/sponsor-global-new-user-popup.test.js
- src/pages/sponsors/sponsor-users-list-page/components/tests/sponsor-global-import-users-popup.test.js
smarcet
reviewed
Jun 23, 2026
smarcet
requested changes
Jun 23, 2026
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/pages/sponsors/sponsor-users-list-page/components/sponsor-global-import-users-popup.js (2)
102-108: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftGate
onClose()on a confirmed successful import.With the current thunk contract,
importSponsorUsersresolves even on failure (its internal.catch), so this.thenruns and the dialog closes / selection is lost even when nothing was imported, and there is no explicit.catch. Have the thunk reject or return an explicit success flag and only callonClose()on success.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/sponsors/sponsor-users-list-page/components/sponsor-global-import-users-popup.js` around lines 102 - 108, The `handleImport` flow closes the popup too early because `importSponsorUsers` currently resolves even when the import fails, so the `.then(() => onClose())` path runs unconditionally. Update the thunk contract used by `handleImport` in `sponsor-global-import-users-popup.js` so `importSponsorUsers` either rejects on failure or returns an explicit success result, then gate `onClose()` behind a confirmed success check and add a failure path that keeps the dialog open.
52-53: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winRead the selected sponsor's
id, notvalue.
formatSelectedValuestores{ id, name, companyId }, soformik.values.sponsor?.valueis alwaysundefined. As a resultsponsorIdstays falsy: the fetch effect (Line 56) never runs, the user checklist (Line 175) never renders, and import (Line 105) sends an undefined target sponsor.Proposed fix
- const sponsorId = formik.values.sponsor?.value; + const sponsorId = formik.values.sponsor?.id;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/sponsors/sponsor-users-list-page/components/sponsor-global-import-users-popup.js` around lines 52 - 53, The sponsor lookup is reading the wrong field from the selected sponsor object: in sponsor-global-import-users-popup.js, use the id stored by formatSelectedValue instead of sponsor.value when deriving sponsorId in the popup logic. Update the sponsorId assignment near the Formik value reads so the fetch effect, checklist rendering, and import flow all receive the actual sponsor identifier, while keeping companyId sourced from the same selected sponsor object.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In
`@src/pages/sponsors/sponsor-users-list-page/components/sponsor-global-import-users-popup.js`:
- Around line 102-108: The `handleImport` flow closes the popup too early
because `importSponsorUsers` currently resolves even when the import fails, so
the `.then(() => onClose())` path runs unconditionally. Update the thunk
contract used by `handleImport` in `sponsor-global-import-users-popup.js` so
`importSponsorUsers` either rejects on failure or returns an explicit success
result, then gate `onClose()` behind a confirmed success check and add a failure
path that keeps the dialog open.
- Around line 52-53: The sponsor lookup is reading the wrong field from the
selected sponsor object: in sponsor-global-import-users-popup.js, use the id
stored by formatSelectedValue instead of sponsor.value when deriving sponsorId
in the popup logic. Update the sponsorId assignment near the Formik value reads
so the fetch effect, checklist rendering, and import flow all receive the actual
sponsor identifier, while keeping companyId sourced from the same selected
sponsor object.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bfc6f2b3-f74f-407a-840d-25af6ce009df
📒 Files selected for processing (2)
src/actions/sponsor-users-actions.jssrc/pages/sponsors/sponsor-users-list-page/components/sponsor-global-import-users-popup.js
💤 Files with no reviewable changes (1)
- src/actions/sponsor-users-actions.js
…omise errors Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
tomrndom
force-pushed
the
feature/sponsor-global-user-add-import
branch
from
3b2bc70 to
cbc2f4b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/pages/sponsors/sponsor-users-list-page/components/sponsor-global-import-users-popup.js (1)
52-53: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winUse
sponsor.idinstead ofsponsor.valuefor the selected sponsor key.Line 52 reads
formik.values.sponsor?.value, but this form storesid(Line 41, Line 44, Line 168). That leavessponsorIdundefined for fetch/render/import paths (Line 56, Line 105, Line 176).Proposed fix
- const sponsorId = formik.values.sponsor?.value; + const sponsorId = formik.values.sponsor?.id;Also applies to: 105-106, 176-176
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/sponsors/sponsor-users-list-page/components/sponsor-global-import-users-popup.js` around lines 52 - 53, The selected sponsor key is being read from sponsor.value even though this form stores the sponsor identifier on sponsor.id, which leaves sponsorId undefined in the fetch/render/import flows. Update sponsor-global-import-users-popup.js in the relevant sponsorId reads and any dependent logic in the import/render paths to use the sponsor object’s id field consistently, keeping companyId unchanged, and verify the same identifier is used anywhere the selected sponsor is passed into the query or import handlers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/pages/sponsors/sponsor-users-list-page/components/sponsor-global-import-users-popup.js`:
- Around line 77-93: The load-more handler in
sponsor-global-import-users-popup.js can merge stale results and crash when the
current userOptions state is null. Update handleLoadMoreUsers to guard the
setUserOptions merge against a missing previous state, and ensure the response
from fetchSponsorUsersBySummit is only applied if it still matches the current
sponsor/summit context (for example by checking currentSummit.id,
selectedSummit, and companyId against the request that was issued). Keep the
protection around isLoadingMore and the pagination check, but avoid appending
userData.data to a stale or null value.data.
---
Duplicate comments:
In
`@src/pages/sponsors/sponsor-users-list-page/components/sponsor-global-import-users-popup.js`:
- Around line 52-53: The selected sponsor key is being read from sponsor.value
even though this form stores the sponsor identifier on sponsor.id, which leaves
sponsorId undefined in the fetch/render/import flows. Update
sponsor-global-import-users-popup.js in the relevant sponsorId reads and any
dependent logic in the import/render paths to use the sponsor object’s id field
consistently, keeping companyId unchanged, and verify the same identifier is
used anywhere the selected sponsor is passed into the query or import handlers.
🪄 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: b38f6943-66e0-4fb0-9496-c633764c25e1
📒 Files selected for processing (2)
src/actions/sponsor-users-actions.jssrc/pages/sponsors/sponsor-users-list-page/components/sponsor-global-import-users-popup.js
💤 Files with no reviewable changes (1)
- src/actions/sponsor-users-actions.js
Comment on lines
+77
to
+93
| const handleLoadMoreUsers = () => { | ||
| if (isLoadingMore) return; | ||
| if (userOptions.current_page < userOptions.last_page) { | ||
| setIsLoadingMore(true); | ||
| fetchSponsorUsersBySummit( | ||
| currentSummit.id, | ||
| selectedSummit, | ||
| companyId, | ||
| userOptions.current_page + 1 | ||
| ) | ||
| .then((userData) => { | ||
| setUserOptions((value) => ({ | ||
| ...userData, | ||
| data: [...value.data, ...userData.data] | ||
| })); | ||
| }) | ||
| .finally(() => setIsLoadingMore(false)); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Protect load-more from stale responses and null merge state.
Lines 88-91 assume value.data exists and always belong to the current sponsor/summit context. If context changes mid-request (Lines 58-59/149-150), this can merge stale rows or throw on value.data when value is null.
Proposed fix
-import React, { useEffect, useState } from "react";
+import React, { useEffect, useRef, useState } from "react";
...
+ const loadMoreRequestIdRef = useRef(0);
...
useEffect(() => {
if (selectedSummit && sponsorId && companyId) {
+ loadMoreRequestIdRef.current += 1; // invalidate in-flight load-more requests
let cancelled = false;
setUserOptions(null);
setSelectedUsers([]);
...
const handleLoadMoreUsers = () => {
- if (isLoadingMore) return;
- if (userOptions.current_page < userOptions.last_page) {
+ if (isLoadingMore || !userOptions) return;
+ if (userOptions.current_page < userOptions.last_page) {
+ const requestId = ++loadMoreRequestIdRef.current;
setIsLoadingMore(true);
fetchSponsorUsersBySummit(
currentSummit.id,
selectedSummit,
companyId,
userOptions.current_page + 1
)
.then((userData) => {
+ if (requestId !== loadMoreRequestIdRef.current || !userData?.data) return;
setUserOptions((value) => ({
+ ...(value || {}),
...userData,
- data: [...value.data, ...userData.data]
+ data: [...(value?.data || []), ...userData.data]
}));
})
.finally(() => setIsLoadingMore(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.
Suggested change
| const handleLoadMoreUsers = () => { | |
| if (isLoadingMore) return; | |
| if (userOptions.current_page < userOptions.last_page) { | |
| setIsLoadingMore(true); | |
| fetchSponsorUsersBySummit( | |
| currentSummit.id, | |
| selectedSummit, | |
| companyId, | |
| userOptions.current_page + 1 | |
| ) | |
| .then((userData) => { | |
| setUserOptions((value) => ({ | |
| ...userData, | |
| data: [...value.data, ...userData.data] | |
| })); | |
| }) | |
| .finally(() => setIsLoadingMore(false)); | |
| const handleLoadMoreUsers = () => { | |
| if (isLoadingMore || !userOptions) return; | |
| if (userOptions.current_page < userOptions.last_page) { | |
| const requestId = ++loadMoreRequestIdRef.current; | |
| setIsLoadingMore(true); | |
| fetchSponsorUsersBySummit( | |
| currentSummit.id, | |
| selectedSummit, | |
| companyId, | |
| userOptions.current_page + 1 | |
| ) | |
| .then((userData) => { | |
| if (requestId !== loadMoreRequestIdRef.current || !userData?.data) return; | |
| setUserOptions((value) => ({ | |
| ...(value || {}), | |
| ...userData, | |
| data: [...(value?.data || []), ...userData.data] | |
| })); | |
| }) | |
| .finally(() => setIsLoadingMore(false)); | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/pages/sponsors/sponsor-users-list-page/components/sponsor-global-import-users-popup.js`
around lines 77 - 93, The load-more handler in
sponsor-global-import-users-popup.js can merge stale results and crash when the
current userOptions state is null. Update handleLoadMoreUsers to guard the
setUserOptions merge against a missing previous state, and ensure the response
from fetchSponsorUsersBySummit is only applied if it still matches the current
sponsor/summit context (for example by checking currentSummit.id,
selectedSummit, and companyId against the request that was issued). Keep the
protection around isLoadingMore and the pagination check, but avoid appending
userData.data to a stale or null value.data.
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.
ref: https://app.clickup.com/t/86ba8m3nd
Signed-off-by: Tomás Castillo tcastilloboireau@gmail.com
Summary by CodeRabbit