feat(content-sharing): Create ContentSharingV2 component #4282
Conversation
|
|
WalkthroughI pity the fool who skips this: adds a feature-flagged V2 path that renders a new ContentSharingV2 (UnifiedShareModal-based), updates ContentSharing props to accept children/features/hasProviders, adds V2 stories and visual tests, and expands Jest transformIgnorePatterns for three Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CS as ContentSharing
participant FF as isFeatureEnabled
participant CSV2 as ContentSharingV2
participant I18n as Internationalize
participant Prov as Providers
participant USM as UnifiedShareModal
participant SM as SharingModal
User->>CS: render / click trigger
CS->>FF: check(features, "contentSharingV2")
alt V2 enabled
FF-->>CS: true
CS->>CSV2: render(itemID,itemType,language,messages,children,hasProviders)
CSV2->>I18n: wrap(language,messages)
I18n->>Prov: wrap(hasProviders)
Prov->>USM: render(item)
USM-->>User: show Unified Share Modal (V2)
else Legacy
FF-->>CS: false
CS->>SM: render legacy SharingModal flow
SM-->>User: show legacy Share UI
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/elements/content-sharing/ContentSharing.js (1)
25-54: Flow type missingfeaturesprop.Function destructures
features, butContentSharingPropsdoesn’t declare it. Flow will bark. Add an optional boolean map.type ContentSharingProps = { @@ /** token - Valid access token */ token: string, /** uuid - Unique identifier, used for refreshing element visibility when called from the ES6 wrapper */ uuid?: string, + /** features - Feature flags for conditional rendering */ + features?: { [string]: boolean }, };
🧹 Nitpick comments (7)
src/elements/content-sharing/types.ts (1)
4-23: Tighten prop typing to match runtime and defaults.Make apiHost optional since you default it in the component, and ensure messages matches what Internationalize expects. Keep it lean, fool.
-export type ContentSharingV2Props = { - /** apiHost - API hostname. Defaults to https://api.box.com */ - apiHost: string; +export type ContentSharingV2Props = { + /** apiHost - API hostname. Defaults to https://api.box.com */ + apiHost?: string; /** itemID - Box file or folder ID */ itemID: string; @@ - /** messages - Localized strings used by the element */ - messages?: StringMap; + /** messages - Localized strings used by the element (id -> message) */ + messages?: StringMap;src/elements/content-sharing/ContentSharing.js (1)
56-64: Consider removing deadcreateAPIpath when V2 is enabled.You still initialize API/state even if you early‑return V2. It’s harmless but wasted work. Up to you to keep or simplify.
src/elements/content-sharing/stories/tests/ContentSharingV2-visual.stories.tsx (2)
7-21: Args don’t match V2 props; fix theme/features.
featuresisn’t a V2 prop, andthemeexpects a Theme object, not 'light'. Keep it tight.args: { apiHost: DEFAULT_HOSTNAME_API, - features: global.FEATURE_FLAGS, hasProviders: true, language: 'en', itemType: TYPE_FILE, itemID: global.FILE_ID, messages: { en: { contentSharingV2: 'Content Sharing V2', }, }, token: global.TOKEN, - theme: 'light', + // theme: { tokens: {/* optional design tokens */} }, },
24-28: Modernization arg likely no-op here.
enableModernizedComponentsapplies via withBlueprintModernization on ContentSharing, not on V2 directly. Drop it or wire the HOC around V2 if needed.-export const Modernization = { - args: { - enableModernizedComponents: true, - }, -}; +export const Modernization = {};src/elements/content-sharing/ContentSharingV2.tsx (2)
51-53: Selector based on raw itemID can collide.Multiple elements for the same item will share
#${itemID}. Prefer a stable unique id (e.g.,${itemType}-${itemID}or a generated uuid).- <ThemingStyles selector={`#${itemID}`} theme={theme} /> + <ThemingStyles selector={`#cs-${itemType}-${itemID}`} theme={theme} />
41-46: Static mock item name is a placeholder.Call it out or fetch the real name when API hookup lands.
- name: 'Box Development Guide.pdf', + name: 'Item',src/elements/content-sharing/stories/ContentSharingV2.stories.tsx (1)
10-23: Align args with V2 props.Drop
features, and fixthemeshape. Don’t pass what you don’t consume, fool.args: { apiHost: DEFAULT_HOSTNAME_API, - features: global.FEATURE_FLAGS, hasProviders: true, language: 'en', itemType: TYPE_FILE, itemID: global.FILE_ID, messages: { en: { contentSharingV2: 'Content Sharing V2', }, }, token: global.TOKEN, - theme: 'light', + // theme: { tokens: {/* optional design tokens */} }, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
scripts/jest/jest.config.js(1 hunks)src/elements/content-sharing/ContentSharing.js(3 hunks)src/elements/content-sharing/ContentSharingV2.tsx(1 hunks)src/elements/content-sharing/stories/ContentSharing.stories.js(2 hunks)src/elements/content-sharing/stories/ContentSharingV2.stories.tsx(1 hunks)src/elements/content-sharing/stories/tests/ContentSharingV2-visual.stories.tsx(1 hunks)src/elements/content-sharing/types.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/elements/content-sharing/types.ts (1)
src/elements/common/theming/types.ts (1)
Theme(1-3)
src/elements/content-sharing/stories/ContentSharing.stories.js (1)
src/elements/content-preview/ContentPreview.js (1)
global(847-847)
src/elements/content-sharing/ContentSharing.js (2)
src/elements/content-sharing/ContentSharingV2.tsx (1)
ContentSharingV2(58-58)src/elements/content-sharing/messages.js (1)
messages(3-62)
src/elements/content-sharing/ContentSharingV2.tsx (1)
src/elements/content-sharing/types.ts (1)
ContentSharingV2Props(4-23)
🪛 Biome (2.1.2)
src/elements/content-sharing/stories/ContentSharing.stories.js
[error] 35-35: type annotation 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: Analyze (javascript-typescript)
- GitHub Check: Summary
🔇 Additional comments (3)
src/elements/content-sharing/stories/ContentSharing.stories.js (1)
24-26: Adding itemID/token defaults is fine.Story args now line up with the component’s needs. Nicely done.
src/elements/content-sharing/ContentSharing.js (1)
14-17: Feature flag import is good.Using
isFeatureEnabledto gate V2 is the right call.scripts/jest/jest.config.js (1)
29-30: Verify Jest transforms ESM packages in CI/watchRegex includes @box/unified-share-modal, @box/user-selector, and @box/copy-input in transformIgnorePatterns (scripts/jest/jest.config.js lines 29–30):
'node_modules/(?!(@box/react-virtualized/dist/es|@box/cldr-data|@box/blueprint-web|@box/blueprint-web-assets|@box/metadata-editor|@box/box-ai-content-answers|@box/box-ai-agent-selector|@box/item-icon|@box/combobox-with-api|@box/tree|@box/metadata-filter|@box/metadata-view|@box/types|@box/box-item-type-selector|@box/unified-share-modal|@box/user-selector|@box/copy-input)/)', ],rg shows many local imports of unified-share-modal. Run CI or jest/watch to confirm no "Unexpected token export" or other ESM syntax failures — I pity the fool who skips the tests.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/elements/content-sharing/ContentSharing.js (1)
109-111: PasstokenandapiHostto V2 or it won’t authenticate. I pity the fool who ships a starving V2!V2 needs auth and host. Without these, API calls will fail.
- if (isFeatureEnabled(features, 'contentSharingV2')) { - return <ContentSharingV2 itemID={itemID} itemType={itemType} language={language} messages={messages} />; - } + if (isFeatureEnabled(features, 'contentSharingV2')) { + return ( + <ContentSharingV2 + apiHost={apiHost} + itemID={itemID} + itemType={itemType} + language={language} + messages={messages} + token={token} + /> + ); + }
🧹 Nitpick comments (2)
src/elements/content-sharing/ContentSharing.js (2)
68-91: Skip eager API creation when V2 is on; keep hooks stable, sucka.You always instantiate
APIeven when short‑circuiting to V2. Avoid the work and possible side effects; lazily initapionly when V1 path is active.function ContentSharing({ apiHost = DEFAULT_HOSTNAME_API, config, customButton, displayInModal, - features = {}, + features = {}, itemID, itemType, language, messages, token, uuid, }: ContentSharingProps) { - const [api, setAPI] = React.useState<API | null>(createAPI(apiHost, itemID, itemType, token)); + const isV2 = isFeatureEnabled(features, 'contentSharingV2'); + const [api, setAPI] = React.useState<API | null>(() => + isV2 ? null : createAPI(apiHost, itemID, itemType, token), + ); @@ - React.useEffect(() => { - if (apiHost && itemID && itemType && token) { - setAPI(createAPI(apiHost, itemID, itemType, token)); - } - }, [apiHost, itemID, itemType, token]); + React.useEffect(() => { + if (isV2) { + setAPI(null); + return; + } + if (apiHost && itemID && itemType && token) { + setAPI(createAPI(apiHost, itemID, itemType, token)); + } + }, [apiHost, itemID, itemType, token, isV2]);(Optional follow-up: reuse
isV2in the render guard.)
97-107: Trim effect deps forlaunchButton. Don’t rerun for nothin’, fool.Most deps aren’t used. Keep it to what’s needed.
-}, [config, customButton, displayInModal, itemID, itemType, language, launchButton, messages, isVisible]); +}, [customButton, launchButton]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/elements/content-sharing/ContentSharing.js(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/elements/content-sharing/ContentSharing.js (2)
src/elements/content-sharing/ContentSharingV2.tsx (1)
ContentSharingV2(58-58)src/elements/content-sharing/messages.js (1)
messages(3-62)
🪛 Biome (2.1.2)
src/elements/content-sharing/ContentSharing.js
[error] 14-14: 'import { type x ident }' 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: Analyze (javascript-typescript)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (3)
src/elements/content-sharing/ContentSharing.js (3)
43-45:featuresprop addition looks good.Type + doc are clear; defaulting is handled in the parameter list.
73-74: Sane default forfeatures.Empty object fallback keeps
isFeatureEnabledfalsey by default. Rock on.
17-17: No change required — ContentSharingV2 is a named export.
I pity the fool who imports it as default — the file ends withexport { ContentSharingV2 };, so the current named import is correct.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/elements/content-sharing/stories/ContentSharing.stories.js (1)
35-35: I pity the fool who uses TypeScript types in JavaScript! Fix this StoryObj type annotation.StoryObj is a TypeScript-only utility type from Storybook. This here's a .js file with Flow comments, fool! You can't be mixin' TypeScript syntax in JavaScript - it'll break your build faster than you can say "I ain't gettin' on no plane!"
-export const ContentSharingV2Enabled: StoryObj = { +export const ContentSharingV2Enabled = {src/elements/content-sharing/ContentSharing.js (2)
14-14: I pity the fool mixing Flow and TypeScript import syntax!You got the right idea separating the imports, but you're still using TypeScript-style
import typesyntax. In Flow files (which this is, fool!), you need to use Flow'simport typeform.The imports should already be correct as Flow supports
import type- my retrieved learnings confirm this is valid Flow syntax. These static analysis warnings are false positives.Also applies to: 22-22
114-120: You're missing required props for V2, fool!The V2 component needs
apiHostandtokenbut you ain't passing them through! Don't starve the component of what it needs.if (isFeatureEnabled(features, 'contentSharingV2')) { return ( - <ContentSharingV2 itemID={itemID} itemType={itemType} language={language} messages={messages}> + <ContentSharingV2 + apiHost={apiHost} + itemID={itemID} + itemType={itemType} + language={language} + messages={messages} + token={token} + > {children} </ContentSharingV2> ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/elements/content-sharing/ContentSharing.js(5 hunks)src/elements/content-sharing/ContentSharingV2.tsx(1 hunks)src/elements/content-sharing/stories/ContentSharing.stories.js(2 hunks)src/elements/content-sharing/types.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-06-25T13:10:19.128Z
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#4160
File: src/elements/content-sidebar/SidebarToggle.js:11-11
Timestamp: 2025-06-25T13:10:19.128Z
Learning: In the box-ui-elements codebase, Flow files support `import type` syntax for type-only imports. The `import type` statement is valid Flow syntax, not just TypeScript, and should not be flagged as an error when used in Flow files (indicated by flow comments).
Applied to files:
src/elements/content-sharing/ContentSharing.jssrc/elements/content-sharing/stories/ContentSharing.stories.js
📚 Learning: 2025-06-25T13:09:45.168Z
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#4160
File: src/elements/content-sidebar/SidebarToggle.js:13-19
Timestamp: 2025-06-25T13:09:45.168Z
Learning: Files with `flow` or `flow strict` comments at the top use Flow type syntax, not TypeScript. Flow type definitions like `type Props = { ... }` and type imports like `type { RouterHistory }` are valid Flow syntax and should not be flagged as TypeScript-only features.
Applied to files:
src/elements/content-sharing/ContentSharing.jssrc/elements/content-sharing/stories/ContentSharing.stories.js
📚 Learning: 2025-06-24T18:35:01.363Z
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#4156
File: src/elements/content-sidebar/SidebarNavButton.js:14-14
Timestamp: 2025-06-24T18:35:01.363Z
Learning: The `import type` syntax is valid in Flow, not just TypeScript. Flow supports `import type { Type } from 'module'` for importing types, even in .js files with flow pragma.
Applied to files:
src/elements/content-sharing/ContentSharing.js
📚 Learning: 2025-06-25T13:09:54.538Z
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#4160
File: src/elements/content-sidebar/SidebarToggle.js:21-27
Timestamp: 2025-06-25T13:09:54.538Z
Learning: The box-ui-elements project uses Flow for type annotations in JavaScript files, as indicated by flow directives in file headers. Type annotations like `: Props` are valid Flow syntax, not TypeScript syntax.
Applied to files:
src/elements/content-sharing/stories/ContentSharing.stories.js
📚 Learning: 2025-06-17T13:30:02.172Z
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#4144
File: src/elements/content-sidebar/versions/VersionsList.js:24-33
Timestamp: 2025-06-17T13:30:02.172Z
Learning: In the box-ui-elements codebase, Flow components use .flow.js type definition files, not TypeScript .ts files. The InternalSidebarNavigation type is a union type where different variants may have different properties like versionId, and proper type safety is ensured through conditional checks in methods like getSelectedVersionId.
Applied to files:
src/elements/content-sharing/stories/ContentSharing.stories.js
🧬 Code graph analysis (1)
src/elements/content-sharing/ContentSharingV2.tsx (1)
src/elements/content-sharing/types.ts (1)
ContentSharingV2Props(3-22)
🪛 Biome (2.1.2)
src/elements/content-sharing/ContentSharing.js
[error] 19-19: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 21-21: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 22-22: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
src/elements/content-sharing/stories/ContentSharing.stories.js
[error] 35-35: type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (6)
src/elements/content-sharing/stories/ContentSharing.stories.js (2)
24-25: Good addition, fool! Adding global identifiers for consistency.I like how you added
itemID: global.FILE_IDandtoken: global.TOKENto the default story args. That's how you keep your stories consistent and properly configured!
35-43: Story structure looks solid after you fix that type annotation.The ContentSharingV2Enabled story properly enables the feature flag and provides the right children prop for the unified share modal trigger. Nice work setting up that feature gate test!
src/elements/content-sharing/types.ts (1)
1-22: Solid type definition for ContentSharingV2, fool!This ContentSharingV2Props type is well-structured with proper documentation for each property. I like how you included both required props (apiHost, itemID, itemType, language, token) and optional ones (children, messages, hasProviders, uuid). The type-only imports are clean too.
src/elements/content-sharing/ContentSharingV2.tsx (2)
18-23: Mock item is fine for now, but watch that comment, fool!The mock item structure is appropriate for the current implementation. The comment "Retrieve item from API later" tells us this is temporary, which is fine for a feature-flagged V2 rollout.
25-31: Clean component structure with proper providers.I like how you wrapped everything in Internationalize and Providers - that's the right way to handle localization and context. The UnifiedShareModal gets the mock item and children properly passed through.
src/elements/content-sharing/ContentSharing.js (1)
31-32: Nice prop additions for the V2 feature gate.Adding the
childrenandfeaturesprops to support the ContentSharingV2 integration is spot on. The optional children for the unified share modal trigger and feature config with sensible defaults - that's how you do it right!Also applies to: 47-48
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/elements/content-sharing/types.ts (3)
5-5: Broaden children to ReactNode for flexibility, fool.Accept text, fragments, arrays, etc. without surprises.
-import type { ReactElement } from 'react'; +import type { ReactNode } from 'react'; - children?: ReactElement; + children?: ReactNode;Confirm no callers rely on rejecting non-element children.
13-13: Make messages immutable like a champ.Avoid accidental mutation of i18n bundles.
- messages?: StringMap; + messages?: Readonly<StringMap>;
3-14: Interface over type for props, if you plan to extend.Interfaces merge and play nicer with augmentation.
-export type ContentSharingV2Props = { +export interface ContentSharingV2Props { /** children - Children for the element to open the Unified Share Modal */ ... -}; +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/elements/content-sharing/ContentSharingV2.tsx(1 hunks)src/elements/content-sharing/types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- 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). (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (2)
src/elements/content-sharing/types.ts (2)
3-14: Solid, lean prop contract. I like your style.Clear, minimal surface for V2. Ship it once the nits above are addressed.
11-11: Keeplanguage— repo useslanguage, notlocale.Found at src/elements/content-sharing/types.ts:11; no
localeoccurrences in src/elements. I pity the fool who renames it.Likely an incorrect or invalid review comment.
src/elements/content-sharing/stories/ContentSharingV2.stories.tsx
Outdated
Show resolved
Hide resolved
src/elements/content-sharing/stories/tests/ContentSharingV2-visual.stories.tsx
Outdated
Show resolved
Hide resolved
src/elements/content-sharing/stories/tests/ContentSharingV2-visual.stories.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/elements/content-sharing/ContentSharing.js (1)
114-120: Pass auth/host to V2 or confirm it’s not needed yet.Right now V2 gets no
token/apiHost. If USM will fetch, wire them through; if not, add a TODO. This was flagged before and seems back.Option A (wire through once V2 accepts them):
- if (isFeatureEnabled(features, 'contentSharingV2')) { - return ( - <ContentSharingV2 itemID={itemID} itemType={itemType} language={language} messages={messages}> - {children} - </ContentSharingV2> - ); - } + if (isFeatureEnabled(features, 'contentSharingV2')) { + return ( + <ContentSharingV2 + apiHost={apiHost} + itemID={itemID} + itemType={itemType} + language={language} + messages={messages} + token={token} + > + {children} + </ContentSharingV2> + ); + }If V2 intentionally stays UI‑only for now, drop this once backend is hooked.
src/elements/content-sharing/stories/tests/ContentSharingV2-visual.stories.tsx (1)
7-12: Drop unknown props and providelanguage, fool.
featuresisn’t a prop on ContentSharingV2, andlanguageis needed (or defaulted). Clean up the args.export default { title: 'Elements/ContentSharingV2/tests/visual-regression-tests', component: ContentSharingV2, args: { - features: global.FEATURE_FLAGS, itemType: TYPE_FILE, itemID: global.FILE_ID, + language: 'en-US', }, };
🧹 Nitpick comments (3)
src/elements/content-sharing/ContentSharingV2.tsx (1)
25-29: Use an itemType-appropriate mock name.Folders named “*.pdf” look goofy, sucka. Tiny tweak keeps demos credible.
- const mockItem = { + const mockItem = { id: itemID, - name: 'Box Development Guide.pdf', + name: itemType === 'folder' ? 'My Folder' : 'Box Development Guide.pdf', type: itemType, };src/elements/content-sharing/stories/tests/ContentSharingV2-visual.stories.tsx (1)
14-18: Remove the Modernization story here.
enableModernizedComponentsapplies to the ContentSharing wrapper HOC, not this V2 component. It’s dead weight in this story.-export const Modernization = { - args: { - enableModernizedComponents: true, - }, -}; +// Modernization toggle lives on ContentSharing (HOC), not ContentSharingV2. Story removed.src/elements/content-sharing/ContentSharing.js (1)
63-71: Optional: add Flow types tocreateAPI.Minor polish for consistency/readability.
-const createAPI = (apiHost, itemID, itemType, token) => +const createAPI = (apiHost: string, itemID: string, itemType: ItemType, token: string) => new API({ apiHost, clientName: CLIENT_NAME_CONTENT_SHARING, id: `${itemType}_${itemID}`, token, version: CLIENT_VERSION, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/elements/content-sharing/ContentSharing.js(3 hunks)src/elements/content-sharing/ContentSharingV2.tsx(1 hunks)src/elements/content-sharing/stories/ContentSharing.stories.js(2 hunks)src/elements/content-sharing/stories/ContentSharingV2.stories.tsx(1 hunks)src/elements/content-sharing/stories/tests/ContentSharingV2-visual.stories.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/elements/content-sharing/stories/ContentSharing.stories.js
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-06-25T13:10:19.128Z
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#4160
File: src/elements/content-sidebar/SidebarToggle.js:11-11
Timestamp: 2025-06-25T13:10:19.128Z
Learning: In the box-ui-elements codebase, Flow files support `import type` syntax for type-only imports. The `import type` statement is valid Flow syntax, not just TypeScript, and should not be flagged as an error when used in Flow files (indicated by flow comments).
Applied to files:
src/elements/content-sharing/ContentSharing.js
📚 Learning: 2025-06-25T13:09:45.168Z
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#4160
File: src/elements/content-sidebar/SidebarToggle.js:13-19
Timestamp: 2025-06-25T13:09:45.168Z
Learning: Files with `flow` or `flow strict` comments at the top use Flow type syntax, not TypeScript. Flow type definitions like `type Props = { ... }` and type imports like `type { RouterHistory }` are valid Flow syntax and should not be flagged as TypeScript-only features.
Applied to files:
src/elements/content-sharing/ContentSharing.js
📚 Learning: 2025-06-24T18:35:01.363Z
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#4156
File: src/elements/content-sidebar/SidebarNavButton.js:14-14
Timestamp: 2025-06-24T18:35:01.363Z
Learning: The `import type` syntax is valid in Flow, not just TypeScript. Flow supports `import type { Type } from 'module'` for importing types, even in .js files with flow pragma.
Applied to files:
src/elements/content-sharing/ContentSharing.js
🧬 Code graph analysis (2)
src/elements/content-sharing/stories/tests/ContentSharingV2-visual.stories.tsx (2)
src/elements/content-sharing/ContentSharingV2.tsx (1)
ContentSharingV2(40-40)src/elements/content-preview/ContentPreview.js (1)
global(847-847)
src/elements/content-sharing/ContentSharing.js (1)
src/elements/content-sharing/ContentSharingV2.tsx (1)
ContentSharingV2(40-40)
🪛 Biome (2.1.2)
src/elements/content-sharing/ContentSharing.js
[error] 20-20: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 21-21: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 22-22: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 27-61: interface are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (3)
src/elements/content-sharing/ContentSharingV2.tsx (1)
31-36: LGTM on the wrapper stack.Internationalize + Providers around USM is clean and future‑proof.
src/elements/content-sharing/stories/ContentSharingV2.stories.tsx (1)
5-5: Keep the Basic story — good smoke check.Solid minimal entry point.
src/elements/content-sharing/ContentSharing.js (1)
72-85: Gating placement is clean.Early return keeps legacy flow untouched. Nice and safe, fool.
src/elements/content-sharing/stories/tests/ContentSharingV2-visual.stories.tsx
Outdated
Show resolved
Hide resolved
tjuanitas
changed the title
reneshen0328
force-pushed
the
feat-create-content-sharing-v2
branch
from
28764ae to
fa7cf77
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/elements/content-sharing/ContentSharing.js (1)
117-129: Resolved: no token/apiHost passed to V2 (by design).Previous feedback asked to pass token/apiHost. V2 doesn’t need them now; keeping the surface minimal is right. I pity the fool who over-plumbs.
🧹 Nitpick comments (3)
src/elements/content-sharing/ContentSharingV2.tsx (2)
10-23: Make props friendlier: default language, optional hasProviders, broader children.I pity the fool who makes callers over-specify! Set a sane default for language, default hasProviders to true, and accept ReactNode to allow strings/fragments.
-export interface ContentSharingV2Props { +export interface ContentSharingV2Props { @@ - children?: React.ReactElement; + children?: React.ReactNode; @@ - /** hasProviders - Whether the element has providers for USM already */ - hasProviders: boolean; + /** hasProviders - Whether the element has providers for USM already (defaults to true) */ + hasProviders?: boolean; @@ - /** language - Language used for the element */ - language?: string; + /** language - Language used for the element (defaults to 'en-US') */ + language?: string; } -function ContentSharingV2({ children, itemID, itemType, hasProviders, language, messages }: ContentSharingV2Props) { +function ContentSharingV2({ + children, + itemID, + itemType, + hasProviders = true, + language = 'en-US', + messages, +}: ContentSharingV2Props) {
36-36: Children passthrough: ensure USM uses it.UnifiedShareModal may ignore children. If children are intended as a trigger, consider owning the trigger here or documenting behavior. I pity the fool who passes dead children.
src/elements/content-sharing/ContentSharing.js (1)
117-129: Nice flag gate; now skip wasted work when V2 is on.You still build/refresh API and clone the customButton even when returning V2. I pity the fool wasting cycles—gate those by feature flag.
-function ContentSharing({ +function ContentSharing({ apiHost = DEFAULT_HOSTNAME_API, children, config, customButton, displayInModal, features = {}, hasProviders = true, itemID, itemType, language, messages, token, uuid, }: ContentSharingProps) { - const [api, setAPI] = React.useState<API | null>(createAPI(apiHost, itemID, itemType, token)); + const useV2 = isFeatureEnabled(features, 'contentSharingV2'); + const [api, setAPI] = React.useState<API | null>(() => (useV2 ? null : createAPI(apiHost, itemID, itemType, token))); const [launchButton, setLaunchButton] = React.useState<React.Element<any> | null>(null); const [isVisible, setIsVisible] = React.useState<boolean>(!customButton); // Reset the API if necessary - React.useEffect(() => { - if (apiHost && itemID && itemType && token) { - setAPI(createAPI(apiHost, itemID, itemType, token)); - } - }, [apiHost, itemID, itemType, token]); + React.useEffect(() => { + if (!useV2 && apiHost && itemID && itemType && token) { + setAPI(createAPI(apiHost, itemID, itemType, token)); + } + }, [useV2, apiHost, itemID, itemType, token]); // Reset state if the API has changed React.useEffect(() => { setIsVisible(!customButton); }, [api, customButton, uuid]); - React.useEffect(() => { - if (customButton && !launchButton) { + React.useEffect(() => { + if (useV2) return; + if (customButton && !launchButton) { setLaunchButton( React.cloneElement(customButton, { onClick: () => { return setIsVisible(true); }, }), ); } - }, [config, customButton, displayInModal, itemID, itemType, language, launchButton, messages, isVisible]); + }, [useV2, config, customButton, displayInModal, itemID, itemType, language, launchButton, messages, isVisible]); - if (isFeatureEnabled(features, 'contentSharingV2')) { + if (useV2) { return ( <ContentSharingV2 itemID={itemID} itemType={itemType} hasProviders={hasProviders} language={language} messages={messages} > {children} </ContentSharingV2> ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/elements/content-sharing/ContentSharing.js(5 hunks)src/elements/content-sharing/ContentSharingV2.tsx(1 hunks)src/elements/content-sharing/stories/ContentSharing.stories.js(2 hunks)src/elements/content-sharing/stories/ContentSharingV2.stories.tsx(1 hunks)src/elements/content-sharing/stories/tests/ContentSharingV2-visual.stories.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/elements/content-sharing/stories/ContentSharing.stories.js
- src/elements/content-sharing/stories/ContentSharingV2.stories.tsx
- src/elements/content-sharing/stories/tests/ContentSharingV2-visual.stories.tsx
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-06-25T13:10:19.128Z
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#4160
File: src/elements/content-sidebar/SidebarToggle.js:11-11
Timestamp: 2025-06-25T13:10:19.128Z
Learning: In the box-ui-elements codebase, Flow files support `import type` syntax for type-only imports. The `import type` statement is valid Flow syntax, not just TypeScript, and should not be flagged as an error when used in Flow files (indicated by flow comments).
Applied to files:
src/elements/content-sharing/ContentSharing.js
📚 Learning: 2025-06-25T13:09:45.168Z
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#4160
File: src/elements/content-sidebar/SidebarToggle.js:13-19
Timestamp: 2025-06-25T13:09:45.168Z
Learning: Files with `flow` or `flow strict` comments at the top use Flow type syntax, not TypeScript. Flow type definitions like `type Props = { ... }` and type imports like `type { RouterHistory }` are valid Flow syntax and should not be flagged as TypeScript-only features.
Applied to files:
src/elements/content-sharing/ContentSharing.js
📚 Learning: 2025-06-24T18:35:01.363Z
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#4156
File: src/elements/content-sidebar/SidebarNavButton.js:14-14
Timestamp: 2025-06-24T18:35:01.363Z
Learning: The `import type` syntax is valid in Flow, not just TypeScript. Flow supports `import type { Type } from 'module'` for importing types, even in .js files with flow pragma.
Applied to files:
src/elements/content-sharing/ContentSharing.js
🪛 Biome (2.1.2)
src/elements/content-sharing/ContentSharing.js
[error] 20-20: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 21-21: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 22-22: '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: Analyze (javascript-typescript)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (5)
src/elements/content-sharing/ContentSharingV2.tsx (2)
26-31: Confirm USM item shape is sufficient.Mock item only sets id/name/type. I pity the fool who ships missing fields if USM expects more (e.g., permissions, shared_link). Verify USM renders without additional item metadata.
33-39: Provider stacking: avoid double-wrapping i18n/providers if upstream already supplies them.If Providers also injects i18n or the same contexts, you may be double-wrapping under Internationalize. I pity the fool causing context confusion—verify there’s no duplicate providers when hasProviders is true.
src/elements/content-sharing/ContentSharing.js (3)
31-33: Prop additions look solid.children is optional and docs are clear. I pity the fool who argues with that.
47-50: Good defaults on features/hasProviders.Sane defaults reduce caller churn. Approved, sucka.
20-22: Ignore Biome false positives on Flow type imports.This file is Flow (
@flow).import typeis valid Flow. I pity the fool who removes types—tune or exclude Biome for Flow files instead.
src/elements/content-sharing/stories/ContentSharingV2.stories.tsx
Outdated
Show resolved
Hide resolved
What
Add skeleton code in Content Sharing to support the new USM shared feature.
Before & After
Feature flip enabled
Feature flip disabled
Summary by CodeRabbit
New Features
Documentation / Stories
Tests / Chores