fix(content-sharing): Add error handler #4414
Conversation
WalkthroughConverts content-sharing APIs and callers from callback-style to Promise-based flows, updates tests to await/reject paths, adds loading state around batched invites, and bumps multiple dev/peer dependencies; two explicit loading clears were removed from SharingNotification. Changes
Sequence Diagram(s)sequenceDiagram
participant Component
participant useInvites
participant sharingService
participant ItemAPI
participant CallbackHandler
Component->>useInvites: createPostCollaborationFn(payloads)
useInvites->>useInvites: setIsLoading(true)
useInvites->>sharingService: sendCollabRequest(payload)
sharingService->>ItemAPI: perform API call (invite/create/update/delete)
alt Success
ItemAPI-->>sharingService: response
sharingService->>CallbackHandler: onUpdateSharedLink/onRemoveSharedLink(response)
CallbackHandler-->>sharingService: callback handled
sharingService-->>useInvites: Promise.resolve(response)
else Failure
ItemAPI-->>sharingService: error
sharingService-->>useInvites: Promise.reject(error)
end
useInvites->>useInvites: Promise.all resolves/rejects
useInvites->>Component: return aggregated results / propagate error
useInvites->>useInvites: setIsLoading(false)
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 (1)
📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (3)📚 Learning: 2025-08-12T18:04:17.698ZApplied to files:
📚 Learning: 2025-09-16T14:53:03.538ZApplied to files:
📚 Learning: 2025-08-21T19:19:44.584ZApplied to files:
🪛 Biome (2.1.2)src/elements/content-sharing/hooks/useInvites.js[error] 55-55: type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax. TypeScript only syntax (parse) [error] 56-56: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax. TypeScript only syntax (parse) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (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.
reneshen0328
force-pushed
the
fix-csh-error-handler
branch
from
f5dfc74 to
cc9bfc1
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 (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
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.
reneshen0328
force-pushed
the
fix-csh-error-handler
branch
from
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.
Merge Queue Status✅ The pull request has been merged at 74e3958 This pull request spent 6 seconds in the queue, with no time running CI. Required conditions to merge
|
What
Testing
Summary by CodeRabbit
Chores
Refactor
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.