feat(content-sharing): Create contact service getContacts #4338
Conversation
WalkthroughAdds a new contact-service hook and contact conversion utilities, re-exports the hook, integrates Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant ContentSharingV2
participant ContactService as useContactService
participant SharingService as useSharingService
participant Modal as UnifiedShareModal
User->>ContentSharingV2: open share UI
ContentSharingV2->>ContactService: init(api, itemID, currentUser?.id)
ContentSharingV2->>SharingService: init(api, itemID, currentUser?.id)
ContentSharingV2->>Modal: render({ contactService, sharingService, ... })
rect rgb(220,240,255)
note over Modal: Request contacts
Modal->>ContactService: getContacts()
ContactService->>useContacts: fetch users & groups
useContacts-->>ContactService: responses
ContactService->>ContactService: convert & merge (users + groups)
ContactService-->>Modal: contacts
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/elements/content-sharing/hooks/useContactService.ts (1)
43-62: Consider extracting the 'Group' magic string.Lines 55 and 58 use the hardcoded string
'Group'. While the comment on line 55 explains the workaround for avatar compatibility, extracting this to a named constant (e.g.,GROUP_PLACEHOLDER_VALUE) would improve maintainability and make future refactoring easier if the avatar component's requirements change.+const GROUP_PLACEHOLDER_VALUE = 'Group'; + export const convertGroupContactsResponse = contactsAPIData => { const { entries = [] } = contactsAPIData; // Only return groups with the correct permissions return entries .filter(({ permissions }) => { return permissions && permissions.can_invite_as_collaborator; }) .map(contact => { const { id, name, type } = contact; return { id, - email: 'Group', // Need this for the avatar to work for isUserContactType + email: GROUP_PLACEHOLDER_VALUE, // Need this for the avatar to work for isUserContactType name, type, - value: 'Group', + value: GROUP_PLACEHOLDER_VALUE, }; }) .sort(sortByName); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/elements/content-sharing/ContentSharingV2.tsx(3 hunks)src/elements/content-sharing/hooks/index.ts(1 hunks)src/elements/content-sharing/hooks/useContactService.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/elements/content-sharing/ContentSharingV2.tsx (2)
src/elements/content-sharing/hooks/useContactService.ts (1)
useContactService(64-86)src/elements/content-sharing/ContentSharing.js (1)
api(89-89)
src/elements/content-sharing/hooks/useContactService.ts (3)
src/features/unified-share-modal/utils/convertData.js (1)
APP_USERS_DOMAIN_REGEXP(111-111)src/elements/content-sharing/SharingModal.js (1)
currentUserID(77-77)src/elements/content-sharing/hooks/index.ts (1)
useContactService(1-1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (5)
src/elements/content-sharing/hooks/index.ts (1)
1-2: LGTM!Clean barrel export pattern for the hooks module.
src/elements/content-sharing/ContentSharingV2.tsx (2)
155-155: LGTM!Passing
contactServicetoUnifiedShareModalintegrates the new contact functionality correctly.
53-53: Ensure UnifiedShareModal accepts or guards against a null contactService prop.UnifiedShareModal comes from @box/unified-share-modal, so confirm its TypeScript definitions or PropTypes allow
contactService: ContactService | null(or it internally guards null). If it doesn’t, defer rendering untilcontactServiceis non-null or wrap the component in a null check.src/elements/content-sharing/hooks/useContactService.ts (2)
64-86: Verify stability of contact hooks’ returns
Confirm thatuseContactsanduseContactsByEmailreturn stable function references across renders and correctly reapply thetransformUsersclosure whencurrentUserIDchanges.
14-38: Enforce explicit ID type and status checks
- Confirm that
currentUserIDand eachcontact.idshare the same runtime type (e.g. both strings) per the API’s type definitions.- Replace the loose
status && status !== STATUS_INACTIVEguard with an explicit status check (e.g.status === STATUS_ACTIVE).
reneshen0328
changed the title
reneshen0328
force-pushed
the
create-contact-service
branch
from
c9099f3 to
6b359ed
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/elements/content-sharing/hooks/useContactService.ts (1)
8-8: Avoid duplicating APP_USERS_DOMAIN_REGEXP.This constant is already flagged in a previous review. Import the existing constant from
src/features/unified-share-modal/utils/convertData.jsinstead of redefining it.
🧹 Nitpick comments (1)
src/elements/content-sharing/hooks/useContactService.ts (1)
64-86: Memoize transformer functions to prevent unnecessary re-renders.The inline arrow functions on lines 66-67 and 71 are recreated on every render, causing
useContactsanduseContactsByEmailto return new function references. This defeats the purpose of theuseMemoon line 74, asgetContactsandgetContactsByEmailchange on every render.Apply this diff to memoize the transformer functions:
export const useContactService = (api, itemID, currentUserID) => { + const transformUsers = React.useCallback( + data => convertUserContactsResponse(data, currentUserID), + [currentUserID] + ); + + const transformGroups = React.useCallback( + data => convertGroupContactsResponse(data), + [] + ); + + const transformUsersByEmail = React.useCallback( + data => convertUserContactsByEmailResponse(data), + [] + ); + const getContacts = useContacts(api, itemID, { - transformUsers: data => convertUserContactsResponse(data, currentUserID), - transformGroups: data => convertGroupContactsResponse(data), + transformUsers, + transformGroups, }); const getContactsByEmail = useContactsByEmail(api, itemID, { - transformUsers: data => convertUserContactsByEmailResponse(data), + transformUsers: transformUsersByEmail, }); const contactService = React.useMemo(() => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/elements/content-sharing/ContentSharingV2.tsx(3 hunks)src/elements/content-sharing/hooks/index.ts(1 hunks)src/elements/content-sharing/hooks/useContactService.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/elements/content-sharing/hooks/index.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/elements/content-sharing/ContentSharingV2.tsx (1)
src/elements/content-sharing/hooks/useContactService.ts (1)
useContactService(64-86)
src/elements/content-sharing/hooks/useContactService.ts (3)
src/features/unified-share-modal/utils/convertData.js (1)
APP_USERS_DOMAIN_REGEXP(111-111)src/elements/content-sharing/SharingModal.js (1)
currentUserID(77-77)src/elements/content-sharing/hooks/index.ts (1)
useContactService(1-1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (6)
src/elements/content-sharing/ContentSharingV2.tsx (3)
11-11: LGTM!The import correctly includes both hooks from the barrel export.
63-63: LGTM!The hook call correctly uses optional chaining for
currentUser?.id, which safely handles the case whencurrentUseris null. ThecontactServicewill be null until the current user is loaded, which is handled appropriately by the hook's implementation.
168-168: LGTM!The
contactServiceprop is passed consistently with the existing pattern used forsharingService.src/elements/content-sharing/hooks/useContactService.ts (3)
9-9: LGTM!The sort comparator correctly uses
localeComparefor locale-aware string comparison.
14-38: LGTM!The function correctly filters out the current user, app users (identified by domain), and inactive users, then maps the remaining entries to the internal contact format and sorts them by name.
43-62: LGTM!The function correctly filters groups with collaboration permissions and maps them to the internal format. The hardcoded
'Group'value for email and value is explained by the inline comment as necessary for avatar functionality.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/elements/content-sharing/utils/convertContactServiceData.ts (1)
38-57: Group contact conversion logic is correct!The function properly:
- Filters groups based on
can_invite_as_collaboratorpermission- Maps to the expected contact format with placeholder values
- Sorts alphabetically by name
The comment on line 50 could be more descriptive:
- email: 'Group', // Need this for the avatar to work for isUserContactType + email: 'Group', // Avatar component requires an email field; 'Group' is used as a sentinel value for group entities
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/elements/content-sharing/hooks/__tests__/useContactService.test.ts(1 hunks)src/elements/content-sharing/hooks/useContactService.ts(1 hunks)src/elements/content-sharing/utils/__tests__/convertContactServiceData.test.ts(1 hunks)src/elements/content-sharing/utils/convertContactServiceData.ts(1 hunks)src/elements/content-sharing/utils/index.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-02T16:47:50.715Z
Learnt from: reneshen0328
PR: box/box-ui-elements#4322
File: src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js:30-36
Timestamp: 2025-10-02T16:47:50.715Z
Learning: In src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js, mockCurrentUserID is intentionally a number (789) while DEFAULT_USER_API_RESPONSE.id is a string ('789'). This represents different stages of data transformation: mockCurrentUserID is the raw API response before conversion, and DEFAULT_USER_API_RESPONSE.id is the post-conversion state after fetchCurrentUser processes it.
Applied to files:
src/elements/content-sharing/utils/__tests__/convertContactServiceData.test.ts
🧬 Code graph analysis (4)
src/elements/content-sharing/utils/__tests__/convertContactServiceData.test.ts (2)
src/elements/content-sharing/utils/convertContactServiceData.ts (2)
convertUserContactsResponse(9-33)convertGroupContactsResponse(38-57)src/elements/content-sharing/utils/index.ts (2)
convertUserContactsResponse(3-3)convertGroupContactsResponse(3-3)
src/elements/content-sharing/hooks/__tests__/useContactService.test.ts (2)
src/elements/content-sharing/utils/convertContactServiceData.ts (2)
convertGroupContactsResponse(38-57)convertUserContactsResponse(9-33)src/elements/content-sharing/hooks/useContactService.ts (1)
useContactService(6-21)
src/elements/content-sharing/utils/convertContactServiceData.ts (2)
src/features/unified-share-modal/utils/convertData.js (1)
APP_USERS_DOMAIN_REGEXP(111-111)src/elements/content-sharing/utils/index.ts (2)
convertUserContactsResponse(3-3)convertGroupContactsResponse(3-3)
src/elements/content-sharing/hooks/useContactService.ts (1)
src/elements/content-sharing/utils/convertContactServiceData.ts (2)
convertUserContactsResponse(9-33)convertGroupContactsResponse(38-57)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint_test_build
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Summary
🔇 Additional comments (5)
src/elements/content-sharing/utils/__tests__/convertContactServiceData.test.ts (1)
1-332: Comprehensive test coverage!The test suite thoroughly covers all critical scenarios for both
convertUserContactsResponseandconvertGroupContactsResponse:
- Empty inputs and valid conversions
- Filtering logic (current user, app domain users, inactive users, missing emails, group permissions)
- Alphabetical sorting
The use of
test.eachfor parametrized testing of invalid statuses is a clean approach.src/elements/content-sharing/hooks/useContactService.ts (1)
1-21: Clean hook implementation!The hook correctly wires contact retrieval with transformation:
- Properly passes transformer callbacks to
useContacts- Correctly memoizes
contactServicewith appropriate dependencies- Returns
nullwhencurrentUserIdis falsy, preventing unnecessary API callssrc/elements/content-sharing/utils/index.ts (1)
1-6: LGTM!The re-exports correctly expose the new contact conversion utilities.
src/elements/content-sharing/hooks/__tests__/useContactService.test.ts (1)
1-71: Thorough test coverage!The test suite validates:
- Null handling when
currentUserIDis falsy- Correct wiring of
getContactsin the returnedcontactService- Internal transform functions properly invoke the conversion utilities with correct parameters
src/elements/content-sharing/utils/convertContactServiceData.ts (1)
9-33: User contact conversion logic is correct!The function properly:
- Filters out the current user, app domain users, inactive users, and entries without email
- Maps to the expected contact format
- Sorts alphabetically by name
reneshen0328
force-pushed
the
create-contact-service
branch
from
c908cbd to
b517321
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/elements/content-sharing/__tests__/useContacts.test.js (1)
37-37: Consider adding test cases for the new gating logic.While adding
currentUserId: '123'maintains existing test coverage, consider adding test cases to verify:
- Behavior when
currentUserIdis undefined- Behavior when
isContentSharingV2Enabledis true butcurrentUserIdis absent (should not set upgetContactsbased on the gating logic inuseContacts.js)These additional tests would ensure the new conditional logic in
useContacts.js(lines 30-32) is properly covered.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/elements/content-sharing/__tests__/useContacts.test.js(1 hunks)src/elements/content-sharing/hooks/__tests__/useContactService.test.ts(1 hunks)src/elements/content-sharing/hooks/useContactService.ts(1 hunks)src/elements/content-sharing/hooks/useContacts.js(2 hunks)src/elements/content-sharing/types.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/elements/content-sharing/hooks/tests/useContactService.test.ts
- src/elements/content-sharing/hooks/useContactService.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-02T16:47:50.715Z
Learnt from: reneshen0328
PR: box/box-ui-elements#4322
File: src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js:30-36
Timestamp: 2025-10-02T16:47:50.715Z
Learning: In src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js, mockCurrentUserID is intentionally a number (789) while DEFAULT_USER_API_RESPONSE.id is a string ('789'). This represents different stages of data transformation: mockCurrentUserID is the raw API response before conversion, and DEFAULT_USER_API_RESPONSE.id is the post-conversion state after fetchCurrentUser processes it.
Applied to files:
src/elements/content-sharing/__tests__/useContacts.test.js
🧬 Code graph analysis (1)
src/elements/content-sharing/hooks/useContacts.js (5)
src/elements/content-sharing/hooks/useInvites.js (1)
noop(20-26)src/elements/content-sharing/hooks/useContactsByEmail.js (1)
noop(23-23)src/elements/content-sharing/hooks/useCollaborators.js (1)
noop(31-31)src/elements/content-sharing/SharingModal.js (1)
getContacts(93-93)src/elements/content-sharing/ContentSharing.js (1)
api(89-89)
🪛 Biome (2.1.2)
src/elements/content-sharing/hooks/useContacts.js
[error] 29-29: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Summary
- GitHub Check: lint_test_build
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
src/elements/content-sharing/types.js (1)
95-95: LGTM!The addition of
currentUserIdtoContentSharingHooksOptionsis well-typed and aligns with the new contact service functionality introduced in this PR.src/elements/content-sharing/hooks/useContacts.js (3)
20-27: LGTM!The destructuring correctly extracts the new options
currentUserIdandisContentSharingV2Enabledalong with existing options. The default values for callbacks are appropriate.
30-32: Early return logic prevents redundant setup.The gating condition correctly prevents re-execution when:
getContactsis already set (avoids redundant creation)- Content Sharing v2 is enabled but
currentUserIdis missing (guards against incomplete configuration)This ensures the hook only sets up the contacts function when properly configured.
72-82: Dependency array correctly expanded.The additions of
currentUserId,getContacts, andisContentSharingV2Enabledto the dependency array are appropriate:
currentUserIdandisContentSharingV2Enabledaffect the early-return condition- Including
getContactsprevents re-creation once it's set, working in tandem with the early return at line 30Note: The Biome linter error flagging line 29 as a conditional hook call is a false positive. The
useEffecthook is called unconditionally at the top level; the early return (line 31) is inside the effect callback, which is permitted.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/elements/content-sharing/types.js (1)
95-95: LGTM! Type additions are correct.The new optional fields
currentUserIdandisContentSharingV2Enabledare properly typed and integrate well with the contact service functionality described in the PR.Consider adding JSDoc comments to document the purpose of these new fields:
export type ContentSharingHooksOptions = { + /** The ID of the current user, used to filter contacts and gate certain features */ currentUserId?: string, handleError?: Function, handleRemoveSharedLinkError?: Function, handleRemoveSharedLinkSuccess?: Function, handleSuccess?: Function, handleUpdateSharedLinkError?: Function, handleUpdateSharedLinkSuccess?: Function, + /** Feature flag to enable ContentSharingV2 contact fetching behavior */ isContentSharingV2Enabled?: boolean,Also applies to: 102-102
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/elements/content-sharing/types.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: lint_test_build
- GitHub Check: Summary
What
Before this PR: does not load suggested email dropdown options in collaborator section
After this PR:
getMarkerBasedUsersAPIandgetMarkerBasedGroupsAPIAPI call (created and used by existing contentSharingV1) are called, the contactService object which returnsgetContactsis passed in successfully, and the UI return the users within the same enterprise in the dropdown properly.Testing:
Screen.Recording.2025-10-16.at.10.46.34.AM.mov
Summary by CodeRabbit
New Features
Refactor
Tests