feat(router): optional router in SIdebarToggle #4160
Conversation
WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SidebarToggle
participant RouterHistory
participant InternalNavHandler
User->>SidebarToggle: Click toggle button
SidebarToggle->>SidebarToggle: handleToggleClick()
alt routerDisabled = false and history present
SidebarToggle->>RouterHistory: replace({ sidebarOpen: toggled })
else routerDisabled = true and internal handler present
SidebarToggle->>InternalNavHandler: handle({ sidebarOpen: toggled }, { replace: true })
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: 3
🧹 Nitpick comments (1)
src/elements/content-sidebar/__tests__/SidebarToggle.test.js (1)
33-87: Consider adding edge case tests for missing props.While the current tests cover the happy path well, consider adding tests for when
routerDisabledis true but required props are missing.Add test cases for edge scenarios:
test('should handle missing internalSidebarNavigationHandler gracefully', async () => { const user = userEvent(); render(<SidebarToggle routerDisabled={true} internalSidebarNavigation={{ sidebar: 'activity' }} />); const toggleButton = screen.getByTestId('sidebartoggle'); await user.click(toggleButton); // Should not throw error and handler should not be called expect(mockInternalSidebarNavigationHandler).not.toHaveBeenCalled(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/elements/content-sidebar/__tests__/__snapshots__/SidebarToggle.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
src/elements/content-sidebar/SidebarToggle.js(1 hunks)src/elements/content-sidebar/__tests__/SidebarToggle.test.js(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/elements/content-sidebar/SidebarToggle.js
[error] 11-11: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 12-19: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 27-27: 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 (7)
src/elements/content-sidebar/SidebarToggle.js (2)
28-42: The toggle logic implementation looks correct.The conditional navigation logic properly handles both router and internal navigation modes. The spread operator correctly preserves existing navigation state while toggling the
openproperty.
33-38: Verify internal navigation handler is always available when routerDisabled is true.The code checks for both
internalSidebarNavigationHandlerandinternalSidebarNavigationbefore calling the handler, but there's no feedback when these are missing in router-disabled mode.Consider adding error handling or warnings when
routerDisabledis true but required props are missing:if (routerDisabled) { // Use internal navigation handler when router is disabled if (internalSidebarNavigationHandler && internalSidebarNavigation) { internalSidebarNavigationHandler({ ...internalSidebarNavigation, open: !isOpen, }, true); // Always use replace for toggle + } else { + console.warn('SidebarToggle: routerDisabled is true but internalSidebarNavigationHandler or internalSidebarNavigation is missing'); } }src/elements/content-sidebar/__tests__/SidebarToggle.test.js (5)
2-2: LGTM! Good migration to React Testing Library.The import update to use React Testing Library and userEvent is a solid improvement over Enzyme for more realistic user interactions.
8-14: Excellent test setup with proper cleanup.The
beforeEachhook for clearing mocks and the render helper function follow React Testing Library best practices and improve test reliability.
16-31: Well-structured parameterized tests for router mode.The test correctly verifies that
history.replaceis called with the toggled state. The parameterized approach efficiently covers both open/closed scenarios.
33-62: Comprehensive coverage for router-disabled mode.The tests properly verify the internal navigation handler is called with the correct parameters including the
replaceflag. Good separation of concerns with dedicated describe block.
64-86: Excellent test for complex navigation state scenarios.This test ensures the spread operator behavior works correctly with complex navigation objects, which is crucial for maintaining existing state while toggling the sidebar.
|
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. |
SidebarToggle updated to use an alternative to react-router when switch is enabled.
Summary by CodeRabbit