fix(content-sharing): getContact error handler#4467
Merged
mergify[bot] merged 1 commit intomasterfrom Mar 6, 2026
Merged
Conversation
Contributor
WalkthroughError handling in content-sharing contact hooks was changed so API errors now resolve with empty fallback values (arrays/objects). Corresponding tests were updated to assert these resolved empty values and to await hook results deterministically. Changes
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/elements/content-sharing/__tests__/SharingModal.test.js`:
- Line 1081: The test uses a promise matcher but doesn't await it; update the
assertion in SharingModal.test.js to await the promise matcher so Jest waits for
it to resolve (i.e., change the `expect(response).resolves.toEqual({})`
assertion to use `await`), keeping the test async and targeting the `response`
variable used in the failing assertion.
In `@src/elements/content-sharing/__tests__/useContacts.test.js`:
- Around line 273-274: The promise assertion is not being returned so Jest may
finish the test before the .resolves matcher runs; update the failing test in
useContacts.test.js to either prepend return to the promise assertion (return
expect(contacts).resolves.toEqual([])) or make the test async and await the
assertion (await expect(contacts).resolves.toEqual([])); reference the test's
handleError mock and the contacts promise to locate the assertion to change.
In `@src/elements/content-sharing/hooks/useContacts.js`:
- Around line 54-57: The per-branch error handlers currently call resolve([])
which lets the other successful branch be concatenated and yields partial
contacts; instead, make each branch propagate failure to the combined lookup so
the overall result becomes empty on any single lookup failure: replace the
per-branch resolve([]) with a reject(error) (or set a shared failure flag) from
the callbacks that call handleError, and then catch that rejection in the outer
combined Promise.all (the code that currently concatenates the results around
the line that resolves to the final contacts) and resolve there to [] so
ContactsField receives an empty array when either users or groups lookup fails.
Use the existing symbols handleError, resolve/reject in the branch callbacks and
the combined Promise.all concatenation point to locate and implement this
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3d135cdd-cb48-4491-bdba-60045f5163e5
📒 Files selected for processing (5)
src/elements/content-sharing/__tests__/SharingModal.test.jssrc/elements/content-sharing/__tests__/useContacts.test.jssrc/elements/content-sharing/__tests__/useContactsByEmail.test.jssrc/elements/content-sharing/hooks/useContacts.jssrc/elements/content-sharing/hooks/useContactsByEmail.js
tjuanitas
approved these changes
Mar 6, 2026
jfox-box
approved these changes
Mar 6, 2026
Contributor
Merge Queue StatusRule:
This pull request spent 15 hours 37 minutes 59 seconds in the queue, with no time running CI. Required conditions to merge
ReasonPull request #4467 has been dequeued. The pull request rule doesn't match anymore HintYou should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. |
reneshen0328
force-pushed
the
fix-getContact-error-handler
branch
from
18258d8 to
6ccc72e
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/elements/content-sharing/hooks/useContacts.js (1)
54-57:⚠️ Potential issue | 🟠 MajorPropagate single-branch failures so the combined lookup can return a true empty state.
Resolving
[]here still lets Line 75 concatenate whichever side succeeded, so a users-only or groups-only failure yields partial contacts instead of the PR’s intended empty-state behavior. Reject the branch promise here and collapse to[]once around the combinedPromise.all.💡 Proposed fix
- const getUsers = new Promise((resolve: (result: Array<UserMini>) => void) => { + const getUsers = new Promise((resolve: (result: Array<UserMini>) => void, reject) => { api.getMarkerBasedUsersAPI(false).getUsersInEnterprise( itemID, (response: UserCollection) => resolveAPICall(resolve, response, transformUsers), error => { handleError(error); - resolve([]); + reject(error); }, { filter_term: filterTerm }, ); }); - const getGroups = new Promise((resolve: (result: Array<GroupMini>) => void) => { + const getGroups = new Promise((resolve: (result: Array<GroupMini>) => void, reject) => { api.getMarkerBasedGroupsAPI(false).getGroupsInEnterprise( itemID, (response: GroupCollection) => resolveAPICall(resolve, response, transformGroups), error => { handleError(error); - resolve([]); + reject(error); }, { fields: [FIELD_NAME, FIELD_PERMISSIONS].toString(), filter_term: filterTerm, }, ); }); - return Promise.all([getUsers, getGroups]).then(contactArrays => [...contactArrays[0], ...contactArrays[1]]); + return Promise.all([getUsers, getGroups]) + .then(([users, groups]) => [...users, ...groups]) + .catch(() => []);Also applies to: 65-68
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/elements/content-sharing/hooks/useContacts.js` around lines 54 - 57, The branch-level error handlers currently call resolve([]) which lets Promise.all succeed with partial results; change those branch handlers to reject(error) (i.e., throw the error from the branch promise instead of resolving an empty array) in the error callback that calls handleError so a single-branch failure will cause the combined Promise.all to reject; then wrap the combined Promise.all (inside useContacts) with a single .catch that returns [] (or otherwise collapses to an empty state) so the overall lookup yields a true empty result; apply the same change to the sibling error handler referenced at the other branch (the error handler around lines 65-68).
🧹 Nitpick comments (1)
src/elements/content-sharing/__tests__/useContactsByEmail.test.js (1)
218-240: Cover the new V2 failure wrapper too.This only protects the legacy
{ emails: [...] }path. SinceuseContactsByEmailnow has separate error wrappers for both legacy andisContentSharingV2Enabled, add a companion V2 failure case here and pass a real error object so thehandleError(error)forwarding is covered as well.🧪 Suggested test expansion
beforeEach(() => { + const apiError = new Error('lookup failed'); getUsersInEnterprise = jest .fn() .mockImplementation((itemID, getUsersInEnterpriseSuccess, getUsersInEnterpriseError) => { - return getUsersInEnterpriseError(); + return getUsersInEnterpriseError(apiError); }); mockAPI = createAPIMock({ getUsersInEnterprise }); }); @@ - expect(handleError).toHaveBeenCalled(); + expect(handleError).toHaveBeenCalledWith(apiError); expect(contacts).toEqual({}); }); + + test('should resolve to an empty object in the V2 failure path', async () => { + const { result } = renderHook(() => + useContactsByEmail(mockAPI, MOCK_ITEM_ID, { + isContentSharingV2Enabled: true, + handleSuccess, + handleError, + transformUsers: mockTransformUsers, + }), + ); + + let contacts; + await act(async () => { + contacts = await result.current(MOCK_EMAIL); + }); + + expect(handleError).toHaveBeenCalledWith(apiError); + expect(contacts).toEqual({}); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/elements/content-sharing/__tests__/useContactsByEmail.test.js` around lines 218 - 240, Add a new test in the same file that mirrors the existing failure case but exercises the V2 path: call useContactsByEmail with the isContentSharingV2Enabled flag set (via the hook options) and mock the V2 failure path to reject/invoke the error callback with a real Error instance; then invoke result.current({ emails: [MOCK_EMAIL] }) inside act, assert the V2 API/mock was called, assert handleError was called with the Error object, and assert the returned contacts is {} so the V2 error wrapper forwarding is covered.
🤖 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/elements/content-sharing/hooks/useContacts.js`:
- Around line 54-57: The branch-level error handlers currently call resolve([])
which lets Promise.all succeed with partial results; change those branch
handlers to reject(error) (i.e., throw the error from the branch promise instead
of resolving an empty array) in the error callback that calls handleError so a
single-branch failure will cause the combined Promise.all to reject; then wrap
the combined Promise.all (inside useContacts) with a single .catch that returns
[] (or otherwise collapses to an empty state) so the overall lookup yields a
true empty result; apply the same change to the sibling error handler referenced
at the other branch (the error handler around lines 65-68).
---
Nitpick comments:
In `@src/elements/content-sharing/__tests__/useContactsByEmail.test.js`:
- Around line 218-240: Add a new test in the same file that mirrors the existing
failure case but exercises the V2 path: call useContactsByEmail with the
isContentSharingV2Enabled flag set (via the hook options) and mock the V2
failure path to reject/invoke the error callback with a real Error instance;
then invoke result.current({ emails: [MOCK_EMAIL] }) inside act, assert the V2
API/mock was called, assert handleError was called with the Error object, and
assert the returned contacts is {} so the V2 error wrapper forwarding is
covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 01347f04-9bb3-4b21-8ef3-2dfe64b43cca
📒 Files selected for processing (5)
src/elements/content-sharing/__tests__/SharingModal.test.jssrc/elements/content-sharing/__tests__/useContacts.test.jssrc/elements/content-sharing/__tests__/useContactsByEmail.test.jssrc/elements/content-sharing/hooks/useContacts.jssrc/elements/content-sharing/hooks/useContactsByEmail.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/elements/content-sharing/tests/useContacts.test.js
- src/elements/content-sharing/tests/SharingModal.test.js
Contributor
Merge Queue StatusRule:
This pull request spent 7 seconds in the queue, with no time running CI. Required conditions to merge
|
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
3 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.
If get user or group contacts API fail, we want to resolve the response so that the dropdown render empty state UI with no results instead of loading infinitely.
Summary by CodeRabbit
Bug Fixes
Tests