fix(content-sharing): Add error handler #4414
Conversation
WalkthroughThis PR updates dev/peer dependency versions and converts content-sharing APIs and callers to Promise-based flows: sharingService methods now return Promises, useInvites batches via Promise.all, tests updated to await/reject handling, and two loading clears removed from SharingNotification. Changes
Sequence Diagram(s)sequenceDiagram
participant Component
participant useInvites
participant sharingService
participant ItemAPI
participant CallbackHandler
Component->>useInvites: sendCollabRequest(payload)
useInvites->>sharingService: sendCollabRequest(payload)
sharingService->>ItemAPI: perform API call (create/update/delete)
alt Success
ItemAPI-->>sharingService: response
sharingService->>CallbackHandler: onUpdateSharedLink/onRemoveSharedLink(response)
CallbackHandler-->>sharingService: callback handled
sharingService-->>useInvites: Promise.resolve(response)
useInvites-->>Component: resolved results (Promise.all)
else Failure
ItemAPI-->>sharingService: error
sharingService->>CallbackHandler: handleError(error)
CallbackHandler-->>sharingService: error handled
sharingService-->>useInvites: Promise.reject(error)
useInvites-->>Component: rejection (propagated)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (7)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used🧠 Learnings (6)📚 Learning: 2025-08-27T17:03:48.322ZApplied to files:
📚 Learning: 2025-06-25T20:05:18.480ZApplied to files:
📚 Learning: 2025-10-02T16:47:50.715ZApplied to files:
📚 Learning: 2025-08-12T18:04:17.698ZApplied to files:
📚 Learning: 2025-08-21T19:19:44.584ZApplied to files:
📚 Learning: 2025-09-16T14:53:03.538ZApplied to files:
🧬 Code graph analysis (1)src/elements/content-sharing/sharingService.ts (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). (4)
🔇 Additional comments (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
313-313: Update peerDependency version to match devDependency.
@box/unified-share-modalis pinned to^1.36.1indevDependencies(line 143) but^1.7.1inpeerDependencies(line 313). This mismatch means consumers will install v1.7.1, which may lack features or APIs introduced in v1.36.1, causing runtime incompatibilities.Update peerDependencies to
^1.36.1:- "@box/unified-share-modal": "^1.7.1", + "@box/unified-share-modal": "^1.36.1",
🤖 Fix all issues with AI agents
In @src/elements/content-sharing/hooks/useInvites.js:
- Around line 38-54: sendCollabRequest sets loading with setIsLoading(true) but
never resets it; wrap the Promise returned from
api.getCollaborationsAPI(false).addCollaboration in a .finally(() =>
setIsLoading(false)) so the loading state is cleared whether resolve or reject
happens, keeping handleSuccess/handleError and transformResponse intact for the
resolve/reject branches (affects sendCollabRequest and preserves behavior when
used with Promise.all).
🧹 Nitpick comments (1)
src/elements/content-sharing/sharingService.ts (1)
37-52: Remove redundantasynckeyword from Promise constructor functions.All five functions declare
asyncbut then explicitly return anew Promise()constructor. Theasynckeyword is unnecessary when manually constructing and returning a Promise. Choose one approach:
- Option 1 (current pattern): Remove
asynckeyword and keep explicit Promise constructor.- Option 2 (if refactoring to async/await): Remove explicit Promise constructor and use
async/awaitwith promisified API calls.Since the underlying API (
itemApiInstance.share,itemApiInstance.updateSharedLink) uses callbacks rather than Promises, Option 1 is more appropriate here.♻️ Proposed fix: Remove async keywords
- const changeSharedLinkAccess = async (access: string): Promise<void> => { + const changeSharedLinkAccess = (access: string): Promise<void> => { return new Promise<void>((resolve, reject) => { itemApiInstance.share( { id, permissions }, access, data => { onUpdateSharedLink(data); resolve(); }, error => { reject(error); }, CONTENT_SHARING_SHARED_LINK_UPDATE_PARAMS, ); }); }; - const changeSharedLinkPermission = async (permissionLevel: string): Promise<void> => { + const changeSharedLinkPermission = (permissionLevel: string): Promise<void> => { return new Promise<void>((resolve, reject) => { itemApiInstance.updateSharedLink( { id, permissions }, { permissions: convertSharedLinkPermissions(permissionLevel) }, data => { onUpdateSharedLink(data); resolve(); }, error => { reject(error); }, CONTENT_SHARING_SHARED_LINK_UPDATE_PARAMS, ); }); }; - const updateSharedLink = async (sharedLinkSettings: SharedLinkSettings) => { + const updateSharedLink = (sharedLinkSettings: SharedLinkSettings): Promise<any> => { const { access, isDownloadAvailable, serverUrl } = options; return new Promise((resolve, reject) => { itemApiInstance.updateSharedLink( { id, permissions }, convertSharedLinkSettings(sharedLinkSettings, access, isDownloadAvailable, serverUrl), data => { onUpdateSharedLink(data); resolve(data); }, error => { reject(error); }, CONTENT_SHARING_SHARED_LINK_UPDATE_PARAMS, ); }); }; - const createSharedLink = async () => { + const createSharedLink = (): Promise<any> => { return new Promise((resolve, reject) => { itemApiInstance.share( { id, permissions }, undefined, // if "access" is undefined, the backend will set the default access level for the shared link data => { onUpdateSharedLink(data); resolve(data); }, error => { reject(error); }, CONTENT_SHARING_SHARED_LINK_UPDATE_PARAMS, ); }); }; - const deleteSharedLink = async () => { + const deleteSharedLink = (): Promise<any> => { return new Promise((resolve, reject) => { itemApiInstance.share( { id, permissions }, ACCESS_NONE, data => { onRemoveSharedLink(data); resolve(data); }, error => { reject(error); }, CONTENT_SHARING_SHARED_LINK_UPDATE_PARAMS, ); }); };Note: I also added explicit return type annotations (
Promise<any>) toupdateSharedLink,createSharedLink, anddeleteSharedLinkfor consistency with the other methods.Also applies to: 54-69, 71-88, 90-105, 107-122
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
package.jsonsrc/elements/content-sharing/__tests__/SharingModal.test.jssrc/elements/content-sharing/__tests__/sharingService.test.tssrc/elements/content-sharing/__tests__/useInvites.test.jssrc/elements/content-sharing/hooks/useInvites.jssrc/elements/content-sharing/sharingService.ts
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: tjuanitas
Repo: box/box-ui-elements PR: 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.
📚 Learning: 2025-08-12T18:04:17.698Z
Learnt from: tjuanitas
Repo: box/box-ui-elements PR: 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-10-02T16:47:50.715Z
Learnt from: reneshen0328
Repo: box/box-ui-elements PR: 4322
File: src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js:30-36
Timestamp: 2025-10-02T16:47:50.715Z
Learning: In src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js, mockCurrentUserID is intentionally a number (789) while DEFAULT_USER_API_RESPONSE.id is a string ('789'). This represents different stages of data transformation: mockCurrentUserID is the raw API response before conversion, and DEFAULT_USER_API_RESPONSE.id is the post-conversion state after fetchCurrentUser processes it.
Applied to files:
src/elements/content-sharing/__tests__/sharingService.test.ts
🧬 Code graph analysis (3)
src/elements/content-sharing/hooks/useInvites.js (1)
src/elements/content-sharing/ContentSharing.js (1)
api(91-91)
src/elements/content-sharing/__tests__/sharingService.test.ts (1)
src/elements/content-sharing/constants.js (2)
CONTENT_SHARING_SHARED_LINK_UPDATE_PARAMS(61-63)CONTENT_SHARING_SHARED_LINK_UPDATE_PARAMS(61-63)
src/elements/content-sharing/sharingService.ts (2)
src/elements/content-sharing/constants.js (2)
CONTENT_SHARING_SHARED_LINK_UPDATE_PARAMS(61-63)CONTENT_SHARING_SHARED_LINK_UPDATE_PARAMS(61-63)src/elements/content-sharing/utils/convertSharingServiceData.ts (2)
convertSharedLinkPermissions(16-25)convertSharedLinkSettings(35-83)
⏰ 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). (4)
- GitHub Check: lint_test_build
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Summary
🔇 Additional comments (11)
src/elements/content-sharing/__tests__/useInvites.test.js (2)
54-96: LGTM! Test updates correctly handle Promise-based flows.The tests properly:
- Wrap async operations in
await act(async () => ...)- Catch errors in failure paths to prevent unhandled rejections (line 73)
- Assert concrete values (
null) instead of Promise objects (line 93)These changes align with the Promise-based API introduced in
useInvites.js.
111-116: LGTM! Async handling for v2 flow is correct.The test properly awaits the Promise-returning hook invocation within an async
actblock.src/elements/content-sharing/__tests__/SharingModal.test.js (1)
1024-1036: LGTM! Error handling for Promise-based USM functions is correct.The try/catch block properly:
- Awaits the Promise-returning USM function
- Catches and asserts the expected error shape
- Continues with assertions for side effects (setIsVisibleMock, Notification)
This aligns with the Promise-based error propagation introduced in the sharingService.
src/elements/content-sharing/__tests__/sharingService.test.ts (8)
49-69: LGTM! Test correctly verifies Promise-based wrapper behavior.The mock implementation now simulates the callback pattern, and assertions use
expect.any(Function)to verify that success/error callbacks are provided. This correctly tests the Promise wrapper without coupling to specific mock instances.
82-96: LGTM! Consistent test pattern for changeSharedLinkAccess.The test follows the same pattern: mocking callback invocation and verifying callback parameters with
expect.any(Function).
108-122: LGTM! Consistent test pattern for createSharedLink.
133-147: LGTM! Consistent test pattern for deleteSharedLink.
159-194: LGTM! Consistent test pattern for updateSharedLink.
197-246: LGTM! Test verifies updateSharedLink with all optional parameters.
249-268: LGTM! Test verifies shared link settings handling.
271-334: LGTM! Comprehensive error handling tests added.The new test suite verifies that all Promise-based methods properly reject when the underlying API callbacks invoke the error handler. This is essential for confirming error propagation works correctly across all sharingService methods.
package.json
Outdated
| "@box/react-virtualized": "^9.22.3-rc-box.10", | ||
| "@box/types": "^0.2.1", | ||
| "@box/unified-share-modal": "^1.7.1", | ||
| "@box/unified-share-modal": "^1.36.1", |
There was a problem hiding this comment.
do we need to bump blueprint or other shared feature packages?
reneshen0328
force-pushed
the
fix-csh-error-handler
branch
from
f5dfc74 to
cc9bfc1
Compare
| const { id, permissions } = options; | ||
|
|
||
| const changeSharedLinkAccess = async (access: string) => { | ||
| const changeSharedLinkAccess = async (access: string): Promise<void> => { |
There was a problem hiding this comment.
disclaimer - i haven't read deeply enough yet into the logic of the PR, but why does changeSharedLinkAccess and changeSharedLinkPermission not get the same style of edits as the other methods?
broadly speaking it sounds like this PR adds a bunch of error handling.
also, what is the dfiference between changeSharedLinkAccess, changeSharedLinkPermission, and updateSharedLink? these sound like they're all doing the same thing
There was a problem hiding this comment.
- changeSharedLinkAccess is the api for this
-
- changeSharedLinkPermission is the api for this
-
- updateSharedLink is the api for this
-
There was a problem hiding this comment.
why does changeSharedLinkAccess and changeSharedLinkPermission not get the same style of edits as the other methods?
Currently even if I add error handling for these 2 APIs, USM does not render any notification if it's not defined in the default notification flows. Therefore I don't want to add those changes in this PR as it's also not within the request. If in the future we have request on needing the error handler for those flows as well, I'll update both USM and content sharing.
| try { | ||
| await wrapper.find(UnifiedShareModal).invoke(`${usmFn}`)(); | ||
| } catch (error) { | ||
| expect(error).toEqual(new Error({ status: '400' })); |
There was a problem hiding this comment.
I'm surprised to see a class instantiation in this way (used to seeing it just in java) which also made me think, does the constructor for Error take an object of {status: '400'}? When I clicked into it in cursor, it did not seem clear whether this was correct.
However, when I asked cursor, it seemed to suggest that you are possibly not creating an Error with a status property, but rather creating an Error with an object that also says status.
I don't 100% get what's going on here but maybe with your context, the issue might make more sense?
There was a problem hiding this comment.
ah, here is some output
What Happens with new Error({ status: '400' })
When you pass an object to new Error(), JavaScript calls .toString() on it:
const err = new Error({ status: '400' });
console.log(err.message); // "[object Object]"
console.log(err.status); // undefined
The object gets stringified to "[object Object]" and becomes the message. The status property is lost - it's not transferred to the Error object.
There was a problem hiding this comment.
Several notes here:
- This test file is for SharingModal, which is used in ContentSharingV1 not for V2.
- The reason why this test failed here is because
- I updated the useInvites hook below that sendCollabRequest is now returning a Promise instead of just calling the callbacks from the consumer.
- SharingModal -> SharingNotification calls useInvites and provide its own error handler callback. Without this catch block the function will result in unhandled rejection.
- You're correct about new Error. It expects a string not an object. The reason why I added it here is because the mock in this file was written as new Error(object), and I don't really want to touch this file too much since it's related to V1 (which will be removed soon) and is not relevant to my PR.
There was a problem hiding this comment.
Actionable comments posted: 0
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/__tests__/useInvites.test.js (1)
60-72: Assert the Promise rejection instead of suppressing itThe
.catch(() => {})pattern silently suppresses rejections and could mask bugs if the Promise unexpectedly resolves. Useawait expect(result.current(...)).rejects.toBeTruthy()to explicitly assert the rejection while keeping themockHandleErrorassertion to verify the callback was invoked.Proposed change
- await result.current({ users: [{ email: 'fail@example.com', role: 'editor' }] }).catch(() => {}); + await expect( + result.current({ users: [{ email: 'fail@example.com', role: 'editor' }] }), + ).rejects.toBeTruthy(); expect(mockHandleError).toHaveBeenCalled();
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
package.jsonsrc/elements/content-sharing/__tests__/useInvites.test.js
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: tjuanitas
Repo: box/box-ui-elements PR: 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.
📚 Learning: 2025-08-27T17:03:48.322Z
Learnt from: bxie-box
Repo: box/box-ui-elements PR: 4250
File: src/features/classification/applied-by-ai-classification-reason/__tests__/AppliedByAiClassificationReason.test.tsx:44-51
Timestamp: 2025-08-27T17:03:48.322Z
Learning: In test files for bxie-box, prefer using queryByTestId over getByTestId when testing for expected elements that should always exist, as null access serves as validation for regressions or unexpected changes rather than masking issues with defensive assertions.
Applied to files:
src/elements/content-sharing/__tests__/useInvites.test.js
📚 Learning: 2025-06-25T20:05:18.480Z
Learnt from: rafalmaksymiuk
Repo: box/box-ui-elements PR: 4156
File: src/elements/content-sidebar/SidebarNavButton.js:77-102
Timestamp: 2025-06-25T20:05:18.480Z
Learning: Rendering functions (functions that return JSX) are considered anti-patterns in React development because they create new function instances on every render, break React DevTools, interfere with reconciliation, and make testing more difficult.
Applied to files:
src/elements/content-sharing/__tests__/useInvites.test.js
📚 Learning: 2025-10-02T16:47:50.715Z
Learnt from: reneshen0328
Repo: box/box-ui-elements PR: 4322
File: src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js:30-36
Timestamp: 2025-10-02T16:47:50.715Z
Learning: In src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js, mockCurrentUserID is intentionally a number (789) while DEFAULT_USER_API_RESPONSE.id is a string ('789'). This represents different stages of data transformation: mockCurrentUserID is the raw API response before conversion, and DEFAULT_USER_API_RESPONSE.id is the post-conversion state after fetchCurrentUser processes it.
Applied to files:
src/elements/content-sharing/__tests__/useInvites.test.js
📚 Learning: 2025-08-12T18:04:17.698Z
Learnt from: tjuanitas
Repo: box/box-ui-elements PR: 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-08-21T19:19:44.584Z
Learnt from: jpan-box
Repo: box/box-ui-elements PR: 4237
File: src/api/ZipDownload.ts:6-8
Timestamp: 2025-08-21T19:19:44.584Z
Learning: For Box TypeScript SDK integrations, jpan-box prefers deep imports (e.g., from box-typescript-sdk-gen/lib/schemas/...generated.d.ts.js) over defining local types, citing that Box v2 APIs don't typically have breaking changes, making the deep import approach more viable despite potential brittleness.
Applied to files:
package.json
📚 Learning: 2025-09-16T14:53:03.538Z
Learnt from: bfoxx1906
Repo: box/box-ui-elements PR: 4275
File: src/components/form-elements/draft-js-mention-selector/index.js:4-4
Timestamp: 2025-09-16T14:53:03.538Z
Learning: The createMentionSelectorState export in src/components/form-elements/draft-js-mention-selector/index.js was confirmed to be internal-only within the box-ui-elements package, with no external references or documentation, making the breaking rename to createMentionTimestampSelectorState safe.
Applied to files:
package.json
⏰ 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). (4)
- GitHub Check: lint_test_build
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Summary
🔇 Additional comments (5)
src/elements/content-sharing/__tests__/useInvites.test.js (3)
44-58: Ensure React updates are flushed (act/waitFor) when awaitingresult.current(...)
IfuseInvitesupdates state as part of the async flow,await result.current(...)may still produce “not wrapped in act(...)” warnings or flakes depending on@testing-library/reactv16renderHooksemantics. Consider switching assertions toawait waitFor(() => ...)(or wrapping the call inact(async () => ...)) so the test remains stable even if the hook schedules updates after the promise resolves.
74-87: Nice: direct await + explicit null assertion
This reads cleaner than the prioractscaffolding; just keep an eye on any async state updates (if any) and move towaitForif warnings appear.
90-109: ConsiderwaitForfor the “2 calls” assertions if hook batching becomes async
If the V2 path usesPromise.alland/or schedules state updates/callbacks in microtasks,expect(...).toHaveBeenCalledTimes(2)right afterawait result.current(...)can get flaky. Usingawait waitFor(() => expect(...).toHaveBeenCalledTimes(2))is more robust.package.json (2)
127-145: Dependency bumps look consistent; verify cross-package compatibility (notably USM/user-selector)
Given the large jumps (e.g.,@box/unified-share-modaland@box/user-selector), please confirm via changelogs / integration smoke tests that the new versions are compatible with the current usage and bundling setup.
299-314: Peer dependency updates: OK, but double-check the strict pin on@box/blueprint-web-assets
Raising peer minimums is fine here (based on project learnings), but@box/blueprint-web-assetsbeing an exact version inpeerDependenciescan force consumers into a potentially unnecessary hard match—confirm this is required (vs^4.91.3/>=4.91.3).
reneshen0328
force-pushed
the
fix-csh-error-handler
branch
from
d541887 to
35f3d62
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/elements/content-sharing/sharingService.ts (2)
37-45: Inconsistent error handling compared to other methods.Unlike
createSharedLink,updateSharedLink, anddeleteSharedLink, this method doesn't wrap the API call in aPromisewith explicit error handling. It passes an empty object{}as the error callback (line 42), which means errors won't be properly caught or propagated as Promise rejections.Per the PR objectives, API failures should trigger error notifications. This method should follow the same pattern as
createSharedLinkfor consistency.🔧 Proposed fix
- const changeSharedLinkAccess = async (access: string): Promise<void> => { - return itemApiInstance.share( - { id, permissions }, - access, - onUpdateSharedLink, - {}, - CONTENT_SHARING_SHARED_LINK_UPDATE_PARAMS, - ); + const changeSharedLinkAccess = async (access: string): Promise<any> => { + return new Promise((resolve, reject) => { + itemApiInstance.share( + { id, permissions }, + access, + data => { + onUpdateSharedLink(data); + resolve(data); + }, + error => { + reject(error); + }, + CONTENT_SHARING_SHARED_LINK_UPDATE_PARAMS, + ); + }); };
47-55: Inconsistent error handling compared to other methods.This method has the same issue as
changeSharedLinkAccess: it doesn't wrap the API call in aPromisewith explicit error handling and passes an empty object{}as the error callback (line 52).This is especially inconsistent since
updateSharedLink(lines 57-74) usesitemApiInstance.updateSharedLink()with proper Promise-based error handling.🔧 Proposed fix
- const changeSharedLinkPermission = async (permissionLevel: string): Promise<void> => { - return itemApiInstance.updateSharedLink( - { id, permissions }, - { permissions: convertSharedLinkPermissions(permissionLevel) }, - onUpdateSharedLink, - {}, - CONTENT_SHARING_SHARED_LINK_UPDATE_PARAMS, - ); + const changeSharedLinkPermission = async (permissionLevel: string): Promise<any> => { + return new Promise((resolve, reject) => { + itemApiInstance.updateSharedLink( + { id, permissions }, + { permissions: convertSharedLinkPermissions(permissionLevel) }, + data => { + onUpdateSharedLink(data); + resolve(data); + }, + error => { + reject(error); + }, + CONTENT_SHARING_SHARED_LINK_UPDATE_PARAMS, + ); + }); };
🤖 Fix all issues with AI agents
In @src/elements/content-sharing/__tests__/sharingService.test.ts:
- Around line 263-304: Add two error-handling tests for changeSharedLinkAccess
and changeSharedLinkPermission: mock mockItemApiInstance.share to call the error
callback with a new Error when testing changeSharedLinkAccess and mock
mockItemApiInstance.updateSharedLink to call the error callback with a new Error
when testing changeSharedLinkPermission; in each test call
createSharingServiceWrapper() and await
expect(service.changeSharedLinkAccess('open')).rejects.toEqual(mockError) and
await
expect(service.changeSharedLinkPermission(PERMISSION_CAN_DOWNLOAD)).rejects.toEqual(mockError)
respectively to ensure the Promise rejects with the mocked error.
In @src/elements/content-sharing/hooks/useInvites.js:
- Around line 38-54: The sendCollabRequest function sets and clears setIsLoading
per-request causing a race when multiple invites run concurrently; remove
setIsLoading(true/false) from sendCollabRequest and instead manage loading at
the batch level in the creator function (createPostCollaborationFn) by setting
setIsLoading(true) before invoking Promise.all([...users.map(sendCollabRequest),
...groups.map(sendCollabRequest)]) and clearing it in a finally block, or
alternatively implement a reference counter (e.g., inFlightRequestsRef increment
in sendCollabRequest start and decrement on completion and only
setIsLoading(false) when the counter reaches zero) so loading only becomes false
when all concurrent requests complete.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
package.jsonsrc/elements/content-sharing/SharingNotification.jssrc/elements/content-sharing/__tests__/SharingModal.test.jssrc/elements/content-sharing/__tests__/sharingService.test.tssrc/elements/content-sharing/__tests__/useInvites.test.jssrc/elements/content-sharing/hooks/useInvites.jssrc/elements/content-sharing/sharingService.ts
💤 Files with no reviewable changes (1)
- src/elements/content-sharing/SharingNotification.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/elements/content-sharing/tests/SharingModal.test.js
- package.json
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: tjuanitas
Repo: box/box-ui-elements PR: 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.
📚 Learning: 2025-08-27T17:03:48.322Z
Learnt from: bxie-box
Repo: box/box-ui-elements PR: 4250
File: src/features/classification/applied-by-ai-classification-reason/__tests__/AppliedByAiClassificationReason.test.tsx:44-51
Timestamp: 2025-08-27T17:03:48.322Z
Learning: In test files for bxie-box, prefer using queryByTestId over getByTestId when testing for expected elements that should always exist, as null access serves as validation for regressions or unexpected changes rather than masking issues with defensive assertions.
Applied to files:
src/elements/content-sharing/__tests__/useInvites.test.js
📚 Learning: 2025-06-25T20:05:18.480Z
Learnt from: rafalmaksymiuk
Repo: box/box-ui-elements PR: 4156
File: src/elements/content-sidebar/SidebarNavButton.js:77-102
Timestamp: 2025-06-25T20:05:18.480Z
Learning: Rendering functions (functions that return JSX) are considered anti-patterns in React development because they create new function instances on every render, break React DevTools, interfere with reconciliation, and make testing more difficult.
Applied to files:
src/elements/content-sharing/__tests__/useInvites.test.js
📚 Learning: 2025-10-02T16:47:50.715Z
Learnt from: reneshen0328
Repo: box/box-ui-elements PR: 4322
File: src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js:30-36
Timestamp: 2025-10-02T16:47:50.715Z
Learning: In src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js, mockCurrentUserID is intentionally a number (789) while DEFAULT_USER_API_RESPONSE.id is a string ('789'). This represents different stages of data transformation: mockCurrentUserID is the raw API response before conversion, and DEFAULT_USER_API_RESPONSE.id is the post-conversion state after fetchCurrentUser processes it.
Applied to files:
src/elements/content-sharing/__tests__/useInvites.test.jssrc/elements/content-sharing/__tests__/sharingService.test.ts
🧬 Code graph analysis (3)
src/elements/content-sharing/__tests__/sharingService.test.ts (1)
src/elements/content-sharing/constants.js (2)
CONTENT_SHARING_SHARED_LINK_UPDATE_PARAMS(61-63)CONTENT_SHARING_SHARED_LINK_UPDATE_PARAMS(61-63)
src/elements/content-sharing/hooks/useInvites.js (1)
src/elements/content-sharing/ContentSharing.js (1)
api(95-95)
src/elements/content-sharing/sharingService.ts (3)
src/elements/content-sharing/utils/convertSharingServiceData.ts (1)
convertSharedLinkSettings(35-83)src/elements/content-sharing/utils/index.ts (1)
convertSharedLinkSettings(8-8)src/elements/content-sharing/constants.js (2)
CONTENT_SHARING_SHARED_LINK_UPDATE_PARAMS(61-63)CONTENT_SHARING_SHARED_LINK_UPDATE_PARAMS(61-63)
⏰ 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). (4)
- GitHub Check: lint_test_build
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Summary
🔇 Additional comments (6)
src/elements/content-sharing/sharingService.ts (3)
57-74: LGTM! Good Promise-based error handling.This method properly wraps the API call in a Promise with explicit success and error handlers. The pattern of calling the callback (
onUpdateSharedLink) and then resolving ensures proper error propagation while maintaining backward compatibility.
76-91: LGTM! Proper error handling implementation.This method correctly implements Promise-based error handling with clear success and error paths. The inline comment explaining the undefined access parameter is helpful.
93-108: LGTM! Consistent error handling.This method follows the same Promise-based pattern as
createSharedLinkandupdateSharedLink, properly callingonRemoveSharedLinkon success and rejecting on failure.src/elements/content-sharing/__tests__/sharingService.test.ts (1)
100-114: LGTM! Tests properly updated for Promise-based API.The mock implementation correctly simulates the API's callback pattern, and using
expect.any(Function)for the callback parameters is appropriate since the implementation now wraps these in anonymous functions within Promises.src/elements/content-sharing/__tests__/useInvites.test.js (2)
1-1: LGTM! Correct import for current testing-library.The update from
@testing-library/react-hooksto@testing-library/react(which includesrenderHook) is correct for current versions of the testing library.
54-54: LGTM! Tests properly updated for Promise-based API.The removal of
act()wrappers and the use of directawaitstatements correctly reflect the change to a Promise-based API. The error handling with.catch(() => {})(line 70) ensures test stability when testing rejection paths.Also applies to: 70-70, 83-84, 102-105
| const sendCollabRequest = collab => { | ||
| setIsLoading(true); | ||
| return api.getCollaborationsAPI(false).addCollaboration( | ||
| itemData, | ||
| collab, | ||
| response => { | ||
| handleSuccess(response); | ||
| return transformResponse(response); | ||
| }, | ||
| handleError, | ||
| ); | ||
| return new Promise((resolve, reject) => { | ||
| api.getCollaborationsAPI(false).addCollaboration( | ||
| itemData, | ||
| collab, | ||
| response => { | ||
| handleSuccess(response); | ||
| resolve(transformResponse(response)); | ||
| }, | ||
| error => { | ||
| handleError(error); | ||
| reject(error); | ||
| }, | ||
| ); | ||
| }).finally(() => setIsLoading(false)); | ||
| }; |
There was a problem hiding this comment.
Good Promise implementation, but potential loading state race condition.
The Promise-based implementation correctly handles success and error paths. However, there's a potential race condition with the loading state when multiple collaborators are invited simultaneously:
- Each
sendCollabRequestsetssetIsLoading(true)(line 39) - Each request independently calls
setIsLoading(false)in itsfinallyblock (line 53) - When
Promise.all(lines 61-64) runs multiple requests concurrently, the first completed request will set loading to false even though other requests are still pending
This could cause the UI to show loading=false prematurely during batch invitations.
Consider refactoring the loading state management:
Option 1: Move loading state management outside sendCollabRequest
const createPostCollaborationFn: SendInvitesFnType =
() => async (collabRequest: InviteCollaboratorsRequest) => {
if (!transformRequest) return Promise.resolve(null);
setIsLoading(true);
try {
const { users, groups } = transformRequest(collabRequest);
return await Promise.all([
...users.map(user => sendCollabRequest(user)),
...groups.map(group => sendCollabRequest(group)),
]);
} finally {
setIsLoading(false);
}
};Option 2: Use a reference counter
Track the number of in-flight requests and only set loading to false when all complete.
🤖 Prompt for AI Agents
In @src/elements/content-sharing/hooks/useInvites.js around lines 38 - 54, The
sendCollabRequest function sets and clears setIsLoading per-request causing a
race when multiple invites run concurrently; remove setIsLoading(true/false)
from sendCollabRequest and instead manage loading at the batch level in the
creator function (createPostCollaborationFn) by setting setIsLoading(true)
before invoking Promise.all([...users.map(sendCollabRequest),
...groups.map(sendCollabRequest)]) and clearing it in a finally block, or
alternatively implement a reference counter (e.g., inFlightRequestsRef increment
in sendCollabRequest start and decrement on completion and only
setIsLoading(false) when the counter reaches zero) so loading only becomes false
when all concurrent requests complete.
There was a problem hiding this comment.
Ah yeah I'm afraid mr rabbit might be right here. The setIsLoading should be in createPostCollaborationFn instead of sendCollabRequest
There was a problem hiding this comment.
@jfox-box Thanks for confirming! You're absolutely right about the race condition.
Would you like me to help implement the fix? I can move the setIsLoading calls from sendCollabRequest to the createPostCollaborationFn level to properly handle the batch loading state. Just let me know!
| "sanitize-html": "^2.14.0", | ||
| "sass": "1.45.0", | ||
| "sass-loader": "^8.0.2", | ||
| "sass": "1.56.0", |
There was a problem hiding this comment.
why did you choose this version?
There was a problem hiding this comment.
@reneshen0328 when you run storybook, are you seeing any deprecation warnings in the console?
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @package.json:
- Around line 274-275: Update the peerDependencies entry for "sass" in
package.json to match the devDependency by changing the peerDependencies "sass"
version from "1.45.0" to "1.56.0" (i.e., ensure the peerDependencies "sass" key
uses 1.56.0 to match devDependencies).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (1)
package.json
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-12T18:04:17.698Z
Learnt from: tjuanitas
Repo: box/box-ui-elements PR: 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-08-21T19:19:44.584Z
Learnt from: jpan-box
Repo: box/box-ui-elements PR: 4237
File: src/api/ZipDownload.ts:6-8
Timestamp: 2025-08-21T19:19:44.584Z
Learning: For Box TypeScript SDK integrations, jpan-box prefers deep imports (e.g., from box-typescript-sdk-gen/lib/schemas/...generated.d.ts.js) over defining local types, citing that Box v2 APIs don't typically have breaking changes, making the deep import approach more viable despite potential brittleness.
Applied to files:
package.json
📚 Learning: 2025-09-16T14:53:03.538Z
Learnt from: bfoxx1906
Repo: box/box-ui-elements PR: 4275
File: src/components/form-elements/draft-js-mention-selector/index.js:4-4
Timestamp: 2025-09-16T14:53:03.538Z
Learning: The createMentionSelectorState export in src/components/form-elements/draft-js-mention-selector/index.js was confirmed to be internal-only within the box-ui-elements package, with no external references or documentation, making the breaking rename to createMentionTimestampSelectorState safe.
Applied to files:
package.json
⏰ 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 (2)
package.json (2)
299-300: peerDependencies properly synchronized with devDependencies.The peerDependency updates correctly mirror the devDependency changes for @box packages, maintaining version consistency. Based on learnings, the team is comfortable with raising peerDependency minimum versions for blueprint-web packages.
Also applies to: 306-306, 313-314
127-128: Fix the sass version mismatch between devDependencies and peerDependencies.All @box package versions (unified-share-modal@1.36.4, user-selector@1.55.3, copy-input@1.22.3, blueprint-web@12.112.3, blueprint-web-assets@4.91.3) exist and are valid. However, there is a critical inconsistency: sass is pinned to
1.56.0in devDependencies (line 274) but1.45.0in peerDependencies (line 355). This mismatch will cause behavior differences between your development environment and consumer environments. Update the sass peerDependency to match the devDependency version.⛔ Skipped due to learnings
Learnt from: tjuanitas Repo: box/box-ui-elements PR: 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.
reneshen0328
force-pushed
the
fix-csh-error-handler
branch
from
bd81900 to
b508b4b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @package.json:
- Around line 274-275: The sass-loader upgrade to v16 switched the default to
the modern Sass JS API; verify compatibility by either explicitly preserving the
legacy API or updating configs and code to use the modern API. Inspect
package.json dependency bump for "sass-loader" and update webpack.config.js or
postcss.config.js to set sassOptions.api = "legacy" if you need to keep the old
behavior, or run the full SCSS test-suite (including any custom
functions/mixins/imports) and refactor any code relying on the legacy Sass JS
API to work with the modern API (adjust custom importer/functions usage and
options in the loader config).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (1)
package.json
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-12T18:04:17.698Z
Learnt from: tjuanitas
Repo: box/box-ui-elements PR: 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-08-21T19:19:44.584Z
Learnt from: jpan-box
Repo: box/box-ui-elements PR: 4237
File: src/api/ZipDownload.ts:6-8
Timestamp: 2025-08-21T19:19:44.584Z
Learning: For Box TypeScript SDK integrations, jpan-box prefers deep imports (e.g., from box-typescript-sdk-gen/lib/schemas/...generated.d.ts.js) over defining local types, citing that Box v2 APIs don't typically have breaking changes, making the deep import approach more viable despite potential brittleness.
Applied to files:
package.json
📚 Learning: 2025-09-16T14:53:03.538Z
Learnt from: bfoxx1906
Repo: box/box-ui-elements PR: 4275
File: src/components/form-elements/draft-js-mention-selector/index.js:4-4
Timestamp: 2025-09-16T14:53:03.538Z
Learning: The createMentionSelectorState export in src/components/form-elements/draft-js-mention-selector/index.js was confirmed to be internal-only within the box-ui-elements package, with no external references or documentation, making the breaking rename to createMentionTimestampSelectorState safe.
Applied to files:
package.json
⏰ 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)
package.json (2)
299-300: LGTM: peerDependencies correctly mirror devDependencies.The peerDependencies updates correctly match the corresponding devDependencies changes, maintaining consistency across the package configuration.
Based on learnings, the team is comfortable with raising peerDependency minimum versions for blueprint-web packages, even when it's a breaking change for consumers.
Also applies to: 306-306, 313-314, 355-355
335-335: React 19 support is already officially documented and confirmed by Box UI Elements. No action needed.As of January 2026, box-ui-elements officially supports React 19 alongside React 18. The dual peerDependency version (
^18.0.0 || ^19.0.0) is the correct pattern for supporting both releases. Keeping devDependencies on React 18.3.1 while allowing React 19 in production is a standard approach to maintain backward compatibility testing.Likely an incorrect or invalid review comment.
| "sass": "1.56.0", | ||
| "sass-loader": "^16.0.6", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
sass-loader version 16 breaking changes from version 8 release notes
💡 Result:
Summary of breaking changes
-
v16.0.0 — use modern Sass JS API by default for
sassandsass-embedded(sass options differ between the legacy and modern JS APIs; you can set options.api = "legacy" to restore previous behavior). [1] -
v8.0.0 — breaking items introduced in 8.x: minimum webpack 4.36.0 and Node.js 8.9.0; moved sass-specific options (includePaths, importer, functions, outputStyle, etc.) under sassOptions (functions must be provided via sassOptions); renamed data → prependData; default of sourceMap now depends on webpack devtool. [2]
Sources
[1] webpack-contrib/sass-loader release v16.0.0. [GitHub].
[2] sass-loader changelog / release notes (8.0.0).
🏁 Script executed:
#!/bin/bash
# Find webpack and build configuration files
fd -e js -e ts -e json | grep -E "(webpack|build|config)" | head -20Repository: box/box-ui-elements
Length of output: 402
🏁 Script executed:
#!/bin/bash
# Check package.json for webpack version and build setup
jq '.devDependencies | {webpack, "webpack-cli": .["webpack-cli"]}' package.jsonRepository: box/box-ui-elements
Length of output: 116
🏁 Script executed:
#!/bin/bash
# Search for sass-loader configuration in the codebase
rg -nP --type=js --type=ts 'webpack|sassLoader|sass-loader|loader.*sass' -i -A3 -B3 | head -100Repository: box/box-ui-elements
Length of output: 8164
🏁 Script executed:
#!/bin/bash
# Check postcss.config.js for sass options
cat postcss.config.jsRepository: box/box-ui-elements
Length of output: 160
🏁 Script executed:
#!/bin/bash
# Search for any sassOptions or sass configuration in the codebase
rg -n "sassOptions|sass.*options|prependData|outputStyle|importer" -i --type=js --type=ts --type=jsonRepository: box/box-ui-elements
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Check if there are any .scss files to understand the current sass usage
fd -e scss | head -20Repository: box/box-ui-elements
Length of output: 954
Verify sass-loader v16 Sass JS API compatibility.
The sass-loader upgrade from ^8.0.2 to ^16.0.6 includes a breaking change in v16.0.0: the default switches from the legacy Sass JS API to the modern Sass JS API. Since the project does not explicitly configure sassOptions.api = "legacy" in webpack.config.js or postcss.config.js, all stylesheet compilation will use the modern API. This should be tested to ensure compatibility with existing SCSS files, or the legacy API should be explicitly preserved if needed.
🤖 Prompt for AI Agents
In @package.json around lines 274 - 275, The sass-loader upgrade to v16 switched
the default to the modern Sass JS API; verify compatibility by either explicitly
preserving the legacy API or updating configs and code to use the modern API.
Inspect package.json dependency bump for "sass-loader" and update
webpack.config.js or postcss.config.js to set sassOptions.api = "legacy" if you
need to keep the old behavior, or run the full SCSS test-suite (including any
custom functions/mixins/imports) and refactor any code relying on the legacy
Sass JS API to work with the modern API (adjust custom importer/functions usage
and options in the loader config).
0c01c9a to
6cb4092
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/elements/content-sharing/sharingService.ts (2)
37-45: Critical: Missing error handling inchangeSharedLinkAccess.This method passes an empty object
{}as the 4th parameter (line 42), which should be an error callback. UnlikecreateSharedLink,updateSharedLink, anddeleteSharedLink(which wrap their API calls in Promises with explicit error handlers), this method will not properly catch or propagate API failures.This is inconsistent with the PR objective to "add error handling for content sharing operations."
🔧 Proposed fix to add proper error handling
- const changeSharedLinkAccess = async (access: string): Promise<void> => { - return itemApiInstance.share( - { id, permissions }, - access, - onUpdateSharedLink, - {}, - CONTENT_SHARING_SHARED_LINK_UPDATE_PARAMS, - ); + const changeSharedLinkAccess = async (access: string): Promise<any> => { + return new Promise((resolve, reject) => { + itemApiInstance.share( + { id, permissions }, + access, + data => { + onUpdateSharedLink(data); + resolve(data); + }, + error => { + reject(error); + }, + CONTENT_SHARING_SHARED_LINK_UPDATE_PARAMS, + ); + }); };
47-55: Critical: Missing error handling inchangeSharedLinkPermission.Similar to
changeSharedLinkAccess, this method passes an empty object{}as the 4th parameter (line 52), which should be an error callback. API failures will not be properly handled or propagated.🔧 Proposed fix to add proper error handling
- const changeSharedLinkPermission = async (permissionLevel: string): Promise<void> => { - return itemApiInstance.updateSharedLink( - { id, permissions }, - { permissions: convertSharedLinkPermissions(permissionLevel) }, - onUpdateSharedLink, - {}, - CONTENT_SHARING_SHARED_LINK_UPDATE_PARAMS, - ); + const changeSharedLinkPermission = async (permissionLevel: string): Promise<any> => { + return new Promise((resolve, reject) => { + itemApiInstance.updateSharedLink( + { id, permissions }, + { permissions: convertSharedLinkPermissions(permissionLevel) }, + data => { + onUpdateSharedLink(data); + resolve(data); + }, + error => { + reject(error); + }, + CONTENT_SHARING_SHARED_LINK_UPDATE_PARAMS, + ); + }); };
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/elements/content-sidebar/__tests__/__snapshots__/SidebarFileProperties.test.js.snapis excluded by!**/*.snapyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
package.jsonsrc/elements/content-sharing/SharingNotification.jssrc/elements/content-sharing/__tests__/SharingModal.test.jssrc/elements/content-sharing/__tests__/sharingService.test.tssrc/elements/content-sharing/__tests__/useInvites.test.jssrc/elements/content-sharing/hooks/useInvites.jssrc/elements/content-sharing/sharingService.ts
💤 Files with no reviewable changes (1)
- src/elements/content-sharing/SharingNotification.js
🚧 Files skipped from review as they are similar to previous changes (3)
- src/elements/content-sharing/tests/SharingModal.test.js
- src/elements/content-sharing/hooks/useInvites.js
- src/elements/content-sharing/tests/sharingService.test.ts
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-08-27T17:03:48.322Z
Learnt from: bxie-box
Repo: box/box-ui-elements PR: 4250
File: src/features/classification/applied-by-ai-classification-reason/__tests__/AppliedByAiClassificationReason.test.tsx:44-51
Timestamp: 2025-08-27T17:03:48.322Z
Learning: In test files for bxie-box, prefer using queryByTestId over getByTestId when testing for expected elements that should always exist, as null access serves as validation for regressions or unexpected changes rather than masking issues with defensive assertions.
Applied to files:
src/elements/content-sharing/__tests__/useInvites.test.js
📚 Learning: 2025-06-25T20:05:18.480Z
Learnt from: rafalmaksymiuk
Repo: box/box-ui-elements PR: 4156
File: src/elements/content-sidebar/SidebarNavButton.js:77-102
Timestamp: 2025-06-25T20:05:18.480Z
Learning: Rendering functions (functions that return JSX) are considered anti-patterns in React development because they create new function instances on every render, break React DevTools, interfere with reconciliation, and make testing more difficult.
Applied to files:
src/elements/content-sharing/__tests__/useInvites.test.js
📚 Learning: 2025-10-02T16:47:50.715Z
Learnt from: reneshen0328
Repo: box/box-ui-elements PR: 4322
File: src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js:30-36
Timestamp: 2025-10-02T16:47:50.715Z
Learning: In src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js, mockCurrentUserID is intentionally a number (789) while DEFAULT_USER_API_RESPONSE.id is a string ('789'). This represents different stages of data transformation: mockCurrentUserID is the raw API response before conversion, and DEFAULT_USER_API_RESPONSE.id is the post-conversion state after fetchCurrentUser processes it.
Applied to files:
src/elements/content-sharing/__tests__/useInvites.test.js
📚 Learning: 2025-08-12T18:04:17.698Z
Learnt from: tjuanitas
Repo: box/box-ui-elements PR: 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-08-21T19:19:44.584Z
Learnt from: jpan-box
Repo: box/box-ui-elements PR: 4237
File: src/api/ZipDownload.ts:6-8
Timestamp: 2025-08-21T19:19:44.584Z
Learning: For Box TypeScript SDK integrations, jpan-box prefers deep imports (e.g., from box-typescript-sdk-gen/lib/schemas/...generated.d.ts.js) over defining local types, citing that Box v2 APIs don't typically have breaking changes, making the deep import approach more viable despite potential brittleness.
Applied to files:
package.json
📚 Learning: 2025-09-16T14:53:03.538Z
Learnt from: bfoxx1906
Repo: box/box-ui-elements PR: 4275
File: src/components/form-elements/draft-js-mention-selector/index.js:4-4
Timestamp: 2025-09-16T14:53:03.538Z
Learning: The createMentionSelectorState export in src/components/form-elements/draft-js-mention-selector/index.js was confirmed to be internal-only within the box-ui-elements package, with no external references or documentation, making the breaking rename to createMentionTimestampSelectorState safe.
Applied to files:
package.json
🧬 Code graph analysis (1)
src/elements/content-sharing/sharingService.ts (2)
src/elements/content-sharing/utils/convertSharingServiceData.ts (1)
convertSharedLinkSettings(35-83)src/elements/content-sharing/constants.js (2)
CONTENT_SHARING_SHARED_LINK_UPDATE_PARAMS(61-63)CONTENT_SHARING_SHARED_LINK_UPDATE_PARAMS(61-63)
⏰ 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). (4)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: lint_test_build
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Summary
🔇 Additional comments (5)
src/elements/content-sharing/__tests__/useInvites.test.js (1)
1-126: LGTM! Test updates align with Promise-based hook behavior.The test changes correctly reflect the new Promise-based flow by:
- Removing
actwrappers in favor of directawaitcalls- Handling error paths with
.catch(() => {})for test stability- Properly asserting null returns for early-exit scenarios
- Covering all critical paths including success, failure, and multi-payload scenarios
src/elements/content-sharing/sharingService.ts (3)
57-74: Excellent error handling implementation!The Promise wrapper with explicit resolve/reject handlers ensures:
- Success side effects are executed via
onUpdateSharedLink(data)before resolving- Errors are properly propagated via
reject(error)- Callers can await and catch failures consistently
This pattern enables the PR objective of showing error notifications instead of success notifications on API failures.
76-91: Excellent error handling implementation!Same solid pattern as
updateSharedLink- proper Promise wrapping with success callback invocation and error rejection.
93-108: Excellent error handling implementation!Consistent with the other properly wrapped methods. The
onRemoveSharedLinkcallback is correctly invoked before resolving.package.json (1)
127-128: All specified dependency versions are valid and exist on npm.The version updates have been verified:
@box/blueprint-web@12.112.3✓@box/blueprint-web-assets@4.91.3✓@box/copy-input@1.22.3✓@box/unified-share-modal@1.36.4✓@box/user-selector@1.55.3✓sass@1.56.0✓sass-loader@16.0.6✓No deprecation warnings detected. Regarding breaking changes from major version updates (e.g.,
sass-loader8.0.2 → 16.0.6,@box/unified-share-modal1.7.1 → 1.36.4): the team is documented as being comfortable with raising peerDependency minimum versions for blueprint-web packages, even when breaking changes are involved.
What
Testing
Summary by CodeRabbit
Chores
Refactor
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.