feat(content-sharing): convert api response for collabs #4322
Conversation
WalkthroughContentSharingV2 now performs staged data fetches: fetchItem → fetchCurrentUser → fetchCollaborators → fetchAvatars, converts collaboration responses into enriched collaborator objects (with avatar URLs) and passes them to UnifiedShareModal; new fetcher modules, types, mocks, tests, and story play functions were added. I pity the fool who doesn't! Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CSV2 as ContentSharingV2
participant Fetchers as Fetchers (fetchItem, fetchCurrentUser, fetchCollaborators, fetchAvatars)
participant Users as UsersAPI
participant Utils as convertCollabsResponse
participant USM as UnifiedShareModal
User->>CSV2: open modal
CSV2->>Fetchers: fetchItem(itemID, itemType)
Fetchers-->>CSV2: item
CSV2->>Fetchers: fetchCurrentUser(itemID)
Fetchers-->>CSV2: currentUser
CSV2->>Fetchers: fetchCollaborators(itemID, itemType)
Fetchers-->>CSV2: collaborations
CSV2->>Fetchers: fetchAvatars(itemID, collaborations)
Fetchers->>Users: getAvatarUrlWithAccessToken per collaborator
Users-->>Fetchers: avatarURLMap
Fetchers-->>CSV2: avatarURLMap
CSV2->>Utils: convertCollabsResponse(collaborations, avatarURLMap)
Utils-->>CSV2: collaborators[]
CSV2->>USM: render({ collaborators })
Note right of CSV2: state resets and guards run on API change
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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: 9
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/constants.js (1)
1-79: Consolidate API_TO_USM_CLASSIFICATION_COLORS_MAP into a shared module, fool! Remove the duplicate incontent-sharing/constants.jsand import it from one source to keep a single source of truth. I pity the fool who doesn’t!
🧹 Nitpick comments (2)
src/elements/content-sharing/ContentSharingV2.tsx (2)
76-76: Redundant guard condition, fool!Line 76 checks
item || sharedLink, but ifitemis set,sharedLinkis also set (seehandleGetItemSuccess). So checking both is redundant, sucka! You can simplify the guard to justitem!Apply this diff to simplify the guard:
- if (!api || isEmpty(api) || item || sharedLink) return; + if (!api || isEmpty(api) || item) return;
86-86: Redundant guard condition, fool!Line 86 checks
item || sharedLink, but as I said before, checking both is redundant, sucka! Simplify the guard to justitem!Apply this diff to simplify the guard:
- if (!api || isEmpty(api) || item || sharedLink || currentUser) return; + if (!api || isEmpty(api) || item || currentUser) return;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
src/elements/content-sharing/ContentSharingV2.tsx(4 hunks)src/elements/content-sharing/__tests__/ContentSharingV2.test.tsx(4 hunks)src/elements/content-sharing/apis/fetchAvatars.ts(1 hunks)src/elements/content-sharing/apis/fetchCollaborators.ts(1 hunks)src/elements/content-sharing/apis/fetchCurrentUser.ts(1 hunks)src/elements/content-sharing/apis/fetchItem.ts(1 hunks)src/elements/content-sharing/apis/index.ts(1 hunks)src/elements/content-sharing/constants.js(2 hunks)src/elements/content-sharing/stories/tests/ContentSharingV2-visual.stories.tsx(1 hunks)src/elements/content-sharing/types.js(2 hunks)src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js(3 hunks)src/elements/content-sharing/utils/__tests__/convertCollaborators.test.ts(1 hunks)src/elements/content-sharing/utils/__tests__/getAllowedAccessLevels.test.ts(1 hunks)src/elements/content-sharing/utils/constants.ts(0 hunks)src/elements/content-sharing/utils/convertCollaborators.ts(1 hunks)src/elements/content-sharing/utils/convertItemResponse.ts(2 hunks)src/elements/content-sharing/utils/getAllowedAccessLevels.ts(1 hunks)src/elements/content-sharing/utils/index.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/elements/content-sharing/utils/constants.ts
🧰 Additional context used
🧬 Code graph analysis (12)
src/elements/content-sharing/apis/fetchCurrentUser.ts (1)
src/elements/content-sharing/apis/index.ts (1)
fetchCurrentUser(3-3)
src/elements/content-sharing/utils/getAllowedAccessLevels.ts (1)
src/elements/content-sharing/constants.js (6)
ANYONE_IN_COMPANY(82-82)ANYONE_IN_COMPANY(82-82)ANYONE_WITH_LINK(81-81)ANYONE_WITH_LINK(81-81)PEOPLE_IN_ITEM(83-83)PEOPLE_IN_ITEM(83-83)
src/elements/content-sharing/utils/convertCollaborators.ts (3)
src/elements/content-sharing/SharingModal.js (1)
currentUserID(77-77)src/elements/content-sharing/__tests__/useAvatars.test.js (1)
avatarURLMap(26-26)src/features/unified-share-modal/utils/convertData.js (1)
collabsAPIData(431-431)
src/elements/content-sharing/__tests__/ContentSharingV2.test.tsx (1)
src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js (2)
MOCK_ITEM(14-18)MOCK_ITEM(14-18)
src/elements/content-sharing/constants.js (2)
src/features/unified-share-modal/utils/convertData.js (1)
API_TO_USM_CLASSIFICATION_COLORS_MAP(100-109)src/features/classification/constants.js (8)
CLASSIFICATION_COLOR_ID_0(78-78)CLASSIFICATION_COLOR_ID_1(79-79)CLASSIFICATION_COLOR_ID_2(80-80)CLASSIFICATION_COLOR_ID_3(81-81)CLASSIFICATION_COLOR_ID_4(82-82)CLASSIFICATION_COLOR_ID_5(83-83)CLASSIFICATION_COLOR_ID_6(84-84)CLASSIFICATION_COLOR_ID_7(85-85)
src/elements/content-sharing/utils/__tests__/convertCollaborators.test.ts (2)
src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js (12)
collabUser1(38-43)collabUser1(38-43)mockAvatarURLMap(30-32)mockAvatarURLMap(30-32)mockOwnerEmail(34-34)mockOwnerEmail(34-34)mockCurrentUserID(36-36)mockCurrentUserID(36-36)collabUser2(45-50)collabUser2(45-50)collabUser3(52-57)collabUser3(52-57)src/elements/content-sharing/utils/convertCollaborators.ts (2)
convertCollab(15-50)convertCollabsResponse(52-63)
src/elements/content-sharing/apis/fetchItem.ts (1)
src/elements/content-sharing/constants.js (2)
CONTENT_SHARING_ITEM_FIELDS(45-59)CONTENT_SHARING_ITEM_FIELDS(45-59)
src/elements/content-sharing/apis/fetchAvatars.ts (2)
src/elements/content-sharing/apis/index.ts (1)
fetchAvatars(1-1)src/elements/content-sharing/__tests__/useAvatars.test.js (1)
avatarURLMap(26-26)
src/elements/content-sharing/utils/convertItemResponse.ts (1)
src/elements/content-sharing/utils/getAllowedAccessLevels.ts (1)
getAllowedAccessLevels(14-35)
src/elements/content-sharing/ContentSharingV2.tsx (6)
src/features/presence/PresenceAvatarList.tsx (1)
Collaborator(10-18)src/elements/content-sharing/apis/fetchItem.ts (1)
fetchItem(8-22)src/elements/content-sharing/apis/fetchCurrentUser.ts (1)
fetchCurrentUser(7-15)src/elements/content-sharing/apis/fetchCollaborators.ts (1)
fetchCollaborators(6-21)src/elements/content-sharing/apis/fetchAvatars.ts (1)
fetchAvatars(3-22)src/elements/content-sharing/utils/convertCollaborators.ts (1)
convertCollabsResponse(52-63)
src/elements/content-sharing/stories/tests/ContentSharingV2-visual.stories.tsx (2)
src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js (6)
mockAPIWithoutSharedLink(180-184)mockAPIWithoutSharedLink(180-184)mockAPIWithSharedLink(175-179)mockAPIWithSharedLink(175-179)mockAPIWithCollaborators(185-189)mockAPIWithCollaborators(185-189)src/elements/content-sharing/stories/ContentSharingV2.stories.tsx (1)
withSharedLink(9-13)
src/elements/content-sharing/utils/__tests__/getAllowedAccessLevels.test.ts (1)
src/elements/content-sharing/utils/getAllowedAccessLevels.ts (1)
getAllowedAccessLevels(14-35)
⏰ 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 (18)
src/elements/content-sharing/utils/index.ts (1)
2-2: LGTM, fool!The export of
convertCollabsResponseis clean and follows the existing pattern in this barrel file. I pity the fool who doesn't appreciate a well-organized public API surface!src/elements/content-sharing/constants.js (1)
81-83: LGTM, fool!These new sharing access constants are clean and well-named. They align with the unified-share-modal terminology. I pity the fool who doesn't appreciate good naming conventions!
src/elements/content-sharing/utils/convertItemResponse.ts (1)
2-2: LGTM, fool!The import change aligns with the constant relocation, and the updated
getAllowedAccessLevelscall correctly passes the newdisabledReasonsparameter. I pity the fool who doesn't appreciate a clean refactor!Also applies to: 12-12, 63-66
src/elements/content-sharing/utils/__tests__/getAllowedAccessLevels.test.ts (1)
25-43: LGTM, fool!These new tests properly verify the
disabledReasonshandling ingetAllowedAccessLevels. I pity the fool who doesn't write comprehensive tests! The coverage for the new feature looks solid.src/elements/content-sharing/apis/index.ts (1)
1-4: LGTM, fool!This barrel file is clean and well-organized. I pity the fool who doesn't appreciate a properly structured API surface!
src/elements/content-sharing/apis/fetchCurrentUser.ts (1)
7-15: Confirm correctfieldsparameter format: I pity the fool who doesn't verify whethergetUserexpects a comma-separated string (from.toString()) or an array.src/elements/content-sharing/utils/convertCollaborators.ts (1)
1-13: LGTM, fool!The imports and interface definition are clean and well-typed. I got no beef with this setup!
src/elements/content-sharing/utils/getAllowedAccessLevels.ts (2)
1-12: LGTM, fool!The imports and the
API_TO_USM_ACCESS_LEVEL_MAPconstant are clean and well-structured. I got no beef with this setup!
22-34: Ignore the undefined-guard suggestion:levelsisAccessLevelType[]andAPI_TO_USM_ACCESS_LEVEL_MAPexhaustively covers those values, soapiLevelnever ends up undefined. I pity the fool who thinks otherwise!Likely an incorrect or invalid review comment.
src/elements/content-sharing/ContentSharingV2.tsx (6)
1-14: LGTM, fool!The imports are clean and well-structured. I got no beef with this setup!
42-48: LGTM, fool!The state declarations for
avatarURLMap,collaborators, andcollaboratorsDataare clean and well-typed. I got no beef with this setup!
64-72: LGTM, fool!The reset state effect is correct and handles the API change scenario properly. I got no beef with this, sucka!
103-114: LGTM, fool!The fetch collaborators effect is correct and includes proper error handling. I got no beef with this, sucka!
127-132: LGTM, fool!The convert collaborators effect is correct and handles the data transformation properly. I got no beef with this, sucka!
136-153: LGTM, fool!The render function is correct and the new
collaboratorsprop is passed toUnifiedShareModalproperly. I got no beef with this, sucka!src/elements/content-sharing/stories/tests/ContentSharingV2-visual.stories.tsx (3)
1-27: LGTM, fool!The imports and
withModernizationstory are clean and the play function tests the UI interactions properly. I got no beef with this setup, sucka!
29-47: LGTM, fool!The
withSharedLinkstory is clean and the play function tests the shared link UI interactions properly. I got no beef with this, sucka!
49-64: LGTM, fool!The
withCollaboratorsstory is clean and the play function tests the collaborators UI interactions properly. I got no beef with this, sucka!
reneshen0328
force-pushed
the
feat-convert-api-response-for-collabs
branch
from
c74ab36 to
a7ecc91
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/elements/content-sharing/ContentSharingV2.tsx (3)
77-80: Missing error handling for fetchItem, fool!I pity the fool who doesn't handle errors! Lines 78-79 call
fetchIteminside an async IIFE without error handling, sucka. IffetchItemthrows, the error is unhandled and may cause a crash. Add a try-catch block to handle errors gracefully!Apply this diff to add error handling:
(async () => { + try { const itemData = await fetchItem({ api, itemID, itemType }); handleGetItemSuccess(itemData); + } catch (error) { + console.error('Failed to fetch item:', error); + } })();
95-98: Missing error handling for fetchCurrentUser, fool!I pity the fool who doesn't handle errors! Lines 96-97 call
fetchCurrentUserinside an async IIFE without error handling, sucka. IffetchCurrentUserthrows, the error is unhandled and may cause a crash. Add a try-catch block to handle errors gracefully!Apply this diff to add error handling:
(async () => { + try { const userData = await fetchCurrentUser({ api, itemID }); getUserSuccess(userData); + } catch (error) { + console.error('Failed to fetch current user:', error); + } })();
119-122: Missing error handling for fetchAvatars, fool!I pity the fool who doesn't handle errors! Lines 120-121 call
fetchAvatarsinside an async IIFE without error handling, sucka. IffetchAvatarsthrows, the error is unhandled and may cause a crash. Add a try-catch block to handle errors gracefully!Apply this diff to add error handling:
(async () => { + try { const response = await fetchAvatars({ api, itemID, collaborators: collaboratorsData.entries }); setAvatarURLMap(response); + } catch (error) { + console.error('Failed to fetch avatars:', error); + setAvatarURLMap({}); + } })();
🧹 Nitpick comments (2)
src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js (1)
61-76: Prefer diverse roles in mock collaborations, fool!I pity the fool who tests only one role! All entries in
MOCK_COLLABORATIONS_RESPONSEhaverole: 'editor', which limits test coverage, sucka. Adding variety (e.g., 'viewer', 'owner') would better exercise role-dependent logic inconvertCollaband related utilities.Apply this diff to add role variety:
export const MOCK_COLLABORATIONS_RESPONSE = { entries: MOCK_COLLABORATORS.map(user => ({ id: `record_${user.id}`, accessible_by: user, expires_at: user.expires_at, created_by: { id: mockCurrentUserID, login: mockOwnerEmail, name: 'Astronaut Otter', type: 'user', }, - role: 'editor', + role: user.id === 456 ? 'editor' : user.id === 457 ? 'viewer' : 'owner', status: 'accepted', type: user.type, })), };src/elements/content-sharing/constants.js (1)
70-79: Duplicate mapping detected, fool!I pity the fool who maintains duplicate code!
API_TO_USM_CLASSIFICATION_COLORS_MAPat lines 70-79 is identical to the mapping insrc/features/unified-share-modal/utils/convertData.js(lines 99-108), sucka. This duplication makes maintenance harder and increases the risk of inconsistency if one copy is updated but not the other.Prefer one of these solutions:
Option 1: Extract to a shared constants module (recommended):
Create
src/shared/constants/classificationColors.js:import { CLASSIFICATION_COLOR_ID_0, CLASSIFICATION_COLOR_ID_1, CLASSIFICATION_COLOR_ID_2, CLASSIFICATION_COLOR_ID_3, CLASSIFICATION_COLOR_ID_4, CLASSIFICATION_COLOR_ID_5, CLASSIFICATION_COLOR_ID_6, CLASSIFICATION_COLOR_ID_7, } from '../../features/classification/constants'; import { bdlDarkBlue50, bdlGray20, bdlGreenLight50, bdlLightBlue50, bdlOrange50, bdlPurpleRain50, bdlWatermelonRed50, bdlYellow50, } from '../../styles/variables'; export const API_TO_USM_CLASSIFICATION_COLORS_MAP = { [bdlYellow50]: CLASSIFICATION_COLOR_ID_0, [bdlOrange50]: CLASSIFICATION_COLOR_ID_1, [bdlWatermelonRed50]: CLASSIFICATION_COLOR_ID_2, [bdlPurpleRain50]: CLASSIFICATION_COLOR_ID_3, [bdlLightBlue50]: CLASSIFICATION_COLOR_ID_4, [bdlDarkBlue50]: CLASSIFICATION_COLOR_ID_5, [bdlGreenLight50]: CLASSIFICATION_COLOR_ID_6, [bdlGray20]: CLASSIFICATION_COLOR_ID_7, };Then import from both locations:
-import { - CLASSIFICATION_COLOR_ID_0, - ... -} from '../../features/classification/constants'; -... -export const API_TO_USM_CLASSIFICATION_COLORS_MAP = { ... }; +import { API_TO_USM_CLASSIFICATION_COLORS_MAP } from '../../shared/constants/classificationColors'; +export { API_TO_USM_CLASSIFICATION_COLORS_MAP };Option 2: Import from the existing location if module dependencies allow:
-import { - CLASSIFICATION_COLOR_ID_0, - ... -} from '../../features/classification/constants'; -... -export const API_TO_USM_CLASSIFICATION_COLORS_MAP = { ... }; +export { API_TO_USM_CLASSIFICATION_COLORS_MAP } from '../../features/unified-share-modal/utils/convertData';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
src/elements/content-sharing/ContentSharingV2.tsx(4 hunks)src/elements/content-sharing/__tests__/ContentSharingV2.test.tsx(4 hunks)src/elements/content-sharing/apis/fetchAvatars.ts(1 hunks)src/elements/content-sharing/apis/fetchCollaborators.ts(1 hunks)src/elements/content-sharing/apis/fetchCurrentUser.ts(1 hunks)src/elements/content-sharing/apis/fetchItem.ts(1 hunks)src/elements/content-sharing/apis/index.ts(1 hunks)src/elements/content-sharing/constants.js(2 hunks)src/elements/content-sharing/stories/tests/ContentSharingV2-visual.stories.tsx(1 hunks)src/elements/content-sharing/types.js(2 hunks)src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js(3 hunks)src/elements/content-sharing/utils/__tests__/convertCollaborators.test.ts(1 hunks)src/elements/content-sharing/utils/__tests__/getAllowedAccessLevels.test.ts(1 hunks)src/elements/content-sharing/utils/constants.ts(0 hunks)src/elements/content-sharing/utils/convertCollaborators.ts(1 hunks)src/elements/content-sharing/utils/convertItemResponse.ts(2 hunks)src/elements/content-sharing/utils/getAllowedAccessLevels.ts(1 hunks)src/elements/content-sharing/utils/index.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/elements/content-sharing/utils/constants.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- src/elements/content-sharing/apis/fetchCurrentUser.ts
- src/elements/content-sharing/utils/convertItemResponse.ts
- src/elements/content-sharing/utils/getAllowedAccessLevels.ts
- src/elements/content-sharing/tests/ContentSharingV2.test.tsx
- src/elements/content-sharing/apis/index.ts
- src/elements/content-sharing/types.js
- src/elements/content-sharing/apis/fetchCollaborators.ts
🧰 Additional context used
🧬 Code graph analysis (7)
src/elements/content-sharing/utils/__tests__/getAllowedAccessLevels.test.ts (2)
src/elements/content-sharing/utils/getAllowedAccessLevels.ts (1)
getAllowedAccessLevels(14-35)src/elements/content-sharing/utils/index.ts (1)
getAllowedAccessLevels(3-3)
src/elements/content-sharing/constants.js (2)
src/features/unified-share-modal/utils/convertData.js (1)
API_TO_USM_CLASSIFICATION_COLORS_MAP(100-109)src/features/classification/constants.js (8)
CLASSIFICATION_COLOR_ID_0(78-78)CLASSIFICATION_COLOR_ID_1(79-79)CLASSIFICATION_COLOR_ID_2(80-80)CLASSIFICATION_COLOR_ID_3(81-81)CLASSIFICATION_COLOR_ID_4(82-82)CLASSIFICATION_COLOR_ID_5(83-83)CLASSIFICATION_COLOR_ID_6(84-84)CLASSIFICATION_COLOR_ID_7(85-85)
src/elements/content-sharing/utils/__tests__/convertCollaborators.test.ts (2)
src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js (12)
collabUser1(38-43)collabUser1(38-43)mockAvatarURLMap(30-32)mockAvatarURLMap(30-32)mockOwnerEmail(34-34)mockOwnerEmail(34-34)mockCurrentUserID(36-36)mockCurrentUserID(36-36)collabUser2(45-50)collabUser2(45-50)collabUser3(52-57)collabUser3(52-57)src/elements/content-sharing/utils/convertCollaborators.ts (2)
convertCollab(15-50)convertCollabsResponse(52-62)
src/elements/content-sharing/apis/fetchItem.ts (1)
src/elements/content-sharing/constants.js (2)
CONTENT_SHARING_ITEM_FIELDS(45-59)CONTENT_SHARING_ITEM_FIELDS(45-59)
src/elements/content-sharing/ContentSharingV2.tsx (5)
src/elements/content-sharing/apis/fetchItem.ts (1)
fetchItem(8-22)src/elements/content-sharing/apis/fetchCurrentUser.ts (1)
fetchCurrentUser(7-15)src/elements/content-sharing/apis/fetchCollaborators.ts (1)
fetchCollaborators(6-21)src/elements/content-sharing/apis/fetchAvatars.ts (1)
fetchAvatars(3-22)src/elements/content-sharing/utils/convertCollaborators.ts (1)
convertCollabsResponse(52-62)
src/elements/content-sharing/utils/convertCollaborators.ts (3)
src/elements/content-sharing/SharingModal.js (1)
currentUserID(77-77)src/elements/content-sharing/__tests__/useAvatars.test.js (1)
avatarURLMap(26-26)src/elements/content-sharing/utils/index.ts (1)
convertCollabsResponse(2-2)
src/elements/content-sharing/stories/tests/ContentSharingV2-visual.stories.tsx (2)
src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js (6)
mockAPIWithoutSharedLink(181-185)mockAPIWithoutSharedLink(181-185)mockAPIWithSharedLink(176-180)mockAPIWithSharedLink(176-180)mockAPIWithCollaborators(186-190)mockAPIWithCollaborators(186-190)src/elements/content-sharing/stories/ContentSharingV2.stories.tsx (1)
withSharedLink(9-13)
⏰ 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 (30)
src/elements/content-sharing/utils/__tests__/getAllowedAccessLevels.test.ts (1)
25-43: DisabledReasons mapping verified: ACCESS_OPEN→peopleWithTheLink, ACCESS_COMPANY→anyoneInCompany, ACCESS_COLLAB→peopleInThisItem—tests align with API_TO_USM_ACCESS_LEVEL_MAP. Suggest adding edge-case coverage for invalid keys, multiple disabledReasons, and null/undefined inputs.src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js (6)
38-59: LGTM, fool!The collaborator user mocks are well-structured and provide good test data coverage for different collaboration scenarios, sucka!
82-102: LGTM, fool!The updates to
DEFAULT_USER_API_RESPONSEandDEFAULT_ITEM_API_RESPONSEalign with the new features for collaborators and disabled reasons, sucka!
109-112: LGTM, fool!The
MOCK_ITEM_API_RESPONSE_WITH_COLLABORATORSmock correctly includes thecollaboratorsfield to support testing scenarios with collaborators, sucka!
141-147: LGTM, fool!I pity the fool who forgets to add stubs! The
getAvatarUrlWithAccessTokenmethod at line 147 correctly provides the avatar URL mapping, resolving the past review concern, sucka!
150-172: LGTM, fool!The new
getFileCollaborationsAPIandgetFolderCollaborationsAPImethods provide consistent mock implementations for collaborator fetching, sucka!
176-190: LGTM, fool!The pre-configured mock APIs (
mockAPIWithSharedLink,mockAPIWithoutSharedLink,mockAPIWithCollaborators) provide comprehensive coverage for different testing scenarios, sucka!src/elements/content-sharing/utils/index.ts (1)
2-2: LGTM, fool!The new export
convertCollabsResponsecorrectly expands the module's public API surface to include collaboration conversion utilities, sucka!src/elements/content-sharing/constants.js (2)
1-20: LGTM, fool!The imports correctly bring in the classification color IDs and variables needed for the new mapping, sucka!
81-83: LGTM, fool!The new constants
ANYONE_WITH_LINK,ANYONE_IN_COMPANY, andPEOPLE_IN_ITEMare well-named and support the content-sharing enhancements, sucka!src/elements/content-sharing/apis/fetchItem.ts (2)
8-13: LGTM, fool!The
fetchItemfunction return type is nowPromise<Item | null>, which resolves the past review concern about type mismatch, sucka! The TYPE_FILE branch correctly callsgetFileAPI().getFilewith the appropriate fields.
15-21: LGTM, fool!The TYPE_FOLDER branch and default return correctly handle folder fetching and unhandled item types, sucka!
src/elements/content-sharing/apis/fetchAvatars.ts (3)
3-5: LGTM, fool!The initialization of
usersAPIandavatarURLMapis correct, sucka!
20-21: LGTM, fool!The use of
Promise.allto wait for all avatar fetches and the return ofavatarURLMapis correct, sucka!
7-18: Ignore async verification for getAvatarUrlWithAccessToken
I pity the fool who doubts this—getAvatarUrlWithAccessTokenis declaredasyncand returns aPromiseinsrc/api/Users.js.Likely an incorrect or invalid review comment.
src/elements/content-sharing/utils/convertCollaborators.ts (2)
15-50: LGTM, fool!The
convertCollabfunction correctly guards against null/undefined collabs and empty roles, resolving the past review concerns, sucka! The logic forisCurrentUser,isExternal, andavatarUrlis sound.
52-62: LGTM, fool!The
convertCollabsResponsefunction correctly handles missingcreated_bywith a fallback (|| {}), resolving the past review concern, sucka! The flatMap logic correctly filters out null results fromconvertCollab.src/elements/content-sharing/ContentSharingV2.tsx (7)
5-14: LGTM, fool!The new imports for types and fetch utilities are correctly added to support the enhanced collaboration flow, sucka!
42-48: LGTM, fool!The new state variables for
avatarURLMap,collaborators, andcollaboratorsDatacorrectly support the enhanced collaboration and avatar fetching flow, sucka!
51-60: LGTM, fool!The
handleGetItemSuccesscallback correctly processes the item response and sets state, sucka!
63-71: LGTM, fool!The state reset logic correctly includes the new
avatarURLMap,collaborators, andcollaboratorsDatastate variables, sucka!
101-113: LGTM, fool!The
fetchCollaboratorsuseEffect correctly includes error handling with a try-catch block and sets a safe fallback state on error, sucka!
126-131: LGTM, fool!The useEffect that converts collaborators using
convertCollabsResponsecorrectly waits for bothcollaboratorsDataandavatarURLMapto be ready, sucka!
133-142: LGTM, fool!The
configobject and the newcollaboratorsprop correctly integrate the enhanced collaboration data into theUnifiedShareModal, sucka!src/elements/content-sharing/utils/__tests__/convertCollaborators.test.ts (2)
14-159: LGTM, fool!The
convertCollabtest suite provides comprehensive coverage for valid collaborations, edge cases (null/undefined, non-accepted status), current user identification, external user identification, avatar URL map variants, and missing expiration dates, sucka!
161-247: LGTM, fool!The
convertCollabsResponsetest suite provides comprehensive coverage for converting valid collaborations, handling empty entries, and processing null avatar URL maps, sucka! The test correctly verifies that only accepted collaborations are included in the result.src/elements/content-sharing/stories/tests/ContentSharingV2-visual.stories.tsx (4)
17-26: LGTM, fool — once you fix that Storybook version!The interaction testing logic is solid as gold chains: proper async/await, good accessibility queries with
getByRole, and clear test flow (find button → click → verify modal elements). I appreciate the organized structure, sucka!Just remember: this whole play function depends on fixing that Storybook v4 incompatibility I flagged above. Get that sorted, and you'll be ready to test!
33-46: I love it when a plan comes together, fool!Reusing
withModernization.play(context)shows you ain't duplicating code like some jibber-jabber fool! The test flow is clean: verify shared link UI → click access button → confirm the "Invited people only" option is properly disabled. This aligns with the PR's disabledReason feature.Solid work, sucka!
53-63: Outstanding work, fool!This test don't mess around - it verifies every piece of that collaborators UI like I verify every member of my team! You're checking the avatars button, the Manage All link, the grid structure, a specific collaborator row, AND the Done button. That's thoroughness I can respect, sucka!
The composition pattern keeps working for you too - reusing
withModernization.playkeeps your code tight and right!
66-74: Story metadata looks good, fool!The default export configuration is solid - proper title, component reference, and common args for the visual regression tests. No changes needed here, sucka!
src/elements/content-sharing/stories/tests/ContentSharingV2-visual.stories.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/elements/content-sharing/apis/__tests__/fetchItem.test.ts (1)
1-51: Good coverage, but I pity the fool who skips the folder error path!These tests nail the file/folder success paths and unsupported types. However, only the file error path is tested (lines 40-49). Consider adding a corresponding test for folder API errors to ensure symmetric coverage, fool!
Add a folder error test after line 49:
+ test('should handle folder API errors', async () => { + const errorMock = jest.fn().mockImplementation((itemID, successCallback, errorCallback) => { + errorCallback(new Error('Folder API Error')); + }); + const errorAPIMock = createItemAPIMock({ getFile: getDefaultFileMock }, { getFolderFields: errorMock }); + + await expect(fetchItem({ api: errorAPIMock, itemID: MOCK_ITEM.id, itemType: TYPE_FOLDER })).rejects.toThrow( + 'Folder API Error', + ); + });src/elements/content-sharing/apis/__tests__/testUtils.ts (1)
1-9: Question the 500ms delay, fool!The
createSuccessMockintroduces a 500ms timeout before resolving. Unless this delay is specifically testing async timing behavior, it unnecessarily slows down your test suite. I pity the fool who waits around when they don't have to!Consider removing the timeout if it's not testing timing-specific behavior:
-export const createSuccessMock = responseFromAPI => (itemID, successFn) => { - return new Promise(resolve => { - setTimeout(() => { - resolve(responseFromAPI); - }, 500); - }).then(response => { - successFn(response); - }); -}; +export const createSuccessMock = responseFromAPI => (itemID, successFn) => { + return Promise.resolve(responseFromAPI).then(response => { + successFn(response); + }); +};src/elements/content-sharing/apis/__tests__/fetchCollaborators.test.ts (1)
1-45: Good start, but I pity the fool who forgets error tests!These tests cover the success paths for file/folder collaborators and correctly handle unsupported types. However, there's no test for API error scenarios (similar to what's in
fetchItem.test.tsandfetchCurrentUser.test.ts). Consider adding error handling tests for consistency, fool!Add error handling tests after line 43:
+ test('should handle file collaborations API errors', async () => { + const errorMock = jest.fn().mockImplementation((itemID, successCallback, errorCallback) => { + errorCallback(new Error('File Collaborations API Error')); + }); + const errorAPIMock = createCollabAPIMock( + { getCollaborations: errorMock }, + { getCollaborations: getDefaultFolderCollabMock }, + ); + + await expect(fetchCollaborators({ api: errorAPIMock, itemID: MOCK_ITEM.id, itemType: TYPE_FILE })).rejects.toThrow( + 'File Collaborations API Error', + ); + }); + + test('should handle folder collaborations API errors', async () => { + const errorMock = jest.fn().mockImplementation((itemID, successCallback, errorCallback) => { + errorCallback(new Error('Folder Collaborations API Error')); + }); + const errorAPIMock = createCollabAPIMock( + { getCollaborations: getDefaultFileCollabMock }, + { getCollaborations: errorMock }, + ); + + await expect(fetchCollaborators({ api: errorAPIMock, itemID: MOCK_ITEM.id, itemType: TYPE_FOLDER })).rejects.toThrow( + 'Folder Collaborations API Error', + ); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/elements/content-sharing/apis/__tests__/fetchAvatars.test.ts(1 hunks)src/elements/content-sharing/apis/__tests__/fetchCollaborators.test.ts(1 hunks)src/elements/content-sharing/apis/__tests__/fetchCurrentUser.test.ts(1 hunks)src/elements/content-sharing/apis/__tests__/fetchItem.test.ts(1 hunks)src/elements/content-sharing/apis/__tests__/testUtils.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-02T16:47:50.687Z
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.687Z
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/apis/__tests__/fetchCurrentUser.test.ts
🧬 Code graph analysis (5)
src/elements/content-sharing/apis/__tests__/fetchItem.test.ts (1)
src/elements/content-sharing/apis/__tests__/testUtils.ts (2)
createSuccessMock(1-9)createItemAPIMock(11-14)
src/elements/content-sharing/apis/__tests__/fetchCurrentUser.test.ts (1)
src/elements/content-sharing/apis/__tests__/testUtils.ts (2)
createSuccessMock(1-9)createUsersAPIMock(21-23)
src/elements/content-sharing/apis/__tests__/fetchCollaborators.test.ts (1)
src/elements/content-sharing/apis/__tests__/testUtils.ts (2)
createSuccessMock(1-9)createCollabAPIMock(16-19)
src/elements/content-sharing/apis/__tests__/fetchAvatars.test.ts (1)
src/elements/content-sharing/apis/__tests__/testUtils.ts (2)
createSuccessMock(1-9)createUsersAPIMock(21-23)
src/elements/content-sharing/apis/__tests__/testUtils.ts (1)
src/elements/content-sidebar/versions/__tests__/VersionsSidebarAPI.test.js (1)
fileAPI(7-10)
⏰ 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 (3)
src/elements/content-sharing/apis/__tests__/fetchCurrentUser.test.ts (1)
1-34: LGTM, fool! Solid test coverage for fetchCurrentUser.These tests cover the critical paths—success and error handling—and properly verify API invocations with correct field parameters. The mock setup is clean and reusable. I pity the fool who breaks this flow!
src/elements/content-sharing/apis/__tests__/fetchAvatars.test.ts (1)
1-95: Excellent test coverage, fool! I'm impressed!These tests are comprehensive—covering success, partial failures, missing data, and empty arrays. The error handling verification ensures failed avatar fetches return
nullwhile allowing successful ones to proceed. I pity the fool who doesn't write tests like these!src/elements/content-sharing/apis/__tests__/testUtils.ts (1)
11-23: LGTM! Clean mock factories, fool!The
createItemAPIMock,createCollabAPIMock, andcreateUsersAPIMockfactories are well-structured and provide the necessary mock surfaces for testing file/folder APIs, collaborations APIs, and users API respectively. I pity the fool who doesn't reuse these utilities!
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/elements/content-sharing/ContentSharingV2.tsx (1)
133-135: Drop the debug log, sucker!I pity the fool who ships
console.login prod. Yank it before this hits master.- console.log('collaborators', collaborators);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/elements/content-sharing/ContentSharingV2.tsx(3 hunks)src/elements/content-sharing/__tests__/ContentSharingV2.test.tsx(5 hunks)src/elements/content-sharing/apis/fetchAvatars.ts(1 hunks)src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/elements/content-sharing/apis/fetchAvatars.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-02T16:47:50.687Z
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.687Z
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/__mocks__/ContentSharingV2Mocks.js
🧬 Code graph analysis (2)
src/elements/content-sharing/ContentSharingV2.tsx (6)
src/features/presence/PresenceAvatarList.tsx (1)
Collaborator(10-18)src/elements/content-sharing/apis/fetchItem.ts (1)
fetchItem(8-22)src/elements/content-sharing/apis/fetchCurrentUser.ts (1)
fetchCurrentUser(7-15)src/elements/content-sharing/apis/fetchCollaborators.ts (1)
fetchCollaborators(6-21)src/elements/content-sharing/apis/fetchAvatars.ts (1)
fetchAvatars(3-22)src/elements/content-sharing/utils/convertCollaborators.ts (1)
convertCollabsResponse(52-62)
src/elements/content-sharing/__tests__/ContentSharingV2.test.tsx (1)
src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js (6)
MOCK_COLLABORATIONS_RESPONSE(65-80)MOCK_COLLABORATIONS_RESPONSE(65-80)mockAvatarURLMap(30-32)mockAvatarURLMap(30-32)MOCK_ITEM(14-18)MOCK_ITEM(14-18)
⏰ 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
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/elements/content-sharing/ContentSharingV2.tsx(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/elements/content-sharing/ContentSharingV2.tsx (5)
src/elements/content-sharing/apis/fetchItem.ts (1)
fetchItem(8-22)src/elements/content-sharing/apis/fetchCurrentUser.ts (1)
fetchCurrentUser(7-15)src/elements/content-sharing/apis/fetchCollaborators.ts (1)
fetchCollaborators(6-21)src/elements/content-sharing/apis/fetchAvatars.ts (1)
fetchAvatars(3-22)src/elements/content-sharing/utils/convertCollaborators.ts (1)
convertCollabsResponse(52-62)
⏰ 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: Summary
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (8)
src/elements/content-sharing/ContentSharingV2.tsx (8)
5-14: LGTM, fool!The new imports are all necessary for the data fetching and transformation flow, sucka! Clean organization separating API utilities from conversion utilities!
42-48: LGTM, fool!Good separation of concerns, sucka! You got raw API data (
collaboratorsData), avatar mapping (avatarURLMap), and final enriched collaborators (collaborators). Clean state management!
68-70: LGTM, fool!You're resetting all the new collaboration state when the API changes, sucka! Prevents stale data from hanging around. Clean!
77-80: Acknowledged deferred error handling, fool!I see you're gonna handle errors in a future PR, sucka - that's cool! The fetchItem call logic looks good for now.
106-111: Good error handling, fool!I like that you wrapped
fetchCollaboratorsin a try-catch, sucka! Setting an empty array as fallback is solid defensive programming!
117-122: Guard logic is correct here, fool!This guard properly waits for
collaboratorsData.entriesto exist before fetching avatars, sucka! The error handling is acknowledged as deferred to future PR.
125-131: LGTM, fool!Clean enrichment logic, sucka! You're waiting for BOTH
collaboratorsDataANDavatarURLMapbefore combining them into final enriched collaborators. Smart!
142-142: LGTM, fool!You're passing the enriched
collaboratorsto UnifiedShareModal, sucka! Clean completion of the data pipeline!
jfox-box
left a comment
jfox-box
left a comment
There was a problem hiding this comment.
LGTM, just 1 change request
src/elements/content-sharing/stories/tests/ContentSharingV2-visual.stories.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/elements/content-sharing/stories/tests/ContentSharingV2-visual.stories.tsx (1)
53-65: I pity the fool who forgets to wrap async operations in waitFor!Good job usin'
waitForaround your collaborators check, fool! ThatsharedWithAvatarsbutton might take a hot minute to render after them avatars get fetched, so wrappin' it inwaitForprevents your test from failin' when the UI ain't ready yet.Your test flow is clean:
- Open the modal (reusing withModernization.play - smart!)
- Wait for and click the "Shared with" button
- Verify all them collaborator UI elements show up
One thing though, sucka - you got
waitForwrappin' the entire block including the click and the subsequent assertions. Consider whether you need to wait for ALL of them assertions or just the initial button. If the modal opens instantly after the click, you could move some assertions outsidewaitForfor faster test execution.You could restructure this to only wait for what needs waitin':
play: async context => { await withModernization.play(context); - await waitFor(async () => { - const sharedWithAvatars = screen.getByRole('button', { name: 'Shared with D R D' }); - expect(sharedWithAvatars).toBeVisible(); - await userEvent.click(sharedWithAvatars); - - expect(screen.getByRole('link', { name: 'Manage All' })).toBeVisible(); - expect(screen.getByRole('grid', { name: 'Collaborators' })).toBeVisible(); - expect(screen.getByRole('row', { name: /Detective Parrot/ })).toBeVisible(); - expect(screen.getByRole('button', { name: 'Done' })).toBeVisible(); - }); + const sharedWithAvatars = await screen.findByRole('button', { name: 'Shared with D R D' }); + expect(sharedWithAvatars).toBeVisible(); + await userEvent.click(sharedWithAvatars); + + expect(screen.getByRole('link', { name: 'Manage All' })).toBeVisible(); + expect(screen.getByRole('grid', { name: 'Collaborators' })).toBeVisible(); + expect(screen.getByRole('row', { name: /Detective Parrot/ })).toBeVisible(); + expect(screen.getByRole('button', { name: 'Done' })).toBeVisible(); },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/elements/content-sharing/stories/tests/ContentSharingV2-visual.stories.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/elements/content-sharing/stories/tests/ContentSharingV2-visual.stories.tsx (2)
src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js (6)
mockAPIWithoutSharedLink(185-189)mockAPIWithoutSharedLink(185-189)mockAPIWithSharedLink(180-184)mockAPIWithSharedLink(180-184)mockAPIWithCollaborators(190-194)mockAPIWithCollaborators(190-194)src/elements/content-sharing/stories/ContentSharingV2.stories.tsx (1)
withSharedLink(9-13)
⏰ 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 (2)
src/elements/content-sharing/stories/tests/ContentSharingV2-visual.stories.tsx (2)
17-26: This play function lookin' good, fool!I like how you're testin' this interaction flow step by step:
- Find and click that button to open the modal
- Verify the heading, combobox, and switch all show up like they supposed to
The async/await pattern is solid, and you're using
findByRolefor the button (which waits automatically) and checking visibility properly. That's how a champion does it!
33-46: Smart move reusing that play function, fool!You're callin'
withModernization.play(context)first to handle the modal opening, then verifying all them shared link elements. That's good code reuse, sucka!The test flow is solid:
- Verify the link URL and settings button
- Click the access dropdown
- Check that "Invited people only" is disabled
The
toHaveAttribute('aria-disabled', 'true')assertion is the right way to verify that disabled state in accessible components.
src/elements/content-sharing/stories/tests/ContentSharingV2-visual.stories.tsx
Show resolved
Hide resolved
What
Testing
Notes: ContentSharingV2 should look like the current ContentSharing when the provided permissions are the same
Summary by CodeRabbit
New Features
Improvements
Tests / Visuals