fix(content-sharing): Improve content sharing performance and add consumer ability to override config #4413
Conversation
WalkthroughWidened the public Changes
Sequence Diagram(s)sequenceDiagram
participant Consumer
participant ContentSharing
participant ContentSharingV2
participant UnifiedShareModal
Consumer->>ContentSharing: render(api, config, itemID, itemType, ...)
ContentSharing->>ContentSharingV2: render(api, config, itemId, itemType, children)
ContentSharingV2->>ContentSharingV2: memoize mergedConfig = { sharedLinkEmail: false, ...usmConfig }
ContentSharingV2->>UnifiedShareModal: render with mergedConfig and props
UnifiedShareModal-->>Consumer: modal UI reflects mergedConfig
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
reneshen0328
changed the title
reneshen0328
force-pushed
the
fix-improve-content-sharing
branch
from
3a74221 to
97b1e57
Compare
![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
🤖 Fix all issues with AI agents
In @src/elements/content-sharing/ContentSharingV2.tsx:
- Line 36: The default parameter in ContentSharingV2 (config: usmConfig = {})
creates a fresh object each render and breaks memoization; define a stable empty
object constant (e.g., STABLE_EMPTY_CONFIG) outside the component and change the
component to use that stable reference when no config is passed (either via
default param pointing to that constant or by assigning inside the function),
then ensure the useMemo that relies on usmConfig uses this stable reference so
it no longer recomputes every render.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/elements/content-sharing/ContentSharing.jssrc/elements/content-sharing/ContentSharingV2.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-11T14:43:02.677Z
Learnt from: jpan-box
Repo: box/box-ui-elements PR: 4166
File: src/elements/content-sidebar/SidebarNav.js:126-126
Timestamp: 2025-07-11T14:43:02.677Z
Learning: In the box-ui-elements repository, there's a file-type-based pattern for internationalization: TypeScript files (.tsx) predominantly use the modern useIntl hook (41 vs 15 files), while JavaScript files (.js) predominantly use the legacy injectIntl HOC (64 vs 5 files). New TypeScript components should use useIntl, while existing JavaScript components typically continue using injectIntl for consistency.
Applied to files:
src/elements/content-sharing/ContentSharingV2.tsx
🪛 Biome (2.1.2)
src/elements/content-sharing/ContentSharing.js
[error] 12-12: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 24-24: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 25-25: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 26-26: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: lint_test_build
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Summary
🔇 Additional comments (6)
src/elements/content-sharing/ContentSharing.js (3)
12-12: LGTM! Type imports are correctly structured.The addition of the
Configurationtype import from@box/unified-share-modalproperly supports the widened config prop type. The static analysis warnings aboutimport typebeing TypeScript-only are false positives—this is valid Flow syntax in a Flow-annotated file.Also applies to: 24-28
40-40: LGTM! Backward-compatible type widening.The config prop type now accepts both
USMConfigandConfiguration, allowing consumers to use either type while maintaining backward compatibility.
128-128: LGTM! Config prop correctly threaded through.The config prop is now properly passed to
ContentSharingV2, enabling configuration customization as intended by the PR objectives.src/elements/content-sharing/ContentSharingV2.tsx (3)
7-7: LGTM! Type imports properly support the new config prop.The addition of
ConfigurationandUSMConfigtype imports correctly enables type-safe config handling.Also applies to: 18-18
28-29: LGTM! Prop definition matches parent component.The config prop definition correctly uses the union type
USMConfig | Configuration, maintaining type consistency withContentSharing.js.
214-214: LGTM! Config merging logic is correct.The memoization correctly merges the default
sharedLinkEmail: falsewith the provided config, allowing consumers to override defaults. The dependency array[usmConfig]is appropriate. (Note: This will work optimally once the default parameter issue on line 36 is addressed.)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/elements/content-sharing/__tests__/ContentSharingV2.test.tsx:
- Around line 180-190: The test "should allow custom config to override default
config" incorrectly sets config.sharedLinkEmail; update it to set the correct
property used by the component: pass config: { showEmailSharedLinkForm: true }
when calling renderComponent so the "Send Shared Link" button visibility is
controlled by showEmailSharedLinkForm; locate the test in
ContentSharingV2.test.tsx and change the config object in the renderComponent
call accordingly (the test uses apiWithSharedLink, getFileMockWithSharedLink,
and renderComponent).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/elements/content-sharing/ContentSharing.jssrc/elements/content-sharing/ContentSharingV2.tsxsrc/elements/content-sharing/__tests__/ContentSharingV2.test.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/elements/content-sharing/ContentSharingV2.tsx
🧰 Additional context used
🪛 Biome (2.1.2)
src/elements/content-sharing/ContentSharing.js
[error] 12-12: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 24-24: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 25-25: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 26-26: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint_test_build
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Summary
🔇 Additional comments (5)
src/elements/content-sharing/__tests__/ContentSharingV2.test.tsx (1)
173-178: LGTM! Good coverage for custom config rendering.The test correctly verifies that UnifiedShareModal renders when a custom config is provided.
src/elements/content-sharing/ContentSharing.js (4)
12-12: LGTM! Configuration type import added correctly.This enables the widened config prop type to support both USMConfig and Configuration from the unified-share-modal library.
40-40: LGTM! Config prop type appropriately widened.The expanded type
USMConfig | Configurationallows consumers to provide either configuration format, which aligns with the PR objectives.
128-128: LGTM! Config properly threaded to ContentSharingV2.The config prop is now correctly passed through to the ContentSharingV2 component.
12-12: Static analysis errors are false positives.Biome is flagging
import typeas "TypeScript only," but this is a Flow file (note the@flowcomment at line 2). Flow fully supportsimport typesyntax—these warnings can be safely ignored.Also applies to: 24-26
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/elements/content-sharing/ContentSharingV2.tsx (1)
35-35: Default empty object defeats memoization when no config is provided.The inline default
config: usmConfig = {}creates a new object reference on each render. SinceuseMemoon line 213 depends onusmConfig, the memo recomputes every render when consumers don't pass a config — defeating the PR's goal of avoiding unnecessary renders.Define a stable default outside the component:
♻️ Proposed fix
+const DEFAULT_USM_CONFIG: Configuration = {}; + -function ContentSharingV2({ api, children, config: usmConfig = {}, itemId, itemType }: ContentSharingV2Props) { +function ContentSharingV2({ api, children, config: usmConfig = DEFAULT_USM_CONFIG, itemId, itemType }: ContentSharingV2Props) {Also applies to: 213-213
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/elements/content-sharing/ContentSharingV2.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-11T14:43:02.677Z
Learnt from: jpan-box
Repo: box/box-ui-elements PR: 4166
File: src/elements/content-sidebar/SidebarNav.js:126-126
Timestamp: 2025-07-11T14:43:02.677Z
Learning: In the box-ui-elements repository, there's a file-type-based pattern for internationalization: TypeScript files (.tsx) predominantly use the modern useIntl hook (41 vs 15 files), while JavaScript files (.js) predominantly use the legacy injectIntl HOC (64 vs 5 files). New TypeScript components should use useIntl, while existing JavaScript components typically continue using injectIntl for consistency.
Applied to files:
src/elements/content-sharing/ContentSharingV2.tsx
⏰ 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 (2)
src/elements/content-sharing/ContentSharingV2.tsx (2)
7-7: LGTM!Correctly importing the
Configurationtype from@box/unified-share-modalalongside the component that uses it.
27-28: LGTM!Clear documentation and proper optional typing for the new configuration prop.
reneshen0328
force-pushed
the
fix-improve-content-sharing
branch
from
0a4419a to
806abc1
Compare
reneshen0328
force-pushed
the
fix-improve-content-sharing
branch
from
806abc1 to
7bbe8f3
Compare
src/elements/content-sharing/__tests__/ContentSharingV2.test.tsx
Outdated
Show resolved
Hide resolved
src/elements/content-sharing/__tests__/ContentSharingV2.test.tsx
Outdated
Show resolved
Hide resolved
src/elements/content-sharing/__tests__/ContentSharingV2.test.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/elements/content-sharing/__tests__/ContentSharingV2.test.tsx (2)
171-174: Consider strengthening this test to validate config usage.This test verifies the component renders when
configis provided, but doesn't validate thatcollaborationLimit: 3actually affects behavior. Consider adding assertions that verify the config property is respected (e.g., checking that collaboration limits are enforced in the UI).
176-184: Test correctly validates config override behavior.The test properly verifies that
sharedLinkEmail: trueenables the "Send Shared Link" button. For more comprehensive coverage, consider adding a complementary test that verifies the button is not visible when using the default config (without the override).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/elements/content-sharing/ContentSharingV2.tsxsrc/elements/content-sharing/__tests__/ContentSharingV2.test.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/elements/content-sharing/ContentSharingV2.tsx
🧰 Additional context used
🧠 Learnings (2)
📚 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__/ContentSharingV2.test.tsx
📚 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__/ContentSharingV2.test.tsx
⏰ 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 (1)
src/elements/content-sharing/__tests__/ContentSharingV2.test.tsx (1)
168-169: Good improvement to async assertion pattern.Replacing
waitForwithfindByRoleis more idiomatic, asfindByRolealready waits for the element to appear.
Merge Queue Status✅ The pull request has been merged at 5099113 This pull request spent 5 seconds in the queue, with no time running CI. Required conditions to merge
|
What
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.