feat(content-sharing): Handle error when fetch init data failed #4350
Conversation
WalkthroughContentSharing now wraps the V2 component with Providers and Internationalize when the contentSharingV2 feature is enabled. ContentSharingV2 drops hasProviders/language/messages props, implements internalized error handling with localized notifications, and renders UnifiedShareModal directly. Tests, stories, messages, and mocks updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Parent as ContentSharing (parent)
participant Intl as Internationalize
participant Prov as Providers
participant CSV2 as ContentSharingV2
participant API as API
participant N as Notification.Provider
Note over Parent: Parent now composes wrappers when feature enabled
Parent->>Intl: provide language/messages
Intl->>Prov: render children
Prov->>CSV2: pass api, itemId, itemType, children
CSV2->>API: fetchItem / fetchCurrentUser / collaborators / avatars
alt fetch error
API-->>CSV2: ElementsXhrError
CSV2->>CSV2: getError -> map via CONTENT_SHARING_ERRORS + messages
CSV2->>N: addNotification(localized error)
else success
API-->>CSV2: data
CSV2->>CSV2: render UnifiedShareModal
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/elements/content-sharing/__tests__/ContentSharingV2.test.tsx (1)
173-227: Good error handling test coverage. Consider additional status codes.The tests properly validate the error notification flow for key scenarios. However, consider adding tests for other status codes defined in
CONTENT_SHARING_ERRORS(403, 404, 500) to ensure complete coverage of the error mapping.Add test cases for additional error statuses:
test('should render no access message for error.status 403', async () => { const error = { status: 403 }; renderComponent({ api: createErrorApi(error) }); await waitFor(() => { expect(mockAddNotification).toHaveBeenCalledWith( expect.objectContaining({ styledText: 'You do not have access to this item.', variant: 'error', }), ); }); }); test('should render not found message for error.status 404', async () => { const error = { status: 404 }; renderComponent({ api: createErrorApi(error) }); await waitFor(() => { expect(mockAddNotification).toHaveBeenCalledWith( expect.objectContaining({ styledText: 'Could not find shared link for this item.', variant: 'error', }), ); }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
i18n/en-US.propertiesis excluded by!i18n/**
📒 Files selected for processing (7)
src/elements/content-sharing/ContentSharing.js(2 hunks)src/elements/content-sharing/ContentSharingV2.tsx(8 hunks)src/elements/content-sharing/__tests__/ContentSharingV2.test.tsx(3 hunks)src/elements/content-sharing/apis/__tests__/fetchAvatars.test.ts(1 hunks)src/elements/content-sharing/apis/fetchAvatars.ts(1 hunks)src/elements/content-sharing/messages.js(1 hunks)src/elements/content-sharing/stories/ContentSharing.stories.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/elements/content-sharing/apis/__tests__/fetchAvatars.test.ts (2)
src/elements/content-sharing/apis/fetchAvatars.ts (1)
fetchAvatars(3-23)src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js (2)
MOCK_ITEM(14-18)MOCK_ITEM(14-18)
src/elements/content-sharing/__tests__/ContentSharingV2.test.tsx (1)
src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js (2)
MOCK_ITEM(14-18)MOCK_ITEM(14-18)
src/elements/content-sharing/ContentSharingV2.tsx (6)
src/elements/content-sharing/SharingModal.js (2)
getError(106-110)errorObject(113-113)src/elements/content-sharing/messages.js (1)
messages(3-91)src/elements/content-sharing/constants.js (2)
CONTENT_SHARING_ERRORS(37-43)CONTENT_SHARING_ERRORS(37-43)src/elements/content-sharing/apis/fetchItem.ts (1)
fetchItem(8-22)src/elements/content-sharing/apis/fetchCurrentUser.ts (1)
fetchCurrentUser(7-15)src/elements/content-sharing/apis/fetchAvatars.ts (1)
fetchAvatars(3-23)
src/elements/content-sharing/ContentSharing.js (1)
src/elements/content-sharing/messages.js (1)
messages(3-91)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (15)
src/elements/content-sharing/apis/__tests__/fetchAvatars.test.ts (1)
47-60: LGTM! Test correctly validates error propagation.The test now properly expects the promise to reject when any avatar fetch fails, aligning with the updated error handling in
fetchAvatars.tswhere errors are rethrown.src/elements/content-sharing/ContentSharing.js (2)
15-16: LGTM! Clean imports for wrapper components.
119-131: LGTM! Proper wrapper composition for V2.The nesting of
Internationalize→Providers→ContentSharingV2correctly distributes props and establishes the necessary context hierarchy for localization and provider functionality.src/elements/content-sharing/stories/ContentSharing.stories.js (1)
16-25: LGTM! Story configuration properly reflects wrapper requirements.src/elements/content-sharing/messages.js (1)
81-90: LGTM! Proper internationalization for notification accessibility.The new messages follow the established pattern and provide necessary accessibility labels for the notification system.
src/elements/content-sharing/__tests__/ContentSharingV2.test.tsx (3)
3-3: LGTM! Required provider imports for test setup.
49-53: LGTM! Proper notification mock for testing.
58-65: LGTM! Test render setup matches production wrapper pattern.The provider nesting correctly mirrors the structure established in
ContentSharing.js.src/elements/content-sharing/ContentSharingV2.tsx (6)
2-19: LGTM! Necessary imports for error handling implementation.
21-30: LGTM! Props correctly simplified.The removal of
hasProviders,language, andmessagesaligns with moving these concerns to the parentContentSharingcomponent viaInternationalizeandProviderswrappers.
44-45: LGTM! Proper hook initialization.
120-131: LGTM! Proper error handling for item fetch.
212-228: LGTM! Simplified rendering with wrappers moved to parent.The direct rendering of
UnifiedShareModalis correct sinceInternationalizeandProvidersare now handled by the parentContentSharingcomponent.
149-196: No issues found. Owner data is properly guarded before use.The avatar fetch effect includes an explicit guard clause at line 176 (
if (...!owner.id) return;) that prevents execution untilowner.idis populated. Sinceowneris initialized with empty strings and populated inhandleGetItemSuccess(line 76), theownerEntryconstruction is safe—it only executes after the guard confirmsowner.idis truthy.src/elements/content-sharing/apis/fetchAvatars.ts (1)
15-17: Confirm fail-fast error handling is intentional and aligns with UX requirements.The fail-fast behavior is tested and intentional: the test "should handle avatar fetch errors gracefully" explicitly validates that
fetchAvatarsrejects on any single error. When rejection occurs,ContentSharingV2catches it, displays an error notification viagetError(error), and leavesavatarUrlMapnull—preventing any avatars from being displayed.While the codebase supports null/undefined avatarUrl (Avatar component has fallback rendering, convertCollaborators handles null maps), the current implementation doesn't return partial results on error. The change from graceful degradation to fail-fast is a meaningful UX shift that warrants confirmation: is it acceptable for users to see no avatars when a single user's avatar fails to load, rather than seeing available avatars with a warning?
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/elements/content-sharing/ContentSharingV2.tsx (1)
87-94: Fix potential undefined errorObject causing runtime error.If the error status code is not mapped in
CONTENT_SHARING_ERRORS(e.g., 418, 429, 502), thenCONTENT_SHARING_ERRORS[error.status]returnsundefined, makingerrorObject = messages[undefined] = undefined. Line 102'sformatMessage(errorObject)will then fail with a runtime error. This directly addresses the concern raised by jfox-box in past review comments.Apply this diff to add fallback handling:
- let errorObject; - if (error.status) { - errorObject = messages[CONTENT_SHARING_ERRORS[error.status]]; - } else if (error.response && error.response.status) { - errorObject = messages[CONTENT_SHARING_ERRORS[error.response.status]]; - } else { - errorObject = messages.loadingError; - } + const statusCode = error.status || error.response?.status; + const errorKey = (statusCode && CONTENT_SHARING_ERRORS[statusCode]) || 'loadingError'; + const errorObject = messages[errorKey];
🧹 Nitpick comments (1)
src/elements/content-sharing/ContentSharingV2.tsx (1)
79-106: Consider resetting error state on successful data fetch.While the current implementation resets
hasErrorwhen the API changes (line 110), it does not reset on successful data retrieval. Although the fetch effects have one-shot guards that prevent re-runs, resettinghasErrorinhandleGetItemSuccesswould improve clarity and future-proof against edge cases where multiple initialization attempts might occur.Add error reset in the success handler:
const handleGetItemSuccess = React.useCallback(itemData => { const { collaborationRoles: collaborationRolesFromApi, item: itemFromApi, ownedBy, sharedLink: sharedLinkFromApi, sharingService: sharingServicePropsFromApi, } = convertItemResponse(itemData); + setHasError(false); setItem(itemFromApi); setSharedLink(sharedLinkFromApi); setSharingServiceProps(sharingServicePropsFromApi); setCollaborationRoles(collaborationRolesFromApi); setOwner({ id: ownedBy.id, email: ownedBy.login, name: ownedBy.name }); }, []);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/elements/content-sharing/ContentSharingV2.tsx(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/elements/content-sharing/ContentSharingV2.tsx (6)
src/elements/content-sharing/SharingModal.js (2)
getError(106-110)errorObject(113-113)src/elements/content-sharing/messages.js (1)
messages(3-91)src/elements/content-sharing/constants.js (2)
CONTENT_SHARING_ERRORS(37-43)CONTENT_SHARING_ERRORS(37-43)src/elements/content-sharing/apis/fetchItem.ts (1)
fetchItem(8-22)src/elements/content-sharing/apis/fetchCurrentUser.ts (1)
fetchCurrentUser(7-15)src/elements/content-sharing/apis/fetchAvatars.ts (1)
fetchAvatars(3-23)
⏰ 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 (9)
src/elements/content-sharing/ContentSharingV2.tsx (9)
1-30: LGTM! Clean integration of internationalization and notification dependencies.The imports correctly add
useIntlfor localized messaging anduseNotificationfor error notifications. The type imports forElementsXhrErrorand error constants are appropriate for the new error handling logic.
32-43: LGTM! State declarations are correctly typed.The
hasErrorboolean state is properly declared and used consistently throughout the component as a deduplication flag. The error message itself is not stored but derived and passed directly to notifications, which is the correct pattern.
44-60: LGTM! Hooks are correctly integrated.The
useIntlanduseNotificationhooks are properly initialized and their methods are used appropriately in the error handling logic.
109-118: LGTM! Proper state reset on API change.The error state is correctly reset along with all other state when the API instance changes, ensuring clean initialization for new API contexts.
121-132: LGTM! Robust error handling for item fetch.The fetch item effect is properly wrapped in try-catch with error routing through the centralized
getErrorhandler. Dependencies are correctly specified.
135-158: LGTM! Proper error handling for user fetch.The current user fetch effect correctly wraps the API call in try-catch and routes errors through
getError. Dependencies are appropriate.
161-173: LGTM! Defensive error handling for collaborators fetch.The collaborators fetch correctly sets empty data on failure (line 169) to prevent infinite retries while still surfacing the error through
getError. This is a good defensive pattern.
176-197: LGTM! Owner entry construction is appropriately placed.The owner entry construction (lines 180-186) correctly creates an object matching the collaborator structure for avatar fetching. Including the owner in the avatar batch request is a logical consolidation of the avatar-fetching logic. Error handling is properly implemented.
213-228: LGTM! Simplified rendering logic.The render now directly outputs
UnifiedShareModalwhen the item is available, with internationalization and provider context managed by the parent wrapper. This is a clean separation of concerns.
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/ContentSharingV2.tsx (1)
112-122: Stale data bug: state doesn’t reset when itemId/itemType change.Only resetting on API changes means switching props to a new item shows the old item and suppresses re‑fetch. Reset on itemId/itemType as well.
Apply this diff:
- // Reset state if the API has changed - React.useEffect(() => { + // Reset state if the API or target item changed + React.useEffect(() => { setHasError(false); setItem(null); setSharedLink(null); setCurrentUser(null); setCollaborationRoles(null); setAvatarUrlMap(null); setCollaborators(null); setCollaboratorsData(null); - }, [api]); + }, [api, itemId, itemType]);
♻️ Duplicate comments (1)
src/elements/content-sharing/ContentSharingV2.tsx (1)
79-110: Error dedupe never resets across item changes; simplify status parsing.
- Without resetting hasError when itemId/itemType change or after a success, later valid errors are suppressed. Add a reset on item change/success. This echoes earlier feedback about error reset.
- Minor: use optional chaining to derive status and collapse branches.
Suggested diff (logic only):
- let errorMessage; - if (error.status) { - errorMessage = messages[CONTENT_SHARING_ERRORS[error.status]]; - } else if (error.response && error.response.status) { - errorMessage = messages[CONTENT_SHARING_ERRORS[error.response.status]]; - } else { - errorMessage = messages.loadingError; - } - - if (!errorMessage) { - errorMessage = messages.defaultErrorNoticeText; - } + const status = (error as any)?.status ?? (error as any)?.response?.status; + const key = status ? CONTENT_SHARING_ERRORS[status] : undefined; + const descriptor = key ? messages[key] : messages.loadingError; + const errorMessage = descriptor || messages.defaultErrorNoticeText;Additionally, reset hasError on success:
const handleGetItemSuccess = React.useCallback(itemData => { const { collaborationRoles: collaborationRolesFromApi, item: itemFromApi, ownedBy, sharedLink: sharedLinkFromApi, sharingService: sharingServicePropsFromApi, } = convertItemResponse(itemData); + setHasError(false); setItem(itemFromApi); ... }, []);
🧹 Nitpick comments (4)
src/elements/content-sharing/messages.js (1)
86-95: A11y labels look correct; consider tiny clarity tweak.Current labels are fine. Optional: “Error notification” can be slightly clearer than just “Error” for screen readers, depending on Notification’s semantics.
src/elements/content-sharing/ContentSharingV2.tsx (1)
212-227: Prefer explicit null over boolean short‑circuit in render.Clearer typing and avoids returning a boolean.
-return ( - item && ( - <UnifiedShareModal +return item ? ( + <UnifiedShareModal config={config} collaborationRoles={collaborationRoles} collaborators={collaborators} contactService={contactService} currentUser={currentUser} item={item} sharedLink={sharedLink} sharingService={sharingService} > {children} </UnifiedShareModal> - ) - ); + ) : null;src/elements/content-sharing/__tests__/ContentSharingV2.test.tsx (2)
49-53: Mocking blueprint is fine; ensure cleanup if other suites rely on real hook.If other suites need the real useNotification, restore in those files or scope the mock with jest.doMock per‑suite.
173-242: Great coverage of status mapping; add dedupe and item-change cases.
- Add a test that triggers two consecutive errors and asserts addNotification called once.
- Add a test that, after an error, changing itemId causes state reset and then re‑fetch (and allows a new error to surface).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
i18n/en-US.propertiesis excluded by!i18n/**
📒 Files selected for processing (3)
src/elements/content-sharing/ContentSharingV2.tsx(8 hunks)src/elements/content-sharing/__tests__/ContentSharingV2.test.tsx(3 hunks)src/elements/content-sharing/messages.js(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/elements/content-sharing/__tests__/ContentSharingV2.test.tsx (1)
src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js (2)
MOCK_ITEM(21-25)MOCK_ITEM(21-25)
src/elements/content-sharing/ContentSharingV2.tsx (5)
src/elements/content-sharing/SharingModal.js (3)
getError(106-110)handleGetItemSuccess(98-103)getUserSuccess(169-176)src/elements/content-sharing/messages.js (1)
messages(3-96)src/elements/content-sharing/constants.js (2)
CONTENT_SHARING_ERRORS(37-43)CONTENT_SHARING_ERRORS(37-43)src/elements/content-sharing/apis/fetchItem.ts (1)
fetchItem(8-22)src/elements/content-sharing/apis/fetchCurrentUser.ts (1)
fetchCurrentUser(7-15)
⏰ 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 (3)
src/elements/content-sharing/messages.js (1)
4-8: LGTM: Sensible default error copy.Copy, id, and description look good. No issues.
src/elements/content-sharing/__tests__/ContentSharingV2.test.tsx (1)
60-65: Verify Intl provider availability.ContentSharingV2 uses useIntl. You rely on withBlueprintModernization to provide Intl in tests. Please confirm this HOC wraps with Intl; if not, wrap the render with IntlProvider to avoid hidden coupling.
src/elements/content-sharing/ContentSharingV2.tsx (1)
154-161: Wrong user fetched: fetchCurrentUser uses itemId.Fetching the “current user” with the content itemId is incorrect and will return the wrong entity. Use the current user endpoint/id (typically 'me') and drop itemId from the call.
Apply this change here and update fetchCurrentUser accordingly:
- const userData = await fetchCurrentUser({ api, itemId }); + const userData = await fetchCurrentUser({ api });And in src/elements/content-sharing/apis/fetchCurrentUser.ts (separate file):
-export const fetchCurrentUser = async ({ api, itemId }: BaseFetchProps): Promise<User | null> => { - return new Promise((resolve, reject) => { - api.getUsersAPI(false).getUser(itemId, resolve, reject, { +export const fetchCurrentUser = async ({ api }: { api: API }): Promise<User | null> => { + return new Promise((resolve, reject) => { + api.getUsersAPI(false).getUser('me', resolve, reject, { params: { fields: [FIELD_ENTERPRISE, FIELD_HOSTNAME].toString(), }, }); }); };⛔ Skipped due to learnings
Learnt from: jfox-box PR: box/box-ui-elements#4285 File: src/elements/content-sharing/ContentSharingV2.tsx:108-118 Timestamp: 2025-09-25T22:28:08.867Z Learning: In ContentSharingV2, the getUsersAPI().getUser() call should use itemID as the first parameter, not 'me', when fetching user data for content sharing functionality.Learnt from: reneshen0328 PR: box/box-ui-elements#4322 File: src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js:30-36 Timestamp: 2025-10-02T16:47:50.715Z Learning: In src/elements/content-sharing/utils/__mocks__/ContentSharingV2Mocks.js, mockCurrentUserID is intentionally a number (789) while DEFAULT_USER_API_RESPONSE.id is a string ('789'). This represents different stages of data transformation: mockCurrentUserID is the raw API response before conversion, and DEFAULT_USER_API_RESPONSE.id is the post-conversion state after fetchCurrentUser processes it.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/elements/content-sharing/ContentSharingV2.tsx (2)
125-136: Remove unnecessary dependency from useEffect.The
sharedLinkdependency on line 136 is not used within the effect body and is unnecessary. The guard clause on line 126 already prevents re-fetching whenitemexists.Apply this diff to clean up the dependencies:
- }, [api, item, itemId, itemType, sharedLink, handleGetItemSuccess, getError]); + }, [api, item, itemId, itemType, handleGetItemSuccess, getError]);
139-162: Remove unnecessary dependencies from useEffect.The
sharedLinkanditemTypedependencies on line 162 are not used within the effect body. While the guard clause prevents unnecessary re-execution, it's better to keep dependencies accurate.Apply this diff to clean up the dependencies:
- }, [api, currentUser, item, itemId, itemType, sharedLink, getError]); + }, [api, currentUser, item, itemId, getError]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/elements/content-sharing/ContentSharingV2.tsx(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/elements/content-sharing/ContentSharingV2.tsx (5)
src/elements/content-sharing/SharingModal.js (3)
getError(106-110)handleGetItemSuccess(98-103)getUserSuccess(169-176)src/elements/content-sharing/messages.js (1)
messages(3-96)src/elements/content-sharing/constants.js (2)
CONTENT_SHARING_ERRORS(37-43)CONTENT_SHARING_ERRORS(37-43)src/elements/content-sharing/apis/fetchItem.ts (1)
fetchItem(8-22)src/elements/content-sharing/apis/fetchCurrentUser.ts (1)
fetchCurrentUser(7-15)
⏰ 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 (8)
src/elements/content-sharing/ContentSharingV2.tsx (8)
2-2: LGTM! Imports are clean and necessary.The new imports for internationalization (useIntl), notifications (useNotification), error constants (CONTENT_SHARING_ERRORS), error types (ElementsXhrError), and messages are all properly utilized in the error handling implementation.
Also applies to: 4-4, 11-11, 16-16, 19-19
35-35: Consider scope of error deduplication flag.The
hasErrorflag prevents multiple notifications, but it doesn't distinguish between errors fromfetchItemvsfetchCurrentUser. If both API calls fail sequentially, only the first error will be displayed.If this is intentional (showing only the first error encountered during initialization), the current implementation is fine. However, if you want users to see errors from both calls, consider either:
- Using separate flags:
hasFetchItemErrorandhasFetchCurrentUserError- Tracking which APIs have errored:
erroredApis: Set<string>Can you confirm whether suppressing the second error is the desired behavior?
44-45: LGTM! Standard hook usage.The hooks are correctly utilized for internationalization and notification management.
79-110: LGTM! Error handling is comprehensive.The error handling logic correctly:
- Implements deduplication to prevent notification spam
- Handles multiple error object shapes (error.status and error.response.status)
- Provides fallbacks when status codes aren't in CONTENT_SHARING_ERRORS
- Uses accessible notification configuration with proper ARIA labels
- Leverages internationalization for user-facing messages
113-122: LGTM! State reset on API change is correct.The effect properly resets all state including
hasErrorwhen the API instance changes, allowing fresh error notifications for new API instances.
63-77: LGTM! Success handler is correctly implemented.The callback is properly memoized and follows the same pattern as the reference implementation in SharingModal.js.
212-227: Verify the UX for the loading state.The component only renders when
itemexists, which means the trigger (children) is also hidden during data fetching. This differs from typical patterns where the trigger remains visible but the modal shows a loading state.Can you confirm this is the intended UX? If the trigger should be visible immediately, consider:
- Rendering children unconditionally and handling the loading state within UnifiedShareModal
- Showing a loading indicator on the trigger itself
Example approach:
return ( <> {children} {item && ( <UnifiedShareModal // ... props > {children} </UnifiedShareModal> )} </> );Or verify that the parent component handles showing the trigger before this component mounts.
21-30: LGTM! Props interface is clean and well-typed.The simplified interface appropriately removes provider-related props since the component now uses hooks internally. This follows React best practices for hook-based components.
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
|
@coderabbitai generate docstrings |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @reneshen0328. * #4350 (comment) The following files were modified: * `src/elements/content-sharing/ContentSharing.js` * `src/elements/content-sharing/ContentSharingV2.tsx`
What
fetchItemandfetchCurrentUserAPI callfetchCollaboratorsthe error notification is handled by default in the shared feature Unified Share Modal already.Testing
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Docs / Stories