feat(router): optional router in SidebarNav, SidebarNavTablist #4177
Conversation
|
""" WalkthroughThe changes introduce optional internal navigation and router disabling support to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SidebarNav
participant SidebarNavTablist
participant Router
participant InternalNavHandler
User->>SidebarNav: Interacts (e.g., keydown)
SidebarNav->>SidebarNavTablist: Pass navigation props
SidebarNavTablist->>SidebarNavTablist: handleKeyDown(event)
alt routerDisabled = false
SidebarNavTablist->>Router: history.push(newPath)
else routerDisabled = true
SidebarNavTablist->>InternalNavHandler: internalSidebarNavigationHandler(newView)
end
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learningssrc/elements/content-sidebar/SidebarNavTablist.js (2)🪛 Biome (1.9.4)src/elements/content-sidebar/SidebarNavTablist.js[error] 12-12: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax. TypeScript only syntax (parse) [error] 13-21: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax. TypeScript only syntax (parse) [error] 21-23: Expected a statement but instead found ', Expected a statement here. (parse) [error] 34-34: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax. TypeScript only syntax (parse) [error] 38-38: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax. TypeScript only syntax (parse) [error] 38-38: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax. TypeScript only syntax (parse) [error] 68-68: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax. TypeScript only syntax (parse) [error] 68-68: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax. TypeScript only syntax (parse) [error] 36-36: Change to an optional chain. Unsafe fix: Change to an optional chain. (lint/complexity/useOptionalChain) [error] 38-38: void is confusing outside a return type or a type parameter. Unsafe fix: Use undefined instead. (lint/suspicious/noConfusingVoidType) [error] 68-68: void is confusing outside a return type or a type parameter. Unsafe fix: Use undefined instead. (lint/suspicious/noConfusingVoidType) [error] 135-135: 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)
🔇 Additional comments (8)
✨ 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 (
|
rafalmaksymiuk
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/elements/content-sidebar/__tests__/SidebarNavTablist.test.js (1)
49-49: Consider moving require() to top-level importWhile the
require()inside the component works, it's unconventional. Consider extracting the history hook to the top level.- const history = require('react-router-dom').useHistory(); + const history = useHistory();And add the import at the top:
-import { MemoryRouter } from 'react-router-dom'; +import { MemoryRouter, useHistory } from 'react-router-dom';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/elements/content-sidebar/__tests__/SidebarNav.test.js(3 hunks)src/elements/content-sidebar/__tests__/SidebarNavTablist.test.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/elements/content-sidebar/tests/SidebarNav.test.js
🧰 Additional context used
🧠 Learnings (2)
📓 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-sidebar/__tests__/SidebarNavTablist.test.js (3)
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.
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#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.
🧬 Code Graph Analysis (1)
src/elements/content-sidebar/__tests__/SidebarNavTablist.test.js (3)
src/elements/content-sidebar/versions/__tests__/VersionsSidebarContainer.rtl.test.js (1)
history(35-35)src/elements/content-sidebar/SidebarNavTablist.js (1)
tablist(36-36)src/elements/content-sidebar/__tests__/SidebarToggle.test.js (1)
mockInternalSidebarNavigationHandler(34-34)
⏰ 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 (7)
src/elements/content-sidebar/__tests__/SidebarNavTablist.test.js (7)
2-3: LGTM: Proper RTL migration setupThe import changes correctly introduce React Testing Library utilities and MemoryRouter, which are appropriate for testing components with routing behavior.
13-31: Well-implemented MockTabComponent with proper forwardRefThe MockTabComponent is correctly implemented using
React.forwardRefwhich is essential for focus management in keyboard navigation tests. The component accepts all necessary props and usesdata-testidfor reliable test queries.
37-39: Good practice: Mock cleanup in beforeEachClearing mocks before each test ensures test isolation and prevents side effects between tests.
41-63: Clever test helper with history captureThe custom render function with the
HistoryCapturecomponent is an elegant solution for testing router behavior. The pattern allows asserting on navigation changes while maintaining proper router context.
75-115: Comprehensive router-enabled navigation testsThe keyboard navigation tests properly simulate user interactions with
userEvent.keyboard()and verify that router history updates correctly. The test structure withtest.eachis clean and covers both arrow up/down navigation and the edge case of arrow right not navigating.
117-178: Thorough router-disabled navigation testsThe internal navigation tests correctly verify that the
internalSidebarNavigationHandleris called with the expected sidebar view whenrouterDisabledis true. The test structure mirrors the router-enabled tests for consistency and includes proper edge case testing.
81-93: Proper userEvent simulation for keyboard navigationThe use of
userEvent.click()followed byuserEvent.keyboard()correctly simulates the user workflow of focusing the tablist and then pressing keys. This is more realistic than directly dispatching keyboard events.
3 participants
SidebarNav and SidebarNavTablist is now accepting routerDisabled prop and acts accordingly using routing handlers instead of ReactRouter. Tests were converted to RTL and expanded.
Summary by CodeRabbit
New Features
Tests