fix(blueprint): Blueprint Modernization #4204
Conversation
WalkthroughI pity the fool — Adds a new HOC Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as App
participant HOC as withBlueprintModernization
participant Ctx as BlueprintModernizationContext
participant Prov as BlueprintModernizationProvider
participant Comp as WrappedComponent
App->>HOC: Render(WrappedComponent, props)
HOC->>Ctx: useContext()
alt No upstream context or upstream not enabled
HOC->>Prov: Render Provider (enableModernizedComponents: props.enableModernizedComponents)
Prov->>Comp: Render(props)
else Upstream context provided and enabled
HOC->>Comp: Render(props)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
greg-in-a-box
force-pushed
the
bp-mod
branch
4 times, most recently
from
7261796 to
fb52e4e
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
🧹 Nitpick comments (1)
src/elements/common/withBlueprintModernization.tsx (1)
14-24: Consider more robust context checking.The conditional logic is sound, but consider checking for both context existence and the flag value more explicitly for clarity and robustness.
Consider this approach for more explicit context checking:
- // no context found from the parent component, so we need to provide our own - if (!upstream.enableModernizedComponents) { + // no context found from the parent component, so we need to provide our own + if (!upstream || !upstream.enableModernizedComponents) {This makes it clearer that we're checking for both the existence of context and the flag value.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/elements/common/__tests__/withBlueprintModernization.test.tsx(1 hunks)src/elements/common/withBlueprintModernization.tsx(1 hunks)src/elements/content-explorer/ContentExplorer.tsx(2 hunks)src/elements/content-open-with/ContentOpenWith.js(2 hunks)src/elements/content-picker/ContentPicker.js(3 hunks)src/elements/content-preview/ContentPreview.js(2 hunks)src/elements/content-sharing/ContentSharing.js(2 hunks)src/elements/content-sidebar/ContentSidebar.js(2 hunks)src/elements/content-uploader/ContentUploader.tsx(3 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: jpan-box
PR: box/box-ui-elements#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.
src/elements/content-sidebar/ContentSidebar.js (3)
Learnt from: rafalmaksymiuk
PR: #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.
Learnt from: rafalmaksymiuk
PR: #4136
File: src/elements/common/types/SidebarNavigation.ts:16-26
Timestamp: 2025-06-11T16:30:10.431Z
Learning: VersionSidebarView intentionally uses the versionId field to stay consistent with current URL parameter naming; a potential rename to fileVersionId is deferred until React Router is removed.
Learnt from: rafalmaksymiuk
PR: #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.
src/elements/content-picker/ContentPicker.js (8)
Learnt from: rafalmaksymiuk
PR: #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.
Learnt from: rafalmaksymiuk
PR: #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).
Learnt from: rafalmaksymiuk
PR: #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.
Learnt from: rafalmaksymiuk
PR: #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.
Learnt from: rafalmaksymiuk
PR: #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.
Learnt from: tjuanitas
PR: #4126
File: .storybook/reactIntl.ts:4-7
Timestamp: 2025-06-17T15:39:16.739Z
Learning: In box-ui-elements, the team prefers using the spread operator in reduce functions to maintain immutability rather than mutating objects, even if it has a minor performance cost. This reflects their preference for functional programming principles and code safety over micro-optimizations.
Learnt from: rafalmaksymiuk
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.
Learnt from: jpan-box
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.
src/elements/content-uploader/ContentUploader.tsx (7)
Learnt from: rafalmaksymiuk
PR: #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.
Learnt from: rafalmaksymiuk
PR: #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).
Learnt from: rafalmaksymiuk
PR: #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.
Learnt from: rafalmaksymiuk
PR: #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.
Learnt from: jpan-box
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.
Learnt from: tjuanitas
PR: #4126
File: .storybook/reactIntl.ts:4-7
Timestamp: 2025-06-17T15:39:16.739Z
Learning: In box-ui-elements, the team prefers using the spread operator in reduce functions to maintain immutability rather than mutating objects, even if it has a minor performance cost. This reflects their preference for functional programming principles and code safety over micro-optimizations.
Learnt from: rafalmaksymiuk
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.
src/elements/content-explorer/ContentExplorer.tsx (1)
Learnt from: rafalmaksymiuk
PR: #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.
src/elements/common/__tests__/withBlueprintModernization.test.tsx (1)
Learnt from: jpan-box
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.
src/elements/content-open-with/ContentOpenWith.js (5)
Learnt from: rafalmaksymiuk
PR: #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.
Learnt from: rafalmaksymiuk
PR: #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.
Learnt from: rafalmaksymiuk
PR: #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.
Learnt from: rafalmaksymiuk
PR: #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).
Learnt from: jpan-box
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.
src/elements/common/withBlueprintModernization.tsx (1)
Learnt from: jpan-box
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.
🧬 Code Graph Analysis (2)
src/elements/content-sidebar/ContentSidebar.js (1)
src/elements/common/withBlueprintModernization.tsx (1)
withBlueprintModernization(8-25)
src/elements/content-preview/ContentPreview.js (1)
src/elements/common/withBlueprintModernization.tsx (1)
withBlueprintModernization(8-25)
⏰ 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 (23)
src/elements/common/withBlueprintModernization.tsx (2)
1-10: LGTM! Clean imports and proper TypeScript typing.The imports are well-organized and the generic type parameter
Pwith proper constraints provides good type safety for the HOC.
11-25: Well-structured HOC implementation.The HOC follows React best practices with proper component naming, props spreading, and clean JSX structure. The conditional rendering logic effectively handles both scenarios.
src/elements/content-sharing/ContentSharing.js (2)
12-12: LGTM! Correct import path and style.The import of
withBlueprintModernizationfollows the established patterns in the file and uses the correct relative path.
122-123: Excellent export pattern for HOC integration.The dual export approach (wrapped default export and unwrapped named export) provides flexibility for testing and direct component usage while making the HOC enhancement the default behavior.
src/elements/content-uploader/ContentUploader.tsx (2)
5-5: LGTM! Clean import additions.The
flowutility from lodash andwithBlueprintModernizationimports are properly placed and follow the file's existing import conventions.Also applies to: 18-18
1328-1328: Perfect HOC composition with flow.The use of
flowto composemakeResponsiveandwithBlueprintModernizationis clean and follows functional programming best practices. The order of composition is logical.src/elements/content-picker/ContentPicker.js (2)
12-12: LGTM! Well-organized imports.The
flowandwithBlueprintModernizationimports are logically placed and consistent with the file's Flow-based import patterns.Also applies to: 22-22
1350-1350: Consistent HOC composition implementation.The flow composition matches the pattern used in ContentUploader and provides clean multi-HOC wrapping. The existing named export on line 1349 already provides access to the unwrapped component.
src/elements/content-preview/ContentPreview.js (2)
38-38: LGTM! Import added correctly.The import of
withBlueprintModernizationis properly added and follows the existing import pattern in the file.
1422-1422: LGTM! HOC integration is consistent.The
withBlueprintModernizationHOC is correctly added to the flow composition in the appropriate position betweenwithFeatureProviderandwithLogger. This maintains the existing wrapper hierarchy while adding blueprint modernization context support.src/elements/content-sidebar/ContentSidebar.js (2)
24-24: LGTM! Import added correctly.The import of
withBlueprintModernizationis properly added and follows the existing import pattern in the file.
436-436: LGTM! HOC integration is consistent.The
withBlueprintModernizationHOC is correctly added to the flow composition betweenwithFeatureProviderandwithLogger, maintaining consistency with the other components in this PR.src/elements/content-open-with/ContentOpenWith.js (3)
11-11: LGTM! Flow import added for HOC composition.The import of
flowfrom lodash is correctly added to support the new HOC composition pattern in the export.
17-17: LGTM! Import added correctly.The import of
withBlueprintModernizationis properly added and follows the existing import pattern in the file.
592-594: LGTM! Export refactored to use flow composition.The export is correctly refactored from a single HOC wrapper to a flow composition that includes both
withErrorBoundaryandwithBlueprintModernization. This maintains the existing error boundary functionality while adding blueprint modernization context support, consistent with the pattern applied across other components.src/elements/content-explorer/ContentExplorer.tsx (2)
98-98: LGTM - Clean import of the new HOCThe import statement correctly brings in the
withBlueprintModernizationHOC from the expected location.
1863-1865: LGTM - Proper HOC composition integrationThe
withBlueprintModernizationHOC is correctly added as the last wrapper in theflowcomposition pipeline. This ensures that the blueprint modernization context is available to the component while maintaining the existing HOC layering order.src/elements/common/__tests__/withBlueprintModernization.test.tsx (6)
1-26: Comprehensive mocking setup with good test isolationThe mocking approach correctly isolates the Blueprint Web package dependencies and provides controlled test doubles. The mock implementations maintain the essential behavior needed for testing while avoiding external dependencies.
28-40: Clean test component setup with proper typingThe test component and wrapper setup provides a clean foundation for testing HOC behavior. The TypeScript types correctly model the expected props interface.
42-49: Thorough validation of default HOC behaviorThis test correctly verifies that the HOC wraps components with the BlueprintModernizationProvider by default and properly handles undefined
enableModernizedComponentsvalues.
51-56: Proper prop forwarding validationThis test ensures that the HOC correctly passes through props to the wrapped component without interference.
58-68: Correct handling of enableModernizedComponents propThis test validates that the
enableModernizedComponentsprop is properly forwarded to the BlueprintModernizationProvider with the correct boolean value.
70-91: Excellent test for upstream context detectionThis test case is particularly valuable as it verifies the HOC's intelligent behavior - when an upstream BlueprintModernizationProvider already exists, the HOC should not add a redundant provider wrapper. This prevents context nesting issues and ensures efficient context usage.
greg-in-a-box
force-pushed
the
bp-mod
branch
2 times, most recently
from
603a40b to
de0bfe7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/elements/common/withBlueprintModernization.tsx (1)
8-25: Fix HOC typing, avoid prop leakage, add null-safety and hoist statics (single refactor).The current signature forces wrapped components to accept BlueprintModernizationContextValue and returns a component whose declared props don’t match the implementation. Also,
upstream.enableModernizedComponentsisn’t null-safe, andenableModernizedComponentsis being passed through to the wrapped component unnecessarily. Refactor as below:@@ -import React, { useContext } from 'react'; +import React, { useContext } from 'react'; +import hoistNonReactStatics from 'hoist-non-react-statics'; @@ -export function withBlueprintModernization<P>( - Component: React.ComponentType<P & BlueprintModernizationContextValue>, -): React.FC<(P & BlueprintModernizationContextValue) | P> { - return function WithBlueprintModernization(props: P & BlueprintModernizationContextValue) { - const upstream = useContext(BlueprintModernizationContext); - - // no context found from the parent component, so we need to provide our own - if (!upstream.enableModernizedComponents) { - return ( - <BlueprintModernizationProvider enableModernizedComponents={!!props.enableModernizedComponents}> - <Component {...props} /> - </BlueprintModernizationProvider> - ); - } - // context found, so load the component with the existing context - return <Component {...props} />; - }; -} +export function withBlueprintModernization<P>( + Component: React.ComponentType<P>, +): React.FC<P & Partial<BlueprintModernizationContextValue>> { + function WithBlueprintModernization(props: P & Partial<BlueprintModernizationContextValue>) { + const upstream = useContext(BlueprintModernizationContext); + const { enableModernizedComponents, ...rest } = props as Partial<BlueprintModernizationContextValue> & P; + + // No context or context disabled → provide our own + if (!upstream?.enableModernizedComponents) { + return ( + <BlueprintModernizationProvider enableModernizedComponents={!!enableModernizedComponents}> + <Component {...(rest as P)} /> + </BlueprintModernizationProvider> + ); + } + // Context enabled → render directly + return <Component {...(rest as P)} />; + } + WithBlueprintModernization.displayName = `withBlueprintModernization(${(Component as any).displayName || (Component as any).name || 'Component'})`; + hoistNonReactStatics(WithBlueprintModernization, Component as any); + return WithBlueprintModernization; +}
🧹 Nitpick comments (1)
src/elements/content-picker/ContentPicker.js (1)
22-23: Reduce FlowFixMe usage by adding a libdef stub.Consider a minimal Flow libdef for the TS module to avoid blanket
$FlowFixMeon import. Optional, but improves type hygiene.Example (flow-typed stub):
// flow-typed/npm/common__withBlueprintModernization.js declare module '../common/withBlueprintModernization' { declare export function withBlueprintModernization<P>( Component: React$ComponentType<P> ): React$ComponentType<P>; // or a more precise HOC type as needed }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
src/elements/common/__tests__/withBlueprintModernization.test.tsx(1 hunks)src/elements/common/withBlueprintModernization.tsx(1 hunks)src/elements/content-explorer/ContentExplorer.tsx(2 hunks)src/elements/content-open-with/ContentOpenWith.js(2 hunks)src/elements/content-picker/ContentPicker.js(3 hunks)src/elements/content-preview/ContentPreview.js(2 hunks)src/elements/content-sharing/ContentSharing.js(2 hunks)src/elements/content-sidebar/ContentSidebar.js(2 hunks)src/elements/content-uploader/ContentUploader.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- src/elements/content-explorer/ContentExplorer.tsx
- src/elements/content-sharing/ContentSharing.js
- src/elements/content-sidebar/ContentSidebar.js
- src/elements/content-uploader/ContentUploader.tsx
- src/elements/common/tests/withBlueprintModernization.test.tsx
- src/elements/content-preview/ContentPreview.js
- src/elements/content-open-with/ContentOpenWith.js
🧰 Additional context used
🧠 Learnings (2)
📚 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-picker/ContentPicker.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-picker/ContentPicker.js
⏰ 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 (2)
src/elements/content-picker/ContentPicker.js (2)
12-12: LGTM: Using lodash/flow for HOC composition is appropriate.
1351-1351: Composition order verified—provider is consistently outermost across the repo.A global search confirms that
withBlueprintModernizationis always the last wrapper in everyflow([...])and the sole wrapper in direct exports:
- src/elements/content-uploader/ContentUploader.tsx
- src/elements/content-picker/ContentPicker.js
- src/elements/content-open-with/ContentOpenWith.js
- src/elements/content-explorer/ContentExplorer.tsx
- src/elements/content-sharing/ContentSharing.js
No instances were found where
withBlueprintModernizationprecedes another HOC. Composition order is consistent and correct—changes are approved.
greg-in-a-box
force-pushed
the
bp-mod
branch
2 times, most recently
from
5cc89ac to
06fbc0b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/elements/content-sidebar/stories/tests/ContentSidebar-visual.stories.tsx (1)
26-30: Type the story for stronger TS safetyOptional, but typing helps catch arg/name issues in CI.
-export const Modernization = { +export const Modernization: StoryObj<typeof ContentSidebar> = { args: { enableModernizedComponents: true, }, };src/elements/content-preview/stories/tests/ContentPreview-visual.stories.js (1)
101-101: Remove unnecessary await or use findAllByTestId to avoid timing flakinessgetAllByTestId is synchronous; awaiting it is a no-op. Prefer findAllByTestId to wait for async rendering.
- const citations = await modal.getAllByTestId('content-answers-citation-status'); + const citations = await modal.findAllByTestId('content-answers-citation-status');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
src/elements/common/__tests__/withBlueprintModernization.test.tsx(1 hunks)src/elements/common/withBlueprintModernization.tsx(1 hunks)src/elements/content-explorer/ContentExplorer.tsx(2 hunks)src/elements/content-explorer/stories/tests/ContentExplorer-visual.stories.js(1 hunks)src/elements/content-open-with/ContentOpenWith.js(2 hunks)src/elements/content-picker/ContentPicker.js(3 hunks)src/elements/content-picker/stories/tests/ContentPicker-visual.stories.js(1 hunks)src/elements/content-preview/ContentPreview.js(2 hunks)src/elements/content-preview/stories/tests/ContentPreview-visual.stories.js(2 hunks)src/elements/content-sharing/ContentSharing.js(2 hunks)src/elements/content-sidebar/ContentSidebar.js(2 hunks)src/elements/content-sidebar/stories/tests/ContentSidebar-visual.stories.tsx(1 hunks)src/elements/content-uploader/ContentUploader.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- src/elements/content-uploader/ContentUploader.tsx
- src/elements/common/withBlueprintModernization.tsx
- src/elements/content-sharing/ContentSharing.js
- src/elements/content-sidebar/ContentSidebar.js
- src/elements/content-picker/ContentPicker.js
- src/elements/content-explorer/ContentExplorer.tsx
- src/elements/content-preview/ContentPreview.js
- src/elements/common/tests/withBlueprintModernization.test.tsx
- src/elements/content-open-with/ContentOpenWith.js
🧰 Additional context used
🧬 Code graph analysis (4)
src/elements/content-sidebar/stories/tests/ContentSidebar-visual.stories.tsx (3)
src/elements/content-explorer/stories/tests/ContentExplorer-visual.stories.js (2)
Modernization(31-35)Modernization(31-35)src/elements/content-picker/stories/tests/ContentPicker-visual.stories.js (2)
Modernization(11-15)Modernization(11-15)src/elements/content-preview/stories/tests/ContentPreview-visual.stories.js (2)
Modernization(33-37)Modernization(33-37)
src/elements/content-picker/stories/tests/ContentPicker-visual.stories.js (3)
src/elements/content-explorer/stories/tests/ContentExplorer-visual.stories.js (2)
Modernization(31-35)Modernization(31-35)src/elements/content-preview/stories/tests/ContentPreview-visual.stories.js (2)
Modernization(33-37)Modernization(33-37)src/elements/content-sidebar/stories/tests/ContentSidebar-visual.stories.tsx (1)
Modernization(26-30)
src/elements/content-explorer/stories/tests/ContentExplorer-visual.stories.js (3)
src/elements/content-picker/stories/tests/ContentPicker-visual.stories.js (2)
Modernization(11-15)Modernization(11-15)src/elements/content-preview/stories/tests/ContentPreview-visual.stories.js (2)
Modernization(33-37)Modernization(33-37)src/elements/content-sidebar/stories/tests/ContentSidebar-visual.stories.tsx (1)
Modernization(26-30)
src/elements/content-preview/stories/tests/ContentPreview-visual.stories.js (3)
src/elements/content-explorer/stories/tests/ContentExplorer-visual.stories.js (2)
Modernization(31-35)Modernization(31-35)src/elements/content-picker/stories/tests/ContentPicker-visual.stories.js (2)
Modernization(11-15)Modernization(11-15)src/elements/content-sidebar/stories/tests/ContentSidebar-visual.stories.tsx (1)
Modernization(26-30)
⏰ 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 (3)
src/elements/content-picker/stories/tests/ContentPicker-visual.stories.js (1)
11-15: Modernization story added — looks good and consistentMatches the pattern used across related stories; no issues.
src/elements/content-explorer/stories/tests/ContentExplorer-visual.stories.js (1)
31-35: Modernization variant aligns with the repo-wide patternConsistent args and placement; all good.
src/elements/content-preview/stories/tests/ContentPreview-visual.stories.js (1)
33-37: Modernization story — LGTMConsistent with other components; no concerns.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/elements/content-explorer/ContentExplorer.tsx (1)
1978-1980: Confirm intended HOC order (withBlueprintModernization as outermost)In lodash.flow, the last entry wraps outermost. This ensures Blueprint context encloses all downstream HOCs and children. If that’s the intent, add a brief comment to prevent future reorder regressions.
Apply this diff to document the ordering:
-export default flow([makeResponsive, withFeatureConsumer, withFeatureProvider, withBlueprintModernization])( - ContentExplorer, -); +// NOTE: Keep withBlueprintModernization last so it is the outermost HOC and provides context to all children. +export default flow([makeResponsive, withFeatureConsumer, withFeatureProvider, withBlueprintModernization])( + ContentExplorer, +);src/elements/common/__tests__/withBlueprintModernization.test.tsx (2)
6-26: Jest module mock placement — verify hoisting in your test setupThis relies on Jest’s mock hoisting. If your config runs ESM without Babel’s hoist, the import on Line 2 may load before the mock. If uncertain, move the mock above imports or convert the import to require after the mock.
Example (safer ordering):
-import * as React from 'react'; -import { BlueprintModernizationContext } from '@box/blueprint-web'; +import * as React from 'react'; + // Mock the Blueprint Web package with simple components jest.mock('@box/blueprint-web', () => { ... }); +// Import after mock to avoid race conditions in ESM setups +// eslint-disable-next-line @typescript-eslint/no-var-requires +const { BlueprintModernizationContext } = require('@box/blueprint-web');
70-90: Add test for “upstream exists but disabled” (override semantics)Current tests don’t cover an upstream provider with enableModernizedComponents=false. Given the HOC wraps when upstream is false, add a test to lock that behavior in.
Apply this diff to add the test:
@@ test('should not wrap component when upstream BlueprintModernizationProvider exists', () => { @@ expect(queryByTestId('blueprint-provider')).not.toBeInTheDocument(); }); + + test('should wrap when upstream provider exists but is disabled (allow per-component override)', () => { + const ParentWithDisabledContext = ({ children }) => ( + <BlueprintModernizationContext.Provider value={{ enableModernizedComponents: false }}> + {children} + </BlueprintModernizationContext.Provider> + ); + + const { getByTestId } = render( + <ParentWithDisabledContext> + <WrappedComponent value="override" enableModernizedComponents={true} /> + </ParentWithDisabledContext>, + ); + + expect(getByTestId('test-component')).toHaveTextContent('Test override'); + // HOC should add its own provider to override disabled upstream + expect(getByTestId('blueprint-provider')).toHaveAttribute('data-enabled', 'true'); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
src/elements/common/__tests__/withBlueprintModernization.test.tsx(1 hunks)src/elements/common/withBlueprintModernization.tsx(1 hunks)src/elements/content-explorer/ContentExplorer.tsx(2 hunks)src/elements/content-explorer/stories/tests/ContentExplorer-visual.stories.js(1 hunks)src/elements/content-picker/ContentPicker.js(3 hunks)src/elements/content-picker/stories/tests/ContentPicker-visual.stories.js(1 hunks)src/elements/content-preview/ContentPreview.js(2 hunks)src/elements/content-preview/stories/tests/ContentPreview-visual.stories.js(2 hunks)src/elements/content-sharing/ContentSharing.js(2 hunks)src/elements/content-sidebar/ContentSidebar.js(2 hunks)src/elements/content-sidebar/stories/tests/ContentSidebar-visual.stories.tsx(1 hunks)src/elements/content-uploader/ContentUploader.tsx(3 hunks)src/elements/content-uploader/stories/tests/ContentUploader-visual.stories.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- src/elements/content-explorer/stories/tests/ContentExplorer-visual.stories.js
- src/elements/content-sidebar/stories/tests/ContentSidebar-visual.stories.tsx
- src/elements/content-sharing/ContentSharing.js
- src/elements/content-preview/stories/tests/ContentPreview-visual.stories.js
- src/elements/content-preview/ContentPreview.js
- src/elements/common/withBlueprintModernization.tsx
- src/elements/content-uploader/ContentUploader.tsx
- src/elements/content-picker/ContentPicker.js
- src/elements/content-sidebar/ContentSidebar.js
- src/elements/content-picker/stories/tests/ContentPicker-visual.stories.js
🧰 Additional context used
🧬 Code graph analysis (3)
src/elements/content-uploader/stories/tests/ContentUploader-visual.stories.js (4)
src/elements/content-explorer/stories/tests/ContentExplorer-visual.stories.js (2)
Modernization(31-35)Modernization(31-35)src/elements/content-picker/stories/tests/ContentPicker-visual.stories.js (2)
Modernization(11-15)Modernization(11-15)src/elements/content-preview/stories/tests/ContentPreview-visual.stories.js (2)
Modernization(33-37)Modernization(33-37)src/elements/content-sidebar/stories/tests/ContentSidebar-visual.stories.tsx (1)
Modernization(26-30)
src/elements/content-explorer/ContentExplorer.tsx (1)
src/elements/common/withBlueprintModernization.tsx (1)
withBlueprintModernization(8-25)
src/elements/common/__tests__/withBlueprintModernization.test.tsx (1)
src/elements/common/withBlueprintModernization.tsx (1)
withBlueprintModernization(8-25)
⏰ 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 (3)
src/elements/content-uploader/stories/tests/ContentUploader-visual.stories.js (1)
26-30: Modernization story variant added correctlyConsistent with other visual stories; args wiring looks right.
src/elements/content-explorer/ContentExplorer.tsx (1)
103-103: Import is correct and localizes HOC usageNo cycle risk; placement near other HOC imports is appropriate.
src/elements/common/__tests__/withBlueprintModernization.test.tsx (1)
58-68: Covers prop-driven enablement pathValidates provider receives enableModernizedComponents=true; good signal the HOC forwards props correctly.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/elements/content-picker/ContentPicker.js (2)
12-12: Alias lodash flow to “compose” for clarity, fool.Naming this import
flowright next to@flowis easy to misread. Alias it tocomposefor readability.-import flow from 'lodash/flow'; +import compose from 'lodash/flow';If you adopt this, update the default export usage accordingly:
export default compose([makeResponsive, withBlueprintModernization])(ContentPicker);
22-23: Add Flow types for withBlueprintModernization or document the suppression, fool!I’ve confirmed that
withBlueprintModernizationis a named export insrc/elements/common/withBlueprintModernization.tsx(export function withBlueprintModernization…)—so your import is correct and the suppression needs context, ya hear? I pity the fool who lets type holes spread!• Location to update suppression
- File:
src/elements/content-picker/ContentPicker.js- Lines: 22–23 (the
// $FlowFixMeabove the import)• Minimal tweak: keep the suppression but clarify why
- // $FlowFixMe + // $FlowFixMe TS interop: named export 'withBlueprintModernization' has no Flow types yet import { withBlueprintModernization } from '../common/withBlueprintModernization';• Preferred fix: add a Flow libdef stub so Flow can type-check the HOC and you can drop the suppression altogether. For example, under
flow-typed/withBlueprintModernization.js:declare module 'src/elements/common/withBlueprintModernization' { import type { ComponentType } from 'react'; import type { BlueprintModernizationContextValue } from '@box/blueprint-web'; declare export function withBlueprintModernization<P>( Component: ComponentType<P & BlueprintModernizationContextValue> ): ComponentType<P>; }That way Flow knows exactly what the HOC shape is and you won’t be hiding behind blanket
$FlowFixMe. I pity the fool who skips this!
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP (Model Context Protocol) integration is disabled
- Jira integration is disabled
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
src/elements/common/__tests__/withBlueprintModernization.test.tsx(1 hunks)src/elements/common/withBlueprintModernization.tsx(1 hunks)src/elements/content-explorer/ContentExplorer.tsx(2 hunks)src/elements/content-explorer/stories/tests/ContentExplorer-visual.stories.js(1 hunks)src/elements/content-picker/ContentPicker.js(3 hunks)src/elements/content-picker/stories/tests/ContentPicker-visual.stories.js(1 hunks)src/elements/content-preview/ContentPreview.js(2 hunks)src/elements/content-preview/stories/tests/ContentPreview-visual.stories.js(2 hunks)src/elements/content-sharing/ContentSharing.js(2 hunks)src/elements/content-sidebar/ContentSidebar.js(2 hunks)src/elements/content-sidebar/stories/tests/ContentSidebar-visual.stories.tsx(1 hunks)src/elements/content-uploader/ContentUploader.tsx(3 hunks)src/elements/content-uploader/stories/tests/ContentUploader-visual.stories.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- src/elements/content-picker/stories/tests/ContentPicker-visual.stories.js
- src/elements/common/tests/withBlueprintModernization.test.tsx
- src/elements/content-sidebar/stories/tests/ContentSidebar-visual.stories.tsx
- src/elements/content-uploader/stories/tests/ContentUploader-visual.stories.js
- src/elements/content-uploader/ContentUploader.tsx
- src/elements/content-sidebar/ContentSidebar.js
- src/elements/content-preview/stories/tests/ContentPreview-visual.stories.js
- src/elements/content-sharing/ContentSharing.js
- src/elements/content-explorer/ContentExplorer.tsx
- src/elements/content-preview/ContentPreview.js
- src/elements/common/withBlueprintModernization.tsx
🧰 Additional context used
🧠 Learnings (2)
📚 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-picker/ContentPicker.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-picker/ContentPicker.js
🧬 Code graph analysis (1)
src/elements/content-explorer/stories/tests/ContentExplorer-visual.stories.js (4)
src/elements/content-picker/stories/tests/ContentPicker-visual.stories.js (2)
Modernization(11-15)Modernization(11-15)src/elements/content-preview/stories/tests/ContentPreview-visual.stories.js (2)
Modernization(33-37)Modernization(33-37)src/elements/content-sidebar/stories/tests/ContentSidebar-visual.stories.tsx (1)
Modernization(26-30)src/elements/content-uploader/stories/tests/ContentUploader-visual.stories.js (2)
Modernization(26-30)Modernization(26-30)
⏰ 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 (2)
src/elements/content-explorer/stories/tests/ContentExplorer-visual.stories.js (1)
31-35: LGTM — clean Modernization variant, fool!Matches the pattern in sibling stories and keeps args focused. I pity the fool who breaks consistency—this one doesn’t.
src/elements/content-picker/ContentPicker.js (1)
1351-1351: HOC order LGTM — left→right wraps as intended.
flow([makeResponsive, withBlueprintModernization])(ContentPicker)yieldswithBlueprintModernization(makeResponsive(ContentPicker)). If that’s the behavior you want, you’re golden. Don’t get it backwards, sucka!
src/elements/content-explorer/stories/tests/ContentExplorer-visual.stories.js
Show resolved
Hide resolved
Summary by CodeRabbit
New Features
Tests