feat(router): optional router in StaticVersionSidebar and VersionsSid… #4152
Conversation
WalkthroughThis change refactors the version sidebar components to support an optional Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SidebarComponent
participant Router
participant InternalNavHandler
User->>SidebarComponent: Clicks Back Button
alt routerDisabled is true
SidebarComponent->>InternalNavHandler: handleNavigation(sidebarName)
else routerDisabled is false
SidebarComponent->>Router: history.push(parentSidebarRoute)
end
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/elements/content-sidebar/versions/VersionsSidebar.js (1)
1-127: Verify StaticVersionSidebar.js component was updated similarly.The tests suggest that
StaticVersionSidebar.jswas updated with similarrouterDisabledfunctionality, but the component file isn't included in this review. Please ensure that component received similar refactoring to maintain consistency.#!/bin/bash # Description: Verify StaticVersionSidebar.js has similar routerDisabled implementation # Expected: Should find routerDisabled prop and similar conditional logic echo "=== Checking StaticVersionSidebar.js for routerDisabled implementation ===" cat src/elements/content-sidebar/versions/StaticVersionSidebar.js | grep -A 10 -B 5 "routerDisabled\|internalSidebarNavigation" echo -e "\n=== Checking for handleBackClick or similar navigation logic ===" cat src/elements/content-sidebar/versions/StaticVersionSidebar.js | grep -A 5 -B 5 "handleBackClick\|BackButton.*onClick" echo -e "\n=== Checking component props structure ===" ast-grep --pattern 'type Props = { $$$ }'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/elements/common/types/SidebarNavigation.js.flow(1 hunks)src/elements/common/types/SidebarNavigation.ts(1 hunks)src/elements/content-sidebar/versions/StaticVersionSidebar.js(2 hunks)src/elements/content-sidebar/versions/VersionsSidebar.js(1 hunks)src/elements/content-sidebar/versions/__tests__/StaticVersionSidebar.test.js(4 hunks)src/elements/content-sidebar/versions/__tests__/VersionsSidebar.test.js(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/elements/common/types/SidebarNavigation.js.flow (1)
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.
🪛 Biome (1.9.4)
src/elements/content-sidebar/versions/VersionsSidebar.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] 25-37: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 48-48: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 54-54: optional parameters are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 54-54: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 62-62: optional parameters are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 62-62: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
src/elements/content-sidebar/versions/StaticVersionSidebar.js
[error] 17-17: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 24-32: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 41-41: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 41-41: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 61-61: optional parameters are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 61-61: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 69-69: optional parameters are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 69-69: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (17)
src/elements/common/types/SidebarNavigation.ts (1)
42-42: LGTM! Type change supports optional router functionality.Making the
openproperty optional inInternalSidebarNavigationaligns well with the newrouterDisabledmode where sidebar state management becomes more flexible.src/elements/common/types/SidebarNavigation.js.flow (1)
33-33: LGTM! Maintains type consistency with TypeScript definition.The optional
openproperty change matches the corresponding TypeScript type definition, ensuring consistent behavior across both type systems.src/elements/content-sidebar/versions/__tests__/StaticVersionSidebar.test.js (2)
105-141: LGTM! Comprehensive test coverage for router vs internal navigation.The test properly verifies that when React Router is enabled, the internal navigation handler is not called and routing uses the standard router mechanism.
180-222: LGTM! Thorough test coverage for routerDisabled mode.The new test suite comprehensively covers the
routerDisabledfunctionality:
- Renders correctly without React Router context
- Internal navigation handler is called with correct parameters
- Props are passed correctly to child components
src/elements/content-sidebar/versions/__tests__/VersionsSidebar.test.js (2)
137-170: LGTM! Consistent test approach with router navigation verification.The updated test properly verifies that router navigation takes precedence over internal navigation when router is enabled, maintaining consistency with the StaticVersionSidebar test pattern.
172-213: LGTM! Complete test coverage for routerDisabled functionality.The test suite provides thorough coverage of the
routerDisabledmode, ensuring:
- Component renders without React Router
- Internal navigation handler is invoked correctly
- Props are passed to child components appropriately
src/elements/content-sidebar/versions/VersionsSidebar.js (4)
19-19: LGTM! Well-defined props for optional router functionality.The new props are properly typed and optional, ensuring backward compatibility while enabling the new internal navigation mode.
Also applies to: 29-31, 33-33
54-60: LGTM! Robust conditional navigation logic.The
handleBackClickcallback correctly handles both routing modes:
- Uses internal navigation when
routerDisabledis true- Falls back to React Router when available
- Proper parameter validation and conditional execution
62-113: LGTM! Clean separation of rendering logic.The
renderContentfunction properly extracts the rendering logic and maintains all existing functionality while supporting the new optionalhistoryparameter for conditional router usage.
115-123: LGTM! Clean conditional rendering implementation.The conditional rendering based on
routerDisabledis clean and maintains the same UI structure in both modes while properly handling router dependency.src/elements/content-sidebar/versions/StaticVersionSidebar.js (7)
17-17: LGTM: Appropriate type imports added.The import of
InternalSidebarNavigationandInternalSidebarNavigationHandlertypes is correctly added to support the new optional navigation functionality.
23-23: LGTM: useCallback import added for performance optimization.The
useCallbackimport is appropriate for the new navigation handler to prevent unnecessary re-renders.
25-32: LGTM: Props type extended with optional navigation properties.The new optional props maintain backward compatibility while enabling the new router-disabled functionality:
internalSidebarNavigation?: For internal navigation stateinternalSidebarNavigationHandler?: Callback for internal navigationrouterDisabled?: Flag to control navigation mode
34-41: LGTM: Component signature updated correctly.The function signature properly destructures the new optional props while maintaining the existing parameter structure.
61-67: LGTM: Well-implemented conditional navigation logic.The
handleBackClickcallback correctly implements the conditional navigation:
- When
routerDisabled=true: uses internal navigation handler- When
routerDisabled=false/undefined: uses React Router historyThe
useCallbackdependencies are correctly specified to prevent unnecessary re-renders.
69-125: LGTM: Clean refactoring of rendering logic.The
renderContentfunction effectively abstracts the rendering logic to support both navigation modes. The optionalhistoryparameter allows the same component structure to work with or without React Router.The props passed to
VersionsMenucorrectly include the new navigation-related props.
127-131: LGTM: Conditional rendering implements the router-disabled feature correctly.The conditional rendering logic cleanly separates the two modes:
- When
routerDisabled=true: renders content directly without Route wrapper- Otherwise: renders within Route component with history access
This maintains backward compatibility while enabling the new functionality.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/elements/content-sidebar/versions/StaticVersionSidebar.js(2 hunks)src/elements/content-sidebar/versions/VersionsSidebar.js(1 hunks)src/elements/content-sidebar/versions/VersionsSidebarContainer.js(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/elements/content-sidebar/versions/VersionsSidebarContainer.js (1)
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.
🪛 Biome (1.9.4)
src/elements/content-sidebar/versions/VersionsSidebarContainer.js
[error] 27-27: '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-sidebar/versions/StaticVersionSidebar.js
[error] 17-21: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 28-36: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 45-45: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 45-45: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 65-65: optional parameters are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 65-65: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 73-73: optional parameters are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 73-73: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
src/elements/content-sidebar/versions/VersionsSidebar.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] 25-37: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 48-48: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 54-54: optional parameters are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 54-54: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 62-62: optional parameters are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 62-62: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (25)
src/elements/content-sidebar/versions/VersionsSidebarContainer.js (3)
27-27: Good type refinement for parentName prop.The change from generic
stringto specificViewTypeValuestype improves type safety and aligns with the other version sidebar components.Also applies to: 44-44
27-27: Good type improvement for better type safety.The change from generic
stringto specificViewTypeValuestype for theparentNameprop improves type safety and aligns with similar changes across related components.Also applies to: 44-44
27-27: Type refinement looks good.The change from generic
stringto specificViewTypeValuestype improves type safety for theparentNameprop.Also applies to: 44-44
src/elements/content-sidebar/versions/VersionsSidebar.js (11)
19-19: Well-structured imports for new navigation features.The addition of the new navigation types and React hooks is properly organized and supports the router-disabled functionality.
Also applies to: 22-22
29-33: Good prop additions for optional routing.The new props are properly typed and optional, allowing backward compatibility while enabling the new internal navigation mode.
54-60: Robust conditional navigation logic.The
handleBackClickcallback correctly handles both router and internal navigation modes with proper guard conditions. The dependency array inuseCallbackis complete and accurate.
115-123: Clean conditional rendering pattern.The pattern of conditional rendering based on
routerDisabledis clean and maintainable. When routing is disabled, it renders content directly; otherwise it wraps with Route to access history.
93-99: Proper prop forwarding to VersionsMenu.The
routerDisabledandinternalSidebarNavigationprops are correctly passed to theVersionsMenucomponent to maintain consistent navigation behavior throughout the component tree.
19-19: LGTM: Well-structured prop additions for router flexibility.The new props
internalSidebarNavigation,internalSidebarNavigationHandler, androuterDisabledare properly typed and provide the necessary flexibility for both router-based and internal navigation modes.Also applies to: 29-33
54-60: Excellent conditional navigation logic.The
handleBackClickcallback properly handles both routing modes:
- Internal navigation when
routerDisabledis true- React Router navigation otherwise
The
useCallbackoptimization and dependency array are correctly implemented.
62-123: Clean refactoring enables flexible rendering modes.The extraction of rendering logic into
renderContent()and conditional wrapper logic allows the component to work both with and without React Router while maintaining the same UI structure and functionality.
54-60: Well-designed conditional navigation logic.The
handleBackClickcallback properly handles both routing scenarios with clear conditional logic. The use ofuseCallbackwith appropriate dependencies is correct.
115-123: Clean separation of routing modes.The conditional rendering based on
routerDisabledcleanly separates the two navigation modes while reusing the same content rendering logic.
62-113: Good refactoring with renderContent function.Moving the rendering logic into a reusable
renderContentfunction that accepts an optionalhistoryparameter is a clean solution that avoids code duplication between routing modes.src/elements/content-sidebar/versions/StaticVersionSidebar.js (11)
17-21: Consistent imports and setup with VersionsSidebar.The import structure and React hook usage align perfectly with the pattern established in VersionsSidebar, promoting consistency across the codebase.
Also applies to: 27-27
30-35: Proper prop typing for navigation features.The new props follow the same pattern as VersionsSidebar with appropriate optional typing, ensuring backward compatibility and type safety.
65-71: Consistent navigation logic implementation.The
handleBackClickimplementation matches the pattern in VersionsSidebar exactly, ensuring consistent behavior across both static and dynamic version sidebar components.
131-135: Identical conditional rendering pattern.The conditional rendering logic perfectly mirrors VersionsSidebar, maintaining consistency in how router-disabled mode is handled across all version sidebar components.
98-105: Correct prop passing to VersionsMenu.The props are properly forwarded to VersionsMenu, ensuring the navigation behavior is consistent throughout the component hierarchy.
17-21: Consistent prop additions align with VersionsSidebar.The new props and types match those added to
VersionsSidebar, ensuring consistent API across both sidebar components.Also applies to: 30-36
65-71: Consistent navigation logic across sidebar components.The
handleBackClickimplementation matches the pattern used inVersionsSidebar, ensuring consistent behavior between static and dynamic sidebar components.
73-135: Consistent refactoring pattern maintains component identity.The refactoring follows the same pattern as
VersionsSidebarwhile preserving the component's unique static content and upsell functionality. This ensures consistent navigation behavior across both sidebar variants.
65-71: Consistent implementation with VersionsSidebar.The
handleBackClickimplementation matches the pattern used inVersionsSidebar.js, providing consistent behavior across components.
131-135: Proper conditional rendering implementation.The routing mode separation follows the same clean pattern as the main
VersionsSidebarcomponent, ensuring consistent behavior.
73-129: Good code reuse with renderContent function.The extracted
renderContentfunction maintains the same structure as the corresponding component, enabling clean separation between routing and non-routing modes.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/elements/content-sidebar/versions/VersionsSidebar.js (1)
53-63: Component extraction follows established patterns but consider the noted anti-pattern concern.Based on past review comments, defining components inside other components was flagged as an anti-pattern. However, this implementation appears to be a transitional approach during migration.
The component signature and prop destructuring are well-organized and type-safe.
src/elements/content-sidebar/versions/StaticVersionSidebar.js (1)
48-56: Component extraction maintains consistency with VersionsSidebar pattern.The StaticVersionsContent component follows the same extraction pattern as VersionsContent, which addresses the migration needs despite the noted anti-pattern concern from past reviews.
🧹 Nitpick comments (1)
src/elements/content-sidebar/versions/StaticVersionSidebar.js (1)
143-155: Address the variable redeclaration issue flagged by static analysis.The static analysis tool correctly identifies that
routerDisabledis being redeclared. While the logic is correct, the destructuring creates an unnecessary redeclaration.Apply this diff to avoid the redeclaration:
const StaticVersionsSidebar = (props: Props): React.Node => { - const { routerDisabled } = props; - - if (routerDisabled) { + if (props.routerDisabled) { return <StaticVersionsContent {...props} />; } return ( <Route> {({ history }) => <StaticVersionsContent {...props} history={history} />} </Route> ); };Alternatively, if you prefer to keep the destructuring for readability, you could rename the destructured variable to avoid the warning.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/elements/content-sidebar/versions/__tests__/__snapshots__/VersionsSidebarContainer.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
src/elements/content-sidebar/versions/StaticVersionSidebar.js(2 hunks)src/elements/content-sidebar/versions/VersionsSidebar.js(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/elements/content-sidebar/versions/StaticVersionSidebar.js
[error] 17-21: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 28-36: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 37-46: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 56-56: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 56-56: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 143-143: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 143-143: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 144-144: Shouldn't redeclare 'routerDisabled'. Consider to delete it or rename it.
'routerDisabled' is defined here:
(lint/suspicious/noRedeclare)
src/elements/content-sidebar/versions/VersionsSidebar.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] 25-37: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 38-51: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 63-63: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 131-131: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (13)
src/elements/content-sidebar/versions/VersionsSidebar.js (7)
19-19: Import statement is correct despite static analysis warning.The static analysis tool flags this as TypeScript syntax, but this is a Flow-typed JavaScript file (indicated by
@flowcomment), whereimport typeis valid syntax.
22-22: Good practice: destructuring useCallback at module level.Extracting
useCallbackfrom React improves readability and follows React hooks best practices.
26-37: Type definitions are well-structured and comprehensive.The Props type correctly includes all the new optional navigation-related properties with appropriate typing.
39-51: VersionsContentProps type definition is appropriate.The type includes
history?: anyfor optional router history and all necessary navigation props. Theanytype for history is acceptable here as it matches React Router's typing patterns.
69-75: Navigation logic is correctly implemented with proper conditional handling.The
handleBackClickcallback properly handles both routing scenarios:
- When
routerDisabledis true: uses internal navigation handler- When routing is enabled: uses React Router's history.push
- Includes proper null checks and dependency array for useCallback
The logic correctly prioritizes the routing mode and handles the navigation appropriately.
108-113: Props forwarding to VersionsMenu is correctly implemented.The component properly passes the new
routerDisabledandinternalSidebarNavigationprops toVersionsMenu, ensuring consistent navigation behavior throughout the component tree.
131-143: Conditional rendering logic is clean and follows the intended pattern.The main component correctly:
- Extracts
routerDisabledfrom props- Returns
VersionsContentdirectly when routing is disabled- Wraps
VersionsContentinRoutewhen routing is enabled, providing history via render propThis approach maintains backward compatibility while supporting the new router-less mode.
src/elements/content-sidebar/versions/StaticVersionSidebar.js (6)
17-21: Import types are correctly defined despite static analysis warning.Similar to the other file, this is Flow-typed JavaScript where
import typesyntax is valid. The imported types are necessary for the new navigation functionality.
27-27: Consistent React hooks usage.Good practice to destructure
useCallbackfrom React at module level, maintaining consistency with the VersionsSidebar implementation.
29-36: Props type definition is comprehensive and well-structured.The type includes all necessary navigation-related properties with appropriate optional typing, consistent with the VersionsSidebar implementation.
38-46: StaticVersionsContentProps type is appropriately defined.The type structure mirrors the VersionsContentProps from the other file, maintaining consistency across the refactoring.
76-82: Navigation logic is identical and correctly implemented.The
handleBackClickimplementation is consistent with VersionsSidebar, properly handling both routing modes with appropriate conditional logic and dependency management.
109-116: Props forwarding to VersionsMenu maintains consistency.The component correctly passes the new navigation-related props to
VersionsMenu, ensuring consistent behavior across both sidebar implementations.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/elements/content-sidebar/versions/VersionsSidebar.js (1)
69-75: Consider extracting shared navigation logic.Both VersionsSidebar and StaticVersionSidebar implement identical
handleBackClicklogic. Consider creating a shared custom hook or utility function to reduce code duplication:// utils/useBackNavigation.js const useBackNavigation = (parentName, routerDisabled, internalSidebarNavigationHandler, history) => { return useCallback(() => { if (routerDisabled && internalSidebarNavigationHandler) { internalSidebarNavigationHandler({ sidebar: parentName }); } else if (!routerDisabled && history) { history.push(`/${parentName}`); } }, [parentName, routerDisabled, internalSidebarNavigationHandler, history]); };This would eliminate duplication and centralize the navigation logic maintenance.
Also applies to: 76-82
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/elements/content-sidebar/versions/__tests__/__snapshots__/VersionsSidebarContainer.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
src/elements/content-sidebar/versions/StaticVersionSidebar.js(2 hunks)src/elements/content-sidebar/versions/VersionsSidebar.js(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/elements/content-sidebar/versions/VersionsSidebar.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] 25-37: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 38-51: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 63-63: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 131-131: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
src/elements/content-sidebar/versions/StaticVersionSidebar.js
[error] 17-21: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 28-36: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 37-46: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 56-56: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 56-56: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 143-143: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 143-143: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 144-144: Shouldn't redeclare 'routerDisabled'. Consider to delete it or rename it.
'routerDisabled' is defined here:
(lint/suspicious/noRedeclare)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (18)
src/elements/content-sidebar/versions/StaticVersionSidebar.js (9)
17-21: LGTM: Type imports are correctly structured.The type imports are properly organized and align with the new navigation functionality being introduced.
27-27: LGTM: useCallback import follows React best practices.Proper destructuring of useCallback from React for the navigation handler.
29-36: LGTM: Props type definition is well-structured.The new props for internal navigation support are properly typed and optional where appropriate. The
parentNametype change fromstringtoViewTypeValuesadds better type safety.
38-46: LGTM: StaticVersionsContentProps type definition is comprehensive.The props type includes all necessary properties for both router and router-disabled modes, with appropriate optional flags.
48-56: LGTM: Component extraction follows good separation of concerns.The StaticVersionsContent component properly accepts all required props and maintains clear boundaries between routing and content logic.
76-82: LGTM: Navigation logic correctly handles both routing modes.The handleBackClick callback properly:
- Uses useCallback for performance optimization
- Implements conditional navigation based on routerDisabled flag
- Includes proper dependency array
- Handles both internal navigation and router navigation paths
94-97: LGTM: BackButton integration is clean.The BackButton properly uses the handleBackClick callback and maintains the data-resin-target attribute for analytics.
109-116: LGTM: VersionsMenu receives appropriate props for navigation support.The VersionsMenu component correctly receives the
routerDisabledandinternalSidebarNavigationprops to support the new navigation mode.
143-155: LGTM: Conditional rendering pattern is well-implemented.The main component correctly:
- Conditionally renders StaticVersionsContent directly when routing is disabled
- Wraps with Route component to provide history when routing is enabled
- Maintains clean separation between routing and non-routing modes
src/elements/content-sidebar/versions/VersionsSidebar.js (9)
19-19: LGTM: Type imports are consistent with StaticVersionSidebar.The import statement correctly includes the necessary types for internal navigation support, maintaining consistency across both sidebar components.
22-22: LGTM: useCallback import follows established pattern.Consistent with the StaticVersionSidebar implementation.
26-37: LGTM: Props type definition maintains consistency.The Props type correctly mirrors the navigation-related props added to StaticVersionSidebar, ensuring a consistent API across both components.
39-51: LGTM: VersionsContentProps type is comprehensive.The type definition includes all necessary props for both routing modes and maintains consistency with the StaticVersionSidebar pattern.
53-63: LGTM: Component extraction follows established pattern.The VersionsContent component extraction is consistent with the StaticVersionSidebar approach and properly destructures the required props.
69-75: LGTM: Navigation logic is consistent and correct.The handleBackClick implementation:
- Matches the pattern established in StaticVersionSidebar
- Properly handles both internal navigation and router navigation
- Uses correct dependency array for useCallback
- Maintains consistent conditional logic
84-84: LGTM: BackButton integration is consistent.The BackButton usage matches the StaticVersionSidebar implementation with proper callback binding.
108-113: LGTM: VersionsMenu props are consistent with StaticVersionSidebar.The VersionsMenu component receives the same navigation-related props as in StaticVersionSidebar, maintaining API consistency.
131-143: LGTM: Conditional rendering pattern matches StaticVersionSidebar.The main component implementation is consistent with the StaticVersionSidebar approach:
- Same conditional rendering logic based on routerDisabled
- Proper Route wrapper when routing is enabled
- Clean separation of concerns
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/elements/content-sidebar/versions/StaticVersionSidebar.js (2)
143-154: Clean conditional rendering approach, but consider simplifying variable destructuring.The conditional rendering logic is well-structured. However, there's a minor issue with variable redeclaration.
Consider this cleaner approach to avoid the redeclaration warning:
-const StaticVersionsSidebar = (props: Props): React.Node => { - const { routerDisabled } = props; - - if (routerDisabled) { +const StaticVersionsSidebar = (props: Props): React.Node => { + if (props.routerDisabled) { return <StaticVersionsContent {...props} />; }
38-46: Consider improving type safety for the history prop.The
historyprop is typed asany, which reduces type safety. Consider using a more specific type fromreact-router-domif available.type StaticVersionsContentProps = { error?: MessageDescriptor, fileId: string, - history?: any, + history?: { push: (path: string) => void }, // ... rest of props };src/elements/content-sidebar/versions/VersionsSidebar.js (2)
131-142: Consider consolidating the duplicate conditional rendering pattern.Both
VersionsSidebarandStaticVersionsSidebarimplement identical conditional rendering logic. Consider extracting this pattern into a higher-order component or custom hook to reduce duplication.Example approach using a custom hook:
const useConditionalRouter = (routerDisabled, ContentComponent, props) => { if (routerDisabled) { return <ContentComponent {...props} />; } return ( <Route> {({ history }) => <ContentComponent {...props} history={history} />} </Route> ); };
39-51: Consider consistent type safety improvement for history prop.Similar to StaticVersionSidebar, the
historyprop is typed asany. For consistency and better type safety, consider using a more specific type.type VersionsContentProps = { error?: MessageDescriptor, fileId: string, - history?: any, + history?: { push: (path: string) => void }, // ... rest of props };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/elements/content-sidebar/versions/StaticVersionSidebar.js(2 hunks)src/elements/content-sidebar/versions/VersionsSidebar.js(1 hunks)src/elements/content-sidebar/versions/__tests__/StaticVersionSidebar.test.js(4 hunks)src/elements/content-sidebar/versions/__tests__/VersionsSidebar.test.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/elements/content-sidebar/versions/tests/StaticVersionSidebar.test.js
- src/elements/content-sidebar/versions/tests/VersionsSidebar.test.js
🧰 Additional context used
🪛 Biome (1.9.4)
src/elements/content-sidebar/versions/StaticVersionSidebar.js
[error] 17-21: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 28-36: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 37-46: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 56-56: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 56-56: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 143-143: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 143-143: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 144-144: Shouldn't redeclare 'routerDisabled'. Consider to delete it or rename it.
'routerDisabled' is defined here:
(lint/suspicious/noRedeclare)
src/elements/content-sidebar/versions/VersionsSidebar.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] 25-37: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 38-51: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 63-63: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 131-131: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Rule: Automatic merge queue (queue)
- GitHub Check: lint_test_build
- GitHub Check: Summary
- GitHub Check: Queue: Embarked in merge queue
🔇 Additional comments (16)
src/elements/content-sidebar/versions/StaticVersionSidebar.js (8)
17-21: LGTM: Type imports properly added for new navigation functionality.The new type imports support the optional router functionality and maintain type safety with Flow.
29-36: LGTM: Props type definition correctly expanded for optional router support.The new optional props properly support both router-enabled and router-disabled modes while maintaining backward compatibility.
48-56: Good separation of concerns with extracted content component.Extracting the main content into
StaticVersionsContentprovides a clean separation between routing logic and UI rendering, making the code more maintainable.
76-82: Well-implemented conditional navigation logic.The
handleBackClickcallback correctly handles both routing scenarios:
- Uses internal navigation handler when router is disabled
- Falls back to React Router when enabled
The
useCallbackoptimization is appropriate here since the callback depends on multiple props.
17-21: Type imports look appropriate for the new functionality.The imported types support the new internal navigation feature and provide proper typing for the refactored component structure.
76-82: Back button navigation logic is well-implemented.The
useCallbackhook is properly used with correct dependencies, and the conditional logic appropriately handles both router-enabled and router-disabled navigation scenarios.
109-116: Props are correctly passed to VersionsMenu.The new
routerDisabledandinternalSidebarNavigationprops are properly forwarded to support the internal navigation functionality.
143-155: Conditional rendering pattern is clean and effective.The wrapper component appropriately handles both router-enabled and router-disabled modes by conditionally rendering the content component directly or wrapped in a Route component.
src/elements/content-sidebar/versions/VersionsSidebar.js (8)
19-19: LGTM: Consistent type imports across sidebar components.The type imports align with the StaticVersionSidebar implementation, maintaining consistency across the codebase.
53-63: Good architectural consistency with StaticVersionSidebar.The extracted
VersionsContentcomponent follows the same pattern asStaticVersionsContent, providing consistent behavior across both sidebar components.
69-75: Consistent navigation logic implementation.The
handleBackClickimplementation mirrors the StaticVersionSidebar approach, ensuring uniform behavior across both components. The conditional logic properly handles both routing modes.
108-113: Props correctly passed to VersionsMenu for router support.The
routerDisabledandinternalSidebarNavigationprops are properly forwarded to the VersionsMenu component, enabling it to handle the new navigation modes.
19-19: Type imports are consistent and appropriate.The imported types align with the StaticVersionSidebar implementation and support the new internal navigation functionality effectively.
69-75: Navigation handling logic is consistent and correct.The
handleBackClickimplementation mirrors the StaticVersionSidebar approach, ensuring consistent behavior across both sidebar components.
108-113: Props forwarding to VersionsMenu is properly implemented.The new navigation-related props are correctly passed through to support the internal navigation functionality.
131-143: Wrapper component follows the established pattern.The conditional rendering logic is consistent with StaticVersionSidebar, maintaining a unified approach to handling router-enabled and router-disabled modes across both sidebar components.
|
This pull request has been removed from the queue for the following reason: The merge conditions cannot be satisfied due to failing checks. You may have to fix your CI before adding the pull request to the queue again. |
3 participants
…ebar
Added router disabling switch to VersionsSidebar components and created router-less branch of code inside.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests