feat(content-sharing): Create contact service getContactByEmail #4342
Conversation
WalkthroughAdds feature-flagged email-based contact retrieval (V2) and conversion, exposes getContactByEmail from the contact service, updates converters and i18n label, migrates tests to React Testing Library renderHook, and adjusts types/exports to include the new contact-by-email flow. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant ContactService as useContactService
participant Hook as useContactsByEmail
participant API as getUsersInEnterprise
participant Converter as convertUserContactByEmailResponse
Client->>ContactService: call getContactByEmail(email)
ContactService->>Hook: request (isContentSharingV2Enabled?, transformUsers)
alt V2 enabled
Hook->>API: getUsersInEnterprise({ filter_term: email })
API-->>Hook: users array
Hook->>Converter: convert first entry
Converter-->>Hook: contact object
else V1 (legacy)
Hook->>API: getUsersInEnterprise({ filter_term: parsedFilterTerm })
API-->>Hook: users array
Hook->>Hook: apply legacy transformUsers (filter_term first email)
end
Hook-->>ContactService: resolved contact(s)
ContactService-->>Client: return contact(s)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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: 5
🧹 Nitpick comments (4)
src/elements/content-sharing/hooks/useContactService.ts (1)
1-2: Add minimal TypeScript annotations to avoid implicit anyThis .ts file has untyped params/return. If noImplicitAny is enabled, builds may fail. Consider adding lightweight types.
Example:
// outside selected lines; illustration only type ContactService = { getContacts: (...args: any[]) => any; getContactByEmail: (...args: any[]) => any; }; export const useContactService = (api: any, itemId: string, currentUserId: string): { contactService: ContactService } => { /* ... */ };If API types exist, replace any with those types.
src/elements/content-sharing/utils/__tests__/convertContactServiceData.test.ts (1)
337-435: Deduplicate/clarify two “undefined login” tests and add multi-entry caseYou currently have overlapping tests for missing/undefined login; one is duplicated and named the same.
Apply:
- test('should handle user contact with undefined login field', () => { - const contactsApiData = { - entries: [ - { - id: 'user-1', - name: 'Jane Smith', - type: 'user', - }, - ], - }; - - const result = convertUserContactByEmailResponse(contactsApiData); - - expect(result).toEqual({ - id: 'user-1', - email: '', - name: 'Jane Smith', - type: 'user', - value: '', - }); - }); + // already covered by "missing login field"Optionally, assert first-entry selection:
// inside the same describe test('should return first entry when multiple are present', () => { const contactsApiData = { entries: [ { id: 'user-1', login: 'a@example.com', name: 'A', type: 'user' }, { id: 'user-2', login: 'b@example.com', name: 'B', type: 'user' }, ], }; expect(convertUserContactByEmailResponse(contactsApiData)).toEqual({ id: 'user-1', email: 'a@example.com', name: 'A', type: 'user', value: 'a@example.com', }); });src/elements/content-sharing/hooks/useContactsByEmail.js (1)
25-27: Allow runtime flag changes (optional)The early return prevents re-binding when isContentSharingV2Enabled flips at runtime. If flags can change, set the function on each change.
Example:
- React.useEffect(() => { - if (getContactsByEmail) return; + React.useEffect(() => { // ... - isContentSharingV2Enabled - ? setGetContactsByEmail(getContactsByEmailV2) - : setGetContactsByEmail(updatedGetContactsByEmailFn); - }, [api, getContactsByEmail, handleError, handleSuccess, isContentSharingV2Enabled, itemID, transformUsers]); + const fn = isContentSharingV2Enabled ? getContactsByEmailV2 : updatedGetContactsByEmailFn; + setGetContactsByEmail(() => fn); + }, [api, handleError, handleSuccess, isContentSharingV2Enabled, itemID, transformUsers]);Also applies to: 73-76
src/elements/content-sharing/__tests__/useContactsByEmail.test.js (1)
227-232: Consider usingwaitForinstead of hardcoded setTimeout.Using a hardcoded 100ms delay with
setTimeoutcan lead to flaky tests if the async operation takes longer. React Testing Library provideswaitForutility for more reliable async testing.Apply this diff to use
waitFor:+ await act(async () => { + await result.current({ emails: [MOCK_EMAIL] }); + }); - result.current({ emails: [MOCK_EMAIL] }); - - // Wait a short time to ensure handleError is called - await act(async () => { - await new Promise(resolve => setTimeout(resolve, 100)); - }); + // The error should be called synchronously or very quickly + await new Promise(resolve => setTimeout(resolve, 0));Alternatively, if the hook doesn't return a promise rejection, you could import and use
waitFor:import { renderHook, act, waitFor } from '@testing-library/react'; // In the test: await act(async () => { result.current({ emails: [MOCK_EMAIL] }); }); await waitFor(() => { expect(handleError).toHaveBeenCalled(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/elements/content-sharing/__tests__/useContactsByEmail.test.js(6 hunks)src/elements/content-sharing/hooks/__tests__/useContactService.test.ts(1 hunks)src/elements/content-sharing/hooks/useContactService.ts(2 hunks)src/elements/content-sharing/hooks/useContactsByEmail.js(2 hunks)src/elements/content-sharing/utils/__tests__/convertContactServiceData.test.ts(2 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 (6)
src/elements/content-sharing/utils/convertContactServiceData.ts (1)
src/elements/content-sharing/utils/index.ts (1)
convertUserContactByEmailResponse(5-5)
src/elements/content-sharing/hooks/useContactsByEmail.js (1)
src/elements/content-sharing/hooks/useContacts.js (2)
noop(20-27)resolveAPICall(34-45)
src/elements/content-sharing/hooks/__tests__/useContactService.test.ts (2)
src/elements/content-sharing/utils/convertContactServiceData.ts (3)
convertGroupContactsResponse(38-57)convertUserContactsResponse(9-33)convertUserContactByEmailResponse(63-78)src/elements/content-sharing/hooks/useContactService.ts (1)
useContactService(5-19)
src/elements/content-sharing/__tests__/useContactsByEmail.test.js (2)
src/features/unified-share-modal/utils/__mocks__/USMMocks.js (3)
MOCK_CONTACTS_BY_EMAIL_CONVERTED_RESPONSE(598-635)MOCK_CONTACTS_API_RESPONSE(415-535)MOCK_ITEM_ID(53-53)src/features/unified-share-modal/utils/convertData.js (2)
src/elements/content-sharing/hooks/useContactService.ts (2)
src/elements/content-sharing/utils/convertContactServiceData.ts (1)
convertUserContactByEmailResponse(63-78)src/elements/content-sharing/utils/index.ts (1)
convertUserContactByEmailResponse(5-5)
src/elements/content-sharing/utils/__tests__/convertContactServiceData.test.ts (2)
src/elements/content-sharing/utils/convertContactServiceData.ts (1)
convertUserContactByEmailResponse(63-78)src/elements/content-sharing/utils/index.ts (1)
convertUserContactByEmailResponse(5-5)
🪛 Biome (2.1.2)
src/elements/content-sharing/hooks/useContactsByEmail.js
[error] 41-41: type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 42-42: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 48-48: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 51-51: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
⏰ 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 (6)
src/elements/content-sharing/utils/index.ts (1)
3-7: Re-export looks correctPublic surface expansion to include convertUserContactByEmailResponse aligns with usages.
src/elements/content-sharing/hooks/useContactService.ts (1)
13-18: Verification confirms change is safe and additiveSearch results show:
contactServiceproperties are not accessed inContentSharingV2.tsx(the sole call site), so no strict type patterns depend on an exact method set.- No type assertions or exhaustive object checks detected.
- The expanded
contactServiceobject withgetContactByEmailandgetContactsis purely additive and safe.The original review assessment stands: the wiring is correct and poses no risk to existing call sites.
src/elements/content-sharing/__tests__/useContactsByEmail.test.js (1)
3-3: LGTM! Clean migration to React Testing Library.The migration from Enzyme to React Testing Library using
renderHookandactis well executed. The test patterns correctly handle async operations and properly verify hook behavior.Also applies to: 38-60, 62-84
src/elements/content-sharing/hooks/__tests__/useContactService.test.ts (3)
38-55: LGTM! Comprehensive test coverage for contactService integration.The test correctly verifies that both
getContactByEmailandgetContactsare exposed through the contact service, and validates that the underlying hooks are called with appropriate parameters including the V2 flag and transform functions.
57-87: LGTM! Thorough validation of transform function wiring.The test comprehensively validates that transform functions passed to both
useContactsanduseContactsByEmailcorrectly invoke their respective conversion utilities (convertUserContactsResponse,convertGroupContactsResponse, andconvertUserContactByEmailResponse) with appropriate parameters.
3-7: LGTM! Clean integration of getContactByEmail support.The additions properly extend the test suite to cover the new
getContactByEmailfunctionality, including appropriate mocks foruseContactsByEmailandconvertUserContactByEmailResponse. The variable renaming (mockItemId,mockCurrentUserId) improves consistency with TypeScript naming conventions.Also applies to: 20-21, 28-28, 31-31
tjuanitas
left a comment
tjuanitas
left a comment
There was a problem hiding this comment.
you might have to walk me through this PR. feel a little lost
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/elements/content-sharing/__tests__/useContactsByEmail.test.js (1)
51-56: Fix Jest matcher usage.Use
expect.any(Function)instead ofexpect.anything(Function). The latter is invalid and will fail.- expect.anything(Function), + expect.any(Function),Apply to all occurrences in this test file (shown line ranges).
Also applies to: 75-80, 166-171, 234-239
src/elements/content-sharing/hooks/useContactsByEmail.js (1)
22-28: Return type/state type mismatch when V2 is enabled.Hook returns
GetContactsByEmailFnType | null, but in V2 you assign aGetContactByEmailFnType. Make the return/state a union to keep Flow happy.-): GetContactsByEmailFnType | null { - const [getContactsByEmail, setGetContactsByEmail] = React.useState<null | GetContactsByEmailFnType>(null); +): GetContactsByEmailFnType | GetContactByEmailFnType | null { + const [getContactsByEmail, setGetContactsByEmail] = + React.useState<null | GetContactsByEmailFnType | GetContactByEmailFnType>(null);
♻️ Duplicate comments (3)
src/elements/content-sharing/__tests__/useContactsByEmail.test.js (1)
177-201: V2 “email not provided” test calls with an object; align with V2 API and intent.The test name states “email is not provided” but it invokes
result.current({ emails: [MOCK_EMAIL] }), which is not a missing email and also mismatches the V2 string signature.Use an empty string (or undefined) and assert no API call/transform, e.g.:
- let contacts; - await act(async () => { - contacts = await result.current({ emails: [MOCK_EMAIL] }); - }); - - expect(handleSuccess).toHaveBeenCalledWith(EMPTY_USERS); - expect(mockTransformUsers).not.toHaveBeenCalled(); - expect(contacts).toEqual({}); + let contacts; + await act(async () => { + contacts = await result.current(''); + }); + + expect(getUsersInEnterprise).not.toHaveBeenCalled(); + expect(handleSuccess).not.toHaveBeenCalled(); + expect(mockTransformUsers).not.toHaveBeenCalled(); + expect(contacts).toEqual({});src/elements/content-sharing/hooks/useContactsByEmail.js (2)
46-63: Normalize V2 to accept both a string email and the V1 object shape.Avoid breaking callers when toggling the flag. Parse a string or
{ emails: string[] }uniformly.- if (isContentSharingV2Enabled) { - const getContactsByEmailV2: GetContactByEmailFnType = () => email => { - if (!email) { - return Promise.resolve({}); - } - - return new Promise(resolve => { - api.getMarkerBasedUsersAPI(false).getUsersInEnterprise( - itemID, - response => resolveAPICall(resolve, response, transformUsers), - handleError, - { filter_term: email }, - ); - }); - }; + if (isContentSharingV2Enabled) { + const getContactsByEmailV2 = () => (input) => { + const email = + typeof input === 'string' + ? input + : (input && Array.isArray(input.emails) && input.emails[0]) || ''; + if (!email) return Promise.resolve({}); + + return new Promise(resolve => { + api.getMarkerBasedUsersAPI(false).getUsersInEnterprise( + itemID, + response => resolveAPICall(resolve, response, transformUsers), + handleError, + { filter_term: email }, + ); + }); + };
64-79: Fix V1 inline param type to match actual usage.
{ [emails: string]: string }is not the intended shape; you index intofilterTerm.emailsas an array.- const updatedGetContactsByEmailFn: GetContactsByEmailFnType = - () => (filterTerm: { [emails: string]: string }) => { + const updatedGetContactsByEmailFn: GetContactsByEmailFnType = + () => (filterTerm: { emails: Array<string> }) => { if (!filterTerm || !Array.isArray(filterTerm.emails) || !filterTerm.emails.length) { return Promise.resolve({}); } const parsedFilterTerm = filterTerm.emails[0];
🧹 Nitpick comments (2)
src/elements/content-sharing/__tests__/useContactsByEmail.test.js (1)
218-233: Remove timer-based flakiness; await error with Testing Library helpers.Replace the manual timeout with
waitForfor deterministic async assertions.-import { renderHook, act } from '@testing-library/react'; +import { renderHook, act, waitFor } from '@testing-library/react'; @@ - // Wait a short time to ensure handleError is called - await act(async () => { - await new Promise(resolve => setTimeout(resolve, 100)); - }); + await waitFor(() => expect(handleError).toHaveBeenCalled());src/elements/content-sharing/hooks/useContactsByEmail.js (1)
30-33: Consider useMemo to avoid stale function when the flag changes.The early return prevents updating the function if
isContentSharingV2Enabledflips after mount. Prefer deriving the function withuseMemokeyed by the flag and deps to avoid setState loops.I can provide a
useMemovariant if you want to adopt it now.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/elements/content-sharing/__tests__/useContactsByEmail.test.js(6 hunks)src/elements/content-sharing/hooks/useContactsByEmail.js(3 hunks)src/elements/content-sharing/types.js(1 hunks)src/elements/content-sharing/utils/convertContactServiceData.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/elements/content-sharing/utils/convertContactServiceData.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__/useContactsByEmail.test.js
🧬 Code graph analysis (3)
src/elements/content-sharing/__tests__/useContactsByEmail.test.js (3)
src/features/unified-share-modal/utils/__mocks__/USMMocks.js (3)
MOCK_CONTACTS_BY_EMAIL_CONVERTED_RESPONSE(598-635)MOCK_CONTACTS_API_RESPONSE(415-535)MOCK_ITEM_ID(53-53)src/elements/content-sharing/__tests__/SharingModal.test.js (1)
createAPIMock(76-89)src/features/unified-share-modal/utils/convertData.js (2)
src/elements/content-sharing/types.js (1)
src/features/unified-share-modal/utils/convertData.js (2)
src/elements/content-sharing/hooks/useContactsByEmail.js (1)
src/elements/content-sharing/hooks/useContacts.js (2)
noop(20-27)resolveAPICall(34-45)
🪛 Biome (2.1.2)
src/elements/content-sharing/types.js
[error] 143-143: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
src/elements/content-sharing/hooks/useContactsByEmail.js
[error] 7-12: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 47-47: type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 64-64: type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 65-65: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 71-71: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 74-74: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
⏰ 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
src/elements/content-sharing/utils/convertContactServiceData.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/elements/content-sharing/utils/convertContactServiceData.ts (1)
38-56: Remove or correct the misleading inline comment referencing non-existentisUserContactType.The code is functionally correct—the
useContactService.ts. However, the inline comment at line 50 referencesisUserContactType, which does not exist anywhere in the codebase. This misleading comment should be removed or rewritten to accurately explain that the display string is used because groups don't have email addresses.Additionally, the parameter name
formatMessageis confusing since it receives a pre-formatted string (e.g.,formatMessage(messages.contactServiceGroupDisplayText)), not a function. Consider renaming it togroupDisplayTextfor clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
i18n/en-US.propertiesis excluded by!i18n/**
📒 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/messages.js(1 hunks)src/elements/content-sharing/utils/__tests__/convertContactServiceData.test.ts(5 hunks)src/elements/content-sharing/utils/convertContactServiceData.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- 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/utils/__tests__/convertContactServiceData.test.ts
🧬 Code graph analysis (3)
src/elements/content-sharing/utils/__tests__/convertContactServiceData.test.ts (1)
src/elements/content-sharing/utils/convertContactServiceData.ts (2)
convertGroupContactsResponse(38-57)convertUserContactByEmailResponse(62-77)
src/elements/content-sharing/utils/convertContactServiceData.ts (1)
src/elements/content-sharing/utils/index.ts (2)
convertGroupContactsResponse(4-4)convertUserContactByEmailResponse(5-5)
src/elements/content-sharing/hooks/__tests__/useContactService.test.ts (2)
src/elements/content-sharing/utils/convertContactServiceData.ts (3)
convertGroupContactsResponse(38-57)convertUserContactsResponse(9-33)convertUserContactByEmailResponse(62-77)src/elements/content-sharing/hooks/useContactService.ts (1)
useContactService(9-26)
⏰ 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 (8)
src/elements/content-sharing/messages.js (1)
62-66: LGTM! Clean i18n message addition.The new
contactServiceGroupDisplayTextmessage follows the existing pattern and provides proper localization support for group contacts.src/elements/content-sharing/utils/__tests__/convertContactServiceData.test.ts (3)
2-6: LGTM! Import correctly updated.The import statement properly includes the new
convertUserContactByEmailResponsefunction.
210-210: LGTM! Tests correctly updated for signature change.The tests properly reflect the updated
convertGroupContactsResponsesignature, passing theformatMessageparameter and expecting thevaluefield to be the group ID instead of the literal 'Group' string.Also applies to: 236-236, 244-244, 251-251, 291-291, 328-328
337-366: LGTM! Comprehensive basic test coverage.The tests for
convertUserContactByEmailResponseproperly cover the empty entries case and valid conversion scenario.src/elements/content-sharing/utils/convertContactServiceData.ts (1)
59-77: LGTM! Docstring correctly updated and implementation is sound.The function properly:
- Extracts the first entry from the API response
- Returns an empty object when no entries are present
- Handles missing/undefined login with a default empty string
- Returns a single contact object with the correct structure
The docstring accurately reflects the implementation, addressing the previous review feedback.
src/elements/content-sharing/hooks/__tests__/useContactService.test.ts (3)
1-32: LGTM! Test setup properly updated.The imports, mocks, and variable naming improvements (ID → Id) are all correct and follow best practices.
38-55: LGTM! Comprehensive test coverage for both retrieval methods.The test properly verifies that both
useContactsanduseContactsByEmailare called with correct parameters and that the returnedcontactServiceobject exposes both functions.
57-87: LGTM! Transform functions thoroughly tested.The test properly verifies that all three transform functions (users, groups, and contact-by-email) are called with the correct data and parameters:
convertUserContactsResponsewith user data and current user IDconvertGroupContactsResponsewith group data and 'Group' display textconvertUserContactByEmailResponsewith email-based contact data
src/elements/content-sharing/utils/__tests__/convertContactServiceData.test.ts
Outdated
Show resolved
Hide resolved
reneshen0328
force-pushed
the
create-contact-service-getContactByEmail
branch
from
407e091 to
000a4f0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/elements/content-sharing/utils/__tests__/convertContactServiceData.test.ts (1)
368-410: Remove duplicate test case.Lines 368-388 and 390-410 are duplicate tests. Both test the exact same scenario: an entry object without a
loginfield. In JavaScript, a missing property and anundefinedproperty are identical—both resolve toundefined. These tests use the same input data and assert the same output.Apply this diff to remove the duplicate:
- test('should handle user contact with undefined login field', () => { - const contactsApiData = { - entries: [ - { - id: 'user-1', - name: 'Jane Smith', - type: 'user', - }, - ], - }; - - const result = convertUserContactByEmailResponse(contactsApiData); - - expect(result).toEqual({ - id: 'user-1', - email: '', - name: 'Jane Smith', - type: 'user', - value: '', - }); - });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/elements/content-sharing/__tests__/useContactsByEmail.test.js(6 hunks)src/elements/content-sharing/utils/__tests__/convertContactServiceData.test.ts(5 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/__tests__/useContactsByEmail.test.jssrc/elements/content-sharing/utils/__tests__/convertContactServiceData.test.ts
🧬 Code graph analysis (2)
src/elements/content-sharing/__tests__/useContactsByEmail.test.js (3)
src/features/unified-share-modal/utils/__mocks__/USMMocks.js (3)
MOCK_CONTACTS_BY_EMAIL_CONVERTED_RESPONSE(598-635)MOCK_CONTACTS_API_RESPONSE(415-535)MOCK_ITEM_ID(53-53)src/elements/content-sharing/__tests__/SharingModal.test.js (1)
createAPIMock(76-89)src/features/unified-share-modal/utils/convertData.js (2)
src/elements/content-sharing/utils/__tests__/convertContactServiceData.test.ts (2)
src/elements/content-sharing/utils/index.ts (2)
convertGroupContactsResponse(4-4)convertUserContactByEmailResponse(5-5)src/elements/content-sharing/utils/convertContactServiceData.ts (2)
convertGroupContactsResponse(38-57)convertUserContactByEmailResponse(62-77)
⏰ 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). (2)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (3)
src/elements/content-sharing/__tests__/useContactsByEmail.test.js (1)
135-175: LGTM! V2 path test correctly extracts and uses email field.The test correctly destructures
loginassrc/elements/content-sharing/utils/__tests__/convertContactServiceData.test.ts (2)
206-254: LGTM! Group conversion tests correctly updated for new signature.The tests properly pass the
formatMessageparameter and validate thatvaluenow contains the group ID rather than a static string.
337-366: LGTM! Basic conversion tests for convertUserContactByEmailResponse.Tests correctly validate empty input returns an empty object and valid input is properly transformed with
loginmapped tovalue.
src/elements/content-sharing/utils/convertContactServiceData.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/elements/content-sharing/hooks/useContactsByEmail.js (2)
48-64: V2 function signature breaks existing consumers.The V2 implementation expects a plain string
UnifiedShareForm.js:278) call with{ emails: [email] }(the V1 signature). TogglingisContentSharingV2Enabledat runtime will cause type errors and unexpected behavior.Per past review comments, unify the signatures. Apply this refactor to make V2 accept the V1 input shape:
- if (isContentSharingV2Enabled) { - const getContactsByEmailV2: GetContactByEmailFnType = () => email => { - if (!email) { + if (isContentSharingV2Enabled) { + const getContactsByEmailV2: GetContactsByEmailFnType = () => filterTerm => { + // Support both V1 object shape and plain string for compatibility + const email = typeof filterTerm === 'string' + ? filterTerm + : (filterTerm && Array.isArray(filterTerm.emails) && filterTerm.emails[0]) || ''; + + if (!email) { return Promise.resolve({}); } return new Promise(resolve => { api.getMarkerBasedUsersAPI(false).getUsersInEnterprise( itemID, response => resolveAPICall(resolve, response, transformUsers), handleError, { filter_term: email }, ); }); }; setGetContactsByEmail(getContactsByEmailV2);
65-84: Fix incorrect type signature for filterTerm.Line 67 uses
{ [emails: string]: string }, which is invalid index signature syntax. The code clearly expects{ emails: Array<string> }based on the runtime checks at lines 68-70.Apply this fix:
- const updatedGetContactsByEmailFn: GetContactsByEmailFnType = - () => (filterTerm: { [emails: string]: string }) => { + const updatedGetContactsByEmailFn: GetContactsByEmailFnType = + () => (filterTerm: { emails: Array<string> }) => { if (!filterTerm || !Array.isArray(filterTerm.emails) || !filterTerm.emails.length) { return Promise.resolve({}); }Additionally, update the type definition in
src/elements/content-sharing/types.jsto match.
🧹 Nitpick comments (1)
src/elements/content-sharing/hooks/useContactsByEmail.js (1)
7-12: Polymorphic return type may complicate consumer code.The return type
GetContactsByEmailFnType | GetContactByEmailFnType | nullmeans consumers must determine which function signature they're receiving at runtime. This can lead to type narrowing issues and fragile consumer code.Consider one of these alternatives:
- Return a single function type with a unified signature (preferred)
- Return an object with explicitly named methods:
{ getByEmail, getByEmails }Also applies to: 20-20, 26-29
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
i18n/en-US.propertiesis excluded by!i18n/**
📒 Files selected for processing (4)
src/elements/content-sharing/hooks/useContactService.ts(1 hunks)src/elements/content-sharing/hooks/useContactsByEmail.js(2 hunks)src/elements/content-sharing/messages.js(1 hunks)src/elements/content-sharing/utils/convertContactServiceData.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/elements/content-sharing/messages.js
- src/elements/content-sharing/utils/convertContactServiceData.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/elements/content-sharing/hooks/useContactService.ts (2)
src/elements/content-sharing/utils/convertContactServiceData.ts (3)
convertUserContactsResponse(9-33)convertGroupContactsResponse(38-57)convertUserContactByEmailResponse(62-77)src/elements/content-sharing/messages.js (1)
messages(3-67)
src/elements/content-sharing/hooks/useContactsByEmail.js (2)
src/elements/content-sharing/SharingModal.js (1)
getContactsByEmail(94-94)src/elements/content-sharing/hooks/useContacts.js (2)
noop(20-27)resolveAPICall(34-45)
🪛 Biome (2.1.2)
src/elements/content-sharing/hooks/useContactsByEmail.js
[error] 7-12: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 23-23: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 24-24: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 25-25: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 26-26: return types can only be used in TypeScript files
(parse)
[error] 29-29: Expected an identifier, an array pattern, or an object pattern but instead found '>'.
Expected an identifier, an array pattern, or an object pattern here.
(parse)
[error] 49-49: type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 66-66: type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 67-67: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 73-73: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 76-76: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
⏰ 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). (2)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (6)
src/elements/content-sharing/hooks/useContactService.ts (4)
1-4: LGTM! Clean import additions.The new imports for localization (
useIntl) and email-based contact retrieval (useContactsByEmail,convertUserContactByEmailResponse) are all properly used in the file.
10-16: Excellent localization improvement.The use of
useIntlandformatMessagefollows react-intl v5 best practices and properly internationalizes the group contact label.
24-24: LGTM! API surface expanded correctly.Exposing both
getContactByEmailandgetContactsaligns with the PR objectives and provides a clear, consistent API for consumers.
19-22: ****The hardcoding of
isContentSharingV2Enabled: trueis intentional and correct.useContactServiceis a V2-specific service wrapper that enforces V2 behavior. The underlying hooks (useContactsanduseContactsByEmail) accept this flag as a configurable parameter, butuseContactServiceis designed to wrap them and always use V2. This is evidenced by:
- The hook is imported and used only in
ContentSharingV2.tsx, indicating V2-specific context- The function signature accepts only
(api, itemId, currentUserId)with no feature flag parameter- The design properly separates concerns: low-level hooks are flexible; high-level service enforces specific behavior
If V1 behavior were needed, a separate hook or different wrapper would be created. No changes required.
Likely an incorrect or invalid review comment.
src/elements/content-sharing/hooks/useContactsByEmail.js (2)
85-85: LGTM! Dependency array correctly updated.Including
isContentSharingV2Enabledin the dependencies ensures the effect re-runs when the feature flag changes, properly switching between V1 and V2 implementations.
1-90: Note: Biome lint errors are false positives.The static analysis hints flagging "TypeScript only syntax" are incorrect. This file uses Flow type annotations (note
// @flowat line 1), which Biome is misidentifying as TypeScript. These errors can be safely ignored, or configure Biome to recognize Flow syntax.
reneshen0328
force-pushed
the
create-contact-service-getContactByEmail
branch
from
f172e4c to
4d26ec8
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/elements/content-sharing/utils/convertContactServiceData.ts (1)
59-61: Docstring still missing empty-state detail (follow‑up to prior comment)Add that the function returns {} when no entries are present to match implementation.
Apply:
- * Convert an enterprise users API response into a single internal USM contact object (from the first entry). + * Convert an enterprise users API response into a single internal USM contact object (from the first entry). + * Returns {} when no entries are present.src/elements/content-sharing/hooks/useContactsByEmail.js (1)
66-83: Fix Flow type for filterTerm; indexer is incorrectThe inline type
{ [emails: string]: string }contradicts usage. Use{ emails: Array<string> }.- const updatedGetContactsByEmailFn: GetContactsByEmailFnType = - () => (filterTerm: { [emails: string]: string }) => { + const updatedGetContactsByEmailFn: GetContactsByEmailFnType = + () => (filterTerm: { emails: Array<string> }) => { if (!filterTerm || !Array.isArray(filterTerm.emails) || !filterTerm.emails.length) { return Promise.resolve({}); } const parsedFilterTerm = filterTerm.emails[0];Additionally, if Biome parses this file, ensure it’s configured for Flow or move type annotations off inline params into variable declarations to avoid parse errors.
🧹 Nitpick comments (4)
src/elements/content-sharing/utils/convertContactServiceData.ts (2)
38-56: Group “email” field holds a label; confirm no downstream relies on a valid emailYou’re placing a localized label in the email field to satisfy avatar rendering. Verify no consumers validate/sanitize this as an actual email (dedupe keys, mailto links, etc.). If any do, consider a dedicated displayLabel while keeping email undefined for groups.
62-77: Add explicit TS types to stabilize API surfaceReturn type is currently implicit and unions {} with a contact shape. Define and use a Contact type (or existing USM type) and annotate the function to return Contact | {} for clarity, or prefer Contact | null to avoid ambiguous empty-object checks.
+type USMContact = { + id: string, + email: string, + name: string, + type: string, + value: string, +}; -export const convertUserContactByEmailResponse = contactsApiData => { +export const convertUserContactByEmailResponse = (contactsApiData: { entries?: Array<any> }): USMContact | {} => {src/elements/content-sharing/hooks/useContactsByEmail.js (2)
20-30: Return type union is fine; consider simplifying by returning a single normalized adapterTo reduce branching at call sites, you can always return a V1-shaped function and adapt inputs internally (see V2 comment below).
1-12: Linter/parser config noteImports and annotations are Flow; if your pipeline runs Biome without Flow support, you’ll get parse errors. Either enable Flow parsing or let ESLint/Flow handle these files.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
i18n/en-US.propertiesis excluded by!i18n/**
📒 Files selected for processing (5)
src/elements/content-sharing/SharingModal.js(3 hunks)src/elements/content-sharing/hooks/useContactService.ts(1 hunks)src/elements/content-sharing/hooks/useContactsByEmail.js(2 hunks)src/elements/content-sharing/messages.js(1 hunks)src/elements/content-sharing/utils/convertContactServiceData.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/elements/content-sharing/messages.js
🧰 Additional context used
🧬 Code graph analysis (4)
src/elements/content-sharing/hooks/useContactsByEmail.js (2)
src/elements/content-sharing/SharingModal.js (1)
getContactsByEmail(91-93)src/elements/content-sharing/hooks/useContacts.js (2)
noop(20-27)resolveAPICall(34-45)
src/elements/content-sharing/utils/convertContactServiceData.ts (1)
src/elements/content-sharing/utils/index.ts (2)
convertGroupContactsResponse(4-4)convertUserContactByEmailResponse(5-5)
src/elements/content-sharing/hooks/useContactService.ts (2)
src/elements/content-sharing/utils/convertContactServiceData.ts (3)
convertUserContactsResponse(9-33)convertGroupContactsResponse(38-57)convertUserContactByEmailResponse(62-77)src/elements/content-sharing/messages.js (1)
messages(3-67)
src/elements/content-sharing/SharingModal.js (3)
src/elements/content-sharing/hooks/useSharedLink.js (3)
changeSharedLinkAccessLevel(35-38)changeSharedLinkPermissionLevel(39-42)onSubmitSettings(43-43)src/elements/content-sharing/hooks/useContacts.js (1)
getContacts(19-19)src/elements/content-sharing/hooks/useContactsByEmail.js (1)
getContactsByEmail(27-29)
🪛 Biome (2.1.2)
src/elements/content-sharing/hooks/useContactsByEmail.js
[error] 7-12: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 23-23: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 24-24: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 25-25: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 26-26: return types can only be used in TypeScript files
(parse)
[error] 29-29: Expected an identifier, an array pattern, or an object pattern but instead found '>'.
Expected an identifier, an array pattern, or an object pattern here.
(parse)
[error] 49-49: type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 66-66: type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 67-67: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 73-73: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 76-76: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
src/elements/content-sharing/SharingModal.js
[error] 93-93: Expected an identifier, an array pattern, or an object pattern but instead found '>'.
Expected an identifier, an array pattern, or an object pattern here.
(parse)
[error] 193-193: type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 201-201: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
⏰ 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/SharingModal.js (2)
84-87: No functional change; state initializers look fineRefactor is formatting-only and OK.
91-93: Union typing for getContactsByEmail is fine; ensure downstream prop typing matchesUnifiedShareModal must accept a function of either signature. Please confirm its prop types and call sites handle both shapes.
src/elements/content-sharing/hooks/useContactService.ts (2)
10-17: Localized group label wiring looks goodPassing a localized label to the group converter is correct and keeps UI consistent.
19-25: Confirm mixed return shapes are handled by consumersgetContactByEmail returns a single contact object (or {}), while getContacts returns arrays. Ensure all call sites (USM/contact service adapters) branch correctly on the path and don’t assume an array.
src/elements/content-sharing/hooks/useContactsByEmail.js (2)
35-46: Empty-state normalization for V1 vs V2resolveAPICall returns {} when no entries; legacy V1 callers may expect an array. Confirm all consumers of V1 path handle
{}(or switch V1 to return[]for empty to preserve the original contract).
85-88: LGTM on effect guardEarly-return when function exists prevents redefinition; dependencies look correct.
What
Before this PR: when user type in already exists managed user's email into the collaboration section and press enter, the collaborator will be created as an external (new) user.
After this PR:
getMarkerBasedUsersAPI(created and used by existing contentSharingV1) is called, the contactService object which returnsgetContactByEmailis passed in successfully, and the in the UI when user type in already exists managed user's email into the collaboration section and press enter, the collaborator should be returned as an API entry hence treat the email input as a managed user (without the isExternal icon).Testing:
Screen.Recording.2025-10-16.at.9.16.13.PM.mov
Summary by CodeRabbit
New Features
Public API
Tests