feat(router): optional router in SidebarNavigation #4194
Conversation
WalkthroughThis update extends and reorders the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PreviewNavigation
participant Router
participant InternalHandler
User->>PreviewNavigation: Click navigation button
alt routerDisabled is false
PreviewNavigation->>Router: Use history.push for navigation
else routerDisabled is true
PreviewNavigation->>InternalHandler: Call internalSidebarNavigationHandler
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
✨ 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. 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 (
|
src/elements/content-preview/__tests__/PreviewNavigation.test.js
Outdated
Show resolved
Hide resolved
src/elements/content-preview/__tests__/PreviewNavigation.test.js
Outdated
Show resolved
Hide resolved
src/elements/content-preview/__tests__/PreviewNavigation.test.js
Outdated
Show resolved
Hide resolved
src/elements/content-preview/__tests__/PreviewNavigation.test.js
Outdated
Show resolved
Hide resolved
![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-preview/PreviewNavigation.js (1)
17-28: Consider improving callback type definitions.The new props for internal navigation are well-designed. However, the
Functiontype foronNavigateLeftandonNavigateRightcould be more specific for better type safety.Consider replacing the generic
Functiontypes with more specific signatures:- onNavigateLeft: Function, - onNavigateRight: Function, + onNavigateLeft: () => void, + onNavigateRight: () => void,Note: The static analysis warnings about TypeScript syntax are false positives - this file correctly uses Flow type syntax as indicated by the
@flowcomment at the top.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/elements/content-preview/PreviewNavigation.js(4 hunks)src/elements/content-preview/__tests__/PreviewNavigation.test.js(2 hunks)src/elements/content-sidebar/versions/__tests__/StaticVersionSidebar.test.js(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/elements/content-sidebar/versions/tests/StaticVersionSidebar.test.js
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#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: 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.
src/elements/content-preview/__tests__/PreviewNavigation.test.js (5)
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.
Learnt from: tjuanitas
PR: #4126
File: scripts/buildTranslations.js:1-8
Timestamp: 2025-06-17T15:16:46.279Z
Learning: The buildTranslations and buildLanguageBundles functions from @box/frontend package are synchronous functions that already handle errors internally, so additional error handling wrappers and await keywords are not needed.
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.
src/elements/content-preview/PreviewNavigation.js (3)
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: #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: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.
🧬 Code Graph Analysis (1)
src/elements/content-preview/PreviewNavigation.js (1)
src/elements/common/types/SidebarNavigation.ts (2)
InternalSidebarNavigation(49-51)InternalSidebarNavigationHandler(55-55)
🪛 Biome (1.9.4)
src/elements/content-preview/PreviewNavigation.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] 18-28: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 36-36: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 25-25: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 26-26: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 100-100: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 155-155: 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. 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 (8)
src/elements/content-preview/__tests__/PreviewNavigation.test.js (5)
4-6: LGTM! Clean migration to React Testing Library.The import changes correctly support the migration from Enzyme to RTL with appropriate utilities and type imports for the new test scenarios.
22-52: Well-structured helper functions for dual testing modes.The separation into
renderComponentWithRouterandrenderComponentWithoutRouterprovides clean abstractions that align with the component's refactored architecture. The default props and flexibility through spread operators are well-implemented.
54-56: Good improvement in test lifecycle management.The change from
afterEachtobeforeEachensures clean state before each test, preventing test pollution and interdependencies.
60-171: Excellent RTL migration with comprehensive coverage.The migration from Enzyme to React Testing Library is well-executed, following RTL best practices:
- Proper use of
screenqueries and presence assertions- Async user interactions with
userEvent- Parameterized tests provide good coverage for different navigation states
- Test descriptions are clear and maintainable
173-280: Comprehensive test coverage for router-disabled mode.The new test suite thoroughly covers the router-disabled functionality:
- Verifies rendering without router context
- Tests internal navigation handler invocation with correct parameters
- Covers both basic navigation and metadata deeplink scenarios
- Ensures original navigation callbacks remain functional
- Validates directional callback isolation (left vs right)
The test cases align well with the new component architecture and provide confidence in the router-disabled implementation.
src/elements/content-preview/PreviewNavigation.js (3)
30-90: Well-preserved router functionality with improved testability.The
PreviewNavigationWithRoutercomponent correctly maintains the original router-based behavior while adding the necessarydata-testidattributes for testing. ThegoToActiveSidebarTabhelper function properly handles deeplink navigation scenarios.
92-153: Well-implemented router-disabled navigation logic.The
PreviewNavigationWithoutRoutercomponent effectively handles the router-disabled use case. ThehandleInternalNavigationfunction correctly differentiates between metadata deeplinks (preserving all navigation properties) and basic navigation (preserving only the sidebar type), which aligns with the routing behavior in the router-enabled version.
155-163: Clean abstraction with backward compatibility.The main
PreviewNavigationcomponent provides a clean interface that conditionally renders the appropriate variant based on therouterDisabledprop. The default value ensures backward compatibility while enabling the new router-disabled functionality when needed.
5 participants
Made React Router usage in SidebarNavigation optional by providing switch and optional navigation object and handler.
Converted component tests to RTL and expanded to cover new Logic
Updated Navigation Object shape with missing properties
Summary by CodeRabbit
New Features
Bug Fixes
Tests