feat: import sponsor users improvements#888
Open
santipalenque wants to merge 4 commits intomasterfrom
Open
Conversation
📝 WalkthroughWalkthroughAdds sponsor-user import task tracking with polling: triggers imports, stores task IDs in Redux, polls task status every 10s, dispatches status updates and success/error snackbars, and adds an error snackbar action and import-status translations. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant Popup as Import Popup
participant Component as SponsorUsersComponent
participant Redux as Redux Store
participant API as API / Task Endpoint
participant UI as Snackbar UI
User->>Popup: Start import
Popup->>Redux: Dispatch IMPORT_SPONSOR_USERS_TRIGGERED (returns task_id)
Redux->>Redux: Append task_id to importTasks
Redux->>Component: Props update (importTasks non-empty)
Component->>Component: Start polling interval (every TEN_SECONDS_IN_MILLISECONDS)
loop Poll every 10s
Component->>Redux: Dispatch trackImportSponsorUsers()
Redux->>API: GET /tasks/{task_id}
API-->>Redux: Return status & results
Redux->>Redux: Dispatch RECEIVE_SPONSOR_USERS_IMPORT_STATUS(status, task_id, errors)
alt status == SUCCESS
Redux->>Redux: Remove task_id from importTasks
Redux->>UI: Dispatch snackbar success (or aggregated errors -> snackbarErrorMsg)
else status == FAILURE
Redux->>Redux: Remove task_id from importTasks
Redux->>UI: Dispatch snackbar error
else
Redux->>Redux: Keep task_id in importTasks
end
alt importTasks empty
Component->>Component: Stop polling interval
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/pages/sponsors/sponsor-page/tabs/sponsor-users-list-per-sponsor/components/import-users-popup.js (1)
36-48: Consider addingcurrentSummit.idto useEffect dependencies.The fetch now depends on
currentSummit.id(passed as the first parameter), but it's not in the dependency array. WhilecurrentSummit.idis typically stable during a session, adding it ensures consistency if the summit context ever changes.Additionally, consider guarding against
currentSummit.id === 0(the default unset value) to prevent querying with an invalid summit ID.♻️ Suggested improvement
useEffect(() => { - if (selectedSummit) { + if (selectedSummit && currentSummit.id) { fetchSponsorUsersBySummit( currentSummit.id, selectedSummit, companyId, 1 ).then((userData) => { setUserOptions(userData); setSelectedUsers([]); }); } - }, [selectedSummit]); + }, [selectedSummit, currentSummit.id, companyId]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/sponsors/sponsor-page/tabs/sponsor-users-list-per-sponsor/components/import-users-popup.js` around lines 36 - 48, The useEffect that calls fetchSponsorUsersBySummit (inside the component using useEffect, setUserOptions, setSelectedUsers) should include currentSummit.id in its dependency array and guard against an unset summit id; update the effect to depend on [selectedSummit, currentSummit.id] and early-return if currentSummit.id is falsy (e.g., 0) or if selectedSummit is not valid before calling fetchSponsorUsersBySummit(currentSummit.id, selectedSummit, companyId, 1), then setUserOptions and setSelectedUsers as before.
🤖 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/actions/sponsor-users-actions.js`:
- Around line 564-596: In trackImportSponsorUsers, the code reads the wrong
state slice and property names causing undefined errors: replace the
destructuring of sponsorUserListState and tasks from getState() with
sponsorUsersListState and importTasks (as defined in the reducer), then use
importTasks where tasks is currently used in the forEach and subsequent
getRequest call; ensure any local references to tasks (inside
trackImportSponsorUsers) are renamed to importTasks so the function reads the
correct state from getState().
In `@src/reducers/sponsors/sponsor-users-list-reducer.js`:
- Around line 161-168: The filter predicate in the reducer is inverted: in the
branch handling IMPORT_SPONSOR_USERS_STATUS.SUCCESS and .FAILURE you currently
use state.importTasks.filter((t) => t === task_id) which keeps only the
completed task; change the predicate to remove that task by using t !== task_id
so importTasks becomes state.importTasks.filter(t => t !== task_id) (update the
assignment where importTasks is set in the reducer handling
IMPORT_SPONSOR_USERS_STATUS).
---
Nitpick comments:
In
`@src/pages/sponsors/sponsor-page/tabs/sponsor-users-list-per-sponsor/components/import-users-popup.js`:
- Around line 36-48: The useEffect that calls fetchSponsorUsersBySummit (inside
the component using useEffect, setUserOptions, setSelectedUsers) should include
currentSummit.id in its dependency array and guard against an unset summit id;
update the effect to depend on [selectedSummit, currentSummit.id] and
early-return if currentSummit.id is falsy (e.g., 0) or if selectedSummit is not
valid before calling fetchSponsorUsersBySummit(currentSummit.id, selectedSummit,
companyId, 1), then setUserOptions and setSelectedUsers as before.
🪄 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: 050a0de6-11ac-40b3-a42b-8a631f2adba0
📒 Files selected for processing (7)
src/actions/base-actions.jssrc/actions/sponsor-users-actions.jssrc/i18n/en.jsonsrc/pages/sponsors/sponsor-page/tabs/sponsor-users-list-per-sponsor/components/import-users-popup.jssrc/pages/sponsors/sponsor-page/tabs/sponsor-users-list-per-sponsor/index.jssrc/reducers/sponsors/sponsor-users-list-reducer.jssrc/utils/constants.js
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/actions/sponsor-users-actions.js`:
- Around line 586-595: Guard the access to response.result.errors before reading
length to avoid TypeError; in the block where you check response.result.errors
(the code around response.result.errors.length > 0 that dispatches
snackbarErrorMsg), use a safe check like verifying response.result exists and
that response.result.errors is an array (e.g.,
Array.isArray(response.result.errors) && response.result.errors.length > 0) and
only build the html payload by joining when it is an array. Update the condition
and the html construction used in the dispatch(snackbarErrorMsg(...)) invocation
so it safely handles missing result/errors (fall back to a generic message or
empty string).
In
`@src/pages/sponsors/sponsor-page/tabs/sponsor-users-list-per-sponsor/__tests__/sponsor-users-list-per-sponsor-page.test.js`:
- Around line 224-238: The test uses importTasks as an array of task objects but
the reducer stores importTasks as an array of IDs; update the tests to pass
arrays of numeric IDs (e.g., importTasks: [1]) instead of objects (e.g., [{ id:
1, status: "running" }]) wherever renderPage is called in this test file so the
mocked state shape matches the real reducer (specifically change the case in the
"starts polling every 10 seconds..." test that calls renderPage({ importTasks:
[...] }) and also update the other occurrences noted in the comment that pass
importTasks at the later assertions).
🪄 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: 1b35a25b-43d0-4eaa-91bd-2801d9933304
📒 Files selected for processing (3)
src/actions/sponsor-users-actions.jssrc/pages/sponsors/sponsor-page/tabs/sponsor-users-list-per-sponsor/__tests__/sponsor-users-list-per-sponsor-page.test.jssrc/reducers/sponsors/sponsor-users-list-reducer.js
Comment on lines
+586
to
+595
| ); | ||
|
|
||
| if (response.result.errors.length > 0) { | ||
| dispatch( | ||
| snackbarErrorMsg({ | ||
| title: T.translate("sponsor_users.import_users.fail"), | ||
| html: response.result.errors.map((e) => e.error).join("<br/>") | ||
| }) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Guard against missing result or errors property.
If the API returns a successful status but response.result is undefined or result.errors is missing, accessing response.result.errors.length will throw a TypeError.
🛡️ Proposed defensive fix
- if (response.result.errors.length > 0) {
+ if (response.result?.errors?.length > 0) {
dispatch(
snackbarErrorMsg({
title: T.translate("sponsor_users.import_users.fail"),
html: response.result.errors.map((e) => e.error).join("<br/>")
})
);
}📝 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
| ); | |
| if (response.result.errors.length > 0) { | |
| dispatch( | |
| snackbarErrorMsg({ | |
| title: T.translate("sponsor_users.import_users.fail"), | |
| html: response.result.errors.map((e) => e.error).join("<br/>") | |
| }) | |
| ); | |
| } | |
| ); | |
| if (response.result?.errors?.length > 0) { | |
| dispatch( | |
| snackbarErrorMsg({ | |
| title: T.translate("sponsor_users.import_users.fail"), | |
| html: response.result.errors.map((e) => e.error).join("<br/>") | |
| }) | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/actions/sponsor-users-actions.js` around lines 586 - 595, Guard the
access to response.result.errors before reading length to avoid TypeError; in
the block where you check response.result.errors (the code around
response.result.errors.length > 0 that dispatches snackbarErrorMsg), use a safe
check like verifying response.result exists and that response.result.errors is
an array (e.g., Array.isArray(response.result.errors) &&
response.result.errors.length > 0) and only build the html payload by joining
when it is an array. Update the condition and the html construction used in the
dispatch(snackbarErrorMsg(...)) invocation so it safely handles missing
result/errors (fall back to a generic message or empty string).
Comment on lines
+224
to
+238
| it("starts polling every 10 seconds when import tasks are present", () => { | ||
| renderPage({ importTasks: [{ id: 1, status: "running" }] }); | ||
|
|
||
| expect(trackImportSponsorUsers).not.toHaveBeenCalled(); | ||
|
|
||
| act(() => { | ||
| jest.advanceTimersByTime(10000); | ||
| }); | ||
| expect(trackImportSponsorUsers).toHaveBeenCalledTimes(1); | ||
|
|
||
| act(() => { | ||
| jest.advanceTimersByTime(10000); | ||
| }); | ||
| expect(trackImportSponsorUsers).toHaveBeenCalledTimes(2); | ||
| }); |
There was a problem hiding this comment.
Test data shape mismatch: importTasks should contain task IDs, not objects.
The reducer stores task IDs directly in importTasks (e.g., [1, 2, 3]), but the test uses objects like [{ id: 1, status: "running" }]. While this may pass due to mocking, it misrepresents the actual state shape and could mask bugs.
🧪 Proposed fix
it("starts polling every 10 seconds when import tasks are present", () => {
- renderPage({ importTasks: [{ id: 1, status: "running" }] });
+ renderPage({ importTasks: [1] });Apply the same fix to other tests using importTasks:
- Line 252:
importTasks: [1] - Line 283:
importTasks={[1]} - Line 293:
importTasks={[]}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/pages/sponsors/sponsor-page/tabs/sponsor-users-list-per-sponsor/__tests__/sponsor-users-list-per-sponsor-page.test.js`
around lines 224 - 238, The test uses importTasks as an array of task objects
but the reducer stores importTasks as an array of IDs; update the tests to pass
arrays of numeric IDs (e.g., importTasks: [1]) instead of objects (e.g., [{ id:
1, status: "running" }]) wherever renderPage is called in this test file so the
mocked state shape matches the real reducer (specifically change the case in the
"starts polling every 10 seconds..." test that calls renderPage({ importTasks:
[...] }) and also update the other occurrences noted in the comment that pass
importTasks at the later assertions).
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/actions/sponsor-users-actions.js (1)
589-596:⚠️ Potential issue | 🟡 MinorGuard against missing
resultorerrorsproperty.If the API returns a successful status but
response.resultis undefined orresult.errorsis missing, accessingresponse.result.errors.lengthwill throw a TypeError.🛡️ Proposed defensive fix
- if (response.result.errors.length > 0) { + if (response.result?.errors?.length > 0) { dispatch( snackbarErrorMsg({ title: T.translate("sponsor_users.import_users.fail"), html: response.result.errors.map((e) => e.error).join("<br/>") }) ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/actions/sponsor-users-actions.js` around lines 589 - 596, The current check accesses response.result.errors.length directly which can throw if response.result or result.errors is missing; update the conditional in the import handling logic to safely guard using a null-safe check (e.g., verify response && response.result && Array.isArray(response.result.errors) or use optional chaining) and treat missing errors as an empty array before calling dispatch(snackbarErrorMsg(...)) so you only call .map/.join when errors exist; keep the same dispatch and snackbarErrorMsg usage but base it on a safely-derived errors array (e.g., errors = response?.result?.errors || []).
🧹 Nitpick comments (1)
src/actions/sponsor-users-actions.js (1)
581-598: Consider refining UX for partial success scenarios.When a task completes with
SUCCESSstatus but has errors inresponse.result.errors, users will see both a success snackbar and then an error snackbar in quick succession. This could be confusing.Consider consolidating to a single notification that indicates partial success, or adjusting the success message to be conditional on whether errors exist.
💡 Example: Show a "completed with warnings" message when errors exist
if (response.status === IMPORT_SPONSOR_USERS_STATUS.SUCCESS) { - dispatch( - snackbarSuccessHandler({ - title: T.translate("general.success"), - html: T.translate("sponsor_users.import_users.success") - }) - ); - - if (response.result.errors.length > 0) { + if (response.result?.errors?.length > 0) { dispatch( snackbarErrorMsg({ title: T.translate("sponsor_users.import_users.fail"), html: response.result.errors.map((e) => e.error).join("<br/>") }) ); + } else { + dispatch( + snackbarSuccessHandler({ + title: T.translate("general.success"), + html: T.translate("sponsor_users.import_users.success") + }) + ); } dispatch(getSponsorUsers(sponsor.id));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/actions/sponsor-users-actions.js` around lines 581 - 598, The current flow dispatches snackbarSuccessHandler when response.status === IMPORT_SPONSOR_USERS_STATUS.SUCCESS and then separately dispatches snackbarErrorMsg if response.result.errors.length > 0, causing two back-to-back notifications; change this to dispatch a single notification: if response.status === IMPORT_SPONSOR_USERS_STATUS.SUCCESS and response.result.errors.length === 0 keep the existing snackbarSuccessHandler, but if response.result.errors.length > 0 dispatch one consolidated message (e.g., via snackbarErrorMsg or a new snackbarWarningHandler) with a "completed with warnings" title and a body that includes both the success context and the joined response.result.errors, and then call getSponsorUsers(sponsor.id) as before; update referencing symbols: response.status, IMPORT_SPONSOR_USERS_STATUS.SUCCESS, response.result.errors, snackbarSuccessHandler, snackbarErrorMsg (or create snackbarWarningHandler), and getSponsorUsers(sponsor.id).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/actions/sponsor-users-actions.js`:
- Around line 589-596: The current check accesses response.result.errors.length
directly which can throw if response.result or result.errors is missing; update
the conditional in the import handling logic to safely guard using a null-safe
check (e.g., verify response && response.result &&
Array.isArray(response.result.errors) or use optional chaining) and treat
missing errors as an empty array before calling dispatch(snackbarErrorMsg(...))
so you only call .map/.join when errors exist; keep the same dispatch and
snackbarErrorMsg usage but base it on a safely-derived errors array (e.g.,
errors = response?.result?.errors || []).
---
Nitpick comments:
In `@src/actions/sponsor-users-actions.js`:
- Around line 581-598: The current flow dispatches snackbarSuccessHandler when
response.status === IMPORT_SPONSOR_USERS_STATUS.SUCCESS and then separately
dispatches snackbarErrorMsg if response.result.errors.length > 0, causing two
back-to-back notifications; change this to dispatch a single notification: if
response.status === IMPORT_SPONSOR_USERS_STATUS.SUCCESS and
response.result.errors.length === 0 keep the existing snackbarSuccessHandler,
but if response.result.errors.length > 0 dispatch one consolidated message
(e.g., via snackbarErrorMsg or a new snackbarWarningHandler) with a "completed
with warnings" title and a body that includes both the success context and the
joined response.result.errors, and then call getSponsorUsers(sponsor.id) as
before; update referencing symbols: response.status,
IMPORT_SPONSOR_USERS_STATUS.SUCCESS, response.result.errors,
snackbarSuccessHandler, snackbarErrorMsg (or create snackbarWarningHandler), and
getSponsorUsers(sponsor.id).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5c6c0e93-ffb3-460f-90ba-fab97a89335b
📒 Files selected for processing (1)
src/actions/sponsor-users-actions.js
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
1 participant
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.
https://app.clickup.com/t/86b9axqt3
Summary by CodeRabbit
New Features
Tests