feat(content-sharing): Convert Content Sharing V2 API Response #4285
Conversation
WalkthroughI pity the fool who don't pass the API: ContentSharingV2 now requires an Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App
participant ContentSharing
participant V2 as ContentSharingV2
participant API
participant USM as UnifiedShareModal
App->>ContentSharing: render({ api, itemID, itemType, ... })
ContentSharing->>V2: render when api truthy
V2->>API: getFileAPI/getFolderAPI.get({ id, fields })
API-->>V2: item response
V2->>V2: convertItemResponse(item)
V2->>API: getUsersAPI.getUser({ fields })
API-->>V2: current user
V2->>USM: render({ item, sharedLink, currentUser, collaborationRoles, config })
Note over V2,USM: USM rendered only after item loaded
sequenceDiagram
autonumber
participant ItemAPI as Item API Response
participant Converter as convertItemResponse
participant Utils as getAllowed*/constants
participant Output as ItemData
ItemAPI->>Converter: raw fields (shared_link, classification, roles, access)
Converter->>Utils: lookup color map & permission/access helpers
Converter-->>Output: normalized { item, sharedLink?, collaborationRoles }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
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: 3
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/ContentSharing.js (1)
117-128: Guard against null API before rendering V2, fool!You’re handin' ContentSharingV2 a maybe-null
api, and that component callsapi.getFileAPI()right away. On a re-render whereapitemporarily collapses to null (for example during prop churn whencreateAPIhasn’t rebuilt yet), you’ll explode with “Cannot read properties of null.” The legacy branch guarded with{api && <SharingModal …/>}—we need the same shield here.Lock it down like this so we only render V2 once the API is real:
if (isFeatureEnabled(features, 'contentSharingV2')) { - return ( - <ContentSharingV2 - api={api} + return ( + api && ( + <ContentSharingV2 + api={api} itemID={itemID} itemType={itemType} hasProviders={hasProviders} language={language} messages={messages} > {children} - </ContentSharingV2> + </ContentSharingV2> + ) ); }I pity the fool who lets a null API slip through!
🧹 Nitpick comments (7)
src/elements/content-sharing/utils/getAllowedAccessLevels.ts (1)
6-12: Streamline this copy loop, fool.You can drop the manual push loop and just spread the array for the same outcome with less noise.
-export const getAllowedAccessLevels = (levels?: Array<string>): Array<string> | null => { - if (!levels) return [ACCESS_OPEN, ACCESS_COMPANY, ACCESS_COLLAB]; - - const allowedAccessLevels = []; - levels.forEach(level => { - allowedAccessLevels.push(level); - }); - - return allowedAccessLevels; +export const getAllowedAccessLevels = (levels?: Array<string>): Array<string> | null => { + if (!levels) { + return [ACCESS_OPEN, ACCESS_COMPANY, ACCESS_COLLAB]; + } + + return [...levels]; };src/elements/content-sharing/utils/__tests__/getAllowedAccessLevels.test.ts (1)
15-23: Test copies or original array, fool?These assertions pass because we’re comparing arrays by value, but
getAllowedAccessLevelscurrently pushes every level into a new array, so it’s really returning a shallow copy. If the contract is “return exactly what you gave me,” let’s prove it by freezing the input or checking referential equality. Otherwise we risk missing accidental mutations.Example tweak:
- const result = getAllowedAccessLevels(levels); - expect(result).toEqual(levels); + const frozen = Object.freeze(levels); + const result = getAllowedAccessLevels(frozen); + expect(result).toBe(frozen);Either tighten the test or clarify the function’s intent so nobody misreads the behavior.
src/elements/content-sharing/utils/__tests__/getAllowedPermissionLevels.test.ts (1)
23-27: Unknown permission scenario needs clarity, fool!Great that you covered the oddball
unknown_permission, but we’re really checking behavior when backend hands us nonsense. Consider naming the test “should return empty array when current permission is unknown and changes are locked” so future readers know why we expect []. Keep those expectations obvious, sucka.src/elements/content-sharing/utils/getAllowedPermissionLevels.ts (1)
3-21: Lock down your parameter types, fool!These params are coming in as
anyright now. This is a.tsfile, so givecanChangeAccessLevel,isDownloadSettingAvailable, andpermissionreal types—booleanandSharedLinkPermission(or at leaststring). That way TypeScript slaps anyone who passes garbage.Something like:
-export const getAllowedPermissionLevels = ( - canChangeAccessLevel, - isDownloadSettingAvailable, - permission, -): Array<string> => { +export const getAllowedPermissionLevels = ( + canChangeAccessLevel: boolean, + isDownloadSettingAvailable: boolean, + permission: string, +): Array<string> => {I pity the fool who leaves implicit
anys lurking in our utils!src/elements/content-sharing/stories/tests/ContentSharingV2-visual.stories.tsx (1)
13-23: Story args need truth, fool!Love the new withSharedLink story, but the default args still point at
mockAPIWithoutSharedLink. Maybe make that explicit in the default export so readers know what base data they’re seeing, or rename the story to shout it loud. Storybook clarity keeps the team from guessing.src/elements/content-sharing/__tests__/ContentSharingV2.test.tsx (1)
21-41: Mock factory drops error callbacks, fool!
createSuccessMockignores the error callback slot, so if someone adds a failure-path assertion in these tests, they’ll hit undefined. Pass through the rest params and let callers trigger errors when needed:-const createSuccessMock = responseFromAPI => (id, successFn) => { +const createSuccessMock = responseFromAPI => (id, successFn, errorFn, ...rest) => { return Promise.resolve(responseFromAPI).then(response => { successFn(response); }); };Maybe even surface a
createErrorMocktwin so we can exercise unhappy paths. Keep those mocks battle-ready, sucka!src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js (1)
21-27: Keep the mock aligned with the real shared link payload, fool.I pity the fool who hands USM an epoch number when the live API dishes out ISO‑8601 strings. Downstream code often slices or formats that string directly, so this mismatch can make stories/tests lie about expiration handling—detective work nobody wants. Let's stick with the wire format.
- unshared_at: 1704067200000, + unshared_at: '2024-01-01T00:00:00.000Z',
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (16)
package.json(1 hunks)src/elements/content-sharing/ContentSharing.js(1 hunks)src/elements/content-sharing/ContentSharingV2.tsx(1 hunks)src/elements/content-sharing/__tests__/ContentSharingV2.test.tsx(1 hunks)src/elements/content-sharing/stories/ContentSharingV2.stories.tsx(1 hunks)src/elements/content-sharing/stories/tests/ContentSharingV2-visual.stories.tsx(1 hunks)src/elements/content-sharing/types.js(1 hunks)src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js(1 hunks)src/elements/content-sharing/utils/__tests__/convertItemResponse.test.ts(1 hunks)src/elements/content-sharing/utils/__tests__/getAllowedAccessLevels.test.ts(1 hunks)src/elements/content-sharing/utils/__tests__/getAllowedPermissionLevels.test.ts(1 hunks)src/elements/content-sharing/utils/constants.ts(1 hunks)src/elements/content-sharing/utils/convertItemResponse.ts(1 hunks)src/elements/content-sharing/utils/getAllowedAccessLevels.ts(1 hunks)src/elements/content-sharing/utils/getAllowedPermissionLevels.ts(1 hunks)src/elements/content-sharing/utils/index.ts(1 hunks)
👮 Files not reviewed due to content moderation or server errors (1)
- src/elements/content-sharing/utils/index.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-12T18:04:17.698Z
Learnt from: tjuanitas
PR: box/box-ui-elements#4224
File: package.json:296-297
Timestamp: 2025-08-12T18:04:17.698Z
Learning: In the box-ui-elements project, the team is comfortable with raising peerDependency minimum versions when upgrading blueprint-web packages, even if it's a breaking change for consumers.
Applied to files:
package.json
🧬 Code graph analysis (11)
src/elements/content-sharing/utils/getAllowedPermissionLevels.ts (2)
src/elements/content-sharing/utils/index.ts (1)
getAllowedPermissionLevels(3-3)src/features/unified-share-modal/utils/convertData.js (1)
isDownloadSettingAvailable(173-179)
src/elements/content-sharing/utils/__tests__/getAllowedAccessLevels.test.ts (2)
src/elements/content-sharing/utils/getAllowedAccessLevels.ts (1)
getAllowedAccessLevels(3-12)src/elements/content-sharing/utils/index.ts (1)
getAllowedAccessLevels(2-2)
src/elements/content-sharing/utils/__tests__/getAllowedPermissionLevels.test.ts (1)
src/elements/content-sharing/utils/getAllowedPermissionLevels.ts (1)
getAllowedPermissionLevels(3-21)
src/elements/content-sharing/utils/constants.ts (1)
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/convertItemResponse.ts (3)
src/elements/content-sharing/utils/constants.ts (1)
API_TO_USM_CLASSIFICATION_COLORS_MAP(22-31)src/elements/content-sharing/utils/getAllowedAccessLevels.ts (1)
getAllowedAccessLevels(3-12)src/elements/content-sharing/utils/getAllowedPermissionLevels.ts (1)
getAllowedPermissionLevels(3-21)
src/elements/content-sharing/stories/ContentSharingV2.stories.tsx (2)
src/elements/content-sharing/stories/tests/ContentSharingV2-visual.stories.tsx (1)
withSharedLink(13-17)src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js (4)
mockAPIWithSharedLink(96-96)mockAPIWithSharedLink(96-96)mockAPIWithoutSharedLink(97-97)mockAPIWithoutSharedLink(97-97)
src/elements/content-sharing/__tests__/ContentSharingV2.test.tsx (1)
src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js (10)
DEFAULT_USER_API_RESPONSE(30-35)DEFAULT_USER_API_RESPONSE(30-35)DEFAULT_ITEM_API_RESPONSE(37-47)DEFAULT_ITEM_API_RESPONSE(37-47)MOCK_ITEM_API_RESPONSE_WITH_SHARED_LINK(49-52)MOCK_ITEM_API_RESPONSE_WITH_SHARED_LINK(49-52)MOCK_ITEM_API_RESPONSE_WITH_CLASSIFICATION(54-57)MOCK_ITEM_API_RESPONSE_WITH_CLASSIFICATION(54-57)MOCK_ITEM(14-18)MOCK_ITEM(14-18)
src/elements/content-sharing/types.js (3)
src/elements/content-sharing/SharingModal.js (4)
item(74-74)item(227-227)sharedLink(75-75)sharedLink(228-234)src/features/unified-share-modal/stories/UnifiedShareModal.stories.js (1)
sharedLink(172-172)src/features/unified-share-modal/utils/convertData.js (1)
sharedLink(198-198)
src/elements/content-sharing/utils/__tests__/convertItemResponse.test.ts (2)
src/elements/content-sharing/utils/convertItemResponse.ts (1)
convertItemResponse(7-99)src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js (6)
DEFAULT_ITEM_API_RESPONSE(37-47)DEFAULT_ITEM_API_RESPONSE(37-47)MOCK_ITEM_API_RESPONSE_WITH_CLASSIFICATION(54-57)MOCK_ITEM_API_RESPONSE_WITH_CLASSIFICATION(54-57)MOCK_ITEM_API_RESPONSE_WITH_SHARED_LINK(49-52)MOCK_ITEM_API_RESPONSE_WITH_SHARED_LINK(49-52)
src/elements/content-sharing/ContentSharingV2.tsx (3)
src/elements/content-sharing/SharingModal.js (5)
handleGetItemSuccess(99-104)itemFromAPI(100-100)getItem(151-161)getUserSuccess(170-177)getUserData(179-185)src/elements/content-sharing/utils/convertItemResponse.ts (1)
convertItemResponse(7-99)src/elements/content-sharing/ContentSharing.js (1)
api(89-89)
src/elements/content-sharing/stories/tests/ContentSharingV2-visual.stories.tsx (2)
src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js (4)
mockAPIWithoutSharedLink(97-97)mockAPIWithoutSharedLink(97-97)mockAPIWithSharedLink(96-96)mockAPIWithSharedLink(96-96)src/elements/content-sharing/stories/ContentSharingV2.stories.tsx (1)
withSharedLink(9-13)
🪛 Biome (2.1.2)
src/elements/content-sharing/types.js
[error] 156-159: interface 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: Analyze (javascript-typescript)
- GitHub Check: Summary
🔇 Additional comments (4)
src/elements/content-sharing/stories/ContentSharingV2.stories.tsx (1)
9-13: Solid variant wiring, sucka!Nice job feeding the storybook args with the right mock APIs so V2 shows both shared-link flavors without extra ceremony.
src/elements/content-sharing/utils/constants.ts (1)
22-31: Color map looks tight, sucka.The mapping lines up with the known classification palette and keeps the USM ids straight.
src/elements/content-sharing/utils/__tests__/convertItemResponse.test.ts (1)
21-117: Comprehensive coverage, fool!These scenarios hit the key branches in the converter—shared links, classification, and permission toggles—so regressions will get caught fast.
src/elements/content-sharing/utils/convertItemResponse.ts (1)
27-99: Clean data shaping, sucka.The conversion logic mirrors the API contract and keeps USM props consistent, especially around classification and shared-link settings.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
package.json(2 hunks)src/elements/content-sharing/ContentSharingV2.tsx(1 hunks)src/elements/content-sharing/__tests__/ContentSharingV2.test.tsx(1 hunks)src/elements/content-sharing/types.js(2 hunks)src/elements/content-sharing/utils/__tests__/convertItemResponse.test.ts(1 hunks)src/elements/content-sharing/utils/convertItemResponse.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/elements/content-sharing/utils/tests/convertItemResponse.test.ts
- package.json
- src/elements/content-sharing/tests/ContentSharingV2.test.tsx
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-06-25T13:09:45.168Z
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#4160
File: src/elements/content-sidebar/SidebarToggle.js:13-19
Timestamp: 2025-06-25T13:09:45.168Z
Learning: Files with `flow` or `flow strict` comments at the top use Flow type syntax, not TypeScript. Flow type definitions like `type Props = { ... }` and type imports like `type { RouterHistory }` are valid Flow syntax and should not be flagged as TypeScript-only features.
Applied to files:
src/elements/content-sharing/types.js
📚 Learning: 2025-06-25T13:10:19.128Z
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#4160
File: src/elements/content-sidebar/SidebarToggle.js:11-11
Timestamp: 2025-06-25T13:10:19.128Z
Learning: In the box-ui-elements codebase, Flow files support `import type` syntax for type-only imports. The `import type` statement is valid Flow syntax, not just TypeScript, and should not be flagged as an error when used in Flow files (indicated by flow comments).
Applied to files:
src/elements/content-sharing/types.js
📚 Learning: 2025-06-17T13:30:02.172Z
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#4144
File: src/elements/content-sidebar/versions/VersionsList.js:24-33
Timestamp: 2025-06-17T13:30:02.172Z
Learning: In the box-ui-elements codebase, Flow components use .flow.js type definition files, not TypeScript .ts files. The InternalSidebarNavigation type is a union type where different variants may have different properties like versionId, and proper type safety is ensured through conditional checks in methods like getSelectedVersionId.
Applied to files:
src/elements/content-sharing/types.js
📚 Learning: 2025-06-25T13:09:54.538Z
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#4160
File: src/elements/content-sidebar/SidebarToggle.js:21-27
Timestamp: 2025-06-25T13:09:54.538Z
Learning: The box-ui-elements project uses Flow for type annotations in JavaScript files, as indicated by flow directives in file headers. Type annotations like `: Props` are valid Flow syntax, not TypeScript syntax.
Applied to files:
src/elements/content-sharing/types.js
🧬 Code graph analysis (2)
src/elements/content-sharing/utils/convertItemResponse.ts (3)
src/elements/content-sharing/utils/constants.ts (1)
API_TO_USM_CLASSIFICATION_COLORS_MAP(22-31)src/elements/content-sharing/utils/getAllowedAccessLevels.ts (1)
getAllowedAccessLevels(3-12)src/elements/content-sharing/utils/getAllowedPermissionLevels.ts (1)
getAllowedPermissionLevels(3-21)
src/elements/content-sharing/ContentSharingV2.tsx (3)
src/elements/content-sharing/SharingModal.js (5)
handleGetItemSuccess(99-104)itemFromAPI(100-100)getItem(151-161)getUserSuccess(170-177)getUserData(179-185)src/elements/content-sharing/utils/convertItemResponse.ts (1)
convertItemResponse(7-96)src/elements/content-sharing/ContentSharing.js (1)
api(89-89)
🪛 Biome (2.1.2)
src/elements/content-sharing/types.js
[error] 2-2: 'import type' 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). (1)
- GitHub Check: Summary
🔇 Additional comments (2)
src/elements/content-sharing/types.js (1)
158-162: Fix this Flow interface, fool!
Flow don't take kindly to TypeScript-styleinterfacedeclarations. Ship this and Flow will blow up parsing faster than I can say “I pity the fool.” Swap it for a Flowtypealias so the build survives.-export interface ItemData { - collaborationRoles: CollaborationRole[]; - item: Item; - sharedLink: SharedLink; -} +export type ItemData = { + collaborationRoles: Array<CollaborationRole>, + item: Item, + sharedLink: SharedLink, +};src/elements/content-sharing/ContentSharingV2.tsx (1)
71-118: Give the SDK real error handlers, sucka!
Passing{}where the SDK expects a function means the first failure will try to call an object and face-plant with “errorCallback is not a function.” Wire up actual callbacks before this ships.const handleGetItemSuccess = React.useCallback(itemData => { const { collaborationRoles: collaborationRolesFromAPI, item: itemFromAPI, sharedLink: sharedLinkFromAPI, } = convertItemResponse(itemData); setItem(itemFromAPI); setSharedLink(sharedLinkFromAPI); setCollaborationRoles(collaborationRolesFromAPI); }, []); + const handleGetItemError = React.useCallback(() => { + setItem(null); + setSharedLink(null); + setCollaborationRoles(null); + }, []); + + const handleGetUserError = React.useCallback(() => { + setCurrentUser(null); + }, []); ... api.getFileAPI().getFile( itemID, handleGetItemSuccess, - {}, + handleGetItemError, { fields: CONTENT_SHARING_ITEM_FIELDS, }, ); ... api.getFolderAPI().getFolderFields( itemID, handleGetItemSuccess, - {}, + handleGetItemError, { fields: CONTENT_SHARING_ITEM_FIELDS, }, ); ... api.getUsersAPI(false).getUser( itemID, getUserSuccess, - {}, + handleGetUserError, { params: { fields: [FIELD_ENTERPRISE, FIELD_HOSTNAME].toString(), }, }, );Don’t forget to drop these new handlers into the dependency arrays so React keeps ‘em fresh.
greg-in-a-box
changed the title
reneshen0328
force-pushed
the
feat-convert-content-sharing-api-response
branch
from
8ec2bcb to
ca99e13
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
src/elements/content-sharing/utils/getAllowedAccessLevels.ts (2)
3-6: Tighten the return type and guard precisely for undefined. I pity the fool who widens types for no reason!You never return null. Narrow the return type and avoid truthiness on arrays to keep semantics identical.
-export const getAllowedAccessLevels = (levels?: Array<string>): Array<string> | null => { - if (!levels) return [ACCESS_OPEN, ACCESS_COMPANY, ACCESS_COLLAB]; - return [...levels]; -}; +export const getAllowedAccessLevels = (levels?: Array<string>): Array<string> => { + if (levels === undefined) return [ACCESS_OPEN, ACCESS_COMPANY, ACCESS_COLLAB]; + return [...levels]; +};
3-6: Optionally validate/dedupe levels. Don’t let bad data play you, fool.If upstream can send unknown/duplicate values, consider filtering to known constants and deduping before returning.
src/elements/content-sharing/__tests__/ContentSharingV2.test.tsx (5)
21-25: Mock ignores error/options; add negative-path coverage. Don’t fear errors, chump—test ’em.createSuccessMock drops error callbacks/options. Consider adding a failure helper and one test that asserts error handling/UI.
61-68: Loosen brittle call assertions. Don’t handcuff yourself to exact arg shapes, fool.Use expect.anything for error cb and objectContaining for options to reduce flakiness if implementation changes.
- expect(getDefaultFileMock).toHaveBeenCalledWith( - MOCK_ITEM.id, - expect.any(Function), - {}, - { - fields: CONTENT_SHARING_ITEM_FIELDS, - }, - ); + expect(getDefaultFileMock).toHaveBeenCalledWith( + MOCK_ITEM.id, + expect.any(Function), + expect.anything(), + expect.objectContaining({ + fields: CONTENT_SHARING_ITEM_FIELDS, + }), + );
79-86: Same robustness for folder API call. Keep it flexible, fool.- expect(getDefaultFolderMock).toHaveBeenCalledWith( - MOCK_ITEM.id, - expect.any(Function), - {}, - { - fields: CONTENT_SHARING_ITEM_FIELDS, - }, - ); + expect(getDefaultFolderMock).toHaveBeenCalledWith( + MOCK_ITEM.id, + expect.any(Function), + expect.anything(), + expect.objectContaining({ + fields: CONTENT_SHARING_ITEM_FIELDS, + }), + );
99-106: Same for shared link case. Consistency wins, sucka.- expect(getFileMockWithSharedLink).toHaveBeenCalledWith( - MOCK_ITEM.id, - expect.any(Function), - {}, - { - fields: CONTENT_SHARING_ITEM_FIELDS, - }, - ); + expect(getFileMockWithSharedLink).toHaveBeenCalledWith( + MOCK_ITEM.id, + expect.any(Function), + expect.anything(), + expect.objectContaining({ + fields: CONTENT_SHARING_ITEM_FIELDS, + }), + );
121-128: And for classification case. Don’t stop now, fool.- expect(getFileMockWithClassification).toHaveBeenCalledWith( - MOCK_ITEM.id, - expect.any(Function), - {}, - { - fields: CONTENT_SHARING_ITEM_FIELDS, - }, - ); + expect(getFileMockWithClassification).toHaveBeenCalledWith( + MOCK_ITEM.id, + expect.any(Function), + expect.anything(), + expect.objectContaining({ + fields: CONTENT_SHARING_ITEM_FIELDS, + }), + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/elements/content-sharing/ContentSharing.js(1 hunks)src/elements/content-sharing/__tests__/ContentSharingV2.test.tsx(1 hunks)src/elements/content-sharing/stories/ContentSharing.stories.js(2 hunks)src/elements/content-sharing/utils/convertItemResponse.ts(1 hunks)src/elements/content-sharing/utils/getAllowedAccessLevels.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/elements/content-sharing/__tests__/ContentSharingV2.test.tsx (1)
src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js (10)
DEFAULT_USER_API_RESPONSE(30-35)DEFAULT_USER_API_RESPONSE(30-35)DEFAULT_ITEM_API_RESPONSE(37-47)DEFAULT_ITEM_API_RESPONSE(37-47)MOCK_ITEM_API_RESPONSE_WITH_SHARED_LINK(49-52)MOCK_ITEM_API_RESPONSE_WITH_SHARED_LINK(49-52)MOCK_ITEM_API_RESPONSE_WITH_CLASSIFICATION(54-57)MOCK_ITEM_API_RESPONSE_WITH_CLASSIFICATION(54-57)MOCK_ITEM(14-18)MOCK_ITEM(14-18)
src/elements/content-sharing/ContentSharing.js (1)
src/elements/content-sharing/messages.js (1)
messages(3-62)
src/elements/content-sharing/utils/convertItemResponse.ts (3)
src/elements/content-sharing/utils/constants.ts (1)
API_TO_USM_CLASSIFICATION_COLORS_MAP(22-31)src/elements/content-sharing/utils/getAllowedAccessLevels.ts (1)
getAllowedAccessLevels(3-6)src/elements/content-sharing/utils/getAllowedPermissionLevels.ts (1)
getAllowedPermissionLevels(3-21)
src/elements/content-sharing/utils/getAllowedAccessLevels.ts (1)
src/elements/content-sharing/utils/index.ts (1)
getAllowedAccessLevels(2-2)
⏰ 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 (2)
src/elements/content-sharing/utils/convertItemResponse.ts (1)
16-27: Guard optional shared_link_features, fool!When the V2 API skips
shared_link_features(like tenants without password support), Line 20 blows up with “Cannot destructure property 'password' of undefined.” I pity the runtime that hits this. Give it a safe fallback so we keep on truckin’.- const { password: isPasswordAvailable } = shared_link_features; + const isPasswordAvailable = shared_link_features?.password ?? false;src/elements/content-sharing/ContentSharing.js (1)
119-130: LGTM on guarded render and passing api to V2. Smooth move, sucka.Passing api and guarding with api && … is solid. Verify V2 honors config/feature toggles parity with v1 to avoid UX drift.
reneshen0328
force-pushed
the
feat-convert-content-sharing-api-response
branch
from
ca99e13 to
0234f79
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
src/elements/content-sharing/utils/constants.ts (1)
22-31: Freeze the map to prevent accidental mutationLock this mapping down so no fool mutates it at runtime.
Apply this diff:
-export const API_TO_USM_CLASSIFICATION_COLORS_MAP = { +export const API_TO_USM_CLASSIFICATION_COLORS_MAP = Object.freeze({ [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, -}; +});src/elements/content-sharing/__tests__/ContentSharingV2.test.tsx (2)
58-74: Use async findBy queries to reduce flakinessAfter async data loads, prefer
findBy*overgetBy*. Don’t make your tests flaky, fool.Apply this diff:
- expect(screen.getByRole('heading', { name: 'Share ‘Box Development Guide.pdf’' })).toBeVisible(); - expect(screen.getByRole('combobox', { name: 'Invite People' })).toBeVisible(); - expect(screen.getByRole('switch', { name: 'Shared link' })).toBeVisible(); + expect(await screen.findByRole('heading', { name: 'Share ‘Box Development Guide.pdf’' })).toBeVisible(); + expect(await screen.findByRole('combobox', { name: 'Invite People' })).toBeVisible(); + expect(await screen.findByRole('switch', { name: 'Shared link' })).toBeVisible();
76-92: Same here for folder caseStick with
findBy*for async UI. Keep it steady, sucka.Apply this diff:
- expect(screen.getByRole('heading', { name: 'Share ‘Box Development Guide.pdf’' })).toBeVisible(); - expect(screen.getByRole('combobox', { name: 'Invite People' })).toBeVisible(); - expect(screen.getByRole('switch', { name: 'Shared link' })).toBeVisible(); + expect(await screen.findByRole('heading', { name: 'Share ‘Box Development Guide.pdf’' })).toBeVisible(); + expect(await screen.findByRole('combobox', { name: 'Invite People' })).toBeVisible(); + expect(await screen.findByRole('switch', { name: 'Shared link' })).toBeVisible();src/elements/content-sharing/utils/getAllowedPermissionLevels.ts (1)
3-7: Type them params, sucka!Explicit types prevent implicit any. Tighten param/return types.
Apply this diff:
-export const getAllowedPermissionLevels = ( - canChangeAccessLevel, - isDownloadSettingAvailable, - permission, -): Array<string> => { +export const getAllowedPermissionLevels = ( + canChangeAccessLevel: boolean, + isDownloadSettingAvailable: boolean, + permission: typeof PERMISSION_CAN_DOWNLOAD | typeof PERMISSION_CAN_PREVIEW, +): Array<typeof PERMISSION_CAN_DOWNLOAD | typeof PERMISSION_CAN_PREVIEW> => {src/elements/content-sharing/ContentSharingV2.tsx (2)
94-95: Keep deps in sync with new handlerAdd handleGetItemError to the effect deps.
Apply this diff:
- }, [api, item, itemID, itemType, sharedLink, handleGetItemSuccess]); + }, [api, item, itemID, itemType, sharedLink, handleGetItemSuccess, handleGetItemError]);
91-94: Lose the isEmpty(api) paranoiaAPI is a class instance; isEmpty check is unnecessary. A simple truthy check is enough.
Apply this diff:
- if (api && !isEmpty(api) && !item && !sharedLink) { + if (api && !item && !sharedLink) { getItem(); }src/elements/content-sharing/utils/convertItemResponse.ts (2)
64-65: Align permission options with actual capabilityUse canChangeDownload (already computed) instead of raw canChangeAccessLevel when deriving permissionLevels, so options match what the user can change.
Apply this diff:
- permissionLevels: getAllowedPermissionLevels(canChangeAccessLevel, isDownloadSettingAvailable, permission), + permissionLevels: getAllowedPermissionLevels( + canChangeDownload, + isDownloadSettingAvailable, + permission, + ),
34-37: Classification color fallbackIf the API returns an unknown color, colorId becomes undefined. Consider a safe default to keep UI consistent.
I can wire a default (e.g., a neutral/gray ID) if you confirm which constant to use.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (17)
package.json(2 hunks)src/elements/content-sharing/ContentSharing.js(1 hunks)src/elements/content-sharing/ContentSharingV2.tsx(1 hunks)src/elements/content-sharing/__tests__/ContentSharingV2.test.tsx(1 hunks)src/elements/content-sharing/stories/ContentSharing.stories.js(2 hunks)src/elements/content-sharing/stories/ContentSharingV2.stories.tsx(1 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(1 hunks)src/elements/content-sharing/utils/__tests__/convertItemResponse.test.ts(1 hunks)src/elements/content-sharing/utils/__tests__/getAllowedAccessLevels.test.ts(1 hunks)src/elements/content-sharing/utils/__tests__/getAllowedPermissionLevels.test.ts(1 hunks)src/elements/content-sharing/utils/constants.ts(1 hunks)src/elements/content-sharing/utils/convertItemResponse.ts(1 hunks)src/elements/content-sharing/utils/getAllowedAccessLevels.ts(1 hunks)src/elements/content-sharing/utils/getAllowedPermissionLevels.ts(1 hunks)src/elements/content-sharing/utils/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- src/elements/content-sharing/utils/index.ts
- package.json
- src/elements/content-sharing/utils/tests/getAllowedAccessLevels.test.ts
- src/elements/content-sharing/stories/ContentSharingV2.stories.tsx
- src/elements/content-sharing/ContentSharing.js
- src/elements/content-sharing/utils/mocks/ContentSharingV2Mocks.js
- src/elements/content-sharing/utils/getAllowedAccessLevels.ts
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-06-25T13:09:45.168Z
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#4160
File: src/elements/content-sidebar/SidebarToggle.js:13-19
Timestamp: 2025-06-25T13:09:45.168Z
Learning: Files with `flow` or `flow strict` comments at the top use Flow type syntax, not TypeScript. Flow type definitions like `type Props = { ... }` and type imports like `type { RouterHistory }` are valid Flow syntax and should not be flagged as TypeScript-only features.
Applied to files:
src/elements/content-sharing/types.js
📚 Learning: 2025-06-25T13:10:19.128Z
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#4160
File: src/elements/content-sidebar/SidebarToggle.js:11-11
Timestamp: 2025-06-25T13:10:19.128Z
Learning: In the box-ui-elements codebase, Flow files support `import type` syntax for type-only imports. The `import type` statement is valid Flow syntax, not just TypeScript, and should not be flagged as an error when used in Flow files (indicated by flow comments).
Applied to files:
src/elements/content-sharing/types.js
📚 Learning: 2025-06-17T13:30:02.172Z
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#4144
File: src/elements/content-sidebar/versions/VersionsList.js:24-33
Timestamp: 2025-06-17T13:30:02.172Z
Learning: In the box-ui-elements codebase, Flow components use .flow.js type definition files, not TypeScript .ts files. The InternalSidebarNavigation type is a union type where different variants may have different properties like versionId, and proper type safety is ensured through conditional checks in methods like getSelectedVersionId.
Applied to files:
src/elements/content-sharing/types.js
📚 Learning: 2025-06-25T13:09:54.538Z
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#4160
File: src/elements/content-sidebar/SidebarToggle.js:21-27
Timestamp: 2025-06-25T13:09:54.538Z
Learning: The box-ui-elements project uses Flow for type annotations in JavaScript files, as indicated by flow directives in file headers. Type annotations like `: Props` are valid Flow syntax, not TypeScript syntax.
Applied to files:
src/elements/content-sharing/types.js
🧬 Code graph analysis (6)
src/elements/content-sharing/utils/constants.ts (1)
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__/convertItemResponse.test.ts (2)
src/elements/content-sharing/utils/convertItemResponse.ts (1)
convertItemResponse(7-98)src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js (6)
DEFAULT_ITEM_API_RESPONSE(37-47)DEFAULT_ITEM_API_RESPONSE(37-47)MOCK_ITEM_API_RESPONSE_WITH_CLASSIFICATION(54-57)MOCK_ITEM_API_RESPONSE_WITH_CLASSIFICATION(54-57)MOCK_ITEM_API_RESPONSE_WITH_SHARED_LINK(49-52)MOCK_ITEM_API_RESPONSE_WITH_SHARED_LINK(49-52)
src/elements/content-sharing/utils/__tests__/getAllowedPermissionLevels.test.ts (1)
src/elements/content-sharing/utils/getAllowedPermissionLevels.ts (1)
getAllowedPermissionLevels(3-21)
src/elements/content-sharing/utils/getAllowedPermissionLevels.ts (1)
src/features/unified-share-modal/utils/convertData.js (1)
isDownloadSettingAvailable(173-179)
src/elements/content-sharing/utils/convertItemResponse.ts (3)
src/elements/content-sharing/utils/constants.ts (1)
API_TO_USM_CLASSIFICATION_COLORS_MAP(22-31)src/elements/content-sharing/utils/getAllowedAccessLevels.ts (1)
getAllowedAccessLevels(3-6)src/elements/content-sharing/utils/getAllowedPermissionLevels.ts (1)
getAllowedPermissionLevels(3-21)
src/elements/content-sharing/stories/tests/ContentSharingV2-visual.stories.tsx (2)
src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js (4)
mockAPIWithoutSharedLink(97-97)mockAPIWithoutSharedLink(97-97)mockAPIWithSharedLink(96-96)mockAPIWithSharedLink(96-96)src/elements/content-sharing/stories/ContentSharingV2.stories.tsx (1)
withSharedLink(9-13)
🪛 CircleCI: build-unit-tests
src/elements/content-sharing/ContentSharingV2.tsx
[error] 5-5: TypeScript error: Module '@box/unified-share-modal' has no exported members CollaborationRole, Item, SharedLink, or User. Found 4 errors in this file. Command failed with exit code 1.
🪛 Gitleaks (8.28.0)
src/elements/content-sharing/stories/ContentSharing.stories.js
[high] 14-14: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 25-25: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 Biome (2.1.2)
src/elements/content-sharing/types.js
[error] 2-2: 'import type' 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 (13)
src/elements/content-sharing/stories/tests/ContentSharingV2-visual.stories.tsx (4)
3-3: Mock import wired right, foolGood move pulling both mocked APIs from the shared factory—keeps the VRT stories aligned with the real scenarios. I pity the fool who’d duplicate that setup.
8-10: Modernization story hooked up properDropping the mock without a shared link into the modernization variant keeps parity with prod behavior. Nice and tight, sucka.
13-17: Shared-link variant lands cleanNew story cleanly reuses the shared-link mock, giving VRT coverage for that path without extra noise. I approve, fool.
23-23: Default args keep the trigger readyProviding the button as default children mirrors the real entry point so the regression tests stay meaningful. That’s how you do it, sucka.
src/elements/content-sharing/stories/ContentSharing.stories.js (1)
13-26: Rip out these hard-coded secrets, sucka!I pity the fool who checks real-looking item IDs and tokens into source control—Gitleaks already ratted them out. Swap back to the injected globals (or other secure config) so stories inherit safe creds instead of leaking them.
customButton: <Button>✿ Launch ContentSharing ✿</Button>, - itemID: '1990109255716', - token: 'bdcf9zdeDEEILZLw1krLmJRJ3R3HNxpu', + itemID: global.FILE_ID, + token: global.TOKEN, @@ features: { ...global.FEATURE_FLAGS, contentSharingV2: true, }, - token: 'bdcf9zdeDEEILZLw1krLmJRJ3R3HNxpu', - itemID: '1990109255716', + token: global.TOKEN, + itemID: global.FILE_ID,src/elements/content-sharing/utils/__tests__/convertItemResponse.test.ts (1)
57-80: Shared link expectations look solidGood coverage on access, permission levels, and settings surface. Clean mapping, sucka.
src/elements/content-sharing/utils/__tests__/getAllowedPermissionLevels.test.ts (1)
4-28: LGTM, chumpSolid edge-case coverage (no-change, download disabled, unknown permission). Tight and right.
src/elements/content-sharing/types.js (2)
2-2: Flow type import is correct—ignore the noisy linter
import typeis valid Flow. Don’t let bogus TS-only warnings from tools confuse you, fool.If Biome complains, exclude Flow files from TS parsing or add a tool config override for
// @flowfiles.
158-162: Fix Flow breakage: replace interface with Flow typeFlow don’t speak “interface.” Swap to a
typealias or you’ll ship a busted build. I pity the fool who mixes TS syntax in Flow files!Apply this diff:
-export interface ItemData { - collaborationRoles: CollaborationRole[]; - item: Item; - sharedLink: SharedLink; -} +export type ItemData = { + collaborationRoles: Array<CollaborationRole>, + item: Item, + sharedLink: SharedLink, +};src/elements/content-sharing/ContentSharingV2.tsx (3)
47-57: Wire proper error handlers. Don’t pass {}, fool!Passing {} as error callbacks will explode at runtime. Add real handlers and use them.
Apply this diff:
const handleGetItemSuccess = React.useCallback(itemData => { @@ setCollaborationRoles(collaborationRolesFromAPI); }, []); + + const handleGetItemError = React.useCallback(() => { + setItem(null); + setSharedLink(null); + setCollaborationRoles(null); + }, []); + + const handleGetUserError = React.useCallback(() => { + setCurrentUser(null); + }, []);
71-88: Stop crashing the SDK: replace {} with error handlers, fool!Use the handlers above for file/folder fetch errors.
Apply this diff:
- api.getFileAPI().getFile( + api.getFileAPI().getFile( itemID, handleGetItemSuccess, - {}, + handleGetItemError, { fields: CONTENT_SHARING_ITEM_FIELDS, }, ); @@ - api.getFolderAPI().getFolderFields( + api.getFolderAPI().getFolderFields( itemID, handleGetItemSuccess, - {}, + handleGetItemError, { fields: CONTENT_SHARING_ITEM_FIELDS, }, );
5-5: Import from ./types won’t work—those types aren’t exported there
The local types.js file doesn’t define CollaborationRole, Item, SharedLink or User; you’ll need to pull them from the correct Box SDK types or add matching exports to your local types file.Likely an incorrect or invalid review comment.
src/elements/content-sharing/utils/convertItemResponse.ts (1)
20-20: Guard optional shared_link_features, fool!shared_link_features can be missing; direct destructuring throws. Use optional chaining.
Apply this diff:
- const { password: isPasswordAvailable } = shared_link_features; + const isPasswordAvailable = shared_link_features?.password ?? false;
src/elements/content-sharing/utils/__tests__/convertItemResponse.test.ts
Show resolved
Hide resolved
reneshen0328
force-pushed
the
feat-convert-content-sharing-api-response
branch
from
0234f79 to
d9023e0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/elements/content-sharing/__tests__/ContentSharingV2.test.tsx (4)
71-74: Use async queries to dodge flaky tests, fool!UI settles after async data; switch to findByRole to wait properly.
Apply this diff:
- expect(screen.getByRole('heading', { name: 'Share ‘Box Development Guide.pdf’' })).toBeVisible(); - expect(screen.getByRole('combobox', { name: 'Invite People' })).toBeVisible(); - expect(screen.getByRole('switch', { name: 'Shared link' })).toBeVisible(); + expect(await screen.findByRole('heading', { name: 'Share ‘Box Development Guide.pdf’' })).toBeVisible(); + expect(await screen.findByRole('combobox', { name: 'Invite People' })).toBeVisible(); + expect(await screen.findByRole('switch', { name: 'Shared link' })).toBeVisible();
89-92: Wait for the folder UI too, sucka!Same async concern for folder view; use findByRole.
Apply this diff:
- expect(screen.getByRole('heading', { name: 'Share ‘Box Development Guide.pdf’' })).toBeVisible(); - expect(screen.getByRole('combobox', { name: 'Invite People' })).toBeVisible(); - expect(screen.getByRole('switch', { name: 'Shared link' })).toBeVisible(); + expect(await screen.findByRole('heading', { name: 'Share ‘Box Development Guide.pdf’' })).toBeVisible(); + expect(await screen.findByRole('combobox', { name: 'Invite People' })).toBeVisible(); + expect(await screen.findByRole('switch', { name: 'Shared link' })).toBeVisible();
112-114: Await shared‑link controls, or face flaky pain!These render after async state; await them.
Apply this diff:
- expect(screen.getByRole('button', { name: 'Link Settings' })).toBeVisible(); - expect(screen.getByRole('button', { name: 'Copy' })).toBeVisible(); + expect(await screen.findByRole('button', { name: 'Link Settings' })).toBeVisible(); + expect(await screen.findByRole('button', { name: 'Copy' })).toBeVisible();
42-51: Add a type to props for cleaner tests.Give getWrapper strong types from the component’s props.
Apply this diff:
-const getWrapper = (props): RenderResult => +const getWrapper = (props: Partial<React.ComponentProps<typeof ContentSharingV2>>): RenderResult =>src/elements/content-sharing/utils/convertItemResponse.ts (2)
9-11: Default invitee roles to empty to avoid crashes, fool.If allowed_invitee_roles is missing, indexOf/map will explode.
Apply this diff:
- allowed_invitee_roles, + allowed_invitee_roles = [],
33-38: Provide a safe fallback for unknown classification colors.Map can return undefined; pick a sensible default color id to keep UI stable.
For example:
colorId: API_TO_USM_CLASSIFICATION_COLORS_MAP[color] ?? /* default color id here */,Confirm the correct default ID from constants before changing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/elements/content-sharing/ContentSharing.js(1 hunks)src/elements/content-sharing/__tests__/ContentSharingV2.test.tsx(1 hunks)src/elements/content-sharing/utils/convertItemResponse.ts(1 hunks)src/elements/content-sharing/utils/getAllowedAccessLevels.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/elements/content-sharing/utils/getAllowedAccessLevels.ts
- src/elements/content-sharing/ContentSharing.js
🧰 Additional context used
🧬 Code graph analysis (2)
src/elements/content-sharing/utils/convertItemResponse.ts (3)
src/elements/content-sharing/utils/constants.ts (1)
API_TO_USM_CLASSIFICATION_COLORS_MAP(22-31)src/elements/content-sharing/utils/getAllowedAccessLevels.ts (1)
getAllowedAccessLevels(3-6)src/elements/content-sharing/utils/getAllowedPermissionLevels.ts (1)
getAllowedPermissionLevels(3-21)
src/elements/content-sharing/__tests__/ContentSharingV2.test.tsx (1)
src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js (10)
DEFAULT_USER_API_RESPONSE(30-35)DEFAULT_USER_API_RESPONSE(30-35)DEFAULT_ITEM_API_RESPONSE(37-47)DEFAULT_ITEM_API_RESPONSE(37-47)MOCK_ITEM_API_RESPONSE_WITH_SHARED_LINK(49-52)MOCK_ITEM_API_RESPONSE_WITH_SHARED_LINK(49-52)MOCK_ITEM_API_RESPONSE_WITH_CLASSIFICATION(54-57)MOCK_ITEM_API_RESPONSE_WITH_CLASSIFICATION(54-57)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). (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (3)
src/elements/content-sharing/__tests__/ContentSharingV2.test.tsx (1)
94-101: Also assert user fetch, or you might miss breakage.You mock getUser but don’t verify it’s called. Add an expectation after the waitFor.
Suggested snippet (place after the existing waitFor):
await waitFor(() => { expect(defaultAPIMock.getUsersAPI().getUser).toHaveBeenCalledWith(expect.any(Function)); });src/elements/content-sharing/utils/convertItemResponse.ts (2)
16-21: Guard optional shared_link_features. I pity the fool who destructures undefined!V2 may omit shared_link_features; current destructuring will throw.
Apply this diff:
- shared_link_features, + shared_link_features, type, } = itemAPIData; - const { password: isPasswordAvailable } = shared_link_features; + const isPasswordAvailable = shared_link_features?.password ?? false;
76-78: Confirm mapping is correct
I pity the fool who skips this—vanity_url is the full link and both convertItemResponse and its tests use vanityDomain for that value, matching the ItemData contract.
reneshen0328
force-pushed
the
feat-convert-content-sharing-api-response
branch
from
d9023e0 to
2f4b5ba
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (16)
package.json(2 hunks)src/elements/content-sharing/ContentSharing.js(1 hunks)src/elements/content-sharing/ContentSharingV2.tsx(1 hunks)src/elements/content-sharing/__tests__/ContentSharingV2.test.tsx(1 hunks)src/elements/content-sharing/stories/ContentSharingV2.stories.tsx(1 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(1 hunks)src/elements/content-sharing/utils/__tests__/convertItemResponse.test.ts(1 hunks)src/elements/content-sharing/utils/__tests__/getAllowedAccessLevels.test.ts(1 hunks)src/elements/content-sharing/utils/__tests__/getAllowedPermissionLevels.test.ts(1 hunks)src/elements/content-sharing/utils/constants.ts(1 hunks)src/elements/content-sharing/utils/convertItemResponse.ts(1 hunks)src/elements/content-sharing/utils/getAllowedAccessLevels.ts(1 hunks)src/elements/content-sharing/utils/getAllowedPermissionLevels.ts(1 hunks)src/elements/content-sharing/utils/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- src/elements/content-sharing/utils/getAllowedPermissionLevels.ts
- src/elements/content-sharing/utils/getAllowedAccessLevels.ts
- src/elements/content-sharing/utils/tests/getAllowedAccessLevels.test.ts
- src/elements/content-sharing/tests/ContentSharingV2.test.tsx
- src/elements/content-sharing/stories/ContentSharingV2.stories.tsx
- src/elements/content-sharing/ContentSharing.js
- src/elements/content-sharing/utils/tests/getAllowedPermissionLevels.test.ts
- src/elements/content-sharing/utils/tests/convertItemResponse.test.ts
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-09-25T22:28:08.858Z
Learnt from: jfox-box
PR: box/box-ui-elements#4285
File: src/elements/content-sharing/ContentSharingV2.tsx:108-118
Timestamp: 2025-09-25T22:28:08.858Z
Learning: In ContentSharingV2, the getUsersAPI().getUser() call should use itemID as the first parameter, not 'me', when fetching user data for content sharing functionality.
Applied to files:
src/elements/content-sharing/ContentSharingV2.tsx
📚 Learning: 2025-08-12T18:04:17.698Z
Learnt from: tjuanitas
PR: box/box-ui-elements#4224
File: package.json:296-297
Timestamp: 2025-08-12T18:04:17.698Z
Learning: In the box-ui-elements project, the team is comfortable with raising peerDependency minimum versions when upgrading blueprint-web packages, even if it's a breaking change for consumers.
Applied to files:
package.json
📚 Learning: 2025-06-25T13:09:45.168Z
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#4160
File: src/elements/content-sidebar/SidebarToggle.js:13-19
Timestamp: 2025-06-25T13:09:45.168Z
Learning: Files with `flow` or `flow strict` comments at the top use Flow type syntax, not TypeScript. Flow type definitions like `type Props = { ... }` and type imports like `type { RouterHistory }` are valid Flow syntax and should not be flagged as TypeScript-only features.
Applied to files:
src/elements/content-sharing/types.js
📚 Learning: 2025-06-25T13:10:19.128Z
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#4160
File: src/elements/content-sidebar/SidebarToggle.js:11-11
Timestamp: 2025-06-25T13:10:19.128Z
Learning: In the box-ui-elements codebase, Flow files support `import type` syntax for type-only imports. The `import type` statement is valid Flow syntax, not just TypeScript, and should not be flagged as an error when used in Flow files (indicated by flow comments).
Applied to files:
src/elements/content-sharing/types.js
📚 Learning: 2025-06-17T13:30:02.172Z
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#4144
File: src/elements/content-sidebar/versions/VersionsList.js:24-33
Timestamp: 2025-06-17T13:30:02.172Z
Learning: In the box-ui-elements codebase, Flow components use .flow.js type definition files, not TypeScript .ts files. The InternalSidebarNavigation type is a union type where different variants may have different properties like versionId, and proper type safety is ensured through conditional checks in methods like getSelectedVersionId.
Applied to files:
src/elements/content-sharing/types.js
📚 Learning: 2025-06-25T13:09:54.538Z
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#4160
File: src/elements/content-sidebar/SidebarToggle.js:21-27
Timestamp: 2025-06-25T13:09:54.538Z
Learning: The box-ui-elements project uses Flow for type annotations in JavaScript files, as indicated by flow directives in file headers. Type annotations like `: Props` are valid Flow syntax, not TypeScript syntax.
Applied to files:
src/elements/content-sharing/types.js
🧬 Code graph analysis (5)
src/elements/content-sharing/utils/constants.ts (1)
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/ContentSharingV2.tsx (2)
src/elements/content-sharing/SharingModal.js (5)
handleGetItemSuccess(99-104)itemFromAPI(100-100)getItem(151-161)getUserSuccess(170-177)getUserData(179-185)src/elements/content-sharing/utils/convertItemResponse.ts (1)
convertItemResponse(8-99)
src/elements/content-sharing/utils/convertItemResponse.ts (4)
src/features/unified-share-modal/utils/convertData.js (5)
isDownloadSettingAvailable(173-179)classificationData(182-182)classificationName(184-184)isEditAllowed(194-194)isDownloadAllowed(214-214)src/elements/content-sharing/utils/constants.ts (1)
API_TO_USM_CLASSIFICATION_COLORS_MAP(22-31)src/elements/content-sharing/utils/getAllowedAccessLevels.ts (1)
getAllowedAccessLevels(3-6)src/elements/content-sharing/utils/getAllowedPermissionLevels.ts (1)
getAllowedPermissionLevels(3-21)
src/elements/content-sharing/stories/tests/ContentSharingV2-visual.stories.tsx (2)
src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js (4)
mockAPIWithoutSharedLink(97-97)mockAPIWithoutSharedLink(97-97)mockAPIWithSharedLink(96-96)mockAPIWithSharedLink(96-96)src/elements/content-sharing/stories/ContentSharingV2.stories.tsx (1)
withSharedLink(9-13)
src/elements/content-sharing/types.js (1)
src/features/shared-link-modal/SharedLink.js (1)
SharedLink(45-133)
🪛 Biome (2.1.2)
src/elements/content-sharing/types.js
[error] 2-2: 'import type' 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 (5)
src/elements/content-sharing/types.js (1)
158-162: Stop using TypeScript interfaces in Flow, fool!This Flow file chokes on
export interface; you gotta swap it for a Flowtypealias or the build falls flat on its face.-export interface ItemData { - collaborationRoles: CollaborationRole[]; - item: Item; - sharedLink: SharedLink; -} +export type ItemData = { + collaborationRoles: Array<CollaborationRole>, + item: Item, + sharedLink: SharedLink, +};src/elements/content-sharing/ContentSharingV2.tsx (3)
47-94: Wire up real error callbacks, sucker!Passing
{}where the SDK expects an error handler blows up at runtime the moment the request fails. Give the item fetch a legit error callback and pass it through instead of that empty object.const handleGetItemSuccess = React.useCallback(itemData => { const { collaborationRoles: collaborationRolesFromAPI, item: itemFromAPI, sharedLink: sharedLinkFromAPI, } = convertItemResponse(itemData); setItem(itemFromAPI); setSharedLink(sharedLinkFromAPI); setCollaborationRoles(collaborationRolesFromAPI); }, []); + const handleGetItemError = React.useCallback(() => { + setItem(null); + setSharedLink(null); + setCollaborationRoles(null); + }, []); + // Get initial data for the item React.useEffect(() => { const getItem = () => { if (itemType === TYPE_FILE) { api.getFileAPI().getFile( itemID, handleGetItemSuccess, - {}, + handleGetItemError, { fields: CONTENT_SHARING_ITEM_FIELDS, }, ); } else if (itemType === TYPE_FOLDER) { api.getFolderAPI().getFolderFields( itemID, handleGetItemSuccess, - {}, + handleGetItemError, { fields: CONTENT_SHARING_ITEM_FIELDS, }, ); } }; if (api && !isEmpty(api) && !item && !sharedLink) { getItem(); } - }, [api, item, itemID, itemType, sharedLink, handleGetItemSuccess]); + }, [api, item, itemID, itemType, sharedLink, handleGetItemSuccess, handleGetItemError]);
96-124: Quit skipping the error handler on user fetch, fool!Same mistake on the user request—passing
{}torpedoes the call when it fails. Hook up a real error callback and feed it togetUser.React.useEffect(() => { const getUserSuccess = userData => { const { enterprise, id } = userData; setCurrentUser({ id, enterprise: { name: enterprise ? enterprise.name : '', }, }); }; + const handleGetUserError = () => { + setCurrentUser(null); + }; + const getUserData = () => { api.getUsersAPI(false).getUser( itemID, getUserSuccess, - {}, + handleGetUserError, { params: { fields: [FIELD_ENTERPRISE, FIELD_HOSTNAME].toString(), }, }, ); }; - if (api && !isEmpty(api) && item && sharedLink && !currentUser) { + if (api && !isEmpty(api) && item && sharedLink && !currentUser) { getUserData(); } - }, [api, currentUser, item, itemID, itemType, sharedLink]); + }, [api, currentUser, item, itemID, itemType, sharedLink]);
121-124: Don’t block the user fetch when there’s no shared link, sucka!When an item lacks a shared link, this guard never loads the current user, breaking the “no shared link” path. Drop the
sharedLinkcheck so we always fetch the user once the item is ready.- if (api && !isEmpty(api) && item && sharedLink && !currentUser) { + if (api && !isEmpty(api) && item && !currentUser) { getUserData(); } - }, [api, currentUser, item, itemID, itemType, sharedLink]); + }, [api, currentUser, item, itemID, itemType, handleGetUserError]);And remember to add
handleGetUserErrorto the dependency array once you wire it up.src/elements/content-sharing/utils/convertItemResponse.ts (1)
21-21: I pity the fool who crashes when shared_link_features is missing!When V2 leaves out
shared_link_features(happens whenever password support is disabled), this destructuring blows up before we can build the payload. Guard it with a safe default so the conversion keeps on trucking, fool.- const { password: isPasswordAvailable } = shared_link_features; + const { password: isPasswordAvailable } = shared_link_features ?? {};
reneshen0328
force-pushed
the
feat-convert-content-sharing-api-response
branch
from
2f4b5ba to
c843e0d
Compare
|
This pull request has been removed from the queue for the following reason: The merge conditions cannot be satisfied due to failing checks: You can check the last failing draft PR here: #4299. You may have to fix your CI before adding the pull request to the queue again. |
|
This pull request has been removed from the queue for the following reason: The merge conditions cannot be satisfied due to failing checks: You can check the last failing draft PR here: #4313. You may have to fix your CI before adding the pull request to the queue again. |
|
https://github.com/Mergifyio requeue |
✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically |
|
This pull request has been removed from the queue for the following reason: The merge conditions cannot be satisfied due to failing checks. You may have to fix your CI before adding the pull request to the queue again. |
|
@Mergifyio requeue |
✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically |
greg-in-a-box
force-pushed
the
feat-convert-content-sharing-api-response
branch
from
864587f to
6510952
Compare
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/stories/tests/ContentSharingV2-visual.stories.tsx (1)
19-27: Add missing api prop to default export in ContentSharingV2-visual.stories.tsx
I pity the fool who forgets to passapi: mockAPIWithoutSharedLinkin the default args for the visual regression tests—add it so stories won't break.
🧹 Nitpick comments (3)
src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js (2)
59-93: Consider error scenarios, fool!The mock API only simulates successful responses. For more comprehensive testing, consider adding optional error callback support or a way to simulate API failures (network errors, permission errors, etc.).
Here's how you could extend it to support error scenarios:
-export const createMockAPI = (itemResponse = DEFAULT_ITEM_API_RESPONSE, userResponse = DEFAULT_USER_API_RESPONSE) => { +export const createMockAPI = ( + itemResponse = DEFAULT_ITEM_API_RESPONSE, + userResponse = DEFAULT_USER_API_RESPONSE, + shouldError = false, + errorResponse = { message: 'Mock API Error' } +) => { const mockFileAPI = { - getFile: (itemID, successCallback) => { + getFile: (itemID, successCallback, errorCallback) => { // Simulate async behavior setTimeout(() => { - successCallback(itemResponse); + if (shouldError && errorCallback) { + errorCallback(errorResponse); + } else { + successCallback(itemResponse); + } }, 100); }, }; // Apply similar pattern to mockFolderAPI and mockUsersAPI
59-93: Clean up these parameter names, fool!The mock API factory simulates async behavior appropriately with
setTimeout, but there are naming inconsistencies I pity:
- Line 80:
getUserreceivesitemIDas a parameter, but it should beuserIDfor clarity.- Lines 62, 71, 80: None of the mock methods actually use the ID parameters - they just return the canned responses. This is acceptable for simple mocks, but the parameter names should match their purpose (e.g.,
fileID,folderID,userID) rather than the genericitemIDeverywhere.Apply this diff to improve parameter naming clarity:
const mockFileAPI = { - getFile: (itemID, successCallback) => { + getFile: (fileID, successCallback) => { // Simulate async behavior setTimeout(() => { successCallback(itemResponse); }, 100); }, }; const mockFolderAPI = { - getFolderFields: (itemID, successCallback) => { + getFolderFields: (folderID, successCallback) => { // Simulate async behavior setTimeout(() => { successCallback(itemResponse); }, 100); }, }; const mockUsersAPI = { - getUser: (itemID, successCallback) => { + getUser: (userID, successCallback) => { // Simulate async behavior setTimeout(() => { successCallback(userResponse); }, 100); }, };src/elements/content-sharing/ContentSharing.js (1)
119-130: Guard condition works, but I pity the fool who returns undefined!The
api &&guard correctly prevents rendering ContentSharingV2 when the API isn't available. However, whenapiis falsy, this expression returnsundefinedrather thannull. While React handles both the same way, returningnullis more explicit and conventional, fool!Consider this more explicit pattern:
- api && ( - <ContentSharingV2 - api={api} - itemID={itemID} - itemType={itemType} - hasProviders={hasProviders} - language={language} - messages={messages} - > - {children} - </ContentSharingV2> - ) + api ? ( + <ContentSharingV2 + api={api} + itemID={itemID} + itemType={itemType} + hasProviders={hasProviders} + language={language} + messages={messages} + > + {children} + </ContentSharingV2> + ) : nullThat said, the current implementation is functionally correct, so this is just a stylistic nitpick, sucka.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (16)
package.json(2 hunks)src/elements/content-sharing/ContentSharing.js(1 hunks)src/elements/content-sharing/ContentSharingV2.tsx(1 hunks)src/elements/content-sharing/__tests__/ContentSharingV2.test.tsx(1 hunks)src/elements/content-sharing/stories/ContentSharingV2.stories.tsx(1 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(1 hunks)src/elements/content-sharing/utils/__tests__/convertItemResponse.test.ts(1 hunks)src/elements/content-sharing/utils/__tests__/getAllowedAccessLevels.test.ts(1 hunks)src/elements/content-sharing/utils/__tests__/getAllowedPermissionLevels.test.ts(1 hunks)src/elements/content-sharing/utils/constants.ts(1 hunks)src/elements/content-sharing/utils/convertItemResponse.ts(1 hunks)src/elements/content-sharing/utils/getAllowedAccessLevels.ts(1 hunks)src/elements/content-sharing/utils/getAllowedPermissionLevels.ts(1 hunks)src/elements/content-sharing/utils/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- package.json
- src/elements/content-sharing/utils/getAllowedAccessLevels.ts
- src/elements/content-sharing/tests/ContentSharingV2.test.tsx
- src/elements/content-sharing/utils/constants.ts
- src/elements/content-sharing/utils/tests/getAllowedAccessLevels.test.ts
- src/elements/content-sharing/utils/getAllowedPermissionLevels.ts
- src/elements/content-sharing/utils/convertItemResponse.ts
- src/elements/content-sharing/stories/ContentSharingV2.stories.tsx
- src/elements/content-sharing/utils/index.ts
- src/elements/content-sharing/utils/tests/getAllowedPermissionLevels.test.ts
- src/elements/content-sharing/utils/tests/convertItemResponse.test.ts
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-09-25T22:28:08.858Z
Learnt from: jfox-box
PR: box/box-ui-elements#4285
File: src/elements/content-sharing/ContentSharingV2.tsx:108-118
Timestamp: 2025-09-25T22:28:08.858Z
Learning: In ContentSharingV2, the getUsersAPI().getUser() call should use itemID as the first parameter, not 'me', when fetching user data for content sharing functionality.
Applied to files:
src/elements/content-sharing/ContentSharingV2.tsx
📚 Learning: 2025-06-25T13:09:45.168Z
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#4160
File: src/elements/content-sidebar/SidebarToggle.js:13-19
Timestamp: 2025-06-25T13:09:45.168Z
Learning: Files with `flow` or `flow strict` comments at the top use Flow type syntax, not TypeScript. Flow type definitions like `type Props = { ... }` and type imports like `type { RouterHistory }` are valid Flow syntax and should not be flagged as TypeScript-only features.
Applied to files:
src/elements/content-sharing/types.js
📚 Learning: 2025-06-25T13:10:19.128Z
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#4160
File: src/elements/content-sidebar/SidebarToggle.js:11-11
Timestamp: 2025-06-25T13:10:19.128Z
Learning: In the box-ui-elements codebase, Flow files support `import type` syntax for type-only imports. The `import type` statement is valid Flow syntax, not just TypeScript, and should not be flagged as an error when used in Flow files (indicated by flow comments).
Applied to files:
src/elements/content-sharing/types.js
📚 Learning: 2025-06-17T13:30:02.172Z
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#4144
File: src/elements/content-sidebar/versions/VersionsList.js:24-33
Timestamp: 2025-06-17T13:30:02.172Z
Learning: In the box-ui-elements codebase, Flow components use .flow.js type definition files, not TypeScript .ts files. The InternalSidebarNavigation type is a union type where different variants may have different properties like versionId, and proper type safety is ensured through conditional checks in methods like getSelectedVersionId.
Applied to files:
src/elements/content-sharing/types.js
📚 Learning: 2025-06-25T13:09:54.538Z
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#4160
File: src/elements/content-sidebar/SidebarToggle.js:21-27
Timestamp: 2025-06-25T13:09:54.538Z
Learning: The box-ui-elements project uses Flow for type annotations in JavaScript files, as indicated by flow directives in file headers. Type annotations like `: Props` are valid Flow syntax, not TypeScript syntax.
Applied to files:
src/elements/content-sharing/types.js
🧬 Code graph analysis (3)
src/elements/content-sharing/ContentSharingV2.tsx (2)
src/elements/content-sharing/SharingModal.js (3)
handleGetItemSuccess(99-104)itemFromAPI(100-100)getItem(151-161)src/elements/content-sharing/utils/convertItemResponse.ts (1)
convertItemResponse(8-99)
src/elements/content-sharing/stories/tests/ContentSharingV2-visual.stories.tsx (2)
src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js (4)
mockAPIWithoutSharedLink(97-97)mockAPIWithoutSharedLink(97-97)mockAPIWithSharedLink(96-96)mockAPIWithSharedLink(96-96)src/elements/content-sharing/stories/ContentSharingV2.stories.tsx (1)
withSharedLink(9-13)
src/elements/content-sharing/types.js (1)
src/features/shared-link-modal/SharedLink.js (1)
SharedLink(45-133)
🪛 Biome (2.1.2)
src/elements/content-sharing/types.js
[error] 2-2: 'import type' 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: Analyze (javascript-typescript)
🔇 Additional comments (36)
src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js (19)
1-6: LGTM, fool!The mock permissions structure is solid—all boolean flags are properly defined for testing scenarios.
8-12: Classification mock looks good!The structure with color, definition, and name is correct for testing classification scenarios.
14-18: LGTM!The mock item structure is appropriate for testing file-based scenarios.
20-28: Shared link mock is solid!The structure correctly represents a shared link with access level, permissions, password flag, expiration, and URLs.
30-35: LGTM!The user API response mock correctly represents a user with enterprise affiliation.
37-47: Default item API response is well-structured!Good reuse of MOCK_ITEM and MOCK_PERMISSIONS constants. The null values for classification and shared_link correctly represent an item without those features.
49-52: LGTM!Clean composition using spread operator to create a variant with a shared link.
54-57: LGTM!Clean composition using spread operator to create a variant with classification.
95-97: LGTM!Pre-configured mock API instances for common test scenarios are well-defined and ready to use.
1-6: LGTM, fool!The mock permissions structure looks solid for testing the happy path with all permissions enabled.
8-12: LGTM, fool!The mock classification data is straightforward and appropriate for testing classification features.
30-35: LGTM, fool!The default user API response structure is clean and sufficient for testing enterprise name display.
37-57: LGTM, fool!I appreciate the composition pattern here - using spread operators to build test scenarios from a base response keeps things DRY and maintainable.
95-97: LGTM, fool!These pre-configured mock APIs provide convenient shortcuts for the most common test scenarios.
20-28: I pity the fool who frets over a mock timestamp
unshared_atis a static fixture for mapping tests; no code compares it to the current date.Likely an incorrect or invalid review comment.
1-18: LGTM, fool! These mock objects are solid.The permission flags, classification data, and item metadata are well-structured for testing purposes. No concerns here, sucka!
20-28: Shared link mock is ready for action!The mock shared link data covers all the key fields including access level, permissions, password flag, expiration timestamp, and URLs. I pity the fool who doesn't use this for testing!
30-57: Default API responses cover all the bases, fool!The default user and item API responses provide comprehensive mock data, including permissions, allowed roles, shared link features, and null defaults. The derived responses for shared link and classification scenarios are well-structured for testing different states.
95-97: Pre-configured mock APIs are ready to roll, sucka!The exported
mockAPIWithSharedLinkandmockAPIWithoutSharedLinkinstances provide clear, convenient test fixtures for different scenarios.src/elements/content-sharing/ContentSharing.js (2)
119-130: LGTM, fool!The guard correctly ensures ContentSharingV2 only renders when the api is available. Props are passed through cleanly, including the new api prop and children.
119-130: LGTM, fool!The conditional rendering guard is solid - you're making sure that api is truthy before passing it to ContentSharingV2, which prevents runtime errors. This pattern matches the existing SharingModal implementation below, keeping things consistent.
src/elements/content-sharing/types.js (4)
158-162: Flow don't understand TypeScript interfaces, fool!This Flow file uses the TypeScript
interfacekeyword, which will break Flow parsing. You need to convert it to a Flowtypealias.Apply this diff:
-export interface ItemData { - collaborationRoles: CollaborationRole[]; - item: Item; - sharedLink: SharedLink; -} +export type ItemData = { + collaborationRoles: Array<CollaborationRole>, + item: Item, + sharedLink: SharedLink, +};
2-3: Don't be fooled by that static analysis hint, fool!The Biome static analysis tool is flagging
import typeas "TypeScript only," but that's a FALSE POSITIVE, sucka! Flow has supportedimport typesyntax since forever - it's valid Flow syntax and you should keep it exactly as is.Based on learnings
158-162: That interface ain't fixed yet, fool!I pity the fool who marked the previous review comment as "Addressed in commit 4f19753" but didn't actually convert this
interfaceto a Flowtypealias! This code is still using TypeScript'sinterfacekeyword, which will break Flow parsing.Apply this diff to fix it for real this time:
-export interface ItemData { - collaborationRoles: CollaborationRole[]; - item: Item; - sharedLink: SharedLink; -} +export type ItemData = { + collaborationRoles: Array<CollaborationRole>, + item: Item, + sharedLink: SharedLink, +};Note the Flow-style changes:
interface→typewith=CollaborationRole[]→Array<CollaborationRole>- Semicolons → commas inside the type body
158-162: Hold up, fool! This interface syntax still ain't Flow!I see the past review claimed this was addressed in commit 4f19753, but the
interfacekeyword is still here, sucka! Flow don't understand TypeScriptinterfacedeclarations - you need to usetypealiases instead, just like the past review suggested.Apply this diff to convert to Flow syntax:
-export interface ItemData { - collaborationRoles: CollaborationRole[]; - item: Item; - sharedLink: SharedLink; -} +export type ItemData = { + collaborationRoles: Array<CollaborationRole>, + item: Item, + sharedLink: SharedLink, +};To verify Flow can parse this file, run:
#!/bin/bash # Check if Flow can parse the types.js file flow check src/elements/content-sharing/types.js 2>&1 | grep -i "interface\|parse\|syntax"src/elements/content-sharing/stories/tests/ContentSharingV2-visual.stories.tsx (2)
1-4: LGTM, fool! These imports are solid.The mock API imports are correctly added to support the visual regression tests with and without shared links.
6-17: These story variants are looking good, fool!Both the
withModernizationandwithSharedLinkstories correctly pass the appropriate mock API instances. This aligns well with the PR objectives to demonstrate ContentSharingV2 behavior with and without shared links.src/elements/content-sharing/ContentSharingV2.tsx (9)
1-12: LGTM! These imports are what you need, fool!All the necessary types, utilities, and constants are properly imported to support the API-driven data flow and response conversion.
16-41: Props looking good, fool!The component signature correctly requires the
apiprop and includes all necessary configuration options. The typing is solid.
42-45: State setup is solid, fool!All the state variables are properly typed and initialized. This provides a clean foundation for the API-driven data flow.
48-57: Callback implementation is clean, fool!The
handleGetItemSuccesscallback correctly transforms the API response usingconvertItemResponseand updates all the necessary state variables in one operation.
60-65: Good thinking resetting state on API change, fool!This effect properly cleans up all state when the API instance changes, preventing stale data issues.
68-94: Item fetching logic looks good, fool!The effect correctly handles both file and folder types and only fetches when necessary. The dependency array is properly set up.
Note: Error callbacks are currently empty objects (
{}), but I see this was already flagged and acknowledged for a future PR.
108-118: User fetching implementation is correct, fool!The
getUsercall correctly usesitemIDas the first parameter to fetch the user associated with the item, not the literal 'me' string.Based on learnings
126-146: Rendering logic is solid, fool!The component correctly waits for item data before rendering the UnifiedShareModal, and passes all the necessary props including config, collaborationRoles, currentUser, item, and sharedLink. The provider structure is maintained properly.
121-124: This sharedLink check gonna break the "without shared link" scenario, fool!Line 121 requires
sharedLinkto be truthy before fetching user data. When there's no shared link,sharedLinkwill benull, sogetUserData()never runs. This breaks the component in scenarios without a shared link.You need to remove the
sharedLinkcheck from the condition:- if (api && !isEmpty(api) && item && sharedLink && !currentUser) { + if (api && !isEmpty(api) && item && !currentUser) { getUserData(); }
What
contentSharingusesconvertDatafunction to convert the returned API data to what the BUIE USM expect. This PR is very similar, only that we are converting the API response to fit the new USM shared feature schema.Testing
Notes: ContentSharingV2 should look like the current ContentSharing when the provided permissions are the same
Without shared link
With shared link
With classification
Summary by CodeRabbit
New Features
Documentation
Tests
Chores